fix(xmtp_mls,xmtp_proto): durable AddMissingInstallations reconciliation on the TaskRunner

[insipx]

Problem

The node-sdk DeviceSync > should sync messages across installations using sendSyncRequest and syncAllDeviceSyncGroups test has been failing most CI runs since Jul 23 (10 of 14 node-sdk jobs across main and several branches), always with the same signature: the group never appears on the second installation for the full 90s poll, while the same code sometimes converges in ~3s.

Root cause: when a new installation registers, the DeviceSync worker's NewSyncGroupFromWelcome handler ran add_new_installation_to_groups() inline, exactly once:

• per-group failures inside queue_for_each were warn-logged and swallowed (groups/intents/queue.rs), so a single lost race (e.g. MissingSequenceId when the membership add races identity propagation — see the diagnosis on tyler/seqid-retryable) silently skipped the add;
• the triggering event is never re-fired and nothing else in the test scenario sends a message that would run the add-missing-installations-on-send fallback, so the miss is permanent — 90s of re-issued sync requests can't recover it (with elements: [] the archive round trip is empty, so the welcome path is the only delivery path).

Retryability classification alone (tyler/seqid-retryable) can't fix this: the queue-time errors are dropped regardless of classification, and an abandoned publish leaves the intent dormant with nothing to re-trigger it.

Fix

Make the reconciliation durable: the welcome handler now enqueues one AddMissingInstallations { group_id } task per eligible group on the TaskRunner (create_or_ignore_task, deduped by payload hash, then a wake). The TaskRunner arm loads the group and runs the existing idempotent MlsGroup::add_missing_installations(); failures ride the standard task backoff (2s ×1.5 capped at 60s, 20 attempts, 3-day expiry), missing/malformed groups delete the task.

• xmtp_proto: new Task oneof variant AddMissingInstallations (tag 7) hand-added to the checked-in generated files ahead of the upstream xmtp/proto PR (same pattern as the earlier KpMaintenance work). Wire encoding pinned in data_hash_encoding_is_pinned.
• xmtp_mls: schedule_add_installations_to_groups() replaces the inline bulk add; new TaskRunner arm; queue_for_each (now dead, and the error-swallowing culprit) removed.
• tests: enqueue+dedup unit test, missing-group task-deletion unit test, live end-to-end integration test (welcome → task → commit → welcome). test_only_added_to_correct_groups polls for convergence (the metric now fires at schedule time), and test_has_same_sync_group_as converges on epoch too — the post-join KeyUpdate commit riding the first HMAC-cycle message landed deterministically inside its old assert window.

Verification

• xmtp_mls 707/707, xmtp_db + xmtp_proto 322/322 (cargo nextest, ci profile)
• just lint-rust clean
• node-sdk DeviceSync.test.ts (including the exact failing CI test) 3/3 green against freshly built bindings

Follow-ups

• Upstream xmtp/proto PR adding the variant (message + oneof field 7 — field numbers are wire-pinned by persisted task rows), then dev/gen_protos.sh regen + proto_version bump.
• Independent JS-side deflake: the DeviceSync tests request elements: [], which produces an empty archive; requesting Messages would make each retry attempt self-sufficient.

🤖 Generated with Claude Code

Note

Add durable AddMissingInstallations task reconciliation to the TaskRunner

• Introduces a new AddMissingInstallations proto task variant and wires it into the TaskWorker dispatch loop, replacing inline membership adds with retryable, deduplicated durable tasks.
• When add_missing_installations() fails during sync group creation in DeviceSyncClient, a durable task is scheduled instead of silently dropping the add.
• SyncWorker.evt_new_sync_group_from_welcome now schedules tasks via schedule_add_installations_to_groups rather than executing inline adds, and recovers from broadcast receiver lag by re-scheduling reconciliation.
• The TaskWorker deletes AddMissingInstallations tasks permanently on malformed group ID or group not found, and retries on transient errors.
• Behavioral Change: QueueIntentBuilder::queue_for_each is removed; callers must use the new task-based scheduling path.

^{Macroscope summarized 761b197.}

[graphite-app]

How to use the Graphite Merge Queue

Add the label mergequeue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

[claude]

Claude finished @insipx's task —— View job

---

PR Review - 2026-07-24 20:30 UTC

Overview

Strong architectural improvement that moves installation reconciliation from fragile one-shot inline execution to durable task-based retry with backoff. The root cause diagnosis is clear, the fix is well-tested, and the implementation follows established patterns from the KpMaintenance work.

Key Observations

✅ Strengths:

• Problem correctly identified: Tyler's point switch hkdf to use RustCrypto hdkf and sha2 #3 confirms the Blacksmith migration timing correlation—this wasn't just flaky, it was a latent race condition exposed by fleet changes
• Durability pattern: create_or_ignore_task + dedup by payload hash is the right fix for lost edge-triggered events
• Test coverage: New tests verify task creation, deduplication, missing-group cleanup, and end-to-end convergence
• Removal of error-swallowing code: queue_for_each is gone—good riddance to the silent failure path

🔍 Areas warranting attention:

1. Tyler's points Refactor to separate wasm bindings from rust crate #1 & Implement hkdf #2 remain unaddressed (per his comment at 18:22 UTC):

   • Point Refactor to separate wasm bindings from rust crate #1: get_sync_group's creation branch (mod.rs:370-379) still runs add_missing_installations().await inline exactly once. The new code only arms the task on failure of the inline add. If the inline add succeeds, no task is created. Tyler's clarification suggests enqueueing the task regardless (or as a "safety net") would close the gap. Current behavior: task only created when inline fails → success path still has no durable reconciliation for the sync group itself.
   • Point Implement hkdf #2: The Lagged recovery (worker.rs:143-154) is good, but Tyler notes the schedule step itself is still at-most-once from the event edge. If NewSyncGroupFromWelcome arrives and gets processed but then the receiver lags before the task is flushed to disk, the event is gone. Running schedule_add_installations_to_groups() once on worker (re)start would make it level-triggered. Status: partial fix—Lagged case is covered, but cold-start isn't.

2. Coverage gap (worker.rs:148-152, lines 6 missing coverage per Codecov): The Lagged recovery path that re-schedules installations isn't exercised by tests. This is a critical failure recovery path—it should have explicit test coverage (simulate a lagged receiver and verify task creation).

3. Minor: Task deletion on malformed group_id (tasks.rs:510-516): The code deletes tasks when GroupId::try_from fails. This is correct for data integrity, but there's no logging of the actual malformed bytes (only the warning). For debugging wire-format issues during the proto transition, consider hex-logging the malformed group_id bytes (not load-bearing, but helpful).

Recommendations

Priority 1 (addresses Tyler's feedback):

• Consider arming the durable task in get_sync_group's creation path even when inline add succeeds (as a safety net), or document why inline-only is acceptable for the sync group itself
• Add explicit test coverage for the Lagged recovery path in worker.rs
• Consider calling schedule_add_installations_to_groups() once on sync worker startup to make the system level-triggered (Tyler's point Implement hkdf #2)

Priority 2 (test coverage):

• Add a test that simulates RecvError::Lagged and verifies task creation (currently 0 coverage on the recovery path)

Priority 3 (observability):

• Log hex-encoded group_id bytes when deleting malformed tasks during the proto transition

Verdict

Architecture is sound, implementation follows best practices, and the tests validate the happy path. Tyler's points #1 and #2 deserve follow-up (either as changes or documented rationale), and the Lagged path needs test coverage. With those addressed, this is a solid fix for the flaky DeviceSync test.

No blocking issues for merge once Tyler's concerns are addressed.

---

[macroscopeapp]

Approvability

Verdict: Needs human review

Unable to check for correctness in f841511. This PR changes the device sync reconciliation from inline execution to durable task-based execution with retry/backoff. While the author owns all changed files and the change is well-tested, the architectural shift in execution model and timing semantics warrants human review.

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

[macroscopeapp]

Approvability

Verdict: Needs human review

Unable to check for correctness in f841511. This PR changes the device sync reconciliation from inline execution to durable task-based execution with retry/backoff. While the author owns all changed files and the change is well-tested, the architectural shift in execution model and timing semantics warrants human review.

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

[macroscopeapp]

Approvability

Verdict: Needs human review

This PR fundamentally changes installation reconciliation from inline synchronous processing to durable task-based execution with retry/backoff. While the author owns all changed files and the fix improves reliability, the architectural changes to core device sync behavior warrant human review.

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

[insipx]

Blocked on xmtp/proto#341 — this PR's generated-code changes for the AddMissingInstallations variant must land upstream first. The gen files here are now verbatim dev/gen_protos.sh output from that proto branch (including proto_descriptor.bin); after xmtp/proto#341 merges, the only remaining step is a proto_version repin + regen commit (expected no-op diff). Marked draft until then.

🤖 Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 87.34177% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.75%. Comparing base (349b92b) to head (761b197).

Files with missing lines	Patch %	Lines
crates/xmtp_mls/src/worker/device_sync/worker.rs	33.33%	6 Missing ⚠️
crates/xmtp_mls/src/worker/tasks.rs	89.47%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3897      +/-   ##
==========================================
- Coverage   85.77%   85.75%   -0.03%     
==========================================
  Files         427      427              
  Lines       68201    68196       -5     
==========================================
- Hits        58501    58483      -18     
- Misses       9700     9713      +13     

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

[tylerhawkes]

@insipx three clarifications from the diagnosis on tyler/seqid-retryable, since this PR lands the durable half of it:

1. get_sync_group's creation branch still has the one-shot shape this PR removes elsewhere: create_and_insert persists the row, then add_missing_installations().await? runs inline exactly once — if that fails, the Some(...) branch never re-attempts and the sync group stays permanently under-membered. Enqueueing the same AddMissingInstallations task for the just-created group (instead of, or as a safety net behind, the inline add) closes it with machinery this PR already builds.

2. The schedule step itself is still at-most-once from the event edge: run_internal's while let Ok(event) = receiver.recv().await exits on a broadcast Lagged, the supervisor restarts the loop on the same receiver, and the lagged events — including the NewSyncGroupFromWelcome that would have scheduled the tasks — are gone with no task row ever created. schedule_add_installations_to_groups() is cheap and dedup'd, so running it once on worker (re)start or on Lagged recovery makes the scheduling level-triggered instead of edge-triggered.

3. For the writeup's why-now: the onset matches the Blacksmith migration (ci: migrate Linux CI to Blacksmith runners with sticky-disk /nix #3883, merged Jul 22 17:40 UTC) — zero failures of this test in the 40 node-sdk runs before it, ~85% of shard-instances during US-night hours after, identical trees passing and failing on either side of the boundary. The race predates Jul 23; the fleet change flipped the odds. That also explains the 3s-vs-never bimodality in the Problem section.

#3895 stays complementary: each task attempt's add_missing_installations re-runs the identity fetch under the intent machinery's retry loop, which the retryable classification re-arms.

Dismissing prior approval to re-evaluate 15dd817

Dismissing prior approval to re-evaluate a69b5f0