ref-count chainHead_v1_unpin across shared follow#27
Open
lrubasze wants to merge 4 commits into
Open
Conversation
lrubasze
requested review from
BigTava,
Cyr06130,
KarimJedda,
ba1uev,
justraman,
leonardocustodio and
tallesborges
as code owners
Contributor
⚡ Performance Report
|
Contributor
Bundle Size ReportChunks over 500 KB:
All files
Commit: 20e6294 |
Contributor
|
❌ E2E Product suite failed on Failed tests: Logs: https://github.com/paritytech/dotli-community/actions/runs/27579711084 |
leonardocustodio
temporarily deployed
to
dotli.dev
GitHub Actions
Inactive
Two sessions sharing one coalesced chainHead_v1_follow each forward their own chainHead_v1_unpin for the same block, so the broker sends two unpins to the single shared upstream. unfollow is ref-counted across sessions but unpin is not.
Sessions sharing one coalesced chainHead_v1_follow each receive every reported block and unpin independently, but the upstream pinned each block only once. Forwarding every unpin sent duplicates to the single shared follow (double-unpin). Track pin holders per shared follow (block hash -> set of local follow tokens) and forward the upstream unpin only when the last holder releases the block, mirroring the existing unfollow ref-counting. The local unpin response is synthesized so it no longer depends on the upstream reply. On disconnect/unfollow a leaving session's orphaned blocks are unpinned upstream while the follow stays alive; the last token's unfollow releases the rest.
leonardocustodio
force-pushed
the
lrubasze/fix-broker-double-unpin
branch
from
f7ef973 to
060d5a4
Compare
leonardocustodio
temporarily deployed
to
dotli.dev
GitHub Actions
Inactive
BigTava
reviewed
Jun 15, 2026
| continue; | ||
| } | ||
| if (holders.size === 0) { | ||
| sharedFollow.pins.delete(hash); |
Collaborator
There was a problem hiding this comment.
Should we include sharedFollow.pins.delete(hash);?
Contributor
Author
There was a problem hiding this comment.
Not needed for dedup reasons.
This is to prevent a leak. if none of the pins (subscriptions) is holding the block, then the block entry is no longer needed and might be dropped from such from pins list.
…-broker-double-unpin
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #26
Problem
When two tabs share one coalesced
chainHead_v1_follow, the broker forwarded a duplicatechainHead_v1_unpinupstream for the same block — a double-unpin on a single subscription.unfollowwas ref-counted across sessions; unpin` was not.Fix
Ref-count pinned blocks per
SharedFollow(block hash → holding follow tokens), mirroring the existingunfollowhandling:initialized,newBlock) is delivered to a session — for live events and replayed snapshots to late joiners.unpin, drop the session's hold and forward upstream only when no other session still holds the block; reply success locally.Tests
packages/protocol/tests/broker.test.ts: