From 65630c251948c4fc4e3b3c08626c7fb11f0b9a04 Mon Sep 17 00:00:00 2001 From: Tiffany Trinh Date: Thu, 27 Aug 2026 01:43:39 -0400 Subject: [PATCH 01/19] =?UTF-8?q?feat(apps):=20harden=20local=20execution?= =?UTF-8?q?=20=E2=80=94=20serialization,=20Source,=20edge=20cases?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Serializes concurrent executions to prevent one call's globalThis.$/registration state from leaking into another, gives each execution its own $.Source object, and closes confused-deputy and zombie-execution registration-poisoning gaps where a completed or abandoned execution could still influence a later one's action-catalog or apps-backend dispatch. Also treats .toJSON as a probed property on the $.Actions proxy so JSON.stringify($) doesn't hang. --- .../apps/src/vite/execution-epoch.test.ts | 89 ++ .../plugins/apps/src/vite/execution-epoch.ts | 49 + .../apps/src/vite/local-execution.test.ts | 869 ++++++++++++++++-- .../plugins/apps/src/vite/local-execution.ts | 302 ++++-- 4 files changed, 1199 insertions(+), 110 deletions(-) create mode 100644 packages/plugins/apps/src/vite/execution-epoch.test.ts create mode 100644 packages/plugins/apps/src/vite/execution-epoch.ts diff --git a/packages/plugins/apps/src/vite/execution-epoch.test.ts b/packages/plugins/apps/src/vite/execution-epoch.test.ts new file mode 100644 index 000000000..14935b46d --- /dev/null +++ b/packages/plugins/apps/src/vite/execution-epoch.test.ts @@ -0,0 +1,89 @@ +// Unless explicitly stated otherwise all files in this repository are licensed under the MIT License. +// This product includes software developed at Datadog (https://www.datadoghq.com/). +// Copyright 2019-Present Datadog, Inc. + +import { createEpochGuard } from '@dd/apps-plugin/vite/execution-epoch'; + +describe('execution-epoch — createEpochGuard', () => { + test('Should report a fresh scope as current and report no active scope before any start()', () => { + const guard = createEpochGuard(); + expect(guard.hasActiveScope()).toBe(false); + + const scope = guard.start(); + expect(scope.isCurrent()).toBe(true); + expect(guard.hasActiveScope()).toBe(true); + }); + + test('Should invalidate an older scope once a newer one starts', () => { + const guard = createEpochGuard(); + const older = guard.start(); + expect(older.isCurrent()).toBe(true); + + const newer = guard.start(); + expect(older.isCurrent()).toBe(false); + expect(newer.isCurrent()).toBe(true); + expect(guard.hasActiveScope()).toBe(true); + }); + + test('Should make concludeIfCurrent a no-op returning false for an already-superseded scope', () => { + const guard = createEpochGuard(); + const older = guard.start(); + guard.start(); + + expect(older.concludeIfCurrent()).toBe(false); + // The newer scope must be unaffected by the older one's no-op conclude. + expect(guard.hasActiveScope()).toBe(true); + }); + + test('Should conclude a still-current scope, clearing hasActiveScope', () => { + const guard = createEpochGuard(); + const scope = guard.start(); + + expect(scope.concludeIfCurrent()).toBe(true); + expect(scope.isCurrent()).toBe(false); + expect(guard.hasActiveScope()).toBe(false); + }); + + test('Should make a second concludeIfCurrent call on the same scope a no-op', () => { + const guard = createEpochGuard(); + const scope = guard.start(); + + expect(scope.concludeIfCurrent()).toBe(true); + expect(scope.concludeIfCurrent()).toBe(false); + }); + + test('Should invalidate the active scope and clear hasActiveScope on forceInvalidate, without starting a new one', () => { + const guard = createEpochGuard(); + const scope = guard.start(); + + guard.forceInvalidate(); + + expect(scope.isCurrent()).toBe(false); + expect(guard.hasActiveScope()).toBe(false); + }); + + test('Should make forceInvalidate followed by a fresh start() behave like an ordinary new scope', () => { + const guard = createEpochGuard(); + const abandoned = guard.start(); + guard.forceInvalidate(); + + const current = guard.start(); + + expect(abandoned.isCurrent()).toBe(false); + expect(current.isCurrent()).toBe(true); + expect(guard.hasActiveScope()).toBe(true); + + // The abandoned scope's late conclude must not corrupt the new one. + expect(abandoned.concludeIfCurrent()).toBe(false); + expect(current.isCurrent()).toBe(true); + }); + + test('Should keep independently-created guards from sharing any state', () => { + const guardA = createEpochGuard(); + const guardB = createEpochGuard(); + + const scopeA = guardA.start(); + expect(guardB.hasActiveScope()).toBe(false); + expect(scopeA.isCurrent()).toBe(true); + }); +}); diff --git a/packages/plugins/apps/src/vite/execution-epoch.ts b/packages/plugins/apps/src/vite/execution-epoch.ts new file mode 100644 index 000000000..f48762b96 --- /dev/null +++ b/packages/plugins/apps/src/vite/execution-epoch.ts @@ -0,0 +1,49 @@ +// Unless explicitly stated otherwise all files in this repository are licensed under the MIT License. +// This product includes software developed at Datadog (https://www.datadoghq.com/). +// Copyright 2019-Present Datadog, Inc. + +/** Generation-counter guard so an abandoned scope's late cleanup can't touch a shared resource a newer scope now owns (used by `network-guard.ts`, `env-guard.ts`, `local-execution.ts`). */ +export interface EpochScope { + /** True until a newer scope starts, or this one (or every scope) is concluded/invalidated. */ + isCurrent(): boolean; + /** Marks no scope active and returns true if still current, otherwise a no-op returning false — call in a `finally` to gate cleanup on still owning the resource. */ + concludeIfCurrent(): boolean; +} + +export interface EpochGuard { + /** Starts a new scope, superseding whichever one was previously active. */ + start(): EpochScope; + /** True if some started scope hasn't yet been concluded or superseded (e.g. for `network-guard.ts`'s `runAllowed`). */ + hasActiveScope(): boolean; + /** Unconditionally invalidates the active scope without starting a new one — the backstop for a scope whose own `fn` never settles. */ + forceInvalidate(): void; +} + +export function createEpochGuard(): EpochGuard { + let currentGeneration = 0; + let activeGeneration: number | null = null; + + return { + start() { + const myGeneration = ++currentGeneration; + activeGeneration = myGeneration; + return { + isCurrent: () => activeGeneration === myGeneration, + concludeIfCurrent: () => { + if (activeGeneration === myGeneration) { + activeGeneration = null; + return true; + } + return false; + }, + }; + }, + hasActiveScope() { + return activeGeneration !== null; + }, + forceInvalidate() { + currentGeneration += 1; + activeGeneration = null; + }, + }; +} diff --git a/packages/plugins/apps/src/vite/local-execution.test.ts b/packages/plugins/apps/src/vite/local-execution.test.ts index ce411dd31..4cfad6836 100644 --- a/packages/plugins/apps/src/vite/local-execution.test.ts +++ b/packages/plugins/apps/src/vite/local-execution.test.ts @@ -56,6 +56,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( @@ -131,6 +133,23 @@ describe('local-execution — executeScriptLocally', () => { expect(dollarDuringModuleLoad).toBeUndefined(); }); + test('Should reject when loadModule itself rejects, same as a native-module load failure would', async () => { + // Simulates a native addon failing to load at require()/import time, before the function is ever reached — not a customer function throwing. + const loadModule: LoadModule = async () => { + throw new Error('cannot find native module'); + }; + await expect( + executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + 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( @@ -155,20 +174,21 @@ describe('local-execution — executeScriptLocally', () => { ); }); - test('Should not hang when a customer function returns an un-invoked $.Actions reference instead of calling it', async () => { - // $.Actions.slack.chat is itself a callable Proxy; returning it without the trailing .postMessage(...) call must not make `await fn(...args)` treat it as a thenable and hang. - const result = await executeScriptLocally( - func, - TEST_PROJECT_ROOT, - [], - stubExecuteAction, - loadModuleReturning({ - example: () => testDollar().Actions.slack.chat, - }), - mockLogger, - 20, - ); - expect(result.data).toBeDefined(); + test('Should reject with a clear error, not hang, when a customer function returns an un-invoked $.Actions reference instead of calling it', async () => { + // $.Actions.slack.chat is itself a callable Proxy; forgetting the trailing .postMessage(...) call and just returning it must not make `await fn(...args)` treat it as a thenable and hang until the timeout, nor make assertJsonSerializable's JSON.stringify probe for .toJSON() leak an unhandled rejection — it should surface the same clear, synchronous "can't be serialized" error as any other bare function result. + await expect( + executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModuleReturning({ + example: () => testDollar().Actions.slack.chat, + }), + mockLogger, + 20, + ), + ).rejects.toThrow(/JSON\.stringify silently drops/); }); test('Should resolve a single-segment $.Actions.foo(...) call to a single-segment fqn', async () => { @@ -487,7 +507,24 @@ describe('local-execution — executeScriptLocally', () => { }); }); - test('Should restore a pre-existing globalThis.$ (e.g. from zx/globals) once the execution completes, not leave the execution context in place permanently', async () => { + test('Should allow a customer module to assign to globalThis.$ (e.g. importing zx/globals) without throwing', async () => { + const result = await executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModuleReturning({ + example: () => { + (globalThis as Record).$ = { notOurs: true }; + return 'done'; + }, + }), + mockLogger, + ); + expect(result).toEqual({ data: 'done' }); + }); + + test('Should restore a pre-existing globalThis.$ (e.g. from zx/globals) once the execution completes, even if the customer function reassigned it', async () => { const preExisting = { notOurs: true }; (globalThis as Record).$ = preExisting; try { @@ -497,53 +534,59 @@ describe('local-execution — executeScriptLocally', () => { [], stubExecuteAction, loadModuleReturning({ - example: () => testDollar().backendFunctionArgs, + example: () => { + (globalThis as Record).$ = { reassigned: true }; + return 'done'; + }, }), mockLogger, ); - expect(result).toEqual({ data: [] }); - // Compares via a plain boolean, not .toBe() directly — $.Actions's get trap returns a Proxy for every property, which crashes Jest's diff formatting if this assertion ever fails. + expect(result).toEqual({ data: 'done' }); expect(Object.is((globalThis as Record).$, preExisting)).toBe(true); } finally { - delete (globalThis as Record).$; + (globalThis as Record).$ = undefined; } }); - test("Should restore a pre-existing globalThis.$ even when the customer function throws, not leave the execution's context behind", async () => { - const preExisting = { notOurs: true }; - (globalThis as Record).$ = preExisting; - try { - await expect( - executeScriptLocally( - func, - TEST_PROJECT_ROOT, - [], - stubExecuteAction, - loadModuleReturning({ - example: () => { - throw new Error('customer function failed'); - }, - }), - mockLogger, - ), - ).rejects.toThrow('customer function failed'); - expect(Object.is((globalThis as Record).$, preExisting)).toBe(true); - } finally { - delete (globalThis as Record).$; - } + test('Should read globalThis.$ as undefined once the execution completes when nothing was defined before it started', async () => { + (globalThis as Record).$ = undefined; + await executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModuleReturning({ example: () => 'done' }), + mockLogger, + ); + expect((globalThis as Record).$).toBeUndefined(); }); - test('Should remove globalThis.$ once the execution completes when nothing was previously defined there', async () => { - delete (globalThis as Record).$; + test("Should not leak one execution's globalThis.$ override into a later, separately-queued execution", async () => { await executeScriptLocally( func, TEST_PROJECT_ROOT, [], stubExecuteAction, - loadModuleReturning({ example: () => 'done' }), + loadModuleReturning({ + example: () => { + (globalThis as Record).$ = { fromFirstExecution: true }; + return 'first'; + }, + }), + mockLogger, + ); + + const second = await executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModuleReturning({ + example: () => Object.keys((globalThis as Record).$).sort(), + }), mockLogger, ); - expect(Object.prototype.hasOwnProperty.call(globalThis, '$')).toBe(false); + expect(second).toEqual({ data: ['Actions', 'Source', 'backendFunctionArgs'] }); }); describe('action-catalog / apps-backend registration', () => { @@ -587,6 +630,55 @@ describe('local-execution — executeScriptLocally', () => { ).rejects.toThrow('Unexpected token in action-catalog/action-execution'); }); + // A sibling registration genuinely failing doesn't affect the action-catalog adapter — it's stable and execution-agnostic, so a call made once no execution is active correctly rejects on its own, with no special-case coordination needed between the two registrations. + test('Should still reject a typed-wrapper call through a successfully-registered action-catalog implementation after the sibling apps-backend registration genuinely fails and the execution concludes', async () => { + jest.spyOn(shared, 'isActionCatalogInstalled').mockReturnValue(true); + jest.spyOn(shared, 'isDatadogAppsBackendInstalled').mockReturnValue(true); + let registeredImpl: + | ((actionId: string, request: unknown) => Promise) + | undefined; + + const loadModule: LoadModule = async (specifier: string) => { + if (specifier === func.absolutePath + LOCAL_EXECUTION_LOAD_SUFFIX) { + return { example: () => 'unreachable' }; + } + if (specifier === '@datadog/action-catalog/action-execution') { + return { + setExecuteActionImplementation: ( + impl: (actionId: string, request: unknown) => Promise, + ) => { + registeredImpl = impl; + }, + }; + } + if (specifier === '@datadog/apps-backend/runtime/jsFunctionWithActions') { + // A real transform/evaluation failure, not module-not-found — must not be swallowed as "package isn't installed". + throw new Error( + 'Unexpected token in apps-backend/runtime/jsFunctionWithActions', + ); + } + const error: NodeJS.ErrnoException = new Error(`Cannot find module '${specifier}'`); + error.code = 'MODULE_NOT_FOUND'; + throw error; + }; + + await expect( + executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModule, + mockLogger, + ), + ).rejects.toThrow('Unexpected token in apps-backend/runtime/jsFunctionWithActions'); + + expect(registeredImpl).toBeDefined(); + await expect( + registeredImpl?.('com.datadoghq.slack.chat.postMessage', { inputs: {} }), + ).rejects.toThrow(/no active local execution/i); + }); + test('Should route an action-catalog typed-wrapper call through the same injected executeAction', async () => { jest.spyOn(shared, 'isActionCatalogInstalled').mockReturnValue(true); const executeAction = jest.fn().mockResolvedValue({ ok: true }); @@ -720,21 +812,185 @@ describe('local-execution — executeScriptLocally', () => { ).rejects.toThrow(/must have an inputs field/); expect(executeAction).not.toHaveBeenCalled(); }); + + // Mirrors the action-catalog abandonment test — apps-backend's setBackend has the same shared-module-level-setter hazard. + test("Should reject an abandoned execution's apps-backend accessor call once concluded", async () => { + jest.spyOn(shared, 'isDatadogAppsBackendInstalled').mockReturnValue(true); + let abandonedCallOutcome: 'pending' | 'resolved' | { rejected: string } = 'pending'; + let registeredBackend: { get: () => unknown } | undefined; + + const loadModule: LoadModule = async (specifier: string) => { + if (specifier === func.absolutePath + LOCAL_EXECUTION_LOAD_SUFFIX) { + return { + example: async () => { + await new Promise((resolve) => setTimeout(resolve, 100)); + try { + registeredBackend?.get(); + abandonedCallOutcome = 'resolved'; + } catch (err) { + abandonedCallOutcome = { + rejected: err instanceof Error ? err.message : String(err), + }; + } + return { data: 'abandoned' }; + }, + }; + } + if (specifier === '@datadog/apps-backend/runtime/jsFunctionWithActions') { + return { + // Mirrors the real package's synchronous $.Source validation, so a poisoned proxy passed through here fails the same way. + buildRuntimeFromJsFunctionWithActions: ($: unknown) => { + const source = ($ as Record).Source as + | { initiator?: unknown } + | undefined; + if (!source || typeof source.initiator !== 'object') { + throw new Error( + 'Invalid $.Source supplied to buildRuntimeFromJsFunctionWithActions', + ); + } + return { get: () => source }; + }, + }; + } + if (specifier === '@datadog/apps-backend/runtime') { + return { + setBackend: (runtime: { get: () => unknown }) => { + registeredBackend = runtime; + }, + }; + } + const notFoundError: NodeJS.ErrnoException = new Error( + `Cannot find module '${specifier}'`, + ); + notFoundError.code = 'MODULE_NOT_FOUND'; + throw notFoundError; + }; + + const abandoned = executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModule, + mockLogger, + 20, + ); + await expect(abandoned).rejects.toThrow(/timed out after 20ms/); + + await new Promise((resolve) => setTimeout(resolve, 100)); + + expect(abandonedCallOutcome).toEqual({ + rejected: expect.stringContaining('already concluded'), + }); + }); + }); + + describe('non-serializable results', () => { + test('Should reject with a clear, attributed error when the result has a circular reference', async () => { + await expect( + executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + 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, + TEST_PROJECT_ROOT, + [], + 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, + TEST_PROJECT_ROOT, + [], + 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, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModuleReturning({ example: () => undefined }), + mockLogger, + ); + expect(result).toEqual({ data: undefined }); + }); + + // dev-server.ts serializes the result again for the HTTP response — returning the original (not the parsed round-trip) would invoke a custom toJSON() twice. + test('Should return the JSON-round-tripped value, not the original, so a custom toJSON() is only invoked once', async () => { + let toJsonCallCount = 0; + const result = await executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModuleReturning({ + example: () => ({ + toJSON() { + toJsonCallCount += 1; + return { callNumber: toJsonCallCount }; + }, + }), + }), + mockLogger, + ); + expect(result).toEqual({ data: { callNumber: 1 } }); + expect(toJsonCallCount).toBe(1); + }); }); 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, TEST_PROJECT_ROOT, [], stubExecuteAction, - loadModuleReturning({ example: delayedResult('A', 20) }), + loadModuleReturning({ example: recordingOrder('A', 20) }), mockLogger, ), executeScriptLocally( @@ -742,23 +998,40 @@ describe('local-execution — executeScriptLocally', () => { TEST_PROJECT_ROOT, [], 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 runs first, its start/end pair must be adjacent — a real race would interleave as [start-A, start-B, end-B, end-A]. + 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), + ); }); - // Reads $.backendFunctionArgs after a delay, which is what would surface cross-contamination between concurrent calls' globalThis.$. function readOwnArgsAfterDelay(delayMs: number): () => Promise { return () => new Promise((resolve) => - setTimeout(() => resolve(testDollar().backendFunctionArgs), delayMs), + setTimeout( + () => resolve((globalThis as Record).$.backendFunctionArgs), + delayMs, + ), ); } - // Known race: two concurrent calls both write globalThis.$ synchronously, so the second write wins for both — skip until calls are serialized through an execution queue. - test.skip("Should let each concurrent call see its OWN backendFunctionArgs via globalThis.$, not the other call's", async () => { + // globalThis.$ is scoped per call via AsyncLocalStorage, independent of the enqueue queue (which exists for the action-catalog/apps-backend module-singleton race). + 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, @@ -780,5 +1053,493 @@ describe('local-execution — executeScriptLocally', () => { 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, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModuleReturning({ + example: () => { + throw new Error('first fails'); + }, + }), + mockLogger, + ); + const second = executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModuleReturning({ example: () => 2 }), + mockLogger, + ); + + await expect(first).rejects.toThrow('first fails'); + await expect(second).resolves.toEqual({ data: 2 }); + }); + + // Covers the raw-$.Actions path: a captured Actions reference (e.g. const { Actions } = $) must reject once its own execution is abandoned, even after globalThis.$ is overwritten by a newer execution. + test('Should reject a captured $.Actions reference once its own execution is abandoned, even after a newer execution has taken over', async () => { + let abandonedCallOutcome: 'pending' | 'resolved' | { rejected: string } = 'pending'; + + const abandoned = executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModuleReturning({ + example: async () => { + // Captured BEFORE the timeout fires — this execution's own Actions proxy, not whatever globalThis.$ points to later. + const { Actions } = (globalThis as Record).$; + // Outlives the 20ms timeout below, so the caller already sees a rejection by the time this line runs. + await new Promise((resolve) => setTimeout(resolve, 100)); + try { + await Actions.foo.bar({ inputs: {} }); + abandonedCallOutcome = 'resolved'; + } catch (err) { + abandonedCallOutcome = { + rejected: err instanceof Error ? err.message : String(err), + }; + } + return { data: 'abandoned' }; + }, + }), + mockLogger, + 20, + ); + await expect(abandoned).rejects.toThrow(/timed out after 20ms/); + + // The queue is free as soon as the timeout wins — the second execution starts and completes normally, becoming "current". + const second = await executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModuleReturning({ example: () => 'second' }), + mockLogger, + ); + expect(second).toEqual({ data: 'second' }); + + // Give the abandoned execution's background timer room to fire its action call before asserting on the outcome. + await new Promise((resolve) => setTimeout(resolve, 100)); + + expect(abandonedCallOutcome).toEqual({ + rejected: expect.stringContaining('already concluded'), + }); + }); + + test("Should resolve a zombie execution's FRESH read of globalThis.$ to its OWN identity, never a newer execution's — even while that newer execution is still in flight", async () => { + const funcA: BackendFunction = { ...func, allowedConnectionIds: ['conn-A'] }; + const funcB: BackendFunction = { ...func, allowedConnectionIds: ['conn-B'] }; + const executeAction = jest.fn().mockResolvedValue({ ok: true }); + + let zombieOutcome: 'pending' | 'resolved' | { rejected: string } = 'pending'; + + const abandoned = executeScriptLocally( + funcA, + TEST_PROJECT_ROOT, + [], + executeAction, + loadModuleReturning({ + example: async () => { + // Fires ~60ms in, squarely inside funcB's in-flight window — a fresh $ read here needs AsyncLocalStorage, not the abandoned closure check, or it would resolve to funcB's $. + await new Promise((resolve) => setTimeout(resolve, 60)); + const $ = (globalThis as Record).$; + try { + // funcB's own connectionId, not funcA's — only valid if this call incorrectly runs under funcB's still-live identity. + await $.Actions.foo.bar({ inputs: {}, connectionId: 'conn-B' }); + zombieOutcome = 'resolved'; + } catch (err) { + zombieOutcome = { + rejected: err instanceof Error ? err.message : String(err), + }; + } + return 'zombie-done'; + }, + }), + mockLogger, + 20, + ); + await expect(abandoned).rejects.toThrow(/timed out after 20ms/); + + // Starts as soon as the queue frees and stays "current" for 80ms, overlapping the zombie's 60ms wakeup; never itself calls $.Actions, so any observed call must be the zombie's. + const second = executeScriptLocally( + funcB, + TEST_PROJECT_ROOT, + [], + executeAction, + loadModuleReturning({ + example: async () => { + await new Promise((resolve) => setTimeout(resolve, 80)); + return 'second'; + }, + }), + mockLogger, + ); + await expect(second).resolves.toEqual({ data: 'second' }); + + // The zombie's fresh read resolved to its OWN $ (funcA's allowedConnectionIds) — funcB's connectionId under funcA's identity is rejected before reaching executeAction. + expect(zombieOutcome).toEqual({ + rejected: expect.stringContaining("not in this function's allowed connections"), + }); + expect(executeAction).not.toHaveBeenCalled(); + }); + + // Action-catalog holds one executeAction implementation in shared module state — a per-closure abandoned guard can't protect a typed-wrapper call once a newer execution re-registers, so poisonActionCatalogRegistration proactively replaces it with a rejecting stub on conclusion. + test("Should reject an abandoned execution's action-catalog typed-wrapper call, not silently run it under a newer registration", async () => { + jest.spyOn(shared, 'isActionCatalogInstalled').mockReturnValue(true); + let abandonedCallOutcome: 'pending' | 'resolved' | { rejected: string } = 'pending'; + let registeredImpl: + | ((actionId: string, request: unknown) => Promise) + | undefined; + + const loadModule: LoadModule = async (specifier: string) => { + if (specifier === func.absolutePath + LOCAL_EXECUTION_LOAD_SUFFIX) { + return { + example: async () => { + await new Promise((resolve) => setTimeout(resolve, 100)); + try { + await registeredImpl?.('com.datadoghq.foo.bar', { inputs: {} }); + abandonedCallOutcome = 'resolved'; + } catch (err) { + abandonedCallOutcome = { + rejected: err instanceof Error ? err.message : String(err), + }; + } + return { data: 'abandoned' }; + }, + }; + } + if (specifier === '@datadog/action-catalog/action-execution') { + return { + setExecuteActionImplementation: ( + impl: (actionId: string, request: unknown) => Promise, + ) => { + registeredImpl = impl; + }, + }; + } + const notFoundError: NodeJS.ErrnoException = new Error( + `Cannot find module '${specifier}'`, + ); + notFoundError.code = 'MODULE_NOT_FOUND'; + throw notFoundError; + }; + + const abandoned = executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModule, + mockLogger, + 20, + ); + await expect(abandoned).rejects.toThrow(/timed out after 20ms/); + + // registeredImpl now points at the abandoned execution's own implementation, poisoned by the timeout handler — deliberately no second execution here, to isolate the poison step. + await new Promise((resolve) => setTimeout(resolve, 100)); + + expect(abandonedCallOutcome).toEqual({ + rejected: expect.stringContaining('already concluded'), + }); + }); + + // Poisoning only protects the window before a newer execution registers — once it does, its own register() call (correctly, from its own perspective) overwrites the poison stub. A zombie action-catalog call made after that point must still be rejected, not routed through the newer execution's identity/allowedConnectionIds. + test("Should reject a zombie execution's action-catalog typed-wrapper call even after a newer execution has legitimately re-registered its own implementation", async () => { + jest.spyOn(shared, 'isActionCatalogInstalled').mockReturnValue(true); + const funcA: BackendFunction = { ...func, allowedConnectionIds: ['conn-A'] }; + const funcB: BackendFunction = { ...func, allowedConnectionIds: ['conn-B'] }; + const executeAction = jest.fn().mockResolvedValue({ ok: true }); + let registeredImpl: + | ((actionId: string, request: unknown) => Promise) + | undefined; + let zombieOutcome: 'pending' | 'resolved' | { rejected: string } = 'pending'; + + const makeLoadModule = (exampleImpl: () => Promise): LoadModule => { + return async (specifier: string) => { + if (specifier === func.absolutePath + LOCAL_EXECUTION_LOAD_SUFFIX) { + return { example: exampleImpl }; + } + if (specifier === '@datadog/action-catalog/action-execution') { + return { + setExecuteActionImplementation: ( + impl: (actionId: string, request: unknown) => Promise, + ) => { + registeredImpl = impl; + }, + }; + } + const notFoundError: NodeJS.ErrnoException = new Error( + `Cannot find module '${specifier}'`, + ); + notFoundError.code = 'MODULE_NOT_FOUND'; + throw notFoundError; + }; + }; + + // Times out at 20ms, then calls the typed wrapper ~60ms in — squarely inside funcB's own in-flight window (funcB registers immediately but doesn't complete, and self-poison, until 80ms) — using conn-B, a connection funcA itself is never allowed to use. + const abandoned = executeScriptLocally( + funcA, + TEST_PROJECT_ROOT, + [], + executeAction, + makeLoadModule(async () => { + await new Promise((resolve) => setTimeout(resolve, 60)); + try { + await registeredImpl?.('com.datadoghq.foo.bar', { + inputs: {}, + connectionId: 'conn-B', + }); + zombieOutcome = 'resolved'; + } catch (err) { + zombieOutcome = { + rejected: err instanceof Error ? err.message : String(err), + }; + } + return 'zombie-done'; + }), + mockLogger, + 20, + ); + await expect(abandoned).rejects.toThrow(/timed out after 20ms/); + + // Starts as soon as the queue frees, registers immediately, but doesn't complete (and self-poison on conclusion) until 80ms — overlapping funcA's 60ms zombie wakeup. + const second = executeScriptLocally( + funcB, + TEST_PROJECT_ROOT, + [], + executeAction, + makeLoadModule(() => new Promise((resolve) => setTimeout(() => resolve('B'), 80))), + mockLogger, + ); + await expect(second).resolves.toEqual({ data: 'B' }); + + // funcB's own registration checks conn-B against funcB's allowedConnectionIds, which passes — the zombie call must not be allowed to reach that registration at all. + expect(zombieOutcome).toEqual({ + rejected: expect.stringContaining('already concluded'), + }); + expect(executeAction).not.toHaveBeenCalled(); + }); + + // The apps-backend loadModule call hangs forever here — a post-Promise.all destructuring assignment would never run, so publishing each handle via .then() is what lets the completed action-catalog registration still get poisoned. + test('Should still register the action-catalog adapter even when the sibling apps-backend registration never settles, and reject a call once no execution is active', async () => { + jest.spyOn(shared, 'isActionCatalogInstalled').mockReturnValue(true); + jest.spyOn(shared, 'isDatadogAppsBackendInstalled').mockReturnValue(true); + let registeredImpl: + | ((actionId: string, request: unknown) => Promise) + | undefined; + + const loadModule: LoadModule = async (specifier: string) => { + if (specifier === func.absolutePath + LOCAL_EXECUTION_LOAD_SUFFIX) { + return { example: () => 'unused' }; + } + if (specifier === '@datadog/action-catalog/action-execution') { + return { + setExecuteActionImplementation: ( + impl: (actionId: string, request: unknown) => Promise, + ) => { + registeredImpl = impl; + }, + }; + } + if ( + specifier === '@datadog/apps-backend/runtime/jsFunctionWithActions' || + specifier === '@datadog/apps-backend/runtime' + ) { + return new Promise(() => {}); + } + const notFoundError: NodeJS.ErrnoException = new Error( + `Cannot find module '${specifier}'`, + ); + notFoundError.code = 'MODULE_NOT_FOUND'; + throw notFoundError; + }; + + const abandoned = executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModule, + mockLogger, + 20, + ); + await expect(abandoned).rejects.toThrow(/timed out after 20ms/); + + expect(registeredImpl).toBeDefined(); + await expect(registeredImpl?.('com.datadoghq.foo.bar', { inputs: {} })).rejects.toThrow( + /no active local execution/i, + ); + }); + + // An abandoned execution's fn() can settle normally later — its finally block must not re-poison the registration over whatever a newer execution already put there. + test("Should not let a late-settling abandoned execution's own conclusion clobber a newer execution's already-registered action-catalog implementation", async () => { + jest.spyOn(shared, 'isActionCatalogInstalled').mockReturnValue(true); + let registeredImpl: + | ((actionId: string, request: unknown) => Promise) + | undefined; + + const makeLoadModule = (exampleImpl: () => Promise): LoadModule => { + return async (specifier: string) => { + if (specifier === func.absolutePath + LOCAL_EXECUTION_LOAD_SUFFIX) { + return { example: exampleImpl }; + } + if (specifier === '@datadog/action-catalog/action-execution') { + return { + setExecuteActionImplementation: ( + impl: (actionId: string, request: unknown) => Promise, + ) => { + registeredImpl = impl; + }, + }; + } + const notFoundError: NodeJS.ErrnoException = new Error( + `Cannot find module '${specifier}'`, + ); + notFoundError.code = 'MODULE_NOT_FOUND'; + throw notFoundError; + }; + }; + + // Times out at 20ms, but its own fn() resolves normally ~100ms later, well after being abandoned. + const abandoned = executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + makeLoadModule( + () => new Promise((resolve) => setTimeout(() => resolve('A-late'), 100)), + ), + mockLogger, + 20, + ); + await expect(abandoned).rejects.toThrow(/timed out after 20ms/); + + // The queue is free as soon as the timeout wins — the second execution registers and finishes well before the abandoned one's 100ms sleep is up. + const second = await executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + makeLoadModule(() => Promise.resolve('B')), + mockLogger, + ); + expect(second).toEqual({ data: 'B' }); + + // Captures whatever B's own conclusion left registered — B poisoning its own registration on completion is fine; nothing else must overwrite it. + const registeredAfterB = registeredImpl; + + // Give the abandoned execution's late-settling fn() and its finally block room to run. + await new Promise((resolve) => setTimeout(resolve, 100)); + + expect(registeredImpl).toBe(registeredAfterB); + }); + + // A's slow-to-resolve registration re-installs the same stable, execution-agnostic dispatcher B's own registration already put in place — replacing the closure instance is harmless, since either one resolves a call against whichever execution is actually on the AsyncLocalStorage-scoped call stack, not against whichever registered it. + test("Should still dispatch correctly after a stale execution's slow-to-resolve registration re-installs the adapter following a newer execution's own registration", async () => { + jest.spyOn(shared, 'isActionCatalogInstalled').mockReturnValue(true); + let registeredImpl: + | ((actionId: string, request: unknown) => Promise) + | undefined; + + const makeLoadModule = (actionCatalogDelayMs: number): LoadModule => { + return async (specifier: string) => { + if (specifier === func.absolutePath + LOCAL_EXECUTION_LOAD_SUFFIX) { + return { example: () => 'result' }; + } + if (specifier === '@datadog/action-catalog/action-execution') { + if (actionCatalogDelayMs > 0) { + await new Promise((resolve) => + setTimeout(resolve, actionCatalogDelayMs), + ); + } + return { + setExecuteActionImplementation: ( + impl: (actionId: string, request: unknown) => Promise, + ) => { + registeredImpl = impl; + }, + }; + } + const notFoundError: NodeJS.ErrnoException = new Error( + `Cannot find module '${specifier}'`, + ); + notFoundError.code = 'MODULE_NOT_FOUND'; + throw notFoundError; + }; + }; + + // Times out at 20ms, well before its own 100ms-delayed action-catalog module load resolves. + const abandoned = executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + makeLoadModule(100), + mockLogger, + 20, + ); + await expect(abandoned).rejects.toThrow(/timed out after 20ms/); + + // The queue is free as soon as the timeout wins — the second execution registers with no artificial delay, well before A's slow load resolves. + const second = await executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + makeLoadModule(0), + mockLogger, + ); + expect(second).toEqual({ data: 'result' }); + + // Give A's slow action-catalog load room to finally resolve and re-install the adapter. + await new Promise((resolve) => setTimeout(resolve, 150)); + + // No execution is active at this point — either closure instance correctly rejects the same way. + await expect(registeredImpl?.('com.datadoghq.foo.bar', { inputs: {} })).rejects.toThrow( + /no active local execution/i, + ); + }); + + // An abandoned execution's loadModule/registration steps might still resolve after timeout — proves the customer function is never invoked once already known-stale. + test('Should never invoke the customer function once already known to be abandoned before it starts', async () => { + let callCount = 0; + const slowLoadModule: LoadModule = async (specifier: string) => { + if (specifier === func.absolutePath + LOCAL_EXECUTION_LOAD_SUFFIX) { + // Slower than the 20ms timeout below — by the time this resolves, the execution is already known-abandoned. + await new Promise((resolve) => setTimeout(resolve, 100)); + return { + example: () => { + callCount += 1; + return 'should never run'; + }, + }; + } + const notFoundError: NodeJS.ErrnoException = new Error( + `Cannot find module '${specifier}'`, + ); + notFoundError.code = 'MODULE_NOT_FOUND'; + throw notFoundError; + }; + + const abandoned = executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + slowLoadModule, + mockLogger, + 20, + ); + await expect(abandoned).rejects.toThrow(/timed out after 20ms/); + + // Give the slow loadModule call room to actually resolve. + await new Promise((resolve) => setTimeout(resolve, 100)); + + expect(callCount).toBe(0); + }); }); }); diff --git a/packages/plugins/apps/src/vite/local-execution.ts b/packages/plugins/apps/src/vite/local-execution.ts index 4e315b08e..babeb3bac 100644 --- a/packages/plugins/apps/src/vite/local-execution.ts +++ b/packages/plugins/apps/src/vite/local-execution.ts @@ -7,11 +7,58 @@ /** Executes a backend function's file directly in-process inside the Vite dev server, mirroring executeScriptViaDatadog's `BackendOutputs` contract in dev-server.ts as a drop-in alternate implementation. */ import type { Logger } from '@dd/core/types'; +import { AsyncLocalStorage } from 'node:async_hooks'; import { isActionCatalogInstalled, isDatadogAppsBackendInstalled } from '../backend/shared'; import type { BackendFunction, BackendOutputs } from '../backend/types'; import { LOCAL_EXECUTION_LOAD_SUFFIX } from '../constants'; +import { createEpochGuard } from './execution-epoch'; + +type BackendGlobals = { + backendFunctionArgs: unknown[]; + Actions: unknown; + Source: ReturnType; +}; + +/** Boxed so a customer module assigning to `globalThis.$` (e.g. importing `zx/globals`, which does exactly this) mutates only its own execution's box, never a concurrent or zombie execution's. */ +type BackendGlobalsBox = { value: unknown }; + +/** Scopes `globalThis.$` per execution via AsyncLocalStorage, not a plain mutable property, so a zombie execution's late "fresh" `globalThis.$` read resolves to its own `$`, never a newer execution's identity/`allowedConnectionIds`. */ +const backendGlobalsContext = new AsyncLocalStorage(); + +/** Backs `globalThis.$` for reads/writes that happen with no execution box on the AsyncLocalStorage-scoped call stack (e.g. this module's own import-time state) — an ordinary mutable slot, since there's no per-execution box to isolate it into. */ +let globalDollarOutsideExecution: unknown; + +Object.defineProperty(globalThis, '$', { + configurable: true, + enumerable: true, + get: () => { + const box = backendGlobalsContext.getStore(); + return box ? box.value : globalDollarOutsideExecution; + }, + set: (value: unknown) => { + const box = backendGlobalsContext.getStore(); + if (box) { + box.value = value; + } else { + globalDollarOutsideExecution = value; + } + }, +}); + +/** What the stable, once-ever-registered action-catalog/apps-backend adapters (below) need to dispatch a typed-wrapper call to the execution that's actually on the AsyncLocalStorage-scoped call stack — kept out of `BackendGlobals` since that object is also `globalThis.$`, directly visible to customer code. */ +type ExecutionDispatch = { + executeAction: ExecuteAction; + allowedConnectionIds: string[]; + isAbandoned: () => boolean; + functionName: string; + $: BackendGlobals; +}; + +/** Distinct from `backendGlobalsContext` so dispatch-only fields (the real `executeAction`, `allowedConnectionIds`) never leak onto `globalThis.$`. */ +const executionDispatchContext = new AsyncLocalStorage(); + interface ActionCallArgs { inputs: Record; connectionId?: string; @@ -22,20 +69,6 @@ function isIndexableRecord(value: unknown): value is Record { return typeof value === 'object' && value !== null; } -/** `globalThis.$` is a runtime-only property `typeof globalThis` doesn't know about; `Reflect.get` reads it without a type assertion, like `deleteGlobalDollar` does for deletion. */ -function getGlobalDollar(): unknown { - return Reflect.get(globalThis, '$'); -} - -/** `Object.assign`'s signature doesn't require its source object's keys to already exist on the target, so this installs `$` without asserting `globalThis`'s type. */ -function setGlobalDollar(value: unknown): void { - Object.assign(globalThis, { $: value }); -} - -function deleteGlobalDollar(): void { - Reflect.deleteProperty(globalThis, '$'); -} - const DEFAULT_TIMEOUT_MS = 10_000; /** Loads a module by specifier, resolved against the customer's own project rather than build-plugins' dependency tree — the dev server passes its Vite instance's `ssrLoadModule` here. */ @@ -83,6 +116,21 @@ function validateActionCall( return { inputs, connectionId }; } +/** Local executions are serialized since action-catalog/apps-backend register runtime context via a shared, module-level setter a concurrent execution would clobber, silently redirecting the first's in-flight calls to the wrong identity. */ +let queueTail: Promise = Promise.resolve(); + +function enqueue(run: () => Promise): Promise { + const result = queueTail.then(run); + queueTail = result.then( + () => undefined, + () => undefined, + ); + return result; +} + +/** One shared guard across all local executions — `enqueue` already serializes them, so starting a new scope always supersedes the previous one only after it has already concluded, but the guard's own generation counter is a belt-and-suspenders backstop if that invariant is ever violated. */ +const executionEpoch = createEpochGuard(); + /** Resolves a nested property path (e.g. $.Actions.slack.chat.postMessage) to a callable that invokes `executeAction` directly — no IPC needed since there's no separate process to cross. */ function makeActionsProxy( executeAction: ExecuteAction, @@ -91,8 +139,8 @@ function makeActionsProxy( ): unknown { return new Proxy(function () {}, { get(_target, prop) { - // An un-invoked reference (e.g. $.Actions.foo.bar with no call) must not look like a thenable, or Promise's resolution protocol calls .then() on it and hangs until timeout. - if (prop === 'then') { + // A customer function that returns an un-invoked reference (e.g. $.Actions.foo.bar without the trailing call) must not be mistaken for a thenable or a custom-serializable object — Promise's resolution protocol probes .then(), and JSON.stringify (assertJsonSerializable) probes .toJSON(); either probe calling into the async apply() below would hang until timeout or leak an unhandled rejection instead of surfacing assertJsonSerializable's clear "can't be serialized" error. + if (prop === 'then' || prop === 'toJSON') { return undefined; } const nestedPathParts = pathParts.concat(String(prop)); @@ -114,12 +162,29 @@ function makeActionsProxy( }); } -/** No-ops if @datadog/action-catalog isn't installed; checks `isActionCatalogInstalled` up front rather than catching a load failure, since `loadModule` doesn't guarantee an error code for a missing bare specifier. */ -async function registerActionCatalogIfInstalled( +/** Keyed by `loadModule` identity, not a bare module-level flag — a real dev server reuses the same Vite `ssrLoadModule` for its whole lifetime (giving true once-ever registration), while each test constructs its own `loadModule` closure (keeping tests isolated from each other's registration state). A rejection is evicted so the next execution retries, rather than permanently poisoning every later execution with one transient load failure. */ +const actionCatalogRegistrations = new WeakMap>(); + +/** No-ops if @datadog/action-catalog isn't installed. Registers ONE stable dispatcher for the process lifetime — it reads `executionDispatchContext.getStore()` at call time to resolve whichever execution is actually on the AsyncLocalStorage-scoped call stack, so a zombie execution's typed-wrapper call can never be routed through a newer execution's identity/allowedConnectionIds just because that execution's own registration is the one currently live. */ +function registerActionCatalogIfInstalled( + loadModule: LoadModule, + projectRoot: string, +): Promise { + const existing = actionCatalogRegistrations.get(loadModule); + if (existing) { + return existing; + } + const registration = registerActionCatalogOnce(loadModule, projectRoot).catch((err) => { + actionCatalogRegistrations.delete(loadModule); + throw err; + }); + actionCatalogRegistrations.set(loadModule, registration); + return registration; +} + +async function registerActionCatalogOnce( loadModule: LoadModule, projectRoot: string, - executeAction: ExecuteAction, - allowedConnectionIds: string[], ): Promise { if (!isActionCatalogInstalled(projectRoot)) { return; @@ -130,21 +195,49 @@ async function registerActionCatalogIfInstalled( return; } setExecuteActionImplementation(async (actionId: string, request: unknown) => { + const dispatch = executionDispatchContext.getStore(); + if (!dispatch) { + throw new Error(`No active local execution to run "${actionId}" under.`); + } + if (dispatch.isAbandoned()) { + throw new Error( + `Execution of "${dispatch.functionName}" already concluded; refusing to run ` + + `"${actionId}" as this stale execution to avoid using a newer execution's identity.`, + ); + } const call: Partial = isIndexableRecord(request) ? request : {}; const { inputs, connectionId } = validateActionCall( call, - allowedConnectionIds, + dispatch.allowedConnectionIds, `"${actionId}"`, ); - return executeAction(actionId, inputs, connectionId); + return dispatch.executeAction(actionId, inputs, connectionId); + }); +} + +/** Mirrors `actionCatalogRegistrations` — see its doc comment for why keying on `loadModule` identity is safe across both real dev-server reuse and per-test isolation. */ +const backendRuntimeRegistrations = new WeakMap>(); + +/** No-ops if @datadog/apps-backend isn't installed. Registers ONE stable runtime Proxy for the process lifetime — every accessor call resolves whichever execution's `$` is on the AsyncLocalStorage-scoped call stack (or rejects if that execution has concluded), rather than a runtime bound to a specific execution's `$` at registration time. */ +function registerBackendRuntimeIfInstalled( + loadModule: LoadModule, + projectRoot: string, +): Promise { + const existing = backendRuntimeRegistrations.get(loadModule); + if (existing) { + return existing; + } + const registration = registerBackendRuntimeOnce(loadModule, projectRoot).catch((err) => { + backendRuntimeRegistrations.delete(loadModule); + throw err; }); + backendRuntimeRegistrations.set(loadModule, registration); + return registration; } -/** No-ops if @datadog/apps-backend isn't installed; see `registerActionCatalogIfInstalled` for why this checks installedness up front rather than catching a load failure. */ -async function registerBackendRuntimeIfInstalled( +async function registerBackendRuntimeOnce( loadModule: LoadModule, projectRoot: string, - $: unknown, ): Promise { if (!isDatadogAppsBackendInstalled(projectRoot)) { return; @@ -162,11 +255,63 @@ async function registerBackendRuntimeIfInstalled( ) { return; } - const backendRuntime = buildRuntimeFromJsFunctionWithActions($); - setBackend(backendRuntime); + // Built once per execution (cached by dispatch identity), not once per accessor call — dispatch.$ is fixed for its whole execution, so rebuilding on every property access wasted work without changing the result. + const runtimeByDispatch = new WeakMap(); + // Every property access returns a callable, not a value — the real package calls specific methods (e.g. getInitiatingUser()), not just reads properties. + const backendRuntimeProxy = new Proxy( + {}, + { + get(_target, prop) { + return (...args: unknown[]) => { + const dispatch = executionDispatchContext.getStore(); + if (!dispatch || dispatch.isAbandoned()) { + throw new Error( + `Execution of "${dispatch?.functionName ?? 'unknown'}" already concluded; ` + + `refusing to resolve a further apps-backend accessor under its identity.`, + ); + } + let runtime = runtimeByDispatch.get(dispatch); + if (runtime === undefined) { + runtime = buildRuntimeFromJsFunctionWithActions(dispatch.$); + runtimeByDispatch.set(dispatch, runtime); + } + const method = isIndexableRecord(runtime) ? runtime[String(prop)] : undefined; + if (typeof method !== 'function') { + throw new Error(`apps-backend runtime has no method "${String(prop)}"`); + } + return method.apply(runtime, args); + }; + }, + }, + ); + setBackend(backendRuntimeProxy); } -/** `globalThis.$` and the registrations above provide the same customer-visible bindings production's generated wrapper module sets up via text injection. */ +/** Rejects a non-JSON-serializable result (circular reference/`BigInt`, or a bare function/`Symbol` that `JSON.stringify` silently drops) here with a clear error, instead of failing downstream when serialized for the HTTP response. */ +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) { + if (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 undefined; + } + // Return the parsed-and-reserialized value, not the original — the caller serializes again for the HTTP response, and the original would invoke a custom toJSON() a second time. + return JSON.parse(serialized); +} + +/** `globalThis.$` and the action-catalog/apps-backend registrations above provide the same customer-visible bindings production's generated wrapper module sets up via text injection; serialized via `enqueue`. */ export async function executeScriptLocally( func: BackendFunction, projectRoot: string, @@ -175,16 +320,58 @@ export async function executeScriptLocally( loadModule: LoadModule, log: Logger, timeoutMs: number = DEFAULT_TIMEOUT_MS, +): Promise { + return enqueue(() => + runScriptLocally(func, projectRoot, args, executeAction, loadModule, log, timeoutMs), + ); +} + +async function runScriptLocally( + func: BackendFunction, + projectRoot: string, + args: unknown[], + executeAction: ExecuteAction, + loadModule: LoadModule, + log: Logger, + timeoutMs: number, ): Promise { // Never log the args themselves — they may carry secrets/PII, matching dev-server.ts's cloud path. log.debug(`Executing "${func.name}" in-process with args`); + // A timed-out execution is abandoned, not cancelled — its fn() may keep running and must not act under a newer execution's identity. The scope's isCurrent() is checked both directly (this execution's own captured `$.Actions` closure) and via `executionDispatchContext` (the stable, shared action-catalog/apps-backend adapters resolve the CALLING execution's own dispatch info from AsyncLocalStorage at call time, so a zombie's call can never be serviced by whichever execution's registration happens to be live). + const scope = executionEpoch.start(); + + const guardedExecuteAction: ExecuteAction = (fqn, inputs, connectionId) => { + if (!scope.isCurrent()) { + // A concluded execution's scope stays concluded forever, not just "not the latest," so the wording stays conclusion-neutral rather than claiming a timeout that may not have happened. + return Promise.reject( + new Error( + `Execution of "${func.name}" already concluded; refusing to run ` + + `"${fqn}" as this stale execution to avoid using a newer execution's identity.`, + ), + ); + } + return executeAction(fqn, inputs, connectionId); + }; + + const concludeExecution = () => { + scope.concludeIfCurrent(); + }; + const $ = { backendFunctionArgs: args, - Actions: makeActionsProxy(executeAction, func.allowedConnectionIds), + Actions: makeActionsProxy(guardedExecuteAction, func.allowedConnectionIds), Source: makeLocalDevSource(), }; + const dispatch: ExecutionDispatch = { + executeAction: guardedExecuteAction, + allowedConnectionIds: func.allowedConnectionIds, + isAbandoned: () => !scope.isCurrent(), + functionName: func.name, + $, + }; + const run = async (): Promise => { // Loads the customer module before installing $ and the SDK bridges, matching production's import order (backend/virtual-entry.ts) — code reaching for $ during top-level evaluation fails the same way locally as in Datadog, instead of succeeding early. const mod = await loadModule(func.absolutePath + LOCAL_EXECUTION_LOAD_SUFFIX); @@ -193,38 +380,41 @@ export async function executeScriptLocally( throw new Error(`"${func.name}" is not a function exported from ${func.absolutePath}`); } - // Restores whatever globalThis.$ held before this call (or removes it) once the execution settles, so a pre-existing global (e.g. zx/globals) isn't clobbered and this execution's context isn't left reachable afterward. - const hadPreviousDollar = Object.prototype.hasOwnProperty.call(globalThis, '$'); - const previousDollar = getGlobalDollar(); - setGlobalDollar($); - try { - const actionCatalogRegistration = registerActionCatalogIfInstalled( - loadModule, - projectRoot, - executeAction, - func.allowedConnectionIds, - ); - const backendRuntimeRegistration = registerBackendRuntimeIfInstalled( - loadModule, - projectRoot, - $, - ); - await Promise.all([actionCatalogRegistration, backendRuntimeRegistration]); - - const result = await fn(...args); - return { data: result }; - } finally { - if (hadPreviousDollar) { - setGlobalDollar(previousDollar); - } else { - deleteGlobalDollar(); - } - } + // Scopes globalThis.$ and the action-catalog/apps-backend dispatch info to this call's own async continuation chain — see backendGlobalsContext's and executionDispatchContext's doc comments. + return backendGlobalsContext.run({ value: $ }, () => + executionDispatchContext.run(dispatch, async () => { + try { + // The action-catalog/apps-backend adapters are stable and idempotent to re-register — see their own doc comments — so no coordination is needed between the two registrations or across executions. + const actionCatalogRegistration = registerActionCatalogIfInstalled( + loadModule, + projectRoot, + ); + const backendRuntimeRegistration = registerBackendRuntimeIfInstalled( + loadModule, + projectRoot, + ); + await Promise.all([actionCatalogRegistration, backendRuntimeRegistration]); + + if (!scope.isCurrent()) { + // Already known-abandoned before the customer function was reached — no point invoking it now. + throw new Error( + `Execution of "${func.name}" was abandoned after timing out before it could start.`, + ); + } + const result = await fn(...args); + return { data: assertJsonSerializable(result, func) }; + } finally { + // However this execution ends, mark it concluded so any further dispatch through it — direct or via the shared adapters — is rejected. + concludeExecution(); + } + }), + ); }; let timer: ReturnType | undefined; const timeout = new Promise((_resolve, reject) => { timer = setTimeout(() => { + concludeExecution(); reject(new Error(`Local execution of "${func.name}" timed out after ${timeoutMs}ms`)); }, timeoutMs); }); From ee4212563279be4b79309143803a1e1c06933b3a Mon Sep 17 00:00:00 2001 From: Tiffany Trinh Date: Thu, 27 Aug 2026 01:57:48 -0400 Subject: [PATCH 02/19] fix(apps): forward the real apps-backend runtime's own property shape MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The stable Proxy wrapped every property access in a synthetic callable, assuming the real @datadog/apps-backend runtime is a flat set of methods. It isn't — e.g. user identity is a nested `.user.getExecutionUser()` namespace — so any nested accessor threw "is not a function". Forward each property straight through to the real, dispatch-cached runtime instead. --- .../plugins/apps/src/vite/local-execution.ts | 34 ++++++++----------- 1 file changed, 14 insertions(+), 20 deletions(-) diff --git a/packages/plugins/apps/src/vite/local-execution.ts b/packages/plugins/apps/src/vite/local-execution.ts index babeb3bac..83de61477 100644 --- a/packages/plugins/apps/src/vite/local-execution.ts +++ b/packages/plugins/apps/src/vite/local-execution.ts @@ -257,30 +257,24 @@ async function registerBackendRuntimeOnce( } // Built once per execution (cached by dispatch identity), not once per accessor call — dispatch.$ is fixed for its whole execution, so rebuilding on every property access wasted work without changing the result. const runtimeByDispatch = new WeakMap(); - // Every property access returns a callable, not a value — the real package calls specific methods (e.g. getInitiatingUser()), not just reads properties. + // Forwards to whatever shape the real runtime's own property has — a nested namespace (e.g. `.user.getExecutionUser()`) as well as a flat method — rather than assuming every property is itself a callable, which the real @datadog/apps-backend runtime is not. const backendRuntimeProxy = new Proxy( {}, { get(_target, prop) { - return (...args: unknown[]) => { - const dispatch = executionDispatchContext.getStore(); - if (!dispatch || dispatch.isAbandoned()) { - throw new Error( - `Execution of "${dispatch?.functionName ?? 'unknown'}" already concluded; ` + - `refusing to resolve a further apps-backend accessor under its identity.`, - ); - } - let runtime = runtimeByDispatch.get(dispatch); - if (runtime === undefined) { - runtime = buildRuntimeFromJsFunctionWithActions(dispatch.$); - runtimeByDispatch.set(dispatch, runtime); - } - const method = isIndexableRecord(runtime) ? runtime[String(prop)] : undefined; - if (typeof method !== 'function') { - throw new Error(`apps-backend runtime has no method "${String(prop)}"`); - } - return method.apply(runtime, args); - }; + const dispatch = executionDispatchContext.getStore(); + if (!dispatch || dispatch.isAbandoned()) { + throw new Error( + `Execution of "${dispatch?.functionName ?? 'unknown'}" already concluded; ` + + `refusing to resolve a further apps-backend accessor under its identity.`, + ); + } + let runtime = runtimeByDispatch.get(dispatch); + if (runtime === undefined) { + runtime = buildRuntimeFromJsFunctionWithActions(dispatch.$); + runtimeByDispatch.set(dispatch, runtime); + } + return isIndexableRecord(runtime) ? runtime[String(prop)] : undefined; }, }, ); From 15db5c14d663544d09d057d808145988cfd1bbcb Mon Sep 17 00:00:00 2001 From: Tiffany Trinh Date: Thu, 27 Aug 2026 02:29:10 -0400 Subject: [PATCH 03/19] fix(apps): address PR review findings on $ seeding, test types, and stale docs Seeds globalDollarOutsideExecution from any globalThis.$ already installed before this module loads (e.g. zx/globals), so installing the accessor doesn't silently discard a pre-existing value. Replaces 4 remaining any-casts in the test file with the existing testDollar() helper, and rewords 10 comments across local-execution.test.ts and execution-epoch.ts that still described the removed poisoning mechanism or named consumer files that don't exist yet. --- .../plugins/apps/src/vite/execution-epoch.ts | 4 +- .../apps/src/vite/local-execution.test.ts | 46 +++++++++++++------ .../plugins/apps/src/vite/local-execution.ts | 4 +- 3 files changed, 35 insertions(+), 19 deletions(-) diff --git a/packages/plugins/apps/src/vite/execution-epoch.ts b/packages/plugins/apps/src/vite/execution-epoch.ts index f48762b96..0578c75c1 100644 --- a/packages/plugins/apps/src/vite/execution-epoch.ts +++ b/packages/plugins/apps/src/vite/execution-epoch.ts @@ -2,7 +2,7 @@ // This product includes software developed at Datadog (https://www.datadoghq.com/). // Copyright 2019-Present Datadog, Inc. -/** Generation-counter guard so an abandoned scope's late cleanup can't touch a shared resource a newer scope now owns (used by `network-guard.ts`, `env-guard.ts`, `local-execution.ts`). */ +/** Generation-counter guard so an abandoned scope's late cleanup can't touch a shared resource a newer scope now owns (used by `local-execution.ts`). */ export interface EpochScope { /** True until a newer scope starts, or this one (or every scope) is concluded/invalidated. */ isCurrent(): boolean; @@ -13,7 +13,7 @@ export interface EpochScope { export interface EpochGuard { /** Starts a new scope, superseding whichever one was previously active. */ start(): EpochScope; - /** True if some started scope hasn't yet been concluded or superseded (e.g. for `network-guard.ts`'s `runAllowed`). */ + /** True if some started scope hasn't yet been concluded or superseded. */ hasActiveScope(): boolean; /** Unconditionally invalidates the active scope without starting a new one — the backstop for a scope whose own `fn` never settles. */ forceInvalidate(): void; diff --git a/packages/plugins/apps/src/vite/local-execution.test.ts b/packages/plugins/apps/src/vite/local-execution.test.ts index 4cfad6836..3fa252c89 100644 --- a/packages/plugins/apps/src/vite/local-execution.test.ts +++ b/packages/plugins/apps/src/vite/local-execution.test.ts @@ -548,6 +548,25 @@ describe('local-execution — executeScriptLocally', () => { } }); + test('Should seed the outside-execution slot from a globalThis.$ that already existed before this module was first loaded', () => { + const originalDescriptor = Object.getOwnPropertyDescriptor(globalThis, '$'); + const preExisting = { fromZxGlobals: true }; + (globalThis as Record).$ = preExisting; + try { + jest.isolateModules(() => { + // A fresh module instance re-runs its top-level Object.defineProperty, which must read + // the current globalThis.$ (still `preExisting`, via the outer instance's own getter) + // before replacing the descriptor with its own — not start from an empty slot. + require('./local-execution'); + }); + expect((globalThis as Record).$).toBe(preExisting); + } finally { + if (originalDescriptor) { + Object.defineProperty(globalThis, '$', originalDescriptor); + } + } + }); + test('Should read globalThis.$ as undefined once the execution completes when nothing was defined before it started', async () => { (globalThis as Record).$ = undefined; await executeScriptLocally( @@ -582,7 +601,7 @@ describe('local-execution — executeScriptLocally', () => { [], stubExecuteAction, loadModuleReturning({ - example: () => Object.keys((globalThis as Record).$).sort(), + example: () => Object.keys(testDollar()).sort(), }), mockLogger, ); @@ -1023,10 +1042,7 @@ describe('local-execution — executeScriptLocally', () => { function readOwnArgsAfterDelay(delayMs: number): () => Promise { return () => new Promise((resolve) => - setTimeout( - () => resolve((globalThis as Record).$.backendFunctionArgs), - delayMs, - ), + setTimeout(() => resolve(testDollar().backendFunctionArgs), delayMs), ); } @@ -1092,7 +1108,7 @@ describe('local-execution — executeScriptLocally', () => { loadModuleReturning({ example: async () => { // Captured BEFORE the timeout fires — this execution's own Actions proxy, not whatever globalThis.$ points to later. - const { Actions } = (globalThis as Record).$; + const { Actions } = testDollar(); // Outlives the 20ms timeout below, so the caller already sees a rejection by the time this line runs. await new Promise((resolve) => setTimeout(resolve, 100)); try { @@ -1146,7 +1162,7 @@ describe('local-execution — executeScriptLocally', () => { example: async () => { // Fires ~60ms in, squarely inside funcB's in-flight window — a fresh $ read here needs AsyncLocalStorage, not the abandoned closure check, or it would resolve to funcB's $. await new Promise((resolve) => setTimeout(resolve, 60)); - const $ = (globalThis as Record).$; + const $ = testDollar(); try { // funcB's own connectionId, not funcA's — only valid if this call incorrectly runs under funcB's still-live identity. await $.Actions.foo.bar({ inputs: {}, connectionId: 'conn-B' }); @@ -1187,7 +1203,7 @@ describe('local-execution — executeScriptLocally', () => { expect(executeAction).not.toHaveBeenCalled(); }); - // Action-catalog holds one executeAction implementation in shared module state — a per-closure abandoned guard can't protect a typed-wrapper call once a newer execution re-registers, so poisonActionCatalogRegistration proactively replaces it with a rejecting stub on conclusion. + // Action-catalog's registered dispatcher is stable and execution-agnostic — it resolves the calling execution's own dispatch from AsyncLocalStorage at call time, so a per-closure guard alone (bypassed once a newer execution re-registers) isn't what protects a stale typed-wrapper call. test("Should reject an abandoned execution's action-catalog typed-wrapper call, not silently run it under a newer registration", async () => { jest.spyOn(shared, 'isActionCatalogInstalled').mockReturnValue(true); let abandonedCallOutcome: 'pending' | 'resolved' | { rejected: string } = 'pending'; @@ -1239,7 +1255,7 @@ describe('local-execution — executeScriptLocally', () => { ); await expect(abandoned).rejects.toThrow(/timed out after 20ms/); - // registeredImpl now points at the abandoned execution's own implementation, poisoned by the timeout handler — deliberately no second execution here, to isolate the poison step. + // registeredImpl still points at this (only) execution's own registration — no second execution registers here. The call is rejected because the dispatcher resolves this execution's own dispatch, already concluded by the 20ms timeout. await new Promise((resolve) => setTimeout(resolve, 100)); expect(abandonedCallOutcome).toEqual({ @@ -1247,7 +1263,7 @@ describe('local-execution — executeScriptLocally', () => { }); }); - // Poisoning only protects the window before a newer execution registers — once it does, its own register() call (correctly, from its own perspective) overwrites the poison stub. A zombie action-catalog call made after that point must still be rejected, not routed through the newer execution's identity/allowedConnectionIds. + // registeredImpl comes to point at funcB's own registration once it registers, but a call made from within funcA's own continuation still resolves funcA's own (concluded) dispatch via AsyncLocalStorage — it must still be rejected, not routed through funcB's identity/allowedConnectionIds just because funcB's registration is the one currently referenced. test("Should reject a zombie execution's action-catalog typed-wrapper call even after a newer execution has legitimately re-registered its own implementation", async () => { jest.spyOn(shared, 'isActionCatalogInstalled').mockReturnValue(true); const funcA: BackendFunction = { ...func, allowedConnectionIds: ['conn-A'] }; @@ -1280,7 +1296,7 @@ describe('local-execution — executeScriptLocally', () => { }; }; - // Times out at 20ms, then calls the typed wrapper ~60ms in — squarely inside funcB's own in-flight window (funcB registers immediately but doesn't complete, and self-poison, until 80ms) — using conn-B, a connection funcA itself is never allowed to use. + // Times out at 20ms, then calls the typed wrapper ~60ms in — squarely inside funcB's own in-flight window (funcB registers immediately but doesn't conclude until 80ms) — using conn-B, a connection funcA itself is never allowed to use. const abandoned = executeScriptLocally( funcA, TEST_PROJECT_ROOT, @@ -1306,7 +1322,7 @@ describe('local-execution — executeScriptLocally', () => { ); await expect(abandoned).rejects.toThrow(/timed out after 20ms/); - // Starts as soon as the queue frees, registers immediately, but doesn't complete (and self-poison on conclusion) until 80ms — overlapping funcA's 60ms zombie wakeup. + // Starts as soon as the queue frees, registers immediately, but doesn't conclude until 80ms — overlapping funcA's 60ms zombie wakeup. const second = executeScriptLocally( funcB, TEST_PROJECT_ROOT, @@ -1324,7 +1340,7 @@ describe('local-execution — executeScriptLocally', () => { expect(executeAction).not.toHaveBeenCalled(); }); - // The apps-backend loadModule call hangs forever here — a post-Promise.all destructuring assignment would never run, so publishing each handle via .then() is what lets the completed action-catalog registration still get poisoned. + // The apps-backend loadModule call hangs forever here — a post-Promise.all destructuring assignment would never run, so publishing each handle via .then() as its own promise resolves is what lets the completed action-catalog registration still take effect. test('Should still register the action-catalog adapter even when the sibling apps-backend registration never settles, and reject a call once no execution is active', async () => { jest.spyOn(shared, 'isActionCatalogInstalled').mockReturnValue(true); jest.spyOn(shared, 'isDatadogAppsBackendInstalled').mockReturnValue(true); @@ -1375,7 +1391,7 @@ describe('local-execution — executeScriptLocally', () => { ); }); - // An abandoned execution's fn() can settle normally later — its finally block must not re-poison the registration over whatever a newer execution already put there. + // An abandoned execution's fn() can settle normally later — its finally block's conclude step must not disturb whatever a newer execution's own registration already put in place. test("Should not let a late-settling abandoned execution's own conclusion clobber a newer execution's already-registered action-catalog implementation", async () => { jest.spyOn(shared, 'isActionCatalogInstalled').mockReturnValue(true); let registeredImpl: @@ -1429,7 +1445,7 @@ describe('local-execution — executeScriptLocally', () => { ); expect(second).toEqual({ data: 'B' }); - // Captures whatever B's own conclusion left registered — B poisoning its own registration on completion is fine; nothing else must overwrite it. + // Captures whatever B's own conclusion left registered — B's own registration staying in place after it concludes is fine; nothing else must overwrite it. const registeredAfterB = registeredImpl; // Give the abandoned execution's late-settling fn() and its finally block room to run. diff --git a/packages/plugins/apps/src/vite/local-execution.ts b/packages/plugins/apps/src/vite/local-execution.ts index 83de61477..7e582a63e 100644 --- a/packages/plugins/apps/src/vite/local-execution.ts +++ b/packages/plugins/apps/src/vite/local-execution.ts @@ -27,8 +27,8 @@ type BackendGlobalsBox = { value: unknown }; /** Scopes `globalThis.$` per execution via AsyncLocalStorage, not a plain mutable property, so a zombie execution's late "fresh" `globalThis.$` read resolves to its own `$`, never a newer execution's identity/`allowedConnectionIds`. */ const backendGlobalsContext = new AsyncLocalStorage(); -/** Backs `globalThis.$` for reads/writes that happen with no execution box on the AsyncLocalStorage-scoped call stack (e.g. this module's own import-time state) — an ordinary mutable slot, since there's no per-execution box to isolate it into. */ -let globalDollarOutsideExecution: unknown; +/** Backs `globalThis.$` for reads/writes that happen with no execution box on the AsyncLocalStorage-scoped call stack (e.g. this module's own import-time state) — an ordinary mutable slot, since there's no per-execution box to isolate it into. Seeded from any `$` already installed before this module loaded (e.g. `zx/globals`, which assigns `globalThis.$` at its own import time), so installing the accessor below doesn't silently discard it. */ +let globalDollarOutsideExecution: unknown = Reflect.get(globalThis, '$'); Object.defineProperty(globalThis, '$', { configurable: true, From 113ac9d656db2a68c6546abdb7a35502776c3224 Mon Sep 17 00:00:00 2001 From: Tiffany Trinh Date: Thu, 27 Aug 2026 03:00:17 -0400 Subject: [PATCH 04/19] fix(apps): reject Map/Set results instead of silently flattening them to {} MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit JSON.stringify(new Map(...)) and JSON.stringify(new Set(...)) both return '{}' — a defined string, not undefined — so assertJsonSerializable's existing undefined-check never caught them, silently dropping all of a Map's/Set's entries instead of surfacing the same clear error given to other non-serializable shapes (BigInt, functions, circular references). --- .../apps/src/vite/local-execution.test.ts | 26 +++++++++++++++++++ .../plugins/apps/src/vite/local-execution.ts | 7 ++++- 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/packages/plugins/apps/src/vite/local-execution.test.ts b/packages/plugins/apps/src/vite/local-execution.test.ts index 3fa252c89..504c22f15 100644 --- a/packages/plugins/apps/src/vite/local-execution.test.ts +++ b/packages/plugins/apps/src/vite/local-execution.test.ts @@ -950,6 +950,32 @@ describe('local-execution — executeScriptLocally', () => { ).rejects.toThrow(/example.*JSON.stringify silently drops/); }); + test('Should reject with a clear, attributed error when the result is a Map (silently flattened to "{}" by JSON.stringify)', async () => { + await expect( + executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModuleReturning({ example: () => new Map([['a', 1]]) }), + mockLogger, + ), + ).rejects.toThrow(/example.*silently flattens/); + }); + + test('Should reject with a clear, attributed error when the result is a Set (silently flattened to "{}" by JSON.stringify)', async () => { + await expect( + executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModuleReturning({ example: () => new Set([1, 2, 3]) }), + mockLogger, + ), + ).rejects.toThrow(/example.*silently flattens/); + }); + test('Should allow an explicit undefined result through unchanged', async () => { const result = await executeScriptLocally( func, diff --git a/packages/plugins/apps/src/vite/local-execution.ts b/packages/plugins/apps/src/vite/local-execution.ts index 7e582a63e..c9aa5e5ea 100644 --- a/packages/plugins/apps/src/vite/local-execution.ts +++ b/packages/plugins/apps/src/vite/local-execution.ts @@ -281,8 +281,13 @@ async function registerBackendRuntimeOnce( setBackend(backendRuntimeProxy); } -/** Rejects a non-JSON-serializable result (circular reference/`BigInt`, or a bare function/`Symbol` that `JSON.stringify` silently drops) here with a clear error, instead of failing downstream when serialized for the HTTP response. */ +/** Rejects a non-JSON-serializable result (circular reference/`BigInt`, a bare function/`Symbol` that `JSON.stringify` silently drops, or a `Map`/`Set` that it silently flattens to `{}` since neither exposes its entries as own enumerable properties) here with a clear error, instead of failing downstream when serialized for the HTTP response. */ function assertJsonSerializable(result: unknown, func: BackendFunction): unknown { + if (result instanceof Map || result instanceof Set) { + throw new Error( + `Local execution of "${func.name}" returned a ${result.constructor.name}, which JSON.stringify silently flattens to "{}" instead of serializing its entries — return a plain array or object instead.`, + ); + } let serialized: string | undefined; try { serialized = JSON.stringify(result); From 1b313cfa4724c71ccbc4c62c190d566ff3f29b82 Mon Sep 17 00:00:00 2001 From: Tiffany Trinh Date: Thu, 27 Aug 2026 10:05:30 -0400 Subject: [PATCH 05/19] fix(apps): re-check installedness on every call instead of caching a negative result MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit registerActionCatalogIfInstalled/registerBackendRuntimeIfInstalled cached the 'not installed' outcome in the same WeakMap as a successful registration, keyed by loadModule identity — a dev server reuses the same loadModule for its whole lifetime, so once neither package was found, a customer installing it mid-session (without restarting) got permanently skipped instead of picked up on the next execution. The uncached installedness check is a cheap require.resolve probe; only a *successful* registration needs the once-ever WeakMap treatment. --- .../apps/src/vite/local-execution.test.ts | 49 +++++++++++++++++++ .../plugins/apps/src/vite/local-execution.ts | 30 +++++------- 2 files changed, 61 insertions(+), 18 deletions(-) diff --git a/packages/plugins/apps/src/vite/local-execution.test.ts b/packages/plugins/apps/src/vite/local-execution.test.ts index 504c22f15..7f4691eae 100644 --- a/packages/plugins/apps/src/vite/local-execution.test.ts +++ b/packages/plugins/apps/src/vite/local-execution.test.ts @@ -622,6 +622,55 @@ describe('local-execution — executeScriptLocally', () => { expect(result).toEqual({ data: 'fine' }); }); + test('Should pick up action-catalog on the very next execution after it becomes installed mid-session, not stay permanently skipped', async () => { + const isInstalledSpy = jest + .spyOn(shared, 'isActionCatalogInstalled') + .mockReturnValue(false); + let registeredImpl: + | ((actionId: string, request: unknown) => Promise) + | undefined; + const loadModule: LoadModule = async (specifier: string) => { + if (specifier === func.absolutePath + LOCAL_EXECUTION_LOAD_SUFFIX) { + return { example: () => 'fine' }; + } + if (specifier === '@datadog/action-catalog/action-execution') { + return { + setExecuteActionImplementation: ( + impl: (actionId: string, request: unknown) => Promise, + ) => { + registeredImpl = impl; + }, + }; + } + const error: NodeJS.ErrnoException = new Error(`Cannot find module '${specifier}'`); + error.code = 'MODULE_NOT_FOUND'; + throw error; + }; + + // Not installed yet — registration is skipped, same as the "neither package installed" case. + await executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModule, + mockLogger, + ); + expect(registeredImpl).toBeUndefined(); + + // Simulates `npm install @datadog/action-catalog` without restarting the dev server — the very next execution must register it, not stay permanently skipped from the first (uncached) negative check. + isInstalledSpy.mockReturnValue(true); + await executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModule, + mockLogger, + ); + expect(registeredImpl).toBeDefined(); + }); + test('Should propagate a real load failure from an installed action-catalog package, not treat it as absent', async () => { jest.spyOn(shared, 'isActionCatalogInstalled').mockReturnValue(true); const loadModule: LoadModule = async (specifier: string) => { diff --git a/packages/plugins/apps/src/vite/local-execution.ts b/packages/plugins/apps/src/vite/local-execution.ts index c9aa5e5ea..9d9b29cc6 100644 --- a/packages/plugins/apps/src/vite/local-execution.ts +++ b/packages/plugins/apps/src/vite/local-execution.ts @@ -165,16 +165,19 @@ function makeActionsProxy( /** Keyed by `loadModule` identity, not a bare module-level flag — a real dev server reuses the same Vite `ssrLoadModule` for its whole lifetime (giving true once-ever registration), while each test constructs its own `loadModule` closure (keeping tests isolated from each other's registration state). A rejection is evicted so the next execution retries, rather than permanently poisoning every later execution with one transient load failure. */ const actionCatalogRegistrations = new WeakMap>(); -/** No-ops if @datadog/action-catalog isn't installed. Registers ONE stable dispatcher for the process lifetime — it reads `executionDispatchContext.getStore()` at call time to resolve whichever execution is actually on the AsyncLocalStorage-scoped call stack, so a zombie execution's typed-wrapper call can never be routed through a newer execution's identity/allowedConnectionIds just because that execution's own registration is the one currently live. */ +/** No-ops if @datadog/action-catalog isn't installed — re-checked on every call, uncached, so installing the package mid-session (without restarting the dev server) is picked up on the very next execution instead of staying permanently no-op. Once installed, registers ONE stable dispatcher for the process lifetime — it reads `executionDispatchContext.getStore()` at call time to resolve whichever execution is actually on the AsyncLocalStorage-scoped call stack, so a zombie execution's typed-wrapper call can never be routed through a newer execution's identity/allowedConnectionIds just because that execution's own registration is the one currently live. */ function registerActionCatalogIfInstalled( loadModule: LoadModule, projectRoot: string, ): Promise { + if (!isActionCatalogInstalled(projectRoot)) { + return Promise.resolve(); + } const existing = actionCatalogRegistrations.get(loadModule); if (existing) { return existing; } - const registration = registerActionCatalogOnce(loadModule, projectRoot).catch((err) => { + const registration = registerActionCatalogOnce(loadModule).catch((err) => { actionCatalogRegistrations.delete(loadModule); throw err; }); @@ -182,13 +185,7 @@ function registerActionCatalogIfInstalled( return registration; } -async function registerActionCatalogOnce( - loadModule: LoadModule, - projectRoot: string, -): Promise { - if (!isActionCatalogInstalled(projectRoot)) { - return; - } +async function registerActionCatalogOnce(loadModule: LoadModule): Promise { const mod = await loadModule('@datadog/action-catalog/action-execution'); const setExecuteActionImplementation = mod.setExecuteActionImplementation; if (typeof setExecuteActionImplementation !== 'function') { @@ -218,16 +215,19 @@ async function registerActionCatalogOnce( /** Mirrors `actionCatalogRegistrations` — see its doc comment for why keying on `loadModule` identity is safe across both real dev-server reuse and per-test isolation. */ const backendRuntimeRegistrations = new WeakMap>(); -/** No-ops if @datadog/apps-backend isn't installed. Registers ONE stable runtime Proxy for the process lifetime — every accessor call resolves whichever execution's `$` is on the AsyncLocalStorage-scoped call stack (or rejects if that execution has concluded), rather than a runtime bound to a specific execution's `$` at registration time. */ +/** No-ops if @datadog/apps-backend isn't installed — re-checked on every call, uncached, so installing the package mid-session (without restarting the dev server) is picked up on the very next execution instead of staying permanently no-op. Once installed, registers ONE stable runtime Proxy for the process lifetime — every accessor call resolves whichever execution's `$` is on the AsyncLocalStorage-scoped call stack (or rejects if that execution has concluded), rather than a runtime bound to a specific execution's `$` at registration time. */ function registerBackendRuntimeIfInstalled( loadModule: LoadModule, projectRoot: string, ): Promise { + if (!isDatadogAppsBackendInstalled(projectRoot)) { + return Promise.resolve(); + } const existing = backendRuntimeRegistrations.get(loadModule); if (existing) { return existing; } - const registration = registerBackendRuntimeOnce(loadModule, projectRoot).catch((err) => { + const registration = registerBackendRuntimeOnce(loadModule).catch((err) => { backendRuntimeRegistrations.delete(loadModule); throw err; }); @@ -235,13 +235,7 @@ function registerBackendRuntimeIfInstalled( return registration; } -async function registerBackendRuntimeOnce( - loadModule: LoadModule, - projectRoot: string, -): Promise { - if (!isDatadogAppsBackendInstalled(projectRoot)) { - return; - } +async function registerBackendRuntimeOnce(loadModule: LoadModule): Promise { const [jsFunctionWithActionsModule, runtimeModule] = await Promise.all([ loadModule('@datadog/apps-backend/runtime/jsFunctionWithActions'), loadModule('@datadog/apps-backend/runtime'), From 4a8acc560bbd8077962ef34194a71b398793f6be Mon Sep 17 00:00:00 2001 From: Tiffany Trinh Date: Thu, 27 Aug 2026 14:49:43 -0400 Subject: [PATCH 06/19] fix(apps): reject non-finite numbers as a local-execution result MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit JSON.stringify silently converts NaN/Infinity to "null" without throwing, unlike every other non-serializable shape this check already catches (Map/Set/BigInt/function/symbol) — a customer bug that produces a non-finite result was returning a silent null instead of a clear, attributed error. --- .../apps/src/vite/local-execution.test.ts | 26 +++++++++++++++++++ .../plugins/apps/src/vite/local-execution.ts | 5 ++++ 2 files changed, 31 insertions(+) diff --git a/packages/plugins/apps/src/vite/local-execution.test.ts b/packages/plugins/apps/src/vite/local-execution.test.ts index 7f4691eae..6047526c1 100644 --- a/packages/plugins/apps/src/vite/local-execution.test.ts +++ b/packages/plugins/apps/src/vite/local-execution.test.ts @@ -1025,6 +1025,32 @@ describe('local-execution — executeScriptLocally', () => { ).rejects.toThrow(/example.*silently flattens/); }); + test('Should reject with a clear, attributed error when the result is NaN (silently converted to "null" by JSON.stringify)', async () => { + await expect( + executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModuleReturning({ example: () => NaN }), + mockLogger, + ), + ).rejects.toThrow(/example.*silently converts to "null"/); + }); + + test('Should reject with a clear, attributed error when the result is Infinity (silently converted to "null" by JSON.stringify)', async () => { + await expect( + executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModuleReturning({ example: () => Infinity }), + mockLogger, + ), + ).rejects.toThrow(/example.*silently converts to "null"/); + }); + test('Should allow an explicit undefined result through unchanged', async () => { const result = await executeScriptLocally( func, diff --git a/packages/plugins/apps/src/vite/local-execution.ts b/packages/plugins/apps/src/vite/local-execution.ts index 9d9b29cc6..88ff1ae7b 100644 --- a/packages/plugins/apps/src/vite/local-execution.ts +++ b/packages/plugins/apps/src/vite/local-execution.ts @@ -282,6 +282,11 @@ function assertJsonSerializable(result: unknown, func: BackendFunction): unknown `Local execution of "${func.name}" returned a ${result.constructor.name}, which JSON.stringify silently flattens to "{}" instead of serializing its entries — return a plain array or object instead.`, ); } + if (typeof result === 'number' && !Number.isFinite(result)) { + throw new Error( + `Local execution of "${func.name}" returned ${result}, which JSON.stringify silently converts to "null" instead of throwing — return a finite number instead.`, + ); + } let serialized: string | undefined; try { serialized = JSON.stringify(result); From 6dfa76ba7d452e265f8d085b24e4cbb235ee2d51 Mon Sep 17 00:00:00 2001 From: Tiffany Trinh Date: Thu, 27 Aug 2026 16:25:46 -0400 Subject: [PATCH 07/19] fix(apps): preserve this-binding on a flat apps-backend runtime method Reading a runtime property directly off the proxy's target lost its this-binding when called as backend.someMethod(), breaking any real accessor that reads its own state via this instead of a closure. Also distinguishes the apps-backend accessor's "no active execution" case from "execution already concluded" the same way the action-catalog dispatcher already does, instead of reporting a timeout that may not have happened. --- .../apps/src/vite/local-execution.test.ts | 62 +++++++++++++++++++ .../plugins/apps/src/vite/local-execution.ts | 20 +++++- 2 files changed, 79 insertions(+), 3 deletions(-) diff --git a/packages/plugins/apps/src/vite/local-execution.test.ts b/packages/plugins/apps/src/vite/local-execution.test.ts index 6047526c1..b5e14760f 100644 --- a/packages/plugins/apps/src/vite/local-execution.test.ts +++ b/packages/plugins/apps/src/vite/local-execution.test.ts @@ -951,6 +951,68 @@ describe('local-execution — executeScriptLocally', () => { rejected: expect.stringContaining('already concluded'), }); }); + + // A flat method reading its own internal state via `this` (a real, common accessor + // pattern) must still work when called through the backend-runtime proxy — not just + // arrow-function methods that close over data instead, which every other test here uses. + test('Should preserve `this` when a flat apps-backend runtime method reads its own internal state', async () => { + jest.spyOn(shared, 'isDatadogAppsBackendInstalled').mockReturnValue(true); + let registeredBackend: { getUserId(): string } | undefined; + let capturedUserId: unknown; + + const loadModule: LoadModule = async (specifier: string) => { + if (specifier === func.absolutePath + LOCAL_EXECUTION_LOAD_SUFFIX) { + return { + example: () => { + capturedUserId = registeredBackend?.getUserId(); + return 'done'; + }, + }; + } + if (specifier === '@datadog/apps-backend/runtime/jsFunctionWithActions') { + return { + buildRuntimeFromJsFunctionWithActions: () => ({ + userId: 'real-user-id', + // A real accessor pattern: reads its own instance state via `this`, + // not a closure — throws if called unbound. + getUserId() { + if ( + !this || + typeof (this as { userId?: unknown }).userId !== 'string' + ) { + throw new Error('getUserId called with no `this`'); + } + return (this as { userId: string }).userId; + }, + }), + }; + } + if (specifier === '@datadog/apps-backend/runtime') { + return { + setBackend: (runtime: { getUserId(): string }) => { + registeredBackend = runtime; + }, + }; + } + const notFoundError: NodeJS.ErrnoException = new Error( + `Cannot find module '${specifier}'`, + ); + notFoundError.code = 'MODULE_NOT_FOUND'; + throw notFoundError; + }; + + const result = await executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModule, + mockLogger, + ); + + expect(result).toEqual({ data: 'done' }); + expect(capturedUserId).toBe('real-user-id'); + }); }); describe('non-serializable results', () => { diff --git a/packages/plugins/apps/src/vite/local-execution.ts b/packages/plugins/apps/src/vite/local-execution.ts index 88ff1ae7b..38119ea4a 100644 --- a/packages/plugins/apps/src/vite/local-execution.ts +++ b/packages/plugins/apps/src/vite/local-execution.ts @@ -257,9 +257,14 @@ async function registerBackendRuntimeOnce(loadModule: LoadModule): Promise { get(_target, prop) { const dispatch = executionDispatchContext.getStore(); - if (!dispatch || dispatch.isAbandoned()) { + if (!dispatch) { throw new Error( - `Execution of "${dispatch?.functionName ?? 'unknown'}" already concluded; ` + + `No active local execution to resolve an apps-backend accessor under.`, + ); + } + if (dispatch.isAbandoned()) { + throw new Error( + `Execution of "${dispatch.functionName}" already concluded; ` + `refusing to resolve a further apps-backend accessor under its identity.`, ); } @@ -268,7 +273,16 @@ async function registerBackendRuntimeOnce(loadModule: LoadModule): Promise runtime = buildRuntimeFromJsFunctionWithActions(dispatch.$); runtimeByDispatch.set(dispatch, runtime); } - return isIndexableRecord(runtime) ? runtime[String(prop)] : undefined; + if (!isIndexableRecord(runtime)) { + return undefined; + } + const value = runtime[String(prop)]; + // A flat method (e.g. .getExecutionUser()) reads its own internal state via + // `this` — returning it unbound would call it with `this` bound to this Proxy's + // empty target instead of the real runtime object. A nested namespace property + // (e.g. .user) is returned as-is; its own methods keep correct `this` since the + // real sub-object, not this proxy, is what ends up receiving the call. + return typeof value === 'function' ? value.bind(runtime) : value; }, }, ); From 7eb0172198327ec091a3f920bc5638b516fc9755 Mon Sep 17 00:00:00 2001 From: Tiffany Trinh Date: Thu, 27 Aug 2026 17:41:58 -0400 Subject: [PATCH 08/19] fix(apps): reject a Map/Set/non-finite number nested anywhere in a local-execution result assertJsonSerializable only rejected a Map, Set, NaN, or Infinity at the top level of a returned result. A JSON.stringify replacer runs on every key/value pair it visits (root included), so checking there catches the same values nested inside a plain object or array too, where JSON.stringify would otherwise silently flatten them to "{}" or "null" instead of throwing. --- .../apps/src/vite/local-execution.test.ts | 39 +++++++++++++++++++ .../plugins/apps/src/vite/local-execution.ts | 31 +++++++++------ 2 files changed, 59 insertions(+), 11 deletions(-) diff --git a/packages/plugins/apps/src/vite/local-execution.test.ts b/packages/plugins/apps/src/vite/local-execution.test.ts index b5e14760f..ac81aeca7 100644 --- a/packages/plugins/apps/src/vite/local-execution.test.ts +++ b/packages/plugins/apps/src/vite/local-execution.test.ts @@ -1113,6 +1113,45 @@ describe('local-execution — executeScriptLocally', () => { ).rejects.toThrow(/example.*silently converts to "null"/); }); + test('Should reject a Map nested inside a plain object, not just at the top level', async () => { + await expect( + executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModuleReturning({ example: () => ({ data: new Map([['a', 1]]) }) }), + mockLogger, + ), + ).rejects.toThrow(/example.*silently flattens/); + }); + + test('Should reject a Set nested inside an array, not just at the top level', async () => { + await expect( + executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModuleReturning({ example: () => [1, new Set([1, 2, 3])] }), + mockLogger, + ), + ).rejects.toThrow(/example.*silently flattens/); + }); + + test('Should reject a NaN nested inside a plain object, not just at the top level', async () => { + await expect( + executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModuleReturning({ example: () => ({ score: NaN }) }), + mockLogger, + ), + ).rejects.toThrow(/example.*silently converts to "null"/); + }); + test('Should allow an explicit undefined result through unchanged', async () => { const result = await executeScriptLocally( func, diff --git a/packages/plugins/apps/src/vite/local-execution.ts b/packages/plugins/apps/src/vite/local-execution.ts index 38119ea4a..7866ce1c2 100644 --- a/packages/plugins/apps/src/vite/local-execution.ts +++ b/packages/plugins/apps/src/vite/local-execution.ts @@ -290,21 +290,30 @@ async function registerBackendRuntimeOnce(loadModule: LoadModule): Promise } /** Rejects a non-JSON-serializable result (circular reference/`BigInt`, a bare function/`Symbol` that `JSON.stringify` silently drops, or a `Map`/`Set` that it silently flattens to `{}` since neither exposes its entries as own enumerable properties) here with a clear error, instead of failing downstream when serialized for the HTTP response. */ +// Thrown from inside assertJsonSerializable's replacer to carry an already-specific, attributed message straight through the outer catch below, rather than being re-wrapped in its generic "can't be serialized" fallback. +class UnsupportedJsonValueError extends Error {} + function assertJsonSerializable(result: unknown, func: BackendFunction): unknown { - if (result instanceof Map || result instanceof Set) { - throw new Error( - `Local execution of "${func.name}" returned a ${result.constructor.name}, which JSON.stringify silently flattens to "{}" instead of serializing its entries — return a plain array or object instead.`, - ); - } - if (typeof result === 'number' && !Number.isFinite(result)) { - throw new Error( - `Local execution of "${func.name}" returned ${result}, which JSON.stringify silently converts to "null" instead of throwing — return a finite number instead.`, - ); - } let serialized: string | undefined; try { - serialized = JSON.stringify(result); + // A replacer runs on every key/value pair JSON.stringify visits, root included, so a Map/Set/non-finite number nested arbitrarily deep inside the result (e.g. `{ data: new Map() }`) is caught the same way a top-level one is — JSON.stringify would otherwise silently flatten either to "{}" or "null" instead of throwing. + serialized = JSON.stringify(result, (key, value) => { + if (value instanceof Map || value instanceof Set) { + throw new UnsupportedJsonValueError( + `Local execution of "${func.name}" returned a ${value.constructor.name}${key ? ` (at "${key}")` : ''}, which JSON.stringify silently flattens to "{}" instead of serializing its entries — return a plain array or object instead.`, + ); + } + if (typeof value === 'number' && !Number.isFinite(value)) { + throw new UnsupportedJsonValueError( + `Local execution of "${func.name}" returned ${value}${key ? ` (at "${key}")` : ''}, which JSON.stringify silently converts to "null" instead of throwing — return a finite number instead.`, + ); + } + return value; + }); } catch (err) { + if (err instanceof UnsupportedJsonValueError) { + throw 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) From b7cf79250e44b9b525c5686dbc07d7fe6ed79919 Mon Sep 17 00:00:00 2001 From: Tiffany Trinh Date: Thu, 27 Aug 2026 17:58:10 -0400 Subject: [PATCH 09/19] docs(apps): fix testDollar()'s stale reference to a removed helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit testDollar()'s doc comment pointed at local-execution.ts's setGlobalDollar, which no longer exists — globalThis.$ is now backed by an Object.defineProperty accessor scoped through AsyncLocalStorage, not a plain get/set/delete helper trio. --- packages/plugins/apps/src/vite/local-execution.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/plugins/apps/src/vite/local-execution.test.ts b/packages/plugins/apps/src/vite/local-execution.test.ts index ac81aeca7..eed87689c 100644 --- a/packages/plugins/apps/src/vite/local-execution.test.ts +++ b/packages/plugins/apps/src/vite/local-execution.test.ts @@ -31,7 +31,7 @@ interface TestGlobalDollar { Source: { initiator: { id: string; orgId: string }; runAsUser: { id: string; orgId: string } }; } -/** Reads the `$` this module installs on `globalThis`, from the customer-code perspective these tests simulate — untyped since it's a runtime-only property (see `setGlobalDollar`). Centralized here instead of repeating the cast at each call site. */ +/** Reads the `$` this module installs onto `globalThis` during an execution, from the customer-code perspective these tests simulate — genuinely untyped from TypeScript's static perspective since it's a runtime-only accessor property local-execution.ts defines via `Object.defineProperty`. Centralized here instead of repeating the same cast at each call site. */ function testDollar(): TestGlobalDollar { return (globalThis as unknown as { $: TestGlobalDollar }).$; } From fe185d4062430a0568053372872bfc46a8980252 Mon Sep 17 00:00:00 2001 From: Tiffany Trinh Date: Thu, 27 Aug 2026 19:32:17 -0400 Subject: [PATCH 10/19] fix(apps): bound a registration load so it can't permanently poison later executions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A real dev server reuses the same loadModule for its whole process lifetime, memoizing the action-catalog/apps-backend registration per loadModule identity. If the underlying package load never settles (a broken/circular module graph, not just a slow one), the cached promise stays pending forever, and every later execution sharing that loadModule hangs on it until its own timeout — never actually running its function, with no recovery short of a restart. Bounding the load to the execution's own timeoutMs turns an unbounded hang into a rejection, which the existing eviction-on-rejection logic already handles correctly. --- .../apps/src/vite/local-execution.test.ts | 53 ++++++++++++++++++ .../plugins/apps/src/vite/local-execution.ts | 56 +++++++++++++++---- 2 files changed, 98 insertions(+), 11 deletions(-) diff --git a/packages/plugins/apps/src/vite/local-execution.test.ts b/packages/plugins/apps/src/vite/local-execution.test.ts index eed87689c..fc740d130 100644 --- a/packages/plugins/apps/src/vite/local-execution.test.ts +++ b/packages/plugins/apps/src/vite/local-execution.test.ts @@ -1593,6 +1593,59 @@ describe('local-execution — executeScriptLocally', () => { ); }); + // A real dev server reuses the same loadModule for its whole lifetime — a registration load that never settles must not permanently poison every later execution sharing it, so this deliberately reuses one loadModule across two calls instead of each test's usual per-call closure. + test('Should let a later execution register and run after an earlier one shared the same loadModule with a registration load that never settles', async () => { + jest.spyOn(shared, 'isActionCatalogInstalled').mockReturnValue(true); + + let actionCatalogLoadCount = 0; + const loadModule: LoadModule = async (specifier: string) => { + if (specifier === func.absolutePath + LOCAL_EXECUTION_LOAD_SUFFIX) { + return { example: () => 'ok' }; + } + if (specifier === '@datadog/action-catalog/action-execution') { + actionCatalogLoadCount += 1; + if (actionCatalogLoadCount === 1) { + // Simulates a genuinely broken/circular module graph, not just a slow one. + return new Promise(() => {}); + } + return { setExecuteActionImplementation: () => {} }; + } + const notFoundError: NodeJS.ErrnoException = new Error( + `Cannot find module '${specifier}'`, + ); + notFoundError.code = 'MODULE_NOT_FOUND'; + throw notFoundError; + }; + + const first = executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModule, + mockLogger, + 20, + ); + await expect(first).rejects.toThrow(/timed out after 20ms/); + + // Gives the first attempt's own registration timeout (also ~20ms, started microseconds after + // the execution's own timeout above) room to fire and evict its cache entry, the same way a + // real dev server's next request would naturally arrive well after that — not racing the two. + await new Promise((resolve) => setTimeout(resolve, 30)); + + // Without evicting the first attempt's still-pending registration, this would hang until it also times out — never actually invoking its own function. + const second = await executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModule, + mockLogger, + 50, + ); + expect(second).toEqual({ data: 'ok' }); + }); + // An abandoned execution's fn() can settle normally later — its finally block's conclude step must not disturb whatever a newer execution's own registration already put in place. test("Should not let a late-settling abandoned execution's own conclusion clobber a newer execution's already-registered action-catalog implementation", async () => { jest.spyOn(shared, 'isActionCatalogInstalled').mockReturnValue(true); diff --git a/packages/plugins/apps/src/vite/local-execution.ts b/packages/plugins/apps/src/vite/local-execution.ts index 7866ce1c2..47e1157a0 100644 --- a/packages/plugins/apps/src/vite/local-execution.ts +++ b/packages/plugins/apps/src/vite/local-execution.ts @@ -162,13 +162,33 @@ function makeActionsProxy( }); } -/** Keyed by `loadModule` identity, not a bare module-level flag — a real dev server reuses the same Vite `ssrLoadModule` for its whole lifetime (giving true once-ever registration), while each test constructs its own `loadModule` closure (keeping tests isolated from each other's registration state). A rejection is evicted so the next execution retries, rather than permanently poisoning every later execution with one transient load failure. */ +/** Bounds a registration's underlying `loadModule` call to `timeoutMs` so a load that never settles (a broken/circular module graph, not just a slow one) rejects instead of leaving its cache entry pending forever — the existing eviction-on-rejection below only fires once the promise actually settles, and an unbounded load never does. Doesn't cancel the underlying promise (not possible for a plain `Promise`), so a load that eventually does settle still runs its side effects late; see the registration functions' own doc comments for why that's harmless here. */ +function withTimeout(promise: Promise, timeoutMs: number, what: string): Promise { + return new Promise((resolve, reject) => { + const timer = setTimeout(() => { + reject(new Error(`Loading ${what} timed out after ${timeoutMs}ms`)); + }, timeoutMs); + promise.then( + (value) => { + clearTimeout(timer); + resolve(value); + }, + (err: unknown) => { + clearTimeout(timer); + reject(err); + }, + ); + }); +} + +/** Keyed by `loadModule` identity, not a bare module-level flag — a real dev server reuses the same Vite `ssrLoadModule` for its whole lifetime (giving true once-ever registration), while each test constructs its own `loadModule` closure (keeping tests isolated from each other's registration state). A rejection is evicted so the next execution retries, rather than permanently poisoning every later execution with one transient load failure — including a load that never settles at all, since `withTimeout` below turns that into a rejection too. */ const actionCatalogRegistrations = new WeakMap>(); /** No-ops if @datadog/action-catalog isn't installed — re-checked on every call, uncached, so installing the package mid-session (without restarting the dev server) is picked up on the very next execution instead of staying permanently no-op. Once installed, registers ONE stable dispatcher for the process lifetime — it reads `executionDispatchContext.getStore()` at call time to resolve whichever execution is actually on the AsyncLocalStorage-scoped call stack, so a zombie execution's typed-wrapper call can never be routed through a newer execution's identity/allowedConnectionIds just because that execution's own registration is the one currently live. */ function registerActionCatalogIfInstalled( loadModule: LoadModule, projectRoot: string, + timeoutMs: number, ): Promise { if (!isActionCatalogInstalled(projectRoot)) { return Promise.resolve(); @@ -177,7 +197,7 @@ function registerActionCatalogIfInstalled( if (existing) { return existing; } - const registration = registerActionCatalogOnce(loadModule).catch((err) => { + const registration = registerActionCatalogOnce(loadModule, timeoutMs).catch((err) => { actionCatalogRegistrations.delete(loadModule); throw err; }); @@ -185,8 +205,12 @@ function registerActionCatalogIfInstalled( return registration; } -async function registerActionCatalogOnce(loadModule: LoadModule): Promise { - const mod = await loadModule('@datadog/action-catalog/action-execution'); +async function registerActionCatalogOnce(loadModule: LoadModule, timeoutMs: number): Promise { + const mod = await withTimeout( + loadModule('@datadog/action-catalog/action-execution'), + timeoutMs, + '@datadog/action-catalog/action-execution', + ); const setExecuteActionImplementation = mod.setExecuteActionImplementation; if (typeof setExecuteActionImplementation !== 'function') { return; @@ -212,13 +236,14 @@ async function registerActionCatalogOnce(loadModule: LoadModule): Promise }); } -/** Mirrors `actionCatalogRegistrations` — see its doc comment for why keying on `loadModule` identity is safe across both real dev-server reuse and per-test isolation. */ +/** Mirrors `actionCatalogRegistrations` — see its doc comment for why keying on `loadModule` identity is safe across both real dev-server reuse and per-test isolation, and for why an unbounded load is treated as a rejection via `withTimeout`. */ const backendRuntimeRegistrations = new WeakMap>(); /** No-ops if @datadog/apps-backend isn't installed — re-checked on every call, uncached, so installing the package mid-session (without restarting the dev server) is picked up on the very next execution instead of staying permanently no-op. Once installed, registers ONE stable runtime Proxy for the process lifetime — every accessor call resolves whichever execution's `$` is on the AsyncLocalStorage-scoped call stack (or rejects if that execution has concluded), rather than a runtime bound to a specific execution's `$` at registration time. */ function registerBackendRuntimeIfInstalled( loadModule: LoadModule, projectRoot: string, + timeoutMs: number, ): Promise { if (!isDatadogAppsBackendInstalled(projectRoot)) { return Promise.resolve(); @@ -227,7 +252,7 @@ function registerBackendRuntimeIfInstalled( if (existing) { return existing; } - const registration = registerBackendRuntimeOnce(loadModule).catch((err) => { + const registration = registerBackendRuntimeOnce(loadModule, timeoutMs).catch((err) => { backendRuntimeRegistrations.delete(loadModule); throw err; }); @@ -235,11 +260,18 @@ function registerBackendRuntimeIfInstalled( return registration; } -async function registerBackendRuntimeOnce(loadModule: LoadModule): Promise { - const [jsFunctionWithActionsModule, runtimeModule] = await Promise.all([ - loadModule('@datadog/apps-backend/runtime/jsFunctionWithActions'), - loadModule('@datadog/apps-backend/runtime'), - ]); +async function registerBackendRuntimeOnce( + loadModule: LoadModule, + timeoutMs: number, +): Promise { + const [jsFunctionWithActionsModule, runtimeModule] = await withTimeout( + Promise.all([ + loadModule('@datadog/apps-backend/runtime/jsFunctionWithActions'), + loadModule('@datadog/apps-backend/runtime'), + ]), + timeoutMs, + '@datadog/apps-backend/runtime', + ); const buildRuntimeFromJsFunctionWithActions = jsFunctionWithActionsModule.buildRuntimeFromJsFunctionWithActions; const setBackend = runtimeModule.setBackend; @@ -409,10 +441,12 @@ async function runScriptLocally( const actionCatalogRegistration = registerActionCatalogIfInstalled( loadModule, projectRoot, + timeoutMs, ); const backendRuntimeRegistration = registerBackendRuntimeIfInstalled( loadModule, projectRoot, + timeoutMs, ); await Promise.all([actionCatalogRegistration, backendRuntimeRegistration]); From bd03db49416a6bc3df7ca0e4c017ef34ae9da1df Mon Sep 17 00:00:00 2001 From: Tiffany Trinh Date: Thu, 27 Aug 2026 21:44:28 -0400 Subject: [PATCH 11/19] fix(apps): fail loudly on globalThis.$ access outside an active execution A customer module's own top-level evaluation runs before this execution's box exists, and previously fell back to a plain undefined read instead of failing the way a real Datadog deployment does at that same point. Also reinstalls the accessor if a prior execution's customer code deleted globalThis.$, so that deletion doesn't permanently break every later execution in the same dev-server process. --- .../apps/src/vite/local-execution.test.ts | 173 +++++++++++++++++- .../plugins/apps/src/vite/local-execution.ts | 89 +++++++-- 2 files changed, 238 insertions(+), 24 deletions(-) diff --git a/packages/plugins/apps/src/vite/local-execution.test.ts b/packages/plugins/apps/src/vite/local-execution.test.ts index fc740d130..058c872ec 100644 --- a/packages/plugins/apps/src/vite/local-execution.test.ts +++ b/packages/plugins/apps/src/vite/local-execution.test.ts @@ -105,12 +105,18 @@ describe('local-execution — executeScriptLocally', () => { ).rejects.toThrow(`"example" is not a function exported from ${func.absolutePath}`); }); - test('Should load and evaluate the customer module before installing globalThis.$, matching production module-evaluation order', async () => { - let dollarDuringModuleLoad: unknown = 'not captured'; + test('Should throw when a customer module reaches for $ during its own top-level evaluation, matching production module-evaluation order', async () => { + let dollarAccessError: unknown = 'not captured'; const loadModule: LoadModule = async (specifier) => { if (specifier === func.absolutePath + LOCAL_EXECUTION_LOAD_SUFFIX) { - // Captures globalThis.$ at module-evaluation time — production's static import runs before its wrapper installs $, so code reaching for $ during top-level evaluation must see the same absence locally. - dollarDuringModuleLoad = (globalThis as Record).$; + // Production's static customer-module import runs before its wrapper installs $, so a + // customer module reaching for $ during its own top-level evaluation fails there too — + // this must fail the same way locally instead of silently resolving to undefined. + try { + dollarAccessError = (globalThis as Record).$; + } catch (error) { + dollarAccessError = error; + } return { example: () => 'done' }; } const notFoundError: NodeJS.ErrnoException = new Error( @@ -130,7 +136,125 @@ describe('local-execution — executeScriptLocally', () => { ); expect(result).toEqual({ data: 'done' }); - expect(dollarDuringModuleLoad).toBeUndefined(); + expect(dollarAccessError).toBeInstanceOf(Error); + expect((dollarAccessError as Error).message).toBe( + 'No active local execution to resolve $ under.', + ); + }); + + test("Should return a pre-existing globalThis.$ during a customer module's top-level evaluation when something (e.g. zx/globals) seeded it before this module loaded", async () => { + const originalDescriptor = Object.getOwnPropertyDescriptor(globalThis, '$'); + const preExisting = { fromZxGlobals: true }; + (globalThis as Record).$ = preExisting; + let isolatedExecuteScriptLocally!: typeof executeScriptLocally; + try { + jest.isolateModules(() => { + // A fresh module instance re-runs its top-level Reflect.has check with preExisting + // already in place, capturing hadPreexistingDollar=true — the outer instance every other + // test in this file uses was imported before any test set globalThis.$, so it can't + // exercise this path. + isolatedExecuteScriptLocally = require('./local-execution').executeScriptLocally; + }); + + let dollarDuringModuleLoad: unknown = 'not captured'; + const loadModule: LoadModule = async (specifier) => { + if (specifier === func.absolutePath + LOCAL_EXECUTION_LOAD_SUFFIX) { + dollarDuringModuleLoad = (globalThis as Record).$; + return { example: () => 'done' }; + } + throw new Error(`Cannot find module '${specifier}'`); + }; + + const result = await isolatedExecuteScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModule, + mockLogger, + ); + expect(result).toEqual({ data: 'done' }); + expect(dollarDuringModuleLoad).toBe(preExisting); + } finally { + if (originalDescriptor) { + Object.defineProperty(globalThis, '$', originalDescriptor); + } else { + delete (globalThis as Record).$; + } + } + }); + + test('Should reinstall the $ accessor if a customer execution deleted globalThis.$, so a later execution can still use it', async () => { + await executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModuleReturning({ + example: () => { + delete (globalThis as Record).$; + return 'first'; + }, + }), + mockLogger, + ); + + const second = await executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModuleReturning({ example: () => testDollar().backendFunctionArgs }), + mockLogger, + ); + expect(second).toEqual({ data: [] }); + }); + + test("Should not leak one execution's top-level zx/globals-style $ write into a later execution's own top-level load", async () => { + await executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + (async (specifier: string) => { + if (specifier === func.absolutePath + LOCAL_EXECUTION_LOAD_SUFFIX) { + // Simulates a customer module's own top-level side effect (e.g. `import 'zx/globals'`) writing $ before this execution's box exists. + (globalThis as Record).$ = { + fromFirstExecutionTopLevel: true, + }; + return { example: () => 'first' }; + } + throw new Error(`Cannot find module '${specifier}'`); + }) as LoadModule, + mockLogger, + ); + + let dollarDuringSecondLoad: unknown = 'not captured'; + let secondLoadError: unknown; + await executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + (async (specifier: string) => { + if (specifier === func.absolutePath + LOCAL_EXECUTION_LOAD_SUFFIX) { + try { + dollarDuringSecondLoad = (globalThis as Record).$; + } catch (error) { + secondLoadError = error; + } + return { example: () => 'second' }; + } + throw new Error(`Cannot find module '${specifier}'`); + }) as LoadModule, + mockLogger, + ); + + expect(dollarDuringSecondLoad).toBe('not captured'); + expect(secondLoadError).toBeInstanceOf(Error); + expect((secondLoadError as Error).message).toBe( + 'No active local execution to resolve $ under.', + ); }); test('Should reject when loadModule itself rejects, same as a native-module load failure would', async () => { @@ -1152,6 +1276,45 @@ describe('local-execution — executeScriptLocally', () => { ).rejects.toThrow(/example.*silently converts to "null"/); }); + test('Should reject a function nested inside a plain object, not just at the top level', async () => { + await expect( + executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModuleReturning({ example: () => ({ status: 'ok', callback: () => {} }) }), + mockLogger, + ), + ).rejects.toThrow(/example.*JSON.stringify silently drops/); + }); + + test('Should reject an explicit undefined nested inside a plain object, not just at the top level', async () => { + await expect( + executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModuleReturning({ example: () => ({ status: 'ok', extra: undefined }) }), + mockLogger, + ), + ).rejects.toThrow(/example.*JSON.stringify silently drops/); + }); + + test('Should reject a Symbol nested inside an array, not just at the top level', async () => { + await expect( + executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModuleReturning({ example: () => [1, Symbol('unsupported')] }), + mockLogger, + ), + ).rejects.toThrow(/example.*JSON.stringify silently drops/); + }); + test('Should allow an explicit undefined result through unchanged', async () => { const result = await executeScriptLocally( func, diff --git a/packages/plugins/apps/src/vite/local-execution.ts b/packages/plugins/apps/src/vite/local-execution.ts index 47e1157a0..84184b293 100644 --- a/packages/plugins/apps/src/vite/local-execution.ts +++ b/packages/plugins/apps/src/vite/local-execution.ts @@ -27,25 +27,63 @@ type BackendGlobalsBox = { value: unknown }; /** Scopes `globalThis.$` per execution via AsyncLocalStorage, not a plain mutable property, so a zombie execution's late "fresh" `globalThis.$` read resolves to its own `$`, never a newer execution's identity/`allowedConnectionIds`. */ const backendGlobalsContext = new AsyncLocalStorage(); -/** Backs `globalThis.$` for reads/writes that happen with no execution box on the AsyncLocalStorage-scoped call stack (e.g. this module's own import-time state) — an ordinary mutable slot, since there's no per-execution box to isolate it into. Seeded from any `$` already installed before this module loaded (e.g. `zx/globals`, which assigns `globalThis.$` at its own import time), so installing the accessor below doesn't silently discard it. */ +/** Whether something (e.g. `zx/globals`, which assigns `globalThis.$` at its own import time) installed `$` before this module's own accessor below — distinguishes that legitimate passthrough from a customer module reaching for `$` during its own top-level evaluation, which has no such prior value and should fail the same way production does. */ +const hadPreexistingDollar = Reflect.has(globalThis, '$'); + +/** Marks specifically the window where a customer module's own top-level code (import-time side effects, evaluated before this execution's box exists) is loading — narrower than "no box on the call stack," which is also true genuinely between executions, where the old undefined-returning fallback below is still correct. Carries its own mutable box (not just a boolean marker) so a top-level write during this window — e.g. `zx/globals`, which assigns `globalThis.$` at its own import time — lands in a box scoped to *this* module's own load, not the shared `globalDollarOutsideExecution` slot a later, unrelated execution's own top-level load would also read from. */ +const customerModuleLoadContext = new AsyncLocalStorage<{ assigned: boolean; value: unknown }>(); + +/** Backs `globalThis.$` for reads/writes that happen with no execution box on the AsyncLocalStorage-scoped call stack (e.g. this module's own import-time state) — an ordinary mutable slot, since there's no per-execution box to isolate it into. Seeded from any `$` already installed before this module loaded, so installing the accessor below doesn't silently discard a legitimate `zx/globals`-style passthrough. */ let globalDollarOutsideExecution: unknown = Reflect.get(globalThis, '$'); -Object.defineProperty(globalThis, '$', { - configurable: true, - enumerable: true, - get: () => { - const box = backendGlobalsContext.getStore(); - return box ? box.value : globalDollarOutsideExecution; - }, - set: (value: unknown) => { - const box = backendGlobalsContext.getStore(); - if (box) { - box.value = value; - } else { - globalDollarOutsideExecution = value; +function ensureDollarAccessorInstalled(): void { + if (Object.getOwnPropertyDescriptor(globalThis, '$')?.get === dollarGetter) { + return; + } + Object.defineProperty(globalThis, '$', { + configurable: true, + enumerable: true, + get: dollarGetter, + set: dollarSetter, + }); +} + +function dollarGetter(): unknown { + const box = backendGlobalsContext.getStore(); + if (box) { + return box.value; + } + const loadBox = customerModuleLoadContext.getStore(); + if (loadBox) { + if (loadBox.assigned) { + return loadBox.value; + } + if (hadPreexistingDollar) { + return globalDollarOutsideExecution; } - }, -}); + // Matches production: a customer module's own top-level evaluation runs before production installs $, so referencing it fails loudly there too, instead of silently resolving to undefined. + throw new Error('No active local execution to resolve $ under.'); + } + return globalDollarOutsideExecution; +} + +function dollarSetter(value: unknown): void { + const box = backendGlobalsContext.getStore(); + if (box) { + box.value = value; + return; + } + const loadBox = customerModuleLoadContext.getStore(); + if (loadBox) { + // Scoped to this one module load, not the shared globalDollarOutsideExecution slot — otherwise a customer module's own top-level write (e.g. zx/globals) would leak into every later, unrelated execution's own top-level load instead of staying local to this one. + loadBox.assigned = true; + loadBox.value = value; + return; + } + globalDollarOutsideExecution = value; +} + +ensureDollarAccessorInstalled(); /** What the stable, once-ever-registered action-catalog/apps-backend adapters (below) need to dispatch a typed-wrapper call to the execution that's actually on the AsyncLocalStorage-scoped call stack — kept out of `BackendGlobals` since that object is also `globalThis.$`, directly visible to customer code. */ type ExecutionDispatch = { @@ -328,7 +366,7 @@ class UnsupportedJsonValueError extends Error {} function assertJsonSerializable(result: unknown, func: BackendFunction): unknown { let serialized: string | undefined; try { - // A replacer runs on every key/value pair JSON.stringify visits, root included, so a Map/Set/non-finite number nested arbitrarily deep inside the result (e.g. `{ data: new Map() }`) is caught the same way a top-level one is — JSON.stringify would otherwise silently flatten either to "{}" or "null" instead of throwing. + // A replacer runs on every key/value pair JSON.stringify visits, root included, so a Map/Set/non-finite number/function/Symbol/undefined nested arbitrarily deep inside the result (e.g. `{ data: new Map() }` or `{ status: 'ok', callback: () => {} }`) is caught the same way a top-level one is — JSON.stringify would otherwise silently flatten, convert, omit, or null out the offending value instead of throwing. The root call (`key === ''`) is excluded from the function/Symbol/undefined check below since a root result of exactly one of those types is a distinct, allowed case handled after this call via the `serialized === undefined` branch. serialized = JSON.stringify(result, (key, value) => { if (value instanceof Map || value instanceof Set) { throw new UnsupportedJsonValueError( @@ -340,6 +378,14 @@ function assertJsonSerializable(result: unknown, func: BackendFunction): unknown `Local execution of "${func.name}" returned ${value}${key ? ` (at "${key}")` : ''}, which JSON.stringify silently converts to "null" instead of throwing — return a finite number instead.`, ); } + if ( + key !== '' && + (typeof value === 'function' || typeof value === 'symbol' || value === undefined) + ) { + throw new UnsupportedJsonValueError( + `Local execution of "${func.name}" returned a ${typeof value} (at "${key}"), which JSON.stringify silently drops instead of serializing — return a plain JSON-compatible value instead.`, + ); + } return value; }); } catch (err) { @@ -426,13 +472,18 @@ async function runScriptLocally( }; const run = async (): Promise => { - // Loads the customer module before installing $ and the SDK bridges, matching production's import order (backend/virtual-entry.ts) — code reaching for $ during top-level evaluation fails the same way locally as in Datadog, instead of succeeding early. - const mod = await loadModule(func.absolutePath + LOCAL_EXECUTION_LOAD_SUFFIX); + // Loads and evaluates the customer's module BEFORE installing $ and the SDK bridges below, matching production's own ordering (backend/virtual-entry.ts statically imports the customer module before its wrapper installs $ and the SDK bridges) — code that reaches for $ or a typed action during its own top-level evaluation fails the same way locally as it would in Datadog, instead of silently succeeding against bindings production wouldn't have installed yet. + const mod = await customerModuleLoadContext.run({ assigned: false, value: undefined }, () => + loadModule(func.absolutePath + LOCAL_EXECUTION_LOAD_SUFFIX), + ); const fn = mod[func.name]; if (typeof fn !== 'function') { throw new Error(`"${func.name}" is not a function exported from ${func.absolutePath}`); } + // Reinstalls the accessor if a prior execution's customer code deleted globalThis.$ — otherwise this execution's box below would be unreachable through globalThis.$ for its whole lifetime, not just for whichever execution did the deleting. Only closes the gap between executions: a deletion made by one execution WHILE another is still concurrently running (its fn() hasn't returned yet) can't be recovered mid-flight — there is no way to intercept a property access on a since-deleted globalThis property without wrapping the global object itself, which isn't possible for a live, already-running process. That narrower case is accepted as-is. + ensureDollarAccessorInstalled(); + // Scopes globalThis.$ and the action-catalog/apps-backend dispatch info to this call's own async continuation chain — see backendGlobalsContext's and executionDispatchContext's doc comments. return backendGlobalsContext.run({ value: $ }, () => executionDispatchContext.run(dispatch, async () => { From e10db5dc38ce066dc16f5a225ee6049d307f466c Mon Sep 17 00:00:00 2001 From: Tiffany Trinh Date: Fri, 28 Aug 2026 01:36:39 -0400 Subject: [PATCH 12/19] fix(apps): stop conflating a real empty-string JSON key with assertJsonSerializable's root call MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The function/Symbol/undefined-drop check was exempted from the JSON root via key === '', but a real object property can also be named the empty string ({ '': ... }) — that property silently lost its value the same way the check exists to prevent, instead of throwing. Tracked via a one-shot flag set on the replacer's first invocation instead, since JSON.stringify always visits the root first regardless of its key. Also un-inlines two loadModule/Promise.all calls passed directly into withTimeout, per the repo's no-inlined-function-call-argument convention. --- .../apps/src/vite/local-execution.test.ts | 26 +++++++++++++++++++ .../plugins/apps/src/vite/local-execution.ts | 19 +++++++++----- 2 files changed, 38 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 058c872ec..c28f3d906 100644 --- a/packages/plugins/apps/src/vite/local-execution.test.ts +++ b/packages/plugins/apps/src/vite/local-execution.test.ts @@ -1302,6 +1302,32 @@ describe('local-execution — executeScriptLocally', () => { ).rejects.toThrow(/example.*JSON.stringify silently drops/); }); + test('Should reject an explicit undefined at a property literally named the empty string, not mistake it for the JSON root', async () => { + await expect( + executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModuleReturning({ example: () => ({ '': undefined, other: 'ok' }) }), + mockLogger, + ), + ).rejects.toThrow(/example.*JSON.stringify silently drops/); + }); + + test('Should reject a function at a property literally named the empty string, not mistake it for the JSON root', async () => { + await expect( + executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModuleReturning({ example: () => ({ '': () => {}, other: 'ok' }) }), + mockLogger, + ), + ).rejects.toThrow(/example.*JSON.stringify silently drops/); + }); + test('Should reject a Symbol nested inside an array, not just at the top level', async () => { await expect( executeScriptLocally( diff --git a/packages/plugins/apps/src/vite/local-execution.ts b/packages/plugins/apps/src/vite/local-execution.ts index 84184b293..65e130870 100644 --- a/packages/plugins/apps/src/vite/local-execution.ts +++ b/packages/plugins/apps/src/vite/local-execution.ts @@ -244,8 +244,9 @@ function registerActionCatalogIfInstalled( } async function registerActionCatalogOnce(loadModule: LoadModule, timeoutMs: number): Promise { + const loadPromise = loadModule('@datadog/action-catalog/action-execution'); const mod = await withTimeout( - loadModule('@datadog/action-catalog/action-execution'), + loadPromise, timeoutMs, '@datadog/action-catalog/action-execution', ); @@ -302,11 +303,12 @@ async function registerBackendRuntimeOnce( loadModule: LoadModule, timeoutMs: number, ): Promise { + const loadPromise = Promise.all([ + loadModule('@datadog/apps-backend/runtime/jsFunctionWithActions'), + loadModule('@datadog/apps-backend/runtime'), + ]); const [jsFunctionWithActionsModule, runtimeModule] = await withTimeout( - Promise.all([ - loadModule('@datadog/apps-backend/runtime/jsFunctionWithActions'), - loadModule('@datadog/apps-backend/runtime'), - ]), + loadPromise, timeoutMs, '@datadog/apps-backend/runtime', ); @@ -366,8 +368,11 @@ class UnsupportedJsonValueError extends Error {} function assertJsonSerializable(result: unknown, func: BackendFunction): unknown { let serialized: string | undefined; try { - // A replacer runs on every key/value pair JSON.stringify visits, root included, so a Map/Set/non-finite number/function/Symbol/undefined nested arbitrarily deep inside the result (e.g. `{ data: new Map() }` or `{ status: 'ok', callback: () => {} }`) is caught the same way a top-level one is — JSON.stringify would otherwise silently flatten, convert, omit, or null out the offending value instead of throwing. The root call (`key === ''`) is excluded from the function/Symbol/undefined check below since a root result of exactly one of those types is a distinct, allowed case handled after this call via the `serialized === undefined` branch. + // A replacer runs on every key/value pair JSON.stringify visits, root included, so a Map/Set/non-finite number/function/Symbol/undefined nested arbitrarily deep inside the result (e.g. `{ data: new Map() }` or `{ status: 'ok', callback: () => {} }`) is caught the same way a top-level one is — JSON.stringify would otherwise silently flatten, convert, omit, or null out the offending value instead of throwing. The root call is excluded from the function/Symbol/undefined check below since a root result of exactly one of those types is a distinct, allowed case handled after this call via the `serialized === undefined` branch. Tracked via a one-shot flag rather than `key === ''`, since a real property can also be named the empty string (`{ '': ... }`) and isn't the root. + let isRootCall = true; serialized = JSON.stringify(result, (key, value) => { + const wasRootCall = isRootCall; + isRootCall = false; if (value instanceof Map || value instanceof Set) { throw new UnsupportedJsonValueError( `Local execution of "${func.name}" returned a ${value.constructor.name}${key ? ` (at "${key}")` : ''}, which JSON.stringify silently flattens to "{}" instead of serializing its entries — return a plain array or object instead.`, @@ -379,7 +384,7 @@ function assertJsonSerializable(result: unknown, func: BackendFunction): unknown ); } if ( - key !== '' && + !wasRootCall && (typeof value === 'function' || typeof value === 'symbol' || value === undefined) ) { throw new UnsupportedJsonValueError( From 563c0500e45c60a52267d8e78c21a9f7fe8afe88 Mon Sep 17 00:00:00 2001 From: Tiffany Trinh Date: Fri, 28 Aug 2026 03:16:14 -0400 Subject: [PATCH 13/19] fix(apps): correct stale concurrency-safety comments on enqueue serialization and the epoch guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two doc comments described mechanisms that no longer match the code: enqueue's own comment blamed a "shared module-level setter a concurrent execution would clobber," but action-catalog/apps-backend registration is now WeakMap-guarded and idempotent, so no concurrent execution clobbers it — the real hazard enqueue guards against is a customer function deleting globalThis.$ while another execution is still mid-flight. Separately, the epoch guard's own comment framed it as a "belt-and-suspenders backstop" redundant with enqueue's serialization, when it's actually the only thing rejecting a timed-out execution's late dispatch during the overlap window enqueue deliberately permits (the queue advances on timeout while the abandoned fn() keeps running). Also replaces two `as Error` casts in local-execution.test.ts with a narrowing assertion helper, and fixes a UK spelling ("cancelled"). --- .../apps/src/vite/local-execution.test.ts | 17 +++++++++++------ .../plugins/apps/src/vite/local-execution.ts | 6 +++--- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/packages/plugins/apps/src/vite/local-execution.test.ts b/packages/plugins/apps/src/vite/local-execution.test.ts index c28f3d906..0113af7e2 100644 --- a/packages/plugins/apps/src/vite/local-execution.test.ts +++ b/packages/plugins/apps/src/vite/local-execution.test.ts @@ -36,6 +36,13 @@ function testDollar(): TestGlobalDollar { return (globalThis as unknown as { $: TestGlobalDollar }).$; } +/** Narrows a caught `unknown` to `Error` without an `as` cast — pairs with a preceding `expect(value).toBeInstanceOf(Error)` so the failure is reported there rather than as a thrown TypeError, and avoids `eslint-plugin-jest`'s no-conditional-expect rule that a plain `if (value instanceof Error)` guard around a second `expect(...)` would trip. */ +function assertIsError(value: unknown): asserts value is Error { + if (!(value instanceof Error)) { + throw new Error(`Expected an Error, got: ${String(value)}`); + } +} + beforeEach(() => { // Neither optional SDK is installed by default; tests exercising the "installed" path override this. jest.spyOn(shared, 'isActionCatalogInstalled').mockReturnValue(false); @@ -137,9 +144,8 @@ describe('local-execution — executeScriptLocally', () => { expect(result).toEqual({ data: 'done' }); expect(dollarAccessError).toBeInstanceOf(Error); - expect((dollarAccessError as Error).message).toBe( - 'No active local execution to resolve $ under.', - ); + assertIsError(dollarAccessError); + expect(dollarAccessError.message).toBe('No active local execution to resolve $ under.'); }); test("Should return a pre-existing globalThis.$ during a customer module's top-level evaluation when something (e.g. zx/globals) seeded it before this module loaded", async () => { @@ -252,9 +258,8 @@ describe('local-execution — executeScriptLocally', () => { expect(dollarDuringSecondLoad).toBe('not captured'); expect(secondLoadError).toBeInstanceOf(Error); - expect((secondLoadError as Error).message).toBe( - 'No active local execution to resolve $ under.', - ); + assertIsError(secondLoadError); + expect(secondLoadError.message).toBe('No active local execution to resolve $ under.'); }); test('Should reject when loadModule itself rejects, same as a native-module load failure would', async () => { diff --git a/packages/plugins/apps/src/vite/local-execution.ts b/packages/plugins/apps/src/vite/local-execution.ts index 65e130870..58fad1a2e 100644 --- a/packages/plugins/apps/src/vite/local-execution.ts +++ b/packages/plugins/apps/src/vite/local-execution.ts @@ -154,7 +154,7 @@ function validateActionCall( return { inputs, connectionId }; } -/** Local executions are serialized since action-catalog/apps-backend register runtime context via a shared, module-level setter a concurrent execution would clobber, silently redirecting the first's in-flight calls to the wrong identity. */ +/** Local executions are serialized since a customer function deleting `globalThis.$` (see `ensureDollarAccessorInstalled`'s own doc comment) would otherwise break `$` access for any other execution concurrently mid-flight, with no way to recover until that other execution's own next run reinstalls the accessor. */ let queueTail: Promise = Promise.resolve(); function enqueue(run: () => Promise): Promise { @@ -166,7 +166,7 @@ function enqueue(run: () => Promise): Promise { return result; } -/** One shared guard across all local executions — `enqueue` already serializes them, so starting a new scope always supersedes the previous one only after it has already concluded, but the guard's own generation counter is a belt-and-suspenders backstop if that invariant is ever violated. */ +/** One shared guard across all local executions — `enqueue` only serializes each execution's *start*, not its full lifetime: a timed-out execution's `fn()` keeps running in the background (see the "abandoned, not canceled" comment below) while the queue advances and a new execution starts, so the two genuinely overlap. This guard's generation counter is what rejects the abandoned execution's late `$.Actions`/adapter dispatch during that overlap window, not a redundant backstop for something serialization already prevents. */ const executionEpoch = createEpochGuard(); /** Resolves a nested property path (e.g. $.Actions.slack.chat.postMessage) to a callable that invokes `executeAction` directly — no IPC needed since there's no separate process to cross. */ @@ -442,7 +442,7 @@ async function runScriptLocally( // Never log the args themselves — they may carry secrets/PII, matching dev-server.ts's cloud path. log.debug(`Executing "${func.name}" in-process with args`); - // A timed-out execution is abandoned, not cancelled — its fn() may keep running and must not act under a newer execution's identity. The scope's isCurrent() is checked both directly (this execution's own captured `$.Actions` closure) and via `executionDispatchContext` (the stable, shared action-catalog/apps-backend adapters resolve the CALLING execution's own dispatch info from AsyncLocalStorage at call time, so a zombie's call can never be serviced by whichever execution's registration happens to be live). + // A timed-out execution is abandoned, not canceled — its fn() may keep running and must not act under a newer execution's identity. The scope's isCurrent() is checked both directly (this execution's own captured `$.Actions` closure) and via `executionDispatchContext` (the stable, shared action-catalog/apps-backend adapters resolve the CALLING execution's own dispatch info from AsyncLocalStorage at call time, so a zombie's call can never be serviced by whichever execution's registration happens to be live). const scope = executionEpoch.start(); const guardedExecuteAction: ExecuteAction = (fqn, inputs, connectionId) => { From d7004e2c5b3ddb3160bb27e3a71bd2afbb804e8d Mon Sep 17 00:00:00 2001 From: Tiffany Trinh Date: Fri, 28 Aug 2026 03:59:25 -0400 Subject: [PATCH 14/19] style(apps): tighten comment prose in local-execution.ts and execution-epoch.ts Comments added by this branch had grown into multi-sentence paragraphs restating the same invariant several ways; compress each to one tight sentence (two only for the few comments carrying a genuinely compound invariant) without dropping the underlying WHY. --- .../plugins/apps/src/vite/execution-epoch.ts | 2 +- .../apps/src/vite/local-execution.test.ts | 101 ++++++++---------- .../plugins/apps/src/vite/local-execution.ts | 58 +++++----- 3 files changed, 74 insertions(+), 87 deletions(-) diff --git a/packages/plugins/apps/src/vite/execution-epoch.ts b/packages/plugins/apps/src/vite/execution-epoch.ts index 0578c75c1..82d87de2b 100644 --- a/packages/plugins/apps/src/vite/execution-epoch.ts +++ b/packages/plugins/apps/src/vite/execution-epoch.ts @@ -4,7 +4,7 @@ /** Generation-counter guard so an abandoned scope's late cleanup can't touch a shared resource a newer scope now owns (used by `local-execution.ts`). */ export interface EpochScope { - /** True until a newer scope starts, or this one (or every scope) is concluded/invalidated. */ + /** True until a newer scope starts, or this scope is concluded or invalidated. */ isCurrent(): boolean; /** Marks no scope active and returns true if still current, otherwise a no-op returning false — call in a `finally` to gate cleanup on still owning the resource. */ concludeIfCurrent(): boolean; diff --git a/packages/plugins/apps/src/vite/local-execution.test.ts b/packages/plugins/apps/src/vite/local-execution.test.ts index 0113af7e2..8cb95618f 100644 --- a/packages/plugins/apps/src/vite/local-execution.test.ts +++ b/packages/plugins/apps/src/vite/local-execution.test.ts @@ -31,12 +31,12 @@ interface TestGlobalDollar { Source: { initiator: { id: string; orgId: string }; runAsUser: { id: string; orgId: string } }; } -/** Reads the `$` this module installs onto `globalThis` during an execution, from the customer-code perspective these tests simulate — genuinely untyped from TypeScript's static perspective since it's a runtime-only accessor property local-execution.ts defines via `Object.defineProperty`. Centralized here instead of repeating the same cast at each call site. */ +/** Reads the `$` local-execution.ts installs onto `globalThis` via `Object.defineProperty` — genuinely untyped, so the cast is centralized here instead of repeated at each call site. */ function testDollar(): TestGlobalDollar { return (globalThis as unknown as { $: TestGlobalDollar }).$; } -/** Narrows a caught `unknown` to `Error` without an `as` cast — pairs with a preceding `expect(value).toBeInstanceOf(Error)` so the failure is reported there rather than as a thrown TypeError, and avoids `eslint-plugin-jest`'s no-conditional-expect rule that a plain `if (value instanceof Error)` guard around a second `expect(...)` would trip. */ +/** Narrows a caught `unknown` to `Error` without an `as` cast, pairing with a preceding `toBeInstanceOf(Error)` so a mismatch is reported there rather than tripping `eslint-plugin-jest`'s no-conditional-expect rule. */ function assertIsError(value: unknown): asserts value is Error { if (!(value instanceof Error)) { throw new Error(`Expected an Error, got: ${String(value)}`); @@ -116,9 +116,7 @@ describe('local-execution — executeScriptLocally', () => { let dollarAccessError: unknown = 'not captured'; const loadModule: LoadModule = async (specifier) => { if (specifier === func.absolutePath + LOCAL_EXECUTION_LOAD_SUFFIX) { - // Production's static customer-module import runs before its wrapper installs $, so a - // customer module reaching for $ during its own top-level evaluation fails there too — - // this must fail the same way locally instead of silently resolving to undefined. + // Production's static import also runs before its wrapper installs $, so this must fail the same way locally instead of resolving to undefined. try { dollarAccessError = (globalThis as Record).$; } catch (error) { @@ -155,10 +153,7 @@ describe('local-execution — executeScriptLocally', () => { let isolatedExecuteScriptLocally!: typeof executeScriptLocally; try { jest.isolateModules(() => { - // A fresh module instance re-runs its top-level Reflect.has check with preExisting - // already in place, capturing hadPreexistingDollar=true — the outer instance every other - // test in this file uses was imported before any test set globalThis.$, so it can't - // exercise this path. + // A fresh module instance re-runs its Reflect.has check with preExisting already set; the outer instance was imported too early to exercise this path. isolatedExecuteScriptLocally = require('./local-execution').executeScriptLocally; }); @@ -217,42 +212,44 @@ describe('local-execution — executeScriptLocally', () => { }); test("Should not leak one execution's top-level zx/globals-style $ write into a later execution's own top-level load", async () => { + const firstLoadModule: LoadModule = async (specifier) => { + if (specifier === func.absolutePath + LOCAL_EXECUTION_LOAD_SUFFIX) { + // Simulates a top-level side effect (e.g. `import 'zx/globals'`) writing $ before this execution's box exists. + (globalThis as Record).$ = { + fromFirstExecutionTopLevel: true, + }; + return { example: () => 'first' }; + } + throw new Error(`Cannot find module '${specifier}'`); + }; await executeScriptLocally( func, TEST_PROJECT_ROOT, [], stubExecuteAction, - (async (specifier: string) => { - if (specifier === func.absolutePath + LOCAL_EXECUTION_LOAD_SUFFIX) { - // Simulates a customer module's own top-level side effect (e.g. `import 'zx/globals'`) writing $ before this execution's box exists. - (globalThis as Record).$ = { - fromFirstExecutionTopLevel: true, - }; - return { example: () => 'first' }; - } - throw new Error(`Cannot find module '${specifier}'`); - }) as LoadModule, + firstLoadModule, mockLogger, ); let dollarDuringSecondLoad: unknown = 'not captured'; let secondLoadError: unknown; + const secondLoadModule: LoadModule = async (specifier) => { + if (specifier === func.absolutePath + LOCAL_EXECUTION_LOAD_SUFFIX) { + try { + dollarDuringSecondLoad = (globalThis as Record).$; + } catch (error) { + secondLoadError = error; + } + return { example: () => 'second' }; + } + throw new Error(`Cannot find module '${specifier}'`); + }; await executeScriptLocally( func, TEST_PROJECT_ROOT, [], stubExecuteAction, - (async (specifier: string) => { - if (specifier === func.absolutePath + LOCAL_EXECUTION_LOAD_SUFFIX) { - try { - dollarDuringSecondLoad = (globalThis as Record).$; - } catch (error) { - secondLoadError = error; - } - return { example: () => 'second' }; - } - throw new Error(`Cannot find module '${specifier}'`); - }) as LoadModule, + secondLoadModule, mockLogger, ); @@ -263,7 +260,7 @@ describe('local-execution — executeScriptLocally', () => { }); test('Should reject when loadModule itself rejects, same as a native-module load failure would', async () => { - // Simulates a native addon failing to load at require()/import time, before the function is ever reached — not a customer function throwing. + // Simulates a native addon failing to load at import time — not a customer function throwing. const loadModule: LoadModule = async () => { throw new Error('cannot find native module'); }; @@ -304,7 +301,7 @@ describe('local-execution — executeScriptLocally', () => { }); test('Should reject with a clear error, not hang, when a customer function returns an un-invoked $.Actions reference instead of calling it', async () => { - // $.Actions.slack.chat is itself a callable Proxy; forgetting the trailing .postMessage(...) call and just returning it must not make `await fn(...args)` treat it as a thenable and hang until the timeout, nor make assertJsonSerializable's JSON.stringify probe for .toJSON() leak an unhandled rejection — it should surface the same clear, synchronous "can't be serialized" error as any other bare function result. + // Returning $.Actions.slack.chat un-invoked must not be mistaken for a thenable (hang) or leak an unhandled rejection — just the ordinary "can't be serialized" error. await expect( executeScriptLocally( func, @@ -683,9 +680,7 @@ describe('local-execution — executeScriptLocally', () => { (globalThis as Record).$ = preExisting; try { jest.isolateModules(() => { - // A fresh module instance re-runs its top-level Object.defineProperty, which must read - // the current globalThis.$ (still `preExisting`, via the outer instance's own getter) - // before replacing the descriptor with its own — not start from an empty slot. + // A fresh module instance re-runs its top-level Object.defineProperty and must read the current $ (still `preExisting`) rather than start from an empty slot. require('./local-execution'); }); expect((globalThis as Record).$).toBe(preExisting); @@ -787,7 +782,7 @@ describe('local-execution — executeScriptLocally', () => { ); expect(registeredImpl).toBeUndefined(); - // Simulates `npm install @datadog/action-catalog` without restarting the dev server — the very next execution must register it, not stay permanently skipped from the first (uncached) negative check. + // Simulates a mid-session install — the very next execution must register it, not stay skipped from the earlier uncached check. isInstalledSpy.mockReturnValue(true); await executeScriptLocally( func, @@ -827,7 +822,7 @@ describe('local-execution — executeScriptLocally', () => { ).rejects.toThrow('Unexpected token in action-catalog/action-execution'); }); - // A sibling registration genuinely failing doesn't affect the action-catalog adapter — it's stable and execution-agnostic, so a call made once no execution is active correctly rejects on its own, with no special-case coordination needed between the two registrations. + // The sibling registration failing doesn't affect this adapter — it's stable and execution-agnostic, so it rejects on its own once no execution is active. test('Should still reject a typed-wrapper call through a successfully-registered action-catalog implementation after the sibling apps-backend registration genuinely fails and the execution concludes', async () => { jest.spyOn(shared, 'isActionCatalogInstalled').mockReturnValue(true); jest.spyOn(shared, 'isDatadogAppsBackendInstalled').mockReturnValue(true); @@ -1419,7 +1414,7 @@ describe('local-execution — executeScriptLocally', () => { ]); expect([resultA, resultB]).toEqual([{ data: 'A' }, { data: 'B' }]); - const order = (globalThis as Record)[ORDER_MARKER]; + const order = (globalThis as Record)[ORDER_MARKER] as string[]; // Whichever call runs first, its start/end pair must be adjacent — a real race would interleave as [start-A, start-B, end-B, end-A]. expect(order).toEqual([ expect.stringMatching(/^start-/), @@ -1427,12 +1422,8 @@ describe('local-execution — executeScriptLocally', () => { 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), - ); + expect(order[0].slice('start-'.length)).toEqual(order[1].slice('end-'.length)); + expect(order[2].slice('start-'.length)).toEqual(order[3].slice('end-'.length)); }); function readOwnArgsAfterDelay(delayMs: number): () => Promise { @@ -1492,7 +1483,7 @@ describe('local-execution — executeScriptLocally', () => { await expect(second).resolves.toEqual({ data: 2 }); }); - // Covers the raw-$.Actions path: a captured Actions reference (e.g. const { Actions } = $) must reject once its own execution is abandoned, even after globalThis.$ is overwritten by a newer execution. + // Covers the raw-$.Actions path: a captured Actions reference must reject once abandoned, even after globalThis.$ is overwritten by a newer execution. test('Should reject a captured $.Actions reference once its own execution is abandoned, even after a newer execution has taken over', async () => { let abandonedCallOutcome: 'pending' | 'resolved' | { rejected: string } = 'pending'; @@ -1556,7 +1547,7 @@ describe('local-execution — executeScriptLocally', () => { executeAction, loadModuleReturning({ example: async () => { - // Fires ~60ms in, squarely inside funcB's in-flight window — a fresh $ read here needs AsyncLocalStorage, not the abandoned closure check, or it would resolve to funcB's $. + // Fires ~60ms in, inside funcB's in-flight window — a fresh $ read here needs AsyncLocalStorage or it would resolve to funcB's $. await new Promise((resolve) => setTimeout(resolve, 60)); const $ = testDollar(); try { @@ -1576,7 +1567,7 @@ describe('local-execution — executeScriptLocally', () => { ); await expect(abandoned).rejects.toThrow(/timed out after 20ms/); - // Starts as soon as the queue frees and stays "current" for 80ms, overlapping the zombie's 60ms wakeup; never itself calls $.Actions, so any observed call must be the zombie's. + // Stays "current" for 80ms, overlapping the zombie's 60ms wakeup; never calls $.Actions itself, so any observed call must be the zombie's. const second = executeScriptLocally( funcB, TEST_PROJECT_ROOT, @@ -1592,14 +1583,14 @@ describe('local-execution — executeScriptLocally', () => { ); await expect(second).resolves.toEqual({ data: 'second' }); - // The zombie's fresh read resolved to its OWN $ (funcA's allowedConnectionIds) — funcB's connectionId under funcA's identity is rejected before reaching executeAction. + // The zombie's fresh read resolved to its own $ (funcA's allowedConnectionIds), so funcB's connectionId is rejected before reaching executeAction. expect(zombieOutcome).toEqual({ rejected: expect.stringContaining("not in this function's allowed connections"), }); expect(executeAction).not.toHaveBeenCalled(); }); - // Action-catalog's registered dispatcher is stable and execution-agnostic — it resolves the calling execution's own dispatch from AsyncLocalStorage at call time, so a per-closure guard alone (bypassed once a newer execution re-registers) isn't what protects a stale typed-wrapper call. + // The dispatcher resolves the calling execution's dispatch from AsyncLocalStorage at call time — a per-closure guard alone would be bypassed once a newer execution re-registers. test("Should reject an abandoned execution's action-catalog typed-wrapper call, not silently run it under a newer registration", async () => { jest.spyOn(shared, 'isActionCatalogInstalled').mockReturnValue(true); let abandonedCallOutcome: 'pending' | 'resolved' | { rejected: string } = 'pending'; @@ -1651,7 +1642,7 @@ describe('local-execution — executeScriptLocally', () => { ); await expect(abandoned).rejects.toThrow(/timed out after 20ms/); - // registeredImpl still points at this (only) execution's own registration — no second execution registers here. The call is rejected because the dispatcher resolves this execution's own dispatch, already concluded by the 20ms timeout. + // No second execution registers here — the call is rejected because the dispatcher resolves this execution's own dispatch, already concluded by the 20ms timeout. await new Promise((resolve) => setTimeout(resolve, 100)); expect(abandonedCallOutcome).toEqual({ @@ -1659,7 +1650,7 @@ describe('local-execution — executeScriptLocally', () => { }); }); - // registeredImpl comes to point at funcB's own registration once it registers, but a call made from within funcA's own continuation still resolves funcA's own (concluded) dispatch via AsyncLocalStorage — it must still be rejected, not routed through funcB's identity/allowedConnectionIds just because funcB's registration is the one currently referenced. + // registeredImpl points at funcB's registration once it registers, but a call from within funcA's own continuation must still resolve funcA's concluded dispatch via AsyncLocalStorage and be rejected, not routed through funcB's identity. test("Should reject a zombie execution's action-catalog typed-wrapper call even after a newer execution has legitimately re-registered its own implementation", async () => { jest.spyOn(shared, 'isActionCatalogInstalled').mockReturnValue(true); const funcA: BackendFunction = { ...func, allowedConnectionIds: ['conn-A'] }; @@ -1692,7 +1683,7 @@ describe('local-execution — executeScriptLocally', () => { }; }; - // Times out at 20ms, then calls the typed wrapper ~60ms in — squarely inside funcB's own in-flight window (funcB registers immediately but doesn't conclude until 80ms) — using conn-B, a connection funcA itself is never allowed to use. + // Times out at 20ms, then calls the typed wrapper ~60ms in — inside funcB's in-flight window — using conn-B, a connection funcA is never allowed to use. const abandoned = executeScriptLocally( funcA, TEST_PROJECT_ROOT, @@ -1736,7 +1727,7 @@ describe('local-execution — executeScriptLocally', () => { expect(executeAction).not.toHaveBeenCalled(); }); - // The apps-backend loadModule call hangs forever here — a post-Promise.all destructuring assignment would never run, so publishing each handle via .then() as its own promise resolves is what lets the completed action-catalog registration still take effect. + // The apps-backend loadModule hangs forever, so a post-Promise.all destructuring would never run — publishing each handle via its own .then() is what lets the completed action-catalog registration still take effect. test('Should still register the action-catalog adapter even when the sibling apps-backend registration never settles, and reject a call once no execution is active', async () => { jest.spyOn(shared, 'isActionCatalogInstalled').mockReturnValue(true); jest.spyOn(shared, 'isDatadogAppsBackendInstalled').mockReturnValue(true); @@ -1787,7 +1778,7 @@ describe('local-execution — executeScriptLocally', () => { ); }); - // A real dev server reuses the same loadModule for its whole lifetime — a registration load that never settles must not permanently poison every later execution sharing it, so this deliberately reuses one loadModule across two calls instead of each test's usual per-call closure. + // Deliberately reuses one loadModule across both calls (not the usual per-call closure) — a real dev server does the same, so a load that never settles must not permanently poison later executions sharing it. test('Should let a later execution register and run after an earlier one shared the same loadModule with a registration load that never settles', async () => { jest.spyOn(shared, 'isActionCatalogInstalled').mockReturnValue(true); @@ -1903,7 +1894,7 @@ describe('local-execution — executeScriptLocally', () => { expect(registeredImpl).toBe(registeredAfterB); }); - // A's slow-to-resolve registration re-installs the same stable, execution-agnostic dispatcher B's own registration already put in place — replacing the closure instance is harmless, since either one resolves a call against whichever execution is actually on the AsyncLocalStorage-scoped call stack, not against whichever registered it. + // A's slow-to-resolve registration re-installs the same stable dispatcher B already put in place — harmless, since either closure resolves a call against whichever execution is on the AsyncLocalStorage call stack, not against whichever registered it. test("Should still dispatch correctly after a stale execution's slow-to-resolve registration re-installs the adapter following a newer execution's own registration", async () => { jest.spyOn(shared, 'isActionCatalogInstalled').mockReturnValue(true); let registeredImpl: diff --git a/packages/plugins/apps/src/vite/local-execution.ts b/packages/plugins/apps/src/vite/local-execution.ts index 58fad1a2e..51bc25345 100644 --- a/packages/plugins/apps/src/vite/local-execution.ts +++ b/packages/plugins/apps/src/vite/local-execution.ts @@ -21,19 +21,19 @@ type BackendGlobals = { Source: ReturnType; }; -/** Boxed so a customer module assigning to `globalThis.$` (e.g. importing `zx/globals`, which does exactly this) mutates only its own execution's box, never a concurrent or zombie execution's. */ +/** Boxed so a customer module assigning to `globalThis.$` (e.g. `zx/globals`) mutates only its own execution's box, never a concurrent or zombie execution's. */ type BackendGlobalsBox = { value: unknown }; -/** Scopes `globalThis.$` per execution via AsyncLocalStorage, not a plain mutable property, so a zombie execution's late "fresh" `globalThis.$` read resolves to its own `$`, never a newer execution's identity/`allowedConnectionIds`. */ +/** Scopes `globalThis.$` per execution via AsyncLocalStorage so a zombie execution's late "fresh" read resolves to its own `$`, never a newer execution's identity. */ const backendGlobalsContext = new AsyncLocalStorage(); -/** Whether something (e.g. `zx/globals`, which assigns `globalThis.$` at its own import time) installed `$` before this module's own accessor below — distinguishes that legitimate passthrough from a customer module reaching for `$` during its own top-level evaluation, which has no such prior value and should fail the same way production does. */ +/** Whether `$` was installed (e.g. by `zx/globals`) before this module's own accessor below — distinguishes that legitimate passthrough from a customer module reaching for `$` with no prior value, which should fail like production does. */ const hadPreexistingDollar = Reflect.has(globalThis, '$'); -/** Marks specifically the window where a customer module's own top-level code (import-time side effects, evaluated before this execution's box exists) is loading — narrower than "no box on the call stack," which is also true genuinely between executions, where the old undefined-returning fallback below is still correct. Carries its own mutable box (not just a boolean marker) so a top-level write during this window — e.g. `zx/globals`, which assigns `globalThis.$` at its own import time — lands in a box scoped to *this* module's own load, not the shared `globalDollarOutsideExecution` slot a later, unrelated execution's own top-level load would also read from. */ +/** Marks the window where a customer module's own top-level code is loading, narrower than "no execution box on the call stack" (also true between executions, where the undefined-returning fallback below is correct). Carries its own mutable box so a top-level `$` write (e.g. `zx/globals`) lands scoped to this module's own load, not the shared `globalDollarOutsideExecution` slot a later, unrelated load would also read from. */ const customerModuleLoadContext = new AsyncLocalStorage<{ assigned: boolean; value: unknown }>(); -/** Backs `globalThis.$` for reads/writes that happen with no execution box on the AsyncLocalStorage-scoped call stack (e.g. this module's own import-time state) — an ordinary mutable slot, since there's no per-execution box to isolate it into. Seeded from any `$` already installed before this module loaded, so installing the accessor below doesn't silently discard a legitimate `zx/globals`-style passthrough. */ +/** Backs `globalThis.$` outside any execution box (e.g. this module's own import-time state); seeded from any `$` already installed before this module loaded so the accessor below doesn't discard a legitimate `zx/globals`-style passthrough. */ let globalDollarOutsideExecution: unknown = Reflect.get(globalThis, '$'); function ensureDollarAccessorInstalled(): void { @@ -61,7 +61,7 @@ function dollarGetter(): unknown { if (hadPreexistingDollar) { return globalDollarOutsideExecution; } - // Matches production: a customer module's own top-level evaluation runs before production installs $, so referencing it fails loudly there too, instead of silently resolving to undefined. + // Matches production, where a customer module's top-level evaluation also runs before $ is installed and fails loudly rather than resolving to undefined. throw new Error('No active local execution to resolve $ under.'); } return globalDollarOutsideExecution; @@ -75,7 +75,7 @@ function dollarSetter(value: unknown): void { } const loadBox = customerModuleLoadContext.getStore(); if (loadBox) { - // Scoped to this one module load, not the shared globalDollarOutsideExecution slot — otherwise a customer module's own top-level write (e.g. zx/globals) would leak into every later, unrelated execution's own top-level load instead of staying local to this one. + // Scoped to this module load, not the shared globalDollarOutsideExecution slot — otherwise a top-level write (e.g. zx/globals) would leak into every later, unrelated load. loadBox.assigned = true; loadBox.value = value; return; @@ -85,7 +85,7 @@ function dollarSetter(value: unknown): void { ensureDollarAccessorInstalled(); -/** What the stable, once-ever-registered action-catalog/apps-backend adapters (below) need to dispatch a typed-wrapper call to the execution that's actually on the AsyncLocalStorage-scoped call stack — kept out of `BackendGlobals` since that object is also `globalThis.$`, directly visible to customer code. */ +/** What the stable, once-ever-registered adapters below need to dispatch a call to whichever execution is on the AsyncLocalStorage call stack — kept out of `BackendGlobals` since that object is also `globalThis.$`, visible to customer code. */ type ExecutionDispatch = { executeAction: ExecuteAction; allowedConnectionIds: string[]; @@ -154,7 +154,7 @@ function validateActionCall( return { inputs, connectionId }; } -/** Local executions are serialized since a customer function deleting `globalThis.$` (see `ensureDollarAccessorInstalled`'s own doc comment) would otherwise break `$` access for any other execution concurrently mid-flight, with no way to recover until that other execution's own next run reinstalls the accessor. */ +/** Serializes local executions — a customer function deleting `globalThis.$` mid-flight would otherwise break `$` access for any other execution concurrently in progress (see `ensureDollarAccessorInstalled`). */ let queueTail: Promise = Promise.resolve(); function enqueue(run: () => Promise): Promise { @@ -166,7 +166,7 @@ function enqueue(run: () => Promise): Promise { return result; } -/** One shared guard across all local executions — `enqueue` only serializes each execution's *start*, not its full lifetime: a timed-out execution's `fn()` keeps running in the background (see the "abandoned, not canceled" comment below) while the queue advances and a new execution starts, so the two genuinely overlap. This guard's generation counter is what rejects the abandoned execution's late `$.Actions`/adapter dispatch during that overlap window, not a redundant backstop for something serialization already prevents. */ +/** One shared guard across all executions — `enqueue` only serializes each execution's *start*; a timed-out `fn()` keeps running afterward (see "abandoned, not canceled" below), so this guard's generation counter is what rejects that zombie's late dispatch during the overlap, not a redundant backstop. */ const executionEpoch = createEpochGuard(); /** Resolves a nested property path (e.g. $.Actions.slack.chat.postMessage) to a callable that invokes `executeAction` directly — no IPC needed since there's no separate process to cross. */ @@ -177,7 +177,7 @@ function makeActionsProxy( ): unknown { return new Proxy(function () {}, { get(_target, prop) { - // A customer function that returns an un-invoked reference (e.g. $.Actions.foo.bar without the trailing call) must not be mistaken for a thenable or a custom-serializable object — Promise's resolution protocol probes .then(), and JSON.stringify (assertJsonSerializable) probes .toJSON(); either probe calling into the async apply() below would hang until timeout or leak an unhandled rejection instead of surfacing assertJsonSerializable's clear "can't be serialized" error. + // An un-invoked $.Actions.foo.bar reference must not be mistaken for a thenable (Promise probes .then()) or serializable (assertJsonSerializable probes .toJSON()) — either probe hitting apply() below would hang or leak a rejection instead of a clear error. if (prop === 'then' || prop === 'toJSON') { return undefined; } @@ -200,7 +200,7 @@ function makeActionsProxy( }); } -/** Bounds a registration's underlying `loadModule` call to `timeoutMs` so a load that never settles (a broken/circular module graph, not just a slow one) rejects instead of leaving its cache entry pending forever — the existing eviction-on-rejection below only fires once the promise actually settles, and an unbounded load never does. Doesn't cancel the underlying promise (not possible for a plain `Promise`), so a load that eventually does settle still runs its side effects late; see the registration functions' own doc comments for why that's harmless here. */ +/** Bounds a registration's `loadModule` call so a load that never settles (a broken/circular module graph) rejects instead of leaving its cache entry pending forever — eviction-on-rejection below only fires once a promise settles. Can't cancel the underlying promise, so a load that eventually settles still runs its side effects late; see the registration functions for why that's harmless. */ function withTimeout(promise: Promise, timeoutMs: number, what: string): Promise { return new Promise((resolve, reject) => { const timer = setTimeout(() => { @@ -219,10 +219,10 @@ function withTimeout(promise: Promise, timeoutMs: number, what: string): P }); } -/** Keyed by `loadModule` identity, not a bare module-level flag — a real dev server reuses the same Vite `ssrLoadModule` for its whole lifetime (giving true once-ever registration), while each test constructs its own `loadModule` closure (keeping tests isolated from each other's registration state). A rejection is evicted so the next execution retries, rather than permanently poisoning every later execution with one transient load failure — including a load that never settles at all, since `withTimeout` below turns that into a rejection too. */ +/** Keyed by `loadModule` identity, not a module-level flag, so a real dev server's reused `ssrLoadModule` gets true once-ever registration while each test's own closure stays isolated. A rejection (including a load `withTimeout` turns into one) is evicted so the next execution retries instead of staying permanently poisoned. */ const actionCatalogRegistrations = new WeakMap>(); -/** No-ops if @datadog/action-catalog isn't installed — re-checked on every call, uncached, so installing the package mid-session (without restarting the dev server) is picked up on the very next execution instead of staying permanently no-op. Once installed, registers ONE stable dispatcher for the process lifetime — it reads `executionDispatchContext.getStore()` at call time to resolve whichever execution is actually on the AsyncLocalStorage-scoped call stack, so a zombie execution's typed-wrapper call can never be routed through a newer execution's identity/allowedConnectionIds just because that execution's own registration is the one currently live. */ +/** No-ops if @datadog/action-catalog isn't installed — the check is re-run uncached on every call, so a mid-session install is picked up on the very next execution. Once installed, registers ONE stable dispatcher that reads `executionDispatchContext.getStore()` at call time, so a zombie's typed-wrapper call can never dispatch under a newer execution's identity just because that execution's registration is the one currently live. */ function registerActionCatalogIfInstalled( loadModule: LoadModule, projectRoot: string, @@ -275,10 +275,10 @@ async function registerActionCatalogOnce(loadModule: LoadModule, timeoutMs: numb }); } -/** Mirrors `actionCatalogRegistrations` — see its doc comment for why keying on `loadModule` identity is safe across both real dev-server reuse and per-test isolation, and for why an unbounded load is treated as a rejection via `withTimeout`. */ +/** Mirrors `actionCatalogRegistrations` — same keying and timeout-eviction rationale. */ const backendRuntimeRegistrations = new WeakMap>(); -/** No-ops if @datadog/apps-backend isn't installed — re-checked on every call, uncached, so installing the package mid-session (without restarting the dev server) is picked up on the very next execution instead of staying permanently no-op. Once installed, registers ONE stable runtime Proxy for the process lifetime — every accessor call resolves whichever execution's `$` is on the AsyncLocalStorage-scoped call stack (or rejects if that execution has concluded), rather than a runtime bound to a specific execution's `$` at registration time. */ +/** Mirrors `registerActionCatalogIfInstalled`'s no-op/re-check/once-ever-registration behavior for @datadog/apps-backend; the registered runtime Proxy resolves whichever execution's `$` is live on the AsyncLocalStorage call stack, rather than binding to one execution's `$` at registration time. */ function registerBackendRuntimeIfInstalled( loadModule: LoadModule, projectRoot: string, @@ -321,9 +321,9 @@ async function registerBackendRuntimeOnce( ) { return; } - // Built once per execution (cached by dispatch identity), not once per accessor call — dispatch.$ is fixed for its whole execution, so rebuilding on every property access wasted work without changing the result. + // Cached by dispatch identity, not rebuilt per accessor call — dispatch.$ is fixed for the whole execution. const runtimeByDispatch = new WeakMap(); - // Forwards to whatever shape the real runtime's own property has — a nested namespace (e.g. `.user.getExecutionUser()`) as well as a flat method — rather than assuming every property is itself a callable, which the real @datadog/apps-backend runtime is not. + // Forwards whatever shape the real runtime's property has (nested namespace or flat method) rather than assuming every property is callable. const backendRuntimeProxy = new Proxy( {}, { @@ -349,11 +349,7 @@ async function registerBackendRuntimeOnce( return undefined; } const value = runtime[String(prop)]; - // A flat method (e.g. .getExecutionUser()) reads its own internal state via - // `this` — returning it unbound would call it with `this` bound to this Proxy's - // empty target instead of the real runtime object. A nested namespace property - // (e.g. .user) is returned as-is; its own methods keep correct `this` since the - // real sub-object, not this proxy, is what ends up receiving the call. + // A flat method must be bound to the real runtime object, not this Proxy's empty target; a nested namespace is returned as-is since its own methods already bind correctly. return typeof value === 'function' ? value.bind(runtime) : value; }, }, @@ -361,14 +357,14 @@ async function registerBackendRuntimeOnce( setBackend(backendRuntimeProxy); } -/** Rejects a non-JSON-serializable result (circular reference/`BigInt`, a bare function/`Symbol` that `JSON.stringify` silently drops, or a `Map`/`Set` that it silently flattens to `{}` since neither exposes its entries as own enumerable properties) here with a clear error, instead of failing downstream when serialized for the HTTP response. */ -// Thrown from inside assertJsonSerializable's replacer to carry an already-specific, attributed message straight through the outer catch below, rather than being re-wrapped in its generic "can't be serialized" fallback. +/** Rejects a non-JSON-serializable result (circular reference, `BigInt`, a dropped function/`Symbol`, a `Map`/`Set` flattened to `{}`) here with a clear error, instead of failing downstream when serialized for the HTTP response. */ +// Lets the replacer's already-specific message pass through the outer catch below unwrapped, instead of being replaced by its generic fallback. class UnsupportedJsonValueError extends Error {} function assertJsonSerializable(result: unknown, func: BackendFunction): unknown { let serialized: string | undefined; try { - // A replacer runs on every key/value pair JSON.stringify visits, root included, so a Map/Set/non-finite number/function/Symbol/undefined nested arbitrarily deep inside the result (e.g. `{ data: new Map() }` or `{ status: 'ok', callback: () => {} }`) is caught the same way a top-level one is — JSON.stringify would otherwise silently flatten, convert, omit, or null out the offending value instead of throwing. The root call is excluded from the function/Symbol/undefined check below since a root result of exactly one of those types is a distinct, allowed case handled after this call via the `serialized === undefined` branch. Tracked via a one-shot flag rather than `key === ''`, since a real property can also be named the empty string (`{ '': ... }`) and isn't the root. + // A replacer visits every key/value pair including the root, so a disallowed value nested arbitrarily deep is caught the same way a top-level one is, instead of JSON.stringify silently flattening/converting/dropping it. The root is excluded from the function/Symbol/undefined check below (handled separately via `serialized === undefined`) and tracked with a one-shot flag, not `key === ''`, since a real property can itself be named `''`. let isRootCall = true; serialized = JSON.stringify(result, (key, value) => { const wasRootCall = isRootCall; @@ -442,12 +438,12 @@ async function runScriptLocally( // Never log the args themselves — they may carry secrets/PII, matching dev-server.ts's cloud path. log.debug(`Executing "${func.name}" in-process with args`); - // A timed-out execution is abandoned, not canceled — its fn() may keep running and must not act under a newer execution's identity. The scope's isCurrent() is checked both directly (this execution's own captured `$.Actions` closure) and via `executionDispatchContext` (the stable, shared action-catalog/apps-backend adapters resolve the CALLING execution's own dispatch info from AsyncLocalStorage at call time, so a zombie's call can never be serviced by whichever execution's registration happens to be live). + // A timed-out execution is abandoned, not canceled — its fn() may keep running and must not act under a newer execution's identity. isCurrent() gates both this execution's own captured `$.Actions` closure and the shared adapters, which resolve the calling execution's dispatch from AsyncLocalStorage rather than whichever registration is currently live. const scope = executionEpoch.start(); const guardedExecuteAction: ExecuteAction = (fqn, inputs, connectionId) => { if (!scope.isCurrent()) { - // A concluded execution's scope stays concluded forever, not just "not the latest," so the wording stays conclusion-neutral rather than claiming a timeout that may not have happened. + // A concluded scope stays concluded forever, not just "not the latest" — the wording stays conclusion-neutral rather than claiming a timeout that may not have happened. return Promise.reject( new Error( `Execution of "${func.name}" already concluded; refusing to run ` + @@ -486,14 +482,14 @@ async function runScriptLocally( throw new Error(`"${func.name}" is not a function exported from ${func.absolutePath}`); } - // Reinstalls the accessor if a prior execution's customer code deleted globalThis.$ — otherwise this execution's box below would be unreachable through globalThis.$ for its whole lifetime, not just for whichever execution did the deleting. Only closes the gap between executions: a deletion made by one execution WHILE another is still concurrently running (its fn() hasn't returned yet) can't be recovered mid-flight — there is no way to intercept a property access on a since-deleted globalThis property without wrapping the global object itself, which isn't possible for a live, already-running process. That narrower case is accepted as-is. + // Reinstalls the accessor if a prior execution's customer code deleted globalThis.$, so this execution's box stays reachable. Only closes the gap between executions — a deletion made mid-flight by a still-running concurrent execution can't be recovered, since there's no way to intercept access on a since-deleted global property; that narrower case is accepted as-is. ensureDollarAccessorInstalled(); - // Scopes globalThis.$ and the action-catalog/apps-backend dispatch info to this call's own async continuation chain — see backendGlobalsContext's and executionDispatchContext's doc comments. + // Scopes globalThis.$ and the dispatch info to this call's own async continuation chain. return backendGlobalsContext.run({ value: $ }, () => executionDispatchContext.run(dispatch, async () => { try { - // The action-catalog/apps-backend adapters are stable and idempotent to re-register — see their own doc comments — so no coordination is needed between the two registrations or across executions. + // Both adapters are stable and idempotent to re-register, so no coordination is needed between them or across executions. const actionCatalogRegistration = registerActionCatalogIfInstalled( loadModule, projectRoot, From 190a216df5db927adcfce5414e7a800a6e999fb1 Mon Sep 17 00:00:00 2001 From: Tiffany Trinh Date: Fri, 28 Aug 2026 14:02:23 -0400 Subject: [PATCH 15/19] fix(apps): stop throwing on typeof $ outside an execution, catch Symbol-keyed results MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit globalThis.$ isn't a property at all in production until main() assigns it, so an unresolvable $ reads as undefined per typeof's spec-defined behavior on unresolvable references — it never throws. Locally, $ is a real accessor property, so throwing from its getter broke that parity for feature-detection code like `typeof $ !== 'undefined'`. Return undefined instead when no execution or prior value has claimed $. Also close a gap in assertJsonSerializable: JSON.stringify's replacer is never invoked for a Symbol-KEYED property (only Symbol-valued ones under a string key) — such properties were silently omitted with no callback at all, defeating the "reject anything JSON.stringify would silently drop" check. Added a dedicated recursive walk for this case. --- .../apps/src/vite/local-execution.test.ts | 67 +++++++++++-------- .../plugins/apps/src/vite/local-execution.ts | 24 ++++++- 2 files changed, 61 insertions(+), 30 deletions(-) diff --git a/packages/plugins/apps/src/vite/local-execution.test.ts b/packages/plugins/apps/src/vite/local-execution.test.ts index 8cb95618f..2c6603ee0 100644 --- a/packages/plugins/apps/src/vite/local-execution.test.ts +++ b/packages/plugins/apps/src/vite/local-execution.test.ts @@ -36,13 +36,6 @@ function testDollar(): TestGlobalDollar { return (globalThis as unknown as { $: TestGlobalDollar }).$; } -/** Narrows a caught `unknown` to `Error` without an `as` cast, pairing with a preceding `toBeInstanceOf(Error)` so a mismatch is reported there rather than tripping `eslint-plugin-jest`'s no-conditional-expect rule. */ -function assertIsError(value: unknown): asserts value is Error { - if (!(value instanceof Error)) { - throw new Error(`Expected an Error, got: ${String(value)}`); - } -} - beforeEach(() => { // Neither optional SDK is installed by default; tests exercising the "installed" path override this. jest.spyOn(shared, 'isActionCatalogInstalled').mockReturnValue(false); @@ -112,16 +105,14 @@ describe('local-execution — executeScriptLocally', () => { ).rejects.toThrow(`"example" is not a function exported from ${func.absolutePath}`); }); - test('Should throw when a customer module reaches for $ during its own top-level evaluation, matching production module-evaluation order', async () => { - let dollarAccessError: unknown = 'not captured'; + test('Should read $ as undefined when a customer module reaches for it during its own top-level evaluation, matching production module-evaluation order', async () => { + let dollarDuringModuleLoad: unknown = 'not captured'; const loadModule: LoadModule = async (specifier) => { if (specifier === func.absolutePath + LOCAL_EXECUTION_LOAD_SUFFIX) { - // Production's static import also runs before its wrapper installs $, so this must fail the same way locally instead of resolving to undefined. - try { - dollarAccessError = (globalThis as Record).$; - } catch (error) { - dollarAccessError = error; - } + // Production's static import also runs before its wrapper installs $, so $ isn't a + // global property yet — reading it must resolve to undefined the same way locally, + // not throw (typeof $ never throws on an unresolvable reference in production). + dollarDuringModuleLoad = (globalThis as Record).$; return { example: () => 'done' }; } const notFoundError: NodeJS.ErrnoException = new Error( @@ -141,9 +132,7 @@ describe('local-execution — executeScriptLocally', () => { ); expect(result).toEqual({ data: 'done' }); - expect(dollarAccessError).toBeInstanceOf(Error); - assertIsError(dollarAccessError); - expect(dollarAccessError.message).toBe('No active local execution to resolve $ under.'); + expect(dollarDuringModuleLoad).toBeUndefined(); }); test("Should return a pre-existing globalThis.$ during a customer module's top-level evaluation when something (e.g. zx/globals) seeded it before this module loaded", async () => { @@ -232,14 +221,9 @@ describe('local-execution — executeScriptLocally', () => { ); let dollarDuringSecondLoad: unknown = 'not captured'; - let secondLoadError: unknown; const secondLoadModule: LoadModule = async (specifier) => { if (specifier === func.absolutePath + LOCAL_EXECUTION_LOAD_SUFFIX) { - try { - dollarDuringSecondLoad = (globalThis as Record).$; - } catch (error) { - secondLoadError = error; - } + dollarDuringSecondLoad = (globalThis as Record).$; return { example: () => 'second' }; } throw new Error(`Cannot find module '${specifier}'`); @@ -253,10 +237,7 @@ describe('local-execution — executeScriptLocally', () => { mockLogger, ); - expect(dollarDuringSecondLoad).toBe('not captured'); - expect(secondLoadError).toBeInstanceOf(Error); - assertIsError(secondLoadError); - expect(secondLoadError.message).toBe('No active local execution to resolve $ under.'); + expect(dollarDuringSecondLoad).toBeUndefined(); }); test('Should reject when loadModule itself rejects, same as a native-module load failure would', async () => { @@ -1289,6 +1270,36 @@ describe('local-execution — executeScriptLocally', () => { ).rejects.toThrow(/example.*JSON.stringify silently drops/); }); + test('Should reject a Symbol-keyed property, which JSON.stringify silently omits with no replacer call at all', async () => { + const secretSymbol = Symbol('secret'); + await expect( + executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModuleReturning({ + example: () => ({ status: 'ok', [secretSymbol]: 'leaked' }), + }), + mockLogger, + ), + ).rejects.toThrow(/example.*Symbol-keyed property/); + }); + + test('Should reject a Symbol-keyed property nested inside an array, not just at the top level', async () => { + const secretSymbol = Symbol('secret'); + await expect( + executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModuleReturning({ example: () => [{ [secretSymbol]: 'leaked' }] }), + mockLogger, + ), + ).rejects.toThrow(/example.*Symbol-keyed property/); + }); + test('Should reject an explicit undefined nested inside a plain object, not just at the top level', async () => { await expect( executeScriptLocally( diff --git a/packages/plugins/apps/src/vite/local-execution.ts b/packages/plugins/apps/src/vite/local-execution.ts index 51bc25345..408d3475e 100644 --- a/packages/plugins/apps/src/vite/local-execution.ts +++ b/packages/plugins/apps/src/vite/local-execution.ts @@ -61,8 +61,11 @@ function dollarGetter(): unknown { if (hadPreexistingDollar) { return globalDollarOutsideExecution; } - // Matches production, where a customer module's top-level evaluation also runs before $ is installed and fails loudly rather than resolving to undefined. - throw new Error('No active local execution to resolve $ under.'); + // Matches production: $ isn't a global property at all until main() assigns it, so an + // unresolvable `$` reads as undefined rather than throwing (per typeof's spec-defined + // behavior on unresolvable references) — returning undefined here keeps that true even + // though $ is a real accessor property locally, not a genuinely absent one. + return undefined; } return globalDollarOutsideExecution; } @@ -361,7 +364,24 @@ async function registerBackendRuntimeOnce( // Lets the replacer's already-specific message pass through the outer catch below unwrapped, instead of being replaced by its generic fallback. class UnsupportedJsonValueError extends Error {} +/** `JSON.stringify`'s replacer never runs for a symbol-KEYED property (only symbol-valued ones under a string key) — it silently omits them with no callback at all, so they need their own recursive check. */ +function findSymbolKeyedObject(value: unknown, visited: Set): boolean { + if (typeof value !== 'object' || value === null || visited.has(value)) { + return false; + } + if (Object.getOwnPropertySymbols(value).length > 0) { + return true; + } + visited.add(value); + return Object.values(value).some((child) => findSymbolKeyedObject(child, visited)); +} + function assertJsonSerializable(result: unknown, func: BackendFunction): unknown { + if (findSymbolKeyedObject(result, new Set())) { + throw new Error( + `Local execution of "${func.name}" returned a value with a Symbol-keyed property, which JSON.stringify silently drops instead of serializing — return a plain JSON-compatible value instead.`, + ); + } let serialized: string | undefined; try { // A replacer visits every key/value pair including the root, so a disallowed value nested arbitrarily deep is caught the same way a top-level one is, instead of JSON.stringify silently flattening/converting/dropping it. The root is excluded from the function/Symbol/undefined check below (handled separately via `serialized === undefined`) and tracked with a one-shot flag, not `key === ''`, since a real property can itself be named `''`. From 670ee45a02b288cff7a4a1e77a60d6e2e733e475 Mon Sep 17 00:00:00 2001 From: Tiffany Trinh Date: Fri, 28 Aug 2026 17:01:12 -0400 Subject: [PATCH 16/19] test(apps): cover registration-caching success path and no-false-positive timeout No existing test asserted the plain success-path dedup of the action-catalog module load, only the eviction-on-failure and mid-session-install paths. Also add coverage that a legitimate in-flight $.Actions call comfortably under timeoutMs resolves normally, since only the genuinely-hung case was previously tested. --- .../apps/src/vite/local-execution.test.ts | 64 +++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/packages/plugins/apps/src/vite/local-execution.test.ts b/packages/plugins/apps/src/vite/local-execution.test.ts index 2c6603ee0..27f5360d3 100644 --- a/packages/plugins/apps/src/vite/local-execution.test.ts +++ b/packages/plugins/apps/src/vite/local-execution.test.ts @@ -493,6 +493,29 @@ describe('local-execution — executeScriptLocally', () => { ).rejects.toThrow(/timed out after 50ms/); }); + // Proves the hang-detection timer only fires for a genuinely stuck execution, not for a legitimate in-flight $.Actions call that's still comfortably within its budget. + test('Should resolve normally when a legitimate in-flight $.Actions call finishes well within the timeout, without the hang-detection timer misfiring', async () => { + const executeAction: ExecuteAction = jest.fn( + () => new Promise((resolve) => setTimeout(() => resolve({ ok: true }), 50)), + ); + const result = await executeScriptLocally( + funcWithConnection, + TEST_PROJECT_ROOT, + [], + executeAction, + loadModuleReturning({ + example: () => + testDollar().Actions.slack.chat.postMessage({ + inputs: { text: 'hi' }, + connectionId: 'conn-1', + }), + }), + mockLogger, + 500, + ); + expect(result).toEqual({ data: { ok: true } }); + }); + // The caller already moved on after the timeout rejection above; this covers the abandoned execution's own eventual failure, which has no caller left to report it to. test('Should log a late failure from an abandoned execution instead of swallowing it silently', async () => { let rejectHung: ((error: Error) => void) | undefined; @@ -776,6 +799,47 @@ describe('local-execution — executeScriptLocally', () => { expect(registeredImpl).toBeDefined(); }); + // The happy-path counterpart to the "shared loadModule with a never-settling load" test below: proves the plain success case is deduped too, not just the failure/eviction paths. + test('Should load the action-catalog module only once across two successful executions that share the same loadModule', async () => { + jest.spyOn(shared, 'isActionCatalogInstalled').mockReturnValue(true); + let actionCatalogLoadCount = 0; + const loadModule: LoadModule = async (specifier: string) => { + if (specifier === func.absolutePath + LOCAL_EXECUTION_LOAD_SUFFIX) { + return { example: () => 'ok' }; + } + if (specifier === '@datadog/action-catalog/action-execution') { + actionCatalogLoadCount += 1; + return { setExecuteActionImplementation: () => {} }; + } + const notFoundError: NodeJS.ErrnoException = new Error( + `Cannot find module '${specifier}'`, + ); + notFoundError.code = 'MODULE_NOT_FOUND'; + throw notFoundError; + }; + + const first = await executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModule, + mockLogger, + ); + const second = await executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModule, + mockLogger, + ); + + expect(first).toEqual({ data: 'ok' }); + expect(second).toEqual({ data: 'ok' }); + expect(actionCatalogLoadCount).toBe(1); + }); + test('Should propagate a real load failure from an installed action-catalog package, not treat it as absent', async () => { jest.spyOn(shared, 'isActionCatalogInstalled').mockReturnValue(true); const loadModule: LoadModule = async (specifier: string) => { From 77d024623f11825d02d17ef6c2ac687bd8b2ba2e Mon Sep 17 00:00:00 2001 From: Tiffany Trinh Date: Fri, 28 Aug 2026 17:01:33 -0400 Subject: [PATCH 17/19] refactor(apps): remove unused EpochGuard.hasActiveScope and forceInvalidate Neither has any caller outside their own test file; local-execution.ts only ever uses start()/isCurrent()/concludeIfCurrent(). --- .../apps/src/vite/execution-epoch.test.ts | 50 +++---------------- .../plugins/apps/src/vite/execution-epoch.ts | 11 ---- 2 files changed, 8 insertions(+), 53 deletions(-) diff --git a/packages/plugins/apps/src/vite/execution-epoch.test.ts b/packages/plugins/apps/src/vite/execution-epoch.test.ts index 14935b46d..5bb180fc0 100644 --- a/packages/plugins/apps/src/vite/execution-epoch.test.ts +++ b/packages/plugins/apps/src/vite/execution-epoch.test.ts @@ -5,15 +5,6 @@ import { createEpochGuard } from '@dd/apps-plugin/vite/execution-epoch'; describe('execution-epoch — createEpochGuard', () => { - test('Should report a fresh scope as current and report no active scope before any start()', () => { - const guard = createEpochGuard(); - expect(guard.hasActiveScope()).toBe(false); - - const scope = guard.start(); - expect(scope.isCurrent()).toBe(true); - expect(guard.hasActiveScope()).toBe(true); - }); - test('Should invalidate an older scope once a newer one starts', () => { const guard = createEpochGuard(); const older = guard.start(); @@ -22,26 +13,24 @@ describe('execution-epoch — createEpochGuard', () => { const newer = guard.start(); expect(older.isCurrent()).toBe(false); expect(newer.isCurrent()).toBe(true); - expect(guard.hasActiveScope()).toBe(true); }); test('Should make concludeIfCurrent a no-op returning false for an already-superseded scope', () => { const guard = createEpochGuard(); const older = guard.start(); - guard.start(); + const newer = guard.start(); expect(older.concludeIfCurrent()).toBe(false); // The newer scope must be unaffected by the older one's no-op conclude. - expect(guard.hasActiveScope()).toBe(true); + expect(newer.isCurrent()).toBe(true); }); - test('Should conclude a still-current scope, clearing hasActiveScope', () => { + test('Should conclude a still-current scope, marking it no longer current', () => { const guard = createEpochGuard(); const scope = guard.start(); expect(scope.concludeIfCurrent()).toBe(true); expect(scope.isCurrent()).toBe(false); - expect(guard.hasActiveScope()).toBe(false); }); test('Should make a second concludeIfCurrent call on the same scope a no-op', () => { @@ -52,38 +41,15 @@ describe('execution-epoch — createEpochGuard', () => { expect(scope.concludeIfCurrent()).toBe(false); }); - test('Should invalidate the active scope and clear hasActiveScope on forceInvalidate, without starting a new one', () => { - const guard = createEpochGuard(); - const scope = guard.start(); - - guard.forceInvalidate(); - - expect(scope.isCurrent()).toBe(false); - expect(guard.hasActiveScope()).toBe(false); - }); - - test('Should make forceInvalidate followed by a fresh start() behave like an ordinary new scope', () => { - const guard = createEpochGuard(); - const abandoned = guard.start(); - guard.forceInvalidate(); - - const current = guard.start(); - - expect(abandoned.isCurrent()).toBe(false); - expect(current.isCurrent()).toBe(true); - expect(guard.hasActiveScope()).toBe(true); - - // The abandoned scope's late conclude must not corrupt the new one. - expect(abandoned.concludeIfCurrent()).toBe(false); - expect(current.isCurrent()).toBe(true); - }); - test('Should keep independently-created guards from sharing any state', () => { const guardA = createEpochGuard(); const guardB = createEpochGuard(); const scopeA = guardA.start(); - expect(guardB.hasActiveScope()).toBe(false); - expect(scopeA.isCurrent()).toBe(true); + const scopeB = guardB.start(); + + guardA.start(); + expect(scopeA.isCurrent()).toBe(false); + expect(scopeB.isCurrent()).toBe(true); }); }); diff --git a/packages/plugins/apps/src/vite/execution-epoch.ts b/packages/plugins/apps/src/vite/execution-epoch.ts index 82d87de2b..cb058117b 100644 --- a/packages/plugins/apps/src/vite/execution-epoch.ts +++ b/packages/plugins/apps/src/vite/execution-epoch.ts @@ -13,10 +13,6 @@ export interface EpochScope { export interface EpochGuard { /** Starts a new scope, superseding whichever one was previously active. */ start(): EpochScope; - /** True if some started scope hasn't yet been concluded or superseded. */ - hasActiveScope(): boolean; - /** Unconditionally invalidates the active scope without starting a new one — the backstop for a scope whose own `fn` never settles. */ - forceInvalidate(): void; } export function createEpochGuard(): EpochGuard { @@ -38,12 +34,5 @@ export function createEpochGuard(): EpochGuard { }, }; }, - hasActiveScope() { - return activeGeneration !== null; - }, - forceInvalidate() { - currentGeneration += 1; - activeGeneration = null; - }, }; } From 70831d54ed939a40be4190eed3eaaa93e7cb2eab Mon Sep 17 00:00:00 2001 From: Tiffany Trinh Date: Fri, 28 Aug 2026 18:09:46 -0400 Subject: [PATCH 18/19] refactor(apps): dedupe the action-catalog/apps-backend once-ever-registration wrapper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit registerActionCatalogIfInstalled and registerBackendRuntimeIfInstalled duplicated the identical no-op/cache-check/register-once/evict-on- rejection wrapper logic around two otherwise-unrelated registration bodies. Extracted into a shared registerOnceIfInstalled taking the installed-check, WeakMap cache, and once-fn as parameters — both call sites keep their existing signatures unchanged. --- .../plugins/apps/src/vite/local-execution.ts | 64 +++++++++++-------- 1 file changed, 39 insertions(+), 25 deletions(-) diff --git a/packages/plugins/apps/src/vite/local-execution.ts b/packages/plugins/apps/src/vite/local-execution.ts index 408d3475e..996e65f60 100644 --- a/packages/plugins/apps/src/vite/local-execution.ts +++ b/packages/plugins/apps/src/vite/local-execution.ts @@ -222,30 +222,49 @@ function withTimeout(promise: Promise, timeoutMs: number, what: string): P }); } -/** Keyed by `loadModule` identity, not a module-level flag, so a real dev server's reused `ssrLoadModule` gets true once-ever registration while each test's own closure stays isolated. A rejection (including a load `withTimeout` turns into one) is evicted so the next execution retries instead of staying permanently poisoned. */ -const actionCatalogRegistrations = new WeakMap>(); - -/** No-ops if @datadog/action-catalog isn't installed — the check is re-run uncached on every call, so a mid-session install is picked up on the very next execution. Once installed, registers ONE stable dispatcher that reads `executionDispatchContext.getStore()` at call time, so a zombie's typed-wrapper call can never dispatch under a newer execution's identity just because that execution's registration is the one currently live. */ -function registerActionCatalogIfInstalled( +/** Shared once-ever-registration wrapper for both adapters below: no-ops if uninstalled (re-checked uncached on every call, so a mid-session install is picked up on the very next execution), reuses the WeakMap-cached registration keyed by `loadModule` identity (true once-ever registration for a real dev server's reused `ssrLoadModule`, isolated per closure for each test), and evicts a rejection so the next execution retries instead of staying permanently poisoned. */ +function registerOnceIfInstalled( + isInstalled: (projectRoot: string) => boolean, + registrations: WeakMap>, + registerOnce: (loadModule: LoadModule, timeoutMs: number) => Promise, loadModule: LoadModule, projectRoot: string, timeoutMs: number, ): Promise { - if (!isActionCatalogInstalled(projectRoot)) { + if (!isInstalled(projectRoot)) { return Promise.resolve(); } - const existing = actionCatalogRegistrations.get(loadModule); + const existing = registrations.get(loadModule); if (existing) { return existing; } - const registration = registerActionCatalogOnce(loadModule, timeoutMs).catch((err) => { - actionCatalogRegistrations.delete(loadModule); + const registration = registerOnce(loadModule, timeoutMs).catch((err) => { + registrations.delete(loadModule); throw err; }); - actionCatalogRegistrations.set(loadModule, registration); + registrations.set(loadModule, registration); return registration; } +/** Keyed by `loadModule` identity — see `registerOnceIfInstalled`'s doc comment. */ +const actionCatalogRegistrations = new WeakMap>(); + +/** Registers ONE stable dispatcher for the process lifetime that reads `executionDispatchContext.getStore()` at call time, so a zombie's typed-wrapper call can never dispatch under a newer execution's identity just because that execution's registration is the one currently live. */ +function registerActionCatalogIfInstalled( + loadModule: LoadModule, + projectRoot: string, + timeoutMs: number, +): Promise { + return registerOnceIfInstalled( + isActionCatalogInstalled, + actionCatalogRegistrations, + registerActionCatalogOnce, + loadModule, + projectRoot, + timeoutMs, + ); +} + async function registerActionCatalogOnce(loadModule: LoadModule, timeoutMs: number): Promise { const loadPromise = loadModule('@datadog/action-catalog/action-execution'); const mod = await withTimeout( @@ -278,28 +297,23 @@ async function registerActionCatalogOnce(loadModule: LoadModule, timeoutMs: numb }); } -/** Mirrors `actionCatalogRegistrations` — same keying and timeout-eviction rationale. */ +/** Mirrors `actionCatalogRegistrations` — see `registerOnceIfInstalled`'s doc comment. */ const backendRuntimeRegistrations = new WeakMap>(); -/** Mirrors `registerActionCatalogIfInstalled`'s no-op/re-check/once-ever-registration behavior for @datadog/apps-backend; the registered runtime Proxy resolves whichever execution's `$` is live on the AsyncLocalStorage call stack, rather than binding to one execution's `$` at registration time. */ +/** Registers ONE stable runtime Proxy for the process lifetime that resolves whichever execution's `$` is live on the AsyncLocalStorage call stack, rather than binding to one execution's `$` at registration time. */ function registerBackendRuntimeIfInstalled( loadModule: LoadModule, projectRoot: string, timeoutMs: number, ): Promise { - if (!isDatadogAppsBackendInstalled(projectRoot)) { - return Promise.resolve(); - } - const existing = backendRuntimeRegistrations.get(loadModule); - if (existing) { - return existing; - } - const registration = registerBackendRuntimeOnce(loadModule, timeoutMs).catch((err) => { - backendRuntimeRegistrations.delete(loadModule); - throw err; - }); - backendRuntimeRegistrations.set(loadModule, registration); - return registration; + return registerOnceIfInstalled( + isDatadogAppsBackendInstalled, + backendRuntimeRegistrations, + registerBackendRuntimeOnce, + loadModule, + projectRoot, + timeoutMs, + ); } async function registerBackendRuntimeOnce( From e069aba128074d0bde233117560ae2bf47d81709 Mon Sep 17 00:00:00 2001 From: Tiffany Trinh Date: Mon, 31 Aug 2026 13:42:19 -0400 Subject: [PATCH 19/19] fix(apps): conclude execution scope on early failure, dedupe abandonment errors concludeExecution() was skipped whenever run() failed before entering its try/finally (loadModule rejecting, or the export not being a function), leaving the epoch guard's shared generation pinned to the failed scope. Wraps the whole run() body in try/finally so every exit path concludes. Extracts the three near-identical "execution already concluded" error messages (action-catalog dispatcher, apps-backend accessor, direct $.Actions call) into one abandonedExecutionError() helper, and corrects the epoch guard's doc comment to describe which mechanism actually rejects a zombie's late dispatch versus which one only guards a scope's own cleanup from clobbering a newer scope. Adds a test for the "abandoned after timing out before it could start" branch, covering the case where cumulative module-load + registration delay crosses the timeout without either step individually exceeding it. --- .../apps/src/vite/local-execution.test.ts | 42 +++++++++++ .../plugins/apps/src/vite/local-execution.ts | 72 ++++++++++--------- 2 files changed, 80 insertions(+), 34 deletions(-) diff --git a/packages/plugins/apps/src/vite/local-execution.test.ts b/packages/plugins/apps/src/vite/local-execution.test.ts index 27f5360d3..1be4470d8 100644 --- a/packages/plugins/apps/src/vite/local-execution.test.ts +++ b/packages/plugins/apps/src/vite/local-execution.test.ts @@ -2035,6 +2035,48 @@ describe('local-execution — executeScriptLocally', () => { ); }); + // Neither the module load nor the registration individually exceeds its own timeout budget, but their SUM crosses the outer timeout — proves the specific "abandoned ... before it could start" rejection fires (and is logged) for this cumulative-delay case, not just an individual step timing out. + test("Should log 'abandoned ... before it could start' when cumulative module-load + registration delay crosses the timeout, without either step individually exceeding it", async () => { + jest.spyOn(shared, 'isActionCatalogInstalled').mockReturnValue(true); + const loadModule: LoadModule = async (specifier: string) => { + if (specifier === func.absolutePath + LOCAL_EXECUTION_LOAD_SUFFIX) { + // Well under the 20ms timeout on its own. + await new Promise((resolve) => setTimeout(resolve, 12)); + return { example: () => 'should never run' }; + } + if (specifier === '@datadog/action-catalog/action-execution') { + // Also well under 20ms on its own, but by now ~24ms have elapsed since scope.start(). + await new Promise((resolve) => setTimeout(resolve, 12)); + return { setExecuteActionImplementation: () => {} }; + } + const notFoundError: NodeJS.ErrnoException = new Error( + `Cannot find module '${specifier}'`, + ); + notFoundError.code = 'MODULE_NOT_FOUND'; + throw notFoundError; + }; + + await expect( + executeScriptLocally( + func, + TEST_PROJECT_ROOT, + [], + stubExecuteAction, + loadModule, + mockLogger, + 20, + ), + ).rejects.toThrow(/timed out after 20ms/); + + // Give registration room to resolve and run()'s own rejection to be logged. + await new Promise((resolve) => setTimeout(resolve, 40)); + + expect(mockLogFn).toHaveBeenCalledWith( + expect.stringContaining('was abandoned after timing out before it could start'), + 'debug', + ); + }); + // An abandoned execution's loadModule/registration steps might still resolve after timeout — proves the customer function is never invoked once already known-stale. test('Should never invoke the customer function once already known to be abandoned before it starts', async () => { let callCount = 0; diff --git a/packages/plugins/apps/src/vite/local-execution.ts b/packages/plugins/apps/src/vite/local-execution.ts index 996e65f60..7a8dde201 100644 --- a/packages/plugins/apps/src/vite/local-execution.ts +++ b/packages/plugins/apps/src/vite/local-execution.ts @@ -169,7 +169,15 @@ function enqueue(run: () => Promise): Promise { return result; } -/** One shared guard across all executions — `enqueue` only serializes each execution's *start*; a timed-out `fn()` keeps running afterward (see "abandoned, not canceled" below), so this guard's generation counter is what rejects that zombie's late dispatch during the overlap, not a redundant backstop. */ +// Shared wording for the "no longer current" rejection at every call site that checks execution abandonment (a direct $.Actions call, the action-catalog dispatcher, and the apps-backend accessor) — a concluded scope stays concluded forever, not just "not the latest", so refusing to act under its identity applies uniformly regardless of entry point. +function abandonedExecutionError(functionName: string, refusedAction: string): Error { + return new Error( + `Execution of "${functionName}" already concluded; refusing to ${refusedAction} ` + + `as this stale execution to avoid using a newer execution's identity.`, + ); +} + +/** One shared guard across all executions — `enqueue` only serializes each execution's *start*; a timed-out `fn()` keeps running afterward (see "abandoned, not canceled" below). `isCurrent()`'s cross-scope generation comparison is what rejects that zombie's later `$.Actions` dispatch, once a newer scope has taken over. Each scope's own `concludeIfCurrent()` is a separate, narrower guard: it only clears the shared generation if THIS scope is still the one active, so a scope's delayed cleanup can never clobber a newer scope that has already superseded it. */ const executionEpoch = createEpochGuard(); /** Resolves a nested property path (e.g. $.Actions.slack.chat.postMessage) to a callable that invokes `executeAction` directly — no IPC needed since there's no separate process to cross. */ @@ -282,10 +290,7 @@ async function registerActionCatalogOnce(loadModule: LoadModule, timeoutMs: numb throw new Error(`No active local execution to run "${actionId}" under.`); } if (dispatch.isAbandoned()) { - throw new Error( - `Execution of "${dispatch.functionName}" already concluded; refusing to run ` + - `"${actionId}" as this stale execution to avoid using a newer execution's identity.`, - ); + throw abandonedExecutionError(dispatch.functionName, `run "${actionId}"`); } const call: Partial = isIndexableRecord(request) ? request : {}; const { inputs, connectionId } = validateActionCall( @@ -352,9 +357,9 @@ async function registerBackendRuntimeOnce( ); } if (dispatch.isAbandoned()) { - throw new Error( - `Execution of "${dispatch.functionName}" already concluded; ` + - `refusing to resolve a further apps-backend accessor under its identity.`, + throw abandonedExecutionError( + dispatch.functionName, + 'resolve a further apps-backend accessor', ); } let runtime = runtimeByDispatch.get(dispatch); @@ -478,12 +483,7 @@ async function runScriptLocally( const guardedExecuteAction: ExecuteAction = (fqn, inputs, connectionId) => { if (!scope.isCurrent()) { // A concluded scope stays concluded forever, not just "not the latest" — the wording stays conclusion-neutral rather than claiming a timeout that may not have happened. - return Promise.reject( - new Error( - `Execution of "${func.name}" already concluded; refusing to run ` + - `"${fqn}" as this stale execution to avoid using a newer execution's identity.`, - ), - ); + return Promise.reject(abandonedExecutionError(func.name, `run "${fqn}"`)); } return executeAction(fqn, inputs, connectionId); }; @@ -507,22 +507,26 @@ async function runScriptLocally( }; const run = async (): Promise => { - // Loads and evaluates the customer's module BEFORE installing $ and the SDK bridges below, matching production's own ordering (backend/virtual-entry.ts statically imports the customer module before its wrapper installs $ and the SDK bridges) — code that reaches for $ or a typed action during its own top-level evaluation fails the same way locally as it would in Datadog, instead of silently succeeding against bindings production wouldn't have installed yet. - const mod = await customerModuleLoadContext.run({ assigned: false, value: undefined }, () => - loadModule(func.absolutePath + LOCAL_EXECUTION_LOAD_SUFFIX), - ); - const fn = mod[func.name]; - if (typeof fn !== 'function') { - throw new Error(`"${func.name}" is not a function exported from ${func.absolutePath}`); - } + // Wraps the whole body, not just the customer-function call below, so a failure while loading/resolving the module (e.g. loadModule rejecting, or the export not being a function) also concludes the scope — otherwise the epoch guard's cross-scope supersession never runs for this execution, leaving activeGeneration pinned to it until the next start() overwrites it. + try { + // Loads and evaluates the customer's module BEFORE installing $ and the SDK bridges below, matching production's own ordering (backend/virtual-entry.ts statically imports the customer module before its wrapper installs $ and the SDK bridges) — code that reaches for $ or a typed action during its own top-level evaluation fails the same way locally as it would in Datadog, instead of silently succeeding against bindings production wouldn't have installed yet. + const mod = await customerModuleLoadContext.run( + { assigned: false, value: undefined }, + () => loadModule(func.absolutePath + LOCAL_EXECUTION_LOAD_SUFFIX), + ); + const fn = mod[func.name]; + if (typeof fn !== 'function') { + throw new Error( + `"${func.name}" is not a function exported from ${func.absolutePath}`, + ); + } - // Reinstalls the accessor if a prior execution's customer code deleted globalThis.$, so this execution's box stays reachable. Only closes the gap between executions — a deletion made mid-flight by a still-running concurrent execution can't be recovered, since there's no way to intercept access on a since-deleted global property; that narrower case is accepted as-is. - ensureDollarAccessorInstalled(); + // Reinstalls the accessor if a prior execution's customer code deleted globalThis.$, so this execution's box stays reachable. Only closes the gap between executions — a deletion made mid-flight by a still-running concurrent execution can't be recovered, since there's no way to intercept access on a since-deleted global property; that narrower case is accepted as-is. + ensureDollarAccessorInstalled(); - // Scopes globalThis.$ and the dispatch info to this call's own async continuation chain. - return backendGlobalsContext.run({ value: $ }, () => - executionDispatchContext.run(dispatch, async () => { - try { + // Scopes globalThis.$ and the dispatch info to this call's own async continuation chain. + return await backendGlobalsContext.run({ value: $ }, () => + executionDispatchContext.run(dispatch, async () => { // Both adapters are stable and idempotent to re-register, so no coordination is needed between them or across executions. const actionCatalogRegistration = registerActionCatalogIfInstalled( loadModule, @@ -544,12 +548,12 @@ async function runScriptLocally( } const result = await fn(...args); return { data: assertJsonSerializable(result, func) }; - } finally { - // However this execution ends, mark it concluded so any further dispatch through it — direct or via the shared adapters — is rejected. - concludeExecution(); - } - }), - ); + }), + ); + } finally { + // However this execution ends, mark it concluded so any further dispatch through it — direct or via the shared adapters — is rejected. + concludeExecution(); + } }; let timer: ReturnType | undefined;