feat(session): retry safe recovery up to 3 times with exponential backoff - #1008
Merged
Conversation
…koff Safe recovery handles a clean stream disconnect before any visible output or tool side effect, where it is safe to silently replay the assistant turn. Previously it retried only once with a fixed 1s delay and then errored the session out. Raise SAFE_RECOVERY_MAX_ATTEMPTS from 1 to 3 and reuse the existing API-path delay() backoff (2s -> 4s -> 8s, capped at 30s) instead of the fixed 1s wait. The budget gate is already generic (safeRecoveryAttempt < maxAttempts); recoveryFor now reports max_attempts: 3, backoff_ms: 2000. The API-error retry path (policy(), 10 attempts) is unchanged. Closes #1006
github-actions
Bot
added
the
harness
Contributor
|
Warning Review limit reached
More reviews will be available in 45 minutes and 4 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR raises safe-recovery max attempts to 3, replaces the fixed replay delay with a shared exponential backoff (threadable via a safeRecoveryDelay hook), adds lifecycle "closing begins" watchers to interrupt backoff races, switches run-incident recommendation to "auto_retry", updates recorder guards, and updates tests accordingly. ChangesSafe-recovery retry backoff upgrade
Sequence DiagramsequenceDiagram
participant Client
participant Processor
participant SessionRetry as RetryPolicy
participant LifecycleProvenance as Lifecycle
Client->>Processor: run session / encounter disconnect
Processor->>RetryPolicy: safeRecoveryPolicy(attempt, delay?)
RetryPolicy-->>Processor: waitDuration (persist next)
Processor->>Lifecycle: whenLifecycleCloseBegins(directory)
par backoff vs close
RetryPolicy->>Processor: backoff timeout fires
Processor->>Processor: schedule replay attempt
and
Lifecycle->>Processor: notify closing begun
Processor->>Processor: re-check retryStillAllowed(during_backoff)
Processor-->>Client: abort if closing/blocked
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request increases the automatic safe recovery retry budget from 1 to 3 attempts and replaces the static 1-second delay with an exponential backoff schedule (2s, 4s, 8s). It updates the recovery policies, types, and corresponding integration and unit tests to accommodate the multi-attempt budget. The review feedback points out an inaccurate comment in retry.test.ts regarding test execution time, clarifying that the test runs instantly because Schedule.toStepWithMetadata only calculates the backoff duration without actually sleeping.
Astro-Han
removed
P2
github-actions
Bot
added
harness
Astro-Han
removed
P2
github-actions
Bot
added
harness
The recommendation value carried a misleading `_once` suffix now that safe recovery retries up to 3 times. Rename across type definition, policy, safety gate, presentation, and all test fixtures.
…play in recorder When safe recovery retries exhaust the budget, the final auto_replay_blocked decision now overwrites the earlier replay decision in the recorder diagnostics. Previously the recorder skipped all subsequent recordRecoveryDecision calls after the first retry_attempted, so the diagnostic summary never reflected the budget exhaustion reason.
…e close Race the backoff sleep against a 500ms lifecycle-close polling loop so that beginLifecycleClose (maintenance reload/dispose) is detected during the sleep, not only at the before/after_backoff checkpoints. Also check isLifecycleClosing in retryStillAllowed so the polling path and the checkpoint path both cover maintenance closes.
…signal, injectable delay P3-1: policy.ts now imports SAFE_RECOVERY_MAX_ATTEMPTS and RETRY_INITIAL_DELAY from retry.ts instead of hardcoding 3 / 2_000. P3-2: Replace 500ms polling loop in processor with whenLifecycleCloseBegins() — a promise-based signal from lifecycle-provenance that resolves when beginLifecycleClose or withLifecycleCloseAction fires for the directory. P3-3: safeRecoveryPolicy accepts an optional delay function. Processor threads it from its Input type. Tests inject a 10ms delay, dropping processor-effect from ~118s to ~21s and retry.test.ts from ~25s to ~11s.
whenLifecycleCloseBegins now returns { promise, cancel } instead of a
bare Promise. The processor wraps it in Effect.callback with a
cleanup finalizer that calls cancel() on interrupt, so the waiter is
removed from closingStartWaiters when backoff completes normally.
Adds closingStartWaiterCount() for test observability and a test that
verifies no waiter leaks after a full safe-recovery exhaust cycle
without any lifecycle close.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/opencode/test/session/retry-decision.test.ts (1)
5-10:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate this fixture to the new 3-attempt auto-retry contract
recommendationwas renamed, butauto_retry.max_attemptsandbackoff_msstill describe the old one-retry behavior. That means the budget-exhaustion case below is still validating the pre-PR boundary, so this suite would miss a regression that blocks attempt 2 or 3 too early.Suggested fix
const safeReplayGate: RunIncident.Recovery = { recommendation: "auto_retry", confidence: "high", reason: "reasoning_only_without_final_text_or_tool_activity", - auto_retry: { max_attempts: 1, backoff_ms: 1_000 }, + auto_retry: { max_attempts: 3, backoff_ms: 2_000 }, safety_scope: "visible_output_and_tool_side_effects", }Then move the exhaustion assertion to
safeRecoveryAttempt: 3so the test matches the new contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/test/session/retry-decision.test.ts` around lines 5 - 10, Update the fixture object safeReplayGate (type RunIncident.Recovery) to reflect the new 3-attempt auto-retry contract: change the renamed recommendation field to its new name as used elsewhere in the codebase, set auto_retry.max_attempts to 3 (instead of 1) and keep backoff_ms as appropriate (1_000), and update any related test variable safeRecoveryAttempt to 3 so the exhaustion assertion validates the new third-attempt boundary.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/opencode/src/session/processor.ts`:
- Around line 1458-1463: The current catchCause on the Effect.race swallows all
causes; change it to only convert the specific schedule-exhausted cause produced
by Schedule.toStepWithMetadata into "exhausted" and rethrow all other causes so
interruptions and real failures still propagate and allow Effect.onInterrupt(()
=> recordProcessInterrupt(attemptID)) to run. Concretely, replace the
unconditional Effect.catchCause with a handler that inspects the incoming Cause
(from Effect.catchCause), detects the schedule-done/exhausted marker emitted by
Schedule.toStepWithMetadata (or otherwise matches the schedule exhaustion
Cause), and returns Effect.succeed("exhausted" as const) only in that case; for
any other Cause rethrow using Effect.failCause(cause) so failures and interrupts
remain intact. Ensure references: backoffResult, Effect.race(safeRecoveryStep,
lifecycleCloseWatch), Effect.catchCause, Schedule.toStepWithMetadata,
recordProcessInterrupt, and attemptID are used to locate and implement the fix.
---
Outside diff comments:
In `@packages/opencode/test/session/retry-decision.test.ts`:
- Around line 5-10: Update the fixture object safeReplayGate (type
RunIncident.Recovery) to reflect the new 3-attempt auto-retry contract: change
the renamed recommendation field to its new name as used elsewhere in the
codebase, set auto_retry.max_attempts to 3 (instead of 1) and keep backoff_ms as
appropriate (1_000), and update any related test variable safeRecoveryAttempt to
3 so the exhaustion assertion validates the new third-attempt boundary.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f2d8638a-2084-4d56-96e8-47cfbd122c0f
📒 Files selected for processing (14)
packages/opencode/src/session/lifecycle-provenance.tspackages/opencode/src/session/processor.tspackages/opencode/src/session/retry.tspackages/opencode/src/session/run-incident/policy.tspackages/opencode/src/session/run-incident/presentation.tspackages/opencode/src/session/run-incident/safety-gate.tspackages/opencode/src/session/run-incident/types.tspackages/opencode/src/session/run-observability/recorder.tspackages/opencode/test/session/export.test.tspackages/opencode/test/session/processor-effect.test.tspackages/opencode/test/session/retry-decision.test.tspackages/opencode/test/session/retry.test.tspackages/opencode/test/session/run-incident-safety-gate.test.tspackages/opencode/test/session/run-observability.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/opencode/test/session/run-incident-safety-gate.test.ts
- packages/opencode/src/session/run-incident/policy.ts
- packages/opencode/test/session/processor-effect.test.ts
…terrupts catchCause on the backoff race now checks Cause.hasInterruptsOnly: pure interrupts are re-raised as Effect.interrupt so onInterrupt still fires, while schedule exhaustion maps to "exhausted".
This was referenced Jun 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Safe recovery handles a clean stream disconnect that happens before any
visible output or tool side effect — the case where it is safe to silently
replay the assistant turn. Previously it retried only once with a fixed
1s delay; if that single replay also failed, the session errored out.
On a flaky connection one extra immediate retry is often not enough.
Closes #1006.
Change
SAFE_RECOVERY_MAX_ATTEMPTSfrom1to3.delay()backoff (2s -> 4s -> 8s, capped at30s) instead of the fixed 1s wait. No jitter (deliberate — matches the
existing API-path schedule).
recoveryFor()now reportsauto_retry: { max_attempts: 3, backoff_ms: 2000 }.The budget gate was already generic (
safeRecoveryAttempt < maxAttempts), sono gate logic changed. Each replay still emits a
type: "retry"status withattempt1/2/3 and the schedulednexttimestamp, so the frontend keepsshowing its "Recovering…" state.
Out of scope
policy(), 10 attempts) is unchanged.the first retry. This only affects developer-facing diagnostics logging,
never the user or the final success/failed outcome — left as-is by design.
Verification
bun test test/session/retry.test.ts— 34 passbun test test/session/processor-effect.test.ts— 33 passbun test test/session/run-incident-safety-gate.test.ts test/session/run-observability.test.ts— 106 passbun run typecheck— cleanNew/updated coverage asserts the real backoff is
2s/4s/8sacross the budgetthen terminates, the safety gate allows 3 replays and blocks the 4th, and the
processor fixtures model 4 total stream calls (initial + 3 replays).
Note: reusing the real backoff makes the
processor-effectsuite wait theactual 2+4+8s per exhausted-budget case, so its wall-clock time grows
(~77s -> ~118s locally). The new
retry.test.tsbackoff test gets an explicit20s timeout so it stays green under Bun's 5s default.
Summary by CodeRabbit