agent-substrate / agent-substrate/substrate

TestConcurrentEnsureImageAndEvict flakes

Aperta
#1,076 0 commenti 0 reazioni 0 assegnatari Vedi su GitHub
area/tests kind/bug
Lingua principale
Go
Stelle
1.8k
Fork
316
Merge medio
2g 43m
PR unite (30g)
287

Descrizione

## 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

Guida per i contributori

Apri la guida per i contributori

Valutazione

Questa issue non è ancora stata valutata.

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.