From 5cc9a1917cc1ba43e93d0dad9b3bc148549427fc Mon Sep 17 00:00:00 2001 From: Ethan Dickson Date: Wed, 26 Aug 2026 16:10:29 +0100 Subject: [PATCH 1/5] fix: cascade-remove inactive descendants when deleting archived workspaces MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The persistent sub-agent lifecycle change (PR #3825) made completed sub-agents persist in config as inactive children. But removeUnlocked's guard used hasDescendantAgentTasks() which checks for ANY descendants (active or inactive), and fires before the force flag is checked. This meant any workspace that ever spawned a sub-agent could never be deleted — shift+click bypass, force delete, and normal delete all failed. Fix: - Change the guard to hasActiveDescendantAgentTasksForWorkspace so only running/queued children block deletion - Cascade-remove inactive descendants deepest-first before removing the parent, mirroring what task_remove requires users to do manually - Add listDescendantAgentTaskIdsDeepestFirst() to TaskService for the cascade ordering --- src/node/services/taskService.ts | 30 +++++++++++++++++++ .../services/taskWorkspaceSeam.testUtils.ts | 1 + src/node/services/taskWorkspaceSeam.ts | 1 + src/node/services/workspaceService.test.ts | 11 +++---- src/node/services/workspaceService.ts | 20 +++++++++++-- 5 files changed, 56 insertions(+), 7 deletions(-) diff --git a/src/node/services/taskService.ts b/src/node/services/taskService.ts index b4d5ab7706..2fd2490a51 100644 --- a/src/node/services/taskService.ts +++ b/src/node/services/taskService.ts @@ -8797,6 +8797,36 @@ export class TaskService implements AgentTaskIntegration { return this.listDescendantAgentTaskIdsFromIndex(index, workspaceId).length > 0; } + /** + * List all descendant agent task IDs sorted deepest-first so callers can + * cascade-remove children before their parents without tripping the orphan guard. + */ + listDescendantAgentTaskIdsDeepestFirst(workspaceId: string): string[] { + assert( + workspaceId.length > 0, + "listDescendantAgentTaskIdsDeepestFirst: workspaceId must be non-empty" + ); + + const cfg = this.config.loadConfigOrDefault(); + const index = this.buildAgentTaskIndex(cfg); + const ids = this.listDescendantAgentTaskIdsFromIndex(index, workspaceId); + + // Sort by depth (deepest first) so leaf children are removed before their parents. + // Ties are broken by insertion order (stable sort). + const depthById = new Map(); + for (const id of ids) { + let depth = 0; + let current: string | undefined = id; + while (current != null && current !== workspaceId) { + depth++; + current = index.parentById.get(current); + } + depthById.set(id, depth); + } + + return ids.sort((a, b) => (depthById.get(b) ?? 0) - (depthById.get(a) ?? 0)); + } + hasActiveDescendantAgentTasksForWorkspace(workspaceId: string): boolean { assert( workspaceId.length > 0, diff --git a/src/node/services/taskWorkspaceSeam.testUtils.ts b/src/node/services/taskWorkspaceSeam.testUtils.ts index 5b81af0977..9eac2e2570 100644 --- a/src/node/services/taskWorkspaceSeam.testUtils.ts +++ b/src/node/services/taskWorkspaceSeam.testUtils.ts @@ -65,6 +65,7 @@ export function makeAgentTaskIntegrationFake( operation(), hasDescendantAgentTasks: () => false, hasActiveDescendantAgentTasksForWorkspace: () => false, + listDescendantAgentTaskIdsDeepestFirst: () => [], hasActiveTopLevelWorkflowRunsForWorkspace: () => Promise.resolve(false), getAgentTaskStatus: () => undefined, resetAutoResumeCount: () => undefined, diff --git a/src/node/services/taskWorkspaceSeam.ts b/src/node/services/taskWorkspaceSeam.ts index ecd8810187..9935aefb65 100644 --- a/src/node/services/taskWorkspaceSeam.ts +++ b/src/node/services/taskWorkspaceSeam.ts @@ -514,6 +514,7 @@ export interface AgentTaskIntegration { withTaskTreeLifecycleLock(workspaceId: string, operation: () => Promise): Promise; hasDescendantAgentTasks(workspaceId: string): boolean; hasActiveDescendantAgentTasksForWorkspace(workspaceId: string): boolean; + listDescendantAgentTaskIdsDeepestFirst(workspaceId: string): string[]; hasActiveTopLevelWorkflowRunsForWorkspace(workspaceId: string): Promise; getAgentTaskStatus(workspaceId: string): AgentTaskStatus | null | undefined; resetAutoResumeCount(workspaceId: string): void; diff --git a/src/node/services/workspaceService.test.ts b/src/node/services/workspaceService.test.ts index 66bc71107a..5c2363d176 100644 --- a/src/node/services/workspaceService.test.ts +++ b/src/node/services/workspaceService.test.ts @@ -12347,7 +12347,7 @@ describe("WorkspaceService assertPricedModelForBudgetedGoal", () => { }); describe("WorkspaceService remove lifecycle coordination", () => { - test("checks descendant tasks while holding the task-tree lifecycle lock", async () => { + test("blocks removal when active descendant tasks exist", async () => { const workspaceId = "parent-remove-lifecycle"; const workspaceService = createWorkspaceServiceForTest({ config: { @@ -12370,24 +12370,25 @@ describe("WorkspaceService remove lifecycle coordination", () => { insideLifecycleLock = false; } }; - const hasDescendantAgentTasks = mock(() => { + const hasActiveDescendantAgentTasksForWorkspace = mock(() => { expect(insideLifecycleLock).toBe(true); return true; }); workspaceService.setAgentTaskIntegration( makeAgentTaskIntegrationFake({ withTaskTreeLifecycleLock: runWithTaskTreeLifecycleLock, - hasDescendantAgentTasks, + hasActiveDescendantAgentTasksForWorkspace, + listDescendantAgentTaskIdsDeepestFirst: mock(() => []), }) ); expect(await workspaceService.remove(workspaceId, true)).toEqual( Err( - "This workspace has descendant sub-agent workspaces. Remove those descendants deepest-first before removing their parent." + "This workspace has active descendant sub-agent workspaces. Stop them before removing their parent." ) ); expect(withTaskTreeLifecycleLock).toHaveBeenCalledWith(workspaceId, expect.any(Function)); - expect(hasDescendantAgentTasks).toHaveBeenCalledWith(workspaceId); + expect(hasActiveDescendantAgentTasksForWorkspace).toHaveBeenCalledWith(workspaceId); }); }); diff --git a/src/node/services/workspaceService.ts b/src/node/services/workspaceService.ts index 3e24599c35..f5adbeae74 100644 --- a/src/node/services/workspaceService.ts +++ b/src/node/services/workspaceService.ts @@ -642,7 +642,7 @@ type WorkspaceDevToolsCleanup = Pick Date: Wed, 26 Aug 2026 16:41:01 +0100 Subject: [PATCH 2/5] Route cascade removal through TaskService with full safeguards Address Codex review comments by replacing the inline cascade loop in WorkspaceService.removeUnlocked() with a dedicated TaskService method (cascadeRemoveInactiveDescendantsWhileTaskTreeLocked) that includes: - Git-patch-artifact lock + wait before removal (comment #3) - Ownership tombstone persistence (comment #6) - Force-flag passthrough from parent instead of hardcoding true (comment #1) - Active/streaming safety checks per descendant The new method is designed to run inside an already-held task-tree lifecycle lock, avoiding deadlock by calling removeWhileTaskTreeLocked directly. --- src/node/services/taskService.ts | 70 +++++++++++++++++++ .../services/taskWorkspaceSeam.testUtils.ts | 2 +- src/node/services/taskWorkspaceSeam.ts | 5 +- src/node/services/workspaceService.test.ts | 2 +- src/node/services/workspaceService.ts | 18 ++--- 5 files changed, 85 insertions(+), 12 deletions(-) diff --git a/src/node/services/taskService.ts b/src/node/services/taskService.ts index 2fd2490a51..06a5e4f4bd 100644 --- a/src/node/services/taskService.ts +++ b/src/node/services/taskService.ts @@ -8827,6 +8827,76 @@ export class TaskService implements AgentTaskIntegration { return ids.sort((a, b) => (depthById.get(b) ?? 0) - (depthById.get(a) ?? 0)); } + /** + * Cascade-remove all inactive descendant agent tasks deepest-first. + * Must be called while the task-tree lifecycle lock is already held (the caller + * in WorkspaceService.remove() acquires it). Includes all safeguards from + * removeInactiveDescendantAgentTask: active/streaming checks, git-patch-artifact + * wait, tombstone persistence, and force-flag passthrough. + */ + async cascadeRemoveInactiveDescendantsWhileTaskTreeLocked( + workspaceId: string, + force: boolean + ): Promise> { + const descendantIds = this.listDescendantAgentTaskIdsDeepestFirst(workspaceId); + + for (const descendantId of descendantIds) { + const config = this.config.loadConfigOrDefault(); + const entry = findWorkspaceEntry(config, descendantId); + if (entry == null) continue; // already removed + + // Safety: active tasks should have been caught by the guard in removeUnlocked, + // but double-check to avoid removing a task that became active in the meantime. + if ( + this.isActiveAgentTaskEntry({ ...entry.workspace, projectPath: entry.projectPath }) || + this.aiService.isStreaming(descendantId) + ) { + return Err( + `Descendant workspace ${descendantId} is still active. Stop it before removing.` + ); + } + + const result = await this.gitPatchArtifactService.withOperationLock( + descendantId, + async () => { + // Wait for any in-flight format-patch job before removal so the artifact + // isn't lost. Refuse removal if a durable pending marker remains (needs the + // child worktree to recover). + await this.gitPatchArtifactService.waitForGeneration(descendantId); + const parentWsId = entry.workspace.parentWorkspaceId; + if (parentWsId) { + const patchArtifact = await readSubagentGitPatchArtifact( + path.join(this.config.sessionsDir, parentWsId), + descendantId + ); + if (patchArtifact?.status === "pending") { + return Err( + `Cannot cascade-remove descendant ${descendantId}: git patch artifact is still pending.` + ); + } + } + + const tombstoneResult = await this.persistRemovedAgentTaskTombstones(descendantId); + if (!tombstoneResult.success) { + return Err( + `Failed to persist tombstones for descendant ${descendantId}: ${tombstoneResult.error}` + ); + } + + return await this.workspaceService.removeWhileTaskTreeLocked(descendantId, force); + } + ); + + if (!result.success) { + return Err( + `Failed to cascade-remove descendant workspace ${descendantId}: ${result.error}` + ); + } + } + + return Ok(undefined); + } + hasActiveDescendantAgentTasksForWorkspace(workspaceId: string): boolean { assert( workspaceId.length > 0, diff --git a/src/node/services/taskWorkspaceSeam.testUtils.ts b/src/node/services/taskWorkspaceSeam.testUtils.ts index 9eac2e2570..8be5cd231b 100644 --- a/src/node/services/taskWorkspaceSeam.testUtils.ts +++ b/src/node/services/taskWorkspaceSeam.testUtils.ts @@ -65,7 +65,7 @@ export function makeAgentTaskIntegrationFake( operation(), hasDescendantAgentTasks: () => false, hasActiveDescendantAgentTasksForWorkspace: () => false, - listDescendantAgentTaskIdsDeepestFirst: () => [], + cascadeRemoveInactiveDescendantsWhileTaskTreeLocked: () => Promise.resolve(Ok(undefined)), hasActiveTopLevelWorkflowRunsForWorkspace: () => Promise.resolve(false), getAgentTaskStatus: () => undefined, resetAutoResumeCount: () => undefined, diff --git a/src/node/services/taskWorkspaceSeam.ts b/src/node/services/taskWorkspaceSeam.ts index 9935aefb65..31064fde4c 100644 --- a/src/node/services/taskWorkspaceSeam.ts +++ b/src/node/services/taskWorkspaceSeam.ts @@ -514,7 +514,10 @@ export interface AgentTaskIntegration { withTaskTreeLifecycleLock(workspaceId: string, operation: () => Promise): Promise; hasDescendantAgentTasks(workspaceId: string): boolean; hasActiveDescendantAgentTasksForWorkspace(workspaceId: string): boolean; - listDescendantAgentTaskIdsDeepestFirst(workspaceId: string): string[]; + cascadeRemoveInactiveDescendantsWhileTaskTreeLocked( + workspaceId: string, + force: boolean + ): Promise>; hasActiveTopLevelWorkflowRunsForWorkspace(workspaceId: string): Promise; getAgentTaskStatus(workspaceId: string): AgentTaskStatus | null | undefined; resetAutoResumeCount(workspaceId: string): void; diff --git a/src/node/services/workspaceService.test.ts b/src/node/services/workspaceService.test.ts index 5c2363d176..ab837acd1c 100644 --- a/src/node/services/workspaceService.test.ts +++ b/src/node/services/workspaceService.test.ts @@ -12378,7 +12378,7 @@ describe("WorkspaceService remove lifecycle coordination", () => { makeAgentTaskIntegrationFake({ withTaskTreeLifecycleLock: runWithTaskTreeLifecycleLock, hasActiveDescendantAgentTasksForWorkspace, - listDescendantAgentTaskIdsDeepestFirst: mock(() => []), + cascadeRemoveInactiveDescendantsWhileTaskTreeLocked: mock(async () => Ok(undefined)), }) ); diff --git a/src/node/services/workspaceService.ts b/src/node/services/workspaceService.ts index f5adbeae74..f43b7d1d05 100644 --- a/src/node/services/workspaceService.ts +++ b/src/node/services/workspaceService.ts @@ -5558,15 +5558,15 @@ export class WorkspaceService extends EventEmitter implements WorkspaceHost { return Err(DESCENDANT_WORKSPACE_REMOVE_ERROR); } - const descendantIds = - this.agentTaskIntegration?.listDescendantAgentTaskIdsDeepestFirst(workspaceId) ?? []; - for (const descendantId of descendantIds) { - const childResult = await this.removeUnlocked(descendantId, true); - if (!childResult.success) { - return Err( - `Failed to cascade-remove descendant workspace ${descendantId}: ${childResult.error}` - ); - } + // Cascade-remove inactive descendants through TaskService, which handles + // git-patch-artifact waits, ownership tombstones, and force-flag passthrough. + const cascadeResult = + await this.agentTaskIntegration?.cascadeRemoveInactiveDescendantsWhileTaskTreeLocked( + workspaceId, + force + ); + if (cascadeResult != null && !cascadeResult.success) { + return Err(cascadeResult.error); } // Stop any active stream before deleting metadata/config to avoid tool calls racing with removal. From ce2304689219eb5287bf3b7de9e8c5644e019043 Mon Sep 17 00:00:00 2001 From: Ethan Dickson Date: Thu, 27 Aug 2026 10:28:41 +0100 Subject: [PATCH 3/5] Cap depth-walk in listDescendantAgentTaskIdsDeepestFirst to prevent hang on corrupted parentWorkspaceId cycles --- src/node/services/taskService.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/node/services/taskService.ts b/src/node/services/taskService.ts index 06a5e4f4bd..4f294afe01 100644 --- a/src/node/services/taskService.ts +++ b/src/node/services/taskService.ts @@ -8813,11 +8813,14 @@ export class TaskService implements AgentTaskIntegration { // Sort by depth (deepest first) so leaf children are removed before their parents. // Ties are broken by insertion order (stable sort). + // Cap the parent-chain walk at ids.length to avoid hanging on corrupted + // parentWorkspaceId cycles (depth can never exceed the descendant count). + const maxDepth = ids.length; const depthById = new Map(); for (const id of ids) { let depth = 0; let current: string | undefined = id; - while (current != null && current !== workspaceId) { + while (current != null && current !== workspaceId && depth < maxDepth) { depth++; current = index.parentById.get(current); } From d3adfc65541dcc69179a6d0f3ecbb2d654c9ead9 Mon Sep 17 00:00:00 2001 From: Ethan Dickson Date: Thu, 27 Aug 2026 10:32:48 +0100 Subject: [PATCH 4/5] Fix require-await lint: use Promise.resolve instead of async arrow --- src/node/services/workspaceService.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node/services/workspaceService.test.ts b/src/node/services/workspaceService.test.ts index ab837acd1c..774cf2387f 100644 --- a/src/node/services/workspaceService.test.ts +++ b/src/node/services/workspaceService.test.ts @@ -12378,7 +12378,7 @@ describe("WorkspaceService remove lifecycle coordination", () => { makeAgentTaskIntegrationFake({ withTaskTreeLifecycleLock: runWithTaskTreeLifecycleLock, hasActiveDescendantAgentTasksForWorkspace, - cascadeRemoveInactiveDescendantsWhileTaskTreeLocked: mock(async () => Ok(undefined)), + cascadeRemoveInactiveDescendantsWhileTaskTreeLocked: mock(() => Promise.resolve(Ok(undefined))), }) ); From 58448dd326ba55b3acbac8cb44327123ecd64aa1 Mon Sep 17 00:00:00 2001 From: Ethan Dickson Date: Thu, 27 Aug 2026 10:37:19 +0100 Subject: [PATCH 5/5] Fix prettier formatting --- src/node/services/workspaceService.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/node/services/workspaceService.test.ts b/src/node/services/workspaceService.test.ts index 774cf2387f..073c5a3103 100644 --- a/src/node/services/workspaceService.test.ts +++ b/src/node/services/workspaceService.test.ts @@ -12378,7 +12378,9 @@ describe("WorkspaceService remove lifecycle coordination", () => { makeAgentTaskIntegrationFake({ withTaskTreeLifecycleLock: runWithTaskTreeLifecycleLock, hasActiveDescendantAgentTasksForWorkspace, - cascadeRemoveInactiveDescendantsWhileTaskTreeLocked: mock(() => Promise.resolve(Ok(undefined))), + cascadeRemoveInactiveDescendantsWhileTaskTreeLocked: mock(() => + Promise.resolve(Ok(undefined)) + ), }) );