refactor: deduplicate trackPidForPort variants#2494
Conversation
Extract shared try/catch + file-read pattern from trackPidForPort (async) and trackPidForPortSync into a common trackPidForPortCore helper. Closes #2480 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@copilot address the review feedback |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
This PR refactors src/pid-tracker.ts to reduce duplication between the async and sync PID lookup entry points that read /proc/net/tcp and resolve the owning process.
Changes:
- Adds a new
trackPidForPortCorehelper for the shared sync-style read/error-handling flow. - Reworks
trackPidForPortto perform an async read first and fall back to the new core on error. - Updates the function ordering/comments around the async and sync PID-tracking APIs.
Show a summary per file
| File | Description |
|---|---|
src/pid-tracker.ts |
Extracts shared PID-tracking logic into a helper and rewires the async/sync entry points around it. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
src/pid-tracker.ts:400
- This catch now handles failures from
resolvePidFromTcpContent(...)as well asfsPromises.readFile(...). If the parse/lookup path ever throws, the async API silently retries the whole operation with synchronous I/O, which both blocks the event loop and can return a different result because/procmay have changed between the two attempts.
} catch (err) {
// Fall back to the synchronous core for consistent error handling
return trackPidForPortCore(
(p) => fs.readFileSync(p, 'utf-8'),
src/pid-tracker.ts:400
- The new fallback branch in the async implementation is not covered by tests. The existing
trackPidForPorttests only exercise the happy path and the case where both reads fail, so they would not catch regressions in the newcatchbehavior that retries synchronously and potentially changes the returned error/result.
} catch (err) {
// Fall back to the synchronous core for consistent error handling
return trackPidForPortCore(
(p) => fs.readFileSync(p, 'utf-8'),
- Files reviewed: 1/1 changed files
- Comments generated: 1
| // Read asynchronously, then delegate to the shared core. | ||
| // The core uses a synchronous reader callback, so we pre-read here. | ||
| try { | ||
| // Read /proc/net/tcp | ||
| const tcpPath = path.join(procPath, 'net', 'tcp'); | ||
| let tcpContent: string; | ||
|
|
||
| try { | ||
| tcpContent = fs.readFileSync(tcpPath, 'utf-8'); | ||
| } catch (err) { | ||
| return { | ||
| pid: -1, | ||
| cmdline: 'unknown', | ||
| comm: 'unknown', | ||
| error: `Failed to read ${tcpPath}: ${err}`, | ||
| }; | ||
| } | ||
|
|
||
| const tcpContent = await fsPromises.readFile(tcpPath, 'utf-8'); | ||
| return resolvePidFromTcpContent(tcpContent, srcPort, procPath); |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Fixed in acd380e. The ESLint failure was Removed |
|
@copilot address the review feedback |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Done in e6a965d. Extracted |
This comment has been minimized.
This comment has been minimized.
|
Smoke Test Results ✅ PASS
|
|
Smoke Test: Copilot BYOK — Running in BYOK offline mode (
PR by @lpcox · reviewer:
|
🔬 Smoke Test Results
PR: "refactor: deduplicate trackPidForPort variants" — author: @lpcox, reviewer: Overall: PARTIAL — MCP ✅, pre-step data unavailable (workflow template variables were not substituted before agent invocation).
|
This comment has been minimized.
This comment has been minimized.
1 similar comment
Smoke TestPRs: Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "registry.npmjs.org"See Network Configuration for more information.
|
Chroot Version Comparison Results
Result: FAILED — Python and Node.js versions differ between host and chroot environments.
|
Smoke Test Results
Overall: FAIL —
|
Summary
Extract the shared try/catch + file-read + error-handling pattern from
trackPidForPort(async) andtrackPidForPortSyncinto a commontrackPidForPortCorehelper insrc/pid-tracker.ts.Before
Both functions had ~28 lines of identical error-handling and return-value boilerplate, differing only in
fs.readFileSyncvsawait fsPromises.readFile.After
trackPidForPortCoreaccepts a file-reader callback and encapsulates the shared structure. The sync variant delegates directly; the async variant pre-reads the file then falls back to the core for error handling.Testing
Closes #2480