Skip to content

fix[notask]: drain in-flight blob blocks before cleanup - #3496

Closed
yuranich wants to merge 3 commits into
tetherto:mainfrom
yuranich:fix/registry-client-drain-inflight-blocks
Closed

fix[notask]: drain in-flight blob blocks before cleanup#3496
yuranich wants to merge 3 commits into
tetherto:mainfrom
yuranich:fix/registry-client-drain-inflight-blocks

Conversation

@yuranich

@yuranich yuranich commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Cancelled registry downloads can still persist Hypercore blocks after the range request is destroyed. Responses already in flight may arrive between destroy() and clear(), so the cache continues growing across distinct cancellations.

How does it solve it?

  • Waits for Hypercore peer requests and data processing to drain before clearing and compacting the cancelled range, with a bounded timeout.
  • On drain timeout, logs remaining per-peer inflight / dataProcessing counters and still clears (best-effort).
  • Adds a real Corestore/Hyperblobs regression test that cancels while blocks are in flight, reopens storage, and verifies that zero blob blocks remain.
  • Adds a real-storage timeout path that holds core.verify so dataProcessing stays nonzero past the drain deadline, plus a focused unit coverage of the timeout log payload.

Breaking changes

None.

@yuranich
yuranich force-pushed the fix/registry-client-drain-inflight-blocks branch from 88ff407 to 058671d Compare July 28, 2026 13:31
@yuranich
yuranich marked this pull request as ready for review July 28, 2026 13:31
@yuranich
yuranich requested review from a team as code owners July 28, 2026 13:31
@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Review Status

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

@localhost41 localhost41 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked this against the current head and the installed Hypercore implementation. peer.inflight tracks outstanding requests, while peer.dataProcessing stays nonzero across the async verify/store portion of data handling, so waiting on both covers the persistence window this fix targets.

Local verification:

  • full client unit suite: 35/35 tests, 132/132 assertions
  • real Corestore/Hyperblobs storage regression: 25/25 repeated runs
  • typecheck and formatting: pass
  • lint: pass (only the existing require-await warnings in test stubs)

The storage test is particularly useful here: it confirms blocks arrive after cancellation and that zero blocks remain after reopening the Corestore. I did not find a functional issue in the destroy -> drain -> clear/compact ordering. The 5-second path remains intentionally best-effort, but the warning makes that operationally visible.

@opaninakuffo opaninakuffo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right idea — drain peer.inflight/dataProcessing before clear, and the real Corestore reopen test is doing the heavy lifting. ran client.download-retry + client.download-storage locally: 16/16 pass.

rebase onto main first (conflicts with #3513 format pass). validate-pr is red until someone puts verified on this fork PR — https://github.com/tetherto/qvac/actions/runs/30363879302/job/90290162427

Comment thread packages/registry-server/client/lib/client.js
Comment thread packages/registry-server/client/lib/client.js
@localhost41

Copy link
Copy Markdown
Contributor

Follow-up/correction to my earlier review after reading the new comments against the installed Hypercore implementation: my “no functional issue” conclusion was too broad.

The timeout objection is valid. Peer._handleData() increments dataProcessing immediately before await this.core.verify(...) and decrements it only in finally; _waitForInflightBlocks() currently returns at five seconds while that write can still be active, then _clearBlobBlocks() can race the eventual commit. The real Corestore regression proves the normal drain path, but it does not exercise this timeout branch.

I could not reproduce the same concern for rangeDownload.destroy() on the current call paths: _getBlobsCore() awaits core.ready() before core.download(), so Download._open() has no opening await and destroy() reaches context.detach() synchronously. An await Promise.resolve() is still inexpensive defensive hardening if that assumption changes.

For the timeout path, a fixed “clear, short wait, clear” is not sufficient unless the second clear is after dataProcessing actually reaches zero. I would make the wait report whether it drained and add a focused timeout regression. Then either perform a final clear after quiescence before closing the session, or explicitly preserve the bounded/best-effort contract and log the remaining inflight/dataProcessing counters. The current happy-path tests remain useful: I reran the retry plus real-storage files, 16/16 tests and 45/45 assertions pass.

@yuranich
yuranich force-pushed the fix/registry-client-drain-inflight-blocks branch from 058671d to dc3a7f4 Compare July 31, 2026 11:44
@yuranich

Copy link
Copy Markdown
Contributor Author

Closing in favor of #3582, which carries the same change from an upstream tetherto/qvac branch instead of a fork.

@yuranich yuranich closed this Jul 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants