fix(commands): take the blocking Tauri commands off the main thread (and correct the count: 82, not 38) - #524
Conversation
Tauri dispatches a synchronous `#[tauri::command]` on the main thread. That is not a documentation claim: `tauri-macros` 2.6.3 starts `WrapperAttributes` at `ExecutionContext::Blocking` and only moves it to `Async` when the function is declared `async` or carries `#[tauri::command(async)]`, and a `Blocking` command is invoked inline on the dispatching thread. On Linux that thread is the GTK thread, so any wait inside such a command freezes the whole window for its duration, with no spinner and nothing the user can tell apart from a crash. #515 already fixed one instance of this. The count this work was scoped with was wrong, and that matters more than any single conversion. It came from a regex anchored on `pub fn` immediately after the attribute, which silently skips every command declared inside a nested `mod` -- that is, most of lib.rs. It reported 38. There are 82, out of 853, and the 44 it missed are the sync index / journal / profile / snapshot / cloud-config family, which is JSON read and write on the main thread. Thirty commands become `pub async fn` with the blocking part inside `tokio::task::spawn_blocking`, the form already merged in portal_chooser.rs, each with its reason in a doc comment. The one that led is `filesystem::list_subdirectories`: it takes its path straight from the frontend, stats it twice, `read_dir`s it, stats every entry and `read_dir`s each subdirectory again for the expand chevron, with no timeout anywhere in our code. On a dead NFS/SMB/SSHFS mount that is the window frozen for the mount's own timeout; on the blocking pool it is one tree node stuck on "Loading...". Three claims in the scoping notes did not survive reading the code and are corrected here: `local_sync_cancel` does one atomic store rather than `File::open` and stays synchronous; `hash_text` hashes a string that arrived over IPC rather than opening a file, so it moves for CPU duration, not I/O; and the four `native_rsync_*` accessors, listed as safe to leave, all reach `native_rsync.toml` on disk. `totp`, `pty` and `local_panel_watcher` needed a design change rather than a rename: `State<'_, T>` borrows the app and cannot enter `spawn_blocking`, so those states now hold their mutex behind an `Arc` and the commands clone a `'static` handle out. Converting only the command that blocks would not have been enough -- they share one mutex, so a synchronous `totp_status` would keep the keystore-write freeze reachable, and a synchronous `pty_resize` would keep `pty_write`'s blocked write reachable by resizing the terminal. Every guard is taken inside the closure: a `MutexGuard` held across an `.await` is `clippy::await_holding_lock`, which is what turned a claimed-green gate red on #515. Two internal callers are fixed rather than worked around: the CLI called `settings::native_rsync_mode_get()` from plain `fn`s, so the synchronous body is now `native_rsync_mode_str()` and the CLI calls that; and 24 test cases drove `hash_text` synchronously, so they drive the named blocking body. `load_secret_internal` lost its last caller and is deleted rather than kept alive with an `#[allow(dead_code)]`. Pins, all three verified by breaking them rather than by reading them: * per command, at compile time: `list_subdirectories` and `hash_text` get a test that `block_on`s them. Reverting `list_subdirectories` to `pub fn` produces E0277, "is not a future", instead of an assertion someone can delete; * class level, addition: a scratch file declaring a new synchronous command turns the audit red, naming it at file and line; * class level, drain: a name left in the pending list after its command went async turns the audit red, demanding the list shrink. The class pin reads the sources rather than reflecting over the crate, so it sees commands behind a `#[cfg]` this build did not compile. It asserts set equality against MAIN_THREAD_ALLOWED (8, justified, each with its reason) plus MAIN_THREAD_NOT_YET_MOVED (74, frozen). The pending list can only shrink; adding to it is not a way to get green. Those 74 live almost entirely in lib.rs and follow in the next PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 58 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthroughChangesThe pull request converts numerous blocking Tauri commands to asynchronous wrappers using Async Tauri command migration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Local gate, with the exit codes read without a pipeEvery step redirected to a file and read
The first run of the gate was red, and it found something real: The pins, verified by breaking themReading a pin does not tell you it holds. All three were made to fail on purpose and then restored. Per command, at compile time. Reverting Not a failing assertion someone can delete: the test stops building. Class pin, the addition direction — the failure mode that actually recurs. A scratch file declaring one new synchronous command, deliberately not declared as a module so nothing recompiles, which is also what proves the scan reads sources rather than reflecting over the crate: Class pin, the drain direction. A name left in the pending list after its command went async: That second direction is what stops the list becoming a record of what used to be true. It can only shrink. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src-tauri/src/pty.rs`:
- Around line 78-80: Update the spawn_shell flow and spawn_shell_blocking setup
to reserve a PTY slot atomically under the manager lock before creating the
child process, so concurrent requests cannot exceed MAX_PTY_SESSIONS. Track the
reservation as pending, convert it to the active session on successful setup,
and release it on every setup failure or task error; do not rely on a late
capacity recheck after spawning.
- Around line 233-236: The PTY session-manager lock is held across blocking I/O,
preventing resize and close while a writer is stuck. In src-tauri/src/pty.rs
lines 233-236, refactor pty_write_blocking to use per-session synchronization or
a dedicated I/O worker; at lines 259-269, release the manager lock before
acquiring the per-session state needed for resize; at lines 302-307, let close
remove or cancel the session without waiting for an active writer.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1ed179c7-e111-482d-b0e0-f75441db1fa6
📒 Files selected for processing (14)
src-tauri/src/aerocrypt_provider.rssrc-tauri/src/ai_tools.rssrc-tauri/src/bin/aeroftp_cli.rssrc-tauri/src/cyber_tools.rssrc-tauri/src/filesystem.rssrc-tauri/src/lib.rssrc-tauri/src/local_panel_watcher.rssrc-tauri/src/peer_commands.rssrc-tauri/src/provider_commands.rssrc-tauri/src/pty.rssrc-tauri/src/settings.rssrc-tauri/src/sync_command_audit.rssrc-tauri/src/totp.rssrc-tauri/src/vault_remote.rs
…anager across a write Both from the CodeRabbit review on #524, and the first one is a regression this PR introduces rather than a pre-existing defect, which is why it is fixed here and not filed. `spawn_shell` checked the session count, released the manager lock, opened the PTY and forked the shell, and only then re-acquired the lock to insert. That is a time-of-check-to-time-of-use gap: two calls both see 19 sessions and both insert, and MAX_PTY_SESSIONS is quietly 21. It was unreachable while the command was synchronous, because Tauri runs those on the main thread and the main thread does one thing at a time. Taking the work off that thread is exactly what makes it reachable, so the fix belongs with the move. The check and the insert now share one acquisition, and the reserved slot is held by a guard whose Drop hands it back, so a spawn that fails part way through cannot leak a slot and shrink the cap for the rest of the session. Second: `pty_write` held the single manager lock across `write_all`, which blocks for as long as the child refuses to drain the master. On the main thread that froze the window, which is the bug this PR is about; off it, it would have parked every other session's write, every resize and every close behind one wedged child. Sessions now sit behind their own locks and the manager is released as soon as the handle is cloned out, so a wedged child costs only its own session. `pty_close` deliberately takes only the manager lock, so closing works while a write to that same session is stuck. Three pins, and the first one had to be rewritten before it was one. Spawning 32 threads in a loop and asserting the cap passes against the racy shape, because the early threads finish before the late ones start: one round of a race proves nothing. With a barrier and 200 rounds it fails at round 1 with 23 sessions against a cap of 20, which is the defect reproduced, and passes on the fix. The other two cover the dropped reservation returning its slot, and the manager staying acquirable while a session is busy. What this does not do, stated so nobody reads more into it: closing removes the session from the map, so every later command reports "session not found", but it does not interrupt a write already in the kernel, because the writer owns a handle taken from the master and dropping our side does not unblock it. That costs one parked blocking-pool thread per wedged child until it dies or drains. Interrupting it means a non-blocking writer with its own cancellation, which is a different change from this one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Part of #517.
The defect
Tauri dispatches a synchronous
#[tauri::command]on the main thread. That is not a documentation claim, it is in the macro:tauri-macros2.6.3 startsWrapperAttributesatExecutionContext::Blockingand only moves it toAsyncwhen the function is declaredasyncor carries#[tauri::command(async)]; aBlockingcommand is invoked inline on the dispatching thread. On Linux that thread is the GTK thread, so any wait inside such a command freezes the entire window for its duration — no spinner, and nothing the user can tell apart from a crash. #515 already fixed one instance (portal_chooser::chooser_unavailable, up to two seconds on a wedged session bus).The count was wrong, and that is the most useful thing in this PR
The work was scoped at "38 synchronous commands". That number came from a regex anchored on
pub fnimmediately after the attribute, which silently skips every command declared inside a nestedmod— that is, most oflib.rs.There are 82 synchronous commands out of 853. The 44 that the original count missed are not marginal: they are the sync index / journal / profile / snapshot / cloud-config / versioning family, which is JSON read and write against the config directory, all of it on the main thread.
Three other claims in the scoping notes did not survive reading the code, and are corrected here:
local_sync_canceldoesFile::openAtomicBool::store. It stays synchronous, and that is now written down with the reason.hash_textdoesFile::openplus hashing, linear in the filehash_fileis the one that opens a file, and it was alreadyasync.native_rsync_enabled_get/mode_get/enabled_set/mode_setcan stay synchronousnative_rsync.toml: the getters stat and read it, the setters take a process-wide write lock and then write and rename. Moved.What this PR moves, and why each one
Thirty commands become
pub async fnwith the blocking part insidetokio::task::spawn_blocking, the form already merged inportal_chooser.rs. Each carries the reason in a doc comment; the ones that are more than mechanical:filesystem::list_subdirectories— the worst of them, and the reason it goes first. It takes its path straight from the frontend, stats it twice,read_dirs it, stats every entry, andread_dirs each subdirectory again for the expand chevron. That is O(entries) syscalls with no timeout anywhere in our code, so on a dead NFS/SMB/SSHFS mount it blocks in the kernel for as long as that mount was configured to wait. Synchronous, that is a frozen window; on the blocking pool it is one tree node stuck on "Loading…" while the app keeps working.filesystem::volumes_changed(Linux) —/proc/mountsalways answers, but/run/user/N/gvfsis a FUSE mount served bygvfsd, and listing it blocks when a share behind it is unreachable. The frontend polls it every 30 seconds, so synchronously this is a periodic freeze with no user action to attribute it to. The Windows and fallback variants move too, so the command does not change asyncness with the target.vault_remote::vault_v2_cleanup_temp— overwrites every byte of the temp vault in 1 MB chunks and thensync_all()s. Duration is linear in the file and ends in an fsync.ai_tools::clipboard_read_image— arboard 3.6.1 gives the X11 selection exchange a 4000 ms budget (LONG_TIMEOUT_DUR), so a slow clipboard owner is up to four seconds of frozen GTK thread; plus base64 of a full-resolution image (~33 MB of RGBA for a 4K screenshot). Checked rather than assumed that this is safe off the main thread on all three platforms: arboard's macOSClipboardis declaredSend + Sync, its Windows one opens and closes the clipboard inside each call so the!Sendhandle never escapes, and the X11 backend runs its own connection.totp::*(7),pty::*(4),local_panel_watcher::*(2) — these needed a small design change, not a rename:State<'_, T>borrows the app and cannot enterspawn_blocking, soTotpStateandLocalPanelWatcherStatenow hold their mutex behind anArcand the commands clone a'statichandle out. Converting only the one command that blocks would not have been enough: they all share one mutex, so leavingtotp_statussynchronous would keep the keystore-write freeze reachable through it, and leavingpty_resizesynchronous would keeppty_write's blocked write reachable by resizing the terminal.provider_commands::aerocrypt_profile_recovery_kit/aerocrypt_verify_recovery_kit— keystore reads, which on Linux are the Secret Service over D-Bus: another process, and one that can be wedged. Same shape as the freeze fix(linux): stop the file picker failing silently, in all 68 places #515 fixed.Every guard is taken inside the closure. A
std::sync::MutexGuardheld across an.awaitisclippy::await_holding_lock, and that lint is the one that turned a claimed-green gate red on #515.Two internal callers broke and are fixed rather than worked around: the CLI called
settings::native_rsync_mode_get()from plainfns, so the synchronous body is nownative_rsync_mode_str()and the CLI calls that (a CLI has no main thread to protect); and 24 test cases drovehash_textsynchronously, so they now drive the namedhash_text_blockingbody.The pins
Per command, at compile time.
list_subdirectoriesandhash_textget a#[test]thatblock_ons them.block_ononly accepts a future, so turning either back into apub fnstops the test building (E0277) instead of failing an assertion someone can delete. Each also asserts the wrapper returns what the blocking body computed, so it is not only a type-level check.Class level,
src-tauri/src/sync_command_audit.rs. The per-command form defends only the commands that already have such a test; it does nothing about the failure mode that actually recurs, which is addition — someone writes a new command in six months, writes it synchronous because that is the shorter spelling, and the count climbs back. So this test reads the sources, collects every synchronous command, and asserts the set is exactlyMAIN_THREAD_ALLOWED(justified, 8 entries, each with its reason) plusMAIN_THREAD_NOT_YET_MOVED(frozen, 74 entries). It reads text rather than reflecting over the crate on purpose: that way it sees commands behind a#[cfg]this build did not compile, which is exactly where a platform-specific one would otherwise hide.It is a set equality in both directions, so it cannot rot: a new synchronous command is red, converting a pending one without deleting its line is red, and an allowlisted command that becomes async is red. The pending list can only shrink — adding to it is not a way to get green.
MAIN_THREAD_ALLOWEDholds the eight that are justified, including two that must stay on the main thread and now say so:aeroshare_notifyand (in the pending list until its entry is written)rebuild_menumarshal their work onto the main thread throughrun_on_main_thread, andtauri-runtime-wry'ssend_user_messageruns the closure inline when the caller already is that thread — so making them async would add a trip through the event loop without moving any work.What is left, and when
MAIN_THREAD_NOT_YET_MOVEDlists the 74 remaining by name. They are mostly thelib.rsconfig/journal/snapshot family plus a handful that touch GTK and will end up justified instead once each has been read. That work follows in the next PR from this same session; the ratchet is what makes it not optional, and what makes this PR honest about not being the whole job.Splitting is deliberate: those 74 live almost entirely in
lib.rs, which several branches are editing right now, and holding thelist_subdirectoriesfix behind a 3000-line mechanical diff helps nobody.Verification
Local gate and its results go in a comment below rather than here, so the claim and the evidence arrive together.
Summary by CodeRabbit
Performance & Reliability
File Monitoring
Tests