Repository navigation
Deliver local activity cancels before evicting a run - #1620
Conversation
051b82d to
b25d552
Compare
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
The cancellation and eviction handoff spans concurrent activity and workflow streams and warrants final human validation.
Review effort: Balanced
Findings: None
What changed in this PR
Fixes a shutdown hang by ensuring local-activity cancellations are dispatched before workflow eviction removes their tracking state.
Changes:
- Tracks cancellation delivery and blocks eviction until queued cancellations are polled.
- Prevents retries and spurious resolutions after cancellation or eviction.
- Adds regression coverage and user-facing changelog entries.
| File | Description |
|---|---|
crates/sdk-core/src/worker/workflow/workflow_stream.rs |
Handles cancellation-delivery notifications. |
crates/sdk-core/src/worker/workflow/mod.rs |
Prioritizes local-activity notifications. |
crates/sdk-core/src/worker/workflow/managed_run.rs |
Withholds eviction while cancellations remain queued. |
crates/sdk-core/src/worker/mod.rs |
Connects the notification channel. |
crates/sdk-core/src/worker/activities/local_activities.rs |
Tracks cancellation state and suppresses retries. |
crates/sdk-core/src/core_tests/workflow_tasks.rs |
Adds eviction and cancellation regressions. |
crates/sdk-core/CHANGELOG.md |
Documents Core behavior changes. |
CHANGELOG.md |
Documents Rust SDK behavior changes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@mjameswh Thanks for the quality review as always. I was thinking about inlining some of that setup stuff myself and ended up liking the fewer LoC, but, I think you're right that it's better to inline. |
I do think inlining can also be a very good thing in some contexts, e.g. they are used consistently, their external behaviors is clear enough that you dont need to actually look into how they work, etc. That all comes down to the cognitive load they put on human developers, and how easy it is for a developer to understand the test. What I'm worried about are one-off test helpers, and especially when they obscure some of the mechanic that we're trying to test. I feel like LLMs have a tendency to proliferate such helpers, which results in a ton of helpers functions that only exist for the few tests that created at a same point in time. |
f45f15b to
46ff96c
Compare
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn cancel_all_in_run_queues_one_cancel_per_attempt_and_reports_delivery() { |
There was a problem hiding this comment.
“Per attempt” appears stale: this test creates only one in-flight attempt and verifies that three cancellation requests collapse into one cancellation plus one delivery notification. Could this be renamed to duplicate_cancel_requests_queue_one_cancel_and_report_delivery?
| if self.activation.is_none() | ||
| && !self.more_pending_work() | ||
| && !self.has_undelivered_la_cancels() |
There was a problem hiding this comment.
Another agent concludes otherwise:
The first part of the reported scenario is possible: eviction can be requested while a normal activation is outstanding, and
_check_more_activationsthen returns before queuingCancelAllInRun.The invalidation conclusion does not appear reachable with the current activation lifecycle, though:
- While that normal activation remains outstanding, this fallback cannot emit eviction because
self.activation.is_none()is false.- The only place that clears the outstanding activation is
finish_activation, called fromWFStream::process_post_activation.- Immediately after clearing it,
process_post_activationcallsrh.check_more_activations()when no buffered WFT already produced another activation.- That call enters
_check_more_activationswithtrying_to_evictset and sinksCancelAllInRunbefore reaching any eviction-producing branch.- If a buffered WFT does produce another activation instead, eviction remains blocked by that new outstanding activation; its eventual post-activation processing repeats the same sequence.
Therefore, by the time this fallback can observe
activation.is_none()and emit eviction, the run has passed through_check_more_activationsand its LA cancellations have been queued. If the LA completed during the outstanding activation, there is no longer a running attempt to cancel.So I do not think the reported path is currently a correctness bug. However, the final guard itself cannot distinguish “all cancels delivered” from “cancels never queued”; safety relies on the surrounding activation lifecycle. A regression test for “eviction requested while a normal activation and LA are both outstanding” would be useful protection and would make this invariant explicit. Moving
CancelAllInRunbefore the outstanding-activation early return would make the invariant more local, but it would also change cancellation timing and is not required to fix a demonstrated bug.
I think that assessment is correct, but it's hard to be 100% sure we're not missing some edge case. That also seems like something that would be easy to break at some point in the future. I'd suggest adding a regression test reproducing that behavior.
There was a problem hiding this comment.
Actually, trying to add the attached test to confirm this, it looks like the described scenario is incorrect in a different way than what Copilot predicted.
The test fails on the following dbg_panic assertion:
There should not be any outgoing commands or messages when preparing a
completion response if the activation was only an eviction. This is an SDK bug.
That's because the first LA's marker was appended to machines_wft_response.commands() during the second activation, but that command was never flushed. So it's still in the outbound command buffer by the time the worker processes the eviction request.
Surprisingly, that specific scenario seems not be validated by the very similar tests in core_tests/workflow_tasks.ts.
There was a problem hiding this comment.
Addressed this by ensuring we allow already-completed LA results to be written into history, but we still avoid recording the cancels of any LAs that were cancelled due to eviction.
Luckily this is possible now b/c of the other recent changes that say which activation an LA should be resolved in.
46ff96c to
823168a
Compare
| || (machines_wft_response.replaying && !data.is_forced_failure) | ||
| || is_query_playback | ||
| || data.activation_was_eviction | ||
| || (data.activation_was_eviction && !eviction_has_output) |
There was a problem hiding this comment.
Just thinking out loud...
With this addition, I think that the if block at L1236-L1239 below is now theoretically unreachable, as (should_respond && activation_was_eviction) => !eviction_has_output => can_report_eviction => !am_broken.
It may still possible to enter the if block if has_query_responses && am_broken && activation_was_eviction, but I do believe that an eviction activation can't contain query response.
Now, I'm not saying that we remove L1236-L1239, but:
- Do you think this can hide a regression in some edge failure scenario?
- Even if I'm correct (not 100% sure about this), we may just decide to keep the condition anyway, as a paranoid defensive measure, but then we should maybe mention on the comment at L1236-1237, to avoid tripping on this at a later time.
There was a problem hiding this comment.
Added a couple more dbg mode invariants to capture the intent here.
mjameswh
left a comment
mjameswh
left a comment
There was a problem hiding this comment.
I left one more comment about a potential edge case, but I may be wrong, and I'm not even sure if that could actually have consequences, so not blocking.
When a run was evicted, Core queued a cancel for each of its in-flight local activities and produced the eviction activation in the same pass. If lang completed the eviction before its activity poll reached the cancel, invalidating the run made the LA manager drop the cancel as untracked, so lang kept running the local activity while Core reported local activity shutdown complete (temporalio/sdk-python#1837). The eviction activation is now withheld while any of the run's in-flight local activities has a queued, not yet polled cancel, and the LA manager wakes the run once the last one is handed to lang. Cancels are queued once per attempt, a local activity still waiting for dispatch resolves as cancelled instead of starting, and an attempt that fails after its cancel was requested is not retried locally. Local activity results that arrive while the eviction is withheld are discarded, as they already were once the eviction activation was outstanding, so the cancellation the eviction caused never reaches workflow code as an activation. CancelAllInRun yields no immediate resolutions for the same reason: it is only sunk for runs that are terminating or evicting, and applying them surfaced spurious cancellations for backing-off or never-dispatched local activities.
…ed during eviction
82641d0 to
9fd1615
Compare
What was changed
A run's eviction activation is withheld until every queued cancel for its in-flight local activities has been returned from an activity poll. Cancels are queued once per attempt, an undispatched local activity resolves as cancelled, and an attempt failing after its cancel was requested is not retried locally, matching the server's
RETRY_STATE_CANCEL_REQUESTED.CancelAllInRunno longer returns immediate resolutions, as it is only sunk for terminating or evicting runsWhy
_check_more_activationssinksCancelAllInRun, which only queues cancels, then produces the eviction activation. Once lang completes it,InvalidateRundrops the run's LAs from the outstanding set andnext_pendingdiscards the queued cancel as untracked, so lang keeps running the LA while Core reports shutdown complete. This was the hang I was originally looking at in temporalio/sdk-python#1837.Testing
New
la_cancel_delivered_before_eviction_activationfails without the fix; six more cover withheld-eviction results, undispatched and backing-off LAs, and no retry after cancel. If any look low-value just lmk.