Skip to content

[rust] Remove fs2 crate and use standard mechanism for file lock - #18203

Merged
bonigarcia merged 2 commits into
trunkfrom
sm_fs2
Oct 19, 2026
Merged

bonigarcia merged 2 commits into
trunkfrom
sm_fs2

Conversation

@bonigarcia

Copy link
Copy Markdown
Member

🔗 Related Issues

💥 What does this PR do?

This PR removse the fs2 crate. Instead, the code now uses standard mechanism for file locking.

🔧 Implementation Notes

🤖 AI assistance

  • No substantial AI assistance used
  • AI assisted (complete below)
    • Tool(s):
    • What was generated:
    • I reviewed all AI output and can explain the change

💡 Additional Considerations

🔄 Types of changes

  • Cleanup (formatting, renaming)

@qodo-code-review

Copy link
Copy Markdown
Contributor

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@selenium-ci

Copy link
Copy Markdown
Member

/agentic_describe

@selenium-ci

Copy link
Copy Markdown
Member

/agentic_review

@selenium-ci selenium-ci added C-rust Rust code is mostly Selenium Manager B-manager Selenium Manager labels Oct 19, 2026
@qodo-code-review

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Use standard-library file locking in Selenium Manager

✨ Enhancement ⚙️ Configuration changes 🕐 10-20 Minutes

Grey Divider

AI Description

• Replace fs2 locking with standard-library file locking while preserving the existing lock
 lifecycle.
• Remove the fs2 dependency and its lockfile entries.
Diagram

graph TD
    A["Manager processes"] --> B["Lock guard"] --> C["Standard File lock"] --> D["Cache access"]
    B --> E["Lock file"]
Loading
High-Level Assessment

Using standard-library locking is the simplest way to remove fs2 without changing the lock guard's role. Retaining fs2 would preserve the dependency without a clear benefit; confirm that supported Rust toolchains provide these File methods.

Files changed (3) +2 / -15

Refactor (1) +2 / -3
lock.rsAcquire and release locks through std::fs::File +2/-3

Acquire and release locks through std::fs::File

• Replaces fs2's exclusive-lock and unlock calls with File::lock and File::unlock. Lock-file creation, removal, and the existing best-effort error handling remain unchanged.

rust/src/lock.rs

Other (2) +0 / -12
Cargo.tomlRemove the fs2 dependency +0/-1

Remove the fs2 dependency

• Removes fs2 from Selenium Manager's declared dependencies because locking now uses the standard library.

rust/Cargo.toml

Cargo.lockRemove fs2 from the resolved dependency graph +0/-11

Remove fs2 from the resolved dependency graph

• Deletes the fs2 package entry and its Selenium Manager dependency reference. Other resolved packages remain unchanged.

rust/Cargo.lock

@qodo-code-review

qodo-code-review Bot commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Locking regressions can go undetected ✓ Resolved
Description
Lock::acquire and Lock::release now use the standard file-locking implementation, but the tests
only verify lock-file creation and deletion rather than mutual exclusion. A regression that stops a
second Selenium Manager process from waiting for an existing lock can therefore pass the test suite
while allowing concurrent cache access.
Code

rust/src/lock.rs[54]

+        file.lock().unwrap_or_default();
Evidence
The changed implementation replaces the exclusive-lock calls with the standard File::lock and
File::unlock methods, while the existing lock tests only assert that the lock file exists during
acquisition and is removed during release or drop; they do not exercise contention or mutual
exclusion.

AGENTS.md: Provide Focused Tests Without Misleading Mocks
rust/src/lock.rs[51-67]
rust/src/lock.rs[116-145]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The file-locking mechanism changed, but existing tests do not verify that a second process or file handle is excluded while the first lock is held.

## Fix Focus Areas
- rust/src/lock.rs[51-67]
- rust/src/lock.rs[100-150]

## Recommended Fix
Add a focused test that holds one lock, attempts acquisition from a separate thread or process, verifies the competing acquisition remains blocked until release, and then verifies it succeeds after release. Keep the test platform-compatible and ensure temporary lock files and directories are cleaned up.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Older Rust users cannot build the manager ✓ Resolved
Description
Lock::acquire now calls File::lock, which was stabilized in Rust 1.89, although the crate does
not declare a minimum Rust version. A user building through the documented Cargo path with Rust 1.88
encounters a compilation error at this call and the new File::unlock call, while the repository’s
Rust 1.98.1 builds remain unaffected.
Code

rust/src/lock.rs[54]

+        file.lock().unwrap_or_default();
Evidence
The changed calls are in the lock implementation; the manifest has no rust-version, the README
documents direct Cargo builds without a version requirement, and Bazel pins a newer compiler. Rust's
File documentation marks lock as available since 1.89.0.

rust/src/lock.rs[50-65]
rust/Cargo.toml[1-6]
rust/README.md[5-21]
MODULE.bazel[379-386]
🌐 The File::lock API is marked as stable since Rust 1.89.0.

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new standard-library file-lock calls require Rust 1.89, but direct Cargo users are not told that requirement.
## Fix Focus Areas
- rust/src/lock.rs[53-65]
- rust/Cargo.toml[1-6]
- rust/README.md[5-21]
## Recommended Fix
Declare Rust 1.89 or newer as the minimum supported version in the manifest and build instructions. If Rust 1.88 must remain supported, use a locking implementation available on that version instead.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources
❌ Tickets: failed to load — Ticket collection could not establish complete evidence
Review mode: Auto: ⚖️ Balanced: File-locking behavior affects concurrency and requires careful compatibility review.
✅ Fix-loop round: fix loop

Grey Divider

Tip of the day
💡 Did you know, you can import rules and skills only from the repositories your team follows

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Previous reviews

Review updated until commit c7da14d ⚖️ Balanced

Results up to commit 08a3cbe ⚖️ Balanced


🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)


Remediation recommended
1. Locking regressions can go undetected ✓ Resolved
Description
Lock::acquire and Lock::release now use the standard file-locking implementation, but the tests
only verify lock-file creation and deletion rather than mutual exclusion. A regression that stops a
second Selenium Manager process from waiting for an existing lock can therefore pass the test suite
while allowing concurrent cache access.
Code

rust/src/lock.rs[54]

+        file.lock().unwrap_or_default();
Evidence
The changed implementation replaces the exclusive-lock calls with the standard File::lock and
File::unlock methods, while the existing lock tests only assert that the lock file exists during
acquisition and is removed during release or drop; they do not exercise contention or mutual
exclusion.

AGENTS.md: Provide Focused Tests Without Misleading Mocks
rust/src/lock.rs[51-67]
rust/src/lock.rs[116-145]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The file-locking mechanism changed, but existing tests do not verify that a second process or file handle is excluded while the first lock is held.

## Fix Focus Areas
- rust/src/lock.rs[51-67]
- rust/src/lock.rs[100-150]

## Recommended Fix
Add a focused test that holds one lock, attempts acquisition from a separate thread or process, verifies the competing acquisition remains blocked until release, and then verifies it succeeds after release. Keep the test platform-compatible and ensure temporary lock files and directories are cleaned up.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Older Rust users cannot build the manager ✓ Resolved
Description
Lock::acquire now calls File::lock, which was stabilized in Rust 1.89, although the crate does
not declare a minimum Rust version. A user building through the documented Cargo path with Rust 1.88
encounters a compilation error at this call and the new File::unlock call, while the repository’s
Rust 1.98.1 builds remain unaffected.
Code

rust/src/lock.rs[54]

+        file.lock().unwrap_or_default();
Evidence
The changed calls are in the lock implementation; the manifest has no rust-version, the README
documents direct Cargo builds without a version requirement, and Bazel pins a newer compiler. Rust's
File documentation marks lock as available since 1.89.0.

rust/src/lock.rs[50-65]
rust/Cargo.toml[1-6]
rust/README.md[5-21]
MODULE.bazel[379-386]
🌐 The File::lock API is marked as stable since Rust 1.89.0.

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new standard-library file-lock calls require Rust 1.89, but direct Cargo users are not told that requirement.
## Fix Focus Areas
- rust/src/lock.rs[53-65]
- rust/Cargo.toml[1-6]
- rust/README.md[5-21]
## Recommended Fix
Declare Rust 1.89 or newer as the minimum supported version in the manifest and build instructions. If Rust 1.88 must remain supported, use a locking implementation available on that version instead.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment thread rust/src/lock.rs
Comment thread rust/src/lock.rs
@selenium-ci

Copy link
Copy Markdown
Member

/agentic_review

@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit c7da14d

@bonigarcia
bonigarcia merged commit 81a7a01 into trunk Oct 19, 2026
51 checks passed
@bonigarcia
bonigarcia deleted the sm_fs2 branch October 19, 2026 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-manager Selenium Manager C-rust Rust code is mostly Selenium Manager

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants