Fix a blocking-pool deadlock in nativelink_util::fs::read_dir [1.6-patch-5]

clippy.toml

@@ -4,6 +4,7 @@ disallowed-methods = [
   { path = "tokio::runtime::Builder::new_current_thread", reason = "use one of the `nativelink-util::task` functions instead" },
   { path = "tokio::runtime::Builder::new_multi_thread", reason = "use one of the `nativelink-util::task` functions instead" },
   { path = "tokio::runtime::Builder::new_multi_thread_alt", reason = "use one of the `nativelink-util::task` functions instead", allow-invalid = true }, # Can be invalid because depends on feature flags
+  { path = "tokio::runtime::Handle::block_on", reason = "never block_on an async op from a blocking-pool thread: it needs a second pool thread and can deadlock the pool" },
   { path = "tokio::runtime::Runtime::block_on", reason = "use one of the `nativelink-util::task` functions instead" },
   { path = "tokio::runtime::Runtime::new", reason = "use one of the `nativelink-util::task` functions instead" },
   { path = "tokio::runtime::Runtime::spawn", reason = "use one of the `nativelink-util::task` functions instead" },

nativelink-util/src/fs.rs

@@ -184,6 +184,14 @@ pub async fn get_permit() -> Result<SemaphorePermit<'static>, Error> {
         .map_err(|e| make_err!(Code::Internal, "Open file semaphore closed {:?}", e))
 }
 /// Acquire a permit from the open file semaphore and call a raw function.
+///
+/// `f` runs on the blocking pool, so it must be *purely* blocking `std::fs`
+/// work. It must never `block_on` an async operation: `tokio::fs` is itself
+/// implemented with `spawn_blocking`, so a nested `block_on` needs a second
+/// pool thread while already holding one. Enough concurrent callers then park
+/// every pool thread on inner tasks that can never be scheduled, freezing all
+/// `fs::` operations in the process. Use [`get_permit`] plus a direct `.await`
+/// for anything already async (see [`read_dir`], [`symlink`]).
 #[inline]
 pub async fn call_with_permit<F, T>(f: F) -> Result<T, Error>
 where
@@ -376,15 +384,13 @@ impl AsMut<tokio::fs::ReadDir> for ReadDir {
 
 pub async fn read_dir(path: impl AsRef<Path>) -> Result<ReadDir, Error> {
     let path = path.as_ref().to_owned();
-    let (permit, inner) = call_with_permit(move |permit| {
-        Ok((
-            permit,
-            tokio::runtime::Handle::current()
-                .block_on(tokio::fs::read_dir(path))
-                .map_err(Into::<Error>::into)?,
-        ))
-    })
-    .await?;
+    // Deliberately NOT `call_with_permit`: `tokio::fs::read_dir` is already
+    // async, so it must be awaited directly rather than `block_on`ed from a
+    // blocking-pool thread. See `call_with_permit` for why.
+    let permit = get_permit().await?;
+    let inner = tokio::fs::read_dir(path)
+        .await
+        .map_err(Into::<Error>::into)?;
     Ok(ReadDir { permit, inner })
 }
 

nativelink-util/tests/fs_test.rs

@@ -59,3 +59,33 @@ async fn freebind_allows_binding_unassigned_address() -> Result<(), Box<dyn core
 
     Ok(())
 }
+
+// Regression test: `fs::read_dir` must complete with a single blocking-pool
+// thread. The pre-fix implementation `block_on`ed `tokio::fs::read_dir`
+// (itself a `spawn_blocking`) from inside a blocking-pool thread, so each
+// call needed two pool threads at once; enough concurrent callers parked
+// every thread on inner tasks that could never run, freezing all `fs::` ops
+// process-wide. On a one-thread pool the old code deadlocks and the timeout
+// below fires.
+#[test]
+#[expect(
+    clippy::disallowed_methods,
+    reason = "test needs a runtime with a one-thread blocking pool; no util wrapper exposes max_blocking_threads"
+)]
+fn read_dir_needs_only_one_blocking_thread() -> Result<(), Box<dyn core::error::Error>> {
+    let rt = tokio::runtime::Builder::new_multi_thread()
+        .worker_threads(1)
+        .max_blocking_threads(1)
+        .enable_all()
+        .build()?;
+    rt.block_on(async {
+        let read_dir = tokio::time::timeout(
+            core::time::Duration::from_secs(5),
+            nativelink_util::fs::read_dir(env::temp_dir()),
+        )
+        .await
+        .expect("read_dir deadlocked: it required a second blocking-pool thread")?;
+        drop(read_dir);
+        Ok(())
+    })
+}