Repository navigation
Conversation
Wraps the per-file upload lambda in error handling and uploads add/update requests in parallel via Parallel.ForEachAsync. Tests migrated from xUnit [Fact] to TUnit [Test]. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes due to a correctness issue in delete-result handling that can report apply success when S3 per-key deletes fail.
What is this? | From workflow: PR Review
Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.
| _logger.LogError("Delete batch failed with status code {StatusCode}", response.HttpStatusCode); | ||
| foreach (var error in response.DeleteErrors) | ||
| var response = await s3Client.DeleteObjectsAsync(deleteObjectsRequest, token); | ||
| if (response.HttpStatusCode != System.Net.HttpStatusCode.OK) |
There was a problem hiding this comment.
[HIGH] Partial delete failures are treated as success
DeleteObjectsAsync can return 200 OK with one or more entries in DeleteErrors. This branch currently treats every 200 response as full success and increments deleteCount by the full batch size, so collector.Errors stays unchanged and Apply(...) can return true even when some keys failed to delete.
Please treat DeleteErrors as failures regardless of HTTP status, emit per-key errors, and only count successfully deleted keys.
Docs preview (local build)Handbook preview: https://docs-v3-preview.elastic.dev/elastic/docs-builder/pull/4238/ |
TestApply subscribes the in-memory exporter to the global activity source.
Any concurrent test that also calls Apply() emits activities that bleed into
TestApply's exported list, causing the tag-count assertions to see the wrong
plan's metrics.
Add [NotInParallel("deploying-apply")] to all tests in the deploying test
projects that exercise Apply() to prevent them running concurrently with
each other.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…TTP status DeleteObjectsAsync returns 200 OK even when individual keys fail; errors are in DeleteErrors. Only counting batch.Length on a 200 response inflated the delete counter and let Apply() return true despite partial failures. Now errors from DeleteErrors are always emitted to the collector, and only successfully deleted keys count toward the progress total. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Requesting changes: please prevent delete operations from running when any upload in the same apply run fails.
What is this? | From workflow: PR Review
Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.
| _ = Interlocked.Increment(ref uploadedCount); | ||
| } | ||
| var directoryRequest = new TransferUtilityUploadDirectoryRequest | ||
| catch (Exception ex) when (ex is not OperationCanceledException) |
There was a problem hiding this comment.
[HIGH] Upload failures are swallowed, then deletes still execute
This catch turns per-file upload failures into diagnostics and continues, but Apply still proceeds to the delete phase. That can produce destructive partial deploys: if a plan includes add new-path + delete old-path (rename/move case), a failed upload of new-path can be followed by successful deletion of old-path, leaving content missing in S3.
Please gate deletes on upload success (for example, track upload-phase failures and return early before Delete when any upload failed).
AwsS3SyncApplyStrategy: main now stages uploads per Content-Type for directory uploads. This branch uploads each file on its own through a Parallel.ForEachAsync, 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. DocsSyncTests: kept the branch's delete-chunking and upload-failure tests, and rewrote main's Content-Type test to assert on per-file upload requests instead of staged directory uploads. IncrementalDeployRoundTripTests: main already serializes tests that share the Apply activity source with NotInParallel(nameof(AwsS3SyncApplyStrategy)). The branch's "deploying-apply" key is dropped in favor of that one so all of them land in a single constraint group. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![infra-vault-gh-plugin-prod[bot]](https://avatars.githubusercontent.com/u/6764390?s=60&v=4){kind=small}
| [Test] | ||
| [NotInParallel(nameof(AwsS3SyncApplyStrategy))] | ||
| public async Task TestApply_DeleteRequests_ChunksDeletesAtS3Limit() | ||
| { | ||
| IReadOnlyCollection<IDiagnosticsOutput> diagnosticsOutputs = []; | ||
| var collector = new DiagnosticsCollector(diagnosticsOutputs); | ||
| var moxS3Client = A.Fake<IAmazonS3>(); | ||
| var moxTransferUtility = A.Fake<ITransferUtility>(); | ||
| var fileSystem = new MockFileSystem(new MockFileSystemOptions | ||
| { | ||
| CurrentDirectory = Path.Join(Paths.WorkingDirectoryRoot.FullName, ".artifacts", "assembly"), | ||
| }); | ||
| var configurationContext = TestHelpers.CreateConfigurationContext(fileSystem); | ||
| var config = AssemblyConfiguration.Create(configurationContext.ConfigurationFileProvider); | ||
| var assembleFs = CheckoutsFileSystem.FromWorkingDirectory(fileSystem); | ||
| var context = new AssembleContext(config, configurationContext, "dev", collector, assembleFs); | ||
| var plan = new SyncPlan | ||
| { | ||
| RemoteListingCompleted = true, | ||
| DeleteThresholdDefault = null, | ||
| ExcludePatterns = [], | ||
| TotalRemoteFiles = 1001, | ||
| TotalSourceFiles = 1, | ||
| TotalSyncRequests = 1001, | ||
| AddRequests = [], | ||
| UpdateRequests = [], | ||
| SkipRequests = [], | ||
| DeleteRequests = Enumerable.Range(0, 1001).Select(i => new DeleteRequest { DestinationPath = $"docs/delete-{i}.md" }).ToArray() | ||
| }; | ||
| var deleteBatches = new ConcurrentBag<DeleteObjectsRequest>(); | ||
| A | ||
| .CallTo(() => moxS3Client.DeleteObjectsAsync(A<DeleteObjectsRequest>._, A<Cancel>._)) | ||
| .Invokes((DeleteObjectsRequest request, Cancel _) => deleteBatches.Add(request)) | ||
| .Returns(new DeleteObjectsResponse { HttpStatusCode = System.Net.HttpStatusCode.OK }); | ||
|
|
||
| var applier = new AwsS3SyncApplyStrategy(new LoggerFactory(), moxS3Client, moxTransferUtility, "fake", context, collector); | ||
|
|
||
| await applier.Apply(plan, Cancel.None); | ||
|
|
||
| deleteBatches.Count.Should().Be(2); |
There was a problem hiding this comment.
Severity: P3 (Low)
The delete path changed: DeleteErrors is now read even when the response is 200 (S3 partial failure), and exceptions from a batch are caught and turned into per-key errors. No test covers this. The new delete test only returns empty OK responses and never checks the return value of Apply. Please add a test where DeleteObjectsAsync returns DeleteErrors (and one where it throws) and assert that Apply returns false and the collector's error count is correct.
Generated by Libra. Specialist: libra-complexity. React with 👍 or 👎 to give feedback on this comment.
| TotalRemoteFiles = 0, | ||
| TotalSourceFiles = 1, | ||
| TotalSyncRequests = 1, | ||
| AddRequests = [new AddRequest { LocalPath = Path.Join(outputDirectory, "docs/add1.md"), DestinationPath = "docs/add1.md" }], |
There was a problem hiding this comment.
Severity: P3 (Low)
This failure test queues only one file, so it cannot verify per-file failure isolation. An implementation that returns false on the first error and skips all remaining uploads would still pass this test and the all-success tests. Add a mixed-success, multi-file case with asynchronous upload completions, and assert that healthy uploads finish while Apply returns false.
Generated by Libra. React with 👍 or 👎 to give feedback on this comment.
Replaces the single
TransferUtilityUploadDirectoryRequestin the assembler deploy path with per-fileParallel.ForEachAsync, isolates upload failures, and ensuresApplyreturnsfalsewhen any file fails.Affects: CI/CD · Deploying
Why
AwsS3SyncApplyStrategy.Applycopied every file into a temp directory and issued oneTransferUtilityUploadDirectoryRequest. That single-threaded path is the bottleneck in the assembler deploy step, and a failure in any one file is opaque — the upload either all-succeeds or throws with no per-file context.What
Parallel per-file upload
Parallel.ForEachAsyncwith a configurable semaphore replaces the single directory transfer.DOCS_SYNC_UPLOAD_CONCURRENCY(default 32) andDOCS_SYNC_DELETE_CONCURRENCY(default 4) tune the parallelism. Each file's upload and each delete batch run independently; a failure is caught, logged, and recorded —Applyreturnsfalseif any file fails rather than masking the error.Error isolation in the deploy service
IncrementalDeployServicewraps the per-file upload lambda in its own catch block so a single bad file does not abort the whole run.Tests
Two new tests exercise the delete-chunking at the S3 1000-key limit and the failure-return-false path. Both use
[Test]withCancellationToken.None.Verify
dotnet test tests/Elastic.Documentation.Deploying.Tests/Supersedes #4208.
🤖 Generated with Claude Code