From a84dfb02098c12866c8c52493c910210381a0a83 Mon Sep 17 00:00:00 2001 From: Aman Agrawal Date: Tue, 19 Nov 2024 15:11:32 +0530 Subject: [PATCH] message_view: Verify msg list data state when restoring cached data. When rendering message list directly from cached data, we sometimes ask server to verify the current data. If the data doesn't match, we include events at the time cached data was restored to help with debugging or to determine if there was race in updating the data. --- web/src/message_fetch.ts | 95 +++++++++++++++++++++++++++++++- web/src/message_view.ts | 4 ++ web/src/server_events.js | 1 + web/tests/server_events.test.cjs | 1 + 4 files changed, 100 insertions(+), 1 deletion(-) diff --git a/web/src/message_fetch.ts b/web/src/message_fetch.ts index ae017f5f00..9450d89f45 100644 --- a/web/src/message_fetch.ts +++ b/web/src/message_fetch.ts @@ -44,7 +44,7 @@ type MessageFetchOptions = { anchor: string | number; num_before: number; num_after: number; - cont: (data: MessageFetchResponse, args: MessageFetchOptions) => void; + cont?: (data: MessageFetchResponse, args: MessageFetchOptions) => void; fetch_again?: boolean; msg_list_data: MessageListData; msg_list?: MessageList | undefined; @@ -650,6 +650,99 @@ export function maybe_load_newer_messages(opts: {msg_list: MessageList}): void { }); } +export function verify_cached_data(data: MessageListData): void { + type EventDetails = { + type: string; + // ...many more properties. + }; + let events_since_restoring_cached_data: EventDetails[] = []; + // Since we are in tight race with events modifying data on + // server, we start by capturing any events that the client + // will receive to include in error logs for debugging. + $(document).on("server_event.zulip", (e) => { + events_since_restoring_cached_data = [ + // @ts-expect-error: Fix by adding `events` as type to TriggeredEvent. + // eslint-disable-next-line @typescript-eslint/consistent-type-assertions + ...(e.events as EventDetails[]), + ...events_since_restoring_cached_data, + ]; + }); + + // Extract the data we want to verify to avoid it being + // changed by the time we received data from the server. + const messages = [...data.all_messages()]; + // For empty narrows, we don't have a `local_id` to select, + // so we always end up contacting server for the latest data. + // Hence, we never reach here to verify data in that case. + assert(messages.length !== 0); + const has_found_newest = data.fetch_status.has_found_newest(); + const has_found_oldest = data.fetch_status.has_found_oldest(); + const history_limited = data.fetch_status.history_limited(); + const first_message = messages[0]; + assert(first_message !== undefined); + let anchor = first_message.id; + // If we have the oldest message, we can verify that the fetch + // status for that is correct by anchoring our request to first + // message. Since `num_before` is `0`, this is our only way of + // knowing if our fetch request has found the oldest data. + if (has_found_oldest) { + anchor = 0; + } + let num_after = messages.length; + // We need to fetch more than the number of messages in the data, + // to verify if the fetched data has has_found_newest. + if (has_found_newest) { + num_after += 1; + } + const opts: MessageFetchOptions = { + anchor, + num_before: 0, + num_after, + msg_list_data: data, + }; + const fetch_request_params = get_parameters_for_message_fetch_api(opts); + void channel.get({ + url: "/json/messages", + data: fetch_request_params, + success(raw_data) { + const data = response_schema.parse(raw_data); + + // Verify that cached data with response from server. + try { + if (has_found_newest) { + assert(data.found_newest); + } + assert(data.messages.length === messages.length); + const cached_msg_ids = new Set(messages.map((msg) => msg.id)); + for (const msg of data.messages) { + assert(cached_msg_ids.has(msg.id)); + } + $(document).off("server_event.zulip"); + } catch (error) { + setTimeout(() => { + blueslip.error( + "Mismatching cached and server data.", + { + fetch_request_params, + server_data: data, + cached_data: { + has_found_newest, + has_found_oldest, + history_limited, + messages, + }, + events_since_restoring_cached_data, + }, + error, + ); + $(document).off("server_event.zulip"); + // Allow 10s for us receive any more relevant events. + }, 10000); + } + }, + }); +} + export function set_initial_pointer_and_offset({ narrow_pointer, narrow_offset, diff --git a/web/src/message_view.ts b/web/src/message_view.ts index b32078cc64..46c9644947 100644 --- a/web/src/message_view.ts +++ b/web/src/message_view.ts @@ -693,6 +693,10 @@ export let show = (raw_terms: NarrowTerm[], show_opts: ShowMessageViewOpts): voi } if (select_immediately) { + // Verify that cached data is correct 5% of the time. + if (DEVELOPMENT || Math.random() < 0.05) { + message_fetch.verify_cached_data(msg_list.data); + } // We can skip the initial fetch since we already have a // message we can render and select and sufficient messages // rendered in the view to provide context around the anchor. diff --git a/web/src/server_events.js b/web/src/server_events.js index e679711ff6..7141677b79 100644 --- a/web/src/server_events.js +++ b/web/src/server_events.js @@ -33,6 +33,7 @@ const get_events_params = {}; let event_queue_expired = false; function get_events_success(events) { + $(document).trigger(new $.Event("server_event.zulip", {events})); let messages = []; const update_message_events = []; const post_message_events = []; diff --git a/web/tests/server_events.test.cjs b/web/tests/server_events.test.cjs index 62d31f825b..79fdd4ece8 100644 --- a/web/tests/server_events.test.cjs +++ b/web/tests/server_events.test.cjs @@ -11,6 +11,7 @@ mock_esm("../src/loading", { destroy_indicator: noop, }); set_global("addEventListener", noop); +set_global("document", "document-stub"); const channel = mock_esm("../src/channel"); mock_esm("../src/reload_state", {