From c28421ce56e50f788af54da2637e2f1a7f03223a Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Tue, 25 Aug 2026 13:45:51 -0500 Subject: [PATCH] docs(configuration): the retry-cap row still promised a zero that the loader refuses (BACKLOG #1217, half 1) PR #383 floored the operator-facing `retry_max_attempts` at `ge=1`. The catalog row in docs/CONFIGURATION.md was not moved with it and still asserted: `0` or a negative value is accepted and dead-letters on the first failure That has been false since #383. A catalog row describing a configuration the loader REFUSES is worse than silence: an operator writes it, the start fails, and the document that sent them there still reads as authoritative. DRIVEN, NOT READ OFF THE FIELD DECLARATION: retry_max_attempts = 0 REJECTED = -1 REJECTED = 1 LOADS -> 1 = 100 LOADS -> 100 = None LOADS -> None key omitted LOADS -> 100 RetryPolicy(max_attempts=0) STILL LEGAL The row now says what the loader does, and keeps naming the internal idiom the floor deliberately does NOT touch. THAT LAST CLAUSE IS THE POINT OF THE WHOLE ITEM AND IT IS EASY TO DELETE BY ACCIDENT. The obvious "remaining half" -- adding `ge=1` to `models.py` -- looks like the symmetrical completion of the same tightening and would instead DELETE A USED MECHANISM: `RetryPolicy(max_attempts=0)` is the deliberate idiom for a permanent, no-retry failure and FOUR test modules depend on it (test_batch_completion, test_postgres_store, test_resend, test_sqlserver_store). settings.py anticipates the mistake in its own comment; the new `test_the_internal_no_retry_idiom_is_untouched_by_the_operator_facing_floor` makes that executable rather than advisory. THE PROSE TEST IS NEGATIVE ON PURPOSE. It asserts the false CLAIM cannot come back, not that the new sentence matches word for word. An exact-sentence assertion reds on any rewording, which trains the next author to edit the test instead of the doc. RED-FIRST: restoring the old sentence verbatim reds `test_the_configuration_catalog_does_not_still_promise_the_pre_floor_behaviour`, 1 of 15 in that selection, with the anchor asserted unique and the file hash asserted changed before scoring, then restored byte-identical. HALF 2 IS NOT IN THIS COMMIT AND IS NOT MINE TO DECIDE. Whether the retry-forever posture needs a TOML or env spelling is a product question -- the row says choosing the spelling IS the item. Measured here rather than quoted, every plausible spelling driven through the real model: 0, -1, "", "none", "null", "forever", "inf" ALL REJECTED key omitted -> 100 python None -> None <- code-first ONLY So retry-forever is reachable in code-first configuration and nowhere else, because TOML has no null literal. The row recommends a string spelling coerced by a field validator; adopting one changes the accepted-configuration set, so it is routed for a ruling rather than settled in a build. Verification, with scope: tests/test_retry_cap_default.py, tests/test_delivery_settings.py and tests/test_settings.py, on .venv/Scripts/python.exe (CPython 3.14.6 non-freethreaded, seven CI extras). ruff 0.15.22 (== the constraints.lock pin) check and format --check clean. NOT the full suite. No test was removed or weakened. The ledger row is not mine to author and is not in this commit; #1217 stays OPEN on half 2. Co-Authored-By: Claude Opus 5 --- docs/CONFIGURATION.md | 2 +- tests/test_retry_cap_default.py | 65 +++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 1 deletion(-) diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index 328f646d8..e077fe83c 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -779,7 +779,7 @@ Because the local diff is cheap and PHI-safe it is **on by default** (zero phone ### `[delivery]` | Key | Type | Default | Notes | |---|---|---|---| -| `retry_max_attempts` | int | `100` | attempts before a delivery dead-letters. **Finite by default** (BACKLOG #1051): 100 attempts under the backoff below is a 28,215 s / 7 h 50 m 15 s window, long enough to ride out a partner outage without letting a lane wedge indefinitely. Safe as a default because attempts are counted **per row** (an outage burns the cap on roughly the lane heads, not the backlog) and an exhausted row **dead-letters into the replayable DLQ**. Raise or lower it freely, but note the two edges: **there is no TOML or env spelling for retry-forever** — this key takes an integer, and `""`/`none`/`null` are load errors — so a never-give-up posture is a per-outbound, code-first `retry=RetryPolicy(max_attempts=None)` (under FIFO that head then blocks its lane until it succeeds or is purged); and **`0` or a negative value is accepted and dead-letters on the first failure**, since the check is `attempts >= max_attempts` against a post-increment count. A permanent `AR` reject fails fast regardless. | +| `retry_max_attempts` | int | `100` | attempts before a delivery dead-letters. **Finite by default** (BACKLOG #1051): 100 attempts under the backoff below is a 28,215 s / 7 h 50 m 15 s window, long enough to ride out a partner outage without letting a lane wedge indefinitely. Safe as a default because attempts are counted **per row** (an outage burns the cap on roughly the lane heads, not the backlog) and an exhausted row **dead-letters into the replayable DLQ**. Raise or lower it freely, but note the two edges: **there is no TOML or env spelling for retry-forever** — this key takes an integer, and `""`/`none`/`null` are load errors — so a never-give-up posture is a per-outbound, code-first `retry=RetryPolicy(max_attempts=None)` (under FIFO that head then blocks its lane until it succeeds or is purged); and **`0` or a negative value is now REFUSED at load** (`ge=1`, BACKLOG #1051). It used to be accepted and dead-letter on the FIRST failure — the check is `attempts >= max_attempts` against a post-increment count, so `0` gave up immediately while *reading* like "no limit". That is why the floor is on this key rather than a documentation note. **The floor is on this operator-facing setting ONLY:** the code-first `retry=RetryPolicy(max_attempts=0)` stays legal and is the deliberate idiom for a permanent, no-retry failure. A permanent `AR` reject fails fast regardless. | | `retry_backoff_seconds`, `retry_backoff_multiplier`, `retry_max_backoff_seconds` | num | 5 / 2 / 300 | exponential backoff between attempts (per-outbound `retry=` overrides) | | `ordering` | enum | `fifo` | default queue ordering per outbound: `fifo` (strict in-order, head-of-line on failure) or `unordered` (batch + rotate-past-failures). Per-outbound `ordering=` overrides. | | `internal_error` | enum | `continue` | what a delivery worker does on an **internal/code error** (a non-`DeliveryError` exception from `send` — our bug, not the partner's): `continue` (dead-letter the row + advance) or `stop` (halt the connection's worker, preserve the message for replay, raise a `connection_stopped` alert). Per-outbound `internal_error=` overrides. Partner NAKs / transport failures are unaffected. | diff --git a/tests/test_retry_cap_default.py b/tests/test_retry_cap_default.py index e6d798343..7bda4b100 100644 --- a/tests/test_retry_cap_default.py +++ b/tests/test_retry_cap_default.py @@ -26,7 +26,10 @@ from __future__ import annotations +from pathlib import Path + import pytest +from pydantic import ValidationError from messagefoundry.config.models import ContentType, OrderingMode, RetryPolicy from messagefoundry.config.settings import DeliverySettings @@ -208,3 +211,65 @@ def test_the_refusal_still_fires_on_an_explicit_retry_forever() -> None: reg, delivery=DeliverySettings(ordering=OrderingMode.UNORDERED, retry_max_attempts=None), ) + + +# --- BACKLOG #1217: the catalog row must describe the loader that ships ---------------------- +# +# The floor landed in PR #383 and the docs/CONFIGURATION.md row was not moved with it, so the +# catalog asserted "`0` or a negative value is accepted and dead-letters on the first failure" +# about a loader that had started REFUSING both. A catalog row describing a configuration the +# loader refuses is worse than silence: an operator writes it, the start fails, and the document +# that sent them there still reads as authoritative. +# +# These drive the REAL settings model rather than re-reading the Field, and then check the prose +# against that behaviour, so the two cannot drift apart again silently. + + +@pytest.mark.parametrize("value", [0, -1, -100]) +def test_the_operator_facing_retry_cap_refuses_zero_and_negatives(value: int) -> None: + """The floor itself, driven rather than read off the Field declaration.""" + with pytest.raises(ValidationError): + DeliverySettings(retry_max_attempts=value) + + +@pytest.mark.parametrize("value", [1, 100, None]) +def test_the_floor_does_not_narrow_what_was_already_legal(value: int | None) -> None: + """The other direction. A floor that also refuses `None` would delete the documented + retry-forever posture, and a suite that only asserts refusals could not tell the two apart.""" + assert DeliverySettings(retry_max_attempts=value).retry_max_attempts == value + + +def test_the_internal_no_retry_idiom_is_untouched_by_the_operator_facing_floor() -> None: + """THE ONE THAT MUST NOT BE 'TIDIED'. `RetryPolicy(max_attempts=0)` is the deliberate idiom for + a permanent, no-retry failure and FOUR test modules depend on it -- test_batch_completion, + test_postgres_store, test_resend, test_sqlserver_store. + + Adding `ge=1` to the RetryPolicy field looks like the symmetrical completion of the same + tightening and would instead DELETE A USED MECHANISM. settings.py says so in its own comment; + this makes it executable. + """ + assert RetryPolicy(max_attempts=0).max_attempts == 0 + + +def test_the_configuration_catalog_does_not_still_promise_the_pre_floor_behaviour() -> None: + """BACKLOG #1217. Pins the PROSE against the behaviour asserted above. + + Deliberately negative rather than matching the new wording: an exact-sentence assertion would + red on any rewording, which trains the next author to edit the test instead of the doc. What + must never come back is the CLAIM that a zero loads. + """ + doc = (Path(__file__).resolve().parents[1] / "docs" / "CONFIGURATION.md").read_text( + encoding="utf-8" + ) + row = next((ln for ln in doc.splitlines() if ln.startswith("| `retry_max_attempts`")), None) + assert row is not None, "the retry_max_attempts catalog row has moved or been renamed" + + assert "is accepted and dead-letters on the first failure" not in row, ( + "the catalog again promises that a zero loads. It has not since PR #383 -- the loader " + "refuses it, which the tests above drive directly." + ) + assert "REFUSED at load" in row, "the row must say what the loader actually does with a zero" + assert "RetryPolicy(max_attempts=0)" in row, ( + "the row must keep naming the internal idiom the floor deliberately does NOT touch, or a " + "later reader completes the tightening and deletes it" + )