fix(subagent): serialize state.json rewrites for off-loop writers

[chenmingwei23]

1. What is the problem?

update_state (src/kiro_crew/subagent_persistence.py) reads state.json, merges the caller's fields and rewrites the whole file. Between the read and the write sits a blocking _atomic_write (fsync + rename). Nothing serialized two writers on the same agent_id, so the second writer's read could predate the first writer's write -- and its rewrite then restored a stale whole-file snapshot. Losing the other writer's fields is the visible half; silently rolling back fields neither writer touched is the damaging half.

One run writes state from both sides at once:

Thread	Writers
event loop	retention keep (subagent_manager/continuation.py), PID, session id + provider, shared-session PID
thread pool	pre-spawn model provenance, CC-path model refinement, per-turn diagnostics -- each await asyncio.to_thread(update_state, ...)

The three pool writers overlap each other during a run: CC-path refinement fires on the first text chunk and the diagnostics write fires on every tool event, while the provenance write's own bounded retry can still be in flight. Cancellation widens the window further -- cancelling a to_thread await detaches the worker rather than stopping it, so it finishes carrying a read that is already stale.

This PR closes the pool-vs-pool half of that. See section 3 for the deliberate scope split and the Known limitation below it for what stays open.

2. Why this issue matters to the user

The fields that get rolled back are the ones recovery depends on. pid and session_id are what orphan recovery and spawn_continue read from disk to find a run again; turns and last_tool are the orphan-recovery diagnostics; resolved_model is the run's served-model provenance (#5394).

Concretely, within the pool-vs-pool half this PR fixes: the per-turn diagnostics write and the CC-path model refinement both fire during a normal run. Unserialized, whichever renames second restores its own earlier snapshot -- so a run can end up with diagnostics that never advanced, or with resolved_model reverted to the spawn-time value, or with a field written by an earlier pool writer simply absent. Nothing logs and the file stays valid JSON; the fields are just not there.

3. How our fix solves it

Symptom -> cause chain:

1. A field written by one pool writer disappears, or state reverts to an earlier snapshot.
2. Because update_state rewrites the whole file from a snapshot it read earlier, so its write carries stale values for every key it was not asked to change.
3. Because the read and the write are separated by a blocking fsync, leaving a wide window.
4. Because nothing serialized two callers on the same agent_id -- the root.

The fix closes (4) for off-loop callers, which makes (3) and (2) harmless between them: each pool writer's read now observes its predecessor's write, so a merge cannot resurrect a pre-write snapshot.

Scope split -- off-loop writers only. Serializing on-loop callers too would mean the event loop waiting on a pool thread's fsync, which is the repo's no-blocking-call-on-event-loop anchor. An earlier revision of this PR bounded that wait; review was correct that bounding shrinks the violation without removing it, and that on expiry the caller pays the wait and writes unserialized. So the lock is now taken only by callers not on the loop, where blocking is legal, and on-loop callers keep exactly their pre-existing behaviour. The finding is structurally unobservable rather than tuned: no event-loop caller can wait on this lock, and no on-loop caller is ever a holder.

Two consequences worth naming:

• No timeout, no fail-open path. Because only pool workers ever block here, and their own write (read + fsync + rename) already exposes them to a wedged FS, waiting on the lock adds no parking risk the write did not already carry. That deleted the bounded budget, the unserialized-write warning, and its test -- the mechanism review objected to is gone, not adjusted.
• The seam is a running-loop check, not a caller flag, because update_state takes **fields: any keyword flag would be indistinguishable from a state field a caller means to persist. asyncio.to_thread / run_in_executor threads have no running loop and so serialize; a synchronous call from a coroutine does and so does not wait. The documented returns whether the merge was written contract is untouched, so the Subagent model provenance: conservative downgrade heuristic + drop redundant spawn persistence write #5394 provenance retry still sees a skipped merge.

The two open questions triage flagged are both answered in-tree, so this mirrors an established pattern rather than inventing one:

• Lock nature. A per-key threading.Lock registry (dict + guard lock) is what this repo already uses to serialize file read-modify-write in three places: learn._lock_for, artifacts._lock_for_root, history.ConversationLog._file_locks. learn.py documents the same in-process-only reasoning verbatim. A filesystem lock is the tool for cross-process contention (session_ledger._locked, history's flock layer) and there is none here -- every update_state caller lives in the gateway process that owns the run.

• Nobody evicts: the registry is self-cleaning. Values are held weakly and every caller keeps a strong reference for the length of its critical section, so an entry lives exactly as long as some writer is using it and then disappears on its own. Keyed per agent, so unrelated runs never queue behind one agent's fsync, and agent ids are per-run uuid4().hex[:8], so nothing accumulates.

  This is a correctness property, not tidiness. Removing an entry explicitly while another writer still holds or is queued on it splits the lock's identity -- the next caller mints a fresh lock and enters state.json alongside the writer still inside it, which is precisely the loss this lock exists to prevent. Earlier revisions of this PR carried three such hooks (delete_agent_folder, prune_stale_tombstones, and a read-failure path) and review found the class twice; all three are now gone. The pruner's case is not hypothetical: _atomic_write calls mkdir(parents=True), so a writer already inside its critical section re-creates the folder the pruner just removed, and the pruner holds no lock so it cannot know. Weak values make the whole class unrepresentable rather than guarded case by case. (threading.Lock cannot be weakly referenced, hence the one-field _AgentLock wrapper the registry stores; callers must retain the wrapper, not just .lock.)

  A write for an id whose folder is already gone still creates an entry -- _lock_for_agent runs before the read that discovers the folder is missing -- but it evaporates when that writer returns, so no hook is needed and none exists.

Known limitation

An interleave with one ON-LOOP participant is still open, unchanged from before this PR. Specifically:

• the loop-side retention keep write against an in-flight pool write -- the defect subagent: retention keep-write at subagent.py:3825 runs update_state on the event loop unguarded, can clobber an in-flight to_thread write #6298 is titled for;
• a detached pool worker against the PID / session-id writes a cancel-respawn recovery run makes on the loop (subagent_manager/run.py) -- the scenario subagent: sibling to_thread update_state writes lack the cancellation drain; update_state is an unlocked read-merge-replace #6308's drain defends against.

Neither is closed here, because the on-loop participant does not take the lock. Both drain comments in run.py were corrected to say so, rather than claiming a guarantee this PR does not deliver: the whole-file rollback that the #6306 shield-and-drain exists to bound is still reachable.

Closing that half means moving the loop-side callers off-loop -- #6288's defect class, which #6306 deliberately scoped to a single site and which five triage passes on #6298/#6308 parked as needing a human ruling. It is filed separately rather than bundled in here. Because of this, the trailers are Refs, not Fixes: both issues' titled defects name an interleave with an on-loop participant, so neither is closed by this PR. Noted on both issues.

4. What tests we did

New file test/test_subagent_state_write_serialization.py (8 tests). Each race test parks one writer between its read and its write and drives the other writer meanwhile, with a bounded park so the test terminates instead of deadlocking. The two split tests are a matched pair: an on-loop caller must NOT wait on a held lock, and the same call from a plain thread MUST wait -- so a change that made every caller skip the lock cannot pass either.

The pool-vs-pool tests were verified RED before the lock existed, failing on the mechanism rather than a missing symbol (a rolled-back keep, a reverted pid, and writers lost their fields: ['field_0', 'field_1', 'field_3', ...] for 8 barrier-released writers).

Mutation matrix, each reversion reddening its own axis and nothing else:

Mutation	Result
no serialization at all	5 failed / 3 passed
collapse the registry to one global lock	2 failed / 6 passed
serialize EVERY caller (the off-loop split removed)	1 failed / 7 passed
strong-ref registry instead of weak (never self-cleans)	1 failed / 7 passed
restored	8 passed, file byte-identical

The global-lock mutation pins the lock as genuinely per-agent: one process-wide lock also passes the race tests while making every unrelated run queue behind one agent's fsync. The split-removed mutation pins the split itself -- and the on-loop test is deliberately built with a separate holder thread on a timed release, because holding the lock on the test's own thread would make that mutation deadlock instead of fail, which is not a usable signal. The strong-ref mutation pins the self-cleaning registry from one side; a matched test pins it from the other, asserting the entry SURVIVES while a queued writer still references it (including across a mid-flight folder delete), which is the property that makes explicit eviction both unnecessary and unsafe.

Targeted suites, 806 passed, 0 failed (630 + 176 across two batches) -- every test file that references subagent_persistence, plus the adjacent subagent suites: test_subagent, test_subagent_persistence, test_subagent_state_write_serialization, test_subagent_provenance_write, test_subagent_continuable, test_continuable_followups, test_session_cleanup, test_subagent_delivery_ttl_anchor, test_subagent_reap_race, test_subagent_resolved_model, test_subagent_manager_boundaries, test_subagent_coverage, test_lazy_data_home_paths, test_members, test_subagent_context_group_plumbing, test_subagent_error_detail, test_subagent_result_path_spelling, test_subagent_scale, test_ci_surface_tests.

Gates: isort, flake8 and black clean on every changed file. mypy reports 2 errors, both in transcribe.py, a file this diff never touches (boto3-stub env drift).

Pattern harvest

Rule candidate: semgrep
Pattern: whole-file read / merge / rewrite with no serialization on the record's key

The defect is not a typo and it is not the first of its kind here. The shape is a function that reads a JSON document, merges caller fields into the parsed dict, and rewrites the whole file, with a blocking fsync between the read and the write and no lock keyed on the record. Any second writer's rewrite then restores a pre-write snapshot, so the loss is not confined to the fields either caller passed. This repo has hit the family repeatedly: #6017 and #6290 on the session ledger, #5244 (config.json materializes every key, so a stored value beats a changed default), #4955 (the kirocrew.json rebuild reads back its own computed values as if user-authored). Three modules already carry the per-key-lock remedy (learn._lock_for, artifacts._lock_for_root, history.ConversationLog._file_locks) and two carry the filesystem-lock variant (session_ledger._locked, history's flock layer). The remedy is settled; it is the detection that is missing.

A useful rule flags a function that both reads a path (read_text / json.loads) and writes that same path, where the enclosing module defines no lock guarding the pair. The signal to key on is the round trip through a parsed dict, not the write primitive: _atomic_write makes each individual write atomic, which is exactly what makes the missing serialization easy to mistake for handled. Two refinements keep it honest -- exempt a writer that never reads first (create_agent_folder builds a fresh document and cannot lose a merge), and require the lock to be per-key rather than module-global, since one process-wide lock is a correct-but-costly answer a naive check would accept.

5. Any other suggestions on the work

• create_agent_folder is not locked. It writes a fresh state.json for an id that has no other writer yet, so it cannot lose a merge. Worth folding in if the invariant is ever stated as "every state.json write serializes" rather than "every read-modify-write serializes".
• Cross-process is still last-writer-wins. A threading.Lock does not span processes. No caller outside the gateway writes subagent state today, so this is a note on what the fix does not claim, not a gap -- learn.py carries the same caveat.
• The split is invisible at the call site. A reader of continuation.py cannot tell that its update_state call is the unserialized one. If the loop-side offload does not land soon, an explicit named wrapper for the on-loop callers would make the asymmetry legible.
• delete_agent_folder has no production callers (First Principles noted this; grep confirms tests and docs only). This PR no longer touches it at all, but the dead function itself is worth removing -- out of scope here.

Refs #6298
Refs #6308

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] fabe2ce

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

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

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

A real lost-write class closed at its root for the only callers allowed to block, with the remaining half honestly scoped, tracked, and mutation-pinned.

Suggestions

• The lock/skip split is decided by an invisible runtime probe (_on_event_loop()), so a future update_state call added inside a coroutine silently joins the unserialized class with nothing going red; the PR's own "explicit named wrapper for the on-loop callers" idea is worth doing now rather than only if the subagent: per-turn update_state runs os.fsync on the event loop #6288 offload stalls — it makes the asymmetry legible at every call site and turns a new on-loop writer into a reviewable decision instead of a silent one.

[DESIGN-REVIEWED] fabe2ce

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] fabe2ce

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

[github-actions]

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of fabe2ced7199f52e01704c7ba91f3069012fba7c — 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 claims verified: the three pool writers (run.py:710, run.py:999, run.py:1097), the on-loop writers (run.py:745,787,1634, continuation.py:90,684), the three pre-existing lock registries, and the absence of unlocked read-modify-write siblings in subagent_persistence.py (tombstone and folder-create are fresh writes, not merges). Final review:

First-Principles-Verdict: PASS

Serializing off-loop state.json writers fixes a nameable field-rollback defect at its cause, with the deliberately-unfixed half declared and tracked.

What this change ships

Intent: stop concurrent background writers on one subagent from silently rolling back each other's state.json fields — a FIX.

1. Two pool writers on one agent no longer erase each other's fields — justified; the fix, derived from the subagent: retention keep-write at subagent.py:3825 runs update_state on the event loop unguarded, can clobber an in-flight to_thread write #6298/subagent: sibling to_thread update_state writes lack the cancellation drain; update_state is an unlocked read-merge-replace #6308 defect class, writers verified at run.py:710,999,1097.
2. On-loop writes stay unserialized; loop-vs-pool rollback remains reachable — declared, mandated by the repo's no-blocking-call-on-event-loop anchor.
3. New per-agent weak-valued lock registry (_STATE_LOCKS/_AgentLock) — justified; meaningfully different from the 3 existing registries (per-run uuid keys would leak forever in learn.py's never-evicting dict, and eviction hooks are the bug class the description documents removing).
4. run.py drain comments corrected to stop claiming a guarantee the lock doesn't give — rides along, but same-commit doc accuracy is a documented invariant.
5. New regression tests pinning both halves of the seam — justified; they pin the seam itself, not incidentals.

Watch

• This is now the fourth per-key lock-registry spelling in-tree (counted, grep _lock_for|_file_locks: learn.py:54, artifacts.py:999, history.py:1058, plus this). Each has a reasoned semantic difference, but one shared helper is the general fix — genuinely larger than this change, accepted-and-deferred.

[FIRST-PRINCIPLES-REVIEWED] fabe2ce

[chenmingwei23]

Escalating a verified design fork -- no override taken

GPT 5.6 has blocked twice on the same anchor (no-blocking-call-on-event-loop) with the same prescribed remedy ("revert the locking hunk until loop-side callers can offload"). I have not used /ai-review override and am not going to. Instead, here is what I verified, why I think neither of the two remedies on the table is mine to pick, and what a ruling would look like.

GPT's premise is factually correct

I went looking for evidence that the loop-side callers were already off-loop. They are not. The retention writes reach update_state from three distinct event-loop entry points:

• src/kiro_crew/subagent_manager/monitoring.py:439 -- the reaper calls _sweep_conversations(now) inline on the loop. Notably it offloads _sample_live_costs to maintenance_executor() two blocks earlier with the comment "Off the event loop: ... the reaper shares the loop with every chat turn and heartbeat", so the inline call here is not an oversight I can quietly reclassify.
• src/kiro_crew/dashboard/handlers/messaging.py:419 -- async def api_spawn_release -> release_conversation -> update_state(conv_id, keep=False).
• src/kiro_crew/subagent_manager/continuation.py:169 -- an in-tree comment states continue_conversation is "also on-loop", and messaging.py:337 says it is called synchronously on purpose.

So a bounded acquire does add a wait of up to the budget on real loop paths. GPT's round-2 sharpening is also fair: on expiry the caller pays the wait and writes unserialized, which is the worst cell of the matrix rather than a clean degradation.

Why neither remedy is mine to choose

Revert leaves #6298 and #6308 open. The defect is real and unfixed on main: update_state rewrites the whole file from a snapshot it read earlier, so a second writer's rewrite restores a pre-write snapshot and rolls back fields neither caller passed -- pid and session_id (what orphan recovery and spawn_continue read from disk) and keep (the single persisted record of retention intent, #1115). Reverting to satisfy the anchor trades a silent data-loss bug for a latency property.

Offloading the loop-side callers is the durable answer to the anchor, and after this PR it is safer than it was -- the lock removes the whole-file rollback, so a detached to_thread worker can now only carry its own stale fields. But it means making continue_conversation / release_conversation awaitable, which reaches crew_chat.py, the dashboard release handler, and the reaper sweep. That is #6288's defect class, which #6306 deliberately scoped to a single site, and which the #6298/#6308 triage passes repeatedly parked as needing a human ruling (including the per-site question of whether a drain expiry there should burn the one-shot cancel-retry). Expanding into it mid-review, on a fix-only pass, is the thing those triage notes asked not to happen by default.

There is a third shape I considered and rejected as too large to slip in unreviewed: on the loop, acquire non-blocking and on contention defer the write to asyncio.to_thread as a background task. The loop never waits and the lock still serializes, but it silently changes update_state's contract from "returns whether the merge was written" to "may not have happened yet" -- and the pre-spawn provenance retry (#5394) depends on that return value.

Where this leaves the PR

Every other lane passes on bb18ae845: Opus 4.8 (no findings -- it evaluated this exact candidate and dropped it, reasoning that the loop already fsynced there unboundedly so the lock adds a bounded wait behind one short critical section), Design Review PASS, First Principles Review PASS, UX PASS, PR Hygiene PASS. 42 checks pass, 1 fails, 0 other failures.

A maintainer ruling is what unblocks this. The options as I see them:

1. Keep the bounded lock as-is and override GPT on the ground Opus already stated. Fixes both issues now; the anchor violation is a bounded wait on a path that already blocks unboundedly.
2. Authorize the offload as part of this PR, accepting the widened blast radius, and decide the per-site cancel-retry question.
3. Split it: land the lock for the pool-side writers only and file the loop-side offload as its own issue with the rulings decided up front.

I am standing down rather than grinding a third identical round. Happy to implement whichever of the three you pick.

[github-actions]

UX Review (Fable 5) — ⏭️ skipped

Revision fabe2ced7199f52e01704c7ba91f3069012fba7c touches no user-facing surface (no changes under website/ or committed screenshots), so the UX review was skipped. Advisory — does not block merge.

[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: serializes subagent_persistence.update_state's read-merge-replace with a per-agent weakly-held threading.Lock taken by off-loop writers only, so two thread-pool writers on one agent_id can no longer interleave across the blocking atomic write and roll back state.json fields neither of them touched; on-loop callers keep their existing unserialized behaviour, so no new blocking call lands on the event loop.

[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: serializes update_state's read/merge/rewrite per agent for off-loop callers so two thread-pool writers can no longer restore a pre-write state.json snapshot — clear root cause (nothing serialized two writers on one agent_id), two source files plus one test.

[bolichen97]

Tier 1 auto-approve: refactor/fix — per-agent write serialization (3 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean (Semgrep + CodeQL both success, 0 annotations, 0 PR-scoped CodeQL alerts), security checklist all-NO, AI reviewers green. Category: fix (3 files, clear root cause — two off-loop pool writers interleave update_state's unserialized read-merge-rewrite on state.json and roll back each other's fields; adds an in-process per-agent threading lock keyed by agent_id, off-loop callers only, on-loop behaviour unchanged).