Uh oh!
There was an error while loading. Please reload this page.
fix(ocap-kernel): make c-list import accounting symmetric - #1020
Conversation
Uh oh!
There was an error while loading. Please reload this page.
Coverage Report
File Coverage |
…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
commented
Aug 13, 2026
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 |
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>
5e3be43 to
1f9c888CompareRebasing 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>
Uh oh!
There was an error while loading. Please reload this page.
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>
Uh oh!
There was an error while loading. Please reload this page.
…hem (MetaMask#1024) Fixes a pre-existing e2e flake on `main`, surfaced while rebasing the MetaMask#1020–MetaMask#1023 stack. Small and self-contained so the whole stack inherits it. ## The defect `control-panel.test.ts` › `should collect garbage` asserted that Carol's root object is `ko6` and Bob's is `ko5`, and that their promises are `kp4` and `kp3`: ```js '{"key":"ko6.owner","value":"v3"}', '{"key":"v3.c.ko6","value":"R o+0"}', ``` Since MetaMask#983, subcluster vats launch **in parallel**. Each vat's root is exported when its own launch finishes, so which of `ko5`/`ko6` belongs to Bob and which to Carol changes between runs. When they come back the other way round, the test fails — and `database-inspector.test.ts` fails alongside it, because it reads the same kv dump. Observed directly: a failing run had `ko6.owner = v2` and `ko5.owner = v3`, the exact inverse of what is asserted. The vat ids themselves are stable — they are handed out in config order, so alice is always `v1` — so only the object and promise krefs need deriving. ## Approach Three small helpers read the dump and look up what the assertions used to hardcode: `rootKrefOf(dump, vatId)` by owner, `promiseKrefOf(dump, vatId)` by c-list entry, and `erefOf(dump, vatId, kref)`. The erefs are derived in **full** rather than matched by prefix. A c-list entry's reverse direction is keyed by eref and valued by kref, so a loose `,"value":"ko5"}` also matches the *owning* vat's own `v2.c.o+0` entry. That passed while both vats were alive and broke the negative assertions the moment one outlived the other — which is what the test checks after terminating v3. ## Testing `yarn lint` clean. Extension e2e run three times: the kref failure is gone, and the two clean runs finish in ~50s rather than ~2.7m because no retries are needed. **What this does not fix.** The extension e2e suite has separate instability that this change does not touch and does not claim to: `object-registry.test.ts` failures, and a UI timing flake where `Terminated vat "v1"` does not render because the panel is still showing query output. One of the three runs hit those. They are unrelated to kref assignment and were present before this change. ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, `README.md`, `CHANGELOG.md`) as appropriate — test-only change, no changelog entry <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Test-only change to e2e assertions and helpers; no production or runtime behavior is modified. > > **Overview** > Fixes flaky **`should collect garbage`** assertions in `control-panel.test.ts` that assumed fixed kernel refs (`ko5`/`ko6`, `kp3`/`kp4`) for Bob and Carol. Parallel subcluster launches mean those object and promise krefs can swap between runs while vat ids (`v2`/`v3`) stay stable. > > Adds helpers to parse the Database Inspector kv dump and **derive** root krefs (via `.owner`), promise krefs (via c-list), and v1’s **erefs** (full c-list lookup so reverse entries don’t false-match). The garbage-collection expectations are built from those values instead of literals. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit d8e81f7. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
The retention was deduplicated by kref, and the failure path undid it if this call was the one that took it. Minting awaits, though, so issuances for the same target overlap: a second `issue` can mint a URL while the first is still in flight, having taken no retention of its own because the ledger already named the kref. If the first then fails it unwinds the retention the second's live URL depends on, and collection can take the capability out from under it. The ledger is a multiset now, one entry and one pin per issuance, so a failed mint releases only what it took. Pins were already a multiset, and each pin here is either released by its own failure or held by its own live URL, so none is left unreleasable — the concern that motivated deduplicating. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
grypez
left a comment
There was a problem hiding this comment.
Review
The invariant-checker-first ordering earned its keep — four of the bugs fixed here were found by the checker. I verified #1006's symptoms 1–3 against the real store and through the real GC handlers: genuinely fixed. add/deleteCListEntry are exactly symmetric, setReachableFlag is idempotent, the (0, 0) birth window is safe, and all four delivery fixes are correct and complete. getPromisesByDecider was previously dead code, so vat and remote termination never rejected orphaned promises at all — good catch. One detail in the design's favour: the stack later adds a third early return to #deliverNotify, and releasing the notification's reference up front covers it by construction; had the decrement stayed after deliverNotify, that new path would have leaked.
On gc.ts:169: you're right and the issue is wrong. A single collectGarbage pass legitimately emits dropExport and retireExport together, so the owner's flag is still set when retireExport is derived — I reproduced that at both ends of the stack. Upstream SwingSet carries the same assert commented out under the same TODO. Keep it disabled; replace the TODO with your explanation.
I reviewed this against the tip of the stack as well, so the notes below distinguish what later PRs fix from what nothing does.
Sequencing
This PR turns auditRefCounts on for every kernel kernel-test builds, and the audit throws into the run loop. Three of its violators are elsewhere:
- At this commit
getImporters(vat.ts:150) filtersgetVatIDs(), so a remote importer never gets aretireImportand its entry dangles on a deleted kref, which theretiringexemption cannot forgive because it only excuses entries that have a queued action. #1023 closes this. - #1021's description says of the stale
gcActionscache: "harmless with auditing off; fatal with it on, which is everykernel-testkernel." This is the PR that turns it on. Verified fixed at the tip, on a savepoint-capable database. - Still live at the tip: the same asymmetry survives for vats.
getImportersreads registration rows while the audit scans the whole store, and#retireVatdeletesvatConfig.<vatId>while the vat's c-list lives until cleanup — so a terminated-but-uncleaned importer is invisible. The recognizable unit it still holds buys no protection: the orphan path queues oneretireImportper visible importer and then deletes the object unconditionally. If the owner is marked terminated first and both terminations land between cranks — cleanup handles one vat per crank, FIFO by mark order — the object is deleted with no action for that importer and the audit reportsdangling, killing the run loop one crank before cleanup would have swept the entry. Reproduced at the store level; the window closes on the next crank, so it is transient and invisible with auditing off. Best fixed in #1023, wheregetImportersis already being changed — deriving importers from c-list entries rather than registration rows closes the class. Raising it there separately.
So the stack is sound as a unit; merged one at a time, main spends three PRs able to die on a remote importer or an aborted retire, and the vat case can still flake kernel-test at the tip. Either land the stack together, or defer auditRefCounts: true in kernel-test/src/utils.ts until getImporters and the audit agree on ground truth.
Belongs in this PR
These all survive to the tip, so nothing downstream will catch them.
incrementRefCounthas nokernelRefExistsguard (refcount.ts:108) whiledecrementRefCountdoes (:152). The guard went to two call sites instead of the primitive, so other paths still resurrect a deleted row —pinObject('ko99')on a kref that never existed yieldskernelRefExists → true,(1, 1),owner undefined. Reachable with no remote involved:retireKernelObjectsqueues theretireImportand callsdeleteKernelObjectin the same breath, so there is always a window where the row is gone while an importer's entry is live. The audit correctly exempts that window, but an increment inside it resurrects from zero, losing the surviving entry's recognizable unit, and the nextsetReachableFlagthrowsrefMismatch(set) "ko1" 2,1in the crank path. Also reached by local ocap-URL redemption, which gets toincrementRefCount(slot, 'resolve|slot')with onlyinsistKRefwhere the remote path fails loudly attranslators.ts:88. Putting the guard in the primitive closes the class.- The audit does not reliably fail the build. The throw at
refcount-audit.ts:354kills the run loop, butKernel.ts:347routes it to#handleRunLoopFailure, which deliberately does not rethrow, andkernel-test'smakeKernelpasses noonRunLoopFailure. A violation fails a test only if that crank has a pendingqueueMessagesubscription to reject — on a GC-only or reap crank, or after the last assertion, it is onlylogger.error'd into avi.fn()nobody asserts.endowment-globals.test.ts:37andio.test.ts:73also build kernels directly and are not audited. refcount-audit.ts:280cannot report the corruption it exists to catch.storedTextgoes throughgetObjectRefCount, whichFails onreachable > recognizable; the operator getsrefMismatch(get) ko7 3,1with no holder list. Parse the raw string as the promise branch already does.- The two headline fixes are untested. Reverting
item.targettotargetatKernelRouter.ts:321keeps the whole suite green — there is no delivery test where routed and queued targets differ. The notify-leak fix is untested on exactly the two early returns it exists for (KernelRouter.test.ts:552,:585assert only the return value). KernelQueueincrementsdata.slotsbefore the state/decider checks thatFail. An illegal resolve leaves the target at(1, 1)with no holder, which the audit reports — so in audit mode this kills the kernel rather than leaking quietly. Move the increments below the checks.- Please state the migration decision.
kernel-storehas no schema version or migration, so a store from the current release opens with every object at(1, 1)and no root pins; with two importers the secondclearReachableFlagthrows"ko1" underflow -1,1from insideperformDropImports, on adropImportssyscall against an existing database. Roots there have no pin either, so the last importer's drop can retire a live vat's root. Still true at the tip — nothing in the stack adds a version, a migration, or a refusal to open a pre-migration store. The BREAKING marker may make "reset required" the right answer; it just needs saying, along with the fact thatrecomputeRefCountsis currently only reachable by constructing a secondmakeKernelStoreover the same database, withRefCountViolationnot re-exported from the package root. - Changelog. The BREAKING entry sits under
### Fixedwhile its sub-bullets are Changed-shaped — the(0, 0)birth and thekrefsToExistingErefs→krefsToErefsrename-and-throw. The rename deserves its own### Changedbullet so a consumer scanning for breakage finds it. And #1022 walks back this entry's "counts too high (a leak)" claim — "it compares counts against the holders it finds… 'a leak' overstated it" — better to state the limit correctly here than to correct it two PRs later.undoOcapURLRetentionis missing from the Added list, and the formatting commit added blank lines inside the unrelated #984 entry.
Follow-ups, not this PR
- A message can be transferred onto a settled promise's queue:
routeAsRequeueis reached from thefulfilledarm without re-checking state,resolveKernelPromisealready deleted that queue, andprovideStoredQueuesilently recreateshead/tail. Verified at the tip — the message is never delivered, the caller's result promise never settles, and the audit stays empty because the recreated entry justifies its own count. The message loss pre-dates this PR; making that entry the sole holder is what turns a visible over-count into a permanent invisible one. Take the requeue path only forunresolved. unpinVatRoot(VatManager.ts:339) spends the lifetime pin — onmainan unbalanced call was a no-op, now it retires a running vat's root. Blast radius at the tip is anOBJECT_DELETEDrejection rather than a dead kernel, but the doc comment "does not make it collectable while the vat lives" is wrong for that path.- The ocap-URL ledger is O(n²) in issuances for one kref (5000 issuances → ~20 KB rows, 868 ms, permanent); encoding counts per kref keeps per-issuance semantics without per-issuance storage. And
revokeonly writesko.revoked, so "revoke is the way to kill the capability" reclaims nothing. addCListEntryis not idempotent — a re-add double-counts recognizable anddeleteCListEntryreleases one. Both in-tree callers are guarded, but it is public and silently gained a refcount side effect here.incRefCount/decRefCountare dead code, and worse: callingincRefCounton a live object writesNaNinto the row and permanently breaksgetObjectRefCountfor that kref. Worth deleting rather than leaving besideincrementRefCount.- The settled-promise c-list TODO. The tip documents the cost accurately ("holds a count forever, so it is never collected") but does not fix it, and three
kernel-testassertions bake it in — worth a tracked issue.
Two notes on the #1010-era repros, if they are reused as regression tests: the remote-importer one registers its remote with a bare initEndpoint, which production never does (establishRemote writes the info row first), and the rollback one runs on makeMapKernelDatabase, whose savepoints are no-ops — so it fails at every commit. Both need corrected setups before they mean anything.
Pin balance across launch/restart/terminate/reload/deleteSubcluster is clean, and krefsToErefs throwing is safe: shouldProcessAction gates all three action types on hasCListEntry in the same synchronous stretch as delivery, and I could not construct a legitimate state reaching the throw.
grypez
left a comment
There was a problem hiding this comment.
Requesting changes on the seven items from my earlier review that belong in this PR, now anchored inline. All seven survive to the tip of the stack (c9b917b96), so nothing in #1021/#1022/#1023 will pick them up.
To be clear about what this is not blocking on: the remote-importer dangle and the retired-export zombie are genuinely fixed downstream (verified by execution at the tip), and the gc.ts:169 judgment call is right. The sequencing concern and the lower-severity follow-ups stay in the earlier comment; this review is only the in-scope asks.
Items 3 and 7 carry concrete suggestions. Items 1, 2, 4, 5 and 6 are judgment calls or need changes outside this diff, so they are comments rather than patches.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
`getObjectRefCount` reads a missing row as (0, 0), so incrementing one writes it back and resurrects a live-looking object with no owner — deliverable to by nobody, and endorsed by the audit, since whatever took the reference is a legitimate holder for exactly the count it finds. `retireKernelObjects` deletes the object and queues the `retireImport` in the same breath, so there is always a window where the row is gone while an importer's entry is still live; an increment inside it loses that entry's recognizable unit, and the next `setReachableFlag` pushes reachable past recognizable and throws mid-crank. This PR guarded the two paths it had found — importing into a c-list, issuing an ocap URL — but `pinObject`, `resolve|slot` and everything else still resurrect. `decrementRefCount` has always guarded the same missing row at the primitive; `incrementRefCount` now does too, and fails rather than returning: releasing a reference to something already gone is ordinary teardown, taking one is always a bug. The two call-site guards stay. They refuse before an eref is allocated or a ledger entry written, and name what was attempted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…n it The audit read each object's stored counts back through `getObjectRefCount`, which `Fail`s when reachable exceeds recognizable — one of the two drifts this module exists to diagnose. Hitting it meant the operator got `refMismatch(get) ko7 3,1` with no holder list, no expected value, and none of the other violations from the same sweep. Objects store the same "reachable,recognizable" encoding the audit renders, so the raw row compares directly. A malformed row is now reported as it stands rather than taking the sweep down with it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…s legal `resolvePromises` incremented every slot before checking the promise's state and decider, so a vat's illegal `syscall.resolve` threw out of those checks having already charged a unit per slot with nobody holding it. This PR removed the `resolve|kpid` increment from the same spot but left the slots, and the audit it adds is what makes the leftover fatal rather than merely leaky: the next crank reports a kref stored at (1, 1) with no holder and kills the kernel. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both were untested on exactly the paths they exist for. Every assertion on `deliver|send|target` used an object target, where the run queue item's target and the routed target are the same kref, so reverting that fix left the suite green; there was no delivery test at all where a message reaches an object through a promise that fulfilled to it. The notify fix is the same story: the two early returns it moved the release in front of asserted only the return value, and the sibling-promise decrement it deletes was never exercised, since the one batch test mocks `getKpidsToRetire` to return the notified promise itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Turning the audit on for every kernel `kernel-test` builds did not make a violation fail the build. The audit reports by throwing, which kills the run loop, and the kernel deliberately hands run loop death to `onRunLoopFailure` rather than rethrowing it — so with no handler a violation surfaced only if that crank happened to have a caller waiting on it. On a garbage collection or reap crank, or one landing after a test's last assertion, it was logged into a mock nobody asserts on and forgotten. `makeAuditedKernelOptions` records the failure and hooks report it, so it fails the test with the message that names the drifted kref rather than an unhandled error that takes the worker down with a useless one. Two kernels built directly rather than through `makeKernel` were not audited at all; they are now. Verified by injecting a double increment into `pinObject`: two `kernel-test` tests fail with the violation, where before this they passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…g rename A store written before this change has every object at (1, 1) and no root pins, and `kernel-store` has no schema version to notice: the second importer's `dropImports` underflows mid-crank, and a legacy store's roots have no pin for the last importer's drop to lose to. There is no migration and none is planned at this version, so say so where an upgrading consumer will read it. The `krefsToExistingErefs` rename moves to `### Changed`, where this file puts its other breaking API changes and where a consumer scanning for breakage looks. The audit entry claimed to catch leaks; it compares counts against the holders it finds, so a holder that should have been torn down but wasn't justifies its own count and is invisible to it. `undoOcapURLRetention` is as public as the two methods listed beside it, and `RefCountViolation` is now exported from the package root, as the entry said it was. Also reverts blank lines this branch's formatting commit inserted into an unrelated entry. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
sirtimid
commented
Aug 17, 2026
All seven items are done, one commit each, pushed as Short version:
On sequencing: I agree the stack is sound as a unit. I would rather land it together than defer Your follow-ups that are not this PR (settled-promise requeue, |
Uh oh!
There was an error while loading. Please reload this page.
An ocap URL retains its target for as long as any URL names it, and which objects get URLs is the holder's choice, not the kernel's — so neither the retention ledger nor the pin list it writes into is bounded by anything the kernel controls. Both kept every entry in a single row, so each issuance read, rewrote and re-sorted the whole thing, and the row grew without limit. Both are now a count per object in a row of its own, which is one write per issuance whatever else is retained. Counting keeps the per-issuance semantics the previous shape needed a multiset for: overlapping issuances for one target share the single pin, and a failed mint spends its own issuance without touching the retention a URL minted alongside it depends on. Ending a retention was one method doing two jobs it cannot both do. `undoOcapURLRetention` unwinds a single issuance, which is right for a mint that failed and wrong for disavowing an object, where every URL naming it goes at once — that case is `releaseOcapURLRetentions`. Revocation is still neither: it writes only its flag, so a revoked object's URLs stay retained. `getPinnedObjects` now names each object once however many pins it holds, and `getPinCount` reports that number, which is what the audit needs to credit a unit per pin. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Where pins and ocap URL retentions are stored is the store's own business: a consumer sees the methods and what they mean, not the keys they write. The entries keep the API changes — the new methods, and `getPinnedObjects` naming each object once — and drop the key layout and the reasoning behind it, which live in the code that implements them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three behaviour fixes, each the sibling of something already fixed here. `incrementRefCount`'s deleted-kref guard now covers promises. It sat below the `isPromise` early return, so a promise increment on a missing row still wrote `NaN` back — a row that reads as existing and that no decrement can bring to zero, so the promise could never be collected. The guard is placed in front of both paths that read a row and write it back, rather than at the top: an object export mutates no count, so it still needs no row to exist. The audit no longer dies on a settled promise that lost its value. Reading it with `getRequired` took the whole sweep down over one row, in the module whose premise is that the store might be wrong; `gc.ts` and `getKpidsToRetire` both allow that state. Read tolerantly, the slots it would have credited are reported as counts too high, which is what they are. `deleteEndpoint` releases the references its c-list entries hold instead of deleting the keys. The prefix fix made this loop live for the first time, and a bare delete leaves the target held by a holder that no longer exists — pinned alive forever, and reported by the audit as a count nothing accounts for. No in-tree change: `cleanupTerminatedVat` has emptied the c-list before it gets here. `clist.test.ts` needed a promise refcount row for the same reason the object tests needed one in 2bf8a5c. Also: `unpinVatRoot`'s doc claimed the opposite of what it does, since pins are fungible and an unbalanced call spends the lifetime pin; four keys in the store's layout block were wrong, which is the class of staleness that caused two of the bugs this PR fixes; and the e2e asserts a root's pin, which is the one invariant the audit cannot check — a root that lost its pin agrees with its refcount and the audit stays silent. 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 ceb6c50. Configure here.
Uh oh!
There was an error while loading. Please reload this page.
`v3Values` is checked three times, spanning v3's termination, so a pin assertion cannot live there: terminating the vat releases the pin its launch took. Asserted on either side of the termination instead, which is worth more than one reading anyway — it pins the release too. The counts come from an actual run rather than derivation: a live root is its own pin plus v1's import at `2,2`, and `1,1` once the pin is spent. That run also showed bob's root at `3,3` behind two pins, the second being the ocap URL retention its issued URL holds, which is the accounting this branch added. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The tolerant read added for a missing value row still asserted `slots` was there, so a row that parsed but carried none returned `undefined` for the caller to iterate — throwing outside the `try`, which is the crash the helper exists to prevent. Reported by Bugbot. A row whose `slots` is a string was worse and unreported: it iterated character by character and credited krefs that never existed, so the audit invented violations against `k`, `o` and `1` instead of dying. A tool whose only value is being believed must not do that, so this checks for an array rather than trusting a cast. Parameterized over all six shapes a value row can take; three of them fail against the previous code, including the fabricated-kref one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| // name a capability that can never be delivered to. Refusing here names | ||
| // what was attempted, and does it before anything has been written. | ||
| this.kernelRefExists(kref) || | ||
| Fail`cannot issue an ocap URL for deleted kref ${kref}`; | ||
| const krefs = this.getOcapURLObjects(); | ||
| krefs.push(kref); | ||
| kv.set('ocapURLObjects', krefs.sort().join(',')); | ||
| this.pinObject(kref); | ||
| const issuances = this.getOcapURLIssuanceCount(kref); | ||
| if (issuances === 0) { | ||
| this.pinObject(kref); | ||
| } | ||
| kv.set(`${OCAP_URL_PREFIX}${kref}`, `${issuances + 1}`); | ||
| }, | ||
| undoOcapURLRetention(kref: KRef): void { | ||
| const krefs = this.getOcapURLObjects(); | ||
| const index = krefs.indexOf(kref); | ||
| if (index === -1) { | ||
| const issuances = this.getOcapURLIssuanceCount(kref); | ||
| if (issuances > 1) { | ||
| kv.set(`${OCAP_URL_PREFIX}${kref}`, `${issuances - 1}`); | ||
| return; | ||
| } | ||
| krefs.splice(index, 1); | ||
| if (krefs.length === 0) { | ||
| kv.delete('ocapURLObjects'); | ||
| } else { | ||
| kv.set('ocapURLObjects', krefs.join(',')); | ||
| // The last issuance, or none at all: either way, what is left of the | ||
| // retention is exactly what a release drops. | ||
| this.releaseOcapURLRetentions(kref); | ||
| }, |
There was a problem hiding this comment.
This mechanism might be overkill. Once the count has been successfully incremented once (i.e., one successful URL issuance that wasn't rolled back due to a failure) it can never return to 0, as there is no GC of issued URLs possible and no notion of URL retirement. It basically needs to be single sticky bit, not a count (a count is certainly one way to implement the sticky bit, but it costs an extra write each time). I'm going to approve the PR, as this is a quibble not worth holding things up for, but if the spirit moves you to address this I promise a quick re-approval.
There was a problem hiding this comment.
Thanks! I think the count does one more thing than a sticky bit can do.
It is not counting live URLs. It counts issuances that are still in flight. Minting awaits, so a second issue for the same kref can finish while the first one is still waiting. Then if the first one fails, its undo has to know if it is the only one holding the pin.
With one bit it can go two ways and both are wrong:
- clear the bit and unpin → we just unpinned an object that a live URL points to
- keep the bit → a single failed issuance leaks the pin forever, which is the exact thing
undoOcapURLRetentionexists for
The test for this is store/index.test.ts:400.
Returning "did I set the bit?" to the caller does not help either. The first issuance is both the one that sets the bit and the one that can fail last.
Also releaseOcapURLRetentions takes the count back to 0 on purpose. Wiring revoke/disavow to it is a follow up, so 0 is a real state, not only the start state.
If you still prefer the bit and you are ok with leaking one pin when an issuance fails, happy to do it — but as a follow up, since 3 PRs are stacked on this one.
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: counts too low, which lets a live capability be collected, and counts too high, which keeps a dead one alive (the issue's symptom 4 would pass an underflow-only check). It compares against the holders it finds, so a holder that should have been torn down but wasn't justifies its own count and is not detectable this way. The credits mirrorincrementRefCountcase for case.Enabled per kernel via
Kernel.make({ auditRefCounts: true }), run after every crank, and on for every kernelkernel-testbuilds. The audit reports by throwing, which kills the run loop, and the kernel hands run loop death toonRunLoopFailurerather than rethrowing it — sokernel-testpasses a handler that fails the test, and a violation on a GC-only crank or after a test's last assertion fails the build too.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.Migration
There is none, and none is planned at this version: a store written before this change must be reset.
kernel-storehas no schema version and no migration path, so such a store opens under this code with every object still at(1, 1), no pin recorded for any vat root, and its pin and retention records in a layout this code does not read. Both consequences land on the crank path, against an existing user's database:dropImportsthrows"ko1" underflow -1,1from insideperformDropImports;initializeAllVatsusesrunVat, which does not pin, and relies on the persisted pin a legacy store does not have — so the last importer's drop can retire a live vat's root.recomputeRefCountsrebuilds the counts from ground truth, but it cannot restore the root pins, so it is a diagnostic for a drifted store rather than an upgrade path. Reach it by callingmakeKernelStoreover the kernel's own database;RefCountViolationis now exported from the package root.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.Changes since review
@grypez's seven in-scope items and @FUDCo's, one commit each.
incrementRefCountguards at the primitive. It nowFails on a missing object row, symmetric with the decrement's guard — the guard was at two call sites, sopinObject,resolve|slotand every other path could still resurrect a deleted object. The call-site guards stay: they refuse before an eref is allocated or a ledger entry is written, and name what was attempted.kernel-testpasses anonRunLoopFailurethat reports the failure toafterEach/afterAllhooks, so the test fails with the message naming the drifted kref. An async rethrow was the first attempt and is worse: underendoify-nodeit exits the worker withprocess.exit unexpectedly called with "-1"and the real error nowhere in sight.io.test.tsandendowment-globals.test.tsbuild kernels directly and are audited now too. Verified by injecting a double increment intopinObject: twocluster-launchtests fail with the violation, where before they passed.getObjectRefCount, whichFails onreachable > recognizable— one of the two drifts it exists to report. A malformed row is now reported as it stands.resolvePromiseschargesdata.slotsafter the state and decider checks, so an illegalsyscall.resolveleaves nothing behind.### Changedas its own BREAKING bullet, the "counts too high (a leak)" claim corrected to name the blind spot,undoOcapURLRetentionadded, and the blank lines my formatting commit put inside the feat(ocap-kernel): reference-marker sigil at queueMessage RPC boundary #984 entry reverted.undoOcapURLRetentionunwinds a single failed issuance,releaseOcapURLRetentionsdrops the object's whole retention for a disavowal.getPinnedObjectsnames each object once;getPinCountgives the count.The follow-ups from the reviews that are not this PR's — the settled-promise requeue,
unpinVatRoot,addCListEntryidempotency,incRefCount/decRefCount, and wiring revocation toreleaseOcapURLRetentions— are noted and will be raised separately.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 and a violation now failing the test that provoked it.Checklist
README.md,CHANGELOG.md) as appropriateNote
High Risk
Touches core capability GC, refcount invariants, and persistent store layout with a mandatory reset for existing databases; incorrect accounting can collect live objects or leak capabilities.
Overview
Fixes #1006 by making import c-list creation/release symmetric: new objects start at
(0, 0),addCListEntrytakes a reference (withsetReachableFlagfor re-handoffs), and owner-side baseline decrements are removed. Vat roots are pinned for the vat lifetime; GC deliveries now update the kernel’s own c-list (dropExports/ retire paths).Adds reference-count auditing (
auditRefCounts,recomputeRefCounts, …) and optionalKernel.make({ auditRefCounts: true })checks after each crank;kernel-testenables this viamakeAuditedKernelOptionsso drift fails tests throughonRunLoopFailure.Corrects several refcount leaks: send delivery charges
item.target(not the routed object), promise requeue transfers refs, notify releases early, promise-queue messages are charged/released consistently,resolvePromisesonly increments slots after legal resolve, andgetPromisesByDeciderscans the real${endpoint}.c.layout.Ocap URL issuance retains targets (per-URL issuance counts,
pinned.${kref}pin counts);krefsToExistingErefs→krefsToErefs(throws if unmapped);incrementRefCountrefuses deleted krefs.BREAKING: existing stores must be reset (no migration); tests/assertions updated for new baselines (e.g. createObject refcounts, v3 root pin in e2e).
Reviewed by Cursor Bugbot for commit 2f9ef11. Bugbot is set up for automated code reviews on this repo. Configure here.