Repository navigation
Conversation
5ba4786 to
4ff1a51
Compare
|
|
||
| #[workflow] | ||
| #[derive(Default)] | ||
| struct RunUntilWorkflow; |
There was a problem hiding this comment.
I'm sure there's some other existing workflow we can re-use rather than adding this.
| // Validation failures skip the SDK polling loops, but Core's activity manager still needs | ||
| // a poll to observe shutdown before its finalizer can complete. | ||
| if matches!(result, Err(WorkerRunError::Validation(_))) { |
There was a problem hiding this comment.
Hmm. This sorta feels like a separate bug that should be addressed... but doesn't need to be in this PR.
There was a problem hiding this comment.
#1646 draft PR for fixing the bug at the source
| /// Maps activity type to the function for executing activities of that type | ||
| activities: ActivityDefinitions, | ||
| task_tokens_to_cancels: HashMap<TaskToken, CancellationToken>, | ||
| tasks: TaskTracker, |
There was a problem hiding this comment.
Huh I did not know about this thing
| for cancellation in self.activity_half.task_tokens_to_cancels.values() { | ||
| cancellation.cancel(); | ||
| } |
There was a problem hiding this comment.
This shouldn't need to be added here. This should've happened as part of initiate_shutdown or otherwise part of the existing shutdown logic. There shouldn't be special activity cancel logic that only runs if you use run_until.
What motivated needing to add this here?
There was a problem hiding this comment.
This was Codex being over-defensive and I missed it.
4ff1a51 to
2d1309f
Compare
| self.common.worker.initiate_shutdown(); | ||
| // Activity completions can release Core's slots before their tasks drop their Core | ||
| // references, so wait for the SDK tasks before taking ownership for finalization. | ||
| self.activity_half.tasks.close(); | ||
| self.activity_half.tasks.wait().await; |
There was a problem hiding this comment.
I still think this maybe isn't quite right... initiate_shutdown I think should be unnecessary here. It's called by shutdown_handle.
Then, waiting for this new tasks thing to close, that should probably just be done in run itself because, same thing, there should be no special waiting logic in run_until... it should really only just be calling the shutdown and waiting for run to resolve.
2d1309f to
cab0ae2
Compare
cab0ae2 to
e80a834
Compare
| .await | ||
| .await; | ||
|
|
||
| self.common.worker.initiate_shutdown(); |
There was a problem hiding this comment.
I'm still not clear on why this line is needed. run_inner ends up always calling this indirectly via shutdown... so it should've already happened.
The only case seemingly is worker validation errors, in which case we should be able to just short-circuit I think and return that early?
This branch has not been deployed
2 participants
What was changed
Add
Worker::run_until(future)to trigger shutdown and await Core'sfinalize_shutdown().run()now waits for activity tasks before returning.Why?
Let applications tie worker lifetime to a future and release its registration when shutdown finishes.
Checklist
run()cleanup on success and error; reuses the existing activity workflow.