fix(appbuilder): Prevent parallel build processes on the same template #6172

mbfreder · 2024-12-06T16:57:48Z

Context

We currently allow multiple build processes to run at the same time for the same template.

Problem

When running a build, it’s possible to start a second build for the same project. This results in both builds failings

Solution

Prevent parallel builds on the same template

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.

License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

seshubaws · 2024-12-06T20:48:12Z

packages/core/src/shared/sam/utils.ts

+ */
+export function throwIfTemplateIsBeingBuilt(templatePath: string) {
+    if (isBuildInProgress(templatePath)) {
+        throw new ToolkitError('Template is already being built', { code: 'BuildInProgress' })


Let's say "This template is already being built" to make sure users know they are trying to build the same template again, and that they can build different templates simultaneously.

seshubaws · 2024-12-06T21:05:55Z

packages/core/src/shared/sam/utils.ts

@@ -108,6 +108,35 @@ export async function updateRecentResponse(
        getLogger().warn(`sam: unable to save response at key "${key}": %s`, err)
    }
 }
+
+const buildProcessMementoRootKey = 'samcli.build.processes'


Let's change this to something like

const BUILD_PROCESS_MEMENTO_ROOT_KEY = 'samcli.build.processes'; and maybe put it in the samutils if it makes sense to. We can also do the same with the 'global' as we should limit the amount of constant strings we have

The above format wouldn't work since eslint only allows camelCase or PascalCase formats. The constant is defined in the sam/utils file. Are you referring to a different utils file?

Ideally, we'd put this in a constants.ts file, but if not we can disable eslint for this line. The convention is that constants should be in all caps.

seshubaws · 2024-12-06T21:10:44Z

packages/core/src/shared/sam/utils.ts

+}
+
+export async function registerTemplateBuild(templatePath: string) {
+    await updateRecentResponse(buildProcessMementoRootKey, 'global', templatePath, 'true')


Can we add error handling here to catch if something goes wrong in the process of registering a template build? I think currently, the error would just bubble up from the actual sam build command, but it could be the case that build command succeeds but the registering fails

updateRecentResponse() already handles that by logging the error.

export async function updateRecentResponse( mementoRootKey: string, identifier: string, key: string, value: string | undefined ) { try { const root = globals.context.workspaceState.get(mementoRootKey, {} as Record<string, Record<string, string>>) await globals.context.workspaceState.update(mementoRootKey, { ...root, [identifier]: { ...root[identifier], [key]: value }, }) } catch (err) { getLogger().warn(`sam: unable to save response at key "${key}": %s`, err) } }

Can we capture the error so that we can see it in our Kibana dashboard? I don't think we see logger errors in our dashboard iirc

seshubaws · 2024-12-06T21:30:28Z

packages/core/src/shared/sam/utils.ts

+ */
+export function throwIfTemplateIsBeingBuilt(templatePath: string) {
+    if (isBuildInProgress(templatePath)) {
+        throw new ToolkitError('Template is already being built', { code: 'BuildInProgress' })


Can we make the error code an enum? I think we had a class with different error codes already right, let's add this one in with them

Those are specific SAM CLI error patterns. I don't think it makes sense to add this one to them since this error doesn't come directly from SAM CLI.

jpinkney-aws · 2024-12-10T12:10:33Z

packages/core/src/shared/sam/constants.ts

+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+export const BUILD_PROCESS_MEMENTO_ROOT_KEY = 'samcli.build.processes'


I think the standard we have in the toolkit is to use camal case, even for constants

jpinkney-aws · 2024-12-10T12:11:11Z

packages/core/src/shared/sam/constants.ts

@@ -0,0 +1,12 @@
+/* eslint-disable header/header */
+/* eslint-disable @typescript-eslint/naming-convention */


you shouldn't need these es-lint disables

jpinkney-aws · 2024-12-10T12:11:19Z

packages/core/src/shared/sam/utils.ts

@@ -1,3 +1,5 @@
+/* eslint-disable header/header */


you shouldn't need these es-lint disables

justinmk3 · 2024-12-11T23:11:20Z

packages/core/src/shared/sam/utils.ts

+ * @Param templatePath The path to the template.yaml file
+ */
+function isBuildInProgress(templatePath: string): boolean {
+    return getRecentResponse(buildProcessMementoRootKey, globalIdentifier, templatePath) !== undefined


Future note: these functions (and probably more, I guess) are using shared mutable state. You may want to consider wrapping this in a dedicated file similar to the existing packages/core/src/shared/globalState.ts , except for "workspace" state. That makes it clear when callers are using this global state. And it collects the pattern in one place, instead of many spread-out functions.

roger-zhangg

Left few comments, thanks for implementing these changes!

roger-zhangg · 2024-12-12T00:05:24Z

packages/core/src/shared/sam/utils.ts

+ */
+export function throwIfTemplateIsBeingBuilt(templatePath: string) {
+    if (isBuildInProgress(templatePath)) {
+        throw new ToolkitError('This template is already being built', { code: 'BuildInProgress' })


I would say this check is too strict. There are possibility that the build is finished or exited but we also failed to unregisterTemplateBuild or even toolkit maybe reload itself during the build process. In this case the user will never able to build the template again. One possible solution is to add a timeout to this cache. Like 2 minutes or 5 minutes. In this way, if the worst case happens ( build finished but we failed to update memeto) the user will still be able to build after time out is reached.

roger-zhangg · 2024-12-12T00:07:33Z

packages/core/src/shared/sam/build.ts

+                location: vscode.ProgressLocation.Notification,
+            },
+            async (progress) => {
+                progress.report({ message: `Building SAM template at ${params.template.uri.path}` })


can we also add a cancel button to it? you can refer the implementation here:

aws-toolkit-vscode/packages/core/src/shared/utilities/cliUtils.ts

Lines 235 to 238 in 5bfb867

const progress = await showMessageWithCancel(

localize('AWS.cli.installProgress', 'Installing: {0} CLI', cliToInstall.name),

timeout

)

roger-zhangg · 2024-12-12T00:13:30Z

packages/core/src/test/shared/sam/build.test.ts

@@ -551,3 +564,23 @@ describe('SAM runBuild', () => {
        })
    })
 })
+
+async function runInParallel(samLocation: SamAppLocation): Promise<[SamBuildResult, SamBuildResult]> {
+    return Promise.all([runBuild(new AppNode(samLocation)), runBuild(new AppNode(samLocation))])


I worry this would cause some flakiness as the two build is run at same time. There might be a race condition that both Build run throwIfTemplateIsBeingBuilt both passes and then both run registerTemplateBuild and proceed to build. In this case the two build will be run together and cause test to fail. I would suggest we add a 0.05s wait time between them to avoid the unlikely but possible race condition.

mbfreder added 4 commits December 5, 2024 01:11

prevent parallel builds on the same template

1568065

add progress notification for sam build

fe82be7

add tests

379ba6b

add changelog

6c23ba3

mbfreder requested a review from a team as a code owner December 6, 2024 16:57

mbfreder changed the title ~~fix(appBuilder): Prevent parallel build processes on the same template~~ fix(appbuilder): Prevent parallel build processes on the same template Dec 6, 2024

seshubaws suggested changes Dec 6, 2024

View reviewed changes

address comments

00e99fe

mbfreder force-pushed the parallel-build branch 2 times, most recently from 654bf52 to 3471429 Compare December 10, 2024 04:52

add constants file

86848f2

mbfreder force-pushed the parallel-build branch from 3471429 to 86848f2 Compare December 10, 2024 05:24

Merge branch 'master' into parallel-build

c261185

jpinkney-aws reviewed Dec 10, 2024

View reviewed changes

revert constants format to camelcase

0922083

mbfreder force-pushed the parallel-build branch from c46878d to 0922083 Compare December 11, 2024 18:56

justinmk3 reviewed Dec 11, 2024

View reviewed changes

roger-zhangg suggested changes Dec 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(appbuilder): Prevent parallel build processes on the same template #6172

fix(appbuilder): Prevent parallel build processes on the same template #6172

mbfreder commented Dec 6, 2024

seshubaws Dec 6, 2024

seshubaws Dec 6, 2024

mbfreder Dec 6, 2024 •

edited

Loading

seshubaws Dec 9, 2024

seshubaws Dec 6, 2024

mbfreder Dec 6, 2024

seshubaws Dec 9, 2024

seshubaws Dec 6, 2024

mbfreder Dec 6, 2024

jpinkney-aws Dec 10, 2024

jpinkney-aws Dec 10, 2024

jpinkney-aws Dec 10, 2024

justinmk3 Dec 11, 2024

roger-zhangg left a comment

roger-zhangg Dec 12, 2024 •

edited

Loading

roger-zhangg Dec 12, 2024

roger-zhangg Dec 12, 2024

		@@ -0,0 +1,12 @@
		/* eslint-disable header/header */
		/* eslint-disable @typescript-eslint/naming-convention */

	const progress = await showMessageWithCancel(
	localize('AWS.cli.installProgress', 'Installing: {0} CLI', cliToInstall.name),
	timeout
	)

fix(appbuilder): Prevent parallel build processes on the same template #6172

Are you sure you want to change the base?

fix(appbuilder): Prevent parallel build processes on the same template #6172

Conversation

mbfreder commented Dec 6, 2024

Context

Problem

Solution

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbfreder Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roger-zhangg left a comment

Choose a reason for hiding this comment

roger-zhangg Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbfreder Dec 6, 2024 •

edited

Loading

roger-zhangg Dec 12, 2024 •

edited

Loading