Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Sep 5, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5f74300
everything?
clee2000 Sep 2, 2025
2b4008f
uncomment tests, theyre not run in CI anyways
clee2000 Sep 2, 2025
2eff324
tc
clee2000 Sep 2, 2025
06475d8
switch to newer ncc?
clee2000 Sep 2, 2025
184dfb1
add await
clee2000 Sep 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions ...-github-runner/modules/runner-binaries-syncer/lambdas/runner-binaries-syncer/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,6 @@
"@types/request": "^2.48.4",
"@typescript-eslint/eslint-plugin": "^4.17.0",
"@typescript-eslint/parser": "^4.17.0",
"@zeit/ncc": "^0.22.1",
"aws-sdk": "^2.863.0",
"eslint": "^7.22.0",
"jest": "^26.6.3",
"prettier": "^2.4.1",
Expand All @@ -31,6 +29,9 @@
"typescript": "^4.2.3"
},
"dependencies": {
"@aws-sdk/client-s3": "^3.879.0",
"@aws-sdk/lib-storage": "^3.879.0",
"@vercel/ncc": "^0.38.3",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is more than likely correct

"request": "^2.88.2",
"yn": "^4.0.0"
}
Expand Down
39 changes: 21 additions & 18 deletions ...unner/modules/runner-binaries-syncer/lambdas/runner-binaries-syncer/src/syncer/handler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import { Octokit } from '@octokit/rest';
import { PassThrough } from 'stream';
import request from 'request';
import { S3 } from 'aws-sdk';
import AWS from 'aws-sdk';
import { S3, Tag } from '@aws-sdk/client-s3';
import { Upload } from '@aws-sdk/lib-storage';
import yn from 'yn';

const versionKey = 'name';
Expand All @@ -14,14 +14,12 @@ interface CacheObject {

async function getCachedVersion(s3: S3, cacheObject: CacheObject): Promise<string | undefined> {
try {
const objectTagging = await s3
.getObjectTagging({
Bucket: cacheObject.bucket,
Key: cacheObject.key,
})
.promise();
const versions = objectTagging.TagSet?.filter((t: S3.Tag) => t.Key === versionKey);
return versions.length === 1 ? versions[0].Value : undefined;
const objectTagging = await s3.getObjectTagging({
Bucket: cacheObject.bucket,
Key: cacheObject.key,
});
const versions = objectTagging.TagSet?.filter((t: Tag) => t.Key === versionKey);
return versions?.length === 1 ? versions[0].Value : undefined;
Comment on lines +21 to +22
Copy link
Preview

Copilot AI Sep 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copilot uses AI. Check for mistakes.

} catch (e) {
console.debug('No tags found');
return undefined;
Expand Down Expand Up @@ -65,12 +63,16 @@ async function getReleaseAsset(

async function uploadToS3(s3: S3, cacheObject: CacheObject, actionRunnerReleaseAsset: ReleaseAsset): Promise<void> {
const writeStream = new PassThrough();
s3.upload({
Bucket: cacheObject.bucket,
Key: cacheObject.key,
Tagging: versionKey + '=' + actionRunnerReleaseAsset.name,
Body: writeStream,
}).promise();
const upload = new Upload({
Copy link
Contributor Author

@clee2000 clee2000 Sep 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

client: s3,
params: {
Bucket: cacheObject.bucket,
Key: cacheObject.key,
Tagging: `${versionKey}=${actionRunnerReleaseAsset.name}`,
Body: writeStream,
},
});
const uploadPromise = upload.done();

await new Promise<void>((resolve, reject) => {
console.debug('Start downloading %s and uploading to S3.', actionRunnerReleaseAsset.name);
Expand All @@ -87,10 +89,11 @@ async function uploadToS3(s3: S3, cacheObject: CacheObject, actionRunnerReleaseA
}).catch((error) => {
console.error(`Exception: ${error}`);
});
await uploadPromise;
Copy link
Preview

Copilot AI Sep 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.

}

export const handle = async (): Promise<void> => {
const s3 = new AWS.S3();
const s3 = new S3();

const fetchPrereleaseBinaries = yn(process.env.GITHUB_RUNNER_ALLOW_PRERELEASE_BINARIES, { default: false });
const distributions = [
Expand Down Expand Up @@ -131,7 +134,7 @@ export const handle = async (): Promise<void> => {
const currentVersion = await getCachedVersion(s3, cacheObject);
console.debug('latest: ' + currentVersion);
if (currentVersion === undefined || currentVersion != actionRunnerReleaseAsset.name) {
uploadToS3(s3, cacheObject, actionRunnerReleaseAsset);
await uploadToS3(s3, cacheObject, actionRunnerReleaseAsset);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} else {
console.debug('Distribution is up-to-date, no action.');
}
Expand Down
Loading
Loading