fix(scheduler): count a long turn as dispatched, bound fire retries (#337) - #340
fix(scheduler): count a long turn as dispatched, bound fire retries (#337)#340mabry1985 wants to merge 2 commits into
Conversation
…337) `_fire` awaited the entire agent turn on a 30s client timeout, so any turn longer than that raised ReadTimeout and was scored a delivery failure. Since `_reschedule_or_delete` only ran on success, `next_fire` never advanced and `last_fire` stayed null — leaving the row permanently past-due for the 1s poll loop to re-claim every tick. Live on the Deck a `*/15 * * * *` job fired every ~31s for ~18h, and because the server keeps running a turn after the client disconnects, every "failed" fire still cost a full turn: ~2,000 orphaned tasks and continuous unattended token burn. - `_fire` now treats ReadTimeout as *dispatched* — the agent accepted the prompt and is working. Connect errors and HTTP error responses stay failures. - Split the timeout: a short connect timeout plus a dispatch-confirmation read window, so the poll loop no longer blocks for a whole turn. - Failures now back off exponentially (persisted to `next_fire`, so a restart mid-backoff can't reset to firing every tick) and give up after 5 attempts: cron skips to its next slot, one-shots are dropped. Tests cover the regression directly: five consecutive ticks against a slow-turn agent must produce exactly one fire, with `next_fire` advanced and `last_fire` recorded. All four fail on main. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughThe LocalScheduler now treats read timeouts after dispatch as successful delivery, separates connection and dispatch timeouts, and applies exponential backoff with capped retries for genuine failures. Tests cover long turns, single-slot firing, increasing delays, and terminal cron or one-shot behavior. ChangesScheduler delivery semantics
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant SchedulerTick
participant LocalScheduler
participant AgentA2A
participant SchedulerSQLite
SchedulerTick->>LocalScheduler: Poll due job
LocalScheduler->>AgentA2A: POST scheduled prompt
alt Agent accepts and response exceeds read timeout
AgentA2A-->>LocalScheduler: ReadTimeout after dispatch
LocalScheduler->>SchedulerSQLite: Advance next_fire and record last_fire
else Delivery failure
AgentA2A-->>LocalScheduler: Connection or HTTP error
LocalScheduler->>SchedulerSQLite: Persist exponential backoff
end
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_scheduler_local.py (1)
416-428: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
_RefusingClienttest double — hoist to module scope.Identical class defined inline in two tests, while
_SlowClientabove is already correctly shared at module level for its two consumers. Extracting one shared helper avoids drift between the copies as the connection-failure simulation evolves.
tests/test_scheduler_local.py#L416-L428: remove this inline_RefusingClientdefinition; import/reuse a single module-level version instead.tests/test_scheduler_local.py#L460-L472: remove this duplicate_RefusingClientdefinition and reuse the same module-level class as above.♻️ Proposed refactor
+class _RefusingClient: + """Agent isn't accepting connections (mirrors a genuine delivery failure).""" + + def __init__(self, *a, **k): + pass + + async def __aenter__(self): + return self + + async def __aexit__(self, *a): + return False + + async def post(self, *a, **k): + import httpx + + raise httpx.ConnectError("Connection refused") + + `@pytest.mark.asyncio` async def test_delivery_failure_backs_off_instead_of_hammering(tmp_path, monkeypatch): """A genuine failure retries later — never at poll frequency.""" import httpx - class _RefusingClient: - def __init__(self, *a, **k): - pass - - async def __aenter__(self): - return self - - async def __aexit__(self, *a): - return False - - async def post(self, *a, **k): - raise httpx.ConnectError("Connection refused") - monkeypatch.setattr(httpx, "AsyncClient", _RefusingClient)Apply the same removal in
test_retries_are_capped.🤖 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 `@tests/test_scheduler_local.py` around lines 416 - 428, Hoist the duplicated _RefusingClient test double to module scope alongside _SlowClient, then remove both inline definitions in the affected locations (tests/test_scheduler_local.py:416-428 and tests/test_scheduler_local.py:460-472) so the tests reuse the shared class, including test_retries_are_capped.
🤖 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 `@scheduler/local.py`:
- Around line 177-178: Persist the consecutive delivery-failure count alongside
next_fire in the scheduler’s durable state, updating both transactionally
whenever a delivery fails so restarts retain the retry budget. Clear the
persisted counter after accepted dispatch or terminal handling, and remove
reliance on the in-memory _fire_failures map. Add a regression test that
restarts between failed deliveries and verifies the five-attempt cap still
terminates the one-shot.
---
Nitpick comments:
In `@tests/test_scheduler_local.py`:
- Around line 416-428: Hoist the duplicated _RefusingClient test double to
module scope alongside _SlowClient, then remove both inline definitions in the
affected locations (tests/test_scheduler_local.py:416-428 and
tests/test_scheduler_local.py:460-472) so the tests reuse the shared class,
including test_retries_are_capped.
🪄 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: c3439c83-d754-472d-b614-a70bcb70c527
📒 Files selected for processing (2)
scheduler/local.pytests/test_scheduler_local.py
CodeRabbit review on #340: `next_fire` survived a restart but the consecutive- failure counter lived in memory, so a process restart between failed deliveries reset the count — an unavailable one-shot could evade the five-attempt cap indefinitely, since the cap only ever bounded a single uninterrupted run. Move the counter to a `fire_attempts` column (lazy migration, same pattern as `context_id`) and write it in the same UPDATE as `next_fire`, so the delay and the budget it spends can never diverge. Cleared on accepted dispatch and when a cron job gives up and skips ahead. Two tests: the retry budget survives repeated restarts (a fresh scheduler against the same jobs.db each round still drops the one-shot at the cap), and a pre-existing jobs.db gains the column defaulting to 0. Verified to bite — simulating the in-memory counter makes every restart log "attempt 1/5" and the one-shot survives forever. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Fixed in ffcad88 — the finding was correct.
The counter now lives in a Added the restart regression test you asked for — it hands the job to a fresh Also covered the lazy migration (a pre-existing
|
1 participant
Fixes #337.
The bug
scheduler/local.pyfired a job by POSTing to its own/a2aand awaiting the entire turn onhttpx.AsyncClient(timeout=30). Any turn longer than 30s raisedReadTimeout→_firereturnedFalse→_reschedule_or_deletenever ran →next_firestayed in the past andlast_firestayednull→ the 1s poll loop re-claimed the same row every tick.Because the server keeps running a turn after the client disconnects, each "failed" fire still cost a full agent turn. Live on the Deck: a
*/15 * * * *job fired every ~31s for ~18h unattended, leaving ~2,000 orphanedTASK_STATE_WORKINGtasks.The fix
ReadTimeoutnow means dispatched, not failed. The agent accepted the prompt and is working; that's the success path. Connect errors (agent not up) and HTTP error responses remain genuine failures.next_fireso a restart mid-backoff can't reset the job to firing every tick. After 5 consecutive failures the job stops retrying that slot: cron skips to its next natural slot, one-shots are dropped rather than retried forever.The retry-on-transient-failure intent from the original code is preserved — a one-shot still survives a network blip, it just can't spin.
Tests
Four new tests, all four fail on main:
test_long_turn_fires_once_per_slot— the regression itself: five consecutive ticks against a slow-turn agent must produce exactly one fire, withnext_fireadvanced past now andlast_firerecorded (so the operator surface stops lying).test_long_turn_counts_as_dispatched— read timeout is INFO + success, not an error.test_delivery_failure_backs_off_instead_of_hammering— a failed job is never left past-due, and successive delays grow.test_retries_are_capped— cron rolls forward at the cap; one-shots are dropped.Existing
_firetests are unchanged and still pass —Truestill means "delivered", the definition just now includes "accepted and still running".50 passedacrosstest_scheduler_local.py,test_schedule_tools.py,test_operator_api_routes.py; ruff check + format clean.🤖 Generated with Claude Code
Summary by CodeRabbit