Make the 7/s Google fetch cap global, without a migration - #649
Merged
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". `harvest_fetch_limiter._DestinationLimiter` kept its pacing state in one process's memory, so PR #644's `rate_per_sec = 7.0` bought 7/s PER PROCESS. That is a real ceiling for the pooled runner (one process, one thread pool) and no ceiling at all for the conveyor: django-q2's workers are separate OS processes, so N concurrent dispatches fetched at N x 7/s - 14/s at the shipped STAGE_E_MAX_CONCURRENT_DISPATCHES = 2, rising with that cap. New `cardpicker/harvest_rate_coordinator.py` moves the pacer's own `max(now, next_allowed) + interval` arithmetic into a single atomic `INSERT ... ON CONFLICT DO UPDATE` over one cursor row every fetching process shares. One round trip per fetch, Postgres's own clock, no advisory lock, no explicit transaction. No new migration: the cursor lives in the existing `shared_cache` table (0092), under a key Django's own `make_key` cannot produce. The migration graph is contended and a one-row table is not worth a leaf. Losing the row is benign - the next reservation re-inserts at "now". Degradation is divided, not open and not closed: if Postgres is unreachable, each process falls back to its own pacer at rate / (STAGE_E_MAX_CONCURRENT_DISPATCHES + 1), so the aggregate ceiling still holds with every process fetching. Failing open would restore the exact defect; failing closed would halt an unattended 230,753-card pass over a blip. A degraded reservation waits, never raises, never trips. Purely additive to #644: the throttle-not-halt conversion, the 429/503 classification, the backoff decay and the envelope bar classification are untouched. Backoff stays per-process and reaches the shared cursor as a widened interval, which can only slow the aggregate. Proven with real forked OS processes, not one limiter: three processes at a 20/s ceiling deliver 20/s together. Against the pre-fix pacer the same test measures 63/s. Coordination costs 1.1-1.3 ms per reservation (0.8-0.9% of the 143 ms interval a 7/s ceiling already imposes); eight acquisitions at the real 7.0/s ceiling take 1.003s against 1.000s for the ceiling alone, +0.3%. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013NhYmT1PxCcyemA16dFDxN
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #644 (merged). Owner clarification, 2026-07-30:
The defect
_DestinationLimiterkeeps_next_allowed, its lock and its semaphore in one Python process's memory.run_image_evidence_cohort)stage_e_dispatchvia django-q2)STAGE_E_MAX_CONCURRENT_DISPATCHES = 2, and rising with that cap#644 established that the rate limit, not the concurrency limit, is what protects the destination. That is exactly what makes the rate limit's globality load-bearing — and it was missing.
Mechanism — the pacer's own arithmetic, moved somewhere shared
The in-memory pacer already computes
next_allowed = max(now, next_allowed) + interval. That is a cursor update. It becomes global by running it in Postgres, in one atomic statement:ON CONFLICT DO UPDATEtakes the row lock for the statement's duration, so the read-modify-write is atomic against every process and thread with no advisory lock, no explicit transaction, noSELECT ... FOR UPDATEpair — one round trip per fetch. The statement returns the caller's own wait in seconds; the caller sleeps locally, holding nothing. Time is Postgres'sclock_timestamp()throughout, never a per-processmonotonic()epoch.Why not
stage_e_concurrency's advisory locksRead first, as asked. An advisory lock is a pure mutual-exclusion primitive: no payload, no time dimension. It can say "how many at once"; a rate needs a remembered value plus a clock. The two shapes buildable from locks alone were both rejected, and both rejections are in the module docstring:
ON CONFLICTalready provides the exclusion.No Redis:
docker/docker-compose.prod.ymlruns django, worker, nginx, postgres, elasticsearch. Checked, as asked.No new migration
The cursor is one number, and the migration graph is actively contended (#611's guard fails a graph that forks as merged with the base; #648 adds a
0102). It therefore lives in the existingshared_cachetable (0092, already created by themigratethatdocker/django/entrypoint.shruns, in prod and CI and every dev database), as a raw numeric string under a key prefix Django's ownmake_keycan never emit (every Django cache key there starts":<version>:"). The two ways Django could remove the row —cache.clear()and_cullaboveMAX_ENTRIES = 1000— are both benign: a missing row is re-inserted at "now", costing at most one immediately-allowed fetch, never a burst.Requirement 1 — proven with more than one limiter, and with real processes
A single-instance test cannot see this defect, and that is precisely how it shipped. So:
test_three_processes_share_one_ceilingforks three real OS processes — the shape django-q2 actually uses — each with its own limiter at a 20/s ceiling, and asserts the merged 30-acquisition timeline. Both an upper bound (aggregate ≤ ceiling, plus a sliding-1s-window peak check) and a lower bound on elapsed span, because a per-process pacer fails the second by a factor of the process count.test_four_independent_limiters_share_one_ceiling— same discriminator, four independent limiter objects in threads, fast enough for every change.Non-vacuity, measured: with
acquire()reverted to the pre-fix per-process pacer, those two tests report 63.47/s and 110.49/s against their ceilings of 20 and 25. They fail loudly at exactly the ratio the defect predicts.Requirement 2 — the pooled runner is not regressed (measured)
GOOGLE_IMAGE.rate_per_sec = 7.0Both are asserted, not just printed (
TestCoordinationCost), so this stays true. No trade needs proposing — the round trip is amortised into a pacing interval that already exists and is two orders of magnitude larger. One dedicated autocommit connection per process (not per thread), so the extra Postgres connection count is 1 per fetching process; the lock serialising threads onto it clears ~925 reservations/s, against a 7/s ceiling.django.db.connectionis deliberately not used: a reservation inside a caller'satomic()block would hold the cursor row lock until that transaction committed, stalling every other fetching process — a global rate ceiling that intermittently becomes a global stop.Requirement 3 — purely additive
Nothing #644 established is touched: the throttle-not-halt conversion, the 429/503 rate-pressure classification, the backoff decay schedule, and the envelope bar classification (HOST_LOAD / RSS / GOOGLE_LOCKOUT still halt) are all unchanged. Backoff stays per-process and reaches the shared cursor as an already-widened interval, which can only ever slow the aggregate.
Requirement 4 & 6 — degrade: neither open nor closed
If Postgres is unreachable for a reservation,
reserve()returnsNone(never raises) and the caller falls back to its own pacer atrate ÷ (STAGE_E_MAX_CONCURRENT_DISPATCHES + 1)— the conveyor's own cross-process dispatch cap, plus one for a pooled/manual runner that holds no dispatch slot. Worst case, every process fetching: N × rate/N = rate. The ceiling survives the outage.A degraded reservation waits. It does not fail, does not halt, does not feed the envelope's fetch-failure window. Failures are latched for 5 s so an outage costs one connection attempt per cooldown rather than one per fetch (asserted).
Requirement 5 — default-on, no flag
The divisor is derived from
STAGE_E_MAX_CONCURRENT_DISPATCHES, not a new setting. A second knob for the same fact is just a way for the two to disagree.Fork safety
os.register_at_fork(after_in_child=...)abandons the inherited connection object rather than closing it — django-q2'sClusterforks its workers, andPQfinishon an inherited handle sends Terminate down a socket the parent is still using. (This is not theoretical: the multi-process test hit exactly that failure viaconnections.close_all()in the child and killed pytest's own session.)Docs
docs/features/stage-e-operations.md(wikiStage-E-Operations), inside #644's own "Rate pressure is throttled, not halted" section. The per-process-versus-global distinction is stated flatly — "not 7/sec for each django-q2 worker, not per dispatch, not per core" — because two readers have now got it wrong, plus what an operator should do instead (add up every fetching process; concurrency knobs now change only how the budget is shared, never how much of it there is) and the degradation behaviour. No dated report.Differences from #648
shared_cache(0092)0102_global_fetch_paceto a contended graph#648's global-backoff argument is real and this PR does not have it; this PR's no-migration and non-fail-open properties are the trade in the other direction.
🤖 Generated with Claude Code
https://claude.ai/code/session_013NhYmT1PxCcyemA16dFDxN