fix worker restart port cleanup on Windows

[cccsaber]

No description provided.

[greptile-apps]

Greptile Summary

This PR fixes a Windows-specific race condition where a restarting worker leaves a ghost TCP socket on its port, blocking the successor from binding. It adds PowerShell/netstat-based port-owner detection, a taskkill /T /F force-eviction path for stale workers, and platform-aware delays in the HTTP server close sequence.

• HealthMonitor.ts: New getWindowsPortOwnerPids (PowerShell + netstat fallback), isLikelyClaudeMemWorkerCommand heuristic, forceKillStaleWorkerPortOwners, and waitForPortFree; called from both the CLI stop/restart commands and worker-spawner.ts.
• GracefulShutdown.ts / Server.ts: Both now apply a 50 ms pre-close delay, a 3 s Windows / 1 s non-Windows timeout race, and a 500 ms post-close wait on Windows to allow the OS to reclaim the socket before the successor spawns.
• tests/infrastructure/health-monitor.test.ts: New test file covering isPortInUse, waitForHealth, waitForPortFree, checkVersionMatch, and the worker-command heuristic.

Confidence Score: 4/5

Safe to merge; the new Windows cleanup path is gated by process.platform === 'win32' and non-Windows behavior is unchanged.

The core logic — PowerShell/netstat detection, process identity heuristic, and taskkill force-eviction — is sound and well-guarded. The issues found are a duplicate server-close implementation across two files and one exported function with no callers, neither of which affects runtime correctness.

HealthMonitor.ts deserves a second look for the unused isWindowsPortHeldByMissingProcess export and the waitForPortFree boundary edge case. The parallel close logic in GracefulShutdown.ts and Server.ts should be reconciled before future modifications to the Windows delay values.

Important Files Changed

Filename	Overview
src/services/infrastructure/HealthMonitor.ts	Core of the Windows fix: adds PowerShell/netstat port-owner detection, process-identity heuristics, and taskkill-based force-eviction for stale workers. Logic is sound; minor issues: isWindowsPortHeldByMissingProcess is exported but has no callers, and waitForPortFree can miss a free port at the exact timeout boundary.
src/services/infrastructure/GracefulShutdown.ts	Adds Windows-specific HTTP server close delays (50 ms pre-delay, 3 s timeout, 500 ms post-close wait). Logic is correct, but the identical code block also appears in Server.ts's new close() method, creating a two-site maintenance surface.
src/services/server/Server.ts	Adds close() method and getHttpServer() accessor with the same Windows-specific close logic as GracefulShutdown.ts; functionally correct but duplicates the platform-specific delay constants.
src/services/worker-service.ts	Updated stop and restart commands to call forceKillStaleWorkerPortOwners + waitForPortFree when the port fails to free after graceful shutdown. Orchestration logic looks correct.
src/services/worker-spawner.ts	Integrates forceKillStaleWorkerPortOwners into the spawn-path fallback so a stale Windows ghost socket is cleared before attempting a new spawn. Clean addition with no obvious issues.
tests/infrastructure/health-monitor.test.ts	New test file covering isPortInUse, isLikelyClaudeMemWorkerCommand, waitForHealth, waitForPortFree, getInstalledPluginVersion, and checkVersionMatch. The most complex new additions (forceKillStaleWorkerPortOwners, getWindowsPortOwnerPids) have no unit tests, which is understandable given the Windows-only execution path but leaves parsing and kill-decision logic unverified.
src/utils/logger.ts	Minor: SHUTDOWN added to the Component enum, used in new shutdown log calls.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[CLI: worker restart] --> B[httpShutdown port reason=restart]
    B --> C{Shutdown accepted?}
    C -- Yes --> D[verifyRestartedWorker primary handoff path]
    D -- ok --> E[exit 0]
    D -- timeout --> F[Fallback: forceKillStaleWorkerPortOwners]
    C -- No --> F
    F --> G{Killed stale owner?}
    G -- Yes --> H[waitForPortFree]
    G -- No --> I{handoffSawLiveWorker?}
    H --> J[spawnDaemon successor]
    I -- No --> K[waitForPortFree 15s]
    I -- Yes --> L[Skip wait - port held by live worker]
    K --> J
    L --> M[verifyRestartedWorker]
    J --> M
    M -- ok --> E
    M -- fail --> N[exit 1]

    subgraph GracefulShutdown_closeHttpServer
        O[server.close] --> P[50ms delay]
        P --> Q[closeIdleConnections + closeAllConnections]
        Q --> R{Race: closed vs 3s/1s timeout}
        R -- closed --> S{Windows?}
        R -- timeout --> T[closeAllConnections again]
        S -- Yes --> U[500ms OS socket reclaim wait]
        S -- No --> V[Done]
        T --> V
        U --> V
    end

Loading

%%{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"}}}%%
flowchart TD
    A[CLI: worker restart] --> B[httpShutdown port reason=restart]
    B --> C{Shutdown accepted?}
    C -- Yes --> D[verifyRestartedWorker primary handoff path]
    D -- ok --> E[exit 0]
    D -- timeout --> F[Fallback: forceKillStaleWorkerPortOwners]
    C -- No --> F
    F --> G{Killed stale owner?}
    G -- Yes --> H[waitForPortFree]
    G -- No --> I{handoffSawLiveWorker?}
    H --> J[spawnDaemon successor]
    I -- No --> K[waitForPortFree 15s]
    I -- Yes --> L[Skip wait - port held by live worker]
    K --> J
    L --> M[verifyRestartedWorker]
    J --> M
    M -- ok --> E
    M -- fail --> N[exit 1]

    subgraph GracefulShutdown_closeHttpServer
        O[server.close] --> P[50ms delay]
        P --> Q[closeIdleConnections + closeAllConnections]
        Q --> R{Race: closed vs 3s/1s timeout}
        R -- closed --> S{Windows?}
        R -- timeout --> T[closeAllConnections again]
        S -- Yes --> U[500ms OS socket reclaim wait]
        S -- No --> V[Done]
        T --> V
        U --> V
    end

Loading

Comments Outside Diff (2)

1. src/services/infrastructure/GracefulShutdown.ts, line 60-96 (link)

   Duplicate HTTP server close logic

   closeHttpServer in GracefulShutdown.ts and Server.close() in Server.ts now contain virtually identical blocks: 50 ms pre-delay → closeIdleConnections → closeAllConnections → 3 s/1 s timeout race → 500 ms Windows post-delay. Both code paths were updated in this PR, but any future change to this logic (e.g., adjusting the Windows post-delay) will need to be applied to both sites. Extracting the shared logic into a single helper (e.g., closeHttpServerWithWindowsCleanup(server)) would eliminate the drift risk.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: src/services/infrastructure/GracefulShutdown.ts
   Line: 60-96
   
   Comment:
   **Duplicate HTTP server close logic**
   
   `closeHttpServer` in `GracefulShutdown.ts` and `Server.close()` in `Server.ts` now contain virtually identical blocks: 50 ms pre-delay → `closeIdleConnections` → `closeAllConnections` → 3 s/1 s timeout race → 500 ms Windows post-delay. Both code paths were updated in this PR, but any future change to this logic (e.g., adjusting the Windows post-delay) will need to be applied to both sites. Extracting the shared logic into a single helper (e.g., `closeHttpServerWithWindowsCleanup(server)`) would eliminate the drift risk.
   
   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!

2. src/services/infrastructure/HealthMonitor.ts, line 290-297 (link)

   waitForPortFree can miss a free port right at the timeout boundary

   The while condition is evaluated at the start of each iteration, so a port that becomes free in the final ~500 ms polling window (between the last successful check and the moment elapsed >= timeoutMs) causes the function to return false even though the port is free. In practice the generous caller timeouts (5–15 s) make this vanishingly unlikely, but a final check after the loop exits would close the gap entirely.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: src/services/infrastructure/HealthMonitor.ts
   Line: 290-297
   
   Comment:
   **`waitForPortFree` can miss a free port right at the timeout boundary**
   
   The `while` condition is evaluated at the start of each iteration, so a port that becomes free in the final ~500 ms polling window (between the last successful check and the moment elapsed >= `timeoutMs`) causes the function to return `false` even though the port is free. In practice the generous caller timeouts (5–15 s) make this vanishingly unlikely, but a final check after the loop exits would close the gap entirely.
   
   How can I resolve this? If you propose a fix, please make it concise.

Prompt To Fix All With AI

Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 3
src/services/infrastructure/HealthMonitor.ts:141-154
**Exported but unreachable function**

`isWindowsPortHeldByMissingProcess` is exported from `HealthMonitor.ts` but has no callers anywhere in the codebase (confirmed via grep). It will never be invoked, so its logic — which returns `false` at the first PID that is either the current process or is still alive, without evaluating remaining PIDs — is never exercised. If this function is intended as a future entry point, leaving it unused now risks it being wired up against the wrong semantics: on a multi-PID result, the presence of any one live PID causes it to say "not held by a missing process", even if another PID in the list is a ghost. If it isn't needed, removing it would reduce surface area.

### Issue 2 of 3
src/services/infrastructure/GracefulShutdown.ts:60-96
**Duplicate HTTP server close logic**

`closeHttpServer` in `GracefulShutdown.ts` and `Server.close()` in `Server.ts` now contain virtually identical blocks: 50 ms pre-delay → `closeIdleConnections` → `closeAllConnections` → 3 s/1 s timeout race → 500 ms Windows post-delay. Both code paths were updated in this PR, but any future change to this logic (e.g., adjusting the Windows post-delay) will need to be applied to both sites. Extracting the shared logic into a single helper (e.g., `closeHttpServerWithWindowsCleanup(server)`) would eliminate the drift risk.

### Issue 3 of 3
src/services/infrastructure/HealthMonitor.ts:290-297
**`waitForPortFree` can miss a free port right at the timeout boundary**

The `while` condition is evaluated at the start of each iteration, so a port that becomes free in the final ~500 ms polling window (between the last successful check and the moment elapsed >= `timeoutMs`) causes the function to return `false` even though the port is free. In practice the generous caller timeouts (5–15 s) make this vanishingly unlikely, but a final check after the loop exits would close the gap entirely.

_{Reviews (1): Last reviewed commit: "fix worker restart port cleanup on Windo..." | Re-trigger Greptile}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bc2ef6c651

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Preserve unverified restart successors

On Windows restart fallback, this force-kill runs before waiting for the port to free, and forceKillStaleWorkerPortOwners will also kill any claude-mem-looking owner whose PID is not oldPid by command-line match. If the old worker's handoff or another launcher has already spawned a successor that has bound the port but has not answered /api/health yet, this kills the legitimate replacement instead of waiting/verifying it, turning a slow restart into a failed one. Consider waiting first or only force-killing the captured oldPid in this path.

Useful? React with 👍 / 👎.

[greptile-apps]

Exported but unreachable function

isWindowsPortHeldByMissingProcess is exported from HealthMonitor.ts but has no callers anywhere in the codebase (confirmed via grep). It will never be invoked, so its logic — which returns false at the first PID that is either the current process or is still alive, without evaluating remaining PIDs — is never exercised. If this function is intended as a future entry point, leaving it unused now risks it being wired up against the wrong semantics: on a multi-PID result, the presence of any one live PID causes it to say "not held by a missing process", even if another PID in the list is a ghost. If it isn't needed, removing it would reduce surface area.

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/services/infrastructure/HealthMonitor.ts
Line: 141-154

Comment:
**Exported but unreachable function**

`isWindowsPortHeldByMissingProcess` is exported from `HealthMonitor.ts` but has no callers anywhere in the codebase (confirmed via grep). It will never be invoked, so its logic — which returns `false` at the first PID that is either the current process or is still alive, without evaluating remaining PIDs — is never exercised. If this function is intended as a future entry point, leaving it unused now risks it being wired up against the wrong semantics: on a multi-PID result, the presence of any one live PID causes it to say "not held by a missing process", even if another PID in the list is a ghost. If it isn't needed, removing it would reduce surface area.

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!

[thedotmack]

Closing as superseded by the newer Windows worker/port recovery work in #3009 and #3219. This older branch overlaps heavily with the current supervisor path.