Skip to content

Commit 7d25360

Browse files
authored
feat(orchestrator): carry the queue explicitly on every internal payload (#527)
## Summary ### Why? Consumers today learn a message's queue only by loading the referenced entity from storage, and entity IDs are opaque — nothing may parse a queue out of an ID prefix. For storage to become queue-scoped (resolved per queue like every other extension), every consumer must hold the queue before its first storage read, so the queue has to ride on the wire explicitly. ### What? The internal payload types (`RequestID`, `BatchID`, `BuildID`, `CancelRequest`) gain a `queue` field; the change is additive JSON, and payloads written before the field existed decode with an empty queue. Publishers stamp it from the entity they already hold: start, mergeconflictsignal, batch, speculate, mergesignal, build, buildsignal, and the orchestrator cancel controller; the gateway cancel controller stamps the authoritative queue from the stored request summary, overriding caller input. Consumers guard that a non-empty payload queue matches the loaded entity's queue and reject mismatches as malformed (non-retryable, straight to DLQ) — the guard becomes the routing input once storage is queue-resolved. Buildsignal's re-publish to speculate now partitions by the batch's queue instead of the inherited batch-ID partition key, matching every other speculate publisher and restoring the per-queue serial-processing guarantee that stage relies on. ## Test Plan ✅ `go test ./...` ✅ `make fmt` ✅ `make lint` — new unit tests cover the queue stamp on published payloads and the mismatch rejection guard.
1 parent 645ae94 commit 7d25360

18 files changed

Lines changed: 217 additions & 44 deletions

File tree

submitqueue/entity/batch.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -187,6 +187,8 @@ func BatchFromBytes(data []byte) (Batch, error) {
187187
type BatchID struct {
188188
// ID is the globally unique identifier for the batch.
189189
ID string `json:"id"`
190+
// Queue is the name of the queue processing the batch. Empty on payloads written before the field existed.
191+
Queue string `json:"queue"`
190192
}
191193

192194
// ToBytes serializes the BatchID to JSON bytes for queue message payload.

submitqueue/entity/build.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,8 @@ func BuildFromBytes(data []byte) (Build, error) {
8181
type BuildID struct {
8282
// ID is the globally unique identifier for the build.
8383
ID string `json:"id"`
84+
// Queue is the name of the queue processing the batch this build verifies. Empty on payloads written before the field existed.
85+
Queue string `json:"queue"`
8486
}
8587

8688
// ToBytes serializes the BuildID to JSON bytes for queue message payload.

submitqueue/entity/cancel.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,8 @@ import "encoding/json"
2121
type CancelRequest struct {
2222
// ID is the globally unique identifier of the request to cancel. Format: "<queue>/<counter_value>".
2323
ID string `json:"id"`
24+
// Queue is the name of the queue processing the request to cancel. Empty on payloads written before the field existed.
25+
Queue string `json:"queue"`
2426
// Reason is an optional free-form explanation of why the cancellation was requested.
2527
Reason string `json:"reason"`
2628
}

submitqueue/entity/request.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,8 @@ func RequestFromBytes(data []byte) (Request, error) {
113113
type RequestID struct {
114114
// ID is the globally unique identifier for the land request.
115115
ID string `json:"id"`
116+
// Queue is the name of the queue processing the land request. Empty on payloads written before the field existed.
117+
Queue string `json:"queue"`
116118
}
117119

118120
// ToBytes serializes the RequestID to JSON bytes for queue message payload.

submitqueue/extension/storage/mock/request_batch_store_mock.go

Lines changed: 2 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

submitqueue/gateway/controller/cancel.go

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,14 +86,19 @@ func (c *cancelController) Cancel(ctx context.Context, req entity.CancelRequest)
8686
)
8787

8888
// Verify the sqid exists before recording intent or publishing.
89-
if _, err := c.requestSummaryStore.Get(ctx, req.ID); err != nil {
89+
summary, err := c.requestSummaryStore.Get(ctx, req.ID)
90+
if err != nil {
9091
if storage.IsNotFound(err) {
9192
metrics.NamedCounter(c.metricsScope, opName, "not_found", 1)
9293
return errs.NewUserError(&RequestNotFoundError{Sqid: req.ID})
9394
}
9495
return fmt.Errorf("failed to look up request summary for sqid=%s: %w", req.ID, err)
9596
}
9697

98+
// Stamp the authoritative queue from the stored summary onto the payload,
99+
// overriding whatever the caller supplied.
100+
req.Queue = summary.Queue
101+
97102
// Record the user's intent in the request log before publishing. Writing direct to the
98103
// store (rather than via the log topic) keeps the gateway-emitted entry consistent with
99104
// the Land "accepted" entry and guarantees the entry is visible the moment Cancel returns.

submitqueue/orchestrator/controller/batch/batch.go

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,13 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er
9595
return fmt.Errorf("failed to get request %s: %w", rid.ID, err)
9696
}
9797

98+
// The payload's queue must match the request's authoritative queue; a
99+
// mismatch is a malformed message. Non-retryable — reject to the DLQ.
100+
if rid.Queue != "" && rid.Queue != request.Queue {
101+
metrics.NamedCounter(c.metricsScope, opName, "queue_mismatch", 1)
102+
return fmt.Errorf("payload queue %q does not match queue %q of request %s", rid.Queue, request.Queue, request.ID)
103+
}
104+
98105
c.logger.Infow("received batch event",
99106
"request_id", request.ID,
100107
"queue", request.Queue,
@@ -355,15 +362,16 @@ func (c *Controller) populateBatch(ctx context.Context, batch entity.Batch) (ent
355362
return batch, nil
356363
}
357364

358-
// publish publishes a batch ID to the specified topic key.
359-
func (c *Controller) publish(ctx context.Context, key consumer.TopicKey, batchID string, partitionKey string) error {
360-
bid := entity.BatchID{ID: batchID}
365+
// publish publishes a batch ID to the specified topic key, stamped with and
366+
// partitioned by the batch's queue.
367+
func (c *Controller) publish(ctx context.Context, key consumer.TopicKey, batchID string, queue string) error {
368+
bid := entity.BatchID{ID: batchID, Queue: queue}
361369
payload, err := bid.ToBytes()
362370
if err != nil {
363371
return fmt.Errorf("failed to serialize batch ID: %w", err)
364372
}
365373

366-
msg := entityqueue.NewMessage(batchID, payload, partitionKey, nil)
374+
msg := entityqueue.NewMessage(batchID, payload, queue, nil)
367375

368376
q, ok := c.registry.Queue(key)
369377
if !ok {

submitqueue/orchestrator/controller/batch/batch_test.go

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -195,6 +195,100 @@ func TestController_Process_Success(t *testing.T) {
195195
require.NoError(t, err)
196196
}
197197

198+
// TestController_Process_QueueMismatchRejected asserts a payload whose queue
199+
// disagrees with the request's authoritative queue is rejected without
200+
// touching the counter, the batch store, or the publisher.
201+
func TestController_Process_QueueMismatchRejected(t *testing.T) {
202+
ctrl := gomock.NewController(t)
203+
204+
request := testRequest()
205+
206+
mockReqStore := storagemock.NewMockRequestStore(ctrl)
207+
mockReqStore.EXPECT().Get(gomock.Any(), request.ID).Return(request, nil)
208+
209+
mockStorage := storagemock.NewMockStorage(ctrl)
210+
mockStorage.EXPECT().GetRequestStore().Return(mockReqStore).AnyTimes()
211+
212+
// Counter with no EXPECTs — must not be called.
213+
cnt := countermock.NewMockCounter(ctrl)
214+
controller := newTestController(t, ctrl, cnt, mockStorage, nil, fmt.Errorf("should not publish"))
215+
216+
payload, err := entity.RequestID{ID: request.ID, Queue: "some-other-queue"}.ToBytes()
217+
require.NoError(t, err)
218+
msg := entityqueue.NewMessage(request.ID, payload, request.Queue, nil)
219+
delivery := consumermock.NewMockDelivery(ctrl)
220+
delivery.EXPECT().Message().Return(msg).AnyTimes()
221+
delivery.EXPECT().Attempt().Return(1).AnyTimes()
222+
223+
require.Error(t, controller.Process(context.Background(), delivery))
224+
}
225+
226+
// TestController_Process_StampsQueueOnSpeculatePayload asserts the batch ID
227+
// published to speculate carries the batch's queue.
228+
func TestController_Process_StampsQueueOnSpeculatePayload(t *testing.T) {
229+
ctrl := gomock.NewController(t)
230+
231+
request := testRequest()
232+
233+
var speculateMsgs []entityqueue.Message
234+
mockPub := queuemock.NewMockPublisher(ctrl)
235+
mockPub.EXPECT().Publish(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(
236+
func(ctx context.Context, topic string, msg entityqueue.Message) error {
237+
if topic == "speculate" {
238+
speculateMsgs = append(speculateMsgs, msg)
239+
}
240+
return nil
241+
},
242+
).AnyTimes()
243+
mockQ := queuemock.NewMockQueue(ctrl)
244+
mockQ.EXPECT().Publisher().Return(mockPub).AnyTimes()
245+
246+
registry, err := consumer.NewTopicRegistry(
247+
[]consumer.TopicConfig{
248+
{Key: topickey.TopicKeySpeculate, Name: "speculate", Queue: mockQ},
249+
{Key: topickey.TopicKeyLog, Name: "log", Queue: mockQ},
250+
},
251+
)
252+
require.NoError(t, err)
253+
254+
mockBatchStore := storagemock.NewMockBatchStore(ctrl)
255+
mockBatchStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
256+
mockBatchStore.EXPECT().Update(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
257+
mockReqStore := storagemock.NewMockRequestStore(ctrl)
258+
mockReqStore.EXPECT().Get(gomock.Any(), request.ID).Return(request, nil).AnyTimes()
259+
mockReqStore.EXPECT().Update(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
260+
mockBatchDependentStore := storagemock.NewMockBatchDependentStore(ctrl)
261+
mockBatchDependentStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
262+
mockRequestBatchStore := storagemock.NewMockRequestBatchStore(ctrl)
263+
mockRequestBatchStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
264+
265+
mockStorage := storagemock.NewMockStorage(ctrl)
266+
mockStorage.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes()
267+
mockStorage.EXPECT().GetBatchStore().Return(mockBatchStore).AnyTimes()
268+
mockStorage.EXPECT().GetBatchDependentStore().Return(mockBatchDependentStore).AnyTimes()
269+
mockStorage.EXPECT().GetRequestBatchStore().Return(mockRequestBatchStore).AnyTimes()
270+
mockStorage.EXPECT().GetRequestStore().Return(mockReqStore).AnyTimes()
271+
272+
analyzerFactory := conflictmock.NewMockFactory(ctrl)
273+
analyzerFactory.EXPECT().For(gomock.Any()).Return(all.New(), nil).AnyTimes()
274+
controller := NewController(
275+
zaptest.NewLogger(t).Sugar(), tally.NoopScope, registry, newSequentialCounter(ctrl),
276+
mockStorage, analyzerFactory, topickey.TopicKeyBatch, "orchestrator-batch",
277+
)
278+
279+
msg := entityqueue.NewMessage(request.ID, requestIDPayload(t, request.ID), request.Queue, nil)
280+
delivery := consumermock.NewMockDelivery(ctrl)
281+
delivery.EXPECT().Message().Return(msg).AnyTimes()
282+
delivery.EXPECT().Attempt().Return(1).AnyTimes()
283+
284+
require.NoError(t, controller.Process(context.Background(), delivery))
285+
require.Len(t, speculateMsgs, 1)
286+
bid, err := entity.BatchIDFromBytes(speculateMsgs[0].Payload)
287+
require.NoError(t, err)
288+
assert.Equal(t, request.Queue, bid.Queue)
289+
assert.Equal(t, request.Queue, speculateMsgs[0].PartitionKey)
290+
}
291+
198292
// TestController_Process_PublishesBatchedLog asserts the controller emits a
199293
// "batched" request log carrying the request ID, the post-CAS request version,
200294
// and the batch ID it was placed into.

submitqueue/orchestrator/controller/build/build.go

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,13 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er
8989
return fmt.Errorf("failed to get batch %s: %w", bid.ID, err)
9090
}
9191

92+
// The payload's queue must match the batch's authoritative queue; a
93+
// mismatch is a malformed message. Non-retryable — reject to the DLQ.
94+
if bid.Queue != "" && bid.Queue != batch.Queue {
95+
metrics.NamedCounter(c.metricsScope, opName, "queue_mismatch", 1)
96+
return fmt.Errorf("payload queue %q does not match queue %q of batch %s", bid.Queue, batch.Queue, batch.ID)
97+
}
98+
9299
c.logger.Infow("received build event",
93100
"batch_id", batch.ID,
94101
"queue", batch.Queue,
@@ -151,7 +158,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er
151158
// Hand off to the buildsignal poll loop; it calls Status, updates the
152159
// persisted Build, publishes to speculate, and holds its delivery
153160
// between polls until terminal.
154-
if err := c.publish(ctx, topickey.TopicKeyBuildSignal, build); err != nil {
161+
if err := c.publish(ctx, topickey.TopicKeyBuildSignal, build, batch.Queue); err != nil {
155162
metrics.NamedCounter(c.metricsScope, opName, "publish_errors", 1)
156163
return fmt.Errorf("failed to publish to buildsignal: %w", err)
157164
}
@@ -184,11 +191,12 @@ func (c *Controller) loadBatches(ctx context.Context, batchIDs []string) ([]enti
184191
return batches, nil
185192
}
186193

187-
// publish publishes a build's ID to the specified topic key. Only the
188-
// identifier travels on the queue; the consumer loads the full Build from
189-
// storage, keeping the message small and the store the single source of truth.
190-
func (c *Controller) publish(ctx context.Context, key consumer.TopicKey, build entity.Build) error {
191-
payload, err := entity.BuildID{ID: build.ID}.ToBytes()
194+
// publish publishes a build's ID to the specified topic key, stamped with the
195+
// batch's queue and partitioned by the batch ID. Only the identifier and its
196+
// queue travel on the queue; the consumer loads the full Build from storage,
197+
// keeping the message small and the store the single source of truth.
198+
func (c *Controller) publish(ctx context.Context, key consumer.TopicKey, build entity.Build, queue string) error {
199+
payload, err := entity.BuildID{ID: build.ID, Queue: queue}.ToBytes()
192200
if err != nil {
193201
return fmt.Errorf("failed to serialize build ID: %w", err)
194202
}

submitqueue/orchestrator/controller/buildsignal/buildsignal.go

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,13 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er
135135
return fmt.Errorf("failed to get batch %s: %w", build.BatchID, err)
136136
}
137137

138+
// The payload's queue must match the batch's authoritative queue; a
139+
// mismatch is a malformed message. Non-retryable — reject to the DLQ.
140+
if buildID.Queue != "" && buildID.Queue != batch.Queue {
141+
metrics.NamedCounter(c.metricsScope, opName, "queue_mismatch", 1)
142+
return fmt.Errorf("payload queue %q does not match queue %q of batch %s", buildID.Queue, batch.Queue, batch.ID)
143+
}
144+
138145
buildRunner, err := c.buildRunners.For(buildrunner.Config{QueueName: batch.Queue})
139146
if err != nil {
140147
metrics.NamedCounter(c.metricsScope, opName, "status_errors", 1)
@@ -169,8 +176,10 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er
169176
return fmt.Errorf("failed to update status for build %s: %w", build.ID, err)
170177
}
171178

172-
// Re-evaluate the batch state machine with the latest build status.
173-
if err := c.publishBatchID(ctx, topickey.TopicKeySpeculate, updatedBuild.BatchID, msg.PartitionKey); err != nil {
179+
// Re-evaluate the batch state machine with the latest build status. The
180+
// speculate topic is partitioned by queue like every other speculate
181+
// publisher, so a queue's batches keep their serial processing guarantee.
182+
if err := c.publishBatchID(ctx, topickey.TopicKeySpeculate, updatedBuild.BatchID, batch.Queue); err != nil {
174183
metrics.NamedCounter(c.metricsScope, opName, "publish_errors", 1)
175184
return fmt.Errorf("failed to publish to speculate: %w", err)
176185
}
@@ -210,15 +219,16 @@ func pollDelay(status entity.BuildStatus) int64 {
210219
}
211220
}
212221

213-
// publishBatchID publishes a batch ID to the topic identified by key.
214-
func (c *Controller) publishBatchID(ctx context.Context, key consumer.TopicKey, batchID string, partitionKey string) error {
215-
bid := entity.BatchID{ID: batchID}
222+
// publishBatchID publishes a batch ID to the topic identified by key, stamped
223+
// with and partitioned by the batch's queue.
224+
func (c *Controller) publishBatchID(ctx context.Context, key consumer.TopicKey, batchID string, queue string) error {
225+
bid := entity.BatchID{ID: batchID, Queue: queue}
216226
payload, err := bid.ToBytes()
217227
if err != nil {
218228
return fmt.Errorf("failed to serialize batch ID: %w", err)
219229
}
220230

221-
msg := entityqueue.NewMessage(batchID, payload, partitionKey, nil)
231+
msg := entityqueue.NewMessage(batchID, payload, queue, nil)
222232

223233
q, ok := c.registry.Queue(key)
224234
if !ok {

0 commit comments

Comments
 (0)