test(e2e): prove a dependent batch is woken by the merge ahead of it by behinddwalls · Pull Request #576 · uber/submitqueue

behinddwalls · 2026-08-12T03:20:17Z

Summary

Why?

The parent change fixes a dropped wake-up: a merged batch fans out to speculate so its dependents can re-plan, and that message used to reuse the bare batch ID the batch controller had already published to the same topic and partition at creation. The queue deduplicates against rows it has not collected yet, consumed ones included, so the fan-out was reported as a success, stored nothing, and never arrived.

That fix shipped with unit coverage on the message ID and integration coverage on the queue semantics, but nothing exercised the path the bug actually broke. It is also a path that hides easily: any other event re-plans the queue and moves the dependent along, so a naive two-request test passes with or without the fix.

What?

A new e2e case isolates the fan-out as the only possible wake-up, following the stop → observe → start shape TestCancel_CaughtPreBatch_NeverLands already uses:

Close the runway-merge gate for the queue before landing, so the lead batch cannot complete its merge.
Land the lead; wait for its merge to park, keyed by the lead's batch ID.
Land the dependent. Its batch serializes behind the lead's, which is in-flight (Merging is a dependency state).
Wait for the dependent to reach speculated — its speculative build has already passed, so its own build signals are finished and nothing else will wake it.
Open the gate. The lead merges and fans out.

The dependent reaching landed is then attributable to the fan-out alone.

Supporting changes: e2e-chain-queue is registered in queues.yaml. It is deliberately absent from the orchestrator's per-queue profiles so it falls through to the baseline profile and its all conflict analyzer, which serializes every new batch behind every in-flight one — that is what builds the chain. A new awaitBatchID harness helper resolves a request's batch ID from the operating store, since merge messages are keyed by batch rather than by the sqid a test holds.

Test Plan

✅ bazel test //test/e2e/... — 3 suites, including the new case (~33s)
✅ make lint-license, make lint-message-id, make lint-queue-shard
Confirmed the test fails against the unfixed code. Reverting the parent's mergesignal message ID to the bare batch ID leaves the dependent stuck at speculated and the suite runs to Bazel's timeout (TIMEOUT in 240.3s with --test_timeout=240); with the fix it passes in 33s.

A stalled pipeline surfaces as a test timeout rather than an assertion failure, which is how this harness reports non-convergence — pollUntil has no deadline of its own by design, so Bazel's timeout is the only one.

Stack

## Summary ### Why? The parent change fixes a dropped wake-up: a merged batch fans out to speculate so its dependents can re-plan, and that message used to reuse the bare batch ID the batch controller had already published to the same topic and partition at creation. The queue deduplicates against rows it has not collected yet, consumed ones included, so the fan-out was reported as a success, stored nothing, and never arrived. That fix shipped with unit coverage on the message ID and integration coverage on the queue semantics, but nothing exercised the path the bug actually broke. It is also a path that hides easily: any other event re-plans the queue and moves the dependent along, so a naive two-request test passes with or without the fix. ### What? A new e2e case isolates the fan-out as the only possible wake-up, following the stop → observe → start shape `TestCancel_CaughtPreBatch_NeverLands` already uses: 1. Close the `runway-merge` gate for the queue before landing, so the lead batch cannot complete its merge. 2. Land the lead; wait for its merge to park, keyed by the lead's batch ID. 3. Land the dependent. Its batch serializes behind the lead's, which is in-flight (`Merging` is a dependency state). 4. Wait for the dependent to reach `speculated` — its speculative build has already passed, so its own build signals are finished and nothing else will wake it. 5. Open the gate. The lead merges and fans out. The dependent reaching `landed` is then attributable to the fan-out alone. Supporting changes: `e2e-chain-queue` is registered in `queues.yaml`. It is deliberately absent from the orchestrator's per-queue profiles so it falls through to the baseline profile and its `all` conflict analyzer, which serializes every new batch behind every in-flight one — that is what builds the chain. A new `awaitBatchID` harness helper resolves a request's batch ID from the operating store, since merge messages are keyed by batch rather than by the sqid a test holds. ## Test Plan - ✅ `bazel test //test/e2e/...` — 3 suites, including the new case (~33s) - ✅ `make lint-license`, `make lint-message-id`, `make lint-queue-shard` - **Confirmed the test fails against the unfixed code.** Reverting the parent's `mergesignal` message ID to the bare batch ID leaves the dependent stuck at `speculated` and the suite runs to Bazel's timeout (`TIMEOUT in 240.3s` with `--test_timeout=240`); with the fix it passes in 33s. A stalled pipeline surfaces as a test timeout rather than an assertion failure, which is how this harness reports non-convergence — `pollUntil` has no deadline of its own by design, so Bazel's timeout is the only one.

## Summary ### Why? `TestDependentBatch_IsWokenByTheMergeAhead` parks the lead batch's merge behind a closed gate, waits for the dependent to reach `speculated`, and only then opens the gate. That ordering encodes the old meaning of `speculated` — a build passed on a path still consistent with how its dependencies are resolving — which a batch could reach while its dependency was still outstanding. Holding `speculating` until the batch can be sent to merge removes that resting point. A dependent blocked on the parked lead now stays `speculating`, and `speculated` arrives only once the lead has merged and the dependent is itself cleared to merge. So the test waits for a status that cannot arrive until it opens the gate, and it does not open the gate until that status arrives. The suite runs to Bazel's timeout. The two changes had not met before: #576 landed on main after this branch was cut, so CI had never run them together. ### What? The observation step waits for the `waiting` event instead of the `speculated` status. It is the same fact the test was reaching for — the dependent has passed its own build and only the lead is outstanding — expressed as the signal that now carries it, and reachable while the lead is still parked. Nothing else moves. The gate still opens next, and the lead and the dependent are still asserted to land, so the dependent's wake-up remains attributable to the fan-out alone. ## Test Plan ✅ `bazel test //test/e2e/submitqueue:go_default_test` — passes in 120s, against a 300s timeout before ✅ `bazel test //submitqueue/... //platform/...` — 69 tests pass

…ng (uber#574) ## 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 uber#352 ## Issues ## Stack 1. @ uber#574 1. uber#576

uber#586) ## Summary ### Why? A request's trail read `batched → speculating → speculated → speculating → speculated → landing → landed`, and the repeats looked like the pipeline regressing. They were not a reporting glitch: `RequestStatusSpeculated` meant "a build passed on a path still consistent with how its dependencies are resolving", so it was published while the batch was still blocked, and `reportSpeculation` republished `speculating` whenever a dependency later resolved against that path's guess. Each extra pair was one speculative guess that passed and was then invalidated. That made `speculated` a per-path, provisional fact wearing a status — the exact shape `RequestEvent` exists for. A batch is not done speculating until it can be sent to merge; waiting on dependencies is still speculating. ### What? Two events join the vocabulary. `waiting` records that a path passed and the batch has nothing of its own left to run; `invalidated` records that a dependency resolved against the guess that path made. Both are occurrence-keyed on the path ID, so a passed path re-observed across runs collapses to one entry. `waiting` is gated on `outcomeWait` rather than on merely holding a live passed path. A merge is decided on that same predicate — `mergeablePath` implies `livePassedPath` — so an ungated report would claim a wait on every request that merges straight through. `reportSpeculation` moves below `decide` to see the outcome; both it and `decide` only read, so the reorder observes nothing different. `speculated` stays a status but now means speculation finished, published from `dispatchMerge` once the batch is cleared to merge. It goes ahead of the dispatch because the merge stage publishes `landing` as its first act on receiving one, and both statuses are non-terminal — so a `speculated` sent afterwards could carry the later timestamp and beat `landing` in the summary. The `hadPassed && !hasPassed` republish of `speculating` is gone. The status never leaves, so there is nothing to republish, and the oscillation goes with it. One trade-off worth naming: `speculated` is now near-instantaneous, so "is this batch blocked on dependencies?" is answerable from the latest event rather than from the status. The second commit adds the end-to-end coverage this had been missing. Nothing in `test/integration/` touches the speculate pipeline — the orchestrator integration suite is `TestPingAPI` and nothing else — and the e2e happy path has no dependencies, so it never speculates across one. `e2e-respeculate-queue` is registered in the gateway's queue list and deliberately takes no profile of its own: falling through to the baseline is what gives it the `all` analyzer, which serializes the queue so a second request becomes a batch depending on the first. The new test forces the wait rather than racing it. Batch IDs come from a per-queue counter as `<queue>/batch/<n>`, so on a fresh queue the leader is `batch/1`, and the build topic partitions by batch — closing the consumer gate on that partition before anything is published holds the leader's build and nothing else. The follower reaches a passed path while its dependency is still outstanding, reports `waiting`, and is asserted to still be `speculating`. Releasing the gate fails the leader, and the follower re-plans and lands with `speculating` and `speculated` recorded exactly once each. `invalidated` is deliberately not asserted end to end. A passed path stops occupying build budget, so by the time the leader fails the follower has usually funded the other side of the guess as well; it never loses its last live passed path, which is the state `invalidated` reports. Forcing that end to end would mean starving the queue's budget, which cannot be done without also starving the follower's first build. A unit test covers the case e2e cannot reach deterministically: a dependency turning terminal in the same run that walks the head resting on it, so the break is seen by a later generation of the finalize loop rather than by the read. A third commit adapts `TestDependentBatch_IsWokenByTheMergeAhead`, which uber#576 added to main after this branch was cut. It parks the lead batch's merge behind a closed gate, waits for the dependent to reach `speculated`, and only then opens the gate — an ordering that encodes the old meaning of `speculated`, which a batch could reach while its dependency was still outstanding. Under this change the dependent rests at `speculating` instead, so the test waited for a status that could not arrive until it opened the gate, and did not open the gate until it arrived. It now waits for the `waiting` event: the same fact it was reaching for, expressed as the signal that carries it, and reachable while the lead is still parked. The gate still opens next and both requests are still asserted to land, so the wake-up remains attributable to the fan-out alone. ## Test Plan ✅ `bazel test //submitqueue/... //platform/... //service/...` — 73 tests pass ✅ `bazel test //test/e2e/...` — 3/3 pass, including the new scenario ✅ `make lint`, `make check-gazelle`, `make check-tidy`, `make check-mocks` Re-verified after rebasing onto main at `42d1cb72`: `bazel test //submitqueue/... //platform/...` — 69 tests pass; `bazel test //test/e2e/submitqueue` — passes in 120s, against the 300s timeout it hit before the third commit. The two `reportSpeculation` tests now assert events. New unit coverage: a merging head reports `speculated` and no wait (the gate's regression test); `speculated` is published before the `TopicKeyMerge` dispatch; and the same-run cascade reports `invalidated`. `TestLand_HappyPath_ReachesLanded` needs no change: `speculating → speculated → landing → landed` still holds as an ordered subsequence, now for a different reason and at a different point in time. ## Issue Closes https://linear.app/uber/issue/CODEM-443 ## Stack 1. uber#585 1. @ uber#586

behinddwalls mentioned this pull request Aug 12, 2026

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

Merged

behinddwalls marked this pull request as ready for review August 12, 2026 03:43

behinddwalls requested review from a team and sbalabanov as code owners August 12, 2026 03:43

mnoah1 approved these changes Aug 12, 2026

View reviewed changes

behinddwalls force-pushed the preetam/messagequeue-e2e-dependent-wake branch from dc085b9 to bcb260d Compare August 12, 2026 18:44

Base automatically changed from messagequeue to main August 12, 2026 19:00

behinddwalls force-pushed the preetam/messagequeue-e2e-dependent-wake branch from bcb260d to 20e0c3d Compare August 12, 2026 19:00

behinddwalls added this pull request to the merge queue Aug 12, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 12, 2026

behinddwalls added this pull request to the merge queue Aug 13, 2026

Merged via the queue into main with commit 28e952b Aug 13, 2026
15 checks passed

behinddwalls deployed to stack-rebase August 13, 2026 17:04 — with GitHub Actions Active

behinddwalls deleted the preetam/messagequeue-e2e-dependent-wake branch August 13, 2026 17:04

behinddwalls mentioned this pull request Aug 13, 2026

feat(speculate): hold speculating until the batch can be sent to merge #586

Merged

Navigation Menu

Uh oh!

test(e2e): prove a dependent batch is woken by the merge ahead of it - #576

test(e2e): prove a dependent batch is woken by the merge ahead of it#576
behinddwalls merged 1 commit into
mainfrom
preetam/messagequeue-e2e-dependent-wake

behinddwalls commented Aug 12, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

behinddwalls commented Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Why?

What?

Test Plan

Stack

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

behinddwalls commented Aug 12, 2026 •

edited

Loading