Skip to content

Commit fd1f6ce

Browse files
committed
feat(stovepipe): throttle logical build admissions
Summary: Intent: - Allow queues to limit expensive logical build admissions independently of concurrency. Changes: - Gate process admission on the durable deadline and hold ineligible deliveries. - Advance the deadline atomically while claiming a build slot. - Add the general minimum admission interval policy, metrics, tests, and design documentation. This change builds on the durable admission state introduced by the parent PR.
1 parent 3f8c82a commit fd1f6ce

7 files changed

Lines changed: 183 additions & 23 deletions

File tree

doc/rfc/stovepipe/steps/process.md

Lines changed: 16 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -25,11 +25,11 @@ For a delivery carrying request id `R`:
2525
4. R.State is accepted. Load the Queue row Q.
2626
5. Coalesce: if CompareRequestID(R.Queue, R.ID, Q.latest_request_id) < 0:
2727
- a newer head exists -> mark R superseded, ack, return. (No slot consumed.)
28-
6. R is the latest head. Gate: if Q.in_flight_count >= max_concurrent (from queue config; see below):
29-
- 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).
28+
6. R is the latest head. Gate: if Q.in_flight_count >= max_concurrent or Q.build_admission_not_before_ms is in the future:
29+
- defer (hold the delivery) -> re-check on redelivery until both gates open (admit) or a newer head supersedes it. See [Waiting for admission](#waiting-for-admission).
3030
7. Admit R:
3131
a. Derive build strategy + baseline (see "Build-strategy decision").
32-
b. CAS the Queue row: in_flight_count += 1.
32+
b. CAS the Queue row: in_flight_count += 1 and advance build_admission_not_before_ms by the configured minimum admission interval.
3333
c. CAS the Request: accepted -> processing, persist build_strategy + base_uri.
3434
d. Announce validation start on the hook topic (see "Hooks").
3535
e. Publish R to build.
@@ -60,8 +60,10 @@ Validation is expensive and shares a baseline, so heads arriving while an earlie
6060
| Source | Field | Meaning |
6161
|---|---|---|
6262
| Queue row | `last_green_uri` | Bookmark `record` advances on whole-repo green; empty until first green. |
63-
| Queue row | `in_flight_count` | Requests past `process` and not yet terminal. `process` increments on admit; `record` (or DLQ reconciliation) decrements on terminal. |
63+
| Queue row | `in_flight_count` | Requests past `process` and not yet terminal. `process` increments on admit; `buildsignal` (or DLQ reconciliation) decrements on terminal. |
64+
| Queue row | `build_admission_not_before_ms` | Durable earliest time for the next logical admission; zero until a time policy advances it. |
6465
| 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). |
66+
| Queue config | `minimum_build_admission_interval_ms` | Minimum start-to-start spacing between logical admissions. **Default 0** disables general throttling. |
6567

6668
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.
6769

@@ -115,7 +117,7 @@ The gate is **not** tied to `process` returning; a slot taken at admit is held u
115117

116118
**Rules**
117119

118-
1. **One slot per in-flight validation** (MVP: one per Queue). `process` increments `in_flight_count` on admit; `record` decrements on terminal.
120+
1. **One slot per in-flight validation** (MVP: one per Queue). `process` increments `in_flight_count` on admit; `buildsignal` decrements when the build becomes terminal.
119121
2. **No skip-ahead while in-flight.** The latest head waits for a slot until the running validation completes; it never preempts.
120122
3. **Intermediates are superseded on sight**, gate open or closed — no slot consumed (step 5).
121123
4. **Coalesce-to-latest on gate open.** When a slot frees, the waiting latest head is admitted.
@@ -135,9 +137,9 @@ A, D, F each get a full cycle; B, C, E end `superseded`. No intermediate is vali
135137

136138
**What does not happen**
137139

138-
- `process` returning does **not** free a slot — only `record` (or DLQ reconciliation) does.
140+
- `process` returning does **not** free a slot — only `buildsignal` (or DLQ reconciliation) does.
139141
- A newer head does **not** preempt an in-flight validation.
140-
- Deferred messages are **not** failed or dead-lettered — they wait for the gate (see [Waiting for a slot](#waiting-for-a-slot)).
142+
- Deferred messages are **not** failed or dead-lettered — they wait for the gate (see [Waiting for admission](#waiting-for-admission)).
141143

142144
## Hooks
143145

@@ -170,10 +172,10 @@ The window to handle is "count incremented, state not yet `processing`". Admit d
170172

171173
`in_flight_count` is a cache; the source of truth is **the set of non-terminal Request rows for the Queue**. Two rules keep it from drifting:
172174

173-
1. **Decrement is bound to the terminal transition.** The single CAS that moves a Request non-terminal → terminal (in `record` or the DLQ reconciler) also decrements. Being CAS-guarded, it fires exactly once per Request even under redelivery.
175+
1. **Decrement precedes the terminal transition.** `buildsignal` or the DLQ reconciler decrements before moving a Request non-terminal → terminal, so a terminal request never strands a slot. Redelivery may transiently over-release after a crash between the two entity writes, which is preferred to a permanent capacity leak.
174176
2. **Increment is bound to the admit transition.** `process` increments only on the `accepted → processing` CAS; a redelivery of an already-`processing` Request takes step 3 and does not increment again.
175177

176-
On a crash between admit and `record`, the Request stays non-terminal; visibility-timeout redelivery drives it forward, and the fail-closed DLQ path eventually forces it terminal, decrementing as it does. The count can drift high only transiently and self-heals as stuck Requests terminate. A reconciler that recomputes the count from non-terminal rows can be added later if drift proves real, but isn't required for MVP.
178+
On a crash between admit and the terminal outcome, the Request stays non-terminal; visibility-timeout redelivery drives it forward, and the fail-closed DLQ path eventually forces it terminal, decrementing as it does. The count can drift high only transiently and self-heals as stuck Requests terminate. A reconciler that recomputes the count from non-terminal rows can be added later if drift proves real, but isn't required for MVP.
177179

178180
## Edge cases
179181

@@ -192,7 +194,8 @@ Runtime coordination only — fields the pipeline writes under CAS:
192194
|---|---|---|
193195
| `name` | Stable logical id (`monorepo/main`); the string ingest accepts | ingest (create) |
194196
| `last_green_uri` | Bookmark; empty until first green | record |
195-
| `in_flight_count` | Active Phase 1 validations | process (+1), record/DLQ (−1) |
197+
| `in_flight_count` | Active Phase 1 validations | process (+1), buildsignal/DLQ (−1) |
198+
| `build_admission_not_before_ms` | Earliest Unix-millisecond time for another logical admission | process and terminal-result policies |
196199
| `latest_request_id` | Request id of the newest head ingest accepted | ingest |
197200
| `version` | Optimistic-locking version | all writers |
198201

@@ -227,14 +230,14 @@ New key/value-shaped operations (single-key reads/writes, no server-side filteri
227230

228231
No "list requests by queue/state" query is introduced; coalescing uses the single-row `latest_request_id` pointer instead, keeping the contract satisfiable by a plain KV backend.
229232

230-
## Waiting for a slot
233+
## Waiting for admission
231234

232-
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`.
235+
When either 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 at most `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`.
233236

234237
Every wake-up re-runs the same **coalesce-then-gate** checks (steps 5 → 6):
235238

236239
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.
237-
2. **Slot free?** If `in_flight_count < max_concurrent` (from config) and `R` is still latest → admit (step 7).
240+
2. **Capacity and time eligible?** If `in_flight_count < max_concurrent`, `build_admission_not_before_ms <= now`, and `R` is still latest → admit (step 7).
238241

239242
Nothing is admitted to `build` until the gate opens.
240243

stovepipe/controller/process/process.go

Lines changed: 44 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import (
2222
"context"
2323
"errors"
2424
"fmt"
25+
"time"
2526

2627
"github.com/uber-go/tally"
2728
basehook "github.com/uber/submitqueue/api/base/hook"
@@ -55,6 +56,7 @@ type Controller struct {
5556
registry consumer.TopicRegistry
5657
topicKey consumer.TopicKey
5758
consumerGroup string
59+
now func() time.Time
5860
}
5961

6062
// Verify Controller implements consumer.Controller interface at compile time.
@@ -85,6 +87,7 @@ func NewController(
8587
registry: registry,
8688
topicKey: topicKey,
8789
consumerGroup: consumerGroup,
90+
now: time.Now,
8891
}
8992
}
9093

@@ -230,6 +233,11 @@ func (c *Controller) coalesce(ctx context.Context, store storage.Storage, reques
230233
// re-runs coalesce-then-gate, so a slot is never spent on a now-stale head; a closed gate
231234
// defers by holding the delivery (redeliver after the gate wait delay) rather than failing.
232235
func (c *Controller) admitLatestHead(ctx context.Context, store storage.Storage, delivery consumer.Delivery, request entity.Request, queueRow entity.Queue, cfg entity.QueueConfig) error {
236+
if cfg.MinimumBuildAdmissionIntervalMs < 0 {
237+
metrics.NamedCounter(c.metricsScope, _opName, "config_errors", 1, metrics.TagsFromContext(ctx)...)
238+
return fmt.Errorf("minimum build admission interval must not be negative for queue %s, got %dms", request.Queue, cfg.MinimumBuildAdmissionIntervalMs)
239+
}
240+
233241
var sc sourcecontrol.SourceControl
234242
var strategy entity.BuildStrategy
235243
var baseURI string
@@ -239,6 +247,10 @@ func (c *Controller) admitLatestHead(ctx context.Context, store storage.Storage,
239247
if queueRow.InFlightCount >= cfg.MaxConcurrent {
240248
return c.holdForBuildSlot(ctx, delivery, request, queueRow.InFlightCount, cfg.GateWaitDelayMs)
241249
}
250+
nowMs := c.now().UnixMilli()
251+
if queueRow.BuildAdmissionNotBeforeMs > nowMs {
252+
return c.holdForBuildThrottle(ctx, delivery, request, queueRow.BuildAdmissionNotBeforeMs, nowMs, cfg.GateWaitDelayMs)
253+
}
242254

243255
if queueRow.LastGreenURI != "" && sc == nil {
244256
sc, err = c.sourceControl.For(sourcecontrol.Config{QueueName: request.Queue})
@@ -255,7 +267,7 @@ func (c *Controller) admitLatestHead(ctx context.Context, store storage.Storage,
255267
return err
256268
}
257269

258-
err = c.claimBuildSlot(ctx, store, &queueRow)
270+
err = c.claimBuildSlot(ctx, store, &queueRow, nowMs, cfg.MinimumBuildAdmissionIntervalMs)
259271
if err == nil {
260272
break
261273
}
@@ -349,13 +361,19 @@ func (c *Controller) deriveBuildStrategy(ctx context.Context, sc sourcecontrol.S
349361
return entity.BuildStrategyFull, "", nil
350362
}
351363

352-
// claimBuildSlot CAS-increments queue.in_flight_count by one. On version mismatch it
353-
// reloads queueRow and returns ErrVersionMismatch so the caller can retry.
354-
func (c *Controller) claimBuildSlot(ctx context.Context, store storage.Storage, queueRow *entity.Queue) error {
364+
// claimBuildSlot atomically claims capacity and reserves the next logical admission time.
365+
// On version mismatch it reloads queueRow and returns ErrVersionMismatch so the caller can retry.
366+
func (c *Controller) claimBuildSlot(ctx context.Context, store storage.Storage, queueRow *entity.Queue, admittedAtMs, minimumIntervalMs int64) error {
355367
queueStore := store.GetQueueStore()
356368

357369
updated := *queueRow
358370
updated.InFlightCount = queueRow.InFlightCount + 1
371+
if minimumIntervalMs > 0 {
372+
notBeforeMs := admittedAtMs + minimumIntervalMs
373+
if notBeforeMs > updated.BuildAdmissionNotBeforeMs {
374+
updated.BuildAdmissionNotBeforeMs = notBeforeMs
375+
}
376+
}
359377
newVersion := queueRow.Version + 1
360378
if err := queueStore.Update(ctx, updated, queueRow.Version, newVersion); err != nil {
361379
if errors.Is(err, storage.ErrVersionMismatch) {
@@ -494,6 +512,28 @@ func (c *Controller) holdForBuildSlot(ctx context.Context, delivery consumer.Del
494512
return nil
495513
}
496514

515+
func (c *Controller) holdForBuildThrottle(ctx context.Context, delivery consumer.Delivery, request entity.Request, notBeforeMs, nowMs, gateWaitDelayMs int64) error {
516+
if gateWaitDelayMs <= 0 {
517+
metrics.NamedCounter(c.metricsScope, _opName, "config_errors", 1, metrics.TagsFromContext(ctx)...)
518+
return fmt.Errorf("requires a positive gate wait delay for queue %s, got %dms", request.Queue, gateWaitDelayMs)
519+
}
520+
521+
delayMs := notBeforeMs - nowMs
522+
if delayMs > gateWaitDelayMs {
523+
delayMs = gateWaitDelayMs
524+
}
525+
delivery.Hold(delayMs)
526+
metrics.NamedCounter(c.metricsScope, _opName, "admission_throttled", 1, metrics.TagsFromContext(ctx)...)
527+
c.logger.Infow("holding latest head until build admission is eligible",
528+
"request_id", request.ID,
529+
"queue", request.Queue,
530+
"uri", request.URI,
531+
"not_before_ms", notBeforeMs,
532+
"delay_ms", delayMs,
533+
)
534+
return nil
535+
}
536+
497537
// loadRequest returns the request for id.
498538
func (c *Controller) loadRequest(ctx context.Context, store storage.Storage, id string) (entity.Request, error) {
499539
return loader.ByID(ctx, id, store.GetRequestStore().Get, "request")

stovepipe/controller/process/process_test.go

Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import (
1818
"context"
1919
"errors"
2020
"testing"
21+
"time"
2122

2223
"github.com/stretchr/testify/assert"
2324
"github.com/stretchr/testify/require"
@@ -48,6 +49,7 @@ const (
4849
testID = "request/monorepo/main/7"
4950
testOlderID = "request/monorepo/main/3"
5051
testURI = "git://repo/monorepo/main/abc123"
52+
testNowMs = int64(2_000_000)
5153
)
5254

5355
func queueContext(queueName string) context.Context {
@@ -71,6 +73,25 @@ type staticStorageFactory struct{ store storage.Storage }
7173
// For returns the fixed store aggregate for any queue.
7274
func (f staticStorageFactory) For(storage.Config) (storage.Storage, error) { return f.store, nil }
7375

76+
type staticQueueConfigStore struct{ config entity.QueueConfig }
77+
78+
func (s staticQueueConfigStore) Get(context.Context, string) (entity.QueueConfig, error) {
79+
return s.config, nil
80+
}
81+
82+
func (s staticQueueConfigStore) List(context.Context) ([]entity.QueueConfig, error) {
83+
return []entity.QueueConfig{s.config}, nil
84+
}
85+
86+
func queueConfig(minimumBuildAdmissionIntervalMs int64) entity.QueueConfig {
87+
return entity.QueueConfig{
88+
Name: testQueue,
89+
MaxConcurrent: 1,
90+
GateWaitDelayMs: 5000,
91+
MinimumBuildAdmissionIntervalMs: minimumBuildAdmissionIntervalMs,
92+
}
93+
}
94+
7495
func newController(t *testing.T, ctrl *gomock.Controller) (*Controller, processMocks) {
7596
t.Helper()
7697
return newControllerWithScope(t, ctrl, tally.NewTestScope("test", nil))
@@ -111,6 +132,7 @@ func newControllerWithScope(t *testing.T, ctrl *gomock.Controller, scope tally.S
111132
stovepipemq.TopicKeyProcess,
112133
"stovepipe-process",
113134
)
135+
c.now = func() time.Time { return time.UnixMilli(testNowMs) }
114136
return c, m
115137
}
116138

@@ -508,6 +530,7 @@ func TestProcess(t *testing.T) {
508530
wantHoldMs int64
509531
wantErr bool
510532
wantRetry bool
533+
config entity.QueueConfig
511534
}{
512535
{
513536
name: "superseded redelivery repairs its state log",
@@ -730,6 +753,67 @@ func TestProcess(t *testing.T) {
730753
}, nil)
731754
},
732755
},
756+
{
757+
name: "latest accepted head holds while admission deadline is active",
758+
wantHoldMs: 5000,
759+
config: queueConfig(3_600_000),
760+
setup: func(m processMocks) {
761+
m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(acceptedRequest(testID), nil)
762+
m.queueStore.EXPECT().Get(gomock.Any(), testQueue).Return(entity.Queue{
763+
Name: testQueue,
764+
LatestRequestID: testID,
765+
BuildAdmissionNotBeforeMs: testNowMs + 7500,
766+
Version: 1,
767+
}, nil)
768+
},
769+
},
770+
{
771+
name: "admission deadline uses remaining duration below gate wait",
772+
wantHoldMs: 2500,
773+
config: queueConfig(3_600_000),
774+
setup: func(m processMocks) {
775+
m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(acceptedRequest(testID), nil)
776+
m.queueStore.EXPECT().Get(gomock.Any(), testQueue).Return(entity.Queue{
777+
Name: testQueue,
778+
LatestRequestID: testID,
779+
BuildAdmissionNotBeforeMs: testNowMs + 2500,
780+
Version: 1,
781+
}, nil)
782+
},
783+
},
784+
{
785+
name: "admission claim reserves the next configured interval",
786+
config: queueConfig(3_600_000),
787+
setup: func(m processMocks) {
788+
m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(acceptedRequest(testID), nil)
789+
m.queueStore.EXPECT().Get(gomock.Any(), testQueue).Return(entity.Queue{
790+
Name: testQueue, LatestRequestID: testID, Version: 1,
791+
}, nil)
792+
m.queueStore.EXPECT().Update(gomock.Any(), entity.Queue{
793+
Name: testQueue,
794+
LatestRequestID: testID,
795+
InFlightCount: 1,
796+
BuildAdmissionNotBeforeMs: testNowMs + 3_600_000,
797+
Version: 1,
798+
}, int32(1), int32(2)).Return(nil)
799+
updatedReq := acceptedRequest(testID)
800+
updatedReq.State = entity.RequestStateProcessing
801+
updatedReq.BuildStrategy = entity.BuildStrategyFull
802+
m.reqStore.EXPECT().Update(gomock.Any(), updatedReq, int32(1), int32(2)).Return(nil)
803+
expectStartAnnounceAndBuildPublish(t, m, testID)
804+
},
805+
},
806+
{
807+
name: "negative admission interval is rejected before claiming a slot",
808+
wantErr: true,
809+
config: queueConfig(-1),
810+
setup: func(m processMocks) {
811+
m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(acceptedRequest(testID), nil)
812+
m.queueStore.EXPECT().Get(gomock.Any(), testQueue).Return(entity.Queue{
813+
Name: testQueue, LatestRequestID: testID, Version: 1,
814+
}, nil)
815+
},
816+
},
733817
{
734818
name: "gate closed after slot claim race holds",
735819
wantHoldMs: 5000,
@@ -754,6 +838,30 @@ func TestProcess(t *testing.T) {
754838
}, nil)
755839
},
756840
},
841+
{
842+
name: "claim conflict reload observes a concurrent admission deadline",
843+
wantHoldMs: 5000,
844+
config: queueConfig(3_600_000),
845+
setup: func(m processMocks) {
846+
m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(acceptedRequest(testID), nil)
847+
m.queueStore.EXPECT().Get(gomock.Any(), testQueue).Return(entity.Queue{
848+
Name: testQueue, LatestRequestID: testID, Version: 1,
849+
}, nil)
850+
m.queueStore.EXPECT().Update(gomock.Any(), entity.Queue{
851+
Name: testQueue,
852+
LatestRequestID: testID,
853+
InFlightCount: 1,
854+
BuildAdmissionNotBeforeMs: testNowMs + 3_600_000,
855+
Version: 1,
856+
}, int32(1), int32(2)).Return(storage.ErrVersionMismatch)
857+
m.queueStore.EXPECT().Get(gomock.Any(), testQueue).Return(entity.Queue{
858+
Name: testQueue,
859+
LatestRequestID: testID,
860+
BuildAdmissionNotBeforeMs: testNowMs + 10_000,
861+
Version: 2,
862+
}, nil)
863+
},
864+
},
757865
{
758866
name: "claim slot retries on queue version mismatch then admits",
759867
setup: func(m processMocks) {
@@ -995,6 +1103,9 @@ func TestProcess(t *testing.T) {
9951103
t.Run(tt.name, func(t *testing.T) {
9961104
ctrl := gomock.NewController(t)
9971105
c, m := newController(t, ctrl)
1106+
if tt.config.Name != "" {
1107+
c.queueConfigs = staticQueueConfigStore{config: tt.config}
1108+
}
9981109
if tt.setup != nil {
9991110
tt.setup(m)
10001111
}

stovepipe/entity/queue_config.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,4 +26,7 @@ type QueueConfig struct {
2626
MaxConcurrent int32 `json:"max_concurrent" yaml:"max_concurrent"`
2727
// GateWaitDelayMs is the redelivery delay while the latest head waits for a slot.
2828
GateWaitDelayMs int64 `json:"gate_wait_delay_ms" yaml:"gate_wait_delay_ms"`
29+
// MinimumBuildAdmissionIntervalMs is the minimum start-to-start spacing between logical
30+
// build admissions for this queue. Zero disables time-based throttling.
31+
MinimumBuildAdmissionIntervalMs int64 `json:"minimum_build_admission_interval_ms" yaml:"minimum_build_admission_interval_ms"`
2932
}

0 commit comments

Comments
 (0)