The withRetry bookkeeping-failure branch is unpinned: removing its error check leaves the suite green #285

New Issue

clawbot · 2026-08-24T03:49:30+02:00

clawbot commented

2026-08-24 03:49:30 +02:00

Found by mutation testing during the second review of #263.

internal/delivery/target_http.go:130-141 — the withRetry bookkeeping-failure branch. Replacing its recordResult error check with _ = leaves the ENTIRE internal/delivery suite green. Its fire-and-forget twin at :80-86 IS pinned by a test.

Why this particular arm matters: it is the one that creates the retrying-with-a-successful-result state, which is exactly what findings 1 and 2 of that PR's first review were about — a delivery whose send succeeded but whose bookkeeping write failed. #263 fixed the handling of that state and added reconciliation for it. The handling is now correct and was verified live; what is missing is a test that would notice if it regressed.

Four other guards in that PR were mutation-checked and all went red when removed. This is the one that did not.

Definition of done

A test that fails when withRetry's bookkeeping-failure branch stops checking its recordResult error. Mutation-verify it: make the mutation, confirm red; restore, confirm green. State that you did so.
While there, check the sibling paths in the same file for the same gap — a guard that no test pins is easy to have more than one of.

Not milestoned: the behaviour is correct as it stands and was verified by execution across both reader and no-reader arms. This is regression insurance for a mechanism that took two review rounds to get right, which is precisely the kind of code that should not be left unpinned.

Found by mutation testing during the second review of https://git.eeqj.de/sneak/webhooker/pulls/263. `internal/delivery/target_http.go:130-141` — the `withRetry` bookkeeping-failure branch. Replacing its `recordResult` error check with `_ =` leaves the ENTIRE `internal/delivery` suite green. Its fire-and-forget twin at `:80-86` IS pinned by a test. Why this particular arm matters: it is the one that creates the retrying-with-a-successful-result state, which is exactly what findings 1 and 2 of that PR's first review were about — a delivery whose send succeeded but whose bookkeeping write failed. https://git.eeqj.de/sneak/webhooker/pulls/263 fixed the handling of that state and added reconciliation for it. The handling is now correct and was verified live; what is missing is a test that would notice if it regressed. Four other guards in that PR were mutation-checked and all went red when removed. This is the one that did not. ## Definition of done - A test that fails when `withRetry`'s bookkeeping-failure branch stops checking its `recordResult` error. Mutation-verify it: make the mutation, confirm red; restore, confirm green. State that you did so. - While there, check the sibling paths in the same file for the same gap — a guard that no test pins is easy to have more than one of. Not milestoned: the behaviour is correct as it stands and was verified by execution across both reader and no-reader arms. This is regression insurance for a mechanism that took two review rounds to get right, which is precisely the kind of code that should not be left unpinned.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/webhooker#285