Skip to content

Split chunked upload phases - #165

Merged
TorstenDittmann merged 5 commits into
mainfrom
feat-phased-chunk-upload-api
May 15, 2026
Merged

Split chunked upload phases#165
TorstenDittmann merged 5 commits into
mainfrom
feat-phased-chunk-upload-api

Conversation

@TorstenDittmann

Copy link
Copy Markdown
Contributor

Summary

  • Preserve existing parallel chunk upload fixes and add public prepare, uploadChunk, and finalize phases to storage devices and telemetry.
  • Split Local and S3 chunk handling so chunk transfer does not finalize uploads.
  • Add focused Local and mocked S3 tests for phased upload behavior.

Tests

  • composer lint
  • ./vendor/bin/phpunit tests/Storage/Device/S3SlowDownTest.php
  • ./vendor/bin/phpunit tests/Storage/Device/LocalTest.php --filter 'testUploadChunkDoesNotFinalizeUntilFinalizeUpload|testFinalizeUploadRequiresAllLocalChunks|testPartUpload|testPartUploadRetry|testAbort|testPartRead'

Notes

  • Full unit suite still has existing environment/fixture failures: Local transfer tests require S3 credentials, and testGetFiles collides with local directory state.

@greptile-apps

greptile-appsBot commented May 7, 2026

Copy link
Copy Markdown

Greptile Summary

This PR splits the monolithic upload method on Local and S3 into three public, independently callable phases — prepareUpload, uploadChunk, and finalizeUpload — and adds corresponding Telemetry wrappers and focused unit tests.

  • Local and S3 now expose phased upload APIs; upload() and uploadData() are reimplemented as orchestrators that call the three phases in sequence, preserving backward-compatible behavior for single-chunk paths.
  • S3::completeMultipartUpload gains SORT_NUMERIC ordering and an explicit content-type: application/xml header, both of which fix latent correctness issues with the complete-multipart request.
  • New mock-based S3SlowDownTest tests verify each phase in isolation, and two new LocalTest cases cover the no-finalize-until-finalize contract and the missing-chunk guard.

Confidence Score: 4/5

Safe to merge after fixing the metadata omission in Local::uploadData's single-chunk path.

The Local::uploadData single-chunk path returns without updating $metadata['parts'] or $metadata['chunks'], leaving them at the zero-state initialised by prepareUpload. Every other single-chunk code path sets these fields correctly, so any caller that reads metadata after a single-chunk uploadData will observe inconsistent state.

src/Storage/Device/Local.php — the uploadData single-chunk early-return path does not update metadata.

Important Files Changed

FilenameOverview
src/Storage/Device/Device.phpAdds three new abstract methods — prepareUpload, uploadChunk, and finalizeUpload — to define the phased upload contract for all adapters.
src/Storage/Device/Local.phpSplits upload into prepare/uploadChunk/finalize phases; uploadData single-chunk path exits without updating $metadata['parts'] and $metadata['chunks'], leaving them at the zero-state set by prepareUpload.
src/Storage/Device/S3.phpSplits S3 upload into phased methods; moves multipart completion into finalizeUpload; adds SORT_NUMERIC to ksort and sets content-type: application/xml for the complete-multipart request.
src/Storage/Device/Telemetry.phpAdds pass-through measurement wrappers for the three new phased upload methods; correctly propagates the &$metadata reference via the existing measure variadic-by-ref mechanism.
tests/Storage/Device/LocalTest.phpAdds two new tests for phased local upload: one verifying the file is absent until finalizeUpload, and one verifying an exception is thrown for a missing chunk.
tests/Storage/Device/S3SlowDownTest.phpExtends TestableS3 with a full mock call() implementation and adds focused tests for each phased method including numeric part ordering and XML content-type validation.

Comments Outside Diff (1)

  1. src/Storage/Device/Local.php, line 155-161 (link)

    P1uploadData's single-chunk path returns without updating $metadata['parts'] or $metadata['chunks']. prepareUpload initialises both to empty/zero, and the early return $chunks exits before setting them. Any caller that inspects metadata after a successful single-chunk uploadData sees chunks=0 and an empty parts array — inconsistent with every other single-chunk path (upload() via uploadChunk, standalone uploadChunk, and S3::uploadChunkData) which all set them correctly.

Reviews (3): Last reviewed commit: "fix: avoid finalizing single chunk uploa..." | Re-trigger Greptile

Comment threadsrc/Storage/Device/S3.php
Comment threadsrc/Storage/Device/Local.php
Comment threadsrc/Storage/Device/S3.php
@TorstenDittmann
TorstenDittmann merged commit 37129cf into mainMay 15, 2026
8 of 9 checks passed
@TorstenDittmann
TorstenDittmann deleted the feat-phased-chunk-upload-api branch May 15, 2026 09:42
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@TorstenDittmann