Skip to content

fix(messagequeue): intent-scoped message IDs so wake-ups stop vanishing - #574

Merged
behinddwalls merged 1 commit into
mainfrom
messagequeue
Aug 12, 2026
Merged

fix(messagequeue): intent-scoped message IDs so wake-ups stop vanishing#574
behinddwalls merged 1 commit into
mainfrom
messagequeue

Conversation

@behinddwalls

@behinddwalls behinddwalls commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

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

Issues

Stack

  1. @ fix(messagequeue): intent-scoped message IDs so wake-ups stop vanishing #574
  2. test(e2e): prove a dependent batch is woken by the merge ahead of it #576

@behinddwalls
behinddwalls marked this pull request as ready for review August 12, 2026 03:03
@behinddwalls
behinddwalls requested review from a team and sbalabanov as code owners August 12, 2026 03:03
@behinddwalls
behinddwalls marked this pull request as draft August 12, 2026 03:21
@behinddwalls
behinddwalls marked this pull request as ready for review August 12, 2026 03:43
Comment thread submitqueue/orchestrator/controller/speculate/speculate.go
## 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
@behinddwalls
behinddwalls added this pull request to the merge queue Aug 12, 2026
Merged via the queue into main with commit e21ab03 Aug 12, 2026
15 checks passed
@behinddwalls
behinddwalls deployed to stack-rebase August 12, 2026 19:00 — with GitHub Actions Active
@behinddwalls
behinddwalls deleted the messagequeue branch August 12, 2026 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

messagequeue: entity-ID message IDs + publish dedup silently drop wake-up events; use intent-scoped message IDs

2 participants