From 1fa50e15786140fa3f3a4dbcb45516b60920663d Mon Sep 17 00:00:00 2001 From: Kevin Harder Date: Fri, 28 Aug 2026 16:59:18 -0500 Subject: [PATCH] fix: harden search metadata lifecycles --- AGENTS.md | 9 + .../ui-lifecycle-and-resource-bounds.md | 14 +- ...-resource-cancellation-audit-2026-08-28.md | 79 +++-- src/SkillView.Core/Ui/RemoveConfirmModal.cs | 34 ++- .../Ui/SearchAgentMetadataLoader.cs | 141 +++++++++ src/SkillView.Core/Ui/SkillViewApp.cs | 75 ++--- .../Ui/RemoveConfirmModalTests.cs | 21 ++ .../Ui/SearchAgentMetadataCacheTests.cs | 24 +- .../Ui/SearchAgentMetadataLoaderTests.cs | 285 ++++++++++++++++++ 9 files changed, 608 insertions(+), 74 deletions(-) create mode 100644 src/SkillView.Core/Ui/SearchAgentMetadataLoader.cs create mode 100644 tests/SkillView.Tests/Ui/SearchAgentMetadataLoaderTests.cs diff --git a/AGENTS.md b/AGENTS.md index cdc23aa..149f2dc 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -176,6 +176,15 @@ the terminal, with both a full-screen TUI and scriptable CLI commands. `CancellationTokenSource`, capture its `CancellationToken` before the first await and use only that value afterward. Never dereference the source from a continuation after navigation can cancel and dispose it. +- Discover agent filtering previews at most four skills concurrently, applies + a 15-second deadline to each metadata preview, and retains the existing + two-minute whole-search deadline. Keep scheduling bounded across overlapping + searches through `SearchAgentMetadataLoader`; do not cache timeout, process, + or other transient preview failures as an empty agent list. +- A modal operation remains owned until its queued UI completion callback has + run and explicitly releases or closes the modal. `Task.IsCompleted` only + describes the worker; it is not permission for keyboard shortcuts to cancel, + close, retry, or escalate against controls whose completion is still queued. - `SkillViewApp` now keeps the search shell and pane state, while `SkillViewWorkflowCoordinator` owns install/update/installed/remove/cleanup/ doctor orchestration plus the shared inventory capture/rescan flow. Put new diff --git a/agent_docs/ui-lifecycle-and-resource-bounds.md b/agent_docs/ui-lifecycle-and-resource-bounds.md index 9e5288d..b592c47 100644 --- a/agent_docs/ui-lifecycle-and-resource-bounds.md +++ b/agent_docs/ui-lifecycle-and-resource-bounds.md @@ -52,6 +52,12 @@ logging, or subprocess adapters. capture limit only with evidence that a supported command needs more output. - `SearchAgentMetadataCache` is a thread-safe 512-entry LRU. Do not replace it with an unbounded dictionary. +- Discover agent metadata loading has a global four-preview concurrency bound, + a 15-second deadline per preview, and the search request's two-minute outer + deadline. A timeout or failed preview stays retryable; only a successful + preview or a result with definitively absent repository metadata is cached. + Keep both the per-request worker bound and shared slot bound so superseded + searches cannot temporarily multiply process concurrency. - Local inventory capture schedules synchronous filesystem work on the thread pool and runs it concurrently with `gh skill list`. Resolver, scanner, package-lock reader, and cleanup classification check cancellation between @@ -97,6 +103,11 @@ logging, or subprocess adapters. - Fire-and-forget UI work must consume `OperationCanceledException` only when its own app/view lifetime is canceled. Normal shutdown must not become a false `CRASH`, while unrelated cancellation and real faults remain visible. +- For synchronous modals that own an async task, non-null task identity is the + ownership boundary. A completed worker may still have an `IApplication.Invoke` + completion queued; shortcuts must remain blocked until that callback clears + the task or closes the modal. Cancel only when the owned task is still + running, then drain it before controls are disposed. ## Focused verification @@ -104,5 +115,6 @@ Run `dotnet build`, then `dotnet test --no-build`. Resource-focused coverage includes large subprocess output, LRU eviction, subscription disposal, callback-safe cancellation gates, bounded inventory files, iteration failures, overlapping preview cancellation, superseded inventory scans, asynchronous -removal cancellation/progress/error bounds, inline busy state, and layout +removal cancellation/progress/error bounds, bounded and timed metadata +previews, inline busy state, and layout guards at 80×24, 100×30, and 140×42. diff --git a/docs/reviews/adversarial-concurrency-resource-cancellation-audit-2026-08-28.md b/docs/reviews/adversarial-concurrency-resource-cancellation-audit-2026-08-28.md index b0ff8f8..3da8bb8 100644 --- a/docs/reviews/adversarial-concurrency-resource-cancellation-audit-2026-08-28.md +++ b/docs/reviews/adversarial-concurrency-resource-cancellation-audit-2026-08-28.md @@ -8,12 +8,15 @@ merged by PR #11 as `40bbf0b` Hardening branch: `fix/adversarial-hardening` Second hardening branch: `fix/resource-lifecycle-hardening-2` Third hardening branch: `fix/removal-lifecycle-hardening` -Status: Active remediation. PRs #12 and #13 are merged. Aggregate log retention, +Fourth hardening branch: `fix/search-metadata-hardening` +Status: Active remediation. PRs #12, #13, and #14 are merged. Aggregate log retention, callback-safe cancellation ownership, bounded/cancellable inventory I/O, and -portable asynchronous removal are implemented. Native handle-relative deletion -for hostile same-user mutation remains a separate security design; metadata -scheduling, install-modal ownership, root CLI cancellation, path identity, Esc/ -LRU coverage, and the lower-risk remainder stay scheduled. +portable asynchronous removal are implemented. The fourth batch adds bounded, +timed metadata-preview scheduling and closes the compact-removal queued- +completion race from PR #14's final review. Native handle-relative deletion for +hostile same-user mutation remains a separate security design; install-modal +ownership, root CLI cancellation, path identity, Esc coverage, and the lower- +risk remainder stay scheduled. ## Executive summary @@ -750,9 +753,10 @@ I/O, and a simulated iteration-time disconnect. Severity: **Medium** -Implementation status: **Partially completed.** New searches now supersede the -previous request and the whole search has a two-minute deadline. Per-metadata -preview deadlines and bounded scheduling remain in remediation item 8. +Implementation status: **Completed on `fix/search-metadata-hardening`.** New +searches supersede the previous request, the whole search has a two-minute +deadline, metadata previews have a 15-second per-item deadline, and no more +than four previews run concurrently across overlapping searches. Locations: @@ -787,6 +791,23 @@ rejected rather than canceling and superseding the stale request. - Preserve the bounded LRU metadata cache. - Add timeout, supersession, partial-failure, and 200-result tests. +### Remediation + +`SearchAgentMetadataLoader` now runs at most four preview workers per request +and enforces the same four-slot ceiling across overlapping/superseded searches. +Each metadata subprocess receives a linked 15-second deadline under the outer +two-minute search request. Parent cancellation is propagated; per-item timeout +and transient preview failure affect only that result and remain retryable +rather than being cached as a false empty-agent result. Missing repository +identity is the only non-preview case cached as definitively empty. + +Deterministic coverage holds all four slots while submitting the full 200-result +maximum, proves no fifth preview starts, then releases the batch and verifies +all results. Separate tests cover per-item timeout with a healthy concurrent +result, transient failure retry, and request cancellation reaching every active +preview. The LRU contract test now touches an older entry before eviction and +also verifies that updating an existing value refreshes recency. + ## Finding 8: install modal async handlers can outlive disposed dialogs Severity: **Medium** @@ -933,6 +954,21 @@ returned after its initial progress event. Refusals now publish a forced terminal completed snapshot with one processed target, zero deleted targets, and the refusal error count. +PR #14's final post-merge review contained one suppressed finding from +unchanged lines. It was independently reassessed and accepted: + +1. **Compact-modal shortcuts can race a queued completion callback — correct + and fixed on `fix/search-metadata-hardening`.** `Task.IsCompleted` can become + true on a worker thread before the callback queued through + `IApplication.Invoke` commits `Removed`/`Failed` and clears or closes the + modal. During that interval, Esc/`n` could overwrite a successful outcome + with `Cancelled`, while `a` could open the advanced wizard against an + already-deleted skill. Any non-null task is now treated as owned until its + UI completion runs. Esc cancels only a still-running worker; completed-but- + queued workers display a finishing status and all close/escalate shortcuts + remain blocked. A state-classification regression test protects the worker- + complete/UI-pending boundary. + ### Impact - Large trees allocate proportional path lists before deletion begins. @@ -1146,14 +1182,14 @@ but the UI message and expected two-step Esc then q behavior are wrong. - Keep the application-level Ctrl+Q test because focused editors are precisely where global quit routing tends to regress. -## Test gap: the LRU test currently proves only FIFO eviction +## Test gap: the LRU test previously proved only FIFO eviction + +Status: **Closed on `fix/search-metadata-hardening`.** -`Store_EvictsLeastRecentlyUsedMetadataAtCapacity` inserts first, second, and -third without touching an older entry before eviction. A FIFO cache would pass -the same test. The implementation's `Has` method does call `Touch`, so this is -not evidence of a production defect, but the contract is unprotected. Touch -`first` before storing `third` and assert that `second` was evicted. Also test -that updating an existing value refreshes recency. +`Store_EvictsLeastRecentlyUsedMetadataAtCapacity` now touches `first` before +storing `third` and asserts that `second` was evicted. A separate test verifies +that updating an existing value refreshes recency and retains the replacement +metadata. ## Lower-risk hardening opportunities @@ -1214,14 +1250,17 @@ coordinated within their current scope: confirmed that supported .NET 10 APIs cannot make path validation and deletion atomic; an audited Unix/Windows native implementation remains a separate Finding 1 security follow-up rather than an implicit portable fix. -9. [ ] Finish metadata-preview deadlines and bounded scheduling (search - supersession and a whole-request deadline are complete). +9. [x] Finish metadata-preview deadlines and bounded scheduling. Search + supersession, the whole-request deadline, a per-preview deadline, and a + global four-process ceiling are complete. 10. [~] Make modal operation lifetimes awaitable and disposal-safe, including - stable token capture before the first await. Removal modals are complete; - the three install flows remain. + stable token capture before the first await. Removal modals are complete, + including the compact worker-complete/UI-pending shortcut boundary; the + three install flows remain. 11. [ ] Wire root CLI cancellation and bounded post-kill waiting. 12. [ ] Centralize cross-platform path identity semantics. -13. [ ] Correct Esc focus behavior and strengthen the LRU contract test. +13. [~] Correct Esc focus behavior and strengthen the LRU contract test. The + LRU contract is complete; Esc focus behavior remains. 14. [ ] Finish the lower-risk hardening and stress coverage. `FileLogSink` lock ordering, Doctor-to-tab cancellation, and Updates operation diff --git a/src/SkillView.Core/Ui/RemoveConfirmModal.cs b/src/SkillView.Core/Ui/RemoveConfirmModal.cs index d3f5e10..19aa6ef 100644 --- a/src/SkillView.Core/Ui/RemoveConfirmModal.cs +++ b/src/SkillView.Core/Ui/RemoveConfirmModal.cs @@ -28,6 +28,13 @@ internal enum Outcome internal sealed record Result(Outcome Outcome, RemoveService.RemoveReport? Report); + internal enum OperationOwnership + { + None, + Running, + AwaitingUiCompletion, + } + private readonly IApplication _app; private readonly RemoveService _remove; private readonly Logger _logger; @@ -61,6 +68,13 @@ internal static bool CanRunCompact(RemoveTargetEvaluation evaluation) return true; } + internal static OperationOwnership ClassifyOperation(Task? operation) => operation switch + { + null => OperationOwnership.None, + { IsCompleted: false } => OperationOwnership.Running, + _ => OperationOwnership.AwaitingUiCompletion, + }; + internal Result Show() { using var lifetime = new CancellationTokenSource(); @@ -287,10 +301,18 @@ async Task RunRemovalAsync(CancellationToken cancellationToken) else if (ch == 'n' || ch == 'N' || key.KeyCode == KeyCode.Esc) { key.Handled = true; - if (activeOperation is { IsCompleted: false }) + var operationOwnership = ClassifyOperation(activeOperation); + if (operationOwnership != OperationOwnership.None) { - lifetime.Cancel(); - status.Text = " canceling removal…"; + if (operationOwnership == OperationOwnership.Running) + { + lifetime.Cancel(); + status.Text = " canceling removal…"; + } + else + { + status.Text = " finishing removal…"; + } return; } lifetime.Cancel(); @@ -300,9 +322,11 @@ async Task RunRemovalAsync(CancellationToken cancellationToken) else if (ch == 'a' || ch == 'A') { key.Handled = true; - if (activeOperation is { IsCompleted: false }) + if (ClassifyOperation(activeOperation) != OperationOwnership.None) { - status.Text = " removal in progress — Esc cancels"; + status.Text = activeOperation!.IsCompleted + ? " finishing removal…" + : " removal in progress — Esc cancels"; return; } outcome = Outcome.EscalateToWizard; diff --git a/src/SkillView.Core/Ui/SearchAgentMetadataLoader.cs b/src/SkillView.Core/Ui/SearchAgentMetadataLoader.cs new file mode 100644 index 0000000..9e09248 --- /dev/null +++ b/src/SkillView.Core/Ui/SearchAgentMetadataLoader.cs @@ -0,0 +1,141 @@ +using System.Collections.Immutable; +using SkillView.Gh.Models; +using SkillView.Logging; + +namespace SkillView.Ui; + +/// +/// Loads the preview front matter needed by Discover's agent filter without +/// allowing one search to fan out into an unbounded number of subprocesses. +/// +internal sealed class SearchAgentMetadataLoader +{ + internal const int DefaultMaxConcurrency = 4; + internal static readonly TimeSpan DefaultPreviewTimeout = TimeSpan.FromSeconds(15); + + private readonly SearchAgentMetadataCache _cache; + private readonly Logger _logger; + private readonly int _maxConcurrency; + private readonly TimeSpan _previewTimeout; + private readonly SemaphoreSlim _previewSlots; + + internal SearchAgentMetadataLoader( + SearchAgentMetadataCache cache, + Logger logger, + int maxConcurrency = DefaultMaxConcurrency, + TimeSpan? previewTimeout = null) + { + ArgumentNullException.ThrowIfNull(cache); + ArgumentNullException.ThrowIfNull(logger); + ArgumentOutOfRangeException.ThrowIfLessThan(maxConcurrency, 1); + + var effectiveTimeout = previewTimeout ?? DefaultPreviewTimeout; + if (effectiveTimeout <= TimeSpan.Zero) + { + throw new ArgumentOutOfRangeException(nameof(previewTimeout)); + } + + _cache = cache; + _logger = logger; + _maxConcurrency = maxConcurrency; + _previewTimeout = effectiveTimeout; + _previewSlots = new SemaphoreSlim(maxConcurrency, maxConcurrency); + } + + internal async Task> FilterAsync( + IReadOnlyList results, + string? requestedAgent, + Func> loadPreviewAsync, + CancellationToken cancellationToken) + { + ArgumentNullException.ThrowIfNull(results); + ArgumentNullException.ThrowIfNull(loadPreviewAsync); + + var normalizedAgent = SearchAgentMetadataCache.NormalizeAgent(requestedAgent); + if (normalizedAgent is null) + { + return results; + } + + await Parallel.ForEachAsync( + results, + new ParallelOptions + { + CancellationToken = cancellationToken, + MaxDegreeOfParallelism = _maxConcurrency, + }, + async (result, token) => + { + await EnsureLoadedAsync(result, loadPreviewAsync, token).ConfigureAwait(false); + }).ConfigureAwait(false); + + cancellationToken.ThrowIfCancellationRequested(); + return _cache.Filter(results, normalizedAgent); + } + + private async Task EnsureLoadedAsync( + SearchResultSkill result, + Func> loadPreviewAsync, + CancellationToken cancellationToken) + { + cancellationToken.ThrowIfCancellationRequested(); + if (_cache.Has(result)) + { + return; + } + + if (string.IsNullOrWhiteSpace(result.Repo)) + { + _cache.Store(result, ImmutableArray.Empty); + return; + } + + await _previewSlots.WaitAsync(cancellationToken).ConfigureAwait(false); + try + { + // Another overlapping search may have populated this result while + // this worker waited for a globally bounded preview slot. + if (_cache.Has(result)) + { + return; + } + + using var timeout = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); + timeout.CancelAfter(_previewTimeout); + try + { + var preview = await loadPreviewAsync(result, timeout.Token).ConfigureAwait(false); + if (!preview.Succeeded) + { + // A subprocess failure is transient. Leave it uncached so + // a later search can retry instead of persisting a false + // "agent not present" result for the cache lifetime. + return; + } + + cancellationToken.ThrowIfCancellationRequested(); + var agents = SearchAgentMetadataCache.ExtractAgentsFromMarkdown( + preview.MarkdownBody ?? preview.Body ?? string.Empty); + _cache.Store(result, agents); + } + catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) + { + throw; + } + catch (OperationCanceledException) when (timeout.IsCancellationRequested) + { + _logger.Warn( + "search.agent", + $"{result.Repo}/{result.SkillName}: metadata preview timed out after {_previewTimeout.TotalSeconds:F0}s"); + } + catch (Exception ex) + { + _logger.Warn("search.agent", $"{result.Repo}/{result.SkillName}: {ex.Message}"); + } + } + finally + { + _previewSlots.Release(); + } + } +} diff --git a/src/SkillView.Core/Ui/SkillViewApp.cs b/src/SkillView.Core/Ui/SkillViewApp.cs index cbec5a8..e978ca0 100644 --- a/src/SkillView.Core/Ui/SkillViewApp.cs +++ b/src/SkillView.Core/Ui/SkillViewApp.cs @@ -34,6 +34,7 @@ public sealed class SkillViewApp private readonly Func _applicationFactory; private readonly bool _probeOnRun; private readonly SearchAgentMetadataCache _searchAgentMetadata = new(); + private readonly SearchAgentMetadataLoader _searchAgentMetadataLoader; private readonly SkillViewWorkflowCoordinator _workflows; private readonly BackgroundTaskTracker _backgroundTasks; private const int MaxVisibleLogLines = 512; @@ -141,6 +142,7 @@ internal SkillViewApp( _applicationFactory = applicationFactory; _probeOnRun = probeOnRun; _backgroundTasks = new BackgroundTaskTracker(LogUnhandledException); + _searchAgentMetadataLoader = new SearchAgentMetadataLoader(_searchAgentMetadata, services.Logger); _workflows = new SkillViewWorkflowCoordinator( services, options, @@ -1064,13 +1066,15 @@ private void SubmitSearch() var owner = _ownerField?.Text.Trim(); var agent = _agentField?.Text.Trim(); var limit = _limitUpDown?.Value ?? GhSkillSearchService.DefaultLimit; + var allowHiddenDirs = HiddenDirsEnabled; UpdateContextBar(); RunOwnedTask( () => RunSearchAsync( query, string.IsNullOrEmpty(owner) ? null : owner, limit, - string.IsNullOrEmpty(agent) ? null : agent), + string.IsNullOrEmpty(agent) ? null : agent, + allowHiddenDirs), "search"); } @@ -1163,7 +1167,12 @@ private async Task GetOrProbeEnvironmentAsync(CancellationTok }, cancellationToken).ConfigureAwait(false); } - private async Task RunSearchAsync(string query, string? owner = null, int? limit = null, string? agent = null) + private async Task RunSearchAsync( + string query, + string? owner = null, + int? limit = null, + string? agent = null, + bool allowHiddenDirs = false) { if (_ghPath is null) { @@ -1190,7 +1199,11 @@ private async Task RunSearchAsync(string query, string? owner = null, int? limit .SearchAsync(_ghPath, query, options, cancellationToken) .ConfigureAwait(false); var results = response.Results; - var filteredResults = await FilterResultsByAgentAsync(results, agent, cancellationToken).ConfigureAwait(false); + var filteredResults = await FilterResultsByAgentAsync( + results, + agent, + allowHiddenDirs, + cancellationToken).ConfigureAwait(false); await InvokeAsync(() => { if (!request.IsCurrent @@ -1270,55 +1283,25 @@ private void ReplaceSearchResults(IReadOnlyList results) private async Task> FilterResultsByAgentAsync( IReadOnlyList results, string? requestedAgent, + bool allowHiddenDirs, CancellationToken cancellationToken) { - var normalizedAgent = SearchAgentMetadataCache.NormalizeAgent(requestedAgent); - if (normalizedAgent is null || _ghPath is null) + if (SearchAgentMetadataCache.NormalizeAgent(requestedAgent) is null || _ghPath is null) { return results; } - foreach (var result in results) - { - cancellationToken.ThrowIfCancellationRequested(); - if (_searchAgentMetadata.Has(result)) - { - continue; - } - - if (string.IsNullOrWhiteSpace(result.Repo)) - { - _searchAgentMetadata.Store(result, ImmutableArray.Empty); - continue; - } - - try - { - var preview = await _services.PreviewService - .PreviewAsync( - _ghPath, - result.Repo, - result.SkillName, - allowHiddenDirs: ShouldAllowHiddenDirs(result, HiddenDirsEnabled), - cancellationToken: cancellationToken) - .ConfigureAwait(false); - var agents = preview.Succeeded - ? SearchAgentMetadataCache.ExtractAgentsFromMarkdown(preview.MarkdownBody ?? preview.Body ?? string.Empty) - : ImmutableArray.Empty; - _searchAgentMetadata.Store(result, agents); - } - catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) - { - throw; - } - catch (Exception ex) - { - _services.Logger.Warn("search.agent", $"{result.Repo}/{result.SkillName}: {ex.Message}"); - _searchAgentMetadata.Store(result, ImmutableArray.Empty); - } - } - - return _searchAgentMetadata.Filter(results, normalizedAgent); + var ghPath = _ghPath; + return await _searchAgentMetadataLoader.FilterAsync( + results, + requestedAgent, + (result, token) => _services.PreviewService.PreviewAsync( + ghPath, + result.Repo!, + result.SkillName, + allowHiddenDirs: ShouldAllowHiddenDirs(result, allowHiddenDirs), + cancellationToken: token), + cancellationToken).ConfigureAwait(false); } private async Task PreviewSelectedAsync() diff --git a/tests/SkillView.Tests/Ui/RemoveConfirmModalTests.cs b/tests/SkillView.Tests/Ui/RemoveConfirmModalTests.cs index 34c40e1..89070b7 100644 --- a/tests/SkillView.Tests/Ui/RemoveConfirmModalTests.cs +++ b/tests/SkillView.Tests/Ui/RemoveConfirmModalTests.cs @@ -9,6 +9,27 @@ namespace SkillView.Tests.Ui; public sealed class RemoveConfirmModalTests { + [Fact] + public void ClassifyOperation_TreatsCompletedWorkerAsOwnedUntilUiCompletion() + { + Assert.Equal( + RemoveConfirmModal.OperationOwnership.AwaitingUiCompletion, + RemoveConfirmModal.ClassifyOperation(Task.CompletedTask)); + } + + [Fact] + public void ClassifyOperation_DistinguishesIdleAndRunningWorkers() + { + var running = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + Assert.Equal( + RemoveConfirmModal.OperationOwnership.None, + RemoveConfirmModal.ClassifyOperation(null)); + Assert.Equal( + RemoveConfirmModal.OperationOwnership.Running, + RemoveConfirmModal.ClassifyOperation(running.Task)); + } + [Fact] public void CanRunCompact_TrueForSimpleSingleSkillEval() { diff --git a/tests/SkillView.Tests/Ui/SearchAgentMetadataCacheTests.cs b/tests/SkillView.Tests/Ui/SearchAgentMetadataCacheTests.cs index dcefb62..5c270bd 100644 --- a/tests/SkillView.Tests/Ui/SearchAgentMetadataCacheTests.cs +++ b/tests/SkillView.Tests/Ui/SearchAgentMetadataCacheTests.cs @@ -92,14 +92,34 @@ public void Store_EvictsLeastRecentlyUsedMetadataAtCapacity() cache.Store(first, ImmutableArray.Create("claude-code")); cache.Store(second, ImmutableArray.Create("github-copilot")); + Assert.True(cache.Has(first)); cache.Store(third, ImmutableArray.Create("gemini-cli")); Assert.Equal(2, cache.CountForTests); - Assert.False(cache.Has(first)); - Assert.True(cache.Has(second)); + Assert.True(cache.Has(first)); + Assert.False(cache.Has(second)); Assert.True(cache.Has(third)); } + [Fact] + public void Store_UpdatingExistingValueRefreshesRecency() + { + var cache = new SearchAgentMetadataCache(capacity: 2); + var first = Skill("owner/one", "alpha"); + var second = Skill("owner/two", "beta"); + var third = Skill("owner/three", "gamma"); + + cache.Store(first, ImmutableArray.Create("claude-code")); + cache.Store(second, ImmutableArray.Create("github-copilot")); + cache.Store(first, ImmutableArray.Create("gemini-cli")); + cache.Store(third, ImmutableArray.Create("github-copilot")); + + Assert.True(cache.Has(first)); + Assert.False(cache.Has(second)); + Assert.True(cache.Has(third)); + Assert.Single(cache.Filter([first], "gemini-cli")); + } + private static SearchResultSkill Skill(string repo, string skillName) => new( Description: null, diff --git a/tests/SkillView.Tests/Ui/SearchAgentMetadataLoaderTests.cs b/tests/SkillView.Tests/Ui/SearchAgentMetadataLoaderTests.cs new file mode 100644 index 0000000..d4031a3 --- /dev/null +++ b/tests/SkillView.Tests/Ui/SearchAgentMetadataLoaderTests.cs @@ -0,0 +1,285 @@ +using System.Collections.Immutable; +using SkillView.Gh.Models; +using SkillView.Logging; +using SkillView.Ui; +using Xunit; + +namespace SkillView.Tests.Ui; + +public sealed class SearchAgentMetadataLoaderTests +{ + [Fact] + public async Task FilterAsync_BoundsTwoHundredMetadataPreviews() + { + const int maxConcurrency = 4; + var cache = new SearchAgentMetadataCache(); + var loader = new SearchAgentMetadataLoader( + cache, + new Logger(LogLevel.Debug), + maxConcurrency, + TimeSpan.FromSeconds(5)); + var results = Enumerable.Range(0, 200) + .Select(i => Skill($"owner/repo-{i}", $"skill-{i}")) + .ToArray(); + var release = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var allSlotsEntered = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var active = 0; + var maxObserved = 0; + var calls = 0; + + var filtering = loader.FilterAsync( + results, + "claude-code", + async (result, cancellationToken) => + { + Interlocked.Increment(ref calls); + var nowActive = Interlocked.Increment(ref active); + UpdateMaximum(ref maxObserved, nowActive); + if (nowActive == maxConcurrency) + { + allSlotsEntered.TrySetResult(); + } + + try + { + await release.Task.WaitAsync(cancellationToken); + return Success(result, "claude-code"); + } + finally + { + Interlocked.Decrement(ref active); + } + }, + TestContext.Current.CancellationToken); + + await allSlotsEntered.Task.WaitAsync(TestContext.Current.CancellationToken); + Assert.Equal(maxConcurrency, Volatile.Read(ref calls)); + Assert.False(filtering.IsCompleted); + + release.SetResult(); + var filtered = await filtering.WaitAsync(TestContext.Current.CancellationToken); + + Assert.Equal(200, calls); + Assert.Equal(maxConcurrency, maxObserved); + Assert.Equal(200, filtered.Count); + Assert.Equal(200, cache.CountForTests); + } + + [Fact] + public async Task FilterAsync_BoundsConcurrencyAcrossOverlappingSearches() + { + const int maxConcurrency = 4; + var loader = new SearchAgentMetadataLoader( + new SearchAgentMetadataCache(), + new Logger(LogLevel.Debug), + maxConcurrency, + TimeSpan.FromSeconds(5)); + var release = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var allSlotsEntered = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var active = 0; + var maxObserved = 0; + var calls = 0; + + async Task LoadAsync(SearchResultSkill result, CancellationToken cancellationToken) + { + Interlocked.Increment(ref calls); + var nowActive = Interlocked.Increment(ref active); + UpdateMaximum(ref maxObserved, nowActive); + if (nowActive == maxConcurrency) + { + allSlotsEntered.TrySetResult(); + } + + try + { + await release.Task.WaitAsync(cancellationToken); + return Success(result, "claude-code"); + } + finally + { + Interlocked.Decrement(ref active); + } + } + + var first = loader.FilterAsync( + Enumerable.Range(0, 20).Select(i => Skill($"one/repo-{i}", $"one-{i}")).ToArray(), + "claude-code", + LoadAsync, + TestContext.Current.CancellationToken); + var second = loader.FilterAsync( + Enumerable.Range(0, 20).Select(i => Skill($"two/repo-{i}", $"two-{i}")).ToArray(), + "claude-code", + LoadAsync, + TestContext.Current.CancellationToken); + + await allSlotsEntered.Task.WaitAsync(TestContext.Current.CancellationToken); + Assert.Equal(maxConcurrency, Volatile.Read(ref calls)); + + release.SetResult(); + await Task.WhenAll(first, second).WaitAsync(TestContext.Current.CancellationToken); + + Assert.Equal(40, calls); + Assert.Equal(maxConcurrency, maxObserved); + } + + [Fact] + public async Task FilterAsync_TimesOutOnePreviewWithoutBlockingOtherResults() + { + var logger = new Logger(LogLevel.Debug); + var cache = new SearchAgentMetadataCache(); + var loader = new SearchAgentMetadataLoader( + cache, + logger, + maxConcurrency: 2, + previewTimeout: TimeSpan.FromMilliseconds(50)); + var stalled = Skill("owner/stalled", "stalled"); + var healthy = Skill("owner/healthy", "healthy"); + + var filtered = await loader.FilterAsync( + [stalled, healthy], + "claude-code", + async (result, cancellationToken) => + { + if (ReferenceEquals(result, stalled)) + { + await Task.Delay(Timeout.InfiniteTimeSpan, cancellationToken); + } + + return Success(result, "claude-code"); + }, + TestContext.Current.CancellationToken); + + Assert.Collection(filtered, result => Assert.Same(healthy, result)); + Assert.Equal(1, cache.CountForTests); + Assert.Contains( + logger.Snapshot(), + entry => entry.Category == "search.agent" + && entry.Message.Contains("timed out", StringComparison.Ordinal)); + } + + [Fact] + public async Task FilterAsync_DoesNotCacheTransientPreviewFailure() + { + var cache = new SearchAgentMetadataCache(); + var loader = new SearchAgentMetadataLoader(cache, new Logger(LogLevel.Debug)); + var result = Skill("owner/repo", "demo"); + var calls = 0; + + var first = await loader.FilterAsync( + [result], + "claude-code", + (item, _) => + { + Interlocked.Increment(ref calls); + return Task.FromResult(PreviewResult.Failure( + item.Repo!, item.SkillName, version: null, exit: 1, err: "temporary failure")); + }, + TestContext.Current.CancellationToken); + + Assert.Empty(first); + Assert.Equal(0, cache.CountForTests); + + var second = await loader.FilterAsync( + [result], + "claude-code", + (item, _) => + { + Interlocked.Increment(ref calls); + return Task.FromResult(Success(item, "claude-code")); + }, + TestContext.Current.CancellationToken); + + Assert.Single(second); + Assert.Equal(2, calls); + Assert.Equal(1, cache.CountForTests); + } + + [Fact] + public async Task FilterAsync_PropagatesRequestCancellationToEveryActivePreview() + { + const int maxConcurrency = 3; + var cache = new SearchAgentMetadataCache(); + var loader = new SearchAgentMetadataLoader( + cache, + new Logger(LogLevel.Debug), + maxConcurrency, + TimeSpan.FromSeconds(5)); + using var cancellation = CancellationTokenSource.CreateLinkedTokenSource( + TestContext.Current.CancellationToken); + var allSlotsEntered = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var started = 0; + var canceled = 0; + + var filtering = loader.FilterAsync( + Enumerable.Range(0, 20) + .Select(i => Skill($"owner/repo-{i}", $"skill-{i}")) + .ToArray(), + "claude-code", + async (_, cancellationToken) => + { + if (Interlocked.Increment(ref started) == maxConcurrency) + { + allSlotsEntered.TrySetResult(); + } + + try + { + await Task.Delay(Timeout.InfiniteTimeSpan, cancellationToken); + throw new InvalidOperationException("The held preview should have been canceled."); + } + catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) + { + Interlocked.Increment(ref canceled); + throw; + } + }, + cancellation.Token); + + await allSlotsEntered.Task.WaitAsync(TestContext.Current.CancellationToken); + cancellation.Cancel(); + + await Assert.ThrowsAnyAsync( + () => filtering.WaitAsync(TestContext.Current.CancellationToken)); + Assert.Equal(maxConcurrency, canceled); + Assert.Equal(0, cache.CountForTests); + } + + private static PreviewResult Success(SearchResultSkill result, string agent) + { + var markdown = $"---\nname: {result.SkillName}\nagents:\n - {agent}\n---\nbody"; + return new PreviewResult + { + Repo = result.Repo!, + SkillName = result.SkillName, + Version = null, + Body = markdown, + MarkdownBody = markdown, + AssociatedFiles = ImmutableArray.Empty, + Succeeded = true, + ExitCode = 0, + ErrorMessage = null, + }; + } + + private static SearchResultSkill Skill(string repo, string skillName) => + new( + Description: null, + Namespace: "demo", + Path: "skills/" + skillName, + Repo: repo, + SkillName: skillName, + Stars: 1); + + private static void UpdateMaximum(ref int maximum, int candidate) + { + while (true) + { + var observed = Volatile.Read(ref maximum); + if (candidate <= observed + || Interlocked.CompareExchange(ref maximum, candidate, observed) == observed) + { + return; + } + } + } +}