diff --git a/docs/adr/0001-retry-default-stays-env-configured.md b/docs/adr/0001-retry-default-stays-env-configured.md index 8aba4b9..fb19c29 100644 --- a/docs/adr/0001-retry-default-stays-env-configured.md +++ b/docs/adr/0001-retry-default-stays-env-configured.md @@ -1,29 +1,12 @@ # Retry default stays env-configured -**Decision:** `postgres_retry`'s default attempt count keeps coming from -`settings.get_retries_number()` — a call-time read of `DB_RETRY_RETRIES_NUMBER`, defaulting to `3`. -We do not make the default an injectable dependency, and we do not inline `settings.py` into -`retry.py`. - -An architecture review flagged this as a deepening candidate. `settings.py` is a shallow module — -one caller, one line — and the attempt count enters through a process-global env var that is -invisible at the decorator's interface. Two deepenings were proposed: inject a resolver -(`default_retries: Callable[[], int] = settings.get_retries_number`) so env-reading becomes the -default adapter at an explicit seam, or inline the one-liner and delete the module. - -Both were declined, because the deepening pattern pays off when it buys testability or locality you -do not already have, and here both are already present. Testability is solved twice over: a call -site controls the count explicitly with `retries=N`, and the default is controllable with -`monkeypatch.setenv("DB_RETRY_RETRIES_NUMBER", …)`, so there is no hard-to-test symptom to relieve. -`settings.py` is shallow but not a pass-through: it hides a real decision — the env var's **name**, -the `3` **default**, and the **call-time re-read contract** — in one named place, and inlining would -scatter that across `retry.py`, costing locality rather than gaining it. For a library consumed by -applications, a 12-factor env default is the conventional documented configuration surface, listed -in the interface rather than smuggled through it. And the resolver seam would have exactly one -adapter, which is hypothetical, not real: it does not remove the env read, only relocates who -performs it, while widening the public surface. - -**Revisit trigger:** a second source for the default appears — a config file, or a settings object -the package must read — or a real caller needs to set the default programmatically, neither via env -nor via a per-call-site `retries=`. Either turns the resolver into a seam with two adapters rather -than one. +A bare `@postgres_retry` resolves its attempt count inside `wrapped_method` on every call, from +`settings.get_retries_number()`, a read of `DB_RETRY_RETRIES_NUMBER` defaulting to `3`; a call site +that wants its own bound passes `retries=N`. A review proposed two deepenings of the one-line +`settings` module: inject a resolver so the env read becomes an explicit default adapter, or inline +the function and delete the module. Both were declined. The env var is this library's whole +configuration surface, and the per-call re-read is the point, because an operator lowering it during +an incident expects the retry storm to stop without a restart. A resolver would relocate the env +read rather than remove it, with exactly one adapter, and inlining would scatter the var's name, the +`3` and the re-read contract across `retry.py`. A second source for the default, a config file or a +caller that must set it programmatically, is what would make the seam real. diff --git a/docs/adr/0002-retriable-taxonomy-is-asyncpg-classes.md b/docs/adr/0002-retriable-taxonomy-is-asyncpg-classes.md index 6444711..aa3fcb3 100644 --- a/docs/adr/0002-retriable-taxonomy-is-asyncpg-classes.md +++ b/docs/adr/0002-retriable-taxonomy-is-asyncpg-classes.md @@ -1,22 +1,13 @@ # The retriable taxonomy is a tuple of asyncpg classes, not SQLSTATE data -**Decision:** `RETRIABLE_ASYNCPG_ERRORS` stays a flat tuple of asyncpg exception classes consumed -by a single `isinstance`. We do not model the taxonomy as richer data — a mapping of SQLSTATE code -to rationale, or a table the predicate looks a code up in. - -The proposal recurs because the tuple reads as a bare list of names: the codes that motivate it -(`40001`, class `08`) appear nowhere near it, so a reader cannot see *why* those two and not their -neighbours. Making it data would put the code and the reason next to the entry. - -It was declined because asyncpg's exception hierarchy already **is** the SQLSTATE taxonomy. Every -class carries its own `sqlstate`, and subclassing tracks the code's class — `ConnectionDoesNotExistError` -(`08003`) is a subclass of `PostgresConnectionError` (`08000`), so naming the parent covers the whole -class for free and keeps covering it when asyncpg adds a member. A parallel SQLSTATE table would -restate a mapping asyncpg maintains, and would drift from it silently: the `isinstance` check cannot -disagree with asyncpg about which code an exception carries, but a hand-written table can, and would -lose subclass coverage the moment it did. The rationale that the table was meant to hold has a home -that cannot drift — the `INVARIANT:` docstring on the test asserting the boundary, which is executed. - -**Revisit trigger:** the predicate needs to branch on something asyncpg's class hierarchy does not -encode — a per-error retry budget, a different backoff per code, or a code that is retriable only in -some contexts. A second axis is what the tuple genuinely cannot carry. +`RETRIABLE_ASYNCPG_ERRORS` is a flat tuple of asyncpg exception classes, consumed by the single +`isinstance` in `_is_retriable_link`. Modelling it as data recurs as a proposal, because the tuple +reads as a bare list of names with the codes that motivate it (`40001`, class `08`) nowhere near it. +It was declined because asyncpg's hierarchy already is the SQLSTATE taxonomy: every class carries +its own `sqlstate`, and subclassing tracks the code class, so naming `PostgresConnectionError` +(`08000`) covers `ConnectionDoesNotExistError` (`08003`) for free and keeps covering members asyncpg +adds later. A hand-written table would restate a mapping asyncpg maintains and drift from it +silently, losing subclass coverage as it did; the rationale it was meant to hold lives instead in +the executed `INVARIANT:` docstring in `tests/test_retriable.py` that pins the boundary against +`StatementCompletionUnknownError` (`40003`). Only a second axis the hierarchy does not encode, a +per-error retry budget or a different backoff per code, is beyond what the tuple can carry.