From a4a6bce577ab28f1626f65d014b4d59c750a7361 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Salavisa?= Date: Thu, 11 Jun 2026 15:41:09 +0100 Subject: [PATCH] fix(task): cascade-archive child tasks on workflow delete (#1332) * fix(task): cascade-archive child tasks on workflow delete Tasks pointing at a deleted workflow_id silently became unreachable because the FK on tasks.workflow_id was dropped to support ephemeral quick-chat tasks, so SQLite cannot CASCADE. Service.DeleteWorkflow now archives every active child task before removing the workflow, re-using the existing ArchiveTask path (events, worktree cleanup, agent stop). Frontend confirm copy and button label updated to reflect the new behavior. Closes #1279 Co-Authored-By: Claude Opus 4.7 * fix(task): make workflow-delete cascade race-safe Address review feedback (Greptile, Claude, cubic): ArchiveTask now returns a typed ErrTaskAlreadyArchived sentinel so DeleteWorkflow can swallow the TOCTOU race where a task is archived between ListTasks and ArchiveTask. Without the sentinel, that race would propagate as an opaque error and leave the workflow row intact even though every task in the snapshot was already in the desired state. Tests: - TestService_DeleteWorkflow_SkipsConcurrentlyArchivedTask covers the sentinel-skip path through the cascade. - TestService_DeleteWorkflow_PartialArchiveErrorPreservesWorkflow covers the fail-fast contract on a real (non-sentinel) error: workflow row survives, previously-archived tasks stay archived, retry is safe. - TestService_ArchiveTask_ReturnsAlreadyArchivedSentinel locks in the sentinel-error contract DeleteWorkflow relies on. Co-Authored-By: Claude Opus 4.7 --------- Co-authored-by: Kandev Agent Co-authored-by: Claude Opus 4.7 --- .../task/service/service_resources.go | 34 +++- .../task/service/service_resources_test.go | 181 ++++++++++++++++++ .../internal/task/service/service_tasks.go | 9 +- .../settings/workflow-card-dialogs.tsx | 4 +- 4 files changed, 223 insertions(+), 5 deletions(-) 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"}