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.
This commit is contained in:
Daniel Hiltgen 2026-07-24 13:23:30 -07:00 committed by GitHub
parent 83d4311ffe
commit a84b315e7b
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 42 additions and 8 deletions

View file

@ -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 {

View file

@ -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())