diff --git a/AGENTS.md b/AGENTS.md index 32fa9b9..cdc23aa 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -132,10 +132,26 @@ the terminal, with both a full-screen TUI and scriptable CLI commands. mount point, or broken link) as a leaf, revalidate containment immediately before deletion, and keep cancellation/depth bounds explicit. Keep traversal lazy with per-depth enumerator frames so cancellation runs between entries - and retained state stays O(depth). Do not claim the path checks are atomic - against a hostile same-user process: supported .NET 10 deletion APIs remain - path-based on Windows and Unix; native handle-relative deletion is separate - follow-up work. + and retained state stays O(depth). TUI removal must use `RemoveAsync` / + `RemoveManyAsync`, whose progress is throttled to 10 updates/second; keep the + modal alive until cancellation quiesces and rescan when cancellation made + any partial filesystem change. Runtime reports retain at most 128 individual + errors plus an omission summary while preserving the exact error count. Do + not overwrite a mid-target cancellation snapshot with completed-target-only + totals: the batch progress adapter owns the latest aggregate state and keeps + processed-target and successfully-deleted-target counts distinct. Retryable + remove dialogs accumulate filesystem mutation totals across attempts and + compact-to-wizard escalation so later failures cannot hide a required rescan. + Every async removal entry point must publish a terminal canceled update even + when its token was already canceled; synthetic cancellation reports retain + the exact observed runtime-error count and mark cancellation explicitly. + All non-canceled return paths, including validation refusals, must publish a + terminal completed progress update with exact processed/deleted/error counts. + CLI JSON must not duplicate validation refusals or unaccepted warnings into + `runtimeErrors`. Do not claim the path checks are atomic + against a hostile same-user process: + supported .NET 10 deletion APIs remain path-based on Windows and Unix; + native handle-relative deletion is separate follow-up work. - Use `Logger.SubscribeWithReplay` whenever a consumer needs retained history plus live entries. Do not recreate snapshot-then-subscribe logic. Preserve the logger's message/total-character budgets and the file sink's date+size diff --git a/agent_docs/ui-lifecycle-and-resource-bounds.md b/agent_docs/ui-lifecycle-and-resource-bounds.md index 920bc76..9e5288d 100644 --- a/agent_docs/ui-lifecycle-and-resource-bounds.md +++ b/agent_docs/ui-lifecycle-and-resource-bounds.md @@ -60,6 +60,26 @@ logging, or subprocess adapters. lazy-enumerator `MoveNext` calls inside the I/O exception boundary because disconnects, ACL changes, and disappearing directories fail there rather than when the enumerable is created. +- Removal uses the same thread-pool boundary: compact remove, the advanced + wizard, cleanup batches, and agent-link unlinking call the asynchronous + `RemoveService` APIs. Progress callbacks are throttled to 10 per second and + immediately handed to `IApplication.Invoke`; they must not use `Progress` + and add another implicit queue. Esc cancels the active traversal, and each + removal window cancels and drains its owned task before disposing controls. + Partial cancellation counts still trigger inventory invalidation/rescan. + `BatchProgressAdapter` retains the latest aggregate snapshot so the outer + cancellation boundary cannot replace mid-target file/directory counts with + completed-target-only totals. Progress tracks processed targets separately + from successfully deleted targets; cancellation summaries must never treat a + failed-but-processed target as removed. Each async removal entry point emits + a final `IsCanceled` update for already-canceled tokens as well as in-flight + work. Retryable remove dialogs retain cumulative file/directory mutation + totals across attempts and across compact-to-wizard escalation, while the + displayed progress remains per-attempt. Synthetic cancellation reports mark + cancellation explicitly and retain the exact runtime-error count even when + the individual logged details are unavailable. + Traversal retains O(depth) enumerator state and no more than 128 detailed + runtime errors plus one omission summary. - Logger subscriptions are disposable. Every long-lived subscriber must retain and dispose its subscription. Disposal deactivates registrations that were already snapshotted and waits for an in-flight callback, so no callback can @@ -83,5 +103,6 @@ logging, or subprocess adapters. 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, inline busy -state, and layout guards at 80×24, 100×30, and 140×42. +overlapping preview cancellation, superseded inventory scans, asynchronous +removal cancellation/progress/error bounds, 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 0532b12..b0ff8f8 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 @@ -7,10 +7,13 @@ PR follow-up reviewed: `df62a22` (`fix: close remaining lifecycle review gaps`), merged by PR #11 as `40bbf0b` Hardening branch: `fix/adversarial-hardening` Second hardening branch: `fix/resource-lifecycle-hardening-2` -Status: Active remediation. PR #12 is merged. Its final disk-budget follow-up, -the analogous cancellation-callback lock hazard, and bounded/cancellable local -inventory I/O are implemented on the second hardening branch. Removal UI work, -hostile same-user path replacement, and findings 7-11 and 14 remain scheduled. +Third hardening branch: `fix/removal-lifecycle-hardening` +Status: Active remediation. PRs #12 and #13 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. ## Executive summary @@ -788,6 +791,12 @@ rejected rather than canceling and superseding the stale request. Severity: **Medium** +Implementation status: **Install flows remain open. The removal modal/wizard +now demonstrate the target lifecycle on `fix/removal-lifecycle-hardening`: +stable token capture, an explicitly retained operation task, inner queued- +callback lifetime checks, cancellation, and task drain before control +disposal.** + Locations: - `src/SkillView.Core/Ui/InstallConfirmModal.cs`, async accepting handler around lines 189-264. @@ -881,6 +890,10 @@ is the notable exception. Severity: **Medium** +Implementation status: **Completed on `fix/removal-lifecycle-hardening`. The +portable path remains non-atomic against hostile same-user path replacement, +as documented under Finding 1.** + Locations: - `src/SkillView.Core/Inventory/RemoveService.cs`, `.ToList()` traversal around lines 106-121. @@ -890,9 +903,35 @@ Locations: ### Current behavior -`RemoveService` materializes every file path and every directory path before -deleting anything. The UI calls it synchronously from Terminal.Gui event -handlers. There is no cancellation token or progress reporting. +The original implementation materialized every file path and directory path +before deletion and ran synchronously from Terminal.Gui event handlers. The +earlier branch replaced that traversal with O(depth) enumerator frames. This +branch completes the remediation: compact remove, advanced remove, cleanup +batches, cleanup validation, and agent-link unlinking run off the UI thread; +Esc cancels active work; progress is throttled to 10 updates per second; and +each owning window cancels and drains its task before disposing controls. + +Cancellation publishes exact aggregate progress, including cancellation +between batch targets. The workflow invalidates and rescans inventory whenever +even a partial target deleted files or directories. Runtime failure detail is +bounded to 128 messages plus an omission summary while `ErrorCount` preserves +the exact total. PR #14's first Copilot review found two terminal-progress +gaps: already-canceled link removal omitted its final canceled event, and the +outer batch catch could overwrite exact mid-target counts with completed-target +totals. Both are fixed by explicit early-cancellation publication and by making +the batch adapter retain and republish its latest aggregate snapshot. + +PR #14's Balanced follow-up review surfaced six suppressed accounting issues +in the same change set. All were accepted and fixed in the PR: progress now +distinguishes processed targets from successfully deleted targets; compact and +advanced removal flows retain mutation totals across retries and escalation; +cancellation reports preserve the exact observed runtime-error count while +marking cancellation separately; and remove JSON only emits runtime-error +fields after validation and confirmation permit an actual removal or dry run. +The subsequent Balanced review also found that a refused direct removal +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. ### Impact @@ -1169,14 +1208,17 @@ coordinated within their current scope: 6. [x] Add log character/byte budgets and correct disk rotation. 7. [x] Enforce aggregate disk retention during active-file growth and remove cancellation-callback execution from request/slot ownership locks. -8. [~] Move inventory and removal I/O off the UI thread with cancellation, and +8. [x] Move inventory and removal I/O off the UI thread with cancellation, and evaluate native handle-relative deletion for hostile same-user mutation. - Inventory scanning and cleanup classification are complete; asynchronous - removal/progress and native deletion evaluation remain. + The portable inventory/removal work is complete. The native evaluation + 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). -10. [ ] Make modal operation lifetimes awaitable and disposal-safe, including - stable token capture before the first await. +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. 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. diff --git a/src/SkillView.Core/Cli/CliDispatcher.cs b/src/SkillView.Core/Cli/CliDispatcher.cs index bccb746..c3578c2 100644 --- a/src/SkillView.Core/Cli/CliDispatcher.cs +++ b/src/SkillView.Core/Cli/CliDispatcher.cs @@ -1188,7 +1188,7 @@ private static void WriteRemoveText( } else { - Console.Error.WriteLine($" remove failed with {r.Errors.Length} error(s)"); + Console.Error.WriteLine($" remove failed with {r.ErrorCount} error(s)"); foreach (var e in r.Errors) Console.Error.WriteLine($" · {e}"); } } @@ -1206,6 +1206,8 @@ internal static string RenderRemoveJson( ParsedRemoveArgs p, RemoveValidator.RemoveValidation validation) { + var removalAttempted = validation.Allowed + && (!validation.RequiresSecondConfirm || p.Yes); using var ms = new MemoryStream(); using (var w = new Utf8JsonWriter(ms, new JsonWriterOptions { Indented = true })) { @@ -1218,6 +1220,7 @@ internal static string RenderRemoveJson( w.WriteString("scope", target.Scope.ToString()); w.WriteNumber("filesDeleted", r.FilesDeleted); w.WriteNumber("directoriesDeleted", r.DirectoriesDeleted); + w.WriteNumber("runtimeErrorCount", removalAttempted ? r.ErrorCount : 0); w.WriteStartArray("errors"); foreach (var e in validation.Errors) { @@ -1237,7 +1240,10 @@ internal static string RenderRemoveJson( } w.WriteEndArray(); w.WriteStartArray("runtimeErrors"); - foreach (var e in r.Errors) w.WriteStringValue(e); + if (removalAttempted) + { + foreach (var e in r.Errors) w.WriteStringValue(e); + } w.WriteEndArray(); w.WriteEndObject(); } diff --git a/src/SkillView.Core/Inventory/RemoveService.cs b/src/SkillView.Core/Inventory/RemoveService.cs index 0a49fef..c4c1103 100644 --- a/src/SkillView.Core/Inventory/RemoveService.cs +++ b/src/SkillView.Core/Inventory/RemoveService.cs @@ -1,4 +1,5 @@ using System.Collections.Immutable; +using System.Diagnostics; using System.IO; using SkillView.Inventory.Models; using SkillView.Logging; @@ -12,6 +13,8 @@ namespace SkillView.Inventory; public sealed class RemoveService { private const int MaxTraversalDepth = 256; + internal const int MaxRetainedErrors = 128; + private static readonly TimeSpan ProgressInterval = TimeSpan.FromMilliseconds(100); private readonly Logger _logger; private readonly Action? _entryObservedForTests; @@ -25,6 +28,16 @@ internal RemoveService(Logger logger, Action? entryObservedForTests) public sealed record Options(bool DryRun = false); + public sealed record RemoveProgress( + int TargetsProcessed, + int TargetsDeleted, + int FilesProcessed, + int DirectoriesProcessed, + int Errors, + string CurrentPath, + bool IsCompleted, + bool IsCanceled); + public sealed record RemoveReport( bool Succeeded, string ResolvedPath, @@ -33,6 +46,9 @@ public sealed record RemoveReport( ImmutableArray Errors, bool DryRun) { + public int ErrorCount { get; init; } = Errors.Length; + public bool IsCanceled { get; init; } + public static RemoveReport Refused(string resolved, string reason) => new( Succeeded: false, ResolvedPath: resolved, @@ -50,13 +66,20 @@ public sealed record BatchRemoveReport( ImmutableArray Errors, bool DryRun) { + public int ErrorCount { get; init; } = Errors.Length; + public bool IsCanceled { get; init; } + public static BatchRemoveReport FromSingle(RemoveReport report, int targetsDeleted) => new( Succeeded: report.Succeeded, TargetsDeleted: targetsDeleted, FilesDeleted: report.FilesDeleted, DirectoriesDeleted: report.DirectoriesDeleted, Errors: report.Errors, - DryRun: report.DryRun); + DryRun: report.DryRun) + { + ErrorCount = report.ErrorCount, + IsCanceled = report.IsCanceled, + }; } /// Removes a previously-validated skill directory. Callers MUST run @@ -70,23 +93,38 @@ public sealed record BatchRemoveReport( public RemoveReport Remove( RemoveValidator.RemoveValidation validation, Options? options = null, - CancellationToken cancellationToken = default) + CancellationToken cancellationToken = default, + IProgress? progress = null) { options ??= new Options(); - cancellationToken.ThrowIfCancellationRequested(); + var target = Path.GetFullPath(validation.ResolvedPath); + var progressTracker = new ProgressTracker(progress, _logger); + progressTracker.Publish(0, 0, 0, 0, target, force: true); + try + { + cancellationToken.ThrowIfCancellationRequested(); + } + catch (OperationCanceledException) + { + progressTracker.Publish(0, 0, 0, 0, target, force: true, isCanceled: true); + throw; + } + if (!validation.Allowed) { var reason = string.Join("; ", validation.Errors.Select(e => $"{e.Kind}: {e.Detail}")); _logger.Error("remove", $"refused: {reason}"); + progressTracker.Publish(1, 0, 0, 1, target, + force: true, isCompleted: true, targetsDeleted: 0); return RemoveReport.Refused(validation.ResolvedPath, reason); } - var target = Path.GetFullPath(validation.ResolvedPath); if (PathResolver.IsSymlink(target)) { if (options.DryRun) { _logger.Info("remove.dryrun", $"would remove symlink {target}"); + progressTracker.Publish(1, 1, 0, 0, target, force: true, isCompleted: true); return new RemoveReport(true, target, 1, 0, ImmutableArray.Empty, DryRun: true); } @@ -94,11 +132,13 @@ public RemoveReport Remove( { TryDeleteSymlink(target); _logger.Info("remove", $"removed symlink {target}"); + progressTracker.Publish(1, 1, 0, 0, target, force: true, isCompleted: true); return new RemoveReport(true, target, 1, 0, ImmutableArray.Empty, DryRun: false); } catch (Exception ex) { _logger.Error("remove", $"delete symlink {target} failed: {ex.Message}"); + progressTracker.Publish(1, 0, 0, 1, target, force: true, isCompleted: true); return new RemoveReport(false, target, 0, 0, ImmutableArray.Create($"{target}: {ex.Message}"), DryRun: false); } @@ -107,10 +147,11 @@ public RemoveReport Remove( if (!Directory.Exists(target)) { _logger.Warn("remove", $"target missing at execute time: {target}"); + progressTracker.Publish(1, 0, 0, 1, target, force: true, isCompleted: true); return RemoveReport.Refused(target, $"target '{target}' no longer exists"); } - var errors = ImmutableArray.CreateBuilder(); + var errors = new FailureCollector(); int files = 0, dirs = 0; var pending = new Stack(); pending.Push(new TraversalFrame(target, depth: 0)); @@ -128,6 +169,7 @@ public RemoveReport Remove( out var attributes, out var validationError)) { RecordFailure(frame.Path, validationError!, errors); + progressTracker.Publish(0, files, dirs, errors.Count, frame.Path); PopAndDispose(pending); continue; } @@ -143,6 +185,7 @@ public RemoveReport Remove( { DeleteLeaf(frame.Path, expectedReparsePoint: true, options.DryRun, target, ref files, errors); + progressTracker.Publish(0, files, dirs, errors.Count, frame.Path); PopAndDispose(pending); continue; } @@ -151,6 +194,7 @@ public RemoveReport Remove( { DeleteLeaf(frame.Path, expectedReparsePoint: false, options.DryRun, target, ref files, errors); + progressTracker.Publish(0, files, dirs, errors.Count, frame.Path); PopAndDispose(pending); continue; } @@ -159,6 +203,7 @@ public RemoveReport Remove( { RecordFailure(frame.Path, $"directory nesting exceeds the safety limit of {MaxTraversalDepth}", errors); + progressTracker.Publish(0, files, dirs, errors.Count, frame.Path); PopAndDispose(pending); continue; } @@ -175,6 +220,7 @@ public RemoveReport Remove( catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) { RecordFailure(frame.Path, $"enumerate failed: {ex.Message}", errors); + progressTracker.Publish(0, files, dirs, errors.Count, frame.Path); PopAndDispose(pending); } continue; @@ -190,6 +236,7 @@ public RemoveReport Remove( catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) { RecordFailure(frame.Path, $"enumerate failed: {ex.Message}", errors); + progressTracker.Publish(0, files, dirs, errors.Count, frame.Path); PopAndDispose(pending); continue; } @@ -199,6 +246,7 @@ public RemoveReport Remove( var child = frame.Current; _entryObservedForTests?.Invoke(child); cancellationToken.ThrowIfCancellationRequested(); + progressTracker.Publish(0, files, dirs, errors.Count, child); pending.Push(new TraversalFrame(child, frame.Depth + 1)); continue; } @@ -206,8 +254,15 @@ public RemoveReport Remove( var completedPath = frame.Path; PopAndDispose(pending); DeleteDirectory(completedPath, options.DryRun, target, ref dirs, errors); + progressTracker.Publish(0, files, dirs, errors.Count, completedPath); } } + catch (OperationCanceledException) + { + progressTracker.Publish(0, files, dirs, errors.Count, target, + force: true, isCanceled: true); + throw; + } finally { while (pending.Count > 0) @@ -219,8 +274,13 @@ public RemoveReport Remove( if (options.DryRun) { _logger.Info("remove.dryrun", $"would remove {target}: {files} file(s), {dirs} dir(s)"); + progressTracker.Publish(1, files, dirs, errors.Count, target, + force: true, isCompleted: true); return new RemoveReport(errors.Count == 0, target, files, dirs, - errors.ToImmutable(), DryRun: true); + errors.ToImmutable(), DryRun: true) + { + ErrorCount = errors.Count, + }; } if (errors.Count == 0) @@ -232,15 +292,104 @@ public RemoveReport Remove( _logger.Error("remove", $"remove {target} completed with {errors.Count} error(s)"); } + progressTracker.Publish(1, files, dirs, errors.Count, target, + force: true, isCompleted: true); + return new RemoveReport( Succeeded: errors.Count == 0, ResolvedPath: target, FilesDeleted: files, DirectoriesDeleted: dirs, Errors: errors.ToImmutable(), - DryRun: false); + DryRun: false) + { + ErrorCount = errors.Count, + }; } + /// Runs filesystem-bound removal work on the thread pool so TUI callers do + /// not block Terminal.Gui's event loop. The delegate deliberately observes + /// cancellation inside the traversal rather than passing the token to + /// Task.Run, which guarantees already-canceled calls still publish their + /// terminal progress state. + public Task RemoveAsync( + RemoveValidator.RemoveValidation validation, + Options? options = null, + CancellationToken cancellationToken = default, + IProgress? progress = null) => + Task.Run(() => Remove(validation, options, cancellationToken, progress)); + + /// Removes one inventory-observed symlink without following its target. + /// This is used for the wizard's "unlink from agent" action, which has no + /// skill-directory validation object because it intentionally leaves the + /// canonical installation in place. + public Task RemoveLinkAsync( + string path, + CancellationToken cancellationToken = default, + IProgress? progress = null) => + Task.Run(() => + { + var fullPath = Path.GetFullPath(path); + var progressTracker = new ProgressTracker(progress, _logger); + progressTracker.Publish(0, 0, 0, 0, fullPath, force: true); + try + { + cancellationToken.ThrowIfCancellationRequested(); + } + catch (OperationCanceledException) + { + progressTracker.Publish(0, 0, 0, 0, fullPath, + force: true, isCanceled: true); + throw; + } + + if (!PathResolver.IsSymlink(fullPath)) + { + const string detail = "path is no longer a symlink"; + _logger.Warn("remove.agent", $"{fullPath}: {detail}"); + progressTracker.Publish(1, 0, 0, 1, fullPath, force: true, isCompleted: true); + return new RemoveReport( + Succeeded: false, + ResolvedPath: fullPath, + FilesDeleted: 0, + DirectoriesDeleted: 0, + Errors: ImmutableArray.Create($"{fullPath}: {detail}"), + DryRun: false); + } + + try + { + cancellationToken.ThrowIfCancellationRequested(); + TryDeleteSymlink(fullPath); + _logger.Info("remove.agent", $"unlinked {fullPath}"); + progressTracker.Publish(1, 1, 0, 0, fullPath, force: true, isCompleted: true); + return new RemoveReport( + Succeeded: true, + ResolvedPath: fullPath, + FilesDeleted: 1, + DirectoriesDeleted: 0, + Errors: ImmutableArray.Empty, + DryRun: false); + } + catch (OperationCanceledException) + { + progressTracker.Publish(0, 0, 0, 0, fullPath, force: true, isCanceled: true); + throw; + } + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) + { + _logger.Warn("remove.agent", $"{fullPath}: {ex.Message}"); + progressTracker.Publish(1, 0, 0, 1, fullPath, force: true, isCompleted: true); + return new RemoveReport( + Succeeded: false, + ResolvedPath: fullPath, + FilesDeleted: 0, + DirectoriesDeleted: 0, + Errors: ImmutableArray.Create($"{fullPath}: {ex.Message}"), + DryRun: false); + } + }); + private static void PopAndDispose(Stack frames) => frames.Pop().Dispose(); @@ -277,7 +426,7 @@ private void DeleteLeaf( bool dryRun, string target, ref int files, - ImmutableArray.Builder errors) + FailureCollector errors) { if (!TryValidateEntry(target, path, allowLeafReparsePoint: true, out var attributes, out var validationError)) @@ -322,7 +471,7 @@ private void DeleteDirectory( bool dryRun, string target, ref int dirs, - ImmutableArray.Builder errors) + FailureCollector errors) { if (!TryValidateEntry(target, path, allowLeafReparsePoint: false, out var attributes, out var validationError)) @@ -357,7 +506,7 @@ private void DeleteDirectory( private void RecordFailure( string path, string detail, - ImmutableArray.Builder errors) + FailureCollector errors) { _logger.Warn("remove", $"{path}: {detail}"); errors.Add($"{path}: {detail}"); @@ -443,38 +592,59 @@ private static void TryDeleteSymlink(string path) public BatchRemoveReport RemoveMany( IEnumerable validations, Options? options = null, - CancellationToken cancellationToken = default) + CancellationToken cancellationToken = default, + IProgress? progress = null) { options ??= new Options(); - var errors = ImmutableArray.CreateBuilder(); + var errors = new FailureCollector(); var seen = new HashSet(StringComparer.Ordinal); var targetsDeleted = 0; var filesDeleted = 0; var directoriesDeleted = 0; + var progressAdapter = new BatchProgressAdapter(progress, _logger); - foreach (var validation in validations) + try { - cancellationToken.ThrowIfCancellationRequested(); - var key = PathResolver.Normalize(validation.ResolvedPath); - if (!seen.Add(key)) + foreach (var validation in validations) { - continue; - } + cancellationToken.ThrowIfCancellationRequested(); + var key = PathResolver.Normalize(validation.ResolvedPath); + if (!seen.Add(key)) + { + continue; + } - var report = Remove(validation, options, cancellationToken); - filesDeleted += report.FilesDeleted; - directoriesDeleted += report.DirectoriesDeleted; - if (report.Succeeded) - { - targetsDeleted++; - } + var report = Remove(validation, options, cancellationToken, progressAdapter); + filesDeleted += report.FilesDeleted; + directoriesDeleted += report.DirectoriesDeleted; + if (report.Succeeded) + { + targetsDeleted++; + } - foreach (var error in report.Errors) - { - errors.Add($"{validation.ResolvedPath}: {error}"); + errors.AddRange( + report.Errors.Select(error => $"{validation.ResolvedPath}: {error}"), + report.ErrorCount); + progressAdapter.CompleteTarget( + targetsDeleted, + filesDeleted, + directoriesDeleted, + errors.Count, + validation.ResolvedPath); } } + catch (OperationCanceledException) + { + progressAdapter.CancelBatch(); + throw; + } + + progressAdapter.CompleteBatch( + targetsDeleted, + filesDeleted, + directoriesDeleted, + errors.Count); return new BatchRemoveReport( Succeeded: errors.Count == 0, @@ -482,6 +652,235 @@ public BatchRemoveReport RemoveMany( FilesDeleted: filesDeleted, DirectoriesDeleted: directoriesDeleted, Errors: errors.ToImmutable(), - DryRun: options.DryRun); + DryRun: options.DryRun) + { + ErrorCount = errors.Count, + }; + } + + public Task RemoveManyAsync( + IEnumerable validations, + Options? options = null, + CancellationToken cancellationToken = default, + IProgress? progress = null) => + Task.Run(() => RemoveMany(validations, options, cancellationToken, progress)); + + private sealed class ProgressTracker(IProgress? progress, Logger logger) + { + private long _lastPublished; + private bool _disabled; + + internal void Publish( + int targets, + int files, + int directories, + int errors, + string currentPath, + bool force = false, + bool isCompleted = false, + bool isCanceled = false, + int? targetsDeleted = null) + { + if (_disabled || progress is null) + { + return; + } + + var now = Stopwatch.GetTimestamp(); + if (!force && _lastPublished != 0 + && Stopwatch.GetElapsedTime(_lastPublished, now) < ProgressInterval) + { + return; + } + + _lastPublished = now; + try + { + progress.Report(new RemoveProgress( + targets, + targetsDeleted ?? (isCompleted && errors == 0 ? targets : 0), + files, + directories, + errors, + currentPath, + isCompleted, + isCanceled)); + } + catch (Exception ex) + { + _disabled = true; + logger.Warn("remove.progress", $"progress observer disabled: {ex.Message}"); + } + } + } + + internal sealed class FailureCollector + { + private readonly ImmutableArray.Builder _retained = + ImmutableArray.CreateBuilder(MaxRetainedErrors); + + internal int Count { get; private set; } + + internal void Add(string detail) + { + Count++; + if (_retained.Count < MaxRetainedErrors) + { + _retained.Add(detail); + } + } + + internal void AddRange(IEnumerable details, int totalCount) + { + Count += totalCount; + if (_retained.Count >= MaxRetainedErrors) + { + return; + } + + foreach (var detail in details) + { + if (_retained.Count >= MaxRetainedErrors) + { + break; + } + _retained.Add(detail); + } + } + + internal ImmutableArray ToImmutable() + { + var retained = _retained.ToImmutable(); + var omitted = Count - retained.Length; + return omitted > 0 + ? retained.Add($"… {omitted} additional error(s) omitted") + : retained; + } + } + + private sealed class BatchProgressAdapter(IProgress? progress, Logger logger) + : IProgress + { + private int _targetsProcessed; + private int _targetsDeleted; + private int _filesBeforeTarget; + private int _directoriesBeforeTarget; + private int _errorsBeforeTarget; + private int _latestTargets; + private int _latestTargetsDeleted; + private int _latestFiles; + private int _latestDirectories; + private int _latestErrors; + private string _currentPath = string.Empty; + private long _lastPublished; + private bool _disabled; + + public void Report(RemoveProgress value) + { + _currentPath = value.CurrentPath; + var aggregate = value with + { + TargetsProcessed = _targetsProcessed + value.TargetsProcessed, + TargetsDeleted = _targetsDeleted + value.TargetsDeleted, + FilesProcessed = _filesBeforeTarget + value.FilesProcessed, + DirectoriesProcessed = _directoriesBeforeTarget + value.DirectoriesProcessed, + Errors = _errorsBeforeTarget + value.Errors, + IsCompleted = false, + }; + Remember(aggregate); + Publish(aggregate, force: value.IsCanceled); + } + + internal void CompleteTarget( + int targetsDeleted, + int files, + int directories, + int errors, + string currentPath) + { + _targetsProcessed++; + _targetsDeleted = targetsDeleted; + _filesBeforeTarget = files; + _directoriesBeforeTarget = directories; + _errorsBeforeTarget = errors; + _currentPath = currentPath; + var aggregate = new RemoveProgress( + _targetsProcessed, + _targetsDeleted, + files, + directories, + errors, + currentPath, + IsCompleted: false, + IsCanceled: false); + Remember(aggregate); + Publish(aggregate, force: false); + } + + internal void CompleteBatch( + int targetsDeleted, + int files, + int directories, + int errors) + { + _targetsDeleted = targetsDeleted; + var aggregate = new RemoveProgress( + _targetsProcessed, + _targetsDeleted, + files, + directories, + errors, + _currentPath, + IsCompleted: true, + IsCanceled: false); + Remember(aggregate); + Publish(aggregate, force: true); + } + + internal void CancelBatch() => + Publish(new RemoveProgress( + _latestTargets, + _latestTargetsDeleted, + _latestFiles, + _latestDirectories, + _latestErrors, + _currentPath, + IsCompleted: false, + IsCanceled: true), force: true); + + private void Remember(RemoveProgress value) + { + _latestTargets = value.TargetsProcessed; + _latestTargetsDeleted = value.TargetsDeleted; + _latestFiles = value.FilesProcessed; + _latestDirectories = value.DirectoriesProcessed; + _latestErrors = value.Errors; + } + + private void Publish(RemoveProgress value, bool force) + { + if (_disabled || progress is null) + { + return; + } + + var now = Stopwatch.GetTimestamp(); + if (!force && _lastPublished != 0 + && Stopwatch.GetElapsedTime(_lastPublished, now) < ProgressInterval) + { + return; + } + + _lastPublished = now; + try + { + progress.Report(value); + } + catch (Exception ex) + { + _disabled = true; + logger.Warn("remove.progress", $"batch progress observer disabled: {ex.Message}"); + } + } } } diff --git a/src/SkillView.Core/Ui/CallbackProgress.cs b/src/SkillView.Core/Ui/CallbackProgress.cs new file mode 100644 index 0000000..5c43f51 --- /dev/null +++ b/src/SkillView.Core/Ui/CallbackProgress.cs @@ -0,0 +1,12 @@ +namespace SkillView.Ui; + +/// +/// Reports progress inline on the producer thread. TUI callers use this to +/// hand the update to IApplication.Invoke themselves, avoiding the +/// unbounded extra queue and ambiguous synchronization-context capture of +/// . +/// +internal sealed class CallbackProgress(Action callback) : IProgress +{ + public void Report(T value) => callback(value); +} diff --git a/src/SkillView.Core/Ui/CleanupScreen.cs b/src/SkillView.Core/Ui/CleanupScreen.cs index 09f01c3..a0c62aa 100644 --- a/src/SkillView.Core/Ui/CleanupScreen.cs +++ b/src/SkillView.Core/Ui/CleanupScreen.cs @@ -16,6 +16,8 @@ namespace SkillView.Ui; /// actions: remove, mark ignored, rescan, export. public sealed class CleanupScreen { + internal sealed record RemovalSummary(int Removed, int Failed, bool Confirmed); + private readonly IApplication _app; private readonly RemoveService _remove; private readonly Logger _logger; @@ -25,6 +27,8 @@ public sealed class CleanupScreen private readonly Func _confirmBatchRemoval; public int RemovedCount { get; private set; } + public int RemovedFileCount { get; private set; } + public int RemovedDirectoryCount { get; private set; } public int IgnoredCount { get; private set; } public CleanupScreen( @@ -47,6 +51,12 @@ public CleanupScreen( public void Show() { + using var lifetime = new CancellationTokenSource(); + Task? activeOperation = null; + RemoveService.RemoveProgress? lastProgress = null; + var windowActive = 1; + var closeAfterCancellation = 0; + using var window = new Window { Title = $"Cleanup — {_candidates.Length} candidate(s)", @@ -144,39 +154,183 @@ void Recompute() var status = new Label { - X = 0, + X = 2, Y = Pos.AnchorEnd(2), - Width = Dim.Fill(), + Width = Dim.Fill(2), Text = _candidates.Length == 0 ? " no cleanup candidates" : $" {_candidates.Length} candidate(s)", }; + var spinner = new SpinnerView + { + X = 0, + Y = Pos.AnchorEnd(2), + Visible = false, + AutoSpin = false, + }; var statusBar = new StatusBar(TuiHelpers.WithMarkdownShortcuts( BuildShortcuts(), includeOpenLink: false)); - TuiHelpers.ApplyScheme(SkillViewStyling.BaseSchemeName, window, header, table, detail, status, statusBar); + TuiHelpers.ApplyScheme(SkillViewStyling.BaseSchemeName, + window, header, table, detail, spinner, status, statusBar); + + void InvokeIfActive(Action action) + { + if (Volatile.Read(ref windowActive) == 0) + { + return; + } + + try + { + _app.Invoke(() => + { + if (Volatile.Read(ref windowActive) == 0) + { + return; + } + + try { action(); } + catch (Exception ex) { _logger.Error("cleanup.ui", ex.Message); } + }); + } + catch (Exception ex) + { + _logger.Error("cleanup.ui", ex.Message); + } + } + + async Task RunRemovalAsync(HashSet selectedRows, CancellationToken cancellationToken) + { + var selectedCount = selectedRows.Count(index => index >= 0 && index < _candidates.Length); + var progress = new CallbackProgress(value => + { + lastProgress = value; + InvokeIfActive(() => status.Text = FormatProgress(value)); + }); + + try + { + var summary = await RemoveSelectedAsync(selectedRows, cancellationToken, progress) + .ConfigureAwait(false); + InvokeIfActive(() => + { + spinner.AutoSpin = false; + spinner.Visible = false; + activeOperation = null; + status.Text = summary.Confirmed + ? $" removed {summary.Removed}, skipped/failed {summary.Failed}" + : " cleanup removal canceled"; + }); + } + catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) + { + var removed = lastProgress?.TargetsDeleted ?? 0; + RemovedCount += removed; + RemovedFileCount += lastProgress?.FilesProcessed ?? 0; + RemovedDirectoryCount += lastProgress?.DirectoriesProcessed ?? 0; + var failed = Math.Max(0, selectedCount - removed); + _logger.Debug("cleanup.remove", "cleanup removal canceled"); + InvokeIfActive(() => + { + spinner.AutoSpin = false; + spinner.Visible = false; + status.Text = $" removal canceled after {removed}; skipped/failed {failed}"; + if (Volatile.Read(ref closeAfterCancellation) != 0) + { + _app.RequestStop(); + } + else + { + activeOperation = null; + } + }); + } + catch (Exception ex) + { + _logger.Error("cleanup.remove", ex.Message); + InvokeIfActive(() => + { + spinner.AutoSpin = false; + spinner.Visible = false; + activeOperation = null; + status.Text = " cleanup removal failed — see logs"; + }); + } + } window.KeyDown += (_, key) => { var r = key.AsRune.Value; - if (r == 'r' || r == 'R') { DoRemove(wrapper.CheckedRows, status); key.Handled = true; } - else if (r == 'i' || r == 'I') { DoIgnore(wrapper.CheckedRows, status); key.Handled = true; } - else if (r == 'x' || r == 'X') { DoExport(status); key.Handled = true; } + if (r == 'r' || r == 'R') + { + key.Handled = true; + if (activeOperation is not null) + { + return; + } + + var selectedRows = wrapper.CheckedRows + .Where(index => index >= 0 && index < _candidates.Length) + .ToHashSet(); + if (selectedRows.Count == 0) + { + status.Text = " no cleanup candidates selected"; + return; + } + spinner.Visible = true; + spinner.AutoSpin = true; + lastProgress = null; + status.Text = " removing… Esc cancels"; + var cancellationToken = lifetime.Token; + activeOperation = RunRemovalAsync(selectedRows, cancellationToken); + } + else if ((r == 'i' || r == 'I') && activeOperation is null) + { + DoIgnore(wrapper.CheckedRows, status); + key.Handled = true; + } + else if ((r == 'x' || r == 'X') && activeOperation is null) + { + DoExport(status); + key.Handled = true; + } else if (key.KeyCode == KeyCode.Esc) { - _app.RequestStop(); key.Handled = true; + if (activeOperation is { IsCompleted: false }) + { + Interlocked.Exchange(ref closeAfterCancellation, 1); + lifetime.Cancel(); + status.Text = " canceling removal…"; + return; + } + + lifetime.Cancel(); + _app.RequestStop(); } }; - window.Add(header, table, detail, status, statusBar); + window.Add(header, table, detail, spinner, status, statusBar); table.SetFocus(); - _app.Run(window); + try + { + _app.Run(window); + } + finally + { + Interlocked.Exchange(ref windowActive, 0); + lifetime.Cancel(); + activeOperation?.GetAwaiter().GetResult(); + } } - private void DoRemove(HashSet checkedRows, Label status) + internal async Task RemoveSelectedAsync( + HashSet checkedRows, + CancellationToken cancellationToken = default, + IProgress? progress = null) { var selected = checkedRows .Where(i => i >= 0 && i < _candidates.Length) @@ -184,65 +338,69 @@ private void DoRemove(HashSet checkedRows, Label status) .ToImmutableArray(); if (selected.IsDefaultOrEmpty) { - status.Text = " no cleanup candidates selected"; - return; + return new RemovalSummary(Removed: 0, Failed: 0, Confirmed: false); } var response = _confirmBatchRemoval(BuildRemoveConfirmationText(selected)); if (response != 1) { - status.Text = " cleanup removal canceled"; - return; + return new RemovalSummary(Removed: 0, Failed: 0, Confirmed: false); } - var removed = 0; + var (validations, failedValidationCount) = await Task.Run( + () => BuildRemovalPlan(checkedRows, cancellationToken)).ConfigureAwait(false); + + var report = await _remove.RemoveManyAsync( + validations, + cancellationToken: cancellationToken, + progress: progress).ConfigureAwait(false); + var removed = report.TargetsDeleted; + var failed = failedValidationCount + validations.Length - removed; + RemovedCount += removed; + RemovedFileCount += report.FilesDeleted; + RemovedDirectoryCount += report.DirectoriesDeleted; + return new RemovalSummary(removed, failed, Confirmed: true); + } + + private (ImmutableArray Validations, int Failed) + BuildRemovalPlan(HashSet checkedRows, CancellationToken cancellationToken) + { var failed = 0; + var validations = ImmutableArray.CreateBuilder(); for (var i = 0; i < _candidates.Length; i++) { + cancellationToken.ThrowIfCancellationRequested(); if (!checkedRows.Contains(i)) continue; - var c = _candidates[i]; + var candidate = _candidates[i]; // For skill-backed candidates, run full validator. Non-skill cleanup // candidates need synthetic validations because they don't look like // install directories, but are still safe to remove when they stay // inside known scan roots. - RemoveValidator.RemoveValidation validation; - if (c.Kind == CleanupClassifier.CandidateKind.BrokenSymlink) - { - validation = ValidateBrokenSymlink(c.Path); - } - else if (c.Kind == CleanupClassifier.CandidateKind.EmptyDirectory) + var validation = candidate.Kind switch { - validation = ValidateEmptyDir(c.Path); - } - else if (c.Skill is not null) - { - validation = RemoveValidator.Validate(c.Skill, _scanRoots, _allSkills); - } - else - { - validation = RefuseUnsupportedCandidate(c); - } + CleanupClassifier.CandidateKind.BrokenSymlink => ValidateBrokenSymlink(candidate.Path), + CleanupClassifier.CandidateKind.EmptyDirectory => ValidateEmptyDir(candidate.Path), + _ when candidate.Skill is not null => + RemoveValidator.Validate(candidate.Skill, _scanRoots, _allSkills), + _ => RefuseUnsupportedCandidate(candidate), + }; if (!validation.Allowed || validation.RequiresSecondConfirm) { failed++; - _logger.Warn("cleanup", $"skipped {c.Path}: {(validation.Allowed ? "needs second confirm" : "validation refused")}"); + _logger.Warn("cleanup", $"skipped {candidate.Path}: {(validation.Allowed ? "needs second confirm" : "validation refused")}"); continue; } - try - { - var report = _remove.Remove(validation); - if (report.Succeeded) removed++; else failed++; - } - catch (Exception ex) - { - failed++; - _logger.Error("cleanup.remove", $"{c.Path}: {ex.Message}"); - } + validations.Add(validation); } - RemovedCount += removed; - status.Text = $" removed {removed}, skipped/failed {failed}"; + + return (validations.ToImmutable(), failed); } + private static string FormatProgress(RemoveService.RemoveProgress progress) => + progress.IsCanceled + ? $" canceling… removed {progress.TargetsDeleted} target(s)" + : $" removing… {progress.TargetsProcessed} target(s), {progress.FilesProcessed} file(s), {progress.DirectoriesProcessed} dir(s) Esc cancels"; + internal static string BuildRemoveConfirmationText( IReadOnlyList selected) { diff --git a/src/SkillView.Core/Ui/RemovalReportState.cs b/src/SkillView.Core/Ui/RemovalReportState.cs new file mode 100644 index 0000000..e334ab4 --- /dev/null +++ b/src/SkillView.Core/Ui/RemovalReportState.cs @@ -0,0 +1,121 @@ +using System.Collections.Immutable; +using SkillView.Inventory; + +namespace SkillView.Ui; + +/// +/// Keeps dialog-session mutation totals separate from the latest attempt's +/// outcome, and builds bounded synthetic reports when an operation exits by +/// cancellation or an unexpected exception. +/// +internal static class RemovalReportState +{ + internal static RemoveService.RemoveReport Accumulate( + RemoveService.RemoveReport? previous, + RemoveService.RemoveReport current) => + current with + { + FilesDeleted = Add(previous?.FilesDeleted ?? 0, current.FilesDeleted), + DirectoriesDeleted = Add( + previous?.DirectoriesDeleted ?? 0, + current.DirectoriesDeleted), + }; + + internal static RemoveService.BatchRemoveReport Accumulate( + RemoveService.BatchRemoveReport? previous, + RemoveService.BatchRemoveReport current) => + current with + { + TargetsDeleted = Add(previous?.TargetsDeleted ?? 0, current.TargetsDeleted), + FilesDeleted = Add(previous?.FilesDeleted ?? 0, current.FilesDeleted), + DirectoriesDeleted = Add( + previous?.DirectoriesDeleted ?? 0, + current.DirectoriesDeleted), + }; + + internal static RemoveService.RemoveReport Canceled( + string resolvedPath, + RemoveService.RemoveProgress? progress) + { + var errorCount = Math.Max(0, progress?.Errors ?? 0); + return new RemoveService.RemoveReport( + Succeeded: false, + ResolvedPath: resolvedPath, + FilesDeleted: Math.Max(0, progress?.FilesProcessed ?? 0), + DirectoriesDeleted: Math.Max(0, progress?.DirectoriesProcessed ?? 0), + Errors: UnavailableErrorDetails(errorCount), + DryRun: false) + { + ErrorCount = errorCount, + IsCanceled = true, + }; + } + + internal static RemoveService.BatchRemoveReport Canceled( + RemoveService.RemoveProgress? progress) + { + var errorCount = Math.Max(0, progress?.Errors ?? 0); + return new RemoveService.BatchRemoveReport( + Succeeded: false, + TargetsDeleted: Math.Max(0, progress?.TargetsDeleted ?? 0), + FilesDeleted: Math.Max(0, progress?.FilesProcessed ?? 0), + DirectoriesDeleted: Math.Max(0, progress?.DirectoriesProcessed ?? 0), + Errors: UnavailableErrorDetails(errorCount), + DryRun: false) + { + ErrorCount = errorCount, + IsCanceled = true, + }; + } + + internal static RemoveService.RemoveReport Failed( + string resolvedPath, + RemoveService.RemoveProgress? progress, + string detail) + { + var priorErrorCount = Math.Max(0, progress?.Errors ?? 0); + return new RemoveService.RemoveReport( + Succeeded: false, + ResolvedPath: resolvedPath, + FilesDeleted: Math.Max(0, progress?.FilesProcessed ?? 0), + DirectoriesDeleted: Math.Max(0, progress?.DirectoriesProcessed ?? 0), + Errors: FailureDetails(detail, priorErrorCount), + DryRun: false) + { + ErrorCount = Add(priorErrorCount, 1), + }; + } + + internal static RemoveService.BatchRemoveReport Failed( + RemoveService.RemoveProgress? progress, + string detail) + { + var priorErrorCount = Math.Max(0, progress?.Errors ?? 0); + return new RemoveService.BatchRemoveReport( + Succeeded: false, + TargetsDeleted: Math.Max(0, progress?.TargetsDeleted ?? 0), + FilesDeleted: Math.Max(0, progress?.FilesProcessed ?? 0), + DirectoriesDeleted: Math.Max(0, progress?.DirectoriesProcessed ?? 0), + Errors: FailureDetails(detail, priorErrorCount), + DryRun: false) + { + ErrorCount = Add(priorErrorCount, 1), + }; + } + + private static ImmutableArray UnavailableErrorDetails(int errorCount) => + errorCount == 0 + ? ImmutableArray.Empty + : ImmutableArray.Create( + $"… {errorCount} runtime error detail(s) unavailable after cancellation; see logs"); + + private static ImmutableArray FailureDetails(string detail, int priorErrorCount) => + priorErrorCount == 0 + ? ImmutableArray.Create(detail) + : ImmutableArray.Create( + detail, + $"… {priorErrorCount} earlier runtime error detail(s) unavailable; see logs"); + + private static int Add(int left, int right) => + (int)Math.Min(int.MaxValue, (long)Math.Max(0, left) + Math.Max(0, right)); +} diff --git a/src/SkillView.Core/Ui/RemoveConfirmModal.cs b/src/SkillView.Core/Ui/RemoveConfirmModal.cs index 357e673..d3f5e10 100644 --- a/src/SkillView.Core/Ui/RemoveConfirmModal.cs +++ b/src/SkillView.Core/Ui/RemoveConfirmModal.cs @@ -1,4 +1,3 @@ -using System.Collections.Immutable; using SkillView.Inventory; using SkillView.Inventory.Models; using SkillView.Logging; @@ -64,11 +63,15 @@ internal static bool CanRunCompact(RemoveTargetEvaluation evaluation) internal Result Show() { + using var lifetime = new CancellationTokenSource(); var validation = _evaluation.Items[0].Validation; var outcome = Outcome.Cancelled; RemoveService.RemoveReport? report = null; + RemoveService.RemoveProgress? lastProgress = null; + Task? activeOperation = null; + var dialogActive = 1; - var dialog = new Dialog + using var dialog = new Dialog { Title = " Remove skill ", Width = Dim.Percent(50), @@ -104,11 +107,18 @@ internal Result Show() var status = new Label { - X = 1, + X = 3, Y = Pos.AnchorEnd(3), - Width = Dim.Fill(2), + Width = Dim.Fill(4), Text = " [y] yes [n] no [a] advanced…", }; + var spinner = new SpinnerView + { + X = 1, + Y = Pos.AnchorEnd(3), + Visible = false, + AutoSpin = false, + }; var yesButton = new Button { @@ -131,36 +141,137 @@ internal Result Show() Text = "Advanced…", }; - yesButton.Accepting += (_, ev) => + void InvokeIfActive(Action action) { - ev.Handled = true; + if (Volatile.Read(ref dialogActive) == 0) + { + return; + } + try { - report = _remove.Remove(validation, new RemoveService.Options(DryRun: false)); - outcome = report.Succeeded ? Outcome.Removed : Outcome.Failed; - if (!report.Succeeded) + _app.Invoke(() => { - status.Text = $" remove failed: {report.Errors.FirstOrDefault() ?? "(no detail)"}"; - return; - } - _app.RequestStop(); + if (Volatile.Read(ref dialogActive) == 0) + { + return; + } + + try { action(); } + catch (Exception ex) { _logger.Error("remove.compact.ui", ex.Message); } + }); + } + catch (Exception ex) + { + _logger.Error("remove.compact.ui", ex.Message); + } + } + + void SetRunning(bool running) + { + spinner.AutoSpin = running; + spinner.Visible = running; + yesButton.Enabled = !running; + noButton.Enabled = !running; + advancedButton.Enabled = !running; + } + + async Task RunRemovalAsync(CancellationToken cancellationToken) + { + var progress = new CallbackProgress(value => + { + lastProgress = value; + InvokeIfActive(() => status.Text = FormatProgress(value)); + }); + + try + { + var completed = await _remove.RemoveAsync( + validation, + new RemoveService.Options(DryRun: false), + cancellationToken, + progress).ConfigureAwait(false); + report = RemovalReportState.Accumulate(report, completed); + outcome = completed.Succeeded ? Outcome.Removed : Outcome.Failed; + InvokeIfActive(() => + { + SetRunning(false); + if (completed.Succeeded) + { + status.Text = " removed — closing"; + _app.RequestStop(); + } + else + { + activeOperation = null; + var detail = TuiHelpers.ErrorSnippet(completed.Errors.FirstOrDefault()); + status.Text = detail.Length > 0 + ? $" remove failed: {detail}" + : " remove failed — see logs"; + } + }); + } + catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) + { + report = RemovalReportState.Accumulate( + report, + RemovalReportState.Canceled(validation.ResolvedPath, lastProgress)); + outcome = Outcome.Cancelled; + _logger.Debug("remove.compact", "removal canceled"); + InvokeIfActive(() => + { + SetRunning(false); + status.Text = " removal canceled"; + _app.RequestStop(); + }); } catch (Exception ex) { _logger.Error("remove.compact", ex.Message); + report = RemovalReportState.Accumulate( + report, + RemovalReportState.Failed( + validation.ResolvedPath, + lastProgress, + ex.Message)); outcome = Outcome.Failed; - status.Text = $" remove failed: {TuiHelpers.ErrorSnippet(ex.Message)}"; + InvokeIfActive(() => + { + SetRunning(false); + activeOperation = null; + var detail = TuiHelpers.ErrorSnippet(ex.Message); + status.Text = detail.Length > 0 + ? $" remove failed: {detail}" + : " remove failed — see logs"; + }); } + } + + yesButton.Accepting += (_, ev) => + { + ev.Handled = true; + if (activeOperation is not null) + { + return; + } + + SetRunning(true); + lastProgress = null; + status.Text = $" removing {_skill.Name}… Esc cancels"; + var cancellationToken = lifetime.Token; + activeOperation = RunRemovalAsync(cancellationToken); }; noButton.Accepting += (_, ev) => { ev.Handled = true; + lifetime.Cancel(); outcome = Outcome.Cancelled; _app.RequestStop(); }; advancedButton.Accepting += (_, ev) => { ev.Handled = true; + lifetime.Cancel(); outcome = Outcome.EscalateToWizard; _app.RequestStop(); }; @@ -176,25 +287,55 @@ internal Result Show() else if (ch == 'n' || ch == 'N' || key.KeyCode == KeyCode.Esc) { key.Handled = true; + if (activeOperation is { IsCompleted: false }) + { + lifetime.Cancel(); + status.Text = " canceling removal…"; + return; + } + lifetime.Cancel(); outcome = Outcome.Cancelled; _app.RequestStop(); } else if (ch == 'a' || ch == 'A') { key.Handled = true; + if (activeOperation is { IsCompleted: false }) + { + status.Text = " removal in progress — Esc cancels"; + return; + } outcome = Outcome.EscalateToWizard; _app.RequestStop(); } }; - dialog.Add(prompt, path, warnings, status, yesButton, noButton, advancedButton); + dialog.Add(prompt, path, warnings, spinner, status, yesButton, noButton, advancedButton); TuiHelpers.ApplyScheme(SkillViewStyling.DialogSchemeName, - dialog, prompt, path, warnings, status, + dialog, prompt, path, warnings, spinner, status, yesButton, noButton, advancedButton); - _app.Run(dialog); - dialog.Dispose(); + try + { + _app.Run(dialog); + } + finally + { + Interlocked.Exchange(ref dialogActive, 0); + lifetime.Cancel(); + activeOperation?.GetAwaiter().GetResult(); + } return new Result(outcome, report); } + + private static string FormatProgress(RemoveService.RemoveProgress progress) + { + if (progress.IsCanceled) + { + return $" canceling… {progress.FilesProcessed} file(s), {progress.DirectoriesProcessed} dir(s)"; + } + + return $" removing… {progress.FilesProcessed} file(s), {progress.DirectoriesProcessed} dir(s) Esc cancels"; + } } diff --git a/src/SkillView.Core/Ui/RemoveScreen.cs b/src/SkillView.Core/Ui/RemoveScreen.cs index 4de6a4a..2b67107 100644 --- a/src/SkillView.Core/Ui/RemoveScreen.cs +++ b/src/SkillView.Core/Ui/RemoveScreen.cs @@ -61,9 +61,13 @@ internal RemoveScreen( public void Show() { + using var lifetime = new CancellationTokenSource(); var targets = RemoveTargetResolver.BuildTargets(_target, _snapshot); var selectedIndex = FindInitialSelection(targets); var currentEvaluation = Evaluate(targets[selectedIndex]); + RemoveService.RemoveProgress? lastProgress = null; + Task? activeOperation = null; + var wizardActive = 1; using var wizard = new Wizard { @@ -148,12 +152,19 @@ public void Show() }; var status = new Label { - X = 0, + X = 2, Y = Pos.AnchorEnd(1), - Width = Dim.Fill(), + Width = Dim.Fill(2), Text = string.Empty, }; - confirmStep.Add(confirmText, secondConfirm, status); + var spinner = new SpinnerView + { + X = 0, + Y = Pos.AnchorEnd(1), + Visible = false, + AutoSpin = false, + }; + confirmStep.Add(confirmText, secondConfirm, spinner, status); TuiHelpers.ApplyScheme( SkillViewStyling.BaseSchemeName, @@ -164,6 +175,7 @@ public void Show() review, confirmText, secondConfirm, + spinner, status); wizard.AddStep(chooseStep); @@ -251,6 +263,11 @@ void RefreshEvaluation() { e.Handled = true; + if (activeOperation is not null) + { + return; + } + if (wizard.CurrentStep != confirmStep) { _app.RequestStop(); @@ -263,42 +280,135 @@ void RefreshEvaluation() return; } - status.Text = " removing…"; + spinner.Visible = true; + spinner.AutoSpin = true; + secondConfirm.Enabled = false; + lastProgress = null; + status.Text = " removing… Esc cancels"; + var evaluation = currentEvaluation; + var cancellationToken = lifetime.Token; + activeOperation = RunRemovalAsync(evaluation, cancellationToken); + }; - try + wizard.KeyDown += (_, key) => + { + if (key.KeyCode != KeyCode.Esc) { - var report = Execute(currentEvaluation); - LastReport = report; - if (report.Succeeded || report.TargetsDeleted > 0) - { - Confirmed = true; - _app.RequestStop(); - } - else - { - status.Text = $" remove failed — {report.Errors.Length} error(s); see logs"; - } + return; } - catch (Exception ex) + + key.Handled = true; + if (activeOperation is { IsCompleted: false }) { - _logger.Error("remove", ex.Message); - status.Text = " remove failed — see logs"; + lifetime.Cancel(); + status.Text = " canceling removal…"; + return; } + + lifetime.Cancel(); + _app.RequestStop(); }; - wizard.KeyDown += (_, key) => + void InvokeIfActive(Action action) { - if (key.KeyCode != KeyCode.Esc) + if (Volatile.Read(ref wizardActive) == 0) { return; } - _app.RequestStop(); - key.Handled = true; - }; + try + { + _app.Invoke(() => + { + if (Volatile.Read(ref wizardActive) == 0) + { + return; + } + + try { action(); } + catch (Exception ex) { _logger.Error("remove.ui", ex.Message); } + }); + } + catch (Exception ex) + { + _logger.Error("remove.ui", ex.Message); + } + } + + async Task RunRemovalAsync( + RemoveTargetEvaluation evaluation, + CancellationToken cancellationToken) + { + var progress = new CallbackProgress(value => + { + lastProgress = value; + InvokeIfActive(() => status.Text = FormatProgress(value)); + }); + + try + { + var report = await ExecuteAsync(evaluation, cancellationToken, progress) + .ConfigureAwait(false); + LastReport = RemovalReportState.Accumulate(LastReport, report); + InvokeIfActive(() => + { + spinner.AutoSpin = false; + spinner.Visible = false; + secondConfirm.Enabled = true; + if (report.Succeeded || report.TargetsDeleted > 0) + { + Confirmed = true; + _app.RequestStop(); + } + else + { + activeOperation = null; + status.Text = $" remove failed — {report.ErrorCount} error(s); see logs"; + } + }); + } + catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) + { + LastReport = RemovalReportState.Accumulate( + LastReport, + RemovalReportState.Canceled(lastProgress)); + _logger.Debug("remove", "removal canceled"); + InvokeIfActive(() => + { + spinner.AutoSpin = false; + spinner.Visible = false; + status.Text = " removal canceled"; + _app.RequestStop(); + }); + } + catch (Exception ex) + { + _logger.Error("remove", ex.Message); + LastReport = RemovalReportState.Accumulate( + LastReport, + RemovalReportState.Failed(lastProgress, ex.Message)); + InvokeIfActive(() => + { + spinner.AutoSpin = false; + spinner.Visible = false; + secondConfirm.Enabled = true; + activeOperation = null; + status.Text = " remove failed — see logs"; + }); + } + } RefreshEvaluation(); - _app.Run(wizard); + try + { + _app.Run(wizard); + } + finally + { + Interlocked.Exchange(ref wizardActive, 0); + lifetime.Cancel(); + activeOperation?.GetAwaiter().GetResult(); + } } internal string BuildSummary() @@ -324,22 +434,37 @@ private RemoveTargetEvaluation Evaluate(RemoveTarget target) return RemoveTargetResolver.Evaluate(target, _snapshot); } - private RemoveService.BatchRemoveReport Execute(RemoveTargetEvaluation evaluation) + private async Task ExecuteAsync( + RemoveTargetEvaluation evaluation, + CancellationToken cancellationToken, + IProgress progress) { if (evaluation.Target.Kind == RemoveTargetKind.AgentSymlink && evaluation.Target.AgentMembership is { } agent) { - System.IO.File.Delete(agent.Path); - _logger.Info("remove.agent", $"unlinked {agent.AgentId}: {agent.Path}"); - return new RemoveService.BatchRemoveReport( - Succeeded: true, - TargetsDeleted: 1, - FilesDeleted: 1, - DirectoriesDeleted: 0, - Errors: ImmutableArray.Empty, - DryRun: false); + var report = await _remove.RemoveLinkAsync(agent.Path, cancellationToken, progress) + .ConfigureAwait(false); + return RemoveService.BatchRemoveReport.FromSingle( + report, + targetsDeleted: report.Succeeded ? 1 : 0); } - return _remove.RemoveMany(evaluation.Items.Select(item => item.Validation)); + return await _remove.RemoveManyAsync( + evaluation.Items.Select(item => item.Validation), + cancellationToken: cancellationToken, + progress: progress).ConfigureAwait(false); + } + + private static string FormatProgress(RemoveService.RemoveProgress progress) + { + var targetCount = progress.IsCanceled + ? progress.TargetsDeleted + : progress.TargetsProcessed; + var targets = targetCount > 0 + ? $"{targetCount} target(s), " + : string.Empty; + return progress.IsCanceled + ? $" canceling… {targets}{progress.FilesProcessed} file(s), {progress.DirectoriesProcessed} dir(s)" + : $" removing… {targets}{progress.FilesProcessed} file(s), {progress.DirectoriesProcessed} dir(s) Esc cancels"; } /// Returns the index of the checkbox the is diff --git a/src/SkillView.Core/Ui/SkillViewWorkflowCoordinator.cs b/src/SkillView.Core/Ui/SkillViewWorkflowCoordinator.cs index ccf7c61..63c0ecb 100644 --- a/src/SkillView.Core/Ui/SkillViewWorkflowCoordinator.cs +++ b/src/SkillView.Core/Ui/SkillViewWorkflowCoordinator.cs @@ -269,7 +269,10 @@ public void ShowCleanupScreen() snapshot.ScannedRoots, snapshot.Skills); screen.Show(); - if (screen.RemovedCount > 0 || screen.IgnoredCount > 0) + if (screen.RemovedCount > 0 + || screen.RemovedFileCount > 0 + || screen.RemovedDirectoryCount > 0 + || screen.IgnoredCount > 0) { _services.ListAdapter.Invalidate(); // Cleanup can remove or ignore skills while the user is @@ -322,6 +325,8 @@ private void OpenRemoveDialogInternal(InstalledSkill target, InventorySnapshot s return; } + RemoveService.RemoveReport? compactAttemptReport = null; + // Compact path: a single skill with no second-confirm warnings and no // validation errors gets the winget-tui `[y] yes [n] no` confirm. // Anything more involved (package/repo group, incoming symlinks, @@ -351,29 +356,80 @@ private void OpenRemoveDialogInternal(InstalledSkill target, InventorySnapshot s } if (compactResult.Outcome == RemoveConfirmModal.Outcome.Failed) { - _setStatusWithLevel( - $"remove failed — {compactResult.Report?.Errors.FirstOrDefault() ?? "see logs (l)"}", - TuiHelpers.NotificationLevel.Error); + if (compactResult.Report is { } failedCompactReport + && HasFilesystemChanges(failedCompactReport)) + { + _services.ListAdapter.Invalidate(); + _setStatusWithLevel( + $"partially removed {failedCompactReport.FilesDeleted} file(s); " + + $"{failedCompactReport.ErrorCount} error(s) — rescanning…", + TuiHelpers.NotificationLevel.Warn); + var envReportFailed = _getLastReport(); + if (envReportFailed is not null) + { + QueueInventoryRescan( + envReportFailed, + successStatus: "partial remove — inventory now {0} skill(s)"); + } + } + else + { + _setStatusWithLevel( + $"remove failed — {compactResult.Report?.Errors.FirstOrDefault() ?? "see logs (l)"}", + TuiHelpers.NotificationLevel.Error); + } return; } if (compactResult.Outcome == RemoveConfirmModal.Outcome.Cancelled) { + if (compactResult.Report is { } canceledReport + && HasFilesystemChanges(canceledReport)) + { + _services.ListAdapter.Invalidate(); + _setStatusWithLevel( + $"remove canceled after {canceledReport.FilesDeleted} file(s) — rescanning…", + TuiHelpers.NotificationLevel.Warn); + var envReportCanceled = _getLastReport(); + if (envReportCanceled is not null) + { + QueueInventoryRescan( + envReportCanceled, + successStatus: "partial remove — inventory now {0} skill(s)"); + } + } return; } // Outcome.EscalateToWizard → fall through to the wizard below. + // Preserve any mutations made by failed compact attempts so + // closing or failing the wizard cannot suppress the rescan. + compactAttemptReport = compactResult.Report; } } var screen = new RemoveScreen(app, _services.RemoveService, _services.Logger, target, snapshot); screen.Show(); - if (screen.LastReport is { TargetsDeleted: > 0 } report) + var report = screen.LastReport; + if (compactAttemptReport is { } compactReport) + { + var compactBatch = RemoveService.BatchRemoveReport.FromSingle( + compactReport, + targetsDeleted: compactReport.Succeeded ? 1 : 0); + report = report is null + ? compactBatch + : RemovalReportState.Accumulate(compactBatch, report); + } + + if (report is { } completedReport && HasFilesystemChanges(completedReport)) { _services.ListAdapter.Invalidate(); _setStatusWithLevel( - report.Succeeded - ? $"removed {report.TargetsDeleted} skill(s) ({report.FilesDeleted} file(s)) — rescanning…" - : $"partially removed {report.TargetsDeleted} skill(s); {report.Errors.Length} error(s) — rescanning…", - report.Succeeded + completedReport.IsCanceled + ? $"remove canceled after {completedReport.FilesDeleted} file(s); " + + $"{completedReport.ErrorCount} error(s) — rescanning…" + : completedReport.Succeeded + ? $"removed {completedReport.TargetsDeleted} skill(s) ({completedReport.FilesDeleted} file(s)) — rescanning…" + : $"partially removed {completedReport.TargetsDeleted} skill(s), {completedReport.FilesDeleted} file(s); {completedReport.ErrorCount} error(s) — rescanning…", + completedReport.Succeeded && !completedReport.IsCanceled ? TuiHelpers.NotificationLevel.Success : TuiHelpers.NotificationLevel.Warn); var envReport = _getLastReport(); @@ -381,19 +437,29 @@ private void OpenRemoveDialogInternal(InstalledSkill target, InventorySnapshot s { QueueInventoryRescan( envReport, - successStatus: report.Succeeded + successStatus: completedReport.Succeeded && !completedReport.IsCanceled ? "removed — inventory now {0} skill(s)" : "partial remove — inventory now {0} skill(s)"); } } - else if (screen.LastReport is { Errors.Length: > 0 } failedReport) + else if (report is { IsCanceled: true }) + { + _setStatusWithLevel("remove canceled", TuiHelpers.NotificationLevel.Warn); + } + else if (report is { Errors.Length: > 0 } failedReport) { _setStatusWithLevel( - $"remove failed — {failedReport.Errors.Length} error(s); no files removed", + $"remove failed — {failedReport.ErrorCount} error(s); no files removed", TuiHelpers.NotificationLevel.Error); } } + private static bool HasFilesystemChanges(RemoveService.RemoveReport report) => + report.FilesDeleted > 0 || report.DirectoriesDeleted > 0; + + private static bool HasFilesystemChanges(RemoveService.BatchRemoveReport report) => + report.TargetsDeleted > 0 || report.FilesDeleted > 0 || report.DirectoriesDeleted > 0; + private void QueueInventoryRescan(EnvironmentReport report, string successStatus) { _runBackground(async cancellationToken => diff --git a/tests/SkillView.Tests/Cli/CliDispatcherJsonSnapshotTests.cs b/tests/SkillView.Tests/Cli/CliDispatcherJsonSnapshotTests.cs index 8f097b2..bf40be7 100644 --- a/tests/SkillView.Tests/Cli/CliDispatcherJsonSnapshotTests.cs +++ b/tests/SkillView.Tests/Cli/CliDispatcherJsonSnapshotTests.cs @@ -278,7 +278,7 @@ public void Remove_JsonIncludesValidationErrorsAndWarnings() ResolvedPath: "/p/a", FilesDeleted: 0, DirectoriesDeleted: 0, - Errors: ImmutableArray.Empty, + Errors: ImmutableArray.Create("ContainsGitDirectory: .git found"), DryRun: false); var p = new CliDispatcher.ParsedRemoveArgs("a", null, Yes: false, Json: true); var doc = Parse(CliDispatcher.RenderRemoveJson(report, Skill("a"), p, validation)); @@ -288,6 +288,70 @@ public void Remove_JsonIncludesValidationErrorsAndWarnings() Assert.Equal("ContainsGitDirectory", doc.GetProperty("errors")[0].GetProperty("kind").GetString()); Assert.Equal(0, doc.GetProperty("warnings").GetArrayLength()); + Assert.Equal(0, doc.GetProperty("runtimeErrorCount").GetInt32()); + Assert.Equal(0, doc.GetProperty("runtimeErrors").GetArrayLength()); + } + + [Fact] + public void Remove_JsonDoesNotMislabelUnacceptedWarningsAsRuntimeErrors() + { + var validation = new RemoveValidator.RemoveValidation( + Errors: ImmutableArray.Empty, + Warnings: ImmutableArray.Create(new RemoveValidator.Warning( + RemoveValidator.WarningKind.HasIncomingSymlinks, + "incoming link found")), + ResolvedPath: "/p/a", + IncomingSymlinkPaths: ImmutableArray.Create("/p/link")); + var report = new RemoveService.RemoveReport( + Succeeded: false, + ResolvedPath: "/p/a", + FilesDeleted: 0, + DirectoriesDeleted: 0, + Errors: ImmutableArray.Create( + "warning HasIncomingSymlinks: incoming link found (pass --yes to accept)"), + DryRun: false); + var parsed = new CliDispatcher.ParsedRemoveArgs("a", null, Yes: false, Json: true); + + var doc = Parse(CliDispatcher.RenderRemoveJson( + report, + Skill("a"), + parsed, + validation)); + + Assert.True(doc.GetProperty("allowed").GetBoolean()); + Assert.Equal(1, doc.GetProperty("warnings").GetArrayLength()); + Assert.Equal(0, doc.GetProperty("runtimeErrorCount").GetInt32()); + Assert.Equal(0, doc.GetProperty("runtimeErrors").GetArrayLength()); + } + + [Fact] + public void Remove_JsonPreservesRuntimeErrorsWhenRemovalWasAttempted() + { + var validation = new RemoveValidator.RemoveValidation( + Errors: ImmutableArray.Empty, + Warnings: ImmutableArray.Empty, + ResolvedPath: "/p/a", + IncomingSymlinkPaths: ImmutableArray.Empty); + var report = new RemoveService.RemoveReport( + Succeeded: false, + ResolvedPath: "/p/a", + FilesDeleted: 1, + DirectoriesDeleted: 0, + Errors: ImmutableArray.Create("delete failed", "… 3 additional error(s) omitted"), + DryRun: false) + { + ErrorCount = 4, + }; + var parsed = new CliDispatcher.ParsedRemoveArgs("a", null, Yes: true, Json: true); + + var doc = Parse(CliDispatcher.RenderRemoveJson( + report, + Skill("a"), + parsed, + validation)); + + Assert.Equal(4, doc.GetProperty("runtimeErrorCount").GetInt32()); + Assert.Equal(2, doc.GetProperty("runtimeErrors").GetArrayLength()); } // --- cleanup --------------------------------------------------------- diff --git a/tests/SkillView.Tests/Inventory/RemoveServiceTests.cs b/tests/SkillView.Tests/Inventory/RemoveServiceTests.cs index 17e6227..a37d127 100644 --- a/tests/SkillView.Tests/Inventory/RemoveServiceTests.cs +++ b/tests/SkillView.Tests/Inventory/RemoveServiceTests.cs @@ -101,6 +101,34 @@ public void Remove_RefusedValidation_ReturnsRefused() Assert.True(Directory.Exists(dir)); } + [Fact] + public async Task RemoveAsync_RefusedValidationPublishesTerminalProgress() + { + var (skill, dir) = MakeSkill("bad-progress"); + Directory.CreateDirectory(Path.Combine(dir, ".git")); + var validation = RemoveValidator.Validate(skill, new[] { Root() }, new[] { skill }); + Assert.False(validation.Allowed); + var updates = new List(); + var progress = new CallbackProgress(updates.Add); + + var report = await new RemoveService(_logger).RemoveAsync( + validation, + cancellationToken: TestContext.Current.CancellationToken, + progress: progress); + + Assert.False(report.Succeeded); + Assert.True(Directory.Exists(dir)); + Assert.Equal(2, updates.Count); + Assert.False(updates[0].IsCompleted); + Assert.True(updates[^1].IsCompleted); + Assert.False(updates[^1].IsCanceled); + Assert.Equal(1, updates[^1].TargetsProcessed); + Assert.Equal(0, updates[^1].TargetsDeleted); + Assert.Equal(0, updates[^1].FilesProcessed); + Assert.Equal(0, updates[^1].DirectoriesProcessed); + Assert.Equal(1, updates[^1].Errors); + } + [Fact] public void RemoveMany_HappyPath_DeletesEveryValidatedTarget() { @@ -282,6 +310,253 @@ public void Remove_CancellationBetweenDirectoryEntriesStopsEnumerationPromptly() Assert.Equal(2_001, Directory.EnumerateFiles(dir).Count()); } + [Fact] + public async Task RemoveAsync_CancellationPublishesTerminalProgress() + { + var (skill, dir) = MakeSkill("cancel-async", extraFiles: 2_000); + var validation = RemoveValidator.Validate(skill, new[] { Root() }, new[] { skill }); + using var cancellation = new CancellationTokenSource(); + var updates = new List(); + var progress = new CallbackProgress(updates.Add); + var service = new RemoveService(_logger, _ => cancellation.Cancel()); + + await Assert.ThrowsAsync(() => + service.RemoveAsync(validation, cancellationToken: cancellation.Token, progress: progress)); + + Assert.NotEmpty(updates); + Assert.True(updates[^1].IsCanceled); + Assert.False(updates[^1].IsCompleted); + Assert.True(Directory.Exists(dir)); + } + + [Fact] + public async Task RemoveAsync_ThrottlesProgressAndPublishesCompletion() + { + var (skill, dir) = MakeSkill("progress", extraFiles: 2_000); + var validation = RemoveValidator.Validate(skill, new[] { Root() }, new[] { skill }); + var updates = new List(); + var progress = new CallbackProgress(updates.Add); + + var report = await new RemoveService(_logger).RemoveAsync( + validation, + new RemoveService.Options(DryRun: true), + TestContext.Current.CancellationToken, + progress); + + Assert.True(report.Succeeded); + Assert.True(Directory.Exists(dir)); + Assert.InRange(updates.Count, 2, 999); + Assert.True(updates[^1].IsCompleted); + Assert.False(updates[^1].IsCanceled); + Assert.Equal(1, updates[^1].TargetsProcessed); + Assert.Equal(1, updates[^1].TargetsDeleted); + Assert.Equal(report.FilesDeleted, updates[^1].FilesProcessed); + Assert.Equal(report.DirectoriesDeleted, updates[^1].DirectoriesProcessed); + } + + [Fact] + public async Task RemoveManyAsync_ReportsAggregateMonotonicProgress() + { + var (first, _) = MakeSkill("progress-one", extraFiles: 2); + var (second, _) = MakeSkill("progress-two", extraFiles: 2); + var firstValidation = RemoveValidator.Validate(first, new[] { Root() }, new[] { first, second }); + var secondValidation = RemoveValidator.Validate(second, new[] { Root() }, new[] { first, second }); + var updates = new List(); + var progress = new CallbackProgress(updates.Add); + + var report = await new RemoveService(_logger).RemoveManyAsync( + [firstValidation, secondValidation], + cancellationToken: TestContext.Current.CancellationToken, + progress: progress); + + Assert.True(report.Succeeded); + Assert.NotEmpty(updates); + Assert.True(updates[^1].IsCompleted); + Assert.Equal(2, updates[^1].TargetsProcessed); + Assert.Equal(2, updates[^1].TargetsDeleted); + Assert.Equal(report.FilesDeleted, updates[^1].FilesProcessed); + Assert.Equal(report.DirectoriesDeleted, updates[^1].DirectoriesProcessed); + Assert.True(updates.Zip(updates.Skip(1), (left, right) => + left.FilesProcessed <= right.FilesProcessed + && left.DirectoriesProcessed <= right.DirectoriesProcessed + && left.TargetsProcessed <= right.TargetsProcessed + && left.TargetsDeleted <= right.TargetsDeleted).All(value => value)); + } + + [Fact] + public async Task RemoveManyAsync_CancellationBetweenTargetsPublishesExactAggregate() + { + var (first, firstDir) = MakeSkill("cancel-after-one", extraFiles: 2); + var (second, secondDir) = MakeSkill("must-remain", extraFiles: 2); + var firstValidation = RemoveValidator.Validate(first, new[] { Root() }, new[] { first, second }); + var secondValidation = RemoveValidator.Validate(second, new[] { Root() }, new[] { first, second }); + using var cancellation = new CancellationTokenSource(); + var updates = new List(); + var progress = new CallbackProgress(updates.Add); + + IEnumerable CancelBeforeSecond() + { + yield return firstValidation; + cancellation.Cancel(); + yield return secondValidation; + } + + await Assert.ThrowsAsync(() => + new RemoveService(_logger).RemoveManyAsync( + CancelBeforeSecond(), + cancellationToken: cancellation.Token, + progress: progress)); + + Assert.False(Directory.Exists(firstDir)); + Assert.True(Directory.Exists(secondDir)); + Assert.True(updates[^1].IsCanceled); + Assert.Equal(1, updates[^1].TargetsProcessed); + Assert.Equal(1, updates[^1].TargetsDeleted); + Assert.Equal(3, updates[^1].FilesProcessed); + Assert.Equal(1, updates[^1].DirectoriesProcessed); + } + + [Fact] + public async Task RemoveManyAsync_FailedTargetThenCancellationDoesNotReportTargetDeleted() + { + var refused = new RemoveValidator.RemoveValidation( + Errors: ImmutableArray.Create(new RemoveValidator.Error( + RemoveValidator.ErrorKind.NotASkillDirectory, + "not a skill")), + Warnings: ImmutableArray.Empty, + ResolvedPath: Path.Combine(_tempRoot, "refused"), + IncomingSymlinkPaths: ImmutableArray.Empty); + var (second, _) = MakeSkill("must-not-run"); + var secondValidation = RemoveValidator.Validate( + second, + new[] { Root() }, + new[] { second }); + using var cancellation = new CancellationTokenSource(); + var updates = new List(); + var progress = new CallbackProgress(updates.Add); + + IEnumerable CancelBeforeSecond() + { + yield return refused; + cancellation.Cancel(); + yield return secondValidation; + } + + await Assert.ThrowsAsync(() => + new RemoveService(_logger).RemoveManyAsync( + CancelBeforeSecond(), + cancellationToken: cancellation.Token, + progress: progress)); + + Assert.True(updates[^1].IsCanceled); + Assert.Equal(1, updates[^1].TargetsProcessed); + Assert.Equal(0, updates[^1].TargetsDeleted); + Assert.Equal(0, updates[^1].FilesProcessed); + Assert.Equal(0, updates[^1].DirectoriesProcessed); + Assert.Equal(1, updates[^1].Errors); + } + + [Fact] + public async Task RemoveManyAsync_CancellationMidTargetPreservesPartialAggregate() + { + var (skill, dir) = MakeSkill("cancel-mid-target", extraFiles: 2_000); + var validation = RemoveValidator.Validate(skill, new[] { Root() }, new[] { skill }); + using var cancellation = new CancellationTokenSource(); + var observedEntries = 0; + var updates = new List(); + var progress = new CallbackProgress(updates.Add); + var service = new RemoveService(_logger, _ => + { + if (Interlocked.Increment(ref observedEntries) == 2) + { + cancellation.Cancel(); + } + }); + + await Assert.ThrowsAsync(() => + service.RemoveManyAsync( + [validation], + cancellationToken: cancellation.Token, + progress: progress)); + + Assert.True(Directory.Exists(dir)); + Assert.True(updates[^1].IsCanceled); + Assert.Equal(0, updates[^1].TargetsProcessed); + Assert.Equal(0, updates[^1].TargetsDeleted); + Assert.Equal(1, updates[^1].FilesProcessed); + Assert.Equal(0, updates[^1].DirectoriesProcessed); + } + + [Fact] + public async Task RemoveLinkAsync_DeletesOnlyObservedLink() + { + var externalFile = Path.Combine(_tempRoot, "keep.txt"); + File.WriteAllText(externalFile, "keep"); + var link = Path.Combine(_tempRoot, "agent-link.txt"); + if (!TryCreateFileLink(link, externalFile)) return; + + var report = await new RemoveService(_logger).RemoveLinkAsync( + link, + TestContext.Current.CancellationToken); + + Assert.True(report.Succeeded, string.Join(System.Environment.NewLine, report.Errors)); + Assert.False(PathResolver.IsSymlink(link)); + Assert.True(File.Exists(externalFile)); + Assert.Equal("keep", File.ReadAllText(externalFile)); + } + + [Fact] + public async Task RemoveLinkAsync_AlreadyCanceledPublishesTerminalProgress() + { + var externalFile = Path.Combine(_tempRoot, "keep-canceled.txt"); + File.WriteAllText(externalFile, "keep"); + var link = Path.Combine(_tempRoot, "agent-link-canceled.txt"); + if (!TryCreateFileLink(link, externalFile)) return; + using var cancellation = new CancellationTokenSource(); + cancellation.Cancel(); + var updates = new List(); + var progress = new CallbackProgress(updates.Add); + + await Assert.ThrowsAsync(() => + new RemoveService(_logger).RemoveLinkAsync( + link, + cancellation.Token, + progress)); + + Assert.True(PathResolver.IsSymlink(link)); + Assert.True(File.Exists(externalFile)); + Assert.Collection( + updates, + update => + { + Assert.False(update.IsCompleted); + Assert.False(update.IsCanceled); + }, + update => + { + Assert.False(update.IsCompleted); + Assert.True(update.IsCanceled); + }); + } + + [Fact] + public void FailureCollector_RetainsBoundedDetailsAndExactCount() + { + var failures = new RemoveService.FailureCollector(); + var total = RemoveService.MaxRetainedErrors + 50; + + for (var i = 0; i < total; i++) + { + failures.Add($"failure {i}"); + } + + var retained = failures.ToImmutable(); + Assert.Equal(total, failures.Count); + Assert.Equal(RemoveService.MaxRetainedErrors + 1, retained.Length); + Assert.Equal("failure 0", retained[0]); + Assert.Contains("50 additional error(s) omitted", retained[^1]); + } + [Fact] public void Remove_WindowsDirectoryJunction_DeletesOnlyJunction() { @@ -364,4 +639,9 @@ private static ProcessStartInfo CreateJunctionStartInfo(string link, string targ info.ArgumentList.Add(target); return info; } + + private sealed class CallbackProgress(Action callback) : IProgress + { + public void Report(T value) => callback(value); + } } diff --git a/tests/SkillView.Tests/Ui/CleanupScreenTests.cs b/tests/SkillView.Tests/Ui/CleanupScreenTests.cs index 1a1b4b3..15c1bcf 100644 --- a/tests/SkillView.Tests/Ui/CleanupScreenTests.cs +++ b/tests/SkillView.Tests/Ui/CleanupScreenTests.cs @@ -1,5 +1,4 @@ using System.Collections.Immutable; -using System.Reflection; using SkillView.Inventory; using SkillView.Inventory.Models; using SkillView.Logging; @@ -12,7 +11,7 @@ namespace SkillView.Tests.Ui; public sealed class CleanupScreenTests { [Fact] - public void DoRemove_RemovesEmptyDirectoryCandidateInsideScanRoot() + public async Task RemoveSelectedAsync_RemovesEmptyDirectoryCandidateInsideScanRoot() { var root = Path.Combine(Path.GetTempPath(), "skillview-cleanup-ui-" + Guid.NewGuid().ToString("N")); Directory.CreateDirectory(root); @@ -27,13 +26,14 @@ public void DoRemove_RemovesEmptyDirectoryCandidateInsideScanRoot() "empty directory under scan root", Skill: null); var screen = CreateScreen([candidate], [new ScanRoot(root, Scope.User, null)]); - var status = new Label(); - - InvokeDoRemove(screen, [0], status); + var summary = await screen.RemoveSelectedAsync( + [0], TestContext.Current.CancellationToken); Assert.False(Directory.Exists(emptyDir)); Assert.Equal(1, screen.RemovedCount); - Assert.Equal(" removed 1, skipped/failed 0", status.Text.ToString()); + Assert.Equal(0, screen.RemovedFileCount); + Assert.Equal(1, screen.RemovedDirectoryCount); + Assert.Equal(new CleanupScreen.RemovalSummary(1, 0, Confirmed: true), summary); } finally { @@ -42,7 +42,7 @@ public void DoRemove_RemovesEmptyDirectoryCandidateInsideScanRoot() } [Fact] - public void DoRemove_RemovesBrokenSymlinkCandidateInsideScanRoot() + public async Task RemoveSelectedAsync_RemovesBrokenSymlinkCandidateInsideScanRoot() { var root = Path.Combine(Path.GetTempPath(), "skillview-cleanup-ui-" + Guid.NewGuid().ToString("N")); Directory.CreateDirectory(root); @@ -58,13 +58,14 @@ public void DoRemove_RemovesBrokenSymlinkCandidateInsideScanRoot() "broken symlink", Skill: null); var screen = CreateScreen([candidate], [new ScanRoot(root, Scope.User, null)]); - var status = new Label(); - - InvokeDoRemove(screen, [0], status); + var summary = await screen.RemoveSelectedAsync( + [0], TestContext.Current.CancellationToken); Assert.False(File.Exists(brokenLink) || Directory.Exists(brokenLink) || PathResolver.IsSymlink(brokenLink)); Assert.Equal(1, screen.RemovedCount); - Assert.Equal(" removed 1, skipped/failed 0", status.Text.ToString()); + Assert.Equal(1, screen.RemovedFileCount); + Assert.Equal(0, screen.RemovedDirectoryCount); + Assert.Equal(new CleanupScreen.RemovalSummary(1, 0, Confirmed: true), summary); } finally { @@ -73,7 +74,7 @@ public void DoRemove_RemovesBrokenSymlinkCandidateInsideScanRoot() } [Fact] - public void DoRemove_RemovesBrokenSymlinkCandidateWhenCandidateCarriesSkill() + public async Task RemoveSelectedAsync_RemovesBrokenSymlinkCandidateWhenCandidateCarriesSkill() { var root = Path.Combine(Path.GetTempPath(), "skillview-cleanup-ui-" + Guid.NewGuid().ToString("N")); Directory.CreateDirectory(root); @@ -93,13 +94,12 @@ public void DoRemove_RemovesBrokenSymlinkCandidateWhenCandidateCarriesSkill() "broken symlink", Skill: skill); var screen = CreateScreen([candidate], [new ScanRoot(root, Scope.User, null)], [skill]); - var status = new Label(); - - InvokeDoRemove(screen, [0], status); + var summary = await screen.RemoveSelectedAsync( + [0], TestContext.Current.CancellationToken); Assert.False(File.Exists(brokenLink) || Directory.Exists(brokenLink) || PathResolver.IsSymlink(brokenLink)); Assert.Equal(1, screen.RemovedCount); - Assert.Equal(" removed 1, skipped/failed 0", status.Text.ToString()); + Assert.Equal(new CleanupScreen.RemovalSummary(1, 0, Confirmed: true), summary); } finally { @@ -108,7 +108,7 @@ public void DoRemove_RemovesBrokenSymlinkCandidateWhenCandidateCarriesSkill() } [Fact] - public void DoRemove_SkipsEmptyDirectoryCandidateWhenDirectoryBecomesNonEmpty() + public async Task RemoveSelectedAsync_SkipsEmptyDirectoryCandidateWhenDirectoryBecomesNonEmpty() { var root = Path.Combine(Path.GetTempPath(), "skillview-cleanup-ui-" + Guid.NewGuid().ToString("N")); Directory.CreateDirectory(root); @@ -124,13 +124,12 @@ public void DoRemove_SkipsEmptyDirectoryCandidateWhenDirectoryBecomesNonEmpty() "empty directory under scan root", Skill: null); var screen = CreateScreen([candidate], [new ScanRoot(root, Scope.User, null)]); - var status = new Label(); - - InvokeDoRemove(screen, [0], status); + var summary = await screen.RemoveSelectedAsync( + [0], TestContext.Current.CancellationToken); Assert.True(Directory.Exists(emptyDir)); Assert.Equal(0, screen.RemovedCount); - Assert.Equal(" removed 0, skipped/failed 1", status.Text.ToString()); + Assert.Equal(new CleanupScreen.RemovalSummary(0, 1, Confirmed: true), summary); } finally { @@ -139,7 +138,7 @@ public void DoRemove_SkipsEmptyDirectoryCandidateWhenDirectoryBecomesNonEmpty() } [Fact] - public void DoRemove_SkipsBrokenSymlinkCandidateWhenPathBecomesDirectory() + public async Task RemoveSelectedAsync_SkipsBrokenSymlinkCandidateWhenPathBecomesDirectory() { var root = Path.Combine(Path.GetTempPath(), "skillview-cleanup-ui-" + Guid.NewGuid().ToString("N")); Directory.CreateDirectory(root); @@ -162,13 +161,12 @@ public void DoRemove_SkipsBrokenSymlinkCandidateWhenPathBecomesDirectory() "broken symlink", Skill: skill); var screen = CreateScreen([candidate], [new ScanRoot(root, Scope.User, null)], [skill]); - var status = new Label(); - - InvokeDoRemove(screen, [0], status); + var summary = await screen.RemoveSelectedAsync( + [0], TestContext.Current.CancellationToken); Assert.True(Directory.Exists(brokenLink)); Assert.Equal(0, screen.RemovedCount); - Assert.Equal(" removed 0, skipped/failed 1", status.Text.ToString()); + Assert.Equal(new CleanupScreen.RemovalSummary(0, 1, Confirmed: true), summary); } finally { @@ -177,7 +175,7 @@ public void DoRemove_SkipsBrokenSymlinkCandidateWhenPathBecomesDirectory() } [Fact] - public void DoRemove_SkipsEmptyDirectoryCandidateWhenPathBecomesSymlink() + public async Task RemoveSelectedAsync_SkipsEmptyDirectoryCandidateWhenPathBecomesSymlink() { var root = Path.Combine(Path.GetTempPath(), "skillview-cleanup-ui-" + Guid.NewGuid().ToString("N")); Directory.CreateDirectory(root); @@ -196,13 +194,12 @@ public void DoRemove_SkipsEmptyDirectoryCandidateWhenPathBecomesSymlink() "empty directory under scan root", Skill: null); var screen = CreateScreen([candidate], [new ScanRoot(root, Scope.User, null)]); - var status = new Label(); - - InvokeDoRemove(screen, [0], status); + var summary = await screen.RemoveSelectedAsync( + [0], TestContext.Current.CancellationToken); Assert.True(PathResolver.IsSymlink(target)); Assert.Equal(0, screen.RemovedCount); - Assert.Equal(" removed 0, skipped/failed 1", status.Text.ToString()); + Assert.Equal(new CleanupScreen.RemovalSummary(0, 1, Confirmed: true), summary); } finally { @@ -351,13 +348,6 @@ private static CleanupScreen CreateScreen( allSkills: allSkills ?? Array.Empty(), confirmBatchRemoval: _ => 1); - private static void InvokeDoRemove(CleanupScreen screen, IEnumerable checkedRows, Label status) - { - var method = typeof(CleanupScreen).GetMethod("DoRemove", BindingFlags.Instance | BindingFlags.NonPublic); - Assert.NotNull(method); - method!.Invoke(screen, [new HashSet(checkedRows), status]); - } - private static void DeletePath(string path) { try diff --git a/tests/SkillView.Tests/Ui/RemovalReportStateTests.cs b/tests/SkillView.Tests/Ui/RemovalReportStateTests.cs new file mode 100644 index 0000000..8300b1c --- /dev/null +++ b/tests/SkillView.Tests/Ui/RemovalReportStateTests.cs @@ -0,0 +1,175 @@ +using System.Collections.Immutable; +using SkillView.Inventory; +using SkillView.Ui; +using Xunit; + +namespace SkillView.Tests.Ui; + +public sealed class RemovalReportStateTests +{ + [Fact] + public void AccumulateSingle_PreservesEarlierMutationsAcrossRetry() + { + var previous = Single( + succeeded: false, + files: 3, + directories: 1, + errors: ["first failure"], + errorCount: 1); + var current = Single( + succeeded: false, + files: 0, + directories: 0, + errors: ["retry failure"], + errorCount: 1); + + var accumulated = RemovalReportState.Accumulate(previous, current); + + Assert.False(accumulated.Succeeded); + Assert.Equal(3, accumulated.FilesDeleted); + Assert.Equal(1, accumulated.DirectoriesDeleted); + Assert.Equal(1, accumulated.ErrorCount); + Assert.Equal(["retry failure"], accumulated.Errors); + } + + [Fact] + public void AccumulateBatch_PreservesCompactMutationsAcrossWizardResult() + { + var compact = Batch( + succeeded: false, + targets: 0, + files: 2, + directories: 0, + errors: ["compact failure"], + errorCount: 1); + var wizard = Batch( + succeeded: false, + targets: 0, + files: 0, + directories: 0, + errors: ["wizard failure"], + errorCount: 1); + + var accumulated = RemovalReportState.Accumulate(compact, wizard); + + Assert.False(accumulated.Succeeded); + Assert.Equal(0, accumulated.TargetsDeleted); + Assert.Equal(2, accumulated.FilesDeleted); + Assert.Equal(0, accumulated.DirectoriesDeleted); + Assert.Equal(["wizard failure"], accumulated.Errors); + } + + [Fact] + public void Accumulate_UsesLatestOutcomeWhileRetainingMutationTotals() + { + var previous = Single( + succeeded: false, + files: 2, + directories: 0, + errors: ["transient failure"], + errorCount: 1); + var current = Single( + succeeded: true, + files: 1, + directories: 1, + errors: [], + errorCount: 0); + + var accumulated = RemovalReportState.Accumulate(previous, current); + + Assert.True(accumulated.Succeeded); + Assert.Equal(3, accumulated.FilesDeleted); + Assert.Equal(1, accumulated.DirectoriesDeleted); + Assert.Equal(0, accumulated.ErrorCount); + Assert.Empty(accumulated.Errors); + } + + [Fact] + public void CanceledSingle_PreservesExactObservedRuntimeErrorCount() + { + var progress = Progress( + targetsProcessed: 1, + targetsDeleted: 0, + files: 4, + directories: 1, + errors: 7); + + var report = RemovalReportState.Canceled("/skills/demo", progress); + + Assert.True(report.IsCanceled); + Assert.False(report.Succeeded); + Assert.Equal(4, report.FilesDeleted); + Assert.Equal(1, report.DirectoriesDeleted); + Assert.Equal(7, report.ErrorCount); + Assert.Single(report.Errors); + Assert.Contains("7 runtime error detail(s)", report.Errors[0]); + } + + [Fact] + public void CanceledBatch_UsesDeletedRatherThanProcessedTargets() + { + var progress = Progress( + targetsProcessed: 3, + targetsDeleted: 1, + files: 4, + directories: 1, + errors: 2); + + var report = RemovalReportState.Canceled(progress); + + Assert.True(report.IsCanceled); + Assert.Equal(1, report.TargetsDeleted); + Assert.Equal(4, report.FilesDeleted); + Assert.Equal(1, report.DirectoriesDeleted); + Assert.Equal(2, report.ErrorCount); + Assert.Contains("2 runtime error detail(s)", report.Errors[0]); + } + + private static RemoveService.RemoveReport Single( + bool succeeded, + int files, + int directories, + ImmutableArray errors, + int errorCount) => new( + Succeeded: succeeded, + ResolvedPath: "/skills/demo", + FilesDeleted: files, + DirectoriesDeleted: directories, + Errors: errors, + DryRun: false) + { + ErrorCount = errorCount, + }; + + private static RemoveService.BatchRemoveReport Batch( + bool succeeded, + int targets, + int files, + int directories, + ImmutableArray errors, + int errorCount) => new( + Succeeded: succeeded, + TargetsDeleted: targets, + FilesDeleted: files, + DirectoriesDeleted: directories, + Errors: errors, + DryRun: false) + { + ErrorCount = errorCount, + }; + + private static RemoveService.RemoveProgress Progress( + int targetsProcessed, + int targetsDeleted, + int files, + int directories, + int errors) => new( + TargetsProcessed: targetsProcessed, + TargetsDeleted: targetsDeleted, + FilesProcessed: files, + DirectoriesProcessed: directories, + Errors: errors, + CurrentPath: "/skills/demo", + IsCompleted: false, + IsCanceled: true); +}