diff --git a/apps/backend/internal/worktree/manager_cleanup.go b/apps/backend/internal/worktree/manager_cleanup.go index 019dd59a9d..d96c02a62a 100644 --- a/apps/backend/internal/worktree/manager_cleanup.go +++ b/apps/backend/internal/worktree/manager_cleanup.go @@ -275,6 +275,11 @@ func (m *Manager) removeWorktree(ctx context.Context, wt *Worktree, removeBranch if wt == nil { return errors.New("worktree cleanup requires a worktree") } + // Cleanup runs after the inventory snapshot has been selected. Keep a + // private copy so a later session/path projection update cannot redirect + // the destructive operation or its reference release. + worktreeSnapshot := *wt + wt = &worktreeSnapshot if strings.TrimSpace(wt.RepositoryPath) == "" || strings.TrimSpace(wt.Path) == "" { return errors.New("worktree cleanup requires repository and worktree paths") } @@ -572,6 +577,12 @@ func (m *Manager) removeWorktreeDir( if err := pathHandles[0].RemoveDirectory(ctx); err != nil { return fmt.Errorf("remove audited worktree directory: %w", err) } + // The parent cleanup is a separate path-based fallback. Revalidate the + // audited target immediately before it so a replacement cannot turn an + // otherwise safe cleanup into recursive deletion of another directory. + if err := pathHandles[0].VerifyPath(worktreePath); err != nil && !errors.Is(err, os.ErrNotExist) { + return fmt.Errorf("worktree path changed before parent cleanup: %w", err) + } pruneCmd := newGitCommand(ctx, "worktree", "prune") pruneCmd.Dir = repoPath if err := runGitCmd(ctx, pruneCmd); err != nil { diff --git a/apps/backend/internal/worktree/manager_cleanup_audit.go b/apps/backend/internal/worktree/manager_cleanup_audit.go index 8e3bd56e0f..0f2a12e564 100644 --- a/apps/backend/internal/worktree/manager_cleanup_audit.go +++ b/apps/backend/internal/worktree/manager_cleanup_audit.go @@ -43,7 +43,7 @@ func (m *Manager) auditWorktreeCleanup( _ = pathHandle.Close() } }() - branchRef, branchOID, err := m.cleanupBranchIdentity(ctx, wt) + branchRef, branchOID, err := m.cleanupBranchIdentity(ctx, wt, pathPresent, removeBranch) if err != nil { return worktreeCleanupAudit{}, err } @@ -119,12 +119,24 @@ func (m *Manager) openCleanupPathHandle( return handle, nil } -func (m *Manager) cleanupBranchIdentity(ctx context.Context, wt *Worktree) (string, string, error) { +func (m *Manager) cleanupBranchIdentity( + ctx context.Context, wt *Worktree, pathPresent, requireImmutableIdentity bool, +) (string, string, error) { branchRef := "" if wt.Branch != "" { branchRef = "refs/heads/" + wt.Branch } expectedOID := strings.TrimSpace(wt.CleanupHeadOID) + if expectedOID == "" && pathPresent { + output, err := m.runBoundedGitInspect(ctx, wt.Path, "rev-parse", "--verify", "HEAD^{commit}") + if err != nil { + return "", "", fmt.Errorf("capture cleanup worktree HEAD: %w", err) + } + expectedOID = strings.TrimSpace(output) + if expectedOID == "" { + return "", "", errors.New("capture cleanup worktree HEAD returned an empty commit") + } + } if branchRef == "" { return branchRef, expectedOID, nil } @@ -138,6 +150,9 @@ func (m *Manager) cleanupBranchIdentity(ctx context.Context, wt *Worktree) (stri // classifier will fail closed rather than deleting another checkout. return branchRef, "", nil } + if expectedOID == "" && requireImmutableIdentity { + return "", "", fmt.Errorf("cleanup branch %q has no immutable expected commit", wt.Branch) + } output, err := m.runBoundedGitInspect(ctx, wt.RepositoryPath, "rev-parse", "--verify", branchRef+"^{commit}") if err != nil { return "", "", fmt.Errorf("resolve cleanup branch %q: %w", wt.Branch, err) diff --git a/apps/backend/internal/worktree/manager_cleanup_identity_test.go b/apps/backend/internal/worktree/manager_cleanup_identity_test.go new file mode 100644 index 0000000000..b90e9c5484 --- /dev/null +++ b/apps/backend/internal/worktree/manager_cleanup_identity_test.go @@ -0,0 +1,160 @@ +package worktree + +import ( + "context" + "errors" + "os" + "path/filepath" + "strings" + "sync" + "testing" + + "github.com/kandev/kandev/internal/task/models" +) + +// blockingReferenceStore pauses cleanup after removeWorktree has copied the +// inventory record. This is reviewer-requested contract coverage for the +// snapshot boundary. +type blockingReferenceStore struct { + *SQLiteStore + entered chan struct{} + release chan struct{} + releaseOnce sync.Once +} + +func (s *blockingReferenceStore) CountActiveWorktreeReferences( + ctx context.Context, + _ string, + _ []string, +) (int, error) { + close(s.entered) + select { + case <-s.release: + return 0, nil + case <-ctx.Done(): + return 0, ctx.Err() + } +} + +func (s *blockingReferenceStore) unblock() { + s.releaseOnce.Do(func() { close(s.release) }) +} + +// postRemovalSwapDirectoryHandle moves the audited directory away before the +// second verification. It lets the test prove that parent cleanup is skipped +// when the lexical path no longer identifies the audited directory. +type postRemovalSwapDirectoryHandle struct { + path string + replacementPath string + verifyCalls int +} + +func (h *postRemovalSwapDirectoryHandle) Close() error { return nil } + +func (h *postRemovalSwapDirectoryHandle) VerifyPath(string) error { + h.verifyCalls++ + if h.verifyCalls == 1 { + return nil + } + return errors.New("pinned directory no longer matches lexical path") +} + +func (h *postRemovalSwapDirectoryHandle) IsValidWorktree() bool { return true } + +func (h *postRemovalSwapDirectoryHandle) RemoveDirectory(context.Context) error { + return os.Rename(h.path, h.replacementPath) +} + +func (h *postRemovalSwapDirectoryHandle) ReadFile(string) ([]byte, error) { + return nil, os.ErrNotExist +} + +func (h *postRemovalSwapDirectoryHandle) WriteFile(string, []byte, os.FileMode) error { + return errors.New("fake directory handle does not support writes") +} + +// Reviewer-requested contract coverage: the post-removal identity check must +// run before the best-effort parent-directory cleanup. +func TestRemoveWorktreeDir_RevalidatesBeforeParentCleanup(t *testing.T) { + mgr, _ := newReferenceCleanupTestManager(t) + tasksBase, err := mgr.config.ExpandedTasksBasePath() + if err != nil { + t.Fatalf("expand tasks base path: %v", err) + } + taskDir := filepath.Join(tasksBase, "task-post-removal") + worktreePath := filepath.Join(taskDir, "repository") + replacementPath := filepath.Join(t.TempDir(), "replacement") + if err := os.MkdirAll(worktreePath, 0o755); err != nil { + t.Fatalf("create worktree path: %v", err) + } + + handle := &postRemovalSwapDirectoryHandle{ + path: worktreePath, + replacementPath: replacementPath, + } + err = mgr.removeWorktreeDir( + context.Background(), + worktreePath, + filepath.Join(t.TempDir(), "repository.git"), + handle, + ) + if err == nil || !strings.Contains(err.Error(), "before parent cleanup") { + t.Fatalf("removeWorktreeDir error = %v, want post-removal identity failure", err) + } + if handle.verifyCalls != 2 { + t.Fatalf("VerifyPath calls = %d, want 2", handle.verifyCalls) + } + if _, err := os.Stat(replacementPath); err != nil { + t.Fatalf("renamed original worktree missing: %v", err) + } + if _, err := os.Stat(taskDir); err != nil { + t.Fatalf("parent task directory was removed after identity failure: %v", err) + } +} + +// Reviewer-requested contract coverage: cleanup must use the copied record +// even when a caller mutates its worktree projection while cleanup waits. +func TestCleanupWorktrees_UsesSnapshotWhenCallerMutatesPath(t *testing.T) { + mgr, store := newReferenceCleanupTestManager(t) + seedReferenceCleanupSession(t, store, "task-snapshot", "session-snapshot", models.TaskSessionStateCompleted) + wt := createReferenceCleanupWorktree(t, mgr, "task-snapshot", "session-snapshot") + originalPath := wt.Path + + blockingStore := &blockingReferenceStore{ + SQLiteStore: store, + entered: make(chan struct{}), + release: make(chan struct{}), + } + mgr.store = blockingStore + t.Cleanup(blockingStore.unblock) + + cleanupDone := make(chan error, 1) + go func() { + cleanupDone <- mgr.CleanupWorktrees(context.Background(), []*Worktree{wt}) + }() + <-blockingStore.entered + + replacementPath := originalPath + "-caller-replacement" + if err := os.MkdirAll(replacementPath, 0o755); err != nil { + blockingStore.unblock() + t.Fatalf("create caller replacement path: %v", err) + } + sentinel := filepath.Join(replacementPath, "sentinel.txt") + if err := os.WriteFile(sentinel, []byte("preserve me\n"), 0o644); err != nil { + blockingStore.unblock() + t.Fatalf("write caller replacement sentinel: %v", err) + } + wt.Path = replacementPath + blockingStore.unblock() + + if err := <-cleanupDone; err != nil { + t.Fatalf("cleanup after caller projection mutation: %v", err) + } + if _, err := os.Stat(originalPath); !os.IsNotExist(err) { + t.Fatalf("snapshotted worktree path remains, stat error = %v", err) + } + if got, err := os.ReadFile(sentinel); err != nil || string(got) != "preserve me\n" { + t.Fatalf("caller replacement changed: contents=%q err=%v", got, err) + } + assertWorktreeReferenceStatus(t, store, wt.ID, StatusDeleted) +} diff --git a/apps/backend/internal/worktree/manager_cleanup_recovery_test.go b/apps/backend/internal/worktree/manager_cleanup_recovery_test.go index 6c0f45ba12..cec35d2abf 100644 --- a/apps/backend/internal/worktree/manager_cleanup_recovery_test.go +++ b/apps/backend/internal/worktree/manager_cleanup_recovery_test.go @@ -2,6 +2,7 @@ package worktree import ( "context" + "errors" "os" "path/filepath" "strings" @@ -27,6 +28,35 @@ type cancellingCleanupScriptHandler struct { cancel context.CancelFunc } +type failingReleaseStore struct { + *SQLiteStore + failUpdate bool +} + +func (s *failingReleaseStore) UpdateWorktree(ctx context.Context, wt *Worktree) error { + if s.failUpdate { + return errors.New("injected release failure") + } + return s.SQLiteStore.UpdateWorktree(ctx, wt) +} + +type swappingCleanupScriptHandler struct{} + +func (h *swappingCleanupScriptHandler) ExecuteSetupScript(context.Context, ScriptExecutionRequest) error { + return nil +} + +func (h *swappingCleanupScriptHandler) ExecuteCleanupScript(_ context.Context, req ScriptExecutionRequest) error { + replacement := req.WorkingDir + "-replacement" + if err := os.Rename(req.WorkingDir, replacement); err != nil { + return err + } + if err := os.MkdirAll(req.WorkingDir, 0o755); err != nil { + return err + } + return os.WriteFile(filepath.Join(req.WorkingDir, "replacement.txt"), []byte("preserve me\n"), 0o644) +} + func (h *cancellingCleanupScriptHandler) ExecuteSetupScript(context.Context, ScriptExecutionRequest) error { return nil } @@ -40,6 +70,7 @@ func TestCleanupWorktrees_RecoversAfterPathOnlyRemoval(t *testing.T) { mgr, store := newReferenceCleanupTestManager(t) seedReferenceCleanupSession(t, store, "task-partial", "session-partial", models.TaskSessionStateCompleted) wt := createReferenceCleanupWorktree(t, mgr, "task-partial", "session-partial") + wt.CleanupHeadOID = strings.TrimSpace(runGit(t, wt.Path, "rev-parse", "HEAD")) if err := os.RemoveAll(wt.Path); err != nil { t.Fatalf("remove worktree path without shared Git metadata: %v", err) @@ -53,10 +84,71 @@ func TestCleanupWorktrees_RecoversAfterPathOnlyRemoval(t *testing.T) { assertWorktreeReferenceStatus(t, store, wt.ID, StatusDeleted) } +func TestCleanupWorktrees_RejectsPathlessRetryWithoutImmutableHead(t *testing.T) { + mgr, store := newReferenceCleanupTestManager(t) + seedReferenceCleanupSession(t, store, "task-no-snapshot", "session-no-snapshot", models.TaskSessionStateCompleted) + wt := createReferenceCleanupWorktree(t, mgr, "task-no-snapshot", "session-no-snapshot") + + if err := os.RemoveAll(wt.Path); err != nil { + t.Fatalf("remove worktree path without shared Git metadata: %v", err) + } + if err := mgr.CleanupWorktrees(context.Background(), []*Worktree{wt}); err == nil { + t.Fatal("cleanup accepted pathless worktree without immutable cleanup identity") + } + assertCleanupBranchPresent(t, wt.RepositoryPath, wt.Branch) + assertWorktreeReferenceStatus(t, store, wt.ID, StatusActive) +} + +func TestCleanupWorktreesPreservingBranches_RetriesAfterReleaseFailure(t *testing.T) { + mgr, store := newReferenceCleanupTestManager(t) + seedReferenceCleanupSession(t, store, "task-preserve-retry", "session-preserve-retry", models.TaskSessionStateCompleted) + wt := createReferenceCleanupWorktree(t, mgr, "task-preserve-retry", "session-preserve-retry") + + mgr.store = &failingReleaseStore{SQLiteStore: store, failUpdate: true} + if err := mgr.CleanupWorktreesPreservingBranches(context.Background(), []*Worktree{wt}); err == nil { + t.Fatal("cleanup succeeded despite injected reference-release failure") + } + if _, err := os.Stat(wt.Path); !os.IsNotExist(err) { + t.Fatalf("worktree path remains after first removal: %v", err) + } + assertCleanupBranchPresent(t, wt.RepositoryPath, wt.Branch) + + mgr.store = store + if err := mgr.CleanupWorktreesPreservingBranches(context.Background(), []*Worktree{wt}); err != nil { + t.Fatalf("retry pathless branch-preserving cleanup: %v", err) + } + assertCleanupBranchPresent(t, wt.RepositoryPath, wt.Branch) + assertWorktreeReferenceStatus(t, store, wt.ID, StatusDeleted) +} + +func TestCleanupWorktrees_PreservesReplacementAfterPostAuditPathSwap(t *testing.T) { + mgr, store := newReferenceCleanupTestManager(t) + seedReferenceCleanupSession(t, store, "task-path-swap", "session-path-swap", models.TaskSessionStateCompleted) + wt := createReferenceCleanupWorktree(t, mgr, "task-path-swap", "session-path-swap") + wt.CleanupHeadOID = strings.TrimSpace(runGit(t, wt.Path, "rev-parse", "HEAD")) + + mgr.SetRepositoryProvider(&fakeRepoProvider{repo: &Repository{ID: wt.RepositoryID, CleanupScript: "swap"}}) + mgr.SetScriptMessageHandler(&swappingCleanupScriptHandler{}) + if err := mgr.CleanupWorktrees(context.Background(), []*Worktree{wt}); err == nil { + t.Fatal("cleanup accepted a path replacement after audit") + } + + replacement := filepath.Join(filepath.Dir(wt.Path), filepath.Base(wt.Path)+"-replacement") + if got, err := os.ReadFile(filepath.Join(wt.Path, "replacement.txt")); err != nil || string(got) != "preserve me\n" { + t.Fatalf("replacement path changed: contents=%q err=%v", got, err) + } + if _, err := os.Stat(replacement); err != nil { + t.Fatalf("renamed original worktree missing: %v", err) + } + assertCleanupBranchPresent(t, wt.RepositoryPath, wt.Branch) + assertWorktreeReferenceStatus(t, store, wt.ID, StatusActive) +} + func TestCleanupWorktrees_PartialFailureRemainsRetryable(t *testing.T) { mgr, store := newReferenceCleanupTestManager(t) seedReferenceCleanupSession(t, store, "task-retry", "session-retry", models.TaskSessionStateCompleted) wt := createReferenceCleanupWorktree(t, mgr, "task-retry", "session-retry") + wt.CleanupHeadOID = strings.TrimSpace(runGit(t, wt.Path, "rev-parse", "HEAD")) if err := os.RemoveAll(wt.Path); err != nil { t.Fatalf("remove worktree path without shared Git metadata: %v", err) @@ -82,6 +174,7 @@ func TestCleanupWorktrees_IsIdempotentAfterVerifiedRemoval(t *testing.T) { mgr, store := newReferenceCleanupTestManager(t) seedReferenceCleanupSession(t, store, "task-idempotent", "session-idempotent", models.TaskSessionStateCompleted) wt := createReferenceCleanupWorktree(t, mgr, "task-idempotent", "session-idempotent") + wt.CleanupHeadOID = strings.TrimSpace(runGit(t, wt.Path, "rev-parse", "HEAD")) for attempt := 1; attempt <= 2; attempt++ { if err := mgr.CleanupWorktrees(context.Background(), []*Worktree{wt}); err != nil {