Skip to content

Commit 6650cb9

Browse files
dhasani23David Hasani
dhasani23
and
David Hasani
authored
fix(amazonq): retry S3 upload (#6246)
## Problem S3 upload sometimes fails due to transient issues. ## Solution Add up to 3 retries. --- - Treat all work as PUBLIC. Private `feature/x` branches will not be squash-merged at release time. - Your code changes must meet the guidelines in [CONTRIBUTING.md](https://github.com/aws/aws-toolkit-vscode/blob/master/CONTRIBUTING.md#guidelines). License: I confirm that my contribution is made under the terms of the Apache 2.0 license. --------- Co-authored-by: David Hasani <[email protected]>
1 parent ee2ac96 commit 6650cb9

File tree

7 files changed

+137
-101
lines changed

7 files changed

+137
-101
lines changed

‎packages/amazonq/.changes/next-release/Bug Fix-f7bb3f35-1b43-4604-b3bd-c2e7f1b65083.json

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
{
2+
"type": "Bug Fix",
3+
"description": "Amazon Q Code Transformation: retry project upload up to 3 times"
4+
}

‎packages/core/src/amazonqGumby/chat/controller/messenger/messenger.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -252,7 +252,7 @@ export class Messenger {
252252
this.dispatcher.sendAsyncEventProgress(
253253
new AsyncEventProgressMessage(tabID, {
254254
inProgress: true,
255-
message: MessengerUtils.createLanguageUpgradeConfirmationPrompt(detectedJavaVersions),
255+
message: CodeWhispererConstants.projectPromptChatMessage,
256256
})
257257
)
258258

‎packages/core/src/amazonqGumby/chat/controller/messenger/messengerUtils.ts

Lines changed: 1 addition & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -69,30 +69,8 @@ export default class MessengerUtils {
6969
}
7070
}
7171

72-
static createLanguageUpgradeConfirmationPrompt = (detectedJavaVersions: Array<JDKVersion | undefined>): string => {
73-
let javaVersionString = 'Java project'
74-
const uniqueJavaOptions = new Set(detectedJavaVersions)
75-
76-
if (detectedJavaVersions.length > 1) {
77-
// this means there is a Java version whose version we weren't able to determine
78-
if (uniqueJavaOptions.has(undefined)) {
79-
javaVersionString = 'Java projects'
80-
} else {
81-
javaVersionString = `Java ${Array.from(uniqueJavaOptions).join(' & ')} projects`
82-
}
83-
} else if (detectedJavaVersions.length === 1) {
84-
if (!uniqueJavaOptions.has(undefined)) {
85-
javaVersionString = `Java ${detectedJavaVersions[0]!.toString()} project`
86-
}
87-
}
88-
89-
return CodeWhispererConstants.projectPromptChatMessage.replace('JAVA_VERSION_HERE', javaVersionString)
90-
}
91-
9272
static createAvailableDependencyVersionString = (versions: DependencyVersions): string => {
93-
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}.
94-
95-
`
73+
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}.`
9674

9775
if (versions.majorVersions !== undefined && versions.majorVersions.length > 0) {
9876
message = message.concat(

‎packages/core/src/codewhisperer/models/constants.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -747,7 +747,7 @@ export const cleanInstallErrorNotification = `Amazon Q could not run the Maven c
747747
export const enterJavaHomeChatMessage = 'Enter the path to JDK '
748748

749749
export const projectPromptChatMessage =
750-
'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.'
750+
'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.'
751751

752752
export const windowsJavaHomeHelpChatMessage =
753753
'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.'

‎packages/core/src/codewhisperer/service/transformByQ/transformApiHandler.ts

Lines changed: 33 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -103,18 +103,45 @@ export async function uploadArtifactToS3(
103103
try {
104104
const uploadFileByteSize = (await nodefs.promises.stat(fileName)).size
105105
getLogger().info(
106-
`Uploading project artifact at %s with checksum %s using uploadId: %s and size %s kB`,
106+
`CodeTransformation: Uploading project artifact at %s with checksum %s using uploadId: %s and size %s kB`,
107107
fileName,
108108
sha256,
109109
resp.uploadId,
110110
Math.round(uploadFileByteSize / 1000)
111111
)
112112

113-
const response = await request.fetch('PUT', resp.uploadUrl, {
114-
body: buffer,
115-
headers: getHeadersObj(sha256, resp.kmsKeyArn),
116-
}).response
117-
getLogger().info(`CodeTransformation: Status from S3 Upload = ${response.status}`)
113+
let response = undefined
114+
/* The existing S3 client has built-in retries but it requires the bucket name, so until
115+
* CreateUploadUrl can be modified to return the S3 bucket name, manually implement retries.
116+
* Alternatively, when waitUntil supports a fixed number of retries and retriableCodes, use that.
117+
*/
118+
const retriableCodes = [408, 429, 500, 502, 503, 504]
119+
for (let i = 0; i < 4; i++) {
120+
try {
121+
response = await request.fetch('PUT', resp.uploadUrl, {
122+
body: buffer,
123+
headers: getHeadersObj(sha256, resp.kmsKeyArn),
124+
}).response
125+
getLogger().info(`CodeTransformation: upload to S3 status on attempt ${i + 1}/4 = ${response.status}`)
126+
if (response.status === 200) {
127+
break
128+
}
129+
throw new Error('Upload failed')
130+
} catch (e: any) {
131+
if (response && !retriableCodes.includes(response.status)) {
132+
throw new Error(`Upload failed with status code = ${response.status}; did not automatically retry`)
133+
}
134+
if (i !== 3) {
135+
await sleep(1000 * Math.pow(2, i))
136+
}
137+
}
138+
}
139+
if (!response || response.status !== 200) {
140+
const uploadFailedError = `Upload failed after up to 4 attempts with status code = ${response?.status ?? 'unavailable'}`
141+
getLogger().error(`CodeTransformation: ${uploadFailedError}`)
142+
throw new Error(uploadFailedError)
143+
}
144+
getLogger().info('CodeTransformation: Upload to S3 succeeded')
118145
} catch (e: any) {
119146
let errorMessage = `The upload failed due to: ${(e as Error).message}. For more information, see the [Amazon Q documentation](${CodeWhispererConstants.codeTransformTroubleshootUploadError})`
120147
if (errorMessage.includes('Request has expired')) {

‎packages/core/src/codewhisperer/service/transformByQ/transformProjectValidationHandler.ts

Lines changed: 3 additions & 70 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,8 @@
22
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
33
* SPDX-License-Identifier: Apache-2.0
44
*/
5-
import { BuildSystem, JDKVersion, TransformationCandidateProject } from '../../models/model'
6-
import { getLogger } from '../../../shared/logger'
7-
import * as CodeWhispererConstants from '../../models/constants'
5+
import { BuildSystem, TransformationCandidateProject } from '../../models/model'
86
import * as vscode from 'vscode'
9-
// Consider using ChildProcess once we finalize all spawnSync calls
10-
import { spawnSync } from 'child_process' // eslint-disable-line no-restricted-imports
117
import {
128
NoJavaProjectsFoundError,
139
NoMavenJavaProjectsFoundError,
@@ -70,72 +66,9 @@ async function getMavenJavaProjects(javaProjects: TransformationCandidateProject
7066
return mavenJavaProjects
7167
}
7268

73-
async function getProjectsValidToTransform(mavenJavaProjects: TransformationCandidateProject[]) {
74-
const projectsValidToTransform: TransformationCandidateProject[] = []
75-
for (const project of mavenJavaProjects) {
76-
let detectedJavaVersion = undefined
77-
const projectPath = project.path
78-
const compiledJavaFiles = await vscode.workspace.findFiles(
79-
new vscode.RelativePattern(projectPath!, '**/*.class'),
80-
'**/node_modules/**',
81-
1
82-
)
83-
if (compiledJavaFiles.length > 0) {
84-
const classFilePath = `${compiledJavaFiles[0].fsPath}`
85-
const baseCommand = 'javap'
86-
const args = ['-v', classFilePath]
87-
const spawnResult = spawnSync(baseCommand, args, { shell: false, encoding: 'utf-8' })
88-
if (spawnResult.status !== 0) {
89-
let errorLog = ''
90-
errorLog += spawnResult.error ? JSON.stringify(spawnResult.error) : ''
91-
errorLog += `${spawnResult.stderr}\n${spawnResult.stdout}`
92-
getLogger().error(`CodeTransformation: Error in running javap command = ${errorLog}`)
93-
let errorReason = ''
94-
if (spawnResult.stdout) {
95-
errorReason = 'JavapExecutionError'
96-
} else {
97-
errorReason = 'JavapSpawnError'
98-
}
99-
if (spawnResult.error) {
100-
const errorCode = (spawnResult.error as any).code ?? 'UNKNOWN'
101-
errorReason += `-${errorCode}`
102-
}
103-
getLogger().error(
104-
`CodeTransformation: Error in running javap command = ${errorReason}, log = ${errorLog}`
105-
)
106-
} else {
107-
const majorVersionIndex = spawnResult.stdout.indexOf('major version: ')
108-
const javaVersion = spawnResult.stdout.slice(majorVersionIndex + 15, majorVersionIndex + 17).trim()
109-
if (javaVersion === CodeWhispererConstants.JDK8VersionNumber) {
110-
detectedJavaVersion = JDKVersion.JDK8
111-
} else if (javaVersion === CodeWhispererConstants.JDK11VersionNumber) {
112-
detectedJavaVersion = JDKVersion.JDK11
113-
} else {
114-
detectedJavaVersion = JDKVersion.UNSUPPORTED
115-
}
116-
}
117-
}
118-
119-
// detectedJavaVersion will be undefined if there are no .class files or if javap errors out, otherwise it will be JDK8, JDK11, or UNSUPPORTED
120-
project.JDKVersion = detectedJavaVersion
121-
projectsValidToTransform.push(project)
122-
}
123-
return projectsValidToTransform
124-
}
125-
126-
/*
127-
* This function filters all open projects by first searching for a .java file and then searching for a pom.xml file in all projects.
128-
* It also tries to detect the Java version of each project by running "javap" on a .class file of each project.
129-
* As long as the project contains a .java file and a pom.xml file, the project is still considered valid for transformation,
130-
* and we allow the user to specify the Java version.
131-
*/
69+
// This function filters all open projects by first searching for a .java file and then searching for a pom.xml file in all projects.
13270
export async function validateOpenProjects(projects: TransformationCandidateProject[]) {
13371
const javaProjects = await getJavaProjects(projects)
134-
13572
const mavenJavaProjects = await getMavenJavaProjects(javaProjects)
136-
137-
// These projects we know must contain a pom.xml and a .java file
138-
const projectsValidToTransform = await getProjectsValidToTransform(mavenJavaProjects)
139-
140-
return projectsValidToTransform
73+
return mavenJavaProjects
14174
}

‎packages/core/src/test/codewhisperer/commands/transformByQ.test.ts

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,8 +42,12 @@ import {
4242
parseBuildFile,
4343
validateSQLMetadataFile,
4444
} from '../../../codewhisperer/service/transformByQ/transformFileHandler'
45+
import { uploadArtifactToS3 } from '../../../codewhisperer/indexNode'
46+
import request from '../../../shared/request'
47+
import * as nodefs from 'fs' // eslint-disable-line no-restricted-imports
4548

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

96104
afterEach(async function () {
@@ -464,4 +472,90 @@ describe('transformByQ', function () {
464472
const isValidMetadata = await validateSQLMetadataFile(sctFileWithInvalidTarget, { tabID: 'abc123' })
465473
assert.strictEqual(isValidMetadata, false)
466474
})
475+
476+
it('should successfully upload on first attempt', async () => {
477+
const successResponse = {
478+
ok: true,
479+
status: 200,
480+
text: () => Promise.resolve('Success'),
481+
}
482+
fetchStub.returns({ response: Promise.resolve(successResponse) })
483+
await uploadArtifactToS3(
484+
'test.zip',
485+
{ uploadId: '123', uploadUrl: 'http://test.com', kmsKeyArn: 'arn' },
486+
'sha256',
487+
Buffer.from('test')
488+
)
489+
sinon.assert.calledOnce(fetchStub)
490+
})
491+
492+
it('should retry upload on retriable error and succeed', async () => {
493+
const failedResponse = {
494+
ok: false,
495+
status: 503,
496+
text: () => Promise.resolve('Service Unavailable'),
497+
}
498+
const successResponse = {
499+
ok: true,
500+
status: 200,
501+
text: () => Promise.resolve('Success'),
502+
}
503+
fetchStub.onFirstCall().returns({ response: Promise.resolve(failedResponse) })
504+
fetchStub.onSecondCall().returns({ response: Promise.resolve(successResponse) })
505+
await uploadArtifactToS3(
506+
'test.zip',
507+
{ uploadId: '123', uploadUrl: 'http://test.com', kmsKeyArn: 'arn' },
508+
'sha256',
509+
Buffer.from('test')
510+
)
511+
sinon.assert.calledTwice(fetchStub)
512+
})
513+
514+
it('should throw error after 4 failed upload attempts', async () => {
515+
const failedResponse = {
516+
ok: false,
517+
status: 500,
518+
text: () => Promise.resolve('Internal Server Error'),
519+
}
520+
fetchStub.returns({ response: Promise.resolve(failedResponse) })
521+
const expectedMessage =
522+
'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)'
523+
await assert.rejects(
524+
uploadArtifactToS3(
525+
'test.zip',
526+
{ uploadId: '123', uploadUrl: 'http://test.com', kmsKeyArn: 'arn' },
527+
'sha256',
528+
Buffer.from('test')
529+
),
530+
{
531+
name: 'Error',
532+
message: expectedMessage,
533+
}
534+
)
535+
sinon.assert.callCount(fetchStub, 4)
536+
})
537+
538+
it('should not retry upload on non-retriable error', async () => {
539+
const failedResponse = {
540+
ok: false,
541+
status: 400,
542+
text: () => Promise.resolve('Bad Request'),
543+
}
544+
fetchStub.onFirstCall().returns({ response: Promise.resolve(failedResponse) })
545+
const expectedMessage =
546+
'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)'
547+
await assert.rejects(
548+
uploadArtifactToS3(
549+
'test.zip',
550+
{ uploadId: '123', uploadUrl: 'http://test.com', kmsKeyArn: 'arn' },
551+
'sha256',
552+
Buffer.from('test')
553+
),
554+
{
555+
name: 'Error',
556+
message: expectedMessage,
557+
}
558+
)
559+
sinon.assert.calledOnce(fetchStub)
560+
})
467561
})

0 commit comments

Comments
 (0)