fix(site-runtime): reject invalid runtime actor names at the site gate and never let ActorOf escalate to the Deployment Manager (review 2026-10-21 01/SR-C1)

An instance unique name Akka refuses as a path element (space, '#', '/',
non-ASCII) made Context.ActorOf throw on the singleton's own thread; the
default Restart stopped every Instance Actor on the site. DeployCompileValidator
now checks the instance + script/alarm/native-source actor names with
ActorPath.IsValidPathElement (the Commons ActorNameRules mirror is stricter
than Akka for " ( ), pinned by a parity sweep), CreateInstanceActor catches
InvalidActorNameException and every caller reports it, and the Instance Actor
skips a refused child name like a compile failure.
This commit is contained in:
Joseph Doherty
2026-10-22 06:24:28 -04:00
parent b5cebf9fa6
commit f5090fcd9e
8 changed files with 606 additions and 23 deletions
@@ -0,0 +1,199 @@
using System.Text.Json;
using Akka.Actor;
using Akka.TestKit.Xunit2;
using Microsoft.Extensions.Logging.Abstractions;
using ZB.MOM.WW.ScadaBridge.Commons.Messages.Deployment;
using ZB.MOM.WW.ScadaBridge.Commons.Messages.Lifecycle;
using ZB.MOM.WW.ScadaBridge.Commons.Messages.ScriptExecution;
using ZB.MOM.WW.ScadaBridge.Commons.Types.Enums;
using ZB.MOM.WW.ScadaBridge.Commons.Types.Flattening;
using ZB.MOM.WW.ScadaBridge.SiteRuntime.Actors;
using ZB.MOM.WW.ScadaBridge.SiteRuntime.Persistence;
using ZB.MOM.WW.ScadaBridge.SiteRuntime.Scripts;
using ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests.TestSupport;
using ZB.MOM.WW.ScadaBridge.TestSupport;
namespace ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests.Actors;
/// <summary>
/// Review 2026-10-21 01/SR-C1: an instance (or script/alarm/native-source) name that is not a
/// legal Akka path element used to make <c>Context.ActorOf</c> throw on the Deployment Manager
/// singleton's own thread. The default Restart directive then stopped EVERY Instance Actor on the
/// site and dropped all in-flight deploy bookkeeping, while the deployer's Ask hung to timeout.
/// </summary>
public class DeploymentManagerActorNameTests : TestKit, IDisposable
{
private readonly SiteStorageService _storage;
private readonly ScriptCompilationService _compilationService;
private readonly SharedScriptLibrary _sharedScriptLibrary;
private readonly TestLocalDb _localDb;
/// <summary>Creates a temporary LocalDb-backed site store per test.</summary>
public DeploymentManagerActorNameTests()
{
_localDb = TestLocalDb.CreateTemp("dm-actor-name-test");
_storage = new SiteStorageService(_localDb.Db, NullLogger<SiteStorageService>.Instance);
_storage.InitializeAsync().GetAwaiter().GetResult();
_compilationService = new ScriptCompilationService(NullLogger<ScriptCompilationService>.Instance);
_sharedScriptLibrary = new SharedScriptLibrary(
_compilationService, NullLogger<SharedScriptLibrary>.Instance);
}
/// <inheritdoc />
void IDisposable.Dispose()
{
Shutdown();
var path = _localDb.Path;
_localDb.Dispose();
TestLocalDb.DeleteFiles(path);
}
private IActorRef CreateBootedDeploymentManager()
{
var health = new DeploymentManagerStartupProbe();
var dm = ActorOf(Props.Create(() => new DeploymentManagerActor(
_storage,
_compilationService,
_sharedScriptLibrary,
null,
new SiteRuntimeOptions { InstanceSweepIntervalSeconds = 0, StartupBatchDelayMs = 10 },
NullLogger<DeploymentManagerActor>.Instance,
dclManager: null,
healthCollector: health,
serviceProvider: null,
loggerFactory: null,
configFetcher: null)));
AwaitCondition(() => health.ReportCount > 0, TimeSpan.FromSeconds(20), TimeSpan.FromMilliseconds(25));
return dm;
}
private static string ConfigJson(string instanceName, string? scriptName = null) =>
JsonSerializer.Serialize(new FlattenedConfiguration
{
InstanceUniqueName = instanceName,
Attributes = [new ResolvedAttribute { CanonicalName = "A", Value = "1", DataType = "Int32" }],
Scripts = scriptName == null
? []
: [new ResolvedScript { CanonicalName = scriptName, Code = "return 1;" }]
});
private DeploymentStatusResponse Deploy(IActorRef dm, string instanceName, string? scriptName = null)
{
dm.Tell(new DeployInstanceCommand(
$"dep-{Guid.NewGuid():N}", instanceName, "rev-1", ConfigJson(instanceName, scriptName),
"tester", DateTimeOffset.UtcNow));
return ExpectMsg<DeploymentStatusResponse>(TimeSpan.FromSeconds(20));
}
private IActorRef ResolveChild(IActorRef dm, string name) =>
Sys.ActorSelection(dm.Path / name).ResolveOne(TimeSpan.FromSeconds(5)).GetAwaiter().GetResult();
/// <summary>
/// The regression the finding asks for: deploying <c>"a b"</c> fails with a readable reason and
/// does NOT stop the instances already running on the site.
/// </summary>
/// <returns>A task that represents the asynchronous operation.</returns>
[Fact]
public async Task Deploy_InvalidInstanceName_FailsCleanly_AndLeavesOtherInstancesRunning()
{
var dm = CreateBootedDeploymentManager();
Assert.Equal(DeploymentStatus.Success, Deploy(dm, "Good1").Status);
var good = ResolveChild(dm, "Good1");
var watcher = CreateTestProbe();
watcher.Watch(good);
var rejected = Deploy(dm, "a b");
Assert.Equal(DeploymentStatus.Failed, rejected.Status);
Assert.Contains("a b", rejected.ErrorMessage);
Assert.Contains("runtime actor name", rejected.ErrorMessage);
watcher.ExpectNoMsg(TimeSpan.FromSeconds(1));
// The manager is still the same live incarnation and keeps serving deploys.
Assert.Equal(DeploymentStatus.Success, Deploy(dm, "Good2").Status);
Assert.Null(await _storage.GetDeployedConfigAsync("a b"));
}
/// <summary>A script whose canonical name Akka refuses is a readable gate rejection, not an S6 init failure.</summary>
/// <returns>A task that represents the asynchronous operation.</returns>
[Fact]
public async Task Deploy_InvalidScriptName_IsRejectedByTheGateWithTheName()
{
var dm = CreateBootedDeploymentManager();
var rejected = Deploy(dm, "Pump1", scriptName: "My Script");
Assert.Equal(DeploymentStatus.Failed, rejected.Status);
Assert.Contains("My Script", rejected.ErrorMessage);
Assert.Null(await _storage.GetDeployedConfigAsync("Pump1"));
}
/// <summary>
/// Enable bypasses the deploy gate, so a row stored before names were checked reaches
/// <c>CreateInstanceActor</c> directly — the refused name must come back as a failed enable.
/// </summary>
/// <returns>A task that represents the asynchronous operation.</returns>
[Fact]
public async Task Enable_StoredRowWithInvalidName_FailsWithoutRestartingTheManager()
{
await _storage.StoreDeployedConfigAsync("a b", ConfigJson("a b"), "d0", "r0", isEnabled: false);
var dm = CreateBootedDeploymentManager();
Assert.Equal(DeploymentStatus.Success, Deploy(dm, "Good1").Status);
var watcher = CreateTestProbe();
watcher.Watch(ResolveChild(dm, "Good1"));
dm.Tell(new EnableInstanceCommand("cmd-1", "a b", DateTimeOffset.UtcNow));
var response = ExpectMsg<InstanceLifecycleResponse>(TimeSpan.FromSeconds(10));
Assert.False(response.Success);
Assert.Contains("a b", response.ErrorMessage);
watcher.ExpectNoMsg(TimeSpan.FromSeconds(1));
}
/// <summary>A stored bad-name row at boot is skipped; its siblings start and stay up.</summary>
/// <returns>A task that represents the asynchronous operation.</returns>
[Fact]
public async Task Boot_StoredRowWithInvalidName_DoesNotTakeDownItsSiblings()
{
await _storage.StoreDeployedConfigAsync("a b", ConfigJson("a b"), "d0", "r0", isEnabled: true);
await _storage.StoreDeployedConfigAsync("Good1", ConfigJson("Good1"), "d1", "r1", isEnabled: true);
var dm = CreateBootedDeploymentManager();
IActorRef? good = null;
AwaitAssert(() => good = ResolveChild(dm, "Good1"), TimeSpan.FromSeconds(10));
var watcher = CreateTestProbe();
watcher.Watch(good!);
watcher.ExpectNoMsg(TimeSpan.FromSeconds(1));
Assert.Equal(DeploymentStatus.Success, Deploy(dm, "Good2").Status);
}
/// <summary>
/// The Instance Actor half: a stored config whose script name Akka refuses skips that one
/// script (logged) instead of failing the whole instance's PreStart.
/// </summary>
[Fact]
public void InstanceActor_InvalidChildName_SkipsThatChild_AndKeepsTheRest()
{
var config = new FlattenedConfiguration
{
InstanceUniqueName = "Pump1",
Attributes = [new ResolvedAttribute { CanonicalName = "A", Value = "1", DataType = "Int32" }],
Scripts =
[
new ResolvedScript { CanonicalName = "bad name", Code = "return 1;" },
new ResolvedScript { CanonicalName = "Good", Code = "return 7;" }
]
};
var instance = ActorOf(Props.Create(() => new InstanceActor(
"Pump1", JsonSerializer.Serialize(config), _storage, _compilationService,
_sharedScriptLibrary, null, new SiteRuntimeOptions(), NullLogger<InstanceActor>.Instance)));
instance.Tell(new ScriptCallRequest("Good", null, 0, "corr-1"));
var result = ExpectMsg<ScriptCallResult>(TimeSpan.FromSeconds(15));
Assert.True(result.Success, result.ErrorMessage);
}
}
@@ -72,11 +72,16 @@ public class DeploymentManagerRedeployTests : TestKit, IDisposable
/// <summary>
/// Builds a config that deserializes and compiles cleanly (so it passes the deploy
/// compile gate) but whose script's canonical name contains a space — illegal as an
/// Akka actor name, so <c>Context.ActorOf(props, "script-dies on init")</c> throws
/// compile gate) but carries two scripts with the SAME canonical name, so the second
/// <c>Context.ActorOf(props, "script-dup")</c> throws a not-unique
/// <c>InvalidActorNameException</c> during the Instance Actor's PreStart. The failure
/// surfaces as an <c>ActorInitializationException</c> the DM decider stops — the exact
/// "Instance Actor dies during init" S6 scenario.
/// <para>
/// This used an illegal (space-containing) script name until review 2026-10-21 01/SR-C1:
/// the gate now rejects such names up front and the Instance Actor skips a refused child
/// name, so a name with a space no longer reaches PreStart as a throw.
/// </para>
/// </summary>
private static string CtorThrowingConfig(string instanceName) =>
JsonSerializer.Serialize(new FlattenedConfiguration
@@ -88,7 +93,8 @@ public class DeploymentManagerRedeployTests : TestKit, IDisposable
],
Scripts =
[
new ResolvedScript { CanonicalName = "dies on init", Code = "return 1;" }
new ResolvedScript { CanonicalName = "dup", Code = "return 1;" },
new ResolvedScript { CanonicalName = "dup", Code = "return 2;" }
]
});
@@ -382,7 +388,7 @@ public class DeploymentManagerRedeployTests : TestKit, IDisposable
public async Task InstanceActorInitFailure_OnStartup_EvictsDeadRefAndLogsSiteEvent()
{
// A stored config that compiles cleanly but whose Instance Actor cannot
// initialize (illegal child actor name → ActorInitializationException → Stop).
// initialize (duplicate child actor name → ActorInitializationException → Stop).
// The DM watches every Instance Actor, so on the unexpected Termination it must
// evict the dead ref from the routing map and log a `deployment` Error event
// rather than leaving a routing entry that points at a stopped actor (S6).
@@ -0,0 +1,154 @@
using System.Text;
using Akka.Actor;
using ZB.MOM.WW.ScadaBridge.Commons.Validation;
namespace ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests.Deployment;
/// <summary>
/// Pins <see cref="ActorNameRules"/> (Commons, which cannot reference Akka) to the rule it
/// mirrors, <see cref="ActorPath.IsValidPathElement"/>. The site deploy gate rejects names
/// through <see cref="ActorNameRules"/> on the strength of this equivalence (review 2026-10-21
/// 01/SR-C1): a name the mirror accepts but Akka refuses would slip past the gate and fail at
/// <c>Context.ActorOf</c>; a name the mirror refuses but Akka accepts would block a deploy that
/// would have worked. Run on this test project because it is the nearest one referencing both.
/// </summary>
public class ActorNameRulesParityTests
{
/// <summary>Printable ASCII 0x20–0x7E.</summary>
private static IEnumerable<char> PrintableAscii()
{
for (var c = (char)0x20; c <= (char)0x7E; c++)
yield return c;
}
/// <summary>
/// Characters Akka.NET 1.5 accepts in a path element that <see cref="ActorNameRules"/> does
/// not (found by this sweep, 2026-10-22). The mirror is STRICTER here — it can only refuse a
/// name Akka would have created, never admit one Akka refuses — which is why the site deploy
/// gate (<c>DeployCompileValidator</c>) decides with <see cref="ActorPath.IsValidPathElement"/>
/// itself rather than the mirror. Reported for the Commons owner to widen the mirror; once
/// it does, this set simply stops matching anything and the test keeps passing.
/// </summary>
private const string KnownMirrorStricterChars = "\"()";
private static void AssertParity(string candidate, List<string> disagreements)
{
var mirror = ActorNameRules.IsValid(candidate);
var akka = ActorPath.IsValidPathElement(candidate);
if (mirror != akka && !IsKnownMirrorStricterGap(candidate, mirror, akka))
{
disagreements.Add(
$"\"{Escape(candidate)}\": ActorNameRules={mirror}, Akka={akka}");
}
}
/// <summary>
/// True when the disagreement is exactly the documented gap: the mirror refuses, Akka
/// accepts, and replacing every <see cref="KnownMirrorStricterChars"/> character with a
/// letter makes the two agree again.
/// </summary>
private static bool IsKnownMirrorStricterGap(string candidate, bool mirror, bool akka)
{
if (mirror || !akka || candidate.IndexOfAny(KnownMirrorStricterChars.ToCharArray()) < 0)
return false;
var patched = new string(candidate
.Select(c => KnownMirrorStricterChars.Contains(c) ? 'a' : c).ToArray());
return ActorNameRules.IsValid(patched) == ActorPath.IsValidPathElement(patched);
}
/// <summary>
/// Akka accepts every known-gap character, so a name such as <c>Pump(A)</c> is deployable —
/// which is why the site gate must not decide with the stricter mirror.
/// </summary>
[Theory]
[InlineData("\"")]
[InlineData("(")]
[InlineData(")")]
[InlineData("Pump(A)")]
public void KnownGapCharacters_AreAcceptedByAkka(string candidate)
{
Assert.True(ActorPath.IsValidPathElement(candidate));
}
private static string Escape(string s)
{
var sb = new StringBuilder();
foreach (var c in s)
sb.Append(c is < (char)0x20 or > (char)0x7E ? $"\\u{(int)c:X4}" : c.ToString());
return sb.ToString();
}
/// <summary>Every printable ASCII character alone, and with a letter on either side.</summary>
[Fact]
public void SingleCharacters_AgreeWithAkka()
{
var disagreements = new List<string>();
foreach (var c in PrintableAscii())
{
AssertParity(c.ToString(), disagreements);
AssertParity("a" + c, disagreements);
AssertParity(c + "a", disagreements);
}
Assert.True(disagreements.Count == 0, string.Join(Environment.NewLine, disagreements));
}
/// <summary>Every ordered pair of printable ASCII characters (95 × 95).</summary>
[Fact]
public void CharacterPairs_AgreeWithAkka()
{
var disagreements = new List<string>();
foreach (var a in PrintableAscii())
foreach (var b in PrintableAscii())
AssertParity(new string([a, b]), disagreements);
Assert.True(disagreements.Count == 0, string.Join(Environment.NewLine, disagreements));
}
/// <summary>Percent-escapes, leading '$', empty/null-ish and non-ASCII samples.</summary>
[Theory]
[InlineData("")]
[InlineData("%41")]
[InlineData("a%41b")]
[InlineData("%4")]
[InlineData("%")]
[InlineData("a%")]
[InlineData("%%41")]
[InlineData("%G1")]
[InlineData("%1G")]
[InlineData("%aF")]
[InlineData("%41%42")]
[InlineData("$")]
[InlineData("$a")]
[InlineData("a$")]
[InlineData("a$b")]
[InlineData("-a")]
[InlineData("Furnace #3")]
[InlineData("Line 1")]
[InlineData("a b")]
[InlineData("Ofen Süd")]
[InlineData("Ünit")]
[InlineData("名前")]
[InlineData("é")]
[InlineData("a\tb")]
[InlineData("a\nb")]
[InlineData("script-Module.Member")]
[InlineData("alarm-HiTemp")]
[InlineData("native-alarm-Conn.Source")]
[InlineData("Pump_01;x'y~z!")]
public void Samples_AgreeWithAkka(string candidate)
{
var disagreements = new List<string>();
AssertParity(candidate, disagreements);
Assert.True(disagreements.Count == 0, string.Join(Environment.NewLine, disagreements));
}
/// <summary>Null is invalid on both sides of the mirror.</summary>
[Fact]
public void Null_IsInvalid()
{
Assert.False(ActorNameRules.IsValid(null));
Assert.NotNull(ActorNameRules.Describe(null));
}
}
@@ -104,4 +104,43 @@ public class DeployCompileValidatorTests
var errors = NewValidator().Validate(cfg);
Assert.Contains(errors, e => e.Contains("A1") && e.Contains("DoesNotExist"));
}
/// <summary>
/// Review 2026-10-21 01/SR-C1: names that become Akka actor names are checked by the gate,
/// with Akka's own rule (so <c>Pump(A)</c>, which Akka accepts, is not rejected).
/// </summary>
[Theory]
[InlineData("a b", true)]
[InlineData("Furnace #3", true)]
[InlineData("Ofen Süd", true)]
[InlineData("$Pump", true)]
[InlineData("Pump(A)", false)]
[InlineData("Line-1_A.B", false)]
public void InstanceName_IsCheckedAsAnActorName(string instanceName, bool rejected)
{
var errors = NewValidator().Validate(ConfigWith("return 1;"), instanceName);
Assert.Equal(rejected, errors.Any(DeployCompileValidator.IsActorNameError));
if (rejected)
Assert.Contains(errors, e => e.Contains(instanceName));
}
/// <summary>Script, alarm and native-source canonical names are checked too.</summary>
[Fact]
public void ChildNames_AreCheckedAsActorNames()
{
var cfg = JsonSerializer.Serialize(new FlattenedConfiguration
{
Scripts = [new ResolvedScript { CanonicalName = "My Script", Code = "return 1;", TriggerType = "Call" }],
Alarms = [new ResolvedAlarm { CanonicalName = "Hi/Temp", TriggerType = "ValueMatch" }],
NativeAlarmSources = [new ResolvedNativeAlarmSource { CanonicalName = "Src#1", ConnectionName = "c" }],
Attributes = []
});
var nameErrors = NewValidator().Validate(cfg).Where(DeployCompileValidator.IsActorNameError).ToList();
Assert.Equal(3, nameErrors.Count);
Assert.Contains(nameErrors, e => e.Contains("My Script"));
Assert.Contains(nameErrors, e => e.Contains("Hi/Temp"));
Assert.Contains(nameErrors, e => e.Contains("Src#1"));
}
}