fix(idempotency): use wall-clock TTL

[NikolayS]

What changed

• calculate dedup claim expiry and expiry qualification with clock_timestamp() instead of transaction-stable now();
• calculate takeover expiry after any conflicting row-lock wait, so a winning contender receives a fresh window;
• use wall-clock expiry in idempotency maintenance sweeps;
• reject non-finite TTLs;
• add long-transaction and PostgreSQL 19 infinite-TTL regressions;
• regenerate the development install artifacts.

Why

A send_idem call made late in an older transaction could commit a claim whose TTL had already expired. An immediate producer retry then appended a second event instead of returning the original event id.

Fixes #305.

Validation

Red evidence on current main:

ERROR: new claim must expire after send time,
got expires_at=...13.021104 now=...13.174104

Green checks:

• bash build/transform.sh
• git diff --check
• PostgreSQL 14: install + complete tests/test_send_idem.sql
• PostgreSQL 18: install + complete tests/test_send_idem.sql + full acceptance suite
• PostgreSQL 19 beta 1: install + complete tests/test_send_idem.sql, including infinite-TTL rejection
• existing concurrent dedup race remains green.

This is intentionally a draft and has not been merged.

[NikolayS]

REV rubric review

This is a manual execution of the repository's five applicable REV rubrics because the automated ultrareview quota was unavailable. It is not bot output. SOC2 was omitted.

• Bug/security: wall-clock claim/takeover expiry and finite-TTL rejection are coherent; no additional defect or security issue found.
• Tests: long-transaction, immediate retry, contention, maintenance, and PG19 infinity behavior are covered; CI is 17/17 green. Original RED is retrospective because implementation and tests share the first commit.
• Docs/guidelines: error contract is precise. The style: follow-up conflicts with the repo-specific type list and its 52-character subject exceeds the repo limit.

Blocking patch findings: none. Remaining constraints: preserve history, use a compliant squash/merge subject, and post exact real-user database evidence before merge. This COMMENT review is not an approval.

[NikolayS]

Real-user verification evidence for head 2dac46c90d98255097a4b3f00f383277f41a1ad5.

I used fresh disposable PostgreSQL databases and ran:

PAGER=cat psql --no-psqlrc "$PGQUE_TEST_DSN" --set=ON_ERROR_STOP=1 \
  --file=devel/sql/pgque.sql
PAGER=cat psql --no-psqlrc "$PGQUE_TEST_DSN" --set=ON_ERROR_STOP=1 \
  --file=tests/test_send_idem.sql
bash build/transform.sh
git diff --check

The long-transaction regression against the pre-fix SQL reproduced an already-expired claim at send time:

ERROR: new claim must expire after send time,
got expires_at=2026-07-11 13:34:34.94415+00 now=2026-07-11 13:34:35.202518+00

The same fresh-install test against the PR head passed:

PASS: idempotency TTL starts at send time, not transaction start
PASS: US-13.1 duplicate returns original id, deduped=true, no insert
PASS: I2 concurrent duplicate race -> exactly one insert
PASS: cleanup complete (idem rows cascade with drop_queue)

The complete idempotency suite passed on PostgreSQL 14, 18, and 19beta1, including the non-finite-TTL rejection on PG19beta1. PostgreSQL 18 also ran the full acceptance suite. The existing concurrent duplicate race remained green.

RED/GREEN chronology: retrospective. The fix and its regression coverage were not published as a test-first RED commit followed by a GREEN commit. I independently ran the test with the pre-fix SQL to confirm RED and with this head to confirm GREEN.

Full CI: 17/17 checks passed.

This comment records test evidence only; it is not a review or merge decision.

[NikolayS]

samorev Code Review Report

• PR: fix(idempotency): use wall-clock TTL #320 - fix(idempotency): use wall-clock TTL
• Author: @NikolayS
• AI-Assisted: Yes

Pipeline	Coverage
unknown	Not reported

---

REVIEW FINDINGS (9)

HIGH MR/PR state - Review target is draft

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

CRITICAL CI/Pipeline - Pipeline status is unknown

Provider CI reported status unknown.
Fix: Fix failing checks and rerun review.

MEDIUM [tests] The infinite-TTL regression test is gated on PostgreSQL 19+, but infinite interval values were introduced in PostgreSQL 17. On PG 17 and 18 the block silently skips, so the new isfinite() rejection is never exercised there — including in the PR's own "PostgreSQL 18: complete tests/test_send_idem.sql" validation run. The accompanying comment ("PG19+") is wrong for the same reason.

-- Infinite TTLs create claims that maintenance can never reclaim (PG19+). / if current_setting('server_version_num')::int >= 190000 then
Fix: Lower the gate to >= 170000 and update the comment to "PG17+". The execute wrapper already keeps the 'infinity'::interval literal from being parsed on older versions, so no other change is needed.

MEDIUM [bugs] The fix moves the TTL anchor from transaction start to send time, but the claim is still written by an uncommitted transaction. If the caller does further work after send_idem and commits later than TTL, the committed claim is already expired at commit, and an immediate producer retry appends a second event — exactly the failure mode of #305, just relocated from "before the call" to "after the call". The new test only sleeps before the call, so this direction is untested and the fix reads as complete when it is partial.

values (v_queue_id, i_idem_key, null, clock_timestamp() + i_ttl) — expiry is fixed at statement time, while visibility to retriers begins at commit time.
Fix: Either document the constraint explicitly ("TTL must exceed the remaining duration of the calling transaction; call send_idem as late as possible in the transaction"), or anchor expiry to commit time (e.g. store the claim with a commit-time-derived expiry, or add a pg_xact_commit_timestamp-based grace margin). At minimum add the mirror-image regression: send with a short TTL, pg_sleep past it within the same transaction, commit, then assert the immediate retry still dedupes — and state the outcome.

MEDIUM [bugs] Switching the maintenance sweeps from now() to clock_timestamp() makes the qual volatile. PostgreSQL's planner refuses to use an expression containing volatile functions as an index qual (match_clause_to_indexcol rejects a rightop for which contain_volatile_functions() is true), so if pgque.idem has an index on expires_at these deletes degrade from index scans to full scans of the table — and with few expired rows the limit 10000 never short-circuits, so every sweep scans the whole table. These sweeps run inline via queue_extra_maint, i.e. inside a producer's send_idem call. The change also buys nothing: now() vs clock_timestamp() here only differ by the sweeping transaction's age, and rows missed by one sweep are collected by the next.

where d.expires_at < clock_timestamp() and where d.queue_id = v_queue_id and d.expires_at < clock_timestamp() (both with limit 10000)
Fix: Revert both maintenance predicates to now(), or keep wall-clock semantics without the volatility penalty by snapshotting into a local variable first (v_cutoff := clock_timestamp(); then where d.expires_at < v_cutoff) — a plpgsql variable is a stable parameter and remains indexable. Confirm with explain (analyze) on a populated pgque.idem before merging.

LOW [tests] The late-transaction regression depends on the second do block executing within 1 second of the first. The claim is written with a 1-second TTL and the retry assertion requires it to still be live, but between the blocks psql commits, round-trips, and reparses. On a loaded or slow CI runner this window is easily exceeded, and the failure would look like a real dedup regression rather than a timing artifact.

'late-txn:k1', '1 second') followed in the next transaction by assert v_dedup and v_eid = v_first, 'immediate post-commit retry must deduplicate a late-transaction send';
Fix: Widen the margin — e.g. perform pg_sleep(1.25) with a '5 seconds' TTL still proves the transaction is older than nothing relevant; better, keep the sleep long enough to exceed the old transaction-anchored expiry by using a larger TTL and a correspondingly larger sleep (sleep 5.25s / TTL 5s), or assert on the stored expires_at value rather than on a second live call.

LOW [tests] The headline behavior change in the conflict path — replacing expires_at = excluded.expires_at with a re-evaluated clock_timestamp() + i_ttl so a takeover gets a fresh window after a row-lock wait — has no test. The PR only claims "existing concurrent dedup race remains green", and that test asserts dedup identity, not the winner's expiry. A regression back to excluded.expires_at (the value computed before the lock wait) would pass every check listed in the Validation section.

set event_id = excluded.event_id, /* Evaluated after any conflicting row-lock wait... */ expires_at = clock_timestamp() + i_ttl
Fix: Add a dblink-based test where session A holds a lock on an expired claim row while session B's send_idem blocks; after A commits, assert B's resulting expires_at is later than B's pre-block time by roughly the full TTL, not shortened by the wait.

LOW [docs] Two user-visible contract changes ship with no documentation or changelog update: the TTL is now measured from wall-clock send time rather than transaction start, and infinite TTLs are now rejected with a changed error string. Any existing doc, SPEC section, or other test asserting the old text ttl must be a positive interval will now mismatch.

raise exception 'ttl must be a positive finite interval'; (was 'ttl must be a positive interval')
Fix: Grep the repo for the old message and update all callers/tests/docs; document the new TTL anchoring and the infinite-TTL rejection in the send_idem reference and changelog.

LOW [bugs] Unverified from the diff — only three now() call sites in send_idem were converted. If the dedupe branch (the else path that returns the existing event_id, not shown in this diff) still compares against now(), the function mixes two clocks: for a long transaction now() < clock_timestamp(), so an expiry check written as expires_at > now() would treat an already-dead claim as live and return a stale event_id.

only values (...), the on conflict clause, and the two maintenance predicates were changed; the rest of the function body is outside the diff hunks.
Fix: Grep the full send_idem body (and any helper it calls) for remaining now()/current_timestamp uses on pgque.idem.expires_at and convert them consistently, or add a comment where transaction-stable time is intentional.

---

Summary

Area	Findings	Potential	Filtered
CI/Pipeline	1	0	0
Security	0	0	0
Bugs	0	3	0
Tests	1	2	0
Guidelines	0	0	0
Docs	0	1	0
Metadata	1	0	0

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

Review metadata

provider=github
kind=pr
project=NikolayS/PgQue
number=320
target=github:NikolayS/PgQue#320
state=OPEN
draft=true
diff_lines=241
diff_added=97
diff_removed=24
diff_bytes=8851
comments_count=1
commits_count=2
ci_status=unknown
ci_summary=total=18 success=17 failure=0 pending=0 other=1
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)