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::spawns. 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’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