fix(mail): retry instead of ack on lease-held reconcile; token-scope lease release (HT-48)

specs/mail/gmail-push.md

@@ -160,16 +160,59 @@ here the cursor itself is unrecoverable.)
   never doubled (inbound-ingestion.md §4). (Cadence is a tuning knob: daily bounds worst-case
   staleness to ~24h for a dropped tail notification; a tighter interval trades quota for
   freshness and can be revisited without changing the design.)
-- **Serialize reconciliation per mailbox — deferred to HT-48.** Push-triggered
-  reconciliation (§2–§3) and this sweep both advance the same mailbox's cursor, so a
-  mailbox's reconciliation runs *should* be serialized by a **reconciliation lease** (the
-  inbound analogue of the outbound delivery lease, sending.md §3a); different mailboxes
-  still reconcile concurrently. This is an efficiency guard, **not** a correctness one —
-  §4 already makes each run's cursor advance independently safe, so a push landing
-  mid-sweep is deduped, never doubled — it only avoids redundant
-  `history.list`/`messages.get` work. Because it is pure optimization and carries a
-  migration, it is **split out of HT-42 into HT-48**: HT-42 ships the renewal cron and the
-  sweep (which are correct without the lease); HT-48 adds the lease.
+- **Reconciliation is serialized per mailbox by a reconciliation lease (HT-48,
+  implemented).** Push-triggered reconciliation (§2–§3) and the daily sweep both advance
+  the same mailbox's cursor, so a mailbox's reconciliation runs are serialized by a
+  **reconciliation lease** — the inbound analogue of the outbound delivery lease
+  (sending.md §3a) — held on `gmail_watch_state.claimed_until` (migration 016,
+  `src/store/gmail-watch-state.ts`'s `claimReconcileLease`/`releaseReconcileLease`);
+  different mailboxes still reconcile concurrently, since the lease is keyed by
+  `mailboxId`. This is an efficiency guard, **not** a correctness one — §4 already makes
+  each run's cursor advance independently safe, so a push landing mid-sweep is deduped,
+  never doubled — it only avoids redundant `history.list`/`messages.get` work.
+
+  The lease lives entirely in the reconcile job's **consumer** (`src/mail/gmail-
+  reconcile.ts`), not in either producer (the push webhook or this sweep): a run claims
+  the lease once it has a confirmed stored cursor and before calling `history.list`; a run
+  that cannot claim it (another holder's lease is still live) does **not** ack — it returns
+  `retry` with a short `backoffSeconds` hint
+  (`DEFAULT_RECONCILE_LEASE_RETRY_BACKOFF_SECONDS`, `src/mail/gmail-reconcile.ts`) and does
+  no Gmail work of its own that attempt. Acking on a failed claim (an earlier version of
+  this handler's behavior) is unsafe: the holder's `history.list` snapshot is fixed the
+  moment it runs, so a message that arrives in Gmail's history *after* that snapshot is
+  invisible to the holder's own cursor advance — acking the notification for it would drop
+  it on the floor until the next trigger (a further push, or the next daily sweep), up to
+  ~24h of added latency on an otherwise-quiet mailbox. Retrying instead means the same job
+  is redelivered shortly after the holder has very likely released, at which point its own
+  `history.list` (from the cursor the holder just advanced to) picks up anything the holder
+  missed — trivially and cheaply in the common case where nothing new arrived. The backoff
+  is sized so that, combined with the queue's own exponential backoff and `maxAttempts`
+  dead-letter ceiling (`src/providers/adapters/postgres-queue/index.ts`), a claim that keeps
+  losing the race still gets an attempt after the holder is *guaranteed* to have released
+  (its lease cannot outlive `reconcileLeaseMs`) before the job is given up on — see that
+  constant's own doc comment for the arithmetic. Even in the pathological case where the job
+  is eventually dead-lettered, no message is lost: cursor-advance and ingest dedup mean the
+  next trigger reconciles the mailbox from wherever the holder left the cursor, exactly as
+  it would have before this lease existed. The lease is released in a `finally` around the
+  `history.list`/fetch/ingest/cursor-advance block, so it is released on every exit — the
+  happy-path ack, the expired-cursor pause, the blocked-retry, and an unexpected thrown error
+  alike — *before* that error propagates to the handler's own top-level catch. This was a
+  deliberate choice: because the lease is a pure efficiency guard, the one failure mode it
+  must never produce is a mailbox permanently (or even needlessly long) locked out of
+  reconciliation after a crash; releasing on every path, including a throw, means the next
+  trigger can reconcile the mailbox immediately rather than waiting out the lease's duration.
+  The lease's own expiry remains as a backstop for the one case a `finally` cannot reach —
+  the process being killed outright before it runs.
+
+  The release itself is scoped to the exact lease this run was granted: `claimReconcileLease`
+  returns an opaque token (the `claimed_until` value it just wrote) that must be passed back
+  to `releaseReconcileLease`, which clears the lease only if that token still matches the
+  row's current `claimed_until` — otherwise it is a silent no-op (`src/store/gmail-watch-
+  state.ts`). This guards against a stale holder (one that overran `reconcileLeaseMs`, e.g. a
+  large post-downtime backlog) releasing a legitimate successor's live lease out from under
+  it, which would otherwise let a third trigger claim and duplicate the successor's in-flight
+  `history.list`/`messages.get` work — precisely the case an unconditional release fails in,
+  and precisely the load under which that redundant work is most expensive.
 - **Failure handling — the token layer owns `needs_reconnect`.** A dead grant
   (revoked/expired, admin change) surfaces as an `invalid_grant` when the OAuth token
   service refreshes, and *that* is what marks the mailbox **needs-reconnect** (HT-38,

src/db/migrate.test.ts

@@ -55,6 +55,7 @@ describe('migrate', () => {
       { id: 13, name: 'queue_jobs' },
       { id: 14, name: 'inbound_delivery_lease' },
       { id: 15, name: 'thread_attachments' },
+      { id: 16, name: 'gmail_reconcile_lease' },
     ])
   })
 
@@ -80,6 +81,7 @@ describe('migrate', () => {
       { id: 13 },
       { id: 14 },
       { id: 15 },
+      { id: 16 },
     ])
   })
 

src/db/migrate.ts

@@ -754,6 +754,39 @@ CREATE TABLE thread_attachments (
 CREATE INDEX thread_attachments_thread_id_idx ON thread_attachments (thread_id);
 `
 
+/**
+ * Migration 016 — the per-mailbox Gmail reconciliation lease (HT-48;
+ * specs/mail/gmail-push.md §6, "reconciliation lease → HT-48"). Adds
+ * `claimed_until` to `gmail_watch_state` (migration 011) — the inbound
+ * analogue of migration 003's `threads.claimed_until` outbound delivery
+ * lease, same column name and same `UPDATE ... WHERE claimed_until IS NULL
+ * OR claimed_until < now()` claim shape (`GmailWatchStateStore
+ * .claimReconcileLease`/`.releaseReconcileLease`, `src/store/gmail-watch-
+ * state.ts`).
+ *
+ * Unlike the outbound lease, there is no accompanying "status" to record on
+ * release — this lease guards nothing but redundant Gmail API work
+ * (`history.list`/`messages.get`) between a push-triggered reconcile
+ * (HT-41) and the daily sweep (HT-42) landing on the SAME mailbox at
+ * overlapping times. It is a pure efficiency guard, not a correctness one:
+ * `src/mail/gmail-reconcile.ts`'s own cursor-advance rule (step 6) and the
+ * ingest pipeline's dedup on `(mailboxId, providerMessageId)`
+ * (inbound-ingestion.md §4) already make either ordering safe with no lease
+ * at all. A run that cannot claim it retries shortly (a short
+ * `backoffSeconds` hint, not an ack) rather than skipping outright — see
+ * `src/mail/gmail-reconcile.ts`'s module doc ("Why a failed claim retries
+ * instead of acking") for why an unconditional skip can silently drop a
+ * message that arrives after the holder's own `history.list` snapshot.
+ *
+ * No `NOT NULL`/CHECK: `NULL` is "unclaimed," matching `threads.claimed_
+ * until`'s own nullability. No index: this column is only ever read via an
+ * equality match on the `mailbox_id` PRIMARY KEY (migration 011), which
+ * already has its own index.
+ */
+const MIGRATION_016_GMAIL_RECONCILE_LEASE = `
+ALTER TABLE gmail_watch_state ADD COLUMN claimed_until timestamptz;
+`
+
 /**
  * Every migration, in the order they must apply. `id` is the sole ordering
  * key (ascending) — array position is not relied upon, so re-sorting this
@@ -831,6 +864,11 @@ const MIGRATIONS: Migration[] = [
     name: 'thread_attachments',
     sql: MIGRATION_015_THREAD_ATTACHMENTS,
   },
+  {
+    id: 16,
+    name: 'gmail_reconcile_lease',
+    sql: MIGRATION_016_GMAIL_RECONCILE_LEASE,
+  },
 ]
 
 /**

src/mail/gmail-connect.test.ts

@@ -490,6 +490,12 @@ describe('createGmailConnectService', () => {
       setWatchExpiration: async () => {
         throw new Error('setWatchExpiration: not used by the connect flow')
       },
+      claimReconcileLease: async () => {
+        throw new Error('claimReconcileLease: not used by the connect flow')
+      },
+      releaseReconcileLease: async () => {
+        throw new Error('releaseReconcileLease: not used by the connect flow')
+      },
     }
     const { fetchImpl } = fakeTokenEndpoint(200, DEFAULT_TOKEN_RESPONSE)
     const service = createGmailConnectService({