Fix cached blocks causing a mutation which would break retries #165
Conversation
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
WalkthroughReorganized Algorand tests into per-dataset lifecycles and switched test typing to AlgorandApiService; refactored Algorand API by removing two batch helper methods and implementing cache-aware concurrent block fetching with Promise.all; added a CHANGELOG entry for a cache mutation fix. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant FetchBlocks as fetchBlocks()
participant Cache as blockInCache
participant API as getBlockByHeight()
rect rgb(245, 255, 240)
Note over Caller,API: Concurrent, cache-aware block fetching (Promise.all)
end
Caller->>FetchBlocks: fetchBlocks([nums])
par For each num
FetchBlocks->>Cache: blockInCache(num)?
alt cache hit
Cache-->>FetchBlocks: cached Block
else cache miss
Cache-->>FetchBlocks: null
FetchBlocks->>API: getBlockByHeight(num)
API-->>FetchBlocks: AlgorandBlock
end
end
FetchBlocks-->>Caller: Promise.all([blocks])
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Coverage reportCaution Test run failed
Test suite run failedFailed tests: 3/21. Failed suites: 2/4.Report generated by 🧪jest coverage report action from d5fb94e |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/node/src/algorand/algorand.spec.ts (1)
148-167: Excellent test for the cache mutation bug fix!This test directly validates the PR objective by ensuring the cache doesn't mutate the input
blockNumsarray. The test strategy is sound:
- Trigger lookahead caching by fetching block N-1
- Fetch block N twice (once potentially from cache, once fresh)
- Assert the input array remains unchanged
- Verify consistent results across fetches
This prevents the bug where mutated block numbers would cause blocks to be skipped during retries.
Minor suggestion: The comment on line 157 could be clearer. Consider:
- // The cache should have cleared the previous block, so this fetch is fresh + // Fetch the same block again (cache may have expired, triggering a fresh request)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/node/src/algorand/algorand.spec.ts(2 hunks)packages/node/src/algorand/api.algorand.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/node/src/algorand/api.algorand.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/node/src/algorand/algorand.spec.ts (2)
packages/node/src/algorand/api.service.algorand.ts (1)
api(63-65)packages/types/src/interfaces.ts (1)
AlgorandBlock(184-199)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Coverage report
🔇 Additional comments (7)
packages/node/src/algorand/algorand.spec.ts (7)
12-12: LGTM!The
AlgorandBlockimport provides proper typing for the test data and improves type safety.
38-66: LGTM!The helper function is well-structured with proper dependency injection and explicit typing for
AlgorandApiService. The factory pattern correctly initializes the service with required dependencies.
121-134: LGTM!The per-dataset lifecycle pattern provides better test isolation by creating fresh app instances for each test in the testnet suite.
137-145: LGTM!The pagination test correctly validates that large blocks are properly handled with multiple paginated requests.
170-183: LGTM!The per-dataset lifecycle pattern for mainnet tests provides proper isolation and resource management. Fetching the test block in
beforeEachis efficient since all tests use the same block.
185-196: LGTM!The circular reference test appropriately validates that blocks and transactions with circular references (
transaction.block) can be safely stringified and parsed without errors.
198-204: LGTM!The grouped transactions test correctly validates the
getTransactionsByGroupfunctionality. The non-null assertions are appropriate for testing against a known, deterministic mainnet block.
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.
Description
Fixes an issue where blocks would not be indexed if fetching failed because of caching blocks.
Because there is a need to look ahead to the next block, to get the previous block hash there is a cache to optimise network requests.
When this cache was hit, it would mutate the block numbers input, this is fine when all network requests work but if there is a failure then it would retry with the same block numbers, except it was mutated to exclude the blocks from the cache.
Type of change
Please delete options that are not relevant.
Checklist
Summary by CodeRabbit