From abc05be59ef2c511eca39d6a48c1dbbb9151809d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Sun, 9 Aug 2026 17:37:48 +0100 Subject: [PATCH] fix(task): recover legacy worktree ownership cutover (#2463) --- .../sqlite/worktree_ownership_migration.go | 19 +-- .../worktree_ownership_migration_test.go | 119 ++++++++++++++-- .../sqlite/worktree_ownership_normalize.go | 78 +++++++--- .../sqlite/worktree_ownership_targets.go | 27 +++- .../plan.md | 134 ++++++++++++++++++ .../task-01-cutover-repair.md | 76 ++++++++++ .../task-02-upgrade-docs.md | 55 +++++++ docs/public/k8s.md | 6 + docs/public/operations.md | 6 + .../session-delete-resource-cleanup/spec.md | 26 +++- 10 files changed, 505 insertions(+), 41 deletions(-) create mode 100644 docs/plans/task-owned-worktree-cutover-recovery/plan.md create mode 100644 docs/plans/task-owned-worktree-cutover-recovery/task-01-cutover-repair.md create mode 100644 docs/plans/task-owned-worktree-cutover-recovery/task-02-upgrade-docs.md diff --git a/apps/backend/internal/task/repository/sqlite/worktree_ownership_migration.go b/apps/backend/internal/task/repository/sqlite/worktree_ownership_migration.go index c67ab09260..95c022270c 100644 --- a/apps/backend/internal/task/repository/sqlite/worktree_ownership_migration.go +++ b/apps/backend/internal/task/repository/sqlite/worktree_ownership_migration.go @@ -111,15 +111,16 @@ func (r *Repository) normalizeTaskWorktreeOwnership() error { return err } cut := &worktreeCutover{ - envs: make(map[string]*legacyEnv), - taskEnvs: make(map[string][]*legacyEnv), - sessions: make(map[string]*legacySession), - sessionTasks: make(map[string]string), - sessionEnvIDs: make(map[string]string), - tasks: make(map[string]*taskWorktreeTargets), - taskEnvIDs: make(map[string]string), - loserEnvIDs: make(map[string]bool), - executorTypes: make(map[string]string), + envs: make(map[string]*legacyEnv), + taskEnvs: make(map[string][]*legacyEnv), + sessions: make(map[string]*legacySession), + sessionTasks: make(map[string]string), + sessionEnvIDs: make(map[string]string), + historicalWorktreeSuperseded: make(map[string]bool), + tasks: make(map[string]*taskWorktreeTargets), + taskEnvIDs: make(map[string]string), + loserEnvIDs: make(map[string]bool), + executorTypes: make(map[string]string), } if err := cut.loadLegacy(tx); err != nil { return err diff --git a/apps/backend/internal/task/repository/sqlite/worktree_ownership_migration_test.go b/apps/backend/internal/task/repository/sqlite/worktree_ownership_migration_test.go index b7f9a0daad..ea68229d77 100644 --- a/apps/backend/internal/task/repository/sqlite/worktree_ownership_migration_test.go +++ b/apps/backend/internal/task/repository/sqlite/worktree_ownership_migration_test.go @@ -157,6 +157,19 @@ func seedLegacyFlatEnv(t *testing.T, db *sqlx.DB, seed legacySeed, worktreeID, p } } +func seedLegacyEnvRepo(t *testing.T, db *sqlx.DB, id, envID, repoID, worktreeID, path, branch string, startedAt time.Time) { + t.Helper() + if _, err := db.Exec(db.Rebind(` + INSERT INTO task_environment_repos ( + id, task_environment_id, repository_id, branch_slug, + worktree_id, worktree_path, worktree_branch, position, + error_message, created_at, updated_at + ) VALUES (?, ?, ?, '', ?, ?, ?, 0, '', ?, ?)`), + id, envID, repoID, worktreeID, path, branch, startedAt, startedAt); err != nil { + t.Fatalf("seed env repo: %v", err) + } +} + func seedCleanupJob(t *testing.T, db *sqlx.DB, id, taskID, trigger, state string, startedAt time.Time) { t.Helper() if _, err := db.Exec(db.Rebind(` @@ -457,7 +470,7 @@ func TestCutover_ConflictingWorktreesFailClosed(t *testing.T) { for _, s := range []string{"sess-c1", "sess-c2"} { if _, err := db.Exec(` INSERT INTO task_sessions (id, task_id, state, started_at, updated_at) - VALUES (?, 'task-conflict', 'COMPLETED', ?, ?)`, s, now, now); err != nil { + VALUES (?, 'task-conflict', 'RUNNING', ?, ?)`, s, now, now); err != nil { t.Fatalf("seed session: %v", err) } } @@ -531,19 +544,109 @@ func TestCutover_RollbackAtEveryFailpoint(t *testing.T) { } } -// TestCutover_AmbiguousPathMismatchFailsClosed proves that two sources -// disagreeing on a worktree's path abort the cutover with a diagnostic. -func TestCutover_AmbiguousPathMismatchFailsClosed(t *testing.T) { +// TestCutover_CanonicalRepoWinsOverStaleFlatMetadata proves that canonical +// repository metadata wins when the deprecated flat environment fields drift. +func TestCutover_CanonicalRepoWinsOverStaleFlatMetadata(t *testing.T) { db := openLegacyDB(t) now := time.Now().UTC().Truncate(time.Second) seed := legacySeed{envID: "env-p", taskID: "task-p", repoID: "repo-p", sessionID: "sess-p"} seedLegacyTask(t, db, seed, now) seedLegacySessionWorktree(t, db, "sess-p", "wt-p", "repo-p", "", "/t/one", "feature/p", "active", now) - seedLegacyFlatEnv(t, db, seed, "wt-p", "/t/two", "feature/p", now) + seedLegacyFlatEnv(t, db, seed, "wt-p", "/t/two", "feature/stale", now) + seedLegacyEnvRepo(t, db, "env-repo-p", "env-p", "repo-p", "wt-p", "/t/one", "feature/p", now) - _, err := NewWithDB(db, db, nil) - if err == nil { - t.Fatal("expected cutover to fail on path mismatch") + repo, err := NewWithDB(db, db, nil) + if err != nil { + t.Fatalf("cutover: %v", err) + } + env, err := repo.GetTaskEnvironment(context.Background(), "env-p") + if err != nil { + t.Fatalf("get task environment: %v", err) + } + if len(env.Repos) != 1 || env.Repos[0].WorktreePath != "/t/one" || + env.Repos[0].WorktreeBranch != "feature/p" { + t.Fatalf("canonical repository metadata = %+v", env.Repos) + } +} + +// TestCutover_IgnoresDeletedHistoricalSessionConflict proves that a deleted +// session reference cannot override an existing task-owned repository row. +func TestCutover_IgnoresDeletedHistoricalSessionConflict(t *testing.T) { + db := openLegacyDB(t) + now := time.Now().UTC().Truncate(time.Second) + seed := legacySeed{envID: "env-history", taskID: "task-history", repoID: "repo-history", sessionID: "sess-history"} + seedLegacyTask(t, db, seed, now) + seedLegacyFlatEnv(t, db, seed, "wt-current", "/tasks/history/repo", "feature/current", now) + seedLegacyEnvRepo(t, db, "env-repo-history", "env-history", "repo-history", "wt-current", "/tasks/history/repo", "feature/current", now) + seedLegacySessionWorktree(t, db, "sess-history", "wt-deleted", "repo-history", "", "/tasks/history/old", "feature/old", "deleted", now) + + repo, err := NewWithDB(db, db, nil) + if err != nil { + t.Fatalf("cutover: %v", err) + } + env, err := repo.GetTaskEnvironment(context.Background(), "env-history") + if err != nil { + t.Fatalf("get task environment: %v", err) + } + if len(env.Repos) != 1 || env.Repos[0].WorktreeID != "wt-current" { + t.Fatalf("canonical repository row = %+v", env.Repos) + } +} + +// TestCutover_IgnoresTerminalHistoricalSessionConflict proves that a +// completed, failed, or cancelled session cannot override a canonical owner. +func TestCutover_IgnoresTerminalHistoricalSessionConflict(t *testing.T) { + for _, state := range []string{"COMPLETED", "FAILED", "CANCELLED"} { + t.Run(state, func(t *testing.T) { + db := openLegacyDB(t) + now := time.Now().UTC().Truncate(time.Second) + seed := legacySeed{envID: "env-terminal", taskID: "task-terminal", repoID: "repo-terminal", sessionID: "sess-terminal"} + seedLegacyTask(t, db, seed, now) + if _, err := db.Exec(db.Rebind(`UPDATE task_sessions SET state = ? WHERE id = ?`), state, seed.sessionID); err != nil { + t.Fatalf("set session state: %v", err) + } + seedLegacyFlatEnv(t, db, seed, "wt-current", "/tasks/terminal/repo", "feature/current", now) + seedLegacyEnvRepo(t, db, "env-repo-terminal", "env-terminal", "repo-terminal", "wt-current", "/tasks/terminal/repo", "feature/current", now) + seedLegacySessionWorktree(t, db, seed.sessionID, "wt-old", "repo-terminal", "", "/tasks/terminal/old", "feature/current", "active", now) + + repo, err := NewWithDB(db, db, nil) + if err != nil { + t.Fatalf("cutover: %v", err) + } + env, err := repo.GetTaskEnvironment(context.Background(), seed.envID) + if err != nil { + t.Fatalf("get task environment: %v", err) + } + if len(env.Repos) != 1 || env.Repos[0].WorktreeID != "wt-current" { + t.Fatalf("canonical repository row = %+v", env.Repos) + } + }) + } +} + +// TestCutover_PreservesCreatedSessionWorktreeWithoutCanonicalOwner proves +// that an unexpected CREATED row is backfilled instead of silently dropped. +func TestCutover_PreservesCreatedSessionWorktreeWithoutCanonicalOwner(t *testing.T) { + db := openLegacyDB(t) + now := time.Now().UTC().Truncate(time.Second) + seed := legacySeed{envID: "env-created", taskID: "task-created", repoID: "repo-created", sessionID: "sess-created"} + seedLegacyTask(t, db, seed, now) + if _, err := db.Exec(db.Rebind(`UPDATE task_sessions SET state = 'CREATED' WHERE id = ?`), seed.sessionID); err != nil { + t.Fatalf("set session state: %v", err) + } + seedLegacyFlatEnv(t, db, seed, "wt-created", "/tasks/created/repo", "feature/created", now) + seedLegacySessionWorktree(t, db, seed.sessionID, "wt-created", "repo-created", "", "/tasks/created/repo", "feature/created", "active", now) + + repo, err := NewWithDB(db, db, nil) + if err != nil { + t.Fatalf("cutover: %v", err) + } + env, err := repo.GetTaskEnvironment(context.Background(), seed.envID) + if err != nil { + t.Fatalf("get task environment: %v", err) + } + if len(env.Repos) != 1 || env.Repos[0].WorktreeID != "wt-created" { + t.Fatalf("created session worktree = %+v", env.Repos) } } diff --git a/apps/backend/internal/task/repository/sqlite/worktree_ownership_normalize.go b/apps/backend/internal/task/repository/sqlite/worktree_ownership_normalize.go index 4374e0272b..c3a10da7ee 100644 --- a/apps/backend/internal/task/repository/sqlite/worktree_ownership_normalize.go +++ b/apps/backend/internal/task/repository/sqlite/worktree_ownership_normalize.go @@ -14,18 +14,19 @@ import ( // worktreeCutover holds the legacy inventory and the normalized result. type worktreeCutover struct { - envs map[string]*legacyEnv - taskEnvs map[string][]*legacyEnv - sessions map[string]*legacySession - sessionTasks map[string]string - sessionEnvIDs map[string]string - envRepos []legacyEnvRepo - sessionWts []legacySessionWorktree - tasks map[string]*taskWorktreeTargets - taskEnvIDs map[string]string - loserEnvIDs map[string]bool - executorTypes map[string]string - conflicts []string + envs map[string]*legacyEnv + taskEnvs map[string][]*legacyEnv + sessions map[string]*legacySession + sessionTasks map[string]string + sessionEnvIDs map[string]string + historicalWorktreeSuperseded map[string]bool + envRepos []legacyEnvRepo + sessionWts []legacySessionWorktree + tasks map[string]*taskWorktreeTargets + taskEnvIDs map[string]string + loserEnvIDs map[string]bool + executorTypes map[string]string + conflicts []string } type legacyEnv struct { @@ -38,6 +39,7 @@ type legacyEnv struct { type legacySession struct { id, taskID, taskEnvironmentID string + state string startedAt time.Time executorID, executorProfileID string repositoryID string @@ -108,13 +110,13 @@ func (c *worktreeCutover) loadLegacySessions(tx *sqlx.Tx) error { // scanned as a string because MIN() makes the driver return the raw // TEXT value instead of a time. rows, err := tx.Query(` - SELECT ts.id, ts.task_id, COALESCE(ts.task_environment_id, ''), + SELECT ts.id, ts.task_id, COALESCE(ts.task_environment_id, ''), ts.state, COALESCE(ts.executor_id, ''), COALESCE(ts.executor_profile_id, ''), COALESCE(ts.repository_id, ''), COALESCE(er.container_id, ''), MIN(ts.started_at) FROM task_sessions ts LEFT JOIN executors_running er ON er.session_id = ts.id - GROUP BY ts.id, ts.task_id, ts.task_environment_id, ts.executor_id, + GROUP BY ts.id, ts.task_id, ts.task_environment_id, ts.state, ts.executor_id, ts.executor_profile_id, ts.repository_id, er.container_id`) if err != nil { return fmt.Errorf("cutover: read legacy sessions: %w", err) @@ -123,7 +125,7 @@ func (c *worktreeCutover) loadLegacySessions(tx *sqlx.Tx) error { for rows.Next() { s := &legacySession{} var startedAt string - if err := rows.Scan(&s.id, &s.taskID, &s.taskEnvironmentID, &s.executorID, + if err := rows.Scan(&s.id, &s.taskID, &s.taskEnvironmentID, &s.state, &s.executorID, &s.executorProfileID, &s.repositoryID, &s.containerID, &startedAt); err != nil { return fmt.Errorf("cutover: scan legacy session: %w", err) } @@ -325,13 +327,28 @@ func (c *worktreeCutover) mergeSessionWorktrees() { continue // already reported in load } targets := c.targetsForTask(session.taskID) - if err := targets.mergeSessionWorktree(wt); err != nil { + if err := targets.mergeSessionWorktree(wt, c.isSupersededHistoricalWorktree(wt)); err != nil { c.conflicts = append(c.conflicts, fmt.Sprintf( "session %s worktree %s: %v", wt.sessionID, wt.worktreeID, err)) } } } +func isLegacyDeletedWorktree(wt legacySessionWorktree) bool { + return wt.status == worktreeRepoStatusDeleted || wt.deletedAt != nil +} + +// isLegacyHistoricalSession reports whether a session no longer represents an +// open execution that can own a conflicting worktree during cutover. +func isLegacyHistoricalSession(state string) bool { + switch state { + case "COMPLETED", "FAILED", "CANCELLED": + return true + default: + return false + } +} + // bindTargetsToEnvironments binds every task's targets to its surviving // environment. func (c *worktreeCutover) bindTargetsToEnvironments() { @@ -438,6 +455,35 @@ func (c *worktreeCutover) linkSessions() { } } +// isSupersededHistoricalWorktree reports whether a legacy row is no longer an +// ownership source because a canonical repository row already represents the +// same task/repository slot. A historical row with no canonical replacement +// remains eligible for backfill. +func (c *worktreeCutover) isSupersededHistoricalWorktree(wt legacySessionWorktree) bool { + cacheKey := wt.sessionID + "\x00" + wt.worktreeID + if superseded, ok := c.historicalWorktreeSuperseded[cacheKey]; ok { + return superseded + } + + superseded := false + if isLegacyDeletedWorktree(wt) { + superseded = true + } else if session, ok := c.sessions[wt.sessionID]; ok && isLegacyHistoricalSession(session.state) { + for _, row := range c.envRepos { + env, ok := c.envs[row.envID] + if !ok || env.taskID != session.taskID || row.worktreeID == "" { + continue + } + if row.repositoryID == wt.repositoryID && row.branchSlug == wt.branchSlug { + superseded = true + break + } + } + } + c.historicalWorktreeSuperseded[cacheKey] = superseded + return superseded +} + // registerWorktreeIdentities ensures every physical worktree is owned by // exactly one environment-repository key. func (c *worktreeCutover) registerWorktreeIdentities() { diff --git a/apps/backend/internal/task/repository/sqlite/worktree_ownership_targets.go b/apps/backend/internal/task/repository/sqlite/worktree_ownership_targets.go index 8c23c04c2a..58a883d745 100644 --- a/apps/backend/internal/task/repository/sqlite/worktree_ownership_targets.go +++ b/apps/backend/internal/task/repository/sqlite/worktree_ownership_targets.go @@ -62,9 +62,9 @@ func (t *taskWorktreeTargets) mergeLegacyEnvRepo(row legacyEnvRepo) error { // environment. func (t *taskWorktreeTargets) mergeFlatEnv(env *legacyEnv) error { if existing := t.targetForWorktree(env.worktreeID); existing != nil { - if err := existing.verifyWorktree(env.worktreeID, env.worktreePath, env.worktreeBranch); err != nil { - return err - } + // task_environment_repos is the canonical source. The deprecated flat + // fields can retain the old task-root path or branch after a repository + // row has been updated, so never let them overwrite canonical metadata. if existing.status == "" { existing.status = worktreeRepoStatusActive } @@ -80,14 +80,20 @@ func (t *taskWorktreeTargets) mergeFlatEnv(env *legacyEnv) error { // mergeSessionWorktree folds a legacy session-worktree row into the task's // targets. Matching is by worktree identity first (legacy rows could carry an // empty repository_id), then by (repository, branch slug). -func (t *taskWorktreeTargets) mergeSessionWorktree(wt legacySessionWorktree) error { +func (t *taskWorktreeTargets) mergeSessionWorktree(wt legacySessionWorktree, historical bool) error { if wt.worktreeID == "" { return nil } if existing := t.targetForWorktree(wt.worktreeID); existing != nil { + if historical { + return nil + } return existing.verifyWorktree(wt.worktreeID, wt.worktreePath, wt.worktreeBranch) } target := t.rowForKey(wt.repositoryID, wt.branchSlug) + if historical && target.worktreeID != "" { + return nil + } status := wt.status if status == "" { status = worktreeRepoStatusActive @@ -262,6 +268,9 @@ func (r *Repository) validateCutover(c *worktreeCutover, tx *sqlx.Tx) error { // Every session that referenced a worktree must resolve to an // environment that owns that worktree. for _, wt := range c.sessionWts { + if c.isSupersededHistoricalWorktree(wt) { + continue + } envID := c.sessionEnvIDs[wt.sessionID] if envID == "" { return fmt.Errorf("cutover: session %s lost its environment", wt.sessionID) @@ -317,8 +326,14 @@ func (c *worktreeCutover) worktreeInventoryMismatch() string { // rows, and flat environment columns. func (c *worktreeCutover) legacyWorktreeInventory() map[string]bool { inventory := make(map[string]bool) + canonicalIDs := make(map[string]bool) + for _, row := range c.envRepos { + if row.worktreeID != "" { + canonicalIDs[row.worktreeID] = true + } + } for _, wt := range c.sessionWts { - if wt.worktreeID == "" || wt.status == worktreeRepoStatusDeleted || wt.deletedAt != nil { + if wt.worktreeID == "" || c.isSupersededHistoricalWorktree(wt) { continue } inventory[worktreeInventoryKey(wt.worktreeID, wt.worktreePath, wt.worktreeBranch)] = true @@ -330,7 +345,7 @@ func (c *worktreeCutover) legacyWorktreeInventory() map[string]bool { inventory[worktreeInventoryKey(row.worktreeID, row.worktreePath, row.worktreeBranch)] = true } for _, env := range c.envs { - if env.worktreeID == "" || c.loserEnvIDs[env.id] { + if env.worktreeID == "" || c.loserEnvIDs[env.id] || canonicalIDs[env.worktreeID] { continue } inventory[worktreeInventoryKey(env.worktreeID, env.worktreePath, env.worktreeBranch)] = true diff --git a/docs/plans/task-owned-worktree-cutover-recovery/plan.md b/docs/plans/task-owned-worktree-cutover-recovery/plan.md new file mode 100644 index 0000000000..552b5e26f6 --- /dev/null +++ b/docs/plans/task-owned-worktree-cutover-recovery/plan.md @@ -0,0 +1,134 @@ +--- +spec: docs/specs/session-delete-resource-cleanup/spec.md +created: 2026-08-09 +status: implemented +--- + +# Fix Plan: Recover Task-Worktree Cutovers Without Blocking Users + +## Overview + +Repair the one-time task-owned-worktree cutover introduced by PR #2456. The +current migration treats historical session references and stale duplicate +representations as live ownership conflicts, so valid legacy databases refuse +startup. The repair keeps canonical `task_environment_repos` ownership, +ignores obsolete terminal/deleted references, preserves fail-closed behavior for +unresolved live ownership, and adds an upgrade regression fixture matching the +affected database. + +Confirmed root cause: `loadLegacySessionWorktrees` loads deleted history and +`mergeSessionWorktree` reconciles it as an owner, while `mergeFlatEnv` rejects +path/branch drift even when the canonical repository row already owns the same +physical worktree. The failure occurs before the schema swap, so the existing +database is recoverable without destructive repair. + +## Backend + +### Migration source precedence + +- Update `apps/backend/internal/task/repository/sqlite/worktree_ownership_normalize.go` + and `worktree_ownership_targets.go` so deleted session rows and stale rows + belonging to terminal sessions cannot create or override ownership when a + canonical repository row exists. +- Preserve the canonical repository row's path and branch when the legacy flat + environment carries the same physical worktree identity with stale metadata. +- Continue returning a diagnostic for non-terminal sessions or worktrees with + no compatible canonical owner; do not silently choose between two live + physical worktrees. + +### Upgrade safety + +- Keep the transaction, pre-upgrade snapshot, shadow-table validation, and + rollback behavior unchanged. +- Ensure the repaired migration remains replay-safe after the legacy table is + removed and does not touch filesystem or Git state. + +## Tests + +- **What:** A deleted historical session row beside a canonical repository row + does not block cutover. + **File:** `apps/backend/internal/task/repository/sqlite/worktree_ownership_migration_test.go`. + **How:** Seed a legacy database with a canonical row, a terminal session, + and a deleted session-worktree row; assert final schema and canonical owner. +- **What:** A stale flat environment path/branch does not override the + canonical physical-worktree row. + **File:** same migration test file. + **How:** Seed matching worktree identity with differing legacy metadata and + assert the canonical path/branch survives. +- **What:** A live unresolved conflict still fails closed and leaves legacy + schema/data intact. + **File:** same migration test file. + **How:** Extend the existing conflicting-worktrees fixture with a + non-terminal session and assert rollback. +- **What:** Terminal session references for `COMPLETED`, `FAILED`, and + `CANCELLED` cannot override a canonical owner. + **File:** same migration test file. + **How:** Run the table-driven `TestCutover_IgnoresTerminalHistoricalSessionConflict` + fixture and assert the canonical worktree remains selected for each state. +- **What:** An unexpected `CREATED` session-worktree reference without a + canonical repository row is preserved for backfill. + **File:** same migration test file. + **How:** Run `TestCutover_PreservesCreatedSessionWorktreeWithoutCanonicalOwner` + and assert the physical worktree is present after cutover. +- **What:** The full repository initializer can upgrade a representative + legacy database. + **File:** same migration test file. + **How:** Run the package's focused cutover tests, then the backend repository + test target required by the task. + +## Public documentation + +Update `docs/public/operations.md` and `docs/public/k8s.md` only after the code +repair is implemented. Document that one initializer must reach health, that a +failed pre-cutover startup leaves the database authoritative, and that rollback +uses a matching pre-upgrade binary plus backup. Do not instruct operators to +delete ownership rows manually. + +## Implementation Waves And Parallel Candidates + +Wave 1 — sequential: + +- [x] [task-01-cutover-repair](task-01-cutover-repair.md) — done + +Wave 2 — sequential: + +- [x] [task-02-upgrade-docs](task-02-upgrade-docs.md) — done + +## State classification + +Use the existing session-state contract: `STARTING`, `RUNNING`, `IDLE`, and +`WAITING_FOR_INPUT` are open/resumable and must not be discarded as historical; +`COMPLETED`, `FAILED`, and `CANCELLED` are terminal for this migration. A +`CREATED` session should normally have no physical worktree reference, but the +test fixture must prove that an unexpected created-session reference is not +silently discarded without a canonical owner. + +## Verification Results + +- `GOCACHE=/tmp/kandev-go-cache go test ./internal/task/repository/sqlite -run + 'TestCutover_(CanonicalRepoWinsOverStaleFlatMetadata|IgnoresDeletedHistoricalSessionConflict)' -count=1` — passed. +- `GOCACHE=/tmp/kandev-go-cache go test ./internal/task/repository/sqlite -run + 'TestCutover_(IgnoresTerminalHistoricalSessionConflict|PreservesCreatedSessionWorktreeWithoutCanonicalOwner)' -count=1` — passed. +- `GOCACHE=/tmp/kandev-go-cache go test ./internal/task/repository/sqlite -run + 'TestCutover' -count=1` — passed. +- `GOCACHE=/tmp/kandev-go-cache go test ./internal/task/repository/sqlite -count=1` — passed. +- `make -C apps/backend test` — passed. +- `node scripts/validate-public-docs.mjs` — passed; 41 pages validated. +- `git diff --check` — passed. + +## PR Fixup Results + +- Remediation commit `44ffff81` added terminal-state coverage for + `COMPLETED`, `FAILED`, and `CANCELLED`, the unexpected `CREATED` safety + fixture, and cached historical ownership classification. +- Focused cutover tests passed after the remediation, and `git diff --check` + passed. +- The actionable review thread was replied to and resolved after the cache + fix; the exact-head unresolved thread count is zero. +- Exact-head PR state for `44ffff81`: checks snapshot complete, 4 passed, 0 + failed, 12 pending, 0 approval-required runs, 0 issue comments, and 0 + unresolved review threads. The PR is open and mergeable; GitHub reports + `UNSTABLE` only because checks are still running. +- Pending checks are Backend (Windows), Backend Postgres, Backend Static + Checks, Backend Tests (1/2), Backend Tests (2/2), five CodeQL analyses, + `deploy-same-repo`, and the Cloudflare Pages preview deployment. diff --git a/docs/plans/task-owned-worktree-cutover-recovery/task-01-cutover-repair.md b/docs/plans/task-owned-worktree-cutover-recovery/task-01-cutover-repair.md new file mode 100644 index 0000000000..9dc0530e77 --- /dev/null +++ b/docs/plans/task-owned-worktree-cutover-recovery/task-01-cutover-repair.md @@ -0,0 +1,76 @@ +--- +id: "01-cutover-repair" +title: "Repair task-worktree cutover precedence" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/session-delete-resource-cleanup/spec.md" +--- + +# Task 01: Repair task-worktree cutover precedence + +## Acceptance + +- Legacy deleted/terminal session history cannot block an upgrade when a + canonical `task_environment_repos` owner exists. +- Canonical repository path/branch metadata wins over stale flat environment + metadata for the same physical worktree identity. +- Unresolved conflicts for non-terminal/live ownership still fail closed and + roll back the legacy schema and data. +- An unexpected `CREATED` session-worktree reference without a canonical + repository row is preserved for backfill, not silently discarded. + +## Verification + +```bash +cd apps/backend && go test ./internal/task/repository/sqlite -run 'TestCutover' -count=1 +make -C apps/backend test +``` + +## Files likely touched + +- `apps/backend/internal/task/repository/sqlite/worktree_ownership_normalize.go` +- `apps/backend/internal/task/repository/sqlite/worktree_ownership_targets.go` +- `apps/backend/internal/task/repository/sqlite/worktree_ownership_migration_test.go` + +## Dependencies + +None. + +## Parallelism + +Sequential. The migration and its tests share schema and normalization state. + +## Inputs + +- Existing `TestCutover_ConflictingWorktreesFailClosed`. +- The affected database pattern: 20 conflicts, 554 deleted legacy session + rows, 96 active rows, and canonical repository rows already present. +- [Task-owned worktree lifetime decision](../../decisions/2026-08-08-task-owned-worktree-lifetime.md). + +## Output contract + +Report the source/test diff, exact test commands and results, any unresolved +terminal-state assumptions, and synchronized task/plan status. + +## Results + +- Added canonical-repository precedence for stale flat metadata. +- Historical deleted/terminal session references no longer override canonical + ownership; live conflicts still fail closed. +- Added terminal-state coverage for `COMPLETED`, `FAILED`, and `CANCELLED`, + plus a `CREATED` reference without a canonical repository owner. +- Source/test diff is limited to cutover precedence, historical-row caching, + and migration regression fixtures; no filesystem or Git state is changed. +- Terminal-state assumption: only `COMPLETED`, `FAILED`, and `CANCELLED` are + historical; `STARTING`, `RUNNING`, `IDLE`, `WAITING_FOR_INPUT`, and + unexpected `CREATED` rows remain eligible for ownership resolution. +- `GOCACHE=/tmp/kandev-go-cache go test ./internal/task/repository/sqlite -run + 'TestCutover_(CanonicalRepoWinsOverStaleFlatMetadata|IgnoresDeletedHistoricalSessionConflict)' -count=1` — passed. +- `GOCACHE=/tmp/kandev-go-cache go test ./internal/task/repository/sqlite -run + 'TestCutover' -count=1` — passed. +- `GOCACHE=/tmp/kandev-go-cache go test ./internal/task/repository/sqlite -count=1` — passed. +- `make -C apps/backend test` — passed. +- Plan status is `implemented`; Task 01 and Task 02 statuses are `done` and + synchronized with the recorded implementation and verification. diff --git a/docs/plans/task-owned-worktree-cutover-recovery/task-02-upgrade-docs.md b/docs/plans/task-owned-worktree-cutover-recovery/task-02-upgrade-docs.md new file mode 100644 index 0000000000..474fe161d6 --- /dev/null +++ b/docs/plans/task-owned-worktree-cutover-recovery/task-02-upgrade-docs.md @@ -0,0 +1,55 @@ +--- +id: "02-upgrade-docs" +title: "Document safe cutover recovery" +status: done +wave: 2 +depends_on: ["01-cutover-repair"] +plan: "plan.md" +spec: "../../specs/session-delete-resource-cleanup/spec.md" +--- + +# Task 02: Document safe cutover recovery + +## Acceptance + +- Public upgrade guidance explains single-initializer rollout and health + verification. +- Recovery guidance pairs a pre-upgrade database backup with a compatible + pre-cutover binary and explicitly forbids manual ownership-row deletion. +- SQLite and Kubernetes guidance remains consistent. + +## Verification + +```bash +node scripts/validate-public-docs.mjs +``` + +## Files likely touched + +- `docs/public/operations.md` +- `docs/public/k8s.md` + +## Dependencies + +Task 01. + +## Parallelism + +Sequential. Documentation must describe the implemented migration behavior. + +## Inputs + +- `docs/specs/session-delete-resource-cleanup/spec.md` +- `docs/plans/task-owned-worktree-cutover-recovery/plan.md` +- Existing cutover backup and rollback guidance. + +## Output contract + +Report changed public pages, validation command and result, and synchronized +task/plan status. + +## Results + +- Updated `docs/public/operations.md` and `docs/public/k8s.md` with + non-destructive cutover recovery guidance. +- `node scripts/validate-public-docs.mjs` — passed; 41 pages validated. diff --git a/docs/public/k8s.md b/docs/public/k8s.md index 589b0ca2cd..cd9e290bbd 100644 --- a/docs/public/k8s.md +++ b/docs/public/k8s.md @@ -252,6 +252,12 @@ This release includes a one-time task-worktree ownership schema cutover (see [Op - All writers stopped during the cutover — with PostgreSQL, do not run a mixed-version fleet across the upgrade; the cutover takes a database advisory lock and fails closed if it cannot serialize. - One successful schema initializer: keep the deployment at a single replica during startup, and check `kubectl rollout status` before scaling out. +If the initializer reports a worktree-ownership conflict, stop the rollout and +do not delete database rows. The transaction leaves the legacy schema intact. +Restore service with a compatible pre-cutover image, or deploy the migration +hotfix and retry against the unchanged PVC. Do not run an older image against a +database after the cutover has committed. + `kubectl rollout undo` changes the image, not the database schema. After the cutover the final schema is intentionally not readable by older binaries, so a binary downgrade requires restoring the matching pre-upgrade database backup — do not start an older image against the normalized database. Reapplying the checked-in `k8s/deployment.yaml` without the image transformation resets the image to `kandev:latest`, so keep your production customization in your own overlay or deployment repository. ## Remove while retaining data diff --git a/docs/public/operations.md b/docs/public/operations.md index 9b24b6abf4..9d35166fe1 100644 --- a/docs/public/operations.md +++ b/docs/public/operations.md @@ -234,6 +234,12 @@ Some releases perform a one-time schema cutover that rewrites ownership tables a 2. Stop all backend writers during the cutover. Do not run a mixed-version fleet across the upgrade; the migration takes a database advisory lock and aborts on lock timeout without changing the schema or data. 3. Let exactly one schema initializer run and reach a healthy state before starting additional instances. +If the new binary reports a worktree-ownership conflict and exits, stop the +rollout. The cutover is transactional, so the legacy database remains +authoritative. Do not delete ownership rows by hand. Start a compatible +pre-cutover binary to restore service, or deploy the migration hotfix and retry +the upgrade against the unchanged database. + The normalized schema is intentionally incompatible with older binaries. To downgrade, stop all instances and restore the pre-upgrade backup — never start an older binary against a post-cutover database. SQLite restores from its automatic pre-migration snapshot; PostgreSQL restores your verified `pg_dump` backup. Switching `database.driver` does not migrate data. PostgreSQL and shared NATS remove two single-process data constraints, but they do not make Kandev horizontally scalable: WebSocket subscriptions, execution lifecycle/control state, and task workspaces remain process- or filesystem-local. The current product and supplied deployment validate one backend replica only; do not add replicas based on the database and event bus alone. diff --git a/docs/specs/session-delete-resource-cleanup/spec.md b/docs/specs/session-delete-resource-cleanup/spec.md index b6f0c82ca4..c26402ac72 100644 --- a/docs/specs/session-delete-resource-cleanup/spec.md +++ b/docs/specs/session-delete-resource-cleanup/spec.md @@ -105,8 +105,15 @@ SQLite and PostgreSQL migrations backfill canonical rows from `task_session_worktrees`. Sessions with a valid `task_environment_id` retain it. Legacy sessions are assigned to the matching existing environment or to a normalized task-owned environment created for their connected worktree group. -Incompatible owners, paths, repository identities, or session groupings fail -closed with a diagnostic. +Canonical `task_environment_repos` rows take precedence when the legacy flat +environment fields or session references repeat the same physical worktree. +Legacy session rows marked `deleted` (or carrying `deleted_at`) and stale +references from terminal sessions are historical evidence, not additional +owners, and bypass validation only when a canonical repository owner exists. +A terminal-only reference without a canonical owner, a non-terminal session, +or a worktree that has no canonical owner still requires compatible identity, +path, branch, and repository data; unresolved ownership fails closed with a +diagnostic. After backfill validation, the same upgrade drops `task_session_worktrees` and the deprecated flat worktree columns from `task_environments`. It also removes @@ -201,6 +208,9 @@ remain the authorization boundary for physical cleanup. - If migration cannot determine a single compatible owner/path, startup fails closed, the transaction rolls back, and the pre-upgrade database remains authoritative instead of authorizing future deletion from ambiguous data. +- Historical deleted session-worktree rows and stale references from terminal + sessions do not block migration when a canonical task-owned repository row + exists; the canonical row remains authoritative. - If migration fails after shadow tables are populated or after legacy DDL has begun, the database transaction restores the complete legacy schema and data. - If SQLite cannot create its pre-upgrade snapshot, startup stops before the @@ -254,6 +264,18 @@ remain the authorization boundary for physical cleanup. - **GIVEN** session/worktree creation and task deletion begin concurrently, **WHEN** one reserves the task row first, **THEN** the resource is either fully included in cleanup or rejected and compensated; it cannot become untracked. +- **GIVEN** a legacy database contains deleted session-worktree history and a + canonical task-owned repository row for the same workspace, **WHEN** the new + binary starts, **THEN** the cutover completes, preserves the canonical row, + and removes the legacy schema without requiring manual database edits. +- **GIVEN** a legacy flat environment path differs from the canonical repository + row for the same physical worktree, **WHEN** the new binary starts, **THEN** + the canonical repository path and branch are retained and the cutover + completes. +- **GIVEN** a non-terminal session references a worktree that conflicts with + the canonical owner and cannot be reconciled, **WHEN** the new binary starts, + **THEN** startup fails closed, the transaction rolls back, and the verified + pre-upgrade backup remains the recovery source. ## Out of Scope