Skip to content

Fix idle range completion scan - #832

Merged
lejeunerenard merged 20 commits into
mainfrom
fix/idle-range-completion
Jul 28, 2026
Merged

Fix idle range completion scan#832
lejeunerenard merged 20 commits into
mainfrom
fix/idle-range-completion

Conversation

@marcus-pousette-hp

@marcus-pousette-hp marcus-pousette-hp commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

co-authored with AI

Comment thread lib/replicator.js
@marcus-pousette-hp
marcus-pousette-hp marked this pull request as ready for review June 23, 2026 16:01
@marcus-pousette-hp
marcus-pousette-hp requested a review from a team June 23, 2026 16:01
Comment thread lib/replicator.js Outdated

@lejeunerenard lejeunerenard 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.

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:

  1. make it pick random subset of ranges to avoid stacking unfulfillable ranges at the front.
  2. 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

Comment thread lib/replicator.js Outdated
Comment thread lib/replicator.js Outdated
Comment thread lib/replicator.js
@holepunchto holepunchto deleted a comment from marcus-pousette Jul 13, 2026
@marcus-pousette-hp

Copy link
Copy Markdown
Contributor Author

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 lejeunerenard 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.

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.

Comment thread lib/replicator.js Outdated
Comment thread lib/replicator.js Outdated
Comment thread lib/replicator.js
Comment thread test/replicate.js Outdated
Comment thread test/replicate.js Outdated
Comment thread test/replicate.js Outdated

@lejeunerenard lejeunerenard 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.

Only one test i'm not sure what it's purpose is, but otherwise only notes about seeks vs ranges behavior for the PR in the future.

Comment thread lib/replicator.js
Comment thread lib/replicator.js Outdated
Comment thread test/replicate.js Outdated
@lejeunerenard
lejeunerenard merged commit 922b0f1 into main Jul 28, 2026
10 of 15 checks passed
@lejeunerenard
lejeunerenard deleted the fix/idle-range-completion branch July 28, 2026 21:56
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.

3 participants