Repository navigation
fix(download): make concurrent first-run downloads safe - #555
Merged
Merged
Conversation
_extract_archive deleted the install dir and then extracted into it in place. When several processes or threads start on an empty cache, a second caller can delete the first caller's install mid-extraction, or pass the "binary exists and is executable" check on a binary that is still being written. On Linux that surfaces as ETXTBSY on spawn; on macOS as dlopen failures and "Directory not empty", and the cache can be left permanently incomplete. Extract into a uniquely named sibling staging dir and rename it into place. If the destination already holds a complete install, another caller won and the staged copy is discarded; an incomplete leftover is still replaced as before.
Same race as the Python wrapper: extractArchive deleted the install dir and extracted into it in place, so concurrent ensureBinary() calls on an empty cache could delete each other's install or return a binary that was still being written (spawn ETXTBSY on Linux). Extract into a uniquely named sibling staging dir and rename it into place, keeping an existing complete install if another caller won. Also name the temporary archive with randomUUID() instead of Date.now(), which collided for calls made in the same millisecond.
Same race as the Python and JS wrappers: ExtractArchive deleted the install dir and extracted into it in place, so concurrent EnsureBinaryAsync() calls on an empty cache failed with "already exists" / DirectoryNotFoundException or saw a half-written install. Extract into a uniquely named sibling staging dir and move it into place, keeping an existing complete install if another caller won.
When an incomplete install dir was left over from an interrupted run, every concurrent caller saw it as incomplete and deleted it in place. The first to finish renamed its staging dir into place; the others' unguarded second rename then failed (ENOTEMPTY, FileNotFoundError, DirectoryNotFoundException), and a late delete could remove files from an install another caller had just moved in. With 8 concurrent callers, 7 raised in each round. Rename the leftover aside to a unique <install>.stale-<id> dir and delete it from there, then retry the staging rename a bounded number of times, keeping a complete install if another caller got there first. Applies to the Python, JS and .NET clients, each with a regression test that starts several concurrent callers on a partial install. The JS test uses separate processes, since callers in one process cannot interleave inside the synchronous rename block.
Antivirus and indexers on Windows briefly hold freshly extracted files open, which fails the directory rename with access denied. Retry that rename for about 2.5 seconds while the destination does not exist, in all three clients. Cleanup of the staging and stale dirs is now best effort so a failed delete never masks the error being raised.
…rent-extract # Conflicts: # js/src/download.ts
Member
|
Thanks for this, @Shub3am. Really solid work. We resolved a small conflict with main (the PRO_MAJOR bump in download.ts) and merged it. The fix will ship in the next wrapper release for Python, JS and .NET. |
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
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.
Problem
When several processes or threads start on an empty cache, they all download the binary and extract it.
_extract_archive(and its JS and .NET ports) deletes the install dir and then extracts into it in place. That leaves two failure windows:This is common in CI and in worker pools that launch N browsers at once on a fresh machine.
Reproduced against a local mirror of the release archive, with 4 processes started together on an empty cache, before this change:
ENOTEMPTY, Directory not empty,dlopen ... Chromium Framework (no such file). The cache was left with 223 of 917 entries, and it stays broken for later runs because the binary itself exists.OSError: [Errno 66] Directory not empty,dlopenfailures. The install was left at 601 or 909 of 917 entries.spawnSync .../chrome ETXTBSY: the binary was executed while another caller was still writing it.Fix
Same change in Python, JS and .NET:
<install>.partial-<random hex>) in the same cache dir, then rename it into place. The rename is atomic on the same filesystem.<install>.stale-<random>and delete it from there, then retry the rename into place (up to 5 attempts, re-checking for a complete install each time). Deleting it in place raced other callers renaming their extraction into the same path.PermissionError, JSEPERM/EACCES/EBUSY, .NETUnauthorizedAccessExceptionor anIOExceptionwith the access denied, sharing violation or lock violation HRESULT.randomUUID()instead ofDate.now(), which collided for calls made in the same millisecond.No new config, and no behaviour change on the single-caller path.
Tests
New regression tests in each client start several concurrent first-run downloads against a local server and assert that:
Before the fix each new test fails:
Before the leftover-replacement fix, the concurrent partial install tests fail every run (5/5 Python, 8/8 JS, 5/5 .NET):
After it, they passed 10/10 runs in each client.
After the fix, full suites:
pytest -m "not slow"vitest rundotnet build+dotnet test(sdk 8.0, same commands as CI)With the fix, the cross-process repro launched 4/4 and 6/6 processes on an empty cache, each leaving a complete 917-entry install.
Notes for review