spatie / laravel-medialibrary

Associate files with Eloquent models
https://spatie.be/docs/laravel-medialibrary
MIT License
5.73k stars 1.07k forks source link

Temporary folders are not deleted after upload #220

Closed philmarc closed 7 years ago

philmarc commented 8 years ago

This happens on my localhost as well as on my production app: When uploading a file, a randomly named folder is created in storage\medialibrary\temp, containing the file. Once the upload is completed, the file is removed from that folder, but the folder itself remains.

The permissions on that folder is 0705 and the permission on the previous folders (storage, medialibrary, and temp) are all 0755.

This only happend since I added a new media conversion to my Model, $this->addMediaConversion('admin')

freekmurze commented 8 years ago

Thanks for reporting this. To me this doesn't seem a big problem. It's also a bit unsafe to remove the directory after the conversion, as queued conversions may also be busy using the folder.

philmarc commented 8 years ago

So this is the natural behaviour not to delete those folders? I guess I'll have to schedule a job to clean the temp folder every day or so, because I have hundreds of folders created everyday, and I feel it's a bit silly to keep those empty folders.

freekmurze commented 8 years ago

Ah sorry, I didn't read your original issue correctly. Your issue is with subfolders within temp not with temp itself. I'll take a look at the deletion of the subfolders, they should be cleaned up.

philmarc commented 8 years ago

It might have to do with my host since my sessions files in storage/framework/sessions are not getting cleaned either by the PHP garbage collector.

Edit: This was a mistake, my session files are actually getting cleaned.

philmarc commented 8 years ago

The temporary folders created in storage\medialibrary\temp all have a permission of 705 when created. Shouldn't it be 604?

I can't find out what is wrong, because the temporary uploaded file inside the temporary folder (ie: temp\KwK2sNOckXgfszLs) is successfully getting cleaned (or moved) after upload has completed. It is just that folder KwK2sNOckXgfszLs that remains...

Am I the only one experiencing this issue?

freekmurze commented 8 years ago

I can't seem to replicate this issue. Deleting the temp directory removes the all files including the directory itself.

ergonomicus commented 7 years ago

@PhilMarc I have bunch of folders inside temp folder also. All of them have some file with jpg extension, but I can not open them. This is happening on both local machine and in production. Have you figured out what's causing this issue?

bradcis commented 7 years ago

This is occurring for me as well. /storage/medialibrary/temp is filled with files, even though the conversions have already been completed. Any thoughts, @freekmurze? Should I create a new issue for this?

freekmurze commented 7 years ago

I'll reopen this issue. I'm not able to recreate the issue myself. I'd greatly appreciate if anyone post some code / any info on how to reliably replicate this issue.

philmarc commented 7 years ago

I believe the error occurs only when using the S3 filesystem.

On localhost as well as on production server,

On upload, I get the following error (on local environment only) - I don't think it's related to the problem but here it is just in case - :

ErrorException in FileAdder.php line 345: 
unlink(C:\xampp\tmp/php4693.tmp): Permission denied

I have checked the C:\xampp\tmp folder, and files are actually getting cleaned after the script run.

// FileAdder.php line 345

        if (! $this->preserveOriginal) {
            unlink($this->pathToFile);
        }
bradcis commented 7 years ago

I'm using S3 and have conversions enabled. I've run into problems where I haven't had memory_limit high enough (it was set to 128M). If a large image is uploaded using $photo->addMediaFromRequest('photo')->toMediaLibrary(); and there isn't enough memory allocated, the original file is uploaded S3 but the conversions fail with an out of memory error. In these cases, it doesn't appear that the temp subfolders are removed, even if the conversions succeed in a future attempt.

barryvanveen commented 7 years ago

When I try to run the tests for this package I get some errors which I think might be related to the temp files discussed in this issue.

I execute vendor/bin/phpunit tests/S3Integration/S3IntegrationTest.php because this contains the tests that fail. It will trigger some errors like "ErrorException: mkdir(): File exists". After some digging around I found the real errors are obscured because /Illuminate/Filesystem/Filesystem.php uses @ unlink and @ rmdir which suppresses the more readable errors.

It turns out that I get 2 different errors:

  1. The first is always something along the lines of "ErrorException: rmdir(/home/localcopy/laravel-medialibrary/vendor/orchestra/testbench/src/Traits/../../fixture/storage/medialibrary/temp/HSkg49x52HCGR9RP): Directory not empty". 2, The second error (and all errors after that) are "ErrorException: rmdir(/home/localcopy/laravel-medialibrary/tests/temp/testfiles): Directory not empty".

The problem seems to be part of deleteDirectory in /Illuminate/Filesystem/Filesystem.php. The weirdest thing is that rmdir fails because the directory is not empty. It can see a file 'thumb.jpg' in the temp folder. While unlink should have deleted that file it is sure that the file does not exist...

So @PhilMarc, it looks like this is related to S3, somehow. And @bradcis, my development machine has a memory limit of 1024M which I think should be more than enough to rule out a memory error.

freekmurze commented 7 years ago

Kinda curious when the tests in that file fail on your machine. On my local machine and on Travis the tests regarding S3 are just running fine.

barryvanveen commented 7 years ago

It might have something to do with working on a virtual machine. I've tested it at work (a custom Ubuntu-vm on a Windows host) and at home (Laravel homestead on a Windows host) and both have these problems. Seems to be some sort of race condition at the file system level...

freekmurze commented 7 years ago

This problem should be solved in v5. Reopen if the problem still persists in that version.

eaguad1337 commented 6 years ago

Did not know why my server was running out of inodes and I discovered that temp folder is full of files, I'm using local driver in a production server.

/dev/vda1 20643840 20643840 0 100% /

Using laravel-medialibrary 6.9.0 and Laravel v5.5.40. I'm deleting the files but how can I prevent this happen again?

mira-thakkar commented 6 years ago

@eaguad1337 i am getting the same issue, my server was running with 'no space on device" issue, while i check for the space available 70% space was consumed by /tmp directory with media files. Do you find solution for this ?

ergonomicus commented 6 years ago

@mira-thakkar I was in a similar situation and I wasn't able to find the solution, so I decided to make a command to empty the content of that folder, and scheduled it to run weekly. I know it's not the solution, but this hack seems to work for me for a long time now. It's really simple, but If you need it I can find the code and paste it here.

Kyawzino commented 5 years ago

In the temp folder files imgs are not showing public folder. How to fix this problem.

Shandur commented 5 years ago

@freekmurze Hey! Have the same problem with "no space" due to /tmp folder filled with media{HASH} files. I'm using s3 buckets to store files from URL but they seem not cleaned up :\ Don't you know what's going? Thanks!!

Shandur commented 5 years ago

@freekmurze Maybe this code is an issue? vendor/spatie/laravel-medialibrary/src/FileAdder/FileAdder.php:312

        if (! $fileAdder->preserveOriginal) {
            unlink($fileAdder->pathToFile);
        }

Perhaps, there should be the following code?

$this->filesystem->removeFile($media, $fileAdder->pathToFile);
devinfd commented 4 years ago

This issue still exists

imannms commented 4 years ago

I am getting the same issue. Medialibrary store images into /tmp directory and never got cleaned up. The same as storage/medialibrary/temp directory, it never got cleaned up too.

So, we have double temporary directories that never got cleaned up.

  1. /tmp in root directory
  2. storage/medialibrary/temp

I'm using Digital Ocean with S3 driver.

mehranhadidi commented 3 years ago

Same issue for me too. When I use the S3 as the default driver, the local temp directory won't get cleaned.

devinfd commented 3 years ago

5 years later this issue still exists. Did anyone find a solution to this?

mbuk commented 3 years ago

I'm seeing the same issue with the s3 driver

ghost commented 2 years ago

Any update?

krystianlaubach commented 2 years ago

I have set up separate local disk for temporary uploads.

.env
TEMP_UPLOADS=temp_uploads
config/filesystems.php
'temp_uploads' => [
    'driver' => 'local',
    'root' => storage_path('app/public/tmp'),
    'url' => env('APP_URL').'/storage/tmp',
    'visibility' => 'private',
],
config/media-library.php
'temp_disk_name' => env('TEMP_UPLOADS', 'public'),

I have also my own model for temporary uploads and I override getDiskName method:

<?php

namespace App\Models;

use Spatie\MediaLibraryPro\Models\TemporaryUpload as SpatieTemporaryUpload;

class TemporaryUpload extends SpatieTemporaryUpload
{
    /**
     * @return string
     */
    protected static function getDiskName(): string
    {
        return static::$disk ?? config('media-library.temp_disk_name');
    }
}

Then I've set up my own DeleteTemporaryUploadsCommand that runs every midnight to remove "old" TemporaryUploads but also folders from temp_upload disk

<?php

namespace App\Console\Commands;

use App\Models\TemporaryUpload;
use Illuminate\Console\Command;
use Illuminate\Support\Facades\Storage;

class DeleteTemporaryUploadsCommand extends Command
{
    /**
     * @var string
     */
    protected $signature = 'media-library:delete-old-temporary-uploads-and-folders';

    /**
     * @var string
     */
    protected $description = 'Delete old temporary uploads';

    public function handle(): void
    {
        $this->deleteTemporaryUploads();
        $this->deleteTemporaryFolders();
    }

    private function deleteTemporaryUploads(): void
    {
        $this->info('Start removing old temporary uploads...');

        $temporaryUploads = TemporaryUpload::old()->get();
        $temporaryUploads->each(fn (TemporaryUpload $temporaryUpload) => $temporaryUpload->delete());

        $this->comment($temporaryUploads->count() . ' old temporary upload(s) deleted!');
    }

    private function deleteTemporaryFolders(): void
    {
        $this->info('Start removing old temporary folders...');

        $temporaryUploadDisk = Storage::disk(config('media-library.temp_disk_name'));
        $temporaryFolders = collect($temporaryUploadDisk->allDirectories());
        $temporaryFolders->each(fn (string $directory) => $temporaryUploadDisk->deleteDirectory($directory));

        $this->comment($temporaryFolders->count() . ' old temporary folder(s) deleted!');
    }
}
aaronmeder commented 2 years ago

Can report that we also had the same issue with the "temp" folder of the medialibrary filling up our / disk - should have had oh dear active I guess 😬

Interestingly the directories that stayed within "temp" were created irregularly - sometimes there are multiple months and then again some folders created on a specific date. I will try to check other logs regarding these days...maybe there is a helpful pattern to discover.

akhzarjaved commented 1 year ago

Still facing same issue when using s3 driver. Anyone ever fixed this? When using public driver the temp files are stored in storage\media-library\temp and gets cleaned + gets uploaded (Works fine) When using s3 driver the temp files are stored in /tmp and gives error Unable to read image from path shouldn't the path for s3 be same as public ?

Example: storage\media-library\temp

AlejandroAkbal commented 1 year ago

I'm having the same issue with S3

akhzarjaved commented 1 year ago

I'm having the same issue with S3

@AlejandroAkbal I solved the issue. The issue was regarding the exception throw. To solve the actual issue set 'throw' => true in filesystem.php and then you'll get a proper exception.

In my case it was permission issue on s3

AlejandroAkbal commented 1 year ago

I'm having the same issue with S3

@AlejandroAkbal I solved the issue. The issue was regarding the exception throw. To solve the actual issue set 'throw' => true in filesystem.php and then you'll get a proper exception.

In my case it was permission issue on s3

Hey, thanks for getting back, unfortunately I already have set throw to true in the filesystem disk config

And I'm still facing the issue :(

JmRy commented 1 year ago

Hello @freekmurze

The files contained in the temporary directory remain if the image conversion fail. (See the failed jobs with Laravel-horizon)

I solved this issue using the Spatie\TemporaryDirectory\TemporaryDirectory library. (Included in media-library)

2 lines of code inserted in a sheduled command

<?php

namespace App\Console\Commands;

use Illuminate\Console\Command;
use Spatie\TemporaryDirectory\TemporaryDirectory as BaseTemporaryDirectory;

class DeleteTemporaryUploads extends Command
{
    /**
     * The name and signature of the console command.
     *
     * @var string
     */
    protected $signature = 'media-library:purge-temporary-folders';

    /**
     * The console command description.
     *
     * @var string
     */
    protected $description = 'Supprime les répertoires temporaires créés par Media-Library';

    /**
     * Execute the console command.
     *
     * @return int
     */
    public function handle()
    {
        return $this->deleteTemporaryFolders() ? Command::SUCCESS : Command::FAILURE;
    }

  private function deleteTemporaryFolders(): bool
  {
    $this->info('Start removing old Media-library temporary folders...');

    $temporaryDirectory = new BaseTemporaryDirectory(config('media-library.temporary_directory_path') ?? storage_path('media-library/temp'));

    $return = $temporaryDirectory->delete();

    $this->comment('Old temporary folder(s) deleted!');

    return $return;
  }
}

Note that the 'temp' directory is automatically recreated during the next conversion.

Works very well under Laravel 9 with "spatie/laravel-medialibrary": "^10.0"

😀