refactor(stovepipe): replace recorded greenness states with build outcomes - #466
Open
behinddwalls wants to merge 1 commit into
Open
refactor(stovepipe): replace recorded greenness states with build outcomes#466behinddwalls wants to merge 1 commit into
behinddwalls wants to merge 1 commit into
Conversation
This was referenced Jul 29, 2026
behinddwalls
marked this pull request as ready for review
July 29, 2026 23:07
…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
force-pushed
the
preetam/stovepipe-request-outcome
branch
from
July 30, 2026 07:51
c13df3a to
0370a56
Compare
mnoah1
approved these changes
Jul 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Why?
Request.Statecarriedrecorded_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 terminalBuildStatus. All three are terminal, alongsidesuperseded.cancelledstays distinct fromfailedprecisely so the greenness policy can decide what a cancellation means rather than having that folded in here.Adds
HasBuildOutcome()next toIsTerminal(): a superseded request is terminal but never ran a build, so the two questions are not the same.Updates the readers.
processacks 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 forcesfailedinstead ofrecorded_not_greenand stops referring to greenness at all; its== processingslot-release guard is unchanged and still correct.buildandbuildsignalkeep short-circuiting onIsTerminal().No behaviour change beyond the DLQ reconciler's terminal state: nothing writes
succeededorcancelledyet. The next commit makesbuildsignalthe writer.Test Plan
✅
bazel test //stovepipe/...— terminal and outcome truth tables for the new states;processacks each outcome without republishing to build; the reconciler forcesfailedand still releases the slot only fromprocessing.✅
bazel test //test/e2e/stovepipe/...— the pipeline still runs end to end.Stack