Skip to content

python/config: make download_reactively race-safe under parallel exec…#1534

Merged
nilfm99 merged 1 commit into
masterfrom
make-downloads-thread-safe
May 12, 2026
Merged

python/config: make download_reactively race-safe under parallel exec…#1534
nilfm99 merged 1 commit into
masterfrom
make-downloads-thread-safe

Conversation

@nilfm99
Copy link
Copy Markdown
Collaborator

@nilfm99 nilfm99 commented May 12, 2026

…ution

download_reactively() invokes urllib.request.urlretrieve(remote_path, local_path) which writes incrementally to local_path. Concurrent processes that share the same fixture cache (e.g. pytest-xdist workers) check os.path.exists(local_path) and short-circuit, but between the moment urlretrieve creates the file and the moment it finishes writing, the file exists and is partial. A second worker that arrives in that window opens local_path, reads garbage, and proceeds. For YUV fixtures consumed by feature extractors, the visible failure is non-deterministic feature scores -- the same test produces different actuals on different runs.

Fix: download into a sibling tempfile via tempfile.mkstemp() then os.replace() onto local_path. os.replace() is atomic on POSIX, so concurrent writers can only ever overwrite a complete file with another complete file; readers never observe a partial file. The unique sibling name (mkstemp suffix) also prevents a second race between two workers writing to the same tempfile.

Behaviour preserved:

  • same early-return when local_path already exists,
  • same os.makedirs(parent, exist_ok=True),
  • same urllib.error.HTTPError handling and re-raise.

…ution

download_reactively() invokes urllib.request.urlretrieve(remote_path,
local_path) which writes incrementally to local_path. Concurrent
processes that share the same fixture cache (e.g. pytest-xdist
workers) check os.path.exists(local_path) and short-circuit, but
between the moment urlretrieve creates the file and the moment it
finishes writing, the file exists and is partial. A second worker
that arrives in that window opens local_path, reads garbage, and
proceeds. For YUV fixtures consumed by feature extractors, the
visible failure is non-deterministic feature scores -- the same test
produces different actuals on different runs.

Fix: download into a sibling tempfile via tempfile.mkstemp() then
os.replace() onto local_path. os.replace() is atomic on POSIX, so
concurrent writers can only ever overwrite a complete file with
another complete file; readers never observe a partial file. The
unique sibling name (mkstemp suffix) also prevents a second race
between two workers writing to the same tempfile.

Behaviour preserved:
  - same early-return when local_path already exists,
  - same os.makedirs(parent, exist_ok=True),
  - same urllib.error.HTTPError handling and re-raise.

Co-authored-by: Cursor <cursoragent@cursor.com>
@nilfm99 nilfm99 requested a review from christosbampis May 12, 2026 00:55
@nilfm99 nilfm99 merged commit 32780bd into master May 12, 2026
32 of 36 checks passed
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