diff --git a/apps/backend/internal/task/service/service_resources.go b/apps/backend/internal/task/service/service_resources.go index 74989bfa2d..f7614948d3 100644 --- a/apps/backend/internal/task/service/service_resources.go +++ b/apps/backend/internal/task/service/service_resources.go @@ -2,6 +2,7 @@ package service import ( "context" + "errors" "fmt" "strings" "time" @@ -202,19 +203,48 @@ func (s *Service) SetWorkflowHidden(ctx context.Context, id string, hidden bool) return nil } -// DeleteWorkflow deletes a workflow +// DeleteWorkflow deletes a workflow, archiving its remaining tasks first so +// they do not linger as orphan rows pointing at a workflow_id that no longer +// exists (the tasks.workflow_id FK was dropped to support empty workflow_id +// on ephemeral tasks, so SQLite cannot cascade for us). func (s *Service) DeleteWorkflow(ctx context.Context, id string) error { workflow, err := s.workflows.GetWorkflow(ctx, id) if err != nil { return err } + + tasks, err := s.tasks.ListTasks(ctx, id) + if err != nil { + s.logger.Error("failed to list tasks for workflow delete cascade", + zap.String("workflow_id", id), zap.Error(err)) + return err + } + archived := 0 + for _, task := range tasks { + if err := s.ArchiveTask(ctx, task.ID); err != nil { + // Concurrent archive between ListTasks and here is a no-op: + // the task is already in the desired state, keep cascading. + if errors.Is(err, ErrTaskAlreadyArchived) { + continue + } + s.logger.Error("failed to archive task during workflow delete cascade", + zap.String("workflow_id", id), + zap.String("task_id", task.ID), + zap.Error(err)) + return err + } + archived++ + } + if err := s.workflows.DeleteWorkflow(ctx, id); err != nil { s.logger.Error("failed to delete workflow", zap.String("workflow_id", id), zap.Error(err)) return err } s.publishWorkflowEvent(ctx, events.WorkflowDeleted, workflow) - s.logger.Info("workflow deleted", zap.String("workflow_id", id)) + s.logger.Info("workflow deleted", + zap.String("workflow_id", id), + zap.Int("archived_tasks", archived)) return nil } diff --git a/apps/backend/internal/task/service/service_resources_test.go b/apps/backend/internal/task/service/service_resources_test.go index c00b32e733..c77677b3e5 100644 --- a/apps/backend/internal/task/service/service_resources_test.go +++ b/apps/backend/internal/task/service/service_resources_test.go @@ -6,6 +6,7 @@ import ( "testing" "github.com/kandev/kandev/internal/task/models" + "github.com/kandev/kandev/internal/task/repository" ) // errWorkspaceRepo is a WorkspaceRepository that always returns an error from @@ -128,6 +129,186 @@ func TestService_GetOfficeWorkflowIDs_DBError(t *testing.T) { } } +// TestService_DeleteWorkflow_ArchivesChildTasks verifies the cascade fix for +// issue #1279: workflow deletion archives any active child tasks instead of +// leaving them with a dangling workflow_id (tasks.workflow_id has no FK, so +// SQLite cannot CASCADE for us). +func TestService_DeleteWorkflow_ArchivesChildTasks(t *testing.T) { + svc, _, repo := createTestService(t) + ctx := context.Background() + + _ = repo.CreateWorkspace(ctx, &models.Workspace{ID: "ws-1", Name: "WS"}) + _ = repo.CreateWorkflow(ctx, &models.Workflow{ID: "wf-doomed", WorkspaceID: "ws-1", Name: "Doomed"}) + _ = repo.CreateWorkflow(ctx, &models.Workflow{ID: "wf-keep", WorkspaceID: "ws-1", Name: "Keep"}) + + tasks := []*models.Task{ + {ID: "task-a", WorkspaceID: "ws-1", WorkflowID: "wf-doomed", WorkflowStepID: "step-1", Title: "A"}, + {ID: "task-b", WorkspaceID: "ws-1", WorkflowID: "wf-doomed", WorkflowStepID: "step-1", Title: "B"}, + {ID: "task-other", WorkspaceID: "ws-1", WorkflowID: "wf-keep", WorkflowStepID: "step-1", Title: "Other"}, + } + for _, task := range tasks { + if err := repo.CreateTask(ctx, task); err != nil { + t.Fatalf("CreateTask %s: %v", task.ID, err) + } + } + + if err := svc.DeleteWorkflow(ctx, "wf-doomed"); err != nil { + t.Fatalf("DeleteWorkflow: %v", err) + } + + if _, err := svc.workflows.GetWorkflow(ctx, "wf-doomed"); err == nil { + t.Fatalf("expected workflow to be deleted") + } + + for _, id := range []string{"task-a", "task-b"} { + got, err := repo.GetTask(ctx, id) + if err != nil { + t.Fatalf("GetTask %s after cascade: %v", id, err) + } + if got.ArchivedAt == nil { + t.Errorf("task %s: expected archived_at to be set, got nil", id) + } + } + + other, err := repo.GetTask(ctx, "task-other") + if err != nil { + t.Fatalf("GetTask task-other: %v", err) + } + if other.ArchivedAt != nil { + t.Errorf("task in unrelated workflow should not be archived, got archived_at=%v", other.ArchivedAt) + } +} + +// leakyListTaskRepo wraps the real TaskRepository and injects extra tasks +// into ListTasks results, simulating a TOCTOU race where a task is archived +// between the snapshot and the cascade loop. +type leakyListTaskRepo struct { + repository.TaskRepository + extra []*models.Task +} + +func (l leakyListTaskRepo) ListTasks(ctx context.Context, workflowID string) ([]*models.Task, error) { + real, err := l.TaskRepository.ListTasks(ctx, workflowID) + if err != nil { + return nil, err + } + return append(real, l.extra...), nil +} + +// TestService_DeleteWorkflow_SkipsConcurrentlyArchivedTask covers the +// TOCTOU race window between Service.tasks.ListTasks and Service.ArchiveTask: +// if a task is archived by another caller in that window, ArchiveTask +// returns ErrTaskAlreadyArchived and the cascade must continue rather than +// abort, so the workflow row still gets deleted. +func TestService_DeleteWorkflow_SkipsConcurrentlyArchivedTask(t *testing.T) { + svc, _, repo := createTestService(t) + ctx := context.Background() + + _ = repo.CreateWorkspace(ctx, &models.Workspace{ID: "ws-1", Name: "WS"}) + _ = repo.CreateWorkflow(ctx, &models.Workflow{ID: "wf-doomed", WorkspaceID: "ws-1", Name: "Doomed"}) + + if err := repo.CreateTask(ctx, &models.Task{ + ID: "task-live", WorkspaceID: "ws-1", WorkflowID: "wf-doomed", WorkflowStepID: "step-1", Title: "Live", + }); err != nil { + t.Fatalf("CreateTask live: %v", err) + } + if err := repo.CreateTask(ctx, &models.Task{ + ID: "task-raced", WorkspaceID: "ws-1", WorkflowID: "wf-doomed", WorkflowStepID: "step-1", Title: "Raced", + }); err != nil { + t.Fatalf("CreateTask raced: %v", err) + } + if err := repo.ArchiveTask(ctx, "task-raced"); err != nil { + t.Fatalf("pre-archive raced task: %v", err) + } + + raced, err := repo.GetTask(ctx, "task-raced") + if err != nil { + t.Fatalf("GetTask raced: %v", err) + } + svc.tasks = leakyListTaskRepo{TaskRepository: repo, extra: []*models.Task{raced}} + + if err := svc.DeleteWorkflow(ctx, "wf-doomed"); err != nil { + t.Fatalf("DeleteWorkflow should swallow ErrTaskAlreadyArchived: %v", err) + } + + if _, err := svc.workflows.GetWorkflow(ctx, "wf-doomed"); err == nil { + t.Fatalf("expected workflow to be deleted despite race") + } + + got, err := repo.GetTask(ctx, "task-live") + if err != nil { + t.Fatalf("GetTask live: %v", err) + } + if got.ArchivedAt == nil { + t.Errorf("live task should be archived by cascade") + } +} + +// TestService_DeleteWorkflow_PartialArchiveErrorPreservesWorkflow verifies +// the fail-fast contract: when ArchiveTask returns a non-sentinel error +// part-way through the cascade, DeleteWorkflow surfaces it, leaves the +// workflow row intact, and the tasks archived before the failure stay +// archived. Retries are safe because ListTasks filters them out. +func TestService_DeleteWorkflow_PartialArchiveErrorPreservesWorkflow(t *testing.T) { + svc, _, repo := createTestService(t) + ctx := context.Background() + + _ = repo.CreateWorkspace(ctx, &models.Workspace{ID: "ws-1", Name: "WS"}) + _ = repo.CreateWorkflow(ctx, &models.Workflow{ID: "wf-doomed", WorkspaceID: "ws-1", Name: "Doomed"}) + + if err := repo.CreateTask(ctx, &models.Task{ + ID: "task-first", WorkspaceID: "ws-1", WorkflowID: "wf-doomed", WorkflowStepID: "step-1", Title: "First", + }); err != nil { + t.Fatalf("CreateTask first: %v", err) + } + // "task-ghost" never actually exists in the DB — the leaky list returns + // it so the cascade's ArchiveTask call hits a real GetTask error. + ghost := &models.Task{ID: "task-ghost", WorkspaceID: "ws-1", WorkflowID: "wf-doomed", WorkflowStepID: "step-1", Title: "Ghost"} + svc.tasks = leakyListTaskRepo{TaskRepository: repo, extra: []*models.Task{ghost}} + + err := svc.DeleteWorkflow(ctx, "wf-doomed") + if err == nil { + t.Fatalf("expected error when ArchiveTask fails mid-cascade") + } + if errors.Is(err, ErrTaskAlreadyArchived) { + t.Fatalf("non-sentinel error must propagate, got sentinel: %v", err) + } + + if _, err := svc.workflows.GetWorkflow(ctx, "wf-doomed"); err != nil { + t.Fatalf("workflow row must survive a partial cascade, got: %v", err) + } + first, err := repo.GetTask(ctx, "task-first") + if err != nil { + t.Fatalf("GetTask first: %v", err) + } + if first.ArchivedAt == nil { + t.Errorf("task archived before the failure should remain archived") + } +} + +// TestService_ArchiveTask_ReturnsAlreadyArchivedSentinel locks in the +// sentinel-error contract DeleteWorkflow relies on. +func TestService_ArchiveTask_ReturnsAlreadyArchivedSentinel(t *testing.T) { + svc, _, repo := createTestService(t) + ctx := context.Background() + + _ = repo.CreateWorkspace(ctx, &models.Workspace{ID: "ws-1", Name: "WS"}) + _ = repo.CreateWorkflow(ctx, &models.Workflow{ID: "wf-1", WorkspaceID: "ws-1", Name: "WF"}) + if err := repo.CreateTask(ctx, &models.Task{ + ID: "task-1", WorkspaceID: "ws-1", WorkflowID: "wf-1", WorkflowStepID: "step-1", Title: "T", + }); err != nil { + t.Fatalf("CreateTask: %v", err) + } + + if err := svc.ArchiveTask(ctx, "task-1"); err != nil { + t.Fatalf("first ArchiveTask: %v", err) + } + err := svc.ArchiveTask(ctx, "task-1") + if !errors.Is(err, ErrTaskAlreadyArchived) { + t.Fatalf("second ArchiveTask: want ErrTaskAlreadyArchived, got %v", err) + } +} + // TestApplyRepositoryUpdates_CopyFilesNilLeavesUntouched verifies the // pointer-nil convention: a nil CopyFiles field on the request must not // clobber an existing repository value. diff --git a/apps/backend/internal/task/service/service_tasks.go b/apps/backend/internal/task/service/service_tasks.go index f348981da9..8240549140 100644 --- a/apps/backend/internal/task/service/service_tasks.go +++ b/apps/backend/internal/task/service/service_tasks.go @@ -2,6 +2,7 @@ package service import ( "context" + "errors" "fmt" "net/url" "os" @@ -29,6 +30,12 @@ const defaultPriority = "medium" // intentionally exempt. var ErrSubtaskDepthExceeded = fmt.Errorf("cannot create a subtask of a subtask — maximum nesting depth is 1 for kanban tasks. Create a sibling task under the same parent or a top-level task instead") +// ErrTaskAlreadyArchived is returned by ArchiveTask when the target task +// already has archived_at set. Sentinel so cascade callers (e.g. +// DeleteWorkflow) can treat a concurrent archive as a no-op instead of +// aborting the whole operation. +var ErrTaskAlreadyArchived = errors.New("task is already archived") + type taskStopTarget struct { sessionID string executionID string @@ -729,7 +736,7 @@ func (s *Service) ArchiveTask(ctx context.Context, id string) error { } if task.ArchivedAt != nil { - return fmt.Errorf("task is already archived: %s", id) + return fmt.Errorf("%w: %s", ErrTaskAlreadyArchived, id) } // 2. Gather data needed for cleanup BEFORE archive diff --git a/apps/web/components/settings/workflow-card-dialogs.tsx b/apps/web/components/settings/workflow-card-dialogs.tsx index 927b75e79d..16452d04cd 100644 --- a/apps/web/components/settings/workflow-card-dialogs.tsx +++ b/apps/web/components/settings/workflow-card-dialogs.tsx @@ -50,7 +50,7 @@ export function WorkflowDeleteDialog({ Delete workflow {hasTasks - ? `This workflow has ${workflowTaskCount} task${workflowTaskCount === 1 ? "" : "s"}. Choose where to migrate them, or delete everything.` + ? `This workflow has ${workflowTaskCount} task${workflowTaskCount === 1 ? "" : "s"}. Choose where to migrate them, or delete the workflow and archive the tasks.` : "This will permanently delete the workflow and all its steps."} @@ -116,7 +116,7 @@ export function WorkflowDeleteDialog({ disabled={deleteLoading || migrateLoading} className="cursor-pointer" > - {hasTasks ? "Delete Everything" : "Delete Workflow"} + {hasTasks ? "Delete & Archive Tasks" : "Delete Workflow"}