-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Fix race condition in stateful pages marker with atomic writes #7142
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Fix a startup race in backend-only mode with multiple workers where a worker could read a truncated `.web/backend/stateful_pages.json` and crash with `JSONDecodeError`. The marker is now written atomically, and a worker that finds no marker evaluates all pages instead of assuming there are none. |
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -12,7 +12,9 @@ | |||||||
| import json | ||||||||
| import logging | ||||||||
| import operator | ||||||||
| import os | ||||||||
| import sys | ||||||||
| import tempfile | ||||||||
| import time | ||||||||
| import traceback | ||||||||
| import urllib.parse | ||||||||
|
|
@@ -25,6 +27,7 @@ | |||||||
| Sequence, | ||||||||
| ) | ||||||||
| from contextvars import Token | ||||||||
| from pathlib import Path | ||||||||
| from types import SimpleNamespace | ||||||||
| from typing import TYPE_CHECKING, Any, overload | ||||||||
|
|
||||||||
|
|
@@ -1714,14 +1717,29 @@ def _compile( | |||||||
| clear_hash_caches() | ||||||||
|
|
||||||||
| def _write_stateful_pages_marker(self): | ||||||||
| """Write list of routes that create dynamic states for the backend to use later.""" | ||||||||
| if self._state is not None: | ||||||||
| stateful_pages_marker = ( | ||||||||
| prerequisites.get_backend_dir() / constants.Dirs.STATEFUL_PAGES | ||||||||
| ) | ||||||||
| stateful_pages_marker.parent.mkdir(parents=True, exist_ok=True) | ||||||||
| with stateful_pages_marker.open("w") as f: | ||||||||
| """Write list of routes that create dynamic states for the backend to use later. | ||||||||
|
|
||||||||
| Multiple backend workers may write the marker at the same time, so the | ||||||||
| content is written to a temporary file and swapped into place with | ||||||||
| ``Path.replace`` to ensure readers only ever see a complete marker. | ||||||||
| """ | ||||||||
| stateful_pages_marker = ( | ||||||||
| prerequisites.get_backend_dir() / constants.Dirs.STATEFUL_PAGES | ||||||||
| ) | ||||||||
| stateful_pages_marker.parent.mkdir(parents=True, exist_ok=True) | ||||||||
| fd, tmp_path = tempfile.mkstemp( | ||||||||
| dir=stateful_pages_marker.parent, | ||||||||
| prefix=f"{stateful_pages_marker.name}.", | ||||||||
| suffix=".tmp", | ||||||||
| ) | ||||||||
| tmp_marker = Path(tmp_path) | ||||||||
|
Comment on lines
+1730
to
+1735
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If frontend compilation and backend startup run under different Unix accounts while sharing generated backend artifacts, this marker may become unreadable. Knowledge Base Used: Application runtime |
||||||||
| try: | ||||||||
| with os.fdopen(fd, "w") as f: | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: If Prompt for AI agents
Suggested change
|
||||||||
| json.dump(list(self._stateful_pages), f) | ||||||||
| tmp_marker.replace(stateful_pages_marker) | ||||||||
|
Comment on lines
+1726
to
+1739
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removing the state check makes stateless dry-run compilation create or replace this marker. The compiler calls this method before its dry-run return, so Knowledge Base Used: Frontend compilation pipeline |
||||||||
| except BaseException: | ||||||||
| tmp_marker.unlink(missing_ok=True) | ||||||||
| raise | ||||||||
|
|
||||||||
| def add_all_routes_endpoint(self): | ||||||||
| """Add an endpoint to the app that returns all the routes.""" | ||||||||
|
|
||||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -1206,6 +1206,23 @@ def _register_plugin_routes(app: App, plugins: Sequence[Plugin]) -> None: | |||||
| app._register_plugin_pages(plugins) | ||||||
|
|
||||||
|
|
||||||
| def _read_stateful_pages_marker() -> list[str] | None: | ||||||
| """Read the routes that create state classes from a previous compile. | ||||||
|
|
||||||
| The marker is swapped into place atomically, so it is either complete or | ||||||
| absent. It may be absent because no compile has happened yet or because a | ||||||
| concurrently starting worker has not finished writing it. | ||||||
|
|
||||||
| Returns: | ||||||
| The stateful routes, or None if no marker has been written yet. | ||||||
| """ | ||||||
| marker = prerequisites.get_backend_dir() / constants.Dirs.STATEFUL_PAGES | ||||||
| try: | ||||||
| return json.loads(marker.read_text()) | ||||||
| except FileNotFoundError: | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: Catch Prompt for AI agents
Suggested change
|
||||||
| return None | ||||||
|
Comment on lines
+1219
to
+1223
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
A deployment can retain a truncated marker produced by the previous direct-write implementation, but this reader handles only a missing file. On the first backend-only startup after upgrading, Knowledge Base Used: |
||||||
|
|
||||||
|
|
||||||
| def compile_app( | ||||||
| app: App, | ||||||
| *, | ||||||
|
|
@@ -1231,15 +1248,14 @@ def compile_app( | |||||
| app._pages = {} | ||||||
|
|
||||||
| should_compile = app._should_compile() | ||||||
| backend_dir = prerequisites.get_backend_dir() | ||||||
| if not dry_run and not should_compile and backend_dir.exists(): | ||||||
| stateful_pages_marker = backend_dir / constants.Dirs.STATEFUL_PAGES | ||||||
| if stateful_pages_marker.exists(): | ||||||
| with stateful_pages_marker.open("r") as file: | ||||||
| stateful_pages = json.load(file) | ||||||
| for route in stateful_pages: | ||||||
| logger.debug(f"BE Evaluating stateful page: {route}") | ||||||
| app._compile_page(route, save_page=False) | ||||||
| if not dry_run and not should_compile: | ||||||
| stateful_pages = _read_stateful_pages_marker() | ||||||
| else: | ||||||
| stateful_pages = None | ||||||
| if stateful_pages is not None: | ||||||
| for route in stateful_pages: | ||||||
| logger.debug(f"BE Evaluating stateful page: {route}") | ||||||
| app._compile_page(route, save_page=False) | ||||||
| if app._state is not None: | ||||||
| utils._restore_bundled_libraries() | ||||||
| utils._compile_initial_state(app._state) | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,7 @@ | |
| import multiprocessing | ||
| import pickle | ||
| import re | ||
| import threading | ||
| import unittest.mock | ||
| import uuid | ||
| from collections.abc import Generator | ||
|
|
@@ -4799,3 +4800,82 @@ def test_compile_emits_stage_spans( | |
| parent = spans[name].parent | ||
| assert parent is not None | ||
| assert parent.span_id == root.get_span_context().span_id | ||
|
|
||
|
|
||
| def test_write_stateful_pages_marker_never_truncates_final_path( | ||
| tmp_path: Path, mocker: MockerFixture, monkeypatch: pytest.MonkeyPatch | ||
| ): | ||
| """The marker is swapped into place atomically, never opened for writing.""" | ||
| mocker.patch("reflex.utils.prerequisites.get_backend_dir", return_value=tmp_path) | ||
| marker = tmp_path / constants.Dirs.STATEFUL_PAGES | ||
| original_open = Path.open | ||
| write_opens: list[str] = [] | ||
|
|
||
| def spy_open(self: Path, mode: str = "r", *args, **kwargs): | ||
| if self == marker and mode != "r": | ||
| write_opens.append(mode) | ||
| return original_open(self, mode, *args, **kwargs) | ||
|
|
||
| monkeypatch.setattr(Path, "open", spy_open) | ||
| app = App(_state=rx.State) | ||
| app._stateful_pages = dict.fromkeys(["index", "about"]) | ||
|
|
||
| app._write_stateful_pages_marker() | ||
|
|
||
| assert write_opens == [] | ||
| assert json.loads(marker.read_text()) == ["index", "about"] | ||
| assert [p.name for p in tmp_path.iterdir()] == [constants.Dirs.STATEFUL_PAGES] | ||
|
|
||
|
|
||
| def test_write_stateful_pages_marker_is_always_written( | ||
| tmp_path: Path, mocker: MockerFixture | ||
| ): | ||
| """Stateless apps write an empty marker so backend workers skip page evaluation.""" | ||
| mocker.patch("reflex.utils.prerequisites.get_backend_dir", return_value=tmp_path) | ||
| app = App(enable_state=False) | ||
|
|
||
| app._write_stateful_pages_marker() | ||
|
|
||
| assert json.loads((tmp_path / constants.Dirs.STATEFUL_PAGES).read_text()) == [] | ||
|
|
||
|
|
||
| def test_write_stateful_pages_marker_concurrent_readers_see_valid_json( | ||
| tmp_path: Path, mocker: MockerFixture | ||
| ): | ||
| """Concurrent writers and readers of the marker never observe a partial file.""" | ||
| mocker.patch("reflex.utils.prerequisites.get_backend_dir", return_value=tmp_path) | ||
| marker = tmp_path / constants.Dirs.STATEFUL_PAGES | ||
| routes = [f"route-{i}" for i in range(4000)] | ||
| app = App(_state=rx.State) | ||
| app._stateful_pages = dict.fromkeys(routes) | ||
| stop = threading.Event() | ||
| errors: list[BaseException] = [] | ||
|
|
||
| def writer(): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: In Prompt for AI agents |
||
| for _ in range(50): | ||
| app._write_stateful_pages_marker() | ||
|
|
||
| def reader(): | ||
| while not stop.is_set(): | ||
| try: | ||
| content = marker.read_text() | ||
| except FileNotFoundError: | ||
| continue | ||
| try: | ||
| assert json.loads(content) == routes | ||
| except (AssertionError, json.JSONDecodeError) as exc: | ||
| errors.append(exc) | ||
| return | ||
|
|
||
| writers = [threading.Thread(target=writer) for _ in range(4)] | ||
| readers = [threading.Thread(target=reader) for _ in range(4)] | ||
| for thread in readers + writers: | ||
| thread.start() | ||
| for thread in writers: | ||
| thread.join() | ||
| stop.set() | ||
| for thread in readers: | ||
| thread.join() | ||
|
|
||
| assert errors == [] | ||
| assert json.loads(marker.read_text()) == routes | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: When frontend and backend run under different Unix accounts, this replacement leaves the marker readable only by the compiling account. Set the temporary file's mode to the marker's intended shared-readable mode before
replace().Prompt for AI agents