Add an ordering extension to a Vec<Envelope>

[insipx]

Try to order envelopes by their dependencies and resolve any that are missing. if resolution fails, drop the missing envelopes and continue with the query.

the ordering extension is modified again in #2775 to remove envelopes from missing. i may backport those changes to this pr for consistency

[insipx]

This PR is part of a stack of 11 bookmarks:

1. trunk()
2. insipx/dependency-resolution
3. insipx/causal-sort
4. insipx/resolution ← this PR
5. insipx/network-backoff
6. insipx/ordered-combinator
7. insipx/test-resolution
8. insipx/test-ordering
9. insipx/add-published-in-epoch
10. insipx/sql-queries
11. insipx/sync-conn
12. insipx/set-dependencies

---

Created with jj-stack

[claude]

Claude finished @insipx's task —— View job

---

Code Review - PR #2771

Review Date: November 13, 2025, 22:28 UTC

---

Critical Issues

1. Infinite Loop Risk in order() (xmtp_api_d14n/src/protocol/order.rs:41)

The while let Some(missing) = sort::causal(...).sort()? loop at line 41 lacks termination guarantees. If resolver.resolve(cursors) returns envelopes that don't satisfy the missing dependencies, the loop will continue indefinitely. The code needs progress detection:

// Current code has no way to detect:
// - resolver returns empty vec repeatedly
// - resolver returns envelopes that don't resolve the missing dependencies
// - resolver returns the same envelopes we already have

Recommendation: Track progress between iterations (e.g., count of missing envelopes, hash of missing set) and break/error if no progress is made after resolution attempt.

---

High Priority Issues

2. Missing Thread Safety Bounds (xmtp_api_d14n/src/protocol/traits/envelopes.rs:17, 39)

As noted by @tylerhawkes, the ProtocolEnvelope and Envelope traits should have MaybeSend + MaybeSync bounds for consistency with other traits in the codebase. Currently:

• Line 17: ProtocolEnvelope only requires std::fmt::Debug
• Line 39: Envelope only requires std::fmt::Debug

But ResolveDependencies (line 25 of dependency_resolution.rs) already uses these bounds, creating inconsistency.

---

Medium Priority Issues

3. Silently Dropping Envelopes on Resolution Failure (order.rs:60-61)

When ResolutionError::ResolutionFailed occurs, the code returns Ok(()) and silently drops all missing envelopes. While the PR description mentions this is intentional ("drop the missing envelopes and continue"), this could mask critical data loss scenarios. Consider:

• Logging dropped envelopes at warn/error level
• Returning metadata about what was dropped
• Exposing metrics for monitoring

4. Error Handling Ambiguity (order.rs:56-64)

The resolver returns ResolutionError::ResolutionFailed to signal "couldn't find dependencies", but this same error type is used for other failures. The error handling would be clearer with distinct error variants for "dependencies not found" vs "network/API failure".

5. Test Coverage (order.rs:76)

The TODO comment at line 76 indicates missing tests for the critical Ordered::order() function. Given the complexity of the async resolution loop and the infinite loop risk, comprehensive tests are essential. The 0% patch coverage reported by Codecov confirms this gap.

---

Low Priority / Style

6. Splice Performance (order.rs:68-69)

Using splice(0..0, ...) twice to prepend elements is less efficient than alternatives:

// Current approach requires shifting elements twice
envelopes.splice(0..0, missing.into_iter());
envelopes.splice(0..0, resolved.into_iter());

// Consider: reserve + rotate or single splice with chain

7. Documentation Clarity (order.rs:66-67)

The comment "apply missing before resolved" is backwards relative to the code execution order (resolved is actually inserted first at index 0, then missing is inserted before it).

---

Summary

The implementation provides a solid foundation for envelope ordering with dependency resolution, but needs:

1. Critical: Loop termination guarantees to prevent infinite loops
2. High: Thread safety trait bounds for WASM compatibility
3. Medium: Tests for the core ordering logic
4. Medium: Better observability for dropped envelopes

The causal sorting logic itself (in causal.rs) is well-tested with property-based tests, which is excellent. The main concerns are in the new async orchestration layer.

---

• Read and analyze key implementation files
• Review code quality and potential bugs
• Check performance and security considerations
• Evaluate test coverage concerns
• Post review feedback

---

[codecov]

Codecov Report

❌ Patch coverage is 0% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.27%. Comparing base (904bfda) to head (21889f8).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
xmtp_api_d14n/src/protocol/order.rs	0.00%	15 Missing ⚠️
xmtp_api_d14n/src/protocol/impls/vector_clock.rs	0.00%	11 Missing ⚠️
xmtp_api_d14n/src/protocol/traits.rs	0.00%	1 Missing ⚠️
..._d14n/src/protocol/traits/dependency_resolution.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2771      +/-   ##
==========================================
- Coverage   77.30%   77.27%   -0.03%     
==========================================
  Files         375      376       +1     
  Lines       53692    53719      +27     
==========================================
+ Hits        41504    41512       +8     
- Misses      12188    12207      +19     

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

Possible infinite loop: while let Some(missing) = sort::causal(...).sort()? can stay Some if resolver.resolve(...) returns no new/related envelopes. Consider detecting no progress (e.g., empty resolved, already-present items, or unchanged missing) and break or return an error.

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

👍 Helpful? React to give us feedback.

[macroscopeapp]

Add asynchronous ordering for Vec<Envelope> and introduce Ordered<T, R>::order() in order.rs

Introduce OrderedEnvelopeCollection with async order() that sorts by timestamp, resolves causal gaps via resolver.resolve, and iterates until no missing; add VectorClock::missing, rename resolution to resolve, and switch ResolveDependencies::resolve to return ResolutionError.

📍Where to Start

Start with the Ordered<T, R>::order() implementation in order.rs.

---

Macroscope summarized 21889f8.

[tylerhawkes]

A VecDeque would be more efficient than calling splice.

[insipx]

using a VecDeque would be better here. there's also a possibility of using reserve+rotate to keep the Vec. I just gave it a shot, but b/c i coupled the implementation of all the sorts to a Vec it required a bit more re-arranging than I would like

I'm going to be opening PRs for testing each these parts, so I'll revisit the VecDeque in those. I'm also anxious to change to the alternative reserve + rotate w/o good unit tests here as well.