ordering proptests

[insipx]

#2789

[insipx]

This PR is part of a stack of 11 bookmarks:

1. trunk()
2. insipx/dependency-resolution
3. insipx/causal-sort
4. insipx/resolution
5. insipx/network-backoff
6. insipx/ordered-combinator
7. insipx/test-resolution
8. insipx/test-ordering ← this PR
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 Complete

Review Date: November 19, 2025, 00:52 UTC

---

Summary

This PR adds comprehensive property-based tests for ordering with dependency resolution, renames Resolved.envelopes → resolved consistently across the codebase, and improves test utilities. The implementation is solid overall with well-structured proptests.

Issues Found

1. Logic Bug in order.rs:84 (Critical) ⚠️

The filter logic appears inverted in xmtp_api_d14n/src/protocol/order.rs:84. You're checking if unresolved contains m.cursor(), but this compares the envelope's own cursor against the set of dependency cursors that couldn't be resolved. This would incorrectly filter out envelopes whose own cursor appears in the unresolved set, rather than filtering out envelopes that depend on unresolved cursors.

The logic should check if any of the envelope's dependencies (from m.depends_on()) are in the unresolved set, not the envelope's own cursor. Macroscopeapp flagged similar logic at line 84, and this needs addressing.

2. Missing Coverage for Edge Case in ExponentialBackoff::builder() (Medium)

In common/src/retry.rs:189-196, the ExponentialBackoffBuilder::build() method doesn't set individual_wait_max, which means it defaults to the struct's field initialization. However, there's no field default for individual_wait_max in the builder. This could cause issues if the field isn't initialized properly. Consider adding individual_wait_max as a builder parameter or documenting why it's hardcoded to 30 seconds.

3. Proptest Edge Case (Low)

In order.rs:221, the test skips envelopes that have_dependency_on_any(&unavailable) but doesn't verify that these skipped envelopes are not in the result. This could hide bugs where unavailable-dependent envelopes incorrectly appear in results. Consider adding an assertion that such envelopes are absent from the final result.

Positive Observations

• Excellent use of prop_compose! to generate complex test scenarios with resolvable/unresolvable dependencies
• The TestEnvelope::timestamp() implementation with deterministic timestamps (order.rs:44-50) is clever and enables consistent timestamp-based sorting in tests
• The proptest regression file captures real failure cases - good practice
• Field rename from envelopes → resolved is consistent and improves clarity
• Test harness with MockResolver is well-designed and reusable

---

Recommendation: Address the critical logic bug in order.rs:84 before merging. The other issues are lower priority but worth considering.

---

[macroscopeapp]

Align protocol ordering proptests and rename Resolved to use resolved while updating Ordered.order to splice resolved and re-sort by timestamp

Refactor dependency resolution by renaming Resolved.envelopes to Resolved.resolved across protocol and tests, adjust Ordered.order to consume the new field and maintain timestamp order, and add property tests with regression seeds for ordering when dependencies are unresolved. Also set default backoff caps in retry.ExponentialBackoffBuilder.build (120s total, 30s individual) and add debugging tools to the Nix dev shell.

📍Where to Start

Start with the Resolved struct change in dependency_resolution.rs, then review its consumption in Ordered.order in order.rs.

---

Macroscope summarized 9f5e2f8.

[macroscopeapp]

Filter is inverted and uses the wrong cursor. unresolved contains dependency cursors, but we compare each envelope’s cursor(), which retains unresolved entries. Suggest: compare unresolved to each envelope’s missing dependency cursors and drop only envelopes still unresolved; keep those with deps fully resolved.

Suggested change

	if !unresolved.contains(&m.cursor()?) {
	if unresolved.contains(&m.cursor()?) {

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

👍 Helpful? React to give us feedback.

[codecov]

Codecov Report

❌ Patch coverage is 93.33333% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.62%. Comparing base (a1a270f) to head (9f5e2f8).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
xmtp_api_d14n/src/protocol/order.rs	83.33%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2802      +/-   ##
==========================================
+ Coverage   74.52%   74.62%   +0.09%     
==========================================
  Files         383      384       +1     
  Lines       49299    49592     +293     
==========================================
+ Hits        36740    37007     +267     
- Misses      12559    12585      +26     

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