Skip to content

tui: a failing runtime store is a visible notice, not a log line - #5972

Open
Hmbown wants to merge 2 commits into
mainfrom
fix/runtime-store-failures-visible-5931
Open

tui: a failing runtime store is a visible notice, not a log line#5972
Hmbown wants to merge 2 commits into
mainfrom
fix/runtime-store-failures-visible-5931

Conversation

@Hmbown

@Hmbown Hmbown commented Sep 7, 2026

Copy link
Copy Markdown
Owner

Closes #5931

The runtime-store half of #5931. The session-id-divergence half landed earlier as #5960, and the approval-receipt half (approval log path + error kind + next action in the tool error) already landed on main in 8edc6cb.

What changed

  • The runtime thread store's load_thread/load_turn/load_item/save_*/list_items_for_turn paths attach a typed RuntimeStoreRecordFailure (operation, record kind, record id, path) as anyhow context, so a monitor recognizes a store fault by type instead of by message text. The context's Display keeps the previous message text (Failed to read turn <path>), so anything matching on the old strings still matches.
  • RuntimeThreadManager gains report_store_failure, which logs and — when the error carries the typed context — publishes a runtime.store_failure event through the existing emit_event -> event_tx -> RuntimeEventRecord channel (the SnapshotsDisabledNotice / SessionIdDivergedNotice precedent from tui: a diverged engine session id is a visible notice, not a log line #5960). The paths that used to be log lines only are wired in: "Failed to terminalize item after monitor failure", "Failed to list turn items", "Failed to load turn after monitor failure", "Failed to persist terminal monitor failure", and the Failed to monitor turn error path. A fault that also blocks event publication stays a log line, since the event log lives in the same store.
  • The TUI taps the manager's broadcast (TaskManager::subscribe_runtime_events) once per event-loop tick and shows each notice as a 12 s warning toast plus a transcript system line: RuntimeStoreUnreadableNotice / RuntimeStoreUnwritableNotice, added as MessageIds and translated in all 15 locale packs exactly as tui: a diverged engine session id is a visible notice, not a log line #5960 did.
  • The task timeline (task_manager::ingest_runtime_event) records the notice as a task Error event, and a terminal: true notice resolves the turn as Failed, so the driver stops waiting instead of idling out.
  • The compat SSE stream (map_compat_stream_event) forwards the event, and docs/RUNTIME_API.md documents the payload.

How a store failure surfaces now

A corrupted or unwritable record under the session's runtime store produces one event with: operation (read|parse|write), record_kind (thread|turn|item), record_id, path, the full error chain, the root-cause reason, a concrete next_action (move that file aside, or check free space and permissions there), a one-line message, and terminal when the turn's own record is unreadable/unwritable — meaning no turn.completed can follow and waiting drivers should treat it as failed.

Gates (worktree tmp/wt-5931, commit 6ffde4b, rebased on main at 2ef252c)

  • cargo fmt --all: clean
  • cargo clippy --workspace --all-targets --all-features --locked -- -D warnings (with the standing -A uninlined_format_args/too_many_arguments/unnecessary_map_or): clean. Its first post-rebase run caught start_with_executor as dead code in the lib target (only test callers remain after this refactor), fixed with #[cfg(test)]
  • Targeted store-failure tests: 6 passed, 0 failed
  • cargo test -p codewhale-tui --lib --locked: 11876 passed, 0 failed, 13 ignored (208.9 s). Two earlier full runs under heavy machine contention (other worktrees running the same suite concurrently) each had a few deadline-sensitive failures (one runtime_chat_relay, six runtime_threads/runtime_api sequencing tests); each of those passes in isolation and none touches code changed here

Devin Review

Note

Medium Risk
Touches durable runtime persistence and turn lifecycle termination on store I/O failures; behavior change is intentional but affects failure modes when disk state is corrupt or full.

Overview
When the background runtime cannot read, parse, or write its own thread/turn/item JSON on disk, the change stops treating that as log-only noise and makes it actionable across the stack.

Runtime: Load/save paths attach typed RuntimeStoreRecordFailure context (operation, record kind, id, path). Monitor/settle paths call report_store_failure, which emits runtime.store_failure with reason, next_action, and terminal: true when the turn’s record is unreadable/unwritable so tasks do not idle waiting for turn.completed.

Consumers: The task timeline records the notice (terminal faults fail the turn). The TUI subscribes to runtime events and shows a warning toast plus transcript line via new locale strings. Compat SSE forwards the event; RUNTIME_API docs describe the payload.

Reviewed by Cursor Bugbot for commit ea45b97. Bugbot is set up for automated code reviews on this repo. Configure here.

A runtime turn whose own store record could not be read, parsed, or
written surfaced only as a 'Failed to read turn ...' log line while the
operator's task sat waiting on a turn.completed that could never arrive
(#5931, store half; the session-id half landed as #5960).

The store's load/save paths now attach a typed RuntimeStoreRecordFailure
(operation, record kind, record id, path) as anyhow context, and the
monitor failure paths publish a runtime.store_failure event through the
existing emit_event -> event_tx -> RuntimeEventRecord channel, following
the SnapshotsDisabledNotice / SessionIdDivergedNotice precedent: the
payload names the file, the root cause, and the next action (move the
file aside, or check free space and permissions). The TUI drains the
manager's event tap each tick and shows a 12 s warning toast plus a
transcript line (RuntimeStoreUnreadableNotice / RuntimeStoreUnwritableNotice,
translated in all 15 packs), the task timeline records the fault, and the
compat SSE stream forwards the event. When the turn's own record is
unreadable or unwritable the notice is terminal, so the task driver fails
at once instead of idling out.

Gates (run in this worktree, on this commit after the rebase):
- cargo fmt --all: clean
- cargo clippy --workspace --all-targets --all-features --locked
  -D warnings (with the standing three -A allowances): clean; its first
  post-rebase run caught start_with_executor as dead code in the lib
  target (its remaining callers are tests), fixed with #[cfg(test)]
- targeted store-failure tests: 6 passed, 0 failed
- cargo test -p codewhale-tui --lib --locked: 11876 passed, 0 failed,
  13 ignored. Two earlier full runs under heavy machine contention
  (concurrent same-binary suites from other worktrees) each had a few
  deadline-sensitive failures (one runtime_chat_relay, six
  runtime_threads/runtime_api sequencing tests); every one of them
  passes in isolation and none touches code changed here.

Signed-off-by: CodeWhale Bot <bot@codewhale.net>
Copilot AI lite review requested due to automatic review settings September 7, 2026 02:47
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 7, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-07T02:56:53.076413Z 6ffde4b PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@cursor

cursor Bot commented Sep 7, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_04cdee42-2057-4017-b5d8-1b28a59fd83a)

@claude

claude Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Claude finished @Hmbown's task in 2m 45s —— View job


Claude finished @Hmbown's task

Todo list

  • Gather context (diff, CLAUDE.md/AGENTS.md, changed files)
  • Understand the request (code review of PR tui: a failing runtime store is a visible notice, not a log line #5972)
  • Review runtime_threads.rs typed failure context + report_store_failure
  • Review task_manager.rs ingest of runtime events / terminal failure handling
  • Review tui/ui/event_loop.rs notice rendering + locale wiring
  • Review runtime_api.rs compat SSE forwarding
  • Review tests for new behavior
  • Post final review summary

Summary

This is a clean, well-scoped change that does exactly what the title says: it turns five previously log-only store-failure sites into a typed, structured runtime.store_failure event that flows through the existing event pipe (task timeline → SSE → TUI toast/transcript). No correctness bugs found.

What I checked closely, and why it holds up:

  • Lock discipline (crates/tui/src/runtime_threads.rs:7114-7234, settle_claimed_turn_failure): _turn_mutation guards are scoped so they drop before the .await on report_store_failure, matching the pre-existing pattern in this function. The one case where report_store_failure is called while _projection is still held is explicitly commented as following the documented lock order (publish takes event_emit after the projection lock) — consistent with the existing emit_turn_completed_if_missing/queue_recovery_receipt calls just below it in the same scope, so this isn't a new deadlock risk.
  • report_store_failure (runtime_threads.rs:5391-5426) always logs via tracing::error! first, then early-returns None if the error doesn't carry the typed RuntimeStoreRecordFailure context — so untyped failures keep behaving as plain log lines, matching the PR's stated intent ("a fault that also blocks event publication stays a log line").
  • terminal flag semantics are applied correctly: true only for the two paths where the turn's own record is unreadable/unwritable (load-turn failure, persist-terminal-turn failure); false for item-level and list failures, and for the generic "Failed to monitor" path (which still resolves through settle_claimed_turn_failure via the normal route, so it isn't itself the terminal signal).
  • start_with_executor#[cfg(test)]: verified via grep that its only callers are in automation_manager.rs tests and runtime_api/tests.rs, so gating it behind #[cfg(test)] doesn't break any non-test call site, matching the PR's clippy note.
  • Locale coverage: confirmed all 15 locale JSON files (ca, de, en, es-419, fr, hi, id, ja, ko, pt-BR, ru, uk, vi, zh-Hans, zh-Hant) each got both RuntimeStoreUnreadableNotice and RuntimeStoreUnwritableNotice keys, and both are registered in ALL_MESSAGE_IDS in localization.rs.
  • Provider/model routing invariant: not implicated — this PR touches only the runtime store/event/notice path, no model or provider selection logic.
  • Tests: good coverage across the stack — typed-context round-trip and next_action text (runtime_threads/tests.rs), event publication from settle_claimed_turn_failure and report_store_failure including the untyped-error-stays-a-log-line case, task-timeline ingestion turning a terminal: true notice into RuntimeTurnStatus::Failed (task_manager.rs), SSE compat forwarding (runtime_api/tests.rs), and toast/transcript rendering for both read and write notice variants (tui/ui/tests.rs).

Minor observations (not blocking):

  • In runtime_store_failure_notice (event_loop.rs), the TUI-facing message is built entirely from the localized template ({record}/{path}/{reason}) and doesn't use the payload's next_action string at all — the "what to do" text is duplicated between RuntimeStoreRecordFailure::next_action() and the locale strings. That's an acceptable/expected duplication given i18n requires the remedy text to live in the translated template rather than the English-only next_action field, but it does mean if next_action()'s wording changes, the locale strings need a matching manual update (as this PR did) — worth a one-line note somewhere near next_action() if that isn't already documented elsewhere.
  • Nothing else stood out; error handling, event ordering, and the new public API surface (RUNTIME_STORE_FAILURE_EVENT, RuntimeStoreRecordFailure, RuntimeStoreFailureNotice) all look intentionally minimal and consistent with the SnapshotsDisabledNotice/SessionIdDivergedNotice precedent this PR cites.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 3 potential issues.

Devin Review

let mut pending_translations = 0usize;
// #5931: the background runtime's own store faults arrive on its event
// channel, which nothing else in this loop reads.
let mut runtime_event_rx = task_manager.subscribe_runtime_events();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Startup store warnings can disappear

When restored tasks fail immediately, subscribe_runtime_events attaches after the worker loop starts. Broadcast events have no replay, so the TUI can miss the warning entirely.

Prompt for agents
Remove the startup gap between TaskManager worker execution and the TUI runtime-event subscription. The manager starts workers in TaskManager::start_with_executor_and_runtime before run_event_loop calls TaskManager::subscribe_runtime_events. Because tokio broadcast receivers only observe later sends, an immediately resumed queued task can publish runtime.store_failure before the TUI receiver exists. Establish the receiver before workers can emit, or add durable replay with sequence-based deduplication when the TUI tap starts.
Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +7308 to +7309
self.report_store_failure(&thread_id, Some(&turn_id), &failure, &error, false)
.await;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Store failures produce duplicate notices

When monitoring hits a store fault, report_store_failure publishes it before settlement retries the same record. Settlement reports it again, duplicating timeline errors, toasts, and transcript lines.

Prompt for agents
Deduplicate runtime.store_failure publication across monitor_claimed_turn and settle_claimed_turn_failure. A typed error is currently reported once in monitor_claimed_turn, then settlement commonly encounters the same unreadable item or unwritable turn and reports it again. Preserve the terminal=true signal when settlement proves no turn.completed can follow, but emit one user-visible fault per underlying record failure, for example by deferring publication to settlement or carrying the original failure into settlement and upgrading its terminal classification there.
Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment thread CHANGELOG.md
Comment on lines +48 to +55
- A background runtime turn whose own store record could not be read, parsed
or written (`Failed to read turn …`, `Failed to read item …`) was only a log
line. The runtime now publishes a `runtime.store_failure` event naming the
file, the root cause and the next action (move the file aside, or check free
space and permissions); the TUI shows it as a warning toast and a transcript
line, the task timeline records it, and the runtime API streams it. A turn
whose own record is unreadable or unwritable is reported as terminal, so its
task fails at once instead of idling out (#5931).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Remove branch changelog edits

Repository rules reserve both changelogs for merge-time updates on main. These additions create the conflicts that policy explicitly avoids.

Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Store-failure event detection and emission has two concrete edge cases that can suppress typed detection under added context and can duplicate user-visible notices for a single underlying store fault.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Implements the runtime-store portion of #5931 by turning on-disk runtime thread store read/parse/write failures into a typed, user-visible notice instead of relying on log lines, and propagating that notice through existing runtime event channels.

Changes:

  • Adds typed RuntimeStoreRecordFailure context to runtime store I/O errors and introduces a runtime.store_failure event payload.
  • Surfaces store-failure events in the TUI (toast + transcript), task timeline (error + terminal fail), and compat SSE stream.
  • Documents the new event and adds localized strings across all locale packs.
File summaries
File Description
docs/RUNTIME_API.md Documents runtime.store_failure event semantics/payload.
crates/tui/src/tui/ui/tests.rs Adds UI test coverage for store-failure toast/transcript.
crates/tui/src/tui/ui/event_loop.rs Subscribes/drains runtime events and renders store-failure notices.
crates/tui/src/task_manager.rs Ingests store-failure events; exposes runtime event subscription.
crates/tui/src/runtime_threads/tests.rs Adds tests for typed context + event publication behavior.
crates/tui/src/runtime_threads.rs Implements typed store failure context + event emission plumbing.
crates/tui/src/runtime_api/tests.rs Adds compat SSE mapping test for store-failure events.
crates/tui/src/runtime_api.rs Forwards runtime.store_failure over compat SSE mapping.
crates/tui/src/localization.rs Adds new MessageIds for store-failure notices.
crates/tui/locales/zh-Hant.json Adds translations for new store-failure notice strings.
crates/tui/locales/zh-Hans.json Adds translations for new store-failure notice strings.
crates/tui/locales/vi.json Adds translations for new store-failure notice strings.
crates/tui/locales/uk.json Adds translations for new store-failure notice strings.
crates/tui/locales/ru.json Adds translations for new store-failure notice strings.
crates/tui/locales/pt-BR.json Adds translations for new store-failure notice strings.
crates/tui/locales/ko.json Adds translations for new store-failure notice strings.
crates/tui/locales/ja.json Adds translations for new store-failure notice strings.
crates/tui/locales/id.json Adds translations for new store-failure notice strings.
crates/tui/locales/hi.json Adds translations for new store-failure notice strings.
crates/tui/locales/fr.json Adds translations for new store-failure notice strings.
crates/tui/locales/es-419.json Adds translations for new store-failure notice strings.
crates/tui/locales/en.json Adds translations for new store-failure notice strings.
crates/tui/locales/de.json Adds translations for new store-failure notice strings.
crates/tui/locales/ca.json Adds translations for new store-failure notice strings.
crates/tui/CHANGELOG.md Notes new visible behavior for runtime store failures.
CHANGELOG.md Notes new visible behavior for runtime store failures.
Review details
  • Files reviewed: 26/26 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +997 to +1001
/// The typed store fault behind an error, if any layer of it is one.
#[must_use]
pub fn from_error(error: &anyhow::Error) -> Option<&Self> {
error.downcast_ref::<Self>()
}
Comment on lines +7304 to +7311
Ok(Err(error)) => {
let failure = format!("Failed to monitor {}: {error}", kind.label());
// An unreadable item or turn under the monitor is the
// operator's own state: name the file before settling (#5931).
self.report_store_failure(&thread_id, Some(&turn_id), &failure, &error, false)
.await;
failure
}

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6ffde4b7a6

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +6702 to +6706
"{record}",
&format!(
"{} {}",
notice.failure.record_kind, notice.failure.record_id
),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Localize the runtime record kind

When a non-English locale is active, {record} is still constructed from RuntimeStoreRecordKind's English Display implementation, so otherwise translated notices contain phrases such as thread <id> or item <id>. Add typed message IDs for these nouns or select a fully localized template rather than composing user-visible prose from the enum display.

AGENTS.md reference: crates/tui/AGENTS.md:L25-L26

Useful? React with 👍 / 👎.

Comment on lines +1176 to +1178
// #5931: the background runtime's own store faults arrive on its event
// channel, which nothing else in this loop reads.
let mut runtime_event_rx = task_manager.subscribe_runtime_events();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Subscribe before recovered task workers can publish

When startup restores queued tasks, TaskManager::start_with_executor_and_runtime spawns its workers before returning, but this broadcast subscription is created only later after additional TUI setup. Because Tokio broadcast receivers do not replay earlier messages, a recovered task can publish runtime.store_failure before this line and the promised toast/transcript notice is permanently missed; establish the receiver before workers start or replay durable failure events on subscription.

Useful? React with 👍 / 👎.

Comment on lines +6648 to +6652
Err(TryRecvError::Lagged(skipped)) => {
tracing::warn!(
skipped,
"runtime event tap lagged; a store-failure notice may have been missed"
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Recover store-failure notices after broadcast lag

When more than the runtime channel's 1,024 events accumulate before this tap drains them, TryRecvError::Lagged advances past the skipped records and only writes a log warning. If runtime.store_failure was among those records, it never reaches the toast or transcript—the exact failure this change is intended to surface. Use a dedicated failure channel or replay the durable event log after lag instead of accepting the loss.

Useful? React with 👍 / 👎.

Comment on lines +1670 to +1677
let record: TurnRecord = serde_json::from_str(&raw).with_context(|| {
RuntimeStoreRecordFailure::new(
RuntimeStoreOperation::Parse,
RuntimeStoreRecordKind::Turn,
turn_id,
&path,
)
})?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Attach typed context to unsupported-schema failures

When a turn record was written by a newer Codewhale version, deserialization succeeds and the subsequent schema-version check fails outside this new typed context. RuntimeStoreRecordFailure::from_error therefore returns None; the monitor logs the failure, settlement fails to load the same turn again, and no terminal runtime.store_failure is emitted, leaving task drivers waiting until their guard timeout without the actionable path notice. Include post-deserialization validation failures in the typed load context as well.

Useful? React with 👍 / 👎.

Comment on lines +5663 to +5668
// The operator's own store failed; the payload names the file and
// the next action, so compat clients see it too (#5931).
crate::runtime_threads::RUNTIME_STORE_FAILURE_EVENT => Some(sse_json(
crate::runtime_threads::RUNTIME_STORE_FAILURE_EVENT,
payload.clone(),
)),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Terminate compatibility streams on terminal store failures

When this mapped event carries terminal: true, take_compat_turn_event still considers only turn.completed terminal. Since this notice explicitly means no turn.completed can follow, the one-turn compatibility SSE handler remains open sending keepalives indefinitely instead of emitting done; existing compatibility clients that wait for the stream's normal terminal marker hang even though the runtime has already declared the turn failed. Treat a terminal runtime.store_failure as a terminal stream event.

Useful? React with 👍 / 👎.

Comment on lines +1007 to +1011
match self.operation {
RuntimeStoreOperation::Read | RuntimeStoreOperation::Parse => format!(
"Move {} aside (or delete it) and retry; the thread's other records stay in place.",
self.path.display()
),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Do not recommend deleting records after generic read errors

When a read fails because of permissions, a transient I/O error, file-descriptor exhaustion, or another non-corruption cause, this branch still tells the operator to move or delete the record even though its contents may be healthy. Following that advice can destroy the only copy of a turn or item while doing nothing to fix the actual access problem; distinguish parser corruption from generic read failures and give read errors a non-destructive permissions/access remedy.

AGENTS.md reference: AGENTS.md:L28-L29

Useful? React with 👍 / 👎.

Comment on lines +6708 to +6709
.replace("{path}", &notice.failure.path.display().to_string())
.replace("{reason}", &notice.reason)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve placeholder text inside runtime paths

When the runtime-store path contains the literal substring {reason}—which is legal in a configured home or data-directory name—the {path} replacement inserts it and the following replacement interprets that inserted text as another template placeholder. The notice therefore displays a different, nonexistent path and gives unusable recovery guidance. Substitute placeholders in one pass, as the existing brace-preserving localization formatter does, rather than chaining replace over already-inserted values.

Useful? React with 👍 / 👎.

@codewhale-agent codewhale-agent Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codewhale review

PR makes runtime store read/parse/write faults visible through typed RuntimeStoreRecordFailure context, a runtime.store_failure event, TUI toasts, task timeline errors, and SSE/docs. The implementation is mostly solid and well-tested, but schema-version load failures are not typed, leaving a task-idling blind spot.

Findings

  • [WARNING] Schema-version load failures bypass typed context and can still idle tasks (crates/tui/src/runtime_threads.rs:1678)
    In load_turn, the schema_version bail after a successful parse returns a plain anyhow error without a RuntimeStoreRecordFailure context. settle_claimed_turn_failure relies on RuntimeStoreRecordFailure::from_error to publish a terminal runtime.store_failure when the turn record cannot be loaded. A turn record that parses but has schema_version > CURRENT_RUNTIME_SCHEMA_VERSION therefore fails with an untyped error, no runtime.store_failure is published, and the driver can wait/idle instead of failing fast. The same visibility gap applies to load_item/load_thread, but load_turn is the terminal-idle case.

Assessment

The change is well-structured, localized, documented, and covered by focused tests. The terminal-behavior blind spot for future-schema turn records should be fixed before merge so every failed load_turn in the terminal path publishes a terminal runtime.store_failure.


Advisory review by Codewhale (codewhale review --pr 5972 --post, head 6ffde4b7a6ec0ab5138e5a3d8da782b3d6f498bd). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.

&path,
)
})?;
if record.schema_version > CURRENT_RUNTIME_SCHEMA_VERSION {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[WARNING] Schema-version load failures bypass typed context and can still idle tasks

In load_turn, the schema_version bail after a successful parse returns a plain anyhow error without a RuntimeStoreRecordFailure context. settle_claimed_turn_failure relies on RuntimeStoreRecordFailure::from_error to publish a terminal runtime.store_failure when the turn record cannot be loaded. A turn record that parses but has schema_version > CURRENT_RUNTIME_SCHEMA_VERSION therefore fails with an untyped error, no runtime.store_failure is published, and the driver can wait/idle instead of failing fast. The same visibility gap applies to load_item/load_thread, but load_turn is the terminal-idle case.

@cursor

cursor Bot commented Sep 7, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_3f1a047d-3c39-4d27-846d-a77fdb1433c0)

@claude

claude Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Claude finished @Hmbown's task in 7m 7s —— View job


Claude's review of PR #5972

I independently traced the diff (origin/main...HEAD) rather than just restating the existing Devin/Copilot/Codex/Codewhale bot comments — where I agree with one of them I've confirmed it with the actual call chain, and I've noted where I think a flagged concern isn't actually live in this code. I posted 4 inline comments for the findings with concrete downstream evidence; summary below.

Correctness bugs (highest priority)

  1. Compat SSE stream never closes on a terminal store failureruntime_api.rs:5665. take_compat_turn_event (runtime_api.rs:5453, unchanged by this PR) only sets its stream-closing terminal bool for event.event == "turn.completed". A runtime.store_failure notice with payload["terminal"] == true — which by this PR's own contract means "no turn.completed can follow" — still leaves that bool false, so all three SSE loop sites keep the connection open on 15s keepalives forever. This directly contradicts the PR's stated goal ("terminal ... so waiting drivers should treat it as failed") for the one-turn compat consumer specifically. The only test touching this event (stream_compat_mapping_forwards_runtime_store_failures) calls map_compat_stream_event directly and never reaches take_compat_turn_event, so it doesn't catch this. Confirmed, would recommend fixing before merge since it's a regression against the PR's own headline claim, not a pre-existing gap.

  2. Schema-version-newer-than-supported load failures bypass typed context entirely — agreeing with and extending the Codewhale bot's finding (runtime_threads.rs:1677, also applies to load_thread/load_item). The bail! after a successful parse when schema_version > CURRENT_RUNTIME_SCHEMA_VERSION carries no RuntimeStoreRecordFailure, so report_store_failure silently returns None for it — no toast, no transcript line, no task-timeline error, no terminal SSE event. For load_turn in settle_claimed_turn_failure, that's precisely the "turn's own record can't be loaded" case meant to be terminal: true; instead it idles a waiting driver out. Narrow trigger (only on a downgrade / mixed-version store), but it's the one load-failure branch this PR didn't wire up.

  3. A single store fault can publish two runtime.store_failure events — agreeing with Devin's and Copilot's flag, confirmed by tracing the call order (runtime_threads.rs:7304-7324 and 7073-7157). monitor_claimed_turn reports the raw monitor_turn error (terminal=false), then unconditionally calls settle_claimed_turn_failure, which re-runs list_items_for_turn/load_turn against the same file and reports again (now correctly terminal=true for the load-turn case). Net effect: duplicate toast, duplicate transcript line, duplicate task-timeline Error entry for one fault. Low severity (cosmetic duplication, not a wrong final state) but a real, reproducible gap in an area whose whole purpose is "show it once, clearly."

Judgment call worth a second look

  1. next_action() recommends deleting the record even for a bare Read failure (runtime_threads.rs:1006-1017, flagged by Codex). Read (permission denied, transient I/O, ENOSPC) and Parse (confirmed-bad JSON) get identical "move it aside (or delete it)" wording. Following that advice on a Read failure that's actually a fixable permissions/mount problem can destroy a healthy record. AGENTS.md is explicit that data-loss handling is never something to cut for brevity — worth splitting the two operations' remedies.

Lower-priority, confirmed but narrow

  • Untranslated {record} noun (event_loop.rs:6701-6706, Codex P1): notice.failure.record_kind's English Display ("thread"/"turn"/"item") is spliced into otherwise-localized notice strings, so non-English locales get an English word mid-sentence. Real, but cosmetic.
  • Startup broadcast-subscription race (Devin, Codex P2): TaskManager spawns workers before event_loop.rs:1178 subscribes to the runtime broadcast channel, so a task that resumes and fails immediately at startup could have its runtime.store_failure toast/transcript line silently dropped (broadcast channels don't replay). Worth noting this doesn't affect the task timeline — ingest_runtime_event runs synchronously off the executor's own event channel, not this broadcast tap — so the fault is still recorded and still fails the task; only the live TUI notice can be missed in that narrow window.
  • Chained .replace() placeholder substitution (event_loop.rs:6700-6709, Codex): if path or reason text happened to contain the literal substring {reason}/{path}, later replacements could corrupt earlier ones. Extremely unlikely in practice (OS paths/error strings don't contain curly-brace placeholders), and it mirrors the pre-existing session_id_divergence_notice pattern a few lines above — not a new class of bug introduced here, just an existing one repeated.
  • RuntimeStoreRecordFailure::from_error's doc claims "any layer" but anyhow::Error::downcast_ref only resolves the outermost .context()/root layer, not arbitrary nesting (Devin). I checked every call site that feeds report_store_failure: all of them pass the store method's Result straight through with ? and no additional .context() wrapping, so this isn't an active bug today — just a footgun if a future caller adds another context layer before the typed error reaches report_store_failure.

What's solid

Lock discipline around _turn_mutation/_projection in settle_claimed_turn_failure is correct (guards drop before .awaits, consistent with the function's existing pattern). Locale coverage is complete across all 15 packs. start_with_executor#[cfg(test)] is a legitimate dead-code fix with no non-test callers. Provider/model routing isn't touched by this PR — no concerns there.
· branch fix/runtime-store-failures-visible-5931

Comment on lines +5665 to +5668
crate::runtime_threads::RUNTIME_STORE_FAILURE_EVENT => Some(sse_json(
crate::runtime_threads::RUNTIME_STORE_FAILURE_EVENT,
payload.clone(),
)),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed gap (traced independently): the mapped SSE event is forwarded fine here, but the stream-closing decision lives one level up in take_compat_turn_event (runtime_api.rs:5453), which only treats event.event == "turn.completed" as terminal. A runtime.store_failure notice with payload["terminal"] == true — which by this PR's stated contract means "no turn.completed can follow" — still leaves take_compat_turn_event's terminal bool false. All three call sites (lines ~5328, ~5349, ~5405) gate yield Ok(sse_json("done", ...)); return; on that bool, so the one-turn compat stream never sends done and stays open on 15s keepalives after a terminal store failure — exactly the "driver waits/idles instead of failing fast" scenario this PR exists to prevent, just for this one consumer.

The one test that touches this event (stream_compat_mapping_forwards_runtime_store_failures, runtime_api/tests.rs:11337) calls map_compat_stream_event directly and never exercises take_compat_turn_event, so the gap has no coverage. Suggest folding this into take_compat_turn_event's terminal check, e.g. event.event == "turn.completed" || (event.event == crate::runtime_threads::RUNTIME_STORE_FAILURE_EVENT && event.payload.get("terminal").and_then(Value::as_bool).unwrap_or(false)).

Comment on lines +7304 to +7309
Ok(Err(error)) => {
let failure = format!("Failed to monitor {}: {error}", kind.label());
// An unreadable item or turn under the monitor is the
// operator's own state: name the file before settling (#5931).
self.report_store_failure(&thread_id, Some(&turn_id), &failure, &error, false)
.await;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed via trace (agrees with Devin's and Copilot's flags on this line): this report_store_failure call (terminal=false) fires on the raw monitor_turn error, then settle_claimed_turn_failure runs immediately after (line 7323) and independently re-runs self.store.list_items_for_turn(turn_id) (line 7073) and self.store.load_turn(turn_id) (line 7116) against the same on-disk fault. If those re-reads hit the same unreadable file — the common case, since nothing repaired it in between — report_store_failure fires again at line 7102 or 7149 (this time terminal=true for the load-turn case), producing two runtime.store_failure events, two toasts, two transcript lines, and two task-timeline Error entries for one underlying fault.

Since the two calls differ only in message text and the terminal flag, one option: have monitor_claimed_turn skip its own report_store_failure when it's about to call settle_claimed_turn_failure anyway (settlement already re-derives the typed failure with the correct terminal classification from its own load/list attempts), and reserve this call site's report for panics/errors that don't route through settlement.

Comment on lines +1670 to +1677
let record: TurnRecord = serde_json::from_str(&raw).with_context(|| {
RuntimeStoreRecordFailure::new(
RuntimeStoreOperation::Parse,
RuntimeStoreRecordKind::Turn,
turn_id,
&path,
)
})?;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirming the Codewhale bot finding with the concrete downstream trace: the schema_version bail! three lines below this (unchanged by this PR) produces a plain anyhow::Error with no RuntimeStoreRecordFailure context, so RuntimeStoreRecordFailure::from_error (line 5405) returns None for it. In settle_claimed_turn_failure (line 7116), this is exactly the "turn's own record is unreadable" case that's supposed to be terminal: true — but here report_store_failure (line 7149) silently returns None and only logs. The turn's on-disk record is never marked Failed either (the load itself failed, so there's nothing to mutate and save), so this one fault mode — a turn record written by a newer binary than the one now reading it — produces zero visible signal: no toast, no transcript line, no task-timeline error, no terminal SSE event. A waiting driver idles out instead of failing fast, which is the exact failure mode this PR exists to close. The same gap applies to load_thread's and load_item's schema checks just above and below this function.

Comment on lines +1006 to +1017
pub fn next_action(&self) -> String {
match self.operation {
RuntimeStoreOperation::Read | RuntimeStoreOperation::Parse => format!(
"Move {} aside (or delete it) and retry; the thread's other records stay in place.",
self.path.display()
),
RuntimeStoreOperation::Write => format!(
"Check free space and permissions for {}, then retry; nothing was overwritten.",
self.path.display()
),
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreeing with Codex's flag on this line: Read and Parse share the same "move it aside (or delete it)" remedy, but they're different failure classes. Parse (JSON deserialization failed) is genuine corruption — deletion is reasonable. Read (read_store_file failed — permission denied, ENOSPC on a temp/lock file, a transient EIO, an NFS hiccup) says nothing about the content being bad; the file may be perfectly healthy and the fix is a permissions/mount problem, not deleting the operator's only copy of a turn or item record. This is the one place in the PR where the notice text can actively cause data loss for a fixable, non-corruption failure — worth splitting the two operations' remedies (e.g. "check permissions and retry; if the file is confirmed corrupt, move it aside" for Read, keep the current wording for Parse).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants