Celery isolation and a global bound on concurrent generations #21

Activity

1. Silthus

   added

   wayfinder:grilling

   on Aug 7, 2026
2. Silthus

   added a parent issue on Aug 7, 2026

   • LLM action for workflows #1
3. Silthus

   marked this as blocking on Aug 7, 2026

   • The spec artifact: what form it takes and where it lands #18
4. Silthus

   self-assigned this

   on Aug 7, 2026
5. Silthus commented on Aug 7, 2026

   

   Silthus

   on Aug 7, 2026

   OwnerAuthor

   More actions

   Resolution: the bound is two decorators on the task. No dedicated queue, no dedicated pool, no new status code.

   The ticket asks three questions that look like one. They separate cleanly once you notice that the queue decides who gets hurt and the semaphore decides how much, and only the second is the thing this map needs to own.

   PostHog already ships the exact control this ticket asks for, on the task that is closest in shape to ours. process_query_task holds a Celery worker for up to ten minutes on a user-facing async query, and it is bounded by a Redis ZSET semaphore declared inline on the task, global first and per-team second:

   @limit_concurrency(150, limit_name="global")   # posthog/tasks/tasks.py:393
   @limit_concurrency(10, key=lambda …: team_id, limit_name="per_team")  # :394-398

   The generation task takes the same stanza with our numbers. That is the whole answer to (b), it lives in the PR, and it costs one import and two decorator lines.

   ---

   (b) What caps total concurrent generations

   @limit_concurrency(settings.WORKFLOW_LLM_MAX_CONCURRENT_GENERATIONS, ttl=120, limit_name="workflow_llm_global") plus the same decorator keyed on team_id. Defaults 30 global, 5 per team, both settings.

   The primitive is posthog/clickhouse/client/limit.py:409-448: an atomic Lua check-and-add over a Redis sorted set (:43-65), acquired at task start and released in a finally (:439-444), raising ConcurrencyLimitExceeded (:405-406) when full.

   Why the numbers. 30 concurrent generations is 30 blocked prefork children at the 60 s worst case. At a realistic 10 s generation it is 3 generations a second fleet-wide, about 260k a day, far above anything a flag-gated pilot reaches. Per team 5 is about 0.5 a second, roughly 43k a day for one project. The ratio matters more than either absolute: at 6:1 no single project can lock the fleet, and six projects can run flat out before anybody waits. Both are get_from_env settings so the fleet number can move without a deploy, following CLICKHOUSE_LLM_ANALYTICS_MAX_CONCURRENT_QUERIES (posthog/settings/data_stores.py:423-427), whose comment is also the model for ours: name what the bound is relative to, because a bound nobody can compare to the resource it protects is decoration.

   ttl=120, not the decorator's default of 900 (limit.py:410). It has to exceed the task's hard time limit so a worker dying mid-generation cannot release a slot the generation still occupies. That is the reasoning already written at limit.py:394-396 for the LLM analytics limiter. 120 against a 90 s hard limit gives that, and keeps a leaked slot short.

   Use the decorator, not RateLimit. They are different primitives and only one of them is safe here. RateLimit.use() honors the DRF throttle allow-list (limit.py:150-166, backed by team_is_allowed_to_bypass_throttle, posthog/rate_limit.py:206) and returns (None, None), meaning admitted with no slot; it also halves or quarters max_concurrency under the ClickHouse kill switch (limit.py:130-137). One allow-listed team would void the bound, and a ClickHouse incident would cut LLM concurrency for reasons unrelated to LLM capacity. The decorator at :409-448 has neither behavior. For a bound whose job is protecting a shared upstream, no-bypass is the requirement.

   (a) Which queue, and whether it needs a pool

   CeleryQueue.LONG_RUNNING (posthog/celery_queues.py:35), whose declared purpose is verbatim ours: "any task that has a good chance of taking more than a few seconds should go here". No new queue, no new pool.

   The instinct was a dedicated workflow_llm queue, and the repo's own most recent precedent for our exact shape supports it: sync_cross_region_flags_task took its own queue with the stated reason that "it makes a blocking cross-region HTTP call: a stalled upstream shouldn't be able to delay the fast, sub-second signal-driven cache rebuilds sharing that queue" (products/feature_flags/backend/tasks.py:75-83). ANALYTICS_LIMITED exists for the adjacent reason, so the pool can be deliberately small (posthog/caching/warming.py:212).

   It loses anyway, on the same argument that killed the CDP pod in #20.

   1. The failure mode is silent and cross-repo. bin/docker-worker-celery:95-105 sources bin/celery-queues.env only when CELERY_WORKER_QUEUES is unset. Production sets it per consumer from the charts repo (posthog/celery_queues.py:6 points at prod-us.yaml.gotmpl). So a queue added to bin/celery-queues.env:5 and not to a charts consumer's queues: list is consumed in dev and CI and by nothing in production. Tasks land in a Redis list nobody reads. No exception, no 5xx, no Celery failure metric, because the task never runs. Every generation times out at the node's give-up and the only symptom is "the LLM node never works". Dev and CI stay green throughout. Rollout ordering for the llm queue #14 was willing to accept a partial rollout because the CDP version of this was "lossless, self-healing, and invisible"; this version is lossy and visible only as a product outage.
   2. The cap makes the pool question much smaller. With the semaphore, at most 30 generation tasks exist in flight fleet-wide. That is a bounded, known bite out of a shared pool, not the unbounded 10N the ticket describes. -Ofair (bin/docker-worker-celery:79) means a worker child only takes a task when it is free, so a queued generation goes to whichever child frees up first rather than sitting behind a prefetched backlog.
   3. The ticket's stated failure mode is fully closed without it. "Celery pool starvation, blast radius every unrelated Celery task on the same pool" is a statement about occupancy. Occupancy is now capped at 30. What a dedicated queue would additionally buy is latency isolation, which is a different and unmeasured concern.

   The honest cost of long_running, recorded rather than waved away. It hosts cohort recalculation (posthog/tasks/calculate_cohort.py:459, 577, 654, 729, 907), including a 4-hour soft limit (:729-730). -Ofair prevents prefetch but does nothing when every child is already inside a minutes-long recalc, and our budget is a 90 s give-up. A head-of-line stall is a user-visible node failure we cannot tune from this repo.

   So it graduates on a measurement, following what #19 did for fairness. Stamp enqueued_at in the task payload and record now - enqueued_at as a histogram at task start. If queue wait p99 exceeds a few seconds during the pilot, split to a dedicated queue: two lines here (posthog/celery_queues.py, bin/celery-queues.env:5) plus one entry in an existing consumer's queues: list. That is a follow-up PR, not a pod, and the charts entry must land before the flag widens, precisely because of point 1.

   No dedicated worker pool either way. Pool size lives in the charts repo, which is the same reason #20 refused to rest a bound on replicas x batch size. A pool is a scaling decision; the cap is a correctness decision, and this one is in the PR.

   (c) The per-project submit throttle

   Stays exactly as #4 specified: 60/minute per project on submit, retrieve unthrottled. Not changed, not removed, and its recorded reason is updated.

   #4 justified it on worker occupancy: "every submit can hold a Celery worker for up to 60 seconds". The per-team semaphore now owns that, so the throttle's reason has to be restated or a future reader deletes it as redundant. Its remaining job is real and nothing else does it: it bounds enqueue rate, which the in-task cap deliberately does not. The semaphore caps how many generations run; without the throttle nothing caps how many tasks pile into the broker.

   Two verified reasons not to lean on the throttle for the concurrency bound, which is why both controls exist:

   • It is bypassable. _allow_request_internal returns early when the instance-wide kill switch is off (posthog/rate_limit.py:186-187) and again for allow-listed teams (:206). A bound that ops can switch off per team is not a fleet protection.
   • A rate is not a concurrency. 60/min at a 60 s generation is 60 concurrent for one project, twice the whole fleet cap. The rate x duration identity the ticket opens with cuts both ways; only an explicit per-team concurrency cap closes it, which is why process_query_task carries both a rate-free global cap and a per-team one.

   Confirming #4's mechanism works for this caller: _TeamBucketRateThrottle (posthog/rate_limit.py:506) extends PersonalApiKeyOrUserRateThrottle, whose allow_request passes personal_api_key_only=False (:425-426), so a TeamSecretTokenUser request is throttled rather than waved through.

   (d) What the user and the workflow see

   Nothing new. This is the strongest evidence the design is the right size: saturation needs no new status code, no new node behavior, and no new copy beyond one error string.

   • While waiting for a slot the task retries in the broker. Retrieve keeps returning 202 pending, indistinguishable from a slow generation, which is what the user should see because it is capacity and not their mistake.
   • On success, 200 with the envelope. Unchanged.
   • When retries are exhausted, the task's on_failure hook writes a terminal record and retrieve returns 200 with status: "failed" and error.code: "capacity_unavailable", obeying The endpoint contract #4's rule that a failed generation is a 200 so the caller does not read a retry instruction into it. process_query_task already does exactly this with on_failure=_process_query_task_failure (posthog/tasks/tasks.py:385), which writes the user-visible error onto the status record. The node then fails whole and inherits on_error, default abort (How a slow generation fits the CDP execution model #11).
   • If the node's 90 s give-up fires first, it is deadline_exceeded, as already specified.
   • Poll by reschedule versus a dedicated llm queue #20's 429-is-a-reschedule rule is untouched. The only 429 on this endpoint remains the per-project throttle's, and it still means reschedule with queueScheduledAt. Saturation does not produce a 429, so no new interaction to design.
   • Preview (What preview runs, and whether a poll loop fits it #13) needs no special case. It submits through the same endpoint and takes a slot like any other generation. A saturated fleet surfaces as the preview-expiry message the 25 s test deadline already produces. No preview-specific spend or capacity guard, as What preview runs, and whether a poll loop fits it #13 decided.

   capacity_unavailable joins #4's error-code list and is user-facing copy in effect, so it goes through /writing-user-facing-copy with the rest. It should say what happened and what happens next, not name a queue.

   Retry budget has to fit inside the node's give-up. 90 s give-up minus a 60 s generation leaves about 30 s for admission retries, so retry_backoff=2, retry_backoff_max=8, max_retries=4 (about 22 s of backoff) rather than process_query_task's ten retries. Set expires to the give-up so a task that has not started by then is discarded instead of running and billing a generation nobody collects. One implementation check: confirm expires survives an autoretry_for republish rather than resetting per attempt.

   Does #11's Celery choice still hold now that the transport changed

   Yes, and the reason is sharper than it was. Temporal does not answer this ticket.

   • Its concurrency knobs are per worker process: max_concurrent_activities and max_concurrent_workflow_tasks both default to 50 per worker (posthog/temporal/common/worker.py:411-413), and the repo says so in as many words for the messaging analogue, "this is per worker process, so the aggregate is this value times the number of messaging worker replicas" (posthog/settings/temporal.py:186-187). So a Temporal version would still hand-roll a global cap, which is exactly what the nearest Temporal prior art does: replay_vision claims an admission slot before start_workflow (products/replay_vision/backend/api/trigger.py:95-123).
   • The one Temporal-native global control, max_task_queue_activities_per_second, is enforced server-side across a whole task queue and is passed nowhere in this repo. It is a rate limit rather than a concurrency limit, and on the shared general-purpose-task-queue it would throttle every other product's activities. Using it needs a dedicated task queue, which needs a charts fleet plus a container-images-cd.yml trigger, and the README is explicit that the missing trigger crash-loops the fleet (posthog/temporal/README.md:442). That is the cost we are avoiding.
   • What Temporal genuinely buys is durable retry, timers, cancellation and exactly-once. The extraction pass contract #6 and The endpoint contract #4 ruled out transport retry, made gateway_unavailable terminal, and got idempotency from the dedupe key plus the cached result. Nothing is left to pay for, and workflow versioning becomes an ongoing obligation (.claude/rules/temporal-workflow-versioning.md).

   The handbook does point the other way and should be answered rather than ignored: "New medium- and large-scale jobs should use Temporal or Dagster instead", and "Temporal for user-facing workflows" (docs/published/handbook/engineering/stack.md:37, 43). This lands on the Celery side of its own decision list (:45-48): one call, no orchestration, no exactly-once requirement, no complex retry policy, bounded at 60 s.

   The one real Temporal advantage, recorded as the revisit trigger. An activity slot is a thread in a pool, so 30 concurrent generations cost about one Temporal worker process where they cost 30 Celery prefork children. If this feature ever leaves pilot volumes, that ratio is the reason to move it, not durability. Worth revisiting when the global cap needs to go past roughly 100.

   ---

   Alternatives rejected

   Admission control at the submit endpoint (acquire a slot in the Django request handler, release it in the Celery task, 429 with Retry-After when full, node reschedules). This was the draft, and it is worse in five ways. It needs the wrong primitive, since only RateLimit has a splittable use()/release() and it carries the allow-list and kill-switch behavior above. It creates a leak class that in-task acquisition does not have: a slot held with no task after a broker failure, a slot held by a dedupe short-circuit that enqueues nothing, a slot that expires while its task is still queued. It makes admission a lottery with no aging, where a job that has waited four minutes has the same odds as one that just arrived, and a shared Retry-After synchronizes contenders into a herd. It costs reschedule writes and run-log lines on the cyclotron job table for work that has not started. And it needs a new 429 meaning, a jittered backoff, a separate admission budget, and a queueMetadata counter, all of which the in-task cap gets for free from Celery's own retry. Its only advantage is bounding broker depth, which the per-project throttle already does.

   A dedicated workflow_llm Celery queue in v1. Covered above. Cheap in lines, but it makes the feature depend on a change in another repo whose absence is a silent product outage. Deferred behind a measurement.

   Gating at dequeue in the Node runtime, using CyclotronJobQueueRateLimitedPostgresV2 and the per-team fair dequeue that already serve email. This is the mechanism with the best properties: no acquire/release split, no lottery, fairness already built, fail-closed on startup. It is also the thing #20 dropped and #19 declined, because it needs the dedicated llm cyclotron queue kind, a partial index and a consumer capability. It is the right shape for park-and-wake, which the map already lists as the endgame transport. Not v1, and no new fog: both are already recorded.

   Dropping the per-project throttle now that a per-team cap exists. Rejected: TeamSecretTokenUser gets no default DRF throttling on this path, so submit would have no request-rate bound at all, and #4 already litigated the number.

   Making the queue a setting so a dedicated queue can be flipped by env rather than a deploy. Rejected as a knob with one user. The follow-up PR is two lines.

   Implementation notes

   • The task belongs in products/workflows/backend/tasks/, which already exists (hog_flows.py). Its module must be imported in that package's __init__.py, which currently imports only hog_flows; autodiscovery imports the package, not its submodules. Nothing catches a missed import: the registration snapshot filters names starting with posthog. (posthog/tasks/test/test_task_registration.py:24), so a products.workflows.… task is outside it and a silent non-registration looks exactly like the unconsumed-queue failure above.
   • Resolve the dedupe record before enqueuing. A resubmit that hits a cached or in-flight record must return the existing handle without a second task. Under the poll-by-reschedule transport a resubmit is the common path, not the rare one.
   • Slot leaks and their window. SoftTimeLimitExceeded is raised inside the task, so finally runs and the slot is released. A hard time_limit kill and a kernel OOM are not, and leak a slot for up to the TTL. The Lua purge is lazy and only runs when the set is already at the limit (limit.py:52-56), so a leak self-heals at the next saturating attempt rather than on a timer. At 30 slots and a 120 s TTL this is acceptable; it is worth one sentence in the PR rather than a mechanism.
   • Do not build a "slots in use" gauge on ZCARD. Because the purge only runs when full, the set over-reports expired members under the limit. The saturation signal is the existing CONCURRENT_TASKS_LIMIT_EXCEEDED_COUNTER (limit.py:36-40), which is already labelled by limit_name, plus the queue-wait and generation-duration histograms. That composes with Poll by reschedule versus a dedicated llm queue #20's condition 4.
   • Testing. CELERY_ALWAYS_EAGER is on in tests (posthog/settings/celery.py:34-38), so a task invoked from the view runs inline and the 202-then-poll protocol is never exercised end to end. Test the endpoint contract with the task's dispatch mocked so the 202, pending and terminal-failure shapes are real, and test the limiter against fakeredis directly. Note also that redis.get_client() memoizes a shared FakeServer per URL in a module global, so ZSET state leaks between tests unless flushed.

   Verified facts worth carrying

   • CELERY_QUEUES and CELERY_DEFAULT_QUEUE in posthog/settings/celery.py:9-10 are dead config. config_from_object(namespace="CELERY") maps them to queues and default_queue; the real keys are task_queues and task_default_queue. The same file's CELERY_IGNORE_RESULT (:30) is dead for the same reason, and its own comment documents the resulting half-broken behavior. Nothing breaks, because task_create_missing_queues defaults true and a Redis broker queue is just a list key, but do not read those two lines as evidence that queues are declared anywhere. Worth one confirmation at implementation time.
   • Nothing enforces the posthog/celery_queues.py to bin/celery-queues.env sync that both files ask for in comments (celery_queues.py:27, celery-queues.env:4), and the env file's comment still points at the enum's old home, posthog/tasks/utils.py, which is now a re-export shim (posthog/tasks/utils.py:11). If the dedicated queue is ever taken, a test asserting every CeleryQueue value appears in bin/celery-queues.env is the one test that change earns. Proposed as a small separate PR, not this map's to land.
   • CeleryQueue.SUBSCRIPTION_DELIVERY (posthog/celery_queues.py:39) has zero producers since subscriptions moved to Temporal, and is still consumed (bin/celery-queues.env:5). Unrelated cleanup, noted only so nobody copies it as a live example.
   • products/stamphog/backend/tasks/digest.py:65 calls an LLM on the default queue with no timeout and no Celery time limit, so the OpenAI SDK's 600 s per attempt with retries applies. It is the existing instance of the failure this ticket exists to prevent, and it is on the shared default pool. Not ours to fix; useful as the concrete argument for why the cap belongs in the PR.

   Amendments

   • The endpoint contract — the submit throttle's stated reason changes from worker occupancy to enqueue rate; the number and the placement do not change. The error-code list gains capacity_unavailable, returned at 200 as a terminal failure like the others. The Celery task's shape is now fixed: queue=CeleryQueue.LONG_RUNNING.value, the two @limit_concurrency decorators, soft_time_limit just above 60 with a 90 s hard limit, expires at the node's give-up, and an on_failure hook that writes the terminal record.
   • How a slow generation fits the CDP execution model — the Celery-with-the-result-in-Redis choice is re-affirmed against Temporal on a new argument: Temporal's concurrency controls are per worker process and would need the same hand-rolled cap. Recorded the revisit trigger, which is worker-seconds per generation and not durability.
   • Per-team fairness and batch sizing on the llm queue — its per-team fairness follow-up, already reduced by Poll by reschedule versus a dedicated llm queue #20 to a note, is now partly answered in a different place: the per-team concurrency cap on the task bounds one project's share of the fleet, which is the property fair dequeue was wanted for. Ordering fairness among waiting generations is still absent and still deferred.
   • Poll by reschedule versus a dedicated llm queue — the gap it raised is closed without touching the transport. Its 429-is-a-reschedule rule is unchanged and remains the throttle's semantics only. Its condition 4 gains two series: queue wait and generation duration.
6. Silthus

   closed this as completedon Aug 7, 2026