[zoewangg]

Motivation and Context

With the Java-based multipart S3 async client, S3TransferManager#downloadFile downloads parts in parallel and writes each part at its own offset in the destination file, so parts can complete out of order. When such a download is paused, the ResumableFileDownload lists the completed parts, and the in-memory multipart resume context records them so that resumeDownloadFile can continue part by part.

The resume context is kept in memory only and is not part of a serialized ResumableFileDownload. This change makes a resume from a serialized ResumableFileDownload that lists completed parts restart the download from the beginning, rather than continue from the end of the destination file. This matches how the CRT-based transfer manager already handles such a token.

Modifications

• ResumableRequestConverter:
  • A resume restarts from the beginning when the token lists completed parts but the request's MultipartDownloadResumeContext does not record them (completedPartsMissingFromResumeContext). A token whose resume context records its completed parts still continues with part GETs for the remaining parts, unchanged.
  • The resume logic in a private canResumeDownload(token, head, request). The existing public canResumeDownload(token, head) is renamed isObjectOrFileModified (with inverted result), and CrtS3TransferManager uses the new name.
  • Requests that restart from the beginning (newDownloadFileRequest) get a new, empty MultipartDownloadResumeContext.
  • The range-GET resume is extracted to resumeWithRangeGetIfPossible, with no behavior change.
  • hasUnresumableRange now delegates to computeResumedRange, so the CRT-based transfer manager's restart decision and the range it requests use the same rule. No behavior change.
• DefaultFileDownload and CrtFileDownload: when a resumed download is paused again before it has recorded any parts or written to the destination file, the new ResumableFileDownload keeps the completed parts of the token being resumed (ResumableRequestConverter.completedPartsToSaveOnPause), so the next resume also restarts from the beginning.

Testing

New and updated unit and functional tests (WireMock) in s3-transfer-manager:

• ResumeDownloadWireMockTest:
  • a serialized token with parts [1,3] or [1,4] completed restarts from part 1, and the file equals the object;
  • a resumed download that is paused again before the HEAD response or before the first part response, with the second token serialized or kept in memory, still restarts from part 1;
  • an in-memory token with parts completed out of order continues with only the remaining parts.
• ResumableRequestConverterTest: restart for completed parts without a resume context and with an empty one; remaining-parts resume with a resume context; range resume without completed parts; the restart request uses a new resume context and replaces the file; hasUnresumableRange cases.
• DefaultFileDownloadTest and CrtFileDownloadTest: a pause before the resumed download writes keeps the completed parts of the token being resumed, and a pause after the file was modified does not.

The new tests fail without the corresponding main-code changes. Module build: s3-transfer-manager (583 tests) passes. A full-repo mvn install was not run.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)

Checklist

• I have read the CONTRIBUTING document
• Local run of mvn install succeeds
• My code follows the code style of this project
• My change requires a change to the Javadoc documentation
• I have updated the Javadoc documentation accordingly
• I have added tests to cover my changes
• All new and existing tests passed
• I have added a changelog entry. Adding a new entry must be accomplished by running the scripts/new-change script and following the instructions. Commit the new file created by the script in .changes/next-release with your changes.
• My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

• I confirm that this pull request can be released under the Apache 2 license

[alextwoods]

Is there something else we could wait on here? Both to make this potentially faster and less flaky.... I've been working through trying to remove some of the sleep usage in our test suites. Maybe possible to poll progress bytestransfered or something from wiremock?

[zoewangg]

Good point, updated the tests to build a mock token and use that token to test re-pause behavior instead of calling pausing so that we don't need to sleep here

[alextwoods]

What does the "Held responses keep a server thread busy for their whole delay" mean practically? I'm slightly worried about the impact on the total wall clock time of these tests. Do the tests that are holding these end up adding a big chunk to our test time?

[zoewangg]

Updated the documentation: it basically means the server thread will be blocked for a stub configured withFixedDelay until the delay ends. By default the mock server only has 10 threads, so we are configuring 50 threads here to make sure we have enough server threads available for other tests.

The delay is actually needed to reproduce the edge case and it doesn't add a lot of test time. Measured locally, this class takes about 0.8 s for 13 tests (0.13-0.16 s for 7 on master), and the module total is about 43 s, in line with master's 40-43 s.

[joviegas]

Possible issue: if a resumed download is paused again before it writes anything, I think the new token can lose track of the zeros left in the file by the earlier pause. The next resume would then append to the file and report success, with part of the file still zeros.

I think it can happen like this (4 parts, 2 KB each):

1. The first pause happens after parts 1 and 3 finish. The local file is part 1, then zeros at bytes 2048-4095, then part 3. The token (T1) records 6144 bytes, the local file's modified time M1, and completedParts = [1, 3]. T1 is serialized.
2. While paused, something on the machine (touch, a backup or sync tool) changes the local file's modified time to M2. The contents and the S3 object don't change.
3. Resuming T1 sees M1 != M2 and restarts. This looks right to me.
4. pause() is called before the first response arrives. Here doPause reads the length and modified time from the local file as it is now (6144, M2). completedPartsToSaveOnPause drops T1's parts because the file changed since T1. So the new token (T2) is 6144 / M2 / [].
5. Resuming T2 finds nothing changed and no parts, so it sends Range: bytes=6144- and appends. Bytes 2048-4095 stay zeros.

Can you test this scenario? I think resumeMultipartDownload_restartRePausedBeforeWritingAnything_shouldStillRestartFromFirstPart covers it with one extra line after serializedTokenWithCompletedParts(...):

destination.toFile().setLastModified(destination.toFile().lastModified() + 5_000);

If it is an issue, I think the fix is: when the resumed download hasn't written anything yet, the new token copies the old token's state (length, modified time, completed parts) instead of reading the current file. The next resume then sees the same mismatch and restarts again. A missing sdkResponse on the progress snapshot looks like a usable "nothing written yet" signal, since it is set when the first response headers arrive, before any bytes are written. CrtFileDownload builds its token the same way, so I think it needs the same change.

WDYT?

[zoewangg]

Good catch, was able to confirm it. Updated DefaultFileDownload and CrtFileDownload to compare the file mtime and length when pause is invoked and return the same token as the previous one.

[github-actions]

This pull request has been closed and the conversation has been locked. Comments on closed PRs are hard for our team to see. If you need more assistance, please open a new issue that references this one.