fix(gitlab): delete task-owned MR/PR associations on hard task delete (#2655)

Co-authored-by: ayattara <alassane.yattara@savoirfairelinux.com>
Co-authored-by: Carlos Florencio <carlosmflorencio@gmail.com>
This commit is contained in:
Alassane
2026-08-15 12:21:14 -04:00
committed by GitHub
parent 39e9bc77f5
commit 49177d742f
27 changed files with 1264 additions and 16 deletions
@@ -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)
}
+3
View File
@@ -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),
+6 -5
View File
@@ -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.
@@ -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
@@ -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 {
@@ -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")
}
}
@@ -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) {
+3
View File
@@ -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)
}
@@ -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,
@@ -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)
}
}
@@ -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
}
@@ -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)
}
}
@@ -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)
}
}
}
@@ -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",
+4 -1
View File
@@ -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
}
+41 -2
View File
@@ -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.
@@ -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
}
@@ -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)
}
}
+4 -1
View File
@@ -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
@@ -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)
@@ -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
}
@@ -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))
}
}
@@ -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)
}
}
}
@@ -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.
+1
View File
@@ -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 |
+4
View File
@@ -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.
+4
View File
@@ -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).