Skip to content

Restore shadowed routing state; settle worker failures - #627

Draft
GoodForOneFare wants to merge 1 commit into
mainfrom
gordo-stdout-router-thread-local-restore
Draft

Restore shadowed routing state; settle worker failures#627
GoodForOneFare wants to merge 1 commit into
mainfrom
gordo-stdout-router-thread-local-restore

Conversation

@GoodForOneFare

@GoodForOneFare GoodForOneFare commented Aug 14, 2026

Copy link
Copy Markdown
Member

What

Capture#run and StdoutRouter.with_id hard-reset Thread.current routing state on exit instead of restoring the values they shadowed. Capture#run also changed Thread#report_on_exception to hide spinner worker failures. Removing that suppression exposed a separate WorkQueue bug: a non-StandardError task exception could kill its worker without settling the future, leaving SpinGroup#wait running forever.

This PR restores routing state with narrowly scoped ensure blocks, leaves exception-reporting policy to the thread owner, and makes WorkQueue settle futures for non-StandardError task exceptions.

The bugs

  1. Capture#run permanently disabled report_on_exception. It set Thread.current.report_on_exception = false with no restore. A caller-owned or reusable worker thread that ran a capture therefore stopped producing Ruby's diagnostic report for later unhandled thread exceptions.

  2. A nested capture lost the outer capture and could crash a prompt. The ensure reset :cliui_current_capture to nil rather than the previous value. After an inner capture completed, Capture.in_alternate_screen could reach current_capture!.stdout while current_capture was nil, raising NoMethodError. Every interactive prompt uses this alternate-screen path.

  3. A nested with_id lost the outer ID. Its ensure reset :cliui_output_id to nil, so output in the remainder of the outer scope silently lost its [id] label.

  4. WorkQueue could abandon a future. Workers rescued StandardError and Interrupt, but exceptions such as NotImplementedError are outside StandardError. Such an exception killed the worker without completing or failing its future. With the old report_on_exception mutation this was a silent hang; without the mutation it was a hang accompanied by a thread-death backtrace over spinner output.

The fix

  • Capture#run saves and restores :cliui_current_capture, :no_cliui_frame_inset, and :cliui_output_hook in local, narrowly scoped ensure blocks. The saves occur only after prerequisites such as assert_enabled! have succeeded, avoiding the method-level-ensure clobbering window.
  • with_id similarly restores the previous :cliui_output_id.
  • Capture#run no longer reads or writes report_on_exception; caller-owned captures retain the thread owner's setting.
  • WorkQueue preserves its special Interrupt handling, then catches other Exception subclasses, fails the corresponding future, and keeps the worker available for subsequent work. The future carries the exception back to the caller instead of remaining incomplete.
  • No new general-purpose public routing or thread-local helper is introduced.

Normal, non-nested routing remains unchanged. Non-StandardError task failures now reach callers through their future instead of killing a worker and deadlocking the spinner.

Tests

  • Nested with_id restores the outer ID on both the normal and raising paths.
  • Nested captures restore the outer capture and output hook, including when the inner capture raises.
  • A nested capture can subsequently enter the alternate screen without crashing.
  • Frame inset state restores correctly under nesting.
  • A capture leaves report_on_exception untouched in both the true and false states.
  • A caller-owned thread that dies inside a capture still produces Ruby's configured thread-death report.
  • A NotImplementedError fails its WorkQueue future and the worker continues with later work.
  • A spinner task raising NotImplementedError neither hangs nor emits a worker thread-death report.

🤖 Updated by an LLM coding agent on behalf of Gord.

@GoodForOneFare
GoodForOneFare force-pushed the gordo-stdout-router-thread-local-restore branch from efc5d78 to 7cd4cd5 Compare August 14, 2026 19:13
@GoodForOneFare GoodForOneFare changed the title Save and restore thread-locals in Capture#run and with_id Restore shadowed thread-locals; stop mutating report_on_exception Aug 14, 2026
Capture#run and with_id hard-reset Thread.current routing state on exit instead of restoring the values they shadowed. A nested capture therefore left its still-active outer capture invisible, which could crash in_alternate_screen at current_capture!.stdout, while a nested output ID stripped the outer scope's labelling.

Save and restore each value in a narrowly scoped ensure after prerequisites have succeeded. Capture#run also stops mutating report_on_exception: that flag belongs to the thread owner, not cli-ui.

WorkQueue now settles futures for non-StandardError task exceptions rather than letting them kill the worker. This prevents thread-death diagnostics from leaking over spinner output and prevents SpinGroup#wait from hanging forever on an incomplete future.

Add regressions for nested IDs and captures, alternate-screen entry after a nested capture, exception-reporting ownership, and NotImplementedError handling in WorkQueue and SpinGroup.

Co-authored-by: River <river@shopify.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Assisted-By: devx/24e11ae9-0556-4186-a8fe-69bb6bc342ff
@GoodForOneFare
GoodForOneFare force-pushed the gordo-stdout-router-thread-local-restore branch from 7cd4cd5 to 7d6cf36 Compare August 14, 2026 20:46
@GoodForOneFare GoodForOneFare changed the title Restore shadowed thread-locals; stop mutating report_on_exception Restore shadowed routing state; settle worker failures Aug 14, 2026
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.

1 participant