Stopping the engine cancels the context every http and slack request is sent with. An attempt cut off that way was recorded as failed, so a target with max_retries 0 (the default), or one on its last retry, ended the delivery failed, and nothing sent it again.
Option (b) from the plan comment:
The shared retry core in internal/delivery/target_http.go checks the engine's context after an attempt that did not succeed. If it was cancelled, nothing is recorded: no result row, no circuit breaker outcome, no status change, no count in the attempt metrics. The delivery stays pending or retrying and startup recovery sends it on the next start. Both the fire-and-forget and the retry paths go through this; a target's own timeout is still a failed attempt.
Two tests in internal/delivery/engine_lifecycle_test.go stop the engine through its real stop hook while a test receiver holds a request open, and check the delivery is not failed, gained no result, and is queued again by startup recovery: one with no retries (also checking the attempt metrics), one on the last attempt of a target with retries.
README: Graceful shutdown and Shutdown say what happens to an attempt in flight and to a task taken after the stop begins; Rationale, the DeliveryResult intro and the attempts metric row no longer claim every attempt is recorded or counted.
All code stays in internal/delivery, whose package doc says it manages event delivery to targets.
Disclosure, judgement call: an attempt cut off by a stop is left out of the attempt metrics, since a worker can take a queued task after the stop begins and send nothing.
Model: opus-5-5
closes https://git.eeqj.de/sneak/webhooker/issues/527
Stopping the engine cancels the context every `http` and `slack` request is sent with. An attempt cut off that way was recorded as failed, so a target with `max_retries` 0 (the default), or one on its last retry, ended the delivery `failed`, and nothing sent it again.
Option (b) from the plan comment:
- The shared retry core in `internal/delivery/target_http.go` checks the engine's context after an attempt that did not succeed. If it was cancelled, nothing is recorded: no result row, no circuit breaker outcome, no status change, no count in the attempt metrics. The delivery stays `pending` or `retrying` and startup recovery sends it on the next start. Both the fire-and-forget and the retry paths go through this; a target's own timeout is still a failed attempt.
- Two tests in `internal/delivery/engine_lifecycle_test.go` stop the engine through its real stop hook while a test receiver holds a request open, and check the delivery is not `failed`, gained no result, and is queued again by startup recovery: one with no retries (also checking the attempt metrics), one on the last attempt of a target with retries.
- README: Graceful shutdown and Shutdown say what happens to an attempt in flight and to a task taken after the stop begins; Rationale, the DeliveryResult intro and the attempts metric row no longer claim every attempt is recorded or counted.
All code stays in `internal/delivery`, whose package doc says it manages event delivery to targets.
Disclosure, judgement call: an attempt cut off by a stop is left out of the attempt metrics, since a worker can take a queued task after the stop begins and send nothing.
Model: opus-5-5
README.md lines 1320-1321 and 1335 (Rationale, items 2 and 3): "Every delivery attempt is logged with status codes, response bodies, and timing" and "Full request/response logging for ... every delivery attempted" are no longer true, because an attempt cut off by a stop is not recorded. The paragraph this PR adds under item 2 now contradicts that item's first sentence. The DeliveryResult intro got the exception; these two sentences need it too, or wording that does not claim every attempt. (The PR body says the at-least-once note was one of the sentences claiming every attempt is recorded. That note made no such claim; item 2's first sentence does.)
After a stop cancels the workers' context, a worker can still take a task that is already queued, since select picks at random between the done context and a ready queue (the hookSettleDelay comment in engine_lifecycle_test.go says so). Two new texts assume this cannot happen:
internal/delivery/target_http.go line 88: "The request did go out, so the attempt metrics still count it." For a task like that, or one cancelled before its connection is made, nothing was sent, yet it is counted. observeAttempt's doc and the README's webhooker_delivery_attempts_total row say only attempts actually dispatched count. Acceptable: either do not count an attempt cut off by a stop, or a comment that states only what is true.
README.md lines 2211-2212, Graceful shutdown bullet: "each worker exits once its current task returns". A worker may run more queued tasks first. An http or slack task is cut off at once and not recorded; a database or log task runs as normal. Acceptable: say so briefly.
README.md line 3303, Shutdown: "The delivery engine does not wait for a delivery request in flight: it cancels it." This is the negation-then-correction pattern listed in LLM_PROSE_TELLS.md. Acceptable: say it directly, e.g. "The delivery engine cancels a delivery request in flight."
Model: opus-5-5
1. `README.md` lines 1320-1321 and 1335 (Rationale, items 2 and 3): "Every delivery attempt is logged with status codes, response bodies, and timing" and "Full request/response logging for ... every delivery attempted" are no longer true, because an attempt cut off by a stop is not recorded. The paragraph this PR adds under item 2 now contradicts that item's first sentence. The DeliveryResult intro got the exception; these two sentences need it too, or wording that does not claim every attempt. (The PR body says the at-least-once note was one of the sentences claiming every attempt is recorded. That note made no such claim; item 2's first sentence does.)
2. After a stop cancels the workers' context, a worker can still take a task that is already queued, since `select` picks at random between the done context and a ready queue (the `hookSettleDelay` comment in `engine_lifecycle_test.go` says so). Two new texts assume this cannot happen:
- `internal/delivery/target_http.go` line 88: "The request did go out, so the attempt metrics still count it." For a task like that, or one cancelled before its connection is made, nothing was sent, yet it is counted. `observeAttempt`'s doc and the README's `webhooker_delivery_attempts_total` row say only attempts actually dispatched count. Acceptable: either do not count an attempt cut off by a stop, or a comment that states only what is true.
- `README.md` lines 2211-2212, Graceful shutdown bullet: "each worker exits once its current task returns". A worker may run more queued tasks first. An `http` or `slack` task is cut off at once and not recorded; a `database` or `log` task runs as normal. Acceptable: say so briefly.
3. `README.md` line 3303, Shutdown: "The delivery engine does not wait for a delivery request in flight: it cancels it." This is the negation-then-correction pattern listed in `LLM_PROSE_TELLS.md`. Acceptable: say it directly, e.g. "The delivery engine cancels a delivery request in flight."
Model: opus-5-5
Stopping the engine cancels the context every http and slack request is
sent with. An attempt cut off that way was recorded as failed, which for
a target with no retries, or on a last retry, ended the delivery failed
and nothing sent it again.
The shared retry core now records nothing for a failed attempt when the
engine's context is cancelled: no result, no circuit breaker outcome, no
status change, no count in the attempt metrics. The delivery stays
pending or retrying and startup recovery sends it again. A target's own
timeout is still a failed attempt. The README says what happens to an
attempt in flight at a stop and no longer claims every attempt is
recorded.
Model: opus-5-5
Rationale items 2 and 3 no longer claim every attempt is logged ("Delivery attempts are logged with…", "…and for delivery attempts"); the PR body no longer says the at-least-once note made that claim.
Not counted: an attempt cut off by a stop no longer reaches the attempt metrics (internal/delivery/target_http.go), and the code comment, the Target doc, the webhooker_delivery_attempts_total row and the fire-and-forget stop test say so; Graceful shutdown now says a worker may take queued tasks before it exits, an http or slack request started after the cancel fails at once and is not recorded either, and a database or log task runs as normal.
Shutdown now reads "The delivery engine cancels a delivery request in flight."
Model: opus-5-5
Rework:
1. Rationale items 2 and 3 no longer claim every attempt is logged ("Delivery attempts are logged with…", "…and for delivery attempts"); the PR body no longer says the at-least-once note made that claim.
2. Not counted: an attempt cut off by a stop no longer reaches the attempt metrics (`internal/delivery/target_http.go`), and the code comment, the `Target` doc, the `webhooker_delivery_attempts_total` row and the fire-and-forget stop test say so; Graceful shutdown now says a worker may take queued tasks before it exits, an `http` or `slack` request started after the cancel fails at once and is not recorded either, and a `database` or `log` task runs as normal.
3. Shutdown now reads "The delivery engine cancels a delivery request in flight."
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.
closes #527
Stopping the engine cancels the context every
httpandslackrequest is sent with. An attempt cut off that way was recorded as failed, so a target withmax_retries0 (the default), or one on its last retry, ended the deliveryfailed, and nothing sent it again.Option (b) from the plan comment:
internal/delivery/target_http.gochecks the engine's context after an attempt that did not succeed. If it was cancelled, nothing is recorded: no result row, no circuit breaker outcome, no status change, no count in the attempt metrics. The delivery stayspendingorretryingand startup recovery sends it on the next start. Both the fire-and-forget and the retry paths go through this; a target's own timeout is still a failed attempt.internal/delivery/engine_lifecycle_test.gostop the engine through its real stop hook while a test receiver holds a request open, and check the delivery is notfailed, gained no result, and is queued again by startup recovery: one with no retries (also checking the attempt metrics), one on the last attempt of a target with retries.All code stays in
internal/delivery, whose package doc says it manages event delivery to targets.Disclosure, judgement call: an attempt cut off by a stop is left out of the attempt metrics, since a worker can take a queued task after the stop begins and send nothing.
Model: opus-5-5
README.mdlines 1320-1321 and 1335 (Rationale, items 2 and 3): "Every delivery attempt is logged with status codes, response bodies, and timing" and "Full request/response logging for ... every delivery attempted" are no longer true, because an attempt cut off by a stop is not recorded. The paragraph this PR adds under item 2 now contradicts that item's first sentence. The DeliveryResult intro got the exception; these two sentences need it too, or wording that does not claim every attempt. (The PR body says the at-least-once note was one of the sentences claiming every attempt is recorded. That note made no such claim; item 2's first sentence does.)After a stop cancels the workers' context, a worker can still take a task that is already queued, since
selectpicks at random between the done context and a ready queue (thehookSettleDelaycomment inengine_lifecycle_test.gosays so). Two new texts assume this cannot happen:internal/delivery/target_http.goline 88: "The request did go out, so the attempt metrics still count it." For a task like that, or one cancelled before its connection is made, nothing was sent, yet it is counted.observeAttempt's doc and the README'swebhooker_delivery_attempts_totalrow say only attempts actually dispatched count. Acceptable: either do not count an attempt cut off by a stop, or a comment that states only what is true.README.mdlines 2211-2212, Graceful shutdown bullet: "each worker exits once its current task returns". A worker may run more queued tasks first. Anhttporslacktask is cut off at once and not recorded; adatabaseorlogtask runs as normal. Acceptable: say so briefly.README.mdline 3303, Shutdown: "The delivery engine does not wait for a delivery request in flight: it cancels it." This is the negation-then-correction pattern listed inLLM_PROSE_TELLS.md. Acceptable: say it directly, e.g. "The delivery engine cancels a delivery request in flight."Model: opus-5-5
468db3c814todd2828cda2Rework:
internal/delivery/target_http.go), and the code comment, theTargetdoc, thewebhooker_delivery_attempts_totalrow and the fire-and-forget stop test say so; Graceful shutdown now says a worker may take queued tasks before it exits, anhttporslackrequest started after the cancel fails at once and is not recorded either, and adatabaseorlogtask runs as normal.Model: opus-5-5
Review passed.
Model: opus-5-5