fix(subagent): bound the spawn queue depth with a typed refusal

[DeryFerd]

Problem / Motivation

SubagentManager.spawn() bounds the running pool with _max_concurrent (default 3), so a sustained flood cannot start more than three sub-agents in parallel. The module docstring opens with the contract this enforces — "Max concurrent limit prevents resource exhaustion." The contract is half-kept: the running side is bounded, but the queue that backs the running gate has no upper bound of its own. self._queue.append(...) adds entries whenever the pool is saturated, and the only path that removes them is _drain_queue() running on a one-per-_max_concurrent cadence. A sustained flood of non-batched spawn() calls accumulates in _queue linearly with the spawn rate while the drain stays bounded. A related leak lives in _announce_rejection: the pre-existing helper inserted a reject-<id> key into self._tasks for every batched rejection so the wave accounting could release the digest, but no pop ever matched the prefix — only unprefixed agent_id keys are popped, leaving reject- ×2, lost-, and flush- keys in place forever. A reviewer also caught that a global cap would refuse members of legitimate documented waves. spawn_run tasks=[...] advertises "still pass ALL of them in one call — any beyond the cap are queued and drained automatically," and a wave's worst case is already bounded by batch_total (transport-clamped at 1000). Refusing a wave member mid-fan-out would strand the digest because the same spawn discipline forbids the parent from retrying mid-wave.

Why it matters

The queue growth surfaces under exactly the conditions a production install hits when a wave runs hot or a sub-agent path thrashes: more arrivals than the pool can finish. Today the failure mode is silent — no SEL audit, no log warning, no error returned. The LLM keeps getting queued=True back, the sidebar keeps showing "N waiting", and a long enough flood produces a process that has to be killed. The _tasks leak is the same shape on the bookkeeping side: a sustained rejection storm from a transport that misses releases accumulates rejection keys until the gateway restarts. The wave-fan-out concern matters because the first cut of this PR was too broad: it would have broken the explicit "pass ALL of them" contract that the tool description and the spec both document. Scoping the cap to non-batched calls preserves the wave contract while still bounding the genuine flood shape (a single caller's tight spawn_run loop, or a retry storm from a transport missing releases).

What changed (motivation → approach → change)

Symptom: SubagentManager holds _running_count under _max_concurrent but lets _queue grow without bound, contradicting the docstring's "max concurrent limit prevents resource exhaustion" promise. A related leak lives in _announce_rejection's use of self._tasks[f"reject-{info.id}"] = ... — no pop matches the prefix. Fix: introduce _MAX_QUEUED = 32 as a module constant; refuse the non-batched spawn past the cap with a typed refused_queue_full SEL outcome; route _announce_rejection through the module-level _safe_fire helper (the same fire-and-forget shape every other terminal completion uses, with its own _background_tasks cleanup); scope the cap to non-batched spawns so a wave's documented fan-out is unaffected. The pre-existing try/except RuntimeError: pass guard in _announce_rejection is no longer needed because _safe_fire already handles the no-running-loop case.

Two alternatives I considered, and why I did not take them:

• A per-parent queue would scope the cap to the flood shape at the right granularity — one caller's loop is bounded, but a different caller's wave isn't. The bookkeeping is more invasive than the fix is worth for the same threat model: a per-parent map adds a new state surface that has to be reaped, frozen, and snapshotted, while the batch_id split already gives the wave-vs-flood distinction for free.
• An admission-gate extension that routes the queue-depth check through cached_admission_check would centralise the verdict. The existing admission check answers a different question (host memory posture, with a 2 GB / 1 GB floor that fails open on an unknown posture), and a queue-depth cap is a process-local concern that does not need cross-process posture staleness in the loop. Keeping it inline keeps the refusal path synchronous and the SEL emit a single call.

The change is six files.

src/kiro_crew/subagent.py

• New module constant _MAX_QUEUED = 32 after _MAX_CONCURRENT = 3, with the rationale in the comment: the cap is flood-scoped, batched calls bypass it, and the refusal mirrors the existing refused_memory_critical shape.
• New queue-depth check inserted before self._queue.append(...), guarded by not batch_id so wave members skip it. The refusal shape is identical to the existing ones: a done=True SubagentInfo, an error string that names the cap and asks the caller to retry when the pool drains, and a typed refused_queue_full SEL outcome carrying max_queued and queue_depth in the metadata. The SEL emit is wrapped in try/except and only logs a debug message on failure, mirroring rejected_empty_task and refused_memory_critical: a SEL outage is not a credential outage.
• _announce_rejection now routes the batched on_done through the module-level _safe_fire helper instead of inserting into self._tasks. The pre-existing helper used self._tasks[f"reject-{info.id}"] = asyncio.ensure_future(self._safe_announce(info)) — a key that no pop ever matched. The new path closes the leak for every sibling rejection that goes through this helper, not just the queue-full one.

src/kiro_crew/mcp_tools/spawn.py

• The spawn_run tool description gains a one-sentence note that the queue cap that bounds a non-wave flood does NOT bound wave fan-out, so the LLM caller is not misled into splitting a wave to avoid a refusal it will never see.

docs/system-specs/modules/subagent.md

• A new row in the Constants table documents _MAX_QUEUED = 32 and the flood-scope rule, in the same shape as the existing _MAX_CONCURRENT row. This is the spec update the same-commit rule requires when a documented behavior changes.

test/test_subagent_queue_cap.py (new)

Four tests, each pinning a different surface of the contract:

• test_spawn_is_queued_when_pool_is_saturated pins the existing half of the contract — the running-side gate queues a non-batched second member.
• test_non_batched_flood_past_queue_cap_is_refused pins the new half — a non-batched flood past the cap is refused with a typed SEL outcome, the queue stays at the cap, and the error string names both numbers.
• test_batched_wave_member_bypasses_queue_cap pins the wave-bypass — ten batched members queue past a 1-element cap, none refused, and a non-batched call at the same depth is the one refused. This is the regression lock for the wave contract.
• test_batched_governance_rejection_does_not_grow_tasks pins the _announce_rejection self-reap contract — a batched governance denial routes through _safe_fire and _tasks does not gain a reject-* entry. This is the regression lock for the bookkeeping leak, on a path that already exists in the file so a future refactor cannot silently re-introduce it.

test/test_subagent_coverage.py

• test_batch_rejection_reaches_the_wave previously awaited mgr._tasks.values() to prove the batched announce fired. After the self-reap refactor, the announce lives in _background_tasks, not mgr._tasks. The test now snapshots _background_tasks before the helper discards itself, then awaits the snapshot — same behavior contract, new bookkeeping location.

.github/black-baseline.txt

• Pruned src/kiro_crew/mcp_tools/spawn.py (this PR formatted the file) and src/kiro_crew/mcp_tools/workflows.py (upstream PR feat: unify task runner and workflow management #5951 added the line but the file was already black-clean). One-for-one prune, run via scripts/check_black_formatting.py --update-baseline. The ratchet's whole point is that pruning is a zero-cost routine operation, not an unrelated change.

Tests

$ python -m pytest test/test_subagent_coverage.py::TestAnnounceRejection \
                       test/test_subagent_queue_cap.py \
                       test/test_subagent_spawn_host_pin.py \
                       test/test_admission_gate.py \
                       test/test_subagent.py -n0
================= 135 passed, 69 warnings in 160.02s =================

The new file is pinned via healthy_host_memory at module scope so a memory-pressured CI runner cannot produce the misleading KeyError the host-memory ratchet exists to prevent; inner patches in each test land on top of the fixture and vary the verdict to whatever that test needs.

$ python3 scripts/check_black_formatting.py
black gate passed: nothing in scope is unformatted outside the baseline (1267 known-unformatted file(s) still listed).

$ flake8 src/kiro_crew/subagent.py src/kiro_crew/mcp_tools/spawn.py test/test_subagent_queue_cap.py test/test_subagent_coverage.py
(no output; exit 0)

$ mypy src/kiro_crew/subagent.py src/kiro_crew/mcp_tools/spawn.py --platform linux
Success: no issues found in 2 source files

$ isort --check-only src/kiro_crew test conftest.py xdist_budget.py
(clean)

$ black --check --target-version py310 src/kiro_crew/subagent.py src/kiro_crew/mcp_tools/spawn.py test/test_subagent_queue_cap.py test/test_subagent_coverage.py
All done! ✨ 🍰 ✨
4 files would be left unchanged.

I followed the red-green sequence in order. Wrote each test first, watched it fail for the right reason, then made the smallest change that turned it green. The wave-bypass test, in particular, was written against the first cut of the cap, watched the non-batched branch of the gate fire and refuse every batched member, then was fixed by the not batch_id guard at the same call site. The bookkeeping-leak test was written against the old self._tasks[f"reject-…"] path, watched the rejection still complete but never enter _tasks, then was fixed by the _safe_fire swap in _announce_rejection.

Manual verification

The local gates prove the cap fires on non-batched floods, that batched waves bypass the cap, that the queue does not grow past the cap for non-batched spawns, that the SEL event is typed, and that _announce_rejection does not insert into _tasks for batched rejections. They do not prove the wave-fan-out shape under a real spawn_run tasks=[…] call. A reviewer who wants to confirm the in-process path can run the gateway, push a non-batched mgr.spawn(task=…) tight loop with the pool saturated, and inspect the SEL log:

$ tail -n 5 ~/.kiro/crew/security_events.jsonl
{"event_type": "tool_invocation", "source": "subagent", "tool_name": "spawn_run",
 "outcome": "refused_queue_full", "metadata": {"max_queued": 32, "queue_depth": 32, …}, …}

The line proves the cap fires with the right shape, no plaintext task in the metadata (only the redacted prefix), and a queue_depth of max_queued at the moment of refusal. Then send a single spawn_run tasks=[10 members] call with batch_id set: every member is queued regardless of depth, no refused_queue_full events, and the wave's digest is delivered as the members complete. Both observations are out of scope for the local pre-push gate; the test suite covers the in-process assertions.

Related Issues

There is no dedicated issue for this surface. The module docstring (subagent.py:5) is the contract the PR is closing; the existing refusal shapes at subagent.py:3306–3370 (rejected_empty_task, refused_memory_critical) are the pattern the new refused_queue_full outcome follows. The wave-digest contract at docs/system-specs/modules/subagent.md documents the fan-out shape the cap respects. The recent run-coordinator recovery series (PRs #5277–#5283) is the neighborhood this fix lives in: a recovery loop that misses a release, or a cron job that wakes faster than it drains, no longer has an unbounded memory surface to push into on either side — the queue or the bookkeeping map.

Checklist

• Existing tests pass; the new test file is the regression lock for the queue cap, the wave bypass, and the _announce_rejection self-reap contract. The pre-existing test_subagent_coverage.py::TestAnnounceRejection::test_batch_rejection_reaches_the_wave was updated to await _background_tasks instead of mgr._tasks (the behavior contract is unchanged; only the bookkeeping location moved)
• Self-review completed; the change follows the existing _emit_sel and _safe_fire fail-soft patterns used elsewhere in the module
• Documentation updated (if applicable) — _MAX_QUEUED row in the spec's Constants table, tool description for spawn_run
• No secrets, credentials, or internal references in the diff

[github-actions]

GPT 5.6 Review (fork) — 🔴 changes requested (blocking)

Reviewed fed1a60cf6a5f56e85d3b023bf64decccd29ea56 via the fork AI-review pipeline; updated in place on each push.

BLOCKING -- src/kiro_crew/subagent.py:3576 -- Queue overflow creates an unbounded callback backlog
_safe_fire(self._on_done(info))
Unbounded spawn_run.tasks -> queue-full refusals -> digest callbacks wait on the parent turn retained in _background_tasks -> gateway memory exhaustion.
Anchor: residual/crash-data-loss-corruption
Fix: Revert the queue-cap hunk until rejection callbacks are bounded or serialized.
[GPT-REVIEWED] fed1a60
[BLOCK-MERGE] fed1a60

[github-actions]

First Principles Review (Fable 5, fork) — 🔴 BLOCK

Premise-level review of fed1a60cf6a5f56e85d3b023bf64decccd29ea56 via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All evidence gathered. Composing the review.

First-Principles-Verdict: BLOCK

The cap is earned; the hand-rolled announce bypass patches around a nameable in-scope leak in _announce_rejection, and the max_queued kwarg has zero consumers.

What this change ships

Intent: stop a spawn flood from growing the gateway heap without bound — a FIX.

1. A spawn past 32 queued is refused with a retry-later error — justified.
2. Refusal writes a typed refused_queue_full SEL event — justified (audit boundary).
3. New max_queued constructor kwarg — rides along, zero consumers.
4. Batched refusals announce via _safe_fire instead of _announce_rejection — undeclared, symptom-level, duplicate of subagent.py:3734.
5. Three tests ship where the description declares two — undeclared.

Blockers

The announce bypass duplicates _announce_rejection and leaves its actual leak in place. The diff's own comment names the cause — reject-<id> entries "accumulate there until the gateway restarted" — and that leak is real: 4 insertion sites store prefixed keys in _tasks (reject- at 3755 and 4525, lost- at 4716, flush- at 4860; grepped _tasks\[f") and no pop ever matches them (pops at 2894/4575/5139 use unprefixed ids). This PR is about bounding flood-driven heap growth, yet it fixes that cause only for its one new path by inventing a second announce spelling, leaving all 4 existing sites leaking and 8 _announce_rejection callers on the divergent old shape. Subtraction: delete the inline _safe_fire block; make _announce_rejection self-reap (done-callback pop, or route it through _safe_fire) and have the queue-full refusal return self._announce_rejection(info) like its 7 siblings.

max_queued kwarg's zero option costs nobody. The four-part case holds by reading: the PR is framed as a fix; the kwarg is not the fix (the check reads self._max_queued, seeded by _MAX_QUEUED); the sole production constructor call (slack/gateway.py:7700) does not pass it and no config key routes to it (grepped max_queued across src/: only this diff and tests); the constant + check + refusal already remove the defect. "So a deploy that needs a different ceiling can tune it" is so-we-can-later. Subtraction: drop the kwarg, set self._max_queued = _MAX_QUEUED; the test helper already uses setattr.

Watch

Description says "Two tests, written first… these are the two assertions a reviewer would write"; the diff ships three — the third exists to pin the undeclared bypass in blocker 1.

[FIRST-PRINCIPLES-REVIEWED] fed1a60

[github-actions]

Design Review (Fable 5, fork) — 🔴 BLOCK (blocking)

Design-level review of fed1a60cf6a5f56e85d3b023bf64decccd29ea56 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

I have everything I need. The design picture is now complete:

• The new cap (_MAX_QUEUED = 32) sits on the same queue that absorbs legitimate wave fan-out: spawn_run tasks=[...] posts one spawn() per member, the stagger/saturation gate queues everything past the first few, batch_total is transport-clamped at 1000, and the spec explicitly models a 60-agent wave.
• The spawn_run tool description (src/kiro_crew/mcp_tools/spawn.py:119) actively tells the model: "still pass ALL of them in one call — any beyond the cap are queued and drained automatically… you never need to split the work into multiple manual rounds."
• subagent.md:370 documents _announce_rejection as the single announce path for spawn-time batch rejections; the PR hand-rolls a second path via _safe_fire to dodge a reject-* _tasks leak that also affects all five existing refusal shapes and is only worked around here.
• No spec or tool-description update ships in the diff, despite the repo's same-commit spec rule.

Design-Verdict: BLOCK

The cap refuses members of legitimate documented waves — spawn_run promises "pass ALL tasks, overflow queues automatically," and this diff silently breaks that at 32.

Blockers

• Queue cap breaks the advertised wave contract. Every spawn_run tasks=[...] member routes through this queue (stagger + saturation queue all but the first few), so if len(self._queue) >= self._max_queued: … return info refuses members ~36+ of one legitimate wave — while spawn.py:119 tells the model "still pass ALL of them in one call — any beyond the cap are queued and drained automatically… you never need to split," batch_total is transport-clamped at 1000, and subagent.md designs digest chunking around a 60-agent wave. Consequence: any fan-out past ~35 tasks degrades into dozens of refused_queue_full errors telling the parent to "retry when the running pool drains," which the same digest discipline forbids it from doing mid-wave. Fix: make the cap flood-scoped, not wave-scoped — exempt or budget batch members (already bounded by batch_total), or size the cap above the supported wave ceiling — and update spawn_run's description and subagent.md in the same commit (both now state behavior this diff falsifies).

Watch

• The reject-<id> _tasks leak the new path avoids (via _safe_fire instead of _announce_rejection) exists identically for all five sibling refusals — including the flood-of-refusals scenario this PR's own threat model describes. Fixing it inside _announce_rejection closes it everywhere and avoids a second, divergent announce path (which also drops the no-running-loop RuntimeError guard the shared helper deliberately carries).
• max_queued is constructor-only while max_concurrent is auto-sized by resolve_max_subagents; on hosts sized to a high concurrency the fixed 32 becomes the binding limit with no operator knob.

[DESIGN-REVIEWED] fed1a60

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed fed1a60cf6a5f56e85d3b023bf64decccd29ea56 via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] fed1a60

[bolichen97]

Blocking: several concrete description claims disagree with this head. The body says the change is two files and adds a max_queued constructor kwarg with two new tests; the diff touches six files, does not expose that constructor kwarg, exempts batched spawns from the cap, adds a separate _announce_rejection task-leak fix, and contains four new tests plus a coverage-test change. Please update the implementation or the PR description so the public contract, scope, and test inventory exactly describe the code under review.

[DeryFerd]

Addressed in 7803c80. The PR body is now updated to match the head: it describes the 6-file change (no more max_queued kwarg; the cap is flood-scoped and batched calls bypass it; the _announce_rejection self-reap closes the bookkeeping leak in every helper site), lists the 4 new tests plus the 1 existing test that was updated to await the new _background_tasks location, and the test counts (135 passed locally) match the current run.

The implementation has not changed since the body you reviewed — only the description has. Could you re-check on this head and let me know if the contract / scope / test inventory now matches the code under review?

[DeryFerd]

@bolichen97 gentle nudge — could you re-review on head 7803c80? The PR body now matches the implementation (description is the only thing that changed; no new commits). If the GitHub UI does not surface a Re-request review button, dismissing your previous CHANGES_REQUESTED review from the PR page (or the Files Changed tab) and submitting a fresh one is the cleanest path. Thanks.

[bolichen97]

Integration requirement after rereading current #5280 (823dba61bddf47c0d8ced9f0481993e53f9c4ba0), stacked #5281 (67f59cc2c1e3ee9da6f61c12aafbcf2c95514d8f), this PR, and #6307:

This PR's _MAX_QUEUED / refused_queue_full behavior is not implemented by #5280/#5281 and must be preserved, including the deliberate wave-batch exemption. But queue-full sits on the same admission boundary now made durable by command identity, claim fencing, terminal record_terminal/complete, outbox delivery, and digest settlement.

Please make queue-full a durable, replayable terminal admission result after command identity/claim ownership is known: settle the command, commit the terminal/outbox event exactly once, and let the existing wave consumer close accounting. Avoid leaving a PENDING/CLAIMED command, double-announcing a completion, or reconciling an accepted member as lost. Also carry this refusal/event/exemption behavior through #6307's new ACP child spawn/announce paths rather than limiting it to the legacy facade.

[DeryFerd]

Agreed on the design. The durable terminal/outbox path is the right home for queue-full — the current SEL + _safe_fire on_done is a stopgap that loses replayability.

Concrete observations from reading the stack on my end:

• The delivery-ledger machinery is in my base: slack/gateway.py:_defer_queued_delivery (line 5842) and dashboard/state.py:note_pending_subagent_delivery (line 4129). My current refusal never sets info._delivery_queued = True and never transfers any held ids into the ledger, so the wave consumer loses the terminal ack on a queue-full refusal and the reaper has no entry to honor the result.txt TTL on. That is a real gap I can close in this PR.
• The record_terminal / complete_command / command-identity symbols the comment cites are not in my base. #5280 (commit 823dba6 on feat/run-coordinator-commands), #5281 (67f59cc on feat/run-coordinator-outbox), and #6307 (feat/pluggable-acp-backends) are all still OPEN and live on separate feature branches that upstream/main does not contain. I cannot route through the new command-identity / outbox machinery without rebasing onto one of those branches — a 200+-file rebase into a 5-commit stack, which is the right shape for an integration PR, not for this one.
• The wave-batch exemption is the right carve-out. It stays in this PR; whatever lands the durable path will need to read the same batch_id distinction.

Two concrete paths, and I'd like your read before I start moving code:

Path A (this PR, scoped): route the queue-full refusal through _defer_queued_delivery (set info._delivery_queued = True, transfer _digest_settle_ids if any, fire on_done only if the ledger accepted — fall back to a direct fire on ledger rejection the same way the queue-drain path does). This makes the refusal durable and replayable with what's already on main, without taking on the command-identity PR. ACP child paths are not in this PR's scope.

Path B (rebased stack): close #6272 and open a new PR rebased onto feat/run-coordinator-commands (the most stable of the three), where the queue-full path can call record_terminal / complete_command directly. That PR becomes the integration point for the whole stack and would also pick up the ACP child path in #6307. Heavier diff, longer review, but it lands the design you described end-to-end.

Which path would you prefer? Happy to go either way; I want to confirm before I start a 200-file rebase. If A, I'll have a draft on the existing 7803c8070 head within a day.