samorev Code Review Report

• PR: fix(go): back off before re-polling failed batch #284 - fix(go): back off before re-polling failed batch
• Author: @NikolayS
• AI-Assisted: Unknown

BLOCKING ISSUES (1)

HIGH MR/PR state - Review target is draft

The review target is still marked as draft.
Fix: Mark it ready for review before merge.

Summary

Note:

• Findings: High-confidence issues (8-10/10) - blocking or non-blocking per severity
• Potential: Medium-confidence issues (4-7/10) - review manually
• Filtered: Low-confidence issues (0-3/10) - excluded as likely false positives

provider=github
kind=pr
project=NikolayS/PgQue
number=284
target=github:NikolayS/PgQue#284
state=OPEN
draft=true
diff_lines=174
diff_added=139
diff_removed=1
diff_bytes=6157
comments_count=0
commits_count=1
ci_status=success
ci_summary=total=14 success=14 failure=0 pending=0 other=0
prompt=.claude/commands/review-mr.md
blocking=false
posted_by=gh
no_comment=false
live_posting=posted

samorev-assisted review (AI analysis by Tanya301/samorev)

Deep review + real testing (unreviewed pass — prior "samorev" review disregarded)

Rebased onto current origin/main (stable/devel split); clean, clients/go-only. Head 2ed70d8, all 16 CI checks green.

Fix review

Correct and consistent. Both new backoff sleeps use the same ctx-aware
select { case <-ctx.Done(): return ctx.Err(); case <-time.After(c.pollInterval): }
idiom already used on the receive-error and empty-poll paths, then continue.
The ack-path refactor (else if n == 0 → error-continue + standalone if n == 0)
is right: a stale/double-ack (n == 0) is a finished batch and must not back off;
only the ack error path (batch unfinished) does. gofmt/vet clean.

• Bounded? Yes — constant delay of one pollInterval. No unbounded growth.
• Resets on success? N/A by construction — there is no accumulating backoff
  state; each failure independently waits one interval, a successful ack waits none.
• Context-cancellation responsive? Yes — both sleeps return ctx.Err() on
  <-ctx.Done(); max exit latency is one pollInterval, same as the other paths.
  Confirmed live: the partial-batch test cancels mid-flight and the loop exits promptly.

Double-processing semantics (by design, documented). The fix delays re-polling;
it does not prevent handler re-execution. An unfinished batch is redelivered whole on
the next Receive, so every handler re-runs — including ones that already succeeded.
This is inherent to PgQ's at-least-once, all-or-nothing finish_batch boundary; the
new code comments state it explicitly. Correct scope for a backoff fix — dedup is the
handler's responsibility (must be idempotent).

Non-blocking observations:

• Fixed-interval (no exponential/jitter) matches the existing receive-error and
  empty-poll backoff, so it is internally consistent. Under a sustained ack/nack
  outage this yields one failing re-execution per pollInterval indefinitely (bounded
  rate). Exponential-with-cap + jitter would cut DB load and de-sync fleets under a long
  outage, but that would also touch the existing receive-error path — out of scope here,
  reasonable follow-up.
• The ctx-sleep select block now appears 3× in Start; a small func (c) sleep(ctx) error
  helper would DRY it. Optional.

Red/green TDD — verified myself

Two guard tests added (nack-failure, ack-failure) counting Receive calls in a 400ms
window, asserting <= window/pollInterval + slack. Reverting the two sleeps to reproduce
the pre-fix behavior:

--- FAIL: TestConsumer_NackFailure_BacksOffBeforeRepoll
    Receive called 99042 times in 400ms with pollInterval 50ms (want <= 11)
--- FAIL: TestConsumer_AckFailure_BacksOffBeforeRepoll
    Receive called 317153 times in 400ms with pollInterval 50ms (want <= 11)

With the fix, both pass (~8 Receive calls each). Decisive demonstration of the hot-loop
and its fix.

Real testing

Postgres 17 + devel/sql/pgque.sql (91 functions installed), full Go client suite:

• go vet ./... clean; gofmt -l clean on both changed files.
• Full suite green (ok github.com/NikolayS/pgque-go 37.9s).
• Backoff tests pass reliably; red/green confirmed as above.

One pre-existing flake, not a blocker: under full-suite parallel load,
TestConsumer_PartialBatch_AcksAfterSomeNacks intermittently fails with
post-ack Receive returned 3 rows — the test's cancel() races the consumer's Ack
(ack error: pgque: ack: context canceled). It reproduces identically on origin/main
and passes 5× in isolation, so it is unrelated to this PR (the fix changes neither the
Ack's context nor whether Ack is attempted). Suggest a separate follow-up to harden that
test (drain the loop before cancelling). CI runs it green.

Verdict: MERGE-READY.

samorev Code Review Report

• PR: fix(go): back off before re-polling failed batch #284 - fix(go): back off before re-polling failed batch
• Author: @NikolayS
• AI-Assisted: Yes

This report supersedes the earlier samorev review --fetch comment on this PR. That comment was a CI/draft gate check only and performed no code analysis (it hardcoded zeros in every area). The results below come from a genuine per-area review of the diff, verified with a red/green test on a fresh PostgreSQL 17 devel build.

No blocking issues. Three non-blocking observations below; two are already tracked in #299.

What was verified

• Backoff correctness: both new sleeps use the same ctx-aware select { case <-ctx.Done(): return ctx.Err(); case <-time.After(c.pollInterval): } idiom already used on the receive-error and empty-poll paths, then continue. Bounded (one pollInterval, no accumulating state); a successful ack waits none.
• ack-path refactor is right: n == 0 (stale/double-ack) is a finished batch and must not back off; only the ack error path (batch unfinished) sleeps. gofmt/vet clean.
• Context cancellation stays responsive: both sleeps return ctx.Err() on <-ctx.Done(); max exit latency is one pollInterval, same as other paths — confirmed live (partial-batch test cancels mid-flight, loop exits promptly).
• Red/green TDD: two guard tests count Receive calls in a 400 ms window. Red (sleeps reverted): Receive called 99042 / 317153 times in 400ms (hot loop). Green: ~8 calls each. go test full suite green (ok … 37.9s).

NON-BLOCKING (3)

INFO clients/go/consumer.go - fixed-interval backoff (no exponential/jitter)

Matches the existing receive-error and empty-poll backoff, so it is internally consistent. Under a sustained ack/nack outage this yields one failing re-execution per pollInterval indefinitely (bounded rate).
Suggestion: Exponential-with-cap + jitter would cut DB load and de-sync fleets under a long outage, but it would also touch the existing receive-error path — reasonable follow-up, out of scope here. Tracked in #299.

INFO clients/go/consumer.go (Start) - duplicated ctx-sleep block

The ctx-sleep select now appears 3× in Start.
Suggestion: A small func (c) sleep(ctx) error helper would DRY it. Optional.

LOW clients/go/consumer_test.go - pre-existing partial-batch test flake (not introduced here)

Under full-suite parallel load, TestConsumer_PartialBatch_AcksAfterSomeNacks intermittently fails (post-ack Receive returned 3 rows) because the test's cancel() races the consumer's Ack. It reproduces identically on origin/main and passes 5× in isolation, so it is unrelated to this fix (which changes neither the Ack's context nor whether Ack is attempted).
Suggestion: Harden the test (drain the loop before cancelling). Tracked in #299.

Double-processing semantics (not a finding): the fix delays re-polling; it does not prevent handler re-execution. An unfinished batch is redelivered whole, so every handler re-runs — inherent to PgQ's at-least-once, all-or-nothing finish_batch boundary, and stated in the new code comments. Dedup remains the handler's responsibility.

Summary

Note:

• Findings: High-confidence issues (8-10/10) - blocking or non-blocking per severity
• Potential: Medium-confidence issues (4-7/10) - review manually
• Filtered: Low-confidence issues (0-3/10) - excluded as likely false positives

provider=github
kind=pr
project=NikolayS/pgque
number=284
target=github:NikolayS/pgque#284
state=OPEN
draft=false
diff_lines=174
diff_added=139
diff_removed=1
diff_bytes=6161
comments_count=1
commits_count=1
ci_status=success
ci_summary=total=16 success=16 failure=0 pending=0 other=0
prompt=.claude/commands/review-mr.md
blocking=false
posted_by=gh
no_comment=false
live_posting=posted

samorev-assisted review (AI analysis by Tanya301/samorev)