fix(terminal): wait for PTY exit on Windows before recreating terminal #1184
Conversation
On Windows, PTY process termination is asynchronous and takes longer than macOS/Linux. When switching worktrees, the terminal would close but not reopen because the new PTY creation conflicted with the still-shutting-down old PTY. This fix adds promise-based wait for PTY exit on Windows: - Add pendingExitPromises map to track terminals being destroyed - Add waitForPtyExit() function with platform-specific timeouts - Modify killPty() to optionally wait for exit - Update destroyTerminal() to wait for PTY exit on Windows only The timeout fallback (2000ms Windows, 500ms Unix) ensures no hangs if the exit event never fires. Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
Test User seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
github-actions
bot
added
area/frontend
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdded guarded PTY exit tracking and wait semantics (platform-tuned timeouts), tightened terminal destruction to avoid races, extended killPty to optionally await PTY exit, introduced Windows shell resolution and a spawnPtyProcess export, and updated PTY handlers to resolve pending exit promises before cleanup. Changes
Sequence Diagram(s)sequenceDiagram
participant Lifecycle as TerminalLifecycle
participant Manager as PtyManager
participant OS as Platform/OS
participant Pty as PTYProcess
Lifecycle->>Manager: destroyTerminal(terminal)
note right of Manager: map delete before kill to avoid race
Lifecycle->>Manager: killPty(terminal, waitForExit=true)
Manager->>Pty: kill()
alt kill throws
Manager->>Manager: cleanup pending exit state
Manager-->>Lifecycle: resolve/return
else kill succeeds
Manager->>Manager: waitForPtyExit(terminalId, timeout)
par PTY exit path
Pty-->>Manager: onExit
Manager->>Manager: resolve pending exit promise
Manager-->>Lifecycle: exit awaited resolved
and timeout path
Manager-->>Lifecycle: timeout resolves
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (5)apps/frontend/src/**/*.{tsx,ts}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{ts,tsx,js,py}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
apps/frontend/src/main/**/*.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
apps/frontend/**/*.{ts,tsx,js}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
apps/frontend/**/*.{ts,tsx}⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (1)📚 Learning: 2026-01-16T09:10:31.701ZApplied to files:
🧬 Code graph analysis (1)apps/frontend/src/main/terminal/pty-manager.ts (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
🔇 Additional comments (5)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @AndyMik90, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical race condition on Windows that caused terminal recreation to fail, particularly when switching worktrees. The core issue stemmed from the asynchronous nature of PTY process termination on Windows. By introducing a promise-based waiting mechanism, the system now ensures that a PTY has fully exited before attempting to create a new one, thereby stabilizing terminal behavior in scenarios requiring rapid terminal lifecycle management. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses a race condition on Windows during terminal recreation by introducing a mechanism to wait for the PTY process to exit. The changes are well-structured, with new functions like waitForPtyExit and modifications to killPty and destroyTerminal being clear and platform-specific as intended. The use of a promise-based approach with timeouts is a solid solution. My main feedback is a suggestion to improve the robustness of waitForPtyExit to prevent potential resource leaks in case it's called multiple times for the same terminal, making the implementation more resilient for future changes.
| export function waitForPtyExit(terminalId: string, timeoutMs?: number): Promise<void> { | ||
| const timeout = timeoutMs ?? (isWindows() ? PTY_EXIT_TIMEOUT_WINDOWS : PTY_EXIT_TIMEOUT_UNIX); | ||
|
|
||
| return new Promise<void>((resolve) => { | ||
| // Set up timeout fallback | ||
| const timeoutId = setTimeout(() => { | ||
| debugLog('[PtyManager] PTY exit timeout for terminal:', terminalId); | ||
| pendingExitPromises.delete(terminalId); | ||
| resolve(); | ||
| }, timeout); | ||
|
|
||
| // Store the promise resolver | ||
| pendingExitPromises.set(terminalId, { resolve, timeoutId }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
If this function is called multiple times for the same terminalId before the PTY exits, it could lead to a resource leak. Each call creates a new promise and a new timeout, but only the latest one is tracked, orphaning previous promises and their timeouts. To make this more robust, it's good practice to clean up any existing waiters for the same terminal ID before creating a new one.
export function waitForPtyExit(terminalId: string, timeoutMs?: number): Promise<void> {
const timeout = timeoutMs ?? (isWindows() ? PTY_EXIT_TIMEOUT_WINDOWS : PTY_EXIT_TIMEOUT_UNIX);
// If we are already waiting for this terminal, clear the old timeout to prevent leaks.
const existing = pendingExitPromises.get(terminalId);
if (existing) {
debugLog('[PtyManager] PTY exit already being awaited, overwriting for terminal:', terminalId);
clearTimeout(existing.timeoutId);
}
return new Promise<void>((resolve) => {
// Set up timeout fallback
const timeoutId = setTimeout(() => {
debugLog('[PtyManager] PTY exit timeout for terminal:', terminalId);
pendingExitPromises.delete(terminalId);
resolve();
}, timeout);
// Store the promise resolver
pendingExitPromises.set(terminalId, { resolve, timeoutId });
});
}![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
Merge Verdict: 🟠 NEEDS REVISION
🟠 Needs revision - 2 issue(s) require attention.
2 issue(s) must be addressed (0 required, 2 recommended), 2 suggestions
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | Low | Based on lines changed |
| Security Impact | None | Based on security findings |
| Scope Coherence | Good | Based on structural review |
Findings Summary
- Medium: 2 issue(s)
- Low: 2 issue(s)
Generated by Auto Claude PR Review
Findings (4 selected of 4 total)
🟡 [0cd9931b91f7] [MEDIUM] Race condition: onExit handler may delete newly created terminal with same ID
📁 apps/frontend/src/main/terminal/pty-manager.ts:171
On Windows, destroyTerminal() awaits PTY exit. During this await, if createTerminal() is called with the same ID, a new terminal is added to the map. When the old PTY's onExit fires, it checks terminals.has(id) which returns true for the NEW terminal, incorrectly deleting it. The race window exists because JavaScript yields to the event loop during await.
Suggested fix:
Store a reference to the specific terminal object (not just ID) in the onExit closure, then compare before deleting: `if (terminals.get(id) === terminal) { terminals.delete(id); }`
🟡 [9484a6ff608d] [MEDIUM] Union return type 'Promise | void' is error-prone for callers
📁 apps/frontend/src/main/terminal/pty-manager.ts:284
The killPty function returns Promise | void based on the waitForExit parameter. This conditional return type is error-prone because callers must remember to await when waitForExit=true. TypeScript does not enforce awaiting promises, making this easy to misuse. A forgotten await would cause the promise to be silently dropped.
Suggested fix:
Use function overloads for type-safe return types:
```typescript
export function killPty(terminal: TerminalProcess, waitForExit: true): Promise<void>;
export function killPty(terminal: TerminalProcess, waitForExit?: false): void;
export function killPty(terminal: TerminalProcess, waitForExit?: boolean): Promise<void> | void { ... }
#### 🔵 [a5a464bd3c5e] [LOW] No error cleanup if terminal.pty.kill() throws with waitForExit=true
📁 `apps/frontend/src/main/terminal/pty-manager.ts:286`
When waitForExit=true, the code creates the exit promise first, then calls kill(). If kill() throws, the promise from waitForPtyExit() will never resolve (only times out after 2000ms), and the pendingExitPromises map entry is left orphaned until timeout. The caller's try/catch handles the exception, but the delayed timeout could cause subtle timing issues.
**Suggested fix:**
Wrap kill() in try/catch and clean up the pending promise on error:
try {
terminal.pty.kill();
} catch (error) {
const pending = pendingExitPromises.get(terminal.id);
if (pending) {
clearTimeout(pending.timeoutId);
pendingExitPromises.delete(terminal.id);
pending.resolve();
}
throw error;
}
#### 🔵 [6048dfc70b60] [LOW] destroyAllTerminals does not wait for PTY exit on Windows (inconsistent)
📁 `apps/frontend/src/main/terminal/terminal-lifecycle.ts:292`
The destroyAllTerminals() function calls killPty(terminal) without waitForExit=true even on Windows, unlike the single-terminal destroyTerminal() which correctly waits. This inconsistency could cause the same race condition if terminals are recreated after destroyAllTerminals(). However, this function is only called during app shutdown where recreation is unlikely.
**Suggested fix:**
Add a comment explaining the intentional difference, or apply the same pattern for consistency:
// Note: No need to wait for exit here since this is only called during
// app shutdown when no terminals will be recreated.
---
*This review was generated by Auto Claude.*
- Fix race condition: compare terminal object reference (not just ID) in onExit handler to prevent deleting newly created terminals with same ID - Add function overloads for killPty() for type-safe return types - Add error cleanup: wrap kill() in try/catch to clean up pending promise if kill() throws - Add comment explaining why destroyAllTerminals() doesn't wait for PTY exit Co-Authored-By: Claude Opus 4.5 <[email protected]>
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| }, timeout); | ||
|
|
||
| // Store the promise resolver | ||
| pendingExitPromises.set(terminalId, { resolve, timeoutId }); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| }, timeout); | ||
|
|
||
| // Store the promise resolver | ||
| pendingExitPromises.set(terminalId, { resolve, timeoutId }); |
There was a problem hiding this comment.
Concurrent destroy with same ID causes wrong promise resolution
Medium Severity
The pendingExitPromises map is keyed only by terminal ID. If terminal A is being destroyed and a new terminal B is created with the same ID during the await, then B is also destroyed, B's promise resolver overwrites A's. When A's PTY exits, it resolves B's promise instead of A's - causing destroyTerminal(B) to return before B's PTY has actually exited. This recreates the race condition the PR is trying to fix. The terminals map handles this with an object identity check, but pendingExitPromises lacks equivalent protection.
Additional Locations (1)
AndyMik90#1184) * fix(terminal): wait for PTY exit on Windows before recreating terminal On Windows, PTY process termination is asynchronous and takes longer than macOS/Linux. When switching worktrees, the terminal would close but not reopen because the new PTY creation conflicted with the still-shutting-down old PTY. This fix adds promise-based wait for PTY exit on Windows: - Add pendingExitPromises map to track terminals being destroyed - Add waitForPtyExit() function with platform-specific timeouts - Modify killPty() to optionally wait for exit - Update destroyTerminal() to wait for PTY exit on Windows only The timeout fallback (2000ms Windows, 500ms Unix) ensures no hangs if the exit event never fires. Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix(terminal): address PR review feedback - Fix race condition: compare terminal object reference (not just ID) in onExit handler to prevent deleting newly created terminals with same ID - Add function overloads for killPty() for type-safe return types - Add error cleanup: wrap kill() in try/catch to clean up pending promise if kill() throws - Add comment explaining why destroyAllTerminals() doesn't wait for PTY exit Co-Authored-By: Claude Opus 4.5 <[email protected]> --------- Co-authored-by: Test User <[email protected]> Co-authored-by: Claude Opus 4.5 <[email protected]>
AndyMik90#1184) * fix(terminal): wait for PTY exit on Windows before recreating terminal On Windows, PTY process termination is asynchronous and takes longer than macOS/Linux. When switching worktrees, the terminal would close but not reopen because the new PTY creation conflicted with the still-shutting-down old PTY. This fix adds promise-based wait for PTY exit on Windows: - Add pendingExitPromises map to track terminals being destroyed - Add waitForPtyExit() function with platform-specific timeouts - Modify killPty() to optionally wait for exit - Update destroyTerminal() to wait for PTY exit on Windows only The timeout fallback (2000ms Windows, 500ms Unix) ensures no hangs if the exit event never fires. Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix(terminal): address PR review feedback - Fix race condition: compare terminal object reference (not just ID) in onExit handler to prevent deleting newly created terminals with same ID - Add function overloads for killPty() for type-safe return types - Add error cleanup: wrap kill() in try/catch to clean up pending promise if kill() throws - Add comment explaining why destroyAllTerminals() doesn't wait for PTY exit Co-Authored-By: Claude Opus 4.5 <[email protected]> --------- Co-authored-by: Test User <[email protected]> Co-authored-by: Claude Opus 4.5 <[email protected]>
Summary
Fixes a race condition on Windows where terminal recreation fails when switching worktrees.
Changes
pendingExitPromisesmap to track terminals being destroyedwaitForPtyExit()function with platform-specific timeouts (2000ms Windows, 500ms Unix)killPty()to optionally wait for exitdestroyTerminal()to wait for PTY exit on Windows onlyTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.