From ce92e8073ea1fe0bc239ee79b0c6ca2888f312a2 Mon Sep 17 00:00:00 2001 From: Torlando <281092095+torlando-agent[bot]@users.noreply.github.com> Date: Sat, 5 Sep 2026 14:39:32 +0000 Subject: [PATCH] fix(lxmf): move conversation-open store I/O off the LVGL task Opening a conversation crashed the same way sending did. load_conversation() (LVGL task, under the LVGL lock held by replace_route) ran the full open pipeline synchronously: identity recall (ustore), display-name read, the message-index read, and the per-message metadata reads. On this device's degraded LittleFS each op is 0.4-2s, so a cold open of a dozen-message conversation held the LVGL mutex past the 5s deadlock guard and asserted at LVGLLock.h:45. The send path already got the mailbox fix; the open path never did. Restructure with the same pattern: - load_conversation() (LVGL task) now only navigates + resets the list and shows the truncated hash in the header. Same-peer re-opens return early with zero store I/O (rows are still built). - prepare_conversation() (main loop, called from update()) does the store I/O between a short guard lock and a short commit lock, then commits the header name + initial bubbles + background-fill arming under a brief LVGL_LOCK. A generation counter discards a stale in-flight prepare when the conversation changes mid-I/O. - refresh() re-arms the prepare instead of re-reading under the lock. The 1Hz store 'not found in index' fetch is pre-existing (present on 2527c6d) and is being tracked separately as a flash-wear follow-up. Build tdeck SUCCESS, 170/170 contract tests pass. --- lib/tdeck_ui/UI/LXMF/ChatScreen.cpp | 216 ++++++++++++------ lib/tdeck_ui/UI/LXMF/ChatScreen.h | 23 ++ lib/tdeck_ui/UI/LXMF/UIManager.cpp | 6 + .../test_chat_scroll_contract.py | 26 ++- 4 files changed, 192 insertions(+), 79 deletions(-) diff --git a/lib/tdeck_ui/UI/LXMF/ChatScreen.cpp b/lib/tdeck_ui/UI/LXMF/ChatScreen.cpp index 5cbde8fb..3ed7755e 100644 --- a/lib/tdeck_ui/UI/LXMF/ChatScreen.cpp +++ b/lib/tdeck_ui/UI/LXMF/ChatScreen.cpp @@ -168,29 +168,93 @@ void ChatScreen::create_input_area() { void ChatScreen::load_conversation(const Bytes& peer_hash, ::LXMF::MessageStore& store) { LVGL_LOCK(); - _peer_hash = peer_hash; _message_store = &store; + // Same-peer re-open (back to the list and re-tap): the content is + // already committed by prepare_conversation() and the rows are still + // built — nothing to do. This keeps re-opens free of any store I/O. + if (_peer_hash == peer_hash && _prepared_peer_hash == peer_hash) { + return; + } + + _peer_hash = peer_hash; + // A peer change cancels any in-flight background fill from the // previous conversation (it would otherwise prepend the previous - // conversation's older rows into the new one). refresh() re-arms the - // fill for the new peer. + // conversation's older rows into the new one). prepare_conversation() + // re-arms the fill for the new peer once its metadata is gathered. if (_bg_fill_active.exchange(false)) { _keep_bottom_during_background_fill.store(false); _bg_fill_target = _display_start_idx; // fill is a no-op now } + // LVGL-task side of a conversation open: navigation + list reset only. + // The store reads (identity recall, display name, message index, per- + // message metadata) moved to prepare_conversation() on the main loop — + // a cold open does dozens of LittleFS/ustore reads that take seconds on + // a degraded filesystem, and running them here (under the LVGL lock, + // held by replace_route) held the mutex past the 5s deadlock guard and + // rebooted the device (assert at LVGLLock.h:45). Same defect class as + // the send path; same fix (see OutgoingSendMailbox.h / UIManager:: + // service_pending_sends). { char log_buf[64]; - snprintf(log_buf, sizeof(log_buf), "Loading conversation with peer %.8s...", + snprintf(log_buf, sizeof(log_buf), "Opening conversation with peer %.8s...", peer_hash.toHex().c_str()); INFO(log_buf); } + // Clear existing messages and row tracking (rows are rebuilt by + // prepare_conversation()'s commit once the metadata is gathered). + lv_obj_clean(_message_list); + _messages.clear(); + _message_rows.clear(); + _all_message_hashes.clear(); + _display_start_idx = 0; + + // Header shows the truncated hash immediately; prepare_conversation() + // upgrades it to the resolved display name once recall completes. + lv_obj_t* label_peer = lv_obj_get_child(_header, 1); // Second child is peer label + { + char hash_buf[20]; + snprintf(hash_buf, sizeof(hash_buf), "%.12s...", peer_hash.toHex().c_str()); + lv_label_set_text(label_peer, hash_buf); + } + + // Arm a prepare for this peer. Bumping the generation discards any + // in-flight prepare for a previous (or same) peer: its commit re-checks + // the generation before touching the UI. + _prepared_peer_hash = Bytes(); + _prepare_generation++; +} + +void ChatScreen::prepare_conversation() { + // Called from UIManager::update() on the main loop. The guard and the + // commit are short locked sections; the store I/O between them (identity + // recall, display name, message index, per-message metadata) runs OFF the + // LVGL lock, so a slow cold open can't hold the mutex past the 5s + // deadlock guard. + Bytes peer_hash; + uint32_t generation = 0; + ::LXMF::MessageStore* store = nullptr; + { + LVGL_LOCK(); + if (!_message_store || _peer_hash.size() == 0) { + return; + } + if (_prepared_peer_hash == _peer_hash) { + return; // already prepared for this peer + } + peer_hash = _peer_hash; + generation = _prepare_generation; + store = _message_store; + } + + // ── slow I/O, off the LVGL lock ──────────────────────────────────────── // Three-tier display name resolution (mirrors ConversationListScreen): // 1. Live announce cache (Identity::recall_app_data) // 2. MessageStore-persisted name (survives reboots) - // 3. Truncated hash (last resort) + // 3. Truncated hash (already shown by load_conversation) // When (1) hits, write through to the persistent cache so future // cold boots get the name back without waiting for a re-announce. String peer_name; @@ -198,97 +262,101 @@ void ChatScreen::load_conversation(const Bytes& peer_hash, ::LXMF::MessageStore& if (app_data && app_data.size() > 0) { peer_name = parse_display_name(app_data); if (peer_name.length() > 0) { - store.set_display_name(peer_hash, std::string(peer_name.c_str())); + store->set_display_name(peer_hash, std::string(peer_name.c_str())); } } if (peer_name.length() == 0) { - std::string cached = store.get_display_name(peer_hash); + std::string cached = store->get_display_name(peer_hash); if (!cached.empty()) { peer_name = String(cached.c_str()); } } - if (peer_name.length() == 0) { - char hash_buf[20]; - snprintf(hash_buf, sizeof(hash_buf), "%.12s...", peer_hash.toHex().c_str()); - peer_name = hash_buf; + + // Load all message hashes from store (sorted by timestamp). + std::vector all_hashes = store->get_messages_for_conversation(peer_hash); + + // Gather only the few NEWEST messages (the rest of the page is streamed + // in by tick_background_fill() a couple per main-loop tick). + size_t display_start_idx = 0; + if (all_hashes.size() > INITIAL_RENDER) { + display_start_idx = all_hashes.size() - INITIAL_RENDER; } - // Update header with peer info - lv_obj_t* label_peer = lv_obj_get_child(_header, 1); // Second child is peer label - lv_label_set_text(label_peer, peer_name.c_str()); - - refresh(); -} - -void ChatScreen::refresh() { - LVGL_LOCK(); - if (!_message_store) { - return; - } - - INFO("Refreshing chat messages"); - - // Clear existing messages and row tracking - lv_obj_clean(_message_list); - _messages.clear(); - _message_rows.clear(); - - // Reserve capacity for message hashes to reduce fragmentation - _all_message_hashes.reserve(200); - - // Load all message hashes from store (sorted by timestamp) - _all_message_hashes = _message_store->get_messages_for_conversation(_peer_hash); - - // Render only the few NEWEST messages synchronously (under the LVGL lock) so - // the conversation opens fast. The rest of the page is streamed in by - // tick_background_fill() a couple per main-loop tick, so the UI never freezes. - // (This runs on the main loop, not a task: the MessageStore shares one - // _json_doc between save + load and isn't safe for concurrent access.) - if (_all_message_hashes.size() > INITIAL_RENDER) { - _display_start_idx = _all_message_hashes.size() - INITIAL_RENDER; - } else { - _display_start_idx = 0; - } - - { - char log_buf[80]; - snprintf(log_buf, sizeof(log_buf), " Found %zu messages, displaying last %zu", - _all_message_hashes.size(), _all_message_hashes.size() - _display_start_idx); - INFO(log_buf); - } - - for (size_t i = _display_start_idx; i < _all_message_hashes.size(); i++) { - const auto& msg_hash = _all_message_hashes[i]; - - // Use fast metadata loader (cache hit: O(1) in-memory; miss: one + std::vector items; + items.reserve(INITIAL_RENDER); + for (size_t i = display_start_idx; i < all_hashes.size(); i++) { + // Fast metadata loader (cache hit: O(1) in-memory; miss: one // LittleFS read that warms the cache for every later touch). - ::LXMF::MessageStore::MessageMetadata meta = _message_store->load_message_metadata(msg_hash); + ::LXMF::MessageStore::MessageMetadata meta = + store->load_message_metadata(all_hashes[i]); if (!meta.valid) { continue; } - MessageItem item; - item.message_hash = msg_hash; + item.message_hash = all_hashes[i]; item.content = String(meta.content.c_str()); format_timestamp(meta.timestamp, item.timestamp_str, sizeof(item.timestamp_str)); item.outgoing = !meta.incoming; item.delivered = (meta.state == static_cast(::LXMF::Type::Message::DELIVERED)); item.failed = (meta.state == static_cast(::LXMF::Type::Message::FAILED)); - - _messages.push_back(item); - create_message_bubble(item); + items.push_back(item); } - // Queue the rest of the first page to stream in on the main loop. Set the - // target before activating so tick sees a consistent target. - _bg_fill_target = (_all_message_hashes.size() > MESSAGES_PER_PAGE) - ? _all_message_hashes.size() - MESSAGES_PER_PAGE - : 0; - const bool initial_fill_active = _display_start_idx > _bg_fill_target; - _keep_bottom_during_background_fill.store(initial_fill_active); - _bg_fill_active.store(initial_fill_active); + { + char log_buf[80]; + snprintf(log_buf, sizeof(log_buf), " Found %zu messages, displaying %zu", + all_hashes.size(), items.size()); + INFO(log_buf); + } + // ─────────────────────────────────────────────────────────────────────── - scroll_to_bottom(); + // ── commit, brief LVGL lock ──────────────────────────────────────────── + { + LVGL_LOCK(); + // The conversation changed (or this peer was re-opened) while the + // I/O ran; the newer open owns the UI now. + if (_peer_hash != peer_hash || _prepare_generation != generation) { + return; + } + if (peer_name.length() > 0) { + lv_obj_t* label_peer = lv_obj_get_child(_header, 1); // Second child is peer label + lv_label_set_text(label_peer, peer_name.c_str()); + } + _all_message_hashes = std::move(all_hashes); + for (const auto& item : items) { + _messages.push_back(item); + create_message_bubble(item); + } + _display_start_idx = display_start_idx; + + // Queue the rest of the first page to stream in on the main loop. Set + // the target before activating so tick sees a consistent target. + _bg_fill_target = (_all_message_hashes.size() > MESSAGES_PER_PAGE) + ? _all_message_hashes.size() - MESSAGES_PER_PAGE + : 0; + const bool initial_fill_active = _display_start_idx > _bg_fill_target; + _keep_bottom_during_background_fill.store(initial_fill_active); + _bg_fill_active.store(initial_fill_active); + + _prepared_peer_hash = peer_hash; + scroll_to_bottom(); + } +} + +void ChatScreen::refresh() { + // Request a full re-gather of this conversation's content: the main + // loop's prepare_conversation() does the store reads off the LVGL lock + // and commits the result under a brief lock. (The old synchronous + // implementation re-read the index + every page of metadata here under + // the lock — the same stall class this fix removes.) Bumping the + // generation discards any in-flight prepare so the re-gather is fresh. + LVGL_LOCK(); + if (!_message_store || _peer_hash.size() == 0) { + return; + } + INFO("Refreshing chat messages"); + _prepared_peer_hash = Bytes(); + _prepare_generation++; } // Stream older messages in a few at a time, called from UIManager::update() on diff --git a/lib/tdeck_ui/UI/LXMF/ChatScreen.h b/lib/tdeck_ui/UI/LXMF/ChatScreen.h index 2cbfd206..dc28daf0 100644 --- a/lib/tdeck_ui/UI/LXMF/ChatScreen.h +++ b/lib/tdeck_ui/UI/LXMF/ChatScreen.h @@ -83,6 +83,21 @@ public: */ void load_conversation(const RNS::Bytes& peer_hash, ::LXMF::MessageStore& store); + /** + * Prepare the current conversation's content. Call from + * UIManager::update() on the main loop after the chat route is active. + * + * load_conversation() (LVGL task) only navigates and clears the list; + * the store reads (identity recall, display-name lookup, message index, + * per-message metadata) are slow on a degraded LittleFS — a cold open + * can take several seconds — so running them under the LVGL lock (as the + * old synchronous path did) held the mutex past the 5s deadlock guard + * and rebooted the device (assert at LVGLLock.h:45). This method does + * that I/O on the main loop, then commits header + initial bubbles + * under a brief LVGL_LOCK. No-op when this peer is already prepared. + */ + void prepare_conversation(); + /** * Add a new message to the chat * @param message LXMF message to add @@ -185,6 +200,14 @@ private: // Map message hash to bubble row for targeted updates std::map _message_rows; + // Conversation-prepare state (main-loop I/O + LVGL commit). Both fields are + // only read/written while holding the LVGL lock (load_conversation, the + // prepare guard, and the commit are all locked sections), so they need no + // atomics. _prepare_generation disambiguates a same-peer re-open that + // happens while a prepare's I/O is in flight. + RNS::Bytes _prepared_peer_hash; + uint32_t _prepare_generation = 0; + BackCallback _back_callback; SendMessageCallback _send_message_callback; CallCallback _call_callback; diff --git a/lib/tdeck_ui/UI/LXMF/UIManager.cpp b/lib/tdeck_ui/UI/LXMF/UIManager.cpp index eb633f31..cde398b1 100644 --- a/lib/tdeck_ui/UI/LXMF/UIManager.cpp +++ b/lib/tdeck_ui/UI/LXMF/UIManager.cpp @@ -818,6 +818,12 @@ void UIManager::update() { // loop, so each small batch only briefly holds the LVGL lock instead of the // whole page blocking it past LVGLLock's 5s timeout. if (_navigation.current() == Route::CHAT && _chat_screen) { + // Conversation open: the LVGL task only navigated + cleared the list + // (load_conversation); the store reads + bubble build run here on the + // main loop so a slow cold open can't hold the LVGL mutex past the 5s + // deadlock guard (same fix class as the send path). No-op once this + // peer is prepared; load_conversation() re-arms it per peer open. + _chat_screen->prepare_conversation(); _chat_screen->tick_background_fill(); // Long-press full-message view: the LVGL event handler only records // the hash; the disk read + modal build run here on the main loop. diff --git a/tests/build_scripts/test_chat_scroll_contract.py b/tests/build_scripts/test_chat_scroll_contract.py index 940c3ff3..df8836be 100644 --- a/tests/build_scripts/test_chat_scroll_contract.py +++ b/tests/build_scripts/test_chat_scroll_contract.py @@ -16,7 +16,7 @@ def function_body(source: str, signature: str, next_signature: str) -> str: def test_initial_background_fill_stays_at_newest_message(): source = CHAT_CPP.read_text() header = CHAT_H.read_text() - refresh = function_body(source, "void ChatScreen::refresh()", "void ChatScreen::tick_background_fill()") + prepare = function_body(source, "void ChatScreen::prepare_conversation()", "void ChatScreen::refresh()") tick = function_body(source, "void ChatScreen::tick_background_fill()", "void ChatScreen::load_more_messages(") on_scroll = function_body(source, "void ChatScreen::on_scroll(", "void ChatScreen::create_message_bubble(") assert "void ChatScreen::scroll_to_bottom()" in source @@ -28,10 +28,26 @@ def test_initial_background_fill_stays_at_newest_message(): "lv_obj_scroll_to_y(_message_list, LV_COORD_MAX, LV_ANIM_OFF)" ) - keep = refresh.index("_keep_bottom_during_background_fill.store(initial_fill_active)") - activate = refresh.index("_bg_fill_active.store(initial_fill_active)") - bottom = refresh.index("scroll_to_bottom()") - assert keep < activate < bottom + # Cold-open I/O (identity recall, display name, message index, per-message + # metadata) runs on the main loop in prepare_conversation, between the + # guard lock and the commit lock — never inside a held LVGL lock, so a + # slow LittleFS open can't hold the mutex past the 5s deadlock guard. + i_io = prepare.index("Identity::recall_app_data") + i_index = prepare.index("get_messages_for_conversation") + i_meta = prepare.index("load_message_metadata") + i_commit_lock = prepare.index("commit, brief LVGL lock") + i_rows = prepare.index("create_message_bubble(item)") + i_target = prepare.index("_bg_fill_target =") + i_keep = prepare.index("_keep_bottom_during_background_fill.store(initial_fill_active)") + i_activate = prepare.index("_bg_fill_active.store(initial_fill_active)") + i_bottom = prepare.index("scroll_to_bottom()") + assert i_commit_lock < i_rows < i_target < i_keep < i_activate < i_bottom + assert i_io < i_index < i_meta < i_commit_lock + # refresh() must not do the slow store reads itself (it just re-arms the + # prepare; the main loop does the I/O off-lock). + refresh = function_body(source, "void ChatScreen::refresh()", "void ChatScreen::tick_background_fill()") + assert "load_message_metadata" not in refresh + assert "get_messages_for_conversation" not in refresh load = tick.index("load_more_messages(") keep_check = tick.index("if (_keep_bottom_during_background_fill.load())")