From 1256d65214adc6cb07889f2f9e06fd1d695c7aac Mon Sep 17 00:00:00 2001 From: Jim Date: Wed, 1 Jul 2026 16:57:56 -0500 Subject: [PATCH] perf(hooks): eliminate multi-second foreground stalls before the detached launch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three foreground costs ran on every commit before the detached rebuild launcher even started: 1. Interpreter probes imported the full package. Each probe executed 'import graphify' wholesale — measured 13s per probe cold on a Windows 11 dev box with AV-scanned site-packages — and up to four probes could run synchronously. Probes now use importlib.util.find_spec, which locates the package without executing it (interpreter startup cost only). A broken install under the selected interpreter still fails loudly in the rebuild log, as before. 2. The shebang probe read a binary. Git for Windows' command -v can return the launcher path WITHOUT its .exe suffix, so the '*.exe)' guard missed and head -1 read a PE binary: the shell warned 'ignored null byte in input' on every commit and the garbage always fell through to the slow python3/python fallbacks. The Windows pip layout is now resolved directly (Scripts/graphify -> sibling ../python.exe, or ./python.exe for venvs), and the remaining POSIX shebang read strips NULs first. 3. GIT_DIR was re-derived. git exports GIT_DIR to hooks; the unconditional rev-parse added ~1.3s more on machines where every git exec is scanned. Now reused from the environment with rev-parse as the manual-run fallback. Measured on the affected machine: hook foreground drops from 26s+ (cold) to ~1.4s, warnings gone. Behavior is unchanged on healthy POSIX setups — probe order and fallback semantics are preserved. Co-Authored-By: Claude Fable 5 --- graphify/hooks.py | 47 ++++++++++++++++++++++++++++++++++++--------- tests/test_hooks.py | 45 ++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 82 insertions(+), 10 deletions(-) diff --git a/graphify/hooks.py b/graphify/hooks.py index a2a3325f4..e4289ad78 100644 --- a/graphify/hooks.py +++ b/graphify/hooks.py @@ -21,9 +21,18 @@ _PYTHON_DETECT = """\ # Detect the correct Python interpreter (handles uv tool, pipx, venv, system installs). # _PINNED was recorded at hook-install time; tried first so the hook works even # when the graphify launcher is not on PATH (common in GUI clients and CI). +# +# Probes check availability with importlib.util.find_spec instead of importing +# the package: a probe that imports graphify wholesale executes the full package +# import (10s+ cold on machines with AV-scanned or large site-packages) and used +# to run up to FOUR times synchronously, stalling every commit before the +# detached launch even started. find_spec locates the package without executing +# it, so each probe costs interpreter startup only. The detached rebuild still +# fails loudly in the log if the package is broken under that interpreter. +_GFY_PROBE="import importlib.util, sys; sys.exit(0 if importlib.util.find_spec('graphify') else 1)" GRAPHIFY_PYTHON="" _PINNED='__PINNED_PYTHON__' -if [ -n "$_PINNED" ] && [ -x "$_PINNED" ] && "$_PINNED" -c "import graphify" 2>/dev/null; then +if [ -n "$_PINNED" ] && [ -x "$_PINNED" ] && "$_PINNED" -c "$_GFY_PROBE" 2>/dev/null; then GRAPHIFY_PYTHON="$_PINNED" fi # Second probe: read graphify-out/.graphify_python (written by the skill and @@ -35,18 +44,34 @@ if [ -z "$GRAPHIFY_PYTHON" ]; then case "$_FROM_FILE" in *[!a-zA-Z0-9/_.@:\\-]*) _FROM_FILE="" ;; # allowlist (covers Windows paths) esac - if [ -n "$_FROM_FILE" ] && [ -x "$_FROM_FILE" ] && "$_FROM_FILE" -c "import graphify" 2>/dev/null; then + if [ -n "$_FROM_FILE" ] && [ -x "$_FROM_FILE" ] && "$_FROM_FILE" -c "$_GFY_PROBE" 2>/dev/null; then GRAPHIFY_PYTHON="$_FROM_FILE" fi fi fi -# Third probe: resolve via the graphify launcher on PATH (shebang probe). +# Third probe: resolve via the graphify launcher on PATH. if [ -z "$GRAPHIFY_PYTHON" ]; then GRAPHIFY_BIN=$(command -v graphify 2>/dev/null) if [ -n "$GRAPHIFY_BIN" ]; then + # Windows pip layout: Scripts/graphify(.exe) sits beside ..\\python.exe + # (or .\\python.exe inside a venv's Scripts dir). NOTE: command -v may + # return the launcher path WITHOUT the .exe suffix, so this cannot key + # on the extension. + _GFY_BINDIR=$(dirname "$GRAPHIFY_BIN") + if [ -x "$_GFY_BINDIR/../python.exe" ] && "$_GFY_BINDIR/../python.exe" -c "$_GFY_PROBE" 2>/dev/null; then + GRAPHIFY_PYTHON="$_GFY_BINDIR/../python.exe" + elif [ -x "$_GFY_BINDIR/python.exe" ] && "$_GFY_BINDIR/python.exe" -c "$_GFY_PROBE" 2>/dev/null; then + GRAPHIFY_PYTHON="$_GFY_BINDIR/python.exe" + fi + fi + if [ -z "$GRAPHIFY_PYTHON" ] && [ -n "$GRAPHIFY_BIN" ]; then + # POSIX launcher: parse the shebang. head -c + tr strip NUL bytes first — + # when the launcher is a Windows binary reached without its .exe suffix, + # a raw `head -1` reads binary into the command substitution and the + # shell warns about ignored null bytes on every commit. case "$GRAPHIFY_BIN" in *.exe) _SHEBANG="" ;; - *) _SHEBANG=$(head -1 "$GRAPHIFY_BIN" | sed 's/^#![[:space:]]*//') ;; + *) _SHEBANG=$(head -c 256 "$GRAPHIFY_BIN" 2>/dev/null | tr -d '\\000' | head -n 1 | sed 's/^#![[:space:]]*//') ;; esac case "$_SHEBANG" in */env\\ *) GRAPHIFY_PYTHON="${_SHEBANG#*/env }" ;; @@ -57,16 +82,16 @@ if [ -z "$GRAPHIFY_PYTHON" ]; then case "$GRAPHIFY_PYTHON" in *[!a-zA-Z0-9/_.@-]*) GRAPHIFY_PYTHON="" ;; esac - if [ -n "$GRAPHIFY_PYTHON" ] && ! "$GRAPHIFY_PYTHON" -c "import graphify" 2>/dev/null; then + if [ -n "$GRAPHIFY_PYTHON" ] && ! "$GRAPHIFY_PYTHON" -c "$_GFY_PROBE" 2>/dev/null; then GRAPHIFY_PYTHON="" fi fi fi # Last resort: try python3 / python (works for system/venv installs on PATH). if [ -z "$GRAPHIFY_PYTHON" ]; then - if command -v python3 >/dev/null 2>&1 && python3 -c "import graphify" 2>/dev/null; then + if command -v python3 >/dev/null 2>&1 && python3 -c "$_GFY_PROBE" 2>/dev/null; then GRAPHIFY_PYTHON="python3" - elif command -v python >/dev/null 2>&1 && python -c "import graphify" 2>/dev/null; then + elif command -v python >/dev/null 2>&1 && python -c "$_GFY_PROBE" 2>/dev/null; then GRAPHIFY_PYTHON="python" else echo "[graphify hook] could not locate a Python with graphify installed. Add the graphify bin dir to PATH or re-run 'graphify hook install' from the env where graphify lives." >&2 @@ -233,7 +258,9 @@ if [ -n "${WINDIR:-}" ] || [ -n "${MSYSTEM:-}" ]; then fi # Skip during rebase/merge/cherry-pick to avoid blocking --continue with unstaged changes -GIT_DIR=$(git rev-parse --git-dir 2>/dev/null) +# git exports GIT_DIR to hooks; the rev-parse fallback only runs when invoked by +# hand (each git exec costs 1s+ on AV-scanned Windows machines). +GIT_DIR=${GIT_DIR:-$(git rev-parse --git-dir 2>/dev/null)} [ -d "$GIT_DIR/rebase-merge" ] && exit 0 [ -d "$GIT_DIR/rebase-apply" ] && exit 0 [ -f "$GIT_DIR/MERGE_HEAD" ] && exit 0 @@ -299,7 +326,9 @@ if [ ! -d "graphify-out" ]; then fi # Skip during rebase/merge/cherry-pick -GIT_DIR=$(git rev-parse --git-dir 2>/dev/null) +# git exports GIT_DIR to hooks; the rev-parse fallback only runs when invoked by +# hand (each git exec costs 1s+ on AV-scanned Windows machines). +GIT_DIR=${GIT_DIR:-$(git rev-parse --git-dir 2>/dev/null)} [ -d "$GIT_DIR/rebase-merge" ] && exit 0 [ -d "$GIT_DIR/rebase-apply" ] && exit 0 [ -f "$GIT_DIR/MERGE_HEAD" ] && exit 0 diff --git a/tests/test_hooks.py b/tests/test_hooks.py index 49f2c1da3..6a37bd932 100644 --- a/tests/test_hooks.py +++ b/tests/test_hooks.py @@ -268,7 +268,7 @@ def _launcher_payload(script: str) -> str: The launcher is the only `-c` invocation whose body begins with `import os, subprocess, sys` (the interpreter-detection probes in - _PYTHON_DETECT use `-c "import graphify"`).""" + _PYTHON_DETECT use `-c "$_GFY_PROBE"`).""" m = re.search(r'-c "(import os, subprocess, sys.*?)"\n', script, re.DOTALL) assert m, "launcher payload not found" return m.group(1) @@ -387,3 +387,46 @@ def test_default_hooks_dir_unaffected(tmp_path): repo = _make_git_repo(tmp_path) install(repo) assert (repo / ".git" / "hooks" / "post-commit").exists() + + +# ── foreground hook cost: probes must be cheap and quiet ───────────────────── + +def test_probes_use_find_spec_not_full_import(): + """`python -c "import graphify"` executes the FULL package import — 10s+ on a + cold cache or AV-scanned site-packages — and could run up to four times + synchronously before the detached launch even started, so every commit + stalled for tens of seconds. Probes must locate the package with + importlib.util.find_spec (no execution); the detached rebuild still reports + a broken install loudly in its log.""" + from graphify.hooks import _PYTHON_DETECT + assert '-c "import graphify"' not in _PYTHON_DETECT, ( + "interpreter probe still imports the full package in the hook foreground" + ) + assert "find_spec" in _PYTHON_DETECT + + +def test_shebang_read_is_null_byte_safe(): + """On Windows, `command -v graphify` can return the launcher path WITHOUT its + .exe suffix, so the `*.exe)` guard misses and the shebang probe reads a + BINARY: the shell then warns 'ignored null byte in input' on every commit and + the extracted garbage always falls through to the slow fallbacks. The read + must strip NULs before the command substitution sees them.""" + from graphify.hooks import _PYTHON_DETECT + assert "tr -d '\\000'" in _PYTHON_DETECT, "shebang read is not NUL-safe" + + +def test_probe_prefers_sibling_python_exe_on_windows_layouts(): + """pip on Windows puts Scripts/graphify(.exe) beside ..\\python.exe (or + .\\python.exe in a venv). Resolving that directly beats shebang-parsing a + binary launcher — and works whether or not command -v kept the suffix.""" + from graphify.hooks import _PYTHON_DETECT + assert "/../python.exe" in _PYTHON_DETECT + assert "/python.exe" in _PYTHON_DETECT + + +@pytest.mark.parametrize("name,script", _HOOK_SCRIPTS) +def test_hooks_reuse_git_dir_from_env(name, script): + """git exports GIT_DIR to hooks, so the rev-parse fallback should only run + when the script is invoked by hand — each extra git exec costs 1s+ on + AV-scanned Windows machines and lands in the commit's foreground.""" + assert "GIT_DIR=${GIT_DIR:-" in script, f"{name} always re-runs git rev-parse"