Migrate fork and resume reads to thread store#18900
Migrate fork and resume reads to thread store#18900wiltzius-openai wants to merge 2 commits intomainfrom
Conversation
a1cf2fd to
3304803
Compare
6ab42ef to
0a97cbd
Compare
3304803 to
4684212
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4684212b00
ℹ️ 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".
1f08dd6 to
a32c66c
Compare
a32c66c to
344d17d
Compare
344d17d to
707e1f3
Compare
58068b9 to
070c747
Compare
707e1f3 to
17882c8
Compare
17882c8 to
a33177f
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a33177f384
ℹ️ 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".
| .thread_store | ||
| .read_thread(StoreReadThreadParams { | ||
| thread_id, | ||
| include_archived: true, | ||
| include_history: true, | ||
| }) |
There was a problem hiding this comment.
Read fork result from the fork-configured store
thread_fork builds a per-request config (with user overrides) and creates the new thread using thread_manager, which selects a store from that config. The follow-up read in read_stored_thread_for_new_fork ignores that and always queries self.thread_store (startup store). If overrides change experimental_thread_store_endpoint, the fork is written to one backend but read from another, causing spurious "not found" failures.
Useful? React with 👍 / 👎.
| .read_thread(StoreReadThreadParams { | ||
| thread_id, | ||
| include_archived: true, | ||
| include_history: true, | ||
| }) |
There was a problem hiding this comment.
Avoid loading full history for excludeTurns forks
read_stored_thread_for_new_fork always requests include_history: true. In thread_fork, this runs even when excludeTurns=true (the intended cheap path), so persistent forks still deserialize the entire rollout history just to build a summary. That adds avoidable latency/memory cost and can make large forks slow or fail under load.
Useful? React with 👍 / 👎.
a33177f to
22e9b89
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22e9b89dde
ℹ️ 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".
| self.thread_store | ||
| .read_thread(StoreReadThreadParams { | ||
| thread_id: existing_thread_id, | ||
| include_archived: true, | ||
| include_history: true, | ||
| }) |
There was a problem hiding this comment.
Keep fallback path lookup when thread-store read is unavailable
When path is omitted, this now unconditionally uses self.thread_store.read_thread(...). In remote-store mode, RemoteThreadStore::read_thread is currently unimplemented, so thread/fork and thread/resume by thread ID fail with INTERNAL errors instead of working from local rollout files. This is a regression from the prior direct rollout-path lookup behavior.
Useful? React with 👍 / 👎.
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
21610d1 to
71885e6
Compare
71885e6 to
86dfb4b
Compare
86dfb4b to
b4e8ff8
Compare
b4e8ff8 to
aa5066a
Compare
aa5066a to
4a63d3d
Compare
4a63d3d to
35f560c
Compare
35f560c to
20ce4b4
Compare
20ce4b4 to
e801d87
Compare
e801d87 to
51786a8
Compare
| if !tokio::fs::try_exists(path).await.unwrap_or(false) { | ||
| return false; | ||
| } | ||
| read_thread_from_rollout_path(store, path.to_path_buf()) |
There was a problem hiding this comment.
surprised we need to double check ids. not sure how we ever get the missalignment
51786a8 to
079c897
Compare
079c897 to
9298207
Compare
Uh oh!
There was an error while loading. Please reload this page.