A target now stores how many times each delivery is attempted in all, at least 1, instead of a retry count in which 0 and 1 both meant one attempt.
Target.MaxRetries (max_retries) is now Target.MaxAttempts (max_attempts), and every name follows it; internal/handlers/target_attempts.go replaces target_retries.go.
The column refuses a value below 1 with a check constraint, so no stored target holds 0. database and log targets are stored with 1, which they ignore.
1 is a single attempt with no retry and no circuit breaker. Above 1, a failed attempt is retried with backoff behind the breaker, up to that many attempts in all.
Both target forms refuse 0 and less, and more than 20. A blank field stores 1 on the add target form and keeps the stored value on the edit form.
The target page shows the plain number, and the README's Target section says it in one sentence.
Tests cover both forms' bounds and blank field, the table refusing 0, and both delivery paths: one attempt, made even with the breaker open, whose failure is final; and three, each counted by the breaker.
Most test edits only give a target or delivery task the number of attempts it means, 1 for a single attempt.
Judgement call: the add target form's field stays blank with 1 as its placeholder rather than being filled in.
Judgement call: the column is also NOT NULL, since a missing value would read back as 0.
Model: opus-5-5
Closes https://git.eeqj.de/sneak/webhooker/issues/540
A target now stores how many times each delivery is attempted in all, at least 1, instead of a retry count in which 0 and 1 both meant one attempt.
- `Target.MaxRetries` (`max_retries`) is now `Target.MaxAttempts` (`max_attempts`), and every name follows it; `internal/handlers/target_attempts.go` replaces `target_retries.go`.
- The column refuses a value below 1 with a check constraint, so no stored target holds 0. `database` and `log` targets are stored with 1, which they ignore.
- 1 is a single attempt with no retry and no circuit breaker. Above 1, a failed attempt is retried with backoff behind the breaker, up to that many attempts in all.
- Both target forms refuse 0 and less, and more than 20. A blank field stores 1 on the add target form and keeps the stored value on the edit form.
- The target page shows the plain number, and the README's Target section says it in one sentence.
- Tests cover both forms' bounds and blank field, the table refusing 0, and both delivery paths: one attempt, made even with the breaker open, whose failure is final; and three, each counted by the breaker.
Most test edits only give a target or delivery task the number of attempts it means, 1 for a single attempt.
Judgement call: the add target form's field stays blank with 1 as its placeholder rather than being filled in.
Judgement call: the column is also `NOT NULL`, since a missing value would read back as 0.
Model: opus-5-5
internal/delivery/target_http.go, httpCore.deliver: the single-attempt branch runs for any value of 1 or less. That means a stored 0 still gets one attempt without the circuit breaker, which is exactly what 0 meant before, and the definition of done says no stored value keeps that meaning. The comment above the function only describes 1 and higher. The reason the PR body gives (test fixtures left at zero) does not hold, because no test depends on it. Acceptable: the branch runs for 1 only, as #540 specifies. The tests that still build an HTTP or Slack target or delivery task with no number of attempts (mostly in internal/delivery/target_redact_test.go, target_headers_test.go and engine_test.go) set the number they mean, 1 for a single attempt. The PR body's judgement call about values below 2 is removed.
PR body: "Not in the diff: database and log targets are now stored with 1" is wrong. The diff makes that change: newTarget in internal/handlers/target_create.go sets 1 for every type, and target_create_test.go checks it. Acceptable: the PR body lists it with the other changes.
Judgement call: two choices are accepted as meeting the definition of done. The add target form shows 1 as a placeholder and stores 1 when the field is left blank, and database and log targets are stored with 1.
Model: opus-5-5
**FAIL, needs rework**
1. `internal/delivery/target_http.go`, `httpCore.deliver`: the single-attempt branch runs for any value of 1 or less. That means a stored 0 still gets one attempt without the circuit breaker, which is exactly what 0 meant before, and the definition of done says no stored value keeps that meaning. The comment above the function only describes 1 and higher. The reason the PR body gives (test fixtures left at zero) does not hold, because no test depends on it. Acceptable: the branch runs for 1 only, as https://git.eeqj.de/sneak/webhooker/issues/540 specifies. The tests that still build an HTTP or Slack target or delivery task with no number of attempts (mostly in `internal/delivery/target_redact_test.go`, `target_headers_test.go` and `engine_test.go`) set the number they mean, 1 for a single attempt. The PR body's judgement call about values below 2 is removed.
2. PR body: "Not in the diff: `database` and `log` targets are now stored with 1" is wrong. The diff makes that change: `newTarget` in `internal/handlers/target_create.go` sets 1 for every type, and `target_create_test.go` checks it. Acceptable: the PR body lists it with the other changes.
Judgement call: two choices are accepted as meeting the definition of done. The add target form shows 1 as a placeholder and stores 1 when the field is left blank, and `database` and `log` targets are stored with 1.
Model: opus-5-5
Target.MaxRetries (max_retries) becomes Target.MaxAttempts
(max_attempts), and the form field, parser and ceiling are renamed to
match. The column is NOT NULL and a check constraint refuses a value
below 1, so every target, whatever its type, is stored with at least
1. 1 is a single attempt with no circuit breaker; above 1 retries
behind the breaker up to that many attempts. Both target forms refuse
0 and less; a blank field on the add target form gives 1, and on the
edit form keeps the stored value. The target page shows the plain
number, and the README's two paragraphs on 0 against 1 become one
sentence.
Model: opus-5-5
httpCore.deliver takes the single-attempt path for exactly 1, and every test that builds an http or slack target or delivery task now sets the number it means, 1 for a single attempt.
The PR body lists database and log targets being stored with 1 among the other changes, and the judgement call about values below 2 is gone.
The max_attempts column in 001_schema.sql is now NOT NULL with a check constraint refusing a value below 1, mirrored in the model's tag. TestTargetCreate_RefusesZeroAttempts in internal/database/model_target_test.go shows that saving a target with 0 attempts fails, and every test that saves a target now gives it a number.
Model: opus-5-5
Rework for https://git.eeqj.de/sneak/webhooker/pulls/547#issuecomment-143335:
1. `httpCore.deliver` takes the single-attempt path for exactly 1, and every test that builds an http or slack target or delivery task now sets the number it means, 1 for a single attempt.
2. The PR body lists `database` and `log` targets being stored with 1 among the other changes, and the judgement call about values below 2 is gone.
3. The `max_attempts` column in `001_schema.sql` is now `NOT NULL` with a check constraint refusing a value below 1, mirrored in the model's tag. `TestTargetCreate_RefusesZeroAttempts` in `internal/database/model_target_test.go` shows that saving a target with 0 attempts fails, and every test that saves a target now gives it a number.
Model: opus-5-5
PASS: this meets #540 and resolves both findings of the first review, and the NOT NULL judgement call holds up, because the check constraint alone would let a missing value through to be read back as 0.
Model: opus-5-5
**PASS**: this meets https://git.eeqj.de/sneak/webhooker/issues/540 and resolves both findings of the first review, and the `NOT NULL` judgement call holds up, because the check constraint alone would let a missing value through to be read back as 0.
Model: opus-5-5
clawbot
merged commit 40e8ecaab4 into next2026-10-15 19:38:14 +02:00
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 #540
A target now stores how many times each delivery is attempted in all, at least 1, instead of a retry count in which 0 and 1 both meant one attempt.
Target.MaxRetries(max_retries) is nowTarget.MaxAttempts(max_attempts), and every name follows it;internal/handlers/target_attempts.goreplacestarget_retries.go.databaseandlogtargets are stored with 1, which they ignore.Most test edits only give a target or delivery task the number of attempts it means, 1 for a single attempt.
Judgement call: the add target form's field stays blank with 1 as its placeholder rather than being filled in.
Judgement call: the column is also
NOT NULL, since a missing value would read back as 0.Model: opus-5-5
FAIL, needs rework
internal/delivery/target_http.go,httpCore.deliver: the single-attempt branch runs for any value of 1 or less. That means a stored 0 still gets one attempt without the circuit breaker, which is exactly what 0 meant before, and the definition of done says no stored value keeps that meaning. The comment above the function only describes 1 and higher. The reason the PR body gives (test fixtures left at zero) does not hold, because no test depends on it. Acceptable: the branch runs for 1 only, as #540 specifies. The tests that still build an HTTP or Slack target or delivery task with no number of attempts (mostly ininternal/delivery/target_redact_test.go,target_headers_test.goandengine_test.go) set the number they mean, 1 for a single attempt. The PR body's judgement call about values below 2 is removed.PR body: "Not in the diff:
databaseandlogtargets are now stored with 1" is wrong. The diff makes that change:newTargetininternal/handlers/target_create.gosets 1 for every type, andtarget_create_test.gochecks it. Acceptable: the PR body lists it with the other changes.Judgement call: two choices are accepted as meeting the definition of done. The add target form shows 1 as a placeholder and stores 1 when the field is left blank, and
databaseandlogtargets are stored with 1.Model: opus-5-5
14a38df093toa69edcdb76a69edcdb76to11fa383503Rework for #547 (comment):
httpCore.delivertakes the single-attempt path for exactly 1, and every test that builds an http or slack target or delivery task now sets the number it means, 1 for a single attempt.databaseandlogtargets being stored with 1 among the other changes, and the judgement call about values below 2 is gone.max_attemptscolumn in001_schema.sqlis nowNOT NULLwith a check constraint refusing a value below 1, mirrored in the model's tag.TestTargetCreate_RefusesZeroAttemptsininternal/database/model_target_test.goshows that saving a target with 0 attempts fails, and every test that saves a target now gives it a number.Model: opus-5-5
PASS: this meets #540 and resolves both findings of the first review, and the
NOT NULLjudgement call holds up, because the check constraint alone would let a missing value through to be read back as 0.Model: opus-5-5