diff --git a/news/7039.bugfix.md b/news/7039.bugfix.md new file mode 100644 index 00000000000..d797a27fe5c --- /dev/null +++ b/news/7039.bugfix.md @@ -0,0 +1 @@ +Compiling an app from several processes against one working directory — pytest-xdist workers, parallel builds, or containers sharing a bind mount — no longer aborts with `FileNotFoundError` or `FileExistsError` while linking a `rx.asset(shared=True)` file into `assets/external/`. A shared asset whose link already points at a different file is repointed at the asset rather than left alone. diff --git a/reflex/assets.py b/reflex/assets.py index 5062dd378b6..6036c7a7b56 100644 --- a/reflex/assets.py +++ b/reflex/assets.py @@ -4,6 +4,7 @@ import inspect import logging import time +import uuid from pathlib import Path from typing import TYPE_CHECKING, overload @@ -15,6 +16,8 @@ _HASH_CHUNK_SIZE = 1024 * 1024 _MAX_HASH_ATTEMPTS = 3 +_MAX_LINK_ATTEMPTS = 3 +_LINK_RETRY_DELAY = 0.01 if TYPE_CHECKING: from typing_extensions import Buffer @@ -201,6 +204,82 @@ def remove_stale_external_asset_symlinks(): dirpath.rmdir() +def _links_to(dst_file: Path, src_file: Path) -> bool: + """Check whether dst_file is already a symlink to src_file. + + Args: + dst_file: The path to inspect. + src_file: The asset the symlink should point at. + + Returns: + Whether dst_file is a symlink resolving to src_file. + """ + try: + if not dst_file.is_symlink(): + return False + resolved = dst_file.resolve() + except (OSError, RuntimeError): + # An unresolvable destination, such as the symlink loop a crashed or + # racing writer can leave behind, which Python below 3.13 reports as + # RuntimeError. It is not the link we want either way, so replace it. + return False + # The source is the caller's to validate, so its errors are not caught + # here: replacing the destination on a bad source would destroy a good + # link on the way to failing anyway. + return resolved == src_file.resolve() + + +def _link_shared_asset(dst_file: Path, src_file: Path) -> None: + """Point dst_file at src_file with a symlink, regardless of what is there. + + Several processes routinely compile into the same assets/external/ + directory at once: pytest-xdist workers, parallel builds, or containers + sharing a bind mount. Linking in place would be check-then-act, so the link + is built under a unique temporary name in the destination directory and + renamed over dst_file instead, which is one atomic replace on POSIX and + leaves no window to interleave with. Whichever process wins the race, + dst_file is a symlink to src_file once this returns. + + Windows is the exception: replacing a destination that another process is + itself replacing is denied rather than serialised, so the rename is retried + there, conceding as soon as the other process turns out to have linked the + same asset. + + Args: + dst_file: The symlink to create in the app's external assets directory. + src_file: The asset file the symlink should point at. + + Raises: + PermissionError: If the destination could not be replaced. + """ + if _links_to(dst_file, src_file): + # Already correct: leave it alone so file watchers see no change. + return + + # The staged name deliberately does not derive from the asset name: that + # would have to be truncated to fit the filesystem's limit on a path + # component, and cutting a name to a byte budget without splitting a + # character is more machinery than a transient name is worth. + tmp_file = dst_file.with_name(f".{uuid.uuid4().hex}.tmp") + try: + tmp_file.symlink_to(src_file) + for attempt in range(_MAX_LINK_ATTEMPTS): + try: + tmp_file.replace(dst_file) + except PermissionError: # noqa: PERF203 # bounded, and dwarfed by the syscall + if _links_to(dst_file, src_file): + # The process that denied us wanted the same link. + return + if attempt == _MAX_LINK_ATTEMPTS - 1: + raise + time.sleep(_LINK_RETRY_DELAY * (attempt + 1)) + else: + return + finally: + # A no-op once the rename succeeded, and the cleanup if it did not. + tmp_file.unlink(missing_ok=True) + + def asset( path: str, shared: bool = False, @@ -285,17 +364,7 @@ def asset( asset_folder = Path.cwd() / assets / external / subfolder asset_folder.mkdir(parents=True, exist_ok=True) - dst_file = asset_folder / path - - 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) + _link_shared_asset(asset_folder / path, src_file_shared) return _versioned_asset_path( f"/{external}/{subfolder}/{path}", diff --git a/tests/units/assets/test_assets.py b/tests/units/assets/test_assets.py index d411c40ef92..27fc1e5003f 100644 --- a/tests/units/assets/test_assets.py +++ b/tests/units/assets/test_assets.py @@ -1,18 +1,26 @@ import copy import hashlib import io +import itertools import logging +import os import pickle import shutil -from collections.abc import Generator +import threading +from collections.abc import Callable, Generator from pathlib import Path from typing import cast import pytest import reflex as rx +import reflex.assets as assets_module import reflex.constants as constants -from reflex.assets import AssetPathStr, remove_stale_external_asset_symlinks +from reflex.assets import ( + AssetPathStr, + _link_shared_asset, + remove_stale_external_asset_symlinks, +) def _asset_hash(path: Path) -> str: @@ -80,6 +88,370 @@ def test_shared_asset(mock_asset_path: Path) -> None: assert not Path(mock_asset_path / "assets" / "external").exists() +# Captured before any patching so the simulated competitor below can write +# symlinks without recursing back into the fake that wraps `os.symlink`. +_REAL_SYMLINK = os.symlink + + +def _shared_dst_file(mock_asset_path: Path) -> Path: + """Return the symlink `rx.asset(shared=True)` creates for this test module. + + The `test_assets` component is the calling module's name, which is what + `asset()` derives the external subfolder from. + + Args: + mock_asset_path: The mock current working directory. + + Returns: + The path of the symlink in the app's external assets directory. + """ + return ( + mock_asset_path + / constants.Dirs.APP_ASSETS + / constants.Dirs.EXTERNAL_APP_ASSETS + / "test_assets" + / "custom_script.js" + ) + + +def _competitor_links(dst_file: Path, target: Path) -> None: + """Have the competing process point `dst_file` at `target`. + + Args: + dst_file: The destination the competitor writes to. + target: The file the competitor links to. + """ + dst_file.unlink(missing_ok=True) + _REAL_SYMLINK(target, dst_file) + + +def _simulate_competing_process( + monkeypatch: pytest.MonkeyPatch, + script: list[tuple[Callable[[], None], Callable[[], None]]], +) -> None: + """Run another process's writes around each `os.symlink` call. + + Models a second app compiling into the same working directory: each entry + of `script` is a (before, after) pair applied around the n-th symlink call, + so the competitor can create, remove or repoint the destination inside the + window a check-then-act implementation depends on. The last entry is reused + once the script is exhausted. + + Patching `os.symlink` rather than the asset code keeps the simulation + implementation-agnostic: an implementation that links straight to the + destination sees the competitor's writes collide with its own, while one + that links to a private temporary name is untouched by them. + + Args: + monkeypatch: A pytest fixture for patching. + script: The (before, after) callbacks to apply to successive calls. + """ + real_symlink = os.symlink + calls = itertools.count() + + def fake_symlink(target, path, *args, **kwargs): + before, after = script[min(next(calls), len(script) - 1)] + before() + try: + return real_symlink(target, path, *args, **kwargs) + finally: + after() + + monkeypatch.setattr(os, "symlink", fake_symlink) + + +def test_shared_asset_survives_concurrent_removal( + mock_asset_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A competitor that creates then removes the link must not break `asset()`. + + Regression test: a destination that existed when the link was created but + was gone again by the time it was cleaned up escaped as `FileNotFoundError` + from `dst_file.unlink()`. + + Args: + mock_asset_path: The mock current working directory. + monkeypatch: A pytest fixture for patching. + """ + source_file = Path(__file__).parent / "custom_script.js" + dst_file = _shared_dst_file(mock_asset_path) + decoy = mock_asset_path / "decoy.js" + decoy.write_text("decoy") + + _simulate_competing_process( + monkeypatch, + [ + ( + lambda: _competitor_links(dst_file, decoy), + lambda: dst_file.unlink(missing_ok=True), + ) + ], + ) + + asset = rx.asset(path="custom_script.js", shared=True) + + assert ( + asset == f"/external/test_assets/custom_script.js?v={_asset_hash(source_file)}" + ) + assert dst_file.is_symlink() + assert dst_file.resolve() == source_file.resolve() + + +def test_shared_asset_survives_concurrent_recreation( + mock_asset_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A competitor recreating the link on every attempt must not break `asset()`. + + Regression test: the retry after `FileExistsError` raised a second, + unhandled `FileExistsError` when the destination reappeared in between. + + Args: + mock_asset_path: The mock current working directory. + monkeypatch: A pytest fixture for patching. + """ + source_file = Path(__file__).parent / "custom_script.js" + dst_file = _shared_dst_file(mock_asset_path) + decoy = mock_asset_path / "decoy.js" + decoy.write_text("decoy") + + _simulate_competing_process( + monkeypatch, [(lambda: _competitor_links(dst_file, decoy), lambda: None)] + ) + + rx.asset(path="custom_script.js", shared=True) + + assert dst_file.is_symlink() + assert dst_file.resolve() == source_file.resolve() + + +@pytest.mark.parametrize("existing", ["symlink_to_decoy", "regular_file"]) +def test_shared_asset_converges_on_correct_target( + mock_asset_path: Path, existing: str +) -> None: + """An existing destination is repointed at the asset rather than trusted. + + Args: + mock_asset_path: The mock current working directory. + existing: What another process left at the destination. + """ + source_file = Path(__file__).parent / "custom_script.js" + dst_file = _shared_dst_file(mock_asset_path) + dst_file.parent.mkdir(parents=True, exist_ok=True) + if existing == "symlink_to_decoy": + decoy = mock_asset_path / "decoy.js" + decoy.write_text("decoy") + dst_file.symlink_to(decoy) + else: + dst_file.write_text("stale copy") + + rx.asset(path="custom_script.js", shared=True) + + assert dst_file.is_symlink() + assert dst_file.resolve() == source_file.resolve() + assert dst_file.read_text() == source_file.read_text() + + +def test_shared_asset_is_thread_safe(mock_asset_path: Path) -> None: + """Concurrent `asset()` calls for the same file all succeed. + + Args: + mock_asset_path: The mock current working directory. + """ + source_file = Path(__file__).parent / "custom_script.js" + dst_file = _shared_dst_file(mock_asset_path) + errors: list[BaseException] = [] + start = threading.Barrier(8) + + def compile_once() -> None: + start.wait() + try: + for _ in range(25): + rx.asset(path="custom_script.js", shared=True) + except BaseException as exc: + errors.append(exc) + + threads = [threading.Thread(target=compile_once) for _ in range(8)] + for thread in threads: + thread.start() + for thread in threads: + thread.join() + + assert not errors + assert dst_file.is_symlink() + assert dst_file.resolve() == source_file.resolve() + # No temporary link is left behind in the destination directory. + assert [p.name for p in dst_file.parent.iterdir()] == ["custom_script.js"] + + +@pytest.mark.parametrize( + "stem", + [ + # All are just under the 255-byte component limit of ext4, APFS and + # NTFS, with no room left for a suffix. The multi-byte names guard a + # staged name derived from the asset name by slicing it: whether the + # slice is taken in characters or in bytes, and if in bytes, whether it + # splits an encoded character (which `off_boundary` does at 64 bytes). + pytest.param("a" * 247, id="ascii"), + pytest.param("😀" * 61, id="multibyte"), + pytest.param("a" + "😀" * 60, id="multibyte_off_boundary"), + ], +) +def test_link_shared_asset_with_long_filename(tmp_path: Path, stem: str) -> None: + """An asset named up to the filesystem's limit still links. + + The temporary name the link is staged under has to stay within the same + limit, so it cannot simply append to an already maximal basename. + + Args: + tmp_path: A temporary directory provided by pytest. + stem: The asset name, without its extension. + """ + name = stem + ".js" + src_dir = tmp_path / "src" + src_dir.mkdir() + src_file = src_dir / name + src_file.write_text("script") + dst_dir = tmp_path / "dst" + dst_dir.mkdir() + + _link_shared_asset(dst_dir / name, src_file) + + assert (dst_dir / name).is_symlink() + assert (dst_dir / name).resolve() == src_file.resolve() + assert [p.name for p in dst_dir.iterdir()] == [name] + + +def _staged_link_fixture(tmp_path: Path) -> tuple[Path, Path]: + """Create a source asset and an empty destination directory. + + Args: + tmp_path: A temporary directory provided by pytest. + + Returns: + The source asset and the destination the link should be created at. + """ + src_dir = tmp_path / "src" + src_dir.mkdir() + src_file = src_dir / "custom_script.js" + src_file.write_text("script") + dst_dir = tmp_path / "dst" + dst_dir.mkdir() + return src_file, dst_dir / "custom_script.js" + + +def test_link_shared_asset_replaces_symlink_loop(tmp_path: Path) -> None: + """A destination caught in a symlink loop is replaced, not reported. + + Resolving a loop raises `RuntimeError` on Python below 3.13, so a + destination left in that state by a crashed or racing writer would + otherwise abort the compile. + + Args: + tmp_path: A temporary directory provided by pytest. + """ + src_file, dst_file = _staged_link_fixture(tmp_path) + partner = dst_file.parent / "loop_partner.js" + dst_file.symlink_to(partner) + partner.symlink_to(dst_file) + + _link_shared_asset(dst_file, src_file) + + assert dst_file.is_symlink() + assert dst_file.resolve() == src_file.resolve() + + +def test_link_shared_asset_leaves_destination_on_source_error( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A source that cannot be resolved surfaces, without touching the link. + + Only the destination is inspected defensively; swallowing a source error + would replace a good link on the way to failing on the source anyway. + + Args: + tmp_path: A temporary directory provided by pytest. + monkeypatch: A pytest fixture for patching. + """ + src_file, dst_file = _staged_link_fixture(tmp_path) + decoy = tmp_path / "decoy.js" + decoy.write_text("decoy") + dst_file.symlink_to(decoy) + real_resolve = Path.resolve + + def fake_resolve(self: Path, *args, **kwargs) -> Path: + if self == src_file: + msg = "Symlink loop" + raise RuntimeError(msg) + return real_resolve(self, *args, **kwargs) + + monkeypatch.setattr(Path, "resolve", fake_resolve) + + with pytest.raises(RuntimeError): + _link_shared_asset(dst_file, src_file) + + # Compared after resolution: Windows reads a symlink back with a `\\?\` + # prefix, so the raw target is not comparable to the path it was made from. + assert dst_file.is_symlink() + assert dst_file.resolve() == decoy.resolve() + + +def test_link_shared_asset_concedes_denied_replace( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A replace denied by the process that linked the same asset is conceded. + + Windows denies a replace while another process is replacing the same + destination, so this cannot be reached on POSIX without the patch. + + Args: + tmp_path: A temporary directory provided by pytest. + monkeypatch: A pytest fixture for patching. + """ + src_file, dst_file = _staged_link_fixture(tmp_path) + + def denied_replace(self: Path, target) -> Path: + _REAL_SYMLINK(src_file, dst_file) + msg = "Access is denied" + raise PermissionError(13, msg) + + monkeypatch.setattr(Path, "replace", denied_replace) + + _link_shared_asset(dst_file, src_file) + + assert dst_file.is_symlink() + assert dst_file.resolve() == src_file.resolve() + assert [p.name for p in dst_file.parent.iterdir()] == ["custom_script.js"] + + +def test_link_shared_asset_raises_when_replace_stays_denied( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A destination that never becomes linkable surfaces the error. + + Args: + tmp_path: A temporary directory provided by pytest. + monkeypatch: A pytest fixture for patching. + """ + src_file, dst_file = _staged_link_fixture(tmp_path) + attempts = 0 + + def denied_replace(self: Path, target) -> Path: + nonlocal attempts + attempts += 1 + msg = "Access is denied" + raise PermissionError(13, msg) + + monkeypatch.setattr(Path, "replace", denied_replace) + monkeypatch.setattr(assets_module, "_LINK_RETRY_DELAY", 0) + + with pytest.raises(PermissionError): + _link_shared_asset(dst_file, src_file) + + assert attempts == assets_module._MAX_LINK_ATTEMPTS + # The staged link is cleaned up rather than left in the assets directory. + assert list(dst_file.parent.iterdir()) == [] + + @pytest.mark.parametrize( ("path", "shared"), [