fix: make worktree cleanup auditable and retryable (#3178)
Co-authored-by: ayattara <alassane.yattara@savoirfairelinux.com> Co-authored-by: Carlos Florencio <carlosmflorencio@gmail.com>
This commit is contained in:
@@ -36,6 +36,46 @@ func (e *Executor) Stop(ctx context.Context, sessionID string, reason string, fo
|
||||
return e.stopWithSession(ctx, session, reason, force)
|
||||
}
|
||||
|
||||
// StopSessionSynchronously cancels a session and waits for its agent process to
|
||||
// stop. Durable task cleanup uses this optional capability before it removes a
|
||||
// worktree, so an agent cannot continue writing into the audited directory
|
||||
// after cleanup starts. Interactive callers keep the legacy asynchronous Stop
|
||||
// behavior.
|
||||
func (e *Executor) StopSessionSynchronously(ctx context.Context, sessionID, reason string, force bool) error {
|
||||
session, err := e.repo.GetTaskSession(ctx, sessionID)
|
||||
if err != nil {
|
||||
if !errors.Is(err, models.ErrTaskSessionNotFound) {
|
||||
return ErrExecutionNotFound
|
||||
}
|
||||
return e.stopMissingSessionSynchronously(ctx, sessionID, reason, force, err)
|
||||
}
|
||||
result, err := e.StopSessionDetailed(ctx, session, reason, force)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
if result.ExecutionID == "" {
|
||||
return nil
|
||||
}
|
||||
return e.StopExecution(ctx, result.ExecutionID, reason, force)
|
||||
}
|
||||
|
||||
func (e *Executor) stopMissingSessionSynchronously(
|
||||
ctx context.Context, sessionID, reason string, force bool, sessionErr error,
|
||||
) error {
|
||||
// Task deletion can remove the session row before the durable cleanup
|
||||
// worker runs. Still ask the lifecycle manager for the exact execution so a
|
||||
// late process cannot keep writing to the worktree.
|
||||
executionID, lookupErr := e.agentManager.GetExecutionIDForSession(ctx, sessionID)
|
||||
switch {
|
||||
case lookupErr == nil && executionID != "":
|
||||
return e.StopExecution(ctx, executionID, reason, force)
|
||||
case executionID == "" || errors.Is(lookupErr, lifecycle.ErrNoExecutionForSession):
|
||||
return fmt.Errorf("%w: %w: %w", ErrExecutionNotFound, runtimeapi.ErrNotFound, sessionErr)
|
||||
default:
|
||||
return fmt.Errorf("%w: lookup execution for session %q: %w", ErrExecutionNotFound, sessionID, lookupErr)
|
||||
}
|
||||
}
|
||||
|
||||
// SessionStopResult describes the synchronous, logical portion of a stop. A
|
||||
// true Changed value means CANCELLED was accepted and runtime teardown is ready
|
||||
// to be scheduled. FinalState is empty when no live execution exists.
|
||||
|
||||
@@ -148,6 +148,64 @@ func TestStop_MissingSessionReportsRuntimeNotFound(t *testing.T) {
|
||||
})
|
||||
}
|
||||
|
||||
func TestStopSessionSynchronouslyWaitsForAgentTeardown(t *testing.T) {
|
||||
repo := newMockRepository()
|
||||
repo.sessions["session-sync"] = &models.TaskSession{
|
||||
ID: "session-sync", TaskID: "task-sync", State: models.TaskSessionStateRunning,
|
||||
}
|
||||
stopCalls := make(chan string, 1)
|
||||
manager := &mockAgentManager{
|
||||
getExecutionIDForSessionFunc: func(context.Context, string) (string, error) {
|
||||
return "execution-sync", nil
|
||||
},
|
||||
stopAgentWithReasonFunc: func(_ context.Context, executionID, _ string, _ bool) error {
|
||||
stopCalls <- executionID
|
||||
return nil
|
||||
},
|
||||
}
|
||||
exec := newTestExecutor(t, manager, repo)
|
||||
|
||||
if err := exec.StopSessionSynchronously(context.Background(), "session-sync", "cleanup", true); err != nil {
|
||||
t.Fatalf("StopSessionSynchronously: %v", err)
|
||||
}
|
||||
select {
|
||||
case executionID := <-stopCalls:
|
||||
if executionID != "execution-sync" {
|
||||
t.Fatalf("stopped execution = %q, want execution-sync", executionID)
|
||||
}
|
||||
default:
|
||||
t.Fatal("synchronous stop returned before agent teardown ran")
|
||||
}
|
||||
if got := repo.sessions["session-sync"].State; got != models.TaskSessionStateCancelled {
|
||||
t.Fatalf("session state = %q, want CANCELLED", got)
|
||||
}
|
||||
}
|
||||
|
||||
func TestStopSessionSynchronouslyStopsLateExecutionAfterSessionDeletion(t *testing.T) {
|
||||
repo := newMockRepository()
|
||||
repo.getTaskSessionFunc = func(context.Context, string) (*models.TaskSession, error) {
|
||||
return nil, fmt.Errorf("session deleted: %w", models.ErrTaskSessionNotFound)
|
||||
}
|
||||
stopCalls := make(chan string, 1)
|
||||
manager := &mockAgentManager{
|
||||
getExecutionIDForSessionFunc: func(context.Context, string) (string, error) {
|
||||
return "execution-late", nil
|
||||
},
|
||||
stopAgentWithReasonFunc: func(_ context.Context, executionID, _ string, _ bool) error {
|
||||
stopCalls <- executionID
|
||||
return nil
|
||||
},
|
||||
}
|
||||
exec := newTestExecutor(t, manager, repo)
|
||||
|
||||
if err := exec.StopSessionSynchronously(context.Background(), "session-deleted", "cleanup", true); err != nil {
|
||||
t.Fatalf("StopSessionSynchronously: %v", err)
|
||||
}
|
||||
if got := <-stopCalls; got != "execution-late" {
|
||||
t.Fatalf("stopped execution = %q, want execution-late", got)
|
||||
}
|
||||
}
|
||||
|
||||
func TestStopSessionDetailed_RejectsInvalidSession(t *testing.T) {
|
||||
exec := newTestExecutor(t, &mockAgentManager{}, newMockRepository())
|
||||
|
||||
|
||||
@@ -3106,6 +3106,14 @@ func (s *Service) StopSession(ctx context.Context, sessionID string, reason stri
|
||||
return s.executor.Stop(ctx, sessionID, reason, force)
|
||||
}
|
||||
|
||||
// StopSessionSynchronously is the internal cleanup variant of StopSession. It
|
||||
// skips user authorization because task cleanup may run after the task/session
|
||||
// rows have been removed, and it waits for the executor process to exit before
|
||||
// returning to the destructive worktree cleanup path.
|
||||
func (s *Service) StopSessionSynchronously(ctx context.Context, sessionID string, reason string, force bool) error {
|
||||
return s.executor.StopSessionSynchronously(ctx, sessionID, reason, force)
|
||||
}
|
||||
|
||||
// DeleteSession deletes a session that is not currently running.
|
||||
func (s *Service) DeleteSession(ctx context.Context, sessionID string) error {
|
||||
if err := s.authorizeSession(ctx, sessionID); err != nil {
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
package workspaces
|
||||
|
||||
import (
|
||||
"context"
|
||||
"encoding/json"
|
||||
"errors"
|
||||
"fmt"
|
||||
@@ -10,13 +11,14 @@ import (
|
||||
|
||||
// DirectoryHandle keeps a directory pinned while a worktree decision is made.
|
||||
// Implementations open every path component without following links and use
|
||||
// the resulting descriptor or native handle for subsequent reads and writes.
|
||||
// This prevents a path replacement between validation and the side effect
|
||||
// from redirecting the operation to another task root.
|
||||
// the resulting descriptor or native handle for subsequent reads, writes, and
|
||||
// removal. This prevents a path replacement between validation and the side
|
||||
// effect from redirecting the operation to another task root.
|
||||
type DirectoryHandle interface {
|
||||
Close() error
|
||||
VerifyPath(path string) error
|
||||
IsValidWorktree() bool
|
||||
RemoveDirectory(ctx context.Context) error
|
||||
ReadFile(name string) ([]byte, error)
|
||||
WriteFile(name string, data []byte, mode os.FileMode) error
|
||||
}
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
package workspaces
|
||||
|
||||
import (
|
||||
"context"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"testing"
|
||||
@@ -43,6 +44,15 @@ func TestDirectoryHandlePinsWorktreeAcrossPathReplacement(t *testing.T) {
|
||||
if err := handle.VerifyPath(original); err == nil {
|
||||
t.Fatal("VerifyPath succeeded after the lexical path changed")
|
||||
}
|
||||
if err := handle.RemoveDirectory(context.Background()); err == nil {
|
||||
t.Fatal("remove pinned original directory succeeded after path replacement")
|
||||
}
|
||||
if _, err := os.Stat(filepath.Join(archived, ".git")); !os.IsNotExist(err) {
|
||||
t.Fatalf("archived original directory contents remain: %v", err)
|
||||
}
|
||||
if _, err := os.Stat(filepath.Join(original, ".git")); err != nil {
|
||||
t.Fatalf("replacement directory changed: %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestCreateDirectoryNoFollowPinsCreatedTaskRootBeforeMarkerWrite(t *testing.T) {
|
||||
|
||||
@@ -3,6 +3,7 @@
|
||||
package workspaces
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"fmt"
|
||||
"io"
|
||||
@@ -18,6 +19,7 @@ type unixDirectoryHandle struct {
|
||||
rootFD int
|
||||
parentFD int
|
||||
targetFD int
|
||||
target string
|
||||
once sync.Once
|
||||
}
|
||||
|
||||
@@ -38,7 +40,10 @@ func OpenDirectoryNoFollow(root, target string) (DirectoryHandle, error) {
|
||||
_ = unix.Close(rootFD)
|
||||
return nil, err
|
||||
}
|
||||
return &unixDirectoryHandle{rootFD: rootFD, parentFD: parentFD, targetFD: targetFD}, nil
|
||||
return &unixDirectoryHandle{
|
||||
rootFD: rootFD, parentFD: parentFD, targetFD: targetFD,
|
||||
target: filepath.Base(filepath.Clean(relative)),
|
||||
}, nil
|
||||
}
|
||||
|
||||
// CreateDirectoryNoFollow creates every missing component below root through
|
||||
@@ -57,7 +62,10 @@ func CreateDirectoryNoFollow(root, target string, mode os.FileMode) (DirectoryHa
|
||||
_ = unix.Close(rootFD)
|
||||
return nil, err
|
||||
}
|
||||
return &unixDirectoryHandle{rootFD: rootFD, parentFD: parentFD, targetFD: targetFD}, nil
|
||||
return &unixDirectoryHandle{
|
||||
rootFD: rootFD, parentFD: parentFD, targetFD: targetFD,
|
||||
target: filepath.Base(filepath.Clean(relative)),
|
||||
}, nil
|
||||
}
|
||||
|
||||
func (h *unixDirectoryHandle) Close() error {
|
||||
@@ -115,6 +123,41 @@ func (h *unixDirectoryHandle) IsValidWorktree() bool {
|
||||
return err == nil && strings.HasPrefix(string(content), "gitdir:")
|
||||
}
|
||||
|
||||
// RemoveDirectory removes the pinned directory through its open descriptors.
|
||||
// It never resolves the directory again from its lexical path, so a rename or
|
||||
// replacement cannot redirect deletion to a different workspace.
|
||||
func (h *unixDirectoryHandle) RemoveDirectory(ctx context.Context) error {
|
||||
if h == nil || h.parentFD < 0 || h.targetFD < 0 || h.target == "" {
|
||||
return errors.New("directory handle is closed")
|
||||
}
|
||||
if err := removeUnixDependencyContents(ctx, h.targetFD); err != nil {
|
||||
return err
|
||||
}
|
||||
if err := ctx.Err(); err != nil {
|
||||
return err
|
||||
}
|
||||
var targetInfo unix.Stat_t
|
||||
if err := unix.Fstat(h.targetFD, &targetInfo); err != nil {
|
||||
return fmt.Errorf("stat pinned directory: %w", err)
|
||||
}
|
||||
var currentInfo unix.Stat_t
|
||||
if err := unix.Fstatat(h.parentFD, h.target, ¤tInfo, unix.AT_SYMLINK_NOFOLLOW); err != nil {
|
||||
if errors.Is(err, unix.ENOENT) {
|
||||
// The pinned directory was renamed. Its contents are safe to clear,
|
||||
// but there is no longer a path entry that this handle can unlink.
|
||||
return nil
|
||||
}
|
||||
return fmt.Errorf("inspect pinned directory entry: %w", err)
|
||||
}
|
||||
if targetInfo.Dev != currentInfo.Dev || targetInfo.Ino != currentInfo.Ino {
|
||||
return errors.New("pinned directory path changed during cleanup")
|
||||
}
|
||||
if err := unix.Unlinkat(h.parentFD, h.target, unix.AT_REMOVEDIR); err != nil && !errors.Is(err, unix.ENOENT) {
|
||||
return err
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
func (h *unixDirectoryHandle) ReadFile(name string) ([]byte, error) {
|
||||
if h == nil || h.targetFD < 0 {
|
||||
return nil, errors.New("directory handle is closed")
|
||||
|
||||
@@ -3,6 +3,7 @@
|
||||
package workspaces
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"fmt"
|
||||
"io"
|
||||
@@ -118,6 +119,22 @@ func (h *windowsDirectoryHandle) IsValidWorktree() bool {
|
||||
return err == nil && strings.HasPrefix(string(content), "gitdir:")
|
||||
}
|
||||
|
||||
// RemoveDirectory removes the pinned directory through its native handle. It
|
||||
// never resolves the directory again from its lexical path, so a rename or
|
||||
// replacement cannot redirect deletion to a different workspace.
|
||||
func (h *windowsDirectoryHandle) RemoveDirectory(ctx context.Context) error {
|
||||
if h == nil || h.targetHandle == 0 {
|
||||
return errors.New("directory handle is closed")
|
||||
}
|
||||
if err := removeWindowsDependencyContents(ctx, h.targetHandle); err != nil {
|
||||
return err
|
||||
}
|
||||
if err := ctx.Err(); err != nil {
|
||||
return err
|
||||
}
|
||||
return markWindowsDependencyForDelete(h.targetHandle)
|
||||
}
|
||||
|
||||
func (h *windowsDirectoryHandle) ReadFile(name string) ([]byte, error) {
|
||||
if h == nil || h.targetHandle == 0 {
|
||||
return nil, errors.New("directory handle is closed")
|
||||
|
||||
@@ -42,6 +42,7 @@ type persistedTaskStopTarget struct {
|
||||
type taskResourceCleanupSnapshot struct {
|
||||
Sessions []*models.TaskSession `json:"sessions,omitempty"`
|
||||
Worktrees []*worktree.Worktree `json:"worktrees,omitempty"`
|
||||
WorktreeHeadOIDs map[string]string `json:"worktree_head_oids,omitempty"`
|
||||
StopTargets []persistedTaskStopTarget `json:"stop_targets,omitempty"`
|
||||
TaskEnvironment *models.TaskEnvironment `json:"task_environment,omitempty"`
|
||||
DeleteEnvironmentRow bool `json:"delete_environment_row,omitempty"`
|
||||
@@ -79,8 +80,12 @@ func (s *Service) persistTaskResourceCleanup(
|
||||
if operationID == "" {
|
||||
operationID = newTaskResourceCleanupOperationID(trigger, taskID)
|
||||
}
|
||||
worktreeHeadOIDs, err := s.captureWorktreeCleanupHeadOIDs(ctx, worktrees)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
snapshot := taskResourceCleanupSnapshot{
|
||||
Sessions: sessions, Worktrees: worktrees,
|
||||
Sessions: sessions, Worktrees: worktrees, WorktreeHeadOIDs: worktreeHeadOIDs,
|
||||
StopTargets: persistStopTargets(stopTargets),
|
||||
TaskEnvironment: envCleanup.env,
|
||||
DeleteEnvironmentRow: envCleanup.deleteRow,
|
||||
@@ -114,6 +119,23 @@ func (s *Service) persistTaskResourceCleanup(
|
||||
return s.resourceCleanups.GetTaskResourceCleanupJobByOperationID(ctx, operationID)
|
||||
}
|
||||
|
||||
func (s *Service) captureWorktreeCleanupHeadOIDs(
|
||||
ctx context.Context, worktrees []*worktree.Worktree,
|
||||
) (map[string]string, error) {
|
||||
if len(worktrees) == 0 || s.worktreeCleanup == nil {
|
||||
return nil, nil
|
||||
}
|
||||
provider, ok := s.worktreeCleanup.(WorktreeCleanupIdentityProvider)
|
||||
if !ok {
|
||||
return nil, nil
|
||||
}
|
||||
identities, err := provider.CaptureCleanupHeadOIDs(ctx, worktrees)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("capture worktree cleanup identities: %w", err)
|
||||
}
|
||||
return identities, nil
|
||||
}
|
||||
|
||||
func persistStopTargets(targets []taskStopTarget) []persistedTaskStopTarget {
|
||||
result := make([]persistedTaskStopTarget, 0, len(targets))
|
||||
for _, target := range targets {
|
||||
@@ -349,6 +371,11 @@ func (s *Service) processTaskResourceCleanupJob(ctx context.Context, id string)
|
||||
if job.IsArchive() {
|
||||
snapshot.DeleteEnvironmentRow = false
|
||||
}
|
||||
for _, wt := range snapshot.Worktrees {
|
||||
if wt != nil && snapshot.WorktreeHeadOIDs != nil {
|
||||
wt.CleanupHeadOID = snapshot.WorktreeHeadOIDs[wt.ID]
|
||||
}
|
||||
}
|
||||
defer s.signalCleanupDoneForTest()
|
||||
cleanupErr := s.executeTaskResourceCleanupJob(runCtx, job, &snapshot)
|
||||
if cleanupErr != nil {
|
||||
@@ -440,6 +467,7 @@ func (s *Service) executeTaskResourceCleanupJob(
|
||||
taskResourceCleanupStopReason(job.Trigger),
|
||||
"task cleanup runtime stop failed",
|
||||
taskResourceCleanupDeletesTask(job.Trigger),
|
||||
true,
|
||||
)
|
||||
failedStops := stopOutcome.failed
|
||||
if cancelled, err := s.cancelIfTaskUnarchived(ctx, job); err != nil || cancelled {
|
||||
@@ -646,6 +674,10 @@ func (s *Service) PrepareTaskResourceCleanup(
|
||||
if err != nil {
|
||||
return fmt.Errorf("list worktrees for cleanup snapshot: %w", err)
|
||||
}
|
||||
worktreeHeadOIDs, err := s.captureWorktreeCleanupHeadOIDs(ctx, worktrees)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
taskEnv, err := s.gatherTaskEnvironmentForCleanup(ctx, taskID)
|
||||
if err != nil {
|
||||
return fmt.Errorf("lookup task environment for cleanup snapshot: %w", err)
|
||||
@@ -655,7 +687,7 @@ func (s *Service) PrepareTaskResourceCleanup(
|
||||
return fmt.Errorf("list remote task directories for cleanup snapshot: %w", err)
|
||||
}
|
||||
snapshot := taskResourceCleanupSnapshot{
|
||||
Sessions: sessions, Worktrees: worktrees,
|
||||
Sessions: sessions, Worktrees: worktrees, WorktreeHeadOIDs: worktreeHeadOIDs,
|
||||
StopTargets: persistStopTargets(stopTargets),
|
||||
TaskEnvironment: taskEnv,
|
||||
DeleteEnvironmentRow: deleteEnvironmentRow,
|
||||
|
||||
@@ -188,6 +188,21 @@ func TestDeleteTaskCleanupRemovesEveryWorktreeAfterLastSessionDeletedAndRestart(
|
||||
if err := svc.DeleteTask(ctx, taskID); err != nil {
|
||||
t.Fatalf("delete task: %v", err)
|
||||
}
|
||||
var encodedSnapshot string
|
||||
if err := repo.DB().QueryRowContext(ctx, `
|
||||
SELECT resource_snapshot FROM task_resource_cleanup_jobs
|
||||
WHERE task_id = ? AND trigger = 'delete'
|
||||
ORDER BY created_at DESC LIMIT 1
|
||||
`, taskID).Scan(&encodedSnapshot); err != nil {
|
||||
t.Fatalf("load delete cleanup snapshot: %v", err)
|
||||
}
|
||||
var cleanupSnapshot taskResourceCleanupSnapshot
|
||||
if err := json.Unmarshal([]byte(encodedSnapshot), &cleanupSnapshot); err != nil {
|
||||
t.Fatalf("decode delete cleanup snapshot: %v", err)
|
||||
}
|
||||
if len(cleanupSnapshot.WorktreeHeadOIDs) != 2 {
|
||||
t.Fatalf("snapshot worktree identities = %#v, want both repository commits", cleanupSnapshot.WorktreeHeadOIDs)
|
||||
}
|
||||
var jobID string
|
||||
if err := repo.DB().QueryRowContext(ctx, `
|
||||
SELECT id FROM task_resource_cleanup_jobs
|
||||
|
||||
@@ -0,0 +1,91 @@
|
||||
package service
|
||||
|
||||
import (
|
||||
"context"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
"github.com/kandev/kandev/internal/task/models"
|
||||
"github.com/kandev/kandev/internal/worktree"
|
||||
)
|
||||
|
||||
func TestDeleteTaskCleanupRetriesWhenAuditedWorktreeRemovalIsUnsafe(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
_, _, repo := createTestService(t)
|
||||
const taskID = "task-audited-cleanup-retry"
|
||||
const sessionID = "session-audited-cleanup-retry"
|
||||
seedCleanupTaskAndSession(t, repo, taskID, sessionID)
|
||||
|
||||
repoPath := initSimpleGitRepo(t)
|
||||
if err := repo.CreateRepository(ctx, &models.Repository{
|
||||
ID: "repo-audited-cleanup", WorkspaceID: "ws-" + taskID,
|
||||
Name: "repo-audited-cleanup", SourceType: "local", LocalPath: repoPath,
|
||||
}); err != nil {
|
||||
t.Fatalf("create repository: %v", err)
|
||||
}
|
||||
mgr := newCleanupTestWorktreeManager(t, repo)
|
||||
wt, err := mgr.Create(ctx, worktree.CreateRequest{
|
||||
TaskID: taskID, SessionID: sessionID, TaskTitle: "Audited cleanup retry",
|
||||
RepositoryID: "repo-audited-cleanup", RepositoryPath: repoPath,
|
||||
BaseBranch: "main", TaskDirName: taskID, RepoName: "repo-audited-cleanup",
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("create worktree: %v", err)
|
||||
}
|
||||
if err := repo.CreateTaskEnvironment(ctx, &models.TaskEnvironment{
|
||||
ID: "env-" + taskID, TaskID: taskID, ExecutorType: "worktree",
|
||||
WorkspacePath: filepath.Dir(wt.Path), Status: models.TaskEnvironmentStatusReady,
|
||||
Repos: []*models.TaskEnvironmentRepo{{
|
||||
ID: "env-repo-audited-cleanup", RepositoryID: "repo-audited-cleanup",
|
||||
WorktreeID: wt.ID, WorktreePath: wt.Path, WorktreeBranch: wt.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
|
||||
session.BaseBranch = "main"
|
||||
if err := repo.UpdateTaskSession(ctx, session); err != nil {
|
||||
t.Fatalf("link task session: %v", err)
|
||||
}
|
||||
sentinel := filepath.Join(wt.Path, "untracked-preserved.txt")
|
||||
if err := os.WriteFile(sentinel, []byte("preserve me\n"), 0o644); err != nil {
|
||||
t.Fatalf("write untracked work: %v", err)
|
||||
}
|
||||
|
||||
svc := newServiceOverRepository(t, repo)
|
||||
svc.SetWorktreeCleanup(mgr)
|
||||
if err := svc.DeleteTask(ctx, taskID); err != nil {
|
||||
t.Fatalf("DeleteTask: %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.Fatal("unsafe worktree cleanup returned nil")
|
||||
}
|
||||
|
||||
job, err := repo.GetTaskResourceCleanupJob(ctx, jobID)
|
||||
if err != nil {
|
||||
t.Fatalf("reload cleanup job: %v", err)
|
||||
}
|
||||
if job.State != models.TaskResourceCleanupStateRetryWait || job.NextAttemptAt == nil {
|
||||
t.Fatalf("cleanup job state = %q next=%v, want retry_wait with next attempt", job.State, job.NextAttemptAt)
|
||||
}
|
||||
if !strings.Contains(job.LastError, "uncommitted or untracked work") {
|
||||
t.Fatalf("cleanup last error = %q, want audited-work refusal", job.LastError)
|
||||
}
|
||||
if got, err := os.ReadFile(sentinel); err != nil || string(got) != "preserve me\n" {
|
||||
t.Fatalf("preserved work changed: contents=%q err=%v", got, err)
|
||||
}
|
||||
}
|
||||
@@ -53,6 +53,14 @@ type WorktreeProvider interface {
|
||||
GetAllByTaskID(ctx context.Context, taskID string) ([]*worktree.Worktree, error)
|
||||
}
|
||||
|
||||
// WorktreeCleanupIdentityProvider captures immutable checkout identities before
|
||||
// a durable cleanup snapshot is stored. Implementations that do not provide it
|
||||
// remain compatible with legacy cleanup wiring, which uses the older live-state
|
||||
// fallback in the worktree manager.
|
||||
type WorktreeCleanupIdentityProvider interface {
|
||||
CaptureCleanupHeadOIDs(ctx context.Context, worktrees []*worktree.Worktree) (map[string]string, error)
|
||||
}
|
||||
|
||||
// WorktreeBatchCleaner extends WorktreeProvider with batch cleanup.
|
||||
type WorktreeBatchCleaner interface {
|
||||
WorktreeProvider
|
||||
@@ -82,6 +90,13 @@ type TaskExecutionStopper interface {
|
||||
RegisterExecutionStopOwner(sessionID, executionID string, force bool)
|
||||
}
|
||||
|
||||
// synchronousTaskExecutionStopper is an optional cleanup-only extension. The
|
||||
// normal StopSession contract schedules process teardown asynchronously, but
|
||||
// destructive resource cleanup must wait until the process exits.
|
||||
type synchronousTaskExecutionStopper interface {
|
||||
StopSessionSynchronously(ctx context.Context, sessionID, reason string, force bool) error
|
||||
}
|
||||
|
||||
// TerminalClarificationCanceller expires durable input requests after a task
|
||||
// service-owned terminal transition, such as archive cancellation.
|
||||
type TerminalClarificationCanceller interface {
|
||||
|
||||
@@ -2670,7 +2670,7 @@ func (s *Service) runTaskCleanup(
|
||||
stopTargets = refreshedTargets
|
||||
s.registerTaskRuntimeStopOwners(stopTargets, true)
|
||||
|
||||
stopOutcome := s.stopTaskRuntimeTargetsWithTaskDeleted(cleanupCtx, id, stopTargets, stopReason, stopFailMsg, taskDeleted)
|
||||
stopOutcome := s.stopTaskRuntimeTargetsWithTaskDeleted(cleanupCtx, id, stopTargets, stopReason, stopFailMsg, taskDeleted, true)
|
||||
|
||||
cleanupErrors := s.performTaskCleanup(cleanupCtx, id, sessions, worktrees, stopTargets, envCleanup,
|
||||
taskCleanupPreserveRows(stopOutcome))
|
||||
@@ -2883,6 +2883,7 @@ func (s *Service) stopTaskRuntimeTargetsWithTaskDeleted(
|
||||
stopTargets []taskStopTarget,
|
||||
stopReason, stopFailMsg string,
|
||||
taskDeleted bool,
|
||||
waitForStop ...bool,
|
||||
) taskRuntimeStopOutcome {
|
||||
outcome := taskRuntimeStopOutcome{
|
||||
failed: make(map[string]struct{}),
|
||||
@@ -2891,11 +2892,12 @@ func (s *Service) stopTaskRuntimeTargetsWithTaskDeleted(
|
||||
if s.executionStopper == nil || len(stopTargets) == 0 {
|
||||
return outcome
|
||||
}
|
||||
wait := len(waitForStop) > 0 && waitForStop[0]
|
||||
for _, target := range stopTargets {
|
||||
if context.Cause(ctx) != nil {
|
||||
return outcome
|
||||
}
|
||||
s.stopTaskRuntimeTarget(ctx, taskID, target, stopReason, stopFailMsg, taskDeleted, &outcome)
|
||||
s.stopTaskRuntimeTarget(ctx, taskID, target, stopReason, stopFailMsg, taskDeleted, wait, &outcome)
|
||||
}
|
||||
return outcome
|
||||
}
|
||||
@@ -2905,14 +2907,14 @@ func (s *Service) stopTaskRuntimeTarget(
|
||||
taskID string,
|
||||
target taskStopTarget,
|
||||
stopReason, stopFailMsg string,
|
||||
taskDeleted bool,
|
||||
taskDeleted, waitForStop bool,
|
||||
outcome *taskRuntimeStopOutcome,
|
||||
) {
|
||||
if target.executionID != "" {
|
||||
s.stopTaskRuntimeExecution(ctx, taskID, target, stopReason, stopFailMsg, outcome)
|
||||
return
|
||||
}
|
||||
s.stopTaskRuntimeSession(ctx, taskID, target, stopReason, stopFailMsg, taskDeleted, outcome)
|
||||
s.stopTaskRuntimeSession(ctx, taskID, target, stopReason, stopFailMsg, taskDeleted, waitForStop, outcome)
|
||||
}
|
||||
|
||||
func (s *Service) stopTaskRuntimeExecution(
|
||||
@@ -2940,9 +2942,19 @@ func (s *Service) stopTaskRuntimeSession(
|
||||
target taskStopTarget,
|
||||
stopReason, stopFailMsg string,
|
||||
taskDeleted bool,
|
||||
waitForStop bool,
|
||||
outcome *taskRuntimeStopOutcome,
|
||||
) {
|
||||
err := s.executionStopper.StopSession(ctx, target.sessionID, stopReason, true)
|
||||
var err error
|
||||
if waitForStop {
|
||||
if synchronous, ok := s.executionStopper.(synchronousTaskExecutionStopper); ok {
|
||||
err = synchronous.StopSessionSynchronously(ctx, target.sessionID, stopReason, true)
|
||||
} else {
|
||||
err = s.executionStopper.StopSession(ctx, target.sessionID, stopReason, true)
|
||||
}
|
||||
} else {
|
||||
err = s.executionStopper.StopSession(ctx, target.sessionID, stopReason, true)
|
||||
}
|
||||
if err == nil {
|
||||
return
|
||||
}
|
||||
|
||||
@@ -232,6 +232,16 @@ type stubStopper struct {
|
||||
onStopSession func()
|
||||
}
|
||||
|
||||
type synchronousStubStopper struct {
|
||||
stubStopper
|
||||
synchronousSessionCalls int
|
||||
}
|
||||
|
||||
func (s *synchronousStubStopper) StopSessionSynchronously(_ context.Context, _, _ string, _ bool) error {
|
||||
s.synchronousSessionCalls++
|
||||
return nil
|
||||
}
|
||||
|
||||
func (s *stubStopper) StopTask(_ context.Context, _, _ string, _ bool) error { return nil }
|
||||
func (s *stubStopper) StopExecution(_ context.Context, _, _ string, _ bool) error {
|
||||
s.stopExecutionCalls++
|
||||
@@ -246,6 +256,31 @@ func (s *stubStopper) StopSession(_ context.Context, _, _ string, _ bool) error
|
||||
return s.stopSessionErr
|
||||
}
|
||||
|
||||
func TestStopTaskRuntimeSession_WaitsForSynchronousStopWhenAvailable(t *testing.T) {
|
||||
svc, _, _ := createTestService(t)
|
||||
stopper := &synchronousStubStopper{}
|
||||
svc.executionStopper = stopper
|
||||
|
||||
outcome := svc.stopTaskRuntimeTargetsWithTaskDeleted(
|
||||
context.Background(),
|
||||
"task-sync-stop",
|
||||
[]taskStopTarget{{sessionID: "session-sync-stop"}},
|
||||
"task deleted",
|
||||
"stop failed",
|
||||
true,
|
||||
true,
|
||||
)
|
||||
if len(outcome.failed) != 0 {
|
||||
t.Fatalf("synchronous session stop failed: %v", outcome.failed)
|
||||
}
|
||||
if stopper.synchronousSessionCalls != 1 {
|
||||
t.Fatalf("synchronous stop calls = %d, want 1", stopper.synchronousSessionCalls)
|
||||
}
|
||||
if stopper.stopSessionCalls != 0 {
|
||||
t.Fatalf("asynchronous StopSession calls = %d, want 0", stopper.stopSessionCalls)
|
||||
}
|
||||
}
|
||||
|
||||
type cancelAfterFirstStopper struct {
|
||||
cancel context.CancelFunc
|
||||
calls []string
|
||||
|
||||
@@ -92,8 +92,22 @@ type worktreeRegistration struct {
|
||||
branch string
|
||||
}
|
||||
|
||||
type worktreeRegistrationOwnershipOptions struct {
|
||||
allowAnyBranchAtPath bool
|
||||
}
|
||||
|
||||
func inspectWorktreeRegistrationOwnership(
|
||||
ctx context.Context, repoPath, worktreePath, branchRef, headOID string,
|
||||
) (worktreeRegistrationOwnership, error) {
|
||||
return inspectWorktreeRegistrationOwnershipWithOptions(
|
||||
ctx, repoPath, worktreePath, branchRef, headOID,
|
||||
worktreeRegistrationOwnershipOptions{},
|
||||
)
|
||||
}
|
||||
|
||||
func inspectWorktreeRegistrationOwnershipWithOptions(
|
||||
ctx context.Context, repoPath, worktreePath, branchRef, headOID string,
|
||||
options worktreeRegistrationOwnershipOptions,
|
||||
) (worktreeRegistrationOwnership, error) {
|
||||
cmd := newGitCommand(ctx, "worktree", "list", "--porcelain", "-z")
|
||||
cmd.Dir = repoPath
|
||||
@@ -106,7 +120,7 @@ func inspectWorktreeRegistrationOwnership(
|
||||
return worktreeRegistrationAbsent, err
|
||||
}
|
||||
return classifyWorktreeRegistrationOwnership(
|
||||
parseWorktreeRegistrations(string(output)), wantPath, branchRef, headOID,
|
||||
parseWorktreeRegistrations(string(output)), wantPath, branchRef, headOID, options,
|
||||
)
|
||||
}
|
||||
|
||||
@@ -131,7 +145,12 @@ func parseWorktreeRegistrations(output string) []worktreeRegistration {
|
||||
|
||||
func classifyWorktreeRegistrationOwnership(
|
||||
registrations []worktreeRegistration, wantPath, branchRef, headOID string,
|
||||
options ...worktreeRegistrationOwnershipOptions,
|
||||
) (worktreeRegistrationOwnership, error) {
|
||||
ownershipOptions := worktreeRegistrationOwnershipOptions{}
|
||||
if len(options) > 0 {
|
||||
ownershipOptions = options[0]
|
||||
}
|
||||
var exactTarget, branchElsewhere, targetClaimed bool
|
||||
for _, registration := range registrations {
|
||||
if registration.path == "" {
|
||||
@@ -142,12 +161,17 @@ func classifyWorktreeRegistrationOwnership(
|
||||
return worktreeRegistrationAbsent, err
|
||||
}
|
||||
if currentPath != wantPath {
|
||||
if registration.branch == branchRef {
|
||||
if branchRef != "" && registration.branch == branchRef {
|
||||
branchElsewhere = true
|
||||
}
|
||||
continue
|
||||
}
|
||||
if registration.branch == branchRef && registration.head == headOID {
|
||||
matchesBranch := registration.branch == branchRef
|
||||
if branchRef == "" && ownershipOptions.allowAnyBranchAtPath {
|
||||
matchesBranch = true
|
||||
}
|
||||
matchesHead := registration.head == headOID
|
||||
if matchesBranch && matchesHead && !exactTarget {
|
||||
exactTarget = true
|
||||
continue
|
||||
}
|
||||
@@ -168,11 +192,101 @@ func (m *Manager) RemoveByID(ctx context.Context, worktreeID string, removeBranc
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
m.enrichCleanupWorktreeFromCache(wt)
|
||||
return m.removeWorktree(ctx, wt, removeBranch)
|
||||
}
|
||||
|
||||
func (m *Manager) enrichCleanupWorktreeFromCache(wt *Worktree) {
|
||||
if wt == nil || (wt.RepositoryPath != "" && wt.BaseBranch != "" && wt.CleanupHeadOID != "") {
|
||||
return
|
||||
}
|
||||
m.mu.RLock()
|
||||
cached := m.worktrees[cacheKey(wt.SessionID, wt.RepositoryID, wt.BranchSlug)]
|
||||
if cached == nil {
|
||||
for _, candidate := range m.worktrees {
|
||||
if candidate != nil && candidate.ID == wt.ID {
|
||||
cached = candidate
|
||||
break
|
||||
}
|
||||
}
|
||||
}
|
||||
m.mu.RUnlock()
|
||||
if cached == nil || cached.ID != wt.ID {
|
||||
return
|
||||
}
|
||||
if wt.RepositoryPath == "" {
|
||||
wt.RepositoryPath = cached.RepositoryPath
|
||||
}
|
||||
if wt.BaseBranch == "" {
|
||||
wt.BaseBranch = cached.BaseBranch
|
||||
}
|
||||
if wt.CleanupHeadOID == "" {
|
||||
wt.CleanupHeadOID = cached.CleanupHeadOID
|
||||
}
|
||||
}
|
||||
|
||||
// CaptureCleanupHeadOIDs records the checkout commit for each worktree before
|
||||
// a durable task cleanup job is persisted. Cleanup later compares this value
|
||||
// with the live branch and registration so a replacement checkout cannot be
|
||||
// mistaken for the original task workspace.
|
||||
func (m *Manager) CaptureCleanupHeadOIDs(ctx context.Context, worktrees []*Worktree) (map[string]string, error) {
|
||||
identities := make(map[string]string, len(worktrees))
|
||||
for _, wt := range worktrees {
|
||||
if wt == nil || wt.ID == "" {
|
||||
continue
|
||||
}
|
||||
m.enrichCleanupWorktreeFromCache(wt)
|
||||
if strings.TrimSpace(wt.RepositoryPath) == "" || strings.TrimSpace(wt.Path) == "" {
|
||||
// Older environment rows can outlive their repository/session rows.
|
||||
// Keep them in the durable snapshot, but let the later cleanup audit
|
||||
// fail closed instead of rejecting the task mutation itself.
|
||||
continue
|
||||
}
|
||||
identityPath := wt.Path
|
||||
info, statErr := os.Stat(wt.Path)
|
||||
if statErr != nil && !errors.Is(statErr, os.ErrNotExist) {
|
||||
return nil, fmt.Errorf("capture cleanup identity for %s: %w", wt.ID, statErr)
|
||||
}
|
||||
if statErr != nil || !info.IsDir() {
|
||||
if wt.Branch == "" {
|
||||
continue
|
||||
}
|
||||
identityPath = wt.RepositoryPath
|
||||
}
|
||||
args := []string{"rev-parse", "--verify", "HEAD^{commit}"}
|
||||
if identityPath == wt.RepositoryPath {
|
||||
args = []string{"rev-parse", "--verify", "refs/heads/" + wt.Branch + "^{commit}"}
|
||||
}
|
||||
output, err := m.runBoundedGitInspect(ctx, identityPath, args...)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("capture cleanup identity for %s: %w", wt.ID, err)
|
||||
}
|
||||
oid := strings.TrimSpace(output)
|
||||
if oid == "" {
|
||||
return nil, fmt.Errorf("capture cleanup identity for %s returned an empty commit", wt.ID)
|
||||
}
|
||||
identities[wt.ID] = oid
|
||||
}
|
||||
return identities, nil
|
||||
}
|
||||
|
||||
// removeWorktree performs the actual removal of a worktree.
|
||||
func (m *Manager) removeWorktree(ctx context.Context, wt *Worktree, removeBranch bool) error {
|
||||
if wt == nil {
|
||||
return errors.New("worktree cleanup requires a worktree")
|
||||
}
|
||||
if strings.TrimSpace(wt.RepositoryPath) == "" || strings.TrimSpace(wt.Path) == "" {
|
||||
return errors.New("worktree cleanup requires repository and worktree paths")
|
||||
}
|
||||
// Serialize cleanup with create/recreate operations that target the same
|
||||
// path. The path lock is acquired before the repository lock to match the
|
||||
// creation order and avoid a lock-order deadlock.
|
||||
releasePath, err := acquireWorktreeTargetPath(ctx, wt.Path)
|
||||
if err != nil {
|
||||
return fmt.Errorf("lock worktree cleanup path: %w", err)
|
||||
}
|
||||
defer releasePath()
|
||||
|
||||
// Get repository lock
|
||||
repoLock := m.getRepoLock(wt.RepositoryPath)
|
||||
repoLock.Lock()
|
||||
@@ -201,45 +315,29 @@ func (m *Manager) removeWorktree(ctx context.Context, wt *Worktree, removeBranch
|
||||
return nil
|
||||
}
|
||||
|
||||
// Execute cleanup script BEFORE removing directory
|
||||
audit, err := m.auditWorktreeCleanup(ctx, wt, removeBranch)
|
||||
if err != nil {
|
||||
return fmt.Errorf("audit worktree cleanup %s: %w", wt.ID, err)
|
||||
}
|
||||
defer func() {
|
||||
if audit.pathHandle != nil {
|
||||
_ = audit.pathHandle.Close()
|
||||
}
|
||||
}()
|
||||
|
||||
// Run the repository script only after the path, Git registration, checkout
|
||||
// contents, and branch identity pass the audit. A script such as `git clean`
|
||||
// must never erase changes before the audit can reject the cleanup.
|
||||
m.runWorktreeCleanupScript(ctx, wt)
|
||||
|
||||
// Remove worktree directory
|
||||
if err := m.removeWorktreeDir(ctx, wt.Path, wt.RepositoryPath); err != nil {
|
||||
m.logger.Warn("failed to remove worktree directory",
|
||||
zap.String("path", wt.Path),
|
||||
zap.Error(err))
|
||||
}
|
||||
|
||||
// Optionally remove the branch from the main repository
|
||||
if removeBranch {
|
||||
m.logger.Info("deleting branch from main repository",
|
||||
zap.String("branch", wt.Branch),
|
||||
zap.String("repository_path", wt.RepositoryPath))
|
||||
|
||||
cmd := newGitCommand(ctx, "branch", "-D", wt.Branch)
|
||||
cmd.Dir = wt.RepositoryPath
|
||||
if output, err := runGitCmdCombinedOutput(ctx, cmd); err != nil {
|
||||
m.logger.Warn("failed to delete branch from main repository",
|
||||
zap.String("branch", wt.Branch),
|
||||
zap.String("repository_path", wt.RepositoryPath),
|
||||
zap.String("output", string(output)),
|
||||
zap.Error(err))
|
||||
} else {
|
||||
m.logger.Info("successfully deleted branch from main repository",
|
||||
zap.String("branch", wt.Branch),
|
||||
zap.String("repository_path", wt.RepositoryPath))
|
||||
}
|
||||
if err := m.completeAuditedWorktreeCleanup(ctx, wt, audit, removeBranch); err != nil {
|
||||
return fmt.Errorf("complete worktree cleanup %s: %w", wt.ID, err)
|
||||
}
|
||||
|
||||
// Update store
|
||||
if m.store != nil {
|
||||
if m.store != nil && wt.SessionID != "" {
|
||||
if err := m.ReleaseWorktreeReference(ctx, wt); err != nil {
|
||||
// Record may already be deleted by another cleanup path (e.g. task deletion).
|
||||
// This is expected and harmless - only log at debug level.
|
||||
m.logger.Debug("failed to update worktree status (may already be deleted)",
|
||||
zap.String("worktree_id", wt.ID),
|
||||
zap.Error(err))
|
||||
return fmt.Errorf("release worktree reference %s: %w", wt.ID, err)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -428,6 +526,7 @@ func (m *Manager) cleanupWorktrees(ctx context.Context, worktrees []*Worktree, r
|
||||
if strings.TrimSpace(wt.ID) == "" {
|
||||
continue
|
||||
}
|
||||
m.enrichCleanupWorktreeFromCache(wt)
|
||||
if err := m.removeWorktree(ctx, wt, removeBranch); err != nil {
|
||||
m.logger.Warn("failed to remove worktree during batch cleanup",
|
||||
zap.String("task_id", wt.TaskID),
|
||||
@@ -437,17 +536,6 @@ func (m *Manager) cleanupWorktrees(ctx context.Context, worktrees []*Worktree, r
|
||||
}
|
||||
}
|
||||
|
||||
m.mu.Lock()
|
||||
for _, wt := range worktrees {
|
||||
if wt == nil {
|
||||
continue
|
||||
}
|
||||
if wt.SessionID != "" {
|
||||
delete(m.worktrees, cacheKey(wt.SessionID, wt.RepositoryID, wt.BranchSlug))
|
||||
}
|
||||
}
|
||||
m.mu.Unlock()
|
||||
|
||||
return lastErr
|
||||
}
|
||||
|
||||
@@ -462,12 +550,37 @@ func (m *Manager) OnTaskDeleted(ctx context.Context, taskID string) error {
|
||||
return m.CleanupWorktrees(ctx, worktrees)
|
||||
}
|
||||
|
||||
// removeWorktreeDir removes a worktree directory using git worktree remove.
|
||||
// After the inner directory is gone it tries to rmdir the parent task
|
||||
// directory left behind by the nested {tasksBase}/{taskDirName}/{repoName}
|
||||
// layout (see issue #1266). The rmdir is a best-effort no-op if the parent
|
||||
// still has siblings or contains workspace-scoped content.
|
||||
func (m *Manager) removeWorktreeDir(ctx context.Context, worktreePath, repoPath string) error {
|
||||
// removeWorktreeDir removes a worktree directory. Audited paths are removed
|
||||
// through their pinned no-follow handle; un-audited callers use git worktree
|
||||
// remove with a safe filesystem fallback. After the inner directory is gone it
|
||||
// tries to rmdir the parent task directory left behind by the nested
|
||||
// {tasksBase}/{taskDirName}/{repoName} layout (see issue #1266). The rmdir is
|
||||
// a best-effort no-op if the parent still has siblings or contains
|
||||
// workspace-scoped content.
|
||||
func (m *Manager) removeWorktreeDir(
|
||||
ctx context.Context,
|
||||
worktreePath, repoPath string,
|
||||
pathHandles ...storageworkspaces.DirectoryHandle,
|
||||
) error {
|
||||
if len(pathHandles) > 0 && pathHandles[0] != nil {
|
||||
// An audited handle pins the original directory identity. Remove through
|
||||
// that handle instead of issuing a path-based Git command, which could
|
||||
// follow a replacement after an external rename.
|
||||
if verifyErr := pathHandles[0].VerifyPath(worktreePath); verifyErr != nil && !errors.Is(verifyErr, os.ErrNotExist) {
|
||||
return fmt.Errorf("worktree path changed during cleanup: %w", verifyErr)
|
||||
}
|
||||
if err := pathHandles[0].RemoveDirectory(ctx); err != nil {
|
||||
return fmt.Errorf("remove audited worktree directory: %w", err)
|
||||
}
|
||||
pruneCmd := newGitCommand(ctx, "worktree", "prune")
|
||||
pruneCmd.Dir = repoPath
|
||||
if err := runGitCmd(ctx, pruneCmd); err != nil {
|
||||
m.logger.Debug("git worktree prune failed", zap.Error(err))
|
||||
}
|
||||
m.tryRemoveEmptyTaskDir(worktreePath)
|
||||
return nil
|
||||
}
|
||||
|
||||
// First try git worktree remove
|
||||
cmd := newGitCommand(ctx, "worktree", "remove", "--force", worktreePath)
|
||||
cmd.Dir = repoPath
|
||||
@@ -476,8 +589,17 @@ func (m *Manager) removeWorktreeDir(ctx context.Context, worktreePath, repoPath
|
||||
zap.String("output", string(output)),
|
||||
zap.Error(err))
|
||||
|
||||
if err := m.forceRemoveDir(ctx, worktreePath); err != nil {
|
||||
return err
|
||||
var removeErr error
|
||||
if tasksBase, managed := m.cleanupWorktreeTasksBase(worktreePath); managed {
|
||||
removeErr = storageworkspaces.RemoveDirectoryNoFollow(ctx, tasksBase, worktreePath)
|
||||
if errors.Is(removeErr, os.ErrNotExist) {
|
||||
removeErr = nil
|
||||
}
|
||||
} else {
|
||||
removeErr = m.forceRemoveDir(ctx, worktreePath)
|
||||
}
|
||||
if removeErr != nil {
|
||||
return removeErr
|
||||
}
|
||||
|
||||
// Prune stale worktree entries
|
||||
@@ -491,6 +613,19 @@ func (m *Manager) removeWorktreeDir(ctx context.Context, worktreePath, repoPath
|
||||
return nil
|
||||
}
|
||||
|
||||
func (m *Manager) cleanupWorktreeTasksBase(worktreePath string) (string, bool) {
|
||||
tasksBase, err := m.config.ExpandedTasksBasePath()
|
||||
if err != nil || tasksBase == "" {
|
||||
return "", false
|
||||
}
|
||||
tasksBase = filepath.Clean(tasksBase)
|
||||
relativePath, err := filepath.Rel(tasksBase, filepath.Clean(worktreePath))
|
||||
if err != nil || relativePath == "." || relativePath == ".." || strings.HasPrefix(relativePath, ".."+string(filepath.Separator)) {
|
||||
return "", false
|
||||
}
|
||||
return tasksBase, true
|
||||
}
|
||||
|
||||
// tryRemoveEmptyTaskDir rmdirs the parent of a removed worktree if that
|
||||
// parent is an immediate child of TasksBasePath (i.e. a per-task container
|
||||
// directory) and is now empty. Silently skips otherwise.
|
||||
|
||||
@@ -0,0 +1,282 @@
|
||||
package worktree
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"fmt"
|
||||
"os"
|
||||
"strings"
|
||||
|
||||
"go.uber.org/zap"
|
||||
|
||||
storageworkspaces "github.com/kandev/kandev/internal/system/storage/workspaces"
|
||||
)
|
||||
|
||||
type worktreeCleanupAudit struct {
|
||||
branchRef string
|
||||
branchOID string
|
||||
deleteBranch bool
|
||||
pathPresent bool
|
||||
registration worktreeRegistrationOwnership
|
||||
pathHandle storageworkspaces.DirectoryHandle
|
||||
}
|
||||
|
||||
func (m *Manager) auditWorktreeCleanup(
|
||||
ctx context.Context, wt *Worktree, removeBranch bool,
|
||||
) (audit worktreeCleanupAudit, err error) {
|
||||
if wt == nil || wt.RepositoryPath == "" || wt.Path == "" {
|
||||
return worktreeCleanupAudit{}, errors.New("worktree cleanup requires repository and worktree paths")
|
||||
}
|
||||
if err = m.validateExistingWorktreePathOwner(wt.Path, wt); err != nil {
|
||||
return worktreeCleanupAudit{}, err
|
||||
}
|
||||
pathPresent, err := cleanupPathPresent(wt.Path)
|
||||
if err != nil {
|
||||
return worktreeCleanupAudit{}, err
|
||||
}
|
||||
pathHandle, err := m.openCleanupPathHandle(wt.Path, pathPresent)
|
||||
if err != nil {
|
||||
return worktreeCleanupAudit{}, err
|
||||
}
|
||||
defer func() {
|
||||
if err != nil && pathHandle != nil {
|
||||
_ = pathHandle.Close()
|
||||
}
|
||||
}()
|
||||
branchRef, branchOID, err := m.cleanupBranchIdentity(ctx, wt)
|
||||
if err != nil {
|
||||
return worktreeCleanupAudit{}, err
|
||||
}
|
||||
registration, err := inspectWorktreeRegistrationOwnershipWithOptions(
|
||||
ctx, wt.RepositoryPath, wt.Path, branchRef, branchOID,
|
||||
worktreeRegistrationOwnershipOptions{allowAnyBranchAtPath: branchRef == ""},
|
||||
)
|
||||
if err != nil {
|
||||
return worktreeCleanupAudit{}, fmt.Errorf("inspect worktree cleanup registration: %w", err)
|
||||
}
|
||||
if registration == worktreeRegistrationCompeting {
|
||||
return worktreeCleanupAudit{}, fmt.Errorf("worktree cleanup target is claimed by unrelated Git metadata: %s", wt.Path)
|
||||
}
|
||||
if pathPresent && registration != worktreeRegistrationOwned {
|
||||
return worktreeCleanupAudit{}, fmt.Errorf("worktree cleanup path is not registered to the recorded branch: %s", wt.Path)
|
||||
}
|
||||
deleteBranch, err := m.auditCleanupBranchDisposition(
|
||||
ctx, wt, branchRef, branchOID, pathPresent, removeBranch,
|
||||
)
|
||||
if err != nil {
|
||||
return worktreeCleanupAudit{}, err
|
||||
}
|
||||
return worktreeCleanupAudit{
|
||||
branchRef: branchRef, branchOID: branchOID, deleteBranch: deleteBranch,
|
||||
pathPresent: pathPresent, registration: registration, pathHandle: pathHandle,
|
||||
}, nil
|
||||
}
|
||||
|
||||
func (m *Manager) auditCleanupBranchDisposition(
|
||||
ctx context.Context,
|
||||
wt *Worktree,
|
||||
branchRef string,
|
||||
branchOID string,
|
||||
pathPresent bool,
|
||||
removeBranch bool,
|
||||
) (bool, error) {
|
||||
if !removeBranch || branchRef == "" || branchOID == "" {
|
||||
return false, nil
|
||||
}
|
||||
if pathPresent {
|
||||
return m.verifyCleanRedundantCheckout(ctx, wt, branchRef)
|
||||
}
|
||||
return m.verifyCleanupBranchRedundant(ctx, wt, branchRef)
|
||||
}
|
||||
|
||||
func cleanupPathPresent(path string) (bool, error) {
|
||||
info, err := os.Lstat(path)
|
||||
if errors.Is(err, os.ErrNotExist) {
|
||||
return false, nil
|
||||
}
|
||||
if err != nil {
|
||||
return false, fmt.Errorf("inspect worktree cleanup path: %w", err)
|
||||
}
|
||||
if info.Mode()&os.ModeSymlink != 0 || !info.IsDir() {
|
||||
return false, fmt.Errorf("worktree cleanup path is not a real directory: %s", path)
|
||||
}
|
||||
return true, nil
|
||||
}
|
||||
|
||||
func (m *Manager) openCleanupPathHandle(
|
||||
path string, pathPresent bool,
|
||||
) (storageworkspaces.DirectoryHandle, error) {
|
||||
if !pathPresent {
|
||||
return nil, nil
|
||||
}
|
||||
handle, err := m.openNoFollowWorktreePath(path)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
if handle == nil {
|
||||
return nil, fmt.Errorf("worktree cleanup path disappeared during audit: %s", path)
|
||||
}
|
||||
return handle, nil
|
||||
}
|
||||
|
||||
func (m *Manager) cleanupBranchIdentity(ctx context.Context, wt *Worktree) (string, string, error) {
|
||||
branchRef := ""
|
||||
if wt.Branch != "" {
|
||||
branchRef = "refs/heads/" + wt.Branch
|
||||
}
|
||||
expectedOID := strings.TrimSpace(wt.CleanupHeadOID)
|
||||
if branchRef == "" {
|
||||
return branchRef, expectedOID, nil
|
||||
}
|
||||
exists, err := m.branchExists(ctx, wt.RepositoryPath, branchRef)
|
||||
if err != nil {
|
||||
return "", "", fmt.Errorf("verify cleanup branch %q: %w", wt.Branch, err)
|
||||
}
|
||||
if !exists {
|
||||
// The local branch is already gone. A stale registration, if any, must
|
||||
// not be adopted without a live ref identity, so the strict ownership
|
||||
// classifier will fail closed rather than deleting another checkout.
|
||||
return branchRef, "", nil
|
||||
}
|
||||
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)
|
||||
}
|
||||
branchOID := strings.TrimSpace(output)
|
||||
if expectedOID != "" && branchOID != expectedOID {
|
||||
return branchRef, "", fmt.Errorf("cleanup branch %q advanced from audited commit %s to %s", wt.Branch, expectedOID, branchOID)
|
||||
}
|
||||
return branchRef, branchOID, nil
|
||||
}
|
||||
|
||||
func (m *Manager) verifyCleanRedundantCheckout(
|
||||
ctx context.Context, wt *Worktree, branchRef string,
|
||||
) (bool, error) {
|
||||
status, err := m.runBoundedGitInspect(ctx, wt.Path, "status", "--porcelain=v1", "--untracked-files=normal")
|
||||
if err != nil {
|
||||
return false, fmt.Errorf("inspect worktree changes before cleanup: %w", err)
|
||||
}
|
||||
if strings.TrimSpace(status) != "" {
|
||||
return false, fmt.Errorf("worktree cleanup refused because %q contains uncommitted or untracked work", wt.Path)
|
||||
}
|
||||
return m.verifyCleanupBranchRedundant(ctx, wt, branchRef)
|
||||
}
|
||||
|
||||
func (m *Manager) verifyCleanupBranchRedundant(
|
||||
ctx context.Context, wt *Worktree, branchRef string,
|
||||
) (bool, error) {
|
||||
base := strings.TrimPrefix(wt.BaseBranch, "origin/")
|
||||
candidates := []string{"refs/remotes/origin/HEAD"}
|
||||
if base != "" && base != wt.Branch {
|
||||
candidates = append([]string{"refs/remotes/origin/" + base, "refs/heads/" + base}, candidates...)
|
||||
}
|
||||
if base == "" {
|
||||
// Session metadata can be gone by the time a durable delete job runs.
|
||||
// Keep the offline fallback limited to conventional named branches.
|
||||
candidates = append(candidates, "refs/heads/main", "refs/heads/master")
|
||||
}
|
||||
// A local HEAD is a valid fallback only when it is attached to a named
|
||||
// branch. A detached HEAD can point at an arbitrary commit and must never
|
||||
// be treated as proof that the task branch is merged.
|
||||
if headRef, err := m.runBoundedGitInspect(ctx, wt.RepositoryPath, "symbolic-ref", "--quiet", "--verify", "HEAD"); err == nil {
|
||||
headRef = strings.TrimSpace(headRef)
|
||||
if strings.HasPrefix(headRef, "refs/heads/") && headRef != "refs/heads/"+wt.Branch {
|
||||
candidates = append(candidates, headRef)
|
||||
}
|
||||
}
|
||||
foundBase := false
|
||||
for _, candidate := range candidates {
|
||||
exists, err := m.branchExists(ctx, wt.RepositoryPath, candidate)
|
||||
if err != nil {
|
||||
return false, fmt.Errorf("verify cleanup base %q: %w", candidate, err)
|
||||
}
|
||||
if !exists {
|
||||
continue
|
||||
}
|
||||
foundBase = true
|
||||
contains, err := m.refContains(ctx, wt.RepositoryPath, candidate, branchRef)
|
||||
if err != nil {
|
||||
return false, fmt.Errorf("verify cleanup branch ancestry: %w", err)
|
||||
}
|
||||
if contains {
|
||||
return true, nil
|
||||
}
|
||||
}
|
||||
if !foundBase {
|
||||
return false, fmt.Errorf("worktree cleanup cannot resolve a containing base for branch %q", wt.Branch)
|
||||
}
|
||||
return false, nil
|
||||
}
|
||||
|
||||
func (m *Manager) completeAuditedWorktreeCleanup(
|
||||
ctx context.Context, wt *Worktree, audit worktreeCleanupAudit, removeBranch bool,
|
||||
) error {
|
||||
if audit.pathHandle != nil {
|
||||
defer func() { _ = audit.pathHandle.Close() }()
|
||||
}
|
||||
switch audit.registration {
|
||||
case worktreeRegistrationOwned:
|
||||
if err := m.completeOwnedWorktreeCleanup(ctx, wt, audit); err != nil {
|
||||
return err
|
||||
}
|
||||
case worktreeRegistrationAbsent:
|
||||
if audit.pathPresent {
|
||||
return fmt.Errorf("refusing to remove unregistered worktree path: %s", wt.Path)
|
||||
}
|
||||
m.tryRemoveEmptyTaskDir(wt.Path)
|
||||
default:
|
||||
return fmt.Errorf("refusing to remove competing worktree registration: %s", wt.Path)
|
||||
}
|
||||
return m.deleteAuditedCleanupBranch(ctx, wt, audit, removeBranch)
|
||||
}
|
||||
|
||||
func (m *Manager) completeOwnedWorktreeCleanup(
|
||||
ctx context.Context, wt *Worktree, audit worktreeCleanupAudit,
|
||||
) error {
|
||||
if audit.pathHandle != nil {
|
||||
if err := audit.pathHandle.VerifyPath(wt.Path); err != nil && !errors.Is(err, os.ErrNotExist) {
|
||||
return fmt.Errorf("worktree path changed after audit: %w", err)
|
||||
}
|
||||
}
|
||||
if err := m.removeWorktreeDir(ctx, wt.Path, wt.RepositoryPath, audit.pathHandle); err != nil {
|
||||
return fmt.Errorf("remove owned worktree path: %w", err)
|
||||
}
|
||||
registration, err := inspectWorktreeRegistrationOwnershipWithOptions(
|
||||
ctx, wt.RepositoryPath, wt.Path, audit.branchRef, audit.branchOID,
|
||||
worktreeRegistrationOwnershipOptions{allowAnyBranchAtPath: audit.branchRef == ""},
|
||||
)
|
||||
if err != nil {
|
||||
return fmt.Errorf("verify removed worktree registration: %w", err)
|
||||
}
|
||||
if registration != worktreeRegistrationAbsent {
|
||||
return fmt.Errorf("worktree registration remains after cleanup: %s", wt.Path)
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
func (m *Manager) deleteAuditedCleanupBranch(
|
||||
ctx context.Context, wt *Worktree, audit worktreeCleanupAudit, removeBranch bool,
|
||||
) error {
|
||||
if !removeBranch || audit.branchOID == "" {
|
||||
return nil
|
||||
}
|
||||
if !audit.deleteBranch {
|
||||
m.logger.Info("preserved cleanup branch with unique commits",
|
||||
zap.String("branch", wt.Branch), zap.String("repository_path", wt.RepositoryPath))
|
||||
return nil
|
||||
}
|
||||
cmd := newGitCommand(ctx, "update-ref", "-d", audit.branchRef, audit.branchOID)
|
||||
cmd.Dir = wt.RepositoryPath
|
||||
if output, err := runGitCmdCombinedOutput(ctx, cmd); err != nil {
|
||||
return fmt.Errorf("delete verified cleanup branch %q: %s: %w",
|
||||
wt.Branch, strings.TrimSpace(string(output)), err)
|
||||
}
|
||||
exists, err := m.branchExists(ctx, wt.RepositoryPath, audit.branchRef)
|
||||
if err != nil {
|
||||
return fmt.Errorf("verify deleted cleanup branch %q: %w", wt.Branch, err)
|
||||
}
|
||||
if exists {
|
||||
return fmt.Errorf("cleanup branch %q remains after deletion", wt.Branch)
|
||||
}
|
||||
return nil
|
||||
}
|
||||
@@ -0,0 +1,45 @@
|
||||
package worktree
|
||||
|
||||
import (
|
||||
"context"
|
||||
"testing"
|
||||
|
||||
"github.com/kandev/kandev/internal/task/models"
|
||||
)
|
||||
|
||||
func TestClassifyWorktreeRegistrationOwnership_AllowsDetachedPathIdentity(t *testing.T) {
|
||||
registrations := []worktreeRegistration{{
|
||||
path: "/tasks/task/repo",
|
||||
head: "abc123",
|
||||
branch: "refs/heads/feature/task",
|
||||
}}
|
||||
|
||||
got, err := classifyWorktreeRegistrationOwnership(
|
||||
registrations, "/tasks/task/repo", "", "abc123",
|
||||
worktreeRegistrationOwnershipOptions{allowAnyBranchAtPath: true},
|
||||
)
|
||||
if err != nil {
|
||||
t.Fatalf("classify detached registration: %v", err)
|
||||
}
|
||||
if got != worktreeRegistrationOwned {
|
||||
t.Fatalf("classification = %v, want owned", got)
|
||||
}
|
||||
}
|
||||
|
||||
func TestVerifyCleanupBranchRedundant_DoesNotUseDetachedRepositoryHEAD(t *testing.T) {
|
||||
mgr, store := newReferenceCleanupTestManager(t)
|
||||
seedReferenceCleanupSession(t, store, "task-detached-base", "session-detached-base", models.TaskSessionStateCompleted)
|
||||
wt := createReferenceCleanupWorktree(t, mgr, "task-detached-base", "session-detached-base")
|
||||
|
||||
// A detached repository HEAD must not be treated as a containing base. The
|
||||
// branch has no explicit BaseBranch, so cleanup must fail closed when the
|
||||
// only candidate is the repository's detached HEAD.
|
||||
wt.BaseBranch = ""
|
||||
runGit(t, wt.Path, "commit", "--allow-empty", "-m", "unique detached branch")
|
||||
runGit(t, wt.RepositoryPath, "checkout", "--detach", wt.Branch)
|
||||
|
||||
delete, err := mgr.verifyCleanupBranchRedundant(context.Background(), wt, "refs/heads/"+wt.Branch)
|
||||
if err == nil && delete {
|
||||
t.Fatal("cleanup accepted detached repository HEAD as a containing base")
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,309 @@
|
||||
package worktree
|
||||
|
||||
import (
|
||||
"context"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
"github.com/kandev/kandev/internal/task/models"
|
||||
)
|
||||
|
||||
type cleanupOrderRecordingScriptHandler struct {
|
||||
cleanupRuns int
|
||||
}
|
||||
|
||||
func (h *cleanupOrderRecordingScriptHandler) ExecuteSetupScript(context.Context, ScriptExecutionRequest) error {
|
||||
return nil
|
||||
}
|
||||
|
||||
func (h *cleanupOrderRecordingScriptHandler) ExecuteCleanupScript(context.Context, ScriptExecutionRequest) error {
|
||||
h.cleanupRuns++
|
||||
return nil
|
||||
}
|
||||
|
||||
type cancellingCleanupScriptHandler struct {
|
||||
cancel context.CancelFunc
|
||||
}
|
||||
|
||||
func (h *cancellingCleanupScriptHandler) ExecuteSetupScript(context.Context, ScriptExecutionRequest) error {
|
||||
return nil
|
||||
}
|
||||
|
||||
func (h *cancellingCleanupScriptHandler) ExecuteCleanupScript(context.Context, ScriptExecutionRequest) error {
|
||||
h.cancel()
|
||||
return nil
|
||||
}
|
||||
|
||||
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")
|
||||
|
||||
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.Fatalf("recover partial cleanup: %v", err)
|
||||
}
|
||||
|
||||
assertNoCleanupRegistration(t, wt.RepositoryPath, wt.Path)
|
||||
assertCleanupBranchAbsent(t, wt.RepositoryPath, wt.Branch)
|
||||
assertWorktreeReferenceStatus(t, store, wt.ID, StatusDeleted)
|
||||
}
|
||||
|
||||
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")
|
||||
|
||||
if err := os.RemoveAll(wt.Path); err != nil {
|
||||
t.Fatalf("remove worktree path without shared Git metadata: %v", err)
|
||||
}
|
||||
cleanupCtx, cancel := context.WithCancel(context.Background())
|
||||
mgr.SetRepositoryProvider(&fakeRepoProvider{repo: &Repository{ID: wt.RepositoryID, CleanupScript: "true"}})
|
||||
mgr.SetScriptMessageHandler(&cancellingCleanupScriptHandler{cancel: cancel})
|
||||
if err := mgr.CleanupWorktrees(cleanupCtx, []*Worktree{wt}); err == nil {
|
||||
t.Fatal("cleanup cancelled after audit returned nil")
|
||||
}
|
||||
assertWorktreeReferenceStatus(t, store, wt.ID, StatusActive)
|
||||
assertCleanupBranchPresent(t, wt.RepositoryPath, wt.Branch)
|
||||
|
||||
if err := mgr.CleanupWorktrees(context.Background(), []*Worktree{wt}); err != nil {
|
||||
t.Fatalf("retry partial cleanup: %v", err)
|
||||
}
|
||||
assertNoCleanupRegistration(t, wt.RepositoryPath, wt.Path)
|
||||
assertCleanupBranchAbsent(t, wt.RepositoryPath, wt.Branch)
|
||||
assertWorktreeReferenceStatus(t, store, wt.ID, StatusDeleted)
|
||||
}
|
||||
|
||||
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")
|
||||
|
||||
for attempt := 1; attempt <= 2; attempt++ {
|
||||
if err := mgr.CleanupWorktrees(context.Background(), []*Worktree{wt}); err != nil {
|
||||
t.Fatalf("cleanup attempt %d: %v", attempt, err)
|
||||
}
|
||||
}
|
||||
assertNoCleanupRegistration(t, wt.RepositoryPath, wt.Path)
|
||||
assertCleanupBranchAbsent(t, wt.RepositoryPath, wt.Branch)
|
||||
assertWorktreeReferenceStatus(t, store, wt.ID, StatusDeleted)
|
||||
}
|
||||
|
||||
func TestCleanupWorktrees_RefusesUniqueWork(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
add func(*testing.T, *Worktree)
|
||||
expectRemoval bool
|
||||
}{
|
||||
{
|
||||
name: "unmerged commit",
|
||||
expectRemoval: true,
|
||||
add: func(t *testing.T, wt *Worktree) {
|
||||
t.Helper()
|
||||
runGit(t, wt.Path, "commit", "--allow-empty", "-m", "unique local work")
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "untracked file",
|
||||
add: func(t *testing.T, wt *Worktree) {
|
||||
t.Helper()
|
||||
if err := os.WriteFile(filepath.Join(wt.Path, "unique.txt"), []byte("keep me\n"), 0o644); err != nil {
|
||||
t.Fatalf("write unique file: %v", err)
|
||||
}
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "tracked modification",
|
||||
add: func(t *testing.T, wt *Worktree) {
|
||||
t.Helper()
|
||||
if err := os.WriteFile(filepath.Join(wt.Path, "README.md"), []byte("keep this edit\n"), 0o644); err != nil {
|
||||
t.Fatalf("modify tracked file: %v", err)
|
||||
}
|
||||
},
|
||||
},
|
||||
}
|
||||
|
||||
for _, test := range tests {
|
||||
t.Run(test.name, func(t *testing.T) {
|
||||
mgr, store := newReferenceCleanupTestManager(t)
|
||||
taskID := "task-unique-" + strings.ReplaceAll(test.name, " ", "-")
|
||||
sessionID := "session-unique-" + strings.ReplaceAll(test.name, " ", "-")
|
||||
seedReferenceCleanupSession(t, store, taskID, sessionID, models.TaskSessionStateCompleted)
|
||||
wt := createReferenceCleanupWorktree(t, mgr, taskID, sessionID)
|
||||
test.add(t, wt)
|
||||
|
||||
err := mgr.CleanupWorktrees(context.Background(), []*Worktree{wt})
|
||||
assertCleanupBranchPresent(t, wt.RepositoryPath, wt.Branch)
|
||||
if test.expectRemoval {
|
||||
if err != nil {
|
||||
t.Fatalf("cleanup preserving unique commit: %v", err)
|
||||
}
|
||||
if _, statErr := os.Lstat(wt.Path); !os.IsNotExist(statErr) {
|
||||
t.Fatalf("clean worktree path remains after branch preservation: %v", statErr)
|
||||
}
|
||||
assertWorktreeReferenceStatus(t, store, wt.ID, StatusDeleted)
|
||||
return
|
||||
}
|
||||
if err == nil {
|
||||
t.Fatal("cleanup of uncommitted work returned nil")
|
||||
}
|
||||
if _, statErr := os.Lstat(wt.Path); statErr != nil {
|
||||
t.Fatalf("worktree containing untracked work was removed: %v", statErr)
|
||||
}
|
||||
assertWorktreeReferenceStatus(t, store, wt.ID, StatusActive)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestCleanupWorktrees_PreservesUnrelatedWorktreeAndBranch(t *testing.T) {
|
||||
mgr, store := newReferenceCleanupTestManager(t)
|
||||
seedReferenceCleanupSession(t, store, "task-target", "session-target", models.TaskSessionStateCompleted)
|
||||
target := createReferenceCleanupWorktree(t, mgr, "task-target", "session-target")
|
||||
|
||||
unrelatedPath := filepath.Join(t.TempDir(), "unrelated-worktree")
|
||||
runGit(t, target.RepositoryPath, "worktree", "add", "-b", "feature/unrelated", unrelatedPath, "main")
|
||||
runGit(t, unrelatedPath, "commit", "--allow-empty", "-m", "unrelated unique work")
|
||||
unrelatedHead := strings.TrimSpace(runGit(t, unrelatedPath, "rev-parse", "HEAD"))
|
||||
|
||||
if err := mgr.CleanupWorktrees(context.Background(), []*Worktree{target}); err != nil {
|
||||
t.Fatalf("cleanup target worktree: %v", err)
|
||||
}
|
||||
if got := strings.TrimSpace(runGit(t, unrelatedPath, "rev-parse", "HEAD")); got != unrelatedHead {
|
||||
t.Fatalf("unrelated worktree HEAD = %q, want %q", got, unrelatedHead)
|
||||
}
|
||||
assertCleanupBranchPresent(t, target.RepositoryPath, "feature/unrelated")
|
||||
assertWorktreeReferenceStatus(t, store, target.ID, StatusDeleted)
|
||||
}
|
||||
|
||||
func TestCleanupWorktrees_RefusesUnrelatedReplacementAtRecordedPath(t *testing.T) {
|
||||
mgr, store := newReferenceCleanupTestManager(t)
|
||||
seedReferenceCleanupSession(t, store, "task-replaced", "session-replaced", models.TaskSessionStateCompleted)
|
||||
wt := createReferenceCleanupWorktree(t, mgr, "task-replaced", "session-replaced")
|
||||
|
||||
runGit(t, wt.RepositoryPath, "worktree", "remove", "--force", wt.Path)
|
||||
if err := os.MkdirAll(wt.Path, 0o755); err != nil {
|
||||
t.Fatalf("create replacement directory: %v", err)
|
||||
}
|
||||
sentinel := filepath.Join(wt.Path, "unrelated.txt")
|
||||
if err := os.WriteFile(sentinel, []byte("unrelated\n"), 0o644); err != nil {
|
||||
t.Fatalf("write replacement sentinel: %v", err)
|
||||
}
|
||||
|
||||
if err := mgr.CleanupWorktrees(context.Background(), []*Worktree{wt}); err == nil {
|
||||
t.Fatal("cleanup of unrelated replacement returned nil")
|
||||
}
|
||||
if got, err := os.ReadFile(sentinel); err != nil || string(got) != "unrelated\n" {
|
||||
t.Fatalf("replacement sentinel changed: contents=%q err=%v", got, err)
|
||||
}
|
||||
assertCleanupBranchPresent(t, wt.RepositoryPath, wt.Branch)
|
||||
assertWorktreeReferenceStatus(t, store, wt.ID, StatusActive)
|
||||
}
|
||||
|
||||
func TestCleanupWorktrees_AuditsBeforeCleanupScript(t *testing.T) {
|
||||
mgr, store := newReferenceCleanupTestManager(t)
|
||||
seedReferenceCleanupSession(t, store, "task-script-order", "session-script-order", models.TaskSessionStateCompleted)
|
||||
wt := createReferenceCleanupWorktree(t, mgr, "task-script-order", "session-script-order")
|
||||
if err := os.WriteFile(filepath.Join(wt.Path, "README.md"), []byte("keep this edit\n"), 0o644); err != nil {
|
||||
t.Fatalf("modify tracked file: %v", err)
|
||||
}
|
||||
|
||||
provider := &fakeRepoProvider{repo: &Repository{ID: wt.RepositoryID, CleanupScript: "git clean -fd"}}
|
||||
handler := &cleanupOrderRecordingScriptHandler{}
|
||||
mgr.SetRepositoryProvider(provider)
|
||||
mgr.SetScriptMessageHandler(handler)
|
||||
|
||||
if err := mgr.CleanupWorktrees(context.Background(), []*Worktree{wt}); err == nil {
|
||||
t.Fatal("cleanup of dirty worktree returned nil")
|
||||
}
|
||||
if handler.cleanupRuns != 0 {
|
||||
t.Fatalf("cleanup script ran %d times before the audit rejected the worktree, want 0", handler.cleanupRuns)
|
||||
}
|
||||
if _, err := os.Stat(filepath.Join(wt.Path, "README.md")); err != nil {
|
||||
t.Fatalf("dirty worktree was changed before audit: %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestCleanupWorktrees_RejectsChangedHeadFromCleanupSnapshot(t *testing.T) {
|
||||
mgr, store := newReferenceCleanupTestManager(t)
|
||||
seedReferenceCleanupSession(t, store, "task-head-snapshot", "session-head-snapshot", models.TaskSessionStateCompleted)
|
||||
wt := createReferenceCleanupWorktree(t, mgr, "task-head-snapshot", "session-head-snapshot")
|
||||
wt.CleanupHeadOID = strings.TrimSpace(runGit(t, wt.Path, "rev-parse", "HEAD"))
|
||||
runGit(t, wt.Path, "commit", "--allow-empty", "-m", "advance cleanup branch")
|
||||
|
||||
if err := mgr.CleanupWorktrees(context.Background(), []*Worktree{wt}); err == nil {
|
||||
t.Fatal("cleanup accepted a worktree whose HEAD changed after the snapshot")
|
||||
}
|
||||
if _, err := os.Stat(wt.Path); err != nil {
|
||||
t.Fatalf("changed worktree path was removed: %v", err)
|
||||
}
|
||||
assertCleanupBranchPresent(t, wt.RepositoryPath, wt.Branch)
|
||||
assertWorktreeReferenceStatus(t, store, wt.ID, StatusActive)
|
||||
}
|
||||
|
||||
func TestCleanupWorktrees_AllowsMissingBranchMetadataForOwnedPath(t *testing.T) {
|
||||
mgr, store := newReferenceCleanupTestManager(t)
|
||||
seedReferenceCleanupSession(t, store, "task-detached-metadata", "session-detached-metadata", models.TaskSessionStateCompleted)
|
||||
wt := createReferenceCleanupWorktree(t, mgr, "task-detached-metadata", "session-detached-metadata")
|
||||
wt.CleanupHeadOID = strings.TrimSpace(runGit(t, wt.Path, "rev-parse", "HEAD"))
|
||||
wt.Branch = ""
|
||||
|
||||
if err := mgr.CleanupWorktrees(context.Background(), []*Worktree{wt}); err != nil {
|
||||
t.Fatalf("cleanup with missing branch metadata: %v", err)
|
||||
}
|
||||
if _, err := os.Stat(wt.Path); !os.IsNotExist(err) {
|
||||
t.Fatalf("owned path remains after branchless cleanup: %v", err)
|
||||
}
|
||||
assertWorktreeReferenceStatus(t, store, wt.ID, StatusDeleted)
|
||||
}
|
||||
|
||||
func TestCleanupWorktrees_BatchEnrichesCachedRepositoryPath(t *testing.T) {
|
||||
mgr, store := newReferenceCleanupTestManager(t)
|
||||
seedReferenceCleanupSession(t, store, "task-batch-cache", "session-batch-cache", models.TaskSessionStateCompleted)
|
||||
created := createReferenceCleanupWorktree(t, mgr, "task-batch-cache", "session-batch-cache")
|
||||
persisted := *created
|
||||
persisted.RepositoryPath = ""
|
||||
persisted.BaseBranch = ""
|
||||
|
||||
if err := mgr.CleanupWorktrees(context.Background(), []*Worktree{&persisted}); err != nil {
|
||||
t.Fatalf("batch cleanup with an incomplete store projection: %v", err)
|
||||
}
|
||||
assertNoCleanupRegistration(t, created.RepositoryPath, created.Path)
|
||||
assertCleanupBranchAbsent(t, created.RepositoryPath, created.Branch)
|
||||
assertWorktreeReferenceStatus(t, store, created.ID, StatusDeleted)
|
||||
}
|
||||
|
||||
func assertNoCleanupRegistration(t *testing.T, repoPath, worktreePath string) {
|
||||
t.Helper()
|
||||
out := runGit(t, repoPath, "worktree", "list", "--porcelain")
|
||||
want, err := normalizedWorktreeTargetPath(worktreePath)
|
||||
if err != nil {
|
||||
t.Fatalf("normalize expected worktree path: %v", err)
|
||||
}
|
||||
for _, registration := range parseWorktreeRegistrations(strings.ReplaceAll(out, "\n", "\x00")) {
|
||||
got, normalizeErr := normalizedWorktreeTargetPath(registration.path)
|
||||
if normalizeErr != nil {
|
||||
t.Fatalf("normalize registered worktree path %q: %v", registration.path, normalizeErr)
|
||||
}
|
||||
if got == want {
|
||||
t.Fatalf("worktree registration remains for %q:\n%s", worktreePath, out)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
func assertCleanupBranchPresent(t *testing.T, repoPath, branch string) {
|
||||
t.Helper()
|
||||
if got := strings.TrimSpace(runGit(t, repoPath, "branch", "--list", branch)); got == "" {
|
||||
t.Fatalf("branch %q was deleted", branch)
|
||||
}
|
||||
}
|
||||
|
||||
func assertCleanupBranchAbsent(t *testing.T, repoPath, branch string) {
|
||||
t.Helper()
|
||||
if got := strings.TrimSpace(runGit(t, repoPath, "branch", "--list", branch)); got != "" {
|
||||
t.Fatalf("branch %q remains: %q", branch, got)
|
||||
}
|
||||
}
|
||||
@@ -70,6 +70,12 @@ type Worktree struct {
|
||||
// Branch is the Git branch name checked out in this worktree.
|
||||
Branch string `json:"branch"`
|
||||
|
||||
// CleanupHeadOID is the immutable checkout identity captured by the durable
|
||||
// task-cleanup snapshot. It is intentionally internal: ordinary worktree
|
||||
// callers do not need to provide it, while durable cleanup uses it to fail
|
||||
// closed if the recorded path or branch advanced before teardown.
|
||||
CleanupHeadOID string `json:"-"`
|
||||
|
||||
// BaseBranch is the branch this worktree was created from.
|
||||
BaseBranch string `json:"base_branch"`
|
||||
|
||||
|
||||
@@ -259,13 +259,13 @@ Read-only Git actions used by the Changes panel include `session.commit_diff`, `
|
||||
|
||||
## Cleanup and data loss
|
||||
|
||||
Worktree cleanup runs the repository cleanup script, forcibly removes the Git worktree directory, and may remove the local branch:
|
||||
Worktree cleanup audits the Git worktree and checkout before it runs the repository cleanup script. It then removes the Git worktree directory and may remove the local branch:
|
||||
|
||||
- Normal task deletion cleans all owned task worktrees and runs `git branch -D` for their local branches. Remote branches are not deleted, but uncommitted and unpushed-only work can be lost.
|
||||
- Normal task deletion audits each owned worktree before mutation. Tracked or untracked changes keep the checkout in place and make durable cleanup retry. A clean branch is removed only when its current commit is already contained by the recorded base or repository default; a clean branch with unique commits is preserved after its checkout is reclaimed. Remote branches are never deleted.
|
||||
- **Reset Environment** is allowed only when no task session is `STARTING` or `RUNNING`. It can optionally push first; a failed requested push aborts the reset. Teardown removes the worktree but deliberately preserves the local branch, then the next launch materializes a fresh environment.
|
||||
- Office handoff cleanup also preserves the branch when it releases a worktree.
|
||||
|
||||
Before deleting a task or performing a hard reset, commit and push anything you need. A cleanup-script failure does not save the directory: Kandev logs the failure and proceeds. If `git worktree remove --force` fails, managed cleanup can fall back to deleting the directory and pruning Git's stale worktree record.
|
||||
Before deleting a task or performing a hard reset, commit and push anything you need. Kandev does not run a cleanup script when the audit finds uncommitted or untracked work. Cleanup scripts should perform transient teardown only; files they create are removed with the audited checkout. If an audited cleanup script fails, Kandev logs the failure and continues with the same recorded worktree. An audited directory is removed through its pinned no-follow handle, and un-audited fallback cleanup can remove a managed directory without following replacement links. Git metadata is then pruned. Registration pruning and local-branch deletion are verified before the durable cleanup job succeeds; a partial failure remains retryable.
|
||||
|
||||
## Troubleshooting
|
||||
|
||||
|
||||
@@ -2,7 +2,7 @@
|
||||
status: draft
|
||||
system: tasks
|
||||
created: 2026-06-22
|
||||
updated: 2026-08-28
|
||||
updated: 2026-08-31
|
||||
owners:
|
||||
- cfl
|
||||
---
|
||||
@@ -33,3 +33,4 @@ and safe when runtimes or task rows are already gone.
|
||||
reactivate the recoverable task-owned worktree instead of requiring
|
||||
attach-only reuse of the deleted workspace.
|
||||
- **AC-TASKS-RUNTIME-CLEANUP-001.8:** When dead-row repair loses its compare-and-set to a newer execution, reconciliation shall preserve the newer row without a warning. Other repair errors shall remain warnings.
|
||||
- **AC-TASKS-RUNTIME-CLEANUP-001.9:** When task deletion reclaims a Git worktree, the system shall verify the exact recorded path, branch, and commit before mutation; preserve a checkout with tracked or untracked changes; preserve a clean local branch with commits not contained by the recorded base or repository default; recover idempotently when the checkout path is already absent; and keep failed registration or branch cleanup retryable.
|
||||
|
||||
@@ -4,7 +4,7 @@ system: tasks
|
||||
requirements:
|
||||
- REQ-TASKS-RUNTIME-CLEANUP-001
|
||||
created: 2026-06-22
|
||||
updated: 2026-08-28
|
||||
updated: 2026-08-31
|
||||
owners:
|
||||
- cfl
|
||||
---
|
||||
@@ -217,7 +217,7 @@ migration lock. The migration never performs filesystem or Git cleanup.
|
||||
`task_resource_cleanup_jobs` is the durable task-lifecycle cleanup intent. It has
|
||||
no foreign key to `tasks`, so delete cleanup survives deletion of the owning row.
|
||||
It stores the trigger, state, retry timing, last error, and a JSON snapshot of the
|
||||
runtime, environment, worktree, and path handles captured before task mutation.
|
||||
runtime, environment, worktree OIDs, and path handles captured before task mutation.
|
||||
Only one non-terminal row exists for an operation ID; repeated event delivery
|
||||
reuses the same cleanup job. `attempts` counts successful worker claims. A
|
||||
terminal `failed` row retains its final error and completion timestamp for
|
||||
@@ -330,6 +330,7 @@ The durable cleanup job wraps that resource lifecycle:
|
||||
`completed_at`, and is excluded from automatic due-job selection.
|
||||
- If cleanup cannot prove that a session worktree belongs to the task being
|
||||
cleaned, destructive worktree deletion fails closed and skips that worktree.
|
||||
Stale Git state is removed only with pinned path, branch, and commit ownership.
|
||||
- If an agentctl process exits unexpectedly, its owned agent subprocess group is
|
||||
killed before agentctl shutdown completes.
|
||||
- If the user sends Ctrl+C to a standalone Kandev process tree, agentctl does
|
||||
|
||||
Reference in New Issue
Block a user