Repository navigation
Give reflex run one owner for shutdown - #7650
FarhanAliRaza wants to merge 5 commits into
Conversation
`reflex run` had no single place that stops its process tree. Each layer reacted to whatever signal reached it and relied on the terminal's process group, Python's KeyboardInterrupt, or exit-code conventions to reach the others. Every delivery route that differed from a terminal Ctrl-C became its own bug: a pid-targeted SIGTERM left `npm run`'s node server behind and hung the CLI on its pipe (#7485), the JSON supervisor's drain cap started only after the child exited so a stalled consumer blocked exit forever and a slow one got a cut record (#7510), and a second Ctrl-C could interrupt the cleanup the first one started. The shutdown now converges on one path at each layer: - `reflex_base.utils.processes.stop_process_tree` signals a process and every descendant it can find (/proc on Linux, ps elsewhere, taskkill /T on Windows), waits for all of them against one deadline and kills what is left. Processes stay in the CLI's process group, so a terminal Ctrl-C, Ctrl-Z or a group kill still reach them directly. - `_run_dev` installs idempotent SIGINT/SIGTERM handlers around the run: the first signal unwinds into cleanup, repeats are ignored. Its cleanup stops the whole frontend tree with that helper, on every platform, and `stream_logs` no longer reports an exit the CLI asked for as a failure. - `supervise_output` forwards every SIGINT/SIGTERM to the command, arms a 15 s deadline from the first one after which the command tree is killed, anchors the 30 s drain wall at the request, and writes record by record while stopping so the drain ends between two complete records. Regression coverage: unit tests for the tree helper, the no-TTY run with a frontend that does not forward signals, the supervisor with a stalled consumer during chatty cleanup, a repeated signal, a command that ignores shutdown, and a slow consumer at the drain cut; plus an integration matrix that stops a real `reflex run` by group SIGINT, pid SIGINT and pid SIGTERM in plain, --json and npm configurations and checks exit time, survivors and error output. Closes #7485 Closes #7510 Claude-Session: https://claude.ai/code/session_012Ps5W65XWRr7LRi3c4DvQU
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d5c45d8eba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return | ||
| # One write per batch while the command runs; one per record once it | ||
| # is stopping, so the drain can end between two complete records. | ||
| records = data.splitlines(keepends=True) if self.stopping.is_set() else (data,) |
There was a problem hiding this comment.
If shutdown begins while a pre-shutdown batch is blocked in _write_all, this one-time check leaves that entire multi-record batch unsplit. When the wall deadline and two-second grace expire, the daemon thread can still be partway through the batch when the supervisor exits, so a slow or paused JSON consumer receives a truncated final record—the exact case the new bounded-drain behavior is intended to prevent.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8772fbb (pushing after the local matrix passes). Records are now grouped into writes of at most PIPE_BUF bytes at every stage, not only after a stop request. A pipe accepts such a write whole or not at all, so an abandoned reader thread cannot leave a partial record; the per-record mode and the stopping event go away. Write counts and throughput for a chatty child match main. Only a single record larger than PIPE_BUF can still be cut, which is the limit of not truncating.
Generated by Claude Code
| if process.returncode is None: | ||
| descendants = [*descendants, *descendant_pids(process.pid)] |
There was a problem hiding this comment.
When the root process handles SIGTERM by spawning a cleanup subprocess and then exiting, that subprocess is absent from the initial snapshot and is reparented before _wait returns. Because the second scan only runs while process.returncode is still None, the new subprocess is never signaled and survives stop_process_tree, violating the whole-tree shutdown guarantee; descendants need to be tracked while the root-parent relationship still exists.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changing this. The gap is real but cannot be closed by scanning: a child forked from a SIGTERM handler is reparented the moment the root exits, so no later scan under the root finds it, and a periodic scan during the wait only narrows the window. Closing it needs a process group or a subreaper, and #7558 ruled out moving the app out of the CLI's group. None of the trees reflex run starts (bun or npm in front of node, granian and its workers) forks while handling SIGTERM. The docstring in 8772fbb now states that descendant discovery is best effort for a process that forks during its own shutdown.
Generated by Claude Code
| try: | ||
| os.kill(pid, 0) | ||
| except ProcessLookupError: | ||
| return False | ||
| except PermissionError: | ||
| return True | ||
| return True |
There was a problem hiding this comment.
On POSIX, kill(pid, 0) succeeds for zombies. If a terminated frontend grandchild is reparented to a non-reaping PID 1, as commonly happens in containers, _wait therefore consumes the full five-second graceful timeout and the additional one-second wait even though nothing remains runnable. Inspecting process state and treating zombies as gone avoids adding this delay to otherwise clean reflex run shutdowns.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8772fbb (pushing after the local matrix passes). _alive first tries waitpid(pid, WNOHANG), which reaps an orphan that was handed to this process when it is PID 1 or a subreaper, and on Linux reads the process state so a zombie counts as gone. Covered by test_alive_treats_an_unreaped_child_as_gone.
Generated by Claude Code
 [Medium impact] The PR appears safe to merge; the remaining import-rule finding is fixed. SummaryThis PR gives
Reviews (4) · Last reviewed commit: "Keep the new test imports at module leve..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 12 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/reflex-base/src/reflex_base/utils/processes.py">
<violation number="1" location="packages/reflex-base/src/reflex_base/utils/processes.py:151">
P1: `stop_process_tree` abandons descendants as soon as the root is reaped, so a SIGTERM-responsive wrapper can leave its still-running frontend orphaned. Capture the tree before forwarding the first signal, or retain an equivalent process-group/tree snapshot for the later forced cleanup.</violation>
<violation number="2" location="packages/reflex-base/src/reflex_base/utils/processes.py:156">
P2: The docstring promises that descendants started during the wait are caught before the kill, but the re-scan runs only while the root is still alive. When the root exits during the wait, children it already spawned can start new processes that are never signalled. Either narrow the docstring or snapshot the tree again before the root exits.</violation>
</file>
<file name="tests/units/reflex_base/utils/test_processes.py">
<violation number="1" location="tests/units/reflex_base/utils/test_processes.py:105">
P3: This `[:1] == [proc.pid]` check depends on the arbitrary order of direct children and on no other child of pytest existing. Assert membership instead: `assert proc.pid in processes.descendant_pids(os.getpid())`.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| with contextlib.suppress(subprocess.TimeoutExpired): | ||
| process.wait(timeout=timeout) | ||
| return | ||
| if process.poll() is not None: |
There was a problem hiding this comment.
P1: stop_process_tree abandons descendants as soon as the root is reaped, so a SIGTERM-responsive wrapper can leave its still-running frontend orphaned. Capture the tree before forwarding the first signal, or retain an equivalent process-group/tree snapshot for the later forced cleanup.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/reflex-base/src/reflex_base/utils/processes.py, line 151:
<comment>`stop_process_tree` abandons descendants as soon as the root is reaped, so a SIGTERM-responsive wrapper can leave its still-running frontend orphaned. Capture the tree before forwarding the first signal, or retain an equivalent process-group/tree snapshot for the later forced cleanup.</comment>
<file context>
@@ -0,0 +1,163 @@
+ with contextlib.suppress(subprocess.TimeoutExpired):
+ process.wait(timeout=timeout)
+ return
+ if process.poll() is not None:
+ return
+ descendants = descendant_pids(process.pid)
</file context>
There was a problem hiding this comment.
This does not match the code. The tree is captured before the first signal (descendants = descendant_pids(process.pid) runs before _signal), and _wait returns early only when the root has exited and no captured descendant is alive; otherwise it runs to the deadline and the survivors get SIGKILL. A wrapper that exits on SIGTERM and leaves its server running is exactly test_stop_process_tree_stops_an_orphaned_grandchild and the orphaning cases of test_no_tty_run_stops_frontend, which pass.
Generated by Claude Code
| descendants = descendant_pids(process.pid) | ||
| _signal([process.pid, *descendants], signal.SIGTERM) | ||
| _wait(process, descendants, time.monotonic() + timeout) | ||
| if process.returncode is None: |
There was a problem hiding this comment.
P2: The docstring promises that descendants started during the wait are caught before the kill, but the re-scan runs only while the root is still alive. When the root exits during the wait, children it already spawned can start new processes that are never signalled. Either narrow the docstring or snapshot the tree again before the root exits.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/reflex-base/src/reflex_base/utils/processes.py, line 156:
<comment>The docstring promises that descendants started during the wait are caught before the kill, but the re-scan runs only while the root is still alive. When the root exits during the wait, children it already spawned can start new processes that are never signalled. Either narrow the docstring or snapshot the tree again before the root exits.</comment>
<file context>
@@ -0,0 +1,163 @@
+ descendants = descendant_pids(process.pid)
+ _signal([process.pid, *descendants], signal.SIGTERM)
+ _wait(process, descendants, time.monotonic() + timeout)
+ if process.returncode is None:
+ descendants = [*descendants, *descendant_pids(process.pid)]
+ _signal([process.pid], signal.SIGKILL)
</file context>
There was a problem hiding this comment.
Fixed in 8772fbb. The docstring now says the second look only helps while the root still runs, and that a process forking during its own shutdown is the documented limit of descendant discovery without a process group.
Generated by Claude Code
| if sys.platform == "win32": | ||
| pytest.skip("taskkill walks the tree on Windows") | ||
| assert processes.descendant_pids(proc.pid) == [grandchild] | ||
| assert processes.descendant_pids(os.getpid())[:1] == [proc.pid] |
There was a problem hiding this comment.
P3: This [:1] == [proc.pid] check depends on the arbitrary order of direct children and on no other child of pytest existing. Assert membership instead: assert proc.pid in processes.descendant_pids(os.getpid()).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At tests/units/reflex_base/utils/test_processes.py, line 105:
<comment>This `[:1] == [proc.pid]` check depends on the arbitrary order of direct children and on no other child of pytest existing. Assert membership instead: `assert proc.pid in processes.descendant_pids(os.getpid())`.</comment>
<file context>
@@ -0,0 +1,153 @@
+ if sys.platform == "win32":
+ pytest.skip("taskkill walks the tree on Windows")
+ assert processes.descendant_pids(proc.pid) == [grandchild]
+ assert processes.descendant_pids(os.getpid())[:1] == [proc.pid]
+ assert grandchild in processes.descendant_pids(os.getpid())
+ finally:
</file context>
| assert processes.descendant_pids(os.getpid())[:1] == [proc.pid] | |
| assert proc.pid in processes.descendant_pids(os.getpid()) |
There was a problem hiding this comment.
Fixed in 6d3167702. The test checks that the child comes before its grandchild in the listing, without assuming it is the first child of pytest.
Generated by Claude Code
Merging this PR will degrade performance by 4.63%1 warning
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing 
|
Review findings on #7650: - A batch blocked in the reader thread when shutdown started was still one write, so an abandoned write could cut a record. Records are now grouped into writes of at most PIPE_BUF bytes at every stage, which a pipe accepts whole or not at all; the per-record mode after a stop request goes away with it. Write counts and throughput for a chatty child match main. - A descendant that exited but was never reaped, as happens for orphans under a non-reaping PID 1, counted as alive and cost the whole stop timeout. _alive now reaps orphans handed to this process and reads the Linux process state, so a zombie counts as gone. Claude-Session: https://claude.ai/code/session_012Ps5W65XWRr7LRi3c4DvQU
Review findings on #7650 and the Windows unit-test failure: - The JSON supervisor forwarded SIGTERM on Windows as TerminateProcess on the command alone, leaving its workers behind; it now stops the whole tree with taskkill at once, since Windows has no graceful signal. - The run's SIGINT/SIGTERM guard is installed on every platform, and the run marks the start of its cleanup so a first signal arriving during the frontend stop is ignored instead of aborting it. - Test helpers no longer race a process that exits between the liveness probe and the /proc read, the tree test checks ordering by membership, and the Windows cleanup uses SIGTERM, which exists there. Claude-Session: https://claude.ai/code/session_012Ps5W65XWRr7LRi3c4DvQU
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
select.PIPE_BUF exists only on POSIX; reading it on Windows raised in the reader thread as soon as a batch held two records. The size is now a module constant with the Linux value as fallback, the test reads that constant, and the supervisor docstring scopes the record-boundary guarantee to records that fit one write. Claude-Session: https://claude.ai/code/session_012Ps5W65XWRr7LRi3c4DvQU
The slow-consumer drain test no longer shrinks the pipe with fcntl: the default pipe already holds far less than the test writes, so the cut happens regardless, and the test now runs on every POSIX platform. psutil is imported at the top of the tree test module like elsewhere in tests. Claude-Session: https://claude.ai/code/session_012Ps5W65XWRr7LRi3c4DvQU
|
The red Generated by Claude Code |
All Submissions:
Type of change
Changes To Core Features:
Why
reflex runhas no single place that stops its process tree. Each layer reacts to whatever signal happens to reach it and relies on the terminal's process group, Python'sKeyboardInterrupt, or exit-code conventions to reach the others. Every delivery route that differs from a terminal Ctrl-C has become its own issue and its own PR: #6980 / #7328 (no TTY), #7509 / #7558 (pid-targeted SIGINT under--json), #7485 (npm leaves node behind and the CLI hangs on its pipe), #7510 / #7428 / #7578 (the JSON drain cap), #6981 / #7266 / #7457 / #7538 (spurious errors on a clean stop).This PR gives shutdown one owner per layer so that the route a stop takes no longer matters. The shape follows what uvicorn, gunicorn, honcho, supervisord, concurrently and tini do: the handler only records the request, the whole set is waited on against one shared deadline, stragglers get SIGKILL, and the parent remembers which exits it asked for.
What changes
reflex_base.utils.processes.stop_process_tree(new) signals a process and every descendant it can find, waits for all of them against one deadline, then kills whatever is left. Descendants come from/procon Linux,pson other POSIX systems andtaskkill /T /Fon Windows, so no new dependency. Processes stay in the CLI's process group on purpose: a terminal Ctrl-C, Ctrl-Z, supervisordkillasgrouporkill -9 -<pgid>still reach every process directly, which is the decision made in #7558.reflex run(_run_dev) installs idempotent SIGINT/SIGTERM handlers around the run. The first signal unwinds into cleanup; repeats, such as the second delivery a terminal produces next to the one the JSON supervisor forwards, are ignored instead of interrupting that cleanup. The cleanup stops the whole frontend tree with the helper on every platform, which replaces the direct-childterminate()and the Windows-only psutil kill.stream_logstakesexpected_exitso an exit the CLI asked for is never reported asStarting frontend failed with exit code ....reflex run --json(supervise_output) forwards every SIGINT/SIGTERM to the command; athreading.Timerarmed by the first one kills the command tree after 15 s, so a child blocked on a full output pipe cannot keep the supervisor alive. The 30 s drain wall is anchored at the request instead of at the child's exit. While stopping, readers write record by record and a record in flight when the wall is hit gets a short grace, so a consumer that still reads never receives a cut record. Normal operation keeps one write per batch, and records are never truncated.Evidence
Run against main's source on Linux, these cells of the new matrix fail: with
REFLEX_USE_NPM=1a pid-targeted SIGTERM leavesnode react-router devrunning andreflex runnever exits (the frontend thread is blocked on the pipe node holds); a pid SIGINT with bun hung the same way in this environment. With this branch all nine cells pass in about 70 s after the one-time init:Scripted supervisor cases (unit tests, compared on main and this branch): a consumer that stops reading while the child prints during cleanup hangs main past 75 s and exits here after the deadline; a child that needs two interrupts gets both; a child that ignores signals is killed at the deadline; a slow consumer's stream ends at a record boundary where main cut mid-record.
Local validation: the touched unit test files pass (243 tests), the full unit suite passes except
tests/units/reflex_cli/v2and the reflex-bench browser tests, which fail identically on main's source in this environment (hosting-cli version gate and a Playwright browser mismatch). Ruff, ruff format and pyright are clean on the changed files.Not in this PR
reflex runsurvives a closed terminal on main and on this branch alike. Mapping SIGHUP to stop in dev mode is a separate decision.bun run/npm run, which would remove the intermediary altogether. Tree stop covers it without changing how the frontend is launched.Closes #7485
Closes #7510
Supersedes #7578
https://claude.ai/code/session_012Ps5W65XWRr7LRi3c4DvQU
Generated by Claude Code