Fix join flow: skip stale groups in discovery + add sync fallback

[yewreeka]

Problem

Two issues causing join flow failures:

1. Scanner shows "New Convo" instead of joined conversation

After scanning a QR code and joining, the app would sometimes show an empty "New Convo" instead of the actual conversation. This happened because discoverNewConversations() in SyncingManager was re-introducing self-created single-member groups (from the unused conversation cache) back into the database after they had been cleaned up.

Fix: Filter out groups where creatorInboxId == client.inboxId && memberCount <= 1 in discoverNewConversations(). These are unused pre-created conversations that should not be re-processed.

2. Join gets stuck at "Verifying" indefinitely

The XMTP conversation stream (StreamConversations) does not call sync_welcomes() at startup — unlike StreamAllMessages which does. This means the stream cannot discover groups the client is added to after subscription. The joiner sends a join request, the host adds them, but the joiner's stream never delivers the new group.

The existing fallback (discoverNewConversations on app resume) is unreliable due to lifecycle edge cases where the SyncingManager misses the background/foreground transition.

Fix: Add requestDiscovery() to SyncingManager (syncs welcomes + discovers new groups on demand). In ConversationStateMachine.handleJoin, start a discovery loop alongside the DB observation that periodically calls requestDiscovery() with backoff (3s → 6s → 12s → 15s cap). The DB observation fires as soon as discoverNewConversations processes the group. The loop is cancelled on success.

Changes

• SyncingManager.swift — Filter self-created single-member groups in discoverNewConversations(), add requestDiscovery() to protocol and implementation
• InboxStateMachine.swift — Expose requestDiscovery()
• InboxStateManager.swift — Add requestDiscovery() to protocol and implementation
• ConversationStateMachine.swift — Add discovery loop in handleJoin, cancel in all exit paths
• Mock updates for protocol conformance

Testing

• Two-simulator test: fresh joiner joins via deep link in ~1.6s (happy path unaffected)
• Built and installed on physical device (iPhone 14 Pro)
• No errors or warnings in logs
• Unit tests pass, lint clean

Note

Run periodic discovery during invite join and skip self-created single-member groups, with ConversationStateMachine starting a 3s→15s exponential-backoff loop and SyncingManager filtering stale groups

Add a background discovery loop in ConversationStateMachine with exponential backoff (3s doubling to 15s) that cancels on completion or cleanup, introduce requestDiscovery through InboxStateManager to SyncingManager, and have SyncingManager skip self-created single-member groups while SessionManager stops services when InboxLifecycleManager.registerExternalService returns false.

📍Where to Start

Start at join handling and discovery loop setup in ConversationStateMachine.swift, then follow InboxStateManager.requestDiscovery to SyncingManager.requestDiscovery in InboxStateManager.swift and SyncingManager.swift.

^{Macroscope summarized 42fc68a.}

[claude]

Claude finished @yewreeka's task —— View job

---

Code Review Complete ✅

Reviewing PR #517: Fix join flow with sync fallback and stale group filtering

Todo List:

• Read and analyze changed files
• Review code quality and best practices
• Check for potential bugs or edge cases
• Evaluate performance considerations
• Assess security concerns
• Review test coverage
• Post comprehensive feedback

---

Summary

This PR addresses two legitimate bugs in the join flow with well-reasoned solutions. The changes are focused, maintain architectural integrity, and include appropriate cleanup in all exit paths. Overall quality is high with only minor suggestions for improvement.

---

Code Quality & Best Practices ✅

Strengths:

• Clean separation of concerns: SyncingManager handles discovery, ConversationStateMachine orchestrates the join flow
• Proper resource cleanup: discoveryTask?.cancel() in all exit paths (handleJoin success/error, handleDelete, cleanUpState, deinit)
• Well-documented code: Clear comments explain the XMTP stream limitation and backoff strategy
• Consistent error handling with CancellationError checks
• Follows project conventions (actor isolation, proper use of @unchecked Sendable)

Minor Suggestions:

1. ConversationStateMachine.swift:680-694 - Consider extracting the discovery loop to a named method for testability:

   private func startDiscoveryLoop() -> Task<Void, Never> {
       Task { [weak self] in
           var interval: Duration = .seconds(3)
           let maxInterval: Duration = .seconds(15)
           while !Task.isCancelled {
               // ... existing loop body
           }
       }
   }

2. SyncingManager.swift:446-449 - Magic number in filter could be a constant:

   private static let minMemberCountForDiscovery = 2
   
   if creatorInboxId == params.client.inboxId && memberCount < minMemberCountForDiscovery {

---

Potential Bugs & Edge Cases ⚠️

Medium Priority

1. Race condition in InboxLifecycleManager.registerExternalService (lines 357-376)

The method checks awakeInboxes[clientId] == nil twice with eviction in between. If another task registers the same service during eviction, both will proceed, potentially creating duplicate MessagingService instances.

Current flow:

guard awakeInboxes[clientId] == nil else { return false }  // Check 1
// ... eviction can happen here (async)
guard awakeInboxes[clientId] == nil else { return false }  // Check 2
awakeInboxes[clientId] = service

Recommendation: The PR already returns Bool to indicate success/failure. The caller (SessionManager.messagingServiceSync) should check the return value and call stop() on the duplicate service if registerExternalService returns false. This prevents the race from causing issues.

2. Discovery loop doesn't respect state changes

In ConversationStateMachine.handleJoin (lines 680-694), the discovery loop only checks Task.isCancelled. If the state machine transitions away from .joining (e.g., user navigates away), the loop continues until the DB observation completes or the task is cancelled.

Recommendation: Add a state guard:

discoveryTask = Task { [weak self] in
    var interval: Duration = .seconds(3)
    let maxInterval: Duration = .seconds(15)
    while !Task.isCancelled {
        try await Task.sleep(for: interval)
        try Task.checkCancellation()
        
        // Add state guard
        guard case .joining = await self?._state else {
            Log.debug("State changed from joining, stopping discovery loop")
            break
        }
        
        Log.info("Join flow: triggering discovery sync...")
        await self?.inboxStateManager.requestDiscovery()
        interval = min(interval * 2, maxInterval)
    }
}

Low Priority

3. discoverNewConversations filter may skip valid groups

The filter in SyncingManager.swift:446-449 skips groups where creatorInboxId == client.inboxId && memberCount <= 1. This assumes that:

• A self-created group with exactly 1 member is always stale
• A self-created group with 2+ members is always valid

Edge case: If a user creates a group and someone joins immediately (before discovery runs), the group could have memberCount == 1 at observation time but be a valid active group.

Recommendation: Add a timestamp check to only skip groups older than a threshold (e.g., created > 1 hour ago):

let createdAt = try await group.createdAtNs
let threshold = Date().addingTimeInterval(-3600).timeIntervalSince1970 * 1_000_000_000
if creatorInboxId == params.client.inboxId && memberCount <= 1 && createdAt < UInt64(threshold) {
    Log.debug("Skipping stale self-created single-member group: \(group.id)")
    continue
}

---

Performance Considerations ⚡

Strengths:

• Exponential backoff prevents excessive network calls (3s → 6s → 12s → 15s cap)
• Discovery loop is short-lived and only runs during active join attempts
• Early termination on successful DB observation minimizes wasted work

Observations:

1. Backoff cap of 15s - This is reasonable for the join flow. Users typically join and see results within 1-6 seconds according to the PR description.

2. Discovery overhead - Each requestDiscovery() call triggers:

   • syncAllConversations() (XMTP network call)
   • discoverNewConversations() (filters all groups)
   • Multiple DB queries

   This is acceptable for a one-time join operation but would be expensive if triggered frequently. The current usage pattern (only during active joins) is appropriate.

3. No max retry limit - The loop continues indefinitely until cancelled or DB observation fires. Consider adding a max duration:

   let maxDuration: Duration = .seconds(60)
   let startTime = ContinuousClock.now
   while !Task.isCancelled {
       if ContinuousClock.now - startTime > maxDuration {
           Log.warning("Discovery loop exceeded max duration, giving up")
           break
       }
       // ... rest of loop
   }

---

Security Concerns 🔒

No security issues identified. ✅

• No exposure of sensitive data
• Proper use of actor isolation for concurrent access
• Filter logic prevents processing of attacker-controlled conversation IDs
• No new authentication/authorization changes

---

Test Coverage 📋

Current Coverage:

• ✅ Author tested two-simulator join flow (1.6s happy path)
• ✅ Author tested on physical device (iPhone 14 Pro)
• ✅ Unit tests pass
• ✅ SwiftLint clean

Gaps:

1. No unit tests for requestDiscovery() plumbing - The new method was added to multiple layers but not tested:

   • SyncingManager.requestDiscovery()
   • InboxStateMachine.requestDiscovery()
   • InboxStateManager.requestDiscovery()

   Recommendation: Add a test verifying that ConversationStateMachine calls inboxStateManager.requestDiscovery() during join flow.

2. No tests for discovery filter logic - The filter in SyncingManager.discoverNewConversations (lines 446-449) that skips self-created single-member groups is not covered.

   Recommendation: Add a test case:

   func testDiscoverNewConversations_skipsSelfCreatedSingleMemberGroups() async throws {
       // Setup: Create a self-created group with 1 member in XMTP but not in local DB
       // Act: Call discoverNewConversations
       // Assert: Group was not written to database
   }

3. No tests for discovery loop cancellation - The loop in ConversationStateMachine.handleJoin should be tested for proper cleanup.

Test File Recommendations:

• Add ConversationStateMachineJoinTests.swift with:
  • Test discovery loop starts and stops correctly
  • Test discovery loop respects cancellation
  • Test DB observation fires before discovery completes
• Add SyncingManagerDiscoveryTests.swift with:
  • Test filter skips self-created single-member groups
  • Test filter processes multi-member groups
  • Test requestDiscovery() triggers sync + discovery

---

Additional Observations

1. Mock updates are correct ✅ - MockInboxStateManager and MockInboxLifecycleManager properly implement new protocol methods as no-ops.

2. InboxLifecycleManager change is orthogonal - The registerExternalService return value change is logically independent of the join flow fixes but is included here. This is fine for a small PR but consider separating in larger changes.

3. QA test doc - The addition of qa/tests/23-scheduled-explode.md appears unrelated to this PR (tests scheduled message exploding). Was this added intentionally?

---

Recommendations Summary

Must Address:

• None (code is production-ready as-is)

Should Address:

• Add state guard to discovery loop (prevents wasted work after state change)
• Add unit tests for discovery filter logic
• Consider max duration for discovery loop (prevents infinite retries)

Could Address:

• Extract discovery loop to named method for testability
• Add timestamp check to discovery filter for edge cases
• Add magic number constants

---

[macroscopeapp]

Re-evaluating approvability for d77ddb6

Dismissing my prior approval and re-evaluating approvability for 630481d

[macroscopeapp]

Re-evaluating approvability for 42fc68a

Dismissing my prior approval and re-evaluating approvability for 42fc68a