Skip to content

fix: report project removal teardown blockers#2783

Open
AgentWrapper wants to merge 1 commit into
mainfrom
ao/agent-orchestrator-3/fix-project-delete-teardown-conflicts
Open

fix: report project removal teardown blockers#2783
AgentWrapper wants to merge 1 commit into
mainfrom
ao/agent-orchestrator-3/fix-project-delete-teardown-conflicts

Conversation

@AgentWrapper

Copy link
Copy Markdown
Owner

Summary

  • add structured project teardown outcomes for killed, cleaned, and blocked sessions
  • return PROJECT_REMOVE_BLOCKED as a 409 when dirty or failed session workspace teardown prevents safe project deletion
  • cover dirty live worktree, stale/prunable cleanup, non-dirty cleanup failure, project-service conflict handling, and HTTP conflict details

Fixes #2598

Tests

  • cd backend && go test ./internal/service/project ./internal/service/session ./internal/session_manager ./internal/httpd/controllers

Risks

  • Project deletion now blocks when teardown reports preserved or failed session workspaces; users must resolve the listed sessions before retrying.

@Nyrokume Nyrokume left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: changes requested

Right direction (aggregate teardown outcomes, return PROJECT_REMOVE_BLOCKED instead of 500), but the blocker policy conflicts with #2598's main requirement: stale/non-dirty local state must not block unregister.

Must fix

  1. Non-dirty cleanup skips become blockers ??? every cleanup.Skipped is appended, including workspace teardown failed. #2598 says missing/prunable/stale worktree metadata should be best-effort and allow unregister; only preserved user work should conflict. Mirror #2599's UserWorkPreserved (or equivalent) gate.
  2. Kill failures block removal ??? session teardown failed blockers prevent delete on the exact stale-runtime cases the issue wants to tolerate. Prefer best-effort continue (log the cause) unless you can prove the failure protects user data.
  3. Happy-path test does not assert empty blockers ??? default fakeCommander.Cleanup still returns a dirty skip for mer-2, so TeardownProject returns blockers while the test only checks err == nil. Assert len(out.Blockers)==0 (and override cleanup result) so Remove cannot silently conflict on the "success" fixture.
  4. OpenAPI/frontend schema omit 409 for DELETE /projects ??? wire the conflict in specgen + regenerate like #2599, or clients keep treating this as an unexpected status.

Notes

  • Duplicate of #2599 on the same issue; #2599's dirty-vs-stale split is closer to the product ask. Close one after the other lands.
  • Blockers lack path/recoverySteps, so the UI still has weak recovery guidance even when the 409 is correct.

SessionID: skip.SessionID,
Phase: "cleanup",
Reason: skip.Reason,
})

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Product mismatch with #2598: promoting every cleanup skip to a blocker means non-dirty workspace teardown failed prevents project unregister. Stale/prunable/missing worktree metadata must be best-effort. Only treat skips that preserve user work (e.g. ErrWorkspaceDirty / a UserWorkPreserved flag) as blockers.

SessionID: rec.ID,
Phase: "kill",
Reason: "session teardown failed",
})

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turning all Kill errors into blockers re-blocks delete on stale runtime/workspace failures ??? the primary #2598 failure mode. Prefer best-effort continue + Warn log (and only conflict when user work is preserved).

svc := &Service{manager: fc, store: st}

if err := svc.TeardownProject(context.Background(), "mer"); err != nil {
if _, err := svc.TeardownProject(context.Background(), "mer"); err != nil {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test ignores the outcome's blockers while the default fake cleanup still skips mer-2 as dirty. Assert len(out.Blockers) == 0 and pass an empty cleanupResult, otherwise Remove would 409 on the supposed happy path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(projects): project deletion should tolerate stale legacy local state

3 participants