Repository navigation
Conversation
There was a problem hiding this comment.
Review mode: initial
Requirement — Pass
Issue #1939 establishes that a recovered live PID can receive a successful resume response while its Hub session remains disconnected. Reconnecting the existing Codex root preserves an in-flight turn and shared sibling roots.
Evidence
- Issue #1939 describes the observed failure and reproducer; base cli/src/runner/run.ts caches a verified recovered PID as a successful spawn result.
Approach — Pass
The runner verifies shared-runtime ownership and requests a root-scoped Hub reconnect. An unverified or unreachable runtime returns an error without stopping the native execution.
Evidence
- cli/src/runner/reconnectSession.ts:18 verifies ownership and calls hapi/reconnectSession; cli/src/codex/shared/runtime.ts:340 dispatches it to the selected root.
Code — Reviewed
The full fixed-head diff was reviewed. One preflight ordering issue limits recovery for uncached shared roots.
- [Minor] Reconnect uncached roots before checking Codex launch availability —
cli/src/runner/run.ts:652
Testing
Not run (automation; PR code execution is prohibited).
- The author reports passing CLI typecheck and tests plus the opt-in integration test; these were not run during this read-only review.
- At the last CI check, integration and windows-codex-mcp had passed; test and pr-review were still in progress.
HAPI Bot
| childStarted: false, | ||
| }; | ||
| } | ||
| if (agent === 'codex' && options.existingSessionId) { |
There was a problem hiding this comment.
[Minor] Reconnect uncached roots before checking Codex launch availability
For a native /new or fork root with no spawn-cache entry, spawnSessionOnce checks getAgentAvailability before reaching this reconnect branch. If the Runner restarts with a changed PATH or the Codex executable is removed while the shared runtime remains alive, resume returns agent_unavailable even though the existing root can be reconnected through its runtime endpoint.
Suggested fix
Try the verified shared-runtime reconnect before the launch-availability preflight; keep that preflight for paths that actually spawn a new Codex process.
There was a problem hiding this comment.
Fixed in 6e5455b. Workspace authorization remains first; verified live-runtime reconnect now precedes launch availability. Reproduced the reported failure in the real Hub/Runner/native test with a nonexistent HAPI_CODEX_APP_SERVER_BIN, then verified uncached sibling recovery succeeds while an archived/cold root still fails launch preflight. The same test now also restarts its own Hub over the same synthetic database and verifies continued native execution/history/sibling messaging. Follow-up CLI typecheck and 5 files / 70 tests passed; updated-head CI is pending.
There was a problem hiding this comment.
Review mode: follow-up after new commits
Requirement — Pass
Issue #1939 shows that a recovered live PID can acknowledge resume while its Hub session remains disconnected. Reconnecting that root preserves its in-flight turn and shared siblings.
Evidence
- Issue #1939 describes the observed failure and a reproducer against the base deduplicator in cli/src/runner/run.ts.
Approach — Pass
The Runner verifies ownership of the live shared runtime and requests a bounded reconnect for the selected root. It retains the existing execution when reconnect cannot be confirmed.
Evidence
- cli/src/runner/reconnectSession.ts:18 verifies runtime ownership before control; cli/src/codex/shared/runtime.ts:340 dispatches reconnect to the selected root.
- cli/src/runner/run.ts:628 checks workspace authorization before reconnect and checks launch availability before a cold start.
Code — Reviewed
Reviewed the full fixed-head merge-base diff. The earlier launch-preflight ordering finding is addressed; no further actionable defect was established.
No reportable code issues found.
Testing
Not run (automation; PR code execution is prohibited).
- PR code and tests were not run during this read-only review. The author reports passing CLI typecheck and focused tests on the updated head.
- At inspection, integration CI had passed; test, windows-codex-mcp, and the current pr-review job were still in progress.
HAPI Bot
|
I tested this PR at For the reported archived-session failure, this PR did not restore usability: cold reopen still returned HTTP 500 / Further isolation identified Windows process-probe blocking as an additional failure path. In a controlled test with 245 runtime records and a live sibling sharing the wrapper, the old registry scan caused archive HTTP 500, an unresponsive Runner control endpoint, and model RPC That fix is in #2035. #2036 addresses the remaining cold-restore readiness and replay paths. Applying both made the reproduced archive/reopen and messaging workflows pass. #2046 is a subsequent shutdown-performance improvement on top of #2035. |
Problem
Fixes #1939.
A live recovered driver PID is not evidence that its HAPI session is connected. After a Runner restart, the spawn deduplicator returns a cached success for a disconnected Codex root without repairing its Hub transport. The native turn can still be alive, so spawning another driver or killing the wrapper is unsafe, especially with shared sibling roots.
The regression also covers the subsequent archive/reopen path: ending one root must release its cached and durable ownership while the wrapper keeps serving siblings. A late archived webhook must not adopt that old root again.
Changes
hapi/reconnectSessioncontrol. A healthy transport is left intact; a stale transport reconnects only that root's Hub socket. Registry/auth/process-generation checks remain mandatory. Unknown or unreachable ownership fails explicitly and preserves execution./newor fork).ps eww -eo ...exited 1 with no rows here).Verification
On macOS arm64, using source Runner/Hub and installed Codex 0.156.1:
The opt-in test uses a disposable real Hub, real Runner restart with persisted PID recovery, real native Codex engine and Socket.IO, and a loopback fake Responses server (no production credentials/model requests). It holds a native turn open, disconnects its Hub socket, waits for real Hub liveness expiry, resumes via the Web REST endpoint, verifies that exact turn completes once, then continues both conversations. It repeats recovery for an uncached sibling and archives/cold-reopens the primary, checking durable ownership contains only the new PID. The new regression failed before cleanup/adoption fixes.
Deterministic tests additionally cover stop-ACK/orphan-sweep re-registration races, reconnect deadlines, invalid runtime replies, healthy transport preservation, and close/archive racing recovery.
Follow-up validation
The initial head passed all upstream CI jobs (test, integration, windows-codex-mcp, pr-review). The review's launch-preflight ordering finding was reproduced in the real integration test with a nonexistent
HAPI_CODEX_APP_SERVER_BIN: uncached sibling resume failed withcodex has invalid runner configuration. After reordering, existing roots reconnect while an archived/cold root still correctly fails launch preflight. A subsequent valid Runner can cold-resume it.The test now owns a private Hub and restarts it over the same temporary database with a native turn held open. Both root sockets disconnect, reconnect to the new Hub, preserve prior history, complete the held model request without executing it twice, and accept new sibling messages. This verifies process restart, not just socket disconnect. It does not count rendered/UI reply duplicates.
Follow-up: CLI typecheck and
git diff --checkpass; 5 files / 70 tests pass (reconnect.integration,agentAvailability,buildCliArgs,reconnectSession,sessionStopGuard). Independent read-only review found no remaining blocker. The full 2,971-test run above is evidence for the initial head; updated-head CI runs separately.Scope / limitations