Fix PTY cleanup on app quit

[janburzinski]

this fixes a crash where the user had a dev server running, quit and then tried to start again and then it crashed (it was @mezotv :D)

[janburzinski]

@greptileai

[greptile-apps]

Greptile Summary

This PR fixes a crash-on-relaunch bug where PTY processes (dev servers, etc.) spawned in a previous session weren't killed on app quit, leaving orphaned processes that collided with the next launch. The fix introduces explicit PTY shutdown plumbing end-to-end.

• LocalPtySession gains killed/exited/suppressExit state flags, a shutdown() method that suppresses the downstream onExit callback before killing the process, and a module-level localPtys set so shutdownLocalPtys() can sweep any PTY not already covered by the session registry.
• PtySessionRegistry adds shutdownAll() which kills every registered PTY (preferring shutdown() over kill()) and unregisters them synchronously; index.ts calls both in the finally block of a fully rewritten before-quit handler, which also gains an isQuitting re-entry guard and per-service error handling.

Confidence Score: 4/5

Safe to merge — the shutdown path is additive and well-guarded; the two rough edges are confined to teardown code that runs once on quit.

The core fix (suppress-then-kill, module-level tracking, structured async cleanup) is correct. Two minor robustness gaps exist in PtySessionRegistry.shutdownAll(): the unregister() call sits outside the per-session try-catch, so an unlikely throw from the input-subscription cleanup function would abort the loop and leave remaining PTYs unkilled; and the 16 ms flush timer in the register() closure is never cleared when suppressExit suppresses the onExit handler, causing a late ptyDataChannel emit after the session is already unregistered. Neither affects normal operation or the launch-crash scenario being fixed.

src/main/core/pty/pty-session-registry.ts — both issues live in this file.

Important Files Changed

Filename	Overview
src/main/core/pty/local-pty.ts	Adds module-level PTY tracking set, killed/exited/suppressExit guards, shutdown() method, and shutdownLocalPtys() export. Logic is sound; guards prevent post-kill writes correctly.
src/main/core/pty/pty-session-registry.ts	Adds shutdownAll() to kill and unregister all active sessions. The unregister() call sits outside the loop's try-catch, and the flush timer from the register() closure is never cleared when the exit handler is suppressed.
src/main/core/pty/pty.ts	Adds optional shutdown?() to the Pty interface. Minimal, correct change.
src/main/index.ts	Rewrites before-quit into a structured async cleanup with an isQuitting re-entry guard, PTY shutdown in the finally block, individual error handling per service, and app.exit(1) fallback on unhandled errors.

Sequence Diagram

sequenceDiagram
    participant App as app (index.ts)
    participant Registry as PtySessionRegistry
    participant Session as LocalPtySession
    participant Proc as node-pty proc

    App->>App: "before-quit fires, isQuitting=false set true"
    App->>App: event.preventDefault()
    App->>App: telemetryService.dispose() await
    Note over App: finally block
    App->>Registry: shutdownAll()
    loop each session in ptyMap snapshot
        Registry->>Session: shutdown()
        Session->>Session: "suppressExit = true"
        Session->>Session: "kill() killed=true localPtys.delete"
        Session->>Proc: proc.kill()
        Registry->>Registry: unregister(sessionId)
    end
    App->>App: shutdownLocalPtys() safety net for unregistered PTYs
    App->>App: dispose agentHookService resourceSampler updateService prSyncScheduler
    App->>App: gitWatcherRegistry.dispose() await
    App->>App: projectManager.dispose() await
    App->>App: app.exit(0)
    Note over Proc: onExit fires async suppressExit=true handler skipped

Loading

Comments Outside Diff (1)

1. src/main/core/pty/pty-session-registry.ts, line 58-73 (link)

   flushTimer is never cleared when suppressExit=true

   When a session is shut down via pty.shutdown(), suppressExit=true causes LocalPtySession.onExit to return early — bypassing the timer cleanup at the top of the onExit handler in register(). If a 16 ms flush timer is pending at the moment of shutdown, it fires after shutdownAll() has already called unregister(sessionId), emitting ptyDataChannel for a session that is no longer tracked. During app exit this is benign, but adding a clearTimeout(flushTimer) call when the session is explicitly shut down would make teardown cleaner.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: src/main/core/pty/pty-session-registry.ts
   Line: 58-73
   
   Comment:
   **`flushTimer` is never cleared when `suppressExit=true`**
   
   When a session is shut down via `pty.shutdown()`, `suppressExit=true` causes `LocalPtySession.onExit` to return early — bypassing the timer cleanup at the top of the `onExit` handler in `register()`. If a 16 ms flush timer is pending at the moment of shutdown, it fires after `shutdownAll()` has already called `unregister(sessionId)`, emitting `ptyDataChannel` for a session that is no longer tracked. During app exit this is benign, but adding a `clearTimeout(flushTimer)` call when the session is explicitly shut down would make teardown cleaner.
   
   How can I resolve this? If you propose a fix, please make it concise.

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/main/core/pty/pty-session-registry.ts:95-106
**`unregister` call outside the try-catch block**

`this.unregister(sessionId)` is placed outside the loop's `try/catch`. If the `ptyInputSubscriptions` off-function throws (it's an arbitrary user-land callback), the exception propagates out of the `for` loop and all remaining PTY sessions in the snapshot go unkilled — which is exactly the bug this PR targets. Wrapping the whole loop body (both the kill and the unregister) in a single try-catch would ensure every session is processed even on individual failures.

### Issue 2 of 2
src/main/core/pty/pty-session-registry.ts:58-73
**`flushTimer` is never cleared when `suppressExit=true`**

When a session is shut down via `pty.shutdown()`, `suppressExit=true` causes `LocalPtySession.onExit` to return early — bypassing the timer cleanup at the top of the `onExit` handler in `register()`. If a 16 ms flush timer is pending at the moment of shutdown, it fires after `shutdownAll()` has already called `unregister(sessionId)`, emitting `ptyDataChannel` for a session that is no longer tracked. During app exit this is benign, but adding a `clearTimeout(flushTimer)` call when the session is explicitly shut down would make teardown cleaner.

_{Reviews (1): Last reviewed commit: "Fix PTY cleanup on app quit" | Re-trigger Greptile}

[greptile-apps]

unregister call outside the try-catch block

this.unregister(sessionId) is placed outside the loop's try/catch. If the ptyInputSubscriptions off-function throws (it's an arbitrary user-land callback), the exception propagates out of the for loop and all remaining PTY sessions in the snapshot go unkilled — which is exactly the bug this PR targets. Wrapping the whole loop body (both the kill and the unregister) in a single try-catch would ensure every session is processed even on individual failures.

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/main/core/pty/pty-session-registry.ts
Line: 95-106

Comment:
**`unregister` call outside the try-catch block**

`this.unregister(sessionId)` is placed outside the loop's `try/catch`. If the `ptyInputSubscriptions` off-function throws (it's an arbitrary user-land callback), the exception propagates out of the `for` loop and all remaining PTY sessions in the snapshot go unkilled — which is exactly the bug this PR targets. Wrapping the whole loop body (both the kill and the unregister) in a single try-catch would ensure every session is processed even on individual failures.

How can I resolve this? If you propose a fix, please make it concise.