From b0eba1a060359cac4938aaf7139481ac50d470ec Mon Sep 17 00:00:00 2001 From: Claire Date: Fri, 21 Apr 2023 16:53:50 +0200 Subject: [PATCH 1/3] Minor clean up and optimization of the automatic post deletion code (#24613) --- app/models/account_statuses_cleanup_policy.rb | 28 +++++++++++-------- 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/app/models/account_statuses_cleanup_policy.rb b/app/models/account_statuses_cleanup_policy.rb index 14ce00abbc..63582b9f9e 100644 --- a/app/models/account_statuses_cleanup_policy.rb +++ b/app/models/account_statuses_cleanup_policy.rb @@ -57,9 +57,9 @@ class AccountStatusesCleanupPolicy < ApplicationRecord before_save :update_last_inspected def statuses_to_delete(limit = 50, max_id = nil, min_id = nil) - scope = account.statuses + scope = account_statuses scope.merge!(old_enough_scope(max_id)) - scope = scope.where(Status.arel_table[:id].gteq(min_id)) if min_id.present? + scope = scope.where(id: min_id..) if min_id.present? scope.merge!(without_popular_scope) unless min_favs.nil? && min_reblogs.nil? scope.merge!(without_direct_scope) if keep_direct? scope.merge!(without_pinned_scope) if keep_pinned? @@ -80,7 +80,7 @@ class AccountStatusesCleanupPolicy < ApplicationRecord def compute_cutoff_id min_id = last_inspected || 0 max_id = Mastodon::Snowflake.id_at(min_status_age.seconds.ago, with_random: false) - subquery = account.statuses.where(Status.arel_table[:id].gteq(min_id)).where(Status.arel_table[:id].lteq(max_id)) + subquery = account_statuses.where(id: min_id..max_id) subquery = subquery.select(:id).reorder(id: :asc).limit(EARLY_SEARCH_CUTOFF) # We're textually interpolating a subquery here as ActiveRecord seem to not provide @@ -91,11 +91,11 @@ class AccountStatusesCleanupPolicy < ApplicationRecord # The most important thing about `last_inspected` is that any toot older than it is guaranteed # not to be kept by the policy regardless of its age. def record_last_inspected(last_id) - redis.set("account_cleanup:#{account.id}", last_id, ex: 1.week.seconds) + redis.set("account_cleanup:#{account_id}", last_id, ex: 2.weeks.seconds) end def last_inspected - redis.get("account_cleanup:#{account.id}")&.to_i + redis.get("account_cleanup:#{account_id}")&.to_i end def invalidate_last_inspected(status, action) @@ -120,9 +120,9 @@ class AccountStatusesCleanupPolicy < ApplicationRecord if EXCEPTION_BOOLS.map { |name| attribute_change_to_be_saved(name) }.compact.include?([true, false]) # Policy has been widened in such a way that any previously-inspected status # may need to be deleted, so we'll have to start again. - redis.del("account_cleanup:#{account.id}") + redis.del("account_cleanup:#{account_id}") end - redis.del("account_cleanup:#{account.id}") if EXCEPTION_THRESHOLDS.map { |name| attribute_change_to_be_saved(name) }.compact.any? { |old, new| old.present? && (new.nil? || new > old) } + redis.del("account_cleanup:#{account_id}") if EXCEPTION_THRESHOLDS.map { |name| attribute_change_to_be_saved(name) }.compact.any? { |old, new| old.present? && (new.nil? || new > old) } end def validate_local_account @@ -141,23 +141,23 @@ class AccountStatusesCleanupPolicy < ApplicationRecord max_id = snowflake_id if max_id.nil? || snowflake_id < max_id - Status.where(Status.arel_table[:id].lteq(max_id)) + Status.where(id: ..max_id) end def without_self_fav_scope - Status.where('NOT EXISTS (SELECT * FROM favourites fav WHERE fav.account_id = statuses.account_id AND fav.status_id = statuses.id)') + Status.where('NOT EXISTS (SELECT 1 FROM favourites fav WHERE fav.account_id = statuses.account_id AND fav.status_id = statuses.id)') end def without_self_bookmark_scope - Status.where('NOT EXISTS (SELECT * FROM bookmarks bookmark WHERE bookmark.account_id = statuses.account_id AND bookmark.status_id = statuses.id)') + Status.where('NOT EXISTS (SELECT 1 FROM bookmarks bookmark WHERE bookmark.account_id = statuses.account_id AND bookmark.status_id = statuses.id)') end def without_pinned_scope - Status.where('NOT EXISTS (SELECT * FROM status_pins pin WHERE pin.account_id = statuses.account_id AND pin.status_id = statuses.id)') + Status.where('NOT EXISTS (SELECT 1 FROM status_pins pin WHERE pin.account_id = statuses.account_id AND pin.status_id = statuses.id)') end def without_media_scope - Status.where('NOT EXISTS (SELECT * FROM media_attachments media WHERE media.status_id = statuses.id)') + Status.where('NOT EXISTS (SELECT 1 FROM media_attachments media WHERE media.status_id = statuses.id)') end def without_poll_scope @@ -170,4 +170,8 @@ class AccountStatusesCleanupPolicy < ApplicationRecord scope = scope.where('COALESCE(status_stats.favourites_count, 0) < ?', min_favs) unless min_favs.nil? scope end + + def account_statuses + Status.where(account_id: account_id) + end end From fbb4de3dbc39e3874a257b37c5c5104e1f887c86 Mon Sep 17 00:00:00 2001 From: Claire Date: Fri, 21 Apr 2023 18:08:28 +0200 Subject: [PATCH 2/3] Fix infinite loop in emoji replacement code (#24615) --- .../mastodon/features/emoji/emoji.js | 60 +++++++++++-------- 1 file changed, 34 insertions(+), 26 deletions(-) diff --git a/app/javascript/mastodon/features/emoji/emoji.js b/app/javascript/mastodon/features/emoji/emoji.js index fe4f6f2869..06800d4329 100644 --- a/app/javascript/mastodon/features/emoji/emoji.js +++ b/app/javascript/mastodon/features/emoji/emoji.js @@ -29,15 +29,15 @@ const emojifyTextNode = (node, customEmojis) => { let i = 0; for (;;) { - let match; + let unicode_emoji; - // Skip to the next potential emoji to replace + // Skip to the next potential emoji to replace (either custom emoji or custom emoji :shortcode: if (customEmojis === null) { - while (i < str.length && !(match = trie.search(str.slice(i)))) { + while (i < str.length && !(unicode_emoji = trie.search(str.slice(i)))) { i += str.codePointAt(i) < 65536 ? 1 : 2; } } else { - while (i < str.length && str[i] !== ':' && !(match = trie.search(str.slice(i)))) { + while (i < str.length && str[i] !== ':' && !(unicode_emoji = trie.search(str.slice(i)))) { i += str.codePointAt(i) < 65536 ? 1 : 2; } } @@ -48,29 +48,37 @@ const emojifyTextNode = (node, customEmojis) => { } let rend, replacement = null; - if (str[i] === ':') { // Potentially the start of a custom emoji shortcode - if (!(rend = str.indexOf(':', i + 1) + 1)) { - continue; // no pair of ':' - } + if (str[i] === ':') { // Potentially the start of a custom emoji :shortcode: + rend = str.indexOf(':', i + 1) + 1; - const shortname = str.slice(i, rend); - // now got a replacee as ':shortname:' - // if you want additional emoji handler, add statements below which set replacement and return true. - if (shortname in customEmojis) { - const filename = autoPlayGif ? customEmojis[shortname].url : customEmojis[shortname].static_url; - replacement = document.createElement('img'); - replacement.setAttribute('draggable', 'false'); - replacement.setAttribute('class', 'emojione custom-emoji'); - replacement.setAttribute('alt', shortname); - replacement.setAttribute('title', shortname); - replacement.setAttribute('src', filename); - replacement.setAttribute('data-original', customEmojis[shortname].url); - replacement.setAttribute('data-static', customEmojis[shortname].static_url); - } else { + // no matching ending ':', skip + if (!rend) { + i++; continue; } - } else { // matched to unicode emoji - rend = i + match.length; + + const shortcode = str.slice(i, rend); + const custom_emoji = customEmojis[shortcode]; + + // not a recognized shortcode, skip + if (!custom_emoji) { + i++; + continue; + } + + // now got a replacee as ':shortcode:' + // if you want additional emoji handler, add statements below which set replacement and return true. + const filename = autoPlayGif ? custom_emoji.url : custom_emoji.static_url; + replacement = document.createElement('img'); + replacement.setAttribute('draggable', 'false'); + replacement.setAttribute('class', 'emojione custom-emoji'); + replacement.setAttribute('alt', shortcode); + replacement.setAttribute('title', shortcode); + replacement.setAttribute('src', filename); + replacement.setAttribute('data-original', custom_emoji.url); + replacement.setAttribute('data-static', custom_emoji.static_url); + } else { // start of an unicode emoji + rend = i + unicode_emoji.length; // If the matched character was followed by VS15 (for selecting text presentation), skip it. if (str.codePointAt(rend - 1) !== VS16 && str.codePointAt(rend) === VS15) { @@ -78,13 +86,13 @@ const emojifyTextNode = (node, customEmojis) => { continue; } - const { filename, shortCode } = unicodeMapping[match]; + const { filename, shortCode } = unicodeMapping[unicode_emoji]; const title = shortCode ? `:${shortCode}:` : ''; replacement = document.createElement('img'); replacement.setAttribute('draggable', 'false'); replacement.setAttribute('class', 'emojione'); - replacement.setAttribute('alt', match); + replacement.setAttribute('alt', unicode_emoji); replacement.setAttribute('title', title); replacement.setAttribute('src', `${assetHost}/emoji/${emojiFilename(filename)}.svg`); } From 501d6197c4a32172e2340c90379b9c3fdb925c08 Mon Sep 17 00:00:00 2001 From: Claire Date: Fri, 21 Apr 2023 18:14:19 +0200 Subject: [PATCH 3/3] Change automatic post deletion thresholds and load detection (#24614) --- .../accounts_statuses_cleanup_scheduler.rb | 45 ++++++++++--------- 1 file changed, 24 insertions(+), 21 deletions(-) diff --git a/app/workers/scheduler/accounts_statuses_cleanup_scheduler.rb b/app/workers/scheduler/accounts_statuses_cleanup_scheduler.rb index f237f1dc9c..5203d4c258 100644 --- a/app/workers/scheduler/accounts_statuses_cleanup_scheduler.rb +++ b/app/workers/scheduler/accounts_statuses_cleanup_scheduler.rb @@ -7,28 +7,30 @@ class Scheduler::AccountsStatusesCleanupScheduler # This limit is mostly to be nice to the fediverse at large and not # generate too much traffic. # This also helps limiting the running time of the scheduler itself. - MAX_BUDGET = 150 + MAX_BUDGET = 300 - # This is an attempt to spread the load across instances, as various - # accounts are likely to have various followers. + # This is an attempt to spread the load across remote servers, as + # spreading deletions across diverse accounts is likely to spread + # the deletion across diverse followers. It also helps each individual + # user see some effect sooner. PER_ACCOUNT_BUDGET = 5 # This is an attempt to limit the workload generated by status removal - # jobs to something the particular instance can handle. - PER_THREAD_BUDGET = 6 + # jobs to something the particular server can handle. + PER_THREAD_BUDGET = 5 - # Those avoid loading an instance that is already under load - MAX_DEFAULT_SIZE = 200 - MAX_DEFAULT_LATENCY = 5 - MAX_PUSH_SIZE = 500 - MAX_PUSH_LATENCY = 10 - - # 'pull' queue has lower priority jobs, and it's unlikely that pushing - # deletes would cause much issues with this queue if it didn't cause issues - # with default and push. Yet, do not enqueue deletes if the instance is - # lagging behind too much. - MAX_PULL_SIZE = 10_000 - MAX_PULL_LATENCY = 5.minutes.to_i + # These are latency limits on various queues above which a server is + # considered to be under load, causing the auto-deletion to be entirely + # skipped for that run. + LOAD_LATENCY_THRESHOLDS = { + default: 5, + push: 10, + # The `pull` queue has lower priority jobs, and it's unlikely that + # pushing deletes would cause much issues with this queue if it didn't + # cause issues with `default` and `push`. Yet, do not enqueue deletes + # if the instance is lagging behind too much. + pull: 5.minutes.to_i, + }.freeze sidekiq_options retry: 0, lock: :until_executed, lock_ttl: 1.day.to_i @@ -62,19 +64,20 @@ class Scheduler::AccountsStatusesCleanupScheduler end def compute_budget + # Each post deletion is a `RemovalWorker` job (on `default` queue), each + # potentially spawning many `ActivityPub::DeliveryWorker` jobs (on the `push` queue). threads = Sidekiq::ProcessSet.new.select { |x| x['queues'].include?('push') }.pluck('concurrency').sum [PER_THREAD_BUDGET * threads, MAX_BUDGET].min end def under_load? - queue_under_load?('default', MAX_DEFAULT_SIZE, MAX_DEFAULT_LATENCY) || queue_under_load?('push', MAX_PUSH_SIZE, MAX_PUSH_LATENCY) || queue_under_load?('pull', MAX_PULL_SIZE, MAX_PULL_LATENCY) + LOAD_LATENCY_THRESHOLDS.any? { |queue, max_latency| queue_under_load?(queue, max_latency) } end private - def queue_under_load?(name, max_size, max_latency) - queue = Sidekiq::Queue.new(name) - queue.size > max_size || queue.latency > max_latency + def queue_under_load?(name, max_latency) + Sidekiq::Queue.new(name).latency > max_latency end def last_processed_id