perf(desktop): batch port scans per interval#1134
Merged
Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughReplaces per-pane incremental port scans with a batched global scan: collect PIDs from regular and daemon sessions, query listening ports once for all PIDs, map results back to panes, and apply self‑healing and scheduled hint-scan logic. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant PM as PortManager
participant RS as RegularSessions
participant DS as DaemonSessions
participant PS as PortScanner
participant Pane as PaneManager
rect rgba(100,149,237,0.5)
RS->>PM: report child PIDs
DS->>PM: report daemon PIDs
end
PM->>PM: build allPids set\nand pidOwnerMap
PM->>PS: getListeningPortsForPids(allPids)
PS-->>PM: return PortInfo[] (batched)
PM->>PM: build portsByPane from PortInfo[]\nusing pidOwnerMap
PM->>Pane: update panes with portsByPane
Pane-->>PM: acknowledge updates
alt pane has empty process tree
PM->>Pane: remove ports (self-heal)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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
🤖 Fix all issues with AI agents
In `@apps/desktop/src/main/lib/terminal/port-manager.ts`:
- Around line 247-270: The catch in collectPidTree silently swallows errors;
change it to log the error and context instead of doing nothing: when
getProcessTree(pid) or subsequent steps throw, call the project logger (e.g.,
processLogger or an existing logger) to record an informative message including
paneId, workspaceId, pid and the error object, while preserving the current
behavior for known non-fatal cases (session-exited). Update collectPidTree to
log the exception before returning, so failures in getProcessTree, scanState
updates, or addPanePids are visible for debugging.
- Around line 149-160: In scheduleHintScan, don't swallow errors from scanPane
with an empty catch; instead catch the error and call the appropriate logger (or
processLogger) to record the failure along with context (paneId and
HINT_SCAN_DELAY_MS or that it was from scheduleHintScan). Update the setTimeout
handler in scheduleHintScan to use .catch(err => { /* log error with paneId and
context */ }) referencing the existing scanPane(paneId) call and
pendingHintScans so failures are visible in logs.
- Around line 162-185: The console.error call in scanPidTreeAndUpdate does not
follow the required "[domain/operation] message" log prefix; replace the current
console.error(`[PortManager] Error scanning ${errorContext}:`, error) with a
prefixed error log that uses the pattern "[port-manager/scanPidTree] ..." (or
similar domain/operation name) and include the error and errorContext in the
message so it matches the project's logging convention; update the call inside
the catch block of scanPidTreeAndUpdate accordingly.
Comment on lines
+149
to
+160
| private scheduleHintScan(paneId: string): void { | ||
| this.clearPendingHintScan(paneId); | ||
|
|
||
| const timeout = setTimeout(() => { | ||
| this.pendingHintScans.delete(paneId); | ||
| this.scanPane(paneId).catch(() => {}); | ||
| }, HINT_SCAN_DELAY_MS); | ||
| // Don't keep Electron alive just for port scanning | ||
| timeout.unref(); | ||
|
|
||
| this.pendingHintScans.set(paneId, timeout); | ||
| } |
Contributor
There was a problem hiding this comment.
Don’t swallow hint-scan failures silently.
The empty catch hides scan failures; please log with context.
Suggested fix
- this.scanPane(paneId).catch(() => {});
+ this.scanPane(paneId).catch((error) => {
+ console.error(
+ `[port-manager/hint-scan] Failed to scan pane ${paneId}:`,
+ error,
+ );
+ });As per coding guidelines: Never silently swallow errors with catch(() => {}) or catch(e) { return null }.
📝 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.
Suggested change
| private scheduleHintScan(paneId: string): void { | |
| this.clearPendingHintScan(paneId); | |
| const timeout = setTimeout(() => { | |
| this.pendingHintScans.delete(paneId); | |
| this.scanPane(paneId).catch(() => {}); | |
| }, HINT_SCAN_DELAY_MS); | |
| // Don't keep Electron alive just for port scanning | |
| timeout.unref(); | |
| this.pendingHintScans.set(paneId, timeout); | |
| } | |
| private scheduleHintScan(paneId: string): void { | |
| this.clearPendingHintScan(paneId); | |
| const timeout = setTimeout(() => { | |
| this.pendingHintScans.delete(paneId); | |
| this.scanPane(paneId).catch((error) => { | |
| console.error( | |
| `[port-manager/hint-scan] Failed to scan pane ${paneId}:`, | |
| error, | |
| ); | |
| }); | |
| }, HINT_SCAN_DELAY_MS); | |
| // Don't keep Electron alive just for port scanning | |
| timeout.unref(); | |
| this.pendingHintScans.set(paneId, timeout); | |
| } |
🤖 Prompt for AI Agents
In `@apps/desktop/src/main/lib/terminal/port-manager.ts` around lines 149 - 160,
In scheduleHintScan, don't swallow errors from scanPane with an empty catch;
instead catch the error and call the appropriate logger (or processLogger) to
record the failure along with context (paneId and HINT_SCAN_DELAY_MS or that it
was from scheduleHintScan). Update the setTimeout handler in scheduleHintScan to
use .catch(err => { /* log error with paneId and context */ }) referencing the
existing scanPane(paneId) call and pendingHintScans so failures are visible in
logs.
Comment on lines
+162
to
+185
| private async scanPidTreeAndUpdate({ | ||
| paneId, | ||
| workspaceId, | ||
| pid, | ||
| errorContext, | ||
| }: { | ||
| paneId: string; | ||
| workspaceId: string; | ||
| pid: number; | ||
| errorContext: string; | ||
| }): Promise<void> { | ||
| try { | ||
| const pids = await getProcessTree(pid); | ||
| if (pids.length === 0) { | ||
| this.removePortsForPane(paneId); | ||
| return; | ||
| } | ||
|
|
||
| const portInfos = await getListeningPortsForPids(pids); | ||
| this.updatePortsForPane({ paneId, workspaceId, portInfos }); | ||
| } catch (error) { | ||
| console.error(`[PortManager] Error scanning ${errorContext}:`, error); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Use the required log prefix format here.
The current message doesn’t follow the [domain/operation] message pattern.
Suggested fix
- console.error(`[PortManager] Error scanning ${errorContext}:`, error);
+ console.error(
+ `[port-manager/scanPidTreeAndUpdate] Error scanning ${errorContext}:`,
+ error,
+ );As per coding guidelines: Use prefixed console logging with pattern [domain/operation] message for all logging.
📝 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.
Suggested change
| private async scanPidTreeAndUpdate({ | |
| paneId, | |
| workspaceId, | |
| pid, | |
| errorContext, | |
| }: { | |
| paneId: string; | |
| workspaceId: string; | |
| pid: number; | |
| errorContext: string; | |
| }): Promise<void> { | |
| try { | |
| const pids = await getProcessTree(pid); | |
| if (pids.length === 0) { | |
| this.removePortsForPane(paneId); | |
| return; | |
| } | |
| const portInfos = await getListeningPortsForPids(pids); | |
| this.updatePortsForPane({ paneId, workspaceId, portInfos }); | |
| } catch (error) { | |
| console.error(`[PortManager] Error scanning ${errorContext}:`, error); | |
| } | |
| } | |
| private async scanPidTreeAndUpdate({ | |
| paneId, | |
| workspaceId, | |
| pid, | |
| errorContext, | |
| }: { | |
| paneId: string; | |
| workspaceId: string; | |
| pid: number; | |
| errorContext: string; | |
| }): Promise<void> { | |
| try { | |
| const pids = await getProcessTree(pid); | |
| if (pids.length === 0) { | |
| this.removePortsForPane(paneId); | |
| return; | |
| } | |
| const portInfos = await getListeningPortsForPids(pids); | |
| this.updatePortsForPane({ paneId, workspaceId, portInfos }); | |
| } catch (error) { | |
| console.error( | |
| `[port-manager/scanPidTreeAndUpdate] Error scanning ${errorContext}:`, | |
| error, | |
| ); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@apps/desktop/src/main/lib/terminal/port-manager.ts` around lines 162 - 185,
The console.error call in scanPidTreeAndUpdate does not follow the required
"[domain/operation] message" log prefix; replace the current
console.error(`[PortManager] Error scanning ${errorContext}:`, error) with a
prefixed error log that uses the pattern "[port-manager/scanPidTree] ..." (or
similar domain/operation name) and include the error and errorContext in the
message so it matches the project's logging convention; update the call inside
the catch block of scanPidTreeAndUpdate accordingly.
Comment on lines
+247
to
+270
| private async collectPidTree({ | ||
| paneId, | ||
| workspaceId, | ||
| pid, | ||
| scanState, | ||
| }: { | ||
| paneId: string; | ||
| workspaceId: string; | ||
| pid: number; | ||
| scanState: ScanState; | ||
| }): Promise<void> { | ||
| try { | ||
| const panePortMap = new Map< | ||
| string, | ||
| { workspaceId: string; pids: number[] } | ||
| >(); | ||
| // Track panes with empty process trees for self-healing | ||
| const emptyTreePanes = new Set<string>(); | ||
|
|
||
| // Scan regular sessions | ||
| for (const [paneId, { session, workspaceId }] of this.sessions) { | ||
| if (!session.isAlive) continue; | ||
|
|
||
| try { | ||
| const pid = session.pty.pid; | ||
| const pids = await getProcessTree(pid); | ||
| if (pids.length > 0) { | ||
| panePortMap.set(paneId, { workspaceId, pids }); | ||
| } else { | ||
| // Process tree is gone - mark for self-healing | ||
| emptyTreePanes.add(paneId); | ||
| } | ||
| } catch { | ||
| // Session may have exited | ||
| } | ||
| const pids = await getProcessTree(pid); | ||
| if (pids.length === 0) { | ||
| scanState.emptyTreePanes.add(paneId); | ||
| return; | ||
| } | ||
|
|
||
| // Scan daemon sessions | ||
| for (const [paneId, { workspaceId, pid }] of this.daemonSessions) { | ||
| // Skip if PID not yet available | ||
| if (pid === null) continue; | ||
|
|
||
| try { | ||
| const pids = await getProcessTree(pid); | ||
| if (pids.length > 0) { | ||
| panePortMap.set(paneId, { workspaceId, pids }); | ||
| } else { | ||
| // Process tree is gone - mark for self-healing | ||
| emptyTreePanes.add(paneId); | ||
| } | ||
| } catch { | ||
| // Session may have exited | ||
| } | ||
| } | ||
| scanState.panePortMap.set(paneId, { workspaceId, pids }); | ||
| this.addPanePids({ paneId, workspaceId, pids, scanState }); | ||
| } catch { | ||
| // Session may have exited | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Log PID-tree collection failures instead of swallowing.
The empty catch hides errors and makes diagnosing scan gaps difficult.
Suggested fix
- } catch {
- // Session may have exited
- }
+ } catch (error) {
+ console.error(
+ `[port-manager/collectPidTree] Failed to collect PID tree for pane ${paneId} (pid ${pid}):`,
+ error,
+ );
+ }As per coding guidelines: Never silently swallow errors with catch(() => {}) or catch(e) { return null }.
🤖 Prompt for AI Agents
In `@apps/desktop/src/main/lib/terminal/port-manager.ts` around lines 247 - 270,
The catch in collectPidTree silently swallows errors; change it to log the error
and context instead of doing nothing: when getProcessTree(pid) or subsequent
steps throw, call the project logger (e.g., processLogger or an existing logger)
to record an informative message including paneId, workspaceId, pid and the
error object, while preserving the current behavior for known non-fatal cases
(session-exited). Update collectPidTree to log the exception before returning,
so failures in getProcessTree, scanState updates, or addPanePids are visible for
debugging.
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
1 participant
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.
Summary\n- batch port scans into a single lsof/netstat call per interval\n- map port results back to panes via pid ownership\n- preserve existing hint-driven and per-pane update behavior\n\n## Testing\n- not run (not requested)\n
Summary by CodeRabbit
Performance Improvements
New Features
Bug Fixes