fix: solve #2960 — PTY spawn failure leaves zombie sessions blocking new terminals#2963
Merged
Conversation
…sessions (#2960) When pty.spawn() fails (e.g. posix_spawnp failed), the subprocess sent an error frame but stayed alive. This left session.isAlive returning true for a broken session with no PTY, causing TerminalHost to store and attach to it — blocking new terminal creation. Two fixes: - pty-subprocess.ts: exit with code 1 after spawn failure so the daemon correctly detects the session as dead - terminal-host.ts: also check session.pid after ready timeout to catch edge cases where subprocess is alive but PTY never spawned
Contributor
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/main/terminal-host/terminal-host.test.ts">
<violation number="1" location="apps/desktop/src/main/terminal-host/terminal-host.test.ts:141">
P1: Using real-looking fake PIDs with `session.dispose()` can send SIGKILL to unrelated OS processes during test runs.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const terminalHostWouldReject = !session.isAlive; | ||
| expect(terminalHostWouldReject).toBe(false); // BUG: should be true! | ||
|
|
||
| await session.dispose(); |
Contributor
There was a problem hiding this comment.
P1: Using real-looking fake PIDs with session.dispose() can send SIGKILL to unrelated OS processes during test runs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/main/terminal-host/terminal-host.test.ts, line 141:
<comment>Using real-looking fake PIDs with `session.dispose()` can send SIGKILL to unrelated OS processes during test runs.</comment>
<file context>
@@ -0,0 +1,263 @@
+ const terminalHostWouldReject = !session.isAlive;
+ expect(terminalHostWouldReject).toBe(false); // BUG: should be true!
+
+ await session.dispose();
+ });
+
</file context>
Contributor
Author
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
siarhei-belavus
pushed a commit
to siarhei-belavus/localset
that referenced
this pull request
Mar 30, 2026
…s blocking new terminals (superset-sh#2963) * fix(desktop): exit subprocess on PTY spawn failure to prevent zombie sessions (superset-sh#2960) When pty.spawn() fails (e.g. posix_spawnp failed), the subprocess sent an error frame but stayed alive. This left session.isAlive returning true for a broken session with no PTY, causing TerminalHost to store and attach to it — blocking new terminal creation. Two fixes: - pty-subprocess.ts: exit with code 1 after spawn failure so the daemon correctly detects the session as dead - terminal-host.ts: also check session.pid after ready timeout to catch edge cases where subprocess is alive but PTY never spawned * Deslop --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Kiet Ho <hoakiet98@gmail.com> (cherry picked from commit 8af71b6)
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
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
pty.spawn()fails (e.g.posix_spawnp failed) inpty-subprocess.ts, the subprocess sends an error frame but does not exit. This leavessession.isAlivereturningtruefor a session with no PTY, soTerminalHoststores and attaches to a broken session — blocking new terminal creation.pty-subprocess.ts): Exit the subprocess with code 1 after spawn failure, so the daemon correctly detects the session as dead viasession.isAlive === false.terminal-host.ts): After the ready timeout, also checksession.pid === nullto catch edge cases where the subprocess is alive but the PTY never spawned (belt-and-suspenders defense).terminal-host.test.tswith 4 tests covering spawn failure scenarios, verifying the bug behavior and the fix.Test plan
session.isAliveistruewhen subprocess is alive but PTY failed to spawnsession.isAlivereturnfalsesession.pid === nullcheck catches broken sessions even if subprocess is aliveCloses #2960
Summary by cubic
Fixes zombie terminal sessions when
pty.spawn()fails. The subprocess now exits on spawn error andTerminalHostrejects sessions with no PID, unblocking new terminals. Closes #2960.Bug Fixes
pty-subprocess.ts, sosession.isAlivebecomes false.terminal-host.ts, also checksession.pid === nulland reject the session.Refactors
chatrouter, simpler error text inworkspace-client, and a tighter auth token check inPskHostAuthProvider.Written for commit f639c7e. Summary will update on new commits.