Skip to content

fix(driver-sql): give redshift its connect-timeout row so the 10s dialect bound applies - #11836

Merged
huangyiirene merged 1 commit into
mainfrom
claude/issue-11784-redshift-connect-timeout-row
Aug 24, 2026
Merged

fix(driver-sql): give redshift its connect-timeout row so the 10s dialect bound applies#11836
huangyiirene merged 1 commit into
mainfrom
claude/issue-11784-redshift-connect-timeout-row

Conversation

@huangyiirene

Copy link
Copy Markdown
Collaborator

Closes#11784

What was measured

SqlDriver answers three separate questions about a knex client name from three separate tables. redshift was a member of the wire table and absent from the connect-timeout table:

questiontableredshift before
whose SQL do I emit?the dialect-identity sets behind isPostgres / isSqlite / isMysqlabsent (deliberate, open decision)
which npm package parses the wire?POSTGRES_WIRE_CLIENTSpresent (calendar-day parser pin)
how do I bound ONE connection attempt?DIALECT_CONNECT_TIMEOUTabsent ← the defect

So withConnectBound injected no connectionTimeoutMillis for a redshift datasource, and the attempt fell through to the strictly looser 15s pool.createTimeoutMillis backstop — while DEFAULT_CONNECT_TIMEOUT_MS's own docblock calls 10s "the effective bound", with the pool value "a strictly looser backstop, reached only by a dialect that has no connect-timeout knob (SQLite) or ignores the one we set".

A redshift host is neither of those, and this was verified rather than inherited from the card: knex's Client_Redshift literally extends Client_PG (knex/lib/dialects/redshift/index.js), inheriting acquireRawConnection, so the settings object reaches pg.Client, which honours connectionTimeoutMillis. It had the knob and would have obeyed it; it just never received it.

The before-state is silence — nothing errors, nothing is logged, the bound is simply 50% looser for one client name. That is why no test here asserts "it works": the assertions pin the injected knex config.

What changed

One row, added as a literal extension of POSTGRES_EMIT_CLIENTS in exactly the shape cockroachdb already had:

- ...[...SqlDriver.POSTGRES_EMIT_CLIENTS, 'cockroachdb'].map(+ ...[...SqlDriver.POSTGRES_EMIT_CLIENTS, 'cockroachdb', 'redshift'].map(

What was deliberately NOT changed

⚠️ One consequence worth flagging for review: the pg arm of DIALECT_CONNECT_TIMEOUT now holds the same five names as POSTGRES_WIRE_CLIENTS. That is a coincidence of today's membership, not an invariant, and spelling it as ...POSTGRES_WIRE_CLIENTS would hand redshift and cockroachdb SQL-emission identity as a silent refactor side effect. Both the table comment and the #11550 membership test now say so explicitly.

Surface check, per the dispatch: this moves a connect-timeout bound, not a contract's accept/reject surface and not a public API surface. DIALECT_CONNECT_TIMEOUT is a private static readonly; no exported type, error code or request verdict moves. The behaviour delta is that a redshift connection attempt which would previously have completed between 10s and 15s now fails at 10s with pg's accurate timeout expired instead of knex's misleading "the pool is probably full".

The load-bearing comment — repaired, not deleted

redshift's absence from this table was load-bearing as documentation in two places, and a third pinned it as a test fixture:

  1. DIALECT_CONNECT_TIMEOUT's own comment said "redshift still carries no entry; see the note in withConnectBound for why that absence is load-bearing." — rewritten.
  2. The note inside withConnectBound cited it by name as the measured reason its early return must not skip the session pins: "a return placed at this point silently opted it out of a fix it needs" — rewritten.
  3. sql-driver-11550-…test.ts's membership pin repeated the claim in its comment — rewritten.

The note is repaired, not dropped, and it keeps both halves. The retired measurement is recorded as retired (redshift really was in POSTGRES_WIRE_CLIENTS and really was absent here, so an early return really did opt it out — that happened, and deleting it would lose it). Then the hazard is restated without depending on the example: every client that needs a session pin happens to have a timeout row today, which is a fact about current membership and not a property either table promises, so the next pg-wire client added without a connect-timeout knob restores the exact bug — silently, with every test green.

Because there is no longer a client that demonstrates the hazard by observation, the guard is a tripwire rather than a behavioural assertion: it computes (POSTGRES_WIRE_CLIENTS ∪ MYSQL_EMIT_CLIENTS) \ keys(DIALECT_CONNECT_TIMEOUT) and goes red the moment that set is non-empty, with a failure message telling the author the divergence is allowed but that they must confirm the fall-through is intact and refresh the note with the live example they just created.

Fixture triage

Three fixtures pinned the old state; each was re-judged individually rather than respelled:

Ablation

Predicted direction stated in advance: RED, naming the 5 cases expected to fail. Mutation proven on disk before any test result was read, by anchored grep counts on the text actually being changed, both directions:

BEFORE mutation: with-redshift 1 without-redshift 0
AFTER mutation: with-redshift 0 without-redshift 1

Restore ran under trap … EXIT INT TERM; verified on disk afterwards (with-redshift 1 / without-redshift 0) with git status --porcelain empty.

Result — exactly the 5 predicted cases, no more, no fewer:

ABLATION_TEST_EXIT=1
Test Files 3 failed (3)
Tests 5 failed | 56 passed | 3 skipped (64)
× is not gated on the connect-timeout table — the two lists are not the same list
× tripwire: a session-pinned client with no connect-timeout row re-arms the early return
× redshift and cockroachdb parse pg wire but do NOT emit Postgres DDL
× membership is the derivation plus its literal extensions, and nothing else
× bounds a redshift connect attempt at the dialect timeout, not the pool backstop (#11784)

No rebuild was performed, and that is a measurement rather than an assumption: these tests import ./sql-driver.js, a relative specifier vitest resolves to src/, with no alias in packages/drivers/driver-sql/vitest.config.ts redirecting it. A dist/-resolved subject would have stayed green — the dangerous direction — so an observed RED is itself the proof that the tests read source. A green ablation would have been rebuilt and re-run rather than believed.

Honest note on what the ablation does not discriminate: leaves a redshift host's own explicit connect timeout alone stays green under the mutation, because with no row nothing is injected and the host's 60s survives either way. It is a companion assertion, not a pin on this change.

Verification

All at the pushed commit e2db65e58e; the tree was clean and unchanged from that commit through every run below. Exit codes captured before any pipe (direct redirect + set -o pipefail), and each gate's own printed verdict line is quoted rather than a bare $?.

  • pnpm --filter @objectstack/driver-sql testTEST_EXIT=0Test Files 126 passed | 8 skipped (134), Tests 1970 passed | 110 skipped (2080)
  • pnpm --filter @objectstack/driver-sql typecheckTYPECHECK_EXIT=0
  • pnpm lint (full repo, eslint . --no-inline-config) → LINT_FULL_EXIT=0 — run whole, so no narrowing is claimed
  • Derived gate union via node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack — 14 path-derived + 6 convention-triggered families, all 21 run, all exit 0 (with check:nul-bytes), including:
    • check-driver-conformance: OK — 45 covered cell(s), 0 in the DEBT ledger, 0 exempt.
    • check-engine-double-contract: OK — 401 pinned, 133 in the DEBT ledger, 2 exempt.
    • check-nul-bytes: OK (scanned 6592 text file(s) … no raw ASCII control bytes)
    • check-type-check-coverage --re-measure: OK — 32 ledger entr(ies) re-measured … none above its recorded number.
    • where-matcher conformance holds: 295 matcher(s) discovered … 0 silently-wrong
    • query-options-erasure ratchet holds … none new

Driver-conformance ledger, before and after (lane standing promise): 45 covered / 0 DEBT / 0 exempt45 covered / 0 DEBT / 0 exempt. Unchanged; nothing added to the ledger.

No reverse-verification of a cross-package type change is reported, because there is none: the change is a private static table inside one package and adds no exported type.


Generated by Claude Code

…ialect bound applies
`SqlDriver` answers three questions about a knex `client` name from three
tables. `redshift` was in `POSTGRES_WIRE_CLIENTS` (#11389 put it there for the
calendar-day parser pin) but absent from `DIALECT_CONNECT_TIMEOUT`, so
`withConnectBound` injected no `connectionTimeoutMillis` and the attempt fell
through to the strictly looser 15s `pool.createTimeoutMillis` backstop — while
the method's own docblock calls 10s "the effective bound", reached past only by
a dialect with no such knob (SQLite) or one that ignores it. Redshift is
neither: knex's `Client_Redshift extends Client_PG`, so `pg` honours the knob.
Nothing errored and nothing was logged; the bound was just 50% looser.
The row is added as a literal extension of `POSTGRES_EMIT_CLIENTS`, exactly as
`cockroachdb` already was — no table is merged, converged or restructured, and
`POSTGRES_WIRE_CLIENTS` and the dialect getters are untouched. No SQL-emission
identity is granted.
`redshift`'s absence from that table was load-bearing as documentation: the note
inside `withConnectBound` cited it as the measured reason its early return must
not skip the session pins. Adding the row retires that example, so the note is
rewritten to record the retired measurement and to carry the reasoning directly,
and the fixtures that pinned the absence are re-judged rather than respelled —
including a tripwire that goes red the moment the two memberships diverge again.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VK8rFDtg8eREaxBGX99Csn
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

2 anchor(s) derived from 1 changed package(s); no hand-written page names any of them. ✅

What this run could not see
  • the SDK route bridge reached 45 of 222 client-bound route-ledger rows — the other 177 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run: node scripts/docs-audit/affected-docs.mjs --bridge-coverage

Coarse fallback — 9 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json bb418686547de42c8feae103fa2e41dd419aa171packageMentionDocs.

Which tree this was computed on

This run read content/docs from f105dd6e3e11084695100b8cbb506fa65a26a065 — the merge of head e2db65e58e842cd41a1ffb5a9f7ccca4935c31fc into base bb418686547de42c8feae103fa2e41dd419aa171, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin f105dd6e3e11084695100b8cbb506fa65a26a065 && git checkout f105dd6e3e11084695100b8cbb506fa65a26a065
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin bb418686547de42c8feae103fa2e41dd419aa171 e2db65e58e842cd41a1ffb5a9f7ccca4935c31fc && git checkout -B drift-repro bb418686547de42c8feae103fa2e41dd419aa171 && git merge --no-ff e2db65e58e842cd41a1ffb5a9f7ccca4935c31fc
node scripts/docs-audit/affected-docs.mjs --json bb418686547de42c8feae103fa2e41dd419aa171

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation tests tooling labels Aug 24, 2026
@huangyiirene
huangyiirene marked this pull request as ready for review August 24, 2026 19:54
@huangyiirene
huangyiirene added this pull request to the merge queueAug 24, 2026
Merged via the queue into main with commit 6757eb2Aug 24, 2026
32 checks passed
@huangyiirene
huangyiirene deleted the claude/issue-11784-redshift-connect-timeout-row branch August 24, 2026 20:10
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/mteststooling

Projects

None yet

2 participants

@huangyiirene@claude