Fix: don't overwrite an unrelated Cloudinary asset on public ID collision

[gabrielcld2]

Summary

• PR #1182 fixed a runaway-duplicate bug (an attachment retrying an interrupted upload kept creating suffixed Cloudinary duplicates) by overwriting the conflicting asset whenever the attachment had no locally-saved _public_id.
• That signal isn't reliable on its own: every never-synced attachment also has no _public_id, not just one recovering from a crashed upload. So a brand new attachment that happens to derive the same public ID as an older, unrelated asset (e.g. WordPress reuses a filename across different months) also takes the overwrite path and silently destroys that older asset.
• This adds a check, Upload_Sync::is_matching_existing_asset(), that only takes the overwrite path when the conflicting Cloudinary asset is confirmed to be the local file. Same file, lost record (PR Fix duplicate assets on DB write fails scenarios #1182's case) → confirmed match → still safely overwritten. Different file, coincidental public ID collision (this issue) → no match → falls back to the pre-Fix duplicate assets on DB write fails scenarios #1182 suffix behavior instead, so the older asset is preserved.

Fixes #1241

Review follow-up

Addressed in a follow-up commit after review:

• Blocker: the check now resolves the local file the same way Api::upload() does (Media::get_upload_file_path(), shared by both), instead of always using get_attached_file(). That matters for any image over big_image_size_threshold (2560px default) — WordPress attaches the -scaled copy but Cloudinary is sent the unscaled original, so comparing against the attached file broke the Fix duplicate assets on DB write fails scenarios #1182 recovery path for large images.
• Confirms with the response's etag (MD5 of the stored asset) once byte sizes already match, closing the remaining false-positive where two unrelated files coincidentally share a byte count.
• Logs via Utils::log() when the check bails out for a missing bytes field, so a future API response change surfaces in the debug log instead of silently reviving the duplicate-per-cycle bug.
• Docblock narrowed and marked @internal.

QA notes

Scenario 1 — the reported bug (#1241) is fixed

1. Upload image A named test.jpg. Wait for sync. Note its ID as <A_ID>.
2. Upload a second, different image (any filename). Note its ID as <B_ID>. Then force it into "first-ever upload of test.jpg" state:

   npx wp-env run cli wp eval '
   $id      = <B_ID>;
   $old     = get_attached_file( $id );
   $new_dir = dirname( $old ) . "-b";
   wp_mkdir_p( $new_dir );
   $new = trailingslashit( $new_dir ) . "test.jpg";
   rename( $old, $new );
   update_attached_file( $id, $new );
   $m = get_post_meta( $id, "_cloudinary", true );
   unset( $m["_public_id"], $m["_sync_signature"] );
   update_post_meta( $id, "_cloudinary", $m );
   '

3. Trigger B's sync:

   npx wp-env run cli wp eval 'Cloudinary\get_plugin_instance()->get_component("sync")->managers["push"]->process_assets(<B_ID>);'

4. Compare public IDs:

   npx wp-env run cli wp eval 'echo get_post_meta(<A_ID>, "_cloudinary", true)["_public_id"] . "\n";'
   npx wp-env run cli wp eval 'echo get_post_meta(<B_ID>, "_cloudinary", true)["_public_id"] . "\n";'

   • On develop (bug present): both print test — B silently claimed A's public ID and overwrote its Cloudinary asset.
   • On this branch (fixed): A stays test, B gets a suffixed public ID (e.g. test_<id><random>) — both assets survive independently.

Scenario 2 — PR #1182's original crash-recovery fix still works

Use an image larger than 2560px on its longest side for step 1 — that's what exercises the -scaled file path the review feedback flagged, and what scenario 2 originally missed.

1. Upload the large image. Wait for sync. Note its ID as <X_ID>.
2. Simulate a lost record (same file, nothing changes — this is the "meta lost" case, not the collision case):

   npx wp-env run cli wp eval '
   $id = <X_ID>;
   $m = get_post_meta( $id, "_cloudinary", true );
   unset( $m["_public_id"], $m["_sync_signature"] );
   update_post_meta( $id, "_cloudinary", $m );
   '

3. Re-trigger the sync:

   npx wp-env run cli wp eval 'Cloudinary\get_plugin_instance()->get_component("sync")->managers["push"]->process_assets(<X_ID>);'

4. Check the saved public_id:

   npx wp-env run cli wp eval 'echo get_post_meta(<X_ID>, "_cloudinary", true)["_public_id"] . "\n";'

   • Expected (fix intact): no suffix — the attachment recovered by overwriting its own orphaned asset, same as before.
   • A suffixed public_id here would mean the crash-recovery path regressed.

Both scenarios were run and confirmed against a real Cloudinary account, including scenario 2 re-run with a 3200×1800px/1.74MB image (large enough to trigger WordPress's -scaled file, per the review feedback): the recovered public_id came back unsuffixed, confirming the crash-recovery path now also works correctly for scaled images.

Automated tests

tests/phpunit/tests/test-upload-sync.php covers is_matching_existing_asset(): matching/mismatched bytes, a missing bytes field, a missing local file, matching/mismatched etag, and a scaled-image case asserting the check compares against the unscaled original rather than the attached -scaled file. Run with npm run env:start && npm run test:unit.

[utkarshcloudinary]

Review with help of AI and I validated those comments:

[utkarshcloudinary]

Blocker: this does not resolve the same file that the upload actually sent, so the #1182 recovery path stops working for large images.

Api::upload() applies the cloudinary_use_original_image filter (default true) and then uses wp_get_original_image_path() for images:

// php/connect/class-api.php:529-534, 564-565
$use_original = apply_filters( 'cloudinary_use_original_image', true, $attachment_id );
...
$get_path_func = $use_original && function_exists( 'wp_get_original_image_path' ) ? 'wp_get_original_image_path' : 'get_attached_file';

Core diverges the two whenever original_image is set in the attachment metadata: wp_get_original_image_path() returns the untouched original, get_attached_file() returns the -scaled file.

So for any image above big_image_size_threshold (2560px default, which covers most phone and camera uploads) this compares the -scaled bytes against the original's bytes. They never match, is_matching_existing_asset() returns false, and the attachment falls back to the suffix branch. That is exactly the runaway-duplicate behaviour PR #1182 existed to stop.

The QA notes in the description do not catch this because both scenarios use a small image that never gets scaled.

Mirroring the upload's own resolution fixes it:

$use_original = apply_filters( 'cloudinary_use_original_image', true, $attachment_id );
if ( $use_original && function_exists( 'wp_get_original_image_path' ) && wp_attachment_is_image( $attachment_id ) ) {
	$file = wp_get_original_image_path( $attachment_id );
} else {
	$file = get_attached_file( $attachment_id );
}

Media::get_attachment_file_size() (php/class-media.php:475-496) already contains this branch. Worth extracting it into one shared helper rather than duplicating a third copy. Note it caches into _file_size meta, so calling that method directly would feed a possibly stale value into a destructive decision.

Please also re-run QA scenario 2 with an image over 2560px, that is the case that currently fails.

[gabrielcld2]

Fixed in 9187312 — thanks for catching this. is_matching_existing_asset() now calls a new shared Media::get_upload_file_path(), which is the exact same resolution Api::upload() uses (cloudinary_use_original_image filter + wp_get_original_image_path() for images, falling back to get_attached_file()), so both now compare against the same file Api::upload() actually sent. Api::upload()'s own inline branch was replaced with a call to the same helper, so there's one canonical implementation instead of a third copy. Added a test (test_matches_using_the_unscaled_original_for_a_scaled_image) that fakes a scaled/original pair via _wp_attachment_metadata['original_image'] and asserts the check matches on the original's bytes, not the attached (scaled) file's.\n\nRe-running QA scenario 2 with an image over 2560px on this commit — will report back once confirmed.

[gabrielcld2]

Confirmed — re-ran QA scenario 2 with a 3200×1800px, 1.74MB image (large enough to get WordPress's -scaled treatment). The recovered public_id came back unsuffixed (scaled-image-test), so the crash-recovery path now works correctly for scaled images too. Updated the PR description accordingly.

[utkarshcloudinary]

Consider: byte size alone is a weak identity signal, in both directions.

• False positive: two unrelated files that derive the same public ID and happen to have the same byte count still take the overwrite path, and the older asset is still destroyed. That is the exact failure mode this PR exists to prevent, narrowed rather than closed.
• False negative: if the product environment applies an incoming transformation, or a cloudinary_upload_options filter adds transformation, the stored asset's bytes will not equal the local file's bytes, and the Fix duplicate assets on DB write fails scenarios #1182 recovery silently stops working for that environment.

Cloudinary returns etag in the same response, which is the MD5 of the stored bytes. Gating on bytes first and then confirming with md5_file() closes the false positive completely, and the hash only runs on the collision path, never on a normal upload:

if ( (int) filesize( $file ) !== (int) $result['bytes'] ) {
	return false;
}

return empty( $result['etag'] ) || md5_file( $file ) === $result['etag'];

[gabrielcld2]

Done in 9187312 — once bytes match, it now confirms with md5_file() against the response's etag, only computed on the collision path as you suggested. Kept it opt-in (empty( $result['etag'] ) ||) rather than required, so a response missing that field still falls back to the byte check instead of always failing closed — the false-negative you flagged (transformation-altered stored bytes) already broke the byte check on its own, so treating a missing etag as inconclusive-in-the-wrong-direction there wouldn't help; it would just add a second silent failure mode. Added two tests covering the etag match/mismatch cases.

[utkarshcloudinary]

Nit: the fallback direction is right, safe over destructive. But it makes the #1182 fix depend on an undocumented field of the existing response. If that field is ever trimmed, the recovery path dies silently, with no error and no log, and suffixed duplicates quietly come back.

A sync note or debug trace when the check bails out on a missing field would save the next person from bisecting two PRs to work out why.

[gabrielcld2]

Added — 9187312 logs via Utils::log() (the existing debug-report mechanism, see class-delivery.php/class-responsive-breakpoints.php for the same pattern) when the check bails out for a missing bytes field, so a future response shape change shows up in the debug log instead of silently reverting to the old duplicate-per-cycle behavior.

[utkarshcloudinary]

Nit: public for a method with a single internal caller. Reasonable trade to make it reachable from the test, but it is now plugin API surface that has to keep its signature. An @internal note in the docblock would set the expectation.

Also worth narrowing the docblock's claim of generality: the folder and cloud_name sync types route to Api::copy(), which uploads from a Cloudinary URL, not a local file, and under offload=cld there may be no local file at all. In practice those types only run once a public_id is recorded, so the second clause of the condition short-circuits first and this is never reached, but the docblock reads as though it applies to any upload.

[gabrielcld2]

Both addressed in 9187312: added an @internal note, and narrowed the docblock to note this only actually runs for the default sync type in practice, since folder/cloud_name route through Api::copy() and, as you noted, only ever get here once a public_id already exists (short-circuiting the first clause). Left it public per your call.