Skip to content

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

Open
cormacrelf wants to merge 3 commits into
TraceMachina:mainfrom
cormacrelf:1.6-patch-5
Open

cormacrelf wants to merge 3 commits into
TraceMachina:mainfrom
cormacrelf:1.6-patch-5

Conversation

@cormacrelf

@cormacrelf cormacrelf commented Jul 31, 2026 •

Copy link
Copy Markdown
Contributor

What and why

The previous implementation acquired a blocking-pool permit and then, from inside that blocking-pool thread, called Handle::current().block_on(tokio::fs::read_dir(path)). But tokio::fs::read_dir is itself implemented as a spawn_blocking, so this nested block_on required a second blocking-pool thread to be available to make progress — every call to read_dir needed two pool threads at once instead of one.

Under enough concurrent callers (e.g. upload_directory's unbounded directory-tree fan-out on an action with many inputs), all blocking-pool threads could end up parked waiting on inner tasks that could never get a thread to run on, freezing every fs:: operation in the process. I believe this may have caused deadlocks on actions with a lot of inputs (700+).

Fix: acquire the permit, then simply .await tokio::fs::read_dir directly instead of block_on-ing it — no nested blocking-pool thread is required.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How was this verified?

New regression test read_dir_needs_only_one_blocking_thread in nativelink-util/tests/fs_test.rs: builds a runtime with max_blocking_threads(1) and asserts fs::read_dir completes within a 5s timeout. The pre-fix implementation deadlocks in this configuration since it requires two blocking-pool threads at once.

Risk

Low. Only changes how read_dir waits on tokio::fs::read_dir. Removes a layer of blocking in async, so the final code is simpler.

AI assistance

Opus did most of the work here.

Checklist

  • Updated documentation if needed
  • Tests added/amended
  • bazel test //... passes locally
  • PR is contained in a single commit, using git amend see some docs

This change is Reviewable

@vercel

vercel Bot commented Jul 31, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
nativelink Ready Ready Preview Oct 6, 2026 5:02am UTC
nativelink-aidm Ready Ready Preview Oct 6, 2026 5:02am UTC

Request Review

@amankrx

amankrx commented Aug 20, 2026

Copy link
Copy Markdown
Member

Hi @cormacrelf, Can you please fix the pre-commit hook test that's failing right now?

@github-actions

This comment has been minimized.

This branch was successfully deployed

2 active deployments
Preview – nativelink — 8828633d Deployed Oct 6, 2026 by vercel[bot]
Preview – nativelink-aidm — 8828633d Deployed Oct 6, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants