fix: include target relays and request lifetime in the request dedup key - #705
fix: include target relays and request lifetime in the request dedup key#705nogringo wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe concurrency key now represents the complete request, including relay configuration and lifecycle behavior. New scenarios test relay isolation and subscription-cache interactions. One local-first deletion test is skipped because cache visibility remains incorrect when ChangesRequest concurrency and cache behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/ndk/lib/domain_layer/usecases/requests/concurrency_check.dart`:
- Around line 45-58: Update the JSON payload used by the concurrency-check
deduplication hash to include stable representations of request.timeoutDuration
and request.desiredCoverage alongside the existing filters and relay fields.
Keep the existing hashing flow and ordering behavior unchanged so queries
differing in either value produce distinct keys.
In
`@packages/ndk/test/scenarios/concurrent_same_filter_different_relays_test.dart`:
- Around line 63-72: Strengthen the assertions in the concurrent query test so
each result contains only its target relay’s event: update the checks for
results[0] and results[1] to compare their complete ID sets against
noteOnRelay1.id and noteOnRelay2.id respectively, excluding the other relay
event.
In `@packages/ndk/test/usecases/local_first/local_first_test.dart`:
- Around line 593-598: The cacheRead=true, cacheWrite=false query path is
returning tombstoned events because visibility filtering is skipped. Update the
cache visibility/filtering logic used by this query path to exclude tombstoned
events, then remove the skip from the tombstone-visibility test so the scenario
remains enabled.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0bca4f76-3686-437a-9a7c-393bfcfaa0d0
📒 Files selected for processing (4)
packages/ndk/lib/domain_layer/usecases/requests/concurrency_check.dartpackages/ndk/test/scenarios/concurrent_same_filter_different_relays_test.dartpackages/ndk/test/scenarios/subscription_cache_read_dedup_test.dartpackages/ndk/test/usecases/local_first/local_first_test.dart
| final jsonString = json.encode({ | ||
| 'filters': request.filters, | ||
| 'closeOnEOSE': request.closeOnEOSE, | ||
| 'explicitRelays': _sorted(request.explicitRelays), | ||
| 'relaySet': request.relaySet == null | ||
| ? null | ||
| : { | ||
| 'id': request.relaySet!.id, | ||
| 'urls': _sorted(request.relaySet!.urls), | ||
| }, | ||
| }); | ||
| final bytes = utf8.encode(jsonString); | ||
| final hash = sha256.convert(bytes); | ||
| return hash.toString(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Include timeout and coverage in the deduplication key.
timeoutDuration and desiredCoverage are omitted from the key. Two queries with the same filters and relays but different values can share one request. The source request can then close before the longer query timeout, or collect fewer relays than the other query requires.
Add a stable representation of request.timeoutDuration and request.desiredCoverage to the encoded payload.
Proposed fix
'filters': request.filters,
'closeOnEOSE': request.closeOnEOSE,
+ 'timeoutDurationMicros': request.timeoutDuration?.inMicroseconds,
+ 'desiredCoverage': request.desiredCoverage,
'explicitRelays': _sorted(request.explicitRelays),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| final jsonString = json.encode({ | |
| 'filters': request.filters, | |
| 'closeOnEOSE': request.closeOnEOSE, | |
| 'explicitRelays': _sorted(request.explicitRelays), | |
| 'relaySet': request.relaySet == null | |
| ? null | |
| : { | |
| 'id': request.relaySet!.id, | |
| 'urls': _sorted(request.relaySet!.urls), | |
| }, | |
| }); | |
| final bytes = utf8.encode(jsonString); | |
| final hash = sha256.convert(bytes); | |
| return hash.toString(); | |
| final jsonString = json.encode({ | |
| 'filters': request.filters, | |
| 'closeOnEOSE': request.closeOnEOSE, | |
| 'timeoutDurationMicros': request.timeoutDuration?.inMicroseconds, | |
| 'desiredCoverage': request.desiredCoverage, | |
| 'explicitRelays': _sorted(request.explicitRelays), | |
| 'relaySet': request.relaySet == null | |
| ? null | |
| : { | |
| 'id': request.relaySet!.id, | |
| 'urls': _sorted(request.relaySet!.urls), | |
| }, | |
| }); | |
| final bytes = utf8.encode(jsonString); | |
| final hash = sha256.convert(bytes); | |
| return hash.toString(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ndk/lib/domain_layer/usecases/requests/concurrency_check.dart`
around lines 45 - 58, Update the JSON payload used by the concurrency-check
deduplication hash to include stable representations of request.timeoutDuration
and request.desiredCoverage alongside the existing filters and relay fields.
Keep the existing hashing flow and ordering behavior unchanged so queries
differing in either value produce distinct keys.
| expect( | ||
| results[0].map((e) => e.id), | ||
| contains(noteOnRelay1.id), | ||
| reason: 'query on relay 1 should return the event stored on relay 1', | ||
| ); | ||
| expect( | ||
| results[1].map((e) => e.id), | ||
| contains(noteOnRelay2.id), | ||
| reason: 'query on relay 2 should return the event stored on relay 2', | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that each result excludes the other relay event.
The current assertions only require the target event to be present. They also pass if both queries return events from both relays. Assert the complete ID set for each result.
Proposed fix
expect(
- results[0].map((e) => e.id),
- contains(noteOnRelay1.id),
+ results[0].map((e) => e.id),
+ unorderedEquals([noteOnRelay1.id]),
reason: 'query on relay 1 should return the event stored on relay 1',
);
expect(
- results[1].map((e) => e.id),
- contains(noteOnRelay2.id),
+ results[1].map((e) => e.id),
+ unorderedEquals([noteOnRelay2.id]),
reason: 'query on relay 2 should return the event stored on relay 2',
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect( | |
| results[0].map((e) => e.id), | |
| contains(noteOnRelay1.id), | |
| reason: 'query on relay 1 should return the event stored on relay 1', | |
| ); | |
| expect( | |
| results[1].map((e) => e.id), | |
| contains(noteOnRelay2.id), | |
| reason: 'query on relay 2 should return the event stored on relay 2', | |
| ); | |
| expect( | |
| results[0].map((e) => e.id), | |
| unorderedEquals([noteOnRelay1.id]), | |
| reason: 'query on relay 1 should return the event stored on relay 1', | |
| ); | |
| expect( | |
| results[1].map((e) => e.id), | |
| unorderedEquals([noteOnRelay2.id]), | |
| reason: 'query on relay 2 should return the event stored on relay 2', | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/ndk/test/scenarios/concurrent_same_filter_different_relays_test.dart`
around lines 63 - 72, Strengthen the assertions in the concurrent query test so
each result contains only its target relay’s event: update the checks for
results[0] and results[1] to compare their complete ID sets against
noteOnRelay1.id and noteOnRelay2.id respectively, excluding the other relay
event.
| skip: | ||
| 'Was green only because the last query got merged into the previous ' | ||
| 'one by ConcurrencyCheck, whose dedup key ignored the target relays. ' | ||
| 'With the key fixed the query runs for real and exposes that ' | ||
| 'visibility filtering is skipped when cacheWrite is false, so the ' | ||
| 'tombstoned event comes back. See the cacheWrite visibility issue.', |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not skip the tombstone-visibility scenario.
The skip reason confirms that a cacheRead: true, cacheWrite: false query returns a tombstoned event. This violates the public query contract tested above. Fix visibility filtering for this cache path, remove skip, and keep the scenario enabled.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ndk/test/usecases/local_first/local_first_test.dart` around lines
593 - 598, The cacheRead=true, cacheWrite=false query path is returning
tombstoned events because visibility filtering is skipped. Update the cache
visibility/filtering logic used by this query path to exclude tombstoned events,
then remove the skip from the tombstone-visibility test so the scenario remains
enabled.
|
address the issues coderabbit raised, since my clanker also point out similar stuff |
3 participants
Summary by CodeRabbit
Bug Fixes
Tests