Skip to content

Rename race condition: Rename to pathname ($newName) that already exists: /var/www/html/site/assets/files/48889/assets.1080x1080-srcset.png.webp #2257

@adrianbj

Description

@adrianbj

Hi @ryancramerdesign - I've been seeing this error with the PageimageSource module (nbcommunication/PageimageSource#8), but it seems like it might need fixing in the core:

Claude's come up with this:

I dug into this and I'm fairly confident it's a benign race condition rather than anything PageimageSource is doing wrong.

The log entries come from WireFileTools::rename() (wire/core/WireFileTools/WireFileTools.php), which logs "Rename to pathname ($newName) that already exists" and returns false whenever the destination already exists. It gets triggered inside Pageimage::size() when generating the srcset variations:

  1. Two requests hit the same page at roughly the same time (visitors, bots, or just multiple images rendering).
  2. Both pass the !$filenameFinalExists check in Pageimage::size() because neither variation exists yet.
  3. Both resize into their own temp file, then both try to rename() temp → final.
  4. The first one wins; the second finds the final already exists, so rename() logs the error and returns false.

Core already anticipates this for the main (non-webp) rename — if the rename fails but the final exists, it silently treats it as "another request won." But WireFileTools::rename() logs the error before returning false, so the files-errors log still fills up. And the webp rename has no race guard at all, which is why we see both the .png and .png.webp entries.

So PageimageSource isn't at fault — it just generates a lot of variations, which increases the odds of concurrent collisions and makes this surface much more often. Chris, your hunch about that area of the module was close, but the dimensions don't actually matter; any concurrently-requested variation can collide.

The clean fix is in core: re-check file_exists() immediately before each rename and discard the temp copy instead of attempting a rename into an existing destination (which is what logs the error). Something like this in Pageimage::size():

  if($sizer->resize($width, $height)) {
      if($options['webpAdd'] && $options['webpOnly']) {
          if(is_file($filenameUnvalidated)) $files->unlink($filenameUnvalidated);
      } else {
          clearstatcache();
          if(file_exists($filenameFinal)) {
              // race condition: another request already created this variation,
              // discard our temp copy rather than attempting a rename that would log an error
              $files->unlink($filenameUnvalidated, true);
          } else if(!$files->rename($filenameUnvalidated, $filenameFinal)) {
              if($files->exists($filenameFinal)) {
                  // potential race condition: another request won
              } else {
                  $this->error = "Rename failed: $filenameUnvalidated => $filenameFinal";
              }
          }
      }
      if($options['webpAdd'] && file_exists($filenameUnvalidatedWebp)) {
          clearstatcache();
          if(file_exists($filenameFinalWebp)) {
              // race condition: another request already created the webp variation
              $files->unlink($filenameUnvalidatedWebp, true);
          } else {
              $files->rename($filenameUnvalidatedWebp, $filenameFinalWebp);
          }
      }
  }

That stops the log spam for both the regular and webp variations in the common case, and the existing $files->exists($filenameFinal) guard still covers the tiny remaining window.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions