fix(ocap-kernel): make c-list import accounting symmetric - #1010
fix(ocap-kernel): make c-list import accounting symmetric#1010sirtimid wants to merge 8 commits into
Conversation
- Drop the refCountScheme migration: no production stores exist with the old counting scheme, so the recompute-on-open path is dead code - Update changelog PR links from #1006 (issue) to #1010 (this PR) - Fix changelog formatting: add blank lines before sub-bullets of the @@name and 'Fix the stale cle./clk.' entries to satisfy auto-changelog --prettier validation Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
grypez
left a comment
grypez
left a comment
There was a problem hiding this comment.
Reviewed the accounting change and the two judgment calls. The core fix reads as correct to me, and the checker-first ordering clearly earned its keep. Both judgment calls are sound; my notes below are on the reasoning around them, not the decisions.
One inline comment on a stale invariant claim, plus the notes here. Everything else I found is pre-existing rather than introduced by this PR, and I've written those up as separate issues rather than pile them onto this diff — links at the end.
The disabled gc.ts assert
I traced this and agree. At gc.ts:210-216, when the last holder drops and retires in one crank, clearReachableFlag takes reachable to 0 and forgetKref takes recognizable to 0 before collectGarbage runs, while the owner's own flag is untouched until the first delivery — so both actions get queued with vatConsidersReachable === true and recognizable === 0, exactly the assert's negation. Leaving it off and replacing the stale TODO with the reason is the right call.
The thing worth drawing out: "The audit is what validates the accounting now" makes the audit load-bearing for correctness, while the Kernel.make JSDoc scopes it as "intended for tests and debugging." Those pull in different directions, and the coverage suggests the first framing is currently ahead of the artifact:
File | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
refcount-audit.ts | 95.57 | 89.65 | 100 | 95.57 | 156,207-210
Line 156 is credit(message.result, …) — no test queues a message with a non-null result. Lines 207-210 are the entire promise-queue branch; nothing in the audit tests calls enqueuePromiseMessage. And drift is asserted in both directions for only one of the eight credit sources (c-list import, refcount-audit.test.ts:124-162); the other seven are exercised only in the "audit is clean" direction, so six of the rules could be off by a constant and the suite would stay green. Since this is the artifact inheriting the assert's job, per-credit-source drift coverage seems worth having before it carries that weight.
Settled promises' c-list entries
The UI constraint is a real product call and I'm not arguing with it; the TODO states the cost accurately. But one inference in the description doesn't hold:
and the audit is green without it
The audit is green here by construction, not as evidence. The retained c-list entry is itself a credited holder (refcount-audit.ts:179-186), so the stored count and the recomputed count agree — and they would agree at any value, as long as an entry exists to justify it. The auditor's ground truth is the holder set, so it can detect a count that disagrees with a holder but structurally cannot detect a leaked holder. Worth knowing precisely because this is the one leak the PR knowingly retains.
Same reason the CHANGELOG line reads broader than the behaviour: "counts too high with no holder (a leak)" catches an orphaned count, not an orphaned reference. Might be worth a sentence in the audit's doc comment saying which of the two it finds.
Follow-ups filed separately
Three things I believe are pre-existing and out of scope here, written up with reproductions so they can be judged independently:
- #1015 —
retireKernelObjectsnever notifies remote importers, leaving a dangling c-list entry. Latent today; the topology is not covered bykernel-test, so it does not contradict the clean-audit claim in the description. - #1016 — a throw inside a crank commits the partial crank rather than rolling it back. Identical
try/finallyshape onmain; this PR only adds one new throw source. - #1017 — GC deliveries to remotes carry rrefs in the sender's frame, so the receiver mints a phantom object and the action has no effect. Also pre-existing; this PR's new
isVatIdcatch actually improves the surrounding failure handling.
| // A vat is local and reliable, so a refusal means it is broken. Undo the | ||
| // teardown rather than commit it: leaving the two disagreeing would have | ||
| // the vat mint fresh krefs for objects the kernel thinks it let go of. | ||
| // Aborting restores the entries and the action; terminating the vat is |
There was a problem hiding this comment.
rollbackCrank doesn't restore the consumed GC action, so this comment's second clause is inverted.
Aborting restores the entries and the action; terminating the vat is what stops that restored action from being retried forever.
The entries, yes — the DB rollback covers those. The action, no. gcActions is a provideCachedStoredValue (store/index.ts:147), which keeps the value in a closure and writes through to kv (base.ts:98-117). rollbackCrank (crank.ts:44-62) rolls the database back and then refreshes only the run queue. The gcActions closure still holds the post-processGCActionSet value, so the reduced set wins and the next set persists the loss.
Reproduction, against a real store:
AssertionError: expected [] to strictly equal [ 'v1 dropExport ko1' ]
reapQueue is cached the same way and behaves the same way; that exposure is pre-existing.
So the causality is the other way round from what the comment says: terminating the vat isn't what stops the restored action being retried — it's what makes losing the action harmless, because the action was going to a vat that no longer exists. Since every abort this function returns is paired with terminate, there's no live bug. I'm flagging it because the comment is the thing a future reader will trust when they add an abort path that isn't paired with a termination.
Fix is one line: re-provide both cached values in rollbackCrank, as reset() already does at store/index.ts:217-218. I have the failing test written and can hand it over.
Adjacent, same function: rollbackCrank doesn't clear ctx.maybeFreeKrefs either, which store/index.ts:140-144 states as an invariant. The GC rollback paths happen to survive it because collectGarbage re-reads counts, but gc.ts:161 getKernelPromise throws for a promise a rollback deleted.
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>
- Drop the refCountScheme migration: no production stores exist with the old counting scheme, so the recompute-on-open path is dead code - Update changelog PR links from #1006 (issue) to #1010 (this PR) - Fix changelog formatting: add blank lines before sub-bullets of the @@name and 'Fix the stale cle./clk.' entries to satisfy auto-changelog --prettier validation Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Follow-up to the c-list accounting fix, addressing defects found in review. An owner that stops naming its own export left the object behind. Both the delivered `retireExport` and the `retireExports`/`abandonExports` syscalls tore down the owner's c-list entry but left `owner` and `refCount` in place, with no path that could ever reclaim them: `cleanupTerminatedVat` finds krefs by walking the owner's c-list, and the collector only revisits krefs in `maybeFreeKrefs`. The records leaked, and the next collection to visit such a kref read the owner's deleted entry through `getRequired` and took the run loop down with it. New `orphanKernelObject` drops the owner mapping and hands the object to the collector, which already knows how to retire an orphan. `collectGarbage` also treats an owner with no c-list entry as orphaned rather than trusting the mapping. Reporting a dead run loop belongs to #1005, which landed on main first. It is what makes the audit usable at all: `assertRefCountsIfAuditing` throws from inside a crank, so with the failure logged and swallowed a violation's sole symptom was a test hanging to its timeout with no mention of reference counts. The `kernel-test` case here asserts that shape — the caller is told the run loop died, and the audit error rides along as the `cause`. Also: GC action delivery survives a vanished endpoint or a failed delivery instead of stopping the loop; `launchVat` tears down a worker whose kernel-side registration failed rather than stranding it; `RefCountViolation` discriminates on `kind` instead of sentinel-matching `stored`; and the store context's auditing flag no longer shares a name with `auditRefCounts()`. Tests cover the crash path, the orphan-and-collect sequence, retiring stragglers, GC-action robustness, and that a violation reaches a caller. The `item.target` charge and both `deliver|notify` early returns now have assertions that fail if the fix is reverted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review of the previous commit found that four of the five error handlers it added turned a crash into a state the kernel can no longer detect. Corrects that, and closes a hole the orphaning opened. `orphanKernelObject` took an object's owner mapping on trust. Nothing upstream of `performExportCleanup` checks that the vref it was handed is even an export — `translateSyscallVtoK` maps both directions alike — so a vat could pass an import to `abandonExports`, which needs no precondition at all, and erase a different live vat's claim to an object it was still exporting. Sends to that object then went splat with OBJECT_DELETED, terminating the victim tripped `cleanupTerminatedVat`'s ownership assertion and took the run loop with it, and the audit could not see any of it, because an export entry carries no count. Disowning is now the owner's own doing: the expected owner is a required argument and must match, and the syscall path rejects a mismatch outright. The vanished-endpoint catch returned before the teardown, but `processGCActionSet` had already consumed the action, so neither the kernel nor the durable set remembered the object — a permanent leak, also invisible to the audit. The kernel's side is now released whether or not anyone is left to tell, and krefs whose entries a cleanup already removed are skipped rather than assumed present. The delivery-failure catch committed the teardown after the endpoint had failed to hear about it, so the endpoint would go on to mint a fresh kref for an object the kernel believed it had let go of — the same object with two identities. It now aborts, which restores both the entries and the action, and terminates the vat that could not accept the delivery. `launchVat`'s cleanup path stopped the worker without marking the vat terminated, so nothing ever reclaimed the records a partial launch had written. The audit counted an importer's c-list entry as a holder during the window between `retireKernelObjects` deleting an object and delivering the matching `retireImport`, so the collector's own output failed the end-of-crank check. The missing assertion in the test covering that sequence is now present. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…very Aborting a failed GC delivery restores the action to the durable set, and `processGCActionSet` is consulted ahead of all other run-queue work. For a vat that is fine, because terminating it is what stops the restored action from coming back. A remote cannot be terminated, so the same item would be selected every crank and nothing else would ever run. A remote is a separate kernel across a link that can drop messages anyway, and it reconciles on the next incarnation change, so its failures no longer abort. Also stop `orphanKernelObject` throwing on an object that is already orphaned. Disowning something nobody owns is a no-op, not an error: only a mismatch with a different, live owner is, which is the case the check exists for. Same for the syscall path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…lback A database rollback cannot reach two pieces of state, so `rollbackCrank` now reverts both itself. Every `provideCachedStoredValue` answers reads from a closure and only writes through to kv. Reverting the database therefore left the closure holding the abandoned crank's value, and the next `set` persisted it. `processGCActionSet` takes an action out of the set before delivering it, so an aborted delivery lost the action outright rather than retrying it. `reapQueue` was exposed the same way. `maybeFreeKrefs` lives in RAM, so nothing reverted it either. Its entries are collection candidates only because of the decrements the rollback undid, and a later `collectGarbage` threw outright on a promise the rollback had deleted, killing the run loop. No live bug either way: every `abort` `#deliverGCAction` returns is paired with a `terminate`, which is what made losing the action harmless. The comment there claimed the rollback restored the action, which is the thing a future reader would trust when adding an abort path that isn't paired with a termination; it now states the real causality. The cached values are declared once so that initialization and the refresher cannot disagree about which ones exist. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The clean-audit cases prove each rule agrees with whatever the store did, which stays true if a rule and the code it mirrors are wrong by the same constant. Six of eight rules could have drifted and the suite would have stayed green. Each of the ten credit sources now pins its count and holder labels to literals and asserts drift in both directions: too low collects a live capability, too high leaks it. That closes the two coverage gaps as a side effect — a run-queue send's result promise, and a message parked on an unresolved promise, neither of which any test reached. Also states what the audit can and cannot find, which matters because its ground truth *is* the holder set: a count that disagrees with its holders is caught either way, but a holder that should have been torn down and wasn't justifies its own count at any value, so a leaked reference is invisible to it by construction. That is exactly the case the retained settled-promise c-list entry leaves behind, so the CHANGELOG no longer claims the audit would catch it. The `auditRefCounts` JSDoc no longer scopes the option as "intended for tests and debugging": it stands in for the invariant `collectGarbage` cannot assert, and is off by default only because it walks the whole store. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…s with Releasing the kernel's side of a garbage-collection action when the endpoint has vanished is right for an endpoint that is gone, and wrong for one that is merely out of reach. `restartVat` keeps the vat's c-list and takes the vat out of the kernel's vat table for as long as launching a worker and negotiating with it takes, so a GC action selected in that window found the vat absent, released entries the returning incarnation still holds, and committed — leaving the vat free to mint fresh krefs for objects the kernel thinks it let go of. That is the same divergence the failed-delivery path below rolls back to avoid. The endpoint is now resolved before anything is torn down, so the outcome is decided rather than discovered halfway through, and the release commits only where the endpoint is genuinely gone: a vat the store has marked terminated, whose cleanup tears the whole c-list down regardless, or a remote, which reconciles on its next incarnation. A vat that is absent yet not terminated fails the crank instead, which is what this path did before the release was added to it. This does not make a vat restart safe, and is not trying to: it stops the GC path from turning that window into silent corruption. The window itself needs the vat to stop being unreachable while it restarts — `restartVat` is an RPC handler mutating kernel state alongside a running run loop, which a send already resolves as a splat and a `notify` already dies on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
6618411 to
988411e
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
❌ 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 fcfa5f2. Configure here.
fcfa5f2 to
988411e
Compare
The lifecycle work was split out of #1010 into its own stacked PR, so the eight entries that describe it were citing a PR that no longer contains them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…plit Both entries describe work #1010 started and #1019 finished. The clause each gained — the relaunch-failure pin release, and restoring the collection candidates rather than emptying them — arrived with the lifecycle commits, so it belongs to #1019 even though the entry it hangs off is #1010's. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Superseded — being split into four PRs. Please don't review this branch further. This PR was 2630 lines across four unrelated concerns. The first piece is now #1020 (the refcount audit + the c-list import accounting symmetry fix, 1637 lines, closes #1006). It is rebased on current Splitting out the rest now:
Why the crank work is merging rather than splitting further: this branch changed One thing worth recording: #983 has since landed a guard in Will close this once the remaining PRs are open, so nothing looks dropped in the meantime. |
Prettier wanted a blank line before the entry following a nested bullet, and the entries still cited #1010, which this PR replaces.
Prettier wanted a blank line before the entry following a nested bullet, and the entries still cited #1010, which this PR replaces.
2 participants
Closes #1006.
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.
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.Two judgment calls worth review
The
gc.ts:169assert is not re-enabled. The issue asks for it; I believe it would fire legitimately. When the last holder drops and retires before GC runs,dropExportandretireExportare queued in the same pass and the owner's flag is still set until the first is delivered —drops an object once the last of several importers lets godemonstrates exactly this. Upstream SwingSet also leaves it disabled with the same TODO. I replaced the dead line and stale TODO with the reason. The audit is what validates the accounting now.Settled promises' c-list entries are still not torn down on notify. SwingSet does this (
translateNotify), and I had it working, but it breaks the debug UI:kernel-uidiscovers exported ocap URLs by scraping settled promise values found through c-list entries, andissueOcapURLis stateless — nothing persists issued URLs, so it has no other source. The refcount corrections in that function are all kept; only the record-freeing cleanup is deferred, with a TODO. This is pre-existing behaviour, not a regression. Giving the UI a real source is separate work.An earlier version of this description said "the audit is green without it," which doesn't hold as evidence and I've withdrawn it — grypez is right that it's green here by construction. The retained c-list entry is itself a credited holder, so the stored count and the recomputed count agree, and they'd agree at any value as long as an entry exists to justify it. The audit's ground truth is the holder set, so it catches a count that disagrees with its holders in either direction but structurally cannot catch a leaked holder — which is exactly what this deferral leaves behind. The audit's doc comment and the CHANGELOG now say so.
Added in review
rollbackCranknow reverts the two pieces of state a database rollback cannot reach. grypez traced this from the abort-path comment in#deliverGCAction, which claimed the rollback restored the consumed GC action; it didn't.provideCachedStoredValueanswers reads from a closure and only writes through to kv, so reverting the database left it holding the abandoned crank's value and the nextsetpersisted that.processGCActionSettakes an action out of the set before delivering it, so an aborted delivery lost the action outright rather than retrying it. Repro, verbatim from the review:expected [] to strictly equal [ 'v1 dropExport ko1' ].reapQueuewas exposed the same way.maybeFreeKrefsis cleared. It lives in RAM, so nothing reverted it; its entries are collection candidates only because of the decrements the rollback just undid, and a latercollectGarbagethrew outright on a promise the rollback had deleted, killing the run loop. This is correct only because every rollback is to the crank's own start —KernelQueue.tsis the solecreateCrankSavepointcaller.No live bug either way, since every
abort#deliverGCActionreturns is paired with aterminate. Fixed because the comment was the thing a future reader would trust when adding an abort path that isn't. The comment now states the real causality.Per-credit-source drift coverage for the audit, which grypez asked for before the audit inherits the disabled assert's job. Drift is now asserted in both directions for all 10 credit sources rather than 1 of 8, and both coverage gaps named in the review are closed — line 156 by a run-queue send's result promise, 207-210 by a message parked on an unresolved promise.
refcount-audit.tsis at 100% stmts/lines, 93.1% branch; the remaining uncovered branches are defaults and a dangling-with-no-holders case.The
auditRefCountsJSDoc no longer scopes the option as "intended for tests and debugging," which pulled against making the audit load-bearing. It's off by default because it walks the whole store, not because it's optional.Verification
auditRefCountsclean across all ofkernel-test, which now runs it after every crankyarn lintclean;@metamask/ocap-kerneland@ocap/kernel-testsuites pass@ocap/kernel-test-local, a local-only package excluded from CI whose vat bundles a fresh worktree does not build; it passes in a fully built treeyarn test:e2e:ci: 17/17 inextension.kernel-node-runtimeis 68/69 —remote-comms > Intentional Disconnect > handles remote intentional disconnect without reconnectingfails identically onorigin/main, so it is pre-existing and not this branchcleanupTerminatedVat(previously covered only by a name-export assertion), and a ≥3-endpoint topology all have regression tests — plus an end-to-end two-importer test inkernel-testproving the shared object survives the first importer letting gorollbackCrankregression tests fails against a real store without the fix, with the symptom it names: the restored GC action, the restoredreapQueue, andcollectGarbagethrowingunknown kernel promise kp1Test expectation changes, and why
object.test.ts,store/index.test.ts:(1,1)→(0,0)at birth, as the issue predictedclist.test.ts: an import entry is born un-flaggedpromise.test.tsgetPromisesByDecider: rewritten against the real key layout — it had mockedgetPrefixedKeysto return the stalecle.keys, which is what hid the prefix bugpersistence.test.ts: a hand-writtenrefCountfixture encoded the old accountingcontrol-panel.test.ts(e2e): dropped theko6.refCountassertion. Root pinning ties that value to vat liveness, so it now flips between1,1and2,2depending on whether carol's termination has been processed when the dump is taken. The semantics are covered deterministically inclist-accounting.test.tsinstead.Note on #994
#994 says
translateRefKtoE(remoteId, kref, true)"allocates a c-list entry and increments the refcount". Before this PR no increment occurred, so its pinned-refcount consequence was unfounded; after this PR the increment does happen. Its other two consequences were always unaffected.🤖 Generated with Claude Code