From dd429260f5a253bdc0a2b888124b6029e0570acd Mon Sep 17 00:00:00 2001 From: Erik Shafer Date: Fri, 24 Jul 2026 19:23:36 -0500 Subject: [PATCH 1/5] =?UTF-8?q?docs:=20add=20prompt=20008=20=E2=80=94=20Te?= =?UTF-8?q?lemetry=20slice=203=20(DriverLocationUpdated=20->=20Kafka)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Session prompt for PR C. The IDriverLocationPublisher seam shipped in PR #45 already carries the generated DriverLocationUpdated, so this session swaps only the implementation and must not touch the contract. A jasperfx-source-verifier pass ran during authoring and closed all six verify-before-wiring gates against local wolverine @ V6.21.0-12. It surfaced the session's one load-bearing fork: W006 §6.3 argues publish-first from failure-mode asymmetry, but a Kafka publishing endpoint defaults to BufferedInMemory, where PublishAsync returns before the broker acks — making that ordering nominal. Resolved to SendInline + UseSyncRetryBlock + UseIdempotentProducer. W006 §11 candidate #1 fires here (first Kafka topic lands) and lands as ADR-019 inside this PR, which also supplies the ADR-004 design-return interleave. --- .../008-telemetry-slice-3-kafka-publish.md | 149 ++++++++++++++++++ 1 file changed, 149 insertions(+) create mode 100644 docs/prompts/implementations/008-telemetry-slice-3-kafka-publish.md diff --git a/docs/prompts/implementations/008-telemetry-slice-3-kafka-publish.md b/docs/prompts/implementations/008-telemetry-slice-3-kafka-publish.md new file mode 100644 index 0000000..db27370 --- /dev/null +++ b/docs/prompts/implementations/008-telemetry-slice-3-kafka-publish.md @@ -0,0 +1,149 @@ +# Prompt 008 — Telemetry Slice 3 (`DriverLocationUpdated` → Kafka) + +| Field | Value | +|---|---| +| **Status** | **Ready** — the two durable forks resolved by the user 2026-07-24 (delivery semantics: `SendInline` + `UseSyncRetryBlock`; the W006 §11 Kafka-topic-naming candidate lands as **ADR-019 inside this PR**). Rides in the PR C session's PR alongside the implementation; not committed standalone. | +| **Authored** | 2026-07-24 | +| **Target artifacts** | `Directory.Packages.props` (`WolverineFx.Protobuf` 6.21.0; `Confluent.Kafka` for the test project), `src/CritterCab.Telemetry/CritterCab.Telemetry.csproj` (`WolverineFx.Kafka`, `WolverineFx.Protobuf`), `src/CritterCab.Telemetry/ReportLocations/IDriverLocationPublisher.cs` (**implementation only — the interface does not change**), `src/CritterCab.Telemetry/Program.cs` (Kafka transport + publishing rule + durability), `tests/CritterCab.Telemetry.Tests/` (Kafka Testcontainer + round-trip assertion), `apphost.cs` (Kafka resource + reference), `protos/crittercab/telemetry/v1/driver_location_updated.proto` (topic/partition-key comment now describes shipped code), **`docs/decisions/019-transport-agnostic-topic-naming.md` (new)**, `docs/workshops/006-telemetry-event-model.md` (Document History), `docs/skills/transport-selection/SKILL.md` (Kafka row moves modeled → built), `docs/skills/DEBT.md`, `docs/prompts/README.md` (index entry), this prompt's retro. | +| **Source-of-truth dependencies** | [W006 §6.3 (the slice), §6.2 (the trigger that fires it), §3.2 (published-not-stored), §11 #1 (the ADR candidate)](../../workshops/006-telemetry-event-model.md); [ADR-005](../../decisions/005-transport-selection-by-flow-type.md) (Kafka for high-volume telemetry); [ADR-009](../../decisions/009-protobuf-contracts-as-first-class-artifacts.md) (the proto is the contract); [ADR-014](../../decisions/014-asb-topic-naming-convention.md) (the ASB-scoped convention ADR-019 generalizes); [ADR-018](../../decisions/018-candidate-projection-ownership-and-telemetry-geospatial-supply.md) (supplier half). Skills: `transport-selection`, `protobuf-contracts`, `vertical-slice-organization`, `csharp-coding-standards`, `testing-integration`, `testing-fundamentals`. | +| **Workflow position** | Third implementation session of the W006 Telemetry chain (**PR C**). Realizes **CritterCab's first Kafka topic** and its **second live transport**. Follows PR [#45](https://github.com/erikshafer/CritterCab/pull/45) (slices 4 + 2). One slice, one PR — the coupled-slices reading that justified PR B does not apply here. Also the **ADR-004 design-return interleave** for this run of Telemetry implementation PRs: ADR-019 is the design artifact riding alongside the third consecutive implementation PR. | + +--- + +## Framing — why this session exists + +PR B made gRPC real. This session makes Kafka real, and it is deliberately the smallest possible session that can do so: **the seam already carries the real contract.** + +`IDriverLocationPublisher.PublishAsync(DriverLocationUpdated, CancellationToken)` was introduced in PR B holding the *generated* `DriverLocationUpdated` — the proto was put through codegen (`GrpcServices="None"`) in PR B precisely so that PR C would have nothing to design. The publish trigger, the payload construction, the publish-first ordering, and the `LastKnownPosition` upsert all shipped and are under test. **What is missing is one implementation class and the host wiring behind it.** This prompt's single most important instruction is therefore a negative one: *do not redesign the contract.* The interface, its signature, its payload type, and the handler that calls it are all settled. If the session finds itself editing `ReportLocationsHandler.cs`, something has gone wrong. + +**The one place the session must think hard is delivery semantics**, and the source-verification pass found that W006 §6.3 and Wolverine's defaults disagree. §6.3 argues publish-first from failure-mode asymmetry — a duplicate is cheaper than a miss — but that argument only has force if the publish's outcome is *known* before the upsert runs. A Kafka publishing endpoint defaults to `BufferedInMemory`, where `await PublishAsync(...)` returns once the record is enqueued in-process, before the broker has seen it. Under that default, publish-first would be ordering in name only and every broker failure would land in the branch §6.3 called the worse one. Fork 1 resolves this (see § Decisions resolved). + +**No narrative anchors this session.** PR #40 decided the narrative layer does not apply to Telemetry (machine-to-machine; no protagonist-perceivable moment). W006 is the direct spec anchor. + +--- + +## Goal + +Replace `LoggingDriverLocationPublisher` with a real WolverineFx.Kafka producer publishing the generated `DriverLocationUpdated` to topic `telemetry.driver-location-updated`, partitioned by `driverId`, protobuf-serialized, sent inline so a broker failure is observable — wire the broker into `apphost.cs` and a Kafka Testcontainer into the test suite, prove the round trip with a broker-backed integration test, and author **ADR-019** generalizing ADR-014's topic-naming convention across transports. + +--- + +## Spec delta + +- **W006 §6.3 moves designed → realized in code.** The topic, the partition key, the dedup/LWW key, and the publish-first/no-outbox coupling become concrete. **First Kafka topic in CritterCab and the second live transport** — after this session, two of ADR-005's three transports are running, and only Azure Service Bus remains modeled-but-unbuilt. +- **W006 §11 candidate #1 fires as an ADR.** *Kafka topic-naming convention* — its trigger is literally "first Kafka topic lands," which happens in this PR. Lands as **ADR-019**, generalizing ADR-014 from ASB-scoped to transport-agnostic, per §11's explicit "own ADR, not folded" instruction. §11 candidate #2 (stream-processing as a fourth modeling shape) remains later-arc; #3 already discharged as a skill in PR B. +- **W006 §6.3's publish-first argument gains an implementation-time qualifier.** The workshop reasoned about publish-first without reference to any transport's durability model. This session records in `## Document History` that the ordering is only real under inline sending, and which mode CritterCab therefore chose. **A spec amendment, not a spec correction** — §6.3's reasoning stands; it simply did not name the precondition its conclusion depends on. + +--- + +## Orientation files (read in order) + +1. **[W006 §6.3](../../workshops/006-telemetry-event-model.md)** — the slice spec. The event-shape table, the Kafka specifics (topic / partition key / dedup key), the publish-first consistency note, and the two GWT sketches. The **Publish** GWT is this session's test contract; the **Dedup** GWT is a *consumer-side* assertion and belongs to PR D, not here. +2. **The shipped seam** — [`src/CritterCab.Telemetry/ReportLocations/IDriverLocationPublisher.cs`](../../../src/CritterCab.Telemetry/ReportLocations/IDriverLocationPublisher.cs). Read the header comment before anything else: it states the contract is fixed and only the implementation moves. Then [`ReportLocationsHandler.cs`](../../../src/CritterCab.Telemetry/ReportLocations/ReportLocationsHandler.cs) `BuildUpdate` — the payload is already fully populated, including the optional `speed`/`heading` presence handling. +3. **The shipped proto** — [`protos/crittercab/telemetry/v1/driver_location_updated.proto`](../../../protos/crittercab/telemetry/v1/driver_location_updated.proto). Already in codegen via `GrpcServices="None"` (see the `.csproj` comment). Its header comment already names the topic and both keys; this session's edit is only to stop describing them as future. +4. **[ADR-014](../../decisions/014-asb-topic-naming-convention.md)** — read in full before drafting ADR-019. ADR-019 generalizes its Option C and must be explicit about which of its *two travelling operational decisions* (session keying, outbox coordination) generalize and which are ASB-specific. Then skim [ADR-005](../../decisions/005-transport-selection-by-flow-type.md) for the per-flow-type transport rule this slice is the first Kafka instance of. +5. **Reference implementations** — [`src/CritterCab.Telemetry/Program.cs`](../../../src/CritterCab.Telemetry/Program.cs) for the connection-string-guarded registration idiom (Marten is registered only when its connection string is present; Kafka follows the same shape, see deliverable 4). [`tests/CritterCab.Telemetry.Tests/TelemetryTestFixture.cs`](../../../tests/CritterCab.Telemetry.Tests/TelemetryTestFixture.cs) for the Testcontainers + Alba + seam-swap fixture this session extends. +6. **6.21.0 Kafka API surface** — the § Verify-before-wiring gates below record exact source citations from local `C:\Code\JasperFx\wolverine` @ `V6.21.0-12-ge08abdeb3`. Re-confirm shape, not existence. + +--- + +## Working pattern + +- Interactive, sign-off per logical chunk, in dependency order: (a) **packages + `apphost.cs` Kafka resource** (broker reachable, host still boots) → (b) **`KafkaDriverLocationPublisher` + `Program.cs` wiring** → (c) **Kafka Testcontainer + round-trip test** → (d) **ADR-019 + docs**. Chunk (d) is deliberately last: the ADR should describe a convention the code already demonstrates. +- **The seam is frozen.** `IDriverLocationPublisher`'s signature, `DriverLocationUpdated` as its payload, and `ReportLocationsHandler`'s call site do not change. `ReportLocationsHandler.cs` should not appear in this PR's diff at all. If a Wolverine API makes that impossible, **stop and surface it** rather than editing the handler — that is a fork, not an implementation detail. +- Docker was re-probed green at session start (`docker run --rm hello-world` succeeds), so Testcontainers runs locally; CI is a backstop, not the only gate. +- Positional `sealed record`s for anything hand-authored — but note almost nothing here is: `DriverLocationUpdated` is generated, and the publisher is a class with one method. +- Branch `telemetry/slice-3-kafka-publish` (already created); never commit to `main`. Retro ships in this PR. Run `critter-skill-auditor` Phase 1 before cutting code, Phase 2 after. + +--- + +## Verify before wiring (jasperfx-source-verifier — local `C:\Code\JasperFx\wolverine` @ `V6.21.0-12-ge08abdeb3`) + +A full pass ran during this prompt's authoring (2026-07-24). **Gates 1–6 are closed**; re-confirm shape when wiring, and treat the two traps as load-bearing. + +1. **Broker connection + topic routing (VERIFIED).** `WolverineOptions.UseKafka(string bootstrapServers)` → `KafkaTransportExpression` (`KafkaTransportExtensions.cs:69`); `IPublishToExpression.ToKafkaTopic(string topicName)` → `KafkaSubscriberConfiguration` (`:178`); entry point `WolverineOptions.PublishMessage()` (`WolverineOptions.Endpoints.cs:102`). Namespace `Wolverine.Kafka`. Topic auto-creation is opt-in via `KafkaTransportExpression.AutoProvision(...)` (`KafkaTransportExpression.cs:183`) — **needed for both Aspire local-dev and the Testcontainer**, since neither pre-creates the topic. + - **⚠ Side effect:** `UseKafka` sets `options.EnableAutomaticFailureAcks = false` globally (`:71-72`) — a documented Kafka-serialization concession, not a bug, but it mutates a non-Kafka option. Note it in the wiring comment. + - **⚠ Do NOT call `ConfigureKafka(this WolverineOptions, string bootstrapServers)` (`:106`)** — it accepts `bootstrapServers` and never uses it. Dead parameter; the name invites the mistake. + - **Aspire-friendly alternative:** `UseKafkaUsingNamedConnection(string connectionStringName, ...)` (`:25`). Evaluate against the Marten-guard idiom in deliverable 4 — CritterCab reads `GetConnectionString(...)` explicitly elsewhere, and consistency with the existing `Program.cs` shape likely wins over the newer overload. +2. **Per-message partition key (VERIFIED — this was the load-bearing gate).** The Kafka key comes from `Envelope.PartitionKey`, falling back to the envelope id: `Key = !string.IsNullOrEmpty(envelope.PartitionKey) ? envelope.PartitionKey : envelope.Id.ToString()` (`KafkaTransportExtensions.cs:266-289`). Set per message via `DeliveryOptions.PartitionKey` (`DeliveryOptions.cs:199-202`, applied to the envelope at `:177-180`). The default `KafkaEnvelopeMapper` never touches `Key`. Round-trip behavior confirmed by Wolverine's own `when_publishing_and_receiving_by_partition_key.cs:62-91`. + - **⚠ TRAP — `PublishRawJson()` silently destroys the key.** `JsonOnlyMapper.MapEnvelopeToOutgoing` assigns `outgoing.Key = envelope.GroupId!` (`IKafkaEnvelopeMapper.cs`), running *after* `CreateMessage` set it from `PartitionKey`. **Never combine `PublishRawJson()` with `DeliveryOptions.PartitionKey`.** Moot here (this session publishes protobuf, not raw JSON) but exactly the kind of thing a later "let's simplify the wire format" change would trip over — record it in the retro. + - `opts.Policies.PropagateGroupIdToPartitionKey()` (`WolverineOptions.Policies.cs:289`) exists but is for *cascaded* messages inheriting an upstream key. Not applicable — the publisher seam is called directly. +3. **Protobuf serialization (VERIFIED; separate package, registry-confirmed).** `WolverineFx.Protobuf` — `ProtobufMessageSerializer : IMessageSerializer`, `ContentType = "binary/protobuf"`, writes via `protobufMessage.WriteTo(stream)` (`Wolverine.Protobuf/Internal/ProtobufMessageSerializer.cs:8,12,62`). Two extension shapes (`WolverineProtobufSerializationExtensions.cs`): a **global** `UseProtobufSerialization(this WolverineOptions, ...)` returning `void` (`:13`, sets `DefaultSerializer` app-wide) and a **per-endpoint** `UseProtobufSerialization(this T endpoint, ...) where T : IEndpointConfiguration` (`:33`). `KafkaSubscriberConfiguration` satisfies the constraint through `InteroperableSubscriberConfiguration` → `SubscriberConfiguration` → `ISubscriberConfiguration` → `IEndpointConfiguration`. Applied on send at `MessageRoute.cs:81`. **Use the per-endpoint overload** — Telemetry's HTTP surface must stay JSON. Wire payload is `Message` (`KafkaTransportExtensions.cs:279`), so binary protobuf is natively expressible. + - **Registry facts closed 2026-07-24 against nuget.org:** `WolverineFx.Protobuf` **6.21.0 is published** (the 6.x line runs through 6.22.0), so it does not lag the main train. Wolverine pins `Google.Protobuf` 3.31.1 (`Directory.Packages.props:29`) — **the same version CritterCab already pins** for Grpc.Tools codegen, so there is no version reconciliation to do. + - **Consumer-side note (PR D, not this session):** `ProtobufMessageSerializer.ReadFromData(byte[])` throws `NotSupportedException` (`:38-41`); only the `(Type, Envelope)` overload works, so PR D's listener must carry the serializer on the endpoint for type info to be available. +4. **Delivery semantics (VERIFIED — and it corrects an unstated W006 assumption).** Modes are `Durable` / `BufferedInMemory` / `Inline` (`Endpoint.cs:58-75`); the default for a Kafka publishing endpoint is **`BufferedInMemory`** (`Endpoint.cs:183`, not overridden by `KafkaTopic`). Config: `UseDurableOutbox()` / `BufferedInMemory()` / `SendInline()` (`SubscriberConfiguration.cs:93,99,105`). **There is no `.NotDurable()`** — that method does not exist. + - **No outbox by default, confirmed.** A DI-injected `IMessageBus` outside a Wolverine transactional chain has `Transaction == null` and therefore takes the `StoreAndForwardAsync` branch, never the outbox branch (`MessageBus.cs:437,478-481`; `Transaction` is set only by `EnlistInOutbox*` at `:370-386`). W006 §6.3's no-outbox mandate needs no opt-out. + - **But `BufferedInMemory` breaks publish-first.** It routes to `BatchedSender` (`KafkaTopic.cs:334-337`), whose `storeAndForwardAsync` only posts to an in-memory queue (`BufferedSendingAgent.cs:65-76`) — `await PublishAsync` returns before the broker acks. Wolverine's own test concedes this with a 5-second sleep (`send_kafka_tombstone.cs:41-42`). + - **And `SendInline()` alone is not enough.** It awaits `_producer.ProduceAsync` to broker ack (`InlineKafkaSender.cs:48-58`), but `InlineSendingAgent` picks its retry block on `DurabilitySettings.UseSyncRetryBlock`, which **has no initializer and defaults to `false`** (`InlineSendingAgent.cs:35-42`; `DurabilitySettings.cs:359`). The async `RetryBlock.PostAsync` **swallows** the exception and re-posts to a background block, so the caller sees success on a rejected record (`JasperFx/Blocks/RetryBlock.cs:63-76`). `RetryBlockSync.PostAsync` retries across four pauses and then **rethrows** (`RetryBlockSync.cs:17-53`). + - **Therefore the chosen shape is `SendInline()` + `opts.Durability.UseSyncRetryBlock = true`** (fork 1). `UseSyncRetryBlock` is **process-global**, not per-endpoint — acceptable because Telemetry publishes to exactly one transport, but it must be commented as such so a future second publisher does not inherit it unnoticed. Pair with `UseIdempotentProducer()` (`KafkaSubscriberConfiguration.cs:102` — `enable.idempotence=true`, `acks=all`). + - **VERIFY at wiring time:** that the rethrow actually propagates out of `IMessageBus.PublishAsync` and up through `ReportLocationsHandler`'s `await publisher.PublishAsync(...)`, skipping the upsert. This is the behavior the whole fork was chosen for, and it is the one claim above that is an inference across two components rather than a single cited line. **Cover it with a test** (deliverable 8) — do not take it on trust. +5. **Test-side consumption (VERIFIED).** Wolverine's own Kafka suite does **not** use Testcontainers for its primary broker despite the misleading class name: `KafkaContainerFixture.ConnectionString` is the hardcoded string `"localhost:9092"` pointing at docker-compose (`confluentinc/confluent-local:8.0.6`, `docker-compose.yml:99-107`). **CritterCab should not copy that** — `Testcontainers.Kafka 4.13.0` is already pinned in `Directory.Packages.props` and the Postgres fixture already establishes the Testcontainers idiom. **Copy the assertion shape, not the host shape:** `send_kafka_tombstone.cs:44-60` builds a raw `ConsumerBuilder` with a unique group id and `AutoOffsetReset.Earliest`, consumes with a timeout, and asserts on `result.Message.Key`. For a protobuf value, assert `DriverLocationUpdated.Parser.ParseFrom(result.Message.Value)`. Use a unique topic name per test if isolation demands it — though here the topic is the *convention under test*, so prefer a shared real topic name and unique consumer groups. + - `Confluent.Kafka` **2.14.0** reaches Wolverine transitively (via `Confluent.SchemaRegistry.Serdes.*`, `Directory.Packages.props:18-19`). The test project needs `ConsumerBuilder` directly, so **add an explicit `Confluent.Kafka` PackageVersion + PackageReference** rather than relying on transitive flow. +6. **Topic listening (VERIFIED — context only, NOT built this session).** `ListenToKafkaTopic(string)` (`KafkaTransportExtensions.cs:143`); consumer group set via `KafkaListenerConfiguration.ConfigureConsumer(Action)` (`:396`) — there is no `.GroupId(string)` method. PR D's territory; recorded here only so PR D need not re-verify. + +--- + +## Decisions resolved (user sign-off 2026-07-24) + +1. **Delivery semantics — `SendInline()` + `UseSyncRetryBlock = true` + `UseIdempotentProducer()`.** Makes W006 §6.3's publish-first ordering literally true rather than nominal: the broker ack is awaited, and a failure throws, so the `LastKnownPosition` upsert is skipped, the baseline stays stale, and the driver's next ping re-evaluates and republishes. Self-healing by construction. Accepted costs, both to be named in the retro: `UseSyncRetryBlock` is process-global, and a broker outage stalls a publishing ping through four retry pauses before throwing. The volume objection is weaker than it first appears — publishes are throttled to cell-change-or-heartbeat, never per raw ping. +2. **W006 §11 candidate #1 lands as ADR-019, inside this PR.** The trigger ("first Kafka topic lands") fires here, so the convention ships with the first code that obeys it instead of trailing it. Also supplies the ADR-004 design-return interleave for the third consecutive Telemetry implementation PR. Its own ADR, per §11's "one-decision-per-ADR, not folded into ADR-018." +3. **Protobuf serialization, endpoint-scoped (session-runner call, not a user fork).** `WolverineFx.Protobuf`'s per-endpoint `UseProtobufSerialization()` on the Kafka publishing rule only. The global overload would drag Telemetry's HTTP surface off JSON. The alternative — System.Text.Json over protoc-generated classes — produces bloated non-canonical JSON on a telemetry hot path and throws away the fact that `protos/` is already the contract of record (ADR-009). +4. **Test strategy: one broker-backed round-trip test; existing slice-2 tests stay on the recording seam (session-runner call).** The recording publisher tests the *trigger* and should keep doing so — they are fast and they are not about transport. Exactly one new test proves the *transport*: topic, partition key, protobuf payload. This mirrors what PR B did for gRPC, where the point of the PR was that the transport was real. + +--- + +## Deliverable plan + +**A. Packages + broker resource (chunk a)** +1. `Directory.Packages.props` — add `WolverineFx.Protobuf` at **6.21.0** (lockstep with the other eleven `WolverineFx*` entries) and `Confluent.Kafka` at **2.14.0** (matching what Wolverine resolves transitively; needed directly by the test project for `ConsumerBuilder`). `WolverineFx.Kafka`, `Aspire.Hosting.Kafka`, and `Testcontainers.Kafka` are **already pinned** — no version work needed for those. +2. `src/CritterCab.Telemetry/CritterCab.Telemetry.csproj` — `PackageReference` for `WolverineFx.Kafka` and `WolverineFx.Protobuf`. +3. `apphost.cs` — add the Kafka resource (`builder.AddKafka("kafka")`, persistent lifetime to match the Postgres container's), `.WithReference(kafka)` + `.WaitFor(kafka)` on the Telemetry project, and a port in the `53xx` band per the `aspire` skill's allocation convention. **Replace the "deliberately NOT wired" comment block** (lines ~48-50) — it describes exactly what this deliverable undoes. Note the file-based apphost has no `.csproj` and **CI cannot build it** (a known gap, its own future session): verify locally with `dotnet run apphost.cs`. + +**B. The publisher (chunk b; `ReportLocations/` feature folder)** +4. `Program.cs` — register the Kafka transport inside `builder.Host.UseWolverine(opts => ...)`: + - `opts.UseKafka(bootstrapServers).AutoProvision()` — `AutoProvision` because neither Aspire nor the Testcontainer pre-creates the topic (gate 1). + - `opts.PublishMessage().ToKafkaTopic("telemetry.driver-location-updated").SendInline().UseIdempotentProducer().UseProtobufSerialization()`. + - `opts.Durability.UseSyncRetryBlock = true` — with a comment stating it is **process-global** and why Telemetry can afford it (gate 4). + - **Guard on the connection string**, mirroring the existing Marten guard: when no Kafka connection string is configured, register `LoggingDriverLocationPublisher` and skip the transport entirely, so a broker-less run (and any test that does not want a broker) still boots. The existing `AddSingleton()` becomes the else-branch, not a deletion. +5. `KafkaDriverLocationPublisher : IDriverLocationPublisher` — the whole class is roughly one method: `bus.PublishAsync(update, new DeliveryOptions { PartitionKey = update.DriverId })`. Two mechanical notes the source pass surfaced: `IMessageBus.PublishAsync` returns `ValueTask` (so `.AsTask()`, since the seam returns `Task`) and **takes no `CancellationToken`** (so the seam's `ct` has nowhere to go on this path — document that in a comment rather than silently dropping it; do **not** change the seam's signature to remove it, PR D or a real cancellation path may want it back). +6. `IDriverLocationPublisher.cs` — update only the comments that describe `LoggingDriverLocationPublisher` as *the* implementation; it survives as the no-broker fallback (deliverable 4), so it is not deleted. **The interface declaration itself does not change.** +7. `protos/crittercab/telemetry/v1/driver_location_updated.proto` — the header comment's topic and key lines currently read as design intent; they now describe shipped code. In-bounds: this session wires the transport this proto's comment specifies. + +**C. Round-trip test (chunk c)** +8. Kafka Testcontainer in the test fixture alongside the existing Postgres one, and **one** integration test proving the transport: drive the gRPC ingest so the publish trigger fires, then consume from `telemetry.driver-location-updated` with a raw `ConsumerBuilder` (unique group id, `AutoOffsetReset.Earliest`) and assert (i) `result.Message.Key == driverId`, (ii) `DriverLocationUpdated.Parser.ParseFrom(result.Message.Value)` round-trips to the expected `h3Cell` / `serverReceivedAt` / `throttlePolicyVersion`. **Plus the gate-4 behavior test:** with the broker unreachable (or the publisher forced to throw), assert `LastKnownPosition` was **not** upserted — this is the claim fork 1 was chosen for and gate 4 flagged as inferred rather than directly cited. + - Keep the existing slice-2 tests on `RecordingDriverLocationPublisher`. Do not convert them. + - Watch fixture cost: the Kafka container should not be paid for by the slice-1 / slice-4 suites. Prefer a separate fixture/collection over widening `TelemetryTestFixture` if the shared-container cost is material. + +**D. ADR + docs (chunk d)** +9. **`docs/decisions/019-transport-agnostic-topic-naming.md`** — generalize ADR-014's Option C (`.`) from ASB to all CritterCab transports, with `telemetry.driver-location-updated` as the first Kafka instance. The load-bearing content is **the divergence**, not the sameness: ADR-014's two travelling operational decisions do **not** generalize — ASB uses `SessionId = ` (ADR-013) plus outbox coordination; Kafka uses `PartitionKey = driverId` plus **no** outbox with publish-first inline sending. State explicitly that ADR-019 extends rather than supersedes ADR-014 (ADR-014 stays Accepted; its ASB operational clauses remain authoritative for ASB), and update ADR-014's status line to cross-reference. Follow the format of the existing ADRs in `docs/decisions/` — Context / Options Considered / Decision / Consequences. +10. `docs/workshops/006-telemetry-event-model.md` `## Document History` — slice 3 realized; the §11 #1 candidate discharged as ADR-019; **and the publish-first qualifier** (§6.3's ordering argument holds only under inline sending; CritterCab chose `SendInline` + `UseSyncRetryBlock` to make it real). +11. `docs/skills/transport-selection/SKILL.md` — Kafka moves from modeled to built, with the topic/partition-key/durability shape this session established. **Check the file first**: if it carries no built-vs-modeled status axis, register a DEBT row instead of inventing a structure mid-session. +12. `docs/skills/DEBT.md` — register at minimum: (a) the `PublishRawJson()` × `PartitionKey` trap (gate 2) as a note for whoever touches Kafka wire format next; (b) a **Kafka publishing skill** if the wiring shape proves to have enough CritterCab-specific content to warrant one (PR B's precedent: the client-streaming pattern landed in `wolverine-grpc-handlers`, not an ADR). `docs/prompts/README.md` — Implementations index entry. +13. This prompt's retro at `docs/retrospectives/implementations/008-telemetry-slice-3-kafka-publish.md`. + +--- + +## Out of scope + +- **Slice 5 / the Dispatch consumer (`NearbyAvailableDriversStub` replacement)** — PR D. This session publishes; nothing consumes. Do not write a listener, and do not "prove the loop" by adding a consumer to Telemetry. +- **The ASB / Driver-Profile-availability half of ADR-018's join** — a forward-constraint to an un-workshopped BC. Do not model or build it. +- **Redesigning `IDriverLocationPublisher` or `ReportLocationsHandler`** — the contract is frozen (see § Framing). `ReportLocationsHandler.cs` should not be in this PR's diff. +- **W006 §6.3's Dedup GWT** — it asserts *consumer* behavior (`the Dispatch consumer dedups on (D, T)`). PR D's test, not this one. +- **Azure Service Bus** — still modeled-but-unbuilt after this PR, and deliberately so. ADR-019 names the ASB convention but builds nothing. +- **CritterWatch** — requires RabbitMQ as a tooling-only broker (ADR-017) and its trial licence expired 2026-07-10. Adding a broker to `apphost.cs` this session is **not** an invitation to add that one. +- **The `wolverine-grpc-bidirectional-handlers` structural rewrite** — an open `tidy: skills` DEBT row from PR B, untouched here. +- **CLAUDE.md status-line refresh** — PR [#43](https://github.com/erikshafer/CritterCab/pull/43) is open against it and is itself stale (written before Telemetry had transport). Not this session's file; flag in the retro if PR C changes what an accurate status line would say. +- **The CI "Verify solution completeness" gap** — CI cannot see `apphost.cs`, which this session edits. Extending that step is its own session (CI changes carry their own blast radius). Register/refresh it as a DEBT row; do not fix it here. + +--- + +## Follow-on PR sequence (arc context; not this session) + +- **PR D** — Slice 5 (Dispatch consumes `telemetry.driver-location-updated` into the `AvailableDriver` view, replacing `NearbyAvailableDriversStub`; the Kafka half of ADR-018's join only). Closes the W001 §5.3 amendment. Gate 6 above records the listener API so PR D need not re-verify it; gate 3's `ReadFromData` caveat is a PR D constraint. +- **Beyond** — ADR-005's third transport (Azure Service Bus) remains unbuilt; the Driver Profile workshop is the prerequisite for the ASB half of the join. + +--- + +## Document history + +- **2026-07-24.** Authored on a clean `main` @ `144899d` immediately after PR #45 merged. Full `jasperfx-source-verifier` pass run during authoring against local `C:\Code\JasperFx\wolverine` @ `V6.21.0-12-ge08abdeb3` — **gates 1–6 all closed at authoring time**, which is a departure from prompt 007 (where several gates stayed open into the session). Two traps recorded (`ConfigureKafka`'s dead parameter; `PublishRawJson()` clobbering `PartitionKey`). The pass also **surfaced a gap between W006 §6.3 and Wolverine's defaults** — publish-first is nominal under the default `BufferedInMemory` mode — which became fork 1 and is the session's one genuinely load-bearing decision. `WolverineFx.Protobuf 6.21.0` confirmed published against nuget.org during authoring, closing the verifier's one flagged registry unknown. Both durable forks resolved by user sign-off the same day. From e52ff89dbb986af7ebc79f91325c4fd6fe4d7fec Mon Sep 17 00:00:00 2001 From: Erik Shafer Date: Fri, 24 Jul 2026 20:18:31 -0500 Subject: [PATCH 2/5] Telemetry slice 3: publish DriverLocationUpdated to Kafka MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Swaps LoggingDriverLocationPublisher for a real WolverineFx.Kafka producer behind the unchanged IDriverLocationPublisher seam (W006 §6.3). CritterCab's first Kafka topic and second live transport; ReportLocationsHandler is untouched, which is what the seam existed for. Wiring: telemetry.driver-location-updated, partition key driverId, endpoint-scoped binary protobuf, AutoProvision, and the broker read by name so Aspire, the Testcontainer, and Event Hubs all work without branching. SendInline + UseSyncRetryBlock rather than the BufferedInMemory default. Buffered returns before the broker acks and Wolverine's default async retry block swallows the failure, which would have made §6.3's publish-before-store ordering true only in statement order — the upsert would land on a lost publish, the branch §6.3 argued against. Inline plus the sync block makes a rejection throw, so the baseline stays stale and the next ping republishes. Tests: a broker-backed round trip asserting the partition key and that the value is binary protobuf the generated parser accepts, on its own fixture so the other suites do not wait on Kafka; plus an ordering test pinning that a failed publish leaves no baseline. 33/33 green. --- Directory.Packages.props | 11 ++ apphost.cs | 29 ++++- .../008-telemetry-slice-3-kafka-publish.md | 23 +++- .../CritterCab.Telemetry.csproj | 2 + src/CritterCab.Telemetry/Program.cs | 82 +++++++++++- .../IDriverLocationPublisher.cs | 18 +-- .../KafkaDriverLocationPublisher.cs | 40 ++++++ .../CritterCab.Telemetry.Tests.csproj | 5 + .../Slice3KafkaPublishTests.cs | 119 ++++++++++++++++++ .../Slice3PublishOrderingTests.cs | 109 ++++++++++++++++ .../TelemetryKafkaTestFixture.cs | 93 ++++++++++++++ .../TelemetryTestFixture.cs | 18 ++- 12 files changed, 527 insertions(+), 22 deletions(-) create mode 100644 src/CritterCab.Telemetry/ReportLocations/KafkaDriverLocationPublisher.cs create mode 100644 tests/CritterCab.Telemetry.Tests/ReportLocations/Slice3KafkaPublishTests.cs create mode 100644 tests/CritterCab.Telemetry.Tests/ReportLocations/Slice3PublishOrderingTests.cs create mode 100644 tests/CritterCab.Telemetry.Tests/TelemetryKafkaTestFixture.cs diff --git a/Directory.Packages.props b/Directory.Packages.props index 4473af1..65fe22d 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -30,6 +30,11 @@ + + + diff --git a/apphost.cs b/apphost.cs index 4d59f51..5165a0b 100644 --- a/apphost.cs +++ b/apphost.cs @@ -8,6 +8,7 @@ #:property ManagePackageVersionsCentrally=false #:package Aspire.Hosting.PostgreSQL@13.4.6 +#:package Aspire.Hosting.Kafka@13.4.6 #:project ./src/CritterCab.Dispatch/CritterCab.Dispatch.csproj #:project ./src/CritterCab.Telemetry/CritterCab.Telemetry.csproj @@ -24,6 +25,22 @@ var dispatchDb = postgres.AddDatabase("crittercab_dispatch"); var telemetryDb = postgres.AddDatabase("crittercab_telemetry"); +// CritterCab's first Kafka broker (W006 §6.3). Host port 5392 is the slot the +// aspire skill's port-allocation table reserved for Kafka before any broker existed +// (5390 Postgres / 5391 SqlServer / 5392 Kafka / 5393 ASB emulator) — this claims a +// reservation rather than making a new allocation. Persistent lifetime matches the +// Postgres container so a restart of the AppHost does not discard the topic. +// +// A real Kafka container, not the Event Hubs Emulator: the emulator serves only the +// producer and consumer APIs, and Telemetry calls AutoProvision() to create the topic +// at startup, which needs the Kafka admin API. Production runs Kafka protocol against +// Azure Event Hubs with pre-provisioned topics; the service code is identical either +// way because it reads the connection string by name. +// Note the port is a constructor argument here, not a .WithHostPort(...) call — on +// Aspire 13.4.6 WithHostPort belongs to the Kafka UI container resource, not the broker. +var kafka = builder.AddKafka("kafka", port: 5392) + .WithLifetime(ContainerLifetime.Persistent); + // === Services === // Pinned to CritterCab's 5300-5307 dashboard band's service slot for Dispatch @@ -45,13 +62,17 @@ // the same arrangement the Dispatch block describes above, so gRPC needs no endpoint of its // own. This is CritterCab's first gRPC surface that actually serves traffic. // -// The Kafka resource that slice 3 needs is still deliberately NOT wired: slice 2's publish -// goes to the IDriverLocationPublisher seam, and PR C swaps that for the real producer and -// adds the broker here. +// Telemetry publishes DriverLocationUpdated to telemetry.driver-location-updated (W006 §6.3), +// so it is the first service to reference the broker. The reference injects the connection +// string under the name "kafka", which is exactly the key Program.cs reads via +// UseKafkaUsingNamedConnection — no address is hard-coded on either side. WaitFor matters here +// beyond ordering hygiene: startup calls AutoProvision() to create the topic. builder.AddProject("telemetry", launchProfileName: null) .WithHttpsEndpoint(port: 5315, name: "https") .WithHttpEndpoint(port: 5316, name: "http") .WithReference(telemetryDb) - .WaitFor(telemetryDb); + .WaitFor(telemetryDb) + .WithReference(kafka) + .WaitFor(kafka); builder.Build().Run(); diff --git a/docs/prompts/implementations/008-telemetry-slice-3-kafka-publish.md b/docs/prompts/implementations/008-telemetry-slice-3-kafka-publish.md index db27370..d20b150 100644 --- a/docs/prompts/implementations/008-telemetry-slice-3-kafka-publish.md +++ b/docs/prompts/implementations/008-telemetry-slice-3-kafka-publish.md @@ -5,7 +5,7 @@ | **Status** | **Ready** — the two durable forks resolved by the user 2026-07-24 (delivery semantics: `SendInline` + `UseSyncRetryBlock`; the W006 §11 Kafka-topic-naming candidate lands as **ADR-019 inside this PR**). Rides in the PR C session's PR alongside the implementation; not committed standalone. | | **Authored** | 2026-07-24 | | **Target artifacts** | `Directory.Packages.props` (`WolverineFx.Protobuf` 6.21.0; `Confluent.Kafka` for the test project), `src/CritterCab.Telemetry/CritterCab.Telemetry.csproj` (`WolverineFx.Kafka`, `WolverineFx.Protobuf`), `src/CritterCab.Telemetry/ReportLocations/IDriverLocationPublisher.cs` (**implementation only — the interface does not change**), `src/CritterCab.Telemetry/Program.cs` (Kafka transport + publishing rule + durability), `tests/CritterCab.Telemetry.Tests/` (Kafka Testcontainer + round-trip assertion), `apphost.cs` (Kafka resource + reference), `protos/crittercab/telemetry/v1/driver_location_updated.proto` (topic/partition-key comment now describes shipped code), **`docs/decisions/019-transport-agnostic-topic-naming.md` (new)**, `docs/workshops/006-telemetry-event-model.md` (Document History), `docs/skills/transport-selection/SKILL.md` (Kafka row moves modeled → built), `docs/skills/DEBT.md`, `docs/prompts/README.md` (index entry), this prompt's retro. | -| **Source-of-truth dependencies** | [W006 §6.3 (the slice), §6.2 (the trigger that fires it), §3.2 (published-not-stored), §11 #1 (the ADR candidate)](../../workshops/006-telemetry-event-model.md); [ADR-005](../../decisions/005-transport-selection-by-flow-type.md) (Kafka for high-volume telemetry); [ADR-009](../../decisions/009-protobuf-contracts-as-first-class-artifacts.md) (the proto is the contract); [ADR-014](../../decisions/014-asb-topic-naming-convention.md) (the ASB-scoped convention ADR-019 generalizes); [ADR-018](../../decisions/018-candidate-projection-ownership-and-telemetry-geospatial-supply.md) (supplier half). Skills: `transport-selection`, `protobuf-contracts`, `vertical-slice-organization`, `csharp-coding-standards`, `testing-integration`, `testing-fundamentals`. | +| **Source-of-truth dependencies** | [W006 §6.3 (the slice), §6.2 (the trigger that fires it), §3.2 (published-not-stored), §11 #1 (the ADR candidate)](../../workshops/006-telemetry-event-model.md); [ADR-005](../../decisions/005-transport-selection-by-flow-type.md) (Kafka for high-volume telemetry); [ADR-009](../../decisions/009-protobuf-contracts-as-first-class-artifacts.md) (the proto is the contract); [ADR-014](../../decisions/014-asb-topic-naming-convention.md) (the ASB-scoped convention ADR-019 generalizes); [ADR-018](../../decisions/018-candidate-projection-ownership-and-telemetry-geospatial-supply.md) (supplier half). Skills: **`wolverine-kafka`** (the load-bearing one — added by the Phase 1 auditor pass, not in this prompt's first draft), `service-bootstrap`, `transport-selection`, `aspire`, `protobuf-contracts`, `vertical-slice-organization`, `csharp-coding-standards`, `testing-integration`, `testing-fundamentals`. | | **Workflow position** | Third implementation session of the W006 Telemetry chain (**PR C**). Realizes **CritterCab's first Kafka topic** and its **second live transport**. Follows PR [#45](https://github.com/erikshafer/CritterCab/pull/45) (slices 4 + 2). One slice, one PR — the coupled-slices reading that justified PR B does not apply here. Also the **ADR-004 design-return interleave** for this run of Telemetry implementation PRs: ADR-019 is the design artifact riding alongside the third consecutive implementation PR. | --- @@ -88,6 +88,7 @@ A full pass ran during this prompt's authoring (2026-07-24). **Gates 1–6 are c 1. **Delivery semantics — `SendInline()` + `UseSyncRetryBlock = true` + `UseIdempotentProducer()`.** Makes W006 §6.3's publish-first ordering literally true rather than nominal: the broker ack is awaited, and a failure throws, so the `LastKnownPosition` upsert is skipped, the baseline stays stale, and the driver's next ping re-evaluates and republishes. Self-healing by construction. Accepted costs, both to be named in the retro: `UseSyncRetryBlock` is process-global, and a broker outage stalls a publishing ping through four retry pauses before throwing. The volume objection is weaker than it first appears — publishes are throttled to cell-change-or-heartbeat, never per raw ping. 2. **W006 §11 candidate #1 lands as ADR-019, inside this PR.** The trigger ("first Kafka topic lands") fires here, so the convention ships with the first code that obeys it instead of trailing it. Also supplies the ADR-004 design-return interleave for the third consecutive Telemetry implementation PR. Its own ADR, per §11's "one-decision-per-ADR, not folded into ADR-018." 3. **Protobuf serialization, endpoint-scoped (session-runner call, not a user fork).** `WolverineFx.Protobuf`'s per-endpoint `UseProtobufSerialization()` on the Kafka publishing rule only. The global overload would drag Telemetry's HTTP surface off JSON. The alternative — System.Text.Json over protoc-generated classes — produces bloated non-canonical JSON on a telemetry hot path and throws away the fact that `protos/` is already the contract of record (ADR-009). +3b. **Protobuf overrides the skill's stated JSON default (user sign-off, escalated from a session-runner call).** `wolverine-kafka/SKILL.md` § Serialization explicitly says "Cab uses Wolverine's default JSON serialization" and defers protobuf to "a future phase." Phase 1 surfaced that contradiction, and it was put to the user rather than absorbed silently: **this is that phase.** The payload is already a `Google.Protobuf.IMessage` generated from the contract of record, so JSON-over-protoc-generated-classes would leave ADR-009's proto governing the type but not the wire. The skill is corrected in-PR (deliverable 11), not left contradicting the code. 4. **Test strategy: one broker-backed round-trip test; existing slice-2 tests stay on the recording seam (session-runner call).** The recording publisher tests the *trigger* and should keep doing so — they are fast and they are not about transport. Exactly one new test proves the *transport*: topic, partition key, protobuf payload. This mirrors what PR B did for gRPC, where the point of the PR was that the transport was real. --- @@ -97,11 +98,11 @@ A full pass ran during this prompt's authoring (2026-07-24). **Gates 1–6 are c **A. Packages + broker resource (chunk a)** 1. `Directory.Packages.props` — add `WolverineFx.Protobuf` at **6.21.0** (lockstep with the other eleven `WolverineFx*` entries) and `Confluent.Kafka` at **2.14.0** (matching what Wolverine resolves transitively; needed directly by the test project for `ConsumerBuilder`). `WolverineFx.Kafka`, `Aspire.Hosting.Kafka`, and `Testcontainers.Kafka` are **already pinned** — no version work needed for those. 2. `src/CritterCab.Telemetry/CritterCab.Telemetry.csproj` — `PackageReference` for `WolverineFx.Kafka` and `WolverineFx.Protobuf`. -3. `apphost.cs` — add the Kafka resource (`builder.AddKafka("kafka")`, persistent lifetime to match the Postgres container's), `.WithReference(kafka)` + `.WaitFor(kafka)` on the Telemetry project, and a port in the `53xx` band per the `aspire` skill's allocation convention. **Replace the "deliberately NOT wired" comment block** (lines ~48-50) — it describes exactly what this deliverable undoes. Note the file-based apphost has no `.csproj` and **CI cannot build it** (a known gap, its own future session): verify locally with `dotnet run apphost.cs`. +3. `apphost.cs` — add the Kafka resource (`builder.AddKafka("kafka")`, persistent lifetime to match the Postgres container's), `.WithReference(kafka)` + `.WaitFor(kafka)` on the Telemetry project, on host port **5392** — Phase 1 found the `aspire` skill's § Port allocation table has *already* assigned `5392` to Kafka (alongside `5390` Postgres / `5391` SqlServer / `5393` ASB emulator), so this is claiming a reserved slot, not making a new allocation decision. **Replace the "deliberately NOT wired" comment block** (lines ~48-50) — it describes exactly what this deliverable undoes. Note the file-based apphost has no `.csproj` and **CI cannot build it** (a known gap, its own future session): verify locally with `dotnet run apphost.cs`. **B. The publisher (chunk b; `ReportLocations/` feature folder)** 4. `Program.cs` — register the Kafka transport inside `builder.Host.UseWolverine(opts => ...)`: - - `opts.UseKafka(bootstrapServers).AutoProvision()` — `AutoProvision` because neither Aspire nor the Testcontainer pre-creates the topic (gate 1). + - `opts.UseKafkaUsingNamedConnection("kafka").AutoProvision()` — **not** `UseKafka(bootstrapServers)`. Gate 1 left this open; Phase 1 closed it: `wolverine-kafka/SKILL.md` § Bootstrap names the named-connection overload "the Cab default" for any Aspire-orchestrated service and lists "hard-coding bootstrap servers in `Program.cs`" as a pitfall. `AutoProvision` because neither Aspire nor the Testcontainer pre-creates the topic. - `opts.PublishMessage().ToKafkaTopic("telemetry.driver-location-updated").SendInline().UseIdempotentProducer().UseProtobufSerialization()`. - `opts.Durability.UseSyncRetryBlock = true` — with a comment stating it is **process-global** and why Telemetry can afford it (gate 4). - **Guard on the connection string**, mirroring the existing Marten guard: when no Kafka connection string is configured, register `LoggingDriverLocationPublisher` and skip the transport entirely, so a broker-less run (and any test that does not want a broker) still boots. The existing `AddSingleton()` becomes the else-branch, not a deletion. @@ -117,9 +118,13 @@ A full pass ran during this prompt's authoring (2026-07-24). **Gates 1–6 are c **D. ADR + docs (chunk d)** 9. **`docs/decisions/019-transport-agnostic-topic-naming.md`** — generalize ADR-014's Option C (`.`) from ASB to all CritterCab transports, with `telemetry.driver-location-updated` as the first Kafka instance. The load-bearing content is **the divergence**, not the sameness: ADR-014's two travelling operational decisions do **not** generalize — ASB uses `SessionId = ` (ADR-013) plus outbox coordination; Kafka uses `PartitionKey = driverId` plus **no** outbox with publish-first inline sending. State explicitly that ADR-019 extends rather than supersedes ADR-014 (ADR-014 stays Accepted; its ASB operational clauses remain authoritative for ASB), and update ADR-014's status line to cross-reference. Follow the format of the existing ADRs in `docs/decisions/` — Context / Options Considered / Decision / Consequences. 10. `docs/workshops/006-telemetry-event-model.md` `## Document History` — slice 3 realized; the §11 #1 candidate discharged as ADR-019; **and the publish-first qualifier** (§6.3's ordering argument holds only under inline sending; CritterCab chose `SendInline` + `UseSyncRetryBlock` to make it real). -11. `docs/skills/transport-selection/SKILL.md` — Kafka moves from modeled to built, with the topic/partition-key/durability shape this session established. **Check the file first**: if it carries no built-vs-modeled status axis, register a DEBT row instead of inventing a structure mid-session. -12. `docs/skills/DEBT.md` — register at minimum: (a) the `PublishRawJson()` × `PartitionKey` trap (gate 2) as a note for whoever touches Kafka wire format next; (b) a **Kafka publishing skill** if the wiring shape proves to have enough CritterCab-specific content to warrant one (PR B's precedent: the client-streaming pattern landed in `wolverine-grpc-handlers`, not an ADR). `docs/prompts/README.md` — Implementations index entry. -13. This prompt's retro at `docs/retrospectives/implementations/008-telemetry-slice-3-kafka-publish.md`. +11. **`docs/skills/wolverine-kafka/SKILL.md` — two session-runner-blocking corrections** (Phase 1 finding; the session cannot follow a skill that contradicts the code and ADR it is shipping — same exception PR B used for the gRPC skills): + - **§ Topic naming convention.** The skill says Cab topics "carry a descriptive name rather than a message-type name" and illustrates with `telemetry.location-pings`. ADR-014 (Accepted) and W006 §6.3's locked-decisions table both mandate `.` — which *is* a message-type name — and the shipped topic is `telemetry.driver-location-updated`. **Spec beats skill.** Correct the section to the ADR-019 convention and replace the illustrative rows with the real topic. Note the skill's example topics (`telemetry.location-pings`, `telemetry.demand-signals`, `pricing.surge-updates`) are speculative, not shipped. + - **§ Serialization and interop.** The skill says "Cab uses Wolverine's default JSON serialization" and defers protobuf to "a future phase." **This is that phase** (fork 3). Correct it to name endpoint-scoped `UseProtobufSerialization()` for topics whose payload is a generated protobuf type, keeping default JSON for everything else, and cross-reference ADR-009. + - Scope discipline: correct these two sections and the pitfalls that restate them. Do **not** rewrite the listener/consumer-group/DLQ sections — those are PR D's territory and untouched by this session. +12. `docs/skills/transport-selection/SKILL.md` — **do not edit.** Phase 1 confirmed the file has no built-vs-modeled status axis to flip (its § Phasing table has a "Lands when" column, not a per-row status flag). Register a DEBT row instead of inventing a structure mid-session, exactly as this deliverable's original contingency directed. +13. `docs/skills/DEBT.md` — register at minimum: (a) the `PublishRawJson()` × `PartitionKey` trap (gate 2) as a note for whoever touches Kafka wire format next; (b) the missing built-vs-modeled status axis in `transport-selection` (deliverable 12); (c) a **Kafka publishing skill** only if the wiring shape proves to have CritterCab-specific content that does not fit `wolverine-kafka` — likely NOT, since that skill already exists and is being corrected here. `docs/prompts/README.md` — Implementations index entry. +14. This prompt's retro at `docs/retrospectives/implementations/008-telemetry-slice-3-kafka-publish.md`. --- @@ -146,4 +151,10 @@ A full pass ran during this prompt's authoring (2026-07-24). **Gates 1–6 are c ## Document history +- **2026-07-24 (session start, Phase 1 amendments).** The `critter-skill-auditor` Phase 1 pass ran against the drafted prompt and **corrected it in four places before any code was cut** — worth recording, because a prompt this heavily source-verified still had discovery gaps: + - **A governing skill was missing entirely.** [`docs/skills/wolverine-kafka/SKILL.md`](../../skills/wolverine-kafka/SKILL.md) exists and is the direct mechanics skill for this session; the first draft's skill list omitted it, naming only `transport-selection` (which decides *whether* Kafka, not *how*). Added, along with `service-bootstrap` and `aspire`. + - **Gate 1's bootstrap lean was wrong.** The draft leaned toward `UseKafka(bootstrapServers)` for consistency with the existing `Program.cs` shape; the skill names `UseKafkaUsingNamedConnection("kafka")` as the Cab default for Aspire-orchestrated services and lists hard-coded bootstrap servers as a pitfall. Deliverable 4 corrected. + - **The Kafka port was already allocated.** `aspire/SKILL.md` § Port allocation had reserved **5392** for Kafka since before any broker existed; the draft treated it as a new decision. Deliverable 3 corrected. + - **Two skill-vs-spec contradictions surfaced**, both now session-runner-blocking corrections in deliverable 11. (i) *Topic naming* — the skill says topics carry a descriptive name "rather than a message-type name," but ADR-014 and W006 §6.3 mandate `.`. Spec wins; this became the substantive question ADR-019 has to answer rather than rubber-stamp. (ii) *Serialization* — the skill says default JSON and defers protobuf to "a future phase"; escalated to the user rather than absorbed, and resolved as fork 3b. + Phase 1 also **confirmed** three of the draft's contingencies: `transport-selection` has no built-vs-modeled status axis (so deliverable 12 becomes a DEBT row, not an edit), `KafkaDriverLocationPublisher` belongs in the `ReportLocations/` feature folder (a `Transports/` folder would be the technical-role anti-pattern), and the new test class follows the shipped `Slice{N}Tests` PascalCase pattern rather than `testing-fundamentals`' snake_case mandate — that mismatch is an existing DEBT row flagged "do not fix slice-locally." - **2026-07-24.** Authored on a clean `main` @ `144899d` immediately after PR #45 merged. Full `jasperfx-source-verifier` pass run during authoring against local `C:\Code\JasperFx\wolverine` @ `V6.21.0-12-ge08abdeb3` — **gates 1–6 all closed at authoring time**, which is a departure from prompt 007 (where several gates stayed open into the session). Two traps recorded (`ConfigureKafka`'s dead parameter; `PublishRawJson()` clobbering `PartitionKey`). The pass also **surfaced a gap between W006 §6.3 and Wolverine's defaults** — publish-first is nominal under the default `BufferedInMemory` mode — which became fork 1 and is the session's one genuinely load-bearing decision. `WolverineFx.Protobuf 6.21.0` confirmed published against nuget.org during authoring, closing the verifier's one flagged registry unknown. Both durable forks resolved by user sign-off the same day. diff --git a/src/CritterCab.Telemetry/CritterCab.Telemetry.csproj b/src/CritterCab.Telemetry/CritterCab.Telemetry.csproj index 2316cda..d642c0f 100644 --- a/src/CritterCab.Telemetry/CritterCab.Telemetry.csproj +++ b/src/CritterCab.Telemetry/CritterCab.Telemetry.csproj @@ -8,6 +8,8 @@ + + diff --git a/src/CritterCab.Telemetry/Program.cs b/src/CritterCab.Telemetry/Program.cs index 617d60c..8fae09c 100644 --- a/src/CritterCab.Telemetry/Program.cs +++ b/src/CritterCab.Telemetry/Program.cs @@ -1,6 +1,7 @@ using CritterCab.Telemetry.LastKnownPosition; using CritterCab.Telemetry.ReportLocations; using CritterCab.Telemetry.TelemetryPolicy; +using CritterCab.Telemetry.V1; using JasperFx; using Wolverine.Grpc; using Marten; @@ -8,7 +9,9 @@ using Wolverine.FluentValidation; using Wolverine.Http; using Wolverine.Http.FluentValidation; +using Wolverine.Kafka; using Wolverine.Marten; +using Wolverine.Protobuf; var builder = WebApplication.CreateBuilder(args); @@ -62,12 +65,29 @@ builder.Services.AddWolverineGrpc(); // The ingest resolves driverId from the ambient request rather than the payload (R5), so it needs -// the accessor. Both registrations below are ready-to-swap seams, not final implementations: -// HeaderDriverPrincipalAccessor gives way to a real Entra claim once Identity exists, and -// LoggingDriverLocationPublisher to the WolverineFx.Kafka producer in PR C. +// the accessor. HeaderDriverPrincipalAccessor is still a ready-to-swap seam — it gives way to a +// real Entra claim once Identity exists. builder.Services.AddHttpContextAccessor(); builder.Services.AddSingleton(); -builder.Services.AddSingleton(); + +// The publish seam (W006 §6.3). Guarded on the connection string the same way Marten is above: +// with a broker configured the real producer is used, and without one the service still boots +// and logs what it would have published. That keeps a broker-less `dotnet run` useful and keeps +// the slice-1/2/4 test suites from having to stand up Kafka to exercise the ingest. +// +// The lifetimes differ on purpose. KafkaDriverLocationPublisher is scoped because it depends on +// IMessageBus, which Wolverine registers scoped; the logging fallback holds only an ILogger and +// stays a singleton. +var kafkaConnectionString = builder.Configuration.GetConnectionString("kafka"); + +if (!string.IsNullOrEmpty(kafkaConnectionString)) +{ + builder.Services.AddScoped(); +} +else +{ + builder.Services.AddSingleton(); +} // Enum names on the wire; Wolverine HTTP shares the Minimal-API JsonOptions this configures. builder.Services.ConfigureSystemTextJsonForWolverineOrMinimalApi(options => @@ -83,6 +103,60 @@ // from DI — without this discovery step it finds no validator and the invalid command // passes through as 200 instead of a 400 ProblemDetails. opts.UseFluentValidation(); + + if (string.IsNullOrEmpty(kafkaConnectionString)) + return; + + // === Kafka: the slice-3 publish (W006 §6.3) === + // + // Read the broker address by NAME rather than by value: 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. + // + // Note UseKafkaUsingNamedConnection has a side effect beyond Kafka: it sets + // EnableAutomaticFailureAcks = false globally, because automatic acks do not interact + // correctly with Kafka serialization failures. That is deliberate upstream behavior — do not + // re-enable the flag. + // + // AutoProvision creates the topic at startup through the Kafka admin API. It works against a + // real broker (what Aspire and Testcontainers both start) but NOT against the Event Hubs + // Emulator, which serves only producer and consumer APIs. An EH-Emulator environment must + // pre-provision the topic and drop this call. + opts.UseKafkaUsingNamedConnection("kafka") + .AutoProvision(); + + // Makes a broker rejection observable to the caller. Without this, an inline send's failure + // is swallowed by Wolverine's default async retry block, which logs, re-posts to a background + // block, and returns success — so ReportLocationsHandler would proceed to upsert + // LastKnownPosition believing a publish happened. The sync block instead retries across a few + // pauses and then rethrows, which is what makes §6.3's publish-first ordering real rather + // than nominal. + // + // PROCESS-GLOBAL, not per-endpoint. Affordable today because Telemetry publishes to exactly + // one transport; a second publisher added here inherits this and should re-weigh it. + opts.Durability.UseSyncRetryBlock = true; + + // The topic name is spelled out rather than shared as a constant with the tests, so the + // round-trip test asserts against the literal ADR-019 convention and would catch a rename + // here instead of silently following it. + opts.PublishMessage() + .ToKafkaTopic("telemetry.driver-location-updated") + // SendInline, not the BufferedInMemory default. Buffered batches into an in-process queue + // and returns before the broker has seen the record, which would make "publish first, + // then upsert" true only in statement order. Inline awaits the broker ack, so a failed + // publish throws, the upsert is skipped, the driver's baseline stays stale, and their + // next ping republishes — self-healing, and the branch §6.3 argued for. The cost is a + // broker round trip per publish, which is affordable because publishes are throttled to + // cell-change-or-heartbeat, never per raw ping. + .SendInline() + // enable.idempotence=true + acks=all: the broker de-duplicates producer retries, so the + // retry block above cannot turn one position into several records. + .UseIdempotentProducer() + // Endpoint-scoped, so only this topic goes binary. DriverLocationUpdated is generated + // from the .proto that IS the contract (ADR-009); serializing it as JSON would leave the + // contract governing the type but not the wire. Telemetry's HTTP surface stays JSON — + // the global UseProtobufSerialization overload would have taken that with it. + .UseProtobufSerialization(); }); var app = builder.Build(); diff --git a/src/CritterCab.Telemetry/ReportLocations/IDriverLocationPublisher.cs b/src/CritterCab.Telemetry/ReportLocations/IDriverLocationPublisher.cs index 12d3be8..4e34172 100644 --- a/src/CritterCab.Telemetry/ReportLocations/IDriverLocationPublisher.cs +++ b/src/CritterCab.Telemetry/ReportLocations/IDriverLocationPublisher.cs @@ -2,19 +2,23 @@ namespace CritterCab.Telemetry.ReportLocations; -// The slice-3 Kafka publish, held behind a seam so slice 2 can be built and tested without any +// The slice-3 Kafka publish, held behind a seam so the ingest can be built and tested without any // broker (W006 §6.3). The payload is the generated DriverLocationUpdated — the real published -// language shared with Dispatch (ADR-018) — so PR C swaps only the IMPLEMENTATION below for a -// WolverineFx.Kafka producer, never this contract. +// language shared with Dispatch (ADR-018) — which is what let PR C swap in the real producer +// (KafkaDriverLocationPublisher) without touching this contract or its caller. +// +// The seam earned its keep twice over: the ingest's own tests still run against a recording +// implementation with no broker in sight, and LoggingDriverLocationPublisher below remains the +// no-broker fallback. public interface IDriverLocationPublisher { Task PublishAsync(DriverLocationUpdated update, CancellationToken ct); } -// PR B stand-in. Deliberately not a no-op: logging makes the publish observable end-to-end while -// the transport is absent, which is how a manual run of the ingest can be seen working before -// Kafka exists. Replaced in PR C by the real producer publishing to -// telemetry.driver-location-updated, partitioned by driverId. +// The no-broker fallback, registered when no Kafka connection string is configured. Originally +// PR B's stand-in for the absent transport; it survives PR C because a broker-less `dotnet run` +// is still worth having, and logging keeps the publish decision observable when there is nothing +// to publish to. Deliberately not a no-op, for that reason. public sealed class LoggingDriverLocationPublisher(ILogger logger) : IDriverLocationPublisher { diff --git a/src/CritterCab.Telemetry/ReportLocations/KafkaDriverLocationPublisher.cs b/src/CritterCab.Telemetry/ReportLocations/KafkaDriverLocationPublisher.cs new file mode 100644 index 0000000..aa8f123 --- /dev/null +++ b/src/CritterCab.Telemetry/ReportLocations/KafkaDriverLocationPublisher.cs @@ -0,0 +1,40 @@ +using CritterCab.Telemetry.V1; +using Wolverine; + +namespace CritterCab.Telemetry.ReportLocations; + +// W006 §6.3 — the real slice-3 publish. CritterCab's first Kafka producer. +// +// Almost nothing happens here, and that is the point: the topic, serializer, partition-key +// source, and durability mode are all declared once as routing configuration in Program.cs. +// Kafka is a transport wire, not a handler shape — so this class holds no topic name, no +// offsets, and no broker types, and ReportLocationsHandler (which calls it) holds even less. +// Swapping LoggingDriverLocationPublisher for this one is the entire behavioral change of PR C. +// +// Registered SCOPED, unlike the singleton logging stub it replaces: IMessageBus is itself +// registered scoped by Wolverine (HostBuilderExtensions.cs:232), so a singleton holding one +// would be a captive dependency. The ingest handler is resolved per window, so a scoped +// publisher lives exactly as long as the window that uses it. +public sealed class KafkaDriverLocationPublisher(IMessageBus bus) : IDriverLocationPublisher +{ + public Task PublishAsync(DriverLocationUpdated update, CancellationToken ct) + { + // PartitionKey is what makes per-driver ordering a property of the transport rather + // than something Dispatch has to reconstruct (R7): every record for a driver lands on + // one partition and is consumed in order. Wolverine falls back to the envelope's GUID + // when this is unset, which would scatter a driver's positions across partitions and + // silently destroy the ordering guarantee — the failure is invisible until a consumer + // computes heading or speed from out-of-order fixes. + // + // The key is the envelope's PartitionKey, NOT the envelope's GroupId. Those two swap + // roles under PublishRawJson(), whose mapper assigns Key = GroupId after the transport + // has already set it from PartitionKey. This topic publishes binary protobuf, so that + // path is not in play — but never combine the two. + // + // ct has nowhere to go: IMessageBus.PublishAsync takes no CancellationToken. Kept on + // the seam anyway, because the seam is the contract and a future implementation (or a + // consumer-side counterpart) may have somewhere to put it. + return bus.PublishAsync(update, new DeliveryOptions { PartitionKey = update.DriverId }) + .AsTask(); + } +} diff --git a/tests/CritterCab.Telemetry.Tests/CritterCab.Telemetry.Tests.csproj b/tests/CritterCab.Telemetry.Tests/CritterCab.Telemetry.Tests.csproj index 84d5ea2..c0ec660 100644 --- a/tests/CritterCab.Telemetry.Tests/CritterCab.Telemetry.Tests.csproj +++ b/tests/CritterCab.Telemetry.Tests/CritterCab.Telemetry.Tests.csproj @@ -16,6 +16,11 @@ + + + diff --git a/tests/CritterCab.Telemetry.Tests/ReportLocations/Slice3KafkaPublishTests.cs b/tests/CritterCab.Telemetry.Tests/ReportLocations/Slice3KafkaPublishTests.cs new file mode 100644 index 0000000..d64a705 --- /dev/null +++ b/tests/CritterCab.Telemetry.Tests/ReportLocations/Slice3KafkaPublishTests.cs @@ -0,0 +1,119 @@ +using Confluent.Kafka; +using CritterCab.Telemetry.ReportLocations; +using CritterCab.Telemetry.V1; +using Shouldly; +using Xunit; +// Confluent.Kafka has its own Metadata and Timestamp types that collide with the gRPC and +// protobuf ones this test also needs. Aliased rather than fully qualified so the test body still +// reads like the slice-2 tests it mirrors. +using GrpcMetadata = Grpc.Core.Metadata; +using ProtoTimestamp = Google.Protobuf.WellKnownTypes.Timestamp; + +namespace CritterCab.Telemetry.Tests.ReportLocations; + +// W006 §6.3's Publish GWT, asserted against a real broker. +// +// This is the one test in the suite that reads the Kafka record directly instead of trusting a +// seam, and that is the whole point of it: the slice-2 tests prove the publish DECISION, this one +// proves the publish actually reaches a topic, under the key and in the format Dispatch will +// depend on. Everything it asserts is a contract with another bounded context, so it is written +// against the literal topic name and the raw bytes rather than against any constant the producer +// could rename in lockstep with it. +// +// §6.3's second GWT (Dedup) is deliberately absent — it asserts CONSUMER behavior against +// at-least-once redelivery, and there is no consumer until PR D. +[Collection("TelemetryKafka")] +public class Slice3KafkaPublishTests +{ + private const string Topic = "telemetry.driver-location-updated"; + private const double LoopLat = 41.8827d, LoopLon = -87.6233d; + + private readonly TelemetryKafkaTestFixture _fixture; + + public Slice3KafkaPublishTests(TelemetryKafkaTestFixture fixture) => _fixture = fixture; + + [Fact] + public async Task a_published_position_lands_on_the_topic_keyed_by_driver_id() + { + await _fixture.ResetPositionsAsync(); + + var driverId = Guid.CreateVersion7(); + + // Subscribe BEFORE publishing. The consumer group is unique per run and starts at the + // earliest offset, so the record is readable either way — but subscribing first keeps the + // test from depending on retention if it is ever run against a shared broker. + using var consumer = CreateConsumer(); + consumer.Subscribe(Topic); + + // Given a driver with no baseline, when a single ping arrives + // Then §6.4's "Return" rule makes it publish immediately — no throttling to wait out. + var ack = await StreamAsync(driverId, PingAt(LoopLat, LoopLon)); + ack.AcceptedCount.ShouldBe(1); + + // AutoProvision creates the topic at host start, and the first consume after a fresh + // subscribe pays for a group join, so allow generous headroom. The send itself is inline + // and already broker-acked by the time the ack above returned. + var result = consumer.Consume(TimeSpan.FromSeconds(30)); + consumer.Close(); + + result.ShouldNotBeNull("No record arrived on " + Topic + " within the timeout."); + + // The partition key is what makes per-driver ordering a transport property (R7). Asserting + // it here is not incidental: without a key Wolverine falls back to the envelope GUID, the + // records still arrive, and only the ORDERING silently breaks — so this assertion is the + // only thing standing between a working system and a subtly wrong one. + result.Message.Key.ShouldBe(driverId.ToString()); + + // And the value is binary protobuf that the generated parser round-trips — the proof that + // the .proto is governing the wire and not just the C# type (ADR-009). A JSON-serialized + // payload would fail to parse here. + var published = DriverLocationUpdated.Parser.ParseFrom(result.Message.Value); + + published.DriverId.ShouldBe(driverId.ToString()); + published.H3Cell.ShouldBe(H3CellIndexer.TryComputeCell(LoopLat, LoopLon, 9)); + published.H3Resolution.ShouldBe(9); + published.ThrottlePolicyVersion.ShouldBe(1L); + published.ServerReceivedAt.ToDateTimeOffset() + .ShouldBeGreaterThan(DateTimeOffset.UtcNow.AddMinutes(-1)); + + // Optional fields the ping did not carry stay absent rather than defaulting to zero — + // proto3 explicit presence, preserved across the wire. + published.HasSpeed.ShouldBeFalse(); + published.HasHeading.ShouldBeFalse(); + } + + private IConsumer CreateConsumer() => + new ConsumerBuilder(new ConsumerConfig + { + BootstrapServers = _fixture.BootstrapServers, + // Unique per run so a re-run never resumes a committed offset and finds nothing. + GroupId = "slice3-verifier-" + Guid.NewGuid().ToString("N"), + AutoOffsetReset = AutoOffsetReset.Earliest + }).Build(); + + private async Task StreamAsync(Guid driverId, params LocationPing[] pings) + { + using var channel = _fixture.CreateGrpcChannel(); + var client = new TelemetryService.TelemetryServiceClient(channel); + + using var call = client.ReportLocations(new GrpcMetadata + { + { HeaderDriverPrincipalAccessor.DriverIdHeader, driverId.ToString() } + }); + + foreach (var ping in pings) + await call.RequestStream.WriteAsync(ping); + + await call.RequestStream.CompleteAsync(); + + return await call.ResponseAsync; + } + + private static LocationPing PingAt(double lat, double lon) => new() + { + Lat = lat, + Lon = lon, + AccuracyMeters = 8d, + DeviceTimestamp = ProtoTimestamp.FromDateTimeOffset(DateTimeOffset.UtcNow) + }; +} diff --git a/tests/CritterCab.Telemetry.Tests/ReportLocations/Slice3PublishOrderingTests.cs b/tests/CritterCab.Telemetry.Tests/ReportLocations/Slice3PublishOrderingTests.cs new file mode 100644 index 0000000..e11502f --- /dev/null +++ b/tests/CritterCab.Telemetry.Tests/ReportLocations/Slice3PublishOrderingTests.cs @@ -0,0 +1,109 @@ +using CritterCab.Telemetry.ReportLocations; +using CritterCab.Telemetry.V1; +using Google.Protobuf.WellKnownTypes; +using Grpc.Core; +using Marten; +using Microsoft.Extensions.DependencyInjection; +using Shouldly; +using Xunit; +using LastKnownPositionDocument = global::CritterCab.Telemetry.LastKnownPosition.LastKnownPosition; + +namespace CritterCab.Telemetry.Tests.ReportLocations; + +// W006 §6.3's publish-first/no-outbox consistency note, pinned as behavior. +// +// §6.3 chose publish-before-store by comparing two failure modes: a failed store after a good +// publish costs a duplicate (absorbed by the consumer's dedup), while a failed publish after a +// good store costs a MISS that Dispatch cannot detect. That argument only holds if a failed +// publish actually prevents the store — which is precisely why slice 3 configures the Kafka +// endpoint SendInline with a synchronous retry block. Under Wolverine's defaults the publish is +// buffered and its failure swallowed, the store would land anyway, and the system would sit in +// the branch §6.3 rejected while looking correct. +// +// No broker here on purpose. What is under test is the HANDLER's ordering contract, so a +// publisher that throws is a truer and faster instrument than a broker that has to be broken. +// The other half of the claim — that Wolverine's inline sender surfaces a broker rejection as an +// exception at all — rests on source verification and the endpoint configuration, not on this +// test; see the retrospective. +[Collection("Telemetry")] +public class Slice3PublishOrderingTests +{ + private const double LoopLat = 41.8827d, LoopLon = -87.6233d; + + private readonly TelemetryTestFixture _fixture; + + public Slice3PublishOrderingTests(TelemetryTestFixture fixture) => _fixture = fixture; + + [Fact] + public async Task a_failed_publish_leaves_the_baseline_unwritten_so_the_next_ping_republishes() + { + await _fixture.ResetToSeedAsync(); + await _fixture.ResetPositionsAsync(); + _fixture.Publisher.Clear(); + + var driverId = Guid.CreateVersion7(); + + // Given a driver with no baseline, whose next publish will be rejected by the broker + _fixture.Publisher.FailNextPublish = true; + + // When they report a position + var call = () => StreamAsync(driverId, PingAt(LoopLat, LoopLon)); + + // Then the window fails rather than reporting a success it cannot vouch for. A silent + // recovery here would be worse than the error: the driver's client would believe its + // position was delivered. + await Should.ThrowAsync(call); + + // And — the load-bearing assertion — no baseline was written. Publish-first means the + // store is downstream of a publish that never happened. + var stored = await LoadAsync(driverId); + stored.ShouldBeNull(); + + // So the driver is still, correctly, unknown to the system: their next ping finds no + // baseline and publishes immediately (§6.4 "Return"), which is how a dropped publish + // self-heals without any retry state being kept anywhere. + var ack = await StreamAsync(driverId, PingAt(LoopLat, LoopLon)); + + ack.AcceptedCount.ShouldBe(1); + _fixture.Publisher.Published.ShouldHaveSingleItem() + .DriverId.ShouldBe(driverId.ToString()); + + // And only now does the baseline appear. + var recovered = await LoadAsync(driverId); + recovered.ShouldNotBeNull(); + recovered.H3Cell.ShouldBe(H3CellIndexer.TryComputeCell(LoopLat, LoopLon, 9)); + } + + private async Task StreamAsync(Guid driverId, params LocationPing[] pings) + { + using var channel = _fixture.CreateGrpcChannel(); + var client = new TelemetryService.TelemetryServiceClient(channel); + + using var call = client.ReportLocations(new Metadata + { + { HeaderDriverPrincipalAccessor.DriverIdHeader, driverId.ToString() } + }); + + foreach (var ping in pings) + await call.RequestStream.WriteAsync(ping); + + await call.RequestStream.CompleteAsync(); + + return await call.ResponseAsync; + } + + private async Task LoadAsync(Guid driverId) + { + var store = _fixture.Host.Services.GetRequiredService(); + await using var session = store.QuerySession(); + return await session.LoadAsync(driverId); + } + + private static LocationPing PingAt(double lat, double lon) => new() + { + Lat = lat, + Lon = lon, + AccuracyMeters = 8d, + DeviceTimestamp = Timestamp.FromDateTimeOffset(DateTimeOffset.UtcNow) + }; +} diff --git a/tests/CritterCab.Telemetry.Tests/TelemetryKafkaTestFixture.cs b/tests/CritterCab.Telemetry.Tests/TelemetryKafkaTestFixture.cs new file mode 100644 index 0000000..cf2e7a0 --- /dev/null +++ b/tests/CritterCab.Telemetry.Tests/TelemetryKafkaTestFixture.cs @@ -0,0 +1,93 @@ +using Alba; +using CritterCab.Telemetry.LastKnownPosition; +using Grpc.Net.Client; +using Marten; +using Microsoft.AspNetCore.TestHost; +using Microsoft.Extensions.DependencyInjection; +using Testcontainers.Kafka; +using Testcontainers.PostgreSql; +using Xunit; +using LastKnownPositionDocument = global::CritterCab.Telemetry.LastKnownPosition.LastKnownPosition; + +namespace CritterCab.Telemetry.Tests; + +// A second, heavier fixture that stands up a REAL Kafka broker alongside Postgres, so slice 3's +// publish can be asserted on the wire rather than at a seam. +// +// Deliberately separate from TelemetryTestFixture rather than folded into it. The slice-1, -2 and +// -4 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 the more honest arrangement: the shared +// fixture swaps the publisher seam for a recorder to test the TRIGGER, while this one leaves the +// production wiring intact to test the TRANSPORT. Two different questions, two different hosts. +public class TelemetryKafkaTestFixture : IAsyncLifetime +{ + private readonly PostgreSqlContainer _postgres = new PostgreSqlBuilder("postgres:18-alpine") + .Build(); + + // Image pinned explicitly, like the Postgres container above — Testcontainers has deprecated + // its parameterless builders, and an unpinned broker is a poor thing to depend on for a + // wire-format test. + // + // It must be a cp-kafka image, NOT confluentinc/confluent-local. KafkaBuilder injects its own + // startup script built around cp-kafka's entrypoint; confluent-local runs KRaft and expects + // its log directory to have been formatted by kafka-storage.sh first, so the two combined + // produce a broker that reads zookeeper.properties, finds no meta.properties, and exits 1. + private readonly KafkaContainer _kafka = new KafkaBuilder("confluentinc/cp-kafka:7.6.1") + .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/ConsumerConfig want a bare host:port list, so strip it. + BootstrapServers = _kafka.GetBootstrapAddress().Replace("PLAINTEXT://", string.Empty); + + Host = await AlbaHost.For(builder => + { + builder.UseSetting("ConnectionStrings:crittercab_telemetry", _postgres.GetConnectionString()); + + // Supplying this key is what flips Program.cs from the logging fallback to the real + // KafkaDriverLocationPublisher — the same guarded branch Aspire drives in local dev. + // Nothing in this fixture registers the publisher itself; the point is to exercise + // the production wiring end to end, AutoProvision included. + builder.UseSetting("ConnectionStrings:kafka", BootstrapServers); + + builder.ConfigureTestServices(services => + { + // Same reasoning as the shared fixture: the eviction timer is the untested half of + // slice 4, and a background sweep racing these tests would only add flake. + var timer = services.FirstOrDefault( + d => d.ImplementationType == typeof(LastKnownPositionEvictionService)); + + if (timer is not null) + services.Remove(timer); + }); + }); + } + + public async Task DisposeAsync() + { + await Host.DisposeAsync(); + await Task.WhenAll(_postgres.DisposeAsync().AsTask(), _kafka.DisposeAsync().AsTask()); + } + + public GrpcChannel CreateGrpcChannel() => + GrpcChannel.ForAddress("http://localhost", new GrpcChannelOptions + { + HttpHandler = Host.GetTestServer().CreateHandler() + }); + + public async Task ResetPositionsAsync() + { + var store = Host.Services.GetRequiredService(); + await store.Advanced.Clean.DeleteDocumentsByTypeAsync(typeof(LastKnownPositionDocument)); + } +} + +[CollectionDefinition("TelemetryKafka")] +public class TelemetryKafkaCollection : ICollectionFixture; diff --git a/tests/CritterCab.Telemetry.Tests/TelemetryTestFixture.cs b/tests/CritterCab.Telemetry.Tests/TelemetryTestFixture.cs index b831d9f..40bb218 100644 --- a/tests/CritterCab.Telemetry.Tests/TelemetryTestFixture.cs +++ b/tests/CritterCab.Telemetry.Tests/TelemetryTestFixture.cs @@ -119,10 +119,26 @@ public sealed class RecordingDriverLocationPublisher : IDriverLocationPublisher public IReadOnlyList Published => [.. _published]; - public void Clear() => _published.Clear(); + // Simulates a broker rejection. Slice 3 configures the Kafka endpoint so a failed publish + // throws rather than being swallowed (SendInline + UseSyncRetryBlock), which is what makes + // W006 §6.3's publish-before-store ordering observable. Setting this lets a test assert what + // the handler does with that exception without needing a broker to break. + public bool FailNextPublish { get; set; } + + public void Clear() + { + _published.Clear(); + FailNextPublish = false; + } public Task PublishAsync(DriverLocationUpdated update, CancellationToken ct) { + if (FailNextPublish) + { + FailNextPublish = false; + throw new InvalidOperationException("Simulated broker rejection."); + } + _published.Enqueue(update); return Task.CompletedTask; } From 03d85c5e047ddafa6243be0ffc1d7c6326e9622e Mon Sep 17 00:00:00 2001 From: Erik Shafer Date: Fri, 24 Jul 2026 20:44:25 -0500 Subject: [PATCH 3/5] docs: ADR-019 transport-agnostic topic naming, W006 slice 3 realized, retro MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fires W006 §11 candidate #1, whose trigger was literally "first Kafka topic lands". ADR-019 generalizes ADR-014's . across transports while explicitly withholding its two ASB-specific operational clauses — session keying and outbox coordination — behind a per-transport table. ADR-014 stays Accepted and authoritative for ASB, with a scope note pointing at 019. It also resolves a contradiction that predated this session: the wolverine-kafka skill had independently proposed a stream-descriptive naming rule for Kafka, so the repo held two conventions before either had a real topic to name. That skill's topic-naming and serialization sections are corrected here under the session-runner-blocking exception — a session cannot follow a skill that contradicts the ADR it is authoring. The listener-side examples still name a LocationPing/telemetry.location-pings pairing that never existed; deferred to PR D as DEBT rather than swapped for differently speculative names. W006's Document History records the publish-first qualifier as an amendment rather than a correction: §6.3's failure-mode argument is sound but never states its own precondition, that the publish outcome is known when the store runs. Three DEBT rows: wolverine-kafka listener examples, transport-selection's missing built-vs-modeled status axis, and the aspire skill's AddKafka example, which does not compile on 13.4.6. --- .../014-asb-topic-naming-convention.md | 4 +- .../019-transport-agnostic-topic-naming.md | 70 ++++++++++ docs/decisions/README.md | 1 + docs/prompts/README.md | 1 + .../008-telemetry-slice-3-kafka-publish.md | 2 +- .../008-telemetry-slice-3-kafka-publish.md | 124 ++++++++++++++++++ docs/skills/DEBT.md | 20 +++ docs/skills/wolverine-kafka/SKILL.md | 90 +++++++++---- docs/workshops/006-telemetry-event-model.md | 11 ++ 9 files changed, 294 insertions(+), 29 deletions(-) create mode 100644 docs/decisions/019-transport-agnostic-topic-naming.md create mode 100644 docs/retrospectives/implementations/008-telemetry-slice-3-kafka-publish.md diff --git a/docs/decisions/014-asb-topic-naming-convention.md b/docs/decisions/014-asb-topic-naming-convention.md index afd84aa..62429d8 100644 --- a/docs/decisions/014-asb-topic-naming-convention.md +++ b/docs/decisions/014-asb-topic-naming-convention.md @@ -1,8 +1,10 @@ # ADR-014: Azure Service Bus Topic Naming Convention -**Status:** Accepted +**Status:** Accepted — extended by [ADR-019](./019-transport-agnostic-topic-naming.md) **Date:** 2026-05-10 +> **ADR-019 generalized the naming rule below to every transport.** This ADR remains authoritative for Azure Service Bus, including its two travelling operational decisions (session keying and outbox coordination) — those are ASB-specific and deliberately do **not** generalize. See ADR-019's per-transport table before applying anything here to a non-ASB broker. + ## Context Business events crossing service boundaries travel via Azure Service Bus per ADR-005's transport-selection decision. With two bounded contexts now publishing to ASB, seven cross-BC topics exist: diff --git a/docs/decisions/019-transport-agnostic-topic-naming.md b/docs/decisions/019-transport-agnostic-topic-naming.md new file mode 100644 index 0000000..77d4c52 --- /dev/null +++ b/docs/decisions/019-transport-agnostic-topic-naming.md @@ -0,0 +1,70 @@ +# ADR-019: Transport-Agnostic Topic Naming + +**Status:** Accepted +**Date:** 2026-07-24 + +## Context + +[ADR-014](./014-asb-topic-naming-convention.md) codified `.` as CritterCab's topic-naming convention, drawn from the seven Azure Service Bus topics Workshops 001 and 002 had produced. It was scoped to ASB because ASB was the only broker anything had been modeled against — the ADR's own title says so. + +CritterCab now has a Kafka topic. Workshop 006 §6.3 locks Telemetry's `DriverLocationUpdated` publication to `telemetry.driver-location-updated`, and the implementation session for that slice is the first code in the repository that produces to a broker other than the (still unbuilt) ASB. W006 §11 anticipated exactly this and registered "generalize ADR-014 transport-agnostically" as an ADR candidate whose trigger was *the first Kafka topic landing*. That trigger has fired. + +Two things make this worth a decision rather than an assumption. + +**First, the convention had already drifted in the skill layer.** [`docs/skills/wolverine-kafka/SKILL.md`](../skills/wolverine-kafka/SKILL.md) § Topic naming convention was written ahead of any implementation and independently proposed a *different* rule for Kafka: topics named `.`, with the explicit guidance that a topic "carries a descriptive name rather than a message-type name" because "a topic like `telemetry.location-pings` may carry `LocationPing` messages today and an enriched `LocationPingV2` tomorrow." Its illustrative topics (`telemetry.location-pings`, `telemetry.demand-signals`, `pricing.surge-updates`) were speculative — none had ever been published. So the repository held two conventions that produce similar-looking names from genuinely different rules, and nothing had forced the question until a real topic needed a name. + +**Second, ADR-014 bundles two operational decisions with the name**, and it is not obvious that either survives the move to Kafka. ADR-014 mandates `SessionId = ` for ordering and Wolverine outbox coordination between the topic publication and the local event append. Kafka has no sessions, and W006 §6.3 explicitly rejects the outbox for this flow. A naming ADR that quietly carried those clauses across would import ASB semantics into a transport that cannot honor them. + +## Options Considered + +### Option A — Per-transport conventions + +Let each transport keep the convention that suits it: ASB stays on ADR-014's `.`, and Kafka adopts the stream-descriptive `.` the `wolverine-kafka` skill proposed. + +This option has a real argument behind it, and it is not the one it appears to be. It is not "different brokers, different habits" — it is that Kafka topics and ASB topics have genuinely different lifetimes. An ASB topic carries discrete business events that a consumer subscribes to and acknowledges; a Kafka topic is a durable, replayable log whose retention outlives any individual payload shape. Naming a log after the message type that happens to be in it today invites a rename the moment the payload evolves, and renaming a Kafka topic is not a rename — it is a migration with dual-write and consumer cutover. + +The cost is that CritterCab would have to explain, at every future BC's first publication, which rule applies before anyone could name anything. Ownership tooling would need two glob patterns. And the premise is weaker than it looks in this codebase specifically: CritterCab versions its contracts in the proto package path (`crittercab.telemetry.v1`, per [ADR-009](./009-protobuf-contracts-as-first-class-artifacts.md)), so payload evolution is already handled a layer down. A `v2` with breaking changes gets a new proto package and, if the break is real, a new topic regardless of how the old one was named — which is the migration this option was trying to avoid, arriving anyway. + +### Option B — One convention across transports: `.` + +ADR-014's Option C generalizes unchanged. `dispatch.ride-assigned` on ASB and `telemetry.driver-location-updated` on Kafka are the same rule applied twice. + +The naming rule is the part of ADR-014 that was never actually ASB-specific — nothing in `.` depends on sessions, subscriptions, or dead-letter semantics. Ownership stays legible from the name alone, `dispatch.*` and `telemetry.*` keep working as glob patterns across a heterogeneous transport estate, and a contributor asking "what does Telemetry publish?" gets one answer rather than one per broker. It also keeps the topic name aligned with the protobuf message name, which is the artifact under review — an operator reading `telemetry.driver-location-updated` in a log line can find `driver_location_updated.proto` without a lookup table. + +The cost is Option A's argument, accepted rather than dismissed: a Kafka topic named after its event is a topic that reads oddly if the event is ever substantially reshaped without a version bump. CritterCab takes that cost because ADR-009 makes the reshape-without-a-version-bump case the one that should not happen. + +### Option C — Encode the transport or version in the name + +`kafka.telemetry.driver-location-updated`, or `telemetry.driver-location-updated.v1`. + +Transport in the name is redundant with the connection: a consumer already knows which broker it is talking to, and encoding it makes a transport migration a rename of every topic. Version in the name duplicates the proto package's version segment and creates a second place for the two to disagree. Neither earns its characters in every log line. + +## Decision + +**Option B.** CritterCab names topics `.` on **every** transport. ADR-014's naming rule is hereby transport-agnostic; `telemetry.driver-location-updated` is its first Kafka instance. + +**ADR-014 is extended, not superseded.** It remains Accepted and remains authoritative for Azure Service Bus. What this ADR generalizes is its naming rule only. Its two travelling operational decisions **do not generalize**, and the per-transport table below is the load-bearing content of this decision: + +| Concern | Azure Service Bus (ADR-014) | Kafka (this ADR, W006 §6.3) | +|---|---|---| +| Topic name | `.` | `.` — same rule | +| Ordering mechanism | `SessionId = ` ([ADR-013](./013-shared-cross-bc-identifier.md)); ASB delivers a session in publish order | Partition key = the stream's natural partition (`driverId` for driver telemetry); per-partition ordering is a broker property | +| Publish/persist coupling | Wolverine outbox — the topic publication commits in the same transaction as the local event append | **No outbox.** Publish first, then persist; inline send so a broker rejection prevents the local write | +| Duplicate handling | Outbox makes duplicates rare; consumers are idempotent by handler | At-least-once by design; consumers dedup on `(partition key, server-stamped timestamp)` | +| Payload serialization | Wolverine's default envelope JSON | Endpoint-scoped binary protobuf where the payload is a generated contract type (ADR-009) | + +The distinction to carry forward: **the name is a convention; the delivery semantics are a property of the transport and the flow.** Naming them in one ADR is what keeps a future BC from inheriting ASB's outbox guarantees by way of a shared topic-name rule. + +Where the `wolverine-kafka` skill's topic-naming and serialization guidance conflicts with this ADR, this ADR wins; the skill is corrected in the session that ships this decision. + +## Consequences + +Every future CritterCab publication is named the same way regardless of broker. Identity's `identity.rider-registered`, Payments' `payments.fare-settled`, and Pricing's `pricing.surge-updated` inherit the convention whether they land on ASB or Kafka, and none of those BCs has to re-derive the question or discover the skill-vs-ADR conflict this decision resolves. + +Ownership tooling stays single-pattern. "What does Telemetry publish?" is answerable with `telemetry.*` across the whole estate. A topic inventory that spans brokers groups by BC without a per-transport translation step — which matters more as CritterCab's transport count grows from two to three. + +The per-transport table becomes the thing new flows consult, not the name. A BC adding its first Kafka topic now has a written answer for the questions that actually bite — how ordering is achieved, whether the outbox applies, how consumers dedup — instead of inferring them from an ASB-shaped ADR. This is the clause most likely to be extended: a third transport adds a column rather than a competing convention. + +Renaming a topic remains expensive, and this ADR does not make it cheaper. It accepts Option A's argument on the merits and answers it with ADR-009 rather than with a naming escape hatch: if a payload's evolution is breaking enough to make `driver-location-updated` a misleading topic name, it is breaking enough to need a new proto package and a deliberate consumer migration. The naming convention is not the mechanism that absorbs contract drift; the contract layer is. + +`telemetry.driver-location-updated` is the reference instance. It is produced with partition key `driverId`, dedup key `serverReceivedAt`, no outbox, inline sending, and binary protobuf — every row of the Kafka column exercised at once, in code, under test. diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 9e28569..3241e23 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -24,3 +24,4 @@ For the template and guidelines on when to write an ADR, see [ADR-001](./001-rec | [016](./016-frontend-live-update-transport.md) | Frontend Live-Update Transport | Accepted | | [017](./017-rabbitmq-for-critterwatch.md) | RabbitMQ for CritterWatch | Accepted | | [018](./018-candidate-projection-ownership-and-telemetry-geospatial-supply.md) | Candidate-Projection Ownership and Telemetry Geospatial Supply | Accepted | +| [019](./019-transport-agnostic-topic-naming.md) | Transport-Agnostic Topic Naming | Accepted | diff --git a/docs/prompts/README.md b/docs/prompts/README.md index 92c00ff..f7972b7 100644 --- a/docs/prompts/README.md +++ b/docs/prompts/README.md @@ -169,3 +169,4 @@ Subsequent sections are prompt-specific. Existing prompts in this directory serv - [`implementations/005-dispatch-slice-5-3-candidates-selected.md`](./implementations/005-dispatch-slice-5-3-candidates-selected.md) — Fourth vertical slice: first slice of the dispatch-round arc. `CandidateSelectionAutomation` reacts to `FareQuoted`, queries `INearbyAvailableDriversSource` (Telemetry + Driver Profile translation-in stub; parking-lot #4 deferred), and emits `CandidatesSelected` (happy path) or `NoCandidatesAvailable` (empty-set path) as a Klefter decision-event. Adds `DispatchPolicySnapshot` DI record (hardcoded `searchRadiusMeters: 5000`, `maxCandidatesPerRound: 5`; Slice 11 swaps for `DispatchPolicyConfigured`-fed projection), `ICandidateSelectionOutcome` marker interface (third instance of the pattern), `RequestRoundsProjection` inline projection (consumed by Slice 9), and `RequestTimeline` extensions for both outcome events. Three Alba integration tests, one per W001 §5.3 GWT. First use of `[WriteAggregate]` bound to a non-first stream event in the codebase. **Third substantive forward exercise of the spec-delta closure-loop convention.** Status: pending (authored 2026-06-16). - [`implementations/006-telemetry-skeleton-and-slice-1-config.md`](./implementations/006-telemetry-skeleton-and-slice-1-config.md) — **Second service in the repo** and the opening PR of the W006 Telemetry transport chain. Bootstraps the `CritterCab.Telemetry` service skeleton **and** W006 slice 1 (`TelemetryPolicyConfigured` config-as-events) in one PR, per the named skeleton-plus-first-slice cadence exception (mirrors the Dispatch skeleton + slice-5.1 precedent). Config-as-events is the dependency-correct first slice: slice 2's gRPC ingest reads this slice's `TelemetryPolicy` view (`throttlePolicyVersion`, `h3Resolution`, intervals). Establishes two firsts in code — **first config-as-events instance** (ADR-011's third instance, first realized; Dispatch/Onboarding were design-only) and **first FluentValidation boundary validation**. Four pre-flight `jasperfx-source-verifier` gates (`IInitialData` seed vehicle, `long` stream-version property, self-aggregating `LiveStreamAggregation` registration + Marten-9 `partial` scope, Wolverine.HTTP FluentValidation middleware). None of W006 §11's three ADR candidates fired (all later-arc), but the Phase-2 audit surfaced an ADR-011 Option-A/B-for-Marten gap that — **per user direction, expanding the session mid-flight** — was resolved in-PR via an **ADR-011 amendment** (`IInitialData` as the canonical Marten Option-A realization; LWW for config singletons). No narrative anchor (PR #40: the narrative layer does not apply to Telemetry). Kafka deliberately **not** wired into `apphost.cs` this PR (transport lands with the slice that needs it — Dispatch-skeleton precedent). Substantive spec delta: W006 §6.1 designed → realized. Status: pending (authored 2026-07-10). - [`implementations/007-telemetry-slices-4-and-2-transport.md`](./implementations/007-telemetry-slices-4-and-2-transport.md) — **CritterCab's first transport in code.** W006 slice 4 (`LastKnownPosition` store + heartbeat-absence eviction) and slice 2 (gRPC `ReportLocations` client-streaming ingest) in one PR under the **coupled-slices** reading of the cadence rule, not the skeleton-plus-first-slice exception (already spent on 006): slice 2's publish trigger evaluates against slice 4's document and slice 4's document is written only when slice 2 publishes, so building either alone means evaluating against a document that does not exist or writing one nothing reads. Establishes five firsts in code — **first gRPC surface serving traffic**, **first client-streaming RPC**, **first proto codegen** (`protos/` had been contracts with no consumer since PR #39), **first non-event-sourced document write path**, and **first recurring/scheduled work**. Retires the three-month client-streaming forward-constraint: WolverineFx.Grpc **6.21.0** auto-generates the shape, so nothing is hand-wired, and the two gRPC skills that still described the workaround are corrected in-session under the session-runner-blocking exception (a session cannot follow a skill telling it to hand-wire). Eleven Verify-before-wiring gates were source-verified before any code; gate 5 **corrected the prompt's own hypothesis** (Wolverine has no recurring-message primitive — the idiom is a plain `BackgroundService`) and gate 11 dissolved (Alba's `TestServer` feeds a `GrpcChannel` directly, so the Alba-first default holds for gRPC). Two W006 under-specifications surfaced and were resolved by user sign-off rather than silently: the `LastKnownPosition` field set (`lastPublishedAt` collapsed into `serverReceivedAt`, since upsert-on-publish-only makes them one instant) and the `accuracyMeters` threshold (**100m, invented at implementation time — W006 names the threshold but fixes no value**). Also fixed `apphost.cs`, which had not compiled since PR #42 and which CI does not build. Substantive spec delta: W006 §6.2 + §6.4 designed → realized; the §11 *windowed client-streaming* ADR candidate lands as a skill, not an ADR. Status: complete (authored 2026-07-20, executed 2026-07-24). Produced retro at [`retrospectives/implementations/007-telemetry-slices-4-and-2-transport.md`](../retrospectives/implementations/007-telemetry-slices-4-and-2-transport.md). +- [`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). diff --git a/docs/prompts/implementations/008-telemetry-slice-3-kafka-publish.md b/docs/prompts/implementations/008-telemetry-slice-3-kafka-publish.md index d20b150..aba524e 100644 --- a/docs/prompts/implementations/008-telemetry-slice-3-kafka-publish.md +++ b/docs/prompts/implementations/008-telemetry-slice-3-kafka-publish.md @@ -2,7 +2,7 @@ | Field | Value | |---|---| -| **Status** | **Ready** — the two durable forks resolved by the user 2026-07-24 (delivery semantics: `SendInline` + `UseSyncRetryBlock`; the W006 §11 Kafka-topic-naming candidate lands as **ADR-019 inside this PR**). Rides in the PR C session's PR alongside the implementation; not committed standalone. | +| **Status** | **Complete (2026-07-24)** — executed as PR C; retro at [`retrospectives/implementations/008-telemetry-slice-3-kafka-publish.md`](../../retrospectives/implementations/008-telemetry-slice-3-kafka-publish.md). Originally: Ready — three durable forks resolved by the user 2026-07-24 (delivery semantics: `SendInline` + `UseSyncRetryBlock`; the W006 §11 Kafka-topic-naming candidate lands as **ADR-019 inside this PR**; protobuf-over-JSON on the wire, escalated after Phase 1 found the `wolverine-kafka` skill said otherwise). Rides in the PR C session's PR alongside the implementation; not committed standalone. | | **Authored** | 2026-07-24 | | **Target artifacts** | `Directory.Packages.props` (`WolverineFx.Protobuf` 6.21.0; `Confluent.Kafka` for the test project), `src/CritterCab.Telemetry/CritterCab.Telemetry.csproj` (`WolverineFx.Kafka`, `WolverineFx.Protobuf`), `src/CritterCab.Telemetry/ReportLocations/IDriverLocationPublisher.cs` (**implementation only — the interface does not change**), `src/CritterCab.Telemetry/Program.cs` (Kafka transport + publishing rule + durability), `tests/CritterCab.Telemetry.Tests/` (Kafka Testcontainer + round-trip assertion), `apphost.cs` (Kafka resource + reference), `protos/crittercab/telemetry/v1/driver_location_updated.proto` (topic/partition-key comment now describes shipped code), **`docs/decisions/019-transport-agnostic-topic-naming.md` (new)**, `docs/workshops/006-telemetry-event-model.md` (Document History), `docs/skills/transport-selection/SKILL.md` (Kafka row moves modeled → built), `docs/skills/DEBT.md`, `docs/prompts/README.md` (index entry), this prompt's retro. | | **Source-of-truth dependencies** | [W006 §6.3 (the slice), §6.2 (the trigger that fires it), §3.2 (published-not-stored), §11 #1 (the ADR candidate)](../../workshops/006-telemetry-event-model.md); [ADR-005](../../decisions/005-transport-selection-by-flow-type.md) (Kafka for high-volume telemetry); [ADR-009](../../decisions/009-protobuf-contracts-as-first-class-artifacts.md) (the proto is the contract); [ADR-014](../../decisions/014-asb-topic-naming-convention.md) (the ASB-scoped convention ADR-019 generalizes); [ADR-018](../../decisions/018-candidate-projection-ownership-and-telemetry-geospatial-supply.md) (supplier half). Skills: **`wolverine-kafka`** (the load-bearing one — added by the Phase 1 auditor pass, not in this prompt's first draft), `service-bootstrap`, `transport-selection`, `aspire`, `protobuf-contracts`, `vertical-slice-organization`, `csharp-coding-standards`, `testing-integration`, `testing-fundamentals`. | diff --git a/docs/retrospectives/implementations/008-telemetry-slice-3-kafka-publish.md b/docs/retrospectives/implementations/008-telemetry-slice-3-kafka-publish.md new file mode 100644 index 0000000..c9ca495 --- /dev/null +++ b/docs/retrospectives/implementations/008-telemetry-slice-3-kafka-publish.md @@ -0,0 +1,124 @@ +# Retrospective — Telemetry Slice 3 (`DriverLocationUpdated` → Kafka) + +## Metadata + +- **Triggering prompt:** [`docs/prompts/implementations/008-telemetry-slice-3-kafka-publish.md`](../../prompts/implementations/008-telemetry-slice-3-kafka-publish.md) +- **Status:** Complete +- **Date authored:** 2026-07-24 +- **Output artifacts:** + - `Directory.Packages.props` — `WolverineFx.Protobuf` 6.21.0 (lockstep with the other eleven `WolverineFx*` entries); `Confluent.Kafka` 2.14.0 for the test project. `WolverineFx.Kafka`, `Aspire.Hosting.Kafka` and `Testcontainers.Kafka` were already pinned and needed no version work. + - `src/CritterCab.Telemetry/CritterCab.Telemetry.csproj` — `WolverineFx.Kafka` + `WolverineFx.Protobuf` references + - `src/CritterCab.Telemetry/ReportLocations/KafkaDriverLocationPublisher.cs` — **the whole behavioral change of the PR**; one method + - `src/CritterCab.Telemetry/ReportLocations/IDriverLocationPublisher.cs` — comments only; the interface declaration is byte-identical + - `src/CritterCab.Telemetry/Program.cs` — Kafka transport behind a connection-string guard, publishing rule (`SendInline` + `UseIdempotentProducer` + endpoint-scoped `UseProtobufSerialization`), `Durability.UseSyncRetryBlock` + - `apphost.cs` — `AddKafka("kafka", port: 5392)`, persistent lifetime, referenced + waited on by Telemetry + - `tests/CritterCab.Telemetry.Tests/TelemetryKafkaTestFixture.cs` — second fixture, Postgres + a real broker + - `tests/CritterCab.Telemetry.Tests/ReportLocations/Slice3KafkaPublishTests.cs` — the round trip, asserted on the raw record + - `tests/CritterCab.Telemetry.Tests/ReportLocations/Slice3PublishOrderingTests.cs` — a failed publish leaves no baseline + - `tests/CritterCab.Telemetry.Tests/TelemetryTestFixture.cs` — `RecordingDriverLocationPublisher` gains `FailNextPublish` + - **`docs/decisions/019-transport-agnostic-topic-naming.md`** — new ADR; `docs/decisions/014-*.md` status line + scope note; `docs/decisions/README.md` index + - `docs/skills/wolverine-kafka/SKILL.md` — topic-naming and serialization sections corrected, two pitfalls added, publish-path examples reconciled against shipped code + - `docs/skills/DEBT.md` — 3 new rows + - `docs/workshops/006-telemetry-event-model.md` `## Document History` — §6.3 realized; §11 candidate #1 discharged; the publish-first qualifier recorded + - `docs/prompts/README.md` — Implementations index entry + - This retro +- **Outcome:** Slice 3 implemented end-to-end. **33/33 green locally** (Telemetry 22, Dispatch 11), 0 warnings. CritterCab's **first Kafka topic** and **second live transport**. `ReportLocationsHandler.cs` does not appear in the diff. + +--- + +## Framing + +PR B's job was to make gRPC real. This one's job was to make Kafka real, and it was deliberately set up to be small: PR B had already put `driver_location_updated.proto` through codegen and typed the `IDriverLocationPublisher` seam against the generated `DriverLocationUpdated`, so the contract, the payload construction, the publish trigger and the publish-first ordering were all shipped and under test before this session started. What remained was one implementation class and the host wiring behind it. + +That setup held. The session's actual difficulty was somewhere the prompt only half-anticipated: **not in writing the producer, but in discovering that the workshop's reasoning about it rested on an assumption no one had written down.** + +--- + +## Outcome summary + +The seam paid off exactly as designed. `KafkaDriverLocationPublisher` is one method — `bus.PublishAsync(update, new DeliveryOptions { PartitionKey = update.DriverId })` — and the diff touches no handler, no payload construction, and no test that was asserting on the trigger. The prompt's instruction that `ReportLocationsHandler.cs` should not appear in the diff was met literally, which is the cleanest available evidence that a seam introduced one PR early was introduced at the right place. + +Everything else that made the session interesting came from two sources: a source-verification pass that found a gap between spec and library defaults, and a skill-discovery pass that found the repo already disagreed with itself about Kafka. + +--- + +## What worked + +**Source-verifying the transport before authoring the prompt, not before writing the code.** Prompt 007 ran its gate pass partly *during* the session and carried several gates open into implementation. This session ran a full `jasperfx-source-verifier` pass against local `wolverine @ V6.21.0-12-ge08abdeb3` while the prompt was still being drafted, and all six gates closed before a line of the prompt's deliverable plan was written. That ordering is what surfaced the delivery-semantics problem early enough to become a signed-off fork rather than a mid-session surprise — and it is the single practice most worth repeating. + +**The Phase 1 auditor pass earned its cost, and would have been cheap to skip.** It corrected the prompt in four places *after* the prompt had already been source-verified and committed, which is the useful data point: source verification and convention discovery catch different classes of error and neither substitutes for the other. It found a governing skill the prompt had missed entirely (`wolverine-kafka`), overturned the prompt's bootstrap lean, found the Kafka port already allocated, and surfaced both skill-vs-spec contradictions. + +**Two tests, two different questions, two different hosts.** The round-trip test reads the raw Kafka record — asserting the partition key and that the value is binary protobuf the generated parser accepts — on its own fixture with a real broker. The ordering test uses a throwing publisher and no broker at all. Keeping them apart meant the slice-1/2/4 suites never wait on Kafka, and it kept each test honest about what it actually proves. + +**Asserting the partition key was not incidental.** It is the one assertion in the suite guarding a failure that is otherwise invisible: without a key the records still arrive, the test still sees a message, and only the per-driver ordering silently breaks. A test that merely confirmed "something landed on the topic" would have passed against a materially broken system. + +--- + +## What was harder than expected + +**Nothing about the producer.** Worth stating plainly, because the estimate was right for once: the code half of this session was as small as the seam promised. + +**The Aspire example in our own skill does not compile.** `builder.AddKafka("kafka").WithHostPort(5392)` — copied from `aspire/SKILL.md` § Port allocation — fails with CS1929 on Aspire 13.4.6, because `WithHostPort` belongs to the Kafka *UI* container resource, not the broker. The port takes a constructor argument instead. This is a small fix but a pointed one: it went unnoticed because `apphost.cs` is a file-based app with no `.csproj`, so CI's "Verify solution completeness" step does not reach it, and nothing mechanically checks either the apphost or the examples written against it. Registered as a DEBT row; the CI gap remains its own session. + +**Choosing a Testcontainers Kafka image is not a free choice.** `confluentinc/confluent-local:7.6.1` — the KRaft image Wolverine's own docker-compose uses — produced a container that exited 1 with `No 'meta.properties' found`. Testcontainers' `KafkaBuilder` injects a startup script built around `cp-kafka`'s entrypoint, and confluent-local expects its log directory formatted by `kafka-storage.sh` first; the combination yields a broker that reads `zookeeper.properties`, finds nothing, and dies. `confluentinc/cp-kafka:7.6.1` works. Pinned with a comment, because the failure mode reads as a Kafka problem rather than an image-compatibility problem. + +--- + +### Design meets code — the assumption W006 §6.3 did not know it was making + +This is the session's substantive finding, and it is the kind that only surfaces at implementation. + +W006 §6.3 argues for publish-before-store from failure-mode asymmetry, and the argument is genuinely good: a failed *store* after a good publish costs a duplicate the consumer's `(driverId, serverReceivedAt)` dedup absorbs, while a failed *publish* after a good store costs a **miss** Dispatch cannot detect. The workshop chose the benign branch and wrote down why. + +What it could not have known is that the conclusion depends on a precondition it never states: **the publish's outcome has to be known by the time the store runs.** Under Wolverine's defaults it is not, twice over. + +1. A Kafka publishing endpoint defaults to `BufferedInMemory`, which batches into an in-process queue. `await PublishAsync(...)` returns before the broker has seen the record. +2. Even `SendInline()` — which does await the broker ack — is insufficient on its own, because `InlineSendingAgent` selects its retry block on `DurabilitySettings.UseSyncRetryBlock`, which has no initializer and defaults to `false`. The resulting async `RetryBlock.PostAsync` catches the exception, logs it, re-posts to a background block, and **returns success**. + +So the naive implementation — write the two calls in the order §6.3 specifies — would have produced a system where publish-first is true in statement order only, every broker failure lands in the branch §6.3 rejected, and nothing anywhere looks wrong. The workshop's reasoning would have been quietly inverted by a default. + +The session ships `SendInline()` + `UseSyncRetryBlock = true` + `UseIdempotentProducer()`, which makes a rejection throw, skip the upsert, leave the baseline stale, and let the driver's next ping republish. **Recorded in W006's Document History as an amendment, not a correction** — §6.3's argument stands; it simply did not name its own precondition. + +Two honest caveats on it. `UseSyncRetryBlock` is **process-global**, not per-endpoint; affordable because Telemetry publishes to exactly one transport, and a second publisher should re-weigh it rather than inherit it. And the half of the claim that says *Wolverine's inline sender surfaces a broker rejection as an exception at all* rests on source verification plus the endpoint configuration, **not on a test** — `Slice3PublishOrderingTests` covers the handler's response to a throwing publisher, which is CritterCab's half of the contract. Breaking a live broker mid-test to cover the other half would be slow and flaky for a claim already traced to source; naming the gap here is the better trade, but it is a gap. + +--- + +### The repo already disagreed with itself about Kafka + +`wolverine-kafka/SKILL.md` was written well ahead of any implementation, and had independently arrived at two positions the shipping code contradicts. + +**Topic naming.** The skill proposed `.` with the explicit rule that a topic "carries a descriptive name rather than a message-type name," reasoning that a topic outlives any one payload shape. ADR-014 and W006 §6.3 mandate `.`, which *is* a message-type name. Both rules produce similar-looking strings from genuinely different logic, so nothing forced the question until a real topic needed naming. Spec beat skill — but the skill's argument was good enough to deserve a real answer rather than a dismissal, and giving it one is what turned ADR-019 from a rubber stamp into a decision with something at stake. The answer: CritterCab versions contracts in the proto package path (ADR-009), so payload evolution is handled a layer below the topic name, and a break large enough to make the event name misleading needs a new proto package and a deliberate consumer migration regardless. + +**Serialization.** The skill said "Cab uses Wolverine's default JSON serialization" and deferred protobuf to "a future phase." Since the payload is a `Google.Protobuf.IMessage` generated from the contract of record, JSON would have left ADR-009's proto governing the C# type but not the wire. This was **escalated to user sign-off rather than absorbed as a session-runner call** — a written convention saying "not yet" is a different thing from a convention that is merely silent, and overriding it is not the session runner's to decide alone. + +Both corrections rode in-PR under the session-runner-blocking exception, on the same reasoning PR B used for the gRPC skills: a session cannot follow a skill that contradicts the ADR it is authoring. The scope was held to the publish path; the listener/consumer-group/batching examples still name a `LocationPing` → `telemetry.location-pings` pairing that never existed, which is now *actively* misleading because `LocationPing` is a real type doing a different job. Those describe slice 5, so they are a DEBT row for PR D to fix from a shipped consumer rather than swap for differently speculative names. + +--- + +## Methodology refinements + +- **Run the source-verification pass during prompt authoring, not at session start.** The gap it found here changed a signed-off fork. Discovered mid-session it would have changed code already written. +- **Source verification and convention discovery are not substitutes.** This prompt was thoroughly gate-verified and still had four convention-level errors the Phase 1 auditor caught, including a missing governing skill. Both passes, in that order. +- **A skill that says "not yet" is a decision, not a silence.** When a session's plan overrides one, that belongs in front of the user. When a skill is merely absent on a point, the session runner decides. This session had one of each — protobuf serialization (escalated) and endpoint-scoped-vs-global (decided) — and the line held usefully. +- **When correcting a skill in-session, scope the correction to what the session actually shipped.** The publish path was corrected against real code; the listener half was left with a warning and a DEBT row, because correcting it now would mean inventing the consumer PR D is going to build. + +--- + +## Outstanding items / next-session inputs + +- **PR D — W006 slice 5**, the last pending slice: Dispatch consumes `telemetry.driver-location-updated` into the `AvailableDriver` view, replacing `NearbyAvailableDriversStub`, and closes the W001 §5.3 amendment. It also owns §6.3's **Dedup GWT**, which asserts consumer behavior and could not be tested here. Two constraints already verified for it: `ListenToKafkaTopic(...).ConfigureConsumer(c => c.GroupId = ...)` is the listener shape (there is no `.GroupId(string)`), and the listener **must** carry `UseProtobufSerialization()` because `ProtobufMessageSerializer.ReadFromData(byte[])` throws — only the `(Type, Envelope)` overload works. +- **Three new DEBT rows**: `wolverine-kafka` listener examples (drain with PR D), `transport-selection`'s missing built-vs-modeled status axis (two of three transports are now built), and the `aspire` `AddKafka` example. +- **Design-return cadence.** ADR-019 served as this run's interleave, so the counter is satisfied — but ASB is now the only modeled-and-unbuilt transport, and the Driver Profile workshop is the prerequisite for the ASB half of ADR-018's join. That is the natural design-side successor once slice 5 lands. +- **CLAUDE.md's status line is further out of date than it was**, and PR [#43](https://github.com/erikshafer/CritterCab/pull/43) — already stale on arrival for describing a transport-less Telemetry — is now stale in one more respect. Re-read its diff before merging; accurate now is *two services, two live transports, W006 slices 1/2/3/4 realized.* +- **The CI-cannot-see-`apphost.cs` gap bit again** (this session edited the file and caught a compile error only locally). Still its own session; extend the existing "Verify solution completeness" step rather than adding a guard. + +--- + +## Spec delta — landed? + +**Yes, in full, plus one amendment the prompt anticipated only in outline.** + +- **W006 §6.3 designed → realized.** Topic, partition key, dedup key, and publish-first/no-outbox coupling are all concrete and under test. First Kafka topic; second live transport. Recorded in [W006 `## Document History`](../../workshops/006-telemetry-event-model.md#document-history) (2026-07-24, second entry). +- **W006 §11 candidate #1 fired as an ADR**, as planned: [ADR-019 — Transport-Agnostic Topic Naming](../../decisions/019-transport-agnostic-topic-naming.md), generalizing ADR-014's naming rule across transports while explicitly withholding its two ASB-specific operational clauses. ADR-014's status line now cross-references it. Candidate #2 remains later-arc; #3 discharged in PR B. +- **The publish-first qualifier landed as a spec amendment**, which the prompt named as a spec-delta line before the session knew how load-bearing it would be. W006 §6.3's ordering guarantee is now recorded as a configuration commitment rather than a free consequence of statement order. +- **One delta the prompt did not name:** ADR-019 also resolves a skill-vs-ADR contradiction on topic naming that predated this session. That was discovered by the Phase 1 pass after the prompt's spec-delta section was written, and it is the more durable half of what ADR-019 does. diff --git a/docs/skills/DEBT.md b/docs/skills/DEBT.md index e437fb0..77062e6 100644 --- a/docs/skills/DEBT.md +++ b/docs/skills/DEBT.md @@ -71,6 +71,25 @@ 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 + +- **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). + +### `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. +- **Not fixed in-session by design:** adding a status axis is a structural change to a skill this session was otherwise only consuming, and prompt 008 named the contingency in advance (check first; register a row rather than invent a structure mid-session). A tidy session should settle the axis shape once, ideally alongside whichever session builds ASB and makes all three rows answerable. +- **Retro source:** [`retrospectives/implementations/008-telemetry-slice-3-kafka-publish.md`](../retrospectives/implementations/008-telemetry-slice-3-kafka-publish.md). + +### `aspire` — the `AddKafka` port-allocation example does not compile on 13.4.6 + +- **Gap:** § Port allocation's worked example shows `builder.AddKafka("kafka").WithHostPort(5392)`. On Aspire 13.4.6 `WithHostPort` is an extension on `IResourceBuilder` — the Kafka **UI** container, not the broker — so the line fails with CS1929. The broker takes its port as a constructor argument: `builder.AddKafka("kafka", port: 5392)`. PR C hit this on first build. The reserved port **5392 itself was correct** and is now claimed. +- **Why it went unnoticed:** `apphost.cs` is a file-based app with no `.csproj`, and CI's "Verify solution completeness" step does not reach it — so nothing mechanically checks the apphost or examples written against it. That CI gap is its own session (CI changes carry their own blast radius); this row is only the skill fix. +- **Retro source:** [`retrospectives/implementations/008-telemetry-slice-3-kafka-publish.md`](../retrospectives/implementations/008-telemetry-slice-3-kafka-publish.md). + --- ## Recently drained @@ -107,4 +126,5 @@ Older entries drop off; the retros and commits remain authoritative. - **2026-05-08.** Initial authoring. Seven rows from the post-D→B→C session — five `marten-*` Marten 8.x / JasperFx namespace extractions plus two `service-bootstrap` registration prerequisites. Three other gaps from the same session (`RunOaktonCommandsAsync` → `RunJasperFxCommands`, `protobuf-contracts` directory layout, `service-bootstrap`/`aspire` connection-string contradiction) were fixed in-flight under the session-runner-blocking exception and do not appear here. - **2026-05-08 (later same day).** Initial 7-row backlog drained via the first skill-tidy session. `Open debt` reset to empty. Retro at [`docs/retrospectives/skills-tidy-marten-and-bootstrap.md`](../retrospectives/skills-tidy-marten-and-bootstrap.md). - **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 three 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, and the `aspire` skill's non-compiling `AddKafka` port example. 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). diff --git a/docs/skills/wolverine-kafka/SKILL.md b/docs/skills/wolverine-kafka/SKILL.md index 531c0f5..b9486f8 100644 --- a/docs/skills/wolverine-kafka/SKILL.md +++ b/docs/skills/wolverine-kafka/SKILL.md @@ -36,23 +36,25 @@ In production Cab runs Kafka protocol against **Azure Event Hubs**. Locally, Asp ``` Telemetry service Dispatch service ┌─────────────────┐ ┌─────────────────┐ -│ GPS handler │──publish──► │ LocationPing │ +│ Ingest handler │──publish──► │ DriverLocation │ │ (Wolverine) │ │ │ handler │ └─────────────────┘ │ └─────────────────┘ ▼ ▲ ┌─────────────────┐ │ │ Kafka topic │──consume────┘ │ telemetry. │──consume────┐ - │ location-pings │ │ - └─────────────────┘ ▼ - ┌─────────────────┐ - │ Pricing service │ - │ LocationPing │ + │ driver- │ │ + │ location- │ ▼ + │ updated │ ┌─────────────────┐ + └─────────────────┘ │ Pricing service │ + │ (future) │ │ handler │ └─────────────────┘ ``` -The Telemetry service publishes `LocationPing` messages to a Kafka topic partitioned by `driver_id`. Dispatch and Pricing each consume the same topic with independent consumer groups. Each handler is a plain messaging handler — it receives a `LocationPing` 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; 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. + +> **⚠ 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. ## Bootstrap @@ -67,8 +69,8 @@ builder.Host.UseWolverine(opts => opts.UseKafkaUsingNamedConnection("kafka") .AutoProvision(); - opts.PublishMessage() - .ToKafkaTopic("telemetry.location-pings"); + opts.PublishMessage() + .ToKafkaTopic("telemetry.driver-location-updated"); }); ``` @@ -88,17 +90,15 @@ For services that only consume (never publish), call `.ConsumeOnly()` to skip th ## Topic naming convention -Cab topics follow the pattern `.`, lowercase with hyphens separating words within a segment and dots separating the bounded-context prefix from the topic name: +**Governed by [ADR-019](../../decisions/019-transport-agnostic-topic-naming.md).** Cab topics are named `.` — the source bounded context's slug, a dot, then the event name in kebab-case. The same rule applies on every transport; Kafka does not get its own convention. -| Topic | Publisher | Consumers | Partition key | -|---|---|---|---| -| `telemetry.location-pings` | Telemetry | Dispatch, Pricing | `driver_id` | -| `telemetry.demand-signals` | Telemetry | Pricing | `zone_id` | -| `pricing.surge-updates` | Pricing | Dispatch | `zone_id` | +| Topic | Publisher | Consumers | Partition key | Status | +|---|---|---|---|---| +| `telemetry.driver-location-updated` | Telemetry | Dispatch (slice 5) | `driverId` | **Shipped** | This mirrors the proto package hierarchy (`crittercab..v`) minus the `crittercab.` prefix — Kafka topics are cluster-scoped, so the org prefix adds no disambiguation value and wastes characters in every log line. -Topics carry a descriptive name rather than a message-type name. A topic like `telemetry.location-pings` may carry `LocationPing` messages today and an enriched `LocationPingV2` tomorrow; the topic name describes the stream, not the current payload shape. +> **Corrected 2026-07-24 (first Kafka topic shipped).** This section previously proposed a *different* rule for Kafka — `.`, on the reasoning that a topic should describe the stream rather than the current payload shape, illustrated with speculative topics (`telemetry.location-pings`, `telemetry.demand-signals`, `pricing.surge-updates`) that were never published. ADR-019 weighed that argument on its merits and rejected it for Cab specifically: contract versioning lives in the proto package path (ADR-009), so payload evolution is handled a layer below the topic name, and a break large enough to make the event name misleading needs a new proto package and a deliberate consumer migration anyway. Do not reintroduce the descriptive-name rule; the topics above are the shipped reality. ## Publishing @@ -107,11 +107,23 @@ Topics carry a descriptive name rather than a message-type name. A topic like `t Map a message type to a specific topic via `PublishMessage().ToKafkaTopic("...")`. Cab's standard pattern uses one routing rule per Cab message type per service: ```csharp -opts.PublishMessage() - .ToKafkaTopic("telemetry.location-pings"); +opts.PublishMessage() + .ToKafkaTopic("telemetry.driver-location-updated"); -opts.PublishMessage() - .ToKafkaTopic("telemetry.demand-signals"); +opts.PublishMessage() + .ToKafkaTopic("telemetry.demand-signalled"); +``` + +The shipped Telemetry rule carries three more calls, each load-bearing — see § Serialization for `UseProtobufSerialization` and § Common pitfalls for why `SendInline` alone is not enough: + +```csharp +opts.Durability.UseSyncRetryBlock = true; // process-global + +opts.PublishMessage() + .ToKafkaTopic("telemetry.driver-location-updated") + .SendInline() // await the broker ack, don't batch + .UseIdempotentProducer() // enable.idempotence=true, acks=all + .UseProtobufSerialization(); ``` For the broader routing surface (`Specification` for partition count + replication factor, named brokers for multi-region), see ai-skills `wolverine-integrations-kafka` § Topic binding. @@ -125,8 +137,7 @@ Wolverine supports `opts.PublishAllMessages().ToKafkaTopics()` for derive-topic- Set a partition key when publishing to control which partition receives the message. Messages with the same partition key land in the same partition and are consumed in order: ```csharp -await bus.PublishAsync(new LocationPing(driverId, lat, lng, timestamp), - new DeliveryOptions { PartitionKey = driverId.ToString() }); +await bus.PublishAsync(update, new DeliveryOptions { PartitionKey = update.DriverId }); ``` If no partition key is set, Wolverine uses the envelope's message ID (a GUID), which distributes messages randomly across partitions. For GPS pings, partitioning by `driver_id` ensures a single driver's location stream stays ordered through the Telemetry -> Dispatch path — critical for computing heading, speed, and ETA. @@ -205,28 +216,51 @@ Wolverine stamps the consumer group ID onto `Envelope.GroupId` for every receive ## Serialization and interop +### Choosing a serializer + +Cab uses **two** serializers on Kafka, chosen by whether the payload is a generated protobuf contract: + +| Payload | Serializer | How | +|---|---|---| +| A protoc-generated type from `protos/` | Binary protobuf | `.UseProtobufSerialization()` on the publishing rule | +| A hand-authored C# record | Wolverine's default envelope JSON | nothing to configure | + +**Endpoint-scoped, always.** `WolverineFx.Protobuf` ships two overloads: one on `WolverineOptions` that replaces the app's `DefaultSerializer` globally, and one on an endpoint configuration. Use the endpoint one — the global overload would take the service's HTTP surface off JSON with it. + +```csharp +opts.PublishMessage() + .ToKafkaTopic("telemetry.driver-location-updated") + .UseProtobufSerialization(); +``` + +The Kafka wire payload is `Message`, so binary protobuf is natively expressible — no base64 wrapping. **A consumer of a protobuf topic must carry the same serializer on its listener endpoint**: `ProtobufMessageSerializer.ReadFromData(byte[])` throws `NotSupportedException`, and only the `(Type, Envelope)` overload works, so the type information has to come from the endpoint configuration. + +> **Corrected 2026-07-24 (first Kafka topic shipped).** This section previously read "Cab uses Wolverine's default JSON serialization" and deferred protobuf to "a future phase," cross-referencing `protobuf-contracts`' forward-looking note. That phase arrived: `DriverLocationUpdated` is generated from the `.proto` that ADR-009 makes the contract of record, and JSON-serializing a protoc-generated class produces bloated, non-canonical output while leaving the contract governing the C# type but not the wire. + ### Default envelope serialization -Cab uses Wolverine's default envelope serialization for service-to-service Kafka communication: message body in the Kafka value, envelope metadata (message ID, correlation ID, content type, message type name) in UTF-8 headers. Both sides must be Wolverine services. +For non-protobuf payloads, Wolverine's default envelope serialization carries the message body in the Kafka value and envelope metadata (message ID, correlation ID, content type, message type name) in UTF-8 headers. Both sides must be Wolverine services. ### Raw JSON interop For interop with non-Wolverine producers/consumers (third-party GPS devices, analytics pipelines), use raw JSON mode. Listener must declare the expected message type at config time: ```csharp -opts.PublishMessage().ToKafkaTopic("telemetry.location-pings").PublishRawJson(); -opts.ListenToKafkaTopic("telemetry.location-pings").ReceiveRawJson(); +opts.PublishMessage().ToKafkaTopic("pricing.demand-signalled").PublishRawJson(); +opts.ListenToKafkaTopic("pricing.demand-signalled").ReceiveRawJson(); ``` Raw JSON strips Wolverine envelope headers — see ai-skills `wolverine-integrations-kafka` § Raw JSON interoperability for the full publisher/listener semantics. +**⚠ `PublishRawJson()` silently destroys `DeliveryOptions.PartitionKey`.** Raw-JSON mode installs `JsonOnlyMapper`, whose `MapEnvelopeToOutgoing` assigns `outgoing.Key = envelope.GroupId` — and it runs *after* the transport has already set the key from `PartitionKey`. Combining the two produces records whose key is the consumer group id (or null), which does not fail, does not log, and only shows up as lost per-partition ordering. Never combine `PublishRawJson()` with a partition key; if a raw-JSON topic needs keyed ordering, set `GroupId` instead and document why. + ### Custom envelope mapper For wire formats that don't fit the default mapping or raw JSON (CloudEvents, Avro with a schema registry, custom wrappers), implement `IKafkaEnvelopeMapper` and register via `.UseInterop(...)` on the listener. Escape hatch — Cab does not currently use it. ### Schema Registry serializers -Wolverine ships `SchemaRegistryAvroSerializer` and `SchemaRegistryJsonSerializer` in the Kafka transport package for Confluent Schema Registry integration. These are outside Cab's current scope — Cab uses Wolverine's default JSON serialization. The `protobuf-contracts` skill's forward-looking note on protobuf-as-unified-schema-language may revisit serialization in a future phase; until then, default JSON is the right choice. +Wolverine ships `SchemaRegistryAvroSerializer` and `SchemaRegistryJsonSerializer` in the Kafka transport package for Confluent Schema Registry integration. These remain outside Cab's scope: Cab's schema authority is the `protos/` tree under ADR-009, not a registry, and `.UseProtobufSerialization()` (above) already gives binary protobuf on the wire without one. Revisit only if a non-Cab producer needs registry-mediated compatibility checks. ## Dead letter topics and error handling @@ -299,7 +333,9 @@ Wolverine's Kafka transport propagates OpenTelemetry trace context through Kafka - **Calling AutoProvision against the Event Hubs Emulator.** The EH Emulator does not support Kafka admin APIs. `AutoProvision()` will throw. Use Aspire's `AddKafka` for local dev (which starts a real Kafka container) and provision topics through the management plane for EH Emulator environments. -- **Forgetting a partition key on ordered streams.** Without a partition key, Wolverine uses the envelope's GUID, scattering messages randomly across partitions. GPS pings without `PartitionKey = driverId` lose their per-driver ordering guarantee. Always set a partition key for streams where ordering matters. +- **Forgetting a partition key on ordered streams.** Without a partition key, Wolverine uses the envelope's GUID, scattering messages randomly across partitions. GPS pings without `PartitionKey = driverId` lose their per-driver ordering guarantee. Always set a partition key for streams where ordering matters — and assert it in an integration test, because the failure is invisible: the records still arrive, only the ordering breaks. + +- **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. diff --git a/docs/workshops/006-telemetry-event-model.md b/docs/workshops/006-telemetry-event-model.md index 8caf1ef..2105342 100644 --- a/docs/workshops/006-telemetry-event-model.md +++ b/docs/workshops/006-telemetry-event-model.md @@ -650,3 +650,14 @@ Resume the paused W006 design (grill R1–R8, 2026-06-25) after signing off ADR- **§11 ADR candidates:** none fired. The *windowed gRPC client-streaming ingest* candidate lands as a **skill**, not an ADR, as §6.2 and the handoff both leaned — the auto-generated shape plus its middleware caveat is library mechanics, not an architectural choice CritterCab made. It is folded into `wolverine-grpc-handlers` in the same PR. The other two (Kafka topic-naming, stream-processing-as-4th-shape) remain later-arc. Slices 3 and 5 remain pending. Session: [`prompts/implementations/007-telemetry-slices-4-and-2-transport.md`](../prompts/implementations/007-telemetry-slices-4-and-2-transport.md). +- **2026-07-24 (second entry, PR C)** — §6.3 (Slice 3) **realized in code**, and §11 candidate #1 **fired as [ADR-019](../decisions/019-transport-agnostic-topic-naming.md)**. + + `DriverLocationUpdated` is published to `telemetry.driver-location-updated`, partitioned by `driverId`, dedup-keyed on `serverReceivedAt`, with no outbox — **CritterCab's first Kafka topic and second live transport.** The `IDriverLocationPublisher` seam PR B introduced did its job exactly as intended: it already carried the generated `DriverLocationUpdated`, so the session replaced one implementation class and the host wiring, and `ReportLocationsHandler` does not appear in the PR diff at all. + + **§6.3's publish-first argument needed a qualifier this workshop did not know to name — this is an amendment, not a correction.** §6.3 reasons from failure-mode asymmetry: a failed *store* after a good publish costs a duplicate the consumer's dedup absorbs, while a failed *publish* after a good store costs a miss Dispatch cannot detect. That reasoning is sound, but it silently assumes the publish's outcome is *known* by the time the store runs. It is not, by default: a Wolverine Kafka publishing endpoint is `BufferedInMemory`, so `PublishAsync` returns once the record is queued in-process, before the broker has acked — and even switching to `SendInline()` is insufficient, because Wolverine's default async retry block swallows a send failure, logs it, re-posts to a background block and returns success. Under the defaults, publish-first would have been true only in statement order, every broker failure would have landed in the branch §6.3 rejected, and nothing would have looked wrong. The session therefore ships `SendInline()` + `opts.Durability.UseSyncRetryBlock = true` + `UseIdempotentProducer()`, which makes a rejection throw, skip the upsert, leave the baseline stale, and let the driver's next ping republish. **Recorded as a property of the slice: §6.3's ordering guarantee is a configuration commitment, not a free consequence of writing the two calls in order.** `UseSyncRetryBlock` is process-global; affordable because Telemetry publishes to one transport, and a second publisher should re-weigh it. + + **Wire format is binary protobuf, endpoint-scoped** — `DriverLocationUpdated` is generated from the `.proto` that ADR-009 makes the contract of record, so JSON would have left the contract governing the C# type but not the wire. This overrode a written convention: `wolverine-kafka/SKILL.md` said Cab uses default JSON and deferred protobuf to "a future phase." Escalated to user sign-off rather than absorbed, and the skill was corrected in the same PR. + + **§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). From e27da33868921af80f498dba804a3c23de3419ce Mon Sep 17 00:00:00 2001 From: Erik Shafer Date: Fri, 24 Jul 2026 20:46:10 -0500 Subject: [PATCH 4/5] docs: fix skill drift and stale csproj comment found by the Phase 2 audit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The topic-naming correction left section 'Convention-based routing' two sections below still justifying itself with the rejected descriptive-name rule, inside a file whose new banner claimed that section was reconciled. Rewritten to the real reason Cab declines convention-based routing: it would bind a wire-visible topic to a C# type name, so a refactor rename would repoint the producer silently. Fourth DEBT row from the same audit — service-bootstrap does not sanction the optional connection-string guard Telemetry has now used twice. --- .../008-telemetry-slice-3-kafka-publish.md | 5 +++-- docs/skills/DEBT.md | 8 +++++++- docs/skills/wolverine-kafka/SKILL.md | 2 +- src/CritterCab.Telemetry/CritterCab.Telemetry.csproj | 8 +++++--- 4 files changed, 16 insertions(+), 7 deletions(-) diff --git a/docs/retrospectives/implementations/008-telemetry-slice-3-kafka-publish.md b/docs/retrospectives/implementations/008-telemetry-slice-3-kafka-publish.md index c9ca495..4172da6 100644 --- a/docs/retrospectives/implementations/008-telemetry-slice-3-kafka-publish.md +++ b/docs/retrospectives/implementations/008-telemetry-slice-3-kafka-publish.md @@ -18,7 +18,7 @@ - `tests/CritterCab.Telemetry.Tests/TelemetryTestFixture.cs` — `RecordingDriverLocationPublisher` gains `FailNextPublish` - **`docs/decisions/019-transport-agnostic-topic-naming.md`** — new ADR; `docs/decisions/014-*.md` status line + scope note; `docs/decisions/README.md` index - `docs/skills/wolverine-kafka/SKILL.md` — topic-naming and serialization sections corrected, two pitfalls added, publish-path examples reconciled against shipped code - - `docs/skills/DEBT.md` — 3 new rows + - `docs/skills/DEBT.md` — 4 new rows - `docs/workshops/006-telemetry-event-model.md` `## Document History` — §6.3 realized; §11 candidate #1 discharged; the publish-first qualifier recorded - `docs/prompts/README.md` — Implementations index entry - This retro @@ -101,13 +101,14 @@ Both corrections rode in-PR under the session-runner-blocking exception, on the - **Source verification and convention discovery are not substitutes.** This prompt was thoroughly gate-verified and still had four convention-level errors the Phase 1 auditor caught, including a missing governing skill. Both passes, in that order. - **A skill that says "not yet" is a decision, not a silence.** When a session's plan overrides one, that belongs in front of the user. When a skill is merely absent on a point, the session runner decides. This session had one of each — protobuf serialization (escalated) and endpoint-scoped-vs-global (decided) — and the line held usefully. - **When correcting a skill in-session, scope the correction to what the session actually shipped.** The publish path was corrected against real code; the listener half was left with a warning and a DEBT row, because correcting it now would mean inventing the consumer PR D is going to build. +- **Correcting one section of a skill leaves the sections that quoted it stale.** The Phase 2 audit found that two sections below the rewritten topic-naming rule, § Convention-based routing still justified itself with *"Cab's `.` topic convention doesn't match type names"* — reintroducing by name the exact rule the correction had just rejected, inside a file whose new banner claimed that section was reconciled. Fixed in-session. The general lesson: after editing a skill's normative section, grep the rest of the file for its old vocabulary rather than trusting section boundaries, because supporting prose tends to restate the rule it depends on. --- ## Outstanding items / next-session inputs - **PR D — W006 slice 5**, the last pending slice: Dispatch consumes `telemetry.driver-location-updated` into the `AvailableDriver` view, replacing `NearbyAvailableDriversStub`, and closes the W001 §5.3 amendment. It also owns §6.3's **Dedup GWT**, which asserts consumer behavior and could not be tested here. Two constraints already verified for it: `ListenToKafkaTopic(...).ConfigureConsumer(c => c.GroupId = ...)` is the listener shape (there is no `.GroupId(string)`), and the listener **must** carry `UseProtobufSerialization()` because `ProtobufMessageSerializer.ReadFromData(byte[])` throws — only the `(Type, Envelope)` overload works. -- **Three new DEBT rows**: `wolverine-kafka` listener examples (drain with PR D), `transport-selection`'s missing built-vs-modeled status axis (two of three transports are now built), and the `aspire` `AddKafka` example. +- **Four new DEBT rows**: `wolverine-kafka` listener examples (drain with PR D), `transport-selection`'s missing built-vs-modeled status axis (two of three transports are now built), the `aspire` `AddKafka` example, and `service-bootstrap`'s undocumented optional connection-string guard — the last surfaced by the Phase 2 audit, which noticed that Telemetry's `Program.cs` has now twice chosen an optional guard over the skill's canonical mandatory-throw without anything sanctioning the deviation. - **Design-return cadence.** ADR-019 served as this run's interleave, so the counter is satisfied — but ASB is now the only modeled-and-unbuilt transport, and the Driver Profile workshop is the prerequisite for the ASB half of ADR-018's join. That is the natural design-side successor once slice 5 lands. - **CLAUDE.md's status line is further out of date than it was**, and PR [#43](https://github.com/erikshafer/CritterCab/pull/43) — already stale on arrival for describing a transport-less Telemetry — is now stale in one more respect. Re-read its diff before merging; accurate now is *two services, two live transports, W006 slices 1/2/3/4 realized.* - **The CI-cannot-see-`apphost.cs` gap bit again** (this session edited the file and caught a compile error only locally). Still its own session; extend the existing "Verify solution completeness" step rather than adding a guard. diff --git a/docs/skills/DEBT.md b/docs/skills/DEBT.md index 77062e6..476d5a3 100644 --- a/docs/skills/DEBT.md +++ b/docs/skills/DEBT.md @@ -84,6 +84,12 @@ This file is the working ledger between retros that surface gaps and the tidy se - **Not fixed in-session by design:** adding a status axis is a structural change to a skill this session was otherwise only consuming, and prompt 008 named the contingency in advance (check first; register a row rather than invent a structure mid-session). A tidy session should settle the axis shape once, ideally alongside whichever session builds ASB and makes all three rows answerable. - **Retro source:** [`retrospectives/implementations/008-telemetry-slice-3-kafka-publish.md`](../retrospectives/implementations/008-telemetry-slice-3-kafka-publish.md). +### `service-bootstrap` — the optional connection-string guard is an undocumented sanctioned deviation + +- **Gap:** The skill's canonical `Program.cs` examples all read a connection string with a mandatory `?? throw new InvalidOperationException(...)`. Telemetry's shipped `Program.cs` instead guards *optionally* — `if (!string.IsNullOrEmpty(connectionString))` — for both Marten (since PR #42) and now Kafka, so the service boots without a database or a broker and degrades to the logging publisher. That is deliberate and useful (a broker-less `dotnet run` stays worth doing, and the non-Kafka test suites need it), but nothing in the skill sanctions it, so each session re-derives the choice from local precedent rather than from a rule. +- **What a fix should say:** name both shapes and the condition that selects one. Mandatory-throw when the dependency is load-bearing for every code path; optional-guard when there is a meaningful degraded mode the service is expected to run in. Reference impl for the optional shape: `src/CritterCab.Telemetry/Program.cs`. +- **Retro source:** [`retrospectives/implementations/008-telemetry-slice-3-kafka-publish.md`](../retrospectives/implementations/008-telemetry-slice-3-kafka-publish.md) (surfaced by the Phase 2 audit). + ### `aspire` — the `AddKafka` port-allocation example does not compile on 13.4.6 - **Gap:** § Port allocation's worked example shows `builder.AddKafka("kafka").WithHostPort(5392)`. On Aspire 13.4.6 `WithHostPort` is an extension on `IResourceBuilder` — the Kafka **UI** container, not the broker — so the line fails with CS1929. The broker takes its port as a constructor argument: `builder.AddKafka("kafka", port: 5392)`. PR C hit this on first build. The reserved port **5392 itself was correct** and is now claimed. @@ -126,5 +132,5 @@ Older entries drop off; the retros and commits remain authoritative. - **2026-05-08.** Initial authoring. Seven rows from the post-D→B→C session — five `marten-*` Marten 8.x / JasperFx namespace extractions plus two `service-bootstrap` registration prerequisites. Three other gaps from the same session (`RunOaktonCommandsAsync` → `RunJasperFxCommands`, `protobuf-contracts` directory layout, `service-bootstrap`/`aspire` connection-string contradiction) were fixed in-flight under the session-runner-blocking exception and do not appear here. - **2026-05-08 (later same day).** Initial 7-row backlog drained via the first skill-tidy session. `Open debt` reset to empty. Retro at [`docs/retrospectives/skills-tidy-marten-and-bootstrap.md`](../retrospectives/skills-tidy-marten-and-bootstrap.md). - **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 three 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, and the `aspire` skill's non-compiling `AddKafka` port example. 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-24 (PR C).** Registered four 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) `service-bootstrap`'s undocumented optional connection-string guard. 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). diff --git a/docs/skills/wolverine-kafka/SKILL.md b/docs/skills/wolverine-kafka/SKILL.md index b9486f8..bb04d41 100644 --- a/docs/skills/wolverine-kafka/SKILL.md +++ b/docs/skills/wolverine-kafka/SKILL.md @@ -130,7 +130,7 @@ For the broader routing surface (`Specification` for partition count + replicati ### Convention-based routing -Wolverine supports `opts.PublishAllMessages().ToKafkaTopics()` for derive-topic-name-from-message-type publishing. **Cab does not use this** — Cab's `.` topic convention doesn't match type names. Named topic routing (above) is the Cab default. +Wolverine supports `opts.PublishAllMessages().ToKafkaTopics()` for derive-topic-name-from-message-type publishing. **Cab does not use this.** ADR-019's `.` is close enough to a type-name derivation to make the shortcut tempting, and that is exactly why it is worth declining: the derivation would bind a wire-visible topic name to a C# type name, so a routine refactor rename would silently repoint the producer at a new topic while consumers stayed on the old one. The BC prefix also has no type to derive from. Named topic routing (above) keeps the topic an explicit, reviewable string — which is what a cross-BC contract should be. ### Partition keys diff --git a/src/CritterCab.Telemetry/CritterCab.Telemetry.csproj b/src/CritterCab.Telemetry/CritterCab.Telemetry.csproj index d642c0f..02951d3 100644 --- a/src/CritterCab.Telemetry/CritterCab.Telemetry.csproj +++ b/src/CritterCab.Telemetry/CritterCab.Telemetry.csproj @@ -45,9 +45,11 @@ GrpcServices="Both" /> + event, generated in PR B so the IDriverLocationPublisher seam carried the real + published-language payload from the start — which is what let PR C swap only the + publisher implementation, never the seam's contract. The generated type is now both the + seam's payload and, via WolverineFx.Protobuf, the binary wire format on + telemetry.driver-location-updated. --> Date: Fri, 24 Jul 2026 21:07:40 -0500 Subject: [PATCH 5/5] refactor: address two-axis code review findings Standards axis, both live rather than theoretical: - Testcontainers were unnamed. Latent while the project had one container; slice 3 added a second Postgres and xUnit runs the two collections in parallel, so a fixed name now collides. Both fixtures get unique names. Following the skill's own fix required correcting it twice over -- WithPullPolicy is really WithImagePullPolicy, and PullPolicy lives in DotNet.Testcontainers.Images. - A guard clause inside the UseWolverine lambda would have silently swallowed any configuration appended below it on the broker-less path, and the connection string was branched on twice in opposite polarity. Read once into kafkaEnabled, extracted ConfigureKafkaPublishing. - StreamAsync/PingAt had been copied into three test classes. Extracted ReportLocationsClient; updated the pre-existing slice-2 copy too, since leaving one caller off a helper this session introduced is worse than the duplication it replaces. Spec axis: closed deliverable 7 (the proto comment now records shipped state), and dropped two speculative topic names the skill correction had minted in the same file whose new banner warns against speculative names. Two DEBT rows added: testing-integration's wrong API names and its collection convention that no shipped fixture follows, and the CI-cannot-build-apphost gap. 33/33 green. --- .../008-telemetry-slice-3-kafka-publish.md | 11 +++- docs/skills/DEBT.md | 15 ++++- docs/skills/wolverine-kafka/SKILL.md | 7 +- .../v1/driver_location_updated.proto | 4 ++ src/CritterCab.Telemetry/Program.cs | 25 +++++-- .../ReportLocations/ReportLocationsClient.cs | 66 +++++++++++++++++++ .../Slice2ReportLocationsTests.cs | 44 ++----------- .../Slice3KafkaPublishTests.cs | 28 ++------ .../Slice3PublishOrderingTests.cs | 29 ++------ .../TelemetryKafkaTestFixture.cs | 8 +++ .../TelemetryTestFixture.cs | 5 ++ 11 files changed, 141 insertions(+), 101 deletions(-) create mode 100644 tests/CritterCab.Telemetry.Tests/ReportLocations/ReportLocationsClient.cs diff --git a/docs/retrospectives/implementations/008-telemetry-slice-3-kafka-publish.md b/docs/retrospectives/implementations/008-telemetry-slice-3-kafka-publish.md index 4172da6..0ae2eae 100644 --- a/docs/retrospectives/implementations/008-telemetry-slice-3-kafka-publish.md +++ b/docs/retrospectives/implementations/008-telemetry-slice-3-kafka-publish.md @@ -18,7 +18,8 @@ - `tests/CritterCab.Telemetry.Tests/TelemetryTestFixture.cs` — `RecordingDriverLocationPublisher` gains `FailNextPublish` - **`docs/decisions/019-transport-agnostic-topic-naming.md`** — new ADR; `docs/decisions/014-*.md` status line + scope note; `docs/decisions/README.md` index - `docs/skills/wolverine-kafka/SKILL.md` — topic-naming and serialization sections corrected, two pitfalls added, publish-path examples reconciled against shipped code - - `docs/skills/DEBT.md` — 4 new rows + - `tests/CritterCab.Telemetry.Tests/ReportLocations/ReportLocationsClient.cs` — shared gRPC client-stream helper, extracted post-review + - `docs/skills/DEBT.md` — 6 new rows - `docs/workshops/006-telemetry-event-model.md` `## Document History` — §6.3 realized; §11 candidate #1 discharged; the publish-first qualifier recorded - `docs/prompts/README.md` — Implementations index entry - This retro @@ -60,6 +61,10 @@ Everything else that made the session interesting came from two sources: a sourc **The Aspire example in our own skill does not compile.** `builder.AddKafka("kafka").WithHostPort(5392)` — copied from `aspire/SKILL.md` § Port allocation — fails with CS1929 on Aspire 13.4.6, because `WithHostPort` belongs to the Kafka *UI* container resource, not the broker. The port takes a constructor argument instead. This is a small fix but a pointed one: it went unnoticed because `apphost.cs` is a file-based app with no `.csproj`, so CI's "Verify solution completeness" step does not reach it, and nothing mechanically checks either the apphost or the examples written against it. Registered as a DEBT row; the CI gap remains its own session. +**The session created a three-way test duplication and did not notice.** `StreamAsync` / `PingAt` were copied into both new test classes from `Slice2ReportLocationsTests`, differing only by a `using` alias. The standards axis flagged it; extracting `ReportLocationsClient` collapsed all three, and updating the pre-existing slice-2 copy was in-bounds because leaving one caller off a helper this session introduced would have been worse than the duplication. Notable because the copies were each written to *match the existing style* — the local instinct that produced them was the right one, and it still produced a smell. + +**A guard clause inside a configuration lambda is a trap, not a style choice.** The first cut used `if (string.IsNullOrEmpty(kafkaConnectionString)) return;` inside `UseWolverine(opts => ...)`. It works today and silently swallows any Wolverine configuration a later session appends below it whenever no broker is configured. Restructured to read the flag once into `kafkaEnabled` and call a `ConfigureKafkaPublishing(opts)` local function under a positive guard — which also fixed a second finding, that the connection string was being branched on twice in opposite polarity forty lines apart. + **Choosing a Testcontainers Kafka image is not a free choice.** `confluentinc/confluent-local:7.6.1` — the KRaft image Wolverine's own docker-compose uses — produced a container that exited 1 with `No 'meta.properties' found`. Testcontainers' `KafkaBuilder` injects a startup script built around `cp-kafka`'s entrypoint, and confluent-local expects its log directory formatted by `kafka-storage.sh` first; the combination yields a broker that reads `zookeeper.properties`, finds nothing, and dies. `confluentinc/cp-kafka:7.6.1` works. Pinned with a comment, because the failure mode reads as a Kafka problem rather than an image-compatibility problem. --- @@ -101,6 +106,8 @@ Both corrections rode in-PR under the session-runner-blocking exception, on the - **Source verification and convention discovery are not substitutes.** This prompt was thoroughly gate-verified and still had four convention-level errors the Phase 1 auditor caught, including a missing governing skill. Both passes, in that order. - **A skill that says "not yet" is a decision, not a silence.** When a session's plan overrides one, that belongs in front of the user. When a skill is merely absent on a point, the session runner decides. This session had one of each — protobuf serialization (escalated) and endpoint-scoped-vs-global (decided) — and the line held usefully. - **When correcting a skill in-session, scope the correction to what the session actually shipped.** The publish path was corrected against real code; the listener half was left with a warning and a DEBT row, because correcting it now would mean inventing the consumer PR D is going to build. +- **Four review passes found four disjoint classes of problem, and none was redundant.** Worth recording, because the instinct after a clean source-verification pass is that further review is ceremony. It was not: the `jasperfx-source-verifier` pass found a spec-vs-library gap; `critter-skill-auditor` Phase 1 found a missing governing skill and two skill-vs-spec contradictions; Phase 2 found a contradiction the session's *own* skill correction had left behind; and the two-axis code review found a live Testcontainer-naming violation, a `return`-inside-a-config-lambda trap, and a three-way test duplication the session had just created. Each pass caught things the others structurally could not. +- **The standards axis is worth running even when the repo's own skill is wrong.** Two of its findings were violations of `testing-integration` rules that turned out to name **non-existent APIs** (`WithPullPolicy`, and `PullPolicy` in the wrong namespace). The rule was right, the code was wrong, and the skill's spelling of the fix was also wrong — so following the finding still required verifying against the package. A review finding is a pointer, not a patch. - **Correcting one section of a skill leaves the sections that quoted it stale.** The Phase 2 audit found that two sections below the rewritten topic-naming rule, § Convention-based routing still justified itself with *"Cab's `.` topic convention doesn't match type names"* — reintroducing by name the exact rule the correction had just rejected, inside a file whose new banner claimed that section was reconciled. Fixed in-session. The general lesson: after editing a skill's normative section, grep the rest of the file for its old vocabulary rather than trusting section boundaries, because supporting prose tends to restate the rule it depends on. --- @@ -108,7 +115,7 @@ Both corrections rode in-PR under the session-runner-blocking exception, on the ## Outstanding items / next-session inputs - **PR D — W006 slice 5**, the last pending slice: Dispatch consumes `telemetry.driver-location-updated` into the `AvailableDriver` view, replacing `NearbyAvailableDriversStub`, and closes the W001 §5.3 amendment. It also owns §6.3's **Dedup GWT**, which asserts consumer behavior and could not be tested here. Two constraints already verified for it: `ListenToKafkaTopic(...).ConfigureConsumer(c => c.GroupId = ...)` is the listener shape (there is no `.GroupId(string)`), and the listener **must** carry `UseProtobufSerialization()` because `ProtobufMessageSerializer.ReadFromData(byte[])` throws — only the `(Type, Envelope)` overload works. -- **Four new DEBT rows**: `wolverine-kafka` listener examples (drain with PR D), `transport-selection`'s missing built-vs-modeled status axis (two of three transports are now built), the `aspire` `AddKafka` example, and `service-bootstrap`'s undocumented optional connection-string guard — the last surfaced by the Phase 2 audit, which noticed that Telemetry's `Program.cs` has now twice chosen an optional guard over the skill's canonical mandatory-throw without anything sanctioning the deviation. +- **Six new DEBT rows**: `wolverine-kafka` listener examples (drain with PR D), `transport-selection`'s missing built-vs-modeled status axis, the `aspire` `AddKafka` example, `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. **The load-bearing one is `testing-integration` Gap B**: `[CollectionDefinition(Name, DisableParallelization = true)]` is the documented Cab default and *no* shipped collection follows it — a decision to make once and apply everywhere, not slice-locally. - **Design-return cadence.** ADR-019 served as this run's interleave, so the counter is satisfied — but ASB is now the only modeled-and-unbuilt transport, and the Driver Profile workshop is the prerequisite for the ASB half of ADR-018's join. That is the natural design-side successor once slice 5 lands. - **CLAUDE.md's status line is further out of date than it was**, and PR [#43](https://github.com/erikshafer/CritterCab/pull/43) — already stale on arrival for describing a transport-less Telemetry — is now stale in one more respect. Re-read its diff before merging; accurate now is *two services, two live transports, W006 slices 1/2/3/4 realized.* - **The CI-cannot-see-`apphost.cs` gap bit again** (this session edited the file and caught a compile error only locally). Still its own session; extend the existing "Verify solution completeness" step rather than adding a guard. diff --git a/docs/skills/DEBT.md b/docs/skills/DEBT.md index 476d5a3..4f42ef0 100644 --- a/docs/skills/DEBT.md +++ b/docs/skills/DEBT.md @@ -84,6 +84,19 @@ This file is the working ledger between retros that surface gaps and the tidy se - **Not fixed in-session by design:** adding a status axis is a structural change to a skill this session was otherwise only consuming, and prompt 008 named the contingency in advance (check first; register a row rather than invent a structure mid-session). A tidy session should settle the axis shape once, ideally alongside whichever session builds ASB and makes all three rows answerable. - **Retro source:** [`retrospectives/implementations/008-telemetry-slice-3-kafka-publish.md`](../retrospectives/implementations/008-telemetry-slice-3-kafka-publish.md). +### `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). +- **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). + +### CI cannot build `apphost.cs` — the "Verify solution completeness" step does not reach it + +- **Gap:** `apphost.cs` is a file-based app with no `.csproj`, so CI's existing "Verify solution completeness" step falls through it and nothing builds the AppHost. It broke for two weeks unnoticed before PR #45 fixed it, and PR C hit a second compile error in it (the `AddKafka` port API) that only a local `dotnet build apphost.cs` caught. Any session editing the AppHost is currently its own CI. +- **Fix shape:** **extend the existing step, do not add a new guard.** Not a skill row in the usual sense — recorded here because this is where the repo tracks known gaps between sessions, and because two consecutive sessions have now paid for it. CI changes carry their own blast radius and warrant a scoped session. +- **Retro source:** [`retrospectives/implementations/008-telemetry-slice-3-kafka-publish.md`](../retrospectives/implementations/008-telemetry-slice-3-kafka-publish.md); first flagged in the post-PR-45 handoff. + ### `service-bootstrap` — the optional connection-string guard is an undocumented sanctioned deviation - **Gap:** The skill's canonical `Program.cs` examples all read a connection string with a mandatory `?? throw new InvalidOperationException(...)`. Telemetry's shipped `Program.cs` instead guards *optionally* — `if (!string.IsNullOrEmpty(connectionString))` — for both Marten (since PR #42) and now Kafka, so the service boots without a database or a broker and degrades to the logging publisher. That is deliberate and useful (a broker-less `dotnet run` stays worth doing, and the non-Kafka test suites need it), but nothing in the skill sanctions it, so each session re-derives the choice from local precedent rather than from a rule. @@ -132,5 +145,5 @@ Older entries drop off; the retros and commits remain authoritative. - **2026-05-08.** Initial authoring. Seven rows from the post-D→B→C session — five `marten-*` Marten 8.x / JasperFx namespace extractions plus two `service-bootstrap` registration prerequisites. Three other gaps from the same session (`RunOaktonCommandsAsync` → `RunJasperFxCommands`, `protobuf-contracts` directory layout, `service-bootstrap`/`aspire` connection-string contradiction) were fixed in-flight under the session-runner-blocking exception and do not appear here. - **2026-05-08 (later same day).** Initial 7-row backlog drained via the first skill-tidy session. `Open debt` reset to empty. Retro at [`docs/retrospectives/skills-tidy-marten-and-bootstrap.md`](../retrospectives/skills-tidy-marten-and-bootstrap.md). - **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 four 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) `service-bootstrap`'s undocumented optional connection-string guard. 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-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). diff --git a/docs/skills/wolverine-kafka/SKILL.md b/docs/skills/wolverine-kafka/SKILL.md index bb04d41..a63dfa2 100644 --- a/docs/skills/wolverine-kafka/SKILL.md +++ b/docs/skills/wolverine-kafka/SKILL.md @@ -109,9 +109,6 @@ Map a message type to a specific topic via `PublishMessage().ToKafkaTopic(".. ```csharp opts.PublishMessage() .ToKafkaTopic("telemetry.driver-location-updated"); - -opts.PublishMessage() - .ToKafkaTopic("telemetry.demand-signalled"); ``` The shipped Telemetry rule carries three more calls, each load-bearing — see § Serialization for `UseProtobufSerialization` and § Common pitfalls for why `SendInline` alone is not enough: @@ -246,8 +243,8 @@ For non-protobuf payloads, Wolverine's default envelope serialization carries th For interop with non-Wolverine producers/consumers (third-party GPS devices, analytics pipelines), use raw JSON mode. Listener must declare the expected message type at config time: ```csharp -opts.PublishMessage().ToKafkaTopic("pricing.demand-signalled").PublishRawJson(); -opts.ListenToKafkaTopic("pricing.demand-signalled").ReceiveRawJson(); +opts.PublishMessage().ToKafkaTopic(".").PublishRawJson(); +opts.ListenToKafkaTopic(".").ReceiveRawJson(); ``` Raw JSON strips Wolverine envelope headers — see ai-skills `wolverine-integrations-kafka` § Raw JSON interoperability for the full publisher/listener semantics. diff --git a/protos/crittercab/telemetry/v1/driver_location_updated.proto b/protos/crittercab/telemetry/v1/driver_location_updated.proto index 3c0de65..02fa547 100644 --- a/protos/crittercab/telemetry/v1/driver_location_updated.proto +++ b/protos/crittercab/telemetry/v1/driver_location_updated.proto @@ -15,6 +15,10 @@ option csharp_namespace = "CritterCab.Telemetry.V1"; // | Dedup / LWW key: server_received_at — server-stamped, monotonic; consumer dedups // on (driver_id, server_received_at) against at-least-once redelivery (R7) // Slice reference: docs/workshops/006-telemetry-event-model.md §6.3. +// SHIPPED 2026-07-24. Telemetry publishes this to the topic above, serialized as binary +// protobuf on the wire (not JSON) — this file is the wire format, not only the C# type. +// Topic naming follows ADR-019; the Kafka delivery semantics that go with it (partition key, +// no outbox, inline send) are the Kafka column of that ADR's per-transport table. message DriverLocationUpdated { string driver_id = 1; double lat = 2; diff --git a/src/CritterCab.Telemetry/Program.cs b/src/CritterCab.Telemetry/Program.cs index 8fae09c..1e2d7eb 100644 --- a/src/CritterCab.Telemetry/Program.cs +++ b/src/CritterCab.Telemetry/Program.cs @@ -78,9 +78,13 @@ // The lifetimes differ on purpose. KafkaDriverLocationPublisher is scoped because it depends on // IMessageBus, which Wolverine registers scoped; the logging fallback holds only an ILogger and // stays a singleton. -var kafkaConnectionString = builder.Configuration.GetConnectionString("kafka"); +// One flag, read once, used by both the registration above and the transport wiring inside +// UseWolverine below. Branching on the connection string twice would let the two drift into the +// state that breaks silently: the real publisher registered against a transport that was never +// configured, which fails at the first publish rather than at startup. +var kafkaEnabled = !string.IsNullOrEmpty(builder.Configuration.GetConnectionString("kafka")); -if (!string.IsNullOrEmpty(kafkaConnectionString)) +if (kafkaEnabled) { builder.Services.AddScoped(); } @@ -104,11 +108,18 @@ // passes through as 200 instead of a 400 ProblemDetails. opts.UseFluentValidation(); - if (string.IsNullOrEmpty(kafkaConnectionString)) - return; + // Guarded rather than early-returned: an early `return` here would silently swallow any + // Wolverine configuration appended below it whenever no broker is configured. + if (kafkaEnabled) + ConfigureKafkaPublishing(opts); +}); - // === Kafka: the slice-3 publish (W006 §6.3) === - // +// === Kafka: the slice-3 publish (W006 §6.3) === +// +// A local function rather than an inline block, so the broker-less path is one guarded call at +// the call site instead of a branch buried in the middle of the Wolverine configuration. +static void ConfigureKafkaPublishing(WolverineOptions opts) +{ // Read the broker address by NAME rather than by value: 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. @@ -157,7 +168,7 @@ // contract governing the type but not the wire. Telemetry's HTTP surface stays JSON — // the global UseProtobufSerialization overload would have taken that with it. .UseProtobufSerialization(); -}); +} var app = builder.Build(); diff --git a/tests/CritterCab.Telemetry.Tests/ReportLocations/ReportLocationsClient.cs b/tests/CritterCab.Telemetry.Tests/ReportLocations/ReportLocationsClient.cs new file mode 100644 index 0000000..dc8090e --- /dev/null +++ b/tests/CritterCab.Telemetry.Tests/ReportLocations/ReportLocationsClient.cs @@ -0,0 +1,66 @@ +using CritterCab.Telemetry.ReportLocations; +using CritterCab.Telemetry.V1; +using Google.Protobuf.WellKnownTypes; +using Grpc.Core; +using Grpc.Net.Client; + +namespace CritterCab.Telemetry.Tests.ReportLocations; + +// Drives a real gRPC client stream against a test host. Extracted once slice 3 added a second +// fixture and a third test class — the open-stream / write-pings / half-close / await-ack dance +// is identical everywhere, and the only thing that ever varies is which host's channel to use. +// +// Deliberately a static helper over a channel factory rather than a base class: the two fixtures +// have nothing else in common (one runs a broker, the other swaps the publisher for a recorder), +// so inheriting from a shared test base would couple them for the sake of four lines. +internal static class ReportLocationsClient +{ + public static async Task StreamAsync( + Func channelFactory, + Guid driverId, + params LocationPing[] pings) + { + using var channel = channelFactory(); + var client = new TelemetryService.TelemetryServiceClient(channel); + + // gRPC call metadata travels as HTTP/2 headers, which is how the dev principal accessor + // sees it. The real Entra claim replaces this without the handler changing. + using var call = client.ReportLocations(new Metadata + { + { HeaderDriverPrincipalAccessor.DriverIdHeader, driverId.ToString() } + }); + + foreach (var ping in pings) + await call.RequestStream.WriteAsync(ping); + + // Half-close: this is what makes the single ack come back. + await call.RequestStream.CompleteAsync(); + + return await call.ResponseAsync; + } + + // Opens a stream carrying no driver identity at all, for the R5 rejection case. + public static async Task StreamWithoutIdentityAsync( + Func channelFactory, + params LocationPing[] pings) + { + using var channel = channelFactory(); + var client = new TelemetryService.TelemetryServiceClient(channel); + using var call = client.ReportLocations(); + + foreach (var ping in pings) + await call.RequestStream.WriteAsync(ping); + + await call.RequestStream.CompleteAsync(); + + return await call.ResponseAsync; + } + + public static LocationPing PingAt(double lat, double lon, double accuracyMeters = 8d) => new() + { + Lat = lat, + Lon = lon, + AccuracyMeters = accuracyMeters, + DeviceTimestamp = Timestamp.FromDateTimeOffset(DateTimeOffset.UtcNow) + }; +} diff --git a/tests/CritterCab.Telemetry.Tests/ReportLocations/Slice2ReportLocationsTests.cs b/tests/CritterCab.Telemetry.Tests/ReportLocations/Slice2ReportLocationsTests.cs index 3ec903c..4572d30 100644 --- a/tests/CritterCab.Telemetry.Tests/ReportLocations/Slice2ReportLocationsTests.cs +++ b/tests/CritterCab.Telemetry.Tests/ReportLocations/Slice2ReportLocationsTests.cs @@ -133,16 +133,9 @@ public async Task a_stream_with_no_driver_identity_is_rejected() // R5: driverId comes from the principal and is never carried in the payload — which is why // the proto has no driver_id field. A stream that presents no identity cannot be attributed. - using var channel = _fixture.CreateGrpcChannel(); - var client = new TelemetryService.TelemetryServiceClient(channel); - using var call = client.ReportLocations(); - - var exception = await Should.ThrowAsync(async () => - { - await call.RequestStream.WriteAsync(PingAt(LoopLat, LoopLon)); - await call.RequestStream.CompleteAsync(); - await call.ResponseAsync; - }); + var exception = await Should.ThrowAsync(() => + ReportLocationsClient.StreamWithoutIdentityAsync( + _fixture.CreateGrpcChannel, PingAt(LoopLat, LoopLon))); exception.StatusCode.ShouldBe(StatusCode.Unauthenticated); _fixture.Publisher.Published.ShouldBeEmpty(); @@ -182,26 +175,8 @@ private async Task ResetAsync() _fixture.Publisher.Clear(); } - private async Task StreamAsync(Guid driverId, params LocationPing[] pings) - { - using var channel = _fixture.CreateGrpcChannel(); - var client = new TelemetryService.TelemetryServiceClient(channel); - - // gRPC call metadata travels as HTTP/2 headers, which is how the dev principal accessor - // sees it. The real Entra claim replaces this without the handler changing. - using var call = client.ReportLocations(new Metadata - { - { HeaderDriverPrincipalAccessor.DriverIdHeader, driverId.ToString() } - }); - - foreach (var ping in pings) - await call.RequestStream.WriteAsync(ping); - - // Half-close: this is what makes the single ack come back. - await call.RequestStream.CompleteAsync(); - - return await call.ResponseAsync; - } + private Task StreamAsync(Guid driverId, params LocationPing[] pings) => + ReportLocationsClient.StreamAsync(_fixture.CreateGrpcChannel, driverId, pings); private async Task LoadAsync(Guid driverId) { @@ -213,11 +188,6 @@ private async Task StreamAsync(Guid driverId, params Location private static string CellAt(double lat, double lon) => H3CellIndexer.TryComputeCell(lat, lon, 9)!; - private static LocationPing PingAt(double lat, double lon, double accuracyMeters = 8d) => new() - { - Lat = lat, - Lon = lon, - AccuracyMeters = accuracyMeters, - DeviceTimestamp = Timestamp.FromDateTimeOffset(DateTimeOffset.UtcNow) - }; + private static LocationPing PingAt(double lat, double lon, double accuracyMeters = 8d) => + ReportLocationsClient.PingAt(lat, lon, accuracyMeters); } diff --git a/tests/CritterCab.Telemetry.Tests/ReportLocations/Slice3KafkaPublishTests.cs b/tests/CritterCab.Telemetry.Tests/ReportLocations/Slice3KafkaPublishTests.cs index d64a705..1d66167 100644 --- a/tests/CritterCab.Telemetry.Tests/ReportLocations/Slice3KafkaPublishTests.cs +++ b/tests/CritterCab.Telemetry.Tests/ReportLocations/Slice3KafkaPublishTests.cs @@ -91,29 +91,9 @@ private IConsumer CreateConsumer() => AutoOffsetReset = AutoOffsetReset.Earliest }).Build(); - private async Task StreamAsync(Guid driverId, params LocationPing[] pings) - { - using var channel = _fixture.CreateGrpcChannel(); - var client = new TelemetryService.TelemetryServiceClient(channel); - - using var call = client.ReportLocations(new GrpcMetadata - { - { HeaderDriverPrincipalAccessor.DriverIdHeader, driverId.ToString() } - }); - - foreach (var ping in pings) - await call.RequestStream.WriteAsync(ping); + private Task StreamAsync(Guid driverId, params LocationPing[] pings) => + ReportLocationsClient.StreamAsync(_fixture.CreateGrpcChannel, driverId, pings); - await call.RequestStream.CompleteAsync(); - - return await call.ResponseAsync; - } - - private static LocationPing PingAt(double lat, double lon) => new() - { - Lat = lat, - Lon = lon, - AccuracyMeters = 8d, - DeviceTimestamp = ProtoTimestamp.FromDateTimeOffset(DateTimeOffset.UtcNow) - }; + private static LocationPing PingAt(double lat, double lon) => + ReportLocationsClient.PingAt(lat, lon); } diff --git a/tests/CritterCab.Telemetry.Tests/ReportLocations/Slice3PublishOrderingTests.cs b/tests/CritterCab.Telemetry.Tests/ReportLocations/Slice3PublishOrderingTests.cs index e11502f..515addc 100644 --- a/tests/CritterCab.Telemetry.Tests/ReportLocations/Slice3PublishOrderingTests.cs +++ b/tests/CritterCab.Telemetry.Tests/ReportLocations/Slice3PublishOrderingTests.cs @@ -1,6 +1,5 @@ using CritterCab.Telemetry.ReportLocations; using CritterCab.Telemetry.V1; -using Google.Protobuf.WellKnownTypes; using Grpc.Core; using Marten; using Microsoft.Extensions.DependencyInjection; @@ -74,23 +73,8 @@ public async Task a_failed_publish_leaves_the_baseline_unwritten_so_the_next_pin recovered.H3Cell.ShouldBe(H3CellIndexer.TryComputeCell(LoopLat, LoopLon, 9)); } - private async Task StreamAsync(Guid driverId, params LocationPing[] pings) - { - using var channel = _fixture.CreateGrpcChannel(); - var client = new TelemetryService.TelemetryServiceClient(channel); - - using var call = client.ReportLocations(new Metadata - { - { HeaderDriverPrincipalAccessor.DriverIdHeader, driverId.ToString() } - }); - - foreach (var ping in pings) - await call.RequestStream.WriteAsync(ping); - - await call.RequestStream.CompleteAsync(); - - return await call.ResponseAsync; - } + private Task StreamAsync(Guid driverId, params LocationPing[] pings) => + ReportLocationsClient.StreamAsync(_fixture.CreateGrpcChannel, driverId, pings); private async Task LoadAsync(Guid driverId) { @@ -99,11 +83,6 @@ private async Task StreamAsync(Guid driverId, params Location return await session.LoadAsync(driverId); } - private static LocationPing PingAt(double lat, double lon) => new() - { - Lat = lat, - Lon = lon, - AccuracyMeters = 8d, - DeviceTimestamp = Timestamp.FromDateTimeOffset(DateTimeOffset.UtcNow) - }; + private static LocationPing PingAt(double lat, double lon) => + ReportLocationsClient.PingAt(lat, lon); } diff --git a/tests/CritterCab.Telemetry.Tests/TelemetryKafkaTestFixture.cs b/tests/CritterCab.Telemetry.Tests/TelemetryKafkaTestFixture.cs index cf2e7a0..b01b8c6 100644 --- a/tests/CritterCab.Telemetry.Tests/TelemetryKafkaTestFixture.cs +++ b/tests/CritterCab.Telemetry.Tests/TelemetryKafkaTestFixture.cs @@ -1,5 +1,6 @@ using Alba; using CritterCab.Telemetry.LastKnownPosition; +using DotNet.Testcontainers.Images; using Grpc.Net.Client; using Marten; using Microsoft.AspNetCore.TestHost; @@ -21,7 +22,12 @@ namespace CritterCab.Telemetry.Tests; // production wiring intact to test the TRANSPORT. Two different questions, two different hosts. public class TelemetryKafkaTestFixture : IAsyncLifetime { + // Unique container names: this project now starts TWO Postgres containers (this fixture and + // TelemetryTestFixture), and xUnit runs their collections in parallel, so a fixed name would + // collide. Same reason the Kafka container below is named. private readonly PostgreSqlContainer _postgres = new PostgreSqlBuilder("postgres:18-alpine") + .WithName($"telemetry-kafka-pg-{Guid.NewGuid():N}") + .WithImagePullPolicy(PullPolicy.Missing) .Build(); // Image pinned explicitly, like the Postgres container above — Testcontainers has deprecated @@ -33,6 +39,8 @@ public class TelemetryKafkaTestFixture : IAsyncLifetime // its log directory to have been formatted by kafka-storage.sh first, so the two combined // produce a broker that reads zookeeper.properties, finds no meta.properties, and exits 1. private readonly KafkaContainer _kafka = new KafkaBuilder("confluentinc/cp-kafka:7.6.1") + .WithName($"telemetry-kafka-{Guid.NewGuid():N}") + .WithImagePullPolicy(PullPolicy.Missing) .Build(); public IAlbaHost Host { get; private set; } = null!; diff --git a/tests/CritterCab.Telemetry.Tests/TelemetryTestFixture.cs b/tests/CritterCab.Telemetry.Tests/TelemetryTestFixture.cs index 40bb218..e317206 100644 --- a/tests/CritterCab.Telemetry.Tests/TelemetryTestFixture.cs +++ b/tests/CritterCab.Telemetry.Tests/TelemetryTestFixture.cs @@ -1,5 +1,6 @@ using System.Collections.Concurrent; using Alba; +using DotNet.Testcontainers.Images; using CritterCab.Telemetry.LastKnownPosition; using CritterCab.Telemetry.ReportLocations; using CritterCab.Telemetry.TelemetryPolicy; @@ -22,7 +23,11 @@ namespace CritterCab.Telemetry.Tests; // default TelemetryPolicy before any test acts. public class TelemetryTestFixture : IAsyncLifetime { + // Named uniquely because the project now starts a second Postgres container in + // TelemetryKafkaTestFixture, and xUnit runs the two collections in parallel. private readonly PostgreSqlContainer _postgres = new PostgreSqlBuilder("postgres:18-alpine") + .WithName($"telemetry-test-{Guid.NewGuid():N}") + .WithImagePullPolicy(PullPolicy.Missing) .Build(); public IAlbaHost Host { get; private set; } = null!;