From f55e9c780243a801bb2152757c2c2f3b2a58c65e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Almeida?= <57680069+zeval@users.noreply.github.com> Date: Fri, 14 Aug 2026 09:33:15 +0100 Subject: [PATCH] fix(task): recover cutover from orphaned sessions (#2651) --- .../sqlite/worktree_ownership_normalize.go | 39 ++- ...orktree_ownership_orphan_migration_test.go | 227 ++++++++++++++++++ .../worktree_ownership_postgres_test.go | 52 ++++ .../plan.md | 167 +++++++++++++ ...-01-classify-orphaned-session-worktrees.md | 114 +++++++++ .../session-delete-resource-cleanup/spec.md | 38 +++ 6 files changed, 635 insertions(+), 2 deletions(-) create mode 100644 apps/backend/internal/task/repository/sqlite/worktree_ownership_orphan_migration_test.go create mode 100644 docs/plans/task-owned-worktree-cutover-orphaned-sessions/plan.md create mode 100644 docs/plans/task-owned-worktree-cutover-orphaned-sessions/task-01-classify-orphaned-session-worktrees.md 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 c406c7323b..ca19834491 100644 --- a/apps/backend/internal/task/repository/sqlite/worktree_ownership_normalize.go +++ b/apps/backend/internal/task/repository/sqlite/worktree_ownership_normalize.go @@ -32,6 +32,7 @@ type worktreeCutover struct { authoritativeWorktreeIDs map[string]bool envRepos []legacyEnvRepo sessionWts []legacySessionWorktree + orphanedSessionWts []legacySessionWorktree tasks map[string]*taskWorktreeTargets taskEnvIDs map[string]string loserEnvIDs map[string]bool @@ -249,8 +250,7 @@ func (c *worktreeCutover) loadLegacySessionWorktrees(tx *sqlx.Tx) error { row.deletedAt = &t } if _, ok := c.sessions[row.sessionID]; !ok { - c.conflicts = append(c.conflicts, fmt.Sprintf( - "task_session_worktrees row %s references missing session %s", row.worktreeID, row.sessionID)) + c.orphanedSessionWts = append(c.orphanedSessionWts, row) continue } c.sessionWts = append(c.sessionWts, row) @@ -289,6 +289,7 @@ func (c *worktreeCutover) normalize(tx *sqlx.Tx) error { c.pickSurvivingEnvironments() c.mergeLegacyEnvRepoRows() c.mergeFlatEnvironmentFields() + c.classifyOrphanedSessionWorktrees() if err := c.backfillMissingEnvironments(tx); err != nil { return err } @@ -308,6 +309,40 @@ func (c *worktreeCutover) normalize(tx *sqlx.Tx) error { return nil } +// classifyOrphanedSessionWorktrees rejects a legacy row whose missing session +// leaves no task or environment owner to recover. Deleted rows and rows whose +// physical identity already has a higher-precedence task-owned source are +// historical evidence and contribute no ownership or metadata. +func (c *worktreeCutover) classifyOrphanedSessionWorktrees() { + for _, wt := range c.orphanedSessionWts { + if isLegacyDeletedWorktree(wt) || c.hasTaskOwnedSourceForWorktree(wt.worktreeID) { + continue + } + c.conflicts = append(c.conflicts, fmt.Sprintf( + "task_session_worktrees row %s references missing session %s", wt.worktreeID, wt.sessionID)) + } +} + +// hasTaskOwnedSourceForWorktree reports whether an exact physical identity is +// represented by a non-deleted normalized target built from a canonical row +// or surviving flat environment. Path, branch, and repository metadata cannot +// recover the task relationship lost with the session. +func (c *worktreeCutover) hasTaskOwnedSourceForWorktree(worktreeID string) bool { + if worktreeID == "" { + return false + } + for _, targets := range c.tasks { + for _, key := range targets.ordering { + target := targets.byKey[key] + if target.worktreeID == worktreeID && target.status != worktreeRepoStatusDeleted && + target.deletedAt == nil { + return true + } + } + } + return false +} + // pickSurvivingEnvironments keeps the most recently updated environment per // task and re-points sessions that referenced a collapsed row. func (c *worktreeCutover) pickSurvivingEnvironments() { diff --git a/apps/backend/internal/task/repository/sqlite/worktree_ownership_orphan_migration_test.go b/apps/backend/internal/task/repository/sqlite/worktree_ownership_orphan_migration_test.go new file mode 100644 index 0000000000..392dc01912 --- /dev/null +++ b/apps/backend/internal/task/repository/sqlite/worktree_ownership_orphan_migration_test.go @@ -0,0 +1,227 @@ +package sqlite + +import ( + "context" + "strings" + "testing" + "time" + + "github.com/jmoiron/sqlx" +) + +// TestCutover_OrphanedSessionWorktreeUsesFlatOwner proves that a legacy row +// whose session was already deleted cannot override the surviving flat source +// for the same physical worktree. +func TestCutover_OrphanedSessionWorktreeUsesFlatOwner(t *testing.T) { + db := openLegacyDB(t) + now := time.Now().UTC().Truncate(time.Second) + seed := legacySeed{envID: "env-orphan-flat", taskID: "task-orphan-flat", repoID: "repo-orphan-flat", sessionID: "sess-orphan-flat"} + seedLegacyTask(t, db, seed, now) + seedLegacyFlatEnv(t, db, seed, "wt-orphan-flat", "/tasks/orphan-flat/current", "feature/current", now) + seedLegacySessionWorktree(t, db, seed.sessionID, "wt-orphan-flat", seed.repoID, "", + "/tasks/orphan-flat/stale", "feature/stale", "active", now) + deleteLegacySession(t, db, seed.sessionID) + + 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-orphan-flat" || + env.Repos[0].WorktreePath != "/tasks/orphan-flat/current" || + env.Repos[0].WorktreeBranch != "feature/current" { + t.Fatalf("flat environment owner = %+v", env.Repos) + } + if legacyTableExists(t, db, "task_session_worktrees") { + t.Fatal("task_session_worktrees must be dropped after recovering the orphan") + } +} + +// TestCutover_OrphanedSessionWorktreeUsesCanonicalOwner proves that a +// non-deleted canonical row remains authoritative over stale orphan metadata. +func TestCutover_OrphanedSessionWorktreeUsesCanonicalOwner(t *testing.T) { + db := openLegacyDB(t) + now := time.Now().UTC().Truncate(time.Second) + seed := legacySeed{envID: "env-orphan-canonical", taskID: "task-orphan-canonical", repoID: "repo-orphan-canonical", sessionID: "sess-orphan-canonical"} + seedLegacyTask(t, db, seed, now) + seedLegacyFlatEnv(t, db, seed, "", "", "", now) + seedLegacyEnvRepo(t, db, "env-repo-orphan-canonical", seed.envID, seed.repoID, + "wt-orphan-canonical", "/tasks/orphan-canonical/current", "feature/current", now) + seedLegacySessionWorktree(t, db, seed.sessionID, "wt-orphan-canonical", seed.repoID, "", + "/tasks/orphan-canonical/stale", "feature/stale", "active", now) + deleteLegacySession(t, db, seed.sessionID) + + 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-orphan-canonical" || + env.Repos[0].WorktreePath != "/tasks/orphan-canonical/current" || + env.Repos[0].WorktreeBranch != "feature/current" { + t.Fatalf("canonical environment owner = %+v", env.Repos) + } +} + +// TestCutover_DeletedOrphanedSessionWorktreeIsHistory proves that a deleted +// legacy row needs no replacement owner when its session is already gone. +func TestCutover_DeletedOrphanedSessionWorktreeIsHistory(t *testing.T) { + db := openLegacyDB(t) + now := time.Now().UTC().Truncate(time.Second) + seed := legacySeed{taskID: "task-orphan-deleted", sessionID: "sess-orphan-deleted"} + seedLegacyTask(t, db, seed, now) + seedLegacySessionWorktree(t, db, seed.sessionID, "wt-orphan-deleted", "repo-orphan-deleted", "", + "/tasks/orphan-deleted/old", "feature/old", "deleted", now) + deleteLegacySession(t, db, seed.sessionID) + + if _, err := NewWithDB(db, db, nil); err != nil { + t.Fatalf("cutover: %v", err) + } + if legacyTableExists(t, db, "task_session_worktrees") { + t.Fatal("task_session_worktrees must be dropped after discarding deleted history") + } + var repoRows int + if err := db.Get(&repoRows, `SELECT COUNT(*) FROM task_environment_repos`); err != nil { + t.Fatalf("count normalized repository rows: %v", err) + } + if repoRows != 0 { + t.Fatalf("normalized repository rows = %d, want 0", repoRows) + } +} + +// TestCutover_ActiveOrphanedSessionWorktreeFailsClosed proves that physical +// identity is required from a live task-owned source before an orphan is safe +// to discard. +func TestCutover_ActiveOrphanedSessionWorktreeFailsClosed(t *testing.T) { + t.Run("unique physical identity", func(t *testing.T) { + db := openLegacyDB(t) + now := time.Now().UTC().Truncate(time.Second) + seed := legacySeed{taskID: "task-orphan-unique", sessionID: "sess-orphan-unique"} + seedLegacyTask(t, db, seed, now) + seedLegacySessionWorktree(t, db, seed.sessionID, "wt-orphan-unique", "repo-orphan-unique", "", + "/tasks/orphan-unique/repo", "feature/unique", "active", now) + deleteLegacySession(t, db, seed.sessionID) + + assertOrphanCutoverFailsClosed(t, db, seed.sessionID, "wt-orphan-unique") + }) + + t.Run("empty physical identity", func(t *testing.T) { + db := openLegacyDB(t) + now := time.Now().UTC().Truncate(time.Second) + seed := legacySeed{taskID: "task-orphan-empty", sessionID: "sess-orphan-empty"} + seedLegacyTask(t, db, seed, now) + seedLegacySessionWorktree(t, db, seed.sessionID, "", "repo-orphan-empty", "", + "/tasks/orphan-empty/repo", "feature/empty", "active", now) + deleteLegacySession(t, db, seed.sessionID) + + assertOrphanCutoverFailsClosed(t, db, seed.sessionID, "") + }) + + t.Run("collapsed flat source", func(t *testing.T) { + db := openLegacyDB(t) + now := time.Now().UTC().Truncate(time.Second) + seed := legacySeed{envID: "env-orphan-winner", taskID: "task-orphan-loser", repoID: "repo-orphan-winner", sessionID: "sess-orphan-loser"} + seedLegacyTask(t, db, seed, now) + loser := seed + loser.envID = "env-orphan-loser" + loser.repoID = "repo-orphan-loser" + seedLegacyFlatEnv(t, db, loser, "wt-orphan-loser", "/tasks/orphan-loser/old", "feature/old", now.Add(-time.Hour)) + seedLegacyFlatEnv(t, db, seed, "wt-orphan-winner", "/tasks/orphan-loser/current", "feature/current", now) + seedLegacySessionWorktree(t, db, seed.sessionID, "wt-orphan-loser", loser.repoID, "", + "/tasks/orphan-loser/old", "feature/old", "active", now) + deleteLegacySession(t, db, seed.sessionID) + + assertOrphanCutoverFailsClosed(t, db, seed.sessionID, "wt-orphan-loser") + }) + + t.Run("deleted canonical source", func(t *testing.T) { + db := openLegacyDB(t) + now := time.Now().UTC().Truncate(time.Second) + seed := legacySeed{envID: "env-orphan-deleted-owner", taskID: "task-orphan-deleted-owner", repoID: "repo-orphan-deleted-owner", sessionID: "sess-orphan-deleted-owner"} + seedLegacyTask(t, db, seed, now) + seedLegacyFlatEnv(t, db, seed, "", "", "", now) + addLegacyRepoLifecycleColumns(t, db) + seedLegacyEnvRepo(t, db, "env-repo-orphan-deleted-owner", seed.envID, seed.repoID, + "wt-orphan-deleted-owner", "/tasks/orphan-deleted-owner/old", "feature/old", now) + if _, err := db.Exec(db.Rebind(` + UPDATE task_environment_repos + SET status = 'deleted', deleted_at = ? + WHERE id = 'env-repo-orphan-deleted-owner'`), now); err != nil { + t.Fatalf("mark canonical repository row deleted: %v", err) + } + seedLegacySessionWorktree(t, db, seed.sessionID, "wt-orphan-deleted-owner", seed.repoID, "", + "/tasks/orphan-deleted-owner/current", "feature/current", "active", now) + deleteLegacySession(t, db, seed.sessionID) + + assertOrphanCutoverFailsClosed(t, db, seed.sessionID, "wt-orphan-deleted-owner") + }) + + t.Run("flat source absorbed by deleted canonical", func(t *testing.T) { + db := openLegacyDB(t) + now := time.Now().UTC().Truncate(time.Second) + seed := legacySeed{envID: "env-orphan-shadowed-flat", taskID: "task-orphan-shadowed-flat", repoID: "repo-orphan-shadowed-flat", sessionID: "sess-orphan-shadowed-flat"} + seedLegacyTask(t, db, seed, now) + seedLegacyFlatEnv(t, db, seed, "wt-orphan-shadowed-flat", + "/tasks/orphan-shadowed-flat/current", "feature/current", now) + addLegacyRepoLifecycleColumns(t, db) + seedLegacyEnvRepo(t, db, "env-repo-orphan-shadowed-flat", seed.envID, seed.repoID, + "wt-orphan-shadowed-flat", "/tasks/orphan-shadowed-flat/old", "feature/old", now) + if _, err := db.Exec(db.Rebind(` + UPDATE task_environment_repos + SET status = 'deleted', deleted_at = ? + WHERE id = 'env-repo-orphan-shadowed-flat'`), now); err != nil { + t.Fatalf("mark canonical repository row deleted: %v", err) + } + seedLegacySessionWorktree(t, db, seed.sessionID, "wt-orphan-shadowed-flat", seed.repoID, "", + "/tasks/orphan-shadowed-flat/session", "feature/session", "active", now) + deleteLegacySession(t, db, seed.sessionID) + + assertOrphanCutoverFailsClosed(t, db, seed.sessionID, "wt-orphan-shadowed-flat") + }) +} + +func deleteLegacySession(t *testing.T, db *sqlx.DB, sessionID string) { + t.Helper() + if _, err := db.Exec(db.Rebind(`DELETE FROM task_sessions WHERE id = ?`), sessionID); err != nil { + t.Fatalf("delete legacy session: %v", err) + } +} + +func addLegacyRepoLifecycleColumns(t *testing.T, db *sqlx.DB) { + t.Helper() + for _, statement := range []string{ + `ALTER TABLE task_environment_repos ADD COLUMN status TEXT NOT NULL DEFAULT 'active'`, + `ALTER TABLE task_environment_repos ADD COLUMN merged_at TIMESTAMP`, + `ALTER TABLE task_environment_repos ADD COLUMN deleted_at TIMESTAMP`, + } { + if _, err := db.Exec(statement); err != nil { + t.Fatalf("add legacy repository lifecycle column: %v", err) + } + } +} + +func assertOrphanCutoverFailsClosed(t *testing.T, db *sqlx.DB, sessionID, worktreeID string) { + t.Helper() + if _, err := NewWithDB(db, db, nil); err == nil || + !strings.Contains(err.Error(), "references missing session "+sessionID) { + t.Fatalf("cutover error = %v, want missing-session conflict", err) + } + if !legacyTableExists(t, db, "task_session_worktrees") { + t.Fatal("failed cutover must retain task_session_worktrees") + } + var rows int + if err := db.Get(&rows, db.Rebind( + `SELECT COUNT(*) FROM task_session_worktrees WHERE session_id = ? AND worktree_id = ?`), + sessionID, worktreeID); err != nil { + t.Fatalf("count retained orphan row: %v", err) + } + if rows != 1 { + t.Fatalf("retained orphan rows = %d, want 1", rows) + } +} diff --git a/apps/backend/internal/task/repository/sqlite/worktree_ownership_postgres_test.go b/apps/backend/internal/task/repository/sqlite/worktree_ownership_postgres_test.go index a31e9dda58..4d5e8f6a2a 100644 --- a/apps/backend/internal/task/repository/sqlite/worktree_ownership_postgres_test.go +++ b/apps/backend/internal/task/repository/sqlite/worktree_ownership_postgres_test.go @@ -2,6 +2,7 @@ package sqlite import ( "context" + "strings" "testing" "time" @@ -72,6 +73,57 @@ func TestCutoverPostgres_NormalizesLegacyFlatEnvironment(t *testing.T) { } } +// TestCutoverPostgres_OrphanedSessionWorktreeUsesFlatOwner proves PostgreSQL +// uses the shared recoverable-orphan classification during cutover. +func TestCutoverPostgres_OrphanedSessionWorktreeUsesFlatOwner(t *testing.T) { + db := openLegacyPostgres(t) + now := time.Now().UTC().Truncate(time.Second) + seed := legacySeed{envID: "env-pg-orphan", taskID: "task-pg-orphan", repoID: "repo-pg-orphan", sessionID: "sess-pg-orphan"} + seedLegacyTask(t, db, seed, now) + seedLegacyFlatEnv(t, db, seed, "wt-pg-orphan", "/tasks/pg-orphan/current", "feature/current", now) + seedLegacySessionWorktree(t, db, seed.sessionID, "wt-pg-orphan", seed.repoID, "", + "/tasks/pg-orphan/stale", "feature/stale", "active", now) + deleteLegacySession(t, db, seed.sessionID) + + repo, err := NewWithDB(db, db, nil) + if err != nil { + t.Fatalf("postgres cutover: %v", err) + } + env, err := repo.GetTaskEnvironment(context.Background(), seed.envID) + if err != nil { + t.Fatalf("GetTaskEnvironment: %v", err) + } + if len(env.Repos) != 1 || env.Repos[0].WorktreeID != "wt-pg-orphan" || + env.Repos[0].WorktreePath != "/tasks/pg-orphan/current" || + env.Repos[0].WorktreeBranch != "feature/current" { + t.Fatalf("normalized postgres repos = %+v", env.Repos) + } +} + +func TestCutoverPostgres_ActiveOrphanedSessionWorktreeFailsClosed(t *testing.T) { + db := openLegacyPostgres(t) + now := time.Now().UTC().Truncate(time.Second) + seed := legacySeed{taskID: "task-pg-orphan-unique", sessionID: "sess-pg-orphan-unique"} + seedLegacyTask(t, db, seed, now) + seedLegacySessionWorktree(t, db, seed.sessionID, "wt-pg-orphan-unique", "repo-pg-orphan-unique", "", + "/tasks/pg-orphan-unique/repo", "feature/unique", "active", now) + deleteLegacySession(t, db, seed.sessionID) + + if _, err := NewWithDB(db, db, nil); err == nil || + !strings.Contains(err.Error(), "references missing session "+seed.sessionID) { + t.Fatalf("postgres cutover error = %v, want missing-session conflict", err) + } + var tableCount int + if err := db.Get(&tableCount, ` + SELECT COUNT(*) FROM information_schema.tables + WHERE table_name = 'task_session_worktrees' AND table_schema = current_schema()`); err != nil { + t.Fatalf("count legacy table: %v", err) + } + if tableCount != 1 { + t.Fatal("failed postgres cutover must leave the legacy table intact") + } +} + func TestCutoverPostgres_CanonicalRepoSupersedesDivergentFlatOwner(t *testing.T) { db := openLegacyPostgres(t) now := time.Now().UTC().Truncate(time.Second) diff --git a/docs/plans/task-owned-worktree-cutover-orphaned-sessions/plan.md b/docs/plans/task-owned-worktree-cutover-orphaned-sessions/plan.md new file mode 100644 index 0000000000..a4235593e2 --- /dev/null +++ b/docs/plans/task-owned-worktree-cutover-orphaned-sessions/plan.md @@ -0,0 +1,167 @@ +--- +spec: docs/specs/session-delete-resource-cleanup/spec.md +created: 2026-08-13 +status: complete +--- + +# Fix Plan: Recover Cutover From Orphaned Session Worktrees + +## Overview + +Repair the one-time task-worktree ownership cutover for legacy databases where +`task_session_worktrees` rows survived deletion of their `task_sessions` rows. +The migration will discard only orphaned history whose physical worktree is +already owned by a higher-precedence task source, while preserving the current +fail-closed behavior for active orphaned worktrees with no recoverable owner. + +## Root Cause + +`worktreeCutover.loadLegacySessionWorktrees` currently requires every legacy +row to resolve through `c.sessions` before the migration has a chance to apply +the established task-owned source precedence. It records every missing session +as an unconditional conflict and omits the row from `c.sessionWts`. + +The read-only production-shaped reproduction contains an active legacy row for +worktree `30e76351-5328-4dd5-9a2e-a8bf4825dfbd` whose session is missing, while +the surviving flat `task_environments` row already owns that exact worktree ID, +path, and branch. Startup therefore aborts even though the physical identity has +one unambiguous higher-precedence owner. `PRAGMA foreign_key_check` confirms the +legacy database also contains other child rows for deleted sessions, explaining +how this state can exist despite the declared foreign key. + +The smallest reliable automated reproduction is a legacy SQLite fixture with a +task and flat environment, an orphaned `task_session_worktrees` row carrying the +same worktree ID, and no corresponding `task_sessions` row. The current +initializer must fail with `references missing session` before the repair. + +## Backend + +### Deferred orphan classification + +Update +`apps/backend/internal/task/repository/sqlite/worktree_ownership_normalize.go`. +Retain missing-session rows in a separate orphan inventory during +`loadLegacySessionWorktrees` instead of immediately adding a conflict. After +`pickSurvivingEnvironments` has selected the authoritative flat environment, +classify each orphan with one narrow helper. Treat the row as superseded only +when: + +- the row is legacy-deleted (`status = deleted` or `deleted_at` is set); or +- the row has a non-empty `worktree_id` represented by a non-deleted normalized + target already built from a canonical `task_environment_repos` row or + surviving flat `task_environments` row. + +The match is based on physical identity, not path, branch, repository, or a +guessed task association, because the missing session removes the authoritative +task link. A superseded orphan contributes no ownership, metadata, inventory, +or demotion entry. An orphan not satisfying the rule adds the exact current +missing-session conflict. Deleted canonical repository rows, flat rows from +collapsed environments, and raw flat rows absorbed into a deleted normalized +target are not owners and must not suppress an active orphan. + +Keep transaction, snapshot, shadow-table validation, schema swap, rollback, +replay, PostgreSQL locking, and filesystem/Git behavior unchanged. The shared +normalizer covers SQLite and PostgreSQL cutovers, so both engines use the same +classification. + +## Tests + +- **What:** An orphaned legacy session-worktree that repeats the surviving flat + environment's physical ID does not block cutover, and stale orphan metadata + does not override the flat source. + **File:** + `apps/backend/internal/task/repository/sqlite/worktree_ownership_orphan_migration_test.go`. + **How:** Seed the flat source, insert the session-worktree without a session, + run the initializer, and assert the normalized row retains flat metadata and + the legacy table is dropped. +- **What:** An orphan that repeats a non-deleted canonical repository row is + ignored, including when its path and branch differ. + **File:** the same migration test file. + **How:** Seed the canonical row and orphan, then assert canonical metadata and + successful cutover. +- **What:** A deleted orphan is historical even without another source. + **File:** the same migration test file. + **How:** Seed a deleted orphan without its session and assert successful + cutover without creating an owner from the orphan. +- **What:** An active orphan with an empty or unique physical ID, one matching + only a deleted canonical repository row or collapsed flat row, or one whose + raw flat source is absorbed into a deleted normalized target remains fatal + and transactional. + **File:** the same migration test file. + **How:** Assert the missing-session diagnostic and that the legacy table and + row survive rollback. +- **What:** PostgreSQL applies the same shared classification. + **File:** + `apps/backend/internal/task/repository/sqlite/worktree_ownership_postgres_test.go`. + **How:** Add the smallest PostgreSQL fixtures for one recoverable orphan and + one unique active orphan that must fail closed. +- **What:** Existing precedence, inventory, rollback, replay, hybrid-schema, + and contested-slot behavior remains unchanged. + **File:** existing migration test files. + **How:** Run the focused orphan tests, all cutover tests, and the complete + SQLite repository package. + +## Verification Results + +- RED: `make -C apps/backend test + GOFLAGS="-v -run=TestCutover.*Orphan.* -count=1"` failed for the recoverable + flat, canonical, and deleted-history fixtures with the reported + `references missing session` diagnostic. The unique active and + deleted-canonical controls passed by proving the current fail-closed result. +- GREEN: `go test -tags fts5 ./internal/task/repository/sqlite -run + 'TestCutover.*Orphan.*' -count=1 -v` passed four SQLite top-level tests and + three fail-closed subtests. The PostgreSQL test skipped locally because + `KANDEV_TEST_POSTGRES_DSN` is not set. +- Focused gate: `make -C apps/backend test + GOFLAGS="-v -run=TestCutover.*Orphan.* -count=1"` passed. +- Cutover gate: `make -C apps/backend test + GOFLAGS="-v -run=TestCutover -count=1"` passed. The package discovers 44 + top-level cutover tests; environment-gated PostgreSQL cases skip locally. +- Backend gate: `make -C apps/backend test GOFLAGS="-v -count=1"` passed. +- A temporary `-tags fts5` repository test ran the initializer against an + SQLite backup of the reported 1.4 GB database. The cutover completed, removed + `task_session_worktrees`, and retained worktree + `30e76351-5328-4dd5-9a2e-a8bf4825dfbd` at the original path and branch. The + temporary test and database copy were removed; the live database was never + modified. +- `git diff --check` passed. +- Public docs need no change: the existing operations guidance still correctly + requires transactional rollback and backup recovery for conflicts that + remain irreconcilable. The repair behavior is recorded in the internal spec. +- PR review remediation added empty-ID and PostgreSQL fail-closed parity plus a + regression for a raw flat row absorbed into a deleted canonical target. The + new P1 regression failed before the classifier inspected normalized targets, + then the focused orphan and complete cutover gates passed after the repair. + +## Implementation Waves And Parallel Candidates + +Wave 1, sequential: + +- [x] [task-01-classify-orphaned-session-worktrees](task-01-classify-orphaned-session-worktrees.md) - done + +No task is parallel-safe. Classification, migration fixtures, and rollback +coverage share one persistence boundary. + +## Risks + +- Matching on path or branch could discard a distinct physical worktree. The + recovery requires an identical, non-empty `worktree_id`. +- Treating a deleted canonical row as authoritative could hide the only active + legacy owner. Only non-deleted canonical rows suppress active orphans. +- Trusting a raw surviving flat row is insufficient when merge precedence has + left its physical identity on a deleted target. The classifier must inspect + the normalized target that will actually be persisted. +- Trying to infer the missing session's task from repository or path data could + silently assign ownership to the wrong task. Recoverable orphans contribute + no ownership; a higher-precedence source must already own the identity. +- Merge and inventory logic must never see a discarded orphan, or its stale + metadata can reappear as a validation conflict. + +## Out Of Scope + +- Repairing unrelated orphaned session messages, turns, Git snapshots, or other + historical foreign-key violations. +- Manual database edits or automated deletion outside this one-time cutover. +- Filesystem or Git inspection, reconciliation, or cleanup during startup. +- Changes to runtime session deletion or task cleanup behavior. +- A new migration, public API, UI, or feature flag after the one-time cutover. diff --git a/docs/plans/task-owned-worktree-cutover-orphaned-sessions/task-01-classify-orphaned-session-worktrees.md b/docs/plans/task-owned-worktree-cutover-orphaned-sessions/task-01-classify-orphaned-session-worktrees.md new file mode 100644 index 0000000000..d2f50840b2 --- /dev/null +++ b/docs/plans/task-owned-worktree-cutover-orphaned-sessions/task-01-classify-orphaned-session-worktrees.md @@ -0,0 +1,114 @@ +--- +id: "01-classify-orphaned-session-worktrees" +title: "Classify orphaned session worktrees" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/session-delete-resource-cleanup/spec.md" +--- + +# Task 01: Classify Orphaned Session Worktrees + +## Acceptance + +- A missing-session row is ignored only when it is deleted history or its exact + non-empty physical worktree ID already has a non-deleted normalized target + built from a canonical or surviving flat owner. +- A recoverable orphan contributes no ownership or stale metadata, while an + active orphan without a higher-precedence owner still fails closed with the + current diagnostic and complete rollback. +- SQLite and PostgreSQL cutovers use the same classification, and existing + cutover compatibility, precedence, inventory, rollback, and replay behavior + remains valid. + +## Verification + +```bash +make -C apps/backend test GOFLAGS="-v -run=TestCutover.*Orphan.* -count=1" +make -C apps/backend test GOFLAGS="-v -run=TestCutover -count=1" +make -C apps/backend test GOFLAGS="-v -count=1" +``` + +The first focused command is the RED/GREEN gate: before changing production +code, add the orphan fixtures and confirm the recoverable cases fail with +`references missing session` while the irreconcilable cases already fail for +that expected reason. + +## Files Likely Touched + +- `apps/backend/internal/task/repository/sqlite/worktree_ownership_normalize.go` +- `apps/backend/internal/task/repository/sqlite/worktree_ownership_orphan_migration_test.go` +- `apps/backend/internal/task/repository/sqlite/worktree_ownership_postgres_test.go` +- `docs/specs/session-delete-resource-cleanup/spec.md` +- `docs/plans/task-owned-worktree-cutover-orphaned-sessions/plan.md` +- `docs/plans/task-owned-worktree-cutover-orphaned-sessions/task-01-classify-orphaned-session-worktrees.md` + +## Dependencies + +None. + +## Parallelism + +Sequential. The production classifier and both database-engine fixtures share +one migration and persistence boundary. + +## Inputs + +- Schema normalization, failure modes, and migration scenarios in the linked + repair spec. +- Root cause and classification rule in `plan.md`. +- Existing `loadLegacySessionWorktrees`, `pickSurvivingEnvironments`, + `isSupersededSessionWorktree`, `isLegacyDeletedWorktree`, + authoritative-source, inventory, and rollback behavior. +- Read-only affected-database evidence: the missing-session row repeats the + exact non-empty worktree ID owned by the surviving flat environment. + +## Output Contract + +Report the classification change, files changed, exact RED/GREEN and broader +test commands with results/counts, assumptions about deleted sources, and any +temporary reproduction cleanup. Update this task to `done`, synchronize the +plan checkbox/status and verification results, and run `git diff --check` in +the same conversation. + +## Results + +- **RED:** `make -C apps/backend test + GOFLAGS="-v -run=TestCutover.*Orphan.* -count=1"` failed for the recoverable + flat, canonical, and deleted-history cases with `references missing session`. + The unique active and deleted-canonical controls already passed by asserting + the expected fail-closed behavior. +- **Implementation:** missing-session rows are retained in a separate orphan + inventory until surviving environments and canonical rows are known. Deleted + orphans and exact non-empty physical IDs represented by a non-deleted + normalized target contribute no ownership or metadata. All other orphans + retain the original missing-session conflict. +- **GREEN:** `go test -tags fts5 ./internal/task/repository/sqlite -run + 'TestCutover.*Orphan.*' -count=1 -v` passed four SQLite top-level tests and + three fail-closed subtests; the PostgreSQL test skipped because + `KANDEV_TEST_POSTGRES_DSN` is not set. +- **Focused gate:** `make -C apps/backend test + GOFLAGS="-v -run=TestCutover.*Orphan.* -count=1"` passed. +- **Cutover gate:** `make -C apps/backend test + GOFLAGS="-v -run=TestCutover -count=1"` passed; 44 top-level cutover tests + are discovered, with environment-gated PostgreSQL cases skipped locally. +- **Backend gate:** `make -C apps/backend test GOFLAGS="-v -count=1"` passed. +- **Affected database:** a temporary `-tags fts5` repository test initialized + an SQLite backup of the reported 1.4 GB database. It removed the legacy table + and retained the reported worktree ID, path, and branch. The temporary test + and database copy were removed; the live database was untouched. +- **Files changed:** + `worktree_ownership_normalize.go`, + `worktree_ownership_orphan_migration_test.go`, + `worktree_ownership_postgres_test.go`, the repair spec, and this plan/task + package. +- **Hygiene:** `gofmt` and `git diff --check` passed. No temporary reproduction + artifacts remain. External side effects: none beyond the later authorized + branch push and PR workflow. +- **PR review remediation:** the raw-flat-plus-deleted-canonical regression + failed before the follow-up with a nil cutover error, proving an active orphan + could be suppressed without a persisted active owner. The classifier now + inspects non-deleted normalized targets. Empty worktree IDs and PostgreSQL + unique active orphans also have explicit fail-closed coverage. The focused + orphan and complete cutover Make gates passed after the change. diff --git a/docs/specs/session-delete-resource-cleanup/spec.md b/docs/specs/session-delete-resource-cleanup/spec.md index 0de526d517..e7f2faa850 100644 --- a/docs/specs/session-delete-resource-cleanup/spec.md +++ b/docs/specs/session-delete-resource-cleanup/spec.md @@ -124,6 +124,16 @@ owners. A higher-precedence task-owned source for the same repository and branch slot, either a canonical repository row or the surviving flat environment row, remains the owner even when a terminal reference carries a different physical `worktree_id`. +Legacy session-worktree rows can also outlive their referenced session in +databases written by older releases. An orphaned row is historical evidence +when it is marked deleted or when its exact non-empty physical `worktree_id` is +already represented by a non-deleted normalized target built from a canonical +repository row or surviving flat environment. The higher-precedence task-owned +source remains authoritative and the orphan does not prevent cutover. A raw +flat row absorbed into a deleted canonical target is not an active owner. An +active orphan with no matching non-deleted normalized target remains +irreconcilable because the cutover cannot recover its task or environment +owner, so startup fails closed. A non-deleted canonical repository row also takes precedence over deprecated flat fields for the same normalized task-owned repository slot: the repository @@ -247,6 +257,16 @@ remain the authorization boundary for physical cleanup. sessions do not block migration when a higher-precedence task-owned source exists for the same repository and branch slot. The higher-precedence source remains authoritative. +- An orphaned legacy session-worktree row does not block migration when it is + deleted history or its exact non-empty physical `worktree_id` is already + represented by a non-deleted normalized target built from a canonical + repository row or the surviving flat environment. The orphan contributes no + ownership or metadata. +- An active orphaned session-worktree row with no higher-precedence source for + the same physical `worktree_id` remains irreconcilable and fails closed. +- A raw surviving flat row does not suppress an active orphan when source + merging leaves that physical identity only on a deleted normalized target. + Startup fails closed rather than dropping the only active legacy evidence. - A legacy session row that repeats a higher-precedence physical `worktree_id` cannot block migration solely because its path or branch metadata is stale, including when the session is resumable. The higher-precedence repository or @@ -339,6 +359,24 @@ remain the authorization boundary for physical cleanup. the surviving flat environment's repository slot, **WHEN** the new binary starts, **THEN** the flat environment remains the owner and the cutover completes. +- **GIVEN** a legacy session-worktree row references a missing session and the + same non-empty `worktree_id` remains on a non-deleted normalized target built + from a surviving flat environment or canonical repository row, **WHEN** the + new binary starts, **THEN** the task-owned source remains authoritative, the + orphan contributes no metadata, and the cutover completes without manual + database edits. +- **GIVEN** a deleted legacy session-worktree row references a missing session, + **WHEN** the new binary starts, **THEN** the row is treated as historical + evidence and does not block the cutover. +- **GIVEN** an active legacy session-worktree row references a missing session + and no higher-precedence source carries its physical `worktree_id`, **WHEN** + the new binary starts, **THEN** startup fails closed and the complete legacy + schema and data remain unchanged. +- **GIVEN** a surviving flat row, a deleted canonical row, and an active orphan + share one physical `worktree_id`, but source merging leaves only a deleted + normalized target, **WHEN** the new binary starts, **THEN** startup fails + closed and preserves the legacy schema rather than dropping the active + orphan evidence. - **GIVEN** `task_environments` already has the normalized schema but a stale `task_session_worktrees` table remains from an intermediate build, **WHEN** the new binary starts, **THEN** the cutover preserves normalized environment