Fix flaky PerTestRecordingSpec by waiting for recording files to stabilize

[borinquenkid]

Summary

PerTestRecordingSpec > the recordings of the previous two tests are different is
flaky (~1% of runs per #16030), with 0 hard failures — same commit,
different outcome on rerun.

Root cause

waitForRecordingFiles polled for candidate recording files by existence + name-match

• count only. Testcontainers' VncRecordingContainer.saveRecordingToFile() copies the
  video with a plain Files.copy(...) and no atomic temp-file+rename, so the destination
  file becomes visible to a directory scan the instant the copy starts — a
  still-writing (possibly 0-byte) file passes the existence check. Two still-partial
  files can register as byte-identical via Files.mismatch, intermittently failing the
  "recordings are different" assertion.

This builds on James's prior fix in c179aac ("Address flaky test"), which fixed an
earlier race in which files get matched but didn't check that a matched file had
finished being written.

Fix

waitForRecordingFiles now tracks each candidate file's size across polls and only
accepts a file once its size is > 0 and unchanged from the prior poll — proof the
copy has actually finished, not just that a file handle exists. Kept the existing
10s timeout / 500ms poll interval. This is a targeted readiness check, not a generic
retry — no @Retry, no blanket rerun.

Testing

• Real Testcontainers run (Chrome + vnc-recorder, full recording lifecycle):
  ./gradlew :grails-test-examples-geb:integrationTest --tests "org.demo.spock.PerTestRecordingSpec"
  — BUILD SUCCESSFUL, all 3 iterations passed including the previously-flaky assertion.
• No new unit test added: waitForRecordingFiles is a private helper embedded in the
  integration spec with no public-API surface to test independently; the existing
  integration spec is the only legitimate exercise path per the repo's public-API
  testing convention.
• CodeNarc/Checkstyle: clean (aggregateStyleViolations, no violations).

Related: #16030

[Copilot]

Pull request overview

This PR addresses a flaky Geb/Testcontainers integration spec (PerTestRecordingSpec) by tightening the readiness criteria for VNC recording files so the test doesn’t compare partially-copied (e.g., 0-byte) videos.

Changes:

• Update waitForRecordingFiles to only treat recording files as “ready” once their size is > 0 and unchanged across two consecutive polling scans.
• Keep the existing polling cadence (10s timeout / 500ms interval) while improving correctness of the “recordings are available” detection.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 51.8821%. Comparing base (6d1acad) to head (08800e4).
⚠️ Report is 139 commits behind head on 8.0.x.

Additional details and impacted files

@@               Coverage Diff                @@
##             8.0.x     #16031         +/-   ##
================================================
+ Coverage         0   51.8821%   +51.8821%     
- Complexity       0      18118      +18118     
================================================
  Files            0       2046       +2046     
  Lines            0      96274      +96274     
  Branches         0      16727      +16727     
================================================
+ Hits             0      49949      +49949     
- Misses           0      38953      +38953     
- Partials         0       7372       +7372     

see 2046 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[jdaugherty]

Thanks for digging into this one — the polling helper is definitely the right place to look, and the stability-tracking code itself is clean and readable.

My concern is with the diagnosis. I walked the recording lifecycle and I don't think the partial-write race described in the PR body can actually occur:

• GebRecordingTestListener.afterIteration calls containerHolder.container.afterTest(...) synchronously, on the test thread, in Spock's per-iteration callback.
• BrowserWebDriverContainer.afterTest → retainRecordingIfNeeded → vncRecordingContainer.saveRecordingToFile(recordingFile).
• saveRecordingToFile is try (InputStream in = streamRecording()) { Files.copy(in, file.toPath(), REPLACE_EXISTING); }, and streamRecording() itself blocks on execInContainer("ffmpeg", ...) before the copy even begins.

So both Files.copy calls have fully returned before the third feature method starts — nothing is still writing to those files while waitForRecordingFiles scans. Files.copy not being atomic only matters if a reader can observe the file concurrently with the writer, and here the spec is @Stepwise, so there is no concurrent writer to observe. That makes me think the change doesn't remove the flake so much as change which assertion reports it.

Worth noting too that the linked flaky report shows 0/1419 hard failures and 1/1419 flaky, so there's no captured failure output in the issue — which means the root cause is still a hypothesis. Before landing a fix I'd want to know which assertion actually failed, because the two candidates want very different fixes:

1. recordingFiles.size() >= minFileCount failed — only one recording landed. GebRecordingTestListener swallows NotFoundException for /newScreen.mp4 with a debug log, which is exactly what happens when the freshly restarted VNC container hadn't captured any frames yet for ffmpeg to re-encode. If that's the cause, the fix belongs in restartVncRecordingContainer (wait until the new container is actually capturing), and this PR makes the symptom worse: the wait now burns the full 10s before failing.
2. Files.mismatch(...) != -1 failed — the two recordings really were byte-identical. Requiring size > 0 rules out the both-files-empty case, but not the case that seems likelier to me: two degenerate near-blank captures that ffmpeg -vcodec libx264 -movflags faststart encodes to the same bytes. Those are non-zero and size-stable, so they sail through the new check. If this is the cause, the byte-comparison assertion is itself the flake and should be replaced with something that asserts the actual framework contract (a distinct, non-trivial recording per feature method) rather than raw byte inequality of ffmpeg output.

Suggestion: rather than land this, could you pull the failure output for the flaky run from the CI/Develocity build scan (or reproduce with the recording container restart forced to fail) so we can confirm which assertion broke? Happy to help chase it. I'd rather not add a readiness check for a race that the synchronous call chain rules out — it costs latency on every run and leaves the real cause in place.

One process note: PerTestRecordingSpec exists on 7.0.x too, with an older waitForRecordingFiles that doesn't even have the per-run directory scoping from c179aac. Whatever we land here, it'd be good to decide whether it goes to 7.0.x first and merges forward, so the two copies of this helper don't keep diverging.

[borinquenkid]

@jdaugherty thanks for the deep read on this one — replied inline on each of the three specific comments, and wanted to close the loop on the top-level review too.

On the diagnosis: you were right that the original stability-polling premise doesn't hold. Verified the same thing you did against the Testcontainers 2.0.5 source (saveRecordingToFile's Files.copy has returned before this feature method ever runs), and reverted that change entirely in 24a0b59.

On confirming which assertion actually failed: I wasn't able to pull the original CI/Develocity output — it's a <1% flake and #16030 has no captured failure info attached, so I don't have a build scan to chase. Rather than sit on this without that signal, 24a0b59 hedges across both of the candidate diagnoses you laid out instead of landing a fix confirmed against the real failure:

1. Fixed a real bug in WebDriverContainerHolder#restartVncRecordingContainer — the vncRecordingContainer field was updated to the new container before start() succeeded, so a swallowed start() failure left it silently pointing at a container that never started. This doesn't cover the "connected but not yet capturing frames" timing gap you described — I don't have a way to force that deterministically without a real repro.
2. Replaced the byte-difference-only check with a minimum-size floor per recording. Kept the byte-mismatch check on top rather than dropping it — it also catches two recordings accidentally resolving to the same file, which the size floor alone wouldn't. The floor isn't a guess anymore either: 08800e4 grounds MIN_MEANINGFUL_RECORDING_BYTES in two real local runs against the container, where every genuine recording measured 75KB-855KB (5,000 bytes stays more than an order of magnitude below the smallest of those).
3. Added WebDriverContainerHolderSpec (08800e4) covering stop()'s try/finally reset and restartVncRecordingContainer()'s guard clauses / exception-swallowing — no Docker required, since neither had unit coverage before.

I'd still take a real repro over this if one turns up — happy to hold or follow up further if you'd rather chase the actual signal first.

On 7.0.x: agreed the two copies of waitForRecordingFiles shouldn't keep diverging. Proposing we land this on 8.0.x first, and I'll follow up with a backport PR to 7.0.x that also picks up the per-run directory scoping from c179aac, since 7.0.x doesn't have that either. Let me know if you'd rather sequence it the other way.

[testlens-app]

✅ All tests passed ✅

⚠️ TestLens detected flakiness ⚠️

Test Summary

CI / Functional Tests (Java 25, indy=false) > :grails-test-examples-gsp-sitemesh3:integrationTest

Test	Runs	Flakiness
EndToEndSpec > async multiple levels of layouts	❌ ✅	1% 🟡

🏷️ Commit: 08800e4
▶️ Tests: 19306 executed
⚪️ Checks: 62/62 completed

---

Learn more about TestLens at testlens.app.