Skip to content

docs(metadata): re-anchor the single-connection-pool hazard on a verified live witness (#7708) - #7840

Merged
huangyiirene merged 1 commit into
mainfrom
claude/issue-7708-pool-hazard-witness
Aug 11, 2026
Merged

docs(metadata): re-anchor the single-connection-pool hazard on a verified live witness (#7708)#7840
huangyiirene merged 1 commit into
mainfrom
claude/issue-7708-pool-hazard-witness

Conversation

@huangyiirene

Copy link
Copy Markdown
Collaborator

Fixes#7708

MetadataManager's listCache policy comment cited plugin-audit's captureBefore as the live example of a caller that threads its transaction by hand to dodge the SQLite single-connection-pool stall. #6656 retired captureBefore and PR #7081 removed its last consumer, so the cited witness is gone — but the hazard claim is independent. This replaces the witness rather than deleting the note.

Verification record — the card told me to distrust the card

The issue's own instruction outranks its confidence: verify the hazard against the current pool implementation rather than trusting the card. So the first step was a measurement, not an edit.

Setup: real ObjectQL + real SqlDriver (better-sqlite3, :memory:), a real DatabaseLoader.list('permission') with the loader's own LRU cache disabled, sys_metadata created via initObjects. Confirmed at runtime: knex.client.pool.max === 1 for the SQLite dialect, and acquireConnectionTimeout is not overridden anywhere in driver-sql (knex default 60s).

CaseTransaction opened viaResult
Bdriver.beginTransaction() — no ambient storeSTALLS, then throws Knex: Timeout acquiring a connection — measured 60 085 ms
Aengine.transaction() — ambient txStore publishedreturns in 12 ms
controlno transaction openreturns immediately

Case A was checked against the obvious false negative (that engine.transaction() had silently degraded to the no-tx-support path). Inside the callback: driver.activeTransactions === 1, txStore.getStore().transaction present, and a spy on driver.find recorded the sys_metadata read arriving with the transaction handle attached (hasTx: true).

Verdict: premise_still_valid: true — the hazard is real, and DatabaseLoader._find() does demonstrate it._find() forwards no options (return this.engine.find(table, query)), so nothing threads the caller's transaction.

But the measurement narrowed the claim, and that refinement is now recorded in the comment: the stall is conditional. It needs an open transaction the engine's ambient txStore (ADR-0034) cannot see — buildDriverOptions prefers an explicit handle and falls back to the ambient store, so a transaction opened through engine.transaction() / ScopedContext gets threaded onto the read on the loader's behalf and does not stall. The old comment stated the hazard flatly and would have led a reader to the wrong conclusion about the engine-opened route.

The replacement witness

A codebase sweep for callers that thread a transaction by hand found exactly one live one: SqlDriver.ensureSequencesTable(), which takes parentTrx and runs its DDL on the caller's transaction for exactly this reason (its own comment names the batch/autonumber deadlock), with assertBareKnexSafe as the tripwire for callers that forget. Both halves are pinned by sql-driver-sqlite-tx-guard.test.ts. plugin-audit's remaining writers no longer mention transactions at all, confirming the old witness is fully gone.

This is a witness I verified stalls (case B) and verified is designed against (the parentTrx branch + its guard test) — not a fresher name substituted on faith, which is the defect this card exists to fix.

Scope

Comment-only, two files, both in packages/metadata/src:

  • metadata-manager.ts — the listCache policy comment.
  • metadata-manager-degraded-list-cache.test.ts — its doc comment carried the identical stale sentence. Updated in the same PR so the duplicate cannot re-seed the stale citation.

No widening: the fix did not land in the pool implementation. The temporary probes used for the measurement were run under packages/runtime (the only place with objectql + driver-sql + metadata in one dependency closure) and deleted — the diff contains no test additions.

Possible follow-up (not taken here, to respect the comment-only scope): the case A / case B split is currently pinned only by this PR's prose. An integration test in packages/runtime asserting "ambient-published transaction ⇒ threaded, bare driver transaction ⇒ stalls" would keep the new comment honest the way sql-driver-sqlite-tx-guard.test.ts keeps the driver honest. Happy to file it as a separate card.

Gates

GateStatus
pnpm check:durability-log-level✅ 24 catch seams, 63 read seams
pnpm check:nul-bytes✅ 7164 files
eslint (changed files)✅ clean
pnpm --filter @objectstack/metadata build
metadata test suite✅ 31 files, 603 tests passed

Changeset: none. Comment-only with no reader-visible generated output ⇒ this wants the skip-changeset label rather than an empty changeset. Flagging for the PM to apply.


Generated by Claude Code

…fied live witness (#7708)
`MetadataManager`'s `listCache` policy comment cited `plugin-audit`'s
`captureBefore` as the live example of a caller that threads its transaction
by hand to dodge the SQLite single-connection-pool stall. #6656 retired
`captureBefore` and PR #7081 removed its last consumer, so the cited witness
no longer exists — but the hazard it illustrated is independent, so this
replaces the witness instead of deleting the note.
Re-measured before editing, against a real `ObjectQL` + real `SqlDriver`
(better-sqlite3) driving a real `DatabaseLoader.list()` with the loader cache
off. `knex.client.pool.max === 1` for the SQLite dialect and
`acquireConnectionTimeout` is left at the knex default:
* transaction opened directly on the driver — the read stalls 60_085ms and
then throws knex's "Timeout acquiring a connection";
* transaction opened through `engine.transaction()` — returns in 12ms, with
`activeTransactions === 1` and the driver observably receiving the handle.
So the hazard is live, but CONDITIONAL in a way the old comment did not
record: it needs an open transaction the engine's ambient `txStore`
(ADR-0034) cannot see, because `buildDriverOptions` threads a published one
onto the read for the loader. The replacement witness is
`SqlDriver.ensureSequencesTable()`, which takes `parentTrx` for exactly this
reason and is pinned by `sql-driver-sqlite-tx-guard.test.ts`.
The same stale sentence appeared in the sibling doc comment on
`metadata-manager-degraded-list-cache.test.ts`; both are updated together so
the duplicate cannot re-seed the stale citation.
Comment-only — no behavior change.
@vercel

vercelBot commented Aug 11, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
ProjectDeploymentActionsUpdated (UTC)
objectstackIgnoredIgnoredAug 11, 2026 8:12pm

Request Review

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/metadata.

6 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:

  • content/docs/concepts/metadata-lifecycle.mdx(via @objectstack/metadata)
  • content/docs/kernel/cluster.mdx(via packages/metadata)
  • content/docs/kernel/services-checklist.mdx(via @objectstack/metadata)
  • content/docs/plugins/packages.mdx(via @objectstack/metadata)
  • content/docs/protocol/kernel/http-protocol.mdx(via @objectstack/metadata)
  • content/docs/protocol/kernel/metadata-service.mdx(via @objectstack/metadata)

2 release-owned page(s) also reference the affected code. These are read-only:

  • content/docs/releases/v12.mdx(via @objectstack/metadata)
  • content/docs/releases/v9.mdx(via @objectstack/metadata)

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.

Advisory only. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs origin/main → pass the list as args.docs.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/sskip-changesetPR has no user-facing published change; bypasses the changeset gatetests

Projects

None yet

2 participants

@huangyiirene@claude