From 4aa63cf6ade785e640e7d90302cc36fc5c0be90a Mon Sep 17 00:00:00 2001 From: packetloss404 Date: Sat, 5 Sep 2026 14:28:17 -0500 Subject: [PATCH] test: give the jobs manager a shutdown budget that survives a loaded machine TestResubmit_IsOnlyAllowedOnce failed rarely on Windows, at seventeen seconds, with an error that named nothing: TempDir RemoveAll cleanup: unlinkat ...: The directory is not empty. No assertion in that test was involved. It never even reached one. newTestManager gave Shutdown a hard-coded, unscaled two seconds and discarded the result. Shutdown returns when its timeout expires whether or not the workers stopped, and says which in the error it returns. Under a parallel suite two seconds expired routinely, so the cleanup returned having shut nothing down, and the workers kept writing job records into the t.TempDir() Go was about to remove. On Windows a directory that is gaining files cannot be removed, so the test died in cleanup -- long after the code under test had done its job correctly. TestResubmit_IsOnlyAllowedOnce was the likeliest victim because it is the only Resubmit test that spawns a job and does not wait for it, so its worker is always still running at cleanup. Two changes. The budget is scaled through internal/testwait like every other deadline in the suite, so a busy machine gets the slack and a genuinely stuck worker still fails. And the error is reported instead of discarded: "N workers still running after 20s" is the one sentence that identifies this immediately, and throwing it away is what made a shutdown that shut nothing down look like success. The same unscaled literal appeared at fourteen other call sites in internal/jobs and internal/app, all with the same trap. They are scaled too, so the flake moves to none of them. Evidence. Forced: with the budget cut to a nanosecond the failure reproduces 6/6 with exactly the message above. Natural: under four concurrent full-suite runs it reproduced on the first attempt at 19.26s. After the fix, 500 iterations under that same load are clean. Co-Authored-By: Claude Opus 5 --- internal/app/app_jobs_e2e_test.go | 2 +- internal/app/app_lifecycle_parity_test.go | 7 ++++--- internal/app/app_slashcmd_test.go | 11 ++++++----- internal/jobs/abandoned_test.go | 3 ++- internal/jobs/manager_test.go | 3 ++- internal/jobs/persistence_test.go | 8 +++++--- internal/jobs/testhelpers_test.go | 19 ++++++++++++++++++- 7 files changed, 38 insertions(+), 15 deletions(-) diff --git a/internal/app/app_jobs_e2e_test.go b/internal/app/app_jobs_e2e_test.go index 13f94c3..6e57757 100644 --- a/internal/app/app_jobs_e2e_test.go +++ b/internal/app/app_jobs_e2e_test.go @@ -148,7 +148,7 @@ func TestE2E_SpawnAgentToolViaSlashCommand(t *testing.T) { if err != nil { t.Fatalf("jobs.NewManager: %v", err) } - defer func() { _ = mgr.Shutdown(2 * time.Second) }() + defer func() { _ = mgr.Shutdown(testwait.Timeout(2 * time.Second)) }() // Build a minimal App by hand so we don't have to stand up every // Deps field. The fields we need for this test are: deps.Jobs, diff --git a/internal/app/app_lifecycle_parity_test.go b/internal/app/app_lifecycle_parity_test.go index cac92d2..a5870f3 100644 --- a/internal/app/app_lifecycle_parity_test.go +++ b/internal/app/app_lifecycle_parity_test.go @@ -7,6 +7,7 @@ import ( tea "github.com/charmbracelet/bubbletea" "github.com/packetcode/packetcode/internal/agent" + "github.com/packetcode/packetcode/internal/testwait" "github.com/packetcode/packetcode/internal/ui/components/agentview" ) @@ -28,7 +29,7 @@ func TestFirstVisibleProgressStopsThinkingSpinner(t *testing.T) { func TestLeftArrowOpensAgentsOnlyFromEmptyIdleInput(t *testing.T) { r := newTestApp(t) mgr := wireJobsManagerForSlashTest(t, r) - t.Cleanup(func() { _ = mgr.Shutdown(2 * time.Second) }) + t.Cleanup(func() { _ = mgr.Shutdown(testwait.Timeout(2 * time.Second)) }) r.app.input.Reset() r.app.handleKey(tea.KeyMsg{Type: tea.KeyLeft}) if !r.app.agentView.Visible() { @@ -46,7 +47,7 @@ func TestLeftArrowOpensAgentsOnlyFromEmptyIdleInput(t *testing.T) { func TestAgentWorkspaceTaskPromptCanClearReturnAndSpawn(t *testing.T) { r := newTestApp(t) mgr := wireJobsManagerForSlashTest(t, r) - t.Cleanup(func() { _ = mgr.Shutdown(2 * time.Second) }) + t.Cleanup(func() { _ = mgr.Shutdown(testwait.Timeout(2 * time.Second)) }) r.app.showAgentView() r.app.handleKey(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune("n")}) @@ -84,7 +85,7 @@ func TestAgentWorkspaceTaskPromptCanClearReturnAndSpawn(t *testing.T) { func TestAgentWorkspaceListActionsAreNotSwallowedByTaskInput(t *testing.T) { r := newTestApp(t) mgr := wireJobsManagerForSlashTest(t, r) - t.Cleanup(func() { _ = mgr.Shutdown(2 * time.Second) }) + t.Cleanup(func() { _ = mgr.Shutdown(testwait.Timeout(2 * time.Second)) }) _, _ = r.app.handleSpawnCommand([]string{"inspect the renderer"}) r.app.showAgentView() diff --git a/internal/app/app_slashcmd_test.go b/internal/app/app_slashcmd_test.go index 5b9d62b..062c307 100644 --- a/internal/app/app_slashcmd_test.go +++ b/internal/app/app_slashcmd_test.go @@ -22,6 +22,7 @@ import ( "github.com/packetcode/packetcode/internal/provider" "github.com/packetcode/packetcode/internal/session" "github.com/packetcode/packetcode/internal/statusline" + "github.com/packetcode/packetcode/internal/testwait" "github.com/packetcode/packetcode/internal/tools" "github.com/packetcode/packetcode/internal/ui/components/agentview" "github.com/packetcode/packetcode/internal/ui/components/approval" @@ -1459,7 +1460,7 @@ func TestApp_Queue_ListDropAndClear(t *testing.T) { func TestApp_Agents_ListUsesBackgroundJobs(t *testing.T) { r := newTestApp(t) mgr := wireJobsManagerForSlashTest(t, r) - defer func() { _ = mgr.Shutdown(2 * time.Second) }() + defer func() { _ = mgr.Shutdown(testwait.Timeout(2 * time.Second)) }() snap, spawnErr := mgr.Spawn(jobs.SpawnRequest{ Prompt: "audit fixtures", @@ -1483,7 +1484,7 @@ func TestApp_Agents_ListUsesBackgroundJobs(t *testing.T) { func TestApp_Agents_DetailOpensJobsPanel(t *testing.T) { r := newTestApp(t) mgr := wireJobsManagerForSlashTest(t, r) - defer func() { _ = mgr.Shutdown(2 * time.Second) }() + defer func() { _ = mgr.Shutdown(testwait.Timeout(2 * time.Second)) }() snap, spawnErr := mgr.Spawn(jobs.SpawnRequest{ Prompt: "inspect flaky test", @@ -1503,7 +1504,7 @@ func TestApp_Agents_DetailOpensJobsPanel(t *testing.T) { func TestApp_Agents_NotFoundUsesAgentLabel(t *testing.T) { r := newTestApp(t) mgr := wireJobsManagerForSlashTest(t, r) - defer func() { _ = mgr.Shutdown(2 * time.Second) }() + defer func() { _ = mgr.Shutdown(testwait.Timeout(2 * time.Second)) }() r.app.handleSlashCommand("agents", []string{"missing"}, "/agents missing") convContains(t, r.app, "[agent:missing not found]") @@ -1512,7 +1513,7 @@ func TestApp_Agents_NotFoundUsesAgentLabel(t *testing.T) { func TestApp_Agents_ViewDoesNotOverrideJobsPanelOverlay(t *testing.T) { r := newTestApp(t) mgr := wireJobsManagerForSlashTest(t, r) - defer func() { _ = mgr.Shutdown(2 * time.Second) }() + defer func() { _ = mgr.Shutdown(testwait.Timeout(2 * time.Second)) }() snap, spawnErr := mgr.Spawn(jobs.SpawnRequest{ Prompt: "inspect overlay", @@ -1646,7 +1647,7 @@ func TestApp_ResizeRecomposesBuiltInStatusLine(t *testing.T) { func TestApp_HandleJobUpdate_IgnoresStaleSeqAndDedupesTerminal(t *testing.T) { r := newTestApp(t) mgr := wireJobsManagerForSlashTest(t, r) - defer func() { _ = mgr.Shutdown(2 * time.Second) }() + defer func() { _ = mgr.Shutdown(testwait.Timeout(2 * time.Second)) }() snap, spawnErr := mgr.Spawn(jobs.SpawnRequest{ Prompt: "finish once", diff --git a/internal/jobs/abandoned_test.go b/internal/jobs/abandoned_test.go index ec7118d..879a1d9 100644 --- a/internal/jobs/abandoned_test.go +++ b/internal/jobs/abandoned_test.go @@ -9,6 +9,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/packetcode/packetcode/internal/testwait" "github.com/packetcode/packetcode/internal/tools" ) @@ -133,7 +134,7 @@ func TestCancel_RecordsRequestAndStaysCancelled(t *testing.T) { // loadPersistedJobs reconciles anything still marked running — so reading // the directory under a live manager would race the flush and rewrite the // very record under test. - require.NoError(t, mgr.Shutdown(2*time.Second)) + require.NoError(t, mgr.Shutdown(testwait.Timeout(2*time.Second))) reloaded, _, unread, lerr := loadPersistedJobs(jobsDir, "") require.NoError(t, lerr) diff --git a/internal/jobs/manager_test.go b/internal/jobs/manager_test.go index 231d3bf..bcb0663 100644 --- a/internal/jobs/manager_test.go +++ b/internal/jobs/manager_test.go @@ -18,6 +18,7 @@ import ( "github.com/packetcode/packetcode/internal/config" "github.com/packetcode/packetcode/internal/permissions" "github.com/packetcode/packetcode/internal/provider" + "github.com/packetcode/packetcode/internal/testwait" "github.com/packetcode/packetcode/internal/tools" ) @@ -322,7 +323,7 @@ func TestManager_Shutdown_Persists(t *testing.T) { return running == N }) - require.NoError(t, mgr.Shutdown(2*time.Second)) + require.NoError(t, mgr.Shutdown(testwait.Timeout(2*time.Second))) for _, id := range ids { snap, ok := mgr.Get(id) diff --git a/internal/jobs/persistence_test.go b/internal/jobs/persistence_test.go index 23dcdf8..64d4d70 100644 --- a/internal/jobs/persistence_test.go +++ b/internal/jobs/persistence_test.go @@ -10,6 +10,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/packetcode/packetcode/internal/testwait" ) // TestSaveSnapshot_AtomicWrite confirms the temp-file-then-rename @@ -175,7 +177,7 @@ func TestManagerPersistenceDebouncesNonterminalUpdates(t *testing.T) { dir := t.TempDir() mgr, _, err := NewManager(Config{JobsDir: dir}) require.NoError(t, err) - t.Cleanup(func() { _ = mgr.Shutdown(time.Second) }) + t.Cleanup(func() { _ = mgr.Shutdown(testwait.Timeout(time.Second)) }) mgr.persistDelay = time.Hour for seq := int64(1); seq <= 3; seq++ { @@ -197,7 +199,7 @@ func TestManagerPersistenceTerminalFlushIsSynchronous(t *testing.T) { dir := t.TempDir() mgr, _, err := NewManager(Config{JobsDir: dir}) require.NoError(t, err) - t.Cleanup(func() { _ = mgr.Shutdown(time.Second) }) + t.Cleanup(func() { _ = mgr.Shutdown(testwait.Timeout(time.Second)) }) mgr.persistDelay = time.Hour require.NoError(t, mgr.savePersistedSnapshot(persistedJob{ID: "terminal", State: "running", Seq: 1})) require.NoError(t, mgr.savePersistedSnapshot(persistedJob{ID: "terminal", State: "completed", Seq: 2, Summary: "done"})) @@ -219,7 +221,7 @@ func TestManagerShutdownFlushesPendingSnapshots(t *testing.T) { mgr.persistDelay = time.Hour require.NoError(t, mgr.savePersistedSnapshot(persistedJob{ID: "shutdown", State: "running", Seq: 7, LastMessage: "latest"})) - require.NoError(t, mgr.Shutdown(time.Second)) + require.NoError(t, mgr.Shutdown(testwait.Timeout(time.Second))) got, ok := readPersistedJob(filepath.Join(dir, "shutdown.json")) require.True(t, ok) assert.Equal(t, int64(7), got.Seq) diff --git a/internal/jobs/testhelpers_test.go b/internal/jobs/testhelpers_test.go index 9798f41..3ff5e49 100644 --- a/internal/jobs/testhelpers_test.go +++ b/internal/jobs/testhelpers_test.go @@ -197,8 +197,25 @@ func newTestManager(t *testing.T, prov provider.Provider, opts ...func(*Config)) } mgr, _, err := NewManager(cfg) require.NoError(t, err) + // Scaled, and reported rather than discarded. + // + // Shutdown returns when its timeout expires whether or not the workers + // stopped. Two seconds was ample on an idle machine and not ample under a + // parallel suite, and what happened then was not a slow test but a broken + // one: the workers were still running, still writing job records into the + // t.TempDir() Go was about to remove. On Windows a directory that is + // gaining files cannot be removed, so the test died in cleanup with + // "TempDir RemoveAll cleanup: ... The directory is not empty" -- naming + // neither the worker, nor the timeout, nor even the assertion it never + // reached. TestResubmit_IsOnlyAllowedOnce failed that way. + // + // The error says "N workers still running after ...", which is the one + // sentence that would have identified this immediately. Discarding it was + // how a shutdown that did not shut anything down looked like success. t.Cleanup(func() { - _ = mgr.Shutdown(2 * time.Second) + if err := mgr.Shutdown(testwait.Timeout(2 * time.Second)); err != nil { + t.Errorf("jobs manager did not shut down cleanly: %v", err) + } }) return mgr, tr }