# 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. open

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.