fix(worktrees): preserve unmerged work on non-main repos and recover stale GC lock

Two reliability bugs in the worktree garbage collector:

- Graveyard preservation compared agent branches against a hardcoded
  `main`, so on a repository whose default branch is not `main` the
  rev-list check failed and was read as "nothing to preserve", letting
  cleanup_all_stale delete unmerged agent commits. The base is now
  resolved from the repo default (origin/HEAD, main, master, and their
  remote refs), and an inconclusive check preserves the branch to the
  graveyard instead of dropping it.

- The GC lock had no stale-lock recovery: a crashed or killed GC left the
  lock behind and wedged every future `bernstein worktrees gc`. The lock
  already recorded the owner pid and start time; acquisition now reclaims
  a lock once when the owning process is gone or the lock is older than a
  generous bound, while still refusing a lock held by a live recent
  process.

Author: chernistry

src/bernstein/cli/commands/worktrees_cmd.py

@@ -25,6 +25,7 @@
 from rich.console import Console
 from rich.table import Table
 
+from bernstein.core.process_utils import is_process_alive
 from bernstein.core.worktrees.classifier import (
     GC_LOCK_RELPATH,
     WORKTREE_GC_LIFECYCLE_EVENT,
@@ -136,21 +137,71 @@ class GcLockError(RuntimeError):
     """Raised when the GC lock cannot be acquired."""
 
 
+# A GC that has "owned" the lock longer than this is treated as a crashed
+# leftover. Generous so a legitimately long sweep is never reclaimed under it.
+_GC_LOCK_MAX_AGE_S = 6 * 3600
+
+
+def _read_gc_lock(lock_path: Path) -> dict[str, object] | None:
+    """Return the lock's recorded ``{pid, started_at}`` payload, or None."""
+    try:
+        data = json.loads(lock_path.read_text(encoding="utf-8"))
+    except (OSError, ValueError):
+        return None
+    return data if isinstance(data, dict) else None
+
+
+def _gc_lock_is_stale(meta: dict[str, object] | None) -> bool:
+    """True when the lock's owning process is gone or the lock is too old.
+
+    An unreadable / mid-write payload is NOT treated as stale, so a lock another
+    process just created (between ``O_EXCL`` and the write) is never reclaimed.
+    The crashed-GC case the recovery targets always leaves a fully written
+    ``{pid, started_at}`` payload, which the pid-liveness check below detects.
+    """
+    if not isinstance(meta, dict):
+        return False
+    pid = meta.get("pid")
+    if isinstance(pid, int) and pid > 0 and not is_process_alive(pid):
+        return True
+    started = meta.get("started_at")
+    return isinstance(started, (int, float)) and (time.time() - started) > _GC_LOCK_MAX_AGE_S
+
+
+def _acquire_gc_lock_fd(lock_path: Path) -> int:
+    """Open the GC lock with ``O_EXCL``, reclaiming a provably stale lock once."""
+    try:
+        return os.open(str(lock_path), os.O_CREAT | os.O_EXCL | os.O_WRONLY, 0o600)
+    except FileExistsError as exc:
+        meta = _read_gc_lock(lock_path)
+        if not _gc_lock_is_stale(meta):
+            owner = f" held by pid {meta['pid']}" if isinstance(meta, dict) and meta.get("pid") else ""
+            raise GcLockError(f"another worktree GC is already running ({lock_path}{owner})") from exc
+        logger.warning("Reclaiming stale worktree GC lock %s (previous owner gone): %s", lock_path, meta)
+        # A competing acquirer may win the race and re-create the lock; the
+        # retry below resolves that case.
+        with contextlib.suppress(OSError):
+            lock_path.unlink()
+        try:
+            return os.open(str(lock_path), os.O_CREAT | os.O_EXCL | os.O_WRONLY, 0o600)
+        except FileExistsError as retry_exc:
+            raise GcLockError(f"another worktree GC is already running ({lock_path})") from retry_exc
+
+
 @contextlib.contextmanager
 def lock_gc(repo_root: Path):  # type: ignore[no-untyped-def]
     """Acquire :data:`GC_LOCK_RELPATH` exclusively, yielding the lock path.
 
-    The lock file is created with ``O_EXCL``; a concurrent invocation
-    sees the file and raises :class:`GcLockError`. The lock is removed
-    on context exit even when the body raises.
+    The lock file is created with ``O_EXCL``; a concurrent invocation by a live
+    process raises :class:`GcLockError`. A lock left behind by a crashed or
+    killed GC (its recorded pid is no longer alive, or it is older than
+    :data:`_GC_LOCK_MAX_AGE_S`) is reclaimed once instead of wedging every future
+    run. The lock is removed on context exit even when the body raises.
     """
     lock_path = repo_root / GC_LOCK_RELPATH
     lock_path.parent.mkdir(parents=True, exist_ok=True)
 
-    try:
-        fd = os.open(str(lock_path), os.O_CREAT | os.O_EXCL | os.O_WRONLY, 0o600)
-    except FileExistsError as exc:
-        raise GcLockError(f"another worktree GC is already running ({lock_path})") from exc
+    fd = _acquire_gc_lock_fd(lock_path)
 
     try:
         with os.fdopen(fd, "w", encoding="utf-8") as fh:

src/bernstein/core/git/worktree.py

@@ -336,20 +336,80 @@ def setup_worktree_env(
             logger.warning("Failed to run worktree setup command: %s", exc)
 
 
+def _branch_exists(repo_root: Path, branch: str) -> bool:
+    """Return True if *branch* resolves to a commit in *repo_root*."""
+    try:
+        result = subprocess.run(
+            ["git", "rev-parse", "--verify", "--quiet", branch],
+            cwd=repo_root,
+            capture_output=True,
+            text=True,
+            timeout=_GRAVEYARD_GIT_TIMEOUT_S,
+        )
+    except (subprocess.TimeoutExpired, FileNotFoundError, OSError):
+        return False
+    return result.returncode == 0
+
+
+def _resolve_graveyard_base(repo_root: Path) -> str:
+    """Resolve the default-branch ref used as the merge baseline for graveyard capture.
+
+    A repository whose default branch is not ``main`` (``master``, ``trunk``,
+    ...) would otherwise lose unmerged agent work, because ``git rev-list
+    <branch> ^main`` fails when ``main`` does not exist and the failure was
+    read as "nothing to preserve". Resolution order mirrors the project's
+    canonical default-branch detection: ``origin/HEAD``, then the conventional
+    names locally and as remote-tracking refs. Falls back to ``"main"`` only as
+    a last resort.
+    """
+    try:
+        head = subprocess.run(
+            ["git", "symbolic-ref", "refs/remotes/origin/HEAD"],
+            cwd=repo_root,
+            capture_output=True,
+            text=True,
+            timeout=_GRAVEYARD_GIT_TIMEOUT_S,
+        )
+        if head.returncode == 0:
+            ref = head.stdout.strip()
+            if ref.startswith("refs/remotes/origin/"):
+                return ref.removeprefix("refs/remotes/origin/")
+        for candidate in ("main", "master", "refs/remotes/origin/main", "refs/remotes/origin/master"):
+            probe = subprocess.run(
+                ["git", "rev-parse", "--verify", "--quiet", candidate],
+                cwd=repo_root,
+                capture_output=True,
+                text=True,
+                timeout=_GRAVEYARD_GIT_TIMEOUT_S,
+            )
+            if probe.returncode == 0:
+                return candidate
+    except (subprocess.TimeoutExpired, FileNotFoundError, OSError):
+        pass
+    return "main"
+
+
 def _count_unmerged_commits(repo_root: Path, branch: str, base: str = "main") -> int:
     """Return how many commits on *branch* are not reachable from *base*.
 
-    Uses ``git rev-list <branch> ^<base> --count``.  If the branch is missing
-    or the command fails, returns ``0`` - callers treat that as "nothing to
-    preserve" which is safe because graveyard capture is best-effort.
+    Uses ``git rev-list <branch> ^<base> --count``.
+
+    Returns the real count, ``0`` when the branch is gone or genuinely fully
+    merged, and ``-1`` when the branch still exists but the count could not be
+    computed (for example the *base* ref does not exist). Callers treat any
+    non-zero result, including ``-1``, as "preserve to the graveyard" so that
+    unmerged work is never silently dropped on an inconclusive check.
 
     Args:
         repo_root: Repository root directory.
         branch: Branch name to compare against *base* (e.g. ``agent/<sid>``).
-        base: Reference to compare against.  Defaults to ``main``.
+        base: Reference to compare against. Resolve it with
+            :func:`_resolve_graveyard_base` so it matches the repo default
+            branch rather than assuming ``main``.
 
     Returns:
-        Number of commits on *branch* not in *base*.  ``0`` on any failure.
+        Number of commits on *branch* not in *base*; ``0`` when nothing is at
+        risk; ``-1`` when the check was inconclusive but the branch exists.
     """
     try:
         result = subprocess.run(
@@ -363,18 +423,18 @@ def _count_unmerged_commits(repo_root: Path, branch: str, base: str = "main") ->
         )
     except (subprocess.TimeoutExpired, FileNotFoundError, OSError) as exc:
         logger.debug("rev-list for %s failed: %s", branch, exc)
-        return 0
+        return -1 if _branch_exists(repo_root, branch) else 0
     if result.returncode != 0:
         logger.debug("rev-list for %s exited %d: %s", branch, result.returncode, result.stderr.strip())
-        return 0
+        return -1 if _branch_exists(repo_root, branch) else 0
     raw = result.stdout.strip()
     if not raw:
         return 0
     try:
         return int(raw)
     except ValueError:
         logger.debug("rev-list for %s produced non-integer output: %r", branch, raw)
-        return 0
+        return -1 if _branch_exists(repo_root, branch) else 0
 
 
 def _resolve_ref(repo_root: Path, ref: str) -> str | None:
@@ -849,16 +909,20 @@ def cleanup_all_stale(self) -> int:
                 # remove --force`` followed by ``git branch -D`` would make
                 # those commits unreachable and gc-eligible.
                 branch_name = f"agent/{session_id}"
+                base_ref = _resolve_graveyard_base(self.repo_root)
                 try:
-                    unmerged = _count_unmerged_commits(self.repo_root, branch_name, base="main")
-                except Exception as exc:  # defensive - never block cleanup
+                    unmerged = _count_unmerged_commits(self.repo_root, branch_name, base=base_ref)
+                except Exception as exc:  # defensive - preserve rather than drop work
                     logger.debug("Graveyard pre-check failed for %s: %s", session_id, exc)
-                    unmerged = 0
-                if unmerged > 0:
+                    unmerged = -1 if _branch_exists(self.repo_root, branch_name) else 0
+                # Any non-zero result preserves: >0 is a real count, -1 means the
+                # check was inconclusive but the branch exists, so we keep the work.
+                if unmerged != 0:
                     logger.warning(
-                        "Stale worktree %s has %d unmerged commit(s); preserving to graveyard",
+                        "Stale worktree %s has %s unmerged commit(s) (base %s); preserving to graveyard",
                         session_id,
-                        unmerged,
+                        unmerged if unmerged > 0 else "an unknown number of",
+                        base_ref,
                     )
                     try:
                         preserve_branch_to_graveyard(self.repo_root, session_id, branch=branch_name)

tests/unit/test_worktree_graveyard.py

@@ -244,3 +244,61 @@ def test_purge_graveyard_zero_days_purges_everything(repo: Path) -> None:
 def test_purge_graveyard_rejects_negative_age(repo: Path) -> None:
     with pytest.raises(ValueError, match=">= 0"):
         purge_graveyard(repo, older_than_days=-1)
+
+
+# ----------------------------------------------------------------------------
+# Non-`main` default branch must not lose unmerged work (gc-reliability)
+# ----------------------------------------------------------------------------
+
+
+def _init_repo_on(tmp_path: Path, branch: str) -> Path:
+    repo = tmp_path / f"repo_{branch}"
+    repo.mkdir()
+    _run(["git", "init", "-b", branch], repo)
+    _run(["git", "config", "user.email", "test@example.com"], repo)
+    _run(["git", "config", "user.name", "Test User"], repo)
+    _run(["git", "config", "commit.gpgsign", "false"], repo)
+    (repo / "README.md").write_text("seed\n", encoding="utf-8")
+    _run(["git", "add", "README.md"], repo)
+    _run(["git", "commit", "-m", "seed"], repo)
+    return repo
+
+
+def test_resolve_graveyard_base_finds_master(tmp_path: Path) -> None:
+    from bernstein.core.git.worktree import _resolve_graveyard_base
+
+    repo = _init_repo_on(tmp_path, "master")
+    assert _resolve_graveyard_base(repo) == "master"
+
+
+def test_count_unmerged_uncertain_when_base_missing_branch_exists(tmp_path: Path) -> None:
+    """On a master-default repo, the old hardcoded ^main check failed -> 0 (delete).
+
+    It must now report -1 (inconclusive but the branch exists) so the caller
+    preserves the unmerged work instead of silently dropping it.
+    """
+    repo = _init_repo_on(tmp_path, "master")
+    wt = repo / ".sdd" / "worktrees" / "sid-x"
+    wt.parent.mkdir(parents=True, exist_ok=True)
+    _run(["git", "worktree", "add", "-b", "agent/sid-x", str(wt)], repo)
+    (wt / "f.txt").write_text("work\n", encoding="utf-8")
+    _run(["git", "add", "f.txt"], wt)
+    _run(["git", "commit", "-m", "unmerged work"], wt)
+
+    assert _count_unmerged_commits(repo, "agent/sid-x", base="main") == -1
+
+
+def test_count_unmerged_accurate_with_resolved_base(tmp_path: Path) -> None:
+    from bernstein.core.git.worktree import _resolve_graveyard_base
+
+    repo = _init_repo_on(tmp_path, "master")
+    wt = repo / ".sdd" / "worktrees" / "sid-y"
+    wt.parent.mkdir(parents=True, exist_ok=True)
+    _run(["git", "worktree", "add", "-b", "agent/sid-y", str(wt)], repo)
+    for i in range(2):
+        (wt / f"f{i}.txt").write_text("x\n", encoding="utf-8")
+        _run(["git", "add", f"f{i}.txt"], wt)
+        _run(["git", "commit", "-m", f"c{i}"], wt)
+
+    base = _resolve_graveyard_base(repo)
+    assert _count_unmerged_commits(repo, "agent/sid-y", base=base) == 2

tests/unit/test_worktrees_cmd.py

@@ -772,3 +772,83 @@ def test_tampering_with_recorded_head_breaks_verify(repo_root: Path) -> None:
     valid, errors = fresh.verify()
     assert valid is False
     assert any("HMAC mismatch" in e or "non-canonical" in e for e in errors)
+
+
+# ----------------------------------------------------------------------------
+# GC lock stale recovery (gc-reliability): a crashed GC must not wedge future runs
+# ----------------------------------------------------------------------------
+
+
+def test_gc_lock_stale_when_pid_dead() -> None:
+    import time as _time
+    from unittest.mock import patch
+
+    from bernstein.cli.commands.worktrees_cmd import _gc_lock_is_stale
+
+    with patch("bernstein.cli.commands.worktrees_cmd.is_process_alive", return_value=False):
+        assert _gc_lock_is_stale({"pid": 999999, "started_at": _time.time()}) is True
+
+
+def test_gc_lock_not_stale_when_pid_alive() -> None:
+    import os as _os
+    import time as _time
+    from unittest.mock import patch
+
+    from bernstein.cli.commands.worktrees_cmd import _gc_lock_is_stale
+
+    with patch("bernstein.cli.commands.worktrees_cmd.is_process_alive", return_value=True):
+        assert _gc_lock_is_stale({"pid": _os.getpid(), "started_at": _time.time()}) is False
+
+
+def test_gc_lock_stale_when_too_old() -> None:
+    import os as _os
+    import time as _time
+    from unittest.mock import patch
+
+    from bernstein.cli.commands.worktrees_cmd import _gc_lock_is_stale
+
+    with patch("bernstein.cli.commands.worktrees_cmd.is_process_alive", return_value=True):
+        assert _gc_lock_is_stale({"pid": _os.getpid(), "started_at": _time.time() - 7 * 3600}) is True
+
+
+def test_gc_lock_unreadable_is_not_stale() -> None:
+    from bernstein.cli.commands.worktrees_cmd import _gc_lock_is_stale
+
+    assert _gc_lock_is_stale(None) is False
+
+
+def test_lock_gc_reclaims_lock_from_dead_owner(repo_root: Path) -> None:
+    """A lock left by a crashed GC (dead pid) is reclaimed, not wedged forever."""
+    import json as _json
+    import time as _time
+    from unittest.mock import patch
+
+    from bernstein.cli.commands.worktrees_cmd import GC_LOCK_RELPATH
+
+    lock_path = repo_root / GC_LOCK_RELPATH
+    lock_path.parent.mkdir(parents=True, exist_ok=True)
+    lock_path.write_text(_json.dumps({"pid": 999999, "started_at": _time.time()}), encoding="utf-8")
+
+    with patch("bernstein.cli.commands.worktrees_cmd.is_process_alive", return_value=False):
+        with lock_gc(repo_root):
+            assert lock_path.exists()
+    assert not lock_path.exists()
+
+
+def test_lock_gc_respects_live_owner(repo_root: Path) -> None:
+    """A lock held by a live, recent process is NOT reclaimed."""
+    import json as _json
+    import os as _os
+    import time as _time
+    from unittest.mock import patch
+
+    from bernstein.cli.commands.worktrees_cmd import GC_LOCK_RELPATH
+
+    lock_path = repo_root / GC_LOCK_RELPATH
+    lock_path.parent.mkdir(parents=True, exist_ok=True)
+    lock_path.write_text(_json.dumps({"pid": _os.getpid(), "started_at": _time.time()}), encoding="utf-8")
+
+    with patch("bernstein.cli.commands.worktrees_cmd.is_process_alive", return_value=True):
+        with pytest.raises(GcLockError):
+            with lock_gc(repo_root):
+                pass