From 7197c3986fb83906de4f298e9c1305f33016b668 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Tue, 8 Sep 2026 01:41:14 -0400 Subject: [PATCH] =?UTF-8?q?fix(central-ui):=20FlowDebugService=20=E2=80=94?= =?UTF-8?q?=20re-flatten=20on=20each=20instance=20selection;=20safe=20Ask?= =?UTF-8?q?=20fallback?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Claude-Session: https://claude.ai/code/session_01NcMG6xXdsJbmD2wYAgkK29 --- .../Services/FlowDebugService.cs | 73 ++++++++++++------- .../Services/FlowDebugServiceTests.cs | 35 ++++++++- 2 files changed, 79 insertions(+), 29 deletions(-) diff --git a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Services/FlowDebugService.cs b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Services/FlowDebugService.cs index d06936fb..4d937cca 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Services/FlowDebugService.cs +++ b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Services/FlowDebugService.cs @@ -90,11 +90,15 @@ public sealed class FlowDebugService : IFlowDebugService private readonly ILogger _logger; /// - /// Flattened script lists by instance id. The service is scoped (one per Blazor circuit - /// interaction), so this costs at most one flatten per instance the operator selects, - /// and the picker, the prediction and the run all read the same snapshot. The raw - /// s ride along because prediction needs the code bodies, - /// which the picker's projection deliberately does not carry. + /// Flattened script lists by instance id. A scoped service in Blazor Server lives for the + /// whole CIRCUIT, so this is emphatically not a per-interaction memo: it is refreshed by + /// every call (the page makes one per instance + /// selection), and only the prediction and the run read it back. That keeps one flatten per + /// picker selection while making a re-selection pick up a template edit or a redeploy — a + /// cache held for the session would otherwise serve a stale parameter schema (unexplained + /// VALIDATION_FAILED) and a stale Ask bound against a runner that re-flattens on every + /// run. The raw s ride along because prediction needs the code + /// bodies, which the picker's projection deliberately does not carry. /// private readonly Dictionary _scriptCache = new(); @@ -188,12 +192,12 @@ public sealed class FlowDebugService : IFlowDebugService /// public async Task> GetInstanceScriptsAsync(int instanceId, CancellationToken cancellationToken = default) - => (await LoadScriptsAsync(instanceId, cancellationToken))?.Options ?? []; + => (await FlattenScriptsAsync(instanceId, cancellationToken))?.Options ?? []; /// public async Task PredictScriptAsync(int instanceId, string scriptName, CancellationToken cancellationToken = default) { - var loaded = await LoadScriptsAsync(instanceId, cancellationToken); + var loaded = await CachedScriptsAsync(instanceId, cancellationToken); var script = loaded?.Scripts.FirstOrDefault(s => s.CanonicalName.Equals(scriptName, StringComparison.OrdinalIgnoreCase)); if (loaded == null || script == null) return PredictedFlow.Empty; @@ -237,16 +241,23 @@ public sealed class FlowDebugService : IFlowDebugService { // The server re-resolves the script and never trusts a client-supplied trigger kind // (design 2026-09-08 §2); the kind is read here only to size the Ask wait — a call kind - // is bounded by the script's own execution timeout — falling back to the runner's own - // FlowDebugRunner.DefaultScriptTimeoutSeconds so client and server agree on the bound — - // a write kind by the trigger-wait window. An unresolvable script falls back - // to the write-kind wait, which is the longer of the two for a default-timeout script. + // is bounded by the script's own execution timeout (falling back to the runner's own + // FlowDebugRunner.DefaultScriptTimeoutSeconds so client and server agree on the bound), + // a write kind by the trigger-wait window. When the script cannot be resolved at all the + // server may still take EITHER branch, so the fallback is the longer of the two — a + // client Ask shorter than the server's bound would report a phantom timeout on a run + // that is still going. var command = new FlowDebugRunScriptCommand(instanceId, scriptName, parameters, value); - var option = (await GetInstanceScriptsAsync(instanceId, cancellationToken)) + var option = (await CachedScriptsAsync(instanceId, cancellationToken))?.Options .FirstOrDefault(o => o.CanonicalName.Equals(scriptName, StringComparison.OrdinalIgnoreCase)); - var timeout = option is { IsWriteKind: false } - ? RunAskTimeoutFor(option.ExecutionTimeoutSeconds ?? FlowDebugRunner.DefaultScriptTimeoutSeconds) - : ValueChangeAskTimeoutFor(command.WaitSeconds); + var writeKindWait = ValueChangeAskTimeoutFor(command.WaitSeconds); + var defaultCallWait = RunAskTimeoutFor(FlowDebugRunner.DefaultScriptTimeoutSeconds); + var timeout = option switch + { + { IsWriteKind: false } => RunAskTimeoutFor(option.ExecutionTimeoutSeconds ?? FlowDebugRunner.DefaultScriptTimeoutSeconds), + not null => writeKindWait, + null => writeKindWait > defaultCallWait ? writeKindWait : defaultCallWait, + }; return await SendAndBuildAsync(command, timeout, cancellationToken); } @@ -402,22 +413,31 @@ public sealed class FlowDebugService : IFlowDebugService } /// - /// Loads an instance's scripts from its flattened configuration (design 2026-09-08 §2) — - /// canonical names, effective trigger kinds and effective parameter definitions, i.e. exactly - /// what the site's Instance Actor keys on — memoised for the life of this scoped service. - /// Returns null when the instance does not exist (Debug — an id from a stale link) or the - /// flatten failed (Warning — the same severity DeploymentService and the Configure - /// page give it, since the instance could not be deployed either). Neither is a page error: - /// the picker simply has no scripts to offer. + /// The instance's scripts as the current picker selection knows them: the cached entry a + /// call just refreshed, or a fresh flatten when the + /// prediction / run is reached without one (a deep link straight into a run). /// - private async Task LoadScriptsAsync(int instanceId, CancellationToken ct) - { - if (_scriptCache.TryGetValue(instanceId, out var cached)) - return cached; + private async Task CachedScriptsAsync(int instanceId, CancellationToken ct) + => _scriptCache.TryGetValue(instanceId, out var cached) ? cached : await FlattenScriptsAsync(instanceId, ct); + /// + /// Flattens an instance's scripts (design 2026-09-08 §2) — canonical names, effective trigger + /// kinds and effective parameter definitions, i.e. exactly what the site's Instance Actor keys + /// on — and refreshes the cache entry, so a template edit or a redeploy is picked up on the + /// next picker selection. Returns null when the instance does not exist (Debug — an id from a + /// stale link) or the flatten failed (Warning — the same severity DeploymentService and + /// the Configure page give it, since the instance could not be deployed either); either way + /// the stale entry is dropped rather than served on. Neither is a page error: the picker + /// simply has no scripts to offer. + /// + private async Task FlattenScriptsAsync(int instanceId, CancellationToken ct) + { var instance = await _templates.GetInstanceByIdAsync(instanceId, ct); if (instance == null) + { + _scriptCache.Remove(instanceId); return null; + } var flattened = await _pipeline.FlattenAndValidateAsync(instance.Id, ct, validateScripts: false); if (flattened.IsFailure) @@ -425,6 +445,7 @@ public sealed class FlowDebugService : IFlowDebugService _logger.LogWarning( "Flow Debugger could not flatten instance {InstanceId} ({UniqueName}): {Error}", instanceId, instance.UniqueName, flattened.Error); + _scriptCache.Remove(instanceId); return null; } diff --git a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Services/FlowDebugServiceTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Services/FlowDebugServiceTests.cs index aa8c5230..21fa0eb6 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Services/FlowDebugServiceTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Services/FlowDebugServiceTests.cs @@ -623,9 +623,9 @@ public sealed class FlowDebugServiceTests : IDisposable } /// - /// The picker, the prediction and the run of one instance share ONE flatten for the life of - /// the scoped service (design 2026-09-08 §5) — a regression here means every keystroke on the - /// page re-walks the template chain. + /// One picker selection costs ONE flatten: the prediction and the run that follow it read the + /// entry just refreshed (design + /// 2026-09-08 §5). A regression here means every keystroke re-walks the template chain. /// [Fact] public async Task GetInstanceScripts_ThenPredict_ThenRun_FlattenOnce() @@ -641,6 +641,35 @@ public sealed class FlowDebugServiceTests : IDisposable await _pipeline.Received(1).FlattenAndValidateAsync(9, Arg.Any(), false, null); } + /// + /// A scoped service lives for the whole Blazor circuit, so re-selecting the instance must + /// re-flatten — otherwise a template edit or a redeploy mid-session leaves the picker, the + /// parameter form and the Ask sizing stale for the rest of the session. + /// + [Fact] + public async Task GetInstanceScripts_CalledAgain_ReflattensAndRefreshes() + { + _templates.GetInstanceByIdAsync(9, Arg.Any()) + .Returns(new Instance("Sim01") { Id = 9, TemplateId = 10, SiteId = 1 }); + static Result Flattened(params ResolvedScript[] scripts) + => Result.Success(new FlatteningPipelineResult( + new FlattenedConfiguration { InstanceUniqueName = "Sim01", TemplateId = 10, SiteId = 1, Scripts = scripts }, + "h", new ValidationResult())); + _pipeline.FlattenAndValidateAsync(9, Arg.Any(), false, null).Returns( + Flattened(new ResolvedScript { CanonicalName = "Manual", Code = "x", TriggerType = "Call" }), + Flattened( + new ResolvedScript { CanonicalName = "Manual", Code = "x", TriggerType = "Call" }, + new ResolvedScript { CanonicalName = "Added", Code = "x", TriggerType = "Call" })); + var svc = CreateService(reply: null); + + var first = await svc.GetInstanceScriptsAsync(9); + var second = await svc.GetInstanceScriptsAsync(9); + + Assert.Equal(["Manual"], first.Select(o => o.CanonicalName).ToList()); + Assert.Equal(["Added", "Manual"], second.Select(o => o.CanonicalName).ToList()); + await _pipeline.Received(2).FlattenAndValidateAsync(9, Arg.Any(), false, null); + } + private sealed class StubAuth : AuthenticationStateProvider { private readonly ClaimsPrincipal _user;