Merge branch 'worktree-agent-aaa0474d0e4c3af87' into fix/flow-debugger-arch-review

2026-09-06 14:13:18 -04:00
parent 38509441bb 97b0a4ba16
commit 17096d6fe2
5 changed files with 111 additions and 17 deletions
@@ -14,8 +14,12 @@ namespace ZB.MOM.WW.ScadaBridge.CLI.Commands;
 /// </summary>
 public static class FlowDebugCommands
 {
-    /// <summary>Client-side wait for the run commands — must exceed the server's run grace + trigger window.</summary>
-    internal static readonly TimeSpan RunTimeout = TimeSpan.FromMinutes(2);
+    /// <summary>
+    /// 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 <c>FlowDebugService.ClientSlack</c> (15s).
+    /// </summary>
+    private const int ClientSlackSeconds = 25;

    /// <summary>Builds the <c>flow-debug</c> command with its subcommands using the given shared CLI options.</summary>
    /// <param name="urlOption">Shared management URL option.</param>
@@ -38,11 +42,24 @@ public static class FlowDebugCommands
    {
        var methodIdOption = new Option<int>("--method-id") { Description = "Inbound API method ID", Required = true };
        var paramsOption = new Option<string?>("--params") { Description = "Flat JSON object of method parameters (validated against the method's schema)" };
+        var timeoutOption = new Option<int>("--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;
@@ -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 <url> flow-debug run-method --method-id <int> [--params <json>]
+scadabridge --url <url> flow-debug run-method --method-id <int> [--params <json>] [--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 <url> flow-debug run-value-change --instance-id <int> --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.
@@ -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 <c>ManagementActor</c> through
 /// <see cref="ManagementActorHolder"/>, exactly as <see cref="DataConnectionMoveService"/>
 /// does, with the current Blazor principal projected to an <see cref="AuthenticatedUser"/>
-/// so the server's Deployer gate runs against the real identity. The Live-run Ask waits up
-/// to <see cref="RunAskTimeout"/> (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 (<see cref="RunAskTimeoutFor"/> /
+/// <see cref="ValueChangeAskTimeoutFor"/>) rather than a fixed duration (arch-review F4) —
+/// a run against a method with a long <c>TimeoutSeconds</c> 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
 /// <see cref="CommunicationService"/> 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.
 /// </summary>
 public sealed class FlowDebugService : IFlowDebugService
 {
-    /// <summary>Ask timeout for the two Live-run commands.</summary>
-    public static readonly TimeSpan RunAskTimeout = TimeSpan.FromMinutes(2);
-
    /// <summary>Ask timeout for Replay (a central DB read).</summary>
    public static readonly TimeSpan ReplayAskTimeout = TimeSpan.FromSeconds(30);

+    /// <summary>Slack the client adds over the server's own bound (Ask hop + serialization).</summary>
+    public static readonly TimeSpan ClientSlack = TimeSpan.FromSeconds(15);
+
+    /// <summary>Floor for a method-run Ask wait, regardless of how short the method's own timeout is.</summary>
+    public static readonly TimeSpan MinRunAskTimeout = TimeSpan.FromSeconds(30);
+
+    /// <summary>
+    /// Client-side Ask wait for <see cref="RunMethodAsync"/>: the method's own
+    /// <c>TimeoutSeconds</c> plus the server's <see cref="FlowDebugRunner.RunGrace"/> plus
+    /// <see cref="ClientSlack"/>, floored at <see cref="MinRunAskTimeout"/> — the server bounds
+    /// the run itself at <c>timeoutSeconds + RunGrace</c> (see <c>FlowDebugRunner.RunMethodAsync</c>),
+    /// so the client must wait at least that long or it cuts the run off before the server does.
+    /// </summary>
+    public static TimeSpan RunAskTimeoutFor(int timeoutSeconds)
+    {
+        var t = TimeSpan.FromSeconds(Math.Max(1, timeoutSeconds)) + FlowDebugRunner.RunGrace + ClientSlack;
+        return t < MinRunAskTimeout ? MinRunAskTimeout : t;
+    }
+
+    /// <summary>
+    /// Client-side Ask wait for <see cref="RunValueChangeAsync"/>: the trigger-wait window
+    /// plus <see cref="ClientSlack"/>.
+    /// </summary>
+    public static TimeSpan ValueChangeAskTimeoutFor(int waitSeconds)
+        => TimeSpan.FromSeconds(Math.Max(1, waitSeconds)) + ClientSlack;
+
    /// <summary>Upper bound on the template parent chain the script resolver walks (cycle guard).</summary>
    private const int MaxTemplateChain = 16;

@@ -176,12 +202,19 @@ public sealed class FlowDebugService : IFlowDebugService
    }

    /// <inheritdoc/>
-    public Task<FlowDebugOutcome> RunMethodAsync(int apiMethodId, JsonElement? parameters, CancellationToken cancellationToken = default)
-        => SendAndBuildAsync(new FlowDebugRunMethodCommand(apiMethodId, parameters), RunAskTimeout, cancellationToken);
+    public async Task<FlowDebugOutcome> 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);
+    }

    /// <inheritdoc/>
    public Task<FlowDebugOutcome> 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);
+    }

    /// <inheritdoc/>
    public Task<FlowDebugOutcome> ReplayAsync(Guid executionId, CancellationToken cancellationToken = default)
@@ -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<int>)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()
    {
@@ -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()
    {