fix(subagent): take a run's three state.json writes off the event loop

[chenmingwei23]

1. What is the problem?

subagent_persistence.update_state reads state.json, merges the caller's
fields and rewrites the whole file, with a blocking _atomic_write (fsync +
rename) between the read and the write.

Three of a run's writers called it directly from an async def body, so that
fsync ran on the gateway's only event loop:

Site	Fields
subagent_manager/run.py _run_inner (PID record)	pid, pid_recorded_at
subagent_manager/run.py _run_inner (session record)	session_id, provider, keep, conversation_key, cwd
subagent_manager/run.py _create_shared_session	pid, pid_recorded_at

That is the no-blocking-call-on-event-loop class #6288 names. The divergence is
what makes it easy to miss: PR #7467 moved every OTHER state writer inside a run
through _write_state_off_loop, and the PID record sits about thirty lines below
one of them, reading almost identically at the call.

2. Why this issue matters to the user

The gateway runs chat turns, the reaper and the heartbeat on that one loop. A
state.json write is small, but its cost is not bounded by its size: it is a
read plus an fsync plus a rename against whatever filesystem the config
directory lives on. On a network mount, a snapshotting volume, or a disk under
load, every one of those calls stalls the loop for as long as the FS takes --
which means an unrelated chat turn stops mid-stream and the heartbeat misses its
window, three times per subagent spawn.

It also left an ordering hole. update_state serializes its read-merge-rewrite
per agent for off-loop callers only (#7280); an on-loop caller deliberately takes
no lock, because waiting on a pool thread's fsync from the loop is the very thing
the anchor forbids. So these three writes were unordered against any pool writer:
because the rewrite is whole-file, a writer that read first and wrote last rolls
back every field the other landed, not just the fields it names. A lost pid
means the reaper can no longer reach the child -- a leaked kiro-cli process
holding its memory for the life of the gateway.

3. How our fix solves it

The chain is: on-loop write -> fsync on the loop, and no per-agent lock -> an
unordered whole-file rewrite. Moving the caller off-loop closes both links at
once, because update_state takes the lock precisely when the caller is not on
the loop.

All three sites now await self._manager._write_state_off_loop(info, "<label>", **fields) -- #7467's helper, unchanged. No new mechanism, no per-site flags:

• Off-loop puts the fsync in a pool thread, so a slow FS can no longer freeze
  the loop.
• Drained on cancellation is what makes going off-loop safe rather than a
  trade: cancelling a to_thread await detaches the worker without stopping it,
  and a detached worker is exactly the stale whole-file writer described above.
  The helper holds cancellation open until the worker lands (bounded at 5s), and
  on expiry marks the run so its conversation stays held.
• The lock now covers these writes, so they are ordered against every other
  pool writer.

Why the two remaining on-loop callers are safe against these three. The other
update_state callers on the loop are the retention writers,
_promote_conversation (keep=True) and release_conversation (keep=False).
The session record carries keep too, so the interleave question is real -- and
answered by the gate #7467 already leaned on: both retention writers are reached
only through _conversation_busy, which refuses while a run is not done. All
three sites here run inside _run_inner / _create_shared_session, i.e. while
the run is live, so a promote or release cannot execute concurrently with them.
info.conversation_key is assigned at construction (continuation.py:328 for a
continuation, admission.py:448 for a spawn), so the gate cannot miss a live
continuation run either.

One behaviour is deliberately preserved rather than tidied: the
_create_shared_session site has no except Exception around it, and still does
not. That method has no best-effort contract, so an _atomic_write failure keeps
propagating to the caller instead of yielding a session with no recorded pid. At
the two _run_inner sites the existing except Exception stays correct because
it does not catch CancelledError, so the helper's post-drain re-raise
propagates instead of being swallowed.

This also makes an existing docstring true: _write_state_off_loop already
claimed "every state.json writer inside a run goes through here", which these
three sites falsified.

Scope. #7302 lists five on-loop callers. This PR moves the three in run.py
-- the self-contained half, and the issue's own suggested first step -- so it is
Refs, not Closes. The two continuation.py retention writers are left, with
the reasons recorded on the issue: they are synchronous functions whose callers
would all have to become awaitable; release_conversation's other work includes
a _cleanup_session_files_sync unlink loop that is blocking too, so offloading
only its 1 ms write would be a half-measure; and its SessionMap mutation is
required to stay on the loop by the Arbiter contract in continuation.py. Those
are design decisions, not substitutions.

4. What tests we did

Five new tests in test/test_subagent_provenance_write.py, all mutation-verified
-- reverting only the two src/ files to base makes all five fail in 7s, while
the three pre-existing #7467 drain tests stay green:

• test_pid_and_session_records_are_written_off_loop -- drives _run_inner with
  the REAL asyncio.to_thread and asserts neither write ran on the loop thread.
  On base: state.json write(s) ran on the event loop: PID record, session record.
• Three drain tests, one per moved site, each gating its own write, cancelling
  mid-flight and asserting the task does not complete, the recovery-gate latch is
  raised, the write still lands, and the latch does not leak.
• test_no_on_loop_update_state_inside_a_coroutine -- a static AST gate, sibling
  to fix(subagent): drain every off-loop state.json writer on cancel #7467's test_no_bare_to_thread_update_state_outside_the_drained_helper.
  That one pins HOW an off-loop write is performed; this one pins WHERE a write
  may happen at all. Scope-aware (innermost frame only), so a synchronous helper
  nested inside a coroutine is correctly not an offender.

The three drain tests bound their gate wait at 5s rather than awaiting it
unbounded. Unbounded, a regression turned each one into a 120s pytest-timeout
with no diagnosis -- six minutes of a shared runner for one mistake. Bounded,
base fails in 7s total with the cause in the message.

Gates on the touched files: 338 tests green across the affected families
(test_subagent, test_session_sharing, test_subagent_persistence,
test_subagent_state_write_serialization, test_subagent_provenance_write,
test_subagent_continuable, test_subagent_manager_boundaries,
test_subagent_reap_race, test_stop_kill_cancel,
test_subagent_stall_activity_scope); flake8, isort and the baseline black gate
clean. Two test_acp_backend_kas.py::TestNoImportCycle failures are
worktree-environment, not the diff: they spawn a bare python -c "import kiro_crew" subprocess and this worktree has no editable install
(ModuleNotFoundError: No module named 'kiro_crew'); both pass with
PYTHONPATH=src. The two mypy errors reported are in
src/kiro_crew/transcribe.py, which this commit does not touch.

5. Any other suggestions on the work

• The interesting residue is not in this diff: release_conversation runs
  _cleanup_session_files_sync on the event loop, an unlink loop over provider
  session files. It is a strictly larger blocking call than the update_state
  subagent: move the five on-loop update_state callers off the event loop #7302 was filed about, and subagent: move the five on-loop update_state callers off the event loop #7302 does not mention it. Worth folding into the
  continuation half rather than discovering it there.
• _write_state_off_loop is keyed on a live SubagentInfo
  (_state_drain_active, _cancel_retry_used, _abandoned_state_writers by
  info.id). The two retention writers only hold a conv_id and may have no
  live run, so they cannot reuse the helper as-is; whoever does that half needs a
  hold/latch decision of their own. Noting it so it is not mistaken for a
  mechanical swap.

Pattern harvest

Rule candidate: an AST/semgrep gate on call POSITION, not just call form -- a
blocking call (one ending in a synchronous fsync, unlink, or rename) must
not be invoked directly from an async def body.

It is already in this PR as a test. The defect class is "a
blocking call reached from a coroutine body", and the reason five triage passes
missed it is that the offending line is textually identical to a legitimate one
in the same file -- convention cannot separate them, position can.
test_no_on_loop_update_state_inside_a_coroutine walks the AST of
src/kiro_crew and fails on any direct update_state(...) whose innermost
enclosing frame is an async def. It is deterministic and false-positive-free
for this symbol, because an off-loop write has exactly one legitimate form (the
helper's own to_thread, which is an argument and not a call).

The generalization worth taking: the repo already has
test_no_blocking_call_on_loop.py and #7467 added a HOW gate for this symbol.
The missing shape was a WHERE gate. Any function that ends in a synchronous
fsync or unlink deserves one at the point it becomes reachable from a
coroutine -- which would have caught these three sites the day they were written,
and would catch _cleanup_session_files_sync today.

Refs #7302
Refs #6288
Refs #7467

[github-actions]

Design Review (Fable 5) — ✅ PASS

Design-level review of eed515fd1992f3a3cede102f200ac8189ee4b0cc — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

The change is a targeted application of an existing, well-documented pattern (_write_state_off_loop, from #7467) to three call sites that were missed, with the persistence docstring updated in the same commit and mutation-verified tests including a positional AST gate. The interleave reasoning (why the two remaining on-loop retention writers cannot race these sites — the _conversation_busy gate refuses while a run is live) is stated in the description and consistent with the code I read. Scope is deliberately half of #7302 with reasons recorded. No design-level findings survive the kill-filter.

Design-Verdict: PASS

Three missed writers moved onto an existing, drain-safe helper; both the blocking-loop and lock-ordering harms close with no new mechanism.

[DESIGN-REVIEWED] eed515f

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of eed515fd1992f3a3cede102f200ac8189ee4b0cc and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] eed515f

False positive or not applicable? A repository writer can comment:
/ai-review override gpt eed515fd1992f3a3cede102f200ac8189ee4b0cc: <one-sentence reason>

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed eed515fd1992f3a3cede102f200ac8189ee4b0cc — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] eed515f

Verdict parsed from the review's SHA-scoped output markers for commit eed515fd1992f3a3cede102f200ac8189ee4b0cc.

False positive or not applicable? A repository writer can comment:
/ai-review override fable eed515fd1992f3a3cede102f200ac8189ee4b0cc: <one-sentence reason>

[github-actions]

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of eed515fd1992f3a3cede102f200ac8189ee4b0cc — 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 verification done. The claims in the description check out against the repository: the helper pre-exists, the two remaining callers are synchronous retention writers as stated, and the new AST gate covers a distinct offense class from the existing #7467 gate.

First-Principles-Verdict: PASS

Three on-loop fsyncs move onto the one existing drained helper; every item is the fix or its pin, siblings counted and declared.

What this change ships

Intent: stop a subagent spawn from stalling the gateway's only event loop (and losing state to unordered whole-file rewrites) three times per run — a FIX.

1. PID record in _run_inner now written off-loop, lock-ordered — justified (the fix; subagent: per-turn update_state runs os.fsync on the event loop #6288/subagent: move the five on-loop update_state callers off the event loop #7302).
2. Session record in _run_inner now written off-loop — justified (same defect, carries the contended keep).
3. Shared-session PID record now written off-loop, failure still propagating — justified (third named site).
4. update_state docstring now names the two remaining on-loop callers — justified (same-commit doc sync, mandated).
5. CI now fails any direct update_state call in a coroutine body — justified (declared; pins the cause).
6. Five tests: thread-identity, three drain contracts, the gate — declared.

Verified counts: direct update_state( callers in src/ after this change: 2 (continuation.py:108, :712), both synchronous defs (_promote_conversation_impl, release_conversation_impl), so the deferral reasons in the description hold — accepted-and-deferred on #7302, not a point patch. The new gate is not a second spelling of test_no_bare_to_thread_update_state_outside_the_drained_helper (test/test_subagent_provenance_write.py:936): that one catches undrained to_thread(update_state, ...), this one catches calls with no to_thread at all — disjoint offense classes, and the existing gate provably missed these three sites. No new public surface, config key, or flag ships; the diff adds call sites to a helper with 5 pre-existing consumers.

[FIRST-PRINCIPLES-REVIEWED] eed515f

[bolichen97]

Tier 1 auto-approve: fix (3 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: moves the subagent run's PID-record and session-record update_state writes off the event loop via _write_state_off_loop (fsync no longer runs on the gateway's only loop; #7302), with a bounded regression gate.

[iamwhatever]

Tier 1 auto-approve: fix (3 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: the subagent run's PID and session state.json writes were calling update_state (which ends in a synchronous fsync) directly from an async body on the shared gateway loop; they now go through the existing drained _write_state_off_loop helper, which also makes them take the per-agent lock on-loop callers skip.

[bolichen97]

Tier 1 auto-approve: fix (3 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: three update_state writes inside a subagent run were fsyncing on the gateway's only event loop (the #6288 class); each is routed through the existing _write_state_off_loop helper, which also means the write now takes update_state's per-agent lock that on-loop callers skip, so it can no longer interleave with another pool writer's read-merge-rewrite.

[dwu96]

Tier 1 auto-approve: fix (3 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: clear root cause — three of a run's state.json writers called update_state directly from async def bodies, so its synchronous fsync ran on the gateway's only event loop and the write skipped update_state's per-agent lock; all three now route through the existing #7467 _write_state_off_loop helper, inheriting its bounded cancellation drain. No new mechanism.