Skip to content

feat(mls,bindings): bound resume_streams + return partial catch-up summary on deadline#3881

Open
tylerhawkes wants to merge 1 commit into
mainfrom
tyler/lifecycle-catchup-hardening
Open

feat(mls,bindings): bound resume_streams + return partial catch-up summary on deadline#3881
tylerhawkes wants to merge 1 commit into
mainfrom
tyler/lifecycle-catchup-hardening

Conversation

@tylerhawkes

@tylerhawkes tylerhawkes commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Two robustness fixes to the app-backgrounding stream lifecycle, both patchable post-ship but worth landing before the surface freezes for the release.

resume_streams is now bounded (10s)

Previously it awaited reconnect with no deadline, so a foreground while offline could hang the caller — and the SDK's serial lifecycle reconciler — indefinitely (join_all hangs on one wedged wire). It now returns after the deadline and logs; the background reconnect keeps retrying independently, so dropping the wait is safe.

catch_up_to_live returns partial counts on a deadline

Previously a background-fetch that delivered N messages then hit its timeout_ms reported messages: 0 (the FFI wrapped the future in tokio::time::timeout and threw away the accumulator), so the host reported noData and the OS throttled future background wakeups — despite N messages being persisted.

The deadline now lives in the core loop (catch_up_bidi), where the accumulated &mut summary is owned and survives the cancelled attempt. Core CatchUpSummary gains a completed flag. The legacy fallback (a terminal store-diff, no partial to hand back) returns completed = false / zero on the deadline, documented.

Also regenerates xmtpv3.swift for the updated FFI doc comments and flips the cancel-safe lifecycle test to assert partial counts.

Verification

  • cargo check + clippy — clean on the changed crates
  • 6/6 v3 lifecycle tests pass
  • compiles under --features d14n

🤖 Generated with Claude Code

Note

Bound resume_streams and return partial catch-up summary on deadline

  • resume_streams now times out after 10 seconds via tokio::time::timeout, returning Ok(()) while the reconnect continues in the background instead of awaiting it unboundedly.
  • catch_up_to_live accepts an optional timeout: Option<Duration>; bidi catch-up returns partial counts with completed=false on deadline, and legacy catch-up returns zeros with completed=false on deadline.
  • CatchUpSummary gains a completed boolean field so callers can distinguish full runs from deadline-cut runs; the FFI converter now maps this field instead of hardcoding true.
  • Behavioral Change: callers passing a timeout to catch_up_to_live now receive partial (non-zero) counts rather than zeroed counts when the deadline is hit.
📊 Macroscope summarized 8f42e44. 3 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted

🗂️ Filtered Issues

No issues evaluated.

@tylerhawkes
tylerhawkes requested a review from a team as a code owner July 21, 2026 21:59
@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

Claude finished @tylerhawkes's task —— View job


Code Review — July 21, 2026 22:00 UTC

Todo List

  • Read and analyze all 5 changed files
  • Review timeout handling in resume_streams
  • Review partial count accumulation in catch_up_to_live
  • Review test changes
  • Check for potential issues (bugs, performance, security)
  • Post comprehensive review feedback

Summary

This PR successfully addresses two real robustness issues in the app-backgrounding lifecycle. The implementation is sound and well-designed. The timeout handling is cancel-safe, the partial-count accumulation correctly survives deadline cuts, and the test coverage validates the key properties.

Detailed Review

resume_streams Bounded (10s Timeout)

bindings/mobile/src/mls.rs:220-233

The 10-second timeout wrapping resume_bidi_streams() is the right fix:

  • Prevents indefinite hangs when foregrounding while offline (previously join_all could wedge on one stuck wire)
  • Safe to drop the wait — the background reconnect loop keeps retrying independently
  • Good ergonomics — returns quickly to unblock the caller/lifecycle reconciler, logs the timeout, reconnect continues in background

Minor observation: The constant RESUME_DEADLINE is well-named and the debug log message is helpful for diagnosing timeout scenarios.

catch_up_to_live Partial Counts on Deadline

crates/xmtp_mls/src/subscriptions/catch_up.rs:217-251 & 261-297

The core improvement is moving the timeout from the FFI layer (which threw away the accumulator) into catch_up_bidi, where &mut summary is owned:

Key strengths:

  1. Cancel-safe accumulation (catch_up.rs:265): summary declared before the retry loop, mutated across attempts, survives deadline cuts
  2. Timeout placement (catch_up.rs:268-273): tokio::time::timeout_at wraps each attempt; on Err(_) the function returns the partial summary immediately
  3. Backoff respects deadline (catch_up.rs:287-289): Won't sleep past the deadline; returns partial instead
  4. completed flag (catch_up.rs:119-123): Clear contract — false means "cut short, counts are partial"
  5. Legacy fallback documented (catch_up.rs:242-248): Returns CatchUpSummary::default() (zero counts, completed=false) on deadline since it has no partial to return

bindings/mobile/src/mls.rs:791-807

The FFI now passes the timeout into core instead of wrapping the future:

  • Correct: The timeout is now Option<Duration> passed to core, which owns the accumulator
  • Doc comments updated: Clearly explain partial counts on deadline (bidi path) vs zero (legacy fallback)

✅ Test Changes

bindings/mobile/src/mls/tests/lifecycle.rs:186-247

The cancel-safety test (bidi_catch_up_to_live_bounded_run_is_cancel_safe) is updated correctly:

  • Lines 215-221: Now asserts that a cut-short run (!bounded.completed) has a partial count ≤ 5 (what was owed)
  • Lines 225-240: The full unbounded run still proves convergence regardless of the cut
  • Key property validated: Partial counts are meaningful, and the cut leaves no corrupt state

The test correctly exercises the new behavior: a 1ms deadline likely fires mid-processing, the partial count is checked, then a full run proves idempotency.

✅ Node Bindings

bindings/node/src/client/catch_up.rs:14-21

The Node bindings correctly drop the completed field from their CatchUpSummary struct — Node's catch_up_to_live is unbounded (no timeout_ms parameter), so it always completes. Clean separation of concerns.

✅ Generated Swift Code

sdks/ios/Sources/XMTPiOS/Libxmtp/xmtpv3.swift

The uniffi-generated Swift correctly reflects the updated doc comments and FFI signature changes (checksums updated). No issues.


Issues Found

None. The implementation is correct.


Suggestions (Non-blocking)

  1. Consider logging partial counts on deadline (catch_up.rs:273): When returning a partial summary due to timeout, a tracing::debug! with the counts could help diagnose whether the deadline was too aggressive or if progress is being made.

    Err(_) => {
        tracing::debug!(
            messages = summary.messages,
            conversations = summary.conversations,
            "catch_up_to_live: deadline reached; returning partial summary"
        );
        return Ok(summary)
    }
  2. Document the legacy fallback zero-count behavior more prominently: The FFI doc comments mention it (mls.rs:794-796), but hosts might not realize that completed=false + messages=0 on the legacy path doesn't mean "no work done" — it means "timeout fired, but messages were still persisted, just not counted." Consider a warning in release notes or migration guides if this surface is already shipped.


Verification Checklist

  • Correctness: Timeout handling is cancel-safe; &mut summary survives the cut
  • Test coverage: The lifecycle test validates partial counts and cancel-safety
  • Documentation: Doc comments accurately describe the new behavior
  • No regressions: Legacy fallback and unbounded paths still work as before
  • Compiles: Author reports clean cargo check + clippy, compiles with --features d14n
  • Tests pass: 6/6 v3 lifecycle tests pass

Recommendation

Approve and merge. This is a solid fix for two real production hazards (indefinite resume_streams hang, noData reports despite background-fetch work). The implementation is cancel-safe, well-tested, and the tradeoffs are documented.


&self,
timeout: Option<Duration>,
) -> Result<CatchUpSummary, CatchUpError> {
let deadline = timeout.map(|d| tokio::time::Instant::now() + d);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium subscriptions/catch_up.rs:221

catch_up_to_live computes the deadline with tokio::time::Instant::now() + d. When a caller passes an oversized Duration (e.g. derived from a large u64 millisecond value), the addition panics because the resulting Instant is not representable. Consider using checked_add and falling back to no deadline (or clamping) when the sum overflows.

Suggested change
let deadline = timeout.map(|d| tokio::time::Instant::now() + d);
let deadline = timeout.and_then(|d| tokio::time::Instant::now().checked_add(d));
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @crates/xmtp_mls/src/subscriptions/catch_up.rs around line 221:

`catch_up_to_live` computes the deadline with `tokio::time::Instant::now() + d`. When a caller passes an oversized `Duration` (e.g. derived from a large `u64` millisecond value), the addition panics because the resulting `Instant` is not representable. Consider using `checked_add` and falling back to no deadline (or clamping) when the sum overflows.

@macroscopeapp

macroscopeapp Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs human review

1 blocking correctness issue found. An unresolved review comment identifies a potential panic when computing deadlines with oversized Duration values. This substantive bug concern should be addressed before approval.

You can customize Macroscope's approvability policy. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant