fix(cli): stop a second Codex restore after graceful shutdown (rebase of #3164) - #3187
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
✅ Deterministic PR hygiene checks passed. |
![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: 605f0acb78
ℹ️ 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".
| // receipt could not be written, the proxy restores it itself and the caller must not | ||
| // attempt a second restore after a graceful stop. A hard-kill always leaves restore | ||
| // to this process. | ||
| return graceful && !teardownNonce; |
There was a problem hiding this comment.
When the receipt cannot be written and the daemon's own teardown fails, /api/stop still returns HTTP 200 with { success: false } and then exits with status 1, but stopProxyGracefully checks only res.ok and process disappearance. Consequently this expression marks the teardown as handled, blocks the caller-side retry, leaves stopFailed false, and lets ocx stop or an update report success while native Codex or Grok remains pointed at the dead proxy. Parse and propagate the teardown result (or otherwise distinguish an unsuccessful graceful exit), and skip the caller restore only after confirmed teardown success; add a focused behavioral test for the failure response rather than only source-string assertions.
AGENTS.md reference: src/AGENTS.md:L24-L25
Useful? React with 👍 / 👎.
2 participants
Summary
Maintainer rebase of #3164 by @x3M3x onto current
dev— cherry-picked with author credit preserved, no conflicts.A graceful proxy shutdown already performs the shared Codex/Grok teardown, but
ocx stop,ocx update, and related lifecycle commands attempted a second Codex resume-history restore afterwards, so the user saw the restore warning twice. The proxy now records that it did the teardown itself, and the caller skips its own restore in that case. Caller-side restoration is preserved for deferred receipts and the hard-kill fallback, which are the paths where the proxy did not get to do it.Verification
Exact head
605f0acb7:bun test ./tests/grok-lifecycle.test.ts ./tests/process-control-graceful.test.ts ./tests/update-stop-first.test.ts— 54 pass, 0 fail, 347 expect() calls.Full-suite and typecheck coverage is left to CI on this exact head. The original PR reported unrelated Windows timing/ACL and spill-publication failures in
responses-stateand neighbours; those are the known environment-dependent set, not this diff.Checklist
dev