From 838a6b999cc710d3c91f55d84003f6fdc1b38cb6 Mon Sep 17 00:00:00 2001 From: Your Name Date: Mon, 31 Aug 2026 11:45:15 +0930 Subject: [PATCH] Add TODO.md; enhance media:maintenance with --server filter and state annotations - TODO.md: capture follow-ups (centralized status media teardown, DM leak fix, no-DB-cascade rationale, remote-edit orphaning, media:gc re-check). - media:maintenance: add --server remote|local|both (default both) to filter orphaned media by origin. - Annotate each row with status state (live/soft-deleted/hard-deleted) next to status_id and profile state (live/soft-deleted/hard-deleted) next to profile_id, in both dry-run table and verbose run output. States are resolved in batched, trashed-aware queries. --- TODO.md | 53 ++++++++ .../Commands/Media/MediaMaintenance.php | 115 +++++++++++++++--- 2 files changed, 154 insertions(+), 14 deletions(-) create mode 100644 TODO.md diff --git a/TODO.md b/TODO.md new file mode 100644 index 000000000..112a5c6ca --- /dev/null +++ b/TODO.md @@ -0,0 +1,53 @@ +# TODO / Future Improvements + +Running list of follow-up work identified while debugging the media lifecycle. + +## Media / Status lifecycle + +- [ ] **Centralize status media teardown.** The detach-then-dispatch media + cleanup is currently copy-pasted in `StatusDelete`, `RemoteStatusDelete`, and + `DeleteRemoteStatusPipeline`. Extract a single helper (e.g. + `MediaStorageService::deleteStatusMedia(Status $status)`) and call it from + every status-delete path so the logic can't drift out of sync. + +- [ ] **Fix DirectMessageController media leak.** `DirectMessageController` + deletes DM statuses via `forceDeleteQuietly()`, which bypasses model events + AND never dispatches `MediaDeletePipeline`. DM media rows and their files + (local/S3/HLS) leak. Route it through the centralized helper above. + +- [ ] **Do NOT use a DB FK `ON DELETE CASCADE` for media→status.** Reasons: + both models use `SoftDeletes` (soft delete is an UPDATE, cascade only fires on + hard DELETE), and a row-level cascade would orphan the actual stored files + (local disk, S3 objects, HLS `.m3u8`/`.ts` segments) since all file cleanup + lives in `MediaDeletePipeline`, not the DB. Keep teardown in application code. + +- [ ] **Optional defense-in-depth:** a `Status::deleting` observer hook that + calls the centralized teardown, so future delete paths are covered + automatically. Note `forceDeleteQuietly()` callers skip events and must still + call the helper explicitly. + +## StatusRemoteUpdatePipeline (remote edits) + +- [ ] **Don't orphan media on remote edit when re-fetch fails.** + `StatusRemoteUpdatePipeline::updateMedia()` detaches all media + (`status_id = null`) then re-imports; if the new attachment HEAD requests 404 + (origin deleted the media) nothing is re-attached and the rows are left + orphaned. Only detach media that is actually being replaced, or keep existing + media when the incoming set resolves to nothing. + +## media:gc (GarbageCollectorMedia) + +- [ ] **Re-check `status_id` immediately before dispatch.** `media:gc` selects a + batch with `->get()` then dispatches per row; a row can be re-attached between + select and dispatch. Re-read `status_id` right before dispatching. + +- [ ] **Consider a `remote_media` guard / policy.** Federated media lifecycle is + owned by the remote instance; decide whether `media:gc` should treat it + differently from local orphan uploads. + +## Diagnostics / tooling + +- [ ] Roll back the temporary `MEDIA-TRACE:` debug logging + (branch `debug/media-lifecycle-trace-logs`) once diagnosis is complete. +- [ ] Consider additional `media:maintenance --scope` routines (e.g. missing + local files, stale cloud URLs, mime backfill). diff --git a/app/Console/Commands/Media/MediaMaintenance.php b/app/Console/Commands/Media/MediaMaintenance.php index c93e8eb87..f3107351c 100644 --- a/app/Console/Commands/Media/MediaMaintenance.php +++ b/app/Console/Commands/Media/MediaMaintenance.php @@ -3,8 +3,12 @@ namespace App\Console\Commands\Media; use App\Models\Media; +use App\Models\Profile; +use App\Models\Status; use App\Services\MediaStorageService; use Illuminate\Console\Command; +use Illuminate\Database\Eloquent\Model; +use Illuminate\Support\Collection; use Illuminate\Support\Facades\DB; class MediaMaintenance extends Command @@ -16,6 +20,7 @@ class MediaMaintenance extends Command */ protected $signature = 'media:maintenance {--scope= : The maintenance routine to run. Supported: orphanedMedia} + {--server=both : Which media to target by origin: remote, local, or both} {--limit=1000 : Max media rows to process this run} {--dry-run : Report what would happen without detaching or deleting} {--force : Skip confirmation prompts}'; @@ -36,6 +41,13 @@ class MediaMaintenance extends Command 'orphanedMedia' => 'handleOrphanedMedia', ]; + /** + * Supported --server values. + * + * @var array + */ + protected array $servers = ['remote', 'local', 'both']; + public function handle(): int { $scope = $this->option('scope'); @@ -52,6 +64,13 @@ class MediaMaintenance extends Command return self::FAILURE; } + $server = (string) $this->option('server'); + if (! in_array($server, $this->servers, true)) { + $this->error('Invalid --server "'.$server.'". Supported: '.implode(', ', $this->servers).'.'); + + return self::FAILURE; + } + return $this->{$this->scopes[$scope]}(); } @@ -67,10 +86,13 @@ class MediaMaintenance extends Command { $limit = max(1, (int) $this->option('limit')); $dryRun = (bool) $this->option('dry-run'); + $server = (string) $this->option('server'); // Media whose status_id is set but has no matching live (non-trashed) - // status row. + // status row, filtered by origin (remote/local/both). $query = Media::whereNotNull('status_id') + ->when($server === 'remote', fn ($q) => $q->where('remote_media', true)) + ->when($server === 'local', fn ($q) => $q->where('remote_media', false)) ->whereNotExists(function ($q) { $q->select(DB::raw(1)) ->from('statuses') @@ -81,37 +103,46 @@ class MediaMaintenance extends Command $total = (clone $query)->count(); if ($total === 0) { - $this->info('No orphaned media found. Nothing to do.'); + $this->info('No orphaned media found for --server='.$server.'. Nothing to do.'); return self::SUCCESS; } $media = $query->limit($limit)->get(); - $this->info('Found '.$total.' orphaned media row(s); processing '.$media->count().' this run (limit '.$limit.').'); + // Resolve status + profile state for the rows in this batch (batched, + // trashed-aware) so we can annotate live vs soft/hard-deleted. + $stateMap = $this->resolveStates($media); + + $this->info('Found '.$total.' orphaned media row(s) [--server='.$server.']; processing '.$media->count().' this run (limit '.$limit.').'); if ($dryRun) { - $columns = $this->output->isVerbose() - ? ['media_id', 'status_id', 'remote_media', 'profile_id', 'mime', 'size', 'created_at', 'media_path'] - : ['media_id', 'status_id', 'remote_media', 'media_path']; + $verbose = $this->output->isVerbose(); + $columns = $verbose + ? ['media_id', 'status_id', 'status_state', 'profile_id', 'profile_state', 'remote_media', 'mime', 'size', 'created_at', 'media_path'] + : ['media_id', 'status_id', 'status_state', 'profile_id', 'profile_state', 'remote_media', 'media_path']; $this->table( $columns, - $media->map(function ($m) { - $base = [ + $media->map(function ($m) use ($stateMap, $verbose) { + $row = [ 'media_id' => $m->id, 'status_id' => $m->status_id, + 'status_state' => $stateMap['status'][$m->status_id] ?? 'unknown', + 'profile_id' => $m->profile_id ?? 'null', + 'profile_state' => $m->profile_id ? ($stateMap['profile'][$m->profile_id] ?? 'unknown') : 'n/a', 'remote_media' => (bool) $m->remote_media ? 'true' : 'false', - 'profile_id' => $m->profile_id, 'mime' => $m->mime, 'size' => $m->size, 'created_at' => optional($m->created_at)->toDateTimeString(), 'media_path' => $m->media_path, ]; - return $this->output->isVerbose() - ? array_values($base) - : [$base['media_id'], $base['status_id'], $base['remote_media'], $base['media_path']]; + if (! $verbose) { + unset($row['mime'], $row['size'], $row['created_at']); + } + + return array_values($row); })->all() ); $this->comment('[dry-run] Would detach and delete the '.$media->count().' row(s) above.'); @@ -142,13 +173,15 @@ class MediaMaintenance extends Command if ($verbose) { $this->line(sprintf( - ' [%d/%d] detached + dispatched delete: media_id=%d status_id=%s remote_media=%s profile_id=%s mime=%s size=%s path=%s', + ' [%d/%d] detached + dispatched delete: media_id=%d status_id=%s (%s) profile_id=%s (%s) remote_media=%s mime=%s size=%s path=%s', $processed, $media->count(), $m->id, $originalStatusId ?? 'null', - (bool) $m->remote_media ? 'true' : 'false', + $stateMap['status'][$originalStatusId] ?? 'unknown', $m->profile_id ?? 'null', + $m->profile_id ? ($stateMap['profile'][$m->profile_id] ?? 'unknown') : 'n/a', + (bool) $m->remote_media ? 'true' : 'false', $m->mime ?? 'null', $m->size ?? 'null', $m->media_path ?? 'null' @@ -170,4 +203,58 @@ class MediaMaintenance extends Command return self::SUCCESS; } + + /** + * Resolve the lifecycle state of the statuses and profiles referenced by a + * batch of media rows. + * + * A referenced id can be: + * - live row exists and is not soft-deleted + * - soft-deleted row exists with deleted_at set + * - hard-deleted row does not exist at all (only meaningful for statuses) + * + * @param Collection $media + * @return array{status: array, profile: array} + */ + protected function resolveStates($media): array + { + $statusIds = $media->pluck('status_id')->filter()->unique()->values(); + $profileIds = $media->pluck('profile_id')->filter()->unique()->values(); + + return [ + 'status' => $this->stateFor(Status::class, $statusIds), + 'profile' => $this->stateFor(Profile::class, $profileIds), + ]; + } + + /** + * Build an id => state map for a soft-deletable model over the given ids. + * + * @param class-string $model + * @param Collection $ids + * @return array + */ + protected function stateFor(string $model, $ids): array + { + if ($ids->isEmpty()) { + return []; + } + + $rows = $model::withTrashed() + ->whereIn('id', $ids->all()) + ->pluck('deleted_at', 'id'); + + $map = []; + foreach ($ids as $id) { + if (! $rows->has($id)) { + $map[$id] = 'hard-deleted'; + } elseif ($rows->get($id) !== null) { + $map[$id] = 'soft-deleted'; + } else { + $map[$id] = 'live'; + } + } + + return $map; + } }