feat(xmtp_mls): verify our own key package is live on the network

[insipx]

The problem

Key-package rotation is driven purely by a local deadline column
(identity.next_key_package_rotation_ns). rotate_if_needed reads
is_identity_needs_rotation(), and if the column says "not due" it returns
Ok(false) and does nothing. Nothing in the client ever verifies that a usable key
package is actually published on the network.

When that column is wrong, the failure is silent, total, and self-sustaining:

1. The client stops rotating. Its key package passes not_after and becomes unusable.
2. fetch_key_packages for that installation returns nothing.
3. Anyone adding it to a group records it in failed_installations instead of emitting
   an Add proposal. It is listed as a group member with no MLS leaf.
4. With no leaf it never receives a welcome, so the existing welcome-driven rotation
   nudge (queue_key_rotation) never fires either.
5. The client observes no local error at all. It syncs, sends messages, and creates
   groups against the groups where it still has valid leaves.

Closed loop, no exit, no logs.

A dev-network installation reached exactly this state and stayed unreachable for weeks.
Across seven client builds in a 31-hour window, with rotation roughly two months overdue,
the "Start rotating keys and uploading the new key package" INFO line never appeared
once. Its absence was the only externally visible symptom. A comparison client on the
same build and network rotated normally, so the rotation machinery itself was fine.

The systemic gap: local rotation state is never reconciled against network reality.

What this adds

KpLiveness — a recurring TaskRunner singleton that probes the network for this
installation's own key package on a ~24h throttle, and queues a rotation when it is
absent, unverifiable, or close to expiry.

KpLiveness dispatch
  ├─ throttled (checked within the interval)  -> reschedule, no network
  └─ probe fetch_key_packages([my_installation_id])
        ├─ valid, ours, comfortable lifetime  -> INFO, stamp, +24h
        ├─ absent | unverifiable | expiring   -> WARN, queue_key_rotation(), +24h
        └─ inconclusive (offline, 5xx, defect) -> WARN, do NOT stamp, +1h

It is seeded next to KpRotation/KpDeletion. create_or_ignore_task keeps an existing
row's deadline, but a fresh seed is due immediately — so the first client build after
upgrade checks at once, which is what rescues installations that are already broken.

Repair reuses queue_key_rotation, the same already-tested path the welcome nudge uses.
Liveness never rotates inline.

Why a separate task, not a branch inside KpRotation

Folding the probe into the existing KpRotation arm was evaluated and rejected:

• Schedule. Liveness needs ~24h; KpRotation's deadline follows the rotation column
  (~30 days). Sharing a row means multiplexing two schedules onto one column — and
  deriving the watchdog's cadence from the very column whose corruption it detects.
• Retry budget. A network probe fails routinely on offline clients. A shared row means
  a failing probe drives rotation into exponential backoff, and vice versa.
• Nudges. PullInDeadline targets a task by data_hash. With one row there is no way
  to say "verify now" as distinct from "rotate now".
• It would need the same DB migration anyway, so it avoids the proto work, not schema work.

A watchdog that shares a schedule and a retry budget with its subject fails with its subject.

Details worth reviewing

Several of these came out of an adversarial review pass and are the non-obvious parts:

• "Absent" has two wire shapes. A short response is MismatchedKeyPackages, but the
  network actually returns a present-but-empty payload — exactly what
  xdbg query fetch-key-packages showed for the affected installation. Both mean absent.
  This is why the probe calls the API directly instead of MlsStore, which collapses an
  empty payload into a generic verification error.
• The API matches responses to requests by position. A backend/cache/ordering defect
  can return a perfectly valid key package belonging to someone else. It verifies, it has
  a healthy lifetime, and we would record ourselves as reachable while staying unaddable —
  rebuilding the exact loop this exists to break. The probe compares the leaf signature key.
  A mismatch is Inconclusive, not unhealthy: an upload cannot repair a mapping defect,
  and rotating would make one backend incident churn the whole fleet every interval.
• Only "asked for 1, got 0" proves absence. Any other count mismatch is a backend
  defect, classified Inconclusive for the same reason.
• The throttle column is never trusted blindly. A stamp in the future — clock skew at
  write time, or corruption — would suppress the watchdog for months. That is byte-for-byte
  the failure this check exists to catch, so the fix must not reproduce it.
• Clamping the stamp is not enough. The same clock jump also writes a future deadline
  onto the task row, and the dispatcher skips future-dated tasks — so the handler's clamp
  becomes unreachable because the handler never runs. Startup reconciliation therefore
  enqueues the liveness pull-in unconditionally.
• Probe failures return a deadline, never Err, so an offline client cannot drive this
  task into the TaskRunner's backoff. Genuine storage errors still propagate for the
  DB-reconnect contract.
• Observability. rotate_if_needed now logs at INFO on both branches. The silent
  "not due" branch is what hid the incident.

A commit-driven trigger (our own installation appearing in failed_installations) was
built and then removed: it wrote durable task rows from unvalidated remote input,
before expected_diff_matches_commit and the credential checks — and it is near-unreachable
anyway, since an installation in failed_installations has no leaf and cannot decrypt the
commit naming it.

Known limitations, including the pre-existing unclaimed-TaskRunner race and its real
destructive worst case, are documented in §5a of the design doc rather than glossed.

Testing

crates/xmtp_mls/src/worker/key_package_maintenance.rs. Classification is extracted into
pure functions (now_ns is a parameter, not a hidden clock) and asserted directly over
every wire shape — no network, no override — so they cannot go false-green if the probe body
changes. A live-backend test additionally pins real backend behavior; that is the test that
found the empty-payload shape.

Covers: healthy → no rotation; absent → rotation queued and nudged; throttle blocks a
repeat probe; nudges coalesce to one pull-in; future stamp does not disable the check;
a clock-skew-stranded row is confirmed non-dispatchable then rescued by reconciliation;
every next_liveness_deadline boundary; every response shape including a valid key package
for a different installation.

• just lint-rust — clean
• cargo nextest run -p xmtp_mls -p xmtp_db — 936 passed (should_reconnect is
  known-flaky and retried green)
• just wasm check — passes

Dependency

Requires xmtp/proto#342, already merged as f53e21af.
crates/xmtp_proto/proto_version pins that rev and the generated Rust was regenerated
through dev/gen_protos.sh — not hand-edited. The regenerated diff covers only the new
KpLiveness message and its oneof arm.

Design doc

docs/superpowers/specs/2026-08-07-kp-liveness-check-design.md

🤖 Generated with Claude Code

Note

Add key package liveness check to verify own key package is live on the network

• Introduces a new KpLiveness task that periodically probes the network to verify the installation's own key package is present, valid, and not near expiry.
• Probes classify the network result into Healthy, Absent, Unverifiable, ExpiringSoon, or Inconclusive; actionable outcomes (Absent, Unverifiable, ExpiringSoon) queue a key package rotation.
• Throttling is managed via a new key_package_liveness_checked_at_ns column in the identity table, with constants for check interval, retry interval, and minimum remaining lifetime.
• The liveness task is seeded and reconciled at startup; KpRotation dispatches now also nudge the liveness task unconditionally.
• Adds a new KpLiveness proto variant and DB migration for the liveness timestamp column.

^{Macroscope summarized 8cf9617.}

[claude]

Claude finished @insipx's task —— View job

---

PR Review - Key Package Liveness Check

Review Date: 2026-08-07T23:46:09Z

This is a well-engineered solution to a critical reliability problem. The implementation shows careful attention to edge cases, failure modes, and defensive design. The code quality is excellent with comprehensive test coverage and thorough documentation.

---

✅ Strengths

1. Design Quality

• Structural independence: Separate KpLiveness task avoids coupling the watchdog to the thing it watches - exactly the right choice given the problem is a wrong rotation deadline
• Pure functions: Classification logic (classify_key_package, classify_fetch_error) takes now_ns as a parameter rather than hiding the clock, making tests reliable and avoiding test-only code paths
• Defense in depth: Multiple recovery paths (scheduled check, rotation handoff, startup reconciliation) ensure the watchdog can't be permanently disabled

2. Edge Case Handling (crates/xmtp_mls/src/worker/key_package_maintenance.rs)

• Two wire shapes for "absent": Lines 208-209 handle both missing entry AND empty payload - the test at line 1065-1072 confirmed real backend behavior
• Positional API defect detection: Lines 228-235 catch backend mapping errors (returning someone else's valid key package) and classify as Inconclusive rather than triggering unnecessary rotation - prevents one backend incident from churning the entire fleet
• Count mismatch handling: Lines 183-193 only treat "asked for 1, got 0" as absence; any other count is a backend defect

3. Clock Skew Defense (key_package_maintenance.rs:297-314, identity.rs:277-288)
The dual-layer protection against forward clock jumps is particularly well thought out:

• Handler-level clamp (lines 302-309) logs and treats future stamps as "due"
• But that's unreachable if the task row itself has a future deadline (dispatcher skips it)
• Startup reconciliation (lines 502-511) unconditionally enqueues a liveness pull-in to rescue stranded rows
• Test at lines 977-1022 confirms both the problem (precondition check) and the fix

4. Test Coverage
12 targeted tests with clear names that match their assertions. Tests use real DB/storage, not mocks. Classification tests operate on raw wire shapes without network calls, so they can't go false-green if probe logic changes.

---

🔍 Observations (not issues)

1. Documented Known Limitation - Concurrent TaskRunners (design doc §5a, lines 350-390)
The multi-TaskRunner race (worst case: concurrent rotations upload A then B, B completes first marking A for deletion, A lands last, material for A deleted after grace period → unreachable) is:

• Pre-existing in KpRotation / rotate_and_upload_key_package
• Not made more likely by this change (liveness only queues rotation when unhealthy)
• Correctly deferred: The fix (atomic task claiming/leases) belongs in TaskRunner and benefits all task types
• Operating assumption of "one TaskRunner per database" is already implicit in existing design

2. TaskRunner Coupling (design doc §5a, lines 391-397)
Liveness runs serially in the same TaskRunner, so a slow/stuck task delays the probe. A fully independent watchdog would be its own worker with its own timeout budget. This is acknowledged as a design tradeoff - the observed failure mode doesn't require that level of independence.

3. Network Probe Writes Acknowledged (design doc §9)
One fetch_key_packages round trip per client per day. At scale this is measurable backend load, but it's the necessary cost of the safety property (detect when local state diverges from network reality).

---

🎯 Suggestions (optional improvements)

1. Consider Metric/Tracing for Inconclusive Backend Defects (key_package_maintenance.rs:229-235, 190)
The ERROR log at line 229 for wrong-installation responses is good, but these are backend mapping defects that should probably trigger metrics/alerts. Consider adding structured logging or metrics for:

• classify_fetch_error returning Inconclusive (count mismatches)
• Installation ID mismatches in classify_key_package

These are rare failure modes that indicate infrastructure problems worth tracking separately from normal "offline client" inconclusive outcomes.

2. Migration Reversibility (down.sql)
The down migration drops the column. Existing deployments won't have data to preserve, but if this ever needs to be rolled back after running in production, the NULL-means-due semantics mean data loss is safe (clients just probe once on next startup). This is fine, just noting that the down migration is destructive.

---

✅ Security Assessment

• No new vectors. The probe is a read-only fetch_key_packages call for the client's own installation
• Rejected commit-driven trigger (design doc §3, "Rejected trigger") correctly avoided writing durable state from unvalidated remote input before security checks
• Throttle prevents a malicious nudge from causing unlimited network probes
• Constants have no test override that could leak into production (lines 260-278 in design doc explain the CI break that led to this decision)

---

✅ Performance Assessment

• Network: 1 fetch per client per day when healthy; up to 1 per hour when offline/inconclusive
• DB writes: Throttle stamp written once per conclusive check (~daily); nudges coalesce via data_hash so at most one pending pull-in row
• No hot path impact: Check runs on TaskRunner's schedule, not in message send/receive path

---

✅ Code Quality

• Follows CLAUDE.md instructions: uses #[xmtp_common::test(unwrap_try = true)], no unnecessary comments, proper error propagation
• Constants properly located in common/mls.rs with clear documentation (lines 27-47)
• DB layer properly separates concerns: QueryIdentity trait for column access, pure business logic in worker module
• Error handling forwards needs_db_reconnect correctly through all layers (lines 45-57, 70-90 in tasks.rs)

---

Summary

Approve with confidence. This PR solves a real production incident with a well-designed, thoroughly tested, and carefully documented solution. The author has clearly thought through failure modes, documented known limitations honestly, and made sound engineering tradeoffs. The CI fix in commit 8cf9617 was the right correction (realistic intervals, direct column writes in tests instead of sleeps).

The only suggestions above are optional observability improvements; the code is production-ready as-is.

---

[macroscopeapp]

Approvability

Verdict: Needs human review

This PR introduces a new key-package liveness watchdog feature that adds daily network probes and automatic repair mechanisms for MLS key management. While well-documented and tested with strong author ownership, the significant new runtime behavior affecting core cryptographic key handling warrants human review.

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

[insipx]

CI fix pushed (f33e167 → 8cf9617)

test-workspace and test-android failed on the first push. Root cause was a test-config value, not the feature logic — but the failing test was right and I was wrong.

What broke. I had given KEY_PACKAGE_LIVENESS_INTERVAL_NS a 3-second override in test/mls.rs so throttle tests could sleep past it. That made any client built more than 3s after registration probe the network on startup. bindings/mobile's create_client_does_not_hit_network asserts that building a client performs no network I/O, and it correctly caught this. The Android job showed the same cause from a different angle: it reached 200/202 with 0 failures and then timed out, with the final tests crawling at ~90s each — extra probes across 202 client builds.

The fix. All three liveness constants now live in common/mls.rs with no prod/test split, so the interval is a realistic 1 day everywhere. Tests that need the check to run write the throttle column directly via set_key_package_liveness_checked_at_ns instead of sleeping. That is both correct and faster — it removed ~8s of sleeping from the suite.

The short interval was the bug. The assertion was load-bearing, and shortening a production interval purely to make a test convenient is what broke it.

Verified locally, matching what CI runs:

• cargo nextest run --profile ci (v3) — 2478/2478 pass
• cargo nextest run --features d14n --profile ci-d14n -E 'package(xmtp_mls)' -E 'rdeps(xmtp_mls)' — all pass
• create_client_does_not_hit_network under d14n — passes (previously failed 4/4)
• cargo clippy --locked --all-features --all-targets -- -Dwarnings and cargo fmt --check — clean

I had only run the v3 profile before the first push; CI runs v3 and d14n, which is how this reached CI at all. My mistake.

Also worth noting for reviewers: this incidentally resolves the review bot's suggestion about co-locating KEY_PACKAGE_LIVENESS_MIN_REMAINING_LIFETIME_NS — all three liveness constants are now together in common/mls.rs. Section 4 of the design doc records why there is deliberately no test override.

[codecov]

Codecov Report

❌ Patch coverage is 96.38554% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.96%. Comparing base (4742763) to head (8cf9617).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
...tes/xmtp_mls/src/worker/key_package_maintenance.rs	97.65%	9 Missing ⚠️
crates/xmtp_db/src/encrypted_store/identity.rs	78.57%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3954      +/-   ##
==========================================
- Coverage   86.02%   85.96%   -0.06%     
==========================================
  Files         417      417              
  Lines       68092    68565     +473     
==========================================
+ Hits        58578    58944     +366     
- Misses       9514     9621     +107     

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

[insipx]

CI status on 8cf9617: the real failure is fixed; the one remaining failure is a known repo-wide flake

Fixed and green: test-workspace / Test (Rust Workspace) now passes. That was the genuine regression from the 3s test-interval override, described in the previous comment. Everything else is green — WASM, Node, browser-sdk (all 4 shards), iOS, Android unit tests, cross-version, cross-talk, lint, cargo-deny.

Remaining failure: test-android / Integration Tests (Android) — this is #3953, not this PR.

Evidence it is pre-existing and unrelated:

• It fails on main in 4 of the last 5 failing main runs, including 31223948308 and 31221165776 from earlier today, both predating this branch.
• The signature matches Flaky CI Failure: Android Integration Tests — Run integration tests times out after 200/202 tests complete #3953 exactly: progress is brisk to 194/202, then the last few tests crawl, the job reports 0 failed, and the step times out. No test asserts anything wrong; the step budget is consumed elsewhere.
• I initially suspected my own change caused it, since the first run on this branch did add extra per-client network I/O. That turned out to be wrong. Removing those probes did not fix it — the second run failed identically (201/202 vs 200/202, i.e. noise). I have corrected my earlier comment on Flaky CI Failure: Android Integration Tests — Run integration tests times out after 200/202 tests complete #3953 accordingly rather than leave a bad lead there.

One detail I contributed back to #3953 that may help whoever picks it up: the tail crawl has an exact 90-second cadence (six consecutive intervals at 90s to the second), which looks like a fixed timeout being hit rather than gradual resource saturation.

Not merging or force-passing anything — flagging it so the red check is not mistaken for a defect in this PR. Happy to rebase or re-run if you'd like a fresh roll of that job.