Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/CONFIGURATION.md
Original file line number Diff line number Diff line change
Expand Up @@ -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. |
Expand Down
65 changes: 65 additions & 0 deletions tests/test_retry_cap_default.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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"
)
Loading