Skip to content

Commit c6cff31

Browse files
committed
fix(driver-sql): report an un-run MySQL widening ALTER at error, naming the fix (#9609)
Boot schema-sync's MySQL widening swallows a failed `ALTER … MODIFY COLUMN` on purpose — correctness never depended on the widening having run, and a migration must not take boot down (#9542 adjudicated exactly that and it is unchanged here). It reported the swallow at `warn`. AGENTS.md's degradation rule decides the level with one question: after the degradation, does the system still look normal from the outside while something it claims is persisted has not landed? Both halves hold — boot completes and serves traffic, and the rule's `error` limb names this case verbatim, "DDL that was supposed to run did not". An un-widened `TIMESTAMP` keeps truncating milliseconds and an un-widened `TIME` keeps ROUNDING fractional seconds, against a canonical storage form that promises the milliseconds are kept, and nothing else reports the column as outstanding. Newly reachable, too: before #9542 the boot ALTER waited MySQL's one-year default and never returned, so this catch could not fire on a metadata-lock block at all. Both messages now report at `error` and carry the second thing an `error` owes — the FIX: identify the metadata-lock holder with `SHOW PROCESSLIST` or `performance_schema.metadata_locks`, end it, then re-run `os migrate apply` or restart, the widening being idempotent. Control flow is untouched. The gate could not see these sites: `check-durability-degradation-log-level.mjs` scans all of `packages/` and its baseline is empty, but its durability vocabulary had no entry for the widening's DDL path. `runWideningAlters` is declared there now — measured to light up exactly these two catches and nothing else — so the class stays fixed rather than the two sites. The emission goes through a named `logDurabilityFailure` helper rather than the file's inline `(this.logger.error ?? this.logger.warn)(…)`: the gate's matcher cannot see that parenthesized shape and reports it as a silent swallow, and the spelling it CAN see, `this.logger.error?.(…)`, prints nothing at all against a sink that has no `error` — worse than the `warn` it replaces. Pinned by a test against such a sink. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XqDQYVU5smx29ts9pAErja
1 parent e8dba8a commit c6cff31

4 files changed

Lines changed: 239 additions & 25 deletions

File tree

‎.changeset/olive-donkeys-brake.md‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
---
2+
'@objectstack/driver-sql': patch
3+
---
4+
5+
Report an un-run MySQL widening ALTER at `error`, naming the fix
6+
7+
Boot schema-sync widens legacy MySQL `TIMESTAMP` columns to `DATETIME(3)` and
8+
zero-precision `TIME` columns to `TIME(3)`. When that DDL cannot run — most
9+
often another session holding the table's metadata lock — the failure is
10+
swallowed on purpose so a migration never takes boot down. It was reported at
11+
`warn`.
12+
13+
That is the case AGENTS.md's degradation rule names for `error` by name: after
14+
the swallow the platform boots, serves traffic and looks entirely normal, while
15+
the DDL that was supposed to run did not. An un-widened `TIMESTAMP` keeps
16+
truncating milliseconds and an un-widened `TIME` keeps rounding fractional
17+
seconds to whole ones, against a canonical storage form that promises the
18+
milliseconds are kept, and nothing else reports the column as outstanding.
19+
20+
Both lines now report at `error` and say what to do about it — identify the
21+
metadata-lock holder, end it, then re-run `os migrate apply` or restart, the
22+
widening being idempotent. Control flow is unchanged: the swallow stays, and
23+
the deferred-DDL flush keeps its loud refusal.
24+
25+
`scripts/check-durability-degradation-log-level.mjs` gains `runWideningAlters`
26+
in its durability vocabulary, so the class stays fixed rather than these two
27+
sites.

‎packages/drivers/driver-sql/src/sql-driver-deferred-ddl-lock-wait.test.ts‎

Lines changed: 129 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -94,17 +94,27 @@ class FakeMysqlDriver extends SqlDriver {
9494

9595
issued: Issued[] = [];
9696
/**
97-
* [#9542] Every `logger.warn` the driver emitted.
97+
* [#9542/#9609] Every log line the driver emitted, **with its level**.
9898
*
9999
* On the boot path this is the ONLY output a blocked widening produces — the
100100
* swallow eats the error itself — so "the bound fires and the operator is
101101
* told" and "the bound fires and nothing at all is printed" are the same
102102
* green suite without a sink to assert on.
103+
*
104+
* ⭐ #9609: the LEVEL is recorded, not just the text. The pins below assert
105+
* `error`, because that is the observable that regresses: a future edit that
106+
* puts this line back on `warn` keeps every message assertion green while
107+
* removing it from the operator's alerting, which is the only signal there
108+
* is. Recording the text alone cannot tell those two apart. The sink
109+
* therefore also HAS an `error` channel — the previous fixture had only
110+
* `warn`, which would have made an `error` call land nowhere and read as a
111+
* missing line rather than as a level change.
103112
*/
104-
warnings: Array<{ msg: string; meta?: any }> = [];
113+
logs: Array<{ level: 'warn' | 'error'; msg: string; meta?: any }> = [];
105114

106115
protected override logger = {
107-
warn: (msg: string, meta?: any) => { this.warnings.push({ msg, meta }); },
116+
warn: (msg: string, meta?: any) => { this.logs.push({ level: 'warn', msg, meta }); },
117+
error: (msg: string, meta?: any) => { this.logs.push({ level: 'error', msg, meta }); },
108118
info: () => {},
109119
};
110120

@@ -174,6 +184,32 @@ function makeDriver(): FakeMysqlDriver {
174184
});
175185
}
176186

187+
/**
188+
* [#9609] The same driver behind a sink that has NO `error` channel.
189+
*
190+
* `SqlDriver.logger.error` is optional by declaration, and a host that injects
191+
* `{ warn }` is a supported composition. This twin exists so the fallback in
192+
* `logDurabilityFailure` is pinned by a test rather than by a comment: the
193+
* obvious way to make the durability gate see this call site is
194+
* `this.logger.error?.(…)`, which against THIS sink prints nothing at all —
195+
* strictly worse than the `warn` it replaced, and invisible to every assertion
196+
* that only looks at the driver with a full sink.
197+
*/
198+
class NoErrorSinkDriver extends FakeMysqlDriver {
199+
protected override logger = {
200+
warn: (msg: string, meta?: any) => { this.logs.push({ level: 'warn' as const, msg, meta }); },
201+
info: () => {},
202+
};
203+
}
204+
205+
function makeNoErrorSinkDriver(): NoErrorSinkDriver {
206+
return new NoErrorSinkDriver({
207+
client: 'better-sqlite3',
208+
connection: { filename: ':memory:' },
209+
useNullAsDefault: true,
210+
});
211+
}
212+
177213
/** Create the table, then arm the deferral over the same metadata. */
178214
async function armedFlush(driver: FakeMysqlDriver): Promise<void> {
179215
await driver.initObjects([WIDGET]); // table now EXISTS — widening applies
@@ -361,27 +397,109 @@ describe('[#9354/#9542] a blocked widening ALTER — bounded on both paths, refu
361397
// boot, and correctness never depended on the widening having run.
362398
});
363399

364-
it('finally reaches the boot `logger.warn` — a bound that printed nothing would deliver nothing', async () => {
400+
it('finally reaches the boot durability log — a bound that printed nothing would deliver nothing', async () => {
365401
driver = makeDriver();
366402
await driver.initObjects([WIDGET]);
367403
driver.issued.length = 0;
368-
driver.warnings.length = 0;
404+
driver.logs.length = 0;
369405

370406
await expect(driver.initObjects([WIDGET])).resolves.toBeUndefined();
371407

372-
// ⭐ The card's whole claim. This warn was already written and was
408+
// ⭐ The card's whole claim. This line was already written and was
373409
// UNREACHABLE in this scenario: the unbounded ALTER never returned, so the
374410
// catch that logs it never ran. A bound whose only effect is a quieter hang
375411
// delivers nothing and looks identical in a green suite, so the delivery is
376412
// asserted on the sink rather than inferred from the bound being armed.
377-
const warn = driver.warnings.find((w) => /could not widen MySQL datetime columns/.test(w.msg));
378-
expect(warn).toBeDefined();
379-
expect(warn!.msg).toContain(WIDGET.name);
413+
const line = driver.logs.find((w) => /widen MySQL datetime columns/.test(w.msg));
414+
expect(line).toBeDefined();
415+
expect(line!.msg).toContain(WIDGET.name);
380416
// Carrying the server's own diagnosis, not a swallowed blank.
381-
expect(String(warn!.meta?.error)).toMatch(/Lock wait timeout exceeded/);
417+
expect(String(line!.meta?.error)).toMatch(/Lock wait timeout exceeded/);
382418
// And it is the SERVER error that was swallowed, not the ADR-0112 refusal:
383419
// that envelope stays flush-only, so its operator sentence is absent here.
384-
expect(String(warn!.meta?.error)).not.toMatch(/PROCESSLIST|No schema change was made/);
420+
expect(String(line!.meta?.error)).not.toMatch(/PROCESSLIST|No schema change was made/);
421+
});
422+
423+
// ───────────────────────────────────────────────────────────────
424+
// #9609 — the LEVEL of that line, which is a separate question
425+
// ───────────────────────────────────────────────────────────────
426+
427+
it('reports the un-run datetime widening at `error`, not `warn`', async () => {
428+
driver = makeDriver();
429+
await driver.initObjects([WIDGET]);
430+
driver.logs.length = 0;
431+
432+
await expect(driver.initObjects([WIDGET])).resolves.toBeUndefined();
433+
434+
// AGENTS.md → "Degradation log levels" decides this with one question:
435+
// after the degradation, does the system still look normal from the outside
436+
// while something it claims is persisted has not landed? Here: yes. Boot
437+
// completed, traffic is served, and the `error` limb names this exact case
438+
// — "DDL that was supposed to run did not". The swallow is unchanged and
439+
// deliberately so (#9542); only the level moved.
440+
const line = driver.logs.find((w) => /widen MySQL datetime columns/.test(w.msg));
441+
expect(line?.level).toBe('error');
442+
// ⭐ Asserted as an ABSENCE too, because `find` above would happily return
443+
// an `error` line while a second `warn` copy of the same degradation kept
444+
// being emitted somewhere else on the path.
445+
expect(driver.logs.filter((w) => w.level === 'warn')).toHaveLength(0);
446+
});
447+
448+
it('reports the un-run TIME widening at `error` too — the twins do not diverge', async () => {
449+
driver = makeDriver();
450+
driver.legacyDatetimeColumns = [];
451+
driver.legacyTimeColumns = [{ name: 'at', nullable: true }];
452+
await driver.initObjects([WIDGET]);
453+
driver.logs.length = 0;
454+
455+
await expect(driver.initObjects([WIDGET])).resolves.toBeUndefined();
456+
457+
// The `Field.time` twin loses fractional seconds by ROUNDING, which changes
458+
// the wall clock that was asked for — if anything the louder of the two. It
459+
// is pinned separately because the two catches are separate code: fixing one
460+
// and not the other is the likeliest way this half-regresses.
461+
const line = driver.logs.find((w) => /widen MySQL time columns/.test(w.msg));
462+
expect(line?.level).toBe('error');
463+
expect(line!.msg).toContain(WIDGET.name);
464+
});
465+
466+
it('names the FIX, not only the consequence — the second thing an `error` owes', async () => {
467+
driver = makeDriver();
468+
await driver.initObjects([WIDGET]);
469+
driver.logs.length = 0;
470+
471+
await expect(driver.initObjects([WIDGET])).resolves.toBeUndefined();
472+
473+
const line = driver.logs.find((w) => /widen MySQL datetime columns/.test(w.msg));
474+
// AGENTS.md: an `error` owes BOTH the consequence and the fix, in the first
475+
// line it prints. An operator woken at `error` with no next step is a worse
476+
// outcome than the `warn` this replaced, so the actionable half is pinned as
477+
// hard as the level. Same three moves the flush's refusal names, because it
478+
// is the same blocker: find the lock holder, end it, re-run.
479+
expect(line!.msg).toMatch(/PROCESSLIST|metadata_locks/);
480+
expect(line!.msg).toMatch(/os migrate apply|restart/);
481+
expect(line!.msg).toMatch(/idempotent/);
482+
// And the consequence stays concrete rather than becoming "degraded".
483+
expect(line!.msg).toMatch(/millisecond/i);
484+
});
485+
486+
it('still delivers the line at `warn` when the injected sink has no `error`', async () => {
487+
const noErrorSink = makeNoErrorSinkDriver();
488+
driver = noErrorSink;
489+
await noErrorSink.initObjects([WIDGET]);
490+
noErrorSink.logs.length = 0;
491+
492+
await expect(noErrorSink.initObjects([WIDGET])).resolves.toBeUndefined();
493+
494+
// ⛔ The regression this guards is `this.logger.error?.(…)`: it satisfies the
495+
// durability gate's matcher and prints NOTHING against this sink, converting
496+
// a loud degradation into a silent one to please a checker. `SqlDriver`
497+
// declares `logger.error` optional, so this composition is supported and the
498+
// fallback is part of the contract, not a nicety.
499+
const line = noErrorSink.logs.find((w) => /widen MySQL datetime columns/.test(w.msg));
500+
expect(line).toBeDefined();
501+
expect(line!.level).toBe('warn');
502+
expect(line!.msg).toMatch(/PROCESSLIST|metadata_locks/);
385503
});
386504

387505
it('clears the flush flag after a refusal, so a later boot sync is unaffected', async () => {

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

Lines changed: 79 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -887,7 +887,8 @@ function backendStatementFaultError(object: string, cause: unknown): Error {
887887
* one. Everything above is reasoning about how long a legitimate metadata-lock
888888
* holder can plausibly hold it — a property of the lock, not of who is waiting
889889
* on it. Boot's difference from the flush is what happens when the bound fires
890-
* (boot warns and carries on; the flush refuses), never how long it waits.
890+
* (boot reports at `error` and carries on; the flush refuses), never how long
891+
* it waits.
891892
*
892893
* ⛔ Deliberately NOT configurable, and deliberately NOT retried — the 2026-08-17
893894
* ruling's explicit minimality. Both wait for measured demand. A knob added now
@@ -3983,6 +3984,45 @@ export class SqlDriver implements IDataDriver {
39833984
error: (msg, meta) => console.error(msg, meta ?? ''),
39843985
};
39853986

3987+
/**
3988+
* [#9609] Emit on the durability-degradation channel: `error` when the
3989+
* injected sink has one, `warn` when it does not.
3990+
*
3991+
* # Why a named method and not the inline `(this.logger.error ?? this.logger.warn)(…)`
3992+
*
3993+
* Two reasons, and the second is the load-bearing one.
3994+
*
3995+
* ① It names the AGENTS.md channel at the call site, so the level is read as
3996+
* a classification ("this is durability, not functionality") rather than as
3997+
* an adjective someone picked. The `error?` field above already documents
3998+
* that channel; this is its verb.
3999+
*
4000+
* ② `check-durability-degradation-log-level.mjs` cannot SEE the inline
4001+
* fallback. Its `loggerLevel()` matches the shape `<logger|log|console>.<level>(…)`
4002+
* — a call whose expression is a property access. `(a ?? b)(…)` is a call on
4003+
* a PARENTHESIZED expression, so the matcher returns nothing and the catch is
4004+
* reported as `catch swallows the failure with no log at all` — a false
4005+
* silent-swallow on code that is loud at runtime. Measured, not inferred:
4006+
* with `runWideningAlters` in the vocabulary, both widening catches reported
4007+
* exactly that while calling the fallback inline.
4008+
*
4009+
* ⛔ Deliberately NOT `this.logger.error?.(…)`, which the gate DOES recognise.
4010+
* That spelling drops the message entirely for a sink that has no `error` —
4011+
* turning a durability-critical failure into true silence in order to
4012+
* satisfy a matcher, which is the failure this whole rule exists to prevent. The gate follows
4013+
* same-file helpers transitively (its header says so, and the audit reports
4014+
* sites as `loud (error@… via <helper>())`), so routing through this method
4015+
* keeps the fallback AND is correctly classified.
4016+
*
4017+
* The pre-existing inline uses in this file (the ADR-0120 D4 index sites) are
4018+
* left alone on purpose — they are a separate change with a separate blast
4019+
* radius, and the matcher blind spot itself is filed rather than patched here.
4020+
*/
4021+
protected logDurabilityFailure(msg: string, meta?: any): void {
4022+
if (this.logger.error) this.logger.error(msg, meta);
4023+
else this.logger.warn(msg, meta);
4024+
}
4025+
39864026
/** Whether the underlying database is a SQLite variant (sqlite3 or better-sqlite3). */
39874027
protected get isSqlite(): boolean {
39884028
const c = (this.config as any).client;
@@ -8113,9 +8153,11 @@ export class SqlDriver implements IDataDriver {
81138153
* better for boot than it is for an operator: a boot blocked on another
81148154
* session's metadata lock waits 31,536,000 seconds having printed nothing,
81158155
* and boot is the path nobody can retry from a prompt. Bounding it turns
8116-
* that into a bounded wait plus the widening's own `logger.warn` — which,
8156+
* that into a bounded wait plus the widening's own durability log — which,
81178157
* until the bound reached here, could never fire at all: the ALTER never
8118-
* returned, so its catch never ran.
8158+
* returned, so its catch never ran. #9609 raised that line from `warn` to
8159+
* `error`; the level is the operator's only signal, and the swallow below is
8160+
* exactly what makes an un-run ALTER invisible without it.
81198161
*
81208162
* What stays gated on {@link flushDeferredSchemaDdl} is the REFUSAL, and
81218163
* only it. Boot still swallows: correctness never depends on the widening
@@ -8150,8 +8192,8 @@ export class SqlDriver implements IDataDriver {
81508192
} catch (err) {
81518193
// #9542: the bound is armed on both callers, the ESCAPE is not.
81528194
// Off the flush this rethrows the server's own error, which the
8153-
// widening's catch logs and swallows — boot's policy unchanged,
8154-
// now reached by a wait that ends.
8195+
// widening's catch reports and swallows — boot's policy
8196+
// unchanged, now reached by a wait that ends.
81558197
if (this.flushingDeferredDdl && isMysqlLockWaitTimeout(err)) {
81568198
throw deferredDdlLockWaitError(table, DEFERRED_DDL_LOCK_WAIT_TIMEOUT_SECONDS, err);
81578199
}
@@ -8219,9 +8261,14 @@ export class SqlDriver implements IDataDriver {
82198261
// lock wait that the flush bounded is the operator's answer to a command
82208262
// they ran, and dropping it would report success for work not done.
82218263
if (isDeferredDdlLockWaitRefusal(err)) throw err;
8222-
this.logger.warn(
8223-
`[sql-driver] could not widen MySQL datetime columns on ${table}; ` +
8224-
`writes stay correct, but the 2038 ceiling and millisecond truncation remain`,
8264+
this.logDurabilityFailure(
8265+
`[sql-driver] FAILED to widen MySQL datetime columns on ${table} — the DDL did not run and boot ` +
8266+
`continued: the columns are still legacy TIMESTAMP, so every write to them keeps truncating ` +
8267+
`milliseconds and keeps the 2038 ceiling, while reads return exactly what was stored and nothing ` +
8268+
`else reports the table as un-widened. Fix: identify the metadata-lock holder with ` +
8269+
'`SHOW PROCESSLIST` or `performance_schema.metadata_locks` (or resolve the error in this ' +
8270+
'record\'s meta), end it, then re-run `os migrate apply` or restart — the widening is ' +
8271+
're-detected from `information_schema` and is idempotent, so re-running is safe.',
82258272
{ error: err instanceof Error ? err.message : String(err) },
82268273
);
82278274
}
@@ -8269,9 +8316,21 @@ export class SqlDriver implements IDataDriver {
82698316
* keeps the milliseconds instead, matching the canonical form's resolution
82708317
* and the `DATETIME(3)` precedent (#3942).
82718318
*
8272-
* Failures are logged and swallowed for the usual reason; the only cost of a
8273-
* `TIME(0)` column that could not be widened is second-rounding of fractional
8274-
* writes — which is today's behaviour.
8319+
* Failures are swallowed for the usual reason; the only cost of a `TIME(0)`
8320+
* column that could not be widened is second-rounding of fractional writes —
8321+
* which is today's behaviour.
8322+
*
8323+
* # The level is `error`, and that is a separate question from the swallow (#9609)
8324+
*
8325+
* Swallow-vs-throw was adjudicated (#9542) and is unchanged: boot must not go
8326+
* down over a migration. `warn`-vs-`error` was never separately decided, and
8327+
* AGENTS.md → "Degradation log levels" decides it with one question — after
8328+
* the degradation, does the system still look normal from the outside while
8329+
* something it claims is persisted has not landed? Both halves hold: boot
8330+
* completes and serves traffic, and its `error` limb names this case by name
8331+
* ("DDL that was supposed to run did not"). Nothing else reports the column
8332+
* as un-widened, so the level IS the signal. Same reasoning, same level, on
8333+
* {@link migrateMysqlDatetimeColumns}.
82758334
*/
82768335
protected async migrateMysqlTimeColumns(
82778336
table: string,
@@ -8307,9 +8366,15 @@ export class SqlDriver implements IDataDriver {
83078366
// lock wait that the flush bounded is the operator's answer to a command
83088367
// they ran, and dropping it would report success for work not done.
83098368
if (isDeferredDdlLockWaitRefusal(err)) throw err;
8310-
this.logger.warn(
8311-
`[sql-driver] could not widen MySQL time columns on ${table}; ` +
8312-
`fractional-second writes keep rounding to whole seconds`,
8369+
this.logDurabilityFailure(
8370+
`[sql-driver] FAILED to widen MySQL time columns on ${table} — the DDL did not run and boot ` +
8371+
`continued: the columns are still zero-precision TIME, so every fractional-second write to them ` +
8372+
`keeps being ROUNDED to whole seconds — the stored wall clock is not the one that was asked for — ` +
8373+
`while reads return exactly what was stored and nothing else reports the table as un-widened. ` +
8374+
'Fix: identify the metadata-lock holder with `SHOW PROCESSLIST` or ' +
8375+
'`performance_schema.metadata_locks` (or resolve the error in this record\'s meta), end it, then ' +
8376+
're-run `os migrate apply` or restart — the widening is re-detected from `information_schema` ' +
8377+
'and is idempotent, so re-running is safe.',
83138378
{ error: err instanceof Error ? err.message : String(err) },
83148379
);
83158380
}

0 commit comments

Comments
 (0)