Skip to content

Commit 08dbfb2

Browse files
committed
feat(stovepipe): Wire the project analysis stage
## Summary **What**: - Route completed build analysis through project analysis before repository recording. - Record repository outcomes from persisted project results and preserve downstream handoffs during recovery. **Why**: - Ensure project status and issue attribution use all final issue observations. A terminal build can still have detection work pending, and repository recording needs the project results to distinguish a partial failure from a repository-wide failure. ## Test Plan - [x] Add unit tests. ## Revert Plan - Revert this PR after pausing intake and draining or reconciling pending project-analysis work, because the revert removes its consumers. ## Issue Links - [CODEM-632](https://linear.app/uber/issue/CODEM-632/projects-issues)
1 parent 6dbfad8 commit 08dbfb2

40 files changed

Lines changed: 910 additions & 810 deletions

‎doc/rfc/stovepipe/steps/buildsignal.md‎

Lines changed: 11 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -10,11 +10,11 @@ It handles only the poll loop: it does not decide build strategy, write greennes
1010

1111
`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)).
1212

13-
Its logic does not branch on phase: it loads the `Build`, polls it toward terminal, persists the result, and publishes the request 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.
13+
Its logic does not branch on phase: it loads the `Build`, polls it toward terminal, persists the result, and queues terminal build analysis. Cancelled outcomes also go directly to `record`. What differs between phases is what `record` does with that publish (whole-repo vs. per-project greenness) — not anything `buildsignal` decides.
1414

1515
`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 reads `Request` via `RequestStore.Get` (for `R.Queue`, to resolve the build-runner) and writes it exactly once, at the terminal transition, to record the build's outcome — the one `Request.State` write outside `process` and the DLQ reconciler.
1616

17-
Its early-exit guard is deliberately narrower than `State.IsTerminal()`: it proceeds when the request is `processing` **or** already carries a build outcome. The second case matters because a redelivery after the outcome was stamped but before the `record` publish landed must re-publish rather than drop the signal; everything it re-runs is a no-op (the status is unchanged, the outcome is already recorded, the slot is not released twice) and the `record` publish is idempotent.
17+
Its early-exit guard is deliberately narrower than `State.IsTerminal()`: it proceeds when the request is `processing` **or** already carries a build outcome. The second case matters because a redelivery after the outcome was stamped but before the terminal-analysis publish landed must re-publish rather than drop the signal; everything it re-runs is a no-op (the status is unchanged, the outcome is already recorded, the slot is not released twice) and the downstream publish is idempotent.
1818

1919
The states the guard actually rejects are terminal-without-an-outcome — today `superseded` alone, which `process` sets only from `accepted`, so it cannot reach the poll loop at all. The guard is kept rather than dropped because the slot release in step 7a is conditioned on *not* already having an outcome, so a superseded request arriving here would decrement a slot that superseding never claimed.
2020

@@ -34,7 +34,7 @@ For a delivery carrying build id `B`:
3434
3. If R.State is neither processing nor already an outcome: ack and return.
3535
- a request reaches the poll loop only after process admitted it, so it is processing —
3636
or already carries this build's outcome on a redelivery, which falls through so the
37-
signal is re-published to record instead of dropped.
37+
terminal analysis is re-published instead of dropped.
3838
- the only other terminal-without-an-outcome state is superseded, and process supersedes
3939
solely from accepted, so it cannot occur here. Guarded anyway: step 7a would otherwise
4040
release a build slot the request never claimed, since superseding consumes none.
@@ -68,14 +68,13 @@ For a delivery carrying build id `B`:
6868
succeeded -> succeeded, failed -> failed, cancelled -> cancelled. First writer wins.
6969
c. If the winning durable R.State is succeeded, advance the last-green bookmark
7070
in a CAS retry loop, even on a terminal redelivery. Preserve a newer bookmark.
71-
Then publish R.ID to the record topic, partitioned by request id; ack, return.
72-
No re-publish to buildsignal.
73-
- record loads the Request directly by this key, derives greenness from its outcome, and
74-
supplies its terminal build id to project-result resolution; it never reaches a Build.
75-
- the message id is the request id, so a redelivery republishing the same terminal signal
76-
dedups into the original message; record is idempotent regardless.
77-
- publish failure -> return raw (non-retryable); the outcome is persisted, operational
78-
republish recovers.
71+
Then publish terminal BuildAnalysis, partitioned by build id. Cancelled requests
72+
also publish R.ID directly to record, partitioned by request id. Ack and return.
73+
- successful or failed outcomes reach record through completed build analysis
74+
and project analysis, not directly from this poll.
75+
- stable handoff IDs make republishing safe.
76+
- publication failures return raw; DLQ recovery repairs the handoff without
77+
polling the runner again.
7978
8079
8. Else hold the delivery for delayMs (postpone: the same message redelivers after the delay):
8180
- delayMs = pollDelay(status): shorter while running, longer while accepted.
@@ -169,4 +168,4 @@ One boundary of that posture is worth stating: the `MaxAttempts` path fires only
169168

170169
## Entity, storage, and queue additions
171170

172-
No additions beyond [build.md](build.md#entity-and-storage-additions-needed): `buildsignal` calls `BuildStore.Get`/`Update` and `RequestStore.Get`/`Update` against the `Build`/`Request` shapes defined there — `Build.ID` being the runner-assigned id it hands straight back to `Status` — plus `QueueStore.Get`/`Update` to release the build slot, and consumes the `BuildSignal` message on `TopicKeyBuildSignal` introduced there. A non-terminal poll calls `delivery.Hold` on that delivery rather than publishing another one. `Request.State` gains the three build outcomes (`succeeded`, `failed`, `cancelled`), all terminal. 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 **request id** reaches the record topic once the build is terminal, partitioned by request id.
171+
No additions beyond [build.md](build.md#entity-and-storage-additions-needed): `buildsignal` calls `BuildStore.Get`/`Update` and `RequestStore.Get`/`Update` against the `Build`/`Request` shapes defined there — `Build.ID` being the runner-assigned id it hands straight back to `Status` — plus `QueueStore.Get`/`Update` to release the build slot, and consumes the `BuildSignal` message on `TopicKeyBuildSignal` introduced there. A non-terminal poll calls `delivery.Hold` on that delivery rather than publishing another one. `Request.State` gains the three build outcomes (`succeeded`, `failed`, `cancelled`), all terminal. Terminal analysis owns the next normal handoff; only cancellation publishes directly to the record topic. Both messages use their existing build-id or request-id partitions.

‎doc/rfc/stovepipe/steps/record.md‎

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2,14 +2,14 @@
22

33
`record` turns a terminal build outcome into a durable validation fact.
44

5-
- Phase 1 records whole-repository greenness and optional project facts. On green it promotes the commit onto the Queue's promotion ref if the Request still holds the last-green bookmark, then announces the outcome as a hook event. This is implemented: see [stovepipe/controller/record/record.go](../../../../stovepipe/controller/record/record.go).
5+
- Phase 1 records whole-repository greenness using project results already persisted by `projectanalysis`. On green it promotes the commit onto the Queue's promotion ref if the Request still holds the last-green bookmark, then announces the outcome as a hook event. This is implemented: see [stovepipe/controller/record/record.go](../../../../stovepipe/controller/record/record.go).
66
- Phase 2 records greenness per project instead of per repository. Sketched here, to be expanded before implementation.
77

88
Notifying downstream systems rides the cross-domain hook framework rather than a Stovepipe-specific extension; see [Hooks](#hooks).
99

1010
See [workflow.md](../workflow.md) for the whole pipeline, [build.md](build.md) for how builds are created, and [buildsignal.md](buildsignal.md) for the terminal-only handoff into this stage.
1111

12-
`record` owns the validation facts and the promotion ref. `buildsignal` advances the last-green build baseline before handing off to this stage. `record` does not decide build scope, poll a build, release the Queue's build slot, interpret a target graph, or map targets to projects. Those belong to `process`, `buildsignal`, and `analyze`.
12+
`record` owns the repository fact and promotion ref; `projectanalysis` owns project facts and issue attribution. `buildsignal` advances the last-green build baseline before handing off to this stage. `record` does not decide build scope, poll a build, release the Queue's build slot, interpret a target graph, or map targets to projects. Those belong to `process`, `buildsignal`, and `projectanalysis`.
1313

1414
## Phase 1 algorithm
1515

@@ -39,8 +39,8 @@ For a delivery carrying a `Record` payload:
3939
- superseded -> ack, no fact. Unreachable in practice.
4040
- accepted / processing / anything else -> return a non-retryable invariant error.
4141
42-
6. Resolve optional project results using R and its TerminalBuildID, and validate
43-
the entire result set before writing facts. A resolver error returns raw.
42+
6. Load the request summary and its persisted ProjectCount. Project analysis
43+
has already recorded results or abandonment before publishing here.
4444
4545
7. Map R.State and the project results to a whole-repository degree and create the
4646
ValidationFact keyed by (R.URI, empty project) in the queue-bound fact store.
@@ -50,9 +50,8 @@ For a delivery carrying a `Record` payload:
5050
- ErrAlreadyExists -> load and reconcile the existing immutable fact.
5151
- other store error -> return raw.
5252
53-
8. Record each project fact with its resolver-provided degree and persist history
54-
for the repository fact and completed project facts. A failure returns raw;
55-
replay completes missing writes without replacing immutable facts.
53+
8. Persist history for the repository fact. A failure returns raw; replay adopts
54+
the immutable fact and repairs its history.
5655
5756
9. If the persisted repository fact is not green, skip to step 11. Report how long
5857
the break went undetected only if step 7 created the fact (see "Observability").
@@ -108,7 +107,7 @@ Absence is still distinct from degree `0`. Callers gating deployments must treat
108107

109108
A fact is written only when the Request reaches `succeeded` or `failed`. A `cancelled` build is acked with no fact.
110109

111-
The fail-closed path is subtler than "no fact". A DLQ reconciler forces the Request to `failed` and does not itself publish here, so reconciliation on its own records nothing. That is not the same as nothing being recorded: a `buildsignal` delivery still in flight can reach this stage afterwards, and it will write `DegreeBroken` from the forced state. See [DLQ and fail-closed behavior](#dlq-and-fail-closed-behavior).
110+
Project-analysis recovery hands off after retaining abandonment. A failed request without a completed set of project facts therefore records `DegreeBroken`; partial writes alone do not supply a project count. Build-status recovery also repairs the downstream handoff without polling the runner again. See [DLQ and fail-closed behavior](#dlq-and-fail-closed-behavior).
112111

113112
### Phase 1 degree mapping
114113

@@ -123,15 +122,15 @@ Whole-repository degree depends on the outcome `buildsignal` stamped on the Requ
123122
| `cancelled` | no fact |
124123

125124

126-
`DegreePartial` is hard coded to `0.5` for now; it does not measure the fraction of projects failing. Each project fact preserves the degree returned by the resolver, unless its immutable identity already exists. After persisting every project fact and its degree index, `record` retains total and broken project counts in `project_facts_recorded` for the request summary. Broken means a persisted degree above zero; retries count adopted facts rather than contradictory resolver results. Empty results record zero counts.
125+
`DegreePartial` is hard coded to `0.5` for now; it does not measure the fraction of projects failing. Each project fact preserves the degree returned by the resolver, unless its immutable identity already exists. After persisting every project fact and its degree index, `projectanalysis` retains total and broken project counts in `project_facts_recorded` for the request summary. Broken means a persisted degree above zero; retries count adopted facts rather than contradictory resolver results. Empty results record zero counts.
127126

128127
## Last-green advancement (Queue bookmark)
129128

130129
`buildsignal` advances the bookmark after releasing the build slot and persisting the successful Request outcome, before publishing to `record`. The bookmark only moves forward. `entity.Queue` carries `LastGreenURI` alongside `LastGreenRequestID`, the request id that owns the current URI, plus `InFlightCount` and `LatestRequestID`. The guard compares ids with `entity.CompareRequestID(R.Queue, …)`, the same ingest-order comparison `ingest` and `process` use for coalescing. It errors on an id that does not match the queue's format, which is non-retryable: re-parsing the same ids cannot start succeeding.
131130

132131
The write goes through `QueueStore.Update(ctx, queue, oldVersion, newVersion)`, so version arithmetic stays in the controller: it computes `newVersion = oldVersion + 1` and the store does a pure conditional write. `ErrVersionMismatch` is absorbed by the loop rather than returned, since a concurrent writer moved the row and reloading converges.
133132

134-
The bookmark is a successful-build baseline and can precede validation facts. Project resolution and Git promotion remain in `record`. Releasing capacity and advancing the bookmark are separate Queue updates, so a new build may be admitted with the previous baseline before the advance lands. A terminal redelivery retries the bookmark update without releasing the slot again.
133+
The bookmark is a successful-build baseline and can precede validation facts. Project resolution runs in `projectanalysis`; Git promotion remains in `record`. Releasing capacity and advancing the bookmark are separate Queue updates, so a new build may be admitted with the previous baseline before the advance lands. A terminal redelivery retries the bookmark update without releasing the slot again.
135134

136135
A failed or cancelled build never moves the bookmark, and so never promotes either.
137136

0 commit comments

Comments
 (0)