Skip to content

fix(ocap-kernel): revert the in-memory caches a crank rollback leaves behind - #1087

Open
sirtimid wants to merge 4 commits into
mainfrom
sirtimid/crank-rollback-reverts-caches
Open

sirtimid wants to merge 4 commits into
mainfrom
sirtimid/crank-rollback-reverts-caches

Conversation

@sirtimid

@sirtimid sirtimid commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

A crank rollback reverts the database and nothing else. Two kinds of state sit
above it and stayed as the abandoned crank left them.

Every cached stored value closes over the last value written through it, so
after a rollback the kernel went on reading the terminated vats and GC actions
the crank had recorded, and the next set wrote them straight back to a
database that no longer had them.

maybeFreeKrefs lives only in RAM. A promise the rollback deleted stayed a
collection candidate, so the next crank's collectGarbage read a promise that
was not there and killed the run loop over work that no longer existed.

revertStateBeneathRollback handles both. The candidate set is restored from a
snapshot the savepoint carries, not cleared: only collectGarbage empties it,
so a candidate added while no crank was open — a peer restart abandoning every
export it held — is still owed a collection and has to survive an unrelated
crank's rollback.

The same staleness was reachable from Kernel.clearStorage(), which emptied the
database and left every cache and the run queue head naming deleted rows. Now
that there is something to call, it calls it.

This is one PR in the split of #1021#1023 into single-defect changes.

Changes

  • store/index.ts: one CACHED_VALUES table declaring every cached stored
    value, keyed by the context's own cached fields so a value added to one and
    not the other does not compile, and context.refreshCachedValues() over it.
  • store/types.ts: savepoints becomes Savepoint[], each carrying the
    collection-candidate set as it stood when the savepoint was taken.
  • store/methods/crank.ts: revertStateBeneathRollback on both rollback paths.
    A failed rollback discards the whole transaction, so RAM goes back to the
    outermost savepoint rather than the named one; the candidate restore runs
    first, being the one step that cannot itself fail.
  • store/index.ts: clear() and reset() share one discardCachedState().
  • @metamask/ocap-kernel changelog entries under Fixed.

Deliberately not here: reverting the caches when releaseAllSavepoints fails
(round-3 finding 8). That rests on a failed RELEASE discarding the
transaction, which is a driver change landing separately; folding it in now
would assert behaviour main does not have.

Testing

store/methods/crank.rollback.test.ts is the regression guard and runs against
real nodejs SQLite :memory: — the map store's rollbackSavepoint is a no-op
and so cannot tell a reverted cache from a stale one. All three cases fail with
the production hunk reverted; removing only refreshCachedValues() fails the
two cache cases and leaves the candidate case green.

store/methods/crank.cross-crank-gc.test.ts is adopted from @grypez's #1039
claim 4. These pass on main — they are not a repro, they guard the new
mechanism against over-correcting. Replacing the snapshot restore with a bare
clear() fails two of the three.

store/methods/crank.test.ts gains three cases over a mocked database, for the
paths a real database will not produce on demand: which savepoint's snapshot is
restored, what happens when the rollback itself throws, and what the error says
when the revert then fails too. store/index.test.ts gains one for clear(),
which had no unit test at all; without the fix it dies with
No value found for key queue.run.1.

Full @metamask/ocap-kernel suite green locally; eslint, constraints and
changelog:validate clean.

Closes #1071

🤖 Generated with Claude Code


Note

High Risk
Touches crank transaction boundaries, GC collection, and run-queue behavior—bugs here can kill or strand the kernel run loop after rollback or storage clear.

Overview
Crank rollback and storage clear now discard in-memory kernel-store caches that DB rollback alone cannot undo, preventing the run loop from acting on terminated vats, queued GC actions, or collection candidates for promises that were rolled back.

On rollbackCrank, savepoints snapshot maybeFreeKrefs; revertStateBeneathRollback restores that set (so peer-restart orphans still collect later), refreshes the run queue and length cache, and refreshCachedValues() re-reads every StoredValue cache from a single CACHED_VALUES table so closures cannot write abandoned state back. Failed DB rollbacks revert RAM to the outermost savepoint. clear() / reset() share discardCachedState() so an emptied database no longer leaves a stale run-queue head.

Tests cover SQLite rollback regressions, cross-crank GC candidate survival, mocked rollback failure paths, and clear() dequeue after wipe.

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

sirtimid and others added 2 commits September 15, 2026 18:56
… behind

A rollback reverts the database and nothing else. Every cached stored
value closes over the last value written through it, so the kernel went
on reading the abandoned crank's terminated vats and GC actions, and the
next `set` wrote them back. `maybeFreeKrefs` lives only in RAM, so a
promise the rollback deleted stayed a collection candidate and the next
crank's `collectGarbage` died reading it.

Savepoints now carry a snapshot of the candidate set, restored rather
than cleared: candidates added while no crank was open are still owed a
collection and must survive an unrelated crank's rollback.

Closes #1071

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

Review follow-up. `clear()` left every cache pointing at rows it had
deleted, so the next dequeue killed the run loop — the same defect one
function over, now that there is something to call. A failed rollback
discards the whole transaction, so RAM goes back to the outermost
savepoint rather than the named one. The candidate set is restored
first, being the one step of the revert that cannot fail, and
`CACHED_VALUES` is keyed by the context's own cached fields so a value
added to one and not the other does not compile.

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 17:19
sirtimid and others added 2 commits September 15, 2026 19:19
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.

An out-of-crank savepoint rollback leaves the run-queue cache ahead of the database

1 participant