Skip to content

Commit b1f92b4

Browse files
committed
Address PR review issues - cancellable retry delay, async file I/O, stream leak, and race guards in Resume/Start
1 parent 9b52a42 commit b1f92b4

2 files changed

Lines changed: 63 additions & 8 deletions

File tree

StabilityMatrix.Core/Models/TrackedDownload.cs

Lines changed: 37 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,7 @@ public class TrackedDownload
7979
public Exception? Exception { get; private set; }
8080

8181
private int attempts;
82+
private CancellationTokenSource? retryDelayCancellationTokenSource;
8283

8384
#region Events
8485
public event EventHandler<ProgressReport>? ProgressUpdate;
@@ -185,6 +186,11 @@ internal void Start()
185186
$"Download state must be inactive or pending to start, not {ProgressState}"
186187
);
187188
}
189+
// Cancel any pending auto-retry delay (defensive: Start() accepts Inactive state).
190+
retryDelayCancellationTokenSource?.Cancel();
191+
retryDelayCancellationTokenSource?.Dispose();
192+
retryDelayCancellationTokenSource = null;
193+
188194
Logger.Debug("Starting download {Download}", FileName);
189195

190196
EnsureDownloadService();
@@ -202,13 +208,19 @@ internal void Start()
202208

203209
internal void Resume()
204210
{
211+
// Cancel any pending auto-retry delay since we're resuming now.
212+
retryDelayCancellationTokenSource?.Cancel();
213+
retryDelayCancellationTokenSource?.Dispose();
214+
retryDelayCancellationTokenSource = null;
215+
205216
if (ProgressState != ProgressState.Inactive && ProgressState != ProgressState.Paused)
206217
{
207218
Logger.Warn(
208219
"Attempted to resume download {Download} but it is not paused ({State})",
209220
FileName,
210221
ProgressState
211222
);
223+
return;
212224
}
213225
Logger.Debug("Resuming download {Download}", FileName);
214226

@@ -236,6 +248,11 @@ internal void Resume()
236248

237249
public void Pause()
238250
{
251+
// Cancel any pending auto-retry delay.
252+
retryDelayCancellationTokenSource?.Cancel();
253+
retryDelayCancellationTokenSource?.Dispose();
254+
retryDelayCancellationTokenSource = null;
255+
239256
if (ProgressState != ProgressState.Working)
240257
{
241258
Logger.Warn(
@@ -265,6 +282,11 @@ public void Cancel()
265282
return;
266283
}
267284

285+
// Cancel any pending auto-retry delay.
286+
retryDelayCancellationTokenSource?.Cancel();
287+
retryDelayCancellationTokenSource?.Dispose();
288+
retryDelayCancellationTokenSource = null;
289+
268290
Logger.Debug("Cancelling download {Download}", FileName);
269291

270292
// Cancel token if it exists
@@ -387,8 +409,21 @@ private void OnDownloadTaskCompleted(Task task)
387409
ProgressState = ProgressState.Inactive;
388410
OnProgressStateChanged(ProgressState.Inactive);
389411

390-
// Fire-and-forget the delayed resume to avoid blocking the task continuation thread.
391-
Task.Delay(Math.Max(delayMs, 0)).ContinueWith(_ => Resume()).SafeFireAndForget();
412+
// Clean up the completed task resources; Resume() will create new ones.
413+
downloadTask = null;
414+
downloadCancellationTokenSource = null;
415+
downloadPauseTokenSource = null;
416+
417+
// Schedule the retry with a cancellation token so Cancel/Pause can abort the delay.
418+
retryDelayCancellationTokenSource?.Dispose();
419+
retryDelayCancellationTokenSource = new CancellationTokenSource();
420+
Task.Delay(Math.Max(delayMs, 0), retryDelayCancellationTokenSource.Token)
421+
.ContinueWith(t =>
422+
{
423+
if (t.IsCompletedSuccessfully)
424+
Resume();
425+
})
426+
.SafeFireAndForget();
392427
return;
393428
}
394429

StabilityMatrix.Core/Services/TrackedDownloadService.cs

Lines changed: 26 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -138,13 +138,33 @@ public async Task TryRestartDownload(TrackedDownload download)
138138
downloadsDir.Create();
139139
var jsonFile = downloadsDir.JoinFile($"{download.Id}.json");
140140

141-
var jsonFileStream = jsonFile.Info.Open(FileMode.Create, FileAccess.ReadWrite, FileShare.Read);
142-
var json = JsonSerializer.Serialize(download);
143-
jsonFileStream.Write(Encoding.UTF8.GetBytes(json));
144-
jsonFileStream.Flush();
141+
var jsonFileStream = new FileStream(
142+
jsonFile.Info.FullName,
143+
FileMode.Create,
144+
FileAccess.ReadWrite,
145+
FileShare.Read,
146+
bufferSize: 4096,
147+
useAsync: true
148+
);
149+
var jsonBytes = JsonSerializer.SerializeToUtf8Bytes(download);
150+
151+
try
152+
{
153+
await jsonFileStream.WriteAsync(jsonBytes).ConfigureAwait(false);
154+
await jsonFileStream.FlushAsync().ConfigureAwait(false);
145155

146-
// Handlers are already attached from the original AddDownload call.
147-
downloads.TryAdd(download.Id, (download, jsonFileStream));
156+
// Handlers are already attached from the original AddDownload call.
157+
if (!downloads.TryAdd(download.Id, (download, jsonFileStream)))
158+
{
159+
// Already tracked; discard the newly opened stream.
160+
await jsonFileStream.DisposeAsync().ConfigureAwait(false);
161+
}
162+
}
163+
catch
164+
{
165+
await jsonFileStream.DisposeAsync().ConfigureAwait(false);
166+
throw;
167+
}
148168

149169
await TryResumeDownload(download).ConfigureAwait(false);
150170
}

0 commit comments

Comments
 (0)