Split chunked upload phases#165
Conversation
Greptile SummaryThis PR splits the monolithic
Confidence Score: 4/5Safe to merge for multi-chunk uploads; single-chunk S3 uploads gain an extra network round-trip and the Local adapter has a minor telemetry gap. The multi-chunk and phased-upload paths are correct and well-tested. The extra S3 exists() HEAD call on every single-chunk upload is a regression from the original direct-write return, and Local::uploadChunk calling prepareUpload internally bypasses Telemetry measurement. Neither breaks correctness but both affect observable behaviour. src/Storage/Device/S3.php and src/Storage/Device/Local.php warrant a second look around the finalizeUpload early-return path and the internal prepareUpload self-call respectively. Important Files Changed
Reviews (1): Last reviewed commit: "merge main into phased upload branch" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| 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; | ||
| } |
There was a problem hiding this comment.
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.
| public function uploadChunk(string $source, string $path, int $chunk = 1, int $chunks = 1, array &$metadata = []): int | ||
| { | ||
| $this->prepareUpload($path, '', $chunks, $metadata); | ||
|
|
There was a problem hiding this comment.
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.
| 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; | ||
| } |
There was a problem hiding this comment.
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.
1 participant
Summary
Tests
Notes