Skip to content

Commit b342ced

Browse files
roychyingJamyDev
authored andcommitted
feat(stovepipe): add and wire the buildsignal stage controller (#409)
## What? Adds `stovepipe/controller/buildsignal` plus the new `Record` queue message/proto and `TopicKeyRecord`. `Process` consumes a `BuildSignal` (build id), loads `Build`+`Request`, no-ops if the request is terminal, polls `buildRunner.Status`, persists a real status transition via CAS (`BuildStore.Update`) with terminal status **write-once** (a later poll of a flaky backend can never overwrite an already-committed terminal status), and either reschedules itself via `PublishAfter` (5s while `accepted`, 2s while `running`) or, once terminal, publishes to `record`. Wires the controller into `main.go` and registers the `buildsignal` (subscribed) and `record` (publish-only, no consumer yet) topics. ## Test Plan `make build`, `make test` - all clean. ## Issue <!-- Link the issue here. - Use 'Closes #123' if this is the final fix. - Use 'Part of #123' or just '#123' if the feature is still in progress. -->
1 parent c8c8ad5 commit b342ced

21 files changed

Lines changed: 1008 additions & 26 deletions

File tree

doc/rfc/stovepipe/steps/buildsignal.md

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -20,14 +20,12 @@ For a delivery carrying build id `B`:
2020

2121
```
2222
1. Load Build B from the build store.
23-
- ErrNotFound -> retryable (build's Create not visible yet; redelivery converges).
23+
- ErrNotFound -> return raw; non-retryable (storage is read-after-write consistent; see [storage README](stovepipe/extension/storage/README.md)).
2424
- other store error -> return raw; classifier decides.
2525
2626
2. Load Request R = store.Get(Build.RequestID) — needed for R.Queue to resolve the build-runner.
27-
- ErrNotFound -> retryable, like step 1: the Build's existence proves the Request write is older,
28-
so a miss here is almost certainly a lagging read; redelivery converges. A genuinely orphaned
29-
Build (integrity fault) still dead-letters at MaxAttempts — the same terminal outcome, without
30-
rejecting straight to DLQ on a stale read.
27+
- ErrNotFound -> return raw; non-retryable, same as step 1 — the Build's existence proves the
28+
Request write is already committed, so a miss here is a storage defect, not a lagging read.
3129
3230
3. If R.State is terminal (superseded / recorded-green / recorded-not-green): ack and return.
3331
- the Request is done (record already ran, or the head was superseded); stop polling.
@@ -102,19 +100,19 @@ Per `platform/errs`'s non-retryable-by-default rule (see [platform/errs/README.m
102100

103101
| Failure | Disposition | Why |
104102
|---|---|---|
105-
| `Build` not found | retryable (`errs.NewRetryableError`) | `build`'s `Create` not visible yet; redelivery converges. |
106-
| `Request` not found | retryable (`errs.NewRetryableError`) | The Build's existence proves the Request write is older, so a miss is a stale read; a genuine orphan still dead-letters at `MaxAttempts`. |
107103
| `Status` call | raw error; classifier decides | Deliberately left open rather than fixed either way — runner timeout/connection is transient, "runner not deployed for this queue" is not, and only a backend classifier can tell them apart. |
108104
| `Update` CAS conflict (`ErrVersionMismatch`) | retryable | A concurrent (redelivered) writer moved the row; reload and re-check converges. |
109105
| `PublishAfter` re-poll | retryable | The poll heartbeat; it runs only after status/persist/record all succeeded, so a transient enqueue blip is worth replaying to `MaxAttempts` before dead-lettering. |
110106

107+
`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.
108+
111109
Everything else — factory lookup, an `Update` store error other than a CAS conflict, and the publish to `record` — 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.
112110

113111
## Idempotency
114112

115113
Every branch is safe under at-least-once redelivery:
116114

117-
- **Build not found** — retryable; converges as the row becomes visible.
115+
- **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.
118116
- **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 to `record` (terminal, idempotent). No corruption.
119117
- **Terminal already published** — a redelivery reloads, re-polls, no-ops at step 6, republishes the same terminal signal to `record` (idempotent), and acks. Harmless.
120118
- **`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.

service/stovepipe/server/BUILD.bazel

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ go_library(
1616
"//service/stovepipe/server/mapper:go_default_library",
1717
"//stovepipe/controller:go_default_library",
1818
"//stovepipe/controller/build:go_default_library",
19+
"//stovepipe/controller/buildsignal:go_default_library",
1920
"//stovepipe/controller/dlq:go_default_library",
2021
"//stovepipe/controller/process:go_default_library",
2122
"//stovepipe/core/messagequeue:go_default_library",

service/stovepipe/server/main.go

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ import (
3838
"github.com/uber/submitqueue/service/stovepipe/server/mapper"
3939
"github.com/uber/submitqueue/stovepipe/controller"
4040
"github.com/uber/submitqueue/stovepipe/controller/build"
41+
"github.com/uber/submitqueue/stovepipe/controller/buildsignal"
4142
"github.com/uber/submitqueue/stovepipe/controller/dlq"
4243
"github.com/uber/submitqueue/stovepipe/controller/process"
4344
stovepipemq "github.com/uber/submitqueue/stovepipe/core/messagequeue"
@@ -382,6 +383,12 @@ func registerPrimaryControllers(
382383
}
383384
count++
384385

386+
buildSignalController := buildsignal.NewController(logger, scope, store, brf, registry, stovepipemq.TopicKeyBuildSignal, "stovepipe-buildsignal")
387+
if err := c.Register(buildSignalController); err != nil {
388+
return count, fmt.Errorf("failed to register buildsignal controller: %w", err)
389+
}
390+
count++
391+
385392
return count, nil
386393
}
387394

@@ -407,8 +414,10 @@ func registerDLQControllers(
407414

408415
// newTopicRegistry builds the TopicRegistry for Stovepipe's internal pipeline queues. ingest
409416
// publishes to the process topic and the process consumer subscribes to it; process publishes
410-
// to the build topic and the build consumer subscribes to it. The buildsignal topic is added
411-
// once the buildsignal controller lands to consume it.
417+
// to the build topic and the build consumer subscribes to it; build publishes to the buildsignal
418+
// topic and the buildsignal consumer subscribes to it, and also republishes to itself while
419+
// polling. buildsignal publishes to the record topic once a build reaches a terminal status; it
420+
// has no Subscription yet since no consumer for it exists until the record stage lands.
412421
func newTopicRegistry(q extqueue.Queue, subscriberName string) (consumer.TopicRegistry, error) {
413422
return consumer.NewTopicRegistry([]consumer.TopicConfig{
414423
{
@@ -427,6 +436,19 @@ func newTopicRegistry(q extqueue.Queue, subscriberName string) (consumer.TopicRe
427436
subscriberName, "stovepipe-build",
428437
),
429438
},
439+
{
440+
Key: stovepipemq.TopicKeyBuildSignal,
441+
Name: "buildsignal",
442+
Queue: q,
443+
Subscription: extqueue.DefaultSubscriptionConfig(
444+
subscriberName, "stovepipe-buildsignal",
445+
),
446+
},
447+
{
448+
Key: stovepipemq.TopicKeyRecord,
449+
Name: "record",
450+
Queue: q,
451+
},
430452
{
431453
Key: dlq.TopicKey(stovepipemq.TopicKeyProcess),
432454
Name: "process_dlq",

stovepipe/controller/build/BUILD.bazel

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ go_library(
1010
"//platform/consumer:go_default_library",
1111
"//platform/errs:go_default_library",
1212
"//platform/metrics:go_default_library",
13+
"//stovepipe/core/loader:go_default_library",
1314
"//stovepipe/core/messagequeue:go_default_library",
1415
"//stovepipe/entity:go_default_library",
1516
"//stovepipe/extension/buildrunner:go_default_library",

stovepipe/controller/build/build.go

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ import (
2828
"github.com/uber/submitqueue/platform/consumer"
2929
"github.com/uber/submitqueue/platform/errs"
3030
"github.com/uber/submitqueue/platform/metrics"
31+
"github.com/uber/submitqueue/stovepipe/core/loader"
3132
stovepipemq "github.com/uber/submitqueue/stovepipe/core/messagequeue"
3233
"github.com/uber/submitqueue/stovepipe/entity"
3334
"github.com/uber/submitqueue/stovepipe/extension/buildrunner"
@@ -149,11 +150,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) (r
149150

150151
// loadRequest returns the request for id.
151152
func (c *Controller) loadRequest(ctx context.Context, id string) (entity.Request, error) {
152-
got, err := c.store.GetRequestStore().Get(ctx, id)
153-
if err != nil {
154-
return entity.Request{}, fmt.Errorf("BuildController failed to load request %s: %w", id, err)
155-
}
156-
return got, nil
153+
return loader.ByID(ctx, id, c.store.GetRequestStore().Get, "BuildController", "request")
157154
}
158155

159156
// publishBuildSignal publishes buildID to the buildsignal stage, partitioned by
Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
load("@rules_go//go:def.bzl", "go_library", "go_test")
2+
3+
go_library(
4+
name = "go_default_library",
5+
srcs = ["buildsignal.go"],
6+
importpath = "github.com/uber/submitqueue/stovepipe/controller/buildsignal",
7+
visibility = ["//visibility:public"],
8+
deps = [
9+
"//platform/base/messagequeue:go_default_library",
10+
"//platform/consumer:go_default_library",
11+
"//platform/errs:go_default_library",
12+
"//platform/metrics:go_default_library",
13+
"//stovepipe/core/loader:go_default_library",
14+
"//stovepipe/core/messagequeue:go_default_library",
15+
"//stovepipe/entity:go_default_library",
16+
"//stovepipe/extension/buildrunner:go_default_library",
17+
"//stovepipe/extension/storage:go_default_library",
18+
"@com_github_uber_go_tally//:go_default_library",
19+
"@org_uber_go_zap//:go_default_library",
20+
],
21+
)
22+
23+
go_test(
24+
name = "go_default_test",
25+
srcs = ["buildsignal_test.go"],
26+
embed = [":go_default_library"],
27+
deps = [
28+
"//platform/base/messagequeue:go_default_library",
29+
"//platform/consumer:go_default_library",
30+
"//platform/errs:go_default_library",
31+
"//platform/extension/messagequeue/mock:go_default_library",
32+
"//stovepipe/core/messagequeue:go_default_library",
33+
"//stovepipe/entity:go_default_library",
34+
"//stovepipe/extension/buildrunner:go_default_library",
35+
"//stovepipe/extension/buildrunner/mock:go_default_library",
36+
"//stovepipe/extension/storage:go_default_library",
37+
"//stovepipe/extension/storage/mock:go_default_library",
38+
"@com_github_stretchr_testify//assert:go_default_library",
39+
"@com_github_stretchr_testify//require:go_default_library",
40+
"@com_github_uber_go_tally//:go_default_library",
41+
"@org_uber_go_mock//gomock:go_default_library",
42+
"@org_uber_go_zap//:go_default_library",
43+
],
44+
)

0 commit comments

Comments
 (0)