Delay `updateAll()` & only update subset immediately by lejeunerenard · Pull Request #815 · holepunchto/hypercore

lejeunerenard · 2026-05-19T21:54:25Z

Update all is too aggressive at the moment and tries to bump any pending ranges etc every time. This is meant to cover edgecases but that can be done via a delay instead.

If the number of peers is greater than or equal to MIN_PEER_UPDATE_ALL (currently 10), only a subset of the peers are scanned immediately. A full scan is schedule later with a delay based on the number of peers. Math.sqrt(this.peers.length) * 100ms

updateAll() now takes an optional limit arg so the code is reused between the subset & complete scans.

All calls to updateAll() were refactor to queueUpdateAll() except in core.truncate() as that is assumed to be strongly propagated.

If the number of peers is greater than or equal to `MIN_PEER_UPDATE_ALL` (currently `10`), only a subset of the peers are scanned immediately. A full scan is schedule later with a delay based on the number of peers. `Math.sqrt(this.peers.length) * 100ms` `updateAll()` now takes an optional `limit` arg so the code is reused between the subset & complete scans. All calls to `updateAll()` were refactor to `queueUpdateAll()` except in `core.truncate()` as that is assumed to be strongly propagated.

mafintosh · 2026-05-19T21:58:50Z

    this._notDownloadingTimer = null

+    this._updateAllBump = null
+    this._updateAllBound = () => {


use a bind instead on an instance method

I needed to set _updateAllBump to null so it would reschedule. would normally put in updateAll but wanted to reuse it for both subset and full scan.

thats ok, just define this as _something and then bind that here.

actually much easier if you add to updateAll no?

if (this._updateBump !== null) {
clearTimeout(this._updateBump)
this._updateBump = null
}

then my below comment on the clear is irrelevant and you can rmeove the clearTimeotu down there also from the else

Adjusted so the bound version is just a .bind() and moved the clear to updateAll(). It checks if a full scan before clearing since the method is used for the subset as well.

mafintosh · 2026-05-19T22:01:25Z


+  queueUpdateAll() {
+    if (this.peers.length >= MIN_PEER_UPDATE_ALL) {
+      const MIN_DELAY_UPDATE_ALL = Math.sqrt(this.peers.length) * 100


normal case, its not a constant. I think you should define a function isntead that just makes this static

getDelay(peers) {
if (peers < 10) return 100
if (peers < 50) return 200
if (peers < 100) return 300
if (peers < 400) return 400
return 1000
}

or whatever we want

Do we still want it to be square root? I like that it doesn't grow linear just in case.

mafintosh · 2026-05-19T22:02:30Z

 const LAST_BLOCKS = 256
-
 const MAX_RANGES = 64
+const MIN_PEER_UPDATE_ALL = 10


we should prob set a bit higher, like

30-40 i think

mafintosh · 2026-05-19T22:19:42Z

+      if (this._updateAllBump !== null) return //skip if already scheduled
+      this._updateAllBump = setTimeout(this._updateAllBound, this.getUpdateAllDelay(this.peers))
+    } else {
+      clearTimeout(this._updateAllBump)


need to set bump to null post this btw, otherwise live lock per above

That's why it sets null here:
https://github.com/holepunchto/hypercore/pull/815/changes#diff-3344a84994d8e172768f33ec4bf514ba2bc28eb7eeffff10fcbbd294efe56edcR2027

mmm, you are cancelling that call here tho

ah, you're right sorry. i have fixed per the other comment.

mafintosh · 2026-05-19T22:36:06Z


  getUpdateAllDelay(peers) {
-    return Math.sqrt(peers.length) * 100
+    return Math.min(3000, Math.max(100, (peers.length - MIN_PEER_UPDATE_ALL) * 5))


ok if MIN_PEER_UPDATE is relatively non small, like 40-50

Updated MIN_PEER_UPDATE_ALL to 40. Previously was 30.

lejeunerenard · 2026-05-19T23:06:01Z

Merged main in as it had a conflict.

marcus-pousette-hp · 2026-05-25T17:56:13Z

I got curious about this PR,
I think this can leave _updateAllBump stuck non-null. If the delayed full scan fires while _applyingReorg is active, updateAll() returns before clearing the timer handle. After that, queueUpdateAll() keeps doing only the limited scan and never schedules another full scan

marcus-pousette-hp · 2026-05-25T18:00:37Z

The regression I am analysing:

test('delayed updateAll clears timer after reorg skip', async function (t) {
  const Replicator = require('../lib/replicator')
  const r = new Replicator({})

  t.teardown(() => {
    if (r._updateAllBump) clearTimeout(r._updateAllBump)
  })

  r.peers = Array.from({ length: 40 }, () => ({}))
  r.getUpdateAllDelay = () => 1

  // Simulate the delayed full scan firing while a reorg is active
  r._applyingReorg = {}

  r.queueUpdateAll()
  await new Promise(resolve => setTimeout(resolve, 10))

  t.is(r._updateAllBump, null, 'timer handle should be cleared after firing')
})

@marcus-pousette-hp

Test based on @marcus-pousette-hp's repo with tweaks to make it more realistic at the cost of taking longer.

marcus-pousette-hp · 2026-06-05T21:20:54Z

Sanity check, I am investigating side effects that

core.upgrade({ wait: true })

can resolve early false because in the case if there is 41 peers, and we do

queueUpdateAll() calls updateAll(40) which call _checkUpgradeIfAvailable() in the end, and from that we can end up calling u.resolve(false)

If I understand it from previous behaviour, this is different, and unexpected. Since we should not resolve early false we we acutally have not waited for the full update.

I could try to produce a regression test

marcus-pousette-hp · 2026-06-08T11:37:10Z

Sanity check, I am investigating side effects that
core.upgrade({ wait: true }) 
can resolve early false because in the case if there is 41 peers, and we do

queueUpdateAll() calls updateAll(40) which call _checkUpgradeIfAvailable() in the end, and from that we can end up calling u.resolve(false)

If I understand it from previous behaviour, this is different, and unexpected. Since we should not resolve early false we we acutally have not waited for the full update.

I could try to produce a regression test

It becomes a bit messy but, this seems to catch the regression (Co-authored with AI)

test('wait update does not resolve false before delayed full scan with many peers', async function (t) {
  const writer = await create(t)
  const blocks = []
  for (let i = 0; i < 100; i++) blocks.push('block-' + i)
  await writer.append(blocks)

  const reader = await create(t, writer.key, { eagerUpgrade: false })

  // Bootstrap reader to the stale length, then disconnect so only the explicit
  // update below can observe the later writer append.
  let streams = replicate(writer, reader, t, { teardown: false })
  await reader.update({ wait: true, force: true })
  await reader.download({ start: 0, end: 100 }).done()
  await unreplicate(streams)

  const stalePeers = []
  for (let i = 0; i < 40; i++) {
    const peer = await create(t, writer.key)

    streams = replicate(writer, peer, t, { teardown: false })
    await peer.update({ wait: true, force: true })
    await peer.download({ start: 0, end: 100 }).done()
    await unreplicate(streams)

    stalePeers.push(peer)
  }

  await writer.append('new-block')

  // Keep real non-primary work pending. Without this, the skipped writer can
  // remain in the first 3 peers that _checkUpgradeIfAvailable() samples.
  await reader.clear(0, 100)
  const range = reader.download({ start: 0, end: 100, linear: true })

  // These live streams can use normal test teardown.
  replicate(writer, reader, t)
  for (const peer of stalePeers) replicate(peer, reader, t)

  while (reader.replicator.peers.length !== 41) {
    await new Promise((resolve) => setImmediate(resolve))
  }

  const oldRandom = Math.random
  let update = null

  try {
    // RandomIterator otherwise makes whether the writer is skipped probabilistic.
    Math.random = () => 0

    update = reader.update({ wait: true, force: true })

    let settled = false
    let result = null
    update.then((value) => {
      settled = true
      result = value
    })

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

    t.is(settled, false, `partial scan should not resolve wait update as ${result}`)
  } finally {
    Math.random = oldRandom

    // In the fixed case, update is intentionally still pending at the assertion.
    // Cancel it so the test does not leave an active upgrade ref behind.
    if (update !== null) {
      reader.core.replicator.clearRequests(reader.activeRequests, null)
      await update.catch(() => {})
    }

    range.destroy()

    // When the cleanup issue in PR is fixed this can be removed
    if (reader.replicator._updateAllBump) {
      clearTimeout(reader.replicator._updateAllBump)
      reader.replicator._updateAllBump = null
    }
  }
})

lejeunerenard · 2026-06-08T18:58:32Z

@marcus-pousette-hp thanks for checking this. This tests also doesn't pass on main. This is because of the limit MAX_PEERS_UPGRADE which constrains how many peers we check when looking for an upgrade. In the test case, it checks the max (3) and determines there is no upgrade available atm so the core is updated.

The docs are a bit confusing as they say:

Use core.findingPeers() or { wait: true } to make await core.update() blocking.

Which does sound like it would be fully blocking by exhaustively checking all peers, but instead is more of a 'blocking, if available' where 'if available' is a check against a random subset of peers.

@marcus-pousette-hp

ht @marcus-pousette-hp for finding this.

The timeout only works if the entire process exits with it. It also was brittle.

marcus-pousette-hp · 2026-06-08T20:45:55Z

@marcus-pousette-hp thanks for checking this. This tests also doesn't pass on main. This is because of the limit MAX_PEERS_UPGRADE which constrains how many peers we check when looking for an upgrade. In the test case, it checks the max (3) and determines there is no upgrade available atm so the core is updated.

The docs are a bit confusing as they say:

Use core.findingPeers() or { wait: true } to make await core.update() blocking.

Which does sound like it would be fully blocking by exhaustively checking all peers, but instead is more of a 'blocking, if available' where 'if available' is a check against a random subset of peers.

Then I am happy with the PR! I guess this is either a documentation bug, or a behaivour bug that would need a follow up in another issue.

lejeunerenard requested a review from a team May 19, 2026 21:54

Lint

36f8e67

mafintosh reviewed May 19, 2026

View reviewed changes

lejeunerenard added 2 commits May 19, 2026 17:04

Update MIN_PEER_UPDATE_ALL to 30

5d01efd

Refactor updateAll delay amount out into a function

c11ee68

mafintosh reviewed May 19, 2026

View reviewed changes

Change getUpdateAllDelay to return a clamped linear delay

a7990ad

mafintosh reviewed May 19, 2026

View reviewed changes

lejeunerenard added 3 commits May 19, 2026 17:55

Clear timeout when running full scan updateAll()

73ce602

Update MIN_PEER_UPDATE_ALL to 40

e5048c3

Merge branch 'main' into min-delay-update-all

598281c

marcus-pousette-hp mentioned this pull request May 26, 2026

Fix pending update on peer close #821

Merged

lejeunerenard added 3 commits June 4, 2026 16:06

Add test for stuck full scan & fix by checking reorg after clear

0632661

Test based on @marcus-pousette-hp's repo with tweaks to make it more realistic at the cost of taking longer.

Merge branch 'main' into min-delay-update-all

d07cb0b

Lint test/replicate.js

0003de0

marcus-pousette-hp reviewed Jun 5, 2026

View reviewed changes

Comment thread lib/replicator.js

lejeunerenard added 3 commits June 8, 2026 14:30

Clear _updateAllBump timer when replicator is destroyed

7b2ad5d

ht @marcus-pousette-hp for finding this.

Refactor test to just check if timer is nulled

9454328

The timeout only works if the entire process exits with it. It also was brittle.

Add updateSubset() to update a subset of peers instead of passing arg

725aa1a

Conversation

lejeunerenard commented May 19, 2026

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lejeunerenard commented May 19, 2026

Uh oh!

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

Uh oh!

Uh oh!

marcus-pousette-hp commented May 25, 2026

Uh oh!

marcus-pousette-hp commented Jun 5, 2026

Uh oh!

Uh oh!

marcus-pousette-hp commented Jun 8, 2026

Uh oh!

lejeunerenard commented Jun 8, 2026

Uh oh!

marcus-pousette-hp commented Jun 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

marcus-pousette-hp commented May 25, 2026 •

edited

Loading