From b61463527ff53525ef7c00ecb4445df4866569f9 Mon Sep 17 00:00:00 2001 From: Rico Huijbers <rix0rrr@gmail.com> Date: Fri, 2 Jun 2023 10:29:03 +0200 Subject: [PATCH 1/7] chore(cli): print detected cycle if work graph stalls To help with diagnosing #25806, if the work graph can't make any progress anymore because of a dependency cycle, print the cycle that was found instead of all the remaining nodes. --- .../aws-custom-resource/sdk-api-metadata.json | 11 ------- packages/aws-cdk/lib/util/work-graph.ts | 29 +++++++++++++++++-- packages/aws-cdk/test/work-graph.test.ts | 7 +++-- 3 files changed, 32 insertions(+), 15 deletions(-) diff --git a/packages/aws-cdk-lib/custom-resources/lib/aws-custom-resource/sdk-api-metadata.json b/packages/aws-cdk-lib/custom-resources/lib/aws-custom-resource/sdk-api-metadata.json index 83d53e4eb03bc..c1f19ce849b8e 100644 --- a/packages/aws-cdk-lib/custom-resources/lib/aws-custom-resource/sdk-api-metadata.json +++ b/packages/aws-cdk-lib/custom-resources/lib/aws-custom-resource/sdk-api-metadata.json @@ -1284,16 +1284,5 @@ }, "internetmonitor": { "name": "InternetMonitor" - }, - "ivsrealtime": { - "prefix": "ivs-realtime", - "name": "IVSRealTime" - }, - "vpclattice": { - "prefix": "vpc-lattice", - "name": "VPCLattice" - }, - "osis": { - "name": "OSIS" } } \ No newline at end of file diff --git a/packages/aws-cdk/lib/util/work-graph.ts b/packages/aws-cdk/lib/util/work-graph.ts index c51a9a2b3f4e8..d35eaf8b9d166 100644 --- a/packages/aws-cdk/lib/util/work-graph.ts +++ b/packages/aws-cdk/lib/util/work-graph.ts @@ -272,7 +272,7 @@ export class WorkGraph { this.readyPool.sort((a, b) => (b.priority ?? 0) - (a.priority ?? 0)); if (this.readyPool.length === 0 && activeCount === 0 && pendingCount > 0) { - throw new Error(`Unable to make progress anymore among: ${this}`); + throw new Error(`Unable to make progress anymore, dependency cycle between remaining artifacts: ${this.findCycle().join(' -> ')}`); } } @@ -283,6 +283,31 @@ export class WorkGraph { } } } + + /** + * Find cycles in a graph + * + * Not the fastest, but effective and should be rare + */ + private findCycle(): string[] { + const self = this; + for (const nodeId of Object.keys(this.nodes)) { + const cycle = recurse(nodeId, [nodeId]); + if (cycle) { return cycle; } + } + return ['No cycle found!']; + + function recurse(nodeId: string, path: string[]): string[] | undefined { + for (const dep of self.nodes[nodeId].dependencies ?? []) { + if (dep === path[0]) { return [...path, dep]; } + + const cycle = recurse(dep, [...path, dep]); + if (cycle) { return cycle; } + } + + return undefined; + } + } } export interface WorkGraphActions { @@ -297,4 +322,4 @@ function sum(xs: number[]) { ret += x; } return ret; -} \ No newline at end of file +} diff --git a/packages/aws-cdk/test/work-graph.test.ts b/packages/aws-cdk/test/work-graph.test.ts index 5b7a2d1235ec8..8486d667e991f 100644 --- a/packages/aws-cdk/test/work-graph.test.ts +++ b/packages/aws-cdk/test/work-graph.test.ts @@ -328,6 +328,7 @@ describe('WorkGraph', () => { toDeploy: createArtifacts([ { id: 'A', type: 'stack', stackDependencies: ['A'] }, ]), + expectedError: 'A -> A', }, { scenario: 'A -> B, B -> A', @@ -335,6 +336,7 @@ describe('WorkGraph', () => { { id: 'A', type: 'stack', stackDependencies: ['B'] }, { id: 'B', type: 'stack', stackDependencies: ['A'] }, ]), + expectedError: 'A -> B -> A', }, { scenario: 'A, B -> C, C -> D, D -> B', @@ -344,12 +346,13 @@ describe('WorkGraph', () => { { id: 'C', type: 'stack', stackDependencies: ['D'] }, { id: 'D', type: 'stack', stackDependencies: ['B'] }, ]), + expectedError: 'B -> C -> D -> B', }, - ])('Failure - Graph Circular Dependencies - $scenario', async ({ toDeploy }) => { + ])('Failure - Graph Circular Dependencies - $scenario', async ({ toDeploy, expectedError }) => { const graph = new WorkGraph(); addTestArtifactsToGraph(toDeploy, graph); - await expect(graph.doParallel(1, callbacks)).rejects.toThrowError(/Unable to make progress anymore/); + await expect(graph.doParallel(1, callbacks)).rejects.toThrowError(new RegExp(`Unable to make progress.*${expectedError}`)); }); }); From 70f254c29f8b37c9c6adc1acaa14195c4c6569db Mon Sep 17 00:00:00 2001 From: Rico Huijbers <rix0rrr@gmail.com> Date: Fri, 2 Jun 2023 13:05:57 +0200 Subject: [PATCH 2/7] Faster cycle finding --- packages/aws-cdk/lib/util/work-graph.ts | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/packages/aws-cdk/lib/util/work-graph.ts b/packages/aws-cdk/lib/util/work-graph.ts index d35eaf8b9d166..4d565ade4be3b 100644 --- a/packages/aws-cdk/lib/util/work-graph.ts +++ b/packages/aws-cdk/lib/util/work-graph.ts @@ -290,6 +290,7 @@ export class WorkGraph { * Not the fastest, but effective and should be rare */ private findCycle(): string[] { + const seen = new Set<string>(); const self = this; for (const nodeId of Object.keys(this.nodes)) { const cycle = recurse(nodeId, [nodeId]); @@ -298,14 +299,24 @@ export class WorkGraph { return ['No cycle found!']; function recurse(nodeId: string, path: string[]): string[] | undefined { - for (const dep of self.nodes[nodeId].dependencies ?? []) { - if (dep === path[0]) { return [...path, dep]; } - - const cycle = recurse(dep, [...path, dep]); - if (cycle) { return cycle; } + if (seen.has(nodeId)) { + return undefined; } + try { + for (const dep of self.nodes[nodeId].dependencies ?? []) { + const index = path.indexOf(dep); + if (index > -1) { + return [...path.slice(index), dep]; + } - return undefined; + const cycle = recurse(dep, [...path, dep]); + if (cycle) { return cycle; } + } + + return undefined; + } finally { + seen.add(nodeId); + } } } } From 880aa27855fa9d63d5f6c11780d4af1c63171e8c Mon Sep 17 00:00:00 2001 From: Rico Huijbers <rix0rrr@gmail.com> Date: Fri, 2 Jun 2023 15:48:37 +0200 Subject: [PATCH 3/7] Undo changes to SDK-API-METADATA --- .../lib/aws-custom-resource/sdk-api-metadata.json | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/packages/aws-cdk-lib/custom-resources/lib/aws-custom-resource/sdk-api-metadata.json b/packages/aws-cdk-lib/custom-resources/lib/aws-custom-resource/sdk-api-metadata.json index c1f19ce849b8e..83d53e4eb03bc 100644 --- a/packages/aws-cdk-lib/custom-resources/lib/aws-custom-resource/sdk-api-metadata.json +++ b/packages/aws-cdk-lib/custom-resources/lib/aws-custom-resource/sdk-api-metadata.json @@ -1284,5 +1284,16 @@ }, "internetmonitor": { "name": "InternetMonitor" + }, + "ivsrealtime": { + "prefix": "ivs-realtime", + "name": "IVSRealTime" + }, + "vpclattice": { + "prefix": "vpc-lattice", + "name": "VPCLattice" + }, + "osis": { + "name": "OSIS" } } \ No newline at end of file From 6dcf810fb184f75c2b26b809de0e106a50a1b2dd Mon Sep 17 00:00:00 2001 From: Rico Huijbers <rix0rrr@gmail.com> Date: Fri, 2 Jun 2023 15:40:59 +0200 Subject: [PATCH 4/7] WIP --- packages/aws-cdk/lib/cdk-toolkit.ts | 2 +- packages/aws-cdk/lib/util/work-graph.ts | 35 ++++--- packages/aws-cdk/test/work-graph.test.ts | 114 +++++++++++++++++++++++ 3 files changed, 132 insertions(+), 19 deletions(-) diff --git a/packages/aws-cdk/lib/cdk-toolkit.ts b/packages/aws-cdk/lib/cdk-toolkit.ts index 7cf0ccc6b42f1..70d7650f58f7b 100644 --- a/packages/aws-cdk/lib/cdk-toolkit.ts +++ b/packages/aws-cdk/lib/cdk-toolkit.ts @@ -359,7 +359,7 @@ export class CdkToolkit { const graphConcurrency: Concurrency = { 'stack': concurrency, 'asset-build': 1, // This will be CPU-bound/memory bound, mostly matters for Docker builds - 'asset-publish': options.assetParallelism ? 8 : 1, // This will be I/O-bound, 8 in parallel seems reasonable + 'asset-publish': (options.assetParallelism ?? true) ? 8 : 1, // This will be I/O-bound, 8 in parallel seems reasonable }; await workGraph.doParallel(graphConcurrency, { diff --git a/packages/aws-cdk/lib/util/work-graph.ts b/packages/aws-cdk/lib/util/work-graph.ts index 4d565ade4be3b..4b544781822a3 100644 --- a/packages/aws-cdk/lib/util/work-graph.ts +++ b/packages/aws-cdk/lib/util/work-graph.ts @@ -244,35 +244,34 @@ export class WorkGraph { } private updateReadyPool() { - let activeCount = 0; - let pendingCount = 0; - for (const node of Object.values(this.nodes)) { - switch (node.deploymentState) { - case DeploymentState.DEPLOYING: - activeCount += 1; - break; - case DeploymentState.PENDING: - pendingCount += 1; - if (Array.from(node.dependencies).every((id) => this.node(id).deploymentState === DeploymentState.COMPLETED)) { - node.deploymentState = DeploymentState.QUEUED; - this.readyPool.push(node); - } - break; - } + const activeCount = Object.values(this.nodes).filter((x) => x.deploymentState === DeploymentState.DEPLOYING).length; + const pendingCount = Object.values(this.nodes).filter((x) => x.deploymentState === DeploymentState.PENDING).length; + + const newlyReady = Object.values(this.nodes).filter((x) => + x.deploymentState === DeploymentState.PENDING && + Array.from(x.dependencies).every((id) => this.node(id).deploymentState === DeploymentState.COMPLETED)); + + // Add newly available nodes to the ready pool + for (const node of newlyReady) { + node.deploymentState = DeploymentState.QUEUED; + this.readyPool.push(node); } + // Remove nodes from the ready pool that have already started deploying for (let i = 0; i < this.readyPool.length; i++) { const node = this.readyPool[i]; if (node.deploymentState !== DeploymentState.QUEUED) { this.readyPool.splice(i, 1); } + // FIXME: BUG } // Sort by reverse priority this.readyPool.sort((a, b) => (b.priority ?? 0) - (a.priority ?? 0)); if (this.readyPool.length === 0 && activeCount === 0 && pendingCount > 0) { - throw new Error(`Unable to make progress anymore, dependency cycle between remaining artifacts: ${this.findCycle().join(' -> ')}`); + const cycle = this.findCycle() ?? ['No cycle found!']; + throw new Error(`Unable to make progress anymore, dependency cycle between remaining artifacts: ${cycle.join(' -> ')}`); } } @@ -289,14 +288,14 @@ export class WorkGraph { * * Not the fastest, but effective and should be rare */ - private findCycle(): string[] { + public findCycle(): string[] | undefined { const seen = new Set<string>(); const self = this; for (const nodeId of Object.keys(this.nodes)) { const cycle = recurse(nodeId, [nodeId]); if (cycle) { return cycle; } } - return ['No cycle found!']; + return undefined; function recurse(nodeId: string, path: string[]): string[] | undefined { if (seen.has(nodeId)) { diff --git a/packages/aws-cdk/test/work-graph.test.ts b/packages/aws-cdk/test/work-graph.test.ts index 8486d667e991f..7f3de570d3ca6 100644 --- a/packages/aws-cdk/test/work-graph.test.ts +++ b/packages/aws-cdk/test/work-graph.test.ts @@ -9,6 +9,8 @@ const sleep = async (duration: number) => new Promise<void>((resolve) => setTime // a chance to start new tasks. const SLOW = 200; +jest.setTimeout(600_000); + /** * Repurposing unused stack attributes to create specific test scenarios * - stack.name = deployment duration @@ -356,6 +358,105 @@ describe('WorkGraph', () => { }); }); +// Generate a bunch of nodes and arbitrary dependencies with arbitrary speeds and check for race conditions +test('race condition checking', async () => { + const N = 1000; + for (let i = 0; i < N; i++) { + const graph = new WorkGraph(); + const M = 10; + const depchance = 0.2; + + const ids = range(M).map((id) => `a${id}`); + + for (const id of ids) { + const type = pick(['stack', 'asset-publish', 'asset-build']) as 'stack'|'asset-publish'|'asset-build'; + switch (type) { + case 'stack': + graph.addNodes({ + type, + id, + deploymentState: DeploymentState.PENDING, + dependencies: new Set<string>(), + stack: { + // We're smuggling information here so that the set of callbacks can do some appropriate action + stackName: Math.round(Math.random() * 50), // Timeout + displayName: '', + } as any, + }); + break; + case 'asset-build': + graph.addNodes({ + type, + id, + deploymentState: DeploymentState.PENDING, + dependencies: new Set<string>(), + asset: DUMMY, + assetManifest: DUMMY, + assetManifestArtifact: DUMMY, + parentStack: { + // We're smuggling information here so that the set of callbacks can do some appropriate action + stackName: `${Math.round(Math.random() * 100)}`, // Timeout + } as any, + }); + break; + case 'asset-publish': + graph.addNodes({ + type, + id, + deploymentState: DeploymentState.PENDING, + dependencies: new Set<string>(), + asset: DUMMY, + assetManifest: DUMMY, + assetManifestArtifact: DUMMY, + parentStack: { + // We're smuggling information here so that the set of callbacks can do some appropriate action + stackName: `${Math.round(Math.random() * 100)}`, // Timeout + } as any, + }); + break; + } + } + + for (const id of ids) { + const dependencies = new Set(ids.flatMap((did) => Math.random() < depchance && id !== did ? [did] : [])); + for (const dep of dependencies) { + graph.node(id).dependencies.add(dep); + if (graph.findCycle()) { + // Oops + graph.node(id).dependencies.delete(dep); + } + } + } + + const callbacks = { + deployStack: async (x: StackNode) => { + const timeout = Number(x.stack.stackName) || 0; + await sleep(timeout); + }, + buildAsset: async(x: AssetBuildNode) => { + const timeout = Number(x.parentStack.stackName) || 0; + await sleep(timeout); + }, + publishAsset: async(x: AssetPublishNode) => { + const timeout = Number(x.parentStack.stackName) || 0; + await sleep(timeout); + }, + }; + + try { + await graph.doParallel({ 'asset-build': 1, 'asset-publish': 1, 'stack': 1 }, callbacks); + // eslint-disable-next-line no-console + console.log(i); + } catch (e: any) { + // eslint-disable-next-line no-console + console.log(e.message); + if (!e.message.includes('->')) { + throw e; + } + } + } +}); + interface TestArtifact { stackDependencies?: string[]; assetDependencies?: string[]; @@ -410,4 +511,17 @@ function addTestArtifactsToGraph(toDeploy: TestArtifact[], graph: WorkGraph) { } } graph.removeUnavailableDependencies(); +} + +function range(n: number): number[] { + const ret = new Array<number>(); + for (let i = 0; i < n; i++) { + ret.push(i); + } + return ret; +} + +function pick<A>(xs: A[]): A { + const i = Math.min(Math.floor(Math.random() * xs.length), xs.length - 1); + return xs[i]; } \ No newline at end of file From 05f23b56a484c8fc3a2b0dcea11a2a1f5442e131 Mon Sep 17 00:00:00 2001 From: Rico Huijbers <rix0rrr@gmail.com> Date: Mon, 5 Jun 2023 10:40:34 +0200 Subject: [PATCH 5/7] Found the real problem --- .../aws-cdk/lib/util/work-graph-builder.ts | 79 ++++++------ packages/aws-cdk/lib/util/work-graph.ts | 67 +++++++++-- .../aws-cdk/test/work-graph-builder.test.ts | 71 +++++++---- packages/aws-cdk/test/work-graph.test.ts | 112 ------------------ 4 files changed, 146 insertions(+), 183 deletions(-) diff --git a/packages/aws-cdk/lib/util/work-graph-builder.ts b/packages/aws-cdk/lib/util/work-graph-builder.ts index 7c5a0d0ff3762..979c68e4d915e 100644 --- a/packages/aws-cdk/lib/util/work-graph-builder.ts +++ b/packages/aws-cdk/lib/util/work-graph-builder.ts @@ -50,8 +50,8 @@ export class WorkGraphBuilder { id: buildId, dependencies: new Set([ ...this.getDepIds(assetArtifact.dependencies), - // If we disable prebuild, then assets inherit dependencies from their parent stack - ...!this.prebuildAssets ? this.getDepIds(parentStack.dependencies) : [], + // If we disable prebuild, then assets inherit (stack) dependencies from their parent stack + ...!this.prebuildAssets ? this.getDepIds(onlyStacks(parentStack.dependencies)) : [], ]), parentStack, assetManifestArtifact: assetArtifact, @@ -66,27 +66,35 @@ export class WorkGraphBuilder { // Always add the publish const publishNodeId = `${this.idPrefix}${asset.id}-publish`; - this.graph.addNodes({ - type: 'asset-publish', - id: publishNodeId, - dependencies: new Set([ - buildId, - // The asset publish step also depends on the stacks that the parent depends on. - // This is purely cosmetic: if we don't do this, the progress printing of asset publishing - // is going to interfere with the progress bar of the stack deployment. We could remove this - // for overall faster deployments if we ever have a better method of progress displaying. - // Note: this may introduce a cycle if one of the parent's dependencies is another stack that - // depends on this asset. To workaround this we remove these cycles once all nodes have - // been added to the graph. - ...this.getDepIds(parentStack.dependencies.filter(cxapi.CloudFormationStackArtifact.isCloudFormationStackArtifact)), - ]), - parentStack, - assetManifestArtifact: assetArtifact, - assetManifest, - asset, - deploymentState: DeploymentState.PENDING, - priority: WorkGraphBuilder.PRIORITIES['asset-publish'], - }); + + const publishNode = this.graph.tryGetNode(publishNodeId); + if (!publishNode) { + this.graph.addNodes({ + type: 'asset-publish', + id: publishNodeId, + dependencies: new Set([ + buildId, + ]), + parentStack, + assetManifestArtifact: assetArtifact, + assetManifest, + asset, + deploymentState: DeploymentState.PENDING, + priority: WorkGraphBuilder.PRIORITIES['asset-publish'], + }); + } + + for (const inheritedDep of this.getDepIds(onlyStacks(parentStack.dependencies))) { + // The asset publish step also depends on the stacks that the parent depends on. + // This is purely cosmetic: if we don't do this, the progress printing of asset publishing + // is going to interfere with the progress bar of the stack deployment. We could remove this + // for overall faster deployments if we ever have a better method of progress displaying. + // Note: this may introduce a cycle if one of the parent's dependencies is another stack that + // depends on this asset. To workaround this we remove these cycles once all nodes have + // been added to the graph. + this.graph.addDependency(publishNodeId, inheritedDep); + } + // This will work whether the stack node has been added yet or not this.graph.addDependency(`${this.idPrefix}${parentStack.id}`, publishNodeId); } @@ -137,20 +145,17 @@ export class WorkGraphBuilder { return ids; } + /** + * We may have accidentally introduced cycles in an attempt to make the messages printed to the + * console not interfere with each other too much. Remove them again. + */ private removeStackPublishCycles() { - const stacks = this.graph.nodesOfType('stack'); - for (const stack of stacks) { - for (const dep of stack.dependencies) { - const node = this.graph.nodes[dep]; - - if (!node || node.type !== 'asset-publish' || !node.dependencies.has(stack.id)) { - continue; + const publishSteps = this.graph.nodesOfType('asset-publish'); + for (const publishStep of publishSteps) { + for (const dep of publishStep.dependencies) { + if (this.graph.reachable(dep, publishStep.id)) { + publishStep.dependencies.delete(dep); } - - // Delete the dependency from the asset-publish onto the stack. - // The publish -> stack dependencies are purely cosmetic to prevent publish output - // from interfering with the progress bar of the stack deployment. - node.dependencies.delete(stack.id); } } } @@ -166,4 +171,8 @@ function stacksFromAssets(artifacts: cxapi.CloudArtifact[]) { } return ret; +} + +function onlyStacks(artifacts: cxapi.CloudArtifact[]) { + return artifacts.filter(cxapi.CloudFormationStackArtifact.isCloudFormationStackArtifact); } \ No newline at end of file diff --git a/packages/aws-cdk/lib/util/work-graph.ts b/packages/aws-cdk/lib/util/work-graph.ts index 4b544781822a3..be4d7f82ce3dd 100644 --- a/packages/aws-cdk/lib/util/work-graph.ts +++ b/packages/aws-cdk/lib/util/work-graph.ts @@ -14,6 +14,10 @@ export class WorkGraph { public addNodes(...nodes: WorkNode[]) { for (const node of nodes) { + if (this.nodes[node.id]) { + throw new Error(`Duplicate use of node id: ${node.id}`); + } + const ld = this.lazyDependencies.get(node.id); if (ld) { for (const x of ld) { @@ -72,6 +76,10 @@ export class WorkGraph { lazyDeps.push(toId); } + public tryGetNode(id: string): WorkNode | undefined { + return this.nodes[id]; + } + public node(id: string) { const ret = this.nodes[id]; if (!ret) { @@ -198,9 +206,24 @@ export class WorkGraph { } public toString() { - return Object.entries(this.nodes).map(([id, node]) => - `${id} := ${node.deploymentState} ${node.type} ${node.dependencies.size > 0 ? `(${Array.from(node.dependencies)})` : ''}`.trim(), - ).join(', '); + return [ + 'digraph D {', + ...Object.entries(this.nodes).flatMap(([id, node]) => renderNode(id, node)), + '}', + ].join('\n'); + + function renderNode(id: string, node: WorkNode): string[] { + const ret = []; + if (node.deploymentState === DeploymentState.COMPLETED) { + ret.push(` "${id}" [style=filled,fillcolor=yellow];`); + } else { + ret.push(` "${id}";`); + } + for (const dep of node.dependencies) { + ret.push(` "${id}" -> "${dep}";`); + } + return ret; + } } /** @@ -258,13 +281,7 @@ export class WorkGraph { } // Remove nodes from the ready pool that have already started deploying - for (let i = 0; i < this.readyPool.length; i++) { - const node = this.readyPool[i]; - if (node.deploymentState !== DeploymentState.QUEUED) { - this.readyPool.splice(i, 1); - } - // FIXME: BUG - } + retainOnly(this.readyPool, (node) => node.deploymentState === DeploymentState.QUEUED); // Sort by reverse priority this.readyPool.sort((a, b) => (b.priority ?? 0) - (a.priority ?? 0)); @@ -318,6 +335,32 @@ export class WorkGraph { } } } + + /** + * Whether the `end` node is reachable from the `start` node, following the dependency arrows + */ + public reachable(start: string, end: string): boolean { + const seen = new Set<string>(); + const self = this; + return recurse(start); + + function recurse(current: string) { + if (seen.has(current)) { + return false; + } + seen.add(current); + + if (current === end) { + return true; + } + for (const dep of self.nodes[current].dependencies) { + if (recurse(dep)) { + return true; + } + } + return false; + } + } } export interface WorkGraphActions { @@ -333,3 +376,7 @@ function sum(xs: number[]) { } return ret; } + +function retainOnly<A>(xs: A[], pred: (x: A) => boolean) { + xs.splice(0, xs.length, ...xs.filter(pred)); +} \ No newline at end of file diff --git a/packages/aws-cdk/test/work-graph-builder.test.ts b/packages/aws-cdk/test/work-graph-builder.test.ts index 39e01cc77c911..e1b378a34cd55 100644 --- a/packages/aws-cdk/test/work-graph-builder.test.ts +++ b/packages/aws-cdk/test/work-graph-builder.test.ts @@ -109,7 +109,7 @@ test('dependencies on unselected artifacts are silently ignored', async () => { })); }); -test('assets with shared contents between dependant stacks', async () => { +describe('tests that use assets', () => { const files = { // Referencing an existing file on disk is important here. // It means these two assets will have the same AssetManifest @@ -121,36 +121,55 @@ test('assets with shared contents between dependant stacks', async () => { }, }, }; + const environment = 'aws://11111/us-east-1'; - addStack(rootBuilder, 'StackA', { - environment: 'aws://11111/us-east-1', - dependencies: ['StackA.assets'], - }); - addAssets(rootBuilder, 'StackA.assets', { files }); + test('assets with shared contents between dependant stacks', async () => { + addStack(rootBuilder, 'StackA', { + environment: 'aws://11111/us-east-1', + dependencies: ['StackA.assets'], + }); + addAssets(rootBuilder, 'StackA.assets', { files }); - addStack(rootBuilder, 'StackB', { - environment: 'aws://11111/us-east-1', - dependencies: ['StackB.assets', 'StackA'], + addStack(rootBuilder, 'StackB', { + environment: 'aws://11111/us-east-1', + dependencies: ['StackB.assets', 'StackA'], + }); + addAssets(rootBuilder, 'StackB.assets', { files }); + + const assembly = rootBuilder.buildAssembly(); + + const traversal: string[] = []; + const graph = new WorkGraphBuilder(true).build(assembly.artifacts); + await graph.doParallel(1, { + deployStack: async (node) => { traversal.push(node.id); }, + buildAsset: async (node) => { traversal.push(node.id); }, + publishAsset: async (node) => { traversal.push(node.id); }, + }); + + expect(traversal).toHaveLength(4); // 1 asset build, 1 asset publish, 2 stacks + expect(traversal).toEqual([ + 'work-graph-builder.test.js:D1-build', + 'work-graph-builder.test.js:D1-publish', + 'StackA', + 'StackB', + ]); }); - addAssets(rootBuilder, 'StackB.assets', { files }); - const assembly = rootBuilder.buildAssembly(); + test('a more complex way to make a cycle', async () => { + // A -> B -> C | A and C share an asset. The asset will have a dependency on B, that is not a *direct* reverse dependency, and will cause a cycle. + addStack(rootBuilder, 'StackA', { environment, dependencies: ['StackA.assets', 'StackB'] }); + addAssets(rootBuilder, 'StackA.assets', { files }); - const traversal: string[] = []; - const graph = new WorkGraphBuilder(true).build(assembly.artifacts); - await graph.doParallel(1, { - deployStack: async (node) => { traversal.push(node.id); }, - buildAsset: async (node) => { traversal.push(node.id); }, - publishAsset: async (node) => { traversal.push(node.id); }, - }); - - expect(traversal).toHaveLength(4); // 1 asset build, 1 asset publish, 2 stacks - expect(traversal).toEqual([ - 'work-graph-builder.test.js:D1-build', - 'work-graph-builder.test.js:D1-publish', - 'StackA', - 'StackB', - ]); + addStack(rootBuilder, 'StackB', { environment, dependencies: ['StackC'] }); + + addStack(rootBuilder, 'StackC', { environment, dependencies: ['StackC.assets'] }); + addAssets(rootBuilder, 'StackC.assets', { files }); + + const assembly = rootBuilder.buildAssembly(); + const graph = new WorkGraphBuilder(true).build(assembly.artifacts); + + expect(graph.findCycle()).toBeUndefined(); + }); }); /** diff --git a/packages/aws-cdk/test/work-graph.test.ts b/packages/aws-cdk/test/work-graph.test.ts index 7f3de570d3ca6..44c86845bcfe1 100644 --- a/packages/aws-cdk/test/work-graph.test.ts +++ b/packages/aws-cdk/test/work-graph.test.ts @@ -358,105 +358,6 @@ describe('WorkGraph', () => { }); }); -// Generate a bunch of nodes and arbitrary dependencies with arbitrary speeds and check for race conditions -test('race condition checking', async () => { - const N = 1000; - for (let i = 0; i < N; i++) { - const graph = new WorkGraph(); - const M = 10; - const depchance = 0.2; - - const ids = range(M).map((id) => `a${id}`); - - for (const id of ids) { - const type = pick(['stack', 'asset-publish', 'asset-build']) as 'stack'|'asset-publish'|'asset-build'; - switch (type) { - case 'stack': - graph.addNodes({ - type, - id, - deploymentState: DeploymentState.PENDING, - dependencies: new Set<string>(), - stack: { - // We're smuggling information here so that the set of callbacks can do some appropriate action - stackName: Math.round(Math.random() * 50), // Timeout - displayName: '', - } as any, - }); - break; - case 'asset-build': - graph.addNodes({ - type, - id, - deploymentState: DeploymentState.PENDING, - dependencies: new Set<string>(), - asset: DUMMY, - assetManifest: DUMMY, - assetManifestArtifact: DUMMY, - parentStack: { - // We're smuggling information here so that the set of callbacks can do some appropriate action - stackName: `${Math.round(Math.random() * 100)}`, // Timeout - } as any, - }); - break; - case 'asset-publish': - graph.addNodes({ - type, - id, - deploymentState: DeploymentState.PENDING, - dependencies: new Set<string>(), - asset: DUMMY, - assetManifest: DUMMY, - assetManifestArtifact: DUMMY, - parentStack: { - // We're smuggling information here so that the set of callbacks can do some appropriate action - stackName: `${Math.round(Math.random() * 100)}`, // Timeout - } as any, - }); - break; - } - } - - for (const id of ids) { - const dependencies = new Set(ids.flatMap((did) => Math.random() < depchance && id !== did ? [did] : [])); - for (const dep of dependencies) { - graph.node(id).dependencies.add(dep); - if (graph.findCycle()) { - // Oops - graph.node(id).dependencies.delete(dep); - } - } - } - - const callbacks = { - deployStack: async (x: StackNode) => { - const timeout = Number(x.stack.stackName) || 0; - await sleep(timeout); - }, - buildAsset: async(x: AssetBuildNode) => { - const timeout = Number(x.parentStack.stackName) || 0; - await sleep(timeout); - }, - publishAsset: async(x: AssetPublishNode) => { - const timeout = Number(x.parentStack.stackName) || 0; - await sleep(timeout); - }, - }; - - try { - await graph.doParallel({ 'asset-build': 1, 'asset-publish': 1, 'stack': 1 }, callbacks); - // eslint-disable-next-line no-console - console.log(i); - } catch (e: any) { - // eslint-disable-next-line no-console - console.log(e.message); - if (!e.message.includes('->')) { - throw e; - } - } - } -}); - interface TestArtifact { stackDependencies?: string[]; assetDependencies?: string[]; @@ -511,17 +412,4 @@ function addTestArtifactsToGraph(toDeploy: TestArtifact[], graph: WorkGraph) { } } graph.removeUnavailableDependencies(); -} - -function range(n: number): number[] { - const ret = new Array<number>(); - for (let i = 0; i < n; i++) { - ret.push(i); - } - return ret; -} - -function pick<A>(xs: A[]): A { - const i = Math.min(Math.floor(Math.random() * xs.length), xs.length - 1); - return xs[i]; } \ No newline at end of file From 2b5e0dcec47513b69d1e46a70b6f2d5b4d4d2876 Mon Sep 17 00:00:00 2001 From: Rico Huijbers <rix0rrr@gmail.com> Date: Mon, 5 Jun 2023 10:53:28 +0200 Subject: [PATCH 6/7] Remove increased timeout --- packages/aws-cdk/test/work-graph.test.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/aws-cdk/test/work-graph.test.ts b/packages/aws-cdk/test/work-graph.test.ts index 44c86845bcfe1..8486d667e991f 100644 --- a/packages/aws-cdk/test/work-graph.test.ts +++ b/packages/aws-cdk/test/work-graph.test.ts @@ -9,8 +9,6 @@ const sleep = async (duration: number) => new Promise<void>((resolve) => setTime // a chance to start new tasks. const SLOW = 200; -jest.setTimeout(600_000); - /** * Repurposing unused stack attributes to create specific test scenarios * - stack.name = deployment duration From 06436ad7543e8cc00e3003daa56ecc027c20825c Mon Sep 17 00:00:00 2001 From: Rico Hermans <rix0rrr@gmail.com> Date: Mon, 5 Jun 2023 11:21:12 +0200 Subject: [PATCH 7/7] Update work-graph-builder.test.ts --- packages/aws-cdk/test/work-graph-builder.test.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/aws-cdk/test/work-graph-builder.test.ts b/packages/aws-cdk/test/work-graph-builder.test.ts index e1b378a34cd55..e89d8e35e911d 100644 --- a/packages/aws-cdk/test/work-graph-builder.test.ts +++ b/packages/aws-cdk/test/work-graph-builder.test.ts @@ -168,6 +168,7 @@ describe('tests that use assets', () => { const assembly = rootBuilder.buildAssembly(); const graph = new WorkGraphBuilder(true).build(assembly.artifacts); + // THEN expect(graph.findCycle()).toBeUndefined(); }); }); @@ -249,4 +250,4 @@ function assertableNode<A extends WorkNode>(x: A) { ...x, dependencies: Array.from(x.dependencies), }; -} \ No newline at end of file +}