Nm/add logging

[neekolas]

Add info logging to xmtp_api_d14n::queries::v3::mls::client.send_group_messages and raise minimum recovery request version to 1.6.1

Add an info log in client.send_group_messages that records the group ID and cursor before issuing the query, and bump workspace and bindings versions to 1.6.1-rc1 along with updating the minimum recovery request version to 1.6.1.

📍Where to Start

Start with the logging insertion in xmtp_api_d14n/src/queries/v3/mls.rs, focusing on client.send_group_messages to see the new tracing::info! call and its context.

---

📊 Macroscope summarized 73e6582. 7 files reviewed, 4 issues evaluated, 4 issues filtered, 0 comments posted

🗂️ Filtered Issues

xmtp_api_d14n/src/protocol/in_memory_cursor_store.rs — 0 comments posted, 2 evaluated, 2 filtered

• line 68: latest now unconditionally calls panic!("using in memory store"), so any caller that legitimately requests the latest cursor from this store will crash the process instead of receiving the previously returned cursor (or a default value). This violates the trait contract and is a runtime bug. [ Already posted ]
• line 80: latest_per_originator now unconditionally panics with panic!("using in memory store"), so callers requesting per-originator cursors will crash instead of receiving the filtered cursor map the trait previously provided. This breaks the trait contract and is a runtime bug. [ Already posted ]

xmtp_db/src/encrypted_store/refresh_state.rs — 0 comments posted, 1 evaluated, 1 filtered

• line 653: latest_cursor_for_id now performs an unconditional SELECT … FROM refresh_state without any filters (lines 653-670) solely so it can log every row before running the real query. This pulls the entire table into memory and iterates/logs it every time the method is called, drastically increasing latency, memory usage, and log volume for large tables and making timeouts/OOMs realistic. The previous version only executed the filtered aggregate query. Please drop this full-table scan/logging or gate it behind debug-only tooling. [ Already posted ]

xmtp_mls/src/groups/welcomes/xmtp_welcome.rs — 0 comments posted, 1 evaluated, 1 filtered

• line 479: requires_processing relies on self.last_sequence_id(&db) to detect already processed welcomes (welcome.sequence_id() > self.last_sequence_id(&db)? as u64, line 307). self.last_sequence_id reads the sequence_id that we persist for the welcome-associated StoredGroupMessage. Prior to this change that field stored welcome.sequence_id(), so the guard correctly rejected duplicates. After the change (line 479) we persist the unrelated message_cursor from the metadata instead (defaulting to 0). For any already processed welcome replayed by the intent pipeline (e.g. when cursor_increment is false or the fetch is retried), welcome.sequence_id() will still be the large spool sequence, while self.last_sequence_id now returns the much smaller metadata cursor. The guard therefore evaluates to true and the duplicate welcome is processed again instead of returning ProcessIntentError::WelcomeAlreadyProcessed, causing duplicate side effects (extra worker events, consent updates, etc.). [ Low confidence ]

[graphite-app]

How to use the Graphite Merge Queue

Add the label mergequeue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

[claude]

Claude finished @neekolas's task —— View job

---

Code Review Complete

Review Date and Time

2025-10-28 17:40 UTC

---

Summary

Reviewed 11 changed files focusing on the logging additions and version bumps. The primary concerns involve debugging code that should be removed before merging and a potential logic issue with cursor handling in welcome message processing.

---

Key Findings

🔴 Critical - Debugging Code Must Be Removed

xmtp_api_d14n/src/protocol/in_memory_cursor_store.rs:68, 80

panic!("using in memory store");

These panic statements appear to be debugging code left in production. The commented-out implementations below them suggest this was intentional for debugging, but this will crash the application if these code paths are reached. Either:

1. Remove the panics and uncomment the working implementations, or
2. If this store is truly deprecated, add clear documentation explaining why and when it should/shouldn't be used

xmtp_db/src/encrypted_store/refresh_state.rs:653-670

let all_refresh_state = dsl::refresh_state
    .select((dsl::entity_id, dsl::entity_kind, dsl::sequence_id, dsl::originator_id))
    .load::<(Vec<u8>, EntityKind, i64, i32)>(conn)?;

for (entity_id, entity_kind, sequence_id, originator_id) in &all_refresh_state {
    tracing::info!(
        "refresh_state: entity_id={}, entity_kind={}, sequence_id={}, originator_id={}",
        hex::encode(entity_id),
        entity_kind,
        sequence_id,
        originator_id
    );
}

This full-table scan that logs every row before the actual query is severe performance issue. This will:

• Pull entire refresh_state table into memory on every call
• Generate massive log volume in production
• Cause significant latency increases
• Risk OOM on large tables

This must be removed before merge. If you need this level of debugging, use a feature flag or environment variable to gate it.

---

⚠️ Potential Logic Issue

xmtp_mls/src/groups/welcomes/xmtp_welcome.rs:465-484
The change from storing welcome.cursor.sequence_id to welcome_metadata.map(|meta| meta.message_cursor).unwrap_or(0) for the sequence_id field may break duplicate welcome detection. The requires_processing check at line 307 relies on comparing welcome.sequence_id() with self.last_sequence_id(&db), but now last_sequence_id will return the metadata cursor (defaulting to 0) instead of the actual welcome sequence ID.

This could allow duplicate welcomes to be processed when they shouldn't be. Consider:

• Storing the actual welcome sequence_id separately from the message_cursor, or
• Updating the duplicate detection logic to account for this change

---

✅ Good Additions

xmtp_api_d14n/src/queries/v3/mls.rs:80-84

tracing::info!(
    "Querying group messages for group ID: {} with cursor: {:?}",
    group_id,
    cursor
);

Good logging addition - provides useful debugging context without performance impact.

xmtp_mls/src/groups/mls_sync.rs (lines with added logging)
The additional logging in the sync module looks reasonable and helpful for debugging.

---

Recommendations

1. Remove debugging code (panics and full-table scan) before merging
2. Verify welcome duplicate detection still works correctly after the sequence_id changes
3. Consider using conditional compilation or feature flags for heavy debugging instrumentation
4. Version bumps to 1.6.1-rc1 look appropriate

---

[codecov]

Codecov Report

❌ Patch coverage is 96.42857% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 77.40%. Comparing base (261ff09) to head (83208bc).
⚠️ Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
xmtp_mls/src/cursor_store.rs	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2693      +/-   ##
==========================================
+ Coverage   77.35%   77.40%   +0.04%     
==========================================
  Files         355      359       +4     
  Lines       52362    52973     +611     
==========================================
+ Hits        40507    41002     +495     
- Misses      11855    11971     +116     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[macroscopeapp]

sequence_id and originator_id default to 0 when welcome_metadata is None, discarding the real MLS commit identifiers from welcome. Consider falling back to welcome.cursor.sequence_id and welcome.cursor.originator_id so older welcomes persist correct commit metadata.

-        let new_cursor = welcome_metadata
-            .map(|meta| meta.message_cursor)
-            .unwrap_or(0);
+        let new_message_cursor = welcome_metadata.map(|meta| meta.message_cursor);
@@
-            sequence_id: new_cursor as i64,
-            originator_id: 0,
+            sequence_id: new_message_cursor.unwrap_or(welcome.cursor.sequence_id) as i64,
+            originator_id: welcome.cursor.originator_id as i64,

🚀 Reply to ask Macroscope to explain or update this suggestion.

👍 Helpful? React to give us feedback.

[macroscopeapp]

Issue on line in xmtp_api_d14n/src/protocol/in_memory_cursor_store.rs:64:

Both latest and latest_per_originator panic on missing cursors, violating the CursorStore trait contract and crashing valid callers. Consider returning the stored or a default cursor for latest, and returning the filtered cursor in latest_per_originator (as done in latest_maybe_missing_per) instead of panicking.

-     fn latest(
-         &self,
-         _topic: &xmtp_proto::types::Topic,
-     ) -> Result<GlobalCursor, crate::protocol::CursorStoreError> {
-         panic!("using in memory store");
-         // Ok(self
-         //     .get_latest(topic)
-         //     .cloned()
-         //     .unwrap_or_else(GlobalCursor::default))
-     }
+     fn latest(
+         &self,
+         topic: &xmtp_proto::types::Topic,
+     ) -> Result<GlobalCursor, crate::protocol::CursorStoreError> {
+         Ok(self
+             .get_latest(topic)
+             .cloned()
+             .unwrap_or_else(GlobalCursor::default))
+     }

🚀 Reply to ask Macroscope to explain or update this suggestion.

👍 Helpful? React to give us feedback.

[macroscopeapp]

latest_cursor_for_id does a full-table scan and logs every row before the real query, causing severe performance and log overhead. Consider removing this pre-scan or gating it behind debug-only tooling.

-            let all_refresh_state = dsl::refresh_state
-                .select((
-                    dsl::entity_id,
-                    dsl::entity_kind,
-                    dsl::sequence_id,
-                    dsl::originator_id,
-                ))
-                .load::<(Vec<u8>, EntityKind, i64, i32)>(conn)?;
-
-            for (entity_id, entity_kind, sequence_id, originator_id) in &all_refresh_state {
-                tracing::info!(
-                    "refresh_state: entity_id={}, entity_kind={}, sequence_id={}, originator_id={}",
-                    hex::encode(entity_id),
-                    entity_kind,
-                    sequence_id,
-                    originator_id
-                );
-            }

🚀 Reply to ask Macroscope to explain or update this suggestion.

👍 Helpful? React to give us feedback.