macltok

public

Star0

Fork 0

Watch0

78 commits 3 branches 0 tags 0 releases3.0 MiB

Commit `722b4c3d0a09`

tarball

atagen <boss@atagen.co>2026-10-25 10:30 UTC

2 files changed+248-0

docs: record the 2026-10-25 review

Modified file: .gitignore+1

@@ -1 +1,2 @@

11 target/

2+/result

Added file: docs/review-2026-10-25.md+247 additions@@ -0,0 +1,247 @@
+# macltok review, 2026-10-25
+
+Read-only review of everything that landed on 2026-10-25. In macltok that is
+event context pagination, the encrypted mentions inbox, bounded history repair,
+segmented history and the pack fixes, as published on `vapour` 8fd8dcf6. In the
+sdk fork it is the sliding sync commit fence, reseeding, room-owned history
+views, replacement snapshots, segmented history and cinny reaction shortcodes,
+up to 9b2c17fb1.
+
+Nothing was built or run. Every finding was read against the code, and the
+ones marked verified were reread by hand after the reviewer reported them.
+Ids carry a status that starts as OPEN.
+
+## S. sdk fork correctness
+
+### S1. A limited sync can lose its gap. verified, OPEN
+
+`handle_sync` takes `older_boundary` from the in-memory linked chunk only
+(`event_cache/caches/room/state.rs:949`). After auto-shrink, or a restart with
+lazy loading, a room whose last chunk is the trailing gap from a recent load
+holds no events in memory. A limited sync for that room then records an
+ordinary gap and no `HistoryGap`. On reopen the retained events and the new
+ones render as continuous, which is the lost interval malice reported.
+
+Fix direction. Take the boundary from the store's newest event when memory has
+none.
+
+### S2. An empty fill page panics when the next chunk is a gap. verified, OPEN
+
+`apply_page` always passes a gap to `push_backwards_pagination_events`
+(`room/history.rs:549`). With no events, `replace_gap_at` removes the gap and
+returns the next chunk's first position. If that chunk is itself a gap,
+`insert_gap_at` returns `ChunkIsAGap` and the `.expect` at
+`event_linked_chunk.rs:403` fires. On native the room's history coordinator
+dies and every later history call for the room returns `ClientDropped`. On wasm
+it likely takes the worker with it.
+
+### S3. A limited sync during a recent fetch leaves two adjacent gaps. OPEN
+
+The recent fetch captures its anchor before the network request
+(`room/history.rs:221`). A limited sync that lands meanwhile installs its own
+gap, and the fetch then inserts its events and gap after it. The stale sync gap
+shows as a gap row over continuous history, and filling it walks into S2.
+
+### S4. Every room open persists a gap chunk. OPEN
+
+The recent load persists a gap whenever an anchor exists, `Joined` included
+(`room/history.rs:284-319`). macltok runs it on every open, so joined gaps pile
+up in storage, in `history_gaps()` and in every gap publication. The trailing
+gap left behind is also what produces the empty in-memory chunk S1 needs. The
+`RecentHistoryOutcome.gap` doc says the field is absent when nothing needs
+joining, and it is not.
+
+### S5. `history_candidate` decrypts the whole room twice per operation. OPEN
+
+It walks every stored chunk under the state write lock, which also blocks the
+room's sync (`room/state.rs:805-825`). A recent load and each fill page do it
+twice, and the commit then holds the whole history in memory until shrink. The
+insertion loop at `history.rs:301-312` is also O(n·m).
+
+### S6. sqlite event cache reads moved onto the write connection. OPEN
+
+72db3bb7d moved `load_all_chunks`, `load_all_chunks_metadata`,
+`load_last_chunk`, `load_previous_chunk`, `filter_duplicated_events` and
+`find_event_relations` into `with_immediate_transaction` so that
+`register_linked_chunk_owner` can do a one-time `INSERT OR IGNORE`. Reads now
+serialise behind every write. Plausibly part of the slow sqlite login.
+
+### S7. Pinned and event-focused views die after a clear. OPEN
+
+`clear_loaded_views` drops the pinned cache and drains event-focused caches,
+while threads are cleared and kept (`room/state.rs:866-877`). An open pinned
+timeline gets a Clear and then a closed stream. An event-focused timeline gets
+a Clear, then `MissingCache` on paginate. An interrupted event-focused
+pagination reports `hit_end_of_timeline`.
+
+### S8. A room `Update::Clear` on error paths wipes view chunks under live caches. plausible, OPEN
+
+Room Clear now deletes thread and pinned chunks and standalone events on all
+three backends. The pagination and shrink error paths send it without calling
+`clear_loaded_views`, so loaded thread and pinned caches keep writing against
+chunk ids that no longer exist.
+
+### S9. Reset on the live timeline drops receipts and identities. OPEN
+
+The Reset arm of the timeline state transaction clears `read_receipts` and
+`fully_read_event` and never repopulates them
+(`timeline/controller/state_transaction.rs:216-231`). It also discards
+`recycled_timeline_ids`, so every surviving row gets a new id.
+
+### S10. A commit hook that never resolves stalls sync for good. OPEN
+
+The hook runs inside `handle_response` holding the position lock
+(`sliding_sync/mod.rs:338`). Nothing can cancel it, and every later
+`sync_once` and `expire_session` waits on that lock. macltok's hook always
+resolves today, so this is a hazard in the api, not a live bug.
+
+### S11. Smaller sdk findings. OPEN
+
+- `visited_cursors` grows by one cursor per page, forever, and is re-encrypted on every gap rewrite.
+- `RoomEventCache::clear` and `reseed` `.expect` their join handles, so a task panic or runtime shutdown panics the caller.
+- sqlite writes plain gaps as a bare string and history gaps as an object behind an untagged reader. Always writing the object would leave one reader shim.
+- A downgrade fails to read history gaps and clears the room. Fine pre-1.0, noted.
+- IndexedDB room Clear decrypts every chunk in the database to find one room's.
+- The cinny `"shortcode"` field is a bare literal in three places, and `reaction_shortcode`'s doc says it only reads MSC4027.
+- The pinned Lagged path subscribes to the new cache and keeps reading the old receiver. Upstream bug, now hit after every clear.
+
+## M. macltok correctness
+
+### M1. The mentions inbox rewrites itself on every sync response. OPEN
+
+`batch` writes a new checkpoint for every response, typing and receipts
+included, and `save` serialises, encrypts and durably commits the whole inbox
+inside the sync hook (`mentions/service.rs`). With the 64 MiB default budget
+that is up to 64 MiB written per typing notification before `pos` may advance,
+at roughly four times that in peak memory.
+
+Related costs on the same path.
+- Every background completion also does a full save. Permanently undecryptable candidates are never dropped and retry forever with backoff capped at 256s, and any decryption report resets every candidate's retry.
+- `filter_ignored` reads the ignore list from the state store once per entry per batch.
+- Budget eviction drops one window and re-encrypts the whole inbox before checking again.
+- Candidate eviction removes the smallest key, which means lexicographic room id, not the oldest.
+
+### M2. A mentions failure stops sync. OPEN
+
+A failed save fails the hook and the hook stops room list sync. An IndexedDB
+quota error means no messages at all. A corrupt mentions store fails
+`Account::build`, and the only way out is `StoreConfig::delete`, which also
+drops the crypto store.
+
+### M3. A crash between hook ack and `pos` write raises a false global gap. verified, OPEN
+
+The hook records `next_pos` P2 durably, then the process dies before the sdk
+writes `pos`. On restart the sdk replays with `previous_pos` P1, the checkpoint
+says P2, and `batch` treats it as a discontinuity, marking every observed room
+unresolved (`mentions/service.rs:404`). Keeping the previous checkpoint and
+accepting a replay from it would fix it.
+
+### M4. Event context lost what the timeline gave it. OPEN
+
+ywsvysot moved `EventContext` from an event-focused sdk timeline to raw
+`/messages` through `normalise_observed`. That fixed real bugs in the focused
+path, but items now have no display names or avatars, edits arrive as separate
+messages, reactions and redactions arrive as `Other`, replies stay
+`ReplyDetails::Pending` forever and UTDs never retry.
+
+Also in context.
+- Truncating a page to `limit` while returning the server's `end` token skips whatever was cut, and `limit: 0` is accepted for pages.
+- Fetch awaits `before` and `after` one after the other.
+- Context media go into the shared 2048-entry registry and are never released, so heavy context paging can evict media for rows still on screen.
+
+### M5. Cancel is reported as failure. OPEN
+
+A user Cancel and the cancel that runs on room close both publish
+`RecentHistoryState::Failed("…interrupted")` and a report with a failure and no
+stop (`engine/repair.rs:284`, `:347`). `HistoryRepairStop::Cancelled` exists
+and is never sent.
+
+### M6. Network failures report as store errors. OPEN
+
+`cache_error` maps every `EventCacheError` to `ErrorKind::Store`
+(`engine/repair.rs:444`), `PaginationError` included. The `Failed => Network`
+arm cannot be reached because the sdk returns the error after committing
+`Failed`.
+
+### M7. Every history completion sends a full reset. OPEN
+
+`publish_history` flushes and then always resyncs (`engine/timeline.rs:134-145`),
+so every completion, failures and close included, sends a `TimelineReset`, and
+every room open produces two. The segmented history doc promises incremental
+insertions. `tests/engine.rs` ignores resets during recent loading, which hides
+it.
+
+### M8. Smaller macltok findings. OPEN
+
+- The mirror never removes `recent_history` or `repairs` entries when a room closes, and `RecentHistoryState::Idle` is never emitted.
+- `initial_history_limit` no longer applies to room timelines, and a failed recent load leaves a room with no initial history at all.
+- An `Updated` result for an already joined gap still counts a page.
+- The `Joined` gap arm in timeline normalisation cannot be reached, because the sdk drops joined gaps.
+- A finished recent load publishes both `RecentHistoryChanged(Loaded)` and a `HistoryRepairReport` for the same edge.
+- `evict_one` sets an entry's revision to the current one, not the next one, so revision-keyed consumers miss the change.
+- Mentions in a room left within the same response are dropped by the joined check.
+
+## D. dead api and design
+
+### D1. Most of the history repair api does nothing. OPEN
+
+`Start.budget` is validated and ignored. The policy is echoed and stored in the
+mentions inbox and never read. `Continue` always errors. `StartRecent`
+duplicates the open-time load. `GetPolicy` and `SetPolicy` in the actor are
+unreachable because the engine handles them first. Never emitted are
+`HistoryRepairStrategy::Overlap`, `HistoryRepairStop::Cancelled` and `NoAnchor`,
+`HistoryRepairPhase::FetchingRecent`, `coverage_duration_ms` and
+`threshold_evaluated`. Pre-1.0, so delete rather than keep them for serde.
+
+### D2. `HistoryGapLifecycle::Fetching` is never emitted. OPEN
+
+A consumer has to match a `Bridging` report's `gap_id` to know a gap is
+filling, then infer the end from a later report. Either emit it on the gap row
+while a fill runs or delete it.
+
+### D3. Mentions shapes. OPEN
+
+- `MentionGap.reason` is a string.
+- `MentionCoverage.incomplete_history` is always true.
+- Every newly seen room adds a gap and the list caps at 128, so `truncated` is true after the first sync on any account with more than 128 rooms.
+- The store is a generic multi-record store with HMAC'd keys that only ever holds `"inbox"`.
+- `MentionsCommand` is consumer-built and missing `#[non_exhaustive]`, and so are `HistoryRepairCommand`, `HistoryRepairPolicy` and `HistoryRepairBudget`.
+
+### D4. Pack shims. OPEN
+
+`clone_pack_image_with_usage`, `clone_sticker_with_usage` and
+`clone_pack_with_usage` sit beside wrappers under the old names. `known_usage`
+drops usage values it does not know, so clone stops being faithful for msc2545
+extensions, and it duplicates the default-usage rule in
+`normalise::image_pack::usage`. `Some(vec![])` silently means inherit.
+
+### D5. `reseed` has no caller. OPEN
+
+`RoomEventCache::reseed`, its state method, `reseed.md` and its tests are about
+500 lines of fork surface. macltok uses `fill_history_gap_once` and the
+segmented history doc says it does not reseed. The generation fencing stays,
+`clear` and `commit_history_candidate` need it.
+
+### D6. Event context shape. OPEN
+
+`EventContext.event` is optional but always present on success.
+`EventContextPage.end_reached` repeats `next_token.is_none()`. The transport
+takes `u32` and clamps to 50 while the engine rejects anything over 50.
+
+## H. housekeeping
+
+- H1. sdk 9b2c17fb1 is only on the fork's `segmented-history` branch. The `macltok` branch is at 9b9b16982. Fast-forward it before tidying branches or cargo cannot fetch the pin. OPEN
+- H2. lyvnurlv d1313f47 is an empty change merged into vapour, and the segmented history doc cites it. At xnzknouu the pins are split three ways. OPEN
+- H3. `repair.rs` landed without rustfmt. OPEN
+- H4. `docs/segmented-history.md` says the lost interval drops are an earlier path, and both `prev_batch` drops are still at `state.rs:943`. It says concurrent history operations return `RoomBusy`, and only a fill does. Much of it is handoff status that will rot. OPEN
+- H5. `docs/mentions-discovery.md` says replies need a notifying highlight while the code accepts `direct` alone, and says context capture runs after the commit while the watch publication runs before the ack. OPEN
+- H6. American spellings in sqlite.rs and the mentions doc. OPEN
+
+## Checked and sound
+
+- Gap metadata and the mentions inbox are encrypted at rest on sqlite and IndexedDB. The inbox head compare-and-swap holds.
+- The sliding sync `pos` advances only after the hook acks, on both stores.
+- Coordinator cancellation, shared recent fetches and atomic event, gap and cursor commits.
+- The old timeline watcher stops before the reset goes out, and the mirror ack precedes the command reply.
+- Both merges keep both sides. All nine sdk pins and both lockfiles agree on 9b2c17fb1.
+- The spare sdk worktrees hold nothing that is not already committed.

macltok

Commit 722b4c3d0a09

Commit `722b4c3d0a09`