From dba5f806d3ba73cd5da668dd332057e6a30503f5 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 17 Aug 2026 03:49:27 -0400 Subject: [PATCH] fix(runtime): explicit 30s command timeout on the Direct-apply artifact read --- .../Drivers/DriverHostActor.cs | 29 ++++ ...verHostActorDirectApplyReadFailureTests.cs | 133 ++++++++++++++++++ 2 files changed, 162 insertions(+) create mode 100644 tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverHostActorDirectApplyReadFailureTests.cs diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverHostActor.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverHostActor.cs index 9c5417c5..af024a99 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverHostActor.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverHostActor.cs @@ -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) diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverHostActorDirectApplyReadFailureTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverHostActorDirectApplyReadFailureTests.cs new file mode 100644 index 00000000..a0ab5f99 --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverHostActorDirectApplyReadFailureTests.cs @@ -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; + +/// +/// The failure path the Direct-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. +/// +/// +/// +/// A pin, not red-first. The behaviour predates this class — LoadArtifactBlob has +/// always caught its own exceptions and returned , which +/// HandleArtifactForApply turns into a Failed ACK. What was missing was a test saying so, and +/// a timed-out command is exactly this shape (a SqlException 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). +/// +/// +/// Sibling coverage reaches the same verdict through EMPTY bytes rather than a throw +/// (DriverHostActorUnreadableArtifactTests), which is the missing-row shape, not the +/// read-failed shape — the two arrive at ArtifactForApply by different routes. +/// +/// +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 { "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(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(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(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(Timeout).Outcome.ShouldBe(ApplyAckOutcome.Applied); + + actor.Tell(new GetDiagnostics(CorrelationId.NewId()), coordinator.Ref); + coordinator.ExpectMsg(Timeout).CurrentRevision.ShouldBe(RevA); + } + + /// Seeds a sealed, parseable deployment and returns its id. + private static DeploymentId SeedDeployment(IDbContextFactory db, RevisionHash rev) + { + var artifact = JsonSerializer.SerializeToUtf8Bytes(new { DriverInstances = Array.Empty() }); + 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; + } + + /// + /// 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. + /// + private sealed class FailingReadConfigDbFactory(IDbContextFactory inner) + : IDbContextFactory + { + private int _allowance = int.MaxValue; + private int _throws; + + /// How many contexts have been failed. + public int Throws => Volatile.Read(ref _throws); + + /// Arms a single throw, after letting more contexts past. + 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(); + } + } +}