fix(speculate): wait for every assumption to settle before merging - #559
Merged
Conversation
behinddwalls
marked this pull request as ready for review
August 10, 2026 18:40
behinddwalls
marked this pull request as draft
August 10, 2026 18:46
behinddwalls
marked this pull request as ready for review
August 10, 2026 19:04
mnoah1
approved these changes
Aug 10, 2026
## Summary ### Why? A head could merge on an assumption that had not come true. `mergeablePath` required every dependency a path assumed would *succeed* to have actually merged, but imposed no wait at all on one it assumed would *fail*. The doc comment stated the reasoning: *"A dependency assumed to fail imposes no wait: the path is broken the moment that dependency succeeds, so a still-live path has already been vindicated on it."* That holds only if the transition were instantaneous. It is not — a dependency spends time in `speculating`, and then in `merging`, having neither succeeded nor failed. `assumptionBroken` only fires on a terminal state, so throughout that window the path is neither broken nor vindicated. It is unsettled, and the gate read unsettled as permission. A path that assumed a dependency would fail was built *without* that dependency's changes. Landing the head while that dependency is still live, and watching it land too, puts a combination on the trunk that no build ever validated — the one thing the queue exists to prevent. It needs no textual conflict to break the trunk, because the two changes were never built together. Measured against the real predicates before fixing, with a passed `fails(D)` path and only D's state varying: | D's state | `mergeablePath` | correct? | | -- | -- | -- | | `speculating` | true | no | | `merging` | true | no | | `cancelling` | true | no | | `failed` | true | yes — the assumption came true | | `succeeded` | false | yes — `assumptionBroken` catches it | `decide` returned `merge` in all three of the wrong rows. ### What? The rule is now symmetric: a path may merge once every dependency has finished the way it assumed. `succeeds` needs `Succeeded`; `fails` needs `Failed` or `Cancelled`. `allAssumedSucceedingMerged` becomes `allAssumptionsSettled`, since it no longer looks only at succeeding dependencies. One wording consequence of the parent commit dropping the *ignored* assumption: the old doc sold this gate as "the head waits only on the dependencies it was built on top of, not its full dependency list". With every dependency carrying a position, that is no longer what speculation buys. What it buys is that the build already ran — when the dependencies land the way the path guessed there is nothing left to execute, and the head merges at once. Note this is not the "bypass large diff" early merge the RFC describes. That reads a passed path for *every* combination of the dependencies, and is not implemented on the controller side — nothing enumerates combinations. A single path betting the right way was never a sound approximation of it, and `speculation.md` now says so rather than describing behaviour the code does not have. One liveness consequence, recorded on CODEM-428 rather than fixed here: a dependency stuck in `Cancelling` now stalls its dependents too, not just itself. `finalizeCancellations` converges `Cancelling → Cancelled` on any subsequent run, so this only bites when no further run is triggered — which is that issue's edge-triggering gap. ## Test Plan - ✅ `make test` — 96/96 pass - ✅ `make lint`, `make check-gazelle`, `make check-tidy` `TestMergeablePath` is rebuilt around the new rule, holding the second dependency settled so the first is the only variable: a fails assumption waits out `speculating`, `merging` and `cancelling`, and merges on `Failed` or `Cancelled`; an assumed-succeeding dependency waits out its merge; one unsettled dependency is enough to wait. ## Issue Fixes https://linear.app/uber/issue/CODEM-429
mnoah1
force-pushed
the
preetam/speculate-merge-gate
branch
from
August 10, 2026 20:12
d35e3e7 to
f81e381
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Why?
A head could merge on an assumption that had not come true.
mergeablePathrequired every dependency a path assumed would succeed to have actually merged, but imposed no wait at all on one it assumed would fail.The doc comment stated the reasoning: "A dependency assumed to fail imposes no wait: the path is broken the moment that dependency succeeds, so a still-live path has already been vindicated on it." That holds only if the transition were instantaneous. It is not — a dependency spends time in
speculating, and then inmerging, having neither succeeded nor failed.assumptionBrokenonly fires on a terminal state, so throughout that window the path is neither broken nor vindicated. It is unsettled, and the gate read unsettled as permission.A path that assumed a dependency would fail was built without that dependency's changes. Landing the head while that dependency is still live, and watching it land too, puts a combination on the trunk that no build ever validated — the one thing the queue exists to prevent. It needs no textual conflict to break the trunk, because the two changes were never built together.
Measured against the real predicates before fixing, with a passed
fails(D)path and only D's state varying:mergeablePathspeculatingmergingcancellingfailedsucceededassumptionBrokencatches itdecidereturnedmergein all three of the wrong rows.What?
The rule is now symmetric: a path may merge once every dependency has finished the way it assumed.
succeedsneedsSucceeded;failsneedsFailedorCancelled.allAssumedSucceedingMergedbecomesallAssumptionsSettled, since it no longer looks only at succeeding dependencies.One wording consequence of the parent commit dropping the ignored assumption: the old doc sold this gate as "the head waits only on the dependencies it was built on top of, not its full dependency list". With every dependency carrying a position, that is no longer what speculation buys. What it buys is that the build already ran — when the dependencies land the way the path guessed there is nothing left to execute, and the head merges at once.
Note this is not the "bypass large diff" early merge the RFC describes. That reads a passed path for every combination of the dependencies, and is not implemented on the controller side — nothing enumerates combinations. A single path betting the right way was never a sound approximation of it, and
speculation.mdnow says so rather than describing behaviour the code does not have.One liveness consequence, recorded on CODEM-428 rather than fixed here: a dependency stuck in
Cancellingnow stalls its dependents too, not just itself.finalizeCancellationsconvergesCancelling → Cancelledon any subsequent run, so this only bites when no further run is triggered — which is that issue's edge-triggering gap.Test Plan
make test— 96/96 passmake lint,make check-gazelle,make check-tidyTestMergeablePathis rebuilt around the new rule, holding the second dependency settled so the first is the only variable: a fails assumption waits outspeculating,mergingandcancelling, and merges onFailedorCancelled; an assumed-succeeding dependency waits out its merge; one unsettled dependency is enough to wait.Issue
Fixes https://linear.app/uber/issue/CODEM-429