From a84b315e7b5400e04e2a6ca60bc3336837bf11da Mon Sep 17 00:00:00 2001 From: Daniel Hiltgen Date: Fri, 24 Jul 2026 13:23:30 -0700 Subject: [PATCH] test: harden flaky updater and transfer unit tests (#17378) app/updater: TestBackgoundChecker / TestAutoUpdateDisabledSkipsDownload hit 'TempDir RemoveAll cleanup: directory not empty' on macOS because the background checker goroutine keeps writing staged files into UpdateStageDir while t.TempDir cleanup runs. The checker's context is cancelled by the time cleanup runs, and after cancellation a new download cannot reach the filesystem (DownloadNewRelease aborts at its HEAD request before any write), so it suffices to wait for any in-flight download to drain. Add a test-only waitDownloadIdle helper (polls the existing cancelDownload sentinel under its lock) and register it via t.Cleanup so TempDir cleanup runs after staged-file handles close. No production code changes. x/transfer: TestDownloadParallelism asserted elapsed <= 1s against 50ms-per-blob delays, too tight for Windows hosted runners' ~15ms timer granularity and shared-runner jitter. Each blob costs two server sleeps (resolve GET + body GET), so model the serial baseline from the deterministic request count, raise per-blob latency to 100ms so timer quantization is a small fraction of each delay, and key the budget to 75% of the serial baseline so the check still proves parallelism while tolerating jitter. --- app/updater/updater_test.go | 22 ++++++++++++++++++++++ x/transfer/transfer_test.go | 28 ++++++++++++++++++++-------- 2 files changed, 42 insertions(+), 8 deletions(-) diff --git a/app/updater/updater_test.go b/app/updater/updater_test.go index 7673e9942..fdca50375 100644 --- a/app/updater/updater_test.go +++ b/app/updater/updater_test.go @@ -190,6 +190,23 @@ func TestDownloadNewReleaseDoesNotUseRawETagAsPathComponent(t *testing.T) { } } +// waitDownloadIdle blocks until no download is in flight, so staged-file +// handles close before t.TempDir cleanup removes the stage directory. After +// the context is cancelled a new download can't write (it aborts at the HEAD +// request), so reaching idle makes cleanup race-free. +func (u *Updater) waitDownloadIdle() { + deadline := time.Now().Add(2 * time.Second) + for time.Now().Before(deadline) { + u.cancelDownloadLock.Lock() + idle := u.cancelDownload == nil + u.cancelDownloadLock.Unlock() + if idle { + return + } + time.Sleep(time.Millisecond) + } +} + func TestBackgroundCheckerSkipsAlreadyStagedETagDownload(t *testing.T) { UpdateStageDir = t.TempDir() oldInstaller := Installer @@ -276,6 +293,7 @@ func TestBackgroundCheckerSkipsAlreadyStagedETagDownload(t *testing.T) { callbacks <- ver return nil }) + t.Cleanup(updater.waitDownloadIdle) for range 2 { select { @@ -364,6 +382,7 @@ func TestBackgoundChecker(t *testing.T) { } updater.StartBackgroundUpdaterChecker(ctx, cb) + t.Cleanup(updater.waitDownloadIdle) select { case <-stallTimer.C: t.Fatal("stalled") @@ -426,6 +445,7 @@ func TestAutoUpdateDisabledSkipsDownload(t *testing.T) { } updater.StartBackgroundUpdaterChecker(ctx, cb) + t.Cleanup(updater.waitDownloadIdle) // Wait enough time for multiple check cycles time.Sleep(50 * time.Millisecond) @@ -488,6 +508,7 @@ func TestAutoUpdateReenabledDownloadsUpdate(t *testing.T) { } upd.StartBackgroundUpdaterChecker(ctx, cb) + t.Cleanup(upd.waitDownloadIdle) // Wait for a few cycles with auto-update disabled - no download should happen time.Sleep(50 * time.Millisecond) @@ -615,6 +636,7 @@ func TestTriggerImmediateCheck(t *testing.T) { } updater.StartBackgroundUpdaterChecker(ctx, cb) + t.Cleanup(updater.waitDownloadIdle) // Wait for the initial check that fires after the initial delay select { diff --git a/x/transfer/transfer_test.go b/x/transfer/transfer_test.go index 206f144be..f58d94dfd 100644 --- a/x/transfer/transfer_test.go +++ b/x/transfer/transfer_test.go @@ -860,12 +860,22 @@ func verifyBlob(t *testing.T, dir string, blob Blob, expected []byte) { // ==================== Parallelism Tests ==================== func TestDownloadParallelism(t *testing.T) { - // Create many blobs to test parallelism serverDir := t.TempDir() numBlobs := 10 blobs := make([]Blob, numBlobs) blobData := make([][]byte, numBlobs) + const ( + concurrency = 4 + perBlobLatency = 100 * time.Millisecond + // Download issues two requests per blob: a resolve GET then the body GET. + requestsPerBlob = 2 + ) + // serialBaseline is the time a strictly serial run would take. 100ms is + // large enough that Windows' ~15ms timer granularity is a small fraction + // of each delay; the elapsed check only needs to beat this by a wide margin. + serialBaseline := time.Duration(numBlobs*requestsPerBlob) * perBlobLatency + for i := range numBlobs { blobs[i], blobData[i] = createTestBlob(t, serverDir, 1024+i*100) } @@ -886,7 +896,7 @@ func TestDownloadParallelism(t *testing.T) { } // Simulate network latency to ensure parallelism is visible - time.Sleep(50 * time.Millisecond) + time.Sleep(perBlobLatency) digest := filepath.Base(r.URL.Path) path := filepath.Join(serverDir, digestToPath(digest)) @@ -907,8 +917,8 @@ func TestDownloadParallelism(t *testing.T) { Blobs: blobs, BaseURL: server.URL, DestDir: clientDir, - Concurrency: 4, - BodyConcurrency: 4, + Concurrency: concurrency, + BodyConcurrency: concurrency, }) elapsed := time.Since(start) @@ -926,10 +936,12 @@ func TestDownloadParallelism(t *testing.T) { t.Errorf("Max concurrent requests was %d, expected at least 2 for parallelism", maxConcurrent.Load()) } - // With 10 blobs at 50ms each, sequential would take ~500ms - // Parallel with 4 workers should take ~150ms (relax to 1s for CI variance) - if elapsed > time.Second { - t.Errorf("Downloads took %v, expected faster with parallelism", elapsed) + // Require finishing under 75% of the serial baseline so the check still + // proves parallelism, with room for Windows timer granularity and runner + // jitter. maxConcurrent above is the real gate that parallelism occurred. + parallelBudget := serialBaseline * 3 / 4 + if elapsed > parallelBudget { + t.Errorf("Downloads took %v, expected faster with parallelism (serial baseline %v, budget %v)", elapsed, serialBaseline, parallelBudget) } t.Logf("Downloaded %d blobs in %v with max %d concurrent requests", numBlobs, elapsed, maxConcurrent.Load())