Skip to content

fix(metadata-protocol): report a failed sys_metadata_commit write instead of swallowing it (#9066) - #9159

Merged
os-zhuang merged 1 commit into
mainfrom
claude/issue-9066-recordcommit-durability
Aug 16, 2026
Merged

fix(metadata-protocol): report a failed sys_metadata_commit write instead of swallowing it (#9066)#9159
os-zhuang merged 1 commit into
mainfrom
claude/issue-9066-recordcommit-durability

Conversation

@os-zhuang

Copy link
Copy Markdown
Contributor

Fixes#9066

What was wrong

recordPackageCommit in packages/metadata-protocol/src/protocol.ts — the ADR-0067 commit writer publishPackageDrafts calls with the revert plan it captured a few lines earlier — sat behind a bare catch that answered null for every reason, with nothing logged. Its comment's premise was true (the publish already succeeded and cannot be unwound) but its conclusion, "grouping is a best-effort overlay", understated the row: sys_metadata_commit is the ONLY record of a turn's revert plan (existedBefore / prevVersion per artifact) that revertCommit and rollbackToPackageCommit can act on.

So when the insert failed: the artifacts went live, the response read success: true with commitId merely absent, the turn could never be reverted, and no line anywhere said so. A commit store that was failing kept failing, and every later publish lost its plan the same silent way — the AGENTS.md durability-degradation shape exactly.

What changed

The failure is discriminated by error TYPE, through the shared isMissingTableError predicate the read seams in this same file already ask (#5532 / #5980 / #8896 — it was already imported in protocol.ts, unlike the sibling card #8906 where a new import was needed):

  • unprovisionedsys_metadata_commit (a first boot, or an env kernel composed without the commit log) is a configuration fact, identical on every publish and fixed in one place: console.info, said once per protocol instance, naming the consequence and how to provision the store;
  • everything else (connection drop, timeout, permission denial, schema drift on that table): console.error, once per turn, carrying the package, the operation, the item count, the driver's own reason, that the publish itself succeeded and still reports success, and the fix.

error per turn and info once are deliberately different dedupe policies: each error line is a different turn whose revert plan was lost, so collapsing them would hide how many turns are unrevertible; the info sentence is the same configuration fact every time.

Publish semantics are untouched, and the tests assert it rather than assume it: the catch still returns null, the publish still answers success: true with the artifacts live, and commitId is still merely ABSENT. Option 3 — telling the caller the turn is unrevertible — is NOT done here; that is a response-field question the #8896 ruling forbids for this family and it stays undecided.

The bounding sweep — the gate that stops this regressing

AGENTS.md's degradation-log-level rule says a newly found seam is added to DURABILITY_CRITICAL_CALLEES in the same PR that fixes it. That vocabulary matches callee NAMES and insert is far too generic to declare repo-wide, so the insert now goes through a named persistPackageCommitRow — the same shape persistAuditTrailRow / dropPromotedDraftRow already take in that vocabulary — and the seam is declared in scripts/check-durability-degradation-log-level.mjs. Nothing else in the repo is named that, so the entry's blast radius is this one seam.

Proved in both directions, not assumed:

# with the fix
packages/metadata-protocol/src/protocol.ts:15721 guards persistPackageCommitRow()@15705 -> loud (error@15762)
check-durability: 26 durability-critical catch seam(s), all loud, rethrowing or propagating (exit 0)
# with the pre-fix bare catch restored (ablation)
X 1 durability-critical catch(es) degrade quietly ... found: catch swallows the failure with no log at all (exit 1)

Verification

Union re-derived against the actual changed paths with node scripts/pm/dispatch-gates.mjs, run at bf19fe51e (git rev-parse --short HEAD from that run — the final commit):

gateresult
pnpm --filter '@objectstack/metadata-protocol^...' build (dependency closure, prefix = upstream deps)exit 0
@objectstack/metadata-protocol full suite113 files, 1574 tests passed
new file alone7 passed
check:durability-log-level26 seams, all loud; read-seam rule 66 seams, clean
check:engine-double-contractOK, 313 pinned (new double routes through assertEngineDeleteDispatch / assertEngineUpdateDispatch)
check:where-matcher248 matchers, none silently wrong, none new
check:query-options-erasureratchet holds, none new
check:cross-package-test-inputsOK
check:changeset-gate-self-tests, check-empty-changeset, check-changeset-no-major, check-adr-0087-registration, check:objectui-changesetall OK
check:filter-alias-parityOK
check:type-check-coverageOK (structural half)
check:nul-bytesOK, 5997 files

Reverse verification (fix committed first, then ablated to the pre-fix bare catch, then restored byte-identically via git checkout of the branch path): 6 of the 7 new tests went red — expected "error" to be called 1 times, but got 0 times, expected "info" to be called 1 times, but got 0 times — and the positive control (a healthy publish records the row and logs nothing) stayed green, which is what makes the six failures attributable to the seam and not to a broken fixture. Green again after restore: 7 passed.

check:type-check-debt --re-measure needs the whole workspace dist/ closure built; that build was still queued behind other agents on the shared verification lock when this PR went up, so it is left to CI. @objectstack/metadata-protocol carries a TEST_DEBT entry (63 frozen errors), and the new test file is written against the same fixture shape as its neighbours.

Changed line ranges (hot file)

packages/metadata-protocol/src/protocol.ts is shared with #9009 this round. This PR's actual changed ranges, post-change: 15632-15687 (the new dedupe flag, persistPackageCommitRow, and the rewritten JSDoc), 15705 (one line: the insert call now goes through the wrapper), 15721-15770 (the discriminated catch). All inside recordPackageCommit's own neighbourhood in the ADR-0067 section; getEffectiveLock is untouched.


Generated by Claude Code

…tead of swallowing it (#9066)
`recordPackageCommit`'s bare `catch` answered `null` for every reason with
nothing logged. The publish really had succeeded — but the commit row is the
only record of the turn's revert plan (`existedBefore`/`prevVersion` per
artifact), so a failed insert left live artifacts, `success: true` with
`commitId` merely absent, a turn that can never be reverted, and no line
anywhere saying so.
Discriminated by error TYPE through the shared `isMissingTableError`
predicate: an unprovisioned commit store is informational and said once per
protocol instance; every other failure logs at `error` per turn with the
package, the operation, the item count, the driver's reason, the consequence
and the fix. Publish semantics are untouched — the catch still returns `null`
and no response field was added.
The insert moves behind a named `persistPackageCommitRow` so the seam can be
declared in `DURABILITY_CRITICAL_CALLEES`; `check:durability-log-level` now
fails if this catch is ever quieted again.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NTKPDRoynY8i3HmdSFUxFj
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

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

3 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-protocol)
  • content/docs/kernel/services-checklist.mdx(via @objectstack/metadata-protocol)
  • content/docs/protocol/kernel/http-protocol.mdx(via @objectstack/metadata-protocol)

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

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

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

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

recordCommit swallows a failed sys_metadata_commit write — the publish reports success and the turn is silently not revertible

2 participants

@os-zhuang@claude