Skip to content

Commit 2e13236

Browse files
committed
feat(stovepipe): cool down admissions after failures
Summary: Intent: - Avoid immediately consuming more build resources after a runner-reported failure. Changes: - Persist the first terminal observation time on Build. - Advance the admission deadline while releasing the failed build slot in one queue CAS. - Keep success, cancellation, and DLQ-forced failure outside the cooldown policy. This change builds on the generic logical admission throttle introduced by the parent PR.
1 parent 6514bbe commit 2e13236

16 files changed

Lines changed: 235 additions & 62 deletions

File tree

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ It handles only the trigger: it does not poll for completion, record greenness,
1010

1111
`build` consumes a request id, published by `process` in Phase 1 (see [workflow.md](doc/rfc/stovepipe/workflow.md#workflow)). Both phases drive the same `build` → `buildsignal` machinery against the same `Request` row. The `process`/`analyze` → `build` topic is partitioned by **request id**; see [Partitioning](#partitioning) for the full rationale, including why `build` → `buildsignal` partitions finer (by build id) than SubmitQueue's equivalent topic.
1212

13-
**`build` is not the sole writer of the rows it touches, and its own writes are narrow.** On `Request`, `build` never writes at all — it only reads the `BuildStrategy`/`BaseURI`/`URI` fields `process` set at admit, and it leaves `Request.State` untouched at `processing` throughout (`process`, `buildsignal`, and the DLQ reconciler are `Request.State`'s only writers). On `Build`, `build` is the sole creator — it calls `BuildStore.Create` exactly once, at step 6 — and never mutates the row again; `buildsignal` is the sole writer of `Build.Status`/`Build.Version` afterward (see [buildsignal.md](doc/rfc/stovepipe/steps/buildsignal.md#input-and-re-entrancy)). This division is why `build` never needs a CAS/version write of its own: `Create` is the only storage mutation in its algorithm.
13+
**`build` is not the sole writer of the rows it touches, and its own writes are narrow.** On `Request`, `build` never writes at all — it only reads the `BuildStrategy`/`BaseURI`/`URI` fields `process` set at admit, and it leaves `Request.State` untouched at `processing` throughout (`process`, `buildsignal`, and the DLQ reconciler are `Request.State`'s only writers). On `Build`, `build` is the sole creator — it calls `BuildStore.Create` exactly once, at step 6 — and never mutates the row again; `buildsignal` is the sole writer of `Build.Status`/`Build.TerminalAtMs`/`Build.Version` afterward (see [buildsignal.md](doc/rfc/stovepipe/steps/buildsignal.md#input-and-re-entrancy)). This division is why `build` never needs a CAS/version write of its own: `Create` is the only storage mutation in its algorithm.
1414

1515
`build` is phase-agnostic: it never asks "which phase is this?" It reads whatever scope is already persisted and immutable on the `Request` and acts on it. Phase 2's project-scoped invocation is expected to read project-scoped equivalents of the scope fields off the same `Request`; the exact shape of a project-scoped trigger is left to the `analyze` design, consistent with `workflow.md`'s "project mapping contract" open question — see [Project-scoped `Trigger`: reserved, not yet designed](#project-scoped-trigger-reserved-not-yet-designed) for the resulting gap in the `BuildRunner` contract itself.
1616

@@ -52,7 +52,7 @@ For a delivery carrying request id `R`:
5252
either domain — the shape is deferred until then, not decided here.
5353
- failure -> return raw; classifier decides (transient runner blip retryable, bad URI not).
5454
55-
6. Persist Build{ID: buildID.ID, RequestID: R.ID, Status: accepted, Version: 1}
55+
6. Persist Build{ID: buildID.ID, RequestID: R.ID, Status: accepted, TerminalAtMs: 0, Version: 1}
5656
via BuildStore.Create.
5757
- the row carries no scope; it is recoverable from the Request's immutable fields
5858
(see the entity table).

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

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

1313
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.
1414

15-
`buildsignal` is the sole writer of `Build.Status`/`Build.Version` after `build` creates the row (see [build.md](doc/rfc/stovepipe/steps/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.
15+
`buildsignal` is the sole writer of `Build.Status`/`Build.TerminalAtMs`/`Build.Version` after `build` creates the row (see [build.md](doc/rfc/stovepipe/steps/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

1717
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.
1818

@@ -55,15 +55,20 @@ For a delivery carrying build id `B`:
5555
- Build.Status already terminal, status differs -> terminal is WRITE-ONCE: do not overwrite;
5656
continue with the STORED status as authoritative (see Edge cases).
5757
- otherwise persist via BuildStore.Update(ctx, Build{...Status: status}, oldVersion, newVersion):
58+
- when status is terminal, stamp TerminalAtMs with the observation time in the same update;
5859
- newVersion = Build.Version + 1; assign Build.Version = newVersion only on success.
5960
- ErrVersionMismatch -> return with its declaration-level retryable classification (a concurrent writer moved the row; reload and re-check).
6061
- with the write-once rule, accepted -> running -> {succeeded|failed|cancelled} is monotonic
6162
by mechanism, not by assumption about the backend.
6263
6364
7. If the stored status is terminal, and R does not already carry an outcome:
64-
a. Release the queue's build slot: CAS-decrement Queue.in_flight_count, clamped at zero.
65+
a. Load the queue config only when the runner-reported status is failed. Apply
66+
failure_cooldown_ms only when it is positive; non-positive values disable the policy.
67+
b. Release the queue's build slot in one Queue CAS: decrement in_flight_count, clamped at zero,
68+
and for failed status advance build_admission_not_before_ms to at least
69+
Build.TerminalAtMs + failure_cooldown_ms.
6570
- failure here aborts the step: R must not go terminal while still holding a slot.
66-
b. CAS R from processing to the outcome the stored status projects onto it:
71+
c. CAS R from processing to the outcome the stored status projects onto it:
6772
succeeded -> succeeded, failed -> failed, cancelled -> cancelled. First writer wins.
6873
Then publish R.ID to the record topic, partitioned by request id; ack, return.
6974
No re-publish to buildsignal.
@@ -86,6 +91,8 @@ For a delivery carrying build id `B`:
8691

8792
**Why `buildsignal` releases the slot rather than `record`**: the gate `process` claims is a *build* slot — it exists to bound concurrent builds per Queue — and once the build is terminal the build is over. Releasing here also keeps the invariant *a terminal `Request` has already released its slot*, which is what makes the DLQ reconciler's early-return on terminal requests safe.
8893

94+
**Why failure cooldown is part of the slot-release CAS**: both fields govern the next logical admission, so updating them from the same freshly loaded Queue snapshot prevents a process admission from slipping between capacity release and deadline advancement. A version conflict reloads the row and recomputes the maximum, preserving a later deadline written by the minimum-interval policy or another terminal result. The deadline is based on the persisted `Build.TerminalAtMs`, not redelivery time, so retries cannot slide the cooldown forward. Only an actual runner-reported `failed` status applies this policy; `cancelled` and DLQ-forced request failure do not.
95+
8996
**Why `record` hears only terminal signals**: `record` has no non-terminal work — by its own contract a non-terminal signal would be a pure no-op — and step 7 already branches on terminality to decide whether to keep polling, so gating the publish costs nothing and spares `record` a no-op delivery on every poll tick of every running build. Crash-safety is unaffected: a crash between the terminal `Update` and the publish redelivers the message; step 5 re-polls (the runner reports the same terminal status), step 6 no-ops, step 7 publishes. This is a deliberate divergence from SubmitQueue, whose buildsignal republishes to `speculate` on every tick — sound there because speculate is a state machine that may act on any signal; stovepipe has no such consumer.
9097

9198
**Why step 6 guards on status and makes terminal write-once**: an unchanged status skips the CAS write entirely, so a long build being polled every couple of seconds doesn't churn `Build.Version` on every tick — the version only advances on a real state transition. The write-once rule exists because CAS alone cannot provide it: optimistic locking defends against *concurrent* writers, but a later delivery that polls a flaky backend and sees a different terminal status would CAS cleanly against the current version and overwrite (see Edge cases). A given `Build` has a single poll partition (see [Partitioning](doc/rfc/stovepipe/steps/build.md#partitioning)), so the only writer racing the CAS is a redelivery of the same message (e.g. after a lapsed visibility lease); `ErrVersionMismatch` carries a retryable classification and converges on redelivery.

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,7 @@ Validation is expensive and shares a baseline, so heads arriving while an earlie
6464
| Queue row | `build_admission_not_before_ms` | Durable earliest time for the next logical admission; zero until a time policy advances it. |
6565
| Queue config | `max_concurrent` | Cap on concurrent in-flight validations. **Default 1** (global wiring default for MVP; per-queue override when a Stovepipe `queueconfig` extension lands). |
6666
| Queue config | `minimum_build_admission_interval_ms` | Minimum start-to-start spacing between logical admissions. Positive values enable general throttling; non-positive values disable it. **Default 0**. |
67+
| Queue config | `failure_cooldown_ms` | Delay after a runner-reported failed build before the next logical admission. Positive values enable the cooldown; non-positive values disable it. **Default 0**. |
6768

6869
A slot is held from admit until the build goes terminal (`process → build → buildsignal`), not just while `process` runs. It is released when the Request reaches **any** terminal state and `in_flight_count` is decremented — `buildsignal` recording the build's outcome, success *or* failure, or the DLQ reconciler forcing a terminal `failed` (see [integrity](#in_flight_count-integrity)). A build *failure* frees the slot just like a success; only a Request that never terminates keeps its slot.
6970

‎service/stovepipe/server/main.go‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -455,13 +455,14 @@ func registerPrimaryControllers(
455455
hooks hookext.Hooks,
456456
) (int, error) {
457457
var count int
458+
queueConfigs := queueconfigdefault.NewStore()
458459

459460
processController := process.NewController(
460461
logger,
461462
scope,
462463
store,
463464
materializer,
464-
queueconfigdefault.NewStore(),
465+
queueConfigs,
465466
sourceControl,
466467
registry,
467468
stovepipemq.TopicKeyProcess,
@@ -478,7 +479,7 @@ func registerPrimaryControllers(
478479
}
479480
count++
480481

481-
buildSignalController := buildsignal.NewController(logger, scope, store, materializer, brf, registry, stovepipemq.TopicKeyBuildSignal, "stovepipe-buildsignal")
482+
buildSignalController := buildsignal.NewController(logger, scope, store, materializer, queueConfigs, brf, registry, stovepipemq.TopicKeyBuildSignal, "stovepipe-buildsignal")
482483
if err := c.Register(buildSignalController); err != nil {
483484
return count, fmt.Errorf("failed to register buildsignal controller: %w", err)
484485
}

‎stovepipe/controller/buildsignal/BUILD.bazel‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ go_library(
1515
"//stovepipe/core/requestlog:go_default_library",
1616
"//stovepipe/entity:go_default_library",
1717
"//stovepipe/extension/buildrunner:go_default_library",
18+
"//stovepipe/extension/queueconfig:go_default_library",
1819
"//stovepipe/extension/storage:go_default_library",
1920
"@com_github_uber_go_tally//:go_default_library",
2021
"@org_uber_go_zap//:go_default_library",
@@ -38,6 +39,7 @@ go_test(
3839
"//stovepipe/entity:go_default_library",
3940
"//stovepipe/extension/buildrunner:go_default_library",
4041
"//stovepipe/extension/buildrunner/mock:go_default_library",
42+
"//stovepipe/extension/queueconfig/mock:go_default_library",
4143
"//stovepipe/extension/storage:go_default_library",
4244
"//stovepipe/extension/storage/mock:go_default_library",
4345
"@com_github_stretchr_testify//assert:go_default_library",

0 commit comments

Comments
 (0)