Skip to content

fix: missing exports, context skip_methods, and compression "none" - #17

Merged
AlexeyShalaev merged 4 commits into
masterfrom
fix/agents-page-findings
Sep 6, 2026
Merged

fix: missing exports, context skip_methods, and compression "none"#17
AlexeyShalaev merged 4 commits into
masterfrom
fix/agents-page-findings

Conversation

@AlexeyShalaev

Copy link
Copy Markdown
Contributor

Five defects that came out of writing docs/agents.md against the source. All five
reproduce; each fix is additive, nothing is renamed, removed or re-defaulted.

1. AsyncContextInterceptor could not be given skip_methods

AsyncServerInterceptor has taken skip_methods since it was written, and
docs/guide/interceptors.md documents it as "a constructor kwarg" without
qualification, but AsyncContextInterceptor.__init__ neither accepted one nor
forwarded one — it called super().__init__() bare.

>>> AsyncContextInterceptor([], skip_methods=SKIPPED_HEALTH_METHODS)
TypeError: AsyncContextInterceptor.__init__() got an unexpected keyword argument 'skip_methods'

So every health probe went through header extraction, and with a required=True
HeaderConfig a kubelet probe — which carries none of the service's headers — was
aborted with INVALID_ARGUMENT.

Fixed: the kwarg is keyword-only and defaults to (), so behaviour is unchanged for
everyone. I deliberately did not default it to SKIPPED_HEALTH_METHODS like the
four observability interceptors do: this one binds values the handler itself may read,
so skipping is an opt-in, not a new default. Two tests in
tests/unit/aio/interceptors/test_context.py (both TypeError before the change).

One correction to the finding as written: this is not "the one interceptor with no way
to opt a method out". AsyncExceptionHandlerInterceptor has no skip_methods either.
I left that one alone — an interceptor whose whole job is mapping exceptions to statuses
has no business skipping RPCs — and made the agents-page table say so instead of leaving
the gap unexplained.

2. BaseGrpcServerSettings rejected compression_algorithm="none"

COMPRESSION_ALGORITHMS maps "none" to gRPC's GRPC_COMPRESS_NONE, and
build_grpc_options has always translated it (tests/unit/test_options.py asserts the
"none" -> 0 mapping), so GrpcServerConfig(compression_algorithm="none") builds a
server fine. The pydantic model typed the same field Literal["deflate", "gzip"] | None:

>>> BaseGrpcServerSettings(compression_algorithm="none")
ValidationError: Input should be 'deflate' or 'gzip' [type=literal_error, input_value='none']

The two settings shapes are documented as interchangeable everywhere the kit accepts
settings, so this was a real hole. Fixed by widening GrpcCompressionAlgorithm to
Literal["none", "deflate", "gzip"] rather than dropping "none" from the option
builder: "none" is an explicit request for gRPC's no-compression algorithm, which is
not the same statement as leaving the field None. Parametrized test in
tests/unit/test_settings.py (fails on [none] before the change).

3. GrpcServiceName was not re-exported at the package root

grpc_server_kit.protocols exports it, the kit's own dishka providers register the
service name under it, and the root re-exported the five protocols beside it and not
this one — so anyone writing their own provider had to import from the submodule:

>>> from grpc_server_kit import GrpcServiceName
ImportError: cannot import name 'GrpcServiceName' from 'grpc_server_kit'

Fixed: exported from grpc_server_kit and from grpc_server_kit.aio (the agents page
describes aio as "everything above plus …", which only stays true if both get it).

4. grpc_server_kit.aio exported reset_signal_handlers but not the async form

aio.reset_signal_handlers_async and aio.SignalManager were both missing, though
ServerLifecycleManager uses reset_async internally, the docstrings recommend the
async form, and ServerLifecycleManager(signal_manager=…) /
run_async_grpc_server(signal_manager=…) both take a SignalManager.

aio.reset_signal_handlers:       exported=True  attr=True
aio.reset_signal_handlers_async: exported=False attr=False
aio.SignalManager:               exported=False attr=False

Fixed: both re-exported. implicit_reexport is off for mypy, so a name missing from
__all__ was not importable for type checkers either — the tests in
tests/unit/test_init.py assert the attribute and __all__ membership.

5. The quickstart was wrong about health and reflection

When health is enabled, the health service name is advertised via reflection
automatically.

Half the story. GrpcApp.build() appends grpc.health.v1.Health only when
enable_reflection([...]) already gave it a list; reflection turned on solely through
settings.enable_reflection = True fails the build:

>>> app = GrpcApp(GrpcServerConfig(enable_reflection=True)); app.enable_health(); app.build()
ValueError: Reflection is enabled but no service names are configured; call
app.enable_reflection([...]) with your fully-qualified service names

Here the doc is what drifted, not the code: reflection with no service names is a
misconfiguration worth failing the build on, and docs/agents.md already states the
rule correctly. Corrected the quickstart line. Documentation only.

Agents page

docs/agents.md changed with the API, per CONTRIBUTING.md:

  • the interceptor table no longer says AsyncContextInterceptor is "the only one with
    no skip_methods argument" — it has one, and row 5 now records that the exception
    handler is the one without;
  • the "note the asymmetry" sentence about COMPRESSION_ALGORITHMS vs
    BaseGrpcServerSettings is gone — there is no asymmetry left;
  • GrpcServiceName moved from "not re-exported at the package root" into the root API
    table, and the aio paragraph and lifecycle table list the signal names it now
    re-exports.

docs/guide/interceptors.md gains a short paragraph on the context interceptor's empty
skip_methods default and why you would pass SKIPPED_HEALTH_METHODS to it.

Verification

$ make check
uv run ruff check .            All checks passed!
uv run ruff format --check .   110 files already formatted
uv run mypy grpc_server_kit    Success: no issues found in 52 source files

$ make test-unit
411 passed, 23 deselected in 1.69s

$ make test-integration
23 passed, 411 deselected in 0.76s

$ make test
434 passed in 2.49s — Required test coverage of 90% reached. Total coverage: 97.60%

uv sync --frozen --all-extras --group dev; uv.lock unchanged.

No changelog entry: CONTRIBUTING.md leaves CHANGELOG.md to Release Please.

Nothing here is breaking

Three added names, one added keyword-only parameter with a behaviour-preserving default,
one widened Literal, and two doc corrections.

 Alex Shalaev added 4 commits September 6, 2026 21:11
The base class has taken `skip_methods` since it was written, and the
guide documents it as "a constructor kwarg" without qualification, but
`AsyncContextInterceptor.__init__` neither accepted it nor forwarded one
-- `AsyncContextInterceptor([...], skip_methods=SKIPPED_HEALTH_METHODS)`
raised `TypeError`. Health traffic therefore always ran through header
extraction, and with a `required=True` HeaderConfig a kubelet probe
(which carries none of the service's headers) was aborted with
INVALID_ARGUMENT.

The kwarg is added keyword-only and defaults to `()`, so existing
behaviour is unchanged: unlike the observability interceptors, this one
binds values the handler itself may read, so skipping health methods is
an opt-in rather than a new default.
`COMPRESSION_ALGORITHMS` maps "none" to gRPC's GRPC_COMPRESS_NONE, and
`build_grpc_options` has always translated it (there is a test for the
mapping), so `GrpcServerConfig(compression_algorithm="none")` builds a
server fine. `BaseGrpcServerSettings` typed the same field as
`Literal["deflate", "gzip"] | None` and rejected it with a
ValidationError -- the two settings shapes are meant to be
interchangeable everywhere the kit accepts settings.

Widening the literal rather than dropping "none" from the option
builder: "none" is an explicit request for gRPC's no-compression
algorithm, which is not the same statement as leaving the field unset.
Two names a caller has to reach into a submodule for while everything
next to them is available from the package root:

- `GrpcServiceName` is the DI key the kit's own dishka providers register
  the service name under, so anyone writing their own provider needs it,
  yet the root re-exported the five protocols beside it and not this one.
  It is now exported from `grpc_server_kit` and `grpc_server_kit.aio`.
- `grpc_server_kit.aio` exported `reset_signal_handlers` but not
  `reset_signal_handlers_async`, the form `ServerLifecycleManager` uses
  internally and the docstrings recommend, nor `SignalManager` -- which
  `ServerLifecycleManager(signal_manager=...)` and
  `run_async_grpc_server(signal_manager=...)` both accept.

Additions only; nothing is renamed or removed. `implicit_reexport` is
off for mypy, so a name missing from `__all__` was not importable for
type checkers either -- the tests assert both the attribute and `__all__`.
"When health is enabled, the health service name is advertised via
reflection automatically" is only half the story: `GrpcApp.build()`
appends `grpc.health.v1.Health` to the reflection list when
`enable_reflection([...])` already gave it one, and if reflection was
turned on solely through `settings.enable_reflection = True` the build
raises ValueError instead. The code is right -- reflection with no
service names is a misconfiguration worth failing on, and the agents
page already states the rule -- so the quickstart line is the one that
drifted.
@AlexeyShalaev
AlexeyShalaev merged commit 9c70e9d into master Sep 6, 2026
5 checks passed
@AlexeyShalaev
AlexeyShalaev deleted the fix/agents-page-findings branch September 6, 2026 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant