.NET: fix: synchronize StateManager for executors that run concurrently in a superstep - #8628
RParnell-YB wants to merge 1 commit into
Conversation
…ly in a superstep InProcessRunner.RunSuperstepAsync delivers a superstep's messages to every receiving executor concurrently (one DeliverMessagesAsync task per receiver, awaited with Task.WhenAll). Each executor reaches the same StateManager through its bound IWorkflowContext, but StateManager kept its queued updates in a plain Dictionary and wrote it without synchronization, so two executors queueing state in the same superstep raced on it. In production this surfaced as InvalidOperationException: Operations that change non-concurrent collections must have exclusive access. A concurrent update was performed on this collection and corrupted its state. thrown from Dictionary.TryInsert inside WriteStateAsync when four fan-out roots finished within 14 ms of each other, and reported as an executor handler failure attributed to no step. Silent corruption is the other possible outcome. Every access to _scopes and _queuedUpdates now takes one lock. StateScope contents are only written by PublishUpdatesAsync and ImportStateAsync, which run between supersteps, so the awaiting StateScope reads stay outside the lock and no lock spans an await. PublishUpdatesAsync takes the queued updates out under the lock and publishes the snapshot; queries over the queue materialize under the lock before the caller enumerates. Test: 64 concurrent writers, 200 write/read/read-keys round-trips each, then publish, for a shared and a private scope. Fails 3/3 on the previous code, passes with this change; the existing 9 StateManager tests and the full Workflows unit-test suite (770) stay green.
|
RParnell-YB please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
1 similar comment
|
RParnell-YB please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Failed publication now discards queued updates before they are successfully committed.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
What changed in this PR
Synchronizes .NET workflow state access during concurrent executor execution.
Changes:
- Adds locking and snapshot-based state update handling.
- Adds concurrent shared/private scope tests.
| File | Description |
|---|---|
StateManager.cs |
Synchronizes state dictionaries and publication. |
StateManagerTests.cs |
Tests concurrent state writes and reads. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| lock (this._syncRoot) | ||
| { | ||
| queued = this._queuedUpdates.ToList(); | ||
| this._queuedUpdates.Clear(); |
Vincent Biret (baywet)
left a comment
There was a problem hiding this comment.
Thanks for the contribution!
| Throw.IfNull(scopeId); | ||
|
|
||
| if (!this._scopes.TryGetValue(scopeId, out StateScope? scope)) | ||
| lock (this._syncRoot) |
There was a problem hiding this comment.
why can't we use concurrent dictionaries instead of regular ones, and do away with all the locks?

Motivation and Context
Fixes #8627.
InProcessRunner.RunSuperstepAsyncdelivers a superstep's messages to every receiving executor concurrently (oneDeliverMessagesAsynctask per receiver, awaited withTask.WhenAll), and each executor reaches the sameStateManagerthrough its boundIWorkflowContext.StateManagerkept its queued updates in a plainDictionary<UpdateKey, StateUpdate>and wrote it with no synchronisation, so two executors queueing state in the same superstep raced on it. In a production host this surfaced aswhen four fan-out roots finished within 14 ms of each other, reported as an executor handler failure attributed to no step. Silent corruption is the other possible outcome of the same race. Neither
StateManager.csnorInProcessRunner.cschanged between 1.21.0 and 1.22.0; #8252 fixed the Declarative package'sWorkflowFormulaState, a different class.Description
_syncRootlock guards every access to_scopesand_queuedUpdates(GetOrCreateScope, the newTryGetScope,WriteStateAsync, bothClearStateAsyncoverloads, the queued-update lookup inReadValueOrDefaultAsync,ExportStateAsync,ImportStateAsync).await.StateScopecontents are only written byPublishUpdatesAsyncandImportStateAsync, which run between supersteps, so the awaitingStateScope.ReadStateAsync/ReadKeysAsynccalls stay outside the lock.GetUpdatesForScopeStrictmaterialises its result under the lock instead of returning a lazyWhereoverKeys, so callers enumerate a snapshot while other executors keep queueing.ApplyUnpublishedUpdatesandClearStateAsync(ScopeId)consume the snapshot's values rather than re-indexing the dictionary.PublishUpdatesAsynctakes the queued updates out under the lock and publishes the snapshot, so the clear is atomic with the read.ExportStateAsyncandImportStateAsynckeep their existing "no queued updates" checks, now under the lock.No public API change;
StateManageris internal.Contribution Checklist
Test:
StateManagerTests.Test_ConcurrentExecutors_QueueAndReadState_WithoutCorruptionAsync— 64 concurrent writers, 200WriteStateAsync/ReadStateAsync/ReadKeysAsyncround-trips each, thenPublishUpdatesAsync, for a shared and a private scope; asserts every key and value survives and nothing is left queued. Onmainit fails 3 runs out of 3; with this change it passes.Microsoft.Agents.AI.Workflows.UnitTests(net10.0): 770 passed, 0 failed.dotnet format whitespace --verify-no-changesclean on both files.