diff --git a/app/Modules/Files/Jobs/ImportOrphanFilesJob.php b/app/Modules/Files/Jobs/ImportOrphanFilesJob.php index 46231a8e..66267993 100644 --- a/app/Modules/Files/Jobs/ImportOrphanFilesJob.php +++ b/app/Modules/Files/Jobs/ImportOrphanFilesJob.php @@ -12,6 +12,9 @@ use Illuminate\Bus\Queueable; use Illuminate\Contracts\Queue\ShouldQueue; use Illuminate\Foundation\Bus\Dispatchable; use Illuminate\Queue\InteractsWithQueue; +use App\Modules\Files\Models\File; +use Illuminate\Support\Facades\Cache; +use Illuminate\Support\Facades\Log; use Throwable; /** @@ -55,6 +58,15 @@ class ImportOrphanFilesJob implements ShouldQueue return; } + // Asked at every chunk, not only when the run was started: a run + // can outlast the access of the person who began it, and each + // chunk adopts files in their name. + if (! $user->active || ! $user->isStaff() || ! $user->can('import_orphans')) { + $progress->fail('The account that started the import can no longer import files.'); + + return; + } + $deadline = microtime(true) + $this->budgetSeconds; foreach ($scanner->importable($user, $this->search) as $i => $item) { @@ -65,15 +77,48 @@ class ImportOrphanFilesJob implements ShouldQueue return; } - $importer->import($user, $item['disk'], $item['path']); - $progress->advance(); + if ($this->claimAndImport($importer, $user, $item['disk'], $item['path'])) { + $progress->advance(); + } } $progress->finish(); } + /** + * Adopt one path, unless another chunk has it or already did. + * + * Chunks normally run one after another, but a run that stalled and a + * new one started after it can both have chunks queued, and with more + * than one worker two chunks scanning at once would both adopt the + * same path: two rows on one set of bytes, where deleting either + * deletes the other's file. The scan alone cannot prevent that, since + * hashing a large file leaves seconds between seeing a path and + * writing its row. So each path is claimed first, and checked for a + * row inside the claim. A path somebody else holds is left to them. + */ + private function claimAndImport(OrphanFileImporter $importer, User $user, string $disk, string $path): bool + { + return (bool) Cache::lock('orphan-files-import:'.sha1($disk.'|'.$path), 600)->get(function () use ($importer, $user, $disk, $path): bool { + if (File::withTrashed()->where('disk', $disk)->where('path', $path)->exists()) { + return false; + } + + $importer->import($user, $disk, $path); + + return true; + }); + } + + /** + * The page shows a plain sentence; the exception goes to the log. A + * storage error can name a bucket, an endpoint or a path, which is + * for whoever reads the log rather than for the screen. + */ public function failed(Throwable $exception): void { - app(OrphanImportProgress::class)->fail($exception->getMessage()); + Log::error('The background orphan import failed.', ['exception' => $exception]); + + app(OrphanImportProgress::class)->fail('An error stopped the import. The details are in the application log.'); } } diff --git a/tests/Feature/Files/OrphanFilesTest.php b/tests/Feature/Files/OrphanFilesTest.php index 235a063e..b0a7d714 100644 --- a/tests/Feature/Files/OrphanFilesTest.php +++ b/tests/Feature/Files/OrphanFilesTest.php @@ -296,13 +296,15 @@ test('a run that stops making progress is reported as stalled and no longer bloc Queue::assertPushed(ImportOrphanFilesJob::class, 1); }); -test('a run whose job fails is reported as failed with the reason', function () { +test('a run whose job fails is reported as failed', function () { app(OrphanImportProgress::class)->tryStart(5); (new ImportOrphanFilesJob($this->admin->id, null))->failed(new RuntimeException('Disk unreachable')); + // The reason itself goes to the log, not the screen: see + // OrphanImportJobHardeningTest. $this->actingAs($this->admin)->getJson('/files/orphans/import-status') - ->assertJson(['status' => 'failed', 'error' => 'Disk unreachable']); + ->assertJson(['status' => 'failed']); }); test('import all with nothing importable queues nothing', function () { diff --git a/tests/Feature/Files/OrphanImportJobHardeningTest.php b/tests/Feature/Files/OrphanImportJobHardeningTest.php new file mode 100644 index 00000000..a3e38b1d --- /dev/null +++ b/tests/Feature/Files/OrphanImportJobHardeningTest.php @@ -0,0 +1,93 @@ +staff = staffWithPermissions(['upload', 'import_orphans']); + makeOrphanFile('2026/10/one.txt'); + makeOrphanFile('2026/10/two.txt'); + app(OrphanImportProgress::class)->tryStart(2); +}); + +test('a chunk does not go on for an account that has lost the permission', function () { + $this->staff->role->permissions()->where('permission', 'import_orphans')->delete(); + forgetRequestState(); + + (new ImportOrphanFilesJob($this->staff->id, null))->handle( + app(App\Modules\Files\OrphanFileScanner::class), + app(App\Modules\Files\OrphanFileImporter::class), + app(OrphanImportProgress::class), + ); + + expect(File::query()->count())->toBe(0) + ->and(app(OrphanImportProgress::class)->current()['status'])->toBe('failed'); +}); + +test('a chunk does not go on for an account that was deactivated', function () { + $this->staff->forceFill(['active' => false])->save(); + + ImportOrphanFilesJob::dispatch($this->staff->id, null); + + expect(File::query()->count())->toBe(0) + ->and(app(OrphanImportProgress::class)->current()['status'])->toBe('failed'); +}); + +test('an account that keeps the permission still imports everything', function () { + ImportOrphanFilesJob::dispatch($this->staff->id, null); + + expect(File::query()->count())->toBe(2) + ->and(app(OrphanImportProgress::class)->current()['status'])->toBe('finished'); +}); + +test('a path another chunk is adopting is left to it, and never adopted twice', function () { + // Another chunk holds 2026/10/one.txt right now. + $held = Cache::lock('orphan-files-import:'.sha1('files|2026/10/one.txt'), 600); + expect($held->get())->toBeTrue(); + + ImportOrphanFilesJob::dispatch($this->staff->id, null); + + expect(File::query()->pluck('path')->all())->toBe(['2026/10/two.txt']); + + $held->release(); +}); + +test('a path that gained a row after the scan is skipped', function () { + // Adopted by someone else between this chunk's scan and its import. + $scanner = Mockery::mock(App\Modules\Files\OrphanFileScanner::class); + $scanner->shouldReceive('importable')->andReturn([['disk' => 'files', 'path' => '2026/10/one.txt']]); + File::factory()->create(['disk' => 'files', 'path' => '2026/10/one.txt', 'uploaded_by' => $this->staff->id]); + + (new ImportOrphanFilesJob($this->staff->id, null))->handle( + $scanner, + app(App\Modules\Files\OrphanFileImporter::class), + app(OrphanImportProgress::class), + ); + + expect(File::query()->where('path', '2026/10/one.txt')->count())->toBe(1); +}); + +test('a failure shows a plain message and keeps the details for the log', function () { + Log::spy(); + + (new ImportOrphanFilesJob($this->staff->id, null)) + ->failed(new RuntimeException('Error executing "PutObject" on "https://bucket.s3.example/secret-path"')); + + $error = app(OrphanImportProgress::class)->current()['error']; + + expect($error)->not->toContain('bucket')->not->toContain('PutObject'); + Log::shouldHaveReceived('error')->once(); +});