Skip to content

fix(pool): preserve a reclaimed worktree that holds unlanded work - #80

Open
leecalcote wants to merge 1 commit into
kunchenguid:mainfrom
leecalcote:fm/get-preserve-unlanded-work
Open

fix(pool): preserve a reclaimed worktree that holds unlanded work#80
leecalcote wants to merge 1 commit into
kunchenguid:mainfrom
leecalcote:fm/get-preserve-unlanded-work

Conversation

@leecalcote

Copy link
Copy Markdown

Root cause

internal/pool/pool.go's acquire reuse loop keys a slot's reusability on live-owner presence and a dirty check, but not on whether the slot is ahead of its base. A worktree holding committed but unlanded work (a feature branch, or a detached HEAD ahead of the default branch) has a clean working tree, so git.IsDirty returns false and the slot passes the check. When its owner_pid no longer refers to a live process - a crashed agent, or a host reboot that leaves the state file with stale PIDs - ownerAlive returns false and the slot reads as available. acquire then calls ResetWorktree (checkout --detach --force + reset --hard + clean -fd), destroying the unlanded commits. Owner-process death does not make committed-but-unlanded work safe to discard.

Fix

  • Add hasUnlandedWork(repoRoot, wtPath): a slot holds unlanded work if it is dirty or its HEAD is not merged into the default branch (git.IsHeadMergedIntoDefault). It fails closed - if either check cannot be evaluated, the slot is treated as holding work, so a reclaim never destructively resets a slot whose state cannot be proven clean.
  • In acquire, replace the dirty-only skip with hasUnlandedWork, so the reuse reset only ever runs on a clean, fully merged worktree. A slot holding unlanded work is preserved; acquire uses another slot or creates a new one.
  • Update the pool-exhaustion error to read "in use, dirty, or hold unlanded work".

The default-branch merge ref comes from the existing git.IsHeadMergedIntoDefault / DefaultBranchMergeRef, which already fails closed against a stale remote tracking ref, so an ahead-of-base slot is never misclassified as reusable. acquire fetches origin before the reuse scan, so the tracking ref is fresh.

Verification

  • TestAcquire_PreservesUnlandedWorkWhenOwnerIsDead (new): a clean worktree holding a committed, unlanded change, with a dead owner PID, is not reused - Acquire hands out a different slot and the unlanded commit and its file survive untouched. Red-green: without the fix this test fails with "Acquire reused the slot holding unlanded work".
  • TestAcquire_ReusesCleanMergedWorktreeWhenOwnerIsDead (new): the guard does not over-refuse - a clean, fully merged worktree whose owner is gone is still reused.
  • gofmt -l . clean; go vet ./... passes; go test ./... passes.

Fixes the data-loss defect reported in #79.

acquire keyed a slot's reusability on live-owner presence and a dirty check,
but not on whether the slot was ahead of its base. A worktree holding committed
but unlanded work has a clean working tree, so IsDirty returned false and the
slot passed the check; when its owner_pid no longer referred to a live process
(a crashed agent, or a reboot leaving stale PIDs in the state file), acquire
reset it via checkout --detach --force + reset --hard + clean -fd, destroying
the unlanded commits.

Add hasUnlandedWork: a slot holds unlanded work if it is dirty or its HEAD is
not merged into the default branch. It fails closed - an indeterminate state is
treated as holding work - so the reuse reset only ever runs on a clean, fully
merged worktree. A slot holding unlanded work is preserved; acquire uses another
slot or creates a new one.

Adds red-green regression coverage: an ahead-of-base clean slot with a dead
owner is preserved, and a clean merged slot with a dead owner is still reused.

Signed-off-by: Lee Calcote <7570704+leecalcote@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant