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.
This commit is contained in:
ignacionelson
2026-10-05 02:27:18 -03:00
parent a63fea8a4d
commit c77d80309e
3 changed files with 145 additions and 5 deletions
+4 -2
View File
@@ -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 () {
@@ -0,0 +1,93 @@
<?php
declare(strict_types=1);
use App\Models\User;
use App\Modules\Files\Jobs\ImportOrphanFilesJob;
use App\Modules\Files\Models\File;
use App\Modules\Files\OrphanImportProgress;
use Illuminate\Support\Facades\Cache;
use Illuminate\Support\Facades\Log;
use Illuminate\Support\Facades\Storage;
/*
* The background "import all" runs as the staff member who started it, in
* chunks, for as long as it takes. Three edges of that, from the review of
* #1809.
*/
beforeEach(function () {
Storage::fake('files');
$this->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();
});