Skip to content

Commit 5240c41

Browse files
committed
fix(speculate): blame the batch that failed, and stop the queue stranding
## Summary ### Why? Two things go wrong when a speculate message dead-letters, and together they leave a queue quietly stuck. The first is misattribution. The speculate stage takes a message naming one batch but does a job covering the whole queue: a run lists every in-flight batch, hands them all to the Speculator, and commits outcomes for any of them. Only the handful of errors before the run even starts are about the batch on the message. Everything after is about some other batch, or about the queue itself. Yet the stage shared `dlq.NewDLQBatchController` with build, merge, and conclude — which marks *the named* batch `Failed` and errors all its requests. So a Speculator error, or a storage failure writing another head's path set, terminated a batch that was never at fault while the real culprit carried on. The second is that the reconcile restored nothing. `failBatch` published no message at all. Speculation is driven only by messages, and a batch admitted to `Speculating` produces no build to signal and no merge to conclude — so once the message that would have funded it is gone, nothing is left to look at it again. The dead letter consumed the queue's last edge and left none behind, stranding every other admitted batch with no error recorded against it and its requests still reading `batched`. From the outside the queue looked like it was working. ### What? Speculate now says what its failures are about, and its dead letters act on that. Every error return is attributed with `errs.Attribute`: the message's batch for the errors raised before the run, the specific batch for errors raised while looping over the queue, and `{Type: queue}` for listing the queue and for both Speculator calls, where no batch is at fault. Attribution is added through `entity.BatchSubject` / `entity.QueueSubject`, and a counter tagged by subject type makes queue-scoped failures graphable. Retryability is untouched — the classifiers still read the cause underneath. The stage gets its own reconciler, `dlq/speculate.go`, instead of the shared batch one: - Batch subjects are taken at their word and those batches are failed, which may not be the batch on the message. - A queue subject, or no subjects at all, falls back to the message's batch — a guess, but DLQ reconciliation exists so requests cannot sit non-terminal forever, and that guarantee has to hold even when nothing can say which batch was at fault. Which of the three happened is recorded as `dlq.attribution`, so a fallback is never mistaken for a confident answer. - Afterwards it republishes one speculate message naming a still-live batch, restoring the edge it consumed. Two guards make it terminate: it runs only after a reconcile that actually transitioned a batch, so a redelivery cannot loop, and it names a *live* batch rather than the one just failed, so each pass fails one more. A genuinely broken queue therefore drains to empty with a reason recorded against every batch, bounded by the batch count, while a queue whose failure was transient or queue-wide recovers on the next run having lost one batch instead of stranding all of them. Separately, the shared `failRequest` passed `nil` where `RequestLog.Metadata` goes, discarding the failure count, originating topic, and timestamp the reconciler already had in hand and logged. It now carries them, along with the failure's subjects and detail. No schema or proto change is needed: `RequestLog.Metadata` is already persisted and already exposed on the gateway's status and history, so this lands in front of users directly. All five reconcilers benefit. ## Test Plan ✅ `bazel test //...` ✅ `bazel test //test/... --sandbox_writable_path=$HOME/.docker --jobs=1` (11 container suites) ✅ `make lint check-tidy check-gazelle check-mocks` New coverage in `dlq/speculate_test.go`: - Attribution as a table: a batch subject fails *that* batch rather than the message's; a queue subject and a subject-less failure each fall back, and each records which it was. - The re-trigger publishes for a queue that still holds live batches, choosing deterministically (`ListByStates` promises no order). - The loop guard: an already-failed batch publishes nothing even with live batches remaining, which is what stops a permanently failing queue from re-triggering forever. ## Issue Closes CODEM-428
1 parent 51d4d6e commit 5240c41

19 files changed

Lines changed: 754 additions & 49 deletions

submitqueue/entity/BUILD.bazel

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,11 +24,13 @@ go_library(
2424
"request_log.go",
2525
"request_summary.go",
2626
"speculation.go",
27+
"subject.go",
2728
],
2829
importpath = "github.com/uber/submitqueue/submitqueue/entity",
2930
visibility = ["//visibility:public"],
3031
deps = [
3132
"//platform/base/change:go_default_library",
33+
"//platform/base/failure:go_default_library",
3234
"//platform/base/mergestrategy:go_default_library",
3335
],
3436
)

submitqueue/entity/subject.go

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
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 entity
16+
17+
import "github.com/uber/submitqueue/platform/base/failure"
18+
19+
// Subject types name what a failure is about. The queue layer treats the type
20+
// as opaque; these are SubmitQueue's own vocabulary for it.
21+
const (
22+
// SubjectTypeBatch identifies a subject by batch ID.
23+
SubjectTypeBatch = "batch"
24+
// SubjectTypeQueue identifies a subject by queue name. Used where no single
25+
// batch is at fault — a failure reading or planning the queue as a whole.
26+
SubjectTypeQueue = "queue"
27+
// SubjectTypeRequest identifies a subject by request ID.
28+
SubjectTypeRequest = "request"
29+
)
30+
31+
// BatchSubject names a batch as what a failure is about.
32+
func BatchSubject(batchID string) failure.Subject {
33+
return failure.Subject{Type: SubjectTypeBatch, ID: batchID}
34+
}
35+
36+
// QueueSubject names a queue as what a failure is about. It is the honest
37+
// subject for work that spans the queue — listing it, planning it — where
38+
// blaming any one batch would be a guess.
39+
func QueueSubject(queue string) failure.Subject {
40+
return failure.Subject{Type: SubjectTypeQueue, ID: queue}
41+
}
42+
43+
// RequestSubject names a request as what a failure is about.
44+
func RequestSubject(requestID string) failure.Subject {
45+
return failure.Subject{Type: SubjectTypeRequest, ID: requestID}
46+
}

submitqueue/orchestrator/controller/dlq/BUILD.bazel

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ go_library(
1010
"mergeconflictsignal.go",
1111
"mergesignal.go",
1212
"request.go",
13+
"speculate.go",
1314
],
1415
importpath = "github.com/uber/submitqueue/submitqueue/orchestrator/controller/dlq",
1516
visibility = ["//visibility:public"],
@@ -18,7 +19,9 @@ go_library(
1819
"//platform/consumer:go_default_library",
1920
"//platform/metrics:go_default_library",
2021
"//submitqueue/core/batch:go_default_library",
22+
"//submitqueue/core/publish:go_default_library",
2123
"//submitqueue/core/request:go_default_library",
24+
"//submitqueue/core/topickey:go_default_library",
2225
"//submitqueue/entity:go_default_library",
2326
"//submitqueue/extension/storage:go_default_library",
2427
"@com_github_uber_go_tally//:go_default_library",
@@ -37,11 +40,13 @@ go_test(
3740
"mergesignal_test.go",
3841
"publisher_test.go",
3942
"request_test.go",
43+
"speculate_test.go",
4044
],
4145
embed = [":go_default_library"],
4246
deps = [
4347
"//api/runway/messagequeue:go_default_library",
4448
"//api/runway/messagequeue/protopb:go_default_library",
49+
"//platform/base/failure:go_default_library",
4550
"//platform/base/messagequeue:go_default_library",
4651
"//platform/consumer:go_default_library",
4752
"//platform/consumer/mock:go_default_library",

submitqueue/orchestrator/controller/dlq/batch.go

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -27,15 +27,21 @@ import (
2727
)
2828

2929
// batchController is the DLQ reconciler for batch-scoped pipeline stages
30-
// (speculate, build, merge, conclude). All four topics carry a
31-
// BatchID payload, so this controller is registered four times — one per
32-
// topic, each with the matching DLQ topic key and consumer group.
30+
// (build, merge, conclude). All three topics carry a BatchID payload, so this
31+
// controller is registered three times — one per topic, each with the matching
32+
// DLQ topic key and consumer group.
3333
//
3434
// On each delivery the controller decodes the BatchID, transitions the batch
3535
// to BatchStateFailed (idempotent if already halted), and fans out by
3636
// transitioning each member request to RequestStateError. The fan-out exists
3737
// because conclude — which normally drives request state from batch state —
3838
// will not run for a DLQ'd batch.
39+
//
40+
// Blaming the batch on the message is right for these stages because their
41+
// work is that batch: whatever failed, it failed doing this batch's build,
42+
// merge, or conclusion. The speculate stage is not like that — it re-plans a
43+
// whole queue from a message that names one batch — so it has its own
44+
// reconciler; see speculate.go.
3945
type batchController struct {
4046
logger *zap.SugaredLogger
4147
metricsScope tally.Scope
@@ -92,6 +98,7 @@ func (c *batchController) Process(ctx context.Context, delivery consumer.Deliver
9298
return fmt.Errorf("failed to resolve storage for queue %q: %w", bid.Queue, err)
9399
}
94100

101+
lastError, failureMeta := failureContext(delivery)
95102
dmeta := delivery.Metadata()
96103
c.logger.Warnw("dlq message received",
97104
"batch_id", bid.ID,
@@ -101,7 +108,7 @@ func (c *batchController) Process(ctx context.Context, delivery consumer.Deliver
101108
"dlq_last_error", dmeta["dlq.last_error"],
102109
)
103110

104-
if err := failBatch(ctx, store, c.registry, c.logger, bid.ID, dmeta["dlq.last_error"]); err != nil {
111+
if _, err := failBatch(ctx, store, c.registry, c.logger, bid.ID, lastError, failureMeta); err != nil {
105112
metrics.NamedCounter(c.metricsScope, opName, "reconcile_errors", 1)
106113
return err
107114
}

submitqueue/orchestrator/controller/dlq/buildsignal.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,7 @@ func (c *buildSignalController) Process(ctx context.Context, delivery consumer.D
9090
return fmt.Errorf("failed to resolve storage for queue %q: %w", buildID.Queue, err)
9191
}
9292

93+
lastError, failureMeta := failureContext(delivery)
9394
dmeta := delivery.Metadata()
9495
c.logger.Warnw("dlq message received",
9596
"build_id", buildID.ID,
@@ -121,7 +122,7 @@ func (c *buildSignalController) Process(ctx context.Context, delivery consumer.D
121122
return nil
122123
}
123124

124-
if err := failBatch(ctx, store, c.registry, c.logger, build.BatchID, dmeta["dlq.last_error"]); err != nil {
125+
if _, err := failBatch(ctx, store, c.registry, c.logger, build.BatchID, lastError, failureMeta); err != nil {
125126
metrics.NamedCounter(c.metricsScope, opName, "reconcile_errors", 1)
126127
return err
127128
}

submitqueue/orchestrator/controller/dlq/dlq.go

Lines changed: 85 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ import (
3737
"context"
3838
"errors"
3939
"fmt"
40+
"strings"
4041

4142
"github.com/uber/submitqueue/platform/consumer"
4243
corebatch "github.com/uber/submitqueue/submitqueue/core/batch"
@@ -62,19 +63,81 @@ func TopicKey(main consumer.TopicKey) consumer.TopicKey {
6263
return consumer.TopicKey(string(main) + topicSuffix)
6364
}
6465

66+
// failureContext reads everything the queue recorded about a dead-lettered
67+
// message: the human-readable reason, and a metadata map to carry alongside it
68+
// on the terminal request log.
69+
//
70+
// The map is what makes a dead letter diagnosable after the fact. The queue
71+
// already hands the reconciler the failure count, the topic it failed on, and
72+
// when — and, when the producer attributed it, which entities it was about.
73+
// All of that used to be logged and then dropped; the request log is where a
74+
// user can actually see it, through the gateway's status and history.
75+
//
76+
// Values are flattened to strings because RequestLog.Metadata and the gateway's
77+
// wire contract are both string maps. Nested detail becomes dotted keys, which
78+
// read well in a display surface where a JSON blob would not.
79+
func failureContext(delivery consumer.Delivery) (string, map[string]string) {
80+
dmeta := delivery.Metadata()
81+
metadata := make(map[string]string, len(dmeta))
82+
for _, key := range []string{"dlq.original_topic", "dlq.failure_count", "dlq.failed_at"} {
83+
if v, ok := dmeta[key]; ok && v != "" {
84+
metadata[key] = v
85+
}
86+
}
87+
88+
lastError := dmeta["dlq.last_error"]
89+
90+
f, failed := delivery.Failure()
91+
if !failed {
92+
return lastError, metadata
93+
}
94+
if f.Message != "" {
95+
lastError = f.Message
96+
}
97+
98+
// One key per subject type, so several batches at fault read as a list
99+
// rather than overwriting each other.
100+
byType := make(map[string][]string, len(f.Subjects))
101+
for _, s := range f.Subjects {
102+
byType[s.Type] = append(byType[s.Type], s.ID)
103+
}
104+
for subjectType, ids := range byType {
105+
metadata["dlq.subject."+subjectType] = strings.Join(ids, ",")
106+
}
107+
108+
flattenDetail("dlq.detail", f.Detail, metadata)
109+
110+
return lastError, metadata
111+
}
112+
113+
// flattenDetail writes a JSON-shaped document into a string map, joining nested
114+
// keys with dots. Values are rendered with %v: this is a display surface, not a
115+
// contract, so a readable rendering beats a faithful one.
116+
func flattenDetail(prefix string, detail map[string]any, out map[string]string) {
117+
for k, v := range detail {
118+
key := prefix + "." + k
119+
if nested, ok := v.(map[string]any); ok {
120+
flattenDetail(key, nested, out)
121+
continue
122+
}
123+
out[key] = fmt.Sprintf("%v", v)
124+
}
125+
}
126+
65127
// failRequest transitions a non-terminal request to RequestStateError and
66128
// appends the matching RequestStatusError log. Redelivery for an existing Error
67129
// state repeats materialization to repair a previous partial attempt. A
68130
// different terminal outcome is left unchanged.
69131
// lastError is the failure reason preserved by the queue in DLQ delivery
70-
// metadata and is exposed through Status and History for diagnosis.
132+
// metadata and is exposed through Status and History for diagnosis, alongside
133+
// metadata carrying the rest of the failure context.
71134
//
72135
// A request in RequestStateCancelling is reconciled to RequestStateError, not
73136
// left in place: DLQ means the pipeline failed to converge, so we cannot
74137
// confirm the cancel completed cleanly. Writing Error is the honest signal and
75138
// keeps the request from being stuck in a non-terminal state forever.
76-
func failRequest(ctx context.Context, store storage.Storage, registry consumer.TopicRegistry, logger *zap.SugaredLogger, requestID, lastError string) error {
77-
res, err := requestcore.TerminateRequest(ctx, store, registry, requestID, entity.RequestStateError, lastError, nil)
139+
func failRequest(ctx context.Context, store storage.Storage, registry consumer.TopicRegistry, logger *zap.SugaredLogger, requestID, lastError string, metadata map[string]string) error {
140+
res, err := requestcore.TerminateRequest(ctx, store, registry, requestID, entity.RequestStateError, lastError, metadata)
78141
if err != nil {
79142
return fmt.Errorf("dlq reconcile request %s failed: %w", requestID, err)
80143
}
@@ -112,19 +175,27 @@ func failRequest(ctx context.Context, store storage.Storage, registry consumer.T
112175
// Idempotency: an existing Failed batch repeats fan-out because a previous
113176
// attempt may have crashed after updating the batch. Succeeded and Cancelled
114177
// are different terminal outcomes and do not fan out errors.
115-
// lastError is propagated to each member request's terminal Error log.
116-
func failBatch(ctx context.Context, store storage.Storage, registry consumer.TopicRegistry, logger *zap.SugaredLogger, batchID, lastError string) error {
178+
// lastError and metadata are propagated to each member request's terminal
179+
// Error log.
180+
//
181+
// It reports whether it transitioned the batch. A caller that only wants to act
182+
// on real progress — republishing to wake the queue, say — can then tell a
183+
// first reconcile from a redelivery of one already done, and avoid doing it
184+
// again forever.
185+
func failBatch(ctx context.Context, store storage.Storage, registry consumer.TopicRegistry, logger *zap.SugaredLogger, batchID, lastError string, metadata map[string]string) (bool, error) {
117186
batch, err := store.GetBatchStore().Get(ctx, batchID)
118187
if err != nil {
119188
if errors.Is(err, storage.ErrNotFound) {
120189
logger.Warnw("dlq reconcile: batch not found, skipping",
121190
"batch_id", batchID,
122191
)
123-
return nil
192+
return false, nil
124193
}
125-
return fmt.Errorf("failed to get batch %s: %w", batchID, err)
194+
return false, fmt.Errorf("failed to get batch %s: %w", batchID, err)
126195
}
127196

197+
transitioned := false
198+
128199
switch batch.State {
129200
case entity.BatchStateFailed:
130201
logger.Infow("dlq reconcile: batch already failed, repairing request fan-out",
@@ -133,31 +204,32 @@ func failBatch(ctx context.Context, store storage.Storage, registry consumer.Top
133204
// A prior attempt may have CAS'd to Failed without completing the
134205
// membership record move; repair it alongside the fan-out.
135206
if err := corebatch.EnsureRecord(ctx, store, batch); err != nil {
136-
return err
207+
return false, err
137208
}
138209
case entity.BatchStateSucceeded, entity.BatchStateCancelled:
139210
logger.Infow("dlq reconcile: batch has a different terminal outcome, skipping",
140211
"batch_id", batchID,
141212
"state", string(batch.State),
142213
)
143-
return nil
214+
return false, nil
144215
default:
145216
previousState := batch.State
146217
updated, err := corebatch.Transition(ctx, store, batch, entity.BatchStateFailed)
147218
if err != nil {
148-
return err
219+
return false, err
149220
}
150221
batch = updated
222+
transitioned = true
151223
logger.Infow("dlq reconcile: batch marked failed",
152224
"batch_id", batchID,
153225
"previous_state", string(previousState),
154226
)
155227
}
156228

157229
for _, requestID := range batch.Contains {
158-
if err := failRequest(ctx, store, registry, logger, requestID, lastError); err != nil {
159-
return fmt.Errorf("fan-out for batch %s: %w", batchID, err)
230+
if err := failRequest(ctx, store, registry, logger, requestID, lastError, metadata); err != nil {
231+
return transitioned, fmt.Errorf("fan-out for batch %s: %w", batchID, err)
160232
}
161233
}
162-
return nil
234+
return transitioned, nil
163235
}

0 commit comments

Comments
 (0)