From ec81c4326c9fcae6fc3297b4611e8040e7dbdd42 Mon Sep 17 00:00:00 2001 From: Sev7eNup <79143581+Sev7eNup@users.noreply.github.com> Date: Sun, 16 Aug 2026 00:02:33 +0200 Subject: [PATCH] Fix four setup and engine defects found in the CM1 lab freeze test All four reproduce on a real installation and are verified fixed on the same machine, not only in unit tests. Unattended install skipped all provisioning once a host already had NodePilot. AnswerMode came from IsUpdateSelected(), which reads ModePage.SelectedValueIndex - and /ANSWERFILE skips the mode page, so the index kept its hard default of 0 ('update'). An answer file saying "mode": "install" therefore dropped every provisioning key on any second install, SCCM re-deploy or golden-image refresh. The file decides on that path now; update mode accepts no provisioning keys, so a Provision run for one performs no action and exits 0. Database provisioning never received -CertificateHostName. The script declares the parameter and the installer builds HostNameInCertificate from the same answer-file key, but the provisioning call dropped it, so it derived the name from -Server. The very normal 'localhost' against a server whose certificate names the FQDN failed with "The target principal name is incorrect" and created nothing. A failed provisioning was invisible unattended. It exits 0 and reports itself inside provision.ini, which the readiness page reads and the silent path did not - so the run walked on to Apply and died in the SQL pre-flight telling the operator to have a DBA create a login that was never the problem. The uninstaller ignored serviceName, installPath and dataPath. It passed GetServiceName('') - the literal 'NodePilot' in the uninstaller process - and {app}, which is only where Inno put the uninstaller, and never passed -DataPath at all. A non-default installation was "uninstalled" with exit 0 while the service, its port, its firewall rule and every program file stayed exactly where they were, and only the bookkeeping that said NodePilot existed was removed. It now reads all three back from the installation marker. waitAny/waitNofM marked whole runs Failed when the losing branches were runScript. Standing a branch down is what the junction is for, and StepRunner records those as Cancelled - but only if the exception reaches it. The PowerShell engines converted the cancellation into an ordinary failed result, and one Failed step fails the execution, so a correct run came back red with every junction, log and returnData green. Branches built from delay were unaffected because they let the exception through. All four sites now rethrow on caller cancellation, including the two catch-alls that re-wrapped the throw as "Isolated execution failed: Script execution cancelled". A timeout stays a failure. Tests: the junction contract end to end (loser Cancelled, run Succeeded), both PowerShell engines' cancel-versus-timeout split, and static contracts for the provisioning gate, the provisioning verdict, the certificate host name and the three uninstall parameters. --- deploy/Invoke-NodePilotSetup.ps1 | 13 ++- deploy/Test-DeploymentTemplates.ps1 | 71 +++++++++++++++- deploy/server/NodePilotServer.iss | 53 +++++++++++- deploy/server/README.md | 8 ++ .../PowerShell/IPowerShellExecutionEngine.cs | 10 +++ .../PowerShell/ProcessExecutionEngine.cs | 37 ++++++--- .../PowerShell/RunspaceExecutionEngine.cs | 16 ++-- src/NodePilot.Remote/WinRmSession.cs | 11 ++- .../PowerShell/ProcessIsolationEngineTests.cs | 15 ++-- .../PowerShell/RunspaceEngineAsyncTests.cs | 24 ++++-- .../WorkflowEngineTests.cs | 80 +++++++++++++++++++ 11 files changed, 296 insertions(+), 42 deletions(-) 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() {