Skip to content

feat(stovepipe)!: key the record stage on the request id - #468

Open
behinddwalls wants to merge 1 commit into
preetam/stovepipe-buildsignal-outcomefrom
preetam/stovepipe-record-request-id
Open

feat(stovepipe)!: key the record stage on the request id#468
behinddwalls wants to merge 1 commit into
preetam/stovepipe-buildsignal-outcomefrom
preetam/stovepipe-record-request-id

Conversation

@behinddwalls

@behinddwalls behinddwalls commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

Summary

Why?

buildsignal published the build id to record, but record's unit of work is a Request: it writes whole-repo greenness for the Request's URI, advances the Queue's last-green bookmark, and fires Hooks. Keying its input on a build artifact forces every future reader to navigate Build→Request, and it bakes a one-build-per-Request assumption into the wire contract.

The assumption is also the wrong one to encode. The concurrency knob gates how many Requests are in flight per Queue — same base, different head, one build each — so the fan-out axis is Queue→N Requests, not Request→N Builds. And if a Request ever did fan out, a build-keyed input would give record N messages with no way to tell "one build failed" from "one failed and three haven't reported yet". A request-keyed input lets record answer its own question.

What?

Record.id now carries the request id, and publishRecord uses it for the payload, the message id, and the partition key. The field name stays id, matching ProcessRequest.id and BuildRequest.id. Partitioning is unchanged — it was already request id — so the only wire change is what the payload means.

No reverse index is needed to make this work: the previous commit projects the build's terminal status onto Request.State, so record reads the Request and never reaches a Build. That also leaves the "no reverse index from Request to its builds" property in build.md intact, for a better reason than before, and the derived-build-key alternative explicitly untriggered — its condition is a stage that must derive a build's key from a Request, which record no longer does.

Marked ! because the record topic's payload semantics change. Nothing consumes it yet — the record stage lands later — so there is no migration.

Test Plan

bazel test //stovepipe/... — new unit test pins that the record payload, message id, and partition key are all the request id.

bazel test //test/e2e/stovepipe/... — the slow-build case now additionally asserts the request reaches succeeded and the queue's in_flight_count returns to 0.

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

## Summary

### Why?

`buildsignal` published the *build* id to `record`, but `record`'s unit of work is a Request: it writes whole-repo greenness for the Request's URI, advances the Queue's last-green bookmark, and fires Hooks. Keying its input on a build artifact forces every future reader to navigate Build→Request, and it bakes a one-build-per-Request assumption into the wire contract.

The assumption is also the wrong one to encode. The concurrency knob gates how many *Requests* are in flight per Queue — same base, different head, one build each — so the fan-out axis is Queue→N Requests, not Request→N Builds. And if a Request ever did fan out, a build-keyed input would give `record` N messages with no way to tell "one build failed" from "one failed and three haven't reported yet". A request-keyed input lets `record` answer its own question.

### What?

`Record.id` now carries the request id, and `publishRecord` uses it for the payload, the message id, and the partition key. The field name stays `id`, matching `ProcessRequest.id` and `BuildRequest.id`. Partitioning is unchanged — it was already request id — so the only wire change is what the payload means.

No reverse index is needed to make this work: the previous commit projects the build's terminal status onto `Request.State`, so `record` reads the Request and never reaches a `Build`. That also leaves the "no reverse index from Request to its builds" property in `build.md` intact, for a better reason than before, and the derived-build-key alternative explicitly untriggered — its condition is a stage that must derive a build's key from a Request, which `record` no longer does.

Marked `!` because the `record` topic's payload semantics change. Nothing consumes it yet — the `record` stage lands later — so there is no migration.

## Test Plan

✅ `bazel test //stovepipe/...` — new unit test pins that the record payload, message id, and partition key are all the request id.

✅ `bazel test //test/e2e/stovepipe/...` — the slow-build case now additionally asserts the request reaches `succeeded` and the queue's `in_flight_count` returns to 0.
@behinddwalls
behinddwalls force-pushed the preetam/stovepipe-record-request-id branch from c524a8a to 1879181 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.

1 participant