Don't crash on concurrent access to telemetrySettings.json

[Copilot]

Two msstore processes running at once could kill one of them with an unhandled IOException, since Program.Main loads and rewrites telemetrySettings.json before the command line is even parsed and ConfigurationManager opened the file with no retry or error handling.

Start-Job { msstore --help }
Start-Job { msstore --help }
# Unhandled exception. System.IO.IOException: The process cannot access the file
# '...\MSStore.CLI\telemetrySettings.json' because it is being used by another process.

ConfigurationManager

• LoadAsync/ClearAsync/SaveAsync open the file via a new OpenAsync helper that retries a sharing violation up to 5 times with incremental backoff (50 ms × attempt). FileNotFoundException/DirectoryNotFoundException are excluded so genuinely bad paths still fail fast.
• LoadAsync handles IOException separately from the existing "invalid config" path: contention now logs a warning and returns a default instance rather than falling through to ClearAsync, which would truncate the file the other process is writing (and throw again from inside the catch).

Program

• CreateTelemetryClientAsync ignores IOException when persisting telemetry settings — this is incidental bookkeeping and should never fail the command being run.

Tests

ConfigurationManagerUnitTests covers a save that retries until an exclusive lock is released, a load under lock returning a default without clobbering the file, and concurrent load/save cycles.

Retry bounds are deliberately short: worst case ~500 ms before falling back, which keeps startup cost negligible for the common uncontended case.

• Fixes Concurrent msstore invocations crash with an unhandled IOException on telemetrySettings.json #176

[Copilot]

Copilot wasn't able to review any files in this pull request.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

The current LoadAsync exception handling can swallow cancellation and perform unintended side effects, and one of the new tests doesn’t yet assert the behavior its name implies.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

MSStore.CLI/Services/ConfigurationManager.cs:91

• LoadAsync's broad catch-all will also catch OperationCanceledException/TaskCanceledException from the passed CancellationToken (e.g., during retry delays or JSON I/O) and, when clearInvalidConfig is true, treat it like a parse failure and call ClearAsync. Cancellation should always propagate to avoid unintended side effects (including potentially overwriting a config file) when a caller cancels the operation.

            catch
            {
                if (!clearInvalidConfig)
                {
                    throw;
                }

                return await ClearAsync(ct);
            }

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The new unit test assertion style is likely to break compilation, and the telemetry-save IOException handling can unintentionally enable telemetry for a run when the settings file can’t be read/persisted under contention.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 3
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The new retry/fallback paths currently treat broad IOException cases as “file in use,” which can mask unrelated IO failures and diverges from the intended contention-only handling.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

MSStore.CLI.UnitTests/ConfigurationManagerUnitTests.cs:39

• In SaveAsyncWaitsForOtherProcessToReleaseTheFile, the lock-holding FileStream isn't in a using/try-finally. If an assertion fails before the explicit Dispose(), the file can remain locked and cascade failures into later tests/cleanup. Making it a using var ensures cleanup even on failure.

MSStore.CLI/Services/ConfigurationManager.cs:122

• OpenAsync retries on any IOException (excluding File/DirectoryNotFound). This can add avoidable startup delay and log noise for IO failures that will never resolve with backoff (e.g., path too long, media errors). Retrying only on sharing/lock violations keeps the retry logic aligned with the "file in use" goal.

                catch (IOException ex) when (attempt < MaxOpenAttempts && ex is not FileNotFoundException and not DirectoryNotFoundException)

• Files reviewed: 3/3 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟢 Approval recommended

The concurrency fix is well-scoped, aligns with the stated behavior (no crash + no clobber under lock), and is backed by targeted unit tests; only a minor test-hygiene nit was found.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

MSStore.CLI.UnitTests/ConfigurationManagerUnitTests.cs:39

• In SaveAsyncWaitsForOtherProcessToReleaseTheFile, the lock FileStream is manually disposed later. If an assertion fails before otherProcessFile.Dispose(), the handle can leak and keep the file locked, potentially causing cleanup or later tests to fail for the wrong reason. Using a using var declaration ensures the handle is always released even on early test failure (you can still call Dispose early to release the lock before awaiting the save).

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Alexandre Zollinger Chohfi (azchohfi)]

Took a pass over this. The ConfigurationManager retry/backoff is sound and I verified the Unix errno constants are correct — dotnet/runtime maps EWOULDBLOCK through new IOException(msg, errorInfo.RawErrno), so HResult really is 11 on Linux and 35 on BSD/macOS.

Build is clean and the 5 new tests pass locally on Windows. Three things below: one behavioural change that reaches outside the diff and I think is a real bug, one suggestion about the root cause, and a test-flakiness nit.

[Alexandre Zollinger Chohfi (azchohfi)]

Copilot please address the three review comments I just left on this PR.

1. MSStore.CLI/Services/ConfigurationManager.cs:105 — this one is a real bug and reaches outside the diff.

LoadAsync(clearInvalidConfig: true) now swallows contention and returns a default T. But CLIConfigurator.ResetAsync loads config that way and then reads config.ClientId from it. An empty config means ClientId.HasValue is false, so the TryClearCredentials guard is skipped entirely, ClearAsync wipes settings.json, and we return true — leaving an orphaned credential in the OS store with no ClientId left to ever find it again. The comment directly above that guard says this is exactly the state we must never reach.

The root problem is that ResetAsync can't distinguish "no ClientId was ever stored" from "I couldn't read the file". Please keep that signal instead of flattening it — either a TryLoadAsync-style API that reports whether the read actually succeeded (like LoadTelemetryConfigurationsAsync already does in Program), or make the default-on-contention fallback opt-in so LoadAsync keeps throwing for callers that haven't asked for it. Then fix ResetAsync to bail out rather than wipe when the read failed, and add test coverage for that case specifically.

2. MSStore.CLI/Services/ConfigurationManager.cs:144 — the exclusive lock on reads is the actual root cause here.

Every open takes FileAccess.ReadWrite, FileShare.None, including the pure-read path. In steady state two msstore --help runs only read telemetrySettings.json (changed == false, since the guid only rotates every 24h), so they collide purely because reading grabs an exclusive lock. Please thread access/share through OpenAsync and open the LoadAsync path as FileAccess.Read, FileShare.Read, keeping ReadWrite/None for SaveAsync and ClearAsync. Readers stop blocking each other, a writer still locks everyone out so we can't read a half-written file, and the retry loop plus the fail-closed telemetry branch stop being load-bearing.

3. MSStore.CLI.UnitTests/ConfigurationManagerUnitTests.cs:49 — test flakiness.

SaveAsyncWaitsForOtherProcessToReleaseTheFile only passes if the lock is released inside the ~500 ms retry budget (attempts land at roughly 0/50/150/300/500 ms, and the 5th is the last). If the thread gets starved on a loaded runner, await saveTask throws. Please make the wait deterministic so this can't flake on the ubuntu/macos legs.

For context on what I checked: the build is clean, all 5 new tests pass locally on Windows, and the Unix errno constants are correct — dotnet/runtime maps EWOULDBLOCK through new IOException(msg, errorInfo.RawErrno), so HResult genuinely is 11 on Linux and 35 on BSD/macOS. No need to revisit those.

[Copilot]

Copilot please address the three review comments I just left on this PR.

1. MSStore.CLI/Services/ConfigurationManager.cs:105 — this one is a real bug and reaches outside the diff....

Addressed all three in 2d819b6:

1. Added ConfigurationManager<T>.TryLoadAsync returning (Configurations, Readable). ResetAsync now uses it and bails (no credential removal, no ClearAsync) when the read failed, instead of proceeding on a default config. Added ResetShouldNotWipeSettingsWhenTheConfigurationCouldNotBeRead for coverage.
2. Threaded FileAccess/FileShare through OpenAsync; LoadAsync now opens with FileAccess.Read, FileShare.Read (concurrent readers no longer block each other), SaveAsync/ClearAsync keep ReadWrite/None.
3. SaveAsyncWaitsForOtherProcessToReleaseTheFile now waits for an actual logged retry attempt (via a small ILogger fake) instead of a fixed delay, so it can't flake on a slow runner.

Full suite passes (199 tests, only the 2 pre-existing CredentialManagerUnix failures remain, unrelated to this PR and present on the base commit too — missing libsecret in this sandbox). CodeQL and code review validation both clean.

[Copilot]

🟡 Changes recommended

The new TryLoadAsync return-value contract is documented inconsistently with its behavior, and a unit test’s retry-signal logger can trigger on unrelated Information logs, weakening reliability.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

TryLoadAsync currently swallows all IOException (not just contention) and can misreport Readable, and a new reset failure message is logged at a level hidden by default so users may see a silent -1 exit.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟢 Approval recommended

The concurrency crash is addressed with targeted retries and safe fallback logic, and the new behavior is backed by focused unit tests; remaining feedback is a minor logging deduplication nit.

Review details

• Files reviewed: 8/8 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

TryLoadAsync currently treats FileNotFoundException/DirectoryNotFoundException (e.g., delete-between-exists-and-open race) as Readable=false, contradicting the documented “missing file is known/default” contract and causing unnecessary aborts for callers like reset.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

MSStore.CLI/Services/ConfigurationManager.cs:139

• TryLoadAsync catches any IOException from LoadAsync(false) and returns (new T(), Readable=false). That includes FileNotFoundException/DirectoryNotFoundException if the file (or settings directory) is deleted between the File.Exists() check in LoadAsync and the subsequent open, which contradicts the interface docs that treat a missing file as a known/default state and can make callers (e.g., ResetAsync) abort unnecessarily on this race. Handle missing-file exceptions here by recreating defaults via ClearAsync and returning Readable=true, falling back to Readable=false only if recreation also fails.

• Files reviewed: 8/8 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟢 Approval recommended

The change set addresses the reported contention crash with bounded retries, safe fallback behavior, and targeted unit tests covering the critical concurrency scenarios.

Review details

• Files reviewed: 8/8 changed files
• Comments generated: 0 new
• Review effort level: Lite

[isourabh]

Will relook at this once the PR to route output to stdout is merged