Move plugin and skill warmup into session startup#23535
Conversation
aibrahim-oai
changed the title
|
@codex review this |
![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: 1960204afa
ℹ️ 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_persistence_fut, | ||
| state_db_fut, | ||
| auth_and_mcp_fut, | ||
| plugin_and_skill_warmup_fut |
There was a problem hiding this comment.
When session startup is canceled while plugin/skill warmup is slow or blocked, adding plugin_and_skill_warmup_fut to this tokio::join! can leave a completed thread_persistence_fut result holding a LiveThread before LiveThreadInitGuard is constructed below. Dropping that unguarded LiveThread skips discard_thread, so the local store can retain a live recorder for a thread that never finished initializing; before this change, this warmup ran before opening thread persistence and did not create that cancellation window.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d947f23d8d
ℹ️ 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".
| use codex_protocol::protocol::ThreadMemoryMode; | ||
|
|
||
| use super::*; | ||
| use crate::BaseInstructions; |
There was a problem hiding this comment.
Running cargo test -p codex-thread-store init_guard_from_completed_join_branch_discards_on_task_cancellation fails to compile this new test because codex-thread-store does not expose crate::BaseInstructions; the codex_protocol::SessionSource reference below has the same module-path issue. Import BaseInstructions from codex_protocol::models and SessionSource from codex_protocol::protocol (then use SessionSource::Exec) so the crate's test target can build.
Useful? React with 👍 / 👎.
2 participants
Why
Plugin and skill loading is useful as warmup and early validation, but session startup does not need to wait for that work before it can continue building the session. Keeping it on the serial startup path adds avoidable latency to every fresh thread start.
We still want invalid skill configurations to show up quickly, and we want the warmup to exercise the same plugin and skill manager caches that the normal turn path uses.
What changed
Testing