Reduce per-call SHA1 allocation in deterministic NewGuid() - #780
Open
berndverst wants to merge 13 commits into
Open
Reduce per-call SHA1 allocation in deterministic NewGuid()#780berndverst wants to merge 13 commits into
berndverst wants to merge 13 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. |
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));
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();
…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
- 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
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}
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
…ealed test doubles, disposed-cache graceful fallback - ExtendedSessionsCache: fix redundant IsInitialized doc comment wording. - GrpcOrchestrationRunner: correct DisposeEvictedExtendedSession comment to attribute eviction-callback triggering to Clear() (invoked from within ExtendedSessionsCache.Dispose()), not MemoryCache.Dispose() itself; reword stale round-9 comment to describe the nullable invariant explicitly; add missing null-forgiving operator for consistency. - TaskOrchestrationShim: reword comment describing wrapper disposal safety -- Execute is async, safety comes from the sequential/non-overlapping calling lifecycle, not from synchronous execution. - TaskOrchestrationContextWrapper: add an explicit guard around HashAlgorithm.Hash instead of an unguarded access, surfacing a clear exception if the never-null-in-practice contract is ever violated. - GrpcInstanceRunnerUtils: gracefully degrade to non-extended-session behavior (same as a null cache) when GetOrInitializeCache throws ObjectDisposedException due to a request racing the tail end of a graceful worker shutdown, instead of letting the exception fail the call. - Tests: seal the four nested test-double orchestrator classes (none serve as base classes); include actual runtime type names in reflection-failure diagnostics; remove remaining stale round-9 wording from test comments; add regression test for the disposed-cache graceful-fallback behavior. No public API, GUID/replay behavior, or algorithm changes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f9b8e1d4-b96d-41ec-adeb-a691aac6fee1
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 01dc5b7c-c742-45b7-ae8c-5cda9bcfbd2c
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 repeated SHA1 allocation in the deterministic
TaskOrchestrationContextWrapper.NewGuid()implementation while preserving the exact GUID algorithm, namespace, counter, byte ordering, version bits, and replay output.The optimization is intentionally limited to worker-owned shims with an explicit disposal lifecycle. Public
DurableTaskShimFactory.CreateOrchestrationcallers remain resource-neutral: they create and dispose SHA1 perNewGuid()call, so callers are not given a hidden disposal obligation through theTaskOrchestrationreturn type. Internal worker paths useCreateOrchestrationWithManagedLifetime, reuse one SHA1, and either dispose the shim or transfer it to the extended-session cache.Extended-session ownership
Extended sessions now use an explicit generation-based ownership state machine:
ExtendedSessionsCache.TryTakeExtendedSessiontransfers ownership to the runner before removing the cache entry.TryStoreExtendedSessionvalidates cache-entry options before ownership transfer and assigns a fresh generation to each insertion. Eviction callbacks can dispose only the generation they were registered for, so callbacks queued by removal, expiration, failed insertion, or shutdown cannot dispose a resumed or reinserted shim.This closes graceful-shutdown and expiration races without holding the cache lock during orchestration execution. If shutdown rejects reinsertion, or cache-entry setup fails (including zero/sub-tick timeouts), ownership remains with or returns to the runner and the shim is disposed there.
Entity cache behavior remains unchanged.
Correctness coverage
Added deterministic tests for:
Compatibility
No breaking change. Public signatures, serialization, gRPC wire behavior, deterministic GUID bytes, and orchestration replay behavior are unchanged.
Validation
Worker.Tests: 152/152 passing.Worker.Grpc.Tests: 151/151 passing.