Skip to content

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

Closed
jimmyp wants to merge 7 commits into
jimpelletier/eft-3295-tentacle-script-abandonment-to-release-the-mutexfrom
jimpelletier/eft-3295-async-signature-propagation
Closed

Propagate async signature higher into CLI host and Kubernetes paths#1235
jimmyp wants to merge 7 commits into
jimpelletier/eft-3295-tentacle-script-abandonment-to-release-the-mutexfrom
jimpelletier/eft-3295-async-signature-propagation

Conversation

@jimmyp
Copy link
Copy Markdown

@jimmyp jimmyp commented May 25, 2026

Summary

Stacks on top of EFT-3295 (the script abandonment PR) to propagate the async signature higher up the call stack — eliminating .GetAwaiter().GetResult() calls that were left documented in the abandon PR.

  • Kubernetes disk-space check chain — IKubernetesDirectoryInformationProvider.GetPathUsedBytesAsync, KubernetesPhysicalFileSystem.GetStorageInformationAsync, awaited from KubernetesScriptPodCreator
  • Service-management CLI command chain — IServiceConfigurator.Configure*Async, LinuxServiceConfigurator, WindowsServiceConfigurator, SystemCtlHelper.RunServiceCommandAsync
  • CLI host async dispatch — ICommand.StartAsync, ICommandHost.RunAsync, Program.Main becomes async Task<int>, sync Start/Run paths removed
  • ICommandLineRunner — fully async via ExecuteAsync; WPF installer's ReviewAndRunScriptTabViewModel and RunProcessDialog updated

Test plan

  • Verify Tentacle agent starts via Topshelf (Windows service host)
  • Verify Kubernetes agent starts and runs scripts
  • Verify WPF installer can run the setup wizard
  • CI build passes

🤖 Generated with Claude Code

@jimmyp jimmyp requested review from a team as code owners May 25, 2026 01:25
@jimmyp jimmyp force-pushed the jimpelletier/eft-3295-tentacle-script-abandonment-to-release-the-mutex branch from 583eb46 to 89077d9 Compare May 25, 2026 02:37
jimmyp and others added 7 commits May 25, 2026 12:41
@jimmyp @claude
Propagate async through Kubernetes disk-space check chain
8b894c0 
GetDriveBytesUsingDu was blocking on ExecuteCommandAsync via
.GetAwaiter().GetResult() because GetPathUsedBytes was called from a
sync IMemoryCache.GetOrCreate factory. The chain is now fully async:

  GetDriveBytesUsingDuAsync → GetPathUsedBytesAsync (GetOrCreateAsync)
  → GetStorageInformationAsync → awaited by CreateScriptContainer

EnsureDiskHasEnoughFreeSpace still blocks (it overrides a sync
IOctopusFileSystem method called from config writes and script workspace
readiness checks), but the sync boundary is now at the override where
the constraint is obvious, not buried inside the du runner.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jimmyp @claude
Propagate async through the service-management CLI command chain
0405f4e 
AbstractCommand gets a StartAsync() virtual hook (defaults to calling
Start() so all existing sync commands need no changes). ServiceCommand
overrides StartAsync() instead of Start(), allowing the whole chain
below it to be properly async:

  ServiceCommand.StartAsync()
    -> IServiceConfigurator.Configure*Async()
    -> LinuxServiceConfigurator / WindowsServiceConfigurator (async)
    -> SystemCtlHelper.RunServiceCommandAsync()
    -> await ExecuteCommandAsync()

Six .GetAwaiter().GetResult() calls removed (2 in SystemCtlHelper,
3 in LinuxServiceConfigurator, 1 in WindowsServiceConfigurator).
Replaced by one sync boundary in AbstractCommand's dispatcher where
the CLI host (OctopusProgram) requires a sync Action<ICommandRuntime>.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jimmyp @claude
Push async all the way to async Task<int> Main
18088ba 
Adds StartAsync(string[], ICommandRuntime, OptionSet) to ICommand and
ICommandHost.RunAsync(Func<ICommandRuntime, Task>, Action) to all four
host implementations. OctopusProgram.Run() delegates to RunAsync();
Program.Main becomes async Task<int>.

The single remaining sync boundary is AbstractCommand.Start(string[]...)
which calls StartAsync(...).GetAwaiter().GetResult() — kept for backwards
compatibility with any callers that hold an ICommand and call Start()
synchronously, but OctopusProgram no longer goes through it.

HelpCommand migrated to override StartAsync(string[]...) instead of
Start(string[]...) for consistency.

WindowsServiceHost.RunAsync uses Task.Run for ServiceBase.Run because
that Windows API must block a thread by design.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jimmyp @claude
Remove sync ICommand.Start and ICommandHost.Run entirely
cc4e531 
ICommand.void Start(string[],...) and ICommandHost.void Run(Action,...)
are now deleted — no production or test code calls them. All callers
have been migrated to StartAsync / RunAsync.

NoninteractiveHost.ManualResetEvent removed (only served the sync Run
path). Three test fixtures (ServerCommsCommandTest, CheckServicesCommand
Fixture, WatchdogCommandFixture) migrated to async Task tests using
StartAsync and Assert.ThrowsAsync.

Two Kubernetes integration test calls also updated to await StartAsync.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jimmyp @claude
Make ICommandLineRunner fully async (Site 6)
1d3b8bc 
ICommandLineRunner.Execute → ExecuteAsync (Task<bool>). CommandLineRunner
now awaits ExecuteCommandAsync directly — no .GetAwaiter().GetResult().

Callers updated:
  - ReviewAndRunScriptTabViewModel: already async Task<bool>; Rollback
    becomes async Task RollbackAsync
  - RunProcessDialog: ThreadPool.QueueUserWorkItem → Task.Run(async)
  - SetupTentacleWizardModelBuilder test: NSubstitute mock updated

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jimmyp @claude
Remove remaining GetAwaiter().GetResult() from command classes
9cdbc5a 
Five commands (PollCommand, RegisterMachineCommandBase, ShowConfiguration,
DeregisterMachine, DeregisterWorker) used a pre-existing pattern where
protected Start() wrapped a private async Task StartAsync(). Now that
AbstractCommand.StartAsync(string[],...) calls the protected virtual
StartAsync() directly, they override that instead — no GetAwaiter needed.

Also remove OctopusProgram.Run() (dead code — Main now calls RunAsync()).

Remaining GetAwaiter().GetResult() sites are all pre-existing forced sync
boundaries: IWritableKeyValueStore, IMachineKeyEncryptor (Kubernetes config),
IOctopusFileSystem override, and WindowsServiceHost (ServiceBase.Run
must block). All have been documented in prior commits or were pre-existing.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jimmyp @claude
Add .worktrees/ to .gitignore
5f2ac43 
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jimmyp jimmyp force-pushed the jimpelletier/eft-3295-async-signature-propagation branch from a1bc78f to 5f2ac43 Compare May 25, 2026 02:43
@jimmyp
Copy link
Copy Markdown
Author

jimmyp commented May 25, 2026

Rebased on top of the restructured #1226 (which now stacks on the new async-migration base PR #1236). End state unchanged from previous tip (preserved at tag claude-safety-2026-05-25-pre-split-1235).

@jimmyp
Copy link
Copy Markdown
Author

jimmyp commented May 25, 2026

Closing — the deeper async-signature propagation work isn't being pursued. The base async migration (#1236) and the abandon feature (#1226) are the two PRs going forward.

@jimmyp jimmyp closed this May 25, 2026
@jimmyp jimmyp deleted the jimpelletier/eft-3295-async-signature-propagation branch May 25, 2026 03:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@jimmyp