From 208e815d2bd2071a71e8fefc6f6bbaec11e78771 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Sat, 19 Sep 2026 14:11:17 +0300 Subject: [PATCH] docs(adr): compress to 2 records in the domain-modeling format Drops 0001 (aio coverage flake fixed in the test): a two-line `# pragma: no cover` is cheap to reverse and the reason already sits in the test as a comment. Keeps 0002 and 0003 unmerged - they are separate boundaries - rewritten as one paragraph each and renumbered 0001 and 0002. No merges. --- ...01-aio-coverage-flake-fixed-in-the-test.md | 40 ------------------- ...protobuf-request-message-not-a-provider.md | 12 ++++++ ...protobuf-request-message-not-a-provider.md | 23 ----------- ...ing-servicer-methods-must-be-generators.md | 12 ++++++ ...ing-servicer-methods-must-be-generators.md | 25 ------------ docs/agents/domain.md | 7 ++-- 6 files changed, 28 insertions(+), 91 deletions(-) delete mode 100644 docs/adr/0001-aio-coverage-flake-fixed-in-the-test.md create mode 100644 docs/adr/0001-protobuf-request-message-not-a-provider.md delete mode 100644 docs/adr/0002-protobuf-request-message-not-a-provider.md create mode 100644 docs/adr/0002-streaming-servicer-methods-must-be-generators.md delete mode 100644 docs/adr/0003-streaming-servicer-methods-must-be-generators.md diff --git a/docs/adr/0001-aio-coverage-flake-fixed-in-the-test.md b/docs/adr/0001-aio-coverage-flake-fixed-in-the-test.md deleted file mode 100644 index 26ee313..0000000 --- a/docs/adr/0001-aio-coverage-flake-fixed-in-the-test.md +++ /dev/null @@ -1,40 +0,0 @@ -# Fix the aio coverage flake in the test, not in CI or the gate - -**Decision:** mark the two lines after `server.stop(0)` in -`test_aio_app_finalizer_runs_on_root_close` with `# pragma: no cover`, rather than adding CI-level -retries, relaxing `--cov-fail-under=100` for Python 3.11, or shifting the blind spot with a -scheduling checkpoint. - -`checks / pytest (3.11)` intermittently failed CI's 100%-coverage gate, always on the same two lines -of that test, despite all tests passing. It reproduced directly on GitHub Actions (1 failure in the -original run plus 1 more within 5 reruns — roughly 1-in-4, not a rare one-off) and never locally on -macOS in 25 attempts. It traces to -[coveragepy#2124](https://github.com/coveragepy/coveragepy/issues/2124): on Python 3.11, -coverage.py's tracer can lose the lines immediately following an `await` that internally cancels a -task and catches `CancelledError`. `grpc.aio`'s `Server.stop()` does exactly that to its own -background tasks, so any test code placed right after `await server.stop(0)` sits in the blind spot. -The bug is open upstream with no released fix, and the issue thread notes -`--concurrency=thread,greenlet` does not work around it. - -Five options were on the table: retry the pytest CI step once on failure; lower `--cov-fail-under` -for the 3.11 job specifically; pin a coverage.py version once the upstream bug is fixed; add a -scheduling checkpoint (`await asyncio.sleep(0)`) after `server.stop(0)` so the blind spot lands on a -throwaway line; or pragma-exclude the affected lines. - -The checkpoint was tried **first** and rejected on evidence — it reproduced the flake again on CI -within 2 rerun attempts. The original failure showed a 2-line miss, not 1, so the blind spot's width -is not fixed; moving it just relocates which line fails, and `--cov-fail-under=100` fails on any -missed line. `# pragma: no cover` is honored by coverage.py's static source parser, not the runtime -tracer, so it is structurally immune to the race regardless of how many lines it swallows on a given -run — confirmed locally by the tracked statement count for `tests/test_aio.py` dropping from 107 to -105. - -Retrying would mask any *other* flaky test that happens to fail for a real reason, and keeps the -flake latent forever. Lowering the gate permanently weakens the 100%-coverage guarantee for one -Python version over a bug that has nothing to do with real coverage. Pinning a fixed coverage -version is not available: the upstream fix is an open PR, not a release. All three remain reasonable -fallbacks if this test-side fix turns out not to generalize. - -**Revisit trigger:** the coverage gate flakes again on a *different* test or different lines — -meaning this diagnosis does not fully explain the bug — or coveragepy ships a released fix for -#2124, at which point the pragmas come out and this record is superseded. diff --git a/docs/adr/0001-protobuf-request-message-not-a-provider.md b/docs/adr/0001-protobuf-request-message-not-a-provider.md new file mode 100644 index 0000000..9a2254f --- /dev/null +++ b/docs/adr/0001-protobuf-request-message-not-a-provider.md @@ -0,0 +1,12 @@ +# The protobuf request message is not exposed as a provider + +`grpc_context_provider` binds `grpc.ServicerContext` at `Scope.REQUEST` and nothing else, even +though every unary-request RPC has its protobuf `Message` in hand when the child container is built. +Binding it would force a `protobuf` import to declare the provider's `bound_type`, adding a runtime +dependency to a package that otherwise needs only `grpcio` and `modern-di`, and buying nothing: gRPC +already hands the request to the servicer method as its first positional argument. It would also be +uneven, because client-streaming RPCs receive an iterator rather than a single message, so the +provider would resolve for two of the four RPC types and raise for the other two. `ServicerContext` +is present for all four, and being the only connection provider is why `_build_child` calls +`integrations.bind` directly rather than `classify_connection`. A request message needed +transitively, several provider edges deep, is the case that would reopen this. diff --git a/docs/adr/0002-protobuf-request-message-not-a-provider.md b/docs/adr/0002-protobuf-request-message-not-a-provider.md deleted file mode 100644 index 565285e..0000000 --- a/docs/adr/0002-protobuf-request-message-not-a-provider.md +++ /dev/null @@ -1,23 +0,0 @@ -# The protobuf request message is not exposed as a provider - -**Decision:** `grpc_context_provider` binds `grpc.ServicerContext` at `Scope.REQUEST` and nothing -else. The protobuf request `Message` is deliberately **not** a second connection provider, even -though every unary-request RPC has one in hand at the moment the child container is built. - -Binding it would put `protobuf` in the runtime dependency set of a package that otherwise needs only -`grpcio` and `modern-di` — the provider's `bound_type` has to be a protobuf message class, so the -adapter would have to import protobuf to declare it. That is a real cost paid by every user for a -value they already have: gRPC hands the request to the servicer method as its first positional -argument, so injecting it buys nothing a parameter does not already give. - -It is also not uniform. Client-streaming RPCs (`stream_unary`, `stream_stream`) have no single -request message — the behavior receives an iterator — so the provider would resolve for two of the -four RPC types and raise for the other two. `ServicerContext` is present for all four, which is what -makes it the right and only connection object here. - -`ServicerContext` alone is also why this integration calls `integrations.bind()` directly and never -`classify_connection`: with one connection provider there is nothing to dispatch across. - -**Revisit trigger:** `protobuf` becomes a runtime dependency of this package for some other reason, -or a use case appears that needs the request message resolved *transitively* — inside a provider -several edges deep, where threading it through as a parameter is no longer possible. diff --git a/docs/adr/0002-streaming-servicer-methods-must-be-generators.md b/docs/adr/0002-streaming-servicer-methods-must-be-generators.md new file mode 100644 index 0000000..bf02c0a --- /dev/null +++ b/docs/adr/0002-streaming-servicer-methods-must-be-generators.md @@ -0,0 +1,12 @@ +# Response-streaming servicer methods must be generators + +The response-streaming contract is the idiomatic (async) generator servicer form only: a method that +returns nothing and streams by calling `context.write(...)` is not supported, and `inject` keeps its +three wrapper shapes rather than growing a fourth to detect one. gRPC accepts both forms, and +dishka's grpcio integration handles the `context.write` coroutine as well, but the two need opposite +lifetimes. A generator behavior is consumed lazily, so `_wrap_stream_sync` and `_wrap_stream_aio` are +themselves generators that keep the per-RPC child and its `ContextVar` alive for as long as the +stream runs, while a `context.write` behavior is a plain coroutine that wants the response-unary +wrapper. `inspect` cannot tell the two apart, and guessing unary for a generator closes the child +before gRPC pulls a single item, tearing down every resolved dependency mid-stream. Supporting the +other form takes an explicit opt-in, never a runtime guess. diff --git a/docs/adr/0003-streaming-servicer-methods-must-be-generators.md b/docs/adr/0003-streaming-servicer-methods-must-be-generators.md deleted file mode 100644 index 184422b..0000000 --- a/docs/adr/0003-streaming-servicer-methods-must-be-generators.md +++ /dev/null @@ -1,25 +0,0 @@ -# Response-streaming servicer methods must be generators - -**Decision:** the response-streaming contract is the idiomatic **(async) generator** servicer form -only. A servicer method that returns nothing and streams by calling `context.write(...)` is not -supported, and `inject` does not grow a fourth wrapper shape to detect one. - -gRPC accepts both forms for a server-streaming or bidi RPC. Dishka's grpcio integration handles the -`context.write` coroutine form as well; modern-di keeps the narrower contract. The reason is that -the two forms need opposite lifetimes from the wrapper. A generator behavior is consumed lazily — -gRPC pulls items only when the wrapper itself is iterated — so `_wrap_stream_sync` / -`_wrap_stream_aio` must themselves be generators, keeping the per-RPC child and the `ContextVar` -alive for exactly as long as the stream runs. A `context.write` behavior is a plain coroutine that -returns once, and would need the response-*unary* wrapper instead. Supporting both means deciding at -wrap time which one a behavior is, and `inspect` cannot tell a `context.write` streamer from any -other coroutine — the only signal is a `unary_stream` / `stream_stream` handler slot holding a -non-generator function, which is also what a genuinely broken servicer looks like. - -The cost of guessing wrong is silent and severe in one direction: pick the unary wrapper for a -generator behavior and the child is closed before gRPC has pulled a single item, so every resolved -dependency is torn down mid-stream. The generator-only contract is the conservative choice, and it -is the form the original spike proved end-to-end across all four RPC types. - -**Revisit trigger:** a user reports a real `context.write` servicer, or gRPC's own guidance shifts to -recommend that form — at which point the dispatch needs an explicit opt-in (a decorator argument or -a distinct decorator), never a runtime guess. diff --git a/docs/agents/domain.md b/docs/agents/domain.md index 24d84c0..a64e9c8 100644 --- a/docs/agents/domain.md +++ b/docs/agents/domain.md @@ -17,8 +17,8 @@ Single-context repo: / ├── CONTEXT.md ├── docs/adr/ -│ ├── 0001-aio-coverage-flake-fixed-in-the-test.md -│ └── 0002-protobuf-request-message-not-a-provider.md +│ ├── 0001-protobuf-request-message-not-a-provider.md +│ └── 0002-streaming-servicer-methods-must-be-generators.md └── modern_di_grpc/ ``` @@ -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 (aio coverage flake fixed in the test), but worth reopening because…_ +> _Contradicts ADR-0001 (the protobuf request message is not exposed as a provider), but worth +> reopening because…_