From 65b3b3ebeda840391d69e323d17c98e54b6fc95e Mon Sep 17 00:00:00 2001
From: Torsten Dittmann <torsten.dittmann@googlemail.com>
Date: Tue, 28 Apr 2026 14:36:37 +0400
Subject: [PATCH] feat: handle parallel chunk upload assembly race in Local and
 S3 devices

---
 .gitignore                         |  3 ++-
 src/Storage/Device/Local.php       | 11 +++++++-
 src/Storage/Device/S3.php          | 10 ++++++++
 tests/Storage/Device/LocalTest.php | 40 +++++++++++++++++++++++++++---
 4 files changed, 59 insertions(+), 5 deletions(-)

diff --git a/.gitignore b/.gitignore
index 3491b43d..282ca759 100644
--- a/.gitignore
+++ b/.gitignore
@@ -3,4 +3,5 @@
 .phpunit.result.cache
 tests/chunk.php
 .idea/
-.env
\ No newline at end of file
+.env
+.DS_Store
\ No newline at end of file
diff --git a/src/Storage/Device/Local.php b/src/Storage/Device/Local.php
index c25612d9..23757936 100644
--- a/src/Storage/Device/Local.php
+++ b/src/Storage/Device/Local.php
@@ -164,8 +164,12 @@ private function countChunks(string $tmp, string $path): int
 
     private function joinChunks(string $path, int $chunks): void
     {
+        if (\file_exists($path)) {
+            return;
+        }
+
         $tmp = \dirname($path).DIRECTORY_SEPARATOR.'tmp_'.\basename($path);
-        $tmpAssemble = \dirname($path).DIRECTORY_SEPARATOR.'tmp_assemble_'.\basename($path);
+        $tmpAssemble = \tempnam(\dirname($path), 'tmp_assemble_'.\basename($path).'_');
 
         $dest = \fopen($tmpAssemble, 'wb');
         if ($dest === false) {
@@ -195,6 +199,11 @@ private function joinChunks(string $path, int $chunks): void
         \fclose($dest);
 
         if (! \rename($tmpAssemble, $path)) {
+            if (\file_exists($path)) {
+                \unlink($tmpAssemble);
+
+                return;
+            }
             \unlink($tmpAssemble);
             throw new Exception('Failed to finalize assembled file '.$path);
         }
diff --git a/src/Storage/Device/S3.php b/src/Storage/Device/S3.php
index d2e79c4e..5e65c095 100644
--- a/src/Storage/Device/S3.php
+++ b/src/Storage/Device/S3.php
@@ -203,6 +203,16 @@ public function uploadData(string $data, string $path, string $contentType, int
         }
         $metadata['parts'][$chunk] = $etag;
         if ($metadata['chunks'] == $chunks) {
+            $headers = $this->headers;
+            $amzHeaders = $this->amzHeaders;
+
+            if ($this->exists($path)) {
+                return $metadata['chunks'];
+            }
+
+            $this->headers = $headers;
+            $this->amzHeaders = $amzHeaders;
+
             $this->completeMultipartUpload($path, $uploadId, $metadata['parts']);
         }
 
diff --git a/tests/Storage/Device/LocalTest.php b/tests/Storage/Device/LocalTest.php
index 2146d413..d141a6c2 100644
--- a/tests/Storage/Device/LocalTest.php
+++ b/tests/Storage/Device/LocalTest.php
@@ -524,19 +524,20 @@ public function testJoinChunksStaleAssemblyFileIsOverwritten(): void
     {
         $storage = $this->makeJoinTestStorage();
         $dest = $storage->getRoot().DIRECTORY_SEPARATOR.'test.dat';
-        $tmpAssemble = $storage->getRoot().DIRECTORY_SEPARATOR.'tmp_assemble_test.dat';
 
         $storage->uploadData('AAAA', $dest, 'application/octet-stream', 1, 3);
         $storage->uploadData('BBBB', $dest, 'application/octet-stream', 2, 3);
 
         // Simulate a stale assembly file left by a previously crashed attempt.
-        \file_put_contents($tmpAssemble, 'STALE_GARBAGE_DATA');
+        // With unique temp paths (tempnam), stale files at old hardcoded paths
+        // are naturally bypassed rather than overwritten.
+        $staleFile = $storage->getRoot().DIRECTORY_SEPARATOR.'tmp_assemble_test.dat';
+        \file_put_contents($staleFile, 'STALE_GARBAGE_DATA');
 
         $storage->uploadData('CCCC', $dest, 'application/octet-stream', 3, 3);
 
         $this->assertTrue(\file_exists($dest));
         $this->assertSame('AAAABBBBCCCC', \file_get_contents($dest), 'Stale assembly file must not corrupt output');
-        $this->assertFalse(\file_exists($tmpAssemble), 'Temp assembly file should be removed after successful rename');
 
         $storage->delete($storage->getRoot(), true);
     }
@@ -577,4 +578,37 @@ public function testOutOfOrderUploadWithRetry(): void
 
         $storage->delete($storage->getRoot(), true);
     }
+
+    public function testParallelChunkUpload(): void
+    {
+        $storage = $this->makeJoinTestStorage();
+        $dest = $storage->getRoot().DIRECTORY_SEPARATOR.'parallel.dat';
+
+        // Upload chunk 1 (creates temp directory)
+        $storage->uploadData('AAAA', $dest, 'application/octet-stream', 1, 2);
+
+        // Upload chunk 2 (assembles the file)
+        $storage->uploadData('BBBB', $dest, 'application/octet-stream', 2, 2);
+
+        // Verify file exists and is correct
+        $this->assertTrue(\file_exists($dest));
+        $this->assertSame('AAAABBBB', \file_get_contents($dest));
+
+        // Simulate the race where another request already assembled the file
+        // by calling joinChunks directly when the file already exists
+        $reflection = new \ReflectionClass($storage);
+        $method = $reflection->getMethod('joinChunks');
+        $method->setAccessible(true);
+
+        try {
+            $method->invoke($storage, $dest, 2);
+        } catch (\Exception $e) {
+            $this->fail('Duplicate assembly should not throw: '.$e->getMessage());
+        }
+
+        $this->assertTrue(\file_exists($dest), 'File should still exist after duplicate assembly attempt');
+        $this->assertSame('AAAABBBB', \file_get_contents($dest), 'File content must not be corrupted');
+
+        $storage->delete($storage->getRoot(), true);
+    }
 }