Skip to content

fix(resource-monitor): add ability to stop sessions #2248

Merged
arnestrickmann merged 3 commits into
mainfrom
emdash/killing-in-resource-monitor-wzxcw
May 27, 2026
Merged

fix(resource-monitor): add ability to stop sessions #2248
arnestrickmann merged 3 commits into
mainfrom
emdash/killing-in-resource-monitor-wzxcw

Conversation

@janburzinski
Copy link
Copy Markdown
Collaborator

summary

  • adds the abiltiy to kill processes in the resource monitor
Screen.Recording.2026-05-27.at.20.07.42.mov

@janburzinski janburzinski changed the title fix(resource-monitor): stop sessions when killing fix(resource-monitor): add ability to kill sessions May 27, 2026
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 27, 2026

Greptile Summary

This PR fixes the resource-monitor kill action that previously appeared to do nothing — a bare pty.kill() left the provider's respawn tracking intact, so the PTY was restarted ~500ms later. The fix adds a stopSession RPC handler that routes through the owning provider to remove the session from knownSessionIds/sessions, and adds a two-click arm/confirm UI in the resource monitor.

  • controller.ts: New stopSession handler distinguishes conversation PTYs from terminal PTYs via registry metadata, delegates to the correct provider method, and falls back to a raw kill for lifecycle scripts that never respawn.
  • resource-monitor-view.tsx: Adds KillButton with a first-click arm state that auto-disarms after 3 s, a confirm "Kill?" button on second click, and a group-hover fade effect to hide the CPU/memory stats while the controls are visible.

Confidence Score: 4/5

The core fix is well-reasoned and the two-click confirmation prevents accidental kills; two edge-case issues are present but neither affects the happy path.

The stopSession controller correctly solves the respawn problem. The main concerns are: the success branch of handleClick never resets killing/armed, so a session entry that survives the post-kill refresh (slow cleanup, race) leaves the button permanently disabled with no retry path; and the task-based code path in the controller has no try/catch around the provider calls, unlike the lifecycle-script path immediately below it.

Both files are worth a second look: resource-monitor-view.tsx for the missing state reset on success, and controller.ts for the asymmetric error handling between the task and lifecycle paths.

Important Files Changed

Filename Overview
src/main/core/pty/controller.ts Adds stopSession RPC handler that routes through the owning provider to prevent respawn; lifecycle path has error handling but task-based path does not wrap provider calls in try/catch
src/renderer/features/command-palette/resource-monitor-view.tsx Adds two-click kill UX with arm/confirm pattern and 3 s auto-disarm; success path omits state reset which can leave the button stuck disabled if the row survives a refresh

Sequence Diagram

sequenceDiagram
    participant U as User
    participant KB as KillButton (renderer)
    participant RPC as rpc.pty
    participant C as ptyController
    participant TM as taskManager
    participant P as Provider (conversations/terminals)
    participant RM as ResourceMonitor

    U->>KB: click (unarmed)
    KB->>KB: setArmed(true), start 3s timer

    U->>KB: click (armed)
    KB->>KB: clearReset(), setKilling(true)
    KB->>RPC: stopSession(sessionId)
    RPC->>C: stopSession(sessionId)
    C->>C: parsePtySessionId()
    alt task found
        C->>TM: getTask(scopeId)
        C->>C: check metadata for providerId
        alt isConversation
            C->>P: task.conversations.stopSession(leafId)
        else terminal
            C->>P: task.terminals.killTerminal(leafId)
        end
        C-->>RPC: ok()
    else lifecycle script (no task)
        C->>C: ptySessionRegistry.get(sessionId)
        C->>C: pty.kill()
        C->>C: ptySessionRegistry.unregister(sessionId)
        C-->>RPC: ok()
    end
    RPC-->>KB: ok()
    KB->>RM: appState.resourceMonitor.refresh()
    RM-->>KB: done
    note over KB: killing/armed NOT reset on success
Loading
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
src/renderer/features/command-palette/resource-monitor-view.tsx:252-258
**Permanently disabled button on success with lingering entry**

If `stopSession` returns successfully but `refresh()` doesn't remove the entry from the snapshot (e.g., the process is slow to clean up, or there's a brief race where the entry is still present), `killing` stays `true` and `armed` stays `true` with no reset path — the "Kill?" button stays permanently disabled and the user has no way to retry. The `catch` branch correctly resets both pieces of state; the success branch should do the same after the refresh resolves.

### Issue 2 of 2
src/main/core/pty/controller.ts:104-112
**Unhandled exceptions in task-based path**

`task.conversations.stopSession` and `task.terminals.killTerminal` are awaited without any try/catch, so if either throws the async function rejects with an unstructured error. The lifecycle-script path below wraps `pty.kill()` in its own try/catch (lines 118–122) for exactly this reason. Either wrap these two calls similarly, or let the RPC framework document that it handles raw rejections — the current asymmetry leaves the task path exposed if either provider method throws unexpectedly.

Reviews (1): Last reviewed commit: "fix(resource-monitor): stop sessions whe..." | Re-trigger Greptile

Comment thread src/renderer/features/command-palette/resource-monitor-view.tsx
Comment thread src/main/core/pty/controller.ts
@arnestrickmann
Copy link
Copy Markdown
Contributor

Nice!
Can we change the wording from kill to sth different?

@janburzinski
Copy link
Copy Markdown
Collaborator Author

haha fair, maybe close?

@janburzinski
Copy link
Copy Markdown
Collaborator Author

renmaing it to "stop" :D

@janburzinski janburzinski changed the title fix(resource-monitor): add ability to kill sessions fix(resource-monitor): add ability to stop sessions May 27, 2026
@arnestrickmann
Copy link
Copy Markdown
Contributor

yes, better, thank you

@arnestrickmann arnestrickmann merged commit 892e7c4 into main May 27, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants