fix(assets): link shared assets atomically instead of check-then-act

Merged
AD
2mo ago
+455-13

Problem

rx.asset(shared=True) symlinks the asset into assets/external/ with a
check-then-act sequence that loses every race it can enter:

if not dst_file.exists() and (
    not dst_file.is_symlink() or dst_file.resolve() != src_file_shared.resolve()
):
    try:
        dst_file.symlink_to(src_file_shared)
    except FileExistsError:
        # This happens when Simon builds the app on a bind mount in a docker container.
        dst_file.unlink()
        dst_file.symlink_to(src_file_shared)
  • dst_file.unlink() raises FileNotFoundError when another process removed
    the link between this one's FileExistsError and its unlink().
  • The retry symlink_to() raises FileExistsError when another process
    recreated it in the same window — unhandled, so it escapes too.
  • The guard itself is racy, and wrong independently of concurrency: exists()
    follows the link, so a destination pointing at some other file that happens
    to exist makes it skip instead of repointing.

asset() runs during page evaluation for every shared asset, so any concurrent
compile hits this. In our CI it is pytest-xdist workers each standing up an app
instance against a shared checkout: one split of four fails, the other three
pass on the same commit, and which split fails moves between runs.

FileNotFoundError: [Errno 2] No such file or directory:
  '.../assets/external/reflex_components_internal/components/base/gradient_profile/GradientProfile.js'
Happened while evaluating page 'builder'

The existing comment blames docker bind mounts. That is one cause; the general
one is concurrency, with no container involved — and reading as a niche
environment quirk is why the path was left non-idempotent.

Fix

Build the link under a unique temporary name in the destination directory and
rename it into place. os.replace() is atomic on POSIX and overwrites whatever
the loser of the race left behind, so there is no unlink(), no retry, and no
window another process can invalidate:

try:
    # Already correct: leave it alone so file watchers see no change.
    if dst_file.readlink() == src_file:
        return
except OSError:
    # Missing, or not a symlink: fall through and replace it.
    pass

tmp_file = dst_file.with_name(f".{dst_file.name}.{uuid.uuid4().hex}.tmp")
try:
    tmp_file.symlink_to(src_file)
    tmp_file.replace(dst_file)
except OSError:
    tmp_file.unlink(missing_ok=True)
    raise

No except was widened — errors still propagate, so asset() can never return
a path with no symlink behind it and fail later as a 404 on the built asset.

The exists()/is_symlink() guard is dropped. The replacement fast path
compares readlink() against the intended target, which keeps the "don't churn
the file watcher by re-creating a correct link" property that
remove_stale_external_asset_symlinks() cares about, while actually converging
on the right target when a concurrent writer pointed the link elsewhere.

Verification

Both reported failures reproduce deterministically before the fix, via a
monkeypatched competitor that writes to the destination around each symlink
call:

testold behavior
test_shared_asset_survives_concurrent_removalthe reported FileNotFoundError at dst_file.unlink()
test_shared_asset_survives_concurrent_recreationunhandled FileExistsError from the retry
test_shared_asset_converges_on_correct_target[symlink_to_decoy]leaves the link pointing at the wrong file
test_shared_asset_converges_on_correct_target[regular_file]leaves a stale regular file in place
test_shared_asset_is_thread_safe(new) 8 threads x 25 calls, asserts no temp link is left behind

Unmocked multi-process repro — 8 concurrent processes, 64 tasks x 300 compiles
into one working directory, with a competitor unlinking the destination:
29/64 workers fail before, 0/64 after.

Full unit suite passes (8130 passed, 75.86% coverage); ruff and pyright clean.

Notes for review

  • Behavior change: a plain file at the destination is now replaced by a
    symlink, where exists() previously left it alone. This is the intended
    convergence guarantee, and it is the proper fix for the bind-mount case the
    old comment described rather than the retry. That path is generated territory.
  • Leftover temp links: a process killed between symlink_to and replace
    leaves a dot-prefixed .tmp link pointing at a valid target, which
    remove_stale_external_asset_symlinks() won't reap since it only collects
    broken links. Two-syscall window; happy to add a sweep if reviewers want it.
  • Out of scope, same bug class: remove_stale_external_asset_symlinks() is
    itself check-then-act — path.unlink() and dirpath.rmdir() will raise under
    the same concurrent compiles. Left for a follow-up to keep this diff local.
  • Windows unverified: os.replace uses MOVEFILE_REPLACE_EXISTING so
    replacing an existing link should work, but this was only exercised on Linux.
    Symlink-privilege requirements are unchanged.