feat(mail): send idempotency + delivery worker (HT-16)

specs/api/agent-inbox-v1.md

@@ -1,9 +1,10 @@
 # Agent Inbox API v1
 
-Status: accepted (HT-17 reads, HT-18 writes). Helpthread's first public API, designed
-**native** — on Helpthread's own domain model, not reverse-engineered from any other
-helpdesk's wire format. (It supersedes the earlier `conversations-v1.md` draft, which was
-shaped for a FreeScout-consumer cutover that no longer applies — see the project history.)
+Status: accepted (HT-17 reads, HT-18 writes, HT-16 send idempotency). Helpthread's first
+public API, designed **native** — on Helpthread's own domain model, not reverse-engineered
+from any other helpdesk's wire format. (It supersedes the earlier `conversations-v1.md`
+draft, which was shaped for a FreeScout-consumer cutover that no longer applies — see the
+project history.)
 
 ## 1. Purpose
 
@@ -73,11 +74,13 @@ appears, not preemptively.
   interface ApiError { error: { code: string; message: string } }
   ```
   `code` is a machine-readable slug (`unauthorized`, `not_found`, `validation_failed`,
-  `method_not_allowed`, `send_failed`, `server_error`); `message` is user-safe and MUST
-  NEVER contain an internal detail — no stack, no SQL, no upstream body, no id it wasn't
-  given. HTTP status pairs with `code`: 400 `validation_failed`, 401 `unauthorized`, 404
-  `not_found`, 405 `method_not_allowed`, 500 `server_error`, 502 `send_failed` (§4a, the
-  provider rejected an outbound reply).
+  `method_not_allowed`, `send_failed`, `retry_in_progress`, `server_error`); `message` is
+  user-safe and MUST NEVER contain an internal detail — no stack, no SQL, no upstream body,
+  no id it wasn't given. HTTP status pairs with `code`: 400 `validation_failed`, 401
+  `unauthorized`, 404 `not_found`, 405 `method_not_allowed`, 409 `retry_in_progress` (§4a,
+  HT-16 — a concurrent delivery attempt for the same `Idempotency-Key` already holds the
+  lease), 500 `server_error`, 502 `send_failed` (§4a, the provider rejected an outbound
+  reply).
 - **Unknown routes / methods:** an unmatched path is `404 not_found`; a known path with an
   unsupported method is `405` (with an `Allow` header). Both still require auth first — an
   unauthenticated request gets `401` before routing details leak.
@@ -116,6 +119,18 @@ is indistinguishable from a nonexistent one to this API, on purpose).
 
 ### 4a. `POST /api/v1/conversations/{id}/replies` — the Agent replies
 
+**Header:** `Idempotency-Key` is **REQUIRED** on every call (HT-16) — a non-empty,
+caller-chosen string, scoped per-conversation. This is a deliberate breaking change from
+the HT-15 shape of this endpoint; it has no external consumer yet (this API is
+dogfood-only — CHARTER.md "dogfooded first"), so tightening the contract here has no
+compatibility cost. The header is **trimmed of leading/trailing whitespace before any
+other check**, so `" key "` and `"key"` are the same idempotency key — a caller whose
+client or proxy adds incidental whitespace does not silently get a second send. The
+**trimmed** value is what is validated, stored, and passed through to `sendReply`: it
+must be non-empty and **at most 255 characters** after trimming. A missing header, a
+header that is empty (or all whitespace) after trimming, or a trimmed value over 255
+characters is `400 validation_failed`, checked before the body is parsed.
+
 Body: `{ text: string; html?: string }` — `text` 1–5000 chars, server-enforced; `html`
 optional. The Agent supplies only the message; every mail header is DERIVED server-side
 from the conversation, so the client never sets recipients or threading headers:
@@ -131,26 +146,53 @@ from the conversation, so the client never sets recipients or threading headers:
   them (it is outbound-token-anchored; threading.md §2). Omitted when no prior message-id
   exists (e.g. an inbound message that arrived without a `Message-ID`).
 
-The handler then calls `sendReply` (`src/mail/send.ts`), which mints the reply token into
-the outbound `Message-ID`, persists the outbound thread (`delivery_status` `pending`→`sent`),
-and sends via the injected `EmailSender`.
+The handler then calls `sendReply` (`src/mail/send.ts`), passing the `Idempotency-Key` value
+through. `sendReply` mints the reply token into the outbound `Message-ID` (on a genuinely
+new send), persists the outbound thread with a snapshot of its envelope
+(`send_envelope`: `to`/`cc`/`subject`/`references`, `sending.md` §3a), and sends via the
+injected `EmailSender`.
+
+**Replay semantics: same key + same conversation = same logical send, never re-diffed
+against the body.** If a call reuses a key already recorded against this conversation, the
+NEW request's body is irrelevant — the response reflects the ORIGINAL attempt's outcome:
+
+- If the original attempt already succeeded (`delivery_status: 'sent'`), this call returns
+  `201` with that SAME `ThreadView` again, WITHOUT invoking the sender a second time.
+- If the original attempt is `pending`/`failed`, this call attempts delivery using the
+  ORIGINAL row's stored `messageId` and `send_envelope` — never the replay call's own
+  `to`/`subject`/`references`, even if they differ (sending.md §3a's snapshot rule) — after
+  first claiming that row's delivery lease.
+- If the lease could not be claimed (another attempt — a concurrent replay, or the delivery
+  worker, sending.md §3a — currently holds it), this call sends nothing and returns
+  `409 retry_in_progress`.
 
 Outcomes:
-- **`201`** with the created `ThreadView` on success. A reply to a `closed` conversation
-  **reopens** it (the store's existing append policy).
+- **`201`** with the created (or, on a replay after success, the ORIGINAL) `ThreadView`. A
+  reply to a `closed` conversation **reopens** it (the store's existing append policy) —
+  only on the call that actually creates the row, not on a replay.
+- **`400 validation_failed`** on a missing/empty `Idempotency-Key` header, or a body that
+  violates the limits.
 - **`404 not_found`** if the conversation is missing or `deleted` — no message is sent; a
-  reply token minted before the append resolves is simply discarded (mirrors §3b).
-- **`400 validation_failed`** on a body that violates the limits.
+  reply token minted before the append resolves is simply discarded (mirrors §3b). This
+  applies even to a KEYED REPLAY of a key whose original attempt already succeeded: if the
+  conversation has since been deleted, the replay call returns `404`, not the original
+  `201`. Replay-of-original-outcome does not survive a conversation delete — there is no
+  mail-safety impact, since the original send already happened regardless of what a later
+  replay call observes.
+- **`409 retry_in_progress`** (HT-16) — the delivery lease for this `Idempotency-Key` is
+  currently held by another in-flight attempt; nothing was sent by this call. The caller
+  should retry the SAME key later, not mint a new one (a new key would create an
+  independent send, defeating the point of the dedup key).
 - **`502 send_failed`** if the provider rejects the message — nothing was delivered.
   `sendReply` returns a `send-failed` result (it does not throw): the outbound thread is
-  left `delivery_status = 'failed'` (a future delivery worker, HT-16, retries it with the
-  same Message-ID) — or, if even that mark fails, stuck `pending`. The response therefore
-  says only that the reply *could not be delivered* — never a specific persisted state,
-  never a raw provider error. This is the one outcome where an undelivered reply is
-  surfaced to the caller distinctly from an internal error. (Note the asymmetry: once the
-  provider ACCEPTS the message it is delivered, so a subsequent failure to record `'sent'`
-  is NOT a `send_failed` — it resolves to `201`, since reporting a delivered message as
-  failed would invite a resend.)
+  left `delivery_status = 'failed'` (retryable — by a replay with the same key, or the
+  delivery worker's sweep, sending.md §3a — with the same Message-ID) — or, if even that
+  mark fails, stuck `pending`. The response therefore says only that the reply *could not
+  be delivered* — never a specific persisted state, never a raw provider error. This is the
+  one outcome where an undelivered reply is surfaced to the caller distinctly from an
+  internal error. (Note the asymmetry: once the provider ACCEPTS the message it is
+  delivered, so a subsequent failure to record `'sent'` is NOT a `send_failed` — it resolves
+  to `201`, since reporting a delivered message as failed would invite a resend.)
 
 ### 4b. `PATCH /api/v1/conversations/{id}` — close or reopen
 

specs/mail/sending.md

@@ -1,7 +1,7 @@
 # Outbound sending & the reply-token lifecycle
 
-Status: accepted (HT-15). Companion to [threading.md](./threading.md) — that spec
-decides which conversation an *inbound* message joins; this one covers how an
+Status: accepted (HT-15, HT-16). Companion to [threading.md](./threading.md) — that
+spec decides which conversation an *inbound* message joins; this one covers how an
 *outbound* reply is minted, persisted, and sent, and is where the threading
 model's authority actually originates.
 
@@ -66,6 +66,89 @@ threading handles for one logical message and risk double-sends. The stable
 `Message-ID` is the idempotency anchor: a provider that de-dupes on `Message-ID`
 will not double-deliver a retried send.
 
+## 3a. Send idempotency + delivery leasing (HT-16)
+
+§3's "retries reuse, never re-mint" rule describes what a retry must DO once
+one is recognized; this section is how a retry gets recognized and kept safe
+under concurrency, closing the increment §5 of the HT-15 version of this spec
+left open.
+
+**Caller-supplied idempotency key, scoped per conversation.** A caller that
+needs at-most-once delivery (the Agent Inbox API, `agent-inbox-v1.md` §4a)
+supplies an `idempotencyKey` alongside the reply. `src/store/conversations.ts`'s
+`appendThread` resolves it as an atomic **get-or-insert**: `INSERT ... ON
+CONFLICT (conversation_id, idempotency_key) WHERE idempotency_key IS NOT NULL
+DO NOTHING RETURNING *`, falling back to a `SELECT` of the pre-existing row on
+conflict — inside the same transaction that holds the conversation row's `FOR
+UPDATE` lock, so two callers racing with the identical key on the identical
+conversation are serialized rather than double-inserting. Omitting the key is
+still legal and unchanged from HT-15: a fresh send every call, no dedup
+protection — a deliberate, permanently-tested contract for callers that don't
+need it.
+
+**The envelope is a snapshot, never a recomputation.** Every outbound send
+(keyed or not) now persists a `send_envelope` — `{ to, cc?, subject,
+references? }` — verbatim at insert. A retry (whether replayed by the
+original caller with the same key, or picked up by the delivery worker below)
+resends EXACTLY that stored envelope, never re-derives `to`/`subject`/
+`references` from the conversation's current thread list. This matters
+because time passes between an attempt and its retry, and inbound mail can
+arrive in that gap: recomputing `References` at retry time could silently
+absorb a message that wasn't part of the original send, changing what goes
+out without anyone deciding it should (CHARTER.md invariant #5). The
+persisted snapshot makes a retry byte-identical to the attempt it retries, by
+construction.
+
+**A lease keeps at most one attempt in flight per row.** Before either a
+keyed retry or the delivery worker sends a `pending`/`failed` row, it must
+first claim the row's delivery lease (`claimThreadForDelivery`: an atomic
+`UPDATE ... WHERE claimed_until IS NULL OR claimed_until < now()`). A failed
+claim means someone else already holds it; the caller does not send and
+reports back accordingly rather than retrying the claim itself. A successful
+attempt releases the lease as it marks `sent`/`failed`. This is what makes
+"exactly one send in flight per row" hold even when a caller retries the
+same key concurrently with the delivery worker sweeping the same row —
+**but only if the lease strictly outlives the send it is protecting.** The
+lease duration (`DEFAULT_LEASE_MS`, `src/mail/send.ts`) MUST strictly exceed
+the worst-case duration of the configured `EmailSender`'s `send()` call; a
+send that outlives its own lease can be re-claimed and retried by another
+attempt while the original call is still in flight — a genuine concurrent
+double-send, not merely a race over which of two callers marks the outcome.
+Every `EmailSender` used behind these retry paths must therefore bound its
+own call time well below this lease (see §4).
+
+**Delivery is at-least-once, not at-most-once — and nothing above changes
+that.** The idempotency key, the envelope snapshot, and the lease all close
+off *spurious* re-sends — a retry racing another retry, or a caller
+deliberately replaying — but none of them lets the engine observe what the
+provider actually did with a send it already accepted. The residual case
+(§3's "sent but unmarked" asymmetry, sharpened): the provider accepts the
+message — the customer's mailbox already has it — and then the write that
+marks the row `'sent'` fails, so the row remains `pending` with a live,
+already-delivered envelope on it. If nothing revisits that row for a while,
+it goes stale; once it is stale (and its lease has freed), the delivery
+worker's sweep or a keyed replay's claim will find it eligible and re-send
+an already-delivered message. The engine has no way to distinguish "crashed
+before the provider was ever called" from "the provider was called and
+succeeded, but the mark-sent write failed" — both leave the identical
+stale `pending` row with a stored envelope, and both are, correctly,
+retried. So: **at-least-once is the actual guarantee this system provides.
+At-most-once is not something the engine can produce on its own — it holds
+only to the extent the `EmailSender` provider de-duplicates on the outbound
+`Message-ID`** (§4).
+
+**The delivery worker (`src/mail/delivery-worker.ts`) is a plain, invocable
+sweep function** — `runDeliveryWorker(deps, options?)` — not built on a
+queue or scheduler provider (no such adapter exists yet; see §5). One call
+selects a bounded batch of eligible rows (`delivery_status = 'failed'`, or
+`'pending'` older than a staleness threshold, with a free lease and a stored
+envelope — pre-HT-16 rows with no envelope are left for manual handling
+rather than guessed at), claims each in turn, and retries it via the exact
+same "rebuild `OutboundEmail` from the row, send, mark" helper a keyed
+`sendReply` retry uses. Wiring a real schedule around it (Vercel Cron, or a
+future `SchedulerProvider` adapter) is deferred — at that point it is a
+one-line call to this function, not a rewrite of it.
+
 ## 4. What a sender provider must guarantee
 
 The `EmailSender` provider (`src/providers/`) is handed a fully-formed outbound
@@ -85,19 +168,49 @@ accept raw MIME; reject any that will not carry `Message-ID` unaltered. The
 in-repo fake used by the engine tests proves only that `sendReply` *passes* the
 value to the seam — not that any given adapter preserves it on the wire.
 
-## 5. Scope of the first increment (HT-15)
+**Recommended: a provider SHOULD de-duplicate on `Message-ID` (HT-16).** This
+is not a precondition the engine requires — at-least-once delivery (§3a) holds
+with or without it — but it is not an aside either: it is the one thing
+standing between this system's structural at-least-once delivery (§3a) and
+true at-most-once delivery from the operator's point of view. Where a
+provider does not de-duplicate on the `Message-ID` it is handed verbatim,
+the operator is knowingly accepting
+at-least-once delivery: the residual "accepted, then unmarked, then
+re-sent" case (§3a) will occasionally reach the customer's mailbox twice,
+identical down to the `Message-ID`, and nothing in the engine can prevent
+that without provider-side dedup. A provider adapter's wire-level contract
+test (above) should note whether the provider is known to de-dupe, so this
+gap is a documented, deliberate property of a given deployment rather than
+a surprise discovered in production.
+
+**A lease that outlives the provider's `send()` call is a precondition
+too.** §3a's lease only holds "at most one attempt in flight per row" if the
+provider's `send()` reliably returns well inside the lease window — an
+adapter whose HTTP call has no timeout (or one comparable to or longer than
+the lease) can outlive its own claim and collide with a re-claimed retry.
+See each adapter's own timeout documentation for its bound.
+
+## 5. Scope
 
 Deliberately narrow; each deferral below has a named later home:
 
-- **Synchronous send only** — the persist→send→mark flow runs inline. No queue
-  or retry worker yet; the `failed` status plus the stable id/`Message-ID` are
-  the seam a later delivery worker (queue provider, already interfaced) picks up.
+- **Synchronous send only** — the persist→send→mark flow runs inline within
+  one `sendReply` call. Retrying a stuck row is now covered (§3a: a keyed
+  replay, or the delivery worker's sweep) — what's still deferred is wiring a
+  real *schedule* around that sweep (Vercel Cron, or a future
+  `SchedulerProvider` adapter, CHARTER.md §4) — today it is only invoked
+  directly (e.g. from a test or a manual trigger), never on a timer.
 - **Reply to an existing conversation only.** Agent-*initiated* brand-new
   conversations are a separate later flow.
 - **`In-Reply-To`/`References` are caller-supplied** (from the inbound message
   being answered). Deriving the full `References` chain from stored threads is a
-  later refinement.
+  later refinement. Once persisted into `send_envelope` (§3a) that snapshot is
+  authoritative for every retry regardless of how it was originally derived.
 - **A missing or deleted conversation is refused** — the token is minted first
   (before `appendThread` resolves) and then discarded on refusal; only
   persistence and sending are skipped, and the sender is never called (mirrors
   the store's `appendThread` policy; threading.md §5).
+- **No cross-conversation or cross-Agent idempotency-key reuse policy.** A key
+  is scoped to one conversation (§3a); reusing the same string across
+  different conversations is unrelated and creates independent rows, by
+  design — there is no global key registry.