Skip to content

Add bounded agent-step recovery retries - #1123

Open
viniciusdebruin wants to merge 2 commits into
CodebuffAI:mainfrom
viniciusdebruin:contrib/engine-recovery
Open

Add bounded agent-step recovery retries#1123
viniciusdebruin wants to merge 2 commits into
CodebuffAI:mainfrom
viniciusdebruin:contrib/engine-recovery

Conversation

@viniciusdebruin

Copy link
Copy Markdown

Summary

  • add a bounded outer recovery retry for transient agent-step failures
  • cover network errors, idle timeouts, rate limits, and server 5xx responses
  • preserve the same run/history while emitting recovery metadata and a recovery message
  • skip retries for aborts, authentication failures, and client 4xx errors

Validation

  • 156 focused runtime tests passing
  • typecheck reaches unrelated pre-existing missing researcher imports in read-docs-tool.test.ts and web-search-tool.test.ts

@codebuff-team

Copy link
Copy Markdown
Contributor

Good targeted addition. classifyAgentRecovery in agent-recovery.ts cleanly separates retryable transient failures (network, idle-timeout, 429/5xx) from aborts/auth/4xx, and the unit tests in agent-recovery.test.ts cover the boundary cases well (401/403 vs 400, explicit isRetryable signal, abort). The integration test in loop-agent-steps.test.ts demonstrates the retry actually recovers mid-loop and tags the injected message with AGENT_RECOVERY, which is the right level of test for this kind of change.

A few things worth double-checking before this lands in the private tree:

  1. Cost/credit accounting. creditsBefore/childrenBefore are captured once before the retry loop in run-agent-step.ts, but if the failed first attempt already streamed partial tokens/tool calls before throwing, it's unclear whether that usage is reflected in currentAgentState by the time the retry succeeds. Worth confirming the diff computed after the loop doesn't silently drop or double-count the failed attempt's partial cost.
  2. MAX_AGENT_STEP_RECOVERY_ATTEMPTS = 1 vs. the exponential backoff design. getAgentRecoveryDelayMs supports many attempts (tested up to 20), but with the cap at 1 it will only ever be called with attempt=1 in production, so the backoff curve is currently dead code. Either bump the cap slightly or simplify the delay function to match actual usage — as written it reads like it's designed for more retries than the code allows.
  3. Worth confirming that partial/successful side effects on currentAgentState from a failed runAgentStep call (if any exist internally) are not carried into the retry, since the loop reuses the same currentAgentState reference across attempts.

None of these are blocking correctness issues from what's visible in the diff, just things the maintainer porting this by hand should verify against the private tree's actual runAgentStep internals, which aren't shown here.

@codebuff-team codebuff-team added bot:triaged Classified by the community triage bot pr:port-candidate Worth porting into the private source tree labels Aug 25, 2026
@viniciusdebruin

Copy link
Copy Markdown
Author

Thanks for the review. I addressed the three points in c488f2e:

  • Each recovery attempt now snapshots and restores the message history, partial output, and context before retrying, so failed-attempt side effects are not carried into the successful attempt.
  • Credits and child run IDs are preserved from the failed attempt and merged without double-counting usage.
  • MAX_AGENT_STEP_RECOVERY_ATTEMPTS is now 2, so the existing exponential backoff is exercised for two additional bounded attempts.

Validation:

  • Focused recovery and integration tests: 42 passed, 0 failed.
  • Typecheck reaches only the same two pre-existing missing agents-graveyard/researcher imports; there are no errors in the changed files.

This keeps aborts, authentication failures, and client 4xx responses non-retryable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:triaged Classified by the community triage bot pr:port-candidate Worth porting into the private source tree

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants