diff --git a/docs/requirements/Component-SiteRuntime.md b/docs/requirements/Component-SiteRuntime.md index 45670d53..7b6345c5 100644 --- a/docs/requirements/Component-SiteRuntime.md +++ b/docs/requirements/Component-SiteRuntime.md @@ -158,6 +158,8 @@ flowchart TD > **Dead-child bookkeeping + two-signal deploy join (S6).** All Instance Actors are `Watch`ed; an unexpected `Terminated` evicts the actor ref from the singleton's map. **Deviation from the naive design (2026-07-09, see master-tracker "Plan deviations during Wave 1 PLAN-03 execution"):** deploy-success is **NOT** reported on persistence alone — the config store can commit before the Instance Actor's asynchronous `PreStart` throws. The design therefore uses a **two-signal join**: the Instance Actor sends a new `InstanceActorInitialized` readiness message at the end of a successful `PreStart`, and the Deployment Manager reports `Success` only once **both** persistence has committed **and** that readiness signal has arrived. An actor that dies during init never signals readiness → the `Terminated` fallback replies `Failed` and rolls the optimistic in-memory state back (the persisted-row rollback is deferred until the store commits so it cannot race the write). +> **Runtime names are validated before anything is applied (review 2026-10-21 01/SR-C1).** The instance unique name becomes the Instance Actor's name, and every script / alarm / native-alarm-source canonical name becomes a child actor name (`script-…`, `alarm-…`, `native-alarm-…`). Akka refuses any of these that is not a legal path element (a space, `#`, `/`, `[`, `]`, a bare `%`, a non-ASCII letter, a leading `$`), and the refusal is a **synchronous** `InvalidActorNameException` from `Context.ActorOf` — on the Deployment Manager singleton's own thread for an instance name, where the default Restart directive used to stop **every** Instance Actor on the site, lose all in-flight deploy bookkeeping, and leave the deployer's Ask to time out (so a retry repeated the outage). The site compile gate (`DeployCompileValidator`) now checks all of those names with Akka's own `ActorPath.IsValidPathElement` and rejects the deploy with `Site deploy validation failed: Invalid runtime name: …` before any reset, persistence or actor work. It deliberately does **not** decide with the Commons mirror `ActorNameRules`: the mirror is currently stricter than Akka (it refuses `"`, `(` and `)`), so gating on it would reject names that deploy today. Behind the gate, `CreateInstanceActor` catches the exception and returns a failure that each caller reports — a Failed deploy, a failed enable, an Error site event for a self-heal start, a skipped instance in the boot batch — so a row stored before names were checked can never escalate to the singleton. The Instance Actor applies the same rule to its children: a refused child name is logged and that one child skipped, exactly like a compile failure, instead of failing the whole instance's `PreStart`. + ### System-Wide Artifact Handling - Receives updated shared scripts, external system definitions, database connection definitions, and data connection definitions from central. (Notification lists and SMTP configuration are central-only and are not deployed to sites — see Component-NotificationService.md.) - Stores all artifacts in local SQLite. After artifact deployment, the site is fully self-contained — all runtime configuration is read from local SQLite with no access to the central configuration database. diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs index 796f6305..005dd86b 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs @@ -840,8 +840,9 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers for (var i = startIdx; i < endIdx; i++) { var config = state.Configs[i]; - // CreateInstanceActor ensures this config's DCL connections (register row 85). - CreateInstanceActor(config.InstanceUniqueName, config.ConfigJson, config.RevisionHash); + // CreateInstanceActor ensures this config's DCL connections (register row 85). A + // refused actor name is logged inside and skips just that instance (01/SR-C1). + CreateInstanceActor(config.InstanceUniqueName, config.ConfigJson, config.RevisionHash, out _); } UpdateInstanceCounts(); @@ -1051,17 +1052,24 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers // (SiteScriptCompileCache, owned by the ScriptCompilationService singleton this // actor shares with every Instance Actor) too, and the Instance Actor's PreStart // reuses the gate's compile (N4). - var errors = _deployCompileValidator.Validate(command.FlattenedConfigurationJson); + // + // The gate also rejects any instance/script/alarm/native-source name that is not a legal + // Akka actor name (review 2026-10-21 01/SR-C1) — such a name used to throw out of + // Context.ActorOf on THIS actor's thread and restart the singleton with every instance. + var errors = _deployCompileValidator.Validate( + command.FlattenedConfigurationJson, command.InstanceUniqueName); if (errors.Count > 0) { var instanceName = command.InstanceUniqueName; var joined = string.Join(" | ", errors); + var nameRejected = errors.Any(DeployCompileValidator.IsActorNameError); LogDeploymentEvent("Error", instanceName, - $"Instance {instanceName} deploy rejected — site compile validation failed (deploymentId={command.DeploymentId})", + $"Instance {instanceName} deploy rejected — site {(nameRejected ? "deploy" : "compile")} validation failed (deploymentId={command.DeploymentId})", joined); replyTo.Tell(new DeploymentStatusResponse( command.DeploymentId, instanceName, DeploymentStatus.Failed, - "Script compilation failed on site: " + joined, DateTimeOffset.UtcNow)); + (nameRejected ? "Site deploy validation failed: " : "Script compilation failed on site: ") + joined, + DateTimeOffset.UtcNow)); return; } @@ -1341,6 +1349,14 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers { var instanceName = command.InstanceUniqueName; + // Review 2026-10-21 01/SR-C1, defence in depth behind the compile gate: never dispatch + // the static-state reset or persist a row for a name Context.ActorOf will refuse. + if (!ActorPath.IsValidPathElement(instanceName)) + { + FailDeployForInvalidName(command, sender); + return; + } + // Static overrides and mirrored native alarm state are reset on redeployment per // design decision — the new config may bind different sources, and the source snapshot // re-seeds the mirror on (re)subscribe. Dispatched HERE, BEFORE the successor exists, @@ -1359,9 +1375,20 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers // Create the Instance Actor immediately. CreateInstanceActor first ensures the DCL // connections this config's data-sourced attributes subscribe through (register row 85). - CreateInstanceActor( - instanceName, command.FlattenedConfigurationJson, command.RevisionHash, - staticStateResetGate); + if (!CreateInstanceActor( + instanceName, command.FlattenedConfigurationJson, command.RevisionHash, + out var createError, staticStateResetGate)) + { + // Unreachable while the name pre-check above holds; kept so an ActorOf refusal can + // never escape to the supervisor and restart the singleton. + LogDeploymentEvent("Error", instanceName, + $"Instance {instanceName} deploy failed (deploymentId={command.DeploymentId})", createError); + sender.Tell(new DeploymentStatusResponse( + command.DeploymentId, instanceName, DeploymentStatus.Failed, + createError, DateTimeOffset.UtcNow)); + return; + } + if (!isRedeploy) _deployedInstanceNames.Add(instanceName); UpdateInstanceCounts(); @@ -1415,6 +1442,24 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers }).PipeTo(Self); } + /// + /// Replies for an instance name Akka refuses as an + /// actor name (review 2026-10-21 01/SR-C1). Nothing has been applied when this runs. + /// + private void FailDeployForInvalidName(DeployInstanceCommand command, IActorRef sender) + { + var instanceName = command.InstanceUniqueName; + var error = DescribeInvalidInstanceName(instanceName); + LogDeploymentEvent("Error", instanceName, + $"Instance {instanceName} deploy rejected (deploymentId={command.DeploymentId})", error); + sender.Tell(new DeploymentStatusResponse( + command.DeploymentId, instanceName, DeploymentStatus.Failed, error, DateTimeOffset.UtcNow)); + } + + private static string DescribeInvalidInstanceName(string instanceName) => + $"Instance name '{instanceName}' cannot be used as a runtime actor name on the site " + + "(spaces, '#', '/', '[', ']', '%' and non-ASCII letters are not allowed, nor a leading '$')."; + /// /// Records the persistence half of the deploy-success join. On a persistence /// failure the optimistically-created Instance Actor is stopped and the counter @@ -1661,9 +1706,17 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers return; } - if (!_instanceActors.ContainsKey(instanceName)) + if (!_instanceActors.ContainsKey(instanceName) && + !CreateInstanceActor(instanceName, result.Config.ConfigJson, result.Config.RevisionHash, + out var createError)) { - CreateInstanceActor(instanceName, result.Config.ConfigJson, result.Config.RevisionHash); + // Review 2026-10-21 01/SR-C1: the enable path bypasses the deploy gate, so a stored + // row whose name Akka refuses (deployed before the gate checked names) lands here. + // Report it instead of letting ActorOf restart the singleton. + LogDeploymentEvent("Error", instanceName, $"Instance {instanceName} enable failed", createError); + result.OriginalSender.Tell(new InstanceLifecycleResponse( + result.Command.CommandId, instanceName, false, createError, DateTimeOffset.UtcNow)); + return; } UpdateInstanceCounts(); @@ -1831,7 +1884,15 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers { var instanceName = config.InstanceUniqueName; - CreateInstanceActor(instanceName, config.ConfigJson, config.RevisionHash); + if (!CreateInstanceActor(instanceName, config.ConfigJson, config.RevisionHash, out var createError)) + { + // Review 2026-10-21 01/SR-C1: surfaced, never escalated. The sweep claims one attempt + // per (instance, revision), so this is reported once rather than every tick. + LogDeploymentEvent("Error", instanceName, + $"Instance {instanceName} could not be started by {reason} self-heal", createError); + return; + } + _deployedInstanceNames.Add(instanceName); UpdateInstanceCounts(); @@ -3462,16 +3523,25 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers /// the single creation funnel, so recording it here covers every path that can ever start an /// Instance Actor. /// + /// + /// Why the actor could not be created, or on success (including the + /// already-exists no-op). + /// /// /// The deploy path's static-override / native-alarm reset, dispatched BEFORE this call so the /// new actor's PreStart override load can be ordered after it. /// on every non-deploy start (boot batch, enable, self-heal), where /// nothing is being reset and the load runs immediately as before. /// - internal void CreateInstanceActor( - string instanceName, string configJson, string revisionHash, + /// + /// when Akka refused the actor name (review 2026-10-21 01/SR-C1); + /// nothing was created or recorded. Callers turn that into a Failed reply / site event. + /// + internal bool CreateInstanceActor( + string instanceName, string configJson, string revisionHash, out string? error, Task? staticStateResetGate = null) { + error = null; // Deferred register row 85 — an Instance Actor is useless without the DCL connection // actors its data-sourced attributes subscribe through, so ensuring them belongs in the // single creation funnel rather than at each caller. @@ -3500,7 +3570,7 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers if (_instanceActors.ContainsKey(instanceName)) { _logger.LogWarning("Instance Actor {Instance} already exists, skipping creation", instanceName); - return; + return true; } // Reuse the shared, host-configured logger factory @@ -3520,7 +3590,26 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers null, staticStateResetGate)); - var actorRef = Context.ActorOf(props, instanceName); + // Review 2026-10-21 01/SR-C1: ActorOf throws InvalidActorNameException SYNCHRONOUSLY on + // this actor's thread for a name Akka refuses (space, '#', '/', non-ASCII, ...). Escaping + // the handler it would hit this singleton's own supervisor, whose default Restart stops + // EVERY Instance Actor on the site and drops all in-flight deploy bookkeeping. The deploy + // gate rejects such names up front; this catch covers the paths that bypass it (boot + // load, enable, self-heal of a row stored before the gate checked names). + IActorRef actorRef; + try + { + actorRef = Context.ActorOf(props, instanceName); + } + catch (InvalidActorNameException ex) + { + error = ActorPath.IsValidPathElement(instanceName) + ? $"Instance Actor '{instanceName}' could not be created: {ex.Message}" + : DescribeInvalidInstanceName(instanceName); + _logger.LogError(ex, "Instance Actor {Instance} could not be created", instanceName); + return false; + } + _instanceActors[instanceName] = actorRef; _runningRevisions[instanceName] = revisionHash; // Watch every Instance Actor so an unexpected termination (init failure or @@ -3530,6 +3619,7 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers Context.Watch(actorRef); _logger.LogDebug("Created Instance Actor for {Instance}", instanceName); + return true; } /// diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/InstanceActor.cs b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/InstanceActor.cs index 77c64d2c..96bb7d45 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/InstanceActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/InstanceActor.cs @@ -1943,7 +1943,8 @@ public class InstanceActor : ReceiveActor _healthCollector, _serviceProvider)); - var actorRef = Context.ActorOf(props, $"script-{script.CanonicalName}"); + var actorRef = TryCreateChild(props, $"script-{script.CanonicalName}", "Script", script.CanonicalName); + if (actorRef == null) continue; _scriptActors[script.CanonicalName] = actorRef; RouteChild(actorRef, TriggerRouting.ForScript(script.TriggerType, script.TriggerConfiguration)); } @@ -2001,7 +2002,8 @@ public class InstanceActor : ReceiveActor // Per-script timeout for the alarm on-trigger script. onTriggerTimeoutSeconds)); - var actorRef = Context.ActorOf(props, $"alarm-{alarm.CanonicalName}"); + var actorRef = TryCreateChild(props, $"alarm-{alarm.CanonicalName}", "Alarm", alarm.CanonicalName); + if (actorRef == null) continue; _alarmActors[alarm.CanonicalName] = actorRef; _alarmPriorities[alarm.CanonicalName] = alarm.PriorityLevel; _alarmMesReportable[alarm.CanonicalName] = alarm.IsMesReportable; @@ -2034,7 +2036,9 @@ public class InstanceActor : ReceiveActor nativeKind, _serviceProvider)); - var actorRef = Context.ActorOf(props, $"native-alarm-{nativeSource.CanonicalName}"); + var actorRef = TryCreateChild( + props, $"native-alarm-{nativeSource.CanonicalName}", "Native alarm source", nativeSource.CanonicalName); + if (actorRef == null) continue; _nativeAlarmActors[nativeSource.CanonicalName] = actorRef; // Same kind handed to the NativeAlarmActor — used to stamp the // (cosmetic) kind badge on placeholder rows for quiet bindings. @@ -2046,6 +2050,31 @@ public class InstanceActor : ReceiveActor _instanceUniqueName, _scriptActors.Count, _alarmActors.Count, _nativeAlarmActors.Count); } + /// + /// Creates one child actor, or returns when Akka refuses its name + /// (review 2026-10-21 01/SR-C1). The site deploy gate rejects such names before anything is + /// applied, so this only fires for a config stored before the gate checked names (boot load, + /// enable, self-heal) — and there it is handled like a compile failure: the offending child is + /// logged and skipped, the rest of the instance runs. It used to throw out of + /// and fail the whole instance. + /// + private IActorRef? TryCreateChild(Props props, string actorName, string kind, string canonicalName) + { + try + { + return Context.ActorOf(props, actorName); + } + // Only a name Akka refuses OUTRIGHT is skipped. A duplicate canonical name (also an + // InvalidActorNameException, "not unique") keeps failing PreStart as it always has. + catch (InvalidActorNameException ex) when (!ActorPath.IsValidPathElement(actorName)) + { + _logger.LogError(ex, + "{Kind} '{Name}' on instance '{Instance}' skipped — its name cannot be used as a runtime actor name", + kind, canonicalName, _instanceUniqueName); + return null; + } + } + /// /// Maps a bound connection's protocol to the native alarm kind stamped on /// emitted events. MxAccess Gateway connections yield diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Deployment/DeployCompileValidator.cs b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Deployment/DeployCompileValidator.cs index 3e432233..e4fde7d8 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Deployment/DeployCompileValidator.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Deployment/DeployCompileValidator.cs @@ -1,4 +1,5 @@ using System.Text.Json; +using Akka.Actor; using ZB.MOM.WW.ScadaBridge.Commons.Types.Flattening; using ZB.MOM.WW.ScadaBridge.SiteRuntime.Scripts; @@ -30,6 +31,18 @@ public sealed class DeployCompileValidator public DeployCompileValidator(ScriptCompilationService compilationService) => _compilationService = compilationService; + /// + /// Prefix carried by every runtime-name error, so a caller can tell a name rejection + /// () from a compile failure when wording its reply. + /// + private const string ActorNameErrorPrefix = "Invalid runtime name: "; + + /// True when came from the runtime-name check. + /// One error returned by . + /// for a runtime-name error. + public static bool IsActorNameError(string error) => + error.StartsWith(ActorNameErrorPrefix, StringComparison.Ordinal); + /// /// Validates every compilable artifact in the flattened configuration. /// Returns an empty list when the whole configuration compiles; otherwise @@ -37,8 +50,13 @@ public sealed class DeployCompileValidator /// single error (this also fronts the poison-config case for startup load). /// /// The flattened-configuration JSON to validate. + /// + /// The instance unique name the Deployment Manager will create the Instance Actor under. + /// When supplied it is checked as an actor name along with every script/alarm/native-source + /// child name (review 2026-10-21 01/SR-C1); checks the child names only. + /// /// A list of compile-error messages; empty if the configuration is clean. - public IReadOnlyList Validate(string configJson) + public IReadOnlyList Validate(string configJson, string? instanceName = null) { FlattenedConfiguration? config; try @@ -55,6 +73,10 @@ public sealed class DeployCompileValidator var errors = new List(); + // Runtime names first: they are cheap, and a name Akka refuses would otherwise surface + // as an InvalidActorNameException at Context.ActorOf instead of a readable rejection. + AddActorNameErrors(errors, config, instanceName); + // Scripts: full execution-path compile against ScriptGlobals. foreach (var script in config.Scripts) { @@ -97,6 +119,48 @@ public sealed class DeployCompileValidator return errors; } + /// + /// Review 2026-10-21 01/SR-C1: every name that becomes an Akka actor name on the site — the + /// Instance Actor itself (DeploymentManagerActor.CreateInstanceActor) and its + /// script-/alarm-/native-alarm- children + /// (InstanceActor.CreateChildActors) — must be a legal path element. An instance named + /// "Furnace #3" used to make Context.ActorOf throw on the Deployment Manager + /// singleton's own thread, restarting it and every Instance Actor on the site. + /// + /// + /// Decided with Akka's own , not the Commons mirror + /// ActorNameRules: the mirror is currently STRICTER (it refuses ", ( and + /// ), which Akka accepts), so gating on it would reject deployments that work today. + /// + /// + private static void AddActorNameErrors( + List errors, FlattenedConfiguration config, string? instanceName) + { + if (instanceName != null) + AddActorNameError(errors, "Instance", instanceName, instanceName); + + foreach (var script in config.Scripts) + AddActorNameError(errors, "Script", script.CanonicalName, $"script-{script.CanonicalName}"); + + foreach (var alarm in config.Alarms) + AddActorNameError(errors, "Alarm", alarm.CanonicalName, $"alarm-{alarm.CanonicalName}"); + + foreach (var source in config.NativeAlarmSources) + AddActorNameError(errors, "Native alarm source", source.CanonicalName, + $"native-alarm-{source.CanonicalName}"); + } + + private static void AddActorNameError( + List errors, string kind, string? name, string actorName) + { + if (ActorPath.IsValidPathElement(actorName)) + return; + + errors.Add($"{ActorNameErrorPrefix}{kind} name '{name}' cannot be used as a runtime actor " + + "name — use ASCII letters, digits and -_.*$+:@&=,!~';() only (no spaces, '#', " + + "'/', '[', ']', '%' or non-ASCII letters), and do not start it with '$'."); + } + /// /// Compiles the boolean trigger expression for an Expression-triggered script /// or alarm and records any failure. No-op for non-Expression triggers or a diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DeploymentManagerActorNameTests.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DeploymentManagerActorNameTests.cs new file mode 100644 index 00000000..8e4c707e --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DeploymentManagerActorNameTests.cs @@ -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; + +/// +/// 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 Context.ActorOf 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. +/// +public class DeploymentManagerActorNameTests : TestKit, IDisposable +{ + private readonly SiteStorageService _storage; + private readonly ScriptCompilationService _compilationService; + private readonly SharedScriptLibrary _sharedScriptLibrary; + private readonly TestLocalDb _localDb; + + /// Creates a temporary LocalDb-backed site store per test. + public DeploymentManagerActorNameTests() + { + _localDb = TestLocalDb.CreateTemp("dm-actor-name-test"); + _storage = new SiteStorageService(_localDb.Db, NullLogger.Instance); + _storage.InitializeAsync().GetAwaiter().GetResult(); + _compilationService = new ScriptCompilationService(NullLogger.Instance); + _sharedScriptLibrary = new SharedScriptLibrary( + _compilationService, NullLogger.Instance); + } + + /// + 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.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(TimeSpan.FromSeconds(20)); + } + + private IActorRef ResolveChild(IActorRef dm, string name) => + Sys.ActorSelection(dm.Path / name).ResolveOne(TimeSpan.FromSeconds(5)).GetAwaiter().GetResult(); + + /// + /// The regression the finding asks for: deploying "a b" fails with a readable reason and + /// does NOT stop the instances already running on the site. + /// + /// A task that represents the asynchronous operation. + [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")); + } + + /// A script whose canonical name Akka refuses is a readable gate rejection, not an S6 init failure. + /// A task that represents the asynchronous operation. + [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")); + } + + /// + /// Enable bypasses the deploy gate, so a row stored before names were checked reaches + /// CreateInstanceActor directly — the refused name must come back as a failed enable. + /// + /// A task that represents the asynchronous operation. + [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(TimeSpan.FromSeconds(10)); + + Assert.False(response.Success); + Assert.Contains("a b", response.ErrorMessage); + watcher.ExpectNoMsg(TimeSpan.FromSeconds(1)); + } + + /// A stored bad-name row at boot is skipped; its siblings start and stay up. + /// A task that represents the asynchronous operation. + [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); + } + + /// + /// 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. + /// + [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.Instance))); + + instance.Tell(new ScriptCallRequest("Good", null, 0, "corr-1")); + var result = ExpectMsg(TimeSpan.FromSeconds(15)); + Assert.True(result.Success, result.ErrorMessage); + } +} diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DeploymentManagerRedeployTests.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DeploymentManagerRedeployTests.cs index 8db764a2..28f71cec 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DeploymentManagerRedeployTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DeploymentManagerRedeployTests.cs @@ -72,11 +72,16 @@ public class DeploymentManagerRedeployTests : TestKit, IDisposable /// /// 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 Context.ActorOf(props, "script-dies on init") throws + /// compile gate) but carries two scripts with the SAME canonical name, so the second + /// Context.ActorOf(props, "script-dup") throws a not-unique /// InvalidActorNameException during the Instance Actor's PreStart. The failure /// surfaces as an ActorInitializationException the DM decider stops — the exact /// "Instance Actor dies during init" S6 scenario. + /// + /// 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. + /// /// 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). diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Deployment/ActorNameRulesParityTests.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Deployment/ActorNameRulesParityTests.cs new file mode 100644 index 00000000..2a83ec1c --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Deployment/ActorNameRulesParityTests.cs @@ -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; + +/// +/// Pins (Commons, which cannot reference Akka) to the rule it +/// mirrors, . The site deploy gate rejects names +/// through 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 +/// Context.ActorOf; 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. +/// +public class ActorNameRulesParityTests +{ + /// Printable ASCII 0x20–0x7E. + private static IEnumerable PrintableAscii() + { + for (var c = (char)0x20; c <= (char)0x7E; c++) + yield return c; + } + + /// + /// Characters Akka.NET 1.5 accepts in a path element that 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 (DeployCompileValidator) decides with + /// 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. + /// + private const string KnownMirrorStricterChars = "\"()"; + + private static void AssertParity(string candidate, List 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}"); + } + } + + /// + /// True when the disagreement is exactly the documented gap: the mirror refuses, Akka + /// accepts, and replacing every character with a + /// letter makes the two agree again. + /// + 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); + } + + /// + /// Akka accepts every known-gap character, so a name such as Pump(A) is deployable — + /// which is why the site gate must not decide with the stricter mirror. + /// + [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(); + } + + /// Every printable ASCII character alone, and with a letter on either side. + [Fact] + public void SingleCharacters_AgreeWithAkka() + { + var disagreements = new List(); + 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)); + } + + /// Every ordered pair of printable ASCII characters (95 × 95). + [Fact] + public void CharacterPairs_AgreeWithAkka() + { + var disagreements = new List(); + 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)); + } + + /// Percent-escapes, leading '$', empty/null-ish and non-ASCII samples. + [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(); + AssertParity(candidate, disagreements); + Assert.True(disagreements.Count == 0, string.Join(Environment.NewLine, disagreements)); + } + + /// Null is invalid on both sides of the mirror. + [Fact] + public void Null_IsInvalid() + { + Assert.False(ActorNameRules.IsValid(null)); + Assert.NotNull(ActorNameRules.Describe(null)); + } +} diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Deployment/DeployCompileValidatorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Deployment/DeployCompileValidatorTests.cs index eb0ab14a..09c2ecff 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Deployment/DeployCompileValidatorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Deployment/DeployCompileValidatorTests.cs @@ -104,4 +104,43 @@ public class DeployCompileValidatorTests var errors = NewValidator().Validate(cfg); Assert.Contains(errors, e => e.Contains("A1") && e.Contains("DoesNotExist")); } + + /// + /// Review 2026-10-21 01/SR-C1: names that become Akka actor names are checked by the gate, + /// with Akka's own rule (so Pump(A), which Akka accepts, is not rejected). + /// + [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)); + } + + /// Script, alarm and native-source canonical names are checked too. + [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")); + } }