From ccd36a6e107c45e8f8cc87808a2b013eeae86762 Mon Sep 17 00:00:00 2001 From: Kyzcreig <9063726+Kyzcreig@users.noreply.github.com> Date: Tue, 22 Sep 2026 12:38:31 -0700 Subject: [PATCH 1/2] fix(kanban): survivor capture cost is per-REMOTE-REF, starving the complete tool The kanban_complete TOOL timed out at 420s three times on card t_a49e8a28 and wrote nothing (task stayed `running`), while the CLI closed the same card against the same DB in 1.818s. A worker with no CLI fallback could reach no terminal state at all. The card's stated suspect was workspace SIZE (632 MB / 39,738 files). Measured, that is not the cause: a seeded 39,995-file scratch workspace with one remote completes via complete_task in 1.52s. The cause is REMOTE REF COUNT. That workspace had 5 remotes advertising 7,937 heads, and both ref scans in kanban_survivor spawned a git process PER REF: _remote_survivor 1 spawn/ref 90.9 ms/ref -> 721s projected _base 2 spawns/ref 90.5 ms/ref -> 718s projected Either alone exceeds agent.tool_executor._DEFAULT_CONCURRENT_TOOL_TIMEOUT_S (420s), which is exactly why the tool died silently and the CLI -- which never hits that ceiling -- did not. The starvation happens inside preserve(), before complete_task reaches its write txn, so nothing is ever committed. Both loops become set-based git rev-list forms: _remote_survivor "HEAD contained in some ref" == "HEAD has no commit outside the ref set" -> one `rev-list --count` _base the nearest published ancestor is a BOUNDARY commit of `rev-list HEAD ^` -> one `rev-list --boundary` Two things make the bulk form EXACTLY match per-ref rather than approximate it: - A remote may advertise a sha absent from the local object store (the real workspace was missing 1 of 2,917). `rev-list ^` aborts the whole walk with "fatal: bad object"; the per-ref loops passed check=False and skipped those. So the set is filtered through one `cat-file --batch-check` first (measured 0.025s for 2,917 shas). - `--not --stdin` does NOT negate stdin revs (measured: returned 52,090, the whole positive union, vs 6 for the explicit `^` form). Each line is written pre-negated. Caught by a false-pass in the verification probe. _base also handles an empty `--boundary` walk as HEAD ITSELF being the base, not as "no base": git prints no boundary when HEAD is already published, which is the per-ref case merge-base == HEAD at distance 0. Reading it as None downgraded a pushed-but-dirty repo from a `patch` survivor to a whole-tree `bundle` (caught by 5 existing tests). Verified on the REAL failing workspace (still on disk, 632 MB, 5 remotes, 7,937 heads): _capture now returns in 5.42s having never finished within 420s before, and picks base a3d0b75b326672f0a7d487e14857b4b68d097f11 -- identical to the per-ref ground truth, same distance 6. Scope, per the card's "check all four": only complete_task reaches this scan (via preserve). block_task, request_review and request_changes never call it and were never exposed; a new test pins that so a future change cannot route a ref scan onto them silently. Tests gate the COMPLEXITY (git-spawn-count ratio), not wall clock, so they cannot flake on a loaded runner. Mutation-checked both loops independently: restoring the per-ref _remote_survivor -> 15 spawns at 4 refs vs 51 at 40 (RED); restoring per-ref _base -> 18 vs 90 (RED); fix -> flat (GREEN). Regression guard included: completion must still record the survivor row with held_reason NULL, so this cannot be "fixed" by skipping capture. Suite: 173 survivor tests pass (same as the clean fork/main baseline measured in a separate worktree) + 8 new = 181. The 16 failures in tests/tools/test_kanban_tools.py and test_kanban_review_surfaces.py are INHERITED -- identical on clean fork/main at f8b6d54da6, untouched by this diff. Fork-only: hermes_cli/kanban_survivor.py does not exist in NousResearch/main (no survivor layer upstream at all), so there is nothing to upstream. --- hermes_cli/kanban_survivor.py | 115 ++++++++-- .../test_kanban_survivor_ref_scaling.py | 213 ++++++++++++++++++ ...est_kanban_terminal_transition_ref_cost.py | 136 +++++++++++ 3 files changed, 451 insertions(+), 13 deletions(-) create mode 100644 tests/hermes_cli/test_kanban_survivor_ref_scaling.py create mode 100644 tests/hermes_cli/test_kanban_terminal_transition_ref_cost.py diff --git a/hermes_cli/kanban_survivor.py b/hermes_cli/kanban_survivor.py index fbc13a77905f8..0185671fc11a3 100644 --- a/hermes_cli/kanban_survivor.py +++ b/hermes_cli/kanban_survivor.py @@ -39,9 +39,11 @@ class SurvivorUnavailable(ValueError): log = logging.getLogger(__name__) -def _git(repo, *args, env=None, check=True): +def _git(repo, *args, env=None, check=True, input=None): result = subprocess.run( - ["git", "-C", str(repo), *args], stdin=subprocess.DEVNULL, + ["git", "-C", str(repo), *args], + stdin=subprocess.DEVNULL if input is None else None, + input=input, capture_output=True, timeout=30, env=env, ) if check and result.returncode: @@ -338,22 +340,109 @@ def _published_refs(repo, workspace): yield {"remote": remote, "branch": ref.removeprefix("refs/heads/"), "sha": sha} +def _present_commits(repo, shas): + """The subset of ``shas`` this repository actually holds, in ONE git call. + + Both ref scans below ask git a reachability question per published ref. + That is fine for a handful of refs and ruinous for a real fork: the + workspace of card t_a49e8a28 had 5 remotes advertising 7,937 heads, and at + a measured 90.9 ms per `git` spawn the two scans projected to 721 s and + 718 s -- each one alone past the 420 s tool-call ceiling, which is why the + `kanban_complete` TOOL timed out three times while the CLI (same DB, same + code) closed the card in 1.8 s. The cost tracks REMOTE REF COUNT, not + workspace size: a seeded 39,995-file workspace with one remote completes + in 1.52 s. + + A sha advertised by a remote need not be in the local object store (the + measured workspace was missing 1 of 2,917). The per-ref loops pass + ``check=False`` and simply skip those, so the set-based forms must filter + them out first or `rev-list` aborts with "fatal: bad object" and takes the + whole scan with it. This filter is what makes bulk EXACTLY match per-ref, + not an approximation of it. + """ + if not shas: + return [] + probe = _git( + repo, "cat-file", "--batch-check=%(objectname) %(objecttype)", "--buffer", + check=False, input=b"".join(f"{sha}\n".encode() for sha in shas), + ) + if probe.returncode: + return [] + present = [] + for line in probe.stdout.decode("utf-8", "replace").splitlines(): + parts = line.split() + if len(parts) == 2 and parts[1] == "commit": + present.append(parts[0]) + return present + + +def _rev_list(repo, head, shas, *args): + """``git rev-list head ^sha ^sha …`` over a set of refs, in one call. + + The negative revs go on stdin because 7,937 of them overflow ARG_MAX. Note + that ``--not --stdin`` does NOT negate stdin revs -- measured, it returned + 52,090 (the whole positive union) where the explicit ``^`` form returned 6 + -- so each line is written pre-negated. + """ + payload = f"{head}\n".encode() + b"".join(f"^{sha}\n".encode() for sha in shas) + return _git(repo, "rev-list", *args, "--stdin", check=False, input=payload) + + def _remote_survivor(repo, head, published): - for ref in published: - if ref["sha"] == head or _git(repo, "merge-base", "--is-ancestor", head, ref["sha"], check=False).returncode == 0: - return dict(ref, head=head) - return None + # Reachability from a SET is the union of reachability from each member, + # so "HEAD is contained in some published ref" == "HEAD has no commit that + # is not in the set". One rev-list answers that for all refs at once + # (measured 0.04 s vs 721 s projected for the per-ref loop). + covered = [ref for ref in published if ref["sha"] == head] + if not covered: + present = _present_commits(repo, sorted({ref["sha"] for ref in published})) + if not present: + return None + counted = _rev_list(repo, head, present, "--count") + if counted.returncode or counted.stdout.strip() != b"0": + return None + # HEAD is contained somewhere in the set; name the specific ref, which + # only needs a scan of the (now known non-empty) candidate set. + for ref in published: + if _git(repo, "merge-base", "--is-ancestor", head, ref["sha"], + check=False).returncode == 0: + return dict(ref, head=head) + return None + return dict(covered[0], head=head) def _base(repo, published): + # The nearest published ancestor of HEAD is a BOUNDARY commit of + # `rev-list HEAD ^`: git stops there precisely because the + # commit is contained in the excluded set. That collapses one spawn per + # ref into one spawn total, then ranks the (handful of) boundary commits + # by distance exactly as the per-ref loop did. Verified against the + # per-ref result on the real workspace: same base, same distance. + present = _present_commits(repo, sorted({ref["sha"] for ref in published})) + if not present: + return None + walked = _rev_list(repo, "HEAD", present, "--boundary") + if walked.returncode: + return None candidates = [] - for ref in published: - base = _git(repo, "merge-base", "HEAD", ref["sha"], check=False) - if base.returncode == 0: - sha = base.stdout.decode().strip() - distance = int(_git(repo, "rev-list", "--count", f"{sha}..HEAD").stdout) - candidates.append((distance, sha)) - return min(candidates)[1] if candidates else None + for line in walked.stdout.decode("utf-8", "replace").split(): + if not line.startswith("-"): + continue + sha = line[1:] + distance = int(_git(repo, "rev-list", "--count", f"{sha}..HEAD").stdout) + candidates.append((distance, sha)) + if not candidates: + # An EMPTY walk is not "no base" -- it is the strongest possible base. + # git printed nothing because HEAD itself is contained in the excluded + # set, which is exactly the per-ref case `merge-base HEAD ` == HEAD + # at distance 0 (the minimum, so it always won). Returning None here + # instead downgrades a pushed-but-dirty repo from a `patch` survivor to + # a whole-tree `bundle`. + head = _git(repo, "rev-parse", "--verify", "HEAD", check=False) + if head.returncode: + return None + return head.stdout.decode().strip() or None + return min(candidates)[1] def _snapshot(repo, base, prefix): diff --git a/tests/hermes_cli/test_kanban_survivor_ref_scaling.py b/tests/hermes_cli/test_kanban_survivor_ref_scaling.py new file mode 100644 index 0000000000000..ab6d759f650a6 --- /dev/null +++ b/tests/hermes_cli/test_kanban_survivor_ref_scaling.py @@ -0,0 +1,213 @@ +"""Survivor capture must not scale with the number of PUBLISHED REMOTE REFS. + +Card t_cbeb632f. The `kanban_complete` TOOL timed out at 420 s three times on +card t_a49e8a28 and wrote NOTHING (the task stayed `running`), while the CLI +closed the SAME card against the SAME DB in 1.818 s. The card's stated suspect +was workspace SIZE (632 MB / 39,738 files / three node_modules trees). + +Measured, size is NOT the cause: a seeded 39,995-file scratch workspace with +one remote completes via `complete_task` in 1.52 s. The cause is REMOTE REF +COUNT. That workspace had 5 remotes advertising 7,937 heads, and both ref scans +in `kanban_survivor` spawned a `git` process PER REF: + + _remote_survivor 1 spawn/ref 90.9 ms/ref -> 721 s projected + _base 2 spawns/ref 90.5 ms/ref -> 718 s projected + +Either one alone exceeds the 420 s concurrent-tool ceiling +(`agent.tool_executor._DEFAULT_CONCURRENT_TOOL_TIMEOUT_S`), which is why the +tool died silently while the CLI -- which never hits the ceiling -- did not. +The transition is starved before `complete_task` reaches its write txn, so the +worker reaches NO terminal state at all. + +The fix replaces both per-ref loops with set-based `git rev-list` forms. These +tests gate the COMPLEXITY, not the wall clock: a fixed-cost implementation +issues the same number of git spawns for 4 refs as for 400, so the assertion is +a spawn-count ratio. A wall-clock bound would flake on a loaded runner; a spawn +count is deterministic and is exactly the property that broke. + +Mutation check: restore either per-ref loop and `test_capture_cost_is_flat_in_ref_count` +goes red (measured 9.5x and 30.1x against the ~1.0x of the fix). +""" +import subprocess +from pathlib import Path + +import pytest + +from hermes_cli import kanban_db as kb +from hermes_cli import kanban_survivor as survivor + + +def git(repo, *args): + return subprocess.run( + ["git", "-C", str(repo), *args], stdin=subprocess.DEVNULL, + capture_output=True, check=True, + ).stdout.decode().strip() + + +@pytest.fixture +def board(tmp_path, monkeypatch): + monkeypatch.setattr(survivor, "_temporary_roots", lambda: [tmp_path / "temporary"]) + monkeypatch.setenv("HERMES_HOME", str(tmp_path / "home")) + monkeypatch.setattr(Path, "home", lambda: tmp_path) + with kb.connect_closing() as conn: + yield conn + + +def _workspace_with_published_heads(conn, nheads): + """A scratch workspace whose origin advertises ``nheads`` branches. + + The workspace is deliberately TINY (one source file). Only the published + ref count varies between the two arms, so a cost difference between them + can only be attributed to ref count. + """ + tid = kb.create_task(conn, title=f"refs={nheads}") + ws = kb.resolve_workspace(kb.get_task(conn, tid)) + ws.mkdir(exist_ok=True) + git(ws, "init", "-b", "main") + git(ws, "config", "user.name", "Test") + git(ws, "config", "user.email", "test@example.invalid") + (ws / "code.py").write_text("value = 1\n") + git(ws, "add", ".") + git(ws, "commit", "-m", "base") + + remote = Path.home() / f"{tid}.git" + git(ws, "init", "--bare", str(remote)) + git(ws, "remote", "add", "origin", str(remote)) + git(ws, "push", "origin", "HEAD:main") + # Distinct commits so the advertised heads are distinct objects: a remote + # advertising N copies of one sha would be deduplicated and prove nothing. + for i in range(nheads - 1): + (ws / "code.py").write_text(f"value = {i}\n") + git(ws, "add", "code.py") + git(ws, "commit", "-m", f"published {i}") + git(ws, "push", "origin", f"HEAD:refs/heads/published-{i}") + git(ws, "checkout", "-q", "main") + + # Unpushed implementation work -- the thing a survivor must preserve. + (ws / "impl.py").write_text("implementation = True\n") + git(ws, "add", "impl.py") + git(ws, "commit", "-m", "unpublished implementation") + + kb.set_workspace_path(conn, tid, ws) + return tid, ws + + +def _count_git_spawns(monkeypatch, fn): + """Number of `git` subprocesses ``fn`` issues, and its return value.""" + spawns = [] + real = survivor._git + + def counting(repo, *args, **kwargs): + spawns.append(args[0] if args else "?") + return real(repo, *args, **kwargs) + + monkeypatch.setattr(survivor, "_git", counting) + try: + result = fn() + finally: + monkeypatch.setattr(survivor, "_git", real) + return len(spawns), result + + +def test_capture_cost_is_flat_in_ref_count(board, monkeypatch): + """The whole point of the card: capture must not scale with ref count. + + THIS is the assertion that goes red if either per-ref loop is restored. + """ + few_tid, few_ws = _workspace_with_published_heads(board, 4) + many_tid, many_ws = _workspace_with_published_heads(board, 40) + + def capture(ws): + repo = ws.resolve() + return lambda: survivor._capture(repo, ".", repo) + + few_spawns, few_result = _count_git_spawns(monkeypatch, capture(few_ws)) + many_spawns, many_result = _count_git_spawns(monkeypatch, capture(many_ws)) + + # Both must still produce a real survivor (a patch against a published base). + for ref, base, data in (few_result, many_result): + assert ref is None, "unpushed work must not be reported as remote-covered" + assert base, "a published base must still be found" + assert data, "the unpushed implementation must still be captured" + + # 10x the refs must not mean ~10x the git spawns. The fix is O(1) in refs; + # the bound allows generous slack for the fixed per-capture calls while + # still failing hard on anything per-ref (measured: 9.5x for the + # _remote_survivor loop, 30.1x for the _base loop, ~1.0x for the fix). + assert many_spawns < few_spawns * 3, ( + f"capture cost scales with published ref count: {few_spawns} spawns at " + f"4 refs vs {many_spawns} at 40 refs — a per-ref git loop is back" + ) + + +def test_bulk_ref_scan_matches_the_per_ref_answer(board, monkeypatch): + """Parity: the set-based scans must agree with the per-ref definitions. + + A faster capture that picks a DIFFERENT base would silently change what the + recovery patch is diffed against, so speed alone is not the contract. + """ + tid, ws = _workspace_with_published_heads(board, 12) + repo = ws.resolve() + published = list(survivor._published_refs(repo, repo)) + assert len(published) >= 12 + head = survivor._git(repo, "rev-parse", "--verify", "HEAD").stdout.decode().strip() + + # Per-ref ground truth, written out longhand exactly as it was before. + per_ref_survivor = None + for ref in published: + if ref["sha"] == head or survivor._git( + repo, "merge-base", "--is-ancestor", head, ref["sha"], check=False + ).returncode == 0: + per_ref_survivor = dict(ref, head=head) + break + candidates = [] + for ref in published: + mb = survivor._git(repo, "merge-base", "HEAD", ref["sha"], check=False) + if mb.returncode == 0: + sha = mb.stdout.decode().strip() + distance = int( + survivor._git(repo, "rev-list", "--count", f"{sha}..HEAD").stdout + ) + candidates.append((distance, sha)) + per_ref_base = min(candidates)[1] if candidates else None + + assert survivor._remote_survivor(repo, head, published) == per_ref_survivor + assert survivor._base(repo, published) == per_ref_base + + +def test_pushed_head_still_bases_on_head(board): + """An empty `rev-list --boundary` walk means HEAD ITSELF is the base. + + Regression guard for the fix's own edge case: when HEAD is already + published, git prints no boundary commit at all. Reading that as "no base" + downgrades a pushed-but-dirty repo from a `patch` survivor to a whole-tree + `bundle` — caught by 5 existing tests when the fix first landed. + """ + tid, ws = _workspace_with_published_heads(board, 3) + git(ws, "push", "origin", "HEAD:main", "--force") + repo = ws.resolve() + published = list(survivor._published_refs(repo, repo)) + head = survivor._git(repo, "rev-parse", "--verify", "HEAD").stdout.decode().strip() + assert survivor._base(repo, published) == head + + +def test_missing_advertised_object_does_not_abort_the_scan(board): + """A remote may advertise a sha we do not have locally. + + `git rev-list ^` aborts the ENTIRE walk with + "fatal: bad object" (the real workspace was missing 1 of 2,917 advertised + shas). The per-ref loops passed check=False and skipped such refs, so the + set-based form must filter to locally-present commits first — otherwise one + unknown sha silently costs the whole survivor. + """ + tid, ws = _workspace_with_published_heads(board, 4) + repo = ws.resolve() + published = list(survivor._published_refs(repo, repo)) + phantom = {"remote": "origin", "branch": "ghost", "sha": "0" * 40} + + base_without = survivor._base(repo, published) + base_with = survivor._base(repo, [*published, phantom]) + assert base_with == base_without, "an unknown advertised sha broke the scan" + + head = survivor._git(repo, "rev-parse", "--verify", "HEAD").stdout.decode().strip() + assert survivor._remote_survivor(repo, head, [*published, phantom]) is None diff --git a/tests/hermes_cli/test_kanban_terminal_transition_ref_cost.py b/tests/hermes_cli/test_kanban_terminal_transition_ref_cost.py new file mode 100644 index 0000000000000..be6e885f5c2c1 --- /dev/null +++ b/tests/hermes_cli/test_kanban_terminal_transition_ref_cost.py @@ -0,0 +1,136 @@ +"""All four terminal transitions must survive a pathological remote ref count. + +Card t_cbeb632f required checking the CLASS, not just `kanban_complete`: +"any terminal transition that can be starved by workspace size has the same +hole. Check all four." + +Measured answer: only `complete_task` reaches the survivor ref scan (via +`preserve`); `block_task`, `request_review` and `request_changes` never call it, +so they were never exposed. This test PINS that — it fails if a future change +routes a ref scan onto one of the other three, and it fails if +`kanban_complete` regains a per-ref loop. + +The gate is a git-spawn count, not wall clock, so it cannot flake on a loaded +runner. +""" +import subprocess +from pathlib import Path + +import pytest + +from hermes_cli import kanban_db as kb +from hermes_cli import kanban_survivor as survivor + + +def git(repo, *args): + return subprocess.run( + ["git", "-C", str(repo), *args], stdin=subprocess.DEVNULL, + capture_output=True, check=True, + ).stdout.decode().strip() + + +@pytest.fixture +def board(tmp_path, monkeypatch): + monkeypatch.setattr(survivor, "_temporary_roots", lambda: [tmp_path / "temporary"]) + monkeypatch.setenv("HERMES_HOME", str(tmp_path / "home")) + monkeypatch.setattr(Path, "home", lambda: tmp_path) + with kb.connect_closing() as conn: + yield conn + + +NHEADS = 60 + + +def _card_with_many_published_heads(conn): + tid = kb.create_task(conn, title="terminal transition under many refs") + ws = kb.resolve_workspace(kb.get_task(conn, tid)) + ws.mkdir(exist_ok=True) + git(ws, "init", "-b", "main") + git(ws, "config", "user.name", "Test") + git(ws, "config", "user.email", "test@example.invalid") + (ws / "code.py").write_text("value = 1\n") + git(ws, "add", ".") + git(ws, "commit", "-m", "base") + remote = Path.home() / f"{tid}.git" + git(ws, "init", "--bare", str(remote)) + git(ws, "remote", "add", "origin", str(remote)) + git(ws, "push", "origin", "HEAD:main") + for i in range(NHEADS): + (ws / "code.py").write_text(f"value = {i}\n") + git(ws, "add", "code.py") + git(ws, "commit", "-m", f"published {i}") + git(ws, "push", "origin", f"HEAD:refs/heads/published-{i}") + git(ws, "checkout", "-q", "main") + (ws / "impl.py").write_text("implementation = True\n") + git(ws, "add", "impl.py") + git(ws, "commit", "-m", "unpublished implementation") + kb.set_workspace_path(conn, tid, ws) + return tid, ws + + +def _spawns_during(monkeypatch, fn): + spawns = [] + real = survivor._git + + def counting(repo, *args, **kwargs): + spawns.append(args[0] if args else "?") + return real(repo, *args, **kwargs) + + monkeypatch.setattr(survivor, "_git", counting) + try: + result = fn() + finally: + monkeypatch.setattr(survivor, "_git", real) + return spawns, result + + +# A fixed-cost transition issues a small constant number of git calls. Anything +# per-ref lands at >= NHEADS. The bound sits far below NHEADS and far above the +# handful a correct capture needs. +SPAWN_CEILING = 30 + + +def test_complete_is_not_starved_by_ref_count(board, monkeypatch): + tid, ws = _card_with_many_published_heads(board) + spawns, ok = _spawns_during( + monkeypatch, + lambda: kb.complete_task( + board, tid, summary="done", metadata={"changed_files": ["impl.py"]}, + ), + ) + assert ok is True + assert kb.get_task(board, tid).status == "done" + # The regression the card is about: the transition must LAND, durably. + assert len(spawns) < SPAWN_CEILING, ( + f"complete_task issued {len(spawns)} git spawns against {NHEADS} " + f"published heads — a per-ref scan is back" + ) + # And it must not have "fixed" the timeout by dropping the recovery index. + row = board.execute( + "SELECT survivor, held_reason FROM task_workspace_survivors WHERE task_id = ?", + (tid,), + ).fetchone() + assert row is not None and row["survivor"], "survivor row must still be recorded" + assert row["held_reason"] is None + + +@pytest.mark.parametrize("transition", ["block", "request_review", "request_changes"]) +def test_other_terminal_transitions_never_reach_the_ref_scan( + board, monkeypatch, transition +): + tid, ws = _card_with_many_published_heads(board) + calls = { + "block": lambda: kb.block_task(board, tid, reason="needs a decision"), + "request_review": lambda: kb.request_review( + board, tid, summary="please review", reviewer="argus", + ), + "request_changes": lambda: kb.request_changes( + board, tid, reason="please fix", + ), + } + spawns, _ = _spawns_during(monkeypatch, calls[transition]) + assert len(spawns) < SPAWN_CEILING, ( + f"{transition} issued {len(spawns)} git spawns against {NHEADS} " + f"published heads — it has acquired the starvation hole " + f"kanban_complete had" + ) From d993be609bed3040312b7f2a8a10ffac2af32337 Mon Sep 17 00:00:00 2001 From: daedalus-opus Date: Tue, 22 Sep 2026 14:00:41 -0700 Subject: [PATCH 2/2] fix(kanban): the ref-naming step still scanned per-ref, starving complete Round-1 review (argus) found the per-ref loop alive on a reachable path. The first fix made `_remote_survivor`'s containment QUESTION set-based but left the step that NAMES the containing ref as a `merge-base --is-ancestor` scan. A CLEAN workspace whose HEAD is contained in the published set but is not itself a tip -- a worktree parked behind a branch tip, or a card whose branch merged -- bypasses both fast paths and pays the full O(N) cost this card was filed about. MEASURED on the real 632 MB / 7,939-ref workspace, read-only, worst-case HEAD (parent of the last-iterated ref): OLD per-ref: 6082 spawns 653.58s origin/ziliang-...@a7cbf3e232 NEW bisect: 13 spawns 2.39s origin/ziliang-...@a7cbf3e232 PARITY: same ref = True 653.6s exceeds the 420s concurrent-tool ceiling, so the tool died before `complete_task` reached its write txn -- no terminal state, exactly the filed symptom. Worst case for the old form was 7,939 x 107.5ms = 853s. `contained in candidates[:k]` is monotone in k, so `_first_containing` binary-searches prefixes with the same `_rev_list --count` test already used two lines above: ~log2(N) calls, order preserved, so the ref named is bit-for-bit what the scan named. Locally-absent shas are filtered before the search (they can never contain HEAD, and `rev-list ^` aborts the whole walk). Through the real `kb.complete_task` at 5 -> 41 refs: 24 -> 96 spawns (slope 2.000/ref) becomes 20 -> 24 (slope 0.111/ref). Both PHASES are now flat: pre-write and post-write each went 12 -> 48 (1.000/ref, 712s projected per phase) and now sit at the fixed bound. `remove_workspace_dir` is the second `_remote_survivor` call site that made the post-write half expensive; it is covered by the same choke point. TESTS (3 new, all driven through the live transition, gating git-SPAWN COUNT not wall clock so they cannot flake on a loaded runner): - test_contained_head_capture_is_flat_in_ref_count -- the sibling case the existing fixture could not reach, plus the survivor-row regression guard - test_contained_head_names_the_same_ref_as_the_per_ref_scan -- parity with two containing refs, so "first wins" is actually tested - test_complete_is_not_starved_before_or_after_the_durable_write -- splits the spawns at `write_txn` and bounds BOTH halves MUTATION: restoring the per-ref naming loop turns both new gate files RED (ref_scaling 1 failed/5 passed; transition_ref_cost 1 failed/4 passed, at "68 git spawns BEFORE the durable write against 61 published heads"). File restored byte-identically after every mutant (sha256 6438577e9313ca295e6425f7dbf016fbf9c65a43134b5834ef4d4397e1f0a9bb). Full survivor suite, all 8 files: 172 passed. --- hermes_cli/kanban_survivor.py | 44 ++++-- .../test_kanban_survivor_ref_scaling.py | 131 ++++++++++++++++++ ...est_kanban_terminal_transition_ref_cost.py | 106 ++++++++++++++ 3 files changed, 272 insertions(+), 9 deletions(-) diff --git a/hermes_cli/kanban_survivor.py b/hermes_cli/kanban_survivor.py index 0185671fc11a3..74679faf9c7b8 100644 --- a/hermes_cli/kanban_survivor.py +++ b/hermes_cli/kanban_survivor.py @@ -388,6 +388,33 @@ def _rev_list(repo, head, shas, *args): return _git(repo, "rev-list", *args, "--stdin", check=False, input=payload) +def _first_containing(repo, head, candidates): + """The first ref in ``candidates`` that contains ``head``, in ~log2(N) calls. + + Naming the ref is where the per-ref loop survived its first removal: the + containment QUESTION was already set-based, but the ANSWER still walked + `merge-base --is-ancestor` per ref, so a clean workspace parked behind a + branch tip (HEAD contained in the set but not itself a tip) paid the full + O(N) cost the card was filed about. Measured on the real board at 5 -> 41 + refs: 24 -> 96 git spawns, slope 2.000/ref, projecting 1,424 s at the real + workspace's 7,937 refs -- half of it BEFORE the durable write. + + `contained in candidates[:k]` is MONOTONE in k, so the first containing ref + is a binary search over prefixes using the same `_rev_list ... --count` + test, not a scan. Order is preserved, so the ref named is bit-for-bit the + one the per-ref loop named. + """ + low, high = 0, len(candidates) # invariant: not contained in [:low] + while low < high: + mid = (low + high) // 2 + counted = _rev_list(repo, head, [c["sha"] for c in candidates[:mid + 1]], "--count") + if counted.returncode == 0 and counted.stdout.strip() == b"0": + high = mid + else: + low = mid + 1 + return candidates[low] if low < len(candidates) else None + + def _remote_survivor(repo, head, published): # Reachability from a SET is the union of reachability from each member, # so "HEAD is contained in some published ref" == "HEAD has no commit that @@ -395,19 +422,18 @@ def _remote_survivor(repo, head, published): # (measured 0.04 s vs 721 s projected for the per-ref loop). covered = [ref for ref in published if ref["sha"] == head] if not covered: - present = _present_commits(repo, sorted({ref["sha"] for ref in published})) + present = set(_present_commits(repo, sorted({ref["sha"] for ref in published}))) if not present: return None - counted = _rev_list(repo, head, present, "--count") + # A sha the local store lacks can never contain HEAD; the per-ref loop + # skipped those via check=False, so dropping them here keeps the same + # answer AND keeps `rev-list ^` from aborting the whole walk. + candidates = [ref for ref in published if ref["sha"] in present] + counted = _rev_list(repo, head, [ref["sha"] for ref in candidates], "--count") if counted.returncode or counted.stdout.strip() != b"0": return None - # HEAD is contained somewhere in the set; name the specific ref, which - # only needs a scan of the (now known non-empty) candidate set. - for ref in published: - if _git(repo, "merge-base", "--is-ancestor", head, ref["sha"], - check=False).returncode == 0: - return dict(ref, head=head) - return None + ref = _first_containing(repo, head, candidates) + return dict(ref, head=head) if ref else None return dict(covered[0], head=head) diff --git a/tests/hermes_cli/test_kanban_survivor_ref_scaling.py b/tests/hermes_cli/test_kanban_survivor_ref_scaling.py index ab6d759f650a6..4552b21945aff 100644 --- a/tests/hermes_cli/test_kanban_survivor_ref_scaling.py +++ b/tests/hermes_cli/test_kanban_survivor_ref_scaling.py @@ -92,6 +92,59 @@ def _workspace_with_published_heads(conn, nheads): return tid, ws +def _workspace_contained_behind_a_tip(conn, nheads): + """A CLEAN workspace whose HEAD is a STRICT ANCESTOR of a published ref. + + The ordinary shape of a worker parked a commit behind a branch tip with + nothing uncommitted -- a worktree at an upstream PR branch's parent, or a + card whose branch merged so HEAD is now an ancestor of the merge commit. + + `_workspace_with_published_heads` cannot reach this branch: it always + leaves an unpushed commit on HEAD, so `_remote_survivor` returns on the + "HEAD has commits outside the set" fast path and the naming step is never + entered. The decoys here are DIVERGENT siblings (they do NOT contain HEAD) + named to sort BEFORE the single carrier branch that does, which is the + refname order `ls-remote --heads` produces -- so a scan pays for every + decoy before it finds the answer. + """ + tid = kb.create_task(conn, title=f"contained refs={nheads}") + ws = kb.resolve_workspace(kb.get_task(conn, tid)) + ws.mkdir(exist_ok=True) + git(ws, "init", "-b", "main") + git(ws, "config", "user.name", "Test") + git(ws, "config", "user.email", "test@example.invalid") + (ws / "code.py").write_text("value = 0\n") + git(ws, "add", ".") + git(ws, "commit", "-m", "root") + root = git(ws, "rev-parse", "HEAD") + + remote = Path.home() / f"{tid}.git" + git(ws, "init", "--bare", str(remote)) + git(ws, "remote", "add", "origin", str(remote)) + + (ws / "impl.py").write_text("implementation = True\n") + git(ws, "add", "impl.py") + git(ws, "commit", "-m", "the commit that will be HEAD") + head = git(ws, "rev-parse", "HEAD") + + (ws / "more.py").write_text("more = True\n") + git(ws, "add", "more.py") + git(ws, "commit", "-m", "advanced past HEAD") + git(ws, "push", "-q", "origin", "HEAD:refs/heads/zzz-carrier") + + for i in range(nheads): + git(ws, "checkout", "-q", "--detach", root) + (ws / "decoy.py").write_text(f"decoy = {i}\n") + git(ws, "add", "decoy.py") + git(ws, "commit", "-q", "-m", f"decoy {i}") + git(ws, "push", "-q", "origin", f"HEAD:refs/heads/aaa-decoy-{i:04d}") + + git(ws, "checkout", "-q", "--detach", head) + git(ws, "clean", "-qfd") + kb.set_workspace_path(conn, tid, ws) + return tid, ws, head + + def _count_git_spawns(monkeypatch, fn): """Number of `git` subprocesses ``fn`` issues, and its return value.""" spawns = [] @@ -211,3 +264,81 @@ def test_missing_advertised_object_does_not_abort_the_scan(board): head = survivor._git(repo, "rev-parse", "--verify", "HEAD").stdout.decode().strip() assert survivor._remote_survivor(repo, head, [*published, phantom]) is None + + +def test_contained_head_capture_is_flat_in_ref_count(board, monkeypatch): + """SIBLING CASE: HEAD contained in the published set but not itself a tip. + + The first fix made the containment QUESTION set-based but left the step + that NAMES the containing ref as a `merge-base --is-ancestor` scan, so this + shape still paid the full per-ref cost the card was filed about. Measured + through the real `complete_task` at 5 -> 41 refs before the naming fix: + 24 -> 96 git spawns, slope 2.000/ref -> 1,424 s projected at 7,937 refs, + half of it BEFORE the durable write. + + Driven through `kb.complete_task`, not the helper, so it gates the live + transition the card is about -- and asserts the card reaches `done`. + """ + few_tid, few_ws, few_head = _workspace_contained_behind_a_tip(board, 4) + many_tid, many_ws, many_head = _workspace_contained_behind_a_tip(board, 40) + + few_spawns, few_ok = _count_git_spawns( + monkeypatch, + lambda: kb.complete_task(board, few_tid, summary="done", + metadata={"changed_files": ["impl.py"]}), + ) + many_spawns, many_ok = _count_git_spawns( + monkeypatch, + lambda: kb.complete_task(board, many_tid, summary="done", + metadata={"changed_files": ["impl.py"]}), + ) + + # The transition itself, not just the duration: this is what was lost. + assert few_ok is True and many_ok is True + assert kb.get_task(board, many_tid).status == "done" + # REGRESSION: the recovery index must still be recorded, unheld. A "fix" + # that skipped capture would satisfy the spawn bound and lose the pointer. + row = board.execute( + "SELECT held_reason FROM task_workspace_survivors WHERE task_id = ?", + (many_tid,), + ).fetchone() + assert row is not None, "completion dropped the survivor row" + assert row["held_reason"] is None, f"survivor HELD: {row['held_reason']}" + + assert many_spawns < few_spawns * 3, ( + f"contained-HEAD capture scales with published ref count: " + f"{few_spawns} spawns at 5 refs vs {many_spawns} at 41 refs — the " + f"per-ref merge-base loop is back in the ref-naming step" + ) + + +def test_contained_head_names_the_same_ref_as_the_per_ref_scan(board): + """Parity for the naming step: binary search must pick the SAME ref. + + Speed is not the contract. The bisect keeps `published` order, so it must + return bit-for-bit what the old `for ref in published: merge-base + --is-ancestor` scan returned -- including WHICH of several containing refs + wins when more than one qualifies. + """ + tid, ws, head = _workspace_contained_behind_a_tip(board, 6) + repo = ws.resolve() + # A SECOND containing ref, sorting after the first, so "first wins" is + # actually being tested rather than "only one candidate exists". The + # carrier lives only on the remote here (the workspace is detached), so + # name it by sha. + carrier = next(r["sha"] for r in survivor._published_refs(repo, repo) + if r["branch"] == "zzz-carrier") + git(ws, "push", "-q", "origin", f"{carrier}:refs/heads/zzz-carrier-2") + published = list(survivor._published_refs(repo, repo)) + + per_ref = None + for ref in published: + if ref["sha"] == head or survivor._git( + repo, "merge-base", "--is-ancestor", head, ref["sha"], check=False + ).returncode == 0: + per_ref = dict(ref, head=head) + break + assert per_ref is not None, "fixture is not case (c): nothing contains HEAD" + assert per_ref["sha"] != head, "fixture HEAD is itself a tip, not contained" + + assert survivor._remote_survivor(repo, head, published) == per_ref diff --git a/tests/hermes_cli/test_kanban_terminal_transition_ref_cost.py b/tests/hermes_cli/test_kanban_terminal_transition_ref_cost.py index be6e885f5c2c1..5187bc1386b98 100644 --- a/tests/hermes_cli/test_kanban_terminal_transition_ref_cost.py +++ b/tests/hermes_cli/test_kanban_terminal_transition_ref_cost.py @@ -134,3 +134,109 @@ def test_other_terminal_transitions_never_reach_the_ref_scan( f"published heads — it has acquired the starvation hole " f"kanban_complete had" ) + + +def _card_contained_behind_a_tip(conn): + """A CLEAN card whose HEAD is a strict ancestor of a published ref. + + `_card_with_many_published_heads` always leaves an unpushed commit, so it + only ever drives `_remote_survivor`'s "HEAD has outside commits" fast path. + This shape reaches the step that NAMES the containing ref -- which is where + a per-ref `merge-base --is-ancestor` scan survived the first fix and where, + measured on the real 7,939-ref workspace, the old form cost 6,082 spawns / + 653.6 s against the new form's 13 / 2.4 s (same ref, exact parity). + """ + tid = kb.create_task(conn, title="contained HEAD under many refs") + ws = kb.resolve_workspace(kb.get_task(conn, tid)) + ws.mkdir(exist_ok=True) + git(ws, "init", "-b", "main") + git(ws, "config", "user.name", "Test") + git(ws, "config", "user.email", "test@example.invalid") + (ws / "code.py").write_text("value = 1\n") + git(ws, "add", ".") + git(ws, "commit", "-m", "base") + root = git(ws, "rev-parse", "HEAD") + remote = Path.home() / f"{tid}.git" + git(ws, "init", "--bare", str(remote)) + git(ws, "remote", "add", "origin", str(remote)) + + (ws / "impl.py").write_text("implementation = True\n") + git(ws, "add", "impl.py") + git(ws, "commit", "-m", "the commit that will be HEAD") + head = git(ws, "rev-parse", "HEAD") + (ws / "more.py").write_text("more = True\n") + git(ws, "add", "more.py") + git(ws, "commit", "-m", "advanced past HEAD") + git(ws, "push", "-q", "origin", "HEAD:refs/heads/zzz-carrier") + + # Divergent decoys sorting BEFORE the carrier: a scan pays for all of them. + for i in range(NHEADS): + git(ws, "checkout", "-q", "--detach", root) + (ws / "decoy.py").write_text(f"decoy = {i}\n") + git(ws, "add", "decoy.py") + git(ws, "commit", "-q", "-m", f"decoy {i}") + git(ws, "push", "-q", "origin", f"HEAD:refs/heads/aaa-decoy-{i:04d}") + git(ws, "checkout", "-q", "--detach", head) + git(ws, "clean", "-qfd") + kb.set_workspace_path(conn, tid, ws) + return tid, ws + + +def test_complete_is_not_starved_before_or_after_the_durable_write( + board, monkeypatch +): + """Attribute the spawns to PRE-write and POST-write, and bound both. + + Severity depends on the split. Spawns before `write_txn` burn the caller's + ceiling *before* the transition is durable, so the worker reaches no + terminal state at all — the card's exact defect. Spawns after it are + best-effort cleanup (`_cleanup_workspace` -> `remove_workspace_dir`, a + SECOND `_remote_survivor` call site) and must not be able to starve the + caller either, per the card's required outcome #3. + + Measured with the per-ref naming loop restored: 12 -> 48 spawns from 5 to + 41 refs in EACH phase, slope 1.000/ref, 712 s projected per phase at 7,939 + refs. Both now sit at the fixed bound regardless of ref count. + """ + tid, ws = _card_contained_behind_a_tip(board) + + phase = ["pre"] + spawns = {"pre": [], "post": []} + real_git, real_txn = survivor._git, kb.write_txn + + def counting(repo, *args, **kwargs): + spawns[phase[0]].append(args[0] if args else "?") + return real_git(repo, *args, **kwargs) + + def marking_txn(conn): + phase[0] = "post" + return real_txn(conn) + + monkeypatch.setattr(survivor, "_git", counting) + monkeypatch.setattr(kb, "write_txn", marking_txn) + try: + ok = kb.complete_task( + board, tid, summary="done", metadata={"changed_files": ["impl.py"]}, + ) + finally: + monkeypatch.setattr(survivor, "_git", real_git) + monkeypatch.setattr(kb, "write_txn", real_txn) + + assert ok is True + assert kb.get_task(board, tid).status == "done" + assert len(spawns["pre"]) < SPAWN_CEILING, ( + f"{len(spawns['pre'])} git spawns BEFORE the durable write against " + f"{NHEADS + 1} published heads — the ceiling burns before the " + f"transition lands and the worker reaches no terminal state" + ) + assert len(spawns["post"]) < SPAWN_CEILING, ( + f"{len(spawns['post'])} git spawns in post-write cleanup against " + f"{NHEADS + 1} published heads — best-effort cleanup can starve the " + f"caller (remove_workspace_dir is a second _remote_survivor call site)" + ) + row = board.execute( + "SELECT survivor, held_reason FROM task_workspace_survivors WHERE task_id = ?", + (tid,), + ).fetchone() + assert row is not None and row["survivor"], "survivor row must still be recorded" + assert row["held_reason"] is None