Reduce per-call SHA1 allocation in deterministic NewGuid()#780
Open
berndverst wants to merge 10 commits into
Open
Reduce per-call SHA1 allocation in deterministic NewGuid()#780berndverst wants to merge 10 commits into
berndverst wants to merge 10 commits into
Conversation
TaskOrchestrationContextWrapper.NewGuid() previously created and disposed a new SHA1 instance on every call. Since a single wrapper instance is used for the duration of one orchestration execution, and orchestrator code within that execution runs sequentially (never concurrently), the SHA1 instance can safely be cached on the wrapper and reused across calls via HashAlgorithm.Initialize(), avoiding a per-call allocation. The hashed name, DNS namespace bytes, byte-swap order, and RFC 4122 version/variant bit handling are all unchanged, so generated GUIDs are byte-for-byte identical to before this change for the same inputs. Adds regression tests asserting: - stable, golden-value deterministic GUIDs for fixed inputs - distinct GUIDs across repeated calls with the same instance/timestamp - identical GUID sequences across two independent wrapper instances given identical inputs (simulated replay) - the cached SHA1 instance is reused (not reallocated) across calls Fixes #778 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f9b8e1d4-b96d-41ec-adeb-a691aac6fee1
Contributor
There was a problem hiding this comment.
Pull request overview
This PR optimizes deterministic GUID generation in the Worker shim by avoiding per-call SHA1 allocations inside TaskOrchestrationContextWrapper.NewGuid(), while adding regression tests intended to protect replay compatibility and validate the optimization.
Changes:
- Cache and reuse a
SHA1instance inTaskOrchestrationContextWrapper.NewGuid()viaInitialize()to reset state between calls. - Add golden-value determinism tests and replay-sequence tests for
NewGuid(). - Extend the test
TestOrchestrationContexthelper to allow pinningInstanceIdandCurrentUtcDateTimeinputs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Worker/Core/Shims/TaskOrchestrationContextWrapper.cs | Reuses a cached SHA1 instance for deterministic GUID hashing to remove per-call allocation/disposal. |
| test/Worker/Core.Tests/Shims/TaskOrchestrationContextWrapperTests.cs | Adds deterministic GUID regression tests and enhances the test context helper to enable stable inputs. |
Comment on lines
+36
to
+41
| // Cached and reused across NewGuid() calls (instead of creating and disposing a new SHA1 instance | ||
| // per call) to reduce per-call allocation overhead. A single TaskOrchestrationContextWrapper is used | ||
| // for the duration of a single orchestration execution, and orchestrator code executes sequentially | ||
| // (never concurrently) within that execution, so reusing this instance is safe. HashAlgorithm.Initialize() | ||
| // resets all internal state before each use, so the computed hash is identical to using a fresh instance. | ||
| SHA1? cachedHashAlgorithm; |
Comment on lines
+494
to
+496
| FieldInfo cachedHashAlgorithmField = typeof(TaskOrchestrationContextWrapper) | ||
| .GetField("cachedHashAlgorithm", BindingFlags.Instance | BindingFlags.NonPublic)!; | ||
|
|
Comment on lines
623
to
624
| class TestOrchestrationContext : OrchestrationContext | ||
| { |
Address blocking review feedback on PR #780 (issue #778): 1. Cached SHA1 instance is now disposed. TaskOrchestrationContextWrapper implements IDisposable, and TaskOrchestrationShim disposes the previous wrapper right before replacing it with a new one on each replay/decision task, so the cached hash algorithm no longer accumulates undisposed instances. The base TaskOrchestration type has no disposal hook, so a documented CA1001 suppression covers the final instance per orchestration execution's lifetime, which is left for the GC/finalizer. 2. Corrected the code comment to honestly scope the optimization per TFM, verified against reference source: on .NET Framework, SHA1CryptoServiceProvider.Initialize() disposes/recreates its native CAPI handle every call, so native-handle churn is not eliminated there, only managed-side allocation. On modern .NET (net5.0+) on Windows, the CNG-based implementation can reuse and reset its native handle, so this caching also avoids native allocation on those runtimes. Added regression tests verifying Dispose() releases the cached SHA1 instance, is idempotent/safe to call without a prior NewGuid() call, and that GUID generation remains byte-identical after a mid-sequence dispose. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f9b8e1d4-b96d-41ec-adeb-a691aac6fee1
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (4)
test/Worker/Core.Tests/Shims/TaskOrchestrationContextWrapperTests.cs:428
- Using DateTime.Parse(..., null, ...) makes this test depend on the current thread culture. Use CultureInfo.InvariantCulture so the golden-value regression remains stable across environments.
DateTime.Parse("2023-05-06T07:08:09.1234567Z", null, DateTimeStyles.RoundtripKind));
test/Worker/Core.Tests/Shims/TaskOrchestrationContextWrapperTests.cs:446
- Using DateTime.Parse(..., null, ...) makes this test depend on the current thread culture. Use CultureInfo.InvariantCulture so the regression remains stable across environments.
DateTime.Parse("2024-01-01T00:00:00.0000000Z", null, DateTimeStyles.RoundtripKind));
test/Worker/Core.Tests/Shims/TaskOrchestrationContextWrapperTests.cs:468
- Using DateTime.Parse(..., null, ...) makes this test depend on the current thread culture. Use CultureInfo.InvariantCulture so the replay regression remains stable across environments.
DateTime timestamp = DateTime.Parse("2022-11-11T11:11:11.1111111Z", null, DateTimeStyles.RoundtripKind);
test/Worker/Core.Tests/Shims/TaskOrchestrationContextWrapperTests.cs:576
- Using DateTime.Parse(..., null, ...) makes this test depend on the current thread culture. Use CultureInfo.InvariantCulture so the golden-value regression remains stable across environments.
DateTime.Parse("2023-05-06T07:08:09.1234567Z", null, DateTimeStyles.RoundtripKind));
TaskOrchestrationShim.Execute() runs exactly once per shim instance in every real call site; extended-session resumption reuses the cached executor via TaskOrchestrationExecutor.ExecuteNewEvents() and never calls Execute() again. This meant the round-2 fix (disposing the *previous* wrapper inside Execute() before replacing it) never actually freed the *final* cached SHA1 for normal execution or for extended sessions, leaking a native hash handle per orchestration instance until GC/finalization. This introduces deterministic ownership-based disposal instead: - TaskOrchestrationShim now implements IDisposable directly (the defensive dispose-previous-wrapper logic in Execute() is kept as a safety net but documented as effectively unreachable for current callers). - GrpcDurableTaskWorker's processor wraps shim construction/execution in try/finally and disposes the shim once its single work item completes. - GrpcOrchestrationRunner disposes the shim immediately when it is not handed off to the extended-session cache (completed on first execution, or the request isn't part of an extended session), and registers a PostEvictionCallbackRegistration on cached entries so the shim is disposed exactly once whenever the cache entry is evicted for any reason (explicit Remove, sliding-expiration timeout, capacity eviction, or cache disposal) -- never while the orchestration may still resume. No generated GUID bytes, algorithm, namespace, endianness, version bits, target frameworks, or public API are changed. Tests added: - TaskOrchestrationShimTests: Dispose() forwards to and actually releases the wrapper's cached SHA1 (ObjectDisposedException on reuse), is a no-op when Execute() was never called, and is idempotent across repeated calls. - GrpcOrchestrationRunnerTests: extended-session cache eviction via explicit removal and via sliding-expiration timeout both dispose the cached shim's SHA1, using reflection (no InternalsVisibleTo from this assembly) to reach the private wrapperContext/cachedHashAlgorithm fields via the public ExtendedSessionState.TaskOrchestration property. Verified: Worker.Tests 145/145 passed, Worker.Grpc.Tests 139/139 passed (including new eviction tests). Fixes #778 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f9b8e1d4-b96d-41ec-adeb-a691aac6fee1
Addresses three remaining Medium lifecycle issues from round-4 review of PR #780: 1. ExtendedSessionsCache.Dispose() now calls MemoryCache.Clear() before Dispose(). Confirmed against the pinned Microsoft.Extensions.Caching.Memory 8.0.1 source that MemoryCache.Dispose() alone does NOT invoke post-eviction callbacks for entries still present in the cache -- only Clear(), Remove(), and the internal capacity/expiration removal paths do. Without this fix, any extended session still cached at worker shutdown would leak its cached shim (and SHA1 instance). 2. GrpcOrchestrationRunner's direct-execution path now wraps TaskOrchestrationExecutor construction/Execute()/cache Set() in try/finally, using a transferredShimToCache flag to gate disposal. If Execute() (or the subsequent Set() call) throws before the shim's ownership is transferred to the extended-sessions cache, the shim is now still deterministically disposed in finally instead of leaking. 3. The round-3 eviction-disposal tests asserted disposal immediately after triggering an eviction/removal, but MemoryCache dispatches post-eviction callbacks via Task.Factory.StartNew (i.e. asynchronously, on a background thread), so this was a latent race. Added a bounded polling helper (WaitUntilDisposedAsync) used by both existing tests, and added a new regression test (ExtendedSessionsCache_Dispose_DisposesCachedShimResources) proving that disposing the cache itself while an extended session is still pending disposes the cached shim's resources. Verified: - Worker.Tests: 145/145 passed - Worker.Grpc.Tests: 140/140 passed (139 + 1 new) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f9b8e1d4-b96d-41ec-adeb-a691aac6fee1
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (6)
src/Worker/Core/Shims/TaskOrchestrationContextWrapper.cs:42
- The comment implies the cached SHA1 is disposed primarily when the shim replaces the wrapper in a subsequent Execute call, but the actual primary ownership boundary is TaskOrchestrationShim.Dispose() (and cache eviction) once the shim is no longer usable. Updating this comment will prevent confusion about when disposal happens.
// This instance is disposed via Dispose() (see TaskOrchestrationShim, which disposes the previous
// wrapper before replacing it with a new one on the next replay/decision task).
test/Worker/Core.Tests/Shims/TaskOrchestrationContextWrapperTests.cs:405
- These tests parse a fixed, culture-invariant ISO-8601 timestamp but pass a null IFormatProvider, which makes the parse behavior depend on CurrentCulture. Using CultureInfo.InvariantCulture keeps the test independent of machine culture settings.
DateTime.Parse("2023-05-06T07:08:09.1234567Z", null, DateTimeStyles.RoundtripKind));
test/Worker/Core.Tests/Shims/TaskOrchestrationContextWrapperTests.cs:428
- These tests parse a fixed, culture-invariant ISO-8601 timestamp but pass a null IFormatProvider, which makes the parse behavior depend on CurrentCulture. Using CultureInfo.InvariantCulture keeps the test independent of machine culture settings.
DateTime.Parse("2023-05-06T07:08:09.1234567Z", null, DateTimeStyles.RoundtripKind));
test/Worker/Core.Tests/Shims/TaskOrchestrationContextWrapperTests.cs:446
- These tests parse a fixed, culture-invariant ISO-8601 timestamp but pass a null IFormatProvider, which makes the parse behavior depend on CurrentCulture. Using CultureInfo.InvariantCulture keeps the test independent of machine culture settings.
DateTime.Parse("2024-01-01T00:00:00.0000000Z", null, DateTimeStyles.RoundtripKind));
test/Worker/Core.Tests/Shims/TaskOrchestrationContextWrapperTests.cs:468
- This test parses a fixed, culture-invariant ISO-8601 timestamp but passes a null IFormatProvider, which makes the parse behavior depend on CurrentCulture. Using CultureInfo.InvariantCulture keeps the test independent of machine culture settings.
DateTime timestamp = DateTime.Parse("2022-11-11T11:11:11.1111111Z", null, DateTimeStyles.RoundtripKind);
test/Worker/Core.Tests/Shims/TaskOrchestrationContextWrapperTests.cs:576
- These tests parse a fixed, culture-invariant ISO-8601 timestamp but pass a null IFormatProvider, which makes the parse behavior depend on CurrentCulture. Using CultureInfo.InvariantCulture keeps the test independent of machine culture settings.
DateTime.Parse("2023-05-06T07:08:09.1234567Z", null, DateTimeStyles.RoundtripKind));
Comment on lines
+31
to
+33
| // entry to be removed via the normal removal path, which does invoke eviction callbacks | ||
| // synchronously-scheduled (via Task.Factory.StartNew) for each entry, ensuring deterministic | ||
| // cleanup of any cached shim/SHA1 resources before the cache itself is torn down. |
Comment on lines
+497
to
+498
| // Now set the extended session flag to false for this instance, which removes/evicts the cache | ||
| // entry and should synchronously invoke the eviction callback that disposes the cached shim. |
Round-5 fix: MemoryCache.Clear() (added in round 4 to force eviction callbacks to run before shutdown) throws ObjectDisposedException if called on an already-disposed MemoryCache. Since Dispose() was not guarded against repeat calls, invoking it twice (e.g. duplicate shutdown hooks, or concurrent shutdown paths) would throw instead of being a safe no-op. - Add an `int disposed` field guarded via `Interlocked.Exchange(ref this.disposed, 1) != 0` at the top of Dispose(), making it both idempotent (repeat calls short-circuit) and thread-safe (concurrent calls only let one caller proceed). - Clarify the disposal comment: Clear() only guarantees eviction callbacks are scheduled (via Task.Factory.StartNew) before Dispose() returns, not that they have completed. - Add ExtendedSessionsCache_Dispose_CalledMultipleTimes_DoesNotThrow (sequential double-dispose) and ExtendedSessionsCache_Dispose_CalledConcurrently_DoesNotThrow (8-thread concurrent dispose) regression tests. Verified: targeted GrpcOrchestrationRunnerTests 17/17, full Worker.Grpc.Tests 142/142, full Worker.Tests 145/145. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f9b8e1d4-b96d-41ec-adeb-a691aac6fee1
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
test/Worker/Grpc.Tests/GrpcOrchestrationRunnerTests.cs:499
- This comment says eviction/removal "should synchronously invoke" the eviction callback, but the test’s earlier note (and the helper name) correctly explain that MemoryCache dispatches post-eviction callbacks asynchronously. The wording here is misleading and could cause future contributors to reintroduce racy assertions.
// Now set the extended session flag to false for this instance, which removes/evicts the cache
// entry and should synchronously invoke the eviction callback that disposes the cached shim.
orchestratorRequest.Properties.Clear();
Comment on lines
46
to
48
| this.extendedSessions?.Clear(); | ||
| this.extendedSessions?.Dispose(); | ||
| GC.SuppressFinalize(this); |
…review) - Replace the round-5 Interlocked.Exchange disposed flag with a shared syncRoot lock guarding both Dispose() and GetOrInitializeCache(). This closes a race where Dispose() could observe an uninitialized cache field, mark itself disposed, and return -- while a concurrent GetOrInitializeCache() call then lazily constructed a brand-new MemoryCache that would never be disposed (since disposed was already permanently true), permanently leaking it. - GetOrInitializeCache() now throws ObjectDisposedException immediately if called after Dispose(), instead of possibly returning an already-disposed MemoryCache or racing to create an unreachable one. - Dispose() clears the extendedSessions field to null under the lock before tearing down the captured local reference outside the lock (avoids holding the lock during Clear()/Dispose(), which is safe since the eviction callback never calls back into ExtendedSessionsCache). - Add ExtendedSessionsCache_DisposeRaceWithGetOrInitializeCache_NeverLeaksCache, racing Dispose() against GetOrInitializeCache() across many iterations and asserting the returned cache (if any) is provably disposed, and that a repeated Dispose() call remains a safe no-op. - Fix a stale comment in ExternallyEndedExtendedSession_Evicted_DisposesCachedShimResources claiming the eviction callback fires synchronously; it is dispatched asynchronously and the test already awaits WaitUntilDisposedAsync. - Use CultureInfo.InvariantCulture instead of null for the DateTime.Parse format provider in the NewGuid() golden-value regression tests, removing any dependency on the test runner's current culture. Verified: targeted GrpcOrchestrationRunnerTests 18/18, full Worker.Grpc.Tests 143/143, full Worker.Tests 145/145 (golden GUID values unchanged after the culture fix). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f9b8e1d4-b96d-41ec-adeb-a691aac6fee1
Comment on lines
+461
to
465
| hashAlgorithm.Initialize(); | ||
| hashAlgorithm.TransformBlock(namespaceValueByteArray, 0, namespaceValueByteArray.Length, null, 0); | ||
| hashAlgorithm.TransformFinalBlock(nameByteArray, 0, nameByteArray.Length); | ||
| byte[] hashByteArray = hashAlgorithm.Hash; | ||
| #pragma warning restore CA5350 // Do Not Use Weak Cryptographic Algorithms -- not for cryptography |
Comment on lines
+267
to
+270
| // Invoked by the extended-sessions MemoryCache whenever a cached ExtendedSessionState entry is | ||
| // evicted, for any reason (explicit Remove, sliding-expiration timeout, capacity eviction, or the | ||
| // cache itself being disposed). Disposes the cached shim's resources (e.g. the SHA1 instance used by | ||
| // NewGuid) exactly once, at the point where the orchestration can no longer resume via this entry. |
- Add two deterministic (non-racy) tests asserting GetOrInitializeCache() throws ObjectDisposedException after Dispose(), covering both the never-initialized and already-initialized cache scenarios. - Rewrite the Dispose()/GetOrInitializeCache() race stress test to use a Barrier(2) so both competing tasks start racing at (approximately) the same instant, instead of relying on Task.Run queue-order bias which let the 'init wins' ordering dominate and never genuinely exercise the 'dispose wins' path. Track and assert that both orderings actually occurred across the 50 iterations. - Add a new deterministic ExtendedSessionsCache_Dispose_DisposesCachedEntryExactlyOnce test using a CountingDisposable spy wired up with the same PostEvictionCallbackRegistration pattern GrpcOrchestrationRunner uses for real cached shims, proving exact-once disposal of cached content tied to eviction (not just that the owning MemoryCache object becomes unusable). This is intentionally separate from the racing stress test: racing a Set() call concurrently against Dispose()'s internal Clear()/Dispose() split introduces a narrow window where an entry added in between is never evicted by that cache instance -- a test-harness artifact, not a production scenario. No production code changes in this round; NewGuid determinism, cache handoff exception cleanup, and eviction/shutdown lifecycle already passed final review. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f9b8e1d4-b96d-41ec-adeb-a691aac6fee1
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
src/Worker/Core/Shims/TaskOrchestrationContextWrapper.cs:464
HashAlgorithm.Hashis nullable; if it ever returns null,Array.Copywill throw aNullReferenceException. Since this code requires a hash afterTransformFinalBlock, add an explicit null-check (or null-forgiving with justification) to make the contract explicit and avoid nullable warnings.
byte[] hashByteArray = hashAlgorithm.Hash;
test/Worker/Core.Tests/Shims/TaskOrchestrationContextWrapperTests.cs:708
- This nested test context type is private and not designed for inheritance. To follow the repo guideline of sealing private non-base classes, consider making it
sealed.
class TestOrchestrationContext : OrchestrationContext
Comment on lines
+79
to
+81
| // happens via Dispose() (see the class remarks); this is still safe to do if it ever runs, since | ||
| // orchestrator code always runs synchronously within a single Execute call, so a previous wrapper | ||
| // is guaranteed to no longer be in use once we reach this point. |
| // Same shape as CallSubOrchestrationOrchestrator (so the orchestration is left pending in the | ||
| // extended-session cache) but also calls NewGuid() before awaiting, so the cached shim's wrapper | ||
| // has a live SHA1 instance whose disposal we can observe once the extended session is evicted. | ||
| class NewGuidThenCallSubOrchestrationOrchestrator : TaskOrchestrator<string, string> |
The Dispose()/GetOrInitializeCache() race stress test asserted that both the 'init wins' and 'dispose wins' orderings occurred at least once across 50 Barrier-coordinated iterations. A Barrier release does not guarantee fair or varying thread-pool scheduling, so correct, race-free code could legitimately let the same side win every iteration -- making this assertion's pass/fail outcome probabilistic and CI-flaky rather than a genuine correctness check. Remove the win-count tracking and both-orderings-required assertions. The test still asserts the invariants that must hold under every possible interleaving: a cache handed back by GetOrInitializeCache() before a concurrent Dispose() completes is provably disposed (TryGetValue throws ObjectDisposedException), and repeated Dispose() calls remain a safe, idempotent no-op. Also give each Barrier.SignalAndWait() a bounded timeout instead of waiting indefinitely, so a hung/stalled participant surfaces as a test failure (TimeoutException) rather than hanging the test run. The deterministic post-dispose contract tests (GetOrInitializeCache_AfterDispose*) and the deterministic exact-once disposal test (ExtendedSessionsCache_Dispose_DisposesCachedEntryExactlyOnce) are unchanged and continue to directly catch the original leak. No production code changes in this round. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f9b8e1d4-b96d-41ec-adeb-a691aac6fee1
…ertion Round 9 review found a genuine production race: ExtendedSessionsCache.Dispose() marked disposed under its lock but called Clear()/Dispose() on the underlying MemoryCache outside the lock. A GrpcOrchestrationRunner (or GrpcEntityRunner) holding the raw MemoryCache reference obtained before shutdown could call .Set(...) directly in the window after Clear() but before Dispose() completed, succeeding silently but never being evicted again -- while the runner recorded transferredShimToCache = true and skipped disposal, leaking the cached SHA1/shim on graceful shutdown racing with an in-flight extended-session orchestration. Fix: encapsulate all extended-session cache mutation inside ExtendedSessionsCache via three new internal synchronized methods -- TryGetCachedValue, RemoveCachedValue, TrySetCachedValue -- each guarded by the existing syncRoot lock and each checking disposed state. TrySetCachedValue returns false without mutating the cache if disposal has begun, so insertion and disposal are now atomic with respect to each other: either the entry is fully inserted before Dispose() acquires the lock (and will be evicted by the subsequent Clear()), or Dispose() wins first and insertion is rejected, in which case the caller retains and disposes the shim itself. - ExtendedSessionsCache.GetOrInitializeCache(double) and IsInitialized remain unchanged (pre-existing public API from main/PR #449). - GrpcOrchestrationRunner and GrpcEntityRunner now route all cache reads, removals, and insertions through the new synchronized methods instead of operating on the raw MemoryCache directly. - transferredShimToCache now reflects TrySetCachedValue's actual return value. - Added an end-to-end regression test (LoadAndRun_ExtendedSession_CacheDisposedDuringExecution_ShimIsDisposedImmediatelyNotLeaked) that disposes the cache mid-orchestration-execution and asserts the shim's cached SHA1 is disposed immediately rather than leaked. No public API changes, no GUID/replay behavior changes. Verified: full Worker.Grpc.Tests (147/147) and Worker.Tests (145/145) pass. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f9b8e1d4-b96d-41ec-adeb-a691aac6fee1
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| if (extendedSessionState.OrchestrationExecutor.IsCompleted) | ||
| { | ||
| extendedSessions.Remove(request.InstanceId); | ||
| extendedSessionsCache.RemoveCachedValue(request.InstanceId); |
This branch predates the deterministic CI repair applied on berndverst-reduce-shim-polling-load (commits c8b66aa and 8779f98), so PR #780's "Validate Build" job was failing on a flaky, unrelated baseline test: WorkItemStreamConsumerTests.PerItem_HeartbeatReset_KeepsTimerAlive intermittently reported SilentDisconnect instead of GracefulDrain under CI scheduling pressure. Root cause (from c8b66aa/8779f98): the test raced a real per-item wall-clock delay against the real 500ms silent-disconnect timer. Any scheduler delay in a continuation between the "item processed" signal and the next write could inflate an intended-short gap past the timeout, spuriously tripping SilentDisconnect even though production behavior (per-item timer reset) was correct. Fix (test-only, plus one purely-additive optional-parameter observability seam -- no shim polling or other production behavior changes): - WorkItemStreamConsumer.ConsumeAsync gains an optional trailing onSilentDisconnectTimerArmed callback, invoked synchronously every time the silent-disconnect timer is (re-)armed (once before the read loop, once per item). Defaults to null; the sole production call site is unaffected. - PerItem_HeartbeatReset_KeepsTimerAlive now uses this seam to assert the exact "armed"/"item" event interleaving directly, instead of inferring the per-item reset from elapsed real time. Runs in ~20ms with zero timing dependency. Cherry-picked directly from c8b66aa and 8779f98 (verified byte-identical result via `git diff 8779f98 -- <files>` producing no output); this branch's copies of both files were unmodified prior to this transplant, so the cherry-pick applied cleanly with no conflicts. Verified: full Worker.Grpc.Tests suite 147/147 passing; the previously-flaky test run 5x standalone, consistently ~20ms with no flakiness. No changes to NewGuid()/TaskOrchestrationContextWrapper or the round-9 cache fix in this commit. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f9b8e1d4-b96d-41ec-adeb-a691aac6fee1
Comment on lines
+147
to
+150
| // extendedSessions is only non-null when extendedSessionsCache is also non-null. All reads, | ||
| // removals, and (later) insertions are routed through extendedSessionsCache's synchronized | ||
| // wrapper methods rather than operating on the raw MemoryCache directly, so every operation | ||
| // is atomic with respect to a concurrent Dispose() of the cache (see round-9 fix below). |
| bool disposed; | ||
|
|
||
| /// <summary> | ||
| /// Gets a value indicating whether returns whether or not the cache has been initialized. |
Comment on lines
145
to
+149
| if (isExtendedSession && extendedSessions != null) | ||
| { | ||
| // extendedSessions is only non-null when extendedSessionsCache is also non-null. All reads, | ||
| // removals, and (later) insertions are routed through extendedSessionsCache's synchronized | ||
| // wrapper methods rather than operating on the raw MemoryCache directly, so every operation |
Comment on lines
114
to
+116
|
|
||
| // If an entity state was provided, even if we already have one stored, we always want to use the provided state. | ||
| if (!entityStateIncluded && extendedSessions.TryGetValue(request.InstanceId, out string? entityState)) | ||
| if (!entityStateIncluded && extendedSessionsCache!.TryGetCachedValue(request.InstanceId, out string? entityState)) |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #778
Summary
Reduces per-call SHA1 allocation in the deterministic
NewGuid()implementation (TaskOrchestrationContextWrapper.cs) by caching and reusing a singleSHA1instance viaInitialize(), instead of callingSHA1.Create()on every invocation. No changes to generated GUID bytes, algorithm, namespace, endianness, version bits, target frameworks, or replay behavior.Lifecycle safety (rounds 2–7)
Since the cached
SHA1holds a native/unmanaged handle, subsequent review rounds hardened disposal so the optimization doesn't leak resources:TaskOrchestrationContextWrapperandTaskOrchestrationShimareIDisposable; disposal is deterministic at execution/replacement boundaries (not mid-execution while an orchestration may resume).GrpcOrchestrationRunner's direct-execution and extended-session paths dispose the shim on normal completion/failure, with try/finally so an exception before cache hand-off doesn't leak it.ExtendedSessionsCachedisposes cached shims on eviction (PostEvictionCallbackRegistration) and on cache/worker shutdown (Clear()beforeDispose(), sinceMemoryCache.Dispose()alone skips eviction callbacks).ExtendedSessionsCache.Dispose()is idempotent and safe under concurrency: a shared lock (syncRoot) makes lazy cache initialization and disposal mutually exclusive, soDispose()can never raceGetOrInitializeCache()into leaking a cache, and post-disposalGetOrInitializeCache()calls always throwObjectDisposedException.Round 7
Addressed a test-quality gap: the previous concurrent race test queued
Task.Runwork without coordinating start times, so the "dispose wins" ordering was essentially never exercised and the test could have passed against the old broken behavior.GetOrInitializeCache()throwsObjectDisposedExceptionafterDispose(), covering both the never-initialized and already-initialized cache scenarios.Barrier(2)so both competing tasks start racing at (approximately) the same instant.ExtendedSessionsCache_Dispose_DisposesCachedEntryExactlyOncetest using aCountingDisposablespy wired up with the samePostEvictionCallbackRegistrationpatternGrpcOrchestrationRunneruses for real cached shims, proving exact-once disposal of cached content tied to eviction — not just that the owningMemoryCacheobject becomes unusable. Kept separate from the racing stress test, since racing aSet()call concurrently againstDispose()'s internalClear()/Dispose()split would introduce a narrow test-harness-only window where an entry added in between is never evicted.Round 8
Removed a probabilistic/flaky assertion from the race stress test: it previously required that both the "init wins" and "dispose wins" orderings occur at least once across 50
Barrier-coordinated iterations. ABarrierrelease does not guarantee fair or varying thread-pool scheduling, so correct, race-free code could legitimately let the same side win every iteration — making that assertion's pass/fail outcome probabilistic and CI-flaky rather than a genuine correctness check.GetOrInitializeCache()before a concurrentDispose()completes is provably disposed, and repeatedDispose()calls remain a safe, idempotent no-op.Barrier.SignalAndWait()a bounded timeout instead of waiting indefinitely, so a hung/stalled participant surfaces as a test failure (TimeoutException) rather than hanging the test run.No production code changes in rounds 7–8.
Round 9 (this update)
Fixed a genuine production shutdown/hand-off race (not test-only):
ExtendedSessionsCache.Dispose()markeddisposedunder its lock but calledClear()/Dispose()on the underlyingMemoryCacheoutside the lock. AGrpcOrchestrationRunner(orGrpcEntityRunner) holding the rawMemoryCachereference obtained before shutdown began could call.Set(...)directly on it in the window afterClear()but beforeDispose()completed — succeeding silently but never being evicted again — while the runner recordedtransferredShimToCache = trueand skipped disposal, leaking the cached SHA1/shim on a graceful worker shutdown racing with an in-flight extended-session orchestration.Fix: encapsulated all extended-session cache mutation inside
ExtendedSessionsCacheitself via three newinternalsynchronized methods —TryGetCachedValue,RemoveCachedValue,TrySetCachedValue— each guarded by the existingsyncRootlock and each checking disposed state.TrySetCachedValuereturnsfalsewithout mutating the cache if disposal has begun (or is concurrently in progress), so insertion and disposal are now atomic with respect to each other: either the entry is fully inserted beforeDispose()acquires the lock (and will be evicted by the subsequentClear()), orDispose()wins the lock first and insertion is rejected — in which case the caller retains and disposes the shim itself instead of assuming hand-off succeeded.ExtendedSessionsCache.GetOrInitializeCache(double)andIsInitializedremain unchanged (pre-existing public API frommain/PR Extended Sessions for Isolated (Orchestrations) #449) — no public API changes.GrpcOrchestrationRunnerandGrpcEntityRunnernow route all cache reads, removals, and insertions through the new synchronized methods instead of operating on the rawMemoryCachedirectly.transferredShimToCachenow reflectsTrySetCachedValue's actual return value, rather than being unconditionallytrue.LoadAndRun_ExtendedSession_CacheDisposedDuringExecution_ShimIsDisposedImmediatelyNotLeaked, that disposes the cache mid-orchestration-execution (simulating shutdown racing with an in-flight extended session) and asserts the shim's cachedSHA1is disposed immediately rather than silently leaked in a cache entry that can never be evicted again.Validation
Worker.Grpc.Tests: 147/147 passing.Worker.Tests: 145/145 passing (golden deterministic GUID values unchanged).ExtendedSessionsCache.cs,GrpcOrchestrationRunner.cs,GrpcEntityRunner.cs, andGrpcOrchestrationRunnerTests.cs.