From 409bfd5e894c31b4769cdc6d0c4a0c297a8cc803 Mon Sep 17 00:00:00 2001 From: Caleb Allen Date: Mon, 17 Aug 2026 09:22:51 -0400 Subject: [PATCH] test(download): wait for the work, not for the state that precedes it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TestManagerEndToEndAutoPick waits for StateComplete and then asserts that staging was released and the library was rescanned. Those happen *after* the state is recorded: manager.go sets StateComplete, then satisfies the request, then releases staging, then scans. So waiting on the state is not waiting on either assertion, and on a loaded machine the worker is descheduled in between and the test reads the world one step too early: manager_test.go:209: staging not released: 1 dirs remain manager_test.go:218: library scans = 0, want 1 It passed alone every time and failed three times under a full-suite run, which is the signature of a test race rather than a broken manager — nothing here is wrong except what the test chose to wait on. It blocks pushes, since the pre-push hook is exactly the loaded run. It polls for the side effects now, through the waitFor this package already has and already uses for the same reason one file over (service_test.go waits for a request to become satisfied after the same StateComplete). Not reproduced on demand: eight spinners and -count=5 did not provoke it with or without the fix, so this rests on the ordering being plain in the code rather than on a red-to-green demonstration. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01AfVYUVExXsx1nSWrXN8mAh --- backend/download/manager_test.go | 31 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/backend/download/manager_test.go b/backend/download/manager_test.go index 016830f..72d504e 100644 --- a/backend/download/manager_test.go +++ b/backend/download/manager_test.go @@ -199,24 +199,23 @@ func TestManagerEndToEndAutoPick(t *testing.T) { t.Errorf("expected imported file at %s: %v", want, err) } - // Staging was released only after a successful import. - entries, err := os.ReadDir(f.staging.Root()) - if err != nil { - t.Fatalf("read staging root: %v", err) - } + // Staging release and the rescan happen *after* the state is + // recorded (manager.go sets StateComplete, then releases, then + // scans), so waiting on the state is not waiting on these. Under + // load the worker is descheduled in between and asserting straight + // away reads the world one step too early -- which is exactly how + // this test failed on a busy machine while passing alone. + waitFor(t, func() bool { + entries, err := os.ReadDir(f.staging.Root()) + if err != nil || len(entries) != 0 { + return false + } - if len(entries) != 0 { - t.Errorf("staging not released: %d dirs remain", len(entries)) - } + f.lib.mu.Lock() + defer f.lib.mu.Unlock() - // The library was told to rescan. - f.lib.mu.Lock() - scanned := len(f.lib.scanned) - f.lib.mu.Unlock() - - if scanned != 1 { - t.Errorf("library scans = %d, want 1", scanned) - } + return len(f.lib.scanned) == 1 + }, "staging was never released, or the library was never rescanned") } // An ambiguous result set must park for the user rather than guess.