fix(notification-service): evict stale OAuth2 token cache entries after credential rotation (review 2026-10-21 06/MEM-3)

src/ZB.MOM.WW.ScadaBridge.NotificationService/OAuth2TokenService.cs

@@ -15,8 +15,17 @@ namespace ZB.MOM.WW.ScadaBridge.NotificationService;
 /// </summary>
 public class OAuth2TokenService
 {
+    /// <summary>
+    /// How long an entry must have gone unused — with its token expired or never acquired —
+    /// before the next token request evicts it. A credential rotation otherwise leaves the
+    /// old credential's entry (and its semaphore) cached for the life of the process
+    /// (review 2026-10-21 06/MEM-3).
+    /// </summary>
+    internal static readonly TimeSpan StaleEntryGrace = TimeSpan.FromHours(1);
+
     private readonly IHttpClientFactory _httpClientFactory;
     private readonly ILogger<OAuth2TokenService> _logger;
+    private readonly TimeProvider _timeProvider;
 
     // Cache keyed by a hash of the credential string. Each distinct
     // tenant/client/secret triple gets its own cached token and its own lock.
@@ -28,11 +37,27 @@ public class OAuth2TokenService
     public OAuth2TokenService(
         IHttpClientFactory httpClientFactory,
         ILogger<OAuth2TokenService> logger)
+        : this(httpClientFactory, logger, TimeProvider.System)
+    {
+    }
+
+    /// <summary>Test seam: initializes the service with an explicit clock.</summary>
+    /// <param name="httpClientFactory">Factory used to create HTTP clients for token endpoint requests.</param>
+    /// <param name="logger">Logger instance.</param>
+    /// <param name="timeProvider">Clock used for token expiry and stale-entry eviction.</param>
+    internal OAuth2TokenService(
+        IHttpClientFactory httpClientFactory,
+        ILogger<OAuth2TokenService> logger,
+        TimeProvider timeProvider)
     {
         _httpClientFactory = httpClientFactory;
         _logger = logger;
+        _timeProvider = timeProvider;
     }
 
+    /// <summary>The number of credential entries currently cached (test observability).</summary>
+    internal int CachedEntryCount => _cache.Count;
+
     /// <summary>
     /// Gets a valid access token, refreshing if expired.
     /// Credentials format: "tenantId:clientId:clientSecret"
@@ -54,12 +79,15 @@ public class OAuth2TokenService
         // Cache key incorporates authority + scope so two configurations with the same
         // credentials but different endpoints/scopes never share a token.
         var key = CredentialKey($"{credentials}|{authority}|{scope}");
+        var now = _timeProvider.GetUtcNow();
+        EvictStaleEntries(key, now);
         var entry = _cache.GetOrAdd(key, _ => new CacheEntry());
+        entry.LastUsedTicks = now.UtcTicks;
 
         // Read the token/expiry pair through a single volatile reference so a lock-free
         // reader can never observe a torn token/expiry combination (arch-review S8).
         var current = entry.Current;
-        if (current != null && DateTimeOffset.UtcNow < current.Expiry)
+        if (current != null && _timeProvider.GetUtcNow() < current.Expiry)
         {
             return current.Token;
         }
@@ -69,7 +97,7 @@ public class OAuth2TokenService
         {
             // Double-check after acquiring the per-credential lock.
             current = entry.Current;
-            if (current != null && DateTimeOffset.UtcNow < current.Expiry)
+            if (current != null && _timeProvider.GetUtcNow() < current.Expiry)
             {
                 return current.Token;
             }
@@ -116,7 +144,7 @@ public class OAuth2TokenService
             // Publish the token/expiry pair as a single atomic reference assignment
             // (Refresh 60s before expiry).
             entry.Current = new TokenValue(
-                token, DateTimeOffset.UtcNow.AddSeconds(expiresIn - 60));
+                token, _timeProvider.GetUtcNow().AddSeconds(expiresIn - 60));
 
             // The token endpoint identity is logged by tenant only — never
             // the client secret or the access token itself.
@@ -130,6 +158,29 @@ public class OAuth2TokenService
         }
     }
 
+    /// <summary>
+    /// Drops every other entry that has been idle past <see cref="StaleEntryGrace"/> and
+    /// holds no live token. An entry whose lock is held (a refresh in flight) is skipped.
+    /// Losing a race with a caller that just fetched the entry is harmless: that caller
+    /// completes against the orphaned entry and the next request simply re-fetches.
+    /// The semaphore is deliberately not disposed — a racing waiter may still hold it,
+    /// and an un-awaited <see cref="SemaphoreSlim"/> owns no unmanaged handle.
+    /// </summary>
+    private void EvictStaleEntries(string keepKey, DateTimeOffset now)
+    {
+        var idleCutoff = now.UtcTicks - StaleEntryGrace.Ticks;
+        foreach (var pair in _cache)
+        {
+            if (pair.Key == keepKey) continue;
+            var entry = pair.Value;
+            if (entry.LastUsedTicks > idleCutoff) continue;
+            var current = entry.Current;
+            if (current != null && now < current.Expiry) continue;
+            if (entry.Lock.CurrentCount == 0) continue;
+            _cache.TryRemove(pair);
+        }
+    }
+
     /// <summary>
     /// A stable, non-reversible key for the credential string so the cache
     /// is partitioned by credential identity without holding the secret as a key.
@@ -149,5 +200,8 @@ public class OAuth2TokenService
         // always sees a self-consistent token/expiry pair (or null for "no token yet").
         public volatile TokenValue? Current;
         public readonly SemaphoreSlim Lock = new(1, 1);
+
+        // UTC ticks of the last GetTokenAsync touch; drives stale-entry eviction.
+        public long LastUsedTicks;
     }
 }