Uh oh!
There was an error while loading. Please reload this page.
fix: dispose call args on all failure paths per new StubHook ownership contract - #241
Conversation
🦋 Changeset detectedLatest commit: 7e7bea9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
commit: |
This comment was marked as outdated.
This comment was marked as outdated.
One semantics question I hit while in here: if a call's args contain unresolved promises, 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 |
ndisidore
commented
Aug 12, 2026
/bonk review this |
77ff252 to
1cd41dcCompare
This comment was marked as outdated.
This comment was marked as outdated.
ndisidore
commented
Aug 12, 2026
/bonk review this |
1cd41dc to
ed803feCompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…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.
b9f79da to
c93a265Compare- 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.
c93a265 to
a8be070CompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- 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.
Uh oh!
There was an error while loading. Please reload this page.
…orStubHook Per review: PromiseStubHook already handles a rejected backing promise -- it disposes the arguments of queued calls (since #241) and surfaces the error through pull() and onBroken() -- so the constructor no longer maps rejection to an ErrorStubHook resolution. Observable change: a pipelined call whose result is neither awaited nor disposed now fires an unhandled rejection event, matching the existing behavior of local async calls. The unhandled-rejection tests now dispose the discarded results, which both silences the event and models correct usage.
* feat: construct RpcPromise from a Promise Resolves the long-standing TODO on the RpcPromise constructor: the application may now pass a Promise (or any other thenable) for the eventual resolution. Calls made before the promise settles are queued and delivered in order once it does, so an RpcPromise can stand in for a capability that doesn't exist yet -- for example, one that will only become available after a broken session has been re-established. The promise may resolve to an RpcTarget, a stub, or a plain value. Promise.resolve() performs thenable assimilation natively, so no hand-rolled hardening against misbehaving thenables is needed. The resolution is adopted with return semantics (the same representation used for resolutions of local async calls), so awaiting delivers the value, pipelined calls forward through it without forcing a pull, and brokenness of a stub resolution is preserved. Passing an existing RpcPromise adopts its hook directly, keeping it lazy. A rejection is adopted as an ErrorStubHook rather than left to reject the backing promise, so the promise chains behind queued calls never reject: calls land on the ErrorStubHook (which disposes their arguments) and the error surfaces only through pull() or onBroken(). Without this, a discarded pipelined call on a promise-backed stub would raise an unhandled rejection event when the promise rejects (crashing Node under its default handling), even though fire-and-forget calls on the session-backed stub it stands in for reject only on pull. * fix: address review feedback on RpcPromise promise construction - Only adopt the hook of an existing RpcPromise; a bare stub's hook may not implement pull(), so bare stubs now take the generic path, whose resolution payload handles them correctly (await previously rejected with "Tried to resolve a non-promise stub."). Regression test added. - Inline hookForPromiseArg and hookForResolution into the constructor. - Collapse the constructor's type overloads into a single signature, narrowing the accepted type to Promise (runtime still assimilates arbitrary thenables). - Reframe the README section around the local-loopback RPC equivalence, and align the jsdoc and changeset with it. * fix: address code-review findings on RpcPromise-from-Promise - Adopting an existing RpcPromise now consumes the source: its hook is neutered to DISPOSED_HOOK, so disposing the source can no longer silently kill the wrapper. Using the source after wrapping reports the standard disposed error. - Restore the invariant that every RpcPromise has a defined path by defaulting pathIfPromise to [] on the internal StubHook path. - Wrap workerd-native RpcPromise/RpcProperty values (rpc-thenable) in a TargetStubHook so pipelined calls aren't eagerly assimilated. - Document ownership transfer on adoption and the dup() workaround for keeping a deferred capability lazy when resolving a native Promise with an RpcPromise. * docs: remove constructor special-case paragraphs per review Per review feedback on #242: the ownership-transfer note (nobody wraps an RpcPromise they already hold on purpose) and the thenable-assimilation note (not specific to this constructor) don't belong in the public docs. The behaviors themselves are unchanged and remain pinned by tests. * fix: repair dup(), onRpcBroken, and property adoption for promise-wrapped native stubs - get([]) on a thenable-backed TargetStubHook now returns dup() instead of throwing, fixing dup() and argument-passing of wrapped native promises. - onBroken() now subscribes to a thenable target's rejection, so onRpcBroken fires when a wrapped native promise rejects instead of silently no-oping. - Property promises share the source hook and path so the get() happens lazily on first use, avoiding eager wire pushes / getter side effects. * fix: let rejection reject the backing promise instead of adopting ErrorStubHook Per review: PromiseStubHook already handles a rejected backing promise -- it disposes the arguments of queued calls (since #241) and surfaces the error through pull() and onBroken() -- so the constructor no longer maps rejection to an ErrorStubHook resolution. Observable change: a pipelined call whose result is neither awaited nor disposed now fires an unhandled rejection event, matching the existing behavior of local async calls. The unhandled-rejection tests now dispose the discarded results, which both silences the event and models correct usage.
0.12.0 added a way to construct an `RpcPromise` from an ordinary `Promise`, so callers can pipeline against a capability you have not obtained yet. That is new public API, not a bug fix, and the docs said nothing about it -- main documented it in the root README, which this branch replaced with a pointer to here, so the merge would otherwise have dropped it on the floor. `concepts/promises.md` gets a section, and it leans on the equivalence the API docs make: wrapping a promise means the same thing as a local-loopback call returning it. That is worth stating plainly, because it derives all the rules that would otherwise have to be listed as a second set to memorise -- the resolution is serialized, targets and functions come back as stubs, ownership transfers, rejections propagate. The ownership one is the sharp edge and is called out: disposing the promise disposes the resolution, so resolve with a `.dup()` if you want to keep a stub. The cheat sheet gets a one-line constructor note, matching the one `RpcStub` already has. Also: a Changelog entry in the sidebar under Reference, pointing at the GitHub releases page. Off-site on purpose -- release notes are generated from changesets on every publish, so a page here would be a copy that goes stale the next time anyone ships. Nimbus recognises the absolute URL and adds `target="_blank" rel="noopener"` itself. Nothing else in the merge needed documenting. #241, #243, #251 and #253 are leak and typing fixes with no API surface to describe; #253's note that `RpcPromise<RpcStub<T>>` should now be written `RpcPromise<T>` applies to no annotation anywhere in these pages.
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.
Practical example:
lets say token is expired so authenticate() rejects. Every call pipelined behind it had already deep-copied its arguments, including a dup of
progressCallback's hook. Before this PR, those copies were simply dropped on the rejection path:progressCallback'sdisposer never runs, its entry stays pinned.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(), andmap()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)stream()(which leaked the result hook whenpull()threw)MapVariableHookmapplaceholderPromiseStubHooknow 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.