Conversation
When different LLB vertexes (eg. parallel requests referencing local sources from different sessions) generate same cache keys during solve they are merged together into a single operation. Currently, when this happened the progress for the vertex that was dropped got lost. This fixes this case by adding the progressWriter of the redirected vertex as a target to the source one. This should also work with multiple levels of merged edges, just multiple nested multiwriters as well. Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com> (cherry picked from commit e1da8b7)
Before this, it was possible for an edge merge to happen to a target edge/state that is no longer in the actives map, e.g. 1. Job A solves some edges 2. Job B solves some edges that get merged with job A's edges 3. Job A is discarded 4. Job C solves some edges that get merged with job B's edges, which recursively end up merging to job A's, which no longer exist in the actives map. While this doesn't always result in an error, given the right state and order of operations this can result in `getState` or `getEdge` being called on the "stale" edges that are inactive and an error. E.g. if a stale dep transitions from desired state `cache-fast` to `cache-slow`, the slow cache logic will call `getState` on it and return a `compute cache` error. I also *suspect* this is the same root cause behind `inconsistent graph state` errors still seen occasionally, but have not repro'd that error locally so can't be 100% sure yet. The fix here updates `state.setEdge` to register all the jobs in the source edge's state with the target edge's state. This works out because: 1. edges that are the target of a merge will not have their state removed from the actives map if only the original job creating them is discarded 1. those edges are still removed eventually, but only when all jobs referencing them (now including jobs referencing them via edge merges) are discarded Signed-off-by: Erik Sipsma <erik@sipsma.dev> (cherry picked from commit 1b3daaa)
Signed-off-by: Erik Sipsma <erik@sipsma.dev> (cherry picked from commit ffdbd86)
addJobs (moby/buildkit#4887) logs through bklog, which this fork's jobs.go did not import yet.
… failure Drives a buildkitd into "failed to get state for index", the slow-cache lookup of the same deleted state behind "inconsistent graph state", with no cancellation: a cache-hit session ends while a second session, whose edges merged into the first one's, still needs them to run. It fails 15 to 18 of 24 builds on earthbuild/buildkitd:v0.8.18-b0385986 and none on this branch. See hack/lostedge-repro/README.md.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The reproduction can report success without establishing or executing the required race conditions.
Review effort: Balanced
Findings: 2
Open (3)
What changed in this PR
Backports BuildKit’s stale edge-merge fix and adds a daemon-level reproduction.
Changes:
- Propagates jobs through merged solver states.
- Adds solver regression coverage.
- Adds a real-daemon reproduction utility.
| File | Description |
|---|---|
solver/jobs.go |
Implements merged-state job propagation. |
solver/scheduler_test.go |
Tests edge merging and job lifetimes. |
util/progress/multiwriter.go |
Adds an interface assertion. |
hack/lostedge-repro/main.go |
Implements the reproduction driver. |
hack/lostedge-repro/run.sh |
Runs the driver against a containerized daemon. |
hack/lostedge-repro/README.md |
Documents reproduction usage and results. |
hack/lostedge-repro/go.mod |
Defines reproduction dependencies. |
hack/lostedge-repro/.gitignore |
Excludes generated artifacts. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+149
to
+152
| if *depth > 10 || *keep >= *depth { | ||
| fmt.Fprintln(os.Stderr, "need -depth <= 10 and -keep < -depth") | ||
| os.Exit(2) | ||
| } |
Comment on lines
+186
to
+189
| pruned, err := prune(ctx, c) | ||
| if err != nil { | ||
| fmt.Printf("iter %d: prune failed: %v\n", it, err) | ||
| } |
| require.NotContains(t, s.actives[depV1.Digest()].jobs, j0) | ||
| require.Contains(t, s.actives[depV1.Digest()].jobs, j1) | ||
|
|
||
| // discard j0, verify that v0 is still active and it's state contains j1 since j1's |
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.
Backports moby/buildkit#4887 (and #4347, which it builds on) to the current fork, and adds a real-daemon reproduction of the failure it fixes. This is an alternative to waiting for the upstream merge in #22 / EarthBuild/earthbuild#442, which already includes both. If the upgrade is close, close this and keep only the reproduction.
The failure
failed to get edge: inconsistent graph state(moby/buildkit#2303, #22). On a shared builder it arises without any cancellation:Discarddeletes the states B's merged edges still use, and B fails looking one up.#4887 makes
setEdgeadd the merge source's jobs to the target state and its ancestors, so step 3 no longer deletes them.Changes
15bdffcb7cherry-picks solver: fix printing progress messages after merged edges moby/buildkit#4347:setEdgetakes the target state. This is the signature #4887 extends. The conflict was only in its context: the fork hashasOwnerthere.3be4af221and8a81118a7cherry-pick #4887's two solver commits, with its testTestStaleEdgeMerge. Its third commit only changes debug logging and is left out.b1191ea90importsbkloginsolver/jobs.go, whichaddJobsneeds.980efc9b3addshack/lostedge-repro, a driver that runs the sequence above against any buildkitd image:./hack/lostedge-repro/run.sh <image> -keep 0.Testing
go test ./solver/passes exceptTestJobsIntegration, which fails identically onmainon a machine without the integration sandbox.The reproduction, 24 B builds per run on
linux/arm64:-keep 3-keep 0earthbuild/buildkitd:v0.8.18-b0385986(51fe8fb9)b1191ea90(the last commit only addshack/)2c7e20713)Not covered
#4887 leaves a narrower race, where a
Discardruns between the edge index lookup andsetEdge. It still fails the solver unit test from #22 on stock moby/buildkit v0.33.0. #28 fixes it on top of #22. This reproduction does not reach it.