refactor: migrate hand-written RetryableError impls to #[derive(Retryable)]

[insipx]

Summary

Stacked on #3753 — merge that first. (Absorbs former #3757; one migration PR.)

Replaces ~47 hand-written impl RetryableError blocks across 8 crates (+xnet) with #[derive(Retryable)]. Net −456 lines (43 files, +297/−753).

How each shape migrated

• Forwarding arms (~110) → explicit #[retry(inherit)] (#[from] alone does nothing)
• Hardcoded-true oddballs (GroupError::LockUnavailable, SubscribeError::StreamStale, EnvelopeError::Decode, ApiClientError::Http/Expired, …) → #[retry(true)]
• Hardcoded-false variants → no annotation (baseline). ConversionError's 16 variants: zero attrs.
• All-true / inverted enums (NotFound, GrpcError, DeviceSyncError's !matches!) → #[retry(default = true)] (+ #[retry(false)] exceptions)
• ClientError::Generic(String) → #[retry(when = this.contains("database is locked"))]
• Multi-field forwards (ApiClientError::ClientWithEndpoint, BootstrapSynthesisError::IdentityUpdateLookup, CommitLogError::FailedToSendReadd) → #[retry(when = source.is_retryable())]
• Collection aggregation (CommitLogError::FailedReadds) → #[retry(when = errors.iter().any(|e| e.is_retryable()))]
• GroupMessageProcessingError's nested match collapsed via a new derive on inner ProcessMessageWithAppDataError — the one API-surface change: that generic enum gains a ProcessMessageError<S>: RetryableError<Mls> where-bound (the derive's documented generics rule; sole workspace instantiation satisfies it)

Testing philosophy

Per-enum golden tests are deliberately not included. The macro's own suite in #3753 (18 behavior tests + 11 trybuild fixtures) is the behavioral contract; after the one-time equivalence audit, the annotations are the source of truth — the same trust model the hand-written impls had (which were almost entirely untested). Pre-existing retry-behavior tests (e.g. SignatureError #3394 propagation, multi_node_client fail-fast) still pass unchanged.

Equivalence audit: every migrated variant (~350 across ~47 types) was re-derived against the verbatim deleted impls by an adversarial pass — zero mismatches. (The audit tables exist as a local artifact; ask if you want them attached.)

Deliberately NOT migrated

The 22 specialized impl RetryableError<Mls> for openmls::*/diesel orphan-rule impls (mirror-type macro support would cost more code than it removes), GroupAppDataError<SqlKeyStoreError> (instantiation-specific), ReceiveErrors/SyncSummary one-line aggregators, test mocks. Hand-written impls are now a single explainable category: foreign-type orphan workarounds.

Verification

• cargo check --workspace --all-targets clean; wasm cargo check -p xmtp_db --target wasm32-unknown-unknown clean (wasm PlatformStorageError migrated)
• Full crate suites green under nextest; fmt + clippy -Dwarnings clean

🤖 Generated with Claude Code

Note

Migrate hand-written RetryableError impls to #[derive(Retryable)] across MLS and DB crates

Replaces ~30 manual impl RetryableError blocks with the #[derive(Retryable)] macro across error enums in xmtp_mls, xmtp_db, xmtp_api, xmtp_api_d14n, xmtp_proto, xmtp_id, and related crates.

• Per-variant retryability is now expressed via #[retry(inherit)], #[retry(true)], #[retry(false)], #[retry(default = true)], or predicate-based #[retry(when = ...)] attributes instead of exhaustive match arms.
• Behavioral semantics are preserved: variants that delegated to inner errors use inherit, unconditionally retryable variants use true, and non-retryable defaults use false or no attribute.
• DeviceSyncError switches to #[retry(default = true)] with explicit #[retry(false)] on four non-retryable variants, inverting the previous match structure.
• Risk: any mismatch between old match arms and new derive attributes would silently change retry behavior; review of annotated variants is advised.

^{Macroscope summarized e23fad2.}

[claude]

Claude finished @insipx's task —— View job

---

Code Review Complete

Review Date: 2026-06-11 16:42 UTC

Summary

I've reviewed the migration from hand-written RetryableError implementations to #[derive(Retryable)]. The PR represents a substantial refactor (43 files, +296/-753 lines) with the goal of declaratively expressing retry behavior through proc macro attributes.

Assessment

Overall Quality: ✅ Strong

The migration appears well-executed with proper understanding of the macro semantics. The key patterns are correct:

• Baseline non-retryable: Unannotated variants default to non-retryable (e.g., OrderedStreamError::Resolver)
• Explicit forwarding: #[retry(inherit)] correctly propagates retryability from wrapped errors
• Conditional expressions: #[retry(when = ...)] for complex logic (e.g., ClientError::Generic checking for "database is locked")
• Inverted enums: #[retry(default = true)] for all-retryable enums like NotFound

Key Observations

1. OrderedStreamError discussion is resolved: The Macroscope bot's concern about OrderedStreamError::Resolver was incorrect. The author's response is accurate—an unannotated #[from] variant takes the container baseline (non-retryable), exactly matching the original behavior. The macro design explicitly does NOT auto-forward on #[from] alone.

2. ProcessMessageWithAppDataError migration is correct: This enum (crates/xmtp_mls/src/groups/app_data/mod.rs:62-85) correctly uses:

   • #[retry(inherit)] on OpenMls variant (forwards ProcessMessageError's retryability)
   • No annotation on AppDataDecode variant (baseline non-retryable)

   The where-bound ProcessMessageError<S>: RetryableError<Mls> mentioned in the PR description is properly present at line 66.

3. Consistency across error types: Sampled several large error enums (GroupError, StorageError, ClientError, GroupMessageProcessingError, ResolutionError) and the annotation patterns are consistent and semantically correct.

4. No duplicate annotations found: Despite the grep showing files with multiple #[retry(inherit)], these are across different variants, not duplicates on the same variant. The author mentions fixing duplicates in later commits (d20c21f, 526c596), but those appear to be outside the current PR scope.

No Issues Found

All migrated code follows the documented macro behavior. The annotations preserve the original retry semantics as claimed. Per the PR's testing philosophy, the macro's own test suite provides the behavioral contract, which is appropriate for this type of mechanical refactor.

---

No changes requested. The migration is correct and the -456 line reduction is a genuine improvement in maintainability.

---

[macroscopeapp]

Approvability

Verdict: Needs human review

1 blocking correctness issue found. Mechanical refactor migrating hand-written RetryableError implementations to derive macros. An unresolved review comment identifies a potential compilation error (missing trait import) that should be addressed before merging.

^{You can customize Macroscope's approvability policy. Learn more.}

Dismissing prior approval to re-evaluate 4f5e8c2

Dismissing prior approval to re-evaluate d20c21f

[insipx]

Both duplicate #[retry(inherit)] annotations fixed in d20c21f — the one flagged here (cursor_store.rs) plus a second instance the same scan found in dependency_resolution.rs (ResolutionError::Api). Root cause: the re-annotation pass inserted inherit on variants the original migration had already annotated.

The macro now makes this class of mistake impossible: duplicate #[retry(...)] keys are a spanned compile error as of #3753 (193d75b), with trybuild coverage. All goldens + crate suites green, fmt/clippy clean.

[codecov]

Codecov Report

❌ Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 84.83%. Comparing base (8a5996e) to head (e23fad2).

Files with missing lines	Patch %	Lines
crates/xmtp_mls/src/worker/device_sync/mod.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@                     Coverage Diff                     @@
##           insipx/retryable-derive    #3754      +/-   ##
===========================================================
+ Coverage                    84.47%   84.83%   +0.35%     
===========================================================
  Files                          409      407       -2     
  Lines                        60089    59573     -516     
===========================================================
- Hits                         50759    50537     -222     
+ Misses                        9330     9036     -294     

☔ View full report in Codecov by Harness.
📢 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.

Dismissing prior approval to re-evaluate 526c596

Dismissing prior approval to re-evaluate 89a43a6

Dismissing prior approval to re-evaluate e23fad2