From 41a772ee470410a837c08c61461c04e6042e6de7 Mon Sep 17 00:00:00 2001 From: Tiffany Trinh Date: Fri, 7 Aug 2026 01:00:57 -0700 Subject: [PATCH 1/2] =?UTF-8?q?feat(apps):=20harden=20local=20execution=20?= =?UTF-8?q?=E2=80=94=20serialization,=20Source,=20edge=20cases?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Serializes local backend-function executions via a promise-chain queue, since @datadog/action-catalog and @datadog/apps-backend both register runtime context via a shared, module-level setter that isn't safe under concurrent in-process execution. Also populates $.Source with a synthetic local-dev identity (deferred from Milestone 0), and adds edge-case coverage: non-serializable results, a top-level module throw, and a real concurrent-execution test against a genuine @datadog/apps-backend typed import confirming no cross-execution state leakage. --- .../apps/src/vite/local-execution.test.ts | 149 +++++++++++++++++- .../plugins/apps/src/vite/local-execution.ts | 71 ++++++++- 2 files changed, 213 insertions(+), 7 deletions(-) diff --git a/packages/plugins/apps/src/vite/local-execution.test.ts b/packages/plugins/apps/src/vite/local-execution.test.ts index c04f34184..16b23e6d9 100644 --- a/packages/plugins/apps/src/vite/local-execution.test.ts +++ b/packages/plugins/apps/src/vite/local-execution.test.ts @@ -34,6 +34,8 @@ function loadModuleReturning(exports: Record): LoadModule { }; } +const ORDER_MARKER = '__ddLocalExecutionTestOrder'; + describe('local-execution — executeScriptLocally', () => { test('Should run a simple function in-process and return its result', async () => { const result = await executeScriptLocally( @@ -77,6 +79,18 @@ describe('local-execution — executeScriptLocally', () => { ).rejects.toThrow(`"example" is not a function exported from ${func.absolutePath}`); }); + test('Should reject when loadModule itself rejects, same as a native-module load failure would', async () => { + // Simulates e.g. a native addon failing to load at require()/import + // time, rather than a customer function throwing during its own + // logic — the failure happens before the function is ever reached. + const loadModule: LoadModule = async () => { + throw new Error('cannot find native module'); + }; + await expect( + executeScriptLocally(func, [], stubExecuteAction, loadModule, mockLogger), + ).rejects.toThrow('cannot find native module'); + }); + test('Should resolve a $.Actions.foo.bar(...) call through the injected executeAction, including connectionId', async () => { const executeAction = jest.fn().mockResolvedValue({ ok: true }); const result = await executeScriptLocally( @@ -156,7 +170,20 @@ describe('local-execution — executeScriptLocally', () => { ).rejects.toThrow(/timed out after 50ms/); }); - test('Should never expose an auth token via globalThis', async () => { + test('Should never expose an auth token to the customer module — only backendFunctionArgs, Actions, and Source are visible on globalThis.$', async () => { + const result = await executeScriptLocally( + func, + [], + stubExecuteAction, + loadModuleReturning({ + example: () => Object.keys((globalThis as Record).$).sort(), + }), + mockLogger, + ); + expect(result).toEqual({ data: ['Actions', 'Source', 'backendFunctionArgs'] }); + }); + + test('Should never expose an auth token via globalThis either', async () => { const result = await executeScriptLocally( func, [], @@ -245,29 +272,139 @@ describe('local-execution — executeScriptLocally', () => { }); }); + describe('non-serializable results', () => { + test('Should reject with a clear, attributed error when the result has a circular reference', async () => { + await expect( + executeScriptLocally( + func, + [], + stubExecuteAction, + loadModuleReturning({ + example: () => { + const o: Record = {}; + o.self = o; + return o; + }, + }), + mockLogger, + ), + ).rejects.toThrow(/example.*can't be serialized to JSON/); + }); + + test('Should reject with a clear, attributed error when the result contains a BigInt', async () => { + await expect( + executeScriptLocally( + func, + [], + stubExecuteAction, + loadModuleReturning({ example: () => BigInt(10) }), + mockLogger, + ), + ).rejects.toThrow(/example.*can't be serialized to JSON/); + }); + + test('Should reject with a clear, attributed error when the result is a bare function (silently dropped by JSON.stringify)', async () => { + await expect( + executeScriptLocally( + func, + [], + stubExecuteAction, + loadModuleReturning({ example: () => function notSerializable() {} }), + mockLogger, + ), + ).rejects.toThrow(/example.*JSON.stringify silently drops/); + }); + + test('Should allow an explicit undefined result through unchanged', async () => { + const result = await executeScriptLocally( + func, + [], + stubExecuteAction, + loadModuleReturning({ example: () => undefined }), + mockLogger, + ); + expect(result).toEqual({ data: undefined }); + }); + }); + describe('serialization of concurrent executions', () => { - function delayedResult(label: T, delayMs: number): () => Promise { - return () => new Promise((resolve) => setTimeout(() => resolve(label), delayMs)); + beforeEach(() => { + delete (globalThis as Record)[ORDER_MARKER]; + }); + + function recordingOrder(label: string, delayMs: number): () => Promise { + return async () => { + const marker = + ((globalThis as Record)[ORDER_MARKER] as string[]) ?? []; + (globalThis as Record)[ORDER_MARKER] = marker; + marker.push(`start-${label}`); + await new Promise((r) => setTimeout(r, delayMs)); + marker.push(`end-${label}`); + return label; + }; } - test("Should allow two independent calls to run without cross-contaminating each other's result", async () => { + test('Should never interleave two concurrent executions — the second never starts until the first fully finishes', async () => { const [resultA, resultB] = await Promise.all([ executeScriptLocally( func, [], stubExecuteAction, - loadModuleReturning({ example: delayedResult('A', 20) }), + loadModuleReturning({ example: recordingOrder('A', 20) }), mockLogger, ), executeScriptLocally( func, [], stubExecuteAction, - loadModuleReturning({ example: delayedResult('B', 0) }), + loadModuleReturning({ example: recordingOrder('B', 0) }), mockLogger, ), ]); + expect([resultA, resultB]).toEqual([{ data: 'A' }, { data: 'B' }]); + const order = (globalThis as Record)[ORDER_MARKER]; + // Whichever call the queue happened to run first, its start/end + // pair must be adjacent — never interrupted by the other call's + // start. A real race (no queueing) would produce + // ['start-A', 'start-B', 'end-B', 'end-A'] here, since B's 0ms + // delay would let it finish first if both started immediately. + expect(order).toEqual([ + expect.stringMatching(/^start-/), + expect.stringMatching(/^end-/), + expect.stringMatching(/^start-/), + expect.stringMatching(/^end-/), + ]); + expect((order as string[])[0].slice('start-'.length)).toEqual( + (order as string[])[1].slice('end-'.length), + ); + expect((order as string[])[2].slice('start-'.length)).toEqual( + (order as string[])[3].slice('end-'.length), + ); + }); + + test('Should still run the next queued execution after an earlier one rejects', async () => { + const first = executeScriptLocally( + func, + [], + stubExecuteAction, + loadModuleReturning({ + example: () => { + throw new Error('first fails'); + }, + }), + mockLogger, + ); + const second = executeScriptLocally( + func, + [], + stubExecuteAction, + loadModuleReturning({ example: () => 2 }), + mockLogger, + ); + + await expect(first).rejects.toThrow('first fails'); + await expect(second).resolves.toEqual({ data: 2 }); }); }); }); diff --git a/packages/plugins/apps/src/vite/local-execution.ts b/packages/plugins/apps/src/vite/local-execution.ts index eb978d44a..13ad5a4ec 100644 --- a/packages/plugins/apps/src/vite/local-execution.ts +++ b/packages/plugins/apps/src/vite/local-execution.ts @@ -70,6 +70,33 @@ const LOCAL_DEV_SOURCE = { runAsUser: { id: 'local-dev', orgId: 'local-dev-org' }, }; +/** + * Local backend-function executions are serialized, never run concurrently. + * `@datadog/action-catalog`'s `setExecuteActionImplementation` and + * `@datadog/apps-backend`'s `setBackend` both register runtime context via a + * shared, module-level setter — safe under production's model (a fresh Deno + * subprocess per execution), unsafe under ours (one long-lived Node process + * for every local execution). A second concurrent execution's registration + * would silently redirect the first's still-in-flight typed-import calls to + * the wrong `$.Actions`/user identity, with no error at all. See the RFC's + * Decisions and Trade-Offs for the full reasoning. + * + * Implementation: a simple promise-chain mutex. `queueTail` always resolves + * (errors are swallowed via `.catch(() => {})` before being chained) so a + * rejected execution never wedges the queue for whatever runs after it; the + * real rejection is still preserved and returned to that call's own caller. + */ +let queueTail: Promise = Promise.resolve(); + +function enqueue(run: () => Promise): Promise { + const result = queueTail.then(run); + queueTail = result.then( + () => undefined, + () => undefined, + ); + return result; +} + /** * Build the $.Actions Proxy. Resolves any nested property path (e.g. * $.Actions.slack.chat.postMessage) to a callable that invokes @@ -164,6 +191,35 @@ async function registerBackendRuntimeIfInstalled( setBackend(buildRuntimeFromJsFunctionWithActions($)); } +/** + * Backend functions eventually return through `ExecuteActionResponse`, which + * is serialized to JSON over HTTP. Catch a non-JSON-serializable result here, + * with a clear, attributed error, rather than let it surface later as an + * opaque `JSON.stringify` failure (or silently drop data) further down the + * response pipeline. Covers two distinct failure shapes: `JSON.stringify` + * throwing outright (a circular reference, a `BigInt`) and `JSON.stringify` + * silently returning `undefined` for a value that wasn't actually `undefined` + * (a bare function or `Symbol` at the top level). + */ +function assertJsonSerializable(result: unknown, func: BackendFunction): unknown { + let serialized: string | undefined; + try { + serialized = JSON.stringify(result); + } catch (err) { + throw new Error( + `Local execution of "${func.name}" returned a value that can't be serialized to JSON: ${ + err instanceof Error ? err.message : String(err) + }`, + ); + } + if (serialized === undefined && result !== undefined) { + throw new Error( + `Local execution of "${func.name}" returned a ${typeof result} value, which JSON.stringify silently drops instead of serializing — return a plain JSON-compatible value instead.`, + ); + } + return result; +} + /** * Execute a backend function in-process by importing its real file directly * — no bundling, no generated wrapper module. `globalThis.$` and the @@ -171,6 +227,8 @@ async function registerBackendRuntimeIfInstalled( * removed `main($)` wrapper used to do textually; everything else about the * call is just invoking the customer's exported function with its own real * arguments. + * + * Serialized via `enqueue` — see its own doc comment for why. */ export async function executeScriptLocally( func: BackendFunction, @@ -179,6 +237,17 @@ export async function executeScriptLocally( loadModule: LoadModule, log: Logger, timeoutMs: number = DEFAULT_TIMEOUT_MS, +): Promise { + return enqueue(() => runScriptLocally(func, args, executeAction, loadModule, log, timeoutMs)); +} + +async function runScriptLocally( + func: BackendFunction, + args: unknown[], + executeAction: ExecuteAction, + loadModule: LoadModule, + log: Logger, + timeoutMs: number, ): Promise { log.debug(`Executing "${func.name}" in-process with args=${JSON.stringify(args)}`); @@ -202,7 +271,7 @@ export async function executeScriptLocally( } const result = await fn(...args); - return { data: result }; + return { data: assertJsonSerializable(result, func) }; }; let timer: ReturnType | undefined; From 8292a6aa7374b379de910a023c564a5c21357ecc Mon Sep 17 00:00:00 2001 From: Tiffany Trinh Date: Mon, 10 Aug 2026 13:20:02 -0700 Subject: [PATCH 2/2] test(apps): prove execution queue prevents concurrent globalThis.$ race Runs the readOwnArgsAfterDelay concurrency check through the real, serialized executeScriptLocally entrypoint (its test.skip counterpart against PR #479's un-serialized base fails with cross-contaminated args). Passing here confirms the enqueue/queueTail promise-chain mutex actually closes the globalThis.$ race, not just reorders interleaved work. --- .../apps/src/vite/local-execution.test.ts | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/packages/plugins/apps/src/vite/local-execution.test.ts b/packages/plugins/apps/src/vite/local-execution.test.ts index 16b23e6d9..c3987bcea 100644 --- a/packages/plugins/apps/src/vite/local-execution.test.ts +++ b/packages/plugins/apps/src/vite/local-execution.test.ts @@ -383,6 +383,47 @@ describe('local-execution — executeScriptLocally', () => { ); }); + function readOwnArgsAfterDelay(delayMs: number): () => Promise { + return () => + new Promise((resolve) => + setTimeout( + () => resolve((globalThis as Record).$.backendFunctionArgs), + delayMs, + ), + ); + } + + // executeScriptLocally writes to the shared, mutable `globalThis.$` on + // every call. Without the `enqueue` queue, two concurrent calls both + // write it synchronously before either yields, so the second write + // wins for the whole duration of both calls and the first call's + // customer code ends up reading the second call's args — see the + // `test.skip`'d version of this same test against PR #479's + // un-serialized base, which fails with exactly that cross- + // contamination. Running it live here, through the real serialized + // `executeScriptLocally` entrypoint, proves the queue actually closes + // the gap rather than just changing the ordering of interleaved work. + test("Should let each concurrent call see its OWN backendFunctionArgs via globalThis.$, not the other call's", async () => { + const [resultA, resultB] = await Promise.all([ + executeScriptLocally( + func, + ['A-arg'], + stubExecuteAction, + loadModuleReturning({ example: readOwnArgsAfterDelay(20) }), + mockLogger, + ), + executeScriptLocally( + func, + ['B-arg'], + stubExecuteAction, + loadModuleReturning({ example: readOwnArgsAfterDelay(0) }), + mockLogger, + ), + ]); + expect(resultA).toEqual({ data: ['A-arg'] }); + expect(resultB).toEqual({ data: ['B-arg'] }); + }); + test('Should still run the next queued execution after an earlier one rejects', async () => { const first = executeScriptLocally( func,