From 97b0a4ba167f618ed5b70786869bb1e62b8fa232 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sun, 6 Sep 2026 14:13:02 -0400 Subject: [PATCH] fix(central-ui,cli): Flow Debugger client waits derive from the method timeout / wait window instead of a fixed 2 minutes (arch-review F4) Claude-Session: https://claude.ai/code/session_01NcMG6xXdsJbmD2wYAgkK29 --- .../Commands/FlowDebugCommands.cs | 25 +++++++-- src/ZB.MOM.WW.ScadaBridge.CLI/README.md | 10 ++-- .../Services/FlowDebugService.cs | 51 +++++++++++++++---- .../FlowDebugCommandsTests.cs | 24 +++++++++ .../Services/FlowDebugServiceTests.cs | 18 +++++++ 5 files changed, 111 insertions(+), 17 deletions(-) diff --git a/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/FlowDebugCommands.cs b/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/FlowDebugCommands.cs index c34d1fbd..944ee576 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/FlowDebugCommands.cs +++ b/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/FlowDebugCommands.cs @@ -14,8 +14,12 @@ namespace ZB.MOM.WW.ScadaBridge.CLI.Commands; /// public static class FlowDebugCommands { - /// Client-side wait for the run commands — must exceed the server's run grace + trigger window. - internal static readonly TimeSpan RunTimeout = TimeSpan.FromMinutes(2); + /// + /// Slack the CLI's HTTP wait adds over the server's own bound (arch-review F4): the CLI + /// process is one more hop out from the management Ask than the Central UI's in-process + /// call, so it carries a larger fixed margin than FlowDebugService.ClientSlack (15s). + /// + private const int ClientSlackSeconds = 25; /// Builds the flow-debug command with its subcommands using the given shared CLI options. /// Shared management URL option. @@ -38,11 +42,24 @@ public static class FlowDebugCommands { var methodIdOption = new Option("--method-id") { Description = "Inbound API method ID", Required = true }; var paramsOption = new Option("--params") { Description = "Flat JSON object of method parameters (validated against the method's schema)" }; + var timeoutOption = new Option("--timeout-seconds") + { + Description = "Client-side wait for the run; set to the method's TimeoutSeconds + 25 for long methods", + DefaultValueFactory = _ => 120, + }; var cmd = new Command("run-method") { Description = "Run an inbound API method on the active central node with full tracing and print the flow's steps" }; cmd.Add(methodIdOption); cmd.Add(paramsOption); + cmd.Add(timeoutOption); cmd.SetAction(async (ParseResult result) => { + var timeoutSeconds = result.GetValue(timeoutOption); + if (timeoutSeconds <= 0) + { + OutputFormatter.WriteError("--timeout-seconds must be greater than zero.", "INVALID_ARGUMENT"); + return 1; + } + JsonElement? parameters; try { @@ -57,7 +74,7 @@ public static class FlowDebugCommands return await CommandHelpers.ExecuteCommandAsync( result, urlOption, formatOption, usernameOption, passwordOption, new FlowDebugRunMethodCommand(result.GetValue(methodIdOption), parameters), - timeout: RunTimeout, + timeout: TimeSpan.FromSeconds(timeoutSeconds), tableProjector: FlowDebugTableProjection.ProjectEvents); }); return cmd; @@ -90,7 +107,7 @@ public static class FlowDebugCommands result.GetValue(attributeOption)!, result.GetValue(valueOption)!, waitSeconds), - timeout: RunTimeout, + timeout: TimeSpan.FromSeconds(waitSeconds + ClientSlackSeconds), tableProjector: FlowDebugTableProjection.ProjectEvents); }); return cmd; diff --git a/src/ZB.MOM.WW.ScadaBridge.CLI/README.md b/src/ZB.MOM.WW.ScadaBridge.CLI/README.md index a254cec0..7c680d27 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CLI/README.md +++ b/src/ZB.MOM.WW.ScadaBridge.CLI/README.md @@ -2305,16 +2305,17 @@ Run an inbound API method with the given parameters, exactly as an external call and print the traced flow. ```sh -scadabridge --url flow-debug run-method --method-id [--params ] +scadabridge --url flow-debug run-method --method-id [--params ] [--timeout-seconds 120] ``` | Option | Required | Description | |--------|----------|-------------| | `--method-id` | yes | Inbound API method ID (`api-method list`) | | `--params` | no | Flat JSON object of parameters, validated against the method's parameter schema. Omit for a parameterless method. | +| `--timeout-seconds` | no | Client-side wait for the run (default 120). Raise this above the method's own `TimeoutSeconds + 25` for a long-running method, or the client gives up before the server's own run grace does. | -The client waits up to 2 minutes. Parameters that fail schema validation are reported as -`VALIDATION_FAILED` without running the script. +Parameters that fail schema validation are reported as `VALIDATION_FAILED` without running +the script. #### `flow-debug run-value-change` @@ -2332,7 +2333,8 @@ scadabridge --url flow-debug run-value-change --instance-id --attrib | `--value` | yes | Value to write, in canonical string form | | `--wait-seconds` | no | How long to wait for a triggered run before reporting `NO_TRIGGER_WITHIN_WINDOW` (default 45) | -The write itself is real — the attribute changes — and is the root step of the flow. +The client waits `--wait-seconds` + 25 (no separate option — the client's HTTP wait always +derives from `--wait-seconds`). The write itself is real — the attribute changes — and is the root step of the flow. A static attribute triggers directly; a data-sourced attribute triggers when the device echoes the write back, which the site's trace arm (`SiteRuntime:TraceArmWindowSeconds`, default 30) correlates back to this run. diff --git a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Services/FlowDebugService.cs b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Services/FlowDebugService.cs index 2d2eb085..fa78b341 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Services/FlowDebugService.cs +++ b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Services/FlowDebugService.cs @@ -12,6 +12,7 @@ using ZB.MOM.WW.ScadaBridge.Commons.Messages.Notification; using ZB.MOM.WW.ScadaBridge.Commons.Types.Debugging; using ZB.MOM.WW.ScadaBridge.Communication; using ZB.MOM.WW.ScadaBridge.ManagementService; +using ZB.MOM.WW.ScadaBridge.ManagementService.FlowDebug; using ZB.MOM.WW.ScadaBridge.ScriptAnalysis; using ZB.MOM.WW.ScadaBridge.Security; @@ -23,21 +24,46 @@ namespace ZB.MOM.WW.ScadaBridge.CentralUI.Services; /// the run/replay commands go to the per-node ManagementActor through /// , exactly as /// does, with the current Blazor principal projected to an -/// so the server's Deployer gate runs against the real identity. The Live-run Ask waits up -/// to (the server bounds a run by the method timeout + grace and -/// a value-change by its wait window; both fit inside two minutes). +/// so the server's Deployer gate runs against the real identity. The Live-run Ask wait is +/// derived from the server's own bound ( / +/// ) rather than a fixed duration (arch-review F4) — +/// a run against a method with a long TimeoutSeconds used to be cut off client-side +/// well before the server's own grace period expired. /// Retry / Discard of a parked outbound node (follow-ups plan 2026-09-06 D4) goes through /// to the SAME central-local proxies the Site Calls and /// Notification report pages use, so the page and the report never disagree on what a relay does. /// public sealed class FlowDebugService : IFlowDebugService { - /// Ask timeout for the two Live-run commands. - public static readonly TimeSpan RunAskTimeout = TimeSpan.FromMinutes(2); - /// Ask timeout for Replay (a central DB read). public static readonly TimeSpan ReplayAskTimeout = TimeSpan.FromSeconds(30); + /// Slack the client adds over the server's own bound (Ask hop + serialization). + public static readonly TimeSpan ClientSlack = TimeSpan.FromSeconds(15); + + /// Floor for a method-run Ask wait, regardless of how short the method's own timeout is. + public static readonly TimeSpan MinRunAskTimeout = TimeSpan.FromSeconds(30); + + /// + /// Client-side Ask wait for : the method's own + /// TimeoutSeconds plus the server's plus + /// , floored at — the server bounds + /// the run itself at timeoutSeconds + RunGrace (see FlowDebugRunner.RunMethodAsync), + /// so the client must wait at least that long or it cuts the run off before the server does. + /// + public static TimeSpan RunAskTimeoutFor(int timeoutSeconds) + { + var t = TimeSpan.FromSeconds(Math.Max(1, timeoutSeconds)) + FlowDebugRunner.RunGrace + ClientSlack; + return t < MinRunAskTimeout ? MinRunAskTimeout : t; + } + + /// + /// Client-side Ask wait for : the trigger-wait window + /// plus . + /// + public static TimeSpan ValueChangeAskTimeoutFor(int waitSeconds) + => TimeSpan.FromSeconds(Math.Max(1, waitSeconds)) + ClientSlack; + /// Upper bound on the template parent chain the script resolver walks (cycle guard). private const int MaxTemplateChain = 16; @@ -176,12 +202,19 @@ public sealed class FlowDebugService : IFlowDebugService } /// - public Task RunMethodAsync(int apiMethodId, JsonElement? parameters, CancellationToken cancellationToken = default) - => SendAndBuildAsync(new FlowDebugRunMethodCommand(apiMethodId, parameters), RunAskTimeout, cancellationToken); + public async Task RunMethodAsync(int apiMethodId, JsonElement? parameters, CancellationToken cancellationToken = default) + { + var method = await _inboundApi.GetApiMethodByIdAsync(apiMethodId, cancellationToken); + var timeout = RunAskTimeoutFor(method?.TimeoutSeconds ?? 30); + return await SendAndBuildAsync(new FlowDebugRunMethodCommand(apiMethodId, parameters), timeout, cancellationToken); + } /// public Task RunValueChangeAsync(int instanceId, string attributeName, string value, CancellationToken cancellationToken = default) - => SendAndBuildAsync(new FlowDebugRunValueChangeCommand(instanceId, attributeName, value), RunAskTimeout, cancellationToken); + { + var command = new FlowDebugRunValueChangeCommand(instanceId, attributeName, value); + return SendAndBuildAsync(command, ValueChangeAskTimeoutFor(command.WaitSeconds), cancellationToken); + } /// public Task ReplayAsync(Guid executionId, CancellationToken cancellationToken = default) diff --git a/tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/FlowDebugCommandsTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/FlowDebugCommandsTests.cs index 2e0f33f8..b548e442 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/FlowDebugCommandsTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/FlowDebugCommandsTests.cs @@ -35,17 +35,41 @@ public class FlowDebugCommandsTests var runMethod = group.Subcommands.Single(c => c.Name == "run-method"); Assert.Contains(runMethod.Options, o => o.Name == "--method-id" && o.Required); Assert.Contains(runMethod.Options, o => o.Name == "--params" && !o.Required); + Assert.Contains(runMethod.Options, o => o.Name == "--timeout-seconds" && !o.Required); + Assert.DoesNotContain(runMethod.Options, o => o.Name == "--wait-seconds"); var runValueChange = group.Subcommands.Single(c => c.Name == "run-value-change"); Assert.Contains(runValueChange.Options, o => o.Name == "--instance-id" && o.Required); Assert.Contains(runValueChange.Options, o => o.Name == "--attribute" && o.Required); Assert.Contains(runValueChange.Options, o => o.Name == "--value" && o.Required); Assert.Contains(runValueChange.Options, o => o.Name == "--wait-seconds" && !o.Required); + Assert.DoesNotContain(runValueChange.Options, o => o.Name == "--timeout-seconds"); var replay = group.Subcommands.Single(c => c.Name == "replay"); Assert.Contains(replay.Options, o => o.Name == "--execution-id" && o.Required); } + [Fact] + public void RunMethod_TimeoutSecondsDefaultsTo120() + { + var group = FlowDebugCommands.Build(Url, Format, Username, Password); + var parse = group.Parse("run-method --method-id 4"); + + Assert.Empty(parse.Errors); + var timeoutOption = (Option)parse.CommandResult.Command.Options.Single(o => o.Name == "--timeout-seconds"); + Assert.Equal(120, parse.GetValue(timeoutOption)); + } + + [Fact] + public async Task RunMethod_TimeoutSecondsZero_IsRejectedWithInvalidArgument() + { + var group = FlowDebugCommands.Build(Url, Format, Username, Password); + + var exitCode = await group.Parse("run-method --method-id 4 --timeout-seconds 0").InvokeAsync(); + + Assert.Equal(1, exitCode); + } + [Fact] public void RunValueChange_WaitSecondsDefaultsTo45_MatchingTheCommandRecord() { 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 c4414b7b..43b78400 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Services/FlowDebugServiceTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Services/FlowDebugServiceTests.cs @@ -318,6 +318,24 @@ public sealed class FlowDebugServiceTests : IDisposable Assert.Contains("Deployer", envelope.User.Roles); } + [Theory] + [InlineData(180, 205)] + [InlineData(1, 30)] + public void RunAskTimeoutFor_IsMethodTimeoutPlusRunGracePlusClientSlack_FlooredAtMinRunAskTimeout(int timeoutSeconds, int expectedSeconds) + { + var timeout = FlowDebugService.RunAskTimeoutFor(timeoutSeconds); + + Assert.Equal(TimeSpan.FromSeconds(expectedSeconds), timeout); + } + + [Fact] + public void ValueChangeAskTimeoutFor_IsWaitSecondsPlusClientSlack() + { + var timeout = FlowDebugService.ValueChangeAskTimeoutFor(45); + + Assert.Equal(TimeSpan.FromSeconds(60), timeout); + } + [Fact] public async Task RunValueChange_AndReplay_SendTheirCommands() {