fix(update): sweep aborted-fetch tmp_pack debris before it corrupts the pack directory (#93732)
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.
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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.
|
||||
|
||||
|
||||
@@ -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 <remote>` 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(
|
||||
|
||||
101
tests/hermes_cli/test_gitlock_tmp_packs.py
Normal file
101
tests/hermes_cli/test_gitlock_tmp_packs.py
Normal file
@@ -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
|
||||
Reference in New Issue
Block a user