[APPS-2792] Add: in-process local execution for backend functions - #479
[APPS-2792] Add: in-process local execution for backend functions#479tyffical wants to merge 4 commits into
Conversation
98ded08 to
7b74053
Compare
Runs the readOwnArgsAfterDelay concurrency check through the real, serialized executeScriptLocally entrypoint (its test.skip counterpart against PR #479's un-serialized base fails with cross-contaminated args). Passing here confirms the enqueue/queueTail promise-chain mutex actually closes the globalThis.$ race, not just reorders interleaved work.
🎉 All green!🧪 All tests passed 🔗 Commit SHA: 0eb11e8 | Docs | View more details | Give us feedback! |
Runs the readOwnArgsAfterDelay concurrency check through the real, serialized executeScriptLocally entrypoint (its test.skip counterpart against PR #479's un-serialized base fails with cross-contaminated args). Passing here confirms the enqueue/queueTail promise-chain mutex actually closes the globalThis.$ race, not just reorders interleaved work.
Executes a backend function by importing its real *.backend.ts file directly (via an injected loadModule), inside the Vite dev server's own process -- no bundling, no forked child process. The dev server is already the isolation boundary from production, so a crash or hang here only affects the developer's own dev server; process-level isolation is deliberately not added preemptively. $.Actions calls resolve through a Proxy (ported from the render.ts $.Actions logic) that invokes an injected executeAction function directly -- no IPC needed, since there's no separate process to cross. The same executeAction backs an action-catalog typed-wrapper registration and an apps-backend runtime-context registration, both loaded through the same loadModule (so they resolve against the customer's own project, not build-plugins' dependency tree) mirroring what the removed generated wrapper module used to do textually. The remote call itself is still a stub pending the single-action execution endpoint. The $ context passed to the customer's module -- exposed via globalThis.$, since the customer's own function takes its own real arguments rather than a $ parameter -- carries only backendFunctionArgs, Actions, and Source, verified by test, so that once a real auth token is wired in for real action execution, it can live in a module-private closure the customer's imported code has no way to reach.
executeScriptLocally writes globalThis.$ synchronously on every call, which two concurrent calls can race on -- the existing concurrency test never read globalThis.$ from either customer function, so it couldn't catch this. Add a test that does, confirming the race is real in this un-serialized base; skipped here since the fix (serializing executions via a promise-chain queue) lands in the Hardening milestone stacked on this PR. Co-Authored-By: Claude <noreply@anthropic.com>
046ca9a to
2f10d6a
Compare
Runs the readOwnArgsAfterDelay concurrency check through the real, serialized executeScriptLocally entrypoint (its test.skip counterpart against PR #479's un-serialized base fails with cross-contaminated args). Passing here confirms the enqueue/queueTail promise-chain mutex actually closes the globalThis.$ race, not just reorders interleaved work.
There was a problem hiding this comment.
Pull request overview
Friend, this PR adds the initial in-process backend-function execution mechanism for Vite local development.
Changes:
- Directly loads and invokes backend modules.
- Provides
globalThis.$and SDK runtime registration. - Adds execution, action-routing, timeout, and concurrency tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
local-execution.ts |
Implements local execution and action proxies. |
local-execution.test.ts |
Tests execution behavior and known race conditions. |
Suppressed comments (2)
packages/plugins/apps/src/vite/local-execution.ts:198
- The intended
server.ssrLoadModuleimplementation will not load the real backend export here. The Apps Vite transform matches every.backend.tsID and replaces it with the frontend RPC proxy (vite/index.ts:121-154), whose function callsglobalThis.DD_APPS_RUNTIME(backend/proxy-codegen.ts:25-33). Local execution therefore invokes the proxy instead of the customer's function. Mark this load with a distinct query suffix and make the transform bypass proxy generation for that marker; cover it through the real Vite transform pipeline rather than only injected module doubles.
const mod = await loadModule(func.absolutePath);
packages/plugins/apps/src/vite/local-execution.ts:218
Promise.raceonly stops waiting; it does not stoprun(). After this timeout rejects, customer code continues in the dev-server process and can later call the injectedexecuteAction, potentially after another execution has replaced the global SDK registrations. Track the execution as abandoned and reject all subsequent raw and action-catalog action calls (in addition to serializing executions), so timed-out work cannot perform authenticated side effects.
try {
return await Promise.race([run(), timeout]);
} finally {
clearTimeout(timer);
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f10d6ad88
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try { | ||
| return await Promise.race([run(), timeout]); |
There was a problem hiding this comment.
When an asynchronous backend function exceeds the timeout but later resumes, Promise.race rejects only the caller-facing promise and leaves run() executing. That function can subsequently invoke real $.Actions side effects after the client has already received a timeout, and a retry can duplicate those effects; the timeout needs cancellation/isolation semantics, or it must not claim the execution has ended.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is the intentional "abandoned, not cancelled" model for v1 — documented in the RFC's Decisions and Trade-Offs and the Kickoff doc's own accepted-limitations list (real cancellation would need an AbortSignal threaded through code we don't control, or re-architecting onto a killable worker thread — bigger than this milestone's scope). Within that model, side effects from a stale execution are the real risk, not the wasted CPU alone — that's closed via an epoch-gated poisoning mechanism in the stacked Hardening PR (#480): the shared action-catalog/apps-backend registrations get proactively replaced with a rejecting stub once an execution concludes (including via timeout, gated so it can't clobber a newer execution's own valid registration). See #480's own PR description for the full mechanism. I'm also adding, in this same review pass, an explicit early-return so a detected-abandoned run skips invoking the customer function at all rather than continuing pointlessly once we already know it's stale.
There was a problem hiding this comment.
Agreed this is real, and it's the root of a limitation the rest of this PR stack already builds mitigations around, rather than something fixable in isolation here: this execution model abandons a timed-out function rather than cancelling it (there's no real cancellation primitive for arbitrary third-party async JS without either AbortController cooperation from every dependency, which can't be assumed, or full OS-process isolation, which this design doesn't use). Later PRs in this stack (#480, #484) add the epoch-gated poisoning of shared registrations and the network/subprocess guard specifically to contain the consequences of exactly this — a stale execution's late-settling code still running and potentially still calling $.Actions — as much as is practical without changing the underlying execution model.
Closing this fully would mean either real cancellation semantics or real process isolation, both larger architectural decisions than this PR. Leaving unresolved to keep it visible, and flagging it as a named limitation worth calling out explicitly in the design doc rather than silently accepted.
…led" registerActionCatalogIfInstalled/registerBackendRuntimeIfInstalled caught every loadModule failure and silently treated it as "package isn't installed" — including a real syntax/transform/evaluation error in an actually-installed package. That left the SDK unregistered with no indication anything went wrong; typed action calls would then fail later with a confusing, unrelated error instead of the real one. Only swallow errors carrying Node's own module-not-found codes now; everything else propagates.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5f0b19a7b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| registerBackendRuntimeIfInstalled(loadModule, $), | ||
| ]); | ||
|
|
||
| const mod = await loadModule(func.absolutePath); |
There was a problem hiding this comment.
When loadModule is the intended Vite server.ssrLoadModule, this absolute .backend.ts ID still passes through the apps plugin transform in vite/index.ts, whose backend-file handler replaces every export with a frontend proxy. Therefore fn is the generated globalThis.DD_APPS_RUNTIME.executeBackendFunction(...) wrapper rather than the customer's implementation, so execution either fails because that browser runtime is absent in SSR or dispatches recursively without running the backend body. The local loader needs a raw-source marker or another path that bypasses the frontend proxy transform.
Useful? React with 👍 / 👎.
| function isModuleNotFoundError(error: unknown): boolean { | ||
| const code = (error as NodeJS.ErrnoException | undefined)?.code; | ||
| return code === 'MODULE_NOT_FOUND' || code === 'ERR_MODULE_NOT_FOUND'; |
There was a problem hiding this comment.
When an installed action-catalog or apps-backend module imports a missing transitive dependency, Node reports the same MODULE_NOT_FOUND or ERR_MODULE_NOT_FOUND code used for a missing top-level package. The updated implementation provides fresh evidence that the previous issue remains because it classifies solely by that code, so this case is silently treated as an absent optional SDK and later produces an unrelated uninitialized typed-action/runtime error. Check that the resolution failure identifies the exact probed specifier, or resolve package presence separately before suppressing the load failure.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (6)
Previously missed (2) — in code that hasn't changed since the last review.
packages/plugins/apps/src/vite/local-execution.ts:187
- The repository's coding rules prohibit passing a function call directly as another call's argument. Store the constructed runtime in a named local before registering it.
This issue also appears on line 206 of the same file.
setBackend(buildRuntimeFromJsFunctionWithActions($));
packages/plugins/apps/src/vite/local-execution.ts:91
- This assertion makes untrusted runtime data satisfy
ActionCallArgswithout validation. For example,{ inputs: [], connectionId: 123 }passes the currentinputscheck and invokes anExecuteActionwhose contract requires an object input and a string connection ID. Narrow the first argument and validate both fields instead of casting it.
This issue also appears on line 147 of the same file.
const { inputs, connectionId } = (args[0] ?? {}) as Partial<ActionCallArgs>;
if (typeof inputs !== 'object' || !inputs) {
packages/plugins/apps/src/vite/local-execution.ts:206
- Logging the serialized backend arguments exposes arbitrary customer payloads—including credentials or personal data—in dev-server logs. The existing cloud path intentionally logs only that arguments were supplied (
dev-server.ts:326). Keep the function name but omit argument values here.
log.debug(`Executing "${func.name}" in-process with args=${JSON.stringify(args)}`);
packages/plugins/apps/src/vite/local-execution.ts:118
- Checking only the error code also classifies a missing transitive dependency of an installed SDK as “SDK not installed.” Node uses
MODULE_NOT_FOUND/ERR_MODULE_NOT_FOUNDin both cases, so this silently skips registration and hides the actual broken installation, contrary to the propagation behavior documented above. Confirm that the missing specifier is the optional package being probed before swallowing the error.
function isModuleNotFoundError(error: unknown): boolean {
const code = (error as NodeJS.ErrnoException | undefined)?.code;
return code === 'MODULE_NOT_FOUND' || code === 'ERR_MODULE_NOT_FOUND';
packages/plugins/apps/src/vite/local-execution.ts:211
- This reuses one mutable
LOCAL_DEV_SOURCEobject for every invocation. Customer code can mutate$.Sourceduring one execution, causing later executions—and the apps-backend runtime built from this object—to observe a modified identity. Create a fresh source object per call or expose a deeply immutable value.
Source: LOCAL_DEV_SOURCE,
packages/plugins/apps/src/vite/local-execution.ts:149
- The action-catalog path bypasses the validation applied by
makeActionsProxyand casts an arbitrary request before forwarding it. Malformed JavaScript calls can therefore send missing inputs or non-string connection IDs despite theExecuteActioncontract. Reuse one runtime validator for both call paths.
setExecuteActionImplementation(async (actionId: string, request: unknown) => {
const { inputs, connectionId } = (request ?? {}) as Partial<ActionCallArgs>;
return executeAction(actionId, inputs, connectionId);
|
@cursor review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
packages/plugins/apps/src/vite/local-execution.ts:118
MODULE_NOT_FOUNDandERR_MODULE_NOT_FOUNDcan also report a missing transitive import from an installed SDK. Since this helper does not verify which requested module is missing, both registration paths silently treat that broken installation as “not installed,” leaving the SDK unregistered and surfacing a misleading failure later. Only swallow the error when it identifies the exact SDK export being probed; rethrow missing transitive dependencies.
function isModuleNotFoundError(error: unknown): boolean {
const code = (error as NodeJS.ErrnoException | undefined)?.code;
return code === 'MODULE_NOT_FOUND' || code === 'ERR_MODULE_NOT_FOUND';
| example: () => | ||
| Object.keys(globalThis).some((k) => k.toLowerCase().includes('token')), |
Runs the readOwnArgsAfterDelay concurrency check through the real, serialized executeScriptLocally entrypoint (its test.skip counterpart against PR #479's un-serialized base fails with cross-contaminated args). Passing here confirms the enqueue/queueTail promise-chain mutex actually closes the globalThis.$ race, not just reorders interleaved work.
isModuleNotFoundError only matched Node's own MODULE_NOT_FOUND/ ERR_MODULE_NOT_FOUND codes, but loadModule here is Vite's ssrLoadModule, which throws its own ERR_LOAD_URL when a specifier can't be resolved at all — a real dev server session without @datadog/action-catalog or @datadog/apps-backend installed would hit a hard error instead of local execution silently skipping the optional registration. Caught via a downstream real end-to-end Vite server test, not a mock.
Runs the readOwnArgsAfterDelay concurrency check through the real, serialized executeScriptLocally entrypoint (its test.skip counterpart against PR #479's un-serialized base fails with cross-contaminated args). Passing here confirms the enqueue/queueTail promise-chain mutex actually closes the globalThis.$ race, not just reorders interleaved work.
Motivation
child_process.fork()-based isolation entirely: the Vite dev server is already the isolation boundary from production, so a crash or hang in a customer's own local dev server is a contained, recoverable failure, not something that needs a separate forked child process.data:URL, the dev server can just directly import the customer's real*.backend.tsfile. This PR ships that simplified design from the start, rather than shipping the bundle-based version and rewriting it later.Architecture
executeScriptLocally(local-execution.ts) introduces three collaborating pieces: an injectedloadModulestanding in forserver.ssrLoadModule, aglobalThis.$context populated once per call, and a$.ActionsProxy that turns nested property access into a singleexecuteActioncall.Changes
executeScriptLocally, which imports a backend function's real file directly via an injectedloadModule(the dev server's realserver.ssrLoadModule, or a test double) — no bundling, no wrapper module, nodata:URL.$.ActionsProxy (nested-property-path walk →{fqn, inputs, connectionId}) from the closed fork-based prototype as a direct in-process function call to an injectedExecuteAction, which now carriesconnectionIdfrom day one instead of dropping it.registerActionCatalogIfInstalled/registerBackendRuntimeIfInstalled, which register@datadog/action-catalog'ssetExecuteActionImplementationand@datadog/apps-backend'ssetBackenddirectly in TypeScript, loaded through the sameloadModule(so they resolve against the customer's own project, not build-plugins' own dependency tree) — this replaces what the removed generated wrapper module used to do textually.$context passed to the customer's module (exposed viaglobalThis.$, since the customer's real function takes its own arguments, not a$parameter) carries onlybackendFunctionArgs,Actions, andSource— verified by test — so a real auth token can later live in a module-private closure the customer's imported code has no way to reach.loadModule-result correctness,$.Actionscall resolution (includingconnectionIdforwarding) and validation, sync/async error propagation, timeout behavior, action-catalog typed-wrapper routing, and the no-token-exposure invariant.QA Instructions
yarn test:unit packages/plugins/apps/src/vite/local-execution.test.ts # Expected: Test Suites: 1 passed / Tests: 13 passed ✅ VERIFIEDyarn test:unit packages/plugins/apps # Expected: Test Suites: 23 passed / Tests: 298 passed ✅ VERIFIEDyarn workspace @dd/apps-plugin run typecheck # Expected: no output, clean exit ✅ VERIFIEDnpx eslint packages/plugins/apps/src/vite/local-execution.ts packages/plugins/apps/src/vite/local-execution.test.ts --quiet # Expected: no output, clean exit ✅ VERIFIEDNo manual local or staging QA for this PR specifically: this module isn't wired into
createDevServerMiddlewareyet, so there's nonpm run devrequest path that reachesexecuteScriptLocally()— nothing a human can click through yet, matching the same situation the original fork-based prototype (#461) was in. The tests above exercise a realloadModulecontract (the same shapeserver.ssrLoadModulefulfills), not a mocked substitute for the interesting logic. Real local + staging manual QA becomes possible once this is wired into the dev server (follow-up PR, #481).Blast Radius
Out of Scope / Follow-ups
handleExecuteAction, threading a realLoadModulefromserver.ssrLoadModule)$.ActionsexecutionExecuteActionstays a caller-supplied stub until thenDocumentation