diff --git a/docs/adr/0001-fake-broker-keeps-immediate-delivery.md b/docs/adr/0001-fake-broker-keeps-immediate-delivery.md index ba86754..0e39ef3 100644 --- a/docs/adr/0001-fake-broker-keeps-immediate-delivery.md +++ b/docs/adr/0001-fake-broker-keeps-immediate-delivery.md @@ -1,46 +1,12 @@ -# The fake broker keeps immediate delivery; no in-memory `TimerStore` - -**Decision:** `TestTimersBroker` keeps its current model — the fake producer encodes and dispatches -every published Timer immediately to the handler via `process_message`, regardless of its Activation -time. We do **not** add an `InMemoryTimerStore` adapter or a `TimerStore` `Protocol` for testing. - -## Context - -An architecture review proposed that "the fake fakes the wrong layer": put a real -`InMemoryTimerStore` behind the `TimerStore` seam so the fake broker exercises real Claim / Lease / -Commit and truthful inspection, instead of stubbing the Redis client and dispatching immediately. -Three defects were claimed: (1) the fake duplicates the envelope encode; (2) `has_pending`, -`get_pending_timers` and `cancel_all` are stubbed to canned empties ("lies"); (3) at-least-once and -Lease semantics cannot be tested through the fake. The options weighed were: A — drive the *real* -subscriber poll loop against an in-memory store; B — a real in-memory store for state with -deterministic delivery; or drop it. - -## Decision & rationale - -Researching FastStream's own Redis broker (`faststream/redis/testing.py`) dismantled the premises: - -- **The encode is not duplicated.** FastStream's `FakeProducer.publish` also encodes via - `build_message` / `message_format.encode` — encoding is inherent to the fake-producer pattern, not - a smell. `FakeTimersProducer` mirrors it. -- **The inspection stubs are not lies.** FastStream fakes *every* subscriber — including the polling - list subscriber, our closest analog — by bypassing the poll loop and calling - `handler.process_message` directly from the fake producer; it never runs the real loop in tests. - Under an immediate-delivery contract a published Timer has already fired and been removed, so - `has_pending → False`, `get_pending_timers → []` and `cancel_all → 0` are *truthful*, not stubbed - lies. -- **Option A is non-idiomatic and fragile.** Running the real `_consume` loop under FastStream's - `TestBroker` means skipping `_fake_start` — forfeiting the handler-mock wiring every FastStream - test broker relies on — and driving an infinite 50 ms poll loop inside a unit test; a spike doing - so hung at teardown. - -That leaves only one way to make inspection report *pending* future Timers: stop delivering future -Timers immediately. That is a **breaking change** to a public testing API — existing user tests -publish a future Timer and expect their handler to fire without waiting — so it is rejected. With -immediate delivery retained the inspection methods are already correct, and the proposal has no -remaining defect to fix. `scheduled_timers` already lets users assert what was Scheduled. - -**Revisit trigger:** a concrete need to test Schedule / Pending / cancel semantics — a future Timer -observed as Pending before it fires — through the fake. If that arises, prefer an **opt-in** -`TestTimersBroker(..., respect_activation=True)` (default off, so non-breaking) that withholds -not-yet-Due Timers; not a change to the default immediate-delivery contract, and not a real -subscriber loop in tests. +# The fake broker delivers every Timer immediately + +`FakeTimersProducer` encodes each published Timer and hands it straight to the matching subscriber's +`process_message`, ignoring the Activation time, and `_patch_broker` stubs the Redis client so +`has_pending` is False, `get_pending_timers` empty and `cancel_all` zero. Putting a real in-memory +store behind the `TimerStore` seam, so the fake exercised Claim, Lease and Commit, was rejected: +FastStream fakes every subscriber, its polling list subscriber included, by bypassing the poll loop, +and under immediate delivery a published Timer has already fired, so those inspection results are +truthful rather than canned. Driving the real `_consume` loop under `TestBroker` means skipping +`_fake_start` and running a 50 ms poll loop inside a unit test; a spike doing so hung at teardown. +Reporting a future Timer as Pending requires withholding it, which breaks every user test that +publishes one and expects its handler to fire, so it could only arrive as an opt-in flag. diff --git a/docs/adr/0002-keep-construction-factories.md b/docs/adr/0002-keep-construction-factories.md deleted file mode 100644 index 3e6c3f0..0000000 --- a/docs/adr/0002-keep-construction-factories.md +++ /dev/null @@ -1,40 +0,0 @@ -# Keep the construction factories; don't inline them into the registrator - -**Decision:** `publisher/factory.py` (`create_publisher`) and `subscriber/factory.py` -(`create_subscriber`) stay as-is. We do **not** inline their construction into -`TimersRegistrator.publisher()` / `.subscriber()`. - -## Context - -An architecture review observed that both factories are pure constructor-bundling — no validation, -no branching, no logic — and each has exactly one caller (the matching registrator method, which -already forwards nearly all the same arguments). By the deletion test they are pass-throughs: -inlining them *moves* the construction lines into the registrator rather than *concentrating* -complexity. The option weighed was to inline both and delete the two files: a small locality win, -two fewer files, one fewer hop. - -## Decision & rationale - -Keep them. The factories are shallow **because our domain is simple**, not because the structure is -wrong: - -- FastStream's own Redis broker carries the same `publisher/factory.py` + `subscriber/factory.py` - shape, where `create_subscriber` validates options and selects among roughly nine subscriber types - (Channel / List / Stream × Batch / Concurrent). This project deliberately mirrors FastStream's - Redis structure. We have one subscriber type and one publisher type today, so there is nothing to - validate or select — but the seam is the natural place that logic would live if it arrived, and - keeping it preserves structural parity with the framework a contributor already knows. -- The inlining win is genuinely small (locality plus two deleted files), and it buys that by - *diverging* from the upstream structure. The trade isn't worth it while the factories cost - essentially nothing to keep. - -This is the opposite call from the fake-broker proposal in -[ADR-0001](0001-fake-broker-keeps-immediate-delivery.md), which was rejected because its premises -were false. Here the premise — the factories are shallow — is *true*; we simply judge the cleanup -not worth the divergence. - -**Revisit trigger:** structural parity with FastStream stops being a goal while the factories are -still single-type — then inlining them is worth reconsidering. A second timer subscriber or -publisher type (batch timers, an alternative polling strategy) is *not* a revisit trigger in the -other direction: at that point the factory starts validating and selecting, and this decision is -simply confirmed. diff --git a/docs/adr/0002-no-per-call-middlewares-kwarg.md b/docs/adr/0002-no-per-call-middlewares-kwarg.md new file mode 100644 index 0000000..3cf4a46 --- /dev/null +++ b/docs/adr/0002-no-per-call-middlewares-kwarg.md @@ -0,0 +1,14 @@ +# No per-call `middlewares=` on subscribers, publishers and routes + +`TimersRegistrator.subscriber()`, `.publisher()`, `TimersRoute` and `TimersRoutePublisher` take no +`middlewares=` keyword; `TimersBroker` and `TimersRouter` keep theirs, and broker or router scope is +the only way to wrap timer handling. FastStream 0.7 removed `add_call(middlewares_=...)`, so the +forwarding target no longer exists. Routing the kwarg to broker scope internally would have been a +silent change of meaning, since broker middleware runs for every Topic, and re-implementing +per-subscriber middleware locally would reproduce what upstream had just deleted while binding us to +an internal we had stopped depending on. The package was at version `"0"` with no stability promise, +so a hard break users see at import time was in policy. Behaving exactly like the brokers FastStream +ships is this integration's value, which is also why the single-caller `create_publisher` and +`create_subscriber` factories stay rather than being inlined into the registrator: they mirror +FastStream's own Redis broker layout. If upstream restores call-scoped middleware the kwarg returns +as a thin forward. diff --git a/docs/adr/0003-no-per-call-middlewares-kwarg.md b/docs/adr/0003-no-per-call-middlewares-kwarg.md deleted file mode 100644 index bb4165e..0000000 --- a/docs/adr/0003-no-per-call-middlewares-kwarg.md +++ /dev/null @@ -1,35 +0,0 @@ -# No per-call `middlewares=` on subscribers, publishers and routes - -**Decision:** `TimersRegistrator.subscriber()`, `TimersRegistrator.publisher()`, `TimersRoute` and -`TimersRoutePublisher` take no `middlewares=` keyword. Middleware is configured at broker or router -scope only. - -## Context - -The package exposed a per-call `middlewares=` kwarg that forwarded to FastStream's -`add_call(middlewares_=…)`. FastStream 0.7 removed that parameter upstream, so the forwarding target -no longer exists. Migrating to 0.7 forced a choice, and the kwarg was dropped rather than preserved. -Two alternatives were weighed and both lose: - -- **Keep the kwarg, route it to broker scope internally.** Semantically wrong: broker-scope - middleware runs for every Topic, not the one the kwarg was attached to. A silent change of meaning - is worse for a user than a hard break they see at import time. -- **Re-implement per-subscriber middleware locally.** This reproduces behaviour upstream had just - removed, and carries the ongoing cost of tracking a framework internal we had deliberately stopped - depending on. - -The package was at version `"0"` with no stability promise when this landed, so a hard break was in -policy provided it was stated in the release. - -## Decision & rationale - -Follow upstream. This package is a FastStream broker integration and its value is behaving exactly -like the brokers FastStream ships; carrying a call-scoped middleware feature the framework no longer -has would make it the odd one out for every contributor and every user reading FastStream's own -documentation. `middlewares=` on `TimersBroker` and `TimersRouter` is unaffected — those are the -broker- and router-scope hooks FastStream still supports, and they remain the way to wrap timer -handling. - -**Revisit trigger:** FastStream reintroduces call-scoped middleware, under any spelling. At that -point the kwarg returns as a thin forward to whatever upstream provides — never as a local -re-implementation. diff --git a/docs/adr/0003-typing-extensions-over-version-gated-imports.md b/docs/adr/0003-typing-extensions-over-version-gated-imports.md new file mode 100644 index 0000000..8715e19 --- /dev/null +++ b/docs/adr/0003-typing-extensions-over-version-gated-imports.md @@ -0,0 +1,13 @@ +# Backport 3.12-only typing via `typing_extensions`, not version-gated imports + +Lowering the supported floor from 3.13 to 3.11 broke exactly two constructs: the PEP 695 +`type RedisClient = ...` alias, a `SyntaxError` on 3.11, and `override` imported from `typing`, +which landed in 3.12. The alias became a plain `typing.TypeAlias` annotation, which needs no +backport; `override` is imported from `typing_extensions` unconditionally, never gated on +`sys.version_info`, and `typing-extensions` is a declared direct runtime dependency rather than a +pin borrowed from FastStream, which is free to drop it. A `sys.version_info` gate was rejected +because 3.11 needs the package installed either way, so it buys only more code at every affected +site and a second path for `ty` to check. Dropping `@override` was rejected because it is what +catches a signature drift against FastStream's base classes, precisely the failure this integration +is exposed to. When the floor rises to 3.12 both constructs gain stdlib spellings and the direct +dependency goes with them. diff --git a/docs/adr/0004-typing-extensions-over-version-gated-imports.md b/docs/adr/0004-typing-extensions-over-version-gated-imports.md deleted file mode 100644 index c88f198..0000000 --- a/docs/adr/0004-typing-extensions-over-version-gated-imports.md +++ /dev/null @@ -1,39 +0,0 @@ -# Backport 3.12-only typing via `typing_extensions`, not version-gated imports - -**Decision:** `override` is imported from `typing_extensions` unconditionally, never gated on -`sys.version_info`, and `typing-extensions` is a declared **direct** runtime dependency rather than -a borrowed transitive one. The PEP 695 `type RedisClient = …` alias became a plain -`typing.TypeAlias` annotation, which needs no backport at all. - -## Context - -Lowering the supported-Python floor from 3.13 to 3.11 hit exactly two constructs that 3.11 cannot -parse or import: the PEP 695 `type` alias (a `SyntaxError` on 3.11) and `override` imported from -`typing` (an `ImportError` on 3.11, since it landed in 3.12). The alias had a stdlib answer — -`typing.TypeAlias` has existed since 3.10 — so only `override` needed a backport. Everything else -in the package was verified to work at the new floor: `typing.Self` and `datetime.UTC` both exist -in 3.11, and `datetime.UTC` is what fixes 3.11 as the floor rather than something lower. - -Two alternatives were weighed for `override`: - -- **`sys.version_info`-gated stdlib imports** — take `typing.override` on 3.12+ and the - `typing_extensions` one below it. This is more code at every affected site, and it does not - remove the dependency: 3.11 still needs `typing_extensions` installed, so the gate buys nothing - beyond a marginally shorter import on newer interpreters. -- **Drop `@override` entirely** — the problem disappears if the package stops using it. Rejected: - `@override` is what catches an override-mismatch against FastStream's base classes when upstream - renames or re-signatures a method, which is precisely the failure this integration is exposed to. - -## Decision & rationale - -`typing_extensions` was already resolved transitively — FastStream pins `>=4.12.0`, and `override` -has been in `typing_extensions` since 4.4.0 — so declaring it directly costs nothing at install -time and makes the reliance explicit instead of borrowing a transitive pin that FastStream is free -to drop. The unconditional import is the simplest form that is correct on every supported -interpreter, and it keeps a single code path for `ty` to check rather than one path per interpreter -version. - -**Revisit trigger:** the supported floor rises to 3.12 or above, at which point both constructs -have stdlib spellings on every supported interpreter and the direct `typing-extensions` dependency -can be dropped in the same change. A new 3.12+-only construct arriving before then is *not* a -revisit trigger — it takes the same `typing_extensions` treatment. diff --git a/docs/agents/domain.md b/docs/agents/domain.md index 8830541..43f91f7 100644 --- a/docs/agents/domain.md +++ b/docs/agents/domain.md @@ -18,7 +18,7 @@ Single-context repo: ├── CONTEXT.md ├── docs/adr/ │ ├── 0001-fake-broker-keeps-immediate-delivery.md -│ └── 0002-keep-construction-factories.md +│ └── 0002-no-per-call-middlewares-kwarg.md └── faststream_redis_timers/ ``` @@ -32,4 +32,5 @@ If the concept you need isn't in the glossary yet, that's a signal: either you'r If your output contradicts an existing ADR, surface it explicitly rather than silently overriding: -> _Contradicts ADR-0001 (fake broker keeps immediate delivery), but worth reopening because…_ +> _Contradicts ADR-0001 (the fake broker delivers every Timer immediately), but worth reopening +> because…_