From 49177d742fef0a03aaaea373ceab93067f3bd1b0 Mon Sep 17 00:00:00 2001 From: Alassane <79718216+yattdev@users.noreply.github.com> Date: Sat, 15 Aug 2026 12:21:14 -0400 Subject: [PATCH] fix(gitlab): delete task-owned MR/PR associations on hard task delete (#2655) Co-authored-by: ayattara Co-authored-by: Carlos Florencio --- .../backendapp/gitlab_service_test.go | 12 +- apps/backend/internal/backendapp/main.go | 3 + apps/backend/internal/backendapp/services.go | 11 +- apps/backend/internal/backendapp/types.go | 1 + .../internal/github/service_task_events.go | 15 +- .../service_task_events_cleanup_test.go | 37 +++ .../github/service_task_events_test.go | 18 ++ apps/backend/internal/github/store.go | 3 + .../github/store_ci_automation_test.go | 14 + .../internal/github/store_connections_test.go | 18 +- .../internal/github/store_task_cleanup.go | 79 ++++++ .../github/store_task_cleanup_qa_test.go | 256 ++++++++++++++++++ .../github/store_task_cleanup_test.go | 94 +++++++ .../github/store_task_pr_detach_test.go | 3 + apps/backend/internal/gitlab/provider.go | 5 +- apps/backend/internal/gitlab/service.go | 43 ++- .../internal/gitlab/service_task_events.go | 34 +++ .../gitlab/service_task_events_test.go | 169 ++++++++++++ apps/backend/internal/gitlab/store.go | 5 +- .../gitlab/store_mr_watch_migration_test.go | 4 +- .../internal/gitlab/store_task_cleanup.go | 77 ++++++ .../gitlab/store_task_cleanup_qa_test.go | 244 +++++++++++++++++ .../gitlab/store_task_cleanup_test.go | 105 +++++++ ...-13-hard-delete-task-contribution-links.md | 21 ++ docs/decisions/INDEX.md | 1 + docs/specs/gitlab-integration/spec.md | 4 + docs/specs/ui/ci-pr-automation.md | 4 + 27 files changed, 1264 insertions(+), 16 deletions(-) create mode 100644 apps/backend/internal/github/service_task_events_cleanup_test.go create mode 100644 apps/backend/internal/github/store_task_cleanup.go create mode 100644 apps/backend/internal/github/store_task_cleanup_qa_test.go create mode 100644 apps/backend/internal/github/store_task_cleanup_test.go create mode 100644 apps/backend/internal/gitlab/service_task_events.go create mode 100644 apps/backend/internal/gitlab/service_task_events_test.go create mode 100644 apps/backend/internal/gitlab/store_task_cleanup.go create mode 100644 apps/backend/internal/gitlab/store_task_cleanup_qa_test.go create mode 100644 apps/backend/internal/gitlab/store_task_cleanup_test.go create mode 100644 docs/decisions/2026-08-13-hard-delete-task-contribution-links.md diff --git a/apps/backend/internal/backendapp/gitlab_service_test.go b/apps/backend/internal/backendapp/gitlab_service_test.go index 41001b1b59..3aaddd6102 100644 --- a/apps/backend/internal/backendapp/gitlab_service_test.go +++ b/apps/backend/internal/backendapp/gitlab_service_test.go @@ -45,18 +45,26 @@ func TestInitGitLabServiceLoadsConfiguredHostAfterRestart(t *testing.T) { log := newTestLogger() secretsStore := emptySecretStore{} - first := initGitLabService(pool, nil, secretsStore, log) + first, firstCleanup := initGitLabService(pool, nil, secretsStore, log) if first == nil { t.Fatal("first GitLab service is nil") } + if firstCleanup == nil { + t.Fatal("first GitLab cleanup is nil") + } + t.Cleanup(func() { _ = firstCleanup() }) if err := first.ConfigureHost(context.Background(), gitlabServer.URL+"/"); err != nil { t.Fatalf("ConfigureHost: %v", err) } - second := initGitLabService(pool, nil, secretsStore, log) + second, secondCleanup := initGitLabService(pool, nil, secretsStore, log) if second == nil { t.Fatal("second GitLab service is nil") } + if secondCleanup == nil { + t.Fatal("second GitLab cleanup is nil") + } + t.Cleanup(func() { _ = secondCleanup() }) if got := second.Host(); got != gitlabServer.URL { t.Fatalf("Host after restart = %q, want %q", got, gitlabServer.URL) } diff --git a/apps/backend/internal/backendapp/main.go b/apps/backend/internal/backendapp/main.go index 732d9842ba..12c0d1d156 100644 --- a/apps/backend/internal/backendapp/main.go +++ b/apps/backend/internal/backendapp/main.go @@ -379,6 +379,9 @@ func startServices( //nolint:cyclop if services.Workflow != nil { addCleanup(services.Workflow.Close) } + if services.GitLabCleanup != nil { + addCleanup(services.GitLabCleanup) + } services.RuntimeFlags = runtimeflags.NewService( repos.RuntimeFlags, runtimeflags.RuntimeOptionsFromAppliedConfig(runtimeFlagDefaults, cfg), diff --git a/apps/backend/internal/backendapp/services.go b/apps/backend/internal/backendapp/services.go index 860671ea5d..aa2bf69e05 100644 --- a/apps/backend/internal/backendapp/services.go +++ b/apps/backend/internal/backendapp/services.go @@ -150,7 +150,7 @@ func provideServices(cfg *config.Config, log *logger.Logger, repos *Repositories taskSvc.SetTaskStatusSummaryPRReader(&githubTaskStatusSummaryPRReader{gh: githubSvc}) githubSvc.SetPromptResolver(promptSvc) } - gitlabSvc := initGitLabService(dbPool, eventBus, repos.Secrets, log) + gitlabSvc, gitlabCleanup := initGitLabService(dbPool, eventBus, repos.Secrets, log) if gitlabSvc != nil { gitlabSvc.SetPromptResolver(promptSvc) } @@ -231,6 +231,7 @@ func provideServices(cfg *config.Config, log *logger.Logger, repos *Repositories Workflow: workflowSvc, GitHub: githubSvc, GitLab: gitlabSvc, + GitLabCleanup: gitlabCleanup, AzureDevOps: azureDevOpsSvc, Jira: jiraSvc, Linear: linearSvc, @@ -604,14 +605,14 @@ func (s *gitlabHostStore) SetHost(ctx context.Context, host string) error { // initGitLabService wires up the GitLab integration. Failures are non-fatal: // the rest of the backend still boots without GitLab configured. -func initGitLabService(dbPool *db.Pool, eventBus bus.EventBus, secretsStore secrets.SecretStore, log *logger.Logger) *gitlab.Service { +func initGitLabService(dbPool *db.Pool, eventBus bus.EventBus, secretsStore secrets.SecretStore, log *logger.Logger) (*gitlab.Service, func() error) { adapter := &gitlabSecretAdapter{store: secretsStore} hostStore, hostStoreErr := newGitLabHostStore(dbPool) if hostStoreErr != nil { log.Warn("GitLab host store unavailable (non-fatal)", zap.Error(hostStoreErr)) - return nil + return nil, nil } - svc, _, err := gitlab.Provide(context.Background(), adapter, hostStore, log) + svc, cleanup, err := gitlab.Provide(context.Background(), adapter, hostStore, log) if err != nil { log.Warn("GitLab service initialization failed (non-fatal)", zap.Error(err)) } @@ -630,7 +631,7 @@ func initGitLabService(dbPool *db.Pool, eventBus bus.EventBus, secretsStore secr log.Warn("GitLab task-mr store unavailable (non-fatal)", zap.Error(storeErr)) } } - return svc + return svc, cleanup } // initJiraService wires up the Jira integration. Failures are non-fatal. diff --git a/apps/backend/internal/backendapp/types.go b/apps/backend/internal/backendapp/types.go index 9f73a40790..c0671ebf31 100644 --- a/apps/backend/internal/backendapp/types.go +++ b/apps/backend/internal/backendapp/types.go @@ -77,6 +77,7 @@ type Services struct { Workflow *workflowservice.Service GitHub *github.Service GitLab *gitlab.Service + GitLabCleanup func() error AzureDevOps *azuredevops.Service Jira *jira.Service Linear *linear.Service diff --git a/apps/backend/internal/github/service_task_events.go b/apps/backend/internal/github/service_task_events.go index d0e924a6b4..8e1debbb7d 100644 --- a/apps/backend/internal/github/service_task_events.go +++ b/apps/backend/internal/github/service_task_events.go @@ -66,14 +66,14 @@ func (s *Service) handleTaskUpdated(ctx context.Context, event *bus.Event) error return nil } -// handleTaskDeleted deletes PR watches when a task is hard-deleted. +// handleTaskDeleted removes task-owned GitHub state when a task is hard-deleted. func (s *Service) handleTaskDeleted(ctx context.Context, event *bus.Event) error { taskID, _ := taskIDAndArchivedFrom(event) if taskID == "" { return nil } s.revokeCredentialTask(taskID) - s.pruneWatchesForTask(ctx, taskID, "deleted") + s.pruneTaskOwnedStateForTask(ctx, taskID, "deleted") return nil } @@ -154,6 +154,17 @@ func (s *Service) revokeCredentialTask(taskID string) { } } +func (s *Service) pruneTaskOwnedStateForTask(ctx context.Context, taskID, reason string) { + n, err := s.store.DeleteTaskOwnedStateByTaskID(ctx, taskID) + if err != nil { + s.logger.Error("failed to delete task-owned GitHub state", zap.String("task_id", taskID), zap.String("reason", reason), zap.Error(err)) + return + } + if n > 0 { + s.logger.Info("pruned task-owned GitHub state after task change", zap.String("task_id", taskID), zap.String("reason", reason), zap.Int64("deleted", n)) + } +} + func (s *Service) pruneWatchesForTask(ctx context.Context, taskID, reason string) { n, err := s.store.DeletePRWatchesByTaskID(ctx, taskID) if err != nil { diff --git a/apps/backend/internal/github/service_task_events_cleanup_test.go b/apps/backend/internal/github/service_task_events_cleanup_test.go new file mode 100644 index 0000000000..f7037a4589 --- /dev/null +++ b/apps/backend/internal/github/service_task_events_cleanup_test.go @@ -0,0 +1,37 @@ +package github + +import ( + "context" + "testing" + + "github.com/kandev/kandev/internal/events" + "github.com/kandev/kandev/internal/events/bus" +) + +func TestHandleTaskDeletedRemovesTaskPRsAndWatches(t *testing.T) { + _, svc, _, store := setupPollerTest(t) + ctx := context.Background() + for _, taskID := range []string{"deleted", "retained"} { + seedTask(t, store, taskID, false) + if err := store.CreateTaskPR(ctx, &TaskPR{TaskID: taskID, WorkspaceID: testWorkspaceID, Owner: "owner", Repo: "repo", PRNumber: 1}); err != nil { + t.Fatalf("create task PR: %v", err) + } + mustCreateWatch(t, store, "session-"+taskID, taskID) + } + event := bus.NewEvent(events.TaskDeleted, "task-service", map[string]interface{}{"task_id": "deleted"}) + if err := svc.handleTaskDeleted(ctx, event); err != nil { + t.Fatalf("first delivery: %v", err) + } + if err := svc.handleTaskDeleted(ctx, event); err != nil { + t.Fatalf("repeat delivery: %v", err) + } + if got, _ := store.ListTaskPRsByTaskIncludingDetached(ctx, "deleted"); len(got) != 0 { + t.Fatalf("deleted task PRs = %d, want 0", len(got)) + } + if got, _ := store.ListTaskPRsByTaskIncludingDetached(ctx, "retained"); len(got) != 1 { + t.Fatalf("retained task PRs = %d, want 1", len(got)) + } + if got, _ := store.GetPRWatchBySession(ctx, "session-retained"); got == nil { + t.Fatal("unrelated watch was removed") + } +} diff --git a/apps/backend/internal/github/service_task_events_test.go b/apps/backend/internal/github/service_task_events_test.go index 3c4bb414dc..02bf0b6d2d 100644 --- a/apps/backend/internal/github/service_task_events_test.go +++ b/apps/backend/internal/github/service_task_events_test.go @@ -53,6 +53,9 @@ func TestHandleTaskUpdated_ArchiveDeletesWatches(t *testing.T) { seedTask(t, store, "t1", false) mustCreateWatch(t, store, "s1", "t1") + if err := store.CreateTaskPR(ctx, &TaskPR{TaskID: "t1", WorkspaceID: testWorkspaceID, Owner: "owner", Repo: "repo", PRNumber: 1}); err != nil { + t.Fatalf("create task PR: %v", err) + } // Simulate task-service publishing task.updated with archived_at set. event := bus.NewEvent(events.TaskUpdated, "task-service", map[string]interface{}{ @@ -66,6 +69,10 @@ func TestHandleTaskUpdated_ArchiveDeletesWatches(t *testing.T) { if got, _ := store.GetPRWatchBySession(ctx, "s1"); got != nil { t.Errorf("expected watch to be deleted after archive event, got %+v", got) } + // AC4: archive must not delete the task/PR association, only the watch. + if got, _ := store.ListTaskPRsByTaskIncludingDetached(ctx, "t1"); len(got) != 1 { + t.Errorf("task PRs after archive event = %d, want 1 (association retained)", len(got)) + } } func TestHandleTaskUpdated_NonArchiveUpdateLeavesWatches(t *testing.T) { @@ -130,6 +137,14 @@ func TestHandleTaskEvents_MalformedPayloadIsNoop(t *testing.T) { seedTask(t, store, "t1", false) mustCreateWatch(t, store, "s1", "t1") + if err := store.CreateTaskPR(ctx, &TaskPR{TaskID: "t1", WorkspaceID: testWorkspaceID, Owner: "owner", Repo: "repo", PRNumber: 1}); err != nil { + t.Fatalf("create task PR: %v", err) + } + + // Nil event โ€” should be ignored, not crash. + if err := svc.handleTaskDeleted(ctx, nil); err != nil { + t.Fatalf("handleTaskDeleted nil event: %v", err) + } // Wrong payload type โ€” should be ignored, not crash. bad := bus.NewEvent(events.TaskUpdated, "task-service", "not-a-map") @@ -146,6 +161,9 @@ func TestHandleTaskEvents_MalformedPayloadIsNoop(t *testing.T) { if got, _ := store.GetPRWatchBySession(ctx, "s1"); got == nil { t.Error("expected watch to persist when payload is malformed") } + if got, _ := store.ListTaskPRsByTaskIncludingDetached(ctx, "t1"); len(got) != 1 { + t.Errorf("task PRs after malformed events = %d, want 1", len(got)) + } } func TestHandleWorkspaceDeletedRemovesOnlyOwnedConnectionSecrets(t *testing.T) { diff --git a/apps/backend/internal/github/store.go b/apps/backend/internal/github/store.go index 4f641b8c03..38ea24227a 100644 --- a/apps/backend/internal/github/store.go +++ b/apps/backend/internal/github/store.go @@ -587,6 +587,9 @@ func (s *Store) initSchemaData(legacyUpgrade bool) error { if err := s.backfillPRWatchesRepositoryID(); err != nil { return fmt.Errorf("backfill github_pr_watches.repository_id: %w", err) } + if err := s.healTaskOwnedOrphans(); err != nil { + return err + } if err := s.migrateTaskCIOptionsToPRScope(); err != nil { return fmt.Errorf("migrate task CI options to PR scope: %w", err) } diff --git a/apps/backend/internal/github/store_ci_automation_test.go b/apps/backend/internal/github/store_ci_automation_test.go index 68e3f1923e..d3e184e2bd 100644 --- a/apps/backend/internal/github/store_ci_automation_test.go +++ b/apps/backend/internal/github/store_ci_automation_test.go @@ -565,6 +565,15 @@ func seedLegacyTaskCIOptions(t *testing.T, store *Store, taskID string, autoFix, } } +// insertTestTask registers taskID in the tasks table so the task-contribution +// orphan sweep does not delete rows the test seeds against it. +func insertTestTask(t *testing.T, store *Store, taskID string) { + t.Helper() + if _, err := store.db.Exec(`INSERT INTO tasks (id, workspace_id) VALUES (?, ?)`, taskID, "ws-1"); err != nil { + t.Fatalf("insert task %s: %v", taskID, err) + } +} + // TestStoreMigrateTaskCIOptionsToPRScope_FansOutToLinkedPRs covers AC14: a // pre-upgrade task row with two linked PRs yields, after one boot, two // per-PR rows each matching the legacy booleans. @@ -573,6 +582,7 @@ func TestStoreMigrateTaskCIOptionsToPRScope_FansOutToLinkedPRs(t *testing.T) { ctx := context.Background() now := time.Now().UTC() + insertTestTask(t, store, "task-1") seedLegacyTaskCIOptions(t, store, "task-1", true, false) if err := store.CreateTaskPR(ctx, &TaskPR{ TaskID: "task-1", RepositoryID: "repo-1", Owner: "o", Repo: "r", PRNumber: 1, CreatedAt: now, @@ -612,6 +622,7 @@ func TestStoreMigrateTaskCIOptionsToPRScope_SkipsDetachedPRs(t *testing.T) { ctx := context.Background() now := time.Now().UTC() + insertTestTask(t, store, "task-1") seedLegacyTaskCIOptions(t, store, "task-1", true, true) active := &TaskPR{ TaskID: "task-1", RepositoryID: "repo-1", Owner: "o", Repo: "r", PRNumber: 1, CreatedAt: now, @@ -654,6 +665,7 @@ func TestStoreMigrateTaskCIOptionsToPRScope_Idempotent(t *testing.T) { ctx := context.Background() now := time.Now().UTC() + insertTestTask(t, store, "task-1") seedLegacyTaskCIOptions(t, store, "task-1", true, false) if err := store.CreateTaskPR(ctx, &TaskPR{ TaskID: "task-1", RepositoryID: "repo-1", Owner: "o", Repo: "r", PRNumber: 1, CreatedAt: now, @@ -688,6 +700,7 @@ func TestStoreMigrateTaskCIOptionsToPRScope_DoesNotReenableUserDisabled(t *testi ctx := context.Background() now := time.Now().UTC() + insertTestTask(t, store, "task-1") seedLegacyTaskCIOptions(t, store, "task-1", true, false) if err := store.CreateTaskPR(ctx, &TaskPR{ TaskID: "task-1", RepositoryID: "repo-1", Owner: "o", Repo: "r", PRNumber: 1, CreatedAt: now, @@ -725,6 +738,7 @@ func TestStoreMigrateTaskCIOptionsToPRScope_NewlyLinkedPRStartsAllOff(t *testing ctx := context.Background() now := time.Now().UTC() + insertTestTask(t, store, "task-1") seedLegacyTaskCIOptions(t, store, "task-1", true, true) if err := store.CreateTaskPR(ctx, &TaskPR{ TaskID: "task-1", RepositoryID: "repo-1", Owner: "o", Repo: "r", PRNumber: 1, CreatedAt: now, diff --git a/apps/backend/internal/github/store_connections_test.go b/apps/backend/internal/github/store_connections_test.go index 3de745f920..134e9aaa59 100644 --- a/apps/backend/internal/github/store_connections_test.go +++ b/apps/backend/internal/github/store_connections_test.go @@ -258,8 +258,8 @@ func TestStoreLegacyMigrationSeedsExistingWorkspacesAndBackfillsOwnership(t *tes assertStoredWorkspaceID(t, db, "github_pr_watches", "watch-owned", "ws-1") assertStoredWorkspaceID(t, db, "github_task_prs", "pr-owned", "ws-2") - assertStoredWorkspaceID(t, db, "github_pr_watches", "watch-orphan", "") - assertStoredWorkspaceID(t, db, "github_task_prs", "pr-orphan", "") + assertRowDeleted(t, db, "github_pr_watches", "watch-orphan") + assertRowDeleted(t, db, "github_task_prs", "pr-orphan") var targetLogin string if err := db.GetContext(ctx, &targetLogin, `SELECT target_login FROM github_review_watches WHERE id = 'review-watch-1'`); err != nil { @@ -686,3 +686,17 @@ func assertStoredWorkspaceID(t *testing.T, db *sqlx.DB, table, id, want string) t.Fatalf("%s %s workspace_id = %q, want %q", table, id, got, want) } } + +// assertRowDeleted asserts a row referencing a nonexistent task was removed +// by the task-contribution orphan sweep rather than surviving with a +// backfilled (empty) workspace_id. +func assertRowDeleted(t *testing.T, db *sqlx.DB, table, id string) { + t.Helper() + var count int + if err := db.QueryRow(`SELECT COUNT(*) FROM `+table+` WHERE id = ?`, id).Scan(&count); err != nil { + t.Fatalf("count %s %s: %v", table, id, err) + } + if count != 0 { + t.Fatalf("%s %s still present, want deleted as a task-contribution orphan", table, id) + } +} diff --git a/apps/backend/internal/github/store_task_cleanup.go b/apps/backend/internal/github/store_task_cleanup.go new file mode 100644 index 0000000000..174ce92ac4 --- /dev/null +++ b/apps/backend/internal/github/store_task_cleanup.go @@ -0,0 +1,79 @@ +package github + +import ( + "context" + "database/sql" + "errors" + "fmt" +) + +var githubTaskOwnedTables = []string{ + "github_pr_watches", + "github_task_prs", + "github_task_ci_options", + "github_task_pr_automation_options", + "github_task_ci_pr_state", +} + +// DeleteTaskPRsByTaskID removes every contribution association owned by a +// hard-deleted task. It is safe to replay after a missed event. +func (s *Store) DeleteTaskPRsByTaskID(ctx context.Context, taskID string) (int64, error) { + result, err := s.db.ExecContext(ctx, `DELETE FROM github_task_prs WHERE task_id = ?`, taskID) + if err != nil { + return 0, err + } + return result.RowsAffected() +} + +// DeleteTaskOwnedStateByTaskID removes all GitHub rows owned by a hard-deleted +// task in one transaction. It is safe to replay after a missed event. +func (s *Store) DeleteTaskOwnedStateByTaskID(ctx context.Context, taskID string) (int64, error) { + tx, err := s.db.BeginTxx(ctx, nil) + if err != nil { + return 0, err + } + defer func() { _ = tx.Rollback() }() + + var deleted int64 + for _, table := range githubTaskOwnedTables { + result, err := tx.ExecContext(ctx, `DELETE FROM `+table+` WHERE task_id = ?`, taskID) + if err != nil { + return 0, fmt.Errorf("delete task-owned rows from %s: %w", table, err) + } + rows, err := result.RowsAffected() + if err != nil { + return 0, fmt.Errorf("count deleted rows from %s: %w", table, err) + } + deleted += rows + } + if err := tx.Commit(); err != nil { + return 0, err + } + return deleted, nil +} + +func (s *Store) healTaskOwnedOrphans() error { + var exists int + err := s.db.Get(&exists, `SELECT 1 FROM sqlite_master WHERE type = 'table' AND name = 'tasks'`) + if errors.Is(err, sql.ErrNoRows) { + return nil + } + if err != nil { + return fmt.Errorf("check tasks table for orphan sweep: %w", err) + } + + tx, err := s.db.Beginx() + if err != nil { + return fmt.Errorf("begin orphan sweep: %w", err) + } + defer func() { _ = tx.Rollback() }() + for _, table := range githubTaskOwnedTables { + if _, err := tx.Exec(`DELETE FROM ` + table + ` WHERE NOT EXISTS (SELECT 1 FROM tasks WHERE tasks.id = ` + table + `.task_id)`); err != nil { + return fmt.Errorf("remove orphaned %s: %w", table, err) + } + } + if err := tx.Commit(); err != nil { + return fmt.Errorf("commit orphan sweep: %w", err) + } + return nil +} diff --git a/apps/backend/internal/github/store_task_cleanup_qa_test.go b/apps/backend/internal/github/store_task_cleanup_qa_test.go new file mode 100644 index 0000000000..dd6184ac3d --- /dev/null +++ b/apps/backend/internal/github/store_task_cleanup_qa_test.go @@ -0,0 +1,256 @@ +package github + +import ( + "context" + "strings" + "sync" + "testing" + "time" + + "github.com/kandev/kandev/internal/events" + "github.com/kandev/kandev/internal/events/bus" +) + +var testGitHubTaskOwnedTables = []string{ + "github_pr_watches", + "github_task_prs", + "github_task_ci_options", + "github_task_pr_automation_options", + "github_task_ci_pr_state", +} + +func seedGitHubTaskOwnedAutomationState(t *testing.T, store *Store, taskID string) { + t.Helper() + now := time.Now().UTC() + if _, err := store.db.Exec(` + INSERT INTO github_task_ci_options (task_id, created_at, updated_at) + VALUES (?, ?, ?)`, taskID, now, now); err != nil { + t.Fatalf("seed task CI options for %s: %v", taskID, err) + } + if _, err := store.db.Exec(` + INSERT INTO github_task_pr_automation_options + (task_id, repository_id, pr_number, created_at, updated_at) + VALUES (?, ?, ?, ?, ?)`, taskID, "", 1, now, now); err != nil { + t.Fatalf("seed PR automation options for %s: %v", taskID, err) + } + if _, err := store.db.Exec(` + INSERT INTO github_task_ci_pr_state + (task_id, repository_id, pr_number, created_at, updated_at) + VALUES (?, ?, ?, ?, ?)`, taskID, "", 1, now, now); err != nil { + t.Fatalf("seed PR automation state for %s: %v", taskID, err) + } +} + +func countGitHubTaskOwnedRows(t *testing.T, store *Store, table, taskID string) int { + t.Helper() + var count int + if err := store.db.Get(&count, `SELECT COUNT(*) FROM `+table+` WHERE task_id = ?`, taskID); err != nil { + t.Fatalf("count %s for %s: %v", table, taskID, err) + } + return count +} + +// countTaskPRRows counts github_task_prs rows directly, bypassing the +// task-scoped read paths so a test can observe rows for task IDs those paths +// would refuse (empty string, injection payloads). +func countTaskPRRows(t *testing.T, store *Store) int { + t.Helper() + var n int + if err := store.db.Get(&n, `SELECT COUNT(*) FROM github_task_prs`); err != nil { + t.Fatalf("count github_task_prs: %v", err) + } + return n +} + +// TestDeleteTaskPRsByTaskIDIsParameterized proves the task-scoped delete binds +// its task ID rather than interpolating it. A task_id carrying SQL +// metacharacters must delete exactly its own row and leave the table intact. +func TestDeleteTaskPRsByTaskIDIsParameterized(t *testing.T) { + _, _, _, store := setupPollerTest(t) + ctx := context.Background() + + const injection = `x'; DROP TABLE github_task_prs; --` + for i, taskID := range []string{injection, "innocent"} { + seedTask(t, store, taskID, false) + if err := store.CreateTaskPR(ctx, &TaskPR{ + TaskID: taskID, WorkspaceID: testWorkspaceID, Owner: "owner", Repo: "repo", PRNumber: i + 1, + }); err != nil { + t.Fatalf("create task PR for %q: %v", taskID, err) + } + } + + n, err := store.DeleteTaskPRsByTaskID(ctx, injection) + if err != nil { + t.Fatalf("delete by injection task id: %v", err) + } + if n != 1 { + t.Fatalf("deleted = %d, want 1", n) + } + // The table must still exist and still hold the unrelated row. + if got := countTaskPRRows(t, store); got != 1 { + t.Fatalf("remaining rows = %d, want 1 (table intact, innocent row kept)", got) + } + if got, _ := store.ListTaskPRsByTaskIncludingDetached(ctx, "innocent"); len(got) != 1 { + t.Fatalf("innocent task PRs = %d, want 1", len(got)) + } +} + +// TestHandleTaskDeletedConcurrentDeliveryIsIdempotent fires the same deletion +// event from many goroutines at once, the shape a redelivered or duplicated +// bus message takes. It exercises credential revocation, watch pruning and +// task-PR pruning together, and under -race guards the service state they share. +func TestHandleTaskDeletedConcurrentDeliveryIsIdempotent(t *testing.T) { + _, svc, _, store := setupPollerTest(t) + ctx := context.Background() + + for _, taskID := range []string{"doomed", "bystander"} { + seedTask(t, store, taskID, false) + if err := store.CreateTaskPR(ctx, &TaskPR{ + TaskID: taskID, WorkspaceID: testWorkspaceID, Owner: "owner", Repo: "repo", PRNumber: 1, + }); err != nil { + t.Fatalf("create task PR for %s: %v", taskID, err) + } + mustCreateWatch(t, store, "session-"+taskID, taskID) + } + + event := bus.NewEvent(events.TaskDeleted, "task-service", map[string]interface{}{"task_id": "doomed"}) + var wg sync.WaitGroup + errs := make(chan error, 16) + for range 16 { + wg.Add(1) + go func() { + defer wg.Done() + if err := svc.handleTaskDeleted(ctx, event); err != nil { + errs <- err + } + }() + } + wg.Wait() + close(errs) + for err := range errs { + t.Fatalf("concurrent delivery returned an error: %v", err) + } + + if got, _ := store.ListTaskPRsByTaskIncludingDetached(ctx, "doomed"); len(got) != 0 { + t.Fatalf("doomed task PRs = %d, want 0", len(got)) + } + if got, _ := store.ListTaskPRsByTaskIncludingDetached(ctx, "bystander"); len(got) != 1 { + t.Fatalf("bystander task PRs = %d, want 1", len(got)) + } + if got, _ := store.GetPRWatchBySession(ctx, "session-doomed"); got != nil { + t.Fatal("doomed watch survived the delete") + } + if got, _ := store.GetPRWatchBySession(ctx, "session-bystander"); got == nil { + t.Fatal("bystander watch was removed by a concurrent delete of another task") + } +} + +func TestHandleTaskDeletedRemovesAllTaskOwnedState(t *testing.T) { + _, svc, _, store := setupPollerTest(t) + ctx := context.Background() + + for _, taskID := range []string{"doomed", "bystander"} { + seedTask(t, store, taskID, false) + if err := store.CreateTaskPR(ctx, &TaskPR{ + TaskID: taskID, WorkspaceID: testWorkspaceID, Owner: "owner", Repo: "repo", PRNumber: 1, + }); err != nil { + t.Fatalf("create task PR for %s: %v", taskID, err) + } + mustCreateWatch(t, store, "session-"+taskID, taskID) + seedGitHubTaskOwnedAutomationState(t, store, taskID) + } + + event := bus.NewEvent(events.TaskDeleted, "task-service", map[string]interface{}{"task_id": "doomed"}) + if err := svc.handleTaskDeleted(ctx, event); err != nil { + t.Fatalf("handle task deleted: %v", err) + } + + for _, table := range testGitHubTaskOwnedTables { + if got := countGitHubTaskOwnedRows(t, store, table, "doomed"); got != 0 { + t.Fatalf("%s rows for doomed task = %d, want 0", table, got) + } + if got := countGitHubTaskOwnedRows(t, store, table, "bystander"); got != 1 { + t.Fatalf("%s rows for bystander task = %d, want 1", table, got) + } + } +} + +func TestHealTaskOwnedOrphansSurfacesDatabaseFailure(t *testing.T) { + _, _, _, store := setupPollerTest(t) + if err := store.db.Close(); err != nil { + t.Fatalf("close database: %v", err) + } + + if err := store.healTaskOwnedOrphans(); err == nil { + t.Fatal("orphan sweep returned nil for a closed database") + } +} + +// TestHealTaskOwnedOrphansHandlesUnusualTaskIDs pins how the startup +// sweep treats rows whose task_id can never match a tasks row: the empty +// string, and a very long unicode identifier. Both are ownerless by +// definition, so both must be swept, while a real task's rows survive. +func TestHealTaskOwnedOrphansHandlesUnusualTaskIDs(t *testing.T) { + _, _, _, store := setupPollerTest(t) + ctx := context.Background() + seedTask(t, store, "real-task", false) + + longUnicode := strings.Repeat("๐Ÿ› ๏ธไปปๅ‹™", 200) + for i, taskID := range []string{"real-task", "", longUnicode} { + if err := store.CreateTaskPR(ctx, &TaskPR{ + TaskID: taskID, WorkspaceID: testWorkspaceID, Owner: "owner", Repo: "repo", PRNumber: i + 1, + }); err != nil { + t.Fatalf("create task PR %d: %v", i, err) + } + } + if got := countTaskPRRows(t, store); got != 3 { + t.Fatalf("seeded rows = %d, want 3", got) + } + + if _, err := NewStore(store.db, store.ro); err != nil { + t.Fatalf("reopen store: %v", err) + } + + if got := countTaskPRRows(t, store); got != 1 { + t.Fatalf("rows after sweep = %d, want 1 (only the owned row survives)", got) + } + if got, _ := store.ListTaskPRsByTaskIncludingDetached(ctx, "real-task"); len(got) != 1 { + t.Fatalf("owned task PRs = %d, want 1", len(got)) + } +} + +// TestHandleTaskDeletedRemovesDetachedTaskPRTombstones covers a row shape the +// happy-path tests miss: a PR the user explicitly unlinked leaves a detached +// tombstone behind. Hard-deleting the owning task must remove the tombstone +// too, otherwise the "permanent delete" contract leaks rows no surface reads. +func TestHandleTaskDeletedRemovesDetachedTaskPRTombstones(t *testing.T) { + _, svc, _, store := setupPollerTest(t) + ctx := context.Background() + seedTask(t, store, "doomed", false) + + if err := store.CreateTaskPR(ctx, &TaskPR{ + TaskID: "doomed", WorkspaceID: testWorkspaceID, Owner: "owner", Repo: "repo", PRNumber: 1, + }); err != nil { + t.Fatalf("create task PR: %v", err) + } + if _, err := store.db.Exec( + `UPDATE github_task_prs SET detached_at = CURRENT_TIMESTAMP WHERE task_id = 'doomed'`, + ); err != nil { + t.Fatalf("detach task PR: %v", err) + } + if got, _ := store.ListTaskPRsByTaskIncludingDetached(ctx, "doomed"); len(got) != 1 { + t.Fatalf("detached tombstone missing before delete, got %d rows", len(got)) + } + + event := bus.NewEvent(events.TaskDeleted, "task-service", map[string]interface{}{"task_id": "doomed"}) + if err := svc.handleTaskDeleted(ctx, event); err != nil { + t.Fatalf("handle task deleted: %v", err) + } + + if got, _ := store.ListTaskPRsByTaskIncludingDetached(ctx, "doomed"); len(got) != 0 { + t.Fatalf("detached tombstone survived task deletion, got %d rows", len(got)) + } + if got := countTaskPRRows(t, store); got != 0 { + t.Fatalf("github_task_prs rows = %d, want 0", got) + } +} diff --git a/apps/backend/internal/github/store_task_cleanup_test.go b/apps/backend/internal/github/store_task_cleanup_test.go new file mode 100644 index 0000000000..5cf6d20afd --- /dev/null +++ b/apps/backend/internal/github/store_task_cleanup_test.go @@ -0,0 +1,94 @@ +package github + +import ( + "context" + "testing" +) + +func TestDeleteTaskPRsByTaskIDRemovesOnlyTargetRows(t *testing.T) { + _, _, _, store := setupPollerTest(t) + ctx := context.Background() + for _, taskID := range []string{"delete-me", "keep-me"} { + seedTask(t, store, taskID, false) + if err := store.CreateTaskPR(ctx, &TaskPR{TaskID: taskID, WorkspaceID: testWorkspaceID, Owner: "owner", Repo: "repo", PRNumber: 1}); err != nil { + t.Fatalf("create task PR: %v", err) + } + mustCreateWatch(t, store, "session-"+taskID, taskID) + } + + if _, err := store.DeletePRWatchesByTaskID(ctx, "delete-me"); err != nil { + t.Fatalf("delete watches: %v", err) + } + if _, err := store.DeleteTaskPRsByTaskID(ctx, "delete-me"); err != nil { + t.Fatalf("delete task PRs: %v", err) + } + + if got, err := store.ListTaskPRsByTaskIncludingDetached(ctx, "delete-me"); err != nil || len(got) != 0 { + t.Fatalf("target PRs = %d, %v; want 0", len(got), err) + } + if got, err := store.ListTaskPRsByTaskIncludingDetached(ctx, "keep-me"); err != nil || len(got) != 1 { + t.Fatalf("other PRs = %d, %v; want 1", len(got), err) + } + if got, _ := store.GetPRWatchBySession(ctx, "session-keep-me"); got == nil { + t.Fatal("unrelated watch was removed") + } +} + +// TestHealTaskOwnedOrphansOnStoreInit covers AC5/AC6: store +// initialization deletes github_task_prs/github_pr_watches rows whose task_id +// has no matching tasks.id, preserves rows for active and archived tasks, and +// a repeat initialization is a no-op. +func TestHealTaskOwnedOrphansOnStoreInit(t *testing.T) { + _, _, _, store := setupPollerTest(t) + ctx := context.Background() + + seedTask(t, store, "active-task", false) + seedTask(t, store, "archived-task", true) + // "orphan-task" is deliberately never inserted into tasks. + + for _, taskID := range []string{"active-task", "archived-task", "orphan-task"} { + if err := store.CreateTaskPR(ctx, &TaskPR{TaskID: taskID, WorkspaceID: testWorkspaceID, Owner: "owner", Repo: "repo", PRNumber: 1}); err != nil { + t.Fatalf("create task PR for %s: %v", taskID, err) + } + mustCreateWatch(t, store, "session-"+taskID, taskID) + seedGitHubTaskOwnedAutomationState(t, store, taskID) + } + + reopened, err := NewStore(store.db, store.ro) + if err != nil { + t.Fatalf("reopen store: %v", err) + } + for _, taskID := range []string{"active-task", "archived-task"} { + if got, err := reopened.ListTaskPRsByTaskIncludingDetached(ctx, taskID); err != nil || len(got) != 1 { + t.Fatalf("PRs for %s = %d, %v; want 1", taskID, len(got), err) + } + if got, _ := reopened.GetPRWatchBySession(ctx, "session-"+taskID); got == nil { + t.Fatalf("watch for %s was removed, want preserved", taskID) + } + for _, table := range testGitHubTaskOwnedTables { + if got := countGitHubTaskOwnedRows(t, reopened, table, taskID); got != 1 { + t.Fatalf("%s rows for %s = %d, want 1", table, taskID, got) + } + } + } + if got, err := reopened.ListTaskPRsByTaskIncludingDetached(ctx, "orphan-task"); err != nil || len(got) != 0 { + t.Fatalf("orphan PRs = %d, %v; want 0", len(got), err) + } + if got, _ := reopened.GetPRWatchBySession(ctx, "session-orphan-task"); got != nil { + t.Fatal("orphan watch was not removed") + } + for _, table := range testGitHubTaskOwnedTables { + if got := countGitHubTaskOwnedRows(t, reopened, table, "orphan-task"); got != 0 { + t.Fatalf("orphan %s rows = %d, want 0", table, got) + } + } + + if _, err := NewStore(reopened.db, reopened.ro); err != nil { + t.Fatalf("replay store init: %v", err) + } + for _, taskID := range []string{"active-task", "archived-task"} { + if got, err := reopened.ListTaskPRsByTaskIncludingDetached(ctx, taskID); err != nil || len(got) != 1 { + t.Fatalf("PRs for %s after replay = %d, %v; want 1", taskID, len(got), err) + } + } +} diff --git a/apps/backend/internal/github/store_task_pr_detach_test.go b/apps/backend/internal/github/store_task_pr_detach_test.go index e2c2345870..e273d142cb 100644 --- a/apps/backend/internal/github/store_task_pr_detach_test.go +++ b/apps/backend/internal/github/store_task_pr_detach_test.go @@ -17,6 +17,9 @@ func TestTaskPRDetachFiltersActiveRowsAndPersistsTombstone(t *testing.T) { } ctx := context.Background() now := time.Now().UTC() + if _, err := store.db.Exec(`INSERT INTO tasks (id, workspace_id) VALUES ('task-1', 'ws-1')`); err != nil { + t.Fatalf("seed task-1: %v", err) + } first := &TaskPR{ WorkspaceID: "ws-1", TaskID: "task-1", RepositoryID: "repo-1", Owner: "acme", Repo: "demo", PRNumber: 1, PRURL: "https://github.com/acme/demo/pull/1", diff --git a/apps/backend/internal/gitlab/provider.go b/apps/backend/internal/gitlab/provider.go index d7a2effe59..982c393bcc 100644 --- a/apps/backend/internal/gitlab/provider.go +++ b/apps/backend/internal/gitlab/provider.go @@ -46,7 +46,10 @@ func Provide( svc.SetHostStore(hostStore) } - cleanup := func() error { return nil } + cleanup := func() error { + svc.unsubscribeTaskEvents() + return nil + } return svc, cleanup, nil } diff --git a/apps/backend/internal/gitlab/service.go b/apps/backend/internal/gitlab/service.go index a524cda4be..91ac6d0557 100644 --- a/apps/backend/internal/gitlab/service.go +++ b/apps/backend/internal/gitlab/service.go @@ -13,6 +13,7 @@ import ( "github.com/kandev/kandev/internal/common/logger" "github.com/kandev/kandev/internal/common/securityutil" + "github.com/kandev/kandev/internal/events" "github.com/kandev/kandev/internal/events/bus" "github.com/kandev/kandev/internal/watchreset" ) @@ -107,6 +108,7 @@ type Service struct { hostStore HostStore store *Store eventBus bus.EventBus + taskEventSubs []bus.Subscription taskDeleter TaskDeleter cascadeTaskDeleter watchreset.TaskDeleter taskSessionChecker TaskSessionChecker @@ -140,8 +142,9 @@ func (s *Service) getPromptResolver() PromptResolver { // SetEventBus wires the event bus for publishing review/issue/feedback events. func (s *Service) SetEventBus(b bus.EventBus) { s.mu.Lock() - defer s.mu.Unlock() s.eventBus = b + s.mu.Unlock() + s.subscribeTaskEvents() } // SetTaskDeleter wires the task-deletion dependency used by cleanup sweepers. @@ -258,8 +261,44 @@ func (s *Service) resolveWatchRepository(ctx context.Context, workspaceID, repos // task-mr endpoints return empty results and SyncTaskMR is a no-op. func (s *Service) SetStore(store *Store) { s.mu.Lock() - defer s.mu.Unlock() s.store = store + s.mu.Unlock() + s.subscribeTaskEvents() +} + +func (s *Service) subscribeTaskEvents() { + s.mu.Lock() + busReady := s.eventBus != nil && s.store != nil && len(s.taskEventSubs) == 0 + eventBus := s.eventBus + s.mu.Unlock() + if !busReady { + return + } + sub, err := eventBus.Subscribe(events.TaskDeleted, s.handleTaskDeleted) + if err != nil { + s.logger.Error("failed to subscribe to task.deleted events", zap.Error(err)) + return + } + s.mu.Lock() + defer s.mu.Unlock() + if len(s.taskEventSubs) > 0 { + // A concurrent call already subscribed while we were outside the lock. + _ = sub.Unsubscribe() + return + } + s.taskEventSubs = append(s.taskEventSubs, sub) +} + +func (s *Service) unsubscribeTaskEvents() { + s.mu.Lock() + subs := s.taskEventSubs + s.taskEventSubs = nil + s.mu.Unlock() + for _, sub := range subs { + if err := sub.Unsubscribe(); err != nil { + s.logger.Error("failed to unsubscribe from task event", zap.Error(err)) + } + } } // SetWorkspaceSecretStore wires deterministic per-workspace credential storage. diff --git a/apps/backend/internal/gitlab/service_task_events.go b/apps/backend/internal/gitlab/service_task_events.go new file mode 100644 index 0000000000..782302a378 --- /dev/null +++ b/apps/backend/internal/gitlab/service_task_events.go @@ -0,0 +1,34 @@ +package gitlab + +import ( + "context" + + "go.uber.org/zap" + + "github.com/kandev/kandev/internal/events/bus" +) + +func (s *Service) handleTaskDeleted(ctx context.Context, event *bus.Event) error { + if event == nil { + return nil + } + payload, ok := event.Data.(map[string]interface{}) + if !ok { + return nil + } + taskID, _ := payload["task_id"].(string) + if taskID == "" { + return nil + } + store := s.requireStore() + if store == nil { + return nil + } + n, err := store.DeleteTaskOwnedStateByTaskID(ctx, taskID) + if err != nil { + s.logger.Error("failed to delete task-owned GitLab state", zap.String("task_id", taskID), zap.Error(err)) + } else if n > 0 { + s.logger.Info("pruned task-owned GitLab state after task deletion", zap.String("task_id", taskID), zap.Int64("deleted", n)) + } + return nil +} diff --git a/apps/backend/internal/gitlab/service_task_events_test.go b/apps/backend/internal/gitlab/service_task_events_test.go new file mode 100644 index 0000000000..d39558402d --- /dev/null +++ b/apps/backend/internal/gitlab/service_task_events_test.go @@ -0,0 +1,169 @@ +package gitlab + +import ( + "context" + "testing" + + "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/events" + "github.com/kandev/kandev/internal/events/bus" +) + +func TestTaskDeletedEventRemovesTaskMRsAndWatches(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + seedWorkspace(t, store, "ws") + for _, taskID := range []string{"deleted", "retained"} { + seedTask(t, store, taskID, "ws") + if err := store.UpsertTaskMR(ctx, newTestMR(taskID, "", "group/project", 1)); err != nil { + t.Fatalf("create task MR: %v", err) + } + if err := store.CreateMRWatch(ctx, &MRWatch{SessionID: "session-" + taskID, TaskID: taskID, ProjectPath: "group/project", Branch: "main"}); err != nil { + t.Fatalf("create MR watch: %v", err) + } + } + log, _ := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "console"}) + eventBus := bus.NewMemoryEventBus(log) + svc := NewService(DefaultHost, NewMockClient(DefaultHost), "", nil, log) + svc.SetEventBus(eventBus) + svc.SetStore(store) + t.Cleanup(svc.unsubscribeTaskEvents) + if err := eventBus.Publish(ctx, events.TaskDeleted, bus.NewEvent(events.TaskDeleted, "task-service", map[string]interface{}{"task_id": "deleted"})); err != nil { + t.Fatalf("publish: %v", err) + } + if got, _ := store.ListTaskMRsByTask(ctx, "deleted"); len(got) != 0 { + t.Fatalf("deleted task MRs = %d, want 0", len(got)) + } + if got, _ := store.ListTaskMRsByTask(ctx, "retained"); len(got) != 1 { + t.Fatalf("retained task MRs = %d, want 1", len(got)) + } + if got, _ := store.GetMRWatchBySession(ctx, "session-deleted"); got != nil { + t.Fatal("deleted task watch survived") + } + if got, _ := store.GetMRWatchBySession(ctx, "session-retained"); got == nil { + t.Fatal("unrelated watch was removed") + } +} + +// TestHandleTaskDeletedEvent_MalformedPayloadIsNoop covers AC3: a nil event, a +// non-map payload, and a payload without a non-empty task_id all leave +// gitlab_task_mrs/gitlab_mr_watches rows untouched. +func TestHandleTaskDeletedEvent_MalformedPayloadIsNoop(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + seedWorkspace(t, store, "ws") + seedTask(t, store, "t1", "ws") + if err := store.UpsertTaskMR(ctx, newTestMR("t1", "", "group/project", 1)); err != nil { + t.Fatalf("create task MR: %v", err) + } + if err := store.CreateMRWatch(ctx, &MRWatch{SessionID: "s1", TaskID: "t1", ProjectPath: "group/project", Branch: "main"}); err != nil { + t.Fatalf("create MR watch: %v", err) + } + + log, _ := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "console"}) + svc := NewService(DefaultHost, NewMockClient(DefaultHost), "", nil, log) + svc.SetStore(store) + + if err := svc.handleTaskDeleted(ctx, nil); err != nil { + t.Fatalf("nil event: %v", err) + } + if err := svc.handleTaskDeleted(ctx, bus.NewEvent(events.TaskDeleted, "task-service", "not-a-map")); err != nil { + t.Fatalf("non-map payload: %v", err) + } + if err := svc.handleTaskDeleted(ctx, bus.NewEvent(events.TaskDeleted, "task-service", map[string]interface{}{})); err != nil { + t.Fatalf("missing task_id: %v", err) + } + + if got, _ := store.ListTaskMRsByTask(ctx, "t1"); len(got) != 1 { + t.Fatalf("task MRs after malformed events = %d, want 1", len(got)) + } + if got, _ := store.GetMRWatchBySession(ctx, "s1"); got == nil { + t.Fatal("watch was removed by a malformed event") + } +} + +// TestHandleTaskUpdated_ArchiveRetainsTaskMRs covers AC4: GitLab does not +// subscribe to task.updated, so an archive event never touches task/MR links. +func TestHandleTaskUpdated_ArchiveRetainsTaskMRs(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + seedWorkspace(t, store, "ws") + seedTask(t, store, "t1", "ws") + if err := store.UpsertTaskMR(ctx, newTestMR("t1", "", "group/project", 1)); err != nil { + t.Fatalf("create task MR: %v", err) + } + if err := store.CreateMRWatch(ctx, &MRWatch{SessionID: "s1", TaskID: "t1", ProjectPath: "group/project", Branch: "main"}); err != nil { + t.Fatalf("create MR watch: %v", err) + } + + log, _ := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "console"}) + eventBus := bus.NewMemoryEventBus(log) + svc := NewService(DefaultHost, NewMockClient(DefaultHost), "", nil, log) + svc.SetEventBus(eventBus) + svc.SetStore(store) + t.Cleanup(svc.unsubscribeTaskEvents) + + // GitLab only ever subscribes to events.TaskDeleted (see + // subscribeTaskEvents), so this assertion pins that set: the retention + // below is a no-op because nothing handles task.updated, not because a + // handler inspects the event and chooses to retain. If GitLab ever grows + // a task.updated subscriber, this count changes and the assumption below + // must be revisited alongside it. + if got := len(svc.taskEventSubs); got != 1 { + t.Fatalf("task event subscriptions = %d, want 1 (task.deleted only)", got) + } + + archiveEvent := bus.NewEvent(events.TaskUpdated, "task-service", map[string]interface{}{ + "task_id": "t1", + "archived_at": "2026-04-19T12:00:00Z", + }) + if err := eventBus.Publish(ctx, events.TaskUpdated, archiveEvent); err != nil { + t.Fatalf("publish: %v", err) + } + + if got, _ := store.ListTaskMRsByTask(ctx, "t1"); len(got) != 1 { + t.Fatalf("task MRs after archive event = %d, want 1", len(got)) + } + if got, _ := store.GetMRWatchBySession(ctx, "s1"); got == nil { + t.Fatal("watch was removed by an archive event") + } +} + +// TestSubscribeTaskEventsIsIdempotentRegardlessOfSetOrder covers the +// duplicate-subscription risk: SetEventBus/SetStore may be called more than +// once, and in either order, but must only ever register one task.deleted +// subscription. +func TestSubscribeTaskEventsIsIdempotentRegardlessOfSetOrder(t *testing.T) { + store := newTestStore(t) + log, _ := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "console"}) + eventBus := bus.NewMemoryEventBus(log) + + svc := NewService(DefaultHost, NewMockClient(DefaultHost), "", nil, log) + svc.SetStore(store) + svc.SetEventBus(eventBus) + t.Cleanup(svc.unsubscribeTaskEvents) + if got := len(svc.taskEventSubs); got != 1 { + t.Fatalf("subscriptions after store-then-bus = %d, want 1", got) + } + + svc.SetStore(store) + svc.SetEventBus(eventBus) + if got := len(svc.taskEventSubs); got != 1 { + t.Fatalf("subscriptions after repeat Set calls = %d, want 1", got) + } + + // Repeat with the opposite setter order: SetEventBus before SetStore. + svc2 := NewService(DefaultHost, NewMockClient(DefaultHost), "", nil, log) + svc2.SetEventBus(eventBus) + svc2.SetStore(store) + t.Cleanup(svc2.unsubscribeTaskEvents) + if got := len(svc2.taskEventSubs); got != 1 { + t.Fatalf("subscriptions after bus-then-store = %d, want 1", got) + } + + svc2.SetEventBus(eventBus) + svc2.SetStore(store) + if got := len(svc2.taskEventSubs); got != 1 { + t.Fatalf("subscriptions after repeat Set calls (bus-then-store) = %d, want 1", got) + } +} diff --git a/apps/backend/internal/gitlab/store.go b/apps/backend/internal/gitlab/store.go index 37bb0a1bc5..4c90a8d75b 100644 --- a/apps/backend/internal/gitlab/store.go +++ b/apps/backend/internal/gitlab/store.go @@ -235,7 +235,10 @@ func (s *Store) createTables() error { if err := s.migrateMRWatchUniqueKey(); err != nil { return err } - return s.ensureMRWatchIndexes() + if err := s.ensureMRWatchIndexes(); err != nil { + return err + } + return s.healTaskOwnedOrphans() } // migrateTaskMRAutomationFields adds the reviewer/merge-readiness columns diff --git a/apps/backend/internal/gitlab/store_mr_watch_migration_test.go b/apps/backend/internal/gitlab/store_mr_watch_migration_test.go index c0251e676a..d6247f76a4 100644 --- a/apps/backend/internal/gitlab/store_mr_watch_migration_test.go +++ b/apps/backend/internal/gitlab/store_mr_watch_migration_test.go @@ -28,6 +28,7 @@ func newLegacyMRWatchStore(t *testing.T) *Store { if _, err := sqlxDB.Exec(` CREATE TABLE workspaces (id TEXT PRIMARY KEY, created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP); CREATE TABLE tasks (id TEXT PRIMARY KEY, workspace_id TEXT NOT NULL DEFAULT '', archived_at DATETIME); + INSERT INTO tasks (id) VALUES ('task-1'); CREATE TABLE gitlab_mr_watches ( id TEXT PRIMARY KEY, session_id TEXT NOT NULL, @@ -103,7 +104,8 @@ func TestMigrateMRWatchUniqueKey_ReplayIsNoOp(t *testing.T) { t.Cleanup(func() { _ = sqlxDB.Close() }) if _, err := sqlxDB.Exec(` CREATE TABLE workspaces (id TEXT PRIMARY KEY, created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP); - CREATE TABLE tasks (id TEXT PRIMARY KEY, workspace_id TEXT NOT NULL DEFAULT '', archived_at DATETIME)`); err != nil { + CREATE TABLE tasks (id TEXT PRIMARY KEY, workspace_id TEXT NOT NULL DEFAULT '', archived_at DATETIME); + INSERT INTO tasks (id) VALUES ('task-1')`); err != nil { t.Fatalf("create base schema: %v", err) } store, err := NewStore(sqlxDB, sqlxDB) diff --git a/apps/backend/internal/gitlab/store_task_cleanup.go b/apps/backend/internal/gitlab/store_task_cleanup.go new file mode 100644 index 0000000000..a01df0e5ff --- /dev/null +++ b/apps/backend/internal/gitlab/store_task_cleanup.go @@ -0,0 +1,77 @@ +package gitlab + +import ( + "context" + "database/sql" + "errors" + "fmt" +) + +var gitlabTaskOwnedTables = []string{ + "gitlab_mr_watches", + "gitlab_task_mrs", + "gitlab_task_mr_options", + "gitlab_task_mr_state", +} + +// DeleteTaskMRsByTaskID removes every contribution association owned by a +// hard-deleted task. It is safe to replay after a missed event. +func (s *Store) DeleteTaskMRsByTaskID(ctx context.Context, taskID string) (int64, error) { + result, err := s.db.ExecContext(ctx, `DELETE FROM gitlab_task_mrs WHERE task_id = ?`, taskID) + if err != nil { + return 0, err + } + return result.RowsAffected() +} + +// DeleteTaskOwnedStateByTaskID removes all GitLab rows owned by a hard-deleted +// task in one transaction. It is safe to replay after a missed event. +func (s *Store) DeleteTaskOwnedStateByTaskID(ctx context.Context, taskID string) (int64, error) { + tx, err := s.db.BeginTxx(ctx, nil) + if err != nil { + return 0, err + } + defer func() { _ = tx.Rollback() }() + + var deleted int64 + for _, table := range gitlabTaskOwnedTables { + result, err := tx.ExecContext(ctx, `DELETE FROM `+table+` WHERE task_id = ?`, taskID) + if err != nil { + return 0, fmt.Errorf("delete task-owned rows from %s: %w", table, err) + } + rows, err := result.RowsAffected() + if err != nil { + return 0, fmt.Errorf("count deleted rows from %s: %w", table, err) + } + deleted += rows + } + if err := tx.Commit(); err != nil { + return 0, err + } + return deleted, nil +} + +func (s *Store) healTaskOwnedOrphans() error { + var exists int + err := s.db.Get(&exists, `SELECT 1 FROM sqlite_master WHERE type = 'table' AND name = 'tasks'`) + if errors.Is(err, sql.ErrNoRows) { + return nil // tasks table not yet created โ€” skip sweep + } + if err != nil { + return err + } + tx, err := s.db.Beginx() + if err != nil { + return fmt.Errorf("begin orphan sweep: %w", err) + } + defer func() { _ = tx.Rollback() }() + for _, table := range gitlabTaskOwnedTables { + if _, err := tx.Exec(`DELETE FROM ` + table + ` WHERE NOT EXISTS (SELECT 1 FROM tasks WHERE tasks.id = ` + table + `.task_id)`); err != nil { + return fmt.Errorf("remove orphaned %s: %w", table, err) + } + } + if err := tx.Commit(); err != nil { + return fmt.Errorf("commit orphan sweep: %w", err) + } + return nil +} diff --git a/apps/backend/internal/gitlab/store_task_cleanup_qa_test.go b/apps/backend/internal/gitlab/store_task_cleanup_qa_test.go new file mode 100644 index 0000000000..feac6d6147 --- /dev/null +++ b/apps/backend/internal/gitlab/store_task_cleanup_qa_test.go @@ -0,0 +1,244 @@ +package gitlab + +import ( + "context" + "strings" + "sync" + "testing" + "time" + + "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/events" + "github.com/kandev/kandev/internal/events/bus" +) + +var testGitLabTaskOwnedTables = []string{ + "gitlab_mr_watches", + "gitlab_task_mrs", + "gitlab_task_mr_options", + "gitlab_task_mr_state", +} + +func seedGitLabTaskOwnedAutomationState(t *testing.T, store *Store, taskID string) { + t.Helper() + now := time.Now().UTC() + if _, err := store.db.Exec(` + INSERT INTO gitlab_task_mr_options (task_id, created_at, updated_at) + VALUES (?, ?, ?)`, taskID, now, now); err != nil { + t.Fatalf("seed task MR options for %s: %v", taskID, err) + } + if _, err := store.db.Exec(` + INSERT INTO gitlab_task_mr_state + (task_id, repository_id, project_path, mr_iid, created_at, updated_at) + VALUES (?, ?, ?, ?, ?, ?)`, taskID, "repo-1", "group/project", 1, now, now); err != nil { + t.Fatalf("seed MR automation state for %s: %v", taskID, err) + } +} + +func countGitLabTaskOwnedRows(t *testing.T, store *Store, table, taskID string) int { + t.Helper() + var count int + if err := store.db.Get(&count, `SELECT COUNT(*) FROM `+table+` WHERE task_id = ?`, taskID); err != nil { + t.Fatalf("count %s for %s: %v", table, taskID, err) + } + return count +} + +// countTaskMRRows counts gitlab_task_mrs rows directly, bypassing +// ListTaskMRsByTask so a test can observe rows for task IDs the normal read +// path would refuse (empty string, injection payloads). +func countTaskMRRows(t *testing.T, store *Store) int { + t.Helper() + var n int + if err := store.db.Get(&n, `SELECT COUNT(*) FROM gitlab_task_mrs`); err != nil { + t.Fatalf("count gitlab_task_mrs: %v", err) + } + return n +} + +// TestDeleteTaskMRsByTaskIDIsParameterized proves the task-scoped delete binds +// its task ID rather than interpolating it. A task_id carrying SQL +// metacharacters must delete exactly its own row and leave the table intact. +func TestDeleteTaskMRsByTaskIDIsParameterized(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + seedWorkspace(t, store, "ws") + + const injection = `x'; DROP TABLE gitlab_task_mrs; --` + seedTask(t, store, injection, "ws") + seedTask(t, store, "innocent", "ws") + if err := store.UpsertTaskMR(ctx, newTestMR(injection, "", "group/project", 1)); err != nil { + t.Fatalf("create injection task MR: %v", err) + } + if err := store.UpsertTaskMR(ctx, newTestMR("innocent", "", "group/project", 2)); err != nil { + t.Fatalf("create innocent task MR: %v", err) + } + + n, err := store.DeleteTaskMRsByTaskID(ctx, injection) + if err != nil { + t.Fatalf("delete by injection task id: %v", err) + } + if n != 1 { + t.Fatalf("deleted = %d, want 1", n) + } + // The table must still exist and still hold the unrelated row. + if got := countTaskMRRows(t, store); got != 1 { + t.Fatalf("remaining rows = %d, want 1 (table intact, innocent row kept)", got) + } + if got, _ := store.ListTaskMRsByTask(ctx, "innocent"); len(got) != 1 { + t.Fatalf("innocent task MRs = %d, want 1", len(got)) + } +} + +// TestHandleTaskDeletedSurvivesStoreFailure covers the dependency-failure path: +// when the underlying database is unusable the handler must log and return nil +// rather than panicking or propagating an error that would poison the shared +// event bus for other subscribers. +func TestHandleTaskDeletedSurvivesStoreFailure(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + seedWorkspace(t, store, "ws") + seedTask(t, store, "t1", "ws") + if err := store.UpsertTaskMR(ctx, newTestMR("t1", "", "group/project", 1)); err != nil { + t.Fatalf("create task MR: %v", err) + } + + log, _ := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "console"}) + svc := NewService(DefaultHost, NewMockClient(DefaultHost), "", nil, log) + svc.SetStore(store) + + // Simulate the dependency failing underneath a live subscription. + if err := store.db.Close(); err != nil { + t.Fatalf("close db: %v", err) + } + + event := bus.NewEvent(events.TaskDeleted, "task-service", map[string]interface{}{"task_id": "t1"}) + if err := svc.handleTaskDeleted(ctx, event); err != nil { + t.Fatalf("handler must swallow store failures, got: %v", err) + } +} + +// TestHandleTaskDeletedConcurrentDeliveryIsIdempotent fires the same deletion +// event from many goroutines at once, the shape a redelivered or duplicated +// bus message takes. Run under -race it also guards the store/service locking +// around s.store. +func TestHandleTaskDeletedConcurrentDeliveryIsIdempotent(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + seedWorkspace(t, store, "ws") + seedTask(t, store, "doomed", "ws") + seedTask(t, store, "bystander", "ws") + for _, taskID := range []string{"doomed", "bystander"} { + if err := store.UpsertTaskMR(ctx, newTestMR(taskID, "", "group/project", 1)); err != nil { + t.Fatalf("create task MR for %s: %v", taskID, err) + } + if err := store.CreateMRWatch(ctx, &MRWatch{ + SessionID: "session-" + taskID, TaskID: taskID, ProjectPath: "group/project", Branch: "main", + }); err != nil { + t.Fatalf("create MR watch for %s: %v", taskID, err) + } + } + + log, _ := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "console"}) + svc := NewService(DefaultHost, NewMockClient(DefaultHost), "", nil, log) + svc.SetStore(store) + + event := bus.NewEvent(events.TaskDeleted, "task-service", map[string]interface{}{"task_id": "doomed"}) + var wg sync.WaitGroup + errs := make(chan error, 16) + for range 16 { + wg.Add(1) + go func() { + defer wg.Done() + if err := svc.handleTaskDeleted(ctx, event); err != nil { + errs <- err + } + }() + } + wg.Wait() + close(errs) + for err := range errs { + t.Fatalf("concurrent delivery returned an error: %v", err) + } + + if got, _ := store.ListTaskMRsByTask(ctx, "doomed"); len(got) != 0 { + t.Fatalf("doomed task MRs = %d, want 0", len(got)) + } + if got, _ := store.ListTaskMRsByTask(ctx, "bystander"); len(got) != 1 { + t.Fatalf("bystander task MRs = %d, want 1", len(got)) + } + if got, _ := store.GetMRWatchBySession(ctx, "session-doomed"); got != nil { + t.Fatal("doomed watch survived the delete") + } + if got, _ := store.GetMRWatchBySession(ctx, "session-bystander"); got == nil { + t.Fatal("bystander watch was removed by a concurrent delete of another task") + } +} + +func TestHandleTaskDeletedRemovesAllTaskOwnedState(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + seedWorkspace(t, store, "ws") + + for _, taskID := range []string{"doomed", "bystander"} { + seedTask(t, store, taskID, "ws") + if err := store.UpsertTaskMR(ctx, newTestMR(taskID, "", "group/project", 1)); err != nil { + t.Fatalf("create task MR for %s: %v", taskID, err) + } + if err := store.CreateMRWatch(ctx, &MRWatch{ + SessionID: "session-" + taskID, TaskID: taskID, ProjectPath: "group/project", Branch: "main", + }); err != nil { + t.Fatalf("create MR watch for %s: %v", taskID, err) + } + seedGitLabTaskOwnedAutomationState(t, store, taskID) + } + + log, _ := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "console"}) + svc := NewService(DefaultHost, NewMockClient(DefaultHost), "", nil, log) + svc.SetStore(store) + event := bus.NewEvent(events.TaskDeleted, "task-service", map[string]interface{}{"task_id": "doomed"}) + if err := svc.handleTaskDeleted(ctx, event); err != nil { + t.Fatalf("handle task deleted: %v", err) + } + + for _, table := range testGitLabTaskOwnedTables { + if got := countGitLabTaskOwnedRows(t, store, table, "doomed"); got != 0 { + t.Fatalf("%s rows for doomed task = %d, want 0", table, got) + } + if got := countGitLabTaskOwnedRows(t, store, table, "bystander"); got != 1 { + t.Fatalf("%s rows for bystander task = %d, want 1", table, got) + } + } +} + +// TestHealTaskOwnedOrphansHandlesUnusualTaskIDs pins how the startup +// sweep treats rows whose task_id can never match a tasks row: the empty +// string, and a very long unicode identifier. Both are ownerless by +// definition, so both must be swept, while a real task's rows survive. +func TestHealTaskOwnedOrphansHandlesUnusualTaskIDs(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + seedWorkspace(t, store, "ws") + seedTask(t, store, "real-task", "ws") + + longUnicode := strings.Repeat("๐Ÿ› ๏ธไปปๅ‹™", 200) + for i, taskID := range []string{"real-task", "", longUnicode} { + if err := store.UpsertTaskMR(ctx, newTestMR(taskID, "", "group/project", i+1)); err != nil { + t.Fatalf("create task MR %d: %v", i, err) + } + } + if got := countTaskMRRows(t, store); got != 3 { + t.Fatalf("seeded rows = %d, want 3", got) + } + + if _, err := NewStore(store.db, store.ro); err != nil { + t.Fatalf("reopen store: %v", err) + } + + if got := countTaskMRRows(t, store); got != 1 { + t.Fatalf("rows after sweep = %d, want 1 (only the owned row survives)", got) + } + if got, _ := store.ListTaskMRsByTask(ctx, "real-task"); len(got) != 1 { + t.Fatalf("owned task MRs = %d, want 1", len(got)) + } +} diff --git a/apps/backend/internal/gitlab/store_task_cleanup_test.go b/apps/backend/internal/gitlab/store_task_cleanup_test.go new file mode 100644 index 0000000000..46b0527911 --- /dev/null +++ b/apps/backend/internal/gitlab/store_task_cleanup_test.go @@ -0,0 +1,105 @@ +package gitlab + +import ( + "context" + "testing" +) + +func TestDeleteTaskMRsByTaskIDRemovesOnlyTargetRows(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + seedWorkspace(t, store, "ws") + for _, taskID := range []string{"delete-me", "keep-me"} { + seedTask(t, store, taskID, "ws") + if err := store.UpsertTaskMR(ctx, newTestMR(taskID, "", "group/project", 1)); err != nil { + t.Fatalf("create task MR: %v", err) + } + if err := store.CreateMRWatch(ctx, &MRWatch{SessionID: "session-" + taskID, TaskID: taskID, ProjectPath: "group/project", Branch: "main"}); err != nil { + t.Fatalf("create MR watch: %v", err) + } + } + + if _, err := store.DeleteMRWatchesByTaskID(ctx, "delete-me"); err != nil { + t.Fatalf("delete watches: %v", err) + } + if _, err := store.DeleteTaskMRsByTaskID(ctx, "delete-me"); err != nil { + t.Fatalf("delete task MRs: %v", err) + } + + if got, err := store.ListTaskMRsByTask(ctx, "delete-me"); err != nil || len(got) != 0 { + t.Fatalf("target MRs = %d, %v; want 0", len(got), err) + } + if got, err := store.ListTaskMRsByTask(ctx, "keep-me"); err != nil || len(got) != 1 { + t.Fatalf("other MRs = %d, %v; want 1", len(got), err) + } + if got, _ := store.GetMRWatchBySession(ctx, "session-keep-me"); got == nil { + t.Fatal("unrelated watch was removed") + } +} + +// TestHealTaskOwnedOrphansOnStoreInit covers AC5/AC6: store +// initialization deletes gitlab_task_mrs/gitlab_mr_watches rows whose task_id +// has no matching tasks.id, preserves rows for active and archived tasks, and +// a repeat initialization is a no-op. +func TestHealTaskOwnedOrphansOnStoreInit(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + seedWorkspace(t, store, "ws") + + seedTask(t, store, "active-task", "ws") + seedTask(t, store, "archived-task", "ws") + if _, err := store.db.Exec(`UPDATE tasks SET archived_at = CURRENT_TIMESTAMP WHERE id = 'archived-task'`); err != nil { + t.Fatalf("archive task: %v", err) + } + // "orphan-task" is deliberately never inserted into tasks. + + for i, taskID := range []string{"active-task", "archived-task", "orphan-task"} { + if err := store.UpsertTaskMR(ctx, newTestMR(taskID, "", "group/project", i+1)); err != nil { + t.Fatalf("create task MR for %s: %v", taskID, err) + } + if err := store.CreateMRWatch(ctx, &MRWatch{SessionID: "session-" + taskID, TaskID: taskID, ProjectPath: "group/project", Branch: "main"}); err != nil { + t.Fatalf("create MR watch for %s: %v", taskID, err) + } + if taskID != "orphan-task" { + seedGitLabTaskOwnedAutomationState(t, store, taskID) + } + } + + reopened, err := NewStore(store.db, store.ro) + if err != nil { + t.Fatalf("reopen store: %v", err) + } + for _, taskID := range []string{"active-task", "archived-task"} { + if got, err := reopened.ListTaskMRsByTask(ctx, taskID); err != nil || len(got) != 1 { + t.Fatalf("MRs for %s = %d, %v; want 1", taskID, len(got), err) + } + if got, _ := reopened.GetMRWatchBySession(ctx, "session-"+taskID); got == nil { + t.Fatalf("watch for %s was removed, want preserved", taskID) + } + for _, table := range testGitLabTaskOwnedTables { + if got := countGitLabTaskOwnedRows(t, reopened, table, taskID); got != 1 { + t.Fatalf("%s rows for %s = %d, want 1", table, taskID, got) + } + } + } + if got, err := reopened.ListTaskMRsByTask(ctx, "orphan-task"); err != nil || len(got) != 0 { + t.Fatalf("orphan MRs = %d, %v; want 0", len(got), err) + } + if got, _ := reopened.GetMRWatchBySession(ctx, "session-orphan-task"); got != nil { + t.Fatal("orphan watch was not removed") + } + for _, table := range testGitLabTaskOwnedTables { + if got := countGitLabTaskOwnedRows(t, reopened, table, "orphan-task"); got != 0 { + t.Fatalf("orphan %s rows = %d, want 0", table, got) + } + } + + if _, err := NewStore(reopened.db, reopened.ro); err != nil { + t.Fatalf("replay store init: %v", err) + } + for _, taskID := range []string{"active-task", "archived-task"} { + if got, err := reopened.ListTaskMRsByTask(ctx, taskID); err != nil || len(got) != 1 { + t.Fatalf("MRs for %s after replay = %d, %v; want 1", taskID, len(got), err) + } + } +} diff --git a/docs/decisions/2026-08-13-hard-delete-task-contribution-links.md b/docs/decisions/2026-08-13-hard-delete-task-contribution-links.md new file mode 100644 index 0000000000..3c64c4fd2f --- /dev/null +++ b/docs/decisions/2026-08-13-hard-delete-task-contribution-links.md @@ -0,0 +1,21 @@ +# ADR-2026-08-13-hard-delete-task-contribution-links: Hard delete owns task contribution links + +**Status:** accepted +**Date:** 2026-08-13 +**Area:** backend + +## Context + +GitHub and GitLab contribution tables associate a pull request or merge request with a task, but their existing SQLite schemas do not enforce that ownership with a foreign key. Keeping those rows after a hard task delete produces inaccessible orphaned state, while archive remains the recoverable task lifecycle. + +## Decision + +Task contribution associations and their refresh watches survive backend restart and archive/unarchive. A hard `task.deleted` event removes the GitHub and GitLab rows by task ID, and provider-store initialization removes historical rows without a matching `tasks.id`. + +## Consequences + +Provider cleanup is idempotent and repairs missed event delivery on the next startup without rebuilding existing SQLite tables. A future contribution-history feature must introduce an explicitly owned history model rather than relying on deleted task associations. + +## Alternatives Considered + +Rebuilding the association tables with foreign keys would make deletion atomic but has unnecessary migration risk. Retaining rows as implicit history was rejected because the rows have no first-class history owner or usable task surface. diff --git a/docs/decisions/INDEX.md b/docs/decisions/INDEX.md index 19853089bc..1088e19432 100644 --- a/docs/decisions/INDEX.md +++ b/docs/decisions/INDEX.md @@ -166,3 +166,4 @@ Read individual ADRs for full context. Create new ones via `/record decision` or | 2026-08-12-empty-utility-bindings-inherit-default | [Empty Built-in Utility Bindings Inherit the Default](2026-08-12-empty-utility-bindings-inherit-default.md) | accepted | backend, frontend | 2026-08-12 | | 2026-08-12-setup-timeout-owns-launch-budget | [One Setup Timeout Owns Launch Budgets](2026-08-12-setup-timeout-owns-launch-budget.md) | accepted | backend | 2026-08-12 | | 2026-08-10-no-em-dash-public-copy | [Keep Em Dashes Out of Public Copy](2026-08-10-no-em-dash-public-copy.md) | accepted | frontend, infra | 2026-08-10 | +| 2026-08-13-hard-delete-task-contribution-links | [Hard delete owns task contribution links](2026-08-13-hard-delete-task-contribution-links.md) | accepted | backend | 2026-08-13 | diff --git a/docs/specs/gitlab-integration/spec.md b/docs/specs/gitlab-integration/spec.md index a743d96679..3395df44cc 100644 --- a/docs/specs/gitlab-integration/spec.md +++ b/docs/specs/gitlab-integration/spec.md @@ -165,6 +165,8 @@ into the secret store. - `gitlab_task_mrs` remains the durable task-to-MR association. Its unique key is `(task_id, repository_id, project_path, mr_iid)`; `task_id` and a non-empty `repository_id` must belong to the same workspace as the resolved connection. + Associations survive restart and archive/unarchive, but hard task deletion + removes them and their refresh watches. Decision: ADR-2026-08-13-hard-delete-task-contribution-links. - `gitlab_review_watches` and `gitlab_issue_watches` are workspace-owned durable automation definitions. Their workflow, workflow step, repository (when present), agent profile, and executor profile must belong to that workspace. @@ -421,6 +423,8 @@ protocol action name for compatibility. - Workspace config rows, PAT secrets, watch definitions, dedup reservations, task-to-MR associations, and last known MR status survive backend restarts. +- Archived tasks retain task-to-MR associations for later unarchive; hard task + deletion removes task-owned associations and refresh watches. - The startup migration moves the legacy global host/token to the active workspace, or the earliest-created workspace when no active workspace is available. It is idempotent and never duplicates automation watches. diff --git a/docs/specs/ui/ci-pr-automation.md b/docs/specs/ui/ci-pr-automation.md index 17a171c742..b4f2e65a24 100644 --- a/docs/specs/ui/ci-pr-automation.md +++ b/docs/specs/ui/ci-pr-automation.md @@ -16,6 +16,10 @@ close. Users and task agents need automation controls that keep a linked PR moving throughout its lifecycle, configured independently per linked PR so a task with several open PRs does not force the same setting onto all of them. +Task-to-PR associations survive restarts and archive/unarchive. Hard deletion +removes task-owned associations and refresh watches; it is not contribution +history. Decision: ADR-2026-08-13-hard-delete-task-contribution-links. + Decision: [ADR-0051](../../decisions/0051-pr-agent-notifications-extend-task-pr-automation.md) (the task-level control plane for the five switches was superseded by per-PR scoping; see that ADR's Consequences section).