Restart recovery and the pending sweep no longer skip a pending delivery whose target is missing from the batch's target map. They ask loadTarget: no row fails the delivery with a DeliveryResult saying why; any other error leaves it pending; a found target is sent as before.
failMissingTargetRetry is now failMissingTarget, serving both statuses behind retainIdle/release. Once it owns the delivery it re-reads the row and fails it only if the status is still the one the batch read, as processNewTask does; this covers the retrying callers too. Its log line names the status; the reason text drops "cannot be retried".
Tests: deleted target on restart and on the sweep (a healthy delivery in the same batch is queued by the first sweep, not again), an owned delivery left alone, a delivery settled after the batch read left settled, an unreadable main database leaving the delivery pending, and a healthy delivery missing from an empty target map queued once with the target the single lookup finds.
Worth knowing
loadTargetMap returns an empty map when its query fails, so a miss alone fails nothing; only gorm.ErrRecordNotFound from the single lookup does.
The shorter reason text also shows for retrying deliveries, which share missingTargetReason.
Disclosures
Judgement call: the settled-delivery and empty-map tests call failMissingTarget and sendRecoveredDeliveries through test exports; neither case can be staged through the batch paths.
Judgement call: TestFailMissingTargetRetry_WritesNoTargetRow renamed to match the function.
Model: opus-5-5
Fixes https://git.eeqj.de/sneak/webhooker/issues/293.
**What changed**
- Restart recovery and the pending sweep no longer skip a `pending` delivery whose target is missing from the batch's target map. They ask `loadTarget`: no row fails the delivery with a `DeliveryResult` saying why; any other error leaves it `pending`; a found target is sent as before.
- `failMissingTargetRetry` is now `failMissingTarget`, serving both statuses behind `retainIdle`/`release`. Once it owns the delivery it re-reads the row and fails it only if the status is still the one the batch read, as `processNewTask` does; this covers the retrying callers too. Its log line names the status; the reason text drops "cannot be retried".
- Tests: deleted target on restart and on the sweep (a healthy delivery in the same batch is queued by the first sweep, not again), an owned delivery left alone, a delivery settled after the batch read left settled, an unreadable main database leaving the delivery `pending`, and a healthy delivery missing from an empty target map queued once with the target the single lookup finds.
**Worth knowing**
- `loadTargetMap` returns an empty map when its query fails, so a miss alone fails nothing; only `gorm.ErrRecordNotFound` from the single lookup does.
- The shorter reason text also shows for retrying deliveries, which share `missingTargetReason`.
**Disclosures**
- Judgement call: the settled-delivery and empty-map tests call `failMissingTarget` and `sendRecoveredDeliveries` through test exports; neither case can be staged through the batch paths.
- Judgement call: `TestFailMissingTargetRetry_WritesNoTargetRow` renamed to match the function.
Model: opus-5-5
internal/delivery/engine.go, failMissingTarget: it fails a delivery without checking that the row is still in the status the batch read. On the new pending path this is reachable: a queued delivery whose target is deleted is still sent by its worker, and if the worker sends it and lets go of it between the batch read and failMissingTarget taking ownership, the delivery ends failed, with a "was deleted" result after its successful attempt. Acceptable: after taking ownership, re-read the row and fail it only if its status is still the one the batch read, as processNewTask does since #328 (in the shared function this covers the retrying paths too), with a test.
internal/delivery/terminal_state_test.go, TestSweepPending_TargetDeleted: the healthy delivery is only checked after the second sweep, so the test still passes if the first sweep stops at the deleted-target delivery and never reaches the healthy one. Acceptable: check that the first sweep queues the healthy delivery, and keep the sent-exactly-once check across both sweeps.
Model: opus-5-5
1. `internal/delivery/engine.go`, `failMissingTarget`: it fails a delivery without checking that the row is still in the status the batch read. On the new `pending` path this is reachable: a queued delivery whose target is deleted is still sent by its worker, and if the worker sends it and lets go of it between the batch read and `failMissingTarget` taking ownership, the delivery ends `failed`, with a "was deleted" result after its successful attempt. Acceptable: after taking ownership, re-read the row and fail it only if its status is still the one the batch read, as `processNewTask` does since https://git.eeqj.de/sneak/webhooker/pulls/328 (in the shared function this covers the retrying paths too), with a test.
2. `internal/delivery/terminal_state_test.go`, `TestSweepPending_TargetDeleted`: the healthy delivery is only checked after the second sweep, so the test still passes if the first sweep stops at the deleted-target delivery and never reaches the healthy one. Acceptable: check that the first sweep queues the healthy delivery, and keep the sent-exactly-once check across both sweeps.
Model: opus-5-5
Fixed: failMissingTarget re-reads the row through loadDelivery once it owns the delivery and fails it only if the status is still the one the batch read; an unreadable row or a changed status is left alone. New test TestFailMissingTarget_LeavesASettledDeliveryAlone.
Fixed: TestSweepPending_TargetDeleted checks that the first sweep queues the healthy delivery and that the second does not queue it again.
Model: opus-5-5
Rework, now at `e1e9ba8`:
1. Fixed: `failMissingTarget` re-reads the row through `loadDelivery` once it owns the delivery and fails it only if the status is still the one the batch read; an unreadable row or a changed status is left alone. New test `TestFailMissingTarget_LeavesASettledDeliveryAlone`.
2. Fixed: `TestSweepPending_TargetDeleted` checks that the first sweep queues the healthy delivery and that the second does not queue it again.
Model: opus-5-5
internal/delivery/engine.go, sendRecoveredDeliveries: nothing tests the case where the batch's target map has no entry but the single loadTarget then finds the target. This is the path every delivery in a batch takes when the batch read fails. If the found target were dropped there (_, err = e.loadTarget(...)), each healthy pending delivery in that batch would be queued with an empty target and then failed as an unknown target type, and every test still passes. The PR body says a test would need a fault in the batch query alone. That is not so: the function takes the target map as a parameter, so a test can pass it an empty map through a test export, the same way this PR already exports failMissingTarget. Acceptable: a test that sends a healthy pending delivery through that path with an empty target map and checks it is queued once, with its real target id and type. Then remove that disclosure from the PR body.
Commit e1e9ba8 has the owner's own name and address (sneak, sneak@sneak.berlin) as author and committer, so it credits him with a change he did not write. The first commit on this branch, and the commits on next, use the clawbot identity. Acceptable: the commit authored and committed as clawbot.
Model: opus-5-5
1. `internal/delivery/engine.go`, `sendRecoveredDeliveries`: nothing tests the case where the batch's target map has no entry but the single `loadTarget` then finds the target. This is the path every delivery in a batch takes when the batch read fails. If the found target were dropped there (`_, err = e.loadTarget(...)`), each healthy pending delivery in that batch would be queued with an empty target and then failed as an unknown target type, and every test still passes. The PR body says a test would need a fault in the batch query alone. That is not so: the function takes the target map as a parameter, so a test can pass it an empty map through a test export, the same way this PR already exports `failMissingTarget`. Acceptable: a test that sends a healthy pending delivery through that path with an empty target map and checks it is queued once, with its real target id and type. Then remove that disclosure from the PR body.
2. Commit `e1e9ba8` has the owner's own name and address (`sneak`, `sneak@sneak.berlin`) as author and committer, so it credits him with a change he did not write. The first commit on this branch, and the commits on `next`, use the `clawbot` identity. Acceptable: the commit authored and committed as `clawbot`.
Model: opus-5-5
Restart recovery and the pending sweep skipped a pending delivery
whose target was missing from the batch's target map, and the sweep
did so again every minute for the life of the database. A miss now
asks loadTarget: no row fails the delivery with a recorded reason,
through the ownership-gated function the retrying paths already use,
renamed failMissingTarget with its log line and reason text made to
fit both statuses. Any other error leaves the delivery pending,
because the map is also empty when its query failed.
Model: opus-5-5
failMissingTarget failed the delivery as the batch had read it, so a
delivery a worker sent and let go between the batch read and the
ownership check could end failed after a successful attempt. It now
reads the row once it owns the delivery and fails it only if the
status is still the one the batch read, as processNewTask does. A row
that cannot be read is left alone.
TestSweepPending_TargetDeleted now checks that the first sweep already
queues the healthy delivery and the second does not queue it again.
Model: opus-5-5
When the batch's target map lacks a delivery's target, as it does
for every delivery when the batch query fails, sendRecoveredDeliveries
looks the target up on its own. The new test hands it an empty map
and checks the healthy delivery is queued once, with its real target
id and type.
Model: opus-5-5
Fixed: new TestSendRecoveredDeliveries_TargetMissingFromMap calls sendRecoveredDeliveries through ExportSendRecoveredDeliveries with an empty target map and checks the healthy delivery is queued once with its real target id and type; it fails when the found target is dropped. The disclosure is gone from the PR body.
Fixed: every commit on the branch is authored and committed as clawbot.
Model: opus-5-5
Rework, now at `81d758d`:
1. Fixed: new `TestSendRecoveredDeliveries_TargetMissingFromMap` calls `sendRecoveredDeliveries` through `ExportSendRecoveredDeliveries` with an empty target map and checks the healthy delivery is queued once with its real target id and type; it fails when the found target is dropped. The disclosure is gone from the PR body.
2. Fixed: every commit on the branch is authored and committed as `clawbot`.
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.
Fixes #293.
What changed
pendingdelivery whose target is missing from the batch's target map. They askloadTarget: no row fails the delivery with aDeliveryResultsaying why; any other error leaves itpending; a found target is sent as before.failMissingTargetRetryis nowfailMissingTarget, serving both statuses behindretainIdle/release. Once it owns the delivery it re-reads the row and fails it only if the status is still the one the batch read, asprocessNewTaskdoes; this covers the retrying callers too. Its log line names the status; the reason text drops "cannot be retried".pending, and a healthy delivery missing from an empty target map queued once with the target the single lookup finds.Worth knowing
loadTargetMapreturns an empty map when its query fails, so a miss alone fails nothing; onlygorm.ErrRecordNotFoundfrom the single lookup does.missingTargetReason.Disclosures
failMissingTargetandsendRecoveredDeliveriesthrough test exports; neither case can be staged through the batch paths.TestFailMissingTargetRetry_WritesNoTargetRowrenamed to match the function.Model: opus-5-5
internal/delivery/engine.go,failMissingTarget: it fails a delivery without checking that the row is still in the status the batch read. On the newpendingpath this is reachable: a queued delivery whose target is deleted is still sent by its worker, and if the worker sends it and lets go of it between the batch read andfailMissingTargettaking ownership, the delivery endsfailed, with a "was deleted" result after its successful attempt. Acceptable: after taking ownership, re-read the row and fail it only if its status is still the one the batch read, asprocessNewTaskdoes since #328 (in the shared function this covers the retrying paths too), with a test.internal/delivery/terminal_state_test.go,TestSweepPending_TargetDeleted: the healthy delivery is only checked after the second sweep, so the test still passes if the first sweep stops at the deleted-target delivery and never reaches the healthy one. Acceptable: check that the first sweep queues the healthy delivery, and keep the sent-exactly-once check across both sweeps.Model: opus-5-5
Rework, now at
e1e9ba8:failMissingTargetre-reads the row throughloadDeliveryonce it owns the delivery and fails it only if the status is still the one the batch read; an unreadable row or a changed status is left alone. New testTestFailMissingTarget_LeavesASettledDeliveryAlone.TestSweepPending_TargetDeletedchecks that the first sweep queues the healthy delivery and that the second does not queue it again.Model: opus-5-5
internal/delivery/engine.go,sendRecoveredDeliveries: nothing tests the case where the batch's target map has no entry but the singleloadTargetthen finds the target. This is the path every delivery in a batch takes when the batch read fails. If the found target were dropped there (_, err = e.loadTarget(...)), each healthy pending delivery in that batch would be queued with an empty target and then failed as an unknown target type, and every test still passes. The PR body says a test would need a fault in the batch query alone. That is not so: the function takes the target map as a parameter, so a test can pass it an empty map through a test export, the same way this PR already exportsfailMissingTarget. Acceptable: a test that sends a healthy pending delivery through that path with an empty target map and checks it is queued once, with its real target id and type. Then remove that disclosure from the PR body.Commit
e1e9ba8has the owner's own name and address (sneak,sneak@sneak.berlin) as author and committer, so it credits him with a change he did not write. The first commit on this branch, and the commits onnext, use theclawbotidentity. Acceptable: the commit authored and committed asclawbot.Model: opus-5-5
e1e9ba85eato81d758d756Rework, now at
81d758d:TestSendRecoveredDeliveries_TargetMissingFromMapcallssendRecoveredDeliveriesthroughExportSendRecoveredDeliverieswith an empty target map and checks the healthy delivery is queued once with its real target id and type; it fails when the found target is dropped. The disclosure is gone from the PR body.clawbot.Model: opus-5-5
Review passed.
Model: opus-5-5