diff --git a/packages/reflex-base/news/7075.bugfix.md b/packages/reflex-base/news/7075.bugfix.md new file mode 100644 index 00000000000..d0deb2973ab --- /dev/null +++ b/packages/reflex-base/news/7075.bugfix.md @@ -0,0 +1 @@ +Keep the project-local modules imported by `rxconfig.py` when the config is reloaded from the same project root, so classes they define are not duplicated and states are not registered twice. diff --git a/packages/reflex-base/src/reflex_base/config.py b/packages/reflex-base/src/reflex_base/config.py index afa65b5be50..ba77431fbf1 100644 --- a/packages/reflex-base/src/reflex_base/config.py +++ b/packages/reflex-base/src/reflex_base/config.py @@ -7,7 +7,7 @@ import sys import threading import urllib.parse -from collections.abc import Iterator, Sequence +from collections.abc import Iterable, Iterator, Sequence from contextlib import contextmanager from importlib.machinery import PathFinder from pathlib import Path, PureWindowsPath @@ -841,10 +841,14 @@ def _set_persistent(self, **kwargs): self._replace_defaults(**kwargs) -# Project-local modules first imported while loading rxconfig.py; evicted -# before the next load so projects don't reuse each other's dependencies. -# Only mutated under _load_config_lock. +# Project-local modules first imported while loading rxconfig.py, and the +# project root they were recorded under. Evicted before a load from a different +# root so projects don't reuse each other's dependencies. A load from the same +# root keeps them: re-executing them would create a second copy of every class +# they define, distinct from the one the app already imported. Only mutated +# under _load_config_lock. _config_module_deps: set[str] = set() +_config_module_deps_root: Path | None = None class _ImportRecorder: @@ -884,6 +888,28 @@ def find_spec(self, fullname: str, path: Any = None, target: Any = None) -> None _import_recorder = _ImportRecorder() +def _project_local_modules(names: Iterable[str], project_root: Path) -> set[str]: + """Filter recorded import names down to modules that live in the project. + + Args: + names: Module names observed by the import recorder. + project_root: The root that classifies a module as project-local. + + Returns: + The names whose module file is under project_root and not installed. + """ + project_local: set[str] = set() + for name in names: + origin = getattr(sys.modules.get(name), "__file__", None) + if ( + origin + and (path := Path(origin)).is_relative_to(project_root) + and "site-packages" not in path.parts + ): + project_local.add(name) + return project_local + + @contextmanager def _record_imports() -> Iterator[_ImportRecorder]: """Record imports made on the current thread while rxconfig loads. @@ -949,6 +975,8 @@ def _get_config(project_root: Path | None = None) -> Config: Returns: The app config. """ + global _config_module_deps_root + project_root = (project_root or Path.cwd()).resolve() with _load_config_lock: # A fresh str object, so the exact inserted entry can be removed by @@ -957,14 +985,18 @@ def _get_config(project_root: Path | None = None) -> Config: cwd = str(project_root) sys.path.insert(0, cwd) try: - # Never cache rxconfig or its project-local dependencies — each load - # goes to disk so different RegistrationContexts hold independent - # Config instances resolved against the current project. Evict - # before importing so an earlier project cannot supply the module. + # Never cache rxconfig itself — each load goes to disk so different + # RegistrationContexts hold independent Config instances. sys.modules.pop(constants.Config.MODULE, None) - for dep in _config_module_deps: - sys.modules.pop(dep, None) - _config_module_deps.clear() + if _config_module_deps_root != project_root: + # Evict the previous project's dependencies so this project's + # rxconfig.py imports its own, not same-named modules another + # project directory left behind. Same-root loads skip this so + # the modules the app imported stay the ones rxconfig.py sees. + for dep in _config_module_deps: + sys.modules.pop(dep, None) + _config_module_deps.clear() + _config_module_deps_root = project_root # Only the requested project may supply rxconfig; searching all of # sys.path can pick up an unrelated editable app during reflex init. # PathFinder also supports a project-local rxconfig package. @@ -974,15 +1006,14 @@ def _get_config(project_root: Path | None = None) -> Config: try: rxconfig = importlib.import_module(constants.Config.MODULE) finally: - # Record even on failure so a retry evicts partially-imported deps. - for name in recorder.names: - origin = getattr(sys.modules.get(name), "__file__", None) - if ( - origin - and (path := Path(origin)).is_relative_to(project_root) - and "site-packages" not in path.parts - ): - _config_module_deps.add(name) + # Record even on failure so a later load from another root + # evicts what this one imported. Nothing is evicted here: + # Python already drops a module whose execution failed, and + # one that imported completely may be held by another + # thread, so it is kept like on any same-root reload. + _config_module_deps.update( + _project_local_modules(recorder.names, project_root) + ) return rxconfig.config finally: for i, entry in enumerate(sys.path): diff --git a/tests/units/test_config.py b/tests/units/test_config.py index 2dbd2247701..08e3233048e 100644 --- a/tests/units/test_config.py +++ b/tests/units/test_config.py @@ -3,6 +3,7 @@ import logging import multiprocessing import os +import pickle import sys import textwrap import threading @@ -1126,13 +1127,24 @@ def clean_config_modules() -> Generator[None, None, None]: Yields: None, once the module table is clean. """ - names = ("rxconfig", "side_module", "chdir_dep_module") + names = ( + "rxconfig", + "side_module", + "chdir_dep_module", + "reload_dep_module", + "shared_helper", + "config_reload_state_module", + "failing_dep_module", + "kept_helper", + "failed_only_helper", + ) try: yield finally: for name in names: sys.modules.pop(name, None) reflex_base.config._config_module_deps.clear() + reflex_base.config._config_module_deps_root = None # Reruns: taking the prepended entry back out is itself a sys.path shrink, so @@ -1296,6 +1308,236 @@ def test_config_deps_recorded_against_load_root_when_rxconfig_chdirs( assert "chdir_dep_module" in reflex_base.config._config_module_deps +def test_same_root_reload_keeps_dependency_modules( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, clean_config_modules: None +): + """Reloading from the same project keeps rxconfig's project-local modules. + + rxconfig.py itself is re-read from disk, but the modules it imports must + stay the objects the app already holds. Re-executing them creates a second + copy of every class they define, and pickling an instance of the app's copy + then fails because the qualified name resolves to the other class. + + Args: + tmp_path: The pytest tmp_path fixture. + monkeypatch: The pytest monkeypatch fixture. + clean_config_modules: Cleanup for modules left behind by the load. + """ + (tmp_path / "reload_dep_module.py").write_text("class Marker:\n pass\n") + rxconfig_template = textwrap.dedent( + """ + import reload_dep_module # noqa: F401 + import reflex as rx + + config = rx.Config(app_name={app_name!r}) + """ + ) + (tmp_path / "rxconfig.py").write_text(rxconfig_template.format(app_name="first")) + monkeypatch.chdir(tmp_path) + monkeypatch.delitem(sys.modules, "reload_dep_module", raising=False) + + assert reflex_base.config._get_config().app_name == "first" + module = sys.modules["reload_dep_module"] + marker = module.Marker() + + (tmp_path / "rxconfig.py").write_text( + rxconfig_template.format(app_name="second load") + ) + assert reflex_base.config._get_config().app_name == "second load" + assert sys.modules["reload_dep_module"] is module + assert type(pickle.loads(pickle.dumps(marker))) is module.Marker + assert "reload_dep_module" in reflex_base.config._config_module_deps + + +def test_failed_load_records_dependencies_for_other_root_eviction( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, clean_config_modules: None +): + """A failed load still records its imports so another project evicts them. + + Nothing is evicted at failure time: a module that imported completely may + already be held elsewhere. It is recorded, so a load from a different root + drops it like any other dependency of the previous project. + + Args: + tmp_path: The pytest tmp_path fixture. + monkeypatch: The pytest monkeypatch fixture. + clean_config_modules: Cleanup for modules left behind by the load. + """ + broken = tmp_path / "broken" + broken.mkdir() + (broken / "failing_dep_module.py").write_text("VALUE = 1\n") + (broken / "rxconfig.py").write_text( + textwrap.dedent( + """ + import failing_dep_module # noqa: F401 + + raise RuntimeError("broken rxconfig") + """ + ) + ) + other = tmp_path / "other" + other.mkdir() + (other / "rxconfig.py").write_text( + "import reflex as rx\n\nconfig = rx.Config(app_name='other')\n" + ) + monkeypatch.delitem(sys.modules, "failing_dep_module", raising=False) + + with pytest.raises(RuntimeError, match="broken rxconfig"): + reflex_base.config._get_config(broken) + assert "failing_dep_module" in sys.modules + assert "failing_dep_module" in reflex_base.config._config_module_deps + + assert reflex_base.config._get_config(other).app_name == "other" + assert "failing_dep_module" not in sys.modules + assert "failing_dep_module" not in reflex_base.config._config_module_deps + + +def test_failed_reload_keeps_modules_from_last_good_load( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, clean_config_modules: None +): + """A failed same-root reload leaves the last good load's modules alone. + + The running app may hold classes from the last successful load, so those + modules must survive both the failure and the retry after it. What the + failed attempt imported is recorded alongside them. + + Args: + tmp_path: The pytest tmp_path fixture. + monkeypatch: The pytest monkeypatch fixture. + clean_config_modules: Cleanup for modules left behind by the load. + """ + (tmp_path / "kept_helper.py").write_text("class Kept:\n pass\n") + (tmp_path / "failed_only_helper.py").write_text("VALUE = 1\n") + good_rxconfig = textwrap.dedent( + """ + import kept_helper # noqa: F401 + import reflex as rx + + config = rx.Config(app_name="good") + """ + ) + broken_rxconfig = textwrap.dedent( + """ + import kept_helper # noqa: F401 + import failed_only_helper # noqa: F401 + import reflex as rx + + raise RuntimeError("broken rxconfig") + """ + ) + (tmp_path / "rxconfig.py").write_text(good_rxconfig) + monkeypatch.chdir(tmp_path) + monkeypatch.delitem(sys.modules, "kept_helper", raising=False) + monkeypatch.delitem(sys.modules, "failed_only_helper", raising=False) + + assert reflex_base.config._get_config().app_name == "good" + kept = sys.modules["kept_helper"] + + (tmp_path / "rxconfig.py").write_text(broken_rxconfig) + with pytest.raises(RuntimeError, match="broken rxconfig"): + reflex_base.config._get_config() + assert sys.modules["kept_helper"] is kept + assert "failed_only_helper" in reflex_base.config._config_module_deps + + (tmp_path / "rxconfig.py").write_text(good_rxconfig) + assert reflex_base.config._get_config().app_name == "good" + assert sys.modules["kept_helper"] is kept + + +def test_other_root_load_evicts_dependency_modules( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, clean_config_modules: None +): + """Loading a different project evicts the previous project's dependencies. + + Two projects with a same-named helper module must each resolve their own + copy, in whichever order they are loaded. + + Args: + tmp_path: The pytest tmp_path fixture. + monkeypatch: The pytest monkeypatch fixture. + clean_config_modules: Cleanup for modules left behind by the load. + """ + for name, value in (("first", 1), ("second", 2)): + project = tmp_path / name + project.mkdir() + (project / "shared_helper.py").write_text(f"VALUE = {value}\n") + (project / "rxconfig.py").write_text( + textwrap.dedent( + """ + import shared_helper + import reflex as rx + + config = rx.Config(app_name=f"app{shared_helper.VALUE}") + """ + ) + ) + monkeypatch.delitem(sys.modules, "shared_helper", raising=False) + + assert reflex_base.config._get_config(tmp_path / "first").app_name == "app1" + first_helper = sys.modules["shared_helper"] + assert reflex_base.config._get_config(tmp_path / "second").app_name == "app2" + assert sys.modules["shared_helper"] is not first_helper + assert reflex_base.config._get_config(tmp_path / "first").app_name == "app1" + assert sys.modules["shared_helper"] is not first_helper + assert sys.modules["shared_helper"].VALUE == 1 + + +def test_reload_config_keeps_state_module_registered( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, clean_config_modules: None +): + """Reloading a config whose rxconfig.py imports a state module does not redefine the state. + + Re-importing the module would run the state class body again and trip the + shadowing check for the class still registered in the context. Covers both + a plain reload and a reload in a forked context, the shape AppHarness uses. + + Args: + tmp_path: The pytest tmp_path fixture. + monkeypatch: The pytest monkeypatch fixture. + clean_config_modules: Cleanup for modules left behind by the load. + """ + from reflex_base.registry import RegistrationContext + + (tmp_path / "config_reload_state_module.py").write_text( + textwrap.dedent( + """ + import reflex as rx + + + class ConfigReloadState(rx.State): + value: str = "" + """ + ) + ) + (tmp_path / "rxconfig.py").write_text( + textwrap.dedent( + """ + import config_reload_state_module # noqa: F401 + import reflex as rx + + config = rx.Config(app_name="statereload") + """ + ) + ) + monkeypatch.chdir(tmp_path) + monkeypatch.delitem(sys.modules, "config_reload_state_module", raising=False) + + with RegistrationContext() as ctx: + assert reflex_base.config.get_config().app_name == "statereload" + state_cls = sys.modules["config_reload_state_module"].ConfigReloadState + + assert reflex_base.config.reload_config().app_name == "statereload" + assert sys.modules["config_reload_state_module"].ConfigReloadState is state_cls + + forked = ctx.fork() + token = RegistrationContext._context_var.set(forked) + try: + assert reflex_base.config.reload_config().app_name == "statereload" + finally: + RegistrationContext._context_var.reset(token) + assert sys.modules["config_reload_state_module"].ConfigReloadState is state_cls + + def test_record_imports_never_rebinds_meta_path(): """Recording must mutate sys.meta_path in place, never rebind it.