fix(execution): stop a cancelled run reporting success when its wait swallows the cancellation - #6775
Conversation
…swallows the cancellation Cancellation reaches a running execution over Redis pub/sub, which is at-most-once. The engine turns that into `status: 'cancelled'` via `signalCancelled`. But the wait handler also polled the durable Redis cancellation key itself, and on a hit it broke out of its sleep and returned an ordinary successful block output. The engine's `cancelledFlag` stayed false, so a cancelled run finished as `success: true` — and with a block after the wait, kept executing. Whichever detector fired first won. The engine's pub/sub path normally wins by about one round trip; when the wait's own 500ms poll landed inside that window the cancellation was lost. Consolidate detection in the engine, which is the only component that can project run status: extend the once-at-start durable backstop into a poll that runs for the life of the run and routes through `signalCancelled`. The wait handler and loop orchestrator now observe only `ctx.abortSignal`, which the engine aborts, so no leaf can observe a cancellation the engine has not seen. The loop orchestrator additionally used to ignore `abortSignal.aborted` whenever Redis was enabled, so a mid-loop timeout or client disconnect was invisible to it, and it awaited a Redis round trip on every iteration. Handlers that abort their own I/O off `ctx.abortSignal` are unaffected: that surfaces as a throw, which the cancelled branch of `run` already classifies.
The Wait page claimed a 10-minute cap for a synchronous wait in three places. `MAX_INPROCESS_WAIT_MS`, the block description, the sub-block hint, and the validation error all say 5 minutes.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryHigh Risk Overview ExecutionEngine now owns all durable cancellation detection: the one-shot startup Redis backstop becomes a 500ms poll for the entire run, routed through The Wait handler drops its Redis polling and only sleeps on Docs for the Wait block update the in-line cap from 10 minutes to 5 minutes to match Reviewed by Cursor Bugbot for commit c3819ce. Configure here. |
Greptile SummaryThe PR centralizes durable cancellation detection in
Confidence Score: 5/5The PR appears safe to merge, with cancellation ownership consistently centralized in the execution engine and no actionable regression identified. The durable fallback remains active for the run lifecycle, routes cancellation through the engine’s status flag and abort signal, prevents overlapping polls, ignores reads completing after cleanup, and is covered by cancellation and timer-cleanup regressions.
|
| Filename | Overview |
|---|---|
| apps/sim/executor/execution/engine.ts | Adds engine-owned durable cancellation polling, routes detections through signalCancelled, and reliably stops polling during cancellation or cleanup. |
| apps/sim/executor/execution/engine.test.ts | Adds focused regression tests for durable-only cancellation and interval cleanup after execution settles. |
| apps/sim/executor/handlers/wait/wait-handler.ts | Removes leaf-level Redis polling so synchronous waits observe only the engine-owned abort signal. |
| apps/sim/executor/orchestrators/loop.ts | Replaces per-iteration Redis reads with the shared abort signal, fixing non-Redis abort handling and avoiding cancellation classification outside the engine. |
| apps/sim/lib/workflows/custom-blocks/child-execution.ts | Documents how parent-engine polling and parent-signal propagation provide durable cancellation coverage to child executions. |
| apps/docs/content/docs/en/workflows/blocks/wait.mdx | Corrects public documentation to state the synchronous Wait block’s five-minute limit. |
Sequence Diagram
sequenceDiagram
participant User as Cancellation request
participant Redis as Redis durable key
participant PubSub as Redis pub/sub
participant Engine as ExecutionEngine
participant Handler as Wait/Loop/Child handler
User->>Redis: Write cancellation key
User->>PubSub: Publish cancellation event
alt Event delivered
PubSub->>Engine: Cancellation event
else Event missed
loop Every 500 ms while run is active
Engine->>Redis: Read durable cancellation key
Redis-->>Engine: Cancelled
end
end
Engine->>Engine: signalCancelled()
Engine->>Handler: Abort ctx.abortSignal
Engine-->>User: success: false, status: cancelled
Reviews (1): Last reviewed commit: "docs(wait): correct the in-line wait cei..." | Re-trigger Greptile
Summary
status: 'cancelled'. But the wait handler also polled the durable Redis cancellation key itself, and on a hit returned an ordinary successful block output — leaving the engine'scancelledFlagfalse, so a cancelled run finished assuccess: true.cancelledFlag === falseand keeps executing, so a cancel silently doesn't cancel.ExecutionEngine, the only component that can project run status: the once-at-start durable backstop is now a poll that runs for the life of the run and routes throughsignalCancelled. The wait handler and loop orchestrator observe onlyctx.abortSignal, which the engine aborts — no leaf can observe a cancellation the engine hasn't seen.abortSignal.abortedwhenever Redis was enabled, so a mid-loop timeout or client disconnect was invisible to it. It also awaited a Redis round trip on every loop iteration, now a local boolean read.Why this shape
Redis pub/sub has no persistence, acks, or replay for a disconnected subscriber, so a correctness-critical signal carried over it needs a durable fallback read. The codebase already encodes that:
markExecutionCancelledwrites the durable key before publishing, and the engine already had a backstop — it just only ran once, at engine start. This completes that design rather than adding a new mechanism.status: 'cancelled'is not a new state. Every consumer across the sync route, async job, webhook, schedule, resume, column-execution, and child-workflow paths already branches on it — it's what the pub/sub path produces in the common case. This makes the lost-delivery case converge on the already-handled state.Handlers that abort their own I/O off
ctx.abortSignal(Mothership, Pi babysit) are unaffected: that surfaces as a throw, which the cancelled branch ofrunalready classifies. They were never part of this defect class.Type of Change
Testing
Tested manually against the failing scenario, plus:
success: false, status: 'cancelled'. Verified it goes red with the poll removed.executor/suite: 1984 passed. Cancellation-adjacent suites (child-execution, executor lib, run-control, schedule/resume/column execution, cancel-workflow-execution, cancellation, cancel route): 314 passed. Execute route: 119 passed.bun run type-checkclean,bun run lintclean,bun run check:audits29/29,docs:checkin sync.Checklist