From 87781b5213cd58951c60481fd8980e402e5ce473 Mon Sep 17 00:00:00 2001 From: Preetam Dwivedi Date: Tue, 28 Jul 2026 14:59:43 -0700 Subject: [PATCH] refactor(stovepipe): replace recorded greenness states with build outcomes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary ### Why? `Request.State` carried `recorded_green` / `recorded_not_green` — a greenness verdict stored on the Request. But greenness is specified as its own persisted fact, keyed per URI and per project, and its *absence* is meaningful ("not yet recorded" is distinct from green and must be treated as not-green for gating). Encoding a verdict on the Request duplicates a fact that belongs in a per-URI store that does not exist yet, and it conflates two different lifecycles: where a request is in the pipeline, versus what the answer turned out to be. It also puts the wrong thing on the Request. What the pipeline actually knows when a build finishes is the build's terminal status. Whether that means green is a policy decision — one that has to account for cancellation, and later for per-project degrees — and it belongs wherever greenness is recorded, not on the Request. ### What? Replaces the two recorded states with the three build outcomes — `succeeded`, `failed`, `cancelled` — as a 1:1 projection of the terminal `BuildStatus`. All three are terminal, alongside `superseded`. `cancelled` stays distinct from `failed` precisely so the greenness policy can decide what a cancellation means rather than having that folded in here. Adds `HasBuildOutcome()` next to `IsTerminal()`: a superseded request is terminal but never ran a build, so the two questions are not the same. Updates the readers. `process` acks on any outcome state instead of falling through to its unexpected-state warning — the build already finished, so a stale redelivery has nothing to do. The DLQ reconciler forces `failed` instead of `recorded_not_green` and stops referring to greenness at all; its `== processing` slot-release guard is unchanged and still correct. `build` and `buildsignal` keep short-circuiting on `IsTerminal()`. No behaviour change beyond the DLQ reconciler's terminal state: nothing writes `succeeded` or `cancelled` yet. The next commit makes `buildsignal` the writer. ## Test Plan ✅ `bazel test //stovepipe/...` — terminal and outcome truth tables for the new states; `process` acks each outcome without republishing to build; the reconciler forces `failed` and still releases the slot only from `processing`. ✅ `bazel test //test/e2e/stovepipe/...` — the pipeline still runs end to end. --- doc/rfc/stovepipe/steps/build.md | 12 +++--- doc/rfc/stovepipe/steps/buildsignal.md | 8 ++-- doc/rfc/stovepipe/steps/process.md | 6 +-- doc/rfc/stovepipe/workflow.md | 2 +- stovepipe/controller/build/build.go | 4 +- stovepipe/controller/build/build_test.go | 16 ++++++-- .../buildsignal/buildsignal_test.go | 15 +++++-- stovepipe/controller/dlq/dlq.go | 16 ++++---- stovepipe/controller/dlq/dlq_test.go | 12 +++--- stovepipe/controller/process/process.go | 4 +- stovepipe/controller/process/process_test.go | 24 +++++++++++ stovepipe/entity/request.go | 41 +++++++++++++++---- stovepipe/entity/request_test.go | 27 +++++++++++- 13 files changed, 138 insertions(+), 49 deletions(-) diff --git a/doc/rfc/stovepipe/steps/build.md b/doc/rfc/stovepipe/steps/build.md index 13040efd..c74de713 100644 --- a/doc/rfc/stovepipe/steps/build.md +++ b/doc/rfc/stovepipe/steps/build.md @@ -10,7 +10,7 @@ It handles only the trigger: it does not poll for completion, record greenness, `build` consumes a request id, published by `process` in Phase 1 (see [workflow.md](doc/rfc/stovepipe/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](doc/rfc/stovepipe/steps/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 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 the DLQ reconciler 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](doc/rfc/stovepipe/steps/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 — see [Project-scoped `Trigger`: reserved, not yet designed](#project-scoped-trigger-reserved-not-yet-designed) for the resulting gap in the `BuildRunner` contract itself. @@ -25,9 +25,9 @@ For a delivery carrying request id `R`: - ErrNotFound -> return raw; non-retryable (storage is read-after-write consistent; see [storage README](stovepipe/extension/storage/README.md)). - 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. +2. If R.State is terminal (superseded / succeeded / failed / cancelled): ack and return. + - a redelivery after the build outcome was already recorded, 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). @@ -88,7 +88,7 @@ Every branch is safe under at-least-once redelivery — with SubmitQueue's postu ## 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](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 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](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. +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. 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`. @@ -98,7 +98,7 @@ One boundary is worth stating explicitly: this path fires only when `build` (or ## Error classification -Per `platform/errs`'s non-retryable-by-default rule (see [platform/errs/README.md](platform/errs/README.md)), a plain returned error is already non-retryable and rejects straight to DLQ, where the fail-closed path forces a conservative not-green so a Request never wedges its Queue's slot ([workflow.md](doc/rfc/stovepipe/workflow.md#fail-closed-on-unprocessable-work)). So this section documents only the departures from that default, not every failure the algorithm can hit: +Per `platform/errs`'s non-retryable-by-default rule (see [platform/errs/README.md](platform/errs/README.md)), a plain returned error is already non-retryable and rejects straight to DLQ, where the fail-closed path forces a conservative terminal `failed` so a Request never wedges its Queue's slot ([workflow.md](doc/rfc/stovepipe/workflow.md#fail-closed-on-unprocessable-work)). So this section documents only the departures from that default, not every failure the algorithm can hit: | Failure | Disposition | Why | |---|---|---| diff --git a/doc/rfc/stovepipe/steps/buildsignal.md b/doc/rfc/stovepipe/steps/buildsignal.md index e574c35e..a05d96ab 100644 --- a/doc/rfc/stovepipe/steps/buildsignal.md +++ b/doc/rfc/stovepipe/steps/buildsignal.md @@ -27,8 +27,8 @@ For a delivery carrying build id `B`: - ErrNotFound -> return raw; non-retryable, same as step 1 — the Build's existence proves the Request write is already committed, so a miss here is a storage defect, not a lagging 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. +3. If R.State is terminal (superseded / succeeded / failed / cancelled): ack and return. + - the Request is done (its build outcome was recorded, 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}). @@ -100,7 +100,7 @@ Package-level `var`s (not `const`s) so tests can shorten them; the server always ## Error classification -Per `platform/errs`'s non-retryable-by-default rule (see [platform/errs/README.md](platform/errs/README.md)), a plain returned error is already non-retryable and rejects straight to DLQ, where the fail-closed path forces a conservative not-green so a Request never wedges its Queue's slot ([workflow.md](doc/rfc/stovepipe/workflow.md#fail-closed-on-unprocessable-work)). So this section documents only the departures from that default, not every failure the algorithm can hit: +Per `platform/errs`'s non-retryable-by-default rule (see [platform/errs/README.md](platform/errs/README.md)), a plain returned error is already non-retryable and rejects straight to DLQ, where the fail-closed path forces a conservative terminal `failed` so a Request never wedges its Queue's slot ([workflow.md](doc/rfc/stovepipe/workflow.md#fail-closed-on-unprocessable-work)). So this section documents only the departures from that default, not every failure the algorithm can hit: | Failure | Disposition | Why | |---|---|---| @@ -130,7 +130,7 @@ The window to guard is between persisting status (step 6) and ack (steps 7–8); ## 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](doc/rfc/stovepipe/workflow.md#fail-closed-on-unprocessable-work) and the `in_flight_count` slot lifecycle in [process.md](doc/rfc/stovepipe/steps/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. +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](doc/rfc/stovepipe/workflow.md#fail-closed-on-unprocessable-work) and the `in_flight_count` slot lifecycle in [process.md](doc/rfc/stovepipe/steps/process.md#concurrency-lifecycle), a `Request` stuck at `buildsignal` past `MaxAttempts` dead-letters, and the DLQ reconciler forces a conservative terminal `failed` 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](doc/rfc/stovepipe/steps/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. diff --git a/doc/rfc/stovepipe/steps/process.md b/doc/rfc/stovepipe/steps/process.md index 76508f53..63e3e4f5 100644 --- a/doc/rfc/stovepipe/steps/process.md +++ b/doc/rfc/stovepipe/steps/process.md @@ -18,7 +18,7 @@ For a delivery carrying request id `R`: ``` 1. Load Request R from the request store. - not found -> non-retryable (storage is read-after-write consistent; see [storage README](../../../../stovepipe/extension/storage/README.md)). -2. If R.State is terminal (superseded / recorded-green / recorded-not-green): +2. If R.State is terminal (superseded / succeeded / failed / cancelled): - ack and return (idempotent no-op). 3. If R.State is processing (strategy already recorded): - re-publish R to build (the prior publish may have failed), ack, return. @@ -64,7 +64,7 @@ Validation is expensive and shares a baseline, so heads arriving while an earlie A slot is held for the **entire** Phase 1 cycle (`process → build → buildsignal → record`), not just while `process` runs. It is released when the Request reaches **any** terminal state and `in_flight_count` is decremented — `record` writing green *or* not-green, or the DLQ reconciler forcing a terminal not-green (see [integrity](#in_flight_count-integrity)). A build *failure* frees the slot just like a success; only a Request that never terminates keeps its slot. -**Liveness — a stuck Request wedges the whole Queue.** The slot is shared per-Queue, so a Request admitted but never driven terminal holds the only slot and stalls the whole Queue. The fail-closed path in [workflow.md](../workflow.md#fail-closed-on-unprocessable-work) prevents this: every admitted Request terminates — `build`/`buildsignal` errors retry to `MaxAttempts`, then dead-letter, and the DLQ reconciler forces a conservative terminal not-green, freeing the slot. Residual risk is operational: a **poison DLQ message** the reconciler can't process loops forever (it runs always-retryable), wedging that Queue until an operator removes it. The gate makes the blast radius the whole Queue, not one Request — monitor and alert on the DLQ. +**Liveness — a stuck Request wedges the whole Queue.** The slot is shared per-Queue, so a Request admitted but never driven terminal holds the only slot and stalls the whole Queue. The fail-closed path in [workflow.md](../workflow.md#fail-closed-on-unprocessable-work) prevents this: every admitted Request terminates — `build`/`buildsignal` errors retry to `MaxAttempts`, then dead-letter, and the DLQ reconciler forces a conservative terminal `failed`, freeing the slot. Residual risk is operational: a **poison DLQ message** the reconciler can't process loops forever (it runs always-retryable), wedging that Queue until an operator removes it. The gate makes the blast radius the whole Queue, not one Request — monitor and alert on the DLQ. **Future alternative — time-bounded leases.** To self-heal instead of relying on operator cleanup, `in_flight_count` could become a set of `{owner, expires_at}` leases: the gate counts only unexpired leases, terminal transitions drop the owner's lease, and a leaked lease is reclaimed on expiry — bounding any stall to a `max_validation_ms`. A *list* of leases would also generalize to `max_concurrent > 1`. Deferred; `in_flight_count` plus the fail-closed path is enough for MVP. @@ -196,7 +196,7 @@ Per-queue knobs such as `max_concurrent` live outside this row — see [Per-Queu | `accepted` | Ingested, awaiting `process` | no | | `processing` | Admitted; strategy recorded; build in flight | no | | `superseded` | Skipped by coalescing for a newer head | **yes** | -| *(owned by record)* recorded-green / recorded-not-green | Phase 1 outcome | **yes** | +| succeeded / failed / cancelled | Phase 1 build outcome | **yes** | | *(later)* building, recording, … | Finer states as downstream stages need them | — | Transitions use the repo's optimistic-locking pattern: compute `newVersion = oldVersion + 1`, call `RequestStore.Update(ctx, req, oldVersion, newVersion)`, assign `req.Version = newVersion` only on success (see [storage README](../../../../submitqueue/extension/storage/README.md) and [CLAUDE.md](../../../../CLAUDE.md)). diff --git a/doc/rfc/stovepipe/workflow.md b/doc/rfc/stovepipe/workflow.md index 9f74c676..c752321c 100644 --- a/doc/rfc/stovepipe/workflow.md +++ b/doc/rfc/stovepipe/workflow.md @@ -164,7 +164,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 not-green outcome** 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. 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. ## Open questions diff --git a/stovepipe/controller/build/build.go b/stovepipe/controller/build/build.go index b488d93d..f5048176 100644 --- a/stovepipe/controller/build/build.go +++ b/stovepipe/controller/build/build.go @@ -95,8 +95,8 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er return err } - // A redelivery after record already finished, or after process superseded - // the head, must not start a fresh build. + // A redelivery after the build outcome was already recorded, or after process + // superseded the head, must not start a fresh build. if request.State.IsTerminal() { return nil } diff --git a/stovepipe/controller/build/build_test.go b/stovepipe/controller/build/build_test.go index da176089..e2aead4f 100644 --- a/stovepipe/controller/build/build_test.go +++ b/stovepipe/controller/build/build_test.go @@ -160,18 +160,26 @@ func TestProcess(t *testing.T) { }, }, { - name: "recorded green is a no-op", + name: "succeeded request is a no-op", setup: func(m buildMocks) { req := processingRequest(entity.BuildStrategyFull, "") - req.State = entity.RequestStateRecordedGreen + req.State = entity.RequestStateSucceeded m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(req, nil) }, }, { - name: "recorded not green is a no-op", + name: "failed request is a no-op", setup: func(m buildMocks) { req := processingRequest(entity.BuildStrategyFull, "") - req.State = entity.RequestStateRecordedNotGreen + req.State = entity.RequestStateFailed + m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(req, nil) + }, + }, + { + name: "cancelled request is a no-op", + setup: func(m buildMocks) { + req := processingRequest(entity.BuildStrategyFull, "") + req.State = entity.RequestStateCancelled m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(req, nil) }, }, diff --git a/stovepipe/controller/buildsignal/buildsignal_test.go b/stovepipe/controller/buildsignal/buildsignal_test.go index 3e06be15..beb71469 100644 --- a/stovepipe/controller/buildsignal/buildsignal_test.go +++ b/stovepipe/controller/buildsignal/buildsignal_test.go @@ -165,17 +165,24 @@ func TestProcess(t *testing.T) { }, }, { - name: "recorded green request is a no-op", + name: "succeeded request is a no-op", setup: func(m buildsignalMocks) { m.buildStore.EXPECT().Get(gomock.Any(), testBuildID).Return(build(entity.BuildStatusRunning, 2), nil) - m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(requestWithState(entity.RequestStateRecordedGreen), nil) + m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(requestWithState(entity.RequestStateSucceeded), nil) }, }, { - name: "recorded not green request is a no-op", + name: "failed request is a no-op", setup: func(m buildsignalMocks) { m.buildStore.EXPECT().Get(gomock.Any(), testBuildID).Return(build(entity.BuildStatusRunning, 2), nil) - m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(requestWithState(entity.RequestStateRecordedNotGreen), nil) + m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(requestWithState(entity.RequestStateFailed), nil) + }, + }, + { + name: "cancelled request is a no-op", + setup: func(m buildsignalMocks) { + m.buildStore.EXPECT().Get(gomock.Any(), testBuildID).Return(build(entity.BuildStatusRunning, 2), nil) + m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(requestWithState(entity.RequestStateCancelled), nil) }, }, { diff --git a/stovepipe/controller/dlq/dlq.go b/stovepipe/controller/dlq/dlq.go index 4912ea3e..ad1bf54f 100644 --- a/stovepipe/controller/dlq/dlq.go +++ b/stovepipe/controller/dlq/dlq.go @@ -25,8 +25,9 @@ // Reconciliation strategy. Each DLQ topic carries the same payload as its originating // topic (the queue framework preserves the bytes verbatim under a new `{topic}_dlq` // name). The DLQ controller decodes that payload to recover the affected request, then -// transitions it to RequestStateRecordedNotGreen — the conservative not-green verdict -// for gating (see entity.RequestState) — with an idempotent optimistic-locking write so +// transitions it to RequestStateFailed — the conservative unsuccessful outcome, which +// whatever records greenness treats as not-green for gating (see entity.RequestState) — +// with an idempotent optimistic-locking write so // concurrent activity (a late successful pipeline transition) wins cleanly. If the request had // already been admitted (processing) and was holding a concurrency slot, the // reconciler also releases it by CAS-decrementing the queue's in_flight_count, per @@ -57,9 +58,10 @@ func TopicKey(main consumer.TopicKey) consumer.TopicKey { return consumer.TopicKey(string(main) + topicSuffix) } -// failRequest transitions request to RequestStateRecordedNotGreen if it is not already +// failRequest transitions request to RequestStateFailed if it is not already // in a terminal state. If the request had reached RequestStateProcessing — meaning process's -// admit step already CAS-incremented the queue's in_flight_count for it — the queue's +// admit step already CAS-incremented the queue's in_flight_count for it and no terminal +// outcome has released it yet — the queue's // slot is released first. Queue and Request are separate entities with no cross-entity // transaction, so the two writes cannot be atomic and the ordering picks which crash // failure mode we accept: a crash between the writes leaves the request non-terminal, @@ -97,12 +99,12 @@ func failRequest(ctx context.Context, store storage.Storage, logger *zap.Sugared } updated := request - updated.State = entity.RequestStateRecordedNotGreen + updated.State = entity.RequestStateFailed newVersion := request.Version + 1 if err := store.GetRequestStore().Update(ctx, updated, request.Version, newVersion); err != nil { - return fmt.Errorf("failed to update request %s state to recorded_not_green: %w", requestID, err) + return fmt.Errorf("failed to update request %s state to failed: %w", requestID, err) } - logger.Infow("dlq reconcile: request forced terminal not-green", + logger.Infow("dlq reconcile: request forced terminal failed", "request_id", requestID, "previous_state", string(request.State), ) diff --git a/stovepipe/controller/dlq/dlq_test.go b/stovepipe/controller/dlq/dlq_test.go index 7e6ed037..06b659de 100644 --- a/stovepipe/controller/dlq/dlq_test.go +++ b/stovepipe/controller/dlq/dlq_test.go @@ -99,7 +99,7 @@ func TestProcess(t *testing.T) { setup: func(m dlqMocks) { m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(requestWithState(entity.RequestStateAccepted), nil) updated := requestWithState(entity.RequestStateAccepted) - updated.State = entity.RequestStateRecordedNotGreen + updated.State = entity.RequestStateFailed m.reqStore.EXPECT().Update(gomock.Any(), updated, int32(2), int32(3)).Return(nil) }, }, @@ -114,7 +114,7 @@ func TestProcess(t *testing.T) { Name: testQueue, InFlightCount: 0, Version: 5, }, int32(5), int32(6)).Return(nil) updated := requestWithState(entity.RequestStateProcessing) - updated.State = entity.RequestStateRecordedNotGreen + updated.State = entity.RequestStateFailed m.reqStore.EXPECT().Update(gomock.Any(), updated, int32(2), int32(3)).Return(nil) }, }, @@ -127,7 +127,7 @@ func TestProcess(t *testing.T) { { name: "already failed is a no-op", setup: func(m dlqMocks) { - m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(requestWithState(entity.RequestStateRecordedNotGreen), nil) + m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(requestWithState(entity.RequestStateFailed), nil) }, }, { @@ -141,7 +141,7 @@ func TestProcess(t *testing.T) { setup: func(m dlqMocks) { m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(requestWithState(entity.RequestStateAccepted), nil) updated := requestWithState(entity.RequestStateAccepted) - updated.State = entity.RequestStateRecordedNotGreen + updated.State = entity.RequestStateFailed m.reqStore.EXPECT().Update(gomock.Any(), updated, int32(2), int32(3)).Return(storage.ErrVersionMismatch) }, wantErr: true, @@ -163,7 +163,7 @@ func TestProcess(t *testing.T) { Name: testQueue, InFlightCount: 0, Version: 6, }, int32(6), int32(7)).Return(nil) updated := requestWithState(entity.RequestStateProcessing) - updated.State = entity.RequestStateRecordedNotGreen + updated.State = entity.RequestStateFailed m.reqStore.EXPECT().Update(gomock.Any(), updated, int32(2), int32(3)).Return(nil) }, }, @@ -175,7 +175,7 @@ func TestProcess(t *testing.T) { Name: testQueue, InFlightCount: 0, Version: 5, }, nil) updated := requestWithState(entity.RequestStateProcessing) - updated.State = entity.RequestStateRecordedNotGreen + updated.State = entity.RequestStateFailed m.reqStore.EXPECT().Update(gomock.Any(), updated, int32(2), int32(3)).Return(nil) }, }, diff --git a/stovepipe/controller/process/process.go b/stovepipe/controller/process/process.go index a1931735..1b46e651 100644 --- a/stovepipe/controller/process/process.go +++ b/stovepipe/controller/process/process.go @@ -106,7 +106,9 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er return fmt.Errorf("failed to publish request %s to build: %w", request.ID, err) } return nil - case entity.RequestStateSuperseded: + case entity.RequestStateSuperseded, entity.RequestStateSucceeded, entity.RequestStateFailed, entity.RequestStateCancelled: + // Terminal: a newer head preempted this request, or its build already finished. + // A stale redelivery has nothing left to do. return nil case entity.RequestStateAccepted: return c.processAccepted(ctx, request) diff --git a/stovepipe/controller/process/process_test.go b/stovepipe/controller/process/process_test.go index 35a0bbf8..1ebc8504 100644 --- a/stovepipe/controller/process/process_test.go +++ b/stovepipe/controller/process/process_test.go @@ -426,6 +426,30 @@ func TestProcess(t *testing.T) { }, nil) }, }, + { + name: "succeeded is no-op", + setup: func(m processMocks) { + m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(entity.Request{ + ID: testID, Queue: testQueue, State: entity.RequestStateSucceeded, Version: 2, + }, nil) + }, + }, + { + name: "failed is no-op", + setup: func(m processMocks) { + m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(entity.Request{ + ID: testID, Queue: testQueue, State: entity.RequestStateFailed, Version: 2, + }, nil) + }, + }, + { + name: "cancelled is no-op", + setup: func(m processMocks) { + m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(entity.Request{ + ID: testID, Queue: testQueue, State: entity.RequestStateCancelled, Version: 2, + }, nil) + }, + }, { name: "processing republishes to build", setup: func(m processMocks) { diff --git a/stovepipe/entity/request.go b/stovepipe/entity/request.go index 817b5250..38b5e328 100644 --- a/stovepipe/entity/request.go +++ b/stovepipe/entity/request.go @@ -36,19 +36,42 @@ const ( RequestStateProcessing RequestState = "processing" // RequestStateSuperseded means process skipped the request because a newer head exists. RequestStateSuperseded RequestState = "superseded" - // RequestStateRecordedGreen means record wrote whole-repo greenness as green for this URI. - RequestStateRecordedGreen RequestState = "recorded_green" - // RequestStateRecordedNotGreen means record wrote whole-repo greenness as not-green for this - // URI — either a genuine build failure, or a conservative verdict forced by the DLQ - // reconciler when the request could never complete (see workflow.md's fail-closed posture). - RequestStateRecordedNotGreen RequestState = "recorded_not_green" + // RequestStateSucceeded means the build validating this request's commit reached a + // successful terminal status. + RequestStateSucceeded RequestState = "succeeded" + // RequestStateFailed means the build validating this request's commit reached an + // unsuccessful terminal status, or the request could never complete and was forced to a + // conservative unsuccessful outcome (see workflow.md's fail-closed posture). + RequestStateFailed RequestState = "failed" + // RequestStateCancelled means the build validating this request's commit was cancelled + // before reaching a verdict. Distinct from failed: what a cancellation implies for + // greenness is decided where greenness is recorded, not here. + RequestStateCancelled RequestState = "cancelled" ) // IsTerminal returns true if the state is one the pipeline never advances past: superseded (a -// newer head preempted this request) or either recorded outcome (record wrote greenness for -// this URI, whether via the normal path or the fail-closed reconciler). +// newer head preempted this request before it was validated) or one of the three build +// outcomes. Greenness derived from an outcome is a separate fact recorded against the URI, so +// it is not part of this lifecycle. func (s RequestState) IsTerminal() bool { - return s == RequestStateSuperseded || s == RequestStateRecordedGreen || s == RequestStateRecordedNotGreen + switch s { + case RequestStateSuperseded, RequestStateSucceeded, RequestStateFailed, RequestStateCancelled: + return true + default: + return false + } +} + +// HasBuildOutcome returns true if the state records the terminal status of this request's +// build: succeeded, failed, or cancelled. Superseded requests never ran a build, so they are +// terminal without an outcome. +func (s RequestState) HasBuildOutcome() bool { + switch s { + case RequestStateSucceeded, RequestStateFailed, RequestStateCancelled: + return true + default: + return false + } } // BuildStrategy defines how build validates the request's commit. diff --git a/stovepipe/entity/request_test.go b/stovepipe/entity/request_test.go index e4120d6f..3861b02e 100644 --- a/stovepipe/entity/request_test.go +++ b/stovepipe/entity/request_test.go @@ -86,8 +86,9 @@ func TestRequestState_IsTerminal(t *testing.T) { expected bool }{ {name: "superseded is terminal", state: RequestStateSuperseded, expected: true}, - {name: "recorded green is terminal", state: RequestStateRecordedGreen, expected: true}, - {name: "recorded not green is terminal", state: RequestStateRecordedNotGreen, expected: true}, + {name: "succeeded is terminal", state: RequestStateSucceeded, expected: true}, + {name: "failed is terminal", state: RequestStateFailed, expected: true}, + {name: "cancelled is terminal", state: RequestStateCancelled, expected: true}, {name: "unknown is not terminal", state: RequestStateUnknown, expected: false}, {name: "accepted is not terminal", state: RequestStateAccepted, expected: false}, {name: "processing is not terminal", state: RequestStateProcessing, expected: false}, @@ -100,6 +101,28 @@ func TestRequestState_IsTerminal(t *testing.T) { } } +func TestRequestState_HasBuildOutcome(t *testing.T) { + tests := []struct { + name string + state RequestState + expected bool + }{ + {name: "succeeded has an outcome", state: RequestStateSucceeded, expected: true}, + {name: "failed has an outcome", state: RequestStateFailed, expected: true}, + {name: "cancelled has an outcome", state: RequestStateCancelled, expected: true}, + {name: "superseded is terminal without an outcome", state: RequestStateSuperseded, expected: false}, + {name: "unknown has no outcome", state: RequestStateUnknown, expected: false}, + {name: "accepted has no outcome", state: RequestStateAccepted, expected: false}, + {name: "processing has no outcome", state: RequestStateProcessing, expected: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.expected, tt.state.HasBuildOutcome()) + }) + } +} + func TestRequestID_SerializationRoundTrip(t *testing.T) { original := RequestID{ID: "request/monorepo/main/100"}