From 54375d4627dcb028e693f76febcef993db22bdf8 Mon Sep 17 00:00:00 2001 From: Erik Shafer Date: Fri, 24 Jul 2026 23:09:38 -0500 Subject: [PATCH 1/8] =?UTF-8?q?Prompt=20009=20=E2=80=94=20Dispatch=20consu?= =?UTF-8?q?mes=20telemetry.driver-location-updated=20(W006=20slice=205)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- ...h-w006-slice-5-nearby-available-drivers.md | 211 ++++++++++++++++++ 1 file changed, 211 insertions(+) create mode 100644 docs/prompts/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md diff --git a/docs/prompts/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md b/docs/prompts/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md new file mode 100644 index 0000000..4cfddd2 --- /dev/null +++ b/docs/prompts/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md @@ -0,0 +1,211 @@ +# Prompt 009 — Dispatch consumes `telemetry.driver-location-updated` (W006 slice 5 / W001 §5.3 close) + +| Field | Value | +|---|---| +| **Status** | Ready — three durable forks resolved by user sign-off 2026-07-24 (availability half: exclude-until-ASB with an unbound handler; H3 query resolution: track last-received; ETA: derived from distance via a named invented constant). All eight Verify-before-wiring gates closed at authoring time. Rides in the PR D session's PR alongside the implementation; not committed standalone. | +| **Authored** | 2026-07-24 | +| **Target artifacts** | `src/CritterCab.Dispatch/CritterCab.Dispatch.csproj` (Kafka + Protobuf + H3 + proto codegen), `src/CritterCab.Dispatch/AvailableDrivers/` (**new slice folder** — document, two handlers, the query adapter, the H3 helper), `src/CritterCab.Dispatch/Program.cs` (Kafka listener, Marten document schema, source registration swap), `tests/CritterCab.Dispatch.Tests/AvailableDrivers/` (**new** Kafka-backed fixture + suites), `apphost.cs` (Dispatch → Kafka reference), `docs/workshops/006-telemetry-event-model.md` (§6.5 Document History), `docs/workshops/001-dispatch-event-model.md` (§5.3 realization note), `docs/skills/wolverine-kafka/SKILL.md` (listener sections rewritten from shipped code — the deferred DEBT row), `docs/skills/DEBT.md` (close that row), `docs/prompts/README.md` (index entry), this prompt's retro. | +| **Source-of-truth dependencies** | [W006 §6.5 (the slice), §6.3's Dedup GWT (consumer-side, untestable until now), §3.2 (stream-processing shape)](../../workshops/006-telemetry-event-model.md); [W001 §5.3 (the amendment target) + §10 parking-lot #4](../../workshops/001-dispatch-event-model.md); [ADR-018](../../decisions/018-candidate-projection-ownership-and-telemetry-geospatial-supply.md) (consumer half); [ADR-005](../../decisions/005-transport-selection-by-flow-type.md); [ADR-009](../../decisions/009-protobuf-contracts-as-first-class-artifacts.md); [ADR-019](../../decisions/019-transport-agnostic-topic-naming.md). Skills: **`wolverine-handlers`** (base shape), **`wolverine-messaging-handlers`** (the governing skill for both new handlers), **`wolverine-kafka`** (transport wiring — and the one this session corrects), `marten-querying`, `transport-selection`, `protobuf-contracts`, `vertical-slice-organization`, `csharp-coding-standards`, `testing-integration`, `testing-fundamentals`, `service-bootstrap`, `aspire`. **`wolverine-marten-automation` does *not* govern this session** — see § Skill corrections. | +| **Workflow position** | **PR D** — fourth implementation session of the W006 chain and the last pending W006 slice. First session to write code in `CritterCab.Dispatch` since slice 5.3 (prompt 005). Follows PR [#46](https://github.com/erikshafer/CritterCab/pull/46). **The next session must be a design-or-tidy return** — see § Follow-on PR sequence. | + +--- + +## Framing — why this session exists + +Every prior session in this chain built one side of a boundary. This one crosses it. + +Until now "CritterCab has two services" has meant two services that have never spoken. Telemetry accepts a gRPC ping, throttles it, publishes `DriverLocationUpdated` to a Kafka topic — and nothing reads it. Dispatch quotes fares and selects candidates entirely in-process over Marten, from a hardcoded `NearbyAvailableDriversStub`. After this session a GPS ping entering Telemetry over gRPC comes out the other side as a row in Dispatch's document store. **That is CritterCab's first cross-service flow**, and it is the thing the whole project exists to demonstrate. + +Three shifts make this session unlike the last three, and each one is a place to slow down: + +1. **It lands in `CritterCab.Dispatch`, not Telemetry.** Dispatch has never had a transport. Its `Program.cs` has no `UseKafka*`, its `.csproj` has no protobuf codegen, and its test fixture has no broker. All three change here. +2. **Dispatch generates its own copy of a Telemetry-owned proto.** This is the first time two services compile the same `.proto`. It is *not* a shared assembly and must never become one — the no-shared-code constraint holds, and gate 3 below explains why Wolverine tolerates two independently generated copies of the same type. +3. **The ADR-018 join is only half-built by this PR, deliberately.** Slice 5 does the Kafka half (driver positions). The ASB half (Driver-Profile availability) is a forward-constraint to an un-workshopped BC. **Do not model or build it.** Fork 1 below settles exactly how far the availability side is allowed to go. + +**No narrative anchors this session.** The consumer side of a telemetry feed has no protagonist-perceivable moment; PR #40's reasoning for Telemetry applies unchanged. W006 §6.5 and W001 §5.3 are the direct spec anchors. + +**One negative instruction, as load-bearing as anything positive here:** `CandidateSelectionAutomation.cs` must not appear in the diff. W006 §6.5 promises "the stub is replaced, handlers untouched," and that promise is the entire payoff of having built `INearbyAvailableDriversSource` as a seam back in slice 5.3. If the session finds itself editing the automation, the adapter is the wrong shape. + +--- + +## Goal + +Stand up a Wolverine Kafka listener in Dispatch that consumes `telemetry.driver-location-updated` into a per-driver `AvailableDriver` document via atomic last-writer-wins upsert, back `INearbyAvailableDriversSource` with an H3 k-ring query over that document store, demote `NearbyAvailableDriversStub` from production registration to test double, and close W001 §5.3's parking-lot #4 in code. + +--- + +## Spec delta + +- **W006 §6.5 moves designed → realized in code.** The `AvailableDriver` document, the LWW-per-side consistency model, the H3 k-ring radius query, and the stub-seam replacement all become concrete. This is the **last pending W006 slice** — the workshop's slice walk closes. +- **W006 §6.3's Dedup GWT is finally exercised.** PR C structurally could not test it: it asserts *consumer* behaviour against at-least-once redelivery, and there was no consumer. It lands here as a real test against a real broker. +- **W001 §5.3's amendment moves from recorded to realized.** The 2026-06-30 W006 amendment already resolved parking-lot #4 *on paper*; this session adds the realization note saying the transport now runs, the view exists, and the stub is no longer in the production graph. **An amendment to an amendment — a realization record, not a new design decision.** +- **W006 §6.5 gains three implementation-time qualifiers it did not name** (the three forks below): what a driver with no availability side means before Driver Profile exists; where the H3 query resolution comes from when the resolution is Telemetry's to set; and how `EtaSeconds` is produced by a view with no ETA source. **Amendments, not corrections** — §6.5 is silent on all three rather than wrong about any. + +--- + +## Orientation files (read in order) + +1. This prompt, end to end, including § Decisions resolved. +2. [`docs/workshops/006-telemetry-event-model.md`](../../workshops/006-telemetry-event-model.md) **§6.5** (the slice), then **§6.3** (the publish this consumes + the Dedup GWT), then **§3.2/§3.3** (why none of this is event-sourced). +3. [`docs/workshops/001-dispatch-event-model.md`](../../workshops/001-dispatch-event-model.md) **§5.3**, including its 2026-06-30 W006 amendment at the end — that is what this session realizes. +4. [`ADR-018`](../../decisions/018-candidate-projection-ownership-and-telemetry-geospatial-supply.md) — why the view is Dispatch-owned rather than a Telemetry query. +5. `src/CritterCab.Telemetry/Program.cs` § Kafka block and `ReportLocations/KafkaDriverLocationPublisher.cs` — **the producer this consumer must mirror.** The listener's serialization and naming must match what ships there. +6. `src/CritterCab.Telemetry/LastKnownPosition/` — **the closest working precedent in the repo** for a plain-document, non-event-sourced write path, which is exactly what `AvailableDriver` is. Read it before designing the document. +7. `src/CritterCab.Dispatch/CandidateSelection/INearbyAvailableDriversSource.cs` and `CandidateSelectionAutomation.cs` — the port to satisfy and the consumer that must not change. +8. `src/CritterCab.Telemetry/ReportLocations/H3CellIndexer.cs` — **read the comment, not just the code.** The lat/lon axis-and-units footgun it documents applies identically to this session's k-ring helper. +9. `docs/skills/wolverine-kafka/SKILL.md` — publish-path sections are corrected and trustworthy; **§ Listening, § Consumer groups, § Batch processing and § DLQ are known-wrong** (they name a `LocationPing` → `telemetry.location-pings` pairing that never existed). Read them for mechanics only. Fixing them is this session's deliverable. +10. `tests/CritterCab.Telemetry.Tests/` Kafka fixture — the Testcontainers pattern to mirror on the Dispatch side. + +--- + +## Working pattern + +- Branch off `main`; never commit to `main`. One prompt = one session = one PR; the retro ships **inside** this PR. +- Run `critter-skill-auditor` Phase 1 before cutting code and Phase 2 after. Phase 1 materially corrected an already-source-verified prompt last session; assume it will again. +- Build and test locally — Docker works, so Testcontainers runs locally. **Do not lean on CI as the only gate**, and note that CI still cannot build `apphost.cs`, so run `dotnet build apphost.cs` by hand after touching it. +- Escalate rather than silently resolve: if a skill contradicts a spec, or the spec is silent on something load-bearing, put it to the user. Both prior sessions had authoring hypotheses overturned this way. +- Run `code-review` before opening the PR. Its two-axis form found three real defects in PR C after two auditor passes had already run. +- Surface the full PR URL on open. + +--- + +## Verify before wiring (`jasperfx-source-verifier` — local `C:\Code\JasperFx\wolverine` @ `V6.21.0-12-ge08abdeb3`, `C:\Code\JasperFx\marten` @ `V9.18.0`) + +**All eight gates were closed at authoring time** — the second consecutive prompt to achieve this. **Three gate premises were contradicted by source**, and each would have produced wrong code. Do not re-verify these; do not re-derive them. + +| # | Gate | Outcome | +|---|---|---| +| 1 | Does `.UseProtobufSerialization()` bind on the Kafka **listener** side? | **Yes.** It is a generic extension on `IEndpointConfiguration` (`Wolverine.Protobuf/WolverineProtobufSerializationExtensions.cs:33`), and `KafkaListenerConfiguration` satisfies the constraint through `InteroperableListenerConfiguration` → `ListenerConfiguration` → `IListenerConfiguration`. **Caveat:** protobuf-over-Kafka is *unexercised upstream* — it appears on no Kafka endpoint anywhere in the Wolverine repo. CritterCab is its first integration test. | +| 2 | How does the listener learn the incoming message **type**? | **Premise contradicted.** The type does not have to come from endpoint config: the default `KafkaEnvelopeMapper` writes a `message-type` header on publish, and the shipped producer uses that default mapper, so the header **is** on the wire. `.DefaultIncomingMessage()` (`ListenerConfiguration.cs:456`) exists and *replaces* the header mapping with a constant — use it as hardening on a single-type topic, not as a workaround. There is **no** `ListenToKafkaTopic(...)` generic overload and no `.ReceivesMessage()` fluent method. | +| 3 | Will Dispatch's independently generated `DriverLocationUpdated` agree with Telemetry's on the wire? | **Yes.** Wolverine's identity is `Type.FullName`-based and **assembly-agnostic** (`WolverineMessageNaming.cs:86-107`, `Envelope.cs:52`). Both resolve to `CritterCab.Telemetry.V1.DriverLocationUpdated`. **This is what makes the no-shared-assembly rule workable.** The fragile dependency is the proto's `csharp_namespace` matching across both builds, and the message staying **top-level** (a nested proto type gets a `DeclaringType_` prefix). | +| 4 | Does Dispatch's `CustomizeHandlerDiscovery(d => d.Includes.WithNameSuffix("Automation"))` **replace** the default conventions? | **Premise contradicted — it is additive.** `specifyConventionalHandlerDiscovery()` runs inside `FindCalls()` at bootstrap, *after* user customization (`HandlerDiscovery.cs:87-95, 205`), and `CompositeFilter.Matches` OR's the includes. **Name the consumer `DriverLocationUpdatedHandler`; no discovery change, no rename to `*Automation`.** Requirements that still bind: public, concrete, closed-generic, method named `Handle`/`Handles`/`Consume`/`Consumes`. | +| 5 | How does a test deterministically wait for a Kafka message to be consumed? | **Premise partly contradicted.** `IncludeExternalTransports()` exists but governs *outgoing* tracking, not arrivals. The waiter is **`WaitForMessageToBeReceivedAt(IHost)`** (`TrackedSessionConfiguration.cs:183`). For a test that produces with a raw `ProducerBuilder`, drop `IncludeExternalTransports()` entirely — pattern at `Wolverine.Kafka.Tests/publish_and_receive_raw_json.cs:87-98`. Note `TrackedSession.IsCompleted()` short-circuits on the first satisfied condition, so you cannot assert "nothing else happened" from the same session. | +| 6 | Marten indexing + `Contains` translation for the k-ring query. | `Duplicate(...)` (real column + index) and `Index(...)` (computed JSONB index) are **different APIs, not synonyms** — use `Duplicate`. `.Where(d => cells.Contains(d.H3Cell))` translates to **`= ANY(:param)`** — a *single array parameter*, not an N-term `IN` list (`IsOneOf.cs:65-71`). Enums are special-cased and also translate. `Duplicate` calls chain safely (`Alter`'s setter appends). | +| 7 | Is read-then-conditionally-`Store` the best LWW upsert available? | **Premise contradicted — no.** `session.TryUpdateRevision(doc, revision)` (`IDocumentOperations.cs:125`) is an **atomic single-statement upsert** whose `where … mt_version < ?` guard makes a stale write a silent no-op, and it handles the document-absent case. **Two traps:** (a) the guard silently does nothing unless the document is registered `UseNumericRevisions(true)`; (b) `mt_version` is a monotonic `long`, **not a timestamp** — project `serverReceivedAt` to unix-ms yourself, and note revision `0` means "always win". | +| 8 | pocketken.H3 4.5.0.1 k-ring surface (probed by reflection, not a JasperFx gate). | `H3.Algorithms.Rings.GetKRing(H3Index origin, int k)` → `IEnumerable`. `H3Index.GetHexagonEdgeLengthAverageInM(int resolution)` → `double`, static. `new H3Index(string)` parses a cell id back. `LatLng.FromCoordinate(Coordinate)` + `GetGreatCircleDistanceInMeters(LatLng)` for the exact-distance filter. **The `new LatLng(lat, lon)` ctor takes RADIANS** — same footgun `H3CellIndexer` documents, so go through `FromCoordinate(new Coordinate(lon, lat))` and stay degrees-native. | + +**Gate 2 + gate 4 interact, and the interaction is the sharpest edge in this session.** `HandlerPipeline` resolves the wire name through `HandlerGraph._messageTypes`, which is populated **only from discovered handler chains** (`HandlerGraph.cs:530`). If the handler is not discovered, the message is never deserialized at all — it short-circuits to `NoHandlerContinuation` and the failure looks like silence, not an error. **Handler discovery is a precondition for deserialization, not just for dispatch.** A test that sees no document and no exception should suspect discovery first. + +--- + +## Skill corrections (`critter-skill-auditor` Phase 1, 2026-07-24) + +The Phase 1 pass corrected this prompt after the source-verification pass had already closed all eight gates — the second consecutive session where convention discovery caught what source verification structurally could not. Applied above; recorded here so the reasoning survives. + +- **`wolverine-marten-automation` does not govern this session, and naming it was a category error.** Its own scope statement (`SKILL.md:12-22`) restricts it to handlers reacting to a domain event *forwarded from a Marten stream*. Neither new handler is that shape — nothing in `AvailableDrivers/` touches `UseFastEventForwarding`. `wolverine-kafka` says so itself at line 14: *"Kafka is a transport wire, not a handler shape… a vanilla Wolverine messaging handler,"* and its § Prerequisites defers to `wolverine-messaging-handlers`. **Replaced with `wolverine-handlers` (base shape) + `wolverine-messaging-handlers` (inbound-message shape and idempotency).** +- **`marten-querying` was missing.** `NearbyAvailableDriversView` is a read-side query adapter, squarely its charter. It also settles a mechanical point: **inject `IQuerySession` in the view** (pure read) and `IDocumentSession` in the two handlers (they write). +- **`protobuf-contracts:418` names an anti-pattern whose *literal wording* this session's csproj wiring resembles** — "defining a shared type inside a service's package" and importing it cross-BC. This is **not** that violation: a producer's published *event contract* is a different relationship from an arbitrarily borrowed *shared value type*, and gate 3 establishes that the mechanics hold. But the skill's text does not draw that distinction anywhere, so **the csproj comment must name the tension explicitly** rather than leave a future reader to hit it cold. +- **Handler naming is confirmed on ubiquitous-language grounds, not just mechanical ones.** W001 §5.2 pins "Automation" to handlers reacting to a domain event already on a Marten stream. A Kafka-triggered handler is not that shape in Cab's vocabulary, so `*Handler` is correct — matching `csharp-coding-standards:710`. +- **`AvailableDrivers/` as a new folder is sanctioned**, grounded in ADR-018:36 ("a Dispatch-owned local projection") and `vertical-slice-organization`'s "what does this folder do?" test: maintaining the view and deciding candidates are different capabilities. + +--- + +## Decisions resolved (user sign-off 2026-07-24) + +### Fork 1 — the availability half: exclude, with an unbound handler + +W006 §6.5's `AvailableDriver` joins a Kafka location side with an ASB availability side, but ASB is a forward-constraint to an un-workshopped Driver Profile BC — and `CandidateSelectionAutomation` filters on `VehicleClass`, which lives on the availability side. + +**Resolved:** the Kafka handler writes the **location side only**. A `DriverAvailabilityChangedHandler` ships with **no transport bound to it** — tests drive it in-process via `IMessageBus`. **A driver with a location but no availability side is excluded from selection.** + +*Why:* you cannot dispatch to a driver whose capability you do not know. Optimistic defaults (`Available` + `STANDARD`) would keep a demo alive by fabricating a capability claim Dispatch has no source for — and fabricating it *invisibly*, at the point of query. The accepted cost is that a real end-to-end run yields `NoCandidatesAvailable` until Driver Profile ships. **That is honest rather than broken:** ADR-018 says the view needs both feeders, and with one feeder it correctly reports that it cannot dispatch. The cross-service Kafka flow is still real and observable in the document store, which is what this PR exists to prove. + +**Scope guard on the placeholder message.** `DriverAvailabilityChanged` is Dispatch-local and carries exactly the four availability-side fields W006 §6.5 already locks (`driverId`, `availabilityState`, `vehicleClass`, `availabilityUpdatedAt`) — **no more**. It is not a proto, it is not a published contract, and it does **not** pre-empt Driver Profile's workshop, which will dictate the real shape (W001 §5.3 anticipates four distinct events: `DriverCameOnline` / `WentOnBreak` / `WentOffline` / `VehicleChanged`). Mirroring §6.5's document columns 1:1 is the boundary; inventing transitions beyond them is not. + +### Fork 2 — H3 query resolution: track last-received + +Dispatch must compute the pickup's cell at the **same resolution Telemetry used**, and that resolution is a `TelemetryPolicy` value Dispatch does not own. + +**Resolved:** store `h3Resolution` on each `AvailableDriver` document and derive the query resolution from the **most recently ingested** document. If the store is empty, short-circuit to an empty result rather than guessing a resolution. + +*Why:* the alternative — a Dispatch-side constant that must match Telemetry's — is a silent cross-service coupling with no enforcement, where a Telemetry policy change breaks Dispatch's queries with no error, just an empty candidate set that reads as "no drivers nearby." Deriving it from the stream keeps the resolution flowing with the data that depends on it. A `TelemetryPolicy` roll produces a brief window where mixed-resolution documents are missed, **self-healed within one heartbeat — the same accepted-v1-staleness argument W006 §6.3 and §6.4 already make** for dropped publishes and for eviction. Union-across-resolutions was rejected as more code and an extra hot-path query to close a window the spec elsewhere accepts. + +**Note the ambiguity trap:** resolution `0` is a *valid* H3 resolution, so a `FirstOrDefault()` returning `0` cannot be distinguished from "no documents." Branch on document existence, not on the resolution value. + +### Fork 3 — `EtaSeconds`: derived from distance, invented constant named as such + +`INearbyAvailableDriversSource` returns `NearbyDriver(DriverId, DistanceMeters, EtaSeconds, VehicleClass)`, and a location+availability view has no ETA source. W001 §5.3 locks match-score as inverse straight-line distance for v1 and defers road-network ETA to a future gRPC counterparty. + +**Resolved:** `EtaSeconds = distanceMeters / assumedUrbanSpeed`, with the speed as a **single named constant documented as invented at implementation time** — the same honest treatment PR #45 gave its 100 m accuracy threshold. + +*Why:* a `0` sentinel is a lie of a different kind (W001 §5.4's offer broadcast would show riders a 0-second ETA with nothing marking it absent rather than instant), and making the field nullable would touch `CandidatesSelected`'s locked event table — a spec amendment beyond this slice, which already carries two. The constant keeps the value meaningful and keeps the seam ready for a real ETA service to replace one method. + +--- + +## Deliverable plan + +### 1. Project wiring + +- **`Directory.Packages.props`** — expected **unchanged**; every needed `PackageVersion` (`WolverineFx.Kafka`, `WolverineFx.Protobuf`, `Google.Protobuf`, `Grpc.Tools`, `pocketken.H3`, `Confluent.Kafka`) already exists from the Telemetry chain. Verify rather than assume. +- **`src/CritterCab.Dispatch/CritterCab.Dispatch.csproj`** — add `WolverineFx.Kafka`, `WolverineFx.Protobuf`, `Google.Protobuf`, `Grpc.Tools`, `pocketken.H3`, and a ``. **Comment why this is not a shared assembly**: both services compile the same contract file independently, which is exactly what ADR-009 means by the proto being the artifact of record, and gate 3 is why Wolverine tolerates the two copies. **The same comment must address `protobuf-contracts:418`** — a producer's published event contract subscribed to by a consumer is a different relationship from a shared value type borrowed across packages, and the skill does not currently distinguish them. + +### 2. New slice folder — `src/CritterCab.Dispatch/AvailableDrivers/` + +A new folder rather than an addition to `CandidateSelection/`: this is W006 slice 5 with its own inbound transport, while `CandidateSelection/` is the W001 slice 5.3 that *consumes* it. The port (`INearbyAvailableDriversSource`) stays with its consumer; this folder supplies the adapter. + +- **`AvailableDriver.cs`** — the per-driver document. Location side (`H3Cell`, `Lat`, `Lon`, `H3Resolution`, `ServerReceivedAt`) marked **`required`**; availability side (`AvailabilityState`, `VehicleClass`, `AvailabilityUpdatedAt`) plainly **nullable, not `required`** — absence is meaningful per fork 1, which is exactly the "genuinely optional by business logic" case `csharp-coding-standards:461-463` sanctions. Plus `Version` for numeric revisions. +- **`DriverLocationUpdatedHandler.cs`** — the Kafka consumer, taking `IDocumentSession`. Name fixed by gate 4. Reads the generated protobuf message, projects `ServerReceivedAt` to unix-ms, calls `TryUpdateRevision`. **Must not overwrite the availability side** — read-modify-write or a targeted update, not a whole-document replace. +- **`DriverAvailabilityChanged.cs` + `DriverAvailabilityChangedHandler.cs`** — the unbound ASB-half landing site, scoped per fork 1. `sealed record` with `required` properties per `csharp-coding-standards:116` (the DTO shape rule is project-wide, not event-sourcing-scoped). Mirror-image constraint: **must not overwrite the location side.** +- **`NearbyAvailableDriversView.cs`** — implements `INearbyAvailableDriversSource`, taking **`IQuerySession`** (pure read). Resolves the query resolution (fork 2), computes the pickup cell, derives `k` from `searchRadiusMeters`, queries by `= ANY` over the k-ring cells filtered to available + capable, applies the exact-distance filter, derives ETA (fork 3), returns `NearbyDriver` records. +- **`H3KRing.cs`** — wraps the k-derivation, the ring enumeration, and the great-circle distance. **Wrap the footguns the way `H3CellIndexer` does, and say why in a comment**: the degrees-vs-radians and lat/lon-vs-x/y hazards are identical here. Derive `k` as `ceil(radiusMeters / (edgeLengthM × √3))` and **round up generously** — the exact-distance filter runs afterward, so over-covering costs only a longer array while under-covering silently drops drivers. + +### 3. `src/CritterCab.Dispatch/Program.cs` + +- Marten: register `AvailableDriver` with `Duplicate(x => x.H3Cell)`, `Duplicate(x => x.VehicleClass)`, `Duplicate(x => x.AvailabilityState)` and **`UseNumericRevisions(true)`** (gate 7 trap (a) — without it `TryUpdateRevision` degrades silently to an unguarded upsert). +- Kafka: `UseKafkaUsingNamedConnection("kafka")` + `ListenToKafkaTopic("telemetry.driver-location-updated").ConfigureConsumer(c => c.GroupId = ...).UseProtobufSerialization().DefaultIncomingMessage()`. **Guard on the connection string** exactly the way Telemetry's `kafkaEnabled` flag does — one flag read once, used by both registration and transport wiring, so the two cannot drift. Remember `ConfigureConsumer` **replaces** the parent `ConsumerConfig`: bootstrap servers are inherited, other settings are not. +- Swap `INearbyAvailableDriversSource`: register `NearbyAvailableDriversView` when Marten is configured, `NearbyAvailableDriversStub` when it is not. **The stub is demoted to a test double, not deleted** — existing slice 5.2/5.3 suites override the registration through the fixture and must keep passing untouched. +- **`opts.Durability.UseSyncRetryBlock` is Telemetry-only and must not be copied here.** It exists to make §6.3's publish-first ordering real on the *producer*; a listener does not inherit it and does not need it. + +### 4. `apphost.cs` + +Add `.WithReference(kafka).WaitFor(kafka)` to the Dispatch resource. Do **not** add a second `AddKafka` — the broker exists at host port 5392. **Run `dotnet build apphost.cs` by hand**; CI cannot see this file, and it has broken silently twice. + +### 5. Tests — `tests/CritterCab.Dispatch.Tests/AvailableDrivers/` + +A **new fixture with its own Kafka Testcontainer**, mirroring Telemetry's split so the existing Dispatch suites never wait on a broker. Cover: + +- **Location upsert** (W006 §6.5 GWT 1) — a published `DriverLocationUpdated` lands as an `AvailableDriver` location side. +- **Dedup** (W006 §6.3 GWT 2) — **the GWT PR C could not test.** Redeliver the same `(driverId, serverReceivedAt)`; assert the projection applies it at most once. Then send a *stale* update and assert it is silently discarded, which is the redelivery-after-rebalance case that motivates the revision guard. +- **Availability** (§6.5 GWT 2) — the unbound handler invoked in-process sets the availability side without disturbing the location side. +- **Selection read** (§6.5 GWT 3) — k-ring + exact-distance + vehicle-class filtering returns the right drivers in the right order. +- **Fork-1 exclusion** — a driver with a location but no availability side is not selected. +- **`H3KRing` unit tests** — pinning tests in the spirit of `H3CellIndexerTests`, locking the axis order, the units, and the k-derivation at a known resolution. + +Use `WaitForMessageToBeReceivedAt(host)` per gate 5. **No `Thread.Sleep`.** + +### 6. Documentation + +- **`docs/workshops/006-telemetry-event-model.md`** — §6.5 Document History entry: slice 5 realized; the three fork resolutions recorded as **amendments** (§6.5 was silent, not wrong); the slice walk closes. +- **`docs/workshops/001-dispatch-event-model.md`** — §5.3 realization note under the existing 2026-06-30 amendment: the transport now runs, the view exists, the stub is out of the production graph. +- **`docs/skills/wolverine-kafka/SKILL.md`** — **rewrite § Listening, § Consumer groups, § Batch processing and § DLQ from this session's shipped code.** This is the deferred DEBT row, and PR D is the session that can finally do it honestly: the examples were left wrong in PR C because replacing speculative names with *different* speculative names would have been no better. Fold in the gate 1/2/4/5 findings — especially that discovery is a precondition for deserialization. +- **`docs/skills/DEBT.md`** — close that row; register anything new this session surfaces. +- **`docs/prompts/README.md`** — index entry. +- **Retro** at `docs/retrospectives/implementations/009-...`, shipping inside this PR. + +--- + +## Out of scope + +- **The ASB half of ADR-018.** No Azure Service Bus transport, no Driver Profile modeling, no availability events beyond the four fields fork 1 permits. +- **`CandidateSelectionAutomation.cs`** — must not appear in the diff. +- **Deleting `NearbyAvailableDriversStub`** — it is demoted, not removed. +- **The three decision-class DEBT rows** (test-class naming, `testing-integration` Gap B, `identity-acl` streaming exception). Each needs a call the user has not made; a routine tidy must not drain them. +- **The CI-cannot-build-`apphost.cs` gap.** Extending the "Verify solution completeness" step is its own session — CI changes carry their own blast radius. +- **`transport-selection`'s built-vs-modeled status axis** — a separate DEBT row. +- **CritterWatch.** It becomes genuinely useful *after* this PR creates real cross-service traffic, but it needs RabbitMQ and its trial licence expired 2026-07-10. Revisit as its own session. +- **Road-network ETA, the v2 staleness ceiling, adaptive radius widening (W001 slice 9), `DispatchPolicyConfigured` (slice 11).** + +--- + +## Follow-on PR sequence (arc context; not this session) + +**PR D closes W006.** All five slices realized; two of ADR-005's three transports live; the first cross-service flow running. + +**The next session must be a design-or-tidy return.** Per ADR-004's design-return cadence, PR D is the fourth consecutive implementation PR in this chain — ADR-019 served as the interleave for PR C, and nothing covers PR D. Candidates, in rough order of pull: the **Driver Profile workshop** (which the ASB half now blocks on, and which fork 1 deliberately declined to pre-empt), a **`tidy: skills` session** draining the accumulated DEBT rows including the three that need decisions, or **CritterWatch** now that there is real traffic for it to render. + +--- + +## Document history + +- **2026-07-24** — Authored. Eight Verify-before-wiring gates closed at authoring time (second consecutive prompt); three gate premises contradicted by source (listener message-type inference, handler-discovery replacement, read-then-`Store` as the best LWW upsert). Three durable forks resolved by user sign-off: availability-half exclusion with an unbound handler, last-received H3 resolution tracking, distance-derived ETA with a named invented constant. +- **2026-07-24** — `critter-skill-auditor` Phase 1 corrections applied (see § Skill corrections): `wolverine-marten-automation` removed as a category error and replaced with `wolverine-handlers` + `wolverine-messaging-handlers`; `marten-querying` added along with the `IQuerySession`/`IDocumentSession` split; the `protobuf-contracts:418` wording tension made an explicit deliverable; `LastKnownPosition/` added as orientation reading. **Carry to the retro:** the existing DEBT row for "no skill covers a non-event-sourced document write path inside an event-sourced service" gets its second and sharper instance here — Telemetry is stream-processing throughout, whereas Dispatch is the project's canonical event-sourced BC now carrying a plain-document LWW view alongside its aggregates. Also note `DriverAvailabilityChanged` is a genuine hybrid with no governing skill (event-shaped, no stream, no transport) — a skill-authoring candidate if the pattern recurs when Driver Profile's real events land. From e9677fb8955f434c162998a8c9c23c0bb25a9b87 Mon Sep 17 00:00:00 2001 From: Erik Shafer Date: Fri, 24 Jul 2026 23:54:57 -0500 Subject: [PATCH 2/8] Dispatch consumes telemetry.driver-location-updated into AvailableDriver (W006 slice 5) --- apphost.cs | 11 +- .../AvailableDrivers/AvailableDriver.cs | 93 ++++++++ .../DriverAvailabilityChanged.cs | 76 +++++++ .../DriverLocationUpdatedHandler.cs | 82 ++++++++ .../AvailableDrivers/H3KRing.cs | 123 +++++++++++ .../NearbyAvailableDriversView.cs | 103 +++++++++ .../CritterCab.Dispatch.csproj | 37 ++++ src/CritterCab.Dispatch/Program.cs | 119 ++++++++++- .../AvailableDrivers/H3KRingTests.cs | 114 ++++++++++ .../Slice5DriverLocationConsumerTests.cs | 173 +++++++++++++++ .../Slice5NearbyAvailableDriversViewTests.cs | 178 ++++++++++++++++ .../CritterCab.Dispatch.Tests.csproj | 5 + .../DispatchKafkaTestFixture.cs | 199 ++++++++++++++++++ 13 files changed, 1310 insertions(+), 3 deletions(-) create mode 100644 src/CritterCab.Dispatch/AvailableDrivers/AvailableDriver.cs create mode 100644 src/CritterCab.Dispatch/AvailableDrivers/DriverAvailabilityChanged.cs create mode 100644 src/CritterCab.Dispatch/AvailableDrivers/DriverLocationUpdatedHandler.cs create mode 100644 src/CritterCab.Dispatch/AvailableDrivers/H3KRing.cs create mode 100644 src/CritterCab.Dispatch/AvailableDrivers/NearbyAvailableDriversView.cs create mode 100644 tests/CritterCab.Dispatch.Tests/AvailableDrivers/H3KRingTests.cs create mode 100644 tests/CritterCab.Dispatch.Tests/AvailableDrivers/Slice5DriverLocationConsumerTests.cs create mode 100644 tests/CritterCab.Dispatch.Tests/AvailableDrivers/Slice5NearbyAvailableDriversViewTests.cs create mode 100644 tests/CritterCab.Dispatch.Tests/DispatchKafkaTestFixture.cs diff --git a/apphost.cs b/apphost.cs index 5165a0b..5fc91dd 100644 --- a/apphost.cs +++ b/apphost.cs @@ -48,11 +48,20 @@ // launch profile — the AppHost-declared endpoints are authoritative. gRPC rides // the HTTPS endpoint via Kestrel HTTP/2; 5312 is reserved if a dedicated gRPC // listener is ever needed. See docs/skills/aspire/SKILL.md § Port allocation. +// +// Dispatch LISTENS to telemetry.driver-location-updated (W006 §6.5) — its first transport, and the +// second half of CritterCab's first cross-service flow. Same named-connection arrangement as +// Telemetry below: the reference injects the broker address under the key "kafka", which is what +// UseKafkaUsingNamedConnection reads. Unlike Telemetry, Dispatch does NOT AutoProvision — the +// producer owns the topic. WaitFor is therefore ordering hygiene here rather than a hard +// requirement: a listener that starts before the broker retries, it does not fail. builder.AddProject("dispatch", launchProfileName: null) .WithHttpsEndpoint(port: 5310, name: "https") .WithHttpEndpoint(port: 5311, name: "http") .WithReference(dispatchDb) - .WaitFor(dispatchDb); + .WaitFor(dispatchDb) + .WithReference(kafka) + .WaitFor(kafka); // 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 diff --git a/src/CritterCab.Dispatch/AvailableDrivers/AvailableDriver.cs b/src/CritterCab.Dispatch/AvailableDrivers/AvailableDriver.cs new file mode 100644 index 0000000..4fdfd5e --- /dev/null +++ b/src/CritterCab.Dispatch/AvailableDrivers/AvailableDriver.cs @@ -0,0 +1,93 @@ +using JasperFx; + +namespace CritterCab.Dispatch.AvailableDrivers; + +// Dispatch's local view of who is out there and dispatchable (W006 §6.5, ADR-018 consumer half). +// +// A plain Marten document, NOT an event projection — and that is the load-bearing decision of this +// slice. Dispatch is CritterCab's canonical event-sourced BC: RideRequest is an aggregate, the +// timeline and rounds are projections, the whole decider apparatus is here. This document sits +// alongside all of that and is none of it. Event-sourcing the inbound location stream would +// reimport onto Dispatch's event store exactly the per-ping volume Telemetry's throttle exists to +// suppress. So Dispatch consumes the telemetry stream the same shape Telemetry produces it: +// document upsert, last-writer-wins, no history. The stream-processing shape crosses the BC +// boundary along with the data, by necessity. +// +// TWO SIDES, TWO WRITERS, TWO CLOCKS. The location side is fed by Kafka (DriverLocationUpdated, +// W006 slice 3); the availability side is fed by Azure Service Bus from Driver Profile — a BC that +// has not been workshopped, so that half is a forward-constraint (ADR-018). Each side is upserted +// independently by its own handler and neither may clobber the other. They are joined only by +// DriverId, which is why this is a document and not two. +public sealed record AvailableDriver : ILongVersioned +{ + // driverId. Marten takes `Id` as document identity by convention, so "one document per driver" + // falls out of the shape. An external key that arrives on the wire, never minted here — hence + // no Guid.CreateVersion7() (contrast the event rows in RideRequesting/). + public required Guid Id { get; init; } + + // === Location side — written by DriverLocationUpdatedHandler, from Kafka === + + // The H3 index at H3Resolution, as published by Telemetry. Published language: both BCs agree + // on the cell system, and the cell id travels from Telemetry's H3CellIndexer to this column + // unchanged. Duplicated to a real column in Program.cs so the k-ring `= ANY` query is indexed. + public required string H3Cell { get; init; } + + // Carried alongside the cell rather than bit-decoded from it. Dispatch does not own the + // TelemetryPolicy that sets this, so it cannot assume a value — it reads whatever Telemetry + // published. NearbyAvailableDriversView derives the query resolution from the most recently + // ingested document precisely because this field exists (W006 §6.5, fork 2). + public required int H3Resolution { get; init; } + + public required double Lat { get; init; } + + // `Lon`, not `Lng` — driver_location_updated.proto is the contract and it says `lon` (ADR-009). + // Matches Telemetry's LastKnownPosition for the same reason. + public required double Lon { get; init; } + + // The location side's LWW / dedup key (W006 §6.3 R7). Server-stamped by Telemetry and + // monotonic per driver. Stored for readability and assertions; the actual staleness guard runs + // in the database off Marten's numeric revision, which carries this same instant as unix-ms. + public required DateTimeOffset ServerReceivedAt { get; init; } + + // === Availability side — written by DriverAvailabilityChangedHandler, from ASB (unbuilt) === + // + // Nullable rather than `required`, and the nullability is meaningful rather than incidental: + // null means "Dispatch has never heard from Driver Profile about this driver", which is the + // steady state until that BC ships. NearbyAvailableDriversView EXCLUDES such drivers — you + // cannot dispatch to a driver whose capability you do not know (W006 §6.5, fork 1). Defaulting + // these to Available/Standard would keep a demo alive by fabricating a capability claim + // Dispatch has no source for, invisibly, at the point of query. + + public DriverAvailabilityState? AvailabilityState { get; init; } + + public VehicleClass? VehicleClass { get; init; } + + // The availability side's own LWW key. Deliberately NOT the same clock as ServerReceivedAt: + // the two sides arrive over different transports from different services and are ordered + // independently. W006 §6.5: "LWW per driver per side." + public DateTimeOffset? AvailabilityUpdatedAt { get; init; } + + // Marten's numeric revision, guarding both sides against stale redelivery. Registered with + // UseNumericRevisions(true) in Program.cs — without that registration TryUpdateRevision + // silently degrades to an unguarded upsert, which is the failure mode this field exists to + // prevent. See DriverLocationUpdatedHandler for how the revision is derived. + // + // ILongVersioned, NOT IRevisioned, and the difference is not cosmetic. IRevisioned is `int + // Version` and backs an `integer` mt_version column; ILongVersioned is `long` and backs + // `bigint`. The revision this slice stores is a unix-MILLISECOND timestamp (~1.7e12), which + // overflowed int in 1970 — so IRevisioned would have silently truncated every guard value and + // made the LWW comparison meaningless. The interface choice is what makes a timestamp usable + // as a revision at all. + public long Version { get; set; } +} + +// Dispatch's own enum, not a shared one. Driver Profile will have its own availability vocabulary +// and the ASB translation handler maps into this at the boundary — BCs own their enums (the same +// rule VehicleClass follows). The three states are what W001 §5.3's four anticipated Driver Profile +// events collapse to from Dispatch's point of view: only Available is dispatchable. +public enum DriverAvailabilityState +{ + Available, + OnBreak, + Offline +} diff --git a/src/CritterCab.Dispatch/AvailableDrivers/DriverAvailabilityChanged.cs b/src/CritterCab.Dispatch/AvailableDrivers/DriverAvailabilityChanged.cs new file mode 100644 index 0000000..df875aa --- /dev/null +++ b/src/CritterCab.Dispatch/AvailableDrivers/DriverAvailabilityChanged.cs @@ -0,0 +1,76 @@ +using Marten; + +namespace CritterCab.Dispatch.AvailableDrivers; + +// The ASB half of ADR-018's join — as a landing site, not as a transport. +// +// READ THIS BEFORE EXTENDING IT. Driver Profile has not been workshopped. It owns the real +// availability vocabulary, and W001 §5.3 anticipates FOUR distinct events from it (DriverCameOnline, +// DriverWentOnBreak, DriverWentOffline, DriverVehicleChanged). This type is deliberately none of +// those. It carries exactly the four availability-side fields W006 §6.5 already locks on the +// AvailableDriver document and nothing more, so it describes the SHAPE OF THE HOLE rather than +// guessing at the vocabulary that will fill it. When Driver Profile ships, its published events +// replace this type and this handler becomes their translation target; inventing transitions here +// now would pre-empt that workshop and make the replacement a migration instead of a swap. +// +// It is a Dispatch-local message: not a proto, not a published contract, not registered as a Marten +// event type. Nothing publishes it and NO TRANSPORT IS BOUND TO IT. Until ASB is wired, the only +// callers are tests driving it in-process through IMessageBus — which is enough to prove the join +// and the exclusion rule work, without pretending the feeder exists. +public sealed record DriverAvailabilityChanged +{ + public required Guid DriverId { get; init; } + + public required DriverAvailabilityState AvailabilityState { get; init; } + + // The driver's in-service vehicle class at the moment of the transition. W001 §5.3 locks + // one vehicle in service at a time for v1, and each Driver Profile transition carries the + // capability — which is why capability rides on the availability side rather than being a + // fifth field of its own. + public required VehicleClass VehicleClass { get; init; } + + // The availability side's own LWW key. A separate clock from the location side's + // ServerReceivedAt: different service, different transport, independently ordered. + public required DateTimeOffset AvailabilityUpdatedAt { get; init; } +} + +// Mirror image of DriverLocationUpdatedHandler: writes the availability side and must leave the +// location side untouched. +public static class DriverAvailabilityChangedHandler +{ + public static async Task Handle( + DriverAvailabilityChanged message, + IDocumentSession session, + CancellationToken ct) + { + var existing = await session.LoadAsync(message.DriverId, ct); + + // An availability event can legitimately arrive before Dispatch has ever seen a position + // for this driver — a driver who comes on shift indoors, or whose first ping is still in + // Telemetry's throttle window. There is no location side to write yet, and inventing one + // (0,0 is in the Gulf of Guinea) would put a real driver in a real k-ring somewhere. Drop + // it: Telemetry publishes on the driver's first cell change or heartbeat regardless, so + // the document appears within one heartbeat interval, carrying this state if it arrives + // first. Consistent with W006's heartbeat-as-backstop reasoning throughout. + if (existing is null) + return; + + var updated = existing with + { + AvailabilityState = message.AvailabilityState, + VehicleClass = message.VehicleClass, + AvailabilityUpdatedAt = message.AvailabilityUpdatedAt + }; + + // Same database-side LWW guard as the location side, on this side's own clock. Note both + // sides share ONE revision column, so an availability update and a location update compete + // for it. That is acceptable while the availability feed is unbuilt and low-rate, and it is + // the first thing to revisit when ASB lands: if the two clocks interleave under load, the + // sides need independent guards rather than one shared revision. + var revision = Math.Max(1, message.AvailabilityUpdatedAt.ToUnixTimeMilliseconds()); + + session.TryUpdateRevision(updated, revision); + + await session.SaveChangesAsync(ct); + } +} diff --git a/src/CritterCab.Dispatch/AvailableDrivers/DriverLocationUpdatedHandler.cs b/src/CritterCab.Dispatch/AvailableDrivers/DriverLocationUpdatedHandler.cs new file mode 100644 index 0000000..86f7705 --- /dev/null +++ b/src/CritterCab.Dispatch/AvailableDrivers/DriverLocationUpdatedHandler.cs @@ -0,0 +1,82 @@ +using CritterCab.Telemetry.V1; +using Marten; + +namespace CritterCab.Dispatch.AvailableDrivers; + +// CritterCab's first cross-service consumer (W006 §6.5, ADR-018 consumer half). A ping that entered +// Telemetry over gRPC, survived the throttle, and was published to telemetry.driver-location-updated +// lands here as a row in Dispatch's document store. +// +// A vanilla Wolverine message handler — Kafka is a transport wire, not a handler shape. Nothing +// about the topic appears in this file; the binding lives entirely in Program.cs. +// +// NOT named *Automation, and the reason is vocabulary rather than mechanics. W001 §5.2 pins +// "Automation" to handlers reacting to a domain event already on a Marten stream (FareQuoteAutomation +// off RideRequested). This reacts to an inbound message from another service, which is a different +// shape in Cab's language. Mechanically either name would be discovered: Wolverine's conventional +// discovery is ADDITIVE, so Program.cs's CustomizeHandlerDiscovery(WithNameSuffix("Automation")) +// widens the default set rather than replacing it, and the built-in "Handler" suffix still applies. +// +// Worth knowing when this goes quiet: handler discovery is a precondition for DESERIALIZATION, not +// just for dispatch. Wolverine resolves the wire type name through HandlerGraph._messageTypes, which +// is populated from discovered handler chains. If this class stops being discovered — made internal, +// renamed, moved out of the scanned assembly — the message is never deserialized and the failure +// presents as silence, not as an error. +public static class DriverLocationUpdatedHandler +{ + public static async Task Handle( + DriverLocationUpdated message, + IDocumentSession session, + CancellationToken ct) + { + var driverId = Guid.Parse(message.DriverId); + var serverReceivedAt = message.ServerReceivedAt.ToDateTimeOffset(); + + // Read the existing document to preserve the availability side. The two sides have separate + // writers and separate clocks (W006 §6.5), so a blind whole-document write here would erase + // whatever Driver Profile last told us about this driver. Null on first sight is the normal + // case, not an error — a driver's first published position precedes any availability event + // roughly as often as it follows one. + var existing = await session.LoadAsync(driverId, ct); + + var updated = new AvailableDriver + { + Id = driverId, + H3Cell = message.H3Cell, + H3Resolution = message.H3Resolution, + Lat = message.Lat, + Lon = message.Lon, + ServerReceivedAt = serverReceivedAt, + + // Carried forward untouched. See above. + AvailabilityState = existing?.AvailabilityState, + VehicleClass = existing?.VehicleClass, + AvailabilityUpdatedAt = existing?.AvailabilityUpdatedAt + }; + + // The dedup/LWW guard W006 §6.3 asks for, enforced in the database rather than in this + // method. TryUpdateRevision emits a single upsert whose WHERE clause compares the stored + // revision, so a stale or duplicate delivery is discarded server-side and silently — no + // exception, no second round trip, and no window between a read and a write for a + // concurrent delivery to slip through. + // + // Why this matters despite the partition key: Kafka partitions by driverId, so per-driver + // ordering IS guaranteed in steady state and a naive Store() would usually be fine. It is + // redelivery that breaks the assumption — on a consumer-group rebalance, uncommitted offsets + // replay, so an OLDER position can arrive after a newer one was already applied. That is a + // real regression of the view, and the guard makes it free to prevent. + // + // The revision is unix-MILLISECONDS of the server-stamped receipt time, because Marten's + // revision is a monotonic long and not a timestamp. Two consequences: + // - Equal timestamps are a no-op, which is exactly the dedup semantics §6.3 specifies + // ("the projection applies the position at most once"). + // - Revision 0 means "always win" to Marten, so a zero-valued timestamp would defeat the + // guard entirely. Unreachable in practice (Telemetry server-stamps every publish), but + // the floor costs one Math.Max and removes the failure mode. + var revision = Math.Max(1, serverReceivedAt.ToUnixTimeMilliseconds()); + + session.TryUpdateRevision(updated, revision); + + await session.SaveChangesAsync(ct); + } +} diff --git a/src/CritterCab.Dispatch/AvailableDrivers/H3KRing.cs b/src/CritterCab.Dispatch/AvailableDrivers/H3KRing.cs new file mode 100644 index 0000000..d97312f --- /dev/null +++ b/src/CritterCab.Dispatch/AvailableDrivers/H3KRing.cs @@ -0,0 +1,123 @@ +using H3; +using H3.Algorithms; +using H3.Extensions; +using H3.Model; +using NetTopologySuite.Geometries; + +namespace CritterCab.Dispatch.AvailableDrivers; + +// The geospatial half of W006 §6.5's radius query: "an H3 k-ring around the pickup cell + exact +// distance filter". Wrapped rather than inlined for the same reason Telemetry wraps H3CellIndexer +// — the binding has footguns on two axes and they are mirror opposites depending on the API path: +// +// H3.Model.LatLng is (lat, lon) in RADIANS +// NetTopologySuite.Coordinate is (lon, lat) = (X, Y) in DEGREES +// +// Everything Dispatch holds is degrees (the proto's lat/lon, Location.Lat/Lon), so every path here +// goes through Coordinate and LatLng.FromCoordinate, which is degrees-native. That removes the unit +// conversion entirely and collapses the risk to argument ORDER, which H3KRingTests pins. +// +// DO NOT reach for `new LatLng(lat, lon)`. That constructor takes radians, and passing degrees is a +// ~57x scale error that still produces a plausible-looking distance rather than an exception. +public static class H3KRing +{ + // Metres covered per ring, as a multiple of the edge length. This is 1.5, and the derivation + // matters because the obvious answer is wrong in the dangerous direction. + // + // Centre-to-centre spacing between adjacent hexagons is edge x sqrt(3) — but that is the + // distance gained per hop only when travelling ALONG a lattice axis. The axes are 60 degrees + // apart, so a bearing that falls between two of them advances less per hop, in the worst case + // by a factor of cos(30) = sqrt(3)/2. Dividing a radius by the full axis spacing therefore + // UNDER-estimates k, and under-estimating k silently drops drivers near the edge of the search + // radius — a wrong dispatch decision with no error anywhere. + // + // worst-case metres per ring = edge x sqrt(3) x sqrt(3)/2 = edge x 1.5 + // + // Sanity check on the numbers, because two different "edge lengths" are in circulation: + // pocketken's GetHexagonEdgeLengthAverageInM returns the edge of a REGULAR hexagon with the + // average cell area (~201m at resolution 9). H3's published tables instead list ~174m, the + // average of the actual distorted edges. Those two differ by exactly this sqrt(3)/2, so + // 201 x 1.5 = 302m reproduces the published figure — and 302m is what the grid empirically + // advances per ring. H3KRingTests pins the resulting k values against measured grid distances. + private const double MetersPerRingPerEdge = 1.5; + + // Computes the H3 cell containing a point, at the given resolution. Same Coordinate path as + // Telemetry's H3CellIndexer, and it must stay the same path: a cell id computed differently + // here would not match the ids Telemetry publishes, and the join would silently return nothing. + 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; + } + + // Every cell id within `radiusMeters` of the origin cell, as a coarse over-approximation. + // + // Deliberately over-covers. k is rounded UP and then widened by one further ring, because the + // two error directions are not symmetric: over-covering costs a longer array in a single + // `= ANY(...)` parameter and is then corrected by the exact-distance filter that runs after, + // while under-covering silently drops drivers who really are in range and produces a wrong + // dispatch decision with no signal. Cheap insurance against boundary arithmetic. + // + // Scale note for anyone tuning this: at resolution 9 a 5km radius yields k=16 and roughly 800 + // cells. That is affordable specifically because Marten translates Contains() to `= ANY(:param)` + // — ONE array parameter — rather than to an N-term IN list. + public static IReadOnlyList CellsWithin(string originCell, int resolution, int radiusMeters) + { + var origin = new H3Index(originCell); + + if (!origin.IsValidCell) + return []; + + var k = DeriveK(resolution, radiusMeters); + + // GridDiskDistances, not the older GetKRing — GetKRing is [Obsolete] as of H3 4.0 and this + // is its named replacement. The per-cell Distance it also returns is the RING index (how + // many hops out), not metres, so it is no substitute for the great-circle filter the caller + // applies afterward; discarded here. + // + // Cell ids are stringified for comparison against the h3_cell column, where they live as + // the same opaque strings Telemetry published. ToString() is nullable-typed, so the null + // filter is a compiler requirement rather than a real case. + return Rings.GridDiskDistances(origin, k) + .Select(ring => ring.Index.ToString()) + .Where(cell => cell is not null) + .Select(cell => cell!) + .ToList(); + } + + // The ring count needed to cover a radius at a given resolution, over-approximated by one ring. + // Internal rather than private so the pinning tests can assert the arithmetic directly instead + // of inferring it from cell counts. + internal static int DeriveK(int resolution, int radiusMeters) + { + if (radiusMeters <= 0) + return 0; + + var edgeMeters = H3Index.GetHexagonEdgeLengthAverageInM(resolution); + + // Guard against a resolution H3 does not know: it returns 0 or NaN rather than throwing, + // and dividing by it would yield an infinite k that GetKRing would try to enumerate. + if (double.IsNaN(edgeMeters) || edgeMeters <= 0) + return 0; + + var metersPerRing = edgeMeters * MetersPerRingPerEdge; + + return (int)Math.Ceiling(radiusMeters / metersPerRing) + 1; + } + + // Great-circle distance in metres between two degree-denominated points. Used for the exact + // filter that trims the k-ring's deliberate over-approximation back to the true radius. + public static double DistanceMeters( + double fromLatDegrees, double fromLonDegrees, + double toLatDegrees, double toLonDegrees) + { + // X = lon, Y = lat — again. FromCoordinate handles the degree-to-radian conversion that the + // LatLng constructor does not. + var from = LatLng.FromCoordinate(new Coordinate(fromLonDegrees, fromLatDegrees)); + var to = LatLng.FromCoordinate(new Coordinate(toLonDegrees, toLatDegrees)); + + return from.GetGreatCircleDistanceInMeters(to); + } +} diff --git a/src/CritterCab.Dispatch/AvailableDrivers/NearbyAvailableDriversView.cs b/src/CritterCab.Dispatch/AvailableDrivers/NearbyAvailableDriversView.cs new file mode 100644 index 0000000..648afc3 --- /dev/null +++ b/src/CritterCab.Dispatch/AvailableDrivers/NearbyAvailableDriversView.cs @@ -0,0 +1,103 @@ +using CritterCab.Dispatch.CandidateSelection; +using Marten; + +namespace CritterCab.Dispatch.AvailableDrivers; + +// The adapter that finally backs slice 5.3's stub seam with real data (W006 §6.5). +// +// This is the payoff for having built INearbyAvailableDriversSource as a port back when there was +// nothing to put behind it: CandidateSelectionAutomation does not change, at all, to go from +// hardcoded drivers to a live cross-service feed. W001 §5.3's "No external call — operates entirely +// on already-available views" also survives intact, and it survives BECAUSE ADR-018 chose the local +// projection over a Telemetry gRPC query. The rejected option would have made that line false. +// +// IQuerySession, not IDocumentSession: this path only reads. The two handlers in this folder write. +public sealed class NearbyAvailableDriversView(IQuerySession session) : INearbyAvailableDriversSource +{ + // Invented at implementation time, and flagged as such rather than buried. + // + // W001 §5.3 locks match-score as inverse straight-line distance for v1 and defers road-network + // ETA to "a future gRPC counterparty" — so the spec gives a DISTANCE but never an ETA, and this + // view has no ETA source. Deriving one from an assumed urban average speed keeps the field + // meaningful and keeps the seam ready: when the ETA service lands, it replaces this one + // expression and no event shape changes. 30 km/h is a common urban-average figure and is a + // placeholder, not a measurement — the same honest treatment PR #45 gave its 100m accuracy + // threshold. Do not tune it as though it were calibrated. + private const double AssumedUrbanSpeedMetersPerSecond = 30_000.0 / 3_600.0; + + public async Task> GetDriversAsync( + Location pickup, + int searchRadiusMeters, + VehicleClass vehicleClassRequired, + CancellationToken ct = default) + { + // The k-ring must be computed at the SAME H3 resolution Telemetry published at, and that + // resolution is a TelemetryPolicy value Dispatch does not own. Rather than hold a constant + // that must silently match another service's config, read it off the stream: the most + // recently ingested document carries the resolution in force when it was published. + // + // The alternative — a Dispatch-side constant — is a cross-service coupling with no + // enforcement, where a Telemetry policy change breaks these queries with no error at all, + // just an empty result that reads as "no drivers nearby". + // + // Note resolution 0 is a VALID H3 resolution, so a FirstOrDefaultAsync returning 0 could + // not be told apart from "no documents". Take(1) into a list and branch on emptiness + // instead — the distinction is the whole point, and a default value cannot carry it. + var resolutions = await session.Query() + .OrderByDescending(d => d.ServerReceivedAt) + .Select(d => d.H3Resolution) + .Take(1) + .ToListAsync(ct); + + // No documents means no drivers, which short-circuits before any H3 work. This is also the + // steady state on a cold start before the first Kafka message arrives. + if (resolutions.Count == 0) + return []; + + var resolution = resolutions[0]; + var pickupCell = H3KRing.TryComputeCell(pickup.Lat, pickup.Lon, resolution); + + if (pickupCell is null) + return []; + + var cells = H3KRing.CellsWithin(pickupCell, resolution, searchRadiusMeters); + + if (cells.Count == 0) + return []; + + // Marten translates Contains() over a captured collection to `= ANY(:param)` — a SINGLE + // array parameter against the duplicated h3_cell column, not an N-term IN list. That is + // what makes a ~1,000-cell ring affordable in one round trip. + // + // The availability predicates are the fork-1 exclusion rule expressed as SQL. A driver whose + // AvailabilityState is null has never been heard about from Driver Profile, and `d.X == v` + // is not true for a null X, so those drivers fall out here without a special case. Until the + // ASB half is built that is EVERY driver, and the empty result is correct rather than + // broken: Dispatch genuinely cannot know who is dispatchable yet. + var candidates = await session.Query() + .Where(d => cells.Contains(d.H3Cell) + && d.AvailabilityState == DriverAvailabilityState.Available + && d.VehicleClass == vehicleClassRequired) + .ToListAsync(ct); + + // The exact-distance filter that trims the k-ring's deliberate over-approximation back to + // the true radius. Ordering by distance here rather than in SQL is deliberate: the distance + // is a great-circle computation over two columns, so Postgres would have to compute it for + // every row of the ring anyway, and the ring is already bounded to a single pickup radius. + return candidates + .Select(d => new + { + Driver = d, + Distance = H3KRing.DistanceMeters(pickup.Lat, pickup.Lon, d.Lat, d.Lon) + }) + .Where(x => x.Distance <= searchRadiusMeters) + .OrderBy(x => x.Distance) + .Select(x => new NearbyDriver( + DriverId: x.Driver.Id, + DistanceMeters: (int)Math.Round(x.Distance), + EtaSeconds: (int)Math.Round(x.Distance / AssumedUrbanSpeedMetersPerSecond), + // Non-null by construction: the query above filters to a concrete vehicle class. + VehicleClass: x.Driver.VehicleClass!.Value)) + .ToList(); + } +} diff --git a/src/CritterCab.Dispatch/CritterCab.Dispatch.csproj b/src/CritterCab.Dispatch/CritterCab.Dispatch.csproj index 3ff7d15..a970cea 100644 --- a/src/CritterCab.Dispatch/CritterCab.Dispatch.csproj +++ b/src/CritterCab.Dispatch/CritterCab.Dispatch.csproj @@ -6,9 +6,46 @@ + + + + + + + + + + + + + + diff --git a/src/CritterCab.Dispatch/Program.cs b/src/CritterCab.Dispatch/Program.cs index dd8f435..914f5e1 100644 --- a/src/CritterCab.Dispatch/Program.cs +++ b/src/CritterCab.Dispatch/Program.cs @@ -1,12 +1,16 @@ +using CritterCab.Dispatch.AvailableDrivers; using CritterCab.Dispatch.CandidateSelection; using CritterCab.Dispatch.FareQuoting; using CritterCab.Dispatch.RideRequesting; +using CritterCab.Telemetry.V1; using JasperFx; using Marten; using JasperFx.Events.Projections; using Wolverine; using Wolverine.Http; +using Wolverine.Kafka; using Wolverine.Marten; +using Wolverine.Protobuf; var builder = WebApplication.CreateBuilder(args); @@ -38,6 +42,23 @@ opts.Projections.Add(new RequestTimelineProjection(), ProjectionLifecycle.Inline); opts.Projections.Add(new FareQuoteAttemptsProjection(), ProjectionLifecycle.Inline); opts.Projections.Add(new RequestRoundsProjection(), ProjectionLifecycle.Inline); + + // AvailableDriver is a plain document, NOT an event stream and NOT a projection — the + // stream-processing shape crossing the BC boundary with the data (W006 §6.5). It is the + // only non-event-sourced write path in this otherwise fully event-sourced service, so it + // gets its schema configured here rather than registered above with the projections. + opts.Schema.For() + // Duplicated COLUMNS, not computed JSONB indexes: the k-ring query filters on all three + // together, and `= ANY(...)` over a real indexed column is the whole reason a + // thousand-cell ring is affordable in one round trip. + .Duplicate(x => x.H3Cell) + .Duplicate(x => x.VehicleClass) + .Duplicate(x => x.AvailabilityState) + // Load-bearing, and silent if omitted. TryUpdateRevision only applies its + // `where mt_version < ?` guard when the document is registered for numeric revisions; + // without this it degrades to a plain unguarded upsert and stale Kafka redeliveries + // start overwriting fresh positions with no error anywhere. + .UseNumericRevisions(true); }) .IntegrateWithWolverine(integration => { @@ -56,8 +77,29 @@ // from the DispatchPolicy projection instead. builder.Services.AddSingleton(FareQuoteRetryPolicy.Default); -// Nearby drivers source — stub until transport is decided (parking-lot #4). -builder.Services.AddSingleton(); +// Nearby drivers source (W006 §6.5, ADR-018) — W001 §10 parking-lot #4, closed. +// +// The real view whenever there is a document store to query; the stub only when there is not, so a +// database-less `dotnet run` still boots. The stub is DEMOTED here, not deleted: it remains the +// test double that slice 5.2 and 5.3's suites inject through the fixture, which is why those suites +// are untouched by this slice. +// +// Scoped, not singleton — it depends on IQuerySession. The stub stays a singleton because it holds +// nothing but a list. Same lifetime asymmetry, and same reason, as Telemetry's publisher seam. +if (!string.IsNullOrEmpty(connectionString)) +{ + builder.Services.AddScoped(); +} +else +{ + builder.Services.AddSingleton(); +} + +// One flag, read once, used by both the guard below and the listener wiring inside UseWolverine. +// Branching on the connection string twice would let the two drift into the state that breaks +// silently — a listener configured against a transport that was never registered. Same arrangement +// as Telemetry's kafkaEnabled. +var kafkaEnabled = !string.IsNullOrEmpty(builder.Configuration.GetConnectionString("kafka")); // Dispatch policy — hardcoded defaults per W001 §5.3; Slice 11 swaps for the // DispatchPolicyConfigured-fed projection. @@ -80,9 +122,82 @@ // Workshop §5.2 pins "Automation" as the CritterCab term for event-driven // handlers; expose the convention to Wolverine's handler discovery. + // + // This is ADDITIVE, not a replacement — Wolverine appends its built-in conventions ("Handler", + // "Consumer", Saga, IWolverineHandler, [WolverineHandler]) at bootstrap, after this runs, and + // the include filters are OR'd. So DriverLocationUpdatedHandler is discovered by the built-in + // suffix without appearing here. Only DisableConventionalDiscovery() would change that. opts.Discovery.CustomizeHandlerDiscovery(d => d.Includes.WithNameSuffix("Automation")); + + // Guarded rather than early-returned, so a broker-less run cannot silently swallow any + // Wolverine configuration appended after this line. + if (kafkaEnabled) + ConfigureKafkaListening(opts); }); +// === Kafka: the slice-5 consumer (W006 §6.5) === +// +// Dispatch's FIRST transport. Everything else this service does is in-process Marten over HTTP. +// This is also CritterCab's first cross-service flow: a ping entering Telemetry over gRPC comes out +// here as a document write. +static void ConfigureKafkaListening(WolverineOptions opts) +{ + // Read the broker address by NAME, as Telemetry does — Aspire injects it under the "kafka" key + // via .WithReference(kafka), and the same code then works against a local container, the test + // Testcontainer, and Azure Event Hubs with no environment branching. + // + // No AutoProvision() on this side. Telemetry provisions the topic because it owns it (ADR-018 + // supplier half); a consumer that auto-created the topic would mask a misconfigured topic name + // by silently creating an empty one and then waiting forever on it. + opts.UseKafkaUsingNamedConnection("kafka"); + + // The topic name is spelled out rather than shared as a constant with Telemetry — there is no + // shared assembly to put one in, and that is the point (ADR-019 names the convention; the + // .proto carries it in a comment). A rename on either side must be a deliberate edit on both. + opts.ListenToKafkaTopic("telemetry.driver-location-updated") + // ConfigureConsumer REPLACES the parent ConsumerConfig rather than merging into it. + // Bootstrap servers are inherited from the connection above; anything else set on the + // parent would NOT be. There is no .GroupId(string) shortcut — this is the shape. + // + // The group id is the service, not the topic: every Dispatch instance shares it, so Kafka + // distributes partitions across instances instead of delivering every position to all of + // them. It is also what makes the redelivery-on-rebalance case real, which is precisely + // what DriverLocationUpdatedHandler's revision guard defends against. + .ConfigureConsumer(c => c.GroupId = "dispatch") + // Cold-start position, and it ONLY applies when the group has no committed offset — after + // the first commit the group resumes where it left off and this is ignored. Stated + // explicitly rather than left to Confluent's default, both because ConfigureConsumer above + // REPLACES the ConsumerConfig and because the choice is a real one. + // + // Latest, not Earliest, on W006's own reasoning: a stale position is worthless here — §6.4 + // actively evicts positions older than three heartbeats — and the heartbeat guarantees the + // view refills within heartbeatIntervalSeconds regardless. Earliest would replay however + // many hours of retained telemetry the topic holds on a first deploy, discard all but the + // newest position per driver through the revision guard, and arrive at exactly the state + // one heartbeat would have produced in seconds. + // + // The cost is that a consumer which has not yet joined its group misses what is published + // in the meantime. That is invisible in production (the heartbeat covers it) but very + // visible in a test, which is why DispatchKafkaTestFixture performs a warm-up handshake + // before any test produces. + .BeginAtLatest() + // Mandatory, and its absence fails at the FIRST MESSAGE rather than at startup. + // ProtobufMessageSerializer.ReadFromData(byte[]) throws NotSupportedException; only the + // (Type, Envelope) overload works. Endpoint-scoped, matching the publisher's endpoint-scoped + // choice on the other side — Dispatch's HTTP surface stays JSON. + .UseProtobufSerialization() + // Hardening, not a requirement. Wolverine's default Kafka envelope mapper writes a + // `message-type` header on publish and Telemetry uses that default, so the type would + // resolve from the wire. Declaring it here REPLACES that header mapping with a constant, + // which makes this listener immune to a producer that ever omits or misspells the header. + // Safe precisely because the topic carries exactly one message type. + .DefaultIncomingMessage(); + + // Deliberately NOT setting opts.Durability.UseSyncRetryBlock. Telemetry sets it process-globally + // to make its publish-first ordering real (W006 §6.3); it is a PRODUCER concern. A listener does + // not inherit it across services and does not need it. +} + var app = builder.Build(); app.MapHealthChecks("/health"); diff --git a/tests/CritterCab.Dispatch.Tests/AvailableDrivers/H3KRingTests.cs b/tests/CritterCab.Dispatch.Tests/AvailableDrivers/H3KRingTests.cs new file mode 100644 index 0000000..1a6b448 --- /dev/null +++ b/tests/CritterCab.Dispatch.Tests/AvailableDrivers/H3KRingTests.cs @@ -0,0 +1,114 @@ +using CritterCab.Dispatch.AvailableDrivers; +using Shouldly; +using Xunit; + +namespace CritterCab.Dispatch.Tests.AvailableDrivers; + +// Pinning tests for the H3 binding, in the same spirit as Telemetry's H3CellIndexerTests and for +// the same reason: every failure mode in this file is SILENT. A swapped lat/lon, a degrees-for- +// radians mix-up, or a k that is one ring too small all produce plausible-looking output — a valid +// cell id, a finite distance, a non-empty ring — and the only symptom downstream is a dispatch that +// quietly finds no drivers. +// +// No broker and no database: these are pure functions. +public class H3KRingTests +{ + // Chicago's Loop. Positive latitude, NEGATIVE longitude — chosen deliberately, because the sign + // asymmetry is what makes an axis swap detectable at all. + private const double LoopLat = 41.8827d, LoopLon = -87.6233d; + + [Fact] + public void a_cell_matches_the_one_telemetry_would_publish_for_the_same_point() + { + // The literal is the pin. Telemetry computes the same value through H3CellIndexer, and the + // join between the two services is string equality on exactly this — if either side's + // binding drifts, the k-ring stops matching stored cells and the view silently empties. + H3KRing.TryComputeCell(LoopLat, LoopLon, 9).ShouldBe("892664c1a97ffff"); + } + + [Fact] + public void arguments_are_lat_then_lon_not_x_then_y() + { + // The swapped point is in the Southern Ocean off Antarctica; it is still a VALID cell, which + // is precisely why the mistake survives a smoke test. + var correct = H3KRing.TryComputeCell(LoopLat, LoopLon, 9); + var swapped = H3KRing.TryComputeCell(LoopLon, LoopLat, 9); + + correct.ShouldNotBe(swapped); + } + + [Fact] + public void an_out_of_range_resolution_yields_null_rather_than_throwing() + { + // H3 signals bad input by returning an invalid index, never by throwing — so "invalid" is a + // value to branch on. Matches H3CellIndexer's contract on the Telemetry side. + H3KRing.TryComputeCell(LoopLat, LoopLon, 42).ShouldBeNull(); + } + + [Fact] + public void distance_is_in_metres_and_degree_denominated() + { + // Roughly 1 degree of latitude ~ 111km. If the inputs were being read as radians this would + // come back off by a factor of ~57, so the assertion is a units check as much as a + // distance check. + var meters = H3KRing.DistanceMeters(LoopLat, LoopLon, LoopLat + 1.0d, LoopLon); + + meters.ShouldBeInRange(110_000d, 112_000d); + } + + [Fact] + public void distance_to_the_same_point_is_zero() + { + H3KRing.DistanceMeters(LoopLat, LoopLon, LoopLat, LoopLon).ShouldBeLessThan(0.001d); + } + + [Theory] + // A ring advances edge x 1.5 metres in the worst case, not edge x sqrt(3) — see H3KRing for + // why the axis-aligned spacing over-states the coverage. At resolution 9 that is ~302m. + // + // These numbers are pinned against MEASURED grid distances, not against the formula: a point + // 4982m due north of the Loop sits at grid distance 17, so k=17 is the true requirement at 5km + // and the +1 is real margin on top. The earlier sqrt(3) form produced k=16 here and silently + // excluded that point. + [InlineData(9, 1_000, 5)] // ceil(1000/302) = 4, +1 + [InlineData(9, 5_000, 18)] // ceil(5000/302) = 17, +1 + [InlineData(8, 5_000, 8)] // coarser cells (~797m per ring), fewer rings + public void k_covers_the_radius_with_one_ring_to_spare(int resolution, int radius, int expectedK) + { + H3KRing.DeriveK(resolution, radius).ShouldBe(expectedK); + } + + [Theory] + // The real invariant behind DeriveK, asserted end to end rather than through the arithmetic: a + // point at the very edge of the radius must fall in a cell the ring contains. This is what + // would fail if the sqrt(3) spacing factor were "simplified" away, or if the edge-length basis + // were swapped for the smaller documented figure. + // + // 5000m is DispatchPolicySnapshot.Default's production search radius, so that row is the one + // that actually protects live dispatch. ~0.009 degrees of latitude is ~1000m due north. + [InlineData(1_000, 0.0089d)] + [InlineData(5_000, 0.0448d)] + public void the_ring_actually_reaches_the_radius_it_claims_to(int radius, double latOffset) + { + var originCell = H3KRing.TryComputeCell(LoopLat, LoopLon, 9)!; + var cells = H3KRing.CellsWithin(originCell, 9, radius); + + var edgeCell = H3KRing.TryComputeCell(LoopLat + latOffset, LoopLon, 9)!; + + cells.ShouldContain(edgeCell); + } + + [Fact] + public void the_ring_always_contains_its_own_origin() + { + var originCell = H3KRing.TryComputeCell(LoopLat, LoopLon, 9)!; + + H3KRing.CellsWithin(originCell, 9, 500).ShouldContain(originCell); + } + + [Fact] + public void an_unparseable_origin_yields_an_empty_ring_rather_than_throwing() + { + H3KRing.CellsWithin("not-a-cell", 9, 1_000).ShouldBeEmpty(); + } +} diff --git a/tests/CritterCab.Dispatch.Tests/AvailableDrivers/Slice5DriverLocationConsumerTests.cs b/tests/CritterCab.Dispatch.Tests/AvailableDrivers/Slice5DriverLocationConsumerTests.cs new file mode 100644 index 0000000..c81b46e --- /dev/null +++ b/tests/CritterCab.Dispatch.Tests/AvailableDrivers/Slice5DriverLocationConsumerTests.cs @@ -0,0 +1,173 @@ +using CritterCab.Dispatch.AvailableDrivers; +using CritterCab.Telemetry.V1; +using JasperFx.Core; +using Microsoft.Extensions.DependencyInjection; +using Shouldly; +using Wolverine; +using Wolverine.Tracking; +using Xunit; +using ProtoTimestamp = Google.Protobuf.WellKnownTypes.Timestamp; + +namespace CritterCab.Dispatch.Tests.AvailableDrivers; + +// W006 §6.5's Location-update and Availability GWTs, plus §6.3's Dedup GWT — the one PR C could not +// write, because it asserts CONSUMER behaviour against at-least-once redelivery and there was no +// consumer until this slice. +// +// This is CritterCab's first cross-service assertion. Everything here goes over a real broker: the +// fixture produces the same binary-protobuf record Telemetry's publisher produces, and the service's +// own production listener wiring picks it up. Nothing is stubbed on the transport path. +[Collection("DispatchKafka")] +public class Slice5DriverLocationConsumerTests +{ + private const double LoopLat = 41.8827d, LoopLon = -87.6233d; + private const int Resolution = 9; + + private readonly DispatchKafkaTestFixture _fixture; + + public Slice5DriverLocationConsumerTests(DispatchKafkaTestFixture fixture) => _fixture = fixture; + + [Fact] + public async Task a_published_position_lands_in_the_available_driver_document() + { + await _fixture.ResetDriversAsync(); + + var driverId = Guid.CreateVersion7(); + var at = DateTimeOffset.UtcNow; + + await ProduceAndWaitAsync(PositionOf(driverId, LoopLat, LoopLon, at)); + + var driver = await _fixture.LoadDriverAsync(driverId); + + driver.ShouldNotBeNull("No AvailableDriver document was written — if the handler stopped " + + "being discovered, the message is never even deserialized."); + driver.H3Cell.ShouldBe(H3KRing.TryComputeCell(LoopLat, LoopLon, Resolution)); + driver.H3Resolution.ShouldBe(Resolution); + driver.Lat.ShouldBe(LoopLat); + driver.Lon.ShouldBe(LoopLon); + + // The availability side stays empty — this driver has a position and no capability, which + // is the steady state until Driver Profile ships (W006 §6.5 fork 1). + driver.AvailabilityState.ShouldBeNull(); + driver.VehicleClass.ShouldBeNull(); + } + + // W006 §6.3's Dedup GWT. + [Fact] + public async Task a_redelivered_position_is_applied_at_most_once() + { + await _fixture.ResetDriversAsync(); + + var driverId = Guid.CreateVersion7(); + var at = DateTimeOffset.UtcNow; + var position = PositionOf(driverId, LoopLat, LoopLon, at); + + await ProduceAndWaitAsync(position); + var afterFirst = await _fixture.LoadDriverAsync(driverId); + afterFirst.ShouldNotBeNull(); + + // Byte-for-byte the same record, exactly as a consumer-group rebalance would replay it. + await ProduceAndWaitAsync(PositionOf(driverId, LoopLat, LoopLon, at)); + + var afterSecond = await _fixture.LoadDriverAsync(driverId); + afterSecond.ShouldNotBeNull(); + + // Equal serverReceivedAt means an equal revision, and Marten's guard is a strict `<`, so + // the duplicate is discarded in the database rather than reapplied. The version standing + // still is the observable proof: had the guard been missing (or had the document been + // registered IRevisioned instead of ILongVersioned, truncating the revision), this would + // have incremented. + afterSecond.Version.ShouldBe(afterFirst.Version); + afterSecond.ServerReceivedAt.ToUnixTimeMilliseconds() + .ShouldBe(at.ToUnixTimeMilliseconds()); + } + + // The redelivery case that actually regresses a view: not a duplicate, but an OLDER position + // arriving after a newer one was already applied. + [Fact] + public async Task a_stale_position_never_overwrites_a_newer_one() + { + await _fixture.ResetDriversAsync(); + + var driverId = Guid.CreateVersion7(); + var newer = DateTimeOffset.UtcNow; + var older = newer.AddSeconds(-30); + + await ProduceAndWaitAsync(PositionOf(driverId, LoopLat, LoopLon, newer)); + + // A different cell, so a successful overwrite would be unmistakable in the assertion below. + const double StaleLat = 41.9d, StaleLon = -87.7d; + await ProduceAndWaitAsync(PositionOf(driverId, StaleLat, StaleLon, older)); + + var driver = await _fixture.LoadDriverAsync(driverId); + + driver.ShouldNotBeNull(); + driver.Lat.ShouldBe(LoopLat); + driver.Lon.ShouldBe(LoopLon); + driver.ServerReceivedAt.ToUnixTimeMilliseconds().ShouldBe(newer.ToUnixTimeMilliseconds()); + } + + // W006 §6.5's Availability GWT. The ASB transport does not exist, so the handler is invoked + // in-process — which is exactly what fork 1 committed to: the landing site ships, the feeder + // does not. + [Fact] + public async Task an_availability_event_fills_the_other_side_without_disturbing_the_location() + { + await _fixture.ResetDriversAsync(); + + var driverId = Guid.CreateVersion7(); + var at = DateTimeOffset.UtcNow; + + await ProduceAndWaitAsync(PositionOf(driverId, LoopLat, LoopLon, at)); + + // IMessageBus is scoped, so it cannot come from the root provider. + using var scope = _fixture.Host.Services.CreateScope(); + var bus = scope.ServiceProvider.GetRequiredService(); + + await bus.InvokeAsync(new DriverAvailabilityChanged + { + DriverId = driverId, + AvailabilityState = DriverAvailabilityState.Available, + VehicleClass = VehicleClass.Standard, + AvailabilityUpdatedAt = at.AddSeconds(1) + }); + + var driver = await _fixture.LoadDriverAsync(driverId); + + driver.ShouldNotBeNull(); + driver.AvailabilityState.ShouldBe(DriverAvailabilityState.Available); + driver.VehicleClass.ShouldBe(VehicleClass.Standard); + + // The location side survived the availability write. The two handlers share one document + // and one revision column, so a blind whole-document write on either side would erase the + // other — this is the assertion that catches it. + driver.H3Cell.ShouldBe(H3KRing.TryComputeCell(LoopLat, LoopLon, Resolution)); + driver.Lat.ShouldBe(LoopLat); + } + + // Produces the record and waits for Dispatch's listener to finish handling it. + // + // WaitForMessageToBeReceivedAt is the API that waits on an ARRIVAL. IncludeExternalTransports() + // is deliberately absent: it governs whether OUTGOING sends stay open pending a receipt, and + // there is no outgoing Wolverine send here — the fixture produces with a raw ProducerBuilder, + // so there is nothing for the tracked session to observe on the way out. + private Task ProduceAndWaitAsync(DriverLocationUpdated message) => + _fixture.Host + .TrackActivity() + .Timeout(30.Seconds()) + .WaitForMessageToBeReceivedAt(_fixture.Host) + .ExecuteAndWaitAsync(_ => _fixture.ProduceAsync(message)); + + private static DriverLocationUpdated PositionOf( + Guid driverId, double lat, double lon, DateTimeOffset at) => + new() + { + DriverId = driverId.ToString(), + Lat = lat, + Lon = lon, + H3Cell = H3KRing.TryComputeCell(lat, lon, Resolution)!, + H3Resolution = Resolution, + ServerReceivedAt = ProtoTimestamp.FromDateTimeOffset(at), + ThrottlePolicyVersion = 1L + }; +} diff --git a/tests/CritterCab.Dispatch.Tests/AvailableDrivers/Slice5NearbyAvailableDriversViewTests.cs b/tests/CritterCab.Dispatch.Tests/AvailableDrivers/Slice5NearbyAvailableDriversViewTests.cs new file mode 100644 index 0000000..d03c9c9 --- /dev/null +++ b/tests/CritterCab.Dispatch.Tests/AvailableDrivers/Slice5NearbyAvailableDriversViewTests.cs @@ -0,0 +1,178 @@ +using CritterCab.Dispatch.AvailableDrivers; +using CritterCab.Dispatch.CandidateSelection; +using CritterCab.Telemetry.V1; +using JasperFx.Core; +using Microsoft.Extensions.DependencyInjection; +using Shouldly; +using Wolverine; +using Wolverine.Tracking; +using Xunit; +using ProtoTimestamp = Google.Protobuf.WellKnownTypes.Timestamp; + +namespace CritterCab.Dispatch.Tests.AvailableDrivers; + +// W006 §6.5's Selection-read GWT, and the fork-1 exclusion rule that governs it. +// +// These exercise the REAL INearbyAvailableDriversSource registration — the one that replaced the +// stub — resolved from the running host, so the k-ring query, the exact-distance filter and the +// availability join are all under test as production wires them. +[Collection("DispatchKafka")] +public class Slice5NearbyAvailableDriversViewTests +{ + // Chicago's Loop, and two points at known distances from it. The near driver sits a few hundred + // metres away, the far one several kilometres — comfortably either side of a 2km radius, so the + // test is not sensitive to the exact great-circle arithmetic. + private const double PickupLat = 41.8827d, PickupLon = -87.6233d; + private const double NearLat = 41.8850d, NearLon = -87.6250d; + private const double FarLat = 41.9400d, FarLon = -87.6900d; + private const int Resolution = 9; + private const int RadiusMeters = 2_000; + + private readonly DispatchKafkaTestFixture _fixture; + + public Slice5NearbyAvailableDriversViewTests(DispatchKafkaTestFixture fixture) => _fixture = fixture; + + [Fact] + public async Task drivers_in_range_and_capable_are_returned_nearest_first() + { + await _fixture.ResetDriversAsync(); + + var near = await AvailableDriverAt(NearLat, NearLon, VehicleClass.Standard); + var far = await AvailableDriverAt(FarLat, FarLon, VehicleClass.Standard); + + var found = await QueryAsync(RadiusMeters, VehicleClass.Standard); + + // The far driver is inside the k-ring — the ring deliberately over-approximates — and is + // trimmed by the exact-distance filter. That is the assertion that proves the two-stage + // query works rather than the ring accidentally being tight enough. + found.Select(d => d.DriverId).ShouldBe([near]); + found[0].DistanceMeters.ShouldBeLessThan(RadiusMeters); + + // ETA is derived from distance against an invented urban-speed constant (W006 §6.5 fork 3). + // Asserted as a relationship, not a value: pinning the number here would freeze a + // placeholder that a real ETA service is meant to replace. + found[0].EtaSeconds.ShouldBeGreaterThan(0); + + far.ShouldNotBe(near); + } + + [Fact] + public async Task a_driver_of_the_wrong_vehicle_class_is_excluded() + { + await _fixture.ResetDriversAsync(); + + await AvailableDriverAt(NearLat, NearLon, VehicleClass.Standard); + + var found = await QueryAsync(RadiusMeters, VehicleClass.Accessible); + + // W001 §5.3's ACCESSIBLE-scarcity path: the driver is present, in range, and available, but + // cannot serve the request. The filter is a real SQL predicate over a duplicated column. + found.ShouldBeEmpty(); + } + + [Fact] + public async Task a_driver_who_is_not_available_is_excluded() + { + await _fixture.ResetDriversAsync(); + + await AvailableDriverAt(NearLat, NearLon, VehicleClass.Standard, DriverAvailabilityState.Offline); + + var found = await QueryAsync(RadiusMeters, VehicleClass.Standard); + + found.ShouldBeEmpty(); + } + + // The fork-1 rule, and the one most worth locking down: this is the state EVERY driver is in + // until Driver Profile ships, so if it ever silently flipped to "included", Dispatch would + // start offering rides based on a capability it never learned. + [Fact] + public async Task a_driver_with_a_position_but_no_availability_is_excluded() + { + await _fixture.ResetDriversAsync(); + + // A location update only — no availability event follows it. + var driverId = Guid.CreateVersion7(); + await PublishPositionAsync(driverId, NearLat, NearLon); + + var stored = await _fixture.LoadDriverAsync(driverId); + stored.ShouldNotBeNull("the position itself should still have landed"); + stored.AvailabilityState.ShouldBeNull(); + + var found = await QueryAsync(RadiusMeters, VehicleClass.Standard); + + found.ShouldBeEmpty(); + } + + [Fact] + public async Task an_empty_store_returns_no_drivers_without_guessing_a_resolution() + { + await _fixture.ResetDriversAsync(); + + // With no documents there is no resolution to derive, and resolution 0 is itself valid — + // so the view must short-circuit on document ABSENCE rather than on a default value. + var found = await QueryAsync(RadiusMeters, VehicleClass.Standard); + + found.ShouldBeEmpty(); + } + + private async Task AvailableDriverAt( + double lat, + double lon, + VehicleClass vehicleClass, + DriverAvailabilityState state = DriverAvailabilityState.Available) + { + var driverId = Guid.CreateVersion7(); + + await PublishPositionAsync(driverId, lat, lon); + + // IMessageBus is scoped, so it cannot come from the root provider. Standing in for the ASB + // transport that would deliver this in a built system (W006 §6.5 fork 1). + using var scope = _fixture.Host.Services.CreateScope(); + var bus = scope.ServiceProvider.GetRequiredService(); + + await bus.InvokeAsync(new DriverAvailabilityChanged + { + DriverId = driverId, + AvailabilityState = state, + VehicleClass = vehicleClass, + AvailabilityUpdatedAt = DateTimeOffset.UtcNow + }); + + return driverId; + } + + private Task PublishPositionAsync(Guid driverId, double lat, double lon) + { + var message = new DriverLocationUpdated + { + DriverId = driverId.ToString(), + Lat = lat, + Lon = lon, + H3Cell = H3KRing.TryComputeCell(lat, lon, Resolution)!, + H3Resolution = Resolution, + ServerReceivedAt = ProtoTimestamp.FromDateTimeOffset(DateTimeOffset.UtcNow), + ThrottlePolicyVersion = 1L + }; + + return _fixture.Host + .TrackActivity() + .Timeout(30.Seconds()) + .WaitForMessageToBeReceivedAt(_fixture.Host) + .ExecuteAndWaitAsync(_ => _fixture.ProduceAsync(message)); + } + + // Resolves the source from the host rather than newing it up, so the test exercises whatever + // Program.cs registered. If the stub were ever wired back in by accident, these tests would + // start passing for the wrong reason — hence the scoped resolution and the type assertion. + private async Task> QueryAsync( + int radiusMeters, VehicleClass vehicleClass) + { + using var scope = _fixture.Host.Services.CreateScope(); + var source = scope.ServiceProvider.GetRequiredService(); + + source.ShouldBeOfType(); + + return await source.GetDriversAsync( + new Location(PickupLat, PickupLon), radiusMeters, vehicleClass); + } +} diff --git a/tests/CritterCab.Dispatch.Tests/CritterCab.Dispatch.Tests.csproj b/tests/CritterCab.Dispatch.Tests/CritterCab.Dispatch.Tests.csproj index a2cbb47..790fe2d 100644 --- a/tests/CritterCab.Dispatch.Tests/CritterCab.Dispatch.Tests.csproj +++ b/tests/CritterCab.Dispatch.Tests/CritterCab.Dispatch.Tests.csproj @@ -16,6 +16,11 @@ + + + diff --git a/tests/CritterCab.Dispatch.Tests/DispatchKafkaTestFixture.cs b/tests/CritterCab.Dispatch.Tests/DispatchKafkaTestFixture.cs new file mode 100644 index 0000000..fe93fd5 --- /dev/null +++ b/tests/CritterCab.Dispatch.Tests/DispatchKafkaTestFixture.cs @@ -0,0 +1,199 @@ +using Alba; +using Confluent.Kafka; +using CritterCab.Dispatch.AvailableDrivers; +using DotNet.Testcontainers.Images; +using Google.Protobuf; +using JasperFx.Core; +using Marten; +using Microsoft.Extensions.DependencyInjection; +using Wolverine.Tracking; +// Confluent.Kafka ships its own Timestamp, which collides with protobuf's well-known type. +// Aliased rather than fully qualified, matching how the Telemetry suite handles the same clash. +using ProtoTimestamp = Google.Protobuf.WellKnownTypes.Timestamp; +using Testcontainers.Kafka; +using Testcontainers.PostgreSql; +using Xunit; + +namespace CritterCab.Dispatch.Tests; + +// A second, heavier Dispatch fixture that stands up a REAL Kafka broker alongside Postgres, so +// slice 5's consumer can be asserted on the wire rather than at a seam. +// +// Deliberately separate from DispatchTestFixture, mirroring the split the Telemetry project made +// for the same reason: the slice 5.1/5.2/5.3 suites have no interest in a broker, and widening the +// shared fixture would make every one of them wait on a Kafka container. The split is also more +// honest about what each host is for — the shared fixture swaps INearbyAvailableDriversSource for a +// stub to test the DECISION, this one leaves the production wiring intact to test the TRANSPORT and +// the view behind it. +public class DispatchKafkaTestFixture : IAsyncLifetime +{ + // The topic name is written out rather than imported from the service. There is no shared + // constant by design (no shared assembly between the services), so the test asserts against the + // literal ADR-019 name and would catch a rename in Program.cs rather than silently follow it. + public const string Topic = "telemetry.driver-location-updated"; + + // Unique container names: this project now starts TWO Postgres containers (this fixture and + // DispatchTestFixture) and xUnit runs their collections in parallel, so a fixed name collides. + private readonly PostgreSqlContainer _postgres = new PostgreSqlBuilder("postgres:18-alpine") + .WithName($"dispatch-kafka-pg-{Guid.NewGuid():N}") + .WithImagePullPolicy(PullPolicy.Missing) + .Build(); + + // cp-kafka, NOT confluentinc/confluent-local — KafkaBuilder injects a startup script built + // around cp-kafka's entrypoint, while confluent-local runs KRaft and expects its log directory + // to have been formatted by kafka-storage.sh first. The combination exits 1 at startup. Same + // pinned image and same reasoning as the Telemetry fixture. + private readonly KafkaContainer _kafka = new KafkaBuilder("confluentinc/cp-kafka:7.6.1") + .WithName($"dispatch-kafka-{Guid.NewGuid():N}") + .WithImagePullPolicy(PullPolicy.Missing) + .Build(); + + public IAlbaHost Host { get; private set; } = null!; + + public string BootstrapServers { get; private set; } = null!; + + public async Task InitializeAsync() + { + await Task.WhenAll(_postgres.StartAsync(), _kafka.StartAsync()); + + // Testcontainers reports the address with a protocol prefix (PLAINTEXT://host:port); + // Confluent's ProducerConfig wants a bare host:port list, so strip it. + BootstrapServers = _kafka.GetBootstrapAddress().Replace("PLAINTEXT://", string.Empty); + + // Dispatch does not AutoProvision — the producer owns the topic (ADR-018 supplier half) — + // so nothing in the service creates it. Create it here, standing in for the Telemetry host + // that would have done so in a real deployment. Without this the listener subscribes to a + // topic that does not exist and simply waits. + await CreateTopicAsync(); + + Host = await AlbaHost.For(builder => + { + builder.UseSetting("ConnectionStrings:crittercab_dispatch", _postgres.GetConnectionString()); + + // Supplying this key is what flips Program.cs from "no transport" to the real Kafka + // listener — the same guarded branch Aspire drives in local dev. Nothing in this + // fixture registers a handler or a listener itself; the point is to exercise the + // production wiring end to end. + builder.UseSetting("ConnectionStrings:kafka", BootstrapServers); + }); + + await WarmUpListenerAsync(); + } + + // Produces throwaway records until one is observably handled, then clears it. + // + // This exists because the listener declares BeginAtLatest() — it starts at the TAIL on a cold + // start, so anything produced before the consumer group has finished joining is legitimately + // missed. In production that window is invisible: Telemetry's heartbeat republishes every + // driver within heartbeatIntervalSeconds, which is exactly why Latest is the right cold-start + // policy there. A test has no heartbeat, so it produces once and waits forever on a record the + // broker never delivered. + // + // Retrying rather than sleeping a fixed interval: group join time varies with broker startup, + // and a sleep long enough to be safe on a slow machine wastes that time on every run. This + // returns as soon as the round trip demonstrably works, which is the actual condition every + // test depends on. + private async Task WarmUpListenerAsync() + { + var deadline = DateTimeOffset.UtcNow.AddSeconds(60); + var warmUpDriverId = Guid.CreateVersion7(); + + while (DateTimeOffset.UtcNow < deadline) + { + var message = new CritterCab.Telemetry.V1.DriverLocationUpdated + { + DriverId = warmUpDriverId.ToString(), + Lat = 0d, + Lon = 0d, + H3Cell = "8f754e64992d6d8", + H3Resolution = 15, + ServerReceivedAt = ProtoTimestamp.FromDateTimeOffset(DateTimeOffset.UtcNow), + ThrottlePolicyVersion = 1L + }; + + try + { + await Host + .TrackActivity() + .Timeout(10.Seconds()) + .WaitForMessageToBeReceivedAt(Host) + .ExecuteAndWaitAsync(_ => ProduceAsync(message)); + + // The round trip works. Remove the warm-up driver so no test sees it. + await ResetDriversAsync(); + return; + } + catch (TimeoutException) + { + // Group not joined yet. Produce again — under BeginAtLatest the previous record is + // already behind the tail and will never arrive, so retrying is the only option. + } + } + + throw new TimeoutException( + "Dispatch's Kafka listener never consumed a warm-up record. The consumer group did not " + + "join within 60s, or the listener is not configured."); + } + + public async Task DisposeAsync() + { + await Host.DisposeAsync(); + await Task.WhenAll(_postgres.DisposeAsync().AsTask(), _kafka.DisposeAsync().AsTask()); + } + + // Produces a raw binary-protobuf record exactly as Telemetry's publisher would, standing in for + // the Telemetry service without running it. + // + // Note what is written by hand: the partition key (driverId, per W006 §6.3 R7) and the + // content-type header. The `message-type` header is deliberately NOT written — the listener + // declares .DefaultIncomingMessage(), so this also proves that hardening + // works and that the consumer would survive a producer that dropped the header. + public async Task ProduceAsync(CritterCab.Telemetry.V1.DriverLocationUpdated message) + { + using var producer = new ProducerBuilder(new ProducerConfig + { + BootstrapServers = BootstrapServers + }).Build(); + + await producer.ProduceAsync(Topic, new Message + { + Key = message.DriverId, + Value = message.ToByteArray(), + Headers = [new Header("content-type", "binary/protobuf"u8.ToArray())] + }); + + producer.Flush(TimeSpan.FromSeconds(10)); + } + + public async Task LoadDriverAsync(Guid driverId) + { + await using var session = Host.Services.GetRequiredService().QuerySession(); + return await session.LoadAsync(driverId); + } + + public async Task ResetDriversAsync() + { + var store = Host.Services.GetRequiredService(); + await store.Advanced.Clean.DeleteDocumentsByTypeAsync(typeof(AvailableDriver)); + } + + private async Task CreateTopicAsync() + { + using var admin = new AdminClientBuilder(new AdminClientConfig + { + BootstrapServers = BootstrapServers + }).Build(); + + await admin.CreateTopicsAsync([ + new Confluent.Kafka.Admin.TopicSpecification + { + Name = Topic, + NumPartitions = 1, + ReplicationFactor = 1 + } + ]); + } +} + +[CollectionDefinition("DispatchKafka")] +public class DispatchKafkaCollection : ICollectionFixture; From ce7456ec72a3f8c91c3fbc4ead68b455e42c88d8 Mon Sep 17 00:00:00 2001 From: Erik Shafer Date: Sat, 25 Jul 2026 00:06:04 -0500 Subject: [PATCH 3/8] Drain wolverine-kafka listener DEBT row; W006/W001 spec amendments; retro 009 --- docs/prompts/README.md | 1 + ...h-w006-slice-5-nearby-available-drivers.md | 2 +- ...h-w006-slice-5-nearby-available-drivers.md | 123 ++++++++++++++++++ docs/skills/DEBT.md | 28 +++- docs/skills/wolverine-kafka/SKILL.md | 88 ++++++++++--- docs/workshops/001-dispatch-event-model.md | 9 ++ docs/workshops/006-telemetry-event-model.md | 19 +++ src/CritterCab.Dispatch/Program.cs | 5 + 8 files changed, 249 insertions(+), 26 deletions(-) create mode 100644 docs/retrospectives/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md diff --git a/docs/prompts/README.md b/docs/prompts/README.md index f7972b7..a947941 100644 --- a/docs/prompts/README.md +++ b/docs/prompts/README.md @@ -170,3 +170,4 @@ Subsequent sections are prompt-specific. Existing prompts in this directory serv - [`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). - [`implementations/008-telemetry-slice-3-kafka-publish.md`](./implementations/008-telemetry-slice-3-kafka-publish.md) — **CritterCab's first Kafka topic and second live transport.** W006 slice 3: `DriverLocationUpdated` published to `telemetry.driver-location-updated`, partitioned by `driverId`, dedup-keyed on `serverReceivedAt`, no outbox. Deliberately the smallest session that could make a transport real — PR #45 had already put the proto through codegen and typed the `IDriverLocationPublisher` seam against the generated `DriverLocationUpdated`, so this session swapped one implementation class and the host wiring; **`ReportLocationsHandler.cs` does not appear in the diff.** Also wires Aspire's Kafka resource (host port 5392, a slot the `aspire` skill had reserved before any broker existed) and a Kafka Testcontainer on its own fixture, so the slice-1/2/4 suites never wait on a broker. **The load-bearing finding is a gap between spec and library defaults** that the authoring-time `jasperfx-source-verifier` pass caught: W006 §6.3 argues publish-before-store from failure-mode asymmetry, but that argument silently assumes the publish's outcome is known when the store runs — and a Kafka publishing endpoint defaults to `BufferedInMemory` (returns before the broker acks), while even `SendInline()` alone is insufficient because Wolverine's default async retry block swallows the send failure and returns success. Resolved by user sign-off to `SendInline()` + process-global `Durability.UseSyncRetryBlock` + `UseIdempotentProducer()`, and recorded in W006 as an **amendment, not a correction** (§6.3's reasoning stands; it did not name its own precondition). Fires W006 §11 candidate #1 as **[ADR-019 — Transport-Agnostic Topic Naming](../decisions/019-transport-agnostic-topic-naming.md)**, generalizing ADR-014's `.` across transports while explicitly withholding its two ASB-specific operational clauses (session keying, outbox coordination) — which also serves as the ADR-004 design-return interleave for the third consecutive Telemetry implementation PR. **Second escalated fork:** `wolverine-kafka/SKILL.md` said Cab uses default JSON and deferred protobuf to "a future phase" — overridden to endpoint-scoped binary protobuf by user sign-off, since the payload is generated from the `.proto` ADR-009 makes the contract of record. That skill's topic-naming section had *also* independently proposed a rule contradicting ADR-014; both corrected in-PR under the session-runner-blocking exception, the listener half deferred to PR D as DEBT. **First prompt whose Verify-before-wiring gates all closed at authoring time**, and first whose `critter-skill-auditor` Phase 1 pass materially corrected an already-source-verified prompt (a missing governing skill, a wrong bootstrap lean, an already-allocated port, two skill-vs-spec contradictions). 33/33 green. Status: complete (authored + executed 2026-07-24). Produced retro at [`retrospectives/implementations/008-telemetry-slice-3-kafka-publish.md`](../retrospectives/implementations/008-telemetry-slice-3-kafka-publish.md). +- [`implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md`](./implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md) — **CritterCab's first cross-service flow**, and the session that completes W006's slice walk. W006 slice 5: Dispatch consumes `telemetry.driver-location-updated` into per-driver `AvailableDriver` documents, backs the slice-5.3 `INearbyAvailableDriversSource` port with an H3 k-ring query over them, and demotes `NearbyAvailableDriversStub` from production registration to test double — closing **W001 §10 parking-lot #4 in code** three months after the design closed it on paper. First code in `CritterCab.Dispatch` since prompt 005, and Dispatch's **first transport**: everything it did before was in-process Marten over HTTP. Also the first time two services compile the same `.proto` — deliberately not a shared assembly, which holds because Wolverine's message identity is `Type.FullName`-based and assembly-agnostic (the real coupling is the proto's `csharp_namespace`, not the assembly). `CandidateSelectionAutomation.cs` **does not appear in the diff**, which was the whole point of building the port as a seam in slice 5.3. **Second consecutive prompt whose Verify-before-wiring gates all closed at authoring time, and three of eight premises were contradicted by source** — most consequentially that Marten's `TryUpdateRevision` is an atomic single-statement LWW upsert, strictly better than the read-then-conditionally-store that "LWW upsert" naturally suggests and which would have been racy. `critter-skill-auditor` Phase 1 again corrected an already-source-verified prompt, this time catching that it **named the wrong governing skill** (`wolverine-marten-automation`, which is scoped to Marten-stream-forwarded events; replaced with `wolverine-handlers` + `wolverine-messaging-handlers`). Three durable forks resolved by user sign-off — availability-half exclusion with an unbound `DriverAvailabilityChanged` handler (the ASB half is a forward-constraint to an un-workshopped Driver Profile BC, and fabricating `Available`/`STANDARD` defaults was explicitly rejected), H3 query resolution read from the most recently ingested document rather than a Dispatch-side constant, and `EtaSeconds` derived from distance against a constant **invented at implementation time**. A **fourth** fork surfaced mid-session and was escalated rather than absorbed: the cold-start offset policy (`BeginAtLatest()`, chosen on W006 §6.4's own eviction logic, with a fixture warm-up handshake as the test cost — `BeginAtEarliest()` would have turned four failing tests green in one line while silently committing the service to replaying its entire retained topic on every fresh deploy). **A strengthened test caught a real geometry bug:** the k-ring derivation divided by the axis-aligned hexagon spacing (`edge × √3`) rather than the worst-case per-ring advance (`edge × 1.5`), so at Dispatch's production 5 km radius drivers near the edge were silently dropped — invisible at the 1 km value the first version of the test used. Also drains the `wolverine-kafka` listener DEBT row **inside an implementation PR**, establishing that a row blocked on code that does not exist yet is drained by the session that writes the code. 54/54 green. Status: complete (authored + executed 2026-07-24). Produced retro at [`retrospectives/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md`](../retrospectives/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md). diff --git a/docs/prompts/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md b/docs/prompts/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md index 4cfddd2..09fdf8b 100644 --- a/docs/prompts/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md +++ b/docs/prompts/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md @@ -2,7 +2,7 @@ | Field | Value | |---|---| -| **Status** | Ready — three durable forks resolved by user sign-off 2026-07-24 (availability half: exclude-until-ASB with an unbound handler; H3 query resolution: track last-received; ETA: derived from distance via a named invented constant). All eight Verify-before-wiring gates closed at authoring time. Rides in the PR D session's PR alongside the implementation; not committed standalone. | +| **Status** | **Complete (2026-07-24)** — executed as PR D; retro at [`retrospectives/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md`](../../retrospectives/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md). A **fourth** fork surfaced during execution and was escalated rather than absorbed: the listener's cold-start offset policy (`BeginAtLatest()`, with a fixture warm-up handshake as the test cost). Originally: Ready — three durable forks resolved by user sign-off 2026-07-24 (availability half: exclude-until-ASB with an unbound handler; H3 query resolution: track last-received; ETA: derived from distance via a named invented constant). All eight Verify-before-wiring gates closed at authoring time. Rides in the PR D session's PR alongside the implementation; not committed standalone. | | **Authored** | 2026-07-24 | | **Target artifacts** | `src/CritterCab.Dispatch/CritterCab.Dispatch.csproj` (Kafka + Protobuf + H3 + proto codegen), `src/CritterCab.Dispatch/AvailableDrivers/` (**new slice folder** — document, two handlers, the query adapter, the H3 helper), `src/CritterCab.Dispatch/Program.cs` (Kafka listener, Marten document schema, source registration swap), `tests/CritterCab.Dispatch.Tests/AvailableDrivers/` (**new** Kafka-backed fixture + suites), `apphost.cs` (Dispatch → Kafka reference), `docs/workshops/006-telemetry-event-model.md` (§6.5 Document History), `docs/workshops/001-dispatch-event-model.md` (§5.3 realization note), `docs/skills/wolverine-kafka/SKILL.md` (listener sections rewritten from shipped code — the deferred DEBT row), `docs/skills/DEBT.md` (close that row), `docs/prompts/README.md` (index entry), this prompt's retro. | | **Source-of-truth dependencies** | [W006 §6.5 (the slice), §6.3's Dedup GWT (consumer-side, untestable until now), §3.2 (stream-processing shape)](../../workshops/006-telemetry-event-model.md); [W001 §5.3 (the amendment target) + §10 parking-lot #4](../../workshops/001-dispatch-event-model.md); [ADR-018](../../decisions/018-candidate-projection-ownership-and-telemetry-geospatial-supply.md) (consumer half); [ADR-005](../../decisions/005-transport-selection-by-flow-type.md); [ADR-009](../../decisions/009-protobuf-contracts-as-first-class-artifacts.md); [ADR-019](../../decisions/019-transport-agnostic-topic-naming.md). Skills: **`wolverine-handlers`** (base shape), **`wolverine-messaging-handlers`** (the governing skill for both new handlers), **`wolverine-kafka`** (transport wiring — and the one this session corrects), `marten-querying`, `transport-selection`, `protobuf-contracts`, `vertical-slice-organization`, `csharp-coding-standards`, `testing-integration`, `testing-fundamentals`, `service-bootstrap`, `aspire`. **`wolverine-marten-automation` does *not* govern this session** — see § Skill corrections. | diff --git a/docs/retrospectives/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md b/docs/retrospectives/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md new file mode 100644 index 0000000..eda15bc --- /dev/null +++ b/docs/retrospectives/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md @@ -0,0 +1,123 @@ +# Retrospective — Dispatch consumes `telemetry.driver-location-updated` (W006 slice 5 / W001 §5.3 close) + +## Metadata + +- **Triggering prompt:** [`docs/prompts/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md`](../../prompts/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md) +- **Status:** Complete +- **Date authored:** 2026-07-24 +- **Output artifacts:** + - `src/CritterCab.Dispatch/CritterCab.Dispatch.csproj` — Kafka + Protobuf + H3 packages; the repo's **first second consumer of an existing `.proto`** + - `src/CritterCab.Dispatch/AvailableDrivers/AvailableDriver.cs` — the document; `ILongVersioned`, two independently-written sides + - `src/CritterCab.Dispatch/AvailableDrivers/DriverLocationUpdatedHandler.cs` — **CritterCab's first cross-service consumer** + - `src/CritterCab.Dispatch/AvailableDrivers/DriverAvailabilityChanged.cs` — the ASB landing site with no transport bound to it, plus its handler + - `src/CritterCab.Dispatch/AvailableDrivers/NearbyAvailableDriversView.cs` — the adapter that replaced the stub + - `src/CritterCab.Dispatch/AvailableDrivers/H3KRing.cs` — k-derivation, ring enumeration, great-circle distance + - `src/CritterCab.Dispatch/Program.cs` — Kafka listener, `AvailableDriver` schema, source registration swap + - `apphost.cs` — Dispatch → Kafka reference + - `tests/CritterCab.Dispatch.Tests/DispatchKafkaTestFixture.cs` — broker-backed fixture with a warm-up handshake + - `tests/CritterCab.Dispatch.Tests/AvailableDrivers/` — three suites, 21 new tests + - `docs/skills/wolverine-kafka/SKILL.md` — listener sections rewritten from shipped code (the deferred DEBT row) + - `docs/skills/DEBT.md` — one row drained, three registered + - `docs/workshops/006-telemetry-event-model.md`, `docs/workshops/001-dispatch-event-model.md` — spec amendments +- **Tests:** 54/54 green (32 Dispatch, 22 Telemetry), up from 33. No pre-existing test modified. + +--- + +## Framing + +The last three sessions each built one side of a boundary. This one crossed it. Before PR D, "CritterCab has two services" meant two services that had never spoken; after it, a GPS ping entering Telemetry over gRPC comes out as a document write in Dispatch, over Kafka, in a different bounded context. That is the thing the project exists to demonstrate, and it took four PRs of groundwork to make it a small session. + +It was a small session. The two facts that made it so are worth naming, because both were deliberate choices made in *earlier* sessions: slice 5.3 built `INearbyAvailableDriversSource` as a port with a stub behind it, and PR B put `driver_location_updated.proto` through codegen before anything consumed it. So this session wrote an adapter and a handler, and `CandidateSelectionAutomation` — the code the whole slice exists to serve — does not appear in the diff. + +--- + +## Outcome summary + +W006's slice walk is **complete**; all five slices run. W001 §10 parking-lot #4 is closed in code three months after it closed on paper. `NearbyAvailableDriversStub` is out of the production graph, demoted to a test double, and the three W001 §5.3 GWTs plus W006 §6.3's Dedup GWT are exercised against a real broker. + +Two of ADR-005's three transports are live in both directions. Azure Service Bus remains the only modeled-but-unbuilt one, and the next thing it blocks is a bounded context, not a slice. + +--- + +## What worked + +**Running `jasperfx-source-verifier` at prompt-authoring time, again.** Second consecutive session where every gate closed before the deliverable plan existed, and **three of eight gate premises were contradicted by source**. Each contradiction would have produced working-looking wrong code: + +| Gate premise | Reality | +|---|---| +| A Kafka listener must declare its message type because protobuf cannot infer it | The default envelope mapper *writes* a `message-type` header; `DefaultIncomingMessage()` is hardening, not a workaround | +| `CustomizeHandlerDiscovery(...)` replaces the built-in conventions, so the consumer needs an `*Automation` name | Discovery is **additive** and OR'd; `*Handler` is discovered normally | +| Read-then-conditionally-`Store` is the best available LWW upsert | `TryUpdateRevision` is atomic, database-side, one round trip, and not racy | + +The third is the one that mattered most. "LWW upsert" in W006 §6.5 reads naturally as read-then-store, and read-then-store is *racy* — the window between the read and the write is exactly where a redelivery slips through. The spec was not wrong; the obvious implementation of it was. + +**Running `critter-skill-auditor` Phase 1 after the gates had all closed.** Also the second consecutive time it corrected an already-source-verified prompt, and again it found something source verification structurally cannot: **the prompt named the wrong governing skill.** `wolverine-marten-automation` is scoped to handlers reacting to a Marten-stream-forwarded event; neither new handler is that shape. `wolverine-kafka` says so itself — *"Kafka is a transport wire, not a handler shape"* — and defers to `wolverine-messaging-handlers`, which the prompt had not named. The two passes are not redundant and the order matters. + +**The seam-first discipline paid off twice in one session.** `INearbyAvailableDriversSource` (built in slice 5.3, with no real source in sight) and the pre-generated proto (PR B, with no consumer in sight) each turned what could have been a redesign into a registration change. Both were speculative when built. Both were right. + +**Deciding the availability half rather than defaulting it.** The tempting move — default a location-only driver to `Available`/`STANDARD` so the demo works end to end — would have had Dispatch fabricate a capability claim at the point of query, invisibly. Excluding instead makes the system honestly report that it cannot dispatch yet, which is true: ADR-018 says the view needs two feeders and only one exists. + +--- + +## What was harder than expected + +### A test caught a real geometry bug that no review would have + +The k-ring derivation was wrong in the dangerous direction, and it took a deliberately strengthened test to find it. + +`DeriveK` divided the search radius by the hexagon centre-to-centre spacing, `edge × √3`. That is the distance gained per hop **only when travelling along a lattice axis**. The axes are 60° apart, so a bearing falling between two of them advances less per hop — worst case by `cos(30°)`. The correct divisor is `edge × 1.5`. + +The measured consequence: a point 4,982 m due north of the test origin sits at grid distance **17**, and the original formula produced k=16. At Dispatch's production 5 km search radius, drivers near the edge would have been silently dropped from every candidate set. No exception, no log, no empty result — just slightly fewer candidates than there should be, which is indistinguishable from a quiet market. + +What found it was not the formula review. The first version of the coverage test asserted only the 1 km case, which passed under both formulas. Extending it to the production 5 km radius is what failed. **Two lessons:** pin geometric invariants at the value production actually uses, not at a convenient small one; and when a computation over-approximates on purpose, test the *edge* of the approximation, because that is the only place the error lives. + +A sharpener: pocketken's `GetHexagonEdgeLengthAverageInM` returns ~201 m at resolution 9, while H3's published tables say ~174 m. Those are different quantities — one is the edge of a regular hexagon of average *area*, the other averages the actual distorted edges — and they differ by exactly the `√3/2` this bug turned on. Two plausible numbers a factor apart, where the factor *is* the bug, is about as good a trap as geometry offers. + +### A production semantic hiding inside a test hang + +Four tests hung for 30 s and failed with "No activity detected." The instinct is to treat that as flake and add a retry. It was not flake: the listener declares no cold-start offset policy, so it inherited Confluent's `Latest`, and anything produced before the consumer group finished joining was legitimately behind the tail forever. + +The fix is a **production decision**, not a test fix, and it was escalated as one. `BeginAtLatest()` ships, on W006's own logic — §6.4 already evicts positions older than three heartbeats, so replaying retained history to reach the state one heartbeat produces in seconds is wasted work. The test cost is a warm-up handshake in the fixture, which retries a throwaway round trip until one is observably handled rather than sleeping a guessed interval. + +Worth noting how close this came to being resolved silently. `BeginAtEarliest()` would have turned all four tests green in one line and needed no fixture machinery — and would have committed the service to replaying its entire retained topic on every fresh deploy, as an invisible side effect of making a test pass. + +### `int` versus `long`, caught by arithmetic rather than by a test + +`TryUpdateRevision` takes a `long`, and the revision carries `serverReceivedAt` as unix-milliseconds — about 1.7 × 10¹². Marten's revision column comes in two widths, chosen by which interface the document implements: `IRevisioned` is `int`/`integer`, `ILongVersioned` is `long`/`bigint`. `IRevisioned` is the more familiar name and the wrong one here by a factor of about 800. + +This was caught by noticing the magnitude mismatch mid-write, not by a failing test — and it would **not** have failed loudly. A truncated revision still compares, still guards, and still looks like it works, on the wrong number. + +--- + +## Methodology refinements + +**A DEBT row blocked on code that does not exist yet is drained by the session that writes the code.** The `wolverine-kafka` listener row was deferred in PR C with an explicit condition — fix it from a real consumer — because replacing speculative names with differently speculative ones is not progress. PR D satisfied that condition, so the row drained *inside an implementation PR* rather than waiting for a `tidy: skills` session. Deferring further would have kept a known-wrong skill in place for no gain. This is a genuine refinement to the tidy-session convention, and it is now recorded in `DEBT.md`'s document history. + +**Verify a library's numbers, not just its API names.** Prior sessions established that our own skills carry non-compiling API claims. This one adds a subtler case: the API name was right, the call compiled, the return value was plausible, and the *semantics of the number* were not what the surrounding arithmetic assumed. Source verification as currently practised checks signatures. It does not check units, and units are where this session's real bug lived. + +**Strengthening a passing test is worth doing when the assertion is an approximation.** The coverage test passed. Extending it to production values broke it. There is no general rule that tests should be strengthened at random, but "this test asserts that an over-approximation is big enough" is a specific signal that the chosen sample matters. + +--- + +## Outstanding items / next-session inputs + +**The next session must be a design-or-tidy return.** PR D is the fourth consecutive implementation PR in this chain; ADR-019 served as the interleave for PR C and nothing covers this one. Per ADR-004's cadence rule, candidates in rough order of pull: + +1. **The Driver Profile workshop.** The ASB half of ADR-018 now blocks on it, this session deliberately declined to pre-empt its vocabulary, and it is the only thing standing between the current state and a genuinely end-to-end dispatch. +2. **A `tidy: skills` session.** Three rows registered this session join the standing backlog — and **three older rows are decisions, not cleanups**, and must not be drained by a routine tidy without a call: test-class naming (`Slice{N}Tests` vs. the skill's snake_case mandate), `testing-integration` Gap B (no shipped collection follows the documented Strategy 1, and this session added a **fourth** non-conforming collection, `DispatchKafka`), and the `identity-acl` streaming exception. +3. **CritterWatch.** It renders meaningfully only once real cross-service traffic exists, which is exactly what this PR created. Needs RabbitMQ as a tooling-only broker (ADR-017); trial licence expired 2026-07-10, so re-check before planning. + +**Still true and still unaddressed:** CI cannot build `apphost.cs`. It was edited again this session and verified by hand. That row remains open and remains its own session. + +--- + +## Spec delta — landed? + +**Yes, in full, plus three amendments the prompt anticipated in kind but not in specifics.** + +- **W006 §6.5 designed → realized.** ✅ Document, LWW upsert, k-ring query, stub replacement, all shipped. The slice walk closes; W006 is fully realized. +- **W006 §6.3's Dedup GWT exercised.** ✅ Against a real broker, plus a stale-redelivery case the GWT does not name but the revision guard makes free to assert. +- **W001 §5.3's amendment realized.** ✅ Recorded as a realization note under the existing 2026-06-30 design amendment — an amendment to an amendment, which is the right shape: the design decision did not change, its status did. +- **Three implementation-time qualifiers added to §6.5.** ✅ Availability-half exclusion, H3 resolution provenance, ETA derivation. All **amendments, not corrections** — §6.5 is silent on each rather than wrong about any, and each was resolved by user sign-off rather than absorbed. +- **One qualifier the prompt did not anticipate:** the `BeginAtLatest()` cold-start policy. Recorded in W006 for the same reason as the others — the spec is silent, the choice is durable, and the reasoning is W006's own. +- **§11 ADR candidates:** none fired. #2 (stream-processing as a fourth modeling shape) gained its second and sharper data point and stays later-arc, registered as skill DEBT rather than promoted. diff --git a/docs/skills/DEBT.md b/docs/skills/DEBT.md index 4f42ef0..39d44ac 100644 --- a/docs/skills/DEBT.md +++ b/docs/skills/DEBT.md @@ -71,12 +71,23 @@ This file is the working ledger between retros that surface gaps and the tidy se - **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). -### `wolverine-kafka` — listener/consumer-group/batching examples name a message type that was never published +### `protobuf-contracts` — the shared-type anti-pattern does not distinguish a published event contract from a borrowed value type -- **Gap:** The skill's § Listening, § Consumer groups, § Batch processing and § DLQ sections illustrate with `LocationPing` flowing through `telemetry.location-pings`. Neither exists: `LocationPing` is the gRPC **ingest** message (W006 §6.2) and is never a Kafka payload, and the shipped topic is `telemetry.driver-location-updated` carrying `DriverLocationUpdated`. These names are now actively misleading rather than merely speculative, because `LocationPing` is a real type in the codebase doing a *different* job — a session reading these sections would go looking for a Kafka handler that cannot exist. -- **Why it was not fixed in-session:** PR C corrected everything on the **publish** path (§ Topic naming, § Publishing, § Serialization, the mental model, and the pitfalls that restate them) under the session-runner-blocking exception, and left a scoped warning above the remaining sections. The listener half describes W006 slice 5, which **PR D builds** — refreshing those examples before the consumer exists would only swap speculative names for differently speculative ones. Fix them from the shipped consumer. -- **Fold in when drained:** § Consumer groups asserts "Dispatch and Pricing each get their own consumer group." Pricing is not a service; the fan-out is hypothetical. -- **Retro source:** [`retrospectives/implementations/008-telemetry-slice-3-kafka-publish.md`](../retrospectives/implementations/008-telemetry-slice-3-kafka-publish.md). +- **Gap:** § Anti-patterns names as wrong "defining a shared type inside a service's package" and importing it cross-BC (`GeoLocation` in `dispatch.v1`, pulled into `trips.v1`). PR D's csproj wiring has the same literal shape — Dispatch compiles `crittercab/telemetry/v1/driver_location_updated.proto` — but is a different relationship: Telemetry *publishes* `DriverLocationUpdated` as its outbound event contract (ADR-009, ADR-018) and Dispatch subscribes to it, which is what published language means. The skill draws no line between the two, so the wiring reads as a violation of its own guidance. +- **Why it was not fixed in-session:** the fix is a new distinction in a skill this session was otherwise only consuming, and it wants stating once for all future producer/consumer pairs rather than as a footnote to the first one. PR D put the reasoning in the csproj comment so the next reader is not left cold, but the skill is where it belongs. +- **Retro source:** [`retrospectives/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md`](../retrospectives/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md). + +### No skill covers a non-event-sourced document write path inside an **event-sourced** service + +- **Gap:** Already registered from Telemetry's `LastKnownPosition` (PR #45), but PR D is the sharper instance and worth noting against the existing row. Telemetry is stream-processing throughout, so a plain document there is unremarkable. **Dispatch is CritterCab's canonical event-sourced BC** — aggregates, inline projections, the whole decider apparatus — and now also carries `AvailableDriver`: a plain document, LWW-only, deliberately not a projection, sitting beside all of it. Nothing tells a session when that mixture is correct rather than a modelling mistake, and W006 §6.5's reasoning (event-sourcing an inbound high-volume feed would reimport the volume the producer's throttle exists to suppress) is the general rule but lives in a workshop. +- **Why it is worth a line:** the next session to consume a high-volume feed in an event-sourced BC will re-derive this from scratch, and the plausible wrong answer — "we event-source everything here, so make it a projection" — is the expensive one. +- **Retro source:** [`retrospectives/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md`](../retrospectives/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md). + +### No skill covers the "forward-constraint placeholder message" shape + +- **Gap:** `DriverAvailabilityChanged` is event-shaped (past tense, `domain-event-conventions` naming) but is appended to no Marten stream, so §6's `AddEventType()` registration does not apply; and it is not a real integration event either, because no transport is bound to it and only tests invoke it. Neither `domain-event-conventions` (Marten-stream-scoped) nor `wolverine-messaging-handlers` (assumes a transport) names this shape. PR D scoped it by mirroring W006 §6.5's document columns 1:1 so it describes the shape of the hole rather than guessing at an un-workshopped BC's vocabulary — a rule worth writing down. +- **Why it is worth a line rather than tolerating:** the pattern will recur every time a slice half-lands across a boundary, and without a rule the scoping discipline (mirror the locked view fields; invent no transitions) is re-derived or skipped. +- **Retro source:** [`retrospectives/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md`](../retrospectives/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md). ### `transport-selection` — no built-vs-modeled status axis @@ -113,6 +124,12 @@ This file is the working ledger between retros that surface gaps and the tidy se ## Recently drained +### 2026-07-24 — `wolverine-kafka` listener sections, from the shipped consumer (PR D) + +1 row drained *inside an implementation PR* rather than a `tidy: skills` session, which is the point of it: the row was deferred in PR C precisely because it could only be fixed honestly once a real consumer existed, and PR D is the session that built one. + +- **`wolverine-kafka` listener/consumer-group/batching examples named a message type that was never published.** § Listening rewritten around the shipped `DriverLocationUpdatedHandler` and the real `telemetry.driver-location-updated` listener config; § Consumer groups grounded in the verified `ConsumerConfig.GroupId ??= ServiceName` default plus the reason Cab pins it explicitly; § Batch processing re-illustrated and **marked as a mechanic Cab does not use**; the stale ⚠ banner above § Bootstrap removed. The "fold in when drained" note (a hypothetical Pricing consumer) is resolved. Three new subsections capture what building the consumer taught: handler discovery as a **precondition for deserialization**, `DefaultIncomingMessage()` as hardening rather than a requirement, and the `BeginAtLatest()`/`BeginAtEarliest()` cold-start choice with its testing consequence. Four new pitfalls. Retro at [`retrospectives/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md`](../retrospectives/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md). + ### 2026-07-02 — wolverine-marten-automation tidy 2 rows drained via [`prompts/skills-tidy-wolverine-marten-automation.md`](../prompts/skills-tidy-wolverine-marten-automation.md), authoring a new skill rather than extending an existing one: @@ -147,3 +164,4 @@ Older entries drop off; the retros and commits remain authoritative. - **2026-06-25.** Registered two at-threshold rows surfaced by retro 005 (slice 5.3) and carried in the 2026-06-16 post-slice-5.3 handoff: the marker-interface union return type and the event-triggered automation handler shape, both grouped under `wolverine-handlers` (or a possible new `wolverine-marten-automation` skill). Registering, not fixing — the fix is a future `tidy: skills` session. The **bundling-rule encoding** gap (also flagged past-threshold in retro 005 and the handoff) was deliberately *not* registered: neither source names a target skill, and this file's convention requires a row to name the skill. It stays for a session that can ground the target. - **2026-07-24 (PR C).** Registered six rows from the slice-3 Kafka session: `wolverine-kafka` listener-example refresh (deferred to PR D, which builds the consumer those examples describe), `transport-selection`'s missing built-vs-modeled status axis, the `aspire` skill's non-compiling `AddKafka` port example, and, from the Phase 2 audit and the two-axis code review, `service-bootstrap`'s undocumented optional connection-string guard, `testing-integration`'s two wrong Testcontainers API names plus its unfollowed collection convention, and the CI-cannot-build-`apphost.cs` gap. Two *other* `wolverine-kafka` gaps found the same session were **fixed in-flight** under the session-runner-blocking exception and do not appear here: its topic-naming section proposed a rule contradicting ADR-014/ADR-019, and its serialization section mandated JSON where the session ships protobuf — a session cannot follow a skill that contradicts the ADR it is authoring. - **2026-07-02.** Drained both 2026-06-25 rows via a new `docs/skills/wolverine-marten-automation/SKILL.md` skill (critter-skill-auditor Phase 1 discovery ruled out both `wolverine-handlers` and `marten-wolverine-aggregates` as bolt-on homes). `Open debt` reset to empty. Item 1 of the [post-W006 handoff](../planning/2026-07-02-post-w006-next-steps-handoff.md)'s ordered table. Retro at [`docs/retrospectives/skills-tidy-wolverine-marten-automation.md`](../retrospectives/skills-tidy-wolverine-marten-automation.md). +- **2026-07-24 (PR D).** Drained the `wolverine-kafka` listener row **inside an implementation PR**, which is a first for this file and worth naming as a pattern: the row was registered in PR C with an explicit "fix it from the shipped consumer" condition, and PR D is the session that satisfied that condition. A row whose fix is *blocked on code that does not exist yet* is drained by the session that writes the code, not by a later `tidy: skills` session — deferring it further would only have kept a known-wrong skill in place for no gain. Registered three new rows the same session: `protobuf-contracts`' undrawn line between a published event contract and a borrowed shared type, a second and sharper instance of the missing non-event-sourced-document-in-an-event-sourced-BC guidance, and the unnamed "forward-constraint placeholder message" shape. The three decision-class rows (test-class naming, `testing-integration` Gap B, `identity-acl`) were deliberately left alone — each needs a call, not a tidy. diff --git a/docs/skills/wolverine-kafka/SKILL.md b/docs/skills/wolverine-kafka/SKILL.md index a63dfa2..e88488e 100644 --- a/docs/skills/wolverine-kafka/SKILL.md +++ b/docs/skills/wolverine-kafka/SKILL.md @@ -52,9 +52,9 @@ Telemetry service Dispatch service └─────────────────┘ ``` -The Telemetry service publishes `DriverLocationUpdated` messages to a Kafka topic partitioned by `driverId`. Dispatch consumes it; a future Pricing service would consume the same topic under its own consumer group. Each handler is a plain messaging handler — it receives the message and does its work. The Kafka-specific concerns (partitioning, consumer groups, offsets) are configured in `Program.cs`, invisible to the handler. +The Telemetry service publishes `DriverLocationUpdated` messages to a Kafka topic partitioned by `driverId`. Dispatch consumes it into its `AvailableDriver` view; any future consumer would read the same topic under its own consumer group. Each handler is a plain messaging handler — it receives the message and does its work. The Kafka-specific concerns (partitioning, consumer groups, offsets, cold-start position) are configured in `Program.cs`, invisible to the handler. -> **⚠ Illustrative names below predate the implementation.** Only the shipped topic above and the § Topic naming, § Publishing and § Serialization sections have been reconciled against real code (2026-07-24). The listener, consumer-group, batching and DLQ examples further down still use a speculative `LocationPing` / `telemetry.location-pings` pairing that **does not exist**: `LocationPing` is the gRPC *ingest* message (W006 §6.2), never a Kafka payload, and nothing consumes this topic until W006 slice 5. Read those sections for the mechanic, not the names. A DEBT row tracks refreshing them once the consumer is real. +> **Reconciled against shipped code 2026-07-24.** Both halves of this topic now run: Telemetry publishes (PR C) and Dispatch consumes (PR D). Every named topic, message type and handler in this skill exists in the codebase. Where a section illustrates a mechanic Cab does **not** currently use — `ProcessInline`, batching, raw JSON, tombstones, custom envelope mappers — it says so inline. ## Bootstrap @@ -143,19 +143,57 @@ For surge-pricing demand signals, partitioning by `zone_id` keeps all demand eve ## Listening -### Single-topic listeners +> **Rewritten 2026-07-24 from shipped code (PR D).** This section and § Consumer groups previously illustrated with a `LocationPing` → `telemetry.location-pings` pairing that never existed — `LocationPing` is the gRPC *ingest* message (W006 §6.2) and is never a Kafka payload. Everything below is now the real Dispatch consumer. -`opts.ListenToKafkaTopic("...")` creates a consumer; messages route through the Wolverine handler pipeline as standard messaging handlers — the handler doesn't know it came from Kafka: +### The shipped listener, whole + +Cab's only Kafka listener lives in `CritterCab.Dispatch`. Read it as the reference for every clause: + +```csharp +opts.UseKafkaUsingNamedConnection("kafka"); // no AutoProvision — the producer owns the topic + +opts.ListenToKafkaTopic("telemetry.driver-location-updated") + .ConfigureConsumer(c => c.GroupId = "dispatch") + .BeginAtLatest() + .UseProtobufSerialization() + .DefaultIncomingMessage(); +``` ```csharp -public static class LocationPingHandler +public static class DriverLocationUpdatedHandler { - public static void Handle(LocationPing ping, ILogger logger) => - logger.LogDebug("Ping from driver {DriverId} at {Lat},{Lng}", - ping.DriverId, ping.Latitude, ping.Longitude); + public static async Task Handle( + DriverLocationUpdated message, IDocumentSession session, CancellationToken ct) + { /* ... */ } } ``` +The handler is a **vanilla Wolverine messaging handler** — nothing in it references Kafka, a topic, an offset or a partition. Handler *shape* is `wolverine-messaging-handlers`' subject, not this skill's. + +### Handler discovery is a precondition for deserialization + +The single most surprising failure mode, and the first thing to suspect when a listener goes quiet with **no error at all**. + +Wolverine resolves an incoming message's wire type name through `HandlerGraph._messageTypes`, which is populated *from discovered handler chains*. If no handler is discovered for a message type, the pipeline short-circuits to `NoHandlerContinuation` and the payload is **never deserialized** — so a handler that is made `internal`, renamed out of convention, or moved to an unscanned assembly does not produce a deserialization error. It produces silence. + +Note also that `CustomizeHandlerDiscovery(...)` is **additive**: Wolverine appends its built-in conventions (`*Handler`, `*Consumer`, `Saga`, `IWolverineHandler`, `[WolverineHandler]`) at bootstrap, *after* your customization, and the include filters are OR'd. Dispatch registers a `*Automation` suffix and still discovers `DriverLocationUpdatedHandler` by the built-in one. Only `DisableConventionalDiscovery()` changes this. + +### Declaring the incoming message type + +`.DefaultIncomingMessage()` pins the message type at the endpoint. It is **hardening, not a requirement**: Wolverine's default Kafka envelope mapper writes a `message-type` header on publish, so a Wolverine producer's type resolves from the wire without it. Declaring it *replaces* that header mapping with a constant, which makes the listener immune to a producer that omits or misspells the header — appropriate for a single-type topic, wrong for a shared one. + +There is **no** `ListenToKafkaTopic(...)` generic overload and no `.ReceivesMessage()` fluent method; `DefaultIncomingMessage()` is the API. + +### Cold-start position — `BeginAtLatest()` / `BeginAtEarliest()` + +Both apply **only when the consumer group has no committed offset**. Once the group commits, it resumes from its committed position and these are ignored. + +State one explicitly. `ConfigureConsumer` replaces the parent `ConsumerConfig`, so leaving it unset falls through to Confluent's default (`Latest`) rather than to anything Wolverine chose for you. + +Cab's telemetry consumer uses `BeginAtLatest()`: a stale position is worthless (W006 §6.4 evicts positions older than three heartbeats), and the heartbeat refills the view within `heartbeatIntervalSeconds` regardless — whereas `BeginAtEarliest()` would replay however many hours of retained telemetry the topic holds on a first deploy to arrive at the same state. Prefer `BeginAtEarliest()` instead when a topic carries facts that are *not* self-healing and a cold-start gap would lose them permanently. + +**Testing consequence:** under `BeginAtLatest()`, anything produced before the consumer group finishes joining is legitimately missed. Invisible in production; a guaranteed hang in a test that produces once and waits. Cab's `DispatchKafkaTestFixture` performs a warm-up handshake — producing throwaway records until one is observably handled — before any test runs. Do not paper over this with a fixed `Thread.Sleep`. + ### Multi-topic listeners (topic groups) For consuming several related topics from a single consumer (reducing rebalance churn), use `opts.ListenToKafkaTopics("...", "...")`. Each topic still routes to its own handler based on message type. Generic mechanic — see ai-skills `wolverine-integrations-kafka` § Topic binding. @@ -165,39 +203,43 @@ For consuming several related topics from a single consumer (reducing rebalance For streams like GPS pings where throughput matters more than durability guarantees, `ProcessInline()` bypasses the durable inbox and processes messages synchronously in the Kafka consumer loop: ```csharp -opts.ListenToKafkaTopic("telemetry.location-pings") +opts.ListenToKafkaTopic("telemetry.driver-location-updated") .ProcessInline(); ``` -Without `ProcessInline()`, Wolverine stores incoming messages in the durable inbox (the PostgreSQL or SQL Server-backed transactional inbox) before processing. That's the right default for domain events on ASB where reliability trumps throughput. For GPS pings arriving at hundreds per second per driver, the inbox write is unnecessary overhead — a lost ping is replaced by the next one in seconds. +Without `ProcessInline()`, Wolverine stores incoming messages in the durable inbox (the PostgreSQL or SQL Server-backed transactional inbox) before processing. That's the right default for domain events on ASB where reliability trumps throughput. For a throttled position feed, a lost message is replaced by the next heartbeat in seconds, so the inbox write buys little. + +**Cab's shipped listener does not use it.** `telemetry.driver-location-updated` is already throttled to cell-change-or-heartbeat, not raw GPS, so its volume does not justify giving up the inbox. Reach for `ProcessInline()` when a topic carries genuinely per-ping volume — and note the durability trade in § Common pitfalls. ### Batch processing -For handlers that benefit from processing many messages at once (aggregating GPS pings per driver, computing demand across a zone), use `opts.BatchMessagesOf()` paired with the listener: +For handlers that benefit from processing many messages at once, use `opts.BatchMessagesOf()` paired with the listener: ```csharp -opts.ListenToKafkaTopic("telemetry.location-pings"); -opts.BatchMessagesOf(); +opts.ListenToKafkaTopic("telemetry.driver-location-updated"); +opts.BatchMessagesOf(); -public static class LocationPingBatchHandler +public static class DriverLocationUpdatedBatchHandler { - public static void Handle(LocationPing[] pings, ILogger logger) + public static void Handle(DriverLocationUpdated[] updates, ILogger logger) { - var byDriver = pings.GroupBy(p => p.DriverId); + var byDriver = updates.GroupBy(u => u.DriverId); foreach (var group in byDriver) - logger.LogDebug("Batch of {Count} pings for driver {DriverId}", + logger.LogDebug("Batch of {Count} positions for driver {DriverId}", group.Count(), group.Key); } } ``` -Batch processing pairs naturally with high-volume Kafka topics where per-message invocation overhead is wasteful. +Batch processing pairs naturally with high-volume topics where per-message invocation overhead is wasteful. **Cab does not currently batch** — the shipped consumer handles one position at a time, because its per-message work is a single guarded upsert and batching would only complicate the last-writer-wins guard. Illustrated here as the mechanic, not as Cab's practice. ## Consumer groups ### Default group ID -Wolverine sets the Kafka consumer group ID to the **service name** (`WolverineOptions.ServiceName`) by default. In Cab, each service has a unique name, so Dispatch and Pricing each get their own consumer group on `telemetry.location-pings` automatically — no explicit configuration needed for the standard fan-out pattern. +Wolverine sets the Kafka consumer group ID to the **service name** by default — `ConsumerConfig.GroupId ??= runtime.Options.ServiceName` in `KafkaTransport`. Each Cab service has a unique name, so several services listening to `telemetry.driver-location-updated` would each get their own consumer group automatically, which is the standard fan-out pattern. + +**Cab's shipped listener pins it explicitly anyway** (`c.GroupId = "dispatch"`), and the reason is worth copying: a group id derived from the service name silently changes if the service is ever renamed, and a *new* group under `BeginAtLatest()` starts at the tail — quietly discarding the old group's committed position with no error. A literal is cheap insurance against an invisible offset reset. ### Transport-level override @@ -334,7 +376,13 @@ Wolverine's Kafka transport propagates OpenTelemetry trace context through Kafka - **Assuming a publish reached the broker when `PublishAsync` returns.** A Kafka publishing endpoint defaults to `BufferedInMemory`, which batches into an in-process queue and returns before the broker has seen anything. If a flow's correctness depends on the publish landing *before* some local write, `SendInline()` is required — and even inline is not enough on its own, because Wolverine's default async retry block swallows the send failure, logs it, re-posts to a background block and returns success. Pair `SendInline()` with `opts.Durability.UseSyncRetryBlock = true` (process-global) so a broker rejection actually throws. `telemetry.driver-location-updated` does exactly this for W006 §6.3's publish-before-store ordering. -- **Assuming ConfigureConsumer merges with the parent.** `ConfigureConsumer` on a per-topic listener **replaces** the parent `ConsumerConfig`. Bootstrap servers are auto-inherited, but other settings (SASL, timeouts) from the transport-level config are lost. Re-apply them in the per-topic override if needed. +- **Assuming ConfigureConsumer merges with the parent.** `ConfigureConsumer` on a per-topic listener **replaces** the parent `ConsumerConfig`. Bootstrap servers are auto-inherited, but other settings (SASL, timeouts) from the transport-level config are lost. Re-apply them in the per-topic override if needed. Note this is also why an unset cold-start position falls through to Confluent's default rather than to a Wolverine one — see § Listening. + +- **A silent listener is a discovery problem before it is a serialization problem.** If messages are demonstrably on the topic and nothing happens — no handler invocation, no exception, no dead letter — check that the handler is `public`, concrete, conventionally named, and in a scanned assembly *before* looking at serializers. Wolverine resolves the wire type name from discovered handler chains, so an undiscovered handler means the payload is never deserialized at all, and the failure mode is silence rather than an error. + +- **Producing before the consumer group has joined, in a test.** Under `BeginAtLatest()` (Cab's default for the telemetry feed) a record published before the group finishes joining is behind the tail and will never be delivered. Production never notices because the heartbeat republishes; a test hangs until its timeout. Warm the listener with a throwaway round trip first — and retry rather than sleeping, because group-join time varies with broker startup. + +- **Registering a document for `TryUpdateRevision` without `UseNumericRevisions(true)`.** The last-writer-wins guard only engages when the document is configured for numeric revisions; without it the call degrades to a plain unguarded upsert and stale redeliveries start overwriting fresh state, with no error anywhere. If the revision is a timestamp, the document must also implement `ILongVersioned` (`long`) rather than `IRevisioned` (`int`) — unix-milliseconds overflowed `int` in 1970. - **Using Kafka for domain events that need dead-lettering.** Kafka is append-only; dead-letter routing is a Wolverine-layer construct that produces to a separate topic. Azure Service Bus has native dead-letter queues with built-in inspection, replay, and session support. If your flow needs robust DLQ semantics, it probably belongs on ASB per `transport-selection`. diff --git a/docs/workshops/001-dispatch-event-model.md b/docs/workshops/001-dispatch-event-model.md index c622c74..83cea59 100644 --- a/docs/workshops/001-dispatch-event-model.md +++ b/docs/workshops/001-dispatch-event-model.md @@ -503,6 +503,15 @@ Then: NoCandidatesAvailable { rideRequestId: X, roundNumber: 1, searchParameters This **vindicates** the slice's original "No external call" framing: Option B (local projection) keeps it true; the rejected Option A (gRPC query) would have falsified it. Parking-lot #4 is closed; §11 ADR-candidate #3 is authored as ADR-018. +#### Realized in code (2026-07-24, PR D) + +The amendment above closed parking-lot #4 as a *design* decision. This entry records that it now **runs**. See [W006 §6.5's Document History entry](006-telemetry-event-model.md#document-history) for the full session record. + +- **`NearbyAvailableDrivers` exists** as per-driver `AvailableDriver` documents in `CritterCab.Dispatch`, maintained by a Wolverine Kafka handler consuming `telemetry.driver-location-updated`. Radius queries are an H3 k-ring over the published cell ids plus an exact great-circle filter, exactly as modelled. +- **`NearbyAvailableDriversStub` is out of the production graph**, demoted to a test double. `CandidateSelectionAutomation` is **unchanged** — the slice-5.3 `INearbyAvailableDriversSource` seam absorbed the entire swap, which is what it was built for. This slice's "No external call. Operates entirely on already-available views" is now a statement about running code. +- **The location half only.** The Translation-in sources table above lists two feeders; Kafka (Telemetry) is built and ASB (Driver Profile) is not, because that BC has not been workshopped. Dispatch therefore holds a `DriverAvailabilityChanged` handler with **no transport bound to it**, and **a driver with a position but no availability data is excluded from selection** — you cannot dispatch to a driver whose `vehicleClass` capability you have never been told. Until Driver Profile ships, that is every driver, so this slice's three GWTs are exercised against handler-seeded availability rather than a live feed. Not a gap in this slice: the availability half was always Driver Profile's to supply. +- **Two of this slice's locked decisions gained implementation-time detail** recorded in W006 rather than here, because they are properties of the view's population rather than of the selection decision: where the H3 query resolution comes from, and how `EtaSeconds` is produced by a view with no ETA source (the "Match-score algorithm" row above locks the *distance*; the ETA is derived from it against a constant invented at implementation time). + ### 5.4 Slice 4 — OfferSent (per-candidate broadcast) **Pattern:** Command Pattern applied per-candidate (drawn once on the board with "×N" annotation). diff --git a/docs/workshops/006-telemetry-event-model.md b/docs/workshops/006-telemetry-event-model.md index 2105342..6f4bdd0 100644 --- a/docs/workshops/006-telemetry-event-model.md +++ b/docs/workshops/006-telemetry-event-model.md @@ -661,3 +661,22 @@ Resume the paused W006 design (grill R1–R8, 2026-06-25) after signing off ADR- **§11 candidate #1 fired.** Its trigger was literally "first Kafka topic lands," so [ADR-019 — Transport-Agnostic Topic Naming](../decisions/019-transport-agnostic-topic-naming.md) ships in this PR, generalizing [ADR-014](../decisions/014-asb-topic-naming-convention.md)'s `.` across transports while explicitly **not** generalizing its two travelling operational decisions (ASB session keying and outbox coordination). ADR-019 also resolved a live contradiction the workshop had not seen: the `wolverine-kafka` skill had independently proposed a stream-descriptive naming rule for Kafka, giving the repo two conventions before either had a real topic to name. Candidate #2 (stream-processing as a fourth modeling shape) remains later-arc; #3 discharged as a skill in PR B. Slice 5 (the Dispatch consumer) remains the last pending slice — and with it §6.3's second GWT (Dedup), which asserts consumer behavior and could not be tested here. Session: [`prompts/implementations/008-telemetry-slice-3-kafka-publish.md`](../prompts/implementations/008-telemetry-slice-3-kafka-publish.md). +- **2026-07-24 (third entry, PR D)** — §6.5 (Slice 5) **realized in code**. **The slice walk is complete: all five W006 slices now run.** + + Dispatch consumes `telemetry.driver-location-updated` into per-driver `AvailableDriver` documents, and `NearbyAvailableDriversStub` is out of the production graph. **This is CritterCab's first cross-service flow** — a GPS ping entering Telemetry over gRPC now comes out the other side as a document write in a different service, over a different transport, in a different bounded context. It also closes W001 §10 parking-lot #4 in code, three months after the design closed it on paper. + + **§6.5's seam promise held exactly.** The slice-5.3 `INearbyAvailableDriversSource` port was built as a stub precisely so a real source could slot in without reshaping `CandidateSelectionAutomation` — and the automation does not appear in the PR diff. W001 §5.3's "No external call. Operates entirely on already-available views" survives intact, and survives *because* ADR-018 chose the local projection over the Telemetry gRPC query; the rejected option would have falsified that line. The stub is **demoted to a test double, not deleted**: the slice-5.2/5.3 suites still inject it through the fixture, which is why they are untouched. + + **Three under-specifications surfaced, all resolved by user sign-off. Amendments, not corrections — §6.5 is silent on each rather than wrong about any.** + + 1. **A driver with a location but no availability side is EXCLUDED from selection.** §6.5 defines the joined view but never says what a half-populated document means, and the ASB half is a forward-constraint to an un-workshopped Driver Profile BC — so *every* driver is in that state today. Excluding them is the semantically correct reading (you cannot dispatch to a driver whose capability you do not know) and it makes the honest consequence visible: a real end-to-end run yields `NoCandidatesAvailable` until Driver Profile ships. The rejected alternative — defaulting to `Available`/`STANDARD` on the Kafka upsert — would have kept a demo alive by fabricating a capability claim Dispatch has no source for, invisibly, at the point of query. A `DriverAvailabilityChanged` handler ships as the ASB landing site with **no transport bound to it**, scoped to exactly the four availability-side fields §6.5 locks so it describes the shape of the hole without pre-empting Driver Profile's vocabulary. + 2. **The H3 query resolution is read from the most recently ingested document**, not held as a Dispatch-side constant. §6.5 specifies a k-ring query but not where the resolution comes from, and the resolution is a `TelemetryPolicy` value Dispatch does not own. A constant would have been a cross-service coupling with no enforcement, where a Telemetry policy change breaks Dispatch's queries with no error at all — just an empty result that reads as "no drivers nearby." A policy roll now produces a brief mixed-resolution window, self-healed within one heartbeat, **which is the same accepted-v1-staleness argument §6.3 and §6.4 already make** for dropped publishes and for eviction. + 3. **`EtaSeconds` is derived from distance against an invented urban-speed constant.** The port returns an ETA; W001 §5.3 locks match-score as inverse straight-line distance and defers road-network ETA to a future gRPC counterparty, so the spec supplies a distance and no ETA source. **That constant is invented at implementation time, not specified by this workshop** — the same honest treatment PR B gave its 100m `accuracyMeters` threshold. + + **§6.3's Dedup GWT is finally exercised**, against a real broker. It could not be tested in PR C because it asserts *consumer* behaviour under at-least-once redelivery. The guard is Marten's `TryUpdateRevision`, whose `where mt_version < ?` clause discards a stale or duplicate delivery **server-side, in one statement** — strictly better than the read-then-conditionally-store the phrase "LWW upsert" suggests, which is two round trips with a race between them. Two traps worth recording: the guard silently does nothing unless the document is registered `UseNumericRevisions(true)`, and because the revision carries `serverReceivedAt` as unix-milliseconds the document must implement `ILongVersioned` (`long`) rather than `IRevisioned` (`int`) — a value that overflowed `int` in 1970. + + **One cold-start decision this workshop did not anticipate:** the listener declares `BeginAtLatest()`, so on a brand-new consumer group it starts at the tail rather than replaying retained history. Chosen on §6.4's own logic — a stale position is worthless, §6.4 actively evicts them, and the heartbeat refills the view within `heartbeatIntervalSeconds` — where `BeginAtEarliest()` would replay hours of telemetry to reach the same state. + + **§11 ADR candidates: none fired.** #2 (stream-processing as a fourth modeling shape) gains its **second and sharper data point** and remains later-arc: Telemetry is stream-processing throughout, so a plain document there is unremarkable, whereas Dispatch is CritterCab's canonical event-sourced BC and now carries a non-event-sourced LWW view beside its aggregates and projections. Registered as skill DEBT rather than promoted. + + **W006 is now fully realized.** What remains is not a W006 slice: the ASB half of ADR-018 belongs to Driver Profile's workshop. Session: [`prompts/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md`](../prompts/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md). diff --git a/src/CritterCab.Dispatch/Program.cs b/src/CritterCab.Dispatch/Program.cs index 914f5e1..33975ae 100644 --- a/src/CritterCab.Dispatch/Program.cs +++ b/src/CritterCab.Dispatch/Program.cs @@ -163,6 +163,11 @@ static void ConfigureKafkaListening(WolverineOptions opts) // distributes partitions across instances instead of delivering every position to all of // them. It is also what makes the redelivery-on-rebalance case real, which is precisely // what DriverLocationUpdatedHandler's revision guard defends against. + // + // Set explicitly even though Wolverine would default it to ServiceName — pinning it means a + // future rename of the service cannot silently create a NEW consumer group, which under + // BeginAtLatest below would start at the tail and quietly drop the group's committed + // position. A literal is cheap insurance against an invisible reset. .ConfigureConsumer(c => c.GroupId = "dispatch") // Cold-start position, and it ONLY applies when the group has no committed offset — after // the first commit the group resumes where it left off and this is ignored. Stated From 68da1d49f2f84c7a10d6de392859b8802651f753 Mon Sep 17 00:00:00 2001 From: Erik Shafer Date: Sat, 25 Jul 2026 00:28:03 -0500 Subject: [PATCH 4/8] Separate per-side LWW from write concurrency; code-review fixes --- docs/prompts/README.md | 2 +- ...h-w006-slice-5-nearby-available-drivers.md | 31 +++++- docs/skills/DEBT.md | 13 ++- docs/skills/wolverine-kafka/SKILL.md | 12 ++- docs/workshops/006-telemetry-event-model.md | 8 +- .../DriverAvailabilityChanged.cs | 48 +++++++--- .../DriverLocationUpdatedHandler.cs | 57 ++++++----- .../AvailableDrivers/H3KRing.cs | 28 +++++- src/CritterCab.Dispatch/Program.cs | 17 ++++ .../Slice5DriverLocationConsumerTests.cs | 94 +++++++++++++++++++ .../Slice5NearbyAvailableDriversViewTests.cs | 24 +++-- 11 files changed, 278 insertions(+), 56 deletions(-) diff --git a/docs/prompts/README.md b/docs/prompts/README.md index a947941..82dbb63 100644 --- a/docs/prompts/README.md +++ b/docs/prompts/README.md @@ -170,4 +170,4 @@ Subsequent sections are prompt-specific. Existing prompts in this directory serv - [`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). - [`implementations/008-telemetry-slice-3-kafka-publish.md`](./implementations/008-telemetry-slice-3-kafka-publish.md) — **CritterCab's first Kafka topic and second live transport.** W006 slice 3: `DriverLocationUpdated` published to `telemetry.driver-location-updated`, partitioned by `driverId`, dedup-keyed on `serverReceivedAt`, no outbox. Deliberately the smallest session that could make a transport real — PR #45 had already put the proto through codegen and typed the `IDriverLocationPublisher` seam against the generated `DriverLocationUpdated`, so this session swapped one implementation class and the host wiring; **`ReportLocationsHandler.cs` does not appear in the diff.** Also wires Aspire's Kafka resource (host port 5392, a slot the `aspire` skill had reserved before any broker existed) and a Kafka Testcontainer on its own fixture, so the slice-1/2/4 suites never wait on a broker. **The load-bearing finding is a gap between spec and library defaults** that the authoring-time `jasperfx-source-verifier` pass caught: W006 §6.3 argues publish-before-store from failure-mode asymmetry, but that argument silently assumes the publish's outcome is known when the store runs — and a Kafka publishing endpoint defaults to `BufferedInMemory` (returns before the broker acks), while even `SendInline()` alone is insufficient because Wolverine's default async retry block swallows the send failure and returns success. Resolved by user sign-off to `SendInline()` + process-global `Durability.UseSyncRetryBlock` + `UseIdempotentProducer()`, and recorded in W006 as an **amendment, not a correction** (§6.3's reasoning stands; it did not name its own precondition). Fires W006 §11 candidate #1 as **[ADR-019 — Transport-Agnostic Topic Naming](../decisions/019-transport-agnostic-topic-naming.md)**, generalizing ADR-014's `.` across transports while explicitly withholding its two ASB-specific operational clauses (session keying, outbox coordination) — which also serves as the ADR-004 design-return interleave for the third consecutive Telemetry implementation PR. **Second escalated fork:** `wolverine-kafka/SKILL.md` said Cab uses default JSON and deferred protobuf to "a future phase" — overridden to endpoint-scoped binary protobuf by user sign-off, since the payload is generated from the `.proto` ADR-009 makes the contract of record. That skill's topic-naming section had *also* independently proposed a rule contradicting ADR-014; both corrected in-PR under the session-runner-blocking exception, the listener half deferred to PR D as DEBT. **First prompt whose Verify-before-wiring gates all closed at authoring time**, and first whose `critter-skill-auditor` Phase 1 pass materially corrected an already-source-verified prompt (a missing governing skill, a wrong bootstrap lean, an already-allocated port, two skill-vs-spec contradictions). 33/33 green. Status: complete (authored + executed 2026-07-24). Produced retro at [`retrospectives/implementations/008-telemetry-slice-3-kafka-publish.md`](../retrospectives/implementations/008-telemetry-slice-3-kafka-publish.md). -- [`implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md`](./implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md) — **CritterCab's first cross-service flow**, and the session that completes W006's slice walk. W006 slice 5: Dispatch consumes `telemetry.driver-location-updated` into per-driver `AvailableDriver` documents, backs the slice-5.3 `INearbyAvailableDriversSource` port with an H3 k-ring query over them, and demotes `NearbyAvailableDriversStub` from production registration to test double — closing **W001 §10 parking-lot #4 in code** three months after the design closed it on paper. First code in `CritterCab.Dispatch` since prompt 005, and Dispatch's **first transport**: everything it did before was in-process Marten over HTTP. Also the first time two services compile the same `.proto` — deliberately not a shared assembly, which holds because Wolverine's message identity is `Type.FullName`-based and assembly-agnostic (the real coupling is the proto's `csharp_namespace`, not the assembly). `CandidateSelectionAutomation.cs` **does not appear in the diff**, which was the whole point of building the port as a seam in slice 5.3. **Second consecutive prompt whose Verify-before-wiring gates all closed at authoring time, and three of eight premises were contradicted by source** — most consequentially that Marten's `TryUpdateRevision` is an atomic single-statement LWW upsert, strictly better than the read-then-conditionally-store that "LWW upsert" naturally suggests and which would have been racy. `critter-skill-auditor` Phase 1 again corrected an already-source-verified prompt, this time catching that it **named the wrong governing skill** (`wolverine-marten-automation`, which is scoped to Marten-stream-forwarded events; replaced with `wolverine-handlers` + `wolverine-messaging-handlers`). Three durable forks resolved by user sign-off — availability-half exclusion with an unbound `DriverAvailabilityChanged` handler (the ASB half is a forward-constraint to an un-workshopped Driver Profile BC, and fabricating `Available`/`STANDARD` defaults was explicitly rejected), H3 query resolution read from the most recently ingested document rather than a Dispatch-side constant, and `EtaSeconds` derived from distance against a constant **invented at implementation time**. A **fourth** fork surfaced mid-session and was escalated rather than absorbed: the cold-start offset policy (`BeginAtLatest()`, chosen on W006 §6.4's own eviction logic, with a fixture warm-up handshake as the test cost — `BeginAtEarliest()` would have turned four failing tests green in one line while silently committing the service to replaying its entire retained topic on every fresh deploy). **A strengthened test caught a real geometry bug:** the k-ring derivation divided by the axis-aligned hexagon spacing (`edge × √3`) rather than the worst-case per-ring advance (`edge × 1.5`), so at Dispatch's production 5 km radius drivers near the edge were silently dropped — invisible at the 1 km value the first version of the test used. Also drains the `wolverine-kafka` listener DEBT row **inside an implementation PR**, establishing that a row blocked on code that does not exist yet is drained by the session that writes the code. 54/54 green. Status: complete (authored + executed 2026-07-24). Produced retro at [`retrospectives/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md`](../retrospectives/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md). +- [`implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md`](./implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md) — **CritterCab's first cross-service flow**, and the session that completes W006's slice walk. W006 slice 5: Dispatch consumes `telemetry.driver-location-updated` into per-driver `AvailableDriver` documents, backs the slice-5.3 `INearbyAvailableDriversSource` port with an H3 k-ring query over them, and demotes `NearbyAvailableDriversStub` from production registration to test double — closing **W001 §10 parking-lot #4 in code** three months after the design closed it on paper. First code in `CritterCab.Dispatch` since prompt 005, and Dispatch's **first transport**: everything it did before was in-process Marten over HTTP. Also the first time two services compile the same `.proto` — deliberately not a shared assembly, which holds because Wolverine's message identity is `Type.FullName`-based and assembly-agnostic (the real coupling is the proto's `csharp_namespace`, not the assembly). `CandidateSelectionAutomation.cs` **does not appear in the diff**, which was the whole point of building the port as a seam in slice 5.3. **Second consecutive prompt whose Verify-before-wiring gates all closed at authoring time, and three of eight premises were contradicted by source** — most consequentially that Marten's `TryUpdateRevision` is an atomic single-statement LWW upsert, strictly better than the read-then-conditionally-store that "LWW upsert" naturally suggests and which would have been racy. `critter-skill-auditor` Phase 1 again corrected an already-source-verified prompt, this time catching that it **named the wrong governing skill** (`wolverine-marten-automation`, which is scoped to Marten-stream-forwarded events; replaced with `wolverine-handlers` + `wolverine-messaging-handlers`). Three durable forks resolved by user sign-off — availability-half exclusion with an unbound `DriverAvailabilityChanged` handler (the ASB half is a forward-constraint to an un-workshopped Driver Profile BC, and fabricating `Available`/`STANDARD` defaults was explicitly rejected), H3 query resolution read from the most recently ingested document rather than a Dispatch-side constant, and `EtaSeconds` derived from distance against a constant **invented at implementation time**. A **fourth** fork surfaced mid-session and was escalated rather than absorbed: the cold-start offset policy (`BeginAtLatest()`, chosen on W006 §6.4's own eviction logic, with a fixture warm-up handshake as the test cost — `BeginAtEarliest()` would have turned four failing tests green in one line while silently committing the service to replaying its entire retained topic on every fresh deploy). **A strengthened test caught a real geometry bug:** the k-ring derivation divided by the axis-aligned hexagon spacing (`edge × √3`) rather than the worst-case per-ring advance (`edge × 1.5`), so at Dispatch's production 5 km radius drivers near the edge were silently dropped — invisible at the 1 km value the first version of the test used. A `code-review` two-axis pass then found the session's most serious defect, after the source-verifier and BOTH auditor passes had run clean, with **both axes converging on it independently**: the implementation used one shared Marten revision column where W006 §6.5 locks "LWW per driver **per side**", so a heartbeat position stamped after an availability transition would silently discard it and leave an **offline driver dispatchable**. Fixed by separating business ordering (per-side timestamp comparison) from write concurrency (an incrementing revision plus a `ConcurrencyException` retry policy) — and the regression tests were themselves proven by reverting the old behaviour, after a first version of them passed against both implementations. Also drains the `wolverine-kafka` listener DEBT row **inside an implementation PR**, establishing that a row blocked on code that does not exist yet is drained by the session that writes the code. 57/57 green. Status: complete (authored + executed 2026-07-24). Produced retro at [`retrospectives/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md`](../retrospectives/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md). diff --git a/docs/retrospectives/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md b/docs/retrospectives/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md index eda15bc..160c588 100644 --- a/docs/retrospectives/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md +++ b/docs/retrospectives/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md @@ -19,7 +19,8 @@ - `docs/skills/wolverine-kafka/SKILL.md` — listener sections rewritten from shipped code (the deferred DEBT row) - `docs/skills/DEBT.md` — one row drained, three registered - `docs/workshops/006-telemetry-event-model.md`, `docs/workshops/001-dispatch-event-model.md` — spec amendments -- **Tests:** 54/54 green (32 Dispatch, 22 Telemetry), up from 33. No pre-existing test modified. +- **Tests:** 57/57 green (35 Dispatch, 22 Telemetry), up from 33. No pre-existing test modified. +- **Review passes run:** `jasperfx-source-verifier` (authoring time, 8 gates), `critter-skill-auditor` Phase 1 (pre-code) and Phase 2 (post-code), `code-review` two-axis (pre-PR). **The last of these found the session's most serious defect, after the first three had run clean** — see § What was harder than expected. --- @@ -61,6 +62,29 @@ The third is the one that mattered most. "LWW upsert" in W006 §6.5 reads natura ## What was harder than expected +### The most serious defect survived two auditor passes and a source-verification pass + +`code-review`'s two-axis form found it, and **both axes found it independently** — which is the strongest signal the separation is worth its cost. + +W006 §6.5 locks *"Eventual, **LWW per driver per side** — location LWW on `serverReceivedAt`, availability LWW on **its own ordering key**."* The implementation used a single Marten revision column carrying whichever side wrote last. That conflates two genuinely different problems: + +- **business ordering** — which update is newer, *per side*, on that side's own clock; +- **write concurrency** — did anyone change the row between my read and my write. + +One column cannot serve both when there are two clocks. The concrete failure: a driver goes Offline at 12:00:05, a heartbeat position stamped 12:00:07 arrives first and raises the revision, and the Offline write is then discarded by `where mt_version < ?` — **leaving an offline driver dispatchable, with no error anywhere.** A second, subtler failure sat underneath it: both handlers load-then-write the whole document, and the revision guarded the revision rather than the freshness of the read, so the two sides could lost-update each other. + +The fix separates the two concerns, which is what §6.5 described all along: each handler compares **its own side's timestamp** against the stored one (business LWW, and equality is the dedup no-op §6.3 wants), and the revision becomes a plain incrementing concurrency token via `UpdateRevision`, with a Wolverine `OnException().RetryWithCooldown(...)` policy re-running the handler against fresh state. + +**Why the earlier passes could not have caught it.** `jasperfx-source-verifier` answered "what is the best Marten API for an LWW upsert" correctly — `TryUpdateRevision` genuinely is that, for *one* writer. `critter-skill-auditor` checks conventions, and nothing about the code was unconventional. The defect lived in the gap between a correct API and a spec clause about *two* writers, which is exactly the seam a spec-axis reviewer reads and an API verifier does not. + +**Methodology consequence worth carrying:** when a source-verification gate asks "what is the best API for X," the answer is scoped to the question's implicit cardinality. This gate asked about an upsert and got an answer about *an* upsert. The spec said "per side," and nobody re-read that clause against the chosen primitive until the review. + +### Regression tests must be proven to fail + +Having written the fix, I wrote two regression tests, and they passed. That is not evidence — a test that passes on both the broken and fixed implementation pins nothing. + +Reverting the availability handler alone still passed, because the location handler's new small sequential revisions made the old code's huge timestamp revision always win; the original defect needed *both* handlers on timestamp revisions. Only after reverting both did the two tests fail, and then pass again on restore. **The first version of the "regression" test was a false witness**, and a five-minute revert-and-rerun was what distinguished it from a real one. + ### A test caught a real geometry bug that no review would have The k-ring derivation was wrong in the dangerous direction, and it took a deliberately strengthened test to find it. @@ -95,6 +119,8 @@ This was caught by noticing the magnitude mismatch mid-write, not by a failing t **Verify a library's numbers, not just its API names.** Prior sessions established that our own skills carry non-compiling API claims. This one adds a subtler case: the API name was right, the call compiled, the return value was plausible, and the *semantics of the number* were not what the surrounding arithmetic assumed. Source verification as currently practised checks signatures. It does not check units, and units are where this session's real bug lived. +**A verified gate can go stale during the session that verified it, and the prompt is the wrong place to fix it.** Gate 8 recorded `Rings.GetKRing` as the k-ring API. That was true of the *public surface* and false of the *guidance*: `GetKRing` is `[Obsolete]` as of H3 4.0 in favour of `GridDiskDistances`, which the compiler said on first build and which the shipped code follows. The gate table was deliberately left as authored — a prompt is a historical record of intent at session start, not a living document, so the correction belongs here and in the code comment that carries it. Worth naming as a shape: **reflection-probed API gates report existence, not deprecation**, so a probe-based gate should check for `[Obsolete]` explicitly or expect the compiler to be the real reviewer. + **Strengthening a passing test is worth doing when the assertion is an approximation.** The coverage test passed. Extending it to production values broke it. There is no general rule that tests should be strengthened at random, but "this test asserts that an over-approximation is big enough" is a specific signal that the chosen sample matters. --- @@ -104,7 +130,7 @@ This was caught by noticing the magnitude mismatch mid-write, not by a failing t **The next session must be a design-or-tidy return.** PR D is the fourth consecutive implementation PR in this chain; ADR-019 served as the interleave for PR C and nothing covers this one. Per ADR-004's cadence rule, candidates in rough order of pull: 1. **The Driver Profile workshop.** The ASB half of ADR-018 now blocks on it, this session deliberately declined to pre-empt its vocabulary, and it is the only thing standing between the current state and a genuinely end-to-end dispatch. -2. **A `tidy: skills` session.** Three rows registered this session join the standing backlog — and **three older rows are decisions, not cleanups**, and must not be drained by a routine tidy without a call: test-class naming (`Slice{N}Tests` vs. the skill's snake_case mandate), `testing-integration` Gap B (no shipped collection follows the documented Strategy 1, and this session added a **fourth** non-conforming collection, `DispatchKafka`), and the `identity-acl` streaming exception. +2. **A `tidy: skills` session.** Four rows registered this session join the standing backlog — and **three older rows are decisions, not cleanups**, and must not be drained by a routine tidy without a call: test-class naming (`Slice{N}Tests` vs. the skill's snake_case mandate), `testing-integration` Gap B (no shipped collection follows the documented Strategy 1, and this session added a **fourth** non-conforming collection, `DispatchKafka`), and the `identity-acl` streaming exception. 3. **CritterWatch.** It renders meaningfully only once real cross-service traffic exists, which is exactly what this PR created. Needs RabbitMQ as a tooling-only broker (ADR-017); trial licence expired 2026-07-10, so re-check before planning. **Still true and still unaddressed:** CI cannot build `apphost.cs`. It was edited again this session and verified by hand. That row remains open and remains its own session. @@ -119,5 +145,6 @@ This was caught by noticing the magnitude mismatch mid-write, not by a failing t - **W006 §6.3's Dedup GWT exercised.** ✅ Against a real broker, plus a stale-redelivery case the GWT does not name but the revision guard makes free to assert. - **W001 §5.3's amendment realized.** ✅ Recorded as a realization note under the existing 2026-06-30 design amendment — an amendment to an amendment, which is the right shape: the design decision did not change, its status did. - **Three implementation-time qualifiers added to §6.5.** ✅ Availability-half exclusion, H3 resolution provenance, ETA derivation. All **amendments, not corrections** — §6.5 is silent on each rather than wrong about any, and each was resolved by user sign-off rather than absorbed. +- **One limitation shipped as an explicit deferral, recorded in W006:** an availability transition for a driver Dispatch has never seen a position for is **dropped, not buffered**, and does not self-heal. Closing it requires a decision that belongs to Driver Profile's contract. Pinned by a test so it cannot change silently. - **One qualifier the prompt did not anticipate:** the `BeginAtLatest()` cold-start policy. Recorded in W006 for the same reason as the others — the spec is silent, the choice is durable, and the reasoning is W006's own. - **§11 ADR candidates:** none fired. #2 (stream-processing as a fourth modeling shape) gained its second and sharper data point and stays later-arc, registered as skill DEBT rather than promoted. diff --git a/docs/skills/DEBT.md b/docs/skills/DEBT.md index 39d44ac..1a4ca81 100644 --- a/docs/skills/DEBT.md +++ b/docs/skills/DEBT.md @@ -89,6 +89,12 @@ This file is the working ledger between retros that surface gaps and the tidy se - **Why it is worth a line rather than tolerating:** the pattern will recur every time a slice half-lands across a boundary, and without a rule the scoping discipline (mirror the locked view fields; invent no transitions) is re-derived or skipped. - **Retro source:** [`retrospectives/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md`](../retrospectives/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md). +### `csharp-coding-standards` — § Geospatial Values mandates a `GeoLocation` type that does not exist + +- **Gap:** § Geospatial Values says the agent "should NOT pass raw `(double, double)` tuples around. Use a single `GeoLocation` record everywhere," and shows an implementation. **No `GeoLocation` exists in the codebase.** Dispatch has `Shared/Location.cs` (`record Location(double Lat, double Lon, string? StreetAddress)`), Telemetry has no equivalent, and Telemetry's shipped `H3CellIndexer.TryComputeCell(double, double, int)` takes raw degrees — as does PR D's `H3KRing`, which deliberately mirrored it so the two services compute identical cell ids. The skill therefore describes a convention with zero adherents and one named-but-different type. +- **Why it was not fixed in-session:** introducing `GeoLocation` means either a shared type across two services (which the structural constraints forbid) or a per-BC one plus a translation at the boundary — a modelling decision, not a cleanup. Converging `H3KRing` alone would also have required editing Telemetry's `H3CellIndexer`, an out-of-scope file. **Decide the shape first:** per-BC value objects with boundary translation (consistent with the BC-owned-enums rule), or accept raw degrees at the H3 binding layer and scope the rule to domain surfaces only. +- **Retro source:** [`retrospectives/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md`](../retrospectives/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md) (surfaced by the two-axis code review). + ### `transport-selection` — no built-vs-modeled status axis - **Gap:** The skill's § Phasing table has a "Lands when" column but no per-row status a session can flip when a transport actually ships. As of PR C two of three transports are **built** (gRPC serving traffic, Kafka publishing) and one remains **modeled only** (Azure Service Bus) — and nothing in the skill distinguishes them, so a reader cannot tell which guidance is grounded in shipped code and which is still design intent. That distinction bites precisely where the skill gets used: choosing a transport for a new flow. @@ -98,7 +104,8 @@ This file is the working ledger between retros that surface gaps and the tidy se ### `testing-integration` — two wrong API names, and a collection convention no shipped fixture follows - **Gap A (wrong API, costs a compile error):** § Testcontainers patterns shows `WithPullPolicy(PullPolicy.Missing)`. On Testcontainers 4.13.0 the builder method is **`WithImagePullPolicy`**, and `PullPolicy` lives in **`DotNet.Testcontainers.Images`**, not `DotNet.Testcontainers.Configurations`. Copying the skill's line verbatim fails with CS1061 + CS0103; PR C did exactly that and had to source the real names from the package's XML docs. -- **Gap B (convention vs. reality):** § Parallelization *Strategy 1 (Cab default)* specifies `[CollectionDefinition(Name, DisableParallelization = true)]` with a `public const string Name`. **None of the three shipped collections do this** — `Dispatch`, `Telemetry`, and now `TelemetryKafka` all use a bare string literal with no `DisableParallelization`. This is a repo-wide deviation, not a slice-local one, so PR C deliberately did not "fix" only its own collection: doing so would have deepened exactly the kind of inconsistency the test-class-naming row above warns about. **Decide, then apply everywhere** — either the fixtures adopt Strategy 1, or the skill records that Cab runs collections in parallel and relies on per-fixture container isolation (which is what it actually does today, and what PR C's unique container names now make safe). +- **Gap B (convention vs. reality):** § Parallelization *Strategy 1 (Cab default)* specifies `[CollectionDefinition(Name, DisableParallelization = true)]` with a `public const string Name`. **None of the four shipped collections do this** — `Dispatch`, `Telemetry`, `TelemetryKafka` and, as of PR D, `DispatchKafka` all use a bare string literal with no `DisableParallelization`. This is a repo-wide deviation, not a slice-local one, so PR C deliberately did not "fix" only its own collection and PR D followed that precedent: doing so would have deepened exactly the kind of inconsistency the test-class-naming row above warns about. **Decide, then apply everywhere** — either the fixtures adopt Strategy 1, or the skill records that Cab runs collections in parallel and relies on per-fixture container isolation (which is what it actually does today, and what PR C's unique container names now make safe). +- **Gap B companion (registered 2026-07-24, PR D):** the same skill's canonical per-test cleanup shape — `IAsyncLifetime.InitializeAsync() => _fixture.CleanAllMartenDataAsync()` — is followed by **zero** test classes in the repo, Dispatch or Telemetry, old or new. Every suite instead calls a fixture reset inline as the first statement of each `[Fact]`. Structurally the same kind of gap as Gap B (a documented convention with no adherents) and it wants deciding in the same session, so it is filed here rather than as a row of its own. PR D followed the shipped precedent rather than introducing a fifth style. - **Related, and already fixed:** PR C added `.WithName($"...-{Guid.NewGuid():N}")` to both Telemetry fixtures. The skill's own pitfall ("Sharing a Testcontainer name across fixtures without `Guid.NewGuid()`") was correct and was being violated; it went unnoticed while the project had only one container and became live when slice 3 added a second Postgres. - **Retro source:** [`retrospectives/implementations/008-telemetry-slice-3-kafka-publish.md`](../retrospectives/implementations/008-telemetry-slice-3-kafka-publish.md) (surfaced by the two-axis code review). @@ -128,7 +135,7 @@ This file is the working ledger between retros that surface gaps and the tidy se 1 row drained *inside an implementation PR* rather than a `tidy: skills` session, which is the point of it: the row was deferred in PR C precisely because it could only be fixed honestly once a real consumer existed, and PR D is the session that built one. -- **`wolverine-kafka` listener/consumer-group/batching examples named a message type that was never published.** § Listening rewritten around the shipped `DriverLocationUpdatedHandler` and the real `telemetry.driver-location-updated` listener config; § Consumer groups grounded in the verified `ConsumerConfig.GroupId ??= ServiceName` default plus the reason Cab pins it explicitly; § Batch processing re-illustrated and **marked as a mechanic Cab does not use**; the stale ⚠ banner above § Bootstrap removed. The "fold in when drained" note (a hypothetical Pricing consumer) is resolved. Three new subsections capture what building the consumer taught: handler discovery as a **precondition for deserialization**, `DefaultIncomingMessage()` as hardening rather than a requirement, and the `BeginAtLatest()`/`BeginAtEarliest()` cold-start choice with its testing consequence. Four new pitfalls. Retro at [`retrospectives/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md`](../retrospectives/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md). +- **`wolverine-kafka` listener/consumer-group/batching examples named a message type that was never published.** § Listening rewritten around the shipped `DriverLocationUpdatedHandler` and the real `telemetry.driver-location-updated` listener config; § Consumer groups grounded in the verified `ConsumerConfig.GroupId ??= ServiceName` default plus the reason Cab pins it explicitly; § Batch processing and § Dead letter topics re-illustrated and **marked as mechanics Cab does not use** (the DLQ section gained the reasoning: a self-healing position feed makes a dead-lettered message worth less than the cost of a topic to inspect); the stale ⚠ banner above § Bootstrap removed. The "fold in when drained" note (a hypothetical Pricing consumer) is resolved. Three new subsections capture what building the consumer taught: handler discovery as a **precondition for deserialization**, `DefaultIncomingMessage()` as hardening rather than a requirement, and the `BeginAtLatest()`/`BeginAtEarliest()` cold-start choice with its testing consequence. Four new pitfalls. Retro at [`retrospectives/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md`](../retrospectives/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md). ### 2026-07-02 — wolverine-marten-automation tidy @@ -164,4 +171,4 @@ Older entries drop off; the retros and commits remain authoritative. - **2026-06-25.** Registered two at-threshold rows surfaced by retro 005 (slice 5.3) and carried in the 2026-06-16 post-slice-5.3 handoff: the marker-interface union return type and the event-triggered automation handler shape, both grouped under `wolverine-handlers` (or a possible new `wolverine-marten-automation` skill). Registering, not fixing — the fix is a future `tidy: skills` session. The **bundling-rule encoding** gap (also flagged past-threshold in retro 005 and the handoff) was deliberately *not* registered: neither source names a target skill, and this file's convention requires a row to name the skill. It stays for a session that can ground the target. - **2026-07-24 (PR C).** Registered six rows from the slice-3 Kafka session: `wolverine-kafka` listener-example refresh (deferred to PR D, which builds the consumer those examples describe), `transport-selection`'s missing built-vs-modeled status axis, the `aspire` skill's non-compiling `AddKafka` port example, and, from the Phase 2 audit and the two-axis code review, `service-bootstrap`'s undocumented optional connection-string guard, `testing-integration`'s two wrong Testcontainers API names plus its unfollowed collection convention, and the CI-cannot-build-`apphost.cs` gap. Two *other* `wolverine-kafka` gaps found the same session were **fixed in-flight** under the session-runner-blocking exception and do not appear here: its topic-naming section proposed a rule contradicting ADR-014/ADR-019, and its serialization section mandated JSON where the session ships protobuf — a session cannot follow a skill that contradicts the ADR it is authoring. - **2026-07-02.** Drained both 2026-06-25 rows via a new `docs/skills/wolverine-marten-automation/SKILL.md` skill (critter-skill-auditor Phase 1 discovery ruled out both `wolverine-handlers` and `marten-wolverine-aggregates` as bolt-on homes). `Open debt` reset to empty. Item 1 of the [post-W006 handoff](../planning/2026-07-02-post-w006-next-steps-handoff.md)'s ordered table. Retro at [`docs/retrospectives/skills-tidy-wolverine-marten-automation.md`](../retrospectives/skills-tidy-wolverine-marten-automation.md). -- **2026-07-24 (PR D).** Drained the `wolverine-kafka` listener row **inside an implementation PR**, which is a first for this file and worth naming as a pattern: the row was registered in PR C with an explicit "fix it from the shipped consumer" condition, and PR D is the session that satisfied that condition. A row whose fix is *blocked on code that does not exist yet* is drained by the session that writes the code, not by a later `tidy: skills` session — deferring it further would only have kept a known-wrong skill in place for no gain. Registered three new rows the same session: `protobuf-contracts`' undrawn line between a published event contract and a borrowed shared type, a second and sharper instance of the missing non-event-sourced-document-in-an-event-sourced-BC guidance, and the unnamed "forward-constraint placeholder message" shape. The three decision-class rows (test-class naming, `testing-integration` Gap B, `identity-acl`) were deliberately left alone — each needs a call, not a tidy. +- **2026-07-24 (PR D).** Drained the `wolverine-kafka` listener row **inside an implementation PR**, which is a first for this file and worth naming as a pattern: the row was registered in PR C with an explicit "fix it from the shipped consumer" condition, and PR D is the session that satisfied that condition. A row whose fix is *blocked on code that does not exist yet* is drained by the session that writes the code, not by a later `tidy: skills` session — deferring it further would only have kept a known-wrong skill in place for no gain. Registered four new rows the same session: `protobuf-contracts`' undrawn line between a published event contract and a borrowed shared type, a second and sharper instance of the missing non-event-sourced-document-in-an-event-sourced-BC guidance, the unnamed "forward-constraint placeholder message" shape, and `csharp-coding-standards`' `GeoLocation` mandate for a type that does not exist. Also folded a companion note into the standing `testing-integration` Gap B row (its per-test cleanup convention has no adherents either) and corrected that row's collection count, which was stale by one. The three decision-class rows (test-class naming, `testing-integration` Gap B, `identity-acl`) were deliberately left alone — each needs a call, not a tidy. diff --git a/docs/skills/wolverine-kafka/SKILL.md b/docs/skills/wolverine-kafka/SKILL.md index e88488e..1917850 100644 --- a/docs/skills/wolverine-kafka/SKILL.md +++ b/docs/skills/wolverine-kafka/SKILL.md @@ -207,9 +207,15 @@ opts.ListenToKafkaTopic("telemetry.driver-location-updated") .ProcessInline(); ``` -Without `ProcessInline()`, Wolverine stores incoming messages in the durable inbox (the PostgreSQL or SQL Server-backed transactional inbox) before processing. That's the right default for domain events on ASB where reliability trumps throughput. For a throttled position feed, a lost message is replaced by the next heartbeat in seconds, so the inbox write buys little. +**Get the baseline right first: a Kafka listener is NOT durable by default.** `Endpoint` defaults to `EndpointMode.BufferedInMemory`, and the Kafka transport does not override it. So an un-configured listener already buffers in memory and can lose in-flight messages on a crash. The durable inbox is opt-*in* via `UseDurableInbox()`, not opt-out via `ProcessInline()`. The three modes: -**Cab's shipped listener does not use it.** `telemetry.driver-location-updated` is already throttled to cell-change-or-heartbeat, not raw GPS, so its volume does not justify giving up the inbox. Reach for `ProcessInline()` when a topic carries genuinely per-ping volume — and note the durability trade in § Common pitfalls. +| Mode | How | Behaviour | +|---|---|---| +| `BufferedInMemory` | **the default** — nothing to configure | Queued in-process, handled on a worker; in-flight messages lost on crash | +| `ProcessInline()` | explicit | Handled on the Kafka consumer loop itself; no queue, back-pressure straight to the broker | +| `UseDurableInbox()` | explicit | Written to the PostgreSQL/SQL Server inbox before handling; survives a crash | + +**Cab's shipped listener configures none of them**, so it runs buffered — which is the honest default for this flow: `telemetry.driver-location-updated` carries positions the heartbeat reproduces within `heartbeatIntervalSeconds` (W006 §6.4), so paying for an inbox write to protect a fact that regenerates itself is poor value. Reach for `UseDurableInbox()` when a topic carries facts that do *not* regenerate — and note that per `transport-selection` such a flow may belong on ASB in the first place. ### Batch processing @@ -307,6 +313,8 @@ Wolverine ships `SchemaRegistryAvroSerializer` and `SchemaRegistryJsonSerializer Kafka has no built-in DLQ. Wolverine implements dead-letter routing as a separate Kafka topic; opt in per listener with `.EnableNativeDeadLetterQueue()`. Override the default name (`wolverine-dead-letter-queue`) globally with `.DeadLetterQueueTopicName("crittercab-dlq")`. Wolverine stamps four diagnostic headers on dead-lettered messages (`exception-type`, `exception-message`, `exception-stack`, `failed-at`). See ai-skills `wolverine-integrations-kafka` § Dead letter queue. +**Cab's shipped listener does not enable it**, and the reason generalizes: `telemetry.driver-location-updated` carries positions that are self-healing by design — a dropped message is superseded by the next heartbeat within `heartbeatIntervalSeconds` (W006 §6.4), so a dead-lettered position is worth strictly less than the operational cost of a topic to inspect and replay. Enable the native DLT when a topic carries facts that are **not** reproducible on a timer, and note that per `transport-selection` such a flow may belong on ASB in the first place — see § Common pitfalls. + ### Retry policies Retry and error-handling policies are Wolverine-native, not Kafka-specific. Combine retries with dead-letter routing via `opts.Policies.OnException().RetryTimes(N).Then.MoveToErrorQueue()`. Without `EnableNativeDeadLetterQueue()` on the listener, `MoveToErrorQueue` routes to Wolverine's database-backed dead-letter storage instead of the Kafka DLT. See ai-skills `wolverine-messaging-resiliency-policies` for the full retry/DLQ surface. diff --git a/docs/workshops/006-telemetry-event-model.md b/docs/workshops/006-telemetry-event-model.md index 6f4bdd0..1dd212d 100644 --- a/docs/workshops/006-telemetry-event-model.md +++ b/docs/workshops/006-telemetry-event-model.md @@ -673,7 +673,13 @@ Resume the paused W006 design (grill R1–R8, 2026-06-25) after signing off ADR- 2. **The H3 query resolution is read from the most recently ingested document**, not held as a Dispatch-side constant. §6.5 specifies a k-ring query but not where the resolution comes from, and the resolution is a `TelemetryPolicy` value Dispatch does not own. A constant would have been a cross-service coupling with no enforcement, where a Telemetry policy change breaks Dispatch's queries with no error at all — just an empty result that reads as "no drivers nearby." A policy roll now produces a brief mixed-resolution window, self-healed within one heartbeat, **which is the same accepted-v1-staleness argument §6.3 and §6.4 already make** for dropped publishes and for eviction. 3. **`EtaSeconds` is derived from distance against an invented urban-speed constant.** The port returns an ETA; W001 §5.3 locks match-score as inverse straight-line distance and defers road-network ETA to a future gRPC counterparty, so the spec supplies a distance and no ETA source. **That constant is invented at implementation time, not specified by this workshop** — the same honest treatment PR B gave its 100m `accuracyMeters` threshold. - **§6.3's Dedup GWT is finally exercised**, against a real broker. It could not be tested in PR C because it asserts *consumer* behaviour under at-least-once redelivery. The guard is Marten's `TryUpdateRevision`, whose `where mt_version < ?` clause discards a stale or duplicate delivery **server-side, in one statement** — strictly better than the read-then-conditionally-store the phrase "LWW upsert" suggests, which is two round trips with a race between them. Two traps worth recording: the guard silently does nothing unless the document is registered `UseNumericRevisions(true)`, and because the revision carries `serverReceivedAt` as unix-milliseconds the document must implement `ILongVersioned` (`long`) rather than `IRevisioned` (`int`) — a value that overflowed `int` in 1970. + **§6.3's Dedup GWT is finally exercised**, against a real broker. It could not be tested in PR C because it asserts *consumer* behaviour under at-least-once redelivery. + + **§6.5's "LWW per driver per side" is load-bearing, and the session initially got it wrong.** The first implementation used a single Marten revision column carrying whichever side wrote last — which conflates *business ordering* (which update is newer, per side, on that side's clock) with *write concurrency* (did anyone change the row between my read and my write). One column cannot serve both when there are two clocks, and the failure is a real dispatch bug: a driver going Offline at 12:00:05 has that write silently discarded by a heartbeat position stamped 12:00:07 that already raised the revision, **leaving an offline driver dispatchable with no error anywhere**. Caught by a two-axis code review after two convention audits and an API-verification pass had run clean. The shipped design separates the two, exactly as §6.5 describes: each handler compares its own side's timestamp (equality being the dedup no-op §6.3 specifies), and the revision is a plain incrementing concurrency token with a Wolverine retry on `ConcurrencyException`. **The workshop was right and the implementation had to catch up** — no amendment needed here beyond recording that the clause has teeth. + + Two Marten traps worth recording regardless: the revision guard silently does nothing unless the document is registered `UseNumericRevisions(true)`, and a document whose revision could ever carry a millisecond timestamp must implement `ILongVersioned` (`long`) rather than `IRevisioned` (`int`) — a value that overflowed `int` in 1970. + + **One limitation shipped as an explicit deferral.** An availability transition for a driver Dispatch has never seen a position for is **dropped, not buffered** — and it does not self-heal, because the heartbeat creates the document with a null availability side. Closing it means deciding something that belongs to Driver Profile's un-workshopped contract (a republish-current-state endpoint, or a Dispatch-side buffer for unmatched transitions). §6.5 chose neither because it assumed both feeders existed. **Recorded here as a forward-constraint on the Driver Profile workshop**, and pinned by a test so the behaviour cannot change silently. **One cold-start decision this workshop did not anticipate:** the listener declares `BeginAtLatest()`, so on a brand-new consumer group it starts at the tail rather than replaying retained history. Chosen on §6.4's own logic — a stale position is worthless, §6.4 actively evicts them, and the heartbeat refills the view within `heartbeatIntervalSeconds` — where `BeginAtEarliest()` would replay hours of telemetry to reach the same state. diff --git a/src/CritterCab.Dispatch/AvailableDrivers/DriverAvailabilityChanged.cs b/src/CritterCab.Dispatch/AvailableDrivers/DriverAvailabilityChanged.cs index df875aa..543e6a2 100644 --- a/src/CritterCab.Dispatch/AvailableDrivers/DriverAvailabilityChanged.cs +++ b/src/CritterCab.Dispatch/AvailableDrivers/DriverAvailabilityChanged.cs @@ -45,16 +45,38 @@ public static async Task Handle( { var existing = await session.LoadAsync(message.DriverId, ct); - // An availability event can legitimately arrive before Dispatch has ever seen a position - // for this driver — a driver who comes on shift indoors, or whose first ping is still in - // Telemetry's throttle window. There is no location side to write yet, and inventing one - // (0,0 is in the Gulf of Guinea) would put a real driver in a real k-ring somewhere. Drop - // it: Telemetry publishes on the driver's first cell change or heartbeat regardless, so - // the document appears within one heartbeat interval, carrying this state if it arrives - // first. Consistent with W006's heartbeat-as-backstop reasoning throughout. + // An availability event can arrive before Dispatch has ever seen a position for this driver + // — a driver who comes on shift indoors, or whose first ping is still inside Telemetry's + // throttle window. There is no location side to write, and inventing one is not an option + // (0,0 is in the Gulf of Guinea, and a fabricated position would put a real driver in a + // real k-ring). + // + // BE PRECISE ABOUT WHAT HAPPENS HERE: the event is DROPPED, not deferred. Telemetry's + // heartbeat will create the document within heartbeatIntervalSeconds, but it creates it + // with a null availability side — the location handler carries forward whatever it finds, + // and it finds nothing. So the driver stays excluded from selection until Driver Profile + // sends its NEXT transition, which for a driver who simply came on shift and stayed on + // shift may be hours. + // + // Shipped as an explicit deferral rather than solved, because solving it means deciding + // something that belongs to Driver Profile's un-workshopped contract: either it republishes + // current state on demand (a snapshot/replay endpoint), or Dispatch buffers unmatched + // availability events and applies them when a position lands. Both are real designs; W006 + // §6.5 chose neither because it assumed both feeders existed. Recorded as a forward- + // constraint on that workshop; pinned by a test so the behaviour cannot change silently. if (existing is null) return; + // The AVAILABILITY side's last-writer-wins guard, on its OWN clock (W006 §6.5: "LWW per + // driver per side"). Mirror image of the location handler's guard — and deliberately not + // the document revision, which cannot order two independent clocks. See that handler for + // the dispatch bug this separation exists to prevent. + if (existing.AvailabilityUpdatedAt is not null + && message.AvailabilityUpdatedAt <= existing.AvailabilityUpdatedAt) + { + return; + } + var updated = existing with { AvailabilityState = message.AvailabilityState, @@ -62,14 +84,10 @@ public static async Task Handle( AvailabilityUpdatedAt = message.AvailabilityUpdatedAt }; - // Same database-side LWW guard as the location side, on this side's own clock. Note both - // sides share ONE revision column, so an availability update and a location update compete - // for it. That is acceptable while the availability feed is unbuilt and low-rate, and it is - // the first thing to revisit when ASB lands: if the two clocks interleave under load, the - // sides need independent guards rather than one shared revision. - var revision = Math.Max(1, message.AvailabilityUpdatedAt.ToUnixTimeMilliseconds()); - - session.TryUpdateRevision(updated, revision); + // Concurrency guard only, exactly as on the location side: this closes the window between + // the LoadAsync above and this write, so the two handlers cannot lost-update each other's + // side. A losing race throws and Wolverine retries the handler against fresh state. + session.UpdateRevision(updated, existing.Version + 1); await session.SaveChangesAsync(ct); } diff --git a/src/CritterCab.Dispatch/AvailableDrivers/DriverLocationUpdatedHandler.cs b/src/CritterCab.Dispatch/AvailableDrivers/DriverLocationUpdatedHandler.cs index 86f7705..029d51b 100644 --- a/src/CritterCab.Dispatch/AvailableDrivers/DriverLocationUpdatedHandler.cs +++ b/src/CritterCab.Dispatch/AvailableDrivers/DriverLocationUpdatedHandler.cs @@ -29,7 +29,14 @@ public static async Task Handle( IDocumentSession session, CancellationToken ct) { - var driverId = Guid.Parse(message.DriverId); + // Guard the wire before trusting it. proto3 has no required fields, so a malformed or + // partially-populated record deserializes happily with "" and null defaults — Guid.Parse + // would throw FormatException and ServerReceivedAt would NRE, both of which dead-letter a + // message that is simply not addressed to us. Drop instead, matching W006 §6.2's treatment + // of invalid pings on the producing side: bad input is a value to branch on, not an error. + if (!Guid.TryParse(message.DriverId, out var driverId) || message.ServerReceivedAt is null) + return; + var serverReceivedAt = message.ServerReceivedAt.ToDateTimeOffset(); // Read the existing document to preserve the availability side. The two sides have separate @@ -39,6 +46,24 @@ public static async Task Handle( // roughly as often as it follows one. var existing = await session.LoadAsync(driverId, ct); + // The LOCATION side's last-writer-wins guard, compared against the location side's OWN + // clock — W006 §6.5 locks "LWW per driver per side", and the two sides are ordered + // independently because they arrive from different services over different transports. + // + // This is deliberately NOT the document's revision. An earlier cut of this handler used + // serverReceivedAt AS the Marten revision, which collapsed the two sides onto one ordering + // key and produced a real dispatch bug: a driver going Offline at 12:00:05 could have that + // write silently discarded by a heartbeat position stamped 12:00:07 that had already raised + // the revision — leaving an offline driver dispatchable, with no error anywhere. Business + // ordering and write concurrency are two different problems and need two different guards. + // + // Equality is a no-op, not an update: that IS §6.3's dedup semantics ("the projection + // applies the position at most once"). A strictly-older timestamp is the redelivery case + // that actually regresses the view — on a consumer-group rebalance, uncommitted offsets + // replay, so an older position can arrive after a newer one was already applied. + if (existing is not null && serverReceivedAt <= existing.ServerReceivedAt) + return; + var updated = new AvailableDriver { Id = driverId, @@ -54,28 +79,14 @@ public static async Task Handle( AvailabilityUpdatedAt = existing?.AvailabilityUpdatedAt }; - // The dedup/LWW guard W006 §6.3 asks for, enforced in the database rather than in this - // method. TryUpdateRevision emits a single upsert whose WHERE clause compares the stored - // revision, so a stale or duplicate delivery is discarded server-side and silently — no - // exception, no second round trip, and no window between a read and a write for a - // concurrent delivery to slip through. - // - // Why this matters despite the partition key: Kafka partitions by driverId, so per-driver - // ordering IS guaranteed in steady state and a naive Store() would usually be fine. It is - // redelivery that breaks the assumption — on a consumer-group rebalance, uncommitted offsets - // replay, so an OLDER position can arrive after a newer one was already applied. That is a - // real regression of the view, and the guard makes it free to prevent. - // - // The revision is unix-MILLISECONDS of the server-stamped receipt time, because Marten's - // revision is a monotonic long and not a timestamp. Two consequences: - // - Equal timestamps are a no-op, which is exactly the dedup semantics §6.3 specifies - // ("the projection applies the position at most once"). - // - Revision 0 means "always win" to Marten, so a zero-valued timestamp would defeat the - // guard entirely. Unreachable in practice (Telemetry server-stamps every publish), but - // the floor costs one Math.Max and removes the failure mode. - var revision = Math.Max(1, serverReceivedAt.ToUnixTimeMilliseconds()); - - session.TryUpdateRevision(updated, revision); + // The document revision guards CONCURRENCY, not business ordering: it closes the window + // between the LoadAsync above and this write, during which the availability handler could + // have committed its own side. UpdateRevision throws ConcurrencyException on a losing race + // rather than swallowing it, and Program.cs registers a Wolverine retry policy that runs + // the handler again — reloading, re-evaluating the LWW guard, and merging against fresh + // state. Without this the two handlers would lost-update each other: both load at revision + // N, both write the whole document, and whichever commits second erases the other's side. + session.UpdateRevision(updated, (existing?.Version ?? 0) + 1); await session.SaveChangesAsync(ct); } diff --git a/src/CritterCab.Dispatch/AvailableDrivers/H3KRing.cs b/src/CritterCab.Dispatch/AvailableDrivers/H3KRing.cs index d97312f..881c8c1 100644 --- a/src/CritterCab.Dispatch/AvailableDrivers/H3KRing.cs +++ b/src/CritterCab.Dispatch/AvailableDrivers/H3KRing.cs @@ -41,6 +41,12 @@ public static class H3KRing // advances per ring. H3KRingTests pins the resulting k values against measured grid distances. private const double MetersPerRingPerEdge = 1.5; + // ~3,700 cells at the boundary (3k(k+1)+1). Comfortably above anything a sane + // resolution/radius pairing produces — resolution 9 at 5km needs 18 — and far below the point + // where a single `= ANY` array becomes the problem. See DeriveK for why this refuses rather + // than clamps. + private const int MaxRingRadius = 35; + // Computes the H3 cell containing a point, at the given resolution. Same Coordinate path as // Telemetry's H3CellIndexer, and it must stay the same path: a cell id computed differently // here would not match the ids Telemetry publishes, and the join would silently return nothing. @@ -103,8 +109,28 @@ internal static int DeriveK(int resolution, int radiusMeters) return 0; var metersPerRing = edgeMeters * MetersPerRingPerEdge; + var k = (int)Math.Ceiling(radiusMeters / metersPerRing) + 1; + + // A grid disk holds 3k(k+1)+1 cells, so k grows the array quadratically — and k itself is + // driven by a resolution Dispatch does not control (it is read off whatever Telemetry last + // published). At resolution 9 a 5km radius is k=18 and ~1,000 cells; at resolution 12 the + // same radius is k≈369 and ~410,000 cells, which would be materialised into a single + // `= ANY` parameter and would take the query down rather than return slowly. + // + // Refuse loudly instead of degrading. This is a misconfiguration — a telemetry resolution + // that fine is not a tuning choice, it is a mistake — and an exception names it at the one + // moment someone can act on it. Returning a truncated k would silently under-cover, which + // is the failure mode this whole method is written to avoid. + if (k > MaxRingRadius) + { + throw new InvalidOperationException( + $"An H3 k-ring of {k} rings is required to cover {radiusMeters}m at resolution " + + $"{resolution}, which exceeds the {MaxRingRadius}-ring ceiling. The cell " + + "resolution published by Telemetry is too fine for this search radius — widen " + + "the TelemetryPolicy resolution or narrow the radius."); + } - return (int)Math.Ceiling(radiusMeters / metersPerRing) + 1; + return k; } // Great-circle distance in metres between two degree-denominated points. Used for the exact diff --git a/src/CritterCab.Dispatch/Program.cs b/src/CritterCab.Dispatch/Program.cs index 33975ae..550303c 100644 --- a/src/CritterCab.Dispatch/Program.cs +++ b/src/CritterCab.Dispatch/Program.cs @@ -4,8 +4,11 @@ using CritterCab.Dispatch.RideRequesting; using CritterCab.Telemetry.V1; using JasperFx; +using JasperFx.Core; using Marten; +using Marten.Exceptions; using JasperFx.Events.Projections; +using Wolverine.ErrorHandling; using Wolverine; using Wolverine.Http; using Wolverine.Kafka; @@ -54,6 +57,10 @@ .Duplicate(x => x.H3Cell) .Duplicate(x => x.VehicleClass) .Duplicate(x => x.AvailabilityState) + // Not part of the ring predicate, but the view sorts on it to derive the H3 query + // resolution — and an un-duplicated sort key means a full scan plus a JSONB sort over + // the whole fleet before every dispatch. + .Duplicate(x => x.ServerReceivedAt) // Load-bearing, and silent if omitted. TryUpdateRevision only applies its // `where mt_version < ?` guard when the document is registered for numeric revisions; // without this it degrades to a plain unguarded upsert and stale Kafka redeliveries @@ -129,6 +136,16 @@ // suffix without appearing here. Only DisableConventionalDiscovery() would change that. opts.Discovery.CustomizeHandlerDiscovery(d => d.Includes.WithNameSuffix("Automation")); + // AvailableDriver has two independent writers (the Kafka location handler and the availability + // handler), each of which loads the document, merges its own side, and writes the whole thing + // back. Marten's revision guard turns a losing race into a ConcurrencyException rather than a + // silent lost update — this policy is the other half of that: retry the handler, which reloads + // and re-merges against the winner's state. Three attempts with short pauses, because the + // contention window is a single round trip and a conflict that survives three retries is a + // symptom of something else. + opts.Policies.OnException() + .RetryWithCooldown(50.Milliseconds(), 100.Milliseconds(), 250.Milliseconds()); + // Guarded rather than early-returned, so a broker-less run cannot silently swallow any // Wolverine configuration appended after this line. if (kafkaEnabled) diff --git a/tests/CritterCab.Dispatch.Tests/AvailableDrivers/Slice5DriverLocationConsumerTests.cs b/tests/CritterCab.Dispatch.Tests/AvailableDrivers/Slice5DriverLocationConsumerTests.cs index c81b46e..95c5101 100644 --- a/tests/CritterCab.Dispatch.Tests/AvailableDrivers/Slice5DriverLocationConsumerTests.cs +++ b/tests/CritterCab.Dispatch.Tests/AvailableDrivers/Slice5DriverLocationConsumerTests.cs @@ -145,6 +145,100 @@ await bus.InvokeAsync(new DriverAvailabilityChanged driver.Lat.ShouldBe(LoopLat); } + // The regression test for the defect a two-axis code review found: with ONE revision column + // shared by both sides, a heartbeat position stamped later than an availability transition + // would silently discard that transition — leaving an offline driver dispatchable, with no + // error anywhere. Per-side LWW is what W006 §6.5 locks, and this is what it buys. + [Fact] + public async Task an_availability_transition_is_applied_even_when_a_later_position_arrived_first() + { + await _fixture.ResetDriversAsync(); + + var driverId = Guid.CreateVersion7(); + var t = DateTimeOffset.UtcNow; + + // The exact interleaving that broke under a single shared revision column: a position whose + // timestamp is AHEAD of the availability transition lands FIRST. Under the old scheme that + // position set the revision to t+10s, and the Offline transition stamped t+5s then failed + // the `mt_version < ?` guard and was discarded server-side — silently leaving an offline + // driver dispatchable. Per-side LWW compares the transition against the AVAILABILITY + // side's own clock, which is empty here, so it applies. + await ProduceAndWaitAsync(PositionOf(driverId, LoopLat, LoopLon, t)); + await ProduceAndWaitAsync(PositionOf(driverId, LoopLat, LoopLon, t.AddSeconds(10))); + + await ApplyAvailabilityAsync(driverId, DriverAvailabilityState.Offline, t.AddSeconds(5)); + + var driver = await _fixture.LoadDriverAsync(driverId); + + driver.ShouldNotBeNull(); + driver.AvailabilityState.ShouldBe(DriverAvailabilityState.Offline); + + // And the location side kept the newest position — neither write clobbered the other. + driver.ServerReceivedAt.ToUnixTimeMilliseconds() + .ShouldBe(t.AddSeconds(10).ToUnixTimeMilliseconds()); + } + + // Mirror image: a stale availability transition must not win over a newer one just because it + // arrived later. Each side is ordered on its own clock. + [Fact] + public async Task a_stale_availability_transition_never_overwrites_a_newer_one() + { + await _fixture.ResetDriversAsync(); + + var driverId = Guid.CreateVersion7(); + var now = DateTimeOffset.UtcNow; + + await ProduceAndWaitAsync(PositionOf(driverId, LoopLat, LoopLon, now)); + await ApplyAvailabilityAsync(driverId, DriverAvailabilityState.Offline, now); + await ApplyAvailabilityAsync(driverId, DriverAvailabilityState.Available, now.AddSeconds(-30)); + + var driver = await _fixture.LoadDriverAsync(driverId); + + driver.ShouldNotBeNull(); + driver.AvailabilityState.ShouldBe(DriverAvailabilityState.Offline); + } + + // Pins a known, deliberate limitation rather than a desired behaviour — see + // DriverAvailabilityChangedHandler for why it is a deferral and what closing it would require + // from Driver Profile. Here so the drop cannot start or stop happening silently. + [Fact] + public async Task an_availability_event_for_an_unseen_driver_is_dropped_not_buffered() + { + await _fixture.ResetDriversAsync(); + + var driverId = Guid.CreateVersion7(); + var at = DateTimeOffset.UtcNow; + + // Availability first, with no position ever received for this driver. + await ApplyAvailabilityAsync(driverId, DriverAvailabilityState.Available, at); + + (await _fixture.LoadDriverAsync(driverId)).ShouldBeNull(); + + // A position arrives later. The availability side does NOT reappear — the earlier event is + // gone, not queued. + await ProduceAndWaitAsync(PositionOf(driverId, LoopLat, LoopLon, at.AddSeconds(5))); + + var driver = await _fixture.LoadDriverAsync(driverId); + + driver.ShouldNotBeNull(); + driver.AvailabilityState.ShouldBeNull(); + } + + private async Task ApplyAvailabilityAsync( + Guid driverId, DriverAvailabilityState state, DateTimeOffset at) + { + using var scope = _fixture.Host.Services.CreateScope(); + var bus = scope.ServiceProvider.GetRequiredService(); + + await bus.InvokeAsync(new DriverAvailabilityChanged + { + DriverId = driverId, + AvailabilityState = state, + VehicleClass = VehicleClass.Standard, + AvailabilityUpdatedAt = at + }); + } + // Produces the record and waits for Dispatch's listener to finish handling it. // // WaitForMessageToBeReceivedAt is the API that waits on an ARRIVAL. IncludeExternalTransports() diff --git a/tests/CritterCab.Dispatch.Tests/AvailableDrivers/Slice5NearbyAvailableDriversViewTests.cs b/tests/CritterCab.Dispatch.Tests/AvailableDrivers/Slice5NearbyAvailableDriversViewTests.cs index d03c9c9..29f3494 100644 --- a/tests/CritterCab.Dispatch.Tests/AvailableDrivers/Slice5NearbyAvailableDriversViewTests.cs +++ b/tests/CritterCab.Dispatch.Tests/AvailableDrivers/Slice5NearbyAvailableDriversViewTests.cs @@ -24,6 +24,7 @@ public class Slice5NearbyAvailableDriversViewTests // test is not sensitive to the exact great-circle arithmetic. private const double PickupLat = 41.8827d, PickupLon = -87.6233d; private const double NearLat = 41.8850d, NearLon = -87.6250d; + private const double MidLat = 41.8950d, MidLon = -87.6300d; private const double FarLat = 41.9400d, FarLon = -87.6900d; private const int Resolution = 9; private const int RadiusMeters = 2_000; @@ -37,23 +38,30 @@ public async Task drivers_in_range_and_capable_are_returned_nearest_first() { await _fixture.ResetDriversAsync(); - var near = await AvailableDriverAt(NearLat, NearLon, VehicleClass.Standard); - var far = await AvailableDriverAt(FarLat, FarLon, VehicleClass.Standard); + // Two drivers inside the radius at different distances, so the ORDERING is actually + // exercised — seeded farther-first so a missing OrderBy returns them the wrong way round + // rather than accidentally right. Plus one outside it, to prove the trim. + var nearer = await AvailableDriverAt(NearLat, NearLon, VehicleClass.Standard); + var farther = await AvailableDriverAt(MidLat, MidLon, VehicleClass.Standard); + var outside = await AvailableDriverAt(FarLat, FarLon, VehicleClass.Standard); var found = await QueryAsync(RadiusMeters, VehicleClass.Standard); - // The far driver is inside the k-ring — the ring deliberately over-approximates — and is - // trimmed by the exact-distance filter. That is the assertion that proves the two-stage + // Nearest first — W006 §6.5's "exact-distance ranked". + found.Select(d => d.DriverId).ShouldBe([nearer, farther]); + found[0].DistanceMeters.ShouldBeLessThan(found[1].DistanceMeters); + + // The outside driver is inside the k-ring — the ring deliberately over-approximates — and + // is trimmed by the exact-distance filter. That is the assertion that proves the two-stage // query works rather than the ring accidentally being tight enough. - found.Select(d => d.DriverId).ShouldBe([near]); - found[0].DistanceMeters.ShouldBeLessThan(RadiusMeters); + found.Select(d => d.DriverId).ShouldNotContain(outside); + found[1].DistanceMeters.ShouldBeLessThan(RadiusMeters); // ETA is derived from distance against an invented urban-speed constant (W006 §6.5 fork 3). // Asserted as a relationship, not a value: pinning the number here would freeze a // placeholder that a real ETA service is meant to replace. found[0].EtaSeconds.ShouldBeGreaterThan(0); - - far.ShouldNotBe(near); + found[0].EtaSeconds.ShouldBeLessThan(found[1].EtaSeconds); } [Fact] From ea198cada77166a1fdd75e05173f2f7d8e25708a Mon Sep 17 00:00:00 2001 From: Erik Shafer Date: Sat, 25 Jul 2026 00:44:49 -0500 Subject: [PATCH 5/8] ci: pre-pull Testcontainers images so parallel assemblies don't race Docker Hub --- .github/workflows/dotnet.yml | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/.github/workflows/dotnet.yml b/.github/workflows/dotnet.yml index 94ab3a6..1737ceb 100644 --- a/.github/workflows/dotnet.yml +++ b/.github/workflows/dotnet.yml @@ -69,6 +69,31 @@ jobs: - name: Build run: dotnet build CritterCab.slnx --no-restore --configuration Release + # Pull every Testcontainers image serially BEFORE the test run, so nothing is fetched from + # Docker Hub while tests are starting. + # + # Why this is needed: xUnit runs the two test assemblies in parallel, and each one stands up + # its own Testcontainers session — a Ryuk resource reaper plus a Postgres per fixture plus a + # Kafka broker. Once Dispatch grew its own Kafka-backed fixture (W006 slice 5) that became six + # containers and two reapers racing to pull at once, and the runner's connection to + # registry-1.docker.io started timing out mid-pull. The symptom is misleading: the failure + # surfaces as DockerApiException "context deadline exceeded" inside + # ResourceReaper.GetAndStartNewAsync, in whichever suite happened to lose the race, so it + # reads like a broken test rather than a saturated network. + # + # After this step the images are local and the fixtures' PullPolicy.Missing makes every pull a + # no-op. Keep this list in sync with the tags pinned in the fixtures — a tag that drifts out + # of sync here does not fail the build, it just quietly restores the old behaviour. + # + # The ryuk tag is NOT ours to choose: Testcontainers pins it internally (4.13.0 → 0.14.0, + # by digest). Re-read it from the package when Testcontainers is bumped, otherwise this line + # pre-pulls an image nothing uses and the reaper goes back to racing for its own. + - name: Pre-pull Testcontainers images + run: | + docker pull postgres:18-alpine + docker pull confluentinc/cp-kafka:7.6.1 + docker pull testcontainers/ryuk:0.14.0 + - name: Test run: > dotnet test CritterCab.slnx From d5fcaf90b18883ee5456c4c46ea01f13400e0d78 Mon Sep 17 00:00:00 2001 From: Erik Shafer Date: Sat, 25 Jul 2026 00:51:55 -0500 Subject: [PATCH 6/8] Record CI pre-pull finding in retro and DEBT --- ...patch-w006-slice-5-nearby-available-drivers.md | 15 ++++++++++++++- docs/skills/DEBT.md | 8 +++++++- 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/docs/retrospectives/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md b/docs/retrospectives/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md index 160c588..6b8bd2d 100644 --- a/docs/retrospectives/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md +++ b/docs/retrospectives/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md @@ -79,6 +79,17 @@ The fix separates the two concerns, which is what §6.5 described all along: eac **Methodology consequence worth carrying:** when a source-verification gate asks "what is the best API for X," the answer is scoped to the question's implicit cardinality. This gate asked about an upsert and got an answer about *an* upsert. The spec said "per side," and nobody re-read that clause against the chosen primitive until the review. +### This PR broke CI, and the honest test for that was to re-run `main` + +CI failed twice with `DockerApiException: "Get https://registry-1.docker.io/v2/: context deadline exceeded"`, thrown from `ResourceReaper.GetAndStartNewAsync` — and it failed in **Telemetry's** pre-existing suites, which this PR does not touch. Every signal said flake. + +It was not flake. Re-running `main`'s own workflow at the same moment passed, which is the experiment that settles it: Docker Hub was healthy, and the added load was this PR's. xUnit runs the two test assemblies in parallel, each standing up its own Testcontainers session, and slice 5 took that from four containers to six — including a **second** ~800 MB Kafka image. The concurrent pulls saturated the runner's registry connection, and whichever assembly lost the race reported it as a broken test. + +Fixed with a serial `docker pull` step ahead of `dotnet test`, by user sign-off, since CI changes are conventionally their own session. Two things worth carrying: + +- **"It failed in code I didn't touch" is evidence about *load*, not innocence.** The blast radius of a new test fixture is the whole CI job, not its own assembly. +- **Pinning an image tag by hand is a place to verify, not guess.** The first version of the pre-pull step named `testcontainers/ryuk:0.11.0`; Testcontainers 4.13.0 actually pins `0.14.0` by digest, read out of the package assembly. A wrong tag there fails silently in the worst way — it pre-pulls an image nothing uses and quietly restores the behaviour it was meant to fix, while looking like a fix. + ### Regression tests must be proven to fail Having written the fix, I wrote two regression tests, and they passed. That is not evidence — a test that passes on both the broken and fixed implementation pins nothing. @@ -133,7 +144,9 @@ This was caught by noticing the magnitude mismatch mid-write, not by a failing t 2. **A `tidy: skills` session.** Four rows registered this session join the standing backlog — and **three older rows are decisions, not cleanups**, and must not be drained by a routine tidy without a call: test-class naming (`Slice{N}Tests` vs. the skill's snake_case mandate), `testing-integration` Gap B (no shipped collection follows the documented Strategy 1, and this session added a **fourth** non-conforming collection, `DispatchKafka`), and the `identity-acl` streaming exception. 3. **CritterWatch.** It renders meaningfully only once real cross-service traffic exists, which is exactly what this PR created. Needs RabbitMQ as a tooling-only broker (ADR-017); trial licence expired 2026-07-10, so re-check before planning. -**Still true and still unaddressed:** CI cannot build `apphost.cs`. It was edited again this session and verified by hand. That row remains open and remains its own session. +**Still true and still unaddressed:** CI cannot build `apphost.cs`. It was edited again this session and verified by hand. That row remains open and remains its own session — and note this session touched the workflow for a *different* reason (the pre-pull step), so a future CI session inherits a file that has already been edited once outside its remit. + +**New CI consideration for every future service:** the pre-pull list is now something a new Testcontainers-backed fixture must extend. It is not enforced — a fixture pinning an unlisted image still passes locally and merely reintroduces the race in CI. Worth folding into `testing-integration` whenever that skill's other rows are settled. --- diff --git a/docs/skills/DEBT.md b/docs/skills/DEBT.md index 1a4ca81..acee29f 100644 --- a/docs/skills/DEBT.md +++ b/docs/skills/DEBT.md @@ -89,6 +89,12 @@ This file is the working ledger between retros that surface gaps and the tidy se - **Why it is worth a line rather than tolerating:** the pattern will recur every time a slice half-lands across a boundary, and without a rule the scoping discipline (mirror the locked view fields; invent no transitions) is re-derived or skipped. - **Retro source:** [`retrospectives/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md`](../retrospectives/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md). +### `testing-integration` — a new Testcontainers image must be added to CI's pre-pull list, and nothing enforces it + +- **Gap:** As of PR D, `.github/workflows/dotnet.yml` pre-pulls every Testcontainers image serially before `dotnet test`, because six containers across two parallel test assemblies saturated the runner's connection to Docker Hub and the reaper's pull timed out. The skill says nothing about this. A new fixture pinning an unlisted image passes locally, passes review, and merely reintroduces the race in CI — where it surfaces as a `DockerApiException` in whichever *unrelated* suite lost the race. +- **Why it is worth a line:** the failure blames the wrong code, and the repo's trajectory is one new fixture per bounded context. Also worth recording alongside: the ryuk tag in that list is pinned by Testcontainers itself (4.13.0 → `0.14.0`, by digest) and must be re-read from the package on every Testcontainers bump. +- **Retro source:** [`retrospectives/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md`](../retrospectives/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md). + ### `csharp-coding-standards` — § Geospatial Values mandates a `GeoLocation` type that does not exist - **Gap:** § Geospatial Values says the agent "should NOT pass raw `(double, double)` tuples around. Use a single `GeoLocation` record everywhere," and shows an implementation. **No `GeoLocation` exists in the codebase.** Dispatch has `Shared/Location.cs` (`record Location(double Lat, double Lon, string? StreetAddress)`), Telemetry has no equivalent, and Telemetry's shipped `H3CellIndexer.TryComputeCell(double, double, int)` takes raw degrees — as does PR D's `H3KRing`, which deliberately mirrored it so the two services compute identical cell ids. The skill therefore describes a convention with zero adherents and one named-but-different type. @@ -171,4 +177,4 @@ Older entries drop off; the retros and commits remain authoritative. - **2026-06-25.** Registered two at-threshold rows surfaced by retro 005 (slice 5.3) and carried in the 2026-06-16 post-slice-5.3 handoff: the marker-interface union return type and the event-triggered automation handler shape, both grouped under `wolverine-handlers` (or a possible new `wolverine-marten-automation` skill). Registering, not fixing — the fix is a future `tidy: skills` session. The **bundling-rule encoding** gap (also flagged past-threshold in retro 005 and the handoff) was deliberately *not* registered: neither source names a target skill, and this file's convention requires a row to name the skill. It stays for a session that can ground the target. - **2026-07-24 (PR C).** Registered six rows from the slice-3 Kafka session: `wolverine-kafka` listener-example refresh (deferred to PR D, which builds the consumer those examples describe), `transport-selection`'s missing built-vs-modeled status axis, the `aspire` skill's non-compiling `AddKafka` port example, and, from the Phase 2 audit and the two-axis code review, `service-bootstrap`'s undocumented optional connection-string guard, `testing-integration`'s two wrong Testcontainers API names plus its unfollowed collection convention, and the CI-cannot-build-`apphost.cs` gap. Two *other* `wolverine-kafka` gaps found the same session were **fixed in-flight** under the session-runner-blocking exception and do not appear here: its topic-naming section proposed a rule contradicting ADR-014/ADR-019, and its serialization section mandated JSON where the session ships protobuf — a session cannot follow a skill that contradicts the ADR it is authoring. - **2026-07-02.** Drained both 2026-06-25 rows via a new `docs/skills/wolverine-marten-automation/SKILL.md` skill (critter-skill-auditor Phase 1 discovery ruled out both `wolverine-handlers` and `marten-wolverine-aggregates` as bolt-on homes). `Open debt` reset to empty. Item 1 of the [post-W006 handoff](../planning/2026-07-02-post-w006-next-steps-handoff.md)'s ordered table. Retro at [`docs/retrospectives/skills-tidy-wolverine-marten-automation.md`](../retrospectives/skills-tidy-wolverine-marten-automation.md). -- **2026-07-24 (PR D).** Drained the `wolverine-kafka` listener row **inside an implementation PR**, which is a first for this file and worth naming as a pattern: the row was registered in PR C with an explicit "fix it from the shipped consumer" condition, and PR D is the session that satisfied that condition. A row whose fix is *blocked on code that does not exist yet* is drained by the session that writes the code, not by a later `tidy: skills` session — deferring it further would only have kept a known-wrong skill in place for no gain. Registered four new rows the same session: `protobuf-contracts`' undrawn line between a published event contract and a borrowed shared type, a second and sharper instance of the missing non-event-sourced-document-in-an-event-sourced-BC guidance, the unnamed "forward-constraint placeholder message" shape, and `csharp-coding-standards`' `GeoLocation` mandate for a type that does not exist. Also folded a companion note into the standing `testing-integration` Gap B row (its per-test cleanup convention has no adherents either) and corrected that row's collection count, which was stale by one. The three decision-class rows (test-class naming, `testing-integration` Gap B, `identity-acl`) were deliberately left alone — each needs a call, not a tidy. +- **2026-07-24 (PR D).** Drained the `wolverine-kafka` listener row **inside an implementation PR**, which is a first for this file and worth naming as a pattern: the row was registered in PR C with an explicit "fix it from the shipped consumer" condition, and PR D is the session that satisfied that condition. A row whose fix is *blocked on code that does not exist yet* is drained by the session that writes the code, not by a later `tidy: skills` session — deferring it further would only have kept a known-wrong skill in place for no gain. Registered five new rows the same session: `protobuf-contracts`' undrawn line between a published event contract and a borrowed shared type, a second and sharper instance of the missing non-event-sourced-document-in-an-event-sourced-BC guidance, the unnamed "forward-constraint placeholder message" shape, `csharp-coding-standards`' `GeoLocation` mandate for a type that does not exist, and the unenforced CI pre-pull list this session had to introduce. Also folded a companion note into the standing `testing-integration` Gap B row (its per-test cleanup convention has no adherents either) and corrected that row's collection count, which was stale by one. The three decision-class rows (test-class naming, `testing-integration` Gap B, `identity-acl`) were deliberately left alone — each needs a call, not a tidy. From 108537f42d19f5fe8fdbda49166dd9fdd95519c8 Mon Sep 17 00:00:00 2001 From: Erik Shafer Date: Sat, 25 Jul 2026 00:57:23 -0500 Subject: [PATCH 7/8] Widen TrackedSession timeouts destabilised by the added CI container load --- .../Slice53CandidatesSelectedTests.cs | 11 ++++++++++- .../FareQuoting/Slice52FareQuotedFailurePathTests.cs | 6 +++++- .../FareQuoting/Slice52FareQuotedHappyPathTests.cs | 7 ++++++- 3 files changed, 21 insertions(+), 3 deletions(-) diff --git a/tests/CritterCab.Dispatch.Tests/CandidateSelection/Slice53CandidatesSelectedTests.cs b/tests/CritterCab.Dispatch.Tests/CandidateSelection/Slice53CandidatesSelectedTests.cs index e600c53..7459024 100644 --- a/tests/CritterCab.Dispatch.Tests/CandidateSelection/Slice53CandidatesSelectedTests.cs +++ b/tests/CritterCab.Dispatch.Tests/CandidateSelection/Slice53CandidatesSelectedTests.cs @@ -158,6 +158,15 @@ private async Task SubmitRideAndAwaitOutcome(VehicleClass v NotesForDriver: null); IScenarioResult httpResult = null!; + + // 30s rather than the 5s default. The cascade under test is RideRequested → FareQuoted → + // CandidatesSelected across three handlers, and the first invocation in the assembly also + // pays for Marten schema creation — which fits comfortably in 5s on a developer machine and + // did until slice 5 added a second Postgres and a Kafka broker to this project's fixtures. + // Both assemblies now start their containers in parallel on a 2-core CI runner, and this + // test began timing out intermittently at 4.6s. The timeout is a safety net against a hung + // cascade, not an assertion about latency, so widening it costs nothing and removes a + // failure that says nothing about the code. await _host.ExecuteAndWaitAsync(async () => { httpResult = await _host.Scenario(s => @@ -165,7 +174,7 @@ await _host.ExecuteAndWaitAsync(async () => s.Post.Json(command).ToUrl("/api/rides/request"); s.StatusCodeShouldBe(HttpStatusCode.Created); }); - }); + }, timeoutInMilliseconds: 30_000); var response = httpResult.ReadAsJson(); response.ShouldNotBeNull(); diff --git a/tests/CritterCab.Dispatch.Tests/FareQuoting/Slice52FareQuotedFailurePathTests.cs b/tests/CritterCab.Dispatch.Tests/FareQuoting/Slice52FareQuotedFailurePathTests.cs index a32d84d..7ef901f 100644 --- a/tests/CritterCab.Dispatch.Tests/FareQuoting/Slice52FareQuotedFailurePathTests.cs +++ b/tests/CritterCab.Dispatch.Tests/FareQuoting/Slice52FareQuotedFailurePathTests.cs @@ -151,6 +151,10 @@ private async Task SubmitRideAndAwaitOutcome() NotesForDriver: null); IScenarioResult httpResult = null!; + + // 30s rather than the 5s default — and this suite needs it most, because the retry loop + // under test spends real time in cooldowns before reaching a terminal outcome. See the + // same widening in Slice53CandidatesSelectedTests for why the default stopped sufficing. await _host.ExecuteAndWaitAsync(async () => { httpResult = await _host.Scenario(s => @@ -158,7 +162,7 @@ await _host.ExecuteAndWaitAsync(async () => s.Post.Json(command).ToUrl("/api/rides/request"); s.StatusCodeShouldBe(HttpStatusCode.Created); }); - }); + }, timeoutInMilliseconds: 30_000); var response = httpResult.ReadAsJson(); response.ShouldNotBeNull(); diff --git a/tests/CritterCab.Dispatch.Tests/FareQuoting/Slice52FareQuotedHappyPathTests.cs b/tests/CritterCab.Dispatch.Tests/FareQuoting/Slice52FareQuotedHappyPathTests.cs index ecfe631..054041a 100644 --- a/tests/CritterCab.Dispatch.Tests/FareQuoting/Slice52FareQuotedHappyPathTests.cs +++ b/tests/CritterCab.Dispatch.Tests/FareQuoting/Slice52FareQuotedHappyPathTests.cs @@ -39,6 +39,11 @@ public async Task fare_quote_automation_records_fare_quoted_on_stream() NotesForDriver: "meet at side entrance"); IScenarioResult httpResult = null!; + + // 30s rather than the 5s default. Same reason as the widening in + // Slice53CandidatesSelectedTests: the timeout guards against a hung cascade, not slowness, + // and 5s became marginal on CI once slice 5 added a second Postgres and a Kafka broker to + // this project's fixtures. var tracked = await _host.ExecuteAndWaitAsync(async () => { httpResult = await _host.Scenario(s => @@ -46,7 +51,7 @@ public async Task fare_quote_automation_records_fare_quoted_on_stream() s.Post.Json(command).ToUrl("/api/rides/request"); s.StatusCodeShouldBe(HttpStatusCode.Created); }); - }); + }, timeoutInMilliseconds: 30_000); var response = httpResult.ReadAsJson(); response.ShouldNotBeNull(); From 6989c010abb8f8d2f7ebe1b989617fe38c3d3c96 Mon Sep 17 00:00:00 2001 From: Erik Shafer Date: Sat, 25 Jul 2026 01:06:46 -0500 Subject: [PATCH 8/8] Record the CI-load timeout finding in retro 009 --- .../009-dispatch-w006-slice-5-nearby-available-drivers.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/retrospectives/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md b/docs/retrospectives/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md index 6b8bd2d..afa058d 100644 --- a/docs/retrospectives/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md +++ b/docs/retrospectives/implementations/009-dispatch-w006-slice-5-nearby-available-drivers.md @@ -90,6 +90,10 @@ Fixed with a serial `docker pull` step ahead of `dotnet test`, by user sign-off, - **"It failed in code I didn't touch" is evidence about *load*, not innocence.** The blast radius of a new test fixture is the whole CI job, not its own assembly. - **Pinning an image tag by hand is a place to verify, not guess.** The first version of the pre-pull step named `testcontainers/ryuk:0.11.0`; Testcontainers 4.13.0 actually pins `0.14.0` by digest, read out of the package assembly. A wrong tag there fails silently in the worst way — it pre-pulls an image nothing uses and quietly restores the behaviour it was meant to fix, while looking like a fix. +**The load pressure then surfaced a second, unrelated symptom.** With the pulls fixed, one *pre-existing* test — `Slice53CandidatesSelectedTests` — timed out at the `TrackedSession` default of 5 s, having reached 4.6 s. Nothing about it changed; the three-handler cascade it exercises simply no longer fits in 5 s on a 2-core runner that is also starting six containers. Widened to 30 s there and in the two `Slice52` helpers carrying the same default, since all three were equally exposed and leaving them would have left landmines for the next contributor to trip. + +That is worth separating from the "no opportunistic edits" rule rather than blurring into it: those are files this session's prompt did not name, and the edits were still right, because **the session destabilised them**. The rule exists to stop unrelated improvements riding along, not to stop a session cleaning up after itself. A useful test for the distinction — *would this file still need touching if my change were reverted?* If no, it is in scope. + ### Regression tests must be proven to fail Having written the fix, I wrote two regression tests, and they passed. That is not evidence — a test that passes on both the broken and fixed implementation pins nothing.