Don't let AbortSignals passed over JS RPC keep the callee's request open by npaun · Pull Request #8139 · cloudflare/workerd

npaun · 2026-11-02T02:32:05Z

When a Worker passes an AbortSignal over JS RPC, the callee's ExternalPusher.pushAbortSignal() returns a signal and a trigger through the session's CompletionMembrane. While the caller held either, the membrane counted the session as in use, so the callee's request stayed open after it returned and ended as canceled.

CompletionMembrane and RevokerMembrane redirect pushAbortSignal() to a forwarder that returns trigger unwrapped. A trigger the caller holds no longer keeps the session open, and isn't revoked when the session ends, so a later abort() still reaches the signal.
The forwarder wraps signal in the membrane's DetachedMembranePolicy, which holds no reference to the membrane. The caller's copy of signal no longer keeps the session open either, and passing it back still unwraps it, so the callee recognizes it.

A new test checks the callee's outcome, and that an abort sent after the callee returned still arrives, including for Durable Objects and after a Durable Object reset. The race the second change fixes needs the callee in another process, so it doesn't reproduce in workerd's tests; the internal test suite covers it.

Possible follow-ups

When a Durable Object receives an AbortSignal over JS RPC and registers an onabort or abort listener on it, the pending listener no longer keeps the Durable Object's request open waiting for an abort that may
never come. The request ends when the method returns, and a later abort still fires the listener. Without this fix, such a request stays open until the caller's RPC session is torn down and then ends as canceled,
and the waiting listener also stops the Durable Object from hibernating.

🤖 Generated with Claude Code

npaun · 2026-11-02T02:52:01Z

@@ -3542,8 +3564,8 @@ kj::Promise<WorkerInterface::CustomEvent::Result> JsRpcSessionCustomEvent::sendR
  // TODO(cleanup): It feels like there's something wrong with the design here. Can we make this
  //   less ugly?
  auto completionPaf = kj::newPromiseAndFulfiller<void>();
-  cap = capnp::membrane(
-      kj::mv(cap), kj::refcounted<CompletionMembrane>(kj::mv(completionPaf.fulfiller)));
+  cap = capnp::membrane(kj::mv(cap),


I don't love that "redirctPushAbortSignal" has appeared thrice in code not related to it. Maybe one of y'all has a suggestion on how to avoid this? Or maybe it's fine to leave the way it is?

npaun · 2026-11-02T02:52:28Z


  auto completionPaf = kj::newPromiseAndFulfiller<void>();
-  cap = capnp::membrane(
-      kj::mv(cap), kj::refcounted<CompletionMembrane>(kj::mv(completionPaf.fulfiller)));
+  cap = capnp::membrane(kj::mv(cap),


Same here...

Serializing an AbortSignal for JS RPC calls the callee's ExternalPusher.pushAbortSignal(), and the sender keeps the returned `trigger` so that it can forward a later abort. That call crossed the session's CompletionMembrane, which wrapped `trigger` like any other result. While the sender held it, the membrane saw a live capability, so the callee's request stayed open after the callee returned, and ended only when the caller's session was torn down. Its outcome was then "canceled", or "exception" when the hang detector fired first. CompletionMembrane and RevokerMembrane now accept a MembraneCallRedirector. JS RPC sessions (and restore sessions) pass redirectPushAbortSignal(), which redirects pushAbortSignal() calls, in either direction, to a forwarder. The forwarder makes the call, wraps the returned `signal` in the membrane, and returns `trigger` unwrapped. `signal` must stay wrapped: the sender passes it back inside a serialized value, and unwrapAbortSignal() recognizes it only once the membrane has unwrapped it again. The forwarder waits for the response instead of pipelining on it. Over RPC, a capability pipelined on a call keeps the callee's copy of the results, including the wrapped `signal`, alive while it is held. Exempting `trigger` from the client side's RevokerMembrane keeps it usable after the session ends, rather than revoked. An abort sent after the receiving request has ended is safe: AbortTriggerRpcServer writes the reason into the atomically refcounted PendingAbortReasonBox, which later requests can poll, and fulfills a fulfiller whose promise may already be gone. Byte streams, stream cancelers and delayed channel tokens still count toward session liveness. Fixes #8073 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

pushAbortSignal()'s forwarder wraps the returned `signal` in the session's membrane so that unwrapAbortSignal() can recognize it when the sender passes it back. The sender serializes `signal` from the call's pipeline, so it reaches the receiver as an unresolved promise. The receiver recognizes it, and releases that param, only after the sender's Resolve arrives, and the sender releases its own copy of the wrapped `signal` only after that. Cross-process, this exchange can finish after the call has returned and the sender's request has ended. The sender's session is then canceled while the receiver's CompletionMembrane still counts the wrapped `signal`, so a Durable Object that received a signal ended with outcome "canceled". The forwarder now wraps `signal` in a DetachedMembranePolicy, which CompletionMembrane and RevokerMembrane return from rootPolicy(). The membrane still unwraps `signal` when it crosses back, because the root policies match, so recognition works as before. But the wrapper holds no reference to the membrane's own policy, so it doesn't count toward completion and isn't revoked. The session's completion then no longer depends on when the sender releases `signal`. This keeps the protocol and the existing recognition path. Sending `signal` resolved would need the push to complete before the synchronous serialization returns. The forwarder no longer needs to avoid pipelining to keep `signal` from being held, so the comment explaining that is removed. The forwarder still waits for the response. The race needs the messages to cross a process boundary. A workerd test whose caller ends its request right after the call returns passed without this fix, so no workerd test is added. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

npaun · 2026-11-02T03:00:13Z

[Still needs a bit of review from me but I am marking it as ready so I can see if clanky has anything to say]

clanky-the-clanker

💬 Approved with comments — The membrane redirect is consistent and well tested. Only a suggestion about a hard-coded method ordinal and a note about a weak test assertion remain.

Findings: 1 info · 1 suggestion

_{The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.}

📋 Code Quality: 💬 1 info · 1 suggestion

The redirect logic is consistent. Two minor points: a hard-coded method ordinal and a weak reset test.

Recommendation: 💬 Optional Nits

[INFO] abortAfterActorReset only checks that a later call succeeds, not that the abort was delivered. scheduler.wait(0) doesn't guarantee the abort went out first, so the test only guards against crashes or hangs. (src/workerd/api/tests/abortsignal-test.js:1262)
[SUGGESTION] PUSH_ABORT_SIGNAL_METHOD_ID = 1 duplicates the schema ordinal with no compile-time link. If the ordinal changed, the redirect would silently stop matching. Derive it from the generated schema or assert it. (src/workerd/io/external-pusher.c++:305)

View full review

Hard-coded method ordinal (src/workerd/io/external-pusher.c++:305)

PUSH_ABORT_SIGNAL_METHOD_ID = 1 duplicates the ordinal of pushAbortSignal @1 in worker-interface.capnp.
If the ordinal ever changed, the redirect would silently stop matching and the old behaviour (a held trigger keeping the session open) would come back. Only the tail-outcome tests would catch it.
Consider deriving the ID from the generated schema (for example capnp::Schema::from<rpc::JsValue::ExternalPusher>().getMethodByName("pushAbortSignal")), or tie the constant to the schema with an assertion.

Reset test (src/workerd/api/tests/abortsignal-test.js:1262-1277)

abortAfterActorReset only checks that a later call to the actor returns 'ok'. It never checks that the abort was delivered or handled.
scheduler.wait(0) doesn't guarantee that the abort has gone out before the follow-up call.
As written, the test only guards against a crash or hang. Either say that in a comment, or assert on the outcome.

✅ All other checks passed

🔒 Security: ✅ LGTM

No security issues found. A trigger that outlives the session can only abort the signal it was created for.

📖 AGENTS.md: ✅ LGTM

No AGENTS.md updates are needed.

⚡ Performance: ✅ LGTM

No performance issues found.

📝 Documentation: ✅ LGTM

The update to docs/reference/detail/abort-signal.md matches the new behaviour.

_{Reviewed commit: 2f300862}

npaun force-pushed the npaun/abort-trigger-membrane branch from 9bf5dde to eb0d6b3 Compare November 2, 2026 02:45

npaun commented Nov 2, 2026

View reviewed changes

npaun and others added 2 commits November 2, 2026 02:54

npaun force-pushed the npaun/abort-trigger-membrane branch from eb0d6b3 to 2f30086 Compare November 2, 2026 02:58

npaun marked this pull request as ready for review November 2, 2026 03:00

npaun requested review from a team as code owners November 2, 2026 03:00

clanky-the-clanker Bot reviewed Nov 2, 2026

View reviewed changes

Navigation Menu

Repository navigation

Don't let AbortSignals passed over JS RPC keep the callee's request open - #8139

Don't let AbortSignals passed over JS RPC keep the callee's request open#8139
npaun wants to merge 2 commits into
mainfrom
npaun/abort-trigger-membrane

npaun commented Nov 2, 2026 •

edited

Loading

Uh oh!

npaun Nov 2, 2026

Uh oh!

npaun Nov 2, 2026

Uh oh!

npaun commented Nov 2, 2026

Uh oh!

clanky-the-clanker Bot left a comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

npaun commented Nov 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Possible follow-ups

Uh oh!

npaun Nov 2, 2026

Choose a reason for hiding this comment

Uh oh!

npaun Nov 2, 2026

Choose a reason for hiding this comment

Uh oh!

npaun commented Nov 2, 2026

Uh oh!

clanky-the-clanker Bot left a comment

Choose a reason for hiding this comment

📋 Code Quality: 💬 1 info · 1 suggestion

🔒 Security: ✅ LGTM

📖 AGENTS.md: ✅ LGTM

⚡ Performance: ✅ LGTM

📝 Documentation: ✅ LGTM

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

npaun commented Nov 2, 2026 •

edited

Loading