diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index d21a2c8d..51d1e5b4 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -7981,6 +7981,7 @@ gate is the wrong shape, validation of the walk is the right one. > 🔢 **Filed 2026-08-11 -- found by Session C while building #1051; measured and documented rather than fixed, because a floor changes the accepted-configuration set.** Value **4/10** -- Difficulty **2/10** -- _fill-in_. Two halves. (1) **`0` or a negative value loads clean and dead-letters on the FIRST failure** -- the check is `attempts >= max_attempts` against a post-increment count, so `0` means "give up immediately" while reading like "no limit". (2) **There is no TOML or env spelling for retry-forever**: `""`, `none` and `null` all raise `ValidationError`, so that posture is reachable **per-outbound in code-first configuration only**. > Both are conditional on a first deployment: an operator who wrote `retry_max_attempts = 0` intending "unlimited" would get single-attempt dead-lettering, silently. Nothing is misconfigured today; there are zero deployments. Note the interaction with **#1051**, which set the finite default to 100 -- a floor should be decided alongside whether retry-forever needs a config spelling at all. +> **AMENDED 2026-08-14 -- HALF 1 IS SHIPPED; THE ITEM STAYS OPEN ON HALF 2.** PR #383 floors the operator-facing `retry_max_attempts` at `ge=1`, so `0` and negative values are now REFUSED AT LOAD rather than accepted and silently dead-lettering on the first failure. Refused, not clamped: a silently-corrected setting is one the operator never learns was wrong. **The floor is on the OPERATOR-FACING setting only** -- `RetryPolicy(max_attempts=0)` remains a deliberate internal idiom for a permanent no-retry failure (store `mark_failed`), asserted by five live tests, and constraining it would have deleted a used mechanism while claiming to add a guard. `None` stays legal: retry-forever is a documented posture. **HALF 2 -- whether retry-forever needs a TOML or env spelling -- IS UNTOUCHED AND IS A PRODUCT QUESTION**, which this item's own text says should be decided alongside the floor. Banner authored by the Lander under the sanctioned interim, because a builder may not author ledger content (owner ruling 2026-08-13). > ⚠️ **AMENDED 2026-08-13 (dispatcher) -- HALF 1 BUILT on PR #383, HALF 2 OUTSTANDING, ITEM STAYS OPEN. Do not close this on #383.** Banner authored by the dispatcher rather than the builder, per the owner's 2026-08-13 ruling that a builder may not author ledger content. > diff --git a/messagefoundry/config/settings.py b/messagefoundry/config/settings.py index 73b66374..0acf4766 100644 --- a/messagefoundry/config/settings.py +++ b/messagefoundry/config/settings.py @@ -1056,7 +1056,13 @@ class DeliverySettings(_Section): # 100 default (#1051) — a test guards the sync, and the two MUST move together: leaving this at # None would restore retry-forever for every outbound that declares no retry= of its own, which # is the overwhelmingly common shape. - retry_max_attempts: int | None = 100 + # `ge=1` because a configured 0 loaded clean and dead-lettered on the FIRST failure: the delivery + # check is `item.attempts >= max_attempts` against a POST-increment count, so 0 means give up + # immediately while READING like "no limit". `None` is the documented retry-forever posture and + # stays legal. The floor is on the OPERATOR-FACING setting only -- `RetryPolicy(max_attempts=0)` + # remains a deliberate internal idiom for a permanent, no-retry failure (store `mark_failed`), and + # constraining that instead would delete a used mechanism while claiming to add a guard. + retry_max_attempts: int | None = Field(default=100, ge=1) retry_backoff_seconds: float = 5.0 retry_backoff_multiplier: float = 2.0 retry_max_backoff_seconds: float = 300.0 diff --git a/tests/test_settings.py b/tests/test_settings.py index cda62b85..631f7767 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -13,6 +13,7 @@ from messagefoundry.config.settings import ( ApiSettings, AuthSettings, + DeliverySettings, DrSettings, ServiceSettings, SqlAuth, @@ -1273,3 +1274,32 @@ def test_an_ip_literal_origin_is_still_fine_with_no_tls_posture_declared() -> No developer running on 127.0.0.1 is locked out.""" s = ApiSettings(public_origin="http://127.0.0.1:8765", serve_ui=True) assert s.public_origin == "http://127.0.0.1:8765" + + +@pytest.mark.parametrize("bad", [0, -1, -100]) +def test_retry_max_attempts_below_one_is_refused_at_load(bad: int) -> None: # #1217 + """`0` loaded clean and dead-lettered on the FIRST failure, while READING like "no limit". + + The delivery check is ``item.attempts >= max_attempts`` against a POST-increment count, so a + configured `0` means give up immediately -- the opposite of what an operator writing `0` in a + retry field almost certainly intends, and the opposite of what `None` means two lines above it. + A negative value was equally accepted and equally silent. + + Refused at LOAD rather than clamped, because a silently-corrected setting is a setting the + operator never learns was wrong. + """ + with pytest.raises(ValidationError): + DeliverySettings(retry_max_attempts=bad) + + +def test_retry_forever_and_the_finite_default_both_survive_the_floor() -> None: # #1217 + """The negative control, and it guards the two values the floor must NOT touch. + + `None` is retry-forever and is a legitimate, documented posture -- a floor that refused it would + remove a capability while claiming to add a guard. `100` is the finite default #1051 set, and the + settings model mirrors `RetryPolicy`'s; a test already guards that sync and the two must move + together. + """ + assert DeliverySettings(retry_max_attempts=None).retry_max_attempts is None + assert DeliverySettings().retry_max_attempts == 100 + assert DeliverySettings(retry_max_attempts=1).retry_max_attempts == 1