Fix/fetched ranges recording race - #703
Open
nogringo wants to merge 4 commits into
Open
Conversation
📝 WalkthroughWalkthroughRequest processing records fetched ranges from network-delivered events after response completion. Range bounds use the oldest returned event. Integration tests cover limited queries and relay-capped responses. ChangesFetched range tracking
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #703 +/- ##
==========================================
+ Coverage 71.32% 71.35% +0.03%
==========================================
Files 225 225
Lines 13201 13198 -3
==========================================
+ Hits 9416 9418 +2
+ Misses 3785 3780 -5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1-leo
requested changes
Aug 8, 2026
![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
🤖 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/requests.dart`:
- Around line 582-602: Update the range-recording logic around the EOSE response
handling to avoid using callback-time DateTime.now() as an unbounded until
value. Track the newest received event per relay and use its created-at
timestamp for filters without until; for empty responses, reuse a fixed
request-dispatch cutoff captured when the request was sent. Add a
delayed-response test covering an event published after dispatch and verify gap
detection still reports it.
🪄 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: baea84e6-62ce-4542-835f-0acccfa75ce3
📒 Files selected for processing (2)
packages/ndk/lib/domain_layer/usecases/requests/requests.dartpackages/ndk/test/usecases/fetched_ranges/fetched_ranges_integration_test.dart
Comment on lines
582
to
+602
| final now = DateTime.now().millisecondsSinceEpoch ~/ 1000; | ||
|
|
||
| // Get all events from the replay subject | ||
| final events = state.controller.values.toList(); | ||
|
|
||
| // Group events by source relay | ||
| final eventsByRelay = <String, List<Nip01Event>>{}; | ||
| for (final event in events) { | ||
| for (final source in event.sources) { | ||
| eventsByRelay.putIfAbsent(source, () => []).add(event); | ||
| } | ||
| } | ||
|
|
||
| for (final entry in state.requests.entries) { | ||
| final relayUrl = entry.key; | ||
| final relayState = entry.value; | ||
|
|
||
| if (!relayState.receivedEOSE) continue; | ||
|
|
||
| final relayEvents = eventsByRelay[relayUrl]; | ||
| final oldestEvent = oldestEventByRelay[relayUrl]; | ||
|
|
||
| // Record fetched range for each filter sent to this relay | ||
| for (final filter in relayState.filters) { | ||
| int since; | ||
| int until; | ||
|
|
||
| if (relayEvents != null && relayEvents.isNotEmpty) { | ||
| // Use oldest event timestamp for since, filter.until or now for until | ||
| // EOSE means relay has no more events, so fetched range extends to query end | ||
| final timestamps = relayEvents.map((e) => e.createdAt).toList(); | ||
| since = timestamps.reduce((a, b) => a < b ? a : b); | ||
| until = filter.until ?? now; | ||
| } else if (filter.since != null || filter.until != null) { | ||
| // No events but filter has explicit bounds | ||
| since = filter.since ?? 0; | ||
| until = filter.until ?? now; | ||
| } else { | ||
| // No events, no bounds - relay has nothing, record 0 to now | ||
| since = 0; | ||
| until = now; | ||
| int since = filter.since ?? 0; | ||
| final int until = filter.until ?? now; | ||
|
|
||
| if (oldestEvent != null) { | ||
| // A relay can cap a response below the requested limit, or with no | ||
| // limit in the filter at all (NIP-11 max_limit, which we don't read), | ||
| // so a full response is indistinguishable from a truncated one. Only | ||
| // claim coverage down to the oldest event received. | ||
| since = oldestEvent; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not use response-completion time as an unbounded upper bound.
Line 582 captures now after the relay response has completed. For a filter without until, an event can be created after the relay evaluated the request but before this callback runs. The recorded range then claims that event was fetched. Later gap detection can skip the event.
Track the newest received event per relay and use it as until when the filter has no upper bound. For an empty response, use a fixed request-dispatch cutoff. Add a delayed-response test that publishes an event after request dispatch and verifies that it remains a gap.
🤖 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/requests.dart` around lines
582 - 602, Update the range-recording logic around the EOSE response handling to
avoid using callback-time DateTime.now() as an unbounded until value. Track the
newest received event per relay and use its created-at timestamp for filters
without until; for empty responses, reuse a fixed request-dispatch cutoff
captured when the request was sent. Add a delayed-response test covering an
event published after dispatch and verify gap detection still reports it.
1-leo
requested changes
Aug 8, 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
2 participants
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.
Summary by CodeRabbit
Bug Fixes
Tests