Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 26 additions & 3 deletions internal/pool/pool.go
Original file line number Diff line number Diff line change
Expand Up @@ -122,8 +122,13 @@ func acquire(repoRoot, poolDir string, poolSize int, postCreate []string, opts a
if inUse {
continue
}
dirty, _ := git.IsDirty(wt.Path)
if dirty {
// Owner-process death does not make a slot safe to reuse. A crashed
// or rebooted agent can leave uncommitted changes or committed but
// unlanded work behind, and the reset below would discard it. Skip
// any slot that still holds work so the reset only ever runs on a
// clean, fully merged worktree; a slot whose state cannot be proven
// clean is preserved rather than reset.
if hasUnlandedWork(repoRoot, wt.Path) {
continue
}
// Found an available one — reset it
Expand All @@ -143,7 +148,7 @@ func acquire(repoRoot, poolDir string, poolSize int, postCreate []string, opts a

// No available worktree — create new if pool allows
if len(state.Worktrees) >= poolSize {
return fmt.Errorf("all %d worktrees are in use or dirty (max_trees = %d). Run 'treehouse status' to see details, or increase max_trees in treehouse.toml", len(state.Worktrees), poolSize)
return fmt.Errorf("all %d worktrees are in use, dirty, or hold unlanded work (max_trees = %d). Run 'treehouse status' to see details, or increase max_trees in treehouse.toml", len(state.Worktrees), poolSize)
}

name := nextName(state)
Expand Down Expand Up @@ -409,6 +414,24 @@ func ownerAlive(wt WorktreeEntry) bool {
return ok && startedAt == wt.OwnerStartedAt
}

// hasUnlandedWork reports whether the worktree at wtPath holds work that a reset
// would destroy: uncommitted changes (dirty), or commits that are not yet merged
// into the default branch (ahead of base). Owner-process death alone never makes
// such a slot safe to reuse. It fails closed: if either check cannot be
// evaluated, the worktree is treated as holding work so a reclaim never
// destructively resets a slot whose state cannot be proven clean.
func hasUnlandedWork(repoRoot, wtPath string) bool {
dirty, err := git.IsDirty(wtPath)
if err != nil || dirty {
return true
}
merged, _, err := git.IsHeadMergedIntoDefault(repoRoot, wtPath)
if err != nil || !merged {
return true
}
return false
}

func reserveOwner(wt *WorktreeEntry) error {
pid := int32(os.Getpid())
startedAt, ok := process.StartedAt(pid)
Expand Down
95 changes: 95 additions & 0 deletions internal/pool/pool_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2318,3 +2318,98 @@ func quoteForShell(p string) string {
// Double-quote works in both sh and cmd.exe for paths without quotes.
return `"` + p + `"`
}

// gitOutput runs git in dir and returns its trimmed stdout, failing the test on error.
func gitOutput(t *testing.T, dir string, args ...string) string {
t.Helper()
cmd := exec.Command("git", args...)
cmd.Dir = dir
out, err := cmd.CombinedOutput()
if err != nil {
t.Fatalf("git %s failed: %v\n%s", strings.Join(args, " "), err, out)
}
return strings.TrimSpace(string(out))
}

// A worktree that is clean but holds committed, unlanded work (ahead of the
// default branch) must not be destructively reset when its owner process is
// gone. Availability is keyed on live-owner presence, but owner-process death
// (a crash or reboot) does not make committed-but-unlanded work safe to discard.
// Acquire must skip such a slot and hand out a different one instead, preserving
// the unlanded commit.
func TestAcquire_PreservesUnlandedWorkWhenOwnerIsDead(t *testing.T) {
repoDir, poolDir := setupRepo(t)

wtPath, err := Acquire(repoDir, poolDir, 4, nil)
if err != nil {
t.Fatalf("Acquire failed: %v", err)
}

// Commit unlanded work on the worktree, leaving a clean working tree so the
// dirty check alone would not protect it.
if err := os.WriteFile(filepath.Join(wtPath, "unlanded.txt"), []byte("precious\n"), 0o644); err != nil {
t.Fatal(err)
}
runGit(t, wtPath, "add", "unlanded.txt")
runGit(t, wtPath, "commit", "-m", "unlanded work")
unlandedSHA := gitOutput(t, wtPath, "rev-parse", "HEAD")

// Simulate the owning agent crashing or the host rebooting: the recorded
// owner PID no longer refers to the live owner, so the slot reads as
// available even though it still holds the work.
state, err := ReadState(poolDir)
if err != nil {
t.Fatalf("ReadState failed: %v", err)
}
state.Worktrees[0].OwnerPID = 999999
if err := WriteState(poolDir, state); err != nil {
t.Fatalf("WriteState failed: %v", err)
}

next, err := Acquire(repoDir, poolDir, 4, nil)
if err != nil {
t.Fatalf("second Acquire failed: %v", err)
}
if next == wtPath {
t.Fatalf("Acquire reused the slot holding unlanded work (%s); it must be preserved", wtPath)
}

// The unlanded commit and its file must both survive untouched.
if got := gitOutput(t, wtPath, "rev-parse", "HEAD"); got != unlandedSHA {
t.Fatalf("unlanded worktree HEAD changed: expected %s, got %s (it was destructively reset)", unlandedSHA, got)
}
if _, err := os.Stat(filepath.Join(wtPath, "unlanded.txt")); err != nil {
t.Fatalf("unlanded file was destroyed: %v", err)
}
}

// The guard must not over-refuse: a clean, fully merged worktree whose owner is
// gone is genuinely reclaimable and must still be reused (reset and handed out),
// not needlessly preserved.
func TestAcquire_ReusesCleanMergedWorktreeWhenOwnerIsDead(t *testing.T) {
repoDir, poolDir := setupRepo(t)

wtPath, err := Acquire(repoDir, poolDir, 4, nil)
if err != nil {
t.Fatalf("Acquire failed: %v", err)
}

// Owner is gone, but the worktree is clean and at the default branch: no
// unlanded work, so it is safe to reclaim.
state, err := ReadState(poolDir)
if err != nil {
t.Fatalf("ReadState failed: %v", err)
}
state.Worktrees[0].OwnerPID = 999999
if err := WriteState(poolDir, state); err != nil {
t.Fatalf("WriteState failed: %v", err)
}

next, err := Acquire(repoDir, poolDir, 4, nil)
if err != nil {
t.Fatalf("second Acquire failed: %v", err)
}
if next != wtPath {
t.Fatalf("Acquire did not reuse the clean, merged, owner-dead slot: expected %s, got %s", wtPath, next)
}
}
Loading