diff --git a/backend/canisters/user/CHANGELOG.md b/backend/canisters/user/CHANGELOG.md index ec91a15857..3529c50b67 100644 --- a/backend/canisters/user/CHANGELOG.md +++ b/backend/canisters/user/CHANGELOG.md @@ -5,6 +5,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). ## [unreleased] +### Removed + +- Remove group summary cache ([#5067](https://github.com/open-chat-labs/open-chat/pull/5067)) + ## [[2.0.989](https://github.com/open-chat-labs/open-chat/releases/tag/v2.0.989-user)] - 2023-12-20 ### Added diff --git a/backend/canisters/user/api/can.did b/backend/canisters/user/api/can.did index 71fee921da..60e90e3849 100644 --- a/backend/canisters/user/api/can.did +++ b/backend/canisters/user/api/can.did @@ -737,10 +737,6 @@ type MigrateUserPrincipalResponse = variant { InternalError : text; }; -type InitialStateArgs = record { - disable_cache : opt bool; -}; - type InitialStateResponse = variant { Success : record { timestamp : TimestampMillis; @@ -1043,7 +1039,7 @@ service : { messages_by_message_index : (MessagesByMessageIndexArgs) -> (MessagesByMessageIndexResponse) query; deleted_message : (DeletedMessageArgs) -> (DeletedMessageResponse) query; - initial_state : (InitialStateArgs) -> (InitialStateResponse) query; + initial_state : (EmptyArgs) -> (InitialStateResponse) query; updates : (UpdatesArgs) -> (UpdatesResponse) query; search_messages : (SearchMessagesArgs) -> (SearchMessagesResponse) query; // Search just the messages of one direct chat bio : (BioArgs) -> (BioResponse) query; diff --git a/backend/canisters/user/api/src/queries/initial_state.rs b/backend/canisters/user/api/src/queries/initial_state.rs index f1db9d4f9f..1c95ad482c 100644 --- a/backend/canisters/user/api/src/queries/initial_state.rs +++ b/backend/canisters/user/api/src/queries/initial_state.rs @@ -1,11 +1,8 @@ use candid::CandidType; use serde::{Deserialize, Serialize}; -use types::{Chat, ChatId, DirectChatSummary, GroupChatSummary, TimestampMillis, UserId}; +use types::{Chat, ChatId, DirectChatSummary, Empty, GroupChatSummary, TimestampMillis, UserId}; -#[derive(CandidType, Serialize, Deserialize, Debug)] -pub struct Args { - pub disable_cache: Option, -} +pub type Args = Empty; #[derive(CandidType, Serialize, Deserialize, Debug)] pub enum Response { diff --git a/backend/canisters/user/impl/src/group_summaries.rs b/backend/canisters/user/impl/src/group_summaries.rs deleted file mode 100644 index e7623d8d25..0000000000 --- a/backend/canisters/user/impl/src/group_summaries.rs +++ /dev/null @@ -1,224 +0,0 @@ -use crate::{CachedGroupSummaries, Data}; -use group_index_canister::c2c_active_groups; -use ic_cdk::api::call::CallResult; -use itertools::Itertools; -use std::collections::{HashMap, HashSet}; -use types::{ - CanisterId, ChatId, DeletedGroupInfo, GroupCanisterGroupChatSummary, GroupCanisterGroupChatSummaryUpdates, TimestampMillis, -}; - -#[derive(Debug, Default)] -pub struct UpdatesSince { - pub timestamp: TimestampMillis, - pub group_chats: Vec, -} - -#[derive(Debug)] -pub struct GroupChatUpdatesSince { - pub chat_id: ChatId, - pub updates_since: TimestampMillis, -} - -pub(crate) struct SummariesArgs { - group_index_canister_id: CanisterId, - group_chat_ids: Vec, - cached_group_summaries: Option, - pub now: TimestampMillis, -} - -pub(crate) struct UpdatesArgs { - group_index_canister_id: CanisterId, - updates_since: UpdatesSince, - group_chat_ids: Vec, - group_chat_ids_with_my_changes: Vec, -} - -pub(crate) struct Updates { - pub added: Vec, - pub updated: Vec, - pub deleted: Vec, -} - -pub(crate) fn build_summaries_args(disable_cache: bool, now: TimestampMillis, data: &Data) -> SummariesArgs { - SummariesArgs { - group_index_canister_id: data.group_index_canister_id, - group_chat_ids: data.group_chats.iter().map(|g| g.chat_id).collect(), - cached_group_summaries: if disable_cache { None } else { data.cached_group_summaries.clone() }, - now, - } -} - -pub(crate) struct SummariesResult { - pub summaries: Vec, - pub deleted: Vec, -} - -pub(crate) async fn summaries(args: SummariesArgs) -> Result { - let updates_args = UpdatesArgs { - group_index_canister_id: args.group_index_canister_id, - updates_since: args - .cached_group_summaries - .as_ref() - .map_or(UpdatesSince::default(), |c| c.updates_args()), - group_chat_ids: args.group_chat_ids, - group_chat_ids_with_my_changes: Vec::new(), - }; - - let updates = updates(updates_args).await?; - - let summaries = if let Some(cached) = args.cached_group_summaries { - let mut merged = merge_updates(cached.groups, updates.updated); - if !updates.deleted.is_empty() { - let deleted: HashSet<_> = updates.deleted.iter().map(|d| d.id).collect(); - merged.retain(|g| !deleted.contains(&g.chat_id)); - } - merged.extend(updates.added); - merged - } else { - updates.added - }; - - Ok(SummariesResult { - summaries, - deleted: updates.deleted, - }) -} - -async fn updates(args: UpdatesArgs) -> Result { - let group_chat_args_map: HashMap<_, _> = args - .updates_since - .group_chats - .iter() - .map(|g| (g.chat_id, g.updates_since)) - .collect(); - - let mut group_chats_added = Vec::new(); - let mut group_chats_to_check_for_updates = Vec::new(); - - for chat_id in args.group_chat_ids { - if let Some(updates_since) = group_chat_args_map.get(&chat_id) { - group_chats_to_check_for_updates.push((chat_id, *updates_since)); - } else { - group_chats_added.push(chat_id); - } - } - - let mut all_groups: Vec<_> = group_chats_added.clone(); - all_groups.extend(group_chats_to_check_for_updates.iter().map(|(id, _)| *id)); - - let mut added = Vec::new(); - let mut updated = Vec::new(); - let mut deleted = Vec::new(); - if !all_groups.is_empty() { - let active_groups_args = c2c_active_groups::Args { - group_ids: all_groups, - community_ids: Vec::new(), - active_since: Some(args.updates_since.timestamp), - }; - let active_groups_result = - match group_index_canister_c2c_client::c2c_active_groups(args.group_index_canister_id, &active_groups_args).await { - Ok(c2c_active_groups::Response::Success(result)) => result, - Err(error) => return Err(format!("Failed to call 'c2c_active_groups': {error:?}")), - }; - - let active_groups: HashSet<_> = active_groups_result.active_groups.into_iter().collect(); - group_chats_added.retain(|id| !has_group_been_deleted(&active_groups_result.deleted_groups, id)); - group_chats_to_check_for_updates - .retain(|(id, _)| active_groups.contains(id) || args.group_chat_ids_with_my_changes.contains(id)); - - let summaries_future = c2c::summaries(group_chats_added); - let summary_updates_future = c2c::summary_updates(group_chats_to_check_for_updates); - - let (s, su) = futures::future::join(summaries_future, summary_updates_future).await; - - added = s.map_err(|(code, msg)| format!("Failed to get summaries. {code:?}: {msg}"))?; - updated = su.map_err(|(code, msg)| format!("Failed to get summary updates. {code:?}: {msg}"))?; - deleted = active_groups_result.deleted_groups; - } - - Ok(Updates { added, updated, deleted }) -} - -fn merge_updates( - summaries: Vec, - updates: Vec, -) -> Vec { - if updates.is_empty() { - summaries - } else { - let mut updates_map: HashMap<_, _> = updates.into_iter().map(|s| (s.chat_id, s)).collect(); - - summaries - .into_iter() - .map(|s| if let Some(u) = updates_map.remove(&s.chat_id) { s.merge(u) } else { s }) - .collect() - } -} - -fn has_group_been_deleted(groups: &[DeletedGroupInfo], group_id: &ChatId) -> bool { - groups.iter().any(|g| g.id == *group_id) -} - -mod c2c { - use super::*; - - pub async fn summaries(chat_ids: Vec) -> CallResult> { - if chat_ids.is_empty() { - return Ok(Vec::new()); - } - - let mut summaries = Vec::new(); - let args = group_canister::c2c_summary::Args { on_behalf_of: None }; - for batch in &chat_ids.into_iter().chunks(5) { - let futures: Vec<_> = batch - .map(|chat_id| group_canister_c2c_client::c2c_summary(chat_id.into(), &args)) - .collect(); - - // Exit if any failed, this ensures we never miss any updates - for response in futures::future::try_join_all(futures).await? { - if let group_canister::c2c_summary::Response::Success(result) = response { - summaries.push(result.summary); - } - } - } - - Ok(summaries) - } - - pub async fn summary_updates( - group_chats: Vec<(ChatId, TimestampMillis)>, - ) -> CallResult> { - if group_chats.is_empty() { - return Ok(Vec::new()); - } - - async fn get_summary_updates( - canister_id: CanisterId, - args: group_canister::c2c_summary_updates::Args, - ) -> CallResult { - group_canister_c2c_client::c2c_summary_updates(canister_id, &args).await - } - - let mut summary_updates = Vec::new(); - for batch in &group_chats.into_iter().chunks(5) { - let futures: Vec<_> = batch - .map(|(g, t)| { - let args = group_canister::c2c_summary_updates::Args { - updates_since: t, - on_behalf_of: None, - }; - get_summary_updates(g.into(), args) - }) - .collect(); - - // Exit if any failed, this ensures we never miss any updates - for response in futures::future::try_join_all(futures).await? { - if let group_canister::c2c_summary_updates::Response::Success(result) = response { - summary_updates.push(result.updates); - } - } - } - - Ok(summary_updates) - } -} diff --git a/backend/canisters/user/impl/src/lib.rs b/backend/canisters/user/impl/src/lib.rs index d7c7c5523d..1b861650c8 100644 --- a/backend/canisters/user/impl/src/lib.rs +++ b/backend/canisters/user/impl/src/lib.rs @@ -1,4 +1,3 @@ -use crate::model::cached_group_summaries::CachedGroupSummaries; use crate::model::communities::Communities; use crate::model::community::Community; use crate::model::direct_chats::DirectChats; @@ -30,7 +29,6 @@ use utils::regular_jobs::RegularJobs; mod crypto; mod governance_clients; -mod group_summaries; mod guards; mod lifecycle; mod memory; @@ -161,7 +159,6 @@ struct Data { pub group_index_canister_id: CanisterId, pub notifications_canister_id: CanisterId, pub proposals_bot_canister_id: CanisterId, - #[serde(default = "escrow_canister_id")] pub escrow_canister_id: CanisterId, pub avatar: Timestamped>, pub test_mode: bool, @@ -170,7 +167,6 @@ struct Data { pub username: Timestamped, pub display_name: Timestamped>, pub bio: Timestamped, - pub cached_group_summaries: Option, pub storage_limit: u64, pub phone_is_verified: bool, pub user_created: TimestampMillis, @@ -183,15 +179,10 @@ struct Data { pub saved_crypto_accounts: Vec, pub next_event_expiry: Option, pub token_swaps: TokenSwaps, - #[serde(default)] pub p2p_trades: P2PTrades, pub rng_seed: [u8; 32], } -fn escrow_canister_id() -> CanisterId { - CanisterId::from_text("s4yi7-yiaaa-aaaar-qacpq-cai").unwrap() -} - impl Data { #[allow(clippy::too_many_arguments)] pub fn new( @@ -226,7 +217,6 @@ impl Data { username: Timestamped::new(username, now), display_name: Timestamped::default(), bio: Timestamped::new("".to_string(), now), - cached_group_summaries: None, storage_limit: 0, phone_is_verified: false, user_created: now, @@ -263,11 +253,6 @@ impl Data { pub fn remove_group(&mut self, chat_id: ChatId, now: TimestampMillis) -> Option { self.favourite_chats.remove(&Chat::Group(chat_id), now); self.hot_group_exclusions.add(chat_id, None, now); - - if let Some(cached_groups) = &mut self.cached_group_summaries { - cached_groups.remove_group(&chat_id); - } - self.group_chats.remove(chat_id, now) } diff --git a/backend/canisters/user/impl/src/model/cached_group_summaries.rs b/backend/canisters/user/impl/src/model/cached_group_summaries.rs deleted file mode 100644 index a9a24ed6da..0000000000 --- a/backend/canisters/user/impl/src/model/cached_group_summaries.rs +++ /dev/null @@ -1,29 +0,0 @@ -use crate::group_summaries::{GroupChatUpdatesSince, UpdatesSince}; -use serde::{Deserialize, Serialize}; -use types::{ChatId, GroupCanisterGroupChatSummary, TimestampMillis}; - -#[derive(Serialize, Deserialize, Default, Clone)] -pub struct CachedGroupSummaries { - pub timestamp: TimestampMillis, - pub groups: Vec, -} - -impl CachedGroupSummaries { - pub fn updates_args(&self) -> UpdatesSince { - UpdatesSince { - timestamp: self.timestamp, - group_chats: self - .groups - .iter() - .map(|g| GroupChatUpdatesSince { - chat_id: g.chat_id, - updates_since: g.last_updated, - }) - .collect(), - } - } - - pub fn remove_group(&mut self, chat_id: &ChatId) { - self.groups.retain(|g| g.chat_id != *chat_id); - } -} diff --git a/backend/canisters/user/impl/src/model/mod.rs b/backend/canisters/user/impl/src/model/mod.rs index 71f8ce342a..c127be4629 100644 --- a/backend/canisters/user/impl/src/model/mod.rs +++ b/backend/canisters/user/impl/src/model/mod.rs @@ -1,4 +1,3 @@ -pub mod cached_group_summaries; pub mod communities; pub mod community; pub mod contacts; diff --git a/backend/canisters/user/impl/src/queries/initial_state.rs b/backend/canisters/user/impl/src/queries/initial_state.rs index 4d56c7d157..593a643d09 100644 --- a/backend/canisters/user/impl/src/queries/initial_state.rs +++ b/backend/canisters/user/impl/src/queries/initial_state.rs @@ -1,17 +1,15 @@ use crate::guards::caller_is_owner; -use crate::model::group_chat::GroupChat; use crate::{read_state, RuntimeState}; use ic_cdk_macros::query; -use std::collections::HashMap; -use types::{BuildVersion, GroupCanisterGroupChatSummary, GroupChatSummary, ThreadSyncDetails, UserId}; +use types::UserId; use user_canister::initial_state::{Response::*, *}; #[query(guard = "caller_is_owner")] -fn initial_state(args: Args) -> Response { - read_state(|state| initial_state_impl(args, state)) +fn initial_state(_args: Args) -> Response { + read_state(initial_state_impl) } -fn initial_state_impl(args: Args, state: &RuntimeState) -> Response { +fn initial_state_impl(state: &RuntimeState) -> Response { let now = state.env.now(); let my_user_id: UserId = state.env.canister_id().into(); let avatar_id = state.data.avatar.value.as_ref().map(|a| a.id); @@ -22,6 +20,12 @@ fn initial_state_impl(args: Args, state: &RuntimeState) -> Response { pinned: state.data.direct_chats.pinned().to_vec(), }; + let group_chats = GroupChatsInitial { + summaries: state.data.group_chats.iter().map(|g| g.to_summary()).collect(), + pinned: state.data.group_chats.pinned().to_vec(), + cached: None, + }; + let communities = CommunitiesInitial { summaries: state.data.communities.iter().map(|c| c.to_summary()).collect(), }; @@ -31,44 +35,6 @@ fn initial_state_impl(args: Args, state: &RuntimeState) -> Response { pinned: state.data.favourite_chats.pinned().to_vec(), }; - let pinned_groups = state.data.group_chats.pinned().to_vec(); - - let disable_cache = args.disable_cache.unwrap_or_default(); - - let group_chats = if let Some(cached) = (!disable_cache) - .then_some(state.data.cached_group_summaries.as_ref()) - .flatten() - { - // We must handle the scenario where some groups are missing from the cache due to them - // being inaccessible while the cache was refreshed. To do this, we get all groups, and any - // groups not found in the cache are included in `group_chats_added`. - let mut group_chats: HashMap<_, _> = state.data.group_chats.iter().map(|g| (g.chat_id, g)).collect(); - - let cached = CachedGroupChatSummaries { - summaries: cached - .groups - .iter() - .filter_map(|c| group_chats.remove(&c.chat_id).map(|g| (c, g))) - .map(|(c, g)| hydrate_cached_summary(c, g)) - .collect(), - timestamp: cached.timestamp, - }; - - GroupChatsInitial { - summaries: group_chats.values().map(|g| g.to_summary()).collect(), - pinned: pinned_groups, - cached: Some(cached), - } - } else { - let chats = state.data.group_chats.iter().map(|g| g.to_summary()).collect(); - - GroupChatsInitial { - summaries: chats, - pinned: pinned_groups, - cached: None, - } - }; - Success(SuccessResult { timestamp: now, direct_chats, @@ -80,58 +46,3 @@ fn initial_state_impl(args: Args, state: &RuntimeState) -> Response { suspended: state.data.suspended.value, }) } - -fn hydrate_cached_summary(cached: &GroupCanisterGroupChatSummary, user_details: &GroupChat) -> GroupChatSummary { - let mut threads = HashMap::new(); - for thread in cached.latest_threads.iter() { - threads.insert(thread.root_message_index, ThreadSyncDetails::from(thread)); - } - for (&root_message_index, read_up_to) in user_details.messages_read.threads_read.iter() { - threads - .entry(root_message_index) - .or_insert(ThreadSyncDetails { - root_message_index, - latest_event: None, - latest_message: None, - read_up_to: None, - last_updated: 0, - }) - .read_up_to = Some(read_up_to.value); - } - - GroupChatSummary { - chat_id: cached.chat_id, - local_user_index_canister_id: cached.local_user_index_canister_id, - last_updated: cached.last_updated, - name: cached.name.clone(), - description: cached.description.clone(), - subtype: cached.subtype.clone(), - avatar_id: cached.avatar_id, - is_public: cached.is_public, - history_visible_to_new_joiners: cached.history_visible_to_new_joiners, - min_visible_event_index: cached.min_visible_event_index, - min_visible_message_index: cached.min_visible_message_index, - latest_message: cached.latest_message.clone(), - latest_event_index: cached.latest_event_index, - latest_message_index: cached.latest_message_index, - joined: cached.joined, - read_by_me_up_to: user_details.messages_read.read_by_me_up_to.value, - notifications_muted: cached.notifications_muted, - participant_count: cached.participant_count, - role: cached.role, - mentions: cached.mentions.clone(), - permissions_v2: cached.permissions_v2.clone(), - metrics: cached.metrics.clone(), - my_metrics: cached.my_metrics.clone(), - latest_threads: threads.into_values().collect(), - archived: user_details.archived.value, - frozen: cached.frozen.clone(), - wasm_version: BuildVersion::default(), - date_last_pinned: cached.date_last_pinned, - date_read_pinned: user_details.messages_read.date_read_pinned.value, - events_ttl: cached.events_ttl, - events_ttl_last_updated: cached.events_ttl_last_updated, - gate: cached.gate.clone(), - rules_accepted: cached.rules_accepted, - } -} diff --git a/backend/canisters/user/impl/src/regular_jobs.rs b/backend/canisters/user/impl/src/regular_jobs.rs index d08c05a7c0..e5e8241586 100644 --- a/backend/canisters/user/impl/src/regular_jobs.rs +++ b/backend/canisters/user/impl/src/regular_jobs.rs @@ -1,9 +1,7 @@ -use crate::group_summaries::{build_summaries_args, SummariesArgs, SummariesResult}; -use crate::{can_borrow_state, mutate_state, CachedGroupSummaries, Data}; -use tracing::{error, info}; +use crate::Data; use utils::env::Environment; use utils::regular_jobs::{RegularJob, RegularJobs}; -use utils::time::{MINUTE_IN_MS, WEEK_IN_MS}; +use utils::time::MINUTE_IN_MS; pub(crate) fn build() -> RegularJobs { let check_cycles_balance = RegularJob::new("Check cycles balance", check_cycles_balance, 5 * MINUTE_IN_MS); @@ -13,14 +11,11 @@ pub(crate) fn build() -> RegularJobs { 5 * MINUTE_IN_MS, ); let retry_deleting_files = RegularJob::new("Retry deleting files", retry_deleting_files, MINUTE_IN_MS); - let update_cached_group_summaries = - RegularJob::new("Update cached group summaries", update_cached_group_summaries, WEEK_IN_MS); RegularJobs::new(vec![ check_cycles_balance, aggregate_direct_chat_metrics, retry_deleting_files, - update_cached_group_summaries, ]) } @@ -35,39 +30,3 @@ fn aggregate_direct_chat_metrics(_: &dyn Environment, data: &mut Data) { fn retry_deleting_files(_: &dyn Environment, _: &mut Data) { storage_bucket_client::retry_failed(); } - -fn update_cached_group_summaries(env: &dyn Environment, data: &mut Data) { - let summaries_args = build_summaries_args(false, env.now(), data); - - ic_cdk::spawn(update_cached_group_summaries_impl(summaries_args)); -} - -async fn update_cached_group_summaries_impl(args: SummariesArgs) { - let start = args.now; - if let Ok(SummariesResult { summaries, deleted }) = crate::group_summaries::summaries(args).await { - // We need this check because the call to `summaries` may have been synchronous in which - // case we still hold the borrow on `data` which was passed into - // `update_cached_group_summaries`. - if can_borrow_state() { - mutate_state(|state| { - state.data.cached_group_summaries = Some(CachedGroupSummaries { - groups: summaries - .into_iter() - // This ensures we don't cache any groups which have been deleted or the - // user has been removed from, which they were members of at the beginning - // of this async operation. - .filter(|g| state.data.group_chats.exists(&g.chat_id)) - .collect(), - timestamp: start, - }); - let now = state.env.now(); - for group in deleted { - state.data.group_chats.remove(group.id, now); - } - info!("Group summaries cache updated"); - }); - } - } else { - error!("Failed to update group summaries cache"); - } -} diff --git a/backend/integration_tests/src/client/user.rs b/backend/integration_tests/src/client/user.rs index 084c2ae984..e56cec327b 100644 --- a/backend/integration_tests/src/client/user.rs +++ b/backend/integration_tests/src/client/user.rs @@ -155,7 +155,7 @@ pub mod happy_path { env, sender.principal, sender.canister(), - &user_canister::initial_state::Args { disable_cache: None }, + &user_canister::initial_state::Args {}, ); let user_canister::initial_state::Response::Success(result) = response;