Migrate SilentProcessRunner to async by jimmyp · Pull Request #1236 · OctopusDeploy/OctopusTentacle

jimmyp · 2026-05-25T02:28:01Z

Summary

Makes SilentProcessRunner.ExecuteCommand async. Required foundation for the EFT-3295 script-abandonment feature (PR #1226, which stacks on top of this) but merging it on its own makes the next PR much easier to review.

What this PR does

SilentProcessRunner.ExecuteCommand → ExecuteCommandAsync (and the matching interfaces and helpers)
The six immediate sync callers (PowerShellPrerequisite, KubernetesDirectoryInformationProvider, SystemCtlHelper x2, LinuxServiceConfigurator x3, WindowsServiceConfigurator) block via .GetAwaiter().GetResult() with a comment explaining the call context and why blocking on a thread-pool worker is deadlock-safe

Generated with Claude Code

Replaces the sync WaitForExit() with await WaitForExitAsync(cancel). The cancel token is passed directly so the existing cancel semantics are preserved: cancel firing throws OCE from the await and unwinds. DoOurBestToCleanUp continues to fire on cancel via cancel.Register exactly as it did in the sync version. Adds a net48 polyfill for WaitForExitAsync using Process.Exited + TaskCompletionSource. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

ISilentProcessRunner, SilentProcessRunnerWrapper, and the SilentProcessRunnerExtended helpers now return Task<int> and call SilentProcessRunner.ExecuteCommandAsync directly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

CommandLineRunner.Execute is consumed by Octopus.Manager.Tentacle (a WPF app), so the public method stays sync. It now blocks on the new SilentProcessRunner.ExecuteCommandAsync via GetAwaiter().GetResult() — safe because the WPF callers dispatch through ThreadPool.QueueUserWorkItem, which has no synchronisation context. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

RunningScript now awaits SilentProcessRunner.ExecuteCommandAsync through a new RunScriptAsync helper. The monitored-startup path also awaits the async helper inside Task.Run. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

The six immediate sync callers of SilentProcessRunner now go through ExecuteCommandAsync(...).GetAwaiter().GetResult(): - Octopus.Manager.Tentacle PowerShellPrerequisite (WPF installer) - KubernetesDirectoryInformationProvider (IMemoryCache factory) - SystemCtlHelper (2 sites — start and sudo retry) - LinuxServiceConfigurator (3 sites — chmod, systemctl probe, sudo probe) - WindowsServiceConfigurator (sc.exe wrapper) Each site gets a comment explaining why it must be sync and why blocking on a thread-pool worker is deadlock-safe. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Updates Kubernetes integration test setup helpers, PowerShell startup-detection tests, integration support, and Linux test fixtures to call the new async ExecuteCommandAsync API. Tests that don't await directly (NUnit static helpers, cache factories) block on .GetAwaiter().GetResult() and document why it's deadlock-safe on the test threadpool. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Rewrites the six sync-boundary comments to name the canonical pattern ("sync-over-async"), link Stephen Cleary's "Don't Block on Async Code" reference, and keep the "we are here / we do this / safe because" structure. Removes em-dashes per style preference. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

jimmyp · 2026-05-25T03:28:49Z

                        process.BeginErrorReadLine();

-                        process.WaitForExit();
+#if NETFRAMEWORK


Remove all changes from this method implementation, this should be a signature change only in this class

jimmyp · 2026-05-25T03:29:02Z

            }
        }

+#if NETFRAMEWORK


Remove this and put it in the next PR

jimmyp · 2026-05-25T03:30:10Z


-            var exitCode = SilentProcessRunner.ExecuteCommand(
+            // Dispose() cannot be made async; .GetAwaiter().GetResult() is safe here
+            // because this runs in test teardown (not inside an async context with a sync-blocking SynchronizationContext).


This doesn't match our lighthouse example comment on the wpf installer. Specifically where it calls out the pattern and links the blog post

jimmyp · 2026-05-25T03:30:43Z

    public void Dispose()
    {
-        var exitCode = SilentProcessRunner.ExecuteCommand(
+        // Dispose() cannot be made async; .GetAwaiter().GetResult() is safe here


This also doesn't follow our lighthouse comment example

jimmyp · 2026-05-25T03:31:07Z


            var chmodCmd = new CommandLineInvocation("/bin/bash", $"-c \"chmod 777 {scriptPath}\"");
-            chmodCmd.ExecuteCommand();
+            // Safe: sync test helper, no synchronisation context.


Does not follow our lighthouse comment example

Also there are many instances of this in this class. Can we create a helper method to dry it up or push this sync to async transition higher?

jimmyp · 2026-05-25T03:32:25Z

                log,
-                CancellationToken.None);
+                cancel: CancellationToken.None)
+                // Safe: static void helper, no synchronisation context.


Does not follow our lighthouse comment example

jimmyp · 2026-05-25T03:32:33Z

        {
            var commandLineInvocation = new CommandLineInvocation("/bin/bash", arguments);
-            var result = commandLineInvocation.ExecuteCommand();
+            // Safe: constructor-time helper, no synchronisation context.


Does not follow our lighthouse comment example

jimmyp · 2026-05-25T03:32:49Z

        }

+        // Sync-over-async is safe here: NUnit runs tests on a plain ThreadPool thread with no
+        // synchronisation context, so there is no risk of deadlock.


Does not follow our lighthouse comment example and is not at the actual call site

jimmyp · 2026-05-25T03:34:01Z

-            var result = commandLineInvocation.ExecuteCommand();
+            // We're in WriteUnitFile, called from IServiceConfigurator.ConfigureService implementations
+            // which are sync (called from the Tentacle service-management CLI), so we block on the
+            // async call with .GetAwaiter().GetResult().


ITs not clear why these things have to be sync?

jimmyp · 2026-05-25T03:34:18Z

+            // We're in WriteUnitFile, called from IServiceConfigurator.ConfigureService implementations
+            // which are sync (called from the Tentacle service-management CLI), so we block on the
+            // async call with .GetAwaiter().GetResult().
+            // This is sync-over-async but is safe because the CLI dispatches us on a plain


How do we know the cli dispatches us on a thread pool worker?

jimmyp · 2026-05-25T03:34:28Z

            var commandLineInvocation = new CommandLineInvocation("/bin/bash", "-c \"command -v systemctl >/dev/null\"");
-            var result = commandLineInvocation.ExecuteCommand();
+            // Same sync-over-async boundary as WriteUnitFile: CheckSystemPrerequisites is called
+            // from the sync ConfigureService path, on a plain thread-pool worker.


Does not follow our lighthouse comment example

jimmyp · 2026-05-25T03:34:39Z

        {
            var commandLineInvocation = new CommandLineInvocation("/bin/bash", "-c \"sudo -vn 2> /dev/null\"");
-            var result = commandLineInvocation.ExecuteCommand();
+            // Same sync-over-async boundary as IsSystemdInstalled.


Does not follow our lighthouse comment example

…, lighthouse comments, async tests - Strip SilentProcessRunner.cs to signature-only async change (move polyfill, EnableRaisingEvents, and await WaitForExitAsync to #1226 abandon PR) - Apply lighthouse comment pattern uniformly to all sync-over-async sites including test scaffolding and the second/third sites in LinuxServiceConfigurator - Improve LinuxServiceConfigurator justification: name the sync interface chain (IServiceConfigurator -> AbstractCommand -> ICommand -> Topshelf) and the no-SynchronizationContext property of Main/Topshelf worker threads - Move SilentProcessRunnerFixture sync-over-async comment to actual call site - Convert LinuxConfigureServiceHelperFixture test methods to async Task, eliminating sync-over-async entirely in that file Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

jimmyp · 2026-05-25T04:55:52Z

+                // synchronously, so we block on the async call with .GetAwaiter().GetResult().
+                // This is sync-over-async but is safe because the installer dispatches us on a
+                // plain thread-pool worker. No captured SynchronizationContext, so no deadlock.
+                // See https://blog.stephencleary.com/2012/07/dont-block-on-async-code.html


@LukeButters do you have enough knowledge of this repo to verify Claude claims on each of these? Or shall I organise us a sync to go through them one by one

Maybe @sburmanoctopus knows about this, he has done UI programming.

From my WPF days, blocking would cause the UI to freeze, which wasn't great.

When async/await came along, it was amazing, because you could do async stuff as long as you added .ConfigureAwait(true) to the end. This would ensure the logic returned to the calling thread, which was the UI thread. This freed up the UI thread for other things while async code was running, and didn't block the UI thread anymore!

So by default, I would suggest using .ConfigureAwait(true).

However, in saying that, I have no context on where this code is being called, how it's being called etc. It's possible something else is dealing with the thread locking issue, or this is being called from a place where we must block the UI thread.

LukeButters · 2026-05-25T05:35:59Z

+            // This is sync-over-async but is safe because the cache factory runs on a plain
+            // thread-pool worker. No captured SynchronizationContext, so no deadlock.
+            // See https://blog.stephencleary.com/2012/07/dont-block-on-async-code.html
+            var exitCode = silentProcessRunner.ExecuteCommandAsync("du", $"-s -B 1 {directoryPath}", "/", stdOut.Add, stdErr.Add)


@jimmyp Might want check with modern deployments on this one or push the async.

jimmyp and others added 6 commits May 25, 2026 12:17

jimmyp requested review from a team as code owners May 25, 2026 02:28

This was referenced May 25, 2026

Add EFT-3295 Tentacle script abandon design doc #1226

Open

Propagate async signature higher into CLI host and Kubernetes paths #1235

Closed

jimmyp commented May 25, 2026

View reviewed changes

LukeButters reviewed May 25, 2026

View reviewed changes

Conversation

jimmyp commented May 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What this PR does

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jimmyp commented May 25, 2026 •

edited

Loading