Skip to content

fix(kanban): survivor capture cost is per-REMOTE-REF, starving the kanban_complete tool at 420s - #888

Merged
Kyzcreig merged 2 commits into
mainfrom
daedalus-opus/t_cbeb632f
Sep 23, 2026
Merged

Kyzcreig merged 2 commits into
mainfrom
daedalus-opus/t_cbeb632f

Conversation

@Kyzcreig

@Kyzcreig Kyzcreig commented Sep 22, 2026 •

Copy link
Copy Markdown
Collaborator

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 ^<every ref> -> 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 ^<unknown> 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 a3d0b75 -- 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 f8b6d54, 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.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

Kyzcreig and others added 2 commits September 22, 2026 12:38
…mplete 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 ^<every ref>` -> 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 ^<unknown>` 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 a3d0b75 -- 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 f8b6d54, 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.
…lete

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 ^<unknown>` 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.
@Kyzcreig

Copy link
Copy Markdown
Collaborator Author

🤖 merged-by: apollo · lane: landing-stack · gate: BYPASS: FleetReview is DISABLED by operator (Ace: systemctl disable --now fleetreview-router 09:30 PT; 'Fleet reviews currently paused'), so no FR record can exist for any head. Landing on kanban APPROVAL (Argus, EXECUTION lens, measured base) + fully green CI + Apollo pre-flight (no open review runs). Ace 18:17 PT: 'regarding the landing stack, 888 and 891, can we just handle that right now'. · why: t_cbeb632f APPROVED r2 EXECUTION lens (Argus checked out + ran everything; case (c) measured O(log N) on the live 632MB / 9,559-ref workspace). kanban_complete TOOL timed out at 420s from a per-ref git loop (2.000 spawns/ref x 7,937 heads); CLI closed the same card in 1.5s. CI 38/0 CLEAN. Foundation of the survivor stack; #889 and #891 land behind it.

@Kyzcreig
Kyzcreig added this pull request to the merge queue Sep 23, 2026
@Kyzcreig

Copy link
Copy Markdown
Collaborator Author

🤖 merged-by: apollo · lane: fr-pause-0922 · gate: BYPASS: FR PAUSED by Ace ruling 2026-09-22 (state/fleetreview-pause-20260922.md); t_cbeb632f approved · why: argus APPROVED r2 EXECUTION (survivor capture cost per-remote); CI green; Apollo merge pass 2026-09-22

Merged via the queue into main with commit 3489dd8 Sep 23, 2026
54 checks passed
@Kyzcreig
Kyzcreig deleted the daedalus-opus/t_cbeb632f branch September 23, 2026 01:39
Kyzcreig added a commit that referenced this pull request Sep 23, 2026
fork/main advanced 83 commits since 0dc715c, including the survivor
stack (#837 #842 #848 #856 #872 #879 #886 #888) which rewrote
kanban_survivor.py from 475 to 1516 lines. Four conflicts, all in
kanban_survivor.py, resolved toward main's shapes:

  _git         union: main's input= (needed by _present_commits/_rev_list)
               plus this PR's timeout= (needed by _content_advisory's
               120s fetch).
  _capture     kept main's extraction; folded this PR's canonical-tree
               fallback + mirror_hint advisory INTO it, so the
               _explain_broken_object_store classifier still wraps every
               object-reading step.
  ref arm      kept main's 'not bundles and len(refs) == len(repos) +
               len(carried)' (the #842/#848 carried-survivor accounting)
               and this PR's canonical sidecar.

The pre-#848 unbound-claim path is NOT reintroduced: _verified_explicit,
_unbound_keys, _reusable and _bound are main's, untouched. The landed
arm in preserve() and _verify_landed/_landed_contains_history merged
without conflict.

Verified:
  11 survivor files (4 from this PR + 7 landed since): 255 passed, 0 failed
  the PR's own 4 files:                                  98 passed, 0 failed
  mutation, dirty-tree guard neutered:    16 passed, 2 FAILED
  mutation, history binding neutered:     15 passed, 3 FAILED
  ruff on kanban_survivor.py + kanban_db.py: clean
  git diff --check: clean
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant