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.4701%. Comparing base (6d1acad) to head (7d7b39d).
⚠️ Report is 17 commits behind head on 8.0.x.

Additional details and impacted files

@@               Coverage Diff                @@
##             8.0.x     #16031         +/-   ##
================================================
+ Coverage         0   51.4701%   +51.4701%     
- Complexity       0      17754      +17754     
================================================
  Files            0       2039       +2039     
  Lines            0      95537      +95537     
  Branches         0      16571      +16571     
================================================
+ Hits             0      49173      +49173     
- Misses           0      39062      +39062     
- Partials         0       7302       +7302     

see 2039 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.

[jdaugherty]

The premise in this comment doesn't hold, so this check is guarding against something that can't happen.

GebRecordingTestListener.afterIteration calls container.afterTest(...) synchronously on the test thread. That reaches BrowserWebDriverContainer.retainRecordingIfNeeded, which calls vncRecordingContainer.saveRecordingToFile(recordingFile):

try (InputStream inputStream = streamRecording()) {
    Files.copy(inputStream, file.toPath(), StandardCopyOption.REPLACE_EXISTING);
}

and streamRecording() blocks on execInContainer("ffmpeg", ...) before the copy starts. Because the spec is @Stepwise, features 1 and 2 have both returned from afterIteration — and therefore from Files.copy — before this feature method runs. Files.copy lacking a temp-file+rename only matters when a reader can race a live writer; there is no live writer here.

Separately, even on its own terms "size unchanged since the previous poll" isn't proof a copy finished — a docker tar stream that stalls for more than the 500ms poll interval under CI I/O contention gives two equal readings mid-copy. If a readiness check is genuinely needed, it would want N consecutive stable polls rather than one.

[jdaugherty]

previousSizes starts empty, so on the first poll previousSizes[path] == size is null == size for every candidate and readyFiles is always empty — no matter how long the recordings have been sitting complete on disk.

That means every run of this spec now sleeps at least one full pollIntervalMillis (500ms) before it can succeed, where previously the first poll could break immediately. Seeding previousSizes from an initial scan before entering the loop would avoid the unconditional penalty if this check is kept.

[jdaugherty]

On timeout this returns readyFiles, and candidateFiles is discarded. That makes a real failure harder to diagnose than before: previously the returned list held whatever was on disk, so the recordingFiles.size() >= minFileCount failure message showed the files that were found. Now a timeout typically reports an empty list, with no signal distinguishing "no recording was ever written" from "two recordings existed but never satisfied the stability check" — and those point at different bugs.

If the check stays, please surface the candidates on timeout (return them, or at minimum log their names and sizes) so the next flaky run leaves behind something actionable.

[testlens-app]

✅ All tests passed ✅

🏷️ Commit: 24a0b59
▶️ Tests: 56163 executed
⚪️ Checks: 61/61 completed

---

Learn more about TestLens at testlens.app.