Test Builder cache reuse across disposable VMs - #355
Conversation
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Flaky builder image readiness wait
- The readiness grace period was increased from 100ms to 600ms so the test no longer races with the build manager’s 500ms builder-image readiness poll window.
Or push these changes by commenting:
@cursor push 82e7750190
Preview (82e7750190)
diff --git a/integration/builder_cache_linux_test.go b/integration/builder_cache_linux_test.go
--- a/integration/builder_cache_linux_test.go
+++ b/integration/builder_cache_linux_test.go
@@ -128,8 +128,8 @@
}
require.True(collect, ready)
}, 5*time.Minute, time.Second)
- // ensureBuilderImage records the ready image in the build manager after conversion.
- time.Sleep(100 * time.Millisecond)
+ // ensureBuilderImage updates the build manager on its next readiness poll.
+ time.Sleep(600 * time.Millisecond)
builder, err := builderManager.CreateBuilder(ctx, builders.CreateBuilderRequest{DiskSizeGb: 4})
require.NoError(t, err)You can send follow-ups to the cloud agent here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Missing iptables xtables wait
- Updated both registry firewall insert and cleanup iptables calls to include
-w 5so they wait for the xtables lock instead of failing under contention.
- Updated both registry firewall insert and cleanup iptables calls to include
Or push these changes by commenting:
@cursor push 00643cd05c
Preview (00643cd05c)
diff --git a/integration/builder_cache_linux_test.go b/integration/builder_cache_linux_test.go
--- a/integration/builder_cache_linux_test.go
+++ b/integration/builder_cache_linux_test.go
@@ -181,9 +181,9 @@
t.Cleanup(func() { _ = server.Close() })
port := strconv.Itoa(listener.Addr().(*net.TCPAddr).Port)
firewallArgs := []string{"INPUT", "-i", bridge, "-p", "tcp", "--dport", port, "-j", "ACCEPT"}
- output, err := exec.Command("iptables", append([]string{"-I"}, firewallArgs...)...).CombinedOutput()
+ output, err := exec.Command("iptables", append([]string{"-w", "5", "-I"}, firewallArgs...)...).CombinedOutput()
require.NoErrorf(t, err, "allow registry traffic: %s", output)
- t.Cleanup(func() { _ = exec.Command("iptables", append([]string{"-D"}, firewallArgs...)...).Run() })
+ t.Cleanup(func() { _ = exec.Command("iptables", append([]string{"-w", "5", "-D"}, firewallArgs...)...).Run() })
return net.JoinHostPort(gateway, port), string(certPEM)
}You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit b7bdbb2. Configure here.
sjmiller609
left a comment
There was a problem hiding this comment.
integration/builder_cache_linux_test.go:130-142: after polling image readiness, then uses an arbitrary 600ms sleep to infer build-manager readiness. Even though likely to work, I think should poll again or some way that doesn't need arbitrary sleep just to avoid potential source of test flakiness.
|
@sjmiller609 addressed in 59a3d7c: the test now polls the build manager’s actual readiness state instead of sleeping for an inferred polling interval. |


summary
validation
TestBuilderPersistentCacheReuseNote
Low Risk
Changes are primarily a new integration test plus a small read-only readiness hook on the build manager; no production build or cache behavior is altered.
Overview
Adds a Linux integration test (
TestBuilderPersistentCacheReuse) that proves BuildKit cache on a Builder’s persistent disk survives across separate builder VM instances.The test runs two builds on the same Builder with a Dockerfile that uses
RUN --mount=type=cache; it expectsBUILDER_CACHE_MISSon the first run andBUILDER_CACHE_HITon the second, and asserts the two builds used differentBuilderInstanceIDs. The harness spins up the full stack (network, builders, builds), a TLS registry on the gateway IP, optional nft firewall rules for bridge traffic, and waits for the internal builder image before submitting work.To avoid racing startup builder-image prep, the builds
Managerinterface gainsReadyForBuilds(), implemented by reading the existingbuilderReadyflag; the test blocks on that after the builder image shows ready in the image store.Reviewed by Cursor Bugbot for commit 59a3d7c. Bugbot is set up for automated code reviews on this repo. Configure here.