fix(execution): keep terminal reconnect off runs a Sim run tool owns

Merged
IC
2mo ago
+596-64

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}/stream returned 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) sets currentExecutionId and 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 and executeWorkflowWithFullLogging write it on the X-Execution-Id header.
  • Starting the run adds and activates the workflow tab in Chat, which mounts useWorkflowExecution (embedded workflow.tsx and EmbeddedWorkflowActions), and setActiveWorkflow flips activeWorkflowId after the workflow fetch. Both re-run the hook's reconnect effect mid-run.
  • The effect's claim check only knows "pointer present and the current execution id matches", which is exactly the state a live run tool creates. cancelExecute is a no-op for the tool's raw fetch, so the effect opened a reconnect GET in parallel with the live POST.
  • That GET races the POST's 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, sets isExecuting false, 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 gets executionOrder: 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.
  • When the tool gives up an interrupted run it notifies subscribeToRunToolRelease subscribers after releasing ownership; the hook subscribes and bumps reconnectAttemptNonce, 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 for bindRunToolToExecution to re-report after a reload.

2. One stream-interruption classifier for every live stream (commit 2)

  • Only useExecutionStream.execute wrapped a transport failure as SSEStreamInterruptedError; executeWorkflowWithFullLogging rethrew the raw TypeError. So on the Chat path a mid-run network drop took the generic branch: the tool reported error, 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).
  • toStreamInterruptedError is 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: reports background (truthful, the server continues), keeps the pointer, and releases the run to the terminal reconnect via change 1.
  • Review rounds hardened the classifier in place: it matches the browsers' transport messages as patterns (so Firefox's 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)

  • The async path wrote the 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. Async launches now write only that, 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 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

  • Hook: the 404 path on an unowned pointer still logs the Run Error (proves the guard test is not vacuous); an owned pointer skips reconnect, logs nothing, and keeps store and pointer intact; a release for the active workflow reconnects from the pointer's lastEventId, one for another workflow does not, and unmount unsubscribes.
  • Run tool: ownership is set before the pointer write and held exactly while the run is in flight; both interruption kinds release only after ownership and store teardown, keep the pointer, and send a background confirm; 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.
  • Shared executor: a transport drop after the server acknowledged the run rejects with SSEStreamInterruptedError carrying the server execution id; an AbortError, the run tool's string abort reason, and a non-transport error are rethrown untouched.
  • Mutation-tested: removing the guard, the nonce bump, the notify, the streamInterrupted condition, 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 test on the four files plus chat.test.tsx, bun run type-check (all packages), biome on every changed path, and bun run check:audits (44/44) pass.

Accepted notes from review

  • A server-initiated stream teardown (buffer write failure, which aborts the run and errors the stream) now reads as a transport drop on the Chat path and is reported as background; the same classification the manual path already makes, and the confirm route resolves against the trusted execution on any later confirm.
  • A tab running the previous client with a stuck async pointer that reloads onto this build can log the synthetic Run Error once before bindRunToolToExecution cleans the pointer up, exactly as the previous client did to itself.

🤖 Generated with Claude Code