[rb] Lock the starting port in a file rather than on the port below it - #17893
Open
ikraamg wants to merge 5 commits into
Open
[rb] Lock the starting port in a file rather than on the port below it#17893ikraamg wants to merge 5 commits into
ikraamg wants to merge 5 commits into
Conversation
|
|
Contributor
PR Summary by QodoUse tmpdir flock-based PortLock to avoid needing a neighbouring port
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
Contributor
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Contributor
|
Code review by qodo was updated up to the latest commit 24fb6c1 |
Member
|
Can you use the PR template? |
ServiceManager held its startup lock by binding port - 1, so starting a driver on port N required N-1 to be free as well. When an unrelated service already listened there, the lock could never be taken: startup spun for 45 seconds and then raised, even though the requested port itself was free. The lock now lives in a file named after the starting port, so it needs no port of its own. Mutual exclusion across processes is unchanged.
The entry was there for a TCPServer rescue that PortLock does not have, and it named a file this branch removes. PortLock type checks without an ignore.
Windows refuses to open a file another process holds a lock on, so the read-only fallback ran during ordinary contention, and an exclusive lock cannot be taken on a read-only handle there. Every driver start on Windows then spun for the full timeout. A file that cannot be opened now counts as the lock being unavailable and is retried.
ikraamg
force-pushed
the
fix/port-lock-without-a-neighbouring-port
branch
from
August 10, 2026 14:37
24fb6c1 to
354bcfc
Compare
Comment on lines
+74
to
+76
| def open_lock_file | ||
| file = File.open(@path, File::RDWR | File::CREAT) # rubocop:disable Style/FileOpen | ||
| file.close_on_exec = true |
Contributor
There was a problem hiding this comment.
1. Tmp lockfile path hijack 🐞 Bug ⛨ Security
PortLock uses a predictable filename under Dir.tmpdir and opens it without validating that the path is a safe regular file, allowing a local process to pre-create/lock that pathname and force ServiceManager startup to block until timeout. This is a local availability/DoS risk and can also produce confusing failures if the path is replaced with a directory/symlink.
Agent Prompt
## Issue description
`PortLock` builds a deterministic lockfile path in `Dir.tmpdir` and opens it without validating file type/ownership or hardening the lock namespace. In shared temp directories this permits local interference (pre-locked file, replaced path, etc.) that can block driver startup until timeout.
## Issue Context
`ServiceManager#start` wraps startup in `port_lock.locked`, so lock acquisition failures directly delay or prevent driver startup.
## Fix Focus Areas
- rb/lib/selenium/webdriver/common/port_lock.rb[33-36]
- rb/lib/selenium/webdriver/common/port_lock.rb[71-81]
- rb/lib/selenium/webdriver/common/service_manager.rb[55-59]
### Suggested implementation direction
- Create a dedicated lock directory under `Dir.tmpdir` with safe permissions (e.g., `0700`) and store lockfiles there.
- Open the lockfile with an explicit mode (e.g., `0o600`) and validate it is a regular file (e.g., `File.lstat` + `file.ftype == 'file'`) before locking.
- Consider defending against symlink/path tricks where supported (e.g., refusing symlinks).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Code review by qodo was updated up to the latest commit 354bcfc |
A file that cannot be opened is treated as the lock being held elsewhere, which is what Windows needs, but a read-only temp directory never becomes writable. Startup spun for the full 45 seconds and then blamed the lock, hiding the real cause, where the socket lock this replaces did not touch the filesystem at all. EROFS now raises straight away and names the path. The lock file is also created 0600 rather than at the default umask, since nothing but the creating process ever needs to open it.
The spec locked port 4444, which names the same file a driver started on the default port uses, so it contended with a real driver or with another run of the spec on the same machine. The port only names the lock file here, so deriving it from the pid is enough to keep runs apart.
Comment on lines
+69
to
+72
| it 'fails without waiting out the timeout when the lock file cannot be created' do | ||
| allow(File).to receive(:open).and_raise(Errno::EROFS) | ||
|
|
||
| expect { port_lock.locked { :never } } |
Contributor
There was a problem hiding this comment.
1. Stubbed file.open in spec 📘 Rule violation ▣ Testability
The new unit test stubs File.open using RSpec, which violates the requirement to avoid mocks unless backed by a contract-driven integration. This can reduce test fidelity by asserting behavior against a mock rather than the real filesystem behavior.
Agent Prompt
## Issue description
`rb/spec/unit/selenium/webdriver/common/port_lock_spec.rb` uses RSpec stubbing (`allow(File).to receive(:open)`) to simulate `Errno::EROFS`. The compliance rule requires avoiding mocks in tests unless using a real integration or a contract-driven stub.
## Issue Context
This test aims to verify the error path when the lock file cannot be created. Instead of mocking `File.open`, prefer a real filesystem scenario (or an explicitly contract-backed fake) that triggers the same failure mode.
## Fix Focus Areas
- rb/spec/unit/selenium/webdriver/common/port_lock_spec.rb[69-74]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+78
to
+80
| rescue Errno::EROFS => e | ||
| raise Error::WebDriverError, "unable to create the lock file #{@path}: #{e.message}" | ||
| rescue Errno::EACCES => e |
Contributor
There was a problem hiding this comment.
2. Misleading erofs error text 🐞 Bug ◔ Observability
PortLock#open_lock_file raises a WebDriverError saying it was unable to "create" the lock file on Errno::EROFS, but Errno::EROFS can also occur when opening an already-existing lock file for read/write on a read-only filesystem. This makes failures harder to diagnose, and the new spec locks in the misleading wording.
Agent Prompt
### Issue description
`PortLock#open_lock_file` rescues `Errno::EROFS` from `File.open(@path, File::RDWR | File::CREAT, ...)` and raises an error that specifically claims it was unable to **create** the lock file. Since the open flags also cover opening an existing file for write, `EROFS` may indicate inability to **open/access** the lock file on a read-only filesystem, not strictly creation.
The unit test currently asserts the “unable to create the lock file” wording, which entrenches the misleading message.
### Issue Context
This is a diagnostic/observability issue (not a locking correctness issue), but it affects how actionable the reported error is in real deployments.
### Fix Focus Areas
- rb/lib/selenium/webdriver/common/port_lock.rb[78-80]
- rb/spec/unit/selenium/webdriver/common/port_lock_spec.rb[69-74]
### Suggested change
- Change the message to something that covers both create/open cases, e.g.:
- `"unable to open the lock file #{@path}: #{e.message}"`
- or `"unable to create/open the lock file #{@path}: #{e.message}"`
- Update the spec expectation regex accordingly (match the new wording).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Code review by qodo was updated up to the latest commit 130c925 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔗 Related Issues
Picks up #10176, which was closed with "if anyone wants to PR a better solution, we can do that".
💥 What does this PR do?
ServiceManager takes its startup lock by binding port - 1, so starting a driver on port N needs N-1 to be free as well. When an unrelated service already listens there the lock can never be taken: startup spins for 45 seconds and then raises, even though the requested port is free. This is easy to hit once you pick driver ports yourself, for example a pool assigning a port per process that lands next to a running Redis. I found it running the Firefox WebDriver BiDi render pipeline for trmnl.com in production.
With a listener on 9998 and a driver requested on 9999:
The lock now lives in a file under Dir.tmpdir named after the starting port, so it needs no port of its own.
Cross-process exclusion is unchanged, covered by a spec that holds the lock and asserts a second lock on the same port is refused while a lock on a different port goes through.
🔧 Implementation Notes
The lock only ever needed to be a name two processes could agree on, and binding a TCP port to get one is what drags an unrelated port into the requirements. A file in tmpdir gives the same mutual exclusion without consuming anything.
Keeping the socket lock and binding the requested port instead cannot work, since the lock is held across find_free_port and the port being probed has to stay free. Picking a lock port further away only moves the collision somewhere less predictable, and skipping the lock when a port was passed explicitly drops the protection for the case it exists for, several processes starting drivers at once.
The retry came out of CI. My first version opened the file read-only when it could not be opened for writing, which is fine on POSIX, but on Windows File.open raises Errno::EACCES while another process holds the lock, and an exclusive lock cannot be taken on a read-only handle there. That turned ordinary contention into a permanent spin and failed all ten Windows targets. EACCES is now treated as the lock not being available yet, and retried.
EROFS is not, since a read-only temp directory never becomes writable. Retrying it spun for the full 45 seconds and then blamed the lock, where the socket lock this replaces did not touch the filesystem at all, so it raises straight away and names the path. The spec for it takes 0.01s with the raise and 2.0s without, which is the timeout it used to wait out.
🤖 AI assistance
💡 Additional Considerations
🔄 Types of changes