Review multipart upload completion

Review exercise · TypeScript

Review brief

Intermediate35 min

A media service records each successfully uploaded part before clients request completion. The new completion service validates the caller's manifest, asks object storage to assemble the object, and records the final state used by downstream processors.

Behavioral contract

Only the account that created an upload may complete it. A session in `uploading` state may transition once; any other state is rejected except an exact idempotent replay of an already completed manifest.
The request must contain each durably recorded uploaded part exactly once, with no missing, duplicate, or extra part numbers. Part numbers are integers from 1 through 10,000.
ETags are opaque, case-sensitive strings and must match the recorded value byte for byte, including any quotes. The service must not trim, lowercase, or otherwise normalize them.
Object storage receives parts in ascending numeric part-number order, regardless of request order.
The session remains `uploading` until object storage completion succeeds. If storage rejects, local state remains retryable; only the returned completed object key and version are then recorded.
The object-storage completion operation is idempotent for the same upload ID and exact ordered manifest; storage authentication and individual part upload are outside this change.

Review instructions

1Review caller authorization, session state, manifest equality, ETag handling, ordering, and completion sequencing.
2Use examples containing parts 1, 2, and 10, plus a storage failure after validation.
3Prioritize no more than six findings, reserving test feedback for behavior the current suite cannot detect.

Learning objectives

• Validate a completion manifest against durable upload state
• Preserve opaque identifiers and numeric ordering
• Order local state after external completion

src/multipart-types.tsTypeScript

@@ -0,0 +1,30 @@

1+export interface UploadedPart {

2+ partNumber: number;

3+ eTag: string;

4+}

6+export interface UploadSession {

7+ uploadId: string;

8+ accountId: string;

9+ objectKey: string;

10+ status: "uploading" | "completed" | "aborted";

11+ uploadedParts: readonly UploadedPart[];

12+}

13+

14+export interface CompletedObject {

15+ objectKey: string;

16+ versionId: string;

17+}

18+

19+export interface UploadRepository {

20+ find(uploadId: string): Promise<UploadSession | null>;

21+ markCompleted(uploadId: string, result: CompletedObject): Promise<void>;

22+}

23+

24+export interface MultipartObjectStore {

25+ complete(

26+ uploadId: string,

27+ objectKey: string,

28+ parts: readonly UploadedPart[],

29+ ): Promise<CompletedObject>;

30+}

Reference solution

The findings below are ordered by priority.

Findings

1. Any account can complete any known upload in any state

Location: src/complete-multipart-upload.ts:20-24

The service checks only that the upload exists. It never compares request.accountId with session.accountId and never requires an uploading state, so a caller can finalize another account's upload or re-complete an aborted session.

Authorize against the stored account before exposing session details, then enforce the allowed state transition. Handle a completed session only through the explicitly defined exact-manifest idempotent replay path.

2. The requested manifest is not compared with recorded parts

Location: src/complete-multipart-upload.ts:26-32

Building a Map silently lets the last duplicate part win, while no code consults session.uploadedParts. Missing parts, unexpected parts, out-of-range or fractional numbers, and mismatched ETags can therefore be sent to storage as if they were the durable upload manifest.

Validate part-number range and integrality, reject duplicates, and compare the complete request one-for-one with the recorded part set before calling storage.

3. ETags are corrupted before completion

Location: src/complete-multipart-upload.ts:28-30

The code trims, removes every quote, and lowercases each ETag. Because ETags are opaque and case-sensitive, the value sent to storage can differ from both the client's request and the recorded successful upload, causing rejection or assembling the wrong manifest semantics.

Compare and forward each ETag exactly as recorded, without textual normalization.

4. Part numbers are sorted as strings

Location: src/complete-multipart-upload.ts:34-36

localeCompare orders the string forms, producing 1, 10, 2 rather than ascending part numbers. Storage receives an invalid assembly order once an upload reaches two-digit parts.

Sort by numeric subtraction after all part numbers have passed integer and range validation.

5. Local completion is recorded before object storage succeeds

Location: src/complete-multipart-upload.ts:38-47

The repository is marked completed with a fabricated pending version before the storage operation begins. If storage rejects, local state is no longer retryable and downstream readers can observe an object version that never existed. Even on success, the actual returned version is never persisted.

Call the idempotent storage completion first while the session remains uploading, then atomically transition local state using the returned object key and version. Preserve the uploading state on storage failure.

6. The happy-path test avoids every manifest boundary

Location: src/complete-multipart-upload.test.ts:5-40

The fixture uses the owning account, an uploading session, two already ordered single-digit parts, normalized-looking ETags, and an immediately successful store. Its only interaction assertion cannot detect authorization, exact-set validation, numeric ordering, opaque ETags, or failure sequencing.

Add another account and terminal states; duplicate, missing, extra, invalid, and reordered parts including part 10; quoted mixed-case ETags; and a rejected completion that proves markCompleted was not called.

Reasonable non-findings

Sorting a validated copy instead of requiring clients to submit sorted parts is allowed.
Completing parts sequentially is not relevant here; individual part uploads finished before this boundary.
Object storage supplies idempotency for an exact upload ID and manifest, so adding a second provider idempotency token is unnecessary.