diff --git a/lib/fs/async_fs.rs b/lib/fs/async_fs.rs index 8aa4bd42..cce44235 100644 --- a/lib/fs/async_fs.rs +++ b/lib/fs/async_fs.rs @@ -447,9 +447,11 @@ impl AsyncFs { /// Asynchronously look up an inode by name within a parent directory. /// /// Resolution order: - /// 1. Directory cache (synchronous fast path) - /// 2. Lookup cache (`get_or_try_init` — calls `dp.lookup()` only on a true miss) - /// 3. On success, populates inode table and directory cache + /// 1. Directory cache hit — returns immediately. + /// 2. Directory cache miss in a fully-populated directory — returns `ENOENT` + /// immediately without hitting the remote data provider. + /// 3. Lookup cache (`get_or_try_init` — calls `dp.lookup()` only on a true miss) + /// 4. On success, populates inode table and directory cache pub async fn lookup( &self, parent: LoadedAddr, @@ -469,6 +471,11 @@ impl AsyncFs { // entry so the slow path calls dp.lookup() fresh. self.lookup_cache .remove_sync(&(parent.addr(), Arc::from(name))); + } else if self.directory_cache.is_populated(parent) { + // The directory has been fully populated (e.g. by readdir) and this + // name was not found — the file does not exist. Short-circuit + // without hitting the remote data provider. + return Err(std::io::Error::from_raw_os_error(libc::ENOENT)); } // Note: get_or_try_init deduplicates successful lookups but NOT diff --git a/lib/fs/dcache.rs b/lib/fs/dcache.rs index 4e05bb7a..28ef643b 100644 --- a/lib/fs/dcache.rs +++ b/lib/fs/dcache.rs @@ -142,6 +142,18 @@ impl DCache { children.get(name).cloned() } + /// Returns `true` if the directory at `parent_ino` has been fully + /// populated (i.e. [`finish_populate`](Self::finish_populate) completed + /// successfully and no eviction has since reset the flag). + #[must_use] + pub fn is_populated(&self, parent_ino: LoadedAddr) -> bool { + self.dirs + .read_sync(&parent_ino, |_, state| { + state.populated.load(Ordering::Acquire) == POPULATE_DONE + }) + .unwrap_or(false) + } + /// Atomically inserts or overwrites a child entry in the cache. /// /// Handles two kinds of stale-entry cleanup before the insert: diff --git a/tests/async_fs_correctness.rs b/tests/async_fs_correctness.rs index aaa44243..3b0dbb5a 100644 --- a/tests/async_fs_correctness.rs +++ b/tests/async_fs_correctness.rs @@ -880,6 +880,50 @@ async fn readdir_evict_all_readdir_returns_same_entries() { ); } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn lookup_returns_enoent_without_remote_call_when_dir_populated() { + let root = make_inode(1, INodeType::Directory, 0, None); + let child = make_inode(10, INodeType::File, 42, Some(1)); + + let mut state = MockFsState::default(); + // Configure readdir to return one child. Do NOT configure a lookup + // entry for "nonexistent.txt". If lookup hits the remote, the mock + // will return ENOENT anyway — but we need to verify it does NOT + // call dp.lookup() at all. + state + .directories + .insert(1, vec![(OsString::from("file.txt"), child)]); + // Crucially, also register "nonexistent.txt" in lookups so that if + // the slow path fires, lookup would SUCCEED — proving the test + // catches the bug. + let fake_child = make_inode(99, INodeType::File, 1, Some(1)); + state + .lookups + .insert((1, "nonexistent.txt".into()), fake_child); + let dp = MockFsDataProvider::new(state); + + let table = Arc::new(FutureBackedCache::default()); + let fs = AsyncFs::new(dp, root, Arc::clone(&table)).await; + + // Populate the directory via readdir. + fs.readdir(LoadedAddr::new_unchecked(1), 0, |_, _| false) + .await + .unwrap(); + + // Lookup a name that was NOT in the readdir results. + // With the fix, this should return ENOENT from the dcache + // without ever calling dp.lookup(). + let err = fs + .lookup(LoadedAddr::new_unchecked(1), OsStr::new("nonexistent.txt")) + .await + .unwrap_err(); + assert_eq!( + err.raw_os_error(), + Some(libc::ENOENT), + "lookup for missing file in populated dir should return ENOENT" + ); +} + /// Verify that `IndexedLookupCache::evict_addr` removes only entries /// referencing the evicted inode (parent or child), leaving unrelated /// entries intact. This is the O(k) replacement for the old O(N) scan. @@ -1139,6 +1183,41 @@ async fn remove_sync_cleans_cache_and_parent_reverse_index() { ); } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn lookup_falls_through_to_remote_after_eviction_resets_populated() { + let root = make_inode(1, INodeType::Directory, 0, None); + let child_a = make_inode(10, INodeType::File, 42, Some(1)); + let child_b = make_inode(11, INodeType::File, 99, Some(1)); + + let mut state = MockFsState::default(); + state + .directories + .insert(1, vec![(OsString::from("a.txt"), child_a)]); + // "b.txt" is only reachable via lookup, NOT readdir. + state.lookups.insert((1, "b.txt".into()), child_b); + let dp = MockFsDataProvider::new(state); + + let table = Arc::new(FutureBackedCache::default()); + let fs = AsyncFs::new(dp, root, Arc::clone(&table)).await; + + // Populate via readdir — directory is now DONE. + fs.readdir(LoadedAddr::new_unchecked(1), 0, |_, _| false) + .await + .unwrap(); + + // Evict child_a — this resets populate flag to UNCLAIMED. + fs.directory_cache().evict(LoadedAddr::new_unchecked(10)); + + // Now lookup "b.txt" — directory is no longer DONE, so the + // short-circuit must NOT fire. The slow path should call + // dp.lookup() and find "b.txt". + let result = fs + .lookup(LoadedAddr::new_unchecked(1), OsStr::new("b.txt")) + .await + .unwrap(); + assert_eq!(result.inode.addr, 11); +} + /// Verify that `evict_addr(old_child)` does not spuriously remove a /// freshly-indexed reverse entry for a *new* child that reuses the same /// `LookupKey` `(parent, name)`. diff --git a/tests/dcache_correctness.rs b/tests/dcache_correctness.rs index a91be439..c44ef3f5 100644 --- a/tests/dcache_correctness.rs +++ b/tests/dcache_correctness.rs @@ -740,3 +740,62 @@ async fn evict_generation_bump_prevents_stale_finish_populate() { PopulateStatus::InProgress => panic!("unexpected InProgress"), } } + +#[tokio::test] +async fn is_populated_returns_false_for_unknown_parent() { + let cache = DCache::new(); + assert!( + !cache.is_populated(LoadedAddr::new_unchecked(1)), + "unknown parent should not be populated" + ); +} + +#[tokio::test] +async fn is_populated_returns_false_before_finish_populate() { + let cache = DCache::new(); + let parent = LoadedAddr::new_unchecked(1); + let PopulateStatus::Claimed(_gen) = cache.try_claim_populate(parent) else { + panic!("should claim"); + }; + assert!( + !cache.is_populated(parent), + "in-progress directory should not be populated" + ); +} + +#[tokio::test] +async fn is_populated_returns_true_after_finish_populate() { + let cache = DCache::new(); + let parent = LoadedAddr::new_unchecked(1); + let PopulateStatus::Claimed(claim_gen) = cache.try_claim_populate(parent) else { + panic!("should claim"); + }; + cache.finish_populate(parent, claim_gen); + assert!( + cache.is_populated(parent), + "directory should be populated after finish_populate" + ); +} + +#[tokio::test] +async fn is_populated_returns_false_after_eviction() { + let cache = DCache::new(); + let parent = LoadedAddr::new_unchecked(1); + cache.insert( + parent, + OsString::from("child"), + LoadedAddr::new_unchecked(10), + false, + ); + let PopulateStatus::Claimed(claim_gen) = cache.try_claim_populate(parent) else { + panic!("should claim"); + }; + cache.finish_populate(parent, claim_gen); + assert!(cache.is_populated(parent)); + + cache.evict(LoadedAddr::new_unchecked(10)); + assert!( + !cache.is_populated(parent), + "eviction should reset populated flag" + ); +}