Skip to content

Split chunked upload phases #165

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
TorstenDittmann wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Split chunked upload phases #165

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
65b3b3e
feat: handle parallel chunk upload assembly race in Local and S3 devices
TorstenDittmann Apr 28, 2026
0385c12
feat: split chunked upload phases
TorstenDittmann May 7, 2026
28a6b79
merge main into phased upload branch
TorstenDittmann May 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions src/Storage/Device.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,33 @@ abstract public function getPath(string $filename, ?string $prefix = null): stri
*/
abstract public function upload(string $source, string $path, int $chunk = 1, int $chunks = 1, array &$metadata = []): int;

/**
* Prepare Upload.
*
* Initialize adapter-specific upload state without transferring a chunk body.
*
* @throws Exception
*/
abstract public function prepareUpload(string $path, string $contentType, int $chunks = 1, array &$metadata = []): void;

/**
* Upload Chunk.
*
* Upload exactly one chunk without finalizing the full upload.
*
* @throws Exception
*/
abstract public function uploadChunk(string $source, string $path, int $chunk = 1, int $chunks = 1, array &$metadata = []): int;

/**
* Finalize Upload.
*
* Complete a prepared upload once all chunks are known to be present.
*
* @throws Exception
*/
abstract public function finalizeUpload(string $path, int $chunks = 1, array &$metadata = []): bool;

/**
* Upload Data.
*
Expand Down
60 changes: 51 additions & 9 deletions src/Storage/Device/Local.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,16 +55,37 @@ public function getPath(string $filename, ?string $prefix = null): string
* @throws Exception
*/
public function upload(string $source, string $path, int $chunk = 1, int $chunks = 1, array &$metadata = []): int
{
$this->prepareUpload($path, '', $chunks, $metadata);
$chunksReceived = $this->uploadChunk($source, $path, $chunk, $chunks, $metadata);

if ($chunks === $chunksReceived) {
$this->finalizeUpload($path, $chunks, $metadata);
}

return $chunksReceived;
}

public function prepareUpload(string $path, string $contentType, int $chunks = 1, array &$metadata = []): void
{
$this->createDirectory(\dirname($path));
$metadata['parts'] ??= [];
$metadata['chunks'] ??= 0;
}

public function uploadChunk(string $source, string $path, int $chunk = 1, int $chunks = 1, array &$metadata = []): int
{
$this->prepareUpload($path, '', $chunks, $metadata);

Comment on lines +76 to 79
Copy link
Copy Markdown

@greptile-apps greptile-apps Bot May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 uploadChunk self-calls prepareUpload, bypassing Telemetry wrapper

Local::uploadChunk calls $this->prepareUpload(...) directly on the Local instance (line 78). When a Telemetry device wraps Local and a caller invokes Telemetry::uploadChunk, the delegated Local::uploadChunk triggers Local::prepareUpload internally — never going through Telemetry::prepareUpload. That internal invocation is therefore not measured or recorded. Additionally, when upload() is the caller (which already called prepareUpload on the way in), the metadata is initialized twice, which works only because ??= is idempotent. S3::uploadChunk has no such internal call, making the two adapters asymmetric in their self-initialization behaviour.

// move_uploaded_file() verifies the file is not tampered with
if ($chunks === 1) {
if (! \move_uploaded_file($source, $path)) {
if (! \move_uploaded_file($source, $path) && ! \rename($source, $path)) {
throw new Exception('Can\'t upload file '.$path);
}

return $chunks;
$metadata['parts'][$chunk] = true;
$metadata['chunks'] = 1;

return 1;
}

$tmp = \dirname($path).DIRECTORY_SEPARATOR.'tmp_'.\basename($path);
Expand All @@ -84,14 +105,33 @@ public function upload(string $source, string $path, int $chunk = 1, int $chunks
}

$chunksReceived = $this->countChunks($tmp, $path);
$metadata['parts'][$chunk] = true;
$metadata['chunks'] = $chunksReceived;

if ($chunks === $chunksReceived) {
$this->joinChunks($path, $chunks);
return $chunksReceived;
}

return $chunksReceived;
public function finalizeUpload(string $path, int $chunks = 1, array &$metadata = []): bool
{
if (\file_exists($path)) {
return true;
}

return $chunksReceived;
if ($chunks === 1) {
return false;
}

$tmp = \dirname($path).DIRECTORY_SEPARATOR.'tmp_'.\basename($path);
for ($i = 1; $i <= $chunks; $i++) {
$part = $tmp.DIRECTORY_SEPARATOR.\pathinfo($path, PATHINFO_FILENAME).'.part.'.$i;
if (! \file_exists($part)) {
throw new Exception('Missing chunk '.$i);
}
}

$this->joinChunks($path, $chunks);

return true;
}

/**
Expand All @@ -108,7 +148,7 @@ public function upload(string $source, string $path, int $chunk = 1, int $chunks
*/
public function uploadData(string $data, string $path, string $contentType, int $chunk = 1, int $chunks = 1, array &$metadata = []): int
{
$this->createDirectory(\dirname($path));
$this->prepareUpload($path, $contentType, $chunks, $metadata);

if ($chunks === 1) {
if (! \file_put_contents($path, $data)) {
Expand All @@ -131,9 +171,11 @@ public function uploadData(string $data, string $path, string $contentType, int
}

$chunksReceived = $this->countChunks($tmp, $path);
$metadata['parts'][$chunk] = true;
$metadata['chunks'] = $chunksReceived;

if ($chunks === $chunksReceived) {
$this->joinChunks($path, $chunks);
$this->finalizeUpload($path, $chunks, $metadata);

return $chunksReceived;
}
Expand Down
101 changes: 80 additions & 21 deletions src/Storage/Device/S3.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,64 @@ public static function setRetryDelay(int $delay): void
*/
public function upload(string $source, string $path, int $chunk = 1, int $chunks = 1, array &$metadata = []): int
{
return $this->uploadData(\file_get_contents($source), $path, \mime_content_type($source), $chunk, $chunks, $metadata);
$contentType = \mime_content_type($source) ?: '';
$this->prepareUpload($path, $contentType, $chunks, $metadata);
$chunksReceived = $this->uploadChunk($source, $path, $chunk, $chunks, $metadata);

if ($chunks === $chunksReceived) {
$this->finalizeUpload($path, $chunks, $metadata);
}

return $chunksReceived;
}

public function prepareUpload(string $path, string $contentType, int $chunks = 1, array &$metadata = []): void
{
$metadata['parts'] ??= [];
$metadata['chunks'] ??= 0;
$metadata['content_type'] ??= $contentType;

if ($chunks === 1 || ! empty($metadata['uploadId'])) {
return;
}

$metadata['uploadId'] = $this->createMultipartUpload($path, $contentType);
}

public function uploadChunk(string $source, string $path, int $chunk = 1, int $chunks = 1, array &$metadata = []): int
{
$data = \file_get_contents($source);
if ($data === false) {
throw new Exception('Can\'t read file '.$source);
}

return $this->uploadChunkData($data, $path, $metadata['content_type'] ?? (\mime_content_type($source) ?: ''), $chunk, $chunks, $metadata);
}

public function finalizeUpload(string $path, int $chunks = 1, array &$metadata = []): bool
{
if ($this->exists($path)) {
return true;
}

if ($chunks === 1) {
return false;
}

if (empty($metadata['uploadId'])) {
throw new Exception('Missing multipart upload ID');
}

$metadata['parts'] ??= [];
for ($i = 1; $i <= $chunks; $i++) {
if (! array_key_exists($i, $metadata['parts'])) {
throw new Exception('Missing chunk '.$i);
}
}

$this->completeMultipartUpload($path, $metadata['uploadId'], $metadata['parts']);

return true;
}
Comment on lines +204 to 228
Copy link
Copy Markdown

@greptile-apps greptile-apps Bot May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Extra HEAD request on every single-chunk S3 upload

upload() and uploadData() now call finalizeUpload() whenever $chunks === $chunksReceived, including for single-chunk uploads (where $chunksReceived === 1 === $chunks). For the single-chunk path, write() already completed the PUT, so the $this->exists($path) call at the top of finalizeUpload issues an extra HEAD (s3:info) request before returning true. The original code returned immediately after write() with no follow-up network call. Every single-file S3 upload now incurs an extra API request.


/**
Expand All @@ -183,38 +240,40 @@ public function upload(string $source, string $path, int $chunk = 1, int $chunks
* @throws Exception
*/
public function uploadData(string $data, string $path, string $contentType, int $chunk = 1, int $chunks = 1, array &$metadata = []): int
{
$this->prepareUpload($path, $contentType, $chunks, $metadata);
$chunksReceived = $this->uploadChunkData($data, $path, $contentType, $chunk, $chunks, $metadata);

if ($chunks === $chunksReceived) {
$this->finalizeUpload($path, $chunks, $metadata);
}

return $chunksReceived;
}
Comment on lines 242 to +252
Copy link
Copy Markdown

@greptile-apps greptile-apps Bot May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 finalizeUpload return value is silently discarded

Both upload() (line 175) and uploadData() (line 248) call finalizeUpload() but never check or propagate its return value. finalizeUpload returns false for the single-chunk case when the file does not exist at finalization time. In that scenario the methods still return $chunksReceived, so the caller observes an apparent success while the object is absent. The same pattern exists in Local::upload() and Local::uploadData(). At minimum, a false return should throw or be propagated so callers can react.


private function uploadChunkData(string $data, string $path, string $contentType, int $chunk = 1, int $chunks = 1, array &$metadata = []): int
{
if ($chunk == 1 && $chunks == 1) {
return $this->write($path, $data, $contentType);
$this->write($path, $data, $contentType);
$metadata['parts'][$chunk] = true;
$metadata['chunks'] = 1;

return 1;
}
$uploadId = $metadata['uploadId'] ?? null;
if (empty($uploadId)) {
$uploadId = $this->createMultipartUpload($path, $contentType);
$metadata['uploadId'] = $uploadId;

if (empty($metadata['uploadId'])) {
throw new Exception('Missing multipart upload ID');
}

$metadata['parts'] ??= [];
$metadata['chunks'] ??= 0;

$etag = $this->uploadPart($data, $path, $contentType, $chunk, $uploadId);
$etag = $this->uploadPart($data, $path, $contentType, $chunk, $metadata['uploadId']);
// skip incrementing if the chunk was re-uploaded
if (! array_key_exists($chunk, $metadata['parts'])) {
$metadata['chunks']++;
}
$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']);
}

return $metadata['chunks'];
}
Expand Down Expand Up @@ -307,7 +366,7 @@ protected function completeMultipartUpload(string $path, string $uploadId, array
{
$uri = $path !== '' ? '/'.\str_replace(['%2F', '%3F'], ['/', '?'], \rawurlencode($path)) : '/';

\ksort($parts);
\ksort($parts, SORT_NUMERIC);

$body = '<CompleteMultipartUpload>';
foreach ($parts as $key => $etag) {
Expand Down
15 changes: 15 additions & 0 deletions src/Storage/Device/Telemetry.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,21 @@ public function upload(string $source, string $path, int $chunk = 1, int $chunks
return $this->measure(__FUNCTION__, $source, $path, $chunk, $chunks, $metadata);
}

public function prepareUpload(string $path, string $contentType, int $chunks = 1, array &$metadata = []): void
{
$this->measure(__FUNCTION__, $path, $contentType, $chunks, $metadata);
}

public function uploadChunk(string $source, string $path, int $chunk = 1, int $chunks = 1, array &$metadata = []): int
{
return $this->measure(__FUNCTION__, $source, $path, $chunk, $chunks, $metadata);
}

public function finalizeUpload(string $path, int $chunks = 1, array &$metadata = []): bool
{
return $this->measure(__FUNCTION__, $path, $chunks, $metadata);
}

public function uploadData(string $data, string $path, string $contentType, int $chunk = 1, int $chunks = 1, array &$metadata = []): int
{
return $this->measure(__FUNCTION__, $data, $path, $contentType, $chunk, $chunks, $metadata);
Expand Down
45 changes: 45 additions & 0 deletions tests/Storage/Device/LocalTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,51 @@ public function testPartUpload()
return $dest;
}

public function testUploadChunkDoesNotFinalizeUntilFinalizeUpload(): void
{
$dest = $this->object->getPath('chunked-phase-upload.txt');
$metadata = [];
$parts = [
2 => 'bbb',
1 => 'aaa',
3 => 'ccc',
];

foreach ($parts as $chunk => $data) {
$source = __DIR__.'/chunk-'.$chunk.'.part';
file_put_contents($source, $data);

$this->object->uploadChunk($source, $dest, $chunk, 3, $metadata);
$this->assertFalse($this->object->exists($dest));
}

$this->assertSame(3, $metadata['chunks']);
$this->assertTrue($this->object->finalizeUpload($dest, 3, $metadata));
$this->assertSame('aaabbbccc', $this->object->read($dest));
$this->assertTrue($this->object->finalizeUpload($dest, 3, $metadata));

$this->object->delete($dest);
}

public function testFinalizeUploadRequiresAllLocalChunks(): void
{
$dest = $this->object->getPath('chunked-phase-missing.txt');
$metadata = [];
$source = __DIR__.'/chunk-missing.part';
file_put_contents($source, 'aaa');

$this->object->uploadChunk($source, $dest, 1, 2, $metadata);

try {
$this->object->finalizeUpload($dest, 2, $metadata);
$this->fail('Expected missing chunk exception');
} catch (\Exception $e) {
$this->assertSame('Missing chunk 2', $e->getMessage());
} finally {
$this->object->abort($dest);
}
}

public function testPartUploadRetry()
{
$source = __DIR__.'/../../resources/disk-a/large_file.mp4';
Expand Down
Loading
Loading