From 6992a5a597d7819806f0869c05cb72591bcb8850 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 3 Sep 2026 13:14:33 +0000 Subject: [PATCH 01/10] Initial plan From 603061a0a54fd9c4c8ff2528442e04e400cc92da Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 3 Sep 2026 13:18:24 +0000 Subject: [PATCH 02/10] Tolerate concurrent access to configuration files Co-authored-by: isourabh <2982389+isourabh@users.noreply.github.com> --- .../ConfigurationManagerUnitTests.cs | 92 +++++++++++++++++++ MSStore.CLI/Program.cs | 11 ++- MSStore.CLI/Services/ConfigurationManager.cs | 41 ++++++++- 3 files changed, 140 insertions(+), 4 deletions(-) create mode 100644 MSStore.CLI.UnitTests/ConfigurationManagerUnitTests.cs diff --git a/MSStore.CLI.UnitTests/ConfigurationManagerUnitTests.cs b/MSStore.CLI.UnitTests/ConfigurationManagerUnitTests.cs new file mode 100644 index 0000000..f780afe --- /dev/null +++ b/MSStore.CLI.UnitTests/ConfigurationManagerUnitTests.cs @@ -0,0 +1,92 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using MSStore.CLI.Services; +using MSStore.CLI.Services.Telemetry; + +namespace MSStore.CLI.UnitTests +{ + [TestClass] + public class ConfigurationManagerUnitTests + { + private ConfigurationManager _configurationManager = null!; + + public TestContext TestContext { get; set; } = null!; + + [TestInitialize] + public void Initialize() + { + _configurationManager = new ConfigurationManager( + TelemetrySourceGenerationContext.Default.TelemetryConfigurations, + $"test_{Guid.NewGuid()}.json", + null); + } + + [TestCleanup] + public void Cleanup() + { + if (File.Exists(_configurationManager.ConfigPath)) + { + File.Delete(_configurationManager.ConfigPath); + } + } + + [TestMethod] + public async Task SaveAsyncWaitsForOtherProcessToReleaseTheFile() + { + await _configurationManager.ClearAsync(TestContext.CancellationToken); + + var otherProcessFile = File.Open(_configurationManager.ConfigPath, FileMode.Open, FileAccess.ReadWrite, FileShare.None); + + var saveTask = _configurationManager.SaveAsync(new TelemetryConfigurations { TelemetryEnabled = true }, TestContext.CancellationToken); + + await Task.Delay(100, TestContext.CancellationToken); + + otherProcessFile.Dispose(); + + await saveTask; + + var telemetryConfigurations = await _configurationManager.LoadAsync(true, TestContext.CancellationToken); + + telemetryConfigurations.TelemetryEnabled.Should().BeTrue(); + } + + [TestMethod] + public async Task LoadAsyncDoesNotThrowIfFileIsLockedByAnotherProcess() + { + await _configurationManager.ClearAsync(TestContext.CancellationToken); + await _configurationManager.SaveAsync(new TelemetryConfigurations { TelemetryEnabled = true }, TestContext.CancellationToken); + + using var otherProcessFile = File.Open(_configurationManager.ConfigPath, FileMode.Open, FileAccess.ReadWrite, FileShare.None); + + var telemetryConfigurations = await _configurationManager.LoadAsync(true, TestContext.CancellationToken); + + telemetryConfigurations.Should().NotBeNull(); + + // The file of the other process should not have been cleared. + otherProcessFile.Length.Should().BeGreaterThan(0); + } + + [TestMethod] + public async Task ConcurrentLoadsAndSavesDoNotThrow() + { + var tasks = new List(); + + for (var i = 0; i < 5; i++) + { + tasks.Add(Task.Run( + async () => + { + var telemetryConfigurations = await _configurationManager.LoadAsync(true, TestContext.CancellationToken); + telemetryConfigurations.TelemetryGuid = Guid.NewGuid().ToString(); + await _configurationManager.SaveAsync(telemetryConfigurations, TestContext.CancellationToken); + }, + TestContext.CancellationToken)); + } + + var act = () => Task.WhenAll(tasks); + + await act.Should().NotThrowAsync(); + } + } +} diff --git a/MSStore.CLI/Program.cs b/MSStore.CLI/Program.cs index d969440..408c9cf 100644 --- a/MSStore.CLI/Program.cs +++ b/MSStore.CLI/Program.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Generic; using System.CommandLine.Invocation; +using System.IO; using System.Linq; using System.Net.Http; using System.Net.Http.Headers; @@ -274,7 +275,15 @@ private static async Task CreateTelemetryClientAsync(Configurat if (changed) { - await telemetryConfigurationManager.SaveAsync(telemetryConfigurations, CancellationToken.None); + try + { + await telemetryConfigurationManager.SaveAsync(telemetryConfigurations, CancellationToken.None); + } + catch (IOException) + { + // Telemetry settings are incidental bookkeeping. If another instance of the CLI + // is using the file, just move on instead of failing the command. + } } TelemetryConfiguration telemetryConfiguration = TelemetryConfiguration.CreateDefault(); diff --git a/MSStore.CLI/Services/ConfigurationManager.cs b/MSStore.CLI/Services/ConfigurationManager.cs index 0b19aef..658abe9 100644 --- a/MSStore.CLI/Services/ConfigurationManager.cs +++ b/MSStore.CLI/Services/ConfigurationManager.cs @@ -19,6 +19,8 @@ namespace MSStore.CLI.Services internal class ConfigurationManager(JsonTypeInfo jsonTypeInfo, string fileName, ILogger>? logger) : IConfigurationManager where T : new() { + private const int MaxOpenAttempts = 5; + private static readonly string SettingsDirectory = Path.Combine(GetSystemLocalApplicationDataPath(), "Microsoft", "MSStore.CLI"); private static string GetSystemLocalApplicationDataPath() @@ -43,6 +45,8 @@ private static string GetSystemLocalApplicationDataPath() return Environment.GetFolderPath(Environment.SpecialFolder.LocalApplicationData); } + private static readonly TimeSpan OpenRetryDelay = TimeSpan.FromMilliseconds(50); + private readonly string _settingsPath = Path.Combine(SettingsDirectory, fileName); private readonly JsonTypeInfo _jsonTypeInfo = jsonTypeInfo ?? throw new ArgumentNullException(nameof(jsonTypeInfo)); private readonly ILogger? _logger = logger; @@ -59,10 +63,23 @@ public async Task LoadAsync(bool clearInvalidConfig, CancellationToken ct) return await ClearAsync(ct); } - using var file = File.Open(_settingsPath, FileMode.Open); + using var file = await OpenAsync(FileMode.Open, ct); return await JsonSerializer.DeserializeAsync(file, _jsonTypeInfo, ct) ?? new T(); } + catch (IOException ex) + { + // Another process is using the file. Do not overwrite its contents, + // just fallback to the default configuration. + _logger?.LogWarning(ex, "Could not read the configuration file: {SettingsPath}", _settingsPath); + + if (!clearInvalidConfig) + { + throw; + } + + return new T(); + } catch { if (!clearInvalidConfig) @@ -77,7 +94,7 @@ public async Task LoadAsync(bool clearInvalidConfig, CancellationToken ct) public async Task ClearAsync(CancellationToken ct) { EnsureDirectoryExists(); - using var file = File.Open(_settingsPath, FileMode.OpenOrCreate); + using var file = await OpenAsync(FileMode.OpenOrCreate, ct); file.SetLength(0); await file.FlushAsync(ct); file.Position = 0; @@ -88,12 +105,30 @@ public async Task ClearAsync(CancellationToken ct) public async Task SaveAsync(T config, CancellationToken ct) { - using var file = File.Open(_settingsPath, FileMode.OpenOrCreate); + using var file = await OpenAsync(FileMode.OpenOrCreate, ct); file.SetLength(0); file.Position = 0; await JsonSerializer.SerializeAsync(file, config, _jsonTypeInfo, ct); } + private async Task OpenAsync(FileMode fileMode, CancellationToken ct) + { + for (var attempt = 1; ; attempt++) + { + try + { + return File.Open(_settingsPath, fileMode, FileAccess.ReadWrite, FileShare.None); + } + catch (IOException ex) when (attempt < MaxOpenAttempts && ex is not FileNotFoundException and not DirectoryNotFoundException) + { + // The file is being used by another process. Wait a bit and try again. + _logger?.LogInformation("Configuration file '{SettingsPath}' is in use. Retrying ({Attempt}/{MaxOpenAttempts})...", _settingsPath, attempt, MaxOpenAttempts); + + await Task.Delay(OpenRetryDelay * attempt, ct); + } + } + } + private void EnsureDirectoryExists() { if (Directory.Exists(SettingsDirectory)) From 5bbdd3de89be9edd2041e981fec267449df8b486 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 4 Sep 2026 07:41:34 +0000 Subject: [PATCH 03/10] Assert save is pending while the config file is locked Co-authored-by: isourabh <2982389+isourabh@users.noreply.github.com> --- MSStore.CLI.UnitTests/ConfigurationManagerUnitTests.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/MSStore.CLI.UnitTests/ConfigurationManagerUnitTests.cs b/MSStore.CLI.UnitTests/ConfigurationManagerUnitTests.cs index f780afe..d3276a1 100644 --- a/MSStore.CLI.UnitTests/ConfigurationManagerUnitTests.cs +++ b/MSStore.CLI.UnitTests/ConfigurationManagerUnitTests.cs @@ -42,6 +42,10 @@ public async Task SaveAsyncWaitsForOtherProcessToReleaseTheFile() await Task.Delay(100, TestContext.CancellationToken); + // The save must not have completed yet, otherwise it did not really + // wait for the other process to release the file. + saveTask.IsCompleted.Should().BeFalse(); + otherProcessFile.Dispose(); await saveTask; From eec0f72e121988acc630fe95e981a06ada7d8d8e Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 4 Sep 2026 08:03:38 +0000 Subject: [PATCH 04/10] Address review: repair path for missing file, fail-closed telemetry, test style Co-authored-by: isourabh <2982389+isourabh@users.noreply.github.com> --- .../ConfigurationManagerUnitTests.cs | 21 ++++++++++++++++--- MSStore.CLI/Program.cs | 8 +++++++ MSStore.CLI/Services/ConfigurationManager.cs | 2 +- 3 files changed, 27 insertions(+), 4 deletions(-) diff --git a/MSStore.CLI.UnitTests/ConfigurationManagerUnitTests.cs b/MSStore.CLI.UnitTests/ConfigurationManagerUnitTests.cs index d3276a1..5d93de8 100644 --- a/MSStore.CLI.UnitTests/ConfigurationManagerUnitTests.cs +++ b/MSStore.CLI.UnitTests/ConfigurationManagerUnitTests.cs @@ -71,6 +71,22 @@ public async Task LoadAsyncDoesNotThrowIfFileIsLockedByAnotherProcess() otherProcessFile.Length.Should().BeGreaterThan(0); } + [TestMethod] + public async Task LoadAsyncRecreatesTheFileIfItContainsInvalidJson() + { + await _configurationManager.ClearAsync(TestContext.CancellationToken); + + await File.WriteAllTextAsync(_configurationManager.ConfigPath, "not json", TestContext.CancellationToken); + + var telemetryConfigurations = await _configurationManager.LoadAsync(true, TestContext.CancellationToken); + + telemetryConfigurations.Should().NotBeNull(); + + // The invalid file should have been repaired. + var content = await File.ReadAllTextAsync(_configurationManager.ConfigPath, TestContext.CancellationToken); + content.Should().NotBe("not json"); + } + [TestMethod] public async Task ConcurrentLoadsAndSavesDoNotThrow() { @@ -88,9 +104,8 @@ public async Task ConcurrentLoadsAndSavesDoNotThrow() TestContext.CancellationToken)); } - var act = () => Task.WhenAll(tasks); - - await act.Should().NotThrowAsync(); + // Any exception thrown by a concurrent load/save fails the test. + await Task.WhenAll(tasks); } } } diff --git a/MSStore.CLI/Program.cs b/MSStore.CLI/Program.cs index 408c9cf..8706571 100644 --- a/MSStore.CLI/Program.cs +++ b/MSStore.CLI/Program.cs @@ -258,9 +258,11 @@ void AddMSCorrelationId(HttpRequestHeaders defaultRequestHeaders) private static async Task CreateTelemetryClientAsync(ConfigurationManager telemetryConfigurationManager, TelemetryConfigurations telemetryConfigurations) { var changed = false; + var telemetryEnabledDefaulted = false; if (!telemetryConfigurations.TelemetryEnabled.HasValue) { telemetryConfigurations.TelemetryEnabled = true; + telemetryEnabledDefaulted = true; changed = true; } @@ -283,6 +285,12 @@ private static async Task CreateTelemetryClientAsync(Configurat { // Telemetry settings are incidental bookkeeping. If another instance of the CLI // is using the file, just move on instead of failing the command. + if (telemetryEnabledDefaulted) + { + // We could not read the file, so we might be defaulting over a user that + // has opted out. Fail closed and keep telemetry off for this run. + telemetryConfigurations.TelemetryEnabled = false; + } } } diff --git a/MSStore.CLI/Services/ConfigurationManager.cs b/MSStore.CLI/Services/ConfigurationManager.cs index 658abe9..1f81a14 100644 --- a/MSStore.CLI/Services/ConfigurationManager.cs +++ b/MSStore.CLI/Services/ConfigurationManager.cs @@ -67,7 +67,7 @@ public async Task LoadAsync(bool clearInvalidConfig, CancellationToken ct) return await JsonSerializer.DeserializeAsync(file, _jsonTypeInfo, ct) ?? new T(); } - catch (IOException ex) + catch (IOException ex) when (ex is not FileNotFoundException and not DirectoryNotFoundException) { // Another process is using the file. Do not overwrite its contents, // just fallback to the default configuration. From 097c635a87c6f645b87a4a6acd3a61f3522d86ae Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 4 Sep 2026 08:38:58 +0000 Subject: [PATCH 05/10] Only treat real sharing violations as contention; never persist telemetry defaults over an unread file Co-authored-by: isourabh <2982389+isourabh@users.noreply.github.com> --- .../ConfigurationManagerUnitTests.cs | 12 +++++ MSStore.CLI/Program.cs | 52 +++++++++++++++---- MSStore.CLI/Services/ConfigurationManager.cs | 28 +++++++++- 3 files changed, 79 insertions(+), 13 deletions(-) diff --git a/MSStore.CLI.UnitTests/ConfigurationManagerUnitTests.cs b/MSStore.CLI.UnitTests/ConfigurationManagerUnitTests.cs index 5d93de8..2918f2c 100644 --- a/MSStore.CLI.UnitTests/ConfigurationManagerUnitTests.cs +++ b/MSStore.CLI.UnitTests/ConfigurationManagerUnitTests.cs @@ -71,6 +71,18 @@ public async Task LoadAsyncDoesNotThrowIfFileIsLockedByAnotherProcess() otherProcessFile.Length.Should().BeGreaterThan(0); } + [TestMethod] + public async Task LoadAsyncThrowsIfFileIsLockedAndClearInvalidConfigIsDisabled() + { + await _configurationManager.ClearAsync(TestContext.CancellationToken); + + using var otherProcessFile = File.Open(_configurationManager.ConfigPath, FileMode.Open, FileAccess.ReadWrite, FileShare.None); + + // Callers that need to tell a locked file apart from an invalid one rely on this. + await Assert.ThrowsExactlyAsync( + () => _configurationManager.LoadAsync(false, TestContext.CancellationToken)); + } + [TestMethod] public async Task LoadAsyncRecreatesTheFileIfItContainsInvalidJson() { diff --git a/MSStore.CLI/Program.cs b/MSStore.CLI/Program.cs index 8706571..0d0ae02 100644 --- a/MSStore.CLI/Program.cs +++ b/MSStore.CLI/Program.cs @@ -51,8 +51,8 @@ public static async Task Main(params string[] args) TelemetrySourceGenerationContext.Default.TelemetryConfigurations, "telemetrySettings.json", null); - TelemetryConfigurations telemetryConfigurations = await telemetryConfigurationManager.LoadAsync(true, CancellationToken.None); - TelemetryClient telemetryClient = await CreateTelemetryClientAsync(telemetryConfigurationManager, telemetryConfigurations); + (TelemetryConfigurations telemetryConfigurations, bool telemetryConfigurationsReadable) = await LoadTelemetryConfigurationsAsync(telemetryConfigurationManager); + TelemetryClient telemetryClient = await CreateTelemetryClientAsync(telemetryConfigurationManager, telemetryConfigurations, telemetryConfigurationsReadable); var ansiConsole = AnsiConsole.Create(new() { Interactive = Console.IsErrorRedirected ? InteractionSupport.No : InteractionSupport.Yes, @@ -255,14 +255,42 @@ void AddMSCorrelationId(HttpRequestHeaders defaultRequestHeaders) internal static string SessionId { get; } = Guid.NewGuid().ToString(); - private static async Task CreateTelemetryClientAsync(ConfigurationManager telemetryConfigurationManager, TelemetryConfigurations telemetryConfigurations) + /// + /// Loads the telemetry configurations, reporting whether they could actually be read. + /// A concurrent invocation of the CLI may be holding the file, in which case the stored + /// preferences are unknown and must not be overwritten. + /// + private static async Task<(TelemetryConfigurations Configurations, bool Readable)> LoadTelemetryConfigurationsAsync(ConfigurationManager telemetryConfigurationManager) + { + try + { + // Do not repair yet, so that a locked file is distinguishable from an invalid one. + return (await telemetryConfigurationManager.LoadAsync(false, CancellationToken.None), true); + } + catch (IOException) + { + return (new TelemetryConfigurations(), false); + } + catch + { + // The file is invalid for some other reason. Repair it. + try + { + return (await telemetryConfigurationManager.LoadAsync(true, CancellationToken.None), true); + } + catch (IOException) + { + return (new TelemetryConfigurations(), false); + } + } + } + + private static async Task CreateTelemetryClientAsync(ConfigurationManager telemetryConfigurationManager, TelemetryConfigurations telemetryConfigurations, bool telemetryConfigurationsReadable) { var changed = false; - var telemetryEnabledDefaulted = false; if (!telemetryConfigurations.TelemetryEnabled.HasValue) { telemetryConfigurations.TelemetryEnabled = true; - telemetryEnabledDefaulted = true; changed = true; } @@ -275,6 +303,14 @@ private static async Task CreateTelemetryClientAsync(Configurat changed = true; } + if (!telemetryConfigurationsReadable) + { + // We could not read the stored preferences, so we cannot tell whether the user opted + // out. Fail closed for this run, and do not persist defaults over their settings. + telemetryConfigurations.TelemetryEnabled = false; + changed = false; + } + if (changed) { try @@ -285,12 +321,6 @@ private static async Task CreateTelemetryClientAsync(Configurat { // Telemetry settings are incidental bookkeeping. If another instance of the CLI // is using the file, just move on instead of failing the command. - if (telemetryEnabledDefaulted) - { - // We could not read the file, so we might be defaulting over a user that - // has opted out. Fail closed and keep telemetry off for this run. - telemetryConfigurations.TelemetryEnabled = false; - } } } diff --git a/MSStore.CLI/Services/ConfigurationManager.cs b/MSStore.CLI/Services/ConfigurationManager.cs index 1f81a14..e14642a 100644 --- a/MSStore.CLI/Services/ConfigurationManager.cs +++ b/MSStore.CLI/Services/ConfigurationManager.cs @@ -21,6 +21,15 @@ internal class ConfigurationManager(JsonTypeInfo jsonTypeInfo, string file { private const int MaxOpenAttempts = 5; + // HRESULTs Windows reports when another process holds the file open. + private const int ErrorSharingViolation = unchecked((int)0x80070020); // ERROR_SHARING_VIOLATION (32) + private const int ErrorLockViolation = unchecked((int)0x80070021); // ERROR_LOCK_VIOLATION (33) + + // On Unix, FileShare is implemented with flock(), and .NET surfaces the raw errno + // (EWOULDBLOCK) as the HResult when the lock cannot be taken. The value differs per platform. + private const int ErrorWouldBlockLinux = 11; // EAGAIN/EWOULDBLOCK on Linux + private const int ErrorWouldBlockBsd = 35; // EAGAIN/EWOULDBLOCK on macOS and other BSDs + private static readonly string SettingsDirectory = Path.Combine(GetSystemLocalApplicationDataPath(), "Microsoft", "MSStore.CLI"); private static string GetSystemLocalApplicationDataPath() @@ -47,6 +56,21 @@ private static string GetSystemLocalApplicationDataPath() private static readonly TimeSpan OpenRetryDelay = TimeSpan.FromMilliseconds(50); + /// + /// Checks whether an was caused by another process holding the file open, + /// as opposed to an unrelated I/O failure that should not be retried or silently ignored. + /// + private static bool IsFileInUse(IOException ex) + { + // A missing file/directory is never a sharing violation, even though both derive from IOException. + if (ex is FileNotFoundException or DirectoryNotFoundException) + { + return false; + } + + return ex.HResult is ErrorSharingViolation or ErrorLockViolation or ErrorWouldBlockLinux or ErrorWouldBlockBsd; + } + private readonly string _settingsPath = Path.Combine(SettingsDirectory, fileName); private readonly JsonTypeInfo _jsonTypeInfo = jsonTypeInfo ?? throw new ArgumentNullException(nameof(jsonTypeInfo)); private readonly ILogger? _logger = logger; @@ -67,7 +91,7 @@ public async Task LoadAsync(bool clearInvalidConfig, CancellationToken ct) return await JsonSerializer.DeserializeAsync(file, _jsonTypeInfo, ct) ?? new T(); } - catch (IOException ex) when (ex is not FileNotFoundException and not DirectoryNotFoundException) + catch (IOException ex) when (IsFileInUse(ex)) { // Another process is using the file. Do not overwrite its contents, // just fallback to the default configuration. @@ -119,7 +143,7 @@ private async Task OpenAsync(FileMode fileMode, CancellationToken ct { return File.Open(_settingsPath, fileMode, FileAccess.ReadWrite, FileShare.None); } - catch (IOException ex) when (attempt < MaxOpenAttempts && ex is not FileNotFoundException and not DirectoryNotFoundException) + catch (IOException ex) when (attempt < MaxOpenAttempts && IsFileInUse(ex)) { // The file is being used by another process. Wait a bit and try again. _logger?.LogInformation("Configuration file '{SettingsPath}' is in use. Retrying ({Attempt}/{MaxOpenAttempts})...", _settingsPath, attempt, MaxOpenAttempts); From 2d819b6eb46674dc746acffa823bbc0ea97cea5b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 8 Sep 2026 21:44:55 +0000 Subject: [PATCH 06/10] Address review: TryLoadAsync for ResetAsync, read-shared LoadAsync, deterministic retry test Co-authored-by: azchohfi <527713+azchohfi@users.noreply.github.com> --- .../ConfigurationManagerUnitTests.cs | 65 +++++++++++++++---- .../ReconfigureCredentialSafetyUnitTests.cs | 30 +++++++-- MSStore.CLI/Program.cs | 32 +-------- MSStore.CLI/Services/CLIConfigurator.cs | 11 +++- MSStore.CLI/Services/ConfigurationManager.cs | 37 +++++++++-- MSStore.CLI/Services/IConfigurationManager.cs | 8 +++ 6 files changed, 131 insertions(+), 52 deletions(-) diff --git a/MSStore.CLI.UnitTests/ConfigurationManagerUnitTests.cs b/MSStore.CLI.UnitTests/ConfigurationManagerUnitTests.cs index 2918f2c..f2a6ea8 100644 --- a/MSStore.CLI.UnitTests/ConfigurationManagerUnitTests.cs +++ b/MSStore.CLI.UnitTests/ConfigurationManagerUnitTests.cs @@ -1,6 +1,7 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. +using Microsoft.Extensions.Logging; using MSStore.CLI.Services; using MSStore.CLI.Services.Telemetry; @@ -9,6 +10,29 @@ namespace MSStore.CLI.UnitTests [TestClass] public class ConfigurationManagerUnitTests { + /// + /// Signals as soon as the configuration manager logs its first open retry, so tests can + /// react to an actual retry attempt instead of racing against a fixed delay. + /// + private sealed class RetrySignalingLogger : ILogger> + { + private readonly TaskCompletionSource _retryObserved = new(TaskCreationOptions.RunContinuationsAsynchronously); + + public Task RetryObserved => _retryObserved.Task; + + public IDisposable? BeginScope(TState state) + where TState : notnull => null; + + public bool IsEnabled(LogLevel logLevel) => true; + + public void Log(LogLevel logLevel, EventId eventId, TState state, Exception? exception, Func formatter) + { + if (logLevel == LogLevel.Information) + { + _retryObserved.TrySetResult(); + } + } + } private ConfigurationManager _configurationManager = null!; public TestContext TestContext { get; set; } = null!; @@ -34,25 +58,44 @@ public void Cleanup() [TestMethod] public async Task SaveAsyncWaitsForOtherProcessToReleaseTheFile() { - await _configurationManager.ClearAsync(TestContext.CancellationToken); + var logger = new RetrySignalingLogger(); + var configurationManager = new ConfigurationManager( + TelemetrySourceGenerationContext.Default.TelemetryConfigurations, + $"test_{Guid.NewGuid()}.json", + logger); - var otherProcessFile = File.Open(_configurationManager.ConfigPath, FileMode.Open, FileAccess.ReadWrite, FileShare.None); + try + { + await configurationManager.ClearAsync(TestContext.CancellationToken); - var saveTask = _configurationManager.SaveAsync(new TelemetryConfigurations { TelemetryEnabled = true }, TestContext.CancellationToken); + var otherProcessFile = File.Open(configurationManager.ConfigPath, FileMode.Open, FileAccess.ReadWrite, FileShare.None); - await Task.Delay(100, TestContext.CancellationToken); + var saveTask = configurationManager.SaveAsync(new TelemetryConfigurations { TelemetryEnabled = true }, TestContext.CancellationToken); - // The save must not have completed yet, otherwise it did not really - // wait for the other process to release the file. - saveTask.IsCompleted.Should().BeFalse(); + // Wait for an actual retry attempt to be logged, instead of a fixed delay, so this + // cannot flake if the runner is slow to schedule this thread: as soon as the first + // retry is observed, the file is released well within the remaining retry budget. + await logger.RetryObserved.WaitAsync(TimeSpan.FromSeconds(30), TestContext.CancellationToken); - otherProcessFile.Dispose(); + // The save must not have completed yet, otherwise it did not really + // wait for the other process to release the file. + saveTask.IsCompleted.Should().BeFalse(); - await saveTask; + otherProcessFile.Dispose(); - var telemetryConfigurations = await _configurationManager.LoadAsync(true, TestContext.CancellationToken); + await saveTask; - telemetryConfigurations.TelemetryEnabled.Should().BeTrue(); + var telemetryConfigurations = await configurationManager.LoadAsync(true, TestContext.CancellationToken); + + telemetryConfigurations.TelemetryEnabled.Should().BeTrue(); + } + finally + { + if (File.Exists(configurationManager.ConfigPath)) + { + File.Delete(configurationManager.ConfigPath); + } + } } [TestMethod] diff --git a/MSStore.CLI.UnitTests/ReconfigureCredentialSafetyUnitTests.cs b/MSStore.CLI.UnitTests/ReconfigureCredentialSafetyUnitTests.cs index 874602e..880dbd8 100644 --- a/MSStore.CLI.UnitTests/ReconfigureCredentialSafetyUnitTests.cs +++ b/MSStore.CLI.UnitTests/ReconfigureCredentialSafetyUnitTests.cs @@ -205,8 +205,8 @@ public async Task ResetShouldNotDiscardSettingsWhenTheCredentialSurvivesTheClear // settings.json while an unremovable credential lingers leaves the machine worse off than before. ArrangeExistingClientSecretConfiguration(validationSucceeds: true); FakeConfigurationManager - .Setup(x => x.LoadAsync(It.IsAny(), It.IsAny())) - .ReturnsAsync(new Configurations { ClientId = new Guid(ExistingClientId) }); + .Setup(x => x.TryLoadAsync(It.IsAny())) + .ReturnsAsync((new Configurations { ClientId = new Guid(ExistingClientId) }, true)); CredentialManager .Setup(x => x.ClearCredentials(It.IsAny())) .Callback(() => { }); @@ -226,8 +226,8 @@ public async Task ResetShouldClearEverythingWhenTheCredentialIsRemoved() { ArrangeExistingClientSecretConfiguration(validationSucceeds: true); FakeConfigurationManager - .Setup(x => x.LoadAsync(It.IsAny(), It.IsAny())) - .ReturnsAsync(new Configurations { ClientId = new Guid(ExistingClientId) }); + .Setup(x => x.TryLoadAsync(It.IsAny())) + .ReturnsAsync((new Configurations { ClientId = new Guid(ExistingClientId) }, true)); FakeConsole .Setup(x => x.YesNoConfirmationAsync(It.IsAny(), It.IsAny())) .ReturnsAsync(true); @@ -239,6 +239,28 @@ public async Task ResetShouldClearEverythingWhenTheCredentialIsRemoved() TokenManager.Verify(x => x.ClearAllCacheAsync(), Times.Once); } + [TestMethod] + public async Task ResetShouldNotWipeSettingsWhenTheConfigurationCouldNotBeRead() + { + // If settings.json could not be read (e.g. a concurrent `msstore` process is holding it open), + // we cannot tell a config that never had a ClientId apart from one we simply couldn't read. Wiping + // settings.json in that case could orphan a credential in the OS store with no ClientId left to find it. + ArrangeExistingClientSecretConfiguration(validationSucceeds: true); + FakeConfigurationManager + .Setup(x => x.TryLoadAsync(It.IsAny())) + .ReturnsAsync((new Configurations(), false)); + FakeConsole + .Setup(x => x.YesNoConfirmationAsync(It.IsAny(), It.IsAny())) + .ReturnsAsync(true); + + await ParseAndInvokeAsync(["reconfigure", "--reset"], expectedResult: -1); + + _credentialStore[ExistingClientId].Should().Be(ExistingSecret); + CredentialManager.Verify(x => x.ClearCredentials(It.IsAny()), Times.Never); + FakeConfigurationManager.Verify(x => x.ClearAsync(It.IsAny()), Times.Never); + TokenManager.Verify(x => x.ClearAllCacheAsync(), Times.Never); + } + [TestMethod] public async Task ReconfigureShouldValidateBeforeMutatingTheCredentialStore() { diff --git a/MSStore.CLI/Program.cs b/MSStore.CLI/Program.cs index 0d0ae02..909508b 100644 --- a/MSStore.CLI/Program.cs +++ b/MSStore.CLI/Program.cs @@ -51,7 +51,7 @@ public static async Task Main(params string[] args) TelemetrySourceGenerationContext.Default.TelemetryConfigurations, "telemetrySettings.json", null); - (TelemetryConfigurations telemetryConfigurations, bool telemetryConfigurationsReadable) = await LoadTelemetryConfigurationsAsync(telemetryConfigurationManager); + (TelemetryConfigurations telemetryConfigurations, bool telemetryConfigurationsReadable) = await telemetryConfigurationManager.TryLoadAsync(); TelemetryClient telemetryClient = await CreateTelemetryClientAsync(telemetryConfigurationManager, telemetryConfigurations, telemetryConfigurationsReadable); var ansiConsole = AnsiConsole.Create(new() { @@ -255,36 +255,6 @@ void AddMSCorrelationId(HttpRequestHeaders defaultRequestHeaders) internal static string SessionId { get; } = Guid.NewGuid().ToString(); - /// - /// Loads the telemetry configurations, reporting whether they could actually be read. - /// A concurrent invocation of the CLI may be holding the file, in which case the stored - /// preferences are unknown and must not be overwritten. - /// - private static async Task<(TelemetryConfigurations Configurations, bool Readable)> LoadTelemetryConfigurationsAsync(ConfigurationManager telemetryConfigurationManager) - { - try - { - // Do not repair yet, so that a locked file is distinguishable from an invalid one. - return (await telemetryConfigurationManager.LoadAsync(false, CancellationToken.None), true); - } - catch (IOException) - { - return (new TelemetryConfigurations(), false); - } - catch - { - // The file is invalid for some other reason. Repair it. - try - { - return (await telemetryConfigurationManager.LoadAsync(true, CancellationToken.None), true); - } - catch (IOException) - { - return (new TelemetryConfigurations(), false); - } - } - } - private static async Task CreateTelemetryClientAsync(ConfigurationManager telemetryConfigurationManager, TelemetryConfigurations telemetryConfigurations, bool telemetryConfigurationsReadable) { var changed = false; diff --git a/MSStore.CLI/Services/CLIConfigurator.cs b/MSStore.CLI/Services/CLIConfigurator.cs index 8bad0ff..c1557ba 100644 --- a/MSStore.CLI/Services/CLIConfigurator.cs +++ b/MSStore.CLI/Services/CLIConfigurator.cs @@ -676,7 +676,16 @@ public async Task ResetAsync(CancellationToken ct = default) try { - var config = await _configurationManager.LoadAsync(true, ct: ct); + var (config, readable) = await _configurationManager.TryLoadAsync(ct); + + // If the settings file could not be read (e.g. another process is holding it open), + // we cannot tell whether a credential was ever stored, so we must not wipe settings.json: + // that could leave an orphaned credential in the OS store with no ClientId left to find it. + if (!readable) + { + _logger.LogError("Could not read the configuration file. Please try again."); + return false; + } // Remove the credential before discarding the settings, and only continue if it is really gone. // Wiping settings.json while an unremovable credential lingers would leave the machine in a worse diff --git a/MSStore.CLI/Services/ConfigurationManager.cs b/MSStore.CLI/Services/ConfigurationManager.cs index e14642a..1e40497 100644 --- a/MSStore.CLI/Services/ConfigurationManager.cs +++ b/MSStore.CLI/Services/ConfigurationManager.cs @@ -87,7 +87,9 @@ public async Task LoadAsync(bool clearInvalidConfig, CancellationToken ct) return await ClearAsync(ct); } - using var file = await OpenAsync(FileMode.Open, ct); + // Reading does not need to exclude other readers: only a concurrent writer can + // produce a half-written file, and that already holds an exclusive lock. + using var file = await OpenAsync(FileMode.Open, FileAccess.Read, FileShare.Read, ct); return await JsonSerializer.DeserializeAsync(file, _jsonTypeInfo, ct) ?? new T(); } @@ -115,10 +117,35 @@ public async Task LoadAsync(bool clearInvalidConfig, CancellationToken ct) } } + public async Task<(T Configurations, bool Readable)> TryLoadAsync(CancellationToken ct = default) + { + try + { + // Do not repair yet, so that a locked file is distinguishable from an invalid one. + return (await LoadAsync(false, ct), true); + } + catch (IOException) + { + return (new T(), false); + } + catch + { + // The file is invalid for some other reason. Repair it. + try + { + return (await LoadAsync(true, ct), true); + } + catch (IOException) + { + return (new T(), false); + } + } + } + public async Task ClearAsync(CancellationToken ct) { EnsureDirectoryExists(); - using var file = await OpenAsync(FileMode.OpenOrCreate, ct); + using var file = await OpenAsync(FileMode.OpenOrCreate, FileAccess.ReadWrite, FileShare.None, ct); file.SetLength(0); await file.FlushAsync(ct); file.Position = 0; @@ -129,19 +156,19 @@ public async Task ClearAsync(CancellationToken ct) public async Task SaveAsync(T config, CancellationToken ct) { - using var file = await OpenAsync(FileMode.OpenOrCreate, ct); + using var file = await OpenAsync(FileMode.OpenOrCreate, FileAccess.ReadWrite, FileShare.None, ct); file.SetLength(0); file.Position = 0; await JsonSerializer.SerializeAsync(file, config, _jsonTypeInfo, ct); } - private async Task OpenAsync(FileMode fileMode, CancellationToken ct) + private async Task OpenAsync(FileMode fileMode, FileAccess fileAccess, FileShare fileShare, CancellationToken ct) { for (var attempt = 1; ; attempt++) { try { - return File.Open(_settingsPath, fileMode, FileAccess.ReadWrite, FileShare.None); + return File.Open(_settingsPath, fileMode, fileAccess, fileShare); } catch (IOException ex) when (attempt < MaxOpenAttempts && IsFileInUse(ex)) { diff --git a/MSStore.CLI/Services/IConfigurationManager.cs b/MSStore.CLI/Services/IConfigurationManager.cs index 612cf16..7baf1da 100644 --- a/MSStore.CLI/Services/IConfigurationManager.cs +++ b/MSStore.CLI/Services/IConfigurationManager.cs @@ -11,6 +11,14 @@ internal interface IConfigurationManager { string ConfigPath { get; } Task LoadAsync(bool clearInvalidConfig = false, CancellationToken ct = default); + + /// + /// Loads the configuration, repairing invalid content but distinguishing a locked file + /// (contention with another process) from one that was genuinely unreadable, so callers + /// can avoid making destructive decisions based on a config they never actually read. + /// + /// The loaded (or default) configuration, and whether it was actually read from disk. + Task<(T Configurations, bool Readable)> TryLoadAsync(CancellationToken ct = default); Task ClearAsync(CancellationToken ct); Task SaveAsync(T config, CancellationToken ct); } From 98ec1f0d9099bbdef2958c2661e15d851bad126f Mon Sep 17 00:00:00 2001 From: Alexandre Zollinger Chohfi Date: Tue, 8 Sep 2026 15:05:10 -0700 Subject: [PATCH 07/10] Address Copilot review: scope the retry signal and correct the TryLoadAsync contract RetrySignalingLogger completed on any Information log, but ConfigurationManager also logs at Information when it creates the settings directory. On a machine that has never run the CLI, ClearAsync in the arrange step fired the signal before a single open retry had happened, so the test could proceed without ever observing the behaviour it exists to prove. Match the retry message instead. The TryLoadAsync docs claimed the flag reported whether the configuration was "actually read from disk", but it is also true for a missing file (created with defaults) and for repaired invalid content. Describe what it actually means: the stored state is known, and it is false only under contention. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 99c1d195-484e-4482-bafe-1df997d5aafc --- .../ConfigurationManagerUnitTests.cs | 5 ++++- MSStore.CLI/Services/IConfigurationManager.cs | 13 +++++++++---- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/MSStore.CLI.UnitTests/ConfigurationManagerUnitTests.cs b/MSStore.CLI.UnitTests/ConfigurationManagerUnitTests.cs index f2a6ea8..a85f5ea 100644 --- a/MSStore.CLI.UnitTests/ConfigurationManagerUnitTests.cs +++ b/MSStore.CLI.UnitTests/ConfigurationManagerUnitTests.cs @@ -27,7 +27,10 @@ private sealed class RetrySignalingLogger : ILogger(LogLevel logLevel, EventId eventId, TState state, Exception? exception, Func formatter) { - if (logLevel == LogLevel.Information) + // Match the retry message specifically. The configuration manager also logs at + // Information when it creates the settings directory, which would otherwise signal + // before a single retry had happened on a machine that has never run the CLI. + if (logLevel == LogLevel.Information && formatter(state, exception).Contains("Retrying", StringComparison.Ordinal)) { _retryObserved.TrySetResult(); } diff --git a/MSStore.CLI/Services/IConfigurationManager.cs b/MSStore.CLI/Services/IConfigurationManager.cs index 7baf1da..557b155 100644 --- a/MSStore.CLI/Services/IConfigurationManager.cs +++ b/MSStore.CLI/Services/IConfigurationManager.cs @@ -13,11 +13,16 @@ internal interface IConfigurationManager Task LoadAsync(bool clearInvalidConfig = false, CancellationToken ct = default); /// - /// Loads the configuration, repairing invalid content but distinguishing a locked file - /// (contention with another process) from one that was genuinely unreadable, so callers - /// can avoid making destructive decisions based on a config they never actually read. + /// Loads the configuration, repairing invalid content, but reporting separately when the + /// file could not be read at all because another process holds it open, so callers can + /// avoid making destructive decisions based on a configuration they never actually read. /// - /// The loaded (or default) configuration, and whether it was actually read from disk. + /// + /// The loaded (or default) configuration, and whether the stored state is known. + /// The flag is only when the file could not be opened because + /// another process holds it. A missing file (created with defaults) and repaired invalid + /// content both count as known, since in neither case are we discarding a real setting. + /// Task<(T Configurations, bool Readable)> TryLoadAsync(CancellationToken ct = default); Task ClearAsync(CancellationToken ct); Task SaveAsync(T config, CancellationToken ct); From 27b238e51f94b9dfdd6aaad2106036f0265394a7 Mon Sep 17 00:00:00 2001 From: Alexandre Zollinger Chohfi Date: Tue, 8 Sep 2026 15:16:29 -0700 Subject: [PATCH 08/10] Make reset failures visible and stop TryLoadAsync reporting a read that never happened The repair path called LoadAsync(true), which swallows contention and returns a default instance. If the file was invalid on the first attempt and then became locked, TryLoadAsync reported Readable=true for a configuration it never read, which is the exact case ResetAsync relies on the flag to prevent. Recreate the file with ClearAsync instead, which surfaces the lock rather than hiding it. Readable is now also false for non-contention I/O failures, since the caller's question is whether the stored state is known, not why it isn't. The contract is documented that way and both failure paths log the underlying exception. ResetAsync reported its refusals only through LogError, but the default minimum log level is Critical, so `reconfigure --reset` exited -1 with nothing printed. Route both refusals through IAnsiConsole, matching ConfigureAsync, and assert in the credential-safety tests that the reason actually reaches the console. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 99c1d195-484e-4482-bafe-1df997d5aafc --- .../ReconfigureCredentialSafetyUnitTests.cs | 12 ++++++++++-- MSStore.CLI/Commands/ReconfigureCommand.cs | 2 +- MSStore.CLI/Services/CLIConfigurator.cs | 5 ++++- MSStore.CLI/Services/ConfigurationManager.cs | 18 +++++++++++++----- MSStore.CLI/Services/ICLIConfigurator.cs | 2 +- MSStore.CLI/Services/IConfigurationManager.cs | 11 ++++++----- 6 files changed, 35 insertions(+), 15 deletions(-) diff --git a/MSStore.CLI.UnitTests/ReconfigureCredentialSafetyUnitTests.cs b/MSStore.CLI.UnitTests/ReconfigureCredentialSafetyUnitTests.cs index 880dbd8..61e4e0c 100644 --- a/MSStore.CLI.UnitTests/ReconfigureCredentialSafetyUnitTests.cs +++ b/MSStore.CLI.UnitTests/ReconfigureCredentialSafetyUnitTests.cs @@ -214,11 +214,15 @@ public async Task ResetShouldNotDiscardSettingsWhenTheCredentialSurvivesTheClear .Setup(x => x.YesNoConfirmationAsync(It.IsAny(), It.IsAny())) .ReturnsAsync(true); - await ParseAndInvokeAsync(["reconfigure", "--reset"], expectedResult: -1); + var result = await ParseAndInvokeAsync(["reconfigure", "--reset"], expectedResult: -1); _credentialStore[ExistingClientId].Should().Be(ExistingSecret); FakeConfigurationManager.Verify(x => x.ClearAsync(It.IsAny()), Times.Never); TokenManager.Verify(x => x.ClearAllCacheAsync(), Times.Never); + + // The default log level is Critical, so a LogError alone would leave the user staring at + // a bare -1. The reason has to reach the console. + result.Error.Should().Contain("Could not remove the credential for"); } [TestMethod] @@ -253,12 +257,16 @@ public async Task ResetShouldNotWipeSettingsWhenTheConfigurationCouldNotBeRead() .Setup(x => x.YesNoConfirmationAsync(It.IsAny(), It.IsAny())) .ReturnsAsync(true); - await ParseAndInvokeAsync(["reconfigure", "--reset"], expectedResult: -1); + var result = await ParseAndInvokeAsync(["reconfigure", "--reset"], expectedResult: -1); _credentialStore[ExistingClientId].Should().Be(ExistingSecret); CredentialManager.Verify(x => x.ClearCredentials(It.IsAny()), Times.Never); FakeConfigurationManager.Verify(x => x.ClearAsync(It.IsAny()), Times.Never); TokenManager.Verify(x => x.ClearAllCacheAsync(), Times.Never); + + // The default log level is Critical, so a LogError alone would leave the user staring at + // a bare -1. The reason has to reach the console. + result.Error.Should().Contain("Could not read the configuration file."); } [TestMethod] diff --git a/MSStore.CLI/Commands/ReconfigureCommand.cs b/MSStore.CLI/Commands/ReconfigureCommand.cs index 28a0221..cfc352d 100644 --- a/MSStore.CLI/Commands/ReconfigureCommand.cs +++ b/MSStore.CLI/Commands/ReconfigureCommand.cs @@ -118,7 +118,7 @@ public override async Task InvokeAsync(ParseResult parseResult, Cancellatio return await _telemetryClient.TrackCommandEventAsync( (reset == true - ? await _cliConfigurator.ResetAsync(ct: ct) + ? await _cliConfigurator.ResetAsync(_ansiConsole, ct: ct) : await _cliConfigurator.ConfigureAsync( _ansiConsole, askConfirmation, diff --git a/MSStore.CLI/Services/CLIConfigurator.cs b/MSStore.CLI/Services/CLIConfigurator.cs index c1557ba..34355b0 100644 --- a/MSStore.CLI/Services/CLIConfigurator.cs +++ b/MSStore.CLI/Services/CLIConfigurator.cs @@ -666,7 +666,7 @@ private async Task UpdateClientAppAsync(IAnsiConsole ansiConsole, string i }); } - public async Task ResetAsync(CancellationToken ct = default) + public async Task ResetAsync(IAnsiConsole ansiConsole, CancellationToken ct = default) { if (!await _consoleReader.YesNoConfirmationAsync( "Are you sure you want to reset the MSStore CLI credentials?", ct)) @@ -684,6 +684,7 @@ public async Task ResetAsync(CancellationToken ct = default) if (!readable) { _logger.LogError("Could not read the configuration file. Please try again."); + ansiConsole.MarkupLine(":collision: [bold red]Could not read the configuration file. It may be in use by another process. Nothing was changed.[/]"); return false; } @@ -693,6 +694,7 @@ public async Task ResetAsync(CancellationToken ct = default) if (config.ClientId.HasValue && !TryClearCredentials(config.ClientId.Value.ToString())) { _logger.LogError("Could not remove the credential for '{ClientId}' from the credential store. Remove it manually.", config.ClientId.Value); + ansiConsole.MarkupLine($":collision: [bold red]Could not remove the credential for '{config.ClientId.Value}'. Nothing was changed, remove it manually.[/]"); return false; } @@ -705,6 +707,7 @@ public async Task ResetAsync(CancellationToken ct = default) catch (Exception ex) { _logger.LogError(ex, "Error while resetting configuration"); + ansiConsole.MarkupLine($":collision: [bold red]Error while resetting the configuration: {ex.Message.EscapeMarkup()}[/]"); return false; } } diff --git a/MSStore.CLI/Services/ConfigurationManager.cs b/MSStore.CLI/Services/ConfigurationManager.cs index 1e40497..1d4cad5 100644 --- a/MSStore.CLI/Services/ConfigurationManager.cs +++ b/MSStore.CLI/Services/ConfigurationManager.cs @@ -124,19 +124,27 @@ public async Task LoadAsync(bool clearInvalidConfig, CancellationToken ct) // Do not repair yet, so that a locked file is distinguishable from an invalid one. return (await LoadAsync(false, ct), true); } - catch (IOException) + catch (IOException ex) { + // Whether this is contention or a genuine I/O failure, the file is there and we + // could not read it, so the stored state is unknown and must not be reported as + // an empty configuration that the caller is free to act on. + _logger?.LogWarning(ex, "Could not read the configuration file: {SettingsPath}", _settingsPath); return (new T(), false); } - catch + catch (Exception ex) when (ex is not OperationCanceledException) { - // The file is invalid for some other reason. Repair it. + _logger?.LogWarning(ex, "Invalid configuration file, recreating it: {SettingsPath}", _settingsPath); + + // Recreate it directly rather than going through LoadAsync(true), which swallows a + // lock taken since the first attempt and would report a read that never happened. try { - return (await LoadAsync(true, ct), true); + return (await ClearAsync(ct), true); } - catch (IOException) + catch (Exception repairEx) when (repairEx is not OperationCanceledException) { + _logger?.LogWarning(repairEx, "Could not recreate the configuration file: {SettingsPath}", _settingsPath); return (new T(), false); } } diff --git a/MSStore.CLI/Services/ICLIConfigurator.cs b/MSStore.CLI/Services/ICLIConfigurator.cs index fe51958..bd0a5c3 100644 --- a/MSStore.CLI/Services/ICLIConfigurator.cs +++ b/MSStore.CLI/Services/ICLIConfigurator.cs @@ -11,6 +11,6 @@ namespace MSStore.CLI.Services internal interface ICLIConfigurator { Task ConfigureAsync(IAnsiConsole ansiConsole, bool askConfirmation, Guid? tenantId = null, string? sellerId = null, Guid? clientId = null, string? clientSecret = null, string? certificateThumbprint = null, string? certificateFilePath = null, string? certificatePassword = null, bool clientAssertion = false, CancellationToken ct = default); - Task ResetAsync(CancellationToken ct = default); + Task ResetAsync(IAnsiConsole ansiConsole, CancellationToken ct = default); } } diff --git a/MSStore.CLI/Services/IConfigurationManager.cs b/MSStore.CLI/Services/IConfigurationManager.cs index 557b155..50e7657 100644 --- a/MSStore.CLI/Services/IConfigurationManager.cs +++ b/MSStore.CLI/Services/IConfigurationManager.cs @@ -14,14 +14,15 @@ internal interface IConfigurationManager /// /// Loads the configuration, repairing invalid content, but reporting separately when the - /// file could not be read at all because another process holds it open, so callers can - /// avoid making destructive decisions based on a configuration they never actually read. + /// stored state could not be determined at all, so callers can avoid making destructive + /// decisions based on a configuration they never actually read. /// /// /// The loaded (or default) configuration, and whether the stored state is known. - /// The flag is only when the file could not be opened because - /// another process holds it. A missing file (created with defaults) and repaired invalid - /// content both count as known, since in neither case are we discarding a real setting. + /// The flag is when the file exists but could not be read or + /// recreated, whether because another process holds it or because of an I/O failure. + /// A missing file (created with defaults) and repaired invalid content both count as + /// known, since in neither case are we discarding a real setting. /// Task<(T Configurations, bool Readable)> TryLoadAsync(CancellationToken ct = default); Task ClearAsync(CancellationToken ct); From 4d29c2442588fd63a76a07df8e60c681833c50b1 Mon Sep 17 00:00:00 2001 From: Alexandre Zollinger Chohfi Date: Tue, 8 Sep 2026 15:24:40 -0700 Subject: [PATCH 09/10] Don't log the same contention warning twice LoadAsync already logs a warning for sharing/lock violations before rethrowing, so TryLoadAsync logging every IOException duplicated the entry under --verbose. Only report the I/O failures LoadAsync does not. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 99c1d195-484e-4482-bafe-1df997d5aafc --- MSStore.CLI/Services/ConfigurationManager.cs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/MSStore.CLI/Services/ConfigurationManager.cs b/MSStore.CLI/Services/ConfigurationManager.cs index 1d4cad5..6fe8e26 100644 --- a/MSStore.CLI/Services/ConfigurationManager.cs +++ b/MSStore.CLI/Services/ConfigurationManager.cs @@ -129,7 +129,12 @@ public async Task LoadAsync(bool clearInvalidConfig, CancellationToken ct) // Whether this is contention or a genuine I/O failure, the file is there and we // could not read it, so the stored state is unknown and must not be reported as // an empty configuration that the caller is free to act on. - _logger?.LogWarning(ex, "Could not read the configuration file: {SettingsPath}", _settingsPath); + // LoadAsync already logs contention before rethrowing, so only report what it does not. + if (!IsFileInUse(ex)) + { + _logger?.LogWarning(ex, "Could not read the configuration file: {SettingsPath}", _settingsPath); + } + return (new T(), false); } catch (Exception ex) when (ex is not OperationCanceledException) From b15cd9e8771de853838c327d7845cb60b2218613 Mon Sep 17 00:00:00 2001 From: Alexandre Zollinger Chohfi Date: Tue, 8 Sep 2026 15:33:23 -0700 Subject: [PATCH 10/10] Treat a vanished configuration file as missing, not unreadable LoadAsync checks File.Exists and then opens, so a file deleted in between throws FileNotFoundException. TryLoadAsync caught that as a generic IOException and reported Readable=false, contradicting the contract that a missing file is a known state and making ResetAsync abort on a benign race. Recreate the defaults instead, falling back to Readable=false only if recreation itself fails. Extract TryRecreateAsync, now that both the vanished and the invalid-content paths need it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 99c1d195-484e-4482-bafe-1df997d5aafc --- MSStore.CLI/Services/ConfigurationManager.cs | 37 ++++++++++++++------ 1 file changed, 26 insertions(+), 11 deletions(-) diff --git a/MSStore.CLI/Services/ConfigurationManager.cs b/MSStore.CLI/Services/ConfigurationManager.cs index 6fe8e26..ea530b8 100644 --- a/MSStore.CLI/Services/ConfigurationManager.cs +++ b/MSStore.CLI/Services/ConfigurationManager.cs @@ -124,6 +124,15 @@ public async Task LoadAsync(bool clearInvalidConfig, CancellationToken ct) // Do not repair yet, so that a locked file is distinguishable from an invalid one. return (await LoadAsync(false, ct), true); } + catch (IOException ex) when (ex is FileNotFoundException or DirectoryNotFoundException) + { + // The file vanished between the existence check and the open. That leaves us in the + // same state as never having had one, so recreate the defaults instead of reporting + // the configuration as unknown and making callers abort on a benign race. + _logger?.LogWarning(ex, "Configuration file vanished while reading it: {SettingsPath}", _settingsPath); + + return await TryRecreateAsync(ct); + } catch (IOException ex) { // Whether this is contention or a genuine I/O failure, the file is there and we @@ -139,19 +148,25 @@ public async Task LoadAsync(bool clearInvalidConfig, CancellationToken ct) } catch (Exception ex) when (ex is not OperationCanceledException) { + // The content is invalid. Recreate it rather than going through LoadAsync(true), + // which swallows a lock taken since the first attempt and would report a read that + // never happened. _logger?.LogWarning(ex, "Invalid configuration file, recreating it: {SettingsPath}", _settingsPath); - // Recreate it directly rather than going through LoadAsync(true), which swallows a - // lock taken since the first attempt and would report a read that never happened. - try - { - return (await ClearAsync(ct), true); - } - catch (Exception repairEx) when (repairEx is not OperationCanceledException) - { - _logger?.LogWarning(repairEx, "Could not recreate the configuration file: {SettingsPath}", _settingsPath); - return (new T(), false); - } + return await TryRecreateAsync(ct); + } + } + + private async Task<(T Configurations, bool Readable)> TryRecreateAsync(CancellationToken ct) + { + try + { + return (await ClearAsync(ct), true); + } + catch (Exception ex) when (ex is not OperationCanceledException) + { + _logger?.LogWarning(ex, "Could not recreate the configuration file: {SettingsPath}", _settingsPath); + return (new T(), false); } }