From 88ebdc9194420f839eaec58c096da7d14a408096 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 11 Aug 2026 20:12:06 +0000 Subject: [PATCH] docs(metadata): re-anchor the single-connection-pool hazard on a verified live witness (#7708) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- ...tadata-manager-degraded-list-cache.test.ts | 24 ++++++---- packages/metadata/src/metadata-manager.ts | 44 ++++++++++++++----- 2 files changed, 50 insertions(+), 18 deletions(-) diff --git a/packages/metadata/src/metadata-manager-degraded-list-cache.test.ts b/packages/metadata/src/metadata-manager-degraded-list-cache.test.ts index e4c9a8a7f1..df27cc617b 100644 --- a/packages/metadata/src/metadata-manager-degraded-list-cache.test.ts +++ b/packages/metadata/src/metadata-manager-degraded-list-cache.test.ts @@ -23,14 +23,22 @@ * where `DatabaseLoader`'s `engine.find('sys_metadata', …)` tries to acquire a * second knex connection while the transaction holds SQLite's only one, and * knex waits out `acquireConnectionTimeout` (60s). That hazard is live on the - * current stack: `DatabaseLoader._find()` still does not thread the caller's - * transaction, and `driver-sql` still models SQLite as a single-connection pool - * (`activeTransactions`, `assertBareKnexSafe` — a dev/test guard that no-ops in - * production, so production still eats the timeout), which is why - * `plugin-audit`'s `captureBefore` threads the transaction by hand. Refusing to - * cache degraded reads would swap one 30s silent window for a 60s stall *per - * call*. So the policy is: cache it, but mark it `degraded` and expire it on a - * far shorter TTL. + * current stack, re-measured under #7708 on 2026-08-11 against a real + * `ObjectQL` + `SqlDriver` (better-sqlite3): `DatabaseLoader._find()` still + * does not thread the caller's transaction, and `driver-sql` still models + * SQLite as a single-connection pool (`activeTransactions`, + * `assertBareKnexSafe` — a dev/test guard that no-ops in production, so + * production still eats the timeout). The re-measurement narrowed it to a + * CONDITION rather than retiring it: a transaction opened directly on the + * driver stalls the full 60s and then throws knex's acquire-timeout, while one + * opened through `engine.transaction()` returns at once because the engine + * publishes it into the ambient `txStore` (ADR-0034) and threads it onto the + * read. `SqlDriver.ensureSequencesTable()` is the live witness that this is + * worth designing against — it takes `parentTrx` for exactly this reason. (The + * witness cited here until #7708 was `plugin-audit`'s `captureBefore`, retired + * by #6656; it was replaced, not dropped.) Refusing to cache degraded reads + * would swap one 30s silent window for a 60s stall *per call*. So the policy + * is: cache it, but mark it `degraded` and expire it on a far shorter TTL. * * These tests pin all three halves of that: the flag exists on the entry, the * degraded TTL is short, and the healthy TTL is untouched. diff --git a/packages/metadata/src/metadata-manager.ts b/packages/metadata/src/metadata-manager.ts index 946df8861a..4de2cf65ad 100644 --- a/packages/metadata/src/metadata-manager.ts +++ b/packages/metadata/src/metadata-manager.ts @@ -331,16 +331,40 @@ export class MetadataManager implements IMetadataService { // above. The concurrent half is delivered by `inflightListReads` below, which // is why the two fields are one policy and are documented together. // - // [#5184] That hazard is NOT historical — it was re-verified on the current - // driver stack before this policy was chosen. `DatabaseLoader._find()` still - // issues `engine.find('sys_metadata', …)` without threading the caller's - // transaction, and `driver-sql` still treats SQLite as a single-connection - // pool (`activeTransactions`, `assertBareKnexSafe` — the latter a dev/test - // guard that is a no-op in production, so production still waits the timeout - // out). `plugin-audit`'s `captureBefore` threads the transaction by hand for - // exactly this reason. Hence the policy below keeps caching degraded reads - // rather than skipping them: "don't cache a degraded read" would trade one - // 30s silent window for a fresh 60s stall per call. + // [#5184; re-measured under #7708 on 2026-08-11] That hazard is NOT + // historical — and the re-measurement NARROWED it rather than retiring it. + // Measured on the current stack: real `ObjectQL` + real `SqlDriver` + // (better-sqlite3), a real `DatabaseLoader.list()` with the loader's own + // cache off, `knex.client.pool.max === 1` confirmed for the SQLite dialect + // and `acquireConnectionTimeout` left at the knex default of 60s. + // + // • Transaction opened DIRECTLY on the driver (`driver.beginTransaction()`) + // → the read STALLS for the full timeout and then throws knex's "Timeout + // acquiring a connection" (measured: 60_085ms). `DatabaseLoader._find()` + // forwards no options, so nothing threads the caller's transaction, and + // `driver-sql` still models SQLite as a single-connection pool + // (`activeTransactions`, `assertBareKnexSafe` — the latter a dev/test + // guard that is a no-op in production, so production still waits the + // timeout out). `list()` catches that throw and degrades, which is + // precisely the entry whose TTL this policy is choosing. + // • Transaction opened through `engine.transaction()` / `ScopedContext` + // → returns immediately (measured: 12ms, with `activeTransactions === 1` + // and the driver observably receiving the handle on the call). Those + // publish the transaction into the engine's ambient `txStore` (ADR-0034) + // and `buildDriverOptions` threads it onto the read for the loader. + // + // So the stall shape is live but CONDITIONAL: it needs an open transaction + // that the engine's ambient store cannot see. `SqlDriver.ensureSequencesTable()` + // is the live witness that this is worth designing against — it takes + // `parentTrx` and runs its DDL on the caller's transaction for exactly this + // reason, with `assertBareKnexSafe` as the tripwire for callers that forget; + // `sql-driver-sqlite-tx-guard.test.ts` pins both halves. (Until #7708 the + // witness cited here was `plugin-audit`'s `captureBefore`, retired by #6656. + // It was REPLACED rather than dropped: the example died, the hazard did not.) + // + // Hence the policy below keeps caching degraded reads rather than skipping + // them: "don't cache a degraded read" would trade one 30s silent window for + // a fresh 60s stall per call on every caller in the first bullet. // // [#5184] WHAT IS ACTUALLY CACHED, AND FOR HOW LONG — this paragraph is the // contract, and it describes `cacheListResult()` / `readCachedList()` below.