Repository navigation
fix(avif)!: scope AVIF sibling filenames by attachment ID to prevent collisions - #289
Merged
Merged
Conversation
…collisions
AVIF sibling filenames were derived from the source file's basename alone
(e.g. image-300x200.avif), duplicated across AvifVariantGenerator,
AvifCleanup, and FocalPointCropper. WordPress allows two distinct
attachments to share a basename across different extensions (e.g.
siegel.png and siegel.jpg), so both attachments resolved to the identical
AVIF path. That collision caused two production symptoms:
- 0-byte/corrupted AVIF output, from concurrent requests for different
attachments racing to write the same shared file.
- Deleting one attachment (e.g. siegel.jpg) unlinking the AVIF sibling
that actually belonged to a different, still-live attachment
(e.g. siegel.png), via the delete_attachment cleanup hook.
Fixes by centralizing sibling-path derivation in AvifSiblingPath::for(),
which scopes the filename by attachment ID (image-300x200.{id}.avif),
and wiring it into all three previous call sites.
BREAKING CHANGE: AVIF sibling files generated before this change use the
old basename-only naming and are never read again after upgrading (the
resolver only looks up the new attachment-ID-scoped name). They become
orphaned on disk, not corrupted or unsafe, just dead weight, and every
image will lazily regenerate its AVIF sibling under the new name on next
resolution (bounded by the existing per-request generation cap, with the
original <img> as fallback in the meantime). See the PR description for
a cleanup command to reclaim the disk space.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a production bug where two WordPress attachments sharing a basename across different extensions (e.g.
siegel.pngandsiegel.jpg) collided on the same generated AVIF sibling path. Root cause and fix details below.Root cause
AvifVariantGenerator,AvifCleanup, andFocalPointCroppereach independently derived AVIF sibling filenames from the source file's basename alone (image-300x200.jpg->image-300x200.avif). WordPress legitimately allows two different attachments to share a basename as long as the extension differs (wp_unique_filename()only dedupes same-extension collisions), so two unrelated attachments could resolve to the identical AVIF path.This produced two symptoms reported from a live site:
AvifVariantGenerator::generate()writes directly to the shared path with no locking or atomic rename. Two requests resolving different attachments could race on the same file, leaving it truncated for whichever attachment read it next.delete_attachment,AvifCleanupunlinks the sibling it derives from the deleted attachment's own file, with no check that the file actually belongs to that attachment. Deletingsiegel.jpgremoved the AVIF siblingsiegel.pngdepended on.Fix
Centralized sibling-path derivation in a new
AvifSiblingPath::for(string $file, int $attachmentId): ?string, which scopes the filename by attachment ID:image-300x200.jpg->image-300x200.{attachmentId}.avif. Wired it into the three previous call sites (all of them already had the attachment ID in scope, so no new plumbing was needed):AvifVariantGenerator::ensure()AvifCleanup::doForget()FocalPointCropper::purgeAvifSibling()Added regression coverage: two attachments sharing a basename now get distinct AVIF paths, and deleting one never touches the other's sibling. Updated
specs/avif-conversion.mdwith the collision-scoping rule and a new Gherkin scenario.BREAKING CHANGE
AVIF siblings generated before this change used the old basename-only naming (
image-300x200.avif). After upgrading, the resolver only ever looks up the new attachment-ID-scoped name, so:<img>as fallback in the meantime. No broken images during the transition.This is not a PHP API break (no public method signatures changed), but it does mean sites with AVIF enabled will temporarily double their AVIF storage until the old files are cleaned up.
Cleanup command
Run this once per site after upgrading to reclaim the disk space used by orphaned pre-upgrade AVIF files. It only touches files that are legacy-named siblings of a real, current WordPress attachment (original file or a registered subsize), so it will not touch anything that isn't a stale Sproutset-generated sibling.
Save as
cleanup-legacy-avif.phpin the site root and run with WP-CLI:Run the
dry-runpass first and eyeball the file list before deleting. Delete the script after use; it is not part of the package.Verification
vendor/bin/pint --dirtycleancomposer types:check(PHPStan/Larastan, level max) cleancomposer test(81 unit/feature tests) passingcomposer test:integration(58 WordPress-backed tests, including new collision regressions) passing