fix(NODE-1935): Rework read state caching#9792
Open
blind-oracle wants to merge 6 commits intomasterfrom
Open
Conversation
Bownairo
approved these changes
Apr 9, 2026
| pub body: Vec<u8>, | ||
| } | ||
|
|
||
| /// Paths for a read state call |
Contributor
There was a problem hiding this comment.
nit: Since this is pub and could be constructed directly, can we have a note that the paths should be sorted?
andrewbattat
approved these changes
Apr 9, 2026
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
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.
Narrow down the cacheable paths to the following form:
That is, only requests which have 2 paths, each of them has 2 labels, first of which is
canister_rangesorsubnet(or the other way around) and the second one is a validPrincipal(in essence this just means shorter than 30 bytes). All other paths are deemed non-cacheable. This also fixes an issue - before we considered paths with at least one of thecanister_rangesorsubnetcacheable, where in fact we aim to cache only if we have both.Add an explicit check that the request is a subnet read state
Move the logic that decides if paths are cacheable or not to
process.rsto avoid storing the whole pathsVecin the request context, which might be large, up to 4MB. This avoids keeping the large context around for the duration of the request - only cacheable paths are stored. It's not ideal from architecture viewpoint, but I don't see other way w/o reworking the code a lot...Add early checks for the request body in preprocess and response body in read state caching middleware to avoid dealing with known-big in advance bodies
Fix the weigher to size the paths correctly (not that it matters much anymore since we only cache small ones).
Add/fix some tests