Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions .changeset/settled-import-guard.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
---
"capnweb": patch
---

Don't cite a released import id after the reference has settled.

`ImportTableEntry.resolve()` stores the resolution and immediately calls `sendRelease()`, so the
import is dead on both sides — but the entry keeps `importId`, since the release accounting names
the id through it. `getImport()` still returned that id, so passing a settled `RpcPromise` back as
an argument re-serialized a released id. The peer could not find it and threw inside `readLoop`,
which is wrapped in a single session-wide `.catch(err => this.abort(err))` — so a call-level fault
destroyed the whole session. `getImport()` now declines a settled entry and the caller exports a
fresh stub instead, matching `dispose()`, `abort()` and `onBroken()`, which already branch on
`resolution`.
52 changes: 52 additions & 0 deletions __tests__/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4227,3 +4227,55 @@ describe("deserialization and transport correctness", () => {
expect(sentReason).toBe("a".repeat(MAX_CLOSE_REASON_BYTES - 1));
});
});

describe("settled import references", () => {
// Regression: ImportTableEntry.resolve() stores `resolution` and immediately calls
// sendRelease(), so the import id is dead on both sides -- but the entry keeps `importId`,
// because the release accounting names the id through it. getImport() then still handed out
// that dead id, so passing the settled promise object into a later message re-serialized a
// released id. The peer could not find it and threw inside readLoop, and because readLoop is
// wrapped in a single session-wide `.catch(err => this.abort(err))`, a call-level fault
// destroyed the entire session.
//
// dispose(), abort() and onBroken() all already branch on `resolution`; getImport() did not.
it("does not cite a released import id after the reference has settled", async () => {
await using harness = new TestHarness(new TestTarget());

// Awaiting settles the entry: the resolution is stored and the import is released.
let promise = harness.stub.returnNumber(7);
await promise;

// Passing the same *promise object* (not its awaited value) as an argument is what
// re-serialized the now-dead id.
Comment thread
Kanishkrawatt marked this conversation as resolved.
expect(await harness.stub.square(promise)).toBe(49);

// The load-bearing assertion: a failing call would be tolerable, a dead session is not.
expect(await harness.stub.returnNumber(3)).toBe(3);
});

// Same as above, but through a property path on the settled promise, which the serializer
// resolves via hook.get(path) on the settled entry rather than hook.dup().
it("does not cite a released import id via a property of a settled reference", async () => {
await using harness = new TestHarness(new TestTarget());

let promise = harness.stub.callSquare(harness.stub, 7);
await promise;

expect(await harness.stub.square(promise.result)).toBe(2401);
expect(await harness.stub.returnNumber(3)).toBe(3);
});

// A guard that cleared `importId` instead of gating its use would stop the release
// accounting naming the id, leaking the peer's exports.
it("still releases settled imports by id", async () => {
Comment thread
Kanishkrawatt marked this conversation as resolved.
await using harness = new TestHarness(new TestTarget());

await harness.stub.returnNumber(1);
await harness.stub.returnNumber(2);
await pumpMicrotasks();

// Both settled imports must have been released on the wire, not just dropped locally.
expect(harness.client.getStats()).toStrictEqual({imports: 1, exports: 1});
expect(harness.server.getStats()).toStrictEqual({imports: 1, exports: 1});
});
});
6 changes: 5 additions & 1 deletion src/rpc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -662,7 +662,11 @@ class RpcSessionImpl implements Importer, Exporter {
}

getImport(hook: StubHook): ImportId | undefined {
if (hook instanceof RpcImportHook && hook.entry && hook.entry.session === this) {
// A settled entry has already released its import (resolve() calls sendRelease()), so its
// importId no longer names anything on the peer. Fall through to exporting the resolution,
// the way dispose(), abort() and onBroken() all branch on `resolution`.
if (hook instanceof RpcImportHook && hook.entry && hook.entry.session === this &&
!hook.entry.resolution) {
return hook.entry.importId;
} else {
return undefined;
Expand Down
Loading