fix(ocap-kernel): make c-list import accounting symmetric - #1020
Open
sirtimid wants to merge 5 commits into
Open
fix(ocap-kernel): make c-list import accounting symmetric#1020sirtimid wants to merge 5 commits into
sirtimid wants to merge 5 commits into
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
sirtimid
added a commit
that referenced
this pull request
Aug 13, 2026
…ring `retireKernelObjects` deletes an object and queues a `retireImport` for each importer in the same breath, so until that action is delivered an importer's c-list entry names a kref the kernel has already dropped. The audit counted those entries as holders and reported a violation against the collector's own output — and since `assertRefCountsIfAuditing` throws from inside the crank, that killed the run loop for good. Reachable from an ordinary `terminateVat` while a surviving vat holds the dying vat's export in liveslots' dropped-but-recognizable state. No current test produced it; found by Cursor Bugbot on #1020 and reproduced against the real store. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Member
Author
Confirmed and fixed in Reproduced against the real store ( The ordering is as reported: Fix: One correction to the report's framing: Exposure: real but previously unexercised. Nothing in the suite produced this state, which is why it was green. Nothing exotic is needed either — terminating a vat while a surviving vat holds its export in liveslots' normal dropped-but-still-recognizing state is enough. Pinned by |
This was referenced Aug 13, 2026
Creating an import c-list entry changed no refcount while tearing one
down decremented both, and `initKernelObject` compensated by minting
every object at (1, 1). That constant is correct for exactly one
importer, which is why nothing caught it: with two importers a live
capability gets dropped and retired out from under a holder, and the
same unit is claimed by both an importer's drop and the owner's
termination, so cleanup underflows and leaves a vat half-cleaned.
Restore the increment and rebase the baseline to (0, 0), matching
SwingSet, so `collectGarbage` — already a faithful port — receives the
inputs it was written for.
Build the invariant checker first, since every existing compensation
becomes a double-count the moment the increment lands. It recomputes
each kref's counts from ground truth (c-list entries and their reachable
flags, run-queue and promise-queue messages, promise resolution values,
pins) and reports drift in both directions: too low collects a live
capability, too high leaks it. Enabled via `Kernel.make`'s
`auditRefCounts` and run after every crank; on in kernel-test.
The audit found four more unbalanced paths that the phantom baseline had
been absorbing, each fixed here: a delivered message charged its target
against the routed kref rather than the run-queue item's own, so a
message routed through a resolved promise decremented an object nobody
charged and leaked the promise; a notification leaked its reference on
both early-return paths and decremented promises retired alongside it
that nobody had taken; a message queued on an unresolved promise
duplicated every reference it carried on re-enqueue; and `resolve|kpid`
incremented with no matching release.
Two things the baseline was silently standing in for, now explicit: vat
roots are pinned for the lifetime of their vat (a root is addressable
whether or not anyone imports it), and GC action delivery moves the
kernel's own c-list so a dropped export's flag clears and retired
entries don't outlive their objects.
Also fixes the stale `cle.`/`clk.` key prefixes in
`getPromisesByDecider` and `deleteEndpoint`, which stopped matching the
`${endpointId}.c.` layout. `getPromisesByDecider` matched nothing, so
promises a terminating vat was deciding were never rejected — load
bearing here, because releasing a promise's unsettled reference is what
makes the cleanup path's accounting add up.
Refcounts are persisted, so counts written under the old scheme are
recomputed from ground truth on first open, keyed off a new
`refCountScheme` entry.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Prettier wanted a blank line before the entry following a nested bullet, and the entries still cited #1010, which this PR replaces.
…ring `retireKernelObjects` deletes an object and queues a `retireImport` for each importer in the same breath, so until that action is delivered an importer's c-list entry names a kref the kernel has already dropped. The audit counted those entries as holders and reported a violation against the collector's own output — and since `assertRefCountsIfAuditing` throws from inside the crank, that killed the run loop for good. Reachable from an ordinary `terminateVat` while a surviving vat holds the dying vat's export in liveslots' dropped-but-recognizable state. No current test produced it; found by Cursor Bugbot on #1020 and reproduced against the real store. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
sirtimid
force-pushed
the
sirtimid/clist-refcount-symmetry
branch
from
August 13, 2026 17:46
5e3be43 to
1f9c888
Compare
Rebasing the baseline to (0, 0) made every reference explicit, which exposed the holders that were never references at all. An ocap URL carries its kref inside an encrypted bearer token and nothing else, so the kernel cannot discover from its own state that a holder exists: `issueOcapURL` took no reference of any kind. Under the old baseline nothing exported was collectable and it never showed; at (0, 0) the target is collected as soon as the message that carried it to the issuer is delivered, and the URL names a dead capability. The audit is silent on it by construction — the object genuinely has no holder it can see. Retain the target when the URL is issued, before the token exists, since the token is unretractable once it does. One pin per kref however many URLs name it, and no release: the token is persistent and unexpiring, so `revoke` is how the capability dies. Pinning also puts the holder inside the reference graph, so the audit can see it rather than being taught to excuse it. The same shape had a second door. `incrementRefCount` has no `kernelRefExists` guard where `decrementRefCount` does, so importing a deleted kref read its missing counts as (0, 0) and wrote them back, resurrecting a live-looking object with no owner — deliverable to by nobody, and endorsed by the audit, since the new c-list entry is a legitimate holder for exactly the count it finds. Reached by redeeming a URL issued for an object since collected. Guard the point of corruption, `translateRefKtoE`, rather than `incrementRefCount` itself: creating an entry for a deleted kref is the invariant, and releasing a reference to something already gone is how GC teardown is allowed to race deletion. Also release a vat's root pin when `deleteSubcluster` retires vats that never ran here. It bypasses `stopVat`, so nothing released the pin `launchVat` took in the incarnation that did run them, leaving the root's count permanently above zero and `pinnedObjects` naming a vat that no longer exists. `stopVat` and `deleteSubcluster` now share `releaseVatRootPin`. Vat root pinning had no unit coverage at all, so pin-on-launch, release-on-terminate and keep-across-restart are asserted now; the last is what the comment claims and what would break silently. Restores the `maybeFreeKrefs` assertion on `forgetEndpointImports`' ownership-migrated branch, which lost its `not.toHaveBeenCalled` when that branch stopped returning early. Corrects three claims that the (0, 0) birth falsified and that shipped as documentation: both `KernelServiceManager` comments asserting its delete branch cannot fire, when it now does, and a changelog entry asserting (1, 1) birth two dozen lines above one asserting (0, 0). `recomputeRefCounts` no longer describes itself as a migration; nothing calls it, and opening an existing store does not migrate one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Retaining before minting is right: minting awaits, so a collection crank can run in that window. But nothing undid the retention when minting then failed. A rejected kernel-service call is reported to the caller rather than thrown out of the crank, so the crank commits and the pin outlives the kernel that took it, naming a URL that never existed. retainForOcapURL now reports whether this call took the pin, and undoOcapURLRetention unwinds one that never backed a URL. Guarded on the ledger rather than the pin list, so it can only remove the pin it put there: a kref some live URL already names keeps the pin that URL depends on, and a vat root keeps its lifetime pin. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 051d772. Configure here.
| this.#kernelStore.undoOcapURLRetention(kref); | ||
| } | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
Failed mint can unpin live URL
Medium Severity
issueOcapURL treats retainForOcapURL returning true as exclusive ownership of the pin, then undoOcapURLRetention on mint failure. A second overlapping issue for the same kref sees the entry already recorded and takes no pin. Kernel service calls are not awaited, and minting yields on cipher.encrypt, so another crank can mint successfully while the first still holds retained === true. If that first call then fails, it unpins the kref a live URL now depends on, and collection can take the capability.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 051d772. Configure here.
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
1 participant
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.
Closes #1006. Replaces #1010, which carried this plus three unrelated fixes; it is split into four PRs, this one first.
The defect
Creating an import c-list entry changed no refcount; tearing one down decremented both
reachableandrecognizable.initKernelObjectcompensated by minting every object at(1, 1), which is exactly right for one importer — the only topology our tests exercised. There is nosetReachableFlagin the repo; it was never ported.That single unit was also claimed by two parties: importer-side (
object.ts: born at 1 "on the assumption that the new object corresponds to an object that has just been imported") and owner-side (vat.ts: "the baseline decrement below corresponds to the implicit referenceexportFromEndpointinstalled…"). Both an importer's drop and the owner's termination were entitled to spend it.All four symptoms in the issue reproduced against the real store before the fix, and are covered by regression tests now.
main has since grown a second compensation for this
While this was in review, #983 landed this in
cleanupTerminatedVat:That is a guard against the phantom baseline, at the same site this PR deletes the baseline decrement outright. This branch removes it; the condition is moot once no phantom unit exists. #983's parallel-launch tests pass unchanged under the audit.
Approach
Followed the issue's proposed path, in order.
Step 1 — the invariant checker, first.
store/methods/refcount-audit.tsrecomputes each kref's counts from ground truth — c-list entries and their reachable flags, run-queue and promise-queue messages, promise resolution values, pins — and reports drift in both directions: too low collects a live capability, too high leaks it (the issue's symptom 4 would pass an underflow-only check). The credits mirrorincrementRefCountcase for case.Enabled per kernel via
Kernel.make({ auditRefCounts: true }), run after every crank, and on for every kernelkernel-testbuilds — so a violation fails the build.Step 2 — restore the increment, rebase the baseline.
initKernelObject→(0, 0);addCListEntrytakes the entry's reference, mirroringdeleteCListEntry; newsetReachableFlag; owner-side baseline decrements deleted.collectGarbageis already a faithful port ofprocessRefcounts, so this hands it the inputs it was written for.Step 3 — remove the compensations. This is where the checker earned its keep. It found four more unbalanced paths the phantom baseline had been absorbing:
#deliverSendcharged the target against the routed kref, not the run-queue item's own. For a message routed through a resolved promise those differ, so it decremented an object nobody charged and leaked the promise.#deliverNotifyreleased its reference only on the success path, leaking it on both early returns, and decremented promises retired alongside it that nobody had taken.resolve|kpidincremented with no matching release. (I had assumedresolve|decidercancelled it; that releases the distinct unsettled-promise reference.)Two things the baseline was silently standing in for, now explicit:
pinVatRootalready existed and was never called internally.dropExportsclears the owner's flag,retireExports/retireImportstear the entry down.krefsToExistingErefs→krefsToErefs, which throws rather than silently dropping an unmapped kref.Judgment call worth review
The
gc.ts:169assert is not re-enabled. The issue asks for it; I believe it would fire legitimately. Left as a comment explaining why, and the audit covers the same ground from outside.What moved to the other PRs in this stack
This is the first of four. The rest are being prepared now and will be linked here as they open; #1010, #1011, #1012 and #1018 stay open until then, so nothing looks dropped.
rollbackCrank'sfinallyinto atry/catch, this branch changedctx.savepointsfromstring[]to{name, maybeFreeKrefs}[]on the same lines, and composed naively the rethrow fires before themaybeFreeKrefsrestore — a hole neither PR could see alone.Reviewing in order is worthwhile; each one's diff is much smaller than #1010's was.
Testing
yarn lintclean,yarn build31/31.@metamask/ocap-kerneland@ocap/kernel-testfully green, withauditRefCountson for every kernelkernel-testbuilds.Checklist
README.md,CHANGELOG.md) as appropriateNote
High Risk
Touches core capability GC, refcount invariants, and breaking object birth semantics; incorrect accounting can collect live objects or leak capabilities, though auditing and extensive new tests mitigate regression risk.
Overview
BREAKING: Kernel objects are created at
(0, 0)instead of(1, 1), and creating an import c-list entry now takes a reference (matching teardown). Owner-side “baseline” decrements in vat/peer cleanup are removed.krefsToExistingErefsis renamed tokrefsToErefsand throws on unmapped krefs.Adds reference-count auditing (
auditRefCounts,recomputeRefCounts, optionalKernel.make({ auditRefCounts })) that recomputes counts from visible holders and runsassertRefCountsIfAuditingafter every crank; kernel tests enable this by default.Reachability and lifetime: New
setReachableFlag;translateRefKtoEre-establishes reachability when a dropped import is handed over again. Vat roots are pinned for the vat’s lifetime (released on termination / subcluster delete withoutstopVat). OCAP URL issuance retains the target viaretainForOcapURL(with undo on mint failure); deleted krefs cannot be imported or issued.Delivery / queue accounting fixes: GC actions now update the kernel’s c-list (
dropExportsclears reachable; retire tears entries down). Send delivery charges the run-queue item’s target, not the routed target. Promise re-queue transfers refs instead of duplicating; notify and promise-queue paths fix several leak/double-decrement bugs.getPromisesByDeciderscans the correct${endpointId}.c.layout (fixes missed promise rejection on peer restart).Tests and e2e expectations are updated for the new counts; new regression coverage for multi-importer GC, c-list accounting, and refcount audit.
Reviewed by Cursor Bugbot for commit 051d772. Bugbot is set up for automated code reviews on this repo. Configure here.