fix: agent tool status not stopping on abort#13111
Conversation
When clicking stop during agent session, tool status indicators continued showing active states because the UI reads from rawMcpToolResponse.status which was never updated on abort. Now sets it to 'cancelled' (abort) or 'error' (real error) for all in-progress tool blocks. Co-Authored-By: Claude Opus 4.6 <[email protected]>
When clicking stop during agent session, tool status indicators continued showing active states because: 1. The UI reads from rawMcpToolResponse.status which was never updated 2. Block updates were not persisted to IndexedDB Now updates rawMcpToolResponse.status to 'cancelled'/'error' for all in-progress tool blocks and persists the changes via saveUpdatesToDB. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Skip TOOL type blocks in the STREAMING branch since they are handled in the dedicated tool block branch below, avoiding a wasted dispatch that gets immediately overwritten. Co-Authored-By: Claude Opus 4.6 <[email protected]>
EurFelux
left a comment
EurFelux
left a comment
There was a problem hiding this comment.
LGTM! Clean and precise fix for the dual-status system desync on abort.
Summary:
- Root cause correctly identified: UI reads
rawMcpToolResponse.statusbut abort only updatedMessageBlockStatus - Tool blocks (typically in
PENDINGstate, notSTREAMING) were missed by the existing cleanup logic - Fix properly updates both status systems and persists to IndexedDB
The code is minimal, follows existing patterns, and handles edge cases well. One minor non-blocking suggestion left inline about using a positive set check for readability.
| if (block.type === MessageBlockType.TOOL) { | ||
| const toolBlock = block as ToolMessageBlock | ||
| const toolResponse = toolBlock.metadata?.rawMcpToolResponse | ||
| const toolStatus = toolResponse?.status | ||
| if ( | ||
| toolResponse && | ||
| toolStatus && | ||
| toolStatus !== 'done' && | ||
| toolStatus !== 'error' && | ||
| toolStatus !== 'cancelled' | ||
| ) { | ||
| dispatch( | ||
| updateOneBlock({ | ||
| id: block.id, | ||
| changes: { | ||
| status: isErrorTypeAbort ? MessageBlockStatus.PAUSED : MessageBlockStatus.ERROR, | ||
| metadata: { | ||
| ...toolBlock.metadata, | ||
| rawMcpToolResponse: { | ||
| ...toolResponse, | ||
| status: isErrorTypeAbort ? 'cancelled' : 'error' | ||
| } | ||
| } | ||
| } | ||
| }) | ||
| ) | ||
| updatedBlockIds.push(block.id) | ||
| } | ||
| } |
There was a problem hiding this comment.
Good fix! The dual-status system (MessageBlockStatus vs rawMcpToolResponse.status) mismatch was the root cause, and this correctly addresses it.
Nit (non-blocking): This branch intentionally does NOT skip possibleBlockId (unlike the STREAMING branch above at line 148), which means if possibleBlockId happens to be a TOOL block, its MessageBlockStatus gets set twice — once via smartBlockUpdate (line 123) and once here. This is harmless (same value both times) and actually desirable since only this branch updates rawMcpToolResponse.status. Just noting for clarity.
Minor suggestion: The negated status checks could be slightly more readable as a positive set check:
const IN_PROGRESS_TOOL_STATUSES = new Set(['pending', 'streaming', 'invoking'])
if (toolResponse && toolStatus && IN_PROGRESS_TOOL_STATUSES.has(toolStatus)) {This makes it self-documenting and easier to extend if new statuses are added. But the current form is also perfectly clear — up to you.
| const blocksToSave = updatedBlockIds | ||
| .map((id) => getState().messageBlocks.entities[id]) | ||
| .filter(Boolean) as MessageBlock[] | ||
| await saveUpdatesToDB(assistantMsgId, topicId, messageErrorUpdate, blocksToSave) |
There was a problem hiding this comment.
Nice approach — collecting updatedBlockIds then reading from updated state after synchronous dispatches is the right pattern here. This ensures the blocks passed to saveUpdatesToDB contain the final merged state (including the updated rawMcpToolResponse.status), rather than stale pre-dispatch snapshots.
Previously saveUpdatesToDB was called with [], meaning tool block status changes were lost on renderer refresh — this fixes the persistence gap.
zhibisora
left a comment
zhibisora
left a comment
There was a problem hiding this comment.
LGTM. I reviewed the abort cleanup fix and found no blocking issues. The change correctly updates tool UI state via rawMcpToolResponse.status, persists the updated blocks to IndexedDB, stays narrowly scoped, and aligns with the existing callback flow. CI is green and the fix addresses the reported refresh-persistence gap.
### What this PR does Before this PR: - Version is 1.7.24 After this PR: - Version is 1.7.25 - Release notes updated in `electron-builder.yml` ### Why we need it and why it was done in this way Standard release workflow: collect commits since v1.7.24, generate bilingual release notes, bump version. The following tradeoffs were made: N/A The following alternatives were considered: N/A ### Breaking changes **Action Required**: The built-in filesystem MCP server now requires manual approval for write/edit/delete operations by default. ### Special notes for your reviewer **Included commits since v1.7.24:** ✨ New Features: - feat(websearch): add Querit search provider (#13050) - feat(minapp,provider): add MiniMax Agent, IMA mini apps and MiniMax Global, Z.ai providers (#13099) - feat(provider): add agent support filter for provider list (#11932) - feat(agent): add custom environment variables to agent configuration (#13357) - feat: add GPT-5.4 support (#13293) - feat(gemini): add thought signature persistence (#13100) 🐛 Bug Fixes: - fix: secure built-in filesystem MCP root handling (#13294) - fix: check underlying tool permissions for hub invoke/exec (#13282) - fix: remove approval countdown timers and add system notifications (#13281) - fix: agent tool status not stopping on abort (#13111) - fix: resolve spawn ENOENT on Windows for Code Tools (#13405) - fix: Use default assistant for topic auto-renaming (#13387) - fix(backup): defer auto backup during streaming response (#13307) - fix(ui): fix Move To submenu overflow (#13399) - fix: render xml fenced svg blocks as svg previews (#13431) - fix: correct Gemini reasoning params (#13388) - fix: improve Qwen 3.5 reasoning model detection (#13235) - fix: upgrade xAI web search to Responses API (#12812) - fix(api-server): relax chat completion validation (#13279) - fix: install or uninstall button state issues (#13114) - fix: improve update dialog behavior (#13363) - fix: auto-convert reasoning_effort to reasoningEffort (#12831) ### Checklist - [x] PR: The PR description is expressive enough and will help future contributors - [x] Code: Write code that humans can understand and Keep it simple - [x] Refactor: You have left the code cleaner than you found it (Boy Scout Rule) - [x] Upgrade: Impact of this change on upgrade flows was considered and addressed if required - [x] Documentation: A user-guide update was considered and is present (link) or not required. - [x] Self-review: I have reviewed my own code before requesting review from others ### Release note ```release-note See release notes in electron-builder.yml ``` --------- Signed-off-by: kangfenmao <[email protected]>
5 participants
What this PR does
Before this PR:
When clicking the stop button during an agent session, tool status indicators (loading spinners, "Invoking"/"Streaming" labels) continued showing active states. The issue persisted even after refreshing the renderer process.
After this PR:
All in-progress tool blocks properly transition to cancelled/error state when the user clicks stop, and the state is persisted to IndexedDB so it survives renderer refreshes.
Why we need it and why it was done in this way
The UI for tool blocks reads status from
rawMcpToolResponse.status(e.g.,'pending','streaming','invoking'), not fromMessageBlockStatus. When abort fires,baseCallbacks.onErroronly updatedMessageBlockStatusfor blocks inSTREAMINGstate but never touchedrawMcpToolResponse.statusinside tool block metadata. Additionally, tool blocks typically haveMessageBlockStatus.PENDING(notSTREAMING), so they were missed entirely by the existing abort cleanup logic. The Redux updates were also not persisted to IndexedDB.The fix:
rawMcpToolResponse.statusto'cancelled'/'error'for all in-progress tool blocks on abortsaveUpdatesToDBfor IndexedDB persistenceThe following tradeoffs were made:
None significant — the fix is minimal and follows the existing pattern.
The following alternatives were considered:
baseCallbacks.onErroralready handles all abort cleanup centrally.Breaking changes
None.
Special notes for your reviewer
The dual status system (
MessageBlockStatusvsMCPToolResponseStatus) is the root cause. The UI components (ToolStatusIndicator,ToolBlockGroup,MessageAgentTools) all read fromrawMcpToolResponse.statusfor tool display, while the existing abort logic only updatedMessageBlockStatus.Checklist
Release note