Skip to content

fix(speculate): wait for every assumption to settle before merging - #559

Merged
mnoah1 merged 1 commit into
mainfrom
preetam/speculate-merge-gate
Aug 10, 2026
Merged

fix(speculate): wait for every assumption to settle before merging#559
mnoah1 merged 1 commit into
mainfrom
preetam/speculate-merge-gate

Conversation

@behinddwalls

Copy link
Copy Markdown
Collaborator

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

@behinddwalls
behinddwalls marked this pull request as ready for review August 10, 2026 18:40
@behinddwalls
behinddwalls requested review from a team and sbalabanov as code owners August 10, 2026 18:40
@behinddwalls
behinddwalls marked this pull request as draft August 10, 2026 18:46
@behinddwalls
behinddwalls marked this pull request as ready for review August 10, 2026 19:04
Base automatically changed from preetam/drop-ignored-assumption to main August 10, 2026 20:12
## 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
mnoah1 force-pushed the preetam/speculate-merge-gate branch from d35e3e7 to f81e381 Compare August 10, 2026 20:12
@mnoah1
mnoah1 added this pull request to the merge queue Aug 10, 2026
Merged via the queue into main with commit b8caaab Aug 10, 2026
15 checks passed
@mnoah1
mnoah1 deployed to stack-rebase August 10, 2026 23:01 — with GitHub Actions Active
@behinddwalls
behinddwalls deleted the preetam/speculate-merge-gate branch August 10, 2026 23:01
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