Bugfix/623 rate limit poisons dependency graph - #649
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #623 by adding first-class rate-limit detection/handling so transient provider limits don’t consume task retry budgets or lead to max-retries skips that can silently starve dependent tasks. It also improves dependency-graph visibility by surfacing deadlock/starvation conditions both in scheduler messaging and at key workflow-runner decision points.
Changes:
- Add
RateLimitErrorfailure classification plusGet-RateLimitResetTimeto parse common “resets … / try again in …” hints. - Update the workflow runner to defer/backoff on rate limits without consuming retry budget, and to run dependency-deadlock detection at max-retries skip time and at queue drain.
- Improve scheduler “no tasks” messaging to include names of tasks blocked by unmet dependencies; add tests and new execution settings defaults.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Test-TaskActions.ps1 | Adds regression tests for scheduler messaging and deadlock detection around framework-error skips (max-retries). |
| tests/Test-Components.ps1 | Adds unit tests for RateLimitError classification and Get-RateLimitResetTime parsing behavior. |
| src/runtime/Scripts/Invoke-WorkflowProcess.ps1 | Implements rate-limit deferral/backoff/park behavior and triggers deadlock checks at key points; adds new execution settings. |
| src/runtime/Modules/Dotbot.Process/Dotbot.Process.psm1 | Enhances “no tasks” message with blocked task names; adds/uses deadlock detection to surface poisoned subtrees. |
| src/runtime/Modules/Dotbot.Harness/Private/Failure.ps1 | Adds RateLimitError rule (precedence over AuthError) and implements Get-RateLimitResetTime. |
| src/runtime/Modules/Dotbot.Harness/Dotbot.Harness.psm1 | Exports Get-RateLimitResetTime. |
| src/runtime/Modules/Dotbot.Harness/Dotbot.Harness.psd1 | Adds Get-RateLimitResetTime to FunctionsToExport. |
| content/settings/settings.default.json | Introduces default values for new execution settings controlling retries and rate-limit deferral behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $resetIn = Get-RateLimitResetTime -ErrorText 'Rate limited. Try again in 45 seconds.' | ||
| Assert-True -Name "Get-RateLimitResetTime parses 'try again in 45 seconds'" ` | ||
| -Condition ($null -ne $resetIn -and ($resetIn - (Get-Date)).TotalSeconds -gt 150 -and ($resetIn - (Get-Date)).TotalSeconds -lt 180) ` | ||
| -Message "Expected ~165s from now, got '$resetIn'" |
| $resetCodexAbbrev = Get-RateLimitResetTime -ErrorText 'Rate limit reached for gpt-5.5 on tokens per min. Please try again in 3s.' | ||
| Assert-True -Name "Get-RateLimitResetTime parses Codex's abbreviated 'try again in 3s'" ` | ||
| -Condition ($null -ne $resetCodexAbbrev -and ($resetCodexAbbrev - (Get-Date)).TotalSeconds -gt 118 -and ($resetCodexAbbrev - (Get-Date)).TotalSeconds -lt 125) ` | ||
| -Message "Expected ~123s from now (3s + 120s margin), got '$resetCodexAbbrev'" |
| Write-Status "Usage limit window elapsed — retrying: $($task.name)" -Type Info | ||
| Write-ProcessActivity -Id $procId -ActivityType "text" -Message "Rate-limit wait elapsed — retrying task '$($task.name)'" | ||
| continue |
… response (andresharpe#623) Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| if ($ErrorText -match '(?:try\s+again|retry)\s+(?:in|after)\s+(\d+)\s*(s|sec|secs|second|seconds|m|min|mins|minute|minutes|h|hr|hrs|hour|hours)\b') { | ||
| $n = [int]$Matches[1] | ||
| $unit = $Matches[2].ToLowerInvariant() | ||
| $span = if ($unit.StartsWith('s')) { [TimeSpan]::FromSeconds($n) } | ||
| elseif ($unit.StartsWith('m')) { [TimeSpan]::FromMinutes($n) } | ||
| else { [TimeSpan]::FromHours($n) } | ||
| return $now.Add($span).Add($safetyMargin) | ||
| } |
| $resetAt = Get-RateLimitResetTime -ErrorText $harnessErrText | ||
| $withinWaitBudget = $resetAt -and (($resetAt - (Get-Date)) -le [TimeSpan]::FromMinutes($rateLimitMaxWaitMinutes)) | ||
| # A hard quota/billing exhaustion will not clear on its own — it | ||
| # needs an operator to add credit or upgrade — so it must never | ||
| # spend a short-backoff retry; it goes straight to park. | ||
| $looksLikeBilling = [bool]($harnessErrText -match '(?i)insufficient_quota|quota\s+exceeded|billing|upgrade\s+your\s+plan|insufficient\s+(?:funds|credit)') | ||
| # When to resume: the advertised reset if within budget; else a |
carlospedreira
left a comment
carlospedreira
left a comment
There was a problem hiding this comment.
Request changes before merge. The new Get-NextWorkflowTask message reports the aggregate blockedCount as tasks blocked by unmet dependencies, but that counter also includes barrier waits and failed condition-marking. In mixed cases the count/category and listed names are misleading. Please either split the counts/categories or restrict this message to dependency-blocked tasks, with a regression test covering mixed blockers.\n\nAlso add source-controlled runner-path coverage for rate-limit handling (retry budget preservation, deferral cap, billing/quota with a reset hint, and stop during the wait). The checked-in tests cover classifier/parser and scheduler/deadlock behavior, while the runner matrix is currently only described as a standalone harness.\n\nAfter updating, please rebase onto current main and run fresh Layers 1-3 CI; the existing checks are from July 15.
|
Additional design direction: rate limits should not receive a bespoke execution path. They are errors like other provider failures and should use the framework's existing normal error/retry behavior. Please remove the RateLimitError-specific parsing, wait/backoff loop, separate deferral cap, and rate-limit-only parking logic rather than adding more coverage around that behavior. Any error classification should remain only where it serves existing generic handling, not drive a rate-limit-specific policy. |
Linked issue
Closes #623
Summary of changes
Rate-limiting mid-task no longer burns the retry budget or poisons the
dependency graph.
Classification & backoff
RateLimitErrorfailure class (evaluated beforeAuthError) matchingClaude/Codex/Gemini/generic-429 wording, with a guarded 429 regex that
ignores bare numbers ("returned 429 items").
Get-RateLimitResetTimebest-effort parses a reset hint ("resets 3:30pm","try again in 45s", "retry after 2 minutes") into an absolute local time.
within
rate_limit_max_wait_minutes; else, for a transient limit with noreset hint, a short
rate_limit_no_reset_backoff_seconds(default 60s)backoff-and-retry; else park to needs-input. Hard quota/billing exhaustion
always parks immediately. Rate-limit waits never consume the retry budget
and are capped by
rate_limit_max_deferrals.Dependency-graph visibility
dependents. The scheduler's "no tasks" message now names the starved
task(s), and
Test-DependencyDeadlockis wired in at the two points apoisoned subtree becomes observable (right after a max-retries skip, and at
queue drain / wait).
Process-state correctness
-BotRootto thecanonical root, so they don't resolve through the worktree
.controljunction (guards the fix(process): pin process-state writes to canonical BotRoot, not worktree #637 race in the new wait path).
New
executionsettings:max_retries_per_task,rate_limit_max_wait_minutes,rate_limit_max_deferrals,rate_limit_no_reset_backoff_seconds.Screenshots / recordings
N/A — no UI changes.
Testing notes
pwsh tests/Test-Components.ps1- 856/856pwsh tests/Test-TaskActions.ps1- 190/190, incl. the four Rate-limit mid-task burns retries → hard-skip poisons dependency graph #623scheduler/deadlock cases.
resume/park decision matrix (with the billing regex lifted verbatim from the
runner), the deadlock/scheduler path, and the
-BotRootinvariant: 37/37.Checklist
runner rate-limit branch covered by a standalone validation harness)