fix(worker): fix pause/resume functionality #651
Merged
Conversation
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
…ext` but does not have any pending tasks
The `is_shutting_down()` method incorrectly returned true for any non-Running state, including Paused workers. This caused: - `resume()` to fail with ShuttingDown error for paused workers - Paused workers with no in-flight tasks to exit immediately (zombie state) Now `is_shutting_down()` only returns true when: - State is explicitly Stopped (stop() was called) - OR shutdown signal has been triggered Added comprehensive tests for shutdown behavior across all worker states.
ReadinessService::poll_ready() only checked is_shutting_down() but not is_paused(), so paused workers would continue processing new jobs. Now poll_ready() returns Poll::Pending when the worker is paused, blocking new job processing until resume() is called. Added unit tests for ReadinessService verifying: - Running workers are ready to process jobs - Paused workers block job processing (return Pending) - Shutting down workers block job processing - Resumed workers are ready again Also fixed clippy warnings for format string inlining.
geofmureithi
requested changes
Dec 19, 2025
Member
geofmureithi
left a comment
geofmureithi
left a comment
There was a problem hiding this comment.
- See minor changes in the API
- Rethink both tests including
readiness_service_tests - Add a changelog entry it will be needed for workflows to be successful
Member
|
@Himmelschmidt Let me know if you need some help |
Contributor
Author
|
I should be able to do it, seems minor enough, I just have to find the time to actually do it, might take a day or two |
- Add wake() call in resume() to re-poll the worker after unpausing - Store waker in ReadinessService::poll_ready when paused so resume can wake it - Add public is_stopped() method for explicit stopped state checking - Refactor is_shutting_down() to use is_stopped() - Rewrite tests to use real workers with run_with_ctx
geofmureithi
requested changes
Dec 22, 2025
Member
geofmureithi
left a comment
geofmureithi
left a comment
There was a problem hiding this comment.
Please refactor the tests as requested. One worker test should be enough.
Also elaborate why you are changing the waker. That should only be done in WorkerContext::poll unless I am missing something.
Member
|
FYI: Sorry for the strict approach on this one, it affects core parts of the project. |
geofmureithi
requested changes
Dec 22, 2025
Member
geofmureithi
left a comment
geofmureithi
left a comment
There was a problem hiding this comment.
Some few minor things, and a question about replacing the waker.
geofmureithi
approved these changes
Dec 22, 2025
Member
geofmureithi
left a comment
geofmureithi
left a comment
There was a problem hiding this comment.
LGTM!
Thanks for this contribution!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
Fixes #650
Root cause:
is_shutting_down()returned true for paused workers (checked!self.is_running()), causing:resume()to fail withShuttingDownerror when in-flight tasks existedFix:
is_shutting_down()now only returns true when state isStoppedor shutdown signal is triggeredReadinessService::poll_ready()now explicitly checksis_paused()to block new job processingAlso suppresses spurious error log when dropping
WorkerContextwith no pending tasks.