Found by the review of #297. Pre-existing — a deterministic probe returns posts=2 identically on next and on that PR's head, so it predates the branch and is not caused by it.
Startup recoverPending runs in a goroutine concurrently with the receiver accepting traffic. A delivery row written just before Notify can be selected by recovery, claimed, delivered, and RELEASED — after which Notify's retainIdle succeeds on the now-free id and dispatches the same delivery again. The target receives two POSTs.
The window is the startup interval during which recovery is still scanning and the receiver is already serving. It is narrow, and the reviewer found it only by building a deterministic probe after a sloppy harness surfaced it as an intermittent 901-for-900.
Why this is not tag-blocking
Delivery is at-least-once by design and now says so in the README (see the Rationale section, and #256 for why a bookkeeping-write failure deliberately leaves a delivery recoverable rather than lying about it). A receiver is already required to be idempotent. This race produces a duplicate that falls inside that documented guarantee rather than violating it.
It is still worth fixing: every other duplicate in the system is the consequence of a failure, and this one happens on a healthy database during ordinary startup. "At-least-once when something went wrong" and "at-least-once on every restart with concurrent traffic" are different operational characters even though the contract covers both.
Definition of done
A fresh event arriving during startup recovery is delivered exactly once. The likely shape is ordering — do not accept receiver traffic until recovery's scan has claimed what it is going to claim — but confirm that does not simply move the race, and do not solve it by extending shutdown or startup past its documented budget (#134, #102).
The ownership mechanism from #256 (internal/delivery/inflight.go) is the right place to reason about this; the bug is that release-then-reacquire is legal for a delivery that was never actually finished by the second claimant's standard.
A deterministic test, not a probabilistic one. The reviewer built a deterministic probe; reproduce that shape rather than looping until a duplicate appears.
Re-run the at-scale no-duplicate check that #256 established — several hundred events across multiple targets, both sweep arms firing, sink POSTs equal to delivery count exactly.
Not milestoned. Top of the next milestone.
Found by the review of https://git.eeqj.de/sneak/webhooker/pulls/297. **Pre-existing** — a deterministic probe returns `posts=2` identically on `next` and on that PR's head, so it predates the branch and is not caused by it.
Startup `recoverPending` runs in a goroutine concurrently with the receiver accepting traffic. A delivery row written just before `Notify` can be selected by recovery, claimed, delivered, and RELEASED — after which `Notify`'s `retainIdle` succeeds on the now-free id and dispatches the same delivery again. The target receives two POSTs.
The window is the startup interval during which recovery is still scanning and the receiver is already serving. It is narrow, and the reviewer found it only by building a deterministic probe after a sloppy harness surfaced it as an intermittent 901-for-900.
## Why this is not tag-blocking
Delivery is at-least-once by design and now says so in the README (see the Rationale section, and https://git.eeqj.de/sneak/webhooker/issues/256 for why a bookkeeping-write failure deliberately leaves a delivery recoverable rather than lying about it). A receiver is already required to be idempotent. This race produces a duplicate that falls inside that documented guarantee rather than violating it.
It is still worth fixing: every other duplicate in the system is the consequence of a failure, and this one happens on a healthy database during ordinary startup. "At-least-once when something went wrong" and "at-least-once on every restart with concurrent traffic" are different operational characters even though the contract covers both.
## Definition of done
- A fresh event arriving during startup recovery is delivered exactly once. The likely shape is ordering — do not accept receiver traffic until recovery's scan has claimed what it is going to claim — but confirm that does not simply move the race, and do not solve it by extending shutdown or startup past its documented budget (https://git.eeqj.de/sneak/webhooker/issues/134, https://git.eeqj.de/sneak/webhooker/issues/102).
- The ownership mechanism from https://git.eeqj.de/sneak/webhooker/issues/256 (`internal/delivery/inflight.go`) is the right place to reason about this; the bug is that release-then-reacquire is legal for a delivery that was never actually finished by the second claimant's standard.
- A deterministic test, not a probabilistic one. The reviewer built a deterministic probe; reproduce that shape rather than looping until a duplicate appears.
- Re-run the at-scale no-duplicate check that https://git.eeqj.de/sneak/webhooker/issues/256 established — several hundred events across multiple targets, both sweep arms firing, sink POSTs equal to delivery count exactly.
Not milestoned. Top of the next milestone.
Plan. The code the issue names is unchanged on next (6ebac4f). The hole is in the second claimant. Notify takes the delivery with retainIdle, and a worker then runs it through processNewTask, which builds the delivery from the task as pending and sends it without reading its row. If restart recovery claimed, delivered and released the same delivery first, retainIdle succeeds again and the send repeats.
Recommended fix: before a new task is sent, read the delivery's status from its row by primary key. Skip the task if the status is no longer pending, logging that it was already handled. This works whichever claimant goes first, does not depend on clocks, and leaves startup ordering and shutdown timing alone. One of these is a finding in review:
a delay on the receiver or on startup;
a created-before-startup cutoff that depends on the wall clock;
any shape that only narrows the window.
The worker may take a different shape if it closes the race just as plainly; state why on the PR.
Constraints from the issue:
Test: a deterministic test that reproduces the recovery-then-Notify interleaving and asserts one send. It must fail on next without the fix, and it must not loop until a duplicate appears.
Existing behaviour: keep the ownership rules in internal/delivery/inflight.go and the documented startup and shutdown budgets (#134, #102). A failed bookkeeping write that leaves a row pending must still get sent again; that is the at-least-once contract, not this bug.
Scale check: re-run the at-scale no-duplicate check from #256, with several hundred events across several targets and both sweep arms firing. The sink must receive exactly one POST per delivery. Verify it; publish none of the evidence.
Model: opus-5-5
Plan. The code the issue names is unchanged on `next` (`6ebac4f`). The hole is in the second claimant. `Notify` takes the delivery with `retainIdle`, and a worker then runs it through `processNewTask`, which builds the delivery from the task as `pending` and sends it without reading its row. If restart recovery claimed, delivered and released the same delivery first, `retainIdle` succeeds again and the send repeats.
Recommended fix: before a new task is sent, read the delivery's status from its row by primary key. Skip the task if the status is no longer `pending`, logging that it was already handled. This works whichever claimant goes first, does not depend on clocks, and leaves startup ordering and shutdown timing alone. One of these is a finding in review:
- a delay on the receiver or on startup;
- a created-before-startup cutoff that depends on the wall clock;
- any shape that only narrows the window.
The worker may take a different shape if it closes the race just as plainly; state why on the PR.
Constraints from the issue:
- **Test:** a deterministic test that reproduces the recovery-then-`Notify` interleaving and asserts one send. It must fail on `next` without the fix, and it must not loop until a duplicate appears.
- **Existing behaviour:** keep the ownership rules in `internal/delivery/inflight.go` and the documented startup and shutdown budgets (https://git.eeqj.de/sneak/webhooker/issues/134, https://git.eeqj.de/sneak/webhooker/issues/102). A failed bookkeeping write that leaves a row `pending` must still get sent again; that is the at-least-once contract, not this bug.
- **Scale check:** re-run the at-scale no-duplicate check from https://git.eeqj.de/sneak/webhooker/issues/256, with several hundred events across several targets and both sweep arms firing. The sink must receive exactly one POST per delivery. Verify it; publish none of the evidence.
Model: opus-5-5
clawbot
self-assigned this 2026-09-29 03:14:50 +02:00
Built in #328 (branch issue-299-recovery-notify-duplicate, base next), in the shape the plan recommends. Before a worker sends a new task it reads the delivery's row and skips the task unless the row still says pending. A delivery that restart recovery has already sent is therefore not sent again when the receiver's Notify for it arrives afterwards.
Model: opus-5-5
Built in https://git.eeqj.de/sneak/webhooker/pulls/328 (branch `issue-299-recovery-notify-duplicate`, base `next`), in the shape the plan recommends. Before a worker sends a new task it reads the delivery's row and skips the task unless the row still says `pending`. A delivery that restart recovery has already sent is therefore not sent again when the receiver's `Notify` for it arrives afterwards.
Model: opus-5-5
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Found by the review of #297. Pre-existing — a deterministic probe returns
posts=2identically onnextand on that PR's head, so it predates the branch and is not caused by it.Startup
recoverPendingruns in a goroutine concurrently with the receiver accepting traffic. A delivery row written just beforeNotifycan be selected by recovery, claimed, delivered, and RELEASED — after whichNotify'sretainIdlesucceeds on the now-free id and dispatches the same delivery again. The target receives two POSTs.The window is the startup interval during which recovery is still scanning and the receiver is already serving. It is narrow, and the reviewer found it only by building a deterministic probe after a sloppy harness surfaced it as an intermittent 901-for-900.
Why this is not tag-blocking
Delivery is at-least-once by design and now says so in the README (see the Rationale section, and #256 for why a bookkeeping-write failure deliberately leaves a delivery recoverable rather than lying about it). A receiver is already required to be idempotent. This race produces a duplicate that falls inside that documented guarantee rather than violating it.
It is still worth fixing: every other duplicate in the system is the consequence of a failure, and this one happens on a healthy database during ordinary startup. "At-least-once when something went wrong" and "at-least-once on every restart with concurrent traffic" are different operational characters even though the contract covers both.
Definition of done
internal/delivery/inflight.go) is the right place to reason about this; the bug is that release-then-reacquire is legal for a delivery that was never actually finished by the second claimant's standard.Not milestoned. Top of the next milestone.
Plan. The code the issue names is unchanged on
next(6ebac4f). The hole is in the second claimant.Notifytakes the delivery withretainIdle, and a worker then runs it throughprocessNewTask, which builds the delivery from the task aspendingand sends it without reading its row. If restart recovery claimed, delivered and released the same delivery first,retainIdlesucceeds again and the send repeats.Recommended fix: before a new task is sent, read the delivery's status from its row by primary key. Skip the task if the status is no longer
pending, logging that it was already handled. This works whichever claimant goes first, does not depend on clocks, and leaves startup ordering and shutdown timing alone. One of these is a finding in review:The worker may take a different shape if it closes the race just as plainly; state why on the PR.
Constraints from the issue:
Notifyinterleaving and asserts one send. It must fail onnextwithout the fix, and it must not loop until a duplicate appears.internal/delivery/inflight.goand the documented startup and shutdown budgets (#134, #102). A failed bookkeeping write that leaves a rowpendingmust still get sent again; that is the at-least-once contract, not this bug.Model: opus-5-5
Built in #328 (branch
issue-299-recovery-notify-duplicate, basenext), in the shape the plan recommends. Before a worker sends a new task it reads the delivery's row and skips the task unless the row still sayspending. A delivery that restart recovery has already sent is therefore not sent again when the receiver'sNotifyfor it arrives afterwards.Model: opus-5-5