fix: race condition in update lock lets two updates run at once#27
fix: race condition in update lock lets two updates run at once#27Adityakk9031 wants to merge 2 commits into
Conversation
divitsheth
left a comment
There was a problem hiding this comment.
thanks for raising this issue. Using an atomic replacement moves the solution in the right direction but there is one remaining gap. Process A can replace the lock and confirm ownership, but process B can then replace it and also confirm ownership. Both could still proceed with the update.
could you replace the PID-based ownership check with filelock.FileLock and hold that lock inside UpdateLockLease for the full update? Please add filelock as a direct dependency. This gives us OS-backed locking on macOS, Linux, and Windows, with automatic release when the process exits.
It would also help to add a multiprocessing test where process A holds the lock and process B cannot acquire it. The current mocked test returns during the stale check before exercising the replacement race.
Thanks again. This is close and just needs the final ownership guarantee before merging.
|
ok |
close:#26
The update lock had a classic TOCTOU bug. When the lock was stale, the code would:
Between step 2 and 3, another process could grab the lock. Now both processes think they own it, so both fire off uv tool upgrade or pip install at the same time. Corrupts the install.
The fix uses os.replace() to atomically swap in the new lock — no window between removing the old one and planting the new one. Also added a read-back safety check: after the replace, we verify our PID is still in the file. If another process raced past us, we detect it and back off.
Tests cover all the paths: fresh lock, non-stale refusal, stale replacement, and the race-lost scenario.