Settle futures on any exception; validate WorkQueue/Progress input - #628
Draft
GoodForOneFare wants to merge 1 commit into
Draft
Settle futures on any exception; validate WorkQueue/Progress input#628GoodForOneFare wants to merge 1 commit into
GoodForOneFare wants to merge 1 commit into
Conversation
GoodForOneFare
force-pushed
the
gordo-work-queue-progress-guards
branch
from
August 14, 2026 19:38
4bf35b7 to
837a51b
Compare
Settle futures exactly once while deferring asynchronous interrupts during state changes. Track active workers separately from joinable thread history, queue work before checking capacity, and replace workers that die while work remains. Let fatal task exceptions terminate their worker after failing the future, clamp non-positive WorkQueue concurrency to one, and preserve SpinGroup’s documented zero-as-default behavior. SystemExit now interrupts only queues owned by the SpinGroup. Remove the unrelated Progress validation changes and add regressions for Thread.exit recovery, worker-initiated interruption, shared queues, and observable SystemExit cleanup. Co-authored-by: River <river@shopify.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Assisted-By: devx/67971778-53ac-43ed-9659-234fe50f4cd9
GoodForOneFare
force-pushed
the
gordo-work-queue-progress-guards
branch
from
August 14, 2026 20:54
837a51b to
1b9a760
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The main bug: non-StandardError exceptions hang SpinGroup forever
WorkQueue's worker rescued onlyStandardErrorandInterrupt. Any other exception escaped the worker thread without completing its future:Future#valueblocks indefinitely.max_concurrent, so queued work never gets a replacement worker.SpinGroup#waitloops until every task's future completes — so it spins forever.This sounds exotic until you look at what lives outside
StandardError:NotImplementedErrorandLoadErrorare bothScriptErrorsubclasses. A failedrequireinside any spinner task froze the whole CLI with an eternal spinner (verified againstmain).WorkQueue fixes
Exceptionand fail the future, so waiters always observe the error; the worker stays alive for subsequent tasks.Interrupthandling is unchanged (fail + re-raise to terminate the worker).ensure.Thread#raiseandThread#killcan land outside the worker's rescues (e.g.WorkQueue#interrupthitting a worker betweenQueue#popand thebegin). A future abandoned that way is failed withWorkQueue::AbandonedTaskErrorinstead of blockingFuture#valueforever.interrupttolerates theInterruptthatThread#joincan re-raise. TheInterruptit raises into a worker can land after that worker has already left its rescues (it was terminating after the queue closed); the worker dies with an unhandledInterrupt, andjoinwould re-raise it in the interrupting thread — replacing whatever that thread was propagating (e.g. theSystemExitthat triggered the interrupt).WorkQueue.newrejectsmax_concurrent < 1, which produced a queue that accepts work, never starts it, and returns from#waitimmediately while futures stay incomplete — a deadlock for anyFuture#valuecaller.SpinGroup fixes
Task#checktreats any exception other thanInterrupt/SystemExitas a task failure, reported through the normal debrief. (An earlier revision raised non-StandardErrors out ofSpinGroup#wait; review pointed out that skipswork_queue.interrupt, the final render, and the debrief, leaving sibling workers running afterwaitreturns.)waitinterrupts the queue before lettingSystemExitpropagate, so a strayexitin a task doesn't orphan the remaining workers.initializefalls back to 1024 for any non-positivemax_concurrentinstead of raising fromWorkQueue's new guard, consistent with the documented0= unlimited.Progress fixes
tickvalidatespercent/set_percentexplicitly — finite numbers only, with the offending argument named in theArgumentError— instead of relying onArray#minraising on an incomparable value. Validation happens before assignment, so a raisingtickleaves the bar renderable from its last valid state (previously the next render raised a confusing secondary error far from the actual mistake).0.0..1.0. The lower bound was previously unguarded, so a negative percent rendered a negative suffix.Tests
ScriptError: future completes,valuere-raises, a subsequent task on the same worker still runs.Thread.exit) still settles its future, withAbandonedTaskError.SpinGroup: aNotImplementedErrortask is reported throughfailure_debriefwith its sibling still completing;waitreturnsfalsepromptly instead of hanging (with aTimeoutguard so a regression fails the suite rather than wedging CI).SpinGroup:SystemExitpropagates out ofwaitwith its exit status intact, and remaining workers are interrupted rather than orphaned.SpinGroupruns tasks formax_concurrent: 0and-1.ArgumentErrorforWorkQueue.new(0)/(-1).tickvalues (String, NaN, ±Infinity, Array) raise without corrupting the bar's last valid state; error messages name the bad argument; negative values clamp to 0%.🤖 Generated with Claude Code
(posted by an LLM bot on behalf of Gord)