Repository navigation
perf(port-scanner): shared ps per scan, lsof -a, idle-session decay - #5723
Merged
Merged
Conversation
Customer-reported (and reproduced): the port scanner's CPU/process-spawn cost scaled linearly with terminal count and never backed off. - getProcessTreesForPids: one pidtree(-1) system-table read per scan cycle walked in memory, replacing one full `ps -A` spawn per session per 2.5s tick (measured ~5 ps/sec at 26 sessions, all idle). - lsof -a: AND the -p and -iTCP selectors. Without it lsof ORs them and enumerates every TCP listener on the machine (observed 721 lines from 56 unrelated processes), all discarded in JS by the pid filter. - Idle decay: sessions with no PTY output for 60s are scanned every 30s instead of every 2.5s; any output restores the fast cadence. Fixes parked/unattached sessions (boot-restored, reaper-registered) paying full scan cost 24/7 with nobody watching. Re-upserts of the same pid (reaper sync) don't reset the idle clock. - Rename stopPeriodicScanIfIdle → stopPeriodicScanIfNoSessions; it only ever stopped at zero sessions and hid the missing idle handling.
|
Ready to review this PR? Stage has broken it down into 5 individual chapters for you: 
Chapters generated by Stage for commit 84e0426 on Jul 17, 2026 12:22am UTC. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 7 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
Greptile SummaryThis PR addresses a customer-reported background load issue in the port scanner by making three targeted fixes: collapsing O(sessions)
Confidence Score: 4/5Safe to merge. All three fixes are mechanically straightforward, well-tested with mutation-verified tests, and the behavioral changes are clearly documented and match the stated intent. The implementation is solid: idle-clock updates happen before any early returns in packages/host-service/src/terminal/terminal.ts — idle clock update is conditional on the StringDecoder producing a non-empty chunk, unlike daemon-manager.ts which always updates it.
|
| Filename | Overview |
|---|---|
| packages/port-scanner/src/scanner.ts | Replaces per-session getProcessTree(pid) with a single system-wide getProcessTreesForPids(rootPids[]) using pidtree(-1, {advanced:true}); adds -a to lsof to AND selectors. Core of the O(sessions)→O(1) improvement. |
| packages/port-scanner/src/port-manager.ts | Adds lastActivityAt/lastScannedAt to SessionEntry, idle decay logic (isSessionDue), batched collectSessionPids, and promotes checkOutputForHint to accept terminalId as an activity signal. Minor inconsistency in how the idle clock is updated vs. the daemon path. |
| packages/port-scanner/src/port-manager.test.ts | Adds comprehensive idle-decay tests (skip/slow cadence, activity restore, same-pid re-upsert, skipped sessions retain ports, forceScan bypasses cadence) and updates existing tests for the new bulk-tree API. Module-restore afterAll guard prevents mock leakage to scanner.test.ts. |
| packages/port-scanner/src/scanner.test.ts | Adds real-process integration tests for getProcessTreesForPids (child appears in tree, dead roots omitted) and getListeningPortsForPids (Bun.serve listener found with -a in place). Well-structured and complete. |
| packages/host-service/src/terminal/terminal.ts | Passes terminalId to checkOutputForHint, but the call is gated on hintText.length > 0 — if the StringDecoder produces an empty chunk (e.g. partial multibyte sequences from binary output), the idle clock won't be updated for that chunk. |
| apps/desktop/src/main/lib/terminal/daemon/daemon-manager.ts | Trivial one-line update: passes paneId as the new first arg to checkOutputForHint. Called on every data event, so the idle clock is always updated in this path. |
| packages/port-scanner/src/index.ts | Re-exports getProcessTreesForPids in place of the removed getProcessTree. Correct rename. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Interval as "setInterval (2.5s)"
participant PM as PortManager
participant Scanner as "scanner.ts"
participant OS as "OS (ps/lsof)"
note over Interval,OS: Before this PR — per-session ps spawns
Interval->>PM: tick
PM->>Scanner: getProcessTree(pidA)
Scanner->>OS: "ps -A (spawn 1)"
PM->>Scanner: getProcessTree(pidB)
Scanner->>OS: "ps -A (spawn 2)"
PM->>Scanner: "getListeningPortsForPids(allPids)"
Scanner->>OS: "lsof -p pids -iTCP ... (OR: all TCP listeners)"
note over Interval,OS: After this PR — one shared read per tick
Interval->>PM: tick
PM->>PM: "isSessionDue check (skip idle sessions)"
PM->>Scanner: "getProcessTreesForPids([pidA, pidB, ...])"
Scanner->>OS: "ps -A (ONE spawn for all sessions)"
Scanner-->>PM: "Map{pidA:[...], pidB:[...]}"
PM->>Scanner: "getListeningPortsForPids(allPids)"
Scanner->>OS: "lsof -a -p pids -iTCP ... (AND: only requested PIDs)"
Scanner-->>PM: "PortInfo[]"
PM-->>PM: "updatePortsFromScan (skipped idle sessions untouched)"
%%{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 Interval as "setInterval (2.5s)"
participant PM as PortManager
participant Scanner as "scanner.ts"
participant OS as "OS (ps/lsof)"
note over Interval,OS: Before this PR — per-session ps spawns
Interval->>PM: tick
PM->>Scanner: getProcessTree(pidA)
Scanner->>OS: "ps -A (spawn 1)"
PM->>Scanner: getProcessTree(pidB)
Scanner->>OS: "ps -A (spawn 2)"
PM->>Scanner: "getListeningPortsForPids(allPids)"
Scanner->>OS: "lsof -p pids -iTCP ... (OR: all TCP listeners)"
note over Interval,OS: After this PR — one shared read per tick
Interval->>PM: tick
PM->>PM: "isSessionDue check (skip idle sessions)"
PM->>Scanner: "getProcessTreesForPids([pidA, pidB, ...])"
Scanner->>OS: "ps -A (ONE spawn for all sessions)"
Scanner-->>PM: "Map{pidA:[...], pidB:[...]}"
PM->>Scanner: "getListeningPortsForPids(allPids)"
Scanner->>OS: "lsof -a -p pids -iTCP ... (AND: only requested PIDs)"
Scanner-->>PM: "PortInfo[]"
PM-->>PM: "updatePortsFromScan (skipped idle sessions untouched)"
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
packages/host-service/src/terminal/terminal.ts:1231-1232
**Idle clock not updated on empty-decoded chunks**
`checkOutputForHint` is only called when `hintText.length > 0`. A `StringDecoder` buffers partial multibyte sequences and can return an empty string when a chunk boundary lands mid-sequence, so a terminal actively emitting binary-heavy output could silently fail to refresh `lastActivityAt`. In practice this path is rare (ASCII is immediate), but it creates a subtle divergence with `daemon-manager.ts` where the clock is updated on every data event regardless of decoded length. Consider calling `portManager.checkOutputForHint(terminalId, hintText)` unconditionally (relying on the `if (session) session.lastActivityAt = ...` guard in the method itself for the activity update) and keep the port-hint scheduling conditional on non-empty text internally.
Reviews (1): Last reviewed commit: "perf(port-scanner): shared ps per scan, ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+1231
to
+1232
| if (hintText.length > 0) | ||
| portManager.checkOutputForHint(terminalId, hintText); |
Contributor
There was a problem hiding this comment.
checkOutputForHint is only called when hintText.length > 0. A StringDecoder buffers partial multibyte sequences and can return an empty string when a chunk boundary lands mid-sequence, so a terminal actively emitting binary-heavy output could silently fail to refresh lastActivityAt. In practice this path is rare (ASCII is immediate), but it creates a subtle divergence with daemon-manager.ts where the clock is updated on every data event regardless of decoded length. Consider calling portManager.checkOutputForHint(terminalId, hintText) unconditionally (relying on the if (session) session.lastActivityAt = ... guard in the method itself for the activity update) and keep the port-hint scheduling conditional on non-empty text internally.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/terminal/terminal.ts
Line: 1231-1232
Comment:
**Idle clock not updated on empty-decoded chunks**
`checkOutputForHint` is only called when `hintText.length > 0`. A `StringDecoder` buffers partial multibyte sequences and can return an empty string when a chunk boundary lands mid-sequence, so a terminal actively emitting binary-heavy output could silently fail to refresh `lastActivityAt`. In practice this path is rare (ASCII is immediate), but it creates a subtle divergence with `daemon-manager.ts` where the clock is updated on every data event regardless of decoded length. Consider calling `portManager.checkOutputForHint(terminalId, hintText)` unconditionally (relying on the `if (session) session.lastActivityAt = ...` guard in the method itself for the activity update) and keep the port-hint scheduling conditional on non-empty text internally.
How can I resolve this? If you propose a fix, please make it concise.
Member
Author
There was a problem hiding this comment.
Fixed in 89354db — the hintText.length > 0 guard is gone, so checkOutputForHint runs on every output chunk and refreshes the idle clock even when the decoder buffers a partial codepoint; hint matching on "" is a no-op. Now consistent with v1's daemon-manager.
…nd idle clock - A forceScan arriving while a scan is in flight now keeps its cadence-bypass in the coalesced follow-up (forceRequested flag). - host-service marks terminal activity even when the UTF-8 decoder buffers a partial codepoint and yields an empty string — the chunk is still real output and must refresh the idle clock.
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughPort scanning now batches process-tree discovery, tracks terminal activity, skips non-due idle sessions, and supports forced scans. Terminal output passes session identifiers to hint detection, while scanner and PortManager tests cover bulk discovery, cadence, forced follow-ups, and integration behavior. ChangesTerminal port scanning
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TerminalOutput
participant PortManager
participant ProcessScanner
participant PortScanner
TerminalOutput->>PortManager: checkOutputForHint(terminalId, data)
PortManager->>PortManager: select due or forced sessions
PortManager->>ProcessScanner: getProcessTreesForPids(rootPids)
ProcessScanner-->>PortManager: return descendant PID map
PortManager->>PortScanner: getListeningPortsForPids(pids)
PortScanner-->>PortManager: return listening ports
🚥 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/port-scanner/src/port-manager.test.ts`:
- Around line 616-631: Update the “forceScan arriving mid-scan keeps its bypass
in the coalesced follow-up” test to replace the 10ms and 100ms sleeps with a
deferred barrier in the scanner mock. Await the first scan entering
getListeningPortsForPids, invoke forceScan while it is blocked, release the
barrier, then await both periodic and forced scans deterministically.
In `@packages/port-scanner/src/port-manager.ts`:
- Around line 160-164: Update the existing-session branch in the session upsert
flow so when workspaceId changes, migrate every DetectedPort tracked for
terminalId to the new workspace and emit the corresponding port-change
notifications. Preserve the idle clock by keeping the existing SessionEntry
update behavior, and ensure getPortsByWorkspace and killPort observe the new
workspace assignment.
- Around line 320-324: In the due-session processing loop, validate that the
retrieved session entry still has the same PID as the captured pid before
updating or applying results. Discard the iteration when entry.pid !== pid;
otherwise mark the entry scanned and use entry.workspaceId, rather than the
stale destructured workspaceId, when attributing the PID tree.
In `@packages/port-scanner/src/scanner.test.ts`:
- Around line 94-103: Update the child-process setup in the “includes the root
pid and its descendants from one table read” test to use the Bun executable with
a long-lived JavaScript process instead of invoking the platform-specific sleep
command. Preserve the existing PID assertions and cleanup behavior.
🪄 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: 14c46e8a-aa84-43ed-ba9b-1e609d2dc72b
📒 Files selected for processing (7)
apps/desktop/src/main/lib/terminal/daemon/daemon-manager.tspackages/host-service/src/terminal/terminal.tspackages/port-scanner/src/index.tspackages/port-scanner/src/port-manager.test.tspackages/port-scanner/src/port-manager.tspackages/port-scanner/src/scanner.test.tspackages/port-scanner/src/scanner.ts
Contributor
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
…istic tests CodeRabbit review follow-ups: - Skip a session whose pid changed (or that was unregistered) while the shared table read was in flight — the stale tree must not be attributed to the replacement session. - Remove wall-clock sleeps from the force-coalesce test (isScanning is set synchronously; the follow-up is awaited inside the periodic promise) and gate the pid-swap race test on a promise. - Use process.execPath instead of `sleep` in the tree test (win32).
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
Customer report (validated + reproduced empirically with CDP on a live dev desktop): the port scanner is a major source of background system load, in both v1 and v2 (shared
packages/port-scanner).psevery 2.5s.getProcessTree()used pidtree, whose darwin backend spawnsps -Aper call — one per registered session per tick. Measured: 13 sessions → 35 distinctpsspawns/12s, 26 sessions → 60/12s (lower bounds, ~50% sampler catch rate). Linear scaling confirmed; extrapolates to the customer's ~13/sec at 32 panes.lsofwithout-a. lsof ORs the-pand-iTCPselectors, enumerating every TCP listener on the machine (observed: 721 lines from 56 unrelated processes, zero requested pids) only for the JS-side pid filter to discard everything.stopPeriodicScanIfIdleonly stopped at zero sessions.Fix
getProcessTreesForPids()callspidtree(-1, {advanced: true})once (oneps/wmictotal) and walks the parent/child table in memory for every due session. O(sessions) full-table dumps → O(1). Same shape asreadProcessTable()in pty-daemon.lsof -a: selectors are now ANDed; the pid check stays as defense in depth. (~23% faster measured; the big win is not walking every process's fd table.)checkOutputForHintnow takes the terminalId and doubles as the activity signal. Ports are still detected for silently-daemonized servers, just slower; skipped sessions keep their previously detected ports untouched. Re-upserting the same pid (the reaper's periodic sync of unattached daemon sessions) does not reset the idle clock. Addresses the intent of perf(desktop): increase port scan interval and skip scan when no sessions #3238/perf(desktop): skip periodic port scan for idle sessions with no dev servers #3239.stopPeriodicScanIfIdle→stopPeriodicScanIfNoSessions(it never handled idleness).Steady-state for a fully idle machine: from
sessions × 0.4ps/sec to ~0.03 ps/sec + ~0.03 lsof/sec, regardless of session count.Tests
getProcessTreesForPids(spawned child appears in tree, dead roots omitted) andgetListeningPortsForPids(Bun.serve listener found with-ain place).bun test(port-scanner 65 ✓, desktop daemon-manager 10 ✓, host-service terminal 89 ✓), turbo typecheck on all three packages ✓,bun run lint✓.Summary by CodeRabbit
Bug Fixes
Performance
Tests