Repository navigation
Conversation
publisher-s3 uploaded every artifact in one Promise.all, so RELEASES and RELEASES.json could land before the .nupkg/.zip files they list. Clients that polled in that window saw a release whose files were missing. If a package upload failed, the feed was already pointing at a file that never arrived. Upload the other artifacts first and the RELEASES files after all of them have finished. Concurrency within each group is unchanged, and so is the config.
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| await Promise.all( | ||
| artifacts | ||
| .filter((artifact) => !artifact.isReleaseFile) | ||
| .map(uploadArtifact), | ||
| ); | ||
| await Promise.all( | ||
| artifacts | ||
| .filter((artifact) => artifact.isReleaseFile) | ||
| .map(uploadArtifact), | ||
| ); |
There was a problem hiding this comment.
🟡 (optional) On a multi-platform release, a fast platform's manifest now waits on unrelated platforms' uploads to finish, delaying updates it didn't need to wait for. artifacts in publish() contains every platform/arch combined (release.ts calls publisher.publish() once per publisher with all makeResults), but the grouping at PublisherS3.ts:119-126 splits purely on isReleaseFile, not per platform/keyPrefix. So mac's RELEASES.json now blocks on a slow or failing Windows .nupkg upload even though they're unrelated. Before this change each upload resolved independently, so a fast platform's manifest could go live without waiting on a slower one. Fix: gate each release file's upload only on the non-release artifacts sharing its platform/arch (or keyPrefix), not on every artifact globally.
Why this was flagged
A release build produces artifacts for win32, darwin and linux in one make pass; release.ts:158-163 invokes publisher.publish() once with all of them in makeResults. PublisherS3.ts:119-122 uploads every non-release artifact across all platforms concurrently, then PublisherS3.ts:123-126 waits for that whole batch before uploading ANY RELEASES/RELEASES.json, including ones for platforms whose own artifacts already finished. On the base branch each artifact's upload resolved independently (single Promise.all, no cross-artifact wait), so a fast platform's manifest could publish without waiting on an unrelated slow/failing one. No existing check scopes the wait to matching platform/arch.
Verification: nit. The coupling is real and reachable. release.ts:158-163 calls publisher.publish({ makeResults }) exactly once with all platforms/arches combined, and PublisherS3.ts:46-59 folds every make result's artifacts into a single artifacts array. The new two-phase upload (HEAD PublisherS3.ts:118-121 uploads artifacts.filter(a => !a.isReleaseFile), then 123-126 uploads `artifacts.filter(a =>…
There was a problem hiding this comment.
Scoped the wait per platform and arch in b1224ea. Artifacts are grouped by key prefix, platform and arch, each group uploads its RELEASES files after its own artifacts, and the groups run in parallel. Added a spec for a darwin x64 + arm64 publish where arm64's RELEASES.json goes out while the x64 zip is still uploading.
A RELEASES file lists the artifacts of its own platform and arch, so it only needs to wait for those. Waiting for every artifact in the publish coupled unrelated targets: in a darwin x64 + arm64 build, arm64's RELEASES.json waited for the x64 zip, and a failed x64 upload held arm64 back as well. Group the artifacts by key prefix, platform and arch. Each group uploads its other artifacts first and its RELEASES files after them, and the groups run in parallel.
Summarize your changes:
publisher-s3uploads every artifact in a singlePromise.all, soRELEASESandRELEASES.jsoncan land before the.nupkg/.zipfiles they list. In practice they usually do, becausemaker-squirrellistsRELEASESfirst and it is by far the smallest file. A client that polls in that window sees a release whose packages are not there yet. If a package upload fails, the feed points at a file that never arrived until someone re-runs the publish.For each platform and arch, this change uploads the other artifacts first, and the
RELEASESfiles only after every one of them has succeeded. Different platforms and arches don't wait on each other, and the config is unchanged. Most of the diff inPublisherS3.tsis re-indentation from moving the upload body into a localuploadArtifactfunction. I also added one sentence about the ordering to the S3 publisher docs.Three new specs:
RELEASESfiles are uploaded only after every other artifact of the same platform and arch has finishedRELEASESfiles don't wait on another's uploads (darwin x64 + arm64)RELEASESfile is uploaded when an artifact upload failsVerified locally on Windows with Node 22:
yarn test:fast(66 files, 660 passed),tsc -b packages,oxfmt --checkandoxlint(no new warnings).publisher-gcshas the same singlePromise.all. Happy to make the same change there in this PR or in a follow-up, whichever you prefer.Context: I noticed this while writing an Azure Blob Storage publisher on top of
publisher-static, which uploads the release files last for the same reason.