From 43cd3a1544e2eed8b449758dfc1d66a329807606 Mon Sep 17 00:00:00 2001 From: Stuart Meeks Date: Tue, 25 Aug 2026 11:28:40 +0800 Subject: [PATCH] test: cover SelectionsLock's sentinel cleanup and exhaustion (#57) Three gaps, all in behaviour the type's own remarks rely on. DeleteOnClose is what makes stale-lock recovery unnecessary -- "there is no stale-lock recovery code to maintain" -- but no test asserted the sentinel is actually gone after dispose. Acquire_AfterRelease_Succeeds said so in a comment and then only checked that a second acquire worked, which it would even with a leftover file that the second acquirer simply reopened. The ~5.1s backoff ladder ending in a thrown IOException was never reached, so neither its message nor its inner exception was verified. And the lock's actual job -- serialising selections read-modify-write across processes -- was only inferred from a single same-process contention test. Now asserted: the sentinel is absent after dispose; a permanently held lock throws IOException naming the path, saying another process may hold it, and preserving the inner exception; and four concurrent contenders never overlap inside the critical section. Verified to bite: replacing DeleteOnClose with FileOptions.None fails Acquire_AfterRelease_Succeeds and Acquire_SerialisesConcurrentHolders. Both passed against that same change before. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 9 +++ .../Persistence/SelectionsLockTests.cs | 60 ++++++++++++++++++- 2 files changed, 66 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 738adba..9ae9121 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **`SelectionsLock`'s sentinel cleanup and exhaustion path were untested** (#57). The suite + covered acquire, contend and re-acquire, but never asserted that `FileOptions.DeleteOnClose` + actually removes the sentinel — one test's comment claimed it without checking — so dropping + that flag would have left a permanent lock file and still passed. The ~5.1 s backoff ladder + ending in a thrown `IOException` was never reached either, leaving its message and its + behaviour under a stuck peer unverified. Both are now asserted, along with the lock's actual + job: four concurrent holders with overlapping critical sections detected directly, rather + than inferred from one same-process test. + - **The Windows ACL hardening was verified by no test, behind a comment claiming it was** (#47). `CredentialsDirectoryTests` skipped its Windows case with *"verified via the file-perm integration path"*, and no such path existed anywhere in the suite — grepping for diff --git a/tests/NextIteration.SpectreConsole.Auth.Tests/Persistence/SelectionsLockTests.cs b/tests/NextIteration.SpectreConsole.Auth.Tests/Persistence/SelectionsLockTests.cs index 2938790..68b4fd9 100644 --- a/tests/NextIteration.SpectreConsole.Auth.Tests/Persistence/SelectionsLockTests.cs +++ b/tests/NextIteration.SpectreConsole.Auth.Tests/Persistence/SelectionsLockTests.cs @@ -52,11 +52,65 @@ public async Task Acquire_AfterRelease_Succeeds() (await SelectionsLock.AcquireAsync(lockPath, TestContext.Current.CancellationToken)).Dispose(); - // DeleteOnClose should clean up the sentinel file when the holder - // disposes; a fresh acquirer must succeed without seeing a stale - // lock. + // DeleteOnClose is what makes stale-lock recovery unnecessary, and the type's + // remarks lean on it ("there is no stale-lock recovery code to maintain"). The + // comment here used to claim it without asserting it, so dropping the flag would + // have left a permanent lock file and still passed (#57). + Assert.False(File.Exists(lockPath), "sentinel file survived dispose"); + using var second = await SelectionsLock.AcquireAsync(lockPath, TestContext.Current.CancellationToken); Assert.NotNull(second); } + + [Fact] + public async Task Acquire_WhenHeldThroughout_EventuallyThrowsWithAnActionableMessage() + { + using var temp = new TempDir(); + var lockPath = Path.Join(temp.Path, "selections.json.lock"); + + using var holder = await SelectionsLock.AcquireAsync(lockPath, TestContext.Current.CancellationToken); + + // The ~5.1s backoff ladder ending in a thrown IOException was never reached by + // any test, so neither its message nor its behaviour under a genuinely stuck + // peer was verified (#57). + var ex = await Assert.ThrowsAsync( + () => SelectionsLock.AcquireAsync(lockPath, TestContext.Current.CancellationToken)); + + Assert.Contains(lockPath, ex.Message, StringComparison.Ordinal); + Assert.Contains("Another process may be holding it", ex.Message, StringComparison.Ordinal); + + // The IOException that actually blocked acquisition is preserved, not discarded. + Assert.NotNull(ex.InnerException); + } + + [Fact] + public async Task Acquire_SerialisesConcurrentHolders() + { + using var temp = new TempDir(); + var lockPath = Path.Join(temp.Path, "selections.json.lock"); + + // The lock's actual job is serialising FileCredentialManager's selections + // read-modify-write. Assert mutual exclusion directly: overlapping critical + // sections are what a broken lock produces. + var inside = 0; + var overlaps = 0; + + async Task Contend() + { + using var held = await SelectionsLock.AcquireAsync(lockPath, TestContext.Current.CancellationToken); + if (Interlocked.Increment(ref inside) != 1) + { + _ = Interlocked.Increment(ref overlaps); + } + + await Task.Delay(15, TestContext.Current.CancellationToken); + _ = Interlocked.Decrement(ref inside); + } + + await Task.WhenAll(Enumerable.Range(0, 4).Select(_ => Contend())); + + Assert.Equal(0, overlaps); + Assert.False(File.Exists(lockPath), "sentinel file survived the last dispose"); + } } }