fix(terminal): correct error/cancel block status in logs panel#4372
fix(terminal): correct error/cancel block status in logs panel#4372waleedlatif1 merged 3 commits intostagingfrom
Conversation
Three bugs in the workflow editor's terminal/logs panel where block status diverged from the engine's truth on error paths: 1. **Errored block shown as "canceled"** — when the SSE 'add' mode produced a duplicate entry on block error and `cancelRunningEntries` then swept the original placeholder. 2. **Upstream blocks stuck on "Running"** — terminal events arrived before the engine's last block events under reconnect/timeout, so the live panel never received the per-block terminal state. 3. **Phantom "Run Error" pseudo-row** — the failing block rendered as "canceled" while a synthetic row carried the real error text. Fixes: - **Fix B** (`addConsoleErrorEntry`): when a running placeholder exists for `(blockId, executionId)`, route through `updateConsoleErrorEntry` instead of creating a second entry. Aligns 'add' mode with the existing 'update' mode behavior. - **Fix C** (`reconcileFinalBlockLogs`): terminal SSE events now carry `finalBlockLogs` (server-authoritative snapshot). On execution:error / execution:cancelled, reconcile any still-running entries with their server-side terminal state. Recovers correctness on network drop, server timeout/abort, and reconnect-resume paths where individual block:* events may not have reached the client. - **Fix D** (`addExecutionErrorConsoleEntry`): cross-check `useTerminalConsoleStore` for entries with `error` set scoped to the executionId before emitting the synthetic "Run Error" row. Suppresses the phantom row when the failing block already carries the message. - **Signature refactor**: `handleExecutionErrorConsole` / `handleExecutionCancelledConsole` now take a typed `ExecutionConsoleDeps` object instead of stacking positional deps — matches the existing `createBlockEventHandlers(config, deps)` precedent in the same file. Tests: - 12 tests in `workflow-execution-utils.test.ts` covering Fix B/C/D and the deps-object signature refactor. - Centralized terminal-console store mock in `@sim/testing` so future tests can stub the store without per-file boilerplate. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview Also prevents duplicate/phantom error rows: Reviewed by Cursor Bugbot for commit ddcab29. Configure here. |
Greptile SummaryThis PR fixes three terminal/logs panel bugs: errored blocks showing as "canceled" (Fix B), upstream blocks stuck on "Running" after a run ends (Fix C), and a phantom "Run Error" row when the failing block already carries the error inline (Fix D). The architectural refactor of Confidence Score: 5/5Safe to merge — no P0/P1 findings; all three bug fixes are surgical and well-covered by tests. The changes are focused, the test suite covers the happy path and the three specific failure modes, and the architectural cleanup is consistent with existing patterns. No critical logic errors or security concerns found. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant Server as Server (route.ts)
participant SSE as SSE Stream
participant Hook as useWorkflowExecution
participant Utils as workflow-execution-utils
participant Store as useTerminalConsoleStore
Server->>SSE: execution:error / execution:cancelled (+ finalBlockLogs)
SSE->>Hook: onExecutionError(data) / onExecutionCancelled(data)
Hook->>Utils: handleExecutionErrorConsole(deps, params{finalBlockLogs})
Note over Utils: Fix C — reconcile before sweep
Utils->>Store: getWorkflowEntries(workflowId)
Store-->>Utils: entries[]
loop for each log in finalBlockLogs
Utils->>Utils: find running entry by blockId + executionId
Utils->>Store: updateConsole(blockId, {isRunning:false, success, error?}, executionId)
end
Note over Utils: sweep any still-running entries
Utils->>Store: cancelRunningEntries(workflowId)
Note over Utils: Fix D — skip phantom row if block already has error
Utils->>Store: getWorkflowEntries(workflowId)
Store-->>Utils: entries[]
Utils->>Utils: hasBlockErrorInConsole? (executionId scoped, blockType!=error)
alt no block-level error found
Utils->>Store: addConsole(Run Error row)
end
Note over Store: Fix B (block:error SSE path)
SSE->>Hook: block:error event
Hook->>Utils: addConsoleErrorEntry(data)
Utils->>Store: getWorkflowEntries — find isRunning entry for (blockId, executionId)
alt running placeholder exists
Utils->>Store: updateConsoleErrorEntry(data) — reuse, no duplicate
else no running entry
Utils->>Store: addConsole(new error entry)
end
Reviews (3): Last reviewed commit: "fix(terminal): pass durationMs in reconn..." | Re-trigger Greptile |
…tion
Address Greptile review:
- `executeWorkflowWithFullLogging`'s `onExecutionCancelled` was `() => {}`
and silently dropped the `finalBlockLogs` payload, so Bug 2's "upstream
blocks stuck on Running" fix did not fire on copilot-initiated
cancellations. Wire it through `handleExecutionCancelledConsole` to
match the SSE-route `onExecutionCancelled` path.
- Test for the `blockType !== 'error'` filter used a different
`executionId` than the seeded entry, so the executionId scope rejected
the entry before the blockType predicate ran. Align executionIds so
the test actually exercises the filter.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@greptile-apps re: "Copilot cancellation path not wired to Fix C" — fixed in dc6e3f3. |
|
Confirmed — the fix in dc6e3f3 is correct and complete.
The test fix (aligning Both changes look good. The original gap is closed. |
|
@greptile |
|
@cursor review |
Reconnect-resume `onExecutionCancelled` was forwarding `finalBlockLogs` but not `data?.duration`, so the "Run Cancelled" entry rendered with a 0ms duration. Match the other two `handleExecutionCancelledConsole` callsites. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@greptile |
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit ddcab29. Configure here.
Summary
Three bugs in the workflow editor's terminal/logs panel where block status diverged from the engine's truth on error paths.
Bugs
erroredge that itself throws ended upcanceledinstead oferrored.Start/KnowledgeBasekept showing "Running" until a hard refresh.Fixes
addConsoleErrorEntry, bug 1): when a running placeholder exists for(blockId, executionId), route throughupdateConsoleErrorEntryinstead of creating a duplicate entry. Aligns SSEaddmode withupdatemode for the same key. The duplicate is whatcancelRunningEntrieswas sweeping into a "canceled" state.reconcileFinalBlockLogs, bug 2): terminal SSE events (execution:error/execution:cancelled) now carryfinalBlockLogs— the server-authoritative snapshot. On the client, any still-running entry is reconciled against that snapshot. Recovers correctness on network drop, server timeout/abort, and reconnect-resume paths where individualblock:*events may not have reached the client (Redis 1-hour replay buffer can't recover events that were never written there).addExecutionErrorConsoleEntry, bug 3): cross-checkuseTerminalConsoleStorefor entries witherrorset scoped to the executionId before emitting the synthetic "Run Error" row. Suppresses the phantom row when the failing block already carries the message inline.Architectural cleanup
handleExecutionErrorConsole/handleExecutionCancelledConsolenow take a typedExecutionConsoleDepsobject instead of stacking positional deps. Matches the existingcreateBlockEventHandlers(config, deps)precedent in the same file.Why not Fix A / Fix E (considered, dropped)
Earlier drafts included an "error-aware
cancelRunningEntries" (Fix A) and a promise-tracker drain at the SSE terminal boundary (Fix E). Reverse-tracing the production code paths showed Fix A was guarding a state ({ isRunning: true, error: <set> }) that doesn't naturally arise —onBlockStartedis the only writer ofisRunning: trueand it setserror: undefined. Fix E targeted a race that doesn't exist in the inline route callbacks (they'reasyncwith no internalawait, socontroller.enqueue(block:*)always fires before the terminalenqueueregardless ofvoiddiscarding). Both reverted to keep the change set surgical.Test plan
workflow-execution-utils.test.tscover Fix B/C/D and the deps-object refactor (placeholder reuse,finalBlockLogsreconciliation no-op on happy path, phantom-row suppression when block carries error inline, etc.).cancelRunningEntriesbehavior unchanged.terminal-console.mock.tsin@sim/testingso future tests can stub the store without boilerplate.Start → Function2 (syntax error) -[error]→ Function1 (syntax error)— both blocks rendererrored(red), no "Cancelled".Start → KnowledgeBase (empty KB) → Function (throws)—StartandKnowledgeBasereach a terminal state without refresh.Functionblock fetching HTML andJSON.parse-ing — failing block shows the JSON-parse error inline; no "Run Error" row unless it's a true pre-execution validation failure.