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 deeper WorkQueue lifecycle bugs: some failures could abandon a future, kill a worker without replacing its concurrency slot, or leave sibling spinner tasks running after SpinGroup#wait escaped.

This PR restores routing state with narrowly scoped ensure blocks, leaves exception-reporting policy to the owner of caller-provided threads, makes worker failures observable through futures, replaces failed workers while queued work remains, and tears down spinner siblings before propagating exceptional failures.

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. A worker failure could abandon futures and queued work. Workers rescued StandardError and Interrupt, but exceptions such as NotImplementedError are outside StandardError. Such an exception killed the worker without settling its future. A worker killed asynchronously could do the same, and dead workers remained in @workers, permanently occupying their concurrency slot. With max_concurrent: 1, work already queued behind a dead worker could therefore remain unrun forever.

  5. A propagated task failure left sibling work unattended. Once a non-StandardError reached SpinGroup#wait, it escaped mid-render without stopping the group or interrupting its queue, allowing sibling tasks to continue after nobody was watching the group.

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 special Interrupt handling and carries other Exception subclasses back through their futures instead of letting them terminate a worker unnoticed.
  • The dequeue-to-future handoff is protected from asynchronous interruption. An active future is failed with WorkQueue::WorkerDied if its worker exits before settling it.
  • Workers unregister on exit and start a replacement whenever queued work remains below the concurrency limit. wait follows replacement workers, including replacements started after the queue has closed for draining.
  • Deliberate WorkQueue#interrupt teardown is serialized, safely drains queued futures, suppresses diagnostics only on WorkQueue-owned threads being terminated, and does not let a worker's Interrupt replace the caller's in-flight exception.
  • SpinGroup#wait stops the group and its siblings before re-raising a non-StandardError. The exception still propagates to the caller.
  • No new general-purpose public routing or thread-local helper is introduced.

Normal, non-nested routing remains unchanged. Worker failures now settle their active future, and queued work either runs on a replacement worker or is failed during deliberate teardown.

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 killed worker fails its active future and already-queued work runs on a replacement.
  • A task-raised Interrupt replaces the dead worker and does not strand already-queued work.
  • A spinner task raising NotImplementedError neither hangs nor emits a worker thread-death report.
  • A propagated non-StandardError stops the group and interrupts sibling tasks before escaping.

🤖 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
@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
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 routing value in a narrowly scoped ensure after prerequisites have succeeded. Capture#run also stops mutating report_on_exception: that flag belongs to the owner of a caller-provided thread, not cli-ui.

WorkQueue now carries non-StandardError task failures through their futures, protects the dequeue-to-future handoff from asynchronous interruption, and settles an active future if its worker is killed. Workers unregister on exit and replace themselves while queued work remains, so a dead worker cannot strand already-enqueued tasks or retain a concurrency slot. wait follows replacement workers, while interrupt owns deliberate teardown without leaking worker diagnostics or replacing the caller's exception.

SpinGroup stops its queue and sibling tasks before a non-StandardError failure escapes the render loop. Add regressions for nested routing state, alternate-screen entry, exception-reporting ownership, non-StandardError tasks, killed workers, task-raised interrupts, replacement workers, and sibling teardown.

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 7d6cf36 to b8859db Compare August 17, 2026 14:07
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