fix: cascade-remove inactive descendants when deleting archived workspaces - #3982
fix: cascade-remove inactive descendants when deleting archived workspaces#3982ethanndickson wants to merge 5 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3eeef7a926
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
This comment has been minimized.
This comment has been minimized.
…paces 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
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.
5bda4fa to
7da5387
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7da5387e1b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ang on corrupted parentWorkspaceId cycles
|
@codex review |
1 similar comment
|
@codex review |
|
@codex review |
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
1 participant
Summary
Shift+click to bypass the force-delete modal on archived workspaces stopped working after the persistent sub-agent lifecycle change (PR #3825).
Root Cause
removeUnlockedhad a guard usinghasDescendantAgentTasks()that checks for any descendants (active or inactive) and fires before theforceflag is even evaluated. Since PR #3825 made completed sub-agents persist as inactive children in config, any workspace that ever spawned a sub-agent could never be deleted — shift+click bypass, force delete, and normal delete all hit this guard.Fix
hasDescendantAgentTasks→hasActiveDescendantAgentTasksForWorkspace— only blocks deletion when children are actively running (queued/starting/running)removeUnlocked(childId, true). This mirrors whattask_removerequires users to do manually but happens automatically during workspace deletionlistDescendantAgentTaskIdsDeepestFirst()to TaskService for the cascade orderingWhat this fixes
Risks
Low — the safe default (block on active descendants) is preserved. Only inactive (reported/interrupted) descendants are cascade-removed, and each child removal goes through the full
removeUnlockedpath including all cleanup steps.