From c77d80309efe73c76264cc9a2f13eeecde6b019a Mon Sep 17 00:00:00 2001 From: ignacionelson Date: Mon, 5 Oct 2026 02:27:18 -0300 Subject: [PATCH] Harden the background orphan import from #1809 Found in review, none of them reachable in our shipped setups but each cheap to close: - Two chunks could adopt the same path when more than one worker runs the default queue: a run that stalls unblocks a new one after five minutes, and the old chain can resume beside it. Two rows on one set of bytes means deleting either deletes the other's file. Each path is now claimed under a cache lock and checked for a row inside it, so a path another chunk holds is left to it. A lock around the whole chunk was tried first and dropped: a chunk queues the next one while it still holds the lock, so the next one was discarded and the run died. - A chunk now checks that the account that started the run is still active, still staff and still holds import_orphans. A run can outlast that access, and every chunk adopts files in that person's name. - A failure shows a plain sentence and sends the exception to the log. A storage error can name a bucket, an endpoint or a path. --- .../Files/Jobs/ImportOrphanFilesJob.php | 51 +++++++++- tests/Feature/Files/OrphanFilesTest.php | 6 +- .../Files/OrphanImportJobHardeningTest.php | 93 +++++++++++++++++++ 3 files changed, 145 insertions(+), 5 deletions(-) create mode 100644 tests/Feature/Files/OrphanImportJobHardeningTest.php 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(); +});