fix(registry-client): clear blob blocks on cancelled/failed downloads

[a-mium]

Fixes #3218

What problem does this PR solve?

@qvac/registry-client leaks hypercore blocks into the local corestore when a download is cancelled or fails. downloadModel/downloadBlob only cleared the blocks they pulled into the RocksDB-backed corestore on the success path; the catch (error) path closed the core and re-threw without clearing. The leaked blocks are logically live (not tombstoned), so RocksDB compaction never reclaims them and the corestore grows without bound across distinct cancelled loads. The assembled output file is cleaned up by a separate clearCache path, so only the corestore blocks leak and the visible models cache stays empty while the on-disk db dir keeps growing.

Before this change, success paths cleared via _clearBlobBlocks (core.clear(start, end, { diff: true }) + core.compact()) in both downloadModel and downloadBlob, while the catch blocks did not.

Empirical measurement against a running service on the downstream @qvac/sdk:

• Cancelling three distinct models mid-download added +74 MB, +36 MB, +59 MB to the corestore db dir — monotonic — while the models cache dir stayed empty.
• Cancelling the same model repeatedly did not grow monotonically (it re-fetches the same content-addressed range, which churns and later compacts) — consistent with the root cause: the leak is per distinct block range that is never cleared.

How does it solve it?

• In the catch (error) of both downloadModel and downloadBlob, clear the pulled block range before closing the core, guarded by if (core && blockStart != null) so a failure before the range was computed is a no-op. This mirrors the existing success-path clearing.
• Hoisted the blockStart/blockEnd/rangeDownload declarations out of the try (they were const inside it and thus out of scope in the catch); they are now let-declared alongside core/blobs and assigned in place.
• Destroy the range download before clearing, matching the success-path ordering — a mid-flight clear can otherwise be refilled by ongoing replication and leave durable blocks after close.
• Collapsed the destroy/clear/close teardown, which was copied across six sites, into a shared _releaseDownload. Close errors are now warned uniformly, so a close failure after a fully written file no longer rejects a download that actually succeeded.
• Fixed a second leak on the no-outputFile path: cleanup was bound to stream end only, so a consumer destroying the returned stream (which emits close without end) never freed its blocks — the same cancel scenario this PR targets. Release now runs on end or close, behind a once-guard.
• Added regression tests (tests/unit/client.download-retry.test.js) covering the catch path, the stream-close path, the release guards when no core or block range exists yet, and the destroy-before-clear ordering.

Note on verification: npm run test:unit and npm run lint both pass in the package (33/33 tests, 127/127 asserts). The new tests were confirmed to discriminate, not just pass: reverting the destroy-before-clear fails the ordering assertions, binding only end fails the two stream-close tests, and removing the once-guard fails the single-release test with actual: 2, expected: 1. The tests forward the client logger into TAP output, so the run shows which production branch actually executed on each path.

Scope note: _clearBlobBlocks is stubbed in the unit tests, so they prove the orchestration — that clearing happens, once, in the right order, with the right block range — not that core.clear()/compact() reclaims disk. The measurements above remain the evidence for that.

Breaking changes

None.

[github-actions]

Review Status

Current Status: ✅ APPROVED
Approvals so far: Management: 1, Team Lead: 1

[opaninakuffo]

thanks @a-mium! This fixes the exact leak from #3218 — catch paths clear via _clearBlobBlocks, and the regression tests lock that in. ran the unit file locally (9/9). blocking gap: catch doesn't destroy the outer rangeDownload before clear (success path does). harness against hypercore@11 showed mid-flight clear without destroy can refill and leave durable blocks after close; destroy-then-clear-then-close left 0. same treatment needed in downloadBlob.

🔴 requesting changes

[a-mium]

Force-pushed: all four review comments addressed, one commit each. Tests now run in-repo - 33/33 unit tests, 127/127 asserts, lint clean.

Two commits go beyond the review comments, flagging them so they are not a surprise:

• 54e7c65 collapses the destroy/clear/close teardown, which was copied across six sites, into a shared _releaseDownload. Close errors are now warned uniformly, so a close failure after a fully written file no longer rejects a download that succeeded.
• 43f050d fixes a leak the original patch missed: the no-outputFile path bound cleanup to stream end only, so a consumer destroying the returned stream (emitting close without end) never freed its blocks - the same cancel scenario this PR targets.

[opaninakuffo]

thanks @a-mium, updates lgtm. Kindly resolve the merge conflicts

[a-mium]

thanks @a-mium, updates lgtm. Kindly resolve the merge conflicts

done

[Copilot]

Pull request overview

This PR fixes a disk-growth leak in @qvac/registry-client where partially downloaded hypercore blocks could remain live in the local corestore when a download is cancelled, a stream is destroyed, or a download fails. It centralizes teardown so that block-range replication is stopped and cached blocks are cleared on both success and failure paths, matching the intent of Issue #3218.

Changes:

• Hoists block-range state (blockStart/blockEnd/rangeDownload) so the catch path can release resources and clear cached blocks consistently.
• Introduces shared release helpers (_releaseDownload, _releaseOnStreamEnd) to stop replication, clear blocks (when applicable), and close resources, including handling stream close (destroyed stream) in addition to end.
• Adds unit regression tests covering failure-path cleanup, stream-destroy cleanup, single-release guarding, and destroy-before-clear ordering.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
packages/registry-server/client/lib/client.js	Ensures cached block ranges are released on failure/cancel paths and on stream close, consolidating cleanup into shared helpers to prevent corestore block leaks.
packages/registry-server/client/tests/unit/client.download-retry.test.js	Adds regression tests that assert the new cleanup orchestration (clear-on-catch, clear-on-stream-close, once-guard, and destroy-before-clear ordering).

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review is ineligible. To be eligible to request a review, you need a paid Copilot license, or your organization must enable Copilot code review.

[localhost41]

I independently checked the current head (f7da42d4). The earlier destroy-before-clear concern is addressed: both failure paths now stop rangeDownload before clearing, stream cancellation is covered through close, and the end/close guard releases only once.

Local verification from packages/registry-server/client:

• npx brittle tests/unit/client.download-retry.test.js — 14/14 tests, 40/40 assertions passed
• npm run test:unit — 33/33 tests, 127/127 assertions passed
• npm run typecheck — passed
• npm run lint — fails with one error at lib/client.js:520: blockStart != null violates eqeqeq

Because blockStart begins as undefined and is otherwise assigned a numeric offset, blockStart !== undefined preserves the guard while satisfying the project lint rule. I did not find another functional issue in the revised cleanup paths.

[yuranich]

🤖: Approved. This cleanup stops the active range and is a meaningful improvement. A real Corestore test still found that in-flight blocks can land after destroy and before clear; follow-up #3496 drains those requests before cleanup.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

packages/registry-server/client/lib/client.js:513

• _releaseOnStreamEnd only triggers cleanup on 'end' and 'close'. If the hyperblobs read stream emits an 'error' without subsequently emitting 'close' (which can happen for some Readable implementations), the blob/core handles and cached block range may never be released on stream consumers that don't explicitly destroy the stream. Adding an 'error' listener here makes the release behavior robust for the failure path this PR is targeting.

    stream.once('end', release)
    stream.once('close', release)

[a-mium]

@yuranich @opaninakuffo kindly help to merge, I see there is github error: Error: GitHub API error: label strip failed: HTTP 403 (status=403 method=DELETE path=/repos/tetherto/qvac/issues/3219/labels/verified), thank you

[Copilot]

Copilot was unable to review this pull request because the user who requested the review is ineligible. To be eligible to request a review, you need a paid Copilot license, or your organization must enable Copilot code review.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review is ineligible. To be eligible to request a review, you need a paid Copilot license, or your organization must enable Copilot code review.