Repository navigation
Conversation
![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 high-severity reliability gap in per-file upload error isolation.
What is this? | From workflow: PR Review
Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.
| }, async (upload, token) => | ||
| { | ||
| var operation = addPaths.Contains(upload.LocalPath) ? "add" : "update"; | ||
| var fileSize = context.WriteFileSystem.FileInfo.New(upload.LocalPath).Length; |
There was a problem hiding this comment.
[HIGH] Per-file failure isolation does not cover pre-upload file I/O
Apply now reports per-file upload failures via collector.EmitError, but the try/catch only starts at UploadAsync. If a planned file is removed/unreadable between planning and apply, FileInfo.New(upload.LocalPath).Length throws before that catch, which escapes Parallel.ForEachAsync and aborts the whole apply.
Please wrap the full per-file lambda body (metadata read, request construction, and upload call) in the same non-cancellation catch so a single missing/corrupt file is recorded as an error and the batch continues.
e432709 to
3f6802d
Compare
3f6802d to
a4f503c
Compare
Docs preview (local build)Handbook preview: https://docs-v3-preview.elastic.dev/elastic/docs-builder/pull/4208/ |
There was a problem hiding this comment.
Requesting changes: this introduces a destructive partial-deploy path where deletes can run even after upload failures in the same apply run.
What is this? | From workflow: PR Review
Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.
Comments that could not be inline-anchored
src/services/Elastic.Documentation.Deploying/Synchronization/AwsS3SyncApplyStrategy.cs:161
[HIGH] Deletes proceed even after upload failures
Apply now catches per-file upload exceptions and records them, but it still always calls Delete afterward. That can remove still-serving objects even when their replacement upload failed.
Concrete failure scenario: a page rename becomes add(new-path) + delete(old-path). If the add fails (for example transient S3 error), this method still executes deletes, so old-path is removed and the page disappears until a later successful dep…
fe06658 to
e86b8c2
Compare
There was a problem hiding this comment.
Requesting changes because the new tests currently fail to compile in this project.
What is this? | From workflow: PR Review
Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.
| ); | ||
| } | ||
|
|
||
| [Fact] |
There was a problem hiding this comment.
[HIGH] New test attributes break this test project build
This test project is configured for TUnit (tests/Directory.Build.props references TUnit), but these new tests use [Fact], which is not available here. Running dotnet test tests/Elastic.Documentation.Deploying.Tests/ --nologo -v minimal fails with CS0246 on both new [Fact] lines (344 and 386), so CI cannot compile this PR.
Please switch both attributes to [Test] to match the rest of this file and the project’s test framework.
5f271c8 to
377b049
Compare
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>
82c8274 to
a98264e
Compare
|
Closed to retarget to main as a standalone PR. Content preserved in new PR. |
Replaces the single
TransferUtilityUploadDirectoryRequestin the assembler deploy path with per-fileParallel.ForEachAsync, isolates upload failures, and ensuresApplyreturnsfalsewhen any file fails.Affects: CI/CD · Deploying
Prompt summary: Refresh stale PR #3650 — the original parallel S3 upload implementation that never landed due to a mechanical rebase conflict. Move the two integration tests that were deleted from
tests-integration/into the newtests/Elastic.Documentation.Deploying.Tests/project.Why
AwsS3SyncApplyStrategy.Applycopies every file into a temp directory and issues 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 TUnit
[Test](matching the project's current test framework after main merged the TUnit migration) withCancellationToken.Nonerather than xUnit/TUnit context APIs.Verify
dotnet test tests/Elastic.Documentation.Deploying.Tests/Stack: 4 of 5, on top of #4207
🤖 Generated with Claude Code