Skip to content
65 changes: 65 additions & 0 deletions apps/server/src/vcs/GitVcsDriverCore.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3218,6 +3218,44 @@ it.layer(layerTest)("GitVcsDriver core integration", (it) => {
}),
);

it.effect("unlinks ignored directory links without deleting their targets", () =>
Effect.gen(function* () {
const cwd = yield* makeTmpDir();
const { initialBranch } = yield* initRepoWithCommit(cwd);
yield* writeTextFile(cwd, ".gitignore", "linked\n");
yield* git(cwd, ["add", ".gitignore"]);
yield* git(cwd, ["commit", "-m", "ignore links"]);
const fileSystem = yield* FileSystem.FileSystem;
const pathService = yield* Path.Path;
const outside = yield* makeTmpDir("git-worktree-link-target-");
yield* writeTextFile(outside, "keep.txt", "keep\n");
const worktreePath = pathService.join(yield* makeTmpDir("git-worktrees-"), "linked");
const driver = yield* GitVcsDriver.GitVcsDriver;
yield* driver.createWorktree({
cwd,
path: worktreePath,
refName: initialBranch,
newRefName: "feature/linked",
});
// A junction on Windows, which Git for Windows leaves behind; a symlink elsewhere.
NodeFS.symlinkSync(outside, pathService.join(worktreePath, "linked"), "junction");
NodeFS.mkdirSync(pathService.join(worktreePath, "linked-parent"));
NodeFS.symlinkSync(
outside,
pathService.join(worktreePath, "linked-parent", "linked"),
"junction",
);

yield* driver.removeWorktree({ cwd, path: worktreePath });

assert.equal(yield* fileSystem.exists(worktreePath), false);
assert.equal(
yield* fileSystem.readFileString(pathService.join(outside, "keep.txt")),
"keep\n",
);
}),
);

it.effect("keeps a worktree whose untracked files status is configured to hide", () =>
Effect.gen(function* () {
const cwd = yield* makeTmpDir();
Expand Down Expand Up @@ -3246,6 +3284,33 @@ it.layer(layerTest)("GitVcsDriver core integration", (it) => {
}),
);

it.effect("leaves a folder alone that only shares the worktree's short name", () =>
Effect.gen(function* () {
const cwd = yield* makeTmpDir();
const { initialBranch } = yield* initRepoWithCommit(cwd);
const pathService = yield* Path.Path;
const fileSystem = yield* FileSystem.FileSystem;
const driver = yield* GitVcsDriver.GitVcsDriver;
const worktreePath = pathService.join(yield* makeTmpDir("git-worktrees-"), "short");
yield* driver.createWorktree({
cwd,
path: worktreePath,
refName: initialBranch,
newRefName: "feature/short",
});
// Unrelated to the worktree, but resolving "short" against cwd lands here.
const outside = yield* makeTmpDir("git-worktree-link-target-");
const unrelated = pathService.join(cwd, "short");
NodeFS.mkdirSync(unrelated);
NodeFS.symlinkSync(outside, pathService.join(unrelated, "linked"), "junction");

yield* Effect.result(driver.removeWorktree({ cwd, path: "short" }));

assert.isTrue(NodeFS.lstatSync(pathService.join(unrelated, "linked")).isSymbolicLink());
assert.equal(yield* fileSystem.exists(worktreePath), true);
}),
);

it.effect("allows worktree removal to run longer than the default command timeout", () =>
Effect.gen(function* () {
const delegate = yield* ChildProcessSpawner.ChildProcessSpawner;
Expand Down
51 changes: 50 additions & 1 deletion apps/server/src/vcs/GitVcsDriverCore.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import * as Cache from "effect/Cache";
import * as Cause from "effect/Cause";
import * as Data from "effect/Data";
import * as Crypto from "effect/Crypto";
import * as DateTime from "effect/DateTime";
Expand Down Expand Up @@ -36,6 +37,7 @@ import { parseT3ProjectFile } from "@t3tools/shared/t3ProjectFile";
import { resolveProjectFileBackedSetting } from "@t3tools/shared/projectSettings";
import { gitCommandDuration, gitCommandsTotal, withMetrics } from "../observability/Metrics.ts";
import * as GitVcsDriver from "./GitVcsDriver.ts";
import { removeEmptyDirectory } from "./removeEmptyDirectory.ts";
import { resolveWorktreesDirectory } from "../worktreesDirectory.ts";
import {
parseRemoteNames,
Expand Down Expand Up @@ -3781,16 +3783,41 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function*
input.branch,
]);

/**
* Unlinks links under `target` without following them and removes the
* directories that leaves empty. Returns false when anything else remains.
*/
const removeLeftoverLinks = (
target: string,
): Effect.Effect<boolean, PlatformError.PlatformError | Cause.UnknownError> =>
Effect.gen(function* () {
if (Option.isSome(yield* fileSystem.readLink(target).pipe(Effect.option))) {

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.

🟠 High vcs/GitVcsDriverCore.ts:3659

removeLeftoverLinks can delete content outside the removed worktree and can delete a newly written regular file: a link can be replaced after readLink(target) but before remove, stat, or readDirectory, and those operations then act on the replacement or follow it into an external directory. The cleanup needs no-follow, object-bound filesystem operations (or equivalent revalidation) for both deletion and traversal so it only removes the entries that were inspected.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/vcs/GitVcsDriverCore.ts around line 3659:

`removeLeftoverLinks` can delete content outside the removed worktree and can delete a newly written regular file: a link can be replaced after `readLink(target)` but before `remove`, `stat`, or `readDirectory`, and those operations then act on the replacement or follow it into an external directory. The cleanup needs no-follow, object-bound filesystem operations (or equivalent revalidation) for both deletion and traversal so it only removes the entries that were inspected.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not changing this; same reasoning as the CodeRabbit thread above (resolved). The swap has to land inside the stub folder of a worktree Git just unregistered, under the user's own T3 worktrees directory, within the microseconds between readLink and remove. Only a process running as the same user can do that, and it could already delete anything this pass touches, so there is no privilege boundary to defend. Node has no openat/unlinkat-style handle-bound operations, so object-bound traversal would need native code. Folders are only removed with rmdir, so a file written into a folder after the listing is kept.

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.

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

yield* fileSystem.remove(target);
return true;
}
if ((yield* fileSystem.stat(target)).type !== "Directory") return false;
let empty = true;
for (const name of yield* fileSystem.readDirectory(target)) {
Comment thread
coderabbitai[bot] marked this conversation as resolved.
if (!(yield* removeLeftoverLinks(path.join(target, name)))) empty = false;
}
// Only an empty directory goes, so files written since the listing stay.
return empty && (yield* removeEmptyDirectory(target));
});

const removeWorktree: GitVcsDriver.GitVcsDriver["Service"]["removeWorktree"] = Effect.fn(
"removeWorktree",
)(function* (input) {
// Git also accepts a worktree's short name, which can match a different
// folder than the path resolved against `input.cwd`. Resolve it once so Git
// and the leftover cleanup below act on the same directory.
const target = path.resolve(input.cwd, input.path);
// Git refuses to remove a worktree with untracked files unless forced, but
// its check honors `status.showUntrackedFiles=no` and would delete them.
const args = ["-c", "status.showUntrackedFiles=normal", "worktree", "remove"];
if (input.force) {
args.push("--force");
}
args.push(input.path);
args.push(target);
const result = yield* executeGitWithStableDiagnostics(
"GitVcsDriver.removeWorktree",
input.cwd,
Expand All @@ -3804,6 +3831,28 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function*
},
);
if (result.exitCode === 0) {
// Git for Windows never descends into NTFS junctions, such as pnpm's
// node_modules links. It reports success but leaves them and their parent
// directories behind, so a resumed thread would find a stub instead of
// recreating its checkout. Git has already unregistered the worktree, so
// anything left here is logged rather than returned: a retry could never
// succeed.
if (yield* fileSystem.exists(target).pipe(Effect.orElseSucceed(() => false))) {
const removed = yield* removeLeftoverLinks(target).pipe(
Effect.catch((error) =>
Effect.logWarning("GitVcsDriver.removeWorktree: failed to delete leftover links", {
path: target,
error,
}).pipe(Effect.as(true)),
),
);
if (!removed) {
yield* Effect.logWarning(
"GitVcsDriver.removeWorktree: kept files written after git removed the worktree",
{ path: target },
);
}
}
return;
}
// Threads can share a worktree path, and worktrees get removed or pruned
Expand Down
18 changes: 18 additions & 0 deletions apps/server/src/vcs/removeEmptyDirectory.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// @effect-diagnostics nodeBuiltinImport:off - Effect's FileSystem has no rmdir.
import * as NodeFSP from "node:fs/promises";

import * as Effect from "effect/Effect";

/**
* Removes `path` only while it is an empty directory. Succeeds with false when
* anything else is there, including files written after the caller looked.
* Other failures, such as a permission error, stay errors.
*/
export const removeEmptyDirectory = (path: string) =>
Effect.tryPromise(() => NodeFSP.rmdir(path)).pipe(
Effect.as(true),
Effect.catchIf(
(error) => (error.cause as NodeJS.ErrnoException | undefined)?.code === "ENOTEMPTY",
() => Effect.succeed(false),
),
);
Loading