Skip to content

Keep a font failure a render can't tell is stale #2035

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jakejackson1 merged 1 commit into from
Nov 12, 2026
Merged

Keep a font failure a render can't tell is stale #2035

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 49 additions & 6 deletions src/Model/Model_Font_Downloader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,9 +164,10 @@ class Model_Font_Downloader extends Helper_Abstract_Model {

/**
* When each failed file's retry is due (else 0), once the retry also failed when it gave up (else 0), and whether
* an admin dismissed the notice listing it
* an admin dismissed the notice listing it. `conflict` is the size and modified time of another font's file at its
* name when it failed, else ''.
*
* @var array<string, array{retry_at: int, gave_up: int, dismissed: bool}>|null
* @var array<string, array{retry_at: int, gave_up: int, dismissed: bool, conflict: string}>|null
* @since 6.19.0
*/
protected $failures;
Expand Down Expand Up @@ -397,6 +398,8 @@ public function download( string $file, bool $verify = false ): bool {
/* A file at the name of a known size can still be another font's, which only its SHA-1 tells */
if ( $own_copy && $verify && $own_path === $font_path && $this->is_auto_install_enabled( $file ) ) {
$own_copy = ! $this->is_unknown_build( $file );
} elseif ( $own_copy && $this->is_failed_conflict( $file, $own_path ) ) {
$own_copy = false;
}

/* Copied in by hand or saved by another request, so any failure on record is stale */
Expand Down Expand Up @@ -477,6 +480,40 @@ protected function is_unknown_build( string $file ): bool {
return $sha !== '' && ! in_array( $sha, $this->font_sources->get_known_builds( $file )[ $size ] ?? [], true );
}

/**
* Whether the font's own copy is on disk, as is_installed() but without the file a failed download found was another
* font's (see is_failed_conflict())
*
* @since 6.19.0
*/
protected function has_own_copy( string $file ): bool {
$own_path = $this->font_sources->get_own_copy( $file );

return $own_path !== '' && ! $this->is_failed_conflict( $file, $own_path );
}

/**
* Whether the own copy get_own_copy() found by size is the other font's file a failed download found at the name,
* unchanged since. Saves hashing it again on a render.
*
* @since 6.19.0
*/
protected function is_failed_conflict( string $file, string $own_path ): bool {
$conflict = $this->get_failures()[ $file ]['conflict'] ?? '';

/* A hashed copy is the font's own whatever is at the name */
return $conflict !== '' && $own_path === $this->font_sources->get_plain_path( $file ) && $conflict === $this->get_file_stamp( $own_path );
}

/**
* A file's size and modified time, or '' if it isn't on disk
*
* @since 6.19.0
*/
protected function get_file_stamp( string $path ): string {
return is_file( $path ) ? filesize( $path ) . ':' . filemtime( $path ) : '';
}

/**
* Hash a font on disk, separate so tests can count the calls
*
Expand Down Expand Up @@ -1178,7 +1215,7 @@ public function get_wanted_files(): array {
* @since 6.19.0
*/
public function download_or_queue( string $file ): bool {
if ( ! $this->font_sources->is_large_file( $file ) || $this->font_sources->is_installed( $file ) ) {
if ( ! $this->font_sources->is_large_file( $file ) || $this->has_own_copy( $file ) ) {
return $this->download( $file );
}

Expand Down Expand Up @@ -1679,6 +1716,9 @@ protected function is_backing_off( string $file ): bool {
protected function record_failure( string $file ): void {
set_site_transient( self::back_off_key( $file ), time(), self::BACK_OFF );

/* A failed download leaves the font's name as it was, so a file there is another font's */
$conflict = $this->get_file_stamp( $this->font_sources->get_plain_path( $file ) );

$failures = $this->get_failures();
$failure = $failures[ $file ] ?? null;

Expand All @@ -1687,6 +1727,7 @@ protected function record_failure( string $file ): void {
'retry_at' => time() + wp_rand( 0, $this->retry_jitter ),
'gave_up' => 0,
'dismissed' => false,
'conflict' => $conflict,
];

$this->save_failures( $failures );
Expand All @@ -1705,6 +1746,7 @@ protected function record_failure( string $file ): void {
'retry_at' => 0,
'gave_up' => time(),
'dismissed' => false,
'conflict' => $conflict,
];

$this->save_failures( $failures );
Expand Down Expand Up @@ -1736,7 +1778,7 @@ protected function delete_back_off( string $file ): void {
}

/**
* @param array<string, array{retry_at: int, gave_up: int, dismissed: bool}> $failures
* @param array<string, array{retry_at: int, gave_up: int, dismissed: bool, conflict: string}> $failures
*
* @since 6.19.0
*/
Expand All @@ -1749,7 +1791,7 @@ protected function save_failures( array $failures ): void {
/**
* Failures by file, read once per request, when any give-up whose file is now on disk is cleared from the record
*
* @return array<string, array{retry_at: int, gave_up: int, dismissed: bool}>
* @return array<string, array{retry_at: int, gave_up: int, dismissed: bool, conflict: string}>
*
* @since 6.19.0
*/
Expand All @@ -1768,6 +1810,7 @@ protected function get_failures(): array {
'retry_at' => $gave_up > 0 ? 0 : $retry_at,
'gave_up' => $gave_up,
'dismissed' => ( $failure['dismissed'] ?? false ) === true,
'conflict' => is_string( $failure['conflict'] ?? null ) ? $failure['conflict'] : '',
];
}
}
Expand All @@ -1785,7 +1828,7 @@ protected function get_failures(): array {
*/
protected function clear_installed_give_ups(): void {
foreach ( $this->failures as $file => $failure ) {
if ( $failure['gave_up'] > 0 && $this->font_sources->is_installed( $file ) ) {
if ( $failure['gave_up'] > 0 && $this->has_own_copy( $file ) ) {
$this->clear_failure( $file );
}
}
Expand Down
3 changes: 2 additions & 1 deletion tests/phpunit/Concerns/RecordsFontFailures.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,14 @@ trait RecordsFontFailures {
* @param int $gave_up When its retry also failed, or 0 while the retry is pending
* @param int|null $retry_at When the pending retry is due, defaulting to now
*
* @return array{retry_at: int, gave_up: int, dismissed: bool}
* @return array{retry_at: int, gave_up: int, dismissed: bool, conflict: string}
*/
protected function failure( int $gave_up = 0, ?int $retry_at = null ): array {
return [
'retry_at' => $gave_up > 0 ? 0 : ( $retry_at ?? time() ),
'gave_up' => $gave_up,
'dismissed' => false,
'conflict' => '',
];
}

Expand Down
46 changes: 46 additions & 0 deletions tests/phpunit/integration/Model/Test_Model_Font_Downloader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -596,6 +596,52 @@ public function test_a_retry_for_a_file_beside_another_font_is_not_cleared() {
$this->assertEmpty( $this->get_font_failures() );
}

public function test_a_same_size_font_found_when_verifying_keeps_its_failure_until_it_gives_up() {
$this->put_same_size_font_at( self::FONT, self::FONT );
$this->fail_font_downloads();

$this->assertFalse( $this->new_downloader()->download( self::FONT, true ) );
$this->assertNotSame( '', $this->get_font_failures()[ self::FONT ]['conflict'] );

/* A render goes by size, which the other font's file has */
$this->assertFalse( $this->new_downloader()->download( self::FONT ) );
$this->assertSame( 0, $this->get_font_failures()[ self::FONT ]['gave_up'] );

$this->delete_back_off( self::FONT );
$this->assertFalse( $this->new_downloader()->download( self::FONT, true ) );

$this->assertSame( [ self::FONT ], $this->new_downloader()->get_undismissed_failed_files() );
}

public function test_a_render_queues_a_large_font_whose_name_a_failed_download_found_another_font_at() {
$this->put_same_size_font_at( self::FONT, self::FONT );
$this->fail_font_downloads();
$this->new_downloader()->download( self::FONT, true );
$this->delete_back_off( self::FONT );

$manifest = GPDFAPI::get_mvc_class( 'Model_Font_Sources' )->get_manifest();
$manifest[ self::FONT ]['size'] = Model_Font_Sources::LARGE_FILE_SIZE + 1;
$this->set_shared_manifest( $manifest );

$downloader = $this->new_downloader();
$this->assertFalse( $downloader->download_or_queue( self::FONT ) );
$this->assertCount( 1, $this->requested, 'The render leaves the download to the sync' );
$this->assertSame( [ self::FONT ], $downloader->get_wanted_files() );
}

public function test_a_font_copied_over_another_fonts_file_clears_its_failure() {
$this->put_same_size_font_at( self::FONT, self::FONT );
$this->fail_font_downloads();
$this->new_downloader()->download( self::FONT, true );

$this->copy_fixture( self::FONT );
touch( $this->font_path( self::FONT ), time() + 1 );

$this->assertTrue( $this->new_downloader()->download( self::FONT ) );
$this->assertEmpty( $this->get_font_failures() );
$this->assertFalse( $this->get_back_off( self::FONT ) );
}

public function test_prune_never_removes_a_hashed_copy_or_a_replaced_file() {
$sources = GPDFAPI::get_mvc_class( 'Model_Font_Sources' );
$hashed = $sources->get_hashed_name( self::FONT );
Expand Down