fix(signals): bound poison batch retries in grouping v2 - #91367
fix(signals): bound poison batch retries in grouping v2#91367posthog[bot] wants to merge 1 commit into
Conversation
A signal batch whose preparation fails deterministically was re-stashed at the head of the buffer and retried forever, jamming the team's later signals and emitting signal_dropped once per attempt. Count failed attempts per S3 batch key, carried across continue_as_new. After MAX_BATCH_ATTEMPTS, dead-letter the key: drop its signals, remove it from the buffer, and emit signal_dropped once per signal. The prep-phase drop telemetry now defers to this terminal path so the metric measures drops, not retries. In-flight runs replay the old unbounded path behind a workflow patch; fresh runs take the bounded path. Generated-By: PostHog Desktop Task-Id: 85abfc88-077f-4a8c-888b-9610e5a5ebc9
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
🦔 PostHog Review reviewed this pull requestFound 3 must fix, 0 should fix, 0 consider. Published 3 findings (view the review). Resolved comments: 3 left for you |
🤖 CI report
|
|
This changes the behavior of a live, running Temporal workflow (retry/dead-letter logic that now intentionally drops signals after repeated failures), which is production execution/event-processing logic — risky territory — and it currently has zero human or agent reviews or approvals to back it up.
Gate mechanics and policy version
|
|
PostHog Review alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
| for key in collected.object_keys: | ||
| attempts = self._batch_key_attempts.get(key, 0) + 1 | ||
| self._batch_key_attempts[key] = attempts | ||
| (dead_keys if attempts >= MAX_BATCH_ATTEMPTS else retry_keys).append(key) |
There was a problem hiding this comment.
A poison key exhausts healthy keys' retry budgets
Why we think it's a valid issue
- Checked: whether a collected round really holds more than one key, whether a single bad signal fails the whole round, and whether the re-stash rebuilds the same key set. Traced
_collect_next_batch(products/signals/backend/temporal/grouping_v2.py:137-171),_process_signal_batch's prep phase (products/signals/backend/temporal/grouping.py:1062-1205), the buffer flush shape (products/signals/backend/temporal/buffer.py:35-36, 308-321), and the re-stash plus collection order. - Found: the round is multi-key whenever flushes are partial.
BUFFER_FLUSH_TIMEOUT_SECONDS = 5(buffer.py:36) flushes an under-full buffer, so each key can carry far fewer thanBATCH_COLLECT_MAX_SIGNALS = 20signals (grouping_v2.py:37), and the loop at grouping_v2.py:141-169 keeps popping keys until it reaches 20 signals or the 30s deadline. A stalled team — the exact incident state — has many such keys queued. - Found: one bad signal fails the whole round. The prep phase wraps the type-example fetch, embeddings, candidate search, and report-context fetch in one
tryand re-raises for the entire batch (grouping.py:1194-1205), with the comment stating the whole batch is explainable as dropped. So_process_collectedcatches a batch-wide error (grouping_v2.py:251) and_handle_batch_failurecharges an attempt to every key incollected.object_keys(grouping_v2.py:274-277). - Found: the same batch reforms each round. Retry keys go back at the head in their original order (grouping_v2.py:285) and
_collect_next_batchpops from the head (grouping_v2.py:156), while newly submitted keys append at the tail (grouping_v2.py:105). The head keys and their signal counts are unchanged, so the collection loop stops at the same boundary and rebuilds an identical key set. All of them reachMAX_BATCH_ATTEMPTSin the same round and land indead_keystogether (grouping_v2.py:277-282). - Impact: confirmed permanent loss of healthy signals.
_dead_letterdrops every signal of every dead key and emitssignal_droppedfor each (grouping_v2.py:300-310) with no re-queue, so signals that would have prepared successfully on their own never become reports. The loss is user-visible and lands on the same teams the PR targets. - Impact: the gap also contradicts the stated design.
signals_by_keyexists so "a dead-lettered key drops only its own signals" (grouping_v2.py:60-61), but because the failure is batch-wide and membership is stable across rounds, the per-key partition never isolates the poison key — it only splits signals after every co-collected key has already been condemned. - Priority: lowered to
should_fix. On master the same healthy keys are already stuck behind the poison batch forever, together with every later signal for that team, so this PR reduces total loss rather than introducing a regression. The collateral is bounded to the keys that share the failing round (at most about 20 signals) and stops once the poison key leaves the buffer. It is worth fixing — retrying keys individually after a combined failure, and charging attempts only to a key that fails alone — but it does not block the improvement the PR delivers.
Issue description
One preparation failure increments the attempt counter for every collected key. The retry path puts those keys back together. Later rounds usually rebuild the same combined batch. At the cap, the workflow drops healthy keys that only shared a batch with the poison key.
Suggested fix
Isolate the source key after a combined failure. Process each key separately, or bisect the keys, only on the failure path. Increment and dead-letter only a key that fails in isolation.
Prompt to fix with AI (copy-paste)
## Context
@products/signals/backend/temporal/grouping_v2.py#L274-277
@products/signals/backend/temporal/grouping_v2.py#L284-285
<issue_description>
One preparation failure increments the attempt counter for every collected key. The retry path puts those keys back together. Later rounds usually rebuild the same combined batch. At the cap, the workflow drops healthy keys that only shared a batch with the poison key.
</issue_description>
<issue_validation>
- **Checked:** whether a collected round really holds more than one key, whether a single bad signal fails the whole round, and whether the re-stash rebuilds the same key set. Traced `_collect_next_batch` (products/signals/backend/temporal/grouping_v2.py:137-171), `_process_signal_batch`'s prep phase (products/signals/backend/temporal/grouping.py:1062-1205), the buffer flush shape (products/signals/backend/temporal/buffer.py:35-36, 308-321), and the re-stash plus collection order.
- **Found:** the round is multi-key whenever flushes are partial. `BUFFER_FLUSH_TIMEOUT_SECONDS = 5` (buffer.py:36) flushes an under-full buffer, so each key can carry far fewer than `BATCH_COLLECT_MAX_SIGNALS = 20` signals (grouping_v2.py:37), and the loop at grouping_v2.py:141-169 keeps popping keys until it reaches 20 signals or the 30s deadline. A stalled team — the exact incident state — has many such keys queued.
- **Found:** one bad signal fails the whole round. The prep phase wraps the type-example fetch, embeddings, candidate search, and report-context fetch in one `try` and re-raises for the entire batch (grouping.py:1194-1205), with the comment stating the whole batch is explainable as dropped. So `_process_collected` catches a batch-wide error (grouping_v2.py:251) and `_handle_batch_failure` charges an attempt to every key in `collected.object_keys` (grouping_v2.py:274-277).
- **Found:** the same batch reforms each round. Retry keys go back at the head in their original order (grouping_v2.py:285) and `_collect_next_batch` pops from the head (grouping_v2.py:156), while newly submitted keys append at the tail (grouping_v2.py:105). The head keys and their signal counts are unchanged, so the collection loop stops at the same boundary and rebuilds an identical key set. All of them reach `MAX_BATCH_ATTEMPTS` in the same round and land in `dead_keys` together (grouping_v2.py:277-282).
- **Impact:** confirmed permanent loss of healthy signals. `_dead_letter` drops every signal of every dead key and emits `signal_dropped` for each (grouping_v2.py:300-310) with no re-queue, so signals that would have prepared successfully on their own never become reports. The loss is user-visible and lands on the same teams the PR targets.
- **Impact:** the gap also contradicts the stated design. `signals_by_key` exists so "a dead-lettered key drops only its own signals" (grouping_v2.py:60-61), but because the failure is batch-wide and membership is stable across rounds, the per-key partition never isolates the poison key — it only splits signals after every co-collected key has already been condemned.
- **Priority:** lowered to `should_fix`. On master the same healthy keys are already stuck behind the poison batch forever, together with every later signal for that team, so this PR reduces total loss rather than introducing a regression. The collateral is bounded to the keys that share the failing round (at most about 20 signals) and stops once the poison key leaves the buffer. It is worth fixing — retrying keys individually after a combined failure, and charging attempts only to a key that fails alone — but it does not block the improvement the PR delivers.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Isolate the source key after a combined failure. Process each key separately, or bisect the keys, only on the failure path. Increment and dead-letter only a key that fails in isolation.
</potential_solution>
There was a problem hiding this comment.
Confirmed — this is a real defect at the current head, and it works exactly as described. When a collected round holds several keys and processing fails, _handle_batch_failure charges an attempt to every key in the round, not just the one that caused the failure. Because failed keys are re-stashed at the head of the buffer and collection also pops from the head (new keys append at the tail), the same multi-key round reforms each iteration, so every co-collected key hits the attempt cap together and is dead-lettered together. Healthy keys that only shared a round with the poison key lose their signals, which is the opposite of the per-key isolation the signals_by_key grouping was meant to give.
I'm escalating rather than fixing this unattended, for two reasons. First, the remedy is a design decision, not a contained change: isolating the culprit means adding a per-key or bisection retry path and deciding which of those to build. Second, and more important, _process_collected catches any failure from _process_signal_batch, including post-emission failures in the sequential phase (the step-7 ClickHouse wait) where some signals were already assigned. Re-running preparation per key on that kind of failure could create duplicate reports. Getting that right means distinguishing a prep-phase failure (safe to reprocess) from a post-emission one, and that can't be proven safe without the database-backed signals test suites, which don't run in this environment.
What a human needs to decide: (1) per-key reprocessing vs bisection on the failure path, and (2) how the retry path should treat a post-emission partial-commit failure so reprocessing can't double-emit reports. This isn't a blocker for the PR — before this change the same healthy keys were stuck behind the poison batch forever, so the PR is still a clear improvement; this is the follow-up that makes the per-key isolation actually hold.
| try: | ||
| # emit_prep_drops=False: a prep failure is retried, so this path owns the terminal | ||
| # signal_dropped telemetry (see _dead_letter) rather than emitting once per attempt. | ||
| dropped, _type_examples = await _process_signal_batch(collected.signals, emit_prep_drops=False) | ||
| except Exception as error: | ||
| logger.exception( | ||
| "Failed to process signal batch", | ||
| team_id=input.team_id, | ||
| batch_size=len(collected.signals), | ||
| batch_keys=collected.object_keys, | ||
| ) | ||
| await self._handle_batch_failure(input, collected, error) |
There was a problem hiding this comment.
Post-emission failures are retried as preparation failures
Why we think it's a valid issue
- Checked: whether
_process_signal_batchcan raise after signals are assigned, and what the new catch does with that error. Read the prep-phasetry(products/signals/backend/temporal/grouping.py:1088-1205), the sequential phase (grouping.py:1207-1373), step 7 (grouping.py:1375-1395), the parallel variant (products/signals/backend/temporal/parallel_grouping.py:233, 331, 377), and the caller's catch at products/signals/backend/temporal/grouping_v2.py:251-259. - Found: step 7 sits outside every
tryin the function.wait_for_signal_in_clickhouse_activityruns withRetryPolicy(maximum_attempts=2)and a 1h5m start-to-close (grouping.py:1377-1395), and its error propagates straight out of_process_signal_batch, afterassign_and_emit_signal_activityhas already written the signals and their reports (grouping.py:1306-1324; parallel_grouping.py:233). - Found: the code already documents the overlap. The prep-phase handler's comment states that the outer workflow handler "also catches post-emission failures (the CH wait in step 7), where signals were successfully assigned" (grouping.py:1196-1197). The new catch at grouping_v2.py:251 is a bare
except Exception, so it cannot separate the two classes, yet its comment claims the path owns terminal telemetry for prep failures (grouping_v2.py:248-249). - Found: each retry re-emits.
signal_idis a freshuuid.uuid4()per invocation in both the sequential loop (grouping.py:1231) and the parallel path (parallel_grouping.py:331), so a re-run assigns new signals to reports rather than resuming. - Found: the terminal telemetry is mislabeled.
_dead_letterreads the original S3 signals fromcollected.signals_by_keyand emitscapture_signal_dropped(..., stage="grouping_prep")for each (grouping_v2.py:300-310), regardless of the fact that those signals were emitted on an earlier attempt.capture_signal_droppedfeeds bothmetrics.increment_droppedand the product-analytics event stream (products/signals/backend/temporal/drop_telemetry.py:57-59). - Impact: on a ClickHouse-wait failure the batch is dead-lettered as a prep-stage drop while its signals are present in reports, so the
signal_droppedmetric gains a new false-positive source — the same metric this PR exists to make trustworthy for the anomaly alert. TheDropping poison signal batch after repeated failureslog (grouping_v2.py:301-306) then sends an operator to look for missing signals that were actually written, possibly several times. - Priority: lowered to
should_fix. The re-emission on retry is pre-existing: the legacy path re-stashes and retries the identical way with no bound (grouping_v2.py:312-332), so this change reduces the duplicate writes from unbounded to three rounds. The genuinely new harm is wrong telemetry and a misleading terminal log, not user-facing signal loss, and it needs the CH wait to exhaust both attempts in three consecutive rounds. Worth fixing by distinguishing the prep failure from a post-emission failure, but it does not block the improvement the PR delivers.
Issue description
_process_signal_batch can raise after assignment when wait_for_signal_in_clickhouse_activity fails. The broad catch requeues those keys as if preparation failed. Each retry creates new signal IDs and repeats report writes. The terminal attempt then reports the assigned signals as grouping_prep drops.
Suggested fix
Return phase information or raise a dedicated preparation exception from grouping.py. Call _handle_batch_failure only for that exception. When a post-emission wait fails, consume the keys and report the wait failure without signal_dropped.
Prompt to fix with AI (copy-paste)
## Context
@products/signals/backend/temporal/grouping_v2.py#L247-258
@products/signals/backend/temporal/grouping_v2.py#L299-310
<issue_description>
`_process_signal_batch` can raise after assignment when `wait_for_signal_in_clickhouse_activity` fails. The broad catch requeues those keys as if preparation failed. Each retry creates new signal IDs and repeats report writes. The terminal attempt then reports the assigned signals as `grouping_prep` drops.
</issue_description>
<issue_validation>
- **Checked:** whether `_process_signal_batch` can raise after signals are assigned, and what the new catch does with that error. Read the prep-phase `try` (products/signals/backend/temporal/grouping.py:1088-1205), the sequential phase (grouping.py:1207-1373), step 7 (grouping.py:1375-1395), the parallel variant (products/signals/backend/temporal/parallel_grouping.py:233, 331, 377), and the caller's catch at products/signals/backend/temporal/grouping_v2.py:251-259.
- **Found:** step 7 sits outside every `try` in the function. `wait_for_signal_in_clickhouse_activity` runs with `RetryPolicy(maximum_attempts=2)` and a 1h5m start-to-close (grouping.py:1377-1395), and its error propagates straight out of `_process_signal_batch`, after `assign_and_emit_signal_activity` has already written the signals and their reports (grouping.py:1306-1324; parallel_grouping.py:233).
- **Found:** the code already documents the overlap. The prep-phase handler's comment states that the outer workflow handler "also catches post-emission failures (the CH wait in step 7), where signals were successfully assigned" (grouping.py:1196-1197). The new catch at grouping_v2.py:251 is a bare `except Exception`, so it cannot separate the two classes, yet its comment claims the path owns terminal telemetry for prep failures (grouping_v2.py:248-249).
- **Found:** each retry re-emits. `signal_id` is a fresh `uuid.uuid4()` per invocation in both the sequential loop (grouping.py:1231) and the parallel path (parallel_grouping.py:331), so a re-run assigns new signals to reports rather than resuming.
- **Found:** the terminal telemetry is mislabeled. `_dead_letter` reads the original S3 signals from `collected.signals_by_key` and emits `capture_signal_dropped(..., stage="grouping_prep")` for each (grouping_v2.py:300-310), regardless of the fact that those signals were emitted on an earlier attempt. `capture_signal_dropped` feeds both `metrics.increment_dropped` and the product-analytics event stream (products/signals/backend/temporal/drop_telemetry.py:57-59).
- **Impact:** on a ClickHouse-wait failure the batch is dead-lettered as a prep-stage drop while its signals are present in reports, so the `signal_dropped` metric gains a new false-positive source — the same metric this PR exists to make trustworthy for the anomaly alert. The `Dropping poison signal batch after repeated failures` log (grouping_v2.py:301-306) then sends an operator to look for missing signals that were actually written, possibly several times.
- **Priority:** lowered to `should_fix`. The re-emission on retry is pre-existing: the legacy path re-stashes and retries the identical way with no bound (grouping_v2.py:312-332), so this change reduces the duplicate writes from unbounded to three rounds. The genuinely new harm is wrong telemetry and a misleading terminal log, not user-facing signal loss, and it needs the CH wait to exhaust both attempts in three consecutive rounds. Worth fixing by distinguishing the prep failure from a post-emission failure, but it does not block the improvement the PR delivers.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Return phase information or raise a dedicated preparation exception from `grouping.py`. Call `_handle_batch_failure` only for that exception. When a post-emission wait fails, consume the keys and report the wait failure without `signal_dropped`.
</potential_solution>
There was a problem hiding this comment.
Confirmed, and the facts check out. Step 7 — the wait for emitted signals to land in ClickHouse — runs outside every try/except in _process_signal_batch, and it runs after the signals and their reports have already been written in step 6. So when that wait (or the summary-workflow spawn after it) fails, the error looks identical to a preparation failure to the catch in _process_collected. The batch is then retried as if nothing had been written — each retry assigns brand-new signal IDs and writes the reports again — and at the attempt cap the already-written signals are dead-lettered and counted as grouping_prep drops. That pollutes the same signal_dropped metric this PR exists to make trustworthy, and the 'Dropping poison signal batch' log sends an operator hunting for signals that are actually present.
I'm escalating rather than fixing this unattended, and it should be resolved together with the sibling thread on this same catch block (the one about a poison key exhausting healthy keys' retry budgets) — both come from one root cause: the broad except Exception in _process_collected can't tell a preparation failure from a post-emission failure. The fix you suggest is the right shape — raise a dedicated preparation exception (or return phase info) from grouping.py, send only that to the dead-letter/retry path, and handle a post-emission wait failure separately without emitting drops. But choosing that behavior is a design decision, not a mechanical change: consuming the keys means skipping the step-7 ClickHouse-visibility wait that the summary workflows and the next batch's semantic search rely on, so a human needs to confirm that's safe and decide what telemetry a post-emission failure should record. That correctness question can't be proven in this environment, which has no Postgres for the database-backed signals suites.
What a human needs to decide: (1) the prep-vs-post-emission signal (dedicated exception type or a phase field), (2) whether a post-emission wait failure should consume the keys or retry, given the skipped CH-visibility wait, and (3) what telemetry, if any, that failure should emit instead of a grouping_prep drop. This doesn't block the PR — the duplicate re-emission already existed unbounded on the legacy path, so this change bounds it; the newly-introduced harm is wrong telemetry, not signal loss.
| # A batch key that fails processing this many times is dead-lettered: its signals are dropped | ||
| # and the key leaves the buffer, so a deterministic failure cannot jam the team's pipeline. | ||
| MAX_BATCH_ATTEMPTS = 3 |
There was a problem hiding this comment.
Brief dependency outages cause permanent signal loss
Why we think it's a valid issue
- Checked: whether the failure path distinguishes a transient dependency failure from a signal-specific one, how long the three-attempt budget actually lasts in wall clock, and what the pre-change path did with the same error. Read
_handle_batch_failure(products/signals/backend/temporal/grouping_v2.py:268-290), the legacy path (grouping_v2.py:312-332), the prep phase and its inner retry policies (products/signals/backend/temporal/grouping.py:1088-1205), and the round timing constants. - Found: no classification exists anywhere on the path.
_handle_batch_failureincrements the counter for every key unconditionally (grouping_v2.py:274-277);erroris only ever passed through to_dead_letterfor telemetry (grouping_v2.py:280, 310). Greppinggrouping_v2.pyreturns no reference tonon_retryable,ApplicationError, or any retryability check, so an embedding-service 5xx and a permanently malformed signal consume the budget identically. - Found: the inner policies only cover seconds. The prep activities use
RetryPolicy(maximum_attempts=3)for type examples, embeddings, and semantic search, andmaximum_attempts=5for query generation (grouping.py:1097-1159), all on Temporal's default one-second initial interval, so a shared outage surfaces to the workflow within seconds. - Found: the outer budget is minutes, not hours. Each round costs at most
BATCH_COLLECT_TIMEOUTof 30s plus the failed prep plus a fixedRETRY_BACKOFFof 10s (grouping_v2.py:38-39, 290) — the PR's own test cites "the ~44s-per-round wall clock it takes in production" (products/signals/backend/test/test_grouping_v2_workflow.py). Three attempts therefore expire in roughly one to two minutes, so the reviewer's "about 30 seconds" is optimistic but the order of magnitude holds. - Found: the drop is permanent and unbounded in scope.
_dead_letteremits telemetry and returns without re-queueing anything (grouping_v2.py:292-310), and the key never returns to the buffer, so the S3 object is never read again. During a sustained outage every batch fails, so keys keep reaching the cap in waves and each wave is discarded. - Impact: confirmed customer-visible data loss from an ordinary failure mode. Any outage of the LLM gateway, ClickHouse, or Postgres lasting longer than about two minutes discards every signal flowing through the pipeline, and those signals never become reports. This is a regression against the branch point: the legacy path re-stashed and slept without a cap (grouping_v2.py:327-332), so the same batch survived a ten-minute dependency outage and drained on recovery. The change converts recoverable delay into permanent loss for a failure class that has nothing to do with poison data.
- Impact: the trigger is the same dependency that caused the incident this PR addresses — a 500 from the embedding API. A single-signal 500 motivated the fix; a broader 500 burst is the ordinary generalization of it, not a speculative case.
Issue description
_handle_batch_failure ignores whether error is retryable. It spends the three-attempt budget on HTTP 5xx responses and other shared service failures. The inner retry policies use short default delays. The outer path adds only two ten-second waits before the third failure. A fast shared outage can dead-letter valid keys in about 30 seconds. This changes a brief service outage into permanent signal loss, even when no key contains poison data.
Suggested fix
Classify the activity cause before consuming the terminal budget. Dead-letter known signal-specific or non-retryable failures. Move retryable shared failures to a deferred queue with exponential backoff. Continue to process new keys from the main buffer. Add a test where a retryable 5xx outlasts the fast retries and later recovers.
Prompt to fix with AI (copy-paste)
## Context
@products/signals/backend/temporal/grouping_v2.py#L40-42
@products/signals/backend/temporal/grouping_v2.py#L247-258
@products/signals/backend/temporal/grouping_v2.py#L268-290
<issue_description>
`_handle_batch_failure` ignores whether `error` is retryable. It spends the three-attempt budget on HTTP 5xx responses and other shared service failures. The inner retry policies use short default delays. The outer path adds only two ten-second waits before the third failure. A fast shared outage can dead-letter valid keys in about 30 seconds. This changes a brief service outage into permanent signal loss, even when no key contains poison data.
</issue_description>
<issue_validation>
- **Checked:** whether the failure path distinguishes a transient dependency failure from a signal-specific one, how long the three-attempt budget actually lasts in wall clock, and what the pre-change path did with the same error. Read `_handle_batch_failure` (products/signals/backend/temporal/grouping_v2.py:268-290), the legacy path (grouping_v2.py:312-332), the prep phase and its inner retry policies (products/signals/backend/temporal/grouping.py:1088-1205), and the round timing constants.
- **Found:** no classification exists anywhere on the path. `_handle_batch_failure` increments the counter for every key unconditionally (grouping_v2.py:274-277); `error` is only ever passed through to `_dead_letter` for telemetry (grouping_v2.py:280, 310). Grepping `grouping_v2.py` returns no reference to `non_retryable`, `ApplicationError`, or any retryability check, so an embedding-service 5xx and a permanently malformed signal consume the budget identically.
- **Found:** the inner policies only cover seconds. The prep activities use `RetryPolicy(maximum_attempts=3)` for type examples, embeddings, and semantic search, and `maximum_attempts=5` for query generation (grouping.py:1097-1159), all on Temporal's default one-second initial interval, so a shared outage surfaces to the workflow within seconds.
- **Found:** the outer budget is minutes, not hours. Each round costs at most `BATCH_COLLECT_TIMEOUT` of 30s plus the failed prep plus a fixed `RETRY_BACKOFF` of 10s (grouping_v2.py:38-39, 290) — the PR's own test cites "the ~44s-per-round wall clock it takes in production" (products/signals/backend/test/test_grouping_v2_workflow.py). Three attempts therefore expire in roughly one to two minutes, so the reviewer's "about 30 seconds" is optimistic but the order of magnitude holds.
- **Found:** the drop is permanent and unbounded in scope. `_dead_letter` emits telemetry and returns without re-queueing anything (grouping_v2.py:292-310), and the key never returns to the buffer, so the S3 object is never read again. During a sustained outage every batch fails, so keys keep reaching the cap in waves and each wave is discarded.
- **Impact:** confirmed customer-visible data loss from an ordinary failure mode. Any outage of the LLM gateway, ClickHouse, or Postgres lasting longer than about two minutes discards every signal flowing through the pipeline, and those signals never become reports. This is a regression against the branch point: the legacy path re-stashed and slept without a cap (grouping_v2.py:327-332), so the same batch survived a ten-minute dependency outage and drained on recovery. The change converts recoverable delay into permanent loss for a failure class that has nothing to do with poison data.
- **Impact:** the trigger is the same dependency that caused the incident this PR addresses — a 500 from the embedding API. A single-signal 500 motivated the fix; a broader 500 burst is the ordinary generalization of it, not a speculative case.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Classify the activity cause before consuming the terminal budget. Dead-letter known signal-specific or non-retryable failures. Move retryable shared failures to a deferred queue with exponential backoff. Continue to process new keys from the main buffer. Add a test where a retryable 5xx outlasts the fast retries and later recovers.
</potential_solution>
There was a problem hiding this comment.
Confirmed, and this is the most important issue on the PR: as written it is a regression that can lose data. The retry bound treats every failure the same. There is no check anywhere on this path for whether the error is retryable — the failed error is only used for telemetry — so a brief outage of a shared dependency (the embedding gateway, ClickHouse, or Postgres) burns the same three-attempt budget as a genuinely poison signal. That budget is only about one to two minutes of wall clock, and a dead-lettered key is never put back in the buffer, so its S3 batch is never read again. During an outage longer than that, every batch fails, keys hit the cap in waves, and each wave is dropped for good. Before this change the pipeline only delayed those signals and drained them once the dependency recovered, so this converts recoverable delay into permanent loss for a failure class that has nothing to do with poison data.
I'm escalating rather than fixing this unattended, and it should be resolved together with the two sibling threads on this same failure path — all three come down to the fact that _handle_batch_failure and _process_collected handle every failure identically instead of distinguishing a poison signal, a post-emission failure, and a transient shared outage. The classification the fix needs is genuinely hard: the incident trigger was an embedding-API 500, and the same 500 status can be either a transient shared outage (which should keep retrying) or a deterministic per-signal failure (the poison case this PR is meant to bound), so retryability alone doesn't separate them. The suggested deferred queue with exponential backoff is also new machinery that trades directly against the poison-loop protection. That tradeoff, and its behavior under a real outage, can't be proven in this environment, which has no Postgres for the database-backed signals suites.
What a human needs to decide: (1) how to classify a shared-dependency/transient failure apart from a deterministic per-signal one on this path; (2) whether transient failures should be deferred-and-retried (queue + backoff, or a wall-clock-bounded retry that outlasts a typical outage) rather than counted toward the dead-letter budget; and (3) how aggressive the poison bound should stay so it still stops a real poison loop without discarding signals during an ordinary outage. Given this is a data-loss regression against the branch point, it is worth prioritizing before the PR merges.
Problem
issue_createdsignal jammed the grouping pipeline for over 20 hours and later signals queued behind it._process_signal_batchfails,_run_new_collect_batch_pathre-stashed the failed S3 batch keys at the head of the buffer, slept, and continued as new — with no attempt limit, so a deterministic failure (a 500 from the embedding API for one signal) retried forever.grouping_prephandler emittedsignal_droppedfor every signal on every attempt, so the metric counted retries, not drops.Changes
signal_droppednow fires once per dropped signal, not once per retry, so the metric measures drops and stops tripping the anomaly alert.continue_as_new. AtMAX_BATCH_ATTEMPTS, the key is dead-lettered: its signals drop, it leaves the buffer, and itssignal_droppedtelemetry emits once.emit_prep_drops=False); the dead-letter path owns the terminal telemetry.CollectedBatchnow groups signals by source key so a dead-lettered key drops only its own signals.Note
This edits a running
@workflow.defn. In-flight executions replay the old unbounded stash-and-sleep path behind thebounded-batch-retry-v1patch; fresh runs take the bounded path. A poison run drains on its nextcontinue_as_newafter deploy.How did you test this code?
test_poison_batch_is_dead_lettered_after_attempt_cap: a batch whose preparation always fails is read once per attempt, then dead-lettered at the cap, and its one signal drops exactly once. This catches both an unbounded retry (reads would not stop) and per-attempt drop emission (drops would exceed one).test_buffer_workflow.pylocally; both pass.ruffandmypyare clean on the changed files.Automatic notifications
Docs update
None.
🤖 Agent context
Autonomy: Fully autonomous
/writing-tests,/writing-pr-descriptions,/writing-user-facing-copy,/writing-code-comments.elsebranch, so an in-flight replay does not hit a non-determinism error.Created with PostHog Desktop from this inbox report.