Skip to content

fix(logmq): don't wait on in-flight exhausted-retries suppression - #1063

Open
alexluong wants to merge 1 commit into
mainfrom
fix/logmq-suppression-window
Open

fix(logmq): don't wait on in-flight exhausted-retries suppression#1063
alexluong wants to merge 1 commit into
mainfrom
fix/logmq-suppression-window

Conversation

@alexluong

Copy link
Copy Markdown
Collaborator

Fixes the opevent delivery failed … context deadline exceeded nack reported in #1050 for subscribers of alert.attempt.exhausted_retries. #1050 keeps unsubscribed events out of the window entirely; this covers the case where the event is subscribed.

The failure

A destination goes down. Its queued events retry and exhaust around the same time, spread across the log service replicas. Every exhaustion on that destination wants to send one exhausted_retries alert through the same suppression key, opevents:exhausted:<tenant>:<destination>, so that only the first one in the window goes out.

The window was an idempotence.Idempotence configured with only the window TTL, so it inherited the package's 5s conflict wait, meant for delivery idempotency where the claimed work is an HTTP call. When two exhaustions overlap:

  1. Replica A wins SETNX, key is processing, A emits (a few ms) and marks the key processed.
  2. Replica B lands in those few ms, sees processing, and sleeps 5s before rechecking.
  3. B's send ctx had a 5s emit budget when it entered, so the recheck runs on an expired ctx and returns context deadline exceeded.
  4. send() returns the error, sendAll logs opevent delivery failed, and the whole log message nacks. MarkProcessed never runs.
  5. Redelivery re-evaluates the attempt and re-plans its events, so already-sent siblings such as attempt.failed or destination.disabled can go out twice. The window key is processed by then, so the retry acks.

The ErrConflict → delivered branch in send() was written for this case but is unreachable: the sleep always outlives the ctx. One collision costs a goroutine parked 5s, an error log, a nack and redelivery, and possible duplicate alerts. The loser had nothing to wait for; the window's contract is one alert per destination per window, and A's alert is already on its way.

The fix

Replaces the window with a SETNX-per-window type in logmq: losing the claim is the suppression, return immediately. A failed emit releases the claim so the next exhaustion re-alerts. send() drops its ErrConflict handling. Live windows reset once on upgrade since the key value changes, at worst one extra exhausted-retries alert per destination mid-window.

The window was an idempotence.Idempotence with only SuccessfulTTL set, so
its in-flight conflict path inherited the 5s default: a contender that
saw "processing" slept 5s, the same as the emit budget, and then failed
its recheck on the expired ctx. send()'s ErrConflict branch was
unreachable; every concurrent exhaustion on one destination nacked with
"context deadline exceeded" and was redelivered.

Replace it with a SETNX-per-window type in logmq. Losing the claim is
the suppression: return immediately, no in-flight state, no wait. A
failed emit releases the claim so the next exhaustion re-alerts.
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