Reduce per-call SHA1 allocation in deterministic NewGuid()

src/Worker/Core/ExtendedSessionsCache.cs

@@ -11,7 +11,18 @@ namespace Microsoft.DurableTask.Worker;
 /// </summary>
 public class ExtendedSessionsCache : IDisposable
 {
+    // Guards both the lazy-initialization of `extendedSessions` in GetOrInitializeCache() and the
+    // disposal state transition in Dispose(). Without this shared lock, Dispose() could observe
+    // `extendedSessions` as null (because it hasn't been lazily created yet), mark itself disposed,
+    // and return -- while a concurrent GetOrInitializeCache() call races in and constructs a brand
+    // new MemoryCache immediately afterwards. That cache would never be disposed (Dispose() has
+    // already run and is now a permanent no-op), leaking it and any entries added to it. The lock
+    // makes initialization and disposal mutually exclusive, so there's no window where a cache can
+    // be created after (or concurrently with) disposal.
+    readonly object syncRoot = new();
+
     MemoryCache? extendedSessions;
+    bool disposed;
 
     /// <summary>
     /// Gets a value indicating whether returns whether or not the cache has been initialized.
@@ -24,7 +35,38 @@ public class ExtendedSessionsCache : IDisposable
     /// </summary>
     public void Dispose()
     {
-        this.extendedSessions?.Dispose();
+        MemoryCache? cacheToDispose;
+        lock (this.syncRoot)
+        {
+            if (this.disposed)
+            {
+                // Already disposed by a previous (or concurrent, now-completed) call. MemoryCache.Clear()
+                // and MemoryCache.Dispose() are not safe to call more than once -- Clear() throws
+                // ObjectDisposedException if the cache has already been disposed -- so this guard makes
+                // Dispose() idempotent and safe under concurrent callers.
+                return;
+            }
+
+            this.disposed = true;
+
+            // Clear the field (under the same lock used by GetOrInitializeCache()) so that no caller
+            // can observe or lazily recreate a cache after this point; GetOrInitializeCache() checks
+            // `this.disposed` under the lock and throws ObjectDisposedException instead.
+            cacheToDispose = this.extendedSessions;
+            this.extendedSessions = null;
+        }
+
+        // MemoryCache.Dispose() does NOT invoke post-eviction callbacks for entries that are still
+        // present in the cache -- it merely tears down the cache's internal state. Any entries
+        // (e.g. cached extended-session state holding an IDisposable shim) that are still cached at
+        // shutdown would therefore never be disposed. Calling Clear() first forces every remaining
+        // entry to be removed via the normal removal path, which does invoke eviction callbacks for
+        // each entry, ensuring they are triggered instead of silently skipped. Note that eviction
+        // callbacks are queued asynchronously (via Task.Factory.StartNew), so Clear() does not
+        // guarantee those callbacks have completed by the time Dispose() returns -- it only
+        // guarantees they are scheduled before the cache itself is torn down.
+        cacheToDispose?.Clear();
+        cacheToDispose?.Dispose();
         GC.SuppressFinalize(this);
     }
 
@@ -36,13 +78,97 @@ public void Dispose()
     /// This specifies how often the cache checks for stale items, and evicts them.
     /// </param>
     /// <returns>The IMemoryCache that holds the cached <see cref="ExtendedSessionState"/>.</returns>
+    /// <exception cref="ObjectDisposedException">The cache has already been disposed.</exception>
     public MemoryCache GetOrInitializeCache(double expirationScanFrequencyInSeconds)
     {
-        this.extendedSessions ??= new MemoryCache(new MemoryCacheOptions
+        lock (this.syncRoot)
+        {
+            if (this.disposed)
+            {
+                throw new ObjectDisposedException(nameof(ExtendedSessionsCache));
+            }
+
+            this.extendedSessions ??= new MemoryCache(new MemoryCacheOptions
+            {
+                ExpirationScanFrequency = TimeSpan.FromSeconds(expirationScanFrequencyInSeconds / 5),
+            });
+
+            return this.extendedSessions;
+        }
+    }
+
+    /// <summary>
+    /// Attempts to retrieve the cached value for the given key, if present and this cache has not been
+    /// disposed (nor is concurrently being disposed by another thread). Callers should use this instead
+    /// of calling <see cref="IMemoryCache.TryGetValue(object, out object)"/> directly on the
+    /// <see cref="MemoryCache"/> returned by <see cref="GetOrInitializeCache"/>, since this method is
+    /// synchronized with <see cref="Dispose"/> and therefore can never observe -- or throw from -- a
+    /// cache instance that is concurrently being torn down.
+    /// </summary>
+    /// <typeparam name="T">The type of the cached value.</typeparam>
+    /// <param name="key">The cache key.</param>
+    /// <param name="value">When this method returns, contains the cached value, if found.</param>
+    /// <returns><c>true</c> if a value was found; <c>false</c> if not found, or if this cache is disposed.</returns>
+    internal bool TryGetCachedValue<T>(string key, out T? value)
+    {
+        lock (this.syncRoot)
+        {
+            if (this.disposed || this.extendedSessions is null)
+            {
+                value = default;
+                return false;
+            }
+
+            return this.extendedSessions.TryGetValue(key, out value);
+        }
+    }
+
+    /// <summary>
+    /// Removes the cached value for the given key, if present. This is a safe no-op if this cache has
+    /// already been disposed (or is concurrently being disposed). Synchronized with <see cref="Dispose"/>
+    /// for the same reason as <see cref="TryGetCachedValue{T}(string, out T)"/>.
+    /// </summary>
+    /// <param name="key">The cache key to remove.</param>
+    internal void RemoveCachedValue(string key)
+    {
+        lock (this.syncRoot)
+        {
+            if (this.disposed || this.extendedSessions is null)
+            {
+                return;
+            }
+
+            this.extendedSessions.Remove(key);
+        }
+    }
+
+    /// <summary>
+    /// Attempts to insert or replace the cached value for the given key. Returns <c>false</c> without
+    /// modifying the cache if this <see cref="ExtendedSessionsCache"/> has already been disposed, or is
+    /// concurrently being disposed by another thread -- in which case the caller retains ownership of
+    /// <paramref name="value"/> (and remains responsible for disposing it, if applicable) instead of
+    /// assuming the cache accepted it and will eventually evict and dispose it via a post-eviction
+    /// callback. Synchronized with <see cref="Dispose"/> so there is no window in which an entry can be
+    /// inserted after disposal has begun tearing the cache down (e.g. after <c>Clear()</c> has already
+    /// run but before the underlying <see cref="MemoryCache"/> itself has been disposed) -- an insertion
+    /// that would otherwise never be evicted or disposed again.
+    /// </summary>
+    /// <typeparam name="T">The type of the value to cache.</typeparam>
+    /// <param name="key">The cache key.</param>
+    /// <param name="value">The value to cache.</param>
+    /// <param name="options">The cache entry options (e.g. sliding expiration, eviction callback).</param>
+    /// <returns><c>true</c> if the value was inserted; <c>false</c> if rejected because this cache is disposed.</returns>
+    internal bool TrySetCachedValue<T>(string key, T value, MemoryCacheEntryOptions options)
+    {
+        lock (this.syncRoot)
         {
-            ExpirationScanFrequency = TimeSpan.FromSeconds(expirationScanFrequencyInSeconds / 5),
-        });
+            if (this.disposed || this.extendedSessions is null)
+            {
+                return false;
+            }
 
-        return this.extendedSessions;
+            this.extendedSessions.Set(key, value, options);
+            return true;
+        }
     }
 }

src/Worker/Core/Shims/TaskOrchestrationContextWrapper.cs

@@ -16,7 +16,7 @@
 /// <summary>
 /// A wrapper to go from <see cref="OrchestrationContext" /> to <see cref="TaskOrchestrationContext "/>.
 /// </summary>
-sealed partial class TaskOrchestrationContextWrapper : TaskOrchestrationContext
+sealed partial class TaskOrchestrationContextWrapper : TaskOrchestrationContext, IDisposable
 {
     // We use a stack (a custom implementation using a single-linked list) to make it easier for users
     // to abandon external events that they no longer care about. The common case is a Task.WhenAny in a loop.
@@ -33,6 +33,22 @@
     bool preserveUnprocessedEventsOnContinueAsNew;
     TaskOrchestrationEntityContext? entityFeature;
 
+    // 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.
+    // 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).
+    //
+    // Note: on .NET Framework, the underlying SHA1CryptoServiceProvider.Initialize() disposes and
+    // recreates its native CAPI hash handle on every call, so the native-handle churn is not eliminated
+    // there -- only the managed-side allocation (the HashAlgorithm object itself and SHA1.Create()'s
+    // provider lookup) is avoided. On modern .NET (net5.0+) running on Windows, the CNG-based
+    // implementation can use a reusable hash handle (BCRYPT_HASH_REUSABLE_FLAG) and reset it in place,
+    // so this caching also avoids native-handle churn on those runtimes.
+    SHA1? cachedHashAlgorithm;
+
     /// <summary>
     /// Initializes a new instance of the <see cref="TaskOrchestrationContextWrapper"/> class.
     /// </summary>
@@ -434,16 +450,18 @@
         byte[] namespaceValueByteArray = namespaceValueGuid.ToByteArray();
         SwapByteArrayValues(namespaceValueByteArray);
 
-        byte[] hashByteArray;
 #pragma warning disable CA5350 // Do Not Use Weak Cryptographic Algorithms -- not for cryptography
-        using (HashAlgorithm hashAlgorithm = SHA1.Create()) /* CodeQL [SM02196] Suppressed: SHA1 is not used for cryptographic purposes here. The information being hashed is not sensitive,
+        SHA1 hashAlgorithm = this.cachedHashAlgorithm ??= SHA1.Create(); /* CodeQL [SM02196] Suppressed: SHA1 is not used for cryptographic purposes here. The information being hashed is not sensitive,
                                                                and the goal is to generate a deterministic Guid. We cannot update to SHA2-based algorithms without breaking
                                                                customers' inflight orchestrations. */
-        {
-            hashAlgorithm.TransformBlock(namespaceValueByteArray, 0, namespaceValueByteArray.Length, null, 0);
-            hashAlgorithm.TransformFinalBlock(nameByteArray, 0, nameByteArray.Length);
-            hashByteArray = hashAlgorithm.Hash;
-        }
+
+        // Reset internal state before each use since this instance is cached and reused across calls
+        // rather than being created and disposed per call. This produces byte-for-byte identical hashes
+        // to constructing a new SHA1 instance for every call.
+        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
 
         byte[] newGuidByteArray = new byte[16];
@@ -458,6 +476,17 @@
         return new Guid(newGuidByteArray);
     }
 
+    /// <summary>
+    /// Releases the resources cached by this instance, including the <see cref="SHA1"/> instance used by
+    /// <see cref="NewGuid"/>. This should be called once this wrapper is no longer needed, i.e. once the
+    /// orchestration execution that owns it has completed and it is being replaced or discarded.
+    /// </summary>
+    public void Dispose()
+    {
+        this.cachedHashAlgorithm?.Dispose();
+        this.cachedHashAlgorithm = null;
+    }
+
     /// <summary>
     /// exits the critical section, if currently within a critical section. Otherwise, this has no effect.
     /// </summary>
@@ -533,7 +562,7 @@
     /// Gets the serialized custom status.
     /// </summary>
     /// <returns>The custom status serialized to a string, or <c>null</c> if there is not custom status.</returns>
     internal string? GetSerializedCustomStatus()
     {
         return this.DataConverter.Serialize(this.customStatus);
     }

src/Worker/Core/Shims/TaskOrchestrationShim.cs

@@ -13,8 +13,16 @@
 /// <remarks>
 /// This class is intended for use with alternate .NET-based durable task runtimes. It's not intended for use
 /// in application code.
+/// <para>
+/// The base <see cref="TaskOrchestration"/> type (defined in DurableTask.Core) has no disposal hook of its
+/// own, so the framework will never call <see cref="Dispose"/> automatically. Callers that construct a
+/// <see cref="TaskOrchestrationShim"/> directly (e.g. the gRPC worker processor and the orchestration
+/// runner) own its lifetime and are responsible for disposing it once they are done with it -- typically
+/// immediately after the single <see cref="Execute"/> call completes, or (for extended sessions) when the
+/// cached shim is evicted/removed.
+/// </para>
 /// </remarks>
-partial class TaskOrchestrationShim : TaskOrchestration
+partial class TaskOrchestrationShim : TaskOrchestration, IDisposable
 {
     readonly ITaskOrchestrator implementation;
     readonly OrchestrationInvocationContext invocationContext;
@@ -64,12 +72,20 @@
         innerContext.ErrorDataConverter = converterShim;
 
         object? input = this.DataConverter.Deserialize(rawInput, this.implementation.InputType);
+
+        // Defensively dispose any previous wrapper before replacing it, in case this shim instance is
+        // ever reused across more than one Execute call. Current callers construct a fresh shim per
+        // execution and call Execute exactly once, so the actual resource cleanup for this shim's wrapper
+        // 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.
+        this.wrapperContext?.Dispose();
         this.wrapperContext = new(innerContext, this.invocationContext, input, this.properties);
 
         string instanceId = innerContext.OrchestrationInstance.InstanceId;
         if (!innerContext.IsReplaying)
         {
             this.logger.OrchestrationStarted(instanceId, this.invocationContext.Name);
         }
 
         try
@@ -95,7 +111,7 @@
             // failure details are correctly propagated.
             throw new CoreTaskFailedException(e.Message, e.InnerException)
             {
                 FailureDetails = new FailureDetails(e,
                     e.FailureDetails.ToCoreFailureDetails(),
                     properties: e.FailureDetails.Properties),
             };
@@ -118,4 +134,15 @@
     {
         this.wrapperContext?.CompleteExternalEvent(name, input);
     }
+
+    /// <summary>
+    /// Releases the resources (e.g. the cached <see cref="System.Security.Cryptography.SHA1"/> instance
+    /// backing <see cref="TaskOrchestrationContext.NewGuid"/>) held by this shim's current wrapper. Callers
+    /// that construct this shim directly are responsible for calling this once they are finished with it,
+    /// since the base <see cref="TaskOrchestration"/> type provides no framework-invoked disposal hook.
+    /// </summary>
+    public void Dispose()
+    {
+        this.wrapperContext?.Dispose();
+    }
 }

src/Worker/Grpc/GrpcDurableTaskWorker.Processor.cs

@@ -774,14 +774,25 @@ await this.ExecuteWithRetryAsync(
                         };
 
                         TaskOrchestration shim = this.shimFactory.CreateOrchestration(name, orchestrator, parent);
-                        TaskOrchestrationExecutor executor = new(
-                            runtimeState,
-                            shim,
-                            BehaviorOnContinueAsNew.Carryover,
-                            request.EntityParameters.ToCore(),
-                            ErrorPropagationMode.UseFailureDetails,
-                            this.exceptionPropertiesProvider);
-                        result = executor.Execute();
+                        try
+                        {
+                            TaskOrchestrationExecutor executor = new(
+                                runtimeState,
+                                shim,
+                                BehaviorOnContinueAsNew.Carryover,
+                                request.EntityParameters.ToCore(),
+                                ErrorPropagationMode.UseFailureDetails,
+                                this.exceptionPropertiesProvider);
+                            result = executor.Execute();
+                        }
+                        finally
+                        {
+                            // This worker (unlike the extended-session path in GrpcOrchestrationRunner) never
+                            // reuses a shim across work items, so it owns and must dispose it once execution
+                            // of this single work item completes (e.g. to release the SHA1 instance cached by
+                            // NewGuid).
+                            (shim as IDisposable)?.Dispose();
+                        }
                     }
                     else
                     {

src/Worker/Grpc/GrpcEntityRunner.cs

@@ -113,7 +113,7 @@ public static async Task<string> LoadAndRunAsync(
             addToExtendedSessions = true;
 
             // 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))
             {
                 batch.EntityState = entityState;
                 stateCached = true;
@@ -135,15 +135,19 @@ public static async Task<string> LoadAndRunAsync(
 
         if (addToExtendedSessions)
         {
-            // addToExtendedSessions can only be set to true if extendedSessions is not null
-            extendedSessions!.Set(
+            // addToExtendedSessions can only be set to true if extendedSessionsCache is not null.
+            // TrySetCachedValue is synchronized with a concurrent ExtendedSessionsCache.Dispose() (see
+            // GrpcOrchestrationRunner for the full rationale); the entity's cached state is a plain
+            // string with nothing to dispose, but routing through the same encapsulated API keeps cache
+            // mutation consistently guarded against a racing shutdown.
+            extendedSessionsCache!.TrySetCachedValue(
                 request.InstanceId,
                 result.EntityState,
                 new MemoryCacheEntryOptions { SlidingExpiration = TimeSpan.FromSeconds(extendedSessionIdleTimeoutInSeconds) });
         }
         else
         {
-            extendedSessions?.Remove(request.InstanceId);
+            extendedSessionsCache?.RemoveCachedValue(request.InstanceId);
         }
 
         P.EntityBatchResult response = result.ToEntityBatchResult();