fix(mail): reclaim stuck 'received' inbound deliveries via lease (HT-45) - #48
Merged
Conversation
A hard crash between InboundDeliveryStore.claim() committing 'received' and the ingest pipeline's step-5 store transaction (or its catch-block markFailed) stranded the delivery at 'received' forever: claim() only reclaimed 'failed' rows, so re-delivery replayed the same stuck 'in-progress' outcome on every retry, and (HT-41's cursor coupling) could block the mailbox's reconcile cursor from ever advancing. Adds inbound_deliveries.claimed_until (migration 014), mirroring threads.claimed_until (migration 003) and ConversationStore's outbound lease/reclaim pattern. claim() now stamps a lease on every successful claim and reclaims a 'received' row once its lease has lapsed, via the same row-locked UPDATE shape already used for the failed-row reclaim - concurrency-safe by construction, proven by a concurrent-reclaim test. No new sweep function: a re-delivered push notification, or the Gmail reconcile handler's history replay (which keeps re-listing the same stuck message while the cursor can't advance past it, bounded above by the daily watch-maintenance sweep even with no new mail), already calls claim() again for the same key - the lease is what makes that call actually reclaim and reprocess instead of reporting 'in-progress' forever. Decision and reasoning recorded in inbound-deliveries.ts's module doc and specs/mail/inbound-ingestion.md §4. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… (HT-45 review) The lease reclaim HT-45 added had no fencing at commit time: a slow-but-alive owner that outlives its reclaimed lease could still commit after another worker reclaimed it, risking two live owners writing two outcomes for one email (spec §8's "exactly one conversation"). `attempts` now doubles as a claim generation — every successful claim returns it, and every ledger write (markStoredInTx/markSuppressed/markFailed/markDeadLetter) fences its UPDATE on `status = 'received' AND attempts = $claimedAttempts`, throwing the new LeaseLostError on a stale write instead of silently overwriting the current owner. ingestInboundMessage catches it and reports `in-progress` rather than forcing a failed/dead-letter write against a generation it no longer owns. Also closes the retry-budget gap this created: a received-lease reclaim now bumps `attempts` too (a lapsed lease is itself evidence of an abandoned attempt), so a message that hard-crashes the process on every try still converges to dead-letter via MAX_INGEST_ATTEMPTS instead of retrying forever and keeping the mailbox's reconcile cursor wedged behind it. Removes IngestDeps.leaseMs (dead configurability with no consumers — the composition root and tests never set it); DEFAULT_INBOUND_LEASE_MS is used directly. specs/mail/inbound-ingestion.md §4/§8 updated for the fence and the reclaim-counts-toward-budget behavior. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughInbound delivery ingestion now uses expiring claims and attempt-generation fences. Expired ChangesInbound delivery lease recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ingestInboundMessage
participant InboundDeliveryStore
participant Database
ingestInboundMessage->>InboundDeliveryStore: claim(delivery, leaseMs)
InboundDeliveryStore->>Database: atomically claim or reclaim row
Database-->>InboundDeliveryStore: delivery and attempts generation
InboundDeliveryStore-->>ingestInboundMessage: claim result
ingestInboundMessage->>InboundDeliveryStore: markStoredInTx(..., claimedAttempts)
InboundDeliveryStore->>Database: fenced status transition
Database-->>InboundDeliveryStore: stored row or LeaseLostError
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/mail/ingest.ts`:
- Around line 328-344: Update the exhausted reclaim branch in src/mail/ingest.ts
(lines 328-344) to dead-letter the delivery without incrementing attempts,
preserving the already-counted MAX_INGEST_ATTEMPTS value. Update the
corresponding expectation in src/mail/ingest.test.ts (lines 395-426) to assert
MAX_INGEST_ATTEMPTS rather than MAX_INGEST_ATTEMPTS + 1, and retain verifiable
fixture evidence for the mail-semantic completion claim.
In `@src/store/inbound-deliveries.test.ts`:
- Around line 203-221: Update src/store/inbound-deliveries.test.ts lines 203-221
to create the conversation within the same transaction as markStoredInTx, then
assert the stale transaction rolls back without leaving a conversation or
thread. Update src/mail/ingest.test.ts lines 364-381 to pause worker A after
claiming, let worker B expire, reclaim, and commit, then resume A; assert A
returns in-progress, B returns stored, and exactly one conversation/thread
exists.
- Around line 165-184: Update the stale-generation test around store.claim and
the mark* methods so it keeps the delivery status as received while advancing
only its attempts value, rather than calling markFailed. Exercise each mark*
method with the stale delivery.attempts fence and assert LeaseLostError plus
direct-row evidence that no fields were mutated; preserve the fixture-backed
mailbox/delivery setup and verify the expected mail semantics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 43daeb72-f1c8-493f-9ab6-ba41d2132675
📒 Files selected for processing (7)
specs/mail/inbound-ingestion.mdsrc/db/migrate.test.tssrc/db/migrate.tssrc/mail/ingest.test.tssrc/mail/ingest.tssrc/store/inbound-deliveries.test.tssrc/store/inbound-deliveries.ts
Comment on lines
+328
to
+344
| if (delivery.attempts >= MAX_INGEST_ATTEMPTS) { | ||
| const message = | ||
| `lease reclaimed ${delivery.attempts} times without completing ` + | ||
| `(exceeded MAX_INGEST_ATTEMPTS = ${MAX_INGEST_ATTEMPTS})` | ||
| const updated = await deps.inboundDeliveryStore.markDeadLetter( | ||
| delivery.id, | ||
| message, | ||
| delivery.attempts, | ||
| ) | ||
| logIngestEvent({ | ||
| ...base, | ||
| outcome: 'dead-letter', | ||
| stage: 'lease-reclaim-budget', | ||
| attempts: updated.attempts, | ||
| error: message, | ||
| }) | ||
| return { ...base, kind: 'dead-letter', attempts: updated.attempts, error: message } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Keep exhausted reclaim attempts capped at MAX_INGEST_ATTEMPTS.
The reclaim already counts the abandoned attempt. Calling the incrementing markDeadLetter then records a nonexistent additional attempt.
src/mail/ingest.ts#L328-L344: dead-letter the exhausted generation without incrementing its already-counted attempts.src/mail/ingest.test.ts#L395-L426: expectMAX_INGEST_ATTEMPTS, notMAX_INGEST_ATTEMPTS + 1.
As per coding guidelines, mail-semantic completion claims require verifiable fixture evidence. <coding_guidelines>
📍 Affects 2 files
src/mail/ingest.ts#L328-L344(this comment)src/mail/ingest.test.ts#L395-L426
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/mail/ingest.ts` around lines 328 - 344, Update the exhausted reclaim
branch in src/mail/ingest.ts (lines 328-344) to dead-letter the delivery without
incrementing attempts, preserving the already-counted MAX_INGEST_ATTEMPTS value.
Update the corresponding expectation in src/mail/ingest.test.ts (lines 395-426)
to assert MAX_INGEST_ATTEMPTS rather than MAX_INGEST_ATTEMPTS + 1, and retain
verifiable fixture evidence for the mail-semantic completion claim.
Source: Coding guidelines
Comment on lines
+165
to
+184
| it('a mark* write whose claimed-attempts fence no longer matches throws LeaseLostError, and does NOT touch the row', async () => { | ||
| const { db, store, mailboxId } = await freshStore() | ||
| const { delivery } = await store.claim(mailboxId, 'provider-msg-1', LEASE_MS) | ||
|
|
||
| // Simulate a concurrent reclaim having moved the row's generation on | ||
| // (e.g. a received-lease reclaim bumped attempts) — this stale caller's | ||
| // captured fence (0) is no longer current. | ||
| await store.markFailed(delivery.id, 'boom', delivery.attempts) | ||
|
|
||
| await expect( | ||
| store.markSuppressed(delivery.id, 'own-message-loop', delivery.attempts), | ||
| ).rejects.toThrow(LeaseLostError) | ||
|
|
||
| // The row was NOT overwritten by the stale write — read it directly | ||
| // rather than via `claim()`, which would itself reclaim the 'failed' row. | ||
| const row = await db.query<{ status: string }>( | ||
| 'SELECT status FROM inbound_deliveries WHERE id = $1', | ||
| [delivery.id], | ||
| ) | ||
| expect(row[0].status).toBe('failed') |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Exercise the attempts predicate independently of status.
markFailed first changes the row to failed, so the subsequent write is rejected by status = 'received' even if the attempts fence is removed. Keep the row received, advance only attempts, and verify each mark* method rejects the stale generation without mutation.
As per coding guidelines, “Convert vague requests into verifiable success criteria” and require fixture-proven evidence for mail semantics. <coding_guidelines>
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/store/inbound-deliveries.test.ts` around lines 165 - 184, Update the
stale-generation test around store.claim and the mark* methods so it keeps the
delivery status as received while advancing only its attempts value, rather than
calling markFailed. Exercise each mark* method with the stale delivery.attempts
fence and assert LeaseLostError plus direct-row evidence that no fields were
mutated; preserve the fixture-backed mailbox/delivery setup and verify the
expected mail semantics.
Source: Coding guidelines
Comment on lines
+203
to
+221
| const { threadId } = await createConversationStore(db).createConversation({ | ||
| subject: 'Help with my order', | ||
| customerEmail: 'customer@example.test', | ||
| firstMessage: { | ||
| direction: 'inbound', | ||
| messageId: '<cust-1@customer.example.test>', | ||
| fromAddress: 'customer@example.test', | ||
| bodyText: 'Where is my order?', | ||
| }, | ||
| }) | ||
| await expect( | ||
| db.transaction((tx) => | ||
| markStoredInTx(tx, first.delivery.id, threadId, first.delivery.attempts), | ||
| ), | ||
| ).rejects.toThrow(LeaseLostError) | ||
|
|
||
| // B's claim is untouched: still 'received', still holding attempts: 1 — | ||
| // A's stale write did not overwrite it, and no duplicate conversation | ||
| // was left behind (Db.transaction rolled the whole attempt back). |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Verify the actual stale-owner interleaving and rollback.
The current tests do not prove the acceptance scenario where an original worker resumes after another worker reclaimed and committed.
src/store/inbound-deliveries.test.ts#L203-L221: create the conversation inside the same transaction asmarkStoredInTx; currently it commits beforehand and cannot be rolled back. Assert no stale conversation/thread remains.src/mail/ingest.test.ts#L364-L381: orchestrate worker A pausing after its claim, expire/reclaim and commit worker B, then resume A; assert A returnsin-progress, B returnsstored, and exactly one conversation/thread exists.
As per coding guidelines, changes affecting mail semantics require fixture-proven equivalence and verifiable success criteria. <coding_guidelines>
📍 Affects 2 files
src/store/inbound-deliveries.test.ts#L203-L221(this comment)src/mail/ingest.test.ts#L364-L381
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/store/inbound-deliveries.test.ts` around lines 203 - 221, Update
src/store/inbound-deliveries.test.ts lines 203-221 to create the conversation
within the same transaction as markStoredInTx, then assert the stale transaction
rolls back without leaving a conversation or thread. Update
src/mail/ingest.test.ts lines 364-381 to pause worker A after claiming, let
worker B expire, reclaim, and commit, then resume A; assert A returns
in-progress, B returns stored, and exactly one conversation/thread exists.
Source: Coding guidelines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
status = 'received': a hard crash betweenInboundDeliveryStore.claim()committing'received'and the ingest pipeline's step-5 store transaction (or its catch-blockmarkFailed) stranded the row, sinceclaim()only ever reclaimed'failed'rows. Every retry replayed the same stuck'in-progress'outcome, and (via HT-41's cursor coupling) could block the mailbox's reconcile cursor from advancing.inbound_deliveries.claimed_until(migration 014), mirroringthreads.claimed_until(migration 003) andConversationStore's outbound lease/reclaim pattern.claim()now stamps a lease on every successful claim and reclaims a'received'row once its lease has lapsed, via the same row-lockedUPDATEshape already used for the failed-row reclaim.claim()again for the same key — the lease is what makes that call actually reclaim and reprocess instead of reporting'in-progress'forever.attemptsnow doubles as a claim generation: every successful claim returns it, and every ledger write (markStoredInTx/markSuppressed/markFailed/markDeadLetter) fences itsUPDATEonstatus = 'received' AND attempts = $claimedAttempts, throwing a newLeaseLostErroron a stale write instead of silently overwriting the current owner.ingestInboundMessagecatches it and reportsin-progressrather than forcing a failed/dead-letter write against a generation it no longer owns.attemptstoo (a lapsed lease is itself evidence of an abandoned attempt), so a message that hard-crashes the process on every try still converges to dead-letter viaMAX_INGEST_ATTEMPTSinstead of retrying forever.IngestDeps.leaseMs(dead configurability with no consumers — composition root and tests never set it);DEFAULT_INBOUND_LEASE_MSis used directly.specs/mail/inbound-ingestion.md§4/§8 updated for the fence and the reclaim-counts-toward-budget behavior.Design decisions
threads.claimed_until/ConversationStore, not a new mechanism — keeps the concurrency-safety story consistent across the codebase (row-lockedUPDATE, concurrent-reclaim test proves it).'received'rows — relies on existing re-delivery paths (push notification redelivery, Gmail reconcile history replay) callingclaim()again, bounded above by the daily watch-maintenance sweep even with no new mail. This is a deliberate scope call to avoid adding a new background job; flagging for the maintainer's sign-off in case a dedicated sweep is wanted as a belt-and-suspenders backstop.attemptsreused as claim generation for fencing rather than adding a separate generation/version column — smaller schema footprint, but worth the maintainer's sign-off since it overloads a field that also drives the retry-budget/dead-letter threshold (the PR's second commit explicitly reasons through why that overlap is safe: a lapsed lease is itself an abandoned attempt).IngestDeps.leaseMsas dead configurability discovered during this change — flagging in case there was a reason it existed that isn't visible from current call sites.Review
Independent gate: typecheck 0, lint 0, tests 0 (exit codes), clean tree. Adversarial review of record: 5 findings (3 actionable), fixes applied and re-gated.
Verification
Ran in
/Users/tjbaker/Projects/helpthread-worktrees/fix-ht-45-stuck-received-reclaim.git status --porcelainproduced zero lines (tree clean).npm run typecheck(tsc --noEmit -p tsconfig.json) exited 0 with no output/errors.npm run lint(biome check .) exited 0: "Checked 180 files in 81ms. No fixes applied."npm test(vitest run) exited 0 on the full suite: "Test Files 41 passed (41)", "Tests 760 passed".https://resonantiq.atlassian.net/browse/HT-45
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation