Ignore exec AbortSignal when RPC flag is unset#775
Conversation
Passing an AbortSignal to exec() threw a DataCloneError because the signal cannot cross the Worker to Durable Object boundary without the enable_abortsignal_rpc compatibility flag. Detect the unserializable signal before the RPC call, drop it so the command still runs, and warn once explaining how to enable cancellation.
🦋 Changeset detectedLatest commit: 90caf8b 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: |
📦 Preview BuildVersion: Install the SDK preview:
|
aron-cf
requested review from
ghostwriternr,
scuffi and
whoiskatrin
as code owners
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| /** | ||
| * Returns `execOptions` with any `signal` removed when the | ||
| * `enable_abortsignal_rpc` compatibility flag is not set, warning the user | ||
| * once. This keeps `exec()` from throwing a `DataCloneError` across the RPC | ||
| * boundary (issue #764): the command still runs, but cancellation via the | ||
| * signal becomes a no-op until the flag is enabled. | ||
| */ |
There was a problem hiding this comment.
🟡 JSDoc comment references historical context ("issue #764") violating coding standards
The JSDoc for stripUnsupportedSignal at line 658 includes (issue #764), which violates the repository's coding standards defined in .agents/skills/coding-standards/SKILL.md. The rule explicitly states: "Don't reference historical context" and gives the example "❌ Bad: references a bug the reader knows nothing about". A future reader won't have context about what issue #764 was without navigating to GitHub.
| /** | |
| * Returns `execOptions` with any `signal` removed when the | |
| * `enable_abortsignal_rpc` compatibility flag is not set, warning the user | |
| * once. This keeps `exec()` from throwing a `DataCloneError` across the RPC | |
| * boundary (issue #764): the command still runs, but cancellation via the | |
| * signal becomes a no-op until the flag is enabled. | |
| */ | |
| /** | |
| * Returns `execOptions` with any `signal` removed when the | |
| * `enable_abortsignal_rpc` compatibility flag is not set, warning the user | |
| * once. This keeps `exec()` from throwing a `DataCloneError` across the RPC | |
| * boundary: the command still runs, but cancellation via the signal becomes | |
| * a no-op until the flag is enabled. | |
| */ |
Was this helpful? React with 👍 or 👎 to provide feedback.
| logger.warn( | ||
| 'An AbortSignal was passed to exec(), but AbortSignal cannot cross the ' + | ||
| 'Worker -> Durable Object boundary unless the `enable_abortsignal_rpc` ' + | ||
| 'compatibility flag is enabled. The signal is being ignored so the ' + | ||
| 'command can run. Add "enable_abortsignal_rpc" to compatibility_flags ' + | ||
| 'in your Wrangler config to enable cancellation.' | ||
| ); |
There was a problem hiding this comment.
🟡 Warning message hardcodes "exec()" but is also triggered from execStream()
The stripUnsupportedSignal function is called by both the exec handler (line 743) and the execStream handler (line 768), but the warning message at line 672 always says "An AbortSignal was passed to exec()...". When triggered via execStream(), this is misleading. Since warnedAboutAbortSignalRPC ensures the warning fires only once, if execStream triggers it first, the user sees an inaccurate reference to exec().
| logger.warn( | |
| 'An AbortSignal was passed to exec(), but AbortSignal cannot cross the ' + | |
| 'Worker -> Durable Object boundary unless the `enable_abortsignal_rpc` ' + | |
| 'compatibility flag is enabled. The signal is being ignored so the ' + | |
| 'command can run. Add "enable_abortsignal_rpc" to compatibility_flags ' + | |
| 'in your Wrangler config to enable cancellation.' | |
| ); | |
| logger.warn( | |
| 'An AbortSignal was passed to a command method, but AbortSignal cannot cross the ' + | |
| 'Worker -> Durable Object boundary unless the `enable_abortsignal_rpc` ' + | |
| 'compatibility flag is enabled. The signal is being ignored so the ' + | |
| 'command can run. Add "enable_abortsignal_rpc" to compatibility_flags ' + | |
| 'in your Wrangler config to enable cancellation.' | |
| ); |
Was this helpful? React with 👍 or 👎 to provide feedback.
| execStream: (command: string, streamOptions?: StreamOptions) => { | ||
| if (useDefaultSession || streamOptions?.sessionId !== undefined) { | ||
| return stub.execStream(command, streamOptions); | ||
| const safeOptions = stripUnsupportedSignal(streamOptions, clientLogger); | ||
| if (useDefaultSession || safeOptions?.sessionId !== undefined) { | ||
| return stub.execStream(command, safeOptions); | ||
| } | ||
|
|
||
| return stub.execStreamWithSessionToken( | ||
| command, | ||
| DISABLE_SESSION_TOKEN, | ||
| streamOptions | ||
| safeOptions | ||
| ); |
There was a problem hiding this comment.
🟡 streamProcessLogs not protected by stripUnsupportedSignal, same DataCloneError possible
The streamProcessLogs method accepts options?: { signal?: AbortSignal } (packages/shared/src/types.ts:1376) and is NOT listed in enhancedMethods, so calls fall through the proxy directly to the raw stub (packages/sandbox/src/sandbox.ts:899). If a user calls sandbox.streamProcessLogs(id, { signal }) without enable_abortsignal_rpc, the signal will cross the RPC boundary unstripped and throw the same DataCloneError that this PR fixes for exec() and execStream(). The fix pattern should be extended to this method as well.
Prompt for agents
The streamProcessLogs method (defined at packages/sandbox/src/sandbox.ts:4701) accepts { signal?: AbortSignal } and is exposed through the ISandbox interface (packages/shared/src/types.ts:1374-1377). However, it is not in the enhancedMethods object in getSandbox(), so calls to it go directly through the proxy fallthrough to the raw stub (line 899). This means the AbortSignal will be serialized across the RPC boundary without being stripped.
To fix: add streamProcessLogs to the enhancedMethods object in getSandbox() (around line 740-888) with stripUnsupportedSignal applied to its options, similar to how exec and execStream are handled. Something like:
streamProcessLogs: (processId: string, options?: { signal?: AbortSignal }) => {
const safeOptions = stripUnsupportedSignal(options, clientLogger);
return stub.streamProcessLogs(processId, safeOptions);
}
Was this helpful? React with 👍 or 👎 to provide feedback.
| onUnhandledError(error) { | ||
| const isAbortSignalCloneError = | ||
| error.name === 'DataCloneError' && | ||
| /AbortSignal serialization is not enabled/i.test(error.message ?? ''); | ||
| if (isAbortSignalCloneError) return false; |
There was a problem hiding this comment.
🚩 vitest onUnhandledError handler may not conform to Vitest's expected return contract
The onUnhandledError at line 37-41 returns false for DataCloneErrors to suppress them, but implicitly returns undefined for all other errors. In Vitest's API, the behavior of onUnhandledError with different return values may vary by version. If Vitest expects an explicit return or treats undefined differently from no return, other unhandled errors might be swallowed. This is in test configuration only so has no production impact, but worth verifying against the Vitest version used.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Fixes #764
Calling
exec()with an abort signal threw before the command ever ran:The options object is structured-cloned when it crosses from the Worker into
the Durable Object. An
AbortSignalis only cloneable when theenable_abortsignal_rpccompatibility flag is set, so without that flag thecall failed instead of either honoring the signal or ignoring it. This is
issue #764.
The fix
Before the call crosses the boundary, the software development kit now checks
whether an abort signal can actually be serialized. It does this with a local
structuredCloneprobe rather than letting the real call fail, because afailed call also leaves an unhandled rejection inside the runtime's remote
procedure call machinery that the caller cannot reach.
When the flag is missing, the signal is dropped so the command still runs, and
a single warning explains how to turn cancellation on. When the flag is set,
the signal is passed through and honored exactly as before. The same guard is
applied to
execStream(), which accepts a signal through the same optionsshape. The warning fires once per isolate, since the flag cannot change while
the isolate is alive.
How to verify
With the flag enabled, cancellation works as expected:
Without the flag, the previously broken snippet from the issue now completes
instead of throwing, and the logs contain a one-time warning pointing at the
flag.
Testing
Compatibility flags are fixed for the lifetime of an isolate, so a single test
run cannot toggle the flag on and off. To cover both states, the regression
test runs under two test projects in the same suite: one boots the pool without
enable_abortsignal_rpcand one boots it with the flag. A binding tells eachrun which state it is in so the same assertions describe both directions.
The test confirms three things. The raw, unguarded boundary still throws when
the flag is off, which documents the underlying issue. Going through
getSandbox().exec()never surfaces the clone error to the caller. And thewarning about the flag is emitted only when the flag is off.
A deliberate side effect of the flag-off case is an internal rejected promise
created by the runtime that no test code can await. The suite drops only that
exact error through the
onUnhandledErrorhook, so every other unhandlederror across the whole suite still fails the run.