From 22280a496789a1cd86142c2b7a3243042351239d Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 13 Aug 2026 20:08:10 -0500 Subject: [PATCH 1/2] fix(delivery): floor retry_max_attempts at 1 so a configured 0 cannot mean give-up-now (BACKLOG #1217) `retry_max_attempts = 0` loaded clean and dead-lettered on the FIRST failure. The delivery check is `item.attempts >= max_attempts` (wiring_runner.py:5040) against a POST-increment count, so 0 means give up immediately -- the opposite of what an operator writing 0 in a retry field intends, and the opposite of what `None` means two lines above it in the same model. Negative values were equally accepted and equally silent. REFUSED AT LOAD, NOT CLAMPED. A silently-corrected setting is one the operator never learns was wrong. THE FLOOR IS ON THE OPERATOR-FACING SETTING ONLY, AND THAT IS THE WHOLE CARE HERE. `RetryPolicy(max_attempts=0)` is a DELIBERATE internal idiom meaning a permanent, no-retry failure, used by store `mark_failed` and asserted by five live tests -- test_batch_completion.py:206 says it outright: "max_attempts=0 => the batch dead-letters, which is the path that finalizes every member", and three store tests use it for a permanent AR. Constraining RetryPolicy instead would have broken five tests and DELETED A USED MECHANISM while claiming to add a guard. Those five are green and untouched. THE ITEM DEFERRED THIS BECAUSE "a floor changes the accepted-configuration set". Under CLAUDE.md section 0 that cost is vacuous: zero deployments, so there is no accepted configuration to break and no migration to stage. Prefer the simple correct end state. NEGATIVE CONTROL SHIPPED, guarding the two values the floor must NOT touch: `None` stays legal (retry-forever is a documented posture, and a floor refusing it would remove a capability while claiming to add a guard) and the finite default stays at the value #1051 set, which a separate test already guards for sync with RetryPolicy. Red-first: 0, -1 and -100 all failed with DID NOT RAISE ValidationError before the change. HANDED BACK, NOT DECIDED: a TOML/env spelling for retry-forever. Whether that posture needs a config spelling is a product question the item says should be decided alongside the floor. Flagged for the dispatcher; it did not block the floor. Verified, scope stated: ruff format and ruff check clean; mypy strict clean on config/settings.py; test_settings + test_communications_inventory 197 passed; test_batch_completion + test_resend 27 passed 18 skipped. FULL SUITE NOT RUN. Note for the merge: settings.py is concurrently edited on another live branch at a different region (their tip leaves this line unchanged). Checked before committing. --- messagefoundry/config/settings.py | 8 +++++++- tests/test_settings.py | 30 ++++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) 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 From 5e0a2a4227b081f9d3a60d6667a18e550337d163 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 13 Aug 2026 23:06:06 -0500 Subject: [PATCH 2/2] backlog: record #1217 half 1 as shipped; the item stays OPEN on half 2 PR #383 floors the operator-facing `retry_max_attempts` at `ge=1`. #1217 has TWO halves and only the first is done, so this is an amendment and NOT a closure. half 1 SHIPPED 0 and negatives refused AT LOAD rather than silently dead-lettering on the first failure half 2 OPEN whether retry-forever needs a TOML/env spelling -- a product question this item's own text says to decide alongside the floor Control delta is 0 / 0 / 0, which is the point: a body amendment must move nothing. #1217 `is_open` stays True, verified with parse_items via `.is_open` -- the only boolean; `.open` and `.closed` are lists of banner characters and a truth test on them silently answers the wrong question. Authored by the Lander under the sanctioned interim: a builder may not author ledger content (owner ruling 2026-08-13), and the CI gate requires the PR's own diff to update BACKLOG.md, so a compliant builder PR cannot make itself green. --- docs/BACKLOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 069e14fe..0b12e5c2 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -7931,6 +7931,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). **Cluster:** Connections and Transports. **Priority:** P3. **Verdict:** build. **Severity:** minor.