From c14ca0a95d9e50e788e08447c0f2cd9ab06cd375 Mon Sep 17 00:00:00 2001 From: mnoah1 Date: Thu, 16 Jul 2026 17:50:16 +0000 Subject: [PATCH 1/7] Record step design --- doc/rfc/index.md | 3 + doc/rfc/stovepipe/steps/record.md | 228 ++++++++++++++++++++++++++++++ doc/rfc/stovepipe/workflow.md | 1 + 3 files changed, 232 insertions(+) create mode 100644 doc/rfc/stovepipe/steps/record.md diff --git a/doc/rfc/index.md b/doc/rfc/index.md index b7cd7db5..9dc4724a 100644 --- a/doc/rfc/index.md +++ b/doc/rfc/index.md @@ -26,6 +26,9 @@ Design documents and technical proposals, grouped by scope. Shared/cross-cutting - [Stovepipe Workflow](stovepipe/workflow.md) - Post-merge validation pipeline overview: ingest, process, build, record greenness, analyze projects, notify downstream - [Process stage](stovepipe/steps/process.md) - Build-strategy decision, per-queue concurrency gate, backlog coalescing, entity model, platform prerequisites +- [Build stage](stovepipe/steps/build.md) - Trigger-only stage and Stovepipe's URI-based BuildRunner contract +- [Buildsignal stage](stovepipe/steps/buildsignal.md) - Build polling, terminal status persistence, and the handoff to record +- [Record stage](stovepipe/steps/record.md) - Immutable greenness facts, Queue coordination, Hooks notification, and the Phase 1 handoff to analyze ## Runway diff --git a/doc/rfc/stovepipe/steps/record.md b/doc/rfc/stovepipe/steps/record.md new file mode 100644 index 00000000..05285dcf --- /dev/null +++ b/doc/rfc/stovepipe/steps/record.md @@ -0,0 +1,228 @@ +# Record stage + +`record` turns a terminal build into a durable greenness fact. + +- In Phase 1 it records whole-repository greenness, advances the Queue's last-green bookmark when the result is green, releases the Request's validation slot, notifies downstream systems, and completes the Request. +- In Phase 2 the same stage records project greenness and notifies downstream systems at project granularity. Mentioned in this doc, but to be expanded on before future implementation. + +See [workflow.md](../workflow.md) for the complete pipeline, [build.md](build.md) for how builds are created, and [buildsignal.md](buildsignal.md) for the terminal-only handoff into this stage. + +`record` owns persistence and publication of greenness facts. It does not decide build scope, poll a build, interpret a target graph, or map targets to projects. Those responsibilities belong to `process`, `buildsignal`, and `analyze`. + +## Scope of this design + +This document fully specifies the Phase 1 path needed for whole-repository greenness. It also outlines how Phase 2 will be accommodated at a high level. + +In Phase 1, `record` finishes the Request itself: after persisting the whole-repository fact it moves the Request straight from `processing` to `recorded_green` or `recorded_not_green`. There is no `analyzing` state and no fan-out yet — that machinery arrives with Phase 2, which inserts a non-terminal `analyzing` state between `processing` and the recorded states, retargets record's final CAS at it, and adds a publish to `analyze`, which then owns the terminal transition. The insertion is confined to record's last step and the state enum; facts, Queue reconciliation, and Hooks are untouched. + +## Input, partitioning, and re-entry + +`record` consumes a `RecordBuild` message containing a build id. `buildsignal` publishes it only after persisting a terminal `Build.Status`. The build id is the runner-minted `Build.ID`, so `record` loads the Build with a direct key lookup and then loads its Request through `Build.RequestID`. + +The record topic is partitioned by **request id**, not build id. Phase 1 has one build per Request, so the two are equivalent today; the choice means Phase 2's several project builds per Request arrive serialized, keeping completion bookkeeping single-writer, with no topic change. Partitioning only reduces contention — correctness still comes from immutable facts and optimistic locking. + +Phase 1 builds all carry the whole-repository scope (the zero scope) and run the path in this document; Phase 2 branches on the Build's immutable scope to write a project fact instead (see the reservation below). Project identity is deferred to `analyze.md`, with one rule fixed now: a stable project id (what greenness is *about*) is distinct from the opaque build scope (what a runner *builds*), and `record` never infers one by parsing the other. + +## Greenness is an immutable fact + +A greenness fact answers "how broken was this scope at this Queue URI?" Its identity is: + +``` +(queue, uri, project) +``` + +`project` is empty for whole-repository greenness and is a stable project id in Phase 2. The key is derivable from identity the controller already holds, so storage requires no query by attribute or secondary index. + +The fact contains: + +| Field | Meaning | +| ----------- | ----------------------------------------------------------------------------------- | +| `Queue` | Stable Queue name that namespaces the validation | +| `URI` | Opaque commit URI under validation | +| `Project` | Empty for the whole repository; stable project id for Phase 2 | +| `Degree` | Health degree in the closed interval `[0, 1]`; `0` is green and `1` is fully broken | +| `RequestID` | Request that established the fact | +| `BuildID` | Terminal build whose verdict established the fact | +| `CreatedAt` | Millisecond timestamp at which the fact was first recorded | + +Facts are create-only. A duplicate create for the same identity is reconciled by loading the existing row: + +- Same Request → the existing fact is authoritative; continue from it. This absorbs redelivery and duplicate builds, even ones reporting different verdicts. First recorded fact wins. +- Different Request → the `(Queue, URI)` ingest dedup invariant has been violated. Return an error rather than overwrite history. + +Absence remains distinct from degree `0`. Callers gating deployments must treat absence as not green. + +### Phase 1 degree mapping + +MVP whole-repository builds use only the endpoints: + +| Terminal build status | Degree | +| --------------------- | ------ | +| `succeeded` | `0` | +| `failed` | `1` | +| `cancelled` | `1` | + +Intermediate degrees are reserved for project analysis and deferred with the project mapping contract; Phase 1 does not manufacture fractional values. + +## Phase 1 algorithm + +For a delivery carrying build id `B`: + +``` +1. Load Build B. + - ErrNotFound -> retryable; buildsignal's write may not be visible yet. + - other store error -> return raw; the classifier decides. + +2. Require a terminal Build.Status. + - non-terminal -> return a non-retryable invariant error. buildsignal must not publish it. + +3. Load Request R = RequestStore.Get(Build.RequestID). + - ErrNotFound -> retryable; redelivery converges on a lagging read. + - other store error -> return raw. + +4. Inspect R.State. + - superseded -> ack; no fact or notification is written. + - recorded_green / recorded_not_green -> ack; the terminal write is the algorithm's last + durable step, so a terminal state proves every prior effect already happened. + - processing -> continue. + - accepted -> retryable: the Build proves process admitted R, so this is a stale pre-admit + read (the same lagging-read phenomenon as step 3). A genuine state-machine fault still + dead-letters at MaxAttempts. + - anything else -> return a non-retryable invariant error. + +5. Map Build.Status to a whole-repository degree and create the Greenness fact keyed by + (R.Queue, R.URI, empty project). + - ErrAlreadyExists -> load and reconcile the existing immutable fact. + - other store error -> return raw. + +6. Reconcile the Queue in one CAS retry loop: + a. Decrement in_flight_count, clamping at zero. + b. If the persisted fact is green and LastGreenRequestID is empty or older than R.ID + per entity.CompareRequestID, set LastGreenURI = R.URI and LastGreenRequestID = R.ID. + c. If no field changes, skip the write. + +7. Notify the Hooks extension with the Greenness identity. + - return errors raw; the hook backend's classifier decides retryability. + +8. CAS R: processing -> recorded_green if the persisted fact is green, else recorded_not_green. + - newVersion = oldVersion + 1; assign only after Update succeeds. + - ErrVersionMismatch -> retryable; reload and run the algorithm again. + +9. ack. +``` + +Every decision after step 5 uses the **persisted** fact, not the status from the current delivery: if duplicate builds disagree, the first immutable fact controls the Queue bookmark, Hooks event, and final state. + +The durable order is fact → Queue → Hooks → Request terminal. The terminal write comes last deliberately: it is the completion marker, so a Request read as recorded proves the fact, slot release, bookmark, and hook all happened, and step 4 can ack terminal states unconditionally. The cost is that a crash between steps 7 and 8 re-notifies the hook on retry — absorbed by the greenness identity key. Everything before the terminal write is recognize-and-skip on retry: the fact reloads, the clamped decrement bottoms out at zero, and the bookmark guard skips equal-or-older candidates. + +Phase 2 changes only step 8: the CAS retargets `processing → analyzing` and a publish to `analyze` follows it, with `analyze` owning the terminal transition (see the reservation below). + +### Slot release keeps the existing count + +`record` releases the build slot in the queue row by CAS-decrementing `in_flight_count`, clamped at zero, before the Request's terminal write. Since Queue and Request are separate writes - a crash between these two writes may release an extra slot (preferable to holding an extra slot, which could lead to a deadlock). Aligns with existing DLQ reconciler behavior. + +As noted in process.md, we may expand this to track leases by request ID in the future. + +### Last-green advancement (Queue bookmark) + +The bookmark only moves forward. The Queue gains one field, `LastGreenRequestID` — the request id that owns the current `LastGreenURI`. On a green fact, step 6 adopts the pair `(R.URI, R.ID)` only when the stored id is empty or older than `R.ID` per `entity.CompareRequestID` — the same ingest-order comparison `ingest` and `process` already use for coalescing. + +A failed or cancelled build releases its slot but never moves the bookmark. + +## Request lifecycle + +Phase 1 (planned MVP work) uses the states in [stovepipe/entity/request.go](../../../../stovepipe/entity/request.go). `record` sees `processing` on the happy path and CASes it directly to `recorded_green` or `recorded_not_green` per the persisted fact; `superseded` and the recorded states ack at step 4, and `accepted` is the retryable stale read described there. + +Phase 2 inserts a non-terminal `analyzing` state ("whole-repository fact recorded, slot released, project analysis in flight") between `processing` and the recorded states, and broadens the recorded states to mean "all planned facts recorded". + +## Hooks + +Hooks are the notification boundary, not the source of truth; the GreennessStore is authoritative and commits before any hook fires. + +Following the extension rule "identity in, resolve internally," the Hooks contract takes a thin greenness identity rather than a controller-assembled external payload: + +``` +Notify(ctx, GreennessRef{Queue, URI, Project}) error +``` + +An implementation resolves the immutable fact through dependencies injected at Factory construction, translates it to its external representation, and publishes it. Service wiring owns Factory routing and may select an implementation by Queue name. + +Delivery is at-least-once with the greenness identity as the idempotency key; hook implementations or their downstreams must absorb duplicates. "Fire-and-forget" refers to downstream consumption, not the publish itself: `record` never waits for consumers to act on an event, but a failed `Notify` fails the delivery and is retried — the Request cannot complete until the publish succeeds. + +## Phase 2 plans + +Phase 2 can expand upon the record phase: + +- **Pipeline**: the final CAS retargets `processing → analyzing`, and `record` then publishes the Request id to `analyze` — for green and not-green facts alike, since a failed build is when project attribution matters most. `analyze` owns the terminal transition once all planned project facts exist. The message stays id-only and the consumer idempotent. +- **Project builds**: `record` runs the same load-fact-notify flow, keying the fact with the stable project id attached to the Build and notifying Hooks with that identity. The Queue's in-flight count and `LastGreenURI` are whole-repository concerns and stay untouched. + +Other activities like determining project identity, target-graph retrieval, completion tracking, intermediate degree semantics — belongs to `analyze.md`. Two storage boundaries are fixed now. + +`GreennessStore` is key/value-shaped: + +- `Create(ctx, greenness)` creates one immutable fact and returns `ErrAlreadyExists` when its composite identity is taken. +- `Get(ctx, queue, uri, project)` retrieves one fact by full identity and returns `ErrNotFound` when absent. `project` field reserved for future state. + +There is no `Update`, list, filter, or query-by-degree operation. A corrected verdict is a new Request/fact, not an in-place rewrite of historical truth. + +`QueueStore` and `RequestStore` retain their existing generic CAS `Update` methods. Version arithmetic stays in the controller: compute `newVersion = oldVersion + 1`, pass both versions to the store, and assign the in-memory version only after success. + +Phase 2's latest-green project mapping, if adopted, is a separate key/value store rather than an index hidden inside GreennessStore. + +## Message-queue additions + +| Topic key | Message | Producer | Consumer | Partition key | +| --------- | ----------------------- | ------------- | -------- | ------------- | +| `record` | `RecordBuild{build_id}` | `buildsignal` | `record` | Request id | + +## Idempotency and competing outcomes + +At-least-once delivery is safe by construction: + +- **Build or Request not visible yet** — retry until the producing write is visible. +- **Greenness already created** — load it and continue from the authoritative fact. +- **Queue already reconciled** — the clamped decrement bottoms out at zero and the bookmark guard skips equal-or-older candidates. +- **Hook notified, then crash before the terminal write** — retry re-notifies; the greenness identity dedups. +- **Duplicate builds for one Request** — the first Greenness create wins; later builds cannot overwrite it. +- **Request already terminal** — ack. The terminal write is the last durable step, so nothing can be missing. + +An existing fact from a different Request or a non-terminal Build is an invariant violation, not an expected control-flow outcome. A Request read as `accepted` is neither: the Build proves admission happened, so it is a stale read that redelivery converges. + +## Error classification + +Plain errors remain non-retryable by default. Controllers return extension errors raw so the composed backend classifiers decide whether infrastructure failures are retryable. The controller overrides only cases whose meaning is known locally: + +| Failure | Disposition | Reason | +| ------------------------------------------------ | ------------- | ----------------------------------------------------------------------- | +| Build not found | retryable | `buildsignal` may have published ahead of a lagging read | +| Request not found | retryable | the Build references an older Request write | +| Request read as `accepted` | retryable | stale pre-admit version of the row; the Build proves admission happened | +| Queue CAS version mismatch | retryable | reload and reapply idempotent reconciliation | +| Request CAS version mismatch | retryable | reload and re-evaluate the state | +| Non-terminal Build or invalid Request state | non-retryable | producer/state-machine invariant violation | +| Hooks, GreennessStore, QueueStore, RequestStore | raw error | backend classifier has the required failure knowledge | + +## DLQ and fail-closed behavior + +The record DLQ must not call the current generic `failRequest`: its payload names a terminal Build, so the actual verdict is already known, and replacing a successful build with a conservative not-green fact would falsify durable history. + +`record_dlq` runs the same idempotent reconciliation algorithm from the Build id under `errs.AlwaysRetryableProcessor`: + +- If the Build is terminal, persist and propagate its actual immutable fact. +- If Build or Request storage is temporarily unavailable, keep retrying. +- If the payload is malformed or the Build is permanently missing, the message is a poison reconciliation item requiring an operational alert; there is no trustworthy Request identity to mutate. + +The Queue slot is released before Hooks, so a broken notification backend cannot wedge validation of newer heads; the Request stays `processing` until reconciliation completes the hook and the terminal write. + +Earlier-stage DLQ reconciliation still forces a conservative degree `1` when no terminal verdict exists: create the whole-repository fact (with empty `BuildID` — the marker of a conservative rather than observed verdict), release the slot with the same clamped decrement, and transition to `recorded_not_green`. Changing state without writing the fact would leave externally visible greenness absent, failing the fail-closed contract. + +First-fact-wins makes the conservative verdict final, deliberately superseding `workflow.md`'s remark that "a late successful update wins cleanly over the conservative one": a late green for a fail-closed URI is dropped, at bounded cost — the branch keeps moving, and the next head re-establishes greenness on its own Request. Revise `workflow.md` alongside this design. + +## Edge cases + +- **Head equals last-green.** The build still produces a terminal verdict. A green result may share the bookmark's URI; the guard compares request ids, not URIs, so an equal-or-older candidate skips and nothing regresses. +- **Cancelled build.** Stovepipe never initiates cancellation, but a backend may still report it. Record degree `1`, release the slot, complete the Request. +- **History rewrite while a build runs.** The Request keeps the strategy and URI pinned at admission; record stores the fact about that immutable URI. A later head is handled independently by `process`. +- **Late successful result after fail-closed terminal.** The terminal `recorded_not_green` Request wins; record acks without rewriting greenness or reopening the pipeline. +- **Hook succeeds, terminal CAS interrupted.** Retry re-notifies the hook; the greenness identity makes the duplicate safe. +- **Terminal CAS succeeds, ack fails.** Redelivery reads the recorded state at step 4 and acks. diff --git a/doc/rfc/stovepipe/workflow.md b/doc/rfc/stovepipe/workflow.md index 28bc3662..f2b9663f 100644 --- a/doc/rfc/stovepipe/workflow.md +++ b/doc/rfc/stovepipe/workflow.md @@ -163,6 +163,7 @@ Per-stage design detail lives under `steps/` so this doc stays a pipeline overvi - [process.md](steps/process.md) — build-strategy decision, concurrency gate, backlog coalescing, [concurrency lifecycle](steps/process.md#concurrency-lifecycle), entity changes, [waiting for a slot](steps/process.md#waiting-for-a-slot) - [build.md](steps/build.md) — trigger-only stage: reads the decided scope off the Request, triggers the build-runner, hands off to buildsignal; the stovepipe `BuildRunner` contract and why it differs from SubmitQueue's - [buildsignal.md](steps/buildsignal.md) — the poll loop: hold-based re-poll cadence, target-graph return, per-build partitioning, and the fail-closed handoff to record +- [record.md](steps/record.md) — immutable greenness facts, idempotent Queue-slot release, monotonic last-green advancement, Hooks notification, and the Phase 1 handoff to analyze ## Dedup, idempotency, and history rewrites From aea1e5051d9c65e1856d61917a4c2e14cab68bf9 Mon Sep 17 00:00:00 2001 From: mnoah1 Date: Tue, 4 Aug 2026 19:59:44 +0000 Subject: [PATCH 2/7] Update record step --- doc/rfc/index.md | 2 +- doc/rfc/stovepipe/steps/build.md | 32 ++-- doc/rfc/stovepipe/steps/record.md | 251 ++++++++++++++---------------- doc/rfc/stovepipe/workflow.md | 6 +- 4 files changed, 140 insertions(+), 151 deletions(-) diff --git a/doc/rfc/index.md b/doc/rfc/index.md index 9dc4724a..2dc5552d 100644 --- a/doc/rfc/index.md +++ b/doc/rfc/index.md @@ -28,7 +28,7 @@ Design documents and technical proposals, grouped by scope. Shared/cross-cutting - [Process stage](stovepipe/steps/process.md) - Build-strategy decision, per-queue concurrency gate, backlog coalescing, entity model, platform prerequisites - [Build stage](stovepipe/steps/build.md) - Trigger-only stage and Stovepipe's URI-based BuildRunner contract - [Buildsignal stage](stovepipe/steps/buildsignal.md) - Build polling, terminal status persistence, and the handoff to record -- [Record stage](stovepipe/steps/record.md) - Immutable greenness facts, Queue coordination, Hooks notification, and the Phase 1 handoff to analyze +- [Record stage](stovepipe/steps/record.md) - Immutable validation facts, Queue coordination, Hooks notification, and the Phase 1 handoff to analyze ## Runway diff --git a/doc/rfc/stovepipe/steps/build.md b/doc/rfc/stovepipe/steps/build.md index 54e56905..15e1194b 100644 --- a/doc/rfc/stovepipe/steps/build.md +++ b/doc/rfc/stovepipe/steps/build.md @@ -39,7 +39,7 @@ For a delivery carrying request id `R`: - baseURI = R.BaseURI if R.BuildStrategy == incremental_since_green, else "" (full build). - (headURI = R.URI, baseURI) identify the scope; both are opaque SourceControl tokens. -5. Trigger: buildID, err := buildRunner.Trigger(ctx, R.URI, baseURI, metadata) +5. Trigger: buildID, err := buildRunner.Trigger(ctx, baseURI, R.URI, metadata) - Trigger takes no caller-supplied id; the runner mints the build's identity, and buildID becomes Build.ID — SubmitQueue's exact convention (see "Alternatives considered" under the contract sketch). @@ -52,8 +52,10 @@ For a delivery carrying request id `R`: either domain — the shape is deferred until then, not decided here. - failure -> return raw; classifier decides (transient runner blip retryable, bad URI not). -6. Persist Build{ID: buildID.ID, RequestID: R.ID, URI: R.URI, BaseURI: baseURI, - Status: accepted, Version: 1} via BuildStore.Create. +6. Persist Build{ID: buildID.ID, RequestID: R.ID, Status: accepted, Version: 1} + via BuildStore.Create. + - the row carries no scope; it is recoverable from the Request's immutable fields + (see the entity table). - a crash between step 5 and this write orphans the triggered build (see Idempotency). - ErrAlreadyExists -> benign (reachable only with a backend that returns deterministic ids for retried triggers); continue to step 7. @@ -78,7 +80,7 @@ Every branch is safe under at-least-once redelivery — with SubmitQueue's postu - **Request not found** — non-retryable; storage's read-after-write guarantee means a miss here is a storage defect, not a lag condition to retry through. - **Strategy not yet visible** — retryable; the producing stage's write is not visible on this reader yet. - **Request already terminal** (step 2) — ack, no build. A redelivery after `record` finished, or after `process` superseded the head, never starts a stale build. -- **Redelivery while the Request is still in flight** (crash or failure anywhere in steps 5–8) — the redelivery re-runs from step 1, `Trigger` mints a fresh id, `Create` persists a second `Build` row, and a second poll loop starts. Harmless, in three layers: both builds target the identical `(headURI, baseURI)` scope; each `Build` polls in its own partition and `buildsignal` short-circuits the moment the Request goes terminal (its step 3); and `record`'s terminal transition is CAS-guarded, so the second verdict is a no-op. A build triggered but never persisted (crash between steps 5 and 6) is the same story minus the row: an orphan the runner finishes and nobody ever reads. Wasted CI compute, not a correctness risk — the same accepted trade as SubmitQueue. +- **Redelivery while the Request is still in flight** (crash or failure anywhere in steps 5–8) — the redelivery re-runs from step 1, `Trigger` mints a fresh id, `Create` persists a second `Build` row, and a second poll loop starts. Harmless, in three layers: both builds target the identical `(headURI, baseURI)` scope; each `Build` polls in its own partition and `buildsignal` short-circuits the moment the Request goes terminal (its step 3); and `buildsignal`'s outcome write is first-writer-wins, so the second verdict cannot flip the Request's state or overwrite the create-only validation fact. A build triggered but never persisted (crash between steps 5 and 6) is the same story minus the row: an orphan the runner finishes and nobody ever reads. Wasted CI compute, not a correctness risk — the same accepted trade as SubmitQueue. - **Trigger / publish / other store failure** — nothing durable is left half-written that a redelivery can't reconcile; the error rejects to DLQ, and the fail-closed reconciler drives the Request terminal (see [workflow.md](doc/rfc/stovepipe/workflow.md#fail-closed-on-unprocessable-work)). ## Edge cases @@ -128,7 +130,7 @@ The batches are **identity** — thin references carrying ids, not change conten Stovepipe validates **one commit** against a baseline (or in full). Its `build` controller reads two opaque URIs off the `Request` and triggers: ```go -buildID, err := buildRunner.Trigger(ctx, headURI, baseURI, metadata) +buildID, err := buildRunner.Trigger(ctx, baseURI, headURI, metadata) ``` There is no batch, no dependency list, and nothing to resolve — the URIs *are* the identity, owned by `SourceControl`. `process` already decided incremental-vs-full; `build` just reads `R.BuildStrategy`/`R.BaseURI` and acts. @@ -153,16 +155,16 @@ type BuildRunner interface { // Trigger starts a new build every call and mints the build's identity — // there is no caller-supplied dedup input, matching SubmitQueue's contract // exactly (see "Alternatives considered for the build identity" below - // for other shapes this doc considered). headURI is the commit - // under validation; baseURI is the incremental baseline (empty for a full - // build). metadata is caller annotations the runner may echo but must not + // for other shapes this doc considered). baseURI is the incremental + // baseline (empty for a full build); headURI is the commit under + // validation. metadata is caller annotations the runner may echo but must not // depend on — empty today, but expected to carry real data eventually (e.g. // conflict-graph info, or other upstream decisions relevant to the build) // once a concrete need lands in either domain; the shape is deferred until // then, not decided here. Runner-side work is async; callers learn progress // via Status. // Returns the runner-assigned build id, which the caller adopts as Build.ID. - Trigger(ctx context.Context, headURI, baseURI string, metadata entity.BuildMetadata) (entity.BuildID, error) + Trigger(ctx context.Context, baseURI, headURI string, metadata entity.BuildMetadata) (entity.BuildID, error) // Status returns the current status. Takes the id Trigger returned // (Build.ID). May round-trip to the backend. BuildMetadata is @@ -189,7 +191,7 @@ type Factory interface{ For(cfg Config) (BuildRunner, error) } The shape isn't decided here because project semantics belong to `analyze`, not `build`: how a project maps to a buildable scope (a Bazel target pattern, a directory, a service name) is implementer-specific per [workflow.md](doc/rfc/stovepipe/workflow.md#project---greenness-at-a-finer-grain). The expectation is that this stays an opaque token — following the same "identity in, resolve internally" shape already used for `headURI`/`baseURI` (owned and interpreted by `SourceControl`) — that `build` reads off the `Request`/message and hands to the runner uninterpreted, rather than a structured type `build` would have to understand: ```go -Trigger(ctx context.Context, headURI, baseURI string, projectScope entity.ProjectScope, metadata entity.BuildMetadata) (entity.BuildID, error) +Trigger(ctx context.Context, baseURI, headURI string, projectScope entity.ProjectScope, metadata entity.BuildMetadata) (entity.BuildID, error) ``` `ProjectScope` lives in `stovepipe/entity` alongside `BuildID`/`BuildStatus`/`BuildMetadata` — projects have no SubmitQueue equivalent at all, not even a shape to mirror. Its zero value covers Phase 1 (no project — whole-repo/incremental scope only, exactly today's sketch); `analyze` is what would populate a non-zero value for Phase 2. This mirrors the additive optional field already reserved on `BuildRequest` for the same purpose (see [Queue contract additions](#queue-contract-additions)) — the wire message and the extension contract need the same new dimension, and both are deferred to the same design. @@ -198,7 +200,7 @@ Both `Trigger` and `Status`/`Cancel` differ *in contract* between domains, even There is exactly one build id: the runner mints it at `Trigger`, `build` adopts it as `Build.ID`, and every later call and message carries it verbatim — `Status`/`Cancel` take the same value `Trigger` returned, the queue payload is the same value, the store key is the same value. This is SubmitQueue's convention end to end. The id is opaque: no stovepipe reader parses it, derives it, or equates it with another entity's id — the trap SubmitQueue's speculate/cancel path falls into. And per the extension rules a runner keeps only transient local state, so the durable `Request` ↔ `Build` linkage lives in **our** store as `Build.RequestID`, never in the runner. -Supporting entity types: `BuildStatus`, `BuildMetadata`, and `BuildID` live in `stovepipe/entity`, shaped the same as SubmitQueue's `submitqueue/entity` equivalents but defined and duplicated locally rather than shared — `BuildStatus` is the narrow lowercase enum `"" (unknown) / accepted / running / succeeded / failed / cancelled` with an `IsTerminal()` predicate covering the last three, `BuildMetadata` is the free-form `map[string]string`, and `BuildID` is a `{ID string}` wire struct wrapping the one runner-assigned id everywhere it appears — `Trigger`'s return, `Status`/`Cancel`'s parameter, the queue payload. `stovepipe/entity/build.go` keeps what's stovepipe-specific: the `Build` entity itself (`RequestID`/`URI`/`BaseURI` alongside `ID`/`Status`/`Version`). How a target graph reaches `analyze` is out of scope for this doc — left to the `analyze` design. +Supporting entity types: `BuildStatus`, `BuildMetadata`, and `BuildID` live in `stovepipe/entity`, shaped the same as SubmitQueue's `submitqueue/entity` equivalents but defined and duplicated locally rather than shared — `BuildStatus` is the narrow lowercase enum `"" (unknown) / accepted / running / succeeded / failed / cancelled` with an `IsTerminal()` predicate covering the last three, `BuildMetadata` is the free-form `map[string]string`, and `BuildID` is a `{ID string}` wire struct wrapping the one runner-assigned id everywhere it appears — `Trigger`'s return, `Status`/`Cancel`'s parameter, the queue payload. `stovepipe/entity/build.go` keeps what's stovepipe-specific: the `Build` entity itself (`RequestID` alongside `ID`/`Status`/`Version`). How a target graph reaches `analyze` is out of scope for this doc — left to the `analyze` design. ### Alternatives considered for sharing the contract @@ -210,7 +212,7 @@ Several shapes for sharing the `BuildRunner` contract across domains were raised // package platform/extension/buildrunner type BuildRunner interface { Trigger(ctx context.Context, base []entity.Batch, head entity.Batch, metadata entity.BuildMetadata) (entity.BuildID, error) - TriggerChanges(ctx context.Context, headURI, baseURI string, metadata entity.BuildMetadata) (entity.BuildID, error) + TriggerChanges(ctx context.Context, baseURI, headURI string, metadata entity.BuildMetadata) (entity.BuildID, error) Status(ctx context.Context, buildID entity.BuildID) (entity.BuildStatus, entity.BuildMetadata, error) Cancel(ctx context.Context, buildID entity.BuildID) error } @@ -289,18 +291,18 @@ Either could be adopted independently: the idempotency token, if a backend that ## Entity and storage additions needed -**`Build` entity** (`stovepipe/entity/build.go`), following the immutable-except-`Status`/`Version` shape of `entity.Request`; `ID` and `Status` use the stovepipe-local `BuildID`/`BuildStatus` types (see the [contract sketch](#stovepipe-buildrunner-contract-design-sketch)), while `RequestID`/`URI`/`BaseURI` stay stovepipe-specific: +**`Build` entity** (`stovepipe/entity/build.go`), following the immutable-except-`Status`/`Version` shape of `entity.Request`; `ID` and `Status` use the stovepipe-local `BuildID`/`BuildStatus` types (see the [contract sketch](#stovepipe-buildrunner-contract-design-sketch)), while `RequestID` stays stovepipe-specific: | Field | Role | Mutable? | |---|---|---| | `ID` | The build's own key — the runner-assigned id returned by `Trigger` (a Buildkite build number, a CI-gateway job id); opaque, never parsed or derived | no | | `RequestID` | The `Request` this build validates (`Build`→`Request` navigation) | no | -| `URI` | Head URI being built (`== Request.URI`) | no | -| `BaseURI` | Incremental baseline; empty for full builds | no | | `Status` | `accepted / running / succeeded / failed / cancelled` | **yes** — `buildsignal` | | `Version` | `int32` optimistic-locking version | **yes** — with `Status` | +The row deliberately carries no scope: `R.URI`, `R.BaseURI`, and `R.BuildStrategy` — immutable and reachable through `RequestID` — fully determine what a build ran against. + **States** (`Build.Status`): | Status | Meaning | Terminal? | diff --git a/doc/rfc/stovepipe/steps/record.md b/doc/rfc/stovepipe/steps/record.md index 05285dcf..60984456 100644 --- a/doc/rfc/stovepipe/steps/record.md +++ b/doc/rfc/stovepipe/steps/record.md @@ -1,228 +1,215 @@ # Record stage -`record` turns a terminal build into a durable greenness fact. +`record` turns a terminal build outcome into a durable validation fact. -- In Phase 1 it records whole-repository greenness, advances the Queue's last-green bookmark when the result is green, releases the Request's validation slot, notifies downstream systems, and completes the Request. +- In Phase 1 it records whole-repository greenness, advances the Queue's last-green bookmark when the result is green, and notifies downstream systems. - In Phase 2 the same stage records project greenness and notifies downstream systems at project granularity. Mentioned in this doc, but to be expanded on before future implementation. See [workflow.md](../workflow.md) for the complete pipeline, [build.md](build.md) for how builds are created, and [buildsignal.md](buildsignal.md) for the terminal-only handoff into this stage. -`record` owns persistence and publication of greenness facts. It does not decide build scope, poll a build, interpret a target graph, or map targets to projects. Those responsibilities belong to `process`, `buildsignal`, and `analyze`. +`record` owns persistence and publication of validation facts. It does not decide build scope, poll a build, release the Queue's build slot, interpret a target graph, or map targets to projects. Those responsibilities belong to `process`, `buildsignal`, and `analyze`. -## Scope of this design +## Phase 1 algorithm + +For a delivery carrying request id `R`: + +``` +1. Load Request R. + - ErrNotFound -> return raw; non-retryable. + - other store error -> return raw; the classifier decides. -This document fully specifies the Phase 1 path needed for whole-repository greenness. It also outlines how Phase 2 will be accommodated at a high level. +2. Inspect R.State. + - succeeded / failed -> continue. This is the entry condition: buildsignal stamps the + outcome before it publishes here, and both values are verdicts about the code. + - cancelled -> ack. No fact and no notification: the build decided nothing about the + commit (see "When to record an outcome"). + - superseded -> ack; no fact or notification is written. Unreachable in practice. + - accepted / processing / anything else -> return a non-retryable invariant error. -In Phase 1, `record` finishes the Request itself: after persisting the whole-repository fact it moves the Request straight from `processing` to `recorded_green` or `recorded_not_green`. There is no `analyzing` state and no fan-out yet — that machinery arrives with Phase 2, which inserts a non-terminal `analyzing` state between `processing` and the recorded states, retargets record's final CAS at it, and adds a publish to `analyze`, which then owns the terminal transition. The insertion is confined to record's last step and the state enum; facts, Queue reconciliation, and Hooks are untouched. +3. Map R.State to a whole-repository degree and create the ValidationFact keyed by + (R.Queue, R.URI, empty project). + - ErrAlreadyExists -> load and reconcile the existing immutable fact. + - other store error -> return raw. -## Input, partitioning, and re-entry +4. If the persisted fact is green, advance the Queue bookmark in one CAS retry loop: + a. If LastGreenRequestID is empty or older than R.ID per entity.CompareRequestID, set + LastGreenURI = R.URI and LastGreenRequestID = R.ID. + b. If no field changes, skip the write. -`record` consumes a `RecordBuild` message containing a build id. `buildsignal` publishes it only after persisting a terminal `Build.Status`. The build id is the runner-minted `Build.ID`, so `record` loads the Build with a direct key lookup and then loads its Request through `Build.RequestID`. +5. Notify the Hooks extension with the fact identity. + - return errors raw; the hook backend's classifier decides retryability. -The record topic is partitioned by **request id**, not build id. Phase 1 has one build per Request, so the two are equivalent today; the choice means Phase 2's several project builds per Request arrive serialized, keeping completion bookkeeping single-writer, with no topic change. Partitioning only reduces contention — correctness still comes from immutable facts and optimistic locking. +6. ack. +``` -Phase 1 builds all carry the whole-repository scope (the zero scope) and run the path in this document; Phase 2 branches on the Build's immutable scope to write a project fact instead (see the reservation below). Project identity is deferred to `analyze.md`, with one rule fixed now: a stable project id (what greenness is *about*) is distinct from the opaque build scope (what a runner *builds*), and `record` never infers one by parsing the other. +Every decision after step 3 uses the persisted fact (not the outcome read from this delivery's Request). The first immutable fact controls the Queue bookmark and the Hooks event. -## Greenness is an immutable fact +## Validation Fact Recording -A greenness fact answers "how broken was this scope at this Queue URI?" Its identity is: +A validation fact answers "how broken was this scope at this Queue URI?" Its identity is: ``` (queue, uri, project) ``` -`project` is empty for whole-repository greenness and is a stable project id in Phase 2. The key is derivable from identity the controller already holds, so storage requires no query by attribute or secondary index. +`project` is empty for whole-repository greenness and is a stable project id in Phase 2. The fact contains: + | Field | Meaning | | ----------- | ----------------------------------------------------------------------------------- | | `Queue` | Stable Queue name that namespaces the validation | -| `URI` | Opaque commit URI under validation | +| `URI` | Commit URI under validation | | `Project` | Empty for the whole repository; stable project id for Phase 2 | | `Degree` | Health degree in the closed interval `[0, 1]`; `0` is green and `1` is fully broken | | `RequestID` | Request that established the fact | -| `BuildID` | Terminal build whose verdict established the fact | | `CreatedAt` | Millisecond timestamp at which the fact was first recorded | -Facts are create-only. A duplicate create for the same identity is reconciled by loading the existing row: -- Same Request → the existing fact is authoritative; continue from it. This absorbs redelivery and duplicate builds, even ones reporting different verdicts. First recorded fact wins. -- Different Request → the `(Queue, URI)` ingest dedup invariant has been violated. Return an error rather than overwrite history. +Facts are create-only. `ErrAlreadyExists` on create means one of exactly two things: -Absence remains distinct from degree `0`. Callers gating deployments must treat absence as not green. +- Same Request → a redelivery. The existing fact is this delivery's own prior write and necessarily carries the same degree, since the Request's outcome is immutable once stamped. Load it and continue. +- Different Request → the `(Queue, URI)` ingest dedup invariant has been violated. Return an error rather than overwrite history. -### Phase 1 degree mapping +Competing verdicts cannot reach this point: duplicate builds for one Request are resolved a stage earlier, where `buildsignal`'s first-writer-wins outcome write discards the losing build's verdict, so `record` only ever sees one. -MVP whole-repository builds use only the endpoints: +Absence remains distinct from degree `0`. Callers gating deployments must treat absence as not green. -| Terminal build status | Degree | -| --------------------- | ------ | -| `succeeded` | `0` | -| `failed` | `1` | -| `cancelled` | `1` | +### When to record an outcome -Intermediate degrees are reserved for project analysis and deferred with the project mapping contract; Phase 1 does not manufacture fractional values. +A fact is written only when the request reaches a `succeeded` or `failed` verdict. A `cancelled` build is acked with no fact recorded. Callers gating deployments treat an absent fact as not green. -## Phase 1 algorithm +The fail-closed path also produces no fact, as the DLQ reconciler forces `failed` and never publishes to `record`. -For a delivery carrying build id `B`: +### Phase 1 degree mapping -``` -1. Load Build B. - - ErrNotFound -> retryable; buildsignal's write may not be visible yet. - - other store error -> return raw; the classifier decides. +MVP whole-repository builds use only the endpoints, mapped from the outcome `buildsignal` stamped on the Request: -2. Require a terminal Build.Status. - - non-terminal -> return a non-retryable invariant error. buildsignal must not publish it. -3. Load Request R = RequestStore.Get(Build.RequestID). - - ErrNotFound -> retryable; redelivery converges on a lagging read. - - other store error -> return raw. +| Request outcome | Result | +| --------------- | ------------------ | +| `succeeded` | fact at degree `0` | +| `failed` | fact at degree `1` | +| `cancelled` | no fact | -4. Inspect R.State. - - superseded -> ack; no fact or notification is written. - - recorded_green / recorded_not_green -> ack; the terminal write is the algorithm's last - durable step, so a terminal state proves every prior effect already happened. - - processing -> continue. - - accepted -> retryable: the Build proves process admitted R, so this is a stale pre-admit - read (the same lagging-read phenomenon as step 3). A genuine state-machine fault still - dead-letters at MaxAttempts. - - anything else -> return a non-retryable invariant error. - -5. Map Build.Status to a whole-repository degree and create the Greenness fact keyed by - (R.Queue, R.URI, empty project). - - ErrAlreadyExists -> load and reconcile the existing immutable fact. - - other store error -> return raw. -6. Reconcile the Queue in one CAS retry loop: - a. Decrement in_flight_count, clamping at zero. - b. If the persisted fact is green and LastGreenRequestID is empty or older than R.ID - per entity.CompareRequestID, set LastGreenURI = R.URI and LastGreenRequestID = R.ID. - c. If no field changes, skip the write. +Intermediate degrees are reserved for project analysis and deferred with the project mapping contract; Phase 1 does not manufacture fractional values. -7. Notify the Hooks extension with the Greenness identity. - - return errors raw; the hook backend's classifier decides retryability. +### Supporting re-run of same URI (future use case) -8. CAS R: processing -> recorded_green if the persisted fact is green, else recorded_not_green. - - newVersion = oldVersion + 1; assign only after Update succeeds. - - ErrVersionMismatch -> retryable; reload and run the algorithm again. +Widen the key with the `RequestID` they already record so each attempt is its own immutable row, and add a pointer store from `(queue, uri, project)` to the authoritative attempt. Advance to a newer attempt unless the current one is green, since a green build proved the code passed and a later failure only proves the build is non-deterministic. -9. ack. -``` +### Coverage of intermediate commits -Every decision after step 5 uses the **persisted** fact, not the status from the current delivery: if duplicate builds disagree, the first immutable fact controls the Queue bookmark, Hooks event, and final state. +Coalescing means most commits never become a validated Request: a verdict on head `H` with base `B` is implicitly a verdict on every commit in the range `(B, H]`. Downstream tooling still needs to be able to retrieve prior and next green for any commit, including those never ingested directly, or that have been superseded. -The durable order is fact → Queue → Hooks → Request terminal. The terminal write comes last deliberately: it is the completion marker, so a Request read as recorded proves the fact, slot release, bookmark, and hook all happened, and step 4 can ack terminal states unconditionally. The cost is that a crash between steps 7 and 8 re-notifies the hook on retry — absorbed by the greenness identity key. Everything before the terminal write is recognize-and-skip on retry: the fact reloads, the clamped decrement bottoms out at zero, and the bookmark guard skips equal-or-older candidates. +To support that, the rough idea is that we can track this via some additional stores that are written during this record step, which we can expand upon in a separate doc: -Phase 2 changes only step 8: the CAS retargets `processing → analyzing` and a publish to `analyze` follows it, with `analyze` owning the terminal transition (see the reservation below). +- **`CoverageStore`** — on every verdict, green or failed, one row per commit in the covered range: `(queue, uri)` → the covering request and the commit's position within its range. Gives commits with no Request of their own a place in the queue's history. +- **`GreenLogStore`** — on green verdicts only, one row keyed by that position. Because the key is ordered, "previous green" and "next green" become two seeks: the nearest entry below or above a commit's position. -### Slot release keeps the existing count +Neither fits the `ValidationFactStore` proposed above: facts are looked up by exact identity and URIs do not sort, while previous/next-green needs an ordered seek over positions, a different key shape. -`record` releases the build slot in the queue row by CAS-decrementing `in_flight_count`, clamped at zero, before the Request's terminal write. Since Queue and Request are separate writes - a crash between these two writes may release an extra slot (preferable to holding an extra slot, which could lead to a deadlock). Aligns with existing DLQ reconciler behavior. +## Build slot release -As noted in process.md, we may expand this to track leases by request ID in the future. +`Queue.in_flight_count` is released by `buildsignal` before it stamps the outcome and publishes the record step. The DLQ reconciler releases the slot on the fail-closed path for the same reason. `record`'s only Queue write is the last-green bookmark. -### Last-green advancement (Queue bookmark) +## Completion marker: open -The bookmark only moves forward. The Queue gains one field, `LastGreenRequestID` — the request id that owns the current `LastGreenURI`. On a green fact, step 6 adopts the pair `(R.URI, R.ID)` only when the stored id is empty or older than `R.ID` per `entity.CompareRequestID` — the same ingest-order comparison `ingest` and `process` already use for coalescing. +`record` makes no `Request` write in Phase 1, which leaves the stage with no durable marker saying it finished. The consequence is contained today, because every effect is idempotent and a redelivery simply re-runs them, but two things depend on a marker and stay unresolved: -A failed or cancelled build releases its slot but never moves the bookmark. +- **Redelivery cost.** With no marker to check, a redelivery always re-fires the hook. That is safe under at-least-once delivery with the fact identity as the idempotency key, but it is a real duplicate rather than a skipped no-op. +- **Phase 2 completion.** "All planned facts recorded" needs somewhere to live. An earlier draft inserted a non-terminal `analyzing` state between `processing` and the recorded states; that no longer fits, because `buildsignal` drives the Request terminal before `record` ever runs, leaving no non-terminal window to occupy. Phase 2 must either move the outcome write back behind analysis, add a separate completion entity, or track fan-out on the facts themselves. Deferred to `analyze.md`. -## Request lifecycle +## Last-green advancement (Queue bookmark) -Phase 1 (planned MVP work) uses the states in [stovepipe/entity/request.go](../../../../stovepipe/entity/request.go). `record` sees `processing` on the happy path and CASes it directly to `recorded_green` or `recorded_not_green` per the persisted fact; `superseded` and the recorded states ack at step 4, and `accepted` is the retryable stale read described there. +The bookmark only moves forward. The Queue gains one field, `LastGreenRequestID` — the request id that owns the current `LastGreenURI`. It is a proposed addition; `entity.Queue` today carries `LastGreenURI`, `InFlightCount`, and `LatestRequestID` only. On a green fact, step 4 adopts the pair `(R.URI, R.ID)` only when the stored id is empty or older than `R.ID` per `entity.CompareRequestID(R.Queue, …)` — the same ingest-order comparison `ingest` and `process` already use for coalescing. That comparison returns an error on an id that does not match the queue's format; treat it as non-retryable, since re-parsing the same ids cannot succeed. -Phase 2 inserts a non-terminal `analyzing` state ("whole-repository fact recorded, slot released, project analysis in flight") between `processing` and the recorded states, and broadens the recorded states to mean "all planned facts recorded". +A failed or cancelled build never moves the bookmark. ## Hooks -Hooks are the notification boundary, not the source of truth; the GreennessStore is authoritative and commits before any hook fires. +After a validation fact is recorded, hooks will be fired to notify downstream consumers of the greenness change. -Following the extension rule "identity in, resolve internally," the Hooks contract takes a thin greenness identity rather than a controller-assembled external payload: +The Hooks contract takes a thin fact identity, and implementations may resolve additional details as needed: ``` -Notify(ctx, GreennessRef{Queue, URI, Project}) error +Notify(ctx, ValidationFactRef{Queue, URI, Project}) error ``` -An implementation resolves the immutable fact through dependencies injected at Factory construction, translates it to its external representation, and publishes it. Service wiring owns Factory routing and may select an implementation by Queue name. +Delivery is at-least-once with the fact identity as the idempotency key; hook implementations or their downstreams must absorb duplicates. "Fire-and-forget" refers to downstream consumption, not the publish itself: `record` never waits for consumers to act on an event, but a failed `Notify` fails the delivery and is retried. The Request is already terminal and its slot already released by then, so a stuck hook delays the notification without holding up the pipeline. -Delivery is at-least-once with the greenness identity as the idempotency key; hook implementations or their downstreams must absorb duplicates. "Fire-and-forget" refers to downstream consumption, not the publish itself: `record` never waits for consumers to act on an event, but a failed `Notify` fails the delivery and is retried — the Request cannot complete until the publish succeeds. +## Request lifecycle -## Phase 2 plans +Phase 1 uses the states in [stovepipe/entity/request.go](../../../../stovepipe/entity/request.go). `record` runs *after* the Request is terminal: `buildsignal` projects the build's terminal status onto it as `succeeded`, `failed`, or `cancelled` (`RequestState.HasBuildOutcome()`), and only then publishes. So `record` reads an outcome and writes no state. `superseded` is terminal without an outcome. -Phase 2 can expand upon the record phase: +Phase 2 broadens "complete" to mean "all planned facts recorded", which needs a marker this stage does not own — see [Completion marker: open](#completion-marker-open). -- **Pipeline**: the final CAS retargets `processing → analyzing`, and `record` then publishes the Request id to `analyze` — for green and not-green facts alike, since a failed build is when project attribution matters most. `analyze` owns the terminal transition once all planned project facts exist. The message stays id-only and the consumer idempotent. -- **Project builds**: `record` runs the same load-fact-notify flow, keying the fact with the stable project id attached to the Build and notifying Hooks with that identity. The Queue's in-flight count and `LastGreenURI` are whole-repository concerns and stay untouched. +## Message-queue additions -Other activities like determining project identity, target-graph retrieval, completion tracking, intermediate degree semantics — belongs to `analyze.md`. Two storage boundaries are fixed now. +The topic key and message already exist in `stovepipe/core/messagequeue`; only the consumer is outstanding. -`GreennessStore` is key/value-shaped: -- `Create(ctx, greenness)` creates one immutable fact and returns `ErrAlreadyExists` when its composite identity is taken. -- `Get(ctx, queue, uri, project)` retrieves one fact by full identity and returns `ErrNotFound` when absent. `project` field reserved for future state. +| Topic key | Message | Producer | Consumer | Partition key | Message id | +| --------- | ------------------------- | ------------- | -------- | ------------- | ---------- | +| `record` | `Record{id}` (request id) | `buildsignal` | `record` | Request id | Request id | -There is no `Update`, list, filter, or query-by-degree operation. A corrected verdict is a new Request/fact, not an in-place rewrite of historical truth. -`QueueStore` and `RequestStore` retain their existing generic CAS `Update` methods. Version arithmetic stays in the controller: compute `newVersion = oldVersion + 1`, pass both versions to the store, and assign the in-memory version only after success. +Partitioning by request id keeps completion bookkeeping single-writer per Request, and reusing the request id as the message id dedups a redelivered signal into the original message instead of enqueuing a second one. -Phase 2's latest-green project mapping, if adopted, is a separate key/value store rather than an index hidden inside GreennessStore. +## Idempotency and competing outcomes -## Message-queue additions +- **Request not visible** — a storage defect rather than a lag, since the publish follows the committed outcome write. Non-retryable. +- **Fact already created** — load it and continue from the authoritative fact. +- **Bookmark already advanced** — the guard skips equal-or-older candidates and the write is skipped. +- **Hook notified, then crash** — retry re-notifies; the fact identity dedups. +- **Duplicate builds for one Request** — absorbed a stage earlier: `buildsignal`'s outcome write is first-writer-wins, so the Request carries one immutable verdict and `record` never sees a competing one. +- **Redelivery after a complete run** — every effect is recognize-and-skip except the hook, which re-fires (see [Completion marker: open](#completion-marker-open)). -| Topic key | Message | Producer | Consumer | Partition key | -| --------- | ----------------------- | ------------- | -------- | ------------- | -| `record` | `RecordBuild{build_id}` | `buildsignal` | `record` | Request id | +An existing fact from a different Request, or a Request carrying no build outcome, is an invariant violation rather than an expected control-flow outcome. -## Idempotency and competing outcomes +## Error classification -At-least-once delivery is safe by construction: -- **Build or Request not visible yet** — retry until the producing write is visible. -- **Greenness already created** — load it and continue from the authoritative fact. -- **Queue already reconciled** — the clamped decrement bottoms out at zero and the bookmark guard skips equal-or-older candidates. -- **Hook notified, then crash before the terminal write** — retry re-notifies; the greenness identity dedups. -- **Duplicate builds for one Request** — the first Greenness create wins; later builds cannot overwrite it. -- **Request already terminal** — ack. The terminal write is the last durable step, so nothing can be missing. +| Failure | Disposition | Reason | +| ----------------------------------------------- | ------------------------ | ----------------------------------------------------------------------------------------------------------------- | +| Request not found | non-retryable | the publish follows the committed outcome write, so a miss is a storage defect | +| Request carrying no build outcome | non-retryable | producer/state-machine invariant violation | +| Malformed request id at bookmark comparison | non-retryable | re-parsing the same ids cannot succeed | +| Queue CAS version mismatch | retryable at declaration | `storage.ErrVersionMismatch` is declared retryable; return raw, reload, and reapply the idempotent bookmark guard | +| Hooks, ValidationFactStore, QueueStore, RequestStore | raw error | backend classifier has the required failure knowledge | -An existing fact from a different Request or a non-terminal Build is an invariant violation, not an expected control-flow outcome. A Request read as `accepted` is neither: the Build proves admission happened, so it is a stale read that redelivery converges. -## Error classification +## DLQ and fail-closed behavior -Plain errors remain non-retryable by default. Controllers return extension errors raw so the composed backend classifiers decide whether infrastructure failures are retryable. The controller overrides only cases whose meaning is known locally: +`record_dlq` re-runs the same idempotent record algorithm from the request id, under `errs.AlwaysRetryableProcessor`: -| Failure | Disposition | Reason | -| ------------------------------------------------ | ------------- | ----------------------------------------------------------------------- | -| Build not found | retryable | `buildsignal` may have published ahead of a lagging read | -| Request not found | retryable | the Build references an older Request write | -| Request read as `accepted` | retryable | stale pre-admit version of the row; the Build proves admission happened | -| Queue CAS version mismatch | retryable | reload and reapply idempotent reconciliation | -| Request CAS version mismatch | retryable | reload and re-evaluate the state | -| Non-terminal Build or invalid Request state | non-retryable | producer/state-machine invariant violation | -| Hooks, GreennessStore, QueueStore, RequestStore | raw error | backend classifier has the required failure knowledge | +- If the Request carries a build outcome, write and publish its immutable fact as usual. +- If Request storage is temporarily unavailable, keep retrying. +- If the payload is malformed or the Request is permanently missing, the message is poison: there is no trustworthy identity to act on, so it needs an operational alert rather than more retries. -## DLQ and fail-closed behavior +If no build verdict is available, a validation fact is not written. Gating stays safe because an absent fact reads as not green (see [When to record an outcome](#when-to-record-an-outcome)). -The record DLQ must not call the current generic `failRequest`: its payload names a terminal Build, so the actual verdict is already known, and replacing a successful build with a conservative not-green fact would falsify durable history. +## Edge cases -`record_dlq` runs the same idempotent reconciliation algorithm from the Build id under `errs.AlwaysRetryableProcessor`: +- **Head equals last-green.** The build still produces a terminal verdict. A green result may share the bookmark's URI; the guard compares request ids, not URIs, so an equal-or-older candidate skips and nothing regresses. +- **Cancelled build.** Stovepipe never initiates cancellation, but a backend may still report it. No fact is written; the slot was already released and the Request already stamped `cancelled`. The fact identity stays unclaimed, but nothing can claim it today — `cancelled` is a terminal state and re-validation does not exist — so in practice recovery is the next commit; the unclaimed identity only matters to a future re-run mechanism (see [Supporting re-run of same URI](#supporting-re-run-of-same-uri-future-use-case)). +- **History rewrite while a build runs.** The Request keeps the strategy and URI pinned at admission; record stores the fact about that immutable URI. A later head is handled independently by `process`. +- **Late successful result after a fail-closed terminal.** The terminal `failed` Request wins: the fact records degree `1` even though the build passed, because the degree derives from `R.State`. Nothing rewrites greenness or reopens the pipeline. +- **Crash between the fact write and the hook.** Retry reloads the existing fact and re-notifies; the fact identity makes the duplicate safe. +- **Ack fails after a complete run.** Redelivery re-runs every step as a no-op except the hook, which re-fires. -- If the Build is terminal, persist and propagate its actual immutable fact. -- If Build or Request storage is temporarily unavailable, keep retrying. -- If the payload is malformed or the Build is permanently missing, the message is a poison reconciliation item requiring an operational alert; there is no trustworthy Request identity to mutate. +## Phase 2 plans -The Queue slot is released before Hooks, so a broken notification backend cannot wedge validation of newer heads; the Request stays `processing` until reconciliation completes the hook and the terminal write. +Phase 2 can expand upon the record phase: -Earlier-stage DLQ reconciliation still forces a conservative degree `1` when no terminal verdict exists: create the whole-repository fact (with empty `BuildID` — the marker of a conservative rather than observed verdict), release the slot with the same clamped decrement, and transition to `recorded_not_green`. Changing state without writing the fact would leave externally visible greenness absent, failing the fail-closed contract. +- **Pipeline**: `record` publishes the request id onward to `analyze`. It does this for green and not-green facts alike, because a failed build is exactly when project attribution matters most. An earlier draft had `record` retarget the Request from `processing` to `analyzing`, with `analyze` owning the terminal transition. That no longer fits, because the Request is already terminal before `record` runs. So tracking "all facts recorded" belongs to the `analyze` design (see [Completion marker: open](#completion-marker-open)). The message stays id-only and the consumer stays idempotent. +- **Project builds**: `record` runs the same load-fact-notify flow. The fact is keyed by the stable project id carried on the per-project signal, and Hooks is notified with that identity. Each per-project signal needs its own message id (see [Message-queue additions](#message-queue-additions)). The Queue's `last_green_uri` describes the whole repository and stays untouched. -First-fact-wins makes the conservative verdict final, deliberately superseding `workflow.md`'s remark that "a late successful update wins cleanly over the conservative one": a late green for a fail-closed URI is dropped, at bounded cost — the branch keeps moving, and the next head re-establishes greenness on its own Request. Revise `workflow.md` alongside this design. +Other functionality such as deciding project identity, retrieving the target graph, tracking completion, defining intermediate degrees — belongs to `analyze.md`. -## Edge cases +`ValidationFactStore` is key/value-shaped: -- **Head equals last-green.** The build still produces a terminal verdict. A green result may share the bookmark's URI; the guard compares request ids, not URIs, so an equal-or-older candidate skips and nothing regresses. -- **Cancelled build.** Stovepipe never initiates cancellation, but a backend may still report it. Record degree `1`, release the slot, complete the Request. -- **History rewrite while a build runs.** The Request keeps the strategy and URI pinned at admission; record stores the fact about that immutable URI. A later head is handled independently by `process`. -- **Late successful result after fail-closed terminal.** The terminal `recorded_not_green` Request wins; record acks without rewriting greenness or reopening the pipeline. -- **Hook succeeds, terminal CAS interrupted.** Retry re-notifies the hook; the greenness identity makes the duplicate safe. -- **Terminal CAS succeeds, ack fails.** Redelivery reads the recorded state at step 4 and acks. +- `Create(ctx, fact)` writes one immutable fact. It returns `ErrAlreadyExists` when the composite identity is already taken. +- `Get(ctx, queue, uri, project)` reads one fact by its full identity. It returns `ErrNotFound` when no fact exists. The `project` field is reserved for Phase 2. diff --git a/doc/rfc/stovepipe/workflow.md b/doc/rfc/stovepipe/workflow.md index f2b9663f..f982f4a2 100644 --- a/doc/rfc/stovepipe/workflow.md +++ b/doc/rfc/stovepipe/workflow.md @@ -58,7 +58,7 @@ The ref is a *cache* of the last-green URI, not a second record of greenness. It | **Hooks** | Publish Stovepipe's greenness events to downstream systems — "this URI / this project is now green (or not green)". Fire-and-forget notification, decoupled so Stovepipe does not know or care who consumes the event. | | **Storage** | Persist Queues (incl. last-green URI), Requests, build records, and per-URI / per-project greenness. Key/value-shaped per the extension-design rules in [CLAUDE.md](../../../CLAUDE.md). | -The **Hooks** extension is the notification boundary. Whenever a greenness fact is recorded — whole-repo green/not-green, or later a project green/not-green — `record` fires the relevant hook so deployment systems, dashboards, and developer tooling learn about it without polling Stovepipe's store. Hooks are pluggable so each environment can route events to its own downstream (a deploy gate, a Slack notifier, an event bus) without changing the pipeline. +The **Hooks** extension is the notification boundary. Whenever a validation fact is recorded — whole-repo green/not-green, or later a project green/not-green — `record` fires the relevant hook so deployment systems, dashboards, and developer tooling learn about it without polling Stovepipe's store. Hooks are pluggable so each environment can route events to its own downstream (a deploy gate, a Slack notifier, an event bus) without changing the pipeline. ## Workflow @@ -163,7 +163,7 @@ Per-stage design detail lives under `steps/` so this doc stays a pipeline overvi - [process.md](steps/process.md) — build-strategy decision, concurrency gate, backlog coalescing, [concurrency lifecycle](steps/process.md#concurrency-lifecycle), entity changes, [waiting for a slot](steps/process.md#waiting-for-a-slot) - [build.md](steps/build.md) — trigger-only stage: reads the decided scope off the Request, triggers the build-runner, hands off to buildsignal; the stovepipe `BuildRunner` contract and why it differs from SubmitQueue's - [buildsignal.md](steps/buildsignal.md) — the poll loop: hold-based re-poll cadence, target-graph return, per-build partitioning, and the fail-closed handoff to record -- [record.md](steps/record.md) — immutable greenness facts, idempotent Queue-slot release, monotonic last-green advancement, Hooks notification, and the Phase 1 handoff to analyze +- [record.md](steps/record.md) — immutable validation facts, idempotent Queue-slot release, monotonic last-green advancement, Hooks notification, and the Phase 1 handoff to analyze ## Dedup, idempotency, and history rewrites @@ -171,7 +171,7 @@ Ingestion is idempotent on `(Queue, head URI)`, so duplicate poller reports — ## Fail-closed on unprocessable work -Callers gate deployments on greenness, so the dangerous failure is a Request that can never finish and silently leaves a URI with no recorded greenness — indistinguishable, to a naive caller, from "not yet validated". Following SQ's DLQ-reconciliation posture, a Request whose validation can never complete must be driven to a **conservative terminal `failed` outcome** — which whatever records greenness treats as not-green — rather than left non-terminal: gating stays safe (never falsely green), and the pipeline moves on. State writes use optimistic-locking CAS, so a late successful update wins cleanly over the conservative one. See [submitqueue/orchestrator/controller/dlq/README.md](../../../submitqueue/orchestrator/controller/dlq/README.md) for the shared reconcile-only design. +Callers gate deployments on greenness, so the dangerous failure is a Request that can never finish and silently leaves a URI with no recorded greenness — indistinguishable, to a naive caller, from "not yet validated". Following SQ's DLQ-reconciliation posture, a Request whose validation can never complete must be driven to a **conservative terminal `failed` outcome** — which whatever records greenness treats as not-green — rather than left non-terminal: gating stays safe (never falsely green), and the pipeline moves on. That conservative outcome is **final**, not provisional: validation facts are immutable and first-fact-wins, so a late successful result for a fail-closed URI is dropped rather than overwriting recorded history. The cost is bounded — the branch keeps moving, and the next head re-establishes greenness on its own Request. See [record.md](steps/record.md#dlq-and-fail-closed-behavior). See [submitqueue/orchestrator/controller/dlq/README.md](../../../submitqueue/orchestrator/controller/dlq/README.md) for the shared reconcile-only design. ## Open questions From 291c4de7f41acbd67c5b02ee1e0b866995db8883 Mon Sep 17 00:00:00 2001 From: mnoah1 Date: Thu, 13 Aug 2026 15:58:08 +0000 Subject: [PATCH 3/7] docs(stovepipe): align record RFC with the shipped stage and hook framework The record stage landed in #555 while this doc still read as a proposal, so the RFC described a design rather than the code. Re-frames it as-built and corrects the contract details that drifted: ValidationFact carries no Queue field (the queue is the store binding and the leading PK column), Get takes (uri, project), the Record payload carries queue_name beside the request id, degrees are the named DegreeGreen/DegreeBroken constants, LastGreenRequestID is shipped rather than proposed, and the bookmark's CAS conflict is absorbed by the retry loop rather than returned. Adds the two algorithm steps the doc omitted (deserialize, per-queue storage resolution) and the ordering rationale for advancing the bookmark only after the fact is durable. Replaces the inline Hooks design with integration against hook-framework.md, which supersedes it: a HookEvent published to a per-domain hook topic and dispatched to a pluggable extension, not a Notify() call out of the stage. Records where the publish belongs, the event shape, and the decision that `type` names the scope (validation.repository.recorded) rather than the outcome, since scope is the axis consumers cannot route on later. Documents that record_dlq has no consumer even though dead-lettering is enabled on the subscription, so rejected messages accumulate unread. --- doc/rfc/index.md | 2 +- doc/rfc/stovepipe/steps/record.md | 251 +++++++++++++++++++----------- doc/rfc/stovepipe/workflow.md | 14 +- 3 files changed, 170 insertions(+), 97 deletions(-) diff --git a/doc/rfc/index.md b/doc/rfc/index.md index 2dc5552d..4ce71e6e 100644 --- a/doc/rfc/index.md +++ b/doc/rfc/index.md @@ -28,7 +28,7 @@ Design documents and technical proposals, grouped by scope. Shared/cross-cutting - [Process stage](stovepipe/steps/process.md) - Build-strategy decision, per-queue concurrency gate, backlog coalescing, entity model, platform prerequisites - [Build stage](stovepipe/steps/build.md) - Trigger-only stage and Stovepipe's URI-based BuildRunner contract - [Buildsignal stage](stovepipe/steps/buildsignal.md) - Build polling, terminal status persistence, and the handoff to record -- [Record stage](stovepipe/steps/record.md) - Immutable validation facts, Queue coordination, Hooks notification, and the Phase 1 handoff to analyze +- [Record stage](stovepipe/steps/record.md) - Immutable validation facts keyed by `(queue, uri, project)`, monotonic last-green bookmark advancement, and the deferred hook-event and analyze handoffs ## Runway diff --git a/doc/rfc/stovepipe/steps/record.md b/doc/rfc/stovepipe/steps/record.md index 60984456..9afd7374 100644 --- a/doc/rfc/stovepipe/steps/record.md +++ b/doc/rfc/stovepipe/steps/record.md @@ -2,8 +2,10 @@ `record` turns a terminal build outcome into a durable validation fact. -- In Phase 1 it records whole-repository greenness, advances the Queue's last-green bookmark when the result is green, and notifies downstream systems. -- In Phase 2 the same stage records project greenness and notifies downstream systems at project granularity. Mentioned in this doc, but to be expanded on before future implementation. +- In Phase 1 it records whole-repository greenness and advances the Queue's last-green bookmark when the result is green. This is implemented: see [stovepipe/controller/record/record.go](../../../../stovepipe/controller/record/record.go). +- In Phase 2 the same stage records greenness per project rather than per repository. Sketched here, to be expanded before implementation. + +Notifying downstream systems is **not** implemented in either phase. It will ride the cross-domain hook framework rather than a Stovepipe-specific extension; see [Hooks](#hooks). See [workflow.md](../workflow.md) for the complete pipeline, [build.md](build.md) for how builds are created, and [buildsignal.md](buildsignal.md) for the terminal-only handoff into this stage. @@ -11,40 +13,52 @@ See [workflow.md](../workflow.md) for the complete pipeline, [build.md](build.md ## Phase 1 algorithm -For a delivery carrying request id `R`: +For a delivery carrying a `Record` payload: ``` -1. Load Request R. +1. Deserialize the payload into (request id R, queue name Q). + - malformed -> return raw; non-retryable. + +2. Resolve the queue's storage from Q. + - unresolvable -> return raw; non-retryable. A queue with no storage is a + malformed message or a config error, not a transient condition. + +3. Load Request R. - ErrNotFound -> return raw; non-retryable. - other store error -> return raw; the classifier decides. -2. Inspect R.State. +4. If Q is non-empty and disagrees with R.Queue, return a non-retryable error. + The Request is the authoritative binding; Q is a routing hint, empty on + payloads written before the field existed. + +5. Inspect R.State. - succeeded / failed -> continue. This is the entry condition: buildsignal stamps the outcome before it publishes here, and both values are verdicts about the code. - - cancelled -> ack. No fact and no notification: the build decided nothing about the - commit (see "When to record an outcome"). - - superseded -> ack; no fact or notification is written. Unreachable in practice. + - cancelled -> ack. No fact: the build decided nothing about the commit (see + "When to record an outcome"). + - superseded -> ack; no fact is written. Unreachable in practice. - accepted / processing / anything else -> return a non-retryable invariant error. -3. Map R.State to a whole-repository degree and create the ValidationFact keyed by - (R.Queue, R.URI, empty project). +6. Map R.State to a whole-repository degree and create the ValidationFact keyed by + (R.URI, empty project) in the queue-bound fact store. - ErrAlreadyExists -> load and reconcile the existing immutable fact. - other store error -> return raw. -4. If the persisted fact is green, advance the Queue bookmark in one CAS retry loop: +7. If the persisted fact is not green, ack. Otherwise advance the Queue bookmark in a + CAS retry loop: a. If LastGreenRequestID is empty or older than R.ID per entity.CompareRequestID, set - LastGreenURI = R.URI and LastGreenRequestID = R.ID. - b. If no field changes, skip the write. + LastGreenURI = R.URI and LastGreenRequestID = R.ID and write with the version guard. + b. If R.ID is not newer, return without writing. + c. On ErrVersionMismatch, reload and re-evaluate; the guard is idempotent. -5. Notify the Hooks extension with the fact identity. - - return errors raw; the hook backend's classifier decides retryability. - -6. ack. +8. ack. ``` -Every decision after step 3 uses the persisted fact (not the outcome read from this delivery's Request). The first immutable fact controls the Queue bookmark and the Hooks event. +Every decision after step 6 uses the persisted fact, not the outcome read from this delivery's Request. The first immutable fact controls the Queue bookmark, and will control the hook event. + +Once hooks land, the event publish becomes a new step between 7 and 8; see [Hooks](#hooks). -## Validation Fact Recording +## Validation fact recording A validation fact answers "how broken was this scope at this Queue URI?" Its identity is: @@ -54,18 +68,21 @@ A validation fact answers "how broken was this scope at this Queue URI?" Its ide `project` is empty for whole-repository greenness and is a stable project id in Phase 2. -The fact contains: +The `queue` component is the binding of the store the fact lives in, not a field on the entity: storage is resolved per queue (`storage.Factory.For`), so a fact only ever exists inside a queue's store and does not need to name it. + +The column still leads the fact table's primary key — `PRIMARY KEY (queue, uri, project)` — because every domain table in the repo must be shardable by queue: the primary key leads with the queue column and no secondary index leads with anything else, so one queue's rows are unreachable through another queue's binding and every read is a primary-key-prefix scan within a single queue. This is an invariant, not a per-table choice, and `make lint-queue-shard` enforces it. Note that it is not MySQL table partitioning; no schema in the repo uses `PARTITION BY`. +`entity.ValidationFact` therefore carries only the rest of the identity: -| Field | Meaning | -| ----------- | ----------------------------------------------------------------------------------- | -| `Queue` | Stable Queue name that namespaces the validation | -| `URI` | Commit URI under validation | -| `Project` | Empty for the whole repository; stable project id for Phase 2 | -| `Degree` | Health degree in the closed interval `[0, 1]`; `0` is green and `1` is fully broken | -| `RequestID` | Request that established the fact | -| `CreatedAt` | Millisecond timestamp at which the fact was first recorded | +| Field | Meaning | +| ----------- | ------------------------------------------------------------------------------------------------------------------ | +| `URI` | Commit URI under validation | +| `Project` | Empty for the whole repository; stable project id for Phase 2 | +| `Degree` | Health degree in the closed interval `[entity.DegreeGreen, entity.DegreeBroken]` — `0` is green, `1` fully broken | +| `RequestID` | Request that established the fact | +| `CreatedAt` | Millisecond timestamp at which the fact was first recorded | +Greenness is read through `ValidationFact.IsGreen()` rather than compared against a bare literal, and the endpoints are the named constants `entity.DegreeGreen` / `entity.DegreeBroken`. Facts are create-only. `ErrAlreadyExists` on create means one of exactly two things: @@ -78,7 +95,7 @@ Absence remains distinct from degree `0`. Callers gating deployments must treat ### When to record an outcome -A fact is written only when the request reaches a `succeeded` or `failed` verdict. A `cancelled` build is acked with no fact recorded. Callers gating deployments treat an absent fact as not green. +A fact is written only when the request reaches a `succeeded` or `failed` verdict. A `cancelled` build is acked with no fact recorded. Callers gating deployments treat an absent fact as not green. The fail-closed path also produces no fact, as the DLQ reconciler forces `failed` and never publishes to `record`. @@ -86,59 +103,89 @@ The fail-closed path also produces no fact, as the DLQ reconciler forces `failed MVP whole-repository builds use only the endpoints, mapped from the outcome `buildsignal` stamped on the Request: +| Request outcome | Result | +| --------------- | ------------------------------- | +| `succeeded` | fact at `entity.DegreeGreen` | +| `failed` | fact at `entity.DegreeBroken` | +| `cancelled` | no fact | -| Request outcome | Result | -| --------------- | ------------------ | -| `succeeded` | fact at degree `0` | -| `failed` | fact at degree `1` | -| `cancelled` | no fact | +Intermediate degrees are reserved for project analysis and deferred with the project mapping contract; Phase 1 does not manufacture fractional values. +## Last-green advancement (Queue bookmark) -Intermediate degrees are reserved for project analysis and deferred with the project mapping contract; Phase 1 does not manufacture fractional values. +The bookmark only moves forward. `entity.Queue` carries `LastGreenURI` alongside `LastGreenRequestID` — the request id that owns the current URI — plus `InFlightCount` and `LatestRequestID`. On a green fact, step 7 adopts the pair `(R.URI, R.ID)` only when the stored id is empty or older than `R.ID` per `entity.CompareRequestID(R.Queue, …)` — the same ingest-order comparison `ingest` and `process` already use for coalescing. That comparison returns an error on an id that does not match the queue's format; treat it as non-retryable, since re-parsing the same ids cannot succeed. -### Supporting re-run of same URI (future use case) +The write goes through `QueueStore.Update(ctx, queue, oldVersion, newVersion)`, so version arithmetic stays in the controller: it computes `newVersion = oldVersion + 1` and the store performs a pure conditional write. `ErrVersionMismatch` is absorbed by the loop rather than returned — a concurrent writer moved the row, so reloading and re-applying the guard converges. -Widen the key with the `RequestID` they already record so each attempt is its own immutable row, and add a pointer store from `(queue, uri, project)` to the authoritative attempt. Advance to a newer attempt unless the current one is green, since a green build proved the code passed and a later failure only proves the build is non-deterministic. +**Why the bookmark is advanced only after the fact is durable**: the bookmark is a cache of "newest green URI" derived from the facts, so it must never lead them. Losing the advance to a crash is recoverable — the redelivery reloads the same fact and retries — whereas a bookmark with no fact behind it would point at greenness nothing recorded, and `process` would pick a baseline URI that no validation supports. -### Coverage of intermediate commits +A failed or cancelled build never moves the bookmark. -Coalescing means most commits never become a validated Request: a verdict on head `H` with base `B` is implicitly a verdict on every commit in the range `(B, H]`. Downstream tooling still needs to be able to retrieve prior and next green for any commit, including those never ingested directly, or that have been superseded. +## Build slot release -To support that, the rough idea is that we can track this via some additional stores that are written during this record step, which we can expand upon in a separate doc: +`Queue.in_flight_count` is released by `buildsignal` before it stamps the outcome and publishes the record step. The DLQ reconciler releases the slot on the fail-closed path for the same reason. `record`'s only Queue write is the last-green bookmark. -- **`CoverageStore`** — on every verdict, green or failed, one row per commit in the covered range: `(queue, uri)` → the covering request and the commit's position within its range. Gives commits with no Request of their own a place in the queue's history. -- **`GreenLogStore`** — on green verdicts only, one row keyed by that position. Because the key is ordered, "previous green" and "next green" become two seeks: the nearest entry below or above a commit's position. +## Hooks -Neither fits the `ValidationFactStore` proposed above: facts are looked up by exact identity and URIs do not sort, while previous/next-green needs an ordered seek over positions, a different key shape. +Recording a fact is the moment the rest of the company can learn "this URI is now green (or not green)". `record` is the publisher of that event, but it does **not** call a notification extension inline. Delivery rides the cross-domain hook framework in [hook-framework.md](../../hook-framework.md): the stage publishes a `HookEvent` to Stovepipe's durable `hook` topic, and a separate per-domain dispatcher stage consumes it and invokes whichever hooks the host wired. -## Build slot release +Inline notification was the earlier design here — a Stovepipe `Hooks` extension with a `Notify(ctx, ValidationFactRef{…})` call as the last step of the algorithm. The framework rejects both halves of it explicitly: an inline call couples pipeline latency to third-party integrations and drops the notification on a crash between the state write and the call, and a per-domain contract multiplies schemas and sinks for no gain. Nothing in that shape survives except the intent. -`Queue.in_flight_count` is released by `buildsignal` before it stamps the outcome and publishes the record step. The DLQ reconciler releases the slot on the fail-closed path for the same reason. `record`'s only Queue write is the last-green bookmark. +None of this is built. The prerequisites are the shared `HookEvent` contract under `api/base/hook/`, the hook extension at `platform/extension/hook/`, and — Stovepipe's own share — a `hook` topic key, a dispatcher stage, a `hook_dlq` reconciler, and the wiring for all three. + +### Where the publish belongs -## Completion marker: open +The publish is the last thing the delivery does before the ack, after the fact write and after the bookmark advance: -`record` makes no `Request` write in Phase 1, which leaves the stage with no durable marker saying it finished. The consequence is contained today, because every effect is idempotent and a redelivery simply re-runs them, but two things depend on a marker and stay unresolved: +``` +create fact → advance bookmark (green only) → publish HookEvent → [Phase 2: publish to analyze] → ack +``` -- **Redelivery cost.** With no marker to check, a redelivery always re-fires the hook. That is safe under at-least-once delivery with the fact identity as the idempotency key, but it is a real duplicate rather than a skipped no-op. -- **Phase 2 completion.** "All planned facts recorded" needs somewhere to live. An earlier draft inserted a non-terminal `analyzing` state between `processing` and the recorded states; that no longer fits, because `buildsignal` drives the Request terminal before `record` ever runs, leaving no non-terminal window to occupy. Phase 2 must either move the outcome write back behind analysis, add a separate completion entity, or track fan-out on the facts themselves. Deferred to `analyze.md`. +Ordering it after the bookmark matters because the framework forbids entity snapshots in the payload — hooks resolve entities from stores. A hook that reacts to "URI is green" by reading the Queue's `LastGreenURI` must not find a bookmark that still points at the previous commit, so the event cannot precede the write it implies. -## Last-green advancement (Queue bookmark) +Ordering it inside the same delivery is what makes it lossless without an outbox: the state writes are recognize-and-skip on redelivery, so a crash before the ack replays the whole chain and re-publishes. -The bookmark only moves forward. The Queue gains one field, `LastGreenRequestID` — the request id that owns the current `LastGreenURI`. It is a proposed addition; `entity.Queue` today carries `LastGreenURI`, `InFlightCount`, and `LatestRequestID` only. On a green fact, step 4 adopts the pair `(R.URI, R.ID)` only when the stored id is empty or older than `R.ID` per `entity.CompareRequestID(R.Queue, …)` — the same ingest-order comparison `ingest` and `process` already use for coalescing. That comparison returns an error on an id that does not match the queue's format; treat it as non-retryable, since re-parsing the same ids cannot succeed. +### Event shape -A failed or cancelled build never moves the bookmark. +| Envelope field | Value for a validation fact | +| -------------- | --------------------------------------------------------------- | +| `source` | `stovepipe` | +| `type` | `validation.repository.recorded` (see below) | +| `version` | `0` — a fact is create-only and has no version to report | +| `timestamp_ms` | Publish time; the fact's own `CreatedAt` travels in the payload | +| `id` | `source` / `type` / request id / `0` | -## Hooks +The **subject** is the Request. It is not an envelope field — the framework keeps subject, queue, and error in the payload deliberately — but it is what the `id` is minted from and what the event is partitioned by. Choosing the Request over the URI keeps partitioning identical to the `record` topic's own, so per-request ordering carries through the seam unbroken, and gives a consumer the handle it needs to trace an event back into the pipeline. The two are near-interchangeable in practice, since ingest dedups on `(queue, uri)` and one Request means one URI. -After a validation fact is recorded, hooks will be fired to notify downstream consumers of the greenness change. +The payload carries the fact's full identity and value — `queue`, `uri`, `project`, `degree`, `request_id`. `queue` has to be there because the envelope deliberately has no queue field and the fact entity does not carry one either, so the event is the only place a cross-queue consumer can see it. `degree` is included even though a hook could resolve it from the store: a fact is immutable, so the usual staleness objection to putting values on an event does not apply, and it is the field a consumer branches on to tell green from broken — the type does not carry that, for the reasons below. -The Hooks contract takes a thin fact identity, and implementations may resolve additional details as needed: +Build failure detail stays off the event. The framework reserves the payload for facts persisted nowhere else, and a failed build's detail is durable on the `Build` row — a hook that wants it resolves the Request and the Build. -``` -Notify(ctx, ValidationFactRef{Queue, URI, Project}) error -``` +### What the `type` carries -Delivery is at-least-once with the fact identity as the idempotency key; hook implementations or their downstreams must absorb duplicates. "Fire-and-forget" refers to downstream consumption, not the publish itself: `record` never waits for consumers to act on an event, but a failed `Notify` fails the delivery and is retried. The Request is already terminal and its slot already released by then, so a stuck hook delays the notification without holding up the pipeline. +`type` is the only dimension a consumer can use to avoid *receiving* an event: the framework rejected a filter API on the grounds that returning nil costs nothing, and defers per-hook isolation to consumer groups on the same topic key. It is also a component of `id`, so whatever goes in it becomes part of the dedupe key. Everything a consumer needs in order to *interpret* an event it already has can be a payload field instead, which is add-only and costs one read. + +Two axes compete for that slot. **Outcome** — green versus broken — is bounded, cheap to branch on, and stops being binary in Phase 2 once a project degree can be fractional and "acceptable" becomes a per-consumer threshold rather than a property of the fact. **Scope** — whole repository versus project — is bounded too, but carries a volume asymmetry of orders of magnitude: one repository fact per commit against one per project per commit. A third candidate, the project id itself, can never be a type at all, since its cardinality is unbounded and caller-defined; project-level consumers filter on the payload whatever the vocabulary looks like. + +That yields the rule: **an axis belongs in the `type` when consumers need to avoid receiving the event, and in the payload when they need to interpret it.** Scope decides whether an event is wanted at all; degree decides what to do with one already in hand. + +So the type names the scope rather than the outcome — `validation.repository.recorded` in Phase 1, with `validation.project.recorded` beside it in Phase 2. A deploy gate reads `degree` from the payload, which it would have to do regardless once degrees are fractional. Naming the scope while only one of them exists is deliberate: it is the axis that cannot be recovered later, and it keeps Phase 2 from retroactively reinterpreting a bare `validation.green` as "the repository one". + +This also keeps `id` computable from the request id alone, since it no longer depends on the fact's value — an operator republishing from a dead-letter queue never has to read storage first to learn which id they would be duplicating. The rejected alternative, `validation.green` / `validation.broken`, is the shape the framework's own examples take (`request.landed`, `batch.failed`), so it is worth confirming with the framework's owner that a scope-shaped vocabulary is acceptable across domains before the first consumer binds to it. + +### Open questions + +**Subject id and the separator rule.** The framework mints `id` from `source` / `type` / subject id / version and says components are separator-free, but every Stovepipe id has slashes in it — a request id is `request//`. Since the framework also says consumers never parse the id, the requirement that actually matters is uniqueness rather than unambiguous decomposition, and uniqueness holds: one request records exactly one whole-repository fact. Worth confirming with the framework's owner rather than assuming, and worth deciding whether the alternative — hashing the composite identity into a single opaque component — is preferable for consistency across domains. + +**Phase 2 fan-out.** One request records N project facts, so the request id alone stops being a unique subject per event. The framework anticipates this case and appends an ordinal for multiple same-typed events per cause; the natural ordinal here is the project id, which is already the part of the identity that varies. Confirm when the project contract lands. + +### What a consumer can and cannot assume + +Ordering is per-subject only, and the subject is the request. Events for *different* requests can therefore arrive out of order, so a consumer must not infer "the newest green commit" from arrival order — it should compare request ids by ingest order (`entity.CompareRequestID`) or read the Queue's bookmark, which is monotonic by construction. + +Absence of an event is not a signal. A cancelled build records no fact and publishes nothing, and the fail-closed path never reaches this stage at all, so a consumer waiting for an event per ingested commit will wait forever on those. Gating has to keep treating "no recorded fact" as not green. + +Delivery is at-least-once, deduped by `id` within the queue's retention window; hooks must be idempotent on `id`. "Fire-and-forget" describes downstream consumption, not the publish: `record` never waits for a hook to run, but a failed *publish* fails the delivery. Per [`platform/errs`](../../../../platform/errs/README.md) rule 4 that failure is not wrapped retryable just because replaying it is convenient, so it dead-letters — which is the point at which the missing `record_dlq` reconciler below stops being theoretical, since the fact is already durable and only the notification is lost. ## Request lifecycle @@ -146,15 +193,22 @@ Phase 1 uses the states in [stovepipe/entity/request.go](../../../../stovepipe/e Phase 2 broadens "complete" to mean "all planned facts recorded", which needs a marker this stage does not own — see [Completion marker: open](#completion-marker-open). -## Message-queue additions +## Storage and queue contract -The topic key and message already exist in `stovepipe/core/messagequeue`; only the consumer is outstanding. +`ValidationFactStore` is key/value-shaped, and queue-bound rather than queue-parameterized: +- `Create(ctx, fact)` writes one immutable fact for the bound queue. It returns `ErrAlreadyExists` when the composite identity is already taken, leaving the stored fact untouched. +- `Get(ctx, uri, project)` reads one fact by the rest of its identity. It returns `ErrNotFound` when no fact exists. The `project` argument is reserved for Phase 2. -| Topic key | Message | Producer | Consumer | Partition key | Message id | -| --------- | ------------------------- | ------------- | -------- | ------------- | ---------- | -| `record` | `Record{id}` (request id) | `buildsignal` | `record` | Request id | Request id | +There is no `Update`: the first fact written for an identity is the permanent answer, and a caller that needs to know whether it won the race reads `ErrAlreadyExists` and then loads the winner. +The topic key, the message, and the consumer all exist; the DLQ consumer does not (see [DLQ and fail-closed behavior](#dlq-and-fail-closed-behavior)). + +| Topic key | Message | Producer | Consumer | Partition key | Message id | +| --------- | ---------------------------------- | ------------- | -------- | ------------- | ---------- | +| `record` | `Record{id, queue_name}` | `buildsignal` | `record` | Request id | Request id | + +The payload carries the request id plus the queue name, so the consumer can resolve per-queue storage before it loads any state. `queue_name` is empty on payloads written before the field existed, which is why step 4 only enforces the match when it is set. Partitioning by request id keeps completion bookkeeping single-writer per Request, and reusing the request id as the message id dedups a redelivered signal into the original message instead of enqueuing a second one. @@ -163,53 +217,72 @@ Partitioning by request id keeps completion bookkeeping single-writer per Reques - **Request not visible** — a storage defect rather than a lag, since the publish follows the committed outcome write. Non-retryable. - **Fact already created** — load it and continue from the authoritative fact. - **Bookmark already advanced** — the guard skips equal-or-older candidates and the write is skipped. -- **Hook notified, then crash** — retry re-notifies; the fact identity dedups. - **Duplicate builds for one Request** — absorbed a stage earlier: `buildsignal`'s outcome write is first-writer-wins, so the Request carries one immutable verdict and `record` never sees a competing one. -- **Redelivery after a complete run** — every effect is recognize-and-skip except the hook, which re-fires (see [Completion marker: open](#completion-marker-open)). +- **Redelivery after a complete run** — every effect is recognize-and-skip. Once hooks land the publish re-fires, and the framework's dedupe on `id` absorbs it. An existing fact from a different Request, or a Request carrying no build outcome, is an invariant violation rather than an expected control-flow outcome. ## Error classification +| Failure | Disposition | Reason | +| ---------------------------------------------------- | -------------------- | --------------------------------------------------------------------------------- | +| Malformed `Record` payload | non-retryable | a malformed message will never succeed regardless of retries | +| Queue name that resolves to no storage | non-retryable | malformed message or missing config, not a transient condition | +| Payload queue disagreeing with the Request's queue | non-retryable | malformed message; the Request is the authoritative binding | +| Request not found | non-retryable | the publish follows the committed outcome write, so a miss is a storage defect | +| Request carrying no build outcome | non-retryable | producer/state-machine invariant violation | +| Existing fact owned by a different Request | non-retryable | ingest dedup invariant violated; the stored fact is immutable | +| Malformed request id at bookmark comparison | non-retryable | re-parsing the same ids cannot succeed | +| Queue CAS version mismatch | absorbed, not raised | `storage.ErrVersionMismatch` is handled by the bookmark loop: reload and re-apply | +| ValidationFactStore, QueueStore, RequestStore | raw error | backend classifier has the required failure knowledge | -| Failure | Disposition | Reason | -| ----------------------------------------------- | ------------------------ | ----------------------------------------------------------------------------------------------------------------- | -| Request not found | non-retryable | the publish follows the committed outcome write, so a miss is a storage defect | -| Request carrying no build outcome | non-retryable | producer/state-machine invariant violation | -| Malformed request id at bookmark comparison | non-retryable | re-parsing the same ids cannot succeed | -| Queue CAS version mismatch | retryable at declaration | `storage.ErrVersionMismatch` is declared retryable; return raw, reload, and reapply the idempotent bookmark guard | -| Hooks, ValidationFactStore, QueueStore, RequestStore | raw error | backend classifier has the required failure knowledge | +## DLQ and fail-closed behavior +**There is no `record_dlq` consumer today, and the topic it would consume is already receiving messages.** The record subscription is created from `DefaultSubscriptionConfig`, which enables dead-lettering with the `_dlq` suffix, so a rejected record message is moved to `record_dlq` — but the wiring registers only `process_dlq` and `buildsignal_dlq`, and no controller subscribes to `record_dlq`. Messages accumulate there unread. -## DLQ and fail-closed behavior +Gating stays safe, because everything this stage can lose reads as not-green: a request with no fact is indistinguishable from one not yet validated, and callers must already treat absence as not green (see [When to record an outcome](#when-to-record-an-outcome)). What is lost instead is the *fact* — a green build whose fact write permanently failed leaves the URI looking unvalidated, which costs the queue an incremental baseline and forces a full build at the next head. Once hooks land, a lost notification joins that list, and unlike the fact it has no second chance from a later commit. -`record_dlq` re-runs the same idempotent record algorithm from the request id, under `errs.AlwaysRetryableProcessor`: +This is the same failure shape [buildsignal.md](buildsignal.md#what-it-costs-when-a-backend-does-not-classify-status-errors) describes for a deployment that registers primary consumers without their reconciler. When the reconciler is built, it should re-run the same idempotent record algorithm from the request id under `errs.AlwaysRetryableProcessor`: - If the Request carries a build outcome, write and publish its immutable fact as usual. - If Request storage is temporarily unavailable, keep retrying. - If the payload is malformed or the Request is permanently missing, the message is poison: there is no trustworthy identity to act on, so it needs an operational alert rather than more retries. -If no build verdict is available, a validation fact is not written. Gating stays safe because an absent fact reads as not green (see [When to record an outcome](#when-to-record-an-outcome)). - ## Edge cases - **Head equals last-green.** The build still produces a terminal verdict. A green result may share the bookmark's URI; the guard compares request ids, not URIs, so an equal-or-older candidate skips and nothing regresses. -- **Cancelled build.** Stovepipe never initiates cancellation, but a backend may still report it. No fact is written; the slot was already released and the Request already stamped `cancelled`. The fact identity stays unclaimed, but nothing can claim it today — `cancelled` is a terminal state and re-validation does not exist — so in practice recovery is the next commit; the unclaimed identity only matters to a future re-run mechanism (see [Supporting re-run of same URI](#supporting-re-run-of-same-uri-future-use-case)). +- **Cancelled build.** Stovepipe never initiates cancellation, but a backend may still report it. No fact is written; the slot was already released and the Request already stamped `cancelled`. The fact identity stays unclaimed, but nothing can claim it today — `cancelled` is a terminal state and re-validation does not exist — so in practice recovery is the next commit; the unclaimed identity only matters to a future re-run mechanism (see [Supporting re-run of the same URI](#supporting-re-run-of-the-same-uri)). - **History rewrite while a build runs.** The Request keeps the strategy and URI pinned at admission; record stores the fact about that immutable URI. A later head is handled independently by `process`. -- **Late successful result after a fail-closed terminal.** The terminal `failed` Request wins: the fact records degree `1` even though the build passed, because the degree derives from `R.State`. Nothing rewrites greenness or reopens the pipeline. -- **Crash between the fact write and the hook.** Retry reloads the existing fact and re-notifies; the fact identity makes the duplicate safe. -- **Ack fails after a complete run.** Redelivery re-runs every step as a no-op except the hook, which re-fires. +- **Late successful result after a fail-closed terminal.** The terminal `failed` Request wins: the fact records `DegreeBroken` even though the build passed, because the degree derives from `R.State`. Nothing rewrites greenness or reopens the pipeline. +- **Crash between the fact write and the bookmark advance.** Redelivery reloads the existing fact and re-applies the idempotent guard. +- **Ack fails after a complete run.** Redelivery re-runs every step as a no-op. + +## Deferred and open + +### Completion marker: open + +`record` makes no `Request` write in Phase 1, which leaves the stage with no durable marker saying it finished. The consequence is contained today, because every effect is idempotent and a redelivery simply re-runs them, but Phase 2 needs a marker: "all planned facts recorded" has to live somewhere. An earlier draft inserted a non-terminal `analyzing` state between `processing` and the recorded states; that no longer fits, because `buildsignal` drives the Request terminal before `record` ever runs, leaving no non-terminal window to occupy. Phase 2 must either move the outcome write back behind analysis, add a separate completion entity, or track fan-out on the facts themselves. Deferred to `analyze.md`. + +One argument for a marker has since gone away: the worry that a redelivery with nothing to check against would re-fire the notification every time. The hook framework answers it directly, deduping on the event `id` so a replay collapses into the original event rather than becoming a second one. + +### Supporting re-run of the same URI -## Phase 2 plans +Widen the key with the `RequestID` the facts already record, so each attempt is its own immutable row, and add a pointer store from `(queue, uri, project)` to the authoritative attempt. Advance to a newer attempt unless the current one is green, since a green build proved the code passed and a later failure only proves the build is non-deterministic. -Phase 2 can expand upon the record phase: +### Coverage of intermediate commits + +Coalescing means most commits never become a validated Request: a verdict on head `H` with base `B` is implicitly a verdict on every commit in the range `(B, H]`. Downstream tooling still needs to be able to retrieve prior and next green for any commit, including those never ingested directly, or that have been superseded. + +The rough idea is to track this with additional stores written during this stage, to be expanded in a separate doc: + +- **`CoverageStore`** — on every verdict, green or failed, one row per commit in the covered range: `(queue, uri)` → the covering request and the commit's position within its range. Gives commits with no Request of their own a place in the queue's history. +- **`GreenLogStore`** — on green verdicts only, one row keyed by that position. Because the key is ordered, "previous green" and "next green" become two seeks: the nearest entry below or above a commit's position. -- **Pipeline**: `record` publishes the request id onward to `analyze`. It does this for green and not-green facts alike, because a failed build is exactly when project attribution matters most. An earlier draft had `record` retarget the Request from `processing` to `analyzing`, with `analyze` owning the terminal transition. That no longer fits, because the Request is already terminal before `record` runs. So tracking "all facts recorded" belongs to the `analyze` design (see [Completion marker: open](#completion-marker-open)). The message stays id-only and the consumer stays idempotent. -- **Project builds**: `record` runs the same load-fact-notify flow. The fact is keyed by the stable project id carried on the per-project signal, and Hooks is notified with that identity. Each per-project signal needs its own message id (see [Message-queue additions](#message-queue-additions)). The Queue's `last_green_uri` describes the whole repository and stays untouched. +Neither fits `ValidationFactStore`: facts are looked up by exact identity and URIs do not sort, while previous/next-green needs an ordered seek over positions, a different key shape. A genuinely needed reverse lookup getting its own first-class store is the established pattern here — `RequestURIStore` is the existing example (see [storage README](../../../../stovepipe/extension/storage/README.md#key-value-contract)). -Other functionality such as deciding project identity, retrieving the target graph, tracking completion, defining intermediate degrees — belongs to `analyze.md`. +### Phase 2 -`ValidationFactStore` is key/value-shaped: +- **Pipeline**: `record` publishes the request id onward to `analyze`. It does this for green and not-green facts alike, because a failed build is exactly when project attribution matters most. An earlier draft had `record` retarget the Request from `processing` to `analyzing`, with `analyze` owning the terminal transition. That no longer fits, because the Request is already terminal before `record` runs. So tracking "all facts recorded" belongs to the `analyze` design (see [Completion marker: open](#completion-marker-open)). The message onward carries the request id and queue name, exactly as `record`'s own does, and the consumer stays idempotent. +- **Project facts**: `record` runs the same create-fact-then-notify flow. The fact is keyed by the stable project id carried on the per-project signal, and one `validation.project.recorded` event is published per project identity (see [What the `type` carries](#what-the-type-carries)). Each per-project signal needs its own message id (see [Storage and queue contract](#storage-and-queue-contract)). The Queue's `last_green_uri` describes the whole repository and stays untouched. -- `Create(ctx, fact)` writes one immutable fact. It returns `ErrAlreadyExists` when the composite identity is already taken. -- `Get(ctx, queue, uri, project)` reads one fact by its full identity. It returns `ErrNotFound` when no fact exists. The `project` field is reserved for Phase 2. +Deciding project identity, retrieving the target graph, tracking completion, and defining intermediate degrees belong to `analyze.md`. diff --git a/doc/rfc/stovepipe/workflow.md b/doc/rfc/stovepipe/workflow.md index f982f4a2..ba03a58d 100644 --- a/doc/rfc/stovepipe/workflow.md +++ b/doc/rfc/stovepipe/workflow.md @@ -55,10 +55,10 @@ The ref is a *cache* of the last-green URI, not a second record of greenness. It |---|---| | **SourceControl** | Resolve a Queue name to its current head URI; answer ancestry/comparison questions between two URIs (is the new head a fast-forward descendant of the last green, or was history rewritten?); enumerate commits in a range; advance the Queue's **promotion ref** to a commit. The sole owner of URI semantics, including which refs a Queue name resolves to. | | **build-runner** | Build a scope at a URI (optionally relative to a baseline URI), returning pass/fail and the target graph. See [build-runner.md](../submitqueue/build-runner.md). | -| **Hooks** | Publish Stovepipe's greenness events to downstream systems — "this URI / this project is now green (or not green)". Fire-and-forget notification, decoupled so Stovepipe does not know or care who consumes the event. | +| **Hooks** | Deliver Stovepipe's greenness events to downstream systems — "this URI / this project is now green (or not green)". Fire-and-forget notification, decoupled so Stovepipe does not know or care who consumes the event. Not implemented yet; it will be the shared cross-domain hook seam rather than a Stovepipe-specific extension. See [hook-framework.md](../hook-framework.md). | | **Storage** | Persist Queues (incl. last-green URI), Requests, build records, and per-URI / per-project greenness. Key/value-shaped per the extension-design rules in [CLAUDE.md](../../../CLAUDE.md). | -The **Hooks** extension is the notification boundary. Whenever a validation fact is recorded — whole-repo green/not-green, or later a project green/not-green — `record` fires the relevant hook so deployment systems, dashboards, and developer tooling learn about it without polling Stovepipe's store. Hooks are pluggable so each environment can route events to its own downstream (a deploy gate, a Slack notifier, an event bus) without changing the pipeline. +Hooks are the notification boundary. When a validation fact is recorded — whole-repo green/not-green, or later a project green/not-green — the event reaches deployment systems, dashboards, and developer tooling without any of them polling Stovepipe's store, and each environment can route it to its own downstream (a deploy gate, a Slack notifier, an event bus) without changing the pipeline. The mechanism is the cross-domain hook framework rather than a call out of the recording stage: `record` publishes a `HookEvent` to Stovepipe's `hook` topic, and a dispatcher stage consumes it and invokes the wired hooks, so a slow or failing downstream cannot add latency to the pipeline. Neither half exists yet; see [record.md](steps/record.md#hooks) for the fact-to-event mapping and its open questions. ## Workflow @@ -125,7 +125,7 @@ The pipeline runs in two phases against the same Request. **Phase 1** establishe │ ┌──────────────────────────────┐ Hooks └───────────────────►│ record │┄┄┄┄┄► "project P │ Capture per-project greenness │ green / not - │ for the URI; fire Hooks │ green at URI" + │ for the URI; hook event │ green at URI" └──────────────────────────────┘ ``` @@ -135,13 +135,13 @@ The pipeline runs in two phases against the same Request. **Phase 1** establishe 2. **process** — decides build strategy (incremental since last-green vs full monorepo), gates concurrent work per Queue, coalesces backlog to the latest head, and publishes to `build`. See [process.md](steps/process.md). 3. **build** — runs the build-runner for the chosen scope. A flag derived from `process` decides whether to build relative to the last-green **baseline URI** (incremental) or from scratch (full). It records a build and publishes the BuildID. 4. **buildsignal** — records the build's status and target graph when the build completes, then releases the Queue's `in_flight_count` slot, projects the terminal status onto the Request (`succeeded` / `failed` / `cancelled`), and publishes the RequestID to `record`. -5. **record** — writes the whole-repo greenness for the head URI (`0` green / `1` broken to start), derived from the Request's build outcome. On green it advances the Queue's **last-green URI** so the next `process` can build incrementally from here, and asks `SourceControl` to advance the Queue's **promotion ref** to the same commit (see [Promotion ref](#promotion-ref)). It fires the **Hooks** extension with the green/not-green event, then fans out into Phase 2. The Queue's `in_flight_count` was already released by `buildsignal` when the build went terminal. +5. **record** — writes the whole-repo greenness for the head URI (`0` green / `1` broken to start), derived from the Request's build outcome. On green it advances the Queue's **last-green URI** so the next `process` can build incrementally from here, and asks `SourceControl` to advance the Queue's **promotion ref** to the same commit (see [Promotion ref](#promotion-ref--the-last-green-commit-by-name)). It publishes a **hook event** for the green/not-green transition, then fans out into Phase 2. The Queue's `in_flight_count` was already released by `buildsignal` when the build went terminal. ### Phase 2 — project greenness 6. **analyze** (project-analysis) — takes the build's target graph and maps the relevant targets to **projects**, using whatever implementer-specific mapping is configured. It decides which project-scoped builds / CI jobs are needed to attribute breakage to specific projects, and publishes those builds. 7. **build → buildsignal** — the project-scoped CI job runs; its artifacts are stored in a blob store (e.g. TerraBlob), and `buildsignal` reads back the status. This is the same machinery as Phase 1, reused at project granularity. -8. **record** — captures **per-project greenness for the URI** — for each project, green or not at this commit — and fires **Hooks** per project. This is what lets a caller ask "is project P green at URI U?" and "what is the latest URI where project P is green?". +8. **record** — captures **per-project greenness for the URI** — for each project, green or not at this commit — and publishes one hook event per project. This is what lets a caller ask "is project P green at URI U?" and "what is the latest URI where project P is green?". `record` appearing twice is intentional: it is one re-entrant stage that records greenness at whatever granularity the current phase produced and notifies downstream. The Request is *complete* when every planned granularity has been recorded, not at a single terminal hop. @@ -153,7 +153,7 @@ The pipeline runs in two phases against the same Request. **Phase 1** establishe | **process** | RequestID | build | Build strategy, concurrency gate, backlog coalescing → [process.md](steps/process.md) | | **build** | RequestID | buildsignal | Run the build-runner for the chosen scope; baseline = last-green URI iff incremental | | **buildsignal** | BuildID | record (P1), record (P2) | Record build status + target graph; release `in_flight_count`; project the outcome onto the Request; signal completion | -| **record** | RequestID | analyze (P1→P2), Hooks | Write greenness; on whole-repo green advance last-green URI and the promotion ref; fire Hooks | +| **record** | RequestID | analyze (P1→P2), hook topic | Write greenness; on whole-repo green advance last-green URI and the promotion ref; publish the hook event | | **analyze** | RequestID | build | Map broken/at-risk targets → projects; decide project-scoped builds | ## Step RFCs @@ -163,7 +163,7 @@ Per-stage design detail lives under `steps/` so this doc stays a pipeline overvi - [process.md](steps/process.md) — build-strategy decision, concurrency gate, backlog coalescing, [concurrency lifecycle](steps/process.md#concurrency-lifecycle), entity changes, [waiting for a slot](steps/process.md#waiting-for-a-slot) - [build.md](steps/build.md) — trigger-only stage: reads the decided scope off the Request, triggers the build-runner, hands off to buildsignal; the stovepipe `BuildRunner` contract and why it differs from SubmitQueue's - [buildsignal.md](steps/buildsignal.md) — the poll loop: hold-based re-poll cadence, target-graph return, per-build partitioning, and the fail-closed handoff to record -- [record.md](steps/record.md) — immutable validation facts, idempotent Queue-slot release, monotonic last-green advancement, Hooks notification, and the Phase 1 handoff to analyze +- [record.md](steps/record.md) — turning a terminal build outcome into an immutable validation fact, monotonic last-green advancement, and the deferred hook and analyze handoffs ## Dedup, idempotency, and history rewrites From 74f0a48c314bf508babe83f26cc398d2da1ed30d Mon Sep 17 00:00:00 2001 From: mnoah1 Date: Thu, 13 Aug 2026 16:18:35 +0000 Subject: [PATCH 4/7] docs(stovepipe): document ref promotion and correct the fail-closed guarantee Rebasing brought ref promotion and the two commit-timestamp metrics into the record stage, none of which the RFC described: promotion is gated on holding the last-green bookmark so an older green commit cannot drag the ref backwards, and the equal-id case reports holding it so a crash before promoting is recoverable. Also corrects an overstated claim. The fail-closed path was documented as never reaching record, but buildsignal publishes unconditionally once a build goes terminal, outside the guard that makes its state write idempotent. A request forced to failed by buildsignal_dlq reconciliation can therefore be carried into a permanent broken fact by a surviving poll chain that observed success. Records the trace, notes that build_dlq is orphaned alongside record_dlq, and opens the question of whether observed success should outrank a forced failure. --- doc/rfc/index.md | 2 +- doc/rfc/stovepipe/steps/record.md | 118 ++++++++++++++++++++++++------ doc/rfc/stovepipe/workflow.md | 4 +- 3 files changed, 100 insertions(+), 24 deletions(-) diff --git a/doc/rfc/index.md b/doc/rfc/index.md index 4ce71e6e..aa4f4131 100644 --- a/doc/rfc/index.md +++ b/doc/rfc/index.md @@ -28,7 +28,7 @@ Design documents and technical proposals, grouped by scope. Shared/cross-cutting - [Process stage](stovepipe/steps/process.md) - Build-strategy decision, per-queue concurrency gate, backlog coalescing, entity model, platform prerequisites - [Build stage](stovepipe/steps/build.md) - Trigger-only stage and Stovepipe's URI-based BuildRunner contract - [Buildsignal stage](stovepipe/steps/buildsignal.md) - Build polling, terminal status persistence, and the handoff to record -- [Record stage](stovepipe/steps/record.md) - Immutable validation facts keyed by `(queue, uri, project)`, monotonic last-green bookmark advancement, and the deferred hook-event and analyze handoffs +- [Record stage](stovepipe/steps/record.md) - Immutable validation facts keyed by `(queue, uri, project)`, monotonic last-green bookmark advancement and ref promotion, and the deferred hook-event and analyze handoffs ## Runway diff --git a/doc/rfc/stovepipe/steps/record.md b/doc/rfc/stovepipe/steps/record.md index 9afd7374..8e38a554 100644 --- a/doc/rfc/stovepipe/steps/record.md +++ b/doc/rfc/stovepipe/steps/record.md @@ -2,14 +2,14 @@ `record` turns a terminal build outcome into a durable validation fact. -- In Phase 1 it records whole-repository greenness and advances the Queue's last-green bookmark when the result is green. This is implemented: see [stovepipe/controller/record/record.go](../../../../stovepipe/controller/record/record.go). +- In Phase 1 it records whole-repository greenness and, when the result is green, advances the Queue's last-green bookmark and promotes the commit onto the Queue's promotion ref. This is implemented: see [stovepipe/controller/record/record.go](../../../../stovepipe/controller/record/record.go). - In Phase 2 the same stage records greenness per project rather than per repository. Sketched here, to be expanded before implementation. Notifying downstream systems is **not** implemented in either phase. It will ride the cross-domain hook framework rather than a Stovepipe-specific extension; see [Hooks](#hooks). See [workflow.md](../workflow.md) for the complete pipeline, [build.md](build.md) for how builds are created, and [buildsignal.md](buildsignal.md) for the terminal-only handoff into this stage. -`record` owns persistence and publication of validation facts. It does not decide build scope, poll a build, release the Queue's build slot, interpret a target graph, or map targets to projects. Those responsibilities belong to `process`, `buildsignal`, and `analyze`. +`record` owns persistence and publication of validation facts, plus the two caches derived from them: the last-green bookmark and the promotion ref. It does not decide build scope, poll a build, release the Queue's build slot, interpret a target graph, or map targets to projects. Those responsibilities belong to `process`, `buildsignal`, and `analyze`. ## Phase 1 algorithm @@ -44,19 +44,32 @@ For a delivery carrying a `Record` payload: - ErrAlreadyExists -> load and reconcile the existing immutable fact. - other store error -> return raw. -7. If the persisted fact is not green, ack. Otherwise advance the Queue bookmark in a - CAS retry loop: +7. If the persisted fact is not green, ack — reporting how long the break went + undetected, but only if step 6 is the write that created the fact (see + "Observability"). Otherwise advance the Queue bookmark in a CAS retry loop, which + also reports whether R holds the bookmark afterwards: a. If LastGreenRequestID is empty or older than R.ID per entity.CompareRequestID, set - LastGreenURI = R.URI and LastGreenRequestID = R.ID and write with the version guard. - b. If R.ID is not newer, return without writing. - c. On ErrVersionMismatch, reload and re-evaluate; the guard is idempotent. - -8. ack. + LastGreenURI = R.URI and LastGreenRequestID = R.ID and write with the version + guard. R holds the bookmark. + b. If R.ID equals the stored id, R already set it on an earlier delivery. R holds the + bookmark; no write. + c. If R.ID is older, a later green commit holds the bookmark. No write, and R does + not hold it. + d. On ErrVersionMismatch, reload and re-evaluate; the guard is idempotent. + +8. If R does not hold the bookmark, ack: the commit that does hold it also owns the + promotion ref. Otherwise ask SourceControl to point the Queue's promotion ref at + R.URI. + - ErrNotFound -> count and ack. A rewritten history dropped the commit from the + ref, and no retry can promote it. + - other error -> return raw. + +9. ack. ``` -Every decision after step 6 uses the persisted fact, not the outcome read from this delivery's Request. The first immutable fact controls the Queue bookmark, and will control the hook event. +Every decision after step 6 uses the persisted fact, not the outcome read from this delivery's Request. The first immutable fact controls the Queue bookmark and the promotion ref, and will control the hook event. -Once hooks land, the event publish becomes a new step between 7 and 8; see [Hooks](#hooks). +Once hooks land, the event publish becomes a new step between 8 and 9; see [Hooks](#hooks). ## Validation fact recording @@ -97,7 +110,7 @@ Absence remains distinct from degree `0`. Callers gating deployments must treat A fact is written only when the request reaches a `succeeded` or `failed` verdict. A `cancelled` build is acked with no fact recorded. Callers gating deployments treat an absent fact as not green. -The fail-closed path also produces no fact, as the DLQ reconciler forces `failed` and never publishes to `record`. +The fail-closed path is subtler than "no fact". A DLQ reconciler forces the Request to `failed` and does not itself publish here, so reconciliation *alone* records nothing — but that does not mean nothing is recorded. A `buildsignal` delivery still in flight can reach this stage afterwards and will then write `DegreeBroken` from the forced state. See [What fail-closed actually guarantees](#what-fail-closed-actually-guarantees). ### Phase 1 degree mapping @@ -119,7 +132,32 @@ The write goes through `QueueStore.Update(ctx, queue, oldVersion, newVersion)`, **Why the bookmark is advanced only after the fact is durable**: the bookmark is a cache of "newest green URI" derived from the facts, so it must never lead them. Losing the advance to a crash is recoverable — the redelivery reloads the same fact and retries — whereas a bookmark with no fact behind it would point at greenness nothing recorded, and `process` would pick a baseline URI that no validation supports. -A failed or cancelled build never moves the bookmark. +A failed or cancelled build never moves the bookmark, and therefore never promotes either. + +## Promotion ref + +A green commit is also **promoted**: `record` asks `SourceControl` to point the Queue's promotion ref — a stable branch name like `verified-main` — at the commit. That is the pull-shaped counterpart to the hook event's push, letting a deploy gate or cache warmer resolve a name and know nothing about Stovepipe, URIs, or degrees. See [Promotion ref](../workflow.md#promotion-ref--the-last-green-commit-by-name) in the pipeline overview. + +The stage names only the commit. Which ref a Queue promotes to, and whether it has one at all, is integrator configuration injected into the `SourceControl` implementation alongside the endpoint and credentials, so a backend with no promotion target configured makes `Promote` a no-op. + +**Promotion is gated on holding the bookmark.** `advanceLastGreen` reports whether the Request holds the bookmark after the CAS loop, and only the holder promotes. Without that gate an older green commit recording late would drag the ref backwards, because the ref has no version guard of its own — `Promote` lands the URI it is given rather than refusing a non-descendant, so monotonicity has to be enforced by the caller. Deciding it from the bookmark rather than a fresh comparison means both caches move under one already-serialized decision. + +The equal case matters as much as the newer one: a redelivery that finds its own id already on the bookmark reports that it *holds* it rather than skipping, so the promotion behind it is retried. That is what makes a crash between the bookmark write and the promotion recoverable, and it is safe because `Promote` is idempotent — promoting the URI the ref already points at changes nothing. + +Promotion follows the durable fact for the same reason the bookmark does: the ref is a cache of the facts and must never lead them. + +`ErrNotFound` from `Promote` means a rewritten history dropped the commit from the ref. It is counted and acked rather than retried, since no number of retries will put that commit back; the next green commit corrects the ref. + +## Observability + +Two measurements are taken here because this is the stage where greenness becomes known, and both depend on a `SourceControl` lookup of a commit's creation time. + +- **Last-green age** — a gauge carrying the creation timestamp of the commit the bookmark now points at, emitted after the bookmark write is durable. It is a timestamp rather than an elapsed time so that subtracting it from the scrape time yields the current age; a queue that stops going green keeps reporting a staler and staler value without the stage having to re-emit anything. +- **Failure-detection latency** — a histogram of how long the break a build failed on went undetected, measured from the creation time of the base URI it validated against. A distribution rather than a gauge because the point is how long breaks typically survive, not how long the last one did. Unlike the last-green age there is no later moment to sample it from — an elapsed time is only meaningful against the failure that just became known — so this lookup cannot be moved off the delivery path onto a clock. + +Only the delivery that *wrote* the fact reports the latency. This is why `recordFact` returns whether it created the fact rather than just the fact itself: a redelivery adopts the stored fact, and a second sample would count one break twice in the distribution. A full build pins no base URI, so its failures are counted as unmeasurable rather than timed — absent is the ordinary case there, not a fault. + +Both are best-effort. Every way the lookup can fail is counted, logged, and swallowed, because an observability fault must not turn an outcome already durably recorded into a retry. A non-positive creation timestamp is treated as a broken extension contract and dropped rather than emitted, since reporting it would put a 1970 timestamp into a gauge that reads as an infinitely stale queue, or a decades-long sample into the latency distribution. ## Build slot release @@ -135,13 +173,13 @@ None of this is built. The prerequisites are the shared `HookEvent` contract und ### Where the publish belongs -The publish is the last thing the delivery does before the ack, after the fact write and after the bookmark advance: +The publish is the last thing the delivery does before the ack, after the fact write and after both caches derived from it have moved: ``` -create fact → advance bookmark (green only) → publish HookEvent → [Phase 2: publish to analyze] → ack +create fact → advance bookmark (green only) → promote (bookmark holder only) → publish HookEvent → [Phase 2: publish to analyze] → ack ``` -Ordering it after the bookmark matters because the framework forbids entity snapshots in the payload — hooks resolve entities from stores. A hook that reacts to "URI is green" by reading the Queue's `LastGreenURI` must not find a bookmark that still points at the previous commit, so the event cannot precede the write it implies. +Ordering it last matters because the framework forbids entity snapshots in the payload — hooks resolve entities from stores. A hook that reacts to "URI is green" by reading the Queue's `LastGreenURI`, or by fetching the promotion ref, must not find either one still pointing at the previous commit, so the event cannot precede the writes it implies. Ordering it inside the same delivery is what makes it lossless without an outbox: the state writes are recognize-and-skip on redelivery, so a crash before the ack replays the whole chain and re-publishes. @@ -183,7 +221,7 @@ This also keeps `id` computable from the request id alone, since it no longer de Ordering is per-subject only, and the subject is the request. Events for *different* requests can therefore arrive out of order, so a consumer must not infer "the newest green commit" from arrival order — it should compare request ids by ingest order (`entity.CompareRequestID`) or read the Queue's bookmark, which is monotonic by construction. -Absence of an event is not a signal. A cancelled build records no fact and publishes nothing, and the fail-closed path never reaches this stage at all, so a consumer waiting for an event per ingested commit will wait forever on those. Gating has to keep treating "no recorded fact" as not green. +Absence of an event is not a signal. A cancelled build records no fact and publishes nothing, and a Request abandoned before any build reached a terminal status never reaches this stage at all, so a consumer waiting for an event per ingested commit will wait forever on those. Gating has to keep treating "no recorded fact" as not green. The converse also holds: an event is not proof that the code was tested, since a fail-closed Request can produce a broken fact without a build having failed (see [What fail-closed actually guarantees](#what-fail-closed-actually-guarantees)). Delivery is at-least-once, deduped by `id` within the queue's retention window; hooks must be idempotent on `id`. "Fire-and-forget" describes downstream consumption, not the publish: `record` never waits for a hook to run, but a failed *publish* fails the delivery. Per [`platform/errs`](../../../../platform/errs/README.md) rule 4 that failure is not wrapped retryable just because replaying it is convenient, so it dead-letters — which is the point at which the missing `record_dlq` reconciler below stops being theoretical, since the fact is already durable and only the notification is lost. @@ -216,7 +254,8 @@ Partitioning by request id keeps completion bookkeeping single-writer per Reques - **Request not visible** — a storage defect rather than a lag, since the publish follows the committed outcome write. Non-retryable. - **Fact already created** — load it and continue from the authoritative fact. -- **Bookmark already advanced** — the guard skips equal-or-older candidates and the write is skipped. +- **Bookmark already advanced** — the guard skips equal-or-older candidates and the write is skipped. A Request that finds its *own* id there still counts as holding the bookmark, so the promotion behind it is retried. +- **Commit already promoted** — `Promote` is idempotent, so repeating it changes nothing. - **Duplicate builds for one Request** — absorbed a stage earlier: `buildsignal`'s outcome write is first-writer-wins, so the Request carries one immutable verdict and `record` never sees a competing one. - **Redelivery after a complete run** — every effect is recognize-and-skip. Once hooks land the publish re-fires, and the framework's dedupe on `id` absorbs it. @@ -234,11 +273,14 @@ An existing fact from a different Request, or a Request carrying no build outcom | Existing fact owned by a different Request | non-retryable | ingest dedup invariant violated; the stored fact is immutable | | Malformed request id at bookmark comparison | non-retryable | re-parsing the same ids cannot succeed | | Queue CAS version mismatch | absorbed, not raised | `storage.ErrVersionMismatch` is handled by the bookmark loop: reload and re-apply | +| Promotion target unknown to the ref | absorbed, not raised | `sourcecontrol.ErrNotFound` means a rewritten history dropped the commit; counted and acked | +| SourceControl resolution or `Promote` failure | raw error | backend classifier has the required failure knowledge | +| Commit-timestamp lookup for a metric | swallowed | counted and logged; observability must not retry a recorded outcome | | ValidationFactStore, QueueStore, RequestStore | raw error | backend classifier has the required failure knowledge | ## DLQ and fail-closed behavior -**There is no `record_dlq` consumer today, and the topic it would consume is already receiving messages.** The record subscription is created from `DefaultSubscriptionConfig`, which enables dead-lettering with the `_dlq` suffix, so a rejected record message is moved to `record_dlq` — but the wiring registers only `process_dlq` and `buildsignal_dlq`, and no controller subscribes to `record_dlq`. Messages accumulate there unread. +**Neither `record_dlq` nor `build_dlq` has a consumer today, and both topics are already receiving messages.** Every primary subscription is created from `DefaultSubscriptionConfig`, which enables dead-lettering with the `_dlq` suffix, so a rejected message on either stage is moved to its dead-letter topic — but the wiring registers only `process_dlq` and `buildsignal_dlq`. Messages accumulate unread on the other two. Gating stays safe, because everything this stage can lose reads as not-green: a request with no fact is indistinguishable from one not yet validated, and callers must already treat absence as not green (see [When to record an outcome](#when-to-record-an-outcome)). What is lost instead is the *fact* — a green build whose fact write permanently failed leaves the URI looking unvalidated, which costs the queue an incremental baseline and forces a full build at the next head. Once hooks land, a lost notification joins that list, and unlike the fact it has no second chance from a later commit. @@ -248,17 +290,51 @@ This is the same failure shape [buildsignal.md](buildsignal.md#what-it-costs-whe - If Request storage is temporarily unavailable, keep retrying. - If the payload is malformed or the Request is permanently missing, the message is poison: there is no trustworthy identity to act on, so it needs an operational alert rather than more retries. +### What fail-closed actually guarantees + +Fail-closed reconciliation guarantees that a Request which can never complete ends up in a state gating reads as not-green. It does **not** guarantee that this stage never runs for such a Request, and the difference is reachable rather than theoretical. + +A reconciler forcing `failed` publishes nothing here, but it also does nothing to stop a publish. The two are decoupled: `buildsignal` calls its record publish unconditionally once a build reaches a terminal status, outside the guard that makes its state write idempotent. A Request that already carries an outcome is admitted rather than dropped — deliberately, so a redelivery after the outcome was stamped but before the publish landed still reaches `record` — which means a forced `failed` is carried into a fact by any delivery that arrives afterwards. + +The path that turns that into a wrong answer runs through `buildsignal_dlq`, which is wired: + +1. A `build` delivery keeps failing after its side effect. Each redelivery triggers again, minting a fresh build id and persisting another `Build` row, so one Request ends up with several independent poll chains, each partitioned by its own build id. +2. One chain exhausts its retries and dead-letters to `buildsignal_dlq`. The reconciler releases the slot and forces the Request to `failed` — correctly, since nothing will look at *that* build again. +3. Another chain is untouched by any of this and later polls a terminal **success**. Its terminal write is a no-op because the Request already carries an outcome, and it then publishes here. +4. `record` loads a `failed` Request and writes `DegreeBroken` for a commit that actually built green. + +The fact is immutable and first-fact-wins, so that verdict is permanent. Gating is still safe in the direction that matters — the error is toward not-green, never falsely green — and the cost is bounded, because the next head re-establishes greenness on its own Request. But it is a strictly worse outcome than the one the fail-closed posture was reasoned about: not "we never learned the outcome" but "we observed success and recorded broken anyway", and it also denies the queue an incremental baseline it had earned. See [Whether observed success should outrank a forced failure](#whether-observed-success-should-outrank-a-forced-failure-open). + +Dead-lettering at `build` does not currently produce this, for the accidental reason that `build_dlq` has no reconciler: nothing forces `failed`, the Request stays `processing`, and the surviving poll chain stamps the real outcome. Wiring a reconciler onto that orphaned topic — the obvious fix for it — makes this path reachable from `build` too, so the two gaps should be closed together. + ## Edge cases - **Head equals last-green.** The build still produces a terminal verdict. A green result may share the bookmark's URI; the guard compares request ids, not URIs, so an equal-or-older candidate skips and nothing regresses. - **Cancelled build.** Stovepipe never initiates cancellation, but a backend may still report it. No fact is written; the slot was already released and the Request already stamped `cancelled`. The fact identity stays unclaimed, but nothing can claim it today — `cancelled` is a terminal state and re-validation does not exist — so in practice recovery is the next commit; the unclaimed identity only matters to a future re-run mechanism (see [Supporting re-run of the same URI](#supporting-re-run-of-the-same-uri)). -- **History rewrite while a build runs.** The Request keeps the strategy and URI pinned at admission; record stores the fact about that immutable URI. A later head is handled independently by `process`. -- **Late successful result after a fail-closed terminal.** The terminal `failed` Request wins: the fact records `DegreeBroken` even though the build passed, because the degree derives from `R.State`. Nothing rewrites greenness or reopens the pipeline. +- **History rewrite while a build runs.** The Request keeps the strategy and URI pinned at admission; record stores the fact about that immutable URI. A later head is handled independently by `process`. If the rewrite dropped the commit from the ref, the fact and the bookmark still stand — they describe a commit, not a ref — and only the promotion is skipped. +- **A fail-closed terminal outranks a build that actually passed.** The degree derives from `R.State`, so a Request forced to `failed` by DLQ reconciliation records `DegreeBroken` even when one of its builds reports success afterwards. Reachable today, and permanent once written; see [What fail-closed actually guarantees](#what-fail-closed-actually-guarantees). +- **Green fact recorded out of order across Requests.** An older green commit can reach this stage after a newer one. Its fact is written as usual — facts are per-URI and independent — but the bookmark guard skips it and, because it does not hold the bookmark, it does not promote either, so neither cache moves backwards. - **Crash between the fact write and the bookmark advance.** Redelivery reloads the existing fact and re-applies the idempotent guard. +- **Crash between the bookmark advance and the promotion.** Redelivery finds its own id on the bookmark, reports that it holds it, and retries the idempotent promotion. - **Ack fails after a complete run.** Redelivery re-runs every step as a no-op. ## Deferred and open +### Whether observed success should outrank a forced failure: open + +[workflow.md](../workflow.md#fail-closed-on-unprocessable-work) states that the conservative terminal outcome is *final, not provisional*. That was reasoned about a Request whose outcome was never observed, where `failed` is the only honest answer available. The path in [What fail-closed actually guarantees](#what-fail-closed-actually-guarantees) produces a different case, where a successful build was observed and loses to a forced failure anyway. + +The underlying question is what a validation fact asserts. If it is **evidence about the code**, a reconciler-forced failure has no business becoming one: it is evidence about the pipeline, and recording it as a broken commit is a category error. If it is **the pipeline's last word on a Request**, the current behaviour is correct by definition and the false-broken fact is the price of never being falsely green. + +Four ways forward, none free: + +- **Leave it.** The immutability story stays unqualified and no new state is needed. The cost is permanent false-broken facts on commits with nothing wrong with them, plus the incremental baseline the queue had earned. Defensible if the incident rate is low enough — and it is measurable, since the reconciler already counts what it forces. +- **Do not publish here when the terminal write was a no-op.** Rejected: `buildsignal` cannot distinguish "the outcome was already forced" from "I stamped it and crashed before publishing", and the second case depends on republishing to avoid losing the fact entirely. +- **Let a later observed success reopen the outcome.** Requires the Request to record *why* it failed, which it does not today, and it contradicts first-fact-wins unless the fact write is deferred until the Request can no longer change — which is exactly the durable completion marker this stage still lacks. +- **Record no fact from a forced failure.** Requires the same "why" on the Request, but trades a wrong fact for an absent one, which gating already treats as not-green and which a re-run mechanism could later fill. Closest to the evidence reading, and it leaves the identity unclaimed rather than permanently wrong. + +The last two both wait on the Request carrying its failure cause, so this is best decided alongside [Supporting re-run of the same URI](#supporting-re-run-of-the-same-uri) and the [completion marker](#completion-marker-open) rather than on its own. + ### Completion marker: open `record` makes no `Request` write in Phase 1, which leaves the stage with no durable marker saying it finished. The consequence is contained today, because every effect is idempotent and a redelivery simply re-runs them, but Phase 2 needs a marker: "all planned facts recorded" has to live somewhere. An earlier draft inserted a non-terminal `analyzing` state between `processing` and the recorded states; that no longer fits, because `buildsignal` drives the Request terminal before `record` ever runs, leaving no non-terminal window to occupy. Phase 2 must either move the outcome write back behind analysis, add a separate completion entity, or track fan-out on the facts themselves. Deferred to `analyze.md`. diff --git a/doc/rfc/stovepipe/workflow.md b/doc/rfc/stovepipe/workflow.md index ba03a58d..c7b62442 100644 --- a/doc/rfc/stovepipe/workflow.md +++ b/doc/rfc/stovepipe/workflow.md @@ -163,7 +163,7 @@ Per-stage design detail lives under `steps/` so this doc stays a pipeline overvi - [process.md](steps/process.md) — build-strategy decision, concurrency gate, backlog coalescing, [concurrency lifecycle](steps/process.md#concurrency-lifecycle), entity changes, [waiting for a slot](steps/process.md#waiting-for-a-slot) - [build.md](steps/build.md) — trigger-only stage: reads the decided scope off the Request, triggers the build-runner, hands off to buildsignal; the stovepipe `BuildRunner` contract and why it differs from SubmitQueue's - [buildsignal.md](steps/buildsignal.md) — the poll loop: hold-based re-poll cadence, target-graph return, per-build partitioning, and the fail-closed handoff to record -- [record.md](steps/record.md) — turning a terminal build outcome into an immutable validation fact, monotonic last-green advancement, and the deferred hook and analyze handoffs +- [record.md](steps/record.md) — turning a terminal build outcome into an immutable validation fact, monotonic last-green advancement and ref promotion, and the deferred hook and analyze handoffs ## Dedup, idempotency, and history rewrites @@ -171,7 +171,7 @@ Ingestion is idempotent on `(Queue, head URI)`, so duplicate poller reports — ## Fail-closed on unprocessable work -Callers gate deployments on greenness, so the dangerous failure is a Request that can never finish and silently leaves a URI with no recorded greenness — indistinguishable, to a naive caller, from "not yet validated". Following SQ's DLQ-reconciliation posture, a Request whose validation can never complete must be driven to a **conservative terminal `failed` outcome** — which whatever records greenness treats as not-green — rather than left non-terminal: gating stays safe (never falsely green), and the pipeline moves on. That conservative outcome is **final**, not provisional: validation facts are immutable and first-fact-wins, so a late successful result for a fail-closed URI is dropped rather than overwriting recorded history. The cost is bounded — the branch keeps moving, and the next head re-establishes greenness on its own Request. See [record.md](steps/record.md#dlq-and-fail-closed-behavior). See [submitqueue/orchestrator/controller/dlq/README.md](../../../submitqueue/orchestrator/controller/dlq/README.md) for the shared reconcile-only design. +Callers gate deployments on greenness, so the dangerous failure is a Request that can never finish and silently leaves a URI with no recorded greenness — indistinguishable, to a naive caller, from "not yet validated". Following SQ's DLQ-reconciliation posture, a Request whose validation can never complete must be driven to a **conservative terminal `failed` outcome** — which whatever records greenness treats as not-green — rather than left non-terminal: gating stays safe (never falsely green), and the pipeline moves on. That conservative outcome is **final**, not provisional: validation facts are immutable and first-fact-wins, so a late successful result for a fail-closed URI is dropped rather than overwriting recorded history. The cost is bounded — the branch keeps moving, and the next head re-establishes greenness on its own Request. Note that finality cuts deeper than "the outcome is never revised": a forced `failed` can itself be recorded as a broken fact by a delivery still in flight, so a commit whose build actually passed can end up permanently marked broken. Whether that is correct or a category error is open — see [record.md](steps/record.md#what-fail-closed-actually-guarantees). See [submitqueue/orchestrator/controller/dlq/README.md](../../../submitqueue/orchestrator/controller/dlq/README.md) for the shared reconcile-only design. ## Open questions From 900fa2d2b07ec50481688f41e820f5b5cfa9c73b Mon Sep 17 00:00:00 2001 From: mnoah1 Date: Thu, 13 Aug 2026 20:05:32 +0000 Subject: [PATCH 5/7] docs(stovepipe): sharpen the dead-letter mechanics behind the fail-closed path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two dead-letter triggers, not one: a delivery that fails with its budget spent is dead-lettered by the nack itself, but one that never reaches a nack — a crash, or an ack that fails and lets the visibility timeout redeliver — is dead-lettered by the poll loop without the controller running on that final attempt. So the exposure from a missing reconciler is wider than poison payloads. Retraces the wrong-fact path from that premise: build's ack fails, each redelivery triggers another build, and the poll chains outlive the dead-lettered build message because Hold resets retry_count rather than spending it. Also corrects build.md, which claimed a build_dlq reconciler frees the slot; none exists, so a build that dead-letters before persisting its row wedges a slot for good. --- doc/rfc/stovepipe/steps/build.md | 6 +++++- doc/rfc/stovepipe/steps/record.md | 18 +++++++++++------- 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/doc/rfc/stovepipe/steps/build.md b/doc/rfc/stovepipe/steps/build.md index 15e1194b..0078ecd2 100644 --- a/doc/rfc/stovepipe/steps/build.md +++ b/doc/rfc/stovepipe/steps/build.md @@ -81,7 +81,7 @@ Every branch is safe under at-least-once redelivery — with SubmitQueue's postu - **Strategy not yet visible** — retryable; the producing stage's write is not visible on this reader yet. - **Request already terminal** (step 2) — ack, no build. A redelivery after `record` finished, or after `process` superseded the head, never starts a stale build. - **Redelivery while the Request is still in flight** (crash or failure anywhere in steps 5–8) — the redelivery re-runs from step 1, `Trigger` mints a fresh id, `Create` persists a second `Build` row, and a second poll loop starts. Harmless, in three layers: both builds target the identical `(headURI, baseURI)` scope; each `Build` polls in its own partition and `buildsignal` short-circuits the moment the Request goes terminal (its step 3); and `buildsignal`'s outcome write is first-writer-wins, so the second verdict cannot flip the Request's state or overwrite the create-only validation fact. A build triggered but never persisted (crash between steps 5 and 6) is the same story minus the row: an orphan the runner finishes and nobody ever reads. Wasted CI compute, not a correctness risk — the same accepted trade as SubmitQueue. -- **Trigger / publish / other store failure** — nothing durable is left half-written that a redelivery can't reconcile; the error rejects to DLQ, and the fail-closed reconciler drives the Request terminal (see [workflow.md](doc/rfc/stovepipe/workflow.md#fail-closed-on-unprocessable-work)). +- **Trigger / publish / other store failure** — nothing durable is left half-written that a redelivery can't reconcile; the error rejects to DLQ, where the fail-closed posture is meant to drive the Request terminal (see [workflow.md](doc/rfc/stovepipe/workflow.md#fail-closed-on-unprocessable-work)). No reconciler consumes `build_dlq` yet, so that last step does not happen today — see [Fail-closed interaction](#fail-closed-interaction). ## Edge cases @@ -92,6 +92,10 @@ Every branch is safe under at-least-once redelivery — with SubmitQueue's postu A build that never reaches step 8 — `Trigger` failing repeatedly, the publish to `buildsignal` never landing, `BuildStore.Create` down — must not wedge its `Request`'s Queue slot forever: `process`'s per-Queue concurrency gate holds `in_flight_count` open until the Request reaches a terminal state (see [process.md](doc/rfc/stovepipe/steps/process.md#concurrency-lifecycle)). `build` does not implement the forcing function itself. Per [workflow.md](doc/rfc/stovepipe/workflow.md#fail-closed-on-unprocessable-work), every non-retryable failure in the algorithm rejects to DLQ (see [Error classification](#error-classification)), and a Request stuck past `MaxAttempts` is driven to a conservative terminal `failed` by the DLQ reconciler, which decrements `in_flight_count` and frees the slot. This is the same posture `buildsignal` relies on for its own poll loop (see [buildsignal.md](doc/rfc/stovepipe/steps/buildsignal.md#fail-closed-interaction)) — `build` and `buildsignal` are two links in the same fail-closed chain that keeps one bad Request from wedging its Queue. +**That chain is not closed at `build` yet.** The `build` subscription enables dead-lettering, but no controller consumes `build_dlq` — the wiring registers only `process_dlq` and `buildsignal_dlq` — so nothing forces the Request terminal and nothing frees the slot. How much that costs depends on how far the delivery got. If a `Build` row was persisted and its signal published, a poll chain survives the dead-letter and `buildsignal` still releases the slot when the build goes terminal. If the message dead-letters before that — `Trigger` failing every attempt, `BuildStore.Create` down, the publish never landing — the Request stays `processing` and its Queue loses a slot for good, which is exactly the failure [buildsignal.md](doc/rfc/stovepipe/steps/buildsignal.md#fail-closed-interaction) describes for a deployment missing its own reconciler. + +Whoever wires that reconciler has to decide what it records, not just what it releases: forcing `failed` on a Request whose build may still be running is what produces the permanently-wrong-fact path in [record.md](record.md#what-fail-closed-actually-guarantees), so this gap and that open question belong to the same piece of work. + One boundary is worth stating explicitly: this path fires only when `build` (or a downstream stage) *errors*. A `Trigger` call that returns successfully but the backend never actually runs — or a `Build` row created for a build the runner silently drops — has no protocol-level failure to escalate at the `build` stage; nothing here retries or dead-letters, because nothing failed. That gap surfaces one hop later, when `buildsignal` polls: either the runner reports an error (handled by `buildsignal`'s own classification) or it reports a non-terminal status forever, which is `buildsignal`'s fail-closed boundary to close, not `build`'s (see [buildsignal.md](doc/rfc/stovepipe/steps/buildsignal.md#fail-closed-interaction)). `build`'s liveness responsibility ends at a successful publish to `buildsignal`. ## Cancellation: defined, not yet called diff --git a/doc/rfc/stovepipe/steps/record.md b/doc/rfc/stovepipe/steps/record.md index 8e38a554..63ec901c 100644 --- a/doc/rfc/stovepipe/steps/record.md +++ b/doc/rfc/stovepipe/steps/record.md @@ -280,7 +280,9 @@ An existing fact from a different Request, or a Request carrying no build outcom ## DLQ and fail-closed behavior -**Neither `record_dlq` nor `build_dlq` has a consumer today, and both topics are already receiving messages.** Every primary subscription is created from `DefaultSubscriptionConfig`, which enables dead-lettering with the `_dlq` suffix, so a rejected message on either stage is moved to its dead-letter topic — but the wiring registers only `process_dlq` and `buildsignal_dlq`. Messages accumulate unread on the other two. +**Neither `record_dlq` nor `build_dlq` has a consumer today, and both topics are already receiving messages.** Every primary subscription is created from `DefaultSubscriptionConfig`, which enables dead-lettering with the `_dlq` suffix, so a message that is rejected outright *or* runs out of retries is moved to its stage's dead-letter topic — but the wiring registers only `process_dlq` and `buildsignal_dlq`. Messages accumulate unread on the other two. + +Two different things put a message there, and only one of them is a poison payload. A delivery that fails once its retry budget is spent is dead-lettered by the nack itself, carrying the reason it actually failed. A delivery that never reaches a nack at all — a crash, or an **ack that fails** and lets the visibility timeout redeliver — is dead-lettered by the poll loop instead, as soon as `retry_count` reaches `MaxAttempts` (3 by default), without the controller being invoked on that final attempt and with only a generic reason recorded. So what a missing reconciler exposes is wider than malformed messages: a fact can be lost to a storage failure that would have succeeded on a later retry, or to an ack that never landed even though the write did. Gating stays safe, because everything this stage can lose reads as not-green: a request with no fact is indistinguishable from one not yet validated, and callers must already treat absence as not green (see [When to record an outcome](#when-to-record-an-outcome)). What is lost instead is the *fact* — a green build whose fact write permanently failed leaves the URI looking unvalidated, which costs the queue an incremental baseline and forces a full build at the next head. Once hooks land, a lost notification joins that list, and unlike the fact it has no second chance from a later commit. @@ -296,16 +298,18 @@ Fail-closed reconciliation guarantees that a Request which can never complete en A reconciler forcing `failed` publishes nothing here, but it also does nothing to stop a publish. The two are decoupled: `buildsignal` calls its record publish unconditionally once a build reaches a terminal status, outside the guard that makes its state write idempotent. A Request that already carries an outcome is admitted rather than dropped — deliberately, so a redelivery after the outcome was stamped but before the publish landed still reaches `record` — which means a forced `failed` is carried into a fact by any delivery that arrives afterwards. -The path that turns that into a wrong answer runs through `buildsignal_dlq`, which is wired: +The concrete path starts with a `build` delivery whose ack fails: -1. A `build` delivery keeps failing after its side effect. Each redelivery triggers again, minting a fresh build id and persisting another `Build` row, so one Request ends up with several independent poll chains, each partitioned by its own build id. -2. One chain exhausts its retries and dead-letters to `buildsignal_dlq`. The reconciler releases the slot and forces the Request to `failed` — correctly, since nothing will look at *that* build again. -3. Another chain is untouched by any of this and later polls a terminal **success**. Its terminal write is a no-op because the Request already carries an outcome, and it then publishes here. -4. `record` loads a `failed` Request and writes `DegreeBroken` for a commit that actually built green. +1. `build` does its work — triggers the runner, persists the `Build`, publishes the signal — and then fails to ack. The visibility timeout redelivers it, and the controller triggers *again*, minting a fresh build id and persisting another `Build` row. One Request accumulates as many independent poll chains as it had deliveries, each partitioned by its own build id. +2. The retry budget runs out and the poll loop dead-letters the build message to `build_dlq`, where nothing consumes it. +3. The poll chains are untouched by that. They re-poll with `Hold`, which resets `retry_count` and exempts the redelivery from the increment, so a chain polls for as long as its build takes without ever spending its own budget. The chains outlive the build message rather than dying with it. +4. One chain does eventually exhaust its retries — a `Status` call that stays broken through every attempt, say — and dead-letters to `buildsignal_dlq`. That reconciler releases the slot and forces the Request to `failed`, correctly, since nothing will look at *that* build again. +5. Another chain later polls a terminal **success**. Its terminal write is a no-op because the Request already carries an outcome, and it then publishes here. +6. `record` loads a `failed` Request and writes `DegreeBroken` for a commit that actually built green. The fact is immutable and first-fact-wins, so that verdict is permanent. Gating is still safe in the direction that matters — the error is toward not-green, never falsely green — and the cost is bounded, because the next head re-establishes greenness on its own Request. But it is a strictly worse outcome than the one the fail-closed posture was reasoned about: not "we never learned the outcome" but "we observed success and recorded broken anyway", and it also denies the queue an incremental baseline it had earned. See [Whether observed success should outrank a forced failure](#whether-observed-success-should-outrank-a-forced-failure-open). -Dead-lettering at `build` does not currently produce this, for the accidental reason that `build_dlq` has no reconciler: nothing forces `failed`, the Request stays `processing`, and the surviving poll chain stamps the real outcome. Wiring a reconciler onto that orphaned topic — the obvious fix for it — makes this path reachable from `build` too, so the two gaps should be closed together. +Note where the forced failure actually comes from. Not from the `build` dead-letter, which today does nothing whatsoever, because `build_dlq` has no reconciler: had step 4 not happened, the Request would have stayed `processing` until a surviving chain stamped the real outcome, and the fact would have been green. Wiring a reconciler onto that orphaned topic — the obvious fix for it — moves the forced `failed` up to step 2 and makes a *single* surviving poll chain sufficient to record a broken fact for a green commit. That is the sharper reason to treat the two gaps as one piece of work: closing `build_dlq` in isolation widens this hole rather than narrowing it. [build.md](build.md#fail-closed-interaction) covers the other half of that gap, where the same missing reconciler costs the Queue a build slot. ## Edge cases From edf793015b0fc88795f9b91228786ef54d5696cd Mon Sep 17 00:00:00 2001 From: mnoah1 Date: Thu, 13 Aug 2026 20:37:43 +0000 Subject: [PATCH 6/7] docs(stovepipe): tighten record RFC prose Rewrite the denser passages in plainer language and cut redundant scaffolding. No design content removed: the hook framework, Phase 2 sketches, and open questions stay in full, since the doc describes how the step is meant to work and not only what ships today. --- doc/rfc/stovepipe/steps/record.md | 256 +++++++++++++----------------- 1 file changed, 110 insertions(+), 146 deletions(-) diff --git a/doc/rfc/stovepipe/steps/record.md b/doc/rfc/stovepipe/steps/record.md index 63ec901c..45b8a4c2 100644 --- a/doc/rfc/stovepipe/steps/record.md +++ b/doc/rfc/stovepipe/steps/record.md @@ -2,14 +2,14 @@ `record` turns a terminal build outcome into a durable validation fact. -- In Phase 1 it records whole-repository greenness and, when the result is green, advances the Queue's last-green bookmark and promotes the commit onto the Queue's promotion ref. This is implemented: see [stovepipe/controller/record/record.go](../../../../stovepipe/controller/record/record.go). -- In Phase 2 the same stage records greenness per project rather than per repository. Sketched here, to be expanded before implementation. +- Phase 1 records whole-repository greenness. On green it also advances the Queue's last-green bookmark and promotes the commit onto the Queue's promotion ref. This is implemented: see [stovepipe/controller/record/record.go](../../../../stovepipe/controller/record/record.go). +- Phase 2 records greenness per project instead of per repository. Sketched here, to be expanded before implementation. -Notifying downstream systems is **not** implemented in either phase. It will ride the cross-domain hook framework rather than a Stovepipe-specific extension; see [Hooks](#hooks). +Notifying downstream systems is **not** implemented in either phase. It will ride the cross-domain hook framework instead of a Stovepipe-specific extension; see [Hooks](#hooks). -See [workflow.md](../workflow.md) for the complete pipeline, [build.md](build.md) for how builds are created, and [buildsignal.md](buildsignal.md) for the terminal-only handoff into this stage. +See [workflow.md](../workflow.md) for the whole pipeline, [build.md](build.md) for how builds are created, and [buildsignal.md](buildsignal.md) for the terminal-only handoff into this stage. -`record` owns persistence and publication of validation facts, plus the two caches derived from them: the last-green bookmark and the promotion ref. It does not decide build scope, poll a build, release the Queue's build slot, interpret a target graph, or map targets to projects. Those responsibilities belong to `process`, `buildsignal`, and `analyze`. +`record` owns the validation facts and the two caches derived from them, the last-green bookmark and the promotion ref. It does not decide build scope, poll a build, release the Queue's build slot, interpret a target graph, or map targets to projects. Those belong to `process`, `buildsignal`, and `analyze`. ## Phase 1 algorithm @@ -32,11 +32,11 @@ For a delivery carrying a `Record` payload: payloads written before the field existed. 5. Inspect R.State. - - succeeded / failed -> continue. This is the entry condition: buildsignal stamps the - outcome before it publishes here, and both values are verdicts about the code. - - cancelled -> ack. No fact: the build decided nothing about the commit (see + - succeeded / failed -> continue. buildsignal stamps the outcome before it + publishes here, and both values are verdicts about the code. + - cancelled -> ack, no fact: the build decided nothing about the commit (see "When to record an outcome"). - - superseded -> ack; no fact is written. Unreachable in practice. + - superseded -> ack, no fact. Unreachable in practice. - accepted / processing / anything else -> return a non-retryable invariant error. 6. Map R.State to a whole-repository degree and create the ValidationFact keyed by @@ -44,48 +44,35 @@ For a delivery carrying a `Record` payload: - ErrAlreadyExists -> load and reconcile the existing immutable fact. - other store error -> return raw. -7. If the persisted fact is not green, ack — reporting how long the break went - undetected, but only if step 6 is the write that created the fact (see - "Observability"). Otherwise advance the Queue bookmark in a CAS retry loop, which - also reports whether R holds the bookmark afterwards: - a. If LastGreenRequestID is empty or older than R.ID per entity.CompareRequestID, set - LastGreenURI = R.URI and LastGreenRequestID = R.ID and write with the version - guard. R holds the bookmark. - b. If R.ID equals the stored id, R already set it on an earlier delivery. R holds the - bookmark; no write. - c. If R.ID is older, a later green commit holds the bookmark. No write, and R does - not hold it. - d. On ErrVersionMismatch, reload and re-evaluate; the guard is idempotent. - -8. If R does not hold the bookmark, ack: the commit that does hold it also owns the - promotion ref. Otherwise ask SourceControl to point the Queue's promotion ref at - R.URI. +7. If the persisted fact is not green, ack. Report how long the break went undetected + first, but only if step 6 is the write that created the fact (see "Observability"). + +8. Advance the bookmark to (R.URI, R.ID) in a CAS retry loop, which also reports + whether R holds the bookmark afterwards: + - stored id empty or older than R.ID -> write under the version guard; R holds it. + - stored id equals R.ID -> R set it on an earlier delivery; no write, + and R still holds it. + - stored id newer -> no write, and R does not hold it. + - ErrVersionMismatch -> reload and re-evaluate. + +9. If R holds the bookmark, ask SourceControl to point the promotion ref at R.URI. + Otherwise ack: whichever commit holds the bookmark owns the ref. - ErrNotFound -> count and ack. A rewritten history dropped the commit from the ref, and no retry can promote it. - other error -> return raw. -9. ack. +10. ack. ``` -Every decision after step 6 uses the persisted fact, not the outcome read from this delivery's Request. The first immutable fact controls the Queue bookmark and the promotion ref, and will control the hook event. - -Once hooks land, the event publish becomes a new step between 8 and 9; see [Hooks](#hooks). - -## Validation fact recording - -A validation fact answers "how broken was this scope at this Queue URI?" Its identity is: - -``` -(queue, uri, project) -``` +Every decision after step 6 uses the persisted fact, not the outcome read from this delivery's Request. The first immutable fact controls the bookmark and the ref, and will control the hook event. Once hooks land, the publish becomes a new step between 9 and 10. -`project` is empty for whole-repository greenness and is a stable project id in Phase 2. +## Validation facts -The `queue` component is the binding of the store the fact lives in, not a field on the entity: storage is resolved per queue (`storage.Factory.For`), so a fact only ever exists inside a queue's store and does not need to name it. +A fact answers "how broken was this scope at this Queue URI?" Its identity is `(queue, uri, project)`, where `project` is empty for whole-repository greenness and a stable project id in Phase 2. -The column still leads the fact table's primary key — `PRIMARY KEY (queue, uri, project)` — because every domain table in the repo must be shardable by queue: the primary key leads with the queue column and no secondary index leads with anything else, so one queue's rows are unreachable through another queue's binding and every read is a primary-key-prefix scan within a single queue. This is an invariant, not a per-table choice, and `make lint-queue-shard` enforces it. Note that it is not MySQL table partitioning; no schema in the repo uses `PARTITION BY`. +`queue` is the binding of the store the fact lives in, not a field on the entity. Storage is resolved per queue through `storage.Factory.For`, so a fact only ever exists inside one queue's store and does not need to name it. The column still leads the primary key, `PRIMARY KEY (queue, uri, project)`, because every domain table here has to be shardable by queue: the primary key leads with the queue column and no secondary index leads with anything else. That keeps one queue's rows unreachable through another queue's binding and makes every read a primary-key-prefix scan inside a single queue. It is an invariant rather than a per-table choice, and `make lint-queue-shard` enforces it. It is not MySQL table partitioning; no schema here uses `PARTITION BY`. -`entity.ValidationFact` therefore carries only the rest of the identity: +So `entity.ValidationFact` carries only the rest of the identity: | Field | Meaning | | ----------- | ------------------------------------------------------------------------------------------------------------------ | @@ -95,26 +82,26 @@ The column still leads the fact table's primary key — `PRIMARY KEY (queue, uri | `RequestID` | Request that established the fact | | `CreatedAt` | Millisecond timestamp at which the fact was first recorded | -Greenness is read through `ValidationFact.IsGreen()` rather than compared against a bare literal, and the endpoints are the named constants `entity.DegreeGreen` / `entity.DegreeBroken`. +Greenness is read through `ValidationFact.IsGreen()` rather than compared against a bare literal, and the endpoints are the named constants `entity.DegreeGreen` and `entity.DegreeBroken`. -Facts are create-only. `ErrAlreadyExists` on create means one of exactly two things: +Facts are create-only, so `ErrAlreadyExists` on create means one of exactly two things: -- Same Request → a redelivery. The existing fact is this delivery's own prior write and necessarily carries the same degree, since the Request's outcome is immutable once stamped. Load it and continue. -- Different Request → the `(Queue, URI)` ingest dedup invariant has been violated. Return an error rather than overwrite history. +- Same Request: a redelivery. The stored fact is this delivery's own earlier write, and it carries the same degree, since the Request's outcome is immutable once stamped. Load it and continue. +- Different Request: the `(Queue, URI)` ingest dedup invariant is broken. Return an error rather than overwrite history. -Competing verdicts cannot reach this point: duplicate builds for one Request are resolved a stage earlier, where `buildsignal`'s first-writer-wins outcome write discards the losing build's verdict, so `record` only ever sees one. +Competing verdicts never reach this point. Duplicate builds for one Request are resolved a stage earlier, where `buildsignal`'s first-writer-wins outcome write discards the losing build's verdict. -Absence remains distinct from degree `0`. Callers gating deployments must treat absence as not green. +Absence is still distinct from degree `0`. Callers gating deployments must treat absence as not green. ### When to record an outcome -A fact is written only when the request reaches a `succeeded` or `failed` verdict. A `cancelled` build is acked with no fact recorded. Callers gating deployments treat an absent fact as not green. +A fact is written only when the Request reaches `succeeded` or `failed`. A `cancelled` build is acked with no fact. -The fail-closed path is subtler than "no fact". A DLQ reconciler forces the Request to `failed` and does not itself publish here, so reconciliation *alone* records nothing — but that does not mean nothing is recorded. A `buildsignal` delivery still in flight can reach this stage afterwards and will then write `DegreeBroken` from the forced state. See [What fail-closed actually guarantees](#what-fail-closed-actually-guarantees). +The fail-closed path is subtler than "no fact". A DLQ reconciler forces the Request to `failed` and does not itself publish here, so reconciliation on its own records nothing. That is not the same as nothing being recorded: a `buildsignal` delivery still in flight can reach this stage afterwards, and it will write `DegreeBroken` from the forced state. See [What fail-closed actually guarantees](#what-fail-closed-actually-guarantees). ### Phase 1 degree mapping -MVP whole-repository builds use only the endpoints, mapped from the outcome `buildsignal` stamped on the Request: +Whole-repository builds use only the endpoints, mapped from the outcome `buildsignal` stamped on the Request: | Request outcome | Result | | --------------- | ------------------------------- | @@ -122,66 +109,58 @@ MVP whole-repository builds use only the endpoints, mapped from the outcome `bui | `failed` | fact at `entity.DegreeBroken` | | `cancelled` | no fact | -Intermediate degrees are reserved for project analysis and deferred with the project mapping contract; Phase 1 does not manufacture fractional values. +Intermediate degrees are reserved for project analysis and deferred with the project mapping contract. Phase 1 does not manufacture fractional values. ## Last-green advancement (Queue bookmark) -The bookmark only moves forward. `entity.Queue` carries `LastGreenURI` alongside `LastGreenRequestID` — the request id that owns the current URI — plus `InFlightCount` and `LatestRequestID`. On a green fact, step 7 adopts the pair `(R.URI, R.ID)` only when the stored id is empty or older than `R.ID` per `entity.CompareRequestID(R.Queue, …)` — the same ingest-order comparison `ingest` and `process` already use for coalescing. That comparison returns an error on an id that does not match the queue's format; treat it as non-retryable, since re-parsing the same ids cannot succeed. +The bookmark only moves forward. `entity.Queue` carries `LastGreenURI` alongside `LastGreenRequestID`, the request id that owns the current URI, plus `InFlightCount` and `LatestRequestID`. The guard compares ids with `entity.CompareRequestID(R.Queue, …)`, the same ingest-order comparison `ingest` and `process` use for coalescing. It errors on an id that does not match the queue's format, which is non-retryable: re-parsing the same ids cannot start succeeding. -The write goes through `QueueStore.Update(ctx, queue, oldVersion, newVersion)`, so version arithmetic stays in the controller: it computes `newVersion = oldVersion + 1` and the store performs a pure conditional write. `ErrVersionMismatch` is absorbed by the loop rather than returned — a concurrent writer moved the row, so reloading and re-applying the guard converges. +The write goes through `QueueStore.Update(ctx, queue, oldVersion, newVersion)`, so version arithmetic stays in the controller: it computes `newVersion = oldVersion + 1` and the store does a pure conditional write. `ErrVersionMismatch` is absorbed by the loop rather than returned, since a concurrent writer moved the row and reloading converges. -**Why the bookmark is advanced only after the fact is durable**: the bookmark is a cache of "newest green URI" derived from the facts, so it must never lead them. Losing the advance to a crash is recoverable — the redelivery reloads the same fact and retries — whereas a bookmark with no fact behind it would point at greenness nothing recorded, and `process` would pick a baseline URI that no validation supports. +**Why the bookmark moves only after the fact is durable.** The bookmark is a cache of "newest green URI" derived from the facts, so it must never lead them. Losing the advance to a crash is recoverable, since the redelivery reloads the same fact and retries. A bookmark with no fact behind it would point at greenness nothing recorded, and `process` would pick a baseline URI that no validation supports. -A failed or cancelled build never moves the bookmark, and therefore never promotes either. +A failed or cancelled build never moves the bookmark, and so never promotes either. ## Promotion ref -A green commit is also **promoted**: `record` asks `SourceControl` to point the Queue's promotion ref — a stable branch name like `verified-main` — at the commit. That is the pull-shaped counterpart to the hook event's push, letting a deploy gate or cache warmer resolve a name and know nothing about Stovepipe, URIs, or degrees. See [Promotion ref](../workflow.md#promotion-ref--the-last-green-commit-by-name) in the pipeline overview. - -The stage names only the commit. Which ref a Queue promotes to, and whether it has one at all, is integrator configuration injected into the `SourceControl` implementation alongside the endpoint and credentials, so a backend with no promotion target configured makes `Promote` a no-op. +A green commit is also promoted: `record` asks `SourceControl` to point the Queue's promotion ref, a stable branch name like `verified-main`, at that commit. This is the pull-shaped counterpart to the hook event's push, so a deploy gate or cache warmer can resolve a name and know nothing about Stovepipe, URIs, or degrees. See [Promotion ref](../workflow.md#promotion-ref--the-last-green-commit-by-name) in the pipeline overview. The stage names only the commit; which ref a Queue promotes to, and whether it has one at all, is integrator configuration injected into the `SourceControl` implementation alongside the endpoint and credentials, so a backend with no promotion target makes `Promote` a no-op. -**Promotion is gated on holding the bookmark.** `advanceLastGreen` reports whether the Request holds the bookmark after the CAS loop, and only the holder promotes. Without that gate an older green commit recording late would drag the ref backwards, because the ref has no version guard of its own — `Promote` lands the URI it is given rather than refusing a non-descendant, so monotonicity has to be enforced by the caller. Deciding it from the bookmark rather than a fresh comparison means both caches move under one already-serialized decision. +**Promotion is gated on holding the bookmark**, and only the holder promotes. Without that gate an older green commit recording late would drag the ref backwards: the ref has no version guard of its own, and `Promote` lands the URI it is given rather than refusing a non-descendant, so the caller has to enforce monotonicity. Deciding it from the bookmark instead of a fresh comparison means both caches move on one already-serialized decision. -The equal case matters as much as the newer one: a redelivery that finds its own id already on the bookmark reports that it *holds* it rather than skipping, so the promotion behind it is retried. That is what makes a crash between the bookmark write and the promotion recoverable, and it is safe because `Promote` is idempotent — promoting the URI the ref already points at changes nothing. +The equal case matters as much as the newer one. A redelivery that finds its own id on the bookmark reports that it *holds* it rather than skipping, so the promotion behind it is retried. That is what makes a crash between the bookmark write and the promotion recoverable, and it is safe because `Promote` is idempotent. Promotion follows the durable fact for the same reason the bookmark does: the ref is a cache of the facts and must not lead them. -Promotion follows the durable fact for the same reason the bookmark does: the ref is a cache of the facts and must never lead them. - -`ErrNotFound` from `Promote` means a rewritten history dropped the commit from the ref. It is counted and acked rather than retried, since no number of retries will put that commit back; the next green commit corrects the ref. +`ErrNotFound` from `Promote` means a rewritten history dropped the commit from the ref. It is counted and acked rather than retried, since no retry will put that commit back and the next green commit corrects the ref. ## Observability -Two measurements are taken here because this is the stage where greenness becomes known, and both depend on a `SourceControl` lookup of a commit's creation time. +Two things are measured here, because this is where greenness becomes known. Both need a `SourceControl` lookup of a commit's creation time. -- **Last-green age** — a gauge carrying the creation timestamp of the commit the bookmark now points at, emitted after the bookmark write is durable. It is a timestamp rather than an elapsed time so that subtracting it from the scrape time yields the current age; a queue that stops going green keeps reporting a staler and staler value without the stage having to re-emit anything. -- **Failure-detection latency** — a histogram of how long the break a build failed on went undetected, measured from the creation time of the base URI it validated against. A distribution rather than a gauge because the point is how long breaks typically survive, not how long the last one did. Unlike the last-green age there is no later moment to sample it from — an elapsed time is only meaningful against the failure that just became known — so this lookup cannot be moved off the delivery path onto a clock. +- **Last-green age.** A gauge carrying the creation timestamp of the commit the bookmark now points at, emitted once the bookmark write is durable. It is a timestamp rather than an elapsed time so that subtracting it at scrape time gives the current age, and a queue that stops going green keeps reporting a staler value without the stage re-emitting anything. +- **Failure-detection latency.** A histogram of how long the break a build failed on went undetected, measured from the creation time of the base URI it validated against. A distribution rather than a gauge, because what matters is how long breaks typically survive, not how long the last one did. There is no later moment to sample it from, since an elapsed time only means something against the failure that just became known, so this lookup cannot move off the delivery path onto a clock. -Only the delivery that *wrote* the fact reports the latency. This is why `recordFact` returns whether it created the fact rather than just the fact itself: a redelivery adopts the stored fact, and a second sample would count one break twice in the distribution. A full build pins no base URI, so its failures are counted as unmeasurable rather than timed — absent is the ordinary case there, not a fault. +Only the delivery that *wrote* the fact reports the latency. That is why `recordFact` returns whether it created the fact and not just the fact itself: a redelivery adopts the stored fact, and a second sample would count one break twice. A full build pins no base URI, so its failures are counted as unmeasurable instead of timed; absent is the ordinary case there, not a fault. -Both are best-effort. Every way the lookup can fail is counted, logged, and swallowed, because an observability fault must not turn an outcome already durably recorded into a retry. A non-positive creation timestamp is treated as a broken extension contract and dropped rather than emitted, since reporting it would put a 1970 timestamp into a gauge that reads as an infinitely stale queue, or a decades-long sample into the latency distribution. +Both are best-effort. Every way the lookup can fail is counted, logged, and swallowed, because an observability fault must not turn an already-recorded outcome into a retry. A non-positive creation timestamp is treated as a broken extension contract and dropped, since emitting it would put a 1970 timestamp in a gauge that reads as an infinitely stale queue, or a decades-long sample in the latency distribution. ## Build slot release -`Queue.in_flight_count` is released by `buildsignal` before it stamps the outcome and publishes the record step. The DLQ reconciler releases the slot on the fail-closed path for the same reason. `record`'s only Queue write is the last-green bookmark. +`Queue.in_flight_count` is released by `buildsignal` before it stamps the outcome and publishes to the record step. The DLQ reconciler releases the slot on the fail-closed path for the same reason. `record`'s only Queue write is the bookmark. ## Hooks -Recording a fact is the moment the rest of the company can learn "this URI is now green (or not green)". `record` is the publisher of that event, but it does **not** call a notification extension inline. Delivery rides the cross-domain hook framework in [hook-framework.md](../../hook-framework.md): the stage publishes a `HookEvent` to Stovepipe's durable `hook` topic, and a separate per-domain dispatcher stage consumes it and invokes whichever hooks the host wired. - -Inline notification was the earlier design here — a Stovepipe `Hooks` extension with a `Notify(ctx, ValidationFactRef{…})` call as the last step of the algorithm. The framework rejects both halves of it explicitly: an inline call couples pipeline latency to third-party integrations and drops the notification on a crash between the state write and the call, and a per-domain contract multiplies schemas and sinks for no gain. Nothing in that shape survives except the intent. +Recording a fact is when the rest of the company can learn "this URI is now green (or not green)". `record` publishes that event but does **not** call a notification extension inline. Delivery rides the cross-domain hook framework in [hook-framework.md](../../hook-framework.md): the stage publishes a `HookEvent` to Stovepipe's durable `hook` topic, and a per-domain dispatcher stage consumes it and invokes whichever hooks the host wired. The earlier design here, a Stovepipe `Hooks` extension with a `Notify(ctx, ValidationFactRef{…})` call as the last algorithm step, is rejected by the framework on both halves: an inline call ties pipeline latency to third-party integrations and drops the notification on a crash between the state write and the call, and a per-domain contract multiplies schemas and sinks for no gain. -None of this is built. The prerequisites are the shared `HookEvent` contract under `api/base/hook/`, the hook extension at `platform/extension/hook/`, and — Stovepipe's own share — a `hook` topic key, a dispatcher stage, a `hook_dlq` reconciler, and the wiring for all three. +None of this is built. It needs the shared `HookEvent` contract under `api/base/hook/`, the hook extension at `platform/extension/hook/`, and Stovepipe's own share: a `hook` topic key, a dispatcher stage, a `hook_dlq` reconciler, and the wiring for all three. ### Where the publish belongs -The publish is the last thing the delivery does before the ack, after the fact write and after both caches derived from it have moved: +Last thing before the ack, after the fact write and after both caches derived from it have moved: ``` create fact → advance bookmark (green only) → promote (bookmark holder only) → publish HookEvent → [Phase 2: publish to analyze] → ack ``` -Ordering it last matters because the framework forbids entity snapshots in the payload — hooks resolve entities from stores. A hook that reacts to "URI is green" by reading the Queue's `LastGreenURI`, or by fetching the promotion ref, must not find either one still pointing at the previous commit, so the event cannot precede the writes it implies. - -Ordering it inside the same delivery is what makes it lossless without an outbox: the state writes are recognize-and-skip on redelivery, so a crash before the ack replays the whole chain and re-publishes. +It goes last because the framework forbids entity snapshots in the payload, so hooks resolve entities from stores. A hook reacting to "URI is green" by reading `LastGreenURI`, or by fetching the promotion ref, must not find either one still pointing at the previous commit. Keeping it inside the same delivery is what makes it lossless without an outbox: the state writes are recognize-and-skip on redelivery, so a crash before the ack replays the whole chain and re-publishes. ### Event shape @@ -193,54 +172,50 @@ Ordering it inside the same delivery is what makes it lossless without an outbox | `timestamp_ms` | Publish time; the fact's own `CreatedAt` travels in the payload | | `id` | `source` / `type` / request id / `0` | -The **subject** is the Request. It is not an envelope field — the framework keeps subject, queue, and error in the payload deliberately — but it is what the `id` is minted from and what the event is partitioned by. Choosing the Request over the URI keeps partitioning identical to the `record` topic's own, so per-request ordering carries through the seam unbroken, and gives a consumer the handle it needs to trace an event back into the pipeline. The two are near-interchangeable in practice, since ingest dedups on `(queue, uri)` and one Request means one URI. - -The payload carries the fact's full identity and value — `queue`, `uri`, `project`, `degree`, `request_id`. `queue` has to be there because the envelope deliberately has no queue field and the fact entity does not carry one either, so the event is the only place a cross-queue consumer can see it. `degree` is included even though a hook could resolve it from the store: a fact is immutable, so the usual staleness objection to putting values on an event does not apply, and it is the field a consumer branches on to tell green from broken — the type does not carry that, for the reasons below. +The **subject** is the Request. It is not an envelope field, since the framework keeps subject, queue, and error in the payload, but it is what `id` is minted from and what the event is partitioned by. Choosing the Request over the URI keeps partitioning identical to the `record` topic's own, so per-request ordering carries through the seam, and it gives a consumer a handle back into the pipeline. The two are near-interchangeable anyway: ingest dedups on `(queue, uri)`, so one Request means one URI. -Build failure detail stays off the event. The framework reserves the payload for facts persisted nowhere else, and a failed build's detail is durable on the `Build` row — a hook that wants it resolves the Request and the Build. +The payload carries the fact's identity and value: `queue`, `uri`, `project`, `degree`, `request_id`. `queue` has to be there because neither the envelope nor the fact entity carries one, so the event is the only place a cross-queue consumer sees it. `degree` is there even though a hook could read it from the store: the fact is immutable, so the usual staleness objection does not apply, and it is what a consumer branches on to tell green from broken. Build failure detail stays off, since the framework reserves the payload for facts persisted nowhere else and a failed build's detail is durable on the `Build` row. ### What the `type` carries -`type` is the only dimension a consumer can use to avoid *receiving* an event: the framework rejected a filter API on the grounds that returning nil costs nothing, and defers per-hook isolation to consumer groups on the same topic key. It is also a component of `id`, so whatever goes in it becomes part of the dedupe key. Everything a consumer needs in order to *interpret* an event it already has can be a payload field instead, which is add-only and costs one read. - -Two axes compete for that slot. **Outcome** — green versus broken — is bounded, cheap to branch on, and stops being binary in Phase 2 once a project degree can be fractional and "acceptable" becomes a per-consumer threshold rather than a property of the fact. **Scope** — whole repository versus project — is bounded too, but carries a volume asymmetry of orders of magnitude: one repository fact per commit against one per project per commit. A third candidate, the project id itself, can never be a type at all, since its cardinality is unbounded and caller-defined; project-level consumers filter on the payload whatever the vocabulary looks like. +`type` is the only dimension a consumer can use to avoid *receiving* an event: the framework rejected a filter API (returning nil costs nothing) and leaves per-hook isolation to consumer groups on the same topic key. It is also part of `id`, so it lands in the dedupe key. Anything needed only to *interpret* an event already in hand can be a payload field, which is add-only and costs one read. -That yields the rule: **an axis belongs in the `type` when consumers need to avoid receiving the event, and in the payload when they need to interpret it.** Scope decides whether an event is wanted at all; degree decides what to do with one already in hand. +Two axes compete for the slot. **Outcome** (green vs broken) is bounded and cheap to branch on, but stops being binary in Phase 2, once a project degree can be fractional and "acceptable" is a per-consumer threshold rather than a property of the fact. **Scope** (whole repository vs project) is bounded too, but the volumes differ by orders of magnitude: one repository fact per commit against one per project per commit. The project id can never be a type, since its cardinality is unbounded and caller-defined, so project consumers filter on the payload regardless. -So the type names the scope rather than the outcome — `validation.repository.recorded` in Phase 1, with `validation.project.recorded` beside it in Phase 2. A deploy gate reads `degree` from the payload, which it would have to do regardless once degrees are fractional. Naming the scope while only one of them exists is deliberate: it is the axis that cannot be recovered later, and it keeps Phase 2 from retroactively reinterpreting a bare `validation.green` as "the repository one". +So the rule is: **an axis belongs in `type` when consumers need to avoid receiving the event, and in the payload when they need to interpret it.** The type names the scope, `validation.repository.recorded`, with `validation.project.recorded` beside it in Phase 2, and a deploy gate reads `degree` from the payload, which it has to do anyway once degrees are fractional. Naming the scope while only one exists is the point: it is the axis that cannot be recovered later, it stops Phase 2 from retroactively reinterpreting a bare `validation.green` as "the repository one", and it keeps `id` computable from the request id alone, so an operator republishing from a dead-letter queue never has to read storage to learn which id they would duplicate. -This also keeps `id` computable from the request id alone, since it no longer depends on the fact's value — an operator republishing from a dead-letter queue never has to read storage first to learn which id they would be duplicating. The rejected alternative, `validation.green` / `validation.broken`, is the shape the framework's own examples take (`request.landed`, `batch.failed`), so it is worth confirming with the framework's owner that a scope-shaped vocabulary is acceptable across domains before the first consumer binds to it. +The rejected alternative, `validation.green` / `validation.broken`, is the shape the framework's own examples take (`request.landed`, `batch.failed`), so confirm with the framework's owner that a scope-shaped vocabulary works across domains before the first consumer binds. ### Open questions -**Subject id and the separator rule.** The framework mints `id` from `source` / `type` / subject id / version and says components are separator-free, but every Stovepipe id has slashes in it — a request id is `request//`. Since the framework also says consumers never parse the id, the requirement that actually matters is uniqueness rather than unambiguous decomposition, and uniqueness holds: one request records exactly one whole-repository fact. Worth confirming with the framework's owner rather than assuming, and worth deciding whether the alternative — hashing the composite identity into a single opaque component — is preferable for consistency across domains. +**Subject id and the separator rule.** The framework mints `id` from `source` / `type` / subject id / version and says components are separator-free, but every Stovepipe id has slashes in it: a request id is `request//`. Consumers never parse the id, so what actually matters is uniqueness rather than unambiguous decomposition, and uniqueness holds, since one request records exactly one whole-repository fact. Confirm rather than assume, and decide whether hashing the composite identity into one opaque component is better for consistency across domains. -**Phase 2 fan-out.** One request records N project facts, so the request id alone stops being a unique subject per event. The framework anticipates this case and appends an ordinal for multiple same-typed events per cause; the natural ordinal here is the project id, which is already the part of the identity that varies. Confirm when the project contract lands. +**Phase 2 fan-out.** One request records N project facts, so the request id alone stops being a unique subject per event. The framework appends an ordinal for multiple same-typed events per cause, and the natural ordinal here is the project id, which is already the part of the identity that varies. Confirm when the project contract lands. ### What a consumer can and cannot assume -Ordering is per-subject only, and the subject is the request. Events for *different* requests can therefore arrive out of order, so a consumer must not infer "the newest green commit" from arrival order — it should compare request ids by ingest order (`entity.CompareRequestID`) or read the Queue's bookmark, which is monotonic by construction. +Ordering is per-subject only and the subject is the Request, so events for *different* Requests can arrive out of order. A consumer must not infer "the newest green commit" from arrival order; it should compare request ids by ingest order (`entity.CompareRequestID`) or read the bookmark, which is monotonic by construction. -Absence of an event is not a signal. A cancelled build records no fact and publishes nothing, and a Request abandoned before any build reached a terminal status never reaches this stage at all, so a consumer waiting for an event per ingested commit will wait forever on those. Gating has to keep treating "no recorded fact" as not green. The converse also holds: an event is not proof that the code was tested, since a fail-closed Request can produce a broken fact without a build having failed (see [What fail-closed actually guarantees](#what-fail-closed-actually-guarantees)). +Absence of an event is not a signal: a cancelled build records nothing, and a Request abandoned before any build went terminal never reaches this stage, so a consumer waiting for one event per ingested commit waits forever on those. Gating keeps treating "no recorded fact" as not green. The converse holds too — an event is not proof the code was tested, since a fail-closed Request can produce a broken fact without a build having failed. -Delivery is at-least-once, deduped by `id` within the queue's retention window; hooks must be idempotent on `id`. "Fire-and-forget" describes downstream consumption, not the publish: `record` never waits for a hook to run, but a failed *publish* fails the delivery. Per [`platform/errs`](../../../../platform/errs/README.md) rule 4 that failure is not wrapped retryable just because replaying it is convenient, so it dead-letters — which is the point at which the missing `record_dlq` reconciler below stops being theoretical, since the fact is already durable and only the notification is lost. +Delivery is at-least-once, deduped by `id` within the queue's retention window, so hooks must be idempotent on `id`. "Fire-and-forget" describes downstream consumption, not the publish: `record` never waits for a hook, but a failed *publish* fails the delivery. Per [`platform/errs`](../../../../platform/errs/README.md) rule 4 it is not wrapped retryable just because replaying it is convenient, so it dead-letters, which is where the missing `record_dlq` reconciler stops being theoretical: the fact is durable and only the notification is lost. ## Request lifecycle Phase 1 uses the states in [stovepipe/entity/request.go](../../../../stovepipe/entity/request.go). `record` runs *after* the Request is terminal: `buildsignal` projects the build's terminal status onto it as `succeeded`, `failed`, or `cancelled` (`RequestState.HasBuildOutcome()`), and only then publishes. So `record` reads an outcome and writes no state. `superseded` is terminal without an outcome. -Phase 2 broadens "complete" to mean "all planned facts recorded", which needs a marker this stage does not own — see [Completion marker: open](#completion-marker-open). +Phase 2 broadens "complete" to "all planned facts recorded", which needs a marker this stage does not own; see [Completion marker: open](#completion-marker-open). ## Storage and queue contract `ValidationFactStore` is key/value-shaped, and queue-bound rather than queue-parameterized: - `Create(ctx, fact)` writes one immutable fact for the bound queue. It returns `ErrAlreadyExists` when the composite identity is already taken, leaving the stored fact untouched. -- `Get(ctx, uri, project)` reads one fact by the rest of its identity. It returns `ErrNotFound` when no fact exists. The `project` argument is reserved for Phase 2. +- `Get(ctx, uri, project)` reads one fact by the rest of its identity, and returns `ErrNotFound` when none exists. The `project` argument is reserved for Phase 2. -There is no `Update`: the first fact written for an identity is the permanent answer, and a caller that needs to know whether it won the race reads `ErrAlreadyExists` and then loads the winner. +There is no `Update`. The first fact written for an identity is the permanent answer, and a caller that needs to know whether it won the race reads `ErrAlreadyExists` and then loads the winner. -The topic key, the message, and the consumer all exist; the DLQ consumer does not (see [DLQ and fail-closed behavior](#dlq-and-fail-closed-behavior)). +The topic key, the message, and the consumer all exist. The DLQ consumer does not (see [DLQ and fail-closed behavior](#dlq-and-fail-closed-behavior)). | Topic key | Message | Producer | Consumer | Partition key | Message id | | --------- | ---------------------------------- | ------------- | -------- | ------------- | ---------- | @@ -250,17 +225,6 @@ The payload carries the request id plus the queue name, so the consumer can reso Partitioning by request id keeps completion bookkeeping single-writer per Request, and reusing the request id as the message id dedups a redelivered signal into the original message instead of enqueuing a second one. -## Idempotency and competing outcomes - -- **Request not visible** — a storage defect rather than a lag, since the publish follows the committed outcome write. Non-retryable. -- **Fact already created** — load it and continue from the authoritative fact. -- **Bookmark already advanced** — the guard skips equal-or-older candidates and the write is skipped. A Request that finds its *own* id there still counts as holding the bookmark, so the promotion behind it is retried. -- **Commit already promoted** — `Promote` is idempotent, so repeating it changes nothing. -- **Duplicate builds for one Request** — absorbed a stage earlier: `buildsignal`'s outcome write is first-writer-wins, so the Request carries one immutable verdict and `record` never sees a competing one. -- **Redelivery after a complete run** — every effect is recognize-and-skip. Once hooks land the publish re-fires, and the framework's dedupe on `id` absorbs it. - -An existing fact from a different Request, or a Request carrying no build outcome, is an invariant violation rather than an expected control-flow outcome. - ## Error classification | Failure | Disposition | Reason | @@ -273,77 +237,77 @@ An existing fact from a different Request, or a Request carrying no build outcom | Existing fact owned by a different Request | non-retryable | ingest dedup invariant violated; the stored fact is immutable | | Malformed request id at bookmark comparison | non-retryable | re-parsing the same ids cannot succeed | | Queue CAS version mismatch | absorbed, not raised | `storage.ErrVersionMismatch` is handled by the bookmark loop: reload and re-apply | -| Promotion target unknown to the ref | absorbed, not raised | `sourcecontrol.ErrNotFound` means a rewritten history dropped the commit; counted and acked | +| Promotion target unknown to the ref | absorbed, not raised | `sourcecontrol.ErrNotFound` means a rewritten history dropped the commit | | SourceControl resolution or `Promote` failure | raw error | backend classifier has the required failure knowledge | | Commit-timestamp lookup for a metric | swallowed | counted and logged; observability must not retry a recorded outcome | | ValidationFactStore, QueueStore, RequestStore | raw error | backend classifier has the required failure knowledge | -## DLQ and fail-closed behavior +## Edge cases and idempotency -**Neither `record_dlq` nor `build_dlq` has a consumer today, and both topics are already receiving messages.** Every primary subscription is created from `DefaultSubscriptionConfig`, which enables dead-lettering with the `_dlq` suffix, so a message that is rejected outright *or* runs out of retries is moved to its stage's dead-letter topic — but the wiring registers only `process_dlq` and `buildsignal_dlq`. Messages accumulate unread on the other two. +Every effect is recognize-and-skip, so a redelivery after a complete run re-runs each step as a no-op. Once hooks land the publish re-fires, and the framework's dedupe on `id` absorbs it. -Two different things put a message there, and only one of them is a poison payload. A delivery that fails once its retry budget is spent is dead-lettered by the nack itself, carrying the reason it actually failed. A delivery that never reaches a nack at all — a crash, or an **ack that fails** and lets the visibility timeout redeliver — is dead-lettered by the poll loop instead, as soon as `retry_count` reaches `MaxAttempts` (3 by default), without the controller being invoked on that final attempt and with only a generic reason recorded. So what a missing reconciler exposes is wider than malformed messages: a fact can be lost to a storage failure that would have succeeded on a later retry, or to an ack that never landed even though the write did. +- **Request not visible.** A storage defect rather than lag, since the publish follows the committed outcome write. Non-retryable. +- **Fact already created.** Load it and continue from the stored fact. A fact from a *different* Request, or a Request carrying no build outcome, is an invariant violation rather than an expected outcome. +- **Duplicate builds for one Request.** Absorbed a stage earlier: `buildsignal`'s outcome write is first-writer-wins, so the Request carries one immutable verdict and `record` never sees a competing one. +- **Head equals last-green.** The build still produces a terminal verdict. A green result may share the bookmark's URI, but the guard compares request ids rather than URIs, so an equal-or-older candidate skips and nothing regresses. +- **Green fact recorded out of order across Requests.** An older green commit can reach this stage after a newer one. Its fact is written as usual, since facts are per-URI and independent, but the bookmark guard skips it, and because it does not hold the bookmark it does not promote either. Neither cache moves backwards. +- **Cancelled build.** Stovepipe never initiates cancellation, but a backend may still report it. No fact is written; the slot was already released and the Request already stamped `cancelled`. The fact identity stays unclaimed, and nothing can claim it today, since `cancelled` is terminal and re-validation does not exist. Recovery in practice is the next commit; the unclaimed identity only matters to a future re-run mechanism (see [Supporting re-run of the same URI](#supporting-re-run-of-the-same-uri)). +- **History rewrite while a build runs.** The Request keeps the strategy and URI pinned at admission, and record stores the fact about that immutable URI. A later head is handled independently by `process`. If the rewrite dropped the commit from the ref, the fact and the bookmark still stand, since they describe a commit and not a ref, and only the promotion is skipped. +- **A fail-closed terminal outranks a build that passed.** The degree derives from `R.State`, so a Request forced to `failed` by DLQ reconciliation records `DegreeBroken` even when one of its builds reports success afterwards. Reachable today, and permanent once written; see [What fail-closed actually guarantees](#what-fail-closed-actually-guarantees). +- **Crash between the fact write and the bookmark advance.** Redelivery reloads the existing fact and re-applies the idempotent guard. +- **Crash between the bookmark advance and the promotion.** Redelivery finds its own id on the bookmark, reports that it holds it, and retries the idempotent promotion. -Gating stays safe, because everything this stage can lose reads as not-green: a request with no fact is indistinguishable from one not yet validated, and callers must already treat absence as not green (see [When to record an outcome](#when-to-record-an-outcome)). What is lost instead is the *fact* — a green build whose fact write permanently failed leaves the URI looking unvalidated, which costs the queue an incremental baseline and forces a full build at the next head. Once hooks land, a lost notification joins that list, and unlike the fact it has no second chance from a later commit. +## DLQ and fail-closed behavior -This is the same failure shape [buildsignal.md](buildsignal.md#what-it-costs-when-a-backend-does-not-classify-status-errors) describes for a deployment that registers primary consumers without their reconciler. When the reconciler is built, it should re-run the same idempotent record algorithm from the request id under `errs.AlwaysRetryableProcessor`: +**Neither `record_dlq` nor `build_dlq` has a consumer today, and both topics are already receiving messages.** Every primary subscription comes from `DefaultSubscriptionConfig`, which enables dead-lettering with the `_dlq` suffix, so a message that is rejected outright *or* runs out of retries moves to its stage's dead-letter topic. The wiring registers only `process_dlq` and `buildsignal_dlq`, so messages pile up unread on the other two. -- If the Request carries a build outcome, write and publish its immutable fact as usual. -- If Request storage is temporarily unavailable, keep retrying. -- If the payload is malformed or the Request is permanently missing, the message is poison: there is no trustworthy identity to act on, so it needs an operational alert rather than more retries. +Two different things put a message there, and only one is a poison payload. A delivery that fails with its retry budget spent is dead-lettered by the nack itself, carrying the reason it actually failed. A delivery that never reaches a nack, because it crashed or because its **ack failed** and the visibility timeout redelivered it, is dead-lettered by the poll loop once `retry_count` reaches `MaxAttempts` (3 by default), without the controller running on that final attempt and with only a generic reason recorded. So a missing reconciler exposes more than malformed messages: a fact can be lost to a storage failure that would have succeeded on a later retry, or to an ack that never landed even though the write did. + +Gating stays safe, because everything this stage can lose reads as not-green: a Request with no fact is indistinguishable from one not yet validated. What is lost is the *fact*. A green build whose fact write permanently failed leaves the URI looking unvalidated, which costs the queue an incremental baseline and forces a full build at the next head. Once hooks land, a lost notification joins that list, and unlike the fact it gets no second chance from a later commit. + +This is the same failure shape [buildsignal.md](buildsignal.md#what-it-costs-when-a-backend-does-not-classify-status-errors) describes for a deployment that registers primary consumers without their reconciler. When the reconciler is built it should re-run this same idempotent algorithm from the request id, under `errs.AlwaysRetryableProcessor`: write and publish the immutable fact as usual if the Request carries a build outcome, keep retrying if Request storage is temporarily unavailable, and treat a malformed payload or a permanently missing Request as poison, which needs an operational alert rather than more retries. ### What fail-closed actually guarantees Fail-closed reconciliation guarantees that a Request which can never complete ends up in a state gating reads as not-green. It does **not** guarantee that this stage never runs for such a Request, and the difference is reachable rather than theoretical. -A reconciler forcing `failed` publishes nothing here, but it also does nothing to stop a publish. The two are decoupled: `buildsignal` calls its record publish unconditionally once a build reaches a terminal status, outside the guard that makes its state write idempotent. A Request that already carries an outcome is admitted rather than dropped — deliberately, so a redelivery after the outcome was stamped but before the publish landed still reaches `record` — which means a forced `failed` is carried into a fact by any delivery that arrives afterwards. +A reconciler forcing `failed` publishes nothing here, but it does nothing to stop a publish either. `buildsignal` publishes to `record` unconditionally once a build goes terminal, outside the guard that makes its state write idempotent, and a Request that already carries an outcome is admitted rather than dropped. That is on purpose, so a redelivery after the outcome was stamped but before the publish landed still reaches `record`. It also means a forced `failed` gets carried into a fact by any delivery arriving afterwards. The concrete path starts with a `build` delivery whose ack fails: -1. `build` does its work — triggers the runner, persists the `Build`, publishes the signal — and then fails to ack. The visibility timeout redelivers it, and the controller triggers *again*, minting a fresh build id and persisting another `Build` row. One Request accumulates as many independent poll chains as it had deliveries, each partitioned by its own build id. +1. `build` triggers the runner, persists the `Build`, publishes the signal, then fails to ack. The visibility timeout redelivers it and the controller triggers *again*, minting a fresh build id and another `Build` row. One Request accumulates as many independent poll chains as it had deliveries, each partitioned by its own build id. 2. The retry budget runs out and the poll loop dead-letters the build message to `build_dlq`, where nothing consumes it. -3. The poll chains are untouched by that. They re-poll with `Hold`, which resets `retry_count` and exempts the redelivery from the increment, so a chain polls for as long as its build takes without ever spending its own budget. The chains outlive the build message rather than dying with it. -4. One chain does eventually exhaust its retries — a `Status` call that stays broken through every attempt, say — and dead-letters to `buildsignal_dlq`. That reconciler releases the slot and forces the Request to `failed`, correctly, since nothing will look at *that* build again. +3. The poll chains are untouched by that. They re-poll with `Hold`, which resets `retry_count` and exempts the redelivery from the increment, so a chain polls for as long as its build takes without spending its own budget. The chains outlive the build message rather than dying with it. +4. One chain does eventually exhaust its retries, say a `Status` call that stays broken through every attempt, and dead-letters to `buildsignal_dlq`. That reconciler releases the slot and forces the Request to `failed`, correctly, since nothing will look at *that* build again. 5. Another chain later polls a terminal **success**. Its terminal write is a no-op because the Request already carries an outcome, and it then publishes here. 6. `record` loads a `failed` Request and writes `DegreeBroken` for a commit that actually built green. -The fact is immutable and first-fact-wins, so that verdict is permanent. Gating is still safe in the direction that matters — the error is toward not-green, never falsely green — and the cost is bounded, because the next head re-establishes greenness on its own Request. But it is a strictly worse outcome than the one the fail-closed posture was reasoned about: not "we never learned the outcome" but "we observed success and recorded broken anyway", and it also denies the queue an incremental baseline it had earned. See [Whether observed success should outrank a forced failure](#whether-observed-success-should-outrank-a-forced-failure-open). - -Note where the forced failure actually comes from. Not from the `build` dead-letter, which today does nothing whatsoever, because `build_dlq` has no reconciler: had step 4 not happened, the Request would have stayed `processing` until a surviving chain stamped the real outcome, and the fact would have been green. Wiring a reconciler onto that orphaned topic — the obvious fix for it — moves the forced `failed` up to step 2 and makes a *single* surviving poll chain sufficient to record a broken fact for a green commit. That is the sharper reason to treat the two gaps as one piece of work: closing `build_dlq` in isolation widens this hole rather than narrowing it. [build.md](build.md#fail-closed-interaction) covers the other half of that gap, where the same missing reconciler costs the Queue a build slot. +The fact is immutable and first-fact-wins, so that verdict is permanent. Gating is still safe in the direction that matters, since the error is toward not-green and never falsely green, and the cost is bounded because the next head re-establishes greenness on its own Request. But it is worse than the case the fail-closed posture was reasoned about. This is not "we never learned the outcome", it is "we observed success and recorded broken anyway", and it also denies the queue an incremental baseline it had earned. See [Whether observed success should outrank a forced failure](#whether-observed-success-should-outrank-a-forced-failure-open). -## Edge cases - -- **Head equals last-green.** The build still produces a terminal verdict. A green result may share the bookmark's URI; the guard compares request ids, not URIs, so an equal-or-older candidate skips and nothing regresses. -- **Cancelled build.** Stovepipe never initiates cancellation, but a backend may still report it. No fact is written; the slot was already released and the Request already stamped `cancelled`. The fact identity stays unclaimed, but nothing can claim it today — `cancelled` is a terminal state and re-validation does not exist — so in practice recovery is the next commit; the unclaimed identity only matters to a future re-run mechanism (see [Supporting re-run of the same URI](#supporting-re-run-of-the-same-uri)). -- **History rewrite while a build runs.** The Request keeps the strategy and URI pinned at admission; record stores the fact about that immutable URI. A later head is handled independently by `process`. If the rewrite dropped the commit from the ref, the fact and the bookmark still stand — they describe a commit, not a ref — and only the promotion is skipped. -- **A fail-closed terminal outranks a build that actually passed.** The degree derives from `R.State`, so a Request forced to `failed` by DLQ reconciliation records `DegreeBroken` even when one of its builds reports success afterwards. Reachable today, and permanent once written; see [What fail-closed actually guarantees](#what-fail-closed-actually-guarantees). -- **Green fact recorded out of order across Requests.** An older green commit can reach this stage after a newer one. Its fact is written as usual — facts are per-URI and independent — but the bookmark guard skips it and, because it does not hold the bookmark, it does not promote either, so neither cache moves backwards. -- **Crash between the fact write and the bookmark advance.** Redelivery reloads the existing fact and re-applies the idempotent guard. -- **Crash between the bookmark advance and the promotion.** Redelivery finds its own id on the bookmark, reports that it holds it, and retries the idempotent promotion. -- **Ack fails after a complete run.** Redelivery re-runs every step as a no-op. +Note where the forced failure comes from. Not the `build` dead-letter, which today does nothing at all, because `build_dlq` has no reconciler: without step 4 the Request would have stayed `processing` until a surviving chain stamped the real outcome, and the fact would have been green. Wiring a reconciler onto that orphaned topic, the obvious fix for it, moves the forced `failed` up to step 2 and makes a *single* surviving poll chain enough to record a broken fact for a green commit. So the two gaps are one piece of work: closing `build_dlq` on its own widens this hole instead of narrowing it. [build.md](build.md#fail-closed-interaction) covers the other half, where the same missing reconciler costs the Queue a build slot. ## Deferred and open ### Whether observed success should outrank a forced failure: open -[workflow.md](../workflow.md#fail-closed-on-unprocessable-work) states that the conservative terminal outcome is *final, not provisional*. That was reasoned about a Request whose outcome was never observed, where `failed` is the only honest answer available. The path in [What fail-closed actually guarantees](#what-fail-closed-actually-guarantees) produces a different case, where a successful build was observed and loses to a forced failure anyway. +[workflow.md](../workflow.md#fail-closed-on-unprocessable-work) says the conservative terminal outcome is *final, not provisional*. That was reasoned about a Request whose outcome was never observed, where `failed` is the only honest answer available. The path above is a different case: a successful build was observed, and it loses to a forced failure anyway. -The underlying question is what a validation fact asserts. If it is **evidence about the code**, a reconciler-forced failure has no business becoming one: it is evidence about the pipeline, and recording it as a broken commit is a category error. If it is **the pipeline's last word on a Request**, the current behaviour is correct by definition and the false-broken fact is the price of never being falsely green. +The real question is what a validation fact asserts. If it is **evidence about the code**, a reconciler-forced failure should not become one, because it is evidence about the pipeline instead. If it is **the pipeline's last word on a Request**, the current behavior is correct by definition and the false-broken fact is the price of never being falsely green. Four ways forward, none free: -- **Leave it.** The immutability story stays unqualified and no new state is needed. The cost is permanent false-broken facts on commits with nothing wrong with them, plus the incremental baseline the queue had earned. Defensible if the incident rate is low enough — and it is measurable, since the reconciler already counts what it forces. -- **Do not publish here when the terminal write was a no-op.** Rejected: `buildsignal` cannot distinguish "the outcome was already forced" from "I stamped it and crashed before publishing", and the second case depends on republishing to avoid losing the fact entirely. -- **Let a later observed success reopen the outcome.** Requires the Request to record *why* it failed, which it does not today, and it contradicts first-fact-wins unless the fact write is deferred until the Request can no longer change — which is exactly the durable completion marker this stage still lacks. -- **Record no fact from a forced failure.** Requires the same "why" on the Request, but trades a wrong fact for an absent one, which gating already treats as not-green and which a re-run mechanism could later fill. Closest to the evidence reading, and it leaves the identity unclaimed rather than permanently wrong. +- **Leave it.** No new state, and the immutability story stays simple. The cost is permanent false-broken facts on commits with nothing wrong with them, plus the incremental baseline the queue had earned. Defensible if the incident rate is low enough, which is measurable, since the reconciler already counts what it forces. +- **Do not publish here when the terminal write was a no-op.** Rejected: `buildsignal` cannot tell "the outcome was already forced" from "I stamped it and crashed before publishing", and the second case depends on republishing to avoid losing the fact entirely. +- **Let a later observed success reopen the outcome.** Needs the Request to record *why* it failed, which it does not today, and it contradicts first-fact-wins unless the fact write waits until the Request can no longer change. That is exactly the durable completion marker this stage still lacks. +- **Record no fact from a forced failure.** Needs the same "why" on the Request, but trades a wrong fact for an absent one, which gating already treats as not-green and a re-run mechanism could later fill. Closest to the evidence reading, and it leaves the identity unclaimed rather than permanently wrong. -The last two both wait on the Request carrying its failure cause, so this is best decided alongside [Supporting re-run of the same URI](#supporting-re-run-of-the-same-uri) and the [completion marker](#completion-marker-open) rather than on its own. +The last two both wait on the Request carrying its failure cause, so this is best decided alongside [Supporting re-run of the same URI](#supporting-re-run-of-the-same-uri) and the [completion marker](#completion-marker-open). ### Completion marker: open -`record` makes no `Request` write in Phase 1, which leaves the stage with no durable marker saying it finished. The consequence is contained today, because every effect is idempotent and a redelivery simply re-runs them, but Phase 2 needs a marker: "all planned facts recorded" has to live somewhere. An earlier draft inserted a non-terminal `analyzing` state between `processing` and the recorded states; that no longer fits, because `buildsignal` drives the Request terminal before `record` ever runs, leaving no non-terminal window to occupy. Phase 2 must either move the outcome write back behind analysis, add a separate completion entity, or track fan-out on the facts themselves. Deferred to `analyze.md`. +`record` makes no `Request` write in Phase 1, so the stage has no durable marker saying it finished. That is contained today, because every effect is idempotent and a redelivery just re-runs them, but Phase 2 needs a marker: "all planned facts recorded" has to live somewhere. An earlier draft inserted a non-terminal `analyzing` state between `processing` and the recorded states. That no longer fits, because `buildsignal` drives the Request terminal before `record` ever runs, leaving no non-terminal window to occupy. Phase 2 has to either move the outcome write back behind analysis, add a separate completion entity, or track fan-out on the facts themselves. Deferred to `analyze.md`. -One argument for a marker has since gone away: the worry that a redelivery with nothing to check against would re-fire the notification every time. The hook framework answers it directly, deduping on the event `id` so a replay collapses into the original event rather than becoming a second one. +One argument for a marker has gone away: the worry that a redelivery with nothing to check against would re-fire the notification every time. The hook framework answers that by deduping on the event `id`, so a replay collapses into the original event. ### Supporting re-run of the same URI @@ -351,18 +315,18 @@ Widen the key with the `RequestID` the facts already record, so each attempt is ### Coverage of intermediate commits -Coalescing means most commits never become a validated Request: a verdict on head `H` with base `B` is implicitly a verdict on every commit in the range `(B, H]`. Downstream tooling still needs to be able to retrieve prior and next green for any commit, including those never ingested directly, or that have been superseded. +Coalescing means most commits never become a validated Request: a verdict on head `H` with base `B` is implicitly a verdict on every commit in the range `(B, H]`. Downstream tooling still needs prior and next green for any commit, including commits never ingested directly and commits that were superseded. The rough idea is to track this with additional stores written during this stage, to be expanded in a separate doc: - **`CoverageStore`** — on every verdict, green or failed, one row per commit in the covered range: `(queue, uri)` → the covering request and the commit's position within its range. Gives commits with no Request of their own a place in the queue's history. -- **`GreenLogStore`** — on green verdicts only, one row keyed by that position. Because the key is ordered, "previous green" and "next green" become two seeks: the nearest entry below or above a commit's position. +- **`GreenLogStore`** — on green verdicts only, one row keyed by that position. The key is ordered, so "previous green" and "next green" become two seeks: the nearest entry below or above a commit's position. -Neither fits `ValidationFactStore`: facts are looked up by exact identity and URIs do not sort, while previous/next-green needs an ordered seek over positions, a different key shape. A genuinely needed reverse lookup getting its own first-class store is the established pattern here — `RequestURIStore` is the existing example (see [storage README](../../../../stovepipe/extension/storage/README.md#key-value-contract)). +Neither fits `ValidationFactStore`. Facts are looked up by exact identity and URIs do not sort, while previous/next-green needs an ordered seek over positions, a different key shape. A genuinely needed reverse lookup getting its own first-class store is the established pattern here; `RequestURIStore` is the existing example (see [storage README](../../../../stovepipe/extension/storage/README.md#key-value-contract)). ### Phase 2 -- **Pipeline**: `record` publishes the request id onward to `analyze`. It does this for green and not-green facts alike, because a failed build is exactly when project attribution matters most. An earlier draft had `record` retarget the Request from `processing` to `analyzing`, with `analyze` owning the terminal transition. That no longer fits, because the Request is already terminal before `record` runs. So tracking "all facts recorded" belongs to the `analyze` design (see [Completion marker: open](#completion-marker-open)). The message onward carries the request id and queue name, exactly as `record`'s own does, and the consumer stays idempotent. +- **Pipeline**: `record` publishes the request id onward to `analyze`, for green and not-green facts alike, because a failed build is when project attribution matters most. An earlier draft had `record` retarget the Request from `processing` to `analyzing`, with `analyze` owning the terminal transition. That no longer fits, because the Request is already terminal before `record` runs, so tracking "all facts recorded" belongs to the `analyze` design (see [Completion marker: open](#completion-marker-open)). The message onward carries the request id and queue name, exactly as `record`'s own does, and the consumer stays idempotent. - **Project facts**: `record` runs the same create-fact-then-notify flow. The fact is keyed by the stable project id carried on the per-project signal, and one `validation.project.recorded` event is published per project identity (see [What the `type` carries](#what-the-type-carries)). Each per-project signal needs its own message id (see [Storage and queue contract](#storage-and-queue-contract)). The Queue's `last_green_uri` describes the whole repository and stays untouched. Deciding project identity, retrieving the target graph, tracking completion, and defining intermediate degrees belong to `analyze.md`. From 64dc349936b152744e2b26235a13d3e3de507e53 Mon Sep 17 00:00:00 2001 From: mnoah1 Date: Thu, 13 Aug 2026 21:08:36 +0000 Subject: [PATCH 7/7] docs(stovepipe): align record's hook section with the hooks framework RFC Stop restating what hook-framework.md already settles (envelope reasoning, delivery promise, dispatcher, rejected alternatives) and link to it instead, leaving only the decisions Stovepipe owns for validation facts. Flag one conflict with the framework: it mints the event id with the causal message id standing in when there is no version, and a fact is create-only. Taken literally that puts the record message's id on the tail, but buildsignal republishes to record on redelivery, so one Request can yield two record messages and one fact would be emitted as two distinct events. Mint from the request id alone and confirm the deviation with the framework's owner. --- doc/rfc/stovepipe/steps/record.md | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/doc/rfc/stovepipe/steps/record.md b/doc/rfc/stovepipe/steps/record.md index 45b8a4c2..50ff4dca 100644 --- a/doc/rfc/stovepipe/steps/record.md +++ b/doc/rfc/stovepipe/steps/record.md @@ -148,9 +148,11 @@ Both are best-effort. Every way the lookup can fail is counted, logged, and swal ## Hooks -Recording a fact is when the rest of the company can learn "this URI is now green (or not green)". `record` publishes that event but does **not** call a notification extension inline. Delivery rides the cross-domain hook framework in [hook-framework.md](../../hook-framework.md): the stage publishes a `HookEvent` to Stovepipe's durable `hook` topic, and a per-domain dispatcher stage consumes it and invokes whichever hooks the host wired. The earlier design here, a Stovepipe `Hooks` extension with a `Notify(ctx, ValidationFactRef{…})` call as the last algorithm step, is rejected by the framework on both halves: an inline call ties pipeline latency to third-party integrations and drops the notification on a crash between the state write and the call, and a per-domain contract multiplies schemas and sinks for no gain. +Recording a fact is when the rest of the company can learn "this URI is now green (or not green)". `record` publishes that as a `HookEvent` on Stovepipe's durable `hook` topic; it does not call a notification extension inline. -None of this is built. It needs the shared `HookEvent` contract under `api/base/hook/`, the hook extension at `platform/extension/hook/`, and Stovepipe's own share: a `hook` topic key, a dispatcher stage, a `hook_dlq` reconciler, and the wiring for all three. +The mechanics are already settled in [hook-framework.md](../../hook-framework.md) — the envelope, the delivery promise, the per-domain dispatcher stage, the `hook_dlq`, and the reasoning behind each. This section covers only what Stovepipe has to decide for validation facts. The earlier design here, a Stovepipe `Hooks` extension called with `Notify(ctx, ValidationFactRef{…})` as the last algorithm step, is rejected there on both halves: inline calls couple pipeline latency to third-party integrations and drop the notification on a crash between the state write and the call, and a per-domain contract multiplies schemas and sinks for no gain. + +None of it is built. It needs the shared `HookEvent` contract at `api/base/hook/` and the hook extension at `platform/extension/hook/`, plus Stovepipe's own share: a `hook` topic key, a dispatcher stage, a `hook_dlq` reconciler, and the wiring for all three. ### Where the publish belongs @@ -160,7 +162,7 @@ Last thing before the ack, after the fact write and after both caches derived fr create fact → advance bookmark (green only) → promote (bookmark holder only) → publish HookEvent → [Phase 2: publish to analyze] → ack ``` -It goes last because the framework forbids entity snapshots in the payload, so hooks resolve entities from stores. A hook reacting to "URI is green" by reading `LastGreenURI`, or by fetching the promotion ref, must not find either one still pointing at the previous commit. Keeping it inside the same delivery is what makes it lossless without an outbox: the state writes are recognize-and-skip on redelivery, so a crash before the ack replays the whole chain and re-publishes. +Last because the payload carries no entity snapshot and hooks resolve entities from stores: a hook reacting to "URI is green" by reading `LastGreenURI`, or by fetching the promotion ref, must not find either still pointing at the previous commit. Inside the delivery rather than after it, because that is what makes the event lossless without an outbox — the state writes are recognize-and-skip on redelivery, so a crash before the ack replays the whole chain. ### Event shape @@ -170,15 +172,15 @@ It goes last because the framework forbids entity snapshots in the payload, so h | `type` | `validation.repository.recorded` (see below) | | `version` | `0` — a fact is create-only and has no version to report | | `timestamp_ms` | Publish time; the fact's own `CreatedAt` travels in the payload | -| `id` | `source` / `type` / request id / `0` | +| `id` | `source` / `type` / request id (see below) | -The **subject** is the Request. It is not an envelope field, since the framework keeps subject, queue, and error in the payload, but it is what `id` is minted from and what the event is partitioned by. Choosing the Request over the URI keeps partitioning identical to the `record` topic's own, so per-request ordering carries through the seam, and it gives a consumer a handle back into the pipeline. The two are near-interchangeable anyway: ingest dedups on `(queue, uri)`, so one Request means one URI. +The **subject** is the Request — a payload fact rather than an envelope field, but it is what `id` is minted from and what the event partitions on. The Request over the URI keeps partitioning identical to the `record` topic's own, so per-request ordering carries through the seam, and it hands a consumer a way back into the pipeline. The two are near-interchangeable anyway: ingest dedups on `(queue, uri)`, so one Request means one URI. -The payload carries the fact's identity and value: `queue`, `uri`, `project`, `degree`, `request_id`. `queue` has to be there because neither the envelope nor the fact entity carries one, so the event is the only place a cross-queue consumer sees it. `degree` is there even though a hook could read it from the store: the fact is immutable, so the usual staleness objection does not apply, and it is what a consumer branches on to tell green from broken. Build failure detail stays off, since the framework reserves the payload for facts persisted nowhere else and a failed build's detail is durable on the `Build` row. +The payload carries the fact's identity and value: `queue`, `uri`, `project`, `degree`, `request_id`. `queue` has to be there because neither the envelope nor the fact entity carries one, so the event is the only place a cross-queue consumer sees it. `degree` is there even though a hook could read it from the store: the fact is immutable, so the staleness objection behind the no-snapshots rule does not apply, and it is what a consumer branches on to tell green from broken. Build failure detail stays off, since the payload is reserved for facts persisted nowhere else and a failed build's detail is durable on the `Build` row. ### What the `type` carries -`type` is the only dimension a consumer can use to avoid *receiving* an event: the framework rejected a filter API (returning nil costs nothing) and leaves per-hook isolation to consumer groups on the same topic key. It is also part of `id`, so it lands in the dedupe key. Anything needed only to *interpret* an event already in hand can be a payload field, which is add-only and costs one read. +`type` is the framework's single filter dimension, so it is the only thing a consumer can use to avoid *receiving* an event — there is no filter API, and per-hook isolation waits on consumer groups sharing the topic key. It is also part of `id`, so it lands in the dedupe key. Anything needed only to *interpret* an event already in hand can be a payload field, which is add-only and costs one read. Two axes compete for the slot. **Outcome** (green vs broken) is bounded and cheap to branch on, but stops being binary in Phase 2, once a project degree can be fractional and "acceptable" is a per-consumer threshold rather than a property of the fact. **Scope** (whole repository vs project) is bounded too, but the volumes differ by orders of magnitude: one repository fact per commit against one per project per commit. The project id can never be a type, since its cardinality is unbounded and caller-defined, so project consumers filter on the payload regardless. @@ -188,7 +190,9 @@ The rejected alternative, `validation.green` / `validation.broken`, is the shape ### Open questions -**Subject id and the separator rule.** The framework mints `id` from `source` / `type` / subject id / version and says components are separator-free, but every Stovepipe id has slashes in it: a request id is `request//`. Consumers never parse the id, so what actually matters is uniqueness rather than unambiguous decomposition, and uniqueness holds, since one request records exactly one whole-repository fact. Confirm rather than assume, and decide whether hashing the composite identity into one opaque component is better for consistency across domains. +**The `id` tail.** The framework mints `id` from `source` / `type` / subject id / post-transition version, with the causal message id standing in when there is no version. A fact is create-only, so there is no version, and taken literally that puts the `record` message's id on the end. That is wrong for this stage: `buildsignal` republishes to `record` on redelivery, so one Request can produce two `record` messages with different ids, and minting from them would emit one fact as two distinct events. Dropping the tail dedups correctly, because one Request records exactly one whole-repository fact. Confirm the deviation with the framework's owner. + +**Separator-free components.** The framework says `id` components carry no separators, but every Stovepipe id has slashes in it: a request id is `request//`. Consumers never parse the id, so what matters is uniqueness rather than unambiguous decomposition, and uniqueness holds. Confirm rather than assume, and decide whether hashing the composite identity into one opaque component is better for consistency across domains. **Phase 2 fan-out.** One request records N project facts, so the request id alone stops being a unique subject per event. The framework appends an ordinal for multiple same-typed events per cause, and the natural ordinal here is the project id, which is already the part of the identity that varies. Confirm when the project contract lands. @@ -198,7 +202,7 @@ Ordering is per-subject only and the subject is the Request, so events for *diff Absence of an event is not a signal: a cancelled build records nothing, and a Request abandoned before any build went terminal never reaches this stage, so a consumer waiting for one event per ingested commit waits forever on those. Gating keeps treating "no recorded fact" as not green. The converse holds too — an event is not proof the code was tested, since a fail-closed Request can produce a broken fact without a build having failed. -Delivery is at-least-once, deduped by `id` within the queue's retention window, so hooks must be idempotent on `id`. "Fire-and-forget" describes downstream consumption, not the publish: `record` never waits for a hook, but a failed *publish* fails the delivery. Per [`platform/errs`](../../../../platform/errs/README.md) rule 4 it is not wrapped retryable just because replaying it is convenient, so it dead-letters, which is where the missing `record_dlq` reconciler stops being theoretical: the fact is durable and only the notification is lost. +Hooks here must be idempotent on `id`, as everywhere. "Fire-and-forget" describes downstream consumption, not the publish: `record` never waits for a hook, but a failed *publish* fails the delivery. Per [`platform/errs`](../../../../platform/errs/README.md) rule 4 it is not wrapped retryable just because replaying it is convenient, so it dead-letters, which is where the missing `record_dlq` reconciler stops being theoretical: the fact is durable and only the notification is lost. ## Request lifecycle