[Medium] The reused fresh-vs-cached distinction is inoperative in the production path.

The only production caller, _barrier_pg_decrement, unconditionally runs the entry-guard _get_conn() (~line 765) before _apply_decrement. That call populates _local.conn with a live, open connection. So by the time reused is sampled here, _local.conn is always non-None and open → reused is always True on attempt 1 — including the case the design explicitly wants to exclude: a brand-new connection whose first statement dies against a genuinely-down DB.

This is not a double-count bug (the execute-phase retry is unconditionally safe since nothing committed). The practical impact is bounded: one wasted reconnect + a 0.5s time.sleep before surfacing a real DB-down error. But:

• the documented invariant ("a freshly-created one failing is a genuine DB error… so it is not retried", lines 645-647) is not realized through the production entry point — here reused's only real effect is the one-shot bound, which attempt < _BARRIER_DECREMENT_ATTEMPTS already provides;
• test_fresh_conn_execute_failure_is_not_retried passes only because it calls _apply_decrement directly with _local.conn = None, bypassing the guard — false confidence;
• the "Mirrors PgQueueClient.send's reused-guard" claim is operationally inaccurate: in send, reused is sampled before any _get_conn.

Fix: capture reused in _barrier_pg_decrement before the entry-guard _get_conn() and thread it into _apply_decrement. Then add an end-to-end test (through _barrier_pg_decrement) for the fresh-conn case. Alternatively, if the team accepts the execute retry is always safe regardless, soften the docstrings/comments to stop claiming a fresh conn isn't retried.

Minor (same line): capture _local.conn once to avoid the double lookup —

cached = getattr(_local, "conn", None)
reused = cached is not None and not cached.closed

[Medium] Rollback failure is swallowed with no log, and can silently reclassify a non-connection error as a connection death.

Two compounding issues:

1. The rollback exception is discarded with zero logging. The original op error does surface later via the caller's raise, but the rollback failure itself leaves no trace — if rollback fails for an unexpected reason (e.g. an InternalError), that diagnostic is gone.
2. conn_dead = True on a failed rollback is the exact gate that decides whether _apply_decrement retries the execute phase (line 687). The docstring at lines 659-660 promises a DataError "propagates unchanged", but a non-_CONN_DEAD_ERRORS error whose rollback also fails gets relabeled conn_dead=True and routed into the retry path — contradicting the stated invariant. The counter stays safe (execute phase committed nothing), but the real error is masked behind a misleading "cached connection died, reconnecting" warning and re-run.

Fix: log the rollback failure with exc_info before flipping conn_dead:

except Exception:
    logger.warning(
        "PgBarrier: rollback during error recovery failed (original error %s) "
        "— treating the connection as dead and discarding it.",
        type(exc).__name__, exc_info=True,
    )
    conn_dead = True

[Low] The load-bearing "non-autocommit" invariant is documented but unenforced.

The entire exactly-once argument for the execute-phase retry rests on the connection being non-autocommit (so the uncommitted UPDATE rolls back on disconnect). This is currently true only because create_pg_connection (pg_queue/connection.py) never sets autocommit and thus inherits psycopg2's False default — it does not actively open non-autocommit, so the phrase "create_pg_connection opens it that way" slightly overstates. There is no runtime guard: a future change adding conn.autocommit = True there (e.g. to match the queue's commit-immediately pattern) would silently reintroduce the double-count this function exists to prevent — and the entry guard's TRANSACTION_STATUS_IDLE check does not imply non-autocommit.

Fix: (a) soften the wording to "non-autocommit (psycopg2's default; create_pg_connection does not override it)"; and (b) as a code change, assert the invariant loudly, e.g. assert not conn.autocommit at the top of _apply_decrement, so it fails loudly rather than corrupting the counter.

[Low / comment accuracy] This docstring names the wrong caller for commit-phase teardown.

The direct caller, _barrier_pg_decrement, does not catch a commit-phase error — its only except is psycopg2.DataError (~line 791). So a commit-phase failure propagates to whichever ultimate caller drove it:

• run_batch_with_barrier (in-body PG path) — caught and _abort_barrier_in_body runs → teardown. Fail-fast holds. ✅
• barrier_pg_decr_and_check (the Celery .link path, today's live path) — no teardown; the barrier strands to expires_at (max_retries=0, no replay).

The no-double-count guarantee still holds (commit is never retried), but the "tears the barrier down so the execution fails fast" claim is false for the path that's actually live today. Suggest rewording to cover both paths and drop the single-caller framing.

[Low] _CONN_DEAD_ERRORS is now copy #3 of an identical constant (pg_queue/client.py, pg_queue/result_backend.py). The comment here explicitly worries about the "was this a connection death?" test drifting — but the three module-level copies can drift independently, which is the exact risk the constant was meant to kill, just moved up to the module boundary.

Fix: hoist a single _CONN_DEAD_ERRORS into a shared location (e.g. pg_queue/connection.py, which all three already import for create_pg_connection) and import it in all three. If that's deemed too invasive, at minimum add a test asserting the three tuples are equal.

Comment nit: PgQueueClient._CONN_DEAD_ERRORS / PgResultBackend._CONN_DEAD_ERRORS are module-level constants, not class attributes — PgQueueClient._CONN_DEAD_ERRORS would AttributeError. Refer to them by module (pg_queue.client / pg_queue.result_backend).

[Low] The execute-phase give-up raise emits no decrement-level log.

The retry branch logs a warning (lines 688-698) and the commit branch logs (709-716), but this propagate path is silent. For a fresh-conn death (reused False) or a non-conn error, _barrier_pg_decrement only catches DataError, so an OperationalError here propagates with no barrier-specific breadcrumb — and on the Celery .link path the barrier isn't torn down, so it hangs to expires_at. The result is an asymmetric trail: retryable blips are logged, but the terminal "gave up / wasn't retryable" decision isn't. Consider one warning before this raise naming the execution, error type, and that the decrement is propagating without retry (mirroring the commit-phase log).

[Nit / comment accuracy] The "keeps the type checker honest" rationale is wrong.

The return annotation is tuple[int, Any] | None, so an implicit fall-through off the loop would return None — a valid return (the function legitimately returns None at line 718 when fetchone() finds no row). A type checker would therefore not flag a missing return whether or not this line exists. The AssertionError is a pure defensive runtime guard, not a type-checker aid. Reword to e.g. "Defensive: the loop always returns or raises before here." (Keeping the guard is fine — cheap insurance in concurrency code; just fix the justification.)

[Test gap] The time.sleep(_BARRIER_RETRY_BACKOFF_SECONDS) backoff is never asserted.

This _no_sleep fixture (and the inline monkeypatches at ~456/601/631) replace pg_barrier.time.sleep with a bare no-op purely to skip the wall-clock wait — none record or assert the call. If someone deleted the time.sleep in _apply_decrement (line 699), every test would still pass, and a "tighten the retry loop" refactor could silently start hammering a struggling DB.

Fix: have the fixture append to a list, then assert sleeps == [pg_barrier._BARRIER_RETRY_BACKOFF_SECONDS] in the retry test and sleeps == [] in the no-retry tests (commit-phase / fresh-conn).

[Test gap — ties to the reused finding] This test bypasses the real entry point.

It calls _apply_decrement(...) directly with _local.conn = None, so it exercises a fresh-conn state that the production wrapper (_barrier_pg_decrement → barrier_pg_decr_and_check) never produces: that wrapper's entry-guard _get_conn() always pre-populates _local.conn, making reused always True on attempt 1. So this test asserts a property the production path does not actually provide. Add an end-to-end test through _barrier_pg_decrement for the fresh-conn-not-retried case (or, if reused is fixed to sample before the entry guard, this becomes a true assertion).

[Test quality / informational] The stated rationale doesn't match the actual gating branch.

This test is framed around "the one-shot bound" (attempt < _BARRIER_DECREMENT_ATTEMPTS), but trace the path: on attempt 1, _recover_after_error sets _local.conn = None before the continue; on attempt 2, reused recomputes to False, so the retry is refused by reused, not by the attempt bound — both independently block it. The attempt < term can never be the sole reason a retry is refused, because every retry discards the connection. Net: the decrement is hard-capped at one self-heal by the reused logic regardless of the attempt constant, and bumping _BARRIER_DECREMENT_ATTEMPTS would NOT increase self-heals — an untested assumption worth a comment. The propagation outcome asserted here is correct; only the rationale needs fixing.