From 657b92eb69fa37d6c71941cb3d735ba80e3a97d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Tue, 30 Jun 2026 08:24:43 +0100 Subject: [PATCH] fix: restore workspace deletion from settings (#1547) * fix: restore workspace deletion from settings * fix: validate workspace delete in service * fix: clarify workspace delete errors * fix: address workspace delete review feedback * fix: guard confirmed workspace delete before cascade * fix: simplify workspace delete cascade guard * fix: make confirmed workspace delete atomic * fix: preserve confirmed workspace delete side effects * fix: snapshot confirmed workspace cascade rows * fix: clarify confirmed workspace delete cleanup * fix: preserve office workspace deletion cleanup * fix: preserve workspace delete error mapping * fix: streamline workspace delete cascade * fix: harden workspace delete cleanup * fix: keep workspace delete failure logs * fix: fail workspace delete on missing session inventory --------- Co-authored-by: root --- .../executor/executor_mocks_test.go | 3 + apps/backend/internal/task/handlers/errors.go | 1 + .../internal/task/handlers/errors_test.go | 11 +- .../task/handlers/process_handlers_test.go | 6 + .../task/handlers/workspace_handlers.go | 72 ++- .../task/handlers/workspace_handlers_test.go | 365 +++++++++++++ .../internal/task/repository/interface.go | 6 + .../task/repository/repoerrors/errors.go | 10 + .../internal/task/repository/sqlite/errors.go | 10 +- .../task/repository/sqlite/workflow.go | 93 ++-- .../task/repository/sqlite/workspace.go | 180 ++++++- .../task/repository/sqlite/workspace_test.go | 278 ++++++++++ .../task/service/service_resources.go | 240 ++++++++- .../task/service/service_resources_test.go | 504 ++++++++++++++++++ .../internal/task/service/service_tasks.go | 45 +- .../internal/task/service/service_test.go | 34 +- .../task/dockview-desktop-layout.tsx | 75 ++- 17 files changed, 1826 insertions(+), 107 deletions(-) create mode 100644 apps/backend/internal/task/handlers/workspace_handlers_test.go create mode 100644 apps/backend/internal/task/repository/repoerrors/errors.go create mode 100644 apps/backend/internal/task/repository/sqlite/workspace_test.go diff --git a/apps/backend/internal/orchestrator/executor/executor_mocks_test.go b/apps/backend/internal/orchestrator/executor/executor_mocks_test.go index fae265b79d..6f1c877f76 100644 --- a/apps/backend/internal/orchestrator/executor/executor_mocks_test.go +++ b/apps/backend/internal/orchestrator/executor/executor_mocks_test.go @@ -393,6 +393,9 @@ func (m *mockRepository) UpdateWorkspace(ctx context.Context, workspace *models. return nil } func (m *mockRepository) DeleteWorkspace(ctx context.Context, id string) error { return nil } +func (m *mockRepository) DeleteWorkspaceCascadeWithName(ctx context.Context, id, name string) ([]*models.Task, []*models.Workflow, error) { + return nil, nil, m.DeleteWorkspace(ctx, id) +} func (m *mockRepository) ListWorkspaces(ctx context.Context) ([]*models.Workspace, error) { return nil, nil } diff --git a/apps/backend/internal/task/handlers/errors.go b/apps/backend/internal/task/handlers/errors.go index 1b3d15c9f3..2bceec8c9d 100644 --- a/apps/backend/internal/task/handlers/errors.go +++ b/apps/backend/internal/task/handlers/errors.go @@ -44,6 +44,7 @@ func isNotFound(err error) bool { return false } if errors.Is(err, taskrepo.ErrTaskNotFound) || + errors.Is(err, taskrepo.ErrWorkspaceNotFound) || errors.Is(err, taskrepo.ErrNoPrimarySession) || errors.Is(err, service.ErrDocumentNotFound) || errors.Is(err, service.ErrTaskPlanNotFound) || diff --git a/apps/backend/internal/task/handlers/errors_test.go b/apps/backend/internal/task/handlers/errors_test.go index 52b28196d0..4a645c2f91 100644 --- a/apps/backend/internal/task/handlers/errors_test.go +++ b/apps/backend/internal/task/handlers/errors_test.go @@ -17,11 +17,12 @@ import ( // status mapping survives downstream wrap/unwrap changes. func TestErrorsAreClassifiable(t *testing.T) { notFoundSentinels := map[string]error{ - "taskrepo.ErrTaskNotFound": taskrepo.ErrTaskNotFound, - "taskrepo.ErrNoPrimarySession": taskrepo.ErrNoPrimarySession, - "service.ErrDocumentNotFound": service.ErrDocumentNotFound, - "service.ErrTaskPlanNotFound": service.ErrTaskPlanNotFound, - "service.ErrRevisionNotFound": service.ErrRevisionNotFound, + "taskrepo.ErrTaskNotFound": taskrepo.ErrTaskNotFound, + "taskrepo.ErrWorkspaceNotFound": taskrepo.ErrWorkspaceNotFound, + "taskrepo.ErrNoPrimarySession": taskrepo.ErrNoPrimarySession, + "service.ErrDocumentNotFound": service.ErrDocumentNotFound, + "service.ErrTaskPlanNotFound": service.ErrTaskPlanNotFound, + "service.ErrRevisionNotFound": service.ErrRevisionNotFound, } for name, sentinel := range notFoundSentinels { t.Run("isNotFound recognizes "+name, func(t *testing.T) { diff --git a/apps/backend/internal/task/handlers/process_handlers_test.go b/apps/backend/internal/task/handlers/process_handlers_test.go index 74e0e554fa..f9d7a57474 100644 --- a/apps/backend/internal/task/handlers/process_handlers_test.go +++ b/apps/backend/internal/task/handlers/process_handlers_test.go @@ -44,6 +44,12 @@ func (m *mockRepository) UpdateWorkspace(ctx context.Context, workspace *models. func (m *mockRepository) DeleteWorkspace(ctx context.Context, id string) error { return nil } +func (m *mockRepository) DeleteWorkspaceCascade(ctx context.Context, id string) ([]*models.Task, []*models.Workflow, error) { + return nil, nil, m.DeleteWorkspace(ctx, id) +} +func (m *mockRepository) DeleteWorkspaceCascadeWithName(ctx context.Context, id, name string) ([]*models.Task, []*models.Workflow, error) { + return nil, nil, m.DeleteWorkspace(ctx, id) +} func (m *mockRepository) ListWorkspaces(ctx context.Context) ([]*models.Workspace, error) { return nil, nil } diff --git a/apps/backend/internal/task/handlers/workspace_handlers.go b/apps/backend/internal/task/handlers/workspace_handlers.go index e643ebcba0..0033df5d35 100644 --- a/apps/backend/internal/task/handlers/workspace_handlers.go +++ b/apps/backend/internal/task/handlers/workspace_handlers.go @@ -2,6 +2,7 @@ package handlers import ( "context" + "errors" "net/http" "github.com/gin-gonic/gin" @@ -147,14 +148,42 @@ func (h *WorkspaceHandlers) httpUpdateWorkspace(c *gin.Context) { c.JSON(http.StatusOK, dto.FromWorkspace(workspace)) } +type httpDeleteWorkspaceRequest struct { + ConfirmName string `json:"confirm_name"` +} + func (h *WorkspaceHandlers) httpDeleteWorkspace(c *gin.Context) { - if err := h.service.DeleteWorkspace(c.Request.Context(), c.Param("id")); err != nil { - handleNotFound(c, h.logger, err, "workspace not deleted") + var body httpDeleteWorkspaceRequest + if err := c.ShouldBindJSON(&body); err != nil { + c.JSON(http.StatusBadRequest, gin.H{"error": "invalid payload"}) + return + } + if body.ConfirmName == "" { + c.JSON(http.StatusBadRequest, gin.H{"error": "confirm_name is required"}) + return + } + if err := h.service.DeleteWorkspaceWithConfirmName( + c.Request.Context(), + c.Param("id"), + body.ConfirmName, + ); err != nil { + h.handleHTTPDeleteWorkspaceError(c, err) return } c.JSON(http.StatusOK, dto.SuccessResponse{Success: true}) } +func (h *WorkspaceHandlers) handleHTTPDeleteWorkspaceError(c *gin.Context, err error) { + switch { + case errors.Is(err, service.ErrWorkspaceConfirmNameMismatch): + c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) + case isNotFound(err): + c.JSON(http.StatusNotFound, gin.H{"error": "workspace not found"}) + default: + c.JSON(http.StatusInternalServerError, gin.H{"error": "workspace not deleted"}) + } +} + // WS handlers func (h *WorkspaceHandlers) wsListWorkspaces(ctx context.Context, msg *ws.Message) (*ws.Message, error) { @@ -254,12 +283,35 @@ func (h *WorkspaceHandlers) wsUpdateWorkspace(ctx context.Context, msg *ws.Messa return ws.NewResponse(msg.ID, msg.Action, dto.FromWorkspace(workspace)) } -func (h *WorkspaceHandlers) wsDeleteWorkspace(ctx context.Context, msg *ws.Message) (*ws.Message, error) { - return wsHandleIDRequest(ctx, msg, h.logger, "failed to delete workspace", - func(ctx context.Context, id string) (any, error) { - if err := h.service.DeleteWorkspace(ctx, id); err != nil { - return nil, err - } - return dto.SuccessResponse{Success: true}, nil - }) +type wsDeleteWorkspaceRequest struct { + ID string `json:"id"` + ConfirmName string `json:"confirm_name"` +} + +func (h *WorkspaceHandlers) wsDeleteWorkspace(ctx context.Context, msg *ws.Message) (*ws.Message, error) { + var req wsDeleteWorkspaceRequest + if err := msg.ParsePayload(&req); err != nil { + return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeBadRequest, "Invalid payload: "+err.Error(), nil) + } + if req.ID == "" { + return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeValidation, "id is required", nil) + } + if req.ConfirmName == "" { + return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeValidation, "confirm_name is required", nil) + } + if err := h.service.DeleteWorkspaceWithConfirmName(ctx, req.ID, req.ConfirmName); err != nil { + return h.wsDeleteWorkspaceError(msg, err) + } + return ws.NewResponse(msg.ID, msg.Action, dto.SuccessResponse{Success: true}) +} + +func (h *WorkspaceHandlers) wsDeleteWorkspaceError(msg *ws.Message, err error) (*ws.Message, error) { + switch { + case errors.Is(err, service.ErrWorkspaceConfirmNameMismatch): + return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeValidation, err.Error(), nil) + case isNotFound(err): + return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeNotFound, "Workspace not found", nil) + default: + return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeInternalError, "failed to delete workspace", nil) + } } diff --git a/apps/backend/internal/task/handlers/workspace_handlers_test.go b/apps/backend/internal/task/handlers/workspace_handlers_test.go new file mode 100644 index 0000000000..2f1f5b3fb8 --- /dev/null +++ b/apps/backend/internal/task/handlers/workspace_handlers_test.go @@ -0,0 +1,365 @@ +package handlers + +import ( + "context" + "errors" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/gin-gonic/gin" + "github.com/stretchr/testify/require" + + "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/task/models" + "github.com/kandev/kandev/internal/task/service" + ws "github.com/kandev/kandev/pkg/websocket" +) + +type workspaceDeleteRepo struct { + mockRepository + deleteCalled bool + cascadeCalled bool + cascadeID string + cascadeName string + cascadeTasks []*models.Task + cascadeWorkflows []*models.Workflow + cascadeErr error + getCalls int + getErr error +} + +func (r *workspaceDeleteRepo) GetWorkspace(_ context.Context, id string) (*models.Workspace, error) { + r.getCalls++ + if r.getErr != nil { + return nil, r.getErr + } + return &models.Workspace{ID: id, Name: "Delete Me"}, nil +} + +func (r *workspaceDeleteRepo) DeleteWorkspace(_ context.Context, _ string) error { + r.deleteCalled = true + return nil +} + +func (r *workspaceDeleteRepo) DeleteWorkspaceCascade(_ context.Context, id string) ([]*models.Task, []*models.Workflow, error) { + r.cascadeCalled = true + r.cascadeID = id + r.cascadeName = "" + if r.cascadeErr != nil { + return nil, nil, r.cascadeErr + } + return r.cascadeTasks, r.cascadeWorkflows, nil +} + +func (r *workspaceDeleteRepo) DeleteWorkspaceCascadeWithName(_ context.Context, id, name string) ([]*models.Task, []*models.Workflow, error) { + r.cascadeCalled = true + r.cascadeID = id + r.cascadeName = name + if r.cascadeErr != nil { + return nil, nil, r.cascadeErr + } + return r.cascadeTasks, r.cascadeWorkflows, nil +} + +func TestHTTPDeleteWorkspaceRequiresMatchingConfirmName(t *testing.T) { + gin.SetMode(gin.TestMode) + repo := &workspaceDeleteRepo{} + log, _ := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "json", OutputPath: "stdout"}) + svc := service.NewService(service.Repos{ + Workspaces: repo, + Tasks: repo, + TaskRepos: repo, + Workflows: repo, + Messages: repo, + Turns: repo, + Sessions: repo, + GitSnapshots: repo, + RepoEntities: repo, + Executors: repo, + Environments: repo, + TaskEnvironments: repo, + Reviews: repo, + }, nil, log, service.RepositoryDiscoveryConfig{}) + + h := NewWorkspaceHandlers(svc, log) + rec := httptest.NewRecorder() + c, _ := gin.CreateTestContext(rec) + c.Request = httptest.NewRequest( + http.MethodDelete, + "/api/v1/workspaces/ws-1", + strings.NewReader(`{"confirm_name":"Wrong"}`), + ) + c.Request.Header.Set("Content-Type", "application/json") + c.Params = gin.Params{{Key: "id", Value: "ws-1"}} + + h.httpDeleteWorkspace(c) + + require.Equal(t, http.StatusBadRequest, rec.Code) + require.False(t, repo.deleteCalled, "handler must not delete when confirm_name does not match") + require.False(t, repo.cascadeCalled, "handler must not cascade when confirm_name does not match") + require.Equal(t, 1, repo.getCalls, "handler should leave confirmation lookup to the service") +} + +func TestHTTPDeleteWorkspaceRejectsInvalidPayload(t *testing.T) { + gin.SetMode(gin.TestMode) + repo := &workspaceDeleteRepo{} + log, _ := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "json", OutputPath: "stdout"}) + svc := service.NewService(service.Repos{ + Workspaces: repo, + Tasks: repo, + TaskRepos: repo, + Workflows: repo, + Messages: repo, + Turns: repo, + Sessions: repo, + GitSnapshots: repo, + RepoEntities: repo, + Executors: repo, + Environments: repo, + TaskEnvironments: repo, + Reviews: repo, + }, nil, log, service.RepositoryDiscoveryConfig{}) + + h := NewWorkspaceHandlers(svc, log) + rec := httptest.NewRecorder() + c, _ := gin.CreateTestContext(rec) + c.Request = httptest.NewRequest(http.MethodDelete, "/api/v1/workspaces/ws-1", strings.NewReader(`{`)) + c.Request.Header.Set("Content-Type", "application/json") + c.Params = gin.Params{{Key: "id", Value: "ws-1"}} + + h.httpDeleteWorkspace(c) + + require.Equal(t, http.StatusBadRequest, rec.Code) + require.JSONEq(t, `{"error":"invalid payload"}`, rec.Body.String()) + require.Equal(t, 0, repo.getCalls, "invalid payload must not look up the workspace") + require.False(t, repo.deleteCalled, "invalid payload must not delete") + require.False(t, repo.cascadeCalled, "invalid payload must not cascade") +} + +func TestHTTPDeleteWorkspaceReturnsNotFoundWhenWorkspaceMissing(t *testing.T) { + gin.SetMode(gin.TestMode) + repo := &workspaceDeleteRepo{getErr: errors.New("workspace not found")} + log, _ := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "json", OutputPath: "stdout"}) + svc := service.NewService(service.Repos{ + Workspaces: repo, + Tasks: repo, + TaskRepos: repo, + Workflows: repo, + Messages: repo, + Turns: repo, + Sessions: repo, + GitSnapshots: repo, + RepoEntities: repo, + Executors: repo, + Environments: repo, + TaskEnvironments: repo, + Reviews: repo, + }, nil, log, service.RepositoryDiscoveryConfig{}) + + h := NewWorkspaceHandlers(svc, log) + rec := httptest.NewRecorder() + c, _ := gin.CreateTestContext(rec) + c.Request = httptest.NewRequest( + http.MethodDelete, + "/api/v1/workspaces/ws-missing", + strings.NewReader(`{"confirm_name":"Delete Me"}`), + ) + c.Request.Header.Set("Content-Type", "application/json") + c.Params = gin.Params{{Key: "id", Value: "ws-missing"}} + + h.httpDeleteWorkspace(c) + + require.Equal(t, http.StatusNotFound, rec.Code) + require.JSONEq(t, `{"error":"workspace not found"}`, rec.Body.String()) + require.Equal(t, 1, repo.getCalls, "handler should let service resolve the workspace") + require.False(t, repo.deleteCalled, "missing workspace must not delete") + require.False(t, repo.cascadeCalled, "missing workspace must not cascade") +} + +func TestHTTPDeleteWorkspaceDeletesWhenConfirmNameMatches(t *testing.T) { + gin.SetMode(gin.TestMode) + repo := &workspaceDeleteRepo{} + log, _ := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "json", OutputPath: "stdout"}) + svc := service.NewService(service.Repos{ + Workspaces: repo, + Tasks: repo, + TaskRepos: repo, + Workflows: repo, + Messages: repo, + Turns: repo, + Sessions: repo, + GitSnapshots: repo, + RepoEntities: repo, + Executors: repo, + Environments: repo, + TaskEnvironments: repo, + Reviews: repo, + }, nil, log, service.RepositoryDiscoveryConfig{}) + + h := NewWorkspaceHandlers(svc, log) + rec := httptest.NewRecorder() + c, _ := gin.CreateTestContext(rec) + c.Request = httptest.NewRequest( + http.MethodDelete, + "/api/v1/workspaces/ws-1", + strings.NewReader(`{"confirm_name":"Delete Me"}`), + ) + c.Request.Header.Set("Content-Type", "application/json") + c.Params = gin.Params{{Key: "id", Value: "ws-1"}} + + h.httpDeleteWorkspace(c) + + require.Equal(t, http.StatusOK, rec.Code) + require.False(t, repo.deleteCalled, "handler must use the confirmed cascade path") + require.True(t, repo.cascadeCalled, "handler must cascade when confirm_name matches") + require.Equal(t, "ws-1", repo.cascadeID) + require.Equal(t, "Delete Me", repo.cascadeName) + require.Equal(t, 1, repo.getCalls, "confirmed delete should fetch the workspace once") +} + +func TestHTTPDeleteWorkspaceReturnsDeleteFailureWhenCascadeFails(t *testing.T) { + gin.SetMode(gin.TestMode) + repo := &workspaceDeleteRepo{cascadeErr: errors.New("cleanup failed")} + log, _ := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "json", OutputPath: "stdout"}) + svc := service.NewService(service.Repos{ + Workspaces: repo, + Tasks: repo, + TaskRepos: repo, + Workflows: repo, + Messages: repo, + Turns: repo, + Sessions: repo, + GitSnapshots: repo, + RepoEntities: repo, + Executors: repo, + Environments: repo, + TaskEnvironments: repo, + Reviews: repo, + }, nil, log, service.RepositoryDiscoveryConfig{}) + + h := NewWorkspaceHandlers(svc, log) + rec := httptest.NewRecorder() + c, _ := gin.CreateTestContext(rec) + c.Request = httptest.NewRequest( + http.MethodDelete, + "/api/v1/workspaces/ws-1", + strings.NewReader(`{"confirm_name":"Delete Me"}`), + ) + c.Request.Header.Set("Content-Type", "application/json") + c.Params = gin.Params{{Key: "id", Value: "ws-1"}} + + h.httpDeleteWorkspace(c) + + require.Equal(t, http.StatusInternalServerError, rec.Code) + require.JSONEq(t, `{"error":"workspace not deleted"}`, rec.Body.String()) + require.True(t, repo.cascadeCalled, "handler should surface cascade failures separately") + require.Equal(t, 1, repo.getCalls, "confirmed delete should fetch the workspace once") +} + +func TestWSDeleteWorkspaceRequiresConfirmName(t *testing.T) { + repo := &workspaceDeleteRepo{} + log, _ := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "json", OutputPath: "stdout"}) + svc := service.NewService(service.Repos{ + Workspaces: repo, + Tasks: repo, + TaskRepos: repo, + Workflows: repo, + Messages: repo, + Turns: repo, + Sessions: repo, + GitSnapshots: repo, + RepoEntities: repo, + Executors: repo, + Environments: repo, + TaskEnvironments: repo, + Reviews: repo, + }, nil, log, service.RepositoryDiscoveryConfig{}) + + h := NewWorkspaceHandlers(svc, log) + req, err := ws.NewRequest("req-1", ws.ActionWorkspaceDelete, map[string]string{"id": "ws-1"}) + require.NoError(t, err) + + resp, err := h.wsDeleteWorkspace(context.Background(), req) + + require.NoError(t, err) + require.Equal(t, ws.MessageTypeError, resp.Type) + require.Contains(t, string(resp.Payload), "confirm_name is required") + require.Equal(t, 0, repo.getCalls, "missing confirm_name must not look up the workspace") + require.False(t, repo.deleteCalled, "missing confirm_name must not delete") + require.False(t, repo.cascadeCalled, "missing confirm_name must not cascade") +} + +func TestWSDeleteWorkspaceReturnsNotFoundWhenWorkspaceMissing(t *testing.T) { + repo := &workspaceDeleteRepo{getErr: errors.New("workspace not found")} + log, _ := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "json", OutputPath: "stdout"}) + svc := service.NewService(service.Repos{ + Workspaces: repo, + Tasks: repo, + TaskRepos: repo, + Workflows: repo, + Messages: repo, + Turns: repo, + Sessions: repo, + GitSnapshots: repo, + RepoEntities: repo, + Executors: repo, + Environments: repo, + TaskEnvironments: repo, + Reviews: repo, + }, nil, log, service.RepositoryDiscoveryConfig{}) + + h := NewWorkspaceHandlers(svc, log) + req, err := ws.NewRequest("req-1", ws.ActionWorkspaceDelete, map[string]string{ + "id": "ws-missing", + "confirm_name": "Delete Me", + }) + require.NoError(t, err) + + resp, err := h.wsDeleteWorkspace(context.Background(), req) + + require.NoError(t, err) + require.Equal(t, ws.MessageTypeError, resp.Type) + require.Contains(t, string(resp.Payload), string(ws.ErrorCodeNotFound)) + require.Contains(t, string(resp.Payload), "Workspace not found") + require.Equal(t, 1, repo.getCalls, "handler should let service resolve the workspace") + require.False(t, repo.cascadeCalled, "missing workspace must not cascade") +} + +func TestWSDeleteWorkspaceDeletesWhenConfirmNameMatches(t *testing.T) { + repo := &workspaceDeleteRepo{} + log, _ := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "json", OutputPath: "stdout"}) + svc := service.NewService(service.Repos{ + Workspaces: repo, + Tasks: repo, + TaskRepos: repo, + Workflows: repo, + Messages: repo, + Turns: repo, + Sessions: repo, + GitSnapshots: repo, + RepoEntities: repo, + Executors: repo, + Environments: repo, + TaskEnvironments: repo, + Reviews: repo, + }, nil, log, service.RepositoryDiscoveryConfig{}) + + h := NewWorkspaceHandlers(svc, log) + req, err := ws.NewRequest("req-1", ws.ActionWorkspaceDelete, map[string]string{ + "id": "ws-1", + "confirm_name": "Delete Me", + }) + require.NoError(t, err) + + resp, err := h.wsDeleteWorkspace(context.Background(), req) + + require.NoError(t, err) + require.Equal(t, ws.MessageTypeResponse, resp.Type) + require.False(t, repo.deleteCalled, "handler must use the confirmed cascade path") + require.True(t, repo.cascadeCalled, "handler must cascade when confirm_name matches") + require.Equal(t, "ws-1", repo.cascadeID) + require.Equal(t, "Delete Me", repo.cascadeName) + require.Equal(t, 1, repo.getCalls, "confirmed delete should fetch the workspace once") +} diff --git a/apps/backend/internal/task/repository/interface.go b/apps/backend/internal/task/repository/interface.go index 74ed7635c3..e8d3736f8d 100644 --- a/apps/backend/internal/task/repository/interface.go +++ b/apps/backend/internal/task/repository/interface.go @@ -6,15 +6,21 @@ import ( agentdto "github.com/kandev/kandev/internal/agent/dto" "github.com/kandev/kandev/internal/task/models" + "github.com/kandev/kandev/internal/task/repository/repoerrors" v1 "github.com/kandev/kandev/pkg/api/v1" ) +var ErrWorkspaceNameMismatch = repoerrors.ErrWorkspaceNameMismatch +var ErrWorkspaceNotFound = repoerrors.ErrWorkspaceNotFound + // WorkspaceRepository handles workspace CRUD. type WorkspaceRepository interface { CreateWorkspace(ctx context.Context, workspace *models.Workspace) error GetWorkspace(ctx context.Context, id string) (*models.Workspace, error) UpdateWorkspace(ctx context.Context, workspace *models.Workspace) error DeleteWorkspace(ctx context.Context, id string) error + DeleteWorkspaceCascade(ctx context.Context, id string) ([]*models.Task, []*models.Workflow, error) + DeleteWorkspaceCascadeWithName(ctx context.Context, id, name string) ([]*models.Task, []*models.Workflow, error) ListWorkspaces(ctx context.Context) ([]*models.Workspace, error) } diff --git a/apps/backend/internal/task/repository/repoerrors/errors.go b/apps/backend/internal/task/repository/repoerrors/errors.go new file mode 100644 index 0000000000..97c30abc21 --- /dev/null +++ b/apps/backend/internal/task/repository/repoerrors/errors.go @@ -0,0 +1,10 @@ +package repoerrors + +import "errors" + +// ErrWorkspaceNameMismatch reports that a confirmed workspace delete did not +// match the workspace row's current name. +var ErrWorkspaceNameMismatch = errors.New("workspace name mismatch") + +// ErrWorkspaceNotFound reports that no workspace row matched the supplied id. +var ErrWorkspaceNotFound = errors.New("workspace not found") diff --git a/apps/backend/internal/task/repository/sqlite/errors.go b/apps/backend/internal/task/repository/sqlite/errors.go index da4ea5f2fd..5dba6211c8 100644 --- a/apps/backend/internal/task/repository/sqlite/errors.go +++ b/apps/backend/internal/task/repository/sqlite/errors.go @@ -1,6 +1,10 @@ package sqlite -import "errors" +import ( + "errors" + + "github.com/kandev/kandev/internal/task/repository/repoerrors" +) // ErrTaskNotFound is returned by Repository methods (GetTask, UpdateTask, // DeleteTask, UpdateTaskState, …) when no row matches the supplied id. @@ -8,6 +12,10 @@ import "errors" // formatted message, which includes the task id and is therefore brittle. var ErrTaskNotFound = errors.New("task not found") +// ErrWorkspaceNotFound is returned by Repository workspace methods when no row +// matches the supplied id. +var ErrWorkspaceNotFound = repoerrors.ErrWorkspaceNotFound + // ErrNoPrimarySession is returned by GetPrimarySessionByTaskID when the task // has no primary session row. Callers should use errors.Is to distinguish this // "not found" case from genuine backend/DB errors. diff --git a/apps/backend/internal/task/repository/sqlite/workflow.go b/apps/backend/internal/task/repository/sqlite/workflow.go index 5fa9ba526f..2cc279b4ec 100644 --- a/apps/backend/internal/task/repository/sqlite/workflow.go +++ b/apps/backend/internal/task/repository/sqlite/workflow.go @@ -69,23 +69,33 @@ func normalizeWorkflowStyle(style string) string { return models.WorkflowStyleKanban } -// GetWorkflow retrieves a workflow by ID -func (r *Repository) GetWorkflow(ctx context.Context, id string) (*models.Workflow, error) { +const workflowSelectColumns = ` + id, workspace_id, name, description, agent_profile_id, + workflow_template_id, sort_order, hidden, style, created_at, updated_at +` + +type workflowScanner interface { + Scan(dest ...interface{}) error +} + +func scanWorkflowRow(scanner workflowScanner) (*models.Workflow, error) { workflow := &models.Workflow{} var workflowTemplateID, agentProfileID, style sql.NullString - - err := r.ro.QueryRowContext(ctx, r.ro.Rebind(` - SELECT id, workspace_id, name, description, agent_profile_id, workflow_template_id, sort_order, hidden, style, created_at, updated_at - FROM workflows WHERE id = ? - `), id).Scan(&workflow.ID, &workflow.WorkspaceID, &workflow.Name, &workflow.Description, &agentProfileID, &workflowTemplateID, &workflow.SortOrder, &workflow.Hidden, &style, &workflow.CreatedAt, &workflow.UpdatedAt) - - if err == sql.ErrNoRows { - return nil, fmt.Errorf("workflow not found: %s", id) - } - if err != nil { + if err := scanner.Scan( + &workflow.ID, + &workflow.WorkspaceID, + &workflow.Name, + &workflow.Description, + &agentProfileID, + &workflowTemplateID, + &workflow.SortOrder, + &workflow.Hidden, + &style, + &workflow.CreatedAt, + &workflow.UpdatedAt, + ); err != nil { return nil, err } - if agentProfileID.Valid { workflow.AgentProfileID = agentProfileID.String } @@ -100,6 +110,33 @@ func (r *Repository) GetWorkflow(ctx context.Context, id string) (*models.Workfl return workflow, nil } +func scanWorkflowRows(rows *sql.Rows) ([]*models.Workflow, error) { + var result []*models.Workflow + for rows.Next() { + workflow, err := scanWorkflowRow(rows) + if err != nil { + return nil, err + } + result = append(result, workflow) + } + return result, rows.Err() +} + +// GetWorkflow retrieves a workflow by ID +func (r *Repository) GetWorkflow(ctx context.Context, id string) (*models.Workflow, error) { + workflow, err := scanWorkflowRow(r.ro.QueryRowContext(ctx, r.ro.Rebind(fmt.Sprintf(` + SELECT %s + FROM workflows WHERE id = ? + `, workflowSelectColumns)), id)) + if err == sql.ErrNoRows { + return nil, fmt.Errorf("workflow not found: %s", id) + } + if err != nil { + return nil, err + } + return workflow, nil +} + // UpdateWorkflow updates an existing workflow func (r *Repository) UpdateWorkflow(ctx context.Context, workflow *models.Workflow) error { workflow.UpdatedAt = time.Now().UTC() @@ -159,9 +196,10 @@ func (r *Repository) DeleteWorkflow(ctx context.Context, id string) error { // ListWorkflows returns workflows for the given workspace, excluding hidden by default. // Pass includeHidden=true to also return system-only workflows like Improve Kandev. func (r *Repository) ListWorkflows(ctx context.Context, workspaceID string, includeHidden bool) ([]*models.Workflow, error) { - query := ` - SELECT id, workspace_id, name, description, agent_profile_id, workflow_template_id, sort_order, hidden, style, created_at, updated_at FROM workflows - ` + query := fmt.Sprintf(` + SELECT %s + FROM workflows + `, workflowSelectColumns) var args []interface{} var conditions []string if workspaceID != "" { @@ -186,28 +224,7 @@ func (r *Repository) ListWorkflows(ctx context.Context, workspaceID string, incl } defer func() { _ = rows.Close() }() - var result []*models.Workflow - for rows.Next() { - workflow := &models.Workflow{} - var agentProfileID, workflowTemplateID, style sql.NullString - err := rows.Scan(&workflow.ID, &workflow.WorkspaceID, &workflow.Name, &workflow.Description, &agentProfileID, &workflowTemplateID, &workflow.SortOrder, &workflow.Hidden, &style, &workflow.CreatedAt, &workflow.UpdatedAt) - if err != nil { - return nil, err - } - if agentProfileID.Valid { - workflow.AgentProfileID = agentProfileID.String - } - if workflowTemplateID.Valid { - workflow.WorkflowTemplateID = &workflowTemplateID.String - } - if style.Valid && style.String != "" { - workflow.Style = style.String - } else { - workflow.Style = models.WorkflowStyleKanban - } - result = append(result, workflow) - } - return result, rows.Err() + return scanWorkflowRows(rows) } // ReorderWorkflows updates sort_order for workflows within a workspace using a transaction. diff --git a/apps/backend/internal/task/repository/sqlite/workspace.go b/apps/backend/internal/task/repository/sqlite/workspace.go index 84c57b9716..5ab32b32f9 100644 --- a/apps/backend/internal/task/repository/sqlite/workspace.go +++ b/apps/backend/internal/task/repository/sqlite/workspace.go @@ -3,12 +3,15 @@ package sqlite import ( "context" "database/sql" + "errors" "fmt" "time" "github.com/google/uuid" + "github.com/jmoiron/sqlx" "github.com/kandev/kandev/internal/task/models" + "github.com/kandev/kandev/internal/task/repository/repoerrors" ) // CreateWorkspace creates a new workspace @@ -85,7 +88,7 @@ func (r *Repository) GetWorkspace(ctx context.Context, id string) (*models.Works } if err == sql.ErrNoRows { - return nil, fmt.Errorf("workspace not found: %s", id) + return nil, workspaceNotFoundError(id) } return workspace, err } @@ -111,7 +114,7 @@ func (r *Repository) UpdateWorkspace(ctx context.Context, workspace *models.Work rows, _ := result.RowsAffected() if rows == 0 { - return fmt.Errorf("workspace not found: %s", workspace.ID) + return workspaceNotFoundError(workspace.ID) } return nil } @@ -125,11 +128,182 @@ func (r *Repository) DeleteWorkspace(ctx context.Context, id string) error { rows, _ := result.RowsAffected() if rows == 0 { - return fmt.Errorf("workspace not found: %s", id) + return workspaceNotFoundError(id) } return nil } +// DeleteWorkspaceCascade deletes a workspace and its task/workflow rows in one transaction. +func (r *Repository) DeleteWorkspaceCascade( + ctx context.Context, + id string, +) ([]*models.Task, []*models.Workflow, error) { + return r.deleteWorkspaceCascade(ctx, id, nil) +} + +// DeleteWorkspaceCascadeWithName deletes a workspace and its task/workflow rows +// in one transaction, only if the current workspace name matches name. +func (r *Repository) DeleteWorkspaceCascadeWithName( + ctx context.Context, + id, name string, +) ([]*models.Task, []*models.Workflow, error) { + return r.deleteWorkspaceCascade(ctx, id, &name) +} + +func (r *Repository) deleteWorkspaceCascade( + ctx context.Context, + id string, + expectedName *string, +) ([]*models.Task, []*models.Workflow, error) { + tx, err := r.db.BeginTxx(ctx, nil) + if err != nil { + return nil, nil, err + } + defer func() { _ = tx.Rollback() }() + + if expectedName != nil { + if err := r.confirmWorkspaceNameForCascadeDelete(ctx, tx, id, *expectedName); err != nil { + return nil, nil, err + } + } + tasks, err := r.listWorkspaceCascadeDeleteTasks(ctx, tx, id) + if err != nil { + return nil, nil, err + } + workflows, err := r.listWorkspaceCascadeDeleteWorkflows(ctx, tx, id) + if err != nil { + return nil, nil, err + } + + rows, err := r.deleteWorkspaceCascadeRow(ctx, tx, id, expectedName) + if err != nil { + return nil, nil, err + } + if rows == 0 { + if expectedName != nil { + return nil, nil, r.confirmedWorkspaceDeleteMismatch(ctx, tx, id) + } + return nil, nil, workspaceNotFoundError(id) + } + + if _, err := tx.ExecContext(ctx, r.db.Rebind(` + DELETE FROM tasks + WHERE workspace_id = ? + `), id); err != nil { + return nil, nil, err + } + if _, err := tx.ExecContext(ctx, r.db.Rebind(` + DELETE FROM workflows + WHERE workspace_id = ? + `), id); err != nil { + return nil, nil, err + } + if err := tx.Commit(); err != nil { + return nil, nil, err + } + return tasks, workflows, nil +} + +func (r *Repository) deleteWorkspaceCascadeRow( + ctx context.Context, + tx *sqlx.Tx, + id string, + expectedName *string, +) (int64, error) { + var result sql.Result + var err error + if expectedName == nil { + result, err = tx.ExecContext(ctx, r.db.Rebind(` + DELETE FROM workspaces + WHERE id = ? + `), id) + } else { + // Keep the name predicate on the delete itself so a rename racing after + // the confirmation cannot delete the newly renamed workspace. + result, err = tx.ExecContext(ctx, r.db.Rebind(` + DELETE FROM workspaces + WHERE id = ? AND name = ? + `), id, *expectedName) + } + if err != nil { + return 0, err + } + rows, _ := result.RowsAffected() + return rows, nil +} + +func (r *Repository) confirmWorkspaceNameForCascadeDelete(ctx context.Context, tx *sqlx.Tx, id, name string) error { + currentName, err := r.workspaceNameForCascadeDelete(ctx, tx, id) + if err != nil { + return err + } + if currentName != name { + return repoerrors.ErrWorkspaceNameMismatch + } + return nil +} + +func (r *Repository) workspaceNameForCascadeDelete(ctx context.Context, tx *sqlx.Tx, id string) (string, error) { + var currentName string + err := tx.QueryRowContext(ctx, r.db.Rebind(` + SELECT name + FROM workspaces + WHERE id = ? + `), id).Scan(¤tName) + if errors.Is(err, sql.ErrNoRows) { + return "", workspaceNotFoundError(id) + } + return currentName, err +} + +func workspaceNotFoundError(id string) error { + return fmt.Errorf("%w: %s", repoerrors.ErrWorkspaceNotFound, id) +} + +func (r *Repository) confirmedWorkspaceDeleteMismatch(ctx context.Context, tx *sqlx.Tx, id string) error { + if _, err := r.workspaceNameForCascadeDelete(ctx, tx, id); err != nil { + return err + } + return repoerrors.ErrWorkspaceNameMismatch +} + +func (r *Repository) listWorkspaceCascadeDeleteTasks( + ctx context.Context, + tx *sqlx.Tx, + workspaceID string, +) ([]*models.Task, error) { + rows, err := tx.QueryContext(ctx, r.db.Rebind(fmt.Sprintf(` + SELECT %s + FROM tasks + WHERE workspace_id = ? + ORDER BY created_at ASC, id ASC + `, taskSelectColumns("tasks"))), workspaceID) + if err != nil { + return nil, err + } + defer func() { _ = rows.Close() }() + return r.scanTasks(rows) +} + +func (r *Repository) listWorkspaceCascadeDeleteWorkflows( + ctx context.Context, + tx *sqlx.Tx, + workspaceID string, +) ([]*models.Workflow, error) { + rows, err := tx.QueryContext(ctx, r.db.Rebind(` + SELECT `+workflowSelectColumns+` + FROM workflows + WHERE workspace_id = ? + ORDER BY sort_order ASC, created_at ASC + `), workspaceID) + if err != nil { + return nil, err + } + defer func() { _ = rows.Close() }() + + return scanWorkflowRows(rows) +} + // ListWorkspaces returns all workspaces func (r *Repository) ListWorkspaces(ctx context.Context) ([]*models.Workspace, error) { rows, err := r.ro.QueryContext(ctx, ` diff --git a/apps/backend/internal/task/repository/sqlite/workspace_test.go b/apps/backend/internal/task/repository/sqlite/workspace_test.go new file mode 100644 index 0000000000..f8970cd105 --- /dev/null +++ b/apps/backend/internal/task/repository/sqlite/workspace_test.go @@ -0,0 +1,278 @@ +package sqlite + +import ( + "context" + "errors" + "testing" + + "github.com/kandev/kandev/internal/task/models" + "github.com/kandev/kandev/internal/task/repository/repoerrors" +) + +func TestDeleteWorkspaceCascadeWithNameDeletesWorkspaceChildren(t *testing.T) { + ctx := context.Background() + repo := newRepoForHealTests(t) + + seedWorkspaceCascadeRows(t, repo, "ws-delete") + + tasks, workflows, err := repo.DeleteWorkspaceCascadeWithName(ctx, "ws-delete", "Delete Me") + if err != nil { + t.Fatalf("DeleteWorkspaceCascadeWithName: %v", err) + } + if len(tasks) != 1 || tasks[0].ID != "task-delete" { + t.Fatalf("deleted tasks = %#v, want task-delete", tasks) + } + if len(workflows) != 1 || workflows[0].ID != "wf-delete" { + t.Fatalf("deleted workflows = %#v, want wf-delete", workflows) + } + if _, err := repo.GetWorkspace(ctx, "ws-delete"); err == nil { + t.Fatalf("workspace should be deleted") + } + if _, err := repo.GetTask(ctx, "task-delete"); err == nil { + t.Fatalf("workspace task should be deleted") + } + workflows, err = repo.ListWorkflows(ctx, "ws-delete", true) + if err != nil { + t.Fatalf("ListWorkflows: %v", err) + } + if len(workflows) != 0 { + t.Fatalf("workspace workflows should be deleted, got %d", len(workflows)) + } + assertNoWorkspaceCascadeDependents(t, repo) +} + +func TestDeleteWorkspaceCascadeDeletesWorkspaceChildren(t *testing.T) { + ctx := context.Background() + repo := newRepoForHealTests(t) + + seedWorkspaceCascadeRows(t, repo, "ws-delete") + + tasks, workflows, err := repo.DeleteWorkspaceCascade(ctx, "ws-delete") + if err != nil { + t.Fatalf("DeleteWorkspaceCascade: %v", err) + } + if len(tasks) != 1 || tasks[0].ID != "task-delete" { + t.Fatalf("deleted tasks = %#v, want task-delete", tasks) + } + if len(workflows) != 1 || workflows[0].ID != "wf-delete" { + t.Fatalf("deleted workflows = %#v, want wf-delete", workflows) + } + if _, err := repo.GetWorkspace(ctx, "ws-delete"); err == nil { + t.Fatalf("workspace should be deleted") + } + if _, err := repo.GetTask(ctx, "task-delete"); err == nil { + t.Fatalf("workspace task should be deleted") + } + workflows, err = repo.ListWorkflows(ctx, "ws-delete", true) + if err != nil { + t.Fatalf("ListWorkflows: %v", err) + } + if len(workflows) != 0 { + t.Fatalf("workspace workflows should be deleted, got %d", len(workflows)) + } + assertNoWorkspaceCascadeDependents(t, repo) +} + +func TestDeleteWorkspaceCascadeWithNameRejectsMismatchedName(t *testing.T) { + ctx := context.Background() + repo := newRepoForHealTests(t) + + seedWorkspaceCascadeRows(t, repo, "ws-delete") + + _, _, err := repo.DeleteWorkspaceCascadeWithName(ctx, "ws-delete", "Wrong") + if !errors.Is(err, repoerrors.ErrWorkspaceNameMismatch) { + t.Fatalf("expected ErrWorkspaceNameMismatch, got %v", err) + } + if _, err := repo.GetWorkspace(ctx, "ws-delete"); err != nil { + t.Fatalf("workspace should remain: %v", err) + } + if _, err := repo.GetTask(ctx, "task-delete"); err != nil { + t.Fatalf("workspace task should remain: %v", err) + } + if _, err := repo.GetWorkflow(ctx, "wf-delete"); err != nil { + t.Fatalf("workspace workflow should remain: %v", err) + } +} + +func TestDeleteWorkspaceCascadeWithNameRollsBackWhenChildDeleteFails(t *testing.T) { + ctx := context.Background() + repo := newRepoForHealTests(t) + + seedWorkspaceCascadeRows(t, repo, "ws-delete") + if _, err := repo.db.Exec(` + CREATE TRIGGER fail_workspace_task_delete + BEFORE DELETE ON tasks + WHEN OLD.workspace_id = 'ws-delete' + BEGIN + SELECT RAISE(ABORT, 'task delete blocked'); + END + `); err != nil { + t.Fatalf("create trigger: %v", err) + } + + if _, _, err := repo.DeleteWorkspaceCascadeWithName(ctx, "ws-delete", "Delete Me"); err == nil { + t.Fatalf("expected child delete failure") + } + if _, err := repo.GetWorkspace(ctx, "ws-delete"); err != nil { + t.Fatalf("workspace should roll back: %v", err) + } + if _, err := repo.GetTask(ctx, "task-delete"); err != nil { + t.Fatalf("workspace task should roll back: %v", err) + } + if _, err := repo.GetWorkflow(ctx, "wf-delete"); err != nil { + t.Fatalf("workspace workflow should roll back: %v", err) + } +} + +func seedWorkspaceCascadeRows(t *testing.T, repo *Repository, workspaceID string) { + t.Helper() + ctx := context.Background() + if err := repo.CreateWorkspace(ctx, &models.Workspace{ID: workspaceID, Name: "Delete Me"}); err != nil { + t.Fatalf("CreateWorkspace: %v", err) + } + if err := repo.CreateWorkflow(ctx, &models.Workflow{ + ID: "wf-delete", + WorkspaceID: workspaceID, + Name: "Doomed", + }); err != nil { + t.Fatalf("CreateWorkflow: %v", err) + } + if err := repo.CreateTask(ctx, &models.Task{ + ID: "task-delete", + WorkspaceID: workspaceID, + WorkflowID: "wf-delete", + WorkflowStepID: "step-delete", + Title: "Delete task", + }); err != nil { + t.Fatalf("CreateTask: %v", err) + } + if err := repo.CreateRepository(ctx, &models.Repository{ + ID: "repo-delete", + WorkspaceID: workspaceID, + Name: "Repo", + SourceType: "local", + LocalPath: "/tmp/repo-delete", + }); err != nil { + t.Fatalf("CreateRepository: %v", err) + } + if err := repo.CreateRepositoryScript(ctx, &models.RepositoryScript{ + ID: "script-delete", + RepositoryID: "repo-delete", + Name: "test", + Command: "true", + }); err != nil { + t.Fatalf("CreateRepositoryScript: %v", err) + } + if err := repo.CreateTaskRepository(ctx, &models.TaskRepository{ + ID: "task-repo-delete", + TaskID: "task-delete", + RepositoryID: "repo-delete", + BaseBranch: "main", + }); err != nil { + t.Fatalf("CreateTaskRepository: %v", err) + } + if err := repo.CreateTaskEnvironment(ctx, &models.TaskEnvironment{ + ID: "env-delete", + TaskID: "task-delete", + RepositoryID: "repo-delete", + ExecutorType: string(models.ExecutorTypeWorktree), + Status: models.TaskEnvironmentStatusReady, + WorktreeID: "wt-delete", + WorktreePath: "/tmp/wt-delete", + WorkspacePath: "/tmp/wt-delete", + Repos: []*models.TaskEnvironmentRepo{{ + ID: "env-repo-delete", + RepositoryID: "repo-delete", + WorktreeID: "wt-delete", + WorktreePath: "/tmp/wt-delete", + WorktreeBranch: "branch-delete", + }}, + }); err != nil { + t.Fatalf("CreateTaskEnvironment: %v", err) + } + if err := repo.CreateTaskSession(ctx, &models.TaskSession{ + ID: "session-delete", + TaskID: "task-delete", + AgentExecutionID: "exec-delete", + TaskEnvironmentID: "env-delete", + State: models.TaskSessionStateCreated, + }); err != nil { + t.Fatalf("CreateTaskSession: %v", err) + } + if err := repo.CreateTaskSessionWorktree(ctx, &models.TaskSessionWorktree{ + ID: "session-wt-delete", + SessionID: "session-delete", + WorktreeID: "wt-delete", + RepositoryID: "repo-delete", + WorktreePath: "/tmp/wt-delete", + WorktreeBranch: "branch-delete", + }); err != nil { + t.Fatalf("CreateTaskSessionWorktree: %v", err) + } + if err := repo.CreateTurn(ctx, &models.Turn{ + ID: "turn-delete", + TaskSessionID: "session-delete", + TaskID: "task-delete", + }); err != nil { + t.Fatalf("CreateTurn: %v", err) + } + if err := repo.CreateMessage(ctx, &models.Message{ + ID: "message-delete", + TaskSessionID: "session-delete", + TaskID: "task-delete", + TurnID: "turn-delete", + AuthorType: models.MessageAuthorUser, + Content: "hello", + }); err != nil { + t.Fatalf("CreateMessage: %v", err) + } + if err := repo.CreateTaskPlan(ctx, &models.TaskPlan{ + ID: "plan-delete", + TaskID: "task-delete", + Title: "Plan", + Content: "one", + CreatedBy: "agent", + }); err != nil { + t.Fatalf("CreateTaskPlan: %v", err) + } + if err := repo.InsertTaskPlanRevision(ctx, &models.TaskPlanRevision{ + ID: "plan-revision-delete", + TaskID: "task-delete", + RevisionNumber: 1, + Title: "Plan", + Content: "one", + AuthorKind: "agent", + }); err != nil { + t.Fatalf("InsertTaskPlanRevision: %v", err) + } +} + +func assertNoWorkspaceCascadeDependents(t *testing.T, repo *Repository) { + t.Helper() + for _, table := range []string{ + "repositories", + "repository_scripts", + "task_repositories", + "task_sessions", + "task_environments", + "task_environment_repos", + "task_session_worktrees", + "task_session_turns", + "task_session_messages", + "task_plans", + "task_plan_revisions", + } { + assertTableRowCount(t, repo, table, 0) + } +} + +func assertTableRowCount(t *testing.T, repo *Repository, table string, want int) { + t.Helper() + var got int + if err := repo.ro.QueryRow("SELECT COUNT(*) FROM " + table).Scan(&got); err != nil { + t.Fatalf("count %s: %v", table, err) + } + if got != want { + t.Fatalf("%s rows = %d, want %d", table, got, want) + } +} diff --git a/apps/backend/internal/task/service/service_resources.go b/apps/backend/internal/task/service/service_resources.go index f7614948d3..06f527edcc 100644 --- a/apps/backend/internal/task/service/service_resources.go +++ b/apps/backend/internal/task/service/service_resources.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "strings" + "sync" "time" "github.com/google/uuid" @@ -12,10 +13,25 @@ import ( "github.com/kandev/kandev/internal/events" "github.com/kandev/kandev/internal/task/models" + taskrepo "github.com/kandev/kandev/internal/task/repository" "github.com/kandev/kandev/internal/worktree" ) -const spritesTokenEnvKey = "SPRITES_API_TOKEN" +const ( + spritesTokenEnvKey = "SPRITES_API_TOKEN" + workspaceDeletePageSize = 500 + workspaceDeleteCleanupConcurrency = 8 +) + +var ErrWorkspaceConfirmNameMismatch = errors.New("confirm_name does not match workspace name") + +type workspaceDeleteTaskCleanup struct { + task *models.Task + sessions []*models.TaskSession + worktrees []*worktree.Worktree + stopTargets []taskStopTarget + taskEnv *models.TaskEnvironment +} // Workspace operations @@ -90,15 +106,231 @@ func (s *Service) DeleteWorkspace(ctx context.Context, id string) error { if err != nil { return err } - if err := s.workspaces.DeleteWorkspace(ctx, id); err != nil { - s.logger.Error("failed to delete workspace", zap.String("workspace_id", id), zap.Error(err)) + return s.deleteWorkspace(ctx, workspace, nil) +} + +// DeleteWorkspaceWithConfirmName deletes a workspace only when confirmName +// matches the workspace name read for the cascade and final row delete. +func (s *Service) DeleteWorkspaceWithConfirmName(ctx context.Context, id, confirmName string) error { + workspace, err := s.workspaces.GetWorkspace(ctx, id) + if err != nil { return err } + if confirmName != workspace.Name { + return ErrWorkspaceConfirmNameMismatch + } + return s.deleteWorkspace(ctx, workspace, &confirmName) +} + +func (s *Service) deleteWorkspace(ctx context.Context, workspace *models.Workspace, confirmedName *string) error { + tasks, err := s.listAllTasksForWorkspaceDelete(ctx, workspace.ID) + if err != nil { + return err + } + // Runtime cleanup needs task rows before the cascade removes them. + cleanups, err := s.prepareWorkspaceDeleteTaskCleanups(ctx, tasks) + if err != nil { + return err + } + + var deletedTasks []*models.Task + var deletedWorkflows []*models.Workflow + if confirmedName == nil { + deletedTasks, deletedWorkflows, err = s.workspaces.DeleteWorkspaceCascade(ctx, workspace.ID) + } else { + deletedTasks, deletedWorkflows, err = s.workspaces.DeleteWorkspaceCascadeWithName(ctx, workspace.ID, *confirmedName) + } + if err != nil { + return s.mapWorkspaceDeleteError(workspace.ID, err) + } + cleanups = s.appendWorkspaceDeleteMissingTaskCleanups(ctx, cleanups, deletedTasks) + s.publishWorkspaceDeleteChildEvents(ctx, deletedTasks, deletedWorkflows) + s.runWorkspaceDeleteTaskCleanups(cleanups, deletedTasks) s.publishWorkspaceEvent(ctx, events.WorkspaceDeleted, workspace) - s.logger.Info("workspace deleted", zap.String("workspace_id", id)) + s.logger.Info("workspace deleted", zap.String("workspace_id", workspace.ID)) return nil } +func (s *Service) prepareWorkspaceDeleteTaskCleanups(ctx context.Context, tasks []*models.Task) ([]workspaceDeleteTaskCleanup, error) { + cleanups := make([]workspaceDeleteTaskCleanup, 0, len(tasks)) + for _, task := range tasks { + if task == nil || task.ID == "" { + continue + } + cleanup, err := s.prepareWorkspaceDeleteTaskCleanup(ctx, task) + if err != nil { + return nil, err + } + cleanups = append(cleanups, cleanup) + } + return cleanups, nil +} + +func (s *Service) appendWorkspaceDeleteMissingTaskCleanups( + ctx context.Context, + cleanups []workspaceDeleteTaskCleanup, + deletedTasks []*models.Task, +) []workspaceDeleteTaskCleanup { + prepared := make(map[string]struct{}, len(cleanups)) + for _, cleanup := range cleanups { + if cleanup.task != nil && cleanup.task.ID != "" { + prepared[cleanup.task.ID] = struct{}{} + } + } + for _, task := range deletedTasks { + if task == nil || task.ID == "" { + continue + } + if _, ok := prepared[task.ID]; ok { + continue + } + cleanup, err := s.prepareWorkspaceDeleteTaskCleanup(ctx, task) + if err != nil { + s.logger.Error("failed to prepare late workspace task cleanup", + zap.String("task_id", task.ID), + zap.Error(err)) + continue + } + cleanups = append(cleanups, cleanup) + prepared[task.ID] = struct{}{} + } + return cleanups +} + +func (s *Service) prepareWorkspaceDeleteTaskCleanup(ctx context.Context, task *models.Task) (workspaceDeleteTaskCleanup, error) { + cleanup := workspaceDeleteTaskCleanup{ + task: task, + worktrees: s.gatherWorktreesForDelete(ctx, task.ID), + taskEnv: s.gatherTaskEnvironmentForCleanup(ctx, task.ID), + } + var err error + cleanup.sessions, err = s.sessions.ListTaskSessions(ctx, task.ID) + if err != nil { + return workspaceDeleteTaskCleanup{}, fmt.Errorf("list task sessions for workspace delete task %q: %w", task.ID, err) + } + if s.executionStopper == nil { + return cleanup, nil + } + activeSessions, err := s.sessions.ListActiveTaskSessionsByTaskID(ctx, task.ID) + if err != nil { + s.logger.Warn("failed to list active task sessions for workspace delete", + zap.String("task_id", task.ID), + zap.Error(err)) + } + cleanup.stopTargets, err = s.buildStopTargets(ctx, task.ID, activeSessions) + if err != nil { + return workspaceDeleteTaskCleanup{}, fmt.Errorf("list runtime cleanup inventory: %w", err) + } + return cleanup, nil +} + +func (s *Service) publishWorkspaceDeleteChildEvents(ctx context.Context, tasks []*models.Task, workflows []*models.Workflow) { + for _, task := range tasks { + if task == nil || task.ID == "" { + continue + } + s.publishTaskEvent(ctx, events.TaskDeleted, task, nil) + } + for _, workflow := range workflows { + if workflow == nil || workflow.ID == "" { + continue + } + s.publishWorkflowEvent(ctx, events.WorkflowDeleted, workflow) + } +} + +func (s *Service) runWorkspaceDeleteTaskCleanups(cleanups []workspaceDeleteTaskCleanup, deletedTasks []*models.Task) { + jobs := s.workspaceDeleteTaskCleanupJobs(cleanups, deletedTasks) + if len(jobs) == 0 { + return + } + go s.runWorkspaceDeleteTaskCleanupJobs(jobs) +} + +func (s *Service) workspaceDeleteTaskCleanupJobs( + cleanups []workspaceDeleteTaskCleanup, + deletedTasks []*models.Task, +) []workspaceDeleteTaskCleanup { + deletedTaskIDs := make(map[string]struct{}, len(deletedTasks)) + for _, task := range deletedTasks { + if task != nil && task.ID != "" { + deletedTaskIDs[task.ID] = struct{}{} + } + } + jobs := make([]workspaceDeleteTaskCleanup, 0, len(cleanups)) + for _, cleanup := range cleanups { + if cleanup.task == nil { + continue + } + if _, ok := deletedTaskIDs[cleanup.task.ID]; !ok { + continue + } + hasCleanup := len(cleanup.stopTargets) > 0 || s.worktreeCleanup != nil || + len(cleanup.sessions) > 0 || cleanup.task.IsEphemeral || cleanup.taskEnv != nil + if !hasCleanup { + continue + } + jobs = append(jobs, cleanup) + } + return jobs +} + +func (s *Service) runWorkspaceDeleteTaskCleanupJobs(jobs []workspaceDeleteTaskCleanup) { + workers := workspaceDeleteCleanupConcurrency + if len(jobs) < workers { + workers = len(jobs) + } + jobCh := make(chan workspaceDeleteTaskCleanup) + var wg sync.WaitGroup + wg.Add(workers) + for i := 0; i < workers; i++ { + go func() { + defer wg.Done() + for cleanup := range jobCh { + s.runWorkspaceDeleteTaskCleanup(cleanup) + } + }() + } + for _, job := range jobs { + jobCh <- job + } + close(jobCh) + wg.Wait() +} + +func (s *Service) runWorkspaceDeleteTaskCleanup(cleanup workspaceDeleteTaskCleanup) { + envCleanup := taskEnvironmentCleanup{env: cleanup.taskEnv, deleteRow: false} + s.runTaskCleanup(cleanup.task.ID, cleanup.sessions, cleanup.worktrees, cleanup.stopTargets, envCleanup, + "task deleted", "failed to stop session on task delete", "task cleanup completed") +} + +func (s *Service) mapWorkspaceDeleteError(id string, err error) error { + if err == nil { + return nil + } + if errors.Is(err, taskrepo.ErrWorkspaceNameMismatch) { + return ErrWorkspaceConfirmNameMismatch + } + s.logger.Error("failed to delete workspace", zap.String("workspace_id", id), zap.Error(err)) + return err +} + +func (s *Service) listAllTasksForWorkspaceDelete(ctx context.Context, workspaceID string) ([]*models.Task, error) { + var all []*models.Task + for page := 1; ; page++ { + tasks, total, err := s.tasks.ListTasksByWorkspace( + ctx, workspaceID, "", "", "", page, workspaceDeletePageSize, true, true, false, false, + ) + if err != nil { + return nil, fmt.Errorf("list workspace tasks: %w", err) + } + all = append(all, tasks...) + if len(all) >= total || len(tasks) == 0 { + return all, nil + } + } +} + func normalizeOptionalID(value *string) *string { if value == nil { 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 c77677b3e5..2d85d08e05 100644 --- a/apps/backend/internal/task/service/service_resources_test.go +++ b/apps/backend/internal/task/service/service_resources_test.go @@ -3,10 +3,16 @@ package service import ( "context" "errors" + "fmt" + "strings" + "sync/atomic" "testing" + "time" + "github.com/kandev/kandev/internal/events" "github.com/kandev/kandev/internal/task/models" "github.com/kandev/kandev/internal/task/repository" + "github.com/kandev/kandev/internal/worktree" ) // errWorkspaceRepo is a WorkspaceRepository that always returns an error from @@ -16,6 +22,59 @@ type errWorkspaceRepo struct { WorkspaceRepositoryStub } +type blockingWorktreeCleanup struct { + release chan struct{} + active atomic.Int32 + maxActive atomic.Int32 +} + +func (c *blockingWorktreeCleanup) OnTaskDeleted(context.Context, string) error { + return nil +} + +func (c *blockingWorktreeCleanup) GetAllByTaskID(context.Context, string) ([]*worktree.Worktree, error) { + return nil, nil +} + +func (c *blockingWorktreeCleanup) CleanupWorktrees(ctx context.Context, _ []*worktree.Worktree) error { + active := c.active.Add(1) + updateMaxActive(&c.maxActive, active) + defer c.active.Add(-1) + + select { + case <-ctx.Done(): + return ctx.Err() + case <-c.release: + return nil + } +} + +func updateMaxActive(maxActive *atomic.Int32, value int32) { + for { + current := maxActive.Load() + if value <= current || maxActive.CompareAndSwap(current, value) { + return + } + } +} + +func waitForActiveCleanups(t *testing.T, cleanup *blockingWorktreeCleanup, want int32) { + t.Helper() + deadline := time.After(500 * time.Millisecond) + ticker := time.NewTicker(5 * time.Millisecond) + defer ticker.Stop() + for { + if cleanup.active.Load() >= want { + return + } + select { + case <-deadline: + t.Fatalf("active cleanups = %d, want at least %d", cleanup.active.Load(), want) + case <-ticker.C: + } + } +} + // WorkspaceRepositoryStub satisfies the full WorkspaceRepository interface // with no-op / panic stubs. Only methods under test need real implementations. type WorkspaceRepositoryStub struct{} @@ -32,10 +91,73 @@ func (WorkspaceRepositoryStub) UpdateWorkspace(_ context.Context, _ *models.Work func (WorkspaceRepositoryStub) DeleteWorkspace(_ context.Context, _ string) error { panic("not implemented") } +func (WorkspaceRepositoryStub) DeleteWorkspaceCascade(_ context.Context, _ string) ([]*models.Task, []*models.Workflow, error) { + panic("not implemented") +} +func (WorkspaceRepositoryStub) DeleteWorkspaceCascadeWithName(_ context.Context, _, _ string) ([]*models.Task, []*models.Workflow, error) { + panic("not implemented") +} func (WorkspaceRepositoryStub) ListWorkspaces(_ context.Context) ([]*models.Workspace, error) { panic("not implemented") } +type renameBeforeConfirmedDeleteRepo struct { + repository.WorkspaceRepository +} + +func (r renameBeforeConfirmedDeleteRepo) DeleteWorkspaceCascadeWithName( + ctx context.Context, + id, name string, +) ([]*models.Task, []*models.Workflow, error) { + workspace, err := r.GetWorkspace(ctx, id) + if err != nil { + return nil, nil, err + } + workspace.Name = "Renamed" + if err := r.UpdateWorkspace(ctx, workspace); err != nil { + return nil, nil, err + } + return r.WorkspaceRepository.DeleteWorkspaceCascadeWithName(ctx, id, name) +} + +type createDuringConfirmedDeleteRepo struct { + repository.WorkspaceRepository + tasks repository.TaskRepository + workflows repository.WorkflowRepository +} + +func (r createDuringConfirmedDeleteRepo) DeleteWorkspaceCascadeWithName( + ctx context.Context, + id, name string, +) ([]*models.Task, []*models.Workflow, error) { + if err := r.workflows.CreateWorkflow(ctx, &models.Workflow{ + ID: "wf-raced", + WorkspaceID: id, + Name: "Raced", + }); err != nil { + return nil, nil, err + } + if err := r.tasks.CreateTask(ctx, &models.Task{ + ID: "task-raced", + WorkspaceID: id, + WorkflowID: "wf-raced", + WorkflowStepID: "step-raced", + Title: "Raced task", + }); err != nil { + return nil, nil, err + } + return r.WorkspaceRepository.DeleteWorkspaceCascadeWithName(ctx, id, name) +} + +type failingListTaskSessionsRepo struct { + repository.SessionRepository + err error +} + +func (r failingListTaskSessionsRepo) ListTaskSessions(context.Context, string) ([]*models.TaskSession, error) { + return nil, r.err +} + func (e errWorkspaceRepo) ListWorkspaces(_ context.Context) ([]*models.Workspace, error) { return nil, errors.New("db unavailable") } @@ -129,6 +251,388 @@ func TestService_GetOfficeWorkflowIDs_DBError(t *testing.T) { } } +func TestService_DeleteWorkspaceDeletesWorkspaceOwnedTasksAndWorkflows(t *testing.T) { + svc, _, repo := createTestService(t) + ctx := context.Background() + + _ = repo.CreateWorkspace(ctx, &models.Workspace{ID: "ws-delete", Name: "Delete Me"}) + _ = repo.CreateWorkspace(ctx, &models.Workspace{ID: "ws-keep", Name: "Keep Me"}) + _ = repo.CreateWorkflow(ctx, &models.Workflow{ID: "wf-delete", WorkspaceID: "ws-delete", Name: "Doomed"}) + _ = repo.CreateWorkflow(ctx, &models.Workflow{ID: "wf-keep", WorkspaceID: "ws-keep", Name: "Keep"}) + if err := repo.CreateTask(ctx, &models.Task{ + ID: "task-delete", + WorkspaceID: "ws-delete", + WorkflowID: "wf-delete", + WorkflowStepID: "step-delete", + Title: "Delete task", + }); err != nil { + t.Fatalf("CreateTask: %v", err) + } + + if err := svc.DeleteWorkspace(ctx, "ws-delete"); err != nil { + t.Fatalf("DeleteWorkspace: %v", err) + } + + if _, err := repo.GetWorkspace(ctx, "ws-delete"); err == nil { + t.Fatalf("workspace should be deleted") + } + if _, err := repo.GetTask(ctx, "task-delete"); err == nil { + t.Fatalf("workspace task should be deleted") + } + workflows, err := repo.ListWorkflows(ctx, "ws-delete", true) + if err != nil { + t.Fatalf("ListWorkflows: %v", err) + } + if len(workflows) != 0 { + t.Fatalf("workspace workflows should be deleted, got %d", len(workflows)) + } + if _, err := repo.GetWorkflow(ctx, "wf-keep"); err != nil { + t.Fatalf("unrelated workflow should remain: %v", err) + } +} + +func TestService_DeleteWorkspacePublishesChildEventsAndCleansResources(t *testing.T) { + svc, eventBus, repo := createTestService(t) + ctx := context.Background() + svc.setCleanupDoneForTestHook(make(chan struct{}, 1)) + cleanup := &recordingWorktreeCleanup{ + worktrees: []*worktree.Worktree{{ID: "wt-delete", TaskID: "task-delete"}}, + } + svc.SetWorktreeCleanup(cleanup) + + _ = repo.CreateWorkspace(ctx, &models.Workspace{ID: "ws-delete", Name: "Delete Me"}) + _ = repo.CreateWorkflow(ctx, &models.Workflow{ID: "wf-delete", WorkspaceID: "ws-delete", Name: "Doomed"}) + if err := repo.CreateTask(ctx, &models.Task{ + ID: "task-delete", + WorkspaceID: "ws-delete", + WorkflowID: "wf-delete", + WorkflowStepID: "step-delete", + Title: "Delete task", + }); err != nil { + t.Fatalf("CreateTask: %v", err) + } + eventBus.ClearEvents() + + if err := svc.DeleteWorkspace(ctx, "ws-delete"); err != nil { + t.Fatalf("DeleteWorkspace: %v", err) + } + waitForCleanupDone(t, svc) + + eventCounts := make(map[string]int) + for _, event := range eventBus.GetPublishedEvents() { + eventCounts[event.Type]++ + } + if eventCounts[events.TaskDeleted] != 1 { + t.Fatalf("task deleted events = %d, want 1", eventCounts[events.TaskDeleted]) + } + if eventCounts[events.WorkflowDeleted] != 1 { + t.Fatalf("workflow deleted events = %d, want 1", eventCounts[events.WorkflowDeleted]) + } + if eventCounts[events.WorkspaceDeleted] != 1 { + t.Fatalf("workspace deleted events = %d, want 1", eventCounts[events.WorkspaceDeleted]) + } + cleanedIDs := cleanup.cleanedIDs() + if len(cleanedIDs) != 1 || cleanedIDs[0] != "wt-delete" { + t.Fatalf("cleaned worktrees = %#v, want wt-delete", cleanedIDs) + } +} + +func TestService_DeleteWorkspaceStopsBeforeCascadeWhenSessionInventoryFails(t *testing.T) { + svc, _, repo := createTestService(t) + ctx := context.Background() + + _ = repo.CreateWorkspace(ctx, &models.Workspace{ID: "ws-delete", Name: "Delete Me"}) + _ = repo.CreateWorkflow(ctx, &models.Workflow{ID: "wf-delete", WorkspaceID: "ws-delete", Name: "Doomed"}) + if err := repo.CreateTask(ctx, &models.Task{ + ID: "task-delete", + WorkspaceID: "ws-delete", + WorkflowID: "wf-delete", + WorkflowStepID: "step-delete", + Title: "Delete task", + }); err != nil { + t.Fatalf("CreateTask: %v", err) + } + listErr := errors.New("session inventory unavailable") + svc.sessions = failingListTaskSessionsRepo{SessionRepository: repo, err: listErr} + + err := svc.DeleteWorkspace(ctx, "ws-delete") + if !errors.Is(err, listErr) { + t.Fatalf("expected session inventory error, got %v", err) + } + if _, err := repo.GetWorkspace(ctx, "ws-delete"); err != nil { + t.Fatalf("workspace should remain: %v", err) + } + if _, err := repo.GetTask(ctx, "task-delete"); err != nil { + t.Fatalf("workspace task should remain: %v", err) + } + if _, err := repo.GetWorkflow(ctx, "wf-delete"); err != nil { + t.Fatalf("workspace workflow should remain: %v", err) + } +} + +func TestService_RunWorkspaceDeleteTaskCleanupsCapsConcurrency(t *testing.T) { + svc, _, _ := createTestService(t) + taskCount := workspaceDeleteCleanupConcurrency + 3 + svc.setCleanupDoneForTestHook(make(chan struct{}, taskCount)) + cleanup := &blockingWorktreeCleanup{release: make(chan struct{})} + svc.SetWorktreeCleanup(cleanup) + + cleanups := make([]workspaceDeleteTaskCleanup, 0, taskCount) + deletedTasks := make([]*models.Task, 0, taskCount) + for i := 0; i < taskCount; i++ { + taskID := fmt.Sprintf("task-%02d", i) + task := &models.Task{ID: taskID} + cleanups = append(cleanups, workspaceDeleteTaskCleanup{ + task: task, + worktrees: []*worktree.Worktree{{ID: "wt-" + taskID, TaskID: taskID}}, + }) + deletedTasks = append(deletedTasks, task) + } + + svc.runWorkspaceDeleteTaskCleanups(cleanups, deletedTasks) + waitForActiveCleanups(t, cleanup, workspaceDeleteCleanupConcurrency) + close(cleanup.release) + for i := 0; i < taskCount; i++ { + waitForCleanupDone(t, svc) + } + + if got := cleanup.maxActive.Load(); got > workspaceDeleteCleanupConcurrency { + t.Fatalf("max active cleanups = %d, want <= %d", got, workspaceDeleteCleanupConcurrency) + } +} + +func TestService_DeleteWorkspaceWithConfirmNameDeletesWhenNameMatches(t *testing.T) { + svc, _, repo := createTestService(t) + ctx := context.Background() + + _ = repo.CreateWorkspace(ctx, &models.Workspace{ID: "ws-delete", Name: "Delete Me"}) + _ = repo.CreateWorkflow(ctx, &models.Workflow{ID: "wf-delete", WorkspaceID: "ws-delete", Name: "Doomed"}) + if err := repo.CreateTask(ctx, &models.Task{ + ID: "task-delete", + WorkspaceID: "ws-delete", + WorkflowID: "wf-delete", + WorkflowStepID: "step-delete", + Title: "Delete task", + }); err != nil { + t.Fatalf("CreateTask: %v", err) + } + + if err := svc.DeleteWorkspaceWithConfirmName(ctx, "ws-delete", "Delete Me"); err != nil { + t.Fatalf("DeleteWorkspaceWithConfirmName: %v", err) + } + + if _, err := repo.GetWorkspace(ctx, "ws-delete"); err == nil { + t.Fatalf("workspace should be deleted") + } + if _, err := repo.GetTask(ctx, "task-delete"); err == nil { + t.Fatalf("workspace task should be deleted") + } + workflows, err := repo.ListWorkflows(ctx, "ws-delete", true) + if err != nil { + t.Fatalf("ListWorkflows: %v", err) + } + if len(workflows) != 0 { + t.Fatalf("workspace workflows should be deleted, got %d", len(workflows)) + } +} + +func TestService_DeleteWorkspaceWithConfirmNamePublishesChildEventsAndCleansResources(t *testing.T) { + svc, eventBus, repo := createTestService(t) + ctx := context.Background() + svc.setCleanupDoneForTestHook(make(chan struct{}, 1)) + cleanup := &recordingWorktreeCleanup{ + worktrees: []*worktree.Worktree{{ID: "wt-delete", TaskID: "task-delete"}}, + } + svc.SetWorktreeCleanup(cleanup) + + _ = repo.CreateWorkspace(ctx, &models.Workspace{ID: "ws-delete", Name: "Delete Me"}) + _ = repo.CreateWorkflow(ctx, &models.Workflow{ID: "wf-delete", WorkspaceID: "ws-delete", Name: "Doomed"}) + if err := repo.CreateTask(ctx, &models.Task{ + ID: "task-delete", + WorkspaceID: "ws-delete", + WorkflowID: "wf-delete", + WorkflowStepID: "step-delete", + Title: "Delete task", + }); err != nil { + t.Fatalf("CreateTask: %v", err) + } + eventBus.ClearEvents() + + if err := svc.DeleteWorkspaceWithConfirmName(ctx, "ws-delete", "Delete Me"); err != nil { + t.Fatalf("DeleteWorkspaceWithConfirmName: %v", err) + } + waitForCleanupDone(t, svc) + + eventCounts := make(map[string]int) + for _, event := range eventBus.GetPublishedEvents() { + eventCounts[event.Type]++ + } + if eventCounts[events.TaskDeleted] != 1 { + t.Fatalf("task deleted events = %d, want 1", eventCounts[events.TaskDeleted]) + } + if eventCounts[events.WorkflowDeleted] != 1 { + t.Fatalf("workflow deleted events = %d, want 1", eventCounts[events.WorkflowDeleted]) + } + if eventCounts[events.WorkspaceDeleted] != 1 { + t.Fatalf("workspace deleted events = %d, want 1", eventCounts[events.WorkspaceDeleted]) + } + cleanedIDs := cleanup.cleanedIDs() + if len(cleanedIDs) != 1 || cleanedIDs[0] != "wt-delete" { + t.Fatalf("cleaned worktrees = %#v, want wt-delete", cleanedIDs) + } +} + +func TestService_DeleteWorkspaceWithConfirmNamePublishesEventsForAllCascadeDeletedChildren(t *testing.T) { + svc, eventBus, repo := createTestService(t) + ctx := context.Background() + svc.setCleanupDoneForTestHook(make(chan struct{}, 2)) + cleanup := &recordingWorktreeCleanup{ + worktreesByTaskID: map[string][]*worktree.Worktree{ + "task-delete": {{ID: "wt-delete", TaskID: "task-delete"}}, + "task-raced": {{ID: "wt-raced", TaskID: "task-raced"}}, + }, + } + svc.SetWorktreeCleanup(cleanup) + + _ = repo.CreateWorkspace(ctx, &models.Workspace{ID: "ws-delete", Name: "Delete Me"}) + _ = repo.CreateWorkflow(ctx, &models.Workflow{ID: "wf-delete", WorkspaceID: "ws-delete", Name: "Doomed"}) + if err := repo.CreateTask(ctx, &models.Task{ + ID: "task-delete", + WorkspaceID: "ws-delete", + WorkflowID: "wf-delete", + WorkflowStepID: "step-delete", + Title: "Delete task", + }); err != nil { + t.Fatalf("CreateTask: %v", err) + } + svc.workspaces = createDuringConfirmedDeleteRepo{ + WorkspaceRepository: repo, + tasks: repo, + workflows: repo, + } + eventBus.ClearEvents() + + if err := svc.DeleteWorkspaceWithConfirmName(ctx, "ws-delete", "Delete Me"); err != nil { + t.Fatalf("DeleteWorkspaceWithConfirmName: %v", err) + } + waitForCleanupDone(t, svc) + waitForCleanupDone(t, svc) + + // This covers event publication from the repository cascade return value. + // Runtime cleanup is prepared before the cascade and topped up from the + // cascade return value for children that appear after that first snapshot. + eventCounts := make(map[string]int) + for _, event := range eventBus.GetPublishedEvents() { + eventCounts[event.Type]++ + } + if eventCounts[events.TaskDeleted] != 2 { + t.Fatalf("task deleted events = %d, want 2", eventCounts[events.TaskDeleted]) + } + if eventCounts[events.WorkflowDeleted] != 2 { + t.Fatalf("workflow deleted events = %d, want 2", eventCounts[events.WorkflowDeleted]) + } + if _, err := repo.GetTask(ctx, "task-raced"); err == nil { + t.Fatalf("late-created task should be deleted") + } + if _, err := repo.GetWorkflow(ctx, "wf-raced"); err == nil { + t.Fatalf("late-created workflow should be deleted") + } + cleaned := make(map[string]bool) + for _, id := range cleanup.cleanedIDs() { + cleaned[id] = true + } + if len(cleaned) != 2 || !cleaned["wt-delete"] || !cleaned["wt-raced"] { + t.Fatalf("cleaned worktrees = %#v, want wt-delete and wt-raced", cleaned) + } +} + +func TestService_DeleteWorkspaceWithConfirmNameRejectsMismatchedName(t *testing.T) { + svc, _, repo := createTestService(t) + ctx := context.Background() + + _ = repo.CreateWorkspace(ctx, &models.Workspace{ID: "ws-delete", Name: "Delete Me"}) + _ = repo.CreateWorkflow(ctx, &models.Workflow{ID: "wf-delete", WorkspaceID: "ws-delete", Name: "Doomed"}) + if err := repo.CreateTask(ctx, &models.Task{ + ID: "task-delete", + WorkspaceID: "ws-delete", + WorkflowID: "wf-delete", + WorkflowStepID: "step-delete", + Title: "Delete task", + }); err != nil { + t.Fatalf("CreateTask: %v", err) + } + + err := svc.DeleteWorkspaceWithConfirmName(ctx, "ws-delete", "Wrong") + if !errors.Is(err, ErrWorkspaceConfirmNameMismatch) { + t.Fatalf("expected ErrWorkspaceConfirmNameMismatch, got %v", err) + } + + if _, err := repo.GetWorkspace(ctx, "ws-delete"); err != nil { + t.Fatalf("workspace should remain: %v", err) + } + if _, err := repo.GetTask(ctx, "task-delete"); err != nil { + t.Fatalf("workspace task should remain: %v", err) + } + if _, err := repo.GetWorkflow(ctx, "wf-delete"); err != nil { + t.Fatalf("workspace workflow should remain: %v", err) + } +} + +func TestService_DeleteWorkspaceWithConfirmNameReturnsMissingWorkspaceError(t *testing.T) { + svc, _, _ := createTestService(t) + ctx := context.Background() + + err := svc.DeleteWorkspaceWithConfirmName(ctx, "ws-missing", "Missing") + if err == nil { + t.Fatalf("expected missing workspace error") + } + if errors.Is(err, ErrWorkspaceConfirmNameMismatch) { + t.Fatalf("expected missing workspace error, got confirm-name mismatch") + } + if !errors.Is(err, repository.ErrWorkspaceNotFound) { + t.Fatalf("expected ErrWorkspaceNotFound, got %v", err) + } + if !strings.Contains(err.Error(), "workspace not found") { + t.Fatalf("expected workspace not found error, got %v", err) + } +} + +func TestService_DeleteWorkspaceWithConfirmNameRejectsFinalNameMismatch(t *testing.T) { + svc, _, repo := createTestService(t) + ctx := context.Background() + + _ = repo.CreateWorkspace(ctx, &models.Workspace{ID: "ws-delete", Name: "Delete Me"}) + _ = repo.CreateWorkflow(ctx, &models.Workflow{ID: "wf-delete", WorkspaceID: "ws-delete", Name: "Doomed"}) + if err := repo.CreateTask(ctx, &models.Task{ + ID: "task-delete", + WorkspaceID: "ws-delete", + WorkflowID: "wf-delete", + WorkflowStepID: "step-delete", + Title: "Delete task", + }); err != nil { + t.Fatalf("CreateTask: %v", err) + } + svc.workspaces = renameBeforeConfirmedDeleteRepo{WorkspaceRepository: repo} + + err := svc.DeleteWorkspaceWithConfirmName(ctx, "ws-delete", "Delete Me") + if !errors.Is(err, ErrWorkspaceConfirmNameMismatch) { + t.Fatalf("expected ErrWorkspaceConfirmNameMismatch, got %v", err) + } + workspace, err := repo.GetWorkspace(ctx, "ws-delete") + if err != nil { + t.Fatalf("workspace should remain: %v", err) + } + if workspace.Name != "Renamed" { + t.Fatalf("workspace should keep concurrent rename, got %q", workspace.Name) + } + if _, err := repo.GetTask(ctx, "task-delete"); err != nil { + t.Fatalf("workspace task should remain: %v", err) + } + if _, err := repo.GetWorkflow(ctx, "wf-delete"); err != nil { + t.Fatalf("workspace workflow should remain: %v", err) + } +} + // 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 diff --git a/apps/backend/internal/task/service/service_tasks.go b/apps/backend/internal/task/service/service_tasks.go index 1fbdbccac9..387db9774e 100644 --- a/apps/backend/internal/task/service/service_tasks.go +++ b/apps/backend/internal/task/service/service_tasks.go @@ -1012,27 +1012,36 @@ func (s *Service) runAsyncTaskCleanup( envCleanup taskEnvironmentCleanup, stopReason, stopFailMsg, cleanupMsg string, ) { - go func() { - cleanupStart := time.Now() - cleanupCtx, cancel := context.WithTimeout(context.Background(), 60*time.Second) - defer cancel() + go s.runTaskCleanup(id, sessions, worktrees, stopTargets, envCleanup, stopReason, stopFailMsg, cleanupMsg) +} - failedStops := s.stopTaskRuntimeTargets(cleanupCtx, id, stopTargets, stopReason, stopFailMsg) +func (s *Service) runTaskCleanup( + id string, + sessions []*models.TaskSession, + worktrees []*worktree.Worktree, + stopTargets []taskStopTarget, + envCleanup taskEnvironmentCleanup, + stopReason, stopFailMsg, cleanupMsg string, +) { + cleanupStart := time.Now() + cleanupCtx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + defer cancel() - cleanupErrors := s.performTaskCleanup(cleanupCtx, id, sessions, worktrees, stopTargets, envCleanup, failedStops) + failedStops := s.stopTaskRuntimeTargets(cleanupCtx, id, stopTargets, stopReason, stopFailMsg) - if len(cleanupErrors) > 0 { - s.logger.Warn(cleanupMsg+" with errors", - zap.String("task_id", id), - zap.Int("error_count", len(cleanupErrors)), - zap.Duration("duration", time.Since(cleanupStart))) - } else { - s.logger.Info(cleanupMsg, - zap.String("task_id", id), - zap.Duration("duration", time.Since(cleanupStart))) - } - s.signalCleanupDoneForTest() - }() + cleanupErrors := s.performTaskCleanup(cleanupCtx, id, sessions, worktrees, stopTargets, envCleanup, failedStops) + + if len(cleanupErrors) > 0 { + s.logger.Warn(cleanupMsg+" with errors", + zap.String("task_id", id), + zap.Int("error_count", len(cleanupErrors)), + zap.Duration("duration", time.Since(cleanupStart))) + } else { + s.logger.Info(cleanupMsg, + zap.String("task_id", id), + zap.Duration("duration", time.Since(cleanupStart))) + } + s.signalCleanupDoneForTest() } // isCleanableSessionState reports whether a session has no running agent process diff --git a/apps/backend/internal/task/service/service_test.go b/apps/backend/internal/task/service/service_test.go index 2a8010b860..e734e0176a 100644 --- a/apps/backend/internal/task/service/service_test.go +++ b/apps/backend/internal/task/service/service_test.go @@ -704,8 +704,9 @@ func TestService_DeleteTaskCleansSuccessfulSessionResourcesOnPartialStopFailure( if _, err := os.Stat(filepath.Join(quickChatDir, "session-failed")); err != nil { t.Fatalf("failed session quick-chat directory should remain: %v", err) } - if len(cleanup.cleaned) != 1 || cleanup.cleaned[0].ID != "wt-ok" { - t.Fatalf("expected only successful session worktree cleanup, got %#v", cleanup.cleaned) + cleanedIDs := cleanup.cleanedIDs() + if len(cleanedIDs) != 1 || cleanedIDs[0] != "wt-ok" { + t.Fatalf("expected only successful session worktree cleanup, got %#v", cleanedIDs) } if _, err := repo.GetExecutorRunningBySessionID(ctx, "session-failed"); err != nil { t.Fatalf("failed session executor row should remain retryable: %v", err) @@ -897,8 +898,8 @@ func TestService_CleanupTaskResourcesFailsClosedWhenRuntimeInventoryFails(t *tes if _, err := os.Stat(sessionDir); err != nil { t.Fatalf("quick-chat directory should remain when runtime inventory fails: %v", err) } - if len(cleanup.cleaned) != 0 { - t.Fatalf("worktrees should not be cleaned when runtime inventory fails, got %#v", cleanup.cleaned) + if cleanedIDs := cleanup.cleanedIDs(); len(cleanedIDs) != 0 { + t.Fatalf("worktrees should not be cleaned when runtime inventory fails, got %#v", cleanedIDs) } } @@ -975,23 +976,42 @@ func (r failingExecutorRepository) ListExecutorsRunningByTaskID(context.Context, } type recordingWorktreeCleanup struct { - worktrees []*worktree.Worktree - cleaned []*worktree.Worktree + mu sync.Mutex + worktrees []*worktree.Worktree + worktreesByTaskID map[string][]*worktree.Worktree + cleaned []*worktree.Worktree } func (c *recordingWorktreeCleanup) OnTaskDeleted(context.Context, string) error { return nil } -func (c *recordingWorktreeCleanup) GetAllByTaskID(context.Context, string) ([]*worktree.Worktree, error) { +func (c *recordingWorktreeCleanup) GetAllByTaskID(_ context.Context, taskID string) ([]*worktree.Worktree, error) { + if c.worktreesByTaskID != nil { + return c.worktreesByTaskID[taskID], nil + } return c.worktrees, nil } func (c *recordingWorktreeCleanup) CleanupWorktrees(_ context.Context, worktrees []*worktree.Worktree) error { + c.mu.Lock() + defer c.mu.Unlock() c.cleaned = append(c.cleaned, worktrees...) return nil } +func (c *recordingWorktreeCleanup) cleanedIDs() []string { + c.mu.Lock() + defer c.mu.Unlock() + ids := make([]string, 0, len(c.cleaned)) + for _, wt := range c.cleaned { + if wt != nil { + ids = append(ids, wt.ID) + } + } + return ids +} + func waitForCleanupDone(t *testing.T, svc *Service) { t.Helper() select { diff --git a/apps/web/components/task/dockview-desktop-layout.tsx b/apps/web/components/task/dockview-desktop-layout.tsx index 1e89cd3089..c929e323ae 100644 --- a/apps/web/components/task/dockview-desktop-layout.tsx +++ b/apps/web/components/task/dockview-desktop-layout.tsx @@ -254,6 +254,54 @@ type DockviewDesktopLayoutProps = { compact?: boolean; }; +type ReadyDockviewLayoutSetup = { + buildDefaultLayout: (api: DockviewReadyEvent["api"], intentName?: string) => void; + compact: boolean; + initialLayout?: string | null; +}; + +type ReadyDockviewRefs = { + envIdRef: React.MutableRefObject; + readyDisposersRef: React.MutableRefObject void>>; + saveTimerRef: React.MutableRefObject | null>; + setApi: (api: DockviewReadyEvent["api"] | null) => void; +}; + +type ReadyDockviewSetup = { + api: DockviewReadyEvent["api"]; + appStore: ReturnType; + layout: ReadyDockviewLayoutSetup; + refs: ReadyDockviewRefs; +}; + +function setupReadyDockview({ api, appStore, layout, refs }: ReadyDockviewSetup): void { + refs.setApi(api); + + const currentEnvId = refs.envIdRef.current; + const restored = + !layout.initialLayout && restoreEnvLayout(api, currentEnvId, appStore, VALID_COMPONENTS); + if (!restored) { + layout.buildDefaultLayout( + api, + layout.initialLayout ?? (layout.compact ? "compact" : undefined), + ); + } + + useDockviewStore.setState({ currentLayoutEnvId: currentEnvId }); + + refs.readyDisposersRef.current.push(setupGroupTracking(api)); + const sessionTabSyncDisposable = setupSessionTabSync(api, appStore); + refs.readyDisposersRef.current.push(() => sessionTabSyncDisposable.dispose()); + const chatPanelSafetyNetDisposable = setupChatPanelSafetyNet(api, appStore); + refs.readyDisposersRef.current.push(() => chatPanelSafetyNetDisposable.dispose()); + refs.readyDisposersRef.current.push( + setupLayoutPersistence(api, refs.saveTimerRef, refs.envIdRef), + ); + setupPortalCleanup(api, appStore); + refs.readyDisposersRef.current.push(setupContainerResizeSync(api)); + refs.readyDisposersRef.current.push(setupSashDragCapToggle(api)); +} + export const DockviewDesktopLayout = memo(function DockviewDesktopLayout({ sessionId, repository, @@ -290,27 +338,12 @@ export const DockviewDesktopLayout = memo(function DockviewDesktopLayout({ const onReady = useCallback( (event: DockviewReadyEvent) => { - const api = event.api; - setApi(api); - - const currentEnvId = envIdRef.current; - const restored = - !initialLayout && restoreEnvLayout(api, currentEnvId, appStore, VALID_COMPONENTS); - if (!restored) { - buildDefaultLayout(api, initialLayout ?? (compact ? "compact" : undefined)); - } - - useDockviewStore.setState({ currentLayoutEnvId: currentEnvId }); - - readyDisposersRef.current.push(setupGroupTracking(api)); - const sessionTabSyncDisposable = setupSessionTabSync(api, appStore); - readyDisposersRef.current.push(() => sessionTabSyncDisposable.dispose()); - const chatPanelSafetyNetDisposable = setupChatPanelSafetyNet(api, appStore); - readyDisposersRef.current.push(() => chatPanelSafetyNetDisposable.dispose()); - readyDisposersRef.current.push(setupLayoutPersistence(api, saveTimerRef, envIdRef)); - setupPortalCleanup(api, appStore); - readyDisposersRef.current.push(setupContainerResizeSync(api)); - readyDisposersRef.current.push(setupSashDragCapToggle(api)); + setupReadyDockview({ + api: event.api, + appStore, + layout: { buildDefaultLayout, compact, initialLayout }, + refs: { envIdRef, readyDisposersRef, saveTimerRef, setApi }, + }); }, [setApi, buildDefaultLayout, initialLayout, compact, appStore], );