Skip to content

fix(ocap-kernel): leave a settled result promise alone on delivery failure - #1084

Merged
sirtimid merged 3 commits into
mainfrom
sirtimid/reject-result-if-pending
Sep 18, 2026
Merged

sirtimid merged 3 commits into
mainfrom
sirtimid/reject-result-if-pending

Conversation

@sirtimid

@sirtimid sirtimid commented Sep 15, 2026 •

Copy link
Copy Markdown
Contributor

A delivery can fail after the endpoint has already settled the message's result promise: a vat resolves the result and then loses its stream, or the stream dies mid-delivery and retiring the vat rejects every promise it was deciding — this one included, its decider having been set just before the delivery.

#deliverSend's catch rejected the result unconditionally, and resolving a settled promise is a Fail. So the kernel threw from inside the very catch that was handling the delivery's failure, the throw escaped the crank, and the run loop died naming the promise rather than the dead worker.

#rejectResultIfPending reads the promise's state first and leaves a settled one alone, logging that it did: the caller then gets the endpoint's answer instead of DELIVERY_FAILED, and the error line above it is the only other trace of either outcome.

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

Changes

  • KernelRouter#rejectResultIfPending replaces the direct resolvePromises call in the delivery-failure catch, returning early when the result is not unresolved.
  • The early return logs the result's kref, the endpoint and the state it is in.
  • @metamask/ocap-kernel changelog entry under Fixed.

Testing

KernelRouter.result-promise.test.ts is new, because KernelRouter.test.ts mocks resolvePromises and so cannot see a second resolution of one promise. It runs a real nodejs SQLite :memory: store and a real KernelQueue through the run loop's crank — same savepoint name, same collectGarbage and reference count audit at the end, no catch, so a throw escapes as it does in the real loop.

The it.each over an endpoint that fulfils and one that rejects is the regression guard: with the production hunk reverted, both fail with "kp1" was already resolved escaping KernelRouter.deliver. The positive case, rejects it once the delivery reports failure, stays green under that mutation and fails under a guard that always returns early.

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

🤖 Generated with Claude Code


Note

Medium Risk
Touches kernel message delivery and promise settlement on failure paths; incorrect handling could still crash the run loop or mis-report errors to callers.

Overview
Fixes a run-loop crash when message delivery fails after the vat has already settled the message’s result promise (e.g. resolve then stream loss).

KernelRouter’s delivery-failure handler no longer unconditionally rejects the result with DELIVERY_FAILED. New private helper #rejectResultIfPending reads the kernel promise state first: only unresolved results get DELIVERY_FAILED; if the endpoint already fulfilled or rejected it, that outcome is kept and an error is logged instead of calling resolvePromises again (which would Fail and escape the catch).

Adds KernelRouter.result-promise.test.ts with a real SQLite store and queue (existing router tests mock resolvePromises). Unit test setup defaults getKernelPromise to { state: 'unresolved' }. Changelog updated under Fixed.

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

@sirtimid
sirtimid requested a review from a team as a code owner September 15, 2026 16:47
@sirtimid
sirtimid force-pushed the sirtimid/reject-result-if-pending branch from 6086dce to 28accc1 Compare September 18, 2026 17:19
@github-actions

github-actions Bot commented Sep 18, 2026 •

Copy link
Copy Markdown
Contributor

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 73.05%
⬆️ +0.01%
9791 / 13403
🔵 Statements 72.9%
⬆️ +0.01%
9955 / 13655
🔵 Functions 73.61%
⬆️ +0.01%
2302 / 3127
🔵 Branches 67.3%
⬆️ +0.01%
4025 / 5980
File Coverage
File Stmts Branches Functions Lines Uncovered Lines
Changed Files
packages/ocap-kernel/src/KernelRouter.ts 94.4%
⬆️ +0.20%
81.15%
⬆️ +0.56%
100%
🟰 ±0%
94.4%
⬆️ +0.20%
110, 173, 190, 292, 345, 405, 423, 426
Generated in workflow #4861 for commit b7c95ea by the Vitest Coverage Report Action

@sirtimid
sirtimid force-pushed the sirtimid/reject-result-if-pending branch from 28accc1 to 4ad301b Compare September 18, 2026 17:36

@FUDCo FUDCo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice catch.

sirtimid and others added 2 commits September 18, 2026 21:11
…ilure

A failed delivery may have settled its result on the way down: the vat
resolved it and then lost its stream. Resolving a settled promise is a
`Fail`, thrown from inside the catch that is handling the delivery's
failure, so it escaped the crank and killed the run loop.

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

Review follow-up. The caller then gets the endpoint's answer rather than
`DELIVERY_FAILED`, and nothing else said so. The regression test runs the
crank's reference count audit, and `KernelRouter.test.ts`'s shared store
mock reports an unresolved promise by default.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sirtimid
sirtimid force-pushed the sirtimid/reject-result-if-pending branch from 4ad301b to 5f25173 Compare September 18, 2026 19:22
@sirtimid
sirtimid enabled auto-merge September 18, 2026 19:24

@FUDCo FUDCo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Eventually it converges. It does eventually converge, right?

@sirtimid
sirtimid added this pull request to the merge queue Sep 18, 2026
Merged via the queue into main with commit ffac915 Sep 18, 2026
30 checks passed
@sirtimid
sirtimid deleted the sirtimid/reject-result-if-pending branch September 18, 2026 19:47
sirtimid added a commit that referenced this pull request Oct 1, 2026
…e test

#1084 landed with a router built positionally, so the logger took the slot
the new parameter added and the `already settled` assertions saw nothing.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
sirtimid added a commit that referenced this pull request Oct 1, 2026
…e test

#1084 landed with a router built positionally, so the logger took the slot
the new parameter added and the `already settled` assertions saw nothing.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
sirtimid added a commit that referenced this pull request Oct 2, 2026
…e test

#1084 landed with a router built positionally, so the logger took the slot
the new parameter added and the `already settled` assertions saw nothing.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
sirtimid added a commit that referenced this pull request Oct 6, 2026
…e test

#1084 landed with a router built positionally, so the logger took the slot
the new parameter added and the `already settled` assertions saw nothing.

Co-Authored-By: Claude Opus 5.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.

2 participants