Repository navigation
fix(host-service): never kill PTYs during daemon adoption, and say when the daemon goes quiet - #5721
Merged
Merged
Conversation
Updating killed users' terminals (SUPER-833). Terminals are meant to survive an update: the pty-daemon is detached and the relaunched host-service adopts it. But tryAdopt treated transient adoption uncertainty as grounds to destroy the daemon -- socket not connectable within 1s, or hello probe unanswered within 3s, both called terminateProcessTreeAndGroups, which signals the daemon's entire process tree and groups, i.e. every live user shell. Adoption is driven eagerly at host-service boot (the terminal reaper's first pass calls getDaemonClient -> waitForDaemonReady -> ensure -> tryAdopt), so those short timeouts run during the post-update relaunch load spike -- exactly when a healthy-but-busy daemon misses them. Probe a pid-alive daemon patiently across ADOPTION_WEDGE_GRACE_MS (15s) instead. Adopt as soon as it answers; if it exits on its own, fall through to spawn; only force-respawn once it has stayed silent for the whole window. This restores the invariant already stated in expected-version.ts: sessions live in the daemon, we do not auto-kill. The wedged-daemon recovery escape hatch is preserved, just no longer trigger-happy.
|
Ready to review this PR? Stage has broken it down into 6 individual chapters for you: 
Chapters generated by Stage for commit eaf6669 on Jul 17, 2026 1:03am UTC. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesDaemon health and recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TerminalConnectionIndicator
participant TerminalRouter
participant DaemonSupervisor
participant PTYDaemon
TerminalConnectionIndicator->>TerminalRouter: Query daemon health
TerminalRouter->>DaemonSupervisor: Get organization daemon health
DaemonSupervisor->>PTYDaemon: Probe reachability
PTYDaemon-->>DaemonSupervisor: Return response or silence
DaemonSupervisor-->>TerminalConnectionIndicator: Return health state
TerminalConnectionIndicator->>TerminalRouter: Confirm daemon restart
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Contributor
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
Contributor
Greptile SummaryThis PR fixes SUPER-833 by replacing the trigger-happy 1s socket-connect + 3s probe (with force-kill on any timeout) in
Confidence Score: 4/5Safe to merge. The change narrows a destructive action to a tighter condition — the only way it behaves worse than before is that recovery from a daemon that crashes mid-probe takes up to 15s instead of under 1s. The core fix is straightforward and well-tested: The retry loop in
|
| Filename | Overview |
|---|---|
| packages/host-service/src/daemon/DaemonSupervisor.ts | Core fix: tryAdopt's pid-alive path replaced with a patient 15s retry loop, adopting immediately on first probe success and only force-killing after the full grace window expires. One subtle behaviour: if the daemon dies mid-loop the code spins on a dead socket for the remainder of the grace window before the post-loop liveness check short-circuits to tryAdoptFromSocket. |
| packages/host-service/src/daemon/DaemonSupervisor.test.ts | Adds a well-constructed regression guard for SUPER-833 (healthy daemon adopted without termination) and updates the wedged-recovery test to use a short adoptionWedgeGraceMs so CI is not gated on a 15s wait. Both tests include cleanup and appropriate liveness assertions. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[tryAdopt called] --> B{Manifest exists?}
B -- No --> C[tryAdoptFromSocket\nreason: manifest_missing]
B -- Yes --> D{isProcessAlive\nmanifest.pid?}
D -- No --> E[removePtyDaemonManifest]
E --> F[tryAdoptFromSocket\nreason: manifest_pid_dead]
D -- Yes --> G[probeDaemonHelloWithRetry\nup to graceMs / 15s\nreturns immediately on success]
G -- probe succeeds --> H[Return DaemonInstance\nadopted, shells survive]
G -- probe returns null --> I{isProcessAlive\nmanifest.pid?}
I -- No / died during probe --> J[removePtyDaemonManifest]
J --> K[tryAdoptFromSocket\nreason: manifest_pid_dead]
I -- Yes / truly wedged --> L[logEvent pty_daemon_adopt_rejected\nreason: probe_failed_after_grace]
L --> M[terminateProcessTreeAndGroups\nSIGTERM SIGKILL]
M --> N[removePtyDaemonManifest]
N --> O{Socket path changed?}
O -- Yes --> P[tryAdoptFromSocket\nreason: manifest_probe_failed_after_grace]
O -- No --> Q[return null spawn fresh daemon]
style G fill:#d4edda,stroke:#28a745
style H fill:#d4edda,stroke:#28a745
style M fill:#f8d7da,stroke:#dc3545
style L fill:#fff3cd,stroke:#ffc107
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[tryAdopt called] --> B{Manifest exists?}
B -- No --> C[tryAdoptFromSocket\nreason: manifest_missing]
B -- Yes --> D{isProcessAlive\nmanifest.pid?}
D -- No --> E[removePtyDaemonManifest]
E --> F[tryAdoptFromSocket\nreason: manifest_pid_dead]
D -- Yes --> G[probeDaemonHelloWithRetry\nup to graceMs / 15s\nreturns immediately on success]
G -- probe succeeds --> H[Return DaemonInstance\nadopted, shells survive]
G -- probe returns null --> I{isProcessAlive\nmanifest.pid?}
I -- No / died during probe --> J[removePtyDaemonManifest]
J --> K[tryAdoptFromSocket\nreason: manifest_pid_dead]
I -- Yes / truly wedged --> L[logEvent pty_daemon_adopt_rejected\nreason: probe_failed_after_grace]
L --> M[terminateProcessTreeAndGroups\nSIGTERM SIGKILL]
M --> N[removePtyDaemonManifest]
N --> O{Socket path changed?}
O -- Yes --> P[tryAdoptFromSocket\nreason: manifest_probe_failed_after_grace]
O -- No --> Q[return null spawn fresh daemon]
style G fill:#d4edda,stroke:#28a745
style H fill:#d4edda,stroke:#28a745
style M fill:#f8d7da,stroke:#dc3545
style L fill:#fff3cd,stroke:#ffc107
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
packages/host-service/src/daemon/DaemonSupervisor.ts:836-843
When the daemon exits naturally during the grace window the code falls through to `tryAdoptFromSocket` with `reason: "manifest_pid_dead"` — the same string used for the pre-probe dead-pid check at the top of `tryAdopt`. In logs these two paths are indistinguishable, making it hard to determine whether adoption was retried for ~0ms or up to 15s before detecting the dead pid.
```suggestion
// Daemon exited on its own during the grace window — nothing to kill;
// fall through to a socket adopt / fresh spawn.
if (!isProcessAlive(manifest.pid)) {
removePtyDaemonManifest(organizationId);
return this.tryAdoptFromSocket(organizationId, expectedSocketPath, {
reason: "manifest_pid_died_during_probe",
});
}
```
### Issue 2 of 2
packages/host-service/src/daemon/DaemonSupervisor.ts:820-844
**Daemon-exit mid-probe spins the full grace window.** `probeDaemonHelloWithRetry` never checks `isProcessAlive` inside its retry loop, so if the daemon crashes at t=1s of a 15s window, the loop keeps issuing connect attempts against a dead socket (each returning near-instantly with ECONNREFUSED) for the remaining ~14s before the post-loop `!isProcessAlive` check triggers `tryAdoptFromSocket`. In the previous code the equivalent path (socket unreachable → pid check) resolved in under 1s. Adding an `isProcessAlive(manifest.pid)` guard into the retry loop — either via an optional `abortIf` callback on `probeDaemonHelloWithRetry` or as an inline check between attempts in `tryAdopt` — would short-circuit the wait and restore fast recovery for the crash scenario without any risk to the healthy-daemon fix.
Reviews (1): Last reviewed commit: "fix(host-service): never force-kill a li..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
836
to
843
| // Daemon exited on its own during the grace window — nothing to kill; | ||
| // fall through to a socket adopt / fresh spawn. | ||
| if (!isProcessAlive(manifest.pid)) { | ||
| removePtyDaemonManifest(organizationId); | ||
| if (manifest.socketPath !== expectedSocketPath) { | ||
| return this.tryAdoptFromSocket(organizationId, expectedSocketPath, { | ||
| reason: "manifest_version_probe_failed", | ||
| previousManifest: manifest, | ||
| }); | ||
| } | ||
| return null; | ||
| return this.tryAdoptFromSocket(organizationId, expectedSocketPath, { | ||
| reason: "manifest_pid_dead", | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
tryAdoptFromSocket with reason: "manifest_pid_dead" — the same string used for the pre-probe dead-pid check at the top of tryAdopt. In logs these two paths are indistinguishable, making it hard to determine whether adoption was retried for ~0ms or up to 15s before detecting the dead pid.
Suggested change
| // Daemon exited on its own during the grace window — nothing to kill; | |
| // fall through to a socket adopt / fresh spawn. | |
| if (!isProcessAlive(manifest.pid)) { | |
| removePtyDaemonManifest(organizationId); | |
| if (manifest.socketPath !== expectedSocketPath) { | |
| return this.tryAdoptFromSocket(organizationId, expectedSocketPath, { | |
| reason: "manifest_version_probe_failed", | |
| previousManifest: manifest, | |
| }); | |
| } | |
| return null; | |
| return this.tryAdoptFromSocket(organizationId, expectedSocketPath, { | |
| reason: "manifest_pid_dead", | |
| }); | |
| } | |
| // Daemon exited on its own during the grace window — nothing to kill; | |
| // fall through to a socket adopt / fresh spawn. | |
| if (!isProcessAlive(manifest.pid)) { | |
| removePtyDaemonManifest(organizationId); | |
| return this.tryAdoptFromSocket(organizationId, expectedSocketPath, { | |
| reason: "manifest_pid_died_during_probe", | |
| }); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/daemon/DaemonSupervisor.ts
Line: 836-843
Comment:
When the daemon exits naturally during the grace window the code falls through to `tryAdoptFromSocket` with `reason: "manifest_pid_dead"` — the same string used for the pre-probe dead-pid check at the top of `tryAdopt`. In logs these two paths are indistinguishable, making it hard to determine whether adoption was retried for ~0ms or up to 15s before detecting the dead pid.
```suggestion
// Daemon exited on its own during the grace window — nothing to kill;
// fall through to a socket adopt / fresh spawn.
if (!isProcessAlive(manifest.pid)) {
removePtyDaemonManifest(organizationId);
return this.tryAdoptFromSocket(organizationId, expectedSocketPath, {
reason: "manifest_pid_died_during_probe",
});
}
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
820
to
844
| const probe = await probeDaemonHelloWithRetry(manifest.socketPath, graceMs); | ||
| if (probe) { | ||
| const runningVersion = probe.daemonVersion; | ||
| return { | ||
| pid: manifest.pid, | ||
| socketPath: manifest.socketPath, | ||
| reason: "version_probe_failed", | ||
| }); | ||
| await terminateProcessTreeAndGroups(manifest.pid, "SIGTERM"); | ||
| startedAt: manifest.startedAt, | ||
| runningVersion, | ||
| expectedVersion: EXPECTED_DAEMON_VERSION, | ||
| updatePending: !semver.satisfies( | ||
| runningVersion, | ||
| `>=${EXPECTED_DAEMON_VERSION}`, | ||
| ), | ||
| }; | ||
| } | ||
|
|
||
| // Daemon exited on its own during the grace window — nothing to kill; | ||
| // fall through to a socket adopt / fresh spawn. | ||
| if (!isProcessAlive(manifest.pid)) { | ||
| removePtyDaemonManifest(organizationId); | ||
| if (manifest.socketPath !== expectedSocketPath) { | ||
| return this.tryAdoptFromSocket(organizationId, expectedSocketPath, { | ||
| reason: "manifest_version_probe_failed", | ||
| previousManifest: manifest, | ||
| }); | ||
| } | ||
| return null; | ||
| return this.tryAdoptFromSocket(organizationId, expectedSocketPath, { | ||
| reason: "manifest_pid_dead", | ||
| }); | ||
| } | ||
| const runningVersion = probe.daemonVersion; | ||
| const updatePending = !semver.satisfies( | ||
| runningVersion, | ||
| `>=${EXPECTED_DAEMON_VERSION}`, | ||
| ); | ||
|
|
Contributor
There was a problem hiding this comment.
probeDaemonHelloWithRetry never checks isProcessAlive inside its retry loop, so if the daemon crashes at t=1s of a 15s window, the loop keeps issuing connect attempts against a dead socket (each returning near-instantly with ECONNREFUSED) for the remaining ~14s before the post-loop !isProcessAlive check triggers tryAdoptFromSocket. In the previous code the equivalent path (socket unreachable → pid check) resolved in under 1s. Adding an isProcessAlive(manifest.pid) guard into the retry loop — either via an optional abortIf callback on probeDaemonHelloWithRetry or as an inline check between attempts in tryAdopt — would short-circuit the wait and restore fast recovery for the crash scenario without any risk to the healthy-daemon fix.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/daemon/DaemonSupervisor.ts
Line: 820-844
Comment:
**Daemon-exit mid-probe spins the full grace window.** `probeDaemonHelloWithRetry` never checks `isProcessAlive` inside its retry loop, so if the daemon crashes at t=1s of a 15s window, the loop keeps issuing connect attempts against a dead socket (each returning near-instantly with ECONNREFUSED) for the remaining ~14s before the post-loop `!isProcessAlive` check triggers `tryAdoptFromSocket`. In the previous code the equivalent path (socket unreachable → pid check) resolved in under 1s. Adding an `isProcessAlive(manifest.pid)` guard into the retry loop — either via an optional `abortIf` callback on `probeDaemonHelloWithRetry` or as an inline check between attempts in `tryAdopt` — would short-circuit the wait and restore fast recovery for the crash scenario without any risk to the healthy-daemon fix.
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/host-service/src/daemon/DaemonSupervisor.test.ts`:
- Around line 355-400: The adoption test using startFakeDaemon and
invokeTryAdopt currently responds immediately, so it does not exercise retries.
Configure the fake daemon to ignore the first several hello requests, then
respond before adoptionWedgeGraceMs expires; retain the assertions that the
daemon is adopted, remains alive, and produces no pty_daemon_adopt_rejected
event.
In `@packages/host-service/src/daemon/DaemonSupervisor.ts`:
- Around line 820-833: Update the adoption logic around
probeDaemonHelloWithRetry so a successful probe is accepted only when
probe.daemonPid matches the live manifest.pid; otherwise reject the probe rather
than binding the socket to the manifest entry, unless the implementation
explicitly reconciles and persists the reported PID before returning the adopted
daemon state.
- Around line 819-843: The manifest probe path around probeDaemonHelloWithRetry
must stop waiting when manifest.pid exits. Update the retry flow to check
isProcessAlive(manifest.pid) between attempts and return a distinct exited
outcome, then handle that outcome in the surrounding DaemonSupervisor logic by
removing the manifest and proceeding to socket adoption or fresh spawning
without consuming the remaining grace window.
🪄 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: b1b70e0d-1b3c-4c75-8cc5-a70c974fbfb3
📒 Files selected for processing (2)
packages/host-service/src/daemon/DaemonSupervisor.test.tspackages/host-service/src/daemon/DaemonSupervisor.ts
Comment on lines
+820
to
+833
| const probe = await probeDaemonHelloWithRetry(manifest.socketPath, graceMs); | ||
| if (probe) { | ||
| const runningVersion = probe.daemonVersion; | ||
| return { | ||
| pid: manifest.pid, | ||
| socketPath: manifest.socketPath, | ||
| reason: "version_probe_failed", | ||
| }); | ||
| await terminateProcessTreeAndGroups(manifest.pid, "SIGTERM"); | ||
| startedAt: manifest.startedAt, | ||
| runningVersion, | ||
| expectedVersion: EXPECTED_DAEMON_VERSION, | ||
| updatePending: !semver.satisfies( | ||
| runningVersion, | ||
| `>=${EXPECTED_DAEMON_VERSION}`, | ||
| ), | ||
| }; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate the hello-reported PID before adopting.
A successful socket probe is accepted while probe.daemonPid is ignored. If the manifest PID is stale or reused, the supervisor can bind the socket to the wrong PID and later terminate that unrelated process. Require the reported PID to match the live manifest PID, or explicitly reconcile and persist the reported PID before adoption.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/host-service/src/daemon/DaemonSupervisor.ts` around lines 820 - 833,
Update the adoption logic around probeDaemonHelloWithRetry so a successful probe
is accepted only when probe.daemonPid matches the live manifest.pid; otherwise
reject the probe rather than binding the socket to the manifest entry, unless
the implementation explicitly reconciles and persists the reported PID before
returning the adopted daemon state.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
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="packages/host-service/src/daemon/DaemonSupervisor.test.ts">
<violation number="1" location="packages/host-service/src/daemon/DaemonSupervisor.test.ts:368">
P3: This regression test uses a daemon that answers immediately, so it doesn’t actually exercise the new retry-within-grace behavior. Simulating initial hello failures followed by success within `adoptionWedgeGraceMs` would better protect the intended adoption retry path.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Super as DaemonSupervisor (tryAdopt)
participant Manifest as ManifestStore (FS)
participant Daemon as Running pty-daemon (pid-alive)
participant Probe as probeDaemonHelloWithRetry
participant AliveCheck as isProcessAlive
participant Kill as terminateProcessTreeAndGroups
Note over Super,Kill: Adoption Flow (pid-alive daemon) – NEW patience logic
Super->>Manifest: Read manifest (pid, socketPath, ...)
Manifest-->>Super: manifest
Note over Super: pid is alive (verified earlier)
Super->>Probe: probeDaemonHelloWithRetry(socketPath, adoptionWedgeGraceMs)
Note right of Probe: Retries across grace window (default 15s),<br/>adopts as soon as daemon responds
alt Daemon answers probe before timeout
Probe-->>Super: { daemonVersion, ... }
Super->>Super: Build adopted daemon record
Super-->>Super: Return adopted (pid, runningVersion, ...)
Note over Super: No force-kill, shell survives
else Probe returns null (timeout / socket dead)
Probe-->>Super: null
Super->>AliveCheck: isProcessAlive(manifest.pid)
alt Daemon exited gracefully during grace
AliveCheck-->>Super: false (dead)
Super->>Manifest: removePtyDaemonManifest()
Super->>Super: tryAdoptFromSocket(…)
Note over Super: Fallback to socket adoption / fresh spawn
else Daemon still alive (genuinely wedged)
AliveCheck-->>Super: true (still running)
Super->>Super: logEvent("pty_daemon_adopt_rejected", reason="probe_failed_after_grace")
Super->>Kill: terminateProcessTreeAndGroups(pid, SIGTERM)
Kill->>Daemon: Force‑kill daemon process tree (destructive)
Super->>Manifest: removePtyDaemonManifest()
alt manifest.socketPath !== expectedSocketPath
Super->>Super: tryAdoptFromSocket(…, reason="manifest_probe_failed_after_grace")
else
Super-->>Super: return null (spawn fresh daemon)
end
end
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if (!childPid) return; | ||
| const orgId = "org-healthy-adopt"; | ||
| const fake = await startFakeDaemon({ | ||
| respondWithVersion: "0.1.0", |
Contributor
There was a problem hiding this comment.
P3: This regression test uses a daemon that answers immediately, so it doesn’t actually exercise the new retry-within-grace behavior. Simulating initial hello failures followed by success within adoptionWedgeGraceMs would better protect the intended adoption retry path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/host-service/src/daemon/DaemonSupervisor.test.ts, line 368:
<comment>This regression test uses a daemon that answers immediately, so it doesn’t actually exercise the new retry-within-grace behavior. Simulating initial hello failures followed by success within `adoptionWedgeGraceMs` would better protect the intended adoption retry path.</comment>
<file context>
@@ -346,6 +351,70 @@ describe("DaemonSupervisor.tryAdopt", () => {
+ if (!childPid) return;
+ const orgId = "org-healthy-adopt";
+ const fake = await startFakeDaemon({
+ respondWithVersion: "0.1.0",
+ daemonPid: childPid,
+ });
</file context>
Follow-up on the previous commit's approach: a grace window only delayed the kill, it didn't remove it -- a daemon that stayed unreachable for the whole window still had its process tree SIGKILLed, taking the user's shells with it. Delete the destructive path from tryAdopt entirely. If the pid is alive it still owns live PTYs, so adopt it: a failed probe means we couldn't read its version, not that it's disposable. Adopt unprobed with runningVersion "unknown" and updatePending false -- the state DaemonInstance already documents for a failed probe, which is what tryAdopt should have been doing all along instead of killing. Recovering a genuinely wedged daemon stays an explicit user action (Settings > Restart daemon), matching the invariant in expected-version.ts: sessions live in the daemon; the user explicitly triggers restart. Drops ADOPTION_WEDGE_GRACE_MS and the adoptionWedgeGraceMs test option -- with no kill to defer, there's nothing to be patient about, and the probe returns to its plain 3s budget.
Kitenite
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/host-service/src/daemon/DaemonSupervisor.test.ts (1)
338-338: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRemove the guaranteed 1.5-second wall-clock wait.
When adoption works,
waitForProcessExitalways consumes the full timeout. The following liveness assertion already verifies that the awaitedtryAdoptcall did not terminate the process.Proposed fix
- expect(await waitForProcessExit(childPid, 1500)).toBe(false); expect(isProcessAliveForTest(childPid)).toBe(true);Based on learnings, host-service tests should avoid real wall-clock timers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/host-service/src/daemon/DaemonSupervisor.test.ts` at line 338, Remove the waitForProcessExit assertion using the 1500ms timeout from the adoption test. Rely on the existing liveness assertion after the awaited tryAdopt call to verify the process remains running, and avoid introducing any real-time delay or replacement timer.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/host-service/src/daemon/DaemonSupervisor.ts`:
- Around line 802-805: Update the unprobed-adoption flow around
probeDaemonHelloWithRetry to re-check the manifest PID’s liveness after a failed
probe and before ensure() returns. If the PID is no longer alive, follow the
existing dead-manifest recovery path; otherwise preserve the current unprobed
adoption behavior.
- Around line 824-830: Update the tryAdoptFromSocket call in the manifest
socket-path recovery flow to pass manifest.pid as the expected PID. Ensure
tryAdoptFromSocket validates the socket-reported live manifest PID against that
expected value and rejects mismatches before writing or adopting the recovered
manifest, preserving the original daemon as unprobed.
---
Nitpick comments:
In `@packages/host-service/src/daemon/DaemonSupervisor.test.ts`:
- Line 338: Remove the waitForProcessExit assertion using the 1500ms timeout
from the adoption test. Rely on the existing liveness assertion after the
awaited tryAdopt call to verify the process remains running, and avoid
introducing any real-time delay or replacement timer.
🪄 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: 39de1fa3-9f6d-4dfc-9c5f-786965d073a1
📒 Files selected for processing (2)
packages/host-service/src/daemon/DaemonSupervisor.test.tspackages/host-service/src/daemon/DaemonSupervisor.ts
Comment on lines
+824
to
+830
| if (manifest.socketPath !== expectedSocketPath) { | ||
| const viaSocket = await this.tryAdoptFromSocket( | ||
| organizationId, | ||
| expectedSocketPath, | ||
| { reason: "manifest_probe_failed", previousManifest: manifest }, | ||
| ); | ||
| if (viaSocket) return viaSocket; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Only accept the expected socket when it reports the live manifest PID.
tryAdoptFromSocket can adopt another process and overwrite the manifest while the original PID—and its PTYs—remain alive. Pass manifest.pid as the expected PID and reject mismatches before writing the recovered manifest; otherwise preserve the original daemon as unprobed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/host-service/src/daemon/DaemonSupervisor.ts` around lines 824 - 830,
Update the tryAdoptFromSocket call in the manifest socket-path recovery flow to
pass manifest.pid as the expected PID. Ensure tryAdoptFromSocket validates the
socket-reported live manifest PID against that expected value and rejects
mismatches before writing or adopting the recovered manifest, preserving the
original daemon as unprobed.
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
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="packages/host-service/src/daemon/DaemonSupervisor.ts">
<violation number="1" location="packages/host-service/src/daemon/DaemonSupervisor.ts:825">
P1: This fallback should verify that socket adoption corresponds to the current manifest PID before accepting it. Without a PID match guard here, `tryAdoptFromSocket` can adopt a different daemon on `expectedSocketPath` while the original PID is still alive, which risks rewriting manifest ownership away from the daemon that still owns active PTYs.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // scheme changed under us, a daemon may be answering on the expected | ||
| // path — prefer that one. | ||
| if (manifest.socketPath !== expectedSocketPath) { | ||
| const viaSocket = await this.tryAdoptFromSocket( |
Contributor
There was a problem hiding this comment.
P1: This fallback should verify that socket adoption corresponds to the current manifest PID before accepting it. Without a PID match guard here, tryAdoptFromSocket can adopt a different daemon on expectedSocketPath while the original PID is still alive, which risks rewriting manifest ownership away from the daemon that still owns active PTYs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/host-service/src/daemon/DaemonSupervisor.ts, line 825:
<comment>This fallback should verify that socket adoption corresponds to the current manifest PID before accepting it. Without a PID match guard here, `tryAdoptFromSocket` can adopt a different daemon on `expectedSocketPath` while the original PID is still alive, which risks rewriting manifest ownership away from the daemon that still owns active PTYs.</comment>
<file context>
@@ -833,34 +818,35 @@ export class DaemonSupervisor {
+ // scheme changed under us, a daemon may be answering on the expected
+ // path — prefer that one.
+ if (manifest.socketPath !== expectedSocketPath) {
+ const viaSocket = await this.tryAdoptFromSocket(
+ organizationId,
+ expectedSocketPath,
</file context>
Collapse the rest of the branching now that adoption never kills. A live pid owns live PTYs, so tryAdopt has exactly one job: read the version if it can, and return the instance either way. Removes, on top of the kill itself: - the socket-path-mismatch fallback, which only ever ran as post-kill recovery (production socket paths are a pure function of orgId, so they can't drift) - the adopt_unprobed event -- start() already logs the adoption with runningVersion, so "unknown" is visible without a second event - previousManifest plumbing on tryAdoptFromSocket, dead once the kill branches that passed it were gone Net effect on the file: 10 insertions, 43 deletions.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/host-service/src/daemon/DaemonSupervisor.ts (1)
796-814: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDead PID during the probe still gets adopted as “alive.”
manifest.pidis checked once at line 790, but the probe can take up toADOPTION_PROBE_TOTAL_TIMEOUT_MSto resolve. If the process exits during that window, this still returns an adopted instance for a dead pid/socket instead of falling through to themanifest_pid_deadrecovery path (removeManifest +tryAdoptFromSocket).ensure()then hands callers a broken instance until the liveness poller eventually clears it.Proposed fix
const probe = await probeDaemonHelloWithRetry( manifest.socketPath, ADOPTION_PROBE_TOTAL_TIMEOUT_MS, ); + if (!probe && !isProcessAlive(manifest.pid)) { + removePtyDaemonManifest(organizationId); + return this.tryAdoptFromSocket(organizationId, expectedSocketPath, { + reason: "manifest_pid_died_during_probe", + }); + } const runningVersion = probe?.daemonVersion ?? "unknown";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/host-service/src/daemon/DaemonSupervisor.ts` around lines 796 - 814, Revalidate the daemon PID after probeDaemonHelloWithRetry completes in the adoption flow, before returning the adopted instance. If manifest.pid is no longer alive, do not return this result; fall through to the existing manifest_pid_dead recovery path that removes the manifest and calls tryAdoptFromSocket, while preserving the current probe/version handling for still-alive processes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/host-service/src/daemon/DaemonSupervisor.ts`:
- Around line 804-813: Validate that probe.daemonPid matches manifest.pid before
accepting the probe in the tryAdoptFromSocket flow. Reject or ignore mismatched
probes so the returned instance identity, version, and subsequent lifecycle
operations remain tied to the manifest process; preserve the existing handling
for matching probes.
---
Duplicate comments:
In `@packages/host-service/src/daemon/DaemonSupervisor.ts`:
- Around line 796-814: Revalidate the daemon PID after probeDaemonHelloWithRetry
completes in the adoption flow, before returning the adopted instance. If
manifest.pid is no longer alive, do not return this result; fall through to the
existing manifest_pid_dead recovery path that removes the manifest and calls
tryAdoptFromSocket, while preserving the current probe/version handling for
still-alive processes.
🪄 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: 147dcd9b-d692-46e6-a978-8b200fb14495
📒 Files selected for processing (2)
packages/host-service/src/daemon/DaemonSupervisor.test.tspackages/host-service/src/daemon/DaemonSupervisor.ts
💤 Files with no reviewable changes (1)
- packages/host-service/src/daemon/DaemonSupervisor.test.ts
Comment on lines
+804
to
+813
| const runningVersion = probe?.daemonVersion ?? "unknown"; | ||
| return { | ||
| pid: manifest.pid, | ||
| socketPath: manifest.socketPath, | ||
| startedAt: manifest.startedAt, | ||
| runningVersion, | ||
| expectedVersion: EXPECTED_DAEMON_VERSION, | ||
| updatePending, | ||
| updatePending: | ||
| probe != null && | ||
| !semver.satisfies(runningVersion, `>=${EXPECTED_DAEMON_VERSION}`), |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
probe.daemonPid is still not validated against manifest.pid.
A successful probe is accepted purely on manifest.socketPath responding; probe.daemonPid (used elsewhere in tryAdoptFromSocket, e.g. line 839-840) is never compared to manifest.pid here. If the pid was reused or the socket is stale, this can bind the returned instance's identity/version to a process that isn't actually the one at manifest.pid, and later lifecycle operations (update handoff, liveness polling, termination) act on the wrong assumption.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/host-service/src/daemon/DaemonSupervisor.ts` around lines 804 - 813,
Validate that probe.daemonPid matches manifest.pid before accepting the probe in
the tryAdoptFromSocket flow. Reject or ignore mismatched probes so the returned
instance identity, version, and subsequent lifecycle operations remain tied to
the manifest process; preserve the existing handling for matching probes.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/host-service/src/daemon/DaemonSupervisor.ts`:
- Around line 575-600: Update refreshReachability so that when a successful
probe backfills runningVersion and sets updatePending, it also invokes the
existing maybeFireUpdatePending and kickoffAutoUpdate flows used by tryAdopt,
tryAdoptFromSocket, and runUpdate. Preserve the current instance revalidation
and only trigger these actions when the version was previously "unknown" and is
determined to require an update.
🪄 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: ce30df2a-634b-464c-adfe-a94213d578d0
📒 Files selected for processing (5)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalPaneHeaderExtras/components/TerminalConnectionIndicator/TerminalConnectionIndicator.tsxpackages/host-service/src/daemon/DaemonSupervisor.test.tspackages/host-service/src/daemon/DaemonSupervisor.tspackages/host-service/src/trpc/router/terminal/terminal.tsturbo.jsonc
Contributor
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
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/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalPaneHeaderExtras/components/TerminalConnectionIndicator/TerminalConnectionIndicator.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalPaneHeaderExtras/components/TerminalConnectionIndicator/TerminalConnectionIndicator.tsx:111">
P2: Opening several terminal panes sends repeated daemon-health polls rather than the documented single poll per workspace. Move this polling query into a workspace-scoped provider/store and let pane indicators consume its shared result, avoiding host-service traffic proportional to open panes.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| // session closes nothing, so the socket stays "open" and the shell just | ||
| // freezes. The query takes no input, so React Query collapses every pane's | ||
| // copy into one request per workspace. | ||
| const healthQuery = workspaceTrpc.terminal.daemon.getHealth.useQuery( |
Contributor
There was a problem hiding this comment.
P2: Opening several terminal panes sends repeated daemon-health polls rather than the documented single poll per workspace. Move this polling query into a workspace-scoped provider/store and let pane indicators consume its shared result, avoiding host-service traffic proportional to open panes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalPaneHeaderExtras/components/TerminalConnectionIndicator/TerminalConnectionIndicator.tsx, line 111:
<comment>Opening several terminal panes sends repeated daemon-health polls rather than the documented single poll per workspace. Move this polling query into a workspace-scoped provider/store and let pane indicators consume its shared result, avoiding host-service traffic proportional to open panes.</comment>
<file context>
@@ -71,19 +100,61 @@ export function TerminalConnectionIndicator({
+ // session closes nothing, so the socket stays "open" and the shell just
+ // freezes. The query takes no input, so React Query collapses every pane's
+ // copy into one request per workspace.
+ const healthQuery = workspaceTrpc.terminal.daemon.getHealth.useQuery(
+ undefined,
+ { refetchInterval: DAEMON_HEALTH_POLL_MS },
</file context>
… auto-update - tryAdopt drops a stale manifest whose socket has no listener instead of adopting a recycled pid as a phantom daemon (which would wedge terminals and aim a later restart at an unrelated process); falls through to socket adoption or a fresh spawn - refreshReachability now fires update-pending + kicks off auto-update when a late probe reveals a stale version, so a daemon adopted under post-update load still self-updates - probe the socket on a slower 5s cadence with retry (was one 1.5s attempt every 2s) so a busy daemon streaming output isn't falsely flagged unreachable - extract isVersionUpdatePending helper; rename adoptedLivenessTimers -> healthPollTimers to match the generalized poll
- distinct side-by-side actions: solid Reconnect + outline Restart (red only on hover); keep the non-destructive Reconnect available whenever the transport is unhealthy, so a stale daemon-health flag never leaves restart as the only offer after sleep/wake - plain-language copy, no "daemon" jargon anywhere user-facing; "Restart all terminals?" confirm with a "Keep waiting" cancel - amber for recoverable states, red reserved for the destructive path - collapsible log expands the popover to a readable full-width layout with the timestamp as a header line instead of a narrow left gutter
Contributor
There was a problem hiding this comment.
2 issues found across 3 files (changes from recent commits).
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/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalPaneHeaderExtras/components/TerminalConnectionIndicator/TerminalConnectionIndicator.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalPaneHeaderExtras/components/TerminalConnectionIndicator/TerminalConnectionIndicator.tsx:203">
P3: The unresponsive popover never shows how long terminals have been quiet, despite the recovery flow promising that live hint. Render `unreachableForMs` in this mode so users can distinguish a newly surfaced stall from one that has remained wedged before choosing the destructive restart.</violation>
</file>
<file name="packages/host-service/src/daemon/DaemonSupervisor.ts">
<violation number="1" location="packages/host-service/src/daemon/DaemonSupervisor.ts:914">
P1: A wedged daemon whose Unix-socket accept backlog is full can fail both probes, then this path drops its manifest and spawns over the same socket. The new daemon unlinks the old daemon's socket, orphaning its live terminal sessions rather than showing the intended recoverable “daemon quiet” state; retain a pid-alive daemon as unreachable unless process identity can be verified safely.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| ); | ||
| if (!probe) { | ||
| logEvent("pty_daemon_adopt_rejected", { | ||
| if (!probe && !(await isSocketConnectable(manifest.socketPath, 1000))) { |
Contributor
There was a problem hiding this comment.
P1: A wedged daemon whose Unix-socket accept backlog is full can fail both probes, then this path drops its manifest and spawns over the same socket. The new daemon unlinks the old daemon's socket, orphaning its live terminal sessions rather than showing the intended recoverable “daemon quiet” state; retain a pid-alive daemon as unreachable unless process identity can be verified safely.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/host-service/src/daemon/DaemonSupervisor.ts, line 914:
<comment>A wedged daemon whose Unix-socket accept backlog is full can fail both probes, then this path drops its manifest and spawns over the same socket. The new daemon unlinks the old daemon's socket, orphaning its live terminal sessions rather than showing the intended recoverable “daemon quiet” state; retain a pid-alive daemon as unreachable unless process identity can be verified safely.</comment>
<file context>
@@ -885,24 +902,40 @@ export class DaemonSupervisor {
manifest.socketPath,
ADOPTION_PROBE_TOTAL_TIMEOUT_MS,
);
+ if (!probe && !(await isSocketConnectable(manifest.socketPath, 1000))) {
+ // Nothing is even accepting on the socket. A live daemon keeps its
+ // listener no matter how overloaded it is, so this pid isn't our
</file context>
| mode === "reconnecting" && "animate-spin", | ||
| )} | ||
| /> | ||
| <p className="font-medium text-foreground">{label}</p> |
Contributor
There was a problem hiding this comment.
P3: The unresponsive popover never shows how long terminals have been quiet, despite the recovery flow promising that live hint. Render unreachableForMs in this mode so users can distinguish a newly surfaced stall from one that has remained wedged before choosing the destructive restart.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalPaneHeaderExtras/components/TerminalConnectionIndicator/TerminalConnectionIndicator.tsx, line 203:
<comment>The unresponsive popover never shows how long terminals have been quiet, despite the recovery flow promising that live hint. Render `unreachableForMs` in this mode so users can distinguish a newly surfaced stall from one that has remained wedged before choosing the destructive restart.</comment>
<file context>
@@ -147,102 +149,132 @@ export function TerminalConnectionIndicator({
+ mode === "reconnecting" && "animate-spin",
+ )}
+ />
+ <p className="font-medium text-foreground">{label}</p>
+ </div>
+ {/* Reconnect (solid) leads as the safe primary; Restart (outline)
</file context>
Suggested change
| <p className="font-medium text-foreground">{label}</p> | |
| <p className="font-medium text-foreground"> | |
| {mode === "unresponsive" | |
| ? `${label} · quiet for ${Math.floor((health?.unreachableForMs ?? 0) / 1_000)}s` | |
| : label} | |
| </p> |
The popover no longer renders a live "quiet for Ns" counter (dropped when the popover was simplified); describe the confirm + wait-it-out behavior it actually has.
3 tasks done
This was referenced Aug 9, 2026
Closed
Merged
This branch was previously deployed
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.
Links
Summary
tryAdoptdestroyed a live pty-daemon — and every shell under it — whenever post-update adoption was uncertain. That logic is deleted;tryAdoptis now one probe and one return.Part 1 — never kill PTYs during adoption
Terminals are meant to survive an update: the daemon is spawned
detached: trueand the relaunched host-service adopts it. Since #4739, PTY survival rests entirely on that adoption being non-destructive.It wasn't.
tryAdoptcalledterminateProcessTreeAndGroups(manifest.pid, …)when the socket wasn't connectable within 1s, or the hello probe went unanswered within 3s. That walks the daemon's entire process tree + process groups — every live shell — and SIGTERM→SIGKILLs them.Those budgets are spent at the worst moment: adoption runs eagerly at host-service boot (reaper →
getDaemonClient → waitForDaemonReady → ensure → tryAdopt), i.e. during the post-update relaunch spike — exactly when a healthy-but-CPU-starved daemon misses a 1–3s probe.The codebase already said what should happen.
expected-version.ts: "We do NOT auto-kill … sessions live in the daemon; the user explicitly triggers restart." AndDaemonInstancedocumentsrunningVersion: "unknown"for a failed probe, with "Probe failure does NOT set"updatePending. Adopting-on-probe-failure was the intended design; the kill was the bug.Also removed as dead weight once the kill went: the
previousManifestplumbing. (The expected-socket fallback that used to run only as post-kill recovery now has a real job again — it's the stale-manifest recovery path below.)Hardening the adoption path (post-review)
Now that adoption never signals the daemon, two edge cases need care:
isProcessAlive(a barekill(pid, 0)) passes, so blindly adopting would bind to a phantom: every terminal fails withECONNREFUSED, and the user's only recovery — Restart — would SIGTERM an innocent process tree.tryAdoptnow treats pid alive but probe failed and socket not connectable as stale: logpty_daemon_manifest_stale, drop the manifest, fall through to socket adoption / a fresh spawn. A genuinely wedged-but-listening daemon (socket bound, hello unanswered) is still adopted — SUPER-833 preserved.runningVersion: "unknown".refreshReachabilityalready backfills the real version on the next successful probe; it now also fires update-pending and kicks off the background auto-update, so a stale daemon adopted under load still self-updates instead of running outdated indefinitely.kill(pid,0)liveness check stays at 2s. (adoptedLivenessTimers→healthPollTimers; the four copies of the update-pending semver check are now oneisVersionUpdatePendinghelper.)Part 2 — "Terminals aren't responding"
Keeping a wedged daemon alive is right, but it left the tail case as a mystery: the terminal hangs ~15s and dies with
daemon open: timed out after 15000ms.Host-service tracks reachability on the poll that already watches daemons: probe each tick, record when it first went quiet, clear on recovery. Two things fell out of the E2E:
child.on("exit")owns death and crash-respawn (clearing the instance from the poll would have silently disabled respawn).runningVersionno longer stays stuck at"unknown".New
terminal.daemon.getHealthprocedure surfaces{ reachable, unreachableForMs }; it deliberately does notwaitForDaemonReady(it must answer immediately rather than block on the thing that may be wedged).Renderer — the pane's connection indicator, reworked into a plain-language popover:
terminal.daemon.restart. Both can show at once, so a stale daemon-health flag never leaves restart as the only option after sleep/wake (the transport can give up while the daemon flag is still catching up).Design decisions worth calling out:
openand the shell just freezes. The query takes no input, so React Query collapses every pane onto one request per workspace.dialog.showErrorBox(thealertChildCrashedpath) blocks Electron's main process — it wedged the whole app during this work. It would also demand a decision during the exact stall where waiting is correct, with a button that destroys every shell.turbo.jsonc: pass
SUPERSET_PTY_DAEMON_ADOPT_IN_DEVthrough. Turbo filters unlisted env vars, so the flag never reached host-service and the dev app always took the kill-stale-daemon path — making adoption (and this warning) impossible to exercise in dev. One line; no behavior change unless you set it.Testing
bun run lint✅ ·bun run typecheck(host-service + desktop) ✅bun test src/daemon/DaemonSupervisor.test.ts— 50 pass. Adds recycled-pid + expected-socket-fallback adoption tests and late-stale-version auto-update tests on top of the "adopts an unprobeable but pid-alive daemon instead of killing its shells" regression guard and the health suite.bun run test:integration:daemon(real daemons: DaemonClient + DaemonSupervisor) — 18 pass (crash-respawn + fd-handoff intact after the poller change).Stress harness — real daemons + real shells, isolated orgIds
SIGSTOP= a CPU-starved daemon (socket bound, hello unanswered).unknown,updatePending:false); daemon NOT killed; SHELL SURVIVESensure()during stallManual QA — real app, over CDP
Driven against the real dev app (production adoption via
SUPERSET_PTY_DAEMON_ADOPT_IN_DEV), with a real workspace + terminal under the real daemon. All paths confirmed:SIGSTOP;hostServiceCoordinator.restart(= the update's stop+start); hold past the old budget;SIGCONT→ daemon and shell both survived, log showsadopted existing daemon.pty_daemon_manifest_stale, a fresh daemon spawns, the impostor is never signalled.0.0.1daemon asunknown(probe failed viaSIGSTOP), thenSIGCONT→pty_daemon_update_pending→auto_update_attempt→auto_update_ok(fd-handoff to the fresh version).SIGSTOP) and WS give-up together → both Reconnect and Restart render; the red "Terminals aren't responding" pill appears at the 10s threshold;SIGCONT→ the retrying probe clears it, hostgetHealthreturnsreachable:true, shell alive throughout.reachable:true→SIGSTOP→false(crosses the 10s warn threshold) →SIGCONT→truewithin 3s.Known Limitations
killStaleDaemonForDev(dev-only) and the user-triggered restart stay destructive by design.Risks / Rollout