Skip to content

Clear fatal-error interrupt window unconditionally on body exit - #6994

Merged
masenf merged 3 commits into
mainfrom
claude/rel-fix-interrupt-window-followup
Aug 28, 2026
Merged

Clear fatal-error interrupt window unconditionally on body exit#6994
masenf merged 3 commits into
mainfrom
claude/rel-fix-interrupt-window-followup

Conversation

@masenf

@masenf masenf commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Changes To Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully ran tests with your changes locally?

Description

Follow-up to #6990 (merged while this review round was in flight), addressing the two review threads that were still open there:

  1. cubic's finding is valid: run_concurrently_context cleared in_body only on the normal post-body path and in the KeyboardInterrupt handler. If the with-body raises any other exception, in_body stays True — and since the executor shuts down with wait=False, a task failing after the context has unwound could still deliver a stray SIGINT into unrelated caller code. The fix wraps the yield in a try/finally that clears in_body however the body exits, replacing the two scattered clears — a net simplification of the mechanism, not an addition (per the review discussion, no extra guarded setup path).
  2. greptile's nit: the hang regression test's bare 5-second assertion threshold now carries a comment explaining its relationship to the body's 10s wait.

Adds test_run_concurrently_context_no_interrupt_after_body_exception, which pins the escaped-interrupt case (late-failing task + body exception → the body's own ValueError propagates, no stray KeyboardInterrupt).

Test Plan

  • uv run pytest tests/units/utils/test_processes.py -k run_concurrently: 4 passed (the new test plus the three from Fix reflex run hanging after fatal error in the frontend thread #6990).
  • uv run ruff check / ruff format --check clean on changed files; uv run pyright 0 errors on changed files.
  • reflex-release changelog-check --base-ref origin/main passes.
  • The two test_is_process_on_port_* failures in this sandbox are pre-existing on clean main here (container network stack), unrelated to the diff.

🤖 Generated with Claude Code

https://claude.ai/code/session_01EMjBXPozsNeQNSBZecNH8x


Generated by Claude Code

Review in cubic

Review follow-ups: clear in_body in a finally around the yield so a
task failing after the with-body raised its own exception can no longer
interrupt unrelated caller code (the executor shutdown does not wait),
replacing the two scattered clears; document the 5-second assertion
threshold in the hang regression test; add a regression test for the
late-failure case.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EMjBXPozsNeQNSBZecNH8x
@masenf
masenf requested a review from a team as a code owner August 28, 2026 19:46

masenf commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator Author

Note: this was opened moments before your replies landed on the #6990 threads ("if we're going down, we're going down" / "test code"), so calling out the delta explicitly: executor.shutdown(wait=False) means a worker can still be running after the context unwinds, so when the body exits via a non-KeyboardInterrupt exception (where in_body stayed True), a late task failure could deliver SIGINT into caller code beyond the context — the new test pins that concrete case, and the diff is a net simplification (one finally replaces the two scattered clears). If you still consider the edge not worth carrying, feel free to close this — no strong attachment.


Generated by Claude Code

@greptile-apps

greptile-apps Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR clears the fatal-error interrupt window in a finally block whenever context entry or the with-body exits.

  • Consolidates in_body cleanup across normal, exceptional, and pre-body failure paths.
  • Adds regression coverage for late worker failures after body and pre-body exceptions.
  • Documents the timing thresholds used by the existing interruption test.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
reflex/utils/processes.py Moves interrupt-window cleanup into an unconditional nested finally, covering all body-exit paths.
tests/units/utils/test_processes.py Adds coverage for late worker failures and documents the existing timeout relationship.
news/+interrupt-window-followup.bugfix.md Records the corrected late-interrupt behavior in the changelog.

Reviews (3): Last reviewed commit: "Clear the interrupt window when the pre-..." | Re-trigger Greptile

Comment thread tests/units/utils/test_processes.py Outdated
Comment thread tests/units/utils/test_processes.py Outdated
@codspeed-hq

codspeed-hq Bot commented Aug 28, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 32 untouched benchmarks
⏩ 8 skipped benchmarks1


Comparing claude/rel-fix-interrupt-window-followup (276300f) with main (6c6a398)

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.

The 0.3s/0.6s sleeps made ordering scheduler-dependent: under load the
task could fail before the body raised, or outlive the settle window.
Events now enforce the ordering (task fails only after the context has
unwound; a done callback registered after the context's own confirms
the interrupt callback ran), leaving only a 0.1s signal-delivery
window, documented in place.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EMjBXPozsNeQNSBZecNH8x

@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 reflex/utils/processes.py
Review follow-up: raise_first_failure ran outside the try/finally, so a
task failing before the body was entered left in_body armed and a
surviving task could interrupt caller code after the context exited.
The finally now wraps the pre-check as well. New regression test fails
10/10 against the previous structure and passes 10/10 with the fix.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EMjBXPozsNeQNSBZecNH8x
@masenf masenf added this to the v0.9.9 milestone Aug 28, 2026
@masenf
masenf merged commit 9739210 into main Aug 28, 2026
111 checks passed
@masenf
masenf deleted the claude/rel-fix-interrupt-window-followup branch August 28, 2026 20:19
masenf pushed a commit that referenced this pull request Aug 28, 2026
…-context-refactor-jv3pig

Picks up #6994, #6986 and #6991. Clean auto-merge; nothing touches client
state, memoization, or foreach.
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.

2 participants