Skip to content

fix(resolver): synchronize mutable entry-cache snapshots - #44539

Open
steipete wants to merge 1 commit into
oven-sh:mainfrom
steipete:claude/resolver-entry-cache-snapshots
Open

steipete wants to merge 1 commit into
oven-sh:mainfrom
steipete:claude/resolver-entry-cache-snapshots

Conversation

@steipete

@steipete steipete commented Oct 3, 2026

Copy link
Copy Markdown
Collaborator

Entry.cache can be rewritten while another resolver thread copies it. The per-entry mutex serializes writers, but the kind()/symlink() fast paths read Cell<EntryCache> without that lock. need_stat publishes a lazy fill; it cannot protect an already-started reader from subsequent symlink fills, fd updates, or re-stats. A torn Interned pointer/length can reach path parsing.

Use the existing Guarded<EntryCache> for complete snapshots and field updates, preserving the outer entry mutex's stat/fd lifecycle scopes and the outer-entry → cache lock order. Remove the manual Entry Send/Sync implementations. Add concurrent symlink-fill and re-stat tests to the Miri runner; the existing data-URL tests remain enabled normally and are ignored only under Miri because they call native SIMD functions.

This follows up the mutable-cache race explicitly left open by #37274. It is complementary to #40258, which protects the separate abs_path field. The broader #38365 realpath rewrite also retains Cell<EntryCache> and unlocked fast readers at the inspected head; no upstream mutable-cache fix was found.

Validation used the OpenClaw Bun fork, whose prepatch fs.rs, Guarded, and Mutex sources are identical to this upstream base:

  • A native TSan test against the real Entry methods fails unpatched (exit 66), reporting Cell<EntryCache>::set in set_cache_symlink racing Cell::get in symlink/cache. Patched fill and re-stat tests both pass TSan and Miri. TSan rebuilt std with unwind; unrelated data-URL tests were omitted from that temporary native diagnostic to avoid their SIMD link dependencies, then restored.
  • Patched ASAN worker stress: 210 seconds, 256 worker lifetimes, 262,144 imports/requires/resolutions, and 3,615 concurrent symlink-churn/router-reload/build iterations; no sanitizer report.
  • All 12 Rust target triples pass. Resolver all-target clippy and formatting pass. ASAN resolver/router/worker/source-lint suites have 557 passes and one independently reproduced fork-baseline imports-target failure.
  • Matched release builds, eight ABBA samples per arm: warm symlink resolution +1.6%, plain paths +3.6% (about 19ns/resolve), eight-worker contention +5.4%.
  • The original OpenClaw consumer test pair passes 10/10 on the patch (230 assertions), with a passing Node control.

The original consumer native crash has not been directly reproduced; the TSan-reported cache race is the demonstrated defect. A more aggressive symlink-churn fixture still sometimes returns a directory-only resolved path and throws module-not-found on both baseline and patched release binaries. That remaining resolution failure, the separate abs_path race, and the existing cached-fd close/ownership protocol are not claimed fixed here.

Merged fork implementation: openclaw#89 (d894d7fcbf). Both Linux x64 and Darwin arm64 native CI lanes passed on the final fork head: https://github.com/openclaw/bun/actions/runs/37114526179. Independent P2 review of this upstream candidate is scoped-clean.

Protect symlink slices and stat/fd cache snapshots with Guarded while preserving the existing entry lifecycle mutex. Add concurrent fill and re-stat regression coverage to Miri; the unpatched cache fails ThreadSanitizer.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@coderabbitai

coderabbitai Bot commented Oct 3, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

🧰 Additional context used
📚 Code guidelines (1)
src/CLAUDE.md — configured

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: oven-sh/bun/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 70d7ffed-db6a-4396-bcd7-abec66d5443b
📥 Commits

Reviewing files that changed from the base of the PR and between aa83076 and 970c607.

📒 Files selected for processing (3)
  • scripts/rust-miri.ts
  • src/resolver/data_url.rs
  • src/resolver/fs.rs

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.


Walkthrough

The resolver entry cache now uses guarded access instead of Cell. Concurrent cache tests were added. The default Miri crate list now includes bun_resolver, and three data-URL tests are ignored under Miri.

Changes

Resolver cache and Miri coverage

Layer / File(s) Summary
Guarded cache access
src/resolver/fs.rs
Entry.cache now uses Guarded<EntryCache>. Cache snapshots and updates acquire the guard, and successful lazy stat results replace the cache under the guard. The entry_mut accessor and explicit Send/Sync implementations were removed. Concurrent tests exercise cache reads during symlink updates and re-stat operations.
Miri coverage updates
scripts/rust-miri.ts, src/resolver/data_url.rs
The default Miri crate list now includes bun_resolver. Three data-URL tests have Miri-only ignore annotations.

Suggested reviewers: dylan-conway, jarred-sumner

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 970c6

No actionable merge-blocking risk is identified; the change is ready to merge after normal checks.

Security Architecture Review

Security architecture risk: ⚪ Minimal · up to 970c6

The change strengthens synchronization without expanding privileges. The inspected execution and failure paths preserve existing controls, and no material security regression was identified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • observed — The affected state is process-wide resolver entry storage. Cached symlink bytes influence resolved paths, and cached descriptors feed file results. The inspected change synchronizes that existing flow rather than adding a new input source or privileged sink.

Trust Boundaries and Controls

  • observed — The inspected resolver setters, deferred descriptor cleanup, and hot-reload invalidation acquire the entry mutex before accessing the guarded cache. Snapshot and setter methods return without retaining a cache guard. Lazy resolution preserves the existing requirement that its callback not re-enter the entry mutex.
  • observed — Adding the resolver to Miri expands test execution through the existing cargo miri harness. The inspected CI invocation and checkout credential setting remain unchanged, including persist-credentials: false.

Resilience and Maintainability Implications

  • observed — Cache locking uses RAII release. Existing deferred descriptor cleanup still closes and invalidates cached descriptors under the entry mutex, while hot reload invalidates the descriptor before requesting another stat. These paths preserve lifecycle serialization alongside the new snapshot lock.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes synchronizing mutable resolver entry-cache snapshots.
Description check ✅ Passed The description explains the cache synchronization change and provides detailed verification results. It does not use the template headings “What does this PR do?” and “How did you verify your code wo…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

robobun added a commit that referenced this pull request Oct 9, 2026
Store the parts of the published path in an AtomicPtr and an AtomicUsize.
The fill writes the length and then the pointer under the per-entry mutex,
and the Release store of the pointer publishes both. The field is Sync
without the help of `unsafe impl Sync for Entry`, which now covers `cache`
only. The size of Entry does not change.

An in-place refresh already keeps the slot's `dir`, so the store of `dir`
into a recycled entry wrote the same value while other threads read it.
Replace it with a debug assertion and borrow the recycled entry as shared.

The unit test now runs the contended fill eight times and checks that a
reader never sees a published pointer without its length. Under Miri it
fails on every one of 32 seeds when the mutex is removed, when the two
stores are swapped, or when the pointer is stored or loaded with Relaxed.

Use the same Miri list position and the same ignore reason as #44539, so
the two changes do not conflict in those lines.

This branch has not been deployed

No deployments
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.

1 participant