Skip to content

Commit e0ecd41

Browse files
committed
feat(stovepipe): migrate process and buildsignal waits to the hold primitive
## Summary ### Why? The process stage's build-slot wait and buildsignal's status poll loop were built on ack-and-republish: each wait tick acked the delivery and PublishAfter'd a fresh copy of the same message to the controller's own topic. That forced both controllers to be publishers to their own topics, required per-stage message-id minting (a wall-clock suffix in process, a deterministic generation counter in buildsignal) to dodge the queue's (topic, partition_key, id) dedup, wrote a new log row per tick, and hung each loop's liveness on a publish succeeding. The hold primitive (doc/rfc/consumer-hold.md) is the framework-owned replacement. ### What? process: the budget-full branch now records a hold for GateWaitDelayMs and returns success; the delivery is threaded through processAccepted/admitLatestHead, rescheduleProcess and its wall-clock message-id minting are deleted, and the positive-delay config guard moves into holdForBuildSlot. buildsignal: a non-terminal status holds the delivery for the per-status poll delay; nextPollMessageID, the /poll/ generation scheme, and publishBuildSignal are deleted, and publish loses its now-unused delay parameter. The held message is a partition barrier that redelivers in order without counting toward the retry limit, so the DLQ budget stays reserved for real failures, and a failed postpone write lapses into a normal visibility-timeout redelivery instead of killing the loop. Docs updated to the decided mechanism: process.md's "Waiting for a slot" section replaces the previously-deferred Option 1/Option 2 comparison with hold (the primitive that deferral named), buildsignal.md's polling-primitive and algorithm sections describe hold, GateWaitDelayMs is re-documented as a redelivery delay, and the e2e slow-build test's obsolete dedup rationale is rewritten. ## Test Plan ✅ `bazel test //stovepipe/...` — reworked unit tests assert Hold(delay) is recorded on the gate-wait and poll paths and that no Hold happens on terminal/error paths. ✅ `bazel test //test/e2e/stovepipe/...` — the slow-build scenario reaches a terminal build through multiple hold-driven poll ticks and releases the build slot. ✅ `make fmt`.
1 parent a0851e6 commit e0ecd41

9 files changed

Lines changed: 119 additions & 338 deletions

File tree

doc/rfc/stovepipe/steps/buildsignal.md

Lines changed: 13 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -74,15 +74,12 @@ For a delivery carrying build id `B`:
7474
- publish failure -> return raw (non-retryable); the outcome is persisted, operational
7575
republish recovers.
7676
77-
8. Else PublishAfter(B -> buildsignal, delayMs), partitioned by build id:
77+
8. Else hold the delivery for delayMs (postpone: the same message redelivers after the delay):
7878
- delayMs = pollDelay(status): shorter while running, longer while accepted.
79-
- a fresh message (retry_count resets to 0), not a nack — polling is not failure.
80-
- the message id must be unique per tick. The queue dedups on (topic, partition_key, id)
81-
and the delivery being processed is still un-acked, so its row is present: reusing B as
82-
the message id makes every re-poll collide with the message that scheduled it and be
83-
silently discarded, ending the poll loop after one tick.
84-
- publish failure -> return raw (non-retryable), same posture as step 7.
85-
- ack.
79+
- a hold is a postpone, not a nack — the redelivery does not count toward the retry
80+
limit, so polling never burns retry_count toward the DLQ.
81+
- the held message is a barrier for its partition (the build id), so each build keeps
82+
exactly one poll chain; no message ids are minted and no new rows are written per tick.
8683
```
8784

8885
**Why the slot is released before the outcome write, and why a failed release aborts it**: `Queue` and `Request` are separate entities with no cross-entity transaction, so the ordering picks which crash failure mode we accept. Both rules serve one invariant — *the request must not go terminal while still holding a slot* — because a terminal request is skipped by redelivery and by the DLQ reconciler alike, so nothing would ever decrement it. Failing this way leaves the request non-terminal: redelivery re-runs both steps and decrements again, transiently over-admitting by one slot until the zero clamp reconverges. Over-admission is the failure mode this pipeline already prefers, for the same reason and in the same words as the DLQ reconciler (see [process.md](doc/rfc/stovepipe/steps/process.md#in_flight_count-integrity)).
@@ -99,14 +96,14 @@ For a delivery carrying build id `B`:
9996

10097
Returning `TargetGraph` from `Status` in place of `BuildMetadata`, with `buildsignal` persisting it for `analyze` to read later, was considered and set aside — how `analyze` obtains the target graph is left to its own design, not `buildsignal`'s poll loop.
10198

102-
## Polling primitive: `PublishAfter`, not `Nack`
99+
## Polling primitive: hold, not `Nack`
103100

104-
On non-terminal status, step 8 reschedules with `PublishAfter`, never `Nack`:
101+
On non-terminal status, step 8 holds the delivery, never `Nack`s:
105102

106103
- **`Nack`** requeues and increments `retry_count`; at `MaxAttempts` the message dead-letters. That is the primitive for "something failed; retry."
107-
- **`PublishAfter`** emits a fresh message with `retry_count` reset to 0, deferred by a delay. That is the primitive for "still working; check back later."
104+
- **Hold** postpones the same delivery for a delay; the redelivery restarts failure accounting. That is the primitive for "still working; check back later."
108105

109-
Polling is a scheduled heartbeat, neither failure nor retry, so a long-running build never burns `retry_count` toward the DLQ. A genuine `Status` failure (runner down, bad id) is a *different* path: it returns from step 5 to the classifier, which decides retryability, and a retryable verdict nacks normally. See [build-runner.md](doc/rfc/submitqueue/build-runner.md#polling-primitive-publishafter-not-nack) for the full rationale.
106+
Polling is a scheduled heartbeat, neither failure nor retry, so a long-running build never burns `retry_count` toward the DLQ. A genuine `Status` failure (runner down, bad id) is a *different* path: it returns from step 5 to the classifier, which decides retryability, and a retryable verdict nacks normally. See [consumer-hold.md](doc/rfc/consumer-hold.md) for the primitive's full rationale.
110107

111108
## Poll delays
112109

@@ -130,16 +127,16 @@ Per `platform/errs`'s non-retryable-by-default rule (see [platform/errs/README.m
130127

131128
`Build`/`Request` not found (`storage.ErrNotFound`) are **not** in this table: storage is required to be read-after-write consistent (see [storage README](stovepipe/extension/storage/README.md)), so a miss here is already the correct default (non-retryable, straight to DLQ) rather than a departure worth overriding.
132129

133-
Everything else — factory lookup, an `Update` store error other than a CAS conflict, and both publishes — is returned raw with no override, because the default is already correct: a queue with no registered runner is a config error, and storage/queue failures dead-letter and let DLQ reconciliation recover. The `PublishAfter` re-poll is included in that: per `platform/errs` rule 4 a failed queue publish is not wrapped retryable just because replaying it is convenient, which would turn a permanent enqueue failure into an infinite retry instead of dead-lettering.
130+
Everything else — factory lookup, an `Update` store error other than a CAS conflict, and the `record` publish — is returned raw with no override, because the default is already correct: a queue with no registered runner is a config error, and storage/queue failures dead-letter and let DLQ reconciliation recover. The poll loop itself no longer has a publish to fail: holding is a local outcome, and a failed postpone write in the framework lapses into a normal visibility-timeout redelivery, so the loop's liveness never rides on an enqueue succeeding.
134131

135132
## Idempotency
136133

137134
Every branch is safe under at-least-once redelivery:
138135

139136
- **Build not found** — non-retryable; storage's read-after-write guarantee means a miss here is a storage defect, not a lag condition to retry through.
140-
- **Status already persisted** — a redelivery re-runs the whole algorithm from step 1, including a redundant `Status` poll (harmless — the runner reports the same thing); step 6 no-ops on the unchanged status, and the delivery proceeds to re-schedule the poll (non-terminal) or republish the request id to `record` (terminal, idempotent). No corruption.
137+
- **Status already persisted** — a redelivery re-runs the whole algorithm from step 1, including a redundant `Status` poll (harmless — the runner reports the same thing); step 6 no-ops on the unchanged status, and the delivery proceeds to hold for the next poll (non-terminal) or republish the request id to `record` (terminal, idempotent). No corruption.
141138
- **Terminal already published** — a redelivery reloads, re-polls, no-ops at step 6, republishes the same terminal signal to `record` (idempotent), and acks. Harmless.
142-
- **`PublishAfter` failed, then retried** — the nacked delivery re-runs from step 1; there is no way to resume mid-algorithm, so it re-polls the runner too, but the row already carries the non-terminal status and step 6 no-ops. Only the final enqueue does new work.
139+
- **Postpone write failed** — the framework abandons the delivery; the visibility timeout lapses into a normal redelivery, which re-runs from step 1 and no-ops at step 6. The poll loop's continuation is framework-owned.
143140

144141
The window to guard is between persisting status (step 6) and ack (steps 7–8); because status writes are CAS-guarded, monotonic, and write-once at terminal, a redelivery always observes a consistent row.
145142

@@ -152,7 +149,7 @@ The window to guard is between persisting status (step 6) and ack (steps 7–8);
152149

153150
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.
154151

155-
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.
152+
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 hold chain (whose redeliveries deliberately do not count toward the retry limit) 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.
156153

157154
## Entity, storage, and queue additions
158155

0 commit comments

Comments
 (0)