Repository navigation
fix(codex): stabilize cold restore and deduplicate history acknowledgements - #2036
techotaku39 wants to merge 3 commits into
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review mode: initial
Requirement — Pass
Reducing cold-restore delays and redundant acknowledgements addresses a clear reliability problem in supported Codex resume flows.
Evidence
- The snapshot PR description identifies registration delays, repeated ownership probes, and 601 acknowledgement events for a 200-turn replay.
- docs/guide/codex-shared-sessions.md documents cold resume, durable history reconciliation, and process-generation protection.
Suggestions
- Correct the related issue reference: #1887 concerns token-usage dashboard memory exhaustion, rather than Codex restoration.
Approach — Pass
Early control registration, paged history retrieval, batched consumption, and consolidated Windows process checks are viable directions. The defects below can be fixed within this approach.
Evidence
- cli/src/codex/shared/root.ts:212 confirms registration before history projection and gates controls on readiness.
- cli/src/codex/shared/runtime.ts:376 requests resume without embedded turns.
- cli/src/codex/shared/queue.ts:80 persists historical consumption in one batch.
- cli/src/codex/shared/registry.ts:85 uses one Windows process snapshot while retaining generation checks.
Code — Reviewed
Reviewed the entire fixed-head merge-base diff. Found three regressions affecting queued-state correctness and startup reliability.
- [Major] Agent output remains permanently uninvoked —
cli/src/codex/shared/projection.ts:60 - [Major] Emit the history acknowledgement after transcript insertion —
cli/src/codex/shared/projection.ts:213 - [Major] Wait for the initial socket connection before registering —
cli/src/api/rpc/RpcHandlerManager.ts:65
Testing
Not run (automation; PR code execution is prohibited).
- Observed CI: drift-gate, integration, and windows-codex-mcp succeeded; test remained in progress at the last check.
- Added acknowledgement tests count emitted events but do not verify resulting hub invocation state, pagination, or fork availability. Registration tests do not cover a delayed initial connection.
- No PR code, tests, builds, or scripts were executed. Validation reported in the PR description was not independently reproduced.
HAPI Bot
| type: 'agent-run-trace', agentId: this.threadId, cardId: `codex-agent:${this.threadId}`, message: { ...body, id }, id, | ||
| scope: { role: 'child', threadId: this.threadId, parentThreadId: this.parentThreadId }, scope_role: 'child' | ||
| } : { ...body, id }, id); | ||
| } : { ...body, id }, id, { acknowledgeConsumption: false }); |
There was a problem hiding this comment.
[Major] Agent output remains permanently uninvoked
Every projected agent message retains a localId but now suppresses its consumption acknowledgement. The queue only acknowledges user IDs, while hub/src/store/messages.ts:164 inserts every localId-bearing row with invoked_at=NULL. Consequently, ordinary completed Codex output remains in getUninvokedLocalMessages(). Web fork then fails with 'Session has queued messages' (hub/src/sync/syncEngine.ts:1199), and latest-page requests include all these output rows as queued extras, defeating pagination for large sessions.
Suggested fix
Ensure projected agent rows receive a persisted non-null invoked_at, through an already-consumed insertion path or an appropriate post-insert acknowledgement batch. Cover completed-turn forking and bounded latest-page responses with hub-backed regression tests.
| return id ? [id] : []; | ||
| }); | ||
| }); | ||
| await this.committedHistory(ids); |
There was a problem hiding this comment.
[Major] Emit the history acknowledgement after transcript insertion
When importing history whose user rows do not yet exist in the hub, such as cold-resuming a previously standalone native thread, committedHistory() emits the sole acknowledgement before history projection sends those rows. The hub's markMessagesInvoked() only updates existing rows and does not retain acknowledgements for unknown IDs. Subsequent sendUserMessage() calls suppress their acknowledgements, so the imported prompts remain invoked_at=NULL and appear queued on refresh until another reconciliation repairs them.
Suggested fix
Separate durable ledger persistence from acknowledgement emission: persist consumption before projection, then emit one batch after the corresponding transcript packets. Add coverage starting with an empty hub transcript and assert the stored prompts are invoked after one history replay.
| } | ||
|
|
||
| async waitForRegistration(method: string, timeoutMs = 5_000): Promise<void> { | ||
| if (!this.socket?.connected || !this.hasHandler(method)) { |
There was a problem hiding this comment.
[Major] Wait for the initial socket connection before registering
Session bootstrap starts Socket.IO asynchronously and returns without awaiting connection. SharedCodexRoot.bind() now calls this method unconditionally, but it immediately throws whenever the websocket handshake is still pending or a brief disconnect occurs. A slow hub connection can therefore abort an otherwise successful native startup before the advertised five-second timeout is used; initial startup failure shuts down the runtime.
Suggested fix
Make the registration barrier wait for a connected socket within a bounded deadline, then obtain the registration acknowledgement. Add a delayed-connect regression test through the root binding path.
There was a problem hiding this comment.
Review mode: follow-up after new commits
Requirement — Pass
Reducing Codex cold-restore delays and redundant history acknowledgements addresses supported resume and recovery flows.
Evidence
- The snapshot PR description identifies delayed control registration, repeated ownership probes, and excessive replay acknowledgements.
- docs/guide/codex-shared-sessions.md:86 documents history reconciliation after outages; line 97 requires process-generation protection during cold resume.
Approach — Pass
Early control registration, readiness barriers, paged history, batched consumption, and consolidated Windows ownership checks are viable. The remaining defect can be fixed within this approach.
Evidence
- cli/src/codex/shared/root.ts:212 registers controls before history projection and waits for registration.
- cli/src/codex/shared/projection.ts:219 persists user consumption before projection and acknowledges transcript rows afterward.
- cli/src/codex/shared/registry.ts:85 consolidates Windows probes while preserving generation checks.
Code — Reviewed
Reviewed the entire fixed-head merge-base diff. The three previous findings are addressed. One large-history regression remains.
- [Major] Bound history acknowledgements before the SQLite update —
cli/src/codex/shared/projection.ts:252
Testing
Not run (automation; PR code execution is prohibited).
- Observed fixed-head CI: test, integration, windows-codex-mcp, and drift-gate succeeded.
- No PR code, tests, builds, or scripts were executed. Validation reported in the PR description was not independently reproduced.
- The new store-backed replay test covers 80 turns; it does not exercise oversized acknowledgement batches.
- The snapshot explicitly reports unresolved Windows restart and archive lifecycle failures requiring the companion Runner fix.
HAPI Bot
| } | ||
| } | ||
| } | ||
| if (consumed.size) this.session.emitMessagesConsumed([...consumed]); |
There was a problem hiding this comment.
[Major] Bound history acknowledgements before the SQLite update
Cold-importing a history whose projected user and agent IDs exceed SQLite's bind-parameter limit sends the entire list in this single acknowledgement. Store.recordMessagesConsumed forwards it to markMessagesInvoked (hub/src/store/messages.ts:902), which creates one placeholder per ID plus two additional parameters. The query fails, and sessionHandlers catches the error without completing consumption. Newly inserted rows therefore retain invoked_at=NULL: latest-page requests include them all as queued extras, and completed-session forks fail with 'Session has queued messages'. Replaying the same oversized batch cannot repair them.
Suggested fix
Chunk the SQL updates within the existing consumption transaction, or emit bounded post-insert acknowledgement batches. Add a store-backed regression exceeding the SQLite parameter limit and verify invocation state, bounded pagination, and fork availability.
There was a problem hiding this comment.
Review mode: follow-up after new commits
Requirement — Pass
Reducing Codex cold-restore delays and redundant replay acknowledgements supports HAPI’s documented resume and outage-recovery flows.
Evidence
- The snapshot PR description identifies delayed control registration, repeated ownership probes, and excessive history acknowledgements.
- docs/guide/codex-shared-sessions.md documents stable-ID history reconciliation and process-generation protection during cold resume.
Approach — Pass
Early control registration, readiness barriers, consolidated Windows ownership checks, and bounded post-insert acknowledgements address the identified causes while preserving recovery safeguards.
Evidence
- cli/src/codex/shared/root.ts:212 registers controls before initial history projection; configuration RPCs wait for readiness at line 478.
- cli/src/codex/shared/registry.ts:85 uses a fresh Windows snapshot while retaining wrapper and orphan-engine generation checks.
- cli/src/codex/shared/projection.ts:221 persists user consumption before projection; cli/src/api/apiSession.ts:1339 bounds acknowledgements to 500 IDs.
Code — Reviewed
Reviewed the entire fixed-head merge-base diff and surrounding handling. The previous oversized-acknowledgement finding is addressed, and no additional actionable defects were found. Assessment is based on static inspection.
No reportable code issues found.
Testing
Not run (automation; PR code execution is prohibited).
- Fixed-head CI succeeded: test, integration, windows-codex-mcp, and drift-gate.
- Added regression coverage includes 250,001 imported rows, persisted invocation state, bounded pagination, fork availability, and acknowledgement size and ordering.
- No PR code, tests, builds, or scripts were executed; reported local validation was not independently reproduced.
- The snapshot reports unresolved intermittent Windows startup and process-stop failures requiring a companion Runner lifecycle fix; the Windows CI job covers Codex MCP rather than repeated cold restores.
HAPI Bot
This branch has not been deployed
1 participant
Summary
Large archived Codex sessions can time out when control registration queues behind history replay or ownership checks repeatedly probe old process records. History projection also generated redundant consumption acknowledgements, delaying live updates.
This change:
SSE compression remains unchanged; Windows continues using gzip.
Validation
Independently tested from
317e39a0:cli/:bunx vitest run src/api/apiSession.test.ts src/codex/shared/projection.test.ts src/codex/shared/queue.test.ts src/codex/shared/root.test.ts src/api/rpc/RpcHandlerManager.test.ts src/codex/shared/registry.test.ts src/codex/shared/ownershipLiveness.test.ts src/codex/shared/projection.title.test.ts— 138 passed, using task-isolated test authentication, home and port.bun test ./hub/src/store/messages.test.ts ./hub/src/socket/handlers/cli/sessionHandlers.test.ts ./hub/src/socket/handlers/cli/rpcHandlers.test.ts ./hub/src/web/routes/events.replay.test.ts ./hub/src/web/sseCompression.test.ts ./hub/tests/codexProjection.test.ts— 81 passed.bun run typecheck,bun run build, andbun run --cwd cli build:exe:allinone— passed.Invoke-HapiTaskPlaywright.ps1 -Name codex-recovery-pr -Suite Root terminal-wrap-fidelity.spec.ts— 2 passed.too many SQL variablesbefore bounded acknowledgements. With the fix, all rows are invoked, latest-page size remains bounded, and forking succeeds.Invoke-HapiTaskPlaywright.ps1 -Name codex-recovery-pr -Suite Root --config playwright.production-like.config.ts. The final run reopened successfully and passed two browser contexts, refresh, reconnect, lost-response handling and message uniqueness, with gzip verified on the wire.Remaining Windows lifecycle limitation
Intermittent Windows startup-deadline failures and process-stop 500s remain reproducible with this branch alone. A three-cycle restart stress test passed two cold restores and timed out on the third before its startup webhook arrived. Another packaged attempt also timed out during initial reopen.
These failures are retained and are not counted as passing acceptance. Repeated Windows archive/reopen still needs the companion Runner lifecycle fix.
Related Issues
None.
Related PRs: #1886 and #1940. The
excludeTurnsand fresh-thread changes overlap with #1886 and should be reconciled if it lands first.AI Disclosure
Implementation and validation were assisted by OpenAI Codex, using GPT-6.