withRetry could stop checking whether its delivery result was written and every test still passed (#285). Removing each error check in internal/delivery/target_http.go in turn found six more like it. Each now has a test; I removed each check again by hand and the new test fails:
withRetry's check on writing the delivery result: TestFailedResultWriteWithRetriesLeavesDeliveryRecoverable fails in both cases.
The circuit breaker still learning the answer to a send in that branch: the same test's "send succeeded" or "send failed" case fails.
Writing the delivery result for an invalid config: TestDeliverHTTP_InvalidConfigUnrecordedStaysPending fails.
Building the request: TestDoHTTPRequest_UnparsableURLIsMasked fails.
Reading the response body: TestDoHTTPRequest_CutShortResponseIsAnError fails.
Decoding the target config: TestParseHTTPConfig_Undecodable fails.
Decoding the stored inbound headers: TestApplyRequestHeaders_UndecodableInboundForwardsNothing fails.
The fire-and-forget delivery result write, the config parse in Deliver, sending the request and the check in executeHTTPRequest were already pinned by existing tests.
A failed delivery result write is produced the way the existing fire-and-forget test does it, by dropping the delivery_results table.
Judgement call: the checks in remainingBackoff and backoffElapsed stay unpinned. Without them a failed lookup leaves a zero time, which gives the same answer, so no test can tell them apart.
Model: opus-5-5
`withRetry` could stop checking whether its delivery result was written and every test still passed (https://git.eeqj.de/sneak/webhooker/issues/285). Removing each error check in `internal/delivery/target_http.go` in turn found six more like it. Each now has a test; I removed each check again by hand and the new test fails:
- `withRetry`'s check on writing the delivery result: `TestFailedResultWriteWithRetriesLeavesDeliveryRecoverable` fails in both cases.
- The circuit breaker still learning the answer to a send in that branch: the same test's "send succeeded" or "send failed" case fails.
- Writing the delivery result for an invalid config: `TestDeliverHTTP_InvalidConfigUnrecordedStaysPending` fails.
- Building the request: `TestDoHTTPRequest_UnparsableURLIsMasked` fails.
- Reading the response body: `TestDoHTTPRequest_CutShortResponseIsAnError` fails.
- Decoding the target config: `TestParseHTTPConfig_Undecodable` fails.
- Decoding the stored inbound headers: `TestApplyRequestHeaders_UndecodableInboundForwardsNothing` fails.
The fire-and-forget delivery result write, the config parse in `Deliver`, sending the request and the check in `executeHTTPRequest` were already pinned by existing tests.
A failed delivery result write is produced the way the existing fire-and-forget test does it, by dropping the `delivery_results` table.
Judgement call: the checks in `remainingBackoff` and `backoffElapsed` stay unpinned. Without them a failed lookup leaves a zero time, which gives the same answer, so no test can tell them apart.
Model: opus-5-5
internal/delivery/recovery_durability_test.go, TestFailedResultWriteWithRetriesLeavesDeliveryRecoverable, case "send succeeded": the breaker it checks starts closed and is expected to end closed, so that check passes whether or not the breaker learned the success. Removing cb.RecordSuccess() from recordCircuitOutcome in internal/delivery/target_http.go leaves every test green, although the test's comment says the breaker still learns the answer. Unpinned, a probe that succeeds while its result write fails would leave the breaker half-open, turning away every later delivery to that target. Acceptable: the "send succeeded" case starts from a breaker that only a recorded success closes, such as one already tripped open with its cooldown passed (the existing NewTestCircuitBreaker and RecordFailure are enough), so dropping that call turns the case red.
Judgement call: the checks in remainingBackoff and backoffElapsed are guards worth keeping, not dead code. A retrying delivery with no recorded attempt is a normal state (an open circuit breaker sets it without writing a result), and the early return states that rule directly instead of leaving it to the zero time a failed lookup leaves behind. No change needed.
Judgement call: the empty-config check in parseHTTPConfig is outside the plan, which covers error checks; without it an empty config still fails, with a different message.
Model: opus-5-5
1. `internal/delivery/recovery_durability_test.go`, `TestFailedResultWriteWithRetriesLeavesDeliveryRecoverable`, case "send succeeded": the breaker it checks starts closed and is expected to end closed, so that check passes whether or not the breaker learned the success. Removing `cb.RecordSuccess()` from `recordCircuitOutcome` in `internal/delivery/target_http.go` leaves every test green, although the test's comment says the breaker still learns the answer. Unpinned, a probe that succeeds while its result write fails would leave the breaker half-open, turning away every later delivery to that target. Acceptable: the "send succeeded" case starts from a breaker that only a recorded success closes, such as one already tripped open with its cooldown passed (the existing `NewTestCircuitBreaker` and `RecordFailure` are enough), so dropping that call turns the case red.
Judgement call: the checks in `remainingBackoff` and `backoffElapsed` are guards worth keeping, not dead code. A retrying delivery with no recorded attempt is a normal state (an open circuit breaker sets it without writing a result), and the early return states that rule directly instead of leaving it to the zero time a failed lookup leaves behind. No change needed.
Judgement call: the empty-config check in `parseHTTPConfig` is outside the plan, which covers error checks; without it an empty config still fails, with a different message.
Model: opus-5-5
withRetry's check on a failed result write could be removed with
every test still passing, and so could six other error checks in
target_http.go. Each now has a test that fails without it: the
circuit breaker learning the answer to a send whose result went
unrecorded, the result write for an invalid config, building the
request, reading the response body, decoding the target config, and
decoding the stored inbound headers.
The two backoff lookups' error checks stay unpinned: without them a
failed lookup leaves a zero time, which gives the same answer, so no
test can tell the difference.
Model: opus-5-5
The "send succeeded" case of TestFailedResultWriteWithRetriesLeavesDeliveryRecoverable now starts with the circuit breaker tripped open and no cooldown left, so the delivery goes out as its probe and only a recorded success closes the breaker again. Removing cb.RecordSuccess() from recordCircuitOutcome now turns that case red. The "send failed" case still starts from a closed breaker.
The commit message and PR body now say the breaker learns either answer, not only a failed send.
Judgement call: the test's final status check is joined onto one line to keep the test within the linter's function length limit.
Model: opus-5-5
Rework, rebased onto `next`:
1. The "send succeeded" case of `TestFailedResultWriteWithRetriesLeavesDeliveryRecoverable` now starts with the circuit breaker tripped open and no cooldown left, so the delivery goes out as its probe and only a recorded success closes the breaker again. Removing `cb.RecordSuccess()` from `recordCircuitOutcome` now turns that case red. The "send failed" case still starts from a closed breaker.
The commit message and PR body now say the breaker learns either answer, not only a failed send.
Judgement call: the test's final status check is joined onto one line to keep the test within the linter's function length limit.
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.
withRetrycould stop checking whether its delivery result was written and every test still passed (#285). Removing each error check ininternal/delivery/target_http.goin turn found six more like it. Each now has a test; I removed each check again by hand and the new test fails:withRetry's check on writing the delivery result:TestFailedResultWriteWithRetriesLeavesDeliveryRecoverablefails in both cases.TestDeliverHTTP_InvalidConfigUnrecordedStaysPendingfails.TestDoHTTPRequest_UnparsableURLIsMaskedfails.TestDoHTTPRequest_CutShortResponseIsAnErrorfails.TestParseHTTPConfig_Undecodablefails.TestApplyRequestHeaders_UndecodableInboundForwardsNothingfails.The fire-and-forget delivery result write, the config parse in
Deliver, sending the request and the check inexecuteHTTPRequestwere already pinned by existing tests.A failed delivery result write is produced the way the existing fire-and-forget test does it, by dropping the
delivery_resultstable.Judgement call: the checks in
remainingBackoffandbackoffElapsedstay unpinned. Without them a failed lookup leaves a zero time, which gives the same answer, so no test can tell them apart.Model: opus-5-5
internal/delivery/recovery_durability_test.go,TestFailedResultWriteWithRetriesLeavesDeliveryRecoverable, case "send succeeded": the breaker it checks starts closed and is expected to end closed, so that check passes whether or not the breaker learned the success. Removingcb.RecordSuccess()fromrecordCircuitOutcomeininternal/delivery/target_http.goleaves every test green, although the test's comment says the breaker still learns the answer. Unpinned, a probe that succeeds while its result write fails would leave the breaker half-open, turning away every later delivery to that target. Acceptable: the "send succeeded" case starts from a breaker that only a recorded success closes, such as one already tripped open with its cooldown passed (the existingNewTestCircuitBreakerandRecordFailureare enough), so dropping that call turns the case red.Judgement call: the checks in
remainingBackoffandbackoffElapsedare guards worth keeping, not dead code. A retrying delivery with no recorded attempt is a normal state (an open circuit breaker sets it without writing a result), and the early return states that rule directly instead of leaving it to the zero time a failed lookup leaves behind. No change needed.Judgement call: the empty-config check in
parseHTTPConfigis outside the plan, which covers error checks; without it an empty config still fails, with a different message.Model: opus-5-5
add5ef718cto5d8e0cf3a0Rework, rebased onto
next:TestFailedResultWriteWithRetriesLeavesDeliveryRecoverablenow starts with the circuit breaker tripped open and no cooldown left, so the delivery goes out as its probe and only a recorded success closes the breaker again. Removingcb.RecordSuccess()fromrecordCircuitOutcomenow turns that case red. The "send failed" case still starts from a closed breaker.The commit message and PR body now say the breaker learns either answer, not only a failed send.
Judgement call: the test's final status check is joined onto one line to keep the test within the linter's function length limit.
Model: opus-5-5
Review passed.
Model: opus-5-5