fix(cloudflare): Enforce flush timeout across Workflow lifecycle - #24483
matthewbjones wants to merge 6 commits into
Conversation
Cloudflare's flush deadline could expire while transport fetches remained active, Workflow steps could inherit the previous step's flush point and schedule redundant eager drains, and flush-lock finalization could wait outside the configured timeout. Abort requests when a transport drain expires, reset the flush point at each Workflow step boundary, and share one deadline across flush-lock, pending-span, and transport phases. Add focused regression coverage for all three paths and update timer-based tests to await the user task before asserting teardown. Fixes getsentry#24482 Co-authored-by: OpenAI Codex <codex@openai.com>
7ad0606 to
643d751
Compare
Share one deadline across the flush lock, pending spans, client processing, and transport drain so pre-drain timeouts cannot strand buffered envelopes. Add regression coverage for lock and span starvation and for the complete flush deadline. Co-authored-by: OpenAI Codex <codex@openai.com>
Use the Cloudflare test suite's established fake-timer cleanup and explicit timer advancement patterns for flush deadline assertions. Co-authored-by: OpenAI Codex <codex@openai.com>
|
Thanks a lot for opening a PR for this already. I'll take a close look today - as soon we will ship v11 where this should actually not be needed anymore. It would still be good to be fixed in v10 though. |
|
@JPeer264 thanks. Yes, would be great to have in v10. We've been seeing this in our production app for a few months now and finally we were able to identify where these warnings were coming from. I did my best to keep the changes as minimal as possible but still enforce the 2000ms timeout as intended. Each newly added test does exercise a bug and then confirms the refactored code addresses it. We also deployed a version of this Sentry SDK into production and none of our reproduction Workflows can cause the warning to emit anymore. |
|
Alright, I had a check over it and had couple of reproductions running for this. This seemed like a true issue also for the new So, I hope it is ok, to reduce review ping pong on this issue I added a change. I left the I also added an integration test that shows the successful aborted requests when the endpoint is slow (or rather not receiving anything in this case). I'll let bugbot run over it once and see what it says. Do you think this fix is sufficient? Since you already applied this PR to your code, would you apply the current state again and verify? I'm pretty certain it would work as well. |
|
bugbot run |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit fb63820. Configure here.
|
I'd let this sit over the weekend in case another reviewer wants to go over it. I will come back on it on monday |
|
@JPeer264 Thanks for taking a look. I think this retains one of the three fixes: aborting in-flight transport requests. That abort applies to both cache modes, but in our deployed reproduction the transport fix alone eliminated the warnings only with The shared overall flush deadline has also been removed, so Until v11 is released, is Sentry's recommendation for anyone using Cloudflare Workflows with SDK v10 to set |
Very interesting, I couldn't reproduce it. We could add it to make it work for you now - do you have a small reproduction where you reproduced this behavior? I'll add this on monday and do another test on this.
I think that is fine, unless I'm overlooking something critical here.
There is actually no |
|
Yes. The reproduction used the v11/develop SDK with the default class TestWorkflow extends WorkflowEntrypoint {
async run(_event, step) {
for (let i = 0; i < 100; i++) {
await step.do(`step-${i}`, async () => i);
}
}
}
export const Workflow = Sentry.instrumentWorkflowWithSentry(
() => ({
dsn: "https://diagnostic@<slow-worker>/delayed/1",
tracesSampleRate: 1,
}),
TestWorkflow,
);The DSN pointed to a Worker that consumed the request body and delayed every response for 120 seconds: export default {
async fetch(request) {
await request.arrayBuffer();
await new Promise(resolve => setTimeout(resolve, 120_000));
return new Response(null, { status: 204 });
},
};With only the transport-abort change, the default cached-client version continued producing Cloudflare I deployed the slow endpoint and Workflow Worker, then triggered 20 Workflow instances with 100 steps each: bunx wrangler deploy --config apps/waituntil-transport-sink/wrangler.jsonc
bunx wrangler deploy --config apps/waituntil-sentry-transport-control/wrangler.jsonc
for i in {1..20}; do
bunx wrangler workflows trigger condux-waituntil-sentry-delayed "{\"sequence\":${i},\"stepCount\":100}" \
--config apps/waituntil-sentry-transport-control/wrangler.jsonc
doneAfter allowing time for Cloudflare's events to appear, I searched that Worker's Observability events for the exact text As for v10 vs v11, originally I was patching the latest v10 release and eliminating the bugs there, but soon as I went to try and contribute this upstream I saw this project's guidelines says PRs are to be submitted again You can also read the conversation between myself and Cloudflare engineering in this Discord thread: https://discord.com/channels/595317990191398933/1550147114703786087 |

Cloudflare was reporting that Sentry-owned
waitUntil()tasks remained pending after instrumented Workflows had completed. The configuredflush(2000)timeout did not fully bound the work associated with a flush.Three separate lifecycle gaps contributed to the warning.
Transport requests survived a timed-out drain
IsolatedPromiseBuffer.drain(timeout)returnedfalsewhen its timer expired, but the request producers it had started continued running. A slow or stalledfetchtherefore remained attached to the invocation after Sentry reported that its flush had timed out.Each drain now owns an
AbortController. When that drain expires, it aborts only the requests started by that drain.makeCloudflareTransportcombines the drain signal with any caller-providedfetchOptions.signal, removes both listeners when the request settles, and keeps overlapping drains isolated so one timeout cannot abort another drain's requests.Workflow steps inherited the previous step's flush point
Cached Cloudflare clients reuse the Workflow run's isolation scope across step RPC invocations. The first boundary flush marked that scope's
flushPointReachedstate astrue, and later steps inherited it. Envelopes created by those later steps were consequently treated as post-invocation telemetry and registered additional eagerwaitUntil(transport.flush(2000))drains even though every step already performs its own boundary flush.The wrapped step callback now resets
flushPointReachedbefore it captures telemetry. This makes each Workflow step begin before its own flush point, while retaining eager delivery for telemetry that is genuinely created after that step's boundary flush.Flush-lock finalization was outside the timeout
CloudflareClient.flush(timeout)awaitedflushLock.finalize()before applyingtimeoutto pending spans and the transport. A userwaitUntiltask that never settled could therefore keep the Sentry flush pending indefinitely. The pending-span and transport phases could also each receive the original timeout instead of the time remaining from one overall deadline.flush()now computes one deadline and passes the remaining budget through flush-lock finalization, pending-span completion, and the transport drain. It returnsfalsewithout beginning a later phase when the budget has already been exhausted.Validation
Each failure has focused red/green regression coverage:
flush(timeout)to returnfalsewithin the deadline.The production reproduction uses 100 trivial
step.do()calls and an HTTP transport that delays every response for 120 seconds. The transport fix alone removed the warnings withcacheClient: false, but warnings remained on the current cached-client path. Disabling eager envelope delivery isolated the remaining behavior to the inherited Workflow flush point. With eager delivery enabled and both lifecycle fixes applied, 20/20 deployed Workflow instances completed with zero matchingwaitUntil()warnings in Cloudflare Observability.The complete
@sentry/cloudflaresuite passes (955/955), along with package lint, build, and typecheck.yarn lint) & (yarn test).Closes #24482