fix(media): keep cached derivatives a sibling attachment still uses

[parisek]

From project

sloneek, staging. The homepage hero rendered with a black background. The
derivative under wp-content/cache/image/900x0-center/homepage-hero-desktop.avif
had been deleted and regenerated by a broken ImageMagick build — but it should
never have been deleted at all: the file, five attachment rows and the rendered
page all still referenced it.

Why

cleanup_cached_images() runs on delete_attachment and matches the cache by
basename alone. The cache is keyed by file; one file routinely carries
several attachment rows:

• WPML writes one row per language over a single file.
• A duplicate upload can be pointed at a path that already exists.

Deleting any one of those rows wiped the derivatives every other row was still
using.

Measured on that site:

files shared by more than one attachment : 5 542
attachment rows over them                : 25 981
rows sharing the hero's file             : 5

Roughly four fifths of the media library could take a live image down with it.
The failure is silent — no error, nothing in a log, and the damage only shows up
when someone looks at the page.

What changed

The delete runs only when no other attachment points at the same
_wp_attached_file. Comparison is on that meta value because it is what
siblings share; get_attached_file() is filtered and absolute, so it is not
comparable across rows.

The guard fails closed: where the question cannot be answered, the files are
kept. A stale derivative is overwritten by the next resize; one deleted in error
vanishes from a page that is still serving it.

Tests

Written first, red for the right reason, then green (tests/Unit/StarterBase/CleanupCachedImagesTest.php):

Test	Before
deletes derivatives when no other attachment shares the file	passed (behaviour preserved)
keeps derivatives when another attachment shares the file	failed — files deleted
keeps derivatives when the sibling count is unavailable	failed — files deleted

composer test:all 1695 + 18 green, composer phpstan clean, composer adr OK.

Deliberately not done

• No StarterBase feature flag. The convention puts behaviour changes behind
  one, default off. Applying it here would leave a data-loss path live for every
  consumer until they opted out of it. Nothing observable is added — a delete
  that destroyed in-use files stops happening. Flagging this is the reviewer's
  call to overturn; I did not want to make it silently.
• The basename collision is untouched. Two different files that share a
  basename across upload-year folders (2022/03/11.png and 2022/10/11.png)
  collide in the flat cache namespace, so deleting one still removes the other's
  derivative. 336 basenames on that site map to more than one file. That needs a
  cache-naming change, not a guard, and it deserves its own discussion — the
  fix invalidates every existing cached derivative.
• %i instead of interpolating $wpdb->postmeta, which keeps the query
  literal for PHPStan rather than buying silence with an ignore.

[parisek]

Agent review — Codex (gpt-5-codex), 2026-08-26

Ran as an independent adversarial pass over the diff. Four findings; two were real
defects in my own guard, two were test-quality. Every one verified against the code
before acting.

Fixed

The fail-closed guard was not closed. get_var() reports a failed query by
returning null, and (int) null is the same 0 a genuine "no siblings" answer
gives. So any transient database error read as not shared and deleted the files
the guard exists to protect — the exact failure the PR is about, reachable through
a different door. The docblock already promised the opposite behaviour. Null and
last_error are now both checked.

An empty _wp_attached_file returned "not shared". get_attached_file() has
already returned a path by that point, so an empty meta value means a filter
supplied it (offloaded media) and siblings have no key to match on. That is
unanswerable, not negative. Now returns shared.

The stub validated nothing. It returned a preselected count whatever the SQL
said, so the suite would have passed against the wrong table, the wrong meta key,
a dropped post_id != %d, or misordered args. Added a test pinning the prepared
query and its arguments, plus cases for the two branches above.

Teardown deleted files it did not create. WP_CONTENT_DIR is a bootstrap
constant, so the cache directory cannot be varied per run — the tests now record
and remove only their own paths. This narrows the blast radius; it does not make
the suite safe to run twice concurrently, and the reviewer was right that only a
per-run directory would.

Rejected

%i → interpolate $wpdb->postmeta. The finding was that %i needs WP 6.2+
and that on older cores the query fails, get_var() returns null, and the guard
silently deletes. The premise is right and the consequence is what the first fix
above removes: a failed query now keeps the files. What remains on a hypothetical
WP <6.2 is that cleanup stops deleting anything — cache bloat, not data loss, and
visible. The suggested fix would have reintroduced the PHPStan literal-string
error that %i was chosen to avoid, buying an ignore for no safety gain.

Worth recording from the confirmations

The reviewer checked the load-bearing premise against local WP 6.8.1 core rather
than asserting it: wp_delete_attachment() reads $file at post.php:6640 and
fires delete_attachment at :6655, before deleting attachment post meta at
:6672–6675. _wp_attached_file is present during the callback. If that ordering
were the other way round the whole fix would be a no-op, so this is the single
most useful line in the report.

It also noted the query does not filter post_type = 'attachment', so an orphaned
postmeta row makes retention overly conservative. Left alone deliberately —
erring toward keeping files is the direction this PR argues for.

Tests 1699 + 18 green, PHPStan clean, composer adr OK.