Skip to content

Commit a60a7b4

Browse files
authored
feat(stovepipe): reconcile dead-lettered build signals and free the slot (#565)
## What? Add buildsignal dlq controller implementation ## Why? A buildsignal message that dead-letters ends the only poll chain watching a build that is still running, and its request keeps holding one of the queue's in_flight_count slots. With no reconciler on that topic the count stays high for good, so the queue loses a slot per incident until it can no longer admit work. Add the buildsignal DLQ controller, which maps the dead-lettered build back to its request, marks the request failed, and releases the slot, and register it plus its topic in the reference server. Also document in the RFC that the stage's "classifier decides" disposition for Status failures only works if the BuildRunner backend classifies its own transport and HTTP errors. Unclassified, they take the non-retryable default, which is what sends a poll message to the DLQ on the first proxy blip. ## Test Plan - Integrate in go-code's stovepipe service - Monitor for cases of buildsignal remaining stuck on a poll error
1 parent 51d4d6e commit a60a7b4

7 files changed

Lines changed: 398 additions & 4 deletions

File tree

doc/rfc/stovepipe/steps/buildsignal.md

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -122,13 +122,24 @@ Per `platform/errs`'s non-retryable-by-default rule (see [platform/errs/README.m
122122

123123
| Failure | Disposition | Why |
124124
|---|---|---|
125-
| `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. |
125+
| `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. **This means the `BuildRunner` backend has to classify**: an unclassified transport or HTTP error gets the non-retryable default, so one proxy blip ends the poll chain (see below). |
126126
| `Update` CAS conflict (`ErrVersionMismatch`) | declaration-level retryable | A concurrent (redelivered) writer moved the row; reload and re-check converges. |
127127

128128
`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.
129129

130130
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.
131131

132+
### What it costs when a backend does not classify `Status` errors
133+
134+
Leaving `Status` to the classifier only works if the backend classifies. A `BuildRunner` whose transport returns plain `fmt.Errorf` values gets the non-retryable default, and here that default is expensive: dead-lettering ends the *only* poll chain for a build that is still running, and the request keeps holding one of the queue's `in_flight_count` build slots until reconciliation gives it back. A single `502` from a proxy in front of the build API then looks exactly like "this build can never be polled".
135+
136+
Two things keep a blip from stalling a queue, and a backend needs both:
137+
138+
- **The backend classifies its own failures.** Transport errors and 5xx/429/408 responses are `errs.NewRetryableDependencyError`. A 4xx about the request itself — unknown build, forbidden — is `errs.NewDependencyError`. Only the layer that sees the status code can tell these apart, which is why the table above leaves the call to it.
139+
- **The retry budget is worth something.** Retryable means nack, and a nacked message comes back on the next poll, so `Retry.MaxAttempts` counts attempts rather than time — the default three are spent in a few hundred milliseconds. Raising `MaxAttempts` on this subscription buys a little more, but each attempt is another request at a dependency that is already failing, so it does not stretch to cover a proxy restart. Until nacks are spaced by the configured retry backoff, it is the reconciler below rather than the retry budget that keeps a longer outage from costing the queue a slot.
140+
141+
When the budget does run out the message dead-letters, and the buildsignal DLQ reconciler (`stovepipe/controller/dlq/buildsignal.go`) is what makes that recoverable: it maps the build back to its request, releases the slot, and marks the request `failed`. A deployment that registers the primary consumers but not that reconciler has no fail-closed path for this stage, and loses a slot for good every time this happens.
142+
132143
## Idempotency
133144

134145
Every branch is safe under at-least-once redelivery:

service/stovepipe/server/main.go

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -450,6 +450,12 @@ func registerDLQControllers(
450450
}
451451
count++
452452

453+
buildSignalDLQController := dlq.NewBuildSignalController(logger, scope, store, dlq.TopicKey(stovepipemq.TopicKeyBuildSignal), "stovepipe-buildsignal-dlq")
454+
if err := c.Register(buildSignalDLQController); err != nil {
455+
return count, fmt.Errorf("failed to register buildsignal dlq controller: %w", err)
456+
}
457+
count++
458+
453459
return count, nil
454460
}
455461

@@ -499,6 +505,12 @@ func newTopicRegistry(q extqueue.Queue, subscriberName string) (consumer.TopicRe
499505
Queue: q,
500506
Subscription: extqueue.DLQSubscriptionConfig(subscriberName, "stovepipe-process-dlq"),
501507
},
508+
{
509+
Key: dlq.TopicKey(stovepipemq.TopicKeyBuildSignal),
510+
Name: "buildsignal_dlq",
511+
Queue: q,
512+
Subscription: extqueue.DLQSubscriptionConfig(subscriberName, "stovepipe-buildsignal-dlq"),
513+
},
502514
})
503515
}
504516

stovepipe/controller/dlq/BUILD.bazel

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ load("@rules_go//go:def.bzl", "go_library", "go_test")
33
go_library(
44
name = "go_default_library",
55
srcs = [
6+
"buildsignal.go",
67
"dlq.go",
78
"request.go",
89
],
@@ -21,7 +22,10 @@ go_library(
2122

2223
go_test(
2324
name = "go_default_test",
24-
srcs = ["dlq_test.go"],
25+
srcs = [
26+
"buildsignal_test.go",
27+
"dlq_test.go",
28+
],
2529
embed = [":go_default_library"],
2630
deps = [
2731
"//platform/base/messagequeue:go_default_library",
Lines changed: 167 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,167 @@
1+
// Copyright (c) 2025 Uber Technologies, Inc.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package dlq
16+
17+
import (
18+
"context"
19+
"errors"
20+
"fmt"
21+
22+
"github.com/uber-go/tally"
23+
"github.com/uber/submitqueue/platform/consumer"
24+
"github.com/uber/submitqueue/platform/metrics"
25+
stovepipemq "github.com/uber/submitqueue/stovepipe/core/messagequeue"
26+
"github.com/uber/submitqueue/stovepipe/extension/storage"
27+
"go.uber.org/zap"
28+
)
29+
30+
// _buildSignalOpName is the metric operation name shared by every emit in this file.
31+
const _buildSignalOpName = "buildsignal_dlq"
32+
33+
// BuildSignalController is the DLQ reconciler for the buildsignal stage. The
34+
// payload names a build, not a request, so it takes one more step than the
35+
// process reconciler: read the build to get its RequestID, then fail that
36+
// request via failRequest.
37+
//
38+
// This DLQ is the one that matters most. A request only reaches buildsignal
39+
// after process admitted it, so it holds one of the queue's in_flight_count
40+
// build slots, and buildsignal's terminal path is the only thing that gives that
41+
// slot back. Once a poll message dead-letters — a Status call that stayed broken
42+
// through every retry, an unknown build id, a storage write that kept failing —
43+
// nothing else in the pipeline will look at that build again. Without this
44+
// reconciler the request stays processing for good and the slot is never
45+
// returned, so the queue loses one slot per incident until it has none left and
46+
// stops admitting work.
47+
//
48+
// The Build row keeps whatever non-terminal status the runner last reported.
49+
// There is nothing useful to fix: record decides greenness from Request.State,
50+
// not Build.Status, and writing a terminal status here would claim we saw an
51+
// outcome we never saw.
52+
type BuildSignalController struct {
53+
logger *zap.SugaredLogger
54+
metricsScope tally.Scope
55+
stores storage.Factory
56+
topicKey consumer.TopicKey
57+
consumerGroup string
58+
}
59+
60+
// Verify BuildSignalController implements consumer.Controller at compile time.
61+
var _ consumer.Controller = (*BuildSignalController)(nil)
62+
63+
// NewBuildSignalController creates a DLQ controller for the buildsignal stage's
64+
// dead-letter topic. topicKey is typically
65+
// dlq.TopicKey(stovepipemq.TopicKeyBuildSignal).
66+
func NewBuildSignalController(
67+
logger *zap.SugaredLogger,
68+
scope tally.Scope,
69+
stores storage.Factory,
70+
topicKey consumer.TopicKey,
71+
consumerGroup string,
72+
) *BuildSignalController {
73+
return &BuildSignalController{
74+
logger: logger.Named("buildsignal_dlq_controller"),
75+
metricsScope: scope.SubScope("buildsignal_dlq_controller"),
76+
stores: stores,
77+
topicKey: topicKey,
78+
consumerGroup: consumerGroup,
79+
}
80+
}
81+
82+
// Process reconciles a single DLQ delivery for the buildsignal topic. Returns nil
83+
// to ack (success) or an error to nack (retry) — pair this controller only with a
84+
// consumer wired with errs.AlwaysRetryableProcessor so a transient reconcile
85+
// failure retries instead of dead-lettering the DLQ message itself.
86+
func (c *BuildSignalController) Process(ctx context.Context, delivery consumer.Delivery) error {
87+
msg := delivery.Message()
88+
89+
sig := &stovepipemq.BuildSignal{}
90+
if err := stovepipemq.Unmarshal(msg.Payload, sig); err != nil {
91+
metrics.NamedCounter(c.metricsScope, _buildSignalOpName, "deserialize_errors", 1)
92+
// Retried rather than acked, for the same deployment-skew reason the
93+
// process reconciler gives: a newer producer's payload decodes fine once
94+
// the rollout finishes, and acking here would skip the slot release
95+
// without saying so.
96+
return fmt.Errorf("failed to decode dlq payload: %w", err)
97+
}
98+
if sig.Id == "" {
99+
metrics.NamedCounter(c.metricsScope, _buildSignalOpName, "empty_id_errors", 1)
100+
return fmt.Errorf("dlq payload decoded to empty build id")
101+
}
102+
103+
store, err := c.stores.For(storage.Config{QueueName: sig.GetQueueName()})
104+
if err != nil {
105+
metrics.NamedCounter(c.metricsScope, _buildSignalOpName, "storage_resolve_errors", 1)
106+
// Non-retryable: a missing or unresolvable queue is a malformed message.
107+
return fmt.Errorf("failed to resolve storage for queue %q: %w", sig.GetQueueName(), err)
108+
}
109+
110+
dmeta := delivery.Metadata()
111+
c.logger.Warnw("dlq message received",
112+
"build_id", sig.Id,
113+
"attempt", delivery.Attempt(),
114+
"dlq_original_topic", dmeta["dlq.original_topic"],
115+
"dlq_failure_count", dmeta["dlq.failure_count"],
116+
"dlq_last_error", dmeta["dlq.last_error"],
117+
)
118+
119+
build, err := store.GetBuildStore().Get(ctx, sig.Id)
120+
if err != nil {
121+
if errors.Is(err, storage.ErrNotFound) {
122+
// The build row was never written — a crash between Trigger and
123+
// Create. There is no request to recover from this payload; the build
124+
// stage's own DLQ handles the request that triggered it.
125+
c.logger.Warnw("dlq reconcile: build not found, skipping", "build_id", sig.Id)
126+
metrics.NamedCounter(c.metricsScope, _buildSignalOpName, "build_not_found", 1)
127+
return nil
128+
}
129+
metrics.NamedCounter(c.metricsScope, _buildSignalOpName, "build_store_errors", 1)
130+
return fmt.Errorf("failed to get build %s: %w", sig.Id, err)
131+
}
132+
133+
if build.RequestID == "" {
134+
// Defensive: a build with no request has nothing to reconcile and no slot
135+
// to release. Ack it so the DLQ does not grow forever.
136+
c.logger.Errorw("dlq reconcile: build has empty request id, skipping", "build_id", sig.Id)
137+
metrics.NamedCounter(c.metricsScope, _buildSignalOpName, "build_missing_request", 1)
138+
return nil
139+
}
140+
141+
// Every request reachable from a build row is either still processing, and holding
142+
// the slot failRequest releases, or already terminal, and past releasing it: build
143+
// triggers only once process has written the strategy, which lands in the same CAS
144+
// as accepted→processing, and processing exits only to a terminal outcome.
145+
if err := failRequest(ctx, store, c.logger, build.RequestID); err != nil {
146+
metrics.NamedCounter(c.metricsScope, _buildSignalOpName, "reconcile_errors", 1)
147+
return err
148+
}
149+
150+
metrics.NamedCounter(c.metricsScope, _buildSignalOpName, "reconciled", 1)
151+
return nil
152+
}
153+
154+
// Name returns the controller name for logging and metrics.
155+
func (c *BuildSignalController) Name() string {
156+
return "buildsignal_dlq"
157+
}
158+
159+
// TopicKey returns the topic key this controller subscribes to.
160+
func (c *BuildSignalController) TopicKey() consumer.TopicKey {
161+
return c.topicKey
162+
}
163+
164+
// ConsumerGroup returns the consumer group for offset tracking.
165+
func (c *BuildSignalController) ConsumerGroup() string {
166+
return c.consumerGroup
167+
}

0 commit comments

Comments
 (0)