add icebox to OrderedEnvelopeCollection implementation

[insipx]

Add icebox support by requiring a CursorStore in protocol.order.Ordered and icing unresolved orphans during OrderedEnvelopeCollection::order

Introduce a store-backed ordering flow that recovers children, sorts by timestamp/causal order, requests dependencies as RequiredDependency, and ices unresolved orphans in the store; update resolver and traits to accept RequiredDependency and propagate store/decode errors. Core changes center on protocol.order.Ordered and dependent combinators/resolvers, with Envelope dropping consume and tests updated accordingly.

📍Where to Start

Start with the OrderedEnvelopeCollection for Ordered::order implementation in order.rs, then review required_dependencies, recover_lost_children, and the resolver changes in network_backoff.rs.

---

📊 Macroscope summarized 38c66a7. 12 files reviewed, 3 issues evaluated, 3 issues filtered, 0 comments posted

🗂️ Filtered Issues

xmtp_api_d14n/src/protocol/order.rs — 0 comments posted, 1 evaluated, 1 filtered

• line 176: The filter logic change reduces matching precision: the old code matched envelopes by BOTH topic AND cursor using MissingEnvelope::new(topic, cursor), but the new code only matches by cursor alone. This could cause the resolver to return envelopes with matching cursors but incorrect topics, potentially masking bugs where topic matching is important. If RequiredDependency contains topic information that should be used for matching, it's being ignored. [ Previously rejected ]

xmtp_api_d14n/src/protocol/resolve/network_backoff.rs — 0 comments posted, 1 evaluated, 1 filtered

• line 53: The resolve function sleeps before making the first network query. On the first iteration with attempts = 0, self.backoff.backoff(0, time_spent) returns Some(duration) (base duration + jitter), causing an unnecessary delay before any query attempt. The first resolution attempt should be made immediately without sleeping. [ Out of scope ]

xmtp_api_d14n/src/protocol/traits.rs — 0 comments posted, 1 evaluated, 1 filtered

• line 88: In is_retryable() for EnvelopeError, Self::Decode(_) => true marks prost::DecodeError as retryable. However, decode errors indicate malformed protobuf bytes, and retrying a decode operation on the same malformed bytes will produce the same error. This is inconsistent with the rationale documented in ConversionError's is_retryable() implementation which explicitly states that conversions are not retryable because "the bytes are static". This could cause infinite retry loops or wasted retry attempts on permanently failing operations. [ Already posted ]

[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 ← this PR
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 15, 2025, 7:10 PM UTC

This PR successfully integrates icebox functionality into the ordering flow. The core implementation is solid with good property-based testing. Here are the key findings:

Critical Issues

1. Decode Error Retryability (xmtp_api_d14n/src/protocol/traits.rs:88)

• Already flagged by Macroscope, but worth emphasizing: Self::Decode(_) => true marks protobuf decode errors as retryable
• Decode errors on static bytes will never succeed on retry - this creates futile retry loops
• The comment in ConversionError::is_retryable() explicitly states conversions aren't retryable because "the bytes are static"
• Recommendation: Change to Self::Decode(_) => false for consistency and correctness

Correctness Concerns

2. Potential Infinite Loop in Recovery (xmtp_api_d14n/src/protocol/order.rs:136)

• recover_lost_children() is called inside the resolution loop after each partial resolution
• If a child is recovered but still has missing dependencies, it gets added to missing, resolution partially succeeds on unrelated envelopes, child gets re-recovered in next iteration
• This could cause the same orphaned child to be recovered repeatedly until resolution timeout
• The deduplication via cursors HashSet (line 87) only prevents duplicates within a single recovery, not across loop iterations
• Recommendation: Consider tracking recovered children across the entire order() call to prevent repeated recovery attempts

3. Unresolved Dependencies Silently Dropped (xmtp_api_d14n/src/protocol/order.rs:128)

• Resolved { mut resolved, .. } ignores the unresolved field with .. pattern
• If resolver returns partial results with some dependencies marked as unresolved, those are never tracked
• Old code explicitly handled unresolved envelopes; new code may loop until timeout if resolver keeps returning different resolved items while some remain unresolved
• Recommendation: Consider logging or tracking unresolved to provide visibility into why resolution may be taking multiple iterations

Design Observations

4. Test Coverage for Icebox Recovery (xmtp_api_d14n/src/protocol/order.rs:67-95)

• The recover_lost_children() logic is not directly tested in the order.rs tests
• Property test covers unresolvable dependencies but uses NoCursorStore which always returns empty for resolve_children()
• Recommendation: Add test cases with a mock CursorStore that returns orphaned children to verify the recovery logic

5. Performance: Multiple HashSet Constructions (xmtp_api_d14n/src/protocol/order.rs:68-83)

• Already addressed by other reviewers, but line 68 collects cursors into Vec, then line 83 constructs HashSet from it
• Recommendation: Already suggested to optimize this

Security & Safety

No security concerns identified. The changes maintain proper error handling and don't introduce unsafe operations.

Summary

The core icebox integration is well-designed with solid property-based testing. Main concerns are:

1. Must fix: Decode error retryability inconsistency
2. Should investigate: Potential for repeated child recovery in loops
3. Nice to have: Better visibility into unresolved dependencies, direct testing of icebox recovery

The codecov report shows 83.7% patch coverage with 22 missing lines. Missing coverage appears concentrated in error paths and test utilities, which is acceptable.

---

[codecov]

Codecov Report

❌ Patch coverage is 82.48175% with 24 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.61%. Comparing base (0949b7d) to head (38c66a7).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
xmtp_api_d14n/src/protocol/order.rs	81.48%	10 Missing ⚠️
..._api_d14n/src/protocol/utils/test/test_envelope.rs	80.00%	5 Missing ⚠️
xmtp_api_d14n/src/protocol/sort/causal.rs	0.00%	4 Missing ⚠️
xmtp_api_d14n/src/protocol/traits.rs	0.00%	2 Missing ⚠️
..._d14n/src/protocol/traits/dependency_resolution.rs	33.33%	2 Missing ⚠️
.../protocol/utils/test/dependency_resolution_test.rs	92.85%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2882      +/-   ##
==========================================
+ Coverage   73.49%   73.61%   +0.11%     
==========================================
  Files         405      405              
  Lines       51656    51710      +54     
==========================================
+ Hits        37965    38064      +99     
+ Misses      13691    13646      -45     

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