fix(queue): fence stale outcome writes with attempts-generation (HT-43)

[zaridan]

What & why

CodeRabbit flagged a lease-expiry race on the Postgres durable queue adapter (PR #43, HT-43). drainOnce leases a whole batch upfront (claimBatch: UPDATE … SET locked_until = now()+lease, attempts = attempts+1 … FOR UPDATE SKIP LOCKED RETURNING *), then processes rows sequentially. If a handler run outlives its lease, a concurrent drainer (overlapping cron tick, a retry racing a slow run) can reclaim the row — bumping attempts — while the original worker's outcome write still matched only WHERE id = $1. So a stale ack could DELETE, or a stale retry reset locked_until/run_after on, a row another worker now owns.

Bounded today to redundant processing (the only handler, gmail-reconcile, is idempotent and advances its cursor only on terminal outcomes — so no mail is lost), but the queue is a general seam and should be fenced at the SQL level.

The fix

Use the claimed row's post-increment attempts as an optimistic-concurrency generation. Every outcome write now carries AND attempts = <the value this worker claimed> + RETURNING id:

• ack DELETE (index.ts ~L427)
• retry UPDATE (~L477)
• deadLetterJob UPDATE (~L310) — now returns a boolean, checked at both the explicit-deadLetter and retry-past-ceiling call sites

A reclaim has bumped attempts past that value, so a stale write matches 0 rows and touches nothing. Zero affected rows is treated as "reclaimed, not mine": logged (queue_stale_skip, house-style structured console.warn), counted as the new DrainReport.staleSkipped, and not counted as ack/retry/deadLetter. New partition invariant: claimed === acked + retried + deadLettered + staleSkipped.

This is the SQL-level guarantee that complements the config-level defense already shipped in #43 (vercel.json caps maxDuration at 50s, below the 60s DEFAULT_LEASE_MS/cron interval) — it holds even if that config changes.

Never-drop invariant (charter §2) — verified, not asserted

• A pre-change spike over two independent pg connections showed the original code losing a job: worker B reclaimed + rescheduled a row, worker A's stale ack DELETEd it → 0 rows left.
• The new index.race.test.ts proves the job survives (countRows === 1) under that exact race with the fence in place, and that a stale retry cannot release the lease its new owner holds (the real owner then completes its ack cleanly).
• Both race tests fail against the unfenced code and pass with the fence — non-vacuous.

Testing

New index.race.test.ts uses the repo's blessed real-Postgres harness (PGLiteSocketServer + two independent pg pools, mirroring src/db/postgres.test.ts) with a short lease + a deterministic barrier — genuine cross-connection FOR UPDATE SKIP LOCKED lease expiry that in-process PGlite can't exercise. No wall-clock sleeps (lease expiry is forced via locked_until = now() - 1s, the same trick forceAllDue uses), so it's not flaky.

• Full suite: 750/750 passing (41 files)
• tsc --noEmit: clean · biome check .: clean (180 files)

Reviewer note (one judgment call)

The ticket said the existing 12 PGlite tests must "pass unchanged." I added staleSkipped to DrainReport (for the clean partition invariant + cron-log observability of the fence firing), which required extending their 11 exact-match report literals with staleSkipped: 0. I read "unchanged" as the fence must not alter their outcomes — verified: with the fence in, the only failures were the added key, proving the AND attempts= clause is a no-op absent a reclaim. If you'd prefer that file zero-touch, dropping the field for log-only is a small revert.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Prevented stale workers from deleting, rescheduling, or modifying jobs reclaimed by another worker.
  • Improved queue reliability during concurrent processing and lease-expiration races.
  • Added reporting for outcomes skipped because a job was no longer owned by the processing worker.
  • Added warning logs when stale job outcomes are safely ignored.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: ac76efa9-3f0d-4c51-9041-6053d4827f50

📥 Commits

Reviewing files that changed from the base of the PR and between 478bb51 and 23bdf71.

📒 Files selected for processing (3)

• src/providers/adapters/postgres-queue/index.race.test.ts
• src/providers/adapters/postgres-queue/index.test.ts
• src/providers/adapters/postgres-queue/index.ts

---

📝 Walkthrough

Walkthrough

The Postgres queue adds attempt-generation fencing to ACK, retry, and dead-letter writes, reports stale outcomes through staleSkipped, logs structured warnings, and adds deterministic multi-connection race tests.

Changes

Postgres queue stale-outcome fencing

Layer / File(s)	Summary
Fence queue outcomes src/providers/adapters/postgres-queue/index.ts	Outcome writes now require the claimed attempts value; zero affected rows are logged and counted as stale skips in DrainReport.
Validate concurrent reclaim races src/providers/adapters/postgres-queue/index.race.test.ts	Independent Postgres connections test stale ACK and retry behavior during lease expiry and job reclamation.
Update drain report expectations src/providers/adapters/postgres-queue/index.test.ts	Existing drain scenarios expect staleSkipped: 0 for non-racing outcomes.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant WorkerA
  participant PostgresQueue
  participant WorkerB
  WorkerA->>PostgresQueue: Claim job
  WorkerB->>PostgresQueue: Reclaim expired job
  WorkerA->>PostgresQueue: Submit fenced ACK or retry
  PostgresQueue-->>WorkerA: Report stale skip
  WorkerB->>PostgresQueue: Complete reclaimed job

Loading

Possibly related PRs

• Helpthread/helpthread#43: Introduces the initial Postgres queue implementation extended by this change.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: fencing stale queue outcome writes with the claimed attempts value.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/ht-43-queue-lease-fence

---

_{Comment @coderabbitai help to get the list of available commands.}