fix(worker): deliver fiber activity cancellation via scheduler fiber_… - #519
Open
michaeldiscala wants to merge 1 commit into
Open
Conversation
…interrupt Cancel callbacks run on the canceler's fiber, which for activity cancellation is the worker's single task-dispatch fiber. `Fiber#raise` uses resume semantics, so under a scheduler that drives fibers with `Fiber#transfer` (such as `async`) the target fiber never returns to its resumer and the dispatch fiber is suspended forever. The worker then stops dispatching all work -- activity tasks and workflow activations alike -- while the process and the reactor stay healthy. Deliver the exception through `Fiber::Scheduler#fiber_interrupt` (Ruby 3.3+) when the scheduler provides one, so the raise is performed by the scheduler itself and the caller returns immediately. Schedulers without the hook keep the existing `Fiber#raise` behavior. Fixes temporalio#518
chris-olszewski
requested changes
Aug 17, 2026
chris-olszewski
left a comment
Member
There was a problem hiding this comment.
Overall this looks good with a few exceptions.
Could you add a test similar to test_activity_shielding, but it uses
class FiberShieldingActivity < ShieldingActivity
activity_executor :fiber
endIt displays why we need the additional fiber != ::Fiber.current check before using fiber_interrupt
| # worker's single task-dispatch fiber. Fiber#raise uses resume semantics, so under a | ||
| # scheduler that drives fibers with Fiber#transfer (e.g. async) the target never returns | ||
| # to its resumer and the dispatch fiber is suspended forever, wedging the whole worker. | ||
| # Schedulers expose fiber_interrupt (Ruby 3.3+) to perform the raise themselves, which |
Member
There was a problem hiding this comment.
From what I see, this just got added in Ruby 4
| context&.cancellation&.add_cancel_callback do | ||
| fiber.raise(Error::CanceledError.new('Activity canceled')) | ||
| error = Error::CanceledError.new('Activity canceled') | ||
| if scheduler |
Member
There was a problem hiding this comment.
We don't want to use fiber_interrupt if we're on the same fiber already as it bypasses cancellation shielding.
Suggested change
| if scheduler | |
| if scheduler && fiber != ::Fiber.current |
Comment on lines
+42
to
+47
| # Cancel callbacks run on the canceler's fiber, which for activity cancellation is the | ||
| # worker's single task-dispatch fiber. Fiber#raise uses resume semantics, so under a | ||
| # scheduler that drives fibers with Fiber#transfer (e.g. async) the target never returns | ||
| # to its resumer and the dispatch fiber is suspended forever, wedging the whole worker. | ||
| # Schedulers expose fiber_interrupt (Ruby 3.3+) to perform the raise themselves, which | ||
| # returns to the caller immediately. |
Member
There was a problem hiding this comment.
Unsure if all of the words in the comment are useful.
Suggested change
| # Cancel callbacks run on the canceler's fiber, which for activity cancellation is the | |
| # worker's single task-dispatch fiber. Fiber#raise uses resume semantics, so under a | |
| # scheduler that drives fibers with Fiber#transfer (e.g. async) the target never returns | |
| # to its resumer and the dispatch fiber is suspended forever, wedging the whole worker. | |
| # Schedulers expose fiber_interrupt (Ruby 3.3+) to perform the raise themselves, which | |
| # returns to the caller immediately. | |
| # Directly raising from another fiber can strand a `Fiber#transfer`-based | |
| # scheduler's current fiber. |
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.
…interrupt
Cancel callbacks run on the canceler's fiber, which for activity cancellation is the worker's single task-dispatch fiber.
Fiber#raiseuses resume semantics, so under a scheduler that drives fibers withFiber#transfer(such asasync) the target fiber never returns to its resumer and the dispatch fiber is suspended forever. The worker then stops dispatching all work -- activity tasks and workflow activations alike -- while the process and the reactor stay healthy.Deliver the exception through
Fiber::Scheduler#fiber_interrupt(Ruby 3.3+) when the scheduler provides one, so the raise is performed by the scheduler itself and the caller returns immediately. Schedulers without the hook keep the existingFiber#raisebehavior.Fixes #518
What was changed
Moves from raising on a fiber to using the scheduler interrupt for CanceledErrors.
Why?
Prevents FIber based workers from stalling.
Checklist
Closes [Bug] START_TO_CLOSE timeouts deterministically hang Fiber workers #518
Tested against the reproduction harness on [Bug] START_TO_CLOSE timeouts deterministically hang Fiber workers #518 and confirmed that activities proceed as expected
WARNING: Due to the nature of the bug, the new test case just hangs the entire test suite on the unpatched code - I didn't see an easy way to have it just go red quickly. On the patched code it runs as expected.