From 4e5a5a15026f269275d9b215394b164146c0e898 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 ++++++++++++++++++++++ src/node/services/workspaceService.test.ts | 11 ++++---- src/node/services/workspaceService.ts | 18 +++++++++++-- 3 files changed, 52 insertions(+), 7 deletions(-) diff --git a/src/node/services/taskService.ts b/src/node/services/taskService.ts index 7e43d21fde..853a09d696 100644 --- a/src/node/services/taskService.ts +++ b/src/node/services/taskService.ts @@ -10408,6 +10408,36 @@ export class TaskService { 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/workspaceService.test.ts b/src/node/services/workspaceService.test.ts index ec69f044d2..2acdf05391 100644 --- a/src/node/services/workspaceService.test.ts +++ b/src/node/services/workspaceService.test.ts @@ -12360,7 +12360,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: { @@ -12378,22 +12378,23 @@ describe("WorkspaceService remove lifecycle coordination", () => { } } ); - const hasDescendantAgentTasks = mock(() => { + const hasActiveDescendantAgentTasksForWorkspace = mock(() => { expect(insideLifecycleLock).toBe(true); return true; }); workspaceService.setTaskService({ withTaskTreeLifecycleLock, - hasDescendantAgentTasks, + hasActiveDescendantAgentTasksForWorkspace, + listDescendantAgentTaskIdsDeepestFirst: mock(() => []), } as unknown as TaskService); 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 6393454098..e1115cdf74 100644 --- a/src/node/services/workspaceService.ts +++ b/src/node/services/workspaceService.ts @@ -605,7 +605,7 @@ type WorkspaceRuntimeStatus = "running" | "stopped" | "unknown" | "unsupported"; const POST_COMPACTION_METADATA_REFRESH_DEBOUNCE_MS = 100; const DESCENDANT_WORKSPACE_REMOVE_ERROR = - "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."; export interface ArchiveWorkspaceOptions { /** * Refuse to archive when the effective worktree archive behavior would delete the checkout @@ -6154,10 +6154,24 @@ export class WorkspaceService extends EventEmitter { // Try to remove from runtime (filesystem) try { - if (this.taskService?.hasDescendantAgentTasks?.(workspaceId) === true) { + // Block deletion when active descendants are still running — the user must + // stop them first. Inactive (reported/interrupted) descendants are cascade-removed + // deepest-first so persistent sub-agents don't permanently block parent deletion. + if (this.taskService?.hasActiveDescendantAgentTasksForWorkspace?.(workspaceId) === true) { return Err(DESCENDANT_WORKSPACE_REMOVE_ERROR); } + const descendantIds = + this.taskService?.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}` + ); + } + } + // Stop any active stream before deleting metadata/config to avoid tool calls racing with removal. // // IMPORTANT: AIService forwards "stream-abort" asynchronously after partial cleanup. If we roll up From 7da5387e1b51360d8429f61f5e8442965a8eb0ee Mon Sep 17 00:00:00 2001 From: Ethan Dickson 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 ++++++++++++++++++++++ src/node/services/workspaceService.test.ts | 2 +- src/node/services/workspaceService.ts | 18 +++--- 3 files changed, 80 insertions(+), 10 deletions(-) diff --git a/src/node/services/taskService.ts b/src/node/services/taskService.ts index 853a09d696..2fb6225758 100644 --- a/src/node/services/taskService.ts +++ b/src/node/services/taskService.ts @@ -10438,6 +10438,76 @@ export class TaskService { 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( + this.config.getSessionDir(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/workspaceService.test.ts b/src/node/services/workspaceService.test.ts index 2acdf05391..d5dbc6b1f1 100644 --- a/src/node/services/workspaceService.test.ts +++ b/src/node/services/workspaceService.test.ts @@ -12385,7 +12385,7 @@ describe("WorkspaceService remove lifecycle coordination", () => { workspaceService.setTaskService({ withTaskTreeLifecycleLock, hasActiveDescendantAgentTasksForWorkspace, - listDescendantAgentTaskIdsDeepestFirst: mock(() => []), + cascadeRemoveInactiveDescendantsWhileTaskTreeLocked: mock(async () => Ok(undefined)), } as unknown as TaskService); expect(await workspaceService.remove(workspaceId, true)).toEqual( diff --git a/src/node/services/workspaceService.ts b/src/node/services/workspaceService.ts index e1115cdf74..5e97f7ce84 100644 --- a/src/node/services/workspaceService.ts +++ b/src/node/services/workspaceService.ts @@ -6161,15 +6161,15 @@ export class WorkspaceService extends EventEmitter { return Err(DESCENDANT_WORKSPACE_REMOVE_ERROR); } - const descendantIds = - this.taskService?.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.taskService?.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 007ee8579b241a02ae337421832d9678d36c9d5c 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 2fb6225758..350bbc1bcf 100644 --- a/src/node/services/taskService.ts +++ b/src/node/services/taskService.ts @@ -10424,11 +10424,14 @@ export class TaskService { // 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 a0e600fac7a2ef75b9fde316070fc6eadb46b752 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 d5dbc6b1f1..beae7f216a 100644 --- a/src/node/services/workspaceService.test.ts +++ b/src/node/services/workspaceService.test.ts @@ -12385,7 +12385,7 @@ describe("WorkspaceService remove lifecycle coordination", () => { workspaceService.setTaskService({ withTaskTreeLifecycleLock, hasActiveDescendantAgentTasksForWorkspace, - cascadeRemoveInactiveDescendantsWhileTaskTreeLocked: mock(async () => Ok(undefined)), + cascadeRemoveInactiveDescendantsWhileTaskTreeLocked: mock(() => Promise.resolve(Ok(undefined))), } as unknown as TaskService); expect(await workspaceService.remove(workspaceId, true)).toEqual( From 8111f648a54766d89eaf2aaa2bcd3fef6a528b5e 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 beae7f216a..5473898bd1 100644 --- a/src/node/services/workspaceService.test.ts +++ b/src/node/services/workspaceService.test.ts @@ -12385,7 +12385,9 @@ describe("WorkspaceService remove lifecycle coordination", () => { workspaceService.setTaskService({ withTaskTreeLifecycleLock, hasActiveDescendantAgentTasksForWorkspace, - cascadeRemoveInactiveDescendantsWhileTaskTreeLocked: mock(() => Promise.resolve(Ok(undefined))), + cascadeRemoveInactiveDescendantsWhileTaskTreeLocked: mock(() => + Promise.resolve(Ok(undefined)) + ), } as unknown as TaskService); expect(await workspaceService.remove(workspaceId, true)).toEqual(