fix(toolkit-lib): fromAssemblyBuilder mutates globally shared process.env (#528)

The Toolkit communicates with the CDK app passing environment variables
(containing context, and others).

If the CDK app is executed as a subprocess it is trivial to set those
environment variables when executing the subprocess; but for an
in-memory synthesis as allowed by `fromAssemblyBuilder` mutating
`process.env` is not the correct approach: it is clobbering global
state, and multiple async assembly builders might fight with each other
over the single shared global object.

At the same time, it is *extreeeeeemely* convenient for an app author to
not have to think about passing props.

So we leave the current behavior as a convenient default, but add the
option `clobberEnv: false` to explicitly disable this.

In `fromAssemblyBuilder()`, we now pass an additional `env` prop
containing the environment variables we applied (or didn't). There were
already parametes for `outdir` and `context`; with `clobberEnv: false`
those *must* now be passed to an `App` instantiated inside the builder
(whereas otherwise the `App` will pick up on the globally mutated
`process.env` changes).

The rest of this PR is reorganizing the code to make the code that
touches the environment return values instead of directly applying them.

* Remove `withEnv()` and `withContext()`.
* Move logging of the final context object out to the call site instead
of the builder function.

---
By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache-2.0 license

Author: rix0rrr

packages/@aws-cdk/cli-lib-alpha/lib/aws-cdk.ts

@@ -1,6 +1,7 @@
 /* eslint-disable import/no-extraneous-dependencies */
 /* eslint-disable import/no-relative-packages */
-export { prepareContext, prepareDefaultEnvironment } from '../../../aws-cdk/lib/api/cloud-assembly';
+export { contextFromSettings, prepareDefaultEnvironment } from '../../../aws-cdk/lib/api/cloud-assembly';
 export { createAssembly } from '../../../aws-cdk/lib/cxapp/exec';
 export { exec } from '../../../aws-cdk';
 export { debug } from '../../../aws-cdk/lib/logging';
+export { synthParametersFromSettings, writeContextToEnv } from '../../../aws-cdk/lib/api/cloud-assembly';

packages/@aws-cdk/cli-lib-alpha/lib/cli.ts

@@ -1,4 +1,5 @@
-import { exec as runCli, debug, createAssembly, prepareContext, prepareDefaultEnvironment } from './aws-cdk';
+import { format } from 'util';
+import { exec as runCli, debug, createAssembly, prepareDefaultEnvironment, synthParametersFromSettings, writeContextToEnv } from './aws-cdk';
 import type { SharedOptions, DeployOptions, DestroyOptions, BootstrapOptions, SynthOptions, ListOptions } from './commands';
 import { StackActivityProgress, HotswapMode } from './commands';
 
@@ -122,10 +123,26 @@ export class AwsCdkCli implements IAwsCdkCli {
   public static fromCloudAssemblyDirectoryProducer(producer: ICloudAssemblyDirectoryProducer) {
     return new AwsCdkCli(async (args) => changeDir(
       () => runCli(args, async (sdk, config) => {
-        const env = await prepareDefaultEnvironment(sdk, debugFn);
-        const context = await prepareContext(config.settings, config.context.all, env, debugFn);
-
-        return withEnv(async() => createAssembly(await producer.produce(context)), env);
+        const params = synthParametersFromSettings(config.settings);
+
+        const fullCtx = {
+          ...config.context.all,
+          ...params.context,
+        };
+        await debugFn(format('context:', fullCtx));
+
+        const env = {
+          ...process.env,
+          ...await prepareDefaultEnvironment(sdk, debugFn),
+          ...params.env,
+        };
+
+        const cleanupContext = await writeContextToEnv(env, fullCtx);
+        try {
+          return await withEnv(async() => createAssembly(await producer.produce(fullCtx)), env);
+        } finally {
+          await cleanupContext();
+        }
       }),
       producer.workingDirectory,
     ));

packages/@aws-cdk/toolkit-lib/README.md

@@ -331,6 +331,11 @@ const cx = await cdk.fromAssemblyBuilder(async () => {
 });
 ```
 
+> Note that synthesis using the `AssemblyBuilder` is not safe to be performed
+> concurrently, since it mutates the globally shared `process.env`. You can set
+> `clobberEnv: false` to avoid this, but your builder function will need to do a
+> bit more work. See the documentation of `clobberEnv` for an example.
+
 Existing _Cloud Assembly_ directories can be used as source like this:
 
 ```ts

packages/@aws-cdk/toolkit-lib/lib/api/cloud-assembly/environment.ts

@@ -1,5 +1,4 @@
 import * as path from 'path';
-import { format } from 'util';
 import * as cxapi from '@aws-cdk/cx-api';
 import * as fs from 'fs-extra';
 import type { SdkProvider } from '../aws-auth/private';
@@ -36,20 +35,14 @@ export async function prepareDefaultEnvironment(
 }
 
 /**
- * Settings related to synthesis are read from context.
- * The merging of various configuration sources like cli args or cdk.json has already happened.
- * We now need to set the final values to the context.
+ * Create context from settings.
+ *
+ * Mutates the `context` object and returns it.
  */
-export async function prepareContext(
+export function contextFromSettings(
   settings: Settings,
-  context: { [key: string]: any },
-  env: { [key: string]: string | undefined },
-  debugFn: (msg: string) => Promise<void>,
 ) {
-  const debugMode: boolean = settings.get(['debug']) ?? true;
-  if (debugMode) {
-    env.CDK_DEBUG = 'true';
-  }
+  const context: Record<string, unknown> = {};
 
   const pathMetadata: boolean = settings.get(['pathMetadata']) ?? true;
   if (pathMetadata) {
@@ -78,11 +71,23 @@ export async function prepareContext(
   const bundlingStacks = settings.get(['bundlingStacks']) ?? ['**'];
   context[cxapi.BUNDLING_STACKS] = bundlingStacks;
 
-  await debugFn(format('context:', context));
-
   return context;
 }
 
+/**
+ * Convert settings to context/environment variables
+ */
+export function synthParametersFromSettings(settings: Settings) {
+  return {
+    context: contextFromSettings(settings),
+    env: {
+      // An environment variable instead of a context variable, so it can also
+      // be accessed in framework code where we don't have access to a construct tree.
+      ...settings.get(['debug']) ? { CDK_DEBUG: 'true' } : {},
+    },
+  };
+}
+
 export function spaceAvailableForContext(env: { [key: string]: string }, limit: number) {
   const size = (value: string) => value != null ? Buffer.byteLength(value) : 0;
 

packages/@aws-cdk/toolkit-lib/lib/api/cloud-assembly/private/exec.ts

@@ -7,7 +7,7 @@ type EventPublisher = (event: 'open' | 'data_stdout' | 'data_stderr' | 'close',
 
 interface ExecOptions {
   eventPublisher?: EventPublisher;
-  extraEnv?: { [key: string]: string | undefined };
+  env?: { [key: string]: string | undefined };
   cwd?: string;
 }
 
@@ -29,10 +29,7 @@ export async function execInChildProcess(commandAndArgs: string, options: ExecOp
       detached: false,
       shell: true,
       cwd: options.cwd,
-      env: {
-        ...process.env,
-        ...(options.extraEnv ?? {}),
-      },
+      env: options.env,
     });
 
     const eventPublisher: EventPublisher = options.eventPublisher ?? ((type, line) => {

packages/@aws-cdk/toolkit-lib/lib/api/cloud-assembly/private/prepare-source.ts

@@ -16,7 +16,7 @@ import type { IReadLock, IWriteLock } from '../../rwlock';
 import { RWLock } from '../../rwlock';
 import { Settings } from '../../settings';
 import { loadTree, some } from '../../tree';
-import { prepareDefaultEnvironment as oldPrepare, prepareContext, spaceAvailableForContext, guessExecutable } from '../environment';
+import { prepareDefaultEnvironment as oldPrepare, spaceAvailableForContext, guessExecutable } from '../environment';
 import type { AppSynthOptions, LoadAssemblyOptions } from '../source-builder';
 
 type Env = { [key: string]: string };
@@ -168,59 +168,45 @@ export class ExecutionEnvironment implements AsyncDisposable {
       }
     }
   }
+}
 
-  /**
-   * Run code with additional environment variables
-   */
-  public async withEnv<T>(env: Env = {}, block: () => Promise<T>) {
-    const originalEnv = process.env;
-    try {
-      process.env = {
-        ...originalEnv,
-        ...env,
-      };
-
-      return await block();
-    } finally {
-      process.env = originalEnv;
-    }
-  }
-
-  /**
-   * Run code with context setup inside the environment
-   */
-  public async withContext<T>(
-    inputContext: Context,
-    env: Env,
-    synthOpts: AppSynthOptions = {},
-    block: (env: Env, context: Context) => Promise<T>,
-  ) {
-    const context = await prepareContext(synthOptsDefaults(synthOpts), inputContext, env, this.debugFn);
-    let contextOverflowLocation = null;
+/**
+ * Serializes the given context to a set if environment variables environment variables
+ *
+ * Needs to know the size of the rest of the env because that's necessary to do
+ * an overflow computation on Windows. This function will mutate the given
+ * environment in-place. It should be called as the very last operation on the
+ * environment, because afterwards is might be at the maximum size.
+ *
+ * This *would* have returned an `IAsyncDisposable` but that requires messing
+ * with TypeScript type definitions to use it in aws-cdk, so returning an
+ * explicit cleanup function is easier.
+ */
+export function writeContextToEnv(env: Env, context: Context) {
+  let contextOverflowLocation = null;
 
-    try {
-      const envVariableSizeLimit = os.platform() === 'win32' ? 32760 : 131072;
-      const [smallContext, overflow] = splitBySize(context, spaceAvailableForContext(env, envVariableSizeLimit));
+  // On Windows, all envvars together must fit in a 32k block (<https://devblogs.microsoft.com/oldnewthing/20100203-00>)
+  // On Linux, a single entry may not exceed 131k; but we're treating it as all together because that's safe
+  // and it's a single execution path for both platforms.
+  const envVariableSizeLimit = os.platform() === 'win32' ? 32760 : 131072;
+  const [smallContext, overflow] = splitBySize(context, spaceAvailableForContext(env, envVariableSizeLimit));
 
-      // Store the safe part in the environment variable
-      env[cxapi.CONTEXT_ENV] = JSON.stringify(smallContext);
+  // Store the safe part in the environment variable
+  env[cxapi.CONTEXT_ENV] = JSON.stringify(smallContext);
 
-      // If there was any overflow, write it to a temporary file
-      if (Object.keys(overflow ?? {}).length > 0) {
-        const contextDir = fs.mkdtempSync(path.join(os.tmpdir(), 'cdk-context'));
-        contextOverflowLocation = path.join(contextDir, 'context-overflow.json');
-        fs.writeJSONSync(contextOverflowLocation, overflow);
-        env[cxapi.CONTEXT_OVERFLOW_LOCATION_ENV] = contextOverflowLocation;
-      }
+  // If there was any overflow, write it to a temporary file
+  if (Object.keys(overflow ?? {}).length > 0) {
+    const contextDir = fs.mkdtempSync(path.join(os.tmpdir(), 'cdk-context'));
+    contextOverflowLocation = path.join(contextDir, 'context-overflow.json');
+    fs.writeJSONSync(contextOverflowLocation, overflow);
+    env[cxapi.CONTEXT_OVERFLOW_LOCATION_ENV] = contextOverflowLocation;
+  }
 
-      // call the block code with new environment
-      return await block(env, context);
-    } finally {
-      if (contextOverflowLocation) {
-        fs.removeSync(path.dirname(contextOverflowLocation));
-      }
+  return async () => {
+    if (contextOverflowLocation) {
+      await fs.promises.rm(path.dirname(contextOverflowLocation), { recursive: true, force: true });
     }
-  }
+  };
 }
 
 /**
@@ -270,7 +256,7 @@ export async function assemblyFromDirectory(assemblyDir: string, ioHelper: IoHel
   }
 }
 
-function synthOptsDefaults(synthOpts: AppSynthOptions = {}): Settings {
+export function settingsFromSynthOptions(synthOpts: AppSynthOptions = {}): Settings {
   return new Settings({
     debug: false,
     pathMetadata: true,