diff --git a/Directory.Packages.props b/Directory.Packages.props index 84eb10f..4473af1 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -19,17 +19,41 @@ - - - - - - - - - - - + + + + + + + + + + + + + + + + + + + + + diff --git a/apphost.cs b/apphost.cs index c93eccc..4d59f51 100644 --- a/apphost.cs +++ b/apphost.cs @@ -10,6 +10,7 @@ #:package Aspire.Hosting.PostgreSQL@13.4.6 #:project ./src/CritterCab.Dispatch/CritterCab.Dispatch.csproj +#:project ./src/CritterCab.Telemetry/CritterCab.Telemetry.csproj var builder = DistributedApplication.CreateBuilder(args); @@ -36,11 +37,17 @@ .WithReference(dispatchDb) .WaitFor(dispatchDb); -// Telemetry is CritterCab's second service (stream-processing shape, W006). This PR -// stands up its skeleton + config-as-events slice 1 only; the Kafka resource that -// slice 3 needs is deliberately NOT wired yet (same deferral the Dispatch skeleton -// made for transport). Ports follow the +5 slot convention after Dispatch's 5310; -// 5315 https / 5316 http. See docs/skills/aspire/SKILL.md § Port allocation. +// Telemetry is CritterCab's second service (stream-processing shape, W006). Ports follow +// the +5 slot convention after Dispatch's 5310; 5315 https / 5316 http. See +// docs/skills/aspire/SKILL.md § Port allocation. +// +// The ReportLocations gRPC ingest (W006 §6.2) rides the HTTPS endpoint via Kestrel HTTP/2 — +// the same arrangement the Dispatch block describes above, so gRPC needs no endpoint of its +// own. This is CritterCab's first gRPC surface that actually serves traffic. +// +// The Kafka resource that slice 3 needs is still deliberately NOT wired: slice 2's publish +// goes to the IDriverLocationPublisher seam, and PR C swaps that for the real producer and +// adds the broker here. builder.AddProject("telemetry", launchProfileName: null) .WithHttpsEndpoint(port: 5315, name: "https") .WithHttpEndpoint(port: 5316, name: "http") diff --git a/docs/prompts/README.md b/docs/prompts/README.md index 084226b..92c00ff 100644 --- a/docs/prompts/README.md +++ b/docs/prompts/README.md @@ -168,3 +168,4 @@ Subsequent sections are prompt-specific. Existing prompts in this directory serv - [`implementations/004-dispatch-slice-5-2-fare-quoted-failure-paths.md`](./implementations/004-dispatch-slice-5-2-fare-quoted-failure-paths.md) — Third vertical slice: closes W001 §5.2's three FareQuoteFailed alternate-path GWTs (transient retry recovery, exhausted retries, non-transient failure) deferred from slice 003. Adds `FareQuoteFailed` event, `FareQuoteAttempts` terminal-events-only projection, `IFareQuoteOutcome` marker interface, two `IPricingClient` exception types for failure injection, `FareQuoteRetryPolicy` DI record (test-override seam pre-shaping Slice 11's `DispatchPolicyConfigured`), and a manual retry loop in `FareQuoteAutomation`. Three implementation-mechanism choices committed up-front in the prompt: exception-typed failure contract, manual retry loop (not Wolverine chain policy), terminal-only projection. Flags one W001 §5.2 inconsistency to amend in a future workshop-tidy session. **Second substantive forward exercise of the spec-delta closure-loop convention** — meets ADR-016's 2–3-exercises deferral trigger. Status: complete (2026-05-19). Produced retro at [`retrospectives/implementations/004-dispatch-slice-5-2-fare-quoted-failure-paths.md`](../retrospectives/implementations/004-dispatch-slice-5-2-fare-quoted-failure-paths.md). - [`implementations/005-dispatch-slice-5-3-candidates-selected.md`](./implementations/005-dispatch-slice-5-3-candidates-selected.md) — Fourth vertical slice: first slice of the dispatch-round arc. `CandidateSelectionAutomation` reacts to `FareQuoted`, queries `INearbyAvailableDriversSource` (Telemetry + Driver Profile translation-in stub; parking-lot #4 deferred), and emits `CandidatesSelected` (happy path) or `NoCandidatesAvailable` (empty-set path) as a Klefter decision-event. Adds `DispatchPolicySnapshot` DI record (hardcoded `searchRadiusMeters: 5000`, `maxCandidatesPerRound: 5`; Slice 11 swaps for `DispatchPolicyConfigured`-fed projection), `ICandidateSelectionOutcome` marker interface (third instance of the pattern), `RequestRoundsProjection` inline projection (consumed by Slice 9), and `RequestTimeline` extensions for both outcome events. Three Alba integration tests, one per W001 §5.3 GWT. First use of `[WriteAggregate]` bound to a non-first stream event in the codebase. **Third substantive forward exercise of the spec-delta closure-loop convention.** Status: pending (authored 2026-06-16). - [`implementations/006-telemetry-skeleton-and-slice-1-config.md`](./implementations/006-telemetry-skeleton-and-slice-1-config.md) — **Second service in the repo** and the opening PR of the W006 Telemetry transport chain. Bootstraps the `CritterCab.Telemetry` service skeleton **and** W006 slice 1 (`TelemetryPolicyConfigured` config-as-events) in one PR, per the named skeleton-plus-first-slice cadence exception (mirrors the Dispatch skeleton + slice-5.1 precedent). Config-as-events is the dependency-correct first slice: slice 2's gRPC ingest reads this slice's `TelemetryPolicy` view (`throttlePolicyVersion`, `h3Resolution`, intervals). Establishes two firsts in code — **first config-as-events instance** (ADR-011's third instance, first realized; Dispatch/Onboarding were design-only) and **first FluentValidation boundary validation**. Four pre-flight `jasperfx-source-verifier` gates (`IInitialData` seed vehicle, `long` stream-version property, self-aggregating `LiveStreamAggregation` registration + Marten-9 `partial` scope, Wolverine.HTTP FluentValidation middleware). None of W006 §11's three ADR candidates fired (all later-arc), but the Phase-2 audit surfaced an ADR-011 Option-A/B-for-Marten gap that — **per user direction, expanding the session mid-flight** — was resolved in-PR via an **ADR-011 amendment** (`IInitialData` as the canonical Marten Option-A realization; LWW for config singletons). No narrative anchor (PR #40: the narrative layer does not apply to Telemetry). Kafka deliberately **not** wired into `apphost.cs` this PR (transport lands with the slice that needs it — Dispatch-skeleton precedent). Substantive spec delta: W006 §6.1 designed → realized. Status: pending (authored 2026-07-10). +- [`implementations/007-telemetry-slices-4-and-2-transport.md`](./implementations/007-telemetry-slices-4-and-2-transport.md) — **CritterCab's first transport in code.** W006 slice 4 (`LastKnownPosition` store + heartbeat-absence eviction) and slice 2 (gRPC `ReportLocations` client-streaming ingest) in one PR under the **coupled-slices** reading of the cadence rule, not the skeleton-plus-first-slice exception (already spent on 006): slice 2's publish trigger evaluates against slice 4's document and slice 4's document is written only when slice 2 publishes, so building either alone means evaluating against a document that does not exist or writing one nothing reads. Establishes five firsts in code — **first gRPC surface serving traffic**, **first client-streaming RPC**, **first proto codegen** (`protos/` had been contracts with no consumer since PR #39), **first non-event-sourced document write path**, and **first recurring/scheduled work**. Retires the three-month client-streaming forward-constraint: WolverineFx.Grpc **6.21.0** auto-generates the shape, so nothing is hand-wired, and the two gRPC skills that still described the workaround are corrected in-session under the session-runner-blocking exception (a session cannot follow a skill telling it to hand-wire). Eleven Verify-before-wiring gates were source-verified before any code; gate 5 **corrected the prompt's own hypothesis** (Wolverine has no recurring-message primitive — the idiom is a plain `BackgroundService`) and gate 11 dissolved (Alba's `TestServer` feeds a `GrpcChannel` directly, so the Alba-first default holds for gRPC). Two W006 under-specifications surfaced and were resolved by user sign-off rather than silently: the `LastKnownPosition` field set (`lastPublishedAt` collapsed into `serverReceivedAt`, since upsert-on-publish-only makes them one instant) and the `accuracyMeters` threshold (**100m, invented at implementation time — W006 names the threshold but fixes no value**). Also fixed `apphost.cs`, which had not compiled since PR #42 and which CI does not build. Substantive spec delta: W006 §6.2 + §6.4 designed → realized; the §11 *windowed client-streaming* ADR candidate lands as a skill, not an ADR. Status: complete (authored 2026-07-20, executed 2026-07-24). Produced retro at [`retrospectives/implementations/007-telemetry-slices-4-and-2-transport.md`](../retrospectives/implementations/007-telemetry-slices-4-and-2-transport.md). diff --git a/docs/prompts/implementations/007-telemetry-slices-4-and-2-transport.md b/docs/prompts/implementations/007-telemetry-slices-4-and-2-transport.md new file mode 100644 index 0000000..52def55 --- /dev/null +++ b/docs/prompts/implementations/007-telemetry-slices-4-and-2-transport.md @@ -0,0 +1,152 @@ +# Prompt 007 — Telemetry Slice 4 (`LastKnownPosition` store + eviction) + Slice 2 (gRPC `ReportLocations` ingest) + +| Field | Value | +|---|---| +| **Status** | **Complete (2026-07-24)** — executed as PR [#45](https://github.com/erikshafer/CritterCab/pull/45); retro at [`retrospectives/implementations/007-telemetry-slices-4-and-2-transport.md`](../../retrospectives/implementations/007-telemetry-slices-4-and-2-transport.md). Originally: Ready — the three durable forks resolved by the user 2026-07-20 (Kafka deferred to PR C behind a stub seam; `driverId` via an `IDriverPrincipalAccessor` seam; H3 via `H3.net` / pocketken). Rides in the PR B session's PR alongside the implementation; not committed standalone. | +| **Authored** | 2026-07-20 | +| **Target artifacts** | `Directory.Packages.props` (all 11 `WolverineFx*` entries 6.19.0→6.21.0 in lockstep, Grpc.Tools/Google.Protobuf/Grpc.AspNetCore, `pocketken.H3`; **no** WolverineFx.Kafka *reference* this PR — deferred to PR C), `src/CritterCab.Telemetry/CritterCab.Telemetry.csproj` (proto `` codegen items), `src/CritterCab.Telemetry/` (slice-2 `ReportLocations` feature + gRPC stub; slice-4 `LastKnownPosition` feature + eviction sweep; `Program.cs` gRPC + recurring-scheduler wiring), `tests/CritterCab.Telemetry.Tests/` (gRPC client-streaming + eviction-sweep tests), `apphost.cs` (gRPC port; **no Kafka** unless the fork says otherwise), `protos/crittercab/telemetry/v1/report_locations.proto` (stale forward-constraint comment correction), `docs/skills/wolverine-grpc-handlers/SKILL.md` (correct ~6 stale lines + re-home the auto-generated client-streaming pattern — session-runner-blocking), `docs/skills/wolverine-grpc-bidirectional-handlers/SKILL.md` (superseded-banner + frontmatter/table one-liners only; 150-line rewrite is a DEBT row), `docs/workshops/006-telemetry-event-model.md` (Document History), `docs/skills/DEBT.md`, `docs/prompts/README.md` (index entry), this prompt's retro. | +| **Source-of-truth dependencies** | [W006 §6.2 (ingest), §6.4 (store + eviction), §3.3 (`LastKnownPosition` doc), §4 (UL), §11 (ADR candidates)](../../workshops/006-telemetry-event-model.md); [ADR-005](../../decisions/005-transport-selection-by-flow-type.md) (gRPC for service-to-service/streaming); [ADR-009](../../decisions/009-protobuf-contracts-as-first-class-artifacts.md) (proto-first, contract = `.proto`); [ADR-011 + Amendment](../../decisions/011-configuration-as-events-bootstrap.md) + [ADR-018](../../decisions/018-candidate-projection-ownership-and-telemetry-geospatial-supply.md) (context). Skills: `wolverine-grpc-handlers`, `wolverine-grpc-bidirectional-handlers`, `cli-grpc-tooling`, `vertical-slice-organization`, `csharp-coding-standards`, `wolverine-marten-automation` (recurring/scheduled handler shape), `marten-querying` (document upsert/delete), `testing-integration`, `testing-fundamentals`. | +| **Workflow position** | Second implementation session of the W006 Telemetry chain (PR B). Realizes **CritterCab's first gRPC surface in code** and its **first non-event-sourced document write path**. Follows PR #42 (skeleton + slice 1). One slice per PR is the rule; slices 4+2 share this PR because they are mutually dependent (see Framing) — this is not the skeleton+first-slice exception (already spent), it is the "coupled slices" reading, and the § Fallback split names how to break them if the PR gets large. | + +--- + +## Framing — why this session exists + +Slice 1 (PR #42) landed the `TelemetryPolicy` config view that the ingest reads. This session builds the two slices that make Telemetry actually *ingest*: **slice 2** (the windowed gRPC `ReportLocations` client-streaming surface) and **slice 4** (the `LastKnownPosition` overwrite-in-place document + heartbeat-absence eviction sweep). They pair because slice 2's publish trigger (`shouldPublish = heartbeatDue OR (cellChanged AND throttleFloorElapsed)`) evaluates against slice 4's `LastKnownPosition` baseline, and slice 4's document is written *only* when slice 2 decides to publish. Building them apart would mean slice 2 evaluating against a document that does not yet exist, or slice 4 writing a document nothing reads. + +**The forward-constraint that shadowed this slice for three months is gone.** WolverineFx.Grpc `V6.21.0` (shipped 2026-07-20, authored by the CritterCab maintainer) added **client-streaming auto-codegen** for both proto-first and code-first shapes. `ReportLocations` — `stream LocationPing → single LocationIngestAck`, the exact `stream in → unary out` shape — is now auto-generated like the unary and server-streaming RPCs. **The hand-wire-against-`IMessageBus` workaround recorded in every prior handoff is obsolete; do not hand-wire this slice.** This session therefore also corrects the two gRPC skills that still describe client-streaming as unsupported (a session-runner-blocking skill fix — the session cannot follow those skills as written). + +**No narrative anchors this session.** PR #40 decided the narrative layer does not apply to Telemetry (machine-to-machine; no protagonist-perceivable moment — [`narratives/README.md`](../../narratives/README.md) § "When the narrative layer does not apply"). W006 is the direct spec anchor. + +--- + +## Goal + +Produce a running gRPC ingest for Telemetry: bump WolverineFx to 6.21.0, generate the C# gRPC surface behind `report_locations.proto`, implement W006 slice 2 (`ReportLocations` client-streaming ingest — per-ping validation, H3 cell computation, publish-trigger evaluation, window-close ack) as a **proto-first `[WolverineGrpcService]`** stub forwarding to a Wolverine handler, and W006 slice 4 (`LastKnownPosition` upsert-on-publish document + the periodic heartbeat-absence eviction sweep), with Alba/gRPC integration tests covering the §6.2 and §6.4 GWTs. + +--- + +## Spec delta + +- **W006 §6.2 moves designed → realized in code.** The `ReportLocations` client-streaming ingest, the per-ping in-flight pipeline, and the `shouldPublish` trigger become concrete. **First gRPC surface and first client-streaming RPC in CritterCab** — the WolverineFx.Grpc feature the whole project exists to showcase, running in a `.cs` file for the first time. +- **W006 §6.4 moves designed → realized in code.** `LastKnownPosition` (overwrite-in-place Marten document, not an event stream) and the eviction sweep become concrete. **First non-event-sourced document write path and first recurring/scheduled handler in the repo.** +- **The client-streaming forward-constraint is retired, not carried.** W006 §6.2's implementation note (and the shipped proto's comment) recorded "hand-wire against `IMessageBus`"; this session records in W006 `## Document History` that 6.21.0 auto-generates the shape and the constraint is closed. +- **W006 §11 ADR candidate fires (or is skill-homed):** *windowed gRPC client-streaming ingest* — decide at authoring whether it lands as an ADR or a `docs/skills/` skill (§6.2 and the handoff both lean skill). The other two §11 candidates (Kafka topic-naming, stream-processing-as-4th-shape) are PR C / best-evidenced-later — named here as explicit deferrals. + +--- + +## Orientation files (read in order) + +1. **[W006 §6.2, §6.4, §3.3, §4, §11](../../workshops/006-telemetry-event-model.md)** — the two slice specs, the `LastKnownPosition` document sketch, the UL (H3 cell / heartbeat / throttle policy), and the ADR candidates. The GWT sketches in §6.2/§6.4 are the test contract. +2. **The shipped proto** — [`protos/crittercab/telemetry/v1/report_locations.proto`](../../../protos/crittercab/telemetry/v1/report_locations.proto). `service TelemetryService { rpc ReportLocations(stream LocationPing) returns (LocationIngestAck); }`, `csharp_namespace = "CritterCab.Telemetry.V1"`. Note the payload field is **`lon`** (not the workshop prose's `lng`); the contract wins. +3. **gRPC skills** — `docs/skills/wolverine-grpc-handlers/SKILL.md` (proto-first `[WolverineGrpcService]` stub deriving the generated `*Base`; handler lives in the request's vertical slice, not a `Grpc/` folder; unary + server-streaming shapes) and `docs/skills/wolverine-grpc-bidirectional-handlers/SKILL.md`. **Both are STALE on client-streaming as of 6.21.0, but asymmetrically** (inventory 2026-07-20): `handlers` has ~6 localized false lines this session corrects + re-homes the now-auto-generated pattern into (deliverable 13a); `bidirectional-handlers` is structurally premised on the hand-written workaround (~150 lines) — this session only banners it (13b), the rewrite is DEBT (deliverable 15a). **Do not follow either skill's "hand-wire client-streaming" guidance** — it is obsolete; the auto-gen path in § Verify-before-wiring #1 is authoritative. `docs/skills/cli-grpc-tooling/SKILL.md` for buf/grpcurl invocation. +4. **Reference implementations** — `src/CritterCab.Telemetry/TelemetryPolicy/` (the slice-1 idiom this service already uses: positional `sealed record`s, feature-folder layout, static endpoints); `tests/CritterCab.Telemetry.Tests/TelemetryTestFixture.cs` (Testcontainers-Postgres + `AlbaHost.For`). For the recurring-handler shape, `docs/skills/wolverine-marten-automation/SKILL.md` + the Dispatch automation handlers. +5. **6.21.0 API surface (verified 2026-07-20 against local `C:\Code\JasperFx\wolverine` @ `V6.21.0-12`)** — the § Verify before wiring gates below record the exact source citations; re-confirm shape, not existence. + +--- + +## Working pattern + +- Interactive, sign-off per logical chunk, in dependency order: (a) **version bump + proto codegen** green (the gRPC `*Base` type generates and the project builds) → (b) **slice 4** (`LastKnownPosition` document + upsert + eviction sweep, unit + Alba) → (c) **slice 2** (`ReportLocations` stub + handler + trigger, gRPC integration test). Slice 4 precedes slice 2 so the trigger has a real baseline to evaluate and store against. +- **Resolve the three § Decisions-needing-sign-off forks first** (Kafka scope, `driverId`/principal, H3 binding) — they change the deliverable plan. +- **Verify-before-wiring gates (next section) before committing code that depends on them.** The local JasperFx checkout is now current (`V6.21.0-12`) — no staleness caveat this time; still record the checked-out version in each verdict per standing discipline. +- Local Docker container-start was wedged at PR #42 (`docker run` hung). **Re-probe `docker run --rm hello-world` at session start.** If still wedged, lean on CI as the gate (proven: 15/15 on #42); gRPC + Testcontainers-Postgres tests need a working daemon, so a green local run needs Docker back. +- Positional `sealed record`s for commands/events/views/messages — match the shipped Dispatch + Telemetry-slice-1 idiom. `LocationPing`/`LocationIngestAck` are **generated protobuf types**, not hand-authored records — do not re-declare them. +- Branch `telemetry/slices-4-and-2-transport` (already created); never commit to `main`. Retro ships in this PR. Run `critter-skill-auditor` Phase 1 before cutting code, Phase 2 after. + +--- + +## Verify before wiring (jasperfx-source-verifier — local `C:\Code\JasperFx\wolverine` @ `V6.21.0-12`) + +The 6.21.0 client-streaming surface was source-verified during this prompt's authoring (citations below). Re-confirm the shape holds and resolve the remaining open gates. + +1. **Proto-first client-streaming codegen (VERIFIED — re-confirm).** A `[WolverineGrpcService]`-marked stub deriving the Grpc.Tools-generated `TelemetryService.TelemetryServiceBase` gets a generated override that forwards to `IMessageBus.StreamAsync`. Emit path: `GrpcServiceChain.cs:308-311` (`ClientStreaming` case → `ForwardClientStreamToMessageBusFrame`), generating `return await _bus.StreamAsync(WolverineGrpcStreamAdapters.ReadAllAsync(requestStream, ct), ct);` (`:669-711`). The classifier now populates `ClientStreamingMethods` (`:77-80`, `:454-462`) — the symmetry the pre-6.21 gap lacked. +2. **Handler-side dispatch target (VERIFY).** `IMessageBus.StreamAsync(IAsyncEnumerable messages, CancellationToken, TimeSpan?) : Task` is the real signature (`Wolverine/IMessageBus.cs:151`). Confirm the **Wolverine handler shape** it dispatches to — expected `public static Task Handle(IAsyncEnumerable pings, /* deps */)` — and how handler discovery matches a stream-consuming handler (name/return conventions). This is the load-bearing gate: the handler *is* the ingest, and it must be a discoverable Wolverine handler, not the stub. +3. **⚠ Middleware / `Validate` short-circuit is NOT woven for client-streaming (VERIFIED — design consequence).** Before-frames require a concrete `TRequest` in scope at method entry, which a client stream cannot provide (`GrpcServiceChain.cs:270-272`, mirrored in `CodeFirstGrpcServiceChain.cs:220-224`). **Therefore per-ping validation (lat/lon range, `accuracyMeters` threshold) and the publish-trigger evaluation live INSIDE the handler, not at the boundary** — the opposite of slice 1's `ConfigureTelemetryPolicy` FluentValidation `Before()`. Do not attempt a boundary validator on the ingest. +4. **`[EnumeratorCancellation]` / cancellation flow (VERIFY).** The generated wrapper adapts `IAsyncStreamReader` via `WolverineGrpcStreamAdapters.ReadAllAsync` (`Wolverine.Grpc/WolverineGrpcStreamAdapters.cs:22`, carries `[EnumeratorCancellation]`) and passes `context.CancellationToken`. Confirm the handler enumerating `IAsyncEnumerable` receives cancellation on client half-close/disconnect so a window terminates cleanly. +5. **Recurring eviction sweep — no first-class Wolverine primitive (RESOLVED 2026-07-21 trace; corrects the authoring hypothesis).** Both authoring guesses were wrong: `IScheduledJobProcessor`/`ScheduleAsync` is one-shot *delayed delivery*, not periodic, and `PublishMessage().ToLocalQueue()` is *routing* config, not a scheduler. There is **no** recurring-message API. The idiom — used by Wolverine's own internals — is a plain .NET `BackgroundService` whose `ExecuteAsync` loops on `Task.Delay(interval, stoppingToken)` and calls `IMessageBus` each tick, registered via `builder.Services.AddHostedService<...>()`. Wolverine contributes only the `IMessageBus` call *inside* the loop; the recurrence is a vanilla hosted-service primitive. **Template: `Wolverine/Runtime/Heartbeat/HeartbeatBackgroundService.cs:17-78`** — reads its interval from a policy object; wraps the tick body in try/catch ("must never crash the host", `:71-75`); returns cleanly on `OperationCanceledException` (`:49-51`, `:67-69`). Minimal sample: `Samples/PingPong/Pinger/Worker.cs:7-32`. **Recommended shape — split the timer from the work:** a thin `LastKnownPositionEvictionService : BackgroundService` that each tick does `await bus.InvokeAsync(new EvictStalePositions())` (`InvokeAsync`, **not** `PublishAsync` — inline + awaited gives back-pressure so a long sweep can't overlap the next), plus a normal Wolverine handler `EvictStalePositions` holding all the testable logic (threshold `now - 3 × HeartbeatIntervalSeconds` read from the `TelemetryPolicy` view; the delete). The handler is the §6.4 "Evict"/"Return" GWT target; the `BackgroundService` shell stays untested. `wolverine-marten-automation` is prior art for the handler but was event-triggered — the timer half is new to the repo. +6. **Marten document upsert/delete for `LastKnownPosition` (VERIFIED 2026-07-21 trace — one refinement).** `session.Store(doc)` overwrite-in-place for a plain document keyed by `driverId` holds (still confirm the upsert against source when wiring). The bulk-delete-by-predicate API is confirmed: `DeleteWhere(Expression>)` (`IDocumentOperations.cs:74`, impl `DocumentSessionBase.Deletes.cs:152`). **Refinement — prefer `HardDeleteWhere` (`IDocumentOperations.cs:225`) for the eviction sweep.** `DeleteWhere` is *conditional*: if `LastKnownPosition` is ever configured for soft-deletes it silently switches to setting the `mt_deleted` flag and the row stays — which would break slice 4's "Return" GWT (an evicted driver must find *no* baseline and publish immediately). Today the doc is plain (no soft-delete config), so `DeleteWhere` hard-deletes; but `HardDeleteWhere(x => x.ServerReceivedAt < threshold)` makes "the row must be gone" immune to a future soft-delete config change. Decision for the session: `HardDeleteWhere` for intent-clarity, or plain `DeleteWhere` + a test asserting the row is truly absent. First plain-document write path in the repo (everything prior is event-sourced). +7. **WolverineFx 6.21.0 transitive dependency line (VERIFY + CI).** Bumping `WolverineFx`/`WolverineFx.Grpc` 6.19.0→6.21.0 pulls a newer Marten/JasperFx transitively (main was on Marten 9.14 via 6.19). Confirm no new breaking surface (Marten 9 `partial` projection rule already handled; watch for JasperFx 2.x namespace moves) and let CI be the backstop. +8. **H3 cell computation via `H3.net` (pocketken) — API VERIFIED 2026-07-21 via ctx7; (a) correctness recipe changed from the authoring hypothesis, (b) still open.** Slice 2 step 3 computes the cell at `h3Resolution`. Two API paths exist, and they are **mirror opposites on both axes** of the footgun: `Model.LatLng` is **(lat, lon)** in **radians**; NTS `Coordinate` is **(lon, lat)** = (X, Y) in **degrees** (the library's own `docs/api-indexing.md` states this contrast explicitly). The proto carries `lat`/`lon` as **degrees**. + - **(a) RECOMMENDED PATH — NTS `Coordinate`, degrees-native (not the `LatLng`/tuple radians path the authoring pass leaned toward).** `var cell = new Coordinate(ping.Lon, ping.Lat).ToH3Index(policy.H3Resolution);` — `X = lon` first, `Y = lat` second. Degrees in, so **no radian conversion** (kills one of the two footguns); NTS is pulled transitively anyway (zero added dependency); the remaining risk collapses to the single axis-order line. **⚠ TRAP — do NOT use `H3Index.FromLatLng` via the `(double, double)` tuple.** `LatLng` is (lat, lon) **radians**, and its `implicit operator LatLng((double,double) c) => new(c.Item1, c.Item2)` does **no** degree→radian conversion (`H3Index.cs:589` + the struct) — despite ctx7 prose claiming the tuple "accepts degrees", a **doc self-contradiction**. Passing degrees through the tuple is a ~57× scale error that still yields a valid-*looking* cell. If the `LatLng` path is ever used deliberately, convert by hand: `new LatLng(latDeg * Math.PI/180, lonDeg * Math.PI/180)`. + - **Guard behavior:** `FromLatLng`/`ToH3Index` **return `H3Index.Invalid` — they do not throw** — on out-of-range resolution (`resolution is < 0 or > MAX_H3_RES`, and `MAX_H3_RES` = 15, aligning with the proto's 0–15 policy bound) or a non-finite coordinate. A valid `TelemetryPolicy` keeps resolution in range, so treat `Invalid` as the drop-and-count-invalid path (dovetails slice 2's silent-drop rule — a second safety net after per-ping non-finite validation), never as a throw to catch. + - **Pinning test (the actual anti-footgun — a "cell is valid" assertion passes with lat/lon swapped, so it must prove order AND units):** compute the same point via **both** paths — `new Coordinate(lonDeg, latDeg).ToH3Index(9)` and `H3Index.FromLatLng(new LatLng(latDeg*Math.PI/180, lonDeg*Math.PI/180), 9)` — and assert **equal** (the paths are opposite on both axes, so equality catches a unit *or* axis slip in either; two wrongs can't accidentally agree). Then assert the swapped `Coordinate(latDeg, lonDeg)` yields a **different** cell, and assert `.IsValid()`. Prefer this cross-check over a hardcoded "magic cell id" recited from H3 upstream docs. + - **(b) CLOSED 2026-07-24 — registry facts confirmed against nuget.org.** Package id is **`pocketken.H3`**, latest **`4.5.0.1`** (the bare `H3` id is a different, prerelease-only package — do not use it). The `.nupkg` ships a **native `lib/net10.0/pocketken.H3.dll`** (alongside net8.0 / netstandard2.0 / netstandard2.1), so it loads under CritterCab's `net10.0` target with no fallback. Its sole `net10.0` dependency is **`NetTopologySuite 2.6.0`** — which confirms 8a's premise that the `Coordinate` path adds no dependency beyond what H3 already pulls, and confirms the pure-managed/no-P-Invoke CI story. **Gate 8 is fully closed; nothing in gate 8 blocks any chunk.** + +**Gates surfaced by the 2026-07-21 end-to-end source trace (post-authoring; local `@ V6.21.0-12`).** Tracing a client-streaming call hop-by-hop through the shipped source **closed gates 1–4** — the proto-first codegen emit (`GrpcServiceChain.cs:308-311` → frame `:697-722`), the handler shape/discovery (gate 2, the load-bearing one), the middleware skip (`:272`, `:319`), and cancellation (`WolverineGrpcStreamAdapters.cs:22` + the `Cancelled`-status test) all verified against source. **Copy-template found:** `src/Wolverine.Grpc.Tests/GrpcClientStreaming/` — `CollectStub` (empty `[WolverineGrpcService]` stub), `CollectHandler` (`Task Handle(IAsyncEnumerable, CancellationToken)`), and `ClientStreamingFixture` (host recipe). The trace also surfaced three questions the authoring pass missed: + +9. **Cascade / outbox semantics of a stream-invoked handler (VERIFY — handler chunk).** `IMessageBus.StreamAsync` dispatches via `findStreamInvoker()` → `InvokeAsync(...)` keyed on `typeof(IAsyncEnumerable)` (`MessageBus.cs:244-257`) — a normal unary-style invoke where the *whole stream* is the message. **Open:** does that path run the same `MessageContext` flush a unary invoke does — i.e. do **cascading messages** and `[Transactional]`/outbox middleware weave for a handler whose message type is `IAsyncEnumerable`? The current design sidesteps this by fanning out on publish via **injected dependencies called directly** (`IDriverLocationPublisher`, `session.Store`), not cascaded messages — so this only needs confirming if the handler is changed to emit cascading messages or to carry transactional middleware. If direct-call stays, record in the retro that cascade/outbox on a client-streaming handler was deliberately not exercised. +10. **Two client-streaming RPCs sharing a request type collide (DESIGN CONSTRAINT — document, don't block).** The handler slot is keyed on `typeof(IAsyncEnumerable)`, not the RPC name (`MessageBus.cs:248`); two client-streaming RPCs both streaming the same `TRequest` would map to one handler — the disambiguator is the *request type*. Moot for CritterCab v1 (one client-streaming RPC, one `LocationPing`), but record it as a known limitation in the `wolverine-grpc-handlers` proto-first client-streaming subsection (deliverable 13a). Candidate for an upstream known-limitation note (user's JasperFx role — out of CritterCab scope). +11. **Alba host → gRPC channel bridge (VERIFY — slice-2 test chunk).** The reference fixture builds a `GrpcChannel` over a raw `WebApplication…UseTestServer()` host (`ClientStreamingFixture.cs:32-45`: `AddGrpc()` + `AddWolverineGrpc()` + `UseRouting()` + `MapWolverineGrpcServices()`, then `GrpcChannel.ForAddress("http://localhost", new() { HttpHandler = testServer.CreateHandler() })`). CritterCab standardizes on `AlbaHost.For()` (static-endpoints / Alba-first, R). Confirm a `GrpcChannel` can be built over Alba's wrapped `TestServer` (reach `albaHost.Server.CreateHandler()` or equivalent). If it cannot cleanly, fall back to a dedicated gRPC fixture mirroring the Wolverine one and document the deviation from the Alba-first default in the retro. + +--- + +## Decisions resolved (user sign-off 2026-07-20) + +1. **Kafka — deferred to PR C behind a stub seam.** PR B stays "gRPC + document." Slice 2's `shouldPublish` fan-out to the Kafka publish goes behind an `IDriverLocationPublisher` seam (a recording/no-op `LoggingDriverLocationPublisher` in PR B, mirroring Dispatch's `PricingClientStub`/`Forwarding*` ready-to-swap pattern); PR C swaps it for the real WolverineFx.Kafka producer + `apphost.cs` wiring + Kafka Testcontainer. **This overrides the post-PR42 handoff line that put the Kafka Testcontainer in PR B** — recorded as the deliberate scoping call. +2. **`driverId` — `IDriverPrincipalAccessor` seam.** A stubbed accessor reads a well-known claim/header (`x-driver-id` in dev), enforced in the handler (never the payload, R5), with the resolution point documented as the swap site for real Entra-issued claims once Identity is built. Mirrors CritterCab's ready-to-swap seam idiom. +3. **H3 — `H3.net` (pocketken, `/pocketken/h3.net`).** Pure-managed C# port (no native P/Invoke → clean CI). **The binding recipe was corrected by the 2026-07-21 gate-8 pass — this fork's original `H3Index.FromLatLng(LatLng, resolution)` wording is superseded.** Use the NTS `Coordinate`-degrees path, `new Coordinate(ping.Lon, ping.Lat).ToH3Index(policy.H3Resolution)`; the `FromLatLng`/tuple path is a radians trap (§ Verify-before-wiring #8a). **Registry facts closed 2026-07-24:** package id is **`pocketken.H3`** (`4.5.0.1`), it ships a native `lib/net10.0` target, and it pulls **NetTopologySuite 2.6.0** transitively — confirming the zero-added-dependency premise the `Coordinate` path rests on. **Gate 8 is now fully closed (8a + 8b).** + +--- + +## Deliverable plan + +**A. Version bump + proto codegen (chunk a)** +1. `Directory.Packages.props` — **all 11 `WolverineFx*` entries** 6.19.0 → **6.21.0 in lockstep** (bumping only `WolverineFx` + `WolverineFx.Grpc` would leave the other nine as a version island); add `Grpc.Tools`, `Google.Protobuf`, `Grpc.AspNetCore` (proto-first C# codegen); add `pocketken.H3` (gate 8b). **6.21.0, not the newer 6.22.0** — 6.21.0 is the version the § Verify-before-wiring citations were source-verified against (`V6.21.0-12`); 6.22.0 is a later `tidy: packages` bump, verified on its own terms. **No `WolverineFx.Kafka` version change beyond the lockstep bump and no `WolverineFx.Kafka` PackageReference** — Kafka is deferred to PR C. +2. `src/CritterCab.Telemetry/CritterCab.Telemetry.csproj` — `` (+ the proto import root). Generates `TelemetryService.TelemetryServiceBase`, `LocationPing`, `LocationIngestAck` in `CritterCab.Telemetry.V1`. **First codegen-behind-a-proto in the repo.** Verify `dotnet build` emits the `*Base` type before wiring anything to it. +3. `src/CritterCab.Telemetry/Program.cs` — add gRPC to the host (`AddGrpc`/`MapWolverineGrpcServices` per gate 1) and register the eviction sweep as `builder.Services.AddHostedService()` — a **plain .NET `BackgroundService`**, because gate 5's trace established Wolverine has **no first-class recurring/scheduled-message primitive**; the recurrence is a vanilla hosted-service concern and Wolverine contributes only the `IMessageBus` call inside the loop. `apphost.cs` — expose the Telemetry gRPC port (HTTP/2); **no Kafka resource** (PR C). + +**B. Slice 4 — `LastKnownPosition` store + eviction (chunk b; `LastKnownPosition/` feature folder)** +4. `LastKnownPosition` Marten document (per §3.3): `driverId` (id), `lat`, `lon`, `h3Cell`, `serverReceivedAt`, `lastPublishedAt`. Plain document, overwrite-in-place, LWW on `serverReceivedAt` — **not** an aggregate/projection, no `partial` source-gen needed. +5. Upsert path invoked on `shouldPublish` (upsert-on-publish only, never per-ping — §6.4). Wire it so slice 2's trigger calls it; publish-first ordering means the Kafka publish (or its stub seam) runs before the upsert. +6. Eviction sweep — **split timer from work** (gate 5): (i) `LastKnownPositionEvictionService : BackgroundService` — a thin shell looping on `Task.Delay(interval, stoppingToken)` that each tick calls `await bus.InvokeAsync(new EvictStalePositions())` (`InvokeAsync`, **not** `PublishAsync`, so back-pressure prevents a long sweep overlapping the next tick); wrap the tick body in try/catch and return cleanly on `OperationCanceledException` per the `HeartbeatBackgroundService` template. The shell stays untested. (ii) `EvictStalePositions` — a normal Wolverine handler holding all the testable logic: `HardDeleteWhere` (gate 6 refinement — **not** plain `DeleteWhere`, which silently degrades to a soft-delete flag if the document is ever configured for soft-deletes, breaking the "Return" GWT) where `serverReceivedAt < now - 3 × heartbeatIntervalSeconds` (threshold read from the `TelemetryPolicy` view; documented constant `3×`, **not** a policy param in v1). The handler is the §6.4 "Evict"/"Return" GWT target. No per-driver timers; no staleness event published (v1, R8). +7. Tests (§6.4 GWTs): **Upsert** (publish → document overwritten), **No-write** (accepted-but-not-published → unchanged), **Evict** (stale doc → swept), **Return** (evicted driver's next ping has no baseline → trigger publishes immediately). + +**C. Slice 2 — `ReportLocations` gRPC ingest (chunk c; `ReportLocations/` feature folder)** +8. `TelemetryServiceGrpcService` (or `GrpcService` per skill naming) — an `abstract` `[WolverineGrpcService]` stub deriving `TelemetryService.TelemetryServiceBase`. Empty; Wolverine generates the wrapper (gate 1). **Not** in a `Grpc/` folder — but the stub itself is the discovery declaration; the *handler* lives in the `ReportLocations/` slice. +9. `ReportLocationsHandler` — the Wolverine handler `Task Handle(IAsyncEnumerable pings, /* IDriverPrincipalAccessor, IQuerySession, TelemetryPolicy view, IDriverLocationPublisher, ... */)` (shape per gate 2). Implements the §6.2 per-ping pipeline **inside the handler** (gate 3): stamp `serverReceivedAt`; validate (silently drop invalid, count passes); compute H3 cell (gate 8); evaluate `shouldPublish` against `LastKnownPosition` with cache-within-window/re-read-on-open (the driver's own pings are the sole writer during a window); on publish, fan out to the publisher seam + the slice-4 upsert; return `LocationIngestAck { acceptedCount, serverTime, throttlePolicyVersion }` on half-close. +10. `driverId` resolution via the `IDriverPrincipalAccessor` seam — from the principal claim/header (`x-driver-id` in dev), never the payload (R5); enforce in the handler, document the swap site for real Entra claims. +11. Tests (§6.2 GWTs, gRPC client-streaming): **Happy publish** (cell-change + floor elapsed → publish + upsert fire), **Throttled** (cell-change, floor not elapsed → accepted, no publish/store), **Heartbeat** (same cell, heartbeat due → publish + upsert), **Window close** (N pings, M valid → `acceptedCount: M`). Drive a real client stream against the Alba/gRPC host. + +**D. Docs / ledger** +12. `protos/crittercab/telemetry/v1/report_locations.proto` — correct the stale "hand-wire against IMessageBus / re-verify 6.8" comment (lines 21-26) to record 6.21.0 auto-generates client-streaming (in-bounds: this session wires codegen against this proto). +13. **gRPC-skill corrections — asymmetric, scoped deliberately (inventory done 2026-07-20):** + - **13a. `docs/skills/wolverine-grpc-handlers/SKILL.md` (blocking — correct + re-home).** Correct the ~6 stale "Wolverine 5.32 doesn't auto-generate client-streaming / use the hand-written workaround" claims (intro ¶ ~L16; why-not-code-first ~L78+L80; concrete-stub note ~L178; the **pitfall ~L657 that now states the exact opposite of the truth**; see-also ~L681). **And add a compact proto-first client-streaming subsection** mirroring the existing server-streaming one — this skill is the auto-gen home for the shape slice 2 implements. Show the `[WolverineGrpcService]` stub → generated `_bus.StreamAsync(WolverineGrpcStreamAdapters.ReadAllAsync(...), ct)` wrapper, the `Task Handle(IAsyncEnumerable, …)` handler, and the **no-middleware/`Validate`-weaving caveat** (same asymmetry the skill already documents for bidi). + - **13b. `docs/skills/wolverine-grpc-bidirectional-handlers/SKILL.md` (tiny — anti-contradiction banner ONLY).** Add a "⚠ SUPERSEDED as of WolverineFx.Grpc 6.21.0 — client-streaming is now auto-generated; the hand-written workaround below is legacy. See `wolverine-grpc-handlers` and DEBT row ." banner at the head of the client-streaming section (~L150), fix the mental-model table row (~L51 "Rejected at startup") and the frontmatter one-liner. **Do NOT rewrite the ~150-line hand-written-workaround body (L150–293) or the pitfalls here** — that structural rewrite is a `tidy: skills` DEBT row (deliverable 15), not PR B. The banner exists only so the repo doesn't actively contradict the code this PR ships. +14. `docs/workshops/006-telemetry-event-model.md` `## Document History` — slices 2 + 4 realized; the client-streaming forward-constraint closed by 6.21.0. +15. `docs/skills/DEBT.md` — register **two** rows: (a) the **`wolverine-grpc-bidirectional-handlers` structural rewrite** — retitle to bidirectional-only (or "both shapes, both auto-generated"), delete the ~150-line hand-written-workaround section (Patterns A/B, the fail-fast text, applies/doesn't-apply lists), rewrite the mental-model table + the ~4 client-streaming pitfalls; a `tidy: skills` session with `critter-skill-auditor`, referenced by the 13b banner; (b) the **windowed gRPC client-streaming skill** (if § Spec delta lands the §11 candidate as a skill rather than an ADR). `docs/prompts/README.md` — Implementations index entry. +16. This prompt's retro at `docs/retrospectives/implementations/007-telemetry-slices-4-and-2-transport.md`. + +--- + +## Out of scope + +- **Slice 3 (`DriverLocationUpdated` → Kafka)** — PR C. The publish is a stub seam (`IDriverLocationPublisher`) in PR B. +- **Slice 5 (Dispatch consumer / `NearbyAvailableDriversStub` replacement)** — PR D; the Kafka half of ADR-018's join only, and only after slice 3. +- **The ASB / Driver-Profile-availability half of the join** — an ADR-018 forward-constraint to an un-workshopped BC. Do not model or build it. +- **Real authentication / Identity integration** — the `driverId` seam is a stub with a documented swap site (fork 2), not a real auth pipeline. +- **Bidirectional `ReportLocations`** — R5 fixes v1 as client-streaming; bidi is a v2 deferral. Do not "upgrade" the RPC to dodge anything (there is nothing to dodge — 6.21.0 auto-generates client-streaming). +- **v2 staleness-ceiling / eviction-as-event propagation** — R3/R8 defer it; eviction is Telemetry's own storage hygiene in v1. +- **The `wolverine-grpc-bidirectional-handlers` structural rewrite** — deleting/rewriting the ~150-line hand-written-workaround section is a `tidy: skills` DEBT row (deliverable 15a); PR B only banners it (13b). PR B corrects + re-homes into `wolverine-grpc-handlers` (13a) and stops there. +- **CLAUDE.md status-line refresh** — still stale ("first vertical slice / all other BCs pre-workshop"); a `tidy: housekeeping` pass, flag in retro, do not edit here. + +--- + +## Fallback split (if the PR gets large) + +Slices 4 and 2 can separate cleanly: **PR B1** = version bump + proto codegen + slice 4 (`LastKnownPosition` + eviction, exercised by direct handler/Marten tests). **PR B2** = slice 2 (`ReportLocations` gRPC ingest + trigger) against the now-existing document. The dependency runs one way (2 reads/writes 4), so 4-first is the valid order. Prefer the single coupled PR; split only if review surface warrants. + +--- + +## Follow-on PR sequence (arc context; not this session) + +- **PR C** — Slice 3 (`DriverLocationUpdated` → Kafka topic `telemetry.driver-location-updated`, partition `driverId`, publish-first/no-outbox). Swaps the `IDriverLocationPublisher` seam for the real WolverineFx.Kafka producer; wires Kafka into `apphost.cs` + the Kafka Testcontainer; fires the Kafka-topic-naming §11 ADR candidate. +- **PR D** — Slice 5 (Dispatch consumer: replace `NearbyAvailableDriversStub` with the Kafka-fed `AvailableDriver` view — Kafka half of the join only). + +--- + +## Document history + +- **2026-07-20.** Drafted after WolverineFx.Grpc `V6.21.0` shipped client-streaming auto-codegen (retiring the hand-wire forward-constraint). API surface source-verified against local `C:\Code\JasperFx\wolverine` @ `V6.21.0-12`. Three durable forks (Kafka scope, `driverId` provenance, H3 binding) flagged for sign-off before the session runs. +- **2026-07-21.** Full **Verify-before-wiring** pass — source-verified against local `C:\Code\JasperFx\wolverine` @ `V6.21.0-12`, `C:\Code\JasperFx\marten`, and ctx7 `/pocketken/h3.net`. **Gates 1–4 closed** by an end-to-end client-streaming trace; concrete copy-template found at `src/Wolverine.Grpc.Tests/GrpcClientStreaming/` (`CollectStub` + `CollectHandler` + `ClientStreamingFixture`). **Gate 5 corrected** — no first-class recurring primitive exists; the idiom is a `BackgroundService` + `Task.Delay` + `IMessageBus` (template `HeartbeatBackgroundService`), the authoring guesses (`IScheduledJob` / `ToLocalQueue`) were wrong. **Gate 6** verified + `HardDeleteWhere` refinement. **Gate 8a** recipe changed to the NTS `Coordinate(lon, lat)` degrees path (the `LatLng`/tuple radians path is a trap — ctx7 prose claiming the tuple "accepts degrees" contradicts the code); **8b** (package id / net10 TFM) is the lone open sub-gate. **Gates 9–11 added** (cascade/outbox on a stream-invoked handler; request-type collision; Alba→gRPC channel bridge). Net: **nothing open blocks Chunk A** (gate 7 = the version bump itself). **Still owed (internal consistency, not yet applied):** fork 3 in § Decisions-resolved and deliverables 3 + 6 still use the pre-correction vocabulary for the H3 API and the "recurring handler" — align them to gates 5 and 8 before the prompt ships. +- **2026-07-24 (session start, PR B chunk A).** **Internal-consistency debt from the 2026-07-21 entry discharged:** fork 3 (§ Decisions resolved) now names the NTS `Coordinate`-degrees recipe and marks the `FromLatLng` wording superseded; deliverables 3 and 6 now carry gate 5's `BackgroundService` + `EvictStalePositions`-handler split (replacing "recurring scheduler"/"recurring handler") and gate 6's `HardDeleteWhere` refinement. **Gate 8b closed** against nuget.org — package id `pocketken.H3` `4.5.0.1`, native `lib/net10.0`, transitive `NetTopologySuite 2.6.0`; **gate 8 is now fully closed, and no Verify-before-wiring gate remains open outside the chunks that will exercise it** (9 = handler chunk, 11 = slice-2 test chunk). **Version target confirmed at 6.21.0** by user sign-off even though 6.22.0 had shipped, so the gate citations keep matching the local `V6.21.0-12` checkout; the bump is **lockstep across all 11 `WolverineFx*` entries** (deliverable 1 amended). Branch hygiene: the pre-existing branch commit was titled as an implementation but contained only this prompt — rewritten to a `docs:` subject. diff --git a/docs/retrospectives/README.md b/docs/retrospectives/README.md index a56f735..f4fd66f 100644 --- a/docs/retrospectives/README.md +++ b/docs/retrospectives/README.md @@ -111,6 +111,7 @@ Phase 1–3 retrospectives were not authored at the time those phases ran (the r - [`005-dispatch-slice-5-3-candidates-selected.md`](./implementations/005-dispatch-slice-5-3-candidates-selected.md) — Fourth vertical slice: `CandidateSelectionAutomation` reacts to `FareQuoted`, queries `INearbyAvailableDriversSource` (stub; parking-lot #4 deferred), and emits `CandidatesSelected` (≥1 eligible candidate, capped and ordered by inverse-distance match score) or `NoCandidatesAvailable` (with `NoDriversInRange` / `NoCapableDriversInRange` reason). Adds `DispatchPolicySnapshot` DI record, `ICandidateSelectionOutcome` marker interface (third occurrence), `RequestRoundsProjection` (consumed by Slice 9), and `RequestTimeline` extensions for both outcome events. First use of `[WriteAggregate]` bound to a non-first stream event; verified against Wolverine source. **Third substantive forward exercise of the spec-delta closure-loop convention** — both prompt-named amendments (narrative 002 v0.2, W001 v0.7) landed as named. Triggered by [`prompts/implementations/005-dispatch-slice-5-3-candidates-selected.md`](../prompts/implementations/005-dispatch-slice-5-3-candidates-selected.md). Status: complete (2026-06-16). - [`004-dispatch-slice-5-2-fare-quoted-failure-paths.md`](./implementations/004-dispatch-slice-5-2-fare-quoted-failure-paths.md) — Slice 5.2 closure session: lands the three FareQuoteFailed alternate-path GWTs (transient retry recovery, exhausted retries, non-transient failure) deferred from slice 003. Adds `FareQuoteFailed` event, `FareQuoteAttempts` terminal-events-only projection, `IFareQuoteOutcome` marker interface, `FareQuoteRetryPolicy` DI record (pre-shapes Slice 11's `DispatchPolicyConfigured` seam), two `IPricingClient` exception types, and a manual retry loop with two `TransientPricingException` catches (the unfiltered second catch fixes a control-flow bug in the prompt's design-decision snippet — unreachable fallback `return` after the for-loop). **Second substantive forward exercise of the spec-delta closure-loop convention** — meets ADR-016's 2–3-exercises deferral trigger. Two prompt-named amendments (narrative 001 v0.4, W001 v0.6) landed as named. Surfaced one workshop-§5.2 *Reads*-list inconsistency for a future workshop-tidy session. Bundling pattern at fourth confirmation; methodology session recommended next. Triggered by [`prompts/implementations/004-dispatch-slice-5-2-fare-quoted-failure-paths.md`](../prompts/implementations/004-dispatch-slice-5-2-fare-quoted-failure-paths.md). Status: complete (2026-05-19). - [`006-telemetry-skeleton-and-slice-1-config.md`](./implementations/006-telemetry-skeleton-and-slice-1-config.md) — **Second service in the repo** (`CritterCab.Telemetry`) + W006 slice 1 (`TelemetryPolicyConfigured` config-as-events) in one PR (skeleton-plus-first-slice cadence exception). Opening PR of the W006 transport chain — but lands **no transport** (config-as-events is the dependency-correct first slice; slice 2's gRPC ingest reads this slice's `TelemetryPolicy` view). Singleton event stream, boundary FluentValidation, `long` `throttlePolicyVersion` from the Marten stream version, ADR-011 `IInitialData` migration seed. **First config-as-events instance in code** (ADR-011's third instance; Dispatch/Onboarding were design-only) and **first FluentValidation use**. Four `jasperfx-source-verifier` gates; the gRPC re-verification found the local JasperFx checkout stale at V5.37.2 (hand-wire verdict holds through 5.37.2, 6.17 unverified — a slice-2 concern). Local Docker wedged all session → slice-1 integration tests gated on CI. Kafka deliberately not wired into apphost (transport lands with slice 3). DEBT registered for the config-as-events seed skill (ADR-011's deferred follow-up). None of W006 §11's three candidates fired, but the Phase-2 audit's ADR-011 Option-A/B-for-Marten finding was resolved in-PR (user-directed mid-flight expansion) via an **ADR-011 amendment** — `IInitialData` as the canonical Marten Option-A realization + LWW for config singletons. Triggered by [`prompts/implementations/006-telemetry-skeleton-and-slice-1-config.md`](../prompts/implementations/006-telemetry-skeleton-and-slice-1-config.md). Status: complete (2026-07-10). +- [`007-telemetry-slices-4-and-2-transport.md`](./implementations/007-telemetry-slices-4-and-2-transport.md) — **CritterCab's first transport in code.** W006 slice 4 (`LastKnownPosition` plain-document store + heartbeat-absence eviction) and slice 2 (gRPC `ReportLocations` client-streaming ingest) in one PR under the coupled-slices reading of the cadence rule. **Five firsts:** first gRPC surface serving traffic, first client-streaming RPC, first proto codegen, first non-event-sourced document write path, first recurring/scheduled work. Retires the three-month client-streaming forward-constraint — WolverineFx.Grpc 6.21.0 auto-generates the shape, so the empty `[WolverineGrpcService]` stub works and nothing is hand-wired; both gRPC skills, which still described the obsolete workaround, were corrected in-session under the session-runner-blocking exception. **The Verify-before-wiring pass earned its cost by disconfirming, not confirming:** gate 5 overturned the prompt's own hypothesis (Wolverine has no recurring-message primitive — the idiom is a plain `BackgroundService` + `IServiceScopeFactory`, since `IMessageBus` is scoped and a hosted service is a singleton), and gate 8a overturned the H3 recipe (the `LatLng` tuple path is a radians trap; the NTS `Coordinate` path is degrees-native). Gate 11 dissolved — Alba's `TestServer` feeds a `GrpcChannel` directly, so the Alba-first default holds for gRPC with no parallel fixture. Two W006 under-specifications escalated as forks rather than silently defaulted: the `LastKnownPosition` field set (collapsing `lastPublishedAt` into `serverReceivedAt`, a simplification neither source document had noticed) and the `accuracyMeters` threshold (**100m — invented at implementation time; W006 names the threshold but fixes no value**). Both load-bearing tests were designed to fail a naive implementation (two-path H3 equality; the same document surviving one policy and swept under another), and the scoped/singleton DI guard was mutation-verified. Also fixed `apphost.cs`, broken since PR #42 — **the durable finding is that CI's existing solution-completeness guard cannot see a file-based app**. Six DEBT rows registered. Triggered by [`prompts/implementations/007-telemetry-slices-4-and-2-transport.md`](../prompts/implementations/007-telemetry-slices-4-and-2-transport.md). Status: complete (2026-07-24). ### Per-narrative retros diff --git a/docs/retrospectives/implementations/007-telemetry-slices-4-and-2-transport.md b/docs/retrospectives/implementations/007-telemetry-slices-4-and-2-transport.md new file mode 100644 index 0000000..4f28055 --- /dev/null +++ b/docs/retrospectives/implementations/007-telemetry-slices-4-and-2-transport.md @@ -0,0 +1,111 @@ +# Retrospective — Telemetry Slice 4 (`LastKnownPosition` + eviction) + Slice 2 (gRPC `ReportLocations` ingest) + +## Metadata + +- **Triggering prompt:** [`docs/prompts/implementations/007-telemetry-slices-4-and-2-transport.md`](../../prompts/implementations/007-telemetry-slices-4-and-2-transport.md) +- **Status:** Complete +- **Date authored:** 2026-07-24 +- **Output artifacts:** + - `Directory.Packages.props` — all 11 `WolverineFx*` entries 6.19.0 → **6.21.0 in lockstep**; new `Grpc.AspNetCore` / `Grpc.Tools` / `Google.Protobuf` (2.76.0 / 2.76.0 / 3.31.1) and `pocketken.H3` 4.5.0.1 + - `src/CritterCab.Telemetry/CritterCab.Telemetry.csproj` — first `` items in the repo; `report_locations.proto` (`GrpcServices="Both"`) + `driver_location_updated.proto` (`GrpcServices="None"`) + - `src/CritterCab.Telemetry/LastKnownPosition/LastKnownPosition.cs` — plain Marten document; first non-event-sourced write path + - `src/CritterCab.Telemetry/LastKnownPosition/EvictStalePositions.cs` — sweep command + handler (`HardDeleteWhere`, threshold from policy) + - `src/CritterCab.Telemetry/LastKnownPosition/LastKnownPositionEvictionService.cs` — logic-free `BackgroundService` timer + - `src/CritterCab.Telemetry/ReportLocations/TelemetryGrpcService.cs` — empty `[WolverineGrpcService]` stub + - `src/CritterCab.Telemetry/ReportLocations/ReportLocationsHandler.cs` — the ingest; per-ping pipeline + publish trigger + - `src/CritterCab.Telemetry/ReportLocations/H3CellIndexer.cs` — the H3 binding, wrapped for one reason + - `src/CritterCab.Telemetry/ReportLocations/{IDriverPrincipalAccessor,IDriverLocationPublisher}.cs` — two ready-to-swap seams + dev stubs + - `src/CritterCab.Telemetry/Program.cs` — `AddGrpc` / `AddWolverineGrpc` / `MapWolverineGrpcServices`, seam registrations, `AddHostedService` inside the Marten guard + - `tests/CritterCab.Telemetry.Tests/` — `Slice4LastKnownPositionTests` (6), `H3CellIndexerTests` (5), `Slice2ReportLocationsTests` (5); fixture gains `ConfigureTestServices`, `RecordingDriverLocationPublisher`, `CreateGrpcChannel` + - `apphost.cs` — **fixed a two-week-old compile break** (missing `#:project` for Telemetry); gRPC rides the existing HTTPS endpoint + - `protos/crittercab/telemetry/v1/report_locations.proto` — stale forward-constraint comment corrected + - `docs/skills/wolverine-grpc-handlers/SKILL.md` — 7 stale claims corrected + new **Client-streaming handlers** section (deliverable 13a) + - `docs/skills/wolverine-grpc-bidirectional-handlers/SKILL.md` — superseded banners + mental-model/frontmatter corrections only (13b) + - `docs/skills/DEBT.md` — 6 new rows + - `docs/workshops/006-telemetry-event-model.md` `## Document History` — slices 2 + 4 realized; forward-constraint closed + - `docs/prompts/README.md` — Implementations index entry + - This retro +- **Outcome:** Both slices implemented end-to-end. **31/31 green locally and in CI** (Telemetry 20, Dispatch 11), 0 warnings, first-try CI pass. Docker was working again this session, so Testcontainers ran locally rather than leaning on CI as at PR #42. PR [#45](https://github.com/erikshafer/CritterCab/pull/45). + +--- + +## Framing + +This is the session the whole project has been pointing at. gRPC, Kafka and ASB have been modeled across five workshops and sixteen ADRs and wired in **zero lines of code**; `protos/` has held authored contracts with nothing consuming them since PR #39. This session makes the first of them real. + +It was also gated for three months on a genuine library limitation — WolverineFx.Grpc could not auto-generate `stream in → unary out` — which every prior handoff carried forward as "hand-wire `ReportLocations` against `IMessageBus`." That constraint closed four days before the session ran. + +--- + +## Outcome summary + +Four commits, one per chunk plus the prompt. Chunk order was dependency-driven and held: version bump + codegen → slice 4 → slice 2 → docs. Slice 4 preceding slice 2 mattered — the trigger needs a real baseline to evaluate against, and building it the other way would have meant stubbing the document twice. + +**Five firsts in code:** first gRPC surface serving traffic, first client-streaming RPC, first proto codegen, first non-event-sourced document write path, first recurring/scheduled work. + +--- + +## What worked + +**The Verify-before-wiring ledger paid for itself, and it paid off most where it was wrong.** Eleven gates were source-verified before any code. Gate 5 is the case in point: the prompt's own authoring hypothesis — that Wolverine has a recurring-message primitive reachable via `IScheduledJobProcessor` or `PublishMessage().ToLocalQueue()` — was **wrong on both counts**, and the 2026-07-21 trace caught it and replaced it with the real idiom (a plain `BackgroundService`, template `HeartbeatBackgroundService`). Had that not been traced, the session would have spent its budget hunting an API that does not exist. A verification pass that only ever confirms is not doing its job; this one disconfirmed twice (gates 5 and 8a) and closed the rest. + +**Gate 11 dissolved rather than being worked around.** The worry was that Wolverine's client-streaming fixture builds a raw `WebApplication` + `UseTestServer()`, while CritterCab standardizes on Alba. Alba wraps `WebApplicationFactory`, which runs on `TestServer` underneath — so `Host.GetTestServer().CreateHandler()` feeds a `GrpcChannel` directly. No parallel host recipe, no new test packages, Alba-first default intact. The prompt's fallback ("a dedicated gRPC fixture, document the deviation") was never needed. + +**Two under-specifications were escalated rather than silently resolved.** W006 §3.3/§6.4 disagree with the prompt's deliverable 4 on the `LastKnownPosition` field set, and §6.2 names an `accuracyMeters` threshold while fixing no value. Both went to the user as explicit forks with recommendations. The first produced a genuinely better answer than either source document had: since §6.4 locks upsert-on-publish-only, `serverReceivedAt` and `lastPublishedAt` are the same instant **by construction**, so one field serves both the trigger baseline and the eviction key — a simplification neither the workshop nor the prompt had noticed. + +**The pinning tests were designed to fail, not to pass.** Both load-bearing tests in this session assert something a naive version would miss: + +- *H3.* "The cell is valid" passes with lat/lon swapped — a swapped ping indexes to a valid cell in the Indian Ocean. So the test computes the point down **both** API paths, which are mirror opposites on axis order *and* units, and asserts equality; two wrongs cannot cancel. A separate test proves the swapped input yields a *different* cell, which is what makes the first test meaningful. +- *Eviction threshold.* "A stale document is deleted" passes against a hardcoded 90 seconds. So the same 10-second-old document is asserted to **survive** under the seeded 30s heartbeat and be **swept** under a reconfigured 1s one. Only the pair distinguishes "reads policy" from "coincidentally matches the default." + +**The DI guard was mutation-verified.** `IMessageBus` is scoped, a `BackgroundService` is a singleton. The eviction shell takes `IServiceScopeFactory` for that reason, and rather than assert the guard works, the session temporarily injected the bus directly and confirmed `CallSiteValidator` fails host construction — then reverted. The comment claiming protection is now a claim that was tested. + +--- + +## What was harder than expected + +**`apphost.cs` had not compiled since PR #42.** The Telemetry service block was added without its `#:project` directive, so `Projects.CritterCab_Telemetry` never existed. It was found only because this session had to touch the file. + +The durable finding is not the one-line fix — it is **why it survived two weeks**. CI builds `CritterCab.slnx` and even has a "Verify solution completeness" step that fails when a `.csproj` on disk is missing from the solution. The apphost is a **file-based app with no `.csproj`**, so it falls through a guard that already exists and was written for exactly this class of mistake. The fix is to extend that step to build `apphost.cs`, not to invent a new check. Flagged as a next-session input rather than done here: CI changes are their own scope. + +**Nothing else in the session was structurally hard**, which is itself worth recording. The client-streaming shape — three months of forward-constraint, two skills' worth of workaround documentation, a standing entry in every handoff — took an empty one-line stub and a handler that imports nothing from `Grpc.*`. The cost was never in the implementation; it was in the library gap, and once that closed the design absorbed it without a ripple. + +--- + +### Design meets code — three things the workshop could not have known + +1. **Middleware does not weave for client-streaming**, so per-ping validation *must* live in the handler. W006 §6.2 lists validation as pipeline step 2 without saying where it runs; slice 1 established "validate at the HTTP boundary, the aggregate stays thin" as the CritterCab pattern. This slice cannot follow it — a before-frame needs a concrete request at method entry and a stream cannot supply one. The asymmetry is now recorded in the proto, the skill, and `Program.cs` beside the FluentValidation line it contrasts with, because a reader who sees only the handler will otherwise read it as drift. + +2. **`HardDeleteWhere` vs `DeleteWhere` is a domain decision wearing an API costume.** Marten's `DeleteWhere` is *conditional*: configure the document for soft-deletes later and it silently sets a flag instead of removing the row. §6.4's "Return" GWT requires an evicted driver to find **no** baseline so the trigger republishes immediately. Choosing the verb is choosing whether that GWT survives a future configuration change. + +3. **The `identity-acl` gRPC auth pattern does not apply to this shape.** That skill documents caller identity via `[WolverineBefore]` middleware reading `ServerCallContext` — which depends on the same single-request binding that client-streaming lacks. The seam this slice built (`IDriverPrincipalAccessor` over `IHttpContextAccessor`) was designed from the middleware constraint directly, not from the skill; the skill's silence on the exception is now a DEBT row. + +--- + +## Methodology refinements + +- **A prompt's own hypotheses need verifying, not just the library's API.** Gate 5's correction was to the *prompt*, and the prompt was authored by the same process that would have executed it. The verification pass earns its cost specifically by being able to contradict its author. +- **Escalate under-specification as a fork with a recommendation, not as a blocker or a silent default.** Three forks were raised this session; all three took the recommendation, and one produced a better model than either source doc. The cost of asking was one round-trip each. +- **A regression guard deserves a mutation check when it is guarding something invisible.** The scoped/singleton failure is a startup crash, not a test failure — nothing in the normal suite would have noticed the guard rotting. +- **Fix-in-passing needs a stated reason.** `apphost.cs` was repaired because a deliverable targeted a file that did not build; the no-opportunistic-edits rule held everywhere else, including the test-naming violation, which was deferred to DEBT precisely because fixing it slice-locally would have deepened the inconsistency. + +--- + +## Outstanding items / next-session inputs + +- **CI does not build `apphost.cs`.** Extend the existing "Verify solution completeness" step to cover file-based apps. This is the finding, not the fix that landed. +- **Six DEBT rows registered** (`docs/skills/DEBT.md`): the `wolverine-grpc-bidirectional-handlers` structural rewrite (its ~145-line hand-written-workaround body is bannered, not removed); test-class naming across all three Telemetry suites; no skill for plain-document write paths; no skill for recurring work; the `identity-acl` streaming exception; and the feature-folder/type-name collision now on its third occurrence. +- **Gate 9 was deliberately not exercised.** Whether cascading messages and `[Transactional]`/outbox middleware weave for a handler whose message type is `IAsyncEnumerable` remains unverified — the design sidesteps it by fanning out through injected dependencies called directly rather than through cascaded messages. Recorded here as prompt 007 gate 9 asked: if a future slice makes this handler emit cascading messages, that question becomes live. +- **`MaxAccuracyMeters = 100` is invented at implementation time.** W006 §6.2 names the threshold and fixes no value. It follows §6.4's precedent (documented constant, not a policy param, promote in v2) but it is a number this session chose, and W006's Document History says so. +- **Slice 3 (PR C)** swaps `LoggingDriverLocationPublisher` for the real Kafka producer. The seam already carries the generated `DriverLocationUpdated`, so only the implementation changes. + +--- + +## Spec delta — landed? + +**Yes, in full.** + +- **W006 §6.2 designed → realized.** The client-streaming ingest, per-ping pipeline and `shouldPublish` trigger are running code, exercised by real gRPC streams. +- **W006 §6.4 designed → realized.** `LastKnownPosition` and the eviction sweep are running code. +- **The client-streaming forward-constraint is closed, not carried.** Recorded in W006's Document History, corrected in the proto, and corrected in both gRPC skills. +- **W006 §11's *windowed gRPC client-streaming ingest* candidate fired as a skill, not an ADR** — as §6.2 and the handoff both leaned. The auto-generated shape plus its middleware caveat is library mechanics, not an architectural choice CritterCab made; it belongs in `wolverine-grpc-handlers`, where it now lives. The other two §11 candidates (Kafka topic-naming, stream-processing-as-4th-shape) remain later-arc, as the prompt named them. diff --git a/docs/skills/DEBT.md b/docs/skills/DEBT.md index 8e7c49b..e437fb0 100644 --- a/docs/skills/DEBT.md +++ b/docs/skills/DEBT.md @@ -34,6 +34,43 @@ This file is the working ledger between retros that surface gaps and the tidy se - **Gap:** No skill documents the **two-call** wiring that HTTP boundary validation actually requires. `csharp-coding-standards` § FluentValidation shows the nested `AbstractValidator` shape, but the boundary needs BOTH `opts.UseFluentValidation()` in `UseWolverine` (the `WolverineFx.FluentValidation` assembly-scan that *registers* `IValidator<>` into DI) **and** `opts.UseFluentValidationProblemDetailMiddleware()` in `MapWolverineEndpoints` (the `WolverineFx.Http.FluentValidation` middleware that *resolves* them into a 400 ProblemDetails). Wiring only the middleware silently passes invalid input through as 200 — a footgun CI caught on slice-1's first run (`src/CritterCab.Telemetry/Program.cs` is the repo's first FluentValidation instance and the reference wiring). A tidy session should add this to `wolverine-http-handlers` (both packages, both calls, the DI-resolution dependency between them). - **Retro source:** [`retrospectives/implementations/006-telemetry-skeleton-and-slice-1-config.md`](../retrospectives/implementations/006-telemetry-skeleton-and-slice-1-config.md) (§ "CI caught a bug local tooling structurally could not"). +### `wolverine-grpc-bidirectional-handlers` — structural rewrite (client-streaming section is obsolete) + +- **Gap:** The skill is structurally premised on WolverineFx.Grpc being unable to auto-generate client-streaming, and documents a hand-written `IMessageBus` workaround at length. **6.21.0 auto-generates the shape**, so that section describes a workaround for a problem that no longer exists — and CritterCab now ships code that contradicts it (`src/CritterCab.Telemetry/ReportLocations/{TelemetryGrpcService,ReportLocationsHandler}.cs`, passing tests). PR #45 applied only a **superseded banner** plus the frontmatter and mental-model-table one-liners, under the session-runner-blocking exception; the body was deliberately left intact rather than half-rewritten. The tidy session should retitle the skill to bidirectional-only (or "both shapes, both auto-generated"), delete the hand-written-workaround section, and rewrite the client-streaming pitfalls that assert the opposite of current behavior. +- **Why it was not fixed in-session:** the correction that *was* blocking (a session cannot follow a skill telling it to hand-wire) landed in `wolverine-grpc-handlers`, which is now the home for the auto-generated client-streaming pattern. The bidirectional skill's rewrite is structural, not a line fix, and belongs in its own scoped session. +- **Retro source:** [`retrospectives/implementations/007-telemetry-slices-4-and-2-transport.md`](../retrospectives/implementations/007-telemetry-slices-4-and-2-transport.md). + +### `testing-fundamentals` vs. shipped test-class naming (decide, then apply everywhere) + +- **Gap:** The skill mandates snake_case test class names. **All three** Telemetry test classes use PascalCase `Slice{N}Tests` — `Slice1TelemetryPolicyTests` (shipped in PR #42), plus `Slice4LastKnownPositionTests` and `Slice2ReportLocationsTests` (PR #45). The audit confirms the skill is unambiguous and names this exact failure mode ("mixing PascalCase and snake_case test names within one test project") in its own pitfalls. +- **This is a decision, not a cleanup.** Either the skill is right and all three rename, or the `Slice{N}` grouping is a deliberate CritterCab convention — test suites grouped by workshop slice rather than one class per handler, so a reader can map a test class to the slice in the event model — and the skill needs a carve-out. **Do not fix it slice-locally**: renaming only the newer two would deepen the inconsistency the pitfall warns about. Deferred from PR #45 by user sign-off, because renaming a file from a merged PR is exactly the opportunistic edit the scope rule forbids. +- **Retro source:** [`retrospectives/implementations/007-telemetry-slices-4-and-2-transport.md`](../retrospectives/implementations/007-telemetry-slices-4-and-2-transport.md). + +### Plain-document (non-event-sourced) write path — no governing skill + +- **Gap:** Nothing in `docs/skills/` covers writing to a Marten document that is **not** an aggregate or projection: `session.Store()` overwrite-in-place, bulk delete via `DeleteWhere` / `HardDeleteWhere`, and when last-writer-wins is the whole concurrency story rather than a gap in one. `marten-querying` is scoped to reads by its own charter; `marten-wolverine-aggregates` covers the event-sourced path and its "never call `SaveChangesAsync` yourself" rule is scoped to aggregate handlers, so it neither sanctions nor forbids the document-only shape. PR #45's gate work had to source these APIs directly from Marten's `IDocumentOperations.cs`. Reference impl: `src/CritterCab.Telemetry/LastKnownPosition/`. +- **One decision worth codifying:** prefer `HardDeleteWhere` over `DeleteWhere` when the domain requires the row to actually be gone. `DeleteWhere` is *conditional* — it silently becomes a soft-delete flag if the document type is ever configured for soft-deletes, which turns a correctness guarantee into a configuration coincidence. +- **Retro source:** [`retrospectives/implementations/007-telemetry-slices-4-and-2-transport.md`](../retrospectives/implementations/007-telemetry-slices-4-and-2-transport.md). + +### Recurring/periodic work — no governing skill (`wolverine-marten-automation` addendum or new skill) + +- **Gap:** No skill documents periodic work, and the shape is non-obvious in a Wolverine codebase because **Wolverine has no first-class recurring-message primitive** — a session's natural first guesses (`ScheduleAsync`, `PublishMessage().ToLocalQueue()`) are one-shot delayed delivery and routing configuration respectively, neither of them a scheduler. The idiom, used by Wolverine's own internals, is a plain .NET `BackgroundService` looping on `Task.Delay` that calls `IMessageBus` each tick. `wolverine-marten-automation` is the nearest skill but is explicitly event-triggered. +- **Two things a skill must state**, both of which bit or nearly bit this session: (1) split the timer from the work — a logic-free `BackgroundService` shell plus a normal handler holding everything testable, so the recurrence stays untested and the behavior stays covered; (2) the shell **must** take `IServiceScopeFactory` and open a scope per tick, because a `BackgroundService` is a singleton and Wolverine registers `IMessageBus` as scoped — injecting the bus directly fails host construction via `CallSiteValidator` (confirmed by mutation in PR #45). Reference impl: `src/CritterCab.Telemetry/LastKnownPosition/LastKnownPositionEvictionService.cs`. +- **Retro source:** [`retrospectives/implementations/007-telemetry-slices-4-and-2-transport.md`](../retrospectives/implementations/007-telemetry-slices-4-and-2-transport.md). + +### `identity-acl` — the gRPC auth pattern does not apply to client-streaming or bidi + +- **Gap:** `identity-acl/SKILL.md` (§ around L111–136) documents gRPC caller identity as a `[WolverineBefore]` middleware method taking `ServerCallContext` and reading `context.GetHttpContext().User`. That mechanism depends on `Before`/`Validate` frames binding against a concrete single request instance — which is **exactly what Wolverine cannot do for client-streaming or bidirectional RPCs**, where the method begins with a stream, not a message. So the documented pattern silently does not apply to those two shapes, and the skill states no such caveat. +- **CritterCab now has the counter-example in shipped code.** `ReportLocations` resolves identity through a plain DI seam reading `IHttpContextAccessor` (`src/CritterCab.Telemetry/ReportLocations/IDriverPrincipalAccessor.cs`) precisely because no `[WolverineBefore]` seam exists for its shape. The fix is an `identity-acl` addendum scoping the `ServerCallContext` pattern to unary/server-streaming and pointing streaming shapes at the accessor seam. +- **Registering, not fixing:** `identity-acl` is outside PR #45's scope, and the session was not blocked by it — the seam was designed from the middleware constraint directly (verified against `GrpcServiceChain.cs:270-272`), not from the skill. +- **Retro source:** [`retrospectives/implementations/007-telemetry-slices-4-and-2-transport.md`](../retrospectives/implementations/007-telemetry-slices-4-and-2-transport.md). + +### Feature-folder / type-name collision — unstated consequence of `vertical-slice-organization` + +- **Gap:** `vertical-slice-organization` mandates capability-named feature folders and a `{Type}.cs` file per type. Whenever a capability and its primary type share a name, that produces a namespace whose own name shadows the type — `CritterCab.Telemetry.TelemetryPolicy.TelemetryPolicy`, now `CritterCab.Telemetry.LastKnownPosition.LastKnownPosition`. Referring to the type from a *sibling* namespace then resolves to the namespace instead, and needs a `using X = global::…` alias or full qualification. This has now happened three times (the `TelemetryPolicy` view, the slice-1 test, and slice 4), each handled the same way by precedent rather than by any documented rule. +- **Why it is worth a line rather than tolerating:** the failure is a confusing compile error at a call site far from the cause, and a fourth occurrence written without the alias would hit it cold. A one-line addendum naming the pattern and the alias remedy — in `vertical-slice-organization` or `csharp-coding-standards` — is enough; no structural change is implied, since the collision is a *consequence* of a convention the repo wants. +- **Retro source:** [`retrospectives/implementations/007-telemetry-slices-4-and-2-transport.md`](../retrospectives/implementations/007-telemetry-slices-4-and-2-transport.md). + --- ## Recently drained diff --git a/docs/skills/wolverine-grpc-bidirectional-handlers/SKILL.md b/docs/skills/wolverine-grpc-bidirectional-handlers/SKILL.md index 23e5dfd..411ad39 100644 --- a/docs/skills/wolverine-grpc-bidirectional-handlers/SKILL.md +++ b/docs/skills/wolverine-grpc-bidirectional-handlers/SKILL.md @@ -1,17 +1,19 @@ --- name: wolverine-grpc-bidirectional-handlers -description: "Proto-first bidirectional and client-streaming gRPC handlers in CritterCab using Wolverine 5.32+. Covers the bidirectional handler shape (IAsyncEnumerable Handle(TRequest, [EnumeratorCancellation] CancellationToken) — invoked once per inbound request, same as server-streaming), why client-streaming wrappers are NOT auto-generated (NotSupportedException at chain construction with the canonical workaround spelled out), the two hand-written workaround patterns for proto-first client-streaming (separate-proto-service split vs. fully hand-written stub), the Validate / [WolverineBefore] / [WolverineAfter] asymmetry (not woven into bidi methods, not woven onto direct-mapped hand-written stubs), the AIP-193 exception interceptor that DOES still apply, and Cab's canonical use cases (PushTelemetry for client-streaming GPS-ping ingest; SubscribeTripUpdates for bidirectional driver-rider exchange). Use when authoring or modifying a service-side gRPC handler for client-streaming or bidirectional RPCs, or when diagnosing the startup throw that proto-first client-streaming triggers." +description: "Proto-first bidirectional gRPC handlers in CritterCab using WolverineFx.Grpc 6.21+. Covers the bidirectional handler shape (IAsyncEnumerable Handle(TRequest, [EnumeratorCancellation] CancellationToken) — invoked once per inbound request, same as server-streaming), the Validate / [WolverineBefore] / [WolverineAfter] asymmetry (not woven into bidi methods), the AIP-193 exception interceptor that DOES still apply, and Cab's canonical bidirectional use case (SubscribeTripUpdates for driver-rider exchange). PARTIALLY SUPERSEDED: this skill's client-streaming content predates 6.21.0 and describes a hand-written workaround that is now obsolete — client-streaming is auto-generated and documented in wolverine-grpc-handlers. Use when authoring or modifying a service-side gRPC handler for bidirectional RPCs." cluster: wolverine tags: [grpc, wolverine, client-streaming, bidirectional, hand-written-stub, push-telemetry, subscribe-trip-updates, async-stream-reader, async-stream-writer, message-bus, enumerator-cancellation] --- # Wolverine gRPC Bidirectional and Client-Streaming Handlers -CritterCab uses Wolverine 5.32+ for all four gRPC streaming modes per ADR-009 and `protobuf-contracts`. `wolverine-grpc-handlers` covers the two shapes Wolverine auto-generates wrappers for end-to-end (unary, server-streaming). This skill closes out the remaining two: **bidirectional**, which Wolverine also auto-generates but with subtly different middleware semantics, and **client-streaming**, which Wolverine 5.32 does NOT auto-generate at all and which Cab handles via a hand-written stub. +> **⚠ PARTIALLY SUPERSEDED as of WolverineFx.Grpc 6.21.0.** This skill was written when client-streaming could not be auto-generated. **It now is** — via the same empty `[WolverineGrpcService]` stub as every other shape — and the auto-generated pattern lives in **`wolverine-grpc-handlers`**, which is the authority for it. Everything here about **bidirectional** streaming remains correct. The client-streaming sections below document a hand-written workaround that is **obsolete**: do not follow them, and do not hand-wire client-streaming. Their structural rewrite is a registered `tidy: skills` DEBT row (see `docs/skills/DEBT.md`); this banner exists so the repo does not actively contradict shipped, passing code (`src/CritterCab.Telemetry/ReportLocations/`). + +CritterCab uses WolverineFx.Grpc 6.21+ for all four gRPC streaming modes per ADR-009 and `protobuf-contracts`. `wolverine-grpc-handlers` covers the three shapes with straightforward auto-generated wrappers (unary, server-streaming, client-streaming). This skill closes out the fourth: **bidirectional**, which Wolverine also auto-generates but with subtly different middleware semantics. The single most useful idea: **a bidirectional gRPC handler in Cab looks exactly like a server-streaming handler** — one `TRequest` parameter, returning `IAsyncEnumerable`, with `[EnumeratorCancellation]` on the cancellation token. The "bidi" part is in the wire shape, not the handler signature. Wolverine's generated wrapper loops over each inbound request from the client and dispatches each one through `bus.StreamAsync`, pumping every yielded response back to the client. This means **the handler is invoked once per inbound request, not once per stream** — a subtle but consequential semantics that the bidirectional integration tests in Wolverine's source pin down explicitly. -Client-streaming is the opposite story. The `[WolverineGrpcService]` discovery path actively rejects proto-first stubs that declare client-streaming RPCs — `GrpcServiceChain`'s constructor throws `NotSupportedException` at startup with a message naming the offending method and pointing at the workaround. The workaround is a hand-written concrete stub class deriving from the proto-generated base, which Cab dispatches to the message bus directly. This skill lays out both the auto-generated bidi shape and the hand-written client-streaming pattern, including which middleware surfaces still apply on each path. +Client-streaming *used to be* the opposite story: before 6.21.0 the `[WolverineGrpcService]` discovery path rejected proto-first stubs declaring client-streaming RPCs, throwing `NotSupportedException` at startup, and Cab worked around it with a hand-written concrete stub. **6.21.0 added the emit path, so that rejection no longer happens and the workaround is obsolete.** Client-streaming now uses the same empty stub as every other shape, with the handler taking `IAsyncEnumerable` and returning `Task` — documented in `wolverine-grpc-handlers`. This skill is now about the auto-generated **bidi** shape and its middleware asymmetries; its client-streaming sections are retained only as a legacy record pending rewrite. This skill assumes the proto-first bootstrap from `wolverine-grpc-handlers` (Kestrel HTTP/2, `AddGrpc()`, `AddWolverineGrpc()`, `MapWolverineGrpcServices()`) and the proto-naming conventions from `protobuf-contracts`. Both apply unchanged to bidirectional and client-streaming surfaces. @@ -22,10 +24,9 @@ This skill assumes the proto-first bootstrap from `wolverine-grpc-handlers` (Kes Use this skill when: - Authoring a bidirectional gRPC handler (e.g., `SubscribeTripUpdates` on the Trips service). -- Authoring a client-streaming gRPC handler (e.g., `PushTelemetry` on the Telemetry service for mobile-client GPS ingest). -- Diagnosing a `NotSupportedException` thrown at service startup mentioning "Client-streaming" and the offending RPC method name. +- Reading legacy code or docs that reference the pre-6.21.0 hand-written client-streaming workaround, and needing to know what replaced it. **Authoring** a client-streaming handler is `wolverine-grpc-handlers`, not this skill. - Deciding whether a flow that already lives in proto belongs as bidirectional or as client-streaming + server-streaming pair. -- Reviewing a PR that adds the hand-written workaround for a client-streaming RPC. +- Reviewing a PR that adds a hand-written client-streaming stub — to reject it. Since 6.21.0 the auto-generated path is correct and the workaround is obsolete. - Wiring middleware (`Validate`, `[WolverineBefore]`, `[WolverineAfter]`) and confirming the asymmetries between the auto-generated and hand-written paths. Do NOT use this skill for: @@ -41,16 +42,18 @@ Do NOT use this skill for: ## Mental model -Wolverine's gRPC integration recognizes four canonical RPC shapes via reflection over the proto-generated `*Base` class. The four shapes are classified by the `GrpcMethodKind` enum: `Unary`, `ServerStreaming`, `ClientStreaming`, `BidirectionalStreaming`. Three of these are wrapped automatically; one is rejected with a fail-fast at startup. +Wolverine's gRPC integration recognizes four canonical RPC shapes via reflection over the proto-generated `*Base` class. The four shapes are classified by the `GrpcMethodKind` enum: `Unary`, `ServerStreaming`, `ClientStreaming`, `BidirectionalStreaming`. **As of 6.21.0 all four are wrapped automatically.** -| Shape | Proto declaration | Wolverine 5.32 wrapping | Cab path | +| Shape | Proto declaration | Wolverine wrapping | Cab path | |---|---|---|---| | Unary | `rpc X(Req) returns (Resp);` | Auto-generated (`bus.InvokeAsync`) | `wolverine-grpc-handlers` | | Server-streaming | `rpc X(Req) returns (stream Resp);` | Auto-generated (`bus.StreamAsync`) | `wolverine-grpc-handlers` | | **Bidirectional** | `rpc X(stream Req) returns (stream Resp);` | **Auto-generated**, with middleware caveats | This skill | -| **Client-streaming** | `rpc X(stream Req) returns (Resp);` | **Rejected at startup** — hand-written workaround | This skill | +| Client-streaming | `rpc X(stream Req) returns (Resp);` | Auto-generated (`bus.StreamAsync`) — **since 6.21.0** | `wolverine-grpc-handlers` | + +Both bidirectional and client-streaming take an `IAsyncStreamReader` on the wire. The difference is what the server returns: bidirectional returns an `IServerStreamWriter` (a stream of responses), client-streaming returns a single `Task` (one summary response). That single-response shape is why client-streaming needed its own emit path rather than riding the item-streaming `IMessageBus.StreamAsync` — 6.21.0 added a `StreamAsync` overload that folds a whole inbound stream into one reply. Before that overload existed, the shape was rejected at startup rather than half-supported. -Both bidirectional and client-streaming take an `IAsyncStreamReader` on the wire. The difference is what the server returns: bidirectional returns an `IServerStreamWriter` (a stream of responses), client-streaming returns a single `Task` (one summary response). That single-response shape is what makes client-streaming hard to auto-wrap on top of `IMessageBus.StreamAsync`, which is item-streaming by construction. Wolverine's authors chose to fail fast rather than half-support it. +**Both shapes share one consequence that has not changed:** neither weaves `Validate` / `[WolverineBefore]` / `[WolverineAfter]`, because a before-frame needs a concrete request instance at method entry and a stream cannot supply one. Validation and cross-cutting concerns are handler-side for both. The discovery rules that drive each path are in `WolverineGrpcExtensions.IsCodeFirstGrpcServiceType` (matches name suffix `GrpcService` or the `[WolverineGrpcService]` attribute), `GrpcGraph.IsProtoFirstStub` (abstract + attribute + proto base), and `GrpcGraph.AssertNoConcreteProtoStubs` (rejects concrete `[WolverineGrpcService]` classes that derive from a proto base). The hand-written client-streaming workaround threads through these rules deliberately: a concrete class deriving from a proto base, **not marked `[WolverineGrpcService]`**, name ending in `GrpcService` so `MapWolverineGrpcServices()` discovers it for direct mapping. @@ -147,7 +150,14 @@ The AIP-193 exception interceptor (`WolverineGrpcExceptionInterceptor`) is regis --- -## Client-streaming handlers (hand-written workaround) +## Client-streaming handlers (hand-written workaround) — ⚠ OBSOLETE + +> **⚠ SUPERSEDED as of WolverineFx.Grpc 6.21.0 — do not follow this section.** +> +> Client-streaming is auto-generated. The empty `[WolverineGrpcService]` stub works for it exactly as for unary and server-streaming, and the handler shape is +> `Task Handle(IAsyncEnumerable, …)`. See **`wolverine-grpc-handlers` § Client-streaming handlers**, which is the authority. +> +> Everything below documents the pre-6.21.0 hand-written workaround and is retained only as a legacy record. Its removal and this skill's retitling to bidirectional-only are a registered `tidy: skills` DEBT row. CritterCab ships auto-generated client-streaming today (`src/CritterCab.Telemetry/ReportLocations/`), so following this section would contradict working code. Cab's canonical client-streaming case is `PushTelemetry` on the Telemetry service per `transport-selection`: a driver's mobile client streams GPS pings continuously into the Telemetry service, which acknowledges with a single `PushTelemetryResponse` summary when the client closes the stream. Client-streaming is the right shape for this — high-frequency, low-overhead inbound items where the response is incidental. The challenge is that Wolverine 5.32 doesn't generate the wrapper. diff --git a/docs/skills/wolverine-grpc-handlers/SKILL.md b/docs/skills/wolverine-grpc-handlers/SKILL.md index e772600..ffe47ca 100644 --- a/docs/skills/wolverine-grpc-handlers/SKILL.md +++ b/docs/skills/wolverine-grpc-handlers/SKILL.md @@ -1,6 +1,6 @@ --- name: wolverine-grpc-handlers -description: "Proto-first gRPC service handlers in CritterCab using Wolverine 5.32+. Covers the [WolverineGrpcService] stub pattern, unary handlers (Task bus.InvokeAsync), server-streaming handlers (IAsyncEnumerable bus.StreamAsync), the Validate / [WolverineBefore] / [WolverineAfter] middleware surface, AIP-193 exception → StatusCode mapping (default table plus opts.MapException() overrides), opt-in google.rpc.Status rich error details, the Kestrel HTTP/2 + AddGrpc + AddWolverineGrpc + MapWolverineGrpcServices bootstrap, client-side Grpc.Net.Client typed clients with Aspire service discovery, and the wolverine-diagnostics codegen-preview --grpc surface for inspecting generated wrappers. Use when authoring or modifying a service-side gRPC handler for unary or server-streaming RPCs. Client-streaming and bidirectional patterns live in wolverine-grpc-bidirectional-handlers (Phase 4)." +description: "Proto-first gRPC service handlers in CritterCab using WolverineFx.Grpc 6.21+. Covers the [WolverineGrpcService] stub pattern, unary handlers (Task bus.InvokeAsync), server-streaming handlers (IAsyncEnumerable bus.StreamAsync), client-streaming handlers (Task Handle(IAsyncEnumerable) via bus.StreamAsync) and why middleware does NOT weave for that shape, the Validate / [WolverineBefore] / [WolverineAfter] middleware surface, AIP-193 exception → StatusCode mapping (default table plus opts.MapException() overrides), opt-in google.rpc.Status rich error details, the Kestrel HTTP/2 + AddGrpc + AddWolverineGrpc + MapWolverineGrpcServices bootstrap, client-side Grpc.Net.Client typed clients with Aspire service discovery, and the wolverine-diagnostics codegen-preview --grpc surface for inspecting generated wrappers. Use when authoring or modifying a service-side gRPC handler for unary, server-streaming or client-streaming RPCs. Bidirectional patterns live in wolverine-grpc-bidirectional-handlers." cluster: wolverine tags: [grpc, wolverine, proto-first, streaming, server-streaming, unary, aip-193, exception-mapping, validate, wolverine-grpc-service, message-bus, http2] --- @@ -13,7 +13,9 @@ The single most useful idea in this skill: **a Cab gRPC handler is just a Wolver The wiring is what's gRPC-specific: a `[WolverineGrpcService]`-marked abstract stub that derives from the proto-generated base class, plus the bootstrap that turns it on. Wolverine code-generates the concrete subclass at startup, overrides every RPC method, and forwards each call to the message bus. Tooling (`wolverine-diagnostics codegen-preview --grpc`) shows exactly what was emitted. -This skill covers **unary** and **server-streaming** RPCs — the two shapes that account for nearly every Cab gRPC interaction (`RequestRide`, `StreamDriverOffers`, `WatchTripStatus`, `CompleteTrip`, `RequestQuote`). Client-streaming (`PushTelemetry` per `transport-selection`) and bidirectional patterns are deferred to `wolverine-grpc-bidirectional-handlers` (Phase 4) — and Wolverine 5.32 doesn't yet auto-generate client-streaming wrappers, so that skill also covers the hand-written workaround. +This skill covers **unary**, **server-streaming** and **client-streaming** RPCs — the three shapes that account for every Cab gRPC interaction shipped so far (`RequestRide`, `StreamDriverOffers`, `WatchTripStatus`, `CompleteTrip`, `RequestQuote`, `ReportLocations`). Bidirectional patterns are deferred to `wolverine-grpc-bidirectional-handlers`. + +> **Client-streaming is auto-generated as of WolverineFx.Grpc 6.21.0.** Earlier versions had no emit path for `stream in → unary out`, so a `[WolverineGrpcService]` stub declaring one failed fast at startup and the shape had to be hand-wired against `IMessageBus`. **That constraint is closed and the workaround is obsolete** — do not hand-wire client-streaming. Any document still describing it as unsupported is stale. The auto-generated path is documented below, alongside the one genuine asymmetry it carries: middleware does not weave for it. --- @@ -75,9 +77,9 @@ The handlers themselves are not gRPC-aware. They take the request type, return t Two alternative paths exist in Wolverine.Grpc: - **Code-first** with `[ServiceContract]` interfaces (via `protobuf-net.Grpc`) — Wolverine generates a concrete implementation of the interface that forwards to the bus. ADR-009 rejects this path: the contract is the `.proto` file, not the C# interface. -- **Hand-written** services not marked `[WolverineGrpcService]` — the user implements every RPC method directly, calling `IMessageBus` if they want. Used as the workaround for client-streaming RPCs that Wolverine 5.32 doesn't yet auto-generate (covered in `wolverine-grpc-bidirectional-handlers`). +- **Hand-written** services not marked `[WolverineGrpcService]` — the user implements every RPC method directly, calling `IMessageBus` if they want. A legacy/opt-out path, not a shape-specific workaround; it was historically how client-streaming had to be done before 6.21.0 added the emit path. -Cab uses proto-first auto-generation everywhere it can. Hand-written stubs only appear where Wolverine's code-gen doesn't reach yet (today: client-streaming). +Cab uses proto-first auto-generation everywhere. Since 6.21.0 that means **every** RPC shape except bidirectional streaming, so a hand-written stub in Cab code today is either legacy or a mistake — there is no shape left that requires one. --- @@ -175,7 +177,7 @@ public abstract partial class TripsGrpcService : Trips.TripsBase; Three things to know about this declaration: -- **`abstract`** is required. The stub doesn't implement the RPC methods — Wolverine's generated subclass does. Concrete stubs are valid only for the hand-written path (client-streaming workaround), which lives in `wolverine-grpc-bidirectional-handlers`. +- **`abstract`** is required. The stub doesn't implement the RPC methods — Wolverine's generated subclass does. Concrete stubs are valid only on the hand-written path, which since 6.21.0 no RPC shape requires (it was formerly the client-streaming workaround). - **`partial`** is optional but conventional in Cab. Adding `Validate` methods, `[WolverineBefore]` middleware, or other supporting code in a sibling `TripsGrpcService.Validate.cs` file keeps the discovery declaration crisp. - **Naming**: `GrpcService` matches the convention `MapWolverineGrpcServices` uses for discovery (name suffix `GrpcService`). The `[WolverineGrpcService]` attribute is required for proto-first stubs regardless — discovery checks for the suffix OR the attribute, but the attribute is what tells Wolverine "code-generate the wrapper" rather than "map this class directly." @@ -354,6 +356,83 @@ Same convention as unary: `StreamDriverOffersHandler` lives in `StreamDriverOffe --- +## Client-streaming handlers + +The mirror image of server-streaming: many requests in, one response out, returned when the client half-closes. **Auto-generated since WolverineFx.Grpc 6.21.0** — the stub is empty exactly as it is for unary and server-streaming. Reference implementation: Telemetry's `ReportLocations` ingest (W006 §6.2). + +### Proto declaration + +```proto +service TelemetryService { + rpc ReportLocations(stream LocationPing) returns (LocationIngestAck); +} +``` + +### Handler shape + +```csharp +// The stub is empty, like every other shape. +[WolverineGrpcService] +public abstract class TelemetryGrpcService : TelemetryService.TelemetryServiceBase; + +// The whole stream is the message. +public static class ReportLocationsHandler +{ + public static async Task Handle( + IAsyncEnumerable pings, + IDriverLocationPublisher publisher, // ordinary DI + CancellationToken ct) + { + var accepted = 0; + + await foreach (var ping in pings.WithCancellation(ct)) + { + // per-item work + accepted++; + } + + return new LocationIngestAck { AcceptedCount = accepted }; + } +} +``` + +The handler's message type is `IAsyncEnumerable`, not `TRequest`. It imports nothing from `Grpc.*` — the same protocol-agnostic property the other shapes have. + +### What's actually happening at runtime + +Two codegen layers stack, and understanding the split explains why the stub can be empty: + +1. **protoc** emits `TelemetryServiceBase` with a *virtual* `ReportLocations(IAsyncStreamReader, ServerCallContext)` — raw gRPC primitives. +2. **Wolverine** emits the concrete override, which adapts that reader into an `IAsyncEnumerable` via `WolverineGrpcStreamAdapters.ReadAllAsync(requestStream, ct)` and forwards: + +```csharp +return await _bus.StreamAsync( + WolverineGrpcStreamAdapters.ReadAllAsync(requestStream, ct), ct); +``` + +The adapter carries `[EnumeratorCancellation]` and is passed `context.CancellationToken`, so a client half-close or disconnect surfaces as normal cancellation inside the handler's `await foreach`. + +### ⚠ Middleware does NOT weave for client-streaming + +`Validate`, `[WolverineBefore]` and `[WolverineAfter]` **do not run** for a client-streaming RPC. This is structural, not an oversight: a before-frame needs a concrete `TRequest` instance in scope at method entry, and a stream cannot provide one. The generated wrapper skips the middleware frames entirely. + +**The failure mode is silence, not an error.** Wolverine does not warn that your `Validate` will never run — you simply get a validation gate that isn't there. So for this shape: + +- Per-item validation lives **inside** the handler. Decide explicitly whether an invalid item is dropped, counted, or terminates the stream; there is no framework default. +- Cross-cutting concerns you would normally weave (auth checks, tenant resolution) must be handler-side too. + +This is a real asymmetry with the rest of CritterCab. Slice 1's `ConfigureTelemetryPolicy` validates at the HTTP boundary with FluentValidation and the aggregate stays thin; `ReportLocations` cannot, and inlines the checks instead. That is compliant, not drift. + +### Caller identity + +Because there is no `[WolverineBefore]` seam, the `ServerCallContext` → `GetHttpContext().User` pattern in `identity-acl` **does not apply to this shape**. Resolve identity through an ordinary DI seam reading `IHttpContextAccessor` instead — gRPC call metadata travels as HTTP/2 headers, so it is reachable there. CritterCab's `IDriverPrincipalAccessor` is the reference. + +### Known limitation: request type is the handler key + +Wolverine keys the handler slot on `typeof(IAsyncEnumerable)`, not the RPC name. **Two client-streaming RPCs that stream the same request type map to the same handler.** The disambiguator is the request type, so give each client-streaming RPC its own message type even when the shapes look alike. Moot for CritterCab today (one client-streaming RPC, one `LocationPing`), but it will bite silently rather than loudly. + +--- + ## Validate short-circuit Wolverine recognizes a `Validate` or `ValidateAsync` method on the stub class that returns `Status?` (nullable `Grpc.Core.Status`). When non-null, the generated wrapper throws `RpcException(status)` before dispatching to the handler. @@ -654,7 +733,8 @@ For client-side inspection during development, `grpcurl` and `Evans` are the sta - **Plaintext HTTP/2 in production-like environments.** Wolverine's sample uses unencrypted HTTP/2 for simplicity. Cab uses HTTPS in dev (Aspire dev cert) and TLS in production. The `AppContext.SetSwitch("System.Net.Http.SocketsHttpHandler.Http2UnencryptedSupport", true)` flag the sample uses is appropriate only for sample code, never for Cab. - **Not handling `OperationCanceledException` distinctly.** When a streaming handler is cancelled (client disconnect), it throws `OperationCanceledException`, which the AIP-193 table maps to `Cancelled`. This is correct — but if the handler catches all exceptions (`catch (Exception)` instead of letting it propagate), the cancellation becomes invisible and the framework can't terminate the stream cleanly. - **Confusing the gRPC service stub with a Wolverine handler.** They're different things in different roles. The stub (`TripsGrpcService`) is empty and exists for the wrapper code-gen; the handler (`StartTripHandler.Handle`) does the actual work. Adding handler logic to the stub is wrong on two counts: the stub is abstract (so the code wouldn't run anyway), and the gRPC-ness of the request type doesn't change where the handler should live. -- **Trying to use `[WolverineGrpcService]` for a client-streaming RPC.** Wolverine 5.32 doesn't auto-generate wrappers for client-streaming methods — startup fails fast with a clear error. Use the hand-written workaround from `wolverine-grpc-bidirectional-handlers`. +- **Hand-wiring a client-streaming RPC against `IMessageBus`.** This was mandatory before WolverineFx.Grpc 6.21.0 and is now obsolete — `[WolverineGrpcService]` auto-generates the shape. A hand-written client-streaming stub in new code is a stale-doc artifact, not a requirement. +- **Expecting `Validate` or `[WolverineBefore]` to run on a client-streaming RPC.** They do not weave, and the failure is silent — the middleware simply never runs, so a validation gate you believe is protecting the handler is not there. Put the checks inside the handler. See the client-streaming section above. --- @@ -678,7 +758,7 @@ For client-side inspection during development, `grpcurl` and `Evans` are the sta **Downstream:** -- `wolverine-grpc-bidirectional-handlers` (Phase 4) — client-streaming and bidirectional patterns, including the hand-written workaround for client-streaming since Wolverine 5.32 doesn't auto-generate that shape. +- `wolverine-grpc-bidirectional-handlers` — bidirectional streaming. **Note:** that skill still describes client-streaming as requiring a hand-written workaround; it is stale on that point and carries a superseded banner. This skill is the home for the auto-generated client-streaming pattern. - `cli-grpc-tooling` (Phase 3) — buf, grpcurl, Evans CLI invocations. - `testing-integration` — fixture pattern for integration-testing a service with gRPC endpoints. - `testing-advanced` (Phase 4) — gRPC-specific scenario assembly, in-process gRPC clients via `WebApplicationFactory`, streaming-test patterns. @@ -686,7 +766,7 @@ For client-side inspection during development, `grpcurl` and `Evans` are the sta **External:** -- [Wolverine gRPC documentation](https://wolverinefx.net/guide/grpc/) — Wolverine 5.32+ gRPC integration guide. +- [Wolverine gRPC documentation](https://wolverinefx.net/guide/grpc/) — WolverineFx.Grpc integration guide. CritterCab's floor is **6.21.0**, the release that added client-streaming auto-codegen. - [Google AIP-193 — Errors](https://google.aip.dev/193) — the canonical exception → gRPC status code mapping table this skill follows. - [`google.rpc.Status`](https://github.com/googleapis/googleapis/blob/master/google/rpc/status.proto) — the rich-error-details message type used by `UseGrpcRichErrorDetails()`. - [ASP.NET Core gRPC services documentation](https://learn.microsoft.com/aspnet/core/grpc/) — Kestrel HTTP/2 configuration, `AddGrpc()`, `MapGrpcService()`. diff --git a/docs/workshops/006-telemetry-event-model.md b/docs/workshops/006-telemetry-event-model.md index f5f6abb..8caf1ef 100644 --- a/docs/workshops/006-telemetry-event-model.md +++ b/docs/workshops/006-telemetry-event-model.md @@ -638,4 +638,15 @@ Resume the paused W006 design (grill R1–R8, 2026-06-25) after signing off ADR- - **v0.1** (2026-06-30): Full workshop authored in one session, resuming the 2026-06-25 grill-with-docs pause (R1–R8) after ADR-018 sign-off. Scope, stream-processing shape sidebar (§3, CritterCab's fourth modeling shape), UL, event list, five slices walked with per-slice sign-off, cross-reference tables, three ADR candidates, parking lot, retrospective. Cross-workshop amendments (W001 §5.3, context-map edges #5/#6, vision-doc override) land in the same PR. - **2026-07-10** — §6.1 (Slice 1) **realized in code**. The `CritterCab.Telemetry` service skeleton and the `TelemetryPolicyConfigured` config-as-events singleton landed: `ConfigureTelemetryPolicy` command with boundary FluentValidation, the `TelemetryPolicy` self-aggregating live-stream view carrying `throttlePolicyVersion` (typed `long` from the Marten stream version), and the config-as-events bootstrap seed via Marten's `IInitialData` idempotent seam (defaults `h3Resolution: 9`, `heartbeatIntervalSeconds: 30`, `minPublishIntervalSeconds: 5`; `operatorId = "system-bootstrap"`). The seed realizes §6.1's "ADR-011 Option A (migration-time seed)" via Marten's `IInitialData`; the A/B-for-Marten reconciliation the slice-1 audit surfaced is **resolved in the same PR** by the [ADR-011 Amendment (2026-07-10)](../decisions/011-configuration-as-events-bootstrap.md#amendment--2026-07-10-marten-realization-via-iinitialdata) — `IInitialData` is the canonical Marten Option-A realization, and config singletons use last-writer-wins (no optimistic concurrency). Three Alba GWTs (bootstrap / reconfigure / reject) cover the slice. **First config-as-events instance realized in code** in the repo (ADR-011's third instance overall, after Dispatch and Onboarding — both design-only) and **first FluentValidation use in code**. The Telemetry BC is CritterCab's second service. Transport slices (2 gRPC ingest, 3 Kafka publish, 4 `LastKnownPosition`, 5 Dispatch consumer) remain pending; none of §11's three ADR candidates fired this session (all later-arc). Session: [`prompts/implementations/006-telemetry-skeleton-and-slice-1-config.md`](../prompts/implementations/006-telemetry-skeleton-and-slice-1-config.md). -- **2026-07-04** — §10 candidate protobuf surface **realized**. The Telemetry `v1` contracts were authored under `protos/crittercab/telemetry/v1/`: `TelemetryService.ReportLocations(stream LocationPing) → LocationIngestAck` (client-streaming ingest, §6.2) and `DriverLocationUpdated` (Kafka event, §6.3). The ubiquitous-language message names `LocationPing`/`LocationIngestAck` (§4) were preserved via a file-scoped `buf.yaml` lint exception rather than renamed to buf-`STANDARD` `...Request`/`...Response`. Implementation forward-constraint recorded (not a design change): WolverineFx.Grpc has no client-streaming auto-codegen adapter on the verified 5.x line, so §6.2's "first gRPC client-streaming surface" may need hand-wiring against `IMessageBus` (re-verify against 6.8 when implementation begins). Session: [`prompts/decisions/003-protobuf-telemetry-v1.md`](../prompts/decisions/003-protobuf-telemetry-v1.md). +- **2026-07-04** — §10 candidate protobuf surface **realized**. The Telemetry `v1` contracts were authored under `protos/crittercab/telemetry/v1/`: `TelemetryService.ReportLocations(stream LocationPing) → LocationIngestAck` (client-streaming ingest, §6.2) and `DriverLocationUpdated` (Kafka event, §6.3). The ubiquitous-language message names `LocationPing`/`LocationIngestAck` (§4) were preserved via a file-scoped `buf.yaml` lint exception rather than renamed to buf-`STANDARD` `...Request`/`...Response`. Implementation forward-constraint recorded (not a design change): WolverineFx.Grpc has no client-streaming auto-codegen adapter on the verified 5.x line, so §6.2's "first gRPC client-streaming surface" may need hand-wiring against `IMessageBus` (re-verify against 6.8 when implementation begins). **This forward-constraint is now CLOSED — see the 2026-07-24 entry.** Session: [`prompts/decisions/003-protobuf-telemetry-v1.md`](../prompts/decisions/003-protobuf-telemetry-v1.md). +- **2026-07-24** — §6.4 (Slice 4) and §6.2 (Slice 2) **realized in code**, and the client-streaming forward-constraint **closed**. + + **The forward-constraint is retired, not worked around.** WolverineFx.Grpc `6.21.0` added client-streaming auto-codegen for both proto-first and code-first shapes. `ReportLocations` — `stream LocationPing → single LocationIngestAck` — is auto-generated like the unary and server-streaming RPCs, so the hand-wire-against-`IMessageBus` workaround recorded in the 2026-07-04 entry and in every prior handoff is **obsolete and was not used**. An empty `[WolverineGrpcService]` stub deriving the generated `TelemetryServiceBase` is the whole gRPC surface; the handler receives the stream as `IAsyncEnumerable` and never references a gRPC type. The stale comment in `report_locations.proto` was corrected in the same PR. + + **§6.4** — `LastKnownPosition` is a plain Marten document (CritterCab's first non-event-sourced write path), overwrite-in-place, LWW on `serverReceivedAt`, no aggregate boundary. It ships with a **single timestamp**: this workshop's §3.3 sketch and §6.4 prose refer to both `serverReceivedAt` and `lastPublishedAt`, but §6.4's upsert-on-publish-only rule makes them the same instant by construction, so one field serves both the §6.2 trigger baseline and the eviction key. §3.3's `deviceTimestamp` / `speed` / `heading` were omitted as an explicit deferral — §6.3 passes speed and heading to Kafka straight from the ping, so the document never reads them. Field is `lon`, not §3.3's `lng`: the proto is the contract (ADR-009). Eviction is **split** — Wolverine has no first-class recurring-message primitive, so the timer is a plain `BackgroundService` holding no logic and `EvictStalePositions` is a normal handler holding all of it. It uses Marten's `HardDeleteWhere`, not `DeleteWhere`, because the latter silently degrades to a soft-delete flag if the document is ever configured for one, which would break the "Return" GWT. + + **§6.2** — the per-ping pipeline lives **inside the handler**, which is forced rather than stylistic: Wolverine cannot weave `Before`/`Validate` middleware for a client-streaming RPC, because a before-frame needs a concrete request instance at method entry and a stream cannot supply one. This is a genuine asymmetry with §6.1, which validates at the HTTP boundary with FluentValidation, and it is now recorded in the proto and in `wolverine-grpc-handlers`. **One §6.2 under-specification surfaced:** step 2 names an `accuracyMeters` threshold but fixes no value, and it is not a `TelemetryPolicy` parameter. The session chose a documented constant of **100m**, following §6.4's own precedent for the `3 × heartbeatIntervalSeconds` constant — **that number is invented at implementation time, not specified by this workshop.** Promote it to a policy param in v2 if ops needs to tune it. Both fan-out targets are ready-to-swap seams: `IDriverPrincipalAccessor` (R5 — identity from the principal, never the payload) and `IDriverLocationPublisher`, which carries the generated `DriverLocationUpdated` so slice 3 swaps only the implementation. + + **§11 ADR candidates:** none fired. The *windowed gRPC client-streaming ingest* candidate lands as a **skill**, not an ADR, as §6.2 and the handoff both leaned — the auto-generated shape plus its middleware caveat is library mechanics, not an architectural choice CritterCab made. It is folded into `wolverine-grpc-handlers` in the same PR. The other two (Kafka topic-naming, stream-processing-as-4th-shape) remain later-arc. + + Slices 3 and 5 remain pending. Session: [`prompts/implementations/007-telemetry-slices-4-and-2-transport.md`](../prompts/implementations/007-telemetry-slices-4-and-2-transport.md). diff --git a/protos/crittercab/telemetry/v1/report_locations.proto b/protos/crittercab/telemetry/v1/report_locations.proto index 3b4fdf8..c43fd0f 100644 --- a/protos/crittercab/telemetry/v1/report_locations.proto +++ b/protos/crittercab/telemetry/v1/report_locations.proto @@ -18,12 +18,19 @@ option csharp_namespace = "CritterCab.Telemetry.V1"; // the two RPC_*_STANDARD_NAME lint rules are excepted for this file only in // buf.yaml. See docs/prompts/decisions/003-protobuf-telemetry-v1.md. // -// Implementation forward-constraint: WolverineFx.Grpc has no client-streaming -// auto-codegen adapter on the verified 5.x line — a [WolverineGrpcService]-marked -// stub declaring this RPC fails fast at startup. If 6.8 still lacks it, hand-wire -// ReportLocations against IMessageBus (the documented workaround). Re-verify against -// 6.8 source when implementation begins. The .proto itself is plain proto3 — Wolverine -// imposes no proto-side shape. +// Implementation note: WolverineFx.Grpc auto-generates this shape as of 6.21.0. An empty +// [WolverineGrpcService] stub deriving the generated TelemetryServiceBase gets a generated +// override forwarding to IMessageBus.StreamAsync, and the handler receives the stream as +// IAsyncEnumerable. The forward-constraint this comment previously carried — +// "no client-streaming adapter, hand-wire against IMessageBus" — held only through the 5.x +// line and is CLOSED; do not hand-wire this RPC. Realized in code by W006 slice 2. +// +// One consequence worth knowing at the contract level: Wolverine cannot weave Before/Validate +// middleware for a client-streaming RPC, because a before-frame needs a concrete request +// instance at method entry and a stream cannot supply one. Per-ping validation is therefore a +// handler concern, not a boundary one. +// +// The .proto itself is plain proto3 — Wolverine imposes no proto-side shape. service TelemetryService { rpc ReportLocations(stream LocationPing) returns (LocationIngestAck); } diff --git a/src/CritterCab.Telemetry/CritterCab.Telemetry.csproj b/src/CritterCab.Telemetry/CritterCab.Telemetry.csproj index fb28e90..2316cda 100644 --- a/src/CritterCab.Telemetry/CritterCab.Telemetry.csproj +++ b/src/CritterCab.Telemetry/CritterCab.Telemetry.csproj @@ -7,9 +7,49 @@ + + + + + + + + + + + + + + + + + + diff --git a/src/CritterCab.Telemetry/LastKnownPosition/EvictStalePositions.cs b/src/CritterCab.Telemetry/LastKnownPosition/EvictStalePositions.cs new file mode 100644 index 0000000..0723f62 --- /dev/null +++ b/src/CritterCab.Telemetry/LastKnownPosition/EvictStalePositions.cs @@ -0,0 +1,51 @@ +using CritterCab.Telemetry.TelemetryPolicy; +using Marten; +using TelemetryPolicyView = global::CritterCab.Telemetry.TelemetryPolicy.TelemetryPolicy; + +namespace CritterCab.Telemetry.LastKnownPosition; + +// The eviction tick (W006 §6.4). Parameterless by design: the sweep's only inputs are the clock +// and the policy, both resolved handler-side. Raised by LastKnownPositionEvictionService on a +// timer rather than by a Marten stream, so this is a plain *Handler, NOT a *Automation (the +// wolverine-marten-automation shape is for event-triggered work). +public sealed record EvictStalePositions; + +public static class EvictStalePositionsHandler +{ + // "Heartbeat absence" means 3 consecutive missed heartbeats (§6.4). A documented constant + // rather than a fourth TelemetryPolicy parameter in v1: it keeps slice 1's policy shape stable + // and ties eviction semantically to the heartbeat — eviction IS heartbeat absence. Promote it + // to a policy param in v2 only if ops needs to tune it independently. + public const int MissedHeartbeatsBeforeEviction = 3; + + public static async Task Handle( + EvictStalePositions command, + IDocumentSession session, + TimeProvider time, + CancellationToken ct) + { + var policy = await session.Events.AggregateStreamAsync( + TelemetryPolicyStream.Id, token: ct); + + // No policy means the ADR-011 bootstrap seed has not run yet. Sweeping against a guessed + // threshold could evict live drivers, so skip this tick and let the next one pick it up. + if (policy is null) + return; + + var threshold = time.GetUtcNow() + .AddSeconds(-MissedHeartbeatsBeforeEviction * policy.HeartbeatIntervalSeconds); + + // HardDeleteWhere, not DeleteWhere. DeleteWhere is conditional: if LastKnownPosition were + // ever configured for soft-deletes it would silently switch to setting mt_deleted and the + // row would survive — breaking §6.4's "Return" GWT, which requires an evicted driver to + // find NO baseline and republish immediately. HardDeleteWhere makes "the row is gone" + // immune to a future soft-delete configuration change. + session.HardDeleteWhere(x => x.ServerReceivedAt < threshold); + await session.SaveChangesAsync(ct); + + // No staleness event is published (v1 — R3/R8). Eviction is Telemetry's own storage + // hygiene; propagating staleness to Dispatch is the v2 staleness-ceiling. A driver who + // genuinely went off-shift also emits a Driver Profile availability transition, and + // Dispatch's availability filter is the v1 net. + } +} diff --git a/src/CritterCab.Telemetry/LastKnownPosition/LastKnownPosition.cs b/src/CritterCab.Telemetry/LastKnownPosition/LastKnownPosition.cs new file mode 100644 index 0000000..dc7844b --- /dev/null +++ b/src/CritterCab.Telemetry/LastKnownPosition/LastKnownPosition.cs @@ -0,0 +1,41 @@ +namespace CritterCab.Telemetry.LastKnownPosition; + +// Telemetry's location-of-record (W006 §3.3): a plain Marten document, overwrite-in-place. +// NOT an event stream and NOT a projection — this stream-processing BC event-sources only the +// TelemetryPolicy config singleton (§3.4), which makes this CritterCab's first non-event-sourced +// write path. Nothing here is source-generated, so no `partial` is required. +// +// The document defends no invariant, so there is no aggregate boundary and no optimistic +// concurrency: last-writer-wins on ServerReceivedAt is the entire concurrency story (§3.3). A +// driver's own pings are the sole writer for that driver's document. +// +// A record rather than §3.3's mutable class sketch: every write replaces the whole row via +// session.Store(), so nothing is ever mutated in place and immutability costs nothing. +public sealed record LastKnownPosition +{ + // driverId — resolved from the authenticated principal, NEVER the ping payload (R5). Marten + // takes the `Id` property as document identity by convention, so "one document per driver" + // falls out of the shape. Not minted here, hence no Guid.CreateVersion7(): the value is an + // external key that arrives with the request. + public required Guid Id { get; init; } + + public required double Lat { get; init; } + + // `Lon`, not §3.3's `Lng` — report_locations.proto is the contract and it says `lon` (ADR-009). + public required double Lon { get; init; } + + // The H3 index at the policy's H3Resolution (R6) — published language shared with Dispatch + // (ADR-018). Kept in string form so the cell travels to Kafka unchanged in slice 3. + public required string H3Cell { get; init; } + + // Server-stamped receipt time of the ping that caused the last publish. ONE field serving TWO + // roles, deliberately: + // - §6.2's `lastPublishedAt` trigger baseline (heartbeatDue / throttleFloorElapsed), and + // - §6.4's eviction key (swept once older than 3 x heartbeatIntervalSeconds). + // They are the same instant by construction, because §6.4 locks upsert-on-publish-only: the + // document is written only when a publish happens, so its stamp IS the moment of last publish. + // A separate LastPublishedAt would hold a duplicate value in v1 and force eviction and the + // trigger to read different names for one instant. Omitted deliberately — revisit only if + // writes ever become per-ping, where the two would genuinely diverge. + public required DateTimeOffset ServerReceivedAt { get; init; } +} diff --git a/src/CritterCab.Telemetry/LastKnownPosition/LastKnownPositionEvictionService.cs b/src/CritterCab.Telemetry/LastKnownPosition/LastKnownPositionEvictionService.cs new file mode 100644 index 0000000..95faa7f --- /dev/null +++ b/src/CritterCab.Telemetry/LastKnownPosition/LastKnownPositionEvictionService.cs @@ -0,0 +1,64 @@ +using Wolverine; + +namespace CritterCab.Telemetry.LastKnownPosition; + +// The timer half of the §6.4 eviction sweep, deliberately holding no logic. +// +// Wolverine has NO first-class recurring/scheduled-message primitive: ScheduleAsync is one-shot +// delayed delivery, and PublishMessage().ToLocalQueue() is routing configuration, not a +// scheduler. The idiom — used by Wolverine's own internals, see +// Wolverine/Runtime/Heartbeat/HeartbeatBackgroundService.cs — is a plain .NET BackgroundService +// looping on Task.Delay and calling IMessageBus each tick. Wolverine contributes only the +// IMessageBus call INSIDE the loop; the recurrence itself is vanilla hosting. +// +// All testable behavior lives in EvictStalePositionsHandler. This shell is intentionally not +// covered by tests — there is nothing here to assert that would not be asserting Task.Delay. +public sealed class LastKnownPositionEvictionService( + IServiceScopeFactory scopeFactory, + ILogger logger) + : BackgroundService +{ + // How often to sweep — NOT how stale a document must be to be swept. That threshold is + // 3 x heartbeatIntervalSeconds and is read from the policy handler-side. Sweeping at roughly + // the default heartbeat cadence bounds how long an already-stale document lingers to about + // one extra heartbeat. A constant rather than configuration: v1 has no reason to tune it. + public static readonly TimeSpan SweepInterval = TimeSpan.FromSeconds(30); + + protected override async Task ExecuteAsync(CancellationToken stoppingToken) + { + while (!stoppingToken.IsCancellationRequested) + { + try + { + // Delay first: nothing can be stale at host start, so there is no reason to sweep + // before the first interval elapses. + await Task.Delay(SweepInterval, stoppingToken); + + // IMessageBus is registered scoped (Wolverine HostBuilderExtensions.cs:232), and + // this BackgroundService is a singleton — so the bus (and the IDocumentSession the + // handler resolves) must come from a scope created per tick, never from the root + // provider. Injecting IMessageBus directly here would fail DI scope validation at + // startup. + await using var scope = scopeFactory.CreateAsyncScope(); + var bus = scope.ServiceProvider.GetRequiredService(); + + // InvokeAsync, not PublishAsync: inline and awaited, so a slow sweep applies + // back-pressure to its own timer instead of overlapping the next tick. (The + // wolverine-messaging-handlers caution against InvokeAsync is about calling it + // from inside another handler; this is the carved-out in-process case.) + await bus.InvokeAsync(new EvictStalePositions(), stoppingToken); + } + catch (OperationCanceledException) when (stoppingToken.IsCancellationRequested) + { + return; // normal shutdown, not a failure + } + catch (Exception e) + { + // A failed sweep must never crash the host — the next tick retries, and a missed + // sweep only means stale documents linger one interval longer. Same guard + // Wolverine's own HeartbeatBackgroundService applies. + logger.LogError(e, "LastKnownPosition eviction sweep failed; retrying next tick."); + } + } + } +} diff --git a/src/CritterCab.Telemetry/Program.cs b/src/CritterCab.Telemetry/Program.cs index 323e44f..617d60c 100644 --- a/src/CritterCab.Telemetry/Program.cs +++ b/src/CritterCab.Telemetry/Program.cs @@ -1,5 +1,8 @@ +using CritterCab.Telemetry.LastKnownPosition; +using CritterCab.Telemetry.ReportLocations; using CritterCab.Telemetry.TelemetryPolicy; using JasperFx; +using Wolverine.Grpc; using Marten; using Wolverine; using Wolverine.FluentValidation; @@ -39,12 +42,33 @@ // of ADR-011 Option A (see the ADR's 2026-07-10 Amendment). Runs at the deploy-time apply // step and idempotently at host start. See TelemetryPolicyBootstrap. .InitializeWith(); + + // The W006 §6.4 heartbeat-absence eviction sweep. Registered inside the Marten guard on + // purpose: the sweep handler resolves an IDocumentSession, so without a store there is + // nothing for it to sweep and it would only log failures every interval. Wolverine has no + // recurring-message primitive, so the timer is a plain BackgroundService — see + // LastKnownPositionEvictionService for why, and for why it holds no logic. + builder.Services.AddHostedService(); } builder.Services.AddSingleton(TimeProvider.System); builder.Services.AddHealthChecks(); builder.Services.AddWolverineHttp(); +// gRPC ingest (W006 §6.2). AddWolverineGrpc registers the codegen that turns the abstract +// [WolverineGrpcService] stub into a concrete service forwarding to Wolverine handlers; +// MapWolverineGrpcServices (below) discovers and maps it. +builder.Services.AddGrpc(); +builder.Services.AddWolverineGrpc(); + +// The ingest resolves driverId from the ambient request rather than the payload (R5), so it needs +// the accessor. Both registrations below are ready-to-swap seams, not final implementations: +// HeaderDriverPrincipalAccessor gives way to a real Entra claim once Identity exists, and +// LoggingDriverLocationPublisher to the WolverineFx.Kafka producer in PR C. +builder.Services.AddHttpContextAccessor(); +builder.Services.AddSingleton(); +builder.Services.AddSingleton(); + // Enum names on the wire; Wolverine HTTP shares the Minimal-API JsonOptions this configures. builder.Services.ConfigureSystemTextJsonForWolverineOrMinimalApi(options => options.SerializerOptions.Converters.Add( @@ -69,6 +93,13 @@ // short-circuits with an RFC-7807 ProblemDetails 400 before the endpoint handler runs. app.MapWolverineEndpoints(opts => opts.UseFluentValidationProblemDetailMiddleware()); +// Discovers the abstract [WolverineGrpcService] stubs and maps the generated concrete services. +// Note the asymmetry with the line above: FluentValidation middleware weaves for HTTP endpoints, +// but Wolverine cannot weave Before/Validate frames for a client-streaming RPC (a before-frame +// needs a concrete request at method entry, which a stream cannot supply), so ReportLocations +// validates inside its handler instead. +app.MapWolverineGrpcServices(); + if (app.Environment.IsDevelopment()) { app.MapOpenApi(); diff --git a/src/CritterCab.Telemetry/ReportLocations/H3CellIndexer.cs b/src/CritterCab.Telemetry/ReportLocations/H3CellIndexer.cs new file mode 100644 index 0000000..00a0cc7 --- /dev/null +++ b/src/CritterCab.Telemetry/ReportLocations/H3CellIndexer.cs @@ -0,0 +1,38 @@ +using H3; +using H3.Extensions; +using NetTopologySuite.Geometries; + +namespace CritterCab.Telemetry.ReportLocations; + +// Wraps the single line of H3 binding that computes a cell from a ping (W006 §6.2 step 3). +// It is wrapped rather than inlined because that line has a footgun on BOTH axes, and the two +// available API paths are mirror opposites on each: +// +// H3.Model.LatLng is (lat, lon) in RADIANS +// NetTopologySuite.Coordinate is (lon, lat) = (X, Y) in DEGREES +// +// The proto carries lat/lon as degrees, so the Coordinate path is degrees-native and removes the +// unit conversion entirely — collapsing the risk to one axis-order line, which the pinning test +// in H3CellIndexerTests locks down. NetTopologySuite is pulled in by pocketken.H3 itself, so this +// path costs no additional dependency. +// +// DO NOT switch this to H3Index.FromLatLng via its (double, double) tuple overload. The tuple's +// implicit conversion to LatLng performs NO degree-to-radian conversion, so passing degrees is a +// ~57x scale error that still yields a valid-LOOKING cell. (The library's own prose claims the +// tuple accepts degrees; the code disagrees, and the code wins.) +public static class H3CellIndexer +{ + // Returns null rather than throwing when the cell cannot be computed. H3 signals bad input by + // returning H3Index.Invalid — an out-of-range resolution or a non-finite coordinate — and + // never throws, so "invalid" is a value to branch on, not an exception to catch. A valid + // TelemetryPolicy keeps the resolution in range, which makes this a second safety net behind + // per-ping validation rather than the primary guard. Callers treat null as the silent-drop + // path (§6.2: invalid pings are dropped, not errored). + public static string? TryComputeCell(double latDegrees, double lonDegrees, int resolution) + { + // X = lon FIRST, Y = lat second. This argument order is the whole footgun. + var cell = new Coordinate(lonDegrees, latDegrees).ToH3Index(resolution); + + return cell.IsValidCell ? cell.ToString() : null; + } +} diff --git a/src/CritterCab.Telemetry/ReportLocations/IDriverLocationPublisher.cs b/src/CritterCab.Telemetry/ReportLocations/IDriverLocationPublisher.cs new file mode 100644 index 0000000..12d3be8 --- /dev/null +++ b/src/CritterCab.Telemetry/ReportLocations/IDriverLocationPublisher.cs @@ -0,0 +1,33 @@ +using CritterCab.Telemetry.V1; + +namespace CritterCab.Telemetry.ReportLocations; + +// The slice-3 Kafka publish, held behind a seam so slice 2 can be built and tested without any +// broker (W006 §6.3). The payload is the generated DriverLocationUpdated — the real published +// language shared with Dispatch (ADR-018) — so PR C swaps only the IMPLEMENTATION below for a +// WolverineFx.Kafka producer, never this contract. +public interface IDriverLocationPublisher +{ + Task PublishAsync(DriverLocationUpdated update, CancellationToken ct); +} + +// PR B stand-in. Deliberately not a no-op: logging makes the publish observable end-to-end while +// the transport is absent, which is how a manual run of the ingest can be seen working before +// Kafka exists. Replaced in PR C by the real producer publishing to +// telemetry.driver-location-updated, partitioned by driverId. +public sealed class LoggingDriverLocationPublisher(ILogger logger) + : IDriverLocationPublisher +{ + public Task PublishAsync(DriverLocationUpdated update, CancellationToken ct) + { + logger.LogInformation( + "DriverLocationUpdated (not yet transported): driver {DriverId} at cell {H3Cell} " + + "resolution {H3Resolution}, policy version {ThrottlePolicyVersion}.", + update.DriverId, + update.H3Cell, + update.H3Resolution, + update.ThrottlePolicyVersion); + + return Task.CompletedTask; + } +} diff --git a/src/CritterCab.Telemetry/ReportLocations/IDriverPrincipalAccessor.cs b/src/CritterCab.Telemetry/ReportLocations/IDriverPrincipalAccessor.cs new file mode 100644 index 0000000..341f58c --- /dev/null +++ b/src/CritterCab.Telemetry/ReportLocations/IDriverPrincipalAccessor.cs @@ -0,0 +1,34 @@ +namespace CritterCab.Telemetry.ReportLocations; + +// driverId provenance seam (W006 R5). The driver's identity comes from the authenticated +// principal, NEVER from the ping payload — which is why report_locations.proto deliberately has +// no driver_id field. Enforcement lives here and in the handler, not in the contract. +public interface IDriverPrincipalAccessor +{ + // Null when the caller presented no resolvable driver identity; the handler turns that into + // an Unauthenticated status rather than guessing. + Guid? CurrentDriverId { get; } +} + +// Development stand-in: reads a well-known header off the ambient request. gRPC call metadata +// travels as HTTP/2 headers, so a client sending the "x-driver-id" metadata entry lands here. +// +// SWAP SITE — this is the single place that changes when the Identity BC is built. The real +// implementation reads an Entra-issued claim off HttpContext.User instead of a header; nothing +// else in Telemetry moves, because everything downstream depends only on IDriverPrincipalAccessor. +// Mirrors the ready-to-swap seam idiom used by Dispatch's PricingClientStub. +public sealed class HeaderDriverPrincipalAccessor(IHttpContextAccessor accessor) + : IDriverPrincipalAccessor +{ + public const string DriverIdHeader = "x-driver-id"; + + public Guid? CurrentDriverId + { + get + { + var header = accessor.HttpContext?.Request.Headers[DriverIdHeader]; + + return Guid.TryParse(header?.ToString(), out var driverId) ? driverId : null; + } + } +} diff --git a/src/CritterCab.Telemetry/ReportLocations/ReportLocationsHandler.cs b/src/CritterCab.Telemetry/ReportLocations/ReportLocationsHandler.cs new file mode 100644 index 0000000..0c28507 --- /dev/null +++ b/src/CritterCab.Telemetry/ReportLocations/ReportLocationsHandler.cs @@ -0,0 +1,180 @@ +using CritterCab.Telemetry.TelemetryPolicy; +using CritterCab.Telemetry.V1; +using Google.Protobuf.WellKnownTypes; +using Grpc.Core; +using Marten; +using LastKnownPositionDocument = global::CritterCab.Telemetry.LastKnownPosition.LastKnownPosition; +using TelemetryPolicyView = global::CritterCab.Telemetry.TelemetryPolicy.TelemetryPolicy; + +namespace CritterCab.Telemetry.ReportLocations; + +// W006 §6.2 — the windowed GPS ingest. The whole client stream is the message: Wolverine's +// generated wrapper hands the entire inbound RPC stream in as IAsyncEnumerable and +// returns this method's result as the single response on half-close. +// +// The per-ping pipeline lives INSIDE this method rather than at the boundary, and that is forced, +// not stylistic: Wolverine cannot weave Before/Validate middleware for client-streaming, because +// a before-frame needs a concrete request instance at method entry and a stream cannot provide +// one. So unlike slice 1's ConfigureTelemetryPolicy — which validates at the HTTP boundary with +// FluentValidation — validation and the publish trigger are handler concerns here. Do not try to +// add a boundary validator to this RPC. +public static class ReportLocationsHandler +{ + // §6.2 step 2 names an accuracyMeters threshold but fixes no value, and it is not a + // TelemetryPolicy parameter. Documented constant, same call §6.4 made for the 3x-heartbeat + // eviction threshold: it keeps slice 1's policy shape stable. Promote it to a policy param in + // v2 if ops needs to tune it. 100m is a permissive urban-GPS quality gate — it rejects + // wildly-uncertain fixes without discarding ordinary city-canyon noise. + public const double MaxAccuracyMeters = 100d; + + public static async Task Handle( + IAsyncEnumerable pings, + IDriverPrincipalAccessor principal, + IDriverLocationPublisher publisher, + IDocumentSession session, + TimeProvider time, + CancellationToken ct) + { + // R5: identity comes from the principal, never the payload. No driver, no window. + var driverId = principal.CurrentDriverId + ?? throw new RpcException(new Status( + StatusCode.Unauthenticated, + "No driver identity was presented on this stream.")); + + // Read policy once at window open and hold it for the window's duration (§6.2). A policy + // reconfigured mid-window takes effect on the next window, which is what makes + // throttlePolicyVersion in the ack meaningful: it names the policy that actually governed + // these pings. + var policy = await session.Events.AggregateStreamAsync( + TelemetryPolicyStream.Id, token: ct) + ?? throw new RpcException(new Status( + StatusCode.FailedPrecondition, + "No telemetry policy is configured.")); + + // Read the trigger baseline once, then keep it current in memory as this window publishes. + // Safe because within a window the driver's own pings are the sole writer of their own + // document. A null baseline — new driver, or one evicted by the §6.4 sweep — makes the + // first ping publish immediately. + var baseline = await session.LoadAsync(driverId, ct); + + var acceptedCount = 0; + + await foreach (var ping in pings.WithCancellation(ct)) + { + // Server-stamped on arrival: the monotonic LWW/dedup key (R7). deviceTimestamp is + // retained on the wire but never trusted for ordering — client clocks skew. + var serverReceivedAt = time.GetUtcNow(); + + if (!IsAcceptable(ping)) + continue; // silently dropped, not errored — no per-ping error frame in v1 (§6.2) + + var h3Cell = H3CellIndexer.TryComputeCell(ping.Lat, ping.Lon, policy.H3Resolution); + if (h3Cell is null) + continue; // second safety net; H3 returns Invalid rather than throwing + + acceptedCount++; + + if (!ShouldPublish(baseline, h3Cell, serverReceivedAt, policy)) + continue; // accepted but absorbed — no publish, and no document write (§6.4) + + // Publish-first, then store (§6.3). The orderings fail differently and this one fails + // benignly: publish-then-failed-store means the next ping re-evaluates against a stale + // baseline and may republish, which the consumer's (driverId, serverReceivedAt) dedup + // absorbs. Store-then-failed-publish would instead make Dispatch MISS this cell change + // until the next heartbeat. A duplicate is cheaper than a miss, and the heartbeat is + // the self-healing backstop either way. No outbox — the paired write is a document + // upsert, not an event append, and the consistency model is explicitly eventual/LWW. + await publisher.PublishAsync( + BuildUpdate(driverId, ping, h3Cell, serverReceivedAt, policy), ct); + + baseline = new LastKnownPositionDocument + { + Id = driverId, + Lat = ping.Lat, + Lon = ping.Lon, + H3Cell = h3Cell, + ServerReceivedAt = serverReceivedAt + }; + + session.Store(baseline); + await session.SaveChangesAsync(ct); + } + + // Returned once, on half-close (§6.2 window semantics). + return new LocationIngestAck + { + // Only pings that passed validation — the client can compare this against what it sent + // to detect a systematically bad sensor. + AcceptedCount = acceptedCount, + // Lets the client correct for clock skew. + ServerTime = Timestamp.FromDateTimeOffset(time.GetUtcNow()), + // The policy singleton's stream version, so a client can detect a policy change + // across windows. + ThrottlePolicyVersion = policy.Version + }; + } + + // shouldPublish = heartbeatDue OR (cellChanged AND throttleFloorElapsed) — §6.2 + // + // heartbeatDue subsumes the throttle floor by construction, because slice 1's boundary + // validation enforces heartbeatInterval >= minPublishInterval. So the floor only ever gates + // CELL-CHANGE publishes — which is precisely its job: stopping a driver who is hovering on a + // cell boundary from flooding Kafka. + private static bool ShouldPublish( + LastKnownPositionDocument? baseline, + string h3Cell, + DateTimeOffset serverReceivedAt, + TelemetryPolicyView policy) + { + // No baseline: a new driver, or one the eviction sweep removed. Publish immediately so a + // returning driver reappears to Dispatch at once (§6.4 "Return"). + if (baseline is null) + return true; + + // The document is written only on publish, so its stamp IS the last publish (§6.4). + var sinceLastPublish = serverReceivedAt - baseline.ServerReceivedAt; + + var heartbeatDue = sinceLastPublish >= TimeSpan.FromSeconds(policy.HeartbeatIntervalSeconds); + var cellChanged = !string.Equals(h3Cell, baseline.H3Cell, StringComparison.Ordinal); + var throttleFloorElapsed = + sinceLastPublish >= TimeSpan.FromSeconds(policy.MinPublishIntervalSeconds); + + return heartbeatDue || (cellChanged && throttleFloorElapsed); + } + + private static bool IsAcceptable(LocationPing ping) => + double.IsFinite(ping.Lat) && ping.Lat is >= -90d and <= 90d && + double.IsFinite(ping.Lon) && ping.Lon is >= -180d and <= 180d && + double.IsFinite(ping.AccuracyMeters) && + ping.AccuracyMeters is >= 0d and <= MaxAccuracyMeters; + + private static DriverLocationUpdated BuildUpdate( + Guid driverId, + LocationPing ping, + string h3Cell, + DateTimeOffset serverReceivedAt, + TelemetryPolicyView policy) + { + var update = new DriverLocationUpdated + { + DriverId = driverId.ToString(), + Lat = ping.Lat, + Lon = ping.Lon, + H3Cell = h3Cell, + // Carried explicitly rather than bit-decoded from the index: H3 self-encodes its + // resolution, but consumer clarity wins (§6.3). + H3Resolution = policy.H3Resolution, + ServerReceivedAt = Timestamp.FromDateTimeOffset(serverReceivedAt), + ThrottlePolicyVersion = policy.Version + }; + + // Optional pass-through — proto3 explicit presence, so only forward what was actually sent. + if (ping.HasSpeed) + update.Speed = ping.Speed; + + if (ping.HasHeading) + update.Heading = ping.Heading; + + return update; + } +} diff --git a/src/CritterCab.Telemetry/ReportLocations/TelemetryGrpcService.cs b/src/CritterCab.Telemetry/ReportLocations/TelemetryGrpcService.cs new file mode 100644 index 0000000..f4f9712 --- /dev/null +++ b/src/CritterCab.Telemetry/ReportLocations/TelemetryGrpcService.cs @@ -0,0 +1,21 @@ +using CritterCab.Telemetry.V1; +using Wolverine.Grpc; + +namespace CritterCab.Telemetry.ReportLocations; + +// CritterCab's first gRPC surface in code, and its first client-streaming RPC. +// +// Deliberately empty. Grpc.Tools generates TelemetryServiceBase with a virtual ReportLocations +// taking a raw IAsyncStreamReader; Wolverine's codegen then emits the override that +// adapts that reader into an IAsyncEnumerable and forwards it to +// IMessageBus.StreamAsync. The actual ingest is +// ReportLocationsHandler — a plain Wolverine handler that never sees a gRPC type. +// +// This class only declares "there is a Wolverine gRPC service behind this proto". It lives in the +// ReportLocations feature folder rather than a technical Grpc/ folder, with the handler it fronts. +// +// Client-streaming auto-codegen arrived in WolverineFx.Grpc 6.21.0; before that this shape threw +// NotSupportedException at startup and had to be hand-wired against IMessageBus. Any doc still +// describing that workaround is stale. +[WolverineGrpcService] +public abstract class TelemetryGrpcService : TelemetryService.TelemetryServiceBase; diff --git a/tests/CritterCab.Telemetry.Tests/LastKnownPosition/Slice4LastKnownPositionTests.cs b/tests/CritterCab.Telemetry.Tests/LastKnownPosition/Slice4LastKnownPositionTests.cs new file mode 100644 index 0000000..699f0d3 --- /dev/null +++ b/tests/CritterCab.Telemetry.Tests/LastKnownPosition/Slice4LastKnownPositionTests.cs @@ -0,0 +1,217 @@ +using Alba; +using CritterCab.Telemetry.LastKnownPosition; +using CritterCab.Telemetry.TelemetryPolicy; +using Marten; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Hosting; +using Shouldly; +using Xunit; +using LastKnownPositionDocument = global::CritterCab.Telemetry.LastKnownPosition.LastKnownPosition; + +namespace CritterCab.Telemetry.Tests.LastKnownPosition; + +// W006 §6.4 GWTs for the LastKnownPosition store and the heartbeat-absence eviction sweep. +// CritterCab's first tests over a non-event-sourced document, and its first Wolverine message +// invocation from a test. +// +// The §6.4 "No-write" GWT (ping accepted, shouldPublish = false, document unchanged) is NOT here: +// deciding *not* to write is slice 2's publish trigger, which lands in the next chunk. What is +// testable now is the store's own contract — overwrite-in-place, a policy-derived eviction +// threshold, and the absent-baseline state a returning driver finds. +[Collection("Telemetry")] +public class Slice4LastKnownPositionTests +{ + // Two adjacent H3 resolution-9 cells over Chicago. Opaque string literals on purpose: slice 4 + // only ever compares and stores cells, so nothing here should depend on H3 arithmetic. The + // real cell computation (and its lat/lon axis-order trap) belongs to slice 2. + private const string CellC1 = "8a2a1072b59ffff"; + private const string CellC2 = "8a2a1072b5affff"; + + private readonly TelemetryTestFixture _fixture; + + public Slice4LastKnownPositionTests(TelemetryTestFixture fixture) => _fixture = fixture; + + [Fact] + public async Task an_upsert_overwrites_the_drivers_document_in_place() + { + // Given driver D has a stored position at cell C1 + await _fixture.ResetPositionsAsync(); + var driverId = Guid.CreateVersion7(); + await StoreAsync(PositionFor(driverId, CellC1, DateTimeOffset.UtcNow.AddSeconds(-10))); + + // When a later publish stores D's position at cell C2 + var secondPublish = DateTimeOffset.UtcNow; + await StoreAsync(PositionFor(driverId, CellC2, secondPublish)); + + // Then the row is replaced rather than added to — one document per driver, LWW on + // ServerReceivedAt, no history retained (this is a document, not a stream) + var stored = await LoadAsync(driverId); + stored.ShouldNotBeNull(); + stored.H3Cell.ShouldBe(CellC2); + stored.ServerReceivedAt.ShouldBe(secondPublish, TimeSpan.FromMilliseconds(1)); + + (await CountPositionsAsync(driverId)).ShouldBe(1); + } + + [Fact] + public async Task the_sweep_evicts_a_position_older_than_three_heartbeats() + { + // Given the seeded policy (heartbeat 30s, so the eviction threshold is 90s) + await _fixture.ResetToSeedAsync(); + await _fixture.ResetPositionsAsync(); + + // And driver D last published five minutes ago + var driverId = Guid.CreateVersion7(); + await StoreAsync(PositionFor(driverId, CellC1, DateTimeOffset.UtcNow.AddMinutes(-5))); + + // When the periodic sweep runs + await _fixture.InvokeAsync(new EvictStalePositions()); + + // Then D's document is deleted + (await LoadAsync(driverId)).ShouldBeNull(); + + // And no staleness event is published (v1, R3/R8) — eviction is storage hygiene only, so + // the event store still holds nothing but the single bootstrap seed event + (await CountAllEventsAsync()).ShouldBe(1); + } + + [Fact] + public async Task the_sweep_leaves_a_position_inside_the_threshold_untouched() + { + // Given the seeded policy (threshold 90s), a stale driver and a fresh one + await _fixture.ResetToSeedAsync(); + await _fixture.ResetPositionsAsync(); + + var staleDriver = Guid.CreateVersion7(); + var freshDriver = Guid.CreateVersion7(); + await StoreAsync( + PositionFor(staleDriver, CellC1, DateTimeOffset.UtcNow.AddMinutes(-5)), + PositionFor(freshDriver, CellC2, DateTimeOffset.UtcNow.AddSeconds(-10))); + + // When the sweep runs + await _fixture.InvokeAsync(new EvictStalePositions()); + + // Then only the stale driver is evicted — the sweep is threshold-driven, not a blanket wipe + (await LoadAsync(staleDriver)).ShouldBeNull(); + (await LoadAsync(freshDriver)).ShouldNotBeNull(); + } + + [Fact] + public async Task a_returning_driver_finds_no_baseline_and_re_establishes_one() + { + // Given driver D was evicted by the sweep + await _fixture.ResetToSeedAsync(); + await _fixture.ResetPositionsAsync(); + + var driverId = Guid.CreateVersion7(); + await StoreAsync(PositionFor(driverId, CellC1, DateTimeOffset.UtcNow.AddMinutes(-5))); + await _fixture.InvokeAsync(new EvictStalePositions()); + + // When D pings again, the slice-2 trigger looks up D's baseline and finds none. That + // absent baseline is precisely what makes shouldPublish true on a returning driver's first + // ping (§6.4) — the trigger half of this GWT lands with slice 2. + (await LoadAsync(driverId)).ShouldBeNull(); + + // And when that immediate republish stores a position, D has a fresh baseline again + var returnPublish = DateTimeOffset.UtcNow; + await StoreAsync(PositionFor(driverId, CellC2, returnPublish)); + + var baseline = await LoadAsync(driverId); + baseline.ShouldNotBeNull(); + baseline.H3Cell.ShouldBe(CellC2); + baseline.ServerReceivedAt.ShouldBe(returnPublish, TimeSpan.FromMilliseconds(1)); + } + + [Fact] + public async Task the_eviction_threshold_follows_the_configured_heartbeat_interval() + { + // Given the policy is reconfigured to a 1-second heartbeat, so the threshold drops to 3s + await _fixture.ResetToSeedAsync(); + await _fixture.ResetPositionsAsync(); + + await _fixture.Host.Scenario(s => + { + s.Post.Json(new ConfigureTelemetryPolicy( + H3Resolution: 9, + HeartbeatIntervalSeconds: 1, + MinPublishIntervalSeconds: 1, + OperatorId: "ops-alice", + Reason: "Tight heartbeat")).ToUrl("/api/telemetry/policy"); + s.StatusCodeShouldBeOk(); + }); + + // And a position 10 seconds old — comfortably fresh under the seeded 90s threshold, and + // the exact age that survives in the test above + var driverId = Guid.CreateVersion7(); + await StoreAsync(PositionFor(driverId, CellC1, DateTimeOffset.UtcNow.AddSeconds(-10))); + + // When the sweep runs + await _fixture.InvokeAsync(new EvictStalePositions()); + + // Then it is evicted: the threshold is read from the policy view each sweep, not hardcoded + (await LoadAsync(driverId)).ShouldBeNull(); + } + + [Fact] + public async Task the_host_starts_with_the_eviction_timer_registered() + { + // The fixture host strips the eviction BackgroundService for determinism, and the smoke + // test runs without a connection string (so the registration is skipped with the whole + // Marten block). Without this test nothing would cover the production wiring at all. + // + // It matters because a BackgroundService is a SINGLETON: every constructor dependency has + // to resolve from the root provider. Injecting Wolverine's IMessageBus directly would not + // — it is registered scoped (Wolverine HostBuilderExtensions.cs:232) — which is exactly why + // LastKnownPositionEvictionService takes an IServiceScopeFactory and opens a scope per tick. + // Resolving the hosted services here forces construction, so a regression on that shape + // fails here rather than at deploy time. + await using var host = await AlbaHost.For(builder => + builder.UseSetting("ConnectionStrings:crittercab_telemetry", _fixture.ConnectionString)); + + host.Services.GetServices() + .OfType() + .ShouldHaveSingleItem(); + } + + private static LastKnownPositionDocument PositionFor( + Guid driverId, + string h3Cell, + DateTimeOffset serverReceivedAt) => + new() + { + Id = driverId, + Lat = 41.8781, + Lon = -87.6298, + H3Cell = h3Cell, + ServerReceivedAt = serverReceivedAt + }; + + private async Task StoreAsync(params LastKnownPositionDocument[] positions) + { + var store = _fixture.Host.Services.GetRequiredService(); + await using var session = store.LightweightSession(); + session.Store(positions); + await session.SaveChangesAsync(); + } + + private async Task LoadAsync(Guid driverId) + { + var store = _fixture.Host.Services.GetRequiredService(); + await using var session = store.QuerySession(); + return await session.LoadAsync(driverId); + } + + private async Task CountPositionsAsync(Guid driverId) + { + var store = _fixture.Host.Services.GetRequiredService(); + await using var session = store.QuerySession(); + return await session.Query().CountAsync(x => x.Id == driverId); + } + + private async Task CountAllEventsAsync() + { + var store = _fixture.Host.Services.GetRequiredService(); + await using var session = store.QuerySession(); + return await session.Events.QueryAllRawEvents().CountAsync(); + } +} diff --git a/tests/CritterCab.Telemetry.Tests/ReportLocations/H3CellIndexerTests.cs b/tests/CritterCab.Telemetry.Tests/ReportLocations/H3CellIndexerTests.cs new file mode 100644 index 0000000..083a810 --- /dev/null +++ b/tests/CritterCab.Telemetry.Tests/ReportLocations/H3CellIndexerTests.cs @@ -0,0 +1,77 @@ +using CritterCab.Telemetry.ReportLocations; +using H3; +using H3.Extensions; +using H3.Model; +using NetTopologySuite.Geometries; +using Shouldly; +using Xunit; + +namespace CritterCab.Telemetry.Tests.ReportLocations; + +// Pins down the H3 binding's axis order AND units together. A naive "the cell is valid" assertion +// would pass with lat and lon swapped, and would also pass with degrees fed into a radians API — +// both produce a perfectly well-formed cell for the wrong place on Earth. These tests are +// therefore built to fail on either mistake. +// +// Pure unit tests: no host, no container. H3 is a pure function. +public class H3CellIndexerTests +{ + // Chicago, the Art Institute. + private const double LatDegrees = 41.8781d; + private const double LonDegrees = -87.6298d; + private const int Resolution = 9; + + [Fact] + public void the_degrees_coordinate_path_agrees_with_the_radians_latlng_path() + { + // The production path: NetTopologySuite Coordinate is (X, Y) = (lon, lat) in DEGREES. + var viaCoordinate = new Coordinate(LonDegrees, LatDegrees).ToH3Index(Resolution); + + // The other path, with the conversion done by hand: H3's own LatLng is (lat, lon) in + // RADIANS. The two paths are mirror opposites on BOTH axes, so they can only agree if + // production has the order and the units right — two wrongs cannot accidentally cancel. + var viaLatLng = H3Index.FromLatLng( + new LatLng(LatDegrees * Math.PI / 180d, LonDegrees * Math.PI / 180d), + Resolution); + + viaCoordinate.ShouldBe(viaLatLng); + viaCoordinate.IsValidCell.ShouldBeTrue(); + } + + [Fact] + public void swapping_the_axes_yields_a_different_cell() + { + // This is what makes the test above meaningful: a swapped Coordinate still produces a + // valid cell, just one somewhere off the coast of Somalia. Validity alone proves nothing. + var correct = new Coordinate(LonDegrees, LatDegrees).ToH3Index(Resolution); + var swapped = new Coordinate(LatDegrees, LonDegrees).ToH3Index(Resolution); + + swapped.ShouldNotBe(correct); + } + + [Fact] + public void the_indexer_returns_the_cell_computed_by_the_degrees_path() + { + var expected = new Coordinate(LonDegrees, LatDegrees).ToH3Index(Resolution); + + H3CellIndexer.TryComputeCell(LatDegrees, LonDegrees, Resolution) + .ShouldBe(expected.ToString()); + } + + [Fact] + public void an_out_of_range_resolution_yields_no_cell_rather_than_throwing() + { + // H3 signals bad input by returning H3Index.Invalid, never by throwing — so the indexer + // branches on it instead of catching. 15 is MAX_H3_RES, which is also the upper bound + // slice 1's policy validator enforces. + H3CellIndexer.TryComputeCell(LatDegrees, LonDegrees, 16).ShouldBeNull(); + } + + [Fact] + public void a_non_finite_coordinate_yields_no_cell() + { + // The second safety net behind per-ping validation: even if a NaN slipped through, the + // ping is dropped rather than indexed to nowhere. + H3CellIndexer.TryComputeCell(double.NaN, LonDegrees, Resolution).ShouldBeNull(); + } +} diff --git a/tests/CritterCab.Telemetry.Tests/ReportLocations/Slice2ReportLocationsTests.cs b/tests/CritterCab.Telemetry.Tests/ReportLocations/Slice2ReportLocationsTests.cs new file mode 100644 index 0000000..3ec903c --- /dev/null +++ b/tests/CritterCab.Telemetry.Tests/ReportLocations/Slice2ReportLocationsTests.cs @@ -0,0 +1,223 @@ +using Alba; +using CritterCab.Telemetry.ReportLocations; +using CritterCab.Telemetry.V1; +using Google.Protobuf.WellKnownTypes; +using Grpc.Core; +using Marten; +using Microsoft.Extensions.DependencyInjection; +using Shouldly; +using Xunit; +using LastKnownPositionDocument = global::CritterCab.Telemetry.LastKnownPosition.LastKnownPosition; + +namespace CritterCab.Telemetry.Tests.ReportLocations; + +// W006 §6.2 GWTs, driven over a REAL gRPC client stream against the Alba host — CritterCab's +// first gRPC traffic in a test. Also covers the §6.4 "No-write" GWT, which slice 4 could not +// reach on its own because deciding not to write is this slice's trigger. +[Collection("Telemetry")] +public class Slice2ReportLocationsTests +{ + // Two Chicago points roughly 7km apart — comfortably different cells at resolution 9, whose + // edges are ~174m. Cells are computed rather than hardcoded so the tests state the intent + // ("a different cell") instead of reciting opaque H3 ids. + private const double LoopLat = 41.8827d, LoopLon = -87.6233d; + private const double WrigleyLat = 41.9484d, WrigleyLon = -87.6553d; + + private readonly TelemetryTestFixture _fixture; + + public Slice2ReportLocationsTests(TelemetryTestFixture fixture) => _fixture = fixture; + + [Fact] + public async Task a_cell_change_past_the_throttle_floor_publishes_and_stores() + { + // Given the seeded policy { h3: 9, heartbeat: 30s, minPublish: 5s } + // And driver D last published from the Wrigley cell 10 seconds ago + var driverId = await ArrangeAsync( + baselineLat: WrigleyLat, baselineLon: WrigleyLon, lastPublishedSecondsAgo: 10); + + // When a ping arrives from the Loop cell + var ack = await StreamAsync(driverId, PingAt(LoopLat, LoopLon)); + + // Then the ping is accepted, and shouldPublish is true (cell changed AND floor elapsed) + ack.AcceptedCount.ShouldBe(1); + ack.ThrottlePolicyVersion.ShouldBe(1L); + + // And the publish fires with the published-language payload Dispatch will consume + var published = _fixture.Publisher.Published.ShouldHaveSingleItem(); + published.DriverId.ShouldBe(driverId.ToString()); + published.H3Cell.ShouldBe(CellAt(LoopLat, LoopLon)); + published.H3Resolution.ShouldBe(9); + + // And the document is upserted to the new cell (publish-first, then store) + var stored = await LoadAsync(driverId); + stored.ShouldNotBeNull(); + stored.H3Cell.ShouldBe(CellAt(LoopLat, LoopLon)); + } + + [Fact] + public async Task a_cell_change_inside_the_throttle_floor_is_accepted_but_absorbed() + { + // Given D last published from the Wrigley cell only 2 seconds ago (floor is 5s) + var driverId = await ArrangeAsync( + baselineLat: WrigleyLat, baselineLon: WrigleyLon, lastPublishedSecondsAgo: 2); + var baselineBefore = await LoadAsync(driverId); + + // When a ping arrives from the Loop cell + var ack = await StreamAsync(driverId, PingAt(LoopLat, LoopLon)); + + // Then the ping still counts as accepted — throttling is not rejection + ack.AcceptedCount.ShouldBe(1); + + // But nothing is published: the floor gates cell-change publishes, which is what stops a + // driver hovering on a cell boundary from flooding Kafka + _fixture.Publisher.Published.ShouldBeEmpty(); + + // And — the §6.4 "No-write" GWT — the document is untouched, because it is written only + // on publish, never per ping + var baselineAfter = await LoadAsync(driverId); + baselineAfter.ShouldNotBeNull(); + baselineAfter.H3Cell.ShouldBe(baselineBefore!.H3Cell); + baselineAfter.ServerReceivedAt.ShouldBe(baselineBefore.ServerReceivedAt, TimeSpan.FromMilliseconds(1)); + } + + [Fact] + public async Task a_due_heartbeat_publishes_even_without_a_cell_change() + { + // Given D last published from the Loop cell 31 seconds ago (heartbeat is 30s) + var driverId = await ArrangeAsync( + baselineLat: LoopLat, baselineLon: LoopLon, lastPublishedSecondsAgo: 31); + + // When a ping arrives from the SAME cell + var ack = await StreamAsync(driverId, PingAt(LoopLat, LoopLon)); + + // Then it publishes anyway — heartbeatDue is independent of cell change, and is what makes + // a dropped publish self-heal within one heartbeat + ack.AcceptedCount.ShouldBe(1); + _fixture.Publisher.Published.ShouldHaveSingleItem() + .H3Cell.ShouldBe(CellAt(LoopLat, LoopLon)); + + // And the document's stamp advances, restarting both the heartbeat and the floor + var stored = await LoadAsync(driverId); + stored.ShouldNotBeNull(); + stored.ServerReceivedAt.ShouldBeGreaterThan(DateTimeOffset.UtcNow.AddSeconds(-5)); + } + + [Fact] + public async Task the_window_ack_counts_only_pings_that_passed_validation() + { + // Given a driver with no baseline at all + var driverId = await ArrangeAsync(baselineLat: null, baselineLon: null, lastPublishedSecondsAgo: 0); + + // When five pings are streamed, of which two are invalid + var ack = await StreamAsync( + driverId, + PingAt(LoopLat, LoopLon), + PingAt(999d, LoopLon), // latitude out of range + PingAt(LoopLat, LoopLon), + PingAt(LoopLat, LoopLon, accuracyMeters: 5000d), // beyond the accuracy threshold + PingAt(LoopLat, LoopLon)); + + // Then the ack reports only the three that passed. Invalid pings are silently dropped, not + // errored — there is no per-ping error frame in v1, and the stream is not torn down. + ack.AcceptedCount.ShouldBe(3); + + // And exactly one publish happened: the first ping had no baseline so it published + // immediately (§6.4 "Return"), and the rest fell inside the throttle floor + _fixture.Publisher.Published.ShouldHaveSingleItem(); + } + + [Fact] + public async Task a_stream_with_no_driver_identity_is_rejected() + { + await ResetAsync(); + + // R5: driverId comes from the principal and is never carried in the payload — which is why + // the proto has no driver_id field. A stream that presents no identity cannot be attributed. + using var channel = _fixture.CreateGrpcChannel(); + var client = new TelemetryService.TelemetryServiceClient(channel); + using var call = client.ReportLocations(); + + var exception = await Should.ThrowAsync(async () => + { + await call.RequestStream.WriteAsync(PingAt(LoopLat, LoopLon)); + await call.RequestStream.CompleteAsync(); + await call.ResponseAsync; + }); + + exception.StatusCode.ShouldBe(StatusCode.Unauthenticated); + _fixture.Publisher.Published.ShouldBeEmpty(); + } + + // Resets policy, documents and recorded publishes, then optionally seeds D's trigger baseline. + private async Task ArrangeAsync(double? baselineLat, double? baselineLon, int lastPublishedSecondsAgo) + { + await ResetAsync(); + + var driverId = Guid.CreateVersion7(); + + if (baselineLat is null || baselineLon is null) + return driverId; + + var store = _fixture.Host.Services.GetRequiredService(); + await using var session = store.LightweightSession(); + + session.Store(new LastKnownPositionDocument + { + Id = driverId, + Lat = baselineLat.Value, + Lon = baselineLon.Value, + H3Cell = CellAt(baselineLat.Value, baselineLon.Value), + ServerReceivedAt = DateTimeOffset.UtcNow.AddSeconds(-lastPublishedSecondsAgo) + }); + + await session.SaveChangesAsync(); + + return driverId; + } + + private async Task ResetAsync() + { + await _fixture.ResetToSeedAsync(); + await _fixture.ResetPositionsAsync(); + _fixture.Publisher.Clear(); + } + + private async Task StreamAsync(Guid driverId, params LocationPing[] pings) + { + using var channel = _fixture.CreateGrpcChannel(); + var client = new TelemetryService.TelemetryServiceClient(channel); + + // gRPC call metadata travels as HTTP/2 headers, which is how the dev principal accessor + // sees it. The real Entra claim replaces this without the handler changing. + using var call = client.ReportLocations(new Metadata + { + { HeaderDriverPrincipalAccessor.DriverIdHeader, driverId.ToString() } + }); + + foreach (var ping in pings) + await call.RequestStream.WriteAsync(ping); + + // Half-close: this is what makes the single ack come back. + await call.RequestStream.CompleteAsync(); + + return await call.ResponseAsync; + } + + private async Task LoadAsync(Guid driverId) + { + var store = _fixture.Host.Services.GetRequiredService(); + await using var session = store.QuerySession(); + return await session.LoadAsync(driverId); + } + + private static string CellAt(double lat, double lon) => + H3CellIndexer.TryComputeCell(lat, lon, 9)!; + + private static LocationPing PingAt(double lat, double lon, double accuracyMeters = 8d) => new() + { + Lat = lat, + Lon = lon, + AccuracyMeters = accuracyMeters, + DeviceTimestamp = Timestamp.FromDateTimeOffset(DateTimeOffset.UtcNow) + }; +} diff --git a/tests/CritterCab.Telemetry.Tests/TelemetryTestFixture.cs b/tests/CritterCab.Telemetry.Tests/TelemetryTestFixture.cs index 2cf4b99..b831d9f 100644 --- a/tests/CritterCab.Telemetry.Tests/TelemetryTestFixture.cs +++ b/tests/CritterCab.Telemetry.Tests/TelemetryTestFixture.cs @@ -1,9 +1,17 @@ +using System.Collections.Concurrent; using Alba; +using CritterCab.Telemetry.LastKnownPosition; +using CritterCab.Telemetry.ReportLocations; using CritterCab.Telemetry.TelemetryPolicy; +using CritterCab.Telemetry.V1; +using Grpc.Net.Client; using Marten; +using Microsoft.AspNetCore.TestHost; using Microsoft.Extensions.DependencyInjection; using Testcontainers.PostgreSql; +using Wolverine; using Xunit; +using LastKnownPositionDocument = global::CritterCab.Telemetry.LastKnownPosition.LastKnownPosition; namespace CritterCab.Telemetry.Tests; @@ -19,6 +27,13 @@ public class TelemetryTestFixture : IAsyncLifetime public IAlbaHost Host { get; private set; } = null!; + // Exposed so a test can build a second host with the production wiring intact — the fixture + // host deliberately strips the eviction timer, so nothing else would cover its registration. + public string ConnectionString => _postgres.GetConnectionString(); + + // Stands in for the slice-3 Kafka publish so slice-2 tests can assert on the publish trigger. + public RecordingDriverLocationPublisher Publisher { get; } = new(); + public async Task InitializeAsync() { await _postgres.StartAsync(); @@ -26,6 +41,27 @@ public async Task InitializeAsync() Host = await AlbaHost.For(builder => { builder.UseSetting("ConnectionStrings:crittercab_telemetry", _postgres.GetConnectionString()); + + // ConfigureTestServices, not ConfigureServices: this hook runs AFTER the entry point's + // own registrations, which is the only ordering where removing and replacing them + // works. + builder.ConfigureTestServices(services => + { + // Drop the slice-4 eviction timer. The BackgroundService is the deliberately + // untested half of slice 4 — all its logic lives in EvictStalePositionsHandler — + // and leaving it ticking would let a background sweep race the eviction tests' + // own explicit invocations. + var timer = services.FirstOrDefault( + d => d.ImplementationType == typeof(LastKnownPositionEvictionService)); + + if (timer is not null) + services.Remove(timer); + + // Swap the PR B logging stub for a recorder so slice-2 tests can assert what was + // published. This is the same seam PR C swaps for the real Kafka producer, which + // is the point of it being a seam. + services.AddSingleton(Publisher); + }); }); } @@ -44,6 +80,52 @@ public async Task ResetToSeedAsync() await store.Advanced.Clean.DeleteAllEventDataAsync(); await new TelemetryPolicyBootstrap().Populate(store, CancellationToken.None); } + + // LastKnownPosition is a plain document, so it survives ResetToSeedAsync (which only clears + // event data). Slice-4 tests wipe it separately to start from a known-empty store. + public async Task ResetPositionsAsync() + { + var store = Host.Services.GetRequiredService(); + await store.Advanced.Clean.DeleteDocumentsByTypeAsync(typeof(LastKnownPositionDocument)); + } + + // Sends a message through Wolverine exactly the way production does. IMessageBus is registered + // scoped, so it must be resolved from a scope rather than the root provider — the same reason + // LastKnownPositionEvictionService creates a scope per tick. InvokeAsync is inline and awaited, + // so there is no async commit to wait out and no tracked session needed. + public async Task InvokeAsync(object message) + { + await using var scope = Host.Services.CreateAsyncScope(); + var bus = scope.ServiceProvider.GetRequiredService(); + await bus.InvokeAsync(message); + } + + // Gate 11: a GrpcChannel over Alba's host rather than the raw WebApplication+UseTestServer + // recipe Wolverine's own client-streaming fixture uses. Alba wraps WebApplicationFactory, + // which runs on TestServer underneath, so GetTestServer().CreateHandler() reaches the same + // in-memory transport — meaning CritterCab keeps its Alba-first default and gains gRPC + // without a second, parallel host recipe. + public GrpcChannel CreateGrpcChannel() => + GrpcChannel.ForAddress("http://localhost", new GrpcChannelOptions + { + HttpHandler = Host.GetTestServer().CreateHandler() + }); +} + +// Records what slice 2's publish trigger fired, standing in for slice 3's Kafka producer. +public sealed class RecordingDriverLocationPublisher : IDriverLocationPublisher +{ + private readonly ConcurrentQueue _published = new(); + + public IReadOnlyList Published => [.. _published]; + + public void Clear() => _published.Clear(); + + public Task PublishAsync(DriverLocationUpdated update, CancellationToken ct) + { + _published.Enqueue(update); + return Task.CompletedTask; + } } [CollectionDefinition("Telemetry")]