From d7f640f8aba4b29dfd58747077ee8014f8802b46 Mon Sep 17 00:00:00 2001 From: nova28 Date: Mon, 10 Aug 2026 01:21:44 +0800 Subject: [PATCH] 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 --- .../service/resource_cleanup_jobs_test.go | 48 +++++ .../resource_cleanup_task_owned_test.go | 171 ++++++++++++++++++ .../service/service_task_environments_test.go | 90 ++++++++- 3 files changed, 300 insertions(+), 9 deletions(-) diff --git a/apps/backend/internal/task/service/resource_cleanup_jobs_test.go b/apps/backend/internal/task/service/resource_cleanup_jobs_test.go index 2daccdf04a..1804601bcb 100644 --- a/apps/backend/internal/task/service/resource_cleanup_jobs_test.go +++ b/apps/backend/internal/task/service/resource_cleanup_jobs_test.go @@ -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() diff --git a/apps/backend/internal/task/service/resource_cleanup_task_owned_test.go b/apps/backend/internal/task/service/resource_cleanup_task_owned_test.go index c22aca5117..5efe83b758 100644 --- a/apps/backend/internal/task/service/resource_cleanup_task_owned_test.go +++ b/apps/backend/internal/task/service/resource_cleanup_task_owned_test.go @@ -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) + } +} diff --git a/apps/backend/internal/task/service/service_task_environments_test.go b/apps/backend/internal/task/service/service_task_environments_test.go index 0b75f645e3..f26a3b08da 100644 --- a/apps/backend/internal/task/service/service_task_environments_test.go +++ b/apps/backend/internal/task/service/service_task_environments_test.go @@ -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{})