Don't crash on concurrent access to telemetrySettings.json

MSStore.CLI.UnitTests/ConfigurationManagerUnitTests.cs

@@ -0,0 +1,169 @@
+// 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;
+
+namespace MSStore.CLI.UnitTests
+{
+    [TestClass]
+    public class ConfigurationManagerUnitTests
+    {
+        /// <summary>
+        /// 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.
+        /// </summary>
+        private sealed class RetrySignalingLogger : ILogger<ConfigurationManager<TelemetryConfigurations>>
+        {
+            private readonly TaskCompletionSource _retryObserved = new(TaskCreationOptions.RunContinuationsAsynchronously);
+
+            public Task RetryObserved => _retryObserved.Task;
+
+            public IDisposable? BeginScope<TState>(TState state)
+                where TState : notnull => null;
+
+            public bool IsEnabled(LogLevel logLevel) => true;
+
+            public void Log<TState>(LogLevel logLevel, EventId eventId, TState state, Exception? exception, Func<TState, Exception?, string> formatter)
+            {
+                // 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();
+                }
+            }
+        }
+        private ConfigurationManager<TelemetryConfigurations> _configurationManager = null!;
+
+        public TestContext TestContext { get; set; } = null!;
+
+        [TestInitialize]
+        public void Initialize()
+        {
+            _configurationManager = new ConfigurationManager<TelemetryConfigurations>(
+                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()
+        {
+            var logger = new RetrySignalingLogger();
+            var configurationManager = new ConfigurationManager<TelemetryConfigurations>(
+                TelemetrySourceGenerationContext.Default.TelemetryConfigurations,
+                $"test_{Guid.NewGuid()}.json",
+                logger);
+
+            try
+            {
+                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);
+
+                // 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);
+
+                // 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;
+
+                var telemetryConfigurations = await configurationManager.LoadAsync(true, TestContext.CancellationToken);
+
+                telemetryConfigurations.TelemetryEnabled.Should().BeTrue();
+            }
+            finally
+            {
+                if (File.Exists(configurationManager.ConfigPath))
+                {
+                    File.Delete(configurationManager.ConfigPath);
+                }
+            }
+        }
+
+        [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 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<IOException>(
+                () => _configurationManager.LoadAsync(false, TestContext.CancellationToken));
+        }
+
+        [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()
+        {
+            var tasks = new List<Task>();
+
+            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));
+            }
+
+            // Any exception thrown by a concurrent load/save fails the test.
+            await Task.WhenAll(tasks);
+        }
+    }
+}

MSStore.CLI.UnitTests/ReconfigureCredentialSafetyUnitTests.cs

@@ -205,29 +205,33 @@ 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<bool>(), It.IsAny<CancellationToken>()))
-                .ReturnsAsync(new Configurations { ClientId = new Guid(ExistingClientId) });
+                .Setup(x => x.TryLoadAsync(It.IsAny<CancellationToken>()))
+                .ReturnsAsync((new Configurations { ClientId = new Guid(ExistingClientId) }, true));
             CredentialManager
                 .Setup(x => x.ClearCredentials(It.IsAny<string>()))
                 .Callback(() => { });
             FakeConsole
                 .Setup(x => x.YesNoConfirmationAsync(It.IsAny<string>(), It.IsAny<CancellationToken>()))
                 .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<CancellationToken>()), 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]
         public async Task ResetShouldClearEverythingWhenTheCredentialIsRemoved()
         {
             ArrangeExistingClientSecretConfiguration(validationSucceeds: true);
             FakeConfigurationManager
-                .Setup(x => x.LoadAsync(It.IsAny<bool>(), It.IsAny<CancellationToken>()))
-                .ReturnsAsync(new Configurations { ClientId = new Guid(ExistingClientId) });
+                .Setup(x => x.TryLoadAsync(It.IsAny<CancellationToken>()))
+                .ReturnsAsync((new Configurations { ClientId = new Guid(ExistingClientId) }, true));
             FakeConsole
                 .Setup(x => x.YesNoConfirmationAsync(It.IsAny<string>(), It.IsAny<CancellationToken>()))
                 .ReturnsAsync(true);
@@ -239,6 +243,32 @@ 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<CancellationToken>()))
+                .ReturnsAsync((new Configurations(), false));
+            FakeConsole
+                .Setup(x => x.YesNoConfirmationAsync(It.IsAny<string>(), It.IsAny<CancellationToken>()))
+                .ReturnsAsync(true);
+
+            var result = await ParseAndInvokeAsync(["reconfigure", "--reset"], expectedResult: -1);
+
+            _credentialStore[ExistingClientId].Should().Be(ExistingSecret);
+            CredentialManager.Verify(x => x.ClearCredentials(It.IsAny<string>()), Times.Never);
+            FakeConfigurationManager.Verify(x => x.ClearAsync(It.IsAny<CancellationToken>()), 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]
         public async Task ReconfigureShouldValidateBeforeMutatingTheCredentialStore()
         {

MSStore.CLI/Commands/ReconfigureCommand.cs

@@ -118,7 +118,7 @@ public override async Task<int> InvokeAsync(ParseResult parseResult, Cancellatio
 
                 return await _telemetryClient.TrackCommandEventAsync<Handler>(
                     (reset == true
-                        ? await _cliConfigurator.ResetAsync(ct: ct)
+                        ? await _cliConfigurator.ResetAsync(_ansiConsole, ct: ct)
                         : await _cliConfigurator.ConfigureAsync(
                             _ansiConsole,
                             askConfirmation,

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;
@@ -50,8 +51,8 @@ public static async Task<int> 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 telemetryConfigurationManager.TryLoadAsync();
+            TelemetryClient telemetryClient = await CreateTelemetryClientAsync(telemetryConfigurationManager, telemetryConfigurations, telemetryConfigurationsReadable);
             var ansiConsole = AnsiConsole.Create(new()
             {
                 Interactive = Console.IsErrorRedirected ? InteractionSupport.No : InteractionSupport.Yes,
@@ -254,7 +255,7 @@ void AddMSCorrelationId(HttpRequestHeaders defaultRequestHeaders)
 
         internal static string SessionId { get; } = Guid.NewGuid().ToString();
 
-        private static async Task<TelemetryClient> CreateTelemetryClientAsync(ConfigurationManager<TelemetryConfigurations> telemetryConfigurationManager, TelemetryConfigurations telemetryConfigurations)
+        private static async Task<TelemetryClient> CreateTelemetryClientAsync(ConfigurationManager<TelemetryConfigurations> telemetryConfigurationManager, TelemetryConfigurations telemetryConfigurations, bool telemetryConfigurationsReadable)
         {
             var changed = false;
             if (!telemetryConfigurations.TelemetryEnabled.HasValue)
@@ -272,9 +273,25 @@ private static async Task<TelemetryClient> 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)
             {
-                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();

MSStore.CLI/Services/CLIConfigurator.cs

@@ -666,7 +666,7 @@ private async Task<bool> UpdateClientAppAsync(IAnsiConsole ansiConsole, string i
             });
         }
 
-        public async Task<bool> ResetAsync(CancellationToken ct = default)
+        public async Task<bool> ResetAsync(IAnsiConsole ansiConsole, CancellationToken ct = default)
         {
             if (!await _consoleReader.YesNoConfirmationAsync(
                     "Are you sure you want to reset the MSStore CLI credentials?", ct))
@@ -676,14 +676,25 @@ public async Task<bool> 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.");
+                    ansiConsole.MarkupLine(":collision: [bold red]Could not read the configuration file. It may be in use by another process. Nothing was changed.[/]");
+                    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
                 // state than it started in, and reporting success would be a lie.
                 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;
                 }
 
@@ -696,6 +707,7 @@ public async Task<bool> 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;
             }
         }