feat(arns-resolver): multiple resolvers in parallel #365
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #365 +/- ##
===========================================
+ Coverage 72.96% 73.28% +0.31%
===========================================
Files 45 46 +1
Lines 11240 11531 +291
Branches 651 680 +29
===========================================
+ Hits 8201 8450 +249
- Misses 3035 3077 +42
Partials 4 4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
📝 WalkthroughWalkthroughThis pull request introduces a new test suite for the Changes
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
yarn install v1.22.22 ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/resolution/composite-arns-resolver.test.ts (2)
138-142: Consider a shorter timeoutUsing
setTimeout(resolve, 1000)artificially prolongs the test run. For unit tests, consider using a smaller timeout (e.g., 50–100 ms) or a mocked timer to eliminate delays in the test suite. This optimization improves maintainability and developer experience.
198-252: Validate concurrency behavior with multiple requestsThe test verifies max concurrency within a single
resolvecall. As an extra check, you could test multiple parallelresolvecalls to ensure the concurrency limit holds across distinct name resolutions.src/resolution/composite-arns-resolver.ts (2)
44-45: Use descriptive property name forlimitWhile
limiteffectively represents the concurrency limiter, a clearer name—e.g.,runWithLimitorconcurrencyLimit—could improve code readability, especially for new contributors.
118-150: Short-circuiting parallel tasksAlthough
resolveParallelreturns as soon as a successful resolution is obtained, the remaining concurrent promises continue running silently in the background. If you want to cancel or abort them promptly, consider an additional mechanism like an AbortController.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/resolution/composite-arns-resolver.test.ts(1 hunks)src/resolution/composite-arns-resolver.ts(8 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
src/resolution/composite-arns-resolver.test.ts (1)
src/resolution/composite-arns-resolver.ts (2)
CompositeArNSResolver(29-261)resolve(152-260)
src/resolution/composite-arns-resolver.ts (1)
src/types.d.ts (1)
NameResolver(685-693)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test (macos-latest)
🔇 Additional comments (3)
src/resolution/composite-arns-resolver.ts (3)
20-20: Importing p-limit is beneficialGreat choice using
p-limitfor concurrency control. This approach helps maintain a clean concurrency cap without manually managing promises.
53-74: Sensible default concurrencyProviding
maxConcurrentResolutions=2by default is a practical sanity limit. Ensure that users ofCompositeArNSResolverare aware they can override this via configuration if they need a different concurrency level.
77-116: Last resolver has no timeoutIn
resolveWithResolver, the last resolver is allowed to run without a timeout (lines 101–103). Check if this is an intentional design choice. It might be useful to log or document that the final fallback has unlimited time, preventing indefinite waits in certain edge cases.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/resolution/composite-arns-resolver.test.ts (1)
26-35: Avoid shadowing the globalmockResolution.Lines 26-35 define a
mockResolutionobject, and lines 106-114 define anothermockResolutionlocally within the same file. This duplication can lead to confusion or accidental misuse of the wrong reference. Consider renaming one of them or scoping them more clearly to avoid unintentional overlap.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/resolution/composite-arns-resolver.test.ts(1 hunks)src/resolution/composite-arns-resolver.ts(8 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
src/resolution/composite-arns-resolver.test.ts (1)
src/resolution/composite-arns-resolver.ts (2)
CompositeArNSResolver(29-260)resolve(151-259)
src/resolution/composite-arns-resolver.ts (1)
src/types.d.ts (1)
NameResolver(685-693)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test (macos-latest)
- GitHub Check: test (ubuntu-latest)
🔇 Additional comments (5)
src/resolution/composite-arns-resolver.test.ts (2)
52-77: Potential mismatch between test name and assertion
While the test is named "should return early when first resolver succeeds", the second resolver is also being called (line 74 checks that resolver2 was called exactly once). If the intended behavior is truly to short-circuit on success, verify that the second resolver is not invoked, or update the test name and expectations to reflect the actual logic.
198-252: Great test coverage for concurrency control
This set of lines thoroughly tests themaxConcurrentResolutionslimit by tracking active resolutions and verifying the maximum concurrency. These tests provide strong confidence that the resolver’s parallel logic works as intended.src/resolution/composite-arns-resolver.ts (3)
105-108: Consider making the timeout duration injectible
Using a configurable timeout (e.g., passing it as a constructor parameter or retrieving fromoverrides) can facilitate testing and improve flexibility. This is especially relevant for controlling thepTimeoutbehavior in different environments.
77-116: Informative separation of resolution logic
The newresolveWithResolvermethod neatly encapsulates individual resolution handling, including caching and timeouts. This separation of concerns improves readability and modularity, making the codebase easier to maintain.
122-147: Robust parallel resolution approach
This implementation of parallel resolution with a concurrency limit (this.limit) offers a good balance between performance and resource control. UsingPromise.anyensures early return on the first successful resolution. This is a clean and effective solution.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/resolution/composite-arns-resolver.test.ts (1)
52-76:⚠️ Potential issuePotential mismatch between test name and implementation.
The test is named "should return early when first resolver succeeds" but it's actually asserting that both resolvers are called (line 74). If the intent is early return, the second resolver shouldn't be called. This seems to contradict the parallel resolution approach implemented in the code.
Either:
- Rename the test to reflect parallel resolution behavior, or
- Update assertions to match early return behavior:
- assert.strictEqual((resolver2.resolve as any).mock.calls.length, 1); + assert.strictEqual((resolver2.resolve as any).mock.calls.length, 0);
🧹 Nitpick comments (1)
src/resolution/composite-arns-resolver.ts (1)
52-257: Consider adding a unit test for cache expiration race condition.While the test coverage is generally excellent, I don't see a specific test case for the race condition scenario where a cached resolution expires while a new resolution is still pending. This edge case at lines 225-235 could benefit from explicit testing.
Consider adding a test case that:
- Has an expired cache entry
- Simulates a pending resolution that takes longer than expected
- Verifies the fallback to the cached resolution via pTimeout
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/resolution/composite-arns-resolver.test.ts(1 hunks)src/resolution/composite-arns-resolver.ts(8 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/resolution/composite-arns-resolver.ts (1)
src/types.d.ts (1)
NameResolver(685-693)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test (macos-latest)
- GitHub Check: test (ubuntu-latest)
🔇 Additional comments (14)
src/resolution/composite-arns-resolver.test.ts (6)
37-50: Test setup looks good.Well-structured test setup with proper mocking of resolution and registry caches. The beforeEach hook ensures tests start with clean mocks.
78-103: Test for fallback resolution looks good.This test correctly verifies that if the first resolver fails, the second resolver will be attempted. The assertions and test setup are appropriate.
105-138: Caching test is well-implemented.The test correctly verifies that when a valid cached resolution is available, the resolver is not called. Good use of mock.method to simulate cache behavior.
140-165: Timeout handling test looks good.This test effectively verifies that the resolver handles timeouts correctly by setting up a resolver that takes longer than the timeout period, and checking that the second resolver is called and its result is returned.
167-200: Test for all-resolvers-failing scenario is comprehensive.This test properly verifies that when all resolvers fail, the method returns an undefined resolution with the correct structure.
202-257: Excellent concurrency limit test.This test effectively verifies the core functionality of this PR - controlling the number of concurrent resolution attempts. It tracks both the maximum active resolutions and ensures all resolvers are eventually called.
src/resolution/composite-arns-resolver.ts (8)
20-20: Good choice using pLimit for concurrency control.Adding pLimit is appropriate for managing concurrent operations with a controlled limit.
44-45: Well-defined private members for the concurrency control.Adding private members for the concurrency limit and timeout is a clean approach.
54-56: Good constructor parameters with sensible defaults.The new parameters for concurrency control have appropriate defaults, improving usability while maintaining flexibility.
Also applies to: 65-67
77-79: Clean initialization of the concurrency control.The initialization of pLimit with the configured maxConcurrentResolutions is well implemented.
81-120: Well-structured resolver method with proper error handling.The
resolveWithResolvermethod effectively:
- Handles resolution for a single resolver with appropriate caching
- Implements timeout handling (with special handling for the last resolver)
- Provides comprehensive error handling
The approach of letting the last resolver run without a timeout is a good choice to ensure resolution attempts complete even when earlier ones time out.
122-153: Excellent parallel resolution implementation.The
resolveParallelmethod is well implemented:
- Maps over resolvers to create resolution promises
- Applies concurrency limit through pLimit
- Uses Promise.any to resolve with the first successful result
- Includes proper error handling
The use of Promise.any is particularly appropriate for this use case, as it allows for obtaining the first successful resolution.
211-214: Background resolution updated to use parallel approach.The update to use the new parallel resolution method for background cache refreshes is consistent with the overall approach.
225-228: Main resolution logic updated to use parallel approach.The update to the main resolution logic ensures pending resolutions use the new parallel approach.
| @@ -187,7 +222,10 @@ export class CompositeArNSResolver implements NameResolver { | |||
| this.log.info('Cache miss for arns name', { name }); | |||
|
|
|||
| // Ensure that we always use pending resolutions | |||
| this.pendingResolutions[name] ||= resolveName(); | |||
| this.pendingResolutions[name] ||= this.resolveParallel( | |||
There was a problem hiding this comment.
local testing looks good, i forced CU resolution by setting TRUSTED_ARNS_GATEWAY_URL=https://__NAME__.bad-gateway.dev (which is an invalid gateway) and the result was leveraging the resolution from NETWORK_AO_CU_URL=https://cu.ardrive.io.
info: Resolving name... {"class":"CompositeArNSResolver","name":"ao","overrides":{},"timestamp":"2025-03-26T12:59:09.141Z"}
info: Cache miss for arns name {"class":"CompositeArNSResolver","name":"ao","timestamp":"2025-03-26T12:59:09.141Z"}
info: Resolving name... {"class":"TrustedGatewayArNSResolver","name":"ao","timestamp":"2025-03-26T12:59:09.141Z"}
info: Resolving name... {"antCuUrl":"<sdk default>","class":"OnDemandArNSResolver","name":"ao","networkCuUrl":"https://cu.ardrive.io","timestamp":"2025-03-26T12:59:09.143Z"}
warn: Unable to resolve name: getaddrinfo ENOTFOUND ao.bad-gateway.dev {"class":"TrustedGatewayArNSResolver","name":"ao","timestamp":"2025-03-26T12:59:09.181Z"}
info: Resolved name {"class":"CompositeArNSResolver","name":"ao","resolution":{"index":0,"limit":100,"name":"ao","processId":"HY021r2MQL9Zi0qSNFAQ9QRshIc2mNPYf65pZBP04cE","resolvedAt":1742993950038,"resolvedId":"BhzUE-U8GNvT31tyxaNM6hwipbungh-uBMrzjCAcpOk","ttl":3600},"timestamp":"2025-03-26T12:59:10.038Z"}Inversely, I set the NETWORK_AO_CU_URL to the FWD CU - which frequently times out - and ArNS resolution leaned more heavily on the TRUSTED_ARNS_GATEWAY_URL 👏
info: Resolving name... {"class":"CompositeArNSResolver","name":"ao","overrides":{},"timestamp":"2025-03-26T13:01:21.733Z"}
info: Cache miss for arns name {"class":"CompositeArNSResolver","name":"ao","timestamp":"2025-03-26T13:01:21.733Z"}
info: Resolving name... {"class":"TrustedGatewayArNSResolver","name":"ao","timestamp":"2025-03-26T13:01:21.734Z"}
info: Resolving name... {"antCuUrl":"<sdk default>","class":"OnDemandArNSResolver","name":"ao","networkCuUrl":"<sdk default>","timestamp":"2025-03-26T13:01:21.738Z"}
info: Resolved name {"class":"TrustedGatewayArNSResolver","name":"ao","nameUrl":"https://ao.arweave.net","resolvedId":"BhzUE-U8GNvT31tyxaNM6hwipbungh-uBMrzjCAcpOk","timestamp":"2025-03-26T13:01:21.881Z","ttl":3600}
info: Resolved name {"class":"CompositeArNSResolver","name":"ao","resolution":{"index":0,"limit":100,"name":"ao","processId":"HY021r2MQL9Zi0qSNFAQ9QRshIc2mNPYf65pZBP04cE","resolvedAt":1742994081881,"resolvedId":"BhzUE-U8GNvT31tyxaNM6hwipbungh-uBMrzjCAcpOk","ttl":3600},"timestamp":"2025-03-26T13:01:21.881Z"}
| this.log.error('Error resolving name with resolver', { | ||
| resolver: resolver.constructor.name, | ||
| message: error.message, | ||
| stack: error.stack, |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/resolution/composite-arns-resolver.ts (2)
81-131: Well-structured resolver implementation with proper error handling.The
resolveWithResolvermethod effectively:
- Properly wraps individual resolver calls with error handling
- Manages timeouts appropriately with special handling for the last resolver
- Caches successful resolutions
- Includes detailed logging with the resolver name as requested in previous feedback
Consider adding JSDoc comments to document this method's parameters and return value.
+/** + * Attempts to resolve a name using a specific resolver with timeout control. + * @param resolver - The resolver to use for name resolution + * @param name - The name to resolve + * @param baseArNSRecordFn - Function to retrieve the base ArNS record + * @param isLastResolver - Whether this is the last resolver in the chain (skips timeout) + * @returns Resolved name or undefined if resolution failed + */ private async resolveWithResolver( resolver: NameResolver, name: string, baseArNSRecordFn: () => Promise<any>, isLastResolver: boolean, ): Promise<NameResolution | undefined> {
133-166: Good implementation of parallel resolution orchestration.The
resolveParallelmethod effectively:
- Creates parallel resolver promises with controlled concurrency via
pLimit- Uses
Promise.anyto race resolvers and return the first successful result- Includes comprehensive error handling and logging
- Now includes resolver names in error logs as requested in previous feedback
Consider adding JSDoc comments to document this method's parameters and return value.
+/** + * Attempts to resolve a name using multiple resolvers in parallel with controlled concurrency. + * @param name - The name to resolve + * @param baseArNSRecordFn - Function to retrieve the base ArNS record + * @returns The first successful resolution or undefined if all resolvers failed + */ private async resolveParallel( name: string, baseArNSRecordFn: () => Promise<any>, ): Promise<NameResolution | undefined> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docker-compose.yaml(1 hunks)docs/envs.md(1 hunks)src/config.ts(1 hunks)src/metrics.ts(1 hunks)src/resolution/composite-arns-resolver.ts(9 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/resolution/composite-arns-resolver.ts (1)
src/types.d.ts (1)
NameResolver(685-693)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test (macos-latest)
- GitHub Check: test (ubuntu-latest)
🔇 Additional comments (11)
docker-compose.yaml (1)
92-92: Environment variable added for parallel resolution control.This addition of the
ARNS_MAX_CONCURRENT_RESOLUTIONSenvironment variable is well structured and follows the established pattern in the file. The default empty value allows for flexible configuration at runtime.docs/envs.md (1)
73-73: Documentation updated for the new environment variable.The documentation for
ARNS_MAX_CONCURRENT_RESOLUTIONSmatches the implementation and provides clear information about its purpose. The default value is properly explained as being equal to the number of ArNS resolvers.src/config.ts (1)
566-572: Implementation of concurrency control constants for ArNS resolvers.The implementation of these constants follows the established pattern in the file for environment variables. Using
parseIntto convert the string to a number is appropriate, and the fallback toundefinedallows for flexible configuration.src/metrics.ts (1)
259-263: Added metrics for tracking resolver resolution counts.This new counter metric will help monitor and analyze the performance of different resolvers. The inclusion of the 'resolver' label allows for distinguishing between different resolver implementations.
This metric will be valuable for:
- Tracking which resolvers are handling the most resolutions
- Analyzing the distribution of work across resolvers
- Identifying potential bottlenecks or underutilized resolvers
src/resolution/composite-arns-resolver.ts (7)
20-20: Good addition of pLimit for concurrency control.The import of
pLimitis appropriate for implementing parallel name resolution with controlled concurrency.
44-45: LGTM: Good private member additions for concurrency management.The new private members properly support the parallel resolution feature:
limitmanages concurrent resolution operationsresolverTimeoutMsstores the timeout configuration
54-56: Well-implemented constructor parameter enhancements.The constructor now properly:
- Accepts configurable concurrency limits and timeout values
- Falls back to sensible defaults from the config
- Initializes the concurrency limit with the number of resolvers if not specified
This addresses previous feedback about making the timeout injectable.
Also applies to: 65-67, 77-79
224-227: LGTM: Updated background resolution to use parallel approach.The background resolution now uses the new parallel resolution method, which should improve performance and reliability.
238-241: Clean implementation of the pending resolution logic with parallel processing.The update to use
resolveParallelhere maintains the existing functionality while leveraging the new concurrent resolution capability.
192-192: Minor improvement: Property name consistency in error logs.Nice job adding the base name to the error log for better debugging.
160-162: Good job addressing previous feedback on error logging.You've correctly added
resolvers: this.resolvers.map(r => r.constructor.name)to the error logs as suggested in previous comments. This will make debugging resolution issues much easier.
| this.log.info('Resolved name', { | ||
| name, | ||
| resolution, | ||
| resolvedBy: resolver.constructor.name, | ||
| }); |
There was a problem hiding this comment.
👏
core-1 | info: Resolved name {"class":"CompositeArNSResolver","name":"intro-flex","resolution":{"index":0,"limit":100,"name":"intro-flex","processId":"kMaeETcxzxASw9620rI2q1ZR5Tp4hciXTrzysNjzdUo","resolvedAt":1743165953979,"resolvedId":"Tyfqts7e4uzlYY5NAtSiEQSGuXjWo3MD2wZu_FMLh4Q","ttl":900},"resolvedBy":"TrustedGatewayArNSResolver","timestamp":"2025-03-28T12:45:53.979Z"}
|
My mistake, I expected to see this be more weighted towards the |
No description provided.