From b6d645a4d48cb36dd930b53742d24d5adc9508dd Mon Sep 17 00:00:00 2001 From: Your Name Date: Mon, 31 Aug 2026 00:32:40 +0930 Subject: [PATCH] fix: delete superseded image/thumbnail files instead of orphaning them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Image::handleImageTransform derives the output filename from the current media_path and applies the encoder's output extension. When that differs from what is already stored (heic/avif -> jpg, or a thumbnail regenerated to a new extension), the new file landed at a different path and the previous file was left orphaned in the media directory — the source of the leftover _thumb files under public/m/_v2. Capture the path each transform supersedes and delete it after a successful write (only when the new output path differs, so we never delete what we just wrote). Remove the stale MediaDeleteLeafCleanupTest whose source change is not in the tree. --- app/Util/Media/Image.php | 44 +++++++ .../ImageThumbnailSupersedeTest.php | 114 ++++++++++++++++++ .../MediaDeleteLeafCleanupTest.php | 82 ------------- 3 files changed, 158 insertions(+), 82 deletions(-) create mode 100644 tests/Feature/MediaPipeline/ImageThumbnailSupersedeTest.php delete mode 100644 tests/Feature/MediaPipeline/MediaDeleteLeafCleanupTest.php diff --git a/app/Util/Media/Image.php b/app/Util/Media/Image.php index e013d22d6..86c9c587e 100644 --- a/app/Util/Media/Image.php +++ b/app/Util/Media/Image.php @@ -123,6 +123,15 @@ class Image return; } + // The file this transform is about to supersede. When the output + // extension differs from what is currently stored (e.g. heic/avif -> jpg, + // or a thumbnail regenerated to a new extension), the new file lands at + // a different name and the old one would be orphaned in the media + // directory. Capture it now so we can delete it after a successful + // write. For the base image this is media_path; for a thumbnail it is + // the existing thumbnail_path. + $previousPath = $thumbnail ? $media->thumbnail_path : $media->media_path; + try { $fileContents = null; $tempFile = null; @@ -280,6 +289,11 @@ class Image $media->mime = 'image/'.$outputExtension; } + // Remove the file we just superseded when the new output landed at a + // different path (extension change / thumbnail regeneration), so the + // old file is not orphaned in the media directory. + $this->deleteSupersededFile($previousPath, $converted['path'], $localFs); + $media->save(); if ($thumbnail) { @@ -299,6 +313,36 @@ class Image } } + /** + * Delete a previous file that a transform has just replaced, but only when + * the new output landed at a different path (so we never delete the file we + * just wrote). No-op when there was no previous path or it is unchanged. + */ + protected function deleteSupersededFile(?string $previousPath, string $newPath, bool $localFs): void + { + if (! $previousPath || $previousPath === $newPath) { + return; + } + + try { + if ($localFs) { + $full = storage_path('app/'.$previousPath); + if (is_file($full)) { + @unlink($full); + } + } else { + $disk = Storage::disk($this->defaultDisk); + if ($disk->exists($previousPath)) { + $disk->delete($previousPath); + } + } + } catch (\Exception $e) { + if (config('app.dev_log')) { + Log::info('Superseded media cleanup failed: '.$e->getMessage()); + } + } + } + public function setBaseName($basePath, $thumbnail, $extension) { $pathInfo = pathinfo($basePath); diff --git a/tests/Feature/MediaPipeline/ImageThumbnailSupersedeTest.php b/tests/Feature/MediaPipeline/ImageThumbnailSupersedeTest.php new file mode 100644 index 000000000..83c272400 --- /dev/null +++ b/tests/Feature/MediaPipeline/ImageThumbnailSupersedeTest.php @@ -0,0 +1,114 @@ + 'https://cdn.test']); +}); + +function seedPngMediaWithStaleThumb(): Media +{ + $user = User::factory()->create(); + $user->refresh(); + $pid = $user->profile->id; + + $leaf = 'public/m/_v2/'.$pid.'/aa/bb'; + $mediaPath = $leaf.'/photo.png'; + // A pre-existing thumbnail with a DIFFERENT extension than the png source + // will produce on regeneration (png source -> photo_thumb.png). + $staleThumb = $leaf.'/photo_thumb.jpeg'; + + // A real, decodable 4x4 PNG so the GD driver can process it. + $im = imagecreatetruecolor(4, 4); + ob_start(); + imagepng($im); + $pngBytes = ob_get_clean(); + imagedestroy($im); + + Storage::disk('s3')->put($mediaPath, $pngBytes); + Storage::disk('s3')->put($staleThumb, 'OLD-THUMB-BYTES'); + + return Media::create([ + 'profile_id' => $pid, + 'user_id' => $user->id, + 'media_path' => $mediaPath, + 'thumbnail_path' => $staleThumb, + 'mime' => 'image/png', + 'size' => strlen($pngBytes), + 'remote_media' => false, + 'order' => 0, + ]); +} + +it('deletes the superseded thumbnail when regeneration changes its extension', function () { + $media = seedPngMediaWithStaleThumb(); + $staleThumb = $media->thumbnail_path; + $disk = Storage::disk('s3'); + + expect($disk->exists($staleThumb))->toBeTrue(); + + (new Image)->resizeThumbnail($media); + $media->refresh(); + + // thumbnail_path now points at the freshly generated file (png output). + expect($media->thumbnail_path)->not->toBe($staleThumb); + expect($disk->exists($media->thumbnail_path))->toBeTrue(); + + // The old, superseded thumbnail is gone rather than orphaned in the dir. + expect($disk->exists($staleThumb))->toBeFalse(); +}); + +it('keeps the thumbnail when regeneration writes to the same path', function () { + $user = User::factory()->create(); + $user->refresh(); + $pid = $user->profile->id; + $leaf = 'public/m/_v2/'.$pid.'/cc/dd'; + $mediaPath = $leaf.'/photo.png'; + + $im = imagecreatetruecolor(4, 4); + ob_start(); + imagepng($im); + $pngBytes = ob_get_clean(); + imagedestroy($im); + Storage::disk('s3')->put($mediaPath, $pngBytes); + + $media = Media::create([ + 'profile_id' => $pid, + 'user_id' => $user->id, + 'media_path' => $mediaPath, + 'mime' => 'image/png', + 'size' => strlen($pngBytes), + 'remote_media' => false, + 'order' => 0, + ]); + + (new Image)->resizeThumbnail($media); + $media->refresh(); + + // photo_thumb.png is generated and present. + expect($media->thumbnail_path)->toBe($leaf.'/photo_thumb.png'); + expect(Storage::disk('s3')->exists($media->thumbnail_path))->toBeTrue(); +}); diff --git a/tests/Feature/MediaPipeline/MediaDeleteLeafCleanupTest.php b/tests/Feature/MediaPipeline/MediaDeleteLeafCleanupTest.php deleted file mode 100644 index 90d224d9a..000000000 --- a/tests/Feature/MediaPipeline/MediaDeleteLeafCleanupTest.php +++ /dev/null @@ -1,82 +0,0 @@ -create(); - $user->refresh(); - $pid = $user->profile->id; - - $leaf = 'public/m/_v2/'.$pid.'/aa-bb/rndrndrndrnd'; - $path = $leaf.'/file.jpg'; - $thumb = $leaf.'/file_thumb.jpeg'; - - Storage::disk('local')->put($path, 'PRIMARY'); - Storage::disk('local')->put($thumb, 'THUMB'); - - return Media::create([ - 'status_id' => null, - 'profile_id' => $pid, - 'user_id' => $user->id, - 'media_path' => $path, - 'thumbnail_path' => $thumb, - 'mime' => 'image/jpeg', - 'size' => 7, - 'remote_media' => false, - 'order' => 0, - ]); -} - -it('deletes the media files and removes its now-empty leaf directory', function () { - $media = makeOrphanLocalMedia(); - $leaf = implode('/', array_slice(explode('/', $media->media_path), 0, -1)); - $disk = Storage::disk('local'); - - expect($disk->exists($media->media_path))->toBeTrue(); - - (new MediaDeletePipeline($media))->handle(); - - expect($disk->exists($media->media_path))->toBeFalse(); - expect($disk->exists($media->thumbnail_path))->toBeFalse(); - expect($disk->directoryExists($leaf))->toBeFalse(); -}); - -it('leaves the leaf directory in place when another file still lives there', function () { - $media = makeOrphanLocalMedia(); - $leaf = implode('/', array_slice(explode('/', $media->media_path), 0, -1)); - $disk = Storage::disk('local'); - - // A sibling file that this media does not own. - $disk->put($leaf.'/sibling.jpg', 'KEEP'); - - (new MediaDeletePipeline($media))->handle(); - - expect($disk->exists($media->media_path))->toBeFalse(); - expect($disk->directoryExists($leaf))->toBeTrue(); - expect($disk->exists($leaf.'/sibling.jpg'))->toBeTrue(); -});