fix(execution): keep terminal reconnect off runs a Sim run tool owns - #7382
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Greptile SummaryThe PR coordinates Chat run-tool ownership with terminal reconnection, standardizes transport-interruption classification, and avoids creating terminal pointers for asynchronous launches.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/lib/copilot/tools/client/run-tool-execution.ts | Adds workflow-scoped run ownership and release notifications while separating asynchronous completion recovery from live execution pointers. |
| apps/sim/app/workspace/[workspaceId]/w/[workflowId]/hooks/use-workflow-execution.ts | Defers terminal reconnection to an active run tool and retries when an interrupted run is released. |
| apps/sim/hooks/use-execution-stream.ts | Extracts a shared classifier for browser transport failures that interrupt acknowledged execution streams. |
| apps/sim/app/workspace/[workspaceId]/w/[workflowId]/utils/workflow-execution-utils.ts | Preserves acknowledged executions for recovery when the shared executor encounters a classified stream interruption. |
| apps/sim/lib/copilot/tools/client/run-tool-execution.test.ts | Replaces the previously reported untyped callback parameters and verifies abort-signal, ownership, release, and asynchronous recovery behavior. |
Reviews (5): Last reviewed commit: "fix(execution): never classify the strea..." | Re-trigger Greptile
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Heads up: you’re close to your included review allowance. Set a flex budget so reviews don’t pause.
Fix all with cubic | Re-trigger cubic
The terminal's reconnect effect treated "execution pointer present and the current execution id matches" as an orphaned run and claimed it. A Chat run tool creates exactly that state before the server has acknowledged the run, and opening the workflow tab in Chat re-runs the effect mid-run, so the reconnect GET raced the execute POST's buffer init, got a 404, and logged "Execution state is no longer available after reconnect" as a Run Error on a run that succeeded. It also tore down the live run's store state and cleared the pointer the tool keeps for reload recovery. The run tool now exposes its ownership (isRunToolActiveForWorkflow) and the reconnect effect skips a workflow whose run it owns, leaving the pointer in place. When the tool gives up an interrupted run it notifies subscribeToRunToolRelease subscribers and the hook re-arms its reconnect, so the terminal re-attaches from the last persisted event the way the manual run path already does on interruption. Runs the tool observed to completion never notify, so a failed completion report still leaves the pointer for bindRunToolToExecution to re-report after a reload. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…iting a terminal pointer Only useExecutionStream.execute wrapped a transport failure as SSEStreamInterruptedError; executeWorkflowWithFullLogging rethrew the raw TypeError, so a mid-run network drop on the Chat run-tool path took the generic branch: the tool reported "error" to Sim, the confirm route marked the row failed, and the pointer was cleared while the server kept running the workflow. The classifier is now one exported helper (toStreamInterruptedError) used by both execute paths and by the shared executor's post-acknowledgement catch, so the run tool reaches its recoverable branch, reports "background", keeps the pointer, and releases the run to the terminal reconnect. Async launches wrote the terminal execution pointer only so bindRunToolToExecution would find something after a reload, but an async run has no reconnectable stream, so any reconnect against that pointer 404'd into the same synthetic Run Error. The tab-local pending completion report already carries the execution id, so async launches no longer touch the pointer and recovery answers from the pending report first, falling back to the pointer only for a live run this tab was observing. The legacy clearExecutionPointerAfterReport flag is still honoured for pointers older clients left behind. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…s contract Greptile flagged the new mock's `options: any`; the sibling abort test had the same shape. Export WorkflowExecutionOptions from the shared executor and use it in both, with a helper that fails the test if the run tool ever stops passing an abort signal. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
029a959 to
f431355
Compare
|
@cubic-dev-ai review this PR |
@icecrasher321 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 7 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
The transport-failure matcher only knew Chrome's "network error" with a space, so Firefox's "NetworkError when attempting to fetch resource." fell through as a plain failure. Now that every live stream shares this classifier, match the browsers' known messages as patterns and cover each form in the executor test. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
@cubic-dev-ai review this PR |
@icecrasher321 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 7 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
…ections isClientDisconnectError read error.name unguarded, so a stream that rejected with null or undefined would throw inside the catch and mask the original failure. Both predicates now take unknown and bail on non-object values; the executor test covers a nullish body-reader rejection. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
@cubic-dev-ai review this PR |
@icecrasher321 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 7 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
…port drops
An ExecutionStreamHttpError or SSEEventHandlerError whose message happened to
contain a browser transport phrase ("Failed to fetch workflow state") would
have been re-wrapped as a stream interruption, losing the HTTP status and
taking the recovery path for a run that never started. The predicate now
excludes the stream layer's typed errors before looking at message text.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
@cubic-dev-ai review this PR |
@icecrasher321 I have started the AI code review. It will take a few minutes to complete. |
Summary
Running a workflow from Chat on staging produced a "Run Error: Execution state is no longer available after reconnect" entry (0ms, last in the group) on a run whose blocks all succeeded. The entry is synthetic: the terminal's reconnect effect wrote it after a reconnect
GET .../executions/{id}/streamreturned 404. Tracing it end to end surfaced three gaps in how the Chat run tool and the terminal reconnect flow share a run, and this PR closes all three at the level they live.Root cause of the reported error
doExecuteRunTool(apps/sim/lib/copilot/tools/client/run-tool-execution.ts) setscurrentExecutionIdand writes the sessionStorage execution pointer before sending the execute POST. It is the only run path that writes the pointer before the server has created the run buffer; the manual path andexecuteWorkflowWithFullLoggingwrite it on theX-Execution-Idheader.useWorkflowExecution(embeddedworkflow.tsxandEmbeddedWorkflowActions), andsetActiveWorkflowflipsactiveWorkflowIdafter the workflow fetch. Both re-run the hook's reconnect effect mid-run.cancelExecuteis a no-op for the tool's raw fetch, so the effect opened a reconnect GET in parallel with the live POST.initializeExecutionStreamMeta; when it reads first, the stream route answers 404, which the effect treats as non-retryable: it logs the Run Error with the live run's execution id, setsisExecutingfalse, nulls the current execution id, and clears the pointer the tool keeps for reload recovery. The POST headers then restore the id and the blocks stream in normally, which is why the run still looked successful. The error entry getsexecutionOrder: MAX_SAFE_INTEGER, so it always sorts last.Changes
1. The reconnect effect defers to a live run tool, and the tool hands an interrupted run back (commit 1)
isRunToolActiveForWorkflow(workflowId)exposes the run tool's existing per-workflow ownership map. The reconnect effect returns early while it is set, before loading the pointer and without clearing it.subscribeToRunToolReleasesubscribers after releasing ownership; the hook subscribes and bumpsreconnectAttemptNonce, so the terminal re-attaches from the last persisted event, matching what the manual path already does on interruption. Runs the tool observed to completion never notify, so a failed completion report still leaves the pointer forbindRunToolToExecutionto re-report after a reload.2. One stream-interruption classifier for every live stream (commit 2)
useExecutionStream.executewrapped a transport failure asSSEStreamInterruptedError;executeWorkflowWithFullLoggingrethrew the rawTypeError. So on the Chat path a mid-run network drop took the generic branch: the tool reportederror, the confirm route marked the row failed, and the pointer was cleared while the execute route kept running the workflow (the streaming branch never aborts on client disconnect).toStreamInterruptedErroris now the single exported classifier, used by both hook execute paths (pure extraction, no behaviour change) and by the shared executor's post-acknowledgement catch. The run tool therefore reaches its recoverable branch: reportsbackground(truthful, the server continues), keeps the pointer, and releases the run to the terminal reconnect via change 1.NetworkError when attempting to fetch resource.counts), tolerates nullish or non-object rejections, and never classifies the stream layer's own typed errors (ExecutionStreamHttpError,SSEEventHandlerError) by their message text.3. Async launches no longer write the terminal pointer (commit 2)
bindRunToolToExecutionwould find something after a reload, but an async run has no reconnectable stream, so any reconnect against that pointer 404'd into the same synthetic Run Error.clearExecutionPointerAfterReportflag is still honoured so pointers older clients left behind get cleaned up.Traced end to end across both commits: manual runs and run tools cannot overlap on a workflow, reload recovery is unchanged for live runs, async runs were never stoppable from the terminal so nothing loses the pointer, no import cycle, nothing new in the editor bundle. Three independent adversarial reviews over the diffs; their findings that survived are the two accepted notes below.
Tests
lastEventId, one for another workflow does not, and unmount unsubscribes.backgroundconfirm; a completed run whose report fails does not release; async launches never touch the pointer and recover from the pending report alone; a legacy async pointer is cleaned up after its pending report is re-sent.SSEStreamInterruptedErrorcarrying the server execution id; an AbortError, the run tool's string abort reason, and a non-transport error are rethrown untouched.streamInterruptedcondition, the ownership write, the executor wrap, the classifier, the pointer-free async launch, or the pending-first recovery each fails exactly the test written for it.bun run teston the four files pluschat.test.tsx,bun run type-check(all packages), biome on every changed path, andbun run check:audits(44/44) pass.Accepted notes from review
background; the same classification the manual path already makes, and the confirm route resolves against the trusted execution on any later confirm.bindRunToolToExecutioncleans the pointer up, exactly as the previous client did to itself.🤖 Generated with Claude Code