Added simple lock mutex to importer to prevent duplicate imports

app/Http/Controllers/Api/ImportController.php

@@ -11,6 +11,7 @@ use Illuminate\Database\Eloquent\JsonEncodingException;
 use Illuminate\Http\JsonResponse;
 use Illuminate\Http\UploadedFile;
 use Illuminate\Support\Facades\Artisan;
+use Illuminate\Support\Facades\DB;
 use Illuminate\Support\Facades\Log;
 use Illuminate\Support\Facades\Request;
 use Illuminate\Support\Facades\Session;
@@ -291,72 +292,123 @@ class ImportController extends Controller
             return response()->json(Helper::formatStandardApiResponse('import-errors', null, $error), 500);
         }
 
-        $errors = $request->import($import);
-        $redirectTo = 'hardware.index';
-        switch ($request->input('import-type')) {
-            case 'asset':
-            case 'assetHistory':
-                $model_perms = 'App\Models\Asset';
-                $redirectTo = 'hardware.index';
-                break;
-            case 'assetModel':
-                $model_perms = 'App\Models\AssetModel';
-                $redirectTo = 'models.index';
-                break;
-            case 'accessory':
-                $model_perms = 'App\Models\Accessory';
-                $redirectTo = 'accessories.index';
-                break;
-            case 'consumable':
-                $model_perms = 'App\Models\Consumable';
-                $redirectTo = 'consumables.index';
-                break;
-            case 'component':
-                $model_perms = 'App\Models\Component';
-                $redirectTo = 'components.index';
-                break;
-            case 'license':
-                $model_perms = 'App\Models\License';
-                $redirectTo = 'licenses.index';
-                break;
-            case 'user':
-                $model_perms = 'App\Models\User';
-                $redirectTo = 'users.index';
-                break;
-            case 'location':
-                $model_perms = 'App\Models\Location';
-                $redirectTo = 'locations.index';
-                break;
-            case 'supplier':
-                $model_perms = 'App\Models\Supplier';
-                $redirectTo = 'suppliers.index';
-                break;
-            case 'manufacturer':
-                $model_perms = 'App\Models\Manufacturer';
-                $redirectTo = 'manufacturers.index';
-                break;
-            case 'category':
-                $model_perms = 'App\Models\Category';
-                $redirectTo = 'categories.index';
-                break;
+        // Per-import processing mutex. Two calls into process() for the
+        // same import (two admins clicking at once, a double-fire from the
+        // wizard, a browser retry, an intermediate proxy retry) would each
+        // run their own snapshot of the app-layer unique-validation
+        // checks, both find no live duplicates, and both insert - producing
+        // duplicate rows that the downstream unique_undeleted rule can't
+        // retroactively resolve. This UPDATE is an atomic compare-and-set:
+        // only one caller wins per Import row for the duration of that
+        // request. The lock is released at the end of the request (both
+        // success and error paths, see the finally-shaped block below) so
+        // legitimate sequential slices from the SAME caller can fire the
+        // next slice against a released lock. A stale lock from a crashed
+        // slice self-heals after 5 minutes (timeout branch). See
+        // ImportConcurrencyTest for the acquire / release / stale-takeover
+        // assertions.
+        $now = now();
+        $acquired = DB::table('imports')
+            ->where('id', $import_id)
+            ->where(function ($q) use ($now) {
+                $q->whereNull('processing_by')
+                    ->orWhere('processing_started_at', '<', $now->copy()->subMinutes(5));
+            })
+            ->update([
+                'processing_by' => auth()->id(),
+                'processing_started_at' => $now,
+            ]);
+
+        if ($acquired === 0) {
+            return response()->json(Helper::formatStandardApiResponse(
+                'error',
+                null,
+                trans('admin/hardware/message.import.already_processing')
+            ), 409);
         }
 
-        $tally = $request->getTally();
-        // Payload only carries the tally when at least one importer for this
-        // type has been wired up to record it. Un-instrumented importers
-        // leave every count at zero; suppress the block in that case so we
-        // don't surface a misleading all-zero summary in the wizard.
-        $tallyPayload = array_sum($tally) > 0 ? ['tally' => $tally] : null;
+        try {
+            $errors = $request->import($import);
+            $redirectTo = 'hardware.index';
+            switch ($request->input('import-type')) {
+                case 'asset':
+                case 'assetHistory':
+                    $model_perms = 'App\Models\Asset';
+                    $redirectTo = 'hardware.index';
+                    break;
+                case 'assetModel':
+                    $model_perms = 'App\Models\AssetModel';
+                    $redirectTo = 'models.index';
+                    break;
+                case 'accessory':
+                    $model_perms = 'App\Models\Accessory';
+                    $redirectTo = 'accessories.index';
+                    break;
+                case 'consumable':
+                    $model_perms = 'App\Models\Consumable';
+                    $redirectTo = 'consumables.index';
+                    break;
+                case 'component':
+                    $model_perms = 'App\Models\Component';
+                    $redirectTo = 'components.index';
+                    break;
+                case 'license':
+                    $model_perms = 'App\Models\License';
+                    $redirectTo = 'licenses.index';
+                    break;
+                case 'user':
+                    $model_perms = 'App\Models\User';
+                    $redirectTo = 'users.index';
+                    break;
+                case 'location':
+                    $model_perms = 'App\Models\Location';
+                    $redirectTo = 'locations.index';
+                    break;
+                case 'supplier':
+                    $model_perms = 'App\Models\Supplier';
+                    $redirectTo = 'suppliers.index';
+                    break;
+                case 'manufacturer':
+                    $model_perms = 'App\Models\Manufacturer';
+                    $redirectTo = 'manufacturers.index';
+                    break;
+                case 'category':
+                    $model_perms = 'App\Models\Category';
+                    $redirectTo = 'categories.index';
+                    break;
+            }
 
-        if ($errors) { // Failure
-            return response()->json(Helper::formatStandardApiResponse('import-errors', $tallyPayload, $errors), 500);
+            $tally = $request->getTally();
+            // Payload only carries the tally when at least one importer for this
+            // type has been wired up to record it. Un-instrumented importers
+            // leave every count at zero; suppress the block in that case so we
+            // don't surface a misleading all-zero summary in the wizard.
+            $tallyPayload = array_sum($tally) > 0 ? ['tally' => $tally] : null;
+
+            if ($errors) { // Failure
+                return response()->json(Helper::formatStandardApiResponse('import-errors', $tallyPayload, $errors), 500);
+            }
+            // Flash message before the redirect
+            Session::flash('success', trans('admin/hardware/message.import.success'));
+
+            $redirect_url = auth()->user()->can('view', $model_perms) ? route($redirectTo) : route('imports.index');
+
+            return response()->json(Helper::formatStandardApiResponse('success', $tallyPayload, ['redirect_url' => $redirect_url]));
+        } finally {
+            // Release the mutex so the next legitimate slice from the same
+            // caller (or a subsequent process attempt after this one has
+            // finished, including the error path above) can acquire. The
+            // 5-minute stale-timeout branch of the acquire WHERE remains as
+            // a safety net for the case where this release never runs
+            // (fatal error, request killed mid-flight).
+            DB::table('imports')
+                ->where('id', $import_id)
+                ->where('processing_by', auth()->id())
+                ->update([
+                    'processing_by' => null,
+                    'processing_started_at' => null,
+                ]);
         }
-        // Flash message before the redirect
-        Session::flash('success', trans('admin/hardware/message.import.success'));
-
-        $redirect_url = auth()->user()->can('view', $model_perms) ? route($redirectTo) : route('imports.index');
-
-        return response()->json(Helper::formatStandardApiResponse('success', $tallyPayload, ['redirect_url' => $redirect_url]));
     }
 
     /**

database/migrations/2026_08_12_000000_add_processing_lock_columns_to_imports_table.php

@@ -0,0 +1,40 @@
+<?php
+
+use Illuminate\Database\Migrations\Migration;
+use Illuminate\Database\Schema\Blueprint;
+use Illuminate\Support\Facades\Schema;
+
+return new class extends Migration
+{
+    public function up(): void
+    {
+        // Mutex columns for the per-import processing lock. Prevents two
+        // concurrent process() calls on the same import file from racing
+        // the app-layer unique-validation checks and producing duplicate
+        // rows. See Api\ImportController::process for the acquire logic.
+        // Both columns nullable and index-only (no FK constraint, per
+        // project convention).
+        Schema::table('imports', function (Blueprint $table) {
+            if (! Schema::hasColumn('imports', 'processing_by')) {
+                $table->unsignedBigInteger('processing_by')->nullable()->after('created_by');
+                $table->index('processing_by');
+            }
+            if (! Schema::hasColumn('imports', 'processing_started_at')) {
+                $table->timestamp('processing_started_at')->nullable()->after('processing_by');
+            }
+        });
+    }
+
+    public function down(): void
+    {
+        Schema::table('imports', function (Blueprint $table) {
+            if (Schema::hasColumn('imports', 'processing_started_at')) {
+                $table->dropColumn('processing_started_at');
+            }
+            if (Schema::hasColumn('imports', 'processing_by')) {
+                $table->dropIndex(['processing_by']);
+                $table->dropColumn('processing_by');
+            }
+        });
+    }
+};

resources/lang/en-US/admin/hardware/message.php

@@ -76,6 +76,7 @@ return [
         'file_already_deleted' => 'The file selected was already deleted',
         'file_missing_on_disk' => 'The file for this import is no longer on disk. It may have been deleted outside of Snipe-IT. Delete this entry and re-upload the file to try again.',
         'file_empty' => 'This file has no data rows. Nothing can be imported from it.',
+        'already_processing' => 'This import is currently being processed by another user. Please wait for it to finish before trying again.',
         'header_row_missing' => 'This file does not have a recognized header row. Delete this entry and re-upload the file to try again.',
         'header_row_has_malformed_characters' => 'One or more attributes in the header row contain malformed UTF-8 characters',
         'content_row_has_malformed_characters' => 'One or more attributes in the first row of content contain malformed UTF-8 characters',

resources/views/livewire/importer.blade.php

@@ -953,17 +953,32 @@
         // For the importFile part:
         $(function () {
 
+            // Client-side re-entry guard for the Process button. The server
+            // holds the actual per-import mutex (see the acquire/release
+            // block in Api\ImportController::process), which is what closes
+            // the concurrent-writer race for real; this flag just prevents
+            // the same-tab wizard from firing a second startProcessing while
+            // the first slice chain is still running. Cheap UX polish so
+            // the user isn't left wondering whether their impatient
+            // second-click did something.
+            var isProcessingImport = false;
+
             // The #import button lives inside #importMappingModal now, but
             // the modal is rendered as a sibling of #upload-table (not
             // inside it), so delegate from document to catch the click
             // regardless of where in the DOM the modal ends up after
             // Bootstrap moves it.
             $(document).on('click', '#importMappingModal #import', function () {
+                if (isProcessingImport) {
+                    return false;
+                }
                 if (!$wire.$get('typeOfImport')) {
                     $wire.$set('statusType', 'error');
                     $wire.$set('statusText', "An import type is required... "); //TODO: translate?
                     return;
                 }
+                isProcessingImport = true;
+                $(this).prop('disabled', true).attr('aria-busy', 'true');
                 $wire.$set('statusType', 'pending');
                 $wire.$set('statusText', '<i class="fa fa-spinner fa-spin" aria-hidden="true"></i> {{ trans('admin/hardware/form.processing_spinner') }}');
 
@@ -1182,6 +1197,15 @@
                     }
 
                     chain.always(function () {
+                        // Release the client-side re-entry guard so the
+                        // Process button becomes clickable again if the
+                        // user needs to retry (e.g. anySliceFailed branch
+                        // below keeps them on the wizard). On success the
+                        // modal hides and the page redirects anyway, so
+                        // the button state is moot in that case.
+                        isProcessingImport = false;
+                        $('#importMappingModal #import').prop('disabled', false).removeAttr('aria-busy');
+
                         $wire.$set('progress', 100);
                         var somethingLanded = aggregatedTally.created > 0 || aggregatedTally.updated > 0;
 

tests/Feature/Importing/Api/ImportConcurrencyTest.php

@@ -0,0 +1,180 @@
+<?php
+
+namespace Tests\Feature\Importing\Api;
+
+use App\Models\Import;
+use App\Models\User;
+use Illuminate\Support\Carbon;
+use Illuminate\Support\Facades\DB;
+use PHPUnit\Framework\Attributes\Test;
+use Tests\Support\Importing\AssetsImportFileBuilder;
+use Tests\Support\Importing\CleansUpImportFiles;
+
+/**
+ * Coverage for the per-import processing mutex on
+ * Api\ImportController::process. The primitive is an atomic UPDATE against
+ * imports.processing_by / processing_started_at with a two-branch WHERE:
+ * unlocked (NULL) or 5-minute self-heal. Same-user overlapping requests
+ * are intentionally blocked - the correct pattern is acquire-process-release,
+ * with the next slice acquiring only after the previous slice released.
+ */
+class ImportConcurrencyTest extends ImportDataTestCase
+{
+    use CleansUpImportFiles;
+
+    protected function importFileResponse(array $parameters = []): \Illuminate\Testing\TestResponse
+    {
+        if (! array_key_exists('import-type', $parameters)) {
+            $parameters['import-type'] = 'asset';
+        }
+
+        return parent::importFileResponse($parameters);
+    }
+
+    #[Test]
+    public function second_concurrent_caller_is_rejected_while_another_admin_holds_the_lock()
+    {
+        // Simulate admin A holding the mutex: another user_id is stamped
+        // into the row and the timestamp is recent (well within the 5m
+        // stale window).
+        $adminA = User::factory()->superuser()->create();
+        $adminB = User::factory()->superuser()->create();
+
+        $file = AssetsImportFileBuilder::new();
+        $import = Import::factory()->asset()->create(['file_path' => $file->saveToImportsDirectory()]);
+
+        DB::table('imports')->where('id', $import->id)->update([
+            'processing_by' => $adminA->id,
+            'processing_started_at' => Carbon::now(),
+        ]);
+
+        $this->actingAsForApi($adminB);
+
+        $response = $this->importFileResponse(['import' => $import->id]);
+
+        $response->assertStatus(409);
+        $response->assertJson([
+            'status' => 'error',
+            'messages' => trans('admin/hardware/message.import.already_processing'),
+        ]);
+
+        // Mutex owner and timestamp were NOT overwritten by admin B's attempt.
+        $row = DB::table('imports')->where('id', $import->id)->first();
+        $this->assertSame($adminA->id, (int) $row->processing_by);
+    }
+
+    #[Test]
+    public function same_caller_is_also_rejected_while_their_own_prior_request_still_holds_the_lock()
+    {
+        // The customer-reported PedidosYa duplication was two full passes
+        // through the AssetImporter by the same admin, one minute apart.
+        // Whatever fired the second pass (browser retry, proxy retry,
+        // wizard double-fire) presented as the SAME user. The mutex must
+        // block it just as firmly as it blocks a second admin - otherwise
+        // both passes race the app-layer unique-tag validation and both
+        // insert.
+        $admin = User::factory()->superuser()->create();
+
+        $file = AssetsImportFileBuilder::new();
+        $import = Import::factory()->asset()->create(['file_path' => $file->saveToImportsDirectory()]);
+
+        DB::table('imports')->where('id', $import->id)->update([
+            'processing_by' => $admin->id,
+            'processing_started_at' => Carbon::now()->subSeconds(5),
+        ]);
+
+        $this->actingAsForApi($admin);
+
+        $response = $this->importFileResponse(['import' => $import->id]);
+
+        $response->assertStatus(409);
+        $response->assertJson([
+            'status' => 'error',
+            'messages' => trans('admin/hardware/message.import.already_processing'),
+        ]);
+    }
+
+    #[Test]
+    public function lock_is_released_after_a_successful_process_so_the_next_slice_can_acquire()
+    {
+        // The wizard chains slices serially: slice N's ajax completes,
+        // then slice N+1 fires. That only works if process() releases the
+        // mutex before returning, so slice N+1 finds a NULL processing_by
+        // to acquire against.
+        $admin = User::factory()->superuser()->create();
+
+        $file = AssetsImportFileBuilder::new();
+        $import = Import::factory()->asset()->create(['file_path' => $file->saveToImportsDirectory()]);
+
+        $this->actingAsForApi($admin);
+
+        $response = $this->importFileResponse(['import' => $import->id]);
+
+        $this->assertNotEquals(409, $response->status());
+
+        // Lock was released on the way out.
+        $row = DB::table('imports')->where('id', $import->id)->first();
+        $this->assertNull($row->processing_by, 'Lock should have been released after the request finished.');
+        $this->assertNull($row->processing_started_at, 'Lock timestamp should have been cleared after the request finished.');
+    }
+
+    #[Test]
+    public function stale_lock_older_than_five_minutes_is_taken_over()
+    {
+        // A prior slice from admin A crashed mid-import, leaving
+        // processing_by set but processing_started_at is now stale
+        // (older than the 5m self-heal window). Admin B must be able
+        // to acquire and proceed.
+        $adminA = User::factory()->superuser()->create();
+        $adminB = User::factory()->superuser()->create();
+
+        $file = AssetsImportFileBuilder::new();
+        $import = Import::factory()->asset()->create(['file_path' => $file->saveToImportsDirectory()]);
+
+        DB::table('imports')->where('id', $import->id)->update([
+            'processing_by' => $adminA->id,
+            'processing_started_at' => Carbon::now()->subMinutes(10),
+        ]);
+
+        $this->actingAsForApi($adminB);
+
+        $response = $this->importFileResponse(['import' => $import->id]);
+
+        $this->assertNotEquals(409, $response->status(), 'A stale lock older than the self-heal window must not block a new caller.');
+
+        // Stale-owner state got cleared. Ownership transitioned from
+        // adminA (stale) -> adminB (acquired) -> NULL (released on the
+        // way out). The important guarantee is that the stale entry is
+        // no longer holding future callers off.
+        $row = DB::table('imports')->where('id', $import->id)->first();
+        $this->assertNull($row->processing_by, 'Stale lock should have been taken over and then released.');
+        $this->assertNull($row->processing_started_at);
+    }
+
+    #[Test]
+    public function fresh_import_with_no_prior_lock_holder_acquires_cleanly()
+    {
+        // Baseline: no lock at all, the process request acquires the
+        // mutex, processes, and releases before responding.
+        $admin = User::factory()->superuser()->create();
+
+        $file = AssetsImportFileBuilder::new();
+        $import = Import::factory()->asset()->create(['file_path' => $file->saveToImportsDirectory()]);
+
+        // Sanity: no lock in place.
+        $this->assertNull(DB::table('imports')->where('id', $import->id)->value('processing_by'));
+
+        $this->actingAsForApi($admin);
+
+        $response = $this->importFileResponse(['import' => $import->id]);
+
+        $this->assertNotEquals(409, $response->status());
+
+        // Lock released before returning (release-covered separately in
+        // lock_is_released_after_a_successful_process; asserted here too
+        // to catch regressions on the baseline path).
+        $row = DB::table('imports')->where('id', $import->id)->first();
+        $this->assertNull($row->processing_by);
+        $this->assertNull($row->processing_started_at);
+    }
+}