fix(commands): drain the rest of the main-thread commands out of lib.rs (82 to 23) - #532
Conversation
Second and last pass. The previous commit moved the commands that live outside
lib.rs and froze the remainder in MAIN_THREAD_NOT_YET_MOVED, a list that could
only shrink. This empties it, so the list is deleted rather than carried: 59
more commands become `async` with their body on the blocking pool, and the 15
that are left synchronous are in MAIN_THREAD_ALLOWED with the reason written
next to each.
The bulk is the sync index / journal / profile / snapshot / cloud-config /
versioning family, which is JSON read and write against the config directory,
all of it previously on the GTK thread. Three are worth naming because they are
not short at all: `versions_disk_usage` walks a directory tree, and its cost
grows with the user's own version history; `verify_local_transfer` hashes a
file; `safe_picker_start_dir` walks up a path supplied by the frontend with an
`is_dir()` per level, so an unreachable mount stalls it with no bound of ours.
Of the 15 that stay, three must: `restart_app` drives the event loop,
`toggle_menu_bar` calls GTK window operations, and `rebuild_menu` already
marshals menu construction onto the main thread through a channel, which
tauri-runtime-wry runs inline when the caller is already that thread, so making
it async would add a round trip and move no work. The other twelve are bounded
by this crate rather than by the caller: atomic loads and stores, `default()`
structs, compile-time constants, argv and environment reads, one HMAC, and
string classification of an error we produced ourselves.
`get_default_cloud_folder` was on the list of candidates to justify and did not
survive the check: `CloudConfig::default()` calls `dirs::document_dir()`, which
on Linux reads `~/.config/user-dirs.dirs`. That is disk, so by the bar written
above MAIN_THREAD_ALLOWED it gets converted like the rest rather than excused.
Two mechanical notes, both from getting them wrong first:
* the generator initially took the return type with `rindex(')')`, which
picks the paren inside `Result<(), String>` and silently mangles every
command that returns unit. It matches the parameter list by depth now;
* a zero-argument wrapper passes the function rather than `move || f()`,
which is `clippy::redundant_closure`, and a plain-value fallback logs the
JoinError rather than dropping it, which also binds `err` as `-D warnings`
requires. `verify_local_transfer` fails closed on that path, with
`passed: false` and a message, because a verification that never ran did
not pass.
Internal callers fixed rather than worked around: lib.rs's own
`safe_picker_start_dir_tests` drive the blocking body directly, and
`PortableInfo` and `SystemInfo`, both private to this file, derive `Default` so
the fallback is a value rather than a literal that would drift from the struct.
82 synchronous commands out of 853 at the start of this work, 23 now, every one
of them explained where the test can check it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 28 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. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughTauri commands performing blocking system, filesystem, sync, transfer, cloud, and platform operations now use async wrappers with ChangesAsync command migration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 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. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src-tauri/src/lib.rs (6)
15885-15897: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftSame load-modify-save race, on
CloudConfig.conflict_strategy.
update_conflict_strategy_blockingreadsCloudConfig, mutatesconflict_strategy, then saves — vulnerable to concurrent-invocation lost updates now that the command runs on the blocking pool. See consolidated comment.🤖 Prompt for 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. In `@src-tauri/src/lib.rs` around lines 15885 - 15897, Update update_conflict_strategy_blocking to perform the CloudConfig load, conflict_strategy mutation, and save under the same synchronization mechanism used by other CloudConfig load-modify-save operations, preventing concurrent invocations from overwriting each other. Keep update_conflict_strategy’s blocking-task behavior and error propagation unchanged.
15293-15313: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftSame load-modify-save race as
add_cloud_pair_blocking.
update_cloud_pair_blockingreads, mutates, then savesCloudPairsConfigin one call with no synchronization against concurrent invocations. See consolidated comment.🤖 Prompt for 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. In `@src-tauri/src/lib.rs` around lines 15293 - 15313, Synchronize the read-modify-save sequence in update_cloud_pair_blocking with the same locking mechanism used by add_cloud_pair_blocking. Hold the lock across cloud_pairs::load_cloud_pairs_config, the pair mutation, and cloud_pairs::save_cloud_pairs_config so concurrent updates cannot overwrite each other.
15223-15271: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftLoad-modify-save on shared
CloudPairsConfigis no longer atomic underspawn_blocking— breaks the duplicate-pair invariant.
add_cloud_pair_blockingloads the config, checkspair_existsagainst that snapshot, then pushes and saves. Two concurrentadd_cloud_pairinvocations can both pass the duplicate check against the same stale snapshot and then overwrite each other's save, silently dropping one pair (or admitting a duplicate the check was meant to prevent). This guarantee held for free when the command ran serially on the main thread; it no longer does under the blocking pool. See consolidated comment for the full pattern and suggested fix.🤖 Prompt for 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. In `@src-tauri/src/lib.rs` around lines 15223 - 15271, Make the load, duplicate check, append, and save sequence in add_cloud_pair_blocking atomic across concurrent invocations by guarding it with a shared mutex or equivalent synchronization used by every CloudPairsConfig mutation path. Preserve the existing duplicate-pair rejection and return the fully updated configuration after the protected save; ensure the lock is held for the entire read-modify-write operation, not just individual steps.
13295-13323: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftLoad-modify-save on shared
MultiPathConfigis no longer atomic underspawn_blocking.
add_path_pair_blocking/remove_path_pair_blockingload the full config, mutatepairs, then save — previously implicitly atomic on the single-threaded main-thread dispatch, now racy across concurrent blocking-pool invocations (lost updates). See consolidated comment for full analysis and suggested fix.🤖 Prompt for 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. In `@src-tauri/src/lib.rs` around lines 13295 - 13323, The add_path_pair_blocking and remove_path_pair_blocking load-modify-save sequences must be serialized to prevent concurrent updates from overwriting each other. Introduce or reuse a shared synchronization guard around the full load, mutation, and save operation in both functions, while preserving their existing results and error propagation.
15627-15652: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftSame load-modify-save race, on
CloudConfig.enabled/paused.
enable_aerocloud_blockingreadsCloudConfig, mutatesenabled/paused, then saves — vulnerable to concurrent-invocation lost updates now that the command runs on the blocking pool. See consolidated comment.🤖 Prompt for 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. In `@src-tauri/src/lib.rs` around lines 15627 - 15652, Update enable_aerocloud_blocking to serialize the load-modify-save operation for CloudConfig.enabled and CloudConfig.paused, using the same synchronization mechanism established by the consolidated fix. Ensure concurrent enable_aerocloud invocations cannot overwrite each other’s updates, while preserving validation, folder setup, state changes, and persistence behavior.
15274-15290: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftSame load-modify-save race as
add_cloud_pair_blocking.
remove_cloud_pair_blockingreads, mutates, then savesCloudPairsConfigin one call with no synchronization against concurrent invocations. See consolidated comment.🤖 Prompt for 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. In `@src-tauri/src/lib.rs` around lines 15274 - 15290, Synchronize the read-modify-save sequence in remove_cloud_pair_blocking using the same locking mechanism established for add_cloud_pair_blocking. Ensure concurrent add, remove, and other cloud-pair updates cannot interleave between loading and saving the CloudPairsConfig, while preserving the existing “Pair not found” behavior.
🤖 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/lib.rs`:
- Around line 15317-15329: Protect the load-modify-save sequence in
update_excluded_folders_blocking with the same synchronization mechanism used
for other CloudConfig updates, so concurrent invocations cannot overwrite each
other. Keep the excluded_folders assignment and cloud_config::save_cloud_config
operation within the lock’s critical section.
---
Outside diff comments:
In `@src-tauri/src/lib.rs`:
- Around line 15885-15897: Update update_conflict_strategy_blocking to perform
the CloudConfig load, conflict_strategy mutation, and save under the same
synchronization mechanism used by other CloudConfig load-modify-save operations,
preventing concurrent invocations from overwriting each other. Keep
update_conflict_strategy’s blocking-task behavior and error propagation
unchanged.
- Around line 15293-15313: Synchronize the read-modify-save sequence in
update_cloud_pair_blocking with the same locking mechanism used by
add_cloud_pair_blocking. Hold the lock across
cloud_pairs::load_cloud_pairs_config, the pair mutation, and
cloud_pairs::save_cloud_pairs_config so concurrent updates cannot overwrite each
other.
- Around line 15223-15271: Make the load, duplicate check, append, and save
sequence in add_cloud_pair_blocking atomic across concurrent invocations by
guarding it with a shared mutex or equivalent synchronization used by every
CloudPairsConfig mutation path. Preserve the existing duplicate-pair rejection
and return the fully updated configuration after the protected save; ensure the
lock is held for the entire read-modify-write operation, not just individual
steps.
- Around line 13295-13323: The add_path_pair_blocking and
remove_path_pair_blocking load-modify-save sequences must be serialized to
prevent concurrent updates from overwriting each other. Introduce or reuse a
shared synchronization guard around the full load, mutation, and save operation
in both functions, while preserving their existing results and error
propagation.
- Around line 15627-15652: Update enable_aerocloud_blocking to serialize the
load-modify-save operation for CloudConfig.enabled and CloudConfig.paused, using
the same synchronization mechanism established by the consolidated fix. Ensure
concurrent enable_aerocloud invocations cannot overwrite each other’s updates,
while preserving validation, folder setup, state changes, and persistence
behavior.
- Around line 15274-15290: Synchronize the read-modify-save sequence in
remove_cloud_pair_blocking using the same locking mechanism established for
add_cloud_pair_blocking. Ensure concurrent add, remove, and other cloud-pair
updates cannot interleave between loading and saving the CloudPairsConfig, while
preserving the existing “Pair not found” behavior.
🪄 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: cf558d32-a941-4ad3-9a53-0da272f28d8c
📒 Files selected for processing (2)
src-tauri/src/lib.rssrc-tauri/src/sync_command_audit.rs
CodeRabbit correctly flagged that load-modify-save on cloud_config, cloud_pairs and multi_path was only accidentally atomic while the commands sat on the main thread. On the blocking pool two concurrent invocations can both load the same snapshot and last-write-win. Reuse the existing write locks (and add multi_path's) via with_*_mut helpers that hold the lock across load+mutate+save. Convert every GUI mutator of those files and the worker last_sync stamp to the helper so a partial-field update cannot clobber another.
CodeRabbit follow-up (003ae6b)All 7 load-modify-save races (1 inline + 6 outside-diff) were the same shape and are fixed together:
@coderabbitai review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src-tauri/src/lib.rs (1)
15659-15709: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
pause_aerocloud/resume_aerocloudstill run blocking config I/O directly on the async task.Unlike every other cloud-config mutation in this file (
enable_aerocloud_blocking,update_excluded_folders_blocking,update_conflict_strategy_blocking, the cloud-pair helpers), which are synchronous_blockingfunctions invoked viatokio::task::spawn_blocking, these two callcloud_config::with_cloud_config_mut(...)— which does synchronous file load/save — directly inside theasync fnbody, on the tokio executor thread. This reintroduces the class of blocking-call-on-a-shared-thread issue this PR is otherwise eliminating.🔧 Proposed fix: move the config mutation onto the blocking pool
async fn pause_aerocloud(app: AppHandle) -> Result<CloudConfig, String> { if BACKGROUND_SYNC_RUNNING.load(Ordering::SeqCst) { let _ = stop_background_sync(app.clone()).await; } - let config = cloud_config::with_cloud_config_mut(|config| { - if !config.enabled { - return Err("AeroCloud is not configured".to_string()); - } - config.paused = true; - Ok(config.clone()) - })?; + let config = tokio::task::spawn_blocking(|| { + cloud_config::with_cloud_config_mut(|config| { + if !config.enabled { + return Err("AeroCloud is not configured".to_string()); + } + config.paused = true; + Ok(config.clone()) + }) + }) + .await + .map_err(|err| format!("pause_aerocloud task failed: {err}"))??;async fn resume_aerocloud( app: AppHandle, state: tauri::State<'_, AppState>, ) -> Result<CloudConfig, String> { - let config = cloud_config::with_cloud_config_mut(|config| { - if !config.enabled { - return Err("AeroCloud is not configured".to_string()); - } - config.paused = false; - Ok(config.clone()) - })?; + let config = tokio::task::spawn_blocking(|| { + cloud_config::with_cloud_config_mut(|config| { + if !config.enabled { + return Err("AeroCloud is not configured".to_string()); + } + config.paused = false; + Ok(config.clone()) + }) + }) + .await + .map_err(|err| format!("resume_aerocloud task failed: {err}"))??;🤖 Prompt for 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. In `@src-tauri/src/lib.rs` around lines 15659 - 15709, Move the synchronous cloud-config mutations in pause_aerocloud and resume_aerocloud into tokio::task::spawn_blocking closures, matching the existing *_blocking mutation patterns. Preserve the enabled validation, paused-flag updates, cloned CloudConfig result, and error propagation while awaiting each blocking task before continuing with worker control or status handling.
🤖 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/cloud_service.rs`:
- Around line 575-585: The sync update at src-tauri/src/cloud_service.rs lines
575-585 must update cfg.last_sync, release the self.config write guard, then
persist via the blocking save path and handle/log any failure; do not call
with_cloud_config_mut while cfg remains locked. Apply the same change to the
sibling update at src-tauri/src/cloud_service.rs lines 919-929, preserving each
site’s existing timestamp behavior.
---
Outside diff comments:
In `@src-tauri/src/lib.rs`:
- Around line 15659-15709: Move the synchronous cloud-config mutations in
pause_aerocloud and resume_aerocloud into tokio::task::spawn_blocking closures,
matching the existing *_blocking mutation patterns. Preserve the enabled
validation, paused-flag updates, cloned CloudConfig result, and error
propagation while awaiting each blocking task before continuing with worker
control or status handling.
🪄 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: 7b7a7905-e1a5-4e03-a6a3-3fee30f9db38
📒 Files selected for processing (5)
src-tauri/src/cloud_config.rssrc-tauri/src/cloud_pairs.rssrc-tauri/src/cloud_service.rssrc-tauri/src/lib.rssrc-tauri/src/sync.rs
…ync lock across disk RMW - Merge main: #530 made export_sync_template_cmd take Option excludes; the async wrapper still passed Vec into the blocking body (CI E0308). - CodeRabbit: release self.config before with_cloud_config_mut so the sync worker does not hold the tokio Mutex across blocking FS I/O; log last_sync persist failures instead of dropping them.
1 participant
Closes #517. Follows #524, which did the first half and left the second in a list that could only shrink.
What this empties
#524 moved the commands that do filesystem, keystore and clipboard work outside
lib.rs, and froze the remaining 74 inMAIN_THREAD_NOT_YET_MOVED— a ratchet, asserted by the class pin, that a new synchronous command could not join and that converting one without deleting its line would break. This drains it, so the constant is deleted rather than carried.59 more commands become
asyncwith their body on the blocking pool. 15 stay synchronous and are now inMAIN_THREAD_ALLOWEDwith the reason next to each. From 82 out of 853 at the start of this work to 23.The 59
The bulk is one family: sync index, journals, profiles, snapshots, templates, scripts, cloud config, cloud pairs, versioning. All of it JSON read and write against the config directory, all of it previously on the GTK thread. Three are worth naming because they are not short at all:
versions_disk_usagewalks a directory tree, and its cost grows with the user's own version history;verify_local_transferhashes a file;safe_picker_start_dirwalks up a path supplied by the frontend with anis_dir()per level, so an unreachable mount stalls it with no bound of ours — the same shape aslist_subdirectoriesin fix(commands): take the blocking Tauri commands off the main thread (and correct the count: 82, not 38) #524.Plus
copy_to_clipboard,get_system_info(stats the vault and known_hosts),portable_info,flatpak_config_import_*andmount_autostart_blocked.The 15, and why each is not a dodge
Three must be on the main thread, and saying so is the correct answer rather than a concession:
restart_appdrives the event loop,toggle_menu_barcalls GTK window operations, andrebuild_menualready marshals menu construction onto the main thread through a channel — whichtauri-runtime-wry'ssend_user_messageruns inline when the caller is already that thread, so making it async would add an event-loop round trip and move no work at all. Same reasoning that keepsaeroshare_notifywhere it is.The other twelve are bounded by this crate rather than by the caller: atomic loads and stores,
default()structs, version strings baked in bybuild.rs, argv and environment reads, one HMAC, and string classification of an error we produced ourselves.One candidate did not survive its own test.
get_default_cloud_folderwas on the list to justify: it looks like pure construction. ButCloudConfig::default()callsdirs::document_dir(), which on Linux reads~/.config/user-dirs.dirs. That is disk, so by the bar written aboveMAIN_THREAD_ALLOWED— bounded by our own code, waits on nothing outside the process — it gets converted like the rest. A bar that bends for the case in front of you is not a bar.The pin, after
MAIN_THREAD_NOT_YET_MOVEDis gone because it is empty, which is the only good reason to delete a ratchet. What remains is the single allowlist and set equality in both directions: a new synchronous command is red, and an entry whose command became async, was renamed or was deleted is also red.every_allowlist_entry_explains_itselfkeeps a bare name from being a way to get green — and it caught three of my own entries during this work, which were too short to be reasons; they were expanded rather than the assertion loosened.Two mechanical notes, both from getting them wrong first
The conversion was scripted, and the script was wrong twice in ways worth writing down:
rindex(')'), which picks the paren insideResult<(), String>and silently mangles every command that returns unit. It matches the parameter list by depth now. Caught because the first run aborted oncopy_to_clipboard, not because the output was reviewed;move || f(), which isclippy::redundant_closure, and a plain-value fallback must binderror-D warningsrejects it. Both are generated correctly rather than patched afterwards, because the first attempt at patching afterwards used a multi-line regex that matched far too greedily and corrupted the file; that was reverted from git rather than repaired by hand.verify_local_transferfails closed on the JoinError path, withpassed: falseand a message, because a verification that never ran did not pass.Internal callers, fixed rather than worked around
lib.rs's ownsafe_picker_start_dir_testsdrove the command synchronously and now drive the blocking body directly.PortableInfoandSystemInfo, both private tolib.rs, deriveDefaultso the fallback is a value rather than a hand-written literal that would drift from the struct the first time somebody adds a field.Verification
Gate below in a comment, with the exit codes read without a pipe.
Summary by CodeRabbit