diff --git a/internal/app/app_cancel_test.go b/internal/app/app_cancel_test.go index ae888f5..6ae2957 100644 --- a/internal/app/app_cancel_test.go +++ b/internal/app/app_cancel_test.go @@ -136,6 +136,31 @@ type drainPump struct { cmds []tea.Cmd } +// drainCancelledTurn cancels the in-flight turn and pumps until it has +// actually finished. +// +// Starting a turn starts a real agent goroutine, and that goroutine writes +// the session into a directory under t.TempDir(). Cancelling only signals +// it, so a test that returns straight after cancelling leaves its last +// write racing the TempDir cleanup. That is how +// TestLoopSelfPaced_StartedWhileStreamingKeepsOwnership failed on macOS, +// with "TempDir RemoveAll cleanup: ... sessions: directory not empty" and no +// mention of the test's own assertions. +// +// Draining cannot start a second turn: a cancelled turn ends its loop +// through stopLoopAfterFailedTurn rather than re-running the body. +func drainCancelledTurn(t *testing.T, a *App, cmd tea.Cmd) { + t.Helper() + pump := newDrainPump(t, a, cmd) + if a.cancelTurn != nil { + a.cancelTurn() + } + pump.RunUntil(2*time.Second, func() bool { return !a.streaming }) + if a.streaming { + t.Fatalf("turn did not finish after cancellation; its goroutine may still be writing into t.TempDir()") + } +} + func newDrainPump(t *testing.T, a *App, initial tea.Cmd) *drainPump { t.Helper() return &drainPump{t: t, app: a, cmds: []tea.Cmd{initial}} diff --git a/internal/app/app_slashcmd_test.go b/internal/app/app_slashcmd_test.go index bda975f..5b9d62b 100644 --- a/internal/app/app_slashcmd_test.go +++ b/internal/app/app_slashcmd_test.go @@ -350,10 +350,23 @@ func convText(a *App) string { return a.conversation.View() } +// flattenSpace collapses every run of whitespace into a single space. +func flattenSpace(s string) string { return strings.Join(strings.Fields(s), " ") } + +// convContains asserts on the conversation without caring where the view +// soft-wrapped. +// +// The pane wraps to its width and the break lands wherever the message +// happens to run long, which depends on what the message embeds. A +// t.TempDir() path is the usual culprit and its length differs per +// platform, so a literal assertion straddling the break fails for a reason +// unrelated to the behaviour under test. That is precisely how +// "depth now: 0" failed on Linux while passing on Windows: every character +// was present, with the wrap sitting between "depth" and "now:". func convContains(t *testing.T, a *App, needle string) { t.Helper() - if !strings.Contains(convText(a), needle) { - t.Fatalf("conversation does not contain %q; got:\n%s", needle, convText(a)) + if !strings.Contains(flattenSpace(convText(a)), flattenSpace(needle)) { + t.Fatalf("conversation does not contain %q (ignoring wrapping); got:\n%s", needle, convText(a)) } } diff --git a/internal/app/slashcmd_loop_test.go b/internal/app/slashcmd_loop_test.go index 048e34c..3619b9d 100644 --- a/internal/app/slashcmd_loop_test.go +++ b/internal/app/slashcmd_loop_test.go @@ -275,13 +275,11 @@ func TestLoopSelfPaced_StartedWhileStreamingKeepsOwnership(t *testing.T) { // hangs rather than completing: the claim happens as the turn begins. wireAgent(r, &hangingProvider{}) r.app.streaming = false - r.app.startNextQueuedInput() - if r.app.cancelTurn != nil { - defer r.app.cancelTurn() - } + _, cmd := r.app.startNextQueuedInput() if r.app.activeLoopID != "loop1" { t.Fatalf("activeLoopID = %q after the queued turn started; the loop is orphaned", r.app.activeLoopID) } + drainCancelledTurn(t, r.app, cmd) } // The immediate path must behave identically -- one builder, two callers. @@ -295,10 +293,7 @@ func TestLoopSelfPaced_StartedIdleClaimsOwnership(t *testing.T) { wireAgent(r, &hangingProvider{}) r.app.streaming = false - r.app.runLoopBody(ls) - if r.app.cancelTurn != nil { - defer r.app.cancelTurn() - } + cmd := r.app.runLoopBody(ls) if r.app.activeLoopID != "loop2" { t.Fatalf("activeLoopID = %q, want loop2", r.app.activeLoopID) @@ -306,6 +301,7 @@ func TestLoopSelfPaced_StartedIdleClaimsOwnership(t *testing.T) { if len(r.app.queuedInputs) != 0 { t.Fatalf("an idle start should not queue: %d queued", len(r.app.queuedInputs)) } + drainCancelledTurn(t, r.app, cmd) } // An interval loop is driven by its ticker, not by turn completion, so it must diff --git a/internal/procrun/process_posix_test.go b/internal/procrun/process_posix_test.go index 950a9e8..02cb252 100644 --- a/internal/procrun/process_posix_test.go +++ b/internal/procrun/process_posix_test.go @@ -65,7 +65,7 @@ func TestTrackedTreeKillsDescendantAfterRootExits(t *testing.T) { // A group with nothing in it must report Confirmed rather than an error: the // caller asked for the tree to be gone and it is gone. func TestKillTreeOnExitedProcessIsConfirmed(t *testing.T) { - cmd := exec.Command(os.Args[0], "-test.run=^$") + cmd := exec.CommandContext(context.Background(), os.Args[0], "-test.run=^$") ConfigureTreeCancel(cmd) require.NoError(t, cmd.Start()) require.NoError(t, cmd.Wait()) @@ -79,7 +79,7 @@ func TestKillTreeOnExitedProcessIsConfirmed(t *testing.T) { // A live tree torn down before the root is reaped cannot be confirmed, and the // reason must say why rather than leaving the caller to guess. func TestKillTreeBeforeReapIsUnconfirmedWithReason(t *testing.T) { - cmd := exec.Command(os.Args[0], "-test.run=^TestTrackedTreeKillsDescendantAfterRootExits$") + cmd := exec.CommandContext(context.Background(), os.Args[0], "-test.run=^TestTrackedTreeKillsDescendantAfterRootExits$") cmd.Env = append(os.Environ(), "PACKETCODE_PROCRUN_TEST_ROLE=sleeper") ConfigureTreeCancel(cmd) require.NoError(t, cmd.Start()) diff --git a/internal/procrun/run.go b/internal/procrun/run.go index 6a83217..ce2cd7e 100644 --- a/internal/procrun/run.go +++ b/internal/procrun/run.go @@ -53,12 +53,19 @@ type KillOutcome struct { // possibly incomplete. func (o KillOutcome) Unconfirmed() bool { return !o.Confirmed } +// ConfigureTreeCancel installs process-tree teardown on cancellation. +// +// cmd must have been created with exec.CommandContext. os/exec refuses to +// start a command that has a Cancel func but no context, with "command with +// a non-nil Cancel was not created with CommandContext", and that refusal +// surfaces at Start rather than here. func ConfigureTreeCancel(cmd *exec.Cmd) { _ = ConfigureTreeCancelRecorder(cmd) } // ConfigureTreeCancelRecorder is ConfigureTreeCancel that captures the -// evidence from the teardown os/exec performs on cancellation. +// evidence from the teardown os/exec performs on cancellation. The same +// exec.CommandContext requirement applies. // // The teardown happens inside a callback os/exec owns, so without this the // outcome is produced and immediately discarded — which is why callers could