fix(cache): defer TUI warnings until terminal restore - #1065
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
There was a problem hiding this comment.
Pull request overview
This PR fixes suppressed cache persistence warnings during TUI sessions by deferring stderr diagnostics while the TUI owns raw mode/alternate screen, then flushing them after terminal restoration so the once-per-context warning is still visible.
Changes:
- Add a deferred-stderr queue in
tokscale-core(tui_signal) and a helper to emit immediately or defer based on TUI activity. - Route cache persistence warnings through the new deferral mechanism instead of suppressing them (fixing the once-only warning being “consumed” invisibly during TUI).
- Adjust TUI terminal restoration order to flush deferred diagnostics only after leaving the alternate screen / disabling raw mode; add regression tests for deferral and dedup behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| crates/tokscale-core/src/tui_signal.rs | Introduces deferred stderr routing + flush on set_tui_active(false) with race-safe coordination via a shared mutex. |
| crates/tokscale-core/src/message_cache.rs | Switches cache warning fallback to emit_or_defer_stderr and adds regression test for deferral + once-only behavior. |
| crates/tokscale-cli/src/tui/mod.rs | Ensures deferred diagnostics flush only after terminal restore steps so they don’t corrupt the alternate screen. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| static WARNED_CONTEXTS: OnceLock<Mutex<HashSet<&'static str>>> = OnceLock::new(); | ||
| let warned = WARNED_CONTEXTS.get_or_init(|| Mutex::new(HashSet::new())); | ||
| if warned.lock().is_ok_and(|mut warned| warned.insert(context)) | ||
| && !crate::tui_signal::is_tui_active() | ||
| { | ||
| eprintln!("tokscale: warning: {context} ({}): {error}", path.display()); | ||
| if warned.lock().is_ok_and(|mut warned| warned.insert(context)) { | ||
| crate::tui_signal::emit_or_defer_stderr(format!( | ||
| "tokscale: warning: {context} ({}): {error}", | ||
| path.display() | ||
| )); |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
@junhoyeo pls review, thank you💗 |
|
Reviewed as part of a sweep across the open PRs. The diagnosis is exactly right — recording the warning context before checking whether the TUI owns the terminal consumed the once-per-process budget while suppressing the only visible output — and deferring to terminal-restore is the right shape. I walked the lock order in Both bot threads are pointing at the same real thing and I would fix it:
let mut warned = WARNED_CONTEXTS
.lock()
.unwrap_or_else(|poisoned| poisoned.into_inner());Two smaller notes:
One pre-existing gap this PR does not cause but sits right next to: Note no build or test leg ran on this branch — the first-time-contributor gate meant only cubic executed — so worth approving a CI run before merge. |
|
Approved the workflow runs so this branch finally got a build. Results, and one thing you will want to know before rebasing: Lint and Ubuntu tests pass. Windows failed — but not because of your change. All three assert The part worth acting on: the run's top-level conclusion still says That is not a reason to avoid rebasing; it just means a red Windows leg after the rebase is most likely this flake rather than anything you introduced, and re-running the job is the first thing to try. For reference, Probably worth a separate issue about the 8-second budget: a wall-clock assertion that tight is fragile on a contended Windows runner, and it matters more now that the leg is a hard gate than it did when it was advisory. The poisoned-mutex fix in my earlier comment still stands as the one thing I would change before merge. |
|
Checked the two unresolved review threads from @copilot-pull-request-reviewer and cubic — they are both right, and I want to add the verification plus one sequencing note that matters for landing this. The poisoned-mutex drop is real, and it defeats the PR's own stated purposeAt the current head, if warned.lock().is_ok_and(|mut warned| warned.insert(context)) {If The fix the reviewers suggest is already the established idiom in the module this PR routes through — So Sequencing: do not rebase yetThis branch's base predates #1068, so its Windows leg is still advisory. That is why its last run reported The failure is not caused by anything in this PR. All three That is a pre-existing timing flake, now tracked as #1078. The consequence for this PR is concrete: the moment you rebase onto a base containing #1068, that flake becomes a hard red gate on your branch, through no fault of your change. A fix for #1078 is in progress, so it is worth rebasing after it lands rather than before — otherwise a red Windows leg here will look like your bug. So: the poison-recovery fix is the one substantive item, and the rebase is worth holding briefly. |
…face warn_cache_failure_once gated the once-per-process fallback on `lock().is_ok_and(...)`, so a poisoned WARNED_CONTEXTS mutex silently dropped the diagnostic instead of emitting it. That is the exact silent failure the fallback exists to prevent. Recover from poisoning with `unwrap_or_else(|poisoned| poisoned.into_inner())`, matching how tui_signal already handles all of its lock sites, and hoist the static so the poisoned path is testable. Once-per-context semantics and the TUI deferral are unchanged.
|
Pushed df1b6de to this branch, fixing the poisoned-mutex hole that @copilot-pull-request-reviewer and cubic both flagged independently on this PR. They were right, and it was the one thing in here that undercut the fix's own premise. What was wrong
if warned.lock().is_ok_and(|mut warned| warned.insert(context)) {If
The fixSame idiom as if warned_contexts()
.lock()
.unwrap_or_else(|poisoned| poisoned.into_inner())
.insert(context)
{The set only records which contexts were already reported, so its contents stay meaningful across an unwind — recovering is correct here, not just convenient. Once-per-context semantics and the TUI deferral behavior are unchanged; the fallback still defers rather than writing raw stdio mid-render, which is the whole point of #941 and this PR. The static moved from function-local to module-level behind a Red-green proofNew test Against the previous After the fix: Gates
Not rebased on main deliberately: this branch's base predates #1068, and rebasing would pull in the pre-existing timing flake tracked in #1078 (fix in #1081) as a hard red gate unrelated to this change. |
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…poisoned set The poisoned-set regression test reached for process-global state in two ways that can disturb tests running beside it. It swapped the process-global panic hook for a no-op to keep its deliberate panic out of the test output. That suppresses panic diagnostics for whatever else runs in parallel, and races with any other hook user. The unwind happens on the test's own thread, which libtest already captures, so the hook swap is dropped entirely and the expected message now prints only when the test fails. It also set TUI_ACTIVE and restored it by hand just before its final assertion, so a panic anywhere in between leaked the mutation into every later test in the binary. Add TuiActiveGuard next to the state it guards, mirroring paths::test_env::EnvGuard: it restores the previous value on Drop, drains what the test deferred rather than printing it on the real stderr, and carries its own proof that it restores while unwinding. Both message_cache tests and the tui_signal test now use it. Finally, the test poisoned the process-global once-only set, which is irreversible and left every later test depending on the recovery under test. warn_cache_failure_once now delegates to warn_cache_failure_once_in, which takes the set as a parameter, so the test can poison one scoped to itself. Production has a single caller that always passes warned_contexts(), and the deferral test still exercises the real global set, so the once-per-process, once-per-context semantics are unchanged.
|
Both of these were findings against the regression test we added in df1b6de ( The test replaced the process-global panic hookIt swapped Resolved by not touching the hook at all. The unwind happens on the test's own thread, which libtest already captures, so the expected panic message is simply captured and printed only if the test fails — confirmed: the message appears under Non-panic-safe restore of the process-global TUI stateThe old Resolved with The guard carries its own proof ( I also moved the adjacent Permanent poisoning of the process-global once-only setWe flagged this ourselves: the test poisoned the real Scoped it. Red-green re-verifiedThe test still genuinely proves the poisoned-mutex behaviour. Reverting the production line to With the fix restored: Gates
The lib suite was also repeated 5x in parallel with no flakes. |
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…g queued diagnostics TuiActiveGuard::drop transitioned to inactive before restoring self.previous. The inactive transition drains and permanently discards the process-global deferred-stderr queue, so a guard whose captured previous value was active tore the state down, threw away diagnostics an enclosing scope still owed its eventual terminal restore, and only then set the flag back to active. Restore in a single transition instead. transition_tui_active already ties the queue to the state it moves to: restoring to inactive still drains, which is what keeps a test's synthetic diagnostics off the real stderr, and restoring to active now leaves the queue for whoever owns the terminal. Drop stays panic-safe, so an unwind still restores. Covered by a nested-capture test asserting the deferred message survives the inner guard's drop.
|
Follow-up on a finding against code we added in this branch (the test-only
The second-order risk was the one worth fixing for: a test that defers a message and then panics before asserting it has its diagnostics swallowed by the drop — which would mask exactly the deferral regression this PR exists to prevent. Resolved by restoring in a single transition. Red-green, all three guarantees re-checked:
The new test nests its guards so the outermost one still restores to inactive and drains, and its own assertion consumes the queue, so no TUI-active state or queued diagnostic leaks into a later test. Gates, in default parallel mode (no The lib suite was run 5 more times in parallel mode to check for flakes: |
What changed
Why
warn_cache_failure_oncepreviously recorded the warning context before checking whether the TUI was active. A cache failure during a TUI session therefore consumed the once-per-process warning while suppressing its only visible output, leaving users with repeated cold reparses and no diagnostic.This addresses the cache-warning subtask in #941 without closing the umbrella issue.
Validation
cargo fmt --all -- --checkcargo clippy --locked --workspace --all-features -- -D warningscargo test --workspace --all-features: 2,665 tests passed; one unrelated permission test fails under a root container because root can read achmod 000fixture. The failure reproduces on cleanupstream/main, and the same test passes as a non-root user.Summary by cubic
Defers cache warnings while the TUI owns the terminal and flushes them after restore, preventing screen corruption and ensuring the once-per-context warning shows. Hardens the warning path against a poisoned once-only set, and fixes the test guard to preserve deferred output in nested active scopes.
tui_signal::emit_or_defer_stderr; keep once-per-context behavior without consuming it during TUI.set_tui_active(false)last; coordinate state and queue behind a shared lock to avoid enqueue-after-flush races.unwrap_or_else(...into_inner()); factorwarn_cache_failure_once_inso tests isolate a poisoned set.TuiActiveGuard; restore previous state in a single transition so nested active scopes keep their deferred diagnostics; add regression test.Written for commit 314435d. Summary will update on new commits.