DOS-1595: park quota retries until reset window - #16
Conversation
Co-authored-by: multica-agent <github@multica.ai>
WalkthroughProvider quota failures are now retryable with parsed or default backoff timing. Retry tasks persist ChangesProvider quota retry flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TaskService
participant QuotaParser
participant AgentTaskQueue
participant Issue
TaskService->>QuotaParser: Parse provider quota reset timing
QuotaParser-->>TaskService: Return retry timestamp
TaskService->>AgentTaskQueue: Create retry with wait_reason and queued_after
TaskService->>Issue: Post retry scheduling comment
AgentTaskQueue-->>TaskService: Claim task after queued_after
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/internal/service/task.go (1)
1588-1595: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLogging a zero-value
queued_afterfor non-quota retries.
child.QueuedAfter.Timeis logged unconditionally; for non-quota retriesQueuedAfter.Validis false, so this logs the Go zero time (0001-01-01...), which is noisy/misleading in logs.🧹 Optional cleanup
+ queuedAfter := interface{}(nil) + if child.QueuedAfter.Valid { + queuedAfter = child.QueuedAfter.Time + } slog.Info("task auto-retry enqueued", "parent_task_id", util.UUIDToString(parent.ID), "child_task_id", util.UUIDToString(child.ID), "reason", reason, "attempt", child.Attempt, "max_attempts", child.MaxAttempts, - "queued_after", child.QueuedAfter.Time, + "queued_after", queuedAfter, )🤖 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 `@server/internal/service/task.go` around lines 1588 - 1595, Update the “task auto-retry enqueued” log to include queued_after only when child.QueuedAfter.Valid is true; omit the field for non-quota retries instead of logging child.QueuedAfter.Time’s zero value, while preserving the existing fields and values.server/pkg/db/queries/agent.sql (1)
482-522: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winExclude
queued_afterfromExpireStaleQueuedTasks.
queuedTTLSecondsis 2 hours, butCreateRetryTaskcan park provider-quota retries until a provider reset window that is longer than normal queue backlog. Rows withstatus = 'queued'andqueued_after IS NULL OR queued_after <= now()are not claimable yet, so this sweeper fails them withfailure_reason='queued_expired'instead of waiting for the reset window.🩹 Proposed guard
WITH victims AS ( SELECT id FROM agent_task_queue WHERE status = 'queued' AND created_at < now() - make_interval(secs => `@ttl_secs`::double precision) + AND (queued_after IS NULL OR queued_after <= now()) ORDER BY created_at ASC LIMIT `@max_per_tick`::int FOR UPDATE SKIP LOCKED )🤖 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 `@server/pkg/db/queries/agent.sql` around lines 482 - 522, Update ExpireStaleQueuedTasks so both the victims CTE and outer UPDATE exclude deferred retries by requiring queued_after IS NULL OR queued_after > now(). Preserve expiration for immediately claimable queued rows while leaving rows parked until their provider reset window.
🤖 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 `@server/internal/service/task.go`:
- Around line 1443-1452: Update the parked quota-retry handling near retried and
createAgentComment so chat-session tasks also receive the parked-retry message
when IssueID is invalid. Reuse the existing formatted message and route it
through the chat-session history/messaging path, while preserving the
issue-comment path for tasks with a valid IssueID and ensuring users see at
least one failure message.
---
Outside diff comments:
In `@server/internal/service/task.go`:
- Around line 1588-1595: Update the “task auto-retry enqueued” log to include
queued_after only when child.QueuedAfter.Valid is true; omit the field for
non-quota retries instead of logging child.QueuedAfter.Time’s zero value, while
preserving the existing fields and values.
In `@server/pkg/db/queries/agent.sql`:
- Around line 482-522: Update ExpireStaleQueuedTasks so both the victims CTE and
outer UPDATE exclude deferred retries by requiring queued_after IS NULL OR
queued_after > now(). Preserve expiration for immediately claimable queued rows
while leaving rows parked until their provider reset window.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4e0638f8-495c-49eb-8e73-ee72e3f35baf
⛔ Files ignored due to path filters (5)
server/pkg/db/generated/agent.sql.gois excluded by!**/generated/**server/pkg/db/generated/autopilot.sql.gois excluded by!**/generated/**server/pkg/db/generated/chat.sql.gois excluded by!**/generated/**server/pkg/db/generated/models.gois excluded by!**/generated/**server/pkg/db/generated/runtime.sql.gois excluded by!**/generated/**
📒 Files selected for processing (7)
server/cmd/server/rerun_session_test.goserver/internal/handler/squad_comment_trigger_test.goserver/internal/service/task.goserver/internal/service/task_complete_race_test.goserver/migrations/121_agent_task_queue_queued_after.down.sqlserver/migrations/121_agent_task_queue_queued_after.up.sqlserver/pkg/db/queries/agent.sql
| if retried != nil && | ||
| failureReason == taskfailure.ReasonAgentProviderQuotaLimit.String() && | ||
| task.IssueID.Valid && | ||
| retried.QueuedAfter.Valid { | ||
| msg := fmt.Sprintf( | ||
| "Provider quota exhausted; parked retry until %s.", | ||
| retried.QueuedAfter.Time.UTC().Format(time.RFC3339), | ||
| ) | ||
| s.createAgentComment(ctx, task.IssueID, task.AgentID, msg, "system", task.TriggerCommentID) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate task.go and relevant symbols"
fd -a 'task\.go$' . | sed 's#^\./##'
echo
echo "Extract relevant section and nearby comment/send logic"
if [ -f server/internal/service/task.go ]; then
wc -l server/internal/service/task.go
sed -n '1400,1505p' server/internal/service/task.go | cat -n -v | sed 's/^/ /'
else
echo "server/internal/service/task.go not found"
fi
echo
printf '\nSearch for createAgentComment usages and ChatSessionID conditions around task retries\n'
rg -n "createAgentComment|ReasonAgentProviderQuotaLimit|RetryStatus|retried != nil|ChatSessionID|IssueID.Valid|TaskStatusRetry" server/internal/service/task.go server/internal -SRepository: firefly-events/multica
Length of output: 30144
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect maybe retry and retry condition around failed task handling"
sed -n '1500,1615p' server/internal/service/task.go | cat -n -v | sed 's/^/ /'
echo
echo "Find MaybeRetryFailedTask implementation and its return shape"
rg -n "func .*MaybeRetryFailedTask|RetryTask|MaybeRetry|New.*Task|QueuedAfter" server/internal/service/task.go server/internal -S
echo
echo "Focused searches for MaybeRetryFailedTask"
for f in $(rg -l "MaybeRetryFailedTask" server); do
echo "--- $f ---"
wc -l "$f"
ast-grep outline "$f" --match MaybeRetryFailedTask --view expanded || true
rg -n -C 4 "MaybeRetryFailedTask" "$f"
doneRepository: firefly-events/multica
Length of output: 14386
Add a chat-session equivalent for parked quota retries.
When a chat-session task is retried for agent_provider_quota_limit, retried is not nil so the chat history mirror is skipped; because IssueID is not valid, the system comment path is also skipped. Either surface the same parked-retry message to chat sessions or change the suppression so the user sees at least one failure message.
🤖 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 `@server/internal/service/task.go` around lines 1443 - 1452, Update the parked
quota-retry handling near retried and createAgentComment so chat-session tasks
also receive the parked-retry message when IssueID is invalid. Reuse the
existing formatted message and route it through the chat-session
history/messaging path, while preserving the issue-comment path for tasks with a
valid IssueID and ensuring users see at least one failure message.
1 participant
Closes DOS-1595
Summary
Tests
Summary by CodeRabbit
New Features
Bug Fixes