feat: add workspace project lifecycle teardown#2243
Conversation
ee6b937 to
4852308
Compare
neversettle17-101
left a comment
There was a problem hiding this comment.
Review: requesting changes.
The per-repo lifecycle (DestroyWorkspaceProjectWorktree, Restore*, Stash*, ApplyPreserved*) and the manager fan-out across Kill / Cleanup / SaveAndTeardownAll / RestoreAll are well factored, and the state-machine column values used (active/removed/unavailable/retry_remove) all satisfy the session_worktrees.state CHECK constraint. Test coverage is good. Two issues should be resolved before merge:
-
Kill of a dirty workspace project silently force-destroys the worktree, breaking the safety contract that single-repo Kill upholds. In
Kill, a single-repo session with uncommitted changes returns(false, nil)and leaves the worktree intact (m.workspace.Destroyrefuses onErrWorkspaceDirty). The workspace path instead stashes the dirty work to a preserved ref, marks the rowunavailable, andForceDestroys the worktree — so the user's working tree is removed where the single-repo equivalent would have protected it. Worse,RestoreAllonly replays rows in stateremoved(restorableWorktreeRows), so theunavailablepreserved ref captured here is never auto-restored — the work is recoverable only by digging the git ref out manually. Please either make workspace Kill refuse-on-dirty to match single-repo semantics, or confirm this destructive divergence is intended and surface the preserved ref to the user. (see inline) -
Stale comments now contradict the code. This PR turns
session_worktrees.stateinto a live lifecycle field, but the explanatory comments inbackend/internal/domain/project.go(theStatefield: "never set by any live code path and always resolves to the column default") andbackend/internal/storage/sqlite/store/session_worktree_store.go(UpsertSessionWorktree/sessionWorktreeFromGen: "unused multi-repo scaffolding ... no live code path sets ... State") still describe it as dead scaffolding. Those files aren't in this diff, but the PR makes their comments false and will mislead the next reader — please update them as part of this change.
Minor: sessionWorktreeRowsToRepoInfos returns a hard error if a worktree row references a child repo that has since been deregistered from the project, which aborts Kill/Cleanup/Restore for the whole session. Worth tolerating an orphaned child row (e.g. skip-with-warn) so one deregistered repo can't strand a session.
|
Addressed review 4598893233 in cbf35e8.
Validation: cd backend && go test ./... |
neversettle17-101
left a comment
There was a problem hiding this comment.
Re-review: approve. All three points from the previous round are resolved by fix: preserve dirty workspace project kills.
- Kill safety contract restored.
destroyWorkspaceProjectRowsnow takes apreserveDirtyflag:Killpassesfalse, so a dirty worktree returnsErrWorkspaceDirtyandKillmaps it to(false, nil)— refusing and leaving the worktree intact, matching single-repo Kill.Cleanuppassestrueto keep the stash-and-force-destroy behavior, which is the right split. - Stale comments fixed. The
Statefield doc indomain/project.goand both comments insession_worktree_store.gonow correctly describestateas a live lifecycle field. - Deregistered child repo no longer strands the session.
sessionWorktreeRowsToRepoInfosnow warn-logs and skips an unregistered repo row instead of hard-erroring.
Verified: packages build and internal/session_manager tests pass. One small non-blocking note inline.
| if err := adapter.DestroyWorkspaceProjectWorktree(ctx, rows[i]); err != nil { | ||
| preservedRef := "" | ||
| if errors.Is(err, ports.ErrWorkspaceDirty) { | ||
| if !preserveDirty { |
There was a problem hiding this comment.
Non-blocking: with preserveDirty=false, this returns on the first dirty repo, but any clean child processed earlier in the reverse loop has already been removed and marked unavailable. So a refused Kill of a multi-repo session can leave a partial teardown (some repos gone, session still live). A retry recovers cleanly since DestroyWorkspaceProjectWorktree is a no-op on an already-removed path, so this is fine to leave — just flagging that the refusal isn't atomic across repos.
Summary
Scope
This is the next backend milestone after workspace-aware spawn in #2224: multi-worktree kill/cleanup/shutdown/restore using session_worktrees rows. It does not include frontend work, workspace-aware SCM observation, root land/discard APIs, or full workspace status aggregation.
Tests