sql: fixes for problems found after the consolidation (#45304)

docs/runtime/sql.mdx

@@ -635,7 +635,7 @@ Simple queries cannot use parameters (`${value}`). If you need parameters, split
 These rules apply to every string with several statements, also through `sql.file` and `sql.unsafe`:
 
 - On PostgreSQL and MySQL, the query resolves to an array with one result per statement.
-- On SQLite, every statement runs and the query resolves to one result. It holds the rows of the first statement if that statement returns rows. Otherwise its `count` is the number of rows that all the statements changed. Only the first statement can have parameters.
+- On SQLite, every statement runs and the query resolves to one result. It holds the rows of the first statement if that statement returns rows. Otherwise its `count` is the number of rows that all the statements changed. Only the first statement can have parameters. Bun refuses a later statement that has one when it reaches it, like any other statement that fails.
 - Only PostgreSQL runs the string as one implicit transaction. On MySQL and SQLite, the statements before a failing one stay applied. Wrap the call in [`sql.begin`](#transactions) if you need all or nothing.
 
 ### Queries in files
@@ -718,7 +718,25 @@ What `cancel()` does depends on how far the query got:
 
 ### Unhandled rejections
 
-`.then()` and `.catch()` return a promise like any other. When it rejects and nothing handles the rejection, Bun reports an unhandled rejection, which ends the process unless you listen for [`unhandledRejection`](https://nodejs.org/api/process.html#event-unhandledrejection). So does an error that your `.finally()` callback throws. A query that you only start, with `.execute()` or `.finally()`, is the exception: once it has a connection, its own failure is not reported, and the error is lost. On SQLite, `.execute()` outside a transaction does report it. Add a `.catch()` to a query that you start and do not await:
+`.then()` and `.catch()` return a promise like any other. When it rejects and nothing handles the rejection, Bun reports an unhandled rejection, which ends the process unless you listen for [`unhandledRejection`](https://nodejs.org/api/process.html#event-unhandledrejection). So does an error that your `.finally()` callback throws.
+
+A query that you only start, with `.execute()` or `.finally()`, is the exception. Its own failure is not reported, and the error is lost, when:
+
+- The query had a connection: the server refused it, or the connection was lost.
+- `close({ timeout: 0 })` closed the pool while the query waited for a connection.
+- You called `.cancel()` before you started it.
+- Its reservation was released, or its transaction or savepoint has ended.
+
+After `.execute()`, Bun does report the failure when:
+
+- It cannot connect: nothing listens, the login fails or the connection times out.
+- The pool was already closed.
+- The time of `close({ timeout })`, with a timeout above 0, runs out while the query waits for a connection.
+- You call `.cancel()` while the query waits for a connection.
+- It cannot build the query or read the file of `sql.file()`.
+- The adapter is SQLite and the query runs outside a transaction.
+
+Add a `.catch()` to a query that you start and do not await:
 
 ```ts
 sql`UPDATE stats SET hits = hits + 1`.catch(err => console.error(err));
@@ -1170,6 +1188,7 @@ The adapters differ in two ways:
 The `tx` client belongs to its transaction. Once Bun has sent `COMMIT` or `ROLLBACK`, a query on `tx` never reaches the database. It rejects with `ERR_POSTGRES_INVALID_TRANSACTION_STATE`, or the same code with `MYSQL` or `SQLITE`.
 
 Queries are lazy, so this includes a query that you create inside the callback and first await later. Await every query inside the callback, or return the queries in an array.
+`sql.begin` resolves once the database has answered `COMMIT`. If the connection is lost while `COMMIT` is unanswered, `sql.begin` rejects and nobody can know whether the transaction was committed, so check before you run it again.
 
 ```ts
 let late;
@@ -1372,7 +1391,7 @@ await Promise.all([
 - `sql.begin` and `sql.reserve` need a connection of their own, so they wait for an idle one.
 - Bun serves the callers that wait for a connection oldest first.
 - A pool that lost some of its connections opens new ones when queries have to wait.
-- When Bun cannot open a new connection, callers wait for the connections that are open. They reject with the error of that attempt only when the pool has no connection left.
+- When Bun cannot open a new connection, callers wait for the connections that are open. They reject with the error of that attempt only when the pool has no connection left. A caller that finds every open connection reserved makes Bun try again at once. While an open connection can still take queries, Bun adds at most one connection at a time for the queries that wait, and after each failed attempt it pauses before the next: a few tens of milliseconds at first, then twice as long each time, up to 1 second.
 - Bun closes a connection that goes back to the pool inside a transaction, and the server rolls that transaction back. That covers a reserved connection that you release after a manual `BEGIN`, and a query that leaves a transaction open or failed, such as `SELECT 1; BEGIN`. Queries of other callers that Bun already sent behind such a query are rolled back with it. With `max: 1`, where a manual `BEGIN` is allowed, a query can leave its transaction open.
 
 ### Hot reloading

packages/bun-types/sql.d.ts

@@ -907,9 +907,16 @@ declare module "bun" {
      * What `then()` and `catch()` return is a promise like any other: when it
      * rejects with nothing to handle the rejection, that is reported as an
      * unhandled rejection. So is an error that a `finally()` callback throws.
+     *
      * The failure of a query that was only started, with `execute()` or
-     * `finally()`, is not reported once it has a connection. On SQLite,
-     * `execute()` outside a transaction does report it.
+     * `finally()`, is not reported once it has a connection, nor when
+     * `close({ timeout: 0 })` closes the pool while it waits for one, when it
+     * was cancelled before it started, or when its reservation, transaction or
+     * savepoint has ended. After `execute()`, a failure to connect, a pool that
+     * was already closed, a `cancel()` or the end of the time of
+     * `close({ timeout })`, with a timeout above 0, while it waits for a
+     * connection, and a query that cannot be built are reported. So is the
+     * failure of a query that SQLite runs outside a transaction.
      *
      * Bun reads the parameters when it sends the query. Leave a `Buffer`,
      * `TypedArray`, `Date` or object that you passed alone until the query

src/js/bun/sql.ts

@@ -102,6 +102,8 @@ class Scope implements Target {
   /// The caller's statements pass. close() and release() clear it in the call, COMMIT and ROLLBACK where they are handed over.
   accepting = true;
   finishing: Promise<any> | null = null;
+  /// The callback is over and run() waits for the answer to the end. Only that answer settles the transaction.
+  awaitingAnswer = false;
   error: Error | null = null;
   running = new Set<Query>();
   children = new Set<Scope>();
@@ -209,6 +211,10 @@ function onQueryConnected(
 ) {
   query[_waiter] = null;
   if (err) {
+    // The caller dropped it while it waited. As in onQuerySettled(): one that is only started ends no process.
+    if (this.closedAtOnce) {
+      $pokePromiseAsHandled(query);
+    }
     return query.reject(err);
   }
   pooledConnection!.bindQuery?.(query);
@@ -217,6 +223,9 @@ function onQueryConnected(
 
 function dispatchToPool(this: Client, query: Query, handle: QueryHandle) {
   const pool = this.pool;
+  if (pool.closed) {
+    return query.reject(pool.connectionClosedError());
+  }
   pool.connect((query[_waiter] = onQueryConnected.bind(pool, query, handle)));
 }
 
@@ -262,6 +271,8 @@ function dispatch(this: Scope, query: Query, handle: QueryHandle) {
     // SQLite ends a transaction inside some failing statements. What follows would run in autocommit.
     (this.kind !== Kind.reservation && this.pool.isInTransaction?.(this.connection) === false)
   ) {
+    // as in onQuerySettled()
+    $pokePromiseAsHandled(query);
     return query.reject(refusal(this));
   }
   start(this, query, handle);
@@ -396,7 +407,7 @@ function abandon(scope: Scope, err: Error | null) {
   }
   abandonChildren(scope, err);
   // savepoint() settles when its callback does.
-  if (scope.kind === Kind.transaction) {
+  if (scope.kind === Kind.transaction && !scope.awaitingAnswer) {
     const failure = scope.failure;
     settle(scope, scope.reject, failure !== null && err !== null ? withCause(err, failure.error) : refusal(scope));
   }
@@ -562,6 +573,7 @@ async function run(scope: Scope, callback: TransactionCallback) {
   }
 
   let answer: { command?: string | null } | undefined;
+  scope.awaitingAnswer = true;
   try {
     answer = await finish(scope, failure === null, undefined);
   } catch (err) {

src/js/internal/sql/options.ts

@@ -12,11 +12,20 @@ type AnyOptions = Bun.SQL.__internal.AnyOptions;
 type TLSOptions = Bun.SQL.TLSOptions;
 
 const ObjectDefineProperty = Object.defineProperty;
+const ObjectGetPrototypeOf = Object.getPrototypeOf;
 const ObjectHasOwn = Object.hasOwn;
+const ObjectPrototype = Object.prototype;
 const kInspectCustom = Symbol.for("nodejs.util.inspect.custom");
 const env = Bun.env;
 
+/** `Bun.env` inherits from `Object.prototype`, and what is there is no variable. */
+function envVar(name: string): string | undefined {
+  return ObjectHasOwn(env, name) ? env[name] : undefined;
+}
+
 const SQLITE_MEMORY = ":memory:";
+const REDACTED = "[REDACTED]";
+const SCHEME = /^[a-z][a-z0-9+.-]*$/i;
 const SUPPORTED_ADAPTERS = `"postgres", "sqlite", "mysql", "mariadb"`;
 
 function decodeIfValid(value: string | null | undefined): string | null {
@@ -143,7 +152,7 @@ function urlFromEnvironment(
 
     for (let i = 1; i < row.length; i++) {
       const name = row[i] as string;
-      const url = env[name];
+      const url = envVar(name);
       if (!url) continue;
 
       const tls = name.startsWith("TLS_");
@@ -220,10 +229,17 @@ const SERVER_OPTIONS = [
   "onclose",
 ];
 
+/** What somebody put on `Object.prototype` is not the caller's. What the caller's own prototypes have is. */
+function isFromObjectPrototype(object: object | null, name: string): boolean {
+  if (!ObjectHasOwn(ObjectPrototype, name)) return false;
+  while (object && object !== ObjectPrototype && !ObjectHasOwn(object, name)) object = ObjectGetPrototypeOf(object);
+  return !object || object === ObjectPrototype;
+}
+
 /** `options` has the own keys of `source` already. Every key is read once: a getter is the caller's code. */
 function readInherited(options: AnyOptions, source: AnyOptions, names: string[]) {
   for (const name of names) {
-    if (ObjectHasOwn(options, name)) continue;
+    if (ObjectHasOwn(options, name) || isFromObjectPrototype(source, name)) continue;
     const value = source[name];
     if (value !== undefined) options[name] = value;
   }
@@ -245,7 +261,7 @@ function resolveTarget(
     first = undefined;
   }
   const readsEnvironment = target === null;
-  const options: AnyOptions = { ...first, ...second };
+  const options = { __proto__: null, ...first, ...second } as unknown as AnyOptions;
   // An options object that stands alone behind a URL, or behind nothing, also gives what it inherits.
   const inherits = first ? null : second;
   if (inherits) readInherited(options, inherits, TARGET_OPTIONS);
@@ -298,7 +314,9 @@ function resolveTarget(
         ? "postgres"
         : adapterFromScheme(scheme);
     if (!adapter) {
-      throw new Error(`Unsupported protocol: ${scheme}. Supported adapters: ${SUPPORTED_ADAPTERS}`);
+      // Without a scheme, what is in front of a `://` further back can be anything.
+      const named = SCHEME.test(scheme) ? scheme : REDACTED;
+      throw new Error(`Unsupported protocol: ${named}. Supported adapters: ${SUPPORTED_ADAPTERS}`);
     }
   }
 
@@ -375,11 +393,20 @@ function parseSQLiteOptions(target: string | URL | null, options: AnyOptions): B
   const safeIntegers = options.safeIntegers ?? options.bigint;
   if (safeIntegers !== undefined) sqliteOptions.safeIntegers = safeIntegers;
 
-  if (sqliteOptions.transform != null) sqliteOptions.transform = parseTransform(sqliteOptions.transform);
+  const transform = options.transform;
+  if (transform != null) sqliteOptions.transform = parseTransform(transform);
 
   return sqliteOptions;
 }
 
+/** What an error may say of a connection URL that cannot be parsed: anything behind the scheme can be a password. */
+function withoutSecrets(value: string): string {
+  const start = value.indexOf("://") + 3;
+  // The `://` is further back, and anything can be in front of it.
+  if (!SCHEME.test(value.slice(0, start - 3))) return REDACTED;
+  return value.slice(0, start) + (value.startsWith(SQLITE_MEMORY, start) ? SQLITE_MEMORY : REDACTED);
+}
+
 function parseServerUrl(
   target: string | URL,
   adapter: Adapter,
@@ -392,7 +419,7 @@ function parseServerUrl(
     const value = target.includes("://") ? target : `${adapter}://${target}`;
     try {
       url = new URL(value);
-    } catch (e) {
+    } catch {
       // `new URL()` refuses credentials or a port without a host. libpq takes the host from elsewhere.
       const start = value.indexOf("://") + 3;
       let end = start;
@@ -403,13 +430,18 @@ function parseServerUrl(
         if (withHost) return [withHost, ""];
       }
 
+      // The error of `new URL()` has the whole string.
+      const input = withoutSecrets(value);
       if (adapterOption && value.includes("sqlite")) {
         throw new Error(
-          `Invalid URL '${value}' for ${adapterOption}. Did you mean to specify \`{ adapter: "sqlite" }\`?`,
-          { cause: e },
+          `Invalid URL '${input}' for ${adapterOption}. Did you mean to specify \`{ adapter: "sqlite" }\`?`,
         );
       }
-      throw e;
+      // The error of `new URL()` in all but `input`. The intrinsic for this code has `code` on a prototype of its own.
+      const error = new TypeError("Invalid URL");
+      $putByValDirect(error, "code", "ERR_INVALID_URL");
+      $putByValDirect(error, "input", input);
+      throw error;
     }
   }
 
@@ -491,14 +523,14 @@ function isLibpqClientKeyword(key: string, value: string): boolean {
 function setStartupParameter(parameters: Record<string, string>, name: string, key: string, raw: unknown) {
   const value = `${raw}`;
   if (key.includes("\0") || value.includes("\0")) {
-    throw $ERR_INVALID_ARG_VALUE(name, raw, "must not contain null bytes");
+    throw $ERR_INVALID_ARG_VALUE(name, REDACTED, "must not contain null bytes");
   }
   parameters[key] = value;
 }
 
 function readUrlQuery(url: URL, isPostgres: boolean, parameters: Record<string, string>): UrlQuery {
   const query: UrlQuery = { __proto__: null };
-  const queryObject = url.searchParams.toJSON();
+  const queryObject = { __proto__: null, ...url.searchParams.toJSON() };
   for (const key in queryObject) {
     const value = `${queryObject[key]}`;
     const lowerKey = key.toLowerCase();
@@ -519,7 +551,7 @@ function readUrlQuery(url: URL, isPostgres: boolean, parameters: Record<string,
       if (value) query[option] ||= value;
     } else if (isPostgres && (option = LIBPQ_TLS_OPTIONS[lowerKey])) {
       if (!value) continue;
-      const tls = $isObject(query.tls) ? query.tls : (query.tls = {});
+      const tls = $isObject(query.tls) ? query.tls : (query.tls = { __proto__: null } as unknown as TLSOptions);
       if (lowerKey === "sslrootcert" && value === "system") tls.rejectUnauthorized = true;
       else tls[option] = value;
     } else if (!isPostgres || !isLibpqClientKeyword(lowerKey, value)) {
@@ -532,7 +564,7 @@ function readUrlQuery(url: URL, isPostgres: boolean, parameters: Record<string,
 const NO_QUERY: UrlQuery = { __proto__: null };
 
 function envOf(prefix: string, name: string): string | undefined {
-  return env[`${prefix}_${name}`] || env[prefix + name];
+  return envVar(`${prefix}_${name}`) || envVar(prefix + name);
 }
 
 function isDirectory(path: string): boolean {
@@ -581,11 +613,11 @@ function parseTransform(transform: any): import("./shared").Transform {
 
 function inspectOptions(this: Bun.SQL.__internal.DefinedPostgresOrMySQLOptions) {
   const copy = { ...this };
-  if (copy.password) copy.password = "[REDACTED]";
+  if (copy.password) copy.password = REDACTED;
   if ($isObject(copy.tls)) {
     const tls = { ...(copy.tls as TLSOptions) };
-    if (tls.key) tls.key = "[REDACTED]";
-    if (tls.passphrase) tls.passphrase = "[REDACTED]";
+    if (tls.key) tls.key = REDACTED;
+    if (tls.passphrase) tls.passphrase = REDACTED;
     copy.tls = tls;
   }
   return copy;
@@ -619,7 +651,8 @@ function parseOptions(
 
   const parameters = { __proto__: null } as unknown as Record<string, string>;
   if (isPostgres) {
-    const { PGAPPNAME, PGOPTIONS } = env;
+    const PGAPPNAME = envVar("PGAPPNAME");
+    const PGOPTIONS = envVar("PGOPTIONS");
     if (PGAPPNAME) parameters.application_name = PGAPPNAME;
     if (PGOPTIONS) parameters.options = PGOPTIONS;
   }
@@ -638,6 +671,7 @@ function parseOptions(
   const connection = options.connection;
   if (connection && $isObject(connection)) {
     for (const key in connection) {
+      if (isFromObjectPrototype(connection, key)) continue;
       if (connection[key] !== undefined) {
         setStartupParameter(parameters, `options.connection.${key}`, key, connection[key]);
       }
@@ -664,10 +698,10 @@ function parseOptions(
     options.user ||
     query.username ||
     decodeIfValid(url?.username) ||
-    (isPostgres && env.PGUSERNAME) ||
+    (isPostgres && envVar("PGUSERNAME")) ||
     envOf(prefix, "USER") ||
-    env.USER ||
-    env.USERNAME ||
+    envVar("USER") ||
+    envVar("USERNAME") ||
     (isPostgres ? "postgres" : "root");
 
   const password =
@@ -714,7 +748,7 @@ function parseOptions(
       options.connectTimeout ??
       options.connect_timeout ??
       query.connectionTimeout ??
-      ((isPostgres && env.PGCONNECT_TIMEOUT) || undefined),
+      ((isPostgres && envVar("PGCONNECT_TIMEOUT")) || undefined),
   );
   const maxLifetime = secondsToMilliseconds(
     "options.max_lifetime",
@@ -733,7 +767,7 @@ function parseOptions(
 
   const channelBinding = (options.channelBinding ||
     query.channelBinding ||
-    (isPostgres && env.PGCHANNELBINDING) ||
+    (isPostgres && envVar("PGCHANNELBINDING")) ||
     "disable") as Bun.SQL.__internal.ChannelBinding;
   if (channelBinding !== "disable" && channelBinding !== "prefer" && channelBinding !== "require") {
     throw $ERR_INVALID_ARG_VALUE("channelBinding", channelBinding, "must be one of: disable, prefer, require");