Skip to content

fix: dispose call args on all failure paths per new StubHook ownership contract - #241

Open
ndisidore wants to merge 6 commits into
mainfrom
fix/promise-stubhook-disposal
Open

fix: dispose call args on all failure paths per new StubHook ownership contract#241
ndisidore wants to merge 6 commits into
mainfrom
fix/promise-stubhook-disposal

Conversation

@ndisidore

@ndisidore ndisidore commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

RPC call arguments (and map captures) leaked whenever a call failed before reaching a callee. A rejected pipeline promise, a broken or disposed stub, a failed argument serialization, or a sync throw inside a hook.

Rather than patching each site with caller-side try/catch (as an earlier revision of this PR did), the fix defines the rule once on the abstract StubHook: call(), stream(), and map() take ownership of their args/captures even when they throw synchronously, and callers never dispose after invoking.

The implementations that violated this are fixed

  • RpcImportHook.call()/stream() (including mid-serialization failures, where disposal is safe because exported hooks are dups and the Devaluator rolls back its exports)
  • -the default stream() (which leaked the result hook when pull() threw)
  • MapVariableHook
  • the map placeholder

PromiseStubHook now disposes its copied args only when its backing promise rejects i.e. where no callee ever existed to take them.

Also kept from the original PR: PromiseStubHook.dispose() chains on the backing promise so disposal stays ordered behind already-queued calls.

@changeset-bot

changeset-bot Bot commented Aug 12, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 7e7bea9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
capnweb Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@pkg-pr-new

pkg-pr-new Bot commented Aug 12, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/cloudflare/capnweb@241

commit: 7e7bea9

@ask-bonk

This comment was marked as outdated.

@ndisidore

ndisidore commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author

One semantics question I hit while in here: if a call's args contain unresolved promises, deliverCall() waits on them before invoking the target, so a dispose() right after the call can win the race and the method runs on a disposed target.

Pre-existing stuff: this PR just keeps disposal from overtaking forwarding, same as calling the destination hook directly. Is that intended, or should disposal also wait for in-flight deliveries? (That'd be a deliverCall/TargetStubHook lifetime change, so I didn't touch it.)

@ndisidore

Copy link
Copy Markdown
Collaborator Author

/bonk review this

@ndisidore
ndisidore force-pushed the fix/promise-stubhook-disposal branch from 77ff252 to 1cd41dc Compare August 12, 2026 20:50
@ndisidore
ndisidore marked this pull request as ready for review August 12, 2026 20:51
@ask-bonk

This comment was marked as outdated.

@ndisidore

Copy link
Copy Markdown
Collaborator Author

/bonk review this

@ndisidore
ndisidore force-pushed the fix/promise-stubhook-disposal branch from 1cd41dc to ed803fe Compare August 12, 2026 21:35
Comment thread src/core.ts
Comment thread src/core.ts Outdated
Comment thread src/core.ts Outdated
ndisidore and others added 4 commits August 13, 2026 20:28
…ind queued calls

PromiseStubHook.call() and .stream() deep-copy their arguments before
chaining on the backing promise, but if that promise rejects, the copies
were never disposed, leaking any stubs they contained.

PromiseStubHook.dispose() also had a fast path that disposed the
resolution synchronously once available. A call chained on the promise
just before disposal could then be delivered after its target was
already disposed, violating ordering. Disposal now always chains on the
promise so it stays behind previously queued calls.
ErrorStubHook.call() ignored the arguments it takes ownership of, and
ErrorStubHook.map() likewise ignored its captures, so anything forwarded
to a broken or disposed hook leaked. ValueStubHook.call() had the same
gap on its error path (its own map() already disposes captures there),
and PromiseStubHook's forwarding continuations did not clean up when the
destination hook threw synchronously.
Co-authored-by: Kenton Varda <kenton@cloudflare.com>
… args

Document on the abstract StubHook that call(), stream(), and map() take
ownership of their args/captures even on synchronous throw, and fix the
implementations that violated it:

- RpcImportHook.call()/stream(): dispose args if getEntry() throws, and in
  sendCall()/sendStream() when aborted or when argument serialization fails
  (safe: exported hooks are dups and the Devaluator rolls back its exports).
- Default StubHook.stream(): dispose the result hook if pull() throws.
- MapVariableHook and the map-not-loaded placeholder: dispose args/captures
  before throwing.
- ValueStubHook.call(): restructure to the inner-catch pattern so delegation
  clearly hands ownership to the delegate.
- PromiseStubHook: drop the success-path try/catch around hook.call()/
  hook.stream() -- per the contract the resolved callee owns the args; keep
  disposal only on rejection, where no callee ever existed.
@ndisidore
ndisidore force-pushed the fix/promise-stubhook-disposal branch from b9f79da to c93a265 Compare August 14, 2026 02:02
- RpcImportHook: collapse the three hand-rolled getEntry() guards in
  call()/stream()/map() into a private getEntryTakingOwnership() helper.
- ValueStubHook.map(): flatten the nested try/catch into a single catch that
  disposes captures defensively (dispose() is idempotent), matching call()'s
  flat shape.
- Tests: SyncThrowingHook extends ErrorStubHook instead of hand-stubbing all
  eight abstract StubHook methods.
- Condense the changeset to changelog style.
@ndisidore
ndisidore force-pushed the fix/promise-stubhook-disposal branch from c93a265 to a8be070 Compare August 14, 2026 02:06
@ndisidore ndisidore changed the title fix: PromiseStubHook leaks call args on rejection and can dispose out of order fix: dispose call args on all failure paths per new StubHook ownership contract Aug 14, 2026
Comment thread src/core.ts Outdated
Comment thread .changeset/promise-stubhook-disposal.md Outdated
Comment thread src/core.ts
- Revert the mapImpl placeholder change: the stubs are always replaced at
  startup, so handling disposal there is dead code.
- ValueStubHook.map(): restore the narrow inner catch. Once ownership of the
  captures transfers to the delegate or applyMap(), disposing them in the
  outer catch is incorrect (a partially-completed callee may hold live
  references, e.g. hooks stored in the export table).
- Changeset: drop implementation details from the changelog entry.
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