Repository navigation
Give reflex run one owner for shutdown - #7650
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".
 [Medium impact] The changes since the last review appear safe to merge. Summary
Reviews (11) · Last reviewed commit: "Give the kill_descendants tests a root o..." · 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.
All reported issues were addressed across 12 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
Merging this PR will not alter performance1 warning
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 |
The grace after the drain deadline waited for the reader thread to exit, so a descendant that still held the pipe open cost the full two seconds even after the write finished. It now waits for the write in progress. exec.kill lost its only caller when the frontend tree stop replaced the Windows workaround; remove it. Parse /proc/<pid>/stat in one helper.
reflex imports reflex_base.utils.processes, which is unreleased. The floor was 0.10.1.dev0, which the published reflex-base 0.10.1 satisfies without that module, so check-min-deps resolved to it and failed. Claude-Session: https://claude.ai/code/session_012Ps5W65XWRr7LRi3c4DvQU
# Conflicts: # pyproject.toml
The JSON supervisor shares no code with the logging pipeline, so it now lives in reflex_base.utils.supervisor with its tests. The process tree helper is renamed to reflex_base.utils.process_tree so it no longer shares a name with reflex.utils.processes. No logic changes.
There was a problem hiding this comment.
All reported issues were addressed across 10 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
View guided diff | Turn on auto-fix | Re-trigger cubic
A repeat within a second of the first SIGINT or SIGTERM is still the same request delivered twice and is ignored. A later one means the cleanup is stuck, for example on a frontend pipe that a process outside the tree holds open: every descendant is killed and reflex run exits at once.
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
A forced exit found descendants by a fresh scan below reflex run, so a server whose wrapper had already exited during the cleanup, the npm case this branch fixes, survived it and kept its port. The cleanup now records everything below the process when it begins, and the forced exit kills that set as well as whatever a fresh scan finds. The forced exit also wrote its message before killing anything, so a stderr pipe nobody read could hold it up. The message is written only when stderr accepts it without blocking, after the kill. Claude-Session: https://claude.ai/code/session_012Ps5W65XWRr7LRi3c4DvQU
The scripted children are read line by line with a deadline from an unbuffered pipe, so a child that never prints fails the test instead of hanging the suite. The test that keeps a killed grandchild as a zombie under its living parent runs on Linux only, where the zombie is seen as gone. The docstrings say that on Windows a forced exit ends only the CLI, since a console Ctrl-C already reached every process on the console. Claude-Session: https://claude.ai/code/session_012Ps5W65XWRr7LRi3c4DvQU
granian installs its own SIGINT/SIGTERM handlers while it serves and never restores ours, so after the backend returned a later Ctrl-C only set granian's flag and the forced exit never ran in a real app. begin_cleanup now puts the handlers back.
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
The forced exit kills the process IDs recorded when the cleanup began. Between then and a later repeat of the signal an ID can be freed and taken by an unrelated process. A recorded ID is now killed only while it is still in the root's process group, which every process reflex run starts stays in; one that left the group belongs to someone else. 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
The two tests for recorded IDs used the test runner as the root, so the kill reached every live descendant of pytest. They now kill below a child started for the purpose. Claude-Session: https://claude.ai/code/session_012Ps5W65XWRr7LRi3c4DvQU
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.process_tree.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 SIGINT/SIGTERM handlers around the run. The first signal unwinds into cleanup. A repeat within one second, such as the second delivery a terminal produces next to the one the JSON supervisor forwards, is ignored so it cannot interrupt that cleanup. A later repeat forces the exit: every descendant gets SIGKILL and the CLI exits at once, so a cleanup stuck on a pipe that a process outside the tree holds open never needskill -9. 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) moves fromreflex_base.utils.logto its own module,reflex_base.utils.supervisor, with its tests; it shares no code with the logging pipeline. It 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. Readers write in chunks of at mostPIPE_BUFbytes, which a pipe accepts whole or not at all, and a write still in progress when the wall is hit gets a short grace. A consumer that still reads therefore never receives a cut record, unless one record alone is larger thanPIPE_BUF. 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