From e583b068a1fdb21e950cd676a528ff9da32483a5 Mon Sep 17 00:00:00 2001
From: Joseph Doherty <dohejw01@gmail.com>
Date: Sun, 11 Oct 2026 09:13:23 -0400
Subject: [PATCH] fix(historian): release the retention applock only when
 acquired (#667 F3)

---
 .../SqlServerHistorianRetention.cs            | 25 +++++++++--
 .../RetentionTests.cs                         | 42 +++++++++++++++++++
 2 files changed, 63 insertions(+), 4 deletions(-)

diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.SqlServer/SqlServerHistorianRetention.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.SqlServer/SqlServerHistorianRetention.cs
index 3a39dd9e..52e96f0f 100644
--- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.SqlServer/SqlServerHistorianRetention.cs
+++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.SqlServer/SqlServerHistorianRetention.cs
@@ -39,15 +39,31 @@ internal sealed class SqlServerHistorianRetention : IHistorianRetention
     /// <summary>Test hook invoked after every delete batch.</summary>
     internal Action? AfterBatch { get; set; }
 
+    /// <summary>Test seam for <see cref="SqlConnection.ClearPool"/>.</summary>
+    internal Action<SqlConnection> ClearPool { get; set; } = SqlConnection.ClearPool;
+
     public async Task<IReadOnlyList<RetentionPurgeResult>> PurgeAsync(CancellationToken ct)
     {
         await _bootstrap.EnsureAsync(ct).ConfigureAwait(false);
         var cutoff = _time.GetUtcNow().UtcDateTime.AddDays(-_days);
 
         await using var conn = await _conn.OpenAsync(ct).ConfigureAwait(false);
+        var acquired = false;
         try
         {
-            if (!await TryLockAsync(conn, ct).ConfigureAwait(false))
+            try
+            {
+                acquired = await TryLockAsync(conn, ct).ConfigureAwait(false);
+            }
+            catch
+            {
+                // Ambiguous: the server may have granted the lock before the client saw the reply. We cannot
+                // release what we are not sure we hold, so close the session physically instead.
+                ClearPool(conn);
+                throw;
+            }
+
+            if (!acquired)
             {
                 _log.LogInformation("Historian retention purge skipped: another purger holds the applock.");
                 return [];
@@ -74,8 +90,9 @@ internal sealed class SqlServerHistorianRetention : IHistorianRetention
         }
         finally
         {
-            // Also runs when acquire threw: the server may have granted the lock before the client saw the reply.
-            await ReleaseAsync(conn).ConfigureAwait(false);
+            // Only when WE hold it: releasing an applock we were refused fails, which would log a false Warning
+            // and clear the shared pool on every skipped interval.
+            if (acquired) await ReleaseAsync(conn).ConfigureAwait(false);
         }
     }
 
@@ -121,7 +138,7 @@ internal sealed class SqlServerHistorianRetention : IHistorianRetention
             // applock); clearing the pool makes the dispose physically close it, which releases the lock.
             _log.LogWarning("sp_releaseapplock failed ({Exception}); discarding the connection pool so the session " +
                 "holding the retention applock is closed rather than pooled.", ex.GetType().Name);
-            SqlConnection.ClearPool(conn);
+            ClearPool(conn);
         }
     }
 }
diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.SqlServer.IntegrationTests/RetentionTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.SqlServer.IntegrationTests/RetentionTests.cs
index 85960bd4..05c29da5 100644
--- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.SqlServer.IntegrationTests/RetentionTests.cs
+++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.SqlServer.IntegrationTests/RetentionTests.cs
@@ -171,4 +171,46 @@ public sealed class RetentionTests(SqlHistorianFixture fx)
         await Should.ThrowAsync<InvalidOperationException>(() => purger.PurgeAsync(CancellationToken.None));
         (await LockIsFreeAsync(s)).ShouldBeTrue();
     }
+
+    private sealed class CapturingLogger : Microsoft.Extensions.Logging.ILogger
+    {
+        public readonly List<Microsoft.Extensions.Logging.LogLevel> Levels = [];
+        public IDisposable? BeginScope<TState>(TState state) where TState : notnull => null;
+        public bool IsEnabled(Microsoft.Extensions.Logging.LogLevel logLevel) => true;
+        public void Log<TState>(Microsoft.Extensions.Logging.LogLevel logLevel, Microsoft.Extensions.Logging.EventId eventId,
+            TState state, Exception? exception, Func<TState, Exception?, string> formatter)
+        {
+            lock (Levels) Levels.Add(logLevel);
+        }
+    }
+
+    [Fact]
+    public async Task A_skipped_pass_logs_no_warning_and_does_not_clear_the_pool()
+    {
+        fx.SkipIfUnavailable();
+        var (c, b, _, s) = Make();
+        await SeedAsync(b, s);
+
+        await using var holder = new SqlConnection(fx.ConnectionString);
+        await holder.OpenAsync();
+        await using (var cmd = holder.CreateCommand())
+        {
+            cmd.CommandText = $"EXEC sp_getapplock @Resource = N'OtOpcUa.Historian.Retention.{s}', @LockMode = 'Exclusive', @LockOwner = 'Session', @LockTimeout = 0;";
+            await cmd.ExecuteNonQueryAsync();
+        }
+
+        var log = new CapturingLogger();
+        var clears = 0;
+        var purger = new SqlServerHistorianRetention(c, b, TimeSpan.FromHours(1), 30, 4000, log, new FixedTime(Now))
+        {
+            ClearPool = _ => Interlocked.Increment(ref clears),
+        };
+
+        for (var i = 0; i < 3; i++) (await purger.PurgeAsync(CancellationToken.None)).ShouldBeEmpty();
+
+        lock (log.Levels) log.Levels.ShouldNotContain(l => l >= Microsoft.Extensions.Logging.LogLevel.Warning);
+        clears.ShouldBe(0);
+        // The shared pool is still healthy.
+        (await CountAsync($"SELECT COUNT(*) FROM [{s}].Sample")).ShouldBe(21);
+    }
 }