Skip to content

fix: make worktree lifecycle failures durable - #2719

Open
timigod wants to merge 40 commits into
getpaseo:mainfrom
timigod:review-v025-worktree-lifecycle-upstream
Open

fix: make worktree lifecycle failures durable#2719
timigod wants to merge 40 commits into
getpaseo:mainfrom
timigod:review-v025-worktree-lifecycle-upstream

Conversation

@timigod

@timigod timigod commented Aug 1, 2026

Copy link
Copy Markdown

Linked issue

Closes #2720

Type of change

  • Bug fix
  • New feature (with prior issue + design alignment)
  • Refactor / code improvement
  • Docs

What does this PR do

Worktree setup, ownership attachment, archive, physical cleanup, and replacement workspace creation currently cross several independent asynchronous paths. Late ownership or workspace creation can race archive, and a failed cleanup can be reported as success or lose the identity needed for a safe retry.

This focused daemon/protocol change gives those operations one lifecycle boundary:

  • archive claims the workspace and drains already-reserved ownership mutations;
  • agent creation, MCP terminals, and desktop terminals validate active workspace ownership after acquiring that reservation;
  • project removal uses the same full lifecycle archive path as direct workspace removal;
  • logical archive and physical cleanup failures remain visible through command, WebSocket, MCP, and session responses;
  • explicit unknown or stale workspace IDs fail instead of reporting a successful no-op;
  • failed cleanup persists every sibling teardown obligation behind an exact worktree-incarnation fence;
  • terminal-state auto-archive retries transient failures with capped backoff, waits for workspace-registry mutations instead of timer-spinning while physical cleanup is pending, and fences mutations that land during cleanup failure;
  • retry uses a cleanup-only API: it cannot invoke normal workspace, agent, or terminal archive scope when a path has been reused by a live replacement;
  • directory workspace creation and unarchive share cleanup's directory exclusion and revalidate directory existence after waiting, so a workspace cannot be minted while its cwd is being removed;
  • directory setup and cleanup locks are keyed by realpath-aware filesystem identity, so symlink and macOS path aliases cannot bypass that exclusion;
  • daemon shutdown drains registered create-agent lifecycle work, aborts and awaits its active cleanup cycle, and terminates a hanging teardown subprocess tree before shutdown completes; and
  • setup output is bounded while preserving its diagnostic tail.

It contains no provider-health behavior, fleet topology, Plexer policy, machine paths, release bump, or unrelated local patches.

How did you verify it

The lifecycle-only candidate was rebuilt directly from official stable v0.2.5 (6fc491e6220fba6543bbbe4bf1b1f58cfe59228b), then merged normally with upstream main at 70ed70d36e2eabd3876a22931954319ec202c902. Review repairs were added normally on top; no force push or history rewrite was used.

Exact submitted head: bb956ee4c7640c39d14a7984c1695650fa57a8bb (tree 2e019cb70c5e68d986f2ab29d99cc5c90508b748).

Exact focused lifecycle-dispatch amendment: bb956ee4c7640c39d14a7984c1695650fa57a8bb, reconstructed only from frozen sources 67e23daf21868648ec2a696c38a142b4af053bb6 and 5de7e1d0b246b688cc568041c66d56da46c86318. No fork receipt or manifest file was changed.

Exact cleanup-only/cancellation source repair: 5e17fcf769cb855326683413cdcd9b3cbe82ba43 (tree e83f37f06eeb44d5b87ff30d5ca18c0d25288f5b).

Exact cleanup/create exclusion source repair: 5c70d522437ea0c02c1e769ee5f9975b8077811e (tree 1b4b0a140a483504225e043bfdd5cf1f26e7f875).

Exact filesystem-identity lock repair: 759ed14f0b24499f0a3f161cf64c920ac3393c4b (tree 602929fb3aa5106fecf438ab0b4d3570d7daa62b).

  • Focused amendment proof: npx vitest run packages/server/src/server/agent/create-agent-lifecycle-dispatch.test.ts packages/server/src/server/bootstrap.smoke.test.ts --bail=1 passed 26 tests in 2 files.
  • Amendment regressions prove cleanup-pending waits for a registry mutation without timer polling, a concurrent mutation cannot be lost at the cleanup-failure boundary, shutdown waits for an active archive, and a shutdown deadline reports the pending agent ID.
  • Final expanded proof: 14 lifecycle, provisioning, and affected call-site test files passed; 509 tests passed and 5 platform-specific tests skipped.
  • The deterministic cleanup/create regression pauses cleanup at its final owner read, starts replacement creation through a symlink spelling of the same worktree, proves creation remains blocked, then proves post-cleanup revalidation rejects the removed directory without minting a live workspace record.
  • Regression proof also covers stale cleanup A at a reused path versus already-live replacement B, and an active hanging teardown child canceled during shutdown.
  • Server build and server typecheck passed on Node 24.
  • Repository pre-commit lint, formatting, and all-workspace typecheck passed on the exact submitted head.
  • Full repository lint passed with 0 warnings and 0 errors; full formatting and diff checks passed.
  • Earlier broad lifecycle proof remains green: 526 server tests passed (6 skipped) and 470 protocol tests passed.
  • GitHub reports the exact submitted head mergeable with no content conflict.

The stable verification commits and receipts remain in patches/ so the seam is auditable and can be dropped when an equivalent upstream release lands.

This contribution was developed with AI assistance; every verification claim above comes from an actual run against the submitted branch.

Checklist

  • One focused change. Unrelated cleanups split out.
  • npm run typecheck passes
  • npm run lint passes for every changed file
  • Formatting ran and passes
  • UI changes include screenshots or video for every affected platform (n/a — daemon/protocol only)
  • Tests added or updated where it made sense

@greptile-apps

greptile-apps Bot commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR makes workspace and worktree lifecycle failures durable across archive, cleanup, provisioning, agent creation, terminal creation, retries, and daemon shutdown.

  • Adds persisted cleanup and auto-archive obligations with incarnation and filesystem-identity fencing.
  • Routes workspace and project removal through coordinated lifecycle archival and exposes cleanup outcomes over protocol responses.
  • Adds cancellation, retry, shutdown-drain, bounded-output, and race-focused regression coverage.

Confidence Score: 4/5

The PR should not merge until explicit workspace archives cannot report success when the workspace disappears between validation and archive resolution.

The original unresolved-path case now fails explicitly, but the explicit-workspace fix performs two independent registry reads; a concurrent mutation between them causes the archive service to resolve an empty target and return a successful no-op.

Files Needing Attention: packages/server/src/server/worktree/commands.ts and packages/server/src/server/workspace-archive-service.ts

Important Files Changed

Filename Overview
packages/server/src/server/worktree/commands.ts Adds explicit unknown-target failures and cleanup result fields, but the separate validation and archive lookups retain a fabricated-success race.
packages/server/src/server/workspace-archive-service.ts Centralizes logical archive, teardown, physical cleanup, pending-cleanup persistence, and incarnation-fenced retry behavior.
packages/server/src/server/workspace-lifecycle-coordinator.ts Coordinates workspace lifecycle reservations so archive can drain ownership mutations and block conflicting creation.
packages/server/src/server/workspace-cleanup-retry-service.ts Adds mutation-driven, backoff-capped cleanup retries through the cleanup-only archive path.
packages/server/src/server/agent/create-agent-lifecycle-dispatch.ts Persists pending agent creation and auto-archive obligations while coordinating shutdown and lifecycle cleanup.
packages/protocol/src/messages.ts Extends archive responses with backward-compatible optional physical-cleanup status fields.

Sequence Diagram

sequenceDiagram
  participant C as Client
  participant Cmd as Archive command
  participant Reg as Workspace registry
  participant Arc as Archive service
  participant FS as Filesystem
  C->>Cmd: Archive workspaceId
  Cmd->>Reg: Validate active workspace
  Reg-->>Cmd: Workspace exists
  Cmd->>Arc: Archive by workspaceId
  Arc->>Reg: Resolve workspace again
  Reg-->>Arc: Workspace or cleanup obligation
  Arc->>FS: Remove fenced backing directory
  Arc-->>Cmd: Archive and cleanup outcome
  Cmd-->>C: Success, pending cleanup, or error
Loading

Reviews (6): Last reviewed commit: "test(server): keep git discovery on real..." | Re-trigger Greptile

@timigod
timigod marked this pull request as draft August 1, 2026 01:04
@timigod

timigod commented Aug 1, 2026

Copy link
Copy Markdown
Author

A fresh human exact-tip review found four lifecycle gaps that the current tests and Greptile pass did not cover: non-MCP terminal/project-removal paths can bypass the archive reservation; merge auto-archive does not persist cleanup retry intent; explicit unknown workspace IDs still return success; and a partial multi-workspace retry can omit an archived sibling teardown obligation. I have returned this PR to draft while I add the exact regressions and repair those paths. The branch and existing evidence remain unchanged until the focused follow-up is verified.

@timigod

timigod commented Aug 1, 2026

Copy link
Copy Markdown
Author

The four exact-tip lifecycle gaps reported in the prior draft comment are now repaired at d5743f9, with the proof receipt at submitted head d3530db. Added regressions cover post-archive agent/MCP/desktop terminal attachment, project-removal fencing, durable merge-autoarchive cleanup state, explicit unknown workspace IDs, and partial sibling cleanup retries. Node 24 proof: 317 passed / 4 skipped, final ownership subset 144/144, server build/typecheck and changed-file lint/format all pass. Returning the PR for exact-head review.

@timigod
timigod marked this pull request as ready for review August 1, 2026 01:24
@timigod
timigod marked this pull request as draft August 1, 2026 01:24
@timigod

timigod commented Aug 1, 2026

Copy link
Copy Markdown
Author

The four human-review P1 classes have been repaired at exact head d3530db with focused and package proof. Returning the PR to draft for one independent exact-tip review of the repaired state; implementer proof alone is not the acceptance boundary.

@timigod

timigod commented Aug 1, 2026

Copy link
Copy Markdown
Author

Follow-up exact-tip review also caught auto-archive false success when a resolver returns an inactive clean workspace or cleanup remains pending. That is now repaired at 4864613 with 16/16 truthfulness tests; submitted head is bd90c39. A separate final-refresh ownership closure race was then identified, so I am keeping this PR in draft while I add that deterministic gate and repair.

@timigod

timigod commented Aug 1, 2026

Copy link
Copy Markdown
Author

The final-refresh ownership closure and pending-cleanup automation seams are now repaired. Archive discovery iterates to a stable target set and waits existing setup/ownership work for every newly discovered sibling (2ee2896); registry I/O failures remain truthful, and created-worktree, schedule, project, hub, and normal workspace automation reject cleanupPending as retryable failure (7e9b8bf). Exact submitted head: 4a54864. Final proof: 182 passed / 4 skipped, closure 26/26, auto-archive 16/16, combined closure/auto 42/42, Node24 server build/typecheck, lint/format/diff green. PR remains draft for another exact-tip review.

@timigod

timigod commented Aug 1, 2026

Copy link
Copy Markdown
Author

Exact-tip follow-up for the final cleanup-retry review: pushed head 4cf635d4e2c1925921bf663169914f40921d5f20.

The repair adds an autonomous daemon cleanup owner that discovers persisted cleanupPending records on startup and after registry mutation, groups archived siblings by verified worktree incarnation and directory, retries once per worktree scope, bounds each cycle, caps backoff, and cancels timers on shutdown. Created-worktree terminal auto-archive now also self-retries without a second terminal event.

New regression proof includes two archived sibling workspaces with distinct teardown directories on one worktree; both teardown obligations run before the single deletion. The 10-file lifecycle/schedule suite passed 387 tests with 4 platform-specific skips; server build/typecheck and changed-file lint/format/diff pass on Node 24.

@timigod

timigod commented Aug 1, 2026

Copy link
Copy Markdown
Author

Second exact-tip retry follow-up: pushed head 2985ba3cf7846c12d9a1c2fd571adab757030f93; repair commit 4bc441a334c61c17fd15f6e5936d1a4f6ce8b915.

Cleanup retry no longer runs on the daemon readiness path: start() only subscribes and schedules, and stop() cancels timers without awaiting an unbounded teardown. Batching now rotates its cursor, so a persistent failure cannot starve later targets; failed batches preserve capped backoff rather than hot-looping through wakeRequested.

Regressions prove a never-resolving cleanup does not block start or stop, and with maxTargetsPerCycle=1, failing target A backs off while target B is still attempted before A is retried. The 10-file suite now passes 388 tests with 4 platform-specific skips; server build/typecheck and changed-file lint/format/diff remain green.

@timigod

timigod commented Aug 1, 2026

Copy link
Copy Markdown
Author

Review follow-up is now pushed at exact head 322039e7a2b568be05c42c449e0aa89dfb7c77a6. The retry path is cleanup-only and fenced by the exact persisted directory plus worktree incarnation; it cannot archive a live replacement that reuses the path. Daemon shutdown now aborts and awaits the owned retry cycle, including tree-killing a hanging teardown child. Final Node 24 proof: 11 files passed, 434 tests passed, 5 skipped; server build/typecheck and repository pre-commit lint/format/all-workspace typecheck passed. The draft is frozen at this SHA for independent exact-tip review.

@timigod

timigod commented Aug 1, 2026

Copy link
Copy Markdown
Author

Final P1 follow-up is pushed at exact head ad3728d20d546716b5d744914be957dfde279010. Directory workspace creation and unarchive now share cleanup directory exclusion and revalidate the directory after waiting. The deterministic regression pauses cleanup at its final owner read, starts replacement creation, proves it waits, then proves cleanup removes the worktree and creation rejects without minting a live record. Expanded Node 24 proof: 14 files passed, 509 tests passed, 5 skipped; server build/typecheck, changed-file lint/format/diff, and repository pre-commit all-workspace typecheck passed. The draft is frozen at this SHA for exact-tip rereview.

@timigod

timigod commented Aug 1, 2026

Copy link
Copy Markdown
Author

Filesystem-identity follow-up is pushed at exact head 7fa508cfd9ec689868af9b680a5389d4fee8dd89. Directory setup tracking and exclusion now key through the repository realpath-aware identity normalizer. The deterministic final-owner-read race creates through a symlink spelling of the same worktree, proves creation remains blocked by cleanup, then proves it rejects after deletion without minting a live record. Expanded proof remains 14 files / 509 passed / 5 skipped, with server build/typecheck, lint/format/diff, and repository pre-commit all-workspace typecheck green. The draft is frozen at this SHA for terminal exact-tip rereview.

@timigod

timigod commented Aug 1, 2026

Copy link
Copy Markdown
Author

Receipt reproducibility follow-up is pushed at exact head 260fa3174ddf0174cc5d1d467ee19b94b619432f. This is docs-only over the reviewed source 759ed14f0b24499f0a3f161cf64c920ac3393c4b: both 509/5 verification entries now contain the complete 14-file Vitest command rather than a placeholder. Repository pre-commit all-workspace typecheck passed. Frozen for terminal exact-tip rereview.

@timigod
timigod marked this pull request as ready for review August 1, 2026 02:44
@timigod

timigod commented Aug 1, 2026

Copy link
Copy Markdown
Author

Terminal independent exact-tip review is clean at 260fa3174ddf0174cc5d1d467ee19b94b619432f (tree 712799e4d4e66c37c098efe49b137f8a003f9090). Independent runtime proof confirms equivalent /var and /private/var paths share the lock, the symlink cleanup/create race is closed, and the complete Node 22 receipt rerun passed 14 files / 509 tests with 5 platform skips. No P1/P2 findings remain. Marking ready for maintainer review; no merge was performed.

@timigod
timigod marked this pull request as draft August 1, 2026 06:15
@timigod

timigod commented Aug 1, 2026

Copy link
Copy Markdown
Author

Exact-tip lifecycle follow-up is pushed at f128a34dc601fd5f09bc83d85d3d4ce796994e7d (tree 5bc647c29b1b312fcf89da32ed6240394bc3631f).

This repair gives every WebSocket Session the one daemon-owned create-agent lifecycle dispatcher while retaining Session-local worktree creation behavior; reports failed cleanup cancellation as unresolved for durable restart recovery; rearms cleanup only for the workspace IDs named by WorkspaceCleanupPendingError while preserving the mutation race fence; and threads the worker shutdown deadline into resilient daemon teardown so lifecycle budget exhaustion or another step failure cannot skip later critical cleanup.

Focused proof: lifecycle dispatch 11/11; Session 153/153; related Session suites 136 passed / 4 platform skips; WebSocket suites 39/39; bootstrap shutdown 19/19; cleanup restart service 3/3. The create-worktree auto-archive E2E run passed 10 cases while one case hit its 5-second limit under concurrent load; that exact case then passed in isolation (1 passed / 10 skipped). Repository npm run typecheck, npm run lint, npm run format:check, npm run build:server, git diff --check, and the full pre-commit hook all pass. Official getpaseo/paseo main remains 70ed70d36e2eabd3876a22931954319ec202c902, already the branch merge base. Keeping the PR draft for exact-tip review.

@timigod
timigod marked this pull request as ready for review August 1, 2026 13:14
Comment on lines +142 to +145
const result = await archiveByScope(dependencies, {
scope: { kind: "workspace", workspaceId: input.workspaceId },
requestId: input.requestId,
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Concurrent removal reports success

When a registry mutation removes or archives the workspace after requireActiveWorkspaceForArchive succeeds, archiveByScope performs a second lookup that converts the missing workspace into an empty target, causing this command to return ok: true even though nothing was archived.

Context Used: packages/server/AGENTS.md (source)

Knowledge Base Used:

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: workspace archive races setup and can report success after teardown failure

1 participant