Skip to content

Commit 9227c40

Browse files
committed
fix(orchestrator): mint distinct message IDs for cancel re-publishes
## Summary ### Why? The cancel controller published its speculate hand-off with the bare batch ID as the message ID. The queue deduplicates on (topic, partition key, message ID) against every row it has not garbage-collected yet, consumed ones included, so a redelivery's re-publish for the same batch was a silent no-op — leaving a batch stuck Cancelling with nothing driving it to terminal. Same class of bug as b1f5795 in stovepipe. ### What? `publishBatchID` now goes through `submitqueue/core/publish`: `publish.UniqueID` mints a distinct message ID per publish (and documents the dedup rule once), and `publish.Message` owns the registry-lookup plumbing the controller previously hand-rolled. The batch-path test now asserts the payload still carries the batch ID while the message ID is distinct per publish, and the multi-batch tests assert on the payload's batch ID for the same reason. ## Test Plan ✅ `bazel test //submitqueue/orchestrator/controller/cancel/...`
1 parent 492d1cb commit 9227c40

3 files changed

Lines changed: 38 additions & 30 deletions

File tree

submitqueue/orchestrator/controller/cancel/BUILD.bazel

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,9 @@ go_library(
66
importpath = "github.com/uber/submitqueue/submitqueue/orchestrator/controller/cancel",
77
visibility = ["//visibility:public"],
88
deps = [
9-
"//platform/base/messagequeue:go_default_library",
109
"//platform/consumer:go_default_library",
1110
"//platform/metrics:go_default_library",
11+
"//submitqueue/core/publish:go_default_library",
1212
"//submitqueue/core/request:go_default_library",
1313
"//submitqueue/core/topickey:go_default_library",
1414
"//submitqueue/entity:go_default_library",

submitqueue/orchestrator/controller/cancel/cancel.go

Lines changed: 9 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -57,9 +57,9 @@ import (
5757
"sort"
5858

5959
"github.com/uber-go/tally"
60-
entityqueue "github.com/uber/submitqueue/platform/base/messagequeue"
6160
"github.com/uber/submitqueue/platform/consumer"
6261
"github.com/uber/submitqueue/platform/metrics"
62+
"github.com/uber/submitqueue/submitqueue/core/publish"
6363
corerequest "github.com/uber/submitqueue/submitqueue/core/request"
6464
"github.com/uber/submitqueue/submitqueue/core/topickey"
6565
"github.com/uber/submitqueue/submitqueue/entity"
@@ -338,29 +338,18 @@ func (c *Controller) cancelBatch(ctx context.Context, batch entity.Batch) error
338338
}
339339

340340
// publishBatchID publishes a BatchID-payload message to the specified topic key.
341+
//
342+
// The message ID is distinct per publish (publish.UniqueID). The queue
343+
// deduplicates on (topic, partition key, message ID) against every row it has
344+
// not collected yet, consumed ones included, so a bare batch ID would make the
345+
// redelivery re-publish documented above a silent no-op — leaving a batch
346+
// Cancelling with nothing driving it to terminal.
341347
func (c *Controller) publishBatchID(ctx context.Context, key consumer.TopicKey, batchID string, partitionKey string) error {
342-
bid := entity.BatchID{ID: batchID}
343-
payload, err := bid.ToBytes()
348+
payload, err := entity.BatchID{ID: batchID}.ToBytes()
344349
if err != nil {
345350
return fmt.Errorf("failed to serialize batch ID: %w", err)
346351
}
347-
348-
msg := entityqueue.NewMessage(batchID, payload, partitionKey, nil)
349-
350-
q, ok := c.registry.Queue(key)
351-
if !ok {
352-
return fmt.Errorf("no queue registered for topic key %s", key)
353-
}
354-
355-
topicName, ok := c.registry.TopicName(key)
356-
if !ok {
357-
return fmt.Errorf("no topic name registered for topic key %s", key)
358-
}
359-
360-
if err := q.Publisher().Publish(ctx, topicName, msg); err != nil {
361-
return fmt.Errorf("failed to publish message: %w", err)
362-
}
363-
return nil
352+
return publish.Message(ctx, c.registry, key, publish.UniqueID(batchID), payload, partitionKey, 0)
364353
}
365354

366355
// Name returns the controller name for logging and metrics.

submitqueue/orchestrator/controller/cancel/cancel_test.go

Lines changed: 28 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -333,23 +333,31 @@ func TestProcess_UnbatchedRequestDisappears_Retryable(t *testing.T) {
333333

334334
// TestProcess_BatchPath_HandsOffToSpeculate asserts the entire batch path:
335335
// the request intent CAS runs, the batch intent CAS to Cancelling runs, and
336-
// exactly one publish lands on the speculate topic with the batch ID as the
337-
// message ID. The controller does NOT perform a terminal batch CAS, does
338-
// NOT publish to conclude, and does NOT emit a per-request log on this path
339-
// (the gateway already wrote the Cancelling intent log; conclude writes the
340-
// terminal log when it reconciles request state).
336+
// exactly one publish lands on the speculate topic carrying the batch ID. The
337+
// message ID is not the bare batch ID: the queue deduplicates on it, so a
338+
// redelivery's re-publish would be silently dropped and the batch left
339+
// Cancelling with nothing driving it. The controller does NOT perform a
340+
// terminal batch CAS, does NOT publish to conclude, and does NOT emit a
341+
// per-request log on this path (the gateway already wrote the Cancelling
342+
// intent log; conclude writes the terminal log when it reconciles request
343+
// state).
341344
func TestProcess_BatchPath_HandsOffToSpeculate(t *testing.T) {
342345
ctrl := gomock.NewController(t)
343346
registry, pub := newRegistry(t, ctrl)
344347

345348
type pubRec struct {
346349
topic string
347350
msgID string
351+
// payloadID is the batch ID the message actually carries, which is what
352+
// the consumer acts on — the message ID is only the queue's dedup key.
353+
payloadID string
348354
}
349355
var records []pubRec
350356
pub.EXPECT().Publish(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(
351357
func(_ context.Context, topic string, msg entityqueue.Message) error {
352-
records = append(records, pubRec{topic: topic, msgID: msg.ID})
358+
bid, err := entity.BatchIDFromBytes(msg.Payload)
359+
require.NoError(t, err)
360+
records = append(records, pubRec{topic: topic, msgID: msg.ID, payloadID: bid.ID})
353361
return nil
354362
}).AnyTimes()
355363

@@ -380,7 +388,12 @@ func TestProcess_BatchPath_HandsOffToSpeculate(t *testing.T) {
380388
err := controller.Process(context.Background(), newDelivery(t, ctrl, cancelPayload(t, "q/1", "stop"), "q/1"))
381389
require.NoError(t, err)
382390

383-
assert.Equal(t, []pubRec{{topic: "speculate", msgID: "q/batch/1"}}, records)
391+
require.Len(t, records, 1)
392+
assert.Equal(t, "speculate", records[0].topic)
393+
assert.Equal(t, batch.ID, records[0].payloadID)
394+
assert.NotEqual(t, batch.ID, records[0].msgID,
395+
"a bare batch ID as the message ID lets the queue swallow the redelivery re-publish")
396+
assert.Contains(t, records[0].msgID, batch.ID)
384397
}
385398

386399
func TestProcess_CancelsEveryApplicableBatch(t *testing.T) {
@@ -412,7 +425,11 @@ func TestProcess_CancelsEveryApplicableBatch(t *testing.T) {
412425
)
413426
publisher.EXPECT().Publish(gomock.Any(), "speculate", gomock.Any()).DoAndReturn(
414427
func(_ context.Context, _ string, msg entityqueue.Message) error {
415-
operations = append(operations, "publish:"+msg.ID)
428+
// The message ID is the queue's dedup key and is distinct per
429+
// publish; the payload carries the batch ID the consumer acts on.
430+
bid, err := entity.BatchIDFromBytes(msg.Payload)
431+
require.NoError(t, err)
432+
operations = append(operations, "publish:"+bid.ID)
416433
return nil
417434
},
418435
).Times(2)
@@ -450,7 +467,9 @@ func TestProcess_BatchFailureDoesNotPreventLaterCancellation(t *testing.T) {
450467
batchStore.EXPECT().Update(gomock.Any(), batchWithState(batch2, entity.BatchStateCancelling), int32(2), int32(3)).Return(nil)
451468
publisher.EXPECT().Publish(gomock.Any(), "speculate", gomock.Any()).DoAndReturn(
452469
func(_ context.Context, _ string, msg entityqueue.Message) error {
453-
assert.Equal(t, batch2.ID, msg.ID)
470+
bid, err := entity.BatchIDFromBytes(msg.Payload)
471+
require.NoError(t, err)
472+
assert.Equal(t, batch2.ID, bid.ID)
454473
return nil
455474
},
456475
)

0 commit comments

Comments
 (0)