From ab4e98330246e563e3c9aaac36eb28a241f93cda Mon Sep 17 00:00:00 2001 From: AlexZhang Date: Wed, 19 Aug 2026 02:19:26 +0800 Subject: [PATCH 1/3] fix(threads): close the backend thread timeline when its last view closes (#308) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Opening a thread created a focused Timeline, a subscriber task and channels in `JoinedRoomDetails::thread_timelines`, but closing the tab (desktop) or popping the view (mobile) never removed them, so a long session retained one timeline + task per distinct thread ever opened. - sliding_sync: `ThreadTimelineTable` replaces the `thread_timelines` + `pending_thread_timelines` pair and keeps `live ∩ pending = ∅`; `close()` forgets a pending build so a later `finish_create()` is rejected and the built timeline is dropped instead of leaking (Rule th-5). New `MatrixRequest::CloseThreadTimeline` drops the entry, which aborts its subscriber task via `PerTimelineDetails::drop`. `thread_timeline_counts()` and live/pending counts in the create/close logs for observability. - room_screen/state: `TIMELINE_STATES` becomes `TimelineStateCache` with per-kind `invalidate` (drop the cached state now, or mark it so a shown RoomScreen's late save is dropped) and `take` (clears the mark), plus `close_thread_timeline(cx, kind)`. - room_screen/thread_lifecycle: pure `close_if_unreferenced(closed, remaining)` decides "last view gone?" so mobile deep navigation (room → thread → room → thread) never closes a thread still on the stack (Rule th-3). - main_desktop_ui `close_tab` / `close_all_tabs` and app `StackNavigation Pop` call it; desktop ignores the undrawn mobile stack. Spec: specs/task-thread-timeline-lifecycle.spec.md — 9 automated scenarios (3 property tests) pass; 6 manual scenarios pending user verification. Co-Authored-By: Claude Fable 5 --- specs/task-thread-timeline-lifecycle.spec.md | 232 ++++++++++++++ src/app.rs | 16 +- src/home/main_desktop_ui.rs | 23 +- src/home/room_screen/mod.rs | 6 +- src/home/room_screen/state.rs | 157 +++++++++- src/home/room_screen/thread_lifecycle.rs | 153 ++++++++++ src/sliding_sync.rs | 303 ++++++++++++++++--- 7 files changed, 838 insertions(+), 52 deletions(-) create mode 100644 specs/task-thread-timeline-lifecycle.spec.md create mode 100644 src/home/room_screen/thread_lifecycle.rs diff --git a/specs/task-thread-timeline-lifecycle.spec.md b/specs/task-thread-timeline-lifecycle.spec.md new file mode 100644 index 000000000..406d69ec9 --- /dev/null +++ b/specs/task-thread-timeline-lifecycle.spec.md @@ -0,0 +1,232 @@ +spec: task +name: "Thread Timeline Lifecycle — Close The Backend Timeline And Subscriber Task When The Last Thread View Closes" +inherits: project +tags: [bugfix, lifecycle, thread, sliding-sync, memory] +estimate: 1d +--- + +## Intent + +Fix GitHub issue #308: every opened thread creates a focused `Timeline`, a +`timeline_subscriber_handler` task and two channels in +`JoinedRoomDetails::thread_timelines`, but closing the thread's tab (desktop) or +popping its view (mobile) never removes them. In a long session, opening and +closing many distinct threads retains timelines, cached events and tasks until +the whole room/account is torn down. + +The desired behavior is: + +- when the **last** UI view of a thread closes, its backend entry is removed + and its subscriber task terminates; +- a thread still shown by another view (mobile deep navigation) is not closed; +- reopening a closed thread creates a fresh, working timeline; +- a close that races with an in-flight creation never leaves an orphaned entry; +- existing whole-room cleanup (leave / kick / ban / account switch / logout) + still clears every thread timeline. + +## Constraints + +- Keep `MatrixRequest::CreateThreadTimeline` unchanged; add `CloseThreadTimeline { room_id, thread_root_event_id }` +- Do not change `timeline_subscriber_handler`'s protocol (`TimelineRequest`, `is_timeline_open`) or matrix-sdk usage +- Do not introduce new dependencies (`proptest` is already a dev-dependency) +- Do not change the per-room `TIMELINE_ROOM_GENERATIONS` mechanism; per-thread invalidation is additive +- Do not change Makepad DSL files +- Every UI-side "should this thread be closed now" decision is a pure function with no `Cx`, so it is unit- and property-testable + +## Decisions + +- Worker bookkeeping moves into a pure generic table + `ThreadTimelineTable` (`src/sliding_sync.rs`) replacing the two fields + `thread_timelines: HashMap<_, PerTimelineDetails>` and + `pending_thread_timelines: HashSet<_>` on `JoinedRoomDetails`; API: + `begin_create(id) -> bool` (false if live or pending), `finish_create(id, T) -> Result<(), T>` + (Err(T) hands the value back when the id is no longer pending, i.e. it was + closed while building), `fail_create(id)`, `close(id) -> Option` (removes + pending and live), `get/get_mut/contains/live_len/pending_len/clear` +- Invariant kept by the table: `live ∩ pending = ∅`, `close(id)` ⇒ `¬live(id) ∧ ¬pending(id)`, + `finish_create` after `close` does not insert +- `MatrixRequest::CloseThreadTimeline` handler calls `table.close(id)`; dropping + the returned `PerTimelineDetails` aborts its subscriber task (existing `Drop`) +- The create task calls `finish_create`; on `Err(details)` it drops the built + timeline and logs that the thread was closed while building +- UI-side decision helpers (pure, in `src/home/room_screen/thread_lifecycle.rs`): + `thread_kind_of(&SelectedRoom) -> Option` and + `still_referenced(kind, remaining: impl Iterator) -> bool`; + `close_if_unreferenced(kind, remaining) -> Option` returns the kind to close +- Desktop: `MainDesktopUI::close_tab` and `close_all_tabs` compute + `close_if_unreferenced` over the tabs that remain open and, for each returned + kind, call `close_thread_timeline(cx, kind)` +- Mobile: `App` on `StackNavigationAction::Pop` computes it over + `selected_room ∪ mobile_room_nav_stack` after the pop; on desktop the hidden + mobile stack is not a consumer (its RoomScreens are not drawn), so desktop + tab closes ignore it +- `close_thread_timeline(cx, kind)` (in `state.rs`) = `invalidate_timeline_state(kind)` + + `submit_async_request(MatrixRequest::CloseThreadTimeline {..})` +- Per-thread invalidation: `state::invalidate_timeline_state(kind)` removes the + cached `TimelineUiState` and records `kind` in an `INVALIDATED_TIMELINES` set; + `store_timeline_state` drops (instead of storing) a state whose kind is in the + set and clears the entry; `take_timeline_state(kind)` (used by `show_timeline`) + removes any stale entry so a fresh show is never dropped on its next hide +- Observability: `pub fn thread_timeline_counts(room_id) -> Option<(usize, usize)>` + (live, pending) in `sliding_sync.rs`, plus `log!` lines on close and on + "closed while building" + +## Boundaries + +### Allowed Changes +- `src/sliding_sync.rs` +- `src/home/room_screen/state.rs` +- `src/home/room_screen/mod.rs` +- `src/home/room_screen/thread_lifecycle.rs` +- `src/home/main_desktop_ui.rs` +- `src/app.rs` +- `specs/task-thread-timeline-lifecycle.spec.md` + +### Forbidden +- Do not modify `timeline_subscriber_handler`'s message protocol +- Do not modify matrix-sdk `TimelineBuilder` usage beyond what exists +- Do not touch `TIMELINE_ROOM_GENERATIONS` semantics +- Do not change Makepad DSL files +- Do not run `cargo fmt` + +## Acceptance Criteria + + + +### Rule: th-1 — The thread table never holds a closed or double-tracked thread + +Scenario: Table lifecycle — begin, finish, close + Tags: critical + Test: thread_table_begin_finish_close_lifecycle + Given an empty `ThreadTimelineTable` + When `begin_create(a)` then `finish_create(a, 1)` then `close(a)` + Then `begin_create` returns true, `finish_create` returns Ok, `close` returns Some(1) + And afterwards `contains(a)` is false and both lengths are 0 + +Scenario: Table rejects duplicate creation while live or pending + Test: thread_table_rejects_duplicate_begin + Given a table where `a` is pending + When `begin_create(a)` is called again + Then it returns false + And after `finish_create(a, 1)` a further `begin_create(a)` also returns false + +Scenario: Property — table invariants hold under random operation sequences + Tags: critical + Test: prop_thread_table_invariants + Given a random sequence of `begin_create` / `finish_create` / `fail_create` / `close` over a small id alphabet + When the operations are applied to a `ThreadTimelineTable` and to a reference model + Then after every step `live ∩ pending = ∅`, `close(id)` leaves id neither live nor pending + And `finish_create` inserts iff the id was pending at that moment + And the table's live/pending sets equal the reference model's + +### Rule: th-5 — A close that races a creation never leaves an orphan + +Scenario: finish_create after close hands the value back + Tags: critical + Test: thread_table_finish_after_close_is_rejected + Given `begin_create(a)` and then `close(a)` before creation finished + When `finish_create(a, 7)` is called + Then it returns Err(7) + And `contains(a)` is false + +Scenario: Worker drops a timeline that finished building after its close + Test: manual_test_close_during_thread_build_logs_and_drops + Given a slow network and a thread tab opened then immediately closed + When the thread timeline build completes + Then the log contains "closed while building" for that thread + And `thread_timeline_counts(room)` reports (0, 0) for it + +### Rule: th-2 — The last view closes the backend + +Scenario: Closing the only desktop tab of a thread yields a close + Tags: critical + Test: close_if_unreferenced_closes_when_no_remaining_view + Given a closed `SelectedRoom::Thread { room R, thread T }` + And the remaining open rooms are `[JoinedRoom R, Thread { R, U }]` + When `close_if_unreferenced` is evaluated + Then it returns Some(TimelineKind::Thread { R, T }) + +Scenario: Closing a non-thread never yields a close + Test: close_if_unreferenced_ignores_non_threads + Given a closed `SelectedRoom::JoinedRoom { R }` (or Invited / Space) + When `close_if_unreferenced` is evaluated with any remaining set + Then it returns None + +Scenario: Closed thread tab removes the backend entry + Test: manual_test_desktop_thread_tab_close_removes_timeline + Given a joined room with `thread_timeline_counts(room) == (0, 0)` + When the user opens thread T in a tab, waits for it to load, then closes the tab + Then the log contains "Closed thread timeline" for T + And `thread_timeline_counts(room)` returns to (0, 0) + +### Rule: th-3 — Shared views are protected + +Scenario: A thread still in the mobile stack is not closed + Tags: critical + Test: close_if_unreferenced_keeps_thread_still_referenced + Given a popped `Thread { R, T }` + And the remaining stack still contains `Thread { R, T }` (deep navigation) + When `close_if_unreferenced` is evaluated + Then it returns None + +Scenario: Property — a close fires exactly when the last reference disappears + Tags: critical + Test: prop_close_fires_iff_last_reference_removed + Given a random sequence of push/pop over a small set of rooms and threads + When after each pop `close_if_unreferenced(popped, remaining)` is evaluated + Then it returns Some iff the popped item is a thread that no longer appears in the remaining sequence + And it never returns Some for a non-thread + +Scenario: Mobile deep navigation keeps the shared thread alive + Test: manual_test_mobile_deep_nav_shared_thread_not_closed + Given mobile layout, navigation room R → thread T → room R → thread T + When the user goes back once + Then thread T's timeline is still live (no "Closed thread timeline" log yet) + And after going back to the first T and back again the log shows exactly one close + +### Rule: th-4 — Reopen after close is fresh + +Scenario: Invalidated cached state is dropped, fresh state is kept + Tags: critical + Test: invalidated_timeline_state_is_dropped_and_fresh_state_is_stored + Given a thread kind K with a stored `TimelineUiState` + When `invalidate_timeline_state(K)` is called + Then the stored state is gone + And a state for K stored afterwards without an intervening `take_timeline_state(K)` is dropped + And after `take_timeline_state(K)` a new state for K is stored normally + +Scenario: Reopening a closed thread works + Test: manual_test_reopen_closed_thread_creates_fresh_timeline + Given thread T was opened and closed (counts back to (0, 0)) + When the user opens T again + Then the timeline loads and shows messages + And `thread_timeline_counts(room)` is (1, 0) + +### Rule: th-6 — Whole-room cleanup still clears everything + +Scenario: Leaving a room with open threads clears its thread table + Test: manual_test_leave_room_clears_thread_timelines + Given a room with two thread tabs open + When the user leaves the room + Then all its tabs close and no thread timeline log lines or counts remain for that room + +Scenario: Stress — many opens and closes keep counts bounded + Test: manual_test_thread_open_close_stress_bounded + Given a room with at least 20 threads + When the user opens and closes each thread once + Then `thread_timeline_counts(room)` ends at (0, 0) + And the number of "Closed thread timeline" log lines equals the number of opened threads + +## Out Of Scope + +- Reference-counting the hidden mobile stack while in desktop layout (its RoomScreens are not drawn; a later breakpoint switch re-creates the timeline on show) +- Closing main-room timelines when their tabs close +- Bounding the size of `TIMELINE_STATES` for main rooms diff --git a/src/app.rs b/src/app.rs index 6ad1e00c7..40db799ea 100644 --- a/src/app.rs +++ b/src/app.rs @@ -22,7 +22,7 @@ use crate::{ add_menu::{AddMenuAction, AddMenuWidgetRefExt}, add_room::{CreateRoomModalAction, CreateRoomModalWidgetRefExt, JoinRoomModalAction, JoinRoomModalWidgetRefExt, StartChatModalAction, StartChatModalWidgetRefExt}, bot_binding_modal::{BotBindingModalAction, BotBindingModalWidgetRefExt}, - event_source_modal::{EventSourceModalAction, EventSourceModalWidgetRefExt}, invite_modal::{InviteModalAction, InviteModalWidgetRefExt, mark_invite_modal_closed}, invite_screen::{InviteScreenWidgetRefExt, LeaveRoomResultAction}, main_desktop_ui::MainDesktopUiAction, navigation_tab_bar::{NavigationBarAction, SelectedTab}, new_message_context_menu::NewMessageContextMenuWidgetRefExt, room_context_menu::{RoomContextMenuAction, RoomContextMenuWidgetRefExt}, room_screen::{InviteAction, MessageAction, ReportRoomModalAction, ReportRoomModalWidgetRefExt, ReportRoomResultAction, RoomScreenWidgetRefExt, TimelineUpdate, clear_timeline_states, set_room_info_action_modal_open}, room_settings_modal::{PendingAliasWrites, RoomSettingsAction, RoomSettingsFetchReason, RoomSettingsModalWidgetRefExt}, rooms_list::{RoomsListAction, RoomsListRef, RoomsListUpdate, clear_all_invited_rooms, enqueue_rooms_list_update}, rooms_list_header::RoomsListHeaderAction, space_lobby::SpaceLobbyScreenWidgetRefExt, spaces_bar::SpacesBarRef + event_source_modal::{EventSourceModalAction, EventSourceModalWidgetRefExt}, invite_modal::{InviteModalAction, InviteModalWidgetRefExt, mark_invite_modal_closed}, invite_screen::{InviteScreenWidgetRefExt, LeaveRoomResultAction}, main_desktop_ui::MainDesktopUiAction, navigation_tab_bar::{NavigationBarAction, SelectedTab}, new_message_context_menu::NewMessageContextMenuWidgetRefExt, room_context_menu::{RoomContextMenuAction, RoomContextMenuWidgetRefExt}, room_screen::{InviteAction, MessageAction, ReportRoomModalAction, ReportRoomModalWidgetRefExt, ReportRoomResultAction, RoomScreenWidgetRefExt, TimelineUpdate, clear_timeline_states, close_if_unreferenced, close_thread_timeline, set_room_info_action_modal_open}, room_settings_modal::{PendingAliasWrites, RoomSettingsAction, RoomSettingsFetchReason, RoomSettingsModalWidgetRefExt}, rooms_list::{RoomsListAction, RoomsListRef, RoomsListUpdate, clear_all_invited_rooms, enqueue_rooms_list_update}, rooms_list_header::RoomsListHeaderAction, space_lobby::SpaceLobbyScreenWidgetRefExt, spaces_bar::SpacesBarRef }, i18n::{AppLanguage, tr_fmt, tr_key}, join_leave_room_modal::{ report_orphaned_join_leave_results, JoinLeaveModalKind, JoinLeaveRoomModalAction, JoinLeaveRoomModalWidgetRefExt @@ -1654,8 +1654,20 @@ impl MatchEvent for App { // When a stack navigation pop is initiated (back button pressed), // pop the mobile nav stack so it stays in sync with StackNavigation. if let StackNavigationAction::Pop = action.as_widget_action().cast() { - if self.app_state.selected_room.is_some() { + if let Some(popped) = self.app_state.selected_room.take() { self.app_state.selected_room = self.mobile_room_nav_stack.pop(); + // On mobile, popping the last view of a thread frees its backend + // timeline; a thread still deeper in the stack (room → thread → + // room → thread) stays alive (spec task-thread-timeline-lifecycle, + // Rules th-2/th-3). On desktop the mobile stack isn't drawn and + // MainDesktopUI owns thread lifetimes via its dock tabs. + if !effective_is_desktop(cx) { + let remaining = self.mobile_room_nav_stack.iter() + .chain(self.app_state.selected_room.iter()); + if let Some(kind) = close_if_unreferenced(&popped, remaining) { + close_thread_timeline(cx, &kind); + } + } } // Don't `continue` — let StackNavigation also process this Pop. } diff --git a/src/home/main_desktop_ui.rs b/src/home/main_desktop_ui.rs index 90f2e5f4f..4ce3ab730 100644 --- a/src/home/main_desktop_ui.rs +++ b/src/home/main_desktop_ui.rs @@ -11,7 +11,7 @@ use crate::{ sliding_sync::{AccountSwitchAction, current_user_id, get_client}, utils::RoomNameId, }; -use super::{invite_screen::InviteScreenWidgetRefExt, room_screen::RoomScreenWidgetRefExt, rooms_list::RoomsListAction}; +use super::{invite_screen::InviteScreenWidgetRefExt, room_screen::{RoomScreenWidgetRefExt, close_if_unreferenced, close_thread_timeline}, rooms_list::RoomsListAction}; script_mod! { use mod.prelude.widgets.* @@ -375,9 +375,19 @@ impl MainDesktopUI { dock.close_tab(cx, tab_id); self.tab_to_close = None; - self.open_rooms.remove(&tab_id); + let closed_room = self.open_rooms.remove(&tab_id); self.initialized_tabs.remove(&tab_id); self.sync_visible_room_timelines(cx); + + // If that was the last tab showing a thread, free its backend timeline + // and subscriber task (spec task-thread-timeline-lifecycle, Rule th-2). + // `sync_visible_room_timelines` above has already hidden the RoomScreen, + // so its UI state is saved (and then invalidated) before the backend goes. + if let Some(closed_room) = closed_room + && let Some(kind) = close_if_unreferenced(&closed_room, self.open_rooms.values()) + { + close_thread_timeline(cx, &kind); + } } /// Closes every open tab belonging to the given room, including thread tabs. @@ -399,9 +409,16 @@ impl MainDesktopUI { /// Closes all tabs pub fn close_all_tabs(&mut self, cx: &mut Cx) { let dock = self.view.dock(cx, ids!(dock)); - for tab_id in self.open_rooms.keys() { + for tab_id in self.open_rooms.keys() { dock.close_tab(cx, *tab_id); } + // Every thread tab is going away, so every thread timeline can be closed + // (nothing remains to reference them; see `close_tab`). + for room in self.open_rooms.values() { + if let Some(kind) = close_if_unreferenced(room, std::iter::empty()) { + close_thread_timeline(cx, &kind); + } + } dock.select_tab(cx, id!(home_tab)); cx.action(AppStateAction::FocusNone); diff --git a/src/home/room_screen/mod.rs b/src/home/room_screen/mod.rs index 6635fb80d..62c24e8a9 100644 --- a/src/home/room_screen/mod.rs +++ b/src/home/room_screen/mod.rs @@ -70,6 +70,7 @@ mod room_info_pane; mod search; mod small_state; mod state; +mod thread_lifecycle; mod threads_pane; mod updates; @@ -85,7 +86,8 @@ use small_state::*; use state::*; pub use threads_pane::*; pub(crate) use bot_admin::is_known_or_likely_bot; -pub use state::{RoomScreenProps, RoomScreenTooltipActions, TimelineUpdate, clear_timeline_states, invalidate_timeline_state_for_room}; +pub use state::{RoomScreenProps, RoomScreenTooltipActions, TimelineUpdate, clear_timeline_states, close_thread_timeline, invalidate_timeline_state, invalidate_timeline_state_for_room}; +pub use thread_lifecycle::{close_if_unreferenced, thread_kind_of}; /// The maximum number of timeline items to search through /// when looking for a particular event. @@ -2000,7 +2002,7 @@ impl RoomScreen { .expect("BUG: Timeline::show_timeline(): no timeline_kind was set."); let room_id = kind.room_id().clone(); - let state_opt = TIMELINE_STATES.with_borrow_mut(|ts| ts.remove(&kind)); + let state_opt = state::take_timeline_state(&kind); let (mut tl_state, mut is_first_time_being_loaded) = if let Some(existing) = state_opt { (existing, false) } else { diff --git a/src/home/room_screen/state.rs b/src/home/room_screen/state.rs index 266475a2f..96bee7797 100644 --- a/src/home/room_screen/state.rs +++ b/src/home/room_screen/state.rs @@ -235,8 +235,8 @@ thread_local! { /// The global set of all timeline states, one entry per room. /// /// This is only useful when accessed from the main UI thread. - pub(super) static TIMELINE_STATES: RefCell> = - RefCell::new(HashMap::new()); + pub(super) static TIMELINE_STATES: RefCell> = + RefCell::new(TimelineStateCache::default()); /// A per-room generation counter, bumped each time a room's timeline states /// are invalidated (left/kicked/banned). A room absent from this map is at @@ -252,6 +252,68 @@ thread_local! { RefCell::new(HashMap::new()); } +/// The cache of hidden timelines' UI states plus the set of kinds that were +/// invalidated while their state was checked out by a visible RoomScreen. +/// +/// Generic over the state type so the take / store / invalidate protocol +/// (spec `task-thread-timeline-lifecycle`, Rule th-4) is unit-testable without +/// building a real `TimelineUiState`. +pub(super) struct TimelineStateCache { + states: HashMap, + /// Kinds whose backend was closed while a RoomScreen might still be showing + /// them (see [`invalidate_timeline_state`]). When that RoomScreen hides and + /// tries to save its state back, [`TimelineStateCache::store`] drops it + /// instead, so a later show rebuilds from a fresh backend timeline. + invalidated: HashSet, +} +impl Default for TimelineStateCache { + fn default() -> Self { + Self { states: HashMap::new(), invalidated: HashSet::new() } + } +} +impl TimelineStateCache { + /// Checks the state for `kind` out (for a RoomScreen about to show it) and + /// clears any invalidation mark, so a state created by this show is stored + /// normally on the next hide. + pub(super) fn take(&mut self, kind: &TimelineKind) -> Option { + self.invalidated.remove(kind); + self.states.remove(kind) + } + + /// Stores a hidden RoomScreen's state back. Returns `false` (and drops the + /// state) if `kind` was invalidated while it was checked out. + pub(super) fn store(&mut self, kind: TimelineKind, state: S) -> bool { + if self.invalidated.remove(&kind) { + return false; + } + self.states.insert(kind, state); + true + } + + /// Invalidates `kind`: removes a cached state if present, otherwise marks + /// the kind so the currently checked-out state is dropped when stored. + pub(super) fn invalidate(&mut self, kind: &TimelineKind) { + if self.states.remove(kind).is_none() { + self.invalidated.insert(kind.clone()); + } + } + + pub(super) fn retain_rooms(&mut self, mut keep: impl FnMut(&TimelineKind) -> bool) { + self.states.retain(|kind, _| keep(kind)); + self.invalidated.retain(|kind| keep(kind)); + } + + pub(super) fn clear(&mut self) { + self.states.clear(); + self.invalidated.clear(); + } + + #[cfg(test)] + fn contains(&self, kind: &TimelineKind) -> bool { + self.states.contains_key(kind) + } +} + /// The UI-side state of a single room's timeline, which is only accessed/updated by the UI thread. /// /// This struct should only include states that need to be persisted for a given room @@ -440,9 +502,7 @@ pub(super) struct SavedState { /// must only be called by the main UI thread. pub fn clear_timeline_states(_cx: &mut Cx) { // Clear timeline states cache - TIMELINE_STATES.with_borrow_mut(|states| { - states.clear(); - }); + TIMELINE_STATES.with_borrow_mut(|states| states.clear()); } /// Clears all UI-related timeline state (the main room timeline plus any open @@ -457,9 +517,7 @@ pub fn clear_timeline_states(_cx: &mut Cx) { /// which isn't used, but acts as a guarantee that this function /// must only be called by the main UI thread. pub fn invalidate_timeline_state_for_room(_cx: &mut Cx, room_id: &RoomId) { - TIMELINE_STATES.with_borrow_mut(|states| { - states.retain(|kind, _| kind.room_id() != room_id); - }); + TIMELINE_STATES.with_borrow_mut(|states| states.retain_rooms(|kind| kind.room_id() != room_id)); // A RoomScreen currently displaying this room still holds its state and will // try to save it back upon being hidden/closed; bumping the generation makes // `store_timeline_state` reject that state, even if the room gets re-joined @@ -489,21 +547,100 @@ pub(super) fn is_generation_current(room_id: &RoomId, generation: u64) -> bool { generation == current_timeline_generation(room_id) } +/// Invalidates the cached UI state of one timeline (a thread whose backend +/// timeline is being closed), so the next show rebuilds it from a fresh backend +/// instead of reusing a cache whose update channel is about to be dropped. +/// +/// If a RoomScreen is currently showing `kind` (its state is checked out of the +/// map), the kind is remembered and the state is dropped when that RoomScreen +/// hides (see [`store_timeline_state`]). Otherwise the cached state is removed +/// now. Either way, [`take_timeline_state`] on the next show clears the mark. +/// +/// Requires `&mut Cx` (unused) so it can only be called from the UI thread. +pub fn invalidate_timeline_state(_cx: &mut Cx, kind: &TimelineKind) { + TIMELINE_STATES.with_borrow_mut(|ts| ts.invalidate(kind)); +} + +/// Closes a thread timeline end to end: invalidates its cached UI state and +/// asks the sliding-sync worker to drop the backend timeline + subscriber task. +/// +/// Call this only when the last drawn view of the thread has gone away +/// (spec `task-thread-timeline-lifecycle`, Rules th-2/th-3); does nothing for +/// non-thread kinds. +pub fn close_thread_timeline(cx: &mut Cx, kind: &TimelineKind) { + let TimelineKind::Thread { room_id, thread_root_event_id } = kind else { return }; + invalidate_timeline_state(cx, kind); + submit_async_request(MatrixRequest::CloseThreadTimeline { + room_id: room_id.clone(), + thread_root_event_id: thread_root_event_id.clone(), + }); +} + +/// Checks a timeline state out of the global map for a RoomScreen that is +/// about to show it. Also clears any pending invalidation mark for `kind`, so a +/// state created by this show is stored normally on the next hide. +pub(super) fn take_timeline_state(kind: &TimelineKind) -> Option { + TIMELINE_STATES.with_borrow_mut(|ts| ts.take(kind)) +} + /// Saves the given timeline state into the global `TIMELINE_STATES` map, /// unless it was created before the room's last invalidation -/// (left/kicked/banned), in which case it is stale and gets discarded. +/// (left/kicked/banned) or its timeline was invalidated while shown +/// (thread closed), in which case it is stale and gets discarded. pub(super) fn store_timeline_state(tl: TimelineUiState) { if !is_generation_current(tl.kind.room_id(), tl.generation) { log!("Discarding stale timeline state (from before a leave/kick/ban) for room {:?}", tl.kind); return; } - TIMELINE_STATES.with_borrow_mut(|ts| ts.insert(tl.kind.clone(), tl)); + let kind = tl.kind.clone(); + let stored = TIMELINE_STATES.with_borrow_mut(|ts| ts.store(kind.clone(), tl)); + if !stored { + log!("Discarding invalidated timeline state (thread closed while shown) for {kind:?}"); + } } #[cfg(test)] mod tests { use super::*; + #[test] + fn invalidated_timeline_state_is_dropped_and_fresh_state_is_stored() { + let room_id = RoomId::parse("!thread_invalidate_test:example.org").unwrap(); + let k = TimelineKind::Thread { + room_id: room_id.to_owned(), + thread_root_event_id: OwnedEventId::try_from("$root").unwrap(), + }; + let other = TimelineKind::MainRoom { room_id: room_id.to_owned() }; + let mut cache: TimelineStateCache = TimelineStateCache::default(); + + // Cached (hidden) state is removed immediately by invalidate. + assert!(cache.store(k.clone(), 1)); + assert!(cache.contains(&k)); + cache.invalidate(&k); + assert!(!cache.contains(&k)); + + // Checked-out (shown) state: invalidate marks; the late store is dropped. + assert!(cache.store(k.clone(), 2)); + assert_eq!(cache.take(&k), Some(2)); // RoomScreen shows it + cache.invalidate(&k); // thread tab closed while shown + assert!(!cache.store(k.clone(), 2), "late save must be dropped"); + assert!(!cache.contains(&k)); + + // A fresh show clears the mark: take (nothing cached) then store succeeds. + assert_eq!(cache.take(&k), None); + assert!(cache.store(k.clone(), 3)); + assert_eq!(cache.take(&k), Some(3)); + + // Invalidation is per kind: the main room's state is untouched. + assert!(cache.store(other.clone(), 9)); + cache.invalidate(&k); + assert!(cache.contains(&other)); + assert_eq!(cache.take(&other), Some(9)); + // The mark for k is still pending until k is taken or stored once. + assert!(!cache.store(k.clone(), 4)); + assert!(cache.store(k.clone(), 5)); + } + #[test] fn stale_generation_rejected_after_invalidation_and_new_generation_accepted() { let room_id = RoomId::parse("!timeline_generation_test:example.org").unwrap(); diff --git a/src/home/room_screen/thread_lifecycle.rs b/src/home/room_screen/thread_lifecycle.rs new file mode 100644 index 000000000..e0e05913d --- /dev/null +++ b/src/home/room_screen/thread_lifecycle.rs @@ -0,0 +1,153 @@ +//! Pure decision helpers for the thread-timeline lifecycle +//! (spec `task-thread-timeline-lifecycle`, Rules th-2 / th-3). +//! +//! A thread's backend timeline (`sliding_sync::ThreadTimelineTable`) must be +//! closed exactly when the *last* drawn UI view of that thread goes away — +//! never earlier (mobile deep navigation can hold the same thread twice) and +//! never for non-thread rooms. These functions take the closed/popped +//! `SelectedRoom` and whatever views remain, and answer "which timeline kind, +//! if any, should be closed now". They hold no `Cx` and touch no globals, so +//! they are unit- and property-tested; the callers in `MainDesktopUI` / +//! `App` only feed them the right sets. + +use crate::{app::SelectedRoom, sliding_sync::TimelineKind}; + +/// The thread timeline kind of `room`, or `None` for anything that is not a thread. +pub fn thread_kind_of(room: &SelectedRoom) -> Option { + match room { + SelectedRoom::Thread { room_name_id, thread_root_event_id } => Some(TimelineKind::Thread { + room_id: room_name_id.room_id().clone(), + thread_root_event_id: thread_root_event_id.clone(), + }), + _ => None, + } +} + +/// `true` if any of `remaining` still shows the thread `kind`. +pub fn still_referenced<'a>( + kind: &TimelineKind, + remaining: impl IntoIterator, +) -> bool { + remaining + .into_iter() + .any(|room| thread_kind_of(room).as_ref() == Some(kind)) +} + +/// Given the view that just closed and the views that remain, returns the +/// thread timeline kind that should be closed now, if any: +/// `Some(kind)` iff `closed` is a thread and no remaining view shows it. +pub fn close_if_unreferenced<'a>( + closed: &SelectedRoom, + remaining: impl IntoIterator, +) -> Option { + let kind = thread_kind_of(closed)?; + if still_referenced(&kind, remaining) { + None + } else { + Some(kind) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::utils::RoomNameId; + use matrix_sdk::{RoomDisplayName, ruma::{OwnedEventId, OwnedRoomId}}; + + fn room(n: u8) -> RoomNameId { + RoomNameId::new( + RoomDisplayName::Named(format!("room{n}")), + OwnedRoomId::try_from(format!("!room{n}:example.org")).unwrap(), + ) + } + fn ev(n: u8) -> OwnedEventId { + OwnedEventId::try_from(format!("$thread{n}")).unwrap() + } + fn joined(n: u8) -> SelectedRoom { + SelectedRoom::JoinedRoom { room_name_id: room(n) } + } + fn thread(r: u8, t: u8) -> SelectedRoom { + SelectedRoom::Thread { room_name_id: room(r), thread_root_event_id: ev(t) } + } + fn kind(r: u8, t: u8) -> TimelineKind { + TimelineKind::Thread { room_id: room(r).room_id().clone(), thread_root_event_id: ev(t) } + } + + #[test] + fn close_if_unreferenced_closes_when_no_remaining_view() { + let remaining = [joined(1), thread(1, 2)]; + assert_eq!(close_if_unreferenced(&thread(1, 1), remaining.iter()), Some(kind(1, 1))); + // Same thread root in a *different* room is a different thread. + let remaining = [thread(2, 1)]; + assert_eq!(close_if_unreferenced(&thread(1, 1), remaining.iter()), Some(kind(1, 1))); + // Nothing remaining at all. + assert_eq!(close_if_unreferenced(&thread(1, 1), [].iter()), Some(kind(1, 1))); + } + + #[test] + fn close_if_unreferenced_ignores_non_threads() { + let remaining = [thread(1, 1)]; + assert_eq!(close_if_unreferenced(&joined(1), remaining.iter()), None); + assert_eq!(close_if_unreferenced(&joined(1), [].iter()), None); + let invited = SelectedRoom::InvitedRoom { room_name_id: room(3) }; + assert_eq!(close_if_unreferenced(&invited, [].iter()), None); + let space = SelectedRoom::Space { space_name_id: room(4) }; + assert_eq!(close_if_unreferenced(&space, [].iter()), None); + assert_eq!(thread_kind_of(&joined(1)), None); + } + + #[test] + fn close_if_unreferenced_keeps_thread_still_referenced() { + // Mobile deep navigation: room → thread → room → thread (same thread twice). + let remaining = [joined(1), thread(1, 1), joined(1)]; + assert_eq!(close_if_unreferenced(&thread(1, 1), remaining.iter()), None); + assert!(still_referenced(&kind(1, 1), remaining.iter())); + // Once the other reference is gone too, it closes. + let remaining = [joined(1), joined(1)]; + assert_eq!(close_if_unreferenced(&thread(1, 1), remaining.iter()), Some(kind(1, 1))); + } + + mod props { + use super::*; + use proptest::prelude::*; + + #[derive(Clone, Debug)] + enum Op { Push(u8, Option), Pop } + + fn arb_op() -> impl Strategy { + prop_oneof![ + 3 => (0u8..3, prop::option::of(0u8..3)).prop_map(|(r, t)| Op::Push(r, t)), + 2 => Just(Op::Pop), + ] + } + fn item(r: u8, t: Option) -> SelectedRoom { + match t { Some(t) => thread(r, t), None => joined(r) } + } + + proptest! { + /// Rule th-3: a close fires iff the popped view is a thread that no + /// longer appears anywhere in the remaining stack; never for non-threads. + #[test] + fn prop_close_fires_iff_last_reference_removed(ops in prop::collection::vec(arb_op(), 1..40)) { + let mut stack: Vec = Vec::new(); + for op in ops { + match op { + Op::Push(r, t) => stack.push(item(r, t)), + Op::Pop => { + let Some(popped) = stack.pop() else { continue }; + let decision = close_if_unreferenced(&popped, stack.iter()); + let expected = match thread_kind_of(&popped) { + None => None, + Some(k) => { + let refs = stack.iter().filter(|s| thread_kind_of(s).as_ref() == Some(&k)).count(); + if refs == 0 { Some(k) } else { None } + } + }; + prop_assert_eq!(decision, expected); + } + } + } + } + } + } +} diff --git a/src/sliding_sync.rs b/src/sliding_sync.rs index 4088acdb5..386df941e 100644 --- a/src/sliding_sync.rs +++ b/src/sliding_sync.rs @@ -1446,6 +1446,12 @@ pub enum MatrixRequest { room_id: OwnedRoomId, thread_root_event_id: OwnedEventId, }, + /// Request to close a thread timeline: drop its backend timeline and stop its + /// subscriber task. Sent when the last UI view of that thread closes. + CloseThreadTimeline { + room_id: OwnedRoomId, + thread_root_event_id: OwnedEventId, + }, /// Request to knock on (request an invite to) the given room. Knock { room_or_alias_id: OwnedRoomOrAliasId, @@ -3405,6 +3411,25 @@ async fn matrix_worker_task( }); } + MatrixRequest::CloseThreadTimeline { room_id, thread_root_event_id } => { + let mut all_joined_rooms = ALL_JOINED_ROOMS.lock().unwrap(); + let Some(room_info) = all_joined_rooms.get_mut(&room_id) else { + continue; + }; + // Removes a pending creation too, so a build that finishes later + // is rejected by `finish_create` (Rule th-5). Dropping the returned + // details aborts its subscriber task (`PerTimelineDetails::drop`). + if let Some(details) = room_info.thread_timelines.close(&thread_root_event_id) { + drop(details); + log!( + "Closed thread timeline for room {room_id}, thread {thread_root_event_id} \ + (remaining: live={}, pending={}).", + room_info.thread_timelines.live_len(), + room_info.thread_timelines.pending_len(), + ); + } + } + MatrixRequest::CreateThreadTimeline { room_id, thread_root_event_id } => { let main_room_timeline = { let mut all_joined_rooms = ALL_JOINED_ROOMS.lock().unwrap(); @@ -3412,11 +3437,8 @@ async fn matrix_worker_task( error!("BUG: room info not found for create thread timeline request, room {room_id}"); continue; }; - if room_info.thread_timelines.contains_key(&thread_root_event_id) { - continue; - } - let newly_pending = room_info.pending_thread_timelines.insert(thread_root_event_id.clone()); - if !newly_pending { + if !room_info.thread_timelines.begin_create(thread_root_event_id.clone()) { + // Already live or already being built. continue; } room_info.main_timeline.timeline.clone() @@ -3456,35 +3478,42 @@ async fn matrix_worker_task( Some(thread_root_event_id.clone()), ) ); - room_info - .pending_thread_timelines - .remove(&thread_root_event_id); - room_info.thread_timelines.insert( - thread_root_event_id.clone(), - PerTimelineDetails { - timeline: thread_timeline, - timeline_update_sender, - pagination_status: Arc::new( - TimelinePaginationStatus::default(), - ), - timeline_singleton_endpoints: Some(( - timeline_update_receiver, - request_sender, - )), - timeline_subscriber: TimelineSubscriber::Running( - timeline_subscriber_handler_task, - ), - }, - ); - SignalToUI::set_ui_signal(); + let details = PerTimelineDetails { + timeline: thread_timeline, + timeline_update_sender, + pagination_status: Arc::new( + TimelinePaginationStatus::default(), + ), + timeline_singleton_endpoints: Some(( + timeline_update_receiver, + request_sender, + )), + timeline_subscriber: TimelineSubscriber::Running( + timeline_subscriber_handler_task, + ), + }; + match room_info.thread_timelines.finish_create(thread_root_event_id.clone(), details) { + Ok(()) => { + log!( + "Thread timelines for room {room_id}: live={}, pending={}.", + room_info.thread_timelines.live_len(), + room_info.thread_timelines.pending_len(), + ); + SignalToUI::set_ui_signal(); + } + Err(orphan) => { + // The thread was closed while its timeline was building + // (Rule th-5): drop it now, which aborts the subscriber task. + log!("Thread timeline for room {room_id}, thread {thread_root_event_id} was closed while building; dropping it."); + drop(orphan); + } + } } Err(error) => { error!("Failed to create thread-focused timeline for room {room_id}, thread {thread_root_event_id}: {error}"); let mut all_joined_rooms = ALL_JOINED_ROOMS.lock().unwrap(); if let Some(room_info) = all_joined_rooms.get_mut(&room_id) { - room_info - .pending_thread_timelines - .remove(&thread_root_event_id); + room_info.thread_timelines.fail_create(&thread_root_event_id); } let error_detail = format!("{error}"); let room_id_retry = room_id.clone(); @@ -6989,15 +7018,98 @@ impl Drop for PerTimelineDetails { } } +/// Bookkeeping for the thread-focused timelines of one room. +/// +/// A thread is either *pending* (its timeline is being built by an async task), +/// *live* (built and subscribed), or absent. The table keeps +/// `live ∩ pending = ∅` and makes "close while building" safe: `close()` +/// forgets the pending id, so the build task's later `finish_create()` is +/// rejected and the freshly built value is handed back to be dropped instead +/// of leaking into the table (spec `task-thread-timeline-lifecycle`, Rules th-1/th-5). +/// +/// Generic over the payload so its invariants can be property-tested without +/// constructing real SDK timelines. +#[derive(Debug)] +pub(crate) struct ThreadTimelineTable { + live: HashMap, + pending: HashSet, +} +impl Default for ThreadTimelineTable { + fn default() -> Self { + Self { live: HashMap::new(), pending: HashSet::new() } + } +} +impl ThreadTimelineTable { + /// Marks `id` as being created. Returns `false` (and does nothing) if the + /// thread is already live or already pending. + pub(crate) fn begin_create(&mut self, id: OwnedEventId) -> bool { + if self.live.contains_key(&id) { + return false; + } + self.pending.insert(id) + } + + /// Completes a creation started with [`Self::begin_create`]. + /// + /// Inserts `value` only if `id` is still pending; otherwise (the thread was + /// closed while building) returns `Err(value)` so the caller can drop it. + pub(crate) fn finish_create(&mut self, id: OwnedEventId, value: T) -> Result<(), T> { + if !self.pending.remove(&id) { + return Err(value); + } + self.live.insert(id, value); + Ok(()) + } + + /// Abandons a creation started with [`Self::begin_create`] (build failed). + pub(crate) fn fail_create(&mut self, id: &OwnedEventId) -> bool { + self.pending.remove(id) + } + + /// Closes the thread: forgets a pending creation and removes a live entry, + /// returning it so the caller decides how to drop it. + pub(crate) fn close(&mut self, id: &OwnedEventId) -> Option { + self.pending.remove(id); + self.live.remove(id) + } + + #[cfg(test)] + pub(crate) fn contains(&self, id: &OwnedEventId) -> bool { + self.live.contains_key(id) || self.pending.contains(id) + } + pub(crate) fn get(&self, id: &OwnedEventId) -> Option<&T> { + self.live.get(id) + } + pub(crate) fn get_mut(&mut self, id: &OwnedEventId) -> Option<&mut T> { + self.live.get_mut(id) + } + pub(crate) fn live_len(&self) -> usize { + self.live.len() + } + pub(crate) fn pending_len(&self) -> usize { + self.pending.len() + } + #[cfg(test)] + pub(crate) fn live_ids(&self) -> Vec { + let mut v: Vec<_> = self.live.keys().cloned().collect(); + v.sort(); + v + } + #[cfg(test)] + pub(crate) fn pending_ids(&self) -> Vec { + let mut v: Vec<_> = self.pending.iter().cloned().collect(); + v.sort(); + v + } +} + struct JoinedRoomDetails { /// The room ID of this joined room. room_id: OwnedRoomId, /// Details about the main timeline for this room. main_timeline: PerTimelineDetails, - /// Thread-focused timelines for this room, keyed by thread root event ID. - thread_timelines: HashMap, - /// The set of thread timelines currently being created, to avoid duplicate in-flight work. - pending_thread_timelines: HashSet, + /// Thread-focused timelines for this room (live and in-flight), keyed by thread root event ID. + thread_timelines: ThreadTimelineTable, /// A drop guard for the event handler that represents a subscription to typing notices for this room. typing_notice_subscriber: Option, /// A drop guard for the event handler that represents a subscription to pinned events for this room. @@ -7490,6 +7602,14 @@ pub fn take_timeline_endpoints(kind: &TimelineKind) -> Option }) } +/// Returns `(live, pending)` thread-timeline counts for the given joined room, +/// or `None` if the room is not joined. Used for logging and lifecycle tests. +pub fn thread_timeline_counts(room_id: &RoomId) -> Option<(usize, usize)> { + let all_joined_rooms = ALL_JOINED_ROOMS.lock().unwrap(); + let jrd = all_joined_rooms.get(room_id)?; + Some((jrd.thread_timelines.live_len(), jrd.thread_timelines.pending_len())) +} + /// Returns a clone of the timeline update sender for the given timeline. /// /// This can be called multiple times, as it only clones the sender. @@ -8814,8 +8934,7 @@ async fn add_new_room( pagination_status: Arc::new(TimelinePaginationStatus::default()), timeline_subscriber: TimelineSubscriber::NotStarted { request_receiver }, }, - thread_timelines: HashMap::new(), - pending_thread_timelines: HashSet::new(), + thread_timelines: ThreadTimelineTable::default(), typing_notice_subscriber: None, pinned_events_subscriber: None, room_encryption_subscriber_task: None, @@ -11052,3 +11171,117 @@ mod tests { assert!(restore_session_failure_message(&err).contains("latest user")); } } + +#[cfg(test)] +mod thread_timeline_table_tests { + //! Rules th-1 / th-5 of spec `task-thread-timeline-lifecycle`. + use super::ThreadTimelineTable; + use matrix_sdk::ruma::OwnedEventId; + use proptest::prelude::*; + use std::collections::{BTreeMap, BTreeSet}; + + fn ev(n: u8) -> OwnedEventId { + OwnedEventId::try_from(format!("$thread{n}")).unwrap() + } + + #[test] + fn thread_table_begin_finish_close_lifecycle() { + let mut t: ThreadTimelineTable = ThreadTimelineTable::default(); + assert!(t.begin_create(ev(1))); + assert_eq!((t.live_len(), t.pending_len()), (0, 1)); + assert!(t.contains(&ev(1))); + assert!(t.get(&ev(1)).is_none(), "pending is not live"); + assert_eq!(t.finish_create(ev(1), 1), Ok(())); + assert_eq!((t.live_len(), t.pending_len()), (1, 0)); + assert_eq!(t.get(&ev(1)), Some(&1)); + assert_eq!(t.close(&ev(1)), Some(1)); + assert!(!t.contains(&ev(1))); + assert_eq!((t.live_len(), t.pending_len()), (0, 0)); + // Closing again is a no-op. + assert_eq!(t.close(&ev(1)), None); + } + + #[test] + fn thread_table_rejects_duplicate_begin() { + let mut t: ThreadTimelineTable = ThreadTimelineTable::default(); + assert!(t.begin_create(ev(1))); + assert!(!t.begin_create(ev(1)), "already pending"); + assert_eq!(t.finish_create(ev(1), 1), Ok(())); + assert!(!t.begin_create(ev(1)), "already live"); + assert_eq!((t.live_len(), t.pending_len()), (1, 0)); + } + + #[test] + fn thread_table_finish_after_close_is_rejected() { + let mut t: ThreadTimelineTable = ThreadTimelineTable::default(); + assert!(t.begin_create(ev(1))); + assert_eq!(t.close(&ev(1)), None, "nothing live yet, but pending is forgotten"); + assert_eq!(t.finish_create(ev(1), 7), Err(7)); + assert!(!t.contains(&ev(1))); + assert_eq!((t.live_len(), t.pending_len()), (0, 0)); + // fail_create after close is also a harmless no-op. + assert!(!t.fail_create(&ev(1))); + } + + #[derive(Clone, Debug)] + enum Op { Begin(u8), Finish(u8, u32), Fail(u8), Close(u8) } + + fn arb_op() -> impl Strategy { + prop_oneof![ + (0u8..4).prop_map(Op::Begin), + (0u8..4, 0u32..100).prop_map(|(i, v)| Op::Finish(i, v)), + (0u8..4).prop_map(Op::Fail), + (0u8..4).prop_map(Op::Close), + ] + } + + proptest! { + /// Rule th-1: live ∩ pending = ∅; close ⇒ ¬live ∧ ¬pending; finish inserts + /// iff the id was pending; the table matches an obvious reference model. + #[test] + fn prop_thread_table_invariants(ops in prop::collection::vec(arb_op(), 1..60)) { + let mut t: ThreadTimelineTable = ThreadTimelineTable::default(); + let mut live: BTreeMap = BTreeMap::new(); + let mut pending: BTreeSet = BTreeSet::new(); + for op in ops { + match op { + Op::Begin(i) => { + let id = ev(i); + let expect = !live.contains_key(&id) && !pending.contains(&id); + prop_assert_eq!(t.begin_create(id.clone()), expect); + if expect { pending.insert(id); } + } + Op::Finish(i, v) => { + let id = ev(i); + let was_pending = pending.remove(&id); + let res = t.finish_create(id.clone(), v); + if was_pending { + prop_assert_eq!(res, Ok(())); + live.insert(id, v); + } else { + prop_assert_eq!(res, Err(v)); + } + } + Op::Fail(i) => { + let id = ev(i); + prop_assert_eq!(t.fail_create(&id), pending.remove(&id)); + } + Op::Close(i) => { + let id = ev(i); + pending.remove(&id); + prop_assert_eq!(t.close(&id), live.remove(&id)); + prop_assert!(!t.contains(&id)); + } + } + // Invariants after every step. + let live_ids: Vec<_> = live.keys().cloned().collect(); + let pending_ids: Vec<_> = pending.iter().cloned().collect(); + prop_assert_eq!(t.live_ids(), live_ids.clone()); + prop_assert_eq!(t.pending_ids(), pending_ids.clone()); + for id in &live_ids { prop_assert!(!pending_ids.contains(id)); } + prop_assert_eq!(t.live_len(), live.len()); + prop_assert_eq!(t.pending_len(), pending.len()); + } + } + } +} From 8aa9db829b2c6e1b8c1821ef28e26de56765c57a Mon Sep 17 00:00:00 2001 From: AlexZhang Date: Wed, 19 Aug 2026 03:10:16 +0800 Subject: [PATCH 2/3] fix(threads): generation tokens for thread creation to close the ABA race MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review finding on #328: `pending` was keyed by event id only, so begin(A) → close(A) → begin(A) let the *old* build's finish_create consume the *new* attempt's pending slot (registering a timeline that was closed semantically) and its fail_create cancel the new attempt. `begin_create` now returns a `CreateToken { id, generation }` from a monotonic counter; `finish_create(&token, ..)` / `fail_create(&token)` act only if the token is the current pending generation for that id; `close` still forgets any generation. The create task carries its token. Tests: deterministic `thread_table_stale_generation_cannot_hijack_reopened_thread` plus the property model now tracks generations and lets ops replay stale tokens; both catch the id-only mutant. Spec th-5 updated. Co-Authored-By: Claude Fable 5 --- specs/task-thread-timeline-lifecycle.spec.md | 33 ++- src/sliding_sync.rs | 231 +++++++++++++------ 2 files changed, 190 insertions(+), 74 deletions(-) diff --git a/specs/task-thread-timeline-lifecycle.spec.md b/specs/task-thread-timeline-lifecycle.spec.md index 406d69ec9..ebea005a6 100644 --- a/specs/task-thread-timeline-lifecycle.spec.md +++ b/specs/task-thread-timeline-lifecycle.spec.md @@ -39,12 +39,17 @@ The desired behavior is: `ThreadTimelineTable` (`src/sliding_sync.rs`) replacing the two fields `thread_timelines: HashMap<_, PerTimelineDetails>` and `pending_thread_timelines: HashSet<_>` on `JoinedRoomDetails`; API: - `begin_create(id) -> bool` (false if live or pending), `finish_create(id, T) -> Result<(), T>` - (Err(T) hands the value back when the id is no longer pending, i.e. it was - closed while building), `fail_create(id)`, `close(id) -> Option` (removes - pending and live), `get/get_mut/contains/live_len/pending_len/clear` + `begin_create(id) -> Option` (None if live or pending; the token + carries the id and a monotonically increasing creation generation), + `finish_create(&token, T) -> Result<(), T>` (Err(T) hands the value back when + the token is not the *current* pending attempt for its id — closed while + building, or superseded by a newer `begin_create`), `fail_create(&token) -> bool` + (only clears the current attempt), `close(id) -> Option` (removes pending + of any generation and live), `get/get_mut/live_len/pending_len` - Invariant kept by the table: `live ∩ pending = ∅`, `close(id)` ⇒ `¬live(id) ∧ ¬pending(id)`, - `finish_create` after `close` does not insert + `finish_create`/`fail_create` act iff their token's generation is the current + pending generation for that id (so `begin(A) → close(A) → begin(A) → finish(old A)` + is rejected and the new attempt is untouched — no ABA hijack) - `MatrixRequest::CloseThreadTimeline` handler calls `table.close(id)`; dropping the returned `PerTimelineDetails` aborts its subscriber task (existing `Drop`) - The create task calls `finish_create`; on `Err(details)` it drops the built @@ -97,7 +102,8 @@ V(t) = number of drawn UI views of thread t; L(t) = t ∈ live ∨ t ∈ pending th-2 V(t): 1 → 0 ⇒ CloseThreadTimeline sent ⇒ ¬L(t) ∧ task aborted (last view closes backend) th-3 V(t) > 1 ∧ one view closes ⇒ no CloseThreadTimeline for t (shared views protected) th-4 ¬L(t) then open ⇒ fresh create succeeds (reopen) - th-5 close(t) while pending ⇒ finish_create(t) is rejected (race) + th-5 close(t) while pending ⇒ finish_create(old token) is rejected; a newer begin(t) + is never hijacked or cancelled by an older attempt (ABA) (race) th-6 room removal / logout still clears all (existing, unchanged) --> @@ -122,9 +128,11 @@ Scenario: Property — table invariants hold under random operation sequences Tags: critical Test: prop_thread_table_invariants Given a random sequence of `begin_create` / `finish_create` / `fail_create` / `close` over a small id alphabet - When the operations are applied to a `ThreadTimelineTable` and to a reference model + And finish/fail may use any token ever issued for that id (stale generations included) + When the operations are applied to a `ThreadTimelineTable` and to a generation-tracking reference model Then after every step `live ∩ pending = ∅`, `close(id)` leaves id neither live nor pending - And `finish_create` inserts iff the id was pending at that moment + And `finish_create` inserts iff its token is the current pending generation at that moment + And `fail_create` clears pending iff its token is the current pending generation And the table's live/pending sets equal the reference model's ### Rule: th-5 — A close that races a creation never leaves an orphan @@ -137,6 +145,15 @@ Scenario: finish_create after close hands the value back Then it returns Err(7) And `contains(a)` is false +Scenario: A stale creation cannot hijack or cancel a reopened thread + Tags: critical + Test: thread_table_stale_generation_cannot_hijack_reopened_thread + Given `begin_create(a)` yielded token `old`, then `close(a)`, then `begin_create(a)` yielded token `new` + When `finish_create(old, 10)` and `fail_create(old)` are called + Then both are rejected and `a` is still pending for `new` + And `finish_create(new, 20)` succeeds and `get(a)` is 20 + And a later `finish_create(old, 30)` is rejected leaving 20 live + Scenario: Worker drops a timeline that finished building after its close Test: manual_test_close_during_thread_build_logs_and_drops Given a slow network and a thread tab opened then immediately closed diff --git a/src/sliding_sync.rs b/src/sliding_sync.rs index 386df941e..09b970b0f 100644 --- a/src/sliding_sync.rs +++ b/src/sliding_sync.rs @@ -3431,17 +3431,17 @@ async fn matrix_worker_task( } MatrixRequest::CreateThreadTimeline { room_id, thread_root_event_id } => { - let main_room_timeline = { + let (main_room_timeline, create_token) = { let mut all_joined_rooms = ALL_JOINED_ROOMS.lock().unwrap(); let Some(room_info) = all_joined_rooms.get_mut(&room_id) else { error!("BUG: room info not found for create thread timeline request, room {room_id}"); continue; }; - if !room_info.thread_timelines.begin_create(thread_root_event_id.clone()) { + let Some(create_token) = room_info.thread_timelines.begin_create(thread_root_event_id.clone()) else { // Already live or already being built. continue; - } - room_info.main_timeline.timeline.clone() + }; + (room_info.main_timeline.timeline.clone(), create_token) }; let _create_thread_timeline_task = Handle::current().spawn(async move { @@ -3492,7 +3492,7 @@ async fn matrix_worker_task( timeline_subscriber_handler_task, ), }; - match room_info.thread_timelines.finish_create(thread_root_event_id.clone(), details) { + match room_info.thread_timelines.finish_create(&create_token, details) { Ok(()) => { log!( "Thread timelines for room {room_id}: live={}, pending={}.", @@ -3502,8 +3502,9 @@ async fn matrix_worker_task( SignalToUI::set_ui_signal(); } Err(orphan) => { - // The thread was closed while its timeline was building - // (Rule th-5): drop it now, which aborts the subscriber task. + // The thread was closed (or closed and re-requested) while this + // timeline was building (Rule th-5): drop it now, which aborts + // its subscriber task. A newer attempt, if any, is unaffected. log!("Thread timeline for room {room_id}, thread {thread_root_event_id} was closed while building; dropping it."); drop(orphan); } @@ -3513,7 +3514,9 @@ async fn matrix_worker_task( error!("Failed to create thread-focused timeline for room {room_id}, thread {thread_root_event_id}: {error}"); let mut all_joined_rooms = ALL_JOINED_ROOMS.lock().unwrap(); if let Some(room_info) = all_joined_rooms.get_mut(&room_id) { - room_info.thread_timelines.fail_create(&thread_root_event_id); + // Only clears *this* attempt; a newer attempt for the same + // thread keeps its pending state. + room_info.thread_timelines.fail_create(&create_token); } let error_detail = format!("{error}"); let room_id_retry = room_id.clone(); @@ -7029,45 +7032,73 @@ impl Drop for PerTimelineDetails { /// /// Generic over the payload so its invariants can be property-tested without /// constructing real SDK timelines. +/// A ticket for one in-flight creation returned by +/// [`ThreadTimelineTable::begin_create`]. `finish_create` / `fail_create` +/// only act if the ticket still matches the *current* pending generation for +/// that thread, so an old build that completes after `close` + re-`begin` +/// cannot hijack (or cancel) the newer creation (ABA protection). +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct CreateToken { + id: OwnedEventId, + generation: u64, +} #[derive(Debug)] pub(crate) struct ThreadTimelineTable { live: HashMap, - pending: HashSet, + /// In-flight creations: thread id → generation of the current attempt. + pending: HashMap, + /// Monotonic generation counter, bumped on every `begin_create`. + next_generation: u64, } impl Default for ThreadTimelineTable { fn default() -> Self { - Self { live: HashMap::new(), pending: HashSet::new() } + Self { live: HashMap::new(), pending: HashMap::new(), next_generation: 0 } } } impl ThreadTimelineTable { - /// Marks `id` as being created. Returns `false` (and does nothing) if the - /// thread is already live or already pending. - pub(crate) fn begin_create(&mut self, id: OwnedEventId) -> bool { - if self.live.contains_key(&id) { - return false; + /// Marks `id` as being created and returns the ticket for this attempt. + /// Returns `None` (and does nothing) if the thread is already live or + /// already pending. + pub(crate) fn begin_create(&mut self, id: OwnedEventId) -> Option { + if self.live.contains_key(&id) || self.pending.contains_key(&id) { + return None; } - self.pending.insert(id) + self.next_generation += 1; + let generation = self.next_generation; + self.pending.insert(id.clone(), generation); + Some(CreateToken { id, generation }) + } + + fn token_is_current(&self, token: &CreateToken) -> bool { + self.pending.get(&token.id) == Some(&token.generation) } - /// Completes a creation started with [`Self::begin_create`]. + /// Completes the creation identified by `token`. /// - /// Inserts `value` only if `id` is still pending; otherwise (the thread was - /// closed while building) returns `Err(value)` so the caller can drop it. - pub(crate) fn finish_create(&mut self, id: OwnedEventId, value: T) -> Result<(), T> { - if !self.pending.remove(&id) { + /// Inserts `value` only if `token` is still the current pending attempt for + /// its thread; otherwise (closed while building, or superseded by a newer + /// attempt) returns `Err(value)` so the caller can drop it. + pub(crate) fn finish_create(&mut self, token: &CreateToken, value: T) -> Result<(), T> { + if !self.token_is_current(token) { return Err(value); } - self.live.insert(id, value); + self.pending.remove(&token.id); + self.live.insert(token.id.clone(), value); Ok(()) } - /// Abandons a creation started with [`Self::begin_create`] (build failed). - pub(crate) fn fail_create(&mut self, id: &OwnedEventId) -> bool { - self.pending.remove(id) + /// Abandons the creation identified by `token` (build failed). Returns + /// `false` if the token was no longer current (nothing changed). + pub(crate) fn fail_create(&mut self, token: &CreateToken) -> bool { + if !self.token_is_current(token) { + return false; + } + self.pending.remove(&token.id); + true } - /// Closes the thread: forgets a pending creation and removes a live entry, - /// returning it so the caller decides how to drop it. + /// Closes the thread: forgets a pending creation (any generation) and + /// removes a live entry, returning it so the caller decides how to drop it. pub(crate) fn close(&mut self, id: &OwnedEventId) -> Option { self.pending.remove(id); self.live.remove(id) @@ -7075,7 +7106,7 @@ impl ThreadTimelineTable { #[cfg(test)] pub(crate) fn contains(&self, id: &OwnedEventId) -> bool { - self.live.contains_key(id) || self.pending.contains(id) + self.live.contains_key(id) || self.pending.contains_key(id) } pub(crate) fn get(&self, id: &OwnedEventId) -> Option<&T> { self.live.get(id) @@ -7097,7 +7128,7 @@ impl ThreadTimelineTable { } #[cfg(test)] pub(crate) fn pending_ids(&self) -> Vec { - let mut v: Vec<_> = self.pending.iter().cloned().collect(); + let mut v: Vec<_> = self.pending.keys().cloned().collect(); v.sort(); v } @@ -11175,7 +11206,7 @@ mod tests { #[cfg(test)] mod thread_timeline_table_tests { //! Rules th-1 / th-5 of spec `task-thread-timeline-lifecycle`. - use super::ThreadTimelineTable; + use super::{CreateToken, ThreadTimelineTable}; use matrix_sdk::ruma::OwnedEventId; use proptest::prelude::*; use std::collections::{BTreeMap, BTreeSet}; @@ -11187,11 +11218,11 @@ mod thread_timeline_table_tests { #[test] fn thread_table_begin_finish_close_lifecycle() { let mut t: ThreadTimelineTable = ThreadTimelineTable::default(); - assert!(t.begin_create(ev(1))); + let tok = t.begin_create(ev(1)).expect("first begin succeeds"); assert_eq!((t.live_len(), t.pending_len()), (0, 1)); assert!(t.contains(&ev(1))); assert!(t.get(&ev(1)).is_none(), "pending is not live"); - assert_eq!(t.finish_create(ev(1), 1), Ok(())); + assert_eq!(t.finish_create(&tok, 1), Ok(())); assert_eq!((t.live_len(), t.pending_len()), (1, 0)); assert_eq!(t.get(&ev(1)), Some(&1)); assert_eq!(t.close(&ev(1)), Some(1)); @@ -11204,83 +11235,151 @@ mod thread_timeline_table_tests { #[test] fn thread_table_rejects_duplicate_begin() { let mut t: ThreadTimelineTable = ThreadTimelineTable::default(); - assert!(t.begin_create(ev(1))); - assert!(!t.begin_create(ev(1)), "already pending"); - assert_eq!(t.finish_create(ev(1), 1), Ok(())); - assert!(!t.begin_create(ev(1)), "already live"); + let tok = t.begin_create(ev(1)).unwrap(); + assert!(t.begin_create(ev(1)).is_none(), "already pending"); + assert_eq!(t.finish_create(&tok, 1), Ok(())); + assert!(t.begin_create(ev(1)).is_none(), "already live"); assert_eq!((t.live_len(), t.pending_len()), (1, 0)); } #[test] fn thread_table_finish_after_close_is_rejected() { let mut t: ThreadTimelineTable = ThreadTimelineTable::default(); - assert!(t.begin_create(ev(1))); + let tok = t.begin_create(ev(1)).unwrap(); assert_eq!(t.close(&ev(1)), None, "nothing live yet, but pending is forgotten"); - assert_eq!(t.finish_create(ev(1), 7), Err(7)); + assert_eq!(t.finish_create(&tok, 7), Err(7)); assert!(!t.contains(&ev(1))); assert_eq!((t.live_len(), t.pending_len()), (0, 0)); // fail_create after close is also a harmless no-op. - assert!(!t.fail_create(&ev(1))); + assert!(!t.fail_create(&tok)); + } + + /// ABA: begin(A) → close(A) → begin(A) again → the *old* build finishing (or + /// failing) must not touch the new attempt; the new build lands normally. + #[test] + fn thread_table_stale_generation_cannot_hijack_reopened_thread() { + let mut t: ThreadTimelineTable = ThreadTimelineTable::default(); + let old = t.begin_create(ev(1)).unwrap(); + assert_eq!(t.close(&ev(1)), None); + let new = t.begin_create(ev(1)).expect("re-begin after close"); + assert_ne!(old, new); + assert_eq!(t.pending_len(), 1); + + // Old build finishes late: rejected, new attempt still pending. + assert_eq!(t.finish_create(&old, 10), Err(10)); + assert_eq!((t.live_len(), t.pending_len()), (0, 1)); + // Old build failing late: also ignored. + assert!(!t.fail_create(&old)); + assert_eq!(t.pending_len(), 1); + + // New build lands. + assert_eq!(t.finish_create(&new, 20), Ok(())); + assert_eq!(t.get(&ev(1)), Some(&20)); + assert_eq!((t.live_len(), t.pending_len()), (1, 0)); + // And a stale finish after the new one is live is rejected too. + assert_eq!(t.finish_create(&old, 30), Err(30)); + assert_eq!(t.get(&ev(1)), Some(&20)); + + // Symmetric case: old attempt fails late after new attempt is live. + let mut t2: ThreadTimelineTable = ThreadTimelineTable::default(); + let old = t2.begin_create(ev(2)).unwrap(); + t2.close(&ev(2)); + let new = t2.begin_create(ev(2)).unwrap(); + assert!(!t2.fail_create(&old), "stale fail must not clear the new pending attempt"); + assert_eq!(t2.finish_create(&new, 5), Ok(())); + assert_eq!(t2.get(&ev(2)), Some(&5)); + } + + /// Reference model: pending id → generation of the current attempt. + #[derive(Default)] + struct Model { + live: BTreeMap, + pending: BTreeMap, + next_gen: u64, } #[derive(Clone, Debug)] - enum Op { Begin(u8), Finish(u8, u32), Fail(u8), Close(u8) } + enum Op { + Begin(u8), + /// Finish using the token from the k-th (0-based) begin for this id + /// that we still hold — lets old generations resurface. + Finish(u8, usize, u32), + Fail(u8, usize), + Close(u8), + } fn arb_op() -> impl Strategy { prop_oneof![ - (0u8..4).prop_map(Op::Begin), - (0u8..4, 0u32..100).prop_map(|(i, v)| Op::Finish(i, v)), - (0u8..4).prop_map(Op::Fail), - (0u8..4).prop_map(Op::Close), + (0u8..3).prop_map(Op::Begin), + (0u8..3, 0usize..4, 0u32..100).prop_map(|(i, k, v)| Op::Finish(i, k, v)), + (0u8..3, 0usize..4).prop_map(|(i, k)| Op::Fail(i, k)), + (0u8..3).prop_map(Op::Close), ] } proptest! { - /// Rule th-1: live ∩ pending = ∅; close ⇒ ¬live ∧ ¬pending; finish inserts - /// iff the id was pending; the table matches an obvious reference model. + /// Rule th-1 (+ ABA): live ∩ pending = ∅; close ⇒ ¬live ∧ ¬pending; + /// finish/fail act iff their token is the *current* generation for the id; + /// the table matches the reference model after every step. #[test] - fn prop_thread_table_invariants(ops in prop::collection::vec(arb_op(), 1..60)) { + fn prop_thread_table_invariants(ops in prop::collection::vec(arb_op(), 1..80)) { let mut t: ThreadTimelineTable = ThreadTimelineTable::default(); - let mut live: BTreeMap = BTreeMap::new(); - let mut pending: BTreeSet = BTreeSet::new(); + let mut m = Model::default(); + // Every token ever issued, per id (oldest first), so ops can pick stale ones. + let mut tokens: BTreeMap> = BTreeMap::new(); for op in ops { match op { Op::Begin(i) => { let id = ev(i); - let expect = !live.contains_key(&id) && !pending.contains(&id); - prop_assert_eq!(t.begin_create(id.clone()), expect); - if expect { pending.insert(id); } + let expect = !m.live.contains_key(&id) && !m.pending.contains_key(&id); + let got = t.begin_create(id.clone()); + prop_assert_eq!(got.is_some(), expect); + if let Some(tok) = got { + m.next_gen += 1; + m.pending.insert(id.clone(), m.next_gen); + tokens.entry(id).or_default().push((tok, m.next_gen)); + } } - Op::Finish(i, v) => { + Op::Finish(i, k, v) => { let id = ev(i); - let was_pending = pending.remove(&id); - let res = t.finish_create(id.clone(), v); - if was_pending { + let Some(list) = tokens.get(&id) else { continue }; + if list.is_empty() { continue } + let (tok, g) = &list[k % list.len()]; + let current = m.pending.get(&id) == Some(g); + let res = t.finish_create(tok, v); + if current { prop_assert_eq!(res, Ok(())); - live.insert(id, v); + m.pending.remove(&id); + m.live.insert(id, v); } else { prop_assert_eq!(res, Err(v)); } } - Op::Fail(i) => { + Op::Fail(i, k) => { let id = ev(i); - prop_assert_eq!(t.fail_create(&id), pending.remove(&id)); + let Some(list) = tokens.get(&id) else { continue }; + if list.is_empty() { continue } + let (tok, g) = &list[k % list.len()]; + let current = m.pending.get(&id) == Some(g); + prop_assert_eq!(t.fail_create(tok), current); + if current { m.pending.remove(&id); } } Op::Close(i) => { let id = ev(i); - pending.remove(&id); - prop_assert_eq!(t.close(&id), live.remove(&id)); + m.pending.remove(&id); + prop_assert_eq!(t.close(&id), m.live.remove(&id)); prop_assert!(!t.contains(&id)); } } // Invariants after every step. - let live_ids: Vec<_> = live.keys().cloned().collect(); - let pending_ids: Vec<_> = pending.iter().cloned().collect(); + let live_ids: Vec<_> = m.live.keys().cloned().collect(); + let pending_ids: Vec<_> = m.pending.keys().cloned().collect(); prop_assert_eq!(t.live_ids(), live_ids.clone()); prop_assert_eq!(t.pending_ids(), pending_ids.clone()); - for id in &live_ids { prop_assert!(!pending_ids.contains(id)); } - prop_assert_eq!(t.live_len(), live.len()); - prop_assert_eq!(t.pending_len(), pending.len()); + let live_set: BTreeSet<_> = live_ids.iter().collect(); + for id in &pending_ids { prop_assert!(!live_set.contains(id)); } + prop_assert_eq!(t.live_len(), m.live.len()); + prop_assert_eq!(t.pending_len(), m.pending.len()); } } } From 51a1410a5e308fc26cfa70efa9b6b0145c8c8c53 Mon Sep 17 00:00:00 2001 From: AlexZhang Date: Wed, 19 Aug 2026 03:21:52 +0800 Subject: [PATCH 3/3] spec(threads): align table scenarios with the CreateToken API Co-Authored-By: Claude Fable 5 --- specs/task-thread-timeline-lifecycle.spec.md | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/specs/task-thread-timeline-lifecycle.spec.md b/specs/task-thread-timeline-lifecycle.spec.md index ebea005a6..d7b1c649b 100644 --- a/specs/task-thread-timeline-lifecycle.spec.md +++ b/specs/task-thread-timeline-lifecycle.spec.md @@ -113,16 +113,16 @@ Scenario: Table lifecycle — begin, finish, close Tags: critical Test: thread_table_begin_finish_close_lifecycle Given an empty `ThreadTimelineTable` - When `begin_create(a)` then `finish_create(a, 1)` then `close(a)` - Then `begin_create` returns true, `finish_create` returns Ok, `close` returns Some(1) + When `begin_create(a)` returns `Some(token)`, then `finish_create(&token, 1)`, then `close(a)` + Then `finish_create` returns Ok and `close` returns Some(1) And afterwards `contains(a)` is false and both lengths are 0 Scenario: Table rejects duplicate creation while live or pending Test: thread_table_rejects_duplicate_begin - Given a table where `a` is pending + Given a table where `begin_create(a)` returned `Some(token)` and `a` is pending When `begin_create(a)` is called again - Then it returns false - And after `finish_create(a, 1)` a further `begin_create(a)` also returns false + Then it returns None + And after `finish_create(&token, 1)` a further `begin_create(a)` also returns None Scenario: Property — table invariants hold under random operation sequences Tags: critical @@ -140,10 +140,11 @@ Scenario: Property — table invariants hold under random operation sequences Scenario: finish_create after close hands the value back Tags: critical Test: thread_table_finish_after_close_is_rejected - Given `begin_create(a)` and then `close(a)` before creation finished - When `finish_create(a, 7)` is called + Given `begin_create(a)` returned `Some(token)` and then `close(a)` before creation finished + When `finish_create(&token, 7)` is called Then it returns Err(7) And `contains(a)` is false + And `fail_create(&token)` returns false Scenario: A stale creation cannot hijack or cancel a reopened thread Tags: critical