You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This trap has now been hit three times in one week. Each time it was found by accident, and each time the symptom was a ceiling that looked global and was not.
harvest_fetch_limiter: self._semaphore = threading.Semaphore(config.max_concurrency) — per process. django-q2 workers are separate OS processes, so N dispatches each got a full-strength semaphore.
PR Autoscale the Stage E micro-batch size from measured hardware #589: stage_e_batch_sizing used GOOGLE_IMAGE.max_concurrency as a global fetch ceiling term in its sizing arithmetic. It was false, and the live consequence was a memory bug — the divisor under-counted resident processes and handed each dispatch ~2x its ratified RSS allowance.
PR Make the 7/s Google fetch cap global, not per process #648: _DestinationLimiter paced with a threading.Lock and a process-local _next_allowed, so rate_per_sec = 7.0 bought 7/s per process — 14/s at the production dispatch cap, scaling with it.
There is prior art in the repo too: run_image_evidence_cohort's docstring records a retired "descaling hack" that divided the limiter by N precisely because "N independent compute processes would otherwise each construct their own full-strength limiter (aggregate ceiling N times too high)".
Why it keeps happening
A module-level Python object is per-process by construction, and nothing in the code says so. threading.Semaphore, threading.Lock, a module global, an lru_cache, a class attribute — all of them read as program-wide and are process-wide. The distinction only bites where the deployment is multi-process, which is exactly where it is least visible in a unit test: a single-instance test cannot detect it. PR #648's proof required constructing four independent limiters on four threads; using get_limiter() would have returned one shared instance and silently made it a single-process test that passes.
What would close this
Some combination, to be decided:
A lint flagging threading.Semaphore / threading.Lock / module-level mutable state used to express a destination-facing or host-facing ceiling, requiring either an explicit "per-process, deliberately" comment or a cross-process mechanism. Heuristic; would need an allowlist. Evaluate whether the false-positive rate makes it worth it, and say so either way.
A checklist entry in the contributor-facing docs: any limit protecting a shared resource (a remote destination, the host box, the database) must state whether it is per-process or global, and a test asserting the aggregate must construct more than one instance.
A convention: name per-process limits so they cannot be misread (_local_*), the way PR Make the 7/s Google fetch cap global, not per process #648 kept max_concurrency per-process deliberately as a local resource bound while moving the rate to a shared row.
Related: #627 (the #589 instance), #648 (the global rate budget), #647 (containerising the monolith — note a per-container limiter would be this same defect one layer out).
This trap has now been hit three times in one week. Each time it was found by accident, and each time the symptom was a ceiling that looked global and was not.
harvest_fetch_limiter:self._semaphore = threading.Semaphore(config.max_concurrency)— per process. django-q2 workers are separate OS processes, so N dispatches each got a full-strength semaphore.stage_e_batch_sizingusedGOOGLE_IMAGE.max_concurrencyas a global fetch ceiling term in its sizing arithmetic. It was false, and the live consequence was a memory bug — the divisor under-counted resident processes and handed each dispatch ~2x its ratified RSS allowance._DestinationLimiterpaced with athreading.Lockand a process-local_next_allowed, sorate_per_sec = 7.0bought 7/s per process — 14/s at the production dispatch cap, scaling with it.There is prior art in the repo too:
run_image_evidence_cohort's docstring records a retired "descaling hack" that divided the limiter by N precisely because "N independent compute processes would otherwise each construct their own full-strength limiter (aggregate ceiling N times too high)".Why it keeps happening
A module-level Python object is per-process by construction, and nothing in the code says so.
threading.Semaphore,threading.Lock, a module global, anlru_cache, a class attribute — all of them read as program-wide and are process-wide. The distinction only bites where the deployment is multi-process, which is exactly where it is least visible in a unit test: a single-instance test cannot detect it. PR #648's proof required constructing four independent limiters on four threads; usingget_limiter()would have returned one shared instance and silently made it a single-process test that passes.What would close this
Some combination, to be decided:
threading.Semaphore/threading.Lock/ module-level mutable state used to express a destination-facing or host-facing ceiling, requiring either an explicit "per-process, deliberately" comment or a cross-process mechanism. Heuristic; would need an allowlist. Evaluate whether the false-positive rate makes it worth it, and say so either way._local_*), the way PR Make the 7/s Google fetch cap global, not per process #648 keptmax_concurrencyper-process deliberately as a local resource bound while moving the rate to a shared row.Related: #627 (the #589 instance), #648 (the global rate budget), #647 (containerising the monolith — note a per-container limiter would be this same defect one layer out).