Skip to content

refactor(stovepipe): replace recorded greenness states with build outcomes - #466

Open
behinddwalls wants to merge 1 commit into
preetam/stovepipe-poll-idfrom
preetam/stovepipe-request-outcome
Open

refactor(stovepipe): replace recorded greenness states with build outcomes#466
behinddwalls wants to merge 1 commit into
preetam/stovepipe-poll-idfrom
preetam/stovepipe-request-outcome

Conversation

@behinddwalls

@behinddwalls behinddwalls commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

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.

Stack

  1. test(stovepipe): add build-slow marker to the fake build runner #464
  2. fix(stovepipe): mint a distinct message id for each buildsignal re-poll #465
  3. @ refactor(stovepipe): replace recorded greenness states with build outcomes #466
  4. feat(stovepipe): record the build outcome on the request and free its slot #467
  5. feat(stovepipe)!: key the record stage on the request id #468
  6. fix(stovepipe): stop wrapping the buildsignal re-poll publish as retryable #469

…comes

## 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.
@behinddwalls
behinddwalls force-pushed the preetam/stovepipe-request-outcome branch from c13df3a to 0370a56 Compare July 30, 2026 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants