Fix idle range completion scan by marcus-pousette-hp · Pull Request #832 · holepunchto/hypercore

marcus-pousette-hp · 2026-06-22T20:29:54Z

co-authored with AI

marcus-pousette-hp · 2026-06-23T13:29:37Z

-          this._resolveRangeRequest(r)
-          i--
-          if (len > this._ranges.length) len--
-          if (this._ranges.length === MAX_RANGES) updateAll = true


Removed the old this._ranges.length === MAX_RANGES special case because it only re-triggered updateAll when the queue shrank to exactly the cap, and I couldn’t find test coverage or PR discussion showing that boundary behavior is intentional.

The actual MAX_RANGES cap is still preserved. The new cursor loop drains completed ranges in capped chunks without relying on that exact effect.

I dont think this should be removed. The updateAll prompts requesting ranges from a random subset of peers potentially. Since the _updateNonPrimary function is called only when either receiving data or an upgrade, it could deadlock with requests queue in _ranges that haven't been requested of peers that can fulfill them if updateAll() isn't triggered.

The reason for the exact match is that we know in this scenario only 1 range was resolved so this check triggers as soon as there can be at least one range that could not have been requested of peers. Aka triggering another set of range requests could prompt a response which resolves a range not included in the last _updateNonPrimary run.

This doesn't prevent from large backlogs not getting stuck (hence the need for this PR) when there is more unfulfilled range request than the MAX_RANGES though. I don't think it should be updateAll = true for this._ranges.length >= MAX_RANGES though.

Ye, that make sense. I restored this in 5403eb5, preserving the exact this._ranges.length === MAX_RANGES boundary. I also added a regression test with 65 one-block ranges and two peers as an proof/argumetn for this. Though it is quite verbose right now, I will re-iterate on this tomorrow.

mafintosh · 2026-06-29T14:18:19Z

+        if (checked === 0 || checkedSinceResolve >= this._ranges.length) break
+        if (!drain) break
+
+        await yieldToLoop()


why is this here?

Previous behavior had implied event loop starvation prevention mechanics:

hypercore/lib/replicator.js

Lines 2607 to 2609 in c2ee97e

let len = Math.min(MAX_RANGES, this._ranges.length)

for (let i = 0; i < len; i++) {

In this PR I am trying to preserve this behavior by simultaniously also fixing the bug:

while (this._ranges.length > 0) { this._updateRanges(...MAX_RANGES...) ... await yieldToLoop() }

Also possible that debounce/coalescing mechanisms could be more probable to be invoked in _updateNonPrimary because of the yieldToLoop

An additional improvement we can do here is to assert this behavior with a test

@marcus-pousette-hp the debouncing/guard via _updatesPending could delay seeks from being processed assuming a large number of ranges. This prevents from seeks being added here:

hypercore/lib/replicator.js

Line 1545 in ddd6c79

if (this.replicator._updatesPending > 0) return false

Here's a test case for asserting that seeks are processed relatively quickly. Before the PR it passes and after this PR it fails:

test.solo('processing ranges doesnt block seeks', async function (t) { const core = await create(t) const clone = await create(t, core.key) // Well above MAX_RANGES so draining the backlog takes many batches: 50k/64 const totalLength = 50_000 await core.append(new Array(totalLength).fill('a')) // Make all range requests stay in `_ranges` forever so `_updateNonPrimary` has an oversized backlog to scan. await core.clear(0, totalLength) replicate(core, clone, t) // Populate ranges for (let i = 0; i < totalLength; i++) { clone.download({ start: i, end: i + 1 }).done().catch(noop) } t.is(clone.core.replicator._ranges.length, totalLength, 'large range backlog is pending') const bytesBefore = core.byteLength // Appending triggers an upgrade on clone, kicking off `_updateNonPrimary` to attempt the backlog. // Seeking to `bytesBefore` needs that same upgrade (so clone doesnt know about it). await core.append('end') // Baseline (no range backlog) resolves this in ~1ms; 900ms+ with the backlog const BUDGET = 100 const start = Date.now() let delta = -1 const seek = clone.seek(bytesBefore).then(() => { delta = Date.now() - start }) await once(clone, 'append') // To allow merkle IO for seek await new Promise((resolve) => setImmediate(resolve)) t.is(clone.core.replicator._seeks.length, 0, 'seek resolved after upgrade received') await seek t.ok(delta <= BUDGET, `seek resolved within ${BUDGET}ms despite a ${totalLength}-range backlog`) t.comment('seek time', delta) t.is(clone.core.replicator._ranges.length, totalLength, 'range backlog is still pending') })

Test made with a litte AI assistance.

lejeunerenard

This approach is more thorough when completing range requests, but it blocks seek requests when processing large amounts of ranges. Accounting for this seems like it would add more state at the replicator level for tracking where the cursor left off etc.

If we keep _updateNonPrimary's workload small but:

make it pick random subset of ranges to avoid stacking unfulfillable ranges at the front.
trigger non-primary updates more often, maybe when inflight goes idle

We can allow it to yield to seeks while being a minor change. I have the following competing PR with the above, which still doesn't pass atm. #841

lejeunerenard · 2026-07-09T16:17:59Z

 function noop() {}

+function yieldToLoop() {
+  return new Promise((resolve) => setTimeout(resolve, 0))


Suggested change

return new Promise((resolve) => setTimeout(resolve, 0))

return new Promise((resolve) => setImmediate(resolve))

setTimeout defaults to a delay of 1 though the exact timing can be variable I believe.

lejeunerenard · 2026-07-09T20:15:15Z

+        if (checked === 0 || checkedSinceResolve >= this._ranges.length) break
+        if (!drain) break
+
+        await yieldToLoop()


@marcus-pousette-hp the debouncing/guard via _updatesPending could delay seeks from being processed assuming a large number of ranges. This prevents from seeks being added here:

hypercore/lib/replicator.js

Line 1545 in ddd6c79

if (this.replicator._updatesPending > 0) return false

Here's a test case for asserting that seeks are processed relatively quickly. Before the PR it passes and after this PR it fails:

test.solo('processing ranges doesnt block seeks', async function (t) { const core = await create(t) const clone = await create(t, core.key) // Well above MAX_RANGES so draining the backlog takes many batches: 50k/64 const totalLength = 50_000 await core.append(new Array(totalLength).fill('a')) // Make all range requests stay in `_ranges` forever so `_updateNonPrimary` has an oversized backlog to scan. await core.clear(0, totalLength) replicate(core, clone, t) // Populate ranges for (let i = 0; i < totalLength; i++) { clone.download({ start: i, end: i + 1 }).done().catch(noop) } t.is(clone.core.replicator._ranges.length, totalLength, 'large range backlog is pending') const bytesBefore = core.byteLength // Appending triggers an upgrade on clone, kicking off `_updateNonPrimary` to attempt the backlog. // Seeking to `bytesBefore` needs that same upgrade (so clone doesnt know about it). await core.append('end') // Baseline (no range backlog) resolves this in ~1ms; 900ms+ with the backlog const BUDGET = 100 const start = Date.now() let delta = -1 const seek = clone.seek(bytesBefore).then(() => { delta = Date.now() - start }) await once(clone, 'append') // To allow merkle IO for seek await new Promise((resolve) => setImmediate(resolve)) t.is(clone.core.replicator._seeks.length, 0, 'seek resolved after upgrade received') await seek t.ok(delta <= BUDGET, `seek resolved within ${BUDGET}ms despite a ${totalLength}-range backlog`) t.comment('seek time', delta) t.is(clone.core.replicator._ranges.length, totalLength, 'range backlog is still pending') })

Test made with a litte AI assistance.

lejeunerenard · 2026-07-09T21:26:59Z

-          this._resolveRangeRequest(r)
-          i--
-          if (len > this._ranges.length) len--
-          if (this._ranges.length === MAX_RANGES) updateAll = true


I dont think this should be removed. The updateAll prompts requesting ranges from a random subset of peers potentially. Since the _updateNonPrimary function is called only when either receiving data or an upgrade, it could deadlock with requests queue in _ranges that haven't been requested of peers that can fulfill them if updateAll() isn't triggered.

The reason for the exact match is that we know in this scenario only 1 range was resolved so this check triggers as soon as there can be at least one range that could not have been requested of peers. Aka triggering another set of range requests could prompt a response which resolves a range not included in the last _updateNonPrimary run.

This doesn't prevent from large backlogs not getting stuck (hence the need for this PR) when there is more unfulfilled range request than the MAX_RANGES though. I don't think it should be updateAll = true for this._ranges.length >= MAX_RANGES though.

marcus-pousette-hp · 2026-07-16T09:58:15Z

One note, processing can get slow if there are many seeks at once or a very large backlog of ranges. Potentially optimization/perf improrvement can be made here but left it out to make the PR easier

lejeunerenard

Did another pass. Still need to cover some of the tests but found a potential way that ranges now can be blocked instead of seeks.

lejeunerenard · 2026-07-21T15:39:54Z

+        const result = this._updateRanges(rangeIndex, limit)
+        const { checked, resolved } = result
+        rangeIndex = result.index

-        let err = null
-        let res = null
+        if (result.updateAll) updateAll = true


Suggested change

const result = this._updateRanges(rangeIndex, limit)

const { checked, resolved } = result

rangeIndex = result.index

let err = null

let res = null

if (result.updateAll) updateAll = true

const { checked, resolved, index, updateAll: resultUpdateAll } = this._updateRanges(rangeIndex, limit)

rangeIndex = index

if (resultUpdateAll) updateAll = true

lejeunerenard · 2026-07-21T15:47:55Z

+          if (seekIndex < this._seeks.length - 1) {
+            this._seeks[seekIndex] = this._seeks.pop()
+          } else this._seeks.pop()


This can be slightly simpler if we always pop and only swap if the popped version isn't the s. Something like this:

hypercore/lib/replicator.js

Lines 283 to 286 in 3cd8c50

const h = this.ranges.pop()

if (h !== this) {

this.ranges[rangeIndex] = h

}

lejeunerenard · 2026-07-21T16:57:09Z

+        this._updatingSeeks = true
+
+        let checkedSeek = false
+        for (const s of this._seeks.slice()) {


Just a note as I dont think it is an issue in practice (because seeks are mainly used for streaming), but there is no limit to the number of seeks processed. This means just like ranges held up seek resolution, the opposite is still true where a large number of seeks could hold up range updates (allowing the initial update of a batch of ranges).

I propose we limit the length as well and use RandomIterator. Here is a test I made to showcase the issue:

test('seeks cant block range updates', async function (t) { const a = await create(t) const b = await create(t, a.key) const totalBlocks = 1e5 const totalRanges = 129 // above max range for at least 2x batches const totalSeeks = totalBlocks // Seed data await a.append(Array(totalBlocks).fill('a')) await a.clear(0, a.length - totalRanges) // Clear all but the end replicate(a, b, t) // Setup ranges at the end const ranges = [] for (let i = 0; i < totalRanges; i++) { ranges.push(b.download({ start: a.length - (i + 1), end: a.length - i }).done()) } // Setup large number of seeks const seeks = [] for (let i = 0; i < totalSeeks; i++) { seeks.push(b.seek(i).catch(noop)) } // Allow seeks to be added to the replicator await eventFlush() const result = await Promise.race([ b.core.replicator._updateNonPrimary(true).then(() => 'resolved'), new Promise((resolve) => setTimeout(resolve, 100)).then(() => 'timeout') ]) t.is(result, 'resolved', 'updating non-primary didnt block on seeks') })

lejeunerenard · 2026-07-21T17:05:44Z

+  let streams = replicate(writer, firstPeer, t, { teardown: false })
+  await firstPeer.get(0)
+  await unreplicate(streams)
+
+  const cappedPeer = await create(t, writer.key)
+  streams = replicate(writer, cappedPeer, t, { teardown: false })
+  await cappedPeer.get(totalLength - 1)


You don't need to disable the teardown for these replicate() calls. You can unreplicate with it in place.

lejeunerenard · 2026-07-21T17:12:31Z

  t.is(r._updateAllBump, null, 'timer reset to null')
 })

+test('completed range exposes capped range to existing peer', async function (t) {


Not sure what the purpose of this test is besides that peers can get disparate ranges from different peers. It passes on main so isn't part of the idle range fix. Maybe it is trying to capture that there is a MAX_RANGES batch size per _updateNonPrimary() but it doesn't assert anything about the _updateNonPrimary() lifecycle.

lejeunerenard · 2026-07-21T17:31:17Z

+    if (++updates === 1) {
+      setImmediate(() => {
+        peer.paused = false
+        seek = clone.seek(Math.floor(core.byteLength / 2))


I dont understand why we need to wait to process a single batch of range requests to schedule the seek request for after the poll phase in the loop. Calling here let seek = clone.seek(...) works too.

marcus-pousette-hp added 4 commits June 22, 2026 22:29

Fix idle range completion scan

f9700d5

Fix stuck range completion drain

1414e16

Remove unnecessary range update boundary state

76376ac

Fix queued idle range drain race

60823b1

marcus-pousette-hp commented Jun 23, 2026

View reviewed changes

marcus-pousette-hp marked this pull request as ready for review June 23, 2026 16:01

marcus-pousette-hp requested a review from a team June 23, 2026 16:01

mafintosh reviewed Jun 29, 2026

View reviewed changes

lejeunerenard reviewed Jul 9, 2026

View reviewed changes

Restore peer updates at max range boundary

5403eb5

holepunchto deleted a comment from marcus-pousette Jul 13, 2026

marcus-pousette-hp added 8 commits July 14, 2026 14:21

Yield range drains to pending seeks

37224a9

Assert seeks precede range scan completion

ffd7033

Restart range drain after cancellation

fe789a1

Simplify range drain state and tests

62096ed

Remove redundant range empty checks

aded7ec

Merge main into fix/idle-range-completion

11e0082

Fix seek processing during range drains

0e24e5a

Clear range test timeout

51738b8

marcus-pousette-hp requested a review from lejeunerenard July 16, 2026 10:00

lejeunerenard reviewed Jul 21, 2026

View reviewed changes

	let len = Math.min(MAX_RANGES, this._ranges.length)

	for (let i = 0; i < len; i++) {

	return new Promise((resolve) => setTimeout(resolve, 0))
	return new Promise((resolve) => setImmediate(resolve))

	const h = this.ranges.pop()
	if (h !== this) {
	this.ranges[rangeIndex] = h
	}

Uh oh!

Conversation

marcus-pousette-hp commented Jun 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcus-pousette-hp Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lejeunerenard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcus-pousette-hp commented Jul 16, 2026

Uh oh!

lejeunerenard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

marcus-pousette-hp commented Jun 22, 2026 •

edited

Loading

marcus-pousette-hp Jun 30, 2026 •

edited

Loading