review: document bugs and add missing test coverage#69
Open
assafvayner wants to merge 5 commits intomainfrom
Open
review: document bugs and add missing test coverage#69assafvayner wants to merge 5 commits intomainfrom
assafvayner wants to merge 5 commits intomainfrom
Conversation
Add REVIEW.md documenting 7 correctness/invariant concerns found during code review, including a high-severity TOCTOU in poll's deletion path and apply_commit clobbering size on generation mismatch. Add 18 new unit tests covering previously untested areas: - PrefetchState::prepare_fetch adaptive window logic (9 tests) - InodeTable::remove_orphan (2 tests) - VFS symlink/readlink (2 tests) - Poll interaction with dirty inodes (1 test) - FlushManager dedup, cancel_delete, retry on failure (4 tests)
Add tests that demonstrate the bugs documented in REVIEW.md: 1. bug_apply_commit_clobbers_size_on_generation_mismatch (#[should_panic]): Proves apply_commit unconditionally overwrites size and xet_hash even when dirty_generation doesn't match, causing stale metadata after concurrent write + flush. 2. bug_poll_deletes_dirty_inode_toctou (#[should_panic]): Proves the TOCTOU race where an inode becomes dirty between poll's snapshot and Phase 2 write-lock, causing poll to delete it and lose uncommitted data. 3. bug_setattr_write_no_staging_lock: Proves write() does not acquire the staging lock, meaning a concurrent setattr(truncate) can race with pwrite() on the same staging fd.
Replace #[should_panic] with #[ignore] so the tests assert CORRECT behavior and visibly fail when run with --ignored: cargo test --lib --features nfs -- --ignored bug_ Each test now panics with a descriptive message explaining the bug. Normal test runs skip them (3 ignored). When a bug is fixed, remove #[ignore] and the test becomes a passing regression guard.
POSIX Compliance (pjdfstest) |
Benchmark Results |
XciD
added a commit
that referenced
this pull request
Mar 26, 2026
## Summary Fixes REVIEW.md finding 2-[#69](#69): the poll thread could remove an inode from the table while file handles were still referencing it, causing `read()`/`getattr()`/`release()` on those handles to fail with `ENOENT`. **Root cause**: `apply_poll_diff` called `inode_table.remove(ino)` without checking whether any `OpenFile` entries referenced that inode. ### Fix - Wrap `open_files` in `Arc` so it can be shared with the poll task - Pass `open_files` to `apply_poll_diff`, skip deletion when handles exist - Files with open handles stay in the inode table until all handles are released ### Test - `poll_skips_deletion_with_open_handles`: opens a file, simulates remote deletion via poll, verifies the inode survives while the handle is open
XciD
added a commit
that referenced
this pull request
Mar 26, 2026
…match (#75) ## Summary Fixes REVIEW.md finding 3 in #69 : `apply_commit` overwrote `size` and `xet_hash` unconditionally before checking `clear_dirty_if(dirty_generation)`. If a concurrent writer advanced the generation, the in-progress file's size was clobbered with the stale flushed value. **Scenario:** 1. Writer A flushes 10 MB file (captures gen=5) 2. Writer B extends file to 15 MB (gen=6) 3. Flush completes, `apply_commit("hash", 10MB, gen=5)` runs 4. Old code: sets `size=10MB`, then `clear_dirty_if(5)` returns false (gen is 6) 5. Applications see 10 MB until next flush, even though staging file is 15 MB **Fix:** Move `xet_hash`, `size`, and `pending_deletes.clear()` inside the `clear_dirty_if` success branch. One-line semantic change in `src/virtual_fs/inode.rs`, existing test updated.
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
1 participant
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
REVIEW.mddocumenting 7 correctness/invariant concerns found during code review, prioritized by severity (1 high, 4 medium, 2 low)#[ignore]) that fail when run directly, demonstrating the bugsBugs documented
apply_commitclobbers size/xet_hash on generation mismatchsetattrtruncate races with concurrentwritein advanced modeNew tests (21 total)
Prefetch
prepare_fetch(9): StartStream, ContinueStream, window doubling to MAX_WINDOW, backward seek RangeDownload, forward skip boundary, fetch_size clamping near EOFInode (2):
remove_orphanwith nlink=0 and nlink>0VFS (7): poll + dirty inode interaction, symlink create/readlink, readlink on regular file, flush dedup, cancel_delete, cancel_delete_prefix, pending_deletes retry on failure
Bug-proving
#[ignore](3): Run withcargo test --lib --features nfs -- --ignored bug_to see failures with descriptive messages. Remove#[ignore]after fixing each bug.Test plan
cargo test --lib --features nfs— 234 passed, 3 ignored, 0 failedcargo test --lib --features nfs -- --ignored bug_— 3 failed (expected, proves bugs)