fix(desktop): fix Windows FUSE overwrite race and bin E2E test#271
Conversation
When TC08 uploads v2 of a file with the same disk path as v1, Chromium skips the change event because the input value hasn't changed. Clearing input.value before setInputFiles ensures the change event always fires. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: bd5084e0c4c4
Two issues caused Desktop E2E Windows CI failures: 1. **File overwrite race condition**: PowerShell's `Set-Content` does open→truncate→close then open→write→close, creating two background IPFS uploads for the same inode. The stale 0-byte upload could overwrite the correct CID because `write_generation` was never incremented on Windows (always 0), so `drain_upload_completions` accepted both uploads. Fix: increment `write_generation` in `handle_set_file_size`, `handle_overwrite`, and the cleanup flush code, matching the macOS/Linux pattern. This ensures stale uploads are rejected. 2. **Bin test checking non-existent API field**: Test 5 in `test-recycle-bin.ps1` checked `recycleBinIpnsName` from `/vault/config`, but this field is derived client-side via HKDF and never exposed by the API. Fix: verify bin entry creation by checking the desktop log for "Bin entry published" instead. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: f5d35d1b5d65
|
Caution Review failedPull request was closed or merged during review WalkthroughTrack and propagate write generations for size-affecting FUSE ops; prevent caching stale upload data; expose desktop log path to Windows E2E step and switch recycle-bin Test 5 to log-based verification when available; clear file-input before re-upload in E2E helper and add a safe quota-store reader in recycle-bin tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #271 +/- ##
==========================================
+ Coverage 47.95% 48.06% +0.11%
==========================================
Files 109 110 +1
Lines 8360 9050 +690
Branches 652 652
==========================================
+ Hits 4009 4350 +341
- Misses 4179 4528 +349
Partials 172 172
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes two Windows Desktop E2E test failures: a WinFsp file overwrite race condition (stale 0-byte IPFS upload overwriting the correct file CID) and a misconfigured bin test that checked a non-existent API field.
Changes:
- Windows WinFsp
write_generationis now properly incremented inhandle_overwrite,handle_set_file_size(0), and the cleanup flush path — matching the existing macOS/Linux behavior and preventing stale uploads fromdrain_upload_completionsaccepting a 0-byte CID. - The bin E2E test (Test 5) now verifies publish via desktop log inspection instead of checking the non-existent
recycleBinIpnsNameAPI field. DESKTOP_LOGenv var is exported in the Windows CI workflow so the PowerShell test script can locate the log file.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
apps/desktop/src-tauri/src/fuse/windows/write_ops.rs |
Core fix: adds write_generation increments in overwrite/truncate/flush paths, and passes actual generation (not 0) to UploadComplete |
tests/e2e-desktop/scripts/test-recycle-bin.ps1 |
Replaces broken API-field check with desktop-log-based bin publish verification |
.github/workflows/e2e-desktop.yml |
Exports DESKTOP_LOG so the PowerShell test script can read the desktop binary log |
tests/e2e/page-objects/file-browser/upload-zone.page.ts |
Clears file input value before setInputFiles to ensure re-upload of same path triggers change event |
You can also share your feedback on Copilot code review. Take the survey.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: af52ba0136c1
- Guard pending_content→content_cache move with generation check in drain_upload_completions to prevent cache pollution from stale uploads - Clear file input value in uploadFiles() for consistency with uploadFile() Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 61bc5416fb00
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/e2e/tests/recycle-bin.spec.ts (1)
355-360: Quota store is intentionally not exposed; quota assertions are best-effort as designed.The review comment correctly identifies that
quotais not registered in__ZUSTAND_STORES__(onlyauth,vault,folder,syncare exposed inapps/web/src/main.tsx). This makes the quota assertions at lines 360, 416, and 454 unreachable—they always returnnulldue to missing store, and the guards handle this correctly.This is by design. The test explicitly documents this fragility (lines 350–352: "window.ZUSTAND_STORES may not be exposed"), and the learning confirms quota validation is intentionally supplementary—the primary assertion is that permanent delete completes without crashing. Encryption overhead and IPFS chunking make deterministic freed-byte tracking unreliable in E2E tests, so the store was intentionally excluded from exposure.
Optional improvement: Either expose
useQuotaStorein__ZUSTAND_STORES__if quota validation becomes important, or simplify the test by removing the dead-code branches to reduce confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/tests/recycle-bin.spec.ts` around lines 355 - 360, The test relies on a quota store lookup via __ZUSTAND_STORES__.quota which is intentionally not exposed, making the quota assertions in recycle-bin.spec.ts unreachable; either (a) expose useQuotaStore/quota in the global __ZUSTAND_STORES__ registry (add the quota entry where other stores are registered so the lookup in recycle-bin.spec.ts finds the store), or (b) remove/simplify the dead guarded assertions that reference __ZUSTAND_STORES__.quota (and related checks at the locations noted) so the test only performs the primary “permanent delete completes” assertion; update the test comments to reflect the chosen approach.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/e2e/tests/recycle-bin.spec.ts`:
- Around line 355-360: The test relies on a quota store lookup via
__ZUSTAND_STORES__.quota which is intentionally not exposed, making the quota
assertions in recycle-bin.spec.ts unreachable; either (a) expose
useQuotaStore/quota in the global __ZUSTAND_STORES__ registry (add the quota
entry where other stores are registered so the lookup in recycle-bin.spec.ts
finds the store), or (b) remove/simplify the dead guarded assertions that
reference __ZUSTAND_STORES__.quota (and related checks at the locations noted)
so the test only performs the primary “permanent delete completes” assertion;
update the test comments to reflect the chosen approach.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3891d59d-ea81-4901-916e-da141bd7b362
📒 Files selected for processing (1)
tests/e2e/tests/recycle-bin.spec.ts
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
You can also share your feedback on Copilot code review. Take the survey.
- Document DESKTOP_LOG env var in test-recycle-bin.ps1 header - Extract duplicated Zustand quota store access into getQuotaUsedBytes helper Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 4a56f1f17ac9
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
You can also share your feedback on Copilot code review. Take the survey.
| if (quotaDecreased !== null) { | ||
| expect(quotaDecreased).toBe(true); | ||
| if (quotaUsed !== null) { | ||
| expect(typeof quotaUsed === 'number').toBe(true); |
There was a problem hiding this comment.
The assertion at line 370 is a tautological no-op. The getQuotaUsedBytes helper returns number | null from its type signature, and by the time line 370 executes, quotaUsed has already been confirmed to be non-null by the surrounding if (quotaUsed !== null) guard. Therefore typeof quotaUsed === 'number' is always true, and expect(typeof quotaUsed === 'number').toBe(true) never fails under any circumstances. The intent of this secondary assertion (verifying the store is functional and actually holds a numeric value) is preserved by the null-check alone. Consider either removing this inner assertion entirely, or replacing it with a more meaningful check such as expect(quotaUsed).toBeGreaterThanOrEqual(0).
| expect(typeof quotaUsed === 'number').toBe(true); | |
| expect(quotaUsed).toBeGreaterThanOrEqual(0); |
Summary
Set-Contentperforms truncate-then-write as two separate handle cycles, producing two background IPFS uploads for the same inode. The stale 0-byte upload could overwrite the correct CID becausewrite_generationwas hardcoded to 0 on Windows, sodrain_upload_completionsaccepted both. Nowwrite_generationis properly incremented (matching macOS/Linux) so stale uploads are rejected.recycleBinIpnsNamefrom/vault/config, but this field is derived client-side (via HKDF from user's private key) and never exposed by the API. Changed to verify bin publish via desktop log output.DESKTOP_LOGenv var to Windows E2E test runner so the bin test can read the desktop log file.Fixes 4 test failures in Desktop E2E (Windows):
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests