[codex] abort turns when rollout budgets expire (token budget 3/3)#28707
Conversation
b984e14 to
0cf2c87
Compare
5877cdf to
33db2e4
Compare
0cf2c87 to
845074e
Compare
445c194 to
77ae42c
Compare
4473161 to
cbb2072
Compare
77ae42c to
45a551b
Compare
cbb2072 to
7df6e0e
Compare
45a551b to
5fd96c8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5fd96c8985
ℹ️ 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".
5fd96c8 to
bf0d519
Compare
bf0d519 to
adfc267
Compare
74d1a92 to
0c95ffd
Compare
350cc23 to
aaca015
Compare
0e01b88 to
9d97673
Compare
aaca015 to
529597a
Compare
9d97673 to
1807f7f
Compare
529597a to
4db5c2e
Compare
1807f7f to
4333d42
Compare
4db5c2e to
eb51c76
Compare
4333d42 to
bc62a74
Compare
eb51c76 to
4f520fc
Compare
4f520fc to
93aba6e
Compare
| } = compaction_output_result?; | ||
| if let Some(token_usage) = token_usage { | ||
| sess.record_rollout_budget_usage(&token_usage); | ||
| sess.record_rollout_budget_usage(&token_usage)?; |
There was a problem hiding this comment.
will this get turned into Error running remote compact task ?
There was a problem hiding this comment.
yes, I thought it was okay bc the point is to just kill the rollout right?
There was a problem hiding this comment.
hmm actually, i can see how this is bad - fixing now
| true | ||
| } | ||
|
|
||
| async fn on_task_aborted(self: &Arc<Self>) { |
There was a problem hiding this comment.
is there more to share with normal abort path?
| } | ||
| Err(err) => { | ||
| warn!(%err, "session task returned an unexpected error"); | ||
| sess.on_task_finished( |
There was a problem hiding this comment.
Not confident: Should we try to handle error case through onTaskFinished too?
There was a problem hiding this comment.
handling everything through on_task_finished now
pakrym-oai
left a comment
There was a problem hiding this comment.
Can we get rid of net new code for task aborted? There is so much code in task lifecycle already
| ]) | ||
| }; | ||
| let responses = mount_sse_sequence(&server, vec![compact_response]).await; | ||
| let mut model_provider = built_in_model_providers(/*openai_base_url*/ None)["openai"].clone(); |
There was a problem hiding this comment.
this is some very special test setup code
| attempt | ||
| .track(sess.as_ref(), status, codex_error, analytics_details) | ||
| .await; | ||
| if matches!(&result, Err(CodexErr::TurnAborted)) { |
There was a problem hiding this comment.
nit: probably better as a switch.
Stack
Depends on #28494.
Description
This PR propagates shared rollout-budget exhaustion through the existing
CodexErr::TurnAbortedtask result.Each thread records its model usage against the same ledger. Once the ledger is exhausted, that usage update and all later usage updates return
TurnAborted. The task wrapper emits the normal aborted-turn event and lifecycle instead of completing the turn.This is intentionally a soft boundary: there is no cross-thread
Op::Interruptfanout. An in-flight thread can finish its current response before it observes the exhausted ledger, but every thread aborts at its next usage-accounting boundary.Tests
The integration coverage verifies that:
Local checks:
just test -p codex-core exhausted_budget_aborts_current_and_later_turnsjust test -p codex-core subagent_usage_draws_from_the_shared_budgetjust test -p codex-core abort_regular_task_emits_marker_before_turn_abortedjust test -p codex-core compaction_budget_exhaustion_aborts_without_error_or_retryjust fix -p codex-corejust fmtgit diff --checkThe full workspace test suite was not run locally.