Skip to content

fix(config): load rxconfig with cwd prepended - #6933

Open
benedikt-bartscher wants to merge 12 commits into
reflex-dev:mainfrom
benedikt-bartscher:fix-config-syspath-race
Open

fix(config): load rxconfig with cwd prepended#6933
benedikt-bartscher wants to merge 12 commits into
reflex-dev:mainfrom
benedikt-bartscher:fix-config-syspath-race

Conversation

@benedikt-bartscher

@benedikt-bartscher benedikt-bartscher commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Review in cubic

… 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.
@benedikt-bartscher
benedikt-bartscher marked this pull request as ready for review August 23, 2026 17:59
@benedikt-bartscher
benedikt-bartscher requested a review from a team as a code owner August 23, 2026 17:59
@greptile-apps

greptile-apps Bot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR changes configuration loading to preserve the process import path during rxconfig.py imports and scopes dependency recording to the loading thread.

  • Prepends cwd temporarily instead of replacing sys.path.
  • Records project-local imports through a thread-aware meta-path observer.
  • Evicts stale configuration modules before probing and reloading.
  • Adds regression coverage for concurrent imports, path cleanup, dependency tracking, and meta-path replacement.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
packages/reflex-base/src/reflex_base/config.py Reworks config import-path handling and dependency recording while resolving the previously reported cleanup and meta-path lifecycle failures.
tests/units/test_config.py Adds focused regression coverage for concurrent configuration loading and import-system mutations.
packages/reflex-base/news/6933.bugfix.md Documents the corrected config-loading and concurrent-import behavior.

Reviews (10): Last reviewed commit: "Keep the rxconfig import recorder instal..." | Re-trigger Greptile

Comment thread packages/reflex-base/src/reflex_base/config.py Outdated
Comment thread tests/units/test_config.py
@codspeed-hq

codspeed-hq Bot commented Aug 23, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 32 untouched benchmarks
⏩ 8 skipped benchmarks1


Comparing benedikt-bartscher:fix-config-syspath-race (5a384dd) with main (dd96aea)

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.

@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 packages/reflex-base/src/reflex_base/config.py
Comment thread packages/reflex-base/src/reflex_base/config.py Outdated

@abulvenz abulvenz 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.

Thanks, nice finding. I would improve on the in-code-comment as annotated. In the tests those comments can help to understand why that test is needed.

Comment thread packages/reflex-base/src/reflex_base/config.py Outdated
Comment thread packages/reflex-base/src/reflex_base/config.py Outdated
@masenf

masenf commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator

Thanks for digging into this @benedikt-bartscher — the sys.path half is a clear win. Clearing sys.path out from under every other thread was always going to bite us eventually, and prepend + remove-by-identity is the right shape. Nice bonus that it lets the except Exception: retry fallback go away; that only ever existed because the path was nuked in the first place.

Where I'm less sure is _ImportRecorder. Leaving a finder in sys.meta_path[0] permanently means every not-yet-cached import in the process pays a Python-level call for the rest of its life, in any process that ever loads a Reflex config — including someone who just did import reflex in a notebook. Your reasoning for why it can't be removed is right (_find_spec iterates sys.meta_path unlocked, so pulling it out mid-iteration can skip a real finder), but to me that reads more as an argument against the hook than for making it permanent.

And what it buys us seems fairly small? Worst case with the plain sys.modules diff is that a module another thread happened to import concurrently gets misattributed and evicted from sys.modules on the next load. The thread holding the reference is unaffected — it just means a redundant re-import later. That doesn't feel like it's worth a process-wide import hook.

My inclination would be to land the sys.path fix on its own, since that's where the actual user-visible bug is, and either drop the recorder or handle the misattribution some cheaper way. Happy to be talked out of it if you've hit a case where the eviction actually breaks something — you've clearly spent more time in here than I have.

Couple of smaller things while I'm looking:

  • Popping rxconfig from sys.modules before the find_spec probe: the motivation makes sense, but it's asymmetric now. _config_module_deps is still only cleared after the probe, so leaving a project directory drops rxconfig but strands the old project's deps in both sys.modules and the dep set until some later load. I'd rather both happened on the same side of the probe.
  • The new tests pop colorsys / side_module out of sys.modules and never put them back, and the cleanup at the end of test_concurrent_import_not_recorded_as_rxconfig_dep isn't in a finally, so a failing assert leaks state into whatever runs next. monkeypatch.delitem(..., raising=False) would cover both.
  • If the recorder does stay: check self._thread is not None before calling threading.get_ident(), so the inactive path is a single attribute load rather than a syscall-ish call on every import.

Generated by Claude Code

@benedikt-bartscher

Copy link
Copy Markdown
Contributor Author

@masenf good push — you're right that "make it permanent" is a bad trade, but I think there's a third option I missed.

The hazard is in-place mutation, not removal. _find_spec does meta_path = sys.meta_path and iterates that object, so .remove() shifts the list under a concurrent iterator — but rebinding sys.meta_path = [f for f in sys.meta_path if f is not recorder] leaves the concurrent iterator holding the old list, where the recorder is still present and still returns None. Nothing gets skipped. CPython only started copying meta_path in 3.14 (gh-130094); I checked 3.10–3.13 and they all iterate the live list, so this matters for most of our users. Filtering instead of remove() also covers the rxconfig-rebuilt-meta_path case that the ValueError note was about.

So the recorder is now created per load and installed/removed by a pair of rebinds — nothing is left in sys.meta_path afterwards. No process-wide cost for someone who just did import reflex in a notebook, and your third point about get_ident() goes away with it.

On what it buys: I'd gently push back on "redundant re-import". Eviction is filtered to project-local modules, so what gets misattributed is an app module, and re-importing one of those isn't free:

StateValueError: The substate class 'appmod____my_state' has been defined multiple times.
Shadowing substate classes is not allowed.

(the shadowing check in State.__init_subclass__ fires on the second class object). I haven't caught this in the wild and the window is narrow, so I'm not claiming it's common — but the failure mode is a crash rather than a redundant import, which is why I wanted attribution to be right. With the hook now temporary the cost side is small enough that I'd rather keep it.

Other two are fixed: dep eviction moved above the find_spec probe so both happen on the same side of it, and the tests now use monkeypatch.delitem(..., raising=False) plus a fixture whose teardown clears rxconfig / side_module / _config_module_deps, so a failing assert can't leak state.

Comment thread packages/reflex-base/src/reflex_base/config.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 3 files (changes from recent commits).

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

Re-trigger cubic

Comment thread packages/reflex-base/src/reflex_base/config.py Outdated
@benedikt-bartscher

Copy link
Copy Markdown
Contributor Author

Valid, and it invalidates what I said in my last comment — @masenf, flagging so you don't act on the wrong version.

I claimed rebinding sys.meta_path made removal safe. It fixes the skip hazard, but it introduces a lost-update: anything another thread inserts in place between the comprehension and the assignment is dropped. That is not hypothetical here — reflex/components/__init__.py does sys.meta_path.insert(0, _ComponentsRedirect()) at import time, reflex.components is lazily imported (a plain import reflex does not pull it in), and the install is guarded by a one-shot if not any(isinstance(f, _ComponentsRedirect) ...) that never re-runs once the module is in sys.modules. So losing that insertion is permanent:

>>> # drop the redirect, then re-import reflex.components (already cached, guard never re-runs)
ModuleNotFoundError: No module named 'reflex.components.radix'

A permanent ModuleNotFoundError for every reflex.components.* import is strictly worse than the misattributed eviction the recorder exists to prevent. Both ways out of sys.meta_path are unsafe — in-place removal can make a concurrent lookup skip a real finder on 3.10–3.13, rebinding drops concurrent insertions — so the recorder goes back to being installed in place and left there, which is the one option that mutates nothing others can observe as missing.

Worth noting the cost is the same shape as something already shipped: _ComponentsRedirect is itself a permanent sys.meta_path[0] finder doing a split(".") on every import miss. Our recorder is one attribute load plus an is not None when inactive, and it is only installed the first time a config actually loads, so a notebook doing import reflex never gets it.

Also took @masenf's third point while I was in there — self._thread is not None short-circuits ahead of threading.get_ident(), so the inactive path makes no call at all.

Regression test pins the invariant directly (sys.meta_path object identity is preserved across a recording window); it fails against the rebind version.

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.

3 participants