From f2b705beee0d4a1347664aa30b6b624ae851f852 Mon Sep 17 00:00:00 2001 From: denkfabrik-li <274324701+denkfabrik-li@users.noreply.github.com> Date: Fri, 28 Aug 2026 03:10:20 +0200 Subject: [PATCH] Keep an upload's parts until its bytes are stored MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit complete() holds a lock whose comment promises "the lock's TTL releases the claim if a completion dies mid-flight, so a later retry still works". A retry has nothing to work from but the parts, and assemble() unlinked each one inside the loop that read it -- so everything that can fail afterwards took the retry with it. Measured on main, with a disk refusing the write (the case the guard forty lines further down was written for, found against a real GCS bucket): first complete → 422, 0 parts left, the half-written copy left behind retry → 422 "Upload is incomplete: missing parts." For good: listParts() is empty, so no later attempt can ever succeed, and the client has to send the whole file again. The abandoned copy sat in the session directory until the sweeper came round. The parts now go when abort() clears the session directory -- which already ran on success -- and a failure deletes only the half-written copy it made. The cost is temp space: peak usage during assembly is the whole file twice over rather than the file plus one part. The docblock says so. Also checked while here: every read and every write in the concatenation. A failing fwrite is loud in practice, since Laravel's error handler turns the warning into an ErrorException, but loud there is a 500 carrying a PHP message where this method's other storage failure is a sentence the person uploading can act on. A short write arriving without a warning would be worse: the byte count and the checksum describe the buffer that was read, so an unchecked one records a truncated file with a checksum matching bytes that were never stored. Two tests: the retry after a refused write now succeeds, and a temporary directory that refuses writes (/dev/full, skipped where it does not exist) fails the upload with this method's own message. Without the fix both go red. --- app/Modules/Files/Uploads/LocalPartStore.php | 212 ++++++++++++------- tests/Feature/Files/ChunkedUploadsTest.php | 55 +++++ 2 files changed, 194 insertions(+), 73 deletions(-) diff --git a/app/Modules/Files/Uploads/LocalPartStore.php b/app/Modules/Files/Uploads/LocalPartStore.php index ec70e623..95472f0c 100644 --- a/app/Modules/Files/Uploads/LocalPartStore.php +++ b/app/Modules/Files/Uploads/LocalPartStore.php @@ -27,6 +27,12 @@ use Throwable; */ class LocalPartStore { + /** + * Said twice, because a full temp volume can announce itself in the + * middle of the copy or only when the last buffer is flushed. + */ + private const WRITE_FAILED = 'Could not assemble the upload: writing to the temporary directory failed.'; + /** * The route name is a parameter because the same flow is mounted twice: * once on the session-authenticated web routes for the browser, once on @@ -140,9 +146,21 @@ class LocalPartStore } /** - * Stream-append parts in order onto the files disk, hashing as we - * go. Peak temp usage ≈ file size + one part (parts are unlinked - * as they are consumed). + * Stream-append parts in order onto the files disk, hashing as we go. + * + * The parts stay on disk until the assembled bytes are safely on the + * target disk. ChunkedUploadsController's completion lock promises that + * "a later retry still works", and everything that can fail after the + * concatenation — reopening the copy, a disk refusing the write, the + * File row itself — happens while the client has nothing but this + * session to retry with. Unlinking each part as it was consumed left + * listParts() empty, so every later complete() answered "Upload is + * incomplete: missing parts" for good. + * + * The cost is temp space: peak usage is the whole file twice over + * (every part, plus the assembled copy) rather than the file plus one + * part. Both are freed by the abort() below the moment the write lands, + * and by the failure path the moment it does not. * * @return array{path: string, disk: string, size: int, checksum: string} */ @@ -158,6 +176,93 @@ class LocalPartStore } $assembledPath = $this->directory($session).'/assembled'; + + try { + [$size, $checksum] = $this->concatenate($session, $parts, $assembledPath); + + $readStream = fopen($assembledPath, 'rb'); + + if ($readStream === false) { + throw new RuntimeException('Could not reopen assembled file.'); + } + + $diskEvent = new ResolvingUploadDisk($session->user); + Event::dispatch($diskEvent); + $disk = $diskEvent->disk; + + $written = Storage::disk($disk)->writeStream($targetPath, $readStream); + + if (is_resource($readStream)) { + fclose($readStream); + } + + // The disks are configured with 'throw' => false, so a refused + // write is a `false` return rather than an exception — and the + // caller goes on to record a File row for bytes that were never + // stored. Losing an upload silently is worse than failing it, and + // this is the only place that can tell the difference: a real + // instance of it was a GCS bucket rejecting the adapter's ACL, + // which looked exactly like a successful upload. + if ($written === false) { + // The reason is lost by the time it gets here — 'throw' => false + // means Flysystem swallowed the exception rather than passing it + // on — so log what was attempted. Which bucket it was is the + // difference between reading this as "my credentials expired" + // and "I typed the wrong bucket name", and only the log can say + // it: the message below is shown to whoever was uploading, which + // includes clients, and a bucket name is not theirs to see. + Log::error('Upload could not be written to storage.', [ + 'disk' => $disk, + 'bucket' => config('filesystems.disks.'.$disk.'.bucket'), + 'driver' => config('filesystems.disks.'.$disk.'.driver'), + 'path' => $targetPath, + ]); + + throw new RuntimeException( + 'Could not write the assembled upload to the "'.$disk.'" disk. ' + .'Check the storage backend is reachable and its credentials are still valid.' + ); + } + } catch (Throwable $failure) { + // The half-written copy belongs to this attempt and the next one + // makes its own; the parts belong to the client, and they are + // what a retry needs. Deleting the copy here is also the only + // thing that removes it at all on this path — it used to sit in + // the session directory until the sweeper came round. + FileSystem::delete($assembledPath); + + throw $failure; + } + + $this->abort($session); + + return [ + 'path' => $targetPath, + 'disk' => $disk, + 'size' => $size, + 'checksum' => $checksum, + ]; + } + + /** + * Concatenate the parts into $assembledPath, returning the byte count + * and the sha256 of what was written. + * + * Every read and every write is checked. They were not, and while a + * failing fwrite on a full volume is loud in practice — Laravel's + * error handler turns the warning into an ErrorException — loud there + * means a 500 carrying a PHP message, where the disk-refused-the-write + * case a few lines above becomes a sentence the person uploading can + * act on. A short write arriving without a warning would be worse + * still: $size and the hash describe the buffer that was read, so an + * unchecked one yields a truncated file with a checksum matching bytes + * that were never stored. + * + * @param list $parts + * @return array{0: int, 1: string} + */ + private function concatenate(UploadSession $session, array $parts, string $assembledPath): array + { $out = fopen($assembledPath, 'wb'); if ($out === false) { @@ -167,85 +272,46 @@ class LocalPartStore $hash = hash_init('sha256'); $size = 0; - foreach ($parts as $part) { - $partPath = $this->partPath($session, $part['PartNumber']); - $in = fopen($partPath, 'rb'); + try { + foreach ($parts as $part) { + $in = fopen($this->partPath($session, $part['PartNumber']), 'rb'); - if ($in === false) { - fclose($out); - throw new RuntimeException('Could not read part '.$part['PartNumber'].'.'); - } - - while (! feof($in)) { - $buffer = fread($in, 1024 * 1024); - - if ($buffer === false) { - break; + if ($in === false) { + throw new RuntimeException('Could not read part '.$part['PartNumber'].'.'); } - fwrite($out, $buffer); - hash_update($hash, $buffer); - $size += strlen($buffer); + try { + while (! feof($in)) { + $buffer = fread($in, 1024 * 1024); + + if ($buffer === false) { + throw new RuntimeException('Could not read part '.$part['PartNumber'].'.'); + } + + if ($buffer !== '' && @fwrite($out, $buffer) !== strlen($buffer)) { + throw new RuntimeException(self::WRITE_FAILED); + } + + hash_update($hash, $buffer); + $size += strlen($buffer); + } + } finally { + fclose($in); + } } + } catch (Throwable $failure) { + fclose($out); - fclose($in); - unlink($partPath); + throw $failure; } - fclose($out); - - $readStream = fopen($assembledPath, 'rb'); - - if ($readStream === false) { - throw new RuntimeException('Could not reopen assembled file.'); + // fclose flushes, so a volume that filled up on the last buffer + // fails here rather than in the loop. + if (! fclose($out)) { + throw new RuntimeException(self::WRITE_FAILED); } - $diskEvent = new ResolvingUploadDisk($session->user); - Event::dispatch($diskEvent); - $disk = $diskEvent->disk; - - $written = Storage::disk($disk)->writeStream($targetPath, $readStream); - - if (is_resource($readStream)) { - fclose($readStream); - } - - // The disks are configured with 'throw' => false, so a refused - // write is a `false` return rather than an exception — and the - // caller goes on to record a File row for bytes that were never - // stored. Losing an upload silently is worse than failing it, and - // this is the only place that can tell the difference: a real - // instance of it was a GCS bucket rejecting the adapter's ACL, - // which looked exactly like a successful upload. - if ($written === false) { - // The reason is lost by the time it gets here — 'throw' => false - // means Flysystem swallowed the exception rather than passing it - // on — so log what was attempted. Which bucket it was is the - // difference between reading this as "my credentials expired" - // and "I typed the wrong bucket name", and only the log can say - // it: the message below is shown to whoever was uploading, which - // includes clients, and a bucket name is not theirs to see. - Log::error('Upload could not be written to storage.', [ - 'disk' => $disk, - 'bucket' => config('filesystems.disks.'.$disk.'.bucket'), - 'driver' => config('filesystems.disks.'.$disk.'.driver'), - 'path' => $targetPath, - ]); - - throw new RuntimeException( - 'Could not write the assembled upload to the "'.$disk.'" disk. ' - .'Check the storage backend is reachable and its credentials are still valid.' - ); - } - - $this->abort($session); - - return [ - 'path' => $targetPath, - 'disk' => $disk, - 'size' => $size, - 'checksum' => hash_final($hash), - ]; + return [$size, hash_final($hash)]; } public function abort(UploadSession $session): void diff --git a/tests/Feature/Files/ChunkedUploadsTest.php b/tests/Feature/Files/ChunkedUploadsTest.php index 90992f71..0e5977cf 100644 --- a/tests/Feature/Files/ChunkedUploadsTest.php +++ b/tests/Feature/Files/ChunkedUploadsTest.php @@ -329,6 +329,61 @@ test('a storage backend that refuses the write fails the upload instead of recor expect(File::query()->count())->toBe($before); }); +// What survives a completion that failed. The lock in complete() promises +// the client may try again once whatever went wrong is fixed -- "the +// lock's TTL releases the claim if a completion dies mid-flight, so a +// later retry still works" -- and a retry has nothing to work from but +// the parts. +test('a refused write leaves the parts for the retry the lock promises', function () { + $this->actingAs($this->admin); + $sessionId = createSession(11, 'assembled.txt'); + + putPart($sessionId, 1, 'hello-'); + putPart($sessionId, 2, 'world'); + + $refusing = Mockery::mock(Illuminate\Contracts\Filesystem\Filesystem::class); + $refusing->shouldReceive('writeStream')->once()->andReturnFalse(); + Storage::set('files', $refusing); + + $this->postJson("/uploads/{$sessionId}/complete")->assertStatus(422); + + // Both parts are still there, and the half-written copy is not. + expect($this->getJson("/uploads/{$sessionId}/parts")->json())->toHaveCount(2) + ->and(file_exists(partsRoot().'/'.$sessionId.'/assembled'))->toBeFalse(); + + // The operator fixes the bucket; the same session completes. + Storage::fake('files'); + + $this->postJson("/uploads/{$sessionId}/complete")->assertOk(); + + $file = File::query()->latest('id')->firstOrFail(); + expect(Storage::disk('files')->get($file->path))->toBe('hello-world') + ->and($file->size)->toBe(11); +}); + +test('a temporary directory that cannot be written fails the upload, and says so', function () { + // /dev/full accepts an open and refuses every write with ENOSPC, which + // is the failure this guards against without needing a full volume. + // Unchecked, the byte count and the checksum describe what was read + // rather than what landed; checked, it reads like the other storage + // failure a few lines below it in the same method. + $this->actingAs($this->admin); + $sessionId = createSession(11, 'assembled.txt'); + + putPart($sessionId, 1, 'hello-'); + putPart($sessionId, 2, 'world'); + + symlink('/dev/full', partsRoot().'/'.$sessionId.'/assembled'); + + $this->postJson("/uploads/{$sessionId}/complete") + ->assertStatus(422) + ->assertJsonPath('errors.parts.0', fn (string $message): bool => str_contains($message, 'Could not assemble the upload')); + + // Nothing recorded, and the parts are still the client's to retry with. + expect(File::query()->count())->toBe(0) + ->and($this->getJson("/uploads/{$sessionId}/parts")->json())->toHaveCount(2); +})->skip(! file_exists('/dev/full'), 'needs /dev/full, which only exists on Linux'); + // A chunked upload is two requests, and store()'s rule only ever sees the // first one. Delete the folder while the bytes are in flight and the // session still names it -- the version of this that nobody can ask for