Upgrade runner-binaries-syncer to aws sdk v3 #7078

clee2000 · 2025-09-02T17:14:10Z

yarn test is broken on main, you can see this in

test-infra/terraform-aws-github-runner/modules/runner-binaries-syncer/lambdas/runner-binaries-syncer/Makefile

Line 15 in a32b8f6

# yarn test

Testing:
Stole some environment variables from the lambda and mangled the key to upload to a dummy key then ran yarn build; cd dist;node -e 'require("./index").handler();' > t.log
Saw that it uploaded a file, and skipped some because they didn't need to be uploaded.
The one that was uploaded was arm64, which I'm thinking was uploaded manually since it lacks a tag on s3.

I had to add an await since it wasn't working, which I think is a bug in the original code

vercel · 2025-09-02T17:14:17Z

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment

Project	Deployment	Preview	Updated (UTC)
torchci	Ignored	Preview	Sep 3, 2025 8:05pm

...ner/modules/runner-binaries-syncer/lambdas/runner-binaries-syncer/src/syncer/handler.test.ts

clee2000 · 2025-09-02T17:16:05Z

...b-runner/modules/runner-binaries-syncer/lambdas/runner-binaries-syncer/src/syncer/handler.ts

-    Tagging: versionKey + '=' + actionRunnerReleaseAsset.name,
-    Body: writeStream,
-  }).promise();
+  const upload = new Upload({


The original upload function did multipart uploads automatically, but there is no upload function anymore, and putObject doesn't to multipart uploads. This doesn't matter if the file is small but idk how large the file is, so I use this to handle multipart uploads automatically. Code taken from chatgpt

clee2000 · 2025-09-02T21:09:15Z

...aws-github-runner/modules/runner-binaries-syncer/lambdas/runner-binaries-syncer/package.json

  "dependencies": {
+    "@aws-sdk/client-s3": "^3.879.0",
+    "@aws-sdk/lib-storage": "^3.879.0",
+    "@vercel/ncc": "^0.38.3",


was getting build problems so I also swapped the ncc dependency. the other lambdas use @vercel, maybe not this version tho

This is more than likely correct

seemethere

LGTM

seemethere · 2025-09-02T21:13:30Z

...rm-aws-github-runner/modules/runner-binaries-syncer/lambdas/runner-binaries-syncer/yarn.lock

    "@jridgewell/gen-mapping" "^0.3.5"
    "@jridgewell/trace-mapping" "^0.3.24"

+"@aws-crypto/crc32@5.2.0":


nit: I wonder if we can add these to the linguist file at the root to note that these are generated files and should be automatically minimized in PR reviews.

This is unrelated to the migration work but just a thought I was having.

seemethere · 2025-09-02T21:13:50Z

...aws-github-runner/modules/runner-binaries-syncer/lambdas/runner-binaries-syncer/package.json

  "dependencies": {
+    "@aws-sdk/client-s3": "^3.879.0",
+    "@aws-sdk/lib-storage": "^3.879.0",
+    "@vercel/ncc": "^0.38.3",


This is more than likely correct

ZainRizvi

Looks reasonable. Is this testable in canary?

Copilot

Pull Request Overview

This PR upgrades the runner-binaries-syncer Lambda function from AWS SDK v2 to v3 to fix broken tests on the main branch. The changes modernize the S3 client usage and dependencies while maintaining the same functionality.

Replaces AWS SDK v2 imports and usage with v3 equivalents
Updates S3 client initialization and API calls to use the new SDK patterns
Migrates from @zeit/ncc to @vercel/ncc bundler

Reviewed Changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.

File	Description
src/syncer/handler.ts	Updates S3 client imports, initialization, and API calls to use AWS SDK v3 patterns
package.json	Replaces AWS SDK v2 dependencies with v3 equivalents and updates bundler

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-03T19:10:34Z

...b-runner/modules/runner-binaries-syncer/lambdas/runner-binaries-syncer/src/syncer/handler.ts

  }).catch((error) => {
    console.error(`Exception: ${error}`);
  });
+  await uploadPromise;


The uploadPromise is awaited after the request stream has already finished, which could cause the upload to complete before all data is written. Move await uploadPromise; inside the Promise callback after writeStream.end() to ensure proper sequencing.

Copilot · 2025-09-03T19:10:34Z

...b-runner/modules/runner-binaries-syncer/lambdas/runner-binaries-syncer/src/syncer/handler.ts

+    const versions = objectTagging.TagSet?.filter((t: Tag) => t.Key === versionKey);
+    return versions?.length === 1 ? versions[0].Value : undefined;


The optional chaining versions?.length is inconsistent with the original logic. If versions is undefined, the condition should return undefined, but if it's an empty array, it should also return undefined. The original code versions.length === 1 would throw if versions is undefined, which may have been the intended behavior.

Suggested change

const versions = objectTagging.TagSet?.filter((t: Tag) => t.Key === versionKey);

return versions?.length === 1 ? versions[0].Value : undefined;

if (!objectTagging.TagSet) {

throw new Error('TagSet is undefined');

}

const versions = objectTagging.TagSet.filter((t: Tag) => t.Key === versionKey);

return versions.length === 1 ? versions[0].Value : undefined;

clee2000 · 2025-09-03T20:08:48Z

...b-runner/modules/runner-binaries-syncer/lambdas/runner-binaries-syncer/src/syncer/handler.ts

    console.debug('latest: ' + currentVersion);
    if (currentVersion === undefined || currentVersion != actionRunnerReleaseAsset.name) {
-      uploadToS3(s3, cacheObject, actionRunnerReleaseAsset);
+      await uploadToS3(s3, cacheObject, actionRunnerReleaseAsset);


Add an await since uploadToS3 is an async function and is in a for loop, so this should wait for the upload to finish. Alt: add to a list of promises and await at the end

everything?

5f74300

meta-cla bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 2, 2025

clee2000 force-pushed the csl/aws_sdk_runner_binaries branch from 1aae26f to 5f74300 Compare September 2, 2025 17:14

clee2000 commented Sep 2, 2025

View reviewed changes

...ner/modules/runner-binaries-syncer/lambdas/runner-binaries-syncer/src/syncer/handler.test.ts Outdated Show resolved Hide resolved

clee2000 commented Sep 2, 2025

View reviewed changes

clee2000 added 3 commits September 2, 2025 10:44

uncomment tests, theyre not run in CI anyways

2b4008f

tc

2eff324

switch to newer ncc?

06475d8

clee2000 marked this pull request as ready for review September 2, 2025 20:16

clee2000 requested a review from a team September 2, 2025 20:16

clee2000 commented Sep 2, 2025

View reviewed changes

seemethere approved these changes Sep 2, 2025

View reviewed changes

atalman approved these changes Sep 2, 2025

View reviewed changes

ZainRizvi approved these changes Sep 3, 2025

View reviewed changes

ZainRizvi requested a review from Copilot September 3, 2025 19:09

Copilot AI reviewed Sep 3, 2025

View reviewed changes

add await

184dfb1

clee2000 commented Sep 3, 2025

View reviewed changes

clee2000 merged commit 200cb0c into main Sep 5, 2025
5 checks passed

clee2000 deleted the csl/aws_sdk_runner_binaries branch September 5, 2025 15:32

clee2000 mentioned this pull request Sep 10, 2025

Upgrade AWS lambda functions from version 2.x to 3.x of the AWS SDK for JavaScript pytorch/pytorch#137228

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Upgrade runner-binaries-syncer to aws sdk v3 #7078

Upgrade runner-binaries-syncer to aws sdk v3 #7078

Uh oh!

clee2000 commented Sep 2, 2025 •

edited

Loading

Uh oh!

vercel bot commented Sep 2, 2025 •

edited

Loading

Uh oh!

Uh oh!

clee2000 Sep 2, 2025 •

edited

Loading

Uh oh!

clee2000 Sep 2, 2025

Uh oh!

seemethere Sep 2, 2025

Uh oh!

seemethere left a comment

Uh oh!

seemethere Sep 2, 2025

Uh oh!

seemethere Sep 2, 2025

Uh oh!

ZainRizvi left a comment •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Sep 3, 2025

Uh oh!

Copilot AI Sep 3, 2025

Uh oh!

clee2000 Sep 3, 2025

Uh oh!

Uh oh!

Uh oh!

		const versions = objectTagging.TagSet?.filter((t: Tag) => t.Key === versionKey);
		return versions?.length === 1 ? versions[0].Value : undefined;

-    const versions = objectTagging.TagSet?.filter((t: Tag) => t.Key === versionKey);
-    return versions?.length === 1 ? versions[0].Value : undefined;
+    if (!objectTagging.TagSet) {
+      throw new Error('TagSet is undefined');
+    }
+    const versions = objectTagging.TagSet.filter((t: Tag) => t.Key === versionKey);
+    return versions.length === 1 ? versions[0].Value : undefined;

Upgrade runner-binaries-syncer to aws sdk v3 #7078

Upgrade runner-binaries-syncer to aws sdk v3 #7078

Uh oh!

Conversation

clee2000 commented Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vercel bot commented Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

clee2000 Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clee2000 Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

seemethere Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

seemethere left a comment

Choose a reason for hiding this comment

Uh oh!

seemethere Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

seemethere Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

ZainRizvi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

clee2000 Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

clee2000 commented Sep 2, 2025 •

edited

Loading

vercel bot commented Sep 2, 2025 •

edited

Loading

clee2000 Sep 2, 2025 •

edited

Loading

ZainRizvi left a comment •

edited

Loading