fix(runtime): explicit 30s command timeout on the Direct-apply artifact read

2026-08-17 03:49:27 -04:00
parent 6ee553cf8f
commit dba5f806d3
2 changed files with 162 additions and 0 deletions
@@ -2093,6 +2093,35 @@ public sealed class DriverHostActor : ReceiveActor, IWithTimers, IWithUnboundedS
        try
        {
            using var db = _dbFactory.CreateDbContext();
+
+            // Bound this read EXPLICITLY. It runs off the mailbox on a pool thread with no
+            // CancellationToken (BeginDirectApply's Task.Run), so the actor itself cannot be wedged by a
+            // hung central-SQL read — but the read would pin a pool thread for as long as SQL takes to
+            // answer. Until now the only bound was SqlClient's IMPLICIT 30 s command-timeout default:
+            // nothing on the ConfigDb path sets one (AddPooledDbContextFactory uses a plain
+            // UseSqlServer(connectionString), and no connection string carries "Command Timeout="), so a
+            // future connection-string edit or provider-default change could silently unbound it. Stating
+            // it here pins the bound to this call site.
+            //
+            // Deliberately LOCAL to this lease, not on the pooled factory's options: re-timing the global
+            // factory would re-time every ConfigDb consumer — migrations and long admin queries included.
+            // Verified against EF Core 10.0.7 (pool size 1, same context instance re-leased):
+            // Database.SetCommandTimeout does NOT survive the return to the pool — RelationalConnection
+            // resets it as an IResettableService — so the next lease sees the default again and there is
+            // no pool pollution to undo.
+            //
+            // Deliberately NOT a CancellationToken: threading one would mean making the FirstOrDefault
+            // below async, and the only caller-side token is actor lifetime — the timeout gives the same
+            // bound without touching the actor seam. A timeout surfaces as an exception through this
+            // method's existing catch-to-null, so the apply FAILS and the revision does not advance,
+            // keeping last-known-good served (the #485-family "no answer, never tear down" semantics —
+            // already correct, unchanged here).
+            //
+            // Guarded on IsRelational: the timeout is a relational-only concept and the extension throws
+            // on a non-relational provider (the in-memory provider the actor tests run against).
+            if (db.Database.IsRelational())
+                db.Database.SetCommandTimeout(TimeSpan.FromSeconds(30));
+
            return db.Deployments.AsNoTracking()
                .Where(d => d.DeploymentId == deploymentId.Value)
                .Select(d => d.ArtifactBlob)
@@ -0,0 +1,133 @@
+using System.Text.Json;
+using Akka.Actor;
+using Microsoft.EntityFrameworkCore;
+using Shouldly;
+using Xunit;
+using ZB.MOM.WW.OtOpcUa.Commons.Interfaces;
+using ZB.MOM.WW.OtOpcUa.Commons.Messages.Deploy;
+using ZB.MOM.WW.OtOpcUa.Commons.Messages.Fleet;
+using ZB.MOM.WW.OtOpcUa.Commons.Types;
+using ZB.MOM.WW.OtOpcUa.Configuration;
+using ZB.MOM.WW.OtOpcUa.Configuration.Entities;
+using ZB.MOM.WW.OtOpcUa.Configuration.Enums;
+using ZB.MOM.WW.OtOpcUa.Runtime.Drivers;
+using ZB.MOM.WW.OtOpcUa.Runtime.Tests.Harness;
+
+namespace ZB.MOM.WW.OtOpcUa.Runtime.Tests.Drivers;
+
+/// <summary>
+///     The failure path the <c>Direct</c>-apply artifact read's explicit 30 s command timeout relies on: a
+///     read that THROWS is "no answer", so the apply fails and the revision does not advance.
+/// </summary>
+/// <remarks>
+///     <para>
+///         <b>A pin, not red-first.</b> The behaviour predates this class — <c>LoadArtifactBlob</c> has
+///         always caught its own exceptions and returned <see langword="null"/>, which
+///         <c>HandleArtifactForApply</c> turns into a Failed ACK. What was missing was a test saying so, and
+///         a timed-out command is exactly this shape (a <c>SqlException</c> out of the EF read), so the
+///         timeout's whole safety argument rests on it: the node keeps serving last-known-good and the same
+///         revision can be dispatched again (the #485/#486 family).
+///     </para>
+///     <para>
+///         Sibling coverage reaches the same verdict through EMPTY bytes rather than a throw
+///         (<c>DriverHostActorUnreadableArtifactTests</c>), which is the missing-row shape, not the
+///         read-failed shape — the two arrive at <c>ArtifactForApply</c> by different routes.
+///     </para>
+/// </remarks>
+public sealed class DriverHostActorDirectApplyReadFailureTests : RuntimeActorTestBase
+{
+    private static readonly NodeId TestNode = NodeId.Parse("driver-test");
+    private static readonly RevisionHash RevA = RevisionHash.Parse(new string('a', 64));
+    private static readonly TimeSpan Timeout = TimeSpan.FromSeconds(10);
+
+    [Fact]
+    public void A_direct_artifact_read_that_throws_fails_the_apply_and_leaves_the_revision_alone()
+    {
+        var inner = NewInMemoryDbFactory();
+        var db = new FailingReadConfigDbFactory(inner);
+        var deploymentId = SeedDeployment(inner, RevA);
+
+        var coordinator = CreateTestProbe();
+        var actor = Sys.ActorOf(DriverHostActor.Props(
+            db, TestNode, coordinator.Ref,
+            localRoles: new HashSet<string> { "driver" }));
+
+        // Barrier: an answered GetDiagnostics proves PreStart (and its bootstrap ConfigDb read) is done,
+        // so the allowance below counts only the apply's own contexts.
+        actor.Tell(new GetDiagnostics(CorrelationId.NewId()), coordinator.Ref);
+        coordinator.ExpectMsg<NodeDiagnosticsSnapshot>(Timeout);
+
+        // Let ONE more context through — the apply's Applying-row upsert — and fail the NEXT one, which is
+        // the artifact read. Both misconfigurations fail LOUDLY rather than passing vacuously: too small
+        // fails the upsert instead (which swallows its own exceptions), so the read succeeds and the ACK is
+        // Applied; too large never fails the read at all, same Applied ACK.
+        db.FailAfter(allowThrough: 1);
+
+        actor.Tell(new DispatchDeployment(deploymentId, RevA, CorrelationId.NewId()));
+
+        var ack = coordinator.ExpectMsg<ApplyAck>(Timeout);
+        ack.Outcome.ShouldBe(ApplyAckOutcome.Failed);
+        ack.FailureReason.ShouldNotBeNullOrWhiteSpace();
+        db.Throws.ShouldBe(1);   // the failure came from the read, not from somewhere incidental
+
+        // Never applied ⇒ never claimed: the node stays on what it was already serving.
+        actor.Tell(new GetDiagnostics(CorrelationId.NewId()), coordinator.Ref);
+        coordinator.ExpectMsg<NodeDiagnosticsSnapshot>(Timeout).CurrentRevision.ShouldNotBe(RevA);
+
+        // …and because the revision did not advance, re-dispatching THE SAME revision against a healthy DB
+        // is not waved through by the "already at this rev" short-circuit — it actually applies.
+        actor.Tell(new DispatchDeployment(deploymentId, RevA, CorrelationId.NewId()));
+        coordinator.ExpectMsg<ApplyAck>(Timeout).Outcome.ShouldBe(ApplyAckOutcome.Applied);
+
+        actor.Tell(new GetDiagnostics(CorrelationId.NewId()), coordinator.Ref);
+        coordinator.ExpectMsg<NodeDiagnosticsSnapshot>(Timeout).CurrentRevision.ShouldBe(RevA);
+    }
+
+    /// <summary>Seeds a sealed, parseable deployment and returns its id.</summary>
+    private static DeploymentId SeedDeployment(IDbContextFactory<OtOpcUaConfigDbContext> db, RevisionHash rev)
+    {
+        var artifact = JsonSerializer.SerializeToUtf8Bytes(new { DriverInstances = Array.Empty<object>() });
+        var id = DeploymentId.NewId();
+        using var ctx = db.CreateDbContext();
+        ctx.Deployments.Add(new Deployment
+        {
+            DeploymentId = id.Value,
+            RevisionHash = rev.Value,
+            Status = DeploymentStatus.Sealed,
+            CreatedBy = "test",
+            SealedAtUtc = DateTime.UtcNow,
+            ArtifactBlob = artifact,
+        });
+        ctx.SaveChanges();
+        return id;
+    }
+
+    /// <summary>
+    ///     A ConfigDb factory that throws for exactly ONE armed context — the fake shape of a central-SQL
+    ///     read that times out. It throws once and then behaves normally, so the recovery leg of the test
+    ///     runs against a healthy DB without re-wiring the actor.
+    /// </summary>
+    private sealed class FailingReadConfigDbFactory(IDbContextFactory<OtOpcUaConfigDbContext> inner)
+        : IDbContextFactory<OtOpcUaConfigDbContext>
+    {
+        private int _allowance = int.MaxValue;
+        private int _throws;
+
+        /// <summary>How many contexts have been failed.</summary>
+        public int Throws => Volatile.Read(ref _throws);
+
+        /// <summary>Arms a single throw, after letting <paramref name="allowThrough"/> more contexts past.</summary>
+        public void FailAfter(int allowThrough) => Volatile.Write(ref _allowance, allowThrough);
+
+        public OtOpcUaConfigDbContext CreateDbContext()
+        {
+            if (Interlocked.Decrement(ref _allowance) == -1)
+            {
+                Interlocked.Increment(ref _throws);
+                throw new InvalidOperationException("central SQL command timed out (test stub)");
+            }
+
+            return inner.CreateDbContext();
+        }
+    }
+}