From b703b7ff49ec39cb9bf2a8bcea45b1d312d60643 Mon Sep 17 00:00:00 2001 From: Andrew Lee <5784515+andyrewlee@users.noreply.github.com> Date: Sun, 27 Sep 2026 11:20:39 +0900 Subject: [PATCH] e2e: own the shared fakeagent build directory and clean it up (plan 056) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The shared fake-agent binary was built into an amux-fakeagent-* temp dir that nothing owned: failed builds leaked partial outputs and the successful binary outlived every test process. Give the fixture an owner: the injectable build helper removes its owned dir immediately on failure, and TestMain removes the successful build after m.Run() — guarded to the owned prefix so cleanup can never sweep an unrelated path, and idempotent so a second call is a no-op. The shared binary intentionally gets no per-test cleanup: the parallel raw-carriage-return fixture depends on it for the process lifetime. Gates: cleanup/fixture/race suites green; verify-loop green with both raw-agent tests; isolated tmux/e2e + devcheck assertions all pass — the only failures are the known ambient e2e flakes under host load (TempDir RemoveAll teardown, one wheel-coordinate delivery timing), each proven non-regressions by isolated reruns. --- internal/e2e/fakeagent_cleanup_test.go | 149 +++++++++++++++++++++++++ internal/e2e/fakeagent_test.go | 61 ++++++++-- internal/e2e/main_test.go | 3 + plans/README.md | 2 +- 4 files changed, 206 insertions(+), 9 deletions(-) create mode 100644 internal/e2e/fakeagent_cleanup_test.go diff --git a/internal/e2e/fakeagent_cleanup_test.go b/internal/e2e/fakeagent_cleanup_test.go new file mode 100644 index 000000000..8329867bf --- /dev/null +++ b/internal/e2e/fakeagent_cleanup_test.go @@ -0,0 +1,149 @@ +package e2e + +import ( + "errors" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +// TestFakeAgentBuildFailureRemovesOwnedDir: a failed build removes the +// directory it created — including a partial binary — and nothing else. +func TestFakeAgentBuildFailureRemovesOwnedDir(t *testing.T) { + dest := t.TempDir() + var ownedDir string + _, err := buildFakeAgentBinary(dest, func(outPath string) error { + ownedDir = filepath.Dir(outPath) + // Simulate a partial build artifact before failing. + if werr := os.WriteFile(outPath, []byte("partial"), 0o755); werr != nil { + t.Fatalf("write partial: %v", werr) + } + return errors.New("build exploded") + }) + if err == nil { + t.Fatal("expected build error") + } + if ownedDir == "" || !strings.HasPrefix(filepath.Base(ownedDir), fakeAgentBuildDirPrefix) { + t.Fatalf("helper did not allocate an owned dir under %q: %q", dest, ownedDir) + } + if _, serr := os.Stat(ownedDir); !os.IsNotExist(serr) { + t.Fatalf("failed build left owned dir behind: %s", ownedDir) + } + // The destination root itself must survive. + if entries, rerr := os.ReadDir(dest); rerr != nil || len(entries) != 0 { + t.Fatalf("destination root disturbed: entries=%v err=%v", entries, rerr) + } +} + +// TestFakeAgentBuildSuccessKeepsOwnedDir: a successful build keeps its +// directory for the process-level cleanup to remove later. +func TestFakeAgentBuildSuccessKeepsOwnedDir(t *testing.T) { + dest := t.TempDir() + out, err := buildFakeAgentBinary(dest, func(outPath string) error { + return os.WriteFile(outPath, []byte("bin"), 0o755) + }) + if err != nil { + t.Fatalf("build: %v", err) + } + dir := filepath.Dir(out) + if _, serr := os.Stat(out); serr != nil { + t.Fatalf("successful build lost its binary: %v", serr) + } + // Ownership transferred: cleanup removes it, and a second call is a no-op. + if err := cleanupOwnedFakeAgentDir(out); err != nil { + t.Fatalf("cleanup: %v", err) + } + if _, serr := os.Stat(dir); !os.IsNotExist(serr) { + t.Fatalf("cleanup left the owned dir: %s", dir) + } + if err := cleanupOwnedFakeAgentDir(out); err != nil { + t.Fatalf("cleanup not idempotent: %v", err) + } +} + +// TestCleanupFakeAgentEmptyAndRefusal: empty path is a no-op; a directory +// outside the owned naming convention is refused and left intact. +func TestCleanupFakeAgentEmptyAndRefusal(t *testing.T) { + if err := cleanupOwnedFakeAgentDir(""); err != nil { + t.Fatalf("empty path should be a no-op, got %v", err) + } + + unrelated := t.TempDir() // not an amux-fakeagent-* name + sentinel := filepath.Join(unrelated, "sentinel.txt") + if err := os.WriteFile(sentinel, []byte("keep"), 0o644); err != nil { + t.Fatalf("write sentinel: %v", err) + } + err := cleanupOwnedFakeAgentDir(filepath.Join(unrelated, "fakeagent")) + if err == nil { + t.Fatal("expected refusal for unowned directory") + } + if _, serr := os.Stat(sentinel); serr != nil { + t.Fatalf("refusal deleted the unrelated dir anyway: %v", serr) + } +} + +// TestFakeAgentCleanupHelper runs only inside the subprocess spawned by +// TestFakeAgentCleanupAfterTestMain. It builds a fake binary through the real +// owned helper, records the directory for the parent, and relies on the +// process's TestMain to remove it after m.Run — proving process-lifetime +// cleanup, not per-test cleanup. +func TestFakeAgentCleanupHelper(t *testing.T) { + if os.Getenv("AMUX_E2E_FAKEAGENT_CLEANUP_HELPER") != "1" { + t.Skip("helper process only") + } + outFile := os.Getenv("AMUX_E2E_FAKEAGENT_DIR_OUT") + if outFile == "" { + t.Fatal("AMUX_E2E_FAKEAGENT_DIR_OUT unset") + } + out, err := buildFakeAgentBinary(os.TempDir(), func(outPath string) error { + return os.WriteFile(outPath, []byte("bin"), 0o755) + }) + if err != nil { + t.Fatalf("build: %v", err) + } + fakeAgentPath = out // this process's owned binary, as the real once-build sets it + dir := filepath.Dir(out) + if _, err := os.Stat(dir); err != nil { + t.Fatalf("owned dir missing before exit: %v", err) + } + if err := os.WriteFile(outFile, []byte(dir), 0o644); err != nil { + t.Fatalf("record dir: %v", err) + } +} + +// TestFakeAgentCleanupAfterTestMain runs the test binary as a subprocess +// executing only the helper test, with a private TMPDIR, and asserts the +// owned fakeagent directory is gone after normal TestMain exit. +func TestFakeAgentCleanupAfterTestMain(t *testing.T) { + helperTmp := t.TempDir() + outFile := filepath.Join(t.TempDir(), "dir.txt") + self, err := os.Executable() + if err != nil { + t.Fatalf("Executable: %v", err) + } + cmd := exec.Command(self, "-test.run=^TestFakeAgentCleanupHelper$", "-test.count=1") + cmd.Env = append(os.Environ(), + "AMUX_E2E_FAKEAGENT_CLEANUP_HELPER=1", + "AMUX_E2E_FAKEAGENT_DIR_OUT="+outFile, + "TMPDIR="+helperTmp, + ) + if combined, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("helper subprocess failed: %v\n%s", err, combined) + } + dirBytes, err := os.ReadFile(outFile) + if err != nil { + t.Fatalf("helper did not record its dir: %v", err) + } + dir := strings.TrimSpace(string(dirBytes)) + if !strings.HasPrefix(filepath.Base(dir), fakeAgentBuildDirPrefix) { + t.Fatalf("helper recorded unexpected dir %q", dir) + } + if filepath.Dir(dir) != helperTmp { + t.Fatalf("helper built outside its private TMPDIR: %q", dir) + } + if _, serr := os.Stat(dir); !os.IsNotExist(serr) { + t.Fatalf("owned fakeagent dir survived TestMain cleanup: %s", dir) + } +} diff --git a/internal/e2e/fakeagent_test.go b/internal/e2e/fakeagent_test.go index 9d517be5d..527252bf2 100644 --- a/internal/e2e/fakeagent_test.go +++ b/internal/e2e/fakeagent_test.go @@ -6,6 +6,7 @@ import ( "os" "os/exec" "path/filepath" + "strings" "sync" "testing" "time" @@ -19,6 +20,50 @@ var ( fakeAgentErr error ) +// fakeAgentBuildDirPrefix marks the directories this test process creates and +// owns for the shared fakeagent binary; the cleanup guard keys on it. +const fakeAgentBuildDirPrefix = "amux-fakeagent-" + +// buildFakeAgentBinary owns the fakeagent build directory lifecycle: it creates +// an amux-fakeagent-* directory under destRoot, runs the injected build step, +// and removes the directory immediately if the build fails. On success the +// directory is kept — the binary is process-shared, so process-level cleanup +// (cleanupBuiltFakeAgent from TestMain) removes it after every test finishes. +// It never creates or deletes caller-owned directories. +func buildFakeAgentBinary(destRoot string, runBuild func(outPath string) error) (string, error) { + dir, err := os.MkdirTemp(destRoot, fakeAgentBuildDirPrefix+"*") + if err != nil { + return "", err + } + out := filepath.Join(dir, "fakeagent") + if err := runBuild(out); err != nil { + _ = os.RemoveAll(dir) + return "", err + } + return out, nil +} + +// cleanupBuiltFakeAgent removes the shared fakeagent build directory after all +// tests in the process finish. It is a no-op when no binary was built and +// refuses paths outside the owned naming convention. +func cleanupBuiltFakeAgent() error { + return cleanupOwnedFakeAgentDir(fakeAgentPath) +} + +// cleanupOwnedFakeAgentDir removes the directory containing binPath after +// checking it carries this process's owned naming convention. An empty path is +// a no-op; an unrelated directory is refused, never removed. +func cleanupOwnedFakeAgentDir(binPath string) error { + if binPath == "" { + return nil + } + dir := filepath.Dir(binPath) + if !strings.HasPrefix(filepath.Base(dir), fakeAgentBuildDirPrefix) { + return fmt.Errorf("refusing to remove unexpected fakeagent directory %q", dir) + } + return os.RemoveAll(dir) +} + // buildFakeAgent compiles internal/e2e/fakeagent once per test binary and returns // the resulting executable path. Reused by the full close-the-loop E2E test. func buildFakeAgent(t *testing.T) string { @@ -29,18 +74,18 @@ func buildFakeAgent(t *testing.T) string { fakeAgentErr = err return } - dir, err := os.MkdirTemp("", "amux-fakeagent-*") + out, err := buildFakeAgentBinary(os.TempDir(), func(outPath string) error { + cmd := exec.Command("go", "build", "-o", outPath, "./internal/e2e/fakeagent") + cmd.Dir = root + if combined, err := cmd.CombinedOutput(); err != nil { + return fmt.Errorf("build fakeagent: %w\n%s", err, combined) + } + return nil + }) if err != nil { fakeAgentErr = err return } - out := filepath.Join(dir, "fakeagent") - cmd := exec.Command("go", "build", "-o", out, "./internal/e2e/fakeagent") - cmd.Dir = root - if combined, err := cmd.CombinedOutput(); err != nil { - fakeAgentErr = fmt.Errorf("build fakeagent: %w\n%s", err, combined) - return - } fakeAgentPath = out }) if fakeAgentErr != nil { diff --git a/internal/e2e/main_test.go b/internal/e2e/main_test.go index cf914cd84..6dfca1e37 100644 --- a/internal/e2e/main_test.go +++ b/internal/e2e/main_test.go @@ -20,5 +20,8 @@ func TestMain(m *testing.M) { if err := cleanupBuiltAmuxBinary(); err != nil { fmt.Fprintf(os.Stderr, "e2e binary cleanup: %v\n", err) } + if err := cleanupBuiltFakeAgent(); err != nil { + fmt.Fprintf(os.Stderr, "e2e fakeagent cleanup: %v\n", err) + } os.Exit(code) } diff --git a/plans/README.md b/plans/README.md index 007272139..c2cc630b7 100644 --- a/plans/README.md +++ b/plans/README.md @@ -40,7 +40,7 @@ clean baseline. All rows below mean plans ready for an executor, not fixes shipp | [053](053-replace-executable-without-path-gap.md) | Replace executable without a pathname gap | P2 | M | — | DONE | | [054](054-scroll-wrapped-diff-rows.md) | Scroll wrapped diff display rows | P2 | M | — | DONE | | [055](055-load-project-tree-asynchronously.md) | Load the project tree asynchronously | P2 | M | — | DONE | -| [056](056-clean-up-fakeagent-build-directory.md) | Clean up the shared fakeagent fixture | P3 | S | — | TODO | +| [056](056-clean-up-fakeagent-build-directory.md) | Clean up the shared fakeagent fixture | P3 | S | — | DONE | | [057](057-spike-script-output-search.md) | Spike: search retained script output | P3 | S | 038 | TODO | | [058](058-spike-workspace-recovery-diagnostics.md) | Spike: workspace recovery diagnostics | P3 | S | 039, 041, 048 | TODO | | [060](060-prefix-palette-after-restore-flake.md) | Prefix palette intermittently fails after restore | P1 | M | — | DONE — identity-drift mutation guard + stale-duplicate restore skip |