diff --git a/doc/rfc/stovepipe/steps/buildsignal.md b/doc/rfc/stovepipe/steps/buildsignal.md index 67fb1c60..000454a3 100644 --- a/doc/rfc/stovepipe/steps/buildsignal.md +++ b/doc/rfc/stovepipe/steps/buildsignal.md @@ -74,15 +74,12 @@ For a delivery carrying build id `B`: - publish failure -> return raw (non-retryable); the outcome is persisted, operational republish recovers. -8. Else PublishAfter(B -> buildsignal, delayMs), partitioned by build id: +8. Else hold the delivery for delayMs (postpone: the same message redelivers after the delay): - delayMs = pollDelay(status): shorter while running, longer while accepted. - - a fresh message (retry_count resets to 0), not a nack — polling is not failure. - - the message id must be unique per tick. The queue dedups on (topic, partition_key, id) - and the delivery being processed is still un-acked, so its row is present: reusing B as - the message id makes every re-poll collide with the message that scheduled it and be - silently discarded, ending the poll loop after one tick. - - publish failure -> return raw (non-retryable), same posture as step 7. - - ack. + - a hold is a postpone, not a nack — the redelivery does not count toward the retry + limit, so polling never burns retry_count toward the DLQ. + - the held message is a barrier for its partition (the build id), so each build keeps + exactly one poll chain; no message ids are minted and no new rows are written per tick. ``` **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`: 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. -## Polling primitive: `PublishAfter`, not `Nack` +## Polling primitive: hold, not `Nack` -On non-terminal status, step 8 reschedules with `PublishAfter`, never `Nack`: +On non-terminal status, step 8 holds the delivery, never `Nack`s: - **`Nack`** requeues and increments `retry_count`; at `MaxAttempts` the message dead-letters. That is the primitive for "something failed; retry." -- **`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." +- **Hold** postpones the same delivery for a delay; the redelivery restarts failure accounting. That is the primitive for "still working; check back later." -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. +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. ## Poll delays @@ -130,16 +127,16 @@ Per `platform/errs`'s non-retryable-by-default rule (see [platform/errs/README.m `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. -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. +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. ## Idempotency Every branch is safe under at-least-once redelivery: - **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. -- **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. +- **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. - **Terminal already published** — a redelivery reloads, re-polls, no-ops at step 6, republishes the same terminal signal to `record` (idempotent), and acks. Harmless. -- **`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. +- **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. 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. @@ -152,7 +149,7 @@ The window to guard is between persisting status (step 6) and ack (steps 7–8); 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. -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. +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. ## Entity, storage, and queue additions diff --git a/doc/rfc/stovepipe/steps/process.md b/doc/rfc/stovepipe/steps/process.md index f297a395..7f48603a 100644 --- a/doc/rfc/stovepipe/steps/process.md +++ b/doc/rfc/stovepipe/steps/process.md @@ -26,7 +26,7 @@ For a delivery carrying request id `R`: 5. Coalesce: if CompareRequestID(R.Queue, R.ID, Q.latest_request_id) < 0: - a newer head exists -> mark R superseded, ack, return. (No slot consumed.) 6. R is the latest head. Gate: if Q.in_flight_count >= max_concurrent (from queue config; see below): - - defer (Option 1 or Option 2 below) -> re-check until the slot frees (admit) or a newer head supersedes it. See [Waiting for a slot](#waiting-for-a-slot). + - defer (hold the delivery) -> re-check on redelivery until the slot frees (admit) or a newer head supersedes it. See [Waiting for a slot](#waiting-for-a-slot). 7. Admit R: a. Derive build strategy + baseline (see "Build-strategy decision"). b. CAS the Queue row: in_flight_count += 1. @@ -102,7 +102,7 @@ Why not `SourceControl.History`: a history walk is expensive, and after a rewrit Ordering caveat: `counter.Next` doesn't guarantee assignment order, so under concurrent same-Queue ingest (rare — one serial poller) "highest sequence" may not equal "most recently reported". They agree in practice, and a rare inversion self-corrects next poll. Acceptable for MVP. -**The pointer prevents deadlock.** Under `BatchSize = 1`, Option 1 blocks its partition while waiting, so `process` can't learn of newer heads *from the stream* during that wait. Option 2 unblocks the partition, so newer ingest deliveries can arrive immediately. Both options read `latest_request_id` from the Queue row on every wake-up (step 5), so a stale waiter — blocked or delayed — still supersedes correctly. Ingest stamps the pointer independently of the partition (see [Backlog coalescing](#backlog-coalescing)). +**The pointer prevents deadlock.** A held head blocks its partition while waiting, so `process` can't learn of newer heads *from the stream* during that wait. The waiter re-reads `latest_request_id` from the Queue row on every wake-up (step 5), so a stale waiter still supersedes correctly. Ingest stamps the pointer independently of the partition (see [Backlog coalescing](#backlog-coalescing)). **Progress (no starvation).** Superseding is always forward motion toward the newest head, and the newest head is never superseded (nothing is newer). So as long as `process` supersedes faster than ingest adds heads — it does, since superseding is a CAS + ack with no build, far cheaper than the poll cadence — a build always starts; a high commit rate just coalesces more intermediates away. @@ -124,7 +124,7 @@ The gate is **not** tied to `process` returning; a slot taken at admit is held u 1. **A** admitted (`in_flight_count = 1`), published to `build`. 2. While A runs, **B**, **C**, **D** are ingested (`latest_request_id = D.id`). -3. B: older than D → **superseded** (acked), though A is still in flight. Same for **C**. D is latest but the gate is closed → **waits for slot** (Option 1 or 2). +3. B: older than D → **superseded** (acked), though A is still in flight. Same for **C**. D is latest but the gate is closed → **waits for slot** (held). 4. A's build finishes → `record` records A's greenness, `in_flight_count → 0`. 5. D's re-check → gate open, D still latest → **D admitted**, published to `build`. 6. While D runs, **E**, **F** ingested (`latest_request_id = F.id`). E superseded on sight; F waits for slot. @@ -145,7 +145,7 @@ Every branch is safe under redelivery: - **accepted, no strategy** → full admit path. On a crash after incrementing `in_flight_count` but before persisting `processing`, redelivery re-reads `accepted` and re-runs; the increment re-applies only if the count CAS hasn't already moved (see integrity below). - **processing** → re-publish to `build` and ack. The `build` consumer is keyed on the request id and idempotent, so a duplicate publish is harmless. - **terminal** (superseded / recorded) → ack, no-op. -- **deferred (waiting for slot)** → no state or count change; pure deferral (re-enters on renewal or reschedule). +- **deferred (waiting for slot)** → no state or count change; pure deferral (re-enters when the held delivery comes due). The window to handle is "count incremented, state not yet `processing`". Admit does the increment and the state transition as two ordered CAS writes, and the decrement is tied to the state transition, not a side counter (see integrity below). @@ -212,88 +212,35 @@ No "list requests by queue/state" query is introduced; coalescing uses the singl ## Waiting for a slot -When the gate is closed, `process` must defer the latest head without admitting it (no `in_flight_count` increment, no publish to `build`). Two options: +When the gate is closed, `process` must defer the latest head without admitting it (no `in_flight_count` increment, no publish to `build`). The mechanism is the consumer hold primitive ([consumer-hold.md](../../consumer-hold.md)): the controller records a hold for `gate_wait_delay_ms` and returns success, and the framework postpones the delivery — the same message redelivers after the delay, and the redelivery does not count toward `MaxAttempts`. -- Park until a build slot opens, and extend visibility -- Use PublishAfter to re-enqeue the current head if no build slot is available - -Both re-run the same **coalesce-then-gate** checks on every wake-up (steps 5 → 6): +Every wake-up re-runs the same **coalesce-then-gate** checks (steps 5 → 6): 1. **Stale? (checked first.)** If `CompareRequestID(R.Queue, R.ID, Q.latest_request_id) < 0`, `R` is no longer latest → supersede it (ack). A newer head is admitted by its own delivery when its slot attempt runs. 2. **Slot free?** If `in_flight_count < max_concurrent` (from config) and `R` is still latest → admit (step 7). -Neither option admits to `build` until the gate opens. - -### Option 1: park and extend visibility - -**Mechanism.** Keep the in-flight delivery alive. Loop: call `ExtendVisibilityTimeout` on an interval (renews the lease **without** incrementing `retry_count`), reload the Queue row, run coalesce-then-gate. Never ack or nack while waiting. Honor context cancellation — on shutdown return promptly; the head resumes on redelivery. +Nothing is admitted to `build` until the gate opens. -**Partition behavior.** Under `BatchSize = 1`, the delivery stays in-flight and **blocks the partition** until it admits or supersedes. Newer ingest messages queue behind it in the log; coalescing for the waiting head relies on `latest_request_id` from the Queue row, not on newer deliveries arriving while blocked. +**Partition behavior.** A postponed message is a barrier: the queue's partition waits with the held head, and later process messages for the same queue deliver only after it redelivers, in order. Coalescing does not depend on those later deliveries running promptly — `latest_request_id` is stamped by ingest, not by queue consumption, so the waking head reads the Queue row and supersedes itself when a newer head arrived; the intermediates then supersede on sight as the partition drains behind it. -**Walkthrough** — Queue `monorepo/main`, `max_concurrent = 1`, heads A→F, Option 1 chosen: +**Walkthrough** — Queue `monorepo/main`, `max_concurrent = 1`, heads A→F: 1. **A** admitted, published to `build` (`in_flight_count = 1`). `process` returns (acks); A continues through `build → buildsignal → record`. 2. **B**, **C** ingested. Their deliveries run behind A's in-flight validation (not behind a gate wait yet) → superseded on sight (step 5), acked. -3. **D** ingested (`latest_request_id = D.id`). D's delivery: latest, gate closed → **park** (extend loop begins; partition blocked). -4. While D waits, **E**, **F** ingested (`latest_request_id = F.id`). Their process messages sit in the log behind D's blocked delivery — they do not run yet. -5. D's renew loop re-reads the Queue row → D is older than F → **supersede D**, ack (partition unblocks). -6. **E**'s delivery runs → superseded. **F**'s delivery runs → latest, gate still closed → **park**. -7. A completes at `record` → `in_flight_count → 0`. F's renew loop sees gate open → **admit F**, publish to `build`, ack. - -**Supersede reasoning.** Simple while blocked: the waiter periodically re-reads `latest_request_id` and supersedes itself when a newer head appears. Intermediates (B, C, E) only run once the partition unblocks enough to reach their offsets. - -**Tradeoffs.** - -| Pros | Cons | -|---|---| -| Delivery API only — no publisher in `process` | Goroutine blocked in renew loop per waiting head | -| One delivery, minimal log churn | Partition blocked — newer heads wait in the log | -| `ExtendVisibility` does not increment `retry_count` | Lease lapses (missed renewal) increment `retry_count` → `MaxAttempts` risk | -| Strict in-partition serialization while waiting | Tune renewal interval inside `VisibilityTimeoutMs` | - -**Safety.** If renewal lapses, another worker may redeliver the same head concurrently. Harmless: admission is CAS-guarded; one admit wins, the other sees `processing` (step 3) and no-ops. - -### Option 2: ack and `PublishAfter` - -**Mechanism.** On gate closed and still latest: **ack** the current delivery, then **`PublishAfter`** the same `ProcessRequest` to the process topic (same partition key = queue name) after a short delay. Each wake-up is a **fresh** message (`retry_count` starts at 0). Run coalesce-then-gate at the top of every wake-up; if still latest and gate still closed, ack and `PublishAfter` again. Only reschedule when both conditions hold — otherwise supersede or admit immediately. +3. **D** ingested (`latest_request_id = D.id`). D's delivery: latest, gate closed → **hold** (postponed for `gate_wait_delay_ms`; the partition waits behind D). +4. While D waits, **E**, **F** ingested (`latest_request_id = F.id`). Their process messages sit behind D's postponed row — they do not run yet. +5. D's hold expires → D redelivers first, re-reads the Queue row → D is older than F → **supersede D**, ack (partition drains). **E**'s delivery runs → superseded. **F**'s delivery runs → latest, gate still closed → **hold**. +6. A completes at `buildsignal` → `in_flight_count → 0`. F's hold expires → gate open, still latest → **admit F**, publish to `build`, ack. -**Partition behavior.** Acks free the partition. Newer ingest deliveries (E, F, …) are processed and superseded while the latest head waits on a timer. Deferred rows use `visible_after` and are skipped until due — the same non-blocking property as a nacked message — so a reschedule at offset 11 can run *after* a newer ingest message at offset 12. Delivery order reshuffles relative to strict log order; coalescing keys off `latest_request_id`, not offset. - -**Walkthrough** — same scenario, Option 2 chosen: - -1. **A** admitted, published to `build` (`in_flight_count = 1`). -2. **B**, **C** ingested → delivered and **superseded** on sight. -3. **D** ingested (`latest_request_id = D.id`). D's delivery: latest, gate closed → **ack + `PublishAfter(D, delay)`**. Partition free. -4. **E**, **F** ingested (`latest_request_id = F.id`). **E** delivered → superseded. **F** delivered → latest, gate closed → **ack + `PublishAfter(F, delay)`**. D's pending reschedule is now redundant. -5. D's timer fires → D is older than F → **supersede D**, ack (cheap no-op). -6. A completes at `record` → `in_flight_count → 0`. F's timer fires → gate open, still latest → **admit F**, publish to `build`, ack. - -**Supersede reasoning.** Straightforward: every wake-up (immediate or delayed) runs step 5 first. Stale delayed messages (D) supersede on sight. Only the current latest (F) should schedule the next wait. Older delayed rows are expected no-ops, not errors. - -**Tradeoffs.** - -| Pros | Cons | -|---|---| -| No `MaxAttempts` burn while waiting (each cycle acks; reschedule is fresh) | Ack + new log row per poll cycle while gate is closed | -| Partition stays hot — intermediates supersede immediately | `process` needs publisher + topic registry | -| Worker returns between waits — no blocked goroutine | Delivery order ≠ ingest order (correctness unaffected) | -| Stale delayed waiters self-clean via step 5 | Redundant delayed rows if multiple heads reschedule before timers fire | - -### Comparison - -| | **Option 1: park + extend** | **Option 2: ack + `PublishAfter`** | -|---|---|---| -| **`MaxAttempts`** | Safe when renewals keep up; lease lapses increment `retry_count` | Safe — waiting never increments `retry_count` | -| **Partition (`BatchSize = 1`)** | Blocks until admit or supersede | Unblocks; newer heads process immediately | -| **Supersede** | Waiter polls `latest_request_id` in loop | Immediate deliveries + stale timers supersede on wake-up | -| **Churn** | One delivery, periodic extends | One new log row per wait cycle | -| **Wiring** | `ExtendVisibilityTimeout` on `Delivery` | Publisher + topic registry in controller | -| **Worker** | Goroutine in renew loop | Returns; timer brings work back | +**Properties.** This section previously weighed two options — park-and-extend-visibility versus ack-and-`PublishAfter` — and deferred the choice to a future consumer primitive. Hold is that primitive, and it dominates both: -Implementation picks one option and wires step 6 to it. A future `consumer.ErrHold` primitive would resemble Option 2 (release + redeliver) without self-republish; neither option is chosen here yet. +- **`MaxAttempts` safe** — a postponed redelivery restarts failure accounting, so waiting never burns retries toward the DLQ; only genuine failures do (park-and-extend risked lease lapses charging retries). +- **No blocked worker, no lease** — the delivery is finalized between wake-ups; no goroutine sits in a renew loop and no visibility lease can lapse mid-wait. +- **No self-publish** — `process` never publishes to its own topic, so there is no message-id minting to dodge the queue's `(topic, partition_key, id)` dedup and no new log row per wait cycle; a failed postpone write lapses into a normal visibility-timeout redelivery, so the wait's liveness is framework-owned rather than riding on an enqueue succeeding. +- **Ordering** — the partition blocks behind the waiting head, so intermediates supersede when the partition drains rather than immediately; correctness rides on `latest_request_id`, not on delivery order. ## Batch consume -Coalescing uses the latest-request pointer one delivery at a time. Intermediates are each delivered once and superseded. The latest head adds no extra rows under Option 1 (it waits in place); under Option 2 it adds one reschedule row per wait cycle while the gate is closed. +Coalescing uses the latest-request pointer one delivery at a time. Intermediates are each delivered once and superseded. The waiting head adds no extra rows — a hold postpones the existing message in place. If that churn matters at scale, an optional `BatchController` (receiving `[]Delivery` per poll) would let `process` supersede all intermediates in a single tick — an optimization over the single-delivery path that can land later without changing the state machine or storage contract. diff --git a/doc/rfc/stovepipe/workflow.md b/doc/rfc/stovepipe/workflow.md index 93e3c132..e485165d 100644 --- a/doc/rfc/stovepipe/workflow.md +++ b/doc/rfc/stovepipe/workflow.md @@ -156,7 +156,7 @@ Per-stage design detail lives under `steps/` so this doc stays a pipeline overvi - [process.md](steps/process.md) — build-strategy decision, concurrency gate, backlog coalescing, [concurrency lifecycle](steps/process.md#concurrency-lifecycle), entity changes, [waiting for a slot](steps/process.md#waiting-for-a-slot) - [build.md](steps/build.md) — trigger-only stage: reads the decided scope off the Request, triggers the build-runner, hands off to buildsignal; the stovepipe `BuildRunner` contract and why it differs from SubmitQueue's -- [buildsignal.md](steps/buildsignal.md) — the poll loop: `PublishAfter` re-poll cadence, target-graph return, per-build partitioning, and the fail-closed handoff to record +- [buildsignal.md](steps/buildsignal.md) — the poll loop: hold-based re-poll cadence, target-graph return, per-build partitioning, and the fail-closed handoff to record ## Dedup, idempotency, and history rewrites diff --git a/stovepipe/controller/buildsignal/buildsignal.go b/stovepipe/controller/buildsignal/buildsignal.go index 4fbff6ab..fb265319 100644 --- a/stovepipe/controller/buildsignal/buildsignal.go +++ b/stovepipe/controller/buildsignal/buildsignal.go @@ -24,8 +24,6 @@ import ( "context" "errors" "fmt" - "strconv" - "strings" "github.com/uber-go/tally" entityqueue "github.com/uber/submitqueue/platform/base/messagequeue" @@ -54,9 +52,10 @@ var ( ) // Controller consumes BuildSignal messages, polls the build-runner toward a -// terminal status, persists the result, and either reschedules itself or -// releases the queue's build slot, projects the outcome onto the request, and -// publishes the request id to record. Implements consumer.Controller. +// terminal status, persists the result, and either holds the delivery until +// the next poll or releases the queue's build slot, projects the outcome onto +// the request, and publishes the request id to record. Implements +// consumer.Controller. type Controller struct { logger *zap.SugaredLogger metricsScope tally.Scope @@ -95,10 +94,11 @@ func NewController( } // Process reloads the build referenced by the delivery, polls its runner for -// the latest status, persists a real transition, and either reschedules a -// poll or, once terminal, releases the queue's build slot, projects the -// outcome onto the request, and publishes the request id to record. Returns -// nil to ack (success) or an error to nack (retry) / reject (DLQ). +// the latest status, persists a real transition, and either holds the delivery +// until the next poll or, once terminal, releases the queue's build slot, +// projects the outcome onto the request, and publishes the request id to +// record. Returns nil to ack (success) or an error to nack (retry) / reject +// (DLQ). func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) error { msg := delivery.Message() @@ -168,11 +168,12 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er return nil } + // Not terminal yet: hold the delivery so this same message redelivers + // after the poll delay — the partition (keyed by build id) sleeps with it, + // and the redelivery does not count toward the retry limit. delayMs := pollDelay(effective) - if err := c.publishBuildSignal(ctx, build.ID, msg.ID, delayMs); err != nil { - return fmt.Errorf("failed to reschedule poll for build %s: %w", build.ID, err) - } - c.logger.Debugw("rescheduled build status poll", + delivery.Hold(delayMs) + c.logger.Debugw("holding for next build status poll", "build_id", build.ID, "status", string(effective), "delay_ms", delayMs, @@ -345,57 +346,11 @@ func (c *Controller) publishRecord(ctx context.Context, requestID string) error return fmt.Errorf("failed to serialize record: %w", err) } msg := entityqueue.NewMessage(requestID, payload, requestID, nil) - return c.publish(ctx, stovepipemq.TopicKeyRecord, msg, 0) + return c.publish(ctx, stovepipemq.TopicKeyRecord, msg) } -// pollIDInfix separates a build id from its poll generation in a re-poll -// message id: "/poll/". -const pollIDInfix = "/poll/" - -// nextPollMessageID returns the message id for the next poll of buildID, one -// generation past the delivery that scheduled it. currentMsgID is the id of the -// message being processed — either the initial publish from build (no -// generation, so the next is 1) or a previous re-poll. -// -// The generation has to advance because the queue dedups on -// (topic, partition_key, id) and the delivery being processed has not been acked -// yet, so its row is still present: a re-poll reusing the current id would -// collide with the message that scheduled it and be silently discarded, ending -// the poll loop after one tick. -// -// It advances deterministically rather than randomly so the id stays a pure -// function of the delivery. A redelivery racing the original computes the same -// next id, so dedup collapses the two into one message and the build keeps a -// single poll chain — where a random suffix would fork a second chain that -// doubles the poll rate and races the first one's status CAS. -func nextPollMessageID(buildID, currentMsgID string) string { - generation := 0 - if rest, found := strings.CutPrefix(currentMsgID, buildID+pollIDInfix); found { - if n, err := strconv.Atoi(rest); err == nil && n > 0 { - generation = n - } - } - return fmt.Sprintf("%s%s%d", buildID, pollIDInfix, generation+1) -} - -// publishBuildSignal re-publishes buildID to buildsignal after delayMs, -// partitioned by build id so each build's poll loop runs in its own -// partition. A fresh message, not a nack — polling is not failure. -// -// currentMsgID is the id of the delivery being processed; see nextPollMessageID -// for why the new id is derived from it rather than reused or randomized. -func (c *Controller) publishBuildSignal(ctx context.Context, buildID, currentMsgID string, delayMs int64) error { - payload, err := stovepipemq.Marshal(&stovepipemq.BuildSignal{Id: buildID}) - if err != nil { - return fmt.Errorf("failed to serialize build signal: %w", err) - } - msg := entityqueue.NewMessage(nextPollMessageID(buildID, currentMsgID), payload, buildID, nil) - return c.publish(ctx, stovepipemq.TopicKeyBuildSignal, msg, delayMs) -} - -// publish sends msg to the queue registered for key, using PublishAfter when -// delayMs > 0 and Publish otherwise. -func (c *Controller) publish(ctx context.Context, key consumer.TopicKey, msg entityqueue.Message, delayMs int64) error { +// publish sends msg to the queue registered for key. +func (c *Controller) publish(ctx context.Context, key consumer.TopicKey, msg entityqueue.Message) error { q, ok := c.registry.Queue(key) if !ok { return fmt.Errorf("no queue registered for topic key %s", key) @@ -404,9 +359,6 @@ func (c *Controller) publish(ctx context.Context, key consumer.TopicKey, msg ent if !ok { return fmt.Errorf("no topic name registered for topic key %s", key) } - if delayMs > 0 { - return q.Publisher().PublishAfter(ctx, topicName, msg, delayMs) - } return q.Publisher().Publish(ctx, topicName, msg) } diff --git a/stovepipe/controller/buildsignal/buildsignal_test.go b/stovepipe/controller/buildsignal/buildsignal_test.go index 7a2aab48..53daa39d 100644 --- a/stovepipe/controller/buildsignal/buildsignal_test.go +++ b/stovepipe/controller/buildsignal/buildsignal_test.go @@ -84,7 +84,7 @@ func newController(t *testing.T, ctrl *gomock.Controller) (*Controller, buildsig return c, m } -func delivery(t *testing.T, ctrl *gomock.Controller, payload []byte) consumer.Delivery { +func delivery(t *testing.T, ctrl *gomock.Controller, payload []byte) *consumermock.MockDelivery { t.Helper() d := consumermock.NewMockDelivery(ctrl) d.EXPECT().Message().Return(entityqueue.NewMessage(testBuildID, payload, testBuildID, nil)).AnyTimes() @@ -139,11 +139,14 @@ func expectFinish(m buildsignalMocks, state entity.RequestState) { func TestProcess(t *testing.T) { tests := []struct { - name string - payload []byte - setup func(m buildsignalMocks) - wantErr bool - wantRetry bool + name string + payload []byte + setup func(m buildsignalMocks) + // wantHoldMs, when non-zero, expects the delivery to be held for the + // next poll with exactly this delay. Cases without it fail on any Hold. + wantHoldMs int64 + wantErr bool + wantRetry bool }{ { name: "build not found is not retryable", @@ -218,17 +221,18 @@ func TestProcess(t *testing.T) { }, }, { - name: "unchanged status skips write and reschedules", + name: "unchanged status skips write and holds for next poll", + wantHoldMs: PollDelayRunningMs, setup: func(m buildsignalMocks) { m.buildStore.EXPECT().Get(gomock.Any(), testBuildID).Return(build(entity.BuildStatusRunning, 2), nil) m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(requestWithState(entity.RequestStateProcessing), nil) m.runnerFactory.EXPECT().For(buildrunner.Config{QueueName: testQueue}).Return(m.runner, nil) m.runner.EXPECT().Status(gomock.Any(), entity.BuildID{ID: testBuildID}).Return(entity.BuildStatusRunning, nil, nil) - m.publisher.EXPECT().PublishAfter(gomock.Any(), "buildsignal", gomock.Any(), PollDelayRunningMs).Return(nil) }, }, { - name: "status transition persists and reschedules", + name: "status transition persists and holds for next poll", + wantHoldMs: PollDelayRunningMs, setup: func(m buildsignalMocks) { m.buildStore.EXPECT().Get(gomock.Any(), testBuildID).Return(build(entity.BuildStatusAccepted, 1), nil) m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(requestWithState(entity.RequestStateProcessing), nil) @@ -236,7 +240,6 @@ func TestProcess(t *testing.T) { m.runner.EXPECT().Status(gomock.Any(), entity.BuildID{ID: testBuildID}).Return(entity.BuildStatusRunning, nil, nil) updated := build(entity.BuildStatusRunning, 1) m.buildStore.EXPECT().Update(gomock.Any(), updated, int32(1), int32(2)).Return(nil) - m.publisher.EXPECT().PublishAfter(gomock.Any(), "buildsignal", gomock.Any(), PollDelayRunningMs).Return(nil) }, }, { @@ -367,18 +370,6 @@ func TestProcess(t *testing.T) { m.publisher.EXPECT().Publish(gomock.Any(), "record", gomock.Any()).Return(errors.New("queue down")) }, }, - { - name: "reschedule publish failure is not retryable", - wantErr: true, - wantRetry: false, - setup: func(m buildsignalMocks) { - m.buildStore.EXPECT().Get(gomock.Any(), testBuildID).Return(build(entity.BuildStatusAccepted, 1), nil) - m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(requestWithState(entity.RequestStateProcessing), nil) - m.runnerFactory.EXPECT().For(buildrunner.Config{QueueName: testQueue}).Return(m.runner, nil) - m.runner.EXPECT().Status(gomock.Any(), entity.BuildID{ID: testBuildID}).Return(entity.BuildStatusAccepted, nil, nil) - m.publisher.EXPECT().PublishAfter(gomock.Any(), "buildsignal", gomock.Any(), PollDelayAcceptedMs).Return(errors.New("queue down")) - }, - }, { name: "malformed payload is not retryable", payload: []byte("not-json"), @@ -401,7 +392,12 @@ func TestProcess(t *testing.T) { payload = buildSignalPayload(t, testBuildID) } - err := c.Process(context.Background(), delivery(t, ctrl, payload)) + d := delivery(t, ctrl, payload) + if tt.wantHoldMs > 0 { + d.EXPECT().Hold(tt.wantHoldMs) + } + + err := c.Process(context.Background(), d) if tt.wantErr { require.Error(t, err) @@ -436,80 +432,6 @@ func TestPublishRecordCarriesRequestID(t *testing.T) { assert.Equal(t, testID, got.PartitionKey) } -// TestPublishBuildSignalAdvancesPollGeneration is the regression test for the poll -// loop stalling. The queue dedups on (topic, partition_key, id) and the delivery that -// scheduled a re-poll is still un-acked when the re-poll is published, so a reused -// message id makes the reschedule a silent no-op and the build is never polled again. -// The generation therefore has to advance each tick. The partition key must stay the -// build id so each poll loop keeps its own partition. -func TestPublishBuildSignalAdvancesPollGeneration(t *testing.T) { - ctrl := gomock.NewController(t) - c, m := newController(t, ctrl) - - var ids []string - m.publisher.EXPECT().PublishAfter(gomock.Any(), "buildsignal", gomock.Any(), PollDelayRunningMs). - DoAndReturn(func(_ context.Context, _ string, msg entityqueue.Message, _ int64) error { - ids = append(ids, msg.ID) - assert.Equal(t, testBuildID, msg.PartitionKey) - return nil - }).Times(3) - - // Walk a chain: each publish is scheduled by the message the previous one minted. - current := testBuildID - for range 3 { - require.NoError(t, c.publishBuildSignal(context.Background(), testBuildID, current, PollDelayRunningMs)) - current = ids[len(ids)-1] - } - - assert.Equal(t, []string{ - testBuildID + "/poll/1", - testBuildID + "/poll/2", - testBuildID + "/poll/3", - }, ids, "each tick must mint a fresh id, or the reschedule dedups against the message that scheduled it") -} - -// TestPublishBuildSignalIsIdempotentPerDelivery pins the other half of the contract: -// the next id is a pure function of the delivery being processed. A redelivery racing -// the original computes the same id, so dedup collapses them and the build keeps a -// single poll chain rather than forking a second one that doubles the poll rate and -// races the first one's status CAS. -func TestPublishBuildSignalIsIdempotentPerDelivery(t *testing.T) { - ctrl := gomock.NewController(t) - c, m := newController(t, ctrl) - - var ids []string - m.publisher.EXPECT().PublishAfter(gomock.Any(), "buildsignal", gomock.Any(), PollDelayRunningMs). - DoAndReturn(func(_ context.Context, _ string, msg entityqueue.Message, _ int64) error { - ids = append(ids, msg.ID) - return nil - }).Times(2) - - scheduledBy := testBuildID + "/poll/7" - require.NoError(t, c.publishBuildSignal(context.Background(), testBuildID, scheduledBy, PollDelayRunningMs)) - require.NoError(t, c.publishBuildSignal(context.Background(), testBuildID, scheduledBy, PollDelayRunningMs)) - - assert.Equal(t, ids[0], ids[1], "a redelivery of the same message must republish the same id so dedup collapses it") - assert.Equal(t, testBuildID+"/poll/8", ids[0]) -} - -func TestNextPollMessageID(t *testing.T) { - tests := []struct { - name string - current string - expected string - }{ - {name: "initial publish from build starts at one", current: testBuildID, expected: testBuildID + "/poll/1"}, - {name: "advances the generation", current: testBuildID + "/poll/4", expected: testBuildID + "/poll/5"}, - {name: "unparsable generation restarts at one", current: testBuildID + "/poll/x", expected: testBuildID + "/poll/1"}, - {name: "another build's id is not a prefix match", current: "other/poll/9", expected: testBuildID + "/poll/1"}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.expected, nextPollMessageID(testBuildID, tt.current)) - }) - } -} - func TestOutcomeState(t *testing.T) { tests := []struct { name string diff --git a/stovepipe/controller/process/process.go b/stovepipe/controller/process/process.go index 1b46e651..f02b864e 100644 --- a/stovepipe/controller/process/process.go +++ b/stovepipe/controller/process/process.go @@ -22,7 +22,6 @@ import ( "context" "errors" "fmt" - "time" "github.com/uber-go/tally" entityqueue "github.com/uber/submitqueue/platform/base/messagequeue" @@ -111,7 +110,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er // A stale redelivery has nothing left to do. return nil case entity.RequestStateAccepted: - return c.processAccepted(ctx, request) + return c.processAccepted(ctx, delivery, request) default: c.logger.Warnw("ignored request in unexpected state", "request_id", request.ID, @@ -123,8 +122,9 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er } // processAccepted coalesces older heads against queue.latest_request_id, then admits -// the latest head when a build slot is available. -func (c *Controller) processAccepted(ctx context.Context, request entity.Request) error { +// the latest head when a build slot is available. The delivery is threaded down so a +// closed gate can hold it. +func (c *Controller) processAccepted(ctx context.Context, delivery consumer.Delivery, request entity.Request) error { queueRow, err := c.loadQueue(ctx, request.Queue) if err != nil { if !errs.IsRetryable(err) { @@ -154,7 +154,7 @@ func (c *Controller) processAccepted(ctx context.Context, request entity.Request return fmt.Errorf("failed to load queue config for %s: %w", request.Queue, err) } - return c.admitLatestHead(ctx, request, queueRow, cfg) + return c.admitLatestHead(ctx, delivery, request, queueRow, cfg) } // coalesce supersedes request when a newer head exists (RFC process step 5), returning @@ -184,8 +184,8 @@ func (c *Controller) coalesce(ctx context.Context, request entity.Request, lates // admitLatestHead runs the gate-then-admit workflow for the latest head: claim a build // slot, mark the request processing, and publish it to build. Every queue-row reload // re-runs coalesce-then-gate, so a slot is never spent on a now-stale head; a closed gate -// defers by rescheduling the request (ack after re-enqueue) rather than failing. -func (c *Controller) admitLatestHead(ctx context.Context, request entity.Request, queueRow entity.Queue, cfg entity.QueueConfig) error { +// defers by holding the delivery (redeliver after the gate wait delay) rather than failing. +func (c *Controller) admitLatestHead(ctx context.Context, delivery consumer.Delivery, request entity.Request, queueRow entity.Queue, cfg entity.QueueConfig) error { var sc sourcecontrol.SourceControl var strategy entity.BuildStrategy var baseURI string @@ -193,7 +193,7 @@ func (c *Controller) admitLatestHead(ctx context.Context, request entity.Request for { if queueRow.InFlightCount >= cfg.MaxConcurrent { - return c.rescheduleProcess(ctx, request, queueRow.InFlightCount, cfg.GateWaitDelayMs) + return c.holdForBuildSlot(delivery, request, queueRow.InFlightCount, cfg.GateWaitDelayMs) } if queueRow.LastGreenURI != "" && sc == nil { @@ -412,38 +412,18 @@ func (c *Controller) supersedeRequest(ctx context.Context, request entity.Reques } } -// rescheduleProcess re-enqueues the same ProcessRequest after a delay so the gate can be -// re-checked without burning MaxAttempts. delayMs must be positive. -func (c *Controller) rescheduleProcess(ctx context.Context, request entity.Request, inFlightCount int32, delayMs int64) error { +// holdForBuildSlot holds the delivery so the same ProcessRequest redelivers after +// delayMs and the gate is re-checked, without burning MaxAttempts — the partition +// (keyed by queue name) waits with it. delayMs must be positive: a non-positive +// hold would redeliver immediately and hot-loop the gate check. +func (c *Controller) holdForBuildSlot(delivery consumer.Delivery, request entity.Request, inFlightCount int32, delayMs int64) error { if delayMs <= 0 { metrics.NamedCounter(c.metricsScope, _opName, "config_errors", 1) return fmt.Errorf("requires a positive gate wait delay for queue %s, got %dms", request.Queue, delayMs) } - payload, err := stovepipemq.Marshal(&stovepipemq.ProcessRequest{Id: request.ID}) - if err != nil { - return fmt.Errorf("failed to serialize process request %s: %w", request.ID, err) - } - - // Suffix the message id with the publish time so the reschedule can't collide with - // the in-flight delivery's still-present message-store row. - msgID := fmt.Sprintf("%s/reschedule/%d", request.ID, time.Now().UnixMilli()) - msg := entityqueue.NewMessage(msgID, payload, request.Queue, nil) - - q, ok := c.registry.Queue(c.topicKey) - if !ok { - return fmt.Errorf("no queue registered for topic key %s", c.topicKey) - } - topicName, ok := c.registry.TopicName(c.topicKey) - if !ok { - return fmt.Errorf("no topic name registered for topic key %s", c.topicKey) - } - - if err := q.Publisher().PublishAfter(ctx, topicName, msg, delayMs); err != nil { - metrics.NamedCounter(c.metricsScope, _opName, "publish_errors", 1) - return fmt.Errorf("failed to reschedule process request %s: %w", request.ID, err) - } - c.logger.Infow("rescheduled latest head awaiting build slot", + delivery.Hold(delayMs) + c.logger.Infow("holding latest head awaiting build slot", "request_id", request.ID, "queue", request.Queue, "uri", request.URI, diff --git a/stovepipe/controller/process/process_test.go b/stovepipe/controller/process/process_test.go index 1f60edf5..770dbee4 100644 --- a/stovepipe/controller/process/process_test.go +++ b/stovepipe/controller/process/process_test.go @@ -45,14 +45,6 @@ const ( testURI = "git://repo/monorepo/main/abc123" ) -// rescheduledMsg matches a gate-wait re-publish: same partition, but a fresh message id — -// re-publishing under the in-flight delivery's id would be silently deduped against its -// still-present message-store row and lost on ack. -func rescheduledMsg(msg entityqueue.Message) bool { - // Fresh non-empty id, same queue. - return msg.ID != testID && msg.ID != "" && msg.PartitionKey == testQueue -} - type processMocks struct { reqStore *storagemock.MockRequestStore queueStore *storagemock.MockQueueStore @@ -101,7 +93,7 @@ func newControllerWithScope(t *testing.T, ctrl *gomock.Controller, scope tally.S return c, m } -func delivery(t *testing.T, ctrl *gomock.Controller, payload []byte) consumer.Delivery { +func delivery(t *testing.T, ctrl *gomock.Controller, payload []byte) *consumermock.MockDelivery { t.Helper() d := consumermock.NewMockDelivery(ctrl) d.EXPECT().Message().Return(entityqueue.NewMessage(testID, payload, testQueue, nil)).AnyTimes() @@ -412,12 +404,15 @@ func TestProcessRederivesStrategyAfterQueueReload(t *testing.T) { func TestProcess(t *testing.T) { tests := []struct { - name string - id string - payload []byte - setup func(m processMocks) - wantErr bool - wantRetry bool + name string + id string + payload []byte + setup func(m processMocks) + // wantHoldMs, when non-zero, expects the delivery to be held for the + // gate wait with exactly this delay. Cases without it fail on any Hold. + wantHoldMs int64 + wantErr bool + wantRetry bool }{ { name: "superseded is no-op", @@ -533,7 +528,8 @@ func TestProcess(t *testing.T) { }, }, { - name: "latest accepted head reschedules when gate closed", + name: "latest accepted head holds when gate closed", + wantHoldMs: 5000, setup: func(m processMocks) { m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(acceptedRequest(testID), nil) m.queueStore.EXPECT().Get(gomock.Any(), testQueue).Return(entity.Queue{ @@ -543,30 +539,11 @@ func TestProcess(t *testing.T) { LastGreenURI: "git://repo/monorepo/main/green", Version: 1, }, nil) - m.publisher.EXPECT(). - PublishAfter(gomock.Any(), "process", gomock.Cond(rescheduledMsg), int64(5000)). - Return(nil) - }, - }, - { - name: "gate reschedule publish error surfaces", - wantErr: true, - wantRetry: false, - setup: func(m processMocks) { - m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(acceptedRequest(testID), nil) - m.queueStore.EXPECT().Get(gomock.Any(), testQueue).Return(entity.Queue{ - Name: testQueue, - LatestRequestID: testID, - InFlightCount: 1, - Version: 1, - }, nil) - m.publisher.EXPECT(). - PublishAfter(gomock.Any(), "process", gomock.Cond(rescheduledMsg), int64(5000)). - Return(errors.New("queue down")) }, }, { - name: "gate closed after slot claim race reschedules", + name: "gate closed after slot claim race holds", + wantHoldMs: 5000, setup: func(m processMocks) { m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(acceptedRequest(testID), nil) m.queueStore.EXPECT().Get(gomock.Any(), testQueue).Return(entity.Queue{ @@ -586,9 +563,6 @@ func TestProcess(t *testing.T) { InFlightCount: 1, Version: 2, }, nil) - m.publisher.EXPECT(). - PublishAfter(gomock.Any(), "process", gomock.Cond(rescheduledMsg), int64(5000)). - Return(nil) }, }, { @@ -818,7 +792,12 @@ func TestProcess(t *testing.T) { payload = processPayload(t, id) } - err := c.Process(context.Background(), delivery(t, ctrl, payload)) + d := delivery(t, ctrl, payload) + if tt.wantHoldMs > 0 { + d.EXPECT().Hold(tt.wantHoldMs) + } + + err := c.Process(context.Background(), d) if tt.wantErr { require.Error(t, err) @@ -830,11 +809,17 @@ func TestProcess(t *testing.T) { } } -func TestRescheduleProcessRequiresPositiveDelay(t *testing.T) { +// TestHoldForBuildSlotRequiresPositiveDelay pins the config guard: a non-positive +// gate wait delay would redeliver immediately and hot-loop the gate check, so it is +// rejected instead of held. +func TestHoldForBuildSlotRequiresPositiveDelay(t *testing.T) { ctrl := gomock.NewController(t) c, _ := newController(t, ctrl) - err := c.rescheduleProcess(context.Background(), acceptedRequest(testID), 1, 0) + d := consumermock.NewMockDelivery(ctrl) + // No Hold expectation: the guard must reject before recording a hold. + + err := c.holdForBuildSlot(d, acceptedRequest(testID), 1, 0) require.Error(t, err) assert.False(t, errs.IsRetryable(err)) diff --git a/stovepipe/entity/queue_config.go b/stovepipe/entity/queue_config.go index 10797ab9..b6681cd8 100644 --- a/stovepipe/entity/queue_config.go +++ b/stovepipe/entity/queue_config.go @@ -24,6 +24,6 @@ type QueueConfig struct { Name string `json:"name" yaml:"name"` // MaxConcurrent is the cap on concurrent in-flight validations for the queue. MaxConcurrent int32 `json:"max_concurrent" yaml:"max_concurrent"` - // GateWaitDelayMs is the PublishAfter delay when the latest head waits for a slot. + // GateWaitDelayMs is the redelivery delay while the latest head waits for a slot. GateWaitDelayMs int64 `json:"gate_wait_delay_ms" yaml:"gate_wait_delay_ms"` } diff --git a/test/e2e/stovepipe/suite_test.go b/test/e2e/stovepipe/suite_test.go index 66ade520..34a0e34a 100644 --- a/test/e2e/stovepipe/suite_test.go +++ b/test/e2e/stovepipe/suite_test.go @@ -160,19 +160,17 @@ func (s *StovepipeE2ESuite) TestIngest_Idempotent() { } // TestIngest_SlowBuild_PollsToCompletion drives a build that is not terminal on its -// first poll, which is the only path that exercises buildsignal's reschedule. +// first poll, which is the only path that exercises buildsignal's poll loop. // // The queue name carries a fake-buildrunner marker: the fake SourceControl resolves a // queue to "git:///HEAD", so the marker rides into the head URI and the fake // BuildRunner reports running for a while before succeeding. Reaching a terminal build // status therefore requires the poll loop to tick more than once. // -// This is the regression test for the loop stalling: buildsignal re-publishes to its -// own topic to schedule the next poll, and the queue dedups on -// (topic, partition_key, id). While the delivery being processed is still un-acked its -// row is present, so a re-poll that reuses the build id as the message id is silently -// discarded and the build is never polled again — the build would sit at `running` -// forever. +// The loop is driven by holding the delivery: each non-terminal poll postpones the +// same BuildSignal message, which redelivers after the poll delay without minting new +// rows or burning retry attempts. A build that stalled the loop would sit at `running` +// forever, so reaching a terminal status proves the held message kept redelivering. func (s *StovepipeE2ESuite) TestIngest_SlowBuild_PollsToCompletion() { const queue = "monorepo/slow?buildrunner-fake=build-slow" @@ -181,7 +179,7 @@ func (s *StovepipeE2ESuite) TestIngest_SlowBuild_PollsToCompletion() { s.assertIngestPersisted(queue, id) - // Getting here at all means the reschedule produced a deliverable message. + // Getting here at all means the held delivery redelivered and re-polled. s.awaitBuildStatus(id, "succeeded") // buildsignal projects the terminal build status onto the request and, in the