[codex] optimize resume and fork history#28806
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: 9cd3165b07
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48fccc5ff9
ℹ️ 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".
| .read_thread(StoreReadThreadParams { | ||
| thread_id, | ||
| include_archived: true, | ||
| include_history: false, |
There was a problem hiding this comment.
When the local SQLite row has a stale or mismatched rollout_path, LocalThreadStore only verifies the file's SessionMeta when include_history is true; with this include_history: false probe it can return the stale path from metadata. The new fast path then reads that path directly and returns a notLoaded page before the existing full-history loader can fall back to lookup-by-id, so the first thread/turns/list page can be built from another thread's rollout. Verify the session id for the path, or use the full-history read before serving the suffix page.
AGENTS.md reference: AGENTS.md:L103-L110
Useful? React with 👍 / 👎.
| if is_checkpoint { | ||
| found_checkpoint = true; | ||
| return Ok(false); |
There was a problem hiding this comment.
When the latest replacement-history checkpoint has the surviving turn's TurnContext before the Compacted item, stopping the reverse scan as soon as the checkpoint is seen drops that metadata from the optimized resume/fork history. apply_rollout_reconstruction then cannot hydrate previous_turn_settings or the reference context from the newest surviving turn, so an excludeTurns resume/fork after compaction can skip model-change handling/pre-turn compaction and build a different initial context than a full replay. Continue scanning until the checkpoint's turn metadata is included, or fall back to the full loader.
AGENTS.md reference: AGENTS.md:L92-L101
Useful? React with 👍 / 👎.
| tokio::task::yield_now().await; | ||
| // Auto-attach after the response is queued. The attach is still | ||
| // performed before the follow-up notification, but it no longer sits on | ||
| // the fork response latency path. | ||
| log_listener_attach_result( | ||
| self.ensure_conversation_listener( |
There was a problem hiding this comment.
When a client starts a turn on the newly forked thread immediately after receiving thread/fork, turn/start is serialized on the child thread id while the fork request is serialized on the source thread id, so it can run before this post-response listener attach completes. Any early turn lifecycle notifications emitted in that window are not delivered to the connection; the previous flow attached the listener before sending the fork response. Keep the child listener attached before the response is observable, or otherwise serialize child requests behind fork finalization.
AGENTS.md reference: AGENTS.md:L103-L110
Useful? React with 👍 / 👎.
| let optimized_source = if !include_turns && !ephemeral { | ||
| self.read_stored_thread_for_resume_with_compacted_history(&thread_id, path.as_ref()) | ||
| .await? |
There was a problem hiding this comment.
When an excludeTurns fork hits this path, it enables initial_rollout_copy for whatever ThreadStore returned a rollout path. Session startup later persists only the suffix after the source history; that is safe for LocalThreadStore because its recorder writes the parent rollout ref, but other implementations in this repo (for example InMemoryThreadStore::create_thread) ignore CreateThreadParams::initial_rollout_copy, so a fork from such a store that carries a rollout path is persisted with only the tiny suffix and loses the source conversation on later read/resume. Restrict this fast path to stores that materialize the parent ref, or fall back to full history for other stores.
AGENTS.md reference: AGENTS.md:L103-L110
Useful? React with 👍 / 👎.
1 participant
thread/resumeandthread/forkhistory work while preserving fallback behavior for legacy or malformed rollouts.codexbinaries, ran 20 paired CLI-equivalent app-server timings on a copied real Codex home, and reproduced the grader-shaped checkpoint win on the synthetic fixture. Focused validation ranjust fmt, scopedjust test -p codex-app-server, and the requiredjust fixpass.