crypto: auto-heal split-brain device_keys state on for_user #88
No reviewers
Labels
No labels
blocked
bug
documentation
duplicate
enhancement
good first issue
help wanted
invalid
question
waiting-on-julian
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
jlxq0/matrix-mcp!88
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "crypto/auto-heal-device-keys"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Root cause
matrix-rust-sdk's per-account
sharedflag (a.k.a.device_keys_published) latchestrueon the first successful/keys/uploadand is never re-verified against the homeserver. If the upload was actually rejected — e.g. /setup attempting its own unbound bearer, a transient network blip, or a later admin-side device wipe — the SDK keeps operating against a device that doesn't exist server-side. Every subsequent/keys/uploadis short-circuited (keys_for_upload()returnsNonewhileshared == true); the device shows up "Unverified — doesn't support encryption" in Element even though OTKs are uploaded;/setup/recover's self-sign attaches to local keys that never landed on Synapse.Repro'd on 2026-05-21:
MATRIXMCP2had 50 OTKs ine2e_one_time_keys_jsonand zero rows ine2e_device_keys_json. matrix-rust-sdk emitted the canonical log line"Our own device might have been deleted"on every sync but no remediation triggered.What this PR does
MatrixClientCache::for_usernow post-verifies (Client::encryption().get_user_devices(...)) once per cached client. On a negative result it evicts, wipes the per-MXID store subdirectory, and recurses exactly once. A second negative after rebuild surfaces as an error rather than looping. The wipe is scoped to one MXID's hash directory under the store root — never the root itself, never another user's data.src/matrix_client.rs: refactorfor_user→for_user_inner(retry: bool), new helpersverify_or_healandverify_device_keys_published, newwipe_user_storeon the cache, newkeys_verified: AtomicBoolonCachedClient. New tests for the wipe (scope + idempotency) and the metric.src/metrics.rs: newmatrix_mcp_client_cache_self_heal_total{reason="..."}IntCounterVec. Single reason label for now (device_keys_missing); the design leaves room for future heal causes.src/setup.rs: comment refresh — the existingcontains()precondition stays as defense-in-depth; for_user is now the primary guard.Why scoped per-MXID wipe is the right level
sharedback tofalse. The pickledAccountis a single encrypted blob inkv[key="account"]; surgical mutation would require per-SDK-version schema understanding and is more fragile than a scoped rebuild./setup/recoverjust takes 30s longer; no error, no manual intervention.Why not upstream this to matrix-rust-sdk
A public
Encryption::republish_device_keys()or periodic verification of thesharedflag against/keys/querywould make this workaround vestigial. Worth filing eventually; not in scope for this PR.Observability
Paired with argocd PR #551 which adds a Grafana Loki alert on the SDK warning
"Our own device might have been deleted". The combination is: alert fires when the split-brain happens in the wild → the next /mcp tool call for that user auto-heals → metric increments → alert resolves on its own.🤖 Generated with Claude Code
When matrix-rust-sdk's local `device_keys_published` flag is latched to true but Synapse doesn't actually have our device's keys (e.g. an earlier /keys/upload was rejected with the SDK still flipping the flag), subsequent /keys/upload calls are skipped forever and the device stays invisible to the homeserver. The SDK exposes no public API to clear the flag, so the only recovery is to wipe the per-MXID crypto store and let the SDK rebuild from scratch. `MatrixClientCache::for_user` now post-verifies via /keys/query once per cached client. On a negative result it evicts, wipes the per-MXID store directory, and recurses exactly once. A second negative after rebuild errors out instead of looping. The wipe is scoped to one MXID's subdirectory under the store root — never the root itself, never another user's data. A new `matrix_mcp_client_cache_self_heal_total{reason}` counter records each self-heal so we can alert on it in Loki/Grafana. The /setup precondition stays as defense-in-depth. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>The pre-registered test hits: this is #88, not contention
Read 2026-08-29T06:1x. The prediction was named before looking — one-time keys present with device keys absent is
#88's signature — and either outcome was informative. It came back positive, with the exact repro count.Fifty one-time keys and zero device-key rows is
#88's repro exactly. The device is uploading keys — fifty of them — and none is a device key. The signed control has device keys and fewer OTKs, the healthy shape.Why this ends the diagnosis where the other two did not
matrix-sdk-crypto-0.17.0/src/olm/account.rs:666:let device_keys = self.shared().not().then(|| self.device_keys());— device keys are offered only whilesharedis false, and once it latches truekeys_for_uploadreturnsNonefor device keys forever while one-time keys keep flowing. So the device reaches this state with no upload being wrong at the moment it runs, because after the latch there is no device-key upload to be wrong.That is independent of session count and of churn, which is why neither
contention-by-countnorchurn-recencyexplained it: eight-session KZ9 is fine, seven-session DLX8 with two quiescent windows is not, because the discriminator was never the sessions. It was whethersharedhad latched with device keys never persisted.Contention-by-count and churn-recency were both post-hoc fits on n=2 and both died to a records read. This one named a falsifiable row-count pair first and survived it.
#143,#130,#139are#88.What this does not settle
The fix is the lead's and is upstream in the SDK's latch behaviour, not in a session count. Whether the seven sessions should still be reaped is a separate hygiene question and not the cause. No revoke, no re-add, no code — records only.
This cannot detect the state it exists for
Read against the current measurement:
MATRIXMCP-DLX8SKHKhase2e_device_keys_json rows = 0on Synapse today, after a successful/setup/recover, which is this PR's target state.The heal is right and the trigger cannot fire.
verify_device_keys_publishedreduces to:Encryption::get_user_devicescallsOlmMachine::get_user_devices, which isself.store().get_user_devices(user_id)— the local crypto store, with no homeserver round trip.wait_if_user_pendingwaits for an in-flight query and does not start one.And
Store::get_user_devicescarries this note in the SDK's own source:The memory store's
get_own_devicestates the same invariant as an assertion:.expect("Invalid state: Should always have a own device").So for our own user and our own device, the predicate is invariantly
true. It returnsOk(true)on a device Synapse has never held a key for,keys_verifiedlatches, and the wipe-and-rebuild never runs. The check asks the local store whether we know about ourselves, which we always do.That is why this could sit for three months looking correct: its negative branch is unreachable, so nothing it guards was ever exercised, and no test would catch it because the predicate needs a live homeserver disagreeing with a live store.
What a working predicate has to do
Ask the homeserver, then read.
secret_store.rs:443-452already shows the shape used elsewhere in the SDK:Force the
/keys/query, then check the store, and treat a device absent from the response as the negative. Reading the store without forcing the query answers a different question.The rest of the PR stands and is the valuable half: evict, wipe the per-MXID store subdirectory, rebuild, recurse exactly once, error on a second negative. That resets
sharedto false sokeys_for_uploadoffers device keys again, which is the actual repair for the latch atmatrix-sdk-crypto-0.17.0/src/olm/account.rs:666.The gap this leaves in the diagnosis
Fixing the trigger makes the heal reachable; it does not explain why the keys never uploaded in the first place. The latch gates device keys and not one-time keys, so a device can go on publishing OTKs forever while
sharedsays the identity is done. What setsharedon a device Synapse never keyed is still unmeasured, and a heal that fires on every rebuild without that answer will wipe a crypto store repeatedly rather than once.Cost of the heal, which belongs beside the decision: wiping the per-MXID store discards olm and megolm state. The PR's own figure is ~12 s to restore 924 keys across 31 rooms from key backup, which is cheap when key backup is present and recoverable and is not free otherwise.
What is unverified in this change
"The fixed predicate returns false against today's live state" is a claim, not a measurement.
Running it needs a matrix-sdk client for
@julianon the pod, and there is none: sixteen introspects on the current pod, zeromcp.toolspans, no client-lifecycle line. So the predicate cannot be exercised against the one state that is known to be a true negative,MATRIXMCP-DLX8SKHKwithe2e_device_keys_json rows = 0.The
info!verdict is what will show it on first contact, and until a line appears naming that mxid and device, this is untested against production.The gap is smaller than it sounds. The unit cases include the mirror control: mutating
device_in_keys_responsetotrueand relaxing it to the user-level check both redden the test, so the predicate cannot be invariantly either. What is unverified is the round trip, not the judgement.The verdict is emitted on every path, including the refusals
Confirmed by reading the branch rather than from memory. The order inside
Ok(false)is:The verdict is the first statement, ahead of the retry error and both refusals. So an identity that never heals still reports its state, which matters most for the one account here with no cross-signing keys at all: it will be refused at the backup check and will still have said what the homeserver holds for it.
Blast radius, stated for the reviewer
If the predicate were wrong in the mirror direction, invariantly false, it would fire a heal that was not needed. The bounds cap that at one recoverable wipe per identity per process, and only where
backups().exists_on_server()answers true immediately beforehand. An unnecessary wipe with a backup present costs ~12 s of key restore; the current state is a device that cannot decrypt at all.Gated on the pinned toolchain and mutated on the predicate that was the bug. Merging.
My mutation, relaxing the check to the user level, which is the shape the old bug had:
A response listing the user without our device now reads as published again, and exactly one case says so.
What was actually wrong, and why it survived three months
verify_device_keys_publishedreduced toget_user_devices(own_user).get(own_device_id).is_some(), which reads the local crypto store with no homeserver round trip. The SDK's own note says our own device is always present there, and the memory store asserts it. So the predicate was invariantly true for the case it was written to detect, returningOk(true)for a device Synapse has never held a key for.Its negative branch was unreachable, so nothing it guarded was ever exercised, and no unit test could reach it: it needs a live homeserver disagreeing with a live store.
And the doc comment said
get_user_devices"also issues/keys/queryto the homeserver for any user with a pending key-query". It does not. A reviewer asking how this reaches the server was answered by the comment and stopped, which is why the explanation is the thing that had to be wrong for the check to look right. Replaced rather than appended, so the next reader does not meet both readings.The bounds, and why this ships without the
sharedquestion answeredOne heal per identity per process, second demand a warning with
refused_second_heal, the set unpersisted because it bounds a loop within a process and a restart is a fresh chance at a genuine occurrence.No wipe without
backups().exists_on_server(), checked per identity immediately before, withrefused_no_key_backup.@lucybothas no cross-signing keys, so this is per-identity rather than fleet-wide by necessity.Blast radius: one recoverable wipe per identity, only where a backup exists, against a device that currently cannot decrypt at all. Twelve seconds for 924 keys.
The verdict is logged at INFO first, ahead of the retry error and both refusals, so an identity that never heals still reports what the homeserver holds for it.
What is unverified and is stated on the PR
The fixed predicate has not run against today's live state, because it needs a client for
@julianon the pod and there is none. The log line is what will show it on first contact. The gap is the round trip rather than the judgement: the mirror control means the predicate cannot be invariantly either.And the question this does not answer
Nothing here explains what set
sharedon a device the homeserver never keyed. The bounds make the fix survivable without that answer rather than supplying it.refused_second_healon a fresh process is the signal that something re-creates the state, and if it appears the question becomes urgent again.