From ad69fc2e19ff44ff970b90ed2f86aef9e15d8bba Mon Sep 17 00:00:00 2001 From: Mukul Sharma Date: Mon, 28 Sep 2026 15:51:33 -0700 Subject: [PATCH 1/5] Keep killed git processes from leaving index locks that block self-updates, and clear stale ones Co-Authored-By: Claude Opus 5.5 (1M context) --- .gitignore | 4 + CHANGELOG.md | 9 ++ README.md | 1 + bin/build-ws-context.py | 4 +- bin/pulse.py | 13 +++ bin/self_update.py | 90 ++++++++++++++-- tests/test_build_ws_context_in_process.py | 18 ++++ tests/test_pulse.py | 19 ++++ tests/test_self_update.py | 125 +++++++++++++++++++++- 9 files changed, 270 insertions(+), 13 deletions(-) diff --git a/.gitignore b/.gitignore index a080ebf..afe34e6 100644 --- a/.gitignore +++ b/.gitignore @@ -55,6 +55,10 @@ slack-reactor/package-lock.json # untracked, the deployed fleet's self_update.py sees a perpetually-dirty tree, # does `git stash push -u`, and loops a 24h-delay cycle forever. .worktrees/ +# Claude Code's isolated agent worktrees, and the self-update marker a test run +# inside one writes to its grandparent: the same perpetually-dirty tree. +.claude/worktrees/ +.claude/.assistant/ # Runtime state dir when an install keeps it inside the repo instead of ~ # (metrics.jsonl, observer-summaries/, heartbeat.json — never committed). diff --git a/CHANGELOG.md b/CHANGELOG.md index 3664c0f..f98476f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,15 @@ The version is carried in `pyproject.toml` and `src/assistant/__init__.py` and reminders to finish older pending work before starting another task. ### Fixed +- Keep a killed git from blocking self-updates. The pulse's timeout used to + SIGKILL a context check mid-`git status`, leaving `.git/index.lock` behind; + on 2026-09-28 that blocked every self-update, and seven repos on this machine + held such locks. Timeouts now send SIGTERM first (git removes its locks), the + background `git status` and `git log` calls take no index lock at all, and + self-update removes an index.lock older than 10 minutes that no process has + open before it touches the tree. +- Ignore Claude Code's `.claude/worktrees/` and the stray `.claude/.assistant/` + marker, which made the tree look dirty and set off daily auto-stashes. - Ping about an idle workspace only when it needs you: the agent's last lines ask you something, a permission or plan prompt is up, or the session stopped on an API error. Claude's idle alert fires about a minute after every turn, diff --git a/README.md b/README.md index 5602e39..7fe8d23 100644 --- a/README.md +++ b/README.md @@ -305,6 +305,7 @@ These are structural, not just conventions — violating them will cause real pr ## Gotchas - **Self-update refuses a dirty or ahead tree.** `self_update.py` does `git pull --ff-only` only. A dirty tree is surfaced, never steamrolled. +- **Self-update clears a stale git lock.** A `.git/index.lock` older than 10 minutes that no process has open (checked with `lsof`) is removed before the update; a young or open lock is left alone. - **NO_INGEST_GUARD:** if the last send to a workspace returned `transcript_size_delta=0` (cmux sent OK but no Claude process was reading), the orchestrator skips the next resend. This breaks the cleanup-resend-loop class of bug structurally. - **The single-process daemon (`src/assistant/`) is opt-in.** The legacy pulse LaunchAgent keeps running until you explicitly switch over. - **mem0ai requires Python 3.12.** It lives in `.venv-mem0`; `ensure_venv()` transparently re-execs tools into that interpreter. diff --git a/bin/build-ws-context.py b/bin/build-ws-context.py index dc509ba..99f9196 100755 --- a/bin/build-ws-context.py +++ b/bin/build-ws-context.py @@ -614,7 +614,7 @@ def cwd_state(cwd: str | None) -> tuple[bool, bool]: return False, False try: r = subprocess.run( - ["git", "-C", cwd, "status", "--porcelain"], + ["git", "--no-optional-locks", "-C", cwd, "status", "--porcelain"], capture_output=True, text=True, timeout=5, ) dirty = bool(r.stdout.strip()) if r.returncode == 0 else False @@ -622,7 +622,7 @@ def cwd_state(cwd: str | None) -> tuple[bool, bool]: dirty = False try: r = subprocess.run( - ["git", "-C", cwd, "log", "@{u}..", "--oneline"], + ["git", "--no-optional-locks", "-C", cwd, "log", "@{u}..", "--oneline"], capture_output=True, text=True, timeout=5, ) unpushed = bool(r.stdout.strip()) if r.returncode == 0 else False diff --git a/bin/pulse.py b/bin/pulse.py index 2860de8..86baa55 100755 --- a/bin/pulse.py +++ b/bin/pulse.py @@ -295,6 +295,10 @@ def load_bedrock_env() -> dict: _BEDROCK_ENV = load_bedrock_env() +# How long a timed-out child's process group gets after SIGTERM before SIGKILL. +KILL_GRACE_SEC = 3 + + def run(cmd: list[str], *, input_text: str | None = None, timeout: int = 30, env: dict | None = None, merge_bedrock: bool = False) -> tuple[int, str, str]: @@ -331,6 +335,15 @@ def run(cmd: list[str], *, input_text: str | None = None, out, err = proc.communicate(input=input_text, timeout=timeout) return proc.returncode, out, err except subprocess.TimeoutExpired: + # SIGTERM first: git removes its lock files on SIGTERM, but a SIGKILL + # mid-`git status` left .git/index.lock behind in seven repos and + # blocked every later git write there (2026-09-28). + try: + os.killpg(proc.pid, signal.SIGTERM) + proc.communicate(timeout=KILL_GRACE_SEC) + return 124, "", f"timeout after {timeout}s" + except (ProcessLookupError, PermissionError, OSError, subprocess.TimeoutExpired): + pass try: os.killpg(proc.pid, signal.SIGKILL) except (ProcessLookupError, PermissionError, OSError): diff --git a/bin/self_update.py b/bin/self_update.py index 5e68346..2923076 100644 --- a/bin/self_update.py +++ b/bin/self_update.py @@ -42,6 +42,9 @@ from __future__ import annotations import json +import os +import shutil +import signal import subprocess import sys import time @@ -81,18 +84,86 @@ REJECT_REMIND_SEC = 86400 # 1 day +# How long a timed-out git gets after SIGTERM to remove its own lock files +# before it's killed outright. +KILL_GRACE_SEC = 3 + +# git leaves `.git/index.lock` behind when it dies mid-write, and every later git +# write in the repo then fails. A lock no process has open, older than this, is +# stale (2026-09-28: one left by a killed `git status` blocked every self-update). +STALE_LOCK_SEC = 600 + + def _git(repo: Path, *args: str, timeout: int = 90) -> tuple[int, str, str]: - """Run a git command in `repo`. Returns (rc, stdout, stderr); never raises.""" + """Run a git command in `repo`. Returns (rc, stdout, stderr); never raises. + + `--no-optional-locks` keeps read-only commands like `status` from taking + the index lock at all. On a timeout git gets SIGTERM first — it removes + its lock files on SIGTERM, but a SIGKILL leaves them behind.""" try: - p = subprocess.run( - ["git", "-C", str(repo), *args], - capture_output=True, text=True, timeout=timeout, + proc = subprocess.Popen( + ["git", "--no-optional-locks", "-C", str(repo), *args], + stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True, + start_new_session=True, ) - return p.returncode, p.stdout.strip(), p.stderr.strip() - except subprocess.TimeoutExpired: - return -1, "", f"git {' '.join(args)} timed out after {timeout}s" except Exception as e: # noqa: BLE001 return -1, "", str(e) + try: + out, err = proc.communicate(timeout=timeout) + return proc.returncode, out.strip(), err.strip() + except subprocess.TimeoutExpired: + _terminate_group(proc) + return -1, "", f"git {' '.join(args)} timed out after {timeout}s" + + +def _terminate_group(proc: subprocess.Popen, grace: float = KILL_GRACE_SEC) -> None: + """SIGTERM a child's process group, then SIGKILL whatever is left after + `grace` seconds.""" + try: + os.killpg(proc.pid, signal.SIGTERM) + proc.communicate(timeout=grace) + except (ProcessLookupError, PermissionError, subprocess.TimeoutExpired): + try: + os.killpg(proc.pid, signal.SIGKILL) + except (ProcessLookupError, PermissionError): + proc.kill() + proc.communicate() + + +def _lock_is_held(path: Path) -> bool: + """True if some process has `path` open, or if lsof can't say for sure. + lsof exits 1 with no output when nobody has the file open.""" + lsof = shutil.which("lsof") or "/usr/sbin/lsof" + try: + p = subprocess.run([lsof, "-w", "-t", "--", str(path)], + capture_output=True, text=True, timeout=15) + except (OSError, subprocess.TimeoutExpired): + return True + return not (p.returncode == 1 and not p.stdout.strip() and not p.stderr.strip()) + + +def clear_stale_index_lock(repo: Path, *, now: float | None = None, + max_age: float = STALE_LOCK_SEC, + is_held=_lock_is_held) -> str | None: + """Remove the repo's index.lock if it's stale: older than `max_age` and + held open by no process. Returns what was removed, or None. A lock that's + young, held, or can't be checked is left alone.""" + now = time.time() if now is None else now + rc, git_dir, _ = _git(repo, "rev-parse", "--absolute-git-dir") + if rc != 0 or not git_dir: + return None + lock = Path(git_dir) / "index.lock" + try: + age = now - lock.stat().st_mtime + except OSError: + return None + if age < max_age or is_held(lock): + return None + try: + lock.unlink() + except OSError: + return None + return f"removed a stale {lock} from {int(age // 60)} min ago that no process held" def _stash_dirty(repo: Path, label: str) -> tuple[bool, str]: @@ -288,6 +359,11 @@ def _log(msg: str) -> None: result: dict = {"attempted": True, "changed": False, "installed": False, "skipped_reason": None} + cleared = clear_stale_index_lock(repo, now=now) + if cleared: + result["cleared_stale_lock"] = cleared + _log(cleared) + rb = resolve_remote_branch(repo) if rb is None: result["skipped_reason"] = "no-remote" diff --git a/tests/test_build_ws_context_in_process.py b/tests/test_build_ws_context_in_process.py index 532b016..09e2014 100644 --- a/tests/test_build_ws_context_in_process.py +++ b/tests/test_build_ws_context_in_process.py @@ -323,6 +323,24 @@ def test_returns_false_false_when_cwd_missing(self): d, u = self.mod.cwd_state("/no/such/dir-x") self.assertEqual((d, u), (False, False)) + def test_git_checks_never_rewrite_the_index(self): + """cwd_state runs under the pulse's timeout kill; a plain `git status` + rewrites the index under index.lock, and a kill at that moment leaves + the lock behind. Mutation probe: drop `--no-optional-locks` and the + index mtime changes.""" + repo = self._tmp / "repo-locks" + repo.mkdir() + subprocess.run(["git", "init", "-q"], cwd=str(repo), check=True) + (repo / "f.txt").write_text("a") + subprocess.run(["git", "add", "f.txt"], cwd=str(repo), check=True) + subprocess.run(["git", "-c", "user.email=t@t", "-c", "user.name=t", "commit", "-qm", "i"], + cwd=str(repo), check=True) + os.utime(repo / "f.txt", (1, 1)) + index = repo / ".git" / "index" + before = index.stat().st_mtime_ns + self.mod.cwd_state(str(repo)) + self.assertEqual(index.stat().st_mtime_ns, before) + def test_clean_repo(self): # Init a real git repo + empty commit so @{u} doesn't error # (subprocess just returns rc != 0 when no upstream — we treat diff --git a/tests/test_pulse.py b/tests/test_pulse.py index dee8cfc..188c0bc 100644 --- a/tests/test_pulse.py +++ b/tests/test_pulse.py @@ -645,6 +645,25 @@ def test_timeout_with_pipe_holding_grandchild_returns_promptly(self): os.kill(gpid, 9) # clean up before failing self.fail(f"grandchild {gpid} survived the group kill") + def test_timeout_sends_sigterm_first_so_children_clean_up(self): + """A SIGKILL mid-`git status` left .git/index.lock behind in seven + repos. The timeout now sends SIGTERM first, which git handles by + removing its locks. Mutation probe: go straight to SIGKILL and the + marker file is never written.""" + marker = Path(self._tmp_obj.name) / "cleaned" + rc, _, err = self.mod.run( + ["/bin/sh", "-c", f'trap "echo yes > {marker}; exit 0" TERM; while :; do sleep 0.1; done'], + timeout=1) + self.assertEqual(rc, 124) + self.assertEqual(marker.read_text().strip(), "yes") + + def test_timeout_still_kills_a_child_that_ignores_sigterm(self): + import time as _time + t0 = _time.time() + rc, _, err = self.mod.run(["/bin/sh", "-c", 'trap "" TERM; sleep 30'], timeout=1) + self.assertEqual(rc, 124) + self.assertLess(_time.time() - t0, 1 + self.mod.KILL_GRACE_SEC + 5) + def test_input_text_still_reaches_stdin(self): # The Popen rewrite must preserve the input_text contract. rc, out, _ = self.mod.run( diff --git a/tests/test_self_update.py b/tests/test_self_update.py index 110943e..afca158 100644 --- a/tests/test_self_update.py +++ b/tests/test_self_update.py @@ -13,6 +13,7 @@ import os import subprocess import sys +import time import unittest import unittest.mock from pathlib import Path @@ -467,19 +468,135 @@ def test_no_upstream_fallback_to_origin(self): self.assertEqual(rb[0], "origin") def test_git_timeout_returns_minus1(self): - import subprocess as sp - with unittest.mock.patch.object(sp, "run", side_effect=sp.TimeoutExpired("git", 5)): + class SlowGit: + pid = 4242 + + def __init__(self, *a, **k): + self.calls = 0 + + def communicate(self, timeout=None): + self.calls += 1 + if self.calls == 1: + raise subprocess.TimeoutExpired("git", timeout) + return "", "" + + killed = [] + with unittest.mock.patch.object(su.subprocess, "Popen", SlowGit), \ + unittest.mock.patch.object(su.os, "killpg", lambda pid, sig: killed.append(sig)): rc, out, err = su._git(Path("/tmp"), "status") self.assertEqual(rc, -1) self.assertIn("timed out", err) + self.assertEqual(killed, [su.signal.SIGTERM], "SIGTERM first, so git removes its locks") def test_git_os_error_returns_minus1(self): - import subprocess as sp - with unittest.mock.patch.object(sp, "run", side_effect=OSError("no git binary")): + with unittest.mock.patch.object(su.subprocess, "Popen", side_effect=OSError("no git binary")): rc, out, err = su._git(Path("/tmp"), "status") self.assertEqual(rc, -1) self.assertIn("no git binary", err) + def test_git_status_never_rewrites_the_index(self): + """`git status` normally refreshes and rewrites the index under + index.lock; killed at that moment, it leaves the lock behind. The + optional-locks flag makes it read-only. Mutation probe: drop + `--no-optional-locks` and the index mtime changes.""" + with TemporaryDirectory() as t: + clone, _ = make_repos(Path(t)) + index = clone / ".git" / "index" + os.utime(clone / "bin/pulse.py", (1, 1)) # stat-dirty, content unchanged + before = index.stat().st_mtime_ns + rc, out, _ = su._git(clone, "status", "--porcelain") + self.assertEqual((rc, out), (0, "")) + self.assertEqual(index.stat().st_mtime_ns, before) + + +class StaleLockTests(unittest.TestCase): + """2026-09-28: a `git status` killed mid-write left .git/index.lock in the + Assistant checkout, and every self-update after it failed with "Unable to + create index.lock: File exists".""" + + def _lock(self, clone: Path, age_sec: float) -> Path: + lock = clone / ".git" / "index.lock" + lock.write_text("") + t = time.time() - age_sec + os.utime(lock, (t, t)) + return lock + + def test_old_unheld_lock_is_removed(self): + with TemporaryDirectory() as t: + clone, _ = make_repos(Path(t)) + lock = self._lock(clone, 3600) + msg = su.clear_stale_index_lock(clone, is_held=lambda p: False) + self.assertFalse(lock.exists()) + self.assertIn("60 min ago", msg) + + def test_young_held_or_missing_locks_are_left_alone(self): + with TemporaryDirectory() as t: + clone, _ = make_repos(Path(t)) + self.assertIsNone(su.clear_stale_index_lock(clone, is_held=lambda p: False)) + lock = self._lock(clone, 60) + self.assertIsNone(su.clear_stale_index_lock(clone, is_held=lambda p: False)) + self._lock(clone, 3600) + self.assertIsNone(su.clear_stale_index_lock(clone, is_held=lambda p: True)) + self.assertTrue(lock.exists()) + self.assertIsNone(su.clear_stale_index_lock(Path(t) / "not-a-repo")) + + def test_lock_that_cant_be_removed_is_reported_as_not_cleared(self): + with TemporaryDirectory() as t: + clone, _ = make_repos(Path(t)) + lock = clone / ".git" / "index.lock" + lock.mkdir() + os.utime(lock, (1, 1)) + self.assertIsNone(su.clear_stale_index_lock(clone, is_held=lambda p: False)) + + def test_lock_is_held_uses_lsof(self): + with TemporaryDirectory() as t: + path = Path(t) / "index.lock" + path.write_text("") + self.assertFalse(su._lock_is_held(path)) + with open(path) as held: + self.assertTrue(su._lock_is_held(path), "a lock git still has open is live") + held.read() + with unittest.mock.patch.object(su.subprocess, "run", side_effect=OSError("no lsof")): + self.assertTrue(su._lock_is_held(path), "unknown means leave it alone") + + def test_update_goes_through_a_stale_lock(self): + """End to end with the real lsof: the stale lock is cleared and the + fast-forward lands. Mutation probe: skip the clear and the result is + pull-failed.""" + with TemporaryDirectory() as t: + tmp = Path(t) + clone, remote = make_repos(tmp) + new_sha = advance_remote(tmp, remote, {"bin/pulse.py": "# v2\n"}, "v2") + self._lock(clone, 3600) + r = su.maybe_update(clone, marker_path=tmp / "marker.json", + install_sh=tmp / "no-install.sh") + self.assertTrue(r["changed"], r) + self.assertEqual(git(clone, "rev-parse", "HEAD"), new_sha) + self.assertIn("stale", r["cleared_stale_lock"]) + + +class TerminateGroupTests(unittest.TestCase): + def test_sigterm_lets_the_child_clean_up(self): + with TemporaryDirectory() as t: + lock = Path(t) / "index.lock" + lock.write_text("") + proc = subprocess.Popen( + ["/bin/sh", "-c", f'trap "rm -f {lock}; exit 0" TERM; while :; do sleep 0.1; done'], + stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True, start_new_session=True) + time.sleep(0.3) + su._terminate_group(proc, grace=5) + self.assertFalse(lock.exists(), "the child's TERM handler ran") + + def test_a_child_that_ignores_sigterm_is_killed(self): + proc = subprocess.Popen(["/bin/sh", "-c", 'trap "" TERM; sleep 30'], + stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True, + start_new_session=True) + time.sleep(0.3) + t0 = time.time() + su._terminate_group(proc, grace=0.5) + self.assertLess(time.time() - t0, 10) + self.assertIsNotNone(proc.returncode) + class SyntaxGateTests(unittest.TestCase): """The pre-pull gate: fetched commits whose Python won't parse (conflict From 0b609c822e0fad6feb2d7a99c2677a9747ee5d6c Mon Sep 17 00:00:00 2001 From: Mukul Sharma Date: Mon, 28 Sep 2026 15:55:17 -0700 Subject: [PATCH 2/5] Cover the fallback kill when the process group is gone Co-Authored-By: Claude Opus 5.5 (1M context) --- tests/test_self_update.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/test_self_update.py b/tests/test_self_update.py index afca158..4b3a030 100644 --- a/tests/test_self_update.py +++ b/tests/test_self_update.py @@ -587,6 +587,26 @@ def test_sigterm_lets_the_child_clean_up(self): su._terminate_group(proc, grace=5) self.assertFalse(lock.exists(), "the child's TERM handler ran") + def test_a_group_that_is_already_gone_falls_back_to_killing_the_child(self): + class Gone: + pid = 999999 + killed = False + + def kill(self): + self.killed = True + + def communicate(self, timeout=None): + return "", "" + + proc = Gone() + + def no_group(pid, sig): + raise ProcessLookupError + + with unittest.mock.patch.object(su.os, "killpg", no_group): + su._terminate_group(proc, grace=0.1) + self.assertTrue(proc.killed) + def test_a_child_that_ignores_sigterm_is_killed(self): proc = subprocess.Popen(["/bin/sh", "-c", 'trap "" TERM; sleep 30'], stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True, From 2468f01f9fdbc43c454c0266fc7b431d15e89cc9 Mon Sep 17 00:00:00 2001 From: Mukul Sharma Date: Mon, 28 Sep 2026 16:12:57 -0700 Subject: [PATCH 3/5] Treat a lock as live while any git works in the repo, re-check before deleting, bound every kill wait, and ledger each clear Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 9 ++-- README.md | 2 +- bin/comms_lib.py | 2 + bin/pulse.py | 22 +++++++- bin/self_update.py | 86 +++++++++++++++++++++-------- src/assistant/slack.py | 2 + tests/test_pulse.py | 18 +++++++ tests/test_pulse_topup.py | 15 ++++++ tests/test_self_update.py | 111 ++++++++++++++++++++++++++++++++++---- 9 files changed, 229 insertions(+), 38 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f98476f..3ed2f42 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,10 +43,11 @@ The version is carried in `pyproject.toml` and `src/assistant/__init__.py` - Keep a killed git from blocking self-updates. The pulse's timeout used to SIGKILL a context check mid-`git status`, leaving `.git/index.lock` behind; on 2026-09-28 that blocked every self-update, and seven repos on this machine - held such locks. Timeouts now send SIGTERM first (git removes its locks), the - background `git status` and `git log` calls take no index lock at all, and - self-update removes an index.lock older than 10 minutes that no process has - open before it touches the tree. + held such locks. Timeouts now send SIGTERM first (git removes its locks) and + still SIGKILL the group after 3 seconds, the background `git status` and + `git log` calls take no index lock at all, and self-update removes an + index.lock older than 10 minutes when no process has it open and no git + process is working in the repo. Each removal is recorded in the ledger. - Ignore Claude Code's `.claude/worktrees/` and the stray `.claude/.assistant/` marker, which made the tree look dirty and set off daily auto-stashes. - Ping about an idle workspace only when it needs you: the agent's last lines diff --git a/README.md b/README.md index 7fe8d23..0db8fe3 100644 --- a/README.md +++ b/README.md @@ -305,7 +305,7 @@ These are structural, not just conventions — violating them will cause real pr ## Gotchas - **Self-update refuses a dirty or ahead tree.** `self_update.py` does `git pull --ff-only` only. A dirty tree is surfaced, never steamrolled. -- **Self-update clears a stale git lock.** A `.git/index.lock` older than 10 minutes that no process has open (checked with `lsof`) is removed before the update; a young or open lock is left alone. +- **Self-update clears a stale git lock.** A `.git/index.lock` older than 10 minutes is removed before the update when `lsof` shows no process has it open and no git process working in the repo (a `git commit` waiting on its editor holds the lock without keeping the file open). A young or live lock is left alone, and every removal is ledgered. - **NO_INGEST_GUARD:** if the last send to a workspace returned `transcript_size_delta=0` (cmux sent OK but no Claude process was reading), the orchestrator skips the next resend. This breaks the cleanup-resend-loop class of bug structurally. - **The single-process daemon (`src/assistant/`) is opt-in.** The legacy pulse LaunchAgent keeps running until you explicitly switch over. - **mem0ai requires Python 3.12.** It lives in `.venv-mem0`; `ensure_venv()` transparently re-execs tools into that interpreter. diff --git a/bin/comms_lib.py b/bin/comms_lib.py index 42c313f..87c0551 100644 --- a/bin/comms_lib.py +++ b/bin/comms_lib.py @@ -241,6 +241,8 @@ def _clip(text: str, limit: int) -> str: _ACTION_PHRASES: dict[str, tuple[str, str]] = { "ready_for_merge": ("asked a workspace to merge its PR", "ask a workspace to merge its PR"), "self-update": ("updated Assistant to the latest code", "update Assistant to the latest code"), + "self-update-lock-cleared": ("cleared a stale git lock that was blocking my updates", + "clear a stale git lock that was blocking my updates"), "self-update-syntax-fail": ("updated Assistant to the latest code", "update Assistant to the latest code"), "strategist-context": ("started researching a decision that's waiting on you", diff --git a/bin/pulse.py b/bin/pulse.py index 86baa55..a159d84 100755 --- a/bin/pulse.py +++ b/bin/pulse.py @@ -340,10 +340,13 @@ def run(cmd: list[str], *, input_text: str | None = None, # blocked every later git write there (2026-09-28). try: os.killpg(proc.pid, signal.SIGTERM) + except (ProcessLookupError, PermissionError, OSError): + pass + try: proc.communicate(timeout=KILL_GRACE_SEC) - return 124, "", f"timeout after {timeout}s" - except (ProcessLookupError, PermissionError, OSError, subprocess.TimeoutExpired): + except Exception: # noqa: BLE001 — a timeout here just means SIGKILL next pass + # SIGKILL the group regardless, so nothing that ignored SIGTERM survives. try: os.killpg(proc.pid, signal.SIGKILL) except (ProcessLookupError, PermissionError, OSError): @@ -468,6 +471,21 @@ def self_update_pulse(pulse_idx: int) -> None: if result is None: return # throttled — nothing to report + if result.get("cleared_stale_lock"): + # Something left a git lock behind again; make it visible, since the + # cleanup would otherwise hide a new cause. + append_ledger({ + "ts": utc_iso(), + "epoch": utc_ts(), + "pulse_idx": pulse_idx, + "key": f"self-update-lock-cleared-p{pulse_idx}", + "kind": "self-update-lock-cleared", + "ws_ref": "(launchd)", + "outcome": "verified", + "evidence": result["cleared_stale_lock"][:300], + }) + log.info("self-update: %s", result["cleared_stale_lock"]) + reason = result.get("skipped_reason") changed = result.get("changed") # Silent path: attempted, nothing to do, no problem — or a refused commit diff --git a/bin/self_update.py b/bin/self_update.py index 2923076..d14af79 100644 --- a/bin/self_update.py +++ b/bin/self_update.py @@ -114,56 +114,100 @@ def _git(repo: Path, *args: str, timeout: int = 90) -> tuple[int, str, str]: except subprocess.TimeoutExpired: _terminate_group(proc) return -1, "", f"git {' '.join(args)} timed out after {timeout}s" + except Exception as e: # noqa: BLE001 — e.g. undecodable output; never raise + _terminate_group(proc) + return -1, "", str(e) def _terminate_group(proc: subprocess.Popen, grace: float = KILL_GRACE_SEC) -> None: - """SIGTERM a child's process group, then SIGKILL whatever is left after - `grace` seconds.""" + """SIGTERM a child's process group, give it `grace` seconds, then SIGKILL + the group regardless, so nothing that ignored SIGTERM survives. Every wait + is bounded.""" try: os.killpg(proc.pid, signal.SIGTERM) + except (ProcessLookupError, PermissionError): + pass + try: proc.communicate(timeout=grace) - except (ProcessLookupError, PermissionError, subprocess.TimeoutExpired): - try: - os.killpg(proc.pid, signal.SIGKILL) - except (ProcessLookupError, PermissionError): - proc.kill() - proc.communicate() + except Exception: # noqa: BLE001 — a timeout here just means SIGKILL next + pass + try: + os.killpg(proc.pid, signal.SIGKILL) + except (ProcessLookupError, PermissionError): + proc.kill() + try: + proc.communicate(timeout=5) + except Exception: # noqa: BLE001 — reaping is best-effort after SIGKILL + pass -def _lock_is_held(path: Path) -> bool: - """True if some process has `path` open, or if lsof can't say for sure. - lsof exits 1 with no output when nobody has the file open.""" +def _lsof(*args: str) -> subprocess.CompletedProcess | None: lsof = shutil.which("lsof") or "/usr/sbin/lsof" try: - p = subprocess.run([lsof, "-w", "-t", "--", str(path)], - capture_output=True, text=True, timeout=15) + return subprocess.run([lsof, "-w", *args], capture_output=True, text=True, timeout=15) except (OSError, subprocess.TimeoutExpired): + return None + + +def _lsof_found_nothing(p: subprocess.CompletedProcess | None) -> bool: + """lsof exits 1 with no output when nothing matches; anything else means + something matched or lsof couldn't tell.""" + return p is not None and p.returncode == 1 and not p.stdout.strip() and not p.stderr.strip() + + +def git_cwd_in(lsof_fields: str, toplevel: str) -> bool: + """True if `lsof -Fn` output lists a working directory inside `toplevel`.""" + for line in lsof_fields.splitlines(): + if line.startswith("n"): + cwd = os.path.realpath(line[1:]) + if cwd == toplevel or cwd.startswith(toplevel.rstrip("/") + "/"): + return True + return False + + +def _lock_is_held(lock: Path, toplevel: str) -> bool: + """True if the lock may be live, or if that can't be ruled out. + + Two checks, because git doesn't always keep the lock file open: `git commit + -a` or `-p` writes index.lock, closes it, and keeps holding the lock while + hooks and the editor run. So a lock is live if any process has it open, or + if any git process is working in the repo (git runs from the top folder).""" + if not _lsof_found_nothing(_lsof("-t", "--", str(lock))): + return True + p = _lsof("-a", "-c", "git", "-d", "cwd", "-Fn") + if p is None or p.returncode not in (0, 1) or p.stderr.strip(): return True - return not (p.returncode == 1 and not p.stdout.strip() and not p.stderr.strip()) + return git_cwd_in(p.stdout, toplevel) def clear_stale_index_lock(repo: Path, *, now: float | None = None, max_age: float = STALE_LOCK_SEC, is_held=_lock_is_held) -> str | None: - """Remove the repo's index.lock if it's stale: older than `max_age` and - held open by no process. Returns what was removed, or None. A lock that's - young, held, or can't be checked is left alone.""" + """Remove the repo's index.lock if it's stale: older than `max_age`, with + no process holding it and no git working in the repo. Returns what was + removed, or None. A lock that's young, live, replaced while it was being + checked, or can't be checked is left alone.""" now = time.time() if now is None else now rc, git_dir, _ = _git(repo, "rev-parse", "--absolute-git-dir") - if rc != 0 or not git_dir: + rc2, toplevel, _ = _git(repo, "rev-parse", "--show-toplevel") + if rc != 0 or rc2 != 0 or not git_dir or not toplevel: return None lock = Path(git_dir) / "index.lock" try: - age = now - lock.stat().st_mtime + before = lock.stat() except OSError: return None - if age < max_age or is_held(lock): + age = now - before.st_mtime + if age < max_age or is_held(lock, os.path.realpath(toplevel)): return None try: + after = lock.stat() + if (after.st_ino, after.st_mtime_ns) != (before.st_ino, before.st_mtime_ns): + return None lock.unlink() except OSError: return None - return f"removed a stale {lock} from {int(age // 60)} min ago that no process held" + return f"removed a stale {lock} from {int(age // 60)} min ago that no git process held" def _stash_dirty(repo: Path, label: str) -> tuple[bool, str]: diff --git a/src/assistant/slack.py b/src/assistant/slack.py index a8504db..ebfc598 100644 --- a/src/assistant/slack.py +++ b/src/assistant/slack.py @@ -62,6 +62,8 @@ def _clip(text: str, limit: int) -> str: _ACTION_PHRASES: dict[str, tuple[str, str]] = { "ready_for_merge": ("asked a workspace to merge its PR", "ask a workspace to merge its PR"), "self-update": ("updated Assistant to the latest code", "update Assistant to the latest code"), + "self-update-lock-cleared": ("cleared a stale git lock that was blocking my updates", + "clear a stale git lock that was blocking my updates"), "self-update-syntax-fail": ("updated Assistant to the latest code", "update Assistant to the latest code"), "strategist-context": ("started researching a decision that's waiting on you", diff --git a/tests/test_pulse.py b/tests/test_pulse.py index 188c0bc..82f41e5 100644 --- a/tests/test_pulse.py +++ b/tests/test_pulse.py @@ -657,6 +657,24 @@ def test_timeout_sends_sigterm_first_so_children_clean_up(self): self.assertEqual(rc, 124) self.assertEqual(marker.read_text().strip(), "yes") + def test_group_members_that_ignore_sigterm_die_even_after_the_child_exits(self): + pid_file = Path(self._tmp_obj.name) / "stubborn.pid" + script = (f'(trap "" TERM; exec sleep 30) >/dev/null 2>&1 & echo $! > {pid_file}; ' + 'trap "exit 0" TERM; while :; do sleep 0.1; done') + rc, _, _ = self.mod.run(["/bin/sh", "-c", script], timeout=1) + self.assertEqual(rc, 124) + gpid = int(pid_file.read_text().strip()) + import time as _time + for _ in range(50): + try: + os.kill(gpid, 0) + except ProcessLookupError: + break + _time.sleep(0.1) + else: + os.kill(gpid, 9) + self.fail("a group member that ignored SIGTERM survived") + def test_timeout_still_kills_a_child_that_ignores_sigterm(self): import time as _time t0 = _time.time() diff --git a/tests/test_pulse_topup.py b/tests/test_pulse_topup.py index ef9c8ca..0fefa25 100644 --- a/tests/test_pulse_topup.py +++ b/tests/test_pulse_topup.py @@ -88,6 +88,21 @@ def test_self_update_none_throttled_no_ledger(mod, home): assert _read_ledger(home) == [] +def test_cleared_stale_lock_is_ledgered_even_when_nothing_else_happened(mod, home): + """A cleared lock means something left a git lock behind again; the ledger + shows it so the cleanup can't hide a new cause.""" + _inject_self_update(mod, {"changed": False, "skipped_reason": None, "error": None, + "cleared_stale_lock": "removed a stale /r/.git/index.lock"}) + try: + mod.self_update_pulse(9) + finally: + sys.modules.pop("self_update", None) + [entry] = _read_ledger(home) + assert entry["kind"] == "self-update-lock-cleared" + assert entry["key"] == "self-update-lock-cleared-p9" + assert entry["evidence"] == "removed a stale /r/.git/index.lock" + + def test_self_update_clean_no_change_silent(mod, home): _inject_self_update(mod, {"changed": False, "skipped_reason": None, "error": None}) try: diff --git a/tests/test_self_update.py b/tests/test_self_update.py index 4b3a030..fde54c7 100644 --- a/tests/test_self_update.py +++ b/tests/test_self_update.py @@ -486,7 +486,29 @@ def communicate(self, timeout=None): rc, out, err = su._git(Path("/tmp"), "status") self.assertEqual(rc, -1) self.assertIn("timed out", err) - self.assertEqual(killed, [su.signal.SIGTERM], "SIGTERM first, so git removes its locks") + self.assertEqual(killed, [su.signal.SIGTERM, su.signal.SIGKILL], + "SIGTERM first so git removes its locks, then SIGKILL for stragglers") + + def test_git_output_it_cant_decode_returns_minus1(self): + class BadBytes: + pid = 4243 + + def __init__(self, *a, **k): + self.calls = 0 + + def communicate(self, timeout=None): + self.calls += 1 + if self.calls == 1: + raise UnicodeDecodeError("utf-8", b"\xff", 0, 1, "invalid start byte") + return "", "" + + signals = [] + with unittest.mock.patch.object(su.subprocess, "Popen", BadBytes), \ + unittest.mock.patch.object(su.os, "killpg", lambda pid, sig: signals.append(sig)): + rc, _, err = su._git(Path("/tmp"), "show") + self.assertEqual(rc, -1) + self.assertIn("invalid start byte", err) + self.assertEqual(signals, [su.signal.SIGTERM, su.signal.SIGKILL]) def test_git_os_error_returns_minus1(self): with unittest.mock.patch.object(su.subprocess, "Popen", side_effect=OSError("no git binary")): @@ -525,18 +547,18 @@ def test_old_unheld_lock_is_removed(self): with TemporaryDirectory() as t: clone, _ = make_repos(Path(t)) lock = self._lock(clone, 3600) - msg = su.clear_stale_index_lock(clone, is_held=lambda p: False) + msg = su.clear_stale_index_lock(clone, is_held=lambda p, t: False) self.assertFalse(lock.exists()) self.assertIn("60 min ago", msg) def test_young_held_or_missing_locks_are_left_alone(self): with TemporaryDirectory() as t: clone, _ = make_repos(Path(t)) - self.assertIsNone(su.clear_stale_index_lock(clone, is_held=lambda p: False)) + self.assertIsNone(su.clear_stale_index_lock(clone, is_held=lambda p, t: False)) lock = self._lock(clone, 60) - self.assertIsNone(su.clear_stale_index_lock(clone, is_held=lambda p: False)) + self.assertIsNone(su.clear_stale_index_lock(clone, is_held=lambda p, t: False)) self._lock(clone, 3600) - self.assertIsNone(su.clear_stale_index_lock(clone, is_held=lambda p: True)) + self.assertIsNone(su.clear_stale_index_lock(clone, is_held=lambda p, t: True)) self.assertTrue(lock.exists()) self.assertIsNone(su.clear_stale_index_lock(Path(t) / "not-a-repo")) @@ -546,18 +568,66 @@ def test_lock_that_cant_be_removed_is_reported_as_not_cleared(self): lock = clone / ".git" / "index.lock" lock.mkdir() os.utime(lock, (1, 1)) - self.assertIsNone(su.clear_stale_index_lock(clone, is_held=lambda p: False)) + self.assertIsNone(su.clear_stale_index_lock(clone, is_held=lambda p, t: False)) def test_lock_is_held_uses_lsof(self): with TemporaryDirectory() as t: - path = Path(t) / "index.lock" + top = os.path.realpath(t) + path = Path(top) / "index.lock" path.write_text("") - self.assertFalse(su._lock_is_held(path)) + self.assertFalse(su._lock_is_held(path, top)) with open(path) as held: - self.assertTrue(su._lock_is_held(path), "a lock git still has open is live") + self.assertTrue(su._lock_is_held(path, top), "a lock git still has open is live") held.read() with unittest.mock.patch.object(su.subprocess, "run", side_effect=OSError("no lsof")): - self.assertTrue(su._lock_is_held(path), "unknown means leave it alone") + self.assertTrue(su._lock_is_held(path, top), "unknown means leave it alone") + + def test_a_git_working_in_the_repo_keeps_its_lock(self): + """`git commit -a` closes index.lock but keeps the lock while hooks and + the editor run, so an open-file check alone would delete it. Mutation + probe: drop the git-cwd check and this returns False.""" + with TemporaryDirectory() as t: + clone, _ = make_repos(Path(t)) + top = os.path.realpath(clone) + lock = clone / ".git" / "index.lock" + lock.write_text("") + live = subprocess.Popen(["git", "-C", str(clone), "hash-object", "--stdin"], + stdin=subprocess.PIPE, stdout=subprocess.PIPE) + try: + time.sleep(0.3) + self.assertTrue(su._lock_is_held(lock, top)) + finally: + live.communicate(b"") + self.assertFalse(su._lock_is_held(lock, top)) + + def test_lsof_errors_count_as_held(self): + with TemporaryDirectory() as t: + path = Path(t) / "index.lock" + path.write_text("") + odd = subprocess.CompletedProcess([], 1, "", "lsof: status error") + calls = iter([subprocess.CompletedProcess([], 1, "", ""), odd]) + with unittest.mock.patch.object(su, "_lsof", lambda *a: next(calls)): + self.assertTrue(su._lock_is_held(path, t)) + + def test_git_cwd_in(self): + fields = "p1\nn/Users/me/dev/assistant/bin\np2\nn/Users/me/dev/other\n" + self.assertTrue(su.git_cwd_in(fields, "/Users/me/dev/assistant")) + self.assertTrue(su.git_cwd_in("p1\nn/Users/me/dev/assistant\n", "/Users/me/dev/assistant")) + self.assertFalse(su.git_cwd_in("p1\nn/Users/me/dev/assistant-old\n", "/Users/me/dev/assistant")) + self.assertFalse(su.git_cwd_in("", "/Users/me/dev/assistant")) + + def test_a_lock_replaced_during_the_check_is_kept(self): + with TemporaryDirectory() as t: + clone, _ = make_repos(Path(t)) + lock = self._lock(clone, 3600) + + def replaced(p, top): + p.unlink() + p.write_text("new live lock") + return False + + self.assertIsNone(su.clear_stale_index_lock(clone, is_held=replaced)) + self.assertEqual(lock.read_text(), "new live lock") def test_update_goes_through_a_stale_lock(self): """End to end with the real lsof: the stale lock is cleared and the @@ -607,6 +677,27 @@ def no_group(pid, sig): su._terminate_group(proc, grace=0.1) self.assertTrue(proc.killed) + def test_the_final_wait_is_bounded_when_an_escaped_process_holds_the_pipes(self): + """A grandchild in its own session keeps the pipes open after the group + is killed; the reap must still return instead of stalling the pulse.""" + with TemporaryDirectory() as t: + pid_file = Path(t) / "grandchild.pid" + script = ("import signal, subprocess, time; signal.signal(signal.SIGTERM, signal.SIG_IGN); " + "g = subprocess.Popen(['sleep', '30'], start_new_session=True); " + f"open({str(pid_file)!r}, 'w').write(str(g.pid)); time.sleep(30)") + proc = subprocess.Popen([sys.executable, "-c", script], stdout=subprocess.PIPE, + stderr=subprocess.PIPE, text=True, start_new_session=True) + for _ in range(50): + if pid_file.exists() and pid_file.read_text(): + break + time.sleep(0.1) + t0 = time.time() + try: + su._terminate_group(proc, grace=0.5) + self.assertLess(time.time() - t0, 9) + finally: + os.kill(int(pid_file.read_text()), 9) + def test_a_child_that_ignores_sigterm_is_killed(self): proc = subprocess.Popen(["/bin/sh", "-c", 'trap "" TERM; sleep 30'], stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True, From 3eefb862001e4d7582c09d65b1bb574059e23a49 Mon Sep 17 00:00:00 2001 From: Mukul Sharma Date: Mon, 28 Sep 2026 16:17:06 -0700 Subject: [PATCH 4/5] Cover a timeout whose SIGTERM can't be sent Co-Authored-By: Claude Opus 5.5 (1M context) --- tests/test_pulse.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/tests/test_pulse.py b/tests/test_pulse.py index 82f41e5..09500bb 100644 --- a/tests/test_pulse.py +++ b/tests/test_pulse.py @@ -11,11 +11,13 @@ import io import json import os +import signal import subprocess import sys import textwrap import time import unittest +import unittest.mock from pathlib import Path from tempfile import TemporaryDirectory from unittest import mock @@ -675,6 +677,19 @@ def test_group_members_that_ignore_sigterm_die_even_after_the_child_exits(self): os.kill(gpid, 9) self.fail("a group member that ignored SIGTERM survived") + def test_timeout_kills_the_child_even_if_sigterm_cant_be_sent(self): + real_killpg = os.killpg + + def no_term(pid, sig): + if sig == signal.SIGTERM: + raise ProcessLookupError + real_killpg(pid, sig) + + with unittest.mock.patch.object(self.mod.os, "killpg", no_term): + rc, _, err = self.mod.run([sys.executable, "-c", "import time; time.sleep(30)"], + timeout=1) + self.assertEqual(rc, 124) + def test_timeout_still_kills_a_child_that_ignores_sigterm(self): import time as _time t0 = _time.time() From baee6af197f8bb78fb81551770c72d3cee066570 Mon Sep 17 00:00:00 2001 From: Mukul Sharma Date: Mon, 28 Sep 2026 16:30:19 -0700 Subject: [PATCH 5/5] Count only real git processes in this worktree as holding its lock Co-Authored-By: Claude Opus 5.5 (1M context) --- bin/self_update.py | 27 ++++++++++++++++++++++----- tests/test_self_update.py | 25 ++++++++++++++++++++----- 2 files changed, 42 insertions(+), 10 deletions(-) diff --git a/bin/self_update.py b/bin/self_update.py index d14af79..25faa24 100644 --- a/bin/self_update.py +++ b/bin/self_update.py @@ -155,12 +155,29 @@ def _lsof_found_nothing(p: subprocess.CompletedProcess | None) -> bool: return p is not None and p.returncode == 1 and not p.stdout.strip() and not p.stderr.strip() +def _in_this_worktree(cwd: str, toplevel: str) -> bool: + """True if `cwd` is `toplevel` or inside it without crossing into a nested + repo or worktree (a folder with its own `.git`), which has its own index.""" + top = Path(toplevel) + path = Path(cwd) + if path != top and top not in path.parents: + return False + while path != top: + if (path / ".git").exists(): + return False + path = path.parent + return True + + def git_cwd_in(lsof_fields: str, toplevel: str) -> bool: - """True if `lsof -Fn` output lists a working directory inside `toplevel`.""" + """True if `lsof -Fcn` output lists a git process (`git`, or a `git-*` + helper — not look-alikes such as `gitstatusd`) working in `toplevel`.""" + command = "" for line in lsof_fields.splitlines(): - if line.startswith("n"): - cwd = os.path.realpath(line[1:]) - if cwd == toplevel or cwd.startswith(toplevel.rstrip("/") + "/"): + if line.startswith("c"): + command = line[1:] + elif line.startswith("n") and (command == "git" or command.startswith("git-")): + if _in_this_worktree(os.path.realpath(line[1:]), toplevel): return True return False @@ -174,7 +191,7 @@ def _lock_is_held(lock: Path, toplevel: str) -> bool: if any git process is working in the repo (git runs from the top folder).""" if not _lsof_found_nothing(_lsof("-t", "--", str(lock))): return True - p = _lsof("-a", "-c", "git", "-d", "cwd", "-Fn") + p = _lsof("-a", "-c", "git", "-d", "cwd", "-Fcn") if p is None or p.returncode not in (0, 1) or p.stderr.strip(): return True return git_cwd_in(p.stdout, toplevel) diff --git a/tests/test_self_update.py b/tests/test_self_update.py index fde54c7..77d08eb 100644 --- a/tests/test_self_update.py +++ b/tests/test_self_update.py @@ -610,11 +610,26 @@ def test_lsof_errors_count_as_held(self): self.assertTrue(su._lock_is_held(path, t)) def test_git_cwd_in(self): - fields = "p1\nn/Users/me/dev/assistant/bin\np2\nn/Users/me/dev/other\n" - self.assertTrue(su.git_cwd_in(fields, "/Users/me/dev/assistant")) - self.assertTrue(su.git_cwd_in("p1\nn/Users/me/dev/assistant\n", "/Users/me/dev/assistant")) - self.assertFalse(su.git_cwd_in("p1\nn/Users/me/dev/assistant-old\n", "/Users/me/dev/assistant")) - self.assertFalse(su.git_cwd_in("", "/Users/me/dev/assistant")) + with TemporaryDirectory() as t: + top = os.path.realpath(t) + (Path(top) / "bin").mkdir() + nested = Path(top) / ".worktrees" / "feature" + nested.mkdir(parents=True) + (nested / ".git").write_text("gitdir: elsewhere") + other = f"{top}-old" + + def fields(command, cwd): + return f"p1\nc{command}\nn{cwd}\n" + + self.assertTrue(su.git_cwd_in(fields("git", f"{top}/bin"), top)) + self.assertTrue(su.git_cwd_in(fields("git", top), top)) + self.assertTrue(su.git_cwd_in(fields("git-remote-https", top), top)) + self.assertFalse(su.git_cwd_in(fields("gitstatusd", top), top), + "a look-alike process isn't git") + self.assertFalse(su.git_cwd_in(fields("git", str(nested)), top), + "a nested worktree has its own index") + self.assertFalse(su.git_cwd_in(fields("git", other), top)) + self.assertFalse(su.git_cwd_in("", top)) def test_a_lock_replaced_during_the_check_is_kept(self): with TemporaryDirectory() as t: