fix(downloader): keep delayed cleanup alive after download - #16151

Merge Risk: 🟡 Moderate · up to acd66

The fix keeps cleanup alive after the download runtime exits, but one sleeping OS thread is created per completed download. High download concurrency could exhaust process resources and leave stale entries, so a bounded cleanup scheduler is needed before merge.

• Fix all pre-merge checks with AI

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands.}

Thanks.

I reviewed PR #16151 (fix(downloader): keep delayed cleanup alive after download) in rustdesk/rustdesk. The PR changes src/hbbs_http/downloader.rs to avoid losing delayed cleanup when do_download()'s #[tokio::main(flavor = "current_thread")] runtime exits. The existing tokio::spawn cleanup could be cancelled when the runtime was dropped, leaving stale downloader entries in DOWNLOADERS. ([GitHub]1)

Review summary

Overall: The bug diagnosis is correct, but I would request changes before merge because the proposed fix introduces a new resource-management issue and still has a race condition.

Findings

1. 🔴 Delayed cleanup can delete a newer downloader entry

Severity: High

The PR changes cleanup to happen outside Tokio, but the cleanup operation still appears to be keyed only by URL. The sequence below can break:

1. Download A for URL X completes.
2. Cleanup task/thread is scheduled for URL X.
3. Before the delay expires, the old entry is removed and a new download B for the same URL starts.
4. Cleanup from A runs and removes B.

Result:

• Progress polling for B reports "not found".
• A valid active download is unexpectedly destroyed.

This issue was also identified in the automated review comments: the delayed cleanup should be associated with the specific downloader instance/generation, not just the URL. ([GitHub]1)

Suggested fix:

Store a unique identifier:

struct Downloader {
    id: Uuid,
    url: String,
    ...
}

Then cleanup should do:

if DOWNLOADERS
    .get(&url)
    .map(|d| d.id == expected_id)
    .unwrap_or(false)
{
    DOWNLOADERS.remove(&url);
}

This makes cleanup idempotent and prevents deleting replacements.

2. 🟠 One OS thread per completed download is risky

Severity: Medium

The new approach reportedly uses a dedicated std::thread::spawn with a sleep delay for every cleanup. ([GitHub]1)

This fixes the Tokio lifetime problem, but creates a scalability problem:

• 10,000 completed downloads → potentially 10,000 sleeping threads.
• Each thread consumes stack memory and scheduling overhead.
• Cleanup failures are harder to observe.

A better design would be:

• one background cleanup worker;
• a BinaryHeap / deadline queue;
• a channel receiving (deadline, downloader_id).

Example architecture:

download complete
        |
        v
cleanup_tx.send(RemovalTask {
    url,
    downloader_id,
    delete_at
})

cleanup worker thread
        |
        v
sleep_until(next_deadline)
        |
        v
conditional remove

This solves both the Tokio runtime issue and the per-download thread issue.

3. 🟡 Missing failure handling when scheduling cleanup fails

Severity: Medium

If cleanup scheduling fails (channel closed, thread creation failure, etc.), the downloader entry can remain forever.

The PR should define ownership:

• Who removes completed downloaders?
• What happens if delayed cleanup cannot be scheduled?
• Is there a maximum lifetime fallback?

At minimum, log the failure and remove synchronously:

if schedule_cleanup(...).is_err() {
    DOWNLOADERS.remove(&url);
}

4. 🟢 Test coverage should include replacement race

The added regression test covers the original bug: cleanup surviving Tokio runtime shutdown. ([GitHub]1)

A second test is needed:

insert downloader A
schedule cleanup(A)

remove A
insert downloader B with same URL

wait cleanup delay

assert downloader B still exists

Without this, the most dangerous regression remains untested.

Recommendation

Request changes

The PR fixes the original lifecycle bug, but I would not merge it as-is.

Required before merge:

1. ✅ Make cleanup instance-specific (ID/generation check).
2. ✅ Add regression test for remove/reinsert race.
3. ⚠️ Prefer a shared cleanup worker over spawning one thread per download.

The minimal safe patch is probably small: keep the current scheduling mechanism if desired, but pass a unique downloader generation token into cleanup and verify it before removal. That would eliminate the correctness bug without requiring a larger refactor. ([GitHub]1)