macltok

# macltok review, 2026-09-24

Second full review, taken after the restructure and Tier 0 landed. It covers

correctness, races, robustness, duplication, antipatterns and how easy the crate

is to build a client on, then folds in the API gaps reported by Malice, the

first real client built on macltok.

Every finding carries an id so fixes can reference it. Status starts as OPEN.

SDK claims cite matrix-sdk 0.18 source under `~/.cargo/registry` and were

checked against the code rather than recalled.

Baseline when written. `cargo fmt --check` clean, all default-feature tests

pass. The sqlite and clippy runs are recorded at the end.

## C. Correctness

### C1. `Verify(Reject)` never rejects. done

`VerificationController::reject` (`src/encryption.rs:339-389`) validates the

SAS state twice and returns `Ok(())` without ever calling `sas.mismatch()`. The

peer is never told the emoji did not match, the local flow stays in

comparison, and the command reports success. A user who presses "they don't

match" gets no protection from the one action meant to provide it. No test

exercises `Reject`.

### C2. Sync dies permanently after `SyncState::Error`. done

The connection watcher maps `SyncState::Error` to `ConnectionState::Offline`

(`src/engine.rs:1338-1341`) and nothing ever calls `SyncService::start` again.

In the SDK the supervisor breaks out of its loop on error, and even offline

mode ends in `Error` when its reachability check fails

(`matrix-sdk-ui sync_service.rs:313-339`). `start()` is the only way back

(`sync_service.rs:629-649`). The client sees "Offline" forever while the engine

still reports itself healthy. `SyncState::Terminated` has the same problem, it

becomes `ConnectionState::Stopped` while the engine keeps running, so

`Stopped` now means two different things.

Fix direction. Restart with backoff on `Error` and `Terminated`, and report

`Stopped` only from engine shutdown.

### C3. A room's send queue stays disabled after any send failure. done

The SDK disables a room's queue after every send error, recoverable or not

(`matrix-sdk send_queue/mod.rs:1011-1019`), and nothing in matrix-sdk-ui turns

it back on. macltok re-enables only inside `RetrySend`

(`src/engine.rs:2178`), and does so globally. One network blip therefore

pauses every later message in that room until the user explicitly retries a

specific transaction. Element X re-enables queues when sync returns to

running.

Fix direction. Re-enable on each transition to `Online`, and keep

`RetrySend` for unwedging a single item.

### C4. Own encrypted uploads cannot be fetched, and the media cache is split between two formats. done, with a round trip test through the sdk cache

macltok writes ciphertext into the media store under the `Encrypted` source key

and decrypts on every read (`src/engine.rs:2799-2821`, `2864-2874`). The SDK

stores plaintext under that same key, both in `Media::get_media_content`

(`matrix-sdk media.rs:451-480`, `522-528`) and in the send queue. After an

upload the send queue renames its plaintext cache entry to the remote

`Encrypted` source (`send_queue/upload.rs:928-953`). So fetching your own sent

image in an encrypted room reads plaintext, runs it through

`AttachmentDecryptor`, fails the hash check and returns `MediaUnavailable`. The

reverse also holds. Any SDK path that reads a macltok-written entry gets

ciphertext as if it were the image. The media tests force plaintext rooms

(`tests/media.rs:766`), which is why this passed.

Fix direction. Cache plaintext, matching the SDK, and keep the byte bound by

decrypting in a streaming fashion before the write.

### C5. Media single-flight is poisoned by a leader panic. done, shared-future single flight in the media query

In `spawn_media_fetch` (`src/engine.rs:1870-1911`) followers push their id and

return at once, trusting the leader to answer for them. If `fetch_media`

panics, the leader's own id gets "command task panicked", the followers get

nothing, and the `(MediaId, MediaSize)` entry is never removed. Every later

fetch of that media then becomes a follower of a dead leader and never

completes. Largely superseded if media fetch becomes a direct query (see

M2), but the fix is the same either way. Use a drop guard or a shared future

so the entry always clears.

### C6. Back pagination can stick at "start reached" forever. done, the sdk already remembers

`reached_start` is a cached `AtomicBool` that is only ever set from a

pagination result (`src/engine.rs:4060-4071`, `4097`). A limited sync with a

gap makes the event cache shrink to the last chunk

(`event_cache/caches/room/state.rs:917-924`), which the timeline applies as

`VectorDiff::Clear`. History is unloaded again but the flag stays true, so

every later `Paginate` emits `StartReached` without fetching anything. The

room scrolls to a stub and stops.

Fix direction. Drop the flag and follow `Timeline::live_back_pagination_status`

(`matrix-sdk-ui timeline/pagination.rs:130-138`), which is authoritative.

### C7. Soft logout destroys the store. done, `SessionAccessRejected { soft_logout }` and a same-device login

`SessionChange::UnknownToken` carries `soft_logout`

(`matrix-sdk client/mod.rs:166`), but the watcher collapses it into a

payload-free `SessionAccessRejected` (`src/engine.rs:1314`). The README and the

event's doc comment both tell the client to delete the saved record and the

store. On a soft logout the correct response is to re-authenticate and keep

the store, so following the documentation throws away every E2EE key for

nothing.

### C8. Logout and rejected access leave the engine running. done

`spawn_logout` emits `LoggedOut` and returns (`src/engine.rs:825-852`). The

sync service, watchers and room actors keep running against a dead session,

and every later command fails with an authentication error. Neither logout nor

a hard `UnknownToken` shuts the engine down. The engine should stop itself,

emitting `LoggedOut` or the rejection before `Stopped`.

### C9. `MarkRead` can move the read marker backwards and costs two requests. done, open rooms go through the timeline so stale receipts get dropped

`mark_read` (`src/engine.rs:2072-2095`) calls `Room::send_single_receipt`

twice. That call does no ordering check

(`matrix-sdk room/mod.rs:2167-2201`), so a client that marks an older event

drags both the public receipt and the fully-read marker back. The timeline API

refuses stale receipts (`timeline/controller/mod.rs:1621-1690`), and

`Room::send_multiple_receipts` sets fully read and read in one

`/read_markers` call (`room/mod.rs:2213-2230`). Private read receipts are not

offered at all.

Fix direction. When the room is open, route through the actor and use

`Timeline::send_multiple_receipts`. Otherwise use a single

`Room::send_multiple_receipts`.

### C10. `create_direct` always creates a new room. done

`Client::create_dm` never reuses an existing DM

(`matrix-sdk client/mod.rs:1830-1849`). Starting a chat with someone you

already talk to creates a duplicate every time. `Client::get_dm_room` exists

for the lookup (`client/mod.rs:1852`).

### C11. Unregistered tasks swallow panics, contradicting the design doc. done, command tasks answer `Interrupted`, actor internals are supervised and panic the actor

The design doc says a watcher panic is terminal. But the timeline watcher

(`src/engine.rs:589`), pagination worker (`446`), pagination result task

(`472`) and verification watcher (`src/encryption.rs:276`) are bare

`tokio::spawn`s. A panic in a timeline watcher freezes that timeline in

silence while commands keep succeeding. The reverse mistake also exists. The

account-data writers (`src/engine.rs:1111`, `1141`) and the per-room media

upload watcher (`1945`) are registered as `TaskContext::Watcher`, so a panic

in a single write kills the whole engine.

### C12. The room actor blocks on network requests. done, redact reactions and receipts run beside the actor. replies stay serial so sends keep their order

Each room actor runs its commands serially and awaits each one. Most sends

only enqueue, but `Timeline::redact` on a remote event is a direct HTTP call

(`timeline/mod.rs:630`), and so is removing a reaction

(`controller/mod.rs:595`). `Room::make_reply_event` can fetch the replied-to

event (`room/reply.rs:108`). A slow homeserver therefore stalls every later

send, paginate and thread command in that room behind one redaction.

### C13. Media registry evicts media that is still on screen. done, lru by stamp and released on close

`MediaRegistry::insert_all` (`src/engine.rs:358-370`) evicts in first-insertion

order and does not refresh position on re-registration. One registry of 2048

entries is shared by every room. In a long-lived session, the first messages

loaded in a room that is still open are the first evicted, and fetching them

then fails with `MediaUnknown`. Closed timelines never release their entries,

so they compete with open ones.

### C14. Smaller correctness issues. done, bar the lagged session watcher which stays as is

- `CommandResultError::Failed` renders its `Display` through `Debug`

  (`src/transport.rs:840`), so users see a struct dump.

- `download_bounded` builds a fresh `reqwest::Client` per fetch just to build a

  request (`src/engine.rs:3181`). That reloads the TLS roots every time.

  `http_client.get(...)` is enough. On a 401 it refreshes the token and then

  fails rather than retrying once.

- On `Lagged`, the session watcher assumes the dropped message was a token

  refresh (`src/engine.rs:1315`). A dropped `UnknownToken` is lost.

- Leaving a room does not close its open actor, so its timeline, typing

  subscription and room subscription outlive the membership.

- `open_room` on a room that is already open succeeds with no `TimelineReset`

  (`src/engine.rs:1608`). A second frontend sharing the engine gets success and

  no snapshot.

- On shutdown, in-flight commands never get a terminal result. That is

  acceptable, but the docs should say that `Stopped` fails everything still

  outstanding.

- A store path reused after a login whose record was never persisted holds a

  crypto account for another device, and the SDK refuses it. Document one

  store directory per login.

## A. API shape and client-builder friendliness

### A1. Three completion mechanisms, and the useful one is rationed. done, one awaited path and one error type

A client must know which of three patterns each operation uses.

- `send` returns a `CommandId`. The client correlates it by hand against

  `CommandSucceeded`, `CommandSucceededWithRoom` or `CommandFailed` on a

  shared stream.

- Five direct methods (`join_room`, `create_room`, `create_direct`,

  `delete_devices`, and the internal room-result path) await a `oneshot` and

  return `CommandResultError`. These emit nothing on the stream.

- The account-data writes await a `oneshot` but return `ClientQueryError`,

  with no `CommandId` at all.

The headless example is mostly bookkeeping of ids. Malice gap 4 comes straight

from this. The one pattern a web backend can use, an awaited result per

request, is not offered for most operations.

### A2. Pure reads modelled as commands, against the doc's own rule. done, `media` and `mxc_media` are handle queries

The design doc says operations are split by whether they change state. Media

fetch changes no state, yet it is a `MediaCommand` whose bytes arrive as a

`MediaReady` broadcast on the shared stream. Every frontend then receives every

other frontend's image bytes, up to 20 MiB each, and the requester has to

filter by `MediaId`.

### A3. Every timeline item is a `Message`, with presentation strings in `body`. done

`normalise_timeline_item` (`src/normalise/timeline.rs`) flattens every event

into `TimelineMessage`. The consequences for a client are these.

- Membership changes, profile changes and room state changes arrive as

  messages whose body reads `[m.room.member]` or similar.

- Redacted and undecryptable events carry hard-coded English bodies,

  `<message redacted>` and `<unable to decrypt message>`. Only UTD has a flag.

  A redaction cannot be told apart from a user who typed that text.

- `msgtype` is dropped, so emotes and notices cannot be rendered differently

  from text.

- No sender avatar, though the SDK profile has one. `display_name_ambiguous`

  is ignored, so two "Alice"s look identical.

- Reply references hold no preview content, and reply details are never

  fetched. The SDK leaves them `Unavailable` until the app calls

  `Timeline::fetch_details_for_event` (`timeline/event_item/content/reply.rs:38`),

  so replies to anything outside the loaded window never get a sender.

- `TimelineItem` is not `#[non_exhaustive]`, so adding a state-event variant

  later is a break.

This needs a content enum, for example `Message { msgtype, ... }`, `Redacted`,

`UnableToDecrypt`, `Membership`, `ProfileChange`, `RoomState`, `Poll` and

`Other { event_type }`. It is the largest single rendering gap.

### A4. Timeline commands cannot target local echoes. done

`Edit`, `Redact` and `ToggleReaction` take an `OwnedEventId` and wrap it in

`TimelineEventItemId::EventId`. The SDK accepts transaction ids for all three,

but a client cannot edit, redact or react to a message it sent until the

server echo arrives. The commands should take the identifier that the model

already exposes.

### A5. `RoomSummary` is too thin for a room list. done

Missing fields, all cheap SDK reads. Topic, canonical alias, joined and invited

member counts, heroes, tombstone and successor, inviter for invites, a latest

event preview, and space parents and children (Malice gap 3).

### A6. No incoming verification. done, ported from malice's vendored copy

Only outgoing `StartVerification` exists. A request started from another

device, which is the normal "verify this new login" flow, is never observed.

macltok registers no handler for it, and the SDK has no stream, so the app

pattern is an event handler plus `Encryption::get_verification_request`

(`encryption/mod.rs:1060`).

### A7. Missing `#[non_exhaustive]`. done

Adding the attribute later is itself a breaking change, so it belongs now. Output types that will

grow are `TimelineItem`, `CommandKind`, `RoomMembership`, `VerificationState`,

`HistoryState`, `KeyBackupState` and `RecoveryState`. Input structs that will

grow need a constructor plus `#[non_exhaustive]`. These are

`CreateRoomOptions`, `PasswordLogin`, `OAuthLogin` and `UploadThumbnail`.

### A8. Smaller API issues. mostly done. `HistoryState::Failed(String)`, stringly power level events and fixed encryption defaults remain

- `MediaCommand::Upload` duplicates `UploadAttachment`, and the handle exposes

  both `upload_media` and `upload_attachment`.

- Convenience methods cover room and account commands but not timeline send,

  edit, paginate, mark read, typing, recovery or verification, so a client uses

  two styles side by side.

- `HistoryState::Failed(String)` and `RoomPowerLevels::events: Vec<(String, i64)>`

  are stringly typed where typed values exist.

- `rooms_filtered` re-implements the search half of `RoomListFilter::matches`,

  which itself lowercases the needle once per room.

- `RoomsChanged` is sent once per notable room-info update with no coalescing

  at source. A sync touching fifty rooms queues fifty invalidations, each of

  which prompts a full `rooms()` rebuild in a naive client.

- The encryption defaults (`auto_enable_backups`, `OneShot` backup download)

  are hard-coded with no override.

## D. Duplication and antipatterns

mostly done by the engine split. the actor thread registry, one spawn helper, one responder, one media request path, one test support module. `get_room` lookups went from twenty one hand rolled errors to a helper per module

Line references are from the mechanical inventory taken during this review.

- `RoomActor::run` is about 700 lines. The "queue while the thread is opening,

  else QueueFull" block appears four times (`src/engine.rs:3367-3402`,

  `3518-3549`, `3625-3644`), and emit-then-`break 'actor` about twenty times.

  A thread registry type with one `route(id, root, command)` method removes

  most of it.

- Six `spawn_*` helpers share one shape of clone, spawn, run, emit

  (`src/engine.rs:825-960`). `spawn_rename_device` hand-rolls the same thing

  without `emit_command_result`. `CommandFailure` is built by hand in nine

  places. The reply-or-event branch appears nine times. One `spawn_command(id,

  kind, future)` plus one completion sink covers all of them, and A1 removes

  the branch entirely.

- `media_http_request` (`src/engine.rs:2991-3175`) is six near-identical

  branches that differ by endpoint type and token mode.

- Twenty-one `client.get_room(...)` lookups with hand-written `RoomUnknown`

  errors, ten of them consecutive match arms in `execute_room_command`.

- `media_from_message` and `media_registration` walk `MessageType` twice for

  the same item. The descriptor can be derived from the registration.

- The verification controller repeats its "done, then cancelled, then error"

  terminal check eight times. It also carries an unused `_client` parameter,

  and the encryption worker builds a throwaway `VerificationController` to run

  `Recover`. `EncryptionWork` is a single-variant enum.

- `catch_unwind` with a "... panicked" string appears six times.

- Test helpers `receive_until`, `event_messages`, `diff_items`, `start_engine`

  and `stop` are copied across up to five test files. A shared

  `tests/support/mod.rs` removes them.

- `model.rs` has its test module in the middle of the file, with types

  defined after it.

## T. Test coverage

No test touches the whole encryption domain (recovery, backups, verification),

all notification commands, invite, kick, ban and unban, power levels,

`MarkRead`, `SetTyping`, `Redact`, `ToggleReaction`, `CloseThread`, `Logout`,

`RenameDevice`, `DeleteDevices`, `resolve_push_notification` or

`rooms_filtered`. No test uses an encrypted room, which is how C4 got through.

## M. Malice gaps, synthesised

Malice reported four gaps. Each one traces back to a finding above, and fixing

the underlying finding closes the gap without special-casing it.

| Malice gap | Root cause | Proposed shape |

| --- | --- | --- |

| Own profile | Not exposed at all | `ClientHandle::own_profile()`, a direct read via `Account::fetch_user_profile` (`account.rs:353`), returning a `Profile { user_id, display_name, avatar }`. Add `user_profile(user_id)` alongside. Setting the name and avatar become commands. |

| MXC avatar download | Media is reachable only through registered `MediaId`s (A2) | `ClientHandle::mxc_media(uri, MediaRequest)` as a bounded direct query, sharing the download, bound and cache code with timeline media. |

| Space children | `RoomSummary` gap (A5) | Wrap `matrix_sdk_ui::spaces::SpaceService` (`spaces/mod.rs`), which offers top-level joined spaces, `joined_parents_of_child` and a paginated `/hierarchy` `SpaceRoomList`. |

| Timeline media bytes per request | A1 and A2 | `ClientHandle::media(media_id, size)` as an awaited direct query. Remove `MediaCommand::Fetch` and `Event::MediaReady`. |

Malice's rule, that no `matrix-sdk` use remains in the app, holds once these

land. None of the four needs an escape hatch.

## Discussion items

These are genuine trade-offs rather than fixes, each with a recommendation.

### Q1. How far to take awaited results

Option 1. Keep `send` and add `handle.execute(command).await -> Result<CommandOutput, CommandFailure>`

by carrying an optional `oneshot` in `CommandEnvelope`. Stream result events

continue for `send`. This is additive, but it leaves two mechanisms and every

engine path still branches on reply versus event.

Option 2. Make awaited results the only completion path. Remove `CommandId`,

`CommandSucceeded*`, `CommandFailed` and `InteractiveAuthRequired` from the

stream, which then carries only state changes. Fire-and-forget callers spawn a

task. Every engine path produces one `Result` and the nine reply-or-event

branches disappear.

The cost of option 2 is ordering. Today a `TimelineReset` is guaranteed to

precede the `CommandSucceeded` for its `Open` or `Resync` on the same stream.

Across a stream and a `oneshot` the guarantee weakens to "enqueued before", so

a client draining the stream in another task can see the future resolve first.

Either `Open` and `Resync` return the snapshot in their result and stop

emitting `TimelineReset`, which then needs an epoch on `TimelineChanged` so

stale diffs from the old watcher can be discarded, or the contract states that

the snapshot arrives on the stream at some point after success. A second

frontend sharing the engine also stops seeing the other's outcomes, though it

still sees every resulting state change.

Recommendation. Option 2 with snapshot-in-result and an epoch. It is the

larger break, but it is the shape every client ends up wrapping the current API

into, and breakage is free now.

### Q2. Space children source

`SpaceService` gives the full graph, including unjoined children, over the

network via `/hierarchy`. Local `m.space.child` state is instant but lists only

what sync delivered and cannot show rooms the user has not joined.

Recommendation. Own a `SpaceService` in the engine, as with `SyncService`. Put

`parent_spaces` and `child_rooms` on `RoomSummary` from local state for the

sidebar, and add a paginated hierarchy query for browsing.

### Q3. Timeline content model depth

A3 can be closed with a coarse enum (message, redacted, UTD, state, other) or

with typed variants for membership and profile changes, room name and topic,

polls and calls. Coarse is quick but pushes raw event parsing onto every

client. Recommendation. Typed variants for membership, profile, name, topic,

avatar and encryption changes, which cover what Element shows inline, plus

`Other { event_type }` as the escape.

## Verification results

- `cargo fmt -- --check` clean.

- `cargo test --all-targets` 49 passed, none failed.

## follow-up review, 2026-09-25

three parallel reviews of the reworked engine, each finding checked against the

code before fixing. all fixed in the same change

- close answered `Interrupted` when the actor had already gone, and anything

  sent to an actor stopping after a leave was dropped. actors now say they're

  leaving and hand back what they couldn't serve

- closing a timeline released its media before stopping the watcher, so late

  diffs re-registered it

- thread receipts failed while the room was open and the thread wasn't

- verification watchers subscribed after the state they needed, so a peer

  starting sas left the flow waiting forever

- a plain event could leave an encrypted file's ciphertext in the cache

- downloads died at thirty seconds on the sdk client's timeout

- `create_direct` made a second dm when asked twice before sync caught up

- offline mode plus a server refusing sync bounced forever with no backoff

- upload progress missed resumed uploads, failures could be lost to lag

- `SendState::Failed { retriable }` meant the opposite of what a ui wants,

  now `will_retry`, and `Queued` folded into `Sending`

- room previews showed the `* edit` fallback, thumbnails claimed the original's

  mimetype and ignored the asked size, v12 creators' power was dropped,

  cleared names came back as empty strings

- the example logged in afresh every run and broke its own store

- adding the fixes tipped `Engine::run` past the compiler's query depth in every

  consumer, so it now returns a boxed future