fix: offload remaining transcript reads off the gateway loop

[bolichen97]

Summary

Closes the remaining blocking ConversationLog reads that ran on the gateway event loop (issue #7408). The re-triage comment's correction was confirmed: after auditing both asyncio.to_thread and loop.run_in_executor spellings, the real work list was 3 sites, not 6 — three of the six named sites were already offloaded and are left unchanged.

The signature-change judgment the issue flagged was answered by routing to the pre-existing rehydrate_slot_from_history_async rather than mutating the sync helper's signature, so its sync callers are untouched.

Per-site analysis (why each is correct)

#	site (dashboard-relative)	enclosing	reached from loop?	prior offload state	action taken
1	chat_runner.py:5877	async def _run_chat	YES (bare on-loop read in the prompt-submit re-injection guard)	none	Wrapped in await asyncio.to_thread(read_messages, history_key). history_key is an immutable local snapshot; only the disk read + JSON parse cross the hop. len(), the mem_count scan, and _build_history_prefix stay loop-affine. The if state.conversation_log: guard is preserved.
2	session_transfer.py:173	sync _read_chained_history	no	already offloaded via asyncio.to_thread(_read_and_assemble)	unchanged
3	chat_persistence.py:1009	sync _rehydrate_slot_from_history	YES at two async callers (slack/gateway.py:3239 _deliver_script_result, handlers/messaging.py:2135 api_send_message)	none at those sites (async entry existed)	Routed both callers to await rehydrate_slot_from_history_async(...) (same None-for-closed/gone contract; loop-affine slot build stays on the loop). Sync signature unchanged. Gateway's now-unused sync import removed; messaging's import swapped (no F401).
4	channel_slots.py:751	sync nested _load_messages	no	already offloaded via loop.run_in_executor(None, _load_messages)	unchanged. A to_thread-only audit misses this (~447 run_in_executor vs ~2321 to_thread on main) — exactly the re-triage's point.
5	handlers/artifacts.py:755	sync _collect_session_docs	no	already offloaded via _run_off_loop through both callers	unchanged
6	cron_inject.py:98	sync inject_cron_result_to_dashboard	YES at the two gateway suppress callers only (dedup-suppress, silent-suppress; passed no history)	none at those two; other two callers already prefetch	Both suppress callers now prefetch history = await asyncio.to_thread(read_messages, "cron:{job.id}") if conversation_log else [] and pass history=, so history is None is False and the inline read is skipped. cron_inject.py is unchanged and keeps its inline read as the fallback for a synchronous caller. Signature unchanged.

Changed files

Production: dashboard/chat_runner.py, dashboard/handlers/messaging.py, slack/gateway.py.
Tests: test/test_transcript_reads_off_loop.py (new) plus mechanical renamed-symbol updates to test_dashboard_file_io.py, test_cron_origin_injection.py, test_send_message_targeted.py.
Confirmed not in the diff: session_transfer.py, channel_slots.py, handlers/artifacts.py, cron_inject.py. No docs/CHANGELOG touched.

Testing

New test/test_transcript_reads_off_loop.py uses a thread recorder that wraps the real reads and asserts the loop thread's ident never appears, plus a behavior-preservation assertion per site (would fail if any offload were reverted).

Not runnable in this sandbox (network mode INTEGRATIONS_ONLY: PyPI blocked, aiohttp/pytest/pytest-asyncio not installed). Achievable gate passed on the pyenv 3.11.15 interpreter: py_compile exit 0 on all 7 changed files; ruff clean on the new test file and all changed regions; black --check clean on the new test file and changed production files. The new test and the 3 updated tests must run in CI before merge.

Reviewer note (non-blocking)

_run_chat has a benign post-hop staleness window: disk_count is a pre-await snapshot while mem_count is read after the hop. Worst case on a brand-new session's first turn is a redundant or skipped history prefix, never corruption.

Follow-up (not in this PR)

read_messages_chained full-reads a transcript then keeps only messages[-500:] (~254 ms on a 33.6 MB session, ~99% discarded). A tail-read would fix it but must recompute _disk_older_count, which drives the frozen-prefix save model, so it carries data-loss risk and needs its own tests. Tracked separately, intentionally out of scope here.

This PR references #7408 but does not close it via keyword, so the issue stays open for the follow-up.

Pattern harvest

Rule candidate: repo gate (AST walker under scripts/, in the shape of check_harness_parity.py / check_subprocess_encoding.py) — "a blocking ConversationLog read reached directly from an async def".

Pattern to flag: a call to ConversationLog.read_messages, read_messages_chained, or get_metadata (each a whole-file open plus a JSON parse per line — 100–300 ms on a large store) whose nearest enclosing function is an async def, and which does not sit in the callable-argument position of asyncio.to_thread(...) / loop.run_in_executor(...). That is a purely syntactic test — no dataflow needed, because the offload idiom is itself syntactic — and it is exactly what the manual audit behind this PR was doing by hand.

Why the class is real rather than a one-off: the three sites fixed here match that shape, and so do two more that a reviewer found afterwards by grepping \.read_messages(_chained)?\( and reading each enclosing scope — dashboard/handlers/sessions.py (api_session_detail, whose own file-neighbours api_sessions_search and api_session_delete both offload) and sync_bridge.py (handoff_to_slack, plus a bare get_metadata two lines later). Five instances, and a careful hand audit still missed two. Nothing in CI notices: the code type-checks, every test passes, and the only symptom is a latency spike plus — because the loop-stall watchdog is itself a coroutine — a process exit under load. A defect class that is invisible to every existing gate and invisible in review regrows on the next handler someone writes, which is why this one wants a gate rather than another audit. The two sites named above are the gate's first two hits and are listed in this PR's follow-up.

Secondary, review-prompt line: "when you hoist IO across an await to get it off the loop, check both what consumes it and what catches it." The first version of this fix prefetched the cron transcript unconditionally at both suppress sites, although inject_cron_result_to_dashboard reads history only under if not slot.linked_session_key — so it paid, on every suppressed run, the exact cost the offload exists to remove. Worse, the whole of _cron_callback is inside one broad except Exception that ends at record_failure(): hoisting the read up into that scope meant a transcript OSError on a run that had already produced its result would be counted as a cron failure, marching a healthy job toward auto-pause. Generalized as a review question with two halves: "on which branch does the callee actually consume this argument?", and "which handler now catches this exception, and what does it conclude from it?"

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

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

Design-Verdict: CONCERNS

Sound offload routed through the pre-existing async rehydrate, but the cron prefetch reads the whole transcript every run to cover a rare hydration case.

Watch

• The suppress-path prefetch is unconditional, yet inject_cron_result_to_dashboard uses history only when not slot.linked_session_key (cron_inject.py:95-101) — the rare first-link case. Cause → mechanism → consequence: every suppress-path cron completion on an already-linked slot ("Prefetch the transcript off the loop so the inject's linked-session read cannot block") now does an awaited full-transcript read (the ~100-300ms cost this PR is chasing) that is discarded, delaying cron result delivery and adding IO that grows with transcript size — the old code paid it only in the rare unlinked case. Guard the prefetch on the slot's linked_session_key (the slot is fetchable; has_slot was just checked).

Suggestions

• The prefetch duplicates cron_inject internals (the cron:{job.id} key format and the hydration precondition) at what are now four call sites; an inject_cron_result_to_dashboard_async in cron_inject owning the conditional off-loop read would keep that knowledge in one place.

[DESIGN-REVIEWED] c7c1302

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] c7c1302

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

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

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of c7c1302a7a371769d3bad40b7cfae1eec8ce56bd — 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 is done. Summary of what I confirmed: the fix reuses the pre-existing rehydrate_slot_from_history_async (chat_persistence.py:1138, already imported in gateway.py:98) and the pre-existing history= parameter on inject_cron_result_to_dashboard — no new surface is added. Every item in the diff is part of the declared fix. The one real finding: the description claims it "closes the remaining blocking ConversationLog reads" on the gateway loop, but grepping \.read_messages(_chained)?\( across src/kiro_crew and reading the enclosing functions turns up two on-loop reads inside async defs that the audit missed — handlers/sessions.py:1264 (api_session_detail, whose neighbours in the same file offload) and sync_bridge.py:100 (handoff_to_slack). That's a completeness gap, not a defect in what shipped, so it lands as CONCERNS, not BLOCK.

First-Principles-Verdict: CONCERNS

Every item is the declared fix on an existing seam — but "closes the remaining blocking reads" is contradicted by two counted on-loop reads the audit missed.

What this change ships

Intent: stop transcript disk reads from stalling the gateway event loop (issue #7408) — a FIX.

1. Prompt submit no longer blocks the loop on the history re-injection read — justified
2. Cross-session send-message cold-slot restore reads off the loop via existing async helper — justified
3. Slack cron script-result cold-slot restore routed to the same existing helper — justified
4. Cron dedup-suppressed results prefetch the transcript off the loop — justified
5. Cron silent-suppressed results prefetch the transcript off the loop — justified
6. New test suite pins each read to a non-loop thread — justified
7. Mechanical test updates for the renamed patch target — rides along, necessarily

No new config key, flag, signature, or public surface; items 2–5 route through mechanisms that pre-exist the diff (rehydrate_slot_from_history_async at chat_persistence.py:1138, the history= parameter on inject_cron_result_to_dashboard, and the creator path's identical prefetch at gateway.py:4651).

Watch

• The description's "Closes the remaining blocking ConversationLog reads that ran on the gateway event loop" overclaims. Grepping \.read_messages(_chained)?\( (19 hits in src/kiro_crew) and reading enclosing scopes leaves 2 unfixed siblings of the same root cause, both bare reads inside async defs: dashboard/handlers/sessions.py:1264 (api_session_detail — its neighbours api_sessions_search and api_session_delete in the same file already offload) and sync_bridge.py:100 (handoff_to_slack, which also calls get_metadata on-loop at line 105). Each is the same two-line to_thread wrap this PR applies elsewhere — in scope, or name them in the follow-up alongside the read_messages_chained tail-read.

[FIRST-PRINCIPLES-REVIEWED] c7c1302

[github-actions]

GPT 5.6 Review — 🔴 changes requested (blocking)

GPT 5.6 found at least one blocking issue that must be resolved before merging c7c1302a7a371769d3bad40b7cfae1eec8ce56bd.

This comment is updated in place on each push.

BLOCKING -- src/kiro_crew/slack/gateway.py:4584 -- Suppression paths can auto-pause successful cron jobs

await asyncio.to_thread(self.dashboard_state.conversation_log.read_messages, ...)
Already-linked cron slot + exhausted read OSError -> suppression branch -> failure handler -> successful runs accumulate failures and auto-pause.
Anchor: residual/crash-data-loss-corruption
Fix: At both suppression sites, read history only for an unlinked slot; otherwise pass [].
[BLOCK-MERGE] c7c1302
[GPT-REVIEWED] c7c1302
False positive or not applicable? A repository writer can comment:
/ai-review override gpt c7c1302a7a371769d3bad40b7cfae1eec8ce56bd: <one-sentence reason>

[bolichen97]

Closing as superseded by #7469, which is merged (fb494b6c5, "perf(dashboard): take the last blocking transcript reads off the event loop"). Issue #7408 is already closed/completed.

Both PRs close #7408 and offload the same three reads. #7469 was opened 12 minutes after this one and landed first. This is not a judgement call — it is what the rebase says. Resolving this branch onto the current main tip, hunk by hunk in the "main's version plus this PR's delta" direction, the delta turned out to be a strict subset of main's at every site, so every one of the seven conflicts resolved to main's text and git diff --cached origin/main collapsed to a single file: this PR's new test/test_transcript_reads_off_loop.py.

Site by site:

site	this PR	#7469
chat_runner disk-count read	wraps read_messages in asyncio.to_thread	same, differing only in comment wording and whether the result lands in a local
messaging + gateway rehydrate	swaps to rehydrate_slot_from_history_async and awaits it	identical
gateway's two cron suppress paths	prefetches the cron:{id} transcript off-loop, skipping the read when the slot is already linked — as a ~15-line inline block duplicated at both call sites	same behaviour, factored into a named cron_inject.prefetch_cron_history helper

#7469 is a strict superset, not merely an equivalent. It also deleted the synchronous fallback read inside inject_cron_result_to_dashboard and made history a required keyword-only parameter, so a forgotten prefetch is now a TypeError at the call site rather than a silent loop stall. This PR deliberately left that fallback in place — its own body says "cron_inject.py is unchanged".

That difference is also what makes the one remaining file unsalvageable as it stands. test_cron_inject_reads_inline_only_without_history calls inject_cron_result_to_dashboard(state, job, "cron output") with no history= and asserts the inline read fires — i.e. it pins the exact fallback #7469 removed as the defect's root cause. Measured: TypeError: inject_cron_result_to_dashboard() missing 1 required keyword-only argument: 'history'. The other five tests pass but duplicate coverage #7469 already merged, by thread identity at the same three sites.

So the only two ways to "finish" this PR would be to push an effectively empty commit carrying a duplicate test file, or to delete a red test to go green. Neither is worth doing, so the branch is left unpushed at 7618fdac8 and nothing here was force-pushed.

One thing worth keeping, and it is small. test_run_chat_reinjects_when_memory_leads_disk is genuinely additive rather than duplicative — it is a behaviour-preservation test asserting the mem_count > disk_count branch still re-injects after the read moved off the loop, and #7469 has no equivalent. If anyone wants it, it is a ~30-line lift onto main on its own; it does not need this PR.

Process note for whoever dispatches these. #7469 carries Co-authored-by: gh-autofix#2887 and this PR is the same fleet on the same issue number (fix/offload-transcript-reads-7408). Two automated attempts raced the same issue twelve minutes apart and both ran to full review. A dedup check at dispatch time — "is there already an open PR whose branch name or linked issue matches?" — would have saved one of the two entirely, and this is the second instance today: #7382 was superseded the same way by #7380, and #7357/#7292 were self-duplicates of #7354/#7283.