refactor(desktop): couple host-service to Electron, drop adoption#4739
Conversation
Host-service no longer survives Electron — children spawn attached (detached: false) and get SIGTERMed via stopAll() on before-quit. A parent-pid watchdog inside the child handles crash/force-kill paths where SIGTERM never arrives, so we don't leak orphans. PTY survival is now owned entirely by pty-daemon (which host-service supervises with its own detached lifecycle), so coupling host-service itself loses no user-facing property and removes a large class of "wedged adopted service" bugs. Removes tryAdopt/discoverAll/teardownKnownManifests/releaseAll/ adopted-liveness from the coordinator. Manifest writing stays — the CLI in packages/cli still reads it to find a live host-service. Adopts a 3s drain window on SIGTERM so in-flight HTTP/SSE/WS finish or get torn down cleanly.
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
|
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 (5)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughHost-service lifecycle refactored from manifest-based adoption to direct Electron coupling: Electron SIGTERMs host-services on quit via coordinator.stopAll(); host-service gains guarded shutdown and HOST_PARENT_PID watchdog; manifest shape and coordinator adoption/discovery removed; pty-daemon now owns PTY survivability. ChangesHost-Service Lifecycle Redesign
Sequence Diagram(s)sequenceDiagram
participant App as Electron App
participant Coord as HostServiceCoordinator
participant HS as Host-Service
participant PTY as pty-daemon
App->>Coord: before-quit -> stopAll()
Coord->>HS: SIGTERM
HS->>HS: shutdown() (server.close, drain, timeout)
HS->>PTY: manage/hand off PTYs (pty-daemon detached)
Note right of HS: HOST_PARENT_PID watchdog may self-exit if parent dies
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
|
Ready to review this PR? Stage has broken it down into 5 individual chapters for you: 
Chapters generated by Stage for commit 06aa423 on May 19, 2026 11:41pm UTC. |
![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
🤖 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 `@apps/desktop/docs/HOST_SERVICE_LIFECYCLE.md`:
- Line 7: The fenced diagram block in HOST_SERVICE_LIFECYCLE.md is missing a
language tag which triggers markdownlint MD040; update the opening fenced code
block (the triple-backtick diagram block) to include a language specifier such
as "text" so it becomes a fenced block with a language (e.g., change the opening
``` to ```text) to satisfy the linter and preserve the diagram rendering.
🪄 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: fdb24782-426b-44d3-a0e9-c9169943dbc0
📒 Files selected for processing (6)
apps/desktop/docs/HOST_SERVICE_LIFECYCLE.mdapps/desktop/src/lib/electron-app/factories/app/setup.tsapps/desktop/src/main/host-service/index.tsapps/desktop/src/main/index.tsapps/desktop/src/main/lib/host-service-coordinator.test.tsapps/desktop/src/main/lib/host-service-coordinator.ts
| Electron main owns app lifecycle, tray, and host-service management. Host-services run as child processes that can outlive the app via manifest-based adoption. | ||
| Electron main owns app lifecycle, tray, and host-service management. Host-service runs as a child process **coupled to Electron** — it starts and stops with the app. Terminal sessions (PTYs) survive Electron restarts via a separate `pty-daemon` that host-service supervises on its own detached lifecycle. | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Specify a language for the fenced diagram block.
Line 7 opens a fenced block without a language, which triggers markdownlint MD040.
Suggested fix
-```
+```text📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@apps/desktop/docs/HOST_SERVICE_LIFECYCLE.md` at line 7, The fenced diagram
block in HOST_SERVICE_LIFECYCLE.md is missing a language tag which triggers
markdownlint MD040; update the opening fenced code block (the triple-backtick
diagram block) to include a language specifier such as "text" so it becomes a
fenced block with a language (e.g., change the opening ``` to ```text) to
satisfy the linter and preserve the diagram rendering.
Greptile SummaryThis PR couples host-service lifetime to Electron by switching from
Confidence Score: 4/5Safe to merge. The adoption/release machinery is cleanly removed, the new watchdog and drain path handle all quit scenarios correctly, and stopAll() is now reliably called on every quit path. The overall refactor is well-structured and the shutdown flows are sound. The one edge case worth addressing is in isParentAlive: catching EPERM and returning false would cause the watchdog to self-terminate even though the Electron parent is still alive. This scenario requires a uid mismatch between parent and child, which is not the designed deployment, but the signal-0 POSIX contract for EPERM makes the silent catch technically incorrect. apps/desktop/src/main/host-service/index.ts — specifically the isParentAlive EPERM handling in the watchdog.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/main/host-service/index.ts | Adds graceful SIGTERM drain (3 s) and parent-watchdog (2 s poll); one edge case in EPERM handling in isParentAlive. |
| apps/desktop/src/main/lib/host-service-coordinator.ts | Removes adoption/release machinery; start() now directly spawns; detached:false + HOST_PARENT_PID set correctly; child.unref() now unconditional. |
| apps/desktop/src/main/index.ts | before-quit now always calls stopAll() (synchronous); runFullQuitCleanup collapsed into teardownTerminalHost; discoverAll removed from app startup. |
| apps/desktop/src/main/lib/host-service-coordinator.test.ts | Removes adoption test suite; introduces resetMocks() helper; tests updated to use stopAll() instead of releaseAll(). |
| apps/desktop/src/lib/electron-app/factories/app/setup.ts | Comment-only update reflecting new lifecycle (no release/re-adoption). |
| apps/desktop/docs/HOST_SERVICE_LIFECYCLE.md | Docs rewritten to reflect coupled lifecycle, watchdog, and pty-daemon separation. |
Sequence Diagram
sequenceDiagram
participant E as Electron Main
participant C as HostServiceCoordinator
participant HS as host-service child
participant WD as Watchdog in HS
Note over E,HS: Normal launch
E->>C: start(orgId, config)
C->>HS: "spawn detached:false HOST_PARENT_PID=Electron.pid"
HS-->>C: pollHealthCheck returns Connection
HS->>WD: setInterval 2s poll ppid
Note over E,HS: Clean quit
E->>C: stopAll in before-quit sync
C->>HS: SIGTERM
HS->>HS: shutdown SIGTERM server.close
HS->>HS: forceExit timer 3s unrefed
HS->>HS: closeAllConnections then process.exit
Note over E,HS: Force-kill crash
E--xE: parent exits without SIGTERM
WD->>WD: ppid changed or ESRCH
WD->>HS: shutdown parent-exit
HS->>HS: same 3s drain path
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
apps/desktop/src/main/host-service/index.ts:139-146
The `catch` block swallows all errors from `process.kill(parentPid, 0)`, including `EPERM`. On POSIX, `EPERM` means the target process **exists** but the caller lacks permission to signal it (e.g. different uid after a privilege drop). Returning `false` in that case would cause the watchdog to call `shutdown("parent-exit")` even though Electron is still alive, killing the host-service spuriously. `ESRCH` (process not found) is the only error that genuinely means "dead parent".
```suggestion
function isParentAlive(parentPid: number): boolean {
try {
process.kill(parentPid, 0);
return process.ppid === parentPid;
} catch (err) {
// EPERM means the process exists but we can't signal it — still alive.
if ((err as NodeJS.ErrnoException).code === "EPERM") {
return process.ppid === parentPid;
}
return false;
}
}
```
Reviews (1): Last reviewed commit: "refactor(desktop): couple host-service t..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function isParentAlive(parentPid: number): boolean { | ||
| try { | ||
| process.kill(parentPid, 0); | ||
| return process.ppid === parentPid; | ||
| } catch { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
catch block swallows all errors from process.kill(parentPid, 0), including EPERM. On POSIX, EPERM means the target process exists but the caller lacks permission to signal it (e.g. different uid after a privilege drop). Returning false in that case would cause the watchdog to call shutdown("parent-exit") even though Electron is still alive, killing the host-service spuriously. ESRCH (process not found) is the only error that genuinely means "dead parent".
| function isParentAlive(parentPid: number): boolean { | |
| try { | |
| process.kill(parentPid, 0); | |
| return process.ppid === parentPid; | |
| } catch { | |
| return false; | |
| } | |
| } | |
| function isParentAlive(parentPid: number): boolean { | |
| try { | |
| process.kill(parentPid, 0); | |
| return process.ppid === parentPid; | |
| } catch (err) { | |
| // EPERM means the process exists but we can't signal it — still alive. | |
| if ((err as NodeJS.ErrnoException).code === "EPERM") { | |
| return process.ppid === parentPid; | |
| } | |
| return false; | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/host-service/index.ts
Line: 139-146
Comment:
The `catch` block swallows all errors from `process.kill(parentPid, 0)`, including `EPERM`. On POSIX, `EPERM` means the target process **exists** but the caller lacks permission to signal it (e.g. different uid after a privilege drop). Returning `false` in that case would cause the watchdog to call `shutdown("parent-exit")` even though Electron is still alive, killing the host-service spuriously. `ESRCH` (process not found) is the only error that genuinely means "dead parent".
```suggestion
function isParentAlive(parentPid: number): boolean {
try {
process.kill(parentPid, 0);
return process.ppid === parentPid;
} catch (err) {
// EPERM means the process exists but we can't signal it — still alive.
if ((err as NodeJS.ErrnoException).code === "EPERM") {
return process.ppid === parentPid;
}
return false;
}
}
```
How can I resolve this? If you propose a fix, please make it concise.![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
- Delete hasActiveInstances() — no callers post-adoption. - Delete listManifests() from apps/desktop manifest module — only callers were the adoption discoverAll/teardownKnownManifests paths, both removed. - Delete spawnedByAppVersion manifest field + SUPERSET_APP_VERSION env propagation — only consumer was the version-mismatch adoption branch. - Install parent-pid watchdog at the top of the desktop host-service main() so a crash during startup awaits can still reap the child.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
) * refactor(desktop): couple host-service to Electron, drop adoption Host-service no longer survives Electron — children spawn attached (detached: false) and get SIGTERMed via stopAll() on before-quit. A parent-pid watchdog inside the child handles crash/force-kill paths where SIGTERM never arrives, so we don't leak orphans. PTY survival is now owned entirely by pty-daemon (which host-service supervises with its own detached lifecycle), so coupling host-service itself loses no user-facing property and removes a large class of "wedged adopted service" bugs. Removes tryAdopt/discoverAll/teardownKnownManifests/releaseAll/ adopted-liveness from the coordinator. Manifest writing stays — the CLI in packages/cli still reads it to find a live host-service. Adopts a 3s drain window on SIGTERM so in-flight HTTP/SSE/WS finish or get torn down cleanly. * chore(desktop): drop dead code from host-service coupling refactor - Delete hasActiveInstances() — no callers post-adoption. - Delete listManifests() from apps/desktop manifest module — only callers were the adoption discoverAll/teardownKnownManifests paths, both removed. - Delete spawnedByAppVersion manifest field + SUPERSET_APP_VERSION env propagation — only consumer was the version-mismatch adoption branch. - Install parent-pid watchdog at the top of the desktop host-service main() so a crash during startup awaits can still reap the child.
1 participant
Summary
Host-service no longer survives Electron. The detach + manifest-adoption machinery was load-bearing back when host-service owned PTYs directly; now that pty-daemon (separate detached lifecycle, supervised by host-service) owns PTY survival, host-service itself has no reason to outlive the app.
Net: ~390 lines deleted, simpler lifecycle, no orphan host-services.
Test plan
Summary by cubic
Coupled the desktop host-service to Electron so it starts and stops with the app; removed manifest adoption and simplified quit paths. PTY survival is handled by a detached pty-daemon, keeping terminals intact across restarts.
Refactors
detached: false);before-quitcallscoordinator.stopAll().tryAdopt,discoverAll,teardownKnownManifests,releaseAll, adopted-liveness timer, version-mismatch branch.hasActiveInstances,listManifests; droppedspawnedByAppVersionfrom manifest andSUPERSET_APP_VERSIONenv.runFullQuitCleanupwithteardownTerminalHost(v1 only); dev SIGTERM/SIGINT also stops services.reset. CLI inpackages/clistill reads them.Bug Fixes
closeAllConnectionsends SSE/WS so shutdown doesn’t hang.Written for commit 06aa423. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
Improvements
Changes