Repository navigation
fix(surface): keep waitForSignal's transport out of the authored gate on Node 22 - #682
Merged
Merged
Conversation
… on Node 22 A hosted waitForSignal that saw one held 204 failed the run after the body finished: "derived handler for run-1 (f.run) rejected: The operation was aborted due to timeout", or unsettled_derived_work when the request deadline had not fired yet. Hosted sandboxes run the authored body on Node 22, whose bundled undici builds an unreachable body stream even for a 204: its internal promise stays pending, and the request deadline aborting after the response rejects it. The authored completion gate counts every promise created in the body's async context after an f.run as that step's derived work. Run each held request in the module's load context (an AsyncResource), so the transport's internal promises are not attributed to the author. The promise the body awaits is still created and tracked in the authored context and carries every request outcome. Each request's signal is also detached from the deadline and the caller's signal once it settles. No SDK gate rule changes; Node 24 and 26 never created the stream. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
[High impact] The PR appears safe to merge; the previous error-body cleanup issue is fixed.
|
… correct timeout docs Review on #682: - A non-2xx answer released the request before its body was read, so a server that sent error headers and stalled the body held the connection with no deadline (seen on real undici: the socket stayed open past the 2 s deadline; it now closes on refusal). The unread body is cancelled in the request context, while the deadline still reaches the request, and only then released. - The request context pins the AsyncLocalStorage stores active when the module is first evaluated. Documented as a known limit, with a test that pins it; a static import (the documented usage) captures none. - docs/SURFACE.md and the waitForSignal JSDoc recommended bounding the wait with AbortSignal.timeout. After an f.run that fails the run whether the abort is caught or not (and racing a timer leaves the wait in flight), so the docs now say signal stops a run and there is no supported time-out-and-continue yet. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A hosted
waitForSignalthat outlives one held 204 fails the run after the body has finished. Hosted run4e320b7a-98bc-4d40-b44d-6a7f4355d5dfwaited 11 minutes, resumed when the signal landed, ranrun-2to completion, and then failed with:The short case (
aa813b41, signal posted 7 s in) passed because it never saw a 204. Native's onboarding waits for a person, often for minutes, so long waits need to work.Root cause
deploy/e2b/e2b.Dockerfile:24,deploy/daytona/Dockerfile.lite:6,infra/microsandbox-runtime-bridge.ts:23:node:22-bookworm-slim).ReadableStreameven for a 204. The JSResponse.bodyisnull, so nobody can read or cancel that stream. One of its internal promises stays pending forever, and it rejects with the request signal's reason if that signal aborts after the response.holdSignalWaitpassedAbortSignal.timeout(30_000)straight tofetch. A held 204 returns after 25 s, and its deadline fires 5 s later.f.runas work derived from that step (packages/sdk/src/authored-promise-graph.ts:51). That includes undici's internals. A rejected one is reported asoperation_callback_failed(authored-flow-lifecycle.ts:510,authored-flow-operation.ts:241). A pending one is reported asunsettled_derived_work(authored-flow-lifecycle.ts:497,authored-flow-operation.ts:250).unsettled_derived_workif it had not yet fired. Both faces are reproduced below.Why Node 22 differs
A probe tracked the promises created inside a single
fetchthat answered 204. Node 22.14.0 and 22.22.2 leave one pending promise fromnew ReadableStreaminhttpNetworkFetch, whether or not the body is consumed or cancelled. Node 24.22.0 and 26.5.0 create none. The same flow, run on unmodified 2.0.54, fails on Node 22.22.2 and completes on 24.22.0 and 26.5.0.Fix (Surface only)
AsyncResourcecreated at module load and scoped toholdSignalWait's request loop. The transport's internal promises are therefore never attributed to the flow author.AbortSignal.timeoutthat fires later can no longer reach a finishedfetch.No change to
authored-promise-graphor to any SDK gate rule.Design reasoning
waitForSignal's internal HTTP promises are not that.Tests (Node 22.22.2, which CI uses)
New tests:
tests/signals.test.ts:runs each held request outside the caller async context: 3 × 204, then 200.detaches each settled request from its deadline and the caller signal.still aborts an in-flight request on its deadline or the caller signal.tests/authored-flow-lifecycle-executor.test.ts, three cases ofcompletes a signal wait after $rounds 204 round(s) with a $requestTimeoutMs ms request deadline. They use afetchstub that does what Node 22's undici does: a pending internal promise per 204 that rejects on a later abort.Mutation: 4 tests fail when I revert the AsyncResource. I replaced
requestContext.runInAsyncScope(() => fetch(...))with a plain call (the release step kept), rebuilt, and ran:I restored the file (
cmpagainst the saved copy: identical), rebuilt, and re-ran.Against the published 2.0.54 Surface (before any change), the 1-round, 5 ms case failed with the exact production message:
operation_callback_failed: flow "long-signal-wait" derived handler for run-1 (f.run) rejected: The operation was aborted due to timeout.Full runs on Node 22.22.2, with the SDK resolving the local Surface (
npm install ./packages/surface --prefix packages/sdk --no-save --ignore-scripts, as CI does):On the daemon-backed suites:
npm run test:prep(ops/cargo.sh, which uses a per-worktree target directory) and the SDKdistbuilt.CARGO_TARGET_DIR/RELAYFLOWD_BIN, withoutdist, and with aTMPDIRcontaining a space failed on those environment problems ("chain fixture daemon failed to start", "Cannot find module …/dist/cli.js", a%20path). The run above fixes the environment and nothing else.scripts/surface-package-gate.shdid not run locally.bun install --frozen-lockfilefailed on this machine with "failed copying files from cache to destination" for 9 packages, before any test. CI runs it.Real repro (Node 22.22.2, CLI built from this branch)
node packages/sdk/dist/cli.js runwith a flow that callswaitForSignalat the hosted defaults (waitMs25000,requestTimeoutMs30000). It talks to a local HTTPS mock of Cloud's signal GET.relayflows@2.0.54, Node 22.22.2, reproduces the production failure:Not in this PR
packages/sdk/src/cloud-http.ts:205(cloudFetch) uses the same deadline-on-fetchpattern. It is not changed here.Review follow-ups (a097d75)
cancels a stalled refusal body before detaching the request deadline. It fails under each of three reverts: no cancel, release before cancel, and cancel in the caller's context.als.run(...)(for example a dynamic import from a body), that store is pinned for every later request in the process.pins the AsyncLocalStorage store active when the module is first evaluated, which expects['run-A', undefined].docs/SURFACE.mdand thewaitForSignalJSDoc recommendedsignal: AbortSignal.timeout(ms)to bound a wait. After anf.run, that fails the run whether or not the abort is caught. Racing the wait against a timer fails too. Checked with throwaway gate-level probes, not committed:signalstops a run that has waited too long, and that there is no supported way yet to time a wait out and continue. This is a pre-existing gate behaviour, not changed here.npm run check:docsinpackages/surfacereportsAUTHORING.mddrift both at this head and at 5eee931. The drift is thesecrets?: string[]options, which predate this PR; I did not regenerate it here.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need.Agent Relay sessions
claudesessiona3247048· opened viagh pr create· last active 2026-10-21