Skip to content
Merged
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
69 changes: 25 additions & 44 deletions src/Aspire.Cli/Interaction/ConsoleInteractionService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,28 @@ internal class ConsoleInteractionService : IInteractionService

private bool UsesConsoleLogging => _executionContext.ConsoleLogLevel is not null and not LogLevel.None;

private bool ShouldShowInteractiveStatus(string statusText)
{
// Use atomic check-and-set to prevent nested Spectre.Console Status operations.
// Spectre.Console throws if multiple interactive operations run concurrently.
// If already in a status, or in debug/non-interactive/console logging mode, fall back without a spinner.
// Also skip status display if statusText is empty (e.g., when outputting JSON).
// IMPORTANT: CompareExchange must be evaluated last so that short-circuit evaluation
// skips the swap when an earlier condition forces the fallback path. Otherwise the
// swap would set _inStatus to 1 but the try/finally that resets it would never run,
// permanently disabling interactive status for the lifetime of the service.
if (_executionContext.DebugMode ||
Comment thread
JamesNK marked this conversation as resolved.
UsesConsoleLogging ||
!_hostEnvironment.SupportsInteractiveOutput ||
string.IsNullOrEmpty(statusText) ||
Interlocked.CompareExchange(ref _inStatus, 1, 0) != 0)
{
return false;
}

return true;
}

public ConsoleInteractionService(ConsoleEnvironment consoleEnvironment, CliExecutionContext executionContext, ICliHostEnvironment hostEnvironment, IProcessPathProvider processPathProvider, ILoggerFactory loggerFactory, ConsoleLogBufferContext logBufferContext)
{
ArgumentNullException.ThrowIfNull(consoleEnvironment);
Expand All @@ -82,11 +104,6 @@ public ConsoleInteractionService(ConsoleEnvironment consoleEnvironment, CliExecu

public async Task<T> ShowStatusAsync<T>(string statusText, Func<Task<T>> action, KnownEmoji? emoji = null, bool allowMarkup = false)
{
if (UsesConsoleLogging)
{
return await action();
}

if (!allowMarkup)
{
statusText = statusText.EscapeMarkup();
Expand All @@ -97,17 +114,7 @@ public async Task<T> ShowStatusAsync<T>(string statusText, Func<Task<T>> action,
statusText = ConsoleHelpers.FormatEmojiPrefix(e, MessageConsole) + statusText;
}

// Use atomic check-and-set to prevent nested Spectre.Console Status operations.
// Spectre.Console throws if multiple interactive operations run concurrently.
// If already in a status or non-interactive mode is active, fall back to subtle message.
// Also skip status display if statusText is empty (e.g., when outputting JSON).
// IMPORTANT: CompareExchange must be evaluated last so that short-circuit evaluation
// skips the swap when an earlier condition forces the fallback path. Otherwise the
// swap would set _inStatus to 1 but the try/finally that resets it would never run,
// permanently disabling interactive status for the lifetime of the service.
if (!_hostEnvironment.SupportsInteractiveOutput ||
string.IsNullOrEmpty(statusText) ||
Interlocked.CompareExchange(ref _inStatus, 1, 0) != 0)
if (!ShouldShowInteractiveStatus(statusText))
Comment thread
JamesNK marked this conversation as resolved.
{
// Skip displaying if status text is empty (e.g., when outputting JSON)
if (!string.IsNullOrEmpty(statusText))
Expand Down Expand Up @@ -136,22 +143,10 @@ public async Task<T> ShowStatusAsync<T>(string statusText, Func<Task<T>> action,

public async Task<T> ShowDynamicStatusAsync<T>(string initialStatusText, Func<Action<string>, Task<T>> action, KnownEmoji? emoji = null)
{
if (UsesConsoleLogging)
{
return await action(_ => { });
}

var emojiPrefix = emoji is { } e ? ConsoleHelpers.FormatEmojiPrefix(e, MessageConsole) : string.Empty;
var initialDisplayText = emojiPrefix + initialStatusText.EscapeMarkup();

// Mirrors ShowStatusAsync: prevent nested Spectre.Console Status operations, skip with non-interactive output,
// and treat empty text as "no status UI". The fallback path still drives the action so progress logic runs;
// we just hand it an updater that emits subtle messages instead of mutating a live spinner.
// IMPORTANT: CompareExchange must be evaluated last so that short-circuit evaluation skips the swap when
// an earlier condition forces the fallback path; otherwise _inStatus would be left set to 1.
if (!_hostEnvironment.SupportsInteractiveOutput ||
string.IsNullOrEmpty(initialStatusText) ||
Interlocked.CompareExchange(ref _inStatus, 1, 0) != 0)
if (!ShouldShowInteractiveStatus(initialStatusText))
{
if (!string.IsNullOrEmpty(initialStatusText))
{
Expand Down Expand Up @@ -185,12 +180,6 @@ public async Task<T> ShowDynamicStatusAsync<T>(string initialStatusText, Func<Ac

public void ShowStatus(string statusText, Action action, KnownEmoji? emoji = null, bool allowMarkup = false)
{
if (UsesConsoleLogging)
{
action();
return;
}

MessageLogger.LogInformation("Status: {StatusText}", statusText);

if (!allowMarkup)
Expand All @@ -203,15 +192,7 @@ public void ShowStatus(string statusText, Action action, KnownEmoji? emoji = nul
statusText = ConsoleHelpers.FormatEmojiPrefix(e, MessageConsole) + statusText;
}

// Use atomic check-and-set to prevent nested Spectre.Console Status operations.
// Spectre.Console throws if multiple interactive operations run concurrently.
// If already in a status or non-interactive mode is active, fall back to subtle message.
// Also skip status display if statusText is empty (e.g., when outputting JSON).
// IMPORTANT: CompareExchange must be evaluated last so that short-circuit evaluation skips the swap when
// an earlier condition forces the fallback path; otherwise _inStatus would be left set to 1.
if (!_hostEnvironment.SupportsInteractiveOutput ||
string.IsNullOrEmpty(statusText) ||
Interlocked.CompareExchange(ref _inStatus, 1, 0) != 0)
if (!ShouldShowInteractiveStatus(statusText))
{
if (!string.IsNullOrEmpty(statusText))
{
Expand Down
11 changes: 7 additions & 4 deletions tests/Aspire.Cli.EndToEnd.Tests/DoctorCommandTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -103,14 +103,17 @@ await auto.WaitUntilAsync(s =>
}

[Fact]
public async Task DoctorCommand_WithTraceLogging_DoesNotRenderProgress()
public async Task DoctorCommand_WithDebugLogging_DoesNotRenderSpinner()
{
var repoRoot = CliE2ETestHelpers.GetRepoRoot();
var strategy = CliInstallStrategy.Detect(output.WriteLine);
var workspace = TemporaryWorkspace.Create(output);
var testName = nameof(DoctorCommand_WithTraceLogging_DoesNotRenderProgress);
var testName = nameof(DoctorCommand_WithDebugLogging_DoesNotRenderSpinner);
var recordingPath = CliE2ETestHelpers.GetTestResultsRecordingPath(testName);

// The recording path is stable across retries, so remove stale output before the recorder starts.
File.Delete(recordingPath);

using var terminal = CliE2ETestHelpers.CreateDockerTestTerminal(repoRoot, strategy, output, workspace: workspace, testName: testName);

var counter = new SequenceCounter();
Expand All @@ -122,14 +125,14 @@ public async Task DoctorCommand_WithTraceLogging_DoesNotRenderProgress()
await auto.ClearScreenAsync(counter);

var recordingOffset = File.Exists(recordingPath) ? new FileInfo(recordingPath).Length : 0;
await auto.TypeAsync("aspire doctor -l trace");
await auto.TypeAsync("aspire doctor -l debug");
await auto.EnterAsync();
await auto.WaitForSuccessPromptAsync(counter, TimeSpan.FromMinutes(2));

var commandOutput = ReadRecordingOutput(recordingPath, recordingOffset);
Assert.Contains("[dbug]", commandOutput, StringComparison.Ordinal);
Assert.Contains(DoctorCommandStrings.EnvironmentCheckHeader, commandOutput, StringComparison.Ordinal);
Assert.DoesNotContain(DoctorCommandStrings.CheckingPrerequisites, commandOutput, StringComparison.Ordinal);
Assert.Contains(DoctorCommandStrings.CheckingPrerequisites, commandOutput, StringComparison.Ordinal);
Assert.DoesNotContain(commandOutput, SpinnerCharacters.Contains);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,11 @@ public class ConsoleInteractionServiceTests
private static CliExecutionContext CreateExecutionContext(bool debugMode = false, LogLevel? consoleLogLevel = null, string? logFilePath = null) =>
new(new DirectoryInfo("."), new DirectoryInfo("."), new DirectoryInfo("."), s_runtimeDirectory, s_logsDirectory, logFilePath ?? "test.log", identityChannel: "local", debugMode: debugMode, consoleLogLevel: consoleLogLevel);

private static ConsoleInteractionService CreateInteractionService(IAnsiConsole console, CliExecutionContext? executionContext = null, ICliHostEnvironment? hostEnvironment = null)
private static ConsoleInteractionService CreateInteractionService(IAnsiConsole console, CliExecutionContext? executionContext = null, ICliHostEnvironment? hostEnvironment = null, ILoggerFactory? loggerFactory = null)
{
executionContext ??= CreateExecutionContext();
var consoleEnvironment = new ConsoleEnvironment(console, console);
return new ConsoleInteractionService(consoleEnvironment, executionContext, hostEnvironment ?? TestHelpers.CreateInteractiveHostEnvironment(), new EnvironmentProcessPathProvider(), NullLoggerFactory.Instance, new ConsoleLogBufferContext());
return new ConsoleInteractionService(consoleEnvironment, executionContext, hostEnvironment ?? TestHelpers.CreateInteractiveHostEnvironment(), new EnvironmentProcessPathProvider(), loggerFactory ?? NullLoggerFactory.Instance, new ConsoleLogBufferContext());
}

[Fact]
Expand Down Expand Up @@ -274,16 +274,38 @@ public async Task ShowStatusAsync_InDebugMode_DisplaysSubtleMessageInsteadOfSpin
var executionContext = CreateExecutionContext(debugMode: true);
var interactionService = CreateInteractionService(console, executionContext);
var statusText = "Processing request...";
var result = "test result";

// Act
var actualResult = await interactionService.ShowStatusAsync(statusText, () => Task.FromResult(result)).DefaultTimeout();
var statusValue = await interactionService.ShowStatusAsync(statusText, () => Task.FromResult(GetInStatus(interactionService))).DefaultTimeout();

// Assert
Assert.Equal(result, actualResult);
Assert.Equal(0, statusValue);
var outputString = output.ToString();
Assert.Contains(statusText, outputString);
// In debug mode, should use DisplaySubtleMessage instead of spinner
}

[Fact]
public async Task ShowDynamicStatusAsync_InDebugMode_DisplaysSubtleMessagesInsteadOfSpinner()
{
var output = new StringBuilder();
var console = AnsiConsole.Create(new AnsiConsoleSettings
{
Ansi = AnsiSupport.No,
ColorSystem = ColorSystemSupport.NoColors,
Out = new AnsiConsoleOutput(new StringWriter(output))
});

var interactionService = CreateInteractionService(console, CreateExecutionContext(debugMode: true));

var statusValue = await interactionService.ShowDynamicStatusAsync("Processing request...", updateStatus =>
{
updateStatus("Still processing...");
return Task.FromResult(GetInStatus(interactionService));
}).DefaultTimeout();

Assert.Equal(0, statusValue);
Assert.Contains("Processing request...", output.ToString());
Assert.Contains("Still processing...", output.ToString());
}

[Theory]
Expand All @@ -296,13 +318,14 @@ public async Task ShowStatusAsync_InDebugMode_DisplaysSubtleMessageInsteadOfSpin
public async Task StatusMethods_WithConsoleLogging_DoNotStartSpinner(LogLevel consoleLogLevel)
{
var output = new StringBuilder();
using var loggerFactory = LoggerFactory.Create(builder => builder.AddProvider(new SpectreConsoleLoggerProvider(new StringWriter(output), new ConsoleLogBufferContext())));
var console = AnsiConsole.Create(new AnsiConsoleSettings
{
Ansi = AnsiSupport.No,
ColorSystem = ColorSystemSupport.NoColors,
Out = new AnsiConsoleOutput(new StringWriter(output))
});
var interactionService = CreateInteractionService(console, CreateExecutionContext(consoleLogLevel: consoleLogLevel));
var interactionService = CreateInteractionService(console, CreateExecutionContext(consoleLogLevel: consoleLogLevel), loggerFactory: loggerFactory);

var asyncStatusValue = await interactionService.ShowStatusAsync("Working...", () => Task.FromResult(GetInStatus(interactionService))).DefaultTimeout();
var dynamicStatusValue = await interactionService.ShowDynamicStatusAsync("Working...", updateStatus =>
Expand All @@ -316,7 +339,8 @@ public async Task StatusMethods_WithConsoleLogging_DoNotStartSpinner(LogLevel co
Assert.Equal(0, asyncStatusValue);
Assert.Equal(0, dynamicStatusValue);
Assert.Equal(0, synchronousStatusValue);
Assert.Empty(output.ToString());
Assert.Contains("Working...", output.ToString());
Assert.Contains("Still working...", output.ToString());
}

[Fact]
Expand Down Expand Up @@ -344,16 +368,15 @@ public void ShowStatus_InDebugMode_DisplaysSubtleMessageInsteadOfSpinner()
var executionContext = CreateExecutionContext(debugMode: true);
var interactionService = CreateInteractionService(console, executionContext);
var statusText = "Processing synchronous request...";
var actionCalled = false;
var statusValue = -1;

// Act
interactionService.ShowStatus(statusText, () => actionCalled = true);
interactionService.ShowStatus(statusText, () => statusValue = GetInStatus(interactionService));

// Assert
Assert.True(actionCalled);
Assert.Equal(0, statusValue);
var outputString = output.ToString();
Assert.Contains(statusText, outputString);
// In debug mode, should use DisplaySubtleMessage instead of spinner
}

[Fact]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ public void SpectreConsoleLogger_IsEnabled_FiltersCorrectly()
var systemLogger = new SpectreConsoleLogger(output, "System.Test", bufferContext);

// Act & Assert
Assert.False(aspireLogger.IsEnabled(LogLevel.Trace));
Assert.True(aspireLogger.IsEnabled(LogLevel.Debug));
Assert.True(aspireLogger.IsEnabled(LogLevel.Information));
Assert.True(aspireLogger.IsEnabled(LogLevel.Warning));
Expand Down
Loading