perf(dashboard): take the last blocking transcript reads off the event loop

[chenmingwei23]

Problem / Motivation

ConversationLog.read_messages / read_messages_chained open a transcript,
read it whole and JSON-parse every row: 100-300 ms on a large store. Five call
sites still did that on the asyncio event loop, so while one of them ran, every
other session's frames waited. A gateway log on an 810-session, 644 MB store
shows event-loop heartbeat: lag 6.1s (loop was blocked).

Issue #7408 lists six lines. Verified against main bb01943c1, four of the
six are already off the loop -- the offload lives at their callers, which is
exactly why the lines look un-offloaded when read on their own -- and the real
remaining exposure includes four caller sites the issue does not list. The set
this PR fixes is therefore five sites, not six:

site	on the loop on main?	why
dashboard/chat_runner.py:5877	yes	direct read in the body of async def _run_chat
dashboard/handlers/messaging.py:2135	yes	async def api_send_message called the sync _rehydrate_slot_from_history
slack/gateway.py:3239	yes	async def _deliver_script_result, same substitution
slack/gateway.py:4581	yes	dedup-suppressed inject passes no history=, so cron_inject.py:98 reads on the loop
slack/gateway.py:4605	yes	silent-cron inject, same omission (the third call in that function already prefetches)
dashboard/session_transfer.py:173	no	sole caller _read_and_assemble is dispatched by await asyncio.to_thread(...)
dashboard/chat_persistence.py:1009	no	it is the else arm of a _prefetched_messages ternary; rehydrate_slot_from_history_async prefetches off-loop
dashboard/channel_slots.py:751	no	await loop.run_in_executor(None, _load_messages)
dashboard/handlers/artifacts.py:755	no	both callers run under _run_off_loop (run_in_executor(subprocess_executor(), ...))
dashboard/cron_inject.py:98	the cause	the optional history= that let the two gateway callers leave the read here; now deleted

api_session_detail is covered by #7404 and deliberately untouched here.

Why it matters

Every affected path is one a user is waiting on. The prompt-submit read runs on
the hottest path there is -- the keystroke-to-first-token path of every new
turn. The two rehydrate sites fire when a cron or an MCP send_message delivers
into a session whose tab is cold, which is precisely when the transcript is
large enough to be slow. A stall long enough to cross
dashboard.loop_stall_exit_after_secs (25s) does not merely feel slow: the
loop-stall watchdog kills the gateway.

What changed (motivation -> approach -> change)

The convention this codebase already uses for the sync/async boundary is
neither "async-ify the helper" nor "wrap the helper's own line": it is
prefetch in the async caller and pass the data in, leaving the sync
signature and its sync callers untouched. That is what _prefetched_messages=
on _rehydrate_slot_from_history and history= on
inject_cron_result_to_dashboard exist for. No signature changes were needed
for the rehydrate half, so the ripple through slack/gateway.py and the test
surface the issue worried about does not arise.

• chat_runner: the re-injection probe's read_messages now goes through
  asyncio.to_thread. mem_count is counted after the hop, so both sides of
  the comparison read post-await state.
• handlers/messaging.py and slack/gateway.py: both cold-slot lookups now
  await rehydrate_slot_from_history_async, which already exists, prefetches
  the same three reads in a thread, and re-checks the tab-close and deletion
  races on return (returning None, which both callers already handle). The
  now-unused sync imports are dropped from both modules.
• cron_inject.py gains prefetch_cron_history, and the two silent /
  dedup-suppressed gateway sites await it. It reads off the loop and skips
  the read entirely when the slot is already linked -- the state in which the
  injection does not consume history. That guard matters: these two sites fire
  on every suppressed and every silent run, and copying the third site's
  unconditional prefetch would have added a whole-transcript parse whose result
  is discarded.
• The cause is removed, not just its five instances. Those two sites could
  omit history= only because inject_cron_result_to_dashboard -- a
  synchronous function whose every caller is async -- read the transcript for
  itself when the kwarg was absent. That fallback is deleted and history is
  now a required keyword parameter, so a forgotten prefetch is a TypeError at
  the call site instead of a silent loop stall in production. Adopted from the
  First Principles lane's subtraction; it replaces a spec sentence with a
  signature. All four in-repo callers already passed the kwarg, so no production
  call site changed; the two test files that drove the function synchronously
  now do the read in a local _inject helper, which is where a blocking read
  legitimately belongs.

gateway.py:4643 and handlers/cron.py:635 keep their own inline prefetch:
they are already off the loop, and reworking a correct path is not this PR's
business (see the deferral comment on this PR).

Tests

Every new test asserts the thread the read ran on, not the name of the
function called, so a later rename cannot quietly reopen the defect. All three
off-loop tests were confirmed red with only the source changes reverted.

• test_chat_runner_coverage.py::TestPromptSubmitTranscriptRead -- drives a
  real turn with a non-empty in-memory window (what arms the probe) and asserts
  the transcript read happened on some thread other than the loop's. Reverted,
  it fails with "the re-injection probe read the transcript on the event-loop
  thread".
• test_dashboard_file_io.py::...rehydrate_reads_off_the_loop -- POSTs
  /api/send-message with session=origin against a cold slot and asserts the
  same for the rehydration read.
• test_slack_gateway_cron_exec_coverage.py::...rehydration_reads_the_transcript_off_the_loop
  -- same assertion for the script-cron delivery path.
• Four prefetch_cron_history tests: reads off-loop for an unlinked slot, reads
  when no slot exists yet, skips the read for a linked slot (locking in the
  no-added-cost property), and returns None with no conversation log.
• test_dashboard_cron_to_chat.py::...test_history_is_required_so_the_read_cannot_land_on_the_loop
  -- pins the enforcement the deletion buys: calling the injection without
  history= raises TypeError.
• Updated, not weakened: nine test patch targets that named the sync rehydrate
  seam now name the async one, test_silent_cron_injects_to_existing_slot
  asserts the new history=ANY kwarg like its three sibling assertions already
  did, and the 22 synchronous injection call sites in two files route through a
  local _inject helper that performs the read the fallback used to.

Targeted runs (CI owns the full suite): 260 passed across the cron and gateway
files after the subtraction, 435 across the six originally touched test files,
plus test_slack_gateway.py, test_session_restore.py,
test_rehydrate_async.py, test_send_message_targeted.py,
test_cron_locking_regression.py and test_chat_turn_timeout_consistency.py.
Gates: black (repo gate), isort, flake8, mypy src/kiro_crew/ (1220 files
clean), docs-lint, scrub-lint, check_sync_io_in_async,
check_subprocess_encoding, check_agent_sdk_boundary, check_loop_bound_locks,
check_changelog_history, check_harness_parity, check_brand_name,
check_focus_cue.

Manual verification

N/A -- no user-visible surface changes, and the property under test (which
thread a read runs on) is asserted directly rather than inferred from behavior.

Related Issues

Closes #7408

Pattern harvest

Rule candidate: lint -- scripts/check_sync_io_in_async.py is the gate for
exactly this class, and it did not see any of these five sites. Its detection
surface is primitives: MODULE_FUNCS (os.*, shutil.*, ...), NAME_FUNCS,
HTTP verbs, sync client constructors and DB_METHODS behind a receiver regex.
A domain method that wraps open() + json.loads is invisible to it, so a
transcript parse on the loop reads as clean code. The rule candidate is a small
receiver-matched table of domain readers -- .read_messages( /
.read_messages_chained( on a conversation_log-shaped receiver to start --
carried under the same shrink-only baseline discipline the gate already uses.

There is also a design half worth stating, because it is what actually
prevents the recurrence here: an optional argument on a synchronous function
whose every caller is async is a loop-stall waiting to happen. The default is
what makes the omission silent. Requiring the argument moves the failure from
production to the call site, and needs no detector at all.

Two auxiliary notes for whoever picks up the lint rule, both learned here:

• An audit of "which reads are offloaded" must accept both spellings.
  channel_slots.py:751 is offloaded via loop.run_in_executor, not
  asyncio.to_thread; a detector keyed on one spelling would have "found" it as
  an offender. Main carries both idioms in bulk.
• Reading the flagged line alone is not enough to judge it, and this is the
  reason four of the six listed lines needed no change: under the
  prefetch-at-the-caller convention the offload is not at the read. A useful
  detector has to resolve the enclosing function's callers to the nearest async
  boundary, or it will report exactly this issue's false positives.

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

The claims check out end-to-end: rehydrate_slot_from_history_async exists with the documented race re-checks and None contract, both new callers handle None, the dedup/silent injection sites have no suspension point between prefetch_cron_history's linked-slot check and the synchronous injection (the None path never awaits), and the spec was updated in the same commit. The required-kwarg change removes the cause rather than patching instances, and the tests assert the thread, not the function name.

Design-Verdict: PASS

Loop-stall cause is deleted at its root — history is now a required kwarg — using the codebase's existing prefetch-and-pass-in convention, with race windows verifiably closed.

[DESIGN-REVIEWED] b00488b

[github-actions]

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of b00488b189838a53ed3eb3cb9fc73e9549e9bc81 — 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 description's audit holds up: rehydrate_slot_from_history_async pre-exists with four prior consumers, prefetch_cron_history has exactly 2 consumers, all four production callers of the injection pass history=, the sibling read sites I spot-checked (handlers/sessions.py:1264 is declared out-of-scope for #7404; session_control.py reads the in-memory window, not the file; chat_backfill.py documents mandatory offload) are not unfixed defects. The one residue: two inline prefetches the new helper could replace.

First-Principles-Verdict: PASS

Five loop-stalling reads fixed at cause level — the fallback that invited them is deleted, and every new surface has counted consumers.

What this change ships

Intent: stop large-transcript file reads from freezing every dashboard session's frames — a FIX (issue #7408, measured 100–300 ms reads, logged 6.1 s loop lag).

1. Submitting a prompt no longer stalls other sessions during the history re-count — justified
2. Delivering into a cold tab via /api/send-message rehydrates off the loop — justified, reuses existing rehydrate_slot_from_history_async
3. Script-cron delivery into a cold tab, same swap — justified
4. Suppressed/silent cron injections read off-loop, and skip the read when the tab is linked — justified
5. New helper prefetch_cron_history — justified; 2 consumers counted (slack/gateway.py:4590,4615)
6. Forgetting the prefetch is now a TypeError, not a production stall (fallback deleted, history required) — justified, cause-level deletion
7. Spec paragraph updated in the same commit — mandated by AGENTS.md
8. New tests assert the thread a read ran on — justified

Subtractions

• Fold the two remaining hand-rolled prefetches into prefetch_cron_history: slack/gateway.py:4635-4642 and dashboard/handlers/cron.py:631-634 inline the same to_thread(read_messages, f"cron:{job.id}") the helper owns (2 siblings, grepped inject_cron_result_to_dashboard — 4 production callers, 2 not using the helper). Deletes ~12 lines and gives both sites the linked-slot skip for free. The author declared this deferral, so it is accepted-and-deferred, not a demand.

[FIRST-PRINCIPLES-REVIEWED] b00488b

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] b00488b

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

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

[chenmingwei23]

Disposition of the one advisory residue First Principles raised (the two inline asyncio.to_thread(read_messages) prefetch spellings that prefetch_cron_history could replace): defer, deliberately.

Both of those sites -- slack/gateway.py:4636 and handlers/cron.py:635 -- are already off the event loop, so they are not instances of the defect this PR fixes. Routing them through the new helper would not be a pure refactor either: the helper skips the read when the slot is already linked, so adopting it there changes what those two paths do (one fewer transcript parse on the normal cron-delivery path and on the to-chat replay). That is a behavior change to working code, and folding it in would mean this PR touched two correct paths to make a third one read consistently -- exactly the churn the scoped-fix rule exists to prevent.

It is worth doing, just not here. Whoever picks up the check_sync_io_in_async domain-reader rule from the Pattern harvest section is the natural owner: the unification and the detector both want the same statement to be true afterwards ("no path reaches the injection's own fallback read, and no path prefetches a transcript it will discard"), and landing them together means one review of the behavior change rather than two.

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] b00488b

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

[chenmingwei23]

Backend Tests (Windows) shard 3 came back with two failures on 7cfa8617. One was mine and is fixed; the other is main-owned.

Mine -- fixed. test_send_message_targeted.py::TestChannelTextIsNotTheNotificationText::test_session_closed_suffix_stays_on_the_bell patched handlers.messaging._rehydrate_slot_from_history, the seam this PR replaces, so it raised AttributeError: ... does not have the attribute '_rehydrate_slot_from_history'. My sweep for that patch target missed this ninth occurrence because I truncated the search output. Retargeted to rehydrate_slot_from_history_async like the other eight; test_send_message_targeted.py is 72 passed locally. Amended into the single commit (de08472a3).

Main-owned -- filed as #7490, not fixed here. test_security_posture.py::TestGateSideLogRedactorSpelling::test_the_census_holds_no_slack:

`_BASELINE_LOG_SITE_CENSUS` is now looser than the code -- lower or drop these:
  dashboard/handlers/files.py: 1 sites, census says 3

That test does not exist at this PR's base (bb01943c1) and neither does the census dict -- both arrive with the merge ref. #7278 recorded 3 sites for dashboard/handlers/files.py; #7293 then rewrote that file (-232/+127) down to 1 site without lowering the census, and the census is a shrink-only ratchet, so a stale-high count fails by design. This PR's diff touches none of the three files involved, so the remedy is a one-line census lowering on main rather than someone else's edit riding in here. It will clear on the next merge-ref refresh once #7490 lands; if it is still red when everything else is green I will rebase onto settled main rather than fold it in.

[chenmingwei23]

First Principles' subtraction 1 -- adopted in 0e13d6da4. It was the right call: the five sites were instances, the optional history= was the cause.

inject_cron_result_to_dashboard is synchronous and every one of its callers is async, so an optional history meant a caller could hand the whole-transcript parse back to the event loop by simply not thinking about it -- which is exactly what happened at gateway.py:4581 and :4605, and what my earlier spec sentence could only ask people not to do. The fallback read is deleted and history is now a required keyword parameter. A forgotten prefetch is now a TypeError at the call site.

What that cost, precisely:

• No production call site changed. All four already passed history=.
• None stays legal, because prefetch_cron_history returns it when the slot is already linked and the injection therefore does not consume history. The docstring now records why None cannot mean a lost hydration: the only state that consumes history is an unlinked slot, and the sole writer of a cron slot's linked_session_key is the line immediately below, in the same synchronous block.
• 22 synchronous call sites across test_dashboard_cron_to_chat.py and test_cron_context_meter_seed.py now go through a local _inject helper that performs the read the fallback used to. That is the honest home for it: those tests are sync callers, where a blocking read costs nothing, and the production function no longer offers the convenience that only async callers could misuse.
• New test test_history_is_required_so_the_read_cannot_land_on_the_loop pins the enforcement, so a later "helpful" default reintroducing the class fails a test rather than a review.

The spec line shrank accordingly: it now states the parameter is required and why, instead of asking callers not to reach a fallback that no longer exists. Gates re-run clean on the new head (mypy 1220 files, black gate, flake8, isort, docs-lint, sync-io-in-async), 260 tests pass across the cron and gateway files.

Subtraction 2 (folding the two remaining inline prefetch spellings onto prefetch_cron_history) stays deferred for the reason given in the earlier comment, which the lane recorded as accepted-and-deferred: both are already off the loop, and adopting the helper there changes behavior on correct paths by skipping a read.

[chenmingwei23]

The one remaining red is covered by open PR #7492 ("re-measure the files.py log-site census after #7293"), which makes exactly the 3 -> 1 edit this needs.

It is the only failure left on 0e13d6da4: both failing shards -- Backend Tests (3.10, 3) and Backend Tests (Windows) (3) -- report that single test and nothing else, and my own shard-3 failure from the previous head is gone. Waiting on #7492 rather than opening a competing one-line PR against the same entry; once it merges this clears on the next merge-ref refresh, and I will rebase if it does not.

[bolichen97]

CI green, no blocking bot findings, diff matches description. Approved.