From 400f00c5e13c0e8b7fdbf22d2a506cb5adbc717a Mon Sep 17 00:00:00 2001 From: Your Name Date: Sun, 30 Aug 2026 23:52:36 +0930 Subject: [PATCH] feat: storage:maintenance command + in-flow cleanup of emptied dirs Replace the remcache GC (GarbageCollectorRemcache / gc:remcache) with a broader storage:maintenance command that sweeps stale remcache temp files and recursively removes the random empty directories accumulated under the media, story, avatar and import trees (--hours/--only/--except/--dry-run), scheduled daily. Fix the root causes so flows clean up after themselves rather than relying on the sweep: - MediaDeletePipeline removes its own emptied m/_v2 leaf dir - AvatarOptimize logs the previously-swallowed exception, still cleans up the old avatar on failure, and removes the old file's now-empty splayed dir - AvatarController::deleteAvatar removes the emptied splayed dir - StoryExpire/StoryDelete remove the story's own emptied leaf dir - TransformImports removes imports/{userId} once its files are moved out - StoryFetch cleans up its remcache temp file in a finally block --- .../Internal/GarbageCollectorRemcache.php | 103 -------- .../Commands/Internal/StorageMaintenance.php | 246 ++++++++++++++++++ .../Commands/Internal/TransformImports.php | 8 + app/Http/Controllers/AvatarController.php | 5 +- app/Jobs/AvatarPipeline/AvatarOptimize.php | 11 + app/Jobs/StoryPipeline/StoryDelete.php | 10 +- app/Jobs/StoryPipeline/StoryExpire.php | 13 +- app/Jobs/StoryPipeline/StoryFetch.php | 14 +- bootstrap/app.php | 2 +- .../Console/StorageMaintenanceTest.php | 111 ++++++++ .../MediaDeleteLeafCleanupTest.php | 82 ++++++ 11 files changed, 488 insertions(+), 117 deletions(-) delete mode 100644 app/Console/Commands/Internal/GarbageCollectorRemcache.php create mode 100644 app/Console/Commands/Internal/StorageMaintenance.php create mode 100644 tests/Feature/Console/StorageMaintenanceTest.php create mode 100644 tests/Feature/MediaPipeline/MediaDeleteLeafCleanupTest.php diff --git a/app/Console/Commands/Internal/GarbageCollectorRemcache.php b/app/Console/Commands/Internal/GarbageCollectorRemcache.php deleted file mode 100644 index 340680699..000000000 --- a/app/Console/Commands/Internal/GarbageCollectorRemcache.php +++ /dev/null @@ -1,103 +0,0 @@ -option('hours'); - - if ($hours < 1) { - $this->error('The --hours value must be at least 1.'); - - return self::FAILURE; - } - - $dryRun = (bool) $this->option('dry-run'); - $dir = storage_path('app/remcache'); - - if (! is_dir($dir)) { - $this->info('remcache directory does not exist, nothing to do.'); - - return self::SUCCESS; - } - - $cutoff = now()->subHours($hours)->getTimestamp(); - $deleted = 0; - $reclaimed = 0; - - foreach (new \FilesystemIterator($dir, \FilesystemIterator::SKIP_DOTS) as $file) { - if (! $file->isFile()) { - continue; - } - - // Preserve dotfiles such as the directory's .gitignore. - if (str_starts_with($file->getFilename(), '.')) { - continue; - } - - if ($file->getMTime() >= $cutoff) { - continue; - } - - $size = $file->getSize(); - $path = $file->getPathname(); - - if ($dryRun) { - $this->line('[dry-run] would delete: '.$file->getFilename()); - $deleted++; - $reclaimed += $size; - - continue; - } - - if (@unlink($path)) { - $deleted++; - $reclaimed += $size; - } - } - - $verb = $dryRun ? 'Would delete' : 'Deleted'; - $this->info(sprintf('%s %d file(s), %s.', $verb, $deleted, $this->humanBytes($reclaimed))); - - return self::SUCCESS; - } - - private function humanBytes(int $bytes): string - { - if ($bytes < 1024) { - return $bytes.' B'; - } - - $units = ['KB', 'MB', 'GB', 'TB']; - $value = $bytes / 1024; - $i = 0; - - while ($value >= 1024 && $i < count($units) - 1) { - $value /= 1024; - $i++; - } - - return sprintf('%.2f %s', $value, $units[$i]); - } -} diff --git a/app/Console/Commands/Internal/StorageMaintenance.php b/app/Console/Commands/Internal/StorageMaintenance.php new file mode 100644 index 000000000..6d3c23f30 --- /dev/null +++ b/app/Console/Commands/Internal/StorageMaintenance.php @@ -0,0 +1,246 @@ + + */ + protected array $emptyDirRoots = [ + 'public/m/_v2', + 'public/_esm.t3', + 'public/avatars', + 'story_archives', + 'imports', + ]; + + protected bool $dryRun = false; + + public function handle(): int + { + $this->dryRun = (bool) $this->option('dry-run'); + + $tasks = $this->resolveTasks(); + + if (empty($tasks)) { + $this->error('No tasks to run. Valid tasks: remcache, empty-dirs.'); + + return self::FAILURE; + } + + if (in_array('remcache', $tasks, true)) { + $result = $this->sweepRemcache(); + if ($result !== self::SUCCESS) { + return $result; + } + } + + if (in_array('empty-dirs', $tasks, true)) { + $this->pruneEmptyDirectories(); + } + + return self::SUCCESS; + } + + /** + * Determine which tasks to run from --only / --except. + * + * @return list + */ + protected function resolveTasks(): array + { + $all = ['remcache', 'empty-dirs']; + + $only = $this->parseList($this->option('only')); + $except = $this->parseList($this->option('except')); + + $tasks = $only ? array_values(array_intersect($all, $only)) : $all; + + return array_values(array_diff($tasks, $except)); + } + + /** + * @return list + */ + protected function parseList(?string $value): array + { + if (! $value) { + return []; + } + + return collect(explode(',', $value)) + ->map(fn ($v) => trim($v)) + ->filter() + ->unique() + ->values() + ->all(); + } + + /** + * Delete stale temporary files left in storage/app/remcache/. The media + * fetchers now clean up their own temp files (try/finally in + * MediaStorageService); this is the backstop for any writer that misses it. + */ + protected function sweepRemcache(): int + { + $hours = (int) $this->option('hours'); + + if ($hours < 1) { + $this->error('The --hours value must be at least 1.'); + + return self::FAILURE; + } + + $dir = storage_path('app/remcache'); + + if (! is_dir($dir)) { + $this->info('remcache: directory does not exist, nothing to do.'); + + return self::SUCCESS; + } + + $cutoff = now()->subHours($hours)->getTimestamp(); + $deleted = 0; + $reclaimed = 0; + + foreach (new \FilesystemIterator($dir, \FilesystemIterator::SKIP_DOTS) as $file) { + if (! $file->isFile()) { + continue; + } + + // Preserve dotfiles such as the directory's .gitignore. + if (str_starts_with($file->getFilename(), '.')) { + continue; + } + + if ($file->getMTime() >= $cutoff) { + continue; + } + + $size = $file->getSize(); + $path = $file->getPathname(); + + if ($this->dryRun) { + $this->line('[dry-run] remcache: would delete '.$file->getFilename()); + $deleted++; + $reclaimed += $size; + + continue; + } + + if (@unlink($path)) { + $deleted++; + $reclaimed += $size; + } + } + + $verb = $this->dryRun ? 'would delete' : 'deleted'; + $this->info(sprintf('remcache: %s %d file(s), %s.', $verb, $deleted, $this->humanBytes($reclaimed))); + + return self::SUCCESS; + } + + /** + * Sweep each managed local tree and remove the random empty directories + * accumulated over time. This is the safety net: the per-flow jobs clean + * up their own directories, but older data (and any missed edge case) can + * still leave empty folders behind, so we reclaim them here. + */ + protected function pruneEmptyDirectories(): void + { + $disk = Storage::disk('local'); + $total = 0; + + foreach ($this->emptyDirRoots as $root) { + if (! $disk->directoryExists($root)) { + continue; + } + + $removed = $this->pruneEmptyDirectoriesUnder($disk, $root); + + $verb = $this->dryRun ? 'would remove' : 'removed'; + $this->info(sprintf('empty-dirs: %s %s %d empty dir(s).', $root, $verb, $removed)); + $total += $removed; + } + + $verb = $this->dryRun ? 'would remove' : 'removed'; + $this->info(sprintf('empty-dirs: %s %d empty dir(s) in total.', $verb, $total)); + } + + /** + * Recursively remove every empty directory under $root (bottom-up), leaving + * $root itself in place. A directory is treated as empty when it holds no + * files at any depth, so a branch of only-empty subdirectories collapses. + */ + protected function pruneEmptyDirectoriesUnder(Filesystem $disk, string $root): int + { + $directories = $disk->allDirectories($root); + + // Deepest first so children are evaluated before their parents. + usort($directories, fn ($a, $b) => substr_count($b, '/') <=> substr_count($a, '/')); + + $removed = 0; + + foreach ($directories as $directory) { + if (count($disk->allFiles($directory)) !== 0) { + continue; + } + + if ($this->dryRun) { + $this->line('[dry-run] empty-dirs: would remove '.$directory); + $removed++; + + continue; + } + + $disk->deleteDirectory($directory); + $removed++; + } + + return $removed; + } + + private function humanBytes(int $bytes): string + { + if ($bytes < 1024) { + return $bytes.' B'; + } + + $units = ['KB', 'MB', 'GB', 'TB']; + $value = $bytes / 1024; + $i = 0; + + while ($value >= 1024 && $i < count($units) - 1) { + $value /= 1024; + $i++; + } + + return sprintf('%.2f %s', $value, $units[$i]); + } +} diff --git a/app/Console/Commands/Internal/TransformImports.php b/app/Console/Commands/Internal/TransformImports.php index 93d2bb6a0..8fc2e92fe 100644 --- a/app/Console/Commands/Internal/TransformImports.php +++ b/app/Console/Commands/Internal/TransformImports.php @@ -238,6 +238,14 @@ class TransformImports extends Command continue; } + + // Files for this import post were moved OUT of imports/{userId}. + // Remove that directory once it holds no more files so completed + // imports don't leave an empty folder behind. + $importDir = 'imports/'.$id; + if ($disk->exists($importDir) && empty($disk->files($importDir))) { + $disk->deleteDirectory($importDir); + } } } } diff --git a/app/Http/Controllers/AvatarController.php b/app/Http/Controllers/AvatarController.php index cdbe966b0..d37ef4dab 100644 --- a/app/Http/Controllers/AvatarController.php +++ b/app/Http/Controllers/AvatarController.php @@ -126,7 +126,10 @@ class AvatarController extends Controller return response()->json(200); } - if (is_file(storage_path('app/'.$avatar->media_path))) { + $oldPath = $avatar->media_path; + $oldFullPath = storage_path('app/'.$oldPath); + + if (is_file($oldFullPath)) { @unlink(storage_path('app/'.$avatar->media_path)); } diff --git a/app/Jobs/AvatarPipeline/AvatarOptimize.php b/app/Jobs/AvatarPipeline/AvatarOptimize.php index 101cde85a..0fd8547bf 100644 --- a/app/Jobs/AvatarPipeline/AvatarOptimize.php +++ b/app/Jobs/AvatarPipeline/AvatarOptimize.php @@ -12,6 +12,7 @@ use Illuminate\Foundation\Bus\Dispatchable; use Illuminate\Queue\InteractsWithQueue; use Illuminate\Queue\SerializesModels; use Illuminate\Support\Facades\Cache; +use Illuminate\Support\Facades\Log; use Illuminate\Support\Facades\Storage; use Illuminate\Support\Str; use Intervention\Image\Encoders\AvifEncoder; @@ -105,11 +106,21 @@ class AvatarOptimize implements ShouldQueue $avatar->save(); } } catch (\Exception $e) { + Log::error('AvatarOptimize failed for profile '.$this->profile->id.': '.$e->getMessage()); + + // The encode/upload may have failed before the old avatar file was + // removed. $this->current is the previous avatar's absolute path; + // clean it (and its now-stale directory) up so failures don't leak. + $this->deleteOldAvatar('', $this->current); } } protected function deleteOldAvatar($new, $current) { + if (! $current) { + return; + } + if (storage_path('app/'.$new) == $current || Str::endsWith($current, 'avatars/default.png') || Str::endsWith($current, 'avatars/default.jpg')) { diff --git a/app/Jobs/StoryPipeline/StoryDelete.php b/app/Jobs/StoryPipeline/StoryDelete.php index 45ca5b7b6..47490666e 100644 --- a/app/Jobs/StoryPipeline/StoryDelete.php +++ b/app/Jobs/StoryPipeline/StoryDelete.php @@ -53,8 +53,16 @@ class StoryDelete implements ShouldQueue StoryService::delLatest($story->profile_id); StoryService::delById($story->id); - if (Storage::exists($story->path) == true) { + if ($story->path && Storage::exists($story->path) == true) { Storage::delete($story->path); + + // Remove the now-empty leaf dir this story's media lived in (either + // the live public/_esm.t3 tree or the story_archives tree once the + // story has been rotated on expiry). + $dir = implode('/', array_slice(explode('/', $story->path), 0, -1)); + if ($dir !== '' && empty(Storage::files($dir))) { + Storage::deleteDirectory($dir); + } } $story->views()->delete(); diff --git a/app/Jobs/StoryPipeline/StoryExpire.php b/app/Jobs/StoryPipeline/StoryExpire.php index 6c477f047..e6551372e 100644 --- a/app/Jobs/StoryPipeline/StoryExpire.php +++ b/app/Jobs/StoryPipeline/StoryExpire.php @@ -92,8 +92,9 @@ class StoryExpire implements ShouldQueue $story->path = $newPath; $story->save(); - $remainingFiles = Storage::files($dir); - if (empty($remainingFiles)) { + // Remove this story's own now-empty leaf dir the media was moved + // out of (public/_esm.t3/{monthHash}/{userHash}/{random}). + if (empty(Storage::files($dir))) { Storage::deleteDirectory($dir); } } @@ -130,8 +131,14 @@ class StoryExpire implements ShouldQueue $path = $story->path; - if (Storage::exists($path) == true) { + if ($path && Storage::exists($path) == true) { Storage::delete($path); + + // Remove the now-empty leaf dir this story's media lived in. + $dir = implode('/', array_slice(explode('/', $path), 0, -1)); + if ($dir !== '' && empty(Storage::files($dir))) { + Storage::deleteDirectory($dir); + } } $story->views()->delete(); diff --git a/app/Jobs/StoryPipeline/StoryFetch.php b/app/Jobs/StoryPipeline/StoryFetch.php index ad9290580..ffc00fbbf 100644 --- a/app/Jobs/StoryPipeline/StoryFetch.php +++ b/app/Jobs/StoryPipeline/StoryFetch.php @@ -494,8 +494,6 @@ class StoryFetch implements ShouldQueue } if (! $this->validateDownloadedFile($tmpName, $payload['attachment']['mediaType'])) { - unlink($tmpName); - return null; } @@ -503,8 +501,6 @@ class StoryFetch implements ShouldQueue $path = $disk->putFileAs($storagePath, new File($tmpName), $fileName, 'public'); $size = filesize($tmpName); - unlink($tmpName); - if (! $path) { if (config('app.dev_log')) { Log::error('Failed to store file permanently'); @@ -520,10 +516,6 @@ class StoryFetch implements ShouldQueue ]; } catch (Exception $e) { - if (file_exists($tmpName)) { - unlink($tmpName); - } - if (config('app.dev_log')) { Log::error('Media download failed', [ 'url' => $mediaUrl, @@ -532,6 +524,12 @@ class StoryFetch implements ShouldQueue } return null; + } finally { + // Always remove the remcache temp file, even on a non-Exception + // throwable or an early return, so downloads never leak temp files. + if (is_file($tmpName)) { + @unlink($tmpName); + } } } diff --git a/bootstrap/app.php b/bootstrap/app.php index dfeca931c..fecee7bbd 100644 --- a/bootstrap/app.php +++ b/bootstrap/app.php @@ -138,7 +138,7 @@ return Application::configure(basePath: dirname(__DIR__)) $schedule->command('gc:failedjobs')->dailyAt(3)->onOneServer(); $schedule->command('gc:passwordreset')->dailyAt('09:41')->onOneServer(); $schedule->command('gc:sessions')->twiceDaily(13, 23)->onOneServer(); - $schedule->command('gc:remcache')->dailyAt('04:15')->onOneServer(); + $schedule->command('storage:maintenance')->dailyAt('04:15')->onOneServer(); $schedule->command('app:weekly-instance-scan')->weeklyOn(2, '4:20')->onOneServer(); $schedule->command('app:cleanup-expired-app-registrations')->dailyAt(1)->onOneServer(); $schedule->command('passport:purge')->everyFourHours(20)->onOneServer(); diff --git a/tests/Feature/Console/StorageMaintenanceTest.php b/tests/Feature/Console/StorageMaintenanceTest.php new file mode 100644 index 000000000..a2b11a053 --- /dev/null +++ b/tests/Feature/Console/StorageMaintenanceTest.php @@ -0,0 +1,111 @@ +remcacheDir = storage_path('app/remcache'); + if (! is_dir($this->remcacheDir)) { + mkdir($this->remcacheDir, 0755, true); + } +}); + +afterEach(function () { + // Clean up any temp files this test created in the real remcache dir. + foreach (glob($this->remcacheDir.'/{,.}pf-maint-test-*', GLOB_BRACE) ?: [] as $f) { + @unlink($f); + } +}); + +it('deletes stale remcache files older than the cutoff and preserves fresh ones and dotfiles', function () { + $stale = $this->remcacheDir.'/pf-maint-test-stale.tmp'; + $fresh = $this->remcacheDir.'/pf-maint-test-fresh.tmp'; + // A stale dotfile (like the directory's .gitignore) must be preserved. + $dot = $this->remcacheDir.'/.pf-maint-test-keep'; + + file_put_contents($stale, 'old'); + file_put_contents($fresh, 'new'); + file_put_contents($dot, 'keep'); + touch($stale, now()->subHours(48)->getTimestamp()); + touch($dot, now()->subHours(48)->getTimestamp()); + + $this->artisan('storage:maintenance', ['--only' => 'remcache', '--hours' => 24]) + ->assertExitCode(0); + + expect(is_file($stale))->toBeFalse(); + expect(is_file($fresh))->toBeTrue(); + // Dotfiles (e.g. .gitignore) are always preserved. + expect(is_file($dot))->toBeTrue(); +}); + +it('dry-run reports remcache deletions without removing files', function () { + $stale = $this->remcacheDir.'/pf-maint-test-stale.tmp'; + file_put_contents($stale, 'old'); + touch($stale, now()->subHours(48)->getTimestamp()); + + $this->artisan('storage:maintenance', ['--only' => 'remcache', '--hours' => 24, '--dry-run' => true]) + ->assertExitCode(0); + + expect(is_file($stale))->toBeTrue(); +}); + +it('prunes empty directories across managed trees while keeping live files', function () { + $disk = Storage::disk('local'); + + // Empty leftovers. + $disk->makeDirectory('public/m/_v2/9/month/rand'); + $disk->makeDirectory('public/_esm.t3/m/u/leaf'); + $disk->makeDirectory('public/avatars/000/111/222'); + $disk->makeDirectory('imports/42'); + $disk->makeDirectory('story_archives/7/202601'); + + // Live files that must survive. + $disk->put('public/m/_v2/9/month/keep/live.jpg', 'x'); + $disk->put('imports/99/live.jpg', 'y'); + + $this->artisan('storage:maintenance', ['--only' => 'empty-dirs']) + ->assertExitCode(0); + + expect($disk->directoryExists('public/m/_v2/9/month/rand'))->toBeFalse(); + expect($disk->directoryExists('public/_esm.t3/m'))->toBeFalse(); + expect($disk->directoryExists('public/avatars/000'))->toBeFalse(); + expect($disk->directoryExists('imports/42'))->toBeFalse(); + expect($disk->directoryExists('story_archives/7'))->toBeFalse(); + + // Roots and live branches preserved. + expect($disk->exists('public/m/_v2/9/month/keep/live.jpg'))->toBeTrue(); + expect($disk->exists('imports/99/live.jpg'))->toBeTrue(); +}); + +it('dry-run reports empty-dir removals without deleting them', function () { + $disk = Storage::disk('local'); + $disk->makeDirectory('public/m/_v2/9/month/rand'); + + $this->artisan('storage:maintenance', ['--only' => 'empty-dirs', '--dry-run' => true]) + ->assertExitCode(0); + + expect($disk->directoryExists('public/m/_v2/9/month/rand'))->toBeTrue(); +}); + +it('rejects an --hours value below 1', function () { + $this->artisan('storage:maintenance', ['--only' => 'remcache', '--hours' => 0]) + ->assertExitCode(1); +}); + +it('errors when only/except leave no tasks to run', function () { + $this->artisan('storage:maintenance', ['--except' => 'remcache,empty-dirs']) + ->assertExitCode(1); +}); diff --git a/tests/Feature/MediaPipeline/MediaDeleteLeafCleanupTest.php b/tests/Feature/MediaPipeline/MediaDeleteLeafCleanupTest.php new file mode 100644 index 000000000..90d224d9a --- /dev/null +++ b/tests/Feature/MediaPipeline/MediaDeleteLeafCleanupTest.php @@ -0,0 +1,82 @@ +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(); +});