Skip to content

test: failing repros for four defects found reviewing #1012 - #1018

Draft
grypez wants to merge 5 commits into
sirtimid/crank-transaction-integrityfrom
grypez/pr-1012-review-repros
Draft

test: failing repros for four defects found reviewing #1012#1018
grypez wants to merge 5 commits into
sirtimid/crank-transaction-integrityfrom
grypez/pr-1012-review-repros

Conversation

@grypez

@grypez grypez commented Aug 10, 2026

Copy link
Copy Markdown
Member

Explanation

Four failing tests, one per must-fix defect found while reviewing #1012. Repros, not fixes — every test here is expected to fail on this branch. Each commit carries the mechanism, the A/B result against main where there was one, and what we hope to see instead.

Targeting sirtimid/crank-transaction-integrity so the fixes and their tests can land together in #1012 rather than as a follow-up against a known-broken main.

Test Defect
RemoteHandle.test.ts › reports the release failure rather than a missing savepoint handleRemoteMessage releases inside its try and rolls back in the catch. Now that a failed RELEASE discards the savepoint stack, that rollback throws No such savepoint in place of the real error — not even as cause. RemoteManager has the same shape at its peerIncarnation_* savepoint.
KernelQueue.test.ts › reports both failures when endCrank also fails Since the delivery rollback now spares crank, endCrank's release is a real RELEASE+COMMIT on the dying path where it used to be a no-op. #runLoop calls it from a bare finally, so when it throws it replaces the error that killed the kernel.
make-kernel.test.ts › gives the kernel store a logger No production call site passes a logger to makeSQLKernelDatabase, so the four logger?.error calls this PR adds are dead code.
wasm.test.ts › stops believing it is in a transaction when the commit fails commitIfNeeded still steps the COMMIT before clearing _inTx — the same ordering rollbackIfNeeded was corrected for. A throwing COMMIT wedges _inTx true, beginIfNeeded becomes a permanent no-op, and the next savepoint is created bare, which autocommits on release.

Two of these are regressions relative to main, verified by A/B: the remote release failure and the endCrank masking. The other two are gaps in what the PR set out to deliver.

Notes for the fixer

  • Assertions are fix-agnostic where more than one fix is defensible. The endCrank test walks the cause chain, so either wrapping (the shape rollbackCrank's path already uses) or reporting-and-rethrowing satisfies it — both verified. The RemoteHandle test doesn't care whether the release moves out of the try, the rollback failure is tolerated, or the driver's discard changes.
  • The RemoteHandle mock models the driver's bookkeeping, not the expected outcome — a savepoint stack that clears on a failed release and throws for an absent name, matching both drivers. So it's RemoteHandle's error handling under test. It also asserts the rollback is still attempted, so abandoning it isn't a way to pass.

Known gaps

  • RemoteManager has no repro. Same defect, same shape, and currently zero coverage of its savepoint path — so a fix applied to RemoteHandle and forgotten here would leave this suite green. Worth adding, ideally sharing one savepoint-stack model with the RemoteHandle test.
  • make-kernel covers only the nodejs driver. kernel-worker.ts:47 omits the logger too (and has no Logger in scope), so wasm.ts's two logger?.error calls stay dead even once this test passes.

Testing

The four target packages, on this branch: exactly four failures and no others. Previously-passing counts unchanged — kernel-store 96, ocap-kernel 2439, kernel-node-runtime 98. Each test was confirmed to go green under a minimal plausible fix applied in a throwaway worktree, and yarn eslint is clean on all four files.

🤖 Generated with Claude Code

grypez and others added 5 commits August 10, 2026 11:51
@grypez @claude
test(kernel-store): pin the failed COMMIT that wedges _inTx
9c1fa12 
Failing repro, not a fix.

## The issue

`rollbackIfNeeded` was corrected in #1012 to clear `_inTx` *before* stepping
the abort, because the abort can throw and `_inTx` is tracked in the driver
rather than read from SQLite. `commitIfNeeded` has the identical shape and was
left alone:

    function commitIfNeeded(): void {
      if (db._inTx && db._spStack.length === 0) {
        sqlCommitTransaction.step();   // can throw
        sqlCommitTransaction.reset();
        db._inTx = false;              // ...so this never runs
      }
    }

A COMMIT that throws leaves `_inTx` true against a database that may hold no
transaction. `beginIfNeeded` is then a no-op forever after, so the next
`createSavepoint` issues its SAVEPOINT outside a transaction — and a savepoint
taken outside a transaction commits when it is released
(Agoric/agoric-sdk#8423). That is the hazard the whole `beginIfNeeded` dance
exists to prevent, and `commitIfNeeded` is reached from `releaseSavepoint`,
which is the crank's commit point. The writes that leak are a whole crank's.

The nodejs driver is unaffected, for the same reason it was unaffected by the
abort case: it reads `db.inTransaction` live from SQLite.

Worth noting that the comment introduced above `stops believing it is in a
transaction when the abort fails too` asserts that a failed abort is "the one
case that can leave `_inTx` disagreeing with the database". This is the second
case, so that comment needs correcting along with the code.

## What we hope to see instead

`releaseSavepoint` still throws the COMMIT failure, but `_inTx` is false
afterwards, so the next `createSavepoint` opens a transaction of its own
instead of creating a bare savepoint. Same two-line reorder as
`rollbackIfNeeded`, and the "one case" comment updated.

## Current failure

    AssertionError: expected true to be false
      packages/kernel-store/src/sqlite/wasm.test.ts
      > stops believing it is in a transaction when the commit fails

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@grypez @claude
test(ocap-kernel): pin the endCrank failure that buries the real error
e8e70d9 
Failing repro, not a fix.

## The issue

#1012 fixes one error-masking path at the start of a dying crank and opens
another at its end.

Before the two-savepoint scheme, `rollbackCrank('start')` emptied
`ctx.savepoints`, so `endCrank` -> `releaseAllSavepoints` was a guaranteed
no-op on the dying path: nothing to release, nothing that could throw. Now
`rollbackCrank('delivery')` truncates to the ordinal and leaves `['crank']`
behind (crank.ts:56, deliberately — that is what keeps the transaction open for
the work an aborted crank still owes). So `endCrank` issues a real
`RELEASE t0`, which commits, which can fail.

`#runLoop` calls it from a bare `finally`:

    } finally {
      this.#kernelStore.endCrank();
      ...
    }

A throw there replaces the pending exception. The disk error that actually
killed the kernel is discarded — not demoted to `cause`, discarded — and
`run()` rejects with the release failure instead. `#failRunLoop` records that,
so `getRunLoopStatus().detail` loses the root cause too, and
`onRunLoopFailure` — what the daemon logs as fatal — gets the wrong error.

A/B against origin/main with the same repro: main reports `crank exploded`,
this branch reports `database is gone` with `cause: undefined`.

This is the same class of bug as the `No such savepoint: t0` masking that
82b88ce fixes, and the same class the `reports both failures when the
rollback also fails` test above already guards on the other path.

## What we hope to see instead

Whatever names the release failure, the error that killed the crank stays
reachable. The rollback path already has the shape to copy:

    throw new Error(
      `Run loop died and its crank could not be rolled back: ${...}`,
      { cause: error },
    );

The assertion is deliberately fix-agnostic — it walks the `cause` chain — so
either wrapping `endCrank`'s failure with the original as `cause`, or reporting
it and rethrowing the original, will satisfy it.

## Current failure

    AssertionError: expected [ Error: database is gone ]
      to include Error: crank exploded
      packages/ocap-kernel/src/KernelQueue.test.ts
      > reports both failures when endCrank also fails

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@grypez @claude
test(kernel-node-runtime): pin the kernel store's missing logger
a8fc301 
Failing repro, not a fix.

## The issue

#1012 replaces four silently-swallowed aborts with `logger?.error(...)` in the
SQLite drivers, and its description says: "Four swallowed aborts were silent.
Now logged." They are not. No production call site passes a `logger` to
`makeSQLKernelDatabase`, so every one of those calls is dead code:

    packages/kernel-node-runtime/src/kernel/make-kernel.ts:63
    packages/kernel-browser-runtime/src/kernel-worker/kernel-worker.ts:47
    packages/kernel-test-local/src/lms-chat.ts:30
    packages/kernel-node-runtime/test/helpers/remote-comms.ts:172

`make-kernel.ts` is the clearest case: it builds a `rootLogger` and hands
sub-loggers to `NodejsPlatformServices` and to `Kernel.make`, then constructs
the store with `{ dbFilename }` alone. The store is the one collaborator that
gets no logger. Nor does any test pass one, which is why the gap survived
review.

This matters more than a missing log line. On the nodejs driver a failed abort
leaves `db.inTransaction` true with nothing that will ever commit or abort it,
so later writes on that connection join a transaction that vanishes on close.
The driver's own comment concedes "Nothing here can repair that" — the log is
the entire remedy, and it does not reach anyone.

`logger?.error` is the right convention for this package; the injection is what
is missing.

## What we hope to see instead

`makeKernel` passes a tagged sub-logger to `makeSQLKernelDatabase`, as it
already does for its other collaborators — something like
`rootLogger.subLogger({ tags: ['store'] })`. The other three call sites want the
same treatment, and are worth covering once this one is fixed.

## Current failure

    AssertionError: expected "vi.fn()" to be called with arguments:
      [ ObjectContaining{…} ]
    -     "logger": Any<Logger>,
      packages/kernel-node-runtime/src/kernel/make-kernel.test.ts
      > gives the kernel store a logger

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@grypez @claude
test(ocap-kernel): pin the release failure lost at the remote savepoint
f0e63f3 
Failing repro, not a fix.

## The issue

#1012 hardens `releaseSavepoint` so that a failed `RELEASE` discards the
enclosing transaction, clearing the driver's `_spStack` on the way. Two callers
it does not touch depend on the old behaviour, and both are now worse off than
before the change.

`RemoteHandle.handleRemoteMessage` releases inside the `try` and rolls back in
the `catch`:

    this.#kernelStore.setRemoteHighestReceivedSeq(this.remoteId, seq);
    this.#kernelStore.releaseSavepoint(savepointName);   // fails
  } catch (error) {
    this.#kernelStore.rollbackSavepoint(savepointName);  // "No such savepoint"
    throw error;                                        // never reached
  }

Since the release already cleared the stack, the rollback throws
`No such savepoint: receive_r0_1`, which escapes the `catch` and replaces the
real failure. Not demoted to `cause` — replaced. `RemoteManager` has the same
shape at its `peerIncarnation_*` savepoint.

A/B verified against origin/main with a real driver: main's rollback succeeds
and `database or disk is full` propagates; on this branch the caller gets the
missing-savepoint error instead. So the PR description's "the release failure
still propagates" holds for the crank path it fixed and not for these two.

`crank.ts:57-63` shows the author recognised exactly this hazard — a stale
savepoint list producing `No such savepoint` over the real error — and fixed it
for the crank only. The remote paths were missed because nothing exercised them.

Note the secondary effect these tests don't reach: `ctx.savepoints` still lists
the crank's own savepoints after this, so the next `endCrank` throws
`No such savepoint: t0` over whatever is left of the failure.

## What we hope to see instead

The failure the database reported is what reaches the caller. Any of these does
it, and the assertion doesn't care which:

- move the release out of the `try`, so a release failure isn't followed by a
  rollback attempt at all
- have the `catch` tolerate a rollback that reports a savepoint already
  discarded, rethrowing the original either way
- make the driver's discard leave the name rollback-able as a no-op

The mock models the drivers' bookkeeping rather than the expected outcome, so it
is `RemoteHandle`'s error handling under test, not the mock's.

## Current failure

    AssertionError: expected Error: No such savepoint: receive_r0_1
      to be Error: database or disk is full
      packages/ocap-kernel/src/remotes/kernel/RemoteHandle.test.ts
      > reports the release failure rather than a missing savepoint

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@grypez @claude
test: tighten the four repros after review
f3af96f 
No change to what any of them proves; all four still fail for the reasons
their own commits describe.

- `RemoteHandle`: assert the rollback is still *attempted*. Without this,
  deleting the rollback from the `catch` outright would turn the test green,
  which is not the fix — a `RELEASE` that failed for a reason of its own may
  well have left the savepoint standing.
- `RemoteHandle`: drop an unnecessary `as KernelStore` cast, and say why the
  store is replaced wholesale rather than having its methods assigned over
  (`makeKernelStore` hardens what it returns).
- `make-kernel`: note that `kernel-worker.ts` omits the logger too, so the wasm
  driver's pair of `logger?.error` calls stays dead even once this test passes.
  Use `vi.mocked`, as the sibling `make-kernel-options.test.ts` does.
- `causeChain` returns `Error[]`; every element is already narrowed by the loop
  guard.
- Drop "see the commit message for this test" from the four comment blocks: each
  stands alone, and the reference would not survive a squash-merge. Restate the
  claim the wasm comment made by citing a neighbouring test's title, which would
  have broken silently on rename.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@grypez grypez added the no-changelog Indicates that no changelog updates are required, and that related CI checks should be skipped. label Aug 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

no-changelog Indicates that no changelog updates are required, and that related CI checks should be skipped.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@grypez