fix(agents): roll subagent token usage and cost into parent session - #10103
filipkujawa wants to merge 2 commits into
Conversation
Subagents spawned via the delegate tool run in their own sessions, so their token usage and cost never reached the parent session - under-reporting cost-per-outcome and per-session budgets whenever delegation occurred. Fold a finished subagent's lifetime usage/cost into the parent session's accumulated_* totals at the run_subagent_task chokepoint (covers sync and async delegate), without touching the parent's usage (context window) columns so compaction triggers are unaffected. Accumulated totals are now incremented via a single atomic SQL statement (SessionManager::add_accumulated_usage), and update_session_metrics uses the same path, so a background subagent rolling up concurrently with the parent's reply loop cannot clobber it.
|
Ran the same delegating task on pre-fix vs post-fix binaries, then inspected sessions.db: Pre-fix the subagent's ~5.9K tokens are missing from the parent (bug reproduced). Post-fix 24,774 − 5,947 = 18,827 ≈ pre-fix parent-own (18,782) → subagent spend now rolls up. Cost rolls up too (parent $0.0208, both sessions non-zero). Parent context window stayed ~9.4K in both, so compaction is unaffected. |
| Ok(()) | ||
| } | ||
|
|
||
| async fn add_accumulated_usage( |
There was a problem hiding this comment.
is this for migration from old?
There was a problem hiding this comment.
No, not a migration. It adds a delta to a session's accumulated_usage/accumulated_cost., called on every usage update (and again when a subagent's totals roll up into its parent). The COALESCE bits aren't backfilling old rows- the columns are nullable (a fresh session starts with NULL accumulated tokens), so without them NULL + delta would evaluate to NULL and wipe the total on first increment
|
nice @filipkujawa - would it be possible to make it a little more atomic so when it updates it is one SQL call when it can. Fairly minor thing but wondered if worth a try. Something like the following: |
Address review: replace the two-statement parent metrics update (set usage + add_accumulated_usage) with one SessionManager::update_usage_metrics that sets the context-window columns and increments the accumulated totals/cost in a single atomic UPDATE. add_accumulated_usage remains for the subagent roll-up, which only touches accumulated columns.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4bd435dec
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let result = get_agent_messages(params).await; | ||
|
|
||
| roll_up_usage_to_parent(&session_manager, &parent_session_id, &subagent_session_id).await; |
There was a problem hiding this comment.
Make subagent usage roll-up abort-safe
Because the roll-up is sequenced after awaiting the entire subagent run, an async delegate that is cancelled and then force-aborted by handle_load_task_result after its 5-second grace period drops this future while it is still inside get_agent_messages(params).await, so execution never reaches the roll-up call. Any usage already persisted in the subagent session before that abort remains absent from the parent, preserving under-reporting for stuck/cancelled background delegates; move the roll-up to an abort-safe cleanup path or have the abort path add the persisted subagent totals.
Useful? React with 👍 / 👎.
|
superseded-by #10172 |
Subagents spawned via delegate run in their own session, so their token usage and cost never reached the parent - under-reporting cost-per-outcome and per-session budgets whenever delegation occurred. Folds a finished subagent's lifetime usage/cost into the parent's accumulated_* totals at the run_subagent_task chokepoint (covers sync and async delegate), without touching the parent's usage (context window) columns so compaction triggers are unaffected. Both the reply loop's own usage update and a concurrent subagent roll-up now go through single atomic UPDATE statements (SessionManager::update_usage_metrics / add_accumulated_usage), so neither can clobber the other - verified via a 50-concurrent-writer regression test. Adapted from a closed upstream PR (aaif-goose/goose#10103, closed as superseded by a much larger in-progress usage-ledger redesign that hasn't landed) - the underlying fix is sound and independently useful regardless of whether that redesign ever ships.
Subagents run in their own session, so their tokens and cost never reached the parent. Every delegating turn under-reported cost-per-outcome and slipped past per-session budgets.
This folds a finished subagent's usage/cost into the parent's
accumulated_*totals at therun_subagent_taskchokepoint (sync + asyncdelegate). The parent's context window is left untouched, so compaction is unaffected. Accumulated totals now use an atomic SQL increment, so a background subagent rolling up concurrently with the parent's reply loop can't clobber it.Existing cost/usage readers (ACP
usage_update, CLI display) pick this up for free.assisted by claude code