Skip to content

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

Open
behinddwalls wants to merge 3 commits into
mainfrom
preetam/codem-443-speculation-events
Open

feat(speculate): hold speculating until the batch can be sent to merge#586
behinddwalls wants to merge 3 commits into
mainfrom
preetam/codem-443-speculation-events

Conversation

@behinddwalls

@behinddwalls behinddwalls commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

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.

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

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. fix(speculate): write the merge state before dispatching the batch #585
  2. @ feat(speculate): hold speculating until the batch can be sent to merge #586

## Summary

### Why?

`applyOutcome` published a batch to the merge topic before writing `BatchStateMerging`, so a lost compare-and-swap could leave Runway acting on an outcome that was never recorded. That ordering existed to avoid a stall, and the stall is real: nothing re-drives a batch stuck in `Merging`. `Process` self-heals only terminal and `Created` batches, `finalize` walks only heads that are still speculating, and the sole production reader of `BatchStateMerging` is the cancel controller — so a batch written `Merging` whose dispatch never went out would sit there forever.

Giving that stall a repair path lets the write come first, which is the ordering the rest of the state machine already wants.

### What?

`applyOutcome` is restructured into decide-state → recover → write → dispatch. The `terminal` bool falls out: a second switch mirrors the first and dispatches merge or conclude once the state write has landed.

`recoverable` is hoisted above the switch so a cascade-decided *merge* gets a recovery message too, not just a cascade-decided failure. It needs one for the same reason: the write drops the batch out of the speculating set, and `Process`'s self-heal only ever names the trigger batch.

`Process` gains a `BatchStateMerging` branch that re-sends the dispatch through the new `dispatchMerge` helper. That keeps the stable `IntentID`, the inverse of `fanout`'s `UniqueID` — for conclude a stable ID would suppress the repair, for merge it is what stops Runway merging the batch twice.

One side benefit: a lost state CAS now means the dispatch is never sent at all, narrowing the window where a cancelled batch has a live merge request against it.

## Test Plan

✅ `bazel test //submitqueue/... //platform/...` — 68 tests pass

New coverage: the dispatch follows the state write; a lost CAS publishes nothing; a cascade-merged batch gets its recovery signal before the write; `Process` on a `Merging` batch re-dispatches; `dispatchMerge` reuses one message ID per batch.

`TestProcess_MergingRunsButDoesNotAct` asserted the old behaviour — that a `Merging` batch publishes nothing — and is replaced by `TestProcess_MergingSelfHeals`.
## 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.

## Test Plan

✅ `bazel test //submitqueue/... //platform/...` — 68 tests pass

The two `reportSpeculation` tests now assert events. New coverage: a merging head reports `speculated` and no wait — the gate's regression test — and `speculated` is published before the merge dispatch.

`test/e2e/submitqueue/suite_test.go` 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
## Summary

### Why?

Nothing in `test/integration/` touches the speculate pipeline — the orchestrator integration suite is `TestPingAPI` and nothing else — so the only end-to-end coverage was the happy path, which has no dependencies and therefore never speculates across one. The events this stack introduces had unit coverage only.

### What?

`e2e-respeculate-queue` is registered in the gateway's queue list. It 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 e2e 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 then 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.

Two harness helpers come with it: `awaitEvent`, since an event is never a current status and the history is its only witness, and `assertStatusCount`, which is what pins the no-oscillation property the status change is for.

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.

`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.

## Test Plan

✅ `bazel test //submitqueue/... //platform/... //service/...` — 73 tests pass
✅ `bazel test //test/e2e/...` — 3/3 pass, including the new scenario

# Conflicts:
#	service/submitqueue/gateway/server/queues.yaml
#	test/e2e/submitqueue/harness_test.go

# Please enter the commit message for your changes. Lines starting
# with '#' will be kept; you may remove them yourself if you want to.
# An empty message aborts the commit.
#
# interactive rebase in progress; onto bcea46e
# Last commands done (2 commands done):
#    pick e407612 # feat(speculate): hold speculating until the batch can be sent to merge
#    pick 14839e5 # test(speculate): cover speculation across an unresolved dependency
# No commands remaining.
# You are currently rebasing branch 'preetam/codem-443-speculation-events' on 'bcea46ec'.
#
# Changes to be committed:
#	modified:   service/submitqueue/gateway/server/queues.yaml
#	modified:   submitqueue/orchestrator/controller/speculate/run_test.go
#	modified:   test/e2e/submitqueue/harness_test.go
#	modified:   test/e2e/submitqueue/suite_test.go
#
@behinddwalls
behinddwalls force-pushed the preetam/codem-443-speculation-events branch from 14839e5 to 0f79d0b Compare August 13, 2026 19:06
Base automatically changed from preetam/speculate-write-before-dispatch to main August 13, 2026 20:23
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.

2 participants