Skip to content

gRPC #2525 follow-up: Correctness Gaps#2542

Merged
jeremydmiller merged 6 commits intoJasperFx:mainfrom
erikshafer:feature/grpc-middleware-scoping
Apr 20, 2026
Merged

gRPC #2525 follow-up: Correctness Gaps#2542
jeremydmiller merged 6 commits intoJasperFx:mainfrom
erikshafer:feature/grpc-middleware-scoping

Conversation

@erikshafer
Copy link
Copy Markdown
Contributor

@erikshafer erikshafer commented Apr 20, 2026

fix(grpc): #2525 correctness gaps — PR-A

Closes four correctness gaps from #2525 plus Phase 0 of the MiddlewareScoping.Grpc fix.
Target release: Wolverine 5.31.x. or Wolverine 5.32.x

Test state: 115/115 Wolverine.Grpc.Tests green · 7/7 streaming_handler_support green (incl. new mid-stream throw → OTel Error assertion) · 19/19 cascade-related CoreTests green · clean build, 0 errors (pre-existing VSTHRD warnings only).

Scope: 1125 insertions / 66 deletions across 24 files. Framework-side delta is ~165 lines across 6 source files; the rest is tests, fixtures, and two updated docs pages.

Contents

Changes at a glance

# Fix Priority New tests
§1.2 Streaming OTel + success-log post-iteration P0 1 (+ 6 existing, all green)
§1.3 AddWolverineGrpc() idempotent P0 8
§1.4 Generated gRPC type-name uniqueness P1 3
§1.5 Typed IAsyncEnumerable<T> reflection cache P1 0 new (existing 19 cover it)
M15 Phase 0 Scope-filtered middleware discovery + WolverineGrpcOptions.AddMiddleware<T> 20

§1.2 Streaming OTel + success-log fire post-iteration (P0)

Bug. TracingExecutor.StreamCoreAsync logged _messageSucceeded and closed the activity the moment the handler returned the IAsyncEnumerable<T>, before the consumer iterated. A cancellation or mid-stream throw closed the span with Ok status — real failures were invisible on dashboards.

Fix. Moved success/error signals to fire after iteration. C# CS1626 forbids yield return inside a try/catch, so switched to a manual MoveNextAsync loop where the try/catch wraps only the enumerator advance and yield return current; sits outside. Applied the same structural fix to Executor.StreamCoreAsync.

Test. New mid_stream_throw_marks_activity_status_error asserts activity status goes Error when iteration throws. Existing cancellation and faulting-stream tests continue to pass.

Files. Runtime/Handlers/TracingExecutor.cs, Runtime/Handlers/Executor.cs, Testing/CoreTests/Acceptance/streaming_handler_support.cs.

§1.3 AddWolverineGrpc() idempotent (P0)

Bug. Double-calling AddWolverineGrpc() stacked WolverineGrpcExceptionInterceptor twice and double-registered GrpcGraph. Library extensions calling it defensively paid double log + translation overhead per unhandled exception.

Fix. Marker-singleton guard (WolverineGrpcMarker) matching the pattern UseGrpcRichErrorDetails / UseFluentValidation already use. Plus a new overload:

services.AddWolverineGrpc(g => g.AddMiddleware<MyGrpcMiddleware>());

Test. add_wolverine_grpc_idempotency_tests.cs — 8 tests pinning single-registration invariants across interceptor, graph, options, and the new overload.

Files. Wolverine.Grpc/WolverineGrpcExtensions.cs, Wolverine.Grpc/WolverineGrpcExceptionInterceptor.cs (2-line XML-doc cref disambiguation — the bare cref became ambiguous with the new overload).

§1.4 Generated gRPC type-name uniqueness (P1)

Bug. GrpcServiceChain.TypeName = "{ProtoServiceName}GrpcHandler". Two assemblies shipping a Greeter proto — tests + production, or two bounded contexts — emit GreeterGrpcHandler into the same WolverineHandlers child namespace. AttachTypesSynchronously picks whichever GetExportedTypes() returns first; that order is unspecified across assemblies.

Fix. Mirrors HandlerGraph.cs:370-383's post-hoc qualifier-based disambiguation (the issue #2004 pattern) but uses a stable hash of StubType.FullName as the qualifier — gRPC stub simple names aren't reliably unique. Runs once at the tail of DiscoverServices, never on a hot path; zero additional reflection.

Tests. type_name_disambiguation_tests.cs — 3 tests: collisions get hashed suffixes, disambiguation is idempotent + process-stable, single-chain input preserves default name. Collision stubs are internal abstract without [WolverineGrpcService], so GetExportedTypes() and IsProtoFirstStub both skip them — they don't leak into adjacent test suites.

Files. Wolverine.Grpc/GrpcGraph.cs, Wolverine.Grpc/GrpcServiceChain.cs (TypeName now get; private set; plus internal ApplyDisambiguatedTypeName seam used only by the pass).

§1.5 Typed IAsyncEnumerable<T> cascading reflection cache (P1)

Bug. In MessageContext.EnqueueCascadingAsync, every cascading non-enumerable message did message.GetType().GetInterfaces() (allocates) plus MakeGenericMethod on hit. Both per-call, on a hot path.

Fix. Single ImHashMap<Type, MethodInfo?> keyed on message type:

private static ImHashMap<Type, MethodInfo?> _typedEnumerableCascadeMethods =
    ImHashMap<Type, MethodInfo?>.Empty;

Chose ImHashMap for codebase idiom (matches WolverineMessageNaming.cs:128 + five other per-type cache sites — lock-free, copy-on-write, write-rare). Keyed on message type rather than element type because message-type keying eliminates both reflection ops on hit and negative-caches plain cascading messages (the common fall-through path). The only remaining reflection is MethodInfo.Invoke — Expression-compiled delegates could eliminate it, but not worth the diff today.

Tests. Existing typed_async_enumerable_cascades_items_via_regular_invoke plus 19 cascade-related CoreTests exercise the path; all green.

Files. Runtime/MessageContext.cs.

M15 Phase 0 — MiddlewareScoping.Grpc groundwork

Bug. MiddlewareScoping.Grpc shipped in #2525 as a public enum member with attribute-level tests but no execution path. A handler annotated [WolverineBefore(MiddlewareScoping.Grpc)] compiled, tests passed, and the method never ran on any RPC call.

Phase 0 lands the architecture-independent groundwork so the service-wide-vs-per-RPC design question (see Review ask #1) can be resolved without blocking the release window. This PR lands what both options need; Phase 1 is a focused follow-up.

What Phase 0 ships:

  1. Scope-filtered discovery on GrpcServiceChain — new lazy DiscoveredBefores / DiscoveredAfters reusing MiddlewarePolicy.FilterMethods<T> so scope filtering matches the canonical Chain.ApplyImpliedMiddlewareFromHandlers path. Ordinal-sorted for byte-stable codegen; reference-equal caching.
  2. WolverineGrpcOptions.AddMiddleware<T> mirroring WolverineHttpOptions.AddMiddleware. Internal MiddlewarePolicy, typed AddMiddleware<T> with optional per-chain filter. Singleton options registered via the §1.3 idempotency guard.
  3. Invariant pins that Phase 1 depends on:
    • policy_leak_tests.csIPolicies.AddMiddleware<T> must not attach to GrpcServiceChain (the chain is HandlerChain filter at WolverineOptions.Policies.cs:208-216 is load-bearing).
    • frame_cloning_spike_tests.cs — two MethodCalls from the same MethodInfo are distinct instances with per-instance Arguments[] + Creates. Phase 1's per-RPC argument resolution depends on this non-sharing invariant.
  4. Test scaffolding for Phase 1 — proto with distinct request messages for unary/streaming, fixture, capture sink, probe methods covering Before/After × Anywhere/Grpc/MessageHandlers.

No production behavior changes today. MapWolverineGrpcServices is unchanged; nothing consumes DiscoveredBefores at emission time yet. End-user gRPC middleware behavior is byte-identical to main.

Tests. 20/20 green: 8 idempotency + 7 scope_discovery + 2 frame_cloning + 2 smoke + 1 policy_leak.

Files.

  • Wolverine.Grpc/WolverineGrpcOptions.cs (new)
  • Wolverine.Grpc/WolverineGrpcExtensions.cs (+overload)
  • Wolverine.Grpc/GrpcServiceChain.cs (+discovery properties)
  • Wolverine.Grpc.Tests/Wolverine.Grpc.Tests.csproj (proto + folder globs)
  • Wolverine.Grpc.Tests/GrpcMiddlewareScoping/* (12 new fixture/test files; folder name GrpcMiddlewareScoping/ — not MiddlewareScoping/ — to avoid shadowing the enum)

Docs updated in-PR

  • docs/guide/grpc/index.md — drops the "experimental / feature branch" framing from the info block now that gRPC Support for Wolverine HTTP Endpoints + IMessageBus.StreamAsync<T> #2525 is merged. Adds a Current Limitations bullet honestly disclosing the Phase 0/Phase 1 state of MiddlewareScoping.Grpc: the attribute compiles and is scope-filtered correctly but doesn't weave yet; recommend custom gRPC interceptor until M15 Phase 1 lands. Expands the exception-mapping limitation to note that client-side WolverineGrpcClientOptions.MapRpcException already supports per-client override.
  • docs/guide/grpc/handlers.md — replaces stale dotnet run -- describe-routing example with wolverine-diagnostics codegen-preview --grpc Greeter and forward-links the CLI page.
  • docs/guide/grpc/streaming.md — extends the "Exception timing" limitation bullet to note that the server-side OpenTelemetry activity now stays open until the stream fully drains or faults, and is marked Error for both mid-stream throws and cancellation.

Review asks

  1. M15 Phase 1 — Option A vs Option B. Wire middleware via service-wide override (Option A: one override per RPC on the generated GrpcHandler) or per-RPC GrpcRpcChain with its own HandlerChain-like lifecycle (Option B)? Working default if no response: Option A (smaller blast radius; gRPC isn't on NuGet yet so a wrong-A → B refactor is cheap, but a wrong-B permanently reshapes chain topology). Full analysis: .plans/grpc-streaming/M15-middleware-scoping-implementation.md §6.
  2. §1.5 cache choice — picked ImHashMap<Type, MethodInfo?> for codebase idiom (matches 5 existing sites). Flag if you'd rather ConcurrentDictionary for this one.
  3. §1.5 key choice — picked message type (eliminates both reflection ops on hit + negative-caches plain messages) over element type. Flag if you'd prefer element-type.
  4. §1.4 qualifierGetDeterministicHashCode() on StubType.FullName (mirrors HandlerChain). Flag if you'd prefer a different qualifier (e.g., assembly name).

Not in this PR (by design)

  • M15 Phase 1 — codegen weaving, awaiting Option A vs B direction.
  • [WolverineOnException] / [WolverineFinally] discovery on GrpcServiceChain — similar Phase 0 treatment; separate PR.
  • Feature docs for MiddlewareScoping.Grpc — walkthrough/examples deferred to Phase 1 so they describe real execution. Current Limitations disclosure ships now.
  • Tier 2 ambiguity calls (streaming zero-item tolerance, client MapToException default posture, IGrpcStatusDetailsProvider ordering, Google.Api.CommonProtos footprint) — each wants its own decision thread and ships as small follow-ups.
  • Code-first codegen parity (M9) and bidi/client-streaming first-class surface — each is a multi-day focused sprint and ships as its own PR per .plans/next-pr-roadmap.md.

erikshafer and others added 5 commits April 20, 2026 07:13
…otent registration

Implement comprehensive middleware scoping tests for proto-first gRPC services, verifying that `[WolverineBefore(MiddlewareScoping.Grpc)]` attaches while `[WolverineBefore(MiddlewareScoping.MessageHandlers)]` is excluded. Add policy leak guards preventing handler-only middleware from crossing into gRPC chains. Make `AddWolverineGrpc()` idempotent via marker pattern to prevent duplicate interceptor registration. Introduce `WolverineGrpcOptions` for gRPC-specific middleware policy analogous to `WolverineHttpOptions`.

- Add MiddlewareScopingFixture test harness with GreeterMiddlewareTestStub
- Add scope_discovery_tests verifying DiscoveredBefores/DiscoveredAfters filtering
- Add policy_leak_tests pinning HandlerChain-only filter boundary
- Add middleware_scoping_fixture_smoke_tests for unary and server-streaming round trips
- Add add_wolverine_grpc_idempotency_tests preventing duplicate registrations
- Add WolverineGrpcOptions with AddMiddleware<T>(filter) for gRPC chain targeting
- Add GrpcServiceChain.DiscoveredBefores/DiscoveredAfters for Phase-1 codegen
- Add WolverineGrpcMarker singleton guard for repeat AddWolverineGrpc() calls
- Add middleware_scoping_test.proto for test-only GreeterMiddlewareTest service
…ethodCall invariant tests

Sort `DiscoveredBefores` and `DiscoveredAfters` ordinally by method name so Phase-1 generated gRPC service source is byte-stable across runs (reflection order is unspecified). Add `frame_cloning_spike_tests` pinning the invariant that two `MethodCall` instances built from one `MethodInfo` do not share mutable `Arguments[]` or `Creates` collections — if caching ever broke this, one RPC's argument resolution would contaminate siblings during codegen.

- Add OrderBy(m => m.Name, StringComparer.Ordinal) to DiscoveredBefores/DiscoveredAfters
- Add frame_cloning_spike_tests verifying Arguments[] and Creates are per-instance
- Add discovered_befores/afters_are_ordinally_sorted_for_deterministic_codegen tests
- Update XML doc comments explaining ordinal-sort and byte-stability guarantees
…s examples, and document client-side error mapping

Remove experimental/branch references from feature status. Replace `describe-routing` with `codegen-preview --grpc` in diagnostics examples and expand explanation of handler discovery debugging. Document client-side `MapRpcException` override in limitations section. Clarify middleware scoping implementation status and provide workaround guidance until M15 codegen lands.

- Remove feature/grpc-and-streaming-support branch reference from index.md
- Update diagnostics commands in handlers.md with codegen-preview examples
- Add cross-reference to codegen-preview CLI documentation
- Expand Current Limitations section with client-side error mapping note
- Clarify middleware scoping status and interim interceptor workaround
- Update roadmap to reflect deferred M15 and code-first codegen work
…ng fixes, and typed-enumerable cascade optimization

Implement stable hash-based disambiguation for proto services sharing simple names (e.g., `Greeter` in two bounded contexts) so `AttachTypesSynchronously` deterministically resolves unique handler types. Fix streaming handler execution-finished tracking and activity status to fire only after enumeration completes or throws mid-stream. Cache `IAsyncEnumerable<T>` cascade MethodInfo per message type to eliminate repeated reflection.

- Add GrpcGraph.DisambiguateCollidingTypeNames applying stable hash suffixes to colliding TypeName values
- Add GrpcServiceChain.ApplyDisambiguatedTypeName for post-discovery rewrite
- Add type_name_disambiguation_tests verifying stability, idempotence, and collision-free preservation
- Move ExecutionFinished and activity status calls after MoveNextAsync loop in Executor/TracingExecutor
- Add mid_stream_throw_marks_activity_status_error test pinning error propagation
- Add MessageContext._typedEnumerableCascadeMethods ImHashMap cache for IAsyncEnumerable<T> detection
- Replace GetInterfaces + MakeGenericMethod cascade hot path with cached MethodInfo lookup
@erikshafer erikshafer changed the title gRPC #2525 follow-up: Correctness & Polish gRPC #2525 follow-up: Correctness Gaps Apr 20, 2026
Clarify that the OpenTelemetry activity for server-streaming handlers remains open until the `IAsyncEnumerable<T>` fully drains or faults, ensuring dashboards reflect the real terminal state (including mid-stream exceptions and cancellation) rather than the moment the handler returned the enumerable.

- Expand exception timing documentation in streaming.md with activity status behavior
- Note that activity is marked `Error` for both pre-yield and mid-stream exceptions
- Explain deferred activity completion aligns telemetry with actual stream lifecycle
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.

2 participants