ComfyUI-Lora-Manager/docs/plans/issue-1085-rate-limit-design.md

Plan: Global Rate-Limit Abidance for Recipe Ingest & Metadata Fetching

Issue: #1085 — Large Recipe Ingest Appears to not abide by vendor rate limits, possibly a few other errors? Status: v2 — reviewed; decisions recorded in §10. Phase 1 implemented (2026-08-27, commit c2a2048c): coordinator + downloader gate + Fix C failover semantics + helper double-wait fix + settings. Phase 2 implemented (2026-08-27): batch-import rate-limit failures map to SKIPPED + rate_limited WebSocket flag + UI slowdown hint (toast + status text, i18n keys synced); download_to_memory / get_response_headers / download_file register 429 cooldowns. Changes vs v1: Fix C moved to Phase 1, helper double-wait resolved in Phase 1, gate/guard ordering specified. Scope: HTTP API traffic to CivitAI (civitai.red) and CivArchive (civarchive.com) from metadata fetching (bulk refresh, metadata sync, recipe analysis/enrichment, usage-control lookups). Large binary downloads (model files / preview images via download_file) are out of scope for pacing (they are already single-connection transfers) but their 429 responses should still be registered.

Context: a first batch of fixes for this issue was already committed as ee233548 ("fix(recipes): enforce batch-import concurrency bound and harden ingest errors (#1085)"): the batch-import concurrency controller now shares a real semaphore (bounds 1–5 actually apply), the Comfy parser tolerates list/None ckpt_name, CivArchive treats empty error payloads as failures, and offline-cooldown short-circuits log at DEBUG. This plan covers the two remaining orchestration-level fixes: Fix 2 — slow down globally when a vendor rate limit is hit (respect Retry-After, queue instead of hammering); Fix 3 — stop immediately failing over to CivArchive when CivitAI is rate-limited.

---

1. Problem Statement

During a large recipe ingest (e.g. importing the example-images directory, which can be thousands of images), the manager fires one metadata request per checkpoint + per LoRA per image through the fallback provider chain (civitai_api → civarchive_api → sqlite). Consequences observed in #1085:

1. CivitAI gets hammered → 429s. The consumer then immediately tries CivArchive for the same lookup, so CivArchive gets hammered too before it was ever naturally needed (its only real job is recovering metadata for models deleted from CivitAI).
2. Requests are retried per-call after Retry-After, but each concurrent call sleeps independently → thundering herd: thousands of coroutines wake at the same moment and re-flood the vendor.
3. While CivArchive is in the ConnectivityGuard cooldown, every batch item short-circuits and is marked FAILED — the batch import's success/failure accounting is polluted by a transient vendor state (log spam was fixed in ee233548; the item-failure accounting is not).
4. ConnectivityGuard (py/services/connectivity_guard.py) only treats transport-level unreachability as offline; HTTP 429 is invisible to it, so nothing ever intentionally paces request rate.

User expectation from the issue: "once a vendor rate limit time out is hit, you should trigger a slow down with intentional reduction in request rate".

2. Current State (verified against code)

2.1 Where 429s are surfaced

• Downloader.make_request (py/services/downloader.py:1120-1132): HTTP 429 → returns RateLimitError(message, retry_after=…) parsed from Retry-After (missing header defaults to None).
• CivitaiClient._make_request (py/services/civitai_client.py:97-100): converts RateLimitError to a raise immediately; no waiting. Transient 5xx/connection errors are retried 3× with 1s/2s/4s backoff.
• CivArchiveClient._make_request (py/services/civarchive_client.py): raises RateLimitError with provider="civarchive_api" when not set.
• _RateLimitRetryHelper (py/services/model_metadata_provider.py:45-102): per-call retry loop — sleeps retry_after (capped at 1800 s; ≥120 s ⇒ no retry), then re-raises. Because every concurrent call runs its own helper, they sleep in parallel and re-fire in parallel.
• FallbackMetadataProvider (py/services/model_metadata_provider.py:488-508, 564-584 etc.): on a final RateLimitError from one provider it logs "skipping to next provider" and continues to the next network provider — this is the direct cause of the CivArchive flood.
• MetadataSyncService.fetch_and_update_model (py/services/metadata_sync_service.py:248-333): manually iterates provider_attempts; on RateLimitError it continues to the next provider (same failover problem), then reports "Rate limited" when nothing succeeded.
• Downloader.make_request has a per-destination scope already available: _guard_destination(url) returns the hostname (downloader.py:1194-1199), used by ConnectivityGuard.

2.2 What pacing exists today

• ConnectivityGuard: per-destination cooldown (30 s base, ×2 per extra failure batch, 300 s cap) triggered only by transport errors (connectivity_guard.py:168-197).
• AdaptiveConcurrencyController (batch import, fixed in ee233548): shared semaphore enforces 1–5 concurrent items; duration-based adjustment only — it never sees HTTP statuses, so it cannot distinguish "slow because rate limited" from "slow because big image".
• No token bucket, no minimum inter-request interval, no shared Retry-After gate anywhere (grep for throttle/token-bucket/rate-limiter: 0 hits).

3. Requirements & Constraints

R1. Respect Retry-After. After a 429, no further request to that destination may be sent before the vendor's retry window elapses. R2. No thundering herd. Concurrent waiters must share one wake-up (gate), not sleep independently. R3. No double load. A CivitAI 429 must not trigger a CivArchive request for the same lookup. CivArchive should only be consulted when CivitAI legitimately has no answer (404 / "not found"), or when CivitAI is unreachable long-term. R4. No spurious item failures. A rate-limited request must not turn a batch-import item into FAILED; it should wait (bounded) and retry, or at worst be SKIPPED with a clear "rate limited" reason (re-runnable import). R5. Never hang forever. All waiting is bounded by a configurable cap; on expiry the caller receives the RateLimitError and can decide. R6. Keep legitimate failover. Deleted-model recovery via CivArchive/sqlite must keep working (404 paths unchanged). R7. Single choke point. The pacing gate should live where every API call passes (the Downloader), so bulk refresh, metadata sync, recipe analysis, and usage-control lookups all benefit without per-feature work.

4. Approach Comparison

A. Reactive gate — shared Retry-After deadman clock (recommended core)

A process-wide, per-destination coordinator records the next-allowed-send timestamp from each 429 (now + max(retry_after, backoff)). Every request through Downloader.make_request consults the gate before sending and when a 429 arrives; waiters block on a shared asyncio.Event that fires when the cooldown expires.

• Pros: single choke point (R7); herd-free (R2); honors server guidance (R1); no guessing at vendor limits; covers all providers automatically; reuses existing per-destination scoping.
• Cons: still experiences 429s before slowing down (reactive); long Retry-After windows (CivArchive has been observed at ~1500 s) need a sane wait cap + skip/retry UX.

B. Preemptive pacing — minimum inter-request interval (recommended companion)

Per-destination token bucket (simplest form: capacity 1 — at least N seconds between consecutive API requests; N configurable, default ~0.75 s ≈ 80 r/min ceiling).

• Pros: prevents most 429s before they happen — exactly the "intentional reduction in request rate" the issue asks for; trivial to implement on top of A's coordinator.
• Cons: adds latency to bulk operations (thousands of models × N); the exact vendor limits are unknown (CivitAI anonymous vs keyed vs civitai.red mirror differ), so the default must be conservative-but-not-crippling and settings-tunable.

C. Fallback semantics change — stop network→network failover on 429 (must-do, low risk)

FallbackMetadataProvider (and MetadataSyncService.fetch_and_update_model's manual loop) must treat a final RateLimitError as a terminal, non-failover result for network providers. Local-only providers (sqlite archive DB) may stay as a last resort (no vendor cost).

• Pros: directly removes the CivArchive flood; small, surgical change.
• Cons: none significant; requires care to keep 404-failover intact (R6).

Rejected / deferred

• Per-feature retry queues (batch import pauses & resumes whole batches): richer UX but much larger change (batch state machine, WebSocket states); unnecessary once A+B make requests wait at the choke point. Defer unless review finds the bounded-wait UX insufficient.
• Full token bucket with burst credit: overkill; capacity-1 interval is enough given the shared semaphore already caps concurrency at 5.
• Retrying in connectivity_guard: wrong layer — the guard is about transport reachability, not vendor quota.

5. Recommended Architecture

New singleton RateLimitCoordinator (py/services/rate_limit_coordinator.py, mirroring ConnectivityGuard's singleton + per-destination patterns):

state per destination (hostname):
  next_allowed_send: float (monotonic)   # from 429 Retry-After + backoff
  consecutive_429: int                   # for backoff growth
  last_send_at: float                    # for min-interval pacing
  waiters: list[Future] | asyncio.Event  # shared wake-up per cooldown cycle

API:

• async wait_for_slot(destination, request_started_within_window: bool) — called by Downloader.make_request before sending (blocks until min(now >= next_allowed_send) and inter-request interval elapses) and re-armable after a 429.
• register_rate_limit(destination, retry_after: float | None) — called on 429: next_allowed_send = max(now + retry_after_or_backoff, current); consecutive_429 += 1; backoff = retry_after honored, else exponential 30 · 2^(n-1) capped at 1800 s; creates/re-arms the shared wake-up event.
• register_success(destination) — resets consecutive_429 (called from the existing 200 path in make_request).
• remaining_seconds(destination), in_cooldown(destination) — for tests and diagnostics.

Enforcement points:

1. Downloader.make_request (downloader.py:1102-1132): ordering inside the method is connectivity-guard fail-fast first (offline short-circuit costs nothing to check), then await coordinator.wait_for_slot(destination) before session.request. On 429: coordinator.register_rate_limit(...), then wait for the gate and re-send (loop, bounded by rate_limit_max_wait_seconds, default 300; retry_after ≥ cap ⇒ fail immediately). After the loop, return the RateLimitError to the caller (unchanged contract) with exc.gate_handled = True set so downstream retry helpers know the wait already happened. 200 path calls register_success.
2. Downloader.download_to_memory / get_response_headers (phase 2): register 429s (so API calls queue); waiting only in make_request initially.
3. FallbackMetadataProvider (model_metadata_provider.py): remove network→network failover on RateLimitError — re-raise; only sqlite stays as a local last resort (implementation: per-method except RateLimitError handler that marks the chain rate-limited and stops iterating).
4. MetadataSyncService.fetch_and_update_model (metadata_sync_service.py:248-333): on RateLimitError from the default provider, stop appending further network providers (sqlite may remain); the existing any_rate_limited merge already produces "Rate limited".
5. Batch import (batch_import_service.py): no structural change needed — items now wait inside make_request; optionally (phase 2) map residual rate-limit failures (after the wait cap) to SKIPPED with "rate limited (retry_after=…s); re-run the import later" instead of FAILED, and surface a rate_limited flag in the WebSocket progress broadcast.
6. _RateLimitRetryHelper retries (model_metadata_provider.py): Phase 1 — when the raised RateLimitError carries gate_handled = True (set by the downloader after honoring the gate), the helper skips its own retry_after sleep and re-raises immediately, eliminating the double wait. The wiring stays so a RateLimitError still propagates cleanly; full demotion/removal can follow once the gate proves out.

Settings (settings.json, schema extension in SettingsManager):

key	default	meaning
rate_limit_gate_enabled	true	master switch for the coordinator
rate_limit_max_wait_seconds	300	how long make_request waits on a 429 gate before returning the error
rate_limit_min_interval_seconds	0.75	minimum seconds between API requests per destination (pacing, R6-friendly conservative default)

6. Changes by File

File	Change
py/services/rate_limit_coordinator.py (new)	coordinator singleton + per-destination state + tests seam
py/services/downloader.py	gate pre-check + 429 register/wait/retry loop + register_success; log the 429 notice at INFO once per cooldown, then DEBUG
py/services/model_metadata_provider.py	FallbackMetadataProvider: stop network failover on RateLimitError; helper skips its sleep when the error is marked gate_handled
py/services/metadata_sync_service.py	fetch_and_update_model: same failover semantics; keep sqlite last resort
py/services/batch_import_service.py	(phase 2) rate-limit failures → SKIPPED + rate_limited progress flag
py/services/settings_manager.py	new settings keys + defaults
tests/services/test_rate_limit_coordinator.py (new)	gate unit tests
tests/services/test_civitai_client.py / test_civarchive_client.py	provider-level 429 behavior
tests/services/test_metadata_service.py	failover-chain tests
tests/services/test_batch_import_service.py	SKIPPED-on-rate-limit

7. Impact, Risks, Open Questions

• Behavior change: with the gate in make_request, any request can block up to the wait cap — UI actions that call the API (e.g. a model-details fetch) may take longer during cooldowns. Mitigation: bounded cap + INFO log
  • the existing async request handling already tolerates slow responses. Decided (§10): interactive requests take the same bounded wait — one behavior, no call-source plumbing; cooldowns are usually short.
• Gate waits occupy batch slots: with the 1–5 batch semaphore, all slots can park on a gate simultaneously, freezing visible progress for up to one wait cap per wave. Bounded and acceptable; the phase-2 SKIPPED mapping + WebSocket rate_limited flag (both confirmed in scope, §10) make the stall visible and recoverable.
• Rate limit reality check: CivitAI anonymous vs keyed limits, and whether civitai.red differs, is unverified. Default pacing 0.75 s/req is a conservative guess (R6). Open question for maintainer: preferred default and whether an API-keyed ceiling should be higher.
• Long CivArchive windows: Retry-After ~1500 s observed in code comments. Decided (§10): keep the 300 s default cap — such lookups fail/skip rather than park a request path for 25 minutes; batch import maps them to SKIPPED (phase 2) so the user can re-run later.
• Double waiting: _RateLimitRetryHelper + gate could stack waits. Resolved in Phase 1: the downloader marks gate-honored errors with gate_handled = True and the helper skips its own sleep for those.
• Downloads: download_file 429s return an error to download managers unchanged (already handled); only registration is proposed, so future API calls queue behind a large Retry-After from a download burst.

8. Test Plan

1. Coordinator unit tests (new file):
   • 429 with retry_after → wait_for_slot blocks ~that long, then passes.
   • N concurrent waiters all wake together (herd test, wall-clock ≈ one window, not N windows).
   • Consecutive 429s grow backoff; register_success resets.
   • Missing Retry-After → default backoff path.
   • Wait cap: request fails after rate_limit_max_wait_seconds with RateLimitError.
2. Downloader tests (mock aiohttp session): 429 then 200 → make_request returns success after gate delay; two back-to-back calls to the same destination are spaced ≥ min_interval; different destinations are not spaced.
3. Provider tests: FallbackMetadataProvider.get_model_version_info — Civitai raises RateLimitError → CivArchive mock not called; 404 still falls through to CivArchive; sqlite still tried after network 429.
4. Sync-service test: fetch_and_update_model with a rate-limited default provider → result error contains "Rate limited" and sqlite attempt state unchanged.
5. Batch-import test: analysis provider 429s first, then succeeds → item ends SUCCESS (wait path), and post-cap 429 → SKIPPED with rate-limit reason (phase 2).
6. Full regression: pytest tests/services tests/routes tests/standalone (currently 1582 passing).

9. Implementation Phases

• Phase 1 (this plan, reviewed): RateLimitCoordinator + Downloader.make_request integration (guard fail-fast → gate pre-check pacing → 429 register/wait/retry loop with cap → gate_handled marking) + settings + Fix C failover semantics (FallbackMetadataProvider, fetch_and_update_model — moved up from phase 2: smallest diff, kills the CivArchive flood immediately, independent of coordinator correctness) + _RateLimitRetryHelper double-wait fix + coordinator/downloader/provider/ sync tests.
• Phase 2: batch-import SKIPPED-on-rate-limit + rate_limited WebSocket progress flag + slowdown hint (confirmed, §10), download_to_memory/HEAD 429 registration, batch tests.
• Phase 3: full regression + docs + commit referencing (#1085).

10. Review Checklist — Decisions (2026-08-27)

• Default pacing interval 0.75 s — accepted as conservative default; tunable via rate_limit_min_interval_seconds. Revisit if CivitAI publishes keyed/anonymous ceilings.
• Wait cap 300 s — accepted; long-window CivArchive lookups fail → batch import marks them SKIPPED with a rate-limit reason (phase 2).
• Interactive API calls also wait (bounded) — yes, same behavior for all callers.
• Keep sqlite as last resort behind a network rate limit — yes (local-only, no vendor cost).
• UI hint — yes: WebSocket rate_limited flag + "rate limited — slowing down" hint in batch-import progress (phase 2); INFO logging regardless.