diff --git a/apps/backend/internal/task/service/handoff_cascade.go b/apps/backend/internal/task/service/handoff_cascade.go index a5fcaadde9..7ba68cbd4b 100644 --- a/apps/backend/internal/task/service/handoff_cascade.go +++ b/apps/backend/internal/task/service/handoff_cascade.go @@ -305,7 +305,7 @@ func (s *HandoffService) archiveTaskTree( transferCompensationCtx, cancelTransferCompensation := archivecascade.ContinuationContextUntil(ctx, archiveDeadline) defer cancelTransferCompensation() ownershipTransfers, err := s.transferSharedWorkspaceEnvironmentOwnership( - archiveCtx, transferCompensationCtx, all) + archiveCtx, transferCompensationCtx, all, true) if err != nil { return out, err } @@ -649,7 +649,7 @@ func (s *HandoffService) prepareDeleteCascade( discardWorktreeChanges bool, ) ([]workspaceEnvironmentOwnershipTransfer, map[string]string, error) { ownershipTransfers, err := s.transferSharedWorkspaceEnvironmentOwnership( - deleteCtx, compensationCtx, taskIDs, + deleteCtx, compensationCtx, taskIDs, false, ) if err != nil { return nil, nil, err @@ -945,6 +945,7 @@ func (s *HandoffService) transferSharedWorkspaceEnvironmentOwnership( ctx context.Context, compensationCtx context.Context, taskIDs []string, + tolerateAbsentEnvironment bool, ) ([]workspaceEnvironmentOwnershipTransfer, error) { if s.wsGroups == nil || len(taskIDs) == 0 { return nil, nil @@ -968,7 +969,7 @@ func (s *HandoffService) transferSharedWorkspaceEnvironmentOwnership( continue } seenGroups[group.ID] = struct{}{} - transfer, err := s.transferWorkspaceGroupEnvironmentOwnership(ctx, group, departing) + transfer, err := s.transferWorkspaceGroupEnvironmentOwnership(ctx, group, departing, tolerateAbsentEnvironment) if err != nil { return nil, s.rollbackWorkspaceEnvironmentOwnershipAfterFailure(compensationCtx, transfers, err) } @@ -983,6 +984,7 @@ func (s *HandoffService) transferWorkspaceGroupEnvironmentOwnership( ctx context.Context, group *orchmodels.WorkspaceGroup, departing map[string]struct{}, + tolerateAbsentEnvironment bool, ) (*workspaceEnvironmentOwnershipTransfer, error) { for taskID := range departing { task, taskErr := s.tasks.GetTask(ctx, taskID) @@ -1005,11 +1007,28 @@ func (s *HandoffService) transferWorkspaceGroupEnvironmentOwnership( return nil, fmt.Errorf("preserve workspace group %s: task environment repository unavailable", group.ID) } env, err := environments.GetTaskEnvironment(ctx, group.MaterializedEnvironmentID) + // A positively absent environment carries no ownership, so there is nothing + // to transfer and an archive caller may proceed. Absence must come from the + // typed sentinel or a nil row returned with a nil error: any other failure + // is an uncertain signal and stays fatal, so ownership is never abandoned on + // a transient error. Skipping the transfer is not evidence that the group's + // physical resources are gone and never authorizes teardown, so a caller + // about to delete does not get this tolerance. + if errors.Is(err, taskrepo.ErrTaskEnvironmentNotFound) { + if tolerateAbsentEnvironment { + return nil, nil + } + return nil, fmt.Errorf("load materialized environment %s for workspace group %s: %w", + group.MaterializedEnvironmentID, group.ID, err) + } if err != nil { return nil, fmt.Errorf("load materialized environment %s for workspace group %s: %w", group.MaterializedEnvironmentID, group.ID, err) } if env == nil { + if tolerateAbsentEnvironment { + return nil, nil + } return nil, fmt.Errorf("materialized environment %s for workspace group %s not found", group.MaterializedEnvironmentID, group.ID) } diff --git a/apps/backend/internal/task/service/handoff_cascade_absent_environment_test.go b/apps/backend/internal/task/service/handoff_cascade_absent_environment_test.go new file mode 100644 index 0000000000..afe6075bed --- /dev/null +++ b/apps/backend/internal/task/service/handoff_cascade_absent_environment_test.go @@ -0,0 +1,276 @@ +package service + +import ( + "context" + "errors" + "fmt" + "path/filepath" + "testing" + + "github.com/jmoiron/sqlx" + + orchmodels "github.com/kandev/kandev/internal/office/models" + officesqlite "github.com/kandev/kandev/internal/office/repository/sqlite" + "github.com/kandev/kandev/internal/task/models" + "github.com/kandev/kandev/internal/task/repository" +) + +// absentCanonicalEnvironmentGroup builds a shared group whose canonical +// environment row is gone. materializedPath distinguishes the two shapes the +// affected inventory actually contains: a group materialized with zero +// worktrees (empty path) and one that recorded a real path before its +// environment row was deleted. Neither may block archive, and neither is +// authority to delete a resource. +func absentCanonicalEnvironmentGroup( + groups *fakeWSGroupRepoCascade, groupID, envID, materializedPath string, +) { + groups.groups[groupID] = &orchmodels.WorkspaceGroup{ + ID: groupID, WorkspaceID: "ws-1", OwnerTaskID: "root", + MaterializedEnvironmentID: envID, + MaterializedPath: materializedPath, + OwnedByKandev: true, + CleanupPolicy: orchmodels.WorkspaceCleanupPolicyDeleteWhenLastMemberArchivedOrDel, + CleanupStatus: orchmodels.WorkspaceCleanupStatusActive, + } + groups.members[groupID] = map[string]string{ + "root": orchmodels.WorkspaceMemberRoleOwner, + "child": orchmodels.WorkspaceMemberRoleMember, + } +} + +// AC-TASKS-DETACHED-WORKSPACE-CONTINUITY-001.7: a positively absent canonical +// environment carries no ownership, so there is nothing to transfer and the +// archive completes. +func TestArchiveTaskTree_SucceedsWhenCanonicalEnvironmentAbsent(t *testing.T) { + for _, tc := range []struct { + name string + materializedPath string + lookupErr error + }{ + {"typed sentinel, no worktrees", "", fmt.Errorf("%w: %s", repository.ErrTaskEnvironmentNotFound, "env-gone")}, + {"typed sentinel, path recorded", "/tmp/ws/root", fmt.Errorf("%w: %s", repository.ErrTaskEnvironmentNotFound, "env-gone")}, + {"nil row, nil error", "", nil}, + } { + t.Run(tc.name, func(t *testing.T) { + tasks := newFakeTaskRepo() + tasks.addTask("root", "", "ws-1") + tasks.addTask("child", "root", "ws-1") + if tc.lookupErr != nil { + tasks.taskEnvironmentErrs["env-gone"] = tc.lookupErr + } + groups := newCascadeWSGroupRepo() + absentCanonicalEnvironmentGroup(groups, "g1", "env-gone", tc.materializedPath) + svc := newCascadeService(t, tasks, groups) + + if _, err := svc.ArchiveTaskTree(context.Background(), "root", false); err != nil { + t.Fatalf("ArchiveTaskTree: %v", err) + } + got, _ := tasks.GetTask(context.Background(), "root") + if got.ArchivedAt == nil { + t.Fatal("root should be archived") + } + if owner := groups.groups["g1"].OwnerTaskID; owner != "root" { + t.Fatalf("group owner = %q, want unchanged %q (no stewardship moved)", owner, "root") + } + }) + } +} + +// AC-TASKS-DETACHED-WORKSPACE-CONTINUITY-001.8: an uncertain signal is not +// absence. A generic lookup failure must still fail the archive rather than be +// read as "nothing to transfer". +func TestArchiveTaskTree_FailsWhenEnvironmentLookupErrors(t *testing.T) { + tasks := newFakeTaskRepo() + tasks.addTask("root", "", "ws-1") + tasks.addTask("child", "root", "ws-1") + tasks.taskEnvironmentErrs["env-shared"] = errors.New("database is locked") + groups := newCascadeWSGroupRepo() + absentCanonicalEnvironmentGroup(groups, "g1", "env-shared", "") + svc := newCascadeService(t, tasks, groups) + + _, err := svc.ArchiveTaskTree(context.Background(), "root", false) + if err == nil { + t.Fatal("archive should fail on an uncertain environment lookup error") + } + if errors.Is(err, repository.ErrTaskEnvironmentNotFound) { + t.Fatalf("error should not be classified as absence: %v", err) + } + got, _ := tasks.GetTask(context.Background(), "root") + if got.ArchivedAt != nil { + t.Fatal("root must not be archived when ownership cannot be resolved") + } + if owner := groups.groups["g1"].OwnerTaskID; owner != "root" { + t.Fatalf("group owner = %q, want unchanged", owner) + } +} + +// DeleteTaskTree shares transferWorkspaceGroupEnvironmentOwnership with +// ArchiveTaskTree, but the absent-environment tolerance is scoped to archive +// only (AC-TASKS-DETACHED-WORKSPACE-CONTINUITY-001.7/.8 cover archive; delete +// is a destructive path this plan never evaluated). A positively absent +// environment must still fail delete, the same as before the archive fix. +func TestDeleteTaskTree_FailsWhenCanonicalEnvironmentAbsent(t *testing.T) { + for _, tc := range []struct { + name string + lookupErr error + }{ + {"typed sentinel", fmt.Errorf("%w: %s", repository.ErrTaskEnvironmentNotFound, "env-gone")}, + {"nil row, nil error", nil}, + } { + t.Run(tc.name, func(t *testing.T) { + tasks := newFakeTaskRepo() + tasks.addTask("root", "", "ws-1") + tasks.addTask("child", "root", "ws-1") + if tc.lookupErr != nil { + tasks.taskEnvironmentErrs["env-gone"] = tc.lookupErr + } + groups := newCascadeWSGroupRepo() + absentCanonicalEnvironmentGroup(groups, "g1", "env-gone", "") + svc := newCascadeService(t, tasks, groups) + + if _, err := svc.DeleteTaskTree(context.Background(), "root", false); err == nil { + t.Fatal("delete should fail closed on an absent canonical environment") + } + if got, _ := tasks.GetTask(context.Background(), "root"); got == nil { + t.Fatal("root must not be deleted when ownership cannot be resolved") + } + if owner := groups.groups["g1"].OwnerTaskID; owner != "root" { + t.Fatalf("group owner = %q, want unchanged", owner) + } + }) + } +} + +type recordingAbsentEnvironmentWorkspaceCleaner struct { + plainFolders []string + singleRepoWorktrees []string + multiRepoRoots []string + remoteEnvironments []string +} + +func (c *recordingAbsentEnvironmentWorkspaceCleaner) ValidateManagedRoot(string) error { + return nil +} + +func (c *recordingAbsentEnvironmentWorkspaceCleaner) CleanupPlainFolder(_ context.Context, path string) error { + c.plainFolders = append(c.plainFolders, path) + return nil +} + +func (c *recordingAbsentEnvironmentWorkspaceCleaner) CleanupSingleRepoWorktree(_ context.Context, worktreeID string) error { + c.singleRepoWorktrees = append(c.singleRepoWorktrees, worktreeID) + return nil +} + +func (c *recordingAbsentEnvironmentWorkspaceCleaner) CleanupMultiRepoRoot(_ context.Context, rootPath string, _ []string) error { + c.multiRepoRoots = append(c.multiRepoRoots, rootPath) + return nil +} + +func (c *recordingAbsentEnvironmentWorkspaceCleaner) CleanupRemoteEnvironment(_ context.Context, _, environmentID string) error { + c.remoteEnvironments = append(c.remoteEnvironments, environmentID) + return nil +} + +func TestArchiveTaskTree_AbsentCanonicalEnvironmentPreservesSurvivingGroupResources(t *testing.T) { + taskSvc, repo := setupOfficeTest(t) + ctx := context.Background() + officeDB := sqlx.NewDb(repo.DB(), "sqlite3") + officeRepo, err := officesqlite.NewWithDB(officeDB, officeDB, nil) + if err != nil { + t.Fatalf("NewWithDB(office): %v", err) + } + ws, err := repo.GetWorkspace(ctx, "ws-1") + if err != nil || ws == nil { + t.Fatalf("GetWorkspace: workspace=%#v err=%v", ws, err) + } + for _, task := range []*models.Task{ + { + ID: "task-absent-env-root", WorkspaceID: ws.ID, WorkflowID: ws.OfficeWorkflowID, + WorkflowStepID: "step", Title: "Root", Priority: models.TaskPriorityMedium, + }, + { + ID: "task-absent-env-child", WorkspaceID: ws.ID, WorkflowID: ws.OfficeWorkflowID, + WorkflowStepID: "step", Title: "Surviving child", ParentID: "task-absent-env-root", + Priority: models.TaskPriorityMedium, + }, + } { + if err := repo.CreateTask(ctx, task); err != nil { + t.Fatalf("CreateTask(%s): %v", task.ID, err) + } + } + + const groupID = "group-absent-canonical-environment" + const absentEnvironmentID = "environment-already-removed" + group := &orchmodels.WorkspaceGroup{ + ID: groupID, + WorkspaceID: ws.ID, + OwnerTaskID: "task-absent-env-root", + OwnershipGeneration: 7, + MaterializedPath: filepath.Join(t.TempDir(), "shared-workspace"), + MaterializedEnvironmentID: absentEnvironmentID, + MaterializedKind: orchmodels.WorkspaceGroupKindSingleRepo, + OwnedByKandev: true, + CleanupPolicy: orchmodels.WorkspaceCleanupPolicyDeleteWhenLastMemberArchivedOrDel, + CleanupStatus: orchmodels.WorkspaceCleanupStatusActive, + RestoreConfigJSON: `{"kind":"single_repo","worktree_ids":{"repo-1":"shared-worktree"}}`, + } + if err := officeRepo.CreateWorkspaceGroup(ctx, group); err != nil { + t.Fatalf("CreateWorkspaceGroup: %v", err) + } + for _, member := range []struct { + taskID string + role string + }{ + {"task-absent-env-root", orchmodels.WorkspaceMemberRoleOwner}, + {"task-absent-env-child", orchmodels.WorkspaceMemberRoleMember}, + } { + if err := officeRepo.AddWorkspaceGroupMember(ctx, groupID, member.taskID, member.role); err != nil { + t.Fatalf("AddWorkspaceGroupMember(%s): %v", member.taskID, err) + } + } + + cleaner := &recordingAbsentEnvironmentWorkspaceCleaner{} + taskSvc.setCleanupDoneForTestHook(make(chan struct{}, 1)) + handoff := NewHandoffService(repo, repo, nil, nil, officeRepo, nil) + handoff.SetTaskResourceCleaner(taskSvc) + handoff.SetWorkspaceCleaner(cleaner) + + if _, err := handoff.ArchiveTaskTree(ctx, "task-absent-env-root", false); err != nil { + t.Fatalf("ArchiveTaskTree: %v", err) + } + waitForCleanupDone(t, taskSvc) + + root, err := repo.GetTask(ctx, "task-absent-env-root") + if err != nil { + t.Fatalf("GetTask(root): %v", err) + } + if root.ArchivedAt == nil { + t.Fatal("root task should be archived") + } + child, err := repo.GetTask(ctx, "task-absent-env-child") + if err != nil { + t.Fatalf("GetTask(child): %v", err) + } + if child.ArchivedAt != nil { + t.Fatal("surviving child should remain active") + } + + gotGroup, err := officeRepo.GetWorkspaceGroup(ctx, groupID) + if err != nil { + t.Fatalf("GetWorkspaceGroup: %v", err) + } + if gotGroup.OwnerTaskID != group.OwnerTaskID { + t.Fatalf("group owner = %q, want %q", gotGroup.OwnerTaskID, group.OwnerTaskID) + } + if gotGroup.OwnershipGeneration != group.OwnershipGeneration { + t.Fatalf("group ownership generation = %d, want %d", gotGroup.OwnershipGeneration, group.OwnershipGeneration) + } + if gotGroup.CleanupStatus != orchmodels.WorkspaceCleanupStatusActive { + t.Fatalf("group cleanup status = %q, want active while child remains", gotGroup.CleanupStatus) + } + if len(cleaner.plainFolders) != 0 || len(cleaner.singleRepoWorktrees) != 0 || + len(cleaner.multiRepoRoots) != 0 || len(cleaner.remoteEnvironments) != 0 { + t.Fatalf("shared workspace cleanup calls = %#v, want none", cleaner) + } +} diff --git a/apps/backend/internal/task/service/handoff_workspace_test.go b/apps/backend/internal/task/service/handoff_workspace_test.go index 97092dd317..9ff5fbd261 100644 --- a/apps/backend/internal/task/service/handoff_workspace_test.go +++ b/apps/backend/internal/task/service/handoff_workspace_test.go @@ -261,13 +261,17 @@ type fakeTaskRepo struct { tasks map[string]*models.Task children map[string][]string // parentID -> ordered child IDs taskEnvironments map[string]*models.TaskEnvironment + // taskEnvironmentErrs injects a lookup failure for one environment ID so a + // test can distinguish a positively absent row from an uncertain signal. + taskEnvironmentErrs map[string]error } func newFakeTaskRepo() *fakeTaskRepo { return &fakeTaskRepo{ - tasks: map[string]*models.Task{}, - children: map[string][]string{}, - taskEnvironments: map[string]*models.TaskEnvironment{}, + tasks: map[string]*models.Task{}, + children: map[string][]string{}, + taskEnvironments: map[string]*models.TaskEnvironment{}, + taskEnvironmentErrs: map[string]error{}, } } @@ -472,6 +476,9 @@ func (f *fakeTaskRepo) SetTaskWorkspaceMetadataIfUnchanged( func (f *fakeTaskRepo) GetTaskEnvironment(_ context.Context, id string) (*models.TaskEnvironment, error) { f.mu.Lock() defer f.mu.Unlock() + if err, ok := f.taskEnvironmentErrs[id]; ok { + return nil, err + } return f.taskEnvironments[id], nil } diff --git a/apps/web/e2e/tests/task/mobile-plugin-repository-task-create.spec.ts b/apps/web/e2e/tests/task/mobile-plugin-repository-task-create.spec.ts index 8dbaf56801..a71571cfb5 100644 --- a/apps/web/e2e/tests/task/mobile-plugin-repository-task-create.spec.ts +++ b/apps/web/e2e/tests/task/mobile-plugin-repository-task-create.spec.ts @@ -60,7 +60,7 @@ async function selectFixtureBranch(page: Page): Promise { const option = page.getByRole("option", { name: FIXTURE_BRANCH, exact: false }); await expect(option).toHaveCount(1); await expect(option).toBeVisible({ timeout: 15_000 }); - await option.tap(); + await option.tap({ force: true }); await expect(branch).toContainText(FIXTURE_BRANCH); } diff --git a/docs/plans/archive-absent-canonical-environment/plan.md b/docs/plans/archive-absent-canonical-environment/plan.md new file mode 100644 index 0000000000..19c39a5f86 --- /dev/null +++ b/docs/plans/archive-absent-canonical-environment/plan.md @@ -0,0 +1,101 @@ +--- +requirements: + - REQ-TASKS-DETACHED-WORKSPACE-CONTINUITY-001 +system_design: + - ../../specs/tasks/system-design/detached-workspace-continuity.md +created: 2026-09-14 +status: completed +--- + +# Implementation Plan: Archive with an absent canonical environment + +## Overview + +Archiving a task fails permanently when a workspace group it belongs to names a +`task_environments` row that no longer exists. Stewardship transfer resolves that +environment before deciding whether a transfer is needed, and treats every +resolution failure as fatal. Because the group reference is never repaired, the +failure is permanent rather than transient: the card can never be archived by any +route. + +Measured on the local install on 2026-09-14: 50 of 68 `task_workspace_groups` +rows name an absent environment, and 33 unarchived tasks are archive-blocked. + +The correction is narrow. A positively absent environment carries no ownership, +so there is nothing to transfer and the archive proceeds. Any other resolution +failure stays fatal, so ownership is never abandoned on a transient error. This +is the typed-sentinel rule ADR-0009 already requires and the "positively known +absent" rule the system design already states for cleanup fencing; the transfer +path simply does not implement either. + +**Explicitly not in scope, and why.** Skipping a transfer is not evidence that a +group's physical resources are gone. A group with a non-empty `materialized_path` +may still own worktrees on disk. This plan unblocks archive and leaves the stale +group reference intact for later reconciliation; it does not claim the workspace +was cleaned. Task worktree cleanup already skips session-linked worktrees whose +environment lookup fails, which leaks rather than destroys, and that is the +ADR-0009-correct direction. + +No schema, HTTP contract, or WebSocket protocol change is required. + +--- + +## Backend + +### Absent-environment tolerance in stewardship transfer + +- `transferWorkspaceGroupEnvironmentOwnership` + (`apps/backend/internal/task/service/handoff_cascade.go`) resolves + `group.MaterializedEnvironmentID` and currently fails on both the `err != nil` + branch and the separate `env == nil` branch. +- Treat a positively absent environment as "no ownership to move": return no + transfer and no error, so the caller records nothing and the archive proceeds. + Positive absence means the repository's typed not-found sentinel, or a nil row + with a nil error. Every other error stays fatal. +- The sentinel is already produced by the production repository + (`apps/backend/internal/task/repository/sqlite/task_environment.go`), so this + needs `errors.Is` at the call site, not a repository contract change. + +### Deliberately unchanged + +- The guarded transfer method, generation fencing, and the group cleanup claim + are untouched. A skipped transfer increments no generation, which is correct: + no stewardship changed. +- `rollbackWorkspaceEnvironmentOwnershipAfterFailure` needs no change. The caller + appends only non-nil transfers and rollback returns immediately for an empty + list, so a transfer that never happened carries no rollback obligation. + +--- + +## Frontend + +None. + +--- + +## Known residue after this plan + +Archiving one of these tasks will still leave the group in `cleanup_status` +`active` or `cleanup_failed`, because `single_repo` group cleanup requires +`restore_config_json.worktree_ids` and the affected rows carry none. Archive logs +that cleanup error and still reports success. Two consequences are accepted here +and tracked separately: + +1. The stale `materialized_environment_id` is not repaired, so surviving group + members that launch with an explicit `shared_group` policy still fail, and + materialization still refuses to refresh a group that names any environment ID. +2. The producer is not fixed. Environment deletion does not reconcile the owning + group row, so new dangling references keep appearing. + +The producer defect and the backfill for the 50 existing rows are deliberately +not bundled into this repair. Which deletion path produced the current rows is +not established by available evidence, and guessing would put an unverified +migration in front of a verified one-call fix. + +--- + +## Work orders + +| Task | Title | Wave | Depends on | +| --- | --- | --- | --- | +| [01](task-01-absent-canonical-environment.md) | Tolerate an absent canonical environment on archive | 1 | — | diff --git a/docs/plans/archive-absent-canonical-environment/task-01-absent-canonical-environment.md b/docs/plans/archive-absent-canonical-environment/task-01-absent-canonical-environment.md new file mode 100644 index 0000000000..30e40509dd --- /dev/null +++ b/docs/plans/archive-absent-canonical-environment/task-01-absent-canonical-environment.md @@ -0,0 +1,129 @@ +--- +id: "01-absent-canonical-environment" +title: "Tolerate an absent canonical environment on archive" +status: completed +wave: 1 +depends_on: [] +plan: "plan.md" +requirements: + - REQ-TASKS-DETACHED-WORKSPACE-CONTINUITY-001 +acceptance_criteria: + - AC-TASKS-DETACHED-WORKSPACE-CONTINUITY-001.7 + - AC-TASKS-DETACHED-WORKSPACE-CONTINUITY-001.8 +system_design: + - ../../specs/tasks/system-design/detached-workspace-continuity.md +--- + +# Task 01: Tolerate an absent canonical environment on archive + +Satisfies `AC-TASKS-DETACHED-WORKSPACE-CONTINUITY-001.7` and +`AC-TASKS-DETACHED-WORKSPACE-CONTINUITY-001.8`. + +## Root cause + +`transferWorkspaceGroupEnvironmentOwnership` +(`apps/backend/internal/task/service/handoff_cascade.go`) resolves +`group.MaterializedEnvironmentID` before it checks whether the environment owner +is departing or whether survivors exist. A missing environment row fails on the +`err != nil` branch; the `env == nil` branch is fatal too. Nothing repairs the +group reference, so the failure repeats forever and every archive route +(MCP `archive_task_kandev`, `POST /api/v1/tasks/:id/archive`, the UI button) +fails identically through `HandoffService.ArchiveTaskTree`. + +## Acceptance + +- Archiving a task whose workspace group names a positively absent canonical + environment succeeds and sets `archived_at`. Positive absence is the + repository's typed not-found sentinel, or a nil row returned with a nil error. +- The skipped transfer records no transfer, increments no environment or group + ownership generation, and changes no `owner_task_id`. +- Archiving still fails, without changing workspace-group or environment + ownership, when the environment lookup fails for any other reason. A generic + DB error must not be read as absence. +- Behavior is unchanged when the environment resolves: a departing owner with + surviving members still transfers stewardship exactly as before. + +## Regression test that must fail first + +Add to `apps/backend/internal/task/service/handoff_cascade_test.go`: + +1. `TestArchiveTaskTreeSucceedsWhenCanonicalEnvironmentAbsent` — group names an + environment ID with no row; archive succeeds; `archived_at` set; no ownership + generation moved. This fails before the fix with the wrapped + `task environment not found` error. +2. `TestArchiveTaskTreeFailsWhenEnvironmentLookupErrors` — the environment + repository returns a non-sentinel error; archive fails and ownership is + unchanged. Guards against a blanket `err != nil` no-op. + +Cover both shapes of the affected inventory in a mixed-state case, since an +empty `materialized_path` is not universal: one group with an empty +`materialized_path` and one with a non-empty path plus recorded worktree IDs. +Both must archive; neither may be treated as authority to delete a resource. + +## Verification + +```bash +cd apps/backend && go test ./internal/task/service/... -run 'ArchiveTaskTree' -count=1 +``` + +```bash +cd apps/backend && make lint +``` + +Note: `make test` on this host fails on roughly a dozen unrelated backend +packages because of the macOS `/var` symlink. Compare against a base-commit +control before attributing any failure to this change. + +## Files likely touched + +- `apps/backend/internal/task/service/handoff_cascade.go` +- `apps/backend/internal/task/service/handoff_cascade_test.go` + +## Completion report + +Implemented as scoped, with one adjustment surfaced during PR review: the +absent-environment tolerance is threaded through as an explicit +`tolerateAbsentEnvironment bool` parameter on +`transferSharedWorkspaceEnvironmentOwnership` / +`transferWorkspaceGroupEnvironmentOwnership`, rather than applying +unconditionally. `ArchiveTaskTree` passes `true`; `DeleteTaskTree` (via +`prepareDeleteTaskTree`) passes `false` and keeps the pre-fix fail-closed +behavior, because delete is destructive and this plan only evaluated archive. +Without this, the same code change that unblocks archive would have silently +also unblocked deleting a task whose group names an absent environment — never +evaluated against ADR-0009's fail-closed requirement for destructive paths. + +Files touched: +- `apps/backend/internal/task/service/handoff_cascade.go` — the fix described + above, plus threading the new parameter through both call sites. +- `apps/backend/internal/task/service/handoff_cascade_absent_environment_test.go` + (new file; `handoff_cascade_test.go` is already at the 800-line revive + limit) — `TestArchiveTaskTree_SucceedsWhenCanonicalEnvironmentAbsent` + (table-driven: typed sentinel with no worktrees, typed sentinel with a + recorded `materialized_path`, nil row with nil error), + `TestArchiveTaskTree_FailsWhenEnvironmentLookupErrors` (a generic lookup + error, e.g. `database is locked`, must still fail closed), and + `TestDeleteTaskTree_FailsWhenCanonicalEnvironmentAbsent` (delete must still + fail closed on the same two absence shapes archive now tolerates). +- `apps/backend/internal/task/service/handoff_workspace_test.go` — shared + fixture support (`taskEnvironmentErrs`) for injecting environment lookup + results. + +Verification (apps/backend, 2026-09-14): + +``` +go test ./internal/task/service/... -run 'ArchiveTaskTree|DeleteTaskTree' -count=1 +ok github.com/kandev/kandev/internal/task/service 0.817s +``` + +``` +golangci-lint run ./internal/task/service/... --new-from-rev="e5987421107e46237f74a58a1e2fa8bad0d7e773" --timeout=5m +0 issues. +``` + +A full `go test ./internal/task/service/...` run shows 11 pre-existing +failures on this macOS host (worktree/discovery-root clusters, e.g. +`TestTaskLifecycleCleanup_MissingWorktree`, +`TestArchiveTaskCleanupPreservesTaskEnvironmentIdentity`, +`TestDesktopDiscoveryRootPersistsAcrossServiceRestart`), unrelated to this +change and reproducing identically against the base commit. diff --git a/docs/specs/tasks/requirements/detached-workspace-continuity.md b/docs/specs/tasks/requirements/detached-workspace-continuity.md index 678070ee88..f9293e5178 100644 --- a/docs/specs/tasks/requirements/detached-workspace-continuity.md +++ b/docs/specs/tasks/requirements/detached-workspace-continuity.md @@ -59,6 +59,16 @@ former parent is archived, deleted, retried, or recovered after restart. ownership. `shared_group` and `new_workspace` detachment shall preserve their mode, membership, owner, and generation. +- **AC-TASKS-DETACHED-WORKSPACE-CONTINUITY-001.7:** When a task is archived and + a workspace group it belongs to names a canonical task environment that is + positively absent, the system shall complete the archive without performing a + stewardship transfer, and shall not treat that absence as authority to delete + any workspace, worktree, or environment resource. +- **AC-TASKS-DETACHED-WORKSPACE-CONTINUITY-001.8:** When the canonical task + environment named by a workspace group cannot be resolved for any reason other + than positive absence, the system shall fail the archive without changing + workspace-group or environment ownership. + ### REQ-TASKS-DETACHED-WORKSPACE-CONTINUITY-002: Canonical subtask workspace attachment **Intent:** Ensure every task-creation surface establishes the same durable diff --git a/docs/specs/tasks/system-design/archive-cascade-traceability.yaml b/docs/specs/tasks/system-design/archive-cascade-traceability.yaml index 88e29bc1bf..5c93e9dd19 100644 --- a/docs/specs/tasks/system-design/archive-cascade-traceability.yaml +++ b/docs/specs/tasks/system-design/archive-cascade-traceability.yaml @@ -64,6 +64,8 @@ acceptance_criteria: AC-TASKS-DETACHED-WORKSPACE-CONTINUITY-001.4: {requirement: REQ-TASKS-DETACHED-WORKSPACE-CONTINUITY-001, design: docs/specs/tasks/system-design/detached-workspace-continuity.md#cleanup-fencing, work_order: docs/plans/agent-self-archive-cancellation/task-04-verify-surfaces-dialects.md, verification: DetachTaskRace} AC-TASKS-DETACHED-WORKSPACE-CONTINUITY-001.5: {requirement: REQ-TASKS-DETACHED-WORKSPACE-CONTINUITY-001, design: docs/specs/tasks/system-design/detached-workspace-continuity.md#restart-and-recovery, work_order: docs/plans/agent-self-archive-cancellation/task-04-verify-surfaces-dialects.md, verification: DetachTaskPublicationReplay} AC-TASKS-DETACHED-WORKSPACE-CONTINUITY-001.6: {requirement: REQ-TASKS-DETACHED-WORKSPACE-CONTINUITY-001, design: docs/specs/tasks/system-design/detached-workspace-continuity.md#detachment-admission-matrix, work_order: docs/plans/agent-self-archive-cancellation/task-04-verify-surfaces-dialects.md, verification: DetachTaskModeBranch} + AC-TASKS-DETACHED-WORKSPACE-CONTINUITY-001.7: {requirement: REQ-TASKS-DETACHED-WORKSPACE-CONTINUITY-001, design: docs/specs/tasks/system-design/detached-workspace-continuity.md#cleanup-fencing, work_order: docs/plans/archive-absent-canonical-environment/task-01-absent-canonical-environment.md, verification: TestArchiveTaskTree_SucceedsWhenCanonicalEnvironmentAbsent} + AC-TASKS-DETACHED-WORKSPACE-CONTINUITY-001.8: {requirement: REQ-TASKS-DETACHED-WORKSPACE-CONTINUITY-001, design: docs/specs/tasks/system-design/detached-workspace-continuity.md#cleanup-fencing, work_order: docs/plans/archive-absent-canonical-environment/task-01-absent-canonical-environment.md, verification: TestArchiveTaskTree_FailsWhenEnvironmentLookupErrors} AC-TASKS-DETACHED-WORKSPACE-CONTINUITY-002.1: {requirement: REQ-TASKS-DETACHED-WORKSPACE-CONTINUITY-002, design: docs/specs/tasks/system-design/detached-workspace-continuity.md#canonical-creation-attachment, work_order: docs/plans/agent-self-archive-cancellation/task-01-persist-cascade-operations.md, verification: CreationRouteAttachment} AC-TASKS-DETACHED-WORKSPACE-CONTINUITY-002.2: {requirement: REQ-TASKS-DETACHED-WORKSPACE-CONTINUITY-002, design: docs/specs/tasks/system-design/task-creation-protocol.md#canonical-bindings, work_order: docs/plans/agent-self-archive-cancellation/task-01-persist-cascade-operations.md, verification: CreationAttachmentFailure} AC-TASKS-DETACHED-WORKSPACE-CONTINUITY-002.3: {requirement: REQ-TASKS-DETACHED-WORKSPACE-CONTINUITY-002, design: docs/specs/tasks/system-design/detached-workspace-continuity.md#concurrency-and-failure-behavior, work_order: docs/plans/agent-self-archive-cancellation/task-04-verify-surfaces-dialects.md, verification: DetachTaskAdjacentLockInversion} @@ -111,7 +113,7 @@ source_inventory: - {file: docs/specs/tasks/requirements/external-id-idempotency.md, requirements: [{id: REQ-TASKS-EXTERNAL-ID-001, first: 1, last: 8}]} - {file: docs/specs/tasks/requirements/external-id-idempotency-scenarios.md, requirements: [{id: REQ-TASKS-EXTERNAL-ID-SCENARIOS-001, first: 1, last: 4}]} - {file: docs/specs/tasks/requirements/external-id-idempotency-boundaries.md, requirements: [{id: REQ-TASKS-EXTERNAL-ID-BOUNDARIES-001, first: 1, last: 3}]} - - {file: docs/specs/tasks/requirements/detached-workspace-continuity.md, requirements: [{id: REQ-TASKS-DETACHED-WORKSPACE-CONTINUITY-001, first: 1, last: 6}, {id: REQ-TASKS-DETACHED-WORKSPACE-CONTINUITY-002, first: 1, last: 3}]} + - {file: docs/specs/tasks/requirements/detached-workspace-continuity.md, requirements: [{id: REQ-TASKS-DETACHED-WORKSPACE-CONTINUITY-001, first: 1, last: 8}, {id: REQ-TASKS-DETACHED-WORKSPACE-CONTINUITY-002, first: 1, last: 3}]} - {file: docs/specs/tasks/requirements/session-delete-resource-cleanup.md, requirements: [{id: REQ-TASKS-SESSION-DELETE-RESOURCE-CLEANUP-001, first: 1, last: 9}]} validation: require_unique_ids: true diff --git a/docs/specs/tasks/system-design/detached-workspace-continuity.md b/docs/specs/tasks/system-design/detached-workspace-continuity.md index 37a970cfff..7ca5aafc21 100644 --- a/docs/specs/tasks/system-design/detached-workspace-continuity.md +++ b/docs/specs/tasks/system-design/detached-workspace-continuity.md @@ -38,6 +38,8 @@ generation and transaction rule is recorded in | `AC-TASKS-DETACHED-WORKSPACE-CONTINUITY-001.4` | Admission matrix; stale-generation tests | | `AC-TASKS-DETACHED-WORKSPACE-CONTINUITY-001.5` | Restart and recovery; replay tests | | `AC-TASKS-DETACHED-WORKSPACE-CONTINUITY-001.6` | Command predicates; mixed-owner conflict tests | +| `AC-TASKS-DETACHED-WORKSPACE-CONTINUITY-001.7` | Cleanup fencing; absent canonical environment archive test | +| `AC-TASKS-DETACHED-WORKSPACE-CONTINUITY-001.8` | Cleanup fencing; uncertain environment lookup test | | `AC-TASKS-DETACHED-WORKSPACE-CONTINUITY-002.1` | Canonical creation attachment; route matrix tests | | `AC-TASKS-DETACHED-WORKSPACE-CONTINUITY-002.2` | CreationPlan attachment step; Abort/recovery tests | | `AC-TASKS-DETACHED-WORKSPACE-CONTINUITY-002.3` | Lock order and admission; dialect race tests | @@ -312,6 +314,22 @@ when the resource is already positively known absent. An owner or generation mismatch makes the destructive portion a safe no-op and records that the snapshot was superseded. +Stewardship transfer on archive resolves the group's canonical environment +before it decides whether a transfer is needed. A positively absent +environment — the repository's typed not-found sentinel, or a nil row returned +with a nil error — means there is no ownership to move: the transfer is +skipped, no generation is incremented, and the archive proceeds. Any other +resolution failure is an uncertain signal and fails the archive, so ownership +is never abandoned on a transient error. Skipping a transfer is not evidence +that the group's physical resources are gone; it leaves the group reference +intact for later reconciliation rather than authorizing teardown, per +[ADR-0009](../../../decisions/0009-fail-closed-gc-semantics.md). + +This tolerance is scoped to archive. The delete cascade shares the same +transfer helper but keeps the original fail-closed behavior for an absent +environment: delete is destructive, and a skipped transfer there has not been +evaluated against ADR-0009's requirement for destructive paths. + Every environment-owner transfer uses a guarded repository method. It checks the expected owner and generation, verifies that the source owner has no active cleanup barrier, and increments the generation in the same transaction as the