Fix thread-safety crashes during onboarding #3471
Conversation
Per NIP-18, quote posts use q tags: ["q", "<event-id>", "<relay-url>", "<pubkey>"]. Third-party Nostr clients may only include the q tag without a separate p tag for the quoted note's author. This causes Damus to miss quote notifications because the existing notification filter only uses #p (pubkey) matching. This fix adds quote notification detection by: 1. Loading our quotable note IDs from nostrdb at startup (text, longform, highlight) 2. Adding a second notification filter using #q with our note IDs 3. Updating handle_notification() to accept events that either: - Reference our pubkey in a p tag (existing behavior) - OR quote one of our notes via a q tag (new behavior) 4. Tracking new notes we post during the session 5. Refreshing the notification subscription (debounced) when we post new notes so quotes of newly posted notes are also detected Key implementation details: - quotableKinds: Defines which event kinds can be quoted (text, longform, highlight) - subscribe_to_notifications(): Extracted method that can be called to refresh the subscription when our note IDs change - notifications_resub_debouncer: Prevents excessive resubscription when posting multiple notes in quick succession - our_note_ids_order: Tracks insertion order for FIFO eviction when cap exceeded - maxOurNoteIds: Caps set at 1000 to prevent unbounded growth during long sessions The implementation uses nevernesting with guard clauses for clean control flow. Changelog-Fixed: Fixed missing quote notifications from third-party clients Closes damus-io#3447 Closes damus-io#3449 Closes damus-io#3450 Signed-off-by: alltheseas 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Adds QuoteNotificationTests.swift with 5 tests to verify: 1. testEventHasOurPubkey - Standard p-tag detection still works 2. testEventDoesNotHaveOurPubkey - Events without our pubkey are rejected 3. testQuoteEventReferencesOurNote - Quote events with q-tag are detected 4. testQuoteEventDoesNotReferenceOurNote - Unrelated quotes are rejected 5. testCombinedNotificationRelevanceCheck - Both p-tag and q-tag paths work These tests ensure the quote notification fix works correctly and prevents regression. The tests verify the core validation logic that determines whether an event is relevant as a notification. Note: These tests cover the event validation helpers (event_has_our_pubkey, referenced_quote_ids) but not the subscription wiring (subscribe_to_notifications). Integration testing of subscription refresh would require a more complex test harness with mock network components. Closes damus-io#3447 Closes damus-io#3449 Closes damus-io#3450 Signed-off-by: alltheseas 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
alltheseas
added
onboarding
crash
bug
|
@coderabbitai can you please review this PR |
|
@coderabbitai review |
danieldaquino
added
the
pr-active-review
alltheseas
force-pushed
the
fix-onboarding-crash
branch
4 times, most recently
from
2414dca to
612a78d
Compare
|
@danieldaquino addressed one review comment generated via coderabbit, and ended up finding another bug in follow all not working. The last two commits here should be squashed versions of the PR originally submitted, and the recent updates. Docstrings were added throughout this code. |
- Make Contacts class thread-safe with actor-style synchronization - Make PostBox event sending thread-safe with proper queuing - Add comprehensive tests for Contacts class - Fix @mainactor isolation in test setup code 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Add handle_follow_multiple for batch follow operations - Add follow_multiple_references to create single contact event for multiple follows - Fix return value to accurately reflect operation success - Update onboarding to use batch follow instead of individual notifications This prevents race conditions when following many users at once during onboarding. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
alltheseas
force-pushed
the
fix-onboarding-crash
branch
from
612a78d to
66cf167
Compare
|
Update: Fixed return value in handle_follow_multiple Addressed the CodeRabbit feedback about the misleading return value: Before: The function always returned true even when follow_multiple_references failed, making the return value meaningless. After: Returns false when event creation fails |
|
Thank you @alltheseas! The root cause seems to be correct, this is good find! However, the solution uses I will try making a rewrite based on your PR on Monday, but using Swift Structured Concurrency instead. Please let me know if you beat me to it 😅. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis PR introduces batch-follow capability to prevent crashes during mass-follow operations, adds thread-safe state management to the Contacts class with NSLock protection, implements quote-based notifications by tracking our note IDs, and expands PostBox with retry/delayed-send logic and comprehensive relay management. Changes
Sequence Diagram(s)sequenceDiagram
actor UI as UI (SuggestedUsers)
participant CV as ContentView
participant Contacts as Contacts+
participant PB as PostBox
participant Pool as RelayPool
UI->>CV: handle_follow_multiple([pubkey1, pubkey2, ...])
activate CV
CV->>Contacts: follow_multiple_references(pubkeys)
activate Contacts
Contacts->>Contacts: follow_multiple_users_event(creates<br/>single contacts event)
Contacts->>PB: send(event, to: relays)
activate PB
PB->>PB: setEventIfAbsent(atomically queue)
PB->>Pool: dispatch to relays
Pool-->>PB: acknowledge (ok/failed)
deactivate PB
PB-->>Contacts: NostrEvent (success)
deactivate Contacts
Contacts-->>CV: event
CV->>CV: for each pubkey:<br/>add_friend_pubkey()<br/>notify(.followed)
CV-->>UI: true (success)
deactivate CV
sequenceDiagram
participant Relay as Relay
participant Pool as RelayPool
participant HM as HomeModel
participant Filter as NotificationFilter
Relay->>Pool: incoming event (kind: 1 quote)
Pool->>HM: handle relay event
activate HM
Note over HM: Check if event<br/>matches filters
alt Has our pubkey in p-tag
HM->>HM: event_has_our_pubkey()
HM->>HM: is_notification = true
else Has our note ID in q-tag
HM->>HM: matches our_note_ids
HM->>HM: is_notification = true
else Other
HM->>HM: is_notification = false
end
HM-->>Pool: process (or skip)
deactivate HM
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
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: 0
🧹 Nitpick comments (4)
damusTests/NostrNetworkManagerTests/ThreadModelTests.swift (1)
27-30: Consider adding@MainActorfor consistency.For symmetry with
setUpWithError()and to ensure consistent actor isolation throughout the test lifecycle, consider annotatingtearDownWithError()with@MainActoras well. While settingdamusStatetonilmay not strictly require main-actor execution, maintaining consistent isolation contexts for setup and teardown is a best practice.🔎 Suggested change
+ @MainActor override func tearDownWithError() throws { // Put teardown code here. This method is called after the invocation of each test method in the class. damusState = nil }damus/Features/Follows/Models/Contacts.swift (1)
57-65: Consider optimizing the friend removal loop.The current implementation iterates all entries in
pubkey_to_our_friendsto remove the pubkey from each set. While correct, this is O(n) where n is the number of entries.For a potential future optimization (if performance becomes an issue with large contact lists), consider maintaining a reverse index of which pubkeys a friend appears in.
damus/Features/Posting/Models/PostBox.swift (2)
230-230: Remove redundant nil initialization.As flagged by SwiftLint, optional variables are implicitly
nilwhen declared.🔎 Proposed fix
- var callbackToInvoke: ((PostedEvent) -> Void)? = nil + var callbackToInvoke: ((PostedEvent) -> Void)?
38-63: Consider future migration to Swift actors.As noted by reviewer @danieldaquino, Swift Structured Concurrency (actors) would provide compile-time safety for this concurrent state management. The current NSLock approach is correct and well-implemented, but actors would eliminate the possibility of forgetting to acquire the lock.
This is a good candidate for future refactoring when the codebase migrates more fully to structured concurrency.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
F2.pngis excluded by!**/*.pngFollowallcrash.pngis excluded by!**/*.png
📒 Files selected for processing (12)
damus.xcodeproj/project.pbxprojdamus/ContentView.swiftdamus/Features/Follows/Models/Contacts+.swiftdamus/Features/Follows/Models/Contacts.swiftdamus/Features/Onboarding/SuggestedUsersViewModel.swiftdamus/Features/Posting/Models/PostBox.swiftdamus/Features/Timeline/Models/HomeModel.swiftdamusTests/ContactsTests.swiftdamusTests/Mocking/MockDamusState.swiftdamusTests/NostrNetworkManagerTests/NostrNetworkManagerTests.swiftdamusTests/NostrNetworkManagerTests/ThreadModelTests.swiftdamusTests/QuoteNotificationTests.swift
👮 Files not reviewed due to content moderation or server errors (1)
- damus.xcodeproj/project.pbxproj
🧰 Additional context used
🧬 Code graph analysis (6)
damusTests/ContactsTests.swift (1)
damus/Features/Follows/Models/Contacts.swift (7)
get_friend_list(69-71)add_friend_pubkey(96-100)is_friend(141-143)remove_friend(57-65)is_friend_or_self(148-150)follow_state(155-157)follows(89-92)
damus/ContentView.swift (5)
damus/Features/Follows/Models/Contacts.swift (2)
follows(89-92)add_friend_pubkey(96-100)damus/Core/Nostr/Mentions.swift (2)
pubkey(30-32)pubkey(183-185)damus/Features/Follows/Models/Contacts+.swift (1)
follow_multiple_references(105-112)damus/Notify/Notify.swift (1)
notify(34-39)damus/Notify/FollowedNotify.swift (1)
followed(22-24)
damusTests/QuoteNotificationTests.swift (2)
damus/Features/Timeline/Models/HomeModel.swift (1)
event_has_our_pubkey(1371-1373)damus/Features/Chat/Models/ThreadModel.swift (1)
contains(270-272)
damus/Features/Onboarding/SuggestedUsersViewModel.swift (1)
damus/ContentView.swift (1)
handle_follow_multiple(1082-1108)
damus/Features/Follows/Models/Contacts.swift (5)
damus/ContentView.swift (1)
event(979-981)damusTests/ContactsTests.swift (1)
latest_contact_event_changed(288-291)damus/Features/Timeline/Models/HomeModel.swift (1)
latest_contact_event_changed(369-372)nostrdb/ccan/ccan/htable/tools/density.c (1)
key(14-17)damus/Features/Chat/Models/ThreadModel.swift (1)
contains(270-272)
damus/Features/Posting/Models/PostBox.swift (1)
damus/Core/Nostr/RelayPool.swift (3)
get_relay(450-453)send(440-442)handle_event(498-546)
🪛 SwiftLint (0.57.0)
damus/Features/Posting/Models/PostBox.swift
[Warning] 230-230: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
🔇 Additional comments (30)
damusTests/NostrNetworkManagerTests/ThreadModelTests.swift (1)
15-25: LGTM! Correctly fixes @mainactor isolation.The
@MainActorannotation onsetUpWithError()properly addresses the main-actor isolation requirement forgenerate_test_damus_state()and other test setup code. This aligns with the PR's objective to fix @mainactor isolation errors in test setup.damusTests/Mocking/MockDamusState.swift (1)
13-18: LGTM!The
@MainActorannotation correctly ensures test setup runs on the main actor, fixing the isolation errors mentioned in the PR objectives. This aligns with similar changes in other test files.damus/Features/Timeline/Models/HomeModel.swift (6)
86-101: Well-designed tracking structure for quote notification detection.The combination of a
Setfor O(1) lookups and an array for FIFO eviction order is a good pattern. The 1000-entry cap reasonably bounds memory usage and relay filter sizes.
215-244: LGTM!The implementation correctly queries nostrdb for our quotable notes and caches them for efficient lookup. The borrow pattern for note lookup is efficient.
254-270: LGTM!The FIFO eviction logic is correct, and the debounced resubscription prevents excessive churn when posting multiple notes in quick succession.
288-343: LGTM!The notification subscription correctly handles both standard p-tag notifications and quote notifications via q-tag. The documented brief gap during subscription refresh is acceptable given the mitigations (events stored in nostrdb, debounced refresh minimizes window).
920-944: LGTM!The dual relevance check correctly handles both traditional p-tag notifications and NIP-18 quote notifications where third-party clients may only include a q-tag. The documentation clearly explains the rationale.
984-996: LGTM!Tracking our notes as they arrive through the subscription ensures quote notification detection stays current for notes posted during the session.
damusTests/NostrNetworkManagerTests/NostrNetworkManagerTests.swift (1)
15-16: LGTM!The
@MainActorannotation is required sincegenerate_test_damus_stateis now annotated with@MainActor. This correctly fixes the test isolation errors.damus/Features/Onboarding/SuggestedUsersViewModel.swift (1)
89-95: LGTM!Switching to
handle_follow_multiplefor batch processing correctly addresses the race condition issue during "follow all" operations. The async Task dispatch is appropriate for calling from a synchronous context.damusTests/ContactsTests.swift (4)
12-88: LGTM!Comprehensive basic functionality tests with clear Given/When/Then structure. Good coverage of the Contacts class API.
90-161: Excellent thread-safety regression tests.These concurrent access tests directly validate the fix for the "Follow All" crash. The stress testing with 100+ concurrent operations and mixed read/write patterns provides good confidence in the NSLock-based synchronization.
163-240: LGTM!Good coverage of concurrent access to the
eventproperty andfriend_filterclosure. These tests complement the basic functionality tests by ensuring thread-safety across the full Contacts API surface.
242-292: LGTM!Good delegate behavior tests ensuring proper notification semantics. The nil event case correctly validates that the delegate isn't notified unnecessarily.
damus/ContentView.swift (1)
1076-1108: LGTM!The batch follow implementation correctly:
- Creates a single combined event for all follows
- Updates
state.contacts.eventbefore local state changes- Updates local friend state and emits notifications only after successful event creation
This atomicity prevents the race conditions that occurred when firing separate follow notifications rapidly.
damusTests/QuoteNotificationTests.swift (3)
20-81: LGTM!Good foundational tests verifying the
event_has_our_pubkeyhelper works correctly for both positive and negative cases.
83-152: LGTM!These tests directly validate the NIP-18 quote notification edge case where third-party clients only include a q-tag without a p-tag. The assertion on line 116 confirms this scenario is correctly handled.
154-215: LGTM!Comprehensive test covering all four notification relevance scenarios. The
isRelevantNotificationhelper correctly mirrors the dual-check logic inhandle_notification, ensuring test fidelity.damus/Features/Follows/Models/Contacts+.swift (2)
97-112: LGTM! Clean batch-follow helper with good documentation.The function correctly mirrors the single-follow pattern in
follow_reference, delegating event construction tofollow_multiple_users_eventand handling the async send cleanly.
120-151: LGTM! Batch event construction handles duplicates correctly.The logic properly:
- Guards against nil contacts to prevent nuking the contact list
- Skips already-followed references using
is_already_following- Prevents duplicates within the same batch via
tags.contains- Returns nil when nothing was added, avoiding unnecessary event creation
damus/Features/Follows/Models/Contacts.swift (5)
16-34: LGTM! Thread-safe property accessors with proper lock usage.The lock-based accessors for
_eventand_delegatecorrectly protect concurrent access. Usinglock.withLockensures automatic unlock even on early returns.
36-49: Good deadlock prevention pattern for delegate notification.Capturing the delegate and event inside the lock, then invoking the callback outside, prevents potential deadlocks if the delegate implementation tries to access Contacts. This is the correct approach when using NSLock.
104-120: LGTM! Friend contact addition is correctly synchronized.All mutations to
friends,friend_of_friends, andpubkey_to_our_friendsoccur within a single lock acquisition, ensuring atomicity of the compound operation.
166-173: LGTM! Friend filter closure is thread-safe.The closure correctly uses
[weak self]to avoid retain cycles and delegates tois_friend()which handles its own locking.
81-92: No changes needed—the thread-safety pattern is correct and safe.The code's design is sound:
get_followed_hashtags()andfollows(hashtag:)acquire the lock to safely retrieve a reference toself.event, then access its immutable properties after releasing the lock. SinceNostrEvent(aliased toNdbNotefrom the Nostr-ndb library) treats events as immutable records—guaranteed by cryptographic signing—accessing computed properties likereferenced_hashtagswithout holding the lock is safe.damus/Features/Posting/Models/PostBox.swift (5)
75-81: LGTM! Good use of immutable snapshot for network operations.
FlushWorkItemcorrectly captures all data needed for network sends as a value type, enabling safe execution outside the lock.
102-110: LGTM! Atomic check-and-insert prevents duplicate event queueing.This pattern correctly prevents race conditions where multiple callers might try to queue the same event simultaneously.
152-193: LGTM! Correct lock-then-send pattern for flushing events.The implementation correctly:
- Builds a complete work list under the lock (including mutating retry state)
- Performs all network I/O outside the lock
- Uses exponential backoff for retry timing
217-259: LGTM! Callback execution outside lock prevents deadlocks.The pattern of capturing callback info under lock and executing outside is correctly applied here, mirroring the approach in
Contacts.swift.
305-321: LGTM! Send method correctly handles both immediate and delayed dispatch.The atomic insert via
setEventIfAbsentcombined with the conditional flush provides clean semantics for both use cases.
I did the rewrite here: #3503 I also noticed the partial "follow all" fix. I was going to apply it on #3503, but I ran into issues with that when testing. Apparently if someone from the list was already being followed, the "follow all" button does not seem to work with that commit. I will keep this PR open (or maybe split the PR) so that we can take a look at that separately. @alltheseas were the quote notification commits accidentally included in this PR from a separate PR? Or were those commits intentionally included in this PR? |
|
should resubmit the changes Daniel mentioned, don't need to keep this open |
3 participants
Summary
Contactsduring the onboarding "follow all" flowPostBoxwhen sending events concurrently@MainActorisolation errors in test setup codeBoth
ContactsandPostBoxwere being accessed from multiple async contexts without synchronization, causing intermittent crashes during onboarding when users tapped "follow all".Checklist
Standard PR Checklist
Closes:orFixes:tags in the commit messages wherever applicable, or made sure those are not needed. See Submitting patchesTest report
Device: iPhone 17 Pro Simulator
iOS: iOS 26
Damus: 992a2d6 (this PR)
Setup: Fresh install, new account creation
Steps:
Results:
Other notes
The fix uses
NSLockwith thewithLockpattern for thread-safe access to shared mutable state. Network operations and callbacks are executed outside the lock to avoid blocking and potential deadlocks.Closes #3422
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.