From 11524dc3492f06835c56be8ea3a1cc5ebc54fca8 Mon Sep 17 00:00:00 2001 From: Mohamed Boudra Date: Thu, 16 Apr 2026 23:05:41 +0700 Subject: [PATCH] fix(server): make paseo worktree archive idempotent after partial teardown If a first archive attempt removed git's admin dir but failed to clean the working tree (e.g. due to file churn during setup), retrying the archive failed the ownership gate with "Worktree is not a Paseo-owned worktree" because isPaseoOwnedWorktreeCwd required the worktree to still appear in git worktree list. - isPaseoOwnedWorktreeCwd now decides ownership by path shape ($PASEO_HOME/worktrees//[/...]) and treats git as best-effort for resolving repoRoot. - deletePaseoWorktree tolerates a missing admin dir: it skips teardown when the tree is absent, swallows a failing git worktree remove so the rmSync fallback always runs, and follows up with git worktree prune to clear stale admin refs. - Relabel the post-creation setup failure log as "Background worktree setup failed" since the worktree itself already exists at that point. --- .../server/src/server/worktree-session.ts | 2 +- packages/server/src/utils/worktree.test.ts | 90 +++++++++++++++++++ packages/server/src/utils/worktree.ts | 84 +++++++++++------ 3 files changed, 146 insertions(+), 30 deletions(-) diff --git a/packages/server/src/server/worktree-session.ts b/packages/server/src/server/worktree-session.ts index 6edc12bc8..fd21bff89 100644 --- a/packages/server/src/server/worktree-session.ts +++ b/packages/server/src/server/worktree-session.ts @@ -940,7 +940,7 @@ export async function runWorktreeSetupInBackground( worktreePath: worktree.worktreePath, setupStarted, }, - "Background worktree creation failed", + "Background worktree setup failed", ); return; } diff --git a/packages/server/src/utils/worktree.test.ts b/packages/server/src/utils/worktree.test.ts index 9d8b88d56..202b8150a 100644 --- a/packages/server/src/utils/worktree.test.ts +++ b/packages/server/src/utils/worktree.test.ts @@ -789,6 +789,96 @@ describe("paseo worktree manager", () => { expect(existsSync(created.worktreePath)).toBe(true); expect(existsSync(join(repoDir, "teardown-start.log"))).toBe(true); }); + + it("treats a worktree as paseo-owned even when its .git admin is missing", async () => { + const created = await createWorktree({ + branchName: "orphan-admin-branch", + cwd: repoDir, + baseBranch: "main", + worktreeSlug: "orphan-admin", + paseoHome, + }); + + // Simulate a previous archive attempt that removed git's admin dir but left + // the working tree on disk (e.g. because file churn prevented full cleanup). + rmSync(join(repoDir, ".git", "worktrees", "orphan-admin"), { + recursive: true, + force: true, + }); + expect(existsSync(created.worktreePath)).toBe(true); + + const ownership = await isPaseoOwnedWorktreeCwd(created.worktreePath, { paseoHome }); + expect(ownership.allowed).toBe(true); + }); + + it("rejects paths that are not under the paseo worktrees root", async () => { + const outsidePath = join(tempDir, "outside-paseo-home"); + mkdirSync(outsidePath, { recursive: true }); + + const ownership = await isPaseoOwnedWorktreeCwd(outsidePath, { paseoHome }); + + expect(ownership.allowed).toBe(false); + }); + + it("rejects the worktrees root itself and the per-repo hash dir", async () => { + const projectHash = await deriveWorktreeProjectHash(repoDir); + const worktreesRoot = join(paseoHome, "worktrees"); + const projectHashDir = join(worktreesRoot, projectHash); + mkdirSync(projectHashDir, { recursive: true }); + + await expect(isPaseoOwnedWorktreeCwd(worktreesRoot, { paseoHome })).resolves.toMatchObject({ + allowed: false, + }); + await expect(isPaseoOwnedWorktreeCwd(projectHashDir, { paseoHome })).resolves.toMatchObject({ + allowed: false, + }); + }); + + it("deletes a worktree whose .git admin dir has already been removed", async () => { + const created = await createWorktree({ + branchName: "orphan-delete-branch", + cwd: repoDir, + baseBranch: "main", + worktreeSlug: "orphan-delete", + paseoHome, + }); + + rmSync(join(repoDir, ".git", "worktrees", "orphan-delete"), { + recursive: true, + force: true, + }); + expect(existsSync(created.worktreePath)).toBe(true); + + await deletePaseoWorktree({ + cwd: repoDir, + worktreePath: created.worktreePath, + paseoHome, + }); + + expect(existsSync(created.worktreePath)).toBe(false); + }); + + it("is idempotent: deleting an already-absent worktree succeeds", async () => { + const created = await createWorktree({ + branchName: "idempotent-delete-branch", + cwd: repoDir, + baseBranch: "main", + worktreeSlug: "idempotent-delete", + paseoHome, + }); + + await deletePaseoWorktree({ + cwd: repoDir, + worktreePath: created.worktreePath, + paseoHome, + }); + expect(existsSync(created.worktreePath)).toBe(false); + + // Second call — nothing left on disk and no admin entry — must not throw. + await expect( + deletePaseoWorktree({ cwd: repoDir, worktreePath: created.worktreePath, paseoHome }), + ).resolves.toBeUndefined(); + }); }); describe("slugify", () => { diff --git a/packages/server/src/utils/worktree.ts b/packages/server/src/utils/worktree.ts index 14b4f75a5..095b66925 100644 --- a/packages/server/src/utils/worktree.ts +++ b/packages/server/src/utils/worktree.ts @@ -852,37 +852,48 @@ export async function isPaseoOwnedWorktreeCwd( cwd: string, options?: { paseoHome?: string }, ): Promise { - let gitCommonDir: string; - try { - gitCommonDir = await getGitCommonDir(cwd); - } catch { - return { - allowed: false, - worktreePath: normalizePathForOwnership(cwd), - }; - } - const repoRoot = resolveRepoRootFromGitCommonDir(gitCommonDir); - const worktreesRoot = await getPaseoWorktreesRoot(cwd, options?.paseoHome); - const resolvedRoot = normalizePathForOwnership(worktreesRoot) + sep; const resolvedCwd = normalizePathForOwnership(cwd); - if (!resolvedCwd.startsWith(resolvedRoot)) { + // repoRoot is best-effort: git may be unreachable from the worktree (e.g. a + // previous archive attempt removed the admin dir before the working tree + // could be fully cleaned up). We still want to allow archiving in that case. + let repoRoot: string | undefined; + try { + const gitCommonDir = await getGitCommonDir(cwd); + repoRoot = resolveRepoRootFromGitCommonDir(gitCommonDir); + } catch { + // ignore + } + + const paseoHome = options?.paseoHome ? resolve(options.paseoHome) : resolvePaseoHome(); + const paseoWorktreesPrefix = normalizePathForOwnership(join(paseoHome, "worktrees")) + sep; + + // Ownership is defined by the path living under $PASEO_HOME/worktrees//[/...]. + // The / prefix is Paseo-private — nothing else writes there — so the + // path shape alone is sufficient proof of ownership, even when git has already + // forgotten about the worktree. + if (!resolvedCwd.startsWith(paseoWorktreesPrefix)) { return { allowed: false, - repoRoot, - worktreeRoot: worktreesRoot, + ...(repoRoot !== undefined ? { repoRoot } : {}), worktreePath: resolvedCwd, }; } - const worktrees = await listPaseoWorktrees({ cwd, paseoHome: options?.paseoHome }); - const allowed = worktrees.some((entry) => { - const worktreePath = resolve(entry.path); - return resolvedCwd === worktreePath || resolvedCwd.startsWith(worktreePath + sep); - }); + const relative = resolvedCwd.slice(paseoWorktreesPrefix.length); + const parts = relative.split(sep).filter((part) => part.length > 0); + if (parts.length < 2) { + return { + allowed: false, + ...(repoRoot !== undefined ? { repoRoot } : {}), + worktreePath: resolvedCwd, + }; + } + + const worktreesRoot = join(paseoHome, "worktrees", parts[0]!); return { - allowed, - repoRoot, + allowed: true, + ...(repoRoot !== undefined ? { repoRoot } : {}), worktreeRoot: worktreesRoot, worktreePath: resolvedCwd, }; @@ -1039,18 +1050,33 @@ export async function deletePaseoWorktree({ throw new Error("Refusing to delete non-Paseo worktree"); } - await runWorktreeTeardownCommands({ - worktreePath: resolvedWorktree, - }); + if (existsSync(resolvedWorktree)) { + await runWorktreeTeardownCommands({ + worktreePath: resolvedWorktree, + }); + } - await runGitCommand(["worktree", "remove", resolvedWorktree, "--force"], { - cwd, - timeout: 120_000, - }); + try { + await runGitCommand(["worktree", "remove", resolvedWorktree, "--force"], { + cwd, + timeout: 120_000, + }); + } catch { + // `git worktree remove` fails if the admin dir is already gone (e.g. a prior + // archive attempt removed it before the working tree could be fully cleaned + // up). Fall through to the rmSync fallback below so the retry is idempotent. + } if (existsSync(resolvedWorktree)) { rmSync(resolvedWorktree, { recursive: true, force: true }); } + + // Clean up any stale admin entries git may still have cached for this path. + try { + await runGitCommand(["worktree", "prune"], { cwd, timeout: 30_000 }); + } catch { + // not critical; git will prune lazily + } } /**