From df3d41ee67381043bfb6085888781069dc3221f9 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Wed, 26 Aug 2026 16:27:15 -0700 Subject: [PATCH] fix(update): sweep aborted-fetch tmp_pack debris before it corrupts the pack directory (#93732) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every git fetch that dies mid-transfer (timeout, HTTP 429, dropped line) strands a tmp_pack_* file in .git/objects/pack, and git never cleans them. The banner's background update check is the main generator on flaky lines — several aborted fetches a day — and the reporter's install accumulated hundreds of files / 6.0 GB over 9 days until the pack directory corrupted outright and every update check hung or failed permanently. clear_stale_tmp_packs() in gitlock.py sweeps tmp_pack_/tmp_idx_/ tmp_rev_/tmp_mtimes_ debris with the exact safety contract the lock sweep already uses: only files past the 10-minute age floor, never while any git process runs, never raises, real pack-*.pack/.idx files untouchable by construction (prefix match). Wired into all three fetch-adjacent sites: _cmd_update_check, the update apply path, and the banner's passive check (generator = janitor). Live E2E: 300 aged tmp_pack files (the reported scale-shape) swept from a real repo; an in-flight fresh tmp and ancient real packs survived; fsck clean and a real fetch round-trip succeeded after. --- hermes_cli/banner.py | 6 +- hermes_cli/gitlock.py | 65 +++++++++++++ hermes_cli/update_cmd.py | 13 ++- tests/hermes_cli/test_gitlock_tmp_packs.py | 101 +++++++++++++++++++++ 4 files changed, 182 insertions(+), 3 deletions(-) create mode 100644 tests/hermes_cli/test_gitlock_tmp_packs.py diff --git a/hermes_cli/banner.py b/hermes_cli/banner.py index 23042a45d4..898ba2a2b7 100644 --- a/hermes_cli/banner.py +++ b/hermes_cli/banner.py @@ -324,9 +324,13 @@ def _check_via_local_git(repo_dir: Path) -> Optional[int]: # exception below is swallowed, and stale refs get compared against # HEAD — silently degrading the passive check until a human removes # the lock (git never self-heals these). - from hermes_cli.gitlock import clear_stale_git_locks + from hermes_cli.gitlock import clear_stale_git_locks, clear_stale_tmp_packs clear_stale_git_locks(repo_dir) + # The passive check is the main tmp_pack GENERATOR on flaky lines + # (several aborted fetches per day) — it must also be the janitor, + # or debris accumulates unbounded between manual updates (#93732). + clear_stale_tmp_packs(repo_dir) # Scope the fetch to the one branch the behind-count compares against. # An unscoped ``git fetch origin`` transfers every remote head (~1,400 diff --git a/hermes_cli/gitlock.py b/hermes_cli/gitlock.py index 1485f78f7f..120fd925af 100644 --- a/hermes_cli/gitlock.py +++ b/hermes_cli/gitlock.py @@ -15,6 +15,11 @@ This module provides two small, defensive helpers used by the update paths: * :func:`clear_stale_git_locks` — remove abandoned ``.git`` lock files (with an age + git-process guard so a live fetch is never yanked). +* :func:`clear_stale_tmp_packs` — remove aborted-fetch ``tmp_pack_*`` / + ``tmp_idx_*`` debris from ``.git/objects/pack``. On flaky lines every + timed-out fetch leaves one behind; unchecked they accumulated to 6 GB / + hundreds of files over 9 days and eventually corrupted the pack directory + outright, permanently wedging the update check (#93732). * :func:`is_ancestor_of_head` — ask whether a remote tip is already contained in HEAD. Used by the shallow-clone update check to avoid reporting a false "update available" when local cherry-picks sit on top of the remote tip. @@ -103,6 +108,66 @@ def clear_stale_git_locks(repo_root: Path, *, min_age_seconds: Optional[int] = N return removed +# Aborted-fetch pack debris younger than this is presumed live (a fetch may +# be writing it right now) and is never removed. A healthy fetch completes in +# minutes; the same 10-minute bar the lock sweep uses is comfortably safe. +STALE_TMP_PACK_MIN_AGE_SECONDS = STALE_LOCK_MIN_AGE_SECONDS + +# Temp-file prefixes git writes into .git/objects/pack during a transfer and +# renames away on success. Anything left with these names after a fetch died +# is garbage by definition — git itself never reuses or cleans them. +_TMP_PACK_PREFIXES = ("tmp_pack_", "tmp_idx_", "tmp_rev_", "tmp_mtimes_") + + +def clear_stale_tmp_packs( + repo_root: Path, *, min_age_seconds: Optional[int] = None +) -> List[str]: + """Remove aborted-fetch temp pack files under ``.git/objects/pack``. + + Every ``git fetch`` that dies mid-transfer (timeout, HTTP 429, dropped + connection) leaves a ``tmp_pack_*`` (and sometimes ``tmp_idx_*``) file + behind, and git never cleans them up. On a flaky line the banner's + background update check produces several per day; observed in the wild + at hundreds of files / 6 GB after 9 days, after which the pack directory + corrupted outright and every fetch failed permanently (#93732). + + Same safety contract as :func:`clear_stale_git_locks`: only files older + than the age floor, never while a git process is running, never raises. + Returns the removed paths. + """ + pack_dir = Path(repo_root) / ".git" / "objects" / "pack" + if not pack_dir.is_dir(): + return [] + + if _git_proc_running(): + logger.debug("git process running; skipping tmp-pack sweep") + return [] + + cutoff = time.time() - ( + min_age_seconds if min_age_seconds is not None else STALE_TMP_PACK_MIN_AGE_SECONDS + ) + removed: List[str] = [] + try: + entries = list(pack_dir.iterdir()) + except OSError: + return [] + for entry in entries: + name = entry.name + if not name.startswith(_TMP_PACK_PREFIXES): + continue + try: + if entry.is_file() and entry.stat().st_mtime < cutoff: + size = entry.stat().st_size + entry.unlink() + removed.append(str(entry)) + logger.info( + "Removed aborted-fetch pack debris %s (%d bytes)", entry, size + ) + except OSError: + logger.debug("Could not clear %s (skipping)", entry, exc_info=True) + return removed + + def is_ancestor_of_head(repo_root: Path, rev: str) -> bool: """True when ``rev`` is an ancestor of (or equal to) HEAD. diff --git a/hermes_cli/update_cmd.py b/hermes_cli/update_cmd.py index 325d02c222..de20378514 100644 --- a/hermes_cli/update_cmd.py +++ b/hermes_cli/update_cmd.py @@ -3398,11 +3398,17 @@ def _cmd_update_check(branch: str = "main", *, branch_explicit: bool = False): # lock file) behind; every later fetch then fails with "File exists" and # the check reports a hard failure (or, in the banner path, silently # compares stale refs). Self-heal abandoned locks before fetching. - from hermes_cli.gitlock import clear_stale_git_locks + from hermes_cli.gitlock import clear_stale_git_locks, clear_stale_tmp_packs cleared = clear_stale_git_locks(_m().PROJECT_ROOT) for lock_path in cleared: print(f" (removed stale git lock: {lock_path})") + # Aborted fetches on flaky lines also strand tmp_pack_* debris in + # .git/objects/pack — unchecked it reached 6 GB and corrupted the pack + # dir outright (#93732). Same age+process safety contract as the locks. + swept = clear_stale_tmp_packs(_m().PROJECT_ROOT) + if swept: + print(f" (removed {len(swept)} aborted-fetch pack temp file(s))") # Fetch only the branch we compare against; prefer upstream as the canonical # reference. A bare `git fetch ` pulls every ref, and this repo has @@ -6439,11 +6445,14 @@ def _cmd_update_impl(args, gateway_mode: bool): # crashed fetch) before the fetch — otherwise the update fails with # "Unable to create .../shallow.lock: File exists" and never reaches # the network. - from hermes_cli.gitlock import clear_stale_git_locks + from hermes_cli.gitlock import clear_stale_git_locks, clear_stale_tmp_packs cleared = clear_stale_git_locks(_m().PROJECT_ROOT) if cleared: print(" (removed stale git lock(s): %s)" % ", ".join(cleared)) + swept = clear_stale_tmp_packs(_m().PROJECT_ROOT) + if swept: + print(" (removed %d aborted-fetch pack temp file(s))" % len(swept)) print("→ Fetching updates...") fetch_result = subprocess.run( diff --git a/tests/hermes_cli/test_gitlock_tmp_packs.py b/tests/hermes_cli/test_gitlock_tmp_packs.py new file mode 100644 index 0000000000..4f8b8194de --- /dev/null +++ b/tests/hermes_cli/test_gitlock_tmp_packs.py @@ -0,0 +1,101 @@ +"""Aborted-fetch tmp_pack debris sweep (#93732, campaign #91277). + +Every git fetch that dies mid-transfer strands a tmp_pack_* file in +.git/objects/pack; git never cleans them. clear_stale_tmp_packs() removes +them with the same age + live-git-process safety contract the lock sweep +uses. +""" + +from __future__ import annotations + +import os +import time +from pathlib import Path + +from hermes_cli.gitlock import ( + STALE_TMP_PACK_MIN_AGE_SECONDS, + clear_stale_tmp_packs, +) + + +def _mkrepo(tmp_path: Path) -> Path: + pack = tmp_path / ".git" / "objects" / "pack" + pack.mkdir(parents=True) + return tmp_path + + +def _age(path: Path, seconds: float) -> None: + stamp = time.time() - seconds + os.utime(path, (stamp, stamp)) + + +def test_removes_old_tmp_pack_debris(tmp_path, monkeypatch): + repo = _mkrepo(tmp_path) + monkeypatch.setattr("hermes_cli.gitlock._git_proc_running", lambda: False) + pack = repo / ".git" / "objects" / "pack" + + debris = [] + for name in ("tmp_pack_AbCd12", "tmp_idx_XyZ", "tmp_rev_Q1", "tmp_mtimes_M8"): + p = pack / name + p.write_bytes(b"x" * 128) + _age(p, STALE_TMP_PACK_MIN_AGE_SECONDS + 60) + debris.append(p) + + removed = clear_stale_tmp_packs(repo) + assert len(removed) == 4 + for p in debris: + assert not p.exists() + + +def test_spares_fresh_debris_and_real_packs(tmp_path, monkeypatch): + repo = _mkrepo(tmp_path) + monkeypatch.setattr("hermes_cli.gitlock._git_proc_running", lambda: False) + pack = repo / ".git" / "objects" / "pack" + + fresh = pack / "tmp_pack_fresh" # a fetch may be writing this NOW + fresh.write_bytes(b"y") + real_pack = pack / "pack-abc123.pack" # real object data — never touch + real_pack.write_bytes(b"z" * 64) + real_idx = pack / "pack-abc123.idx" + real_idx.write_bytes(b"z") + _age(real_pack, 10 * 24 * 3600) # even when ancient + _age(real_idx, 10 * 24 * 3600) + + removed = clear_stale_tmp_packs(repo) + assert removed == [] + assert fresh.exists() and real_pack.exists() and real_idx.exists() + + +def test_skips_sweep_while_git_is_running(tmp_path, monkeypatch): + repo = _mkrepo(tmp_path) + monkeypatch.setattr("hermes_cli.gitlock._git_proc_running", lambda: True) + pack = repo / ".git" / "objects" / "pack" + p = pack / "tmp_pack_old" + p.write_bytes(b"x") + _age(p, STALE_TMP_PACK_MIN_AGE_SECONDS + 60) + + assert clear_stale_tmp_packs(repo) == [] + assert p.exists() + + +def test_no_git_dir_is_a_noop(tmp_path): + assert clear_stale_tmp_packs(tmp_path) == [] + + +def test_never_raises_on_unlink_failure(tmp_path, monkeypatch): + repo = _mkrepo(tmp_path) + monkeypatch.setattr("hermes_cli.gitlock._git_proc_running", lambda: False) + pack = repo / ".git" / "objects" / "pack" + p = pack / "tmp_pack_stuck" + p.write_bytes(b"x") + _age(p, STALE_TMP_PACK_MIN_AGE_SECONDS + 60) + + real_unlink = Path.unlink + + def failing_unlink(self, *a, **k): + if self.name == "tmp_pack_stuck": + raise OSError(13, "Permission denied") + return real_unlink(self, *a, **k) + + monkeypatch.setattr(Path, "unlink", failing_unlink) + assert clear_stale_tmp_packs(repo) == [] # skipped, not raised