fix(metadata-protocol): restoreVersion carries the row's package binding — a package-bound rollback stops 409ing (#6215) - #6574
Merged
Conversation
…ing (#6215) restoreVersion read the active row package-agnostically and then re-put the historical body without stating a packageId. `put` scopes its optimistic-lock lookup by package and an unstated packageId resolves to the UNBOUND row (`package_id IS NULL`), so for a package-bound overlay the lock looked up a row that does not exist, read a null parent hash, and threw ConflictError — every rollback of a row authored in a Studio package workspace answered 409 "advanced during rollback" while nothing had advanced. Both user-facing callers (rollbackMetaItem, revertCommit) go through this one call. restoreVersion now takes both the parent hash and the ADR-0048 package_id from one raw read of the active row and states the binding on the write, as promoteDraft already did — closing the second face too (an insert of a duplicate unbound row instead of an update of the bound one). Tests: the #4636 PR1 tripwire is flipped to the staged assertion (a package-bound rollback succeeds AND re-registers under the row's own package key), revertCommit gains a real-repository pin on a package-bound row, and both callers keep a genuine-conflict refusal. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KDU3qAuJyajAQm3GkUXdfA
#6215) - protocol-writepath-object-ownership: the #4636 PR1 tripwire is replaced by the assertion staged one test above it — a package-bound rollback succeeds, the registry write-through carries the row's OWN package id, the restored body is what the registry serves, and exactly one row (still bound) survives, which is the defect's duplicate-unbound-row second face. - protocol-commit-history: revertCommit's restore limb driven against the REAL SysMetadataRepository over a package-aware engine double, plus the package-less control. - Both callers keep a genuine-conflict refusal: a row that really advanced between the parent read and the optimistic-lock read is still refused (METADATA_CONFLICT / 409 at the throwing surface, code in revertCommit's failed[] record). The race is staged on the in-transaction read rather than by counting reads, so the pin cannot rot into a green that asserts nothing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KDU3qAuJyajAQm3GkUXdfA
…tore-version-package-scope
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
📓 Docs Drift CheckThis PR changes 1 package(s): 4 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
|
baozhoutao
marked this pull request as ready for review
August 8, 2026 06:00
This was referenced Aug 8, 2026
This was referenced Aug 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #6215
What was wrong
Every rollback of a metadata item authored inside a Studio package workspace failed, and failed by blaming an edit that never happened:
Both user-facing paths were affected, because both are one call:
rollbackMetaItem(the per-item version-history revert) andrevertCommit(the ADR-0067 package-commit revert) go throughSysMetadataRepository.restoreVersion. Only rows with no package binding rolled back at all, and that is the legacy shape — ADR-0070 pushes authoring toward always resolving a writable base package, so the failing share was growing.Cause, re-verified on
origin/mainbefore implementingBoth halves of the issue's premise still held verbatim at
3510e4a:restoreVersionread the current active row package-agnostically (this.get(ref, { state: 'active' })) and re-put the historical body with nopackageId;putscoped its optimistic-lock lookup withwhereFor(ref, state, opts.packageId ?? null), andwhereFortreatsnullas a predicate (package_id IS NULL), not as "any package".So the two reads of one restore disagreed by construction: the parent hash came from the row whatever its binding, the lock looked for a row bound to nothing. For
app.myappthe lock found nothing, readexistingHashasnull, compared it against the real hash, and threwConflictError. The mismatch was never between two writers.The fix (route A, the issue body's preferred answer)
restoreVersionnow reads the raw active row once and takes both facts from it — the parent hash and the ADR-0048package_id— then states that binding on the write. This is the idiompromoteDraftalready uses for the same reason (it was fixed for the draft-promotion path and left the restore path untouched). The row the optimistic lock is taken on is therefore, by construction, the row that gets written.This also closes the defect's second face named in the issue: had the parent check ever passed,
putwould have found no row in itsIS NULLscope and inserted a duplicate unbound row beside the bound one rather than updating it.sys_metadata's partial unique index keys onCOALESCE(package_id,''), so a real database would have accepted that duplicate too.A missing active row (deleted, or never published) still yields
null— the unbound row. That is the only defined answer, becausesys_metadata_historycarries nopackage_idcolumn, so a vanished binding is not recoverable.On route B, and why the
?? nullcollapse is documented rather than flippedThe ruling asked that the
?? nullat that one call site stop erasing "unspecified". The route the issue offered for that — copy the sibling read's'packageId' in optsidiom, so an omitted key means "any package" — is measurably not safe to apply alone, and this is the one place this PR departs from the issue's suggestion:saveMetaItemis the only other caller, and it omits the key for a package-less save (...(request.packageId !== undefined ? { packageId } : {})) while its own parent-hash read two lines above scopes toIS NULL(packageId: request.packageId ?? null). Flippingput's default to "any package" desynchronizes exactly those two reads: a package-less save over a package-bound row would then read anullparent and find a real one, i.e. turn today's silent duplicate-row fork into a spurious 409 on a path this issue does not own. TheMetadataRepositorycontract also documents the omitted case as env-local ("Omit/undefined for env-local overlays"), andcontract-suite.tsexercises it that way throughout.So
putkeeps its documented default, and the line is made non-silent instead: the scope is resolved once into a namedtargetPackageIdused by both the lock lookup and thepackage_idstamp, with the asymmetry againstgetwritten down (a write upserts exactly one row, so its scope is a concrete package or the unbound row — never "any package"). After this PR no caller reachesputmeaning a package and saying nothing.Tests
The tripwire swap. #4636 PR1 left
[tripwire] a package-bound rollback still 409sprecisely so this fix would trip it. It is replaced by the assertion staged one test above it, on the half that was unreachable then — a package-bound rollback succeeds, the registry write-through carries the row's own package id (getAllObjects(APP_PKG)still matches), the registry serves the restored body,_provenanceis still stampedorg, and exactly one row survives, still bound, carrying v1 — which is the duplicate-unbound-row second face asserted rather than described.The second caller.
protocol-commit-history.test.tsgainsrevertCommitdriven against the realSysMetadataRepositoryover a package-aware engine double (both write verbs pinned to ObjectQL's own dispatch predicates), plus the package-less control that proves the legacy shape is not regressed.The refusals survive. A row that genuinely advanced between the parent read and the write is still refused at both callers —
{ code: 'METADATA_CONFLICT', status: 409 }at the throwing surface (rollbackMetaItem), andcode: 'METADATA_CONFLICT'inrevertCommit'sfailed[]record, whose declared shape carries nostatus. The race is staged on the read that runs inside the write transaction (identified by itscontext, not by counting reads) and each test asserts the trap actually fired, so neither can rot into a green that asserts nothing.Reverse verification, direction decided first: revert the implementation, keep the tests. Measured — the two success pins go red, the rollback one with the issue's own message byte for byte (
advanced during rollback ... current is null); the two refusal pins go red onexpect(fired).toBe(true), because pre-fix theIS NULL-scoped lookup returns no row and the staged race cannot even happen; the package-less control stays green in both directions, as a control should.Verification
pnpm --filter @objectstack/objectql --filter @objectstack/metadata-protocol --filter @objectstack/metadata-core --filter @objectstack/metadata test— 147/56/8/26 files, 2471/584/103/519 tests, all passing (re-run after mergingmain).rest885,runtime1606,client263,service-package4 — all passing.turbo run typecheckacrosspackages/*,packages/*/*,apps/*— 120 tasks green;check:type-check-debtre-measured with the full closure built, none above its recorded number.check:*step enumerated from.github/workflows/lint.ymlrun one by one — all pass, includingcheck:engine-double-contract(the new double) andcheck:nul-bytes.Out of scope, filed
objectoverlay at all — it passes no write intent, so restoreVersion defaults tooverride-artifactand the repository answers NOT_OVERRIDABLE #6563 —revertCommitpasses no write intent, sorestoreVersiondefaults tooverride-artifactand the repository answersNOT_OVERRIDABLEfor everyobjectitem in a reverted commit;rollbackMetaItemderivesruntime-onlyand does not. Found while writing these pins (it is why therevertCommitpin usesview: anobjectcannot reach the scoping line this PR fixes). Different cause, different line, outside this issue's ruled file surface.Generated by Claude Code