Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 11 additions & 43 deletions src/ContainerExtension/ContainerExtensionModule.cs
Original file line number Diff line number Diff line change
Expand Up @@ -280,29 +280,20 @@
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(() =>
Expand Down Expand Up @@ -558,12 +549,14 @@
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);

Check failure on line 554 in src/ContainerExtension/ContainerExtensionModule.cs

View workflow job for this annotation

GitHub Actions / Format

Argument 1: cannot convert from 'ContainerExtension.DockerExecutionStrategy' to 'string'

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))
{
Expand All @@ -577,32 +570,7 @@
}
);
}

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;
}

/// <summary>
Expand Down
110 changes: 110 additions & 0 deletions src/ContainerExtension/DockerExecutionStrategy.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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<Guid, CancellationTokenSource> _backgroundRuns = new();

private void ThrowIfDisposed()
{
if (Volatile.Read(ref _disposed) == 1)
Expand Down Expand Up @@ -310,6 +316,92 @@ public WeakReference<Process> StartWeakProcess(ToolCommand command)
}
}

/// <summary>
/// Starts <paramref name="command"/> as a tracked, long-running background container run and returns an
/// opaque handle. Unlike <see cref="StartWeakProcess(ToolCommand)"/> (which exposes a host sentinel
/// <see cref="Process"/>), a Docker run has no host process, so the run is tracked by handle: cancel it
/// with <see cref="StopProcess(Guid)"/> or query it with <see cref="IsProcessRunning(Guid)"/>. The run
/// removes its own entry when it completes.
/// </summary>
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;
}

/// <summary>
/// Stops a background run previously started with <see cref="StartProcess(ToolCommand)"/> by cancelling
/// its token; the run then tears its container down cooperatively. Returns <c>true</c> if a live run was
/// found for <paramref name="handle"/>, otherwise <c>false</c>.
/// </summary>
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;
}

/// <summary>
/// Returns whether a background run started with <see cref="StartProcess(ToolCommand)"/> is still
/// tracked for <paramref name="handle"/>. A completed or stopped run is no longer tracked.
/// </summary>
public bool IsProcessRunning(Guid handle) => _backgroundRuns.ContainsKey(handle);

public string GetStrategyName() => "Docker Container (DotNet API)";

public string GetStrategyKey() => ToolKey;
Expand All @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,13 @@
<ProjectReference Include="..\..\src\ContainerExtension\ContainerExtension.csproj" />
</ItemGroup>

<ItemGroup>
<!-- OneWare.Essentials is excluded from runtime in the main project (host provides it),
but the test runner needs it to resolve ISettingValidation for validator tests. -->
<!-- OneWare.Essentials is excluded from runtime in the main project (the host provides it), but the
test runner has no host, so it needs the real runtime assembly (e.g. to resolve ISettingValidation
for validator tests). Mirror the main project's local/NuGet toggle, but keep runtime assets. -->
<ItemGroup Condition="'$(UseLocalEssentials)' == 'true'">
<ProjectReference Include="..\..\..\OneWare\src\OneWare.Essentials\OneWare.Essentials.csproj" />
</ItemGroup>
<ItemGroup Condition="'$(UseLocalEssentials)' != 'true'">
<PackageReference Include="OneWare.Essentials" />
</ItemGroup>

Expand Down
72 changes: 72 additions & 0 deletions tests/ContainerExtension.UnitTests/QualityVerificationTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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<string>();
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()
{
Expand Down
Loading