Skip to content

Commit 790af45

Browse files
committed
fix: address review feedback.
- Fix Resume() ignoring Pending state, causing queued downloads to stall after ProcessPendingDownloads() enqueues them (P1) - Preserve sidecar files (.cm-info.json, preview image) on download failure so manual retry can succeed; only delete sidecars on explicit cancel or dismiss (P2) - Add Dismiss action to TrackedDownload, ownloadProgressItemViewModel, and ProgressManagerPage so users can clean up failed downloads without retrying, orphaning sidecars in the process - Fix KeyNotFoundException crash in UpdateJsonForDownload when Dismiss fires a state-change event after the download was already removed from the tracking dictionary on failure - Add unit tests: Resume_WhileInPendingState_SetsStateToWorking, OnFailed_SidecarFilesPreservedForRetry, OnCancelled_SidecarFilesAreDeleted
1 parent 4d46aa4 commit 790af45

6 files changed

Lines changed: 274 additions & 7 deletions

File tree

StabilityMatrix.Avalonia/ViewModels/Base/PausableProgressItemViewModelBase.cs

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,8 @@ public abstract partial class PausableProgressItemViewModelBase : ProgressItemVi
1515
nameof(IsCompleted),
1616
nameof(CanPauseResume),
1717
nameof(CanCancel),
18-
nameof(CanRetry)
18+
nameof(CanRetry),
19+
nameof(CanDismiss)
1920
)]
2021
private ProgressState state = ProgressState.Inactive;
2122

@@ -40,6 +41,12 @@ public abstract partial class PausableProgressItemViewModelBase : ProgressItemVi
4041
/// </summary>
4142
public virtual bool SupportsRetry => false;
4243

44+
/// <summary>
45+
/// Override to true in subclasses that support dismissing a failed item,
46+
/// which runs full sidecar cleanup before removing the entry.
47+
/// </summary>
48+
public virtual bool SupportsDismiss => false;
49+
4350
public bool CanPauseResume => SupportsPauseResume && !IsCompleted && !IsPending;
4451
public bool CanCancel => SupportsCancel && !IsCompleted;
4552

@@ -48,6 +55,11 @@ public abstract partial class PausableProgressItemViewModelBase : ProgressItemVi
4855
/// </summary>
4956
public bool CanRetry => SupportsRetry && State == ProgressState.Failed;
5057

58+
/// <summary>
59+
/// True only when this item supports dismiss AND is in the Failed state.
60+
/// </summary>
61+
public bool CanDismiss => SupportsDismiss && State == ProgressState.Failed;
62+
5163
private AsyncRelayCommand? pauseCommand;
5264
public IAsyncRelayCommand PauseCommand => pauseCommand ??= new AsyncRelayCommand(Pause);
5365

@@ -68,6 +80,11 @@ public abstract partial class PausableProgressItemViewModelBase : ProgressItemVi
6880

6981
public virtual Task Retry() => Task.CompletedTask;
7082

83+
private AsyncRelayCommand? dismissCommand;
84+
public IAsyncRelayCommand DismissCommand => dismissCommand ??= new AsyncRelayCommand(Dismiss);
85+
86+
public virtual Task Dismiss() => Task.CompletedTask;
87+
7188
[RelayCommand]
7289
private Task TogglePauseResume()
7390
{

StabilityMatrix.Avalonia/ViewModels/Progress/DownloadProgressItemViewModel.cs

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,12 @@ private void OnProgressStateChanged(ProgressState state)
7676
/// </summary>
7777
public override bool SupportsRetry => true;
7878

79+
/// <summary>
80+
/// Downloads support dismiss, which cleans up all sidecar files when
81+
/// the user discards a failed download without retrying.
82+
/// </summary>
83+
public override bool SupportsDismiss => true;
84+
7985
/// <inheritdoc />
8086
public override Task Cancel()
8187
{
@@ -106,4 +112,13 @@ public override Task Retry()
106112
download.ResetAttempts();
107113
return downloadService.TryRestartDownload(download);
108114
}
115+
116+
/// <inheritdoc />
117+
/// Runs full cleanup (temp file + sidecar files) for a failed download the user
118+
/// chooses not to retry, then transitions to Cancelled so the service removes it.
119+
public override Task Dismiss()
120+
{
121+
download.Dismiss();
122+
return Task.CompletedTask;
123+
}
109124
}

StabilityMatrix.Avalonia/Views/ProgressManagerPage.axaml

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,15 @@
122122
ToolTip.Tip="Retry download">
123123
<ui:SymbolIcon Symbol="Refresh" />
124124
</Button>
125+
126+
<!-- Dismiss button: cleans up sidecar files for failed downloads that won't be retried -->
127+
<Button
128+
Classes="transparent-full"
129+
Command="{Binding DismissCommand}"
130+
IsVisible="{Binding CanDismiss}"
131+
ToolTip.Tip="Dismiss and clean up">
132+
<ui:SymbolIcon Symbol="Delete" />
133+
</Button>
125134
</StackPanel>
126135

127136
<ProgressBar

StabilityMatrix.Core/Models/TrackedDownload.cs

Lines changed: 48 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -217,7 +217,11 @@ internal void Resume()
217217
// Cancel any pending auto-retry delay since we're resuming now.
218218
CancelRetryDelay();
219219

220-
if (ProgressState != ProgressState.Inactive && ProgressState != ProgressState.Paused)
220+
if (
221+
ProgressState != ProgressState.Inactive
222+
&& ProgressState != ProgressState.Paused
223+
&& ProgressState != ProgressState.Pending
224+
)
221225
{
222226
Logger.Warn(
223227
"Attempted to resume download {Download} but it is not paused ({State})",
@@ -272,6 +276,33 @@ public void Pause()
272276
OnProgressStateChanged(ProgressState);
273277
}
274278

279+
/// <summary>
280+
/// Cleans up temp file and all sidecar files (e.g. .cm-info.json, preview image)
281+
/// for a download that has already failed and will not be retried.
282+
/// This transitions the state to <see cref="ProgressState.Cancelled"/> so the
283+
/// service removes the tracking entry.
284+
/// </summary>
285+
public void Dismiss()
286+
{
287+
if (ProgressState != ProgressState.Failed)
288+
{
289+
Logger.Warn(
290+
"Attempted to dismiss download {Download} but it is not in a failed state ({State})",
291+
FileName,
292+
ProgressState
293+
);
294+
return;
295+
}
296+
297+
Logger.Debug("Dismissing failed download {Download}", FileName);
298+
299+
DoCleanup();
300+
301+
OnProgressStateChanging(ProgressState.Cancelled);
302+
ProgressState = ProgressState.Cancelled;
303+
OnProgressStateChanged(ProgressState);
304+
}
305+
275306
public void Cancel()
276307
{
277308
if (ProgressState is not (ProgressState.Working or ProgressState.Inactive))
@@ -313,9 +344,12 @@ public void SetPending()
313344
}
314345

315346
/// <summary>
316-
/// Deletes the temp file and any extra cleanup files
347+
/// Deletes the temp file and, optionally, any extra cleanup files (e.g. sidecar metadata).
348+
/// Pass <paramref name="includeExtraCleanupFiles"/> as <c>false</c> when the download
349+
/// failed but may be retried — sidecar files (.cm-info.json, preview image) should survive
350+
/// so a manual retry doesn't need to recreate them.
317351
/// </summary>
318-
private void DoCleanup()
352+
private void DoCleanup(bool includeExtraCleanupFiles = true)
319353
{
320354
try
321355
{
@@ -326,6 +360,9 @@ private void DoCleanup()
326360
Logger.Warn("Failed to delete temp file {TempFile}", TempFileName);
327361
}
328362

363+
if (!includeExtraCleanupFiles)
364+
return;
365+
329366
foreach (var extraFile in ExtraCleanupFileNames)
330367
{
331368
try
@@ -440,11 +477,17 @@ private void OnDownloadTaskCompleted(Task task)
440477
ProgressState = ProgressState.Success;
441478
}
442479

443-
// For failed or cancelled, delete the temp files
444-
if (ProgressState is ProgressState.Failed or ProgressState.Cancelled)
480+
// For cancelled, delete the temp file and any sidecar metadata.
481+
// For failed, only delete the temp file — sidecar files (.cm-info.json, preview image)
482+
// are preserved so a manual retry doesn't need to recreate them.
483+
if (ProgressState is ProgressState.Cancelled)
445484
{
446485
DoCleanup();
447486
}
487+
else if (ProgressState is ProgressState.Failed)
488+
{
489+
DoCleanup(includeExtraCleanupFiles: false);
490+
}
448491
// For pause, just do nothing
449492

450493
OnProgressStateChanged(ProgressState);

StabilityMatrix.Core/Services/TrackedDownloadService.cs

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -245,12 +245,17 @@ private void AdjustSemaphoreCount()
245245
/// </summary>
246246
private void UpdateJsonForDownload(TrackedDownload download)
247247
{
248+
// The download may have already been removed from the dictionary (e.g. it failed and was
249+
// then dismissed). In that case there is nothing to persist, so skip silently.
250+
if (!downloads.TryGetValue(download.Id, out var downloadInfo))
251+
return;
252+
248253
// Serialize to json
249254
var json = JsonSerializer.Serialize(download);
250255
var jsonBytes = Encoding.UTF8.GetBytes(json);
251256

252257
// Write to file
253-
var (_, fs) = downloads[download.Id];
258+
var (_, fs) = downloadInfo;
254259
fs.Seek(0, SeekOrigin.Begin);
255260
fs.Write(jsonBytes);
256261
fs.Flush();
Lines changed: 178 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,178 @@
1+
using System;
2+
using System.IO;
3+
using System.Threading;
4+
using System.Threading.Tasks;
5+
using NSubstitute;
6+
using StabilityMatrix.Core.Models;
7+
using StabilityMatrix.Core.Models.FileInterfaces;
8+
using StabilityMatrix.Core.Models.Progress;
9+
using StabilityMatrix.Core.Services;
10+
11+
namespace StabilityMatrix.Tests.Models;
12+
13+
[TestClass]
14+
public class TrackedDownloadTests
15+
{
16+
private string tempDir = null!;
17+
18+
[TestInitialize]
19+
public void Setup()
20+
{
21+
tempDir = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName());
22+
Directory.CreateDirectory(tempDir);
23+
}
24+
25+
[TestCleanup]
26+
public void Cleanup()
27+
{
28+
if (Directory.Exists(tempDir))
29+
Directory.Delete(tempDir, recursive: true);
30+
}
31+
32+
private TrackedDownload CreateDownload(IDownloadService downloadService)
33+
{
34+
var download = new TrackedDownload
35+
{
36+
Id = Guid.NewGuid(),
37+
SourceUrl = new Uri("https://example.com/model.safetensors"),
38+
DownloadDirectory = new DirectoryPath(tempDir),
39+
FileName = "model.safetensors",
40+
TempFileName = "model.safetensors.partial",
41+
};
42+
download.SetDownloadService(downloadService);
43+
return download;
44+
}
45+
46+
// Resume() must proceed when the state is Pending (queued resume fix).
47+
48+
[TestMethod]
49+
public async Task Resume_WhileInPendingState_SetsStateToWorking()
50+
{
51+
// Arrange – download service that blocks forever until cancelled.
52+
var downloadService = Substitute.For<IDownloadService>();
53+
downloadService
54+
.ResumeDownloadToFileAsync(
55+
Arg.Any<string>(),
56+
Arg.Any<string>(),
57+
Arg.Any<long>(),
58+
Arg.Any<IProgress<ProgressReport>>(),
59+
Arg.Any<string?>(),
60+
Arg.Any<CancellationToken>()
61+
)
62+
.Returns(callInfo =>
63+
{
64+
var ct = callInfo.Arg<CancellationToken>();
65+
return Task.Delay(Timeout.Infinite, ct);
66+
});
67+
68+
var download = CreateDownload(downloadService);
69+
download.SetPending(); // Simulate being queued
70+
71+
// Act
72+
download.Resume();
73+
74+
// Assert – Resume() must not have returned early; state is now Working.
75+
Assert.AreEqual(ProgressState.Working, download.ProgressState);
76+
77+
// Cleanup – cancel and wait for the task to finish.
78+
var cancelledTcs = new TaskCompletionSource();
79+
download.ProgressStateChanged += (_, state) =>
80+
{
81+
if (state == ProgressState.Cancelled)
82+
cancelledTcs.TrySetResult();
83+
};
84+
download.Cancel();
85+
await cancelledTcs.Task.WaitAsync(TimeSpan.FromSeconds(5));
86+
}
87+
88+
// Sidecar files (.cm-info.json, preview image) must survive a failed
89+
// download so a manual retry can succeed without recreating them.
90+
91+
[TestMethod]
92+
public async Task OnFailed_SidecarFilesPreservedForRetry()
93+
{
94+
// Arrange – create sidecar files that ModelImportService would have written.
95+
var sidecarPath = Path.Combine(tempDir, "model.cm-info.json");
96+
var previewPath = Path.Combine(tempDir, "model.preview.png");
97+
await File.WriteAllTextAsync(sidecarPath, "{}");
98+
await File.WriteAllTextAsync(previewPath, "PNG");
99+
100+
// Download service that immediately throws a non-transient error
101+
// (InvalidOperationException is not an IOException/AuthenticationException,
102+
// so it goes straight to Failed without any auto-retry attempts).
103+
var downloadService = Substitute.For<IDownloadService>();
104+
downloadService
105+
.ResumeDownloadToFileAsync(
106+
Arg.Any<string>(),
107+
Arg.Any<string>(),
108+
Arg.Any<long>(),
109+
Arg.Any<IProgress<ProgressReport>>(),
110+
Arg.Any<string?>(),
111+
Arg.Any<CancellationToken>()
112+
)
113+
.Returns(Task.FromException(new InvalidOperationException("Simulated download error")));
114+
115+
var download = CreateDownload(downloadService);
116+
download.ExtraCleanupFileNames.Add(sidecarPath);
117+
download.ExtraCleanupFileNames.Add(previewPath);
118+
119+
var failedTcs = new TaskCompletionSource();
120+
download.ProgressStateChanged += (_, state) =>
121+
{
122+
if (state == ProgressState.Failed)
123+
failedTcs.TrySetResult();
124+
};
125+
126+
// Act
127+
download.Start();
128+
await failedTcs.Task.WaitAsync(TimeSpan.FromSeconds(5));
129+
130+
// Assert – sidecar files must still exist for a potential manual retry.
131+
Assert.IsTrue(File.Exists(sidecarPath), ".cm-info.json should be preserved after failure");
132+
Assert.IsTrue(File.Exists(previewPath), "Preview image should be preserved after failure");
133+
}
134+
135+
[TestMethod]
136+
public async Task OnCancelled_SidecarFilesAreDeleted()
137+
{
138+
// Sidecar files should still be cleaned up when the user explicitly cancels.
139+
var sidecarPath = Path.Combine(tempDir, "model.cm-info.json");
140+
var previewPath = Path.Combine(tempDir, "model.preview.png");
141+
await File.WriteAllTextAsync(sidecarPath, "{}");
142+
await File.WriteAllTextAsync(previewPath, "PNG");
143+
144+
var downloadService = Substitute.For<IDownloadService>();
145+
downloadService
146+
.ResumeDownloadToFileAsync(
147+
Arg.Any<string>(),
148+
Arg.Any<string>(),
149+
Arg.Any<long>(),
150+
Arg.Any<IProgress<ProgressReport>>(),
151+
Arg.Any<string?>(),
152+
Arg.Any<CancellationToken>()
153+
)
154+
.Returns(callInfo =>
155+
{
156+
var ct = callInfo.Arg<CancellationToken>();
157+
return Task.Delay(Timeout.Infinite, ct);
158+
});
159+
160+
var download = CreateDownload(downloadService);
161+
download.ExtraCleanupFileNames.Add(sidecarPath);
162+
download.ExtraCleanupFileNames.Add(previewPath);
163+
164+
var cancelledTcs = new TaskCompletionSource();
165+
download.ProgressStateChanged += (_, state) =>
166+
{
167+
if (state == ProgressState.Cancelled)
168+
cancelledTcs.TrySetResult();
169+
};
170+
171+
download.Start();
172+
download.Cancel();
173+
await cancelledTcs.Task.WaitAsync(TimeSpan.FromSeconds(5));
174+
175+
Assert.IsFalse(File.Exists(sidecarPath), ".cm-info.json should be deleted on cancel");
176+
Assert.IsFalse(File.Exists(previewPath), "Preview image should be deleted on cancel");
177+
}
178+
}

0 commit comments

Comments
 (0)