Conversions aren't always generated when queued and running parallel

[royduin]

We're having weird issues for a while where some conversions aren't generated. Hard te reproduce because it doesn't happen every time. On a model we do have 3 conversions; mini, thumb and detail. Mini isn't queued and is always generated. The other 2 are queued. In code:

public function registerMediaConversions(?Media $media = null): void
{
    $this->addMediaConversion('mini')
        ->fit(Fit::Crop, 100, 100)
        ->keepOriginalImageFormat()
        ->nonQueued();

    $this->addMediaConversion('thumb')
        ->fit(Fit::Crop, 360, 360)
        ->keepOriginalImageFormat();

    $this->addMediaConversion('detail')
        ->fit(Fit::Crop, 720, 720)
        ->keepOriginalImageFormat();
}

This in combination with Laravel Horizon and QUEUE_CONNECTION=redis and multiple workers:

'supervisor-1' => [
    'connection' => 'redis',
    'queue' => ['default'],
    'balance' => 'simple',
    'processes' => 5,
    'tries' => 3,
],

Does sometimes result in just {"mini": true} in the generated_conversions column instead of {"mini": true, "thumb": true, "detail": true}. The actual images aren't generated as well! Resulting in broken images in the frontend. In those cases the PerformConversionsJob job has a runtime of 0.00s in Horizon.

Running php artisan media-library:regenerate --only-missing does generate them but it should generate them directly.

I added some debugging and it seems like it's caused by a directory which is deleted which shouldn't happen. Related: #3630 but in another file;

laravel-medialibrary/src/Conversions/FileManipulator.php

Line 53 in bce761d

$temporaryDirectory = TemporaryDirectory::create();

here the temp directory is created and the whole directory will be removed at the end:

laravel-medialibrary/src/Conversions/FileManipulator.php

Line 74 in bce761d

$temporaryDirectory->delete();

The temp directory should be unique:

laravel-medialibrary/src/Support/TemporaryDirectory.php

Line 19 in bce761d

return $path.DIRECTORY_SEPARATOR.Str::random(32);

but when I add some logging here:

laravel-medialibrary/src/Conversions/Actions/PerformManipulationsAction.php

Line 45 in bce761d

return $conversionTempFile;

with logger($conversionTempFile); I'm getting:

/storage/media-library/temp/tzFZaU0bIYOVyG6fHdsSsmhnQujw6SxO/dz2O6rgEYt9cGXChnDt7GPtDIFiVZMDBdetail.jpg  
/storage/media-library/temp/P5vMHGcr60fGEBYUe5RHHKZ2vX7WepAT/zbunK1fPBOBLR5ffXRT1tyuxT3Hbv0Wkmini.jpg  
/storage/media-library/temp/4g76pGzlPIjm0eACMogyWsr93EXfdZlB/34cEGcRdwMs7l8jhx8JCc85Be7aUK8Gwmini.jpg  
/storage/media-library/temp/wNHNmdRzBYHHTyVOcEoS7o4j1SCwH9Hx/lbr8LQhqmCTbMI070RACkQfpJPWOXFn0mini.jpg  
/storage/media-library/temp/jKiPsdVzDtg5YOZqq9jTMI1nL6W6Kzye/rcfAnJN3UCF614qR8ZRqV1ozLQtkg3Epmini.jpg  
/storage/media-library/temp/rRsK4aWDoYP7ZEGgV1Or9JuFoS29m6hM/F6hlNEa9wHN43pAs1Alv0kj0bEGUHKv6thumb.jpg    
/storage/media-library/temp/kNCsZApUFbTIQTLlBnta7zVHdnkcVeEl/XjngrkVf98JpowpSfxYCgUV7s3z093wgthumb.jpg    
/storage/media-library/temp/rRsK4aWDoYP7ZEGgV1Or9JuFoS29m6hM/5MHSd1uc5fRuyTv20TdoCziygP0Z54jhdetail.jpg  
/storage/media-library/temp/kNCsZApUFbTIQTLlBnta7zVHdnkcVeEl/YD4x5nYMqxDmzvNtvu46X0GubvHei7mfdetail.jpg  

Where kNCsZApUFbTIQTLlBnta7zVHdnkcVeEl and rRsK4aWDoYP7ZEGgV1Or9JuFoS29m6hM are used twice.

So getTemporaryDirectoryPath() isn't generating unique directories... can we make that truly unique somehow? Or should be check if it's the only file in directory and only delete it in that case?

To reproduce it simply use the same directory here:

laravel-medialibrary/src/Support/TemporaryDirectory.php

Line 19 in bce761d

return $path.DIRECTORY_SEPARATOR.Str::random(32);

with for example:

return $path.DIRECTORY_SEPARATOR.'123';

[royduin]

I'm not the only one: https://laracasts.com/discuss/channels/laravel/repeated-values-from-strrandom

[royduin]

We're switching to a queue with 1 worker for now:

config/horizon.php

'images' => [
    'connection' => 'redis',
    'queue' => ['images'],
    'balance' => 'simple',
    'processes' => 1,
    'tries' => 3,
],

config/media-library.php

'queue_name' => 'images',

[chrispage1]

Ah, I've just raised #3677 which seems to be a duplicate of this. We get the exact same where the run time is 0 seconds exactly and no conversions generated, yet still marked as completed.

@royduin Hows just running a single process going for you?

[royduin]

It still occurs, but less frequent. I think because workers are still reused which somehow cause "duplicated random values".

[chrispage1]

Thanks for your research on this. It's interesting that Str::random() is generating duplicate values on Horizon and as you've pointed out this seems to be an issue that has been raised elsewhere.

I'm just updating a project to run a fork where I've added a prefix value to the temporary path generator. This is the only way I can think that'd ensure uniqueness in this situation - without having to delve deeper into the true issue at this point in time.

I'll let you know how it goes and if it works I'll PR it over.

[chrispage1]

Ok, so from further investigation... the jobs are dispatched that isn't a problem.

$deleteWhenMissingModels = true on the job is obscuring the issue, hence why the job gets marked as complete after 0 seconds.

Logging out inside the job I'm able to frequently replicate this locally. The job is dispatched but the media item doesn't appear to be in the database at the moment the conversion is created.

I've checked if we're being held within a database transaction but that doesn't seem to be the case, and even directly querying the database (using DB::table('media')) the count is returning zero at that point in time.

Still looking into it...

[chrispage1]

Of course - that was the clue and I suspect your case may be similar @royduin

So, we're running Laravel Nova. Some of our posts we're uploading 20+ large-ish files.

Nova wraps the post creation within a database transaction, meaning that until the post is created, the Media models remain within the transaction meaning Horizon has no way of actually looking them up in the database.

That is until the request has completed and the database transactions committed.

So as our files are being uploaded, the database record is still stuck within a transaction and the job immediately fails because $deleteWhenMissingModels is set to true by default on the job.

The solution for this is to ensure that every job on Horizon fires after all database commits by adding after_commit to your queue.connections.redis config, OR to ensure that the FileManipulator only dispatches the job after a commit.

I'll have a bit of a look and see if there are any implications to adding this change...

[chrispage1]

It looks like this has also been raised in the past: #1304

[chrispage1]

Raised a PR to address this #3687

[royduin]

Wow, nice! We're going to test this 🚀

[chrispage1]

Wow, nice! We're going to test this 🚀

Let me know how it goes!

[chrispage1]

P.s. you'll be able to throw as many workers and processes as you want at it again :)

[royduin]

So far so good 🤞🏻

[francoism90]

I don't think this setup works, because you'll update the models possibly in parallel. So they can be hit/updated at random.

You'll indeed need to set afterCommit() in your queues and/or other places, but also combine this with chaining when you want to ensure it doesn't get called simultaneously.