fix(agents): hold the wave-close fallback open while a member's report is in flight

[javenciu]

Problem / Motivation

A multi-member wave can emit its final digest twice, or emit a premature "wave finished" with results missing, when one member's SubagentInfo.done flips before that member's terminal report reaches the gateway completion consumer (issue #8554).

batch_members_pending() stops counting a member the moment info.done flips, but the member's contribution to the consumer's bp["done"] only lands when its (shielded, possibly slow) terminal report actually executes the consumer. In that gap, ANY sibling completion that reaches the consumer sees done < total AND batch_members_pending() == False, so the last-member fallback finalizes early — and the in-flight report then re-creates the batch-progress record via setdefault and "finalizes" the same wave again. Reachable with two RUNNING members alone: member A done-but-unreported while member B's report executes the consumer.

Why it matters

The parent hears "wave finished — all results delivered" while a member's result is still in flight, then receives a second contradictory wave-close digest for the same batch. Both digests carry wrong counts (the premature one is missing the in-flight member entirely), the spawn-discipline gate releases early, and finalize_batch prunes wave bookkeeping twice. Anything a parent agent does on the strength of the first "complete" digest — spawning follow-ups, synthesizing results — runs against a wave that was not actually finished.

What changed (motivation → approach → change)

Observed symptom: double/premature wave-close digests. Root cause: the done-but-unreported window above — the wave-close fallback consults a predicate (batch_members_pending) that stops counting a member strictly before the count it guards (bp["done"]) includes that member.

The change extends the issue's suggested direction — count a member as outstanding from done=True until its report enters the consumer:

• WaveDigestCoordinator.batch_reports_in_flight_impl() (new, subagent_manager/waves.py): True while any member of the batch is done-but-unreported. The consumer's last-member fallback now requires not batch_members_pending(id) and not batch_reports_in_flight(id).
• The hold lives in a manager-level _reports_in_flight registry (batch_id -> set of member ids), NOT on the agent records: _agents membership is operator-mutable (DELETE /api/spawn pops every done member), so a predicate derived from it silently drops the hold when a clear-completed lands inside the window — reopening the exact hole it closes. The report machinery arms the hold in the same synchronous block that flips info.done (safe-announced synthetics arm immediately before their announce; flush-only records never arm), and the consumer (slack/gateway.py) releases it in the same synchronous block that increments bp["done"], so the hold and the count can never be observed apart.
• A report that ends without reaching the consumer releases its hold through ONE structural try/finally wrapping the entire report body past the done-flip (subagent_manager/terminal.py, mirrored for _safe_announce in subagent_manager/admission.py — covers the registered approval-parked rejection): injection timeout, announce failure, cancellation, and early returns all funnel through it, preserving the wave's degraded a-sibling-can-close liveness instead of stranding. The cancelled-recovery limbo arm (subagent_manager/cancellation.py) needs no release at all: holds are armed only at a report's done-flip, which that arm's not info.done guard proves never ran.
• The reaper's digest-hold sweep is in-flight-aware in both directions: it skips a wave only while its close is genuinely in flight, and force-flushes a wave whose hold aged past the deadline with no report in flight — the state where the wave-close flush is never coming (a terminal report ended without reaching the consumer); previously that state was unsweepable and the held sibling results stranded until gateway restart.

Commit 2 (review round): the first commit kept the hold as a flag on the agent records (_report_consumed) with four per-arm clears. Review convicted the record-derived predicate — an operator clear-completed landing inside the done-but-unreported window popped the member from _agents and the hold evaporated with it, reopening the premature-finalize hole (probe: hold established → operator clear → batch_reports_in_flight flips False at the commit-1 tree; stays True after). Commit 2 moves the hold to the manager registry, replaces the per-arm clears with the structural finally, and closes both design concerns raised alongside: a stranded hold is now sweepable, and a report-path failure on the final member no longer parks held siblings behind an unconditional sweep skip.

Alternatives considered and rejected:

• Per-batch outstanding-report counter (the issue's other sketch): a counter decremented "at the top of the report path" is distributed arithmetic with an increment/decrement pairing to keep balanced across the shield, _force_reap, and cancel-recovery — a missed or doubled decrement is invisible (the count is just wrong). The registry keeps per-member identity instead: release is an idempotent discard keyed by member id (double-release harmless, structurally guaranteed by the report path's finally), and a stranded entry names exactly which member stranded. Commit 1's per-member flag had the same idempotence but derived the predicate from _agents membership; the registry keeps the flag design's degrade-safety without the membership dependence.
• Consulting report-task liveness: report tasks are shielded and keyed by several owners (_tasks[id], recovery keys, queued-stop synthetics); deriving "in flight" from task registry state couples wave accounting to task-registry lifecycle details that cancel_all() and the reaper mutate mid-teardown.
• Generalizing a _batch_unqueued_pending bridge: the issue notes the Stop all leaves queued subagents running #8270-adjacent bridge as prior art, but it never landed on main (its PR closed unmerged), so there is nothing to generalize — this change stands alone against current main.

Scope note: if the FINAL member's report terminally fails (timeout/announce-failure), no later completion re-evaluates the wave. Commit 1 left that pre-existing gap to the reaper's sweep as backstop; commit 2 makes the sweep actually able to take it — the in-flight-aware guard force-flushes an aged hold with no report in flight, where before the sweep skipped every no-pending-members wave unconditionally. Held sibling results now flush after the hold deadline instead of stranding until restart.

Tests

test/test_wave_digest_double_finalize.py (new) drives the REAL _subagent_done closure (captured from _init_subagents, same construction as the existing cron-injection suite) with a scripted manager, so the race window is deterministic instead of a timing lottery:

• test_sibling_completion_in_the_done_but_unreported_window_does_not_close_the_wave — THE race: fails before the fix with Expected 'finalize_batch' to not have been called. Called 1 time (the premature finalize), passes after; then the in-flight report lands and the wave closes exactly once.
• test_in_flight_report_cannot_refinalize_after_the_wave_closed — count-side no-double-fire control (passes both sides).
• test_spawn_failure_fallback_still_closes_the_wave — no-new-deny control: the fallback this fix constrains still closes waves whose members failed at spawn (passes both sides).
• test_consumer_releases_the_hold_with_the_count — pins the hold-release/count same-block contract; the release goes to the manager-level registry, so it survives the member having been popped from _agents by an operator clear mid-flight.

test/test_subagent_coverage.py (extended, sibling of TestBatchMembersPending):

• TestBatchReportsInFlight — 8 predicate/registry pins: done-but-unreported → True; consumed → False and the wave's registry entry is pruned, not left empty; test_operator_clear_cannot_drop_the_hold — THE commit-2 conviction: the hold survives the member being popped from _agents (what DELETE /api/spawn does to done members) and the consumer still releases by id; flush-only synthetics never arm; release is idempotent and unarmed release is a no-op; finalize_batch prunes the registry; still-running member → False (that is batch_members_pending's job — the arm happens only at the report's done-flip); other wave's member → False.
• TestReportsInFlightStrandBackstops — 5 pins that the structural finally releases however a report ends: injection-timeout arm and announce-failure arm (each also asserts the hold was VISIBLE mid-announce — armed at the done-flip, released only when the report ended); _safe_announce failure arm (arms before its announce, releases on the raise); the happy path (hold visible to the consumer, released with the count, the report's finally re-release an idempotent no-op); the cancelled-recovery limbo arm (driven deterministically: recovery parked on its original-task wait, then cancelled) never strands the wave — a record terminalized there never carried a hold.
• TestSweepDigestHolds — the sweep's two new directions: test_closing_wave_not_forced (hold armed → skip: the real wave-close flush is in flight) and test_stranded_hold_past_deadline_is_forced (no pending members, no hold, deadline passed → force-flush fires with the wave's parent key: the previously unsweepable stranded state).

test/test_subagent_scale.py (adjacent pins updated to the commit-2 contract): the settle-after-_on_done source inspection follows the report body into _report_terminal_guarded_impl (the split that hosts the structural finally), and TestDigestHoldDeadline.test_closing_wave_is_not_force_flushed now arms the in-flight hold — the aged-hold state WITHOUT one is the stranded state the sweep must flush.

Fails-before, commit 1 (fix stashed, committed tests kept): 12 failed / 2 passed — failures are the race conviction plus the predicate/hold absence; the 2 passes are the two both-sides controls. Fixed tree: 14/14.

Fails-before, commit 2 (at the commit-1 tree): the operator-clear probe — hold established (in_flight=True), DELETE /api/spawn-equivalent pop, in_flight flips False (premature-finalize hole reopened); at the commit-2 tree the same probe holds True through the pop. test_stranded_hold_past_deadline_is_forced also fails at the commit-1 tree (force_digest_flush never called — the sweep skipped every no-pending-members wave unconditionally). Full gate mirror green at each commit: targeted + seam-neighbor suites (subagent, batch injection, scale, reap-race, approval-parked, delivery-TTL, manager boundaries, persistence), mypy clean on all six touched source files, flake8/isort clean, black baseline gate pass, docs-lint pass.

Manual verification

N/A — unit coverage sufficient: the race is convicted deterministically through the real completion-consumer closure with a scripted manager (no timing dependence), and both liveness directions (hold open / never strand) are pinned at the manager level.

Screenshots / video

No visual delta: backend-only PR — subagent wave-close logic, gateway wiring, docs, and tests; zero rendered surfaces change.

Related Issues

Part of #8554 — addresses the done-but-unreported double-finalize mechanism; the issue also discusses whether the finalize decision should move entirely onto the report path (single source of truth), which is a maintainer-owned design question this change deliberately leaves open.

Pattern harvest

Rule candidate: review-prompt — "when a completion decision combines a COUNT (incremented by consumers) with a PREDICATE (derived from state flags), verify the predicate keeps counting every entity until its count contribution lands; a predicate that releases before the count includes it opens an early-completion window between the two." The same shape is worth checking anywhere a done-style flag feeds a pending-predicate while a separate consumer owns the tally.

Adjacent audit performed (not changed here): every done = True writer in subagent_manager/ was audited for report routing — run-path arms report via the finalize claim, the reap path reports, admission rejections announce, queued-stop synthetics register-then-report, lost-submission/digest-flush synthetics never register (invisible to the predicate by construction). The cancelled-recovery limbo arm was the one report-free writer, and it is covered in this change.

Checklist

• At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable) — docs/system-specs/modules/subagent.md (the AGENTS.md-routed owning doc) gains the wave-close contract paragraph and the cancelled-recovery hold-clearing note, same commit
• No secrets, credentials, or internal references in the diff

Contribution License Agreement

Per the template placeholder (CLA text pending): offered under the same terms as my prior merged contributions to this repository (#8835).

[github-actions]

Design Review (Fable 5, fork) — 🟡 CONCERNS

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

Race confirmed against base: batch_members_pending_impl drops a member at the done flip (waves.py:42) while bp["done"] lands only at consumer time, and setdefault at gateway.py:7383 resurrects a finalized wave. The diagnosis and mechanism are real; ownership choice (manager registry vs. operator-mutable _agents) is derived from a named mutator. The one design cost is the new distributed invariant.

Design-Verdict: CONCERNS

Sound fix to a real double-finalize race, but it mints a convention — "every done=True flip must arm" — enforced only by audit across ~8 sites.

Watch

• The flip+arm pairing is structural nowhere: a future terminal path that writes info.done = True without calling arm_report_in_flight silently reopens a (narrower) done-but-unarmed window. The PR's own "adjacent audit performed" of every done = True writer is exactly the manual process that will not be repeated by the next contributor; nothing (test or lint) pins the pairing itself.

Suggestions

• Fold flip+arm into one manager helper (e.g. mark_terminal(info) that sets done and arms), converting the 8 comment-guarded call sites into a single structural seam future writers can't miss.

[DESIGN-REVIEWED] 9ab40ff

[github-actions]

First Principles Review (Fable 5, fork) — 🟡 CONCERNS

Premise-level review of 9ab40ff74f53caeec2fb2d04c253bd19858ff5d4 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 base-tree evidence is in. The delegation-machinery check cleared the one zero-consumer suspect (the manager delegate is mandated at import time by copy_component_docs), and the sibling count of done-flip sites turned up exactly one unarmed flip. Final review:

First-Principles-Verdict: CONCERNS

Cause-level fix that earns its registry — but one terminal done-flip (cancellation.py:127) is left outside the "EVERY flip arms" invariant the spec now claims.

What this change ships

Intent: stop a multi-member wave from emitting a premature and then a duplicate wave-close digest when a sibling completes while a member is done-but-unreported (issue #8554) — a FIX.

1. Wave-close fallback now also waits for in-flight terminal reports; digest fires exactly once — justified (the fix).
2. Every terminal done-flip arms a manager-registry hold, released by the consumer or the report's structural finally — justified; one flip site left late-armed (see Watch).
3. Operator clear-completed mid-window can no longer drop the hold (registry, not _agents) — justified, derived from DELETE /api/spawn.
4. Reaper force-flushes an aged held wave with no report in flight instead of stranding results until restart — declared; the fix's own release-without-accounting arms need this exit.
5. New manager surface batch_reports_in_flight / arm_report_in_flight / consume_report_hold — counted: 2 / 12 / 5 non-test call sites.
6. SubagentManager._report_terminal_guarded delegate — mandated: copy_component_docs (subagent.py:2504) getattrs a facade method per coordinator _impl with no default; import fails without it.
7. subagent.md updated same-commit — mandated by AGENTS.md.

Watch

• Grep info.done = True under subagent_manager/: 13 sites — 11 armed at the flip, 1 justified report-free (cancellation.py:186), and 1 unfixed sibling: cancellation.py:127 (recovery-respawn failure) flips done then reaches its arm only at the report task's first step, after _run_terminal_report's create_task→await yield — the same done-but-unarmed window the six run.py arms close. The added spec sentence "EVERY terminal done transition … arms a hold … in the same synchronous block" overstates the diff; either arm that flip or shrink the spec claim to match.

[FIRST-PRINCIPLES-REVIEWED] 9ab40ff

[github-actions]

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

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

1 of 1 blocking finding(s) are security-class and were withheld from adjudication, so the blocking verdict stands.

BLOCKING -- src/kiro_crew/subagent_manager/terminal.py:127, admission.py:587 -- scheduled reports arm their holds too late
self._manager.arm_report_in_flight(info)
Recovery failure or synthetic rejection marks the member complete -> a ready sibling closes the wave before the scheduled report runs -> the delayed report emits a second digest.
Anchor: residual/crash-data-loss-corruption
Fix: arm synchronously at each completion transition, before scheduling its report or announcement.

[BLOCK-MERGE] 9ab40ff
[GPT-REVIEWED] 9ab40ff

Adjudication (Opus 4.8) — is blocking on each finding proportionate?

I have enough to rule. This is a single fenced finding (F1); my verdict set is UPHOLD-FENCED or FLAG, and FLAG requires a complete record that every condition combination reaching the defect is extreme.

The finding claims the hold is armed too late for synthetic-rejection paths. I confirmed against the diff and the base tree that the synthetic-rejection callers create SubagentInfo(done=True) and route to a deferred _safe_announce via asyncio.ensure_future — _announce_rejection_impl (admission.py:598-602) and record_lost_submission_impl (waves.py:106-110) — while the ONLY arm for those records is inside _safe_announce_impl (admission.py:587, the new-file arm site), which runs at the scheduled task's first step, after a yield. None of the six _announce_rejection call sites (admission.py:153, 202, 237, 270, 288, 320) received a synchronous flip-site arm in this diff (unlike the _run flip sites and the no-approval-mechanism paths, which do arm synchronously). The submitted count is already incremented at spawn (admission.py:130-131), so a governance / low-memory / invalid-cwd / empty-task rejection of one batch member while a sibling's report reaches the consumer in that yield window is an ordinary operational combination, not an extreme one — the wave-close fallback trips (no pending, hold not yet armed) and the late _safe_announce re-finalizes. I cannot build a rarity/impossibility record, so FLAG is unsupported.

Harm rung: matches the fence's unbounded classification (residual/crash-data-loss-corruption — duplicated wave digest). Conditions confirmed above at their file:line; recovery: the reaper digest-hold sweep is fenced off while a report is nominally in flight, and the double-finalize itself has no in-band suppressor on this path. When torn on a fenced item the rule is UPHOLD-FENCED; here the evidence actively defeats FLAG.

[ADJUDICATION] 9ab40ff74f53caeec2fb2d04c253bd19858ff5d4 total=0 uphold=0 downgrade=0
[GPT-ADJUDICATED] 9ab40ff74f53caeec2fb2d04c253bd19858ff5d4
[ADJUDICATION-FENCED] 9ab40ff74f53caeec2fb2d04c253bd19858ff5d4 fenced=1 flagged=0
UPHOLD-FENCED F1 src/kiro_crew/subagent_manager/terminal.py:127 -- Synthetic-rejection records (governance/low-memory/cwd/empty-task/lost-submission) are created done=True and reach a deferred `_safe_announce` whose arm at admission.py:587 runs only after the ensure_future yield, so an ordinary concurrent sibling completion trips the wave-close fallback and the late announce re-finalizes; conditions are common, not extreme.
[GPT-ADJUDICATED-FENCED] 9ab40ff74f53caeec2fb2d04c253bd19858ff5d4

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

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

Review details

One advisory finding — the PR arms every terminal done flip except the recovery-failed report path, leaving a narrowed (not closed) #8554 double-finalize window there. Nothing blocks.

FINDING — src/kiro_crew/subagent_manager/cancellation.py:127 — the recovery-failed arm flips info.done = True with no synchronous arm_report_in_flight; it then reports via await self._manager._run_terminal_report(...), whose report body (_report_terminal_impl) is the only arm and runs only after the await asyncio.shield(task) yield — so during that yield a near-simultaneous sibling completion reads this member as neither pending (batch_members_pending False) nor in-flight (batch_reports_in_flight False, unarmed), trips the wave-close fallback the PR added at gateway.py:7467-7470, finalize_batches the wave, and the recovery report then re-setdefaults the batch-progress record and finalizes the same wave a second time (the #8554 double-digest the PR narrows everywhere else). Advisory, not blocking: line 127 is not a changed line and the PR strictly shrinks (never widens) this pre-existing window → Fix: add self._manager.arm_report_in_flight(info) immediately after info.done = True at line 127, mirroring the run.py failure-path and reap flip sites; the report body's arm then stays an idempotent no-op and _forget/the structural finally release the hold as usual.

[OPUS-REVIEWED] 9ab40ff

[bolichen97]

@javenciu Thanks for staying on this. First, nothing here has been overtaken by main. origin/main has no batch_reports_in_flight, arm_report_in_flight, consume_report_hold or _reports_in_flight; the consumer's wave-close fallback is still not batch_members_pending(_batch_id) alone in src/kiro_crew/slack/gateway.py, and batch_members_pending_impl still drops a member at the done flip in src/kiro_crew/subagent_manager/waves.py. The double-finalize race from #8554 is still live, and no other open PR implements this hold, so please keep the PR open and its current scope.

What remains before it can land:

1. Two review lanes block on terminal flips that do not arm a hold. The synthetic-rejection path in src/kiro_crew/subagent_manager/admission.py arms only after the ensure_future yield, and the recovery-failed flip at src/kiro_crew/subagent_manager/cancellation.py:127 is not armed at all. Your head diff does not touch cancellation.py, so the "every terminal done transition arms a hold" claim in the body and in docs/system-specs/modules/subagent.md is not true yet.
2. The branch is 106 commits behind main and labelled readiness: action required. Please rebase.
3. The body's "What changed" and "Tests" sections predate the head; they omit the per-flip arms in src/kiro_crew/subagent_manager/run.py and the two strand-guard done-callbacks.
4. Please coordinate with feat: report subagent credit usage alongside elapsed time #8003. It inserts at the same info.done = True anchor in src/kiro_crew/subagent_manager/terminal.py that you relocate into _report_terminal_guarded_impl, so whichever lands second must re-apply its hunk inside the other's structure, including the inspect.getsource pin in test/test_subagent_scale.py.

Posted from the 2026-09-08 open-PR relationship audit (read-only, one auditor per PR); reply here if any of this is wrong.