From fc618b9252a0f1182703cd8dbd4dc45952ec5d38 Mon Sep 17 00:00:00 2001 From: gravitypdf-bot Date: Thu, 5 Nov 2026 14:39:59 +1100 Subject: [PATCH] Don't record a failure when another request already saved the font When the move into the fonts folder failed but the font there already matched the manifest (another request saved it first), save() kept that font yet returned false, so download() recorded a failure. The retry then found the file and returned early without clearing it, leaving the failure entry and the failed fonts notice in place. A failed move whose destination matches the manifest now deletes the temp file and succeeds, whether the destination is the font's name or its hashed copy. download() clears any failure and back-off for a font it finds on disk (at its name or as a hashed copy), which also covers fonts copied in by hand and failures that already gave up. get_retries() no longer special-cases a due file on disk, as the sync's download() of it now clears it, so is_resolved() is gone. Co-Authored-By: Claude Opus 5.5 --- src/Model/Model_Font_Downloader.php | 36 +++++++-------- .../Model/Test_Model_Font_Downloader.php | 45 ++++++++++++++++++- 2 files changed, 61 insertions(+), 20 deletions(-) diff --git a/src/Model/Model_Font_Downloader.php b/src/Model/Model_Font_Downloader.php index cd59be24a..8849fff72 100644 --- a/src/Model/Model_Font_Downloader.php +++ b/src/Model/Model_Font_Downloader.php @@ -310,13 +310,15 @@ public function download( string $file, bool $verify = false ): bool { return false; } - if ( $this->font_sources->has_hashed_copy( $file ) ) { - return true; - } - $font_path = $this->font_sources->get_plain_path( $file ); $conflict = is_file( $font_path ); - if ( $conflict && ! $this->is_conflict( $file, $verify && $this->is_auto_install_enabled( $file ) ) ) { + + $own_copy = $this->font_sources->has_hashed_copy( $file ) || ( $conflict && ! $this->is_conflict( $file, $verify && $this->is_auto_install_enabled( $file ) ) ); + + /* Copied in by hand or saved by another request, so any failure on record is stale */ + if ( $own_copy ) { + $this->clear_failure( $file ); + return true; } @@ -403,15 +405,6 @@ protected function hash_blob( string $path, int $size ): string { return Git_Blob::sha1( $path, $size ); } - /** - * Whether the file's own copy is on disk, hashing the file at its name to be sure it's the manifest's - * - * @since 6.19.0 - */ - protected function is_resolved( string $file ): bool { - return $this->font_sources->has_hashed_copy( $file ) || ( is_file( $this->font_sources->get_plain_path( $file ) ) && ! $this->is_conflict( $file, true ) ); - } - /** * Delete the unmodified files (fonts and licences) the Core Font installer downloaded that aren't kept. A file whose * download is turned off stays, as it couldn't come back. Only files of the installer's size are hashed. @@ -1231,8 +1224,7 @@ function ( $file ) use ( $gave_up ) { } /** - * The files whose one retry is due, and when the next one still to come is (or 0). A due file already on disk, - * copied in by hand, no longer needs its retry. + * The files whose one retry is due, and when the next one still to come is (or 0) * * @return array{0: string[], 1: int} * @@ -1250,8 +1242,6 @@ protected function get_retries(): array { if ( $failure['retry_at'] > time() ) { $next = $next === 0 ? $failure['retry_at'] : min( $next, $failure['retry_at'] ); - } elseif ( $this->is_resolved( $file ) ) { - $this->clear_failure( $file ); } else { $due[] = $file; } @@ -1340,8 +1330,16 @@ protected function save( string $file, string $font_path, array $entry ): bool { } if ( ! $this->move( $download_path, $font_path ) ) { + /* The right font is there anyway, as when another request saved it first */ + if ( Git_Blob::matches( $font_path, $entry['size'], $entry['sha'] ) ) { + wp_delete_file( $download_path ); + $this->log->notice( 'Font already in place after a failed move', [ 'name' => $file ] ); + + return true; + } + /* A move across filesystems copies, so a failure can leave a partial font behind */ - if ( is_file( $font_path ) && ! Git_Blob::matches( $font_path, $entry['size'], $entry['sha'] ) ) { + if ( is_file( $font_path ) ) { wp_delete_file( $font_path ); } diff --git a/tests/phpunit/integration/Model/Test_Model_Font_Downloader.php b/tests/phpunit/integration/Model/Test_Model_Font_Downloader.php index 24427c663..5dfa13cca 100644 --- a/tests/phpunit/integration/Model/Test_Model_Font_Downloader.php +++ b/tests/phpunit/integration/Model/Test_Model_Font_Downloader.php @@ -236,8 +236,51 @@ public function test_a_failed_move_keeps_a_valid_font_another_request_saved() { $downloader = $this->new_downloader( file_get_contents( $this->font_fixture( self::FONT ) ) ); $this->serve_font_downloads(); - $this->assertFalse( $downloader->download( self::FONT ) ); + $this->assertTrue( $downloader->download( self::FONT ) ); $this->assertFileEquals( $this->font_fixture( self::FONT ), $this->font_path( self::FONT ) ); + $this->assertNoStrayTmpFiles(); + $this->assertEmpty( $this->get_font_failures() ); + $this->assertCount( 1, $this->log_messages( 'Font already in place after a failed move' ) ); + } + + public function test_a_font_on_disk_clears_its_stale_failure() { + $this->set_font_failures( [ self::FONT => $this->failure( time() ) ] ); + $this->set_back_off( self::FONT ); + $this->copy_fixture( self::FONT ); + $this->serve_font_downloads(); + + $this->assertTrue( $this->downloader->download( self::FONT ) ); + + $this->assertSame( [], $this->requested ); + $this->assertEmpty( $this->get_font_failures() ); + $this->assertFalse( $this->get_back_off( self::FONT ) ); + } + + public function test_a_failed_move_to_a_hashed_copy_another_request_saved_is_not_a_failure() { + $this->put_other_font_at( self::FONT ); + $hashed = GPDFAPI::get_mvc_class( 'Model_Font_Sources' )->get_hashed_name( self::FONT ); + $downloader = $this->new_downloader( file_get_contents( $this->font_fixture( self::FONT ) ) ); + $this->serve_font_downloads(); + + $this->assertTrue( $downloader->download( self::FONT ) ); + $this->assertFileEquals( $this->font_fixture( self::FONT ), $this->font_path( $hashed ) ); + $this->assertFileEquals( $this->font_fixture( 'Chewy.ttf' ), $this->font_path( self::FONT ), 'The other font is left alone' ); + $this->assertNoStrayTmpFiles(); + $this->assertEmpty( $this->get_font_failures() ); + $this->assertCount( 1, $this->log_messages( 'Font already in place after a failed move' ) ); + } + + public function test_a_hashed_copy_on_disk_clears_its_stale_failure() { + $hashed = GPDFAPI::get_mvc_class( 'Model_Font_Sources' )->get_hashed_name( self::FONT ); + $this->set_font_failures( [ self::FONT => $this->failure( time() ) ] ); + $this->put_other_font_at( self::FONT ); + $this->copy_fixture( self::FONT, $hashed ); + $this->serve_font_downloads(); + + $this->assertTrue( $this->downloader->download( self::FONT ) ); + + $this->assertSame( [], $this->requested ); + $this->assertEmpty( $this->get_font_failures() ); } /**