Don't crash on concurrent access to telemetrySettings.json

MSStore.CLI.UnitTests/ConfigurationManagerUnitTests.cs

@@ -0,0 +1,96 @@
+// 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<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()
+        {
+            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);
+
+            // 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();
+        }
+
+        [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<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));
+            }
+
+            var act = () => Task.WhenAll(tasks);
+
+            await act.Should().NotThrowAsync();
+        }
+    }
+}

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<TelemetryClient> 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();

MSStore.CLI/Services/ConfigurationManager.cs

@@ -19,6 +19,8 @@ namespace MSStore.CLI.Services
     internal class ConfigurationManager<T>(JsonTypeInfo<T> jsonTypeInfo, string fileName, ILogger<ConfigurationManager<T>>? logger) : IConfigurationManager<T>
         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<T> _jsonTypeInfo = jsonTypeInfo ?? throw new ArgumentNullException(nameof(jsonTypeInfo));
         private readonly ILogger? _logger = logger;
@@ -59,10 +63,23 @@ public async Task<T> 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<T> LoadAsync(bool clearInvalidConfig, CancellationToken ct)
         public async Task<T> 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<T> 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<FileStream> 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))