fix(desktop): keep terminal-host daemon alive across app quit#3574
fix(desktop): keep terminal-host daemon alive across app quit#3574Haknt wants to merge 2 commits intosuperset-sh:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds an early no-op SIGHUP listener during startup and changes the SIGHUP shutdown handler to ignore the signal with a guarded informational log; other signal and error shutdown flows remain unchanged. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Greptile SummaryThis PR fixes a macOS-specific bug where Cmd+Q was tearing down the terminal-host daemon despite it being spawned with Key changes:
Confidence Score: 4/5Safe to merge — the core fix is correct and well-targeted; two minor issues (unprotected log call and redundant listener) are worth addressing before shipping. The SIGHUP no-op approach is the standard nohup mechanism and is correctly applied. The belt-and-suspenders early handler in main() closes the startup race window. One P1 concern: the
|
| Filename | Overview |
|---|---|
| apps/desktop/src/main/terminal-host/signal-handlers.ts | Adds a SIGHUP no-op listener (nohup semantics) to prevent macOS login-session teardown from killing the daemon; the log() call inside the handler lacks a try/catch that could trigger an unintended shutdown if stdout is a closed pipe. |
| apps/desktop/src/main/terminal-host/index.ts | Adds an early silent SIGHUP guard in main() to cover the race window before setupSignalHandlers() runs; the temporary listener is never removed, leaving two permanent SIGHUP listeners (harmless but redundant). |
Sequence Diagram
sequenceDiagram
participant Electron as Electron (main process)
participant OS as macOS Login Session
participant Daemon as terminal-host daemon
Note over Daemon: main() registers early SIGHUP → () => {}
Note over Daemon: setupSignalHandlers() registers SIGHUP → log + no-op
Electron->>OS: app.exit(0) tears down login session
OS-->>Daemon: SIGHUP (login session teardown)
Daemon->>Daemon: Early handler: silent no-op (fires first)
Daemon->>Daemon: Signal handler: log "ignoring SIGHUP"
Note over Daemon: Daemon survives — PTY sessions intact
Note over Electron: User relaunches app
Electron->>Daemon: connect via Unix socket
Daemon-->>Electron: Session hot-reattach ("attached" not "created")
Prompt To Fix All With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/terminal-host/index.ts
Line: 835-837
Comment:
**Redundant listener left after `setupSignalHandlers()`**
The early silent `() => {}` registered on line 835 is never removed before `setupSignalHandlers()` (line 837) installs its own SIGHUP listener. `process.on` always _adds_ a listener rather than replacing one, so after startup two SIGHUP listeners are permanently active: the silent one and the logging one from `signal-handlers.ts`.
This is harmless in practice — both handlers are no-ops that prevent the default terminate action, and two listeners is well below Node's `MaxListenersExceededWarning` threshold (10). However, it slightly pollutes the listener list and could confuse future readers who expect exactly one handler.
A cleaner approach is to store and remove the temporary listener once the definitive one is installed:
```ts
const earlyHupGuard = () => {};
process.on("SIGHUP", earlyHupGuard);
setupSignalHandlers(); // installs the logging handler
process.removeListener("SIGHUP", earlyHupGuard);
```
Or, since the logging handler is equally inert during startup, simply skip the early guard entirely and let `setupSignalHandlers()` install the only listener — moving the SIGHUP registration to the top of that function achieves the same race-window protection.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/desktop/src/main/terminal-host/signal-handlers.ts
Line: 140-142
Comment:
**Unprotected `log()` call inside signal handler**
If the daemon's stdout/stderr is still connected to Electron's pipe (i.e., spawned with `stdio: 'pipe'` or `stdio: 'inherit'` rather than `stdio: 'ignore'`), and Electron exits first, writing to that closed pipe will throw `EPIPE`. Because the `log()` call here is not wrapped in a `try/catch`, an EPIPE will propagate as an uncaught exception into the `uncaughtException` handler. `EPIPE` is not in `TRANSIENT_ERROR_CODES`, so `shutdownOnce()` would be called with `exitCode: 1` — defeating the entire purpose of this no-op handler.
If stdio is already redirected to a file or `/dev/null` at spawn time, this is a non-issue. But it is worth adding a guard to make the handler unconditionally safe:
```ts
process.on("SIGHUP", () => {
try {
log("info", "Received SIGHUP; ignoring (nohup semantics for daemon survival)");
} catch {
// Suppress logging errors (e.g. EPIPE after parent exit) —
// the whole point of this handler is to survive without side-effects.
}
});
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(desktop): keep terminal-host daemon ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| process.on("SIGHUP", () => {}); | ||
|
|
||
| setupSignalHandlers(); |
There was a problem hiding this comment.
setupSignalHandlers()
The early silent () => {} registered on line 835 is never removed before setupSignalHandlers() (line 837) installs its own SIGHUP listener. process.on always adds a listener rather than replacing one, so after startup two SIGHUP listeners are permanently active: the silent one and the logging one from signal-handlers.ts.
This is harmless in practice — both handlers are no-ops that prevent the default terminate action, and two listeners is well below Node's MaxListenersExceededWarning threshold (10). However, it slightly pollutes the listener list and could confuse future readers who expect exactly one handler.
A cleaner approach is to store and remove the temporary listener once the definitive one is installed:
const earlyHupGuard = () => {};
process.on("SIGHUP", earlyHupGuard);
setupSignalHandlers(); // installs the logging handler
process.removeListener("SIGHUP", earlyHupGuard);Or, since the logging handler is equally inert during startup, simply skip the early guard entirely and let setupSignalHandlers() install the only listener — moving the SIGHUP registration to the top of that function achieves the same race-window protection.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/terminal-host/index.ts
Line: 835-837
Comment:
**Redundant listener left after `setupSignalHandlers()`**
The early silent `() => {}` registered on line 835 is never removed before `setupSignalHandlers()` (line 837) installs its own SIGHUP listener. `process.on` always _adds_ a listener rather than replacing one, so after startup two SIGHUP listeners are permanently active: the silent one and the logging one from `signal-handlers.ts`.
This is harmless in practice — both handlers are no-ops that prevent the default terminate action, and two listeners is well below Node's `MaxListenersExceededWarning` threshold (10). However, it slightly pollutes the listener list and could confuse future readers who expect exactly one handler.
A cleaner approach is to store and remove the temporary listener once the definitive one is installed:
```ts
const earlyHupGuard = () => {};
process.on("SIGHUP", earlyHupGuard);
setupSignalHandlers(); // installs the logging handler
process.removeListener("SIGHUP", earlyHupGuard);
```
Or, since the logging handler is equally inert during startup, simply skip the early guard entirely and let `setupSignalHandlers()` install the only listener — moving the SIGHUP registration to the top of that function achieves the same race-window protection.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Addressed in 903c94b93. Moved the early SIGHUP guard to the very first statement of main() (before any log() call — closes the startup race window the same way your earlier suggestion did) and removed the temporary guard after setupSignalHandlers() installs the definitive listener. Listener set is now clean: exactly one SIGHUP handler for the daemon's lifetime.
| process.on("SIGHUP", () => { | ||
| shutdownOnce({ | ||
| exitCode: 0, | ||
| message: "Received SIGHUP, shutting down...", | ||
| stopServerErrorMessage: "Error during stopServer in SIGHUP shutdown", | ||
| timeoutMessage: "Forced exit after SIGHUP shutdown timeout", | ||
| }); | ||
| log("info", "Received SIGHUP; ignoring (nohup semantics for daemon survival)"); | ||
| }); |
There was a problem hiding this comment.
log() call inside signal handler
If the daemon's stdout/stderr is still connected to Electron's pipe (i.e., spawned with stdio: 'pipe' or stdio: 'inherit' rather than stdio: 'ignore'), and Electron exits first, writing to that closed pipe will throw EPIPE. Because the log() call here is not wrapped in a try/catch, an EPIPE will propagate as an uncaught exception into the uncaughtException handler. EPIPE is not in TRANSIENT_ERROR_CODES, so shutdownOnce() would be called with exitCode: 1 — defeating the entire purpose of this no-op handler.
If stdio is already redirected to a file or /dev/null at spawn time, this is a non-issue. But it is worth adding a guard to make the handler unconditionally safe:
process.on("SIGHUP", () => {
try {
log("info", "Received SIGHUP; ignoring (nohup semantics for daemon survival)");
} catch {
// Suppress logging errors (e.g. EPIPE after parent exit) —
// the whole point of this handler is to survive without side-effects.
}
});Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/terminal-host/signal-handlers.ts
Line: 140-142
Comment:
**Unprotected `log()` call inside signal handler**
If the daemon's stdout/stderr is still connected to Electron's pipe (i.e., spawned with `stdio: 'pipe'` or `stdio: 'inherit'` rather than `stdio: 'ignore'`), and Electron exits first, writing to that closed pipe will throw `EPIPE`. Because the `log()` call here is not wrapped in a `try/catch`, an EPIPE will propagate as an uncaught exception into the `uncaughtException` handler. `EPIPE` is not in `TRANSIENT_ERROR_CODES`, so `shutdownOnce()` would be called with `exitCode: 1` — defeating the entire purpose of this no-op handler.
If stdio is already redirected to a file or `/dev/null` at spawn time, this is a non-issue. But it is worth adding a guard to make the handler unconditionally safe:
```ts
process.on("SIGHUP", () => {
try {
log("info", "Received SIGHUP; ignoring (nohup semantics for daemon survival)");
} catch {
// Suppress logging errors (e.g. EPIPE after parent exit) —
// the whole point of this handler is to survive without side-effects.
}
});
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Addressed in 903c94b93. Wrapped the log() call in try/catch. If the pipe closes first (e.g. Electron exits and our stdio was inherited), EPIPE from the write no longer propagates to uncaughtException → shutdownOnce, preserving the nohup semantics unconditionally.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/main/terminal-host/index.ts (1)
824-835:⚠️ Potential issue | 🟡 MinorMove the SIGHUP guard before startup logging.
The daemon is vulnerable to SIGHUP termination during the three
log()calls at lines 825–827, since the guard is installed only at line 835. Movingprocess.on("SIGHUP", () => {})to the first statement inmain()closes the race window completely.Proposed fix
async function main() { + // Belt-and-suspenders SIGHUP guard against the race window where the + // macOS login session tears down between process boot and + // setupSignalHandlers() registration. Without a registered listener, + // Node's default action for SIGHUP is to terminate the process — which + // would kill the daemon during Electron's exit before we can install our + // no-op handler in setupSignalHandlers(). + process.on("SIGHUP", () => {}); + log("info", "Terminal Host Daemon starting..."); log("info", `Environment: ${process.env.NODE_ENV || "production"}`); log("info", `Home directory: ${SUPERSET_HOME_DIR}`); - // Belt-and-suspenders SIGHUP guard against the race window where the - // macOS login session tears down between process boot and - // setupSignalHandlers() registration. Without a registered listener, - // Node's default action for SIGHUP is to terminate the process — which - // would kill the daemon during Electron's exit before we can install our - // no-op handler in setupSignalHandlers(). - process.on("SIGHUP", () => {}); - setupSignalHandlers();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/terminal-host/index.ts` around lines 824 - 835, In main(), move the SIGHUP guard (process.on("SIGHUP", () => {})) to be the very first statement inside the function, before any log("info", ...) calls so that the no-op handler is installed immediately and eliminates the race window; keep the handler as the same no-op and leave setupSignalHandlers() logic intact for later signal handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/terminal-host/signal-handlers.ts`:
- Around line 140-142: The SIGHUP handler currently calls log(...) which could
throw and bubble to uncaughtException, so change the process.on("SIGHUP", ...)
handler to protect the ignore semantics by wrapping the log call in a try/catch
(and swallow errors or at most fallback to console.error) so that any logging
failure does not rethrow; ensure the handler always returns without throwing.
Target the process.on("SIGHUP", ...) registration and the log(...) invocation
when applying this fix.
---
Outside diff comments:
In `@apps/desktop/src/main/terminal-host/index.ts`:
- Around line 824-835: In main(), move the SIGHUP guard (process.on("SIGHUP", ()
=> {})) to be the very first statement inside the function, before any
log("info", ...) calls so that the no-op handler is installed immediately and
eliminates the race window; keep the handler as the same no-op and leave
setupSignalHandlers() logic intact for later signal handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b682cfd3-e133-42ee-8704-386bae771189
📒 Files selected for processing (2)
apps/desktop/src/main/terminal-host/index.tsapps/desktop/src/main/terminal-host/signal-handlers.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 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/signal-handlers.ts">
<violation number="1" location="apps/desktop/src/main/terminal-host/signal-handlers.ts:141">
P1: Wrap the `log()` call in a try/catch. When SIGHUP fires because Electron quit, its stdio pipes are already closed — writing here throws EPIPE, which propagates to the `uncaughtException` handler and calls `shutdownOnce()`, defeating the purpose of this no-op handler.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Addresses review feedback from coderabbitai and greptile on superset-sh#3574: (1) Wrap the SIGHUP handler's log() call in try/catch. If the daemon was spawned with stdio:'pipe' and Electron exits first, writing to the now-closed pipe throws EPIPE; without the guard, that flows into uncaughtException → shutdownOnce() with exit 1, defeating the entire purpose of the nohup-semantics no-op. (2) Move the early SIGHUP guard to the very first statement of main() so it is installed before the log() calls that follow, closing the startup race window. Then remove the temporary guard after setupSignalHandlers() runs — process.on adds rather than replaces, so leaving both in place would permanently register two SIGHUP listeners.
|
Addressed review feedback in the latest push:
Typecheck passes, 37 terminal-host tests green. |
|
Friendly ping @Kitenite — small single-file PR (~6 net lines) that prevents the terminal-host daemon from dying on Cmd+Q. All bot reviews (greptile, cubic-dev-ai, coderabbit) have been addressed; the only concrete change request — a try/catch guard around the SIGHUP handler's log call — is applied. Verification on macOS 25.3.0 (2026-04-19):
Targets the root cause behind #2501 ("Session terminals disappearing"). Risk is low: swallowing SIGHUP (nohup semantics) is a standard daemon pattern; intentional shutdown still goes through If you have a moment to take a look, much appreciated. PR #3573 and #3575 build on this one. |
Setting SIGHUP to a no-op (nohup semantics) so the detached daemon actually survives `app.exit(0)` on macOS. The marketing blog claims "the terminal survives app restart" via `detached: true` + `unref()`, but in practice the daemon was being killed on every Cmd+Q — log showed "Forced exit after SIGHUP shutdown timeout" within seconds of quit. Root cause: `setsid()` isolates the Unix SID but the daemon still shares the macOS Mach bootstrap / login Security Session with its parent. When Electron tears down that login session, SIGHUP propagates to the daemon and our handler called shutdownOnce(), killing every active PTY along with it. Fix: replace the SIGHUP shutdown handler with a logging no-op (signal-handlers.ts) and register a defensive no-op in main() before setupSignalHandlers() runs (index.ts) to close the race window during daemon boot. Refs superset-sh#2501
Addresses review feedback from coderabbitai and greptile on superset-sh#3574: (1) Wrap the SIGHUP handler's log() call in try/catch. If the daemon was spawned with stdio:'pipe' and Electron exits first, writing to the now-closed pipe throws EPIPE; without the guard, that flows into uncaughtException → shutdownOnce() with exit 1, defeating the entire purpose of the nohup-semantics no-op. (2) Move the early SIGHUP guard to the very first statement of main() so it is installed before the log() calls that follow, closing the startup race window. Then remove the temporary guard after setupSignalHandlers() runs — process.on adds rather than replaces, so leaving both in place would permanently register two SIGHUP listeners.
Haknt
force-pushed
the
fix/daemon-survives-app-quit
branch
from
903c94b to
920432c
Compare
1 participant
Summary
The terminal-host daemon is spawned
detached: true+unref()so it can outlive the Electron main process, which is the mechanism behind the "Terminal That (Almost) Never Dies" promise in the terminal-daemon-deep-dive blog post. In practice, on macOS, Cmd+Q still tore the daemon down along with every active terminal session.Root cause is a macOS-specific signal propagation path.
setsid()(implicit from Node'sdetached: true) isolates the daemon's Unix SID, but the daemon still shares the Mach bootstrap / login Security Session with its parent. When Electron'sapp.exit(0)tears down that login session, macOS propagates SIGHUP to the daemon — and the existing signal handler calledshutdownOnce(), killing every PTY subprocess with it.Fix: register a SIGHUP listener that is a logging no-op (nohup semantics). The daemon now survives a Cmd+Q quit. Intentional daemon shutdown still goes through
killDaemonFromPidFile()(SIGKILL, which this handler cannot intercept) and the explicitshutdownRPC path.Fixes #2501.
Test plan
bun run typecheckcleanpgrep -f terminal-host.jsstill shows the same daemon PID, relaunch, verify sessions hot-reattach withSession ... attachedlog line (notcreated).Notes
Summary by cubic
Keeps the terminal-host daemon alive after Cmd+Q on macOS by ignoring SIGHUP, so terminal sessions persist across app restarts. Fixes #2501.
shutdownOnce; wrap the log in try/catch to avoid EPIPE if stdio is closed.setupSignalHandlers()to close the race and avoid duplicate listeners; intentional shutdown paths remain unchanged.Written for commit 920432c. Summary will update on new commits.
Summary by CodeRabbit