fix: prevent migration timeout overrides

pg-connection-string retains arbitrary query keys, so the driver's own configuration key connectionTimeoutMillis reaches ConnectionParameters straight from a URL — and it was not on the rejected list, while spellings that do nothing were. Rejecting it closes that gap. The precedence claims that guard was built on were partly wrong, so every source was measured against the installed [email protected].0, a real blackhole listener, and a real PostgreSQL rather than inferred from the merge: statement_timeout=0 / =900000 server reported 0 and 15min — wins outright query_timeout=0 overrides the field, but the parsed value is the string "0", which is truthy, so every query gets a 0 ms read timer and fails nondeterministically — it breaks the client rather than loosening the bound connectionTimeoutMillis=0/900000 pollutes connectionParameters.connect_timeout (10 -> 0 or 900 s), but that field only feeds getLibpqConnectionString(), which the pure-JS client never calls; the effective timer was unchanged, with an explicit 500 ms still expiring at ~500 ms either way connect_timeout=0 no effect: recomputed from the explicit field options=-c statement_timeout=… no effect, including a backslash-escaped spelling — the named startup parameter wins PGOPTIONS / PGCONNECT_TIMEOUT no effect unrelated parameters no effect, deliberately untouched Only statement_timeout is a genuine downgrade. The rest are rejected as unsupported configuration and defense in depth: no legitimate DATABASE_URL carries a driver option, the polluted connect_timeout would take effect on the native binding, and query_timeout produces intermittent failures an operator could not diagnose. That distinction is now stated where it was previously asserted wrongly — the earlier comment and ADR text claimed connect_timeout overrode the explicit field, which measurement disproved. Rejection still happens at the one shared construction boundary, before any pg.Client exists, with the existing fixed POSTGRES_MIGRATION_CONFIGURATION_INVALID and no URL, host, port, credential, parameter, or value in the message. The 10 s defaults, narrower test overrides, Pass 2 TLS policy and its URL-conflict checks, and every unrelated parameter are unchanged. Co-Authored-By: Claude Opus 5 <[email protected]>
2026-07-28 19:01:46 -05:00
parent 74e8840f0d
commit 6912680113
5 changed files with 363 additions and 24 deletions
@@ -144,15 +144,32 @@ lock-wait deadline and error codes, and the Pass 2 TLS/hostname/certificate poli
 as are Doctor's and the bootstrap CLI's own timeouts. No timeout environment variable or flag was
 added.

-Explicit constructor fields do **not** win under the pinned driver. `[email protected]` builds parameters as
-`Object.assign({}, config, parse(config.connectionString))`, so the connection string overrides
-passed options — verified directly: an explicit `statement_timeout: 10000` alongside
-`?statement_timeout=0` produced a server-reported `statement_timeout` of `0`, while a clean URL
-produced `10s`. A `DATABASE_URL` carrying `connect_timeout`, `query_timeout`, `statement_timeout`,
-or an `options` value that sets `statement_timeout` is therefore refused with the existing fixed
-`POSTGRES_MIGRATION_CONFIGURATION_INVALID`, before any client is constructed and without echoing the
-URL, parameter, or value. The rejection is deliberately narrow — only settings that actually conflict
-with the selected policy; unrelated parameters are untouched.
+A connection string can reach those values, because `[email protected]` builds parameters as
+`Object.assign({}, config, parse(config.connectionString))` and `pg-connection-string` retains
+*arbitrary* query keys. What each key actually does was measured against the installed driver, a
+real blackhole listener, and a real PostgreSQL rather than inferred from that merge, and only one of
+them genuinely weakens the policy:
+
+| Source | Measured effect on the selected policy |
+| --- | --- |
+| `?statement_timeout=0` / `=900000` | **Wins.** Server reported `0` (disabled) and `15min` against an explicit `statement_timeout: 10000`. The one real downgrade. |
+| `?query_timeout=0` | Overrides the explicit field, but breaks the client rather than loosening it: the parsed value is the string `"0"`, which is truthy, so every query gets a 0 ms read timer and fails nondeterministically with `Query read timeout`. |
+| `?connectionTimeoutMillis=0` / `=900000` | Pollutes `ConnectionParameters.connect_timeout` (10 → `0`/`900` s), but that field feeds only `getLibpqConnectionString()`, which the pure-JS client never calls. The **effective** connect timer is unchanged — an explicit 500 ms still expired at ~500 ms with either value. |
+| `?connect_timeout=0` | No effect: `ConnectionParameters` recomputes it from `connectionTimeoutMillis`. |
+| `?options=-c statement_timeout=…` (incl. a backslash-escaped spelling) | No effect: the named startup parameter wins. Server still reported `10s`. |
+| inherited `PGOPTIONS=-c statement_timeout=0` | No effect. Server still reported `10s`. |
+| inherited `PGCONNECT_TIMEOUT=0` | No effect: consulted only when `connectionTimeoutMillis` is `undefined`, which this boundary never leaves unset. |
+| unrelated parameters (`application_name`, …) | No effect; deliberately untouched. |
+
+A `DATABASE_URL` carrying `statement_timeout`, `query_timeout`, `connectionTimeoutMillis`,
+`connect_timeout`, or an `options` value naming `statement_timeout` is refused with the existing
+fixed `POSTGRES_MIGRATION_CONFIGURATION_INVALID`, before any client is constructed and without
+echoing the URL, parameter, or value. Only the first is a precedence fix; the rest are rejected as
+**unsupported configuration and defense in depth** — no legitimate `DATABASE_URL` carries a driver
+option, `connectionTimeoutMillis` would take effect on the native binding, and `query_timeout`
+produces intermittent failures an operator would struggle to diagnose. Stating that honestly matters:
+an earlier revision of this ADR claimed `connect_timeout` overrode the explicit field, which
+measurement disproved.

 ### Each migration is one PostgreSQL-parsed batch