From 3d65da0529ae64c0884b925e47ee0026bf0a47f7 Mon Sep 17 00:00:00 2001 From: Logan Date: Mon, 17 Aug 2026 14:16:45 -0400 Subject: [PATCH] test(download): stop racing a download these tests never wanted `check` failed on main with two failures in one package, and they are one cause wearing two shapes: service_test.go:66: state = "satisfied", want wanted testing.go:1369: TempDir RemoveAll cleanup: ... directory not empty Every test in service_test.go is about the durable Request that StartDownload leaves behind, and none is about the download. But the fixture is an anchored four-track request with a healthy provider, which is precisely what AutoPickable says yes to -- so Manager.Start fired `go m.grab(...)`, detached and with context.WithoutCancel, and the tests raced it. Measured: the request reaches "satisfied" about 100ms after StartDownload returns, so the first failure is the assertion reading the next state, and the second is that same goroutine still writing into t.TempDir() after the test returned. The fixture now puts the candidate outside the auto-pick size window, so the grab never starts. That is better than waiting for it: with no goroutine there is nothing to be slow, and the tests state what they mean without a timing assumption underneath. A test that does want the download uses managerFixture and sets its own preferences. It passed 20 runs under CPU load, but so did the broken version -- this is a CI-only failure locally, so the cause was proved directly instead: with the fixture's old preferences the request is observably "satisfied" within 100ms of StartDownload, which is what CI read. --- backend/download/service_test.go | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/backend/download/service_test.go b/backend/download/service_test.go index a90fe27..c497047 100644 --- a/backend/download/service_test.go +++ b/backend/download/service_test.go @@ -20,6 +20,30 @@ func newServiceFixture(t *testing.T) serviceFixture { mf := newManagerFixture(t) svc := NewService(slogDiscard(), mf.manager, mf.store, NewMemSecretStore()) + // Every test here is about the durable Request that `StartDownload` + // leaves behind, and none of them is about the download itself -- but + // their fixture is an anchored four-track request with a healthy + // provider, which is exactly what `AutoPickable` says yes to. So + // `Manager.Start` was firing `go m.grab(...)`, detached and with + // `context.WithoutCancel`, and the test then raced it. + // + // It lost, twice, in CI (`check` on c03c0b8, and nowhere locally): + // + // service_test.go:66: state = "satisfied", want wanted + // testing.go:1369: TempDir RemoveAll cleanup: ... directory not empty + // + // The first is the request reaching its *next* state before the + // assertion read it; the second is that same goroutine still writing + // into `t.TempDir()` after the test returned. One cause, two shapes. + // + // Putting the candidate outside the auto-pick size window stops the + // grab from ever starting, which is better than waiting for it: there + // is no goroutine to be slow, so the tests state what they mean + // ("the request exists, in this state") without a timing assumption + // underneath. A test that does want the download has `managerFixture` + // and sets its own preferences. + mf.manager.SetPreferences(AutoDownloadPrefs{MaxSizeMB: 1}) + return serviceFixture{managerFixture: mf, svc: svc} }