Repository navigation
fix(server): removing a worktree on Windows no longer deletes what its junctions point to #21099
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
67c675e
3326d81
e05e0bd
1f801a2
de386bd
0ab0228
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,164 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||
| import { GitCommandError } from "@t3tools/contracts"; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| import * as HostProcess from "@t3tools/shared/HostProcess"; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| import * as Effect from "effect/Effect"; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| import * as FileSystem from "effect/FileSystem"; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| import * as Option from "effect/Option"; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| import * as Path from "effect/Path"; | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| import type * as GitVcsDriver from "./GitVcsDriver.ts"; | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| const INDEX_LIST_MAX_BYTES = 64 * 1024 * 1024; | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * Removes the linked worktree at `worktreePath` by running `git worktree | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * remove` through `run`. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * Git for Windows before 2.54 follows NTFS junctions while it deletes a | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * worktree, and deletes what they point to, such as a shared node_modules. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * Later versions skip junctions but leave them, and the folders holding them, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * behind. On Windows, Git therefore never deletes the files: this makes the | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * checks `git worktree remove` makes before deleting (builtin/worktree.c), | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * deletes the folder with Node's `rm`, which unlinks junctions without | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * following them, and then has Git drop the now-missing worktree's metadata. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * Git elsewhere only unlinks links, so other hosts run Git as before. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||
| export const removeLinkedWorktree = Effect.fn("WorktreeRemoval.removeLinkedWorktree")(function* < | ||||||||||||||||||||||||||||||||||||||||||||||||||
| A, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| E, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| R, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| >(input: { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| readonly operation: string; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| readonly cwd: string; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| readonly worktreePath: string; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| readonly force: boolean; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| readonly execute: GitVcsDriver.GitVcsDriver["Service"]["execute"]; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| readonly run: (args: ReadonlyArray<string>) => Effect.Effect<A, E, R>; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| }) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const fileSystem = yield* FileSystem.FileSystem; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const path = yield* Path.Path; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const worktreePath = input.worktreePath; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const gitFile = path.join(worktreePath, ".git"); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // Git refuses to remove a worktree with untracked files unless forced, but | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // its check honors `status.showUntrackedFiles=no` and would delete them. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const statusConfig = ["-c", "status.showUntrackedFiles=normal"]; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const linked = | ||||||||||||||||||||||||||||||||||||||||||||||||||
| (yield* HostProcess.isWindows) && | ||||||||||||||||||||||||||||||||||||||||||||||||||
| (yield* fileSystem.stat(gitFile).pipe( | ||||||||||||||||||||||||||||||||||||||||||||||||||
| Effect.map((info) => info.type === "File"), | ||||||||||||||||||||||||||||||||||||||||||||||||||
| Effect.orElseSucceed(() => false), | ||||||||||||||||||||||||||||||||||||||||||||||||||
| )); | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+44
to
+49
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: sed -n '1,175p' apps/server/src/vcs/worktreeRemoval.ts
rg -n 'reason._tag === "NotFound"|WORKTREE_REMOVE_TIMEOUT_MS|DEFAULT_TIMEOUT_MS' apps/server/src/vcs apps/server/src/storageCleanup.tsRepository: pingdotgg/t3code Length of output: 7973 🏁 Script executed: set -eu
printf '%s\n' '--- removal call sites ---'
rg -n -F --glob '*.ts' -e 'removeLinkedWorktree' -e 'worktreeRemoval' -e 'WORKTREE_REMOVE_TIMEOUT_MS' apps/server/src/vcs
printf '%s\n' '--- GitVcsDriverCore removal region ---'
sed -n '4140,4220p' apps/server/src/vcs/GitVcsDriverCore.ts
printf '%s\n' '--- filesystem error handling near process helpers ---'
sed -n '480,530p' apps/server/src/vcs/GitVcsDriverCore.ts
printf '%s\n' '--- worktree tests ---'
rg -n -F --glob '*.{ts,tsx}' -e 'junction' -e 'removeWorktree' -e 'removeLinkedWorktree' apps/server/src/vcs
printf '%s\n' '--- Git version/support references ---'
rg -n -i --glob '!**/node_modules/**' -e 'git for windows' -e 'git version' -e 'minGit' -e 'mingit' -e '2\.54' -e '2\.47' -e 'worktree remove' apps/server docs packages .github 2>/dev/null || test "$?" -eq 1
printf '%s\n' '--- base-to-head diff for the helper and callers ---'
git diff --no-ext-diff --unified=35 680152e27b2a961686389ca69a1557cbb2c55f08 0ab022854c533ad1a4a82724790c66f45a97a73b -- apps/server/src/vcs/worktreeRemoval.ts apps/server/src/vcs/GitVcsDriverCore.ts apps/server/src/vcs/GitVcsDriverCore.test.tsRepository: pingdotgg/t3code Length of output: 36077 🌐 Web query:
💡 Result: Fail closed when On Windows, any Keep the direct Git fallback for 🛡️ Suggested fix--- "a/apps/server/src/vcs/worktreeRemoval.ts"
+++ "b/apps/server/src/vcs/worktreeRemoval.ts"
@@ -41,12 +41,24 @@
// Git refuses to remove a worktree with untracked files unless forced, but
// its check honors `status.showUntrackedFiles=no` and would delete them.
const statusConfig = ["-c", "status.showUntrackedFiles=normal"];
const linked =
(yield* HostProcess.isWindows) &&
(yield* fileSystem.stat(gitFile).pipe(
Effect.map((info) => info.type === "File"),
- Effect.orElseSucceed(() => false),
+ Effect.catch((error) =>
+ error.reason._tag === "NotFound"
+ ? Effect.succeed(false)
+ : Effect.fail(
+ new GitCommandError({
+ operation: input.operation,
+ command: "git worktree remove",
+ cwd: input.cwd,
+ detail: `fatal: cannot inspect '${worktreePath}'`,
+ cause: error,
+ }),
+ ),
+ ),
));
if (!linked) {
return yield* input.run([
...statusConfig,📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!linked) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| return yield* input.run([ | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ...statusConfig, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| "worktree", | ||||||||||||||||||||||||||||||||||||||||||||||||||
| "remove", | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ...(input.force ? ["--force"] : []), | ||||||||||||||||||||||||||||||||||||||||||||||||||
| "--", | ||||||||||||||||||||||||||||||||||||||||||||||||||
| worktreePath, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ]); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| // Git's own refusals, worded the way it prints them. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const refuse = (message: string, cause?: unknown) => | ||||||||||||||||||||||||||||||||||||||||||||||||||
| new GitCommandError({ | ||||||||||||||||||||||||||||||||||||||||||||||||||
| operation: input.operation, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| command: "git worktree remove", | ||||||||||||||||||||||||||||||||||||||||||||||||||
| cwd: input.cwd, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| detail: `fatal: ${message}`, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ...(cause === undefined ? {} : { cause }), | ||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const gitOutput = (cwd: string, args: ReadonlyArray<string>, env?: NodeJS.ProcessEnv) => | ||||||||||||||||||||||||||||||||||||||||||||||||||
| input | ||||||||||||||||||||||||||||||||||||||||||||||||||
| .execute({ | ||||||||||||||||||||||||||||||||||||||||||||||||||
| operation: input.operation, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| cwd, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| args, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| maxOutputBytes: INDEX_LIST_MAX_BYTES, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ...(env === undefined ? {} : { env }), | ||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| .pipe(Effect.map((result) => result.stdout)); | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+70
to
+79
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win Pass a timeout to the inspection commands.
Add an explicit timeout that matches the old budget. ⏱️ Proposed fix input
.execute({
operation: input.operation,
cwd,
args,
maxOutputBytes: INDEX_LIST_MAX_BYTES,
+ timeoutMs: 300_000,
...(env === undefined ? {} : { env }),
})📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||
| const samePath = (left: string, right: string) => | ||||||||||||||||||||||||||||||||||||||||||||||||||
| Effect.all([fileSystem.realPath(left), fileSystem.realPath(right)]).pipe( | ||||||||||||||||||||||||||||||||||||||||||||||||||
| Effect.map(([a, b]) => a.toLowerCase() === b.toLowerCase()), | ||||||||||||||||||||||||||||||||||||||||||||||||||
| Effect.orElseSucceed(() => false), | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| // A registered worktree of this repository: its `.git` file names an admin | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // directory under the common `worktrees/`, whose `gitdir` points back. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const notAWorktree = refuse(`'${worktreePath}' is not a working tree`); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const adminDir = path.resolve( | ||||||||||||||||||||||||||||||||||||||||||||||||||
| worktreePath, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| (yield* gitOutput(worktreePath, ["rev-parse", "--absolute-git-dir"]).pipe( | ||||||||||||||||||||||||||||||||||||||||||||||||||
| Effect.mapError(() => notAWorktree), | ||||||||||||||||||||||||||||||||||||||||||||||||||
| )).trim(), | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const commonDir = path.resolve( | ||||||||||||||||||||||||||||||||||||||||||||||||||
| input.cwd, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| (yield* gitOutput(input.cwd, ["rev-parse", "--git-common-dir"])).trim(), | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const pointsBack = yield* fileSystem.readFileString(path.join(adminDir, "gitdir")).pipe( | ||||||||||||||||||||||||||||||||||||||||||||||||||
| Effect.flatMap((gitdir) => samePath(path.resolve(adminDir, gitdir.trim()), gitFile)), | ||||||||||||||||||||||||||||||||||||||||||||||||||
| Effect.orElseSucceed(() => false), | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!pointsBack || !(yield* samePath(path.dirname(adminDir), path.join(commonDir, "worktrees")))) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| return yield* notAWorktree; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // Git counts the file's existence as a lock. T3 passes at most one | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // `--force`, which never removes a locked worktree. Anything but a | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // confirmed absence refuses. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const lockFile = path.join(adminDir, "locked"); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const locked = yield* fileSystem.stat(lockFile).pipe( | ||||||||||||||||||||||||||||||||||||||||||||||||||
| Effect.as(true), | ||||||||||||||||||||||||||||||||||||||||||||||||||
| Effect.catch((error) => | ||||||||||||||||||||||||||||||||||||||||||||||||||
| error.reason._tag === "NotFound" | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ? Effect.succeed(false) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| : Effect.fail(refuse(`cannot inspect the lock of '${worktreePath}'`, error)), | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if (locked) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const reason = (yield* fileSystem.readFileString(lockFile).pipe(Effect.option)).pipe( | ||||||||||||||||||||||||||||||||||||||||||||||||||
| Option.map((text) => text.trim()), | ||||||||||||||||||||||||||||||||||||||||||||||||||
| Option.getOrElse(() => ""), | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| return yield* refuse( | ||||||||||||||||||||||||||||||||||||||||||||||||||
| reason === "" | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ? "cannot remove a locked working tree;\nuse 'remove -f -f' to override or unlock first" | ||||||||||||||||||||||||||||||||||||||||||||||||||
| : `cannot remove a locked working tree, lock reason: ${reason}\nuse 'remove -f -f' to override or unlock first`, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!input.force) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // Bound like Git's own check, so `core.worktree` can't point it elsewhere. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const boundEnv = { GIT_DIR: gitFile, GIT_WORK_TREE: worktreePath }; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const submodules = refuse("working trees containing submodules cannot be moved or removed"); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // An unreadable path counts as present, so a doubt refuses. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const present = (target: string) => | ||||||||||||||||||||||||||||||||||||||||||||||||||
| fileSystem.exists(target).pipe(Effect.orElseSucceed(() => true)); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if (yield* present(path.join(adminDir, "modules"))) return yield* submodules; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const index = yield* gitOutput(worktreePath, ["ls-files", "--stage", "-z"], boundEnv); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| for (const entry of index.split("\0")) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!entry.startsWith("160000 ")) continue; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const submodule = entry.slice(entry.indexOf("\t") + 1); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if (yield* present(path.join(worktreePath, submodule, ".git"))) return yield* submodules; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const status = yield* gitOutput( | ||||||||||||||||||||||||||||||||||||||||||||||||||
| worktreePath, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| [...statusConfig, "status", "--porcelain", "--ignore-submodules=none"], | ||||||||||||||||||||||||||||||||||||||||||||||||||
| boundEnv, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if (status !== "") | ||||||||||||||||||||||||||||||||||||||||||||||||||
| return yield* refuse( | ||||||||||||||||||||||||||||||||||||||||||||||||||
| `'${worktreePath}' contains modified or untracked files, use --force to delete it`, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| // A file held open can stop the delete partway. Putting `.git` back, when | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // that works, leaves a worktree that a retry can remove. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const gitFileContents = yield* fileSystem | ||||||||||||||||||||||||||||||||||||||||||||||||||
| .readFileString(gitFile) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| .pipe(Effect.mapError(() => notAWorktree)); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| yield* fileSystem.remove(worktreePath, { recursive: true }).pipe( | ||||||||||||||||||||||||||||||||||||||||||||||||||
| Effect.tapError(() => fileSystem.writeFileString(gitFile, gitFileContents).pipe(Effect.ignore)), | ||||||||||||||||||||||||||||||||||||||||||||||||||
| Effect.mapError((cause) => refuse(`failed to delete '${worktreePath}'`, cause)), | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // The folder is gone, so Git only drops the worktree's metadata. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| return yield* input.run(["worktree", "remove", "--force", "--force", "--", worktreePath]); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
executeis a GitVcsDriver service dependency passed into a production Effect function, so this helper's requirements are hidden from its Effect environment. Could you acquireGitVcsDriver.GitVcsDriverwithyield*inside the operation and remove theexecuteargument from both callers? Theruncallback can remain as the deliberate caller-specific command strategy.Suggested fix: No self-contained diff; this requires updating both callers.
Posted via Macroscope — Effect Service Conventions