[codex] Add runtime recovery policy#94
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR implements a runtime recovery subsystem that automatically detects rate-limit resets and transient connectivity errors, schedules retry injection with configurable backoff timing and timezone-aware quiet windows, and integrates recovery into both daemon and foreground execution paths. ChangesRuntime Recovery Implementation
Sequence DiagramssequenceDiagram
participant TextOutput as Terminal Output
participant Detector as detect_runtime_recovery
participant Extractor as _extract_reset_time
participant WindowCalc as next_allowed_recovery_at
participant PolicyCfg as RuntimeRecoveryPolicy
TextOutput->>Detector: error/rate-limit text
Detector->>Detector: classify kind
Detector->>Extractor: scan for reset timestamp
Extractor->>PolicyCfg: read reset_grace_seconds
Extractor-->>Detector: candidate retry_at
Detector->>WindowCalc: advance past quiet windows
WindowCalc->>PolicyCfg: read quiet_windows
WindowCalc-->>Detector: clamped retry_at
Detector-->>TextOutput: RuntimeRecoveryObservation
sequenceDiagram
participant LoopExecution as Loop._run_sidecar_inner
participant DetectModule as Detector
participant BuildFn as _build_recovery_instruction
participant TerminalIO as Terminal
participant EventLog as SessionEvents
LoopExecution->>DetectModule: terminal text
DetectModule-->>LoopExecution: RuntimeRecoveryObservation
LoopExecution->>EventLog: record scheduled
LoopExecution->>BuildFn: construct HandoffInstruction
BuildFn-->>LoopExecution: recovery instruction
LoopExecution->>TerminalIO: inject instruction
LoopExecution->>EventLog: record injected
LoopExecution->>EventLog: continue iteration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![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 introduces a runtime recovery system to handle provider-side failures, such as rate limits and transient connectivity issues, by automatically scheduling and injecting retries. Key changes include new configuration options for recovery policies, a dedicated runtime_recovery module for error detection, and integration into the supervisor's sidecar loop. Review feedback identified a critical logic bug where the recovery timestamp could slide forward indefinitely, potential log spamming during exhausted attempts, and a crash risk when parsing invalid timezone strings from user configuration.
| attempts = recovery_attempts.get(recovery.signature, 0) | ||
| if recovery_total_attempts < self.runtime_recovery_policy.max_attempts_per_run: | ||
| scheduled_recovery = recovery | ||
| if scheduled_recovery_signature != recovery.signature: | ||
| scheduled_recovery_signature = recovery.signature | ||
| self.store.append_session_event( | ||
| state.run_id, | ||
| "runtime_recovery_scheduled", | ||
| { | ||
| "kind": recovery.kind, | ||
| "reason": recovery.reason, | ||
| "retry_at": recovery.retry_at.isoformat(), | ||
| "attempt": recovery_total_attempts + 1, | ||
| "max_attempts": self.runtime_recovery_policy.max_attempts_per_run, | ||
| }, | ||
| ) | ||
| self.store.save(state) | ||
| else: | ||
| self.store.append_session_event( | ||
| state.run_id, | ||
| "runtime_recovery_exhausted", | ||
| { | ||
| "kind": recovery.kind, | ||
| "reason": recovery.reason, | ||
| "attempts": recovery_total_attempts, | ||
| "max_attempts": self.runtime_recovery_policy.max_attempts_per_run, | ||
| }, | ||
| ) | ||
| self.store.save(state) |
There was a problem hiding this comment.
This block has several issues:
- Redundant Assignment:
attemptsis assigned but never used here (it is correctly retrieved later at line 1094). - Sliding Fallback Delay:
scheduled_recoveryis updated every tick. If the recovery uses a fallback delay (relative tonow), theretry_attimestamp will slide forward every iteration as long as the error text persists, preventing the recovery from ever triggering. - Log Spamming: If recovery attempts are exhausted, the
runtime_recovery_exhaustedevent is appended to the session log every single tick.
All of these can be resolved by only updating the state and logging when the error signature changes.
if recovery_total_attempts < self.runtime_recovery_policy.max_attempts_per_run:
if scheduled_recovery is None or scheduled_recovery_signature != recovery.signature:
scheduled_recovery = recovery
scheduled_recovery_signature = recovery.signature
self.store.append_session_event(
state.run_id,
"runtime_recovery_scheduled",
{
"kind": recovery.kind,
"reason": recovery.reason,
"retry_at": recovery.retry_at.isoformat(),
"attempt": recovery_total_attempts + 1,
"max_attempts": self.runtime_recovery_policy.max_attempts_per_run,
},
)
self.store.save(state)
elif scheduled_recovery_signature != recovery.signature:
scheduled_recovery_signature = recovery.signature
self.store.append_session_event(
state.run_id,
"runtime_recovery_exhausted",
{
"kind": recovery.kind,
"reason": recovery.reason,
"attempts": recovery_total_attempts,
"max_attempts": self.runtime_recovery_policy.max_attempts_per_run,
},
)
self.store.save(state)| if not checkpoints: | ||
| recovery = detect_runtime_recovery( | ||
| text, | ||
| now=datetime.now(timezone.utc), |
There was a problem hiding this comment.
The variable name now is already used in this scope as a float (from time.monotonic() at line 1013). While using it as a keyword argument name here is technically valid, it makes the code harder to read, especially since now is used again as a float at line 1127. Consider using a more descriptive name for the datetime object or letting the function use its default.
| return timezone(sign * timedelta(hours=hours, minutes=minutes)) | ||
| if normalized.upper() == "UTC": | ||
| return timezone.utc | ||
| return ZoneInfo(normalized) |
There was a problem hiding this comment.
ZoneInfo(normalized) will raise a zoneinfo.ZoneInfoNotFoundError if the provided timezone name is invalid. Since this value can come from user configuration, it should be handled gracefully to avoid crashing the supervisor daemon when it attempts to parse a rate limit error.
| return ZoneInfo(normalized) | |
| try: | |
| return ZoneInfo(normalized) | |
| except Exception: | |
| return timezone.utc |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
7e2531e to
976ed7e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@supervisor/config.py`:
- Around line 277-280: The current construction of RuntimeRecoveryPolicy treats
provider_retry_* as subordinate booleans/defaults rather than fallbacks; change
the assignments so each runtime_recovery_* field is used if explicitly set (not
just truthy) and otherwise falls back to provider_retry_*. Specifically, when
creating policy_with_profile(RuntimeRecoveryPolicy(...)) compute enabled =
(self.runtime_recovery_enabled if self.runtime_recovery_enabled is not None else
self.provider_retry_enabled), and similarly for reset_timezone and profile use
(self.runtime_recovery_reset_timezone if self.runtime_recovery_reset_timezone is
not None else self.provider_retry_reset_timezone) and
(self.runtime_recovery_profile if self.runtime_recovery_profile is not None else
self.provider_retry_profile) so legacy provider_retry_* acts as a true fallback
rather than a kill-switch.
In `@supervisor/loop.py`:
- Around line 918-920: The scheduled_recovery state (scheduled_recovery and
scheduled_recovery_signature) must be cleared when a real checkpoint for the run
is accepted to avoid injecting stale recoveries; update the checkpoint
acceptance code path to set scheduled_recovery = None and
scheduled_recovery_signature = "" immediately after the first accepted
checkpoint for that run (i.e., in the checkpoint acceptance/commit handler where
a run transitions to healthy), and apply the same clear logic in the other
checkpoint-handling block referenced (the second checkpoint acceptance path
covering the alternative flow).
- Around line 1062-1068: detect_runtime_recovery() is repeatedly sliding the
retry deadline by overwriting scheduled_recovery on every poll; change the logic
so you only set scheduled_recovery and append the session event when this is the
first time seeing this recovery signature (i.e., scheduled_recovery_signature !=
recovery.signature) or when no scheduled_recovery exists, and skip updating
scheduled_recovery/retry_at when the incoming recovery.signature matches the
already scheduled_recovery_signature to avoid moving retry_at forward for the
same failure; keep the existing checks for recovery_total_attempts and attempts
from recovery_attempts, but ensure scheduled_recovery,
scheduled_recovery_signature and the call to self.store.append_session_event are
only changed on signature transitions.
In `@supervisor/runtime_recovery.py`:
- Around line 110-114: The issue is that timestamp-only reset messages parsed by
_extract_reset_time (e.g., "expires at ...", "限额将在 ... 重置") are ignored because
the keyword check gates recovery; change the logic so you call
_extract_reset_time(text, policy=policy) first and, if it returns a datetime,
set retry_at to that (then pass through next_allowed_recovery_at), otherwise
fall back to the existing keyword check and rate_limit_fallback_delay_seconds
behavior; update the branch involving "429"/"rate limit"/"使用上限" to only handle
fallback when _extract_reset_time returned None, keeping function names
_extract_reset_time and next_allowed_recovery_at and variables retry_at, policy,
now unchanged for easy location.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d3f66db7-5839-49e8-a44b-b36ea5b6a8c2
📒 Files selected for processing (6)
supervisor/app.pysupervisor/config.pysupervisor/daemon/server.pysupervisor/loop.pysupervisor/runtime_recovery.pytests/test_runtime_recovery.py
976ed7e to
31acce2
Compare
1 participant
Summary
Behavior
retryto resume the current interrupted action.Validation
pytest -q tests/test_runtime_recovery.py tests/test_config.py tests/test_sidecar_loop.pypytest -q(1237 passed)Summary by CodeRabbit
New Features
Tests