Skip to content

fix: bound retry attempt lookup by the failed attempt time - #1152

Open
alexluong wants to merge 2 commits into
mainfrom
fix/retry-lookup-time-bound
Open

alexluong wants to merge 2 commits into
mainfrom
fix/retry-lookup-time-bound

Conversation

@alexluong

@alexluong alexluong commented Oct 18, 2026 •

Copy link
Copy Markdown
Collaborator

Bound the retry path's attempt lookup by the failed attempt's time, so each lookup reads a few granules instead of the whole attempts table.

Problem

Before it publishes a retry, the retry scheduler looks up the latest attempt for the event and destination:

SELECT ... FROM attempts
WHERE tenant_id IN (?) AND event_id IN (?) AND destination_id IN (?)
ORDER BY attempt_time DESC, attempt_id DESC LIMIT 2

On ClickHouse the table's sort key starts with attempt_time, and the query has no time condition, so only the bloom skip indexes apply. The tenant and destination blooms pass almost every granule (few distinct values); the event_id bloom keeps about 2.5 % of all granules as false positives. Cost grows linearly with history: with 8 M rows over 30 days each lookup reads ~13 MiB (192k rows, ~1,500 marks) and uses ~0.3 s of ClickHouse CPU, even for an event with a single attempt. The payload column is not the cost (lazy materialization reads it only for the returned rows); the key columns of the false-positive granules are. On 3 vCPU that caps retries at ~10 lookups/s, so a burst of failed deliveries saturates ClickHouse for minutes.

Idea

The retry task knows when the attempt that scheduled it happened. Carrying that time in the task lets the lookup start there: the latest attempt can't be older than the attempt that failed.

  1. RetryTask gets EventTime and AttemptTime (json:",omitzero"), set when the retry is scheduled from the failed attempt.
  2. The lookup sets TimeFilter.GTE = AttemptTime - 1s on the existing ListAttemptRequest. All three log store drivers (ClickHouse, Postgres, memory) already support it; no driver code changes. The 1 s covers storage precision (the stored attempt time is the same value the task carries; ClickHouse keeps ms).
  3. Tasks without AttemptTime (queued before the upgrade) run today's unbounded query.

There is no upper bound: attempts newer than the carried one (later automatic or manual retries) are still found.

Benchmarked on the same 8 M-row table (read, rows, marks: one lookup, warm caches; CPU and throughput: 50 concurrent lookups, ClickHouse at its 3 vCPU limit):

read read rows marks CPU per lookup lookups/s
today 13.1 MiB 194k 1,554 ~300 ms ~10 lookups/s
bounded 0.04 MiB 552 2 ~14 ms ~210 lookups/s

Postgres already finds the row with an index seek on event_id (idx_attempts_event_time, ~0.1 ms at 2 M rows); the bound adds time >= ... to the same index condition and keeps one code path.

Behavior changes

  • Lookups for new tasks only see attempts at or after the failed attempt. If the failed attempt isn't in the log store yet (log batches are written every LOG_BATCH_THRESHOLD_SECONDS), the lookup now returns nothing and the task goes through the normal exec-error backoff until the write lands. Before, it found the attempt before that one, if any, and continued with a stale attempt number. fix: requeue retries whose attempt is not persisted yet without counting receives #1154 treats this case as "not ready" without counting it as a failed receive.
  • Task JSON: old instances ignore the new fields; new instances treat tasks without them as before.

Results

Local reproduction, 18,000 events in 60 s, destination failing for the first 20 s (setup below):

main this PR
lookup read (p50) 13.1 MiB 0.1 MiB
lookup rows / marks (p50) 192k / 1,532 855 / 1
lookup ClickHouse CPU (p50) 286 ms 13 ms
lookup duration (p50, server) 3.2 s 0.19 s
lookups that completed 1,343 8,292
ClickHouse connection pool timeouts 25,601 4,290
events delivered after the destination recovered 13,242 / 18,000 17,979 / 18,000

The remaining pool timeouts come from the scheduler starting one goroutine per received retry against a 10-connection pool (capped in #1153). The 21 undelivered events stopped after their first retry because of a delete race in the scheduler, fixed in #1151; builds that include that fix (#1153, #1154) delivered all 18,000.

How the numbers were measured

Local reproduction: ClickHouse 25.8 limited to 3 vCPU with 8 M rows in the attempts table (30 days, ~90 KB average payload), 3 Outpost instances (all services in each, 2 CPUs, 2 GiB), Redis-compatible store for the retry queue, NATS as the internal queue. Retry schedule 1, 5, 300, 1800, ... seconds, LOG_BATCH_SIZE=50, LOG_BATCH_THRESHOLD_SECONDS=10, RETRY_VISIBILITY_TIMEOUT_SECONDS=30. Workload: 18,000 events (12 KB payload) in 60 s to one webhook destination that returns 5xx for the first 20 s, then 200 (about 6,000 failed first attempts, so about 6,000 retries due within ~20 s). Lookup cost from system.query_log; ClickHouse CPU from the container's cgroup. One run per build; three runs of the main baseline agreed within 3 % on pool timeouts and delivered events (lookup p50 duration 3.2-3.6 s).

Tests

  • Log store driver suite: new "ListAttempt attempt time lower bound" case for ClickHouse, Postgres and memory (older attempts excluded, newer ones found).
  • internal/deliverymq/retrytask_test.go: JSON compatibility (old payload decodes with zero times and runs unbounded; new payload round-trips; zero times omitted).
  • Retry exec test asserts every lookup carries the bound; message handler test checks the times reach the scheduled task.

Not in this PR

  • Capping the fallback query for old tasks by a time window: they only exist until the longest retry delay has passed after the upgrade.
  • Combining concurrent lookups into one query (feat: batch retry attempt lookups #1155).

🤖 Generated with Claude Code

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant