fix(workspaces): run teardown when deleting never-activated workspaces - #3035
fix(workspaces): run teardown when deleting never-activated workspaces#3035Tyagiquamar wants to merge 2 commits into
Conversation
Greptile SummaryThis PR adds an orphan teardown path so deletion runs workspace teardown even when no live activation exists, and adds contract coverage for retrying a failed teardown.
Confidence Score: 3/5The PR should not merge until removal teardown is consistently consumed across active and orphan paths and inactive worktrees can resolve teardown without a live parent repository record. Active teardown failures are rerun on deletion retry, while parent-record loss can cause an existing worktree's teardown to be skipped before destructive removal. Files Needing Attention: packages/core/src/runtimes/workspace-registry/node/activation.ts, packages/core/src/runtimes/workspace-registry/node/runtime.ts
|
| Filename | Overview |
|---|---|
| packages/core/src/runtimes/workspace-registry/node/activation.ts | Adds orphan teardown execution and in-memory consumption tracking, but active teardown is not recorded in the same consumption domain and parent-dependent config resolution can skip valid worktree teardown. |
| packages/core/src/runtimes/workspace-registry/node/runtime.ts | Routes all removal verbs through the new manager operation while preserving durable handling of returned teardown failures. |
| packages/core/src/runtimes/workspace-registry/node/api/delete-worktree.contract.test.ts | Covers never-activated worktree teardown failure and retry, but not active-failure retry or parent-record-missing deletion. |
Sequence Diagram
sequenceDiagram
participant Delete as Removal verb
participant Manager as Activation manager
participant Config as Lifecycle config
participant Runner as Script runner
Delete->>Manager: deactivateForRemoval(id, path)
alt Live activation exists
Manager->>Runner: active teardown
else Teardown already consumed
Manager-->>Delete: continue removal
else Workspace directory exists
Manager->>Config: resolve teardown policy
Config-->>Manager: teardown command
Manager->>Runner: orphan teardown
Manager->>Manager: mark consumed
end
Manager-->>Delete: teardown result
Prompt To Fix All With AI
### Issue 1
packages/core/src/runtimes/workspace-registry/node/activation.ts:233-234
**Active teardown remains unconsumed**
When an activated workspace's teardown fails during deletion, this branch delegates to `deactivate()` without adding the workspace to `removalTeardowns`. The retry therefore enters the orphan path and executes teardown again instead of proceeding past the settled removal stage; a Stop followed by removal likewise executes a non-idempotent teardown twice.
### Issue 2
packages/core/src/runtimes/workspace-registry/node/activation.ts:237-239
**Parent loss skips worktree teardown**
When an inactive worktree remains on disk after its parent repository record is removed, lifecycle resolution returns no policy before consulting the worktree's own configuration, while deletion can still resolve the repository from disk. This treats the worktree as having no teardown and destroys it without running `.emdash.json`, leaking teardown-managed resources.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix(workspaces): run teardown when delet..." | Re-trigger Greptile
- consume the removal-teardown marker when the active activation's teardown settles, so a failed-teardown retry (or Stop followed by removal) proceeds past it instead of re-running the non-idempotent orphan teardown - keep the delete verbs' session kills: deactivateForRemoval now mirrors Stop's script-then-sessions order with best-effort cleanup - resolve lifecycle config for an orphaned worktree from its own cached .emdash.json when layered resolution has no project root (parent repository record removed); removal still resolves the repository from disk, so its teardown must not be skipped
|
Hi @rabanspiegel, could you please review when you have a chance? Thanks! |
Description
Deleting a task/workspace whose workspace was never activated in the current app run (archived tasks, tasks never opened since launch, or any delete after an app restart) silently skipped the
.emdash.jsonteardown script: all three removal verbs funnel throughdeactivateForRemoval, which ran teardown only viaactivationManager.deactivate()— a no-op without a live activation — and then destroyed the worktree. Archived tasks therefore leaked whatever their teardown was responsible for cleaning up (the reporting case: iOS simulators created by setup).Fix:
WorkspaceActivationManagergainsdeactivateForRemoval(id, path)— the plain activation teardown when an activation exists, otherwise a one-time orphan teardown run through the same runner, time-boxed, notice-reported, and marked consumed even on failure so a retry proceeds past it (same semantics as a spent activation). A missing workspace path has nothing to settle, so already-vanished workspaces still delete. The runtime's removal verbs delegate to it; plaindeactivateWorkspace(the Stop flow) is unchanged.Note: archive of a live task already reaches host deactivation on current main; this closes the remaining destruction-path gap that also covers the archive-then-delete chain from the issue.
Related issues
Fixes #2886
Testing
deleting a worktree that was never activated still runs its teardown script onceindelete-worktree.contract.test.ts: mirrors the existing failing-teardown test minus activation — first delete fails at the durableteardown/transientremoval stage with the artifact intact, retry converges and removes it.pnpm vitest run src/runtimes/workspace-registry/node/api/delete-worktree.contract.test.ts -t "never activated"→ 1 passedpnpm vitest run src/runtimes/workspace-registry/node/api/delete-worktree.contract.test.ts→ same pre-existing results as baseline on this Windows machine (several tests assert POSIX shell output/exit codes that cmd.exe does not produce; verified identical failures with the change stashed)packages/core:pnpm run typecheck→ clean;pnpm run lint→ clean;pnpm run format:check→ cleanScreenshot/Recording (if applicable)
N/A — no UI change.
Checklist
messages and, when possible, the PR title