From 12ea9fc7d2d7c6db46ca73baec828067f27a4ad9 Mon Sep 17 00:00:00 2001 From: Badcuban <108198679+badcuban@users.noreply.github.com> Date: Sat, 14 Nov 2026 12:03:36 -0500 Subject: [PATCH] fix(server): a worktree a thread reached through a symlink can't be removed from under it The removal guard compared the folder being removed with each thread's recorded folders by spelling. git names a worktree by its real path, and a thread can record the same folder through a symlink (a data folder under macOS's /var, which is /private/var), so that thread did not count as in the way and its worktree could be deleted while it was still pointed there. The guard now compares folders as the disk resolves them. The cleanup list still classifies by spelling, so it can offer such a folder; the server then refuses and names the threads. --- .../src/vcs/WorktreeRemovalGuard.test.ts | 29 ++++++++++++++ apps/server/src/vcs/WorktreeRemovalGuard.ts | 38 +++++++++++++++++-- 2 files changed, 64 insertions(+), 3 deletions(-) diff --git a/apps/server/src/vcs/WorktreeRemovalGuard.test.ts b/apps/server/src/vcs/WorktreeRemovalGuard.test.ts index ff86dc49c..87ddef0be 100644 --- a/apps/server/src/vcs/WorktreeRemovalGuard.test.ts +++ b/apps/server/src/vcs/WorktreeRemovalGuard.test.ts @@ -1,3 +1,7 @@ +import * as NodeFs from "node:fs"; +import * as NodeOs from "node:os"; +import * as NodePath from "node:path"; + import { assert, describe, it } from "@effect/vitest"; import * as Data from "effect/Data"; import * as Effect from "effect/Effect"; @@ -46,6 +50,31 @@ describe("ensureWorktreeRemovable", () => { }), ); + // git names a worktree by its real path; a thread can record the same + // folder through a symlink, and must still be in the way. + it.effect("sees a thread that recorded the folder through a symlink", () => { + const base = NodeFs.realpathSync(NodeFs.mkdtempSync(NodePath.join(NodeOs.tmpdir(), "guard-"))); + const real = NodePath.join(base, "feature"); + const linked = NodePath.join(base, "linked"); + NodeFs.mkdirSync(real); + NodeFs.symlinkSync(real, linked); + return Effect.gen(function* () { + const exit = yield* ensureWorktreeRemovable({ + worktreePath: real, + readThreads: Effect.succeed([thread({ worktreePath: linked })]), + }).pipe(Effect.exit); + assert.isTrue(Exit.isFailure(exit)); + + // Another folder beside it is still free to go. + yield* ensureWorktreeRemovable({ + worktreePath: NodePath.join(base, "other"), + readThreads: Effect.succeed([thread({ worktreePath: linked })]), + }); + }).pipe( + Effect.ensuring(Effect.sync(() => NodeFs.rmSync(base, { recursive: true, force: true }))), + ); + }); + // The guard protects against a common mistake; it must not become a reason // that deleting a folder stops working when an unrelated query fails. it.effect("allows removal when the thread list cannot be read", () => diff --git a/apps/server/src/vcs/WorktreeRemovalGuard.ts b/apps/server/src/vcs/WorktreeRemovalGuard.ts index 0c73423ff..9ac02ab3c 100644 --- a/apps/server/src/vcs/WorktreeRemovalGuard.ts +++ b/apps/server/src/vcs/WorktreeRemovalGuard.ts @@ -13,10 +13,17 @@ * skip the step that keeps the thread usable. * * The classification itself lives in `@threadlines/shared/worktreeUsage` so the - * client's cleanup list greys out exactly the folders this guard would refuse. + * client's cleanup list greys out the folders this guard would refuse. The + * guard goes one step further than the list can: it compares folders as the + * disk resolves them. git names a worktree by its real path, and a thread may + * have recorded the same folder through a symlink (a data folder under + * macOS's `/var`, which is `/private/var`); by spelling alone that thread + * would not be in the way. * * @module WorktreeRemovalGuard */ +import * as NodeFs from "node:fs/promises"; + import * as Effect from "effect/Effect"; import { VcsWorktreeInUseError } from "@threadlines/contracts"; import { @@ -45,9 +52,34 @@ export const ensureWorktreeRemovable = (input: { }).pipe(Effect.as([] as ReadonlyArray)), ), ); + // Each distinct folder is resolved once; one that is gone keeps its spelling. + const resolved = new Map(); + const realPath = (path: string) => + Effect.promise(async () => { + const known = resolved.get(path); + if (known !== undefined) return known; + const real = await NodeFs.realpath(path).catch(() => path); + resolved.set(path, real); + return real; + }); + const realPathOrNull = (path: string | null | undefined) => + typeof path === "string" && path.length > 0 ? realPath(path) : Effect.succeed(path); const blockingThreads = findWorktreeBlockingThreads({ - worktreePath: input.worktreePath, - threads, + worktreePath: yield* realPath(input.worktreePath), + threads: yield* Effect.forEach(threads, (thread) => + Effect.gen(function* () { + const session = thread.session; + return { + ...thread, + worktreePath: (yield* realPathOrNull(thread.worktreePath)) ?? null, + effectiveCwd: yield* realPathOrNull(thread.effectiveCwd), + session: + session === null || session === undefined + ? session + : { ...session, checkoutCwd: yield* realPathOrNull(session.checkoutCwd) }, + } satisfies WorktreeUsageThread; + }), + ), }); if (blockingThreads.length === 0) { return;