fix: stop emptying sys.path while loading rxconfig (intermittent ModuleNotFoundError on reflex run) - #7020
fix: stop emptying sys.path while loading rxconfig (intermittent ModuleNotFoundError on reflex run)#7020Alek99 wants to merge 5 commits into
reflex run)#7020Conversation
`_load_config` cleared `sys.path` down to the project root for the duration of the rxconfig import and restored it afterwards. Python's import system walks `sys.path` by index, so any other thread importing during that window (in `reflex run`, the frontend thread loads the config while the main thread is still importing the backend) fails with ModuleNotFoundError for an unrelated, installed module (`socketio`, `granian`, `reflex_components_sonner` were all observed). The window is small but the backend import is long, so dev starts died intermittently with a spurious missing-module traceback. Insert the project root at the front of `sys.path` once and leave it there, which is what `reflex run` already does for the app module. Restoring the list afterwards is not safe either: removing an entry shifts indices under an in-progress import iterator in another thread (measured ~6% failures). Regression test: a thread loads the config in a loop while the main thread imports a stdlib module 200 times; fails on the old loader, passes now.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Greptile SummaryThe PR prevents concurrent configuration loading from temporarily emptying
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| packages/reflex-base/src/reflex_base/config.py | Replaces destructive sys.path clearing and restoration with an idempotent project-root insertion. |
| tests/units/test_config.py | Adds a concurrency regression test and now keeps the standard-library imports at module scope, resolving the prior review thread. |
| packages/reflex-base/news/7020.bugfix.md | Accurately records the intermittent import failure fixed by the configuration loader change. |
Reviews (3): Last reviewed commit: "test: drop assertion made vacuous by the..." | Re-trigger Greptile
Merging this PR will not alter performance
Comparing Footnotes
|
There was a problem hiding this comment.
All reported issues were addressed
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Re-trigger cubic
|
overlaps with #6933, will decide how to reconcile |
…e race test - config: insert the project root whenever it is not already sys.path[0] (cubic). With the previous `cwd not in sys.path` guard a later config load after a cwd change could resolve rxconfig from a stale project directory that was still ahead of it. Re-inserting only shifts entries back, which an in-flight import in another thread tolerates. - test: the probe was a shared stdlib module (colorsys), which rich also imports during config loading, so on Windows CI the loader thread's own import of it collided with the test's sys.modules.pop and died with KeyError inside importlib. Use a private module in a sibling temp directory appended to sys.path instead; nothing else imports it and it sits outside the project root, so the loader never evicts it. Still fails on main's loader (mutation-checked) and passes on this one. - test: importlib imported at module scope (greptile).
|
Re the overlap with #6933 (same bug, same window). Data that may help reconcile, from the stress script in this PR's description (one thread loading the config in a loop, main thread importing an installed module 300 times):
Removing the entry afterwards still races: |
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
On Linux and Windows CI the probe module was evicted from sys.modules during its own exec (KeyError inside importlib), which only the config loader's dependency bookkeeping does; it does not reproduce locally. find_spec walks sys.path exactly like an import but registers nothing, so the loader cannot touch it. Still fails on main's loader (500 of 500 probes miss during the window) and passes on this one.
Ports the regression test from reflex-dev#7020. The existing test_load_config_keeps_sys_path_usable_for_other_threads gates on an Event with _get_config stubbed out, so it pins the invariant but never exercises the loader around a real rxconfig import. This one runs _load_config in a loop against an actual rxconfig.py while the main thread probes a private module at the end of sys.path with find_spec, which walks sys.path the way an import does without registering anything in sys.modules. Mutation-checked against main's loader (sys.path.clear() + restore): 500/500 probe lookups fail. On this branch's loader it passes, but not deterministically -- see the PR discussion for the measured residual rate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012vT8EThKfwQuoPBjL2hZmo
|
taking #6933 instead |
* fix(config): load rxconfig with cwd prepended instead of swapping out sys.path, racing concurrent first-time imports
_load_config cleared sys.path down to the cwd for the duration of the
rxconfig import, so any concurrent first-time import in another thread
failed with ModuleNotFoundError (e.g. the lazy granian import when the
backend starts while another thread loads the config). Prepending the cwd
keeps the same resolution priority without blinding other threads.
Dropping the clear also removes the except-retry fallback, which had been
papering over a second bug: find_spec("rxconfig") answers from sys.modules,
so a leftover module from another project directory faked the existence
probe. Evict rxconfig from sys.modules before probing instead.
* thanks greptile + add news fragment
* fix: don't misattribute concurrent imports to rxconfig; remove exactly the inserted cwd entry
* more reviews
* Install the rxconfig import recorder only for the duration of the load
* Keep the rxconfig import recorder installed instead of rebinding sys.meta_path
* test: cover the rxconfig sys.path race with a real load loop
Ports the regression test from #7020. The existing
test_load_config_keeps_sys_path_usable_for_other_threads gates on an
Event with _get_config stubbed out, so it pins the invariant but never
exercises the loader around a real rxconfig import. This one runs
_load_config in a loop against an actual rxconfig.py while the main
thread probes a private module at the end of sys.path with find_spec,
which walks sys.path the way an import does without registering anything
in sys.modules.
Mutation-checked against main's loader (sys.path.clear() + restore):
500/500 probe lookups fail. On this branch's loader it passes, but not
deterministically -- see the PR discussion for the measured residual
rate.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012vT8EThKfwQuoPBjL2hZmo
* test: let the sys.path race test re-run instead of failing PRs
Taking the prepended cwd entry back out is itself a sys.path shrink, so
a probe walk that overlaps that one `del` can still skip an entry --
about 1 lookup in 5000, which flaked ~12% of runs. That residual is
worth watching but is not actionable by whoever happens to open the next
PR, so mark it flaky and let pytest-rerunfailures absorb it.
reruns=3 puts a spurious failure at roughly 0.02% per run. Verified the
reruns fire and that the test still fails all four attempts against a
loader that clears sys.path outright, so a real regression is not
masked.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012vT8EThKfwQuoPBjL2hZmo
* tersify news fragment
* fix(config): pass the load root into _get_config instead of re-reading cwd
_get_config classified recorded imports as project-local by comparing
them against Path.cwd() read *after* the rxconfig import. An rxconfig.py
that changes the cwd (os.chdir, or anything it imports doing so) moved
that root out from under the comparison, so its own project-local
imports looked external: they were never recorded as deps, so the next
load never evicted them and the following project inherited them from
sys.modules.
_load_config already resolves the root before prepending it to sys.path;
hand that same Path down rather than resolving it twice. Removal of the
sys.path entry was already immune to the drift, since it matches the
inserted str by identity.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012vT8EThKfwQuoPBjL2hZmo
refactor(config): fold _get_config into _load_config
With the retry-on-failure path gone there is nothing left for the inner
function to be a seam for: _load_config prepared sys.path, called
_get_config exactly once, and tore sys.path back down. Merging them
removes the indirection and the second place a project root had to be
plumbed through.
_load_config now takes project_root, defaulting to Path.cwd() resolved
once up front, so a caller can load a config from a directory other than
the cwd.
Test changes follow the seam disappearing:
- The two tests that stubbed _get_config to pause or perturb a load now
drive a real rxconfig.py instead, which exercises the actual import
path rather than a stub. Both were re-checked against the behavior
they cover: by-value sys.path removal and clear()+restore each still
fail them.
- The event-pair setup those tests share with the concurrent-dep test is
now a race_gate fixture rather than being rebuilt inline.
- test_app.py patches _load_config, the seam that remains.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012vT8EThKfwQuoPBjL2hZmo
refactor(config): keep the logic in _get_config, _load_config as entry point
Same merge as the previous commit, inverted: _get_config holds the
prepare/import/teardown body and takes the optional project_root, and
_load_config is the no-argument entry point get_config and reload_config
call. Keeping the _get_config name where the work happens leaves the 34
existing mock sites in test_app.py untouched, so the net delta is two
files instead of three.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012vT8EThKfwQuoPBjL2hZmo
deprecate _load_config in favor of _get_config
get_config and reload_config now call _get_config directly, so nothing
in the tree reaches _load_config any more. Since it may have callers
outside the repo, keep the name working for now: it warns via
console.deprecate (0.9.11, removal in 1.0) and delegates. A
typing_extensions.deprecated stub under TYPE_CHECKING gives IDEs and
type checkers the same signal without a second runtime warning.
Test call sites move to _get_config with the loader. Two of them were
patch targets (monkeypatch of the loader behind get_config, and the
reload side_effect pair) that would have silently stopped intercepting
had they kept pointing at the old name.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012vT8EThKfwQuoPBjL2hZmo
* tersify deprecation news fragment
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012vT8EThKfwQuoPBjL2hZmo
* _get_config: resolve absolute project_root path
---------
Co-authored-by: Masen Furer <m_github@0x26.net>
Co-authored-by: Claude <noreply@anthropic.com>
Summary
reflex runintermittently died at backend start with a spuriousModuleNotFoundErrorfor an installed package (socketio,granian,reflex_components_sonnerwere all observed on the same machine, same venv). It happened on roughly half of dev-server starts while benchmarking a larger app.Mechanism.
reflex_base.config._load_config()clearedsys.pathdown to[cwd]for the duration of therxconfigimport and restored it afterwards. Inreflex runthe frontend thread's firstget_config()(a freshRegistrationContext) does that load while the main thread is still insideimport reflex.app/ the granian import. Python's import system walkssys.pathby index, so any first-time import that overlaps the window fails.Fix. Insert the project root at the front of
sys.pathonce and leave it there (whichreflex runalready does for the app module viaget_app_file()). Restoring the list afterwards is not safe either: removing an entry shifts indices under an in-progress import iterator in another thread. Measured with a stress script (one thread loading the config in a loop, main thread importingsocketio300 times):sys.path.clear()+ restore)The
except Exception: retry with the original pathfallback is gone with it: the original entries are never removed, so there is nothing to retry with.Test
test_load_config_keeps_sys_path_intact_for_other_threads: a thread loads the config in a loop while the main thread imports a stdlib module 200 times. Fails on the old loader, passes on this one (mutation-checked).🤖 Generated with Claude Code