Repository navigation
fix(host-service): detect ports for terminals without an attached renderer - #5325
Merged
Merged
Conversation
…ched renderer v2 registered a terminal for port scanning only when a renderer attached to it, so dev servers in unopened terminals — notably sessions the daemon keeps alive across a host-service restart — were never scanned. v1 desktop avoids this by registering all live daemon sessions at startup. Fold a port-scan sync into the existing terminal reaper, which already lists daemon sessions and joins terminalSessions for workspace ids on the same pass (immediately at startup + on its interval). It registers alive sessions mapped to an active workspace row and not already owned by a live in-memory session, and unregisters watched terminals the daemon no longer reports. The existing event-bus bridge (portManager -> port:changed) and ports.getAll already fan these out to the renderer.
Contributor
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds port-scan reconciliation to the terminal reaper. A new pure ChangesPort-scan reconciliation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found and verified against the latest diff
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Contributor
Greptile SummaryThis PR fixes a port-detection gap in v2 host-service: terminals running after a host restart (not yet attached to any renderer) were never registered with the
Confidence Score: 4/5Safe to merge. The reconciliation logic is correct for all adoption, race-guard, and cleanup scenarios, and is well-covered by the new unit tests. The core planPortScanSync function correctly handles all edge cases: it avoids double-registration of live sessions, clears stale scans when the daemon is empty, and guards against a racy daemon.list() drop of renderer-attached sessions. Two minor inefficiencies exist — the DB query running unconditionally even when liveSessions is empty, and already-adopted sessions being re-upserted on every pass — but neither affects correctness since upsertSession is idempotent and the query is lightweight. Tests cover all six key branches. reaper.ts deserves a second look for the unconditional DB query when liveSessions is empty (rowById is never read in that path).
|
| Filename | Overview |
|---|---|
| packages/host-service/src/terminal/reaper/reaper.ts | Core of the fix: adds planPortScanSync (pure, exported for testing), applyPortScanSync, and wires them into the reapOrphanedSessions loop. The DB query now runs unconditionally even when liveSessions is empty (rowById is unused in that path), but this is a minor efficiency concern. Logic is correct for all cases described in the PR. |
| packages/host-service/src/terminal/reaper/reaper.test.ts | New test file with 6 focused unit tests for planPortScanSync covering all key branches: register, skip-live, skip-inactive/no-workspace, unregister stale, empty daemon clear, and race-guard for attached sessions. |
| packages/host-service/src/terminal/terminal.ts | Adds isLiveTerminalSession(): checks the module-level sessions map for a non-exited session. Simple and correct; correctly returns false for sessions that exist but have exited. |
| packages/port-scanner/src/port-manager.ts | Adds getRegisteredTerminalIds(): returns Array.from(this.sessions.keys()), a snapshot copy — safe to iterate while the map is modified. Minimal, correct addition. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant R as TerminalReaper
participant D as DaemonClient
participant DB as HostDb
participant P as PortManager
participant T as terminal.ts
R->>D: daemon.list()
D-->>R: sessions (id, pid, alive)
R->>R: "filter alive = liveSessions"
R->>DB: SELECT id, status, originWorkspaceId
DB-->>R: rows
R->>R: build rowById Map
R->>T: isLive(terminalId) for each liveSession
T-->>R: false if no in-memory session
R->>P: upsertSession(terminalId, workspaceId, pid)
Note over R,P: register adopted sessions without in-memory owner
R->>T: isLive(registeredId) for stale scans
T-->>R: false if no in-memory session
R->>P: unregisterSession(terminalId)
Note over R,P: drop sessions absent from daemon.list and not live
alt liveSessions is empty
R->>R: clear rowlessPendingSecondPass and return
else has live sessions
R->>R: orphan detection and disposeSessionAndWait
end
%%{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"}}}%%
sequenceDiagram
participant R as TerminalReaper
participant D as DaemonClient
participant DB as HostDb
participant P as PortManager
participant T as terminal.ts
R->>D: daemon.list()
D-->>R: sessions (id, pid, alive)
R->>R: "filter alive = liveSessions"
R->>DB: SELECT id, status, originWorkspaceId
DB-->>R: rows
R->>R: build rowById Map
R->>T: isLive(terminalId) for each liveSession
T-->>R: false if no in-memory session
R->>P: upsertSession(terminalId, workspaceId, pid)
Note over R,P: register adopted sessions without in-memory owner
R->>T: isLive(registeredId) for stale scans
T-->>R: false if no in-memory session
R->>P: unregisterSession(terminalId)
Note over R,P: drop sessions absent from daemon.list and not live
alt liveSessions is empty
R->>R: clear rowlessPendingSecondPass and return
else has live sessions
R->>R: orphan detection and disposeSessionAndWait
end
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/terminal/reaper/reaper.ts:105-119
The full `terminalSessions` SELECT now runs even when `liveSessions` is empty. In that code path `rowById` is never accessed — the `register` loop iterates over `liveSessions` (empty), so the query result is immediately discarded. Before this PR the DB call was guarded by the early-return. Moving the query unconditionally before `applyPortScanSync` is fine for correctness, but the query can be skipped when there are no live sessions because `planPortScanSync` only reads `rowById` inside the register loop.
```suggestion
// Keep the port scanner in sync with the daemon's live sessions on the same
// pass that lists them — registers ports for terminals running without an
// attached renderer, and drops scans for ones the daemon has dropped. Runs
// before the empty-list short-circuit so an empty daemon clears stale scans.
// rowById is only consulted by the register path, so skip the query when
// there are no live sessions to register.
const rowById: Map<string, TerminalRow> =
liveSessions.length > 0
? new Map(
db
.select({
id: terminalSessions.id,
status: terminalSessions.status,
originWorkspaceId: terminalSessions.originWorkspaceId,
})
.from(terminalSessions)
.all()
.map((row) => [row.id, row]),
)
: new Map();
applyPortScanSync(liveSessions, rowById);
```
### Issue 2 of 2
packages/host-service/src/terminal/reaper/reaper.ts:57-68
**Register path re-upserts already-adopted sessions every pass**
The `register` loop never consults `registeredTerminalIds`, so every non-live, active, workspace-mapped daemon session is passed to `portManager.upsertSession` on every reaper invocation (every 5 minutes), regardless of whether it was already registered in a prior pass. `upsertSession` is idempotent (`sessions.set` with the same values), so there is no functional bug. It's worth noting in case a future change makes `upsertSession` non-idempotent (e.g. resetting a `firstSeenAt` timestamp or triggering an eager scan), at which point a guard like `if (!registeredTerminalIds.includes(session.id))` in `planPortScanSync` would be needed.
Reviews (1): Last reviewed commit: "refactor(host-service): trim redundant p..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+105
to
+119
| @@ -31,6 +112,15 @@ async function reapOrphanedSessions( | |||
| .all(); | |||
| const rowById = new Map(rows.map((row) => [row.id, row])); | |||
|
|
|||
| // Reuse the list + row join the reaper already did. Must run before the | |||
| // empty-list short-circuit so an empty daemon still clears stale scans. | |||
| applyPortScanSync(liveSessions, rowById); | |||
|
|
|||
| if (liveSessions.length === 0) { | |||
Contributor
There was a problem hiding this comment.
terminalSessions SELECT now runs even when liveSessions is empty. In that code path rowById is never accessed — the register loop iterates over liveSessions (empty), so the query result is immediately discarded. Before this PR the DB call was guarded by the early-return. Moving the query unconditionally before applyPortScanSync is fine for correctness, but the query can be skipped when there are no live sessions because planPortScanSync only reads rowById inside the register loop.
Suggested change
| // Keep the port scanner in sync with the daemon's live sessions on the same | |
| // pass that lists them — registers ports for terminals running without an | |
| // attached renderer, and drops scans for ones the daemon has dropped. Runs | |
| // before the empty-list short-circuit so an empty daemon clears stale scans. | |
| // rowById is only consulted by the register path, so skip the query when | |
| // there are no live sessions to register. | |
| const rowById: Map<string, TerminalRow> = | |
| liveSessions.length > 0 | |
| ? new Map( | |
| db | |
| .select({ | |
| id: terminalSessions.id, | |
| status: terminalSessions.status, | |
| originWorkspaceId: terminalSessions.originWorkspaceId, | |
| }) | |
| .from(terminalSessions) | |
| .all() | |
| .map((row) => [row.id, row]), | |
| ) | |
| : new Map(); | |
| applyPortScanSync(liveSessions, rowById); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/terminal/reaper/reaper.ts
Line: 105-119
Comment:
The full `terminalSessions` SELECT now runs even when `liveSessions` is empty. In that code path `rowById` is never accessed — the `register` loop iterates over `liveSessions` (empty), so the query result is immediately discarded. Before this PR the DB call was guarded by the early-return. Moving the query unconditionally before `applyPortScanSync` is fine for correctness, but the query can be skipped when there are no live sessions because `planPortScanSync` only reads `rowById` inside the register loop.
```suggestion
// Keep the port scanner in sync with the daemon's live sessions on the same
// pass that lists them — registers ports for terminals running without an
// attached renderer, and drops scans for ones the daemon has dropped. Runs
// before the empty-list short-circuit so an empty daemon clears stale scans.
// rowById is only consulted by the register path, so skip the query when
// there are no live sessions to register.
const rowById: Map<string, TerminalRow> =
liveSessions.length > 0
? new Map(
db
.select({
id: terminalSessions.id,
status: terminalSessions.status,
originWorkspaceId: terminalSessions.originWorkspaceId,
})
.from(terminalSessions)
.all()
.map((row) => [row.id, row]),
)
: new Map();
applyPortScanSync(liveSessions, rowById);
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+57
to
+68
| const register: PortScanSyncPlan["register"] = []; | ||
| for (const session of liveSessions) { | ||
| if (isLive(session.id)) continue; | ||
| const row = rowById.get(session.id); | ||
| if (!row?.originWorkspaceId) continue; | ||
| if (row.status !== "active") continue; | ||
| register.push({ | ||
| terminalId: session.id, | ||
| workspaceId: row.originWorkspaceId, | ||
| pid: session.pid, | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
The register loop never consults registeredTerminalIds, so every non-live, active, workspace-mapped daemon session is passed to portManager.upsertSession on every reaper invocation (every 5 minutes), regardless of whether it was already registered in a prior pass. upsertSession is idempotent (sessions.set with the same values), so there is no functional bug. It's worth noting in case a future change makes upsertSession non-idempotent (e.g. resetting a firstSeenAt timestamp or triggering an eager scan), at which point a guard like if (!registeredTerminalIds.includes(session.id)) in planPortScanSync would be needed.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/terminal/reaper/reaper.ts
Line: 57-68
Comment:
**Register path re-upserts already-adopted sessions every pass**
The `register` loop never consults `registeredTerminalIds`, so every non-live, active, workspace-mapped daemon session is passed to `portManager.upsertSession` on every reaper invocation (every 5 minutes), regardless of whether it was already registered in a prior pass. `upsertSession` is idempotent (`sessions.set` with the same values), so there is no functional bug. It's worth noting in case a future change makes `upsertSession` non-idempotent (e.g. resetting a `firstSeenAt` timestamp or triggering an eager scan), at which point a guard like `if (!registeredTerminalIds.includes(session.id))` in `planPortScanSync` would be needed.
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!
- skip the terminalSessions query when the daemon reports no live sessions (the row join is only consulted to register live sessions; an empty daemon still runs the sync to drop stale scans, just with an empty map) - isolate applyPortScanSync in try/catch so a port-manager error can't abort the reaper's orphan cleanup for that interval
Member
Author
|
Both addressed in 7c5a6e4:
|
|
Ready to review this PR? Stage has broken it down into 4 individual chapters for you:
Chapters generated by Stage for commit 9c940ac on Jun 29, 2026 3:14am UTC. |
This was referenced Jun 29, 2026
Kitenite
added a commit
that referenced
this pull request
Jul 3, 2026
After a host-service restart the detached pty-daemon keeps dev servers alive, but the new host-service starts with an empty port scanner. #5325 re-registers those adopted sessions via the terminal reaper — but the reaper runs its port-scan sync only once immediately and then every 5 minutes, and the just-adopted daemon may not yet list its sessions the instant that first pass runs. So ports stayed missing until the 5-minute tick or until the user selected the workspace (which attaches a renderer and registers the terminal directly). Decouple the port-scan sync from the 5-minute orphan-reap cadence and run it a few times over the first ~90s after startup (2/5/10/20/45/90s). This re-registers restored dev-server ports promptly without disturbing the reaper's two-pass rowless-orphan clock. Also log when the sync registers unattached sessions so the recovery is observable in the field. - Extract syncPortScans() (daemon.list + row join + applyPortScanSync); the reap pass reuses its result, so no extra daemon.list call there. - Warm-up offsets all stay below REAP_INTERVAL_MS; covered by a schedule test.
Kitenite
added a commit
that referenced
this pull request
Jul 3, 2026
…rt (#5430) * fix(host-service): recover v2 ports promptly after host-service restart After a host-service restart the detached pty-daemon keeps dev servers alive, but the new host-service starts with an empty port scanner. #5325 re-registers those adopted sessions via the terminal reaper — but the reaper runs its port-scan sync only once immediately and then every 5 minutes, and the just-adopted daemon may not yet list its sessions the instant that first pass runs. So ports stayed missing until the 5-minute tick or until the user selected the workspace (which attaches a renderer and registers the terminal directly). Decouple the port-scan sync from the 5-minute orphan-reap cadence and run it a few times over the first ~90s after startup (2/5/10/20/45/90s). This re-registers restored dev-server ports promptly without disturbing the reaper's two-pass rowless-orphan clock. Also log when the sync registers unattached sessions so the recovery is observable in the field. - Extract syncPortScans() (daemon.list + row join + applyPortScanSync); the reap pass reuses its result, so no extra daemon.list call there. - Warm-up offsets all stay below REAP_INTERVAL_MS; covered by a schedule test. * fix(host-service): coalesce concurrent port-scan syncs Address review: the warm-up timers and the initial reap pass both call syncPortScans unguarded. A slow daemon.list right after adoption (exactly when the warm-up fires) could let a second sync observe a transiently-empty list and unregister sessions the first just registered. Coalesce concurrent callers onto one in-flight run. * docs(host-service): fix stale port-scan-sync comment after warm-up refactor
This was referenced Jul 3, 2026
Draft
Kitenite
added a commit
that referenced
this pull request
Jul 3, 2026
…ntry (#5438) * fix(desktop): start the terminal reaper in the desktop host-service entry 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. * refactor(host-service): start the terminal reaper inside createApp 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. * refactor(host-service): move the reaper behind a package-owned startHostRuntime 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. * refactor(host-service): drop the startHostRuntime pass-through 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. * chore(desktop): tighten the reaper comment * chore(desktop): drop call-site comment on startTerminalReaper * chore(desktop): one-line comment on startTerminalReaper
This was referenced Jul 17, 2026
3 tasks done
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.
Problem
Ports were detected less completely in v2 than in v1. Both use the same shared
@superset/port-scannerPortManager(2.5s periodic scan of registered terminal sessions, keyed by PID tree), so the gap was in what gets registered:daemon-manager.ts→reconcileOnStartup) pre-registers every alive daemon session withportManager.upsertSession(...)at startup, before any renderer attaches.upsertSessioninsidecreateTerminalSessionInternal— i.e. only when a renderer creates or attaches to a terminal. Terminals the daemon is running but nobody has opened (notably sessions adopted after a host-service restart) were never scanned, so their ports never appeared in the v2 sidebar.Fix
Fold a port-scan sync into the existing terminal reaper (
reaper.ts) instead of adding a parallel module. The reaper already callsdaemon.list()and joinsterminalSessions(forterminalId → workspaceId) on the same pass, immediately at startup and on its interval — so there's no extra daemon call, DB query, or timer.planPortScanSync()— pure decision function:applyPortScanSync()applies the plan toportManageron each reaper pass, before the empty-list short-circuit so an empty daemon clears stale scans.PortManager.getRegisteredTerminalIds()andterminal.ts isLiveTerminalSession()(the latter guards a renderer-attached session against being dropped during a racydaemon.list()).The
terminalId → workspaceIdjoin is irreducible:daemon.list()returns{id, pid, alive}only, and the sidebar groups ports by workspace — that mapping lives in the host DB, which the reaper already reads.End-to-end
eventBus.start()(increateApp) wiresportManager→port:changedbroadcast; the reaper's immediate first pass registers live sessions → periodic scan begins. Bridge is wired before registration, so no early events drop.ports.getAllreturnsportManager.getAllPorts(), now populated for unattached terminals.port:changedstream delivers live add/remove for all live terminals, attached or not.Notes
Testing
reaper.test.tscoversplanPortScanSync(register/skip/unregister/empty-daemon/race-guard); verified each case fails under impl mutation.bun run lintexits 0; typecheck passes for@superset/host-serviceand@superset/port-scanner; existingport-scannertests pass.Summary by cubic
Fixes missing port detection for terminals running without an attached renderer by syncing the port scanner with daemon sessions during reaper passes. Ports now appear for adopted/live terminals right after start and stay accurate.
portManagerwith alive daemon sessions on each reaper pass: register mapped active rows not owned by a live in-memory session; unregister watched terminals the daemon no longer reports; guard renderer-attached sessions viaisLiveTerminalSession.PortManager.getRegisteredTerminalIds(),planPortScanSync, and tests.Written for commit 9c940ac. Summary will update on new commits.
Summary by CodeRabbit
New Features
Tests