fix(speculation): stop one dependency failing the whole snapshot - #554
Closed
behinddwalls wants to merge 1 commit into
Closed
fix(speculation): stop one dependency failing the whole snapshot#554behinddwalls wants to merge 1 commit into
behinddwalls wants to merge 1 commit into
Conversation
behinddwalls
force-pushed
the
preetam/codem-424-lazy-dependency-scoring
branch
6 times, most recently
from
August 10, 2026 18:01
99ced59 to
2991653
Compare
behinddwalls
force-pushed
the
preetam/codem-424-lazy-dependency-scoring
branch
2 times, most recently
from
August 10, 2026 18:30
9228d86 to
597b5f8
Compare
behinddwalls
changed the base branch from
main
to
preetam/speculate-merge-gate
August 10, 2026 18:34
behinddwalls
changed the base branch from
preetam/speculate-merge-gate
to
main
August 10, 2026 18:35
behinddwalls
changed the base branch from
main
to
preetam/speculate-merge-gate
August 10, 2026 18:37
behinddwalls
force-pushed
the
preetam/codem-424-lazy-dependency-scoring
branch
3 times, most recently
from
August 10, 2026 18:57
23af7ae to
96e0660
Compare
behinddwalls
marked this pull request as ready for review
August 10, 2026 19:04
mnoah1
force-pushed
the
preetam/codem-424-lazy-dependency-scoring
branch
from
August 10, 2026 20:12
96e0660 to
14c72c8
Compare
mnoah1
approved these changes
Aug 10, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to invalid changes in the merge commit
Aug 10, 2026
## Summary ### Why? Speculation died for an entire queue whenever any batch depended on one that had reached `merging`: ``` speculator failed for queue demo-queue: score dependency "demo-queue/batch/1": failed to resolve storage for queue "": queue name must not be empty ``` The root cause is a caller contract violation, not a generator bug. `speculator.Speculate` documents `batches` as "every in-flight batch of the queue, plus any finalized batch still referenced as a dependency by an in-flight one". `ask` was passing `snap.speculating` — the speculating heads alone. `read` already assembles the right set in `snap.batches`; it simply was not the slice handed over. A merging dependency was therefore absent from the generator's index, `batchByID[id]` returned a zero `entity.Batch`, and that zero batch reached the scorer with an empty `Queue`. The blast radius was the whole queue rather than one batch, because `Generate` scores every unresolved dependency up front to seed its heap and returned on the first error. `standard.Speculate` then short-circuited before the allocator pulled a single candidate: no path failed, none was ever produced, and the fall-back-to-the-next-path machinery sits downstream of a stage that had already died. A measured run of 20 requests left 9 in `error` and 11 wedged in `speculating` with nothing recorded against them. ### What? **Hand the Speculator the whole queue.** `ask` now flattens `snap.batches` — every batch the run read — and passes that. No ordering is imposed: the generator's heap comparator is a strict total order, so its candidate sequence is identical whatever order the input arrives in (checked over 200 shuffles), and a Speculator that read meaning into input order would be relying on something the contract never offered. `ask`'s doc comment argued for the narrow slice and is rewritten. Widening is safe because of the commit below this one. A head this run has just decided still reads as `Speculating` in the snapshot — `finalize` records only terminal outcomes — and both the Speculator and `check` read head eligibility off that same map, so a stale entry would fool both filters at once. What stops it mattering is that a head now merges only once *every* dependency has settled: the generator pins them all, can therefore construct nothing but the path that already passed, and the allocator skips that as finished. Verified by driving the real `bestfirst`/`sticky` pair with a merged head and settled dependencies — zero actions proposed for it. That ordering is load-bearing, which is why the merge gate is the parent rather than a follow-up. With the old gate a head could merge past a dependency that was still live; the generator would then see that dependency as an open question, offer a path ID the set had never held, and the allocator would fund a fresh build for a batch already handed to Runway. **Make one unpriceable dependency cost only its own estimate.** `score` now substitutes `defaultProbability` when the scorer returns an error, and never calls the scorer at all for a dependency the snapshot did not carry — that batch is zero in every field, so scoring it would price some other batch entirely or fail on its empty queue name. Context cancellation is still fatal, including when it surfaces *as* the scorer's error: the loop checks `ctx` before each call, so a context that dies during the last one would otherwise be absorbed as an unpriceable dependency and hand back an iterator to a caller that has already gone. Scorer failures stay observable through the scorer's own metrics span, which already reports them via `op.Complete(retErr)`. That is the whole containment fix. An earlier revision of this branch also made scoring lazy — heads seeded at an optimistic bound and priced on first pull — and it has been dropped. The only admissible bound for an unpriced head is `log 1`, identical for every head, so the first pull priced the entire queue anyway; the laziness bought one narrow case (a run that pulls nothing because the budget is saturated) in exchange for a placeholder, an admissibility argument, priced and unpriced items sharing a heap, and five reworked tests. Defaulting on failure fixes the bug on its own. **`Merging` is left as an open question in the generator.** Tempting to pin it to *succeeds* — the batch looks committed to landing — but a merge can fail, so nothing is settled, and it would put a state-specific policy inside the search when whether a path betting against a merging batch is worth funding is a question of price that belongs to the scorer. The allocator already draws exactly this line — "no batch state enters this decision — `merging` and the rest are states of a batch, never of a path" — and the generator holds it too. ## Test Plan - ✅ `make test` — 96/96 pass - ✅ `make lint`, `make check-gazelle`, `make check-tidy` New and reworked coverage, per defect: - `TestRun_PassesSnapshotToSpeculator` — the Speculator receives every batch the run read, in a stable order - `TestBestFirst_AbsorbsScorerError` — a scorer error costs that dependency its estimate and nothing else - `TestBestFirst_NeverScoresAnAbsentDependency` — an absent dependency never reaches the scorer, even when the caller hands over a malformed snapshot - `TestBestFirst_MergingDependencyStaysOpen` — a merging dependency is priced like any other and keeps both sides - `TestBestFirst_HonorsCancelledContext/a scorer that fails on a dead context ends the run` The last was added for a defect a review of this branch turned up, and was confirmed to fail against the code as it stood before the fix. Not verified end to end: `make demo-pr` lives on the `sq/demo-pr` branch, so reproducing the original 20-request run needs that target ported across worktrees plus a Docker stack. ## Issue Fixes https://linear.app/uber/issue/CODEM-424 That issue proposed lazy scoring as its primary fix; this lands the containment it was after without the algorithm change, for the reasons above. Follow-up filed as https://linear.app/uber/issue/CODEM-428 — this stops queues wedging this way, but a queue already wedged still has no event that will wake it, because speculation is edge-triggered only. The merge-gate defect found while investigating this one — a head could merge on a *fails* assumption that had not come true — is the parent commit, since the widening here relies on the invariant it restores.
mnoah1
force-pushed
the
preetam/codem-424-lazy-dependency-scoring
branch
from
August 10, 2026 23:01
14c72c8 to
f06eaed
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?
Speculation died for an entire queue whenever any batch depended on one that had reached
merging:The root cause is a caller contract violation, not a generator bug.
speculator.Speculatedocumentsbatchesas "every in-flight batch of the queue, plus any finalized batch still referenced as a dependency by an in-flight one".askwas passingsnap.speculating— the speculating heads alone.readalready assembles the right set insnap.batches; it simply was not the slice handed over. A merging dependency was therefore absent from the generator's index,batchByID[id]returned a zeroentity.Batch, and that zero batch reached the scorer with an emptyQueue.The blast radius was the whole queue rather than one batch, because
Generatescores every unresolved dependency up front to seed its heap and returned on the first error.standard.Speculatethen short-circuited before the allocator pulled a single candidate: no path failed, none was ever produced, and the fall-back-to-the-next-path machinery sits downstream of a stage that had already died. A measured run of 20 requests left 9 inerrorand 11 wedged inspeculatingwith nothing recorded against them.What?
Hand the Speculator the whole queue.
asknow flattenssnap.batches— every batch the run read — and passes that. No ordering is imposed: the generator's heap comparator is a strict total order, so its candidate sequence is identical whatever order the input arrives in (checked over 200 shuffles), and a Speculator that read meaning into input order would be relying on something the contract never offered.ask's doc comment argued for the narrow slice and is rewritten.Widening is safe because of the commit below this one. A head this run has just decided still reads as
Speculatingin the snapshot —finalizerecords only terminal outcomes — and both the Speculator andcheckread head eligibility off that same map, so a stale entry would fool both filters at once. What stops it mattering is that a head now merges only once every dependency has settled: the generator pins them all, can therefore construct nothing but the path that already passed, and the allocator skips that as finished. Verified by driving the realbestfirst/stickypair with a merged head and settled dependencies — zero actions proposed for it.That ordering is load-bearing, which is why the merge gate is the parent rather than a follow-up. With the old gate a head could merge past a dependency that was still live; the generator would then see that dependency as an open question, offer a path ID the set had never held, and the allocator would fund a fresh build for a batch already handed to Runway.
Make one unpriceable dependency cost only its own estimate.
scorenow substitutesdefaultProbabilitywhen the scorer returns an error, and never calls the scorer at all for a dependency the snapshot did not carry — that batch is zero in every field, so scoring it would price some other batch entirely or fail on its empty queue name. Context cancellation is still fatal, including when it surfaces as the scorer's error: the loop checksctxbefore each call, so a context that dies during the last one would otherwise be absorbed as an unpriceable dependency and hand back an iterator to a caller that has already gone. Scorer failures stay observable through the scorer's own metrics span, which already reports them viaop.Complete(retErr).That is the whole containment fix. An earlier revision of this branch also made scoring lazy — heads seeded at an optimistic bound and priced on first pull — and it has been dropped. The only admissible bound for an unpriced head is
log 1, identical for every head, so the first pull priced the entire queue anyway; the laziness bought one narrow case (a run that pulls nothing because the budget is saturated) in exchange for a placeholder, an admissibility argument, priced and unpriced items sharing a heap, and five reworked tests. Defaulting on failure fixes the bug on its own.Mergingis left as an open question in the generator. Tempting to pin it to succeeds — the batch looks committed to landing — but a merge can fail, so nothing is settled, and it would put a state-specific policy inside the search when whether a path betting against a merging batch is worth funding is a question of price that belongs to the scorer. The allocator already draws exactly this line — "no batch state enters this decision —mergingand the rest are states of a batch, never of a path" — and the generator holds it too.Test Plan
make test— 96/96 passmake lint,make check-gazelle,make check-tidyNew and reworked coverage, per defect:
TestRun_PassesSnapshotToSpeculator— the Speculator receives every batch the run read, in a stable orderTestBestFirst_AbsorbsScorerError— a scorer error costs that dependency its estimate and nothing elseTestBestFirst_NeverScoresAnAbsentDependency— an absent dependency never reaches the scorer, even when the caller hands over a malformed snapshotTestBestFirst_MergingDependencyStaysOpen— a merging dependency is priced like any other and keeps both sidesTestBestFirst_HonorsCancelledContext/a scorer that fails on a dead context ends the runThe last was added for a defect a review of this branch turned up, and was confirmed to fail against the code as it stood before the fix.
Not verified end to end:
make demo-prlives on thesq/demo-prbranch, so reproducing the original 20-request run needs that target ported across worktrees plus a Docker stack.Issue
Fixes https://linear.app/uber/issue/CODEM-424
That issue proposed lazy scoring as its primary fix; this lands the containment it was after without the algorithm change, for the reasons above.
Follow-up filed as https://linear.app/uber/issue/CODEM-428 — this stops queues wedging this way, but a queue already wedged still has no event that will wake it, because speculation is edge-triggered only.
The merge-gate defect found while investigating this one — a head could merge on a fails assumption that had not come true — is the parent commit, since the widening here relies on the invariant it restores.