fix: job token budget, iteration cap → Failed, web cancel stops worker - #788
Conversation
commented
Mar 9, 2026
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the stability and resource management of agent jobs. It introduces a token budget system to control LLM costs, refines job failure states to prevent undesirable retry loops, and improves the reliability of job cancellation from the web interface by ensuring worker tasks are properly terminated. These changes collectively contribute to more predictable and efficient agent operations. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
left a comment
There was a problem hiding this comment.
Pull request overview
This PR addresses runaway agent jobs by introducing per-job token budgets, changing iteration-cap/rate-limit terminal behavior to Failed, and making web “cancel” actually stop the running worker via the scheduler.
Changes:
- Add configurable
agent.max_tokens_per_job(settings + env) and plumb it into job creation with optional per-job metadata override. - Change iteration-cap and persistent rate-limit terminal state from
StucktoFailedin the worker loop. - Update web cancel handler to stop the worker via
scheduler.stop(job_id)(with DB-only fallback).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/settings.rs |
Adds AgentSettings.max_tokens_per_job with default 0 (unlimited). |
src/config/agent.rs |
Resolves max_tokens_per_job from env/settings into AgentConfig. |
src/agent/scheduler.rs |
Applies token budget to new jobs, allowing metadata["max_tokens"] override. |
src/agent/worker.rs |
Marks iteration/rate-limit terminal conditions as Failed; adds token usage tracking; adds tests. |
src/channels/web/handlers/jobs.rs |
Uses scheduler stop on cancel to abort worker; falls back to DB update. |
src/agent/dispatcher.rs |
Updates tests to include new AgentConfig field. |
.env.example |
Documents AGENT_MAX_TOKENS_PER_JOB. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Track token usage from LLM call against the job budget. | ||
| let total_tokens = respond_output.usage.total() as u64; | ||
| if total_tokens > 0 { | ||
| let budget_result = self | ||
| .context_manager() | ||
| .update_context(self.job_id, |ctx| ctx.add_tokens(total_tokens)) | ||
| .await?; | ||
| if let Err(msg) = budget_result { | ||
| self.mark_failed(&msg).await?; | ||
| return Ok(()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Token budget enforcement is only applied to the respond_with_tools() call when selections.is_empty(). Iterations that go through select_tools() (and any other LLM calls like planning) are not counted, so a job can still exceed the intended budget without being failed. Consider plumbing TokenUsage out of select_tools() / plan() (or adding a wrapper that records usage for every LLM call in the worker loop) so all LLM token consumption contributes to ctx.add_tokens(...).
There was a problem hiding this comment.
Valid point. select_tools() makes an LLM call but does not expose TokenUsage in its return type. Plumbing usage out requires changing the Reasoning trait — a broader refactor. Added a NOTE comment acknowledging the gap in 7364930. Since there was zero token tracking before this PR, tracking respond_with_tools() alone is a meaningful improvement, and the iteration cap now also serves as a hard stop.
| #[tokio::test] | ||
| async fn test_token_budget_exceeded_fails_job() { | ||
| let worker = make_worker(vec![]).await; | ||
|
|
||
| // Transition to InProgress (required for mark_failed) | ||
| worker | ||
| .context_manager() | ||
| .update_context(worker.job_id, |ctx| { | ||
| ctx.transition_to(JobState::InProgress, None) | ||
| }) | ||
| .await | ||
| .unwrap() | ||
| .unwrap(); | ||
|
|
||
| // Set a token budget | ||
| worker | ||
| .context_manager() | ||
| .update_context(worker.job_id, |ctx| { | ||
| ctx.max_tokens = 100; | ||
| }) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| // Simulate adding tokens that exceed the budget | ||
| let budget_result = worker | ||
| .context_manager() | ||
| .update_context(worker.job_id, |ctx| ctx.add_tokens(200)) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| assert!( | ||
| budget_result.is_err(), | ||
| "Should return error when token budget exceeded" | ||
| ); | ||
|
|
||
| // Verify that mark_failed transitions job to Failed | ||
| worker | ||
| .mark_failed(&budget_result.unwrap_err()) | ||
| .await | ||
| .unwrap(); | ||
| let ctx = worker | ||
| .context_manager() | ||
| .get_context(worker.job_id) | ||
| .await | ||
| .unwrap(); | ||
| assert_eq!(ctx.state, JobState::Failed); | ||
| } |
There was a problem hiding this comment.
These new tests validate JobContext::add_tokens() and Worker::mark_failed(), but they don't exercise the newly added runtime path that increments tokens based on respond_with_tools().usage inside Worker::run(). Adding a test that runs a minimal worker loop iteration with a stub LLM returning a non-zero usage would better cover the regression this PR is addressing (budget enforcement from actual LLM calls).
There was a problem hiding this comment.
Acknowledged. Testing the full worker loop with a mock LLM that returns non-zero usage is substantial (requires mocking Reasoning, select_tools, respond_with_tools). The current tests cover the core path: add_tokens correctly returns an error on budget exceeded, and mark_failed correctly transitions the job. The integration between these pieces in the loop is straightforward (3 lines of glue code). A full integration test with a mock LLM would be valuable but is better suited as a follow-up.
| // Try to stop via scheduler first (stops the actual worker). | ||
| let stopped_via_scheduler = if let Some(ref slot) = state.scheduler { | ||
| let guard = slot.read().await; | ||
| if let Some(ref scheduler) = *guard { | ||
| scheduler.stop(job_id).await.is_ok() | ||
| } else { | ||
| false | ||
| } | ||
| } else { | ||
| false | ||
| }; | ||
|
|
||
| // If scheduler wasn't available, fall back to DB-only update. | ||
| if !stopped_via_scheduler { | ||
| store | ||
| .update_job_status( | ||
| job_id, | ||
| crate::context::JobState::Cancelled, | ||
| Some("Cancelled by user"), | ||
| ) | ||
| .await | ||
| .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()))?; |
There was a problem hiding this comment.
stopped_via_scheduler is computed with scheduler.stop(job_id).await.is_ok(), but Scheduler::stop() currently returns Ok(()) even when the job isn't in the in-memory scheduler map. That means this handler can skip the DB status update while the job remains active in the DB (and no worker was actually stopped). Fix by checking scheduler.is_running(job_id).await (or changing stop() to return NotFound) and only skipping the DB update when a running job was actually stopped (and/or always persist the Cancelled state).
There was a problem hiding this comment.
Good catch. Fixed in 7364930 — the cancel handler now always persists Cancelled to the DB, using scheduler.stop() as an additional best-effort step (to abort the worker task) rather than a replacement for the DB update. This handles the edge case where stop() returns Ok(()) for jobs not in the scheduler map.
left a comment
There was a problem hiding this comment.
Code Review
This pull request introduces several important fixes to job lifecycle management, addressing potential infinite loops and token over-consumption. The changes to enforce a token budget, fail jobs on iteration caps, and ensure web cancellations properly stop worker tasks are well-implemented and crucial for stability. The new tests adequately cover the new failure conditions. I've kept the original suggestions to improve the conciseness and readability of the new logic, as they do not contradict any established rules.
Note: Security Review did not run due to the size of the PR.
| let budget_result = self | ||
| .context_manager() | ||
| .update_context(self.job_id, |ctx| ctx.add_tokens(total_tokens)) | ||
| .await?; | ||
| if let Err(msg) = budget_result { | ||
| self.mark_failed(&msg).await?; | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
This logic for checking the token budget is correct, but it can be made more concise by combining the await? and the if let into a single statement. This improves readability by making the error handling path more direct.
| let budget_result = self | |
| .context_manager() | |
| .update_context(self.job_id, |ctx| ctx.add_tokens(total_tokens)) | |
| .await?; | |
| if let Err(msg) = budget_result { | |
| self.mark_failed(&msg).await?; | |
| return Ok(()); | |
| } | |
| if let Err(msg) = self | |
| .context_manager() | |
| .update_context(self.job_id, |ctx| ctx.add_tokens(total_tokens)) | |
| .await? | |
| { | |
| self.mark_failed(&msg).await?; | |
| return Ok(()); | |
| } |
There was a problem hiding this comment.
Applied — collapsed the nested if into a let-chain per clippy. Fixed in 7364930.
| let stopped_via_scheduler = if let Some(ref slot) = state.scheduler { | ||
| let guard = slot.read().await; | ||
| if let Some(ref scheduler) = *guard { | ||
| scheduler.stop(job_id).await.is_ok() | ||
| } else { | ||
| false | ||
| } | ||
| } else { | ||
| false | ||
| }; |
There was a problem hiding this comment.
This logic to determine if the job was stopped via the scheduler is a bit nested. You can improve readability and reduce nesting by using an async block to scope the awaits and return the result directly.
let stopped_via_scheduler = async {
if let Some(scheduler_slot) = &state.scheduler {
if let Some(scheduler) = scheduler_slot.read().await.as_ref() {
return scheduler.stop(job_id).await.is_ok();
}
}
false
}.await;There was a problem hiding this comment.
Applied a similar simplification using let-chains (collapsed nested ifs). Fixed in 7364930.
left a comment
There was a problem hiding this comment.
Review
Three changes bundled here: token budget enforcement, iteration cap -> Failed (not Stuck), and web cancel stops worker via scheduler. All three are solid improvements.
Token Budget
ctx.add_tokens()/ctx.max_tokenspattern is clean. The metadata override (max_tokensin job metadata) is a nice touch for per-job control.- Concern:
add_tokensreturnsResultviaupdate_context, which returnsResult<T, _>where T is the closure's return value. The double-unwrap pattern (await.unwrap()then checkis_err()) in the test is fine, but in the worker loop the pattern is:This is correct but would be clearer iflet budget_result = self.context_manager().update_context(...).await?; if let Err(msg) = budget_result { ... }
add_tokensreturned a named error type instead ofResult<(), String>.
Iteration Cap -> Failed
- Good fix.
mark_stucktriggers self-repair which retries, creating an infinite loop.mark_failedis the correct terminal state for "tried too many times."
Web Cancel via Scheduler
- The
stopped_via_schedulerpattern correctly tries the in-memory scheduler first (which aborts the task handle), falling back to DB-only update. - Edge case: If
scheduler.stop()succeeds (returns Ok) but the worker hasn't persisted its final state yet, the response says "cancelled" but the DB might still show InProgress briefly. Acceptable for now but worth a comment.
Tests
Both new tests (test_token_budget_exceeded_fails_job, test_iteration_cap_marks_failed_not_stuck) are good regression tests.
Overall looks good -- approve once the existing CI passes.
commented
Mar 10, 2026
|
Thanks for the review @zmanian! Re: Re: DB consistency edge case on cancel — fixed in 7364930. The handler now always persists |
7364930 to
f7cfbeb
Compare
left a comment
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
.github/workflows/staging-ci.yml:122
- The
actions/create-github-app-token@v2step now runs unconditionally. IfGH_RELEASES_MANAGER_APP_ID/GH_RELEASES_MANAGER_APP_PRIVATE_KEYsecrets are missing or empty, this action fails the job before the later fallback togithub.tokencan run. Restore the previousif:guard (or make the stepcontinue-on-errorand handle the empty-token case) so staging CI still works when the GitHub App secrets aren't configured.
- name: Generate GitHub App token
id: app-token
uses: actions/create-github-app-token@v2
with:
app-id: ${{ secrets.GH_RELEASES_MANAGER_APP_ID }}
private-key: ${{ secrets.GH_RELEASES_MANAGER_APP_PRIVATE_KEY }}
.github/workflows/staging-ci.yml:236
- Same issue here: generating the GitHub App token is unconditional, so missing/empty
GH_RELEASES_MANAGER_*secrets will fail the gate job before the workflow can fall back togithub.token. Add back anif:guard or make the step non-fatal and keep the fallback logic effective.
- name: Generate GitHub App token
id: app-token
uses: actions/create-github-app-token@v2
with:
app-id: ${{ secrets.GH_RELEASES_MANAGER_APP_ID }}
private-key: ${{ secrets.GH_RELEASES_MANAGER_APP_PRIVATE_KEY }}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…709) * feat: persist worker events to DB and fix activity tab rendering In-process Worker (used by Scheduler::dispatch_job) now persists events via save_job_event at key execution points: plan creation, LLM responses, tool_use, tool_result, and job completion/failure/stuck. Event data shapes match the container worker format so the gateway activity tab renders them correctly. Frontend: tool_result errors now show a red X icon with danger styling instead of a silent empty output. The result event falls back to the error field when message is absent. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat: wire RoutineEngine into gateway for direct manual trigger firing Replace the message-channel hack in routines_trigger_handler with a direct call to RoutineEngine::fire_manual(), ensuring FullJob routines dispatch correctly when triggered from the web UI. Inject the engine into GatewayState from Agent::run after construction. Also persists user_id in save_job for both PG and libSQL backends, removes the source='sandbox' filter so all jobs are visible, and exposes job_id on RoutineRunInfo for the frontend job link. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: remove stale gateway_state argument from Agent::new test call sites The gateway_state parameter was removed from Agent::new during rebase (replaced by post-construction set_routine_engine_slot), but three test call sites still passed the extra None argument. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address PR review — restore sandbox source filter, remove blank lines - Revert removal of `source = 'sandbox'` filter in all SandboxStore queries (8 sites across PG and libSQL). Sandbox-specific APIs should stay scoped to sandbox jobs; unified job listing for the Jobs tab should use a separate query path. - Remove extra blank lines in agent_loop.rs and worker.rs that caused formatting CI failure. [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address review — regenerate Cargo.lock, add user_id regression test - Regenerate Cargo.lock from main's lockfile to eliminate dependency version downgrades (anyhow, syn, etc.) that were churn from rebase. - Add regression test verifying user_id round-trips through save_job and get_job in the libSQL backend. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * style: remove trailing blank line in libsql jobs.rs [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * test: add Postgres-side regression test for user_id persistence in save_job Mirrors the existing libSQL test (test_save_job_persists_user_id) for the Postgres backend. Gated behind #[cfg(feature = "postgres")] + #[ignore] since it requires a running PostgreSQL instance (integration tier). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…ncel (#698) Jobs could enter infinite retry loops because: (1) no token budget was enforced, (2) iteration cap marked jobs as Stuck (allowing self-repair to restart them), and (3) the web UI cancel button only updated the DB without stopping the running worker. - Add `max_tokens_per_job` config (settings.json + AGENT_MAX_TOKENS_PER_JOB env var, default 0 = unlimited) with per-job metadata override - Track token usage after respond_with_tools() and fail the job on budget exceeded - Change iteration cap and persistent rate limiting from mark_stuck to mark_failed, preventing self-repair restart loops - Fix web cancel handler to call scheduler.stop() which updates in-memory state AND aborts the worker task, falling back to DB-only update Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…check - Cancel handler now always persists Cancelled to DB regardless of whether scheduler.stop() ran, fixing the edge case where stop() returns Ok(()) for jobs not in the scheduler map - Collapse nested ifs per clippy (let-chains) - Add NOTE comment about select_tools() not exposing TokenUsage [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
[skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
f7cfbeb to
ab861a5
Compare
Summary
Fixes #698. Jobs could enter infinite retry loops burning millions of tokens because:
JobContext::add_tokens()existed but was never called. Added configurablemax_tokens_per_job(settings.jsonagent.max_tokens_per_job/AGENT_MAX_TOKENS_PER_JOBenv var, default 0 = unlimited). Token usage is tracked afterrespond_with_tools()and the job fails on budget exceeded. Per-job override viametadata["max_tokens"].mark_stuck()tomark_failed()for iteration cap and persistent rate limiting, soDefaultSelfRepaircannot restart them.scheduler.stop(job_id)which updates in-memoryContextManagerAND aborts the worker task. Falls back to DB-only update when scheduler is unavailable.Test plan
test_token_budget_exceeded_fails_job— token budget enforcement transitions to Failedtest_iteration_cap_marks_failed_not_stuck— iteration cap transitions to Failed, not Stuckcargo clippy --all --all-features— zero warningscargo test— 2758 passed, 0 failedagent.max_tokens_per_jobin settings.json, run a job, verify it stops at the budget🤖 Generated with Claude Code