🐛 fix(kanban): never spawn a tool-less worker; bound per-tick spawn burst - #10
🐛 fix(kanban): never spawn a tool-less worker; bound per-tick spawn burst#10cwest wants to merge 1 commit into
Conversation
…urst
A dispatcher-spawned worker (Salton, knowledge board) came up with ONLY the
base kanban_* coordination tools — no web/shell/git/file — despite its profile
correctly declaring a full toolset, then self-blocked ("only kanban_*
coordination tools"). Root cause: under a stuck->mass-spawn recovery the
dispatcher spawned all 20 ready cards in one tick; _resolve_worker_cli_toolsets
came up degenerate and _default_spawn silently launched the worker WITHOUT a
--toolsets pin (the `if worker_toolsets:` guard), letting it fall back to a
kanban-only surface and waste a full LLM cycle.
Two layers, smallest-footprint first, fixing the whole class (the single
_default_spawn helper covers both the ready and review dispatch call paths):
1. Never spawn tool-less. _default_spawn now REQUIRES a non-empty resolved CLI
toolset and raises when resolution is degenerate. dispatch_once's existing
spawn-failure handler records the failure with release_claim=True, so the
card is reclaimed to `ready` for a clean retry instead of running crippled.
_resolve_worker_cli_toolsets always recovers at least the kanban lifecycle
surface for a real profile home, so None/empty is a genuine failure, not a
legitimately tool-less profile.
2. Bound the per-tick spawn burst. New kanban.max_spawn_per_tick caps how many
workers a single tick may launch (ready + review combined), distinct from
max_spawn (a live concurrency cap). Prevents a stuck->recovery tick from
dumping the whole ready queue at once — the condition under which workers
raced into tool-less spawns. Wired through the gateway dispatcher and the
CLI dispatch path; unset preserves historical unbounded behavior.
Behavior-contract tests: a spawned worker's resolved toolset is pinned and a
degenerate resolution reclaims the card (real _default_spawn against a temp
HERMES_HOME, not a mock); N>cap ready cards spawn at most cap per tick; and the
gateway forwards kanban.max_spawn_per_tick to dispatch_once.
cwest
left a comment
cwest
left a comment
There was a problem hiding this comment.
Read the diff against the head SHA and ran the suite in a throwaway clone. The fix is correct and the reclaim path holds up under inspection: _default_spawn raising on a degenerate toolset flows into dispatch_once's existing except handler, which calls _record_spawn_failure(release_claim=True), and below the failure limit that UPDATEs the card back to status='ready' with claim_lock cleared. So a tool-less resolution reclaims for a clean retry instead of launching a crippled worker — exactly what the description claims. The per-tick break is placed before claim_task in both the ready and review loops, and the gateway wires max_spawn_per_tick through with a test asserting the forward.
Test results from the clone at the head SHA:
- The 7 new tests: all pass.
- test_kanban_db.py + test_kanban_worker_spawn_toolsets.py: 230 passed.
- test_kanban_core_functionality.py: 176 passed, 1 skipped.
- Red-green check: reverting the guard back to
if worker_toolsets:makes both _default_spawn toolless tests fail (they reach the fail_popen stub), so the invariant is genuinely covered.
Commit is signed (ED25519, casey@geeknest.com), Conventional + emoji, no AI attribution.
Two small notes inline, neither blocking. Holding the formal approve since this is still a draft.
| # recovers at least the kanban lifecycle surface for a real profile home, | ||
| # so a None/empty result here is a genuine resolution failure, not a | ||
| # legitimately tool-less profile. | ||
| worker_toolsets = _resolve_worker_cli_toolsets(env.get("HERMES_HOME")) |
There was a problem hiding this comment.
The comment above says _resolve_worker_cli_toolsets "always recovers at least the kanban lifecycle surface," but its except branch also returns None (e.g. load_config or _get_platform_tools raising on an exotic config), and that None now lands here as a hard spawn failure. That's the safe direction — reclaim + retry beats launching tool-less, and the consecutive-failures breaker eventually blocks a card that keeps failing — so no change needed. Just flagging that the "always recovers" framing is a bit stronger than the code guarantees: a config-load exception fails the spawn rather than recovering a surface.
|
|
||
| import subprocess as _subprocess | ||
|
|
||
| def fail_popen(*args, **kwargs): # pragma: no cover - must not be reached |
There was a problem hiding this comment.
This E2E asserts the reclaim outcome, which is the right thing to lock down. Worth knowing it isn't a tight guard on the new raise specifically: because Popen is stubbed to throw, the test still passes if you revert default_spawn to the old if worker_toolsets: path — the spawn fails either way and dispatch_once reclaims. The two test_default_spawn_raises* tests in test_kanban_worker_spawn_toolsets.py are what actually pin the raise (they fail red when the guard is reverted), so coverage of the invariant is fine overall.
|
Superseded by #20, which cherry-picked this fix onto cwest/integration (the fork dev line) and merged. This PR targeted origin/main (upstream mirror), where the fix does not belong. Closing as superseded. |
…e_check_xsrf pitfalls Add two pitfalls discovered when running the skill against a fresh Jupyter server: - Pitfall #9: When the websocket reply channel hangs on every execute even though the kernel actually ran (REST shows execution_state=idle and execution_count increments), force zmq transport with --transport zmq. The zmq transport uses jupyter_client directly and sidesteps the broken websocket layer. - Pitfall #10: A fresh ServerApp rejects POST /api/sessions with "_xsrf argument missing from POST" unless you start it with --ServerApp.disable_check_xsrf=True. Needed for REST-only flows where no browser/cookie is establishing the XSRF token.
1 participant
Symptom (reproduced)
A dispatcher-spawned kanban worker (Salton, knowledge board) came up with only the base
kanban_*coordination tools — no web/shell/git/file — despite its profile correctly declaring a full toolset, then self-blocked ("only kanban_* coordination tools"). Casey: "This block and reason should never happen."Evidence:
gateway.loglines 10902–10954 show theknowledgeboard stuck 16 ticks thenspawned=20in ONE tick at 15:58:43; the 4 blocked cards (t_68b324b4 / t_15e5879f / t_b4defdf4 / t_87a0dc5f) all spawned at that instant with the tool-less block reason.salton/config.yamlcorrectly declares[hermes-cli, terminal, file, web, skills, todo, search].Root cause (whole class)
_default_spawnresolved the worker CLI toolset via_resolve_worker_cli_toolsets, but under the stuck→mass-spawn burst that resolution came up degenerate (None/empty). The oldif worker_toolsets:guard then silently launched the worker WITHOUT a--toolsetspin, letting it fall back to a kanban-only surface and burn a full LLM cycle before self-blocking. The single_default_spawnhelper backs both the ready and review dispatch call paths, so fixing it there fixes the whole class.Fix (two layers, smallest-footprint first)
Never spawn tool-less (the invariant).
_default_spawnnow requires a non-empty resolved CLI toolset and raises when resolution is degenerate.dispatch_once's existing spawn-failure handler records the failure withrelease_claim=True, so the card is reclaimed toreadyfor a clean retry instead of running crippled._resolve_worker_cli_toolsetsalways recovers at least the kanban lifecycle surface for a real profile home, so None/empty is a genuine failure — not a legitimately tool-less profile.Bound the per-tick spawn burst (defense in depth). New
kanban.max_spawn_per_tickcaps how many workers a single tick may launch (ready + review combined), distinct frommax_spawn(a live concurrency cap). Prevents a stuck→recovery tick from dumping the whole ready queue at once — the condition under which workers raced into tool-less spawns. Wired through the gateway dispatcher and the CLI dispatch path; unset (None) preserves historical unbounded behavior; invalid/<1 normalizes to None.Config key the dispatcher loop reads (for the companion hermes-config card): live concurrency cap is
kanban.max_spawn; the new per-tick knob iskanban.max_spawn_per_tick.Tests (behavior contracts, real spawn path vs temp HERMES_HOME)
--toolsetspin equals its profile's declared CLI toolsets (invariant)._default_spawnraises andPopenis never reached → card reclaimed, never a kanban-only worker. Red-green verified: reverting the guard makes both tests fail.N > capready cards spawn at mostcapper tick.kanban.max_spawn_per_ticktodispatch_once.Targeted kanban + new tests: 406 passed, 1 skipped. Full
tests/hermes_cli/failures match the pristine integration baseline (178 pre-existing webhook/web_ui ordering-pollution failures, all in files untouched by this diff) — zero new regressions.PR only — do not deploy. Casey merges and deploys.