fix(worktree): destroy every repo's worktree during multi-repo task cleanup (#2460)

Co-authored-by: nova28 <17953305+nova28@users.noreply.github.com> Co-authored-by: Carlos Florencio <carlosmflorencio@gmail.com>
2026-08-10 01:21:44 +08:00
parent f4bc7f0a8c
commit d7f640f8ab
3 changed files with 300 additions and 9 deletions
@@ -7,6 +7,7 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"reflect"
 	"sync"
 	"testing"
 	"time"
@@ -1126,6 +1127,53 @@ func TestTaskResourceCleanupMissingResourcesSucceed(t *testing.T) {
 	}
 }

+// TestTaskResourceCleanupJobSnapshotRoundTripsMultiRepoWorktrees proves that
+// the durable cleanup job preserves every task-environment repository through
+// JSON snapshot encoding and processing. A future change to the snapshot
+// shape or inventory loading must not silently drop Repos[] and skip a
+// repository's worktree teardown.
+func TestTaskResourceCleanupJobSnapshotRoundTripsMultiRepoWorktrees(t *testing.T) {
+	taskSvc, repo := setupOfficeTest(t)
+	taskSvc.StopTaskResourceCleanupWorker()
+	destroyer := &stubDestroyer{}
+	taskSvc.SetEnvironmentDestroyer(destroyer)
+
+	env := &models.TaskEnvironment{
+		ID: "env-multi-snapshot",
+		Repos: []*models.TaskEnvironmentRepo{
+			{RepositoryID: "repo-a", WorktreeID: "wt-primary"},
+			{RepositoryID: "repo-b", WorktreeID: "wt-secondary"},
+		},
+	}
+	snapshot, err := json.Marshal(taskResourceCleanupSnapshot{
+		TaskEnvironment:      env,
+		DeleteEnvironmentRow: true,
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+	job := &models.TaskResourceCleanupJob{
+		ID:               "multi-repo-snapshot-roundtrip",
+		OperationID:      "cascade_delete:multi-repo-snapshot:roundtrip",
+		TaskID:           "deleted-multi-repo-task",
+		Trigger:          models.TaskResourceCleanupTriggerCascadeDelete,
+		State:            models.TaskResourceCleanupStatePending,
+		ResourceSnapshot: string(snapshot),
+	}
+	if err := repo.CreateTaskResourceCleanupJob(context.Background(), job); err != nil {
+		t.Fatalf("CreateTaskResourceCleanupJob: %v", err)
+	}
+
+	if err := taskSvc.processTaskResourceCleanupJob(context.Background(), job.ID); err != nil {
+		t.Fatalf("processTaskResourceCleanupJob: %v", err)
+	}
+
+	want := []string{"wt-primary", "wt-secondary"}
+	if got := destroyer.worktreeCalls; !reflect.DeepEqual(got, want) {
+		t.Fatalf("worktree destroy calls = %v, want %v (Repos[] did not survive the snapshot round trip)", got, want)
+	}
+}
+
 func TestCancelWorkspaceDeleteCleanupUsesDetachedContext(t *testing.T) {
 	taskSvc, repo := setupOfficeTest(t)
 	ctx := context.Background()
@@ -3,6 +3,10 @@ package service
 import (
 	"context"
 	"encoding/json"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"strings"
 	"testing"

 	"github.com/jmoiron/sqlx"
@@ -13,6 +17,30 @@ import (
 	"github.com/kandev/kandev/internal/worktree"
 )

+type managerEnvironmentDestroyer struct {
+	mgr *worktree.Manager
+}
+
+func (d *managerEnvironmentDestroyer) DestroyContainer(context.Context, string) error {
+	return nil
+}
+
+func (d *managerEnvironmentDestroyer) DestroySandbox(context.Context, string, string) error {
+	return nil
+}
+
+func (d *managerEnvironmentDestroyer) DestroyWorktree(ctx context.Context, worktreeID string) error {
+	return d.mgr.RemoveByID(ctx, worktreeID, true)
+}
+
+func (d *managerEnvironmentDestroyer) PushEnvironmentBranch(context.Context, *models.TaskEnvironment) error {
+	return nil
+}
+
+func (d *managerEnvironmentDestroyer) GetContainerLiveStatus(context.Context, string) (*ContainerLiveStatus, error) {
+	return nil, nil
+}
+
 // TestDeleteTaskCleanupFindsWorktreeAfterLastSessionDeletedAndRestart is the
 // core task-owned cleanup contract: after the task's only session is deleted
 // and the backend restarts (a fresh service over the same database), delete
@@ -80,6 +108,107 @@ func TestDeleteTaskCleanupFindsWorktreeAfterLastSessionDeletedAndRestart(t *test
 	}
 }

+// TestDeleteTaskCleanupRemovesEveryWorktreeAfterLastSessionDeletedAndRestart
+// covers the complete task-owned lifecycle for a multi-repository task. The
+// session rows disappear first, then a fresh service inventories both durable
+// environment-repository rows, persists them in the delete snapshot, and
+// finally removes both worktrees through the normal batch cleanup path.
+func TestDeleteTaskCleanupRemovesEveryWorktreeAfterLastSessionDeletedAndRestart(t *testing.T) {
+	ctx := context.Background()
+	_, _, repo := createTestService(t)
+	taskID := "task-multi-repo-cleanup"
+	sessionID := "session-multi-repo-cleanup"
+	seedCleanupTaskAndSession(t, repo, taskID, sessionID)
+
+	repoAPath := initSimpleGitRepo(t)
+	repoBPath := initSimpleGitRepo(t)
+	workspaceID := "ws-" + taskID
+	for id, path := range map[string]string{"repo-a": repoAPath, "repo-b": repoBPath} {
+		if err := repo.CreateRepository(ctx, &models.Repository{
+			ID: id, WorkspaceID: workspaceID, Name: id, SourceType: "local", LocalPath: path,
+		}); err != nil {
+			t.Fatalf("create repository %s: %v", id, err)
+		}
+	}
+
+	mgr := newCleanupTestWorktreeManager(t, repo)
+	primary, err := mgr.Create(ctx, worktree.CreateRequest{
+		TaskID: taskID, SessionID: sessionID, TaskTitle: "Multi repo cleanup",
+		RepositoryID: "repo-a", RepositoryPath: repoAPath,
+		BaseBranch: "main", TaskDirName: taskID, RepoName: "repo-a",
+	})
+	if err != nil {
+		t.Fatalf("create primary worktree: %v", err)
+	}
+	secondary, err := mgr.Create(ctx, worktree.CreateRequest{
+		TaskID: taskID, SessionID: sessionID, TaskTitle: "Multi repo cleanup",
+		RepositoryID: "repo-b", RepositoryPath: repoBPath,
+		BaseBranch: "main", TaskDirName: taskID, RepoName: "repo-b",
+	})
+	if err != nil {
+		t.Fatalf("create secondary worktree: %v", err)
+	}
+
+	if err := repo.CreateTaskEnvironment(ctx, &models.TaskEnvironment{
+		ID: "env-" + taskID, TaskID: taskID, ExecutorType: "worktree",
+		WorkspacePath: filepath.Dir(primary.Path), Status: models.TaskEnvironmentStatusReady,
+		Repos: []*models.TaskEnvironmentRepo{
+			{ID: "env-repo-a", RepositoryID: "repo-a", WorktreeID: primary.ID, WorktreePath: primary.Path, WorktreeBranch: primary.Branch, Status: "active"},
+			{ID: "env-repo-b", RepositoryID: "repo-b", WorktreeID: secondary.ID, WorktreePath: secondary.Path, WorktreeBranch: secondary.Branch, Status: "active"},
+		},
+	}); err != nil {
+		t.Fatalf("create task environment: %v", err)
+	}
+	session, err := repo.GetTaskSession(ctx, sessionID)
+	if err != nil {
+		t.Fatalf("get task session: %v", err)
+	}
+	session.TaskEnvironmentID = "env-" + taskID
+	if err := repo.UpdateTaskSession(ctx, session); err != nil {
+		t.Fatalf("link task session: %v", err)
+	}
+	if err := repo.DeleteTaskSession(ctx, sessionID); err != nil {
+		t.Fatalf("delete task session: %v", err)
+	}
+
+	// Use fresh service and manager instances over the same database to model a
+	// backend restart after the last session has disappeared.
+	svc := newServiceOverRepository(t, repo)
+	cleanupMgr := newCleanupTestWorktreeManager(t, repo)
+	svc.SetWorktreeCleanup(cleanupMgr)
+	svc.SetEnvironmentDestroyer(&managerEnvironmentDestroyer{mgr: cleanupMgr})
+	worktrees, err := svc.gatherWorktreesForDelete(ctx, taskID)
+	if err != nil {
+		t.Fatalf("gather worktrees: %v", err)
+	}
+	if len(worktrees) != 2 {
+		t.Fatalf("zero-session inventory = %+v, want both repository worktrees", worktrees)
+	}
+
+	if err := svc.DeleteTask(ctx, taskID); err != nil {
+		t.Fatalf("delete task: %v", err)
+	}
+	var jobID string
+	if err := repo.DB().QueryRowContext(ctx, `
+		SELECT id FROM task_resource_cleanup_jobs
+		WHERE task_id = ? AND trigger = 'delete'
+		ORDER BY created_at DESC LIMIT 1
+	`, taskID).Scan(&jobID); err != nil {
+		t.Fatalf("load delete cleanup job: %v", err)
+	}
+	if err := svc.processTaskResourceCleanupJob(ctx, jobID); err != nil {
+		t.Fatalf("process delete cleanup job: %v", err)
+	}
+
+	for name, path := range map[string]string{"primary": primary.Path, "secondary": secondary.Path} {
+		if _, statErr := os.Stat(path); !os.IsNotExist(statErr) {
+			t.Errorf("%s worktree directory still on disk: %s (stat err = %v)", name, path, statErr)
+		}
+	}
+	assertNoWorktreeRegistration(t, repoAPath, primary.Path)
+	assertNoWorktreeRegistration(t, repoBPath, secondary.Path)
+}
+
 // TestArchiveCleanupFindsWorktreeAfterLastSessionDeleted proves the archive
 // path inventories the task-owned worktree with zero sessions and schedules
 // its asynchronous removal.
@@ -181,3 +310,45 @@ func newServiceOverRepository(t *testing.T, repo *sqliterepo.Repository) *Servic
 	svc.SetWorkflowStepGetter(&testWorkflowStepGetter{repo: repo})
 	return svc
 }
+
+func initSimpleGitRepo(t *testing.T) string {
+	t.Helper()
+	repoPath := t.TempDir()
+	runGitTestCmd(t, repoPath, "init", "-b", "main")
+	runGitTestCmd(t, repoPath, "config", "user.email", "test@example.com")
+	runGitTestCmd(t, repoPath, "config", "user.name", "Test User")
+	runGitTestCmd(t, repoPath, "config", "commit.gpgsign", "false")
+	readme := filepath.Join(repoPath, "README.md")
+	if err := os.WriteFile(readme, []byte("initial\n"), 0644); err != nil {
+		t.Fatalf("write README.md: %v", err)
+	}
+	runGitTestCmd(t, repoPath, "add", "README.md")
+	runGitTestCmd(t, repoPath, "commit", "-m", "initial commit")
+	return repoPath
+}
+
+func runGitTestCmd(t *testing.T, repoPath string, args ...string) []byte {
+	t.Helper()
+	cmd := exec.Command("git", args...)
+	cmd.Dir = repoPath
+	cmd.Env = append(os.Environ(),
+		"GIT_CONFIG_NOSYSTEM=1",
+		"GIT_AUTHOR_NAME=Test User",
+		"GIT_AUTHOR_EMAIL=test@example.com",
+		"GIT_COMMITTER_NAME=Test User",
+		"GIT_COMMITTER_EMAIL=test@example.com",
+	)
+	out, err := cmd.CombinedOutput()
+	if err != nil {
+		t.Fatalf("git %s failed: %v\n%s", strings.Join(args, " "), err, out)
+	}
+	return out
+}
+
+func assertNoWorktreeRegistration(t *testing.T, repoPath, worktreePath string) {
+	t.Helper()
+	out := string(runGitTestCmd(t, repoPath, "worktree", "list", "--porcelain"))
+	if strings.Contains(out, worktreePath) {
+		t.Errorf("stale worktree registration for %s remains in source repo %s:\n%s", worktreePath, repoPath, out)
+	}
+}
@@ -53,15 +53,16 @@ func (s *stubEnvRepo) DeleteTaskEnvironment(context.Context, string) error {
 func (s *stubEnvRepo) DeleteTaskEnvironmentsByTask(context.Context, string) error { return nil }

 type stubDestroyer struct {
-	containerCalls       []string
-	sandboxCalls         []string
-	worktreeCalls        []string
-	cancelAfterContainer context.CancelFunc
-	pushCalls            int
-	containerErr         error
-	sandboxErr           error
-	worktreeErr          error
-	pushErr              error
+	containerCalls           []string
+	sandboxCalls             []string
+	worktreeCalls            []string
+	cancelAfterContainer     context.CancelFunc
+	cancelAfterFirstWorktree context.CancelFunc
+	pushCalls                int
+	containerErr             error
+	sandboxErr               error
+	worktreeErr              error
+	pushErr                  error
 }

 func (s *stubDestroyer) DestroyContainer(_ context.Context, id string) error {
@@ -77,6 +78,9 @@ func (s *stubDestroyer) DestroySandbox(_ context.Context, id, _ string) error {
 }
 func (s *stubDestroyer) DestroyWorktree(_ context.Context, id string) error {
 	s.worktreeCalls = append(s.worktreeCalls, id)
+	if s.cancelAfterFirstWorktree != nil && len(s.worktreeCalls) == 1 {
+		s.cancelAfterFirstWorktree()
+	}
 	return s.worktreeErr
 }
 func (s *stubDestroyer) PushEnvironmentBranch(context.Context, *models.TaskEnvironment) error {
@@ -202,6 +206,74 @@ func TestTeardownEnvironmentResources_CancellationStopsBeforeNextResource(t *tes
 	}
 }

+func TestTeardownEnvironmentResources_MultiRepoCancellationStopsBeforeNextWorktree(t *testing.T) {
+	svc := newResetTestService(t, &stubEnvRepo{})
+	ctx, cancel := context.WithCancel(context.Background())
+	destroyer := &stubDestroyer{cancelAfterFirstWorktree: cancel}
+	svc.SetEnvironmentDestroyer(destroyer)
+
+	err := svc.teardownEnvironmentResources(ctx, &models.TaskEnvironment{
+		Repos: []*models.TaskEnvironmentRepo{
+			{RepositoryID: "repo-a", WorktreeID: "wt-first"},
+			{RepositoryID: "repo-b", WorktreeID: "wt-second"},
+		},
+	})
+
+	if !errors.Is(err, context.Canceled) {
+		t.Fatalf("teardown error = %v, want context cancellation", err)
+	}
+	if len(destroyer.worktreeCalls) != 1 || destroyer.worktreeCalls[0] != "wt-first" {
+		t.Fatalf("expected only the first worktree destroyed before cancellation, got %v", destroyer.worktreeCalls)
+	}
+}
+
+func TestTeardownEnvironmentResources_MultiRepoDestroysEveryWorktree(t *testing.T) {
+	svc := newResetTestService(t, &stubEnvRepo{})
+	destroyer := &stubDestroyer{}
+	svc.SetEnvironmentDestroyer(destroyer)
+
+	err := svc.teardownEnvironmentResources(context.Background(), &models.TaskEnvironment{
+		Repos: []*models.TaskEnvironmentRepo{
+			{RepositoryID: "repo-a", WorktreeID: "wt-primary"},
+			{RepositoryID: "repo-b", WorktreeID: "wt-secondary"},
+			{RepositoryID: "repo-c", WorktreeID: "wt-tertiary"},
+		},
+	})
+
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	want := []string{"wt-primary", "wt-secondary", "wt-tertiary"}
+	if len(destroyer.worktreeCalls) != len(want) {
+		t.Fatalf("worktree destroy calls = %v, want %v", destroyer.worktreeCalls, want)
+	}
+	for i, id := range want {
+		if destroyer.worktreeCalls[i] != id {
+			t.Errorf("worktree destroy call[%d] = %q, want %q", i, destroyer.worktreeCalls[i], id)
+		}
+	}
+}
+
+func TestTeardownEnvironmentResources_ReposOnlyEnvironmentIsNotEmpty(t *testing.T) {
+	svc := newResetTestService(t, &stubEnvRepo{})
+	destroyer := &stubDestroyer{}
+	svc.SetEnvironmentDestroyer(destroyer)
+
+	err := svc.teardownEnvironmentResources(context.Background(), &models.TaskEnvironment{
+		Repos: []*models.TaskEnvironmentRepo{
+			{RepositoryID: "repo-a", WorktreeID: "wt-a"},
+			{RepositoryID: "repo-b", WorktreeID: "wt-b"},
+		},
+	})
+
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if len(destroyer.worktreeCalls) != 2 {
+		t.Fatalf("worktree destroy calls = %v, want 2", destroyer.worktreeCalls)
+	}
+}
+
 func TestTeardownEnvironmentResources_GenericWorktreeFailureRemainsError(t *testing.T) {
 	worktreeErr := errors.New("worktree backend unavailable")
 	svc := newResetTestService(t, &stubEnvRepo{})