agent-substrate / agent-substrate/substrate
TestConcurrentEnsureImageAndEvict flakes
- Langage dominant
- Go
- Étoiles
- 1.8k
- Forks
- 316
- Merge moyen
- 2 j 43 min
- PR mergées (30 j)
- 287
Description
## Expected Behavior
TestConcurrentEnsureImageAndEvict passes: per its doc comment, whatever interleaving happens between EnsureImage and a free-everything EvictUnused pass, "either its touch wins and the evictor skips, or the evictor wins and EnsureImage re-pulls fresh dirs" — so EnsureImage never returns an error.
## Actual Behavior
Rarely, under CI load, an iteration fails with the pull path's defensive backstop error:
2026/08/19 20:32:41 INFO Image cache removed retired layer dirs count=1 took=432.006µs
gc_test.go:507: iteration 24: EnsureImage: layer dir vanished during pull (evicted?): stat /tmp/TestConcurrentEnsureImageAndEvict2288464666/001/layers/sha256/99630e.../fs: no such file or directory
--- FAIL: TestConcurrentEnsureImageAndEvict (0.09s)
The test and the implementation disagree about the contract. The reuse/retire protocol serializes per layer — ensureLayer refreshes the dir mtime inside the layer singleflight, and retireLayer re-checks it inside the same flight and vetoes fresh dirs — which is airtight for a retire racing a concurrent reuse of the same layer. But it leaves a window: after ensureLayer returns a dir and before the image-level record rewrite re-references it, a retire can enter the flight with a cutoff at or past the refresh instant and legitimately rename the dir away. The pull path knows this and handles it by design — the check that fired is its explicit backstop (imagecache.go: "Never return LayerDirs that are not on disk right now: a vanished dir fails the pull into a clean RPC retry instead of a bundle spec naming a missing lowerdir"). In production this is a transient, self-healing error; the test treats it as a failure.
So this is a flake in effect (timing-dependent, pre-existing, harmless in production), but it points at a real gap: the implementation provides "fail clean and let the retry heal it," while the test asserts "eviction winning is a bug." The triggering PR (#803) contains no internal/imagecache changes.
Suggested resolution: make the test's stated invariant true rather than weakening the test — when the final stat in EnsureImage finds a vanished layer dir, loop back and re-pull (bounded; one retry suffices). The retry re-enters ensureLayer, which re-unpacks the layer under a fresh mtime, and the record rewrite re-references it — exactly the "evictor wins and EnsureImage re-pulls" behavior the test describes, and it also spares the production path a full RPC retry for the same interleaving.
## Steps to Reproduce the Problem
1. Run go test -race -run TestConcurrentEnsureImageAndEvict ./internal/imagecache/ repeatedly on a loaded machine (observed on a GitHub Actions ubuntu-latest runner during a PR build; the failure hit iteration 24 of 25 in one run).
2. Note it does not reproduce readily on idle hardware: 100 runs × 25 iterations with -race -cpu=2,4 (~4,000 race windows) on unmodified main @ 43fa6ccb produced zero failures on an idle machine (darwin/arm64).
3. The interleaving needing to land is: ensureLayer returns a layer dir → evictor's retireLayer enters that layer's singleflight with cutoff ≥ the refresh mtime and renames the dir → EnsureImage's final stat loop finds the dir missing and returns the backstop error before the record rewrite would have protected it.
## Specifications
- Version: main @ 43fa6ccb (also present on earlier commits containing the GC/eviction work; the failing CI run was on a PR branch with no internal/imagecache changes)
- Platform: observed on GitHub Actions ubuntu-latest (linux/amd64), Go per go.mod; not reproduced in ~4,000 iterations on idle darwin/arm64
Guide de contribution
Ouvrir le guide de contribution
Évaluation
Cette issue n'a pas encore été évaluée.