Skip to content

Dedupe inflation promise per activeRequests session - #94

Open
lejeunerenard wants to merge 5 commits into
mainfrom
shared-tree-node-error
Open

lejeunerenard wants to merge 5 commits into
mainfrom
shared-tree-node-error

Conversation

@lejeunerenard

Copy link
Copy Markdown
Contributor

Prior to the fix, the added test showed destroying stream a causing an error in stream b. This is because the tree node would cache the inflating promise which then made requests shared across streams or other contexts.

This fix tried to maintain the caching as much as possible from #60 (where the inflating promise caching came from) by separating by activeRequests session. That keeps the sessions isolated. The original intent of #60 still needs to factored in however.

Made with help from AI.

If the shared node is cancelled in one, its cancelled in the other
propagating the error. Caching is done via the `.inflating` property on
the `TreeNodePointer` during `inflate()`.
Fixes the issue with sharing requests between `activeRequests` sessions
potentially causing errors in unrelated streams.
@lejeunerenard
lejeunerenard requested a review from a team September 17, 2026 20:47
Means that each session wont have their own requests as it will use the
first request instead, but it will fallback to retrying with the next
session if the original destroys the requests.
This allows inflating the keys & children to still be shared while not
sending another request (because hypercore dedupes block requests).

This fixes the previous commits test case where a 2nd stream hangs even
when it is destroyed / cancelled. It previously would hang waiting on
the cached promise from the first stream. Now it has its own request
that will reject the promise.
@caolan-tether

Copy link
Copy Markdown

This is particularly thorny because inflation can generate new requests using the current config. When we piggyback on an existing 'session' (i.e. config) to avoid duplicate work, we are waiting on requests made under a different config. An attempt to inflate a block with timeout: 10_000 followed by an attempt with timeout: 10 would risk waiting 10 seconds instead of 10 ms for the inflate request.

Hypercore's replicator itself handles varying concurrent timeouts correctly, but for that to work, each 'session' needs to make it's own requests via Hypercore (which also means each session needs to perform its own inflate as it might generate nested requests). That would make efforts to deduplicate inflation requests inside hyperbee2 very difficult.

I don't think piggybacking on another session can be done correctly and instead hyperbee2's inflate should step in to manage its own activeRequests to hypercore. If it owns the requests, it could potentially implement similar timeout handling to hypercore for downstream users of hyperbee2 - though this would be non-trivial and might require retry logic. This complexity might be avoided by removing the option to set a 'session' based timeout and to use a fixed timeout per hyperbee2 instance instead. This would make request sharing possible with a refcount for cancellation. I don't know if losing per-session or per-request timeouts is practical for downstream users, however.

If hyperbee2 did own activeRequests internally, I'd propose hyperbee2 implement an AbortController + AbortSignal based API for inflate cancellation.

This branch has not been deployed

No deployments
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.

2 participants