Post publish upload media dialog: race condition in uploads results in incorrect image replacements

[sgomes]

Description

The issue

When publishing a post with external images, the upload media dialog in the post publish sidebar is shown. This dialog allows you to upload all the images in your post to the media library, and replace the image URLs in the post accordingly, the goal being to benefit from the various performance improvements that WordPress applies to media library images.

However, choosing to perform this upload can result in a race condition where some of the external images get replaced with the wrong local image (see screen recording).

The resulting attachments all have different IDs, but some of them end up pointing to the same file on disk.

The root cause

This took some effort to work out, but here's my understanding of the sequence of events:

• Gutenberg makes several simultaneous POST requests to the REST API (one for each image), all of them with the filename image.png (in the case of PNG uploads, such as in the video example).
• In affected configurations, each of these POST requests spawns a WordPress instance (PHP) as a separate OS-level process, with real concurrency.
• PHP handles the file upload into a unique temporary file, correctly, with no collisions.
• Each of the WordPress instances generates a unique attachment ID. This is done via the DB and thus adequately synchronised, resulting in correct, unique attachment IDs.
• Each of the WordPress instances tries to generate a unique filename. This is done by consulting the filesystem, and generating a name of the form image-N.png such that N is the lowest possible positive integer that isn't already taken.
• The generated filename runs through registered filters, potentially being changed.
• Once the final filename is obtained, the temporary file gets copied over to it.

The issue is that there is no locking mechanism used for the generated filenames, at any level. So if one process consults the filesystem in the time between another process generating the filename and actually creating the file, they'll both end up generating the same filename, and we'll end up with them pointing to the same file on disk, overwriting each other.

Potential fixes

I'll start by saying that I think we should fix this at the media upload dialog level, since even if a fix eventually lands in Core (e.g. by somehow using the DB to synchronise the filenames), this will still affect every prior version of WordPress and will thus still be a concern for a long time to come.

As such, I can think of several strategies to handle this in the media upload dialog:

• Generating a unique name for each upload on the client side. If instead of uploading every image as image.<extension>, we use the block ID, a GUID, or some other sort of unique identifier, we'll bypass the problem by removing the need for the server to generate a unique filename. I believe this is possible in the current codebase, but it may take some changes to mediaUpload (haven't looked too closely).
• Uploading all of the files in a single POST request. This way, a single WordPress instance processes all of them, in sequence, avoiding the issue. This has the downside that it's a single large POST request, increasing the chances of it failing with flaky connections.
• Performing all of the uploads in sequence. This fixes the issue, but can be much slower if individual uploads are not enough to saturate the upload bandwidth.
• Using a hybrid solution, where uploads are batched, and batches are uploaded in sequence. It can provide a tradeoff between the previous two approaches, but it's impossible to get the balance right for every connection scenario.
• Adding a delay to the uploads. Instead of firing all requests at once, we could stagger them somewhat, to attempt to give the server enough time to copy the temporary file over to the final filename. This is not a guaranteed fix, and I'm only really suggesting it as a "best effort" approach to attempt to mitigate the issue if none of the other ideas are viable.

Folks who may be interested

@Mamaduka , who's been looking at the media upload dialog with me and patiently helping me with code reviews @youknowriad , who's kindly been guiding me through the Gutenberg codebase @ellatrix , who likely has the most experience with the media upload dialog, having graciously contributed this great performance feature in the first place

I'm happy to work on a fix myself, but I'd love your opinions on the best approach to solving the issue!

Thank you! 🙏

Step-by-step reproduction instructions

As a race condition, this can be difficult to reproduce. Some environments, such as the WordPress Playground / wp-now seem immune to it; I don't know the technical details there, but I suspect they don't have real concurrency and thus end up with a form of accidental, environment-level synchronisation.

This is how I'm able to reproduce, but your mileage may vary:

• Use wp-env to set up a WordPress environment.
• Use any recent version of WordPress.
• Use Gutenberg trunk, just so we're on the same page, but any version with the post publish upload media dialog should do
• Create a new post and add several image blocks to it, each of them pointing to a different external image (added via "Insert from URL"). Be sure to use external images with no CORS restrictions, that can be fetched by the browser with non-opaque response bodies. One option is to use theme or plugin images in your WordPress install, which still count as external, being outside the media library.
• Hit Publish.
• Hit the upload button in the post publish upload media dialog.
• Ensure all of the images uploaded correctly, by ensuring the post publish upload media dialog goes away.
• Examine the post to see if any of the images incorrectly changed (see video).
• Examine the attachments in the media library to see whether they all point to different filenames (correct), or whether some of them point to the same filename (incorrect).

Screenshots, screen recording, code snippet

Each of the numbers shown is a different external .png image.

https://github.com/user-attachments/assets/522c8118-4b61-4571-93f9-865833762be2

Environment info

• MacOS Sonoma on an Apple Silicon MacBook Pro
• wp-env running WordPress nightly or WordPress 6.6.1 (have tried both)
• Chrome 128 (but it doesn't matter)

Please confirm that you have searched existing issues in the repo.

• [X] Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

• [X] Yes

[sgomes]

Adding a few more folks who may have an opinion here: @ntsekouras @gziolo

Thank you! 🙏

[sgomes]

FYI, I'm leaning towards generating a unique name for each upload on the client side, using a GUID. That seems like a reasonable thing to do, and it should fully resolve the issue.

[ntsekouras]

FYI, I'm leaning towards generating a unique name for each upload on the client side, using a GUID. That seems like a reasonable thing to do, and it should fully resolve the issue.

So right now when we upload from external link the name of the created file is always image? If generating the name client side avoids the WP generated name and resolves the problem, this looks like a good solution to me.

I'd also love some feedback from @swissspidy, since he's working a lot with media.

[swissspidy]

Thanks for looping me in!

I noticed the lack of proper names too when working on the import for just a single image, but didn't think much of it because I already solved it in my project anyway.

Forgot about the same functionality in the pre-publish panel for the whole post. I'll need to implement that in my plugin too :)

The problem is that no name is set at all. It just downloads the image as a Blob and doesn't explicitly set a name. Then image is just a fallback by the browser or WordPress.

In my case, I had solved this by using getFilename() from @wordpress/url to get a file name from the URL and then explicitly create a File object with a proper name. It falls back to unnamed if a file name can't be determined. Though here a UUID would be a more robust fallback.

So if you have https://example.com/some-awesome-image.jpeg in your blog post, it will be imported as some-awesome-image.jpeg. This greatly reduces the risk of race conditions and is also a nicer name than a UUID :). Additionally, my project limits the amount of parallel uploads through client-side thumbnail generation, further reducing that risk of collisions happening on the server.

A few related observations:

• Why does this pre-publish upload dialog only support core/image? In my project I also cover core/media-text, core/cover, and so on. This is quite limiting. In my plugin I support all of those blocks too.
• If I have multiple image blocks with the same image URL, this dialog downloads and uploads the same image multiple times. This is certainly not desired. It would be trivial to only upload it once and still update all image blocks. I do it in my plugin already.

• The MaybeUploadMedia component has a wrong name here:

  https://github.com/WordPress/gutenberg/blob/d179303b68f47c9ed68fb5c5af5a63df743ebb79/packages/editor/src/components/post-publish-panel/maybe-upload-media.js#L61

[sgomes]

Thank you for the feedback, everyone!

I'm happy to use getFilename() to get a meaningful filename instead, yes 👍

We need to consider that it may still lead to collisions, however (/images/folder1/image.jpg, /images/folder2/image.jpg, /images/folder3/image.jpg). We could decide those are not much of a concern and move on, or we could try to be defensive about it on the client side in some way, in which case we'd need to go back to something like appending a GUID (for the cases in which we detect a name collision).

As for your comments, @swissspidy:

Why does this pre-publish upload dialog only support core/image? In my project I also cover core/media-text, core/cover, and so on. This is quite limiting. In my plugin I support all of those blocks too.

Implementing this is exactly how I came across this bug 🙂 I have code sitting in a branch on my machine that will add support for core/media-text and core/cover, but I wanted to solve this issue first, lest it make things worse.

Please let me know if there are any other media-bearing core blocks I'm missing!

If I have multiple image blocks with the same image URL, this dialog downloads and uploads the same image multiple times.

I was hoping to implement this as a follow-up to the PR adding support for other blocks, but I can roll it all into one PR if you feel that's better.

In fact, I'd go one step further and say that ideally we'd also deduplicate images across posts, at least for things like theme images. If a user is fond of a theme pattern with an image and repeatedly uses it in their posts, they'll end up with a ton of copies in the media library if they upload every time. I'm not sure it's possible to implement this in a good way, however.

The MaybeUploadMedia component has a wrong name here

Happy to change that too, as a follow-up 👍

[sgomes]

I decided to roll everything into one PR, since the changes were closely related in places: #65122. Hope this is okay!