test for icebox ordering

[insipx]

Deduplicate and recursively resolve orphaned envelopes in xmtp_api_d14n::protocol::InMemoryCursorStore to test icebox ordering

Convert the icebox to a HashSet with custom Eq/Hash on OrphanedEnvelope that ignore payload bytes, add recursive child resolution without removal, and adjust ordering to sort by timestamp once before the loop. Tests add regression cases, new formatting helpers, and builder utilities for orphans and cursors.

📍Where to Start

Start with the ordering flow in Ordered<T,R,S>::order in xmtp_api_d14n/src/protocol/order.rs, then review icebox behavior in InMemoryCursorStore and resolve_children_inner in xmtp_api_d14n/src/protocol/in_memory_cursor_store.rs, and the OrphanedEnvelope equality/hash changes in xmtp_proto/src/types/orphaned_envelope.rs.

---

Macroscope summarized 92a59ae.

[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
7. insipx/icebox-in-ordering
8. insipx/test-ordering ← this PR
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

---

Code Review Complete

Review Date: December 15, 2025, 19:30 UTC

I've reviewed this PR and have identified several issues that haven't been covered by the existing Macroscope comments. Below are my findings:

---

🔴 Critical Issues

1. Potential False Positive in is_child_of with sequence_id==0 (xmtp_proto/src/types/orphaned_envelope.rs:57-60)

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
}

Issue: While the contains_key check prevents incorrect matches for missing originators, there's a subtle edge case:

• If cursor.sequence_id == 0 and depends_on DOES contain the originator with value 0, this returns true
• However, sequence_id == 0 typically means "no message" or "initial state" in vector clocks
• This could lead to false positives when matching dependencies

Context: Macroscope flagged similar concerns. While the implementation is technically correct, the semantics of sequence_id == 0 should be clarified.

Recommendation: Either document that sequence_id == 0 is a valid dependency, or add an explicit check to reject it if it's not.

---

2. Ordering Change May Break Causal Sort (xmtp_api_d14n/src/protocol/order.rs:134-135)

The refactored order method changes the ordering from prepending to appending:

New behavior:

self.envelopes.append(&mut resolved);
self.envelopes.append(&mut missing);

Problem: The old implementation prepended resolved envelopes and re-ran timestamp sort inside the loop. The new implementation:

1. Appends resolved envelopes to the end
2. Does NOT re-run timestamp sort inside the loop (only at line 125)

Concern: Causal sort algorithms typically require dependencies to appear before dependents in the input list. If causal sort expects this ordering, appending resolved dependencies to the end could break correctness.

Recommendation:

• Add a comment explaining why appending is correct for this causal sort implementation
• Verify with tests that the causal sort doesn't depend on resolved dependencies appearing before dependent envelopes

---

⚠️ Medium Issues

3. Hash Comment Needs Update (xmtp_proto/src/types/orphaned_envelope.rs:23-32)

The comment says "ignore the payload field" but the implementation now hashes depends_on:

// comment: "ignore the payload field"
impl Hash for OrphanedEnvelope {
    fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
        self.cursor.hash(state);
        self.group_id.hash(state);
        self.depends_on.hash(state);  // This is now included!
    }
}

Issue: The comment is outdated. Based on @insipx's response to the review, including depends_on in the hash is intentional.

Recommendation: Update the comment to clarify that both payload is ignored AND depends_on is included for deduplication purposes.

---

4. Variable Shadowing in recover_lost_children (xmtp_api_d14n/src/protocol/order.rs:68, 83)

let cursors: Vec<_> = ... // line 68
// ...
let cursors: HashSet<Cursor> = HashSet::from_iter(cursors); // line 83

Issue: Variable shadowing makes the code harder to maintain and could lead to errors during future modifications.

Recommendation: Use distinct variable names (e.g., cursor_vec and cursor_set or cursors_set).

---

5. Test Helper Excludes Unavailable from all_envelopes (xmtp_api_d14n/src/protocol/order.rs:295-300)

Macroscope correctly identified this issue. all_envelopes() doesn't include unavailable():

fn all_envelopes(&self) -> Vec<TestEnvelope> {
    self.available()
        .into_iter()
        .chain(self.missing.envelopes.clone())
        .chain(self.returned())
        .collect()
}

Impact: has_unavailable_in_dependency_chain won't find transitive dependencies that are unavailable, potentially causing incomplete validation in tests.

Recommendation: Add .chain(self.unavailable()) to include all envelopes for complete transitive checking.

---

ℹ️ Minor Issues / Code Quality

6. Unwrap Chains in Test Assertions (xmtp_api_d14n/src/protocol/order.rs:209, 219)

let clock = cursor.get(&env.topic().unwrap()).unwrap();

Issue: Macroscope correctly flagged this - panics before reaching prop_assert! won't include the caller's message, and #[track_caller] won't help.

Recommendation: Use .expect() with descriptive messages:

let clock = cursor
    .get(&env.topic().expect(message))
    .expect(message);

---

7. Test Timing Issue in orders_with_recovered_children (xmtp_api_d14n/src/protocol/order.rs:474-485)

Macroscope identified that resolve_children is called AFTER order() completes:

ordered.order().now_or_never()...  // line 487
let (second_ordering_pass, ..) = ordered.into_parts();  // line 491

// Then resolve_children is called on line 479 (in variable capture above)

Issue: If order() already recovered and removed children from the icebox, resolve_children will return empty results, making had_children false and skipping important assertions (lines 502-529).

Recommendation: Capture resolved children BEFORE calling order() for accurate test validation.

---

✅ Positive Observations

1. Excellent test coverage: The proptest property-based tests are comprehensive and test complex dependency resolution scenarios
2. Helpful debug utilities: The new DebugDisplay trait and format_enumerated/format_list methods will make debugging test failures much easier
3. Good deduplication: Switching from Vec to HashSet for the icebox prevents duplicate orphans
4. Recursive child resolution: The recursive approach in resolve_children_inner correctly handles transitive dependencies

---

Summary

Critical: Issue #2 (ordering change) should be verified to ensure it doesn't break causal sort correctness.

Medium: Issues #3-5 should be addressed for code correctness and maintainability.

Minor: Issues #6-7 are test improvements that would make failures easier to debug.

Overall, this is solid work on a complex dependency resolution system. The main concerns are around the ordering semantics and ensuring the tests accurately validate the behavior.

---

[codecov]

Codecov Report

❌ Patch coverage is 94.05405% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.71%. Comparing base (7f5684f) to head (92a59ae).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
..._api_d14n/src/protocol/utils/test/test_envelope.rs	76.47%	8 Missing ⚠️
xmtp_api_d14n/src/protocol/order.rs	96.96%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2916      +/-   ##
==========================================
+ Coverage   73.59%   73.71%   +0.11%     
==========================================
  Files         405      405              
  Lines       51710    51883     +173     
==========================================
+ Hits        38057    38245     +188     
+ Misses      13653    13638      -15     

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