dependency resolution tests

[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 ← this PR
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 Complete

Review Date: 2025-11-18 16:58 UTC

Key Issues

1. Logic Error in NetworkBackoffResolver::resolve (network_backoff.rs:75-79)

   The cursor matching logic has a critical flaw already noted by macroscopeapp. Additionally, there's a related issue: when envelopes are successfully fetched but don't match any missing items, the loop continues retrying indefinitely until timeout. Consider tracking when progress is being made:

   let got = envelopes.iter().map(|e| e.cursor()).collect::<Result<HashSet<Cursor>, _>>()?;
   let before_count = missing.len();
   missing.retain(|m| !got.contains(&m.cursor));
   let after_count = missing.len();
   
   if before_count == after_count && !missing.is_empty() {
       // No progress made, these dependencies may not exist
       warn!("no resolution progress after {} attempts", attempts);
       break;
   }

2. Test Utility Design Flaw (test_envelope.rs)

   The TestEnvelope implementation returns unreachable!() for most Envelope trait methods. This makes the test envelope fragile - if any code path accidentally calls these methods, tests will panic with unclear error messages. Consider either:

   • Implementing stub methods that return errors: Err(EnvelopeError::test_envelope_not_implemented())
   • Or documenting exactly which methods are safe to call with #[doc(hidden)] markers

3. Property Test Gap (props.rs:48-57)

   The sorted_dependencies generator has unclear logic around empty envelopes_subset. The comment says "must inherit dependencies of earlier sequence ids from same originator id" but the code sets new_clock = base in both branches. This appears redundant and suggests the logic may not match the intent:

   let new_clock = if envelopes_subset.is_empty() {
       base  // Line 51
   } else {
       for cursor in envelopes_subset.iter().map(|e| &e.cursor) {
           base.apply(cursor);
       }
       base  // Line 56 - same as empty case after loop
   };

4. Missing Error Context (network_backoff.rs:61)

   The warning when dropping dependencies doesn't include the attempt count or elapsed time, making debugging harder:

   warn!("dropping {} dependencies after {} attempts and {:?} elapsed, could not resolve", 
         missing.len(), attempts, time_spent.elapsed());

Test Coverage Notes

The 21 missing lines in test_envelope.rs are all unreachable!() branches, which is acceptable for test utilities. However, the actual resolver tests only cover happy paths and basic timeout. Consider adding:

• Tests for malformed responses from the client
• Tests for cursor collision scenarios across topics
• Tests verifying the LCC calculation with multiple topics

---

[macroscopeapp]

Add test-only macros and refactor dependency resolution to use immutable resolver with network backoff and ordered queries

Introduce an if_only_test macro and test utilities; refactor dependency resolution to accept &self, wrap API errors as retryable ResolutionError, implement a network_backoff constructor, and adjust NetworkBackoffResolver::resolve to return resolved envelopes plus any remaining missing entries. Add ordering combinators that produce causally ordered results and wire them into MlsMessagingApi::query_group_messages to use a network_backoff resolver.

📍Where to Start

Start with the resolver implementation and its usage in ordering: xmtp_api_d14n/src/protocol/traits/dependency_resolution.rs (https://github.com/xmtp/libxmtp/pull/2801/files#diff-93004bcf80e977ac25478c4e1dbbe38a64d3cc8ef509640addb33509b67b2092) and then review the network backoff constructor at xmtp_api_d14n/src/protocol/resolve/network_backoff.rs (https://github.com/xmtp/libxmtp/pull/2801/files#diff-de7799794fbc3300554d0474e251ec4117a5b0773a085ba0f94dd6f863c2f51e).

---

Macroscope summarized b293d24.

[macroscopeapp]

individual_wait_max defaults to Duration::ZERO in build(), so backoff() clamps base waits to zero and only jitter remains. Consider defaulting it to the same as ExponentialBackoff::default() (30s) to keep expected timing.

Suggested change

	individual_wait_max: Default::default(),
	individual_wait_max: ExponentialBackoff::default().individual_wait_max,

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

👍 Helpful? React to give us feedback.

[codecov]

Codecov Report

❌ Patch coverage is 80.48780% with 56 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.70%. Comparing base (fdb57f6) to head (b293d24).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
..._api_d14n/src/protocol/utils/test/test_envelope.rs	46.15%	21 Missing ⚠️
..._d14n/src/protocol/traits/dependency_resolution.rs	0.00%	14 Missing ⚠️
xmtp_proto/src/api_client/impls.rs	25.00%	9 Missing ⚠️
.../protocol/utils/test/dependency_resolution_test.rs	90.00%	7 Missing ⚠️
xmtp_proto/src/traits/error.rs	0.00%	3 Missing ⚠️
xmtp_proto/src/traits.rs	50.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2801      +/-   ##
==========================================
+ Coverage   74.43%   74.70%   +0.26%     
==========================================
  Files         376      381       +5     
  Lines       48820    49001     +181     
==========================================
+ Hits        36338    36605     +267     
+ Misses      12482    12396      -86     

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

[insipx]

this was making the builder not use a max wait. didn't effect anything because we construct the strategy directly rather than use builder

[insipx]

i moved these into their own file here in the first commit of the PR, no changes otherwise

these are re-used by the ordering tests