Skip to content

Commit 6757eb2

Browse files
huangyiireneclaude
andauthored
fix(driver-sql): give redshift its connect-timeout row so the 10s dialect bound applies (#11836)
`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. Claude-Session: https://claude.ai/code/session_01VK8rFDtg8eREaxBGX99Csn Co-authored-by: Claude <noreply@anthropic.com>
1 parent 9a884c6 commit 6757eb2

5 files changed

Lines changed: 146 additions & 25 deletions

File tree

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@objectstack/driver-sql": patch
3+
---
4+
5+
A `redshift` datasource now gets the 10s dialect connect-timeout bound instead of silently degrading to the 15s pool backstop (#11784). `SqlDriver` answers three separate questions about a knex `client` name from three separate tables, and `redshift` was a member of the wire-protocol one (`POSTGRES_WIRE_CLIENTS`, which #11389 put it in so it gets the calendar-day parser pin) while absent from `DIALECT_CONNECT_TIMEOUT`. It reaches the server through the `pg` driver — knex's `Client_Redshift` literally `extends Client_PG` — so it has `connectionTimeoutMillis` and would have obeyed it; it just never received it, and `withConnectBound` skipped the injection. Nothing errored and nothing was logged: the bound was simply 50% looser than the method's own docblock declares ("the effective bound" at 10s, with `pool.createTimeoutMillis` 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. The practical consequence is the framework#3769 failure shape — an endpoint that accepts the TCP connection and never completes the handshake makes every query WAIT rather than fail, and the wait was bounded 5s later than declared, with knex's inaccurate "the pool is probably full" wording instead of pg's `timeout expired`. A host that sets its own `connectionTimeoutMillis` or `pool.createTimeoutMillis` is still left alone. `redshift` gains **no** SQL-emission identity from this: the connect-timeout knob is a property of the npm driver doing the connecting, not of which DDL dialect gets compiled, so this is independent of the open support-scope decision (#11756).

‎packages/drivers/driver-sql/src/sql-driver-11389-date-tz-skew.test.ts‎

Lines changed: 28 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -211,20 +211,40 @@ describe('#11389 — a Postgres `date` never becomes a JS Date', () => {
211211
});
212212

213213
it('is not gated on the connect-timeout table — the two lists are not the same list', async () => {
214-
// `redshift` speaks the pg wire protocol but has no connect-timeout knob,
215-
// so it is absent from DIALECT_CONNECT_TIMEOUT. Measured while fixing
216-
// #11389: with the session pins reached only through that table's early
217-
// return, redshift silently opted out of a fix it needs. This asserts the
218-
// two concerns really are independent — no timeout injected, pin applied.
214+
// ⚠️ This case used to read `redshift`'s ABSENCE from DIALECT_CONNECT_TIMEOUT
215+
// as its demonstration: pin applied, no timeout injected, therefore the two
216+
// concerns are independent. #11784 gave redshift the row it was missing, so
217+
// that vehicle is gone. The timeout assertion is INVERTED here rather than
218+
// deleted, because the fact this case exists to pin is #11389's — a pg-wire
219+
// client reaches the calendar-day parser hook — and that is unchanged.
219220
const rec = recordingPgConnection();
220221
const driver = make({ client: 'redshift', connection: 'postgres://u:p@host:5439/d' });
221-
// knex parses a URL connection into its own object either way, so the
222-
// readable signal is that no timeout key was injected into it.
223-
expect((driver as any).knex.client.config.connection.connectionTimeoutMillis).toBeUndefined();
222+
expect((driver as any).knex.client.config.connection.connectionTimeoutMillis).toBe(10_000);
224223
await runAfterCreate(driver, rec.connection);
225224
expect([...rec.registered.keys()].sort((a, b) => a - b)).toEqual([OID_DATE, OID_DATE_ARRAY]);
226225
});
227226

227+
it('tripwire: a session-pinned client with no connect-timeout row re-arms the early return', () => {
228+
// The hazard `withConnectBound`'s note describes needs a client that needs a
229+
// session pin but has no timeout row. #11784 removed the last one, so there
230+
// is nothing to observe — which is exactly why this is a TRIPWIRE and not an
231+
// assertion about behaviour. It goes red the moment someone adds such a
232+
// client, and points them at the note before they "simplify" the
233+
// fall-through into an early return that would silently opt it out.
234+
const timeoutTable = Object.keys((SqlDriver as any).DIALECT_CONNECT_TIMEOUT);
235+
const sessionPinned = [
236+
...((SqlDriver as any).POSTGRES_WIRE_CLIENTS as ReadonlySet<string>), // #11389 calendar-day pin
237+
...((SqlDriver as any).MYSQL_EMIT_CLIENTS as ReadonlySet<string>), // #3942 UTC session pin
238+
];
239+
expect(
240+
sessionPinned.filter((c) => !timeoutTable.includes(c)),
241+
'A client that needs a session pin now has no DIALECT_CONNECT_TIMEOUT row. That is ' +
242+
'ALLOWED — but it re-arms the hazard documented inside withConnectBound: the session ' +
243+
'pins must stay reachable when `dialect` is falsy. Confirm the fall-through is intact, ' +
244+
'then update this expectation and refresh that note with the live example you just made.',
245+
).toEqual([]);
246+
});
247+
228248
it('chains a host-supplied afterCreate rather than replacing it', async () => {
229249
const seen: string[] = [];
230250
const rec = recordingPgConnection();

‎packages/drivers/driver-sql/src/sql-driver-11550-dialect-client-spellings.test.ts‎

Lines changed: 22 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -241,6 +241,14 @@ describe('#11550 — emission identity is not wire identity (#11756 stays open)'
241241
expect(d.nowDefaultSql('date').toUpperCase(), spelling).toContain('CURRENT_TIMESTAMP');
242242
// …while still getting the #11389 calendar-day wire hook.
243243
expect(emit('POSTGRES_WIRE_CLIENTS'), spelling).toContain(spelling);
244+
// …and, since #11784, the connect-timeout bound too. Both are properties
245+
// of the npm driver doing the connecting (`pg`), which is why neither
246+
// waits on #11756: knex's Client_Redshift/Client_CockroachDB extend
247+
// Client_PG for the WIRE while overriding the query compiler for
248+
// EMISSION. Timeout row present, emission identity still false — the
249+
// independence #11784 asserted, pinned rather than argued.
250+
expect((SqlDriver as any).DIALECT_CONNECT_TIMEOUT[spelling], spelling)
251+
.toEqual({ key: 'connectionTimeoutMillis', urlKey: 'connectionString' });
244252
}
245253
});
246254

@@ -275,12 +283,21 @@ describe('#11550 — the client-keyed tables now derive from one source', () =>
275283
}
276284
});
277285

278-
it('deriving the table changed no membership', () => {
279-
// The refactor half must be a no-op. `redshift`'s ABSENCE is load-bearing —
280-
// `withConnectBound`'s comment cites it as the reason its early return may
281-
// not skip the session pins.
286+
it('membership is the derivation plus its literal extensions, and nothing else', () => {
287+
// #11550's refactor half had to be a no-op, and this pinned that. #11784
288+
// then added `redshift` — a real membership change, deliberate, and the only
289+
// one since. The comment that stood here cited redshift's ABSENCE as
290+
// load-bearing documentation for `withConnectBound`'s early-return note;
291+
// that example was retired together with the row, and the note now carries
292+
// its reasoning directly instead of leaning on this table.
293+
//
294+
// ⚠️ `cockroachdb` and `redshift` must stay LITERAL extensions of
295+
// POSTGRES_EMIT_CLIENTS here. This list now coincides with
296+
// POSTGRES_WIRE_CLIENTS, and spelling it as that set instead would grant
297+
// both of them SQL-emission identity as a silent refactor side effect —
298+
// which is #11756's open decision, not this table's.
282299
expect(Object.keys(table()).sort()).toEqual(
283-
['cockroachdb', 'mysql', 'mysql2', 'pg', 'postgres', 'postgresql'],
300+
['cockroachdb', 'mysql', 'mysql2', 'pg', 'postgres', 'postgresql', 'redshift'],
284301
);
285302
});
286303

‎packages/drivers/driver-sql/src/sql-driver-connect-bound.test.ts‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,55 @@ describe('SqlDriver — connection-attempt bound (framework#3769)', () => {
126126
expect((d as any).knex.client.config.connection).toEqual({ filename: ':memory:' });
127127
});
128128

129+
// #11784 — `redshift` reaches the server through the `pg` driver (knex's
130+
// `Client_Redshift extends Client_PG`), so it HAS `connectionTimeoutMillis`
131+
// and obeys it; it just carried no DIALECT_CONNECT_TIMEOUT row, so nothing
132+
// was injected and the attempt fell through to the strictly looser 15s pool
133+
// backstop. Nothing errored and nothing was logged — the before-state is
134+
// silence, so "it works" is not evidence and the INJECTED CONFIG is pinned.
135+
//
136+
// The two controls are load-bearing. `pg` already had a row, so a broken
137+
// injection path fails them together instead of looking redshift-specific;
138+
// `better-sqlite3` legitimately has none (a file open has no handshake), so
139+
// an injector that stopped reading the table and timed everything would be
140+
// caught rather than read as a pass.
141+
it('bounds a redshift connect attempt at the dialect timeout, not the pool backstop (#11784)', () => {
142+
const injected = (client: string, connection: any) =>
143+
(make({ client, connection, useNullAsDefault: true }) as any).knex.client.config.connection;
144+
145+
// subject — URL form moves into the pg URL slot with the timeout alongside
146+
expect(injected('redshift', 'postgres://u:p@host:5439/d')).toEqual({
147+
connectionString: 'postgres://u:p@host:5439/d',
148+
connectionTimeoutMillis: 10_000,
149+
});
150+
// subject — object form gains the key and disturbs nothing else
151+
expect(injected('redshift', { host: 'wh.eu-west-1.redshift.amazonaws.com', port: 5439, database: 'dev' }))
152+
.toEqual({
153+
host: 'wh.eu-west-1.redshift.amazonaws.com', port: 5439, database: 'dev',
154+
connectionTimeoutMillis: 10_000,
155+
});
156+
157+
// positive control — a client that already had a row
158+
expect(injected('pg', { host: 'db', database: 'app' }).connectionTimeoutMillis).toBe(10_000);
159+
// negative control — a dialect that legitimately has no such knob
160+
expect(injected('better-sqlite3', { filename: ':memory:' }).connectionTimeoutMillis).toBeUndefined();
161+
162+
// The two bounds must not be equal (knex wins a tie and the accurate
163+
// message is never seen). Redshift now takes the strict 10s dialect bound
164+
// with the 15s pool value still strictly looser behind it — which is the
165+
// whole of what #11784 restores.
166+
const d = make({ client: 'redshift', connection: 'postgres://u:p@host:5439/d' });
167+
expect((d as any).knex.client.config.pool.createTimeoutMillis).toBe(15_000);
168+
});
169+
170+
it("leaves a redshift host's own explicit connect timeout alone (#11784)", () => {
171+
const d = make({
172+
client: 'redshift',
173+
connection: { host: 'wh.redshift.amazonaws.com', connectionTimeoutMillis: 60_000 },
174+
});
175+
expect((d as any).knex.client.config.connection.connectionTimeoutMillis).toBe(60_000);
176+
});
177+
129178
it('leaves a function-valued connection alone — the host builds each one itself', () => {
130179
const provider = () => ({ host: 'x' });
131180
const d = make({ client: 'pg', connection: provider });

‎packages/drivers/driver-sql/src/sql-driver.ts‎

Lines changed: 42 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -4576,14 +4576,30 @@ export class SqlDriver implements IDataDriver {
45764576
*/
45774577
private static readonly DIALECT_CONNECT_TIMEOUT: Record<string, { key: string; urlKey: string }> =
45784578
Object.fromEntries<{ key: string; urlKey: string }>([
4579-
// Every spelling that means Postgres SQL, EXTENDED by `cockroachdb`:
4580-
// a separate knex dialect, but one that reaches the server through the
4581-
// same `pg` driver and therefore takes the same knob. Deriving the pg
4582-
// arm from {@link POSTGRES_EMIT_CLIENTS} is what keeps this table from
4583-
// drifting away from the getters again (#11550) — widening recognition
4584-
// now widens this with it. `redshift` still carries no entry; see the
4585-
// note in {@link withConnectBound} for why that absence is load-bearing.
4586-
...[...SqlDriver.POSTGRES_EMIT_CLIENTS, 'cockroachdb'].map(
4579+
// Every spelling that means Postgres SQL, EXTENDED by `cockroachdb` and
4580+
// `redshift`: separate knex dialects, but ones that reach the server
4581+
// through the same `pg` driver and therefore take the same knob. Knex's
4582+
// `Client_Redshift` literally `extends Client_PG`, so the settings object
4583+
// it hands to `pg.Client` honours `connectionTimeoutMillis` exactly as
4584+
// `pg` does. Deriving the pg arm from {@link POSTGRES_EMIT_CLIENTS} is
4585+
// what keeps this table from drifting away from the getters again
4586+
// (#11550) — widening recognition now widens this with it.
4587+
//
4588+
// `redshift`'s row arrived with #11784. It had the knob and would have
4589+
// obeyed it, but carried no entry, so its connection attempt fell through
4590+
// to the strictly looser 15s `pool.createTimeoutMillis` backstop while
4591+
// the docblock above called 10s "the effective bound". Nothing errored
4592+
// and nothing was logged — the bound was simply 50% looser, for one
4593+
// client name.
4594+
//
4595+
// ⚠️ This arm and {@link POSTGRES_WIRE_CLIENTS} now happen to hold the
4596+
// same five names. That is a fact about today's membership, NOT an
4597+
// invariant, and must not be refactored into one: the two answer
4598+
// different questions (how do I spell the connect timeout vs. which npm
4599+
// package parses the wire), and unioning them would hand `redshift` and
4600+
// `cockroachdb` SQL-emission identity as a silent side effect — the open
4601+
// decision #11756, and #11550's subject, not this table's to make.
4602+
...[...SqlDriver.POSTGRES_EMIT_CLIENTS, 'cockroachdb', 'redshift'].map(
45874603
(c): [string, { key: string; urlKey: string }] =>
45884604
[c, { key: 'connectionTimeoutMillis', urlKey: 'connectionString' }],
45894605
),
@@ -4606,10 +4622,24 @@ export class SqlDriver implements IDataDriver {
46064622
// `!dialect` — sqlite, or a client with no connect-timeout knob — means
46074623
// there is no TIMEOUT to inject. It deliberately does not skip the session
46084624
// pins below: those answer a different question (what does a value MEAN on
4609-
// this connection), and the two lists are not the same list. Measured while
4610-
// fixing #11389: `redshift` speaks the pg wire protocol, and therefore
4611-
// needs the calendar-day pin, but carries no entry here — so a `return`
4612-
// placed at this point silently opted it out of a fix it needs.
4625+
// this connection), and the two lists are not the same list.
4626+
//
4627+
// ⚠️ The worked example that used to stand here was RETIRED by #11784,
4628+
// which gave `redshift` the connect-timeout row it was missing. Until then
4629+
// `redshift` was in {@link POSTGRES_WIRE_CLIENTS} (so it needed #11389's
4630+
// calendar-day pin) yet absent from {@link DIALECT_CONNECT_TIMEOUT} — so a
4631+
// `return` placed at this point silently opted it out of a fix it needed.
4632+
// That was measured, not hypothetical, which is why the example is recorded
4633+
// here rather than dropped.
4634+
//
4635+
// The hazard outlives the example. Every client needing a session pin
4636+
// happens to have a timeout row TODAY; that is current membership, not a
4637+
// property either table promises. The next pg-wire client added without a
4638+
// connect-timeout knob restores the exact bug — and it would fail the way
4639+
// this one did, in silence, with every test green. So: no `return` here,
4640+
// and the two tables stay separate. The tripwire that goes red the moment
4641+
// those memberships diverge again lives in
4642+
// `sql-driver-11389-date-tz-skew.test.ts`.
46134643
const dialect = SqlDriver.DIALECT_CONNECT_TIMEOUT[String(knexConfig.client ?? '')];
46144644
if (dialect) {
46154645
const conn = knexConfig.connection;

0 commit comments

Comments
 (0)