Sync: upload/delete S3 files concurrently, not one directory call by reakaleek · Pull Request #3650 · elastic/docs-builder

reakaleek · 2026-07-13T16:17:54Z

Why

S3 sync during deploy is network-bound, not CPU-bound: the old apply strategy copied every file to a temp directory and issued a single TransferUtilityUploadDirectoryRequest, whose concurrency was tied to CPU count via TransferUtilityConfig.ConcurrentServiceRequests. For a docs-site sync with many small files, that under-utilizes available network concurrency and wastes a full local read+write pass per file just to stage a directory that already matched the S3 key layout. A single failed upload/delete could also abort the whole batch.

What

Uploads and deletes now run per-file/per-batch in parallel via Parallel.ForEachAsync, with concurrency configurable through DOCS_SYNC_UPLOAD_CONCURRENCY (default 32, max 128) and DOCS_SYNC_DELETE_CONCURRENCY (default 4, max 16). Each file is uploaded directly from its original path with an explicit S3 key — no temp-directory staging. Failures are now isolated and reported per-file/per-batch through the diagnostics collector instead of aborting the whole sync, and AwsS3SyncApplyStrategy.Apply returns false if any file failed (previously the caller always reported success). The default AmazonS3Client also now configures SDK-level retry (RequestRetryMode.Standard, MaxErrorRetry = 5) to absorb throttling under the higher concurrency.

Test plan

dotnet build succeeds
Integration tests pass (DocsSyncTests, IncrementalDeployRoundTripTests), including new tests covering delete-batch chunking and upload-failure propagation

S3 sync is network-bound, not CPU-bound, so uploads and deletes now run in parallel per-file/per-batch (configurable via DOCS_SYNC_UPLOAD_CONCURRENCY and DOCS_SYNC_DELETE_CONCURRENCY) instead of copying every file to a temp directory and issuing a single directory upload sized to CPU count. Failures are now reported per-file/per-batch via the diagnostics collector instead of aborting the whole sync, and Apply returns false if any file failed. The S3 client also configures SDK-level retry (standard mode) to absorb throttling under the higher concurrency. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…sync-w/o-bound-not-cpu-bound-opportunity-most-systems-ca

Mpdreamz

LGTM, let's make sure we isolate this in a release (EMG a release only containing this change before releasing.

That way we can isolate the change and see it on edge and staging and rollback roll forward.

Refreshes [#3650](#3650), rebased onto main with tests moved to the new project location. The assembler's incremental-deploy upload path copied every file into a temp directory and then issued one `TransferUtilityUploadDirectoryRequest`. That serialised every upload behind a single SDK call and made per-file failures invisible — `Apply` always returned `true` regardless. Each file is now uploaded with its own `TransferUtilityUploadAsync` call, run via `Parallel.ForEachAsync` at a configurable degree of concurrency (`DOCS_SYNC_UPLOAD_CONCURRENCY`, default 32, max 128). Delete batches are also parallelised (`DOCS_SYNC_DELETE_CONCURRENCY`, default 4, max 16). `Apply` now returns `false` when any upload or delete fails. The `IncrementalDeployService` propagates this result to its caller instead of always returning `true` after apply. The `AmazonS3Client` is configured with `RetryMode = Standard` and `MaxErrorRetry = 5` so the SDK absorbs transient throttling under concurrent load before surfacing an error. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

AwsS3SyncApplyStrategy: main now stages uploads per Content-Type for directory uploads. This branch uploads each file on its own, so the branch's upload path stays and sets the spec Content-Type (SpecContentType) on each per-file request, which cannot leak headers onto other files. Main's single-line logger attributes are kept. DocsSyncTests: main moved the file to tests/Elastic.Documentation.Deploying.Tests and migrated it to TUnit. The branch's changes are re-applied there, and the Content-Type test now asserts on per-file upload requests. IncrementalDeployRoundTripTests: kept the branch's per-file upload assertions on top of main's formatting. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

The build's curb auto-format rewrote this branch-owned initializer after the merge. No behavior change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

infra-vault-gh-plugin-prod · 2026-11-04T18:36:50Z

 		// Record sync duration (both histogram for distribution and counter for total)
 		SyncDurationHistogram.Record(sw.Elapsed.TotalSeconds);
+
+		return collector.Errors == errorsBeforeApply;


Severity: P2 (Medium)

Upload failures are now caught and only reported, so Delete(plan, ctx) runs even after uploads fail. Before this change, a failed UploadDirectoryAsync threw and stopped the sync before any deletes. Now a deploy where uploads fail (for example, access denied, or throttling that outlasts the retries) still deletes the remote files that are missing from source. If a page moved, the old key gets deleted while the new key never arrived, so the live site serves 404s until a later deploy succeeds. Skip the delete phase when any upload failed, for example by checking collector.Errors after Upload returns.

_{Generated by Libra. Specialist: libra-complexity. React with 👍 or 👎 to give feedback on this comment.}

infra-vault-gh-plugin-prod · 2026-11-04T18:36:50Z

+			{
+				var operation = addPaths.Contains(upload.LocalPath) ? "add" : "update";
+				var fileSize = context.WriteFileSystem.FileInfo.New(upload.LocalPath).Length;
+				var extension = Path.GetExtension(upload.DestinationPath).ToLowerInvariant();


Severity: P3 (Low)

Move the per-file metadata lookup inside the upload's exception handler. If a source file disappears between plan and apply, accessing Length throws before the try; Parallel.ForEachAsync then cancels the remaining workers and Apply throws instead of recording the file error and returning false. This leaves local-file failures outside the new failure-isolation behavior.

_{Generated by Libra. React with 👍 or 👎 to give feedback on this comment.}

infra-vault-gh-plugin-prod · 2026-11-04T18:36:50Z

+					if (response.HttpStatusCode != System.Net.HttpStatusCode.OK)
+					{
+						_logger.LogError("Delete batch failed with status code {StatusCode}", response.HttpStatusCode);
+						if (response.DeleteErrors is null or { Count: 0 })
+							collector.EmitGlobalError($"Delete batch failed with status code {response.HttpStatusCode}");
+						foreach (var error in response.DeleteErrors ?? Enumerable.Empty<DeleteError>())
+						{
+							_logger.LogError("Failed to delete {Key}: {Message}", error.Key, error.Message);
+							collector.EmitError(error.Key, $"Failed to delete: {error.Message}");
+						}
+					}
+					else
 					{
-						_logger.LogError("Failed to delete {Key}: {Message}", error.Key, error.Message);
-						collector.EmitError(error.Key, $"Failed to delete: {error.Message}");
+						var currentCount = Interlocked.Add(ref deleteCount, batch.Length);
+						_logger.LogInformation(
+							"Deleted {BatchCount} files ({CurrentCount}/{TotalCount})",
+							batch.Length,
+							currentCount,
+							deleteRequests.Count
+						);
 					}
 				}


Severity: P3 (Low)

The new Apply contract says it returns false if any file failed to delete, but this branch only reads DeleteErrors when the HTTP status is not 200. S3's DeleteObjects returns 200 even when some keys fail and lists those keys in DeleteErrors. In that case the code counts the whole batch as deleted, emits no errors, and Apply returns true even though some deletes failed. Check response.DeleteErrors in the 200 case as well.

_{Generated by Libra. Specialist: libra-complexity. React with 👍 or 👎 to give feedback on this comment.}

infra-vault-gh-plugin-prod · 2026-11-04T18:36:50Z

+	[Test]
+	public async Task TestApply_DeleteRequests_ChunksDeletesAtS3Limit()


Severity: P3 (Low)

Give both new apply tests the existing [NotInParallel(nameof(AwsS3SyncApplyStrategy))] annotation. They emit activities on the same process-wide source observed by TestApply, but currently can run alongside it. That test selects the first sync apply activity and asserts its counts, so capturing either new test's activity first makes the suite fail nondeterministically.

_{Generated by Libra. React with 👍 or 👎 to give feedback on this comment.}

infra-vault-gh-plugin-prod · 2026-11-04T18:36:50Z

+		deleteBatches.Count.Should().Be(2);
+		deleteBatches.Should().OnlyContain(r => r.Objects.Count <= 1000);


Severity: P3 (Low)

Add an asynchronous in-flight concurrency test for the new upload and delete loops. The current fakes complete immediately and the assertions only check requests and batch sizes, so replacing either loop with serial processing—or ignoring the configured concurrency limit—would still pass the suite. Use gated fake requests to verify that multiple operations overlap and that the configured maximum is respected.

_{Generated by Libra. React with 👍 or 👎 to give feedback on this comment.}

github-actions · 2026-11-04T18:43:08Z

Docs preview (local build)

Handbook preview: https://docs-v3-preview.elastic.dev/elastic/docs-builder/pull/3650/

reakaleek and others added 2 commits July 13, 2026 18:12

Fix xUnit1051 lint warning in new test

132bc44

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

reakaleek requested a review from a team as a code owner July 13, 2026 16:17

reakaleek added the enhancement label Jul 13, 2026

reakaleek requested a review from technige July 13, 2026 16:17

github-actions Bot deployed to assembler-preview July 13, 2026 16:18

View deployment

Merge branch 'main' into aws-s3-sync-concurrency-analysis-how-aws-s3-…

c4dba00

…sync-w/o-bound-not-cpu-bound-opportunity-most-systems-ca

github-actions Bot deployed to assembler-preview July 13, 2026 16:35

View deployment

Mpdreamz approved these changes Jul 13, 2026

View reviewed changes

This was referenced Sep 22, 2026

Replace Parcel with Vite for frontend builds #4205

Closed

Upload S3 files concurrently instead of one directory call #4208

Closed

Mpdreamz and others added 2 commits November 4, 2026 19:33

Format the S3 client initializer with curb

7de0333

The build's curb auto-format rewrote this branch-owned initializer after the merge. No behavior change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

github-actions Bot deployed to docs-preview November 4, 2026 18:34

View deployment

github-actions Bot deployed to assembler-preview November 4, 2026 18:35

View deployment

infra-vault-gh-plugin-prod Bot approved these changes Nov 4, 2026

View reviewed changes

Navigation Menu

Repository navigation

Sync: upload/delete S3 files concurrently, not one directory call - #3650

Sync: upload/delete S3 files concurrently, not one directory call#3650
reakaleek wants to merge 5 commits into
mainfrom
aws-s3-sync-concurrency-analysis-how-aws-s3-sync-w/o-bound-not-cpu-bound-opportunity-most-systems-ca

reakaleek commented Jul 13, 2026

Uh oh!

Mpdreamz left a comment

Uh oh!

infra-vault-gh-plugin-prod Bot Nov 4, 2026

Uh oh!

infra-vault-gh-plugin-prod Bot Nov 4, 2026

Uh oh!

infra-vault-gh-plugin-prod Bot Nov 4, 2026

Uh oh!

infra-vault-gh-plugin-prod Bot Nov 4, 2026

Uh oh!

infra-vault-gh-plugin-prod Bot Nov 4, 2026

Uh oh!

github-actions Bot commented Nov 4, 2026

Uh oh!

This branch was successfully deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		[Test]
		public async Task TestApply_DeleteRequests_ChunksDeletesAtS3Limit()

		deleteBatches.Count.Should().Be(2);
		deleteBatches.Should().OnlyContain(r => r.Objects.Count <= 1000);

Conversation

reakaleek commented Jul 13, 2026

Why

What

Test plan

Uh oh!

Mpdreamz left a comment

Choose a reason for hiding this comment

Uh oh!

infra-vault-gh-plugin-prod Bot Nov 4, 2026

Choose a reason for hiding this comment

Uh oh!

infra-vault-gh-plugin-prod Bot Nov 4, 2026

Choose a reason for hiding this comment

Uh oh!

infra-vault-gh-plugin-prod Bot Nov 4, 2026

Choose a reason for hiding this comment

Uh oh!

infra-vault-gh-plugin-prod Bot Nov 4, 2026

Choose a reason for hiding this comment

Uh oh!

infra-vault-gh-plugin-prod Bot Nov 4, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Nov 4, 2026

Docs preview (local build)

Uh oh!

This branch was successfully deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants