Skip to content

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

Merged
adhami3310 merged 10 commits into
mainfrom
claude/asset-symlink-toctou-race-f7c306
Sep 3, 2026
Merged

fix(assets): link shared assets atomically instead of check-then-act#7039
adhami3310 merged 10 commits into
mainfrom
claude/asset-symlink-toctou-race-f7c306

Conversation

@adhami3310

@adhami3310 adhami3310 commented Sep 2, 2026

Copy link
Copy Markdown
Member

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:

test old behavior
test_shared_asset_survives_concurrent_removal the reported FileNotFoundError at dst_file.unlink()
test_shared_asset_survives_concurrent_recreation unhandled 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.

Review in cubic

`asset(shared=True)` created the symlink into `assets/external/` with a
sequence that was racy at every step: the `exists()`/`is_symlink()` guard,
the `unlink()` in the `FileExistsError` handler, and the retry `symlink_to()`
after it. Concurrent compiles into one working directory — pytest-xdist
workers, parallel builds, containers on a shared bind mount — lost those
races and aborted the compile with `FileNotFoundError` from the `unlink()`,
or with an unhandled `FileExistsError` from the retry.

Build the link under a unique temporary name in the destination directory
and `os.replace()` it into place, which atomically overwrites whatever the
loser of the race left behind and needs no retry. Errors still propagate,
so `asset()` cannot return a path with no symlink behind it.

The old guard is dropped: `exists()` follows the link, so a destination
pointing at some other existing file made it skip rather than repoint. The
replacement fast path compares `readlink()` against the intended target,
keeping the "no needless re-creation for file watchers" property while
actually converging on the right target.

The `FileExistsError` comment attributed this to docker bind mounts; that
is one cause, but the general one is concurrency, with no container involved.
@adhami3310
adhami3310 requested a review from a team as a code owner September 2, 2026 23:30
@codspeed-hq

codspeed-hq Bot commented Sep 2, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 32 untouched benchmarks
⏩ 8 skipped benchmarks1


Comparing claude/asset-symlink-toctou-race-f7c306 (1e49725) with main (7d6edec)

Open in CodSpeed

Footnotes

  1. 8 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@greptile-apps

greptile-apps Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR replaces the racy shared-asset check-and-link sequence with a uniquely staged symlink and atomic destination replacement.

  • Adds bounded handling for concurrent replacement conflicts.
  • Adds regression coverage for concurrent writers, stale destinations, long filenames, symlink loops, and cleanup.
  • Adds a user-facing bug-fix news fragment.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
reflex/assets.py Introduces staged atomic replacement for shared-asset symlinks and routes shared asset creation through the new helper.
tests/units/assets/test_assets.py Adds focused regression tests for concurrency, destination convergence, platform retry behavior, filename limits, and temporary-link cleanup.
news/7039.bugfix.md Documents the externally visible correction to concurrent shared-asset linking.

Reviews (9): Last reviewed commit: "test(assets): compare the untouched link..." | Re-trigger Greptile

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread reflex/assets.py Outdated

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 2 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread reflex/assets.py Outdated

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 2 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread reflex/assets.py Outdated

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 2 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread reflex/assets.py Outdated

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 2 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread reflex/assets.py
@adhami3310
adhami3310 merged commit 3e3732d into main Sep 3, 2026
111 checks passed
@adhami3310
adhami3310 deleted the claude/asset-symlink-toctou-race-f7c306 branch September 3, 2026 01:24
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.

2 participants