fix(tasks): archive a task whose canonical environment is gone (#3659)
Co-authored-by: nova28 <17953305+nova28@users.noreply.github.com> Co-authored-by: Carlos Florencio <carlosmflorencio@gmail.com>
This commit is contained in:
@@ -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)
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -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
|
||||
}
|
||||
|
||||
|
||||
@@ -60,7 +60,7 @@ async function selectFixtureBranch(page: Page): Promise<void> {
|
||||
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);
|
||||
}
|
||||
|
||||
|
||||
@@ -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 | — |
|
||||
+129
@@ -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.
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user