diff --git a/doc/rfc/stovepipe/steps/build.md b/doc/rfc/stovepipe/steps/build.md new file mode 100644 index 00000000..c4855194 --- /dev/null +++ b/doc/rfc/stovepipe/steps/build.md @@ -0,0 +1,339 @@ +# Build stage + +`build` triggers the build-runner for the scope `process` already decided and hands the resulting build id to `buildsignal`. See [workflow.md](../workflow.md) for where it sits in the pipeline, and [process.md](process.md) for how the scope it reads (`BuildStrategy`, `BaseURI`) is chosen. + +It handles only the trigger: it does not poll for completion, record greenness, or decide incremental-vs-full — those are `buildsignal`'s, `record`'s, and `process`'s jobs respectively. + +`build` is structurally the same controller as `submitqueue/orchestrator/controller/build/build.go`, and [doc/rfc/submitqueue/build-runner.md](../../submitqueue/build-runner.md) is the reference rationale for the trigger-then-poll shape this stage reuses. The `BuildRunner` contract's `Status`/`Cancel` half now carries over as literally shared code with SubmitQueue, via `platform/extension/buildrunner`; what does *not* carry over is `Trigger` — the two domains model different problems, so each keeps its own, following the [extension-contract.md](../../submitqueue/extension-contract.md) "identity in, resolve internally" principle. + +## Input, partitioning, and the single-writer property + +`build` consumes a request id, published by `process` in Phase 1 (see [workflow.md](../workflow.md#workflow)). Both phases drive the same `build` → `buildsignal` machinery against the same `Request` row. The `process`/`analyze` → `build` topic is partitioned by **request id**; see [Partitioning](#partitioning) for the full rationale, including why `build` → `buildsignal` partitions finer (by build id) than SubmitQueue's equivalent topic. + +**`build` is not the sole writer of the rows it touches, and its own writes are narrow.** On `Request`, `build` never writes at all — it only reads the `BuildStrategy`/`BaseURI`/`URI` fields `process` set at admit, and it leaves `Request.State` untouched at `processing` throughout (`process` and `record` are `Request.State`'s only writers). On `Build`, `build` is the sole creator — it calls `BuildStore.Create` exactly once, at step 6 — and never mutates the row again; `buildsignal` is the sole writer of `Build.Status`/`Build.Version` afterward (see [buildsignal.md](buildsignal.md#input-and-re-entrancy)). This division is why `build` never needs a CAS/version write of its own: `Create` is the only storage mutation in its algorithm. + +`build` is phase-agnostic: it never asks "which phase is this?" It reads whatever scope is already persisted and immutable on the `Request` and acts on it. Phase 2's project-scoped invocation is expected to read project-scoped equivalents of the scope fields off the same `Request`; the exact shape of a project-scoped trigger is left to the `analyze` design, consistent with `workflow.md`'s "project mapping contract" open question. + +In both phases `build` is strictly the trigger — `buildsignal` owns polling — so a crash between `Trigger` and the publish leaves a build no poller ever picks up; the redelivery triggers a replacement and the orphan is accepted waste, mirroring SubmitQueue (see [Idempotency](#idempotency)). + +## Algorithm + +For a delivery carrying request id `R`: + +``` +1. Load Request R from the request store. + - ErrNotFound -> retryable (process/analyze write not visible yet; redelivery converges). + - other store error -> return raw; classifier decides. + +2. If R.State is terminal (superseded / recorded-green / recorded-not-green): ack and return. + - a redelivery after record already finished, or after process superseded R, must not start a + fresh build. + +3. Resolve the build-runner for R's Queue: buildRunner = Factory.For(Config{QueueName: R.Queue}). + - lookup failure -> non-retryable (a queue with no builder is a config error). + +4. Read the already-decided scope off R: R.BuildStrategy, R.BaseURI, R.URI. + - build never re-derives incremental-vs-full; process decided it (see process.md). + - BuildStrategy unset (process's CAS not visible on this reader yet) -> retryable, like step 1. + Do NOT default to a full build; converge on redelivery once the write lands. + - 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) + - 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). + - there is deliberately no "already triggered?" pre-check: with a runner-minted + id there is no key to check by, so a redelivery re-triggers and downstream + idempotency absorbs the duplicate (see Idempotency). + - Trigger is async: it returns promptly with the runner-assigned id, not an outcome. + - metadata is empty for now; reserved for future caller annotations. + - 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. + - 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. + - other store error -> return raw; classifier decides. + +7. Publish buildID to the buildsignal topic, partitioned by build id. + - each build's poll loop then runs in its own partition (see [Partitioning](#partitioning)). + - publish failure -> return raw (non-retryable by default); the Build row exists, and the DLQ + path owns recovery (see Error classification). + +8. ack. +``` + +`Build.ID` is **minted by the runner at `Trigger`**, exactly as in SubmitQueue's build controller: the runner returns its native id (a Buildkite build number, a CI-gateway job id), `build` adopts it as the `Build`'s key, and that same id travels on every downstream hop — `build` → `buildsignal` → `record` each carry the build id in the message, so every reader reaches the `Build` by a direct get on identity it was handed. No reader ever *derives* a build id or needs a reverse index; `Build.RequestID` covers the one navigation the pipeline needs in the other direction (`Build` → `Request`). Another approach is deriving the key from the Request (`buildKey(R)`) and/or passed a caller-supplied idempotency key to `Trigger`; see [Alternatives considered](#alternatives-considered-for-the-build-identity) for what each would buy and cost. + +`build` writes only the `Build`, and only at creation; it never mutates `Request.State`. The Request stays `processing` (set by `process`) through `build` and `buildsignal` until `record` moves it terminal. `Build.Status` is the fine-grained build lifecycle; `Request.State` is the coarse pipeline lifecycle. This is also what keeps `process.md` step 3 correct: because `build` leaves the Request at `processing`, a redelivered `process` message still matches its "if processing, re-publish to build" guard. + +## Idempotency + +Every branch is safe under at-least-once redelivery — with SubmitQueue's posture on duplicates adopted wholesale: `build` has no pre-trigger dedup check (there is no caller-derivable key to check by; see [Alternatives considered](#alternatives-considered-for-the-build-identity)), so a redelivery that reaches step 5 starts a second, independent build, and safety comes from downstream idempotency rather than from preventing the duplicate: + +- **Request not found / 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. +- **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](../workflow.md#fail-closed-on-unprocessable-work)). + +## Edge cases + +- **Head equals last-green** (`R.URI == R.BaseURI`). `process.md` calls this degenerate and leaves it to `build`. Resolution: run a normal incremental build with an empty delta — pass `baseURI = R.BaseURI` unchanged and let the runner apply zero changes on top of it. This needs no branch in `build`: the runner must already handle "no delta" (two adjacent commits) as valid input. Rejected: skipping the build and copying last-green's result forward, because that would force `build` to reason about whether last-green's *recorded greenness* (not just its URI) is still valid — a `record` concern `build` has no business making. +- **Phase 2 triggers another build for the same Request.** No collision by construction: every `Trigger` mints a fresh runner id, so the Phase-1 whole-repo build and each Phase-2 project-scoped build get distinct `Build` rows, all carrying the same `RequestID`. Each build's id travels in its own messages, so downstream navigation stays a direct get; how `analyze` obtains and forwards the ids of the project builds it triggers is part of the deferred project-scoped trigger design. + +## Fail-closed interaction + +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](process.md#concurrency-lifecycle)). `build` does not implement the forcing function itself. Per [workflow.md](../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 not-green 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](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. + +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](buildsignal.md#fail-closed-interaction)). `build`'s liveness responsibility ends at a successful publish to `buildsignal`. + +## Cancellation: defined, not yet called + +`Cancel` is in the `BuildRunner` contract for parity with SubmitQueue, but no controller in this design calls it. SubmitQueue cancels from its `speculate`/cancel path when a batch is preempted mid-validation; stovepipe's `process` explicitly does **not** preempt an admitted `Request` ("a newer head does not preempt an in-flight validation" — [process.md](process.md#concurrency-lifecycle)). So there is currently no trigger for cancelling a running build. `Cancel` stays in the contract because a future change — the lease-based self-healing `in_flight_count`, or raising `max_concurrent` past 1, both floated in `process.md` — could need to abandon a build no longer worth finishing. Until then it is unused surface, not dead weight. + +## Error classification + +Per `platform/errs`' non-retryable-by-default rule (see [platform/errs/README.md](../../../../platform/errs/README.md)), the controller returns raw errors and lets the consumer's classifier decide; it opts into retryability explicitly only where it knows something the classifier cannot: + +| Failure | Disposition | Why | +|---|---|---| +| `Request` not found (`storage.ErrNotFound`) | retryable (`errs.NewRetryableError`) | Producing stage's write not visible yet; redelivery converges — same as `process.go`. | +| Factory lookup | non-retryable | A queue with no registered builder is a config error, not transient. | +| Malformed message | non-retryable | The payload is broken; retrying can't fix it. | +| `Trigger` | raw error; classifier decides | Runner timeout/connection is transient (may be classified retryable); a bad URI is permanent. | +| Build store, other than `ErrAlreadyExists` | non-retryable | Storage down; DLQ reconciliation recovers. | +| Publish to buildsignal | non-retryable | Not wrapped retryable, per the errs rule against retryable publishes. The message dead-letters, the fail-closed reconciler forces a conservative not-green, and an operator republish can still resume polling later — a late green wins over the conservative verdict via CAS ([workflow.md](../workflow.md#fail-closed-on-unprocessable-work)). | + +Everything non-retryable rejects to DLQ, where the fail-closed path forces a conservative not-green so a Request never wedges its Queue's slot ([workflow.md](../workflow.md#fail-closed-on-unprocessable-work)). + +## Orchestrator vs. Stovepipe build + +Both domains have a `build` controller that triggers via a build-runner extension, and they share the *pattern*: id-only queue payloads (only the id travels; the entity is reloaded from the store), the swallow-`ErrAlreadyExists`-then-publish-anyway redelivery handling, and the `Name()`/`TopicKey()`/`ConsumerGroup()` consumer shape. They differ in the **`BuildRunner` contract**, because they model different problems. + +### SubmitQueue + +SubmitQueue validates **stacks of changes** before merging. Its `build` controller loads `base []entity.Batch` (ordered dependency batches) and `head entity.Batch` and triggers: + +```go +buildID, err := buildRunner.Trigger(ctx, base, head, metadata) +``` + +The batches are **identity** — thin references carrying ids, not change content — and the runner resolves each batch's changes through injected dependencies. This is "identity in, resolve internally" applied to the batch domain. + +### Stovepipe + +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) +``` + +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. + +### Why separate contracts + +1. **Conceptual mismatch — materialize vs. check out.** SubmitQueue's `Trigger` builds a state that does not exist yet: the runner resolves each batch's changes and applies their patches into composite base/head layers before running CI (see the Buildkite backend). Stovepipe's head already exists on the branch — trunk history is linear, and a landed commit *contains* every commit below it — so the runner checks out `headURI` and, for an incremental build, diffs against `baseURI`; no patch application, no composite commit. Squeezing stovepipe through the patch-list contract breaks in one of two ways for a history interval `a → b → c → d → e` (`a` = last green, `e` = head): modeling it as base `a` + patch `e` misses the intermediate commits `b..d` in the built state (**missing history**), while modeling it as heads `[b, c, d, e]` makes the runner cherry-pick four commits into a composite that is identical to just checking out `e` (**excessive work**). Forcing single-element batch lists is only the mild form of the same mismatch: abstraction with no benefit. +2. **Baseline semantics — and a mode batches can't express.** SubmitQueue's base batches are *stacked* into a dependency DAG validated together; stovepipe's baseline is a single reference point, and the build is either against it (incremental) or from scratch (full). Batches don't model this naturally — an empty base list means "no dependencies", not "ignore ancestry and build the whole repo", so the `full_monorepo` fallback `process` selects on a history rewrite ([process.md](process.md#build-strategy-decision)) has no faithful batch encoding at all. +3. **URI ownership.** Stovepipe's head and baseline are owned by `SourceControl`. Passing URIs directly keeps that boundary clean; passing batch objects would leak batch semantics into a runner that shouldn't know they exist. + +The linearity assumption in point 1 is load-bearing and already guarded upstream: stovepipe assumes a linear trunk by default, and when `SourceControl` reports that last-green is no longer an ancestor of the head (history rewrite), `process` falls back to a full build rather than trusting the interval ([process.md](process.md#build-strategy-decision)). The URI-pair contract is exactly as expressive as that model — a valid `base..head` range, or a full build with an empty baseline — and nothing more. + +So `build`'s `Trigger` gets its own shape under `stovepipe/extension/buildrunner`, still "identity in, resolve internally" — just with URI identity instead of batch identity. `Status` and `Cancel` don't have this mismatch — both domains poll and cancel by the same opaque, runner-minted id with the same async semantics — so they move to a shared `platform/extension/buildrunner.StatusCanceller` sub-interface instead of being duplicated (see the contract sketch below). + +### Stovepipe `BuildRunner` contract (design sketch) + +Not implemented here. `BuildID`, `BuildStatus`, and `BuildMetadata` move to `platform/base` (promoted from each domain's own `entity` package, now that `Status`/`Cancel` are genuinely shared — see [Alternatives considered for sharing the contract](#alternatives-considered-for-sharing-the-contract) for why); `stovepipe/extension/buildrunner` holds only `Trigger`, `Config`, and the `Factory` interface, per [CLAUDE.md](../../../../CLAUDE.md)'s extension rules. + +```go +// platform/extension/buildrunner — shared with SubmitQueue +type StatusCanceller interface { + // Status returns the current status. Takes the id Trigger returned + // (Build.ID). May round-trip to the backend. BuildMetadata is + // caller-supplied, provider-echoed; the runner must not depend on it, but + // a controller may read it for its own purposes (e.g. round-tripping to + // users, or a future short-circuit check) — buildsignal's own poll loop + // doesn't need it to decide when to stop polling, in either domain. + Status(ctx context.Context, buildID base.BuildID) (base.BuildStatus, base.BuildMetadata, error) + + // Cancel requests cancellation; a no-op on terminal builds. Takes the id + // Trigger returned, like Status. Unused today in stovepipe (see + // "Cancellation: defined, not yet called"). + Cancel(ctx context.Context, buildID base.BuildID) error +} + +// package buildrunner (stovepipe/extension/buildrunner) +type BuildRunner interface { + buildrunner.StatusCanceller + + // 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 + // depend on. 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 base.BuildMetadata) (base.BuildID, error) +} + +type Config struct{ QueueName string } // the only identity the system hands a Factory +type Factory interface{ For(cfg Config) (BuildRunner, error) } +``` + +Only `Trigger` differs between domains — batches vs. URIs, per [Why separate contracts](#why-separate-contracts). `Status` and `Cancel` don't have that mismatch: both domains poll and cancel by the same opaque, runner-minted id with the same async semantics, so duplicating them per domain (one copy in each domain's own `entity` package, mirroring the choice already made for `RequestID`) would be "shaped the same" without being shared code. Sharing them for real means `BuildID`/`BuildStatus`/`BuildMetadata` have to be the same Go types on both sides, which is why they move to `platform/base` here rather than staying duplicated — a one-time migration of SubmitQueue's already-shipped controllers, storage, and protobuf mappings onto the shared type, accepted as worth it for real reuse (a dual-implementing backend satisfies both `BuildRunner` interfaces through one embedded method set) instead of two interfaces that only look alike. [Alternatives considered for sharing the contract](#alternatives-considered-for-sharing-the-contract) below records the shapes that were weighed against this one. + +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 `platform/base`, shared verbatim with SubmitQueue rather than duplicated — `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 the shared `ID`/`Status`/`Version`), and, separately, `TargetGraph` — see [below](#target-graph-not-part-of-status-resolved-separately) for where that lives and why it isn't part of `Status`. + +### Alternatives considered for sharing the contract + +Three further shapes for sharing the `BuildRunner` contract across domains were also raised during the design discussion, weighed against the shared-`Status`/`Cancel`-plus-per-domain-`Trigger` split adopted above. Recorded here for context; none of these three were adopted: + +- **One platform-level `BuildRunner` with both trigger verbs.** Move the interface to `platform/extension` and add `TriggerChanges(baseURI, headURI)` beside the batch-based `Trigger`: + + ```go + // 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) + Status(ctx context.Context, buildID entity.BuildID) (entity.BuildStatus, entity.BuildMetadata, error) + Cancel(ctx context.Context, buildID entity.BuildID) error + } + ``` + + Trade-offs: the batch verb would drag SubmitQueue-only entities (`Batch`, its state machine) into `platform/`, which is reserved for genuinely cross-domain types — they would be "shared" in name with exactly one consumer. And a two-verb interface where every caller uses exactly one verb is a sign the contract is modeling two problems; every backend — Buildkite, a mock, a future CI-gateway client — would carry a stubbed or irrelevant half per domain. SubmitQueue's `build` controller would only ever call `Trigger`; stovepipe's would only ever call `TriggerChanges`. +- **Scope smuggled through `BuildMetadata`.** Keep one narrow `Trigger`, pass no scope arguments, and encode base/head (or job-configuring env cards) in metadata: + + ```go + buildID, err := buildRunner.Trigger(ctx, entity.BuildMetadata{ + "head_uri": headURI, + "base_uri": baseURI, + }) + ``` + + Trade-offs: this inverts the metadata contract — `BuildMetadata` is caller annotation the runner echoes but **must not depend on** ([build-runner.md](../../submitqueue/build-runner.md#buildmetadata)). Routing the build's one load-bearing input through it would make the scope untyped, unvalidated, and invisible in the interface — a runner correctly honoring the "must not depend on metadata" rule would ignore `head_uri`/`base_uri` entirely and build the wrong scope. +- **Shared backend under `platform`, thin per-domain contracts.** House the Buildkite / CI-gateway implementation once under `platform/extension` and let each domain define its own contract over it: + + ```go + // platform/extension/buildrunner/buildkite — shared HTTP client, auth, poll loop + package buildkite + + type Client struct{ /* ... */ } + + // submitqueue/extension/buildrunner + func NewBuildkiteRunner(c *buildkite.Client) submitqueuebuildrunner.BuildRunner { /* resolves batches into a patch list, then calls c */ } + + // stovepipe/extension/buildrunner + func NewBuildkiteRunner(c *buildkite.Client) stovepipebuildrunner.BuildRunner { /* checks out headURI, diffs against baseURI, then calls c */ } + ``` + + Trade-offs: the shareable layer is thinner than it looks — the *checkout intent* differs at the CI-pipeline level: SubmitQueue's job applies patch lists into a composite commit, stovepipe's checks out an existing commit and diffs against a baseline — so the pipeline side must know which caller it serves either way. What this option gets right can still be had at the implementation layer without a shared contract: a concrete backend package can implement **both** domain interfaces and share its client/auth/poll plumbing internally (each service wires only the interface it needs — the "one backend, two interfaces" shape), and genuinely domain-free plumbing can live under `platform/`. + +Contract-level reuse is not zero even for the parts that stayed separate: the `Trigger` async contract — returns a handle not an outcome, callers learn progress via `Status` — and the id model carry over verbatim between the two domains' `Trigger` methods, even though `Trigger` itself isn't shared code — see [Carries over vs. new](#carries-over-vs-new). + +### Target graph: not part of `Status`, resolved separately + +`Status` does not return a `TargetGraph` alongside `BuildStatus`. `buildsignal`'s poll loop runs for every build in the pipeline, including every Phase-2 project build, but only `analyze` ever needs a target graph, and only once, right after a Phase-1 build succeeds. Routing it through `Status`/`buildsignal` would mean every poll tick — and every Phase-2 build, which has no further target-to-project mapping to do — computes or shuttles a graph almost nothing reads. Per the extension-contract's "identity in, resolve internally" rule, `analyze` resolves the target graph itself from the identity it already has (`Build.ID`, arriving via `record`'s fan-out), rather than receiving it pre-resolved through a chain of controllers that don't need it. See [Alternatives considered for the target graph](#alternatives-considered-for-the-target-graph) for the shapes weighed for how `analyze` does that, and for the open question of what `TargetGraph` itself contains. + +### Alternatives considered for the target graph + +How the target graph gets from the runner to `analyze` has four shapes worth recording. The last one reflects where this doc leans, not a closed decision — the `analyze` design has the final say: + +- **Inline in `Status`, alongside `BuildStatus`.** `Status(ctx, buildID) (BuildStatus, TargetGraph, error)`, with `buildsignal` persisting whatever comes back. Trade-offs: simplest wiring, but it puts a potentially large, `analyze`-only payload on every poll tick of every build — including every Phase-2 project build, which has no further target-to-project mapping to do — and it forces `Status` back into a stovepipe-only signature, undoing the shared `platform/extension/buildrunner.StatusCanceller` (see [Alternatives considered for sharing the contract](#alternatives-considered-for-sharing-the-contract)). +- **Runner-owned artifact, opaque handle returned from a call.** The runner writes the graph to its own storage (an artifact API, a blob keyed by build id) and returns only a small locator — from `Status`, or from the dedicated call below. Trade-offs: shrinks whatever crosses an RPC or queue to a handle-sized value regardless of graph size, and fits the "identity in, resolve internally" pattern this contract already uses for `Build.ID`. The cost only shows up with more than one concrete `BuildRunner` backend: a single backend can just own wherever it stores the artifact, but a second backend would need the handle format to be uniformly resolvable — a `platform`-level abstraction this doc otherwise avoids building ahead of a second consumer. +- **`buildsignal` fetches it once at the terminal poll and persists it for `analyze`.** `buildsignal` calls a target-graph-fetching method itself, but only on the poll that observes a terminal, successful status, storing the result (or a handle to it) on `Build` for `analyze` to read later. Trade-offs: bounds the fetch to once per build instead of every poll tick, unlike the inline-in-`Status` option — but `buildsignal`'s poll loop still runs for every Phase-2 project build, producing a graph nobody downstream reads, and it makes `buildsignal` a pass-through for data it has no use for itself — the "controller pre-resolves for someone else" shape the extension-contract's "identity in, resolve internally" rule warns against. +- **`analyze` resolves it directly via a dedicated call, keyed by `Build.ID`.** E.g. `TargetGraph(ctx, buildID) (entity.TargetGraph, error)` — either an additional stovepipe-only method on `BuildRunner`, or a separate extension `analyze` depends on; either way resolved through the same `Factory.For(Config{QueueName})` pattern `build`/`buildsignal` already use. Trade-offs: the only option where the party that needs the data is the party that fetches it, so nothing computes or carries a graph on `buildsignal`'s or a Phase-2 build's behalf. The cost is that `analyze` needs its own `Factory.For(Config{QueueName})` resolution — not a new capability, since `build`/`buildsignal` already do this, just one more caller of it. + +### Alternatives considered for the build identity + +Two alternatives to the runner-minted id are worth recording. They are independent knobs — the first changes what keys the `Build`, the second changes what `Trigger` accepts — and they compose (a combined variant would use both). + +#### Alternative A: caller-derived build key + +Key the `Build` by identity derived from the Request — `buildKey(R) = R.ID` for the Phase-1 whole-repo build, a `{R.ID}/{hash(project)}` composite for Phase 2 — and store the runner's id in a separate `Build.RunnerBuildID` field (with a distinct wire type, so the two ids can never be passed for each other). + +| Pros | Cons | +|---|---| +| Redelivery dedup by direct get: checking `BuildStore.Get(buildKey(R))` before triggering means at-least-once delivery never starts a second build | A second id concept (`Build.ID` beside `Build.RunnerBuildID`) carried by every entity, signature, and reader forever | +| `Request` → `Build` navigation with no reverse index, per the KV key-derivation rule in CLAUDE.md | No current reader needs to *derive* a build id — the id travels in every message hop, so each consumer already holds the key it needs | +| Enforces (rather than assumes) the direct-navigation property SubmitQueue's speculate takes on faith | Diverges entity shape and controller flow from SubmitQueue, weakening the "structurally the same controller" claim and dual-implementing-backend symmetry | + +Trade-offs: the dedup guards a rare event at a permanent modeling cost. The duplicate it prevents arises only from a redelivery inside the trigger window — rare, and already harmless (identical scope; `buildsignal`'s terminal-Request short-circuit and `record`'s CAS make the loser a no-op — see [Idempotency](#idempotency)). The prospective key-derivers — a future canceller, or `analyze` reaching back to the Phase-1 target graph — would need to be handed the id by their producing stage instead, if those designs land. + +#### Alternative B: caller-supplied idempotency key on `Trigger` + +Orthogonal to how the `Build` is keyed: give `Trigger` an extra parameter — a stable per-build token the caller already holds (`R.ID` in Phase 1) — that a runner supporting deduplication uses to **re-attach** a retried `Trigger` to the build it already started instead of spawning a second. The runner still returns its own native id; `Build.ID` stays runner-minted. Backends that cannot dedup ignore the token and degrade to today's behavior. + +| Pros | Cons | +|---|---| +| Closes the duplicate-build window at the source (see [Idempotency](#idempotency)) instead of absorbing duplicates downstream | The one concrete backend in hand can't honor it: Buildkite's "create a build" endpoint takes no dedup key or idempotency header in its documented parameters (`author`, `clean_checkout`, `env`, `meta_data`, `pull_request_*`, …) — every call mints a new build number | +| Additive and degradable: a runner that can't dedup ignores the token, and behavior is exactly today's | Speculative surface until such a backend exists — an unused parameter every implementation must carry and document | +| The standard remote-create pattern for at-least-once callers (idempotency keys), so future backends plausibly support it | Diverges `Trigger`'s signature from SubmitQueue's on a parameter neither domain's current backend can act on | + +Trade-offs: a parameter no backend can act on is speculative surface, not a working guarantee — and the failure it would prevent is already accounted for as accepted waste. + +Either could be adopted independently: the idempotency token, if a backend that honors one lands (a purely additive `Trigger` parameter); the derived key, if a stage lands that genuinely must derive a build's key from a Request (moving the runner's id to a distinct `RunnerBuildID` field and wire type, since the two ids would then coexist and must not be confusable). The metric that would justify either is the same: duplicate-build waste showing up in practice. + +### Carries over vs. new + +- **Carries over as literally shared code**: the `BuildStatus` enum and `IsTerminal()` (nothing batch-specific — [build-runner.md](../../submitqueue/build-runner.md#buildstatus)); `BuildMetadata` (caller-supplied, provider-echoed, controller-uninterpreted — [#buildmetadata](../../submitqueue/build-runner.md#buildmetadata)); the async contract — `Trigger` returns a handle not an outcome, `Status` may round-trip, `Cancel` reaches the provider not the engine ([#async-vs-sync-contract](../../submitqueue/build-runner.md#async-vs-sync-contract)); and the id model — no caller-supplied id, the runner mints the build's identity, and that one `base.BuildID` is the store key, queue payload, and `Status`/`Cancel` parameter (see [Alternatives considered](#alternatives-considered-for-the-build-identity)). These now live in `platform/base`/`platform/extension/buildrunner`, not duplicated per domain — see the contract sketch above. +- **New in stovepipe**: URI-based scope in `Trigger` instead of batch lists — the one part of the contract that stays domain-specific, per [Why separate contracts](#why-separate-contracts). Mapping targets to projects is still stovepipe-only, but it doesn't ride `Status`: `analyze` resolves a `TargetGraph` itself through a separate call, once, only when it needs one — see [Target graph: not part of `Status`, resolved separately](#target-graph-not-part-of-status-resolved-separately). + +## Entity and storage additions needed + +**`Build` entity** (`stovepipe/entity/build.go`), following the immutable-except-`Status`/`Version` shape of `entity.Request`; `ID` and `Status` reuse the shared `platform/base` types (`BuildID`, `BuildStatus`) now that `Status`/`Cancel` are shared with SubmitQueue (see the [contract sketch](#stovepipe-buildrunner-contract-design-sketch)), while `RequestID`/`URI`/`BaseURI` stay 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` | + +**States** (`Build.Status`): + +| Status | Meaning | Terminal? | +|---|---|---| +| `` (unknown) | Zero value; never a valid stored status | no | +| `accepted` | Queued by the runner via `Trigger`; not yet started | no | +| `running` | Actively executing | no | +| `succeeded` | Build finished, all checks passed | **yes** | +| `failed` | Build finished, at least one check failed | **yes** | +| `cancelled` | Build stopped before finishing (see [Cancellation](#cancellation-defined-not-yet-called)) | **yes** | + +`IsTerminal()` on `base.BuildStatus` covers exactly the three terminal rows. Once `buildsignal` persists one of them, that status is **write-once** — a later poll reporting a different terminal value never overwrites it (see [buildsignal.md](buildsignal.md#algorithm), step 6). + +Plus the `BuildID{ID string}` wire type from `platform/base` (same "id only travels" convention as `RequestID`, but shared rather than duplicated — see the [contract sketch](#stovepipe-buildrunner-contract-design-sketch)), wrapping the one runner-assigned id everywhere it appears — `Trigger`'s return, the queue payload, `Status`/`Cancel`'s parameter. `buildsignal` and `record` reach a build by the id carried in their messages, so no reverse index from `Request` to its builds is ever needed. + +**`BuildStore`** (new, added to the `Storage` aggregator via `GetBuildStore()`), matching stovepipe's existing `RequestStore` conventions — **generic `Update` with caller-owned version arithmetic, not SubmitQueue's field-specific `UpdateStatus`**: + +- `Create(ctx, build entity.Build) error` — `ErrAlreadyExists` if the id is taken. +- `Get(ctx, id string) (entity.Build, error)` — `ErrNotFound` if absent. +- `Update(ctx, build entity.Build, oldVersion, newVersion int32) error` — pure conditional write; `ErrVersionMismatch` on a stale guard. The controller computes `newVersion = oldVersion + 1`, calls the store, and assigns `build.Version = newVersion` only on success (see [CLAUDE.md](../../../../CLAUDE.md) and the [storage README](../../../../submitqueue/extension/storage/README.md)). + +Single-key reads/writes only — no list-by-request, no query-by-attribute — per the key/value-shaped extension rule in [CLAUDE.md](../../../../CLAUDE.md). + +**`Request` additions** (extending the existing entity, which already has `ID/Queue/URI/State/Version`): + +| Field | Role | Set by | +|---|---|---| +| `BuildStrategy` | `incremental_since_green` or `full_monorepo`; immutable once set | `process` | +| `BaseURI` | Last-green URI for incremental; empty for full | `process` | + +`URI` already exists; `process` sets `BuildStrategy`/`BaseURI` at admit (process.md step 7c) and `build` reads them (step 4). Both are immutable for the Request's life. + +## Queue contract additions + +Two topic keys in `stovepipe/core/messagequeue/topics.go` — `TopicKeyBuild` (`process`/`analyze` → `build`) and `TopicKeyBuildSignal` (`build` → `buildsignal`) — and one proto message per key, since the contract test binds **exactly one message to each topic key** (see [messagequeue-contract.md](../../messagequeue-contract.md)). `ProcessRequest` is bound to `process` and cannot be reused; the new messages mirror its shape (one id field, its own `topic_keys` option): + +- `BuildRequest{ id }` (request id) → `topic_keys "build"`, produced by `process`/`analyze`, consumed by `build`. Phase 2's per-project trigger must also identify its project; because each topic key binds exactly one message, that lands as an **additive optional field on this same message** (protojson discards unknown fields, so the evolution is backward-compatible), not a second message type — the field's shape is deferred to the `analyze` design with the rest of the project-scoped trigger. +- `BuildSignal{ id }` (build id) → `topic_keys "buildsignal"`, produced by `build` and re-produced by `buildsignal`, consumed by `buildsignal` (see [buildsignal.md](buildsignal.md)). + +### Partitioning + +`process`/`analyze` → `build` is partitioned by **request id**: per-request build work is independent (the per-Queue concurrency gate already ran in `process`), and a single Request's (rare) duplicate deliveries stay ordered — though with no pre-trigger dedup check that ordering is a tidiness property, not a correctness dependency; duplicates are absorbed downstream (see [Idempotency](#idempotency)). `build` → `buildsignal` is partitioned by **build id** (the runner-assigned `Build.ID`), so each build's poll loop is an independent partition. The id is unique per build, so one `Request`'s several Phase-2 project builds land in distinct partitions — the point: a slow poll on one must not block the others. The flip side of request-id partitioning on the `build` topic is that all of one Request's Phase-2 *triggers* serialize through a single partition; that is fine because `build` is trigger-only and cheap, and the concurrency Phase 2 needs comes from the per-build poll partitions, not from parallel triggering. This is a deliberate divergence from SubmitQueue, which partitions its build poll loop by *batch id* (its unit of work); stovepipe goes finer. diff --git a/doc/rfc/stovepipe/steps/buildsignal.md b/doc/rfc/stovepipe/steps/buildsignal.md new file mode 100644 index 00000000..8ebed829 --- /dev/null +++ b/doc/rfc/stovepipe/steps/buildsignal.md @@ -0,0 +1,138 @@ +# Buildsignal stage + +`buildsignal` polls the build-runner until a build reaches a terminal state, records the status, and — once the build is terminal — publishes the build id to `record`. See [workflow.md](../workflow.md) for where it sits in the pipeline and [build.md](build.md) for how the build it polls was triggered. + +It handles only the poll loop: it does not decide build strategy, write greenness, or map targets to projects — those are `process`'s, `record`'s, and `analyze`'s jobs. + +`buildsignal` is structurally the same controller as `submitqueue/orchestrator/controller/buildsignal/buildsignal.go`, and the "Flow", "Status delivery", and "Polling primitive" sections of [doc/rfc/submitqueue/build-runner.md](../../submitqueue/build-runner.md) are the reference rationale it reuses directly. The entity and storage shapes it depends on are defined in [build.md](build.md#entity-and-storage-additions-needed); this doc introduces none of its own. + +## Input and re-entrancy + +`buildsignal` consumes a build id, published by `build` — once per phase, since Phase 1 (whole-repo) and Phase 2 (per-project) each run their own `build` → `buildsignal` cycle against builds tied to the same `Request` (see [workflow.md](../workflow.md#workflow)). + +Its logic does not branch on phase: it loads the `Build`, polls it toward terminal, persists the result, and publishes the build id onward to `record`. What differs between phases is what `record` does with that publish (whole-repo vs. per-project greenness) — not anything `buildsignal` decides. + +`buildsignal` is the sole writer of `Build.Status`/`Build.Version` after `build` creates the row (see [build.md](build.md#input-partitioning-and-the-single-writer-property)); it only reads `Request` via `RequestStore.Get` (for `R.Queue`, to resolve the build-runner) and never writes it. + +## Algorithm + +For a delivery carrying build id `B`: + +``` +1. Load Build B from the build store. + - ErrNotFound -> retryable (build's Create not visible yet; redelivery converges). + - other store error -> return raw; classifier decides. + +2. Load Request R = store.Get(Build.RequestID) — needed for R.Queue to resolve the build-runner. + - ErrNotFound -> retryable, like step 1: the Build's existence proves the Request write is older, + so a miss here is almost certainly a lagging read; redelivery converges. A genuinely orphaned + Build (integrity fault) still dead-letters at MaxAttempts — the same terminal outcome, without + rejecting straight to DLQ on a stale read. + +3. If R.State is terminal (superseded / recorded-green / recorded-not-green): ack and return. + - the Request is done (record already ran, or the head was superseded); stop polling. + - mirrors submitqueue's halted-batch short-circuit in buildsignal.go. + +4. Resolve the build-runner: buildRunner = Factory.For(Config{QueueName: R.Queue}). + - lookup failure -> non-retryable (config error, same as build stage). + +5. Poll: status, metadata, err := buildRunner.Status(ctx, base.BuildID{ID: B}) + - B is Build.ID, the runner-assigned id minted at Trigger — one id end to end (see build.md). + - Status may fail transiently (runner unavailable, unknown build id) -> return raw; classifier decides. + - metadata is BuildMetadata (caller-supplied, provider-echoed); the poll loop's own control flow + (steps 6-8) doesn't need it to decide when to stop polling, but it isn't dropped on the floor — + see "Status" below for what it's for and how a future check could use it. + +6. Reconcile the polled status against the stored row: + - status == Build.Status -> no-op poll: skip the write, continue with the stored status. + - Build.Status already terminal, status differs -> terminal is WRITE-ONCE: do not overwrite; + continue with the STORED status as authoritative (see Edge cases). + - otherwise persist via BuildStore.Update(ctx, Build{...Status: status}, oldVersion, newVersion): + - newVersion = Build.Version + 1; assign Build.Version = newVersion only on success. + - ErrVersionMismatch -> retryable (a concurrent writer moved the row; reload and re-check). + - with the write-once rule, accepted -> running -> {succeeded|failed|cancelled} is monotonic + by mechanism, not by assumption about the backend. + +7. If the stored status is terminal: publish B (the build key, Build.ID) to the record topic, + partitioned by request id; ack, return. No re-publish to buildsignal. + - record loads the Build directly by this key (Build->Request gives it R for the per-Queue writes), + so no reverse lookup from Request to its builds is ever needed. + - a redelivery republishes the same terminal signal; record is idempotent. + - publish failure -> return raw (non-retryable); status is persisted, operational republish recovers. + +8. Else PublishAfter(B -> buildsignal, delayMs), partitioned by build id: + - delayMs = pollDelay(status): shorter while running, longer while accepted. + - a fresh message (retry_count resets to 0), not a nack — polling is not failure. + - ack. +``` + +**Why `record` hears only terminal signals**: `record` has no non-terminal work — by its own contract a non-terminal signal would be a pure no-op — and step 7 already branches on terminality to decide whether to keep polling, so gating the publish costs nothing and spares `record` a no-op delivery on every poll tick of every running build. Crash-safety is unaffected: a crash between the terminal `Update` and the publish redelivers the message; step 5 re-polls (the runner reports the same terminal status), step 6 no-ops, step 7 publishes. This is a deliberate divergence from SubmitQueue, whose buildsignal republishes to `speculate` on every tick — sound there because speculate is a state machine that may act on any signal; stovepipe has no such consumer. + +**Why step 6 guards on status and makes terminal write-once**: an unchanged status skips the CAS write entirely, so a long build being polled every couple of seconds doesn't churn `Build.Version` on every tick — the version only advances on a real state transition. The write-once rule exists because CAS alone cannot provide it: optimistic locking defends against *concurrent* writers, but a later delivery that polls a flaky backend and sees a different terminal status would CAS cleanly against the current version and overwrite (see Edge cases). A given `Build` has a single poll partition (see [Partitioning](build.md#partitioning)), so the only writer racing the CAS is a redelivery of the same message (e.g. after a lapsed visibility lease); `ErrVersionMismatch` there is handled as retryable and converges. + +## Status: shared with SubmitQueue + +`Status` is not a point of divergence from SubmitQueue: both domains' `BuildRunner`s share the same `Status(ctx, buildID) (BuildStatus, BuildMetadata, error)` signature via `platform/extension/buildrunner.StatusCanceller` (see [build.md](build.md#stovepipe-buildrunner-contract-design-sketch)). `BuildMetadata` is caller-supplied and provider-echoed — the runner must not depend on it, but nothing stops a consumer from reading it. `buildsignal`'s own poll loop (steps 6-8) doesn't need to interpret it to decide when to stop polling, same as SubmitQueue's, but that's a statement about what the poll loop happens to need, not a rule that the value is unused: `build-runner.md` describes its purpose as round-tripping to users ([#buildmetadata](../../submitqueue/build-runner.md#buildmetadata)), and a future check in either domain's buildsignal is free to read it — e.g. to short-circuit some behavior — without changing the contract. + +Returning `TargetGraph` from `Status` in place of `BuildMetadata`, with `buildsignal` persisting it for `analyze` to read later, was considered and set aside — see [build.md](build.md#target-graph-not-part-of-status-resolved-separately) for where the target graph lives instead and why `buildsignal` isn't the right place to carry it. + +## Polling primitive: `PublishAfter`, not `Nack` + +On non-terminal status, step 8 reschedules with `PublishAfter`, never `Nack`: + +- **`Nack`** requeues and increments `retry_count`; at `MaxAttempts` the message dead-letters. That is the primitive for "something failed; retry." +- **`PublishAfter`** emits a fresh message with `retry_count` reset to 0, deferred by a delay. That is the primitive for "still working; check back later." + +Polling is a scheduled heartbeat, neither failure nor retry, so a long-running build never burns `retry_count` toward the DLQ. A genuine `Status` failure (runner down, bad id) is a *different* path: it returns from step 5 to the classifier, which decides retryability, and a retryable verdict nacks normally. See [build-runner.md](../../submitqueue/build-runner.md#polling-primitive-publishafter-not-nack) for the full rationale. + +## Poll delays + +Two-tier cadence, matching SubmitQueue: + +| Status | Delay | Rationale | +|---|---|---| +| `accepted` | `PollDelayAcceptedMs` (default 5000ms) | Queued by the runner, not started — poll infrequently. | +| `running` | `PollDelayRunningMs` (default 2000ms) | Actively executing — poll more often. | + +Package-level `var`s (not `const`s) so tests can shorten them; the server always uses the defaults. Future: move these behind a `queueconfig`-style extension so operators can tune cadence per queue without a code change (the same TODO SubmitQueue's buildsignal carries). + +## Error classification + +Per `platform/errs`' non-retryable-by-default rule (see [platform/errs/README.md](../../../../platform/errs/README.md)); the controller returns raw errors and lets the classifier decide, opting into retryability only where it knows more than the error value carries: + +| Failure | Disposition | Why | +|---|---|---| +| `Build` not found | retryable (`errs.NewRetryableError`) | `build`'s `Create` not visible yet; redelivery converges. | +| `Request` not found | retryable (`errs.NewRetryableError`) | The Build's existence proves the Request write is older, so a miss is a stale read; a genuine orphan still dead-letters at `MaxAttempts`. | +| Factory lookup | non-retryable | Config error, same as `build`. | +| `Status` call | raw error; classifier decides | Runner timeout/connection is transient; "runner not deployed for this queue" is not. | +| `Update` CAS conflict (`ErrVersionMismatch`) | retryable | A concurrent (redelivered) writer moved the row; reload and re-check converges. | +| `Update` other store error | non-retryable | Storage down; DLQ reconciliation recovers. | +| Publish to `record` | non-retryable | Status persisted; operational republish recovers. | +| `PublishAfter` re-poll | retryable | The poll heartbeat; it runs only after status/persist/record all succeeded, so a transient enqueue blip is worth replaying to `MaxAttempts` before dead-lettering. | + +## Idempotency + +Every branch is safe under at-least-once redelivery: + +- **Build not found** — retryable; converges as the row becomes visible. +- **Status already persisted** — a redelivery re-runs the whole algorithm from step 1, including a redundant `Status` poll (harmless — the runner reports the same thing); step 6 no-ops on the unchanged status, and the delivery proceeds to re-schedule the poll (non-terminal) or republish to `record` (terminal, idempotent). No corruption. +- **Terminal already published** — a redelivery reloads, re-polls, no-ops at step 6, republishes the same terminal signal to `record` (idempotent), and acks. Harmless. +- **`PublishAfter` failed, then retried** — the nacked delivery re-runs from step 1; there is no way to resume mid-algorithm, so it re-polls the runner too, but the row already carries the non-terminal status and step 6 no-ops. Only the final enqueue does new work. + +The window to guard is between persisting status (step 6) and ack (steps 7–8); because status writes are CAS-guarded, monotonic, and write-once at terminal, a redelivery always observes a consistent row. + +## Edge cases + +- **Runner has no record of this build id** (runner restarted without persisting in-flight state; a foreign id leaked in). `Status` returns an error, not a status value — non-retryable by default, unless the classifier has a domain sentinel for "unknown build" it chooses to treat as retryable (a restart may self-resolve). Left to the classifier, not hardcoded, per [build-runner.md](../../submitqueue/build-runner.md#error-classification). +- **A later `Status` returns a *different* terminal status than what's stored** (a flaky backend flipping `succeeded`→`failed` between polls). CAS is *not* the defense here: the earlier delivery already committed its write and acked, so a later delivery would CAS cleanly against the current version — optimistic locking guards concurrent writers, not sequential overwrites. The defense is step 6's write-once rule: a stored terminal status is never overwritten, the delivery proceeds with the stored value, and `record` only ever hears one verdict per build. First terminal wins by design; a backend that flip-flops terminal states is broken in a way consumer-side ordering cannot repair, so a deterministic verdict is the most the pipeline can offer. + +## Fail-closed interaction + +A build that never reaches terminal `Status` — runner outage, a build the runner lost — must not wedge its `Request` forever, since callers gate deployments on greenness reaching a recorded terminal state. `buildsignal` does not implement the forcing function: per [workflow.md](../workflow.md#fail-closed-on-unprocessable-work) and the `in_flight_count` slot lifecycle in [process.md](process.md#concurrency-lifecycle), a `Request` stuck at `buildsignal` past `MaxAttempts` dead-letters, and the DLQ reconciler forces a conservative not-green and releases the Queue's slot. This is the same posture SubmitQueue's build/buildsignal pair relies on: terminal status is what releases the slot and lets validation progress. + +One boundary of that posture is worth stating: the `MaxAttempts` path fires only when polls *fail*. A runner that keeps answering a healthy non-terminal status forever — a hung build on a backend with no timeout of its own — never errors, so the `PublishAfter` chain (which resets `retry_count` by design) re-polls indefinitely and nothing dead-letters; SubmitQueue's poll loop shares this property. Bounding it requires a poll deadline — a `max_validation_ms` past which `buildsignal` treats the build as failed and lets the normal terminal path run — which pairs naturally with the lease idea [process.md](process.md#per-queue-concurrency-gate) floats for `in_flight_count`. Deferred with it; until then a too-old non-terminal `Build` is an operational alert, not a self-healing path. + +## Entity, storage, and queue additions + +No additions beyond [build.md](build.md#entity-and-storage-additions-needed): `buildsignal` calls `BuildStore.Get`/`Update` and `RequestStore.Get` against the `Build`/`Request` shapes defined there — `Build.ID` being the runner-assigned id it hands straight back to `Status` — and consumes/re-produces the `BuildSignal` message on `TopicKeyBuildSignal` introduced there. The message it publishes to `record`, and the `record` topic key itself, are owned by the `record` stage and land with `record.md`; `buildsignal` only needs that the **build id** (the build key, so `record` loads the `Build` directly) reaches the record topic once the build is terminal, partitioned by request id. diff --git a/doc/rfc/stovepipe/workflow.md b/doc/rfc/stovepipe/workflow.md index 20233510..9f74c676 100644 --- a/doc/rfc/stovepipe/workflow.md +++ b/doc/rfc/stovepipe/workflow.md @@ -91,7 +91,7 @@ The pipeline runs in two phases against the same Request. **Phase 1** establishe │ │ Await/record build status + │ │ │ target graph │ │ └───────────────┬──────────────┘ - │ │ RequestID + │ │ BuildID │ ▼ │ ┌──────────────────────────────┐ Hooks │ │ record │┄┄┄┄┄► "URI green / @@ -107,14 +107,14 @@ The pipeline runs in two phases against the same Request. **Phase 1** establishe │ │ → projects (impl-specific); │ │ │ decide project-scoped builds │ │ └───────────────┬──────────────┘ - │ │ BuildID(s) + │ │ RequestID (+ project) │ ▼ │ ┌──────────────────────────────┐ │ │ build → buildsignal │ │ │ CI job runs; artifacts stored │ │ │ in blob store; status read │ │ └───────────────┬──────────────┘ - │ │ RequestID + │ │ BuildID │ ▼ │ ┌──────────────────────────────┐ Hooks └───────────────────►│ record │┄┄┄┄┄► "project P @@ -128,7 +128,7 @@ The pipeline runs in two phases against the same Request. **Phase 1** establishe 1. **ingest** — invoked by the external poller with a **Queue name**. It asks `SourceControl` for that Queue's current head URI, mints a Request namespaced by the Queue, persists it with no recorded greenness yet, and dedups on `(Queue, head URI)` so a re-reported head is processed once. It publishes the RequestID onward. 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 publishes the RequestID to `record`. +4. **buildsignal** — records the build's status and target graph when the build completes, then publishes the BuildID to `record` (the `Build` row carries its RequestID, so `record` reaches the Request with a direct get). 5. **record** — writes the whole-repo greenness for the head URI (`0` green / `1` broken to start). On green it advances the Queue's **last-green URI** so the next `process` can build incrementally from here. It also decrements the Queue's `in_flight_count`, opening the process concurrency gate for the next head. It fires the **Hooks** extension with the green/not-green event, then fans out into Phase 2. ### Phase 2 — project greenness @@ -147,7 +147,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; signal completion | -| **record** | RequestID | analyze (P1→P2), Hooks | Write greenness; advance last-green URI on whole-repo green; decrement `in_flight_count`; fire Hooks | +| **record** | BuildID | analyze (P1→P2), Hooks | Write greenness; advance last-green URI on whole-repo green; decrement `in_flight_count`; fire Hooks | | **analyze** | RequestID | build | Map broken/at-risk targets → projects; decide project-scoped builds | ## Step RFCs @@ -155,6 +155,8 @@ The pipeline runs in two phases against the same Request. **Phase 1** establishe Per-stage design detail lives under `steps/` so this doc stays a pipeline overview: - [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: `PublishAfter` re-poll cadence, target-graph return, per-build partitioning, and the fail-closed handoff to record ## Dedup, idempotency, and history rewrites