fix(ocap-kernel): free retired exports and harden GC delivery - #1022
Open
sirtimid wants to merge 7 commits into
Open
fix(ocap-kernel): free retired exports and harden GC delivery#1022sirtimid wants to merge 7 commits into
sirtimid wants to merge 7 commits into
Conversation
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>
Contributor
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
This was referenced Aug 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #1021, which is stacked on #1020. Review those first. 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, c-list teardown, and ownership invariants; mistakes can corrupt capability accounting or kill the run loop, and the PR notes a merge-order dependency on #1021 when refcount auditing is enabled.
Overview
Hardens kernel garbage collection and object ownership so retired exports do not leak owner records or crash the run loop, and GC delivery behaves correctly when endpoints vanish or refuse work.
Adds
orphanKernelObjectso when an owner stops naming an export (retireExportdelivery orretireExports/abandonExportssyscalls), the owner mapping is dropped and the object is handed to the collector.performExportCleanupnow requires the caller to be the owner, blocking a vat from disowning another endpoint’s export via an import vref.collectGarbagerepairs orphaned owner mappings when the owner’s c-list entry is already gone.KernelRouter.#deliverGCActionis reworked: filter krefs withhasCListEntry, release the kernel c-list even if the endpoint is gone (terminated vat or remote), but fail the crank for a vat that is absent yet not terminated (e.g. mid-restartVat). Local vat delivery failures abort and terminate; remote failures are logged and not retried (avoids GC starvation). Notify/send refcount fixes andRefCountViolationkind: 'mismatch' | 'dangling'tighten the audit story.VatManager.launchVatstops the worker and marks the vat terminated if kernel-side registration fails after the worker starts. Integration tests cover audit errors surfacing to callers and GC settling in multi-importer scenarios.Reviewed by Cursor Bugbot for commit 452b1e6. Bugbot is set up for automated code reviews on this repo. Configure here.