Fix file upload retry reusing an already-consumed stream body

[mchanDev]

Description

File uploads (vip app deploy, media/SQL imports) stream the file from disk as the
fetch request body. fetchWithRetry() retried failed requests by re-calling
fetch with the same init object. Because a Node stream can only be consumed
once, the retry handed undici an already-consumed (disturbed/locked) stream, which
threw:

`Error: Response body object should not be disturbed or locked`

This masked the real, underlying network error (e.g. a connection reset partway
through the upload) and made the retry useless — the retry could never succeed.

This change makes fetchWithRetry() accept an optional body factory that produces a
fresh body for each attempt, and uploadUsingPutObject() now passes one (recreating
the read stream + progress pass-through per attempt). As a safety net, if a stream
body is supplied without a factory, the request is attempted only once so the true
error surfaces instead of the misleading undici message.

Observed in CI with VIP-CLI v4.0.5 on Node v24.16.0, where the upload stalled at 90%
and failed with the message above.

Changelog Description

Fixed

• Fixed file uploads failing with "Response body object should not be disturbed or locked" when an upload request was retried, and ensured retries re-stream the file instead of reusing a consumed stream.

Pull request checklist

• Update SETUP.md with any new environmental variables. (n/a — no new env vars)
• Update the documentation. (n/a)
• Manually test the relevant changes.
• Follow the pull request checklist.
• Add/update automated tests as needed.

Steps to Test

1. Check out PR.
2. Run npm ci.
3. Run npm test -- __tests__/lib/client-file-uploader.js — the new
   fetchWithRetry() suite passes (retry recreates the body; a stream body with no
   factory is attempted once; non-stream bodies still retry; last error propagates).
4. (Optional end-to-end) Run vip app deploy <file> against a flaky connection; a
   transient failure now retries the upload from a fresh stream instead of throwing
   "Response body object should not be disturbed or locked".

[github-actions]

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

[Copilot]

Pull request overview

This PR fixes a retry failure mode in src/lib/client-file-uploader where upload requests that stream files from disk would retry using an already-consumed stream body, causing undici to throw a misleading “disturbed or locked” error and preventing retries from ever succeeding.

Changes:

• Updated fetchWithRetry() to support an optional body factory (createBody) that generates a fresh request body for each attempt, and to avoid retrying one-shot stream bodies when no factory is provided.
• Updated uploadUsingPutObject() to pass a body factory that recreates the file read stream (and progress passthrough) per retry.
• Added unit tests covering stream vs non-stream retry behavior and body recreation.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
src/lib/client-file-uploader.ts	Adds stream-safe retry support via a body factory and updates PutObject uploads to recreate stream bodies per attempt.
__tests__/lib/client-file-uploader.js	Adds coverage for the new fetchWithRetry() behavior (body recreation, stream no-retry safety net, and error propagation).

[sjinks]

Thanks for tracking this down — the root cause analysis and the single-PUT fix are correct. I reproduced the exact failure locally (undici 7.25.0, the version shipped in @automattic/vip@4.0.5): a transient mid-upload reset throws ECONNRESET on attempt 0, and the retry with the same consumed stream throws Response body object should not be disturbed or locked — character-for-character the reported error. The createBody factory recovers correctly (attempt 0 ECONNRESET → attempt 1 fresh stream → 200). 👍

One scope concern before this closes out PLTFRM-2491: the customer's reported case looks like the multipart path, which this PR doesn't convert.

• MULTIPART_THRESHOLD = 32 * MB. The attached archive develop-built-files.zip is 90,757,361 bytes (~86.6 MB), and it's already a .zip (isCompressed = true), so the gzip step is skipped and the size stays ~86.6 MB.
• 86.6 MB ≥ 32 MB → multipart (~6 parts) via uploadUsingMultipart → uploadParts → uploadPart, not uploadUsingPutObject.
• uploadPart still does fetchOptions.body = createReadStream(fileName, { start, end }).pipe(progressPassThrough) and calls fetchWithRetry(url, fetchOptions) without a createBody factory. With this PR, that path hits maxAttempts = 0 (stream body + no factory) → it's attempted once. That's a real improvement (the misleading error is replaced by the true network error), but it does not restore retry/recovery for the multipart case — which is the one the ~86 MB archive actually exercises.

So for the customer's 86 MB deploy on a flaky link, this PR likely surfaces a clearer ECONNRESET but the upload would still fail without retrying.

Two asks:

1. Could you confirm whether the manual repro ("stalled at 90%") used a <32 MB file (single-PUT) or the provided ~86 MB archive (multipart)? That determines whether the verified path matches the customer's.
2. Consider extending the same body-factory treatment to uploadPart. It's a bit more involved because the per-part progressPassThrough is created in uploadParts' parts.map(async part => …) and passed in, so the factory needs to recreate both the ranged read stream and the per-part PassThrough (and rewire progress aggregation).

For context, this stream-reuse defect predates the undici migration: I reproduced the same pattern on the old node-fetch@2 + fetch-retry@5 stack and the retries sent truncated (~1.9 MB of 2 MB) then 0-byte bodies and failed with a generic FetchError: network timeout. node-fetch just failed quietly where undici (stricter on Node 24) now fails loudly — so "works on 4.0.0" reflects a first attempt that didn't need a retry, not a working retry.

[sjinks]

Follow-up: I pushed commit 6cd163a to this branch to close the multipart scope gap I flagged above.

What it does: uploadPart now forwards a per-attempt createBody factory to fetchWithRetry(url, opts, 3, createBody) instead of building fetchOptions.body once — mirroring the single-PUT uploadUsingPutObject path. So multipart part uploads now actually retry (3 attempts) with a fresh body, rather than maxAttempts = 0 (single attempt).

Progress handling: the per-part factory recreates createReadStream(fileName, { start, end }).pipe(new PassThrough()) and resets that part's counters on each attempt, so a retry re-streams the part without reusing a consumed stream and without double-counting bytes. Aggregate progress is now summed from a per-part partBytesRead[] array, which stays correct across concurrent parts and retries.

getSignedUploadRequestData is still called once per part (outside fetchWithRetry), so the presigned URL is reused across attempts — same as the single-PUT path.

Tests: added a uploadParts() suite (mocking undici fetch and ../lib/api/http):

• multi-part upload aggregates progress and never exceeds 100%;
• a transient ECONNRESET on attempt 0 recovers on retry (fetch ×2, http ×1) with final progress exactly 100% — proving the per-attempt counter reset prevents 200% double-counting.

Verification: check-types (tsc) clean, eslint clean, npm run build (babel) OK, and the __tests__/lib/ suite is green (33 suites / 357 tests, including the new cases).

This matters because the reported customer archive (~86.6 MB .zip) exceeds MULTIPART_THRESHOLD (32 MB), so it goes through uploadPart, not uploadUsingPutObject — without this, the PR would only have converted the misleading error into the real one for that case, not restored retry/recovery.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud