fix: refuse to remove a worktree with live processes

CHANGELOG.md

@@ -1,5 +1,42 @@
 # Changelog
 
+## Unreleased
+
+### Fixed — data loss
+
+- **`gwt rm` could half-delete a worktree, and then could not finish the job.**
+  With a dev server still running, `git worktree remove` deletes files while the
+  process recreates them, the final `rmdir` fails — and git has already
+  deregistered the worktree by then. The retry answers `No worktree matching`,
+  and the half-emptied directory can only be cleared by hand. Observed on a
+  worktree with 51 live processes: 144 KB of orphans left behind, including the
+  repository's `CLAUDE.md` and `eslint.config.mjs`.
+
+  `rm` now looks for processes running under the worktree first, names them, and
+  refuses — the same shape as the existing dirty-tree guard, which tells you what
+  to do rather than doing it for you. It prints the `pkill` to run.
+
+  **There is deliberately no flag to override it**, and `--force` does not. A
+  dirty tree is data you may be willing to lose; a live process is a mechanism
+  that corrupts the removal itself. The check is also a heuristic on command
+  lines, so a false positive is possible — refusing one costs a minute of
+  confusion, and stopping one would cost a process that mattered.
+
+### Fixed
+
+- **A branch created with `--from` inherited the base as its upstream.** `git
+  worktree add -b fix/x <path> origin/main` sets `origin/main` as the upstream,
+  so a bare `git push` refuses — and suggests `git push origin HEAD:main`. Copy
+  that suggestion on a tired afternoon and the fix lands on the default branch.
+  `--from` now passes `--no-track`, and the same `git push` suggests
+  `git push --set-upstream origin fix/x` instead.
+
+- **`--from` branched off a stale base.** `add` fetched only when the branch
+  already existed, so creating a new branch from `--from main` used whatever the
+  local ref happened to be — weeks behind on a repository left alone, with
+  nothing said. It now fetches the base first, and when a local base is behind
+  its remote it says by how many commits and suggests `--from origin/<base>`.
+
 ## 0.8.4
 
 Nothing to reinstall — the shell wrapper is unchanged. `gwt sync-env` copies

src/commands/add.ts

@@ -1,6 +1,7 @@
 import { intro, outro, log, select, isCancel } from "@clack/prompts";
 import pc from "picocolors";
 import {
+  fetchBase,
   getRepoRoot,
   getWorktreePath,
   worktreeExists,
@@ -97,6 +98,10 @@ export async function commandAdd(
 
   if (from) {
     log.info(`Creating from base branch: ${from}`);
+    if (!branchAlreadyExists) {
+      log.step("Fetching base…");
+      fetchBase(from);
+    }
   }
 
   log.step("Creating git worktree…");

src/commands/rm.ts

@@ -10,6 +10,7 @@ import { resolveWorktree } from "../lib/resolveWorktree.js";
 import { runCommands } from "../lib/setup.js";
 import { requestCd } from "../lib/shellCd.js";
 import { resolveConfig, REPO_CONFIG_FILE } from "../lib/repoConfig.js";
+import { processesIn, describeProcesses } from "../lib/processes.js";
 
 export async function commandRm(
   query?: string,
@@ -34,6 +35,29 @@ export async function commandRm(
     process.exit(1);
   }
 
+  // Refused, never offered as a prompt — the same shape as the dirty-tree guard
+  // above. Two reasons. Terminating someone's dev servers is not this command's
+  // business: it removes a worktree, and it has no idea whether one of those
+  // processes is a debugger someone is standing in. And the detection is a
+  // heuristic on command lines, so a false positive is possible — refusing one
+  // costs a minute of confusion, killing one costs a process that mattered.
+  //
+  // There is deliberately no flag to override it. A dirty tree is data you may
+  // be willing to lose; a live process is a mechanism that corrupts the removal
+  // itself, and no amount of certainty makes that safe.
+  const running = processesIn(worktreePath);
+  if (running.length > 0) {
+    const names = describeProcesses(running);
+    log.error(
+      `${running.length} process(es) still running in this worktree` +
+        (names.length > 0 ? ` (${names.join(", ")})` : "") +
+        `.\n   Removing now deletes files underneath them, and git leaves the` +
+        `\n   directory half-emptied — a state gwt cannot repair.` +
+        `\n   Stop them first:  pkill -f ${worktreePath}`,
+    );
+    process.exit(1);
+  }
+
   // clack cannot open a prompt without a TTY — it dies with a raw
   // `uv_tty_init returned EINVAL`. Removal is destructive, so a script that
   // cannot be asked has to say up front that it means it.

src/lib/git.ts

@@ -1,6 +1,7 @@
 import { execFileSync } from "child_process";
 import path from "path";
 import fs from "fs";
+import { log } from "@clack/prompts";
 
 export interface WorktreeEntry {
   path: string;
@@ -132,12 +133,56 @@ export function addWorktree(
   from?: string,
 ): void {
   const root = getRepoRoot();
+  // --no-track when branching off a base: git would otherwise set the new
+  // branch's upstream to that base, and a bare `git push` then fails with
+  // "the upstream branch of your current branch does not match the name of your
+  // current branch" and helpfully suggests `git push origin HEAD:production`.
+  // Copy that on a tired afternoon and a fix lands on the default branch.
   const args = branchExists(branch)
     ? ["worktree", "add", worktreePath, branch]
-    : ["worktree", "add", worktreePath, "-b", branch, ...(from ? [from] : [])];
+    : [
+        "worktree",
+        "add",
+        worktreePath,
+        ...(from ? ["--no-track"] : []),
+        "-b",
+        branch,
+        ...(from ? [from] : []),
+      ];
   runVisible("git", args, root);
 }
 
+/**
+ * Refreshes the remote counterpart of `base`, and says so when a local base is
+ * behind it.
+ *
+ * `add` only fetched when the branch already existed, so creating a new branch
+ * from `--from production` used whatever the local ref happened to be — weeks
+ * stale on a repository left alone, silently.
+ */
+export function fetchBase(base: string): void {
+  const root = getRepoRoot();
+  const remoteless = base.startsWith("origin/") ? base.slice("origin/".length) : base;
+  try {
+    runVisible("git", ["fetch", "origin", remoteless], root);
+  } catch {
+    // A base with no remote counterpart is legitimate: a local-only branch, or
+    // a tag. Creating from it is still what was asked.
+    return;
+  }
+
+  if (base.startsWith("origin/")) return;
+  const remote = `origin/${remoteless}`;
+  if (!succeeds("git", ["rev-parse", "--verify", remote], root)) return;
+  const behind = run("git", ["rev-list", "--count", `${base}..${remote}`], root);
+  if (behind !== "0") {
+    log.warn(
+      `Local '${base}' is ${behind} commit(s) behind ${remote}. ` +
+        `Pass --from ${remote} to branch from the remote instead.`,
+    );
+  }
+}
+
 export function removeWorktree(worktreePath: string): void {
   const root = getRepoRoot();
   runVisible("git", ["worktree", "remove", worktreePath, "--force"], root);

src/lib/processes.ts

@@ -0,0 +1,60 @@
+import { execFileSync } from "child_process";
+
+// `git worktree remove` deletes files one by one, so anything still writing
+// under the worktree races it: the files go, a live process recreates one, and
+// the final rmdir fails. git has already deregistered the worktree by then, so
+// `gwt rm` answers "No worktree matching" on the retry and the half-emptied
+// directory can only be finished off by hand. Refusing up front is the only
+// version of this that stays repairable.
+
+// pgrep matches an extended regular expression against the full command line,
+// and a path is full of characters that mean something there — a worktree for
+// `release/1.0.0` would otherwise match paths it does not own.
+function escapeForRegex(value: string): string {
+  return value.replace(/[.*+?^${}()|[\]\\]/g, "\\$&");
+}
+
+/**
+ * PIDs of processes whose command line mentions `worktreePath`.
+ *
+ * A dev server started inside a worktree carries the path in its argv — every
+ * `node <worktree>/node_modules/...` shows up, and so does the Nx daemon. It is
+ * a heuristic rather than a file-handle audit, but it is the one that found 51
+ * live processes under a worktree that `git worktree remove` had just started
+ * deleting.
+ */
+export function processesIn(worktreePath: string): number[] {
+  let out: string;
+  try {
+    out = execFileSync("pgrep", ["-f", "--", escapeForRegex(worktreePath)], {
+      encoding: "utf-8",
+      stdio: ["ignore", "pipe", "ignore"],
+    });
+  } catch {
+    // pgrep exits 1 when nothing matches, and 127 when it is not installed.
+    // Neither is a reason to block a removal.
+    return [];
+  }
+
+  const own = new Set([process.pid, process.ppid]);
+  return out
+    .split("\n")
+    .map((line) => Number(line.trim()))
+    .filter((pid) => Number.isInteger(pid) && pid > 0 && !own.has(pid));
+}
+
+/** Command names for `pids`, deduplicated, for a message a human can act on. */
+export function describeProcesses(pids: number[]): string[] {
+  const names = pids.map((pid) => {
+    try {
+      const comm = execFileSync("ps", ["-o", "comm=", "-p", String(pid)], {
+        encoding: "utf-8",
+        stdio: ["ignore", "pipe", "ignore"],
+      }).trim();
+      return comm.split("/").pop() || "?";
+    } catch {
+      return "?";
+    }
+  });
+  return [...new Set(names)].filter((name) => name !== "?");
+}

tests/add-from-base.test.ts

@@ -0,0 +1,75 @@
+import { test, describe } from "node:test";
+import assert from "node:assert/strict";
+import path from "node:path";
+import { tmpRepoWithRemote, runCli, git } from "./helpers/fixtures.ts";
+
+function siblingOf(repo: string, branch: string): string {
+  return path.join(
+    path.dirname(repo),
+    `${path.basename(repo)}-${branch.replace(/\//g, "-")}`,
+  );
+}
+
+function upstreamOf(worktree: string): string | null {
+  try {
+    return git(worktree, "rev-parse", "--abbrev-ref", "@{upstream}").trim();
+  } catch {
+    return null;
+  }
+}
+
+describe("gwt add --from", () => {
+  // Branching off a remote ref makes git set that ref as the upstream, and a
+  // bare `git push` then refuses with a suggestion to push to the base branch
+  // itself — `git push origin HEAD:main`. On a support fix that is one tired
+  // copy-paste away from landing on the default branch.
+  test("leaves the new branch without an upstream", () => {
+    const { repo } = tmpRepoWithRemote();
+
+    const result = runCli(["add", "fix/x", "--from", "origin/main"], { cwd: repo });
+    assert.equal(result.code, 0, result.output);
+
+    assert.equal(
+      upstreamOf(siblingOf(repo, "fix/x")),
+      null,
+      "a branch created from a base must not inherit the base as its upstream",
+    );
+  });
+
+  test("still tracks normally when no base is given", () => {
+    const { repo } = tmpRepoWithRemote();
+    const result = runCli(["add", "plain"], { cwd: repo });
+    assert.equal(result.code, 0, result.output);
+    // No base, no -b from a remote ref: nothing to inherit either way, but the
+    // worktree has to exist and be usable.
+    assert.match(git(repo, "worktree", "list"), /plain/);
+  });
+
+  // `add` only fetched when the branch already existed, so a new branch created
+  // from `--from main` used whatever the local ref happened to be.
+  test("fetches the base, so the worktree starts from the remote's tip", () => {
+    const { repo, origin } = tmpRepoWithRemote();
+
+    // Someone else pushes while this clone is not looking.
+    const other = path.join(path.dirname(repo), "other");
+    git(path.dirname(repo), "clone", "-q", origin, other);
+    git(other, "config", "user.email", "them@example.com");
+    git(other, "config", "user.name", "Them");
+    // The bare origin's HEAD still points at whatever init.defaultBranch was
+    // when it was created, which is not `main` on every runner — so the clone
+    // may land nowhere. Name the branch rather than trusting the checkout.
+    git(other, "checkout", "-q", "-B", "main", "origin/main");
+    git(other, "commit", "-q", "--allow-empty", "-m", "landed after our last fetch");
+    git(other, "push", "-q", "origin", "main");
+
+    const result = runCli(["add", "fix/y", "--from", "origin/main"], { cwd: repo });
+    assert.equal(result.code, 0, result.output);
+
+    const log = git(siblingOf(repo, "fix/y"), "log", "--oneline");
+    assert.match(
+      log,
+      /landed after our last fetch/,
+      "the base was refreshed before branching",
+    );
+  });
+});

tests/rm-live-worktree.test.ts

@@ -0,0 +1,85 @@
+import { test, describe, after } from "node:test";
+import assert from "node:assert/strict";
+import { spawn, type ChildProcess } from "node:child_process";
+import { tmpRepo, tmpRepoWithRemote, runCli, git } from "./helpers/fixtures.ts";
+import path from "node:path";
+import { processesIn, describeProcesses } from "../dist/lib/processes.js";
+
+const spawned: ChildProcess[] = [];
+after(() => {
+  for (const child of spawned) child.kill("SIGKILL");
+});
+
+/** A process whose argv carries `dir` — the shape a dev server started there has. */
+function sleeperIn(dir: string): ChildProcess {
+  const child = spawn(
+    process.execPath,
+    ["-e", `setTimeout(() => {}, 60000); // ${path.join(dir, "node_modules", "marker")}`],
+    { stdio: "ignore", detached: false },
+  );
+  spawned.push(child);
+  return child;
+}
+
+describe("processesIn", () => {
+  test("finds a process whose command line carries the worktree path", async () => {
+    const dir = tmpRepo("live");
+    assert.deepEqual(processesIn(dir), [], "clean before anything runs");
+
+    const child = sleeperIn(dir);
+    await new Promise((r) => setTimeout(r, 300));
+
+    const found = processesIn(dir);
+    assert.ok(found.includes(child.pid!), `expected ${child.pid} in ${found}`);
+    assert.ok(!found.includes(process.pid), "never reports the running gwt itself");
+    assert.ok(describeProcesses(found).length > 0, "names the processes for the message");
+  });
+
+  test("an unrelated directory is unaffected", async () => {
+    const mine = tmpRepo("mine");
+    const other = tmpRepo("other");
+    sleeperIn(mine);
+    await new Promise((r) => setTimeout(r, 300));
+    assert.deepEqual(processesIn(other), []);
+  });
+});
+
+describe("gwt rm with a live worktree", () => {
+  // The failure this guards: `git worktree remove` deletes files under a running
+  // process, the final rmdir fails, and git has already deregistered the
+  // worktree — so the retry says "No worktree matching" and the half-emptied
+  // directory can only be finished by hand.
+  // Refused, not prompted — the same shape as the dirty-tree guard, and for a
+  // stronger reason: the detection is a heuristic, so a false positive must cost
+  // a confused user rather than a killed process.
+  test("refuses, and --force does not override it", async () => {
+    const { repo } = tmpRepoWithRemote();
+    runCli(["add", "doomed"], { cwd: repo });
+    const worktree = path.join(path.dirname(repo), `${path.basename(repo)}-doomed`);
+
+    sleeperIn(worktree);
+    await new Promise((r) => setTimeout(r, 300));
+
+    const result = runCli(["rm", "doomed", "--force"], { cwd: repo });
+    assert.equal(result.code, 1, "exits non-zero rather than half-removing");
+    assert.match(result.output, /still running/);
+    assert.match(result.output, /pkill -f/, "says how to clear it");
+    assert.doesNotMatch(
+      result.output,
+      /Stop them and remove|Stopped \d+ process/,
+      "never offers to kill them",
+    );
+
+    const listed = git(repo, "worktree", "list");
+    assert.match(listed, /doomed/, "the worktree is still registered");
+  });
+
+  test("removes normally once nothing is running there", () => {
+    const { repo } = tmpRepoWithRemote();
+    runCli(["add", "quiet"], { cwd: repo });
+
+    const result = runCli(["rm", "quiet", "--force"], { cwd: repo });
+    assert.equal(result.code, 0, result.output);
+    assert.doesNotMatch(git(repo, "worktree", "list"), /quiet/);
+  });
+});