diff --git a/.github/changelog/fix-3583-avatar-cache-cleanup b/.github/changelog/fix-3583-avatar-cache-cleanup new file mode 100644 index 0000000000..c36ccf71c1 --- /dev/null +++ b/.github/changelog/fix-3583-avatar-cache-cleanup @@ -0,0 +1,4 @@ +Significance: patch +Type: fixed + +Prevented unused copies of remote profile pictures from accumulating on your server and added automatic cleanup for leftover cached avatars. diff --git a/includes/cache/class-avatar.php b/includes/cache/class-avatar.php index 1dbce36858..5f463acd2e 100644 --- a/includes/cache/class-avatar.php +++ b/includes/cache/class-avatar.php @@ -9,6 +9,8 @@ use Activitypub\Collection\Remote_Actors; +use function Activitypub\object_to_uri; + /** * Avatar cache class. * @@ -125,6 +127,30 @@ public static function maybe_cache( $url, $context, $entity_id = null, $options return $cached_url ?: $url; } + /** + * Cache a remote avatar locally, then drop older versions of it. + * + * Overrides the shared write path so that any avatar hash it leaves behind + * for the same actor is removed in the same call. Remote actors change + * their icon URL frequently, and each change would otherwise pile up an + * orphaned copy of the previous image. + * + * @param string $url The remote URL. + * @param string|int $entity_id The entity identifier (actor post ID). + * @param array $options Optional. Additional options. + * + * @return string|false The local URL on success, false on failure. + */ + public static function cache( $url, $entity_id, $options = array() ) { + $cached_url = parent::cache( $url, $entity_id, $options ); + + if ( $cached_url ) { + self::prune_stale_files( $entity_id, self::generate_hash( $url ) ); + } + + return $cached_url; + } + /** * Maybe clean up cached avatar when actor is deleted. * @@ -169,4 +195,151 @@ public static function save( $actor_id, $avatar_url ) { array( 'max_dimension' => self::MAX_DIMENSION ) ); } + + /** + * Get the hash of the avatar currently referenced by an actor. + * + * Reads the actor's icon directly from post content without running the + * remote media filter, so this never triggers a lazy download. + * + * @since unreleased + * + * @param int $post_id The actor post ID. + * + * @return string|false The md5 hash of the current avatar URL, or false if none. + */ + public static function get_actor_avatar_hash( $post_id ) { + $post = \get_post( $post_id ); + if ( ! $post || empty( $post->post_content ) ) { + return false; + } + + $actor_data = \json_decode( $post->post_content, true ); + if ( empty( $actor_data['icon'] ) ) { + return false; + } + + $avatar_url = object_to_uri( $actor_data['icon'] ); + if ( empty( $avatar_url ) || ! \filter_var( $avatar_url, FILTER_VALIDATE_URL ) ) { + return false; + } + + return self::generate_hash( $avatar_url ); + } + + /** + * Remove cached avatar files that no longer match the current icon. + * + * Keeps any file whose basename starts with the current hash and deletes + * the rest. Runs after the current avatar is written, so the active file + * is never removed. + * + * @since unreleased + * + * @param int $entity_id The actor post ID. + * @param string $current_hash The hash of the current avatar URL. + */ + public static function prune_stale_files( $entity_id, $current_hash ) { + $paths = static::get_storage_paths( $entity_id ); + if ( ! \is_dir( $paths['basedir'] ) ) { + return; + } + + $files = \glob( $paths['basedir'] . '/*' ); + if ( empty( $files ) ) { + return; + } + + $prefix = $current_hash . '.'; + + foreach ( $files as $file ) { + if ( 0 === \strpos( \basename( $file ), $prefix ) ) { + continue; + } + + if ( \is_dir( $file ) ) { + static::delete_directory( $file ); + } else { + static::get_filesystem()->delete( $file ); + } + } + } + + /** + * Clean up stale cached avatars. + * + * Run daily by cron. Deletes orphaned actor directories that no longer + * match an actor post and removes older avatar versions for surviving + * actors. Processed in batches so a large backlog drains over several runs. + * + * @since unreleased + */ + public static function cleanup_actors() { + // Lock the cleanup with an autoload-disabled option so overlapping + // cron workers never run it twice at once. + if ( ! \add_option( 'activitypub_avatar_cache_cleanup_lock', time(), '', false ) ) { + $lock_time = (int) \get_option( 'activitypub_avatar_cache_cleanup_lock' ); + if ( $lock_time && ( time() - $lock_time ) < 30 * MINUTE_IN_SECONDS ) { + return; + } + \delete_option( 'activitypub_avatar_cache_cleanup_lock' ); + if ( ! \add_option( 'activitypub_avatar_cache_cleanup_lock', time(), '', false ) ) { + return; + } + } + + $upload_dir = \wp_upload_dir(); + $root = $upload_dir['basedir'] . static::get_base_dir(); + $dirs = \glob( $root . '/*', GLOB_ONLYDIR ); + + if ( empty( $dirs ) ) { + \delete_option( 'activitypub_avatar_cache_cleanup_lock' ); + \delete_option( 'activitypub_avatar_cache_cursor' ); + return; + } + + // Sort so the resume position stays stable between runs. + \sort( $dirs ); + + /** + * Filters how many actor directories are scanned per cleanup run. + * + * @since unreleased + * + * @param int $limit The maximum number of directories to scan. + */ + $limit = \apply_filters( 'activitypub_cleanup_actor_cache_limit', 100 ); + + // Resume where the previous run stopped so a large backlog drains + // over several runs instead of always revisiting the first batch. + $total = \count( $dirs ); + $start = (int) \get_option( 'activitypub_avatar_cache_cursor', 0 ) % $total; + $batch = \array_slice( $dirs, $start, \max( 1, (int) $limit ) ); + + foreach ( $batch as $dir ) { + $dirname = \basename( $dir ); + + // Only touch directories with a numeric name, to stay clear of junk. + if ( ! \preg_match( '/^\d+$/', $dirname ) ) { + continue; + } + + $post_id = (int) $dirname; + $post = \get_post( $post_id ); + + // Remove directories that no longer belong to an actor post. + if ( ! $post || Remote_Actors::POST_TYPE !== $post->post_type ) { + static::delete_directory( $dir ); + continue; + } + + $hash = self::get_actor_avatar_hash( $post_id ); + if ( $hash ) { + self::prune_stale_files( $post_id, $hash ); + } + } + + \update_option( 'activitypub_avatar_cache_cursor', ( $start + \count( $batch ) ) % $total, false ); + \delete_option( 'activitypub_avatar_cache_cleanup_lock' ); + } } diff --git a/includes/class-scheduler.php b/includes/class-scheduler.php index 143b0732b3..8a3e285111 100644 --- a/includes/class-scheduler.php +++ b/includes/class-scheduler.php @@ -9,6 +9,7 @@ use Activitypub\Activity\Activity; use Activitypub\Activity\Base_Object; +use Activitypub\Cache\Avatar; use Activitypub\Collection\Actors; use Activitypub\Collection\Inbox; use Activitypub\Collection\Outbox; @@ -35,6 +36,7 @@ class Scheduler { const SCHEDULES = array( 'activitypub_update_remote_actors' => 'hourly', 'activitypub_cleanup_remote_actors' => 'daily', + 'activitypub_cleanup_actor_cache' => 'daily', 'activitypub_reprocess_outbox' => 'hourly', 'activitypub_outbox_purge' => 'daily', 'activitypub_inbox_purge' => 'daily', @@ -84,6 +86,9 @@ public static function init() { \add_action( 'activitypub_update_remote_actors', array( self::class, 'update_remote_actors' ) ); \add_action( 'activitypub_cleanup_remote_actors', array( self::class, 'cleanup_remote_actors' ) ); + // Cached avatar cleanup. + \add_action( 'activitypub_cleanup_actor_cache', array( Avatar::class, 'cleanup_actors' ) ); + // Event callbacks. \add_action( 'activitypub_async_batch', array( self::class, 'async_batch' ), 10, 99 ); \add_action( 'activitypub_reprocess_outbox', array( self::class, 'reprocess_outbox' ) ); diff --git a/tests/phpunit/tests/includes/cache/class-test-avatar.php b/tests/phpunit/tests/includes/cache/class-test-avatar.php index 67fc0eb512..a76d0ed867 100644 --- a/tests/phpunit/tests/includes/cache/class-test-avatar.php +++ b/tests/phpunit/tests/includes/cache/class-test-avatar.php @@ -8,6 +8,7 @@ namespace Activitypub\Tests\Cache; use Activitypub\Cache\Avatar; +use Activitypub\Collection\Remote_Actors; use WP_UnitTestCase; /** @@ -132,6 +133,60 @@ public function test_maybe_cache_caches_valid_url() { Avatar::invalidate_entity( $post_id ); } + /** + * Test that caching a new avatar URL drops the previous version. + * + * This is the root cause of #3583: when an actor changes their icon URL, + * the old hash file must not linger next to the new one. + */ + public function test_new_avatar_url_prunes_previous_version() { + $post_id = self::factory()->post->create(); + $old_url = 'https://example.com/old-avatar.jpg'; + $new_url = 'https://example.com/new-avatar.jpg'; + $old_hash = md5( $old_url ); + $new_hash = md5( $new_url ); + $paths = Avatar::get_storage_paths( $post_id ); + $mock_prefix = '/test-avatar-'; + + $mock_download = function ( $result, $download_url ) use ( $old_url, $new_url, $mock_prefix ) { + if ( $download_url === $old_url || $download_url === $new_url ) { + $tmp_file = \wp_tempnam( $mock_prefix . md5( $download_url ) . '.jpg' ); + copy( AP_TESTS_DIR . '/data/assets/test.jpg', $tmp_file ); + + return array( + 'file' => $tmp_file, + 'mime_type' => 'image/jpeg', + ); + } + + return $result; + }; + + \add_filter( 'activitypub_pre_download_url', $mock_download, 10, 2 ); + + // Cache the first avatar. + Avatar::maybe_cache( $old_url, 'avatar', $post_id ); + $this->assertTrue( + \file_exists( $paths['basedir'] . '/' . $old_hash . '.webp' ) || (bool) \glob( $paths['basedir'] . '/' . $old_hash . '.*' ), + 'Old avatar should be cached after the first download' + ); + + // Cache a new avatar; the old one should be gone in the same call. + Avatar::maybe_cache( $new_url, 'avatar', $post_id ); + $this->assertEmpty( + \glob( $paths['basedir'] . '/' . $old_hash . '.*' ), + 'The previous avatar hash should be pruned when the new one is cached' + ); + $this->assertNotEmpty( + \glob( $paths['basedir'] . '/' . $new_hash . '.*' ), + 'The new avatar hash should be cached' + ); + + // Clean up. + \remove_filter( 'activitypub_pre_download_url', $mock_download ); + Avatar::invalidate_entity( $post_id ); + } + /** * Test maybe_cache returns original URL when download fails. */ @@ -228,4 +283,221 @@ public function test_init_registers_action() { has_action( 'before_delete_post', array( Avatar::class, 'maybe_cleanup' ) ) ); } + + /** + * Test that get_actor_avatar_hash resolves the current icon without caching. + */ + public function test_get_actor_avatar_hash() { + $icon_url = 'https://example.com/avatar.jpg'; + $post_id = self::factory()->post->create( + array( + 'post_type' => Remote_Actors::POST_TYPE, + 'post_content' => wp_json_encode( array( 'icon' => array( 'url' => $icon_url ) ) ), + ) + ); + + $fired = false; + $capture = function ( $value ) use ( &$fired ) { + $fired = true; + return $value; + }; + \add_filter( 'activitypub_remote_media_url', $capture ); + + $this->assertEquals( md5( $icon_url ), Avatar::get_actor_avatar_hash( $post_id ) ); + $this->assertFalse( $fired, 'The cache filter should not fire when resolving the hash' ); + + \remove_filter( 'activitypub_remote_media_url', $capture ); + } + + /** + * Test that get_actor_avatar_hash returns false when the actor has no icon. + */ + public function test_get_actor_avatar_hash_no_icon() { + $post_id = self::factory()->post->create( + array( + 'post_type' => Remote_Actors::POST_TYPE, + 'post_content' => wp_json_encode( array( 'name' => 'Test' ) ), + ) + ); + + $this->assertFalse( Avatar::get_actor_avatar_hash( $post_id ) ); + } + + /** + * Test that get_actor_avatar_hash returns false for a missing post. + */ + public function test_get_actor_avatar_hash_missing_post() { + $this->assertFalse( Avatar::get_actor_avatar_hash( 999999 ) ); + } + + /** + * Test that prune_stale_files keeps the current hash and deletes others. + */ + public function test_prune_stale_files_keeps_current() { + $post_id = self::factory()->post->create(); + $current = md5( 'https://example.com/current.jpg' ); + $stale = md5( 'https://example.com/stale.jpg' ); + $paths = Avatar::get_storage_paths( $post_id ); + $current_ext = 'webp'; + $stale_ext = 'jpg'; + wp_mkdir_p( $paths['basedir'] ); + // phpcs:ignore WordPress.WP.AlternativeFunctions.file_system_operations_file_put_contents + file_put_contents( $paths['basedir'] . "/{$current}.{$current_ext}", 'current' ); + // phpcs:ignore WordPress.WP.AlternativeFunctions.file_system_operations_file_put_contents + file_put_contents( $paths['basedir'] . "/{$stale}.{$stale_ext}", 'stale' ); + + Avatar::prune_stale_files( $post_id, $current ); + + $this->assertTrue( file_exists( $paths['basedir'] . "/{$current}.{$current_ext}" ) ); + $this->assertFalse( file_exists( $paths['basedir'] . "/{$stale}.{$stale_ext}" ) ); + + Avatar::invalidate_entity( $post_id ); + } + + /** + * Test that prune_stale_files deletes everything when nothing matches. + */ + public function test_prune_stale_files_no_match() { + $post_id = self::factory()->post->create(); + $hash = md5( 'https://example.com/some.jpg' ); + $paths = Avatar::get_storage_paths( $post_id ); + wp_mkdir_p( $paths['basedir'] ); + // phpcs:ignore WordPress.WP.AlternativeFunctions.file_system_operations_file_put_contents + file_put_contents( $paths['basedir'] . "/{$hash}.jpg", 'x' ); + + Avatar::prune_stale_files( $post_id, md5( 'https://example.com/other.jpg' ) ); + + $this->assertFalse( file_exists( $paths['basedir'] . "/{$hash}.jpg" ) ); + + Avatar::invalidate_entity( $post_id ); + } + + /** + * Test that cleanup_actors removes orphaned actor directories. + */ + public function test_cleanup_actors_removes_orphan_dir() { + $post_id = self::factory()->post->create(); + $paths = Avatar::get_storage_paths( $post_id ); + wp_mkdir_p( $paths['basedir'] ); + // phpcs:ignore WordPress.WP.AlternativeFunctions.file_system_operations_file_put_contents + file_put_contents( $paths['basedir'] . '/abc.jpg', 'x' ); + + Avatar::cleanup_actors(); + + $this->assertFalse( file_exists( $paths['basedir'] ) ); + \delete_option( 'activitypub_avatar_cache_cleanup_lock' ); + \delete_option( 'activitypub_avatar_cache_cursor' ); + } + + /** + * Test that cleanup_actors leaves non-numeric directories alone. + */ + public function test_cleanup_actors_leaves_non_numeric_dir() { + $root = wp_upload_dir()['basedir'] . Avatar::get_base_dir(); + $junk = $root . '/not-a-number'; + wp_mkdir_p( $junk ); + // phpcs:ignore WordPress.WP.AlternativeFunctions.file_system_operations_file_put_contents + file_put_contents( $junk . '/a.jpg', 'x' ); + + Avatar::cleanup_actors(); + + $this->assertTrue( file_exists( $junk . '/a.jpg' ) ); + + wp_delete_file( $junk . '/a.jpg' ); + rmdir( $junk ); // phpcs:ignore WordPress.WP.AlternativeFunctions.file_system_operations_rmdir + \delete_option( 'activitypub_avatar_cache_cleanup_lock' ); + \delete_option( 'activitypub_avatar_cache_cursor' ); + } + + /** + * Test that cleanup_actors prunes stale files for a surviving actor. + */ + public function test_cleanup_actors_prunes_surviving_actor() { + $icon_url = 'https://example.com/current.png'; + $post_id = self::factory()->post->create( + array( + 'post_type' => Remote_Actors::POST_TYPE, + 'post_content' => wp_json_encode( array( 'icon' => array( 'url' => $icon_url ) ) ), + ) + ); + + $current = md5( $icon_url ); + $stale = md5( 'https://example.com/stale.png' ); + $paths = Avatar::get_storage_paths( $post_id ); + wp_mkdir_p( $paths['basedir'] ); + // phpcs:ignore WordPress.WP.AlternativeFunctions.file_system_operations_file_put_contents + file_put_contents( $paths['basedir'] . "/{$current}.png", 'current' ); + // phpcs:ignore WordPress.WP.AlternativeFunctions.file_system_operations_file_put_contents + file_put_contents( $paths['basedir'] . "/{$stale}.png", 'stale' ); + + Avatar::cleanup_actors(); + + $this->assertTrue( file_exists( $paths['basedir'] . "/{$current}.png" ) ); + $this->assertFalse( file_exists( $paths['basedir'] . "/{$stale}.png" ) ); + + Avatar::invalidate_entity( $post_id ); + \delete_option( 'activitypub_avatar_cache_cleanup_lock' ); + \delete_option( 'activitypub_avatar_cache_cursor' ); + } + + /** + * Test that a second cleanup run while locked returns early. + */ + public function test_cleanup_actors_reentrant_lock() { + $post_id = self::factory()->post->create(); + $paths = Avatar::get_storage_paths( $post_id ); + wp_mkdir_p( $paths['basedir'] ); + // phpcs:ignore WordPress.WP.AlternativeFunctions.file_system_operations_file_put_contents + file_put_contents( $paths['basedir'] . '/abc.jpg', 'x' ); + + \add_option( 'activitypub_avatar_cache_cleanup_lock', time(), '', false ); + + Avatar::cleanup_actors(); + + $this->assertTrue( file_exists( $paths['basedir'] . '/abc.jpg' ), 'Orphan dir should not be removed while locked' ); + + Avatar::invalidate_entity( $post_id ); + \delete_option( 'activitypub_avatar_cache_cleanup_lock' ); + \delete_option( 'activitypub_avatar_cache_cursor' ); + } + + /** + * Test that a cleanup run advances past the batch so the backlog drains. + * + * With more actor directories than the per-run limit, the second run + * should pick up where the first left off rather than revisiting the + * same directories. + */ + public function test_cleanup_actors_advances_batch_cursor() { + // Limit the batch to one directory per run. + $limit_one = static function () { + return 1; + }; + \add_filter( 'activitypub_cleanup_actor_cache_limit', $limit_one ); + + $root = wp_upload_dir()['basedir'] . Avatar::get_base_dir(); + + // Two orphan actor directories, sorted so 1 comes before 2. + $first = $root . '/1'; + $second = $root . '/2'; + wp_mkdir_p( $first ); + wp_mkdir_p( $second ); + // phpcs:ignore WordPress.WP.AlternativeFunctions.file_system_operations_file_put_contents + file_put_contents( $first . '/a.jpg', 'x' ); + // phpcs:ignore WordPress.WP.AlternativeFunctions.file_system_operations_file_put_contents + file_put_contents( $second . '/b.jpg', 'x' ); + + // First run should process only the first directory. + Avatar::cleanup_actors(); + $this->assertFalse( file_exists( $first . '/a.jpg' ), 'First run should clean the first batch' ); + $this->assertTrue( file_exists( $second . '/b.jpg' ), 'First run should stop after the batch limit' ); + + // Second run should advance and clean the next directory. + Avatar::cleanup_actors(); + $this->assertFalse( file_exists( $second . '/b.jpg' ), 'Second run should clean the next batch' ); + + \remove_filter( 'activitypub_cleanup_actor_cache_limit', $limit_one ); + \delete_option( 'activitypub_avatar_cache_cleanup_lock' ); + \delete_option( 'activitypub_avatar_cache_cursor' ); + } } diff --git a/tests/phpunit/tests/includes/class-test-scheduler.php b/tests/phpunit/tests/includes/class-test-scheduler.php index 51fb7ed81c..25054dad9e 100644 --- a/tests/phpunit/tests/includes/class-test-scheduler.php +++ b/tests/phpunit/tests/includes/class-test-scheduler.php @@ -9,6 +9,7 @@ use Activitypub\Activity\Activity; use Activitypub\Activity\Base_Object; +use Activitypub\Cache\Avatar; use Activitypub\Collection\Actors; use Activitypub\Collection\Inbox; use Activitypub\Collection\Outbox; @@ -1054,4 +1055,23 @@ public function test_update_remote_actors_skips_identity_change() { $this->assertSame( 'Old Name', \get_post( $id )->post_title, 'A changed remote identity must not be applied to the cached actor.' ); $this->assertEmpty( Remote_Actors::get_outdated(), 'A skipped actor must be touched so it is not re-fetched on the next run.' ); } + + /** + * Test that the avatar cache cleanup schedule is registered. + */ + public function test_cleanup_actor_cache_schedule_registered() { + $this->assertArrayHasKey( 'activitypub_cleanup_actor_cache', Scheduler::SCHEDULES ); + $this->assertEquals( 'daily', Scheduler::SCHEDULES['activitypub_cleanup_actor_cache'] ); + } + + /** + * Test that the avatar cache cleanup action is registered on init. + */ + public function test_cleanup_actor_cache_action_registered() { + Scheduler::init(); + + $this->assertNotFalse( + \has_action( 'activitypub_cleanup_actor_cache', array( Avatar::class, 'cleanup_actors' ) ) + ); + } }