Skip to content

fix(driver-sql): bound lock_wait_timeout on the deferred-DDL flush so a blocked os migrate refuses loudly instead of hanging for a year - #9547

Merged
os-sam merged 2 commits into
mainfrom
claude/issue-9354-bounded-lock-wait-timeout
Aug 18, 2026
Merged

fix(driver-sql): bound lock_wait_timeout on the deferred-DDL flush so a blocked os migrate refuses loudly instead of hanging for a year#9547
os-sam merged 2 commits into
mainfrom
claude/issue-9354-bounded-lock-wait-timeout

Conversation

@claude

@claudeclaudeBot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Fixes#9354

Implements the maintainer ruling of 2026-08-17 (recorded on the card, verbatim 「同意」): Option A, minimal version.

The behaviour

The deferred-DDL flush widens legacy MySQL TIMESTAMP columns to DATETIME(3) (#3942) and TIME to TIME(3) (#3994) with ALTER TABLE … MODIFY COLUMN, which needs an exclusive metadata lock on the table. That ALTER ran on a session inheriting MySQL's default lock_wait_timeout31,536,000 seconds, one year. One other session holding a lock on the table parks the ALTER in Waiting for table metadata lock for that long, and nothing prints.

An operator running os migrate apply against a busy production table meets this as a command that simply hangs, indistinguishable from a crash. It was first measured as a CI stall: a sub-second test blew a 5000ms budget with no error at all.

Two things were wrong, and bounding alone would have fixed neither

  1. Nothing bounded the wait. Re-verified on this branch's merge base: lock_wait_timeout had zero occurrences anywhere under packages/, with flushDeferredSchemaDdl as the positive control returning real hits.
  2. The widening swallows its failures.migrateMysqlDatetimeColumns catches, logs a warn and returns. So a bound on its own would have produced a MySQL error that was immediately discarded — os migrate apply would print "Applied 0 change(s)" and exit 0, reporting success for work it did not do. The hang would have become a silent no-op, which is not obviously an improvement.

The change

The flush now runs its widening ALTERs through one seam, runWideningAlters:

  • On one pinned connection.lock_wait_timeout is a SESSION variable, so a SET SESSION issued through the pool lands on a connection the ALTER never uses — a no-op that looks exactly like a fix. The seam holds a single connection (the same knex-transaction affinity every other multi-statement unit in this file already uses) so both statements provably ride one session. MySQL implicitly commits on DDL, which costs nothing here: the transaction is for connection affinity, not atomicity.
  • Bounded to 120 seconds, then the prior value is restored so the pooled connection carries nothing away.
  • Exactly one condition escapes the swallow. A metadata-lock timeout is re-thrown as an ADR-0112 envelope — DATABASE_ERROR / 500, from the existing closed vocabulary, minting no new code — whose message names the lock wait, the table, the bound it hit, and how to find the holder. Every other failure keeps the swallow it has today.
  • Only on the flush. Boot schema-sync reaches the same widening through the same initObjects lines and must not be taken down by a migration, so it still runs unbounded and still swallows.

Recognition is by errno 1205 / ER_LOCK_WAIT_TIMEOUT, followed through cause (knex re-throws with the driver error attached). Message text is deliberately not sniffed — MySQL and MariaDB word it differently and both translate it, so a prose match is a recognizer that fails silently in another locale, back to the year-long hang.

The proposed bound: 120 seconds

The ruling delegates the value, minutes-scale, with reasoning. It is a diagnosis deadline, not a capacity knob — its job is to end the silence, not to decide how patient a migration may be. So it is the shortest wait that still clears legitimate blockers:

  • Above the noise. Normal OLTP transactions on a table being migrated hold their metadata lock for milliseconds. Two minutes is three orders of magnitude above that, so an ordinary busy table never trips it — the bound does not turn a working migration into a failing one.
  • Below an operator's patience. A command silent for two minutes is still being watched. At ten minutes the operator has already reached for SHOW PROCESSLIST or killed it, so a bound firing later than that arrives after the diagnosis it was meant to provide.
  • Cheap to be wrong in the eager direction.os migrate apply is re-runnable and the widening is idempotent, so an over-eager bound costs one re-run — against an unbounded hang as the cost of one that never fires.

⛔ Not configurable and not retried, per the ruling's explicit minimality. Both wait for measured demand.

Scope decision the ruling did not fully close

The card's own sub-question asked whether the bound covers all deferred-DDL statements or only the widening ALTER. Implemented: both MySQL widening ALTERs — datetime and time — and nothing else. They are adjacent calls in the same flush, the same statement class, taking the same exclusive metadata lock on the same connection, and they share one helper here rather than two copies. Bounding one while its twin still waits a year would leave os migrate hanging exactly as before, with the operator unable to tell which ALTER hung. Create-table / add-column DDL is not bounded: that would mean routing knex's schema builder through the pinning seam, which is a materially larger change than the ruling's "a few lines plus a pin test".

Tests

Ten pins in sql-driver-deferred-ddl-lock-wait.test.ts. They assert the refusal, not that a SET SESSION string was emitted — a suite asserting only emission passes in full while the operator still hangs. Only the connection and the information_schema probe are faked, so the bounding, the 1205 recognition, the envelope and the escape from the swallow all execute for real.

Reverse verification, direction predicted before running:

AblationPredictedObserved
Delete the escape from the datetime catchrefusal swallowed again, the pins routed through caught() go red6 red / 4 green, every failure "expected the flush to refuse, but it resolved"
Stop emitting the boundenvelope pins unaffected, only the bound pins go red3 red / 7 green, exactly the three bound pins

The first ablation going red is also what proves the suite reads this branch's source rather than a stale dist — a stale read would have stayed green. The restore leg was verified byte-identical to the committed tree (git status --porcelain empty) and re-run green, 10/10.

Gates

Re-derived against the actual diff with node scripts/pm/dispatch-gates.mjs (no path arguments) and run at 0c9a3db. All green, including check:type-check-debt --re-measure on the built workspace closure (33 ledger entries re-measured in 267s, none above its recorded number) and the full @objectstack/driver-sql suite (1756 passed / 0 failed) plus the CLI deferred-DDL integration test.

Deliberately not in this PR

Two findings were filed unassigned rather than folded in, both outside the dispatched file surface:

Related: #9350 (the metadata-lock measurement — this change is independent of it and makes the failure diagnosable either way) · #3954 · ADR-0112.


Generated by Claude Code

… a blocked migrate refuses loudly (#9354)
The MySQL widening ALTERs in the deferred-DDL flush ran on a session inheriting
MySQL's default lock_wait_timeout of 31,536,000s (one year), and the widening
swallows its failures. A metadata lock held by another session therefore parked
`os migrate apply` silently, with no output an operator could diagnose from.
The flush now pins one connection, bounds lock_wait_timeout to 120s on that same
session, and re-throws a metadata-lock timeout as an ADR-0112 envelope
(DATABASE_ERROR / 500) naming the lock wait, the table and the bound. Boot sync
and every non-lock-wait failure keep the behaviour they had.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017qYPmkKEsfbWY1yVg83p8F
@github-actions

github-actionsBot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/driver-sql, touching 12 documentable anchor(s).

5 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/data-modeling/drivers.mdx(via SqlDriver (symbol))
  • content/docs/plugins/packages.mdx(via SqlDriver (symbol))
  • content/docs/protocol/kernel/index.mdx(via SqlDriver (symbol))
  • content/docs/protocol/kernel/lifecycle.mdx(via SqlDriver (symbol))
  • content/docs/protocol/objectql/query-syntax.mdx(via SqlDriver (symbol))

1 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v17.mdx(via SqlDriver (symbol))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see

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 b348ac2c24100e26bf88486f704e3064a2e1fee6packageMentionDocs.

Which tree this was computed on

This run read content/docs from 6cba128126ee68b093d2970aa4f99ffd959d3aed — the merge of head 272bd8f9643a13b723d84814cdee524b7e1fbe59 into base b348ac2c24100e26bf88486f704e3064a2e1fee6, 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 6cba128126ee68b093d2970aa4f99ffd959d3aed && git checkout 6cba128126ee68b093d2970aa4f99ffd959d3aed
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin b348ac2c24100e26bf88486f704e3064a2e1fee6 272bd8f9643a13b723d84814cdee524b7e1fbe59 && git checkout -B drift-repro b348ac2c24100e26bf88486f704e3064a2e1fee6 && git merge --no-ff 272bd8f9643a13b723d84814cdee524b7e1fbe59
node scripts/docs-audit/affected-docs.mjs --json b348ac2c24100e26bf88486f704e3064a2e1fee6

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

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs b348ac2c24100e26bf88486f704e3064a2e1fee6 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

… Error overload (#9354)
`new Error(msg, { cause })` needs the ES2022 lib; packages/drivers/driver-sql
targets ES2020, and its tsconfig includes `src/**/*` with no test exclusion, so
`tsc --noEmit` compiled the new pin file and failed:
src/sql-driver-deferred-ddl-lock-wait.test.ts(57,95): error TS2554:
Expected 0-1 arguments, but got 2.
The fixture now defines `cause` with Object.defineProperty — the same idiom the
driver's own refusals in sql-driver.ts use, reproducing what the constructor
produces at runtime including non-enumerability. No tsconfig change, no cast,
no baseline moved.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017qYPmkKEsfbWY1yVg83p8F
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

2 participants

@os-sam@claude