Skip to content

Commit 8f2551a

Browse files
committed
fix(messagequeue): intent-scoped message IDs so wake-ups stop vanishing
## Summary ### Why? The MySQL queue deduplicates publishes on `(topic, partition_key, id)` via `INSERT ... ON DUPLICATE KEY UPDATE topic = topic`, and rows are removed only by `GarbageCollect` — which runs from `subscriber.go` on idle ticks only, with `gcCounter` reset to `0` by any tick that delivered a message. On a busy partition GC never runs, so the dedup horizon is unbounded exactly when traffic is high. A publish that collides is reported as a success, writes nothing, and has no error to retry and no row to deliver. Controllers reusing a bare entity ID as the message ID therefore lose their *second, unrelated* publish about that entity. Concretely: `batch` announces a new batch to speculate under the batch ID; when that batch later merges, `mergesignal.fanout`'s "wake the dependents" publish reuses the same ID and is dropped against the announcement. `fanout` returns nil and the delivery is acked. Other speculate publishers already mint distinct IDs, so another batch's build signal usually re-plans the queue and hides this — the stall shows at the tail, when the merged batch is the last in flight and nothing else pings speculate. Fixing only that call site would leave the shape in place. `submitqueue/core/publish` documented the hazard but was domain-scoped, so `runway/` and `stovepipe/` published bare entity IDs with no shared guidance, and `platform/base/messagequeue` documented none of it. ### What? `submitqueue/core/publish` moves to `platform/publish` — it already imported only `platform/base/messagequeue` and `platform/consumer`, so this is a relocation, and it lets every domain share one helper instead of hand-rolling the resolve-registry-and-publish block three more times. The new `publish.IntentID(entityID, cause...)` names the occasion to publish rather than the entity published about. A retry of the same cause dedups, which is what keeps redelivery safe; a new cause about the same entity can never be swallowed. Deterministic IDs go where a duplicate is harmful and the cause is nameable: | Publish | ID | | --- | --- | | merge result → speculate | `{batch}/merged` (**the bug**) | | merge result → conclude | `{batch}/conclude/merged` | | build poll → speculate | `{batch}/build-signal/{build}/{status}` | | speculate → merge dispatch | `{batch}/merge-dispatch` | | speculate → conclude on terminal | `{batch}/conclude/speculate` | | one-shot hand-offs | bare `IntentID(id)` | `buildsignal` publishes to speculate on *every* poll, so keying on `{batch}/build-signal/{build}` would have deduplicated the terminal wake-up into the first poll's. Including the observed status lands every transition and collapses only the polls that saw nothing new — a reduction in speculate churn versus the previous `UniqueID`. `publish.UniqueID` is kept, and deliberately left in place, for repeat-until-effective nudges whose provoking condition is that nothing recorded the last one — speculate's build dispatch, its self-heal fan-out and `recoverable`, cancel's nudge, the DLQ re-trigger. Those have no stable cause to name, and a deterministic ID would dedup the re-send against the message that went missing. Each now says so at the call site. `runway/controller/dlq` gets its own cause: it answered on the same topic under the same correlation ID as the live handler, so a dead-lettered request could have its terminal failure deduplicated against an answer already sent. `tool/linter/messageid` makes the helper the only door. Judging whether an ID expression is well chosen is not decidable by reading it, so the linter enforces the structural rule instead: `NewMessage` may only be called from `platform/publish` and the queue backends. It found seven production call sites the manual audit missed. ## Test Plan - ✅ `bazel test //...` — 99 unit tests - ✅ `bazel test //test/integration/...` — 8 suites - ✅ `bazel test //test/e2e/...` — 3 suites - ✅ `make lint-license`, `make lint-message-id`, `make lint-queue-shard`; `make fmt` idempotent; `make tidy` clean - New `TestProcess_FanoutDoesNotCollideWithTheBatchAnnouncement` was confirmed to fail against the old code — reverting the one expression reproduces the drop. - New `TestDedupOutlivesConsumption` pins the underlying behaviour against real MySQL: a consumed and acked message still deduplicates a later publish under the same ID, and naming the cause gets it through. - Not covered: no e2e exercises a dependent chain, so the merge→dependent-wake path is not verified end to end. Building that fixture is follow-up work. Out of scope, deliberately: GC never running on a busy partition. It widens this window and also lets `queue_messages` grow without bound, but it is orthogonal to the ID convention and wants its own review. ## Issue Closes #352
1 parent 61db11f commit 8f2551a

57 files changed

Lines changed: 886 additions & 357 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

Makefile

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -172,7 +172,7 @@ integration-test-submitqueue-orchestrator: ## Run Orchestrator integration tests
172172
license-fix: ## Add missing license headers to source files
173173
@$(BAZEL) run //tool/linter/licenseheader -- --fix
174174

175-
lint: lint-fmt lint-license lint-queue-shard ## Run all linters
175+
lint: lint-fmt lint-license lint-message-id lint-queue-shard ## Run all linters
176176
@echo "All lint checks passed."
177177

178178
lint-fmt: fmt ## Check code formatting (fails if unformatted)
@@ -182,6 +182,9 @@ lint-fmt: fmt ## Check code formatting (fails if unformatted)
182182
lint-license: ## Check license headers on all source files
183183
@$(BAZEL) run //tool/linter/licenseheader -- --check
184184

185+
lint-message-id: ## Check queue messages are only constructed through platform/publish
186+
@$(BAZEL) run //tool/linter/messageid
187+
185188
lint-queue-shard: ## Check every table's primary key leads with the queue column
186189
@$(BAZEL) run //tool/linter/queueshard
187190

platform/base/messagequeue/message.go

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,17 @@ import (
2323
// Immutable - use Copy() for modifications.
2424
type Message struct {
2525
// ID uniquely identifies the message for deduplication and tracing.
26+
//
27+
// Deduplication is against every message the backend still holds for the
28+
// same topic and partition key — including ones already consumed, which are
29+
// reclaimed lazily and may outlive their delivery by an unbounded interval.
30+
// A publish whose ID collides is reported as a success and stores nothing.
31+
//
32+
// So the ID names the occasion to publish, not the entity published about.
33+
// Reusing an entity's own ID gives that entity one message for as long as
34+
// the backend remembers the first, and silently discards every later one.
35+
// Producers build IDs with platform/publish.IntentID rather than choosing
36+
// them by hand.
2637
ID string
2738

2839
// Payload is the message body as raw bytes.

platform/extension/messagequeue/README.md

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,10 +96,21 @@ for delivery := range deliveries {
9696
}
9797
```
9898

99+
## Message IDs
100+
101+
A message ID is the deduplication key, scoped to its topic and partition key. A backend matches a publish against messages it still holds — including ones already consumed, since reclamation is lazy and may lag delivery by an unbounded interval — and a collision is reported to the publisher as a success that stored nothing. There is no error to retry and no row to deliver.
102+
103+
The ID therefore names the *occasion* to publish, not the entity published about. An entity's own ID buys exactly one message for that entity for as long as the backend remembers the first, so a stage that announces a batch at creation and another that wakes it after a merge would collide, and the wake-up would vanish.
104+
105+
Producers do not choose IDs by hand. They publish through `platform/publish`, whose `IntentID(entityID, cause...)` composes the entity with the cause of this particular message: a retry of the same cause dedups, which is what makes redelivery safe, while a new cause about the same entity can never be swallowed. `UniqueID` is the fallback for a cause with nothing stable to name it by, and it trades that idempotency for guaranteed delivery.
106+
107+
Backends must treat the ID as opaque and must not derive routing, ordering, or storage layout from its structure.
108+
99109
## Implementing a Backend
100110

101111
1. Create `platform/extension/messagequeue/{backend}/` directory
102112
2. Implement `Queue`, `Publisher`, `Subscriber`, `Delivery` interfaces
103113
3. Map `entityqueue.Message` to backend format
114+
4. Deduplicate publishes on (topic, partition key, message ID)
104115

105116
See `platform/extension/messagequeue/mysql/` for the reference implementation.
Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ load("@rules_go//go:def.bzl", "go_library", "go_test")
33
go_library(
44
name = "go_default_library",
55
srcs = ["publish.go"],
6-
importpath = "github.com/uber/submitqueue/submitqueue/core/publish",
6+
importpath = "github.com/uber/submitqueue/platform/publish",
77
visibility = ["//visibility:public"],
88
deps = [
99
"//platform/base/messagequeue:go_default_library",

platform/publish/publish.go

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
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 publish sends a message to the queue behind a topic key. It owns the
16+
// lookup-and-send plumbing every pipeline stage otherwise repeats — resolve the
17+
// key to a queue and a topic name, wrap the payload in a message, publish — and
18+
// the message-ID convention that controls deduplication (see IntentID).
19+
//
20+
// Every producer publishes through this package. Building a message anywhere
21+
// else would put the ID choice back at each call site, which is the mistake the
22+
// convention exists to prevent, so a linter restricts message construction to
23+
// here and to the queue backends.
24+
package publish
25+
26+
import (
27+
"context"
28+
"fmt"
29+
"strings"
30+
"sync/atomic"
31+
"time"
32+
33+
entityqueue "github.com/uber/submitqueue/platform/base/messagequeue"
34+
"github.com/uber/submitqueue/platform/consumer"
35+
)
36+
37+
// Message publishes payload to the topic registered for key.
38+
//
39+
// msgID selects the dedup behavior, so the caller must choose it deliberately.
40+
// The queue deduplicates on (topic, partition key, message ID) against every
41+
// row it has not garbage-collected yet, consumed ones included — a window with
42+
// no upper bound on a busy partition. A publish that collides is reported as a
43+
// success and writes nothing, and nothing retries it.
44+
//
45+
// Build msgID with IntentID: name the entity the message is about and the cause
46+
// this particular message exists for. A retry of the same cause then dedups,
47+
// which is what makes redelivery safe, while a new cause about the same entity
48+
// can never be swallowed by an older row.
49+
func Message(ctx context.Context, registry consumer.TopicRegistry, key consumer.TopicKey, msgID string, payload []byte, partitionKey string) error {
50+
q, ok := registry.Queue(key)
51+
if !ok {
52+
return fmt.Errorf("no queue registered for topic key %s", key)
53+
}
54+
topicName, ok := registry.TopicName(key)
55+
if !ok {
56+
return fmt.Errorf("no topic name registered for topic key %s", key)
57+
}
58+
59+
msg := entityqueue.NewMessage(msgID, payload, partitionKey, nil)
60+
return q.Publisher().Publish(ctx, topicName, msg)
61+
}
62+
63+
// IntentID names the occasion to publish rather than the entity published
64+
// about: entityID says what the message concerns, and cause says why this
65+
// particular message exists.
66+
//
67+
// Passing no cause asks for at-most-once delivery per entity — every later
68+
// publish about that entity is dropped while an earlier row survives. That is
69+
// right only for a hand-off that happens once in an entity's life, such as
70+
// announcing that it was created. Anything re-sent by design — a wake-up, a
71+
// poll, a re-dispatch, a dead-letter reconciliation — must name its cause, or
72+
// it collides with that one-shot publish and is lost.
73+
//
74+
// Each cause segment must be stable across redeliveries of one occurrence and
75+
// different between occurrences, so derive it from whatever provoked the
76+
// publish: the dependency that reached a terminal state, the build and the
77+
// status observed, the dead letter being reconciled. A wall-clock reading or a
78+
// random value satisfies "different" while destroying "stable", leaving every
79+
// redelivery to publish again. An empty segment carries no information and
80+
// makes two different occasions share an ID, so callers pass none.
81+
func IntentID(entityID string, cause ...string) string {
82+
if len(cause) == 0 {
83+
return entityID
84+
}
85+
return entityID + "/" + strings.Join(cause, "/")
86+
}
87+
88+
// sequence breaks ties between UniqueID calls that land on the same clock
89+
// tick: some platforms quantize time.Now coarsely enough for consecutive calls
90+
// to read the same nanosecond.
91+
var sequence atomic.Uint64
92+
93+
// UniqueID returns a message ID no earlier publish has used, so the publish
94+
// cannot be deduplicated away.
95+
//
96+
// This is the fallback for a cause with nothing stable to name it by, and it
97+
// costs the idempotency IntentID preserves: a redelivery mints a fresh ID and
98+
// publishes a second time, so the consumer has to absorb the duplicate. Prefer
99+
// IntentID wherever the cause can be identified.
100+
func UniqueID(id string) string {
101+
return fmt.Sprintf("%s@%d-%d", id, time.Now().UnixNano(), sequence.Add(1))
102+
}
Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,47 @@ func TestMessage_UnregisteredKey(t *testing.T) {
7070
require.Error(t, err)
7171
}
7272

73+
func TestIntentID(t *testing.T) {
74+
tests := []struct {
75+
name string
76+
entityID string
77+
cause []string
78+
want string
79+
}{
80+
{
81+
name: "no cause is the bare entity ID",
82+
entityID: "batch-1",
83+
want: "batch-1",
84+
},
85+
{
86+
name: "single cause",
87+
entityID: "batch-1",
88+
cause: []string{"merged"},
89+
want: "batch-1/merged",
90+
},
91+
{
92+
name: "multiple causes join in order",
93+
entityID: "batch-1",
94+
cause: []string{"build-signal", "build-9", "running"},
95+
want: "batch-1/build-signal/build-9/running",
96+
},
97+
}
98+
99+
for _, tt := range tests {
100+
t.Run(tt.name, func(t *testing.T) {
101+
assert.Equal(t, tt.want, IntentID(tt.entityID, tt.cause...))
102+
})
103+
}
104+
}
105+
106+
// The convention only works if the same cause is repeatable and a different
107+
// cause is distinguishable — the two properties every call site relies on.
108+
func TestIntentID_StableAcrossCallsAndDistinctPerCause(t *testing.T) {
109+
assert.Equal(t, IntentID("batch-1", "merged"), IntentID("batch-1", "merged"))
110+
assert.NotEqual(t, IntentID("batch-1", "merged"), IntentID("batch-1"))
111+
assert.NotEqual(t, IntentID("batch-1", "merged"), IntentID("batch-1", "cancelling"))
112+
}
113+
73114
func TestUniqueID(t *testing.T) {
74115
a := UniqueID("batch-1")
75116
b := UniqueID("batch-1")

runway/controller/dlq/BUILD.bazel

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,9 @@ go_library(
88
deps = [
99
"//api/runway/messagequeue:go_default_library",
1010
"//api/runway/messagequeue/protopb:go_default_library",
11-
"//platform/base/messagequeue:go_default_library",
1211
"//platform/consumer:go_default_library",
1312
"//platform/metrics:go_default_library",
13+
"//platform/publish:go_default_library",
1414
"@com_github_uber_go_tally//:go_default_library",
1515
"@org_uber_go_zap//:go_default_library",
1616
],

runway/controller/dlq/dlq.go

Lines changed: 8 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -43,9 +43,9 @@ import (
4343
"github.com/uber-go/tally"
4444
runwaymq "github.com/uber/submitqueue/api/runway/messagequeue"
4545
runwaypb "github.com/uber/submitqueue/api/runway/messagequeue/protopb"
46-
entityqueue "github.com/uber/submitqueue/platform/base/messagequeue"
4746
"github.com/uber/submitqueue/platform/consumer"
4847
"github.com/uber/submitqueue/platform/metrics"
48+
"github.com/uber/submitqueue/platform/publish"
4949
"go.uber.org/zap"
5050
)
5151

@@ -153,25 +153,19 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er
153153
}
154154

155155
// publish serializes a MergeResult and publishes it to the signal topic.
156+
//
157+
// Named for the dead letter, because the live handler answers the same request
158+
// on the same topic under the bare correlation ID. Reusing that ID would let
159+
// this terminal failure be deduplicated against an answer that was already
160+
// sent, and the caller would go on waiting for a result nothing will produce.
156161
func (c *Controller) publish(ctx context.Context, result *runwaymq.MergeResult, partitionKey string) error {
157162
payload, err := runwaymq.Marshal(result)
158163
if err != nil {
159164
return fmt.Errorf("failed to serialize merge result: %w", err)
160165
}
161166

162-
msg := entityqueue.NewMessage(result.GetId(), payload, partitionKey, nil)
163-
164-
q, ok := c.registry.Queue(c.signalTopicKey)
165-
if !ok {
166-
return fmt.Errorf("no queue registered for topic key %s", c.signalTopicKey)
167-
}
168-
169-
topicName, ok := c.registry.TopicName(c.signalTopicKey)
170-
if !ok {
171-
return fmt.Errorf("no topic name registered for topic key %s", c.signalTopicKey)
172-
}
173-
174-
if err := q.Publisher().Publish(ctx, topicName, msg); err != nil {
167+
if err := publish.Message(ctx, c.registry, c.signalTopicKey,
168+
publish.IntentID(result.GetId(), "dlq"), payload, partitionKey); err != nil {
175169
return fmt.Errorf("failed to publish message: %w", err)
176170
}
177171

runway/controller/merge/BUILD.bazel

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,9 @@ go_library(
88
deps = [
99
"//api/runway/messagequeue:go_default_library",
1010
"//api/runway/messagequeue/protopb:go_default_library",
11-
"//platform/base/messagequeue:go_default_library",
1211
"//platform/consumer:go_default_library",
1312
"//platform/metrics:go_default_library",
13+
"//platform/publish:go_default_library",
1414
"//runway/extension/merger:go_default_library",
1515
"@com_github_uber_go_tally//:go_default_library",
1616
"@org_uber_go_zap//:go_default_library",

runway/controller/merge/merge.go

Lines changed: 8 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -30,9 +30,9 @@ import (
3030
"github.com/uber-go/tally"
3131
runwaymq "github.com/uber/submitqueue/api/runway/messagequeue"
3232
runwaypb "github.com/uber/submitqueue/api/runway/messagequeue/protopb"
33-
entityqueue "github.com/uber/submitqueue/platform/base/messagequeue"
3433
"github.com/uber/submitqueue/platform/consumer"
3534
"github.com/uber/submitqueue/platform/metrics"
35+
"github.com/uber/submitqueue/platform/publish"
3636
"github.com/uber/submitqueue/runway/extension/merger"
3737
"go.uber.org/zap"
3838
)
@@ -141,25 +141,19 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er
141141
}
142142

143143
// publish serializes a MergeResult and publishes it to the given signal topic.
144+
//
145+
// The message ID is the correlation ID with no cause: a request is answered
146+
// once, so a redelivery that re-answers it is meant to dedup rather than tell
147+
// the caller twice. The dead-letter path answers the same request when this
148+
// one never could, and names itself so it cannot be mistaken for a repeat of
149+
// this answer.
144150
func (c *Controller) publish(ctx context.Context, key consumer.TopicKey, result *runwaymq.MergeResult, partitionKey string) error {
145151
payload, err := runwaymq.Marshal(result)
146152
if err != nil {
147153
return fmt.Errorf("failed to serialize merge result: %w", err)
148154
}
149155

150-
msg := entityqueue.NewMessage(result.GetId(), payload, partitionKey, nil)
151-
152-
q, ok := c.registry.Queue(key)
153-
if !ok {
154-
return fmt.Errorf("no queue registered for topic key %s", key)
155-
}
156-
157-
topicName, ok := c.registry.TopicName(key)
158-
if !ok {
159-
return fmt.Errorf("no topic name registered for topic key %s", key)
160-
}
161-
162-
if err := q.Publisher().Publish(ctx, topicName, msg); err != nil {
156+
if err := publish.Message(ctx, c.registry, key, publish.IntentID(result.GetId()), payload, partitionKey); err != nil {
163157
return fmt.Errorf("failed to publish message: %w", err)
164158
}
165159

0 commit comments

Comments
 (0)