Don't record a font failure when another request already saved the font

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 );
 			}
 

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() );
 	}
 
 	/**