Skip to content

fix(ocap-kernel): credit the crank buffer in the reference count audit - #1095

Open
sirtimid wants to merge 3 commits into
sirtimid/two-crank-savepointsfrom
sirtimid/audit-credits-crank-buffer
Open

sirtimid wants to merge 3 commits into
sirtimid/two-crank-savepointsfrom
sirtimid/audit-credits-crank-buffer

Conversation

@sirtimid

@sirtimid sirtimid commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Stacked on #1090, which is stacked on #1087. Review those first; this PR's own
diff is
git diff sirtimid/two-crank-savepoints...sirtimid/audit-credits-crank-buffer.

computeExpectedRefCounts totals the references the kernel holds by walking the
store — the run queue, c-list entries, promise state, pins — and never looked at
ctx.crankBuffer. A buffered send or notify holds its references all the same:
enqueueSend and enqueueNotify charge for the target, the result and every
slot at the moment they buffer the item, and the run queue row those units
belong to appears only when flushCrankBuffer moves the item across. Between
the charge and the flush the audit saw the counts but not the holder, and
reported a leak.

That is why the audit had to run after the flush. It is also why a violation
could only ever kill the run loop: the flush is what settles the promise
enqueueMessage gave an external caller, so by the time the audit spoke, the
answer had gone out and the state it was computed from could no longer be rolled
back. Crediting the buffer removes the constraint, which is the fix #1062 asks
for and answers claim 2 of #1039.

The audit therefore goes first — but only while a violation can still undo the
crank. Once a vat's death has been recorded, #1090 withholds the delivery
rollback, so there is nothing left to undo and the crank commits whatever
happens; holding the flush back past the audit there would commit the buffered
sends' charges with the sends themselves never written to the run queue. On that
path the flush goes first and each queue row keeps the charge it was given.

Changes

  • store/methods/refcount-audit.ts: computeExpectedRefCounts credits
    ctx.crankBuffer, a send by its target, result and slots and a notify by its
    kpid, exactly as the run queue is credited.
  • KernelQueue.#processCrankResult: the audit runs before the flush while
    #deliveryRollbackAllowed holds, and after it once it does not.
  • @metamask/ocap-kernel changelog entry under Fixed.

Testing

refcount-audit.test.ts gains a crank buffer block: an it.each over a
buffered send, one carrying a slot, one carrying a result promise and two
buffered at once, plus a buffered notify — and one case where a count no
buffered item accounts for is still reported, so the credit cannot be read as
"stay quiet whenever anything is buffered". Each shape is mutation-checked
separately: crediting only the first item fails one case, dropping the result
credit fails another.

KernelQueue.test.ts covers both orders, and covers them behaviourally as well
as by call order: an audit that fires leaves the external caller unanswered, and
a crank that recorded a vat death flushes first. Pinning the order to either
constant fails one side or the other.

KernelQueue.crank-transaction.test.ts runs the real KernelQueue.run against a
real nodejs SQLite :memory: store with auditing on, through a delivery that
buffers a send the way a vat's syscall.send does. Without the credit the first
crank's audit reports that send as a leak and the second delivery never happens.

Full @metamask/ocap-kernel suite green locally; eslint, constraints and
changelog:validate clean. @ocap/kernel-test was not usable as evidence: it
dies with a native better-sqlite3 assertion (RemoveEnvironmentCleanupHook,
Assertion failed: (env) != nullptr) at worker teardown on origin/main too.

Closes #1062

🤖 Generated with Claude Code


Note

Medium Risk
Touches per-crank refcount invariants and ordering of audit, flush, and promise settlement in the kernel run loop—incorrect ordering could false-positive audits or settle callers before rollback.

Overview
Fixes false reference-count leaks when a vat buffers sends/notifies mid-crank: computeExpectedRefCounts now credits ctx.crankBuffer (target, slots, result promise, notify kpid) the same way it credits the run queue.

KernelQueue.#processCrankResult reorders audit vs flush. While delivery can still roll back (#deliveryRollbackAllowed), assertRefCountsIfAuditing runs before #flushCrankBuffer, so an audit failure stops the kernel before external enqueueMessage promises settle from a state that would be undone. After a vat death commits the crank (rollback no longer allowed), flush runs first so buffered items reach the run queue before audit—avoiding charges with no matching queue row.

Changelog Fixed entry and tests cover buffer crediting shapes, audit/flush call order, unanswered callers on audit failure, and an integration path with auditing on and a mid-crank buffered send.

Reviewed by Cursor Bugbot for commit 6bd5923. Bugbot is set up for automated code reviews on this repo. Configure here.

sirtimid and others added 2 commits September 15, 2026 19:55
A buffered send or notify holds its references already — `enqueueSend`
and `enqueueNotify` charge for the target, the result and every slot as
they buffer it — but `computeExpectedRefCounts` saw the counts without
the holder, so any crank that buffered anything read as a leak. That is
why the audit had to run after the flush, and the flush is also what
settles the promise `enqueueMessage` gave an external caller: a
violation could only be found once the answer had gone out.

With the buffer credited, the audit runs first.

Closes #1062

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ll be undone

Review follow-up. Flushing after the audit unconditionally was wrong on
the one path that terminates without aborting: `#terminateVat` has
already withheld the delivery rollback, so a throw from the audit
committed the crank with the buffered sends' charges applied and the
sends themselves never written to the run queue — messages lost and
counts inflated on disk. Where nothing can undo the crank any more, the
flush goes first and each queue row keeps the charge it was given.

The credit's shape is now pinned too: a result promise, a second
buffered item, and a count no buffered item accounts for.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sirtimid
sirtimid requested a review from a team as a code owner September 15, 2026 18:30
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 6bd5923. Configure here.

}
if (!auditBeforeFlush) {
this.#kernelStore.assertRefCountsIfAuditing();
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flush skipped after vat death

High Severity

#flushCrankBuffer now runs after #terminateVat and collectGarbage. Both can throw after #deliveryRollbackAllowed is already false, so the crank commits refcount charges for buffered sends and notifies that never reach the run queue. Successful vat death also enqueues termination resolutions before the dying vat's last messages.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 6bd5923. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant