Skip to content

Commit c3bae8f

Browse files
bezzadclaude
andcommitted
fix: release the download engine when a row reaches a terminal state (task #11)
The reported leak: a completed download's DownloadService (package + chunk buffers) was never disposed, so 2k finished rows accumulated ~6GB that only a restart cleared. FinishTerminal — the single terminal choke point — now calls ReleaseEngine(vm) on Completed/Failed/Stopped: null vm.Download first (so no late staged flush touches a disposed instance), then Dispose. Paused is not terminal and keeps its engine for Resume. Display/resume state is model-backed on the VM and survives; Resume/Retry rebuilds a fresh engine in Start exactly like a first start (engine auto-resume + the on-disk .download file continue the bytes). Tests (TDD, red first): Completed_downloads_release_their_engine (30 real loopback downloads → all rows Download==null, name + 100% kept) and A_released_stopped_row_can_be_retried_to_completion (stop → released → retry completes with a fresh engine). perf-smooth-and-memory task 2 done. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GpEAzmp4bQ5LDSW5ucuXk7
1 parent ffa0098 commit c3bae8f

3 files changed

Lines changed: 222 additions & 3 deletions

File tree

openspec/changes/perf-smooth-and-memory/tasks.md

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,6 @@ Root cause corrected during apply: the freeze is Avalonia's non-virtualized mult
1212

1313
## 2. Release memory on completion (task #11)
1414

15-
- [ ] 2.1 Write an integration test: download ~50 small files via the loopback `HttpListener`; after all Completed + `GC.Collect()/WaitForPendingFinalizers`, assert `GC.GetTotalMemory(true)` is bounded near the pre-batch baseline (fails today because engines are retained). Also assert a per-row "engine released" flag/`Download==null` after terminal state.
16-
- [ ] 2.2 In `DownloadManager` terminal handling, dispose `vm.Download` and null the retained `Download`/`Package`; ensure `DownloadItemViewModel` keeps display fields. Make 2.1 pass.
17-
- [ ] 2.3 Add/confirm tests that a released Stopped/Failed row resumes/retries correctly (fresh engine, continues from partial). Build + full tests green; commit/push; wait for green CI.
15+
- [x] 2.1 Write an integration test: download ~30 small files via the loopback `HttpListener`; after all Completed assert every row's `Download==null` (engine released) — deterministic proof (the leaked `DownloadService` holds the package+buffers) that fails today because engines are retained. (Heap thresholds are unreliable for tiny payloads in a shared parallel process; `Download==null` is the reliable proxy per design.) Display state (name, 100%) preserved.
16+
- [x] 2.2 In `DownloadManager.FinishTerminal`, `ReleaseEngine(vm)` disposes `vm.Download` and nulls it on Completed/Failed/Stopped (Package is owned by the engine → released on Dispose); Paused keeps its engine. `DownloadItemViewModel` keeps its model-backed display fields. Make 2.1 pass.
17+
- [x] 2.3 Test that a released Stopped row retries correctly (fresh engine rebuilt in `Start`, continues to completion). Build + full tests green; commit/push; wait for green CI.
Lines changed: 198 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,198 @@
1+
using System;
2+
using System.Collections.Generic;
3+
using System.Diagnostics;
4+
using System.IO;
5+
using System.Linq;
6+
using System.Net;
7+
using System.Net.Sockets;
8+
using System.Threading.Tasks;
9+
using Avalonia.Headless.XUnit;
10+
using Avalonia.Threading;
11+
using Downloader.Desktop.Models;
12+
using Downloader.Desktop.Services;
13+
using Downloader.Desktop.ViewModels;
14+
using Xunit;
15+
16+
namespace Downloader.Desktop.Tests.Integration;
17+
18+
/// <summary>
19+
/// The reported leak (task #11): a completed download's <c>DownloadService</c> (with its package + buffers)
20+
/// was never released, so thousands of finished rows accumulated GBs that only a restart cleared. These
21+
/// tests download many small files through the real manager/engine over a loopback server and assert the
22+
/// engine handle is released once a row reaches a terminal state — and that a released row can still be
23+
/// retried (a fresh engine is built).
24+
/// </summary>
25+
public class MemoryReleaseTests
26+
{
27+
[AvaloniaFact(Timeout = TestTimeouts.SlowMs)]
28+
public async Task Completed_downloads_release_their_engine()
29+
{
30+
var payload = new byte[16 * 1024];
31+
new Random(7).NextBytes(payload);
32+
using var server = new Loopback(payload);
33+
34+
var dir = Path.Combine(Path.GetTempPath(), "dldesktop_mem_" + Guid.NewGuid().ToString("N"));
35+
Directory.CreateDirectory(dir);
36+
try
37+
{
38+
var cfg = Config.New();
39+
cfg.Settings.DefaultSavePath = dir;
40+
cfg.Settings.EnableNotifications = false;
41+
cfg.Settings.ChunkCount = 2;
42+
cfg.DefaultQueue.MaxConcurrent = 4;
43+
44+
var manager = new DownloadManager();
45+
manager.Initialize(cfg);
46+
47+
const int count = 30;
48+
var vms = new List<DownloadItemViewModel>();
49+
for (var i = 0; i < count; i++)
50+
vms.Add(manager.Add(new DownloadItem { Urls = new() { server.Url + $"file{i}.bin" }, SaveFolder = dir }, autoStart: true));
51+
52+
await PumpUntil(() => vms.All(v => v.Status == DownloadStatus.Completed), (int)TestTimeouts.SlowMs - 5000);
53+
54+
Assert.All(vms, v => Assert.Equal(DownloadStatus.Completed, v.Status));
55+
// The core assertion: no completed row still holds a live engine handle (or its package).
56+
Assert.All(vms, v => Assert.Null(v.Download));
57+
58+
// Display state the grid/resume rely on must survive the release.
59+
Assert.All(vms, v =>
60+
{
61+
Assert.False(string.IsNullOrEmpty(v.FileName));
62+
Assert.Equal(100, (int)v.Progress);
63+
});
64+
}
65+
finally
66+
{
67+
try { Directory.Delete(dir, recursive: true); } catch { /* best-effort */ }
68+
}
69+
}
70+
71+
[AvaloniaFact(Timeout = TestTimeouts.SlowMs)]
72+
public async Task A_released_stopped_row_can_be_retried_to_completion()
73+
{
74+
var payload = new byte[64 * 1024];
75+
new Random(11).NextBytes(payload);
76+
using var server = new Loopback(payload);
77+
78+
var dir = Path.Combine(Path.GetTempPath(), "dldesktop_mem_retry_" + Guid.NewGuid().ToString("N"));
79+
Directory.CreateDirectory(dir);
80+
try
81+
{
82+
var cfg = Config.New();
83+
cfg.Settings.DefaultSavePath = dir;
84+
cfg.Settings.EnableNotifications = false;
85+
86+
var manager = new DownloadManager();
87+
manager.Initialize(cfg);
88+
89+
var vm = manager.Add(new DownloadItem { Urls = new() { server.Url + "retry.bin" }, SaveFolder = dir }, autoStart: true);
90+
91+
// Stop it → terminal (Stopped) → engine released.
92+
manager.Cancel(vm);
93+
await PumpUntil(() => vm.Status == DownloadStatus.Stopped && vm.Download == null, 10000);
94+
Assert.Equal(DownloadStatus.Stopped, vm.Status);
95+
Assert.Null(vm.Download); // released on stop
96+
97+
// Retry must rebuild a fresh engine and complete.
98+
manager.Retry(vm);
99+
await PumpUntil(() => vm.Status == DownloadStatus.Completed, 20000);
100+
Assert.Equal(DownloadStatus.Completed, vm.Status);
101+
Assert.Null(vm.Download); // released again after completing
102+
}
103+
finally
104+
{
105+
try { Directory.Delete(dir, recursive: true); } catch { /* best-effort */ }
106+
}
107+
}
108+
109+
private static async Task PumpUntil(Func<bool> condition, int timeoutMs)
110+
{
111+
var sw = Stopwatch.StartNew();
112+
while (!condition() && sw.ElapsedMilliseconds < timeoutMs)
113+
{
114+
Dispatcher.UIThread.RunJobs();
115+
await Task.Delay(25);
116+
}
117+
Dispatcher.UIThread.RunJobs();
118+
}
119+
120+
/// <summary>Minimal loopback HTTP server with Range support (same shape as IntegrationTests).</summary>
121+
private sealed class Loopback : IDisposable
122+
{
123+
private readonly HttpListener _listener;
124+
private readonly byte[] _data;
125+
public string Url { get; }
126+
127+
public Loopback(byte[] data)
128+
{
129+
_data = data;
130+
var l = new TcpListener(IPAddress.Loopback, 0);
131+
l.Start();
132+
var port = ((IPEndPoint)l.LocalEndpoint).Port;
133+
l.Stop();
134+
Url = $"http://127.0.0.1:{port}/";
135+
_listener = new HttpListener();
136+
_listener.Prefixes.Add(Url);
137+
_listener.Start();
138+
_ = Task.Run(LoopAsync);
139+
}
140+
141+
private async Task LoopAsync()
142+
{
143+
while (_listener.IsListening)
144+
{
145+
HttpListenerContext ctx;
146+
try { ctx = await _listener.GetContextAsync(); }
147+
catch { break; }
148+
_ = Task.Run(() => Handle(ctx));
149+
}
150+
}
151+
152+
private void Handle(HttpListenerContext ctx)
153+
{
154+
try
155+
{
156+
var resp = ctx.Response;
157+
resp.Headers["Accept-Ranges"] = "bytes";
158+
if (ctx.Request.HttpMethod == "HEAD")
159+
{
160+
resp.ContentLength64 = _data.Length;
161+
resp.OutputStream.Close();
162+
return;
163+
}
164+
165+
var range = ctx.Request.Headers["Range"];
166+
int start = 0, end = _data.Length - 1;
167+
if (!string.IsNullOrEmpty(range) && range.StartsWith("bytes=", StringComparison.OrdinalIgnoreCase))
168+
{
169+
var parts = range.Substring(6).Split('-');
170+
if (parts.Length == 2)
171+
{
172+
if (int.TryParse(parts[0], out var s)) start = s;
173+
if (int.TryParse(parts[1], out var e)) end = e;
174+
}
175+
end = Math.Min(end, _data.Length - 1);
176+
start = Math.Max(0, Math.Min(start, end));
177+
resp.StatusCode = 206;
178+
resp.AddHeader("Content-Range", $"bytes {start}-{end}/{_data.Length}");
179+
}
180+
181+
var len = end - start + 1;
182+
resp.ContentLength64 = len;
183+
resp.OutputStream.Write(_data, start, len);
184+
resp.OutputStream.Close();
185+
}
186+
catch
187+
{
188+
try { ctx.Response.Abort(); } catch { /* ignore */ }
189+
}
190+
}
191+
192+
public void Dispose()
193+
{
194+
try { _listener.Stop(); } catch { /* ignore */ }
195+
try { _listener.Close(); } catch { /* ignore */ }
196+
}
197+
}
198+
}

src/Downloader.Desktop/Services/DownloadManager.cs

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1133,12 +1133,33 @@ private bool IsExpiredOrInvalidLink(DownloadItemViewModel vm, System.ComponentMo
11331133
/// </summary>
11341134
private void FinishTerminal(DownloadItemViewModel vm)
11351135
{
1136+
// Release the engine as soon as the row reaches an end state (Completed/Failed/Stopped) — this is
1137+
// the fix for the reported leak (#11): thousands of finished rows each kept their DownloadService
1138+
// (package + chunk buffers) alive, so memory climbed to GBs and only a restart cleared it. Paused
1139+
// is NOT terminal, so its engine is kept for Resume (and the engine's Pause() never fires this).
1140+
if (vm.Status is DownloadStatus.Completed or DownloadStatus.Failed or DownloadStatus.Stopped)
1141+
ReleaseEngine(vm);
1142+
11361143
TryStartNextInQueue(vm.GetItem().QueueId);
11371144
if (vm.Status == DownloadStatus.Completed)
11381145
MaybeAllCompleted();
11391146
NotifyList();
11401147
}
11411148

1149+
/// <summary>Dispose a finished row's engine so its package + buffers can be garbage-collected. The row's
1150+
/// display/resume state (name, size, downloaded, progress, status, folder, urls) is model-backed on the
1151+
/// VM and survives; a later Resume/Retry rebuilds a fresh DownloadService in <see cref="Start"/> exactly
1152+
/// like a first start (engine auto-resume + the on-disk .download file continue the bytes).</summary>
1153+
private static void ReleaseEngine(DownloadItemViewModel vm)
1154+
{
1155+
var engine = vm.Download;
1156+
if (engine == null)
1157+
return;
1158+
vm.Download = null; // drop the reference first so no late staged flush touches a disposed instance
1159+
try { engine.Dispose(); }
1160+
catch { /* best-effort — releasing memory must never surface an error to the user */ }
1161+
}
1162+
11421163
/// <summary>If the resolving plugin offers an action for this completed item (e.g. "Add to Ollama"),
11431164
/// surface it as an actionable notification. The row button appears via <see cref="PostDownloadActionLabel"/>.</summary>
11441165
private void OfferPostDownloadAction(DownloadItemViewModel vm)

0 commit comments

Comments
 (0)