fix(sdk): re-poll update futures after workflow state changes by mfateev · Pull Request #1153 · temporalio/sdk-rust

mfateev · 2026-03-13T19:40:40Z

Summary

Bug: When both run and an update handler use wait_condition on each other's state, they deadlock. Update futures are polled before poll_wf_future, so the update sees stale state. After poll_wf_future sets the flag via state_mut, update futures are never re-polled within the same activation.
Fix: Re-poll update futures after poll_wf_future so that newly-unblocked wait_condition predicates are observed within the same activation. The inline update-polling code is extracted into poll_update_futures to avoid duplication.
Test: Adds a regression test (update_wait_condition_unblocked_by_run_state_change) that exercises the cross-future state_mut/wait_condition pattern.

Test plan

cargo check --tests — zero warnings in changed code
cargo test — all 450 tests pass
New regression test deadlocks without the fix, passes with it

🤖 Generated with Claude Code

…re::poll The convergence loop previously gave one extra re-poll of update futures after poll_wf_future, but missed state changes from futures that called state_mut while still returning Pending. This caused deadlocks when multiple updates/signals depended on each other's state within a single activation. Add a state_mutated flag (Cell<bool>) on WorkflowContextInner that state_mut() sets on every call. The convergence loop checks and resets it each iteration, continuing until no mutations occur in a full round. Also: - Extract poll_signal_futures and poll_update_futures into methods - Handle activation channel drop during shutdown gracefully instead of panicking - Replace the single regression test with a scripted convergence test harness covering 5 scenarios: update→workflow→update relay, cross-update unblocking, signal→update→workflow chain, update→signal dependency, and a full 7-flag chain across all handler types Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Sushisource

Overall makes sense to me, but, some AI cleanup to do

Sushisource · 2026-03-14T00:20:46Z

+                                    if activation_tx.send(Ok(act)).is_err() {
+                                        return;
+                                    }


I don't think we want to silently drop this. I have had spurious errors where the original line can cause a panic, and I've not been able to figure out why - but I don't want to just gloss over it because it means something is wrong somewhere else.

Sushisource · 2026-03-14T00:22:20Z

+type Step = (Option<usize>, Vec<usize>);
+
+fn wait(flag: usize) -> Step { (Some(flag), vec![]) }
+fn set(flag: usize) -> Step { (None, vec![flag]) }


This should be an enum since the wait and set cases are mutually exclusive

Sushisource · 2026-03-14T00:24:18Z

+    }
+}
+
+/// Helper: run a scripted convergence test.  Starts the workflow, sends


Suggested change

/// Helper: run a scripted convergence test. Starts the workflow, sends

/// Run a scripted convergence test. Starts the workflow, sends

Very Claude-y comment

Sushisource · 2026-03-14T00:24:50Z

+// ---------------------------------------------------------------------------
+// Scripted convergence-loop test harness
+// ---------------------------------------------------------------------------
+//


This whole thing could be in its own file. Not really specifically an update test at this point.

Sushisource · 2026-03-14T00:25:36Z

-                Ok(remaining) => self.signal_futures = remaining,
-                Err(e) => {
-                    self.fail_wft(run_id, anyhow!("Signal handler error: {}", e));
+            // Convergence loop: a state_mut call in any future can unblock


"Convergence loop" isn't a phrase that we use elsewhere - can just eliminate it / reword.

Sushisource · 2026-03-14T00:28:17Z

 impl WorkflowFuture {
+    /// Poll all in-progress signal futures, removing completed ones.
+    /// Returns `Err` if a signal handler failed.
+    fn poll_signal_futures(&mut self, cx: &mut Context) -> Result<(), Error> {


I'm not a fan of extracting functions that are only used in one place. Let's just keep these inline.

- Revert silent drop of activation send error (keep original .expect()) - Reword "convergence loop" comment - Change Step type from tuple to enum with Wait/Set variants - Remove "Helper:" prefix from doc comment - Move scripted test harness to poll_loop_tests.rs (own file) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Remove query_only_activation_should_not_advance_workflow and nonexistent_query_should_not_advance_workflow tests. These tests used a poll_fn with side effects (state_mut) that made the workflow advance on re-poll, violating the futures contract. The convergence loop correctly re-polls futures after state mutations, and a properly written workflow won't advance without actual unblocking events. Replace manual Default impl for ChainWf with #[derive(Default)].

mfateev · 2026-03-15T01:21:11Z

Removed two query tests

Removed query_only_activation_should_not_advance_workflow and nonexistent_query_should_not_advance_workflow.

These tests used a CompleteOnSecondPollWf workflow that called state_mut inside a poll_fn, mutating workflow state as a side effect of being polled:

poll_fn(|_| {
    if ctx.state(|s| s.polled_once) {
        Poll::Ready(())
    } else {
        ctx.state_mut(|s| s.polled_once = true);
        Poll::Pending
    }
})

This violates the futures contract — a future that returned Pending should only become Ready when something external has changed (via a waker), not simply because it was polled again. The convergence loop in this PR correctly re-polls futures after state_mut calls, which causes this workflow to complete on the first activation.

The tests were based on an invalid assumption: that the SDK must never poll the workflow future more than once per activation. In reality, extra polls are safe for any correctly written workflow — if nothing has changed, the future just returns Pending again. This is true even for query-only activations.

The remaining query tests (non_legacy_query_should_see_state_after_workflow_advances, query_returns_workflow_context_view_info) use proper patterns (wait_condition, signals) and continue to pass.

Also fixed a clippy derivable_impls warning on ChainWf in poll_loop_tests.rs.

Sushisource

Sorry, one last one, don't want to drop this test entirely. Thanks for this!

Sushisource · 2026-03-16T18:01:08Z

-/// "Workflow completion had a legacy query response along with other commands.
-/// This is not allowed and constitutes an error in the lang SDK."
-#[tokio::test]
-async fn query_only_activation_should_not_advance_workflow() {


I think we need to keep the intent of this test if not the exact implementation. It should be the case that query-only activations cannot cause the main workflow function to advance.

Do you have any idea how this can be tested? Counting activations is not a stable approach.

Restored both tests (query_only_activation_should_not_advance_workflow and nonexistent_query_should_not_advance_workflow). The key fix: switched from state_mut() to Cell<bool> with state() for interior mutability. The original state_mut() triggers the re-polling loop in WorkflowFuture::poll(), which would cause the workflow to complete on the first activation, making the test ineffective. With Cell<bool>, the workflow genuinely stays pending after the first poll, so a spurious poll from a query-only activation would be detected as a CompleteWorkflowExecution command.

…polling The original tests used state_mut() which triggers the re-polling loop, causing the workflow to complete on the first activation and making the tests ineffective. Using Cell<bool> with state() provides interior mutability without triggering re-polling, so the workflow genuinely stays pending and the query-only invariant is properly tested. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Sushisource

Thanks! Looks like it just needs a format.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

eduhenke · 2026-03-25T13:40:03Z

@mfateev
I had this bug and was dealing with a workaround, so I'm happy to see it fixed, However in the sync handlers' case it still seems broken. From what I've seen sync update handlers run inside ctx.state_mut(|wf| Self::handle(wf, ...)) in handle_job, before the convergence loop starts. take_state_mutated() clears the flag at the loop entry, so the mutation from the sync handler is invisible to the loop. Does what I explained make sense and is this expected, or should sync handler's case be covered in this PR too?

mfateev requested a review from a team as a code owner March 13, 2026 19:40

mfateev force-pushed the fix/update-repoll-v2 branch from 0145cb9 to 388df7c Compare March 13, 2026 19:43

mfateev force-pushed the fix/update-repoll-v2 branch from 388df7c to 45f857a Compare March 13, 2026 23:43

Sushisource reviewed Mar 14, 2026

View reviewed changes

mfateev and others added 4 commits March 14, 2026 18:23

inline poll_signal_futures and poll_update_futures

9947d51

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

fix formatting (rustfmt)

0917e0b

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

fix: rustfmt import formatting in queries.rs

bf335db

Sushisource reviewed Mar 16, 2026

View reviewed changes

mfateev and others added 3 commits March 18, 2026 23:52

Merge remote-tracking branch 'upstream/master' into fix/update-repoll-v2

92a1885

chore: restore original comments, remove superfluous ones

56187d9

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Sushisource approved these changes Mar 19, 2026

View reviewed changes

mfateev and others added 2 commits March 19, 2026 00:21

style: fix cargo fmt import formatting in queries.rs

b7e11d7

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Merge branch 'master' into fix/update-repoll-v2

bd3efb8

mfateev merged commit ba203c6 into temporalio:master Mar 19, 2026
19 checks passed

mfateev deleted the fix/update-repoll-v2 branch March 19, 2026 16:47

dependabot Bot mentioned this pull request May 4, 2026

Bump Escendit.Extensions.Workflows and 2 others escendit/extensions-hosting#260

Closed

dependabot Bot mentioned this pull request Jun 27, 2026

Bump Temporalio from 1.5.0 to 1.15.0 juwez/Meowfinder#81

Merged

	/// Helper: run a scripted convergence test. Starts the workflow, sends
	/// Run a scripted convergence test. Starts the workflow, sends

Conversation

mfateev commented Mar 13, 2026

Summary

Test plan

Uh oh!

Sushisource left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mfateev commented Mar 15, 2026

Removed two query tests

Uh oh!

Sushisource left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sushisource left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eduhenke commented Mar 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants