Uh oh!
There was an error while loading. Please reload this page.
fix(metadata-protocol): auditMetaItem propagates a failed audit read instead of reporting an empty trail - #9786
Conversation
…instead of reporting an empty trail (#9638) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019yDEhPBC3tcGkW9bkce1HM
…it-read-catch-narrowing
📓 Docs Drift CheckThis PR changes 1 package(s): 2 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
⛔ 2 release-owned page(s) also name something this change touched. These are read-only:
What this run could not seeCoarse fallback — 7 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 351bc491e1ac25276f3f97cd8221a6b9e1fa758f && git checkout 351bc491e1ac25276f3f97cd8221a6b9e1fa758f
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 985a9cd2dbbad0bec9edce107f35d20791c9ac5c f28e676510b2a4bb452711207a34f4eccbe9baef && git checkout -B drift-repro 985a9cd2dbbad0bec9edce107f35d20791c9ac5c && git merge --no-ff f28e676510b2a4bb452711207a34f4eccbe9baef
node scripts/docs-audit/affected-docs.mjs --json 985a9cd2dbbad0bec9edce107f35d20791c9ac5c
|
Uh oh!
There was an error while loading. Please reload this page.
Fixes#9638.
The defect
packages/metadata-protocol/src/protocol.ts, thecatchclosing the read insideObjectStackProtocolImplementation.auditMetaItem(located by symbol — the file moves):The comment names two benign causes. The clause was unqualified and took every
other one with them — a connection drop, a permission denial, a malformed row, a query
bug, a timeout. All of them reached the caller as the well-formed statement "this item
has no audit entries".
ADR-0110 D3: a miss and a fault are different facts. This is the compliance surface —
auditMetaItemis the read behindGET /api/v1/meta/:type/:name/audit, which exists soStudio's audit-log tab can show who tried what and whether a lock blocked it. An empty
answer there reads as nobody touched this item. It is the same collapse #9426 / PR #9637
fixed at the route one layer up, and worse in one respect: the route's condition was a
static capability gap, while this is a transient read failure, so the same item can
report a full trail one minute and a clean one the next.
The measurement that shaped the fix
The dispatch flagged an assumption worth testing: the benign set may be two distinct
errors, not one. It is, and they are not the same kind of thing.
isMissingTableErrorno such table: sys_metadata_auditSQLITE_ERROR: no such table: …relation "sys_metadata_audit" does not existTable 'db.sys_metadata_audit' doesn't existfindTypeError: p.engine.find is not a functionconnect ECONNREFUSED 127.0.0.1:5432permission denied for table sys_metadata_auditquery timeoutSo a predicate written only against
isMissingTableErrorwould have been a fail-closedregression on the second benign cause — it would have started 503-ing the documented
"metadata-only store" deployment shape.
The change
Two limbs, because measurement says the two benign causes are different kinds of fact.
1. The capability limb is a precondition, not an error shape.
Asked before the
try, because it cannot be asked soundly inside the catch. A missingmethod raises
TypeError: … is not a function, and the only signal separating that from agenuine
TypeErrorraised inside a real driver'sfind— a null deref on a malformedrow, an actual fault — is the V8 message text. Sniffing that text would re-open exactly the
fail-open this card closes, one error class narrower. A
typeofprobe is a fact about theengine, not a guess about an error, so it cannot misclassify a fault as a capability gap.
This is the same shape as the limb one layer up: the
/auditroute's own capability probe(
typeof p.auditMetaItem !== 'function', #9426) likewise decides before the callrather than classifying its failure.
2. The catch now carries exactly one benign cause, which makes it byte-for-byte the
shape of the already-reviewed sibling
listCommits(#5980) in this same file:rethrowUnlessMetadataStoreUnprovisionedis this file's existing, declared spelling forthe propagating half — it asks the shared
isMissingTableErrorpredicate thatDatabaseLoader(#5108) andSysMetadataRepository(#4867) ask, and otherwise throwsmetadataStoreUnavailableError: 503 /SERVICE_UNAVAILABLEcarrying the driver error ascause. The route already wraps this call inhandleRouteError, which readserror.status, so the honest 5xx needs no change inpackages/rest.The
console.warnnow sits after the rethrow, so it fires only on the benign path.On the propagating half's vocabulary (#8901)
The dispatch asked me to check whether #8901 governs the spelling. It has not settled
one — it is
pm:on-hold, and it is about giving the gate's read-seam rule its owndeclared
FAILURE_PROPAGATION_*vocabulary, not about how a seam should spell its throw.Per the dispatch ("if it has not, say so and choose"), I chose the spelling this file
already uses on its sibling reads, adding no new vocabulary.
Scope — Option 2 not built, and no fork
Option 2 (a third wire state distinguishing "read failed") is not commissioned and is
not here. No response schema is widened; no
packages/specfile is touched. The narrow-catchcan express the fix without a response-shape or contract change, so the fork clause does
not bind:
{ events: [] }still means exactly what it documented, and the only change isthat a fault stops being spelled as one.
The pin — both halves
packages/metadata-protocol/src/protocol.audit-read-failure-propagation.test.ts, 13 cases.Both directions, because a method that raised unconditionally would satisfy the first half
and destroy the documented feature:
timeout) each raise, asserted on the ADR-0112 pair
code+status(
SERVICE_UNAVAILABLE+ 503) rather than a baretoThrow, which could not separate"answered with the wrong body" from "did not raise at all" — and the wrong body is the
defect. Plus: the driver error rides as
cause, the status sits in the 5xx bandhandleRouteErroracts on, and aTypeErrorfrom a driver that does havefindis afault, not a capability gap;
{ events: [] }in all four driverphrasings, and a host engine with no
findstill answers{ events: [] }.Anti-vacuity
An "it propagates" assertion is worthless if the assertions cannot tell a populated trail
from an empty one — the
body.item.fieldsvsbody.data.item.fieldsshape this repo hasbeen bitten by. So the file carries a positive control: a real row is read all the way
through the mapping and its fields asserted (
actor,outcome,lockState,lockOverridden,requestId,note), plus an explicit equivalence pin that a genuinezero-row read and a fault are no longer the same answer.
Reverse verification — direction predicted in writing before running
Predicted: reverting
protocol.tsturns 7 of 13 red — the three propagation flavours,the
causepin, the 5xx-band pin, the driver-TypeErrorpin and the equivalence pin —while the six benign/positive-control cases stay green (the ablation touches neither
the success path nor either benign answer), and
protocol.audit-org-scope.test.ts(#8747,same method) stays fully green because every engine it supplies has
findand resolves.Observed, exactly that — 7 red, 6 green, org-scope 7/7 green:
The fix was committed before the ablation and restored with
git checkout HEAD --,then proved byte-identical (
git diff --exit-code, exit 0, clean tree). The tests import./protocol.jsrelative in-package, so vitest resolvessrc/directly — nodistisinvolved and the red proves the ablation reached the code under test.
Read-coupling with #9657 — measured post-merge, not pre-merge
check:durability-log-levelis the gate that judges exactly this catch, and devx card#9657 changed its matcher (PR #9750) while this was in flight.
origin/mainwas mergedinto this branch at
e9534a4acand the gate re-run after the merge, so the readingbelow is against the new callee-reading matcher, not the one this branch forked from:
Counts unmoved in both directions, and no baseline was raised or added.
this gate was green before this fix too. The old catch was loud — it called
console.warn— and the read-seam rule asks whether a catch invents a silent answer, notwhether the answer it invents is correct. So the gate could not have caught this defect,
which is precisely the expressiveness gap #8901 exists to record. It neither helped nor
fought this change.
Verification — all at
f28e67651, the final commit (post-merge)Suite arithmetic, stated because the file count does not move on its own: the pre-change
baseline was 124 files / 1684 tests, and this branch adds one file of 13 cases → 125 / 1697.
@objectstack/metadata-protocoldeclares notypecheckscript — it is one of the 13packages on the DEBT ledger, so
pnpm --filter … typecheckexits 1 withERR_PNPM_RECURSIVE_RUN_NO_SCRIPTrather than silently passing. Type coverage for thischange is therefore carried by the ledger ratchet below and by the package's tsup DTS build
(exit 0), not by a per-package
tsc --noEmit.Gates re-derived from the actual changed paths with
node scripts/pm/dispatch-gates.mjs(no paths passed — the script derives its own change set from the merge base). All five
dispatched gates matched; the derivation added ten beyond them — the changeset family
(
check:changeset-gate-self-tests,check:objectui-changeset,check-adr-0087-registration.mjs,check-changeset-no-major.mjs,check-empty-changeset.mjs) and the convention-triggered family a new test file pulls in(
check:query-options-erasure,check:type-check-coverage,check:type-check-debt,check:engine-double-contract,check:where-matcher). All run post-merge, all green:Ratchet family at the final head, after the full closure build
(
pnpm exec turbo run build --filter=./packages/* --filter=./packages/*/*, 70/70 successful):No baseline was raised or added anywhere in this PR.
Generated by Claude Code