Repository navigation
fix(desktop): start the terminal reaper in the desktop host-service entry - #5438
Conversation
…ntry startTerminalReaper was only wired into packages/host-service serve.ts (the standalone/CLI entry), so the packaged desktop app never ran the orphan reap (#5168) or the port-scan reconcile for daemon-adopted sessions (#5325/#5430). Ports for terminals that survived a host-service restart stayed invisible until the workspace was selected.
|
Ready to review this PR? Stage has broken it down into 2 individual chapters for you:
Chapters generated by Stage for commit bcf499d on Jul 3, 2026 9:25pm UTC. |
|
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:
📝 WalkthroughWalkthroughAdds ChangesTerminal reaper startup wiring
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/main/host-service/index.ts (1)
123-123: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReturned teardown function is discarded.
startTerminalReaperreturns a dispose function that clears its interval and warm-up timers, but it's not captured here. Since the timers are.unref()'d andshutdown()ultimately callsprocess.exit(0), this has no practical runtime impact in this single-shot process entry, but wiring it intoshutdown()would be more symmetric with the explicit cleanup pattern used elsewhere (e.g.installProcessSafetyNet).♻️ Optional: capture and invoke teardown on shutdown
+ const stopReaper = startTerminalReaper(db); - startTerminalReaper(db);Then call
stopReaper()insideshutdown(...)alongsideserver.close().🤖 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/src/main/host-service/index.ts` at line 123, The teardown from startTerminalReaper is currently ignored, so its interval and warm-up timers are never explicitly cleaned up. Capture the dispose function returned by startTerminalReaper(db) in host-service/index.ts, then invoke it inside shutdown() alongside server.close() to match the cleanup pattern already used by installProcessSafetyNet.
🤖 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.
Nitpick comments:
In `@apps/desktop/src/main/host-service/index.ts`:
- Line 123: The teardown from startTerminalReaper is currently ignored, so its
interval and warm-up timers are never explicitly cleaned up. Capture the dispose
function returned by startTerminalReaper(db) in host-service/index.ts, then
invoke it inside shutdown() alongside server.close() to match the cleanup
pattern already used by installProcessSafetyNet.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4f1ad474-96e2-4d77-b934-a1f9abf9e862
📒 Files selected for processing (2)
apps/desktop/src/main/host-service/index.tspackages/host-service/src/index.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 2 files
Architecture diagram
sequenceDiagram
participant Desktop as Desktop App (main process)
participant HostService as Host Service Entry
participant CreateApp as createApp()
participant Reaper as startTerminalReaper(db)
participant Daemon as PTY Daemon
participant DB as Database
Note over Desktop,DB: Desktop Host-Service Startup Flow
Desktop->>HostService: Spawn as Node process
HostService->>HostService: initTerminalBaseEnv()
HostService->>CreateApp: createApp(config)
CreateApp-->>HostService: { app, injectWebSocket, api, db }
HostService->>HostService: Start HTTP server (listen callback)
Note over HostService,DB: Port-scan registration daemon sessions
HostService->>Reaper: startTerminalReaper(db)
alt Warm-up sync (immediate)
Reaper->>DB: Query unattached daemon sessions
DB-->>Reaper: Sessions with status:"active" + no renderer
Reaper->>Daemon: Register sessions for port scanning
Daemon-->>Reaper: Port scan results
Reaper->>DB: upsertSession() for scanned ports
Note over Reaper: Ports visible via ports.getAll()<br/>without workspace open
end
loop Every 5 minutes
Reaper->>DB: Query orphaned daemon sessions
alt Two consecutive reap passes
Reaper->>Daemon: Kill orphaned PTY
Daemon-->>Reaper: Session cleaned
Reaper->>DB: Update session status
end
end
Note over Desktop,DB: Server WebSocket (unchanged)
CreateApp->>Desktop: injectWebSocket()
Desktop->>Desktop: registerServeTerminalType()
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
|
re: coderabbit's nitpick about the discarded |
Both real entry points (serve.ts and the desktop entry) consume createApp, so owning the reaper there removes the per-entry wiring that let the desktop app ship without it. Tests opt out via createTestHost: the reaper's warm-up timers reach the real pty-daemon through the process-wide daemon-client singleton and reap other tests' sessions (observed as 5 integration failures with an unconditional start). The reaper's stop function joins createApp's dispose chain.
|
Restructured per discussion: the reaper now starts inside |
…ostRuntime createApp goes back to building the request-serving surface only — no reaper and no test-only escape hatch. Process-lifetime background work now lives in startHostRuntime (runtime/host-runtime.ts), which both serving entries call once after binding. Tests consume createApp and are untouched.
|
Final shape: dropped the |
The wrapper only delegated to startTerminalReaper. Entries call the reaper directly, same as installProcessSafetyNet and connectRelay — one invocation per serving entry, logic stays in the package.
|
Settled on the direct shape: dropped the |
Summary
startTerminalReaper(db)into the desktop host-service entry point. It was only called frompackages/host-service/src/serve.ts(the standalone/CLI entry), so the packaged desktop app never ran the terminal reaper at all.Why / Context
#5325 added the reaper-driven port-scan reconcile (register daemon sessions no renderer has attached, e.g. after a host-service restart) and #5430 added 2–90s warm-up retries. Both are correct — but
startTerminalReaperwas never invoked byapps/desktop/src/main/host-service/index.ts, the entry the desktop app actually spawns. In the shipped app the onlyportManager.upsertSessioncaller was the renderer terminal-attach path, so ports for surviving sessions stayed invisible until the user opened the workspace.Confirmed against a production install (v1.13.0): the asar contains the #5430 code, the daemon adopts within seconds, valid
terminal_sessionsrows exist — and the host-service log contains zero reaper lines across every run.Supersedes the revert stack in #5437 (kept as a draft safety net; merging that would not fix this symptom).
How It Works
startTerminalReaperis exported from the@superset/host-serviceroot, and each serving entry (serve.ts and the desktop entry) calls it once in its listen callback — the same per-entry pattern asinstallProcessSafetyNet,initTerminalBaseEnv, andconnectRelay. All reaper logic stays in the package.Two single-call-site alternatives were tried and rejected during review discussion: starting it inside
createApprequires a test-only escape hatch (the reaper is a destructive background actor that reaps integration tests' staged daemon sessions mid-test — 5 failures), and astartHostRuntimewrapper was pure pass-through indirection.Manual QA
End-to-end against the freshly built
dist/main/host-service.js(run under Electron-as-Node, exactly as the app spawns it):python3 -m http.server 18923, and disconnected — an alive, unattached daemon session, the post-update state.SUPERSET_PTY_DAEMON_SOCKETpointed at the scratch daemon, and inserted the session'sterminal_sessionsrow (status: "active"+ workspace id).port-scan sync: registered 1 unattached daemon session(s) for scanningwithin seconds — no renderer ever attached.ports.getAllfor that workspace returned{ port: 18923, processName: "Python", ... }.startTerminalReaper(db)call.createApp): same result —registered 1 unattached daemon session(s)andports.getAllreturning the planted port.Full-app verification over CDP (
RENDERER_REMOTE_DEBUG_PORT=9223 bun dev, this branch):python3 -m http.server 18933(row bound to a workspace that was never selected), no renderer attach.port-scan sync: registered 1 unattached daemon session(s) for scanning.18933port badge inside that workspace's item while a different workspace was selected — confirmed via CDP DOM inspection.Known Limitations
startDaemonBootstrapat startup — the daemon boots on demand. The reaper's earliest warm-up (2s) can beat it and logport-scan warm-up sync failed: pty-daemon is not availableonce; later warm-ups (5–90s) and the 5-min reap succeed. Pre-existing ordering, observed once in dev; harmless but noisy.Testing
bun run typecheck --filter=@superset/host-service --filter=@superset/desktopbun run lint(exit 0)bun test src/terminal/reaperinpackages/host-service(9 pass)bun turbo run build --filter=@superset/desktopRisks / Rollout
status: "exited"/"disposed"rows only describe already-dead PTYs, so healthy sessions are unaffected — but this is behavior the desktop app has never exercised in production.Summary by cubic
Start the terminal reaper from the desktop host-service entry with
startTerminalReaper(db). This cleans up orphaned sessions and reconciles ports on boot, fixing ports not showing until a workspace is opened.@superset/host-service; both serving entries run it on boot.startHostRuntimeand keep reaper startup outsidecreateApp.Written for commit bcf499d. Summary will update on new commits.
Summary by CodeRabbit