[APPS-2792] Add: in-process local execution for backend functions - #479
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 CI checks and tests passed. 🎉 All green!🧪 All tests passed 🔗 Commit SHA: 2f45138 | 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.
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.
Confirmed — this PR's own "await Promise.race" has no cancellation/isolation semantics, exactly as described (JS can't actually cancel a settled promise's continuation). #480 (stacking on this PR) addresses this with an abandon-not-cancel model: a timed-out execution is marked abandoned and its shared action-catalog/apps-backend registrations are poisoned with rejecting stubs, so a late-resuming $.Actions call from the stale execution is rejected instead of silently running under a newer execution's identity. It doesn't literally stop the JS execution (not possible), but it does prevent the side-effect/identity-confusion risk you're flagging.
There was a problem hiding this comment.
Deferring the full fix, documenting the tradeoff instead — true cancellation of a timed-out execution requires terminating a Worker thread, which in-process execution cannot do. Added a WHY comment at the Promise.race call explaining this, and this is the reason the pending worker-pool execution model (a separate, already-planned milestone) exists: it gets real isolation/cancellation semantics that this interim model structurally cannot. Manual QA already verified the current abandon-without-crashing behavior is safe for the dev server itself; the residual risk is scoped to a local developer's own retried test run, not production.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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.
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.
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.
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.
…xecution.test.ts The actual ssrLoadModule/frontend-proxy-bypass fix this commit originally introduced now lives further down the stack (PR #479) — only the switch to the shared moduleResolverFor helper (extracted in the previous commit) remains here.
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.
fe6ccca to
7a90c5e
Compare
Executes a backend function's file directly in-process inside the Vite dev server, mirroring executeScriptViaDatadog's BackendOutputs contract as a drop-in alternate implementation for local dev.
A customer function returning $.Actions.foo.bar without calling it made the outer await treat the callable Proxy as a thenable (its get trap returned another callable Proxy for .then too), hanging until the timeout instead of just returning the value. Also converts the apply trap to async, since the manual Promise.reject/try-catch wrapping was only there to turn a synchronous throw into a rejection.
…ng it The regex re-typed BACKEND_FILE_RE's extension list and the literal suffix as an independent pattern, so a change to either one could silently stop matching real backend files without any compiler or lint signal.
Replace direct globalThis.$ assignment/deletion and Partial<ActionCallArgs> casts with helper functions and a type guard, since TypeScript can narrow these without an assertion.
…dule before installing $ The suffix alone was spoofable from frontend source (e.g. a literal ./secrets.backend.ts?dd-local-exec import); requiring SSR context too means a spoofed client-side import still gets the safe RPC-proxy stub instead of the real backend module body. Also loads and evaluates the customer module before installing $ and the SDK bridges, matching production's own ordering, so code that reaches for $ during its own top-level evaluation fails the same way locally as it would in Datadog.
… avoid unhandled rejection A spoofed client-side import falling through to buildProxyModule still carried the ?dd-local-exec suffix in its id, so BACKEND_FILE_RE (anchored to end-of-string) never stripped it — the function registered under a corrupted relativePath/query-name distinct from the file's real registration. Strips the suffix first so it dedupes onto the same entry. Also attaches a no-op catch to run()'s promise once the timeout has already settled the race, since nothing else awaits it — a customer function that rejects after its own timeout would otherwise be an unhandled rejection that crashes the whole dev server. Replaces a remaining raw as-cast in registerActionCatalogIfInstalled with the file's existing isIndexableRecord guard.
…owing it
runPromise.catch(() => {}) discarded the rejection reason from a
hung customer function once the outer timeout race already settled,
leaving the real cause of a slow failure undiagnosable.
The raw $.Actions proxy already rejects a call missing an inputs field; the action-catalog dispatcher funnels into the same executeAction but skipped this check, silently forwarding inputs: undefined instead.
…ints The raw proxy and the action-catalog dispatcher each re-implemented the same inputs/connectionId validation by hand — the exact duplication that let the action-catalog path drift out of sync and skip the inputs check in the first place. Extracted into one validateActionCall helper both entry points now call, so the two can no longer diverge. Also corrects a test comment referencing Object.defineProperty, which this file doesn't use — globalThis.$ is set via setGlobalDollar's Object.assign.
getGlobalDollar read globalThis.$ via an `as Record<string, unknown>`
cast; Reflect.get reads it without one, matching the pattern
deleteGlobalDollar already used for the same property.
The "caller had already stopped waiting" debug log fired on every
run() rejection, not just ones abandoned after the timeout race
already settled, since the .catch handler had no way to tell the two
cases apart. Gates it on whether the race has settled yet.
Also restores the BackendOutputs doc comment's explanation of why the
shape is `{ data: unknown }` (mirrors the app-builder query response),
dropped when the type was consolidated into backend/types.ts.
… filter An include filter scoped only to no-query and the exact `?dd-local-exec` suffix lets an unrecognized query (e.g. `?x`, or a malformed `?dd-local-exec&x`) bypass the transform filter entirely, so Vite falls back to its default loader instead of the safe RPC-proxy stub. Matching every query on a backend file and deciding safety in the handler closes that gap.
Locks in current behavior for two coverage gaps: depth-1 and zero-depth $.Actions proxy calls (the latter yields a trailing-dot fqn), a 5-segment deep chain, and validateActionCall accepting an array as inputs since typeof [] === 'object'.
Matches this file's own no-inlined-call-argument convention at every other call site — three spots (a nested makeActionsProxy call, a buildRuntimeFromJsFunctionWithActions/setBackend pair, and the Promise.all registration array) still inlined a call directly as another call's argument. Also tightens one comment to state the current invariant rather than narrating a past bug.
26c4e47 to
3d9cc89
Compare
The transform-object narrowing extracted into getTransformObject handles the filter/handler access without a cast; extractTransformedCode narrows the awaited transform result to its object form the same way, avoiding four `as` casts a later commit had reintroduced in a file an earlier commit already cleaned up.
3d9cc89 to
c748efd
Compare
oliverli
left a comment
oliverli
left a comment
There was a problem hiding this comment.
Consolidated review of the local-execution module and the widened transform filter. All comments verified against the PR head.
| // 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($); |
There was a problem hiding this comment.
globalThis.$ contamination under concurrent executions (P1)
globalThis.$ is a single mutable global shared across executions. When two executions overlap, execution B's setGlobalDollar($B) overwrites $A while A's customer code is mid-flight, so A reads B's backendFunctionArgs and Source — in a dev server serving concurrent browser requests (multiple tabs, HMR re-fires, parallel fetches), request A's response can contain request B's input data. The test.skip at local-execution.test.ts ("Should let each concurrent call see its OWN backendFunctionArgs...") documents this, but the damage is worse than that test suggests:
The save/restore here is only correct for strictly nested lifetimes. Concrete interleaving: A times out and keeps running (the race only stops the caller from waiting); B installs $B (capturing prev = $A), completes, restores $A; C installs $C (capturing prev = $A); abandoned A then settles, and its finally sees hadPreviousDollar = false and deletes globalThis.$ — destroying C's context mid-execution, crashing C's customer code. Late side effects of abandoned executions likewise run against whatever $ is currently installed.
Suggested fix: serialize executeScriptLocally bodies through a promise-chain mutex/queue. That is local to this module, fixes the whole class of interleavings, and lets the skipped test be un-skipped.
There was a problem hiding this comment.
Fixed downstream in #480: globalThis.$ is now scoped per execution via AsyncLocalStorage (backendGlobalsContext), and the action-catalog/apps-backend dispatchers resolve the calling execution's identity from executionDispatchContext.getStore() at call time rather than binding to whichever execution registered last — so a zombie execution can never read or dispatch under a newer execution's identity.
build-plugins/packages/plugins/apps/src/vite/local-execution.ts
Lines 27 to 28 in 9f6e809
build-plugins/packages/plugins/apps/src/vite/local-execution.ts
Lines 91 to 101 in 9f6e809
#480
| if (typeof setExecuteActionImplementation !== 'function') { | ||
| return; | ||
| } | ||
| setExecuteActionImplementation(async (actionId: string, request: unknown) => { |
There was a problem hiding this comment.
Registrations are last-write-wins and never unregistered (P2)
registerActionCatalogIfInstalled and registerBackendRuntimeIfInstalled both install a closure capturing this execution's allowedConnectionIds (here) or $ (apps-backend), and neither is ever unregistered. Two consequences:
- Concurrency: under overlapping executions this is last-write-wins. Function A (
allowedConnectionIds: []) can have its typed-wrapper call validated against function B's closure (allowedConnectionIds: ['conn-1']), so a connection A's function does not allow passesassertConnectionIdAllowedand reachesexecuteAction— silently defeating the connection-scoping guard this module's comments claim mirrors the cloud path. - Staleness: the registration persists after the execution settles. The action-catalog module retains the last closure (with the last function's
allowedConnectionIdsand the dev server'sexecuteAction) indefinitely; anything invoking the wrapper later runs with stale scoping.
The mutex from the globalThis.$ thread fixes the concurrency half. For the stale half, unregister/reset the implementation after run() settles (or guard registration with a per-execution token).
There was a problem hiding this comment.
Fixed downstream in the same #480 PR: registerActionCatalogIfInstalled/registerBackendRuntimeIfInstalled now register ONE stable dispatcher/proxy for the process lifetime that resolves the current execution from executionDispatchContext at call time, rather than binding to whichever execution's registration happened to run last — so a later registration can no longer clobber dispatch for an execution already in flight.
build-plugins/packages/plugins/apps/src/vite/local-execution.ts
Lines 261 to 288 in 9f6e809
#480
|
|
||
| /* global Proxy, globalThis */ | ||
|
|
||
| /** 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. */ |
There was a problem hiding this comment.
"Drop-in mirror of executeScriptViaDatadog" contract claim is unverifiable from this PR (P2)
The success path here returns { data: result }, matching BackendOutputs. But every failure mode throws: missing export, action-call validation, disallowed connectionId, customer sync/async throw, and timeout. No hunk in this PR touches executeScriptViaDatadog's body, so whether the cloud path also throws — or resolves error payloads inside BackendOutputs, which is common for HTTP-wrapped RPC — cannot be confirmed from the diff.
If the cloud path resolves errors rather than rejecting, the "drop-in alternate implementation" claim is wrong, and a future swap silently changes rejection semantics for callers. Either confirm the cloud path's error behavior and cite it here, or soften the claim (e.g. "returns the same BackendOutputs success shape").
There was a problem hiding this comment.
Verified accurate against the now-landed dev-server.ts (#481): executeScriptViaDatadog throws when the response is missing a data field, and pollQueryExecution throws on any API error response — both match this file's own throw-based error contract rather than returning an ambiguous value on failure.
build-plugins/packages/plugins/apps/src/vite/dev-server.ts
Lines 213 to 217 in f5d6c50
build-plugins/packages/plugins/apps/src/vite/dev-server.ts
Lines 280 to 284 in f5d6c50
|
otherwise looks good |
…export id reaches the transform handler BACKEND_FILE_WITH_QUERY_RE deliberately matches a backend file with any query, so an id that isn't the customer's own real re-transform (e.g. some other Vite load hook resolving to zero-export content) can still reach the "no exported functions" branch, which unconditionally cleared this file's registered functions. That's correct for a genuine no-query re-transform (the case HMR relies on), but wrong for a query-bearing id reaching this branch for an unrelated reason — it would silently and permanently break the file's real (unsuffixed) registration until a file edit or server restart, over an import that never touched its real source. Restricts the destructive clear (and its warning) to the exact no-query case. Vite's own ?raw/?url/?worker load hooks all produce a default export, which enumerateBackendExports already rejects with a loud throw before this branch is reached, so this covers whatever else might legitimately produce zero exports without throwing.
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: queued
mergequeue-status: in_progress
and removed
mergequeue-status: queued
labels
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: done
and removed
mergequeue-status: in_progress
labels
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 from the closed fork-based prototype (nested-property-path walk →{fqn, inputs, connectionId}) as a direct in-process call to an injectedExecuteAction.$.Actionsnow carriesconnectionIdfrom day one instead of dropping it.registerActionCatalogIfInstalled/registerBackendRuntimeIfInstalled, replacing what the removed generated wrapper module used to do via text injection.isActionCatalogInstalled/isDatadogAppsBackendInstalledchecks production's bundler path already uses, rather than catching aloadModulefailure — Vite'sssrLoadModuledoesn't guarantee a stable error code for a missing bare specifier.$context exposed to the customer's module carries onlybackendFunctionArgs,Actions, andSource(verified by test), so a real auth token can later live in a module-private closure the customer's code has no way to reach.BackendOutputs, previously declared identically in both this file anddev-server.ts, is now a single shared type inbackend/types.ts.loadModule-result correctness,$.Actionscall resolution/validation (includingconnectionIdforwarding), sync/async error propagation, timeout behavior, action-catalog typed-wrapper routing (including a real, non-"not installed" load failure), and the no-token-exposure invariant.inputsfield, same as the raw$.Actionsproxy already does — both entry points funnel into the sameexecuteActionand must reject the same malformed shape.getGlobalDollarreadsglobalThis.$viaReflect.getinstead of anascast, matching howdeleteGlobalDollaralready reads/writes the same property.makeActionsProxycall, abuildRuntimeFromJsFunctionWithActions/setBackendpair, and thePromise.allregistration array), matching this file's own convention at every other call site.ascasts a later commit had reintroduced in a file an earlier commit already cleaned up — the transform-object narrowing and a newextractTransformedCodehelper access the same fields without asserting their shape.BACKEND_FILE_REtoBACKEND_FILE_WITH_QUERY_REso a suffixed local-execution load (?dd-local-exec) still reaches the handler; when SSR-loading that suffixed id, the handler returns the real function body instead of the frontend proxy stub. Any other query-bearing id falls through to the existing proxy-stub path, normalized back to the file's real (unsuffixed) id first so it registers under the same key a real import uses.dev-server.ts's twoBackendOutputs-shaped return sites now import the shared type frombackend/types.tsinstead of redeclaring it locally.QA Instructions
yarn test:unit packages/plugins/apps/src/vite/local-execution.test.ts # Expected: Test Suites: 1 passed / Tests: 31 passed, 1 skipped ✅ VERIFIEDyarn test:unit packages/plugins/apps # Expected: Test Suites: 29 passed / Tests: 470 passed, 1 skipped ✅ 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
createDevServerMiddlewareon its own, so there's nonpm run devrequest path reachingexecuteScriptLocally()from this diff alone. The tests above exercise a realloadModulecontract (the same shapeserver.ssrLoadModulefulfills), not a mocked substitute for the interesting logic. Real local + staging manual QA covering this mechanism lives in #481, where the dev-server wiring landed.Blast Radius
executeScriptLocallyitself is net-new and not called from anywhere in the existing dev server, so it has zero effect on any currently-shipping execution path on its own.BACKEND_FILE_REtoBACKEND_FILE_WITH_QUERY_REmeans a query-bearing id that previously skipped the handler entirely now reaches it, falls through the new query-normalization branch, and produces the same proxy-stub output as before — a real change to the live filter, but exercised by existing tests and gated on the id actually carrying a query, which no current production import does.Out of Scope / Follow-ups
handleExecuteAction, threading a realLoadModulefromserver.ssrLoadModule)$.ActionsexecutionExecuteActionstays a caller-supplied stub until then$snapshot-and-restore runs after the customer module's own top-level evaluation, so a module that writes/deletesglobalThis.$during load can have that value mistaken for the pre-existing one; a throwingloadModulealso bypasses the restoringfinallyAsyncLocalStorage-scoped design that doesn't have this failure mode, rather than patching it hereDocumentation