Adversarial Review of Retry and Dead-Letter Changes

I’ll treat this as a hostile correctness review: pin the actual change surface, recover the intended behavior, then probe boundary cases, adjacent contracts, and tests. I’ll report findings only—no code edits.

The working tree is the change: six modified/new files on top of main, with no branch/PR metadata to substitute for it. I’m now pinning that uncommitted diff and checking Entire’s review workflow plus the prior floor-age context before reading implementation details.

Entire’s built-in multi-reviewer command has no configured profile, and configuring one would mutate repo/local settings, so I’m continuing the adversarial pass directly against the pinned tree. The highest-risk seam is the new Retry state machine—especially floor identity under filtered consumers, DLQ publish/ack ordering, and lifecycle/concurrency.

A concrete blocker has emerged: “floor stationary” is treated as equivalent to “message stalled,” but an idle caught-up consumer also has a stationary floor. After 15 idle minutes, the next message that fails twice is classified as an aged blocker immediately; the timer never starts at the transition to pending work. I’m checking for additional independent failures before assigning severity.

I’m adding a throwaway, uncommitted diagnostic test file to make the two state-machine counterexamples executable. It will be removed immediately after the focused run; the user’s working tree will be restored byte-for-byte.

The first two counterexamples reproduced exactly. I’m removing that probe and running one last throwaway integration check against the pinned NATS server version: whether a failed DLQ publish on the final delivery actually redelivers as the API reports, or is retired by MaxDeliver.

The NATS integration probe confirmed a third independent blocker: on the final delivery, failed DLQ capture returns OutcomeRetried and NAKs, but MaxDeliver retires the message—there is no next delivery. I’m now reconciling these results against the captured task intent and doing a final clean-tree/test check.

Review verdict: request changes. Three P1 correctness issues in the uncommitted change atop 143ec67.

1. [P1] Idle time incorrectly ages newly arrived work — retry.go:419

   The poll starts the stationary timer even when NumAckPending == 0. After a healthy consumer sits idle for 16 minutes, the next message at floor+1 is dead-lettered on its second attempt as though it had stalled for 16 minutes. This also makes FloorStationaryFor and the warning log report healthy idle consumers as stalled.

2. [P1] The breaker cannot identify blockers for filtered consumers — retry.go:476

   Matching meta.Sequence.Stream == ackFloor.Stream+1 assumes stream sequences are dense. Filters make them sparse. JetStream provides consumer sequences specifically for this: compare meta.Sequence.Consumer with AckFloor.Consumer+1. A reproduced floor at stream 100/consumer 1 followed by stream 200/consumer 2 remained on the retry ladder indefinitely instead of tripping the breaker.

3. [P1] Failed final DLQ capture is reported as retried, but no retry occurs — retry.go:553

   On the final delivery, a failed capture calls NakWithDelay and returns OutcomeRetried. MaxDeliver has already been reached, so NATS retires the delivery instead. Against the pinned NATS server, MaxDeliver: 2 produced exactly two deliveries, then ack_pending=0; no third capture attempt occurred. The original remains uncaptured and requires manual recovery. DeadLetter has the same fallback at line 598.

Validation passed on the original tree: go test ./..., focused race tests, go vet ./..., mise run lint, go build ./..., and git diff --check. Throwaway adversarial probes reproduced all three failures and were removed; user files remain unchanged.

Entire’s multi-reviewer command had no configured profile, and checkpoint search was unavailable because the CLI is not logged in, so I did not mutate settings or initiate auth.

I’m using the zoom-out skill because you’re asking whether the design itself is sound, beyond the three implementation bugs. I’ll evaluate the failure model, ownership boundary, and operational recovery path.

No—not as one combined abstraction. The direction is good, but the floor-age breaker is being placed at the wrong level.

What I would keep:

• Enforce exactly one retry scheduler.
• Centralize retry/exhaustion settlement.
• Capture to the DLQ before acknowledging.
• Validate MaxDeliver consistently.
• Preserve payload, headers, sequence, and reason.

What I would separate:

• Retry should make deterministic, message-local decisions from delivery metadata.
• Floor monitoring should observe consumer-wide health and emit telemetry.
• The adopter should decide whether a stalled consumer warrants quarantining a specific message.

The core mismatch is that retry state is message-local, while ack-floor state is consumer-global. A client-side snapshot cannot reliably prove which message caused the stall or how long it caused it—idle periods, filters, restarts, replicas, and delivery caps all distort that inference. The three review findings are manifestations of this deeper mismatch rather than incidental coding errors.

I’d aim for this ownership:

The other architectural gap is terminal capture. A synchronous DLQ publish on the broker’s final delivery has no recovery path if that publish fails. A robust design needs either:

• A logical retry limit below the broker’s MaxDeliver, reserving deliveries for DLQ capture retries.
• An idempotent durable outbox/retry stream.
• An explicitly documented manual-recovery state—not OutcomeRetried.

My suggested rollout:

1. Land the single-scheduler enforcement and deterministic Retry/DLQ settlement.
2. Add floor monitoring separately, using consumer sequence and pending state.
3. Run it observe-only in search-indexer.
4. Add automatic quarantine there behind a flag once idle, filtered-stream, restart, replica, and DLQ-outage behavior is demonstrated.
5. Generalize it into go-nuts only after a second consumer proves the abstraction.

So: the incident diagnosis is sound, and centralizing retry mechanics is right. Making a 15-minute floor heuristic a generic library-owned automatic disposition is the part I would reconsider.

I’ll re-review from scratch against the current working tree, treating the earlier findings as stale. I’ll first pin what changed since the last snapshot, then retest the failure model and the revised module boundary.

The implementation changed materially: floor monitoring is now split into a new module, while Retry remains message-local. That matches the architectural direction. I’m checking whether the separation is real in behavior—especially terminal DLQ failure, reserved delivery budget, and whether the monitor avoids idle/filter false positives.

The earlier idle-floor bug is fixed, and the delivery reserve now gives failed captures a real retry before the broker cap. Two suspect seams remain: automatic action still uses stream-sequence adjacency on filtered consumers, and final-delivery Ack failure is still labeled retryable. I’m testing those plus monitor reuse across supervised consumer recreation.

I’ve confirmed the split is only partly fail-safe: a monitor is optional, but once supplied the zero-value configuration immediately enables automatic DLQ+Ack; “observe first” requires remembering to set an inverted boolean. I’m running two focused counterexamples now and checking the supervised recreate lifecycle before finalizing the verdict.

Much better—the revised architecture is broadly the right approach. The idle-floor bug and failed-capture retry path are fixed, and FloorMonitor is now cleanly separable from message disposition. I still wouldn’t ship it unchanged.

1. [P1] Filtered consumers still cannot identify the blocker — retry.go:405

   It still compares stream sequence with floor+1. For a filtered consumer, consumer sequence 2 may correspond to stream sequence 200. My probe reproduced the breaker remaining inactive on the actual blocker. Store AckFloor.Consumer and compare it with msg.Metadata().Sequence.Consumer; retain stream sequence only for telemetry.

2. [P2] Final-delivery Ack failure is still mislabeled as a retry — retry.go:527

   The capture reserve fixes failed publishing, but not failed acknowledgment after a successful capture. At MaxDeliver, Ack() failure returns OutcomeRetried, yet NATS permits no next delivery. The live probe produced two deliveries and no third. This should report a stranded/uncertain settlement, or use bounded DoubleAck to confirm settlement.

3. [P2] Run does not join the monitor before recreating — jsconsumer.go:508

   When the consume loop closes naturally, Run cancels the attempt but does not call Stop or wait for pollDone. It can attach the shared monitor to a new consumer while the old poll is still exiting, allowing stale consumer information to overwrite the new state. Join the old runner before the retry delay/recreation.

One rollout concern: BreakerObserveOnly is opt-in, while supplying RetryConfig.Monitor enables automatic DLQ+Ack by default. Since the docs call observe-only the rollout position, I’d prefer an explicit mode whose zero value is disabled/observe-only, with automatic action requiring an affirmative value.

The original tree passes go test ./..., go test -race ./..., go vet ./..., lint, build, and git diff --check. Temporary probes reproduced findings 1 and 2 and were removed.

Overall verdict: the design direction is now sound; fix the sequence identity and final-Ack classification before shipping. The lifecycle join and safer rollout mode should also be addressed before enabling automatic quarantine.

Thanks. The revision was a substantial improvement; the remaining issues are much narrower and actionable.

I’ll re-pin the current tree and re-run the review from scratch. I’ll specifically verify the three remaining findings against the new code, then probe for regressions introduced by their fixes.

The tree advanced again: all three core implementation files changed, along with the fake message semantics and documentation. Entire still has no configured review profile, so I’m continuing without mutating its settings.

The previous three findings appear addressed in code: blocker inference was replaced with a conservative conjunction, terminal settlement now uses confirmed DoubleAck and reports final failures as stranded, Run joins the old poll, and enforcement is explicit rather than the zero value. I’m now stress-testing the new conjunction and configuration invariants instead of assuming those repairs are sufficient.

The repairs are coherent, but the new quarantine proof has one questionable premise: NumDelivered does not prove the configured NakWithDelay intervals actually elapsed. AckWait expiry, crashes before Settle, or pre-existing delivery history can increment it much faster. I’m validating whether that can make enforcement quarantine newly failing work during an unrelated old stall.

I’m adding a temporary integration probe against the embedded NATS server. It simulates a process repeatedly losing a delivery to AckWait before reaching Settle; this distinguishes actual elapsed failure time from the synthetic ladder time inferred from NumDelivered. I’ll remove it after the run.

The integration probe confirmed the concern: with a three-second configured ladder age, four AckWait redeliveries arrived in 68 ms, yet enforcement recorded FailingFor=3s and dead-lettered the message during an older global stall. I’ve removed the probe and am doing an uncached final validation plus configuration-contract checks.

The uncached suite and uncached race suite both pass. I’m pinning the exact current lines and worktree state now; the remaining issue is semantic rather than something the existing tests catch.

I re-reviewed the current diff (d29a0c… against 143ec67). The previous issues are fixed, but I found one new blocker.

• [P1] Delivery count is not elapsed failure time. failingFor assumes every redelivery followed the configured NakWithDelay ladder. JetStream also increments NumDelivered after AckWait expires—for example, when a handler crashes before calling Settle. My integration probe produced four deliveries in 68 ms, but the code inferred FailingFor=3s and dead-lettered the fresh message during an older floor stall. This invalidates the “never early” safety claim and could quarantine many recoverable messages. Enforcement needs a durable first-failure timestamp; otherwise this should remain observe-only or be explicitly delivery-count-based.

• [P2] Enforce mode can silently operate without a monitor. NewRetry accepts BreakerEnforce with Monitor=nil, after which shouldQuarantine always returns false. A monitor supplied only through Config.FloorMonitor still does not drive the retry breaker. Reject enforce-without-monitor and unknown BreakerMode values during validation.

The earlier idle-floor, filtered-sequence, final-ack, lifecycle, and rollout-safety findings are resolved.

Validation passed:

• go test -count=1 ./...
• go test -race -count=1 ./...
• go vet ./...
• go build ./...
• mise run lint
• git diff --check

Verdict: request changes, primarily because the elapsed-age inference is unsafe.

I’ll re-pin the worktree and review the new implementation as a fresh diff, with particular focus on the two remaining failure modes: elapsed-age inference and breaker configuration validation.

The implementer replaced the delivery-count inference with explicit per-message timing and added the missing breaker validation. I’m checking whether that timing survives the real lifecycle boundaries—restarts, concurrent redelivery, sequence reuse, and capture failure—rather than just the happy-path tests.

A new inconsistency is emerging around RecoverBy: construction validates it, but runtime eligibility never consults it. I’m exercising the concrete case where configuration says recovery is expected by delivery 4, yet enforcement can quarantine on delivery 2 after enough wall time.

The elapsed timer has another edge: it survives a message’s recovery because successful processing bypasses Retry, so a later replay of the same stream sequence inherits the old failure start. I’m confirming that against the current code and then I’ll run the full uncached/race validation.

The main regression is confirmed. I’ve also found that enforcement continues to trust an arbitrarily old floor snapshot after consumer-info polling starts failing; I’m testing that fail-open path because it can turn an observability outage into automatic disposition.

I re-reviewed the updated tree at 143ec67 with diff hash 80e53d…. The earlier delivery-count inference and missing configuration validation are fixed, but I still recommend changes.

• [P1] Failure clocks survive successful recovery. noteFailure retains the first failure for each stream sequence, but normal success directly calls msg.Ack() and never informs Retry. If that message is replayed—after an ack loss or durable recreation—a fresh failure inherits the old age and can be dead-lettered immediately. Records below 4,096 entries are not even TTL-swept. I reproduced a recovered message being quarantined on its second fresh delivery with FailingFor=20m.

• [P1] Stale floor observations remain enforceable. pollOnce leaves the previous observation active when consumer-info polling fails, while FloorStall continues aging it. I reproduced enforcement from a 20-minute-old snapshot with no successful polls during that period. Automatic disposition should require a recent observation or disarm while polling is unhealthy.

• [P2] RecoverBy is never enforced at runtime. Its contract says a message must retain its recovery window, but shouldQuarantine checks only MinDeliveries. A valid configuration with RecoverBy=4 quarantined on delivery 2 after sufficient wall time. Runtime eligibility needs to include the recovery boundary.

The README also still says failure age comes “from its own delivery count” at README.md:170, contradicting the new measured-clock implementation.

All existing validation passes: uncached tests, race tests, vet, build, lint, formatting, and git diff --check. No remote CI applies to this uncommitted tree.