fix(partition-keys): fence lease release

[NikolayS]

What changed

• refuse pgque.release_slot() when the owning worker still has an open slot batch;
• retain the lease so another worker cannot claim through an invalid mid-batch handoff;
• add regression coverage directly in the zombie/takeover scenario;
• regenerate both development install artifacts.

Why

The lease is the process-to-ack fence. Previously an owner could release it after receive_partitioned() but before ack_partitioned(). A successor could then claim the slot and receive the same open batch while the original worker was still processing it, violating the single-processor guarantee.

Crash recovery remains unchanged: a genuinely abandoned batch is recovered after lease expiry, with the existing epoch fencing.

Fixes #304.

Validation

Red/green:

• the new regression failed on current main with fencing: owner release with an open batch must raise;
• it passes after the fix and verifies the attempted release leaves the original lease intact.

Checks run:

• bash build/transform.sh
• git diff --check
• PostgreSQL 14: install + tests/test_partition_keys.sql
• PostgreSQL 18: install + tests/test_partition_keys.sql + full tests/acceptance/run_acceptance.sql
• PostgreSQL 19 beta 1: install + tests/test_partition_keys.sql

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: the open-batch release fence preserves lease ownership and prevents concurrent handoff; no additional defect or security issue found.
• Tests: focused failure and lease-retention assertions cover the hazard; CI is 17/17 green. The initial SQL/test commit makes the original RED retrospective.
• Docs/guidelines: invariant comments are concise and generated artifacts match. The historical test: commit conflicts with the repo-specific CLAUDE type list, although generic rules allow it.

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 73dc1d68c8be7f394fdcbe9e721996226d59abc6.

I tested this from fresh disposable databases. PGQUE_TEST_DSN pointed at the fresh database in each PostgreSQL container. The focused replay was:

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_partition_keys.sql
bash build/transform.sh
git diff --check

The PR regression replayed against the pre-fix SQL failed at the intended assertion:

ERROR: fencing: owner release with an open batch must raise

The same test against the PR head passed and continued through the full partition suite:

PASS fencing: zombie ack raises after takeover; heir re-issued same batch
PASS guard: receive_partitioned renews the lease (heartbeat)
PASS: partition keys (Phase 1A)

This verifies that a mid-batch release_slot() is rejected, the original lease remains in place, a stale owner cannot ack after takeover, and the successor receives the same open batch. Fresh-install coverage was repeated on PostgreSQL 14, 18, and 19beta1; PostgreSQL 18 also ran the full acceptance suite.

RED/GREEN chronology: retrospective. The implementation was already present before the regression-test commit. I independently replayed that regression against the parent/pre-fix SQL to establish RED, then against this head to establish GREEN; this is not a chronological test-first commit sequence.

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(partition-keys): fence lease release #311 - fix(partition-keys): fence lease release
• Author: @NikolayS
• AI-Assisted: Yes

Pipeline	Coverage
unknown	Not reported

---

REVIEW FINDINGS (10)

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 [bugs] The new partition_consumer join is an INNER join, so it changes the lookup's not-found semantics. pc.n is needed only for the batch probe, but a missing/unmatched consumer row now makes the whole select return zero rows, sending release_slot() down its not-found path (return false) for a slot whose lease genuinely exists and is genuinely owned by the caller. The lease then cannot be released cooperatively at all — it can only be cleared by expiry. This also silently regresses any caller that relied on release succeeding during consumer teardown/reconfiguration.

Fix: Use left join pgque.partition_consumer as pc ... so slot resolution and ownership checking keep their previous semantics, and handle v_n is null explicitly (fail closed — raise — rather than skipping the batch probe). If an FK truly guarantees the consumer row exists, add a comment saying so; otherwise the inner join is an unintended behavior change.

MEDIUM [guidelines] Inconsistent error contract on a returns boolean API. Every pre-existing refusal path returns false; the new refusal raises instead, which aborts the caller's transaction. A worker doing graceful shutdown over several slots (for slot in ... loop perform pgque.release_slot(...)) previously skipped un-releasable slots and now loses the entire transaction on the first slot with an open batch. Separately, the exception uses the default P0001/raise_exception with no custom errcode, so callers cannot distinguish "batch still open" from any other P0001 in the extension without matching English prose — the new test demonstrates exactly this, pinning itself to the message text with sqlerrm like 'cannot release slot 0 of consumer w ...', so any future rewording silently breaks both clients and the test.

Fix: Either return false for consistency with the other refusal paths, or keep the raise but attach a stable, matchable condition — e.g. using errcode = '55006' (object_in_use) or a dedicated pgque SQLSTATE — and assert on that errcode in the test instead of on the message string. Document which of the two contracts callers should code against.

MEDIUM [bugs] The guard resolves n from the current partition_consumer row, not from the n the open batch was created under. If a consumer's n can be changed (resize/rebalance), the partition→slot mapping changes, so _slot_batch(..., v_n) probes a different partition set than the one the in-flight batch belongs to, returns NULL, and the guard passes — releasing the lease with a batch still open, i.e. reintroducing the exact defect this PR fixes. The failure is silent and fails open: any anomaly in resolving n (including pc.n being NULL) yields v_batch_id is null and permits the release.

Fix: Derive the open-batch check from the batch's own recorded slot/epoch rather than recomputing it from live n — e.g. probe partition_slot/batch state directly for "any unacked batch attributed to this slot lease". If n is genuinely immutable for a consumer's lifetime, state that invariant in the comment. At minimum, treat v_n is null as "cannot prove the batch is closed" and refuse rather than allow.

MEDIUM [bugs] Check-then-act without a visible row lock. The lookup select (as shown) takes no for update on pgque.partition_slot, so the batch probe and the subsequent update are not atomic with respect to the slot row. Two connections sharing the same i_owner identity — a reconnecting worker, a pooled connection, a worker with a separate heartbeat session — can have one open a batch via receive_partitioned() in the window between the probe returning NULL and the update clearing the lease. The release then succeeds against a slot that now has an open batch, which is the exact invariant the comment claims to enforce.

Fix: Add for update on ps in the lookup (or for no key update) so the slot row is pinned for the duration of the check and the update, or fold the guard into the update's where predicate (where ... and pgque._slot_batch(...) is null) and derive the refusal from the affected row count.

MEDIUM [bugs] No escape hatch for a batch that cannot be acked. The change makes lease expiry the only route out of an open batch, but expiry is driven by lease_until, which the owning worker itself renews. A worker holding a poison or otherwise unackable batch — the case where an operator most wants a cooperative release — can now neither release (refused) nor let the lease lapse (it keeps renewing), pinning the slot indefinitely. The PR description asserts "crash recovery remains unchanged," which is true for a crashed worker but not for a live worker that wants to abandon a batch.

Fix: Add an explicit i_force boolean default false (or a separate admin pgque.abandon_slot()) that releases the lease and marks the batch abandoned/redeliverable in one statement, and document that a worker must stop renewing its lease before expiry can recover the batch.

MEDIUM [docs] This is a breaking, user-visible behavior change to a public API — pgque.release_slot() gains a new failure mode that raises instead of returning — and the changed-file set contains no documentation or changelog update. Existing callers that treat release_slot() as infallible during shutdown will start seeing aborted transactions with no upgrade note explaining why or what the new required call order (receive → ack → release) is.

Changed files are devel/sql/pgque-api/partition_keys.sql, devel/sql/pgque-tle.sql, devel/sql/pgque.sql, tests/test_partition_keys.sql — no docs/changelog entry accompanies the new raise.
Fix: Document the new precondition and error on release_slot() in the API reference, and add a changelog/upgrade note stating the new failure mode, its SQLSTATE, and the required receive → ack → release ordering.

LOW [tests] The "lease intact" assertion is indirect. claim_slot(...) is null does discriminate the red case (with the guard removed the lease would be cleared and the claim would succeed), but it does not verify what the PR description claims — that the original lease is intact. It would also pass if the release had partially mutated the row (e.g. cleared lease_until but not lease_owner, or bumped the epoch) in a way that still blocks a claim. Additionally, issuing a real claim_slot for a new worker id injects wk-early into a scenario that goes on to assert epoch-bump behavior during the heir takeover a few lines later, coupling the new assertion to that later state.

Fix: Assert directly against pgque.partition_slot — that lease_owner is still 'wk-zombie' and lease_until (and the epoch) are unchanged from values captured before the release attempt — and keep the claim_slot probe only if the successor-rejection path is itself the thing under test.

LOW [tests] The added coverage exercises only the refusing path. Nothing in the included hunk asserts the complementary property — that release_slot() still succeeds after ack_partitioned() closes the batch — so the diff as shown does not demonstrate the guard is not over-blocking the legitimate cooperative handoff it is explicitly meant to preserve. Caveat: only this one hunk of tests/test_partition_keys.sql is visible, so such a case may already exist elsewhere in the file.

The new block ends at assert pgque.claim_slot(...) is null and proceeds directly to perform pg_sleep(1.2); for the expiry/takeover path; no post-ack release assertion appears in the hunk.
Fix: If not already covered elsewhere in the file, add a positive case in the same scenario: ack the open batch, then assert pgque.release_slot('pk_q', 'w', 0, 'wk-zombie') returns true and that a successor's claim_slot then succeeds.

---

Summary

Area	Findings	Potential	Filtered
CI/Pipeline	1	0	0
Security	0	0	0
Bugs	0	4	0
Tests	0	2	0
Guidelines	0	1	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=311
target=github:NikolayS/PgQue#311
state=OPEN
draft=true
diff_lines=143
diff_added=63
diff_removed=3
diff_bytes=5719
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)