From 4e88a9e34ac63ab3851edd070f108fcad09d4fb8 Mon Sep 17 00:00:00 2001 From: Sebastian Wittlich Date: Fri, 21 Aug 2026 13:11:43 +0200 Subject: [PATCH 1/2] Add implementation from new interface proposed in https://github.com/one-ware/OneWare/pull/331 --- .../ContainerExtensionModule.cs | 6 +- .../DockerExecutionStrategy.cs | 110 ++++++++++++++++++ .../ContainerExtension.UnitTests.csproj | 10 +- .../QualityVerificationTests.cs | 72 ++++++++++++ 4 files changed, 193 insertions(+), 5 deletions(-) diff --git a/src/ContainerExtension/ContainerExtensionModule.cs b/src/ContainerExtension/ContainerExtensionModule.cs index b8e52a0..4963db6 100644 --- a/src/ContainerExtension/ContainerExtensionModule.cs +++ b/src/ContainerExtension/ContainerExtensionModule.cs @@ -558,12 +558,14 @@ private static void InjectStrategyIntoAllTools(IToolService toolService, DockerE var allTools = toolService.GetAllTools(); if (allTools == null) return; + // Strategy-side opt-in: register the Docker strategy once with a predicate matching every tool — + // including tools registered later, which the tool service re-evaluates on demand. + toolService.RegisterStrategy(dockerStrategy, static _ => true); + foreach (var globalTool in allTools) { if (globalTool == null || string.IsNullOrEmpty(globalTool.Key)) continue; - toolService.RegisterStrategy(globalTool.Key, dockerStrategy); - var settingKey = $"{PerToolImagePrefix}{globalTool.Key.ToLowerInvariant()}"; if (!settingsService.HasSetting(settingKey)) { diff --git a/src/ContainerExtension/DockerExecutionStrategy.cs b/src/ContainerExtension/DockerExecutionStrategy.cs index fefd661..f2fcb3c 100644 --- a/src/ContainerExtension/DockerExecutionStrategy.cs +++ b/src/ContainerExtension/DockerExecutionStrategy.cs @@ -54,6 +54,12 @@ public sealed partial class DockerExecutionStrategy : IToolExecutionStrategy, ID private readonly CancellationTokenSource _strategyCts = new(); private int _disposed; + // Background runs started via StartProcess, keyed by the opaque handle handed back to the caller. Each + // value is that run's CancellationTokenSource (linked to _strategyCts) so StopProcess and Dispose can + // cancel it. Membership is the liveness signal: a run removes its own entry when it finishes, so a key + // present in this map means "still running". + private readonly ConcurrentDictionary _backgroundRuns = new(); + private void ThrowIfDisposed() { if (Volatile.Read(ref _disposed) == 1) @@ -310,6 +316,92 @@ public WeakReference StartWeakProcess(ToolCommand command) } } + /// + /// Starts as a tracked, long-running background container run and returns an + /// opaque handle. Unlike (which exposes a host sentinel + /// ), a Docker run has no host process, so the run is tracked by handle: cancel it + /// with or query it with . The run + /// removes its own entry when it completes. + /// + public Guid StartProcess(ToolCommand command) + { + ThrowIfDisposed(); + + var handle = Guid.NewGuid(); + // Linked to the strategy token so Dispose (which cancels _strategyCts) tears down in-flight runs. + var runCts = CancellationTokenSource.CreateLinkedTokenSource(_strategyCts.Token); + _backgroundRuns[handle] = runCts; + + _ = Task.Run(async () => + { + try + { + await ExecuteAsync(command, runCts.Token).ConfigureAwait(false); + } + catch (Exception ex) + { + try + { + ContainerTelemetry.TrackError("DockerExecutionStrategy", "StartProcess background task crashed", ex, command.Executable); + var errMsg = $"[ERROR] Execution of background task '{command.Executable}' failed: {ex.Message}"; + SafeInvoke(() => + { + (command.ErrorHandler ?? command.OutputHandler)?.Invoke(errMsg); + }); + } + catch (Exception) + { + // Exception is intentionally ignored because error handling failure during shutdown/crash is non-critical. + } + } + finally + { + // Removing the entry marks the run as finished; the disposer of runCts is always this task's + // finally, so StopProcess only ever cancels (never disposes) and no double-dispose can occur. + _backgroundRuns.TryRemove(handle, out _); + try + { + runCts.Dispose(); + } + catch (ObjectDisposedException) + { + // Exception is intentionally ignored because the source may already be disposed during shutdown. + } + } + }, CancellationToken.None); + + return handle; + } + + /// + /// Stops a background run previously started with by cancelling + /// its token; the run then tears its container down cooperatively. Returns true if a live run was + /// found for , otherwise false. + /// + public bool StopProcess(Guid handle) + { + if (!_backgroundRuns.TryRemove(handle, out var runCts)) + { + return false; + } + + try + { + runCts.Cancel(); + } + catch (ObjectDisposedException) + { + // The background run completed and disposed its own CTS between the TryRemove and this Cancel. + } + return true; + } + + /// + /// Returns whether a background run started with is still + /// tracked for . A completed or stopped run is no longer tracked. + /// + public bool IsProcessRunning(Guid handle) => _backgroundRuns.ContainsKey(handle); + public string GetStrategyName() => "Docker Container (DotNet API)"; public string GetStrategyKey() => ToolKey; @@ -321,6 +413,24 @@ public void Dispose() return; } + // Cancel any tracked background runs before tearing down the strategy CTS they are linked to. Each + // run's own finally disposes its CTS and removes its entry, so here we only cancel (guarded against a + // run that just finished and disposed its CTS). + foreach (var handle in _backgroundRuns.Keys) + { + if (_backgroundRuns.TryRemove(handle, out var runCts)) + { + try + { + runCts.Cancel(); + } + catch (ObjectDisposedException) + { + // Exception is intentionally ignored because the run disposed its own CTS as it completed. + } + } + } + try { _strategyCts.Cancel(); diff --git a/tests/ContainerExtension.UnitTests/ContainerExtension.UnitTests.csproj b/tests/ContainerExtension.UnitTests/ContainerExtension.UnitTests.csproj index e06970a..6440b8e 100644 --- a/tests/ContainerExtension.UnitTests/ContainerExtension.UnitTests.csproj +++ b/tests/ContainerExtension.UnitTests/ContainerExtension.UnitTests.csproj @@ -20,9 +20,13 @@ - - + + + + + diff --git a/tests/ContainerExtension.UnitTests/QualityVerificationTests.cs b/tests/ContainerExtension.UnitTests/QualityVerificationTests.cs index cc0c557..6c677e4 100644 --- a/tests/ContainerExtension.UnitTests/QualityVerificationTests.cs +++ b/tests/ContainerExtension.UnitTests/QualityVerificationTests.cs @@ -106,6 +106,78 @@ public async Task StartWeakProcess_KillCall_CancelsExecution() } } + // StartProcess hands back a real, non-empty handle synchronously (the background container run is + // launched fire-and-forget). Daemon-independent: an unreachable daemon still yields a valid handle. + [Fact] + public void StartProcess_ReturnsNonEmptyHandle() + { + using var provider = new E2ETestServiceProvider(); + using var strategy = new DockerExecutionStrategy(provider); + + var command = new ToolCommand + { + Executable = "echo", + ToolName = "echo", + CommandArguments = BuildArgs("hello") + }; + + var handle = strategy.StartProcess(command); + + Assert.NotEqual(Guid.Empty, handle); + } + + // An unknown handle is not tracked, so both queries report "not running" / "nothing to stop" rather + // than throwing. Daemon-independent. + [Fact] + public void IsProcessRunning_AndStopProcess_UnknownHandle_ReturnFalse() + { + using var provider = new E2ETestServiceProvider(); + using var strategy = new DockerExecutionStrategy(provider); + + var unknown = Guid.NewGuid(); + + Assert.False(strategy.IsProcessRunning(unknown)); + Assert.False(strategy.StopProcess(unknown)); + } + + // Full lifecycle against a real daemon: a started run is tracked (IsProcessRunning == true), StopProcess + // finds and cancels it (returns true, then untracked), and the cancellation surfaces to the error + // handler. Requires a reachable Docker daemon, so it is gated out of CI like the E2E suite. + [FactIfNoCI] + public async Task StopProcess_CancelsRunningProcess_ReturnsTrue() + { + using var provider = new E2ETestServiceProvider(); + provider.SettingsService.SetSettingValue(ContainerExtensionModule.DefaultImageSetting, ContainerExtensionModule.FallbackImage); + using var strategy = new DockerExecutionStrategy(provider); + + var stderrList = new List(); + var command = new ToolCommand + { + Executable = "sleep", + ToolName = "sleep", + CommandArguments = BuildArgs("30"), + ErrorHandler = msg => { lock (stderrList) stderrList.Add(msg); return true; } + }; + + var handle = strategy.StartProcess(command); + Assert.NotEqual(Guid.Empty, handle); + + // Give the container run time to reach the running state (still tracked while pulling/executing). + await Task.Delay(3000, TestContext.Current.CancellationToken); + Assert.True(strategy.IsProcessRunning(handle)); + + // Stopping a tracked run returns true and immediately drops it from tracking. + Assert.True(strategy.StopProcess(handle)); + Assert.False(strategy.IsProcessRunning(handle)); + + // Cancellation should propagate into the run and surface via the error handler. + await Task.Delay(3000, TestContext.Current.CancellationToken); + lock (stderrList) + { + Assert.Contains(stderrList, line => line.Contains("cancel", StringComparison.OrdinalIgnoreCase)); + } + } + [Fact] public void LazyInitialization_DoesNotBlockUIOrPropertyGetters() { From 25c0865f911c9048f7debc5ebe34a7bbd388b78c Mon Sep 17 00:00:00 2001 From: Sebastian Wittlich Date: Fri, 21 Aug 2026 14:46:05 +0200 Subject: [PATCH 2/2] Remove CheckBox --- .../ContainerExtensionModule.cs | 48 +++---------------- 1 file changed, 7 insertions(+), 41 deletions(-) diff --git a/src/ContainerExtension/ContainerExtensionModule.cs b/src/ContainerExtension/ContainerExtensionModule.cs index 4963db6..6648aae 100644 --- a/src/ContainerExtension/ContainerExtensionModule.cs +++ b/src/ContainerExtension/ContainerExtensionModule.cs @@ -280,29 +280,20 @@ public override void Initialize(IServiceProvider serviceProvider) using var timer = new PeriodicTimer(TimeSpan.FromSeconds(5)); try { - var strategyKey = dockerStrategy.GetStrategyKey(); while (await timer.WaitForNextTickAsync(ct).ConfigureAwait(false)) { if (ct.IsCancellationRequested) break; // Isolate each tick: a transient fault in one scan must not tear down the - // poller, otherwise late-registered tools would silently never receive the - // strategy until the next IDE restart. + // poller, otherwise late-registered tools would silently never receive their + // per-tool image setting until the next IDE restart. try { - var currentTools = toolService.GetAllTools(); - var needsInjection = false; - foreach (var tool in currentTools) - { - if (settingsService.HasSetting(tool.Key) && settingsService.GetSetting(tool.Key) is ComboBoxSetting comboSetting && (comboSetting.Options == null || comboSetting.Options.Length == 0 || !OptionsContains(comboSetting.Options, strategyKey))) - { - needsInjection = true; - break; - } - } - - var currentToolCount = currentTools.Count; - if (currentToolCount != knownToolCount || needsInjection) + // The Docker strategy is registered once for all tools — current and future — via the + // predicate registration, so late tools need no strategy re-injection, only their + // per-tool image setting, which InjectStrategyIntoAllTools creates when the count grows. + var currentToolCount = toolService.GetAllTools().Count; + if (currentToolCount != knownToolCount) { knownToolCount = currentToolCount; Avalonia.Threading.Dispatcher.UIThread.Post(() => @@ -579,34 +570,9 @@ private static void InjectStrategyIntoAllTools(IToolService toolService, DockerE } ); } - - if (settingsService.HasSetting(globalTool.Key) && settingsService.GetSetting(globalTool.Key) is ComboBoxSetting comboSetting) - { - var strategyKey = dockerStrategy.GetStrategyKey(); - if (!OptionsContains(comboSetting.Options, strategyKey)) - { - var newOptions = new object[comboSetting.Options.Length + 1]; - Array.Copy(comboSetting.Options, newOptions, comboSetting.Options.Length); - newOptions[^1] = strategyKey; - comboSetting.Options = newOptions; - } - } } } - private static bool OptionsContains(object[] options, string value) - { - if (options == null || options.Length == 0) return false; - for (int idx = 0; idx < options.Length; idx++) - { - if (options[idx] is string str && string.Equals(str, value, StringComparison.Ordinal)) - { - return true; - } - } - return false; - } - /// /// Safely terminates all background execution threads, UI handlers, and releases container process subscriptions. ///