diff --git a/deploy/Invoke-NodePilotSetup.ps1 b/deploy/Invoke-NodePilotSetup.ps1 index 45522cb0..dc309601 100644 --- a/deploy/Invoke-NodePilotSetup.ps1 +++ b/deploy/Invoke-NodePilotSetup.ps1 @@ -559,10 +559,21 @@ function Invoke-NodePilotSetupMode { "$env:USERDOMAIN\$env:COMPUTERNAME`$" } else { [string]$answers['identity.account'] } + # The certificate host name travels with the server name or the connection is + # rejected before any DDL runs. The runtime connection string carries it as + # HostNameInCertificate and Invoke-NodePilotPreflight is handed it too; leaving + # it off here meant provisioning derived it from -Server instead, so the very + # normal 'localhost' against a server whose TLS certificate names the FQDN died + # with "The target principal name is incorrect" and no database was created. + # An absent key is fine - the script derives the same fallback itself. + $certificateHostName = if ($answers.Contains('database.sqlCertificateHostName')) { + [string]$answers['database.sqlCertificateHostName'] + } else { '' } $outcome = & (Join-Path $scriptDirectory 'Provision-NodePilotDatabase.ps1') ` -Server ([string]$answers['database.sqlServer']) ` -Database ([string]$answers['database.sqlDatabase']) ` - -Principal $principal + -Principal $principal ` + -CertificateHostName $certificateHostName } Set-NodePilotResult -Buffer $result -Section 'provision.database' -Name 'status' -Value $outcome.Status Set-NodePilotResult -Buffer $result -Section 'provision.database' -Name 'detail' -Value $outcome.Detail diff --git a/deploy/Test-DeploymentTemplates.ps1 b/deploy/Test-DeploymentTemplates.ps1 index 19e987b6..bb12097a 100644 --- a/deploy/Test-DeploymentTemplates.ps1 +++ b/deploy/Test-DeploymentTemplates.ps1 @@ -1043,6 +1043,52 @@ Assert-TextMatches -Name 'uninstalling runs the deployment uninstaller from code -Text $serverIss -Pattern '(?s)usUninstall.*Uninstall-NodePilot\.ps1' Assert-TextMatches -Name 'the purge switch is built at uninstall time' ` -Text $serverIss -Pattern '(?s)usUninstall.*UninstallPurgeData then Switches' + +# Everything below reads the Pascal with its comments removed. The rules these contracts pin are +# each stated, in words, in a comment a few lines above the code that implements them - and a +# contract that matches its own explanation measures nothing. Same trap Remove-CommentLines was +# written for; Pascal just spells the prefix '//'. +$serverIssCode = Remove-CommentLines -Text $serverIss -CommentPrefix '//' + +# The uninstaller must remove the INSTALLATION, not merely the bookkeeping that says it exists. +# It ran with GetServiceName('') - which resolves to the literal 'NodePilot' in the uninstaller +# process, because ExistingServiceName is only ever set by DetectExistingInstallation() in Setup - +# and with {app}, which is where Inno put the uninstaller and NOT installPath when /ANSWERFILE +# supplied one. An install with a non-default service name or path was therefore "uninstalled" +# with exit 0 while the service, its firewall rule and every program file stayed put. -DataPath +# was not passed at all, so -PurgeData wiped the default directory or nothing. +# Patterns avoid quote characters entirely - the Pascal they match is full of them, and escaping +# both layers is how a contract ends up matching nothing. +Assert-TextMatches -Name 'the uninstall reads the installed service name from the marker' ` + -Text $serverIssCode ` + -Pattern '(?s)usUninstall[\s\S]*RegQueryStringValue\(HKLM64,[^)]*ServiceName' +Assert-TextMatches -Name 'the uninstall reads the installed path from the marker' ` + -Text $serverIssCode ` + -Pattern '(?s)usUninstall[\s\S]*RegQueryStringValue\(HKLM64,[^)]*InstallPath' +Assert-TextMatches -Name 'the uninstall passes the installed data path through' ` + -Text $serverIssCode -Pattern '(?s)usUninstall[\s\S]*-DataPath[\s\S]{0,40}InstalledDataPath' +Assert-TextDoesNotMatch -Name 'the uninstall must not pass the wizard default as the service name' ` + -Text $serverIssCode -Pattern '-ServiceName[\s\S]{0,24}GetServiceName' +Assert-TextDoesNotMatch -Name 'the uninstall must not pass {app} as the install path' ` + -Text $serverIssCode -Pattern '-InstallPath[\s\S]{0,24}ExpandConstant' + +# /ANSWERFILE skips the mode page, so IsUpdateSelected() reads ModePage's hard default of 0 +# ('update') and AnswerMode contradicts a file that says "mode": "install". Gating the silent +# provisioning run on AnswerMode alone therefore dropped every provisioning key - database, login, +# generated certificate, runtime - on any host that already carried a NodePilot installation. +Assert-TextMatches -Name 'an answer file reaches provisioning regardless of the unshown mode page' ` + -Text $serverIssCode ` + -Pattern '(?s)WizardSilent\(\) and \(\(AnswerFileOverride <> ..\)[\s\S]{0,40}AnswerMode = .install.' +Assert-TextDoesNotMatch -Name 'the silent provisioning gate must not rest on AnswerMode alone' ` + -Text $serverIssCode -Pattern 'WizardSilent\(\) and \(AnswerMode' + +# A failed database provisioning exits 0 and reports itself inside provision.ini. The readiness +# page has always read that value and stopped; the silent path walked on to Apply and died in the +# SQL pre-flight instead, telling the operator to have a DBA create a login that was never the +# problem. +Assert-TextMatches -Name 'the silent path reads the provisioning verdict, not just the exit code' ` + -Text $serverIssCode ` + -Pattern '(?s)-Mode Provision[\s\S]{0,900}?GetIniString\([^)]*provision\.database[^)]*status' # The data directory is ours; the database is not. There is no option to remove it and there must # not be one: this installer never created it. Assert-TextDoesNotMatch -Name 'the setup must not offer to drop the database' ` @@ -1620,7 +1666,7 @@ Assert-TextMatches -Name 'the readiness probe extracts the Postgres client first Assert-TextMatches -Name 'the auto-fix run extracts it too' ` -Text $serverIss -Pattern '(?s)if WantsFix then[\s\S]{0,200}EnsurePgClient\(\)' Assert-TextMatches -Name 'and so does the unattended path, which never sees a page' ` - -Text $serverIss -Pattern "(?s)WizardSilent\(\) and \(AnswerMode = 'install'\)[\s\S]{0,300}EnsurePgClient\(\)" + -Text $serverIss -Pattern '(?s)WizardSilent\(\) and \(\(AnswerFileOverride[\s\S]{0,400}EnsurePgClient\(\)' # The runtime fix is offered on the readiness page, before PrepareToInstall has extracted the # dontcopy payload. Checking only that the runtime is extracted somewhere misses that ordering bug: @@ -1643,7 +1689,8 @@ if ($interactiveRuntimeIndex -lt 0 -or $interactiveRunIndex -lt 0 -or 'bundled runtime before launching provisioning.') } -$silentProvisionStart = $serverIss.IndexOf("if WizardSilent() and (AnswerMode = 'install') then") +$silentProvisionStart = $serverIss.IndexOf( + "if WizardSilent() and ((AnswerFileOverride <> '') or (AnswerMode = 'install')) then") $silentProvisionEnd = $serverIss.IndexOf("Arguments := '-Mode Apply'", $silentProvisionStart) if ($silentProvisionStart -lt 0 -or $silentProvisionEnd -lt 0) { throw 'Deployment template check failed: could not locate the silent provisioning block.' @@ -1663,10 +1710,10 @@ if ($silentRuntimeIndex -lt 0 -or $silentRunIndex -lt 0 -or $silentRuntimeIndex # file - accepted, validated, then ignored - which is how a fleet rollout ends up with a service # that starts and answers 503 because the computer account was never granted db_owner. Assert-TextMatches -Name 'a silent install runs the provisioning its answer file asks for' ` - -Text $serverIss -Pattern "(?s)WizardSilent\(\) and \(AnswerMode = 'install'\)[\s\S]{0,600}-Mode Provision" + -Text $serverIss -Pattern '(?s)WizardSilent\(\) and \(\(AnswerFileOverride[\s\S]{0,700}-Mode Provision' # Before the install, not after it: everything provisioning does - the runtime, the certificate, # the database grant - is a precondition of the install rather than a follow-up to it. -$silentProvisionIndex = $serverIss.IndexOf("WizardSilent() and (AnswerMode = 'install')") +$silentProvisionIndex = $serverIss.IndexOf("WizardSilent() and ((AnswerFileOverride <> '')") $applyIndex = $serverIss.IndexOf("Arguments := '-Mode Apply'") if ($silentProvisionIndex -lt 0 -or $applyIndex -lt 0 -or $silentProvisionIndex -gt $applyIndex) { throw ('Deployment template check failed: the silent provisioning step does not run before ' + @@ -1826,6 +1873,22 @@ $updateBranch = $setupAdapter.Substring($updateBranchStart, $updateInvokeIndex - Assert-TextDoesNotMatch -Name 'the adapter must not pass a HTTPS port to the updater' ` -Text $updateBranch -Pattern '\bHttpsPort\b' +# The provisioning connection is subject to the same TLS name check as the runtime one, so the +# certificate host name has to travel with the server name. Without it the provisioner derived the +# name from -Server, and the entirely normal 'localhost' against a SQL Server whose certificate +# names the FQDN failed with "The target principal name is incorrect" - leaving no database, and +# an install that then died in the SQL pre-flight blaming a missing login. +$provisionDbIndex = $setupAdapter.IndexOf("'Provision-NodePilotDatabase.ps1'") +if ($provisionDbIndex -lt 0) { + throw 'Deployment template check failed: could not locate the database provisioning call in the setup adapter.' +} +# Bounded to the invocation itself: the adapter reads database.sqlCertificateHostName elsewhere +# (the preflight splat), and a file-wide match would pass on that alone. +$provisionDbCall = $setupAdapter.Substring($provisionDbIndex, + [Math]::Min(400, $setupAdapter.Length - $provisionDbIndex)) +Assert-TextMatches -Name 'database provisioning is given the certificate host name' ` + -Text $provisionDbCall -Pattern '-CertificateHostName' + # powershell.exe -File returns 0 for a script that merely wrote errors, so an implicit # fall-through would report a failed installation as success. Assert-TextMatches -Name 'the adapter exits explicitly' ` diff --git a/deploy/server/NodePilotServer.iss b/deploy/server/NodePilotServer.iss index 7a343061..222f8954 100644 --- a/deploy/server/NodePilotServer.iss +++ b/deploy/server/NodePilotServer.iss @@ -1686,7 +1686,7 @@ end; function PrepareToInstall(var NeedsRestart: Boolean): String; var ResultCode: Integer; - AnswerMode, Arguments, ResultIni, ProvisionIni, Extra: String; + AnswerMode, Arguments, ResultIni, ProvisionIni, DbStatus, Extra: String; begin Result := EnsureSession(); if Result <> '' then Exit; @@ -1709,7 +1709,16 @@ begin // Run unconditionally rather than after parsing the file for a "does it ask for anything" // flag: Pascal Script has no JSON reader, the adapter already has one, and a run with nothing // requested performs no action and exits 0. - if WizardSilent() and (AnswerMode = 'install') then + // + // /ANSWERFILE is deliberately NOT filtered through AnswerMode. That variable comes from + // IsUpdateSelected(), which reads ModePage.SelectedValueIndex - and /ANSWERFILE skips the mode + // page, so the index keeps its hard default of 0 ('update'). Any host that already carried a + // NodePilot installation therefore turned an answer file saying "mode": "install" into + // AnswerMode = 'update' and silently dropped every provisioning key: no database, no login, no + // generated certificate, no runtime. The file decides on this path, by the same reasoning as + // above - the adapter validates it, update mode accepts no provisioning keys at all, so a + // Provision run for an update answer file performs no action and exits 0. + if WizardSilent() and ((AnswerFileOverride <> '') or (AnswerMode = 'install')) then begin // Unattended runs never reached the readiness page, so this is the first and only chance to // put lazy dontcopy payloads where the adapter looks for them. @@ -1723,6 +1732,18 @@ begin IntToStr(ResultCode) + '). Log: ' + ExpandConstant('{%TEMP}') + '\nodepilot-server-setup.log'; Exit; end; + + // A failed database provisioning exits 0 and reports itself INSIDE provision.ini, exactly as + // it does for the readiness page - which reads this same value and stops. Without the check + // the unattended path walked on to Apply and died in the SQL pre-flight instead, telling the + // operator to have a DBA create a login that was never the problem. + DbStatus := GetIniString('provision.database', 'status', '', ProvisionIni); + if (DbStatus <> '') and (DbStatus <> 'Pass') then + begin + Result := 'The database could not be prepared: ' + + ExpandNewlines(GetIniString('provision.database', 'detail', '', ProvisionIni)); + Exit; + end; end; // -Mode Apply, not Install or Update: the answer file already declares which it is, and a @@ -1816,6 +1837,7 @@ procedure CurUninstallStepChanged(CurUninstallStep: TUninstallStep); var ResultCode: Integer; ScriptPath, Arguments, Switches: String; + InstalledServiceName, InstalledInstallPath, InstalledDataPath: String; begin if CurUninstallStep = usUninstall then begin @@ -1834,9 +1856,32 @@ begin Switches := ''; if UninstallPurgeData then Switches := Switches + ' -PurgeData'; + // Read back what was actually installed, from the marker Install-NodePilot.ps1 writes. + // GetServiceName('') resolves to the literal 'NodePilot' in the uninstaller process - + // ExistingServiceName is only ever populated by DetectExistingInstallation(), which runs in + // Setup - and {app} is merely where Inno put the uninstaller, which is NOT installPath when + // /ANSWERFILE supplied one (the dir page never ran). Passing those guesses meant an install + // with a non-default service name or path was "uninstalled" with exit 0 while the service, + // its firewall rule and every program file stayed exactly where they were, and only the + // bookkeeping that said NodePilot existed was removed. -DataPath was not passed at all, so + // -PurgeData wiped the default directory or nothing. + if (not RegQueryStringValue(HKLM64, 'SOFTWARE\NodePilot\Server', 'ServiceName', InstalledServiceName)) or + (InstalledServiceName = '') then + InstalledServiceName := GetServiceName(''); + if (not RegQueryStringValue(HKLM64, 'SOFTWARE\NodePilot\Server', 'InstallPath', InstalledInstallPath)) or + (InstalledInstallPath = '') then + InstalledInstallPath := ExpandConstant('{app}'); + if not RegQueryStringValue(HKLM64, 'SOFTWARE\NodePilot\Server', 'DataPath', InstalledDataPath) then + InstalledDataPath := ''; + Arguments := '-NoProfile -ExecutionPolicy Bypass -File "' + ScriptPath + '"' + - ' -ServiceName "' + GetServiceName('') + '"' + - ' -InstallPath "' + ExpandConstant('{app}') + '"' + Switches; + ' -ServiceName "' + InstalledServiceName + '"' + + ' -InstallPath "' + InstalledInstallPath + '"'; + // Only when known: Uninstall-NodePilot.ps1's own default is the right fallback, and an empty + // -DataPath "" would point it at the current directory. + if InstalledDataPath <> '' then + Arguments := Arguments + ' -DataPath "' + InstalledDataPath + '"'; + Arguments := Arguments + Switches; if not Exec('powershell.exe', Arguments, '', SW_HIDE, ewWaitUntilTerminated, ResultCode) then SuppressibleMsgBox('Could not start PowerShell to remove the NodePilot service.', diff --git a/deploy/server/README.md b/deploy/server/README.md index 38588236..ac0e1990 100644 --- a/deploy/server/README.md +++ b/deploy/server/README.md @@ -619,6 +619,14 @@ JWT-Signaturschlüssel, Data-Protection-Keyring). Default ist **behalten**, übe "C:\Program Files\NodePilot\unins000.exe" /VERYSILENT /SUPPRESSMSGBOXES /PURGEDATA=1 # Daten löschen ``` +**Was entfernt wird, liest der Uninstaller aus dem Installations-Marker** +(`HKLM\SOFTWARE\NodePilot\Server`: `ServiceName`, `InstallPath`, `DataPath`) — nicht aus den +Wizard-Defaults. Das ist der einzige Weg, der bei einer Installation mit abweichendem Dienstnamen +oder abweichenden Pfaden funktioniert: die Modus- und Verzeichnis-Seiten laufen unter +`/ANSWERFILE` nie, `{app}` ist dann lediglich der Ort des Uninstallers, und der Uninstaller-Prozess +kennt den Dienstnamen des Setups nicht mehr. Wer den Marker von Hand löscht, nimmt dem Uninstaller +damit seine einzige Quelle; er fällt dann auf `NodePilot` und `{app}` zurück. + **Die Datenbank wird nie entfernt, und es gibt dafür keine Option.** Dieses Setup legt sie nicht an — sie wurde separat bereitgestellt, hat oft ein eigenes Backup-, Replikations- und Aufbewahrungsregime, und in einem Active/Passive-Cluster teilen sich **beide Knoten dieselbe diff --git a/src/NodePilot.Engine/PowerShell/IPowerShellExecutionEngine.cs b/src/NodePilot.Engine/PowerShell/IPowerShellExecutionEngine.cs index 2a61d729..c860daa5 100644 --- a/src/NodePilot.Engine/PowerShell/IPowerShellExecutionEngine.cs +++ b/src/NodePilot.Engine/PowerShell/IPowerShellExecutionEngine.cs @@ -72,6 +72,16 @@ public sealed class PowerShellExecutionResult public interface IPowerShellExecutionEngine { + /// + /// The message carried by the every engine throws + /// when the caller's token is signalled. Caller cancellation is not a script failure — it is + /// how a waitAny/waitNofM junction stands down its losing branches — so the exception has to + /// reach StepRunner, which records the step as Cancelled. An engine that swallowed it into a + /// failed result marked those branches Failed, and a single Failed row fails the whole run. + /// A timeout is a different matter and still comes back as a failed result with TimedOut set. + /// + public const string CancelledMessage = "Script execution cancelled"; + string EngineType { get; } bool IsAvailable { get; } Task ExecuteAsync(PowerShellExecutionRequest request, CancellationToken ct); diff --git a/src/NodePilot.Engine/PowerShell/ProcessExecutionEngine.cs b/src/NodePilot.Engine/PowerShell/ProcessExecutionEngine.cs index f2c453d3..83c95b00 100644 --- a/src/NodePilot.Engine/PowerShell/ProcessExecutionEngine.cs +++ b/src/NodePilot.Engine/PowerShell/ProcessExecutionEngine.cs @@ -126,16 +126,17 @@ public async Task ExecuteAsync(PowerShellExecutionReq try { process.Kill(entireProcessTree: true); } catch { /* best-effort: process may have exited */ } sw.Stop(); - var isUserCancel = ct.IsCancellationRequested; + // Caller cancel is not a failure - see IPowerShellExecutionEngine.CancelledMessage. + // The process is killed either way; only the verdict differs. + if (ct.IsCancellationRequested) + throw new OperationCanceledException(IPowerShellExecutionEngine.CancelledMessage, ct); return new PowerShellExecutionResult { Success = false, ExitCode = -1, Output = stdout.ToString().TrimEnd(), - Error = isUserCancel - ? "Script execution cancelled" - : $"Script execution timed out after {request.Timeout!.Value.TotalSeconds:0}s", - TimedOut = !isUserCancel, + Error = $"Script execution timed out after {request.Timeout!.Value.TotalSeconds:0}s", + TimedOut = true, Duration = sw.Elapsed, }; } @@ -156,8 +157,11 @@ public async Task ExecuteAsync(PowerShellExecutionReq Duration = sw.Elapsed, }; } - catch (Exception ex) + catch (Exception ex) when (ex is not OperationCanceledException) { + // Same reason as the isolated path: a caller cancel has to leave as an exception, or + // it comes back out of here as "Failed to start …: Script execution cancelled" and the + // junction's losing branch is a Failed step again. sw.Stop(); return EngineFailure($"Failed to start {_executable}: {ex.Message}", sw.Elapsed); } @@ -249,17 +253,20 @@ private async Task ExecuteIsolatedWindowsAsync(PowerS sw.Stop(); - if (userCancel || timedOut) + // Caller cancel is not a failure - see IPowerShellExecutionEngine.CancelledMessage. + // Thrown after the drain above so the job object is already closed and the tree reaped. + if (userCancel) + throw new OperationCanceledException(IPowerShellExecutionEngine.CancelledMessage, ct); + + if (timedOut) { return new PowerShellExecutionResult { Success = false, ExitCode = -1, Output = stdout.TrimEnd(), - Error = userCancel - ? "Script execution cancelled" - : $"Script execution timed out after {request.Timeout!.Value.TotalSeconds:0}s", - TimedOut = timedOut, + Error = $"Script execution timed out after {request.Timeout!.Value.TotalSeconds:0}s", + TimedOut = true, Duration = sw.Elapsed, }; } @@ -291,8 +298,14 @@ private async Task ExecuteIsolatedWindowsAsync(PowerS Duration = sw.Elapsed, }; } - catch (Exception ex) + catch (Exception ex) when (ex is not OperationCanceledException) { + // A caller cancel is deliberately NOT an engine failure: it is a junction standing + // down a losing branch, and it has to reach StepRunner as an exception so the step is + // recorded Cancelled rather than Failed. Without this guard the throw above walked + // straight into this handler and came back out as + // "Isolated execution failed: Script execution cancelled" — the same red step under a + // new name. sw.Stop(); return EngineFailure($"Isolated execution failed: {ex.Message}", sw.Elapsed); } diff --git a/src/NodePilot.Engine/PowerShell/RunspaceExecutionEngine.cs b/src/NodePilot.Engine/PowerShell/RunspaceExecutionEngine.cs index cd0c52f1..1fdbc07c 100644 --- a/src/NodePilot.Engine/PowerShell/RunspaceExecutionEngine.cs +++ b/src/NodePilot.Engine/PowerShell/RunspaceExecutionEngine.cs @@ -208,15 +208,21 @@ private async Task ExecuteOnceAsync(PowerShellExecuti sw.Stop(); // Distinguish caller-cancellation (parent ct) from our internal timeout firing. // Without an explicit timeout the only way we end up here is via parent ct. - var isUserCancel = ct.IsCancellationRequested; + // A caller cancel is NOT a script failure: it is the losing branch of a waitAny / + // waitNofM junction being stood down, and StepRunner's OperationCanceledException + // handler is what records the Cancelled row for it. Returning Success=false here + // instead made the step Failed, and one Failed row fails the whole execution - so + // every junction race reported the run red even though it did exactly what it should. + // `delay` never had the problem because it lets the exception through. + // A timeout stays a failure and keeps its result. + if (ct.IsCancellationRequested) + throw new OperationCanceledException(IPowerShellExecutionEngine.CancelledMessage, ct); return new PowerShellExecutionResult { Success = false, ExitCode = -1, - TimedOut = !isUserCancel, - Error = isUserCancel - ? "Script execution cancelled" - : $"Script timed out after {request.Timeout!.Value.TotalSeconds:0}s", + TimedOut = true, + Error = $"Script timed out after {request.Timeout!.Value.TotalSeconds:0}s", Duration = sw.Elapsed, }; } diff --git a/src/NodePilot.Remote/WinRmSession.cs b/src/NodePilot.Remote/WinRmSession.cs index 8cb09466..2c1803f5 100644 --- a/src/NodePilot.Remote/WinRmSession.cs +++ b/src/NodePilot.Remote/WinRmSession.cs @@ -164,12 +164,17 @@ private async Task ExecuteOnceAsync(string script, int? t RemoteMetrics.ScriptTimeouts.Add(1); RemoteMetrics.ScriptDuration.Record(sw.Elapsed.TotalMilliseconds, new KeyValuePair("result", cancelled ? "cancelled" : "timeout")); + // A caller cancel is not a script failure - it is how a waitAny/waitNofM junction + // stands down its losing branches, and StepRunner records those as Cancelled from the + // exception. Returning Success=false marked them Failed, and one Failed step fails the + // whole run. The session stays poisoned either way; only the verdict differs. A + // timeout remains a failure. + if (cancelled) + throw new OperationCanceledException("Script execution cancelled", ct); return new RemoteExecutionResult { Success = false, - ErrorOutput = cancelled - ? "Script execution cancelled" - : $"Script execution timed out after {timeoutSeconds} seconds", + ErrorOutput = $"Script execution timed out after {timeoutSeconds} seconds", Duration = sw.Elapsed }; } diff --git a/tests/NodePilot.Engine.Tests/PowerShell/ProcessIsolationEngineTests.cs b/tests/NodePilot.Engine.Tests/PowerShell/ProcessIsolationEngineTests.cs index ffbea62a..adf2fe48 100644 --- a/tests/NodePilot.Engine.Tests/PowerShell/ProcessIsolationEngineTests.cs +++ b/tests/NodePilot.Engine.Tests/PowerShell/ProcessIsolationEngineTests.cs @@ -39,8 +39,12 @@ public async Task ExecuteIsolated_Timeout_ReturnsTimedOutResult() } [WindowsFact] - public async Task ExecuteIsolated_CallerCancellation_ReturnsCancelledNotTimedOut() + public async Task ExecuteIsolated_CallerCancellation_ThrowsInsteadOfReturningAFailedResult() { + // Same contract as the in-process runspace engine: a caller cancel is a junction standing + // down a losing branch, not a script failure, so it has to reach StepRunner as an + // OperationCanceledException. Returning Success=false wrote the branch as Failed and + // turned every waitAny/waitNofM run red. The timeout branch above is unaffected. var engine = IsolatedPowerShell(); using var cts = new CancellationTokenSource(); @@ -56,12 +60,13 @@ public async Task ExecuteIsolated_CallerCancellation_ReturnsCancelledNotTimedOut await Task.Delay(400); cts.Cancel(); - var result = await task; + + var thrown = await Assert.ThrowsAnyAsync(() => task); sw.Stop(); - result.Success.Should().BeFalse(); - result.TimedOut.Should().BeFalse("caller cancellation is distinct from a timeout"); - result.Error.Should().Be("Script execution cancelled"); + // Specifically NOT "Isolated execution failed: Script execution cancelled" — the outer + // catch-all used to re-wrap the throw into a failed result under a new name. + thrown.Message.Should().Be(IPowerShellExecutionEngine.CancelledMessage); sw.Elapsed.Should().BeLessThan(TimeSpan.FromSeconds(15), "cancel must tear the job down promptly, not wait out the 60s sleep"); } diff --git a/tests/NodePilot.Engine.Tests/PowerShell/RunspaceEngineAsyncTests.cs b/tests/NodePilot.Engine.Tests/PowerShell/RunspaceEngineAsyncTests.cs index 58edb869..aba87925 100644 --- a/tests/NodePilot.Engine.Tests/PowerShell/RunspaceEngineAsyncTests.cs +++ b/tests/NodePilot.Engine.Tests/PowerShell/RunspaceEngineAsyncTests.cs @@ -8,17 +8,24 @@ namespace NodePilot.Engine.Tests.PowerShell; /// /// Verifies the async behavior of RunspaceExecutionEngine after the BeginInvoke/EndInvoke -/// port. Three properties matter under load: +/// port. Four properties matter under load: /// 1. Caller cancellation tears down the running script promptly (used to be impossible /// with Task.Run(() => ps.Invoke()) — the token only cancelled scheduling). -/// 2. Per-script timeout actually stops the pipeline (same rationale). -/// 3. Many concurrent ExecuteAsync calls all complete with correct, non-interleaved output. +/// 2. Caller cancellation surfaces as OperationCanceledException, not as a failed result. +/// 3. Per-script timeout actually stops the pipeline (same rationale) and stays a failure. +/// 4. Many concurrent ExecuteAsync calls all complete with correct, non-interleaved output. /// public class RunspaceEngineAsyncTests { [Fact] - public async Task Execute_CallerCancellation_StopsPromptlyAndIsNotTimedOut() + public async Task Execute_CallerCancellation_ThrowsInsteadOfReturningAFailedResult() { + // Caller cancellation is how a waitAny/waitNofM junction stands down the branches that + // lost the race. StepRunner records those as Cancelled — but only if the exception reaches + // it. This engine used to convert the cancellation into Success=false, so the loser was + // written as a Failed step, and a single Failed step fails the whole execution: every + // junction race reported a correct run as red. `delay` never had the problem because it + // lets the exception through, which is exactly the behaviour pinned here. using var engine = new RunspaceExecutionEngine( NullLogger.Instance, minRunspaces: 1, @@ -38,12 +45,13 @@ public async Task Execute_CallerCancellation_StopsPromptlyAndIsNotTimedOut() // Give the pipeline a moment to actually start executing on the runspace. await Task.Delay(150); cts.Cancel(); - var result = await task; + + var thrown = await Assert.ThrowsAnyAsync(() => task); sw.Stop(); - result.Success.Should().BeFalse(); - result.TimedOut.Should().BeFalse("caller cancellation is distinct from timeout-fire"); - result.Error.Should().Be("Script execution cancelled"); + thrown.CancellationToken.Should().Be(cts.Token, + "StepRunner tells a junction stand-down from a whole-execution cancel by the token"); + thrown.Message.Should().Be(IPowerShellExecutionEngine.CancelledMessage); // A 30-second sleep cancelled at 150ms must return well under the original sleep. // 5 seconds is a generous bound that won't flake under CI load but still proves the // pipeline was actively stopped (not waited out). diff --git a/tests/NodePilot.Engine.Tests/WorkflowEngineTests.cs b/tests/NodePilot.Engine.Tests/WorkflowEngineTests.cs index a59c56ff..0ce03530 100644 --- a/tests/NodePilot.Engine.Tests/WorkflowEngineTests.cs +++ b/tests/NodePilot.Engine.Tests/WorkflowEngineTests.cs @@ -1102,6 +1102,86 @@ public async Task ExecuteAsync_WaitAnyJunction_FiresAfterFirstBranchCompletes() "waitAny must fire after the fast branch (≈10ms), not after the slow branch (2000ms)"); } + /// + /// Standing a branch down is what a waitAny junction is for, so the run's verdict must not + /// hold it against the workflow. Found in the lab on 2026-08-15: every runbook whose racing + /// branches were runScript reported Failed on a completely correct run, because the + /// PowerShell engines converted the cancellation into an ordinary failed ActivityResult + /// instead of letting the OperationCanceledException reach StepRunner. A single Failed step + /// fails the execution, so the winning branch, both junctions and returnData were all green + /// and the run was still red. Branches built from delay were unaffected — they let the + /// exception through, which is the behaviour pinned here for every activity. + /// + [Fact] + public async Task ExecuteAsync_WaitAnyJunction_StandsDownLosersAsCancelledAndStillSucceeds() + { + _mockExecutor.Setup(e => e.ExecuteAsync( + It.IsAny(), + It.IsAny(), + It.IsAny())) + .Returns(async (ctx, _, ct) => + { + // Task.Delay throws OperationCanceledException on the losing branch, exactly as a + // real activity must now that the PowerShell engines rethrow instead of swallowing. + var ms = ctx.StepId switch + { + "branchFast" => 10, + "branchSlow" => 5000, + _ => 5, + }; + await Task.Delay(ms, ct); + return new ActivityResult { Success = true, Output = ctx.StepId }; + }); + + var mockJunction = new Mock(); + mockJunction.Setup(e => e.ActivityType).Returns("junction"); + mockJunction.Setup(e => e.ExecuteAsync( + It.IsAny(), + It.IsAny(), + It.IsAny())) + .ReturnsAsync(new ActivityResult { Success = true, Output = "merged" }); + + var registry = new ActivityRegistry( + new[] { _mockExecutor.Object, _manualTriggerExecutor.Object, mockJunction.Object }); + var sp = TestDbContext.BuildScopeProviderOnSameConnection(_connection, registry); + var notifier = new Mock(); + var engine = new WorkflowEngine(_db, NullLogger.Instance, sp, notifier.Object); + + var def = "{\"nodes\":[" + TriggerNodeJson + """ + ,{"id":"branchFast","type":"activity","position":{"x":0,"y":0},"data":{"activityType":"runScript","config":{}}}, + {"id":"branchSlow","type":"activity","position":{"x":0,"y":0},"data":{"activityType":"runScript","config":{}}}, + {"id":"join","type":"junction","position":{"x":0,"y":0},"data":{"activityType":"junction","config":{"mode":"waitAny"}}}, + {"id":"final","type":"activity","position":{"x":0,"y":0},"data":{"activityType":"runScript","config":{}}} + ], + "edges":[ + {"id":"t1","source":"trigger-1","target":"branchFast"}, + {"id":"t2","source":"trigger-1","target":"branchSlow"}, + {"id":"e1","source":"branchFast","target":"join"}, + {"id":"e2","source":"branchSlow","target":"join"}, + {"id":"e3","source":"join","target":"final"} + ] + } + """; + + var workflow = CreateWorkflow(def); + _db.Workflows.Add(workflow); + await _db.SaveChangesAsync(); + + var execution = await engine.ExecuteAsync(workflow, "test-user", CancellationToken.None); + + var steps = _db.StepExecutions.Where(s => s.WorkflowExecutionId == execution.Id).ToList(); + + steps.Should().NotContain(s => s.Status == ExecutionStatus.Failed, + "standing a losing branch down is the junction working, not a step failing"); + execution.Status.Should().Be(ExecutionStatus.Succeeded); + + var loser = steps.SingleOrDefault(s => s.StepId == "branchSlow"); + loser.Should().NotBeNull("the losing branch must still leave a row, so the run is explicable"); + loser!.Status.Should().Be(ExecutionStatus.Cancelled); + steps.Single(s => s.StepId == "branchFast").Status.Should().Be(ExecutionStatus.Succeeded); + steps.Single(s => s.StepId == "final").Status.Should().Be(ExecutionStatus.Succeeded); + } + [Fact] public async Task ExecuteAsync_WithParameters_PersistsInputParametersJson() {