Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 47 additions & 2 deletions apps/server/src/vcs/GitVcsDriverCore.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,10 @@ import * as Scope from "effect/Scope";

import { GitCommandError } from "@t3tools/contracts";
import { ServerConfig } from "../config.ts";
import { splitNullSeparatedGitStdoutPaths } from "./GitVcsDriverCore.ts";
import {
parsePorcelainWorktreePaths,
splitNullSeparatedGitStdoutPaths,
} from "./GitVcsDriverCore.ts";
import * as GitVcsDriver from "./GitVcsDriver.ts";

const ServerConfigLayer = ServerConfig.layerTest(process.cwd(), {
Expand Down Expand Up @@ -184,6 +187,48 @@ it.layer(TestLayer)("GitVcsDriver core integration", (it) => {
assert.notInclude(error.detail, "Git command failed in");
}),
);

it.effect("does not filesystem-delete a path that is not a registered worktree", () =>
Effect.gen(function* () {
const cwd = yield* makeTmpDir();
yield* initRepoWithCommit(cwd);
const pathService = yield* Path.Path;
const fileSystem = yield* FileSystem.FileSystem;
const decoyPath = pathService.join(cwd, "not-a-worktree");
yield* fileSystem.makeDirectory(decoyPath, { recursive: true });
yield* writeTextFile(decoyPath, "keep.txt", "keep\n");
const driver = yield* GitVcsDriver.GitVcsDriver;

const error = yield* driver
.removeWorktree({ cwd, path: decoyPath, force: true })
.pipe(Effect.flip);

assert.equal(error._tag, "GitCommandError");
assert.equal(yield* fileSystem.exists(pathService.join(decoyPath, "keep.txt")), true);
}),
);
});

describe("porcelain worktree parsing", () => {
it.effect("parses worktree paths from porcelain output", () =>
Effect.sync(() => {
assert.deepStrictEqual(
parsePorcelainWorktreePaths(
[
"worktree /repo",
"HEAD abc",
"branch refs/heads/main",
"",
"worktree /repo-feature",
"HEAD def",
"branch refs/heads/feature",
"",
].join("\n"),
),
["/repo", "/repo-feature"],
);
}),
);
});

describe("review diff previews", () => {
Expand Down Expand Up @@ -590,7 +635,7 @@ it.layer(TestLayer)("GitVcsDriver core integration", (it) => {
assert.equal(created.worktree.refName, "feature/worktree");
assert.equal(yield* git(worktreePath, ["branch", "--show-current"]), "feature/worktree");

yield* driver.removeWorktree({ cwd, path: worktreePath });
yield* driver.removeWorktree({ cwd, path: worktreePath, force: true });
const fileSystem = yield* FileSystem.FileSystem;
assert.equal(yield* fileSystem.exists(worktreePath), false);
}),
Expand Down
88 changes: 87 additions & 1 deletion apps/server/src/vcs/GitVcsDriverCore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,11 @@ import {
import { ServerConfig } from "../config.ts";

const DEFAULT_TIMEOUT_MS = 30_000;
// Worktrees routinely contain multi-GB install artifacts from setup scripts
// (node_modules, .repos, etc). Deleting those via `git worktree remove` alone
// routinely exceeds short command timeouts on developer machines.
const REMOVE_WORKTREE_DIRECTORY_TIMEOUT_MS = 5 * 60_000;
const REMOVE_WORKTREE_GIT_TIMEOUT_MS = 60_000;
const DEFAULT_MAX_OUTPUT_BYTES = 1_000_000;
const OUTPUT_TRUNCATED_MARKER = "\n\n[truncated]";
const PREPARED_COMMIT_PATCH_MAX_OUTPUT_BYTES = 49_000;
Expand Down Expand Up @@ -227,6 +232,19 @@ export function splitNullSeparatedGitStdoutPaths(
return splitNullSeparatedPaths(result.stdout, result.stdoutTruncated);
}

export function parsePorcelainWorktreePaths(stdout: string): string[] {
const paths: string[] = [];
for (const line of stdout.split("\n")) {
if (line.startsWith("worktree ")) {
const worktreePath = line.slice("worktree ".length).trim();
if (worktreePath.length > 0) {
paths.push(worktreePath);
}
}
}
return paths;
}

function sanitizeRemoteName(value: string): string {
const sanitized = value
.trim()
Expand Down Expand Up @@ -2379,8 +2397,76 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function*
args.push("--force");
}
args.push(input.path);
const commandContext = gitCommandContext({
operation: "GitVcsDriver.removeWorktree",
cwd: input.cwd,
args,
});

// When forcing, wipe the working tree via the filesystem first so git only
// has to clean up worktree registration metadata. That keeps the git step
// fast even for multi-GB trees and avoids interrupting `git worktree remove`
// mid-delete after a short timeout.
//
// Only do this after confirming the path is a registered *linked* worktree.
// Otherwise a mistyped/non-worktree path would be recursively deleted before
// `git worktree remove` could reject it.
if (input.force) {
const worktreeList = yield* executeGit(
"GitVcsDriver.removeWorktree.list",
input.cwd,
["worktree", "list", "--porcelain"],
{
timeoutMs: 5_000,
allowNonZeroExit: true,
fallbackErrorDetail: "git worktree list failed",
},
);
if (worktreeList.exitCode === 0) {
const registeredPaths = parsePorcelainWorktreePaths(worktreeList.stdout);
const canonicalize = (value: string) =>
fileSystem.realPath(value).pipe(Effect.orElseSucceed(() => path.resolve(value)));
const [canonicalTarget, ...canonicalRegistered] = yield* Effect.all(
[canonicalize(input.path), ...registeredPaths.map(canonicalize)],
{ concurrency: "unbounded" },
);
// Porcelain lists the main working tree first; never filesystem-delete it.
const linkedWorktreePaths = new Set(canonicalRegistered.slice(1));
if (linkedWorktreePaths.has(canonicalTarget)) {
const exists = yield* fileSystem
.exists(input.path)
.pipe(Effect.orElseSucceed(() => false));
if (exists) {
yield* fileSystem.remove(input.path, { recursive: true, force: true }).pipe(
Effect.mapError(
(cause) =>
new GitCommandError({
...commandContext,
detail: "Failed to delete worktree directory.",
cause,
}),
),
Effect.timeoutOption(REMOVE_WORKTREE_DIRECTORY_TIMEOUT_MS),
Effect.flatMap((result) =>
Option.match(result, {
onNone: () =>
Effect.fail(
new GitCommandError({
...commandContext,
detail: "Timed out deleting worktree directory.",
}),
),
onSome: () => Effect.void,
}),
),
);

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.

FS failure skips git cleanup

Medium Severity

When force pre-delete fails or times out, removeWorktree returns that error and never runs git worktree remove. Directory wipe can already be partial or complete by then, so git registration metadata is left behind even though that second step is what the change relies on for cleanup. List failures correctly fall through to git; filesystem-phase failures do not.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 35a62d9. Configure here.

}
}
Comment thread
cursor[bot] marked this conversation as resolved.
}
}
Comment thread
cursor[bot] marked this conversation as resolved.

yield* executeGit("GitVcsDriver.removeWorktree", input.cwd, args, {
timeoutMs: 15_000,
timeoutMs: REMOVE_WORKTREE_GIT_TIMEOUT_MS,
fallbackErrorDetail: "git worktree remove failed",
});
});
Expand Down
Loading