Skip to content
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.

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

fix(amazonq): retry S3 upload #6246

Merged
merged 7 commits into from
Dec 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"type": "Bug Fix",
"description": "Amazon Q Code Transformation: retry project upload up to 3 times"
}
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ export class Messenger {
this.dispatcher.sendAsyncEventProgress(
new AsyncEventProgressMessage(tabID, {
inProgress: true,
message: MessengerUtils.createLanguageUpgradeConfirmationPrompt(detectedJavaVersions),
message: CodeWhispererConstants.projectPromptChatMessage,
})
)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,30 +69,8 @@ export default class MessengerUtils {
}
}

static createLanguageUpgradeConfirmationPrompt = (detectedJavaVersions: Array<JDKVersion | undefined>): string => {
let javaVersionString = 'Java project'
const uniqueJavaOptions = new Set(detectedJavaVersions)

if (detectedJavaVersions.length > 1) {
// this means there is a Java version whose version we weren't able to determine
if (uniqueJavaOptions.has(undefined)) {
javaVersionString = 'Java projects'
} else {
javaVersionString = `Java ${Array.from(uniqueJavaOptions).join(' & ')} projects`
}
} else if (detectedJavaVersions.length === 1) {
if (!uniqueJavaOptions.has(undefined)) {
javaVersionString = `Java ${detectedJavaVersions[0]!.toString()} project`
}
}

return CodeWhispererConstants.projectPromptChatMessage.replace('JAVA_VERSION_HERE', javaVersionString)
}

static createAvailableDependencyVersionString = (versions: DependencyVersions): string => {
let message = `I found ${versions.length} other dependency versions that are more recent than the dependency in your code that's causing an error: ${versions.currentVersion}.
`
let message = `I found ${versions.length} other dependency versions that are more recent than the dependency in your code that's causing an error: ${versions.currentVersion}.`

if (versions.majorVersions !== undefined && versions.majorVersions.length > 0) {
message = message.concat(
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/codewhisperer/models/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -747,7 +747,7 @@ export const cleanInstallErrorNotification = `Amazon Q could not run the Maven c
export const enterJavaHomeChatMessage = 'Enter the path to JDK '

export const projectPromptChatMessage =
'I can upgrade your JAVA_VERSION_HERE. To start the transformation, I need some information from you. Choose the project you want to upgrade and the target code version to upgrade to. Then, choose Confirm.'
'I can upgrade your Java project. To start the transformation, I need some information from you. Choose the project you want to upgrade and the target code version to upgrade to. Then, choose Confirm.'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See comment below; rather than having 75+ LOC and running a shell command for each open project just to say "Java 8 project" or "Java 11 project" or "Java 8 and 11 projects", etc. just say "Java project"


export const windowsJavaHomeHelpChatMessage =
'To find the JDK path, run the following commands in a new terminal: `cd "C:/Program Files/Java"` and then `dir`. If you see your JDK version, run `cd <version>` and then `cd` to show the path.'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,18 +103,45 @@ export async function uploadArtifactToS3(
try {
const uploadFileByteSize = (await nodefs.promises.stat(fileName)).size
getLogger().info(
`Uploading project artifact at %s with checksum %s using uploadId: %s and size %s kB`,
`CodeTransformation: Uploading project artifact at %s with checksum %s using uploadId: %s and size %s kB`,
fileName,
sha256,
resp.uploadId,
Math.round(uploadFileByteSize / 1000)
)

const response = await request.fetch('PUT', resp.uploadUrl, {
body: buffer,
headers: getHeadersObj(sha256, resp.kmsKeyArn),
}).response
getLogger().info(`CodeTransformation: Status from S3 Upload = ${response.status}`)
let response = undefined
/* The existing S3 client has built-in retries but it requires the bucket name, so until
* CreateUploadUrl can be modified to return the S3 bucket name, manually implement retries.
* Alternatively, when waitUntil supports a fixed number of retries and retriableCodes, use that.
*/
const retriableCodes = [408, 429, 500, 502, 503, 504]
for (let i = 0; i < 4; i++) {
try {
response = await request.fetch('PUT', resp.uploadUrl, {
body: buffer,
headers: getHeadersObj(sha256, resp.kmsKeyArn),
}).response
getLogger().info(`CodeTransformation: upload to S3 status on attempt ${i + 1}/4 = ${response.status}`)
if (response.status === 200) {
break
}
throw new Error('Upload failed')
} catch (e: any) {
if (response && !retriableCodes.includes(response.status)) {
throw new Error(`Upload failed with status code = ${response.status}; did not automatically retry`)
}
if (i !== 3) {
await sleep(1000 * Math.pow(2, i))
}
}
}
if (!response || response.status !== 200) {
const uploadFailedError = `Upload failed after up to 4 attempts with status code = ${response?.status ?? 'unavailable'}`
getLogger().error(`CodeTransformation: ${uploadFailedError}`)
throw new Error(uploadFailedError)
}
getLogger().info('CodeTransformation: Upload to S3 succeeded')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not a big fan of implementing our own retry logic rather than doing it at the SDK layer, but the existing S3 client's upload functionality requires a bucket name as a part of the request, and 1) I don't think we should have the internal S3 bucket names we use floating around this repo, 2) we use multiple different buckets, and 3) we don't have access to the bucket name in the response of our CreateUploadUrl — this API would have to be changed and then we can use the existing S3 client.

Copy link
Contributor

@justinmk3 justinmk3 Dec 16, 2024

Choose a reason for hiding this comment

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

Thanks for that context! That info is probably worth a code comment here or in the docstring of this function.

For reference, we have discussed some sort of util to "poll/retry stuff", see https://taskei.amazon.dev/tasks/IDE-15280 (startDevEnvironmentWithProgress is a very complex case, maybe not exactly analogous to standard "request retry").

} catch (e: any) {
let errorMessage = `The upload failed due to: ${(e as Error).message}. For more information, see the [Amazon Q documentation](${CodeWhispererConstants.codeTransformTroubleshootUploadError})`
if (errorMessage.includes('Request has expired')) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,8 @@
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
* SPDX-License-Identifier: Apache-2.0
*/
import { BuildSystem, JDKVersion, TransformationCandidateProject } from '../../models/model'
import { getLogger } from '../../../shared/logger'
import * as CodeWhispererConstants from '../../models/constants'
import { BuildSystem, TransformationCandidateProject } from '../../models/model'
import * as vscode from 'vscode'
// Consider using ChildProcess once we finalize all spawnSync calls
import { spawnSync } from 'child_process' // eslint-disable-line no-restricted-imports
import {
NoJavaProjectsFoundError,
NoMavenJavaProjectsFoundError,
Expand Down Expand Up @@ -70,72 +66,9 @@ async function getMavenJavaProjects(javaProjects: TransformationCandidateProject
return mavenJavaProjects
}

async function getProjectsValidToTransform(mavenJavaProjects: TransformationCandidateProject[]) {
const projectsValidToTransform: TransformationCandidateProject[] = []
for (const project of mavenJavaProjects) {
let detectedJavaVersion = undefined
const projectPath = project.path
const compiledJavaFiles = await vscode.workspace.findFiles(
new vscode.RelativePattern(projectPath!, '**/*.class'),
'**/node_modules/**',
1
)
if (compiledJavaFiles.length > 0) {
const classFilePath = `${compiledJavaFiles[0].fsPath}`
const baseCommand = 'javap'
const args = ['-v', classFilePath]
const spawnResult = spawnSync(baseCommand, args, { shell: false, encoding: 'utf-8' })
if (spawnResult.status !== 0) {
let errorLog = ''
errorLog += spawnResult.error ? JSON.stringify(spawnResult.error) : ''
errorLog += `${spawnResult.stderr}\n${spawnResult.stdout}`
getLogger().error(`CodeTransformation: Error in running javap command = ${errorLog}`)
let errorReason = ''
if (spawnResult.stdout) {
errorReason = 'JavapExecutionError'
} else {
errorReason = 'JavapSpawnError'
}
if (spawnResult.error) {
const errorCode = (spawnResult.error as any).code ?? 'UNKNOWN'
errorReason += `-${errorCode}`
}
getLogger().error(
`CodeTransformation: Error in running javap command = ${errorReason}, log = ${errorLog}`
)
} else {
const majorVersionIndex = spawnResult.stdout.indexOf('major version: ')
const javaVersion = spawnResult.stdout.slice(majorVersionIndex + 15, majorVersionIndex + 17).trim()
if (javaVersion === CodeWhispererConstants.JDK8VersionNumber) {
detectedJavaVersion = JDKVersion.JDK8
} else if (javaVersion === CodeWhispererConstants.JDK11VersionNumber) {
detectedJavaVersion = JDKVersion.JDK11
} else {
detectedJavaVersion = JDKVersion.UNSUPPORTED
}
}
}

// detectedJavaVersion will be undefined if there are no .class files or if javap errors out, otherwise it will be JDK8, JDK11, or UNSUPPORTED
project.JDKVersion = detectedJavaVersion
projectsValidToTransform.push(project)
}
return projectsValidToTransform
}

/*
* This function filters all open projects by first searching for a .java file and then searching for a pom.xml file in all projects.
* It also tries to detect the Java version of each project by running "javap" on a .class file of each project.
* As long as the project contains a .java file and a pom.xml file, the project is still considered valid for transformation,
* and we allow the user to specify the Java version.
*/
// This function filters all open projects by first searching for a .java file and then searching for a pom.xml file in all projects.
export async function validateOpenProjects(projects: TransformationCandidateProject[]) {
const javaProjects = await getJavaProjects(projects)

const mavenJavaProjects = await getMavenJavaProjects(javaProjects)

// These projects we know must contain a pom.xml and a .java file
const projectsValidToTransform = await getProjectsValidToTransform(mavenJavaProjects)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleted this function entirely because 1) it was expensive (running a shell command (javap) on every open project) and 2) it was just used to format a String (see createLanguageUpgradeConfirmationPrompt above); every project returned by getMavenJavaProjects above was also returned by getProjectsValidToTransform


return projectsValidToTransform
return mavenJavaProjects
}
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,12 @@ import {
parseBuildFile,
validateSQLMetadataFile,
} from '../../../codewhisperer/service/transformByQ/transformFileHandler'
import { uploadArtifactToS3 } from '../../../codewhisperer/indexNode'
import request from '../../../shared/request'
import * as nodefs from 'fs' // eslint-disable-line no-restricted-imports

describe('transformByQ', function () {
let fetchStub: sinon.SinonStub
let tempDir: string
const validSctFile = `<?xml version="1.0" encoding="UTF-8"?>
<tree>
Expand Down Expand Up @@ -91,6 +95,10 @@ describe('transformByQ', function () {
beforeEach(async function () {
tempDir = (await TestFolder.create()).path
transformByQState.setToNotStarted()
fetchStub = sinon.stub(request, 'fetch')
// use Partial to avoid having to mock all 25 fields in the response; only care about 'size'
const mockStats: Partial<nodefs.Stats> = { size: 1000 }
sinon.stub(nodefs.promises, 'stat').resolves(mockStats as nodefs.Stats)
})

afterEach(async function () {
Expand Down Expand Up @@ -464,4 +472,90 @@ describe('transformByQ', function () {
const isValidMetadata = await validateSQLMetadataFile(sctFileWithInvalidTarget, { tabID: 'abc123' })
assert.strictEqual(isValidMetadata, false)
})

it('should successfully upload on first attempt', async () => {
const successResponse = {
ok: true,
status: 200,
text: () => Promise.resolve('Success'),
}
fetchStub.returns({ response: Promise.resolve(successResponse) })
await uploadArtifactToS3(
'test.zip',
{ uploadId: '123', uploadUrl: 'http://test.com', kmsKeyArn: 'arn' },
'sha256',
Buffer.from('test')
)
sinon.assert.calledOnce(fetchStub)
})

it('should retry upload on retriable error and succeed', async () => {
const failedResponse = {
ok: false,
status: 503,
text: () => Promise.resolve('Service Unavailable'),
}
const successResponse = {
ok: true,
status: 200,
text: () => Promise.resolve('Success'),
}
fetchStub.onFirstCall().returns({ response: Promise.resolve(failedResponse) })
fetchStub.onSecondCall().returns({ response: Promise.resolve(successResponse) })
await uploadArtifactToS3(
'test.zip',
{ uploadId: '123', uploadUrl: 'http://test.com', kmsKeyArn: 'arn' },
'sha256',
Buffer.from('test')
)
sinon.assert.calledTwice(fetchStub)
})

it('should throw error after 4 failed upload attempts', async () => {
const failedResponse = {
ok: false,
status: 500,
text: () => Promise.resolve('Internal Server Error'),
}
fetchStub.returns({ response: Promise.resolve(failedResponse) })
const expectedMessage =
'The upload failed due to: Upload failed after up to 4 attempts with status code = 500. For more information, see the [Amazon Q documentation](https://docs.aws.amazon.com/amazonq/latest/qdeveloper-ug/troubleshooting-code-transformation.html#project-upload-fail)'
await assert.rejects(
uploadArtifactToS3(
'test.zip',
{ uploadId: '123', uploadUrl: 'http://test.com', kmsKeyArn: 'arn' },
'sha256',
Buffer.from('test')
),
{
name: 'Error',
message: expectedMessage,
}
)
sinon.assert.callCount(fetchStub, 4)
})

it('should not retry upload on non-retriable error', async () => {
const failedResponse = {
ok: false,
status: 400,
text: () => Promise.resolve('Bad Request'),
}
fetchStub.onFirstCall().returns({ response: Promise.resolve(failedResponse) })
const expectedMessage =
'The upload failed due to: Upload failed with status code = 400; did not automatically retry. For more information, see the [Amazon Q documentation](https://docs.aws.amazon.com/amazonq/latest/qdeveloper-ug/troubleshooting-code-transformation.html#project-upload-fail)'
await assert.rejects(
uploadArtifactToS3(
'test.zip',
{ uploadId: '123', uploadUrl: 'http://test.com', kmsKeyArn: 'arn' },
'sha256',
Buffer.from('test')
),
{
name: 'Error',
message: expectedMessage,
}
)
sinon.assert.calledOnce(fetchStub)
})
})
Loading