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(appbuilder): Prevent parallel build processes on the same template #6172

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,7 @@

import { samSyncUrl } from '../../../shared/constants'
import { CloudFormationTemplateRegistry } from '../../../shared/fs/templateRegistry'
import { syncMementoRootKey } from '../../../shared/sam/sync'

import { syncMementoRootKey } from '../../../shared/sam/constants'
import { createExitPrompter } from '../../../shared/ui/common/exitPrompter'
import { createTemplatePrompter, TemplateItem } from '../../../shared/ui/sam/templatePrompter'
import { Wizard } from '../../../shared/wizards/wizard'
Expand Down
40 changes: 29 additions & 11 deletions packages/core/src/shared/sam/build.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,15 @@ import {
getSamCliPathAndVersion,
getTerminalFromError,
isDotnetRuntime,
registerTemplateBuild,
throwIfTemplateIsBeingBuilt,
unregisterTemplateBuild,
updateRecentResponse,
} from './utils'
import { getConfigFileUri, validateSamBuildConfig } from './config'
import { runInTerminal } from './processTerminal'
import { buildMementoRootKey } from './constants'

const buildMementoRootKey = 'samcli.build.params'
export interface BuildParams {
readonly template: TemplateItem
readonly projectRoot: vscode.Uri
Expand Down Expand Up @@ -207,6 +210,9 @@ export async function runBuild(arg?: TreeNode): Promise<SamBuildResult> {
throw new CancellationError('user')
}

throwIfTemplateIsBeingBuilt(params.template.uri.path)
await registerTemplateBuild(params.template.uri.path)

const projectRoot = params.projectRoot

const defaultFlags: string[] = ['--cached', '--parallel', '--save-params', '--use-container']
Expand All @@ -229,23 +235,35 @@ export async function runBuild(arg?: TreeNode): Promise<SamBuildResult> {
await updateRecentResponse(buildMementoRootKey, 'global', 'templatePath', templatePath)

try {
const { path: samCliPath } = await getSamCliPathAndVersion()
await vscode.window.withProgress(
{
location: vscode.ProgressLocation.Notification,
},
async (progress) => {
progress.report({ message: `Building SAM template at ${params.template.uri.path}` })

// Create a child process to run the SAM build command
const buildProcess = new ChildProcess(samCliPath, ['build', ...buildFlags], {
spawnOptions: await addTelemetryEnvVar({
cwd: params.projectRoot.fsPath,
env: await getSpawnEnv(process.env),
}),
})
const { path: samCliPath } = await getSamCliPathAndVersion()

// Create a child process to run the SAM build command
const buildProcess = new ChildProcess(samCliPath, ['build', ...buildFlags], {
spawnOptions: await addTelemetryEnvVar({
cwd: params.projectRoot.fsPath,
env: await getSpawnEnv(process.env),
}),
})

// Run SAM build in Terminal
await runInTerminal(buildProcess, 'build')
}
)

// Run SAM build in Terminal
await runInTerminal(buildProcess, 'build')
await unregisterTemplateBuild(params.template.uri.path)

return {
isSuccess: true,
}
} catch (error) {
await unregisterTemplateBuild(params.template.uri.path)
throw ToolkitError.chain(error, 'Failed to build SAM template', {
details: { terminal: getTerminalFromError(error), ...resolveBuildArgConflict(buildFlags) },
code: getErrorCode(error),
Expand Down
10 changes: 10 additions & 0 deletions packages/core/src/shared/sam/constants.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/*!
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
* SPDX-License-Identifier: Apache-2.0
*/

export const buildProcessMementoRootKey = 'samcli.build.processes'
export const globalIdentifier = 'global'
export const buildMementoRootKey = 'samcli.build.params'
export const deployMementoRootKey = 'samcli.deploy.params'
export const syncMementoRootKey = 'samcli.sync.params'
3 changes: 1 addition & 2 deletions packages/core/src/shared/sam/deploy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import {
updateRecentResponse,
} from './utils'
import { runInTerminal } from './processTerminal'
import { deployMementoRootKey } from './constants'

export interface DeployParams {
readonly paramsSource: ParamsSource
Expand All @@ -48,8 +49,6 @@ export interface DeployParams {
[key: string]: any
}

const deployMementoRootKey = 'samcli.deploy.params'

function getRecentDeployParams(identifier: string, key: string): string | undefined {
return getRecentResponse(deployMementoRootKey, identifier, key)
}
Expand Down
3 changes: 1 addition & 2 deletions packages/core/src/shared/sam/sync.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ import { ParamsSource, createSyncParamsSourcePrompter } from '../ui/sam/paramsSo
import { createEcrPrompter } from '../ui/sam/ecrPrompter'
import { BucketSource, createBucketNamePrompter, createBucketSourcePrompter } from '../ui/sam/bucketPrompter'
import { runInTerminal } from './processTerminal'
import { syncMementoRootKey } from './constants'

export interface SyncParams {
readonly paramsSource: ParamsSource
Expand All @@ -68,8 +69,6 @@ export interface SyncParams {
readonly syncFlags?: string
}

export const syncMementoRootKey = 'samcli.sync.params'

// TODO: hook this up so it prompts the user when more than 1 environment is present in `samconfig.toml`
export function createEnvironmentPrompter(config: SamConfig, environments = config.listEnvironments()) {
const recentEnvironmentName = getRecentResponse(syncMementoRootKey, config.location.fsPath, 'environmentName')
Expand Down
28 changes: 28 additions & 0 deletions packages/core/src/shared/sam/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import { telemetry } from '../telemetry/telemetry'
import globals from '../extensionGlobals'
import { getLogger } from '../logger/logger'
import { ChildProcessResult } from '../utilities/processUtils'
import { buildProcessMementoRootKey, globalIdentifier } from './constants'

/**
* @description determines the root directory of the project given Template Item
Expand Down Expand Up @@ -108,6 +109,33 @@ export async function updateRecentResponse(
getLogger().warn(`sam: unable to save response at key "${key}": %s`, err)
}
}

/**
* Returns true if there's an ongoing build process for the provided template, false otherwise
* @Param templatePath The path to the template.yaml file
*/
function isBuildInProgress(templatePath: string): boolean {
return getRecentResponse(buildProcessMementoRootKey, globalIdentifier, templatePath) !== undefined
}

/**
* Throws an error if there's a build in progress for the provided template
* @Param templatePath The path to the template.yaml file
*/
export function throwIfTemplateIsBeingBuilt(templatePath: string) {
if (isBuildInProgress(templatePath)) {
throw new ToolkitError('This template is already being built', { code: 'BuildInProgress' })
}
}

export async function registerTemplateBuild(templatePath: string) {
Copy link
Contributor

@justinmk3 justinmk3 Jan 26, 2025

Choose a reason for hiding this comment

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

"registering" is usually a sign of a design flaw. registries require lifecycle management, and can get "corrupted" by logic bugs (like any "cache" in general).

Is there instead a way we can check SAM CLI's state (e.g. in the .sam-... directory) to see if it's currently building something? Or a way to run sam ... and it will tell us if a build is in-progress? Calculating answers (querying) is much more reliable and robust than storing flags and cleaning up state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We currently can't check sam cli state in the toolkit. I think this is the best we can do right now, until we find a solution in SAM CLI.

await updateRecentResponse(buildProcessMementoRootKey, globalIdentifier, templatePath, 'true')
}

export async function unregisterTemplateBuild(templatePath: string) {
await updateRecentResponse(buildProcessMementoRootKey, globalIdentifier, templatePath, undefined)
}

export function getSamCliErrorMessage(stderr: string): string {
// Split the stderr string by newline, filter out empty lines, and get the last line
const lines = stderr
Expand Down
65 changes: 49 additions & 16 deletions packages/core/src/test/shared/sam/build.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
getBuildFlags,
ParamsSource,
runBuild,
SamBuildResult,
} from '../../../shared/sam/build'
import { TreeNode } from '../../../shared/treeview/resourceTreeDataProvider'
import { createWizardTester } from '../wizards/wizardTestUtils'
Expand All @@ -32,6 +33,7 @@ import { samconfigCompleteData, validTemplateData } from './samTestUtils'
import { CloudFormationTemplateRegistry } from '../../../shared/fs/templateRegistry'
import { getTestWindow } from '../vscode/window'
import { CancellationError } from '../../../shared/utilities/timeoutUtils'
import { SamAppLocation } from '../../../awsService/appBuilder/explorer/samProject'

describe('SAM BuildWizard', async function () {
const createTester = async (params?: Partial<BuildParams>, arg?: TreeNode | undefined) =>
Expand Down Expand Up @@ -388,22 +390,8 @@ describe('SAM runBuild', () => {
verifyCorrectDependencyCall()
})

it('[entry: appbuilder node] with default flags should instantiate correct process in terminal', async () => {
const prompterTester = PrompterTester.init()
.handleQuickPick('Specify parameter source for build', async (quickPick) => {
// Need sometime to wait for the template to search for template file
await quickPick.untilReady()

assert.strictEqual(quickPick.items.length, 2)
const items = quickPick.items
assert.strictEqual(quickPick.items.length, 2)
assert.deepStrictEqual(items[0], { data: ParamsSource.Specify, label: 'Specify build flags' })
assert.deepStrictEqual(items[1].label, 'Use default values')
quickPick.acceptItem(quickPick.items[1])
})
.build()

// Invoke sync command from command palette
it('[entry: appbuilder node] with default flags should instantiate correct process in terminal and show progress notification', async () => {
const prompterTester = getPrompterTester()
const expectedSamAppLocation = {
workspaceFolder: workspaceFolder,
samTemplateUri: templateFile,
Expand All @@ -412,6 +400,10 @@ describe('SAM runBuild', () => {

await runBuild(new AppNode(expectedSamAppLocation))

getTestWindow()
.getFirstMessage()
.assertProgress(`Building SAM template at ${expectedSamAppLocation.samTemplateUri.path}`)

assert.deepEqual(mockChildProcessClass.getCall(0).args, [
'sam-cli-path',
[
Expand All @@ -437,6 +429,27 @@ describe('SAM runBuild', () => {
prompterTester.assertCallAll()
})

it('[entry: appbuilder node] should throw an error when running two build processes in parallel for the same template', async () => {
const prompterTester = getPrompterTester()
const expectedSamAppLocation = {
workspaceFolder: workspaceFolder,
samTemplateUri: templateFile,
projectRoot: projectRoot,
}
await assert.rejects(
async () => {
await runInParallel(expectedSamAppLocation)
},
(e: any) => {
assert.strictEqual(e instanceof ToolkitError, true)
assert.strictEqual(e.message, 'This template is already being built')
assert.strictEqual(e.code, 'BuildInProgress')
return true
}
)
prompterTester.assertCallAll(undefined, 2)
})

it('[entry: command palette] use samconfig should instantiate correct process in terminal', async () => {
const samconfigFile = vscode.Uri.file(await testFolder.write('samconfig.toml', samconfigCompleteData))

Expand Down Expand Up @@ -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))])
}

function getPrompterTester() {
return PrompterTester.init()
.handleQuickPick('Specify parameter source for build', async (quickPick) => {
// Need sometime to wait for the template to search for template file
await quickPick.untilReady()

assert.strictEqual(quickPick.items.length, 2)
const items = quickPick.items
assert.strictEqual(quickPick.items.length, 2)
assert.deepStrictEqual(items[0], { data: ParamsSource.Specify, label: 'Specify build flags' })
assert.deepStrictEqual(items[1].label, 'Use default values')
quickPick.acceptItem(quickPick.items[1])
})
.build()
}
2 changes: 1 addition & 1 deletion packages/core/src/testInteg/sam.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -700,7 +700,7 @@ describe('SAM Integration Tests', async function () {
describe('SAM Build', async function () {
let workspaceFolder: vscode.WorkspaceFolder
// We're testing only one case (python 3.10 (ZIP)) on the integration tests. More niche cases are handled as unit tests.
const scenarioIndex = 2
const scenarioIndex = 3
const scenario = scenarios[scenarioIndex]
let testRoot: string
let sandbox: sinon.SinonSandbox
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"type": "Bug Fix",
"description": "SAM build: prevent running multiple build processes for the same template"
}
Loading