fix(session): implement proper cancel/stop flow with force-kill subprocess#163
Merged
Conversation
…ocess ## Problem When user clicked stop, two things failed: 1. Agent kept running — `query.interrupt()` sends graceful message but doesn't guarantee stop 2. No visual feedback — cancelled message only persisted in catch block, which never ran on interrupt ## Solution Replace `query.interrupt()` (graceful) with `query.close()` (force-kill subprocess + children): - Terminates entire process tree, no need to track individual sub-agent tasks - Synchronous operation, immediate subprocess death Add `cancelledByUser` flag to SessionState: - Set by `handleCancel` before close - Checked in post-loop path to save cancelled message + send Tauri event - Checked in catch block for early return (avoid duplicate error handling) ## Changes **Sidecar (agent cancellation):** - `handleCancel`: Use `close()` instead of `interrupt()`, set `cancelledByUser=true` - Post-loop: When `cancelledByUser`, save cancelled message to DB with `stop_reason: "cancelled"` envelope format - Catch block: Return early if `cancelledByUser` to avoid error handling path - Add TIMING instrumentation throughout for observability **Backend (cleanup dead code):** - Remove `cancelled_at` column updates from POST `/sessions/:id/stop` - Delete unused `getLatestUserMessage` function **Frontend:** - Remove `window.confirm()` blocking confirmation dialog - In AssistantTurn: When cancelled, strip sentinel message from array, put all real messages in hiddenMessages, render warning-styled "Response stopped" badge instead of summary message **Networking/IPC:** - Add RPC logging to understand cancel flow - Socket.rs: Log cancel events ## Testing - `bun run test:sidecar` — all 198 tests pass - `bun run test:backend` — all tests pass - Manual: Start session → click Stop → agent stops immediately, "Response stopped" appears, session idle - Manual: Restart app → cancelled session persists, shows "Response stopped" badge Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Lite 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds extensive timing instrumentation across backend and sidecar components, removes Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Frontend
participant ClaudeHandler as "Claude Handler"
participant SDK
participant DB as "SessionWriter/DB"
participant FrontendClient
User->>Frontend: click stop/cancel
Frontend->>ClaudeHandler: send cancel request
ClaudeHandler->>ClaudeHandler: set cancelledByUser = true
ClaudeHandler->>ClaudeHandler: create end checkpoint
ClaudeHandler->>SDK: close / force-close SDK process
ClaudeHandler->>DB: persist cancelled assistant message
DB->>FrontendClient: notify subscribers
FrontendClient->>Frontend: broadcast cancelled status
Frontend->>User: render "Response stopped" badge
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
sidecar/build.ts (1)
43-44: Update the comment to reflect actual import semantics, or use dynamic import for true optional loading.
@sentry/nodeis statically imported at module load time (line 6 ofsidecar/index.ts), making it a required dependency regardless of the "optional dependency" comment. Only the initialization is conditional on the DSN environment variable. To make Sentry truly optional, use dynamic import inside the conditional block:if (process.env.SENTRY_DSN) { const Sentry = await import("@sentry/node"); Sentry.init({ /* ... */ }); }Alternatively, update the build comment to clarify that it's a required runtime dependency with conditional initialization.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sidecar/build.ts` around lines 43 - 44, The comment in sidecar/build.ts states "@sentry/node" is an optional dependency but the module is statically imported (see the import of "@sentry/node" in sidecar/index.ts), so either update the comment to say it is a required runtime dependency with conditional initialization, or make Sentry truly optional by removing the static import and performing a dynamic import (await import("@sentry/node")) inside the initialization conditional (the block that checks process.env.SENTRY_DSN and calls Sentry.init); pick one approach and update the comment or the import/initialization accordingly.sidecar/agents/claude/claude-handler.ts (1)
678-700: Extract cancellation finalization into one helper.Both branches perform the same persistence/notification/status updates. Centralizing this avoids divergence.
♻️ Refactor sketch
+ const finalizeUserCancellation = () => { + const model = options?.model || "opus"; + saveAssistantMessage( + sessionId, + { + role: "assistant", + content: [{ type: "text", text: "" }], + stop_reason: "cancelled", + }, + model + ); + FrontendClient.sendMessage({ + id: sessionId, + type: "message", + agentType: "claude", + data: { type: "cancelled" }, + }); + updateSessionStatus(sessionId, "idle"); + }; @@ - if (session.cancelledByUser) { - ... - return; - } + if (session.cancelledByUser) { + finalizeUserCancellation(); + return; + } @@ - if (session.cancelledByUser) { - ... - return; - } + if (session.cancelledByUser) { + finalizeUserCancellation(); + return; + }Also applies to: 715-737
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sidecar/agents/claude/claude-handler.ts` around lines 678 - 700, Extract the cancellation finalization logic into a single helper (e.g., finalizeCancellation or handleCancellation) that accepts sessionId, generatorId, and optional model (use options?.model || "opus" inside the caller or pass model from caller); move the common calls saveAssistantMessage(sessionId, { role: "assistant", content: [{ type: "text", text: "" }], stop_reason: "cancelled" }, model), FrontendClient.sendMessage({ id: sessionId, type: "message", agentType: "claude", data: { type: "cancelled" } }), updateSessionStatus(sessionId, "idle"), and console.log(`[${generatorId}] Session cancelled by user: ${sessionId}`) into that helper, then replace the duplicate blocks (the session.cancelledByUser branch and the other branch around 715-737) with a single call to the new helper to avoid divergence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sidecar/agents/claude/claude-handler.ts`:
- Around line 105-107: Timing log in handleQuery currently prints raw prompt
content (prompt.slice(0,80)) which may leak PII; remove the prompt text from the
console.log in claude-handler.ts and instead log only non-sensitive identifiers
and metadata (e.g., sessionId, timestamp, and optionally prompt length or a
redacted/hashed token). Update the log call near tHandleQuery/sessionId to stop
using prompt.slice and use sessionId and safe metadata (referencing handleQuery,
tHandleQuery, sessionId, and prompt) so timing traces remain useful without
exposing sensitive content.
- Around line 540-543: Move the let declaration for messageCount (and any
related variables used in catch like firstMessageTime and tStreamStart if they
are also referenced) out of the try block and initialize them before entering
the try/for-await-of loop over queryResult so the catch block can safely
read/update them; locate the for-await (for await (const message of
queryResult)) and the catch that references messageCount and relocate/initialize
messageCount = 0 (and firstMessageTime = null, tStreamStart = Date.now() if
needed) above the try to ensure no ReferenceError in the catch handler.
In `@sidecar/db/session-writer.ts`:
- Around line 152-153: The timing log in session-writer's updateSessionStatus
currently interpolates raw errorMessage into the persistent log string; replace
that interpolation with a redacted/sanitized token or a bounded sanitized
preview (e.g., "<redacted-error>" or first N chars plus "...") to avoid writing
full error payloads to disk. Update the log template inside updateSessionStatus
(the backtick template that includes errorMessage) to emit only the redacted
category/preview, and apply the same change to the other timing/startup log site
in this file that likewise interpolates error_message so both locations never
persist full error payloads.
In `@sidecar/index.ts`:
- Around line 155-157: The timing log currently includes raw prompt content via
request.prompt?.slice(0, 80) which can leak secrets/PII; update the log in the
block around tQueryReceived and getAgent to remove any prompt substring and
instead emit safe metadata such as prompt length and a stable hash (e.g.,
sha256) or an anonymized id (e.g., promptLength=request.prompt?.length and
promptHash=sha256(request.prompt || '')). Ensure you reference the same logging
location where tQueryReceived is set and avoid including request.prompt directly
in any console/process logs.
In `@src-tauri/src/socket.rs`:
- Around line 281-282: The code is slicing UTF-8 strings by bytes (e.g.,
creating short_id via &session_id[..session_id.len().min(8)]) which can panic on
multibyte characters and crash the socket listener thread; replace the
byte-range truncation of session_id and any error strings with a char-based
truncation that iterates characters and collects the first 8 characters into a
new String, use that String instead of a byte-sliced &str for short_id and for
truncated error messages, and ensure all places referencing short_id or the
truncated error use the new safe String to avoid panics.
---
Nitpick comments:
In `@sidecar/agents/claude/claude-handler.ts`:
- Around line 678-700: Extract the cancellation finalization logic into a single
helper (e.g., finalizeCancellation or handleCancellation) that accepts
sessionId, generatorId, and optional model (use options?.model || "opus" inside
the caller or pass model from caller); move the common calls
saveAssistantMessage(sessionId, { role: "assistant", content: [{ type: "text",
text: "" }], stop_reason: "cancelled" }, model), FrontendClient.sendMessage({
id: sessionId, type: "message", agentType: "claude", data: { type: "cancelled" }
}), updateSessionStatus(sessionId, "idle"), and console.log(`[${generatorId}]
Session cancelled by user: ${sessionId}`) into that helper, then replace the
duplicate blocks (the session.cancelledByUser branch and the other branch around
715-737) with a single call to the new helper to avoid divergence.
In `@sidecar/build.ts`:
- Around line 43-44: The comment in sidecar/build.ts states "@sentry/node" is an
optional dependency but the module is statically imported (see the import of
"@sentry/node" in sidecar/index.ts), so either update the comment to say it is a
required runtime dependency with conditional initialization, or make Sentry
truly optional by removing the static import and performing a dynamic import
(await import("@sentry/node")) inside the initialization conditional (the block
that checks process.env.SENTRY_DSN and calls Sentry.init); pick one approach and
update the comment or the import/initialization accordingly.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (12)
backend/src/db/queries.tsbackend/src/routes/sessions.tssidecar/agents/claude/claude-handler.tssidecar/agents/claude/claude-session.tssidecar/build.tssidecar/db/session-writer.tssidecar/frontend-client.tssidecar/index.tssidecar/rpc-connection.tssrc-tauri/src/socket.rssrc/features/session/hooks/useSessionActions.tssrc/features/session/ui/AssistantTurn.tsx
💤 Files with no reviewable changes (1)
- backend/src/db/queries.ts
Comment on lines
+152
to
153
| `[TIMING][SESSION-WRITER] updateSessionStatus session=${sessionId} status='${status}' update=${updateMs}ms notify=${notifyMs}ms total=${totalMs}ms${attempt > 0 ? ` retries=${attempt}` : ""}${errorMessage ? ` error: ${errorMessage}` : ""}` | ||
| ); |
There was a problem hiding this comment.
Redact raw error payloads from persistent timing/startup logs.
These logs currently emit full errorMessage / error_message, which can leak sensitive runtime/user/provider data into disk logs. Prefer category/code plus redacted presence (or a sanitized bounded preview).
🔐 Minimal redaction example
- `[TIMING][SESSION-WRITER] updateSessionStatus session=${sessionId} status='${status}' update=${updateMs}ms notify=${notifyMs}ms total=${totalMs}ms${attempt > 0 ? ` retries=${attempt}` : ""}${errorMessage ? ` error: ${errorMessage}` : ""}`
+ `[TIMING][SESSION-WRITER] updateSessionStatus session=${sessionId} status='${status}' update=${updateMs}ms notify=${notifyMs}ms total=${totalMs}ms${attempt > 0 ? ` retries=${attempt}` : ""}${errorMessage ? " error=present" : ""}`
@@
- ` [${s.id.substring(0, 8)}] status=${s.status} agent_session_id=${s.agent_session_id ?? "null"} error=${s.error_message ?? "none"} category=${s.error_category ?? "none"}`
+ ` [${s.id.substring(0, 8)}] status=${s.status} agent_session_id=${s.agent_session_id ? `${s.agent_session_id.substring(0, 8)}…` : "null"} error=${s.error_message ? "[redacted]" : "none"} category=${s.error_category ?? "none"}`Also applies to: 295-296
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sidecar/db/session-writer.ts` around lines 152 - 153, The timing log in
session-writer's updateSessionStatus currently interpolates raw errorMessage
into the persistent log string; replace that interpolation with a
redacted/sanitized token or a bounded sanitized preview (e.g.,
"<redacted-error>" or first N chars plus "...") to avoid writing full error
payloads to disk. Update the log template inside updateSessionStatus (the
backtick template that includes errorMessage) to emit only the redacted
category/preview, and apply the same change to the other timing/startup log site
in this file that likewise interpolates error_message so both locations never
persist full error payloads.
Comment on lines
+281
to
+282
| let short_id = &session_id[..session_id.len().min(8)]; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src-tauri/src/socket.rs | sed -n '275,310p'Repository: zvadaadam/box-ide
Length of output: 2819
Byte-index truncation on &str can panic at UTF-8 boundaries, terminating the socket listener thread.
Lines 281 and 302 use byte-slice indexing on potentially untrusted string inputs. If session_id or error contain multibyte UTF-8 characters, the slices may panic at non-character boundaries, crashing the listener thread and halting message flow.
🛠️ Safer truncation (char-based)
- let short_id = &session_id[..session_id.len().min(8)];
+ let short_id: String = session_id.chars().take(8).collect();Line 302:
- format!("session={} category={} error={}", short_id, category, &error[..error.len().min(80)])
+ let error_short: String = error.chars().take(80).collect();
+ format!("session={} category={} error={}", short_id, category, error_short)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let short_id = &session_id[..session_id.len().min(8)]; | |
| let short_id: String = session_id.chars().take(8).collect(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src-tauri/src/socket.rs` around lines 281 - 282, The code is slicing UTF-8
strings by bytes (e.g., creating short_id via
&session_id[..session_id.len().min(8)]) which can panic on multibyte characters
and crash the socket listener thread; replace the byte-range truncation of
session_id and any error strings with a char-based truncation that iterates
characters and collects the first 8 characters into a new String, use that
String instead of a byte-sliced &str for short_id and for truncated error
messages, and ensure all places referencing short_id or the truncated error use
the new safe String to avoid panics.
…uncation Addresses 3 actionable findings from CodeRabbit review: 1. **Redact raw prompts from logs** (comments #2 + #5): Replace `prompt.slice(0, 80)` with `promptLength` in timing logs in both claude-handler.ts and index.ts. User prompts were being written to /tmp log files — now only the length is logged. 2. **Fix messageCount ReferenceError** (comment #3): Move `let messageCount` declaration from inside the `try` block to before it. In JS, `let` inside `try {}` is NOT accessible in `catch {}` — they are separate block scopes. This caused a ReferenceError in the catch block, masking the actual error. 3. **Safe UTF-8 truncation in Rust** (comment #6): Replace `&error[..error.len().min(80)]` with `error.chars().take(80).collect::<String>()` in socket.rs. Byte-index slicing can panic on multibyte UTF-8 boundaries. (session_id truncation left as-is — UUIDv7 is guaranteed ASCII.) Dismissed as not actionable: - Comment #4 (error redaction in session-writer): Error messages are already sanitized by classifyError(). Redacting would lose debuggability for zero security gain. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
reconcileStuckSessions now calls .prepare().all() for diagnostic logging before the UPDATE. The test mock only had run/get methods, causing .all() to be undefined and the function to hit the catch block. This is a test mock gap, not a production bug. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
zvadaadam
added a commit
that referenced
this pull request
Mar 2, 2026
…ing UI Fixes auto-scroll bugs and improves chat streaming animation: - Replace smooth scroll with requestAnimationFrame chase loop that exponentially lerps toward scrollHeight, providing smooth tracking of growing content during streaming with instant pause detection on user scroll-up - Add userSendCount pattern to distinguish human sends from tool_results, triggering auto-resume only on actual user messages - Implement dual streaming/completed render paths in AssistantTurn: STREAMING: all messages through unified groupMessageToolStreaks pipeline COMPLETED: hidden/summary split with collapsible TurnStatsHeader - Fix isLatest bug: only mark turns isLatest when physically the last turn (prevents completed turns from re-entering streaming mode) - Add idle-chase pause detection: when chase loop self-stops at bottom and user scrolls up, pause is detected immediately via scroll listener - Simplify message dimming: only actively-streaming text block gets opacity-60, eliminating intermediate message dimming and its complex state interactions - Integrate PR #163's cancel sentinel pattern: strip empty cancelled messages from UI, render "Response stopped" badge below content - Remove suppressAutoScrollOnExpand module state (5 components, barrel export) - Apply chat-scroll-contain CSS to ~15 nested scrollables to prevent scroll chaining to main container These changes reduce useAutoScroll complexity from 425 to ~180 lines while fixing three scroll bugs: (1) user scroll-up causes yank-back during streaming, (2) completed text permanently dimmed, (3) previous turn re-expands on new message. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
zvadaadam
added a commit
that referenced
this pull request
Mar 2, 2026
…ing UI (#164) Fixes auto-scroll bugs and improves chat streaming animation: - Replace smooth scroll with requestAnimationFrame chase loop that exponentially lerps toward scrollHeight, providing smooth tracking of growing content during streaming with instant pause detection on user scroll-up - Add userSendCount pattern to distinguish human sends from tool_results, triggering auto-resume only on actual user messages - Implement dual streaming/completed render paths in AssistantTurn: STREAMING: all messages through unified groupMessageToolStreaks pipeline COMPLETED: hidden/summary split with collapsible TurnStatsHeader - Fix isLatest bug: only mark turns isLatest when physically the last turn (prevents completed turns from re-entering streaming mode) - Add idle-chase pause detection: when chase loop self-stops at bottom and user scrolls up, pause is detected immediately via scroll listener - Simplify message dimming: only actively-streaming text block gets opacity-60, eliminating intermediate message dimming and its complex state interactions - Integrate PR #163's cancel sentinel pattern: strip empty cancelled messages from UI, render "Response stopped" badge below content - Remove suppressAutoScrollOnExpand module state (5 components, barrel export) - Apply chat-scroll-contain CSS to ~15 nested scrollables to prevent scroll chaining to main container These changes reduce useAutoScroll complexity from 425 to ~180 lines while fixing three scroll bugs: (1) user scroll-up causes yank-back during streaming, (2) completed text permanently dimmed, (3) previous turn re-expands on new message. Co-authored-by: Claude Haiku 4.5 <noreply@anthropic.com>
zvadaadam
added a commit
that referenced
this pull request
Apr 2, 2026
…ing UI Fixes auto-scroll bugs and improves chat streaming animation: - Replace smooth scroll with requestAnimationFrame chase loop that exponentially lerps toward scrollHeight, providing smooth tracking of growing content during streaming with instant pause detection on user scroll-up - Add userSendCount pattern to distinguish human sends from tool_results, triggering auto-resume only on actual user messages - Implement dual streaming/completed render paths in AssistantTurn: STREAMING: all messages through unified groupMessageToolStreaks pipeline COMPLETED: hidden/summary split with collapsible TurnStatsHeader - Fix isLatest bug: only mark turns isLatest when physically the last turn (prevents completed turns from re-entering streaming mode) - Add idle-chase pause detection: when chase loop self-stops at bottom and user scrolls up, pause is detected immediately via scroll listener - Simplify message dimming: only actively-streaming text block gets opacity-60, eliminating intermediate message dimming and its complex state interactions - Integrate PR #163's cancel sentinel pattern: strip empty cancelled messages from UI, render "Response stopped" badge below content - Remove suppressAutoScrollOnExpand module state (5 components, barrel export) - Apply chat-scroll-contain CSS to ~15 nested scrollables to prevent scroll chaining to main container These changes reduce useAutoScroll complexity from 425 to ~180 lines while fixing three scroll bugs: (1) user scroll-up causes yank-back during streaming, (2) completed text permanently dimmed, (3) previous turn re-expands on new message. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixed the session cancel/stop flow to actually stop the running agent and provide visual feedback to users.
Problem: When users clicked Stop, the agent kept running and there was no visual indication of cancellation.
Solution:
query.close()instead ofquery.interrupt()to force-kill the subprocess and all childrencancelledByUserflag to persist cancellation through both post-loop and catch block pathsChanges
interrupt()withclose(), add cancellation sentinel persistence, add TIMING instrumentationcancelled_atupdate code and unusedgetLatestUserMessagefunction🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
New Features
Improvements