Skip to content

fix: bound JSON output shutdown after interrupt - #7578

Open
harsh21234i wants to merge 11 commits into
reflex-dev:mainfrom
harsh21234i:fix/7510-bounded-json-drain
Open

harsh21234i wants to merge 11 commits into
reflex-dev:mainfrom
harsh21234i:fix/7510-bounded-json-drain

Conversation

@harsh21234i

@harsh21234i harsh21234i commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Bound shutdown after the first interrupt, including while output is being drained.
  • Isolate the POSIX child process group so Ctrl-C is forwarded exactly once.
  • Keep shutdown handlers active through drain and ignore repeated signals.
  • Bound oversized JSON records to the pipe's atomic-write limit so every emitted line remains parseable.
  • Add regression coverage for repeated Ctrl-C/SIGTERM, oversized records, drain cleanup, and forced test-process cleanup.
  • Add the Reflex Base bugfix news fragment.

Tests

  • tests/units/reflex_base/utils/test_log.py: 81 passed, 5 skipped on Windows (POSIX-only cases).
  • Linux Docker smoke tests: group SIGINT forwarded once; oversized output stayed valid JSON; blocked drain remained bounded after a second SIGTERM.
  • Ruff formatting, focused lint, and git diff --check passed.

Fixes #7510

@harsh21234i
harsh21234i requested a review from a team as a code owner October 15, 2026 13:57
@greptile-apps

greptile-apps Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5

[Medium impact] The changes since the previous review appear safe to merge.

Summary

This PR adds a shutdown deadline for JSON output and shortens oversized records written during shutdown.

  • Child output shutdown keeps one deadline active through the drain.
  • Pipe output keeps complete records and shortens oversized shutdown lines.

Reviews (9) · Last reviewed commit: "fix(logging): preserve records in interr..." · Reviewed by Greptile

Comment thread packages/reflex-base/src/reflex_base/utils/log.py Outdated
Comment thread packages/reflex-base/src/reflex_base/utils/log.py Outdated
Comment thread packages/reflex-base/src/reflex_base/utils/log.py
Comment thread tests/units/reflex_base/utils/test_log.py Outdated
@codspeed

codspeed Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

Merging this PR will not alter performance

1 warning

⚠️ 20 benchmarks are dominated by system calls

System calls cannot be simulated consistently and are left out of the measure. The reported value understates the real cost. Switch to the Walltime instrument to measure them.

Learn more about system calls

See affected benchmarks

✅ 148 untouched benchmarks


Comparing harsh21234i:fix/7510-bounded-json-drain (f3f4a20) with main (a9b6613)

Open in CodSpeed

@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

Reply with feedback, questions, or to request a fix.

View guided diff | Re-trigger cubic

Comment thread tests/units/reflex_base/utils/test_log.py Outdated
Comment thread packages/reflex-base/news/7510.bugfix.md Outdated
Comment thread packages/reflex-base/src/reflex_base/utils/log.py Outdated
Comment thread packages/reflex-base/src/reflex_base/utils/log.py
Comment thread packages/reflex-base/src/reflex_base/utils/log.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

Reply with feedback, questions, or to request a fix.

View guided diff | Re-trigger cubic

Comment thread packages/reflex-base/src/reflex_base/utils/log.py Outdated
Comment thread packages/reflex-base/src/reflex_base/utils/log.py Outdated
Comment thread tests/units/reflex_base/utils/test_log.py

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

Reply with feedback, questions, or to request a fix.

View guided diff | Re-trigger cubic

Comment thread tests/units/reflex_base/utils/test_log.py
Comment thread packages/reflex-base/src/reflex_base/utils/log.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 2 files (changes from recent commits).

Reply with feedback, questions, or to request a fix.

View guided diff | Re-trigger cubic

Comment thread packages/reflex-base/src/reflex_base/utils/log.py Outdated
Comment thread packages/reflex-base/src/reflex_base/utils/log.py Outdated
Comment thread packages/reflex-base/src/reflex_base/utils/log.py Outdated
@masenf masenf added the bug Something isn't working label Oct 19, 2026 — with ChatGPT Codex Connector
@FarhanAliRaza FarhanAliRaza added the on deck PRs lined up to review / merge next label Oct 22, 2026
@masenf

masenf commented Oct 22, 2026

Copy link
Copy Markdown
Collaborator

Main now includes #7558, which deliberately keeps the child in the supervisor's process group so terminal Ctrl-Z and a process-group SIGKILL still reach the app (review rationale). This PR's start_new_session=True / os.killpg(proc.pid, ...) shutdown path conflicts with that decision in log.py.

A mechanical conflict resolution would either reintroduce the orphaned-child behavior or discard this PR's bounded shutdown behavior. The bounded drain needs to be reconciled with the shared-process-group behavior before a safe main merge. I have left the branch intact for that author/reviewer decision.

Comment thread packages/reflex-base/src/reflex_base/utils/log.py Outdated
Comment thread packages/reflex-base/src/reflex_base/utils/log.py Outdated
Comment thread packages/reflex-base/src/reflex_base/utils/log.py Outdated
Comment thread packages/reflex-base/src/reflex_base/utils/log.py
Comment thread packages/reflex-base/src/reflex_base/utils/log.py Outdated
Comment thread tests/units/reflex_base/utils/test_log.py Outdated
Comment thread packages/reflex-base/news/7510.bugfix.md Outdated
@harsh21234i
harsh21234i force-pushed the fix/7510-bounded-json-drain branch from 4df572a to f44613c Compare October 23, 2026 06:13

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working on deck PRs lined up to review / merge next

Projects

None yet

Development

Successfully merging this pull request may close these issues.

reflex run --json: the 30 s drain cap can end the stream mid-record, and a consumer that stops reading blocks shutdown forever

3 participants