fix(desktop): trigger metadata refresh from lookup/open, fix e2e sync test#456
Merged
Conversation
… test The cross-client sync e2e test polled with stat+cat but never triggered readdir, which is the only FUSE operation that called drain_refresh_completions() -> populate_folder(). This meant remote edits were never detected during the 120s polling window. Fix the test by adding ls (readdir) to the polling loop, and more importantly fix the FUSE layer so that lookup (macOS/Linux) and open (Windows) also check for stale parent folder metadata and spawn background refresh — matching readdir's existing behaviour. This ensures file access eventually picks up remote edits without requiring an explicit directory listing. Also adds the missing Windows PowerShell cross-client sync test and wires it into run-all.ps1 as Step 6. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 89794793ed09
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 46 minutes and 17 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughBackground refresh logic was added to FUSE handlers: stale parent-folder metadata now triggers non-blocking tasks that resolve IPNS, fetch encrypted folder metadata from IPFS, decrypt it, and emit PendingRefresh updates; new cross-client sync end-to-end tests were also added. Changes
Sequence Diagram(s)sequenceDiagram
participant Handler as FUSE Handler
participant Cache as Metadata Cache
participant IPNS as IPNS Resolver
participant IPFS as IPFS Gateway
participant Crypto as Decryption
participant Channel as refresh_tx Channel
Handler->>Cache: Check parent metadata (stale/missing?)
alt stale or missing
Handler->>Handler: Insert ipns_name into refreshing_metadata
Handler->>Handler: Spawn background task (non-blocking)
Handler-->>Handler: Return to caller
rect rgba(100,150,200,0.5)
Note over Handler,Channel: Background task
Handler->>IPNS: Resolve parent IPNS name
alt resolution succeeds
IPNS->>IPFS: Fetch encrypted metadata (CID)
alt fetch succeeds
IPFS->>Crypto: Decrypt with folder_key
alt decrypt succeeds
Crypto->>Channel: Send PendingRefresh { ino, ipns_name, metadata, cid }
else decrypt fails
Crypto->>Handler: Log warning/debug
end
else fetch fails
IPFS->>Handler: Log warning/debug
end
else resolution fails
IPNS->>Handler: Log warning/debug
end
end
else metadata fresh
Handler->>Cache: Use cached metadata
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
github-actions
Bot
added
release:cipherbox-fuse:fix
Contributor
Release Preview
|
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a desktop cross-client sync gap where remote edits weren’t detected during polling unless a directory listing occurred, and updates desktop E2E coverage to reliably trigger refresh logic (including Windows parity).
Changes:
- Trigger stale folder-metadata refresh from file-access paths (
lookupon Unix/macOS andopenon Windows), not only from directory listing operations. - Fix the cross-client sync E2E polling loop to explicitly trigger
readdir(ls/Get-ChildItem) during polling. - Add and wire a Windows PowerShell cross-client sync test into the Windows desktop E2E runner.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
crates/fuse/src/read_ops.rs |
Adds stale-parent metadata refresh triggering from handle_lookup to align file access with existing readdir refresh behavior. |
crates/fuse/src/platform/windows/read_ops.rs |
Drains refresh completions on open and triggers stale-parent metadata refresh from handle_open for Windows parity. |
tests/desktop-e2e/scripts/test-cross-client-sync.sh |
Adds ls in the polling loop to reliably trigger refresh processing during E2E sync checks. |
tests/desktop-e2e/scripts/test-cross-client-sync.ps1 |
Introduces Windows version of the cross-client sync E2E test. |
tests/desktop-e2e/scripts/run-all.ps1 |
Adds Step 6 to run the new cross-client sync test on Windows. |
Cargo.lock |
Updates the locked workspace package version for cipherbox-fuse. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #456 +/- ##
==========================================
+ Coverage 62.34% 71.60% +9.26%
==========================================
Files 135 114 -21
Lines 10157 7262 -2895
Branches 1081 1083 +2
==========================================
- Hits 6332 5200 -1132
+ Misses 3601 1838 -1763
Partials 224 224
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/fuse/src/platform/windows/read_ops.rs (1)
117-175: Coalesce stale-folder refreshes before spawning network work.This branch will keep spawning identical resolve/fetch/decrypt tasks while
metadata_cache.get(..)stays empty. On Windows, burstyopen()traffic can hit the same stale path many times before the first refresh lands. Please add an in-flight guard for folder refreshes, similar toprefetching/resolving_file_pointers, so one stale folder only has one background refresh at a time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fuse/src/platform/windows/read_ops.rs` around lines 117 - 175, This spawns duplicate resolve/fetch/decrypt work while metadata_cache.get(...) is empty; add an in-flight guard so only one background refresh per folder runs: before creating stale_info-driven task, consult and atomically insert into a per-Fs in-flight set (e.g. add a HashSet/ConcurrentHashSet field like refreshing_folders or reuse the existing prefetching/resolving_file_pointers structure) keyed by ipns_name (or refresh_ino+ipns_name); only call rt.spawn and start the cipherbox_api_client::ipns::resolve_ipns / ipfs::fetch_content / decrypt sequence if the insert succeeded, and ensure the spawned task removes the key from the in-flight set on all completion paths (after sending crate::PendingRefresh via refresh_tx or on any error) so subsequent opens can trigger a new refresh once the first finishes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/fuse/src/platform/windows/read_ops.rs`:
- Around line 117-175: This spawns duplicate resolve/fetch/decrypt work while
metadata_cache.get(...) is empty; add an in-flight guard so only one background
refresh per folder runs: before creating stale_info-driven task, consult and
atomically insert into a per-Fs in-flight set (e.g. add a
HashSet/ConcurrentHashSet field like refreshing_folders or reuse the existing
prefetching/resolving_file_pointers structure) keyed by ipns_name (or
refresh_ino+ipns_name); only call rt.spawn and start the
cipherbox_api_client::ipns::resolve_ipns / ipfs::fetch_content / decrypt
sequence if the insert succeeded, and ensure the spawned task removes the key
from the in-flight set on all completion paths (after sending
crate::PendingRefresh via refresh_tx or on any error) so subsequent opens can
trigger a new refresh once the first finishes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e85681f0-b754-4e1e-ad74-b23407e28c7c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
crates/fuse/src/platform/windows/read_ops.rscrates/fuse/src/read_ops.rstests/desktop-e2e/scripts/run-all.ps1tests/desktop-e2e/scripts/test-cross-client-sync.ps1tests/desktop-e2e/scripts/test-cross-client-sync.sh
Add refreshing_metadata HashSet guard to prevent multiple background IPNS resolve+fetch+decrypt tasks for the same folder when the metadata cache is expired. Also fix inaccurate test comments claiming readdir is the only drain_refresh_completions trigger. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 7df7df28c402
Move inode field extraction (parent_ino, children, attr) before the mutable borrow of fs.refreshing_metadata, matching the scoped-block pattern already used in the shared dir_ops.rs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 1860aed4d6be
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
crates/fuse/src/platform/windows/dir_ops.rs:75
- After adding
refreshing_metadatadeduplication, the background refresh task only clears the guard if it successfully sendsPendingRefresh. Any resolve/fetch/decrypt failure leaves the folder’s IPNS name stuck inrefreshing_metadata, preventing further refreshes and potentially freezing metadata updates after a transient error. Ensure the guard is cleared on failure/timeout as well (e.g., send a failure completion or add expiry).
if let Some((ipns_name, folder_key)) = stale_info.filter(|(n, _)| !fs.refreshing_metadata.contains(n)) {
fs.refreshing_metadata.insert(ipns_name.clone());
let api = fs.api.clone();
let rt = fs.rt.clone();
let tx = fs.refresh_tx.clone();
let refresh_ino = ino;
rt.spawn(async move {
crates/fuse/src/dir_ops.rs:69
- With the new
refreshing_metadatadedup guard, a refresh that fails (resolve/fetch/decrypt) will never clear the guard because only successful refreshes sendPendingRefresh. That can permanently disable further metadata refreshes for this folder, leaving the mount stuck with stale entries after any transient network error. Clear the guard on failure/timeout as well (e.g., send a failure message or add an expiry/backoff mechanism).
if let Some((ipns_name, folder_key)) = stale_info.filter(|(n, _)| offset == 0 && !fs.refreshing_metadata.contains(n)) {
fs.refreshing_metadata.insert(ipns_name.clone());
let api = fs.api.clone();
let rt = fs.rt.clone();
let tx = fs.refresh_tx.clone();
let refresh_ino = ino;
Owner
Author
|
@coderabbitai please review the changes since the last review |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/fuse/src/lib.rs (1)
643-648:⚠️ Potential issue | 🟠 MajorClear
refreshing_metadataon refresh failures too.This new guard is only released when a
PendingRefresharrives. The spawned refresh tasks currently just log and return on resolve/fetch/decrypt failure, so one transient error leaves that IPNS name stuck inrefreshing_metadataand every later lookup/open/readdir skips refresh for that folder until remount.Please send a failure notification through the refresh channel, or explicitly remove the key on every error path, so retries remain possible.
As per coding guidelines, background tasks must communicate via
mpscchannels.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fuse/src/lib.rs` around lines 643 - 648, The code currently only removes entries from refreshing_metadata when a successful PendingRefresh is received in drain_refresh_completions, leaving names stuck on task failure; update the background refresh task(s) that perform resolve/fetch/decrypt to always send a terminal notification over the same refresh channel (e.g., send a PendingRefresh or a dedicated failure variant via refresh_tx) when they exit with an error, or alternatively ensure those error paths explicitly remove the IPNS key from refreshing_metadata before returning; reference the refresh_rx/refresh_tx channel, the PendingRefresh type, and the refreshing_metadata/mutated_folders/publish_queue maps so the drain_refresh_completions logic can reliably clear the in-progress marker on both success and failure.crates/fuse/src/read_ops.rs (1)
217-251:⚠️ Potential issue | 🟠 MajorLazy load path has the same
refreshing_metadatacleanup gap.Lines 240, 243, and 246 log errors but don't remove the
ipns_namefromrefreshing_metadata, leaving it orphaned on failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fuse/src/read_ops.rs` around lines 217 - 251, The lazy-load background task adds ipns_name into fs.refreshing_metadata but on several error branches (the Err arms in the rt.spawn async resolving/fetching/decrypting flow) it never removes it, leaving entries orphaned; modify the async closure spawned by rt.spawn so that ipns_name is removed from fs.refreshing_metadata on all exit paths (success and any error) — e.g. capture fs or a clone, and ensure removal happens in every Err arm or use a small RAII guard/local finally-style helper inside the async move block that calls fs.refreshing_metadata.remove(&ipns_name) when dropped, referencing the existing symbols ipns_name, fs.refreshing_metadata, rt.spawn, and the tx send/resolve/fetch/decrypt branches so the cleanup always runs.
🧹 Nitpick comments (1)
crates/fuse/src/read_ops.rs (1)
190-214: Consider extracting the duplicate async refresh logic into a shared helper.The async blocks for
needs_refresh(lines 190-214) andneeds_load(lines 224-248) are nearly identical—both resolve IPNS, fetch content, decrypt, and sendPendingRefresh. Extracting this into a shared function would reduce duplication and simplify applying therefreshing_metadatacleanup fix to both paths.Also applies to: 224-248
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fuse/src/read_ops.rs` around lines 190 - 214, Duplicate async logic in the needs_refresh and needs_load spawn blocks should be extracted into a shared async helper (e.g., fetch_and_send_pending_refresh) that takes the API client (&api), ipns_name, folder_key, refresh_ino, and the tx sender and performs resolve_ipns -> fetch_content -> decrypt_metadata_from_ipfs_public -> tx.send(PendingRefresh) while preserving the same warn/debug log messages on errors; replace both rt.spawn blocks to call rt.spawn(fetch_and_send_pending_refresh(...)) so the identical flow is centralized and you can apply the refreshing_metadata cleanup fix in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/fuse/src/platform/windows/read_ops.rs`:
- Around line 146-175: The async refresh task currently never removes ipns_name
from fs.refreshing_metadata on error, so implement the same failure-reporting
pattern used for content prefetch: add a PendingRefresh::Failure variant to the
PendingRefresh enum (mirroring PendingContent::Failure), and on each error path
inside the spawned task (resolve, fetch, decrypt) send
tx.send(PendingRefresh::Failure { ino: refresh_ino, ipns_name }) instead of just
logging; ensure drain_refresh_completions() (the consumer of refresh_tx) is
updated to handle PendingRefresh::Failure by cleaning up fs.refreshing_metadata
and any other bookkeeping, while leaving the existing success send of
PendingRefresh::Success (or the current PendingRefresh structure with metadata)
intact.
In `@crates/fuse/src/read_ops.rs`:
- Around line 183-215: The async refresh task leaves ipns_name in the
fs.refreshing_metadata set on all error paths (in the resolve/fetch/decrypt
branches), blocking future refreshes; modify the code around the needs_refresh
handling (the rt.spawn block) so the ipns_name is removed from
refreshing_metadata in every outcome—either by cloning a thread-safe handle to
refreshing_metadata (e.g., Arc/Mutex or DashMap) into the task and removing
ipns_name before returning on both success and each Err branch, or by sending a
dedicated cleanup/failure message (via refresh_tx or a new channel) that the
main loop consumes to remove the entry; apply the same pattern to the needs_load
path so load-related tasks also clear refreshing_metadata on error.
---
Outside diff comments:
In `@crates/fuse/src/lib.rs`:
- Around line 643-648: The code currently only removes entries from
refreshing_metadata when a successful PendingRefresh is received in
drain_refresh_completions, leaving names stuck on task failure; update the
background refresh task(s) that perform resolve/fetch/decrypt to always send a
terminal notification over the same refresh channel (e.g., send a PendingRefresh
or a dedicated failure variant via refresh_tx) when they exit with an error, or
alternatively ensure those error paths explicitly remove the IPNS key from
refreshing_metadata before returning; reference the refresh_rx/refresh_tx
channel, the PendingRefresh type, and the
refreshing_metadata/mutated_folders/publish_queue maps so the
drain_refresh_completions logic can reliably clear the in-progress marker on
both success and failure.
In `@crates/fuse/src/read_ops.rs`:
- Around line 217-251: The lazy-load background task adds ipns_name into
fs.refreshing_metadata but on several error branches (the Err arms in the
rt.spawn async resolving/fetching/decrypting flow) it never removes it, leaving
entries orphaned; modify the async closure spawned by rt.spawn so that ipns_name
is removed from fs.refreshing_metadata on all exit paths (success and any error)
— e.g. capture fs or a clone, and ensure removal happens in every Err arm or use
a small RAII guard/local finally-style helper inside the async move block that
calls fs.refreshing_metadata.remove(&ipns_name) when dropped, referencing the
existing symbols ipns_name, fs.refreshing_metadata, rt.spawn, and the tx
send/resolve/fetch/decrypt branches so the cleanup always runs.
---
Nitpick comments:
In `@crates/fuse/src/read_ops.rs`:
- Around line 190-214: Duplicate async logic in the needs_refresh and needs_load
spawn blocks should be extracted into a shared async helper (e.g.,
fetch_and_send_pending_refresh) that takes the API client (&api), ipns_name,
folder_key, refresh_ino, and the tx sender and performs resolve_ipns ->
fetch_content -> decrypt_metadata_from_ipfs_public -> tx.send(PendingRefresh)
while preserving the same warn/debug log messages on errors; replace both
rt.spawn blocks to call rt.spawn(fetch_and_send_pending_refresh(...)) so the
identical flow is centralized and you can apply the refreshing_metadata cleanup
fix in one place.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 38a527d8-456f-4bbb-819b-47de7dd572ac
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
apps/desktop/src-tauri/src/fuse/mod.rsapps/desktop/src-tauri/src/fuse/windows/mod.rscrates/fuse/src/dir_ops.rscrates/fuse/src/lib.rscrates/fuse/src/platform/windows/dir_ops.rscrates/fuse/src/platform/windows/read_ops.rscrates/fuse/src/read_ops.rstests/desktop-e2e/scripts/run-all.ps1tests/desktop-e2e/scripts/test-cross-client-sync.ps1tests/desktop-e2e/scripts/test-cross-client-sync.sh
PendingRefresh was a plain struct, so error paths in spawned metadata refresh tasks never sent a message — leaving ipns_name stuck in refreshing_metadata forever and blocking future refreshes for that folder. Convert PendingRefresh to an enum with Success/Failure variants (matching PendingContent and PendingFilePointer patterns), extract duplicated spawn logic into spawn_metadata_refresh() helper, and update drain_refresh_completions() to handle both variants. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 8e79f5de316f
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Root cause: The cross-client sync e2e test (added in fix(desktop): detect remote file edits and re-resolve IPNS in FUSE mount #454) polled with
stat+catbut never didlson the mount directory. Onlyreaddir(triggered byls) callsdrain_refresh_completions()→populate_folder(), which is the only code path that detectsmodified_atchanges on FilePointers and triggers IPNS re-resolution. Without it, the FUSE mount never detected remote edits during the 120s polling window.FUSE layer fix:
handle_lookup(macOS/Linux) andhandle_open(Windows) now also check for stale parent folder metadata and spawn background refresh — matchingreaddir's existing behaviour. This means file access (stat,open,cat) will eventually pick up remote edits without requiring an explicit directory listing.Test fix: Added
lsto the Test 5 polling loop in the bash script (most reliable trigger sinceDIR_TTL=0means the kernel never caches directory results).Windows parity: Added the missing
test-cross-client-sync.ps1and wired it intorun-all.ps1as Step 6.Changed files
crates/fuse/src/read_ops.rshandle_lookupnow checks parent folder metadata staleness and spawns background refresh for loaded folders (not just lazy-load)crates/fuse/src/platform/windows/read_ops.rshandle_opennow callsdrain_refresh_completions()and checks parent folder stalenesstests/desktop-e2e/scripts/test-cross-client-sync.shlsto Test 5 polling loop to triggerreaddirtests/desktop-e2e/scripts/test-cross-client-sync.ps1tests/desktop-e2e/scripts/run-all.ps1Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests