add child resolution & ice to CursorStore trait

[insipx]

Add child resolution and ICE handling by extending CursorStore with ice and resolve_children across in-memory, SQL, and wrapper implementations

Extend CursorStore with ice and resolve_children, add an in-memory icebox in InMemoryCursorStore, delegate persistence/lookup in the SQLite store, and switch GlobalCursor to BTreeMap with merge via apply. Update callers to construct and return GlobalCursor directly.

📍Where to Start

Start with the trait changes in cursor_store.rs, then review the in-memory implementation in in_memory_cursor_store.rs and the SQLite store in cursor_store.rs.

---

📊 Macroscope summarized 0972aaf. 9 files reviewed, 6 issues evaluated, 6 issues filtered, 0 comments posted

🗂️ Filtered Issues

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

• line 133: rows_to_global_cursor_map performs unchecked signed-to-unsigned integer casts that can silently wrap negative database values, yielding incorrect OriginatorId and SequenceId. Specifically, originator_id as u32 and sequence_id.unwrap_or(0) as u64 will wrap if originator_id or sequence_id are negative, producing large unsigned values instead of rejecting invalid input. This can corrupt the resulting GlobalCursor (e.g., mapping to the wrong originator or an enormous sequence), leading to incorrect cursor computations downstream. Validate non-negativity before casting, or return an error on invalid rows. [ Previously rejected ]
• line 706: latest_cursor_combined may generate invalid SQL when entities contains no kinds that map to group_messages (i.e., neither EntityKind::ApplicationMessage nor EntityKind::CommitMessage). In that case group_message_kinds becomes an empty Vec, producing kind IN () in the constructed SQL, which is a syntax error in SQLite. This is reachable if callers pass, e.g., only EntityKind::Welcome. Ensure you guard against an empty group_message_kinds (e.g., omit the group_messages branch or use a false predicate) before constructing the placeholders. [ Out of scope ]

xmtp_mls/src/cursor_store.rs — 0 comments posted, 4 evaluated, 4 filtered

• line 70: sid from the database is cast to u64 with as without validating non-negativity. If sid is negative (e.g., due to DB corruption or unexpected input), it will wrap to a very large u64, corrupting the cursor and causing incorrect behavior (e.g., dominance/missing comparisons). Validate sid >= 0 and return an error or clamp, or change the DB/API to return an unsigned type. [ Previously rejected ]
• line 101: sid is cast to u64 without validating non-negativity in latest_per_originator for identity updates, which can wrap negatives to huge values and corrupt cursor semantics. Validate or use an unsigned type. [ Previously rejected ]
• line 163: Potential integer wrap: sid from get_latest_sequence_id_for_inbox(...) is cast with as u64 without validation. If sid is negative, the cast will wrap to a very large u64, producing an incorrect GlobalCursor value and potentially causing downstream logic to skip messages. Validate that sid >= 0 and either clamp, normalize, or return an error before inserting into the GlobalCursor. Affected line: map.insert(Originators::INBOX_LOG, sid as u64);. [ Previously rejected ]
• line 163: sid is cast to u64 without validation in latest_for_topics for IdentityUpdatesV1, which can wrap negative SIDs and produce incorrect GlobalCursor values. Validate sid or return an error if negative. [ Previously rejected ]

[insipx]

This PR is part of a stack of 12 bookmarks:

1. trunk()
2. insipx/causal-fix
3. insipx/icebox-global-cursor
4. insipx/batch-queries
5. insipx/extractors
6. insipx/cursor-store-icebox ← this PR
7. insipx/icebox-in-ordering
8. insipx/test-ordering
9. insipx/paged-stream
10. insipx/ordered-stream
11. insipx/ordered-stream-test
12. push-pnvnmqsyqqqy
13. push-syomxrnuqsnk

---

Created with jj-stack

[claude]

Claude finished @insipx's task —— View job

---

PR Review - December 11, 2025

Critical Issues

1. False positive in OrphanedEnvelope::is_child_of (xmtp_proto/src/types/orphaned_envelope.rs:33-36)

The method uses GlobalCursor::get() which returns 0 for missing originators. When cursor.sequence_id == 0, this creates false positives:

pub fn is_child_of(&self, cursor: &Cursor) -> bool {
    self.depends_on.contains_key(&cursor.originator_id)
        && self.depends_on.get(&cursor.originator_id) == cursor.sequence_id
}

If an orphan has no dependency for a given originator, get() returns 0. When the cursor's sequence_id is also 0, the comparison succeeds incorrectly. This affects InMemoryCursorStore::resolve_children at line 124, potentially removing unrelated envelopes from the icebox.

Fix: Check key existence explicitly:

pub fn is_child_of(&self, cursor: &Cursor) -> bool {
    self.depends_on.get(&cursor.originator_id) == cursor.sequence_id
        && self.depends_on.contains_key(&cursor.originator_id)
}

2. SQL error in lowest_common_cursor_combined (xmtp_db/src/encrypted_store/refresh_state.rs:563-569)

The query references conversation_list with columns that don't exist in that table (originator_id, sequence_id, message_kind). This will fail at runtime. Based on the comment mapping and other code, this should query group_messages instead.

Test Coverage Concerns

• 20% patch coverage with 135 untested lines is concerning for core cursor logic
• New trait methods ice() and resolve_children() lack coverage in:
  • InMemoryCursorStore (xmtp_api_d14n/src/protocol/in_memory_cursor_store.rs:111-130)
  • SqliteCursorStore wrapper (xmtp_mls/src/cursor_store.rs:195-210)
  • All trait impls for Option<T>, &T, Arc<T>, Box<T> (xmtp_api_d14n/src/protocol/traits/cursor_store.rs:159-177)
• The database implementation in xmtp_db/src/encrypted_store/icebox.rs has good test coverage, but the higher-level abstractions do not

Additional Issues

3. Missing test for sequence_id=0 edge case

Given the false positive bug, there should be explicit tests covering:

• Orphans with dependencies on sequence_id 0
• Resolution attempts with cursor sequence_id 0
• Empty GlobalCursor behavior in is_child_of

4. GlobalCursor refactor looks good

The HashMap → BTreeMap change is sensible for deterministic ordering and Hash derivation. The new apply() method is a clean addition.

Recommendations

1. Fix the is_child_of false positive bug (high priority)
2. Fix the SQL table reference bug (high priority)
3. Add integration tests exercising ice() and resolve_children() through the full stack
4. Add edge case tests for sequence_id=0 scenarios
5. Consider property-based tests for orphan resolution logic given its complexity

---

Review completed at 2025-12-11 21:04 UTC

[insipx]

changed the backing data structure from a HashMap to a BTreeMap. The BTreeMap makes more sense to me, since we can order the map by keys and also derive Hash on it. the semantics of inserting/"contains" etc. is unchanged

[codecov]

Codecov Report

❌ Patch coverage is 20.11834% with 135 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.49%. Comparing base (fc5cf12) to head (0972aaf).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
xmtp_api_d14n/src/protocol/traits/cursor_store.rs	0.00%	77 Missing ⚠️
xmtp_mls/src/cursor_store.rs	0.00%	21 Missing ⚠️
...tp_api_d14n/src/protocol/in_memory_cursor_store.rs	13.63%	19 Missing ⚠️
...i_d14n/src/queries/d14n/test/send_group_message.rs	0.00%	10 Missing ⚠️
xmtp_proto/src/types/orphaned_envelope.rs	0.00%	4 Missing ⚠️
xmtp_db/src/encrypted_store/refresh_state.rs	70.00%	3 Missing ⚠️
xmtp_proto/src/types/global_cursor.rs	95.23%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2869      +/-   ##
==========================================
- Coverage   73.67%   73.49%   -0.18%     
==========================================
  Files         402      402              
  Lines       51098    51237     +139     
==========================================
+ Hits        37644    37656      +12     
- Misses      13454    13581     +127     

☔ 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.