From 7f136f6f4cb6dd825fbc86f655005eca0aefa223 Mon Sep 17 00:00:00 2001 From: Jan-Niclas Loosen Date: Sat, 26 Sep 2026 02:54:37 +0200 Subject: [PATCH] fix: media usage check fails closed on database errors; refused deletes checked for partial state MediaUsageFinder treats a failed query (null result or last_error) as a possible usage and an incomplete scan, so media-delete refuses without force. A refused wp_delete_attachment() is write_failed only when the attachment is intact, otherwise partial_write. media-update: re-read hint on write_failed, normalized ids in failures, documented lock resolution and kses on restore. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01At3V6SjwbMe6eMzVGW3Hs7 --- src/Abilities/Errors.php | 7 ++- src/Abilities/WriteFailed.php | 8 ++- src/Integrations/Media/MediaDeleteAbility.php | 38 +++++++++++- src/Integrations/Media/MediaMetaWriter.php | 4 ++ src/Integrations/Media/MediaUpdateAbility.php | 20 +++++-- src/Integrations/Media/MediaUsageFinder.php | 54 +++++++++++++++-- tests/Unit/Abilities/WriteFailedTest.php | 9 +++ .../Media/MediaDeleteAbilityTest.php | 52 +++++++++++++++- .../Media/MediaUpdateAbilityTest.php | 7 ++- .../Media/MediaUsageFinderTest.php | 59 +++++++++++++++++++ 10 files changed, 238 insertions(+), 20 deletions(-) diff --git a/src/Abilities/Errors.php b/src/Abilities/Errors.php index 74d5724..52576bf 100644 --- a/src/Abilities/Errors.php +++ b/src/Abilities/Errors.php @@ -102,14 +102,15 @@ final class Errors * A write failed and was rolled back completely. * * @param array $details Extra error details (e.g. a backup that stays). + * @param string|null $hint Replaces the default hint (e.g. "re-read before retrying"). */ - public static function writeFailed(string $reason, array $details = []): WP_Error + public static function writeFailed(string $reason, array $details = [], ?string $hint = null): WP_Error { return self::make( 'write_failed', trim('Speichern fehlgeschlagen: ' . $reason) . ' Nichts wurde gespeichert.', - 'Eingaben prüfen, ggf. korrigieren und erneut versuchen; tritt der Fehler wieder auf, den Administrator ' - . 'informieren (Details im PHP-Error-Log der Website).', + $hint ?? 'Eingaben prüfen, ggf. korrigieren und erneut versuchen; tritt der Fehler wieder auf, den ' + . 'Administrator informieren (Details im PHP-Error-Log der Website).', 500, $details ); diff --git a/src/Abilities/WriteFailed.php b/src/Abilities/WriteFailed.php index a557dc5..d4ef312 100644 --- a/src/Abilities/WriteFailed.php +++ b/src/Abilities/WriteFailed.php @@ -14,7 +14,8 @@ use WP_Error; * client-safe German text. The AbilityRegistrar logs the detail and returns toError(): * lit_write_failed when everything was rolled back, lit_partial_write otherwise. * $state/$hint customise the partial-write error for writes that are not about a post (see - * Errors::partialWrite()); $details are added to either error. + * Errors::partialWrite()); $details are added to either error. $rolledBackHint replaces the + * default hint of the write_failed error. */ final class WriteFailed extends RuntimeException { @@ -28,7 +29,8 @@ final class WriteFailed extends RuntimeException string $detail = '', public readonly ?string $state = null, public readonly ?string $hint = null, - public readonly array $details = [] + public readonly array $details = [], + public readonly ?string $rolledBackHint = null ) { $internal = $detail !== '' ? $detail : ($previous !== null ? $previous->getMessage() : $reason); parent::__construct( @@ -41,7 +43,7 @@ final class WriteFailed extends RuntimeException public function toError(): WP_Error { return $this->rolledBack - ? Errors::writeFailed($this->reason, $this->details) + ? Errors::writeFailed($this->reason, $this->details, $this->rolledBackHint) : Errors::partialWrite( $this->reason, $this->postId, diff --git a/src/Integrations/Media/MediaDeleteAbility.php b/src/Integrations/Media/MediaDeleteAbility.php index 3b86c5a..217481f 100644 --- a/src/Integrations/Media/MediaDeleteAbility.php +++ b/src/Integrations/Media/MediaDeleteAbility.php @@ -10,6 +10,7 @@ use LIT\MCP\Abilities\FollowUp; use LIT\MCP\Abilities\ToolResult; use LIT\MCP\ChangeLog\ChangeLog; use WP_Error; +use WP_Post; /** * lit/media-delete: deletes one attachment after confirmation. WordPress has no media trash by @@ -123,8 +124,9 @@ final class MediaDeleteAbility implements Ability ); } + $file = (string) get_post_meta($id, '_wp_attached_file', true); if (! wp_delete_attachment($id, false)) { - return Errors::writeFailed(sprintf('WordPress hat das Löschen von Medium %d abgelehnt.', $id)); + return $this->refused($id, $title, $file); } $warnings = []; @@ -145,6 +147,40 @@ final class MediaDeleteAbility implements Ability ->toArray(); } + /** + * wp_delete_attachment() returned false/null. It deletes meta, terms and the row before the + * files, so a failure can leave half a deletion: only an attachment that is still there with + * its file reference intact counts as "nothing deleted". + */ + private function refused(int $id, string $title, string $file): WP_Error + { + $reason = sprintf('WordPress hat das Löschen von Medium %d abgelehnt.', $id); + $current = get_post($id); + $intact = $current instanceof WP_Post + && $current->post_type === 'attachment' + && (string) get_post_meta($id, '_wp_attached_file', true) === $file; + if ($intact) { + return Errors::writeFailed($reason); + } + + $this->log->recordPartial( + $this->name(), + 'attachment', + (string) $id, + sprintf('Medium "%s": Löschen nur teilweise ausgeführt', $title) + ); + + return Errors::partialWrite( + $reason, + null, + null, + sprintf('Medium %d ist möglicherweise teilweise gelöscht (Daten oder Dateien fehlen).', $id), + 'Den Aufruf nicht einfach wiederholen. Das Medium mit lit/media-get bzw. in der WordPress-Mediathek ' + . 'prüfen und dem Nutzer Bescheid geben.', + ['attachment_id' => $id] + ); + } + /** @param list> $usages */ private function question(int $id, string $title, bool $trash, array $usages): string { diff --git a/src/Integrations/Media/MediaMetaWriter.php b/src/Integrations/Media/MediaMetaWriter.php index 0077bcc..6eaf608 100644 --- a/src/Integrations/Media/MediaMetaWriter.php +++ b/src/Integrations/Media/MediaMetaWriter.php @@ -12,6 +12,10 @@ use WP_Post; * Writes the editable metadata of an attachment (alt text, title, caption, description) and * restores it from a snapshot taken before (lit/media-update rolls a batch back with it). * Every failure is a RuntimeException with an internal message for the PHP error log. + * + * Both apply() and restore() save through wp_update_post(), which runs the kses filters again + * for users without unfiltered_html: a caption or description with markup the current user may + * not save comes back filtered on restore (like any save of that user in the media library). */ final class MediaMetaWriter { diff --git a/src/Integrations/Media/MediaUpdateAbility.php b/src/Integrations/Media/MediaUpdateAbility.php index 6283370..896a57a 100644 --- a/src/Integrations/Media/MediaUpdateAbility.php +++ b/src/Integrations/Media/MediaUpdateAbility.php @@ -10,6 +10,7 @@ use LIT\MCP\Abilities\FollowUp; use LIT\MCP\Abilities\ToolResult; use LIT\MCP\Abilities\WriteFailed; use LIT\MCP\ChangeLog\ChangeLog; +use LIT\MCP\Integrations\WordPress\PostGuard; use Throwable; use WP_Error; use WP_Post; @@ -21,7 +22,8 @@ use WP_Post; * changed are restored (write_failed) – or, if that fails too, partial_write names them. * * The `modified` lock compares post_modified_gmt, which WordPress bumps on every write of the - * posts row (title, caption, description) but not on a pure alt-text (post meta) change. + * posts row (title, caption, description) but not on a pure alt-text (post meta) change. Its + * resolution is one second: a parallel write within the same second as the read goes unnoticed. */ final class MediaUpdateAbility implements Ability { @@ -274,7 +276,17 @@ final class MediaUpdateAbility implements Ability $detail = 'Media update failed: ' . $cause->getMessage(); if ($unrestored === []) { - throw new WriteFailed(self::REASON, true, null, null, $cause, $detail); + throw new WriteFailed( + self::REASON, + true, + null, + null, + $cause, + $detail, + rolledBackHint: 'Das Zurücksetzen hat modified der Medien geändert: vor einem neuen Versuch die Medien ' + . 'neu lesen (lit/media-get oder lit-media-search) und den Stapel mit dem aktuellen modified ' + . 'senden; tritt der Fehler wieder auf, den Administrator informieren (Details im PHP-Error-Log).' + ); } $ids = []; @@ -362,11 +374,9 @@ final class MediaUpdateAbility implements Ability private static function failure(int $index, array $item, WP_Error $error): array { $payload = json_decode($error->get_error_message(), true); - $id = $item['id'] ?? null; - return [ 'index' => $index, - 'id' => is_int($id) ? $id : null, + 'id' => PostGuard::id($item['id'] ?? null), 'code' => (string) preg_replace('/^lit_/', '', (string) $error->get_error_code()), 'error' => is_array($payload) ? (string) ($payload['message'] ?? '') : $error->get_error_message(), ]; diff --git a/src/Integrations/Media/MediaUsageFinder.php b/src/Integrations/Media/MediaUsageFinder.php index 1042351..0f6d3f2 100644 --- a/src/Integrations/Media/MediaUsageFinder.php +++ b/src/Integrations/Media/MediaUsageFinder.php @@ -34,9 +34,10 @@ class MediaUsageFinder public const POSSIBLE = 'möglicherweise'; public const UNREADABLE_TITLE = '(nicht lesbar)'; public const INCOMPLETE_TITLE = 'Inhaltssuche unvollständig – weitere Verwendungen möglich'; + public const FAILED_TITLE = 'Verwendungsprüfung fehlgeschlagen – Verwendungen unbekannt'; public const WHERE = [ 'featured', 'field', 'content', 'site_icon', 'custom_logo', 'divi_logo', 'header_image', - 'background_image', 'widget', + 'background_image', 'widget', 'unknown', ]; /** Max. entries whose content is inspected per call. */ @@ -62,6 +63,9 @@ class MediaUsageFinder /** * @param Closure(): list $mediaMetaKeys Keys of file/image fields (ContentSchema::mediaFieldKeys()). */ + /** Set by results() when a query of the current collect() run failed. */ + private bool $queryFailed = false; + public function __construct(private Closure $mediaMetaKeys, private ?\wpdb $db = null) { } @@ -109,16 +113,21 @@ class MediaUsageFinder $found = []; $complete = true; + $this->queryFailed = false; $this->addRows($found, $this->featured($wpdb, $attachmentId, $limit), 'featured'); + $this->addFailure($found); if (count($found) < $limit) { $this->addRows($found, $this->fields($wpdb, $attachmentId, $limit), 'field'); + $this->addFailure($found); } if (count($found) < $limit) { $this->addSiteUsages($found, $attachmentId, $fileRegex); } if (count($found) < $limit) { $complete = $this->addContentUsages($found, $wpdb, $attachmentId, $stem, $fileRegex, $limit); + $this->addFailure($found); } + $complete = $complete && ! $this->queryFailed; return [ 'usages' => array_slice(array_values($found), 0, $max), @@ -127,6 +136,41 @@ class MediaUsageFinder ]; } + /** + * A failed query may have hidden usages: add one "possible" entry so the attachment never + * looks unused (media-delete then needs force, the unused filter skips it). + * + * @param array> $found + */ + private function addFailure(array &$found): void + { + if ($this->queryFailed && ! isset($found['check:failed'])) { + $found['check:failed'] = self::siteUsage(self::FAILED_TITLE, 'check', 'unknown', self::POSSIBLE); + } + } + + /** + * get_results() that notices failures: a null result or a database error marks the + * current scan as failed (see addFailure()) instead of passing for "no rows". + * + * @return list + */ + private function results(\wpdb $wpdb, ?string $sql): array + { + $wpdb->last_error = ''; + // phpcs:ignore WordPress.DB.PreparedSQL.NotPrepared -- every caller passes $wpdb->prepare() output. + $rows = is_string($sql) && $sql !== '' ? $wpdb->get_results($sql) : null; + if (is_array($rows) && (string) $wpdb->last_error === '') { + return array_values($rows); + } + + $this->queryFailed = true; + $message = sprintf('[wp-lit-mcp] media usage query failed: %s', (string) $wpdb->last_error); + error_log($message); // phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_error_log + + return []; + } + /** @return array JSON Schema of one entry of usages(). */ public static function usageSchema(): array { @@ -149,7 +193,7 @@ class MediaUsageFinder $statuses = self::EXCLUDED_STATUSES; // phpcs:disable WordPress.DB.PreparedSQL.InterpolatedNotPrepared -- only table names and a constant. - return (array) $wpdb->get_results($wpdb->prepare( + return $this->results($wpdb, $wpdb->prepare( "SELECT p.ID, p.post_title, p.post_type FROM {$wpdb->postmeta} pm INNER JOIN {$wpdb->posts} p ON p.ID = pm.post_id WHERE pm.meta_key = '_thumbnail_id' AND pm.meta_value = %s @@ -172,7 +216,7 @@ class MediaUsageFinder $statuses = self::EXCLUDED_STATUSES; // phpcs:disable WordPress.DB.PreparedSQL.InterpolatedNotPrepared, WordPress.DB.PreparedSQLPlaceholders.ReplacementsWrongNumber -- $placeholders holds one %s per key. - return (array) $wpdb->get_results($wpdb->prepare( + return $this->results($wpdb, $wpdb->prepare( "SELECT DISTINCT p.ID, p.post_title, p.post_type FROM {$wpdb->postmeta} pm INNER JOIN {$wpdb->posts} p ON p.ID = pm.post_id WHERE pm.meta_key IN ({$placeholders}) AND pm.meta_value = %s @@ -340,7 +384,7 @@ class MediaUsageFinder $statuses = self::EXCLUDED_STATUSES; // phpcs:disable WordPress.DB.PreparedSQL.InterpolatedNotPrepared -- only placeholders, table names, a constant. - return (array) $wpdb->get_results($wpdb->prepare( + return $this->results($wpdb, $wpdb->prepare( "SELECT p.ID, p.post_title, p.post_type FROM {$wpdb->posts} p WHERE p.post_type NOT IN ('revision', 'attachment', 'nav_menu_item') AND p.post_status NOT IN ({$statuses}) @@ -359,7 +403,7 @@ class MediaUsageFinder $placeholders = implode(', ', array_fill(0, count($ids), '%d')); // phpcs:disable WordPress.DB.PreparedSQL.InterpolatedNotPrepared, WordPress.DB.PreparedSQLPlaceholders.UnfinishedPrepare -- $placeholders holds one %d per ID. - $rows = (array) $wpdb->get_results($wpdb->prepare( + $rows = $this->results($wpdb, $wpdb->prepare( "SELECT p.ID, p.post_content FROM {$wpdb->posts} p WHERE p.ID IN ({$placeholders})", ...$ids )); diff --git a/tests/Unit/Abilities/WriteFailedTest.php b/tests/Unit/Abilities/WriteFailedTest.php index a42a209..7babd6a 100644 --- a/tests/Unit/Abilities/WriteFailedTest.php +++ b/tests/Unit/Abilities/WriteFailedTest.php @@ -21,6 +21,15 @@ final class WriteFailedTest extends TestCase self::assertSame('lit_write_failed', $failure->toError()->get_error_code()); } + public function testRolledBackHintReplacesTheDefaultHintOfWriteFailedOnly(): void + { + $clean = new WriteFailed('x', true, rolledBackHint: 'Erst neu lesen.'); + $partial = new WriteFailed('x', false, rolledBackHint: 'Erst neu lesen.'); + + self::assertSame('Erst neu lesen.', $clean->toError()->get_error_data()['hint']); + self::assertNotSame('Erst neu lesen.', $partial->toError()->get_error_data()['hint']); + } + public function testIncompleteRollbackMapsToPartialWriteWithTheIds(): void { $error = (new WriteFailed('Felder konnten nicht gespeichert werden.', false, 5, 811))->toError(); diff --git a/tests/Unit/Integrations/Media/MediaDeleteAbilityTest.php b/tests/Unit/Integrations/Media/MediaDeleteAbilityTest.php index e3a484f..42eb80c 100644 --- a/tests/Unit/Integrations/Media/MediaDeleteAbilityTest.php +++ b/tests/Unit/Integrations/Media/MediaDeleteAbilityTest.php @@ -23,6 +23,7 @@ final class MediaDeleteAbilityTest extends TestCase parent::setUp(); Functions\when('get_post')->justReturn(new WP_Post(['ID' => 33, 'post_type' => 'attachment', 'post_status' => 'inherit', 'post_title' => 'Foto'])); Functions\when('current_user_can')->justReturn(true); + Functions\when('get_post_meta')->justReturn('2026/09/foto.jpg'); } private function ability(array $usages, ChangeLog $log, bool $trash = false): MediaDeleteAbility @@ -96,8 +97,9 @@ final class MediaDeleteAbilityTest extends TestCase self::assertSame('trashed', $result['changed'][0]['action']); } - public function testRefusedDeleteIsAWriteFailure(): void + public function testRefusedDeleteWithTheAttachmentIntactIsAWriteFailure(): void { + Functions\when('get_post_meta')->justReturn('2026/09/foto.jpg'); Functions\expect('wp_delete_attachment')->once()->andReturn(false); $result = $this->ability([], Mockery::mock(ChangeLog::class))->execute(['id' => 33, 'confirm' => true]); @@ -105,6 +107,54 @@ final class MediaDeleteAbilityTest extends TestCase self::assertSame('lit_write_failed', $result->get_error_code()); } + public function testRefusedDeleteThatRemovedDataIsAPartialWrite(): void + { + $meta = '2026/09/foto.jpg'; + Functions\when('get_post_meta')->alias(static function () use (&$meta): string { + return $meta; + }); + Functions\expect('wp_delete_attachment')->once()->andReturnUsing(static function () use (&$meta) { + $meta = ''; + + return null; + }); + $log = Mockery::mock(ChangeLog::class); + $log->expects('recordPartial')->once()->with('lit/media-delete', 'attachment', '33', Mockery::type('string')); + + $result = $this->ability([], $log)->execute(['id' => 33, 'confirm' => true]); + + self::assertSame('lit_partial_write', $result->get_error_code()); + self::assertSame(33, $result->get_error_data()['attachment_id']); + } + + public function testADatabaseErrorInTheUsageCheckRefusesWithoutForce(): void + { + Functions\when('get_post_meta')->justReturn('2026/09/foto.jpg'); + Functions\when('get_option')->justReturn(false); + Functions\when('get_theme_mod')->justReturn(false); + Functions\expect('wp_delete_attachment')->never(); + /** @var \Mockery\MockInterface&\wpdb $db */ + $db = Mockery::mock(\wpdb::class)->makePartial(); + $db->allows('get_results')->andReturn(null); + $ability = new MediaDeleteAbility( + new MediaUsageFinder(static fn (): array => [], $db), + Mockery::mock(ChangeLog::class), + static fn (): bool => false + ); + + $file = (string) tempnam(sys_get_temp_dir(), 'lit-mcp-log'); + $previous = ini_set('error_log', $file); + try { + $result = $ability->execute(['id' => 33, 'confirm' => true]); + } finally { + ini_set('error_log', (string) $previous); + unlink($file); + } + + self::assertSame('lit_in_use', $result->get_error_code()); + self::assertSame(MediaUsageFinder::FAILED_TITLE, $result->get_error_data()['usages'][0]['title']); + } + public function testChangeLogFailureIsOnlyAWarning(): void { Functions\expect('wp_delete_attachment')->once()->andReturn(new WP_Post(['ID' => 33])); diff --git a/tests/Unit/Integrations/Media/MediaUpdateAbilityTest.php b/tests/Unit/Integrations/Media/MediaUpdateAbilityTest.php index a883529..b472903 100644 --- a/tests/Unit/Integrations/Media/MediaUpdateAbilityTest.php +++ b/tests/Unit/Integrations/Media/MediaUpdateAbilityTest.php @@ -102,17 +102,19 @@ final class MediaUpdateAbilityTest extends TestCase $failures = $result->get_error_data()['failures']; self::assertSame([1, 2, 3], array_column($failures, 'index')); self::assertSame(['not_found', 'not_found', 'validation_failed'], array_column($failures, 'code')); + self::assertSame([99, 5, 35], array_column($failures, 'id')); self::assertStringContainsString('nicht gefunden', $failures[0]['error']); self::assertStringContainsString('Papierkorb', $failures[2]['error']); self::assertSame([], $this->postUpdates); self::assertSame([], $this->altUpdates); } - public function testFailuresOfOneKindKeepTheirCode(): void + public function testFailuresOfOneKindKeepTheirCodeAndReportTheNormalizedId(): void { - $result = (new MediaUpdateAbility($this->log()))->execute(['items' => [['id' => 99, 'alt' => 'x']]]); + $result = (new MediaUpdateAbility($this->log()))->execute(['items' => [['id' => '99', 'alt' => 'x']]]); self::assertSame('lit_not_found', $result->get_error_code()); + self::assertSame(99, $result->get_error_data()['failures'][0]['id']); } public function testRejectsEmptyDuplicateAndOverlongItems(): void @@ -165,6 +167,7 @@ final class MediaUpdateAbilityTest extends TestCase } catch (WriteFailed $e) { self::assertTrue($e->rolledBack); self::assertSame('lit_write_failed', $e->toError()->get_error_code()); + self::assertStringContainsString('neu lesen', $e->toError()->get_error_data()['hint']); self::assertStringContainsString('Datenbankfehler', $e->getMessage()); } diff --git a/tests/Unit/Integrations/Media/MediaUsageFinderTest.php b/tests/Unit/Integrations/Media/MediaUsageFinderTest.php index 979b388..72510e7 100644 --- a/tests/Unit/Integrations/Media/MediaUsageFinderTest.php +++ b/tests/Unit/Integrations/Media/MediaUsageFinderTest.php @@ -316,4 +316,63 @@ final class MediaUsageFinderTest extends TestCase self::assertStringNotContainsString('foto', $this->queries[1]); self::assertStringNotContainsString("'%/%'", $this->queries[1]); } + + /** + * A finder whose query $failing (SQL fragment) fails – returns null or sets last_error. + */ + private function failingFinder(string $failing, bool $viaLastError): MediaUsageFinder + { + Functions\when('get_post_meta')->justReturn('2026/09/foto.jpg'); + Functions\when('get_option')->justReturn(false); + Functions\when('get_theme_mod')->justReturn(false); + + /** @var MockInterface&\wpdb $db */ + $db = Mockery::mock(\wpdb::class)->makePartial(); + $db->allows('get_results')->andReturnUsing(static function (string $sql) use ($db, $failing, $viaLastError) { + if (! str_contains($sql, $failing)) { + return []; + } + if ($viaLastError) { + $db->last_error = 'Lost connection to MySQL server'; + + return []; + } + + return null; + }); + + return new MediaUsageFinder(static fn (): array => ['photo'], $db); + } + + /** @return mixed Result of $run with error_log() redirected (a PHP internal, not stubbable). */ + private static function quietly(callable $run): mixed + { + $file = (string) tempnam(sys_get_temp_dir(), 'lit-mcp-log'); + $previous = ini_set('error_log', $file); + try { + return $run(); + } finally { + ini_set('error_log', (string) $previous); + unlink($file); + } + } + + public function testAFailedQueryNeverReportsUnused(): void + { + foreach (['_thumbnail_id', 'pm.meta_key IN', 'post_content LIKE'] as $failing) { + foreach ([true, false] as $viaLastError) { + $label = $failing . ($viaLastError ? ' (last_error)' : ' (null)'); + + self::assertTrue(self::quietly(fn () => $this->failingFinder($failing, $viaLastError)->isUsed(33)), $label); + + $scan = self::quietly(fn () => $this->failingFinder($failing, $viaLastError)->scan(33)); + self::assertFalse($scan['complete'], $label); + self::assertSame( + [['post_id' => 0, 'title' => MediaUsageFinder::FAILED_TITLE, 'post_type' => 'check', 'where' => 'unknown', 'certainty' => 'möglicherweise']], + $scan['usages'], + $label + ); + } + } + } }