fix: retry rejected previous-model compaction with selected model - #30319
Conversation
aff99a5 to
ccffcff
Compare
60090a7 to
e8b557b
Compare
celia-oai
changed the title
4809aa8 to
594fed1
Compare
e8b557b to
6d33a24
Compare
594fed1 to
cf62e27
Compare
e7ee322 to
0db34c3
Compare
ec16a25 to
1f3200a
Compare
0db34c3 to
262b29f
Compare
celia-oai
changed the title
celia-oai
changed the title
![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: a38022e872
ℹ️ 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".
| turn_state, | ||
| &compaction_trace, | ||
| compaction_metadata, | ||
| analytics_details, |
There was a problem hiding this comment.
When the previous-model attempt fails after local prompt trimming, run_remote_compact_attempt has already mutated analytics_details.active_context_tokens_before based on a cloned history that never becomes the successful compaction input. Passing that same mutable details object into the selected-model retry means fallback scenarios with oversized function-call outputs can subtract the failed attempt's estimated deletions from the single tracked compaction event, and then subtract again (or keep a subtraction the fallback did not need), underreporting the pre-compaction token count; snapshot the details before the first attempt or only apply trimming deltas from the attempt that succeeds.
Useful? React with 👍 / 👎.
| fallback_result.as_ref().err(), | ||
| ); | ||
| match fallback_result { | ||
| Ok(attempt) => (attempt, &fallback_step_context.turn), |
There was a problem hiding this comment.
When this fallback branch succeeds, the inner implementation uses fallback_step_context.turn for history replacement and item completion, but the outer run_remote_compact_task_inner still invokes run_post_compact_hooks with the original previous-model turn_context. In ChatGPT/OpenAI fallback recoveries with configured PostCompact hooks, the hook payload's model remains the retired/previous model rather than the selected model that actually compacted the thread, so hooks that audit or gate by model run on stale data; propagate the successful compaction context back to the hook layer or run the hook with compaction_turn_context.
Useful? React with 👍 / 👎.
sayan-oai
left a comment
sayan-oai
left a comment
There was a problem hiding this comment.
makes sense to me, might still be worth getting @aibrahim-oai's thoughts
## Why This PR is a behavior-preserving refactor only. It does not add a fallback, change which model is used for compaction, or otherwise change compaction behavior. The behavioral change is implemented in the stacked follow-up, #30319. Pre-sampling compaction deliberately uses the previous turn's context when the compaction compatibility hash changes or when switching to a model with a smaller context window. That preserves the model settings that produced the history being compacted, but the previous context is not always usable. For example, a resumed thread can still reference a model slug that has since been retired, causing compaction to fail before the currently selected model can sample. #30319 addresses that failure mode by retrying compaction with the current turn's selected model when the backend rejects the previous-model attempt. This PR performs only that preparatory refactor. ## What changed - Extracted one legacy `/responses/compact` request attempt into `compact_remote_request.rs`. - Extracted one Responses-based remote compaction request attempt into `compact_remote_v2_attempt.rs`. - Kept hooks, lifecycle events, analytics, window advancement, history processing and installation, and error behavior unchanged in the existing orchestration paths. - Preserved standalone Responses-based compaction's owned client-session lifetime through lifecycle completion. ## Testing - `just test -p codex-core -E 'test(remote_compact)'` (22 tests)
a38022e to
5e2acb5
Compare
2 participants
Why
Pre-sampling compaction intentionally uses the previous turn's model when the compaction compatibility hash changes or when switching to a model with a smaller context window. This keeps compaction aligned with the settings that produced the history, but it can block the next turn when a resumed ChatGPT thread still references a model slug that has since been retired. The Codex backend rejects that compaction request before the user's currently selected model gets a chance to sample.
This PR lets those threads recover without changing previous-model compaction behavior for API-key authentication or custom providers. It is stacked on #31316, which is a behavior-preserving extraction of the individual remote compaction attempts; this PR contains the fallback behavior.
What changed
InvalidRequest, retry compaction once with the selected model for both/responses/compactand Responses Compaction V2.InvalidRequestfailures on their existing paths.Testing
just test -p codex-core -E 'test(pre_sampling_compact) | test(model_unavailable_error)'(10 tests)