Skip to content

fix(tui): stop background cache-warning writes from corrupting TUI screen - #906

Merged
junhoyeo merged 2 commits into
mainfrom
fix/tui-cache-warning-corruption
Jul 17, 2026
Merged

fix(tui): stop background cache-warning writes from corrupting TUI screen#906
junhoyeo merged 2 commits into
mainfrom
fix/tui-cache-warning-corruption

Conversation

@junhoyeo

@junhoyeo junhoyeo commented Jul 16, 2026

Copy link
Copy Markdown
Owner

Fixes #893

Root causes

Two independent, stackable issues cause the TUI's screen to get corrupted when a background cache shard save fails:

  1. Why the write fails on Windows (Access is denied (os error 5)): fs_atomic::windows_replace_file calls MoveFileExW(..., MOVEFILE_REPLACE_EXISTING | MOVEFILE_WRITE_THROUGH) exactly once, with no retry. This is a well-known source of transient ERROR_ACCESS_DENIED (5) / ERROR_SHARING_VIOLATION (32) on Windows — antivirus, indexing, and cloud-sync agents routinely hold a brief scan handle open on a just-written file. fs_atomic::replace_file is the single shared atomic-rename primitive used by 10 call sites across both crates, so this failure mode isn't unique to the cache.

  2. Why the failure corrupts the TUI display: message_cache::warn_cache_failure_once always calls tracing::warn!, then falls back to a raw eprintln! the first time each failure context is seen — because most non-TUI commands never install a tracing subscriber, so tracing::warn! alone would be silent. But the TUI only installs a subscriber under --debug. In normal (non-debug) use, that eprintln! writes directly to stdio while the TUI owns raw mode and the crossterm alternate screen, corrupting the rendered display exactly as reported.

Fix

  • fs_atomic.rs: wrap the Windows MoveFileExW call in a bounded retry loop (5 attempts, ~10ms × attempt backoff, ≤100ms worst case), retrying only on the two known-transient error codes. Benefits all 10 existing replace_file call sites with no call-site changes. Non-Windows path (std::fs::rename) is untouched.
  • New tui_signal.rs: a small process-wide AtomicBool flag (set_tui_active/is_tui_active), mirroring the existing style of paths::is_config_dir_overridden().
  • message_cache.rs: gate the eprintln! fallback in warn_cache_failure_once on !tui_signal::is_tui_active(). tracing::warn! still fires unconditionally and is unchanged. Non-TUI command behavior is byte-for-byte unchanged.
  • tui/mod.rs: toggle the flag to true right after the TUI successfully enters the alternate screen, and back to false in both terminal-restore paths (normal exit and the panic hook's best-effort restore), so it can never get stuck.

Verification

  • cargo build -p tokscale-core -p tokscale-cli — clean
  • cargo clippy -p tokscale-core -p tokscale-cli --all-targets -- -D warnings — zero warnings
  • cargo test -p tokscale-core — 1216 passed, 0 failed, including a new tui_signal::tests::round_trips test
  • cargo fmt --check — clean
  • Manual review of the unsafe extern "system" MoveFileExW retry loop — no Windows machine available locally to compile/run the #[cfg(windows)] path; relying on the existing build-native.yml Windows CI job plus this manual review

Not tested: actual Windows MoveFileEx retry behavior under real transient file-lock contention (AV/indexer/cloud-sync), since no Windows machine is available in this environment.


Summary by cubic

Prevents TUI screen corruption when background cache save warnings fire by suppressing raw stderr during TUI and making Windows file replacement more resilient. Fixes #893.

  • Bug Fixes

    • Windows: retry MoveFileExW in fs_atomic::replace_file (up to 5 attempts with short backoff) on transient ERROR_ACCESS_DENIED/ERROR_SHARING_VIOLATION. Non-Windows unchanged.
    • TUI: add tokscale-core::tui_signal and toggle it from tokscale-cli when entering/leaving the TUI; suppress message_cache::warn_cache_failure_once eprintln! while active. tracing::warn! unchanged; non-TUI behavior remains the same.
  • Refactors

    • Run workspace-wide cargo fmt to satisfy CI; unrelated files only, no behavior changes.

Written for commit eb31d94. Summary will update on new commits.

Review in cubic

…reen

The TUI's screen occasionally gets scrolled/corrupted on a background
cache save failure (#893). Two stackable root causes:

1. On Windows, MoveFileExW replacing the cache file is a well-known
   source of transient ERROR_ACCESS_DENIED/ERROR_SHARING_VIOLATION
   when AV/indexer/cloud-sync briefly holds a scan handle open on the
   just-written file. fs_atomic::windows_replace_file made a single
   attempt with no retry.
2. warn_cache_failure_once() falls back to a raw eprintln! the first
   time each failure context is seen, since most non-TUI commands
   never install a tracing subscriber. But the TUI only installs one
   under --debug, so in normal use that eprintln! is the only visible
   output — and it writes directly to stdio while the TUI owns raw
   mode and the crossterm alternate screen, corrupting the display.

Fixes both: retry the Windows rename a few times with a short backoff
on the specific transient errors (benefits all 10 existing
fs_atomic::replace_file call sites, not just the cache), and add a
process-wide tui_signal flag the TUI sets while it owns the terminal
so the eprintln! fallback is suppressed during that window instead of
corrupting the screen.

Constraint: fs_atomic::replace_file is a single shared primitive used
by 10 call sites across both crates — the retry must be transparent
with no call-site changes
Rejected: Redirect all cache warnings through the TUI's own
status/error UI | would require threading a channel through every
non-TUI call site too; the flag-gated eprintln! keeps non-TUI
behavior byte-for-byte unchanged
Confidence: high
Scope-risk: narrow
Not-tested: actual Windows MoveFileEx retry behavior, no Windows
machine available in this environment — relying on build-native.yml
CI's Windows job plus manual FFI review
@vercel

vercel Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
tokscale Ignored Ignored Preview Jul 16, 2026 6:48pm

Request Review

@cubic-dev-ai cubic-dev-ai 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.

No issues found across 5 files

Re-trigger cubic

CI's Lint job runs cargo fmt --all -- --check across the whole
workspace, not just this PR's diff. Two unrelated files on main had
already drifted out of rustfmt's canonical form before this branch
forked, which was failing this PR's Lint check despite neither file
being touched by the TUI fix itself.

Constraint: cargo fmt --all -- --check must pass workspace-wide, not just on changed files
Confidence: high
Scope-risk: narrow
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Spurious cache shard save warnings break the TUI

1 participant