fix(gateway): terminate owned process before retry to prevent port conflict on Windows#724
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a7a640297
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
electron/gateway/manager.ts
Outdated
| if (this.process && this.ownsProcess) { | ||
| logger.info('Terminating owned Gateway process before retry...'); | ||
| await terminateOwnedGatewayProcess(this.process).catch(() => {}); | ||
| this.process = null; |
There was a problem hiding this comment.
terminateOwnedProcess clears this.process immediately after terminateOwnedGatewayProcess, but that helper can resolve via its timeout path even when the child is still alive (see electron/gateway/supervisor.ts timeout branch). In that case, retries proceed while the old gateway may still hold the port, but subsequent cleanup cannot target it because this.process is now null (later retry hooks become no-ops and stop() also loses the handle). This can leave startup stuck in repeated handshake/port conflicts on the very failure mode this change is trying to recover from.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aff4cdcbb9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (hooks.shouldWaitForPortFree) { | ||
| await hooks.waitForPortFree(hooks.port); |
There was a problem hiding this comment.
The new retry path on Windows awaits waitForPortFree here without any lifecycle assertion, so a superseding stop()/restart() can be delayed for the full port-wait timeout (default 30s) before the next loop iteration notices cancellation. Because startLock stays true during that wait, deferred restarts and manual start requests are blocked noticeably longer than before this change. Add a lifecycle check immediately before/after this wait (or make it abortable) so superseded startup flows exit promptly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6439e7e66
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| hooks.assertLifecycle('start/retry-pre-port-wait'); | ||
| // Wait for port to become free before retrying (handles lingering processes) | ||
| if (hooks.shouldWaitForPortFree) { | ||
| await hooks.waitForPortFree(hooks.port); |
There was a problem hiding this comment.
The new retry branch now blocks on waitForPortFree before continuing, but this wait is still non-abortable, so a stop()/deferred restart() that arrives during this await cannot be acted on until the port wait finishes (up to 30s). Fresh evidence: waitForPortFree in electron/gateway/supervisor.ts polls with setTimeout and has no cancellation hook, so the post-wait lifecycle assert runs only after the full wait. While blocked, startLock stays true and manual start() requests are dropped, which can leave users stuck in a stopped state after a quick stop/start sequence.
Useful? React with 👍 / 👎.
shuang5725
force-pushed
the
fix/gateway-restart-port-conflict
branch
from
c6439e7 to
f5ef2ce
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5ef2ce5e0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| logger.info('Terminating owned Gateway process before retry...'); | ||
| const proc = this.process; | ||
| const pid = proc.pid; | ||
| await terminateOwnedGatewayProcess(proc).catch(() => {}); |
There was a problem hiding this comment.
This new retry path invokes terminateOwnedGatewayProcess(proc) for each transient startup retry, but that helper resolves on timeout even when the child is still alive and does not remove the once('exit') listener it registered in electron/gateway/supervisor.ts. In the failure mode this change targets (process not exiting promptly), repeated retries against the same process will stack listeners and eventually trigger MaxListenersExceededWarning while retaining unnecessary closures. Please clean up the listener on timeout (or avoid re-calling termination on the same still-running child without listener cleanup).
Useful? React with 👍 / 👎.
The retry branch in startup-orchestrator.ts was blocking on waitForPortFree for up to 30 seconds, preventing stop()/restart() requests from being acted upon until the port wait completed. This left users stuck in a stopped state after quick stop/start sequences. Changes: - Add AbortSignal parameter to waitForPortFree in supervisor.ts - Implement 500ms lifecycle polling in startup-orchestrator.ts retry branch - Pass signal parameter through manager.ts hook implementation The port wait now aborts within 500ms when lifecycle changes are detected, instead of blocking for the full 30-second timeout. Fixes code review feedback from chatgpt-codex-connector bot
When terminateOwnedGatewayProcess times out before the process exits,
the once('exit') listener was not being removed. In retry scenarios
where the same process is terminated multiple times, this caused
listener accumulation and MaxListenersExceededWarning.
Now explicitly remove the exit listener when the timeout fires to
prevent memory leaks and listener warnings during repeated retries.
Addresses code review feedback from chatgpt-codex-connector bot
shuang5725
force-pushed
the
fix/gateway-restart-port-conflict
branch
from
91d6ebc to
f672906
Compare
|
📝 Commit History Commit Description1 ff829b3 Terminate owned process before retry to prevent port conflicts |
|
Hi @su8su This PR is ready for review. It fixes the Gateway restart response delay issue identified by the code review bot. Key changes: Made port wait abortable (30s → <1s response) |
1 participant
Problem
On Windows, when the Gateway connect handshake times out, the previously spawned Gateway process may still be holding port 18789. The retry attempt then fails again because the port is still occupied, requiring manual intervention.
Solution
terminateOwnedProcesshook toStartupHooksinterface instartup-orchestrator.tsterminateOwnedProcessto kill the previously spawned process, then waits for the port to become freeterminateOwnedProcessinmanager.tsusing the existingterminateOwnedGatewayProcessutilityFiles Changed
electron/gateway/startup-orchestrator.tselectron/gateway/manager.tsTesting
Verified that after a handshake timeout, the Gateway automatically recovers on the next retry without requiring the user to manually kill the process or restart the app.