Wire global model fallback chain on rate/usage limits + reset-aware cooldown - #2271
Wire global model fallback chain on rate/usage limits + reset-aware cooldown#2271lsm wants to merge 3 commits into
Conversation
Sessions hitting rate/usage caps (e.g. third-party relay 429 with a reset timestamp) never recovered: the watchdog retried the same model at a fixed 10min x3 then gave up, and GlobalSettings.fallbackModels/modelFallbackMap had no runtime consumers. - New pure fallback-recovery module: chain resolution (map override vs global), next-entry selection (skip tried / same-model / unavailable), format-agnostic reset-timestamp extraction (ISO-8601, YYYY-MM-DD HH:mm:ss incl. the Chinese relay shape, epoch s/ms), and a backoff ladder (10m->4h, cap 8h) with jitter. - RateLimitWatchdog drives two-phase recovery: (A) immediate fallback-model switch via injected deps (free, tracked per-episode), then (B) a cooldown at the parsed reset time or on the backoff ladder. Reset-known waits don't count toward maxAutoRetries. - AgentSession wires the watchdog to settings (chain), the provider registry (availability), model-switch (switch+retry after the failed query's finally), and the internal event bus (pause/resume). - Add session.rate_limit_pause / session.rate_limit_resume events.
…-resume With the fallback chain wired (previous commit), a 429 no longer fails a Space worker task — the error broadcast is skipped so the session recovers or waits. This commit adds the visible status: when a worker session pauses on a cap with no fallback left, mark its task rate_limited / usage_limited with a resume-at restriction, and restore it to in_progress when the limit lifts. - Migration 163 widens space_tasks.status CHECK (rate_limited, usage_limited) and adds a nullable restrictions column; test-DB helper kept in parity. - SpaceTaskStatus + SpaceTask.restrictions (TaskRestriction) in shared types; repo reads/writes the JSON blob. - TaskAgentManager subscribes to session.rate_limit_pause/resume and maps the session to its parent task, setting/clearing the paused status. - Web status maps (labels, badges, transitions) cover the two new statuses.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (NeoKai)
Model: glm-5.1 | Client: NeoKai | Provider: GLM
Recommendation: REQUEST_CHANGES
Reviewed from scratch: full diff, all 17 changed files, the integration points (query-runner.ts, model-switch-handler.ts, processing-state-manager.ts), and the surrounding space-runtime tick/rehydrate paths. Ran the new/changed unit suites (53 + 70, all green) and bun run check (lint/typecheck/knip/parity/session-guards/test-quality — all clean). The coder's verification claims hold.
What's solid. The pure recovery module (fallback-recovery.ts) is well-factored and correctly handles the Chinese relay shape (2026-07-22 17:55:10 via LOCAL_DATETIME_RE, the [1308] code correctly ignored), the backoff ladder, and the freeWait semantics. The concurrency core is sound: switchAndRetryForFallback correctly await this.queryPromise so the failed query's finally (null queryObject, env restore, setIdle) completes before handleModelSwitch runs — I traced no double-query / dedup / late-finally race, and cross-provider switching works (config-only branch swaps provider + clears sdkSessionId). Migration 163 follows the established M98/M162 table-rebuild pattern (FK-off, column copy, idempotent guard) and is safe. The onMarkApiSuccess → reset() → notifyResume resume path is correctly wired.
The findings below are real and actionable but none are crash/incorrect-output bugs — they concern reliability of the feature's headline guarantee and test coverage of its central invariants.
P2-1 — Auto-resume does not survive a daemon restart (the motivating use case)
The cooldown is an in-memory unref'd setTimeout (rate-limit-watchdog.ts:295). The persisted restrictions.resetAt blob (migration 163 + repo round-trip) is written on pause but never read on startup. The space-runtime tick loop only drives open/in_progress/blocked tasks (space-runtime.ts:4066), so a usage_limited/rate_limited task is never re-driven, and rehydrating a session constructs a fresh watchdog with no timer/retryCount.
Consequence: for a 5-hour or weekly cap — the exact scenario this PR exists to fix — a daemon restart during the wait leaves the task paused indefinitely with no auto-resume and no error. This is the same "persisted data with no runtime consumer" anti-pattern this PR removes for fallbackModels; the new resetAt repeats it. The manual Resume button (TaskStatusActions.tsx: rate_limited->in_progress) is an escape hatch, so it is recoverable, not a hard-stuck — but the "auto-resumes after reset" criterion (Part C / VERIFICATION) is unmet across restarts, and a paused task with a future resetAt that nobody arms is misleading in the UI.
Fix options: on daemon/workflow start, scan paused tasks — those with resetAt in the past → restore in_progress + clear restrictions; those still future → re-arm a runtime-level scheduled restore (or re-arm the watchdog cooldown on rehydrate). At minimum, explicitly document that auto-resume requires daemon uptime. The persisted resetAt should either be consumed or not persisted as a resume promise.
P2-2 — fireImmediateFallback has no error boundary; a rejection sticks fallbackPending = true
rate-limit-watchdog.ts:321-339 is void-fired from scheduleRetry:239 with no try/catch. switchAndRetryForFallback has a catch-all, but its own catch calls this.stateManager.setIdle() which can throw (DB write); the recursive await this.scheduleRetry(...) at :337 can also reject. If anything escapes, the promise rejects unhandled AND this.fallbackPending is never reset (:326 is skipped), so getState() reports 'fallback-pending' forever and retryNow() (:365) hard-returns false — a stuck-visible state with no recovery short of reset(). Wrap the body in try/catch: on error, log, clear fallbackPending, mark the entry tried, and fall through to a cooldown.
P2-3 — Untested load-bearing invariants
Three behaviors central to this feature have no test, so they will silently regress:
- Parsed-reset waits bypass
maxAutoRetries. This is the design guarantee ("reset-known waits don't count toward the budget"). The only exhaustion test (rate-limit-watchdog.test.ts:159) uses'429'(no timestamp) and assertsfalse; nothing asserts that withretryCount === maxAutoRetries+ a parseable ISO reset,scheduleRetryreturnstrueandretryCountstays put. - Late resume does not resurrect a cancelled/done task. Explicitly in the review checklist. The source guard (
restoreTaskFromRateLimit:610) is correct, but the only resume tests coverusage_limited→in_progress(:99) and thein_progressno-op (:141); nothing publishessession.rate_limit_resumeagainst acancelled/done/archivedtask. The terminal-override test (:129) only coversdoneon the pause side. resetAtbuffer arithmetic on thenotifyPausepayload is asserted nowhere (watchdog emitsdecision.retryAtMs= reset + 30s,:285); a regression dropping the buffer passes every test.
P3 — optional polish (not blocking on their own)
rate-limit-watchdog.ts:283firesnotifyPausebeforesetRateLimitCooldown(:289); reorder so the processing-state flip precedes the bus event and avoids a briefidlewindow whereonIdleCallbackruns.rate-limit-watchdog.ts:237comment says scheduleRetry "must return true synchronously" — it isawaited atquery-runner.ts:1314; rephrase to "must resolve to true".fallback-recovery.ts:301-312classifyLimitKindkeywords are broad ('exceeded','limit reached') and any parsed timestamp →usage_limit, so a transient "rate limit exceeded, retry in 60s" is labelled a daily/weekly cap. Label-only impact (both resume identically), but worth narrowing.- A manual status transition out of
rate_limited/usage_limitedviasetTaskStatus(not the resume path) leaves a stalerestrictionsblob; clear it on exit. No frontend consumer yet, latent. VALID_SPACE_TASK_TRANSITIONSomitsrate_limited/usage_limited → archived(every other non-terminal status can archive). Likely intentional; confirm.
Happy to re-review once P2-1 through P2-3 are addressed.
…ry, invariant tests P2-1 auto-resume across daemon restart: the in-memory watchdog cooldown doesn't survive a restart, so add a SpaceRuntime tick sweep (recoverRateLimitedTasks) that restores rate/usage-limited tasks whose persisted restrictions.resetAt has passed to in_progress (clearing restrictions), driven off the persisted blob so the normal rehydration restarts the worker. Tasks with a future resetAt stay paused. New repo helper listRateLimitedBySpace. P2-2 error boundary: wrap fireImmediateFallback in try/catch so a rejecting switchAndRetry or recursive scheduleRetry can't leave fallbackPending stuck true (which froze getState/retryNow). On error: clear fallbackPending, mark the entry tried, fall through to a cooldown. P2-3 invariant tests: parsed-reset wait bypasses maxAutoRetries (true + retryCount unchanged at the budget); 30s buffer pinned on notifyPause resetAt; late resume does not resurrect cancelled/done/archived; pause-side terminal guard covers done/blocked/cancelled/archived; cross-restart sweep test. P3 polish: setRateLimitCooldown before notifyPause; fix 'return true synchronously' comment; narrow classifyLimitKind keywords (drop 'exceeded'/ 'limit reached' that mislabelled transient rate limits); auto-clear stale restrictions on manual transition out of the paused statuses; add rate_limited/usage_limited → archived transitions + labels.
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (NeoKai)
Model: glm-5.1 | Client: NeoKai | Provider: GLM
Recommendation: APPROVE
Round 2 (fresh, commit 755f340). All three P2 findings and the P3 polish are correctly addressed; I re-verified each on its merits rather than trusting the summary.
P2-1 (cross-restart auto-resume) — fixed. SpaceRuntime.recoverRateLimitedTasks() runs in the tick loop off the persisted restrictions.resetAt: past-reset → restore in_progress + clear restrictions (worker re-driven by the normal in_progress rehydrate/recoverStalledRuns path, which has separate coverage); future-reset → left paused. listRateLimitedBySpace + the repo's auto-clear of stale restrictions on any exit transition back it up. The only residual — a backoff-path rate_limited task carries a synthetic resetAt ≈ now+1h rather than the exact ladder delay — is safe: usage caps (the long-window case) always carry an accurate parsed reset, and a transient rate limit retrying slightly early is harmless. Not worth blocking.
P2-2 (error boundary) — fixed. fireImmediateFallback now wraps switchAndRetry in try/catch with a finally that always clears fallbackPending, and guards the recursive scheduleRetry with a best-effort cooldown fallback. No path leaves fallbackPending stuck or getState()/retryNow() frozen.
P2-3 (invariant tests) — fixed, and the assertions are meaningful: parsed-reset wait bypasses maxAutoRetries (returns true with retryCount unchanged at the budget); the 30s RESET_BUFFER_MS is pinned on the notifyPause payload; late resume does not resurrect cancelled/done/archived and confirms the restrictions auto-clear; pause-side terminal guard broadened to done/blocked/cancelled/archived.
P3 — all applied: setRateLimitCooldown before notifyPause; "must resolve to true" comment; classifyLimitKind narrowed (dropped the over-broad exceeded/limit reached/达到; the Chinese relay still classifies correctly via 上限/小时 and the parsed-reset path); rate_limited/usage_limited → archived transitions + labels.
Verification: bun run check clean (lint/typecheck/knip/parity/session-guards/test-quality); changed suites green (watchdog+fallback 55, listener+tick-loop+repo 125); all 4 prior threads resolved; PR open and mergeable. No new issues or regressions introduced by the fix commit. Ship it.
Sessions hitting 5h/weekly usage caps (e.g. third-party relay
429with a reset timestamp) never recovered: the watchdog retried the same model at a fixed 10min ×3 then gave up, andGlobalSettings.fallbackModels/modelFallbackMaphad no runtime consumers.This makes the configured fallback chain real and switches resets to format-agnostic parsing:
modelFallbackMap[provider/model]?? globalfallbackModels, switch to the next untried available entry via the existing model-switch machinery, and retry immediately. Repeated 429s advance through the chain; switches are free (don't count towardmaxAutoRetries).YYYY-MM-DD HH:mm:ssincl. the Chinese relay message, epoch s/ms) and wait until reset + buffer; otherwise use a backoff ladder (10m→4h, cap 8h, jitter). Reset-known waits don't count toward the retry budget.rate_limited/usage_limitedwith arestrictions.resetAtblob (migration 163) and auto-resumes toin_progresswhen the limit lifts.Pure recovery logic lives in a new
fallback-recovery.tsmodule (fully unit-tested); the watchdog is refactored to take injected deps.query-runner.ts/model-switch-handler.tsare unchanged — the existing skip-error-broadcast gate already prevents task failure on recovery.