Skip to content

test(restore): wait for the upload cleanup before checking the temp file - #1764

Open
zorak1103 wants to merge 1 commit into
perber:mainfrom
zorak1103:fix/restore-upload-cleanup-race
Open

zorak1103 wants to merge 1 commit into
perber:mainfrom
zorak1103:fix/restore-upload-cleanup-race

Conversation

@zorak1103

Copy link
Copy Markdown
Contributor

Related issue

No issue. This is a flaky test that failed on the Windows job of #1763, a PR that does not touch internal/restore: https://github.com/perber/leafwiki/actions/runs/40884561713/job/123484693452

What changed

TestManager_RestoreFromUpload_CleansUpTempFileAfterCompletion could fail with expected upload temp file to be cleaned up, found: [...]. The test change is two lines plus comments, and no production code changes.

The test polls waitForRestoreDone and then looks for the staged upload straight away. But the job is already Done when runFromZipPath calls Finish; the temp file is removed afterwards by the defer cleanup() in the goroutine that TriggerRestoreFromUpload starts. If the check runs between the two, it still sees the file.

Both the success and the failure subtest had this race. They now call Manager.Wait() after waitForRestoreDone, which returns only once that goroutine, including the deferred cleanup, has finished. The doc comment of Wait mentions shutdown, but it waits on the same WaitGroup that tracks this goroutine.

How I tested it

  • To make the race deterministic, I temporarily delayed the cleanup in stageUpload by 200 ms (not committed). Without the fix both subtests fail (success with the message from CI, failure with its "even on failure" variant); with the fix they pass.
  • Without the delay the failure is rare. It failed once in 12 runs on my Windows machine, and not in two later stress runs of 60 and 40. Both stress runs (-count=60 before and -count=40 after the fix) pass.
  • go test passes for ./internal/restore and ./internal/wiki/restore. GOOS=linux go vet, golangci-lint for the package and scripts/check-private-references.sh are clean.
  • I tested on Windows only. Linux runs rely on this PR's CI.

Checklist

  • I discussed the approach on the linked issue before starting (or this is a trivial fix: typo, docs, obvious one-liner)
  • This PR is focused on a single change
  • New behaviour plugs into an extension point; no new if feature branches in shared code (rule 1) (test fix, no new behaviour)
  • I updated documentation if needed (not needed)
  • If users or admins will notice a change in behaviour, the commit carries a Behavior-Change: trailer (commit style) (no behaviour change)
  • For UI changes: behaviour is covered by automated tests where possible (rule 11) (no UI changes)
  • For UI changes: I checked by hand what tests can't cover (look in light/dark mode, mobile width where relevant) (no UI changes)
  • AI tools were used for this PR (name them: Claude Code, Claude Sonnet 5.5). I have reviewed and understand every line

TestManager_RestoreFromUpload_CleansUpTempFileAfterCompletion polled the
job status and then looked for the staged upload straight away. The job
is Done once runFromZipPath calls Finish, but the temp file is removed by
the cleanup that TriggerRestoreFromUpload defers in its goroutine, so the
check could run first and still see the file (seen on the Windows CI).

Call Manager.Wait after the job is Done, which returns only once that
goroutine, including the deferred cleanup, has finished. Both the
success and the failure subtest had the race.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant