delivery claim fencing

docs/architecture/concurrency.md

@@ -1,7 +1,8 @@
 # Concurrency
 
-Four places in this service have a race, and all four are settled by the database
-rather than by a check in Python.
+Four places in this service have a race, and every one of them is settled by the
+database rather than by a check in Python. The first has two halves: taking a
+delivery, and recording what happened to it.
 
 The pattern is the same every time: **read, compare in application code, then
 write** is never used, because two requests can both read, both find room, and
@@ -32,6 +33,35 @@ keeps the claim correct there.
 work and never share it; a locked row is skipped rather than waited on; an
 expired lease is reclaimed by exactly one of two racing workers.
 
+### Recording a result under a claim that was superseded
+
+Claiming and recording are separated by an HTTP request to somebody else's
+server, which can take longer than the lease. So the second half of the claim has
+a race of its own: worker A's lease expires, worker B legitimately reclaims the
+delivery, and *then* A's request comes back.
+
+Each claim mints a `claim_token`, and the transition that records a result is
+conditional on the row still carrying it:
+
+```sql
+UPDATE webhook_deliveries
+SET state = ..., cycle_attempts = ..., claim_expires_at = NULL, claim_token = NULL
+WHERE id = :id AND claim_token = :token
+```
+
+The lease expiry cannot do this job. It says when a claim ends, not which claim it
+is, and A is still looking at a `processing` row, so neither the state nor the
+lease tells A that anything changed. A's update matches no row, and B's state
+stands.
+
+The request A made is still recorded, taking the next free lifetime attempt
+number read under a row lock. See
+[Delivery semantics](delivery-semantics.md#what-happens-to-a-late-attempt).
+
+**Tested against real PostgreSQL** with two connections holding two different
+claims on one row, in both result orderings, so what the fence keys on is
+ownership rather than whether the late worker happened to succeed.
+
 ## 2. Two retries with the same idempotency key
 
 A unique constraint on `(endpoint_id, idempotency_key)` is the authority.

docs/architecture/delivery-semantics.md

@@ -23,6 +23,49 @@ A delivery is claimable when it is `pending` and due, **or** when it is
 `processing` and its lease has expired. The second case is the whole point: a
 worker that dies holding a job does not strand it in `processing` forever.
 
+## Who owns a claim
+
+A lease says *when* a claim ends. It does not say *which* claim the row is
+holding, and that is a different question with a different answer.
+
+Every claim mints a fresh `claim_token` onto the delivery. A worker records its
+result against the token it claimed under, and that update is conditional on the
+row still carrying it:
+
+```sql
+UPDATE webhook_deliveries
+SET state = ..., cycle_attempts = ..., claim_expires_at = NULL, claim_token = NULL
+WHERE id = :id AND claim_token = :the_token_this_request_was_sent_under
+```
+
+This matters because a worker whose lease ran out mid-request has no way of
+noticing on its own. It is still looking at a row that says `processing`, so
+neither the state nor the lease tells it anything. The token does: once another
+worker has reclaimed the delivery, the token no longer matches, and the late
+worker's transition matches no row.
+
+So a superseded worker **cannot**:
+
+- clear or shorten the current owner's lease
+- move the delivery to `delivered`, `failed` or `pending`
+- spend an attempt from the current owner's retry cycle
+- move `next_attempt_at`
+
+What it still does is record the request it genuinely made. See
+[what happens to a late attempt](#what-happens-to-a-late-attempt) below.
+
+**Tested against real PostgreSQL** with independent connections holding two
+different claims on one row. See [Testing](../development/testing.md).
+
+!!! warning "Every worker has to be on the same build"
+
+    This holds only while every running worker maintains the token. A worker from
+    a build older than the `claim_token` migration neither writes it when it
+    claims nor checks it when it completes, so one left running alongside a newer
+    worker defeats the fence. Stop the old workers, migrate, then start the new
+    ones, which is the [deploy order](../operations/worker.md#deploy-order) this
+    project documents anyway.
+
 ## The crash window
 
 Here is the sequence that produces a duplicate:
@@ -42,6 +85,11 @@ Here is the sequence that produces a duplicate:
 
 No queue closes this on its own. It needs the receiver's cooperation.
 
+Note what the claim token does and does not change here. It stops a late worker
+from overwriting a newer worker's state. It does **not** stop the request from
+having been sent twice: by the time ownership is checked, both requests have
+already left. Your receiver still has to deduplicate.
+
 ## What you should do about it
 
 **Deduplicate on the submission `id` in the signed payload.**
@@ -88,8 +136,25 @@ ever being reused, however often a delivery is replayed.
 resets it to zero. That is what lets a replay grant a whole fresh schedule rather
 than one last attempt against an allowance that is already spent.
 
-Until something is replayed the two are the same number, which is why the
-migration that introduced the second one backfilled it from the first.
+The two numbers start out equal, which is why the migration that introduced the
+second one backfilled it from the first. They diverge for one of two reasons: the
+delivery was replayed, or a worker that had lost its claim recorded a request it
+really made. Only the current owner may draw on the retry allowance, so a late
+attempt raises `attempts` and leaves `cycle_attempts` alone.
+
+## What happens to a late attempt
+
+A worker whose claim was superseded has usually already made a real HTTP request.
+Somebody's server received it. Discarding that would make the attempt history
+claim fewer requests went out than actually did, so it is recorded:
+
+- it takes the **next free lifetime attempt number**, read from the stored row
+  rather than from whatever the worker held in memory, so no number is reused
+- it raises `attempts`, because the delivery really did produce that request
+- it changes **nothing else**
+
+The number is taken under a row lock, so a late worker and the current owner
+recording at the same instant still get two different numbers.
 
 ## Terminal is terminal, until an operator says otherwise
 

docs/development/testing.md

@@ -61,6 +61,12 @@ database of its own, so migrating from genuinely nothing is what is being tested
 never share it. A row another worker holds is skipped rather than waited on. An
 expired lease is reclaimed by exactly one of two racing workers.
 
+**Completion ownership.** Two connections hold two different claims on one
+delivery: a worker whose lease has lapsed, and the worker that legitimately
+reclaimed it. The lapsed worker cannot clear the newer lease, move the state,
+spend the retry cycle or reuse an attempt number, in either result ordering. The
+request it really made is still recorded, under a number of its own.
+
 **Manual replay.** Several real connections replay one failed delivery at the same
 instant, and exactly one of them wins.
 

docs/operations/migrations.md

@@ -71,6 +71,8 @@ the DDL before anything touches it.
 
 Migrations through `0004` replay against SQLite too, in batch mode, because
 SQLite cannot `ALTER` a column in place and has to rebuild the table instead.
+`0006` is a plain `ADD COLUMN`, which SQLite performs happily, but it sits behind
+`0005` and so is unreachable there for the reason below.
 
 !!! warning "A fresh SQLite database cannot reach `head`"
 

docs/operations/worker.md

@@ -58,6 +58,12 @@ There is no coordination service and no broker. PostgreSQL is the queue. See
     The defaults leave a wide margin (60 seconds against 15). Keep it that way if
     you change them.
 
+    A lease that runs out mid-request costs you a duplicate send, not a corrupted
+    delivery: the overtaken worker cannot overwrite the state of whoever reclaimed
+    the delivery. It logs a warning saying so, which is the signal that the lease
+    is too short for how long that destination takes to answer. See
+    [Delivery semantics](../architecture/delivery-semantics.md#who-owns-a-claim).
+
 ## Startup checks
 
 The worker performs the same schema check the API does. It reaches the database,

docs/reference/limitations.md

@@ -18,7 +18,9 @@ deploy it, and it is kept complete rather than flattering.
   than the sum. But if `FORMS_WORKER_LEASE_SECONDS` were set below the connect and
   read timeouts combined, another worker could claim a delivery that is still in
   flight and send it twice. The defaults leave a wide margin; keep it that way if
-  you change them.
+  you change them. The overtaken worker cannot overwrite the newer worker's state,
+  and logs a warning when it finds it has lost the claim, but the duplicate
+  request has already gone out by then.
 - **A signing secret cannot be rotated in place.** Rotation happens only as a side
   effect of changing the destination, so re-keying a receiver that stays at the
   same URL means pointing the endpoint elsewhere and back, or standing up a

src/hymical_forms/migrations/versions/0006_20260826_delivery_claim_tokens.py

@@ -0,0 +1,77 @@
+"""
+delivery claim tokens
+
+A worker claims a delivery, sends it, and then records what happened. Nothing
+made those last two steps agree about who owned the delivery: a worker whose
+lease expired mid-request was still holding a row that said ``processing``, and
+its completion overwrote the state, the lease and the counters belonging to the
+worker that had legitimately reclaimed it.
+
+This adds one nullable column, ``claim_token``, holding the identity of the
+claim the row is currently under. A fresh value is written every time the
+delivery is claimed and cleared when it stops being claimed, so a completion can
+be fenced on the exact claim its request was made under. The lease expiry cannot
+do that job: it says when a claim ends, not which claim it is.
+
+Nothing is backfilled. NULL means "no claim is held", which is true of every
+delivery that is not ``processing`` and of every row this column is added to.
+
+**Every worker must be on the new build before any of them runs against this
+schema.** Applying the migration does not fence a worker that predates it. A
+pre-0006 worker neither writes the column when it claims nor consults it when it
+completes, so running one alongside a new worker breaks the guarantee in both
+directions: its own completion still overwrites a newer worker's state exactly
+as before, and because its claim leaves whatever token was already on the row in
+place, a superseded new worker can present that stale token, match, and overwrite
+the old worker's live claim. That is the deploy order this project already
+documents (stop the old processes, migrate, start the new ones), and this
+revision is one of the migrations for which it is load-bearing rather than
+merely tidy.
+
+The downgrade drops the column. A build without it fences on nothing, which is
+the behaviour this revision replaced.
+
+Nothing in this revision imports application code, for the reason given in
+``0001``: a migration is a frozen record of a change, not a view of the current
+models.
+
+revision: 0006
+revises: 0005
+created: 2026-08-26
+"""
+
+from __future__ import annotations
+
+from collections.abc import Sequence
+
+import sqlalchemy as sa
+from alembic import op
+
+revision: str = "0006"
+down_revision: str | None = "0005"
+branch_labels: str | Sequence[str] | None = None
+depends_on: str | Sequence[str] | None = None
+
+
+def upgrade() -> None:
+    """
+    add the column naming the claim a delivery is currently held under
+    """
+    # Nullable, and stays nullable: the absence of a token is meaningful rather
+    # than a gap to be filled, so there is nothing to backfill and no ALTER to
+    # tighten afterwards. A plain ADD COLUMN replays under SQLite as well as
+    # PostgreSQL, though a fresh SQLite database still cannot reach this revision
+    # for the reason recorded in ``0005``.
+    op.add_column(
+        "webhook_deliveries", sa.Column("claim_token", sa.String(length=36), nullable=True)
+    )
+
+
+def downgrade() -> None:
+    """
+    remove the claim token column
+    """
+    # A delivery that is ``processing`` when this runs keeps its lease and is
+    # recovered the ordinary way once that lease expires. The older build simply
+    # has no fence, which is the weakness ``0006`` exists to close.
+    op.drop_column("webhook_deliveries", "claim_token")

src/hymical_forms/models.py

@@ -37,6 +37,7 @@
 from hymical_forms.ingestion import Submission as DomainSubmission
 from hymical_forms.ratelimit import LIMITER_MAX_LENGTH, SUBJECT_MAX_LENGTH
 from hymical_forms.webhooks import (
+    CLAIM_TOKEN_MAX_LENGTH,
     DELIVERY_ATTEMPT_ID_MAX_LENGTH,
     DELIVERY_ERROR_MAX_LENGTH,
     DELIVERY_STATE_MAX_LENGTH,
@@ -331,6 +332,18 @@ class WebhookDelivery(Base):
     # leaving a permanent ``processing`` tombstone.
     claim_expires_at: Mapped[datetime | None] = mapped_column(UtcDateTime, default=None)
 
+    # Which claim the row is currently holding. A fresh value is minted every time
+    # the delivery is claimed and cleared the moment it stops being claimed, so it
+    # says *which* claim rather than when one ends.
+    #
+    # That distinction is the whole reason it exists. A worker whose lease ran out
+    # while its request was still in flight is still looking at a ``processing``
+    # row, so neither the state nor the lease tells it that the delivery has been
+    # taken over. The token does: it presents the one it claimed under, no longer
+    # matches, and its completion is refused rather than overwriting the newer
+    # owner's state.
+    claim_token: Mapped[str | None] = mapped_column(String(CLAIM_TOKEN_MAX_LENGTH), default=None)
+
     created_at: Mapped[datetime] = mapped_column(UtcDateTime, default=utcnow)
     completed_at: Mapped[datetime | None] = mapped_column(UtcDateTime, default=None)