Conversation
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
452b1e6 to
1099770
Compare
1099770 to
a9503d8
Compare
a9503d8 to
c1ad101
Compare
c1ad101 to
cb455cb
Compare
… endpoint lookup #1022 made a GC action for an absent-but-not-terminated vat abort the crank rather than throw, because at that point `restartVat` really does take a vat out of the kernel's reach while cranks run, and the abort hands the action to the incarnation on its way back. This branch closes that window instead: the run loop carries out the restart itself and `provideVat` waits out a vat in flux, so a vat that reaches the lookup's catch is gone with nothing to wait on — and there the abort spins with no delivery to wait for, which is why this branch keeps the throw. The tests move with it. Co-Authored-By: Claude Opus 5 (1M context) <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>
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>
…eanup path Both are new here and neither was reached by a test. The ownership guard is the one that matters: nothing upstream of `performExportCleanup` checks that a vref is even an export, so without it a vat can disown another live vat's object. Removing the guard now fails two cases rather than none. `launchVat`'s registration failure is covered through the store calls it makes, since `VatManager` is hardened and cannot be spied on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Also narrows the audit's Added entry: this branch turns `RefCountViolation` into a discriminated union, and the audit compares counts against the holders it finds, so a holder that should have been torn down but wasn't justifies its own count. "A leak" overstated what it can detect. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`RefCountViolation` gained its `kind` discriminant here, so the expectations #1021 wrote against the old shape needed it. The audit also moved out of the crank and runs after it commits, and `utils.ts` grew hooks that fail a test whose run loop died — so the case that kills the loop on purpose now claims that death as its result. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…n loop `#deliverGCAction` aborts the crank for a vat between incarnations instead of throwing. The throw left `deliver` by the run loop's catch and set a failed state nothing clears — a permanent kernel death, triggered by the `restartVat` button whenever a GC action for that vat was pending. The abort restores the c-list and the action and hands them to the returning incarnation. The action is reselected every crank until it returns; that residual spin is #1061. `processGCActionSet` sorts the krefs before hardening them. `sort` writes back into the array even when it is already in order, so any endpoint with two actions of one type threw out of `filterActionsForProcessing`. One vat dropping two exports in a crank is enough. Reproduces on `main`. `getImporters` reports remotes as well as vats. `retireKernelObjects` deletes the object once it has told the importers, so a remote left out kept a c-list entry naming a kref that no longer existed — dangling, as the audit reports it, which kills the run loop. `stopVat` forgets the vat in a `finally`. A throw from unpinning the root or from the handle's own `terminate` left the handle on the books while the store went on to wipe the vat's state. Adds the first real-store coverage of a kernel-issued GC delivery: selection had it and delivery was only ever tested against a mocked store, which is why the first three of these survived a green suite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
cb455cb to
65a62cb
Compare
… endpoint lookup #1022 made a GC action for an absent-but-not-terminated vat abort the crank rather than throw, because at that point `restartVat` really does take a vat out of the kernel's reach while cranks run, and the abort hands the action to the incarnation on its way back. This branch closes that window instead: the run loop carries out the restart itself and `provideVat` waits out a vat in flux, so a vat that reaches the lookup's catch is gone with nothing to wait on — and there the abort spins with no delivery to wait for, which is why this branch keeps the throw. The tests move with it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 65a62cb. Configure here.
| // cleanup wipes, so `hasVat`, `getVatIds` and `#getEndpoint` go on | ||
| // reporting a vat with no c-list, and the next `bringOutYourDead` | ||
| // selected for it kills the run loop. | ||
| this.#vats.delete(vatId); |
There was a problem hiding this comment.
Failed stopVat leaves vat unrecoverable
High Severity
stopVat now drops the vat handle in finally, but terminateVat marks termination only after stopVat returns and restartVat only relaunches then. If unpinning or vat.terminate throws, the handle is gone while the vat stays unterminated. GC delivery then treats it as between incarnations and abort-spins that action every crank, starving the run loop.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 65a62cb. Configure here.


Stacked on #1021; review that first. #1020 has merged. Third of four; split out of the old #1010.
The refcount audit landed in #1020 as an invariant checker. This is what it caught: four independent ways the kernel could lose track of an object during garbage-collection delivery, three of which killed the run loop outright.
What's fixed
An owner's
ownerandrefCountrecords outlived the c-list entry they were reachable through. When an owner stops naming its own export — a deliveredretireExport, or aretireExports/abandonExportssyscall — nothing dropped the ownership record. They leaked, and the next collection to visit such a kref read a c-list entry that was no longer there and died:This reproduces on
main(494fb5ee9), so it predates this whole stack. NeworphanKernelObjecthands the object to the collector instead, andcollectGarbagenow checkshasCListEntrybefore reading the owner's reachable flag.A vat could disown an object it did not own. Nothing upstream of
performExportCleanupchecked that the vref it was handed is even an export —translateSyscallVtoKmaps both directions alike — so a vat could pass an import toabandonExports, 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 failed withOBJECT_DELETED, and terminating the victim trippedcleanupTerminatedVat's ownership assertion and took the run loop with it. The audit could not see any of it, because an export entry carries no count. The expected owner is now a required argument and must match; the syscall path rejects a mismatch outright.This was the most security-relevant change here and had zero coverage —
gc-handlers.test.tsis new, 10 tests, mutation-verified.GC action delivery hid its own failures. Two cases:
processGCActionSethad 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.It releases only where the endpoint is genuinely gone: a terminated vat, whose cleanup tears the whole c-list down anyway, or a remote, which reconciles on its next incarnation. A vat that is absent yet not terminated is one
restartVathas taken out of the kernel's reach while keeping its c-list, so the crank fails there rather than committing a release the returning incarnation would disagree with.A failed GC delivery to a remote escaped the crank and stopped the run loop. Now logged and survived.
A partial vat launch left records nobody reclaimed.
launchVat's cleanup stopped the worker without marking the vat terminated.Note on the audit's Added entry
This branch narrows what #1020's changelog claims the audit can detect. 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 not detectable this way — "a leak" overstated it.
RefCountViolationalso becomes a discriminated union overkind: 'mismatch' | 'dangling'.Merge order matters
This must not merge before #1021. Its abort path depends on
refreshCachedValues(), which lands there. A database rollback restores the c-list entry and thegcActionsrow, butgcActionsis aprovideCachedStoredValueclosure the rollback never refreshes — so the audit would build itsretiringexemption from a stale cache, credit the restored entry as a holder of a deleted kref, and kill the run loop. Harmless with auditing off; fatal with it on, which is everykernel-testkernel. Verified against a real SQLite store.Issues
getImportersstill filtersgetVatIDs()only, so a remote importer gets noretireImportand the audit reportsdanglingon its entry. This PR supplies the exemption site the issue names as the alternative fix, and thekinddiscriminant that makes the violation classifiable. The open question — whether remotes learn about retirement through another channel — is untouched.translateRefKtoE('r1', ko)→ro+1whilekrefsToErefs('r1', [ko])→ro-1. No flip added here. The advance is the one the issue itself credits: a failed remote GC delivery no longer escapes and kills the run loop.Testing
yarn lintclean,yarn build31/31,changelog:validateclean.@metamask/ocap-kerneland@ocap/kernel-testfully green, withauditRefCountson for every kernelkernel-testbuilds.The export-ownership guard is mutation-tested: deleting it fails exactly two cases.
Checklist
README.md,CHANGELOG.md) as appropriateNote
High Risk
Changes core GC delivery, export ownership, and vat teardown paths that can disagree with live vats or remotes if mishandled; merge order with #1021 matters when refcount auditing is on.
Overview
Fixes several garbage-collection and object-lifecycle bugs that could leak kernel state or kill the run loop, building on the refcount audit from #1020.
Object ownership when exports retire: Adds
orphanKernelObjectso when an owner stops naming an export (retireExportdelivery orretireExports/abandonExportssyscalls), the kernel drops the owner mapping instead of leavingowner/refCountrecords past the c-list entry.performExportCleanupnow requires the caller to be the owner.KernelRouterGC delivery filters to live c-list entries, orphans on deliveredretireExports, releases the kernel side when a terminated vat or remote is gone, aborts (rolls back) when a vat is between incarnations, rolls back and terminates a vat if local GC delivery fails, and logs through remote delivery failures instead of stopping the loop. GC action batching sorts krefs beforehardenso two actions of one type for one vat no longer throw.getImportersincludes remote endpoints soretireImportreaches everyone holding the object.Vat lifecycle: Failed kernel-side registration after a worker starts now stops the worker and marks the vat terminated;
stopVatalways removes the in-memory handle even if unpin/terminate throws.Audit / tests:
RefCountViolationis nowkind: 'mismatch' | 'dangling'; audit failures are also logged.kernel-testaddstakeRunLoopFailureand an integration test that refcount violations surface toqueueMessagecallers (not silent hangs).Reviewed by Cursor Bugbot for commit 65a62cb. Bugbot is set up for automated code reviews on this repo. Configure here.