mirror of
https://github.com/projectsend/projectsend.git
synced 2026-09-11 22:38:54 +00:00
Give each parallel test worker its own directory for upload parts
A full parallel run failed once and passed on retry while I was doing the #1703 follow-up. A flake is worse than a steady failure: it trains you to re-run rather than look, and it quietly weakens every green run reported beside it. Upload parts are real files under storage_path('app/uploads-tmp/{session_id}'), not a faked disk. Every parallel worker gets its own database, so session ids restart at 1 in each of them, and two workers writing parts land in the same directory. On top of that ChunkedUploadsTest's afterEach deleted the whole tree rather than its own share, for everybody. Six test files write parts, so this was reachable without anything I added. The same collision exists inside one worker: RefreshDatabase rolls back, so ids restart at 1 for every test, and a run that died before its cleanup leaves parts sitting under the id the next test is about to claim. LocalPartStore now reads its root from config, defaulting to exactly where it always was -- an installation with UPLOAD_PARTS_PATH unset behaves identically. Tests\TestCase points it at a per-worker directory and empties that directory per test, which closes the cross-worker, the cross-run and the intra-worker versions together. ChunkedUploadsTest's cleanup and its two directory assertions read the configured root rather than the hardcoded path, so they can no longer reach into a neighbour. Verified with eight consecutive parallel runs, green, and by watching the per-worker directories appear separately (w1, w2, w4 … w14) rather than one shared tree. The isolation itself cannot be asserted from inside a single test; what a test can pin is the mechanism it rests on, so one does: parts go where the configured root says.
This commit is contained in:
@@ -255,7 +255,26 @@ class LocalPartStore
|
||||
|
||||
private function directory(UploadSession $session): string
|
||||
{
|
||||
return storage_path('app/uploads-tmp/'.$session->id);
|
||||
return $this->root().'/'.$session->id;
|
||||
}
|
||||
|
||||
/**
|
||||
* Where part files live while a transfer is in progress.
|
||||
*
|
||||
* Configurable only so the test suite can hold it apart per parallel
|
||||
* worker. This is a real directory rather than a faked disk, and each
|
||||
* worker's database restarts session ids at 1, so two workers writing
|
||||
* parts land in the same place — and ChunkedUploadsTest's afterEach
|
||||
* deletes the whole tree, for everybody. Unset, which is every
|
||||
* installation, the path is what it has always been.
|
||||
*/
|
||||
private function root(): string
|
||||
{
|
||||
$configured = config('projectsend.uploads.parts_path');
|
||||
|
||||
return is_string($configured) && $configured !== ''
|
||||
? rtrim($configured, '/')
|
||||
: storage_path('app/uploads-tmp');
|
||||
}
|
||||
|
||||
private function partPath(UploadSession $session, int $partNumber): string
|
||||
|
||||
@@ -19,6 +19,23 @@ return [
|
||||
|
||||
'edition' => Edition::from((string) env('PROJECTSEND_EDITION', 'community')),
|
||||
|
||||
/*
|
||||
|--------------------------------------------------------------------------
|
||||
| Uploads
|
||||
|--------------------------------------------------------------------------
|
||||
|
|
||||
| Where a chunked upload's parts wait while the transfer is running,
|
||||
| before they are assembled onto the storage disk. Leave this unset:
|
||||
| it exists so the test suite can give each parallel worker its own
|
||||
| directory, since parts are real files on a real path rather than a
|
||||
| faked disk, and session ids restart at 1 in every worker's database.
|
||||
|
|
||||
*/
|
||||
|
||||
'uploads' => [
|
||||
'parts_path' => env('UPLOAD_PARTS_PATH'),
|
||||
],
|
||||
|
||||
/*
|
||||
|--------------------------------------------------------------------------
|
||||
| Chunked upload part size (MB)
|
||||
|
||||
@@ -15,6 +15,17 @@ use App\Modules\Platform\Settings\Settings;
|
||||
use Illuminate\Support\Facades\Storage;
|
||||
use Illuminate\Testing\TestResponse;
|
||||
|
||||
/**
|
||||
* Where this worker's parts live. Not storage_path('app/uploads-tmp')
|
||||
* directly: each parallel worker gets its own root (see Tests\TestCase),
|
||||
* because session ids restart at 1 in every worker's database and the
|
||||
* cleanup below would otherwise delete the others' parts mid-test.
|
||||
*/
|
||||
function partsRoot(): string
|
||||
{
|
||||
return (string) config('projectsend.uploads.parts_path');
|
||||
}
|
||||
|
||||
function grantChunkedUploadPermission(User $user): void
|
||||
{
|
||||
RolePermission::query()->firstOrCreate(['role_id' => $user->role_id, 'permission' => Permission::Upload->value]);
|
||||
@@ -26,7 +37,7 @@ beforeEach(function () {
|
||||
});
|
||||
|
||||
afterEach(function () {
|
||||
Illuminate\Support\Facades\File::deleteDirectory(storage_path('app/uploads-tmp'));
|
||||
Illuminate\Support\Facades\File::deleteDirectory(partsRoot());
|
||||
});
|
||||
|
||||
function createSession(int $size = 1024, string $filename = 'big.zip'): string
|
||||
@@ -103,7 +114,7 @@ test('complete assembles parts in order into a verified File record', function (
|
||||
->and($file->checksum)->toBe(hash('sha256', 'hello-world'))
|
||||
->and(Storage::disk('files')->get($file->path))->toBe('hello-world')
|
||||
->and(UploadSession::query()->find($sessionId))->toBeNull()
|
||||
->and(is_dir(storage_path('app/uploads-tmp/'.$sessionId)))->toBeFalse()
|
||||
->and(is_dir(partsRoot().'/'.$sessionId))->toBeFalse()
|
||||
->and(ActivityLog::query()->where('action', Action::FileUploaded)->where('subject_name', 'assembled')->exists())->toBeTrue();
|
||||
});
|
||||
|
||||
@@ -192,7 +203,7 @@ test('abort deletes parts and the purge command clears only stale sessions', fun
|
||||
$aborted = createSession();
|
||||
putPart($aborted, 1, 'data');
|
||||
$this->deleteJson("/uploads/{$aborted}")->assertNoContent();
|
||||
expect(is_dir(storage_path('app/uploads-tmp/'.$aborted)))->toBeFalse()
|
||||
expect(is_dir(partsRoot().'/'.$aborted))->toBeFalse()
|
||||
->and(UploadSession::query()->find($aborted))->toBeNull();
|
||||
|
||||
$fresh = createSession(100, 'fresh.zip');
|
||||
@@ -332,3 +343,21 @@ test('a folder that survives the upload still receives the file', function () {
|
||||
|
||||
expect(File::query()->whereKey($fileId)->value('folder_id'))->toBe($folder->id);
|
||||
});
|
||||
|
||||
// The isolation itself cannot be observed from inside one test, but the
|
||||
// mechanism it rests on can: parts go where the configured root says, so
|
||||
// giving each worker its own root actually holds them apart.
|
||||
test('parts are written under the configured root', function () {
|
||||
$this->actingAs($this->admin);
|
||||
|
||||
$custom = storage_path('app/uploads-tmp/somewhere-else');
|
||||
config(['projectsend.uploads.parts_path' => $custom]);
|
||||
|
||||
$sessionId = createSession(1024);
|
||||
putPart($sessionId, 1, 'hello')->assertOk();
|
||||
|
||||
expect(is_file($custom.'/'.$sessionId.'/1.part'))->toBeTrue()
|
||||
->and(is_dir(storage_path('app/uploads-tmp/'.$sessionId)))->toBeFalse();
|
||||
|
||||
Illuminate\Support\Facades\File::deleteDirectory($custom);
|
||||
});
|
||||
|
||||
+30
-1
@@ -3,8 +3,37 @@
|
||||
namespace Tests;
|
||||
|
||||
use Illuminate\Foundation\Testing\TestCase as BaseTestCase;
|
||||
use Illuminate\Support\Facades\File;
|
||||
use Illuminate\Support\Facades\ParallelTesting;
|
||||
|
||||
abstract class TestCase extends BaseTestCase
|
||||
{
|
||||
//
|
||||
/**
|
||||
* Give each parallel worker its own directory for upload parts.
|
||||
*
|
||||
* Parts are real files under a real path, not a faked disk, and every
|
||||
* worker's database restarts session ids at 1 — so two workers writing
|
||||
* parts land in the same directory, and ChunkedUploadsTest's afterEach
|
||||
* deletes the whole tree for all of them. That showed up as a
|
||||
* one-run-in-three failure, which is worse than a steady one: it
|
||||
* trains you to re-run rather than look.
|
||||
*
|
||||
* Outside a parallel run the token is null and everything lands under
|
||||
* w0, which is still held apart from whatever a previous run left.
|
||||
*/
|
||||
protected function setUp(): void
|
||||
{
|
||||
parent::setUp();
|
||||
|
||||
$root = storage_path('app/uploads-tmp/w'.(ParallelTesting::token() ?: '0'));
|
||||
|
||||
config(['projectsend.uploads.parts_path' => $root]);
|
||||
|
||||
// Emptied per test, not just per file. RefreshDatabase rolls back,
|
||||
// so session ids restart at 1 in every test — two tests in the
|
||||
// same worker reuse the same directory name, and a run that
|
||||
// crashed before its cleanup leaves the previous one's parts
|
||||
// sitting there under the id the next test is about to claim.
|
||||
File::deleteDirectory($root);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user