Make the 7/s Google fetch cap global, not per process - #648
Make the 7/s Google fetch cap global, not per process#648WilfordGrimley wants to merge 1 commit into
Conversation
Owner clarification, 2026-07-30: "to be clear: the 7 fetches per second cap is a global cap, it shouldn't be per process or per core." THE GAP. `_DestinationLimiter` paces with a `threading.Lock` and a process-local `_next_allowed`, so PR #644's `rate_per_sec = 7.0` bought 7/s PER PROCESS. The pooled runner (`run_image_evidence_cohort`) is single-process and was therefore correct by accident. The conveyor is not: django-q2 workers are separate OS processes, each building its own limiter in its own address space, so N concurrent dispatches issued N x 7/s -- 14/s at the production STAGE_E_MAX_CONCURRENT_DISPATCHES = 2, scaling with the cap. Same per-process trap as threading.Semaphore(max_concurrency), which #589 removed a false ceiling term over. #644 established that the rate limit is what protects the destination; that makes its globality load-bearing. MECHANISM. One Postgres row per destination (GlobalFetchPace, migration 0102), advanced by a single atomic UPDATE ... RETURNING that lifts the existing Python pacing arithmetic into the one place every process can see. The caller then sleeps to its slot locally, holding no lock and no transaction. Concurrent reservers serialise on the row lock and, under READ COMMITTED, the blocked statement re-evaluates against the committed row -- so reservations form one strictly-increasing sequence regardless of how many processes compete. WHY NOT stage_e_concurrency's ADVISORY LOCKS (evaluated first, as asked). A lock is a binary held/not-held token: it expresses "how many at once", not "how many per unit time", because a rate needs a remembered timestamp and a lock stores no value. And that module's crash-safety objection to a DB row -- a killed worker leaves a claimed slot claimed forever -- is about a CLAIM. This row holds only a timestamp: a killed process leaves next_allowed_at at most one interval ahead, self-healing in ~143ms with zero reconciliation. The property that made a row unsafe for a slot makes it correct for a pace. No Redis exists in docker-compose.prod.yml, so Postgres is the only shared state. Timestamps come from the database's clock_timestamp(), never time.monotonic(), whose epoch is per-process -- persisting one for another process to read would reintroduce the defect while looking coordinated. The backoff multiplier and clean streak move into the same row because a global rate ceiling requires a global backoff term: processes holding different multipliers would write conflicting paces into the same row. #644's 429/503 semantics and decay schedule are unchanged; only their storage moved. max_concurrency stays per-process on purpose -- it is a local resource bound, not the destination-protecting ceiling. COST: 1.5ms mean / 2.3ms p95 per reservation over 300 calls -- 0.78% of a ~300ms image fetch, ~1% of the 143ms interval it schedules. The pooled runner is not measurably slowed. If the row is unreachable, pacing degrades to per-process and the run continues, logged at ERROR -- deliberately the opposite of stage_e_concurrency's fail-closed, since an unavailable rate budget still leaves every process individually paced. Default-on, no flag. Wiki-facing docs/features/stage-e-operations.md states the per-process-vs-global distinction explicitly, with the mechanism, the rejected alternatives and the measured cost. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013NhYmT1PxCcyemA16dFDxN
54260f6 to
bbe8e51
Compare
|
Closing in favour of #649, which fixes the same defect. Not a quality judgement — I dispatched two agents onto the same task by mistake and both produced sound solutions. #649 wins on three points:
What this PR had that #649 does not, and which is not being lost: the global backoff multiplier and shared clean-streak. The argument is correct — a shared Also preserved there: the observation that Branch left in place; nothing force-pushed or deleted. |
Follow-up to #644 (merged). Owner clarification, 2026-07-30:
The gap
_DestinationLimiterpaces with athreading.Lockand a process-localnext_allowed. #644'srate_per_sec = 7.0therefore bought 7/s per process:run_image_evidence_cohort)stage_e_dispatchvia django-q2)STAGE_E_MAX_CONCURRENT_DISPATCHES = 2, scaling with the capSame per-process trap as
threading.Semaphore(max_concurrency)— the one #589 deleted a false ceiling term over. #644 established that the rate limit, not the concurrency limit, is what protects the destination; that is exactly what makes its globality load-bearing rather than a refinement.Mechanism
One Postgres row per destination (
GlobalFetchPace, migration0102), advanced by a single atomic statement:That is the same arithmetic
acquire()already did in Python, lifted into the one place every process can see. The caller then sleeps to its slot locally, holding no lock and no transaction. Concurrent reservers serialise on Postgres's row lock, and under READ COMMITTED the blocked statement re-evaluates against the committed row — so reservations form one strictly-increasing sequence no matter how many processes compete. Nothing here is advisory or best-effort.Three properties worth knowing:
clock_timestamp()), nevertime.monotonic(), whose epoch is per-process. Persisting one process's monotonic reading for another to read would reintroduce the exact defect while looking coordinated.max_concurrencystays per-process, deliberately — a local resource bound, not the destination-protecting ceiling.Why a row rather than
stage_e_concurrency's advisory locksEvaluated first, as asked, since that module is this repo's established cross-process primitive. It cannot carry a rate, and both halves of its own reasoning invert here:
kill -9'd worker leaves a row-based slot claimed forever. That objection is about a claim. This row holds no claim — only a timestamp. A process killed mid-reservation leavesnext_allowed_atat most one interval ahead, self-healing in ~143 ms at 7/s with zero reconciliation code. The very property that made a row unsafe for a slot makes it correct for a pace.Redis was not an option:
docker/docker-compose.prod.ymlruns django, worker, nginx, postgres and elasticsearch, and no Redis (Django's cache here is the default per-processLocMemCache). Postgres is the one shared, process-visible state this pipeline guarantees.Requirement 2 — the pooled runner is not regressed (measured, not assumed)
Reservation cost over 300 steady-state calls against the containerised Postgres the suite uses:
That is 0.78 % of a ~300 ms image fetch at p95, and ~1 % of the 143 ms pacing interval the reservation itself schedules. The pooled runner's fetch threads touch the row only 7 times a second in total, so the row lock runs at roughly a 1 % duty cycle — no contention. No trade needs proposing; the cost is not material.
Connections are a dedicated, thread-cached
psycopg2connection, notdjango.db.connection. The first reason is a correctness bug, not a preference: a reservation made inside a caller'satomic()block would hold the row lock until that whole transaction committed, serialising every other fetcher in the deployment behind it.autocommit=Truereleases it the instant the statement returns. The second is cost — one connect per fetch thread for the life of the process, not one per request.Requirement 4 — degradation
If the row is unreachable,
RateBudgetUnavailableis raised, the limiter falls back to its own per-process pacer, and the run continues — logged at ERROR. The ceiling reverts to where this repo already was; it never fails or halts. Deliberately the opposite ofstage_e_concurrency's fail-closed, because the downsides differ: an unavailable concurrency cap lets unbounded dispatches pile onto the host, while an unavailable rate budget still leaves every process individually paced at 7/s.Verification
The key test runs four independent limiters concurrently — a single-instance test cannot detect this defect, which is precisely what let it through.
test_four_independent_limiters_share_one_budgetdrives 4 limiters × 10 requests at a configured 50/s and asserts the aggregate.Mutation-checked (red → green). Reverting
acquire()to per-process pacing (wait_time = self._local_wait()) fails it with the defect reproduced exactly:0.181s against a required 0.780s — a 4× overshoot at ~221/s against a 50/s cap. Mutation reverted; green.
Also covered: the reservation sequence asserted directly (8 concurrent reservers each get a distinct, correctly-spaced slot); single-process pacing unregressed; degradation still paces and never raises; a truncated table recreates its row rather than wedging; shared backoff visible across reservers, capped, applied to the shared pace; shared streak so decay is not N× faster; decay floored at the configured ceiling.
cardpicker/tests/suite: 3436 passed, 11 skipped, 0 failed.pre-commit run --all-filesgreen.docs_lint.py --strictclean.check_migration_leaves.py --base origin/master: one leaf per app (102 migrations).0102is a clean single leaf off0101; no open PR claims a migration.Docs
Per the owner documentation rule, this edits the wiki-facing
docs/features/stage-e-operations.md— no dated report. A new subsection under "Rate pressure is throttled, not halted" states the per-process vs global distinction explicitly (with the per-fetcher table above), the mechanism, the three properties, the rejected alternatives, the measured cost, and the degradation behaviour.Preserved from #644
The throttle-not-halt conversion, the 429/503 classification, the decay schedule and the bar classification are all untouched. This is additive.
🤖 Generated with Claude Code
https://claude.ai/code/session_013NhYmT1PxCcyemA16dFDxN