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"); + } } }