[APPS-2792] Add: harden the in-process local execution path - #480
Conversation
|
✅ All CI checks and tests passed. 🎉 All green!🧪 All tests passed 🔗 Commit SHA: e069aba | Docs | View more details | Give us feedback! |
6e85225 to
64c7a61
Compare
64c7a61 to
41a772e
Compare
59e9b78 to
6a19936
Compare
6a19936 to
24c072f
Compare
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Pull request overview
Friend, this PR hardens in-process backend execution with serialization, stale-context guards, and JSON-result validation.
Changes:
- Serializes local executions and poisons concluded runtime registrations.
- Validates returned values for JSON serialization.
- Expands concurrency, timeout, registration, and result tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
packages/plugins/apps/src/vite/local-execution.ts |
Adds execution hardening and result validation. |
packages/plugins/apps/src/vite/local-execution.test.ts |
Adds hardening regression coverage. |
💡 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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
d2bd2a5 to
54c6843
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
ec2a07f to
10a8c9c
Compare
9c0a4ed to
acd24b1
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
packages/plugins/apps/src/vite/local-execution.ts:239
- A queued execution can inherit a doomed registration promise from the execution that just timed out. The execution timer is installed before this registration timer, so when both use
timeoutMs, the queue can start the next request before this promise rejects and the cache entry is evicted; this branch then returns the old promise and makes the new request fail without retrying its own registration. Retry registration after an inherited promise rejects (the rejection handler has evicted it), or cache only completed registrations.
const existing = actionCatalogRegistrations.get(loadModule);
if (existing) {
return existing;
packages/plugins/apps/src/vite/local-execution.ts:295
- This has the same timed-out pending-cache race as the action-catalog registration above: the next queued execution may start before the previous registration timeout evicts this promise, then immediately fail when that stale promise rejects rather than making its own load attempt. Retry after an inherited rejection or retain only successfully completed registrations.
const existing = backendRuntimeRegistrations.get(loadModule);
if (existing) {
return existing;
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: acd24b1ef7
ℹ️ 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".
| const existing = actionCatalogRegistrations.get(loadModule); | ||
| if (existing) { | ||
| return existing; |
There was a problem hiding this comment.
When execution A's SDK load hangs and execution B is already queued with the same ssrLoadModule, A's outer execution timer fires before the registration timer because the latter was created later. The queue then starts B immediately, and this branch returns A's still-cached promise; moments later that promise rejects, causing B to fail instead of retrying the load. This affects both the action-catalog cache here and the symmetric backend-runtime cache, and is observable whenever B's customer module is available before A's registration timeout callback runs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed real — verified the timing: registerActionCatalogIfInstalled/registerBackendRuntimeIfInstalled both use the calling execution's own timeoutMs for the registration's withTimeout wrapper, started a few ticks after that same execution's own outer hang-detection timer with the same duration, so the outer timer reliably fires first and lets the queue advance to a waiting execution before the (still-cached, not-yet-rejected) registration promise itself settles. Not fixing in this pass — correctly closing it means changing eviction to trigger on the outer caller's own timeout/abandonment, not just on the registration's own timer, which is a real design change beyond a mechanical fix. Filing as a documented follow-up in the PR description rather than patching it under review pressure.
Deferring specifically because likelihood and impact are both low: it requires (1) action-catalog or apps-backend actually installed, (2) this being the very first registration attempt for the dev-server process (subsequent calls hit an already-resolved, no-longer-timing-sensitive cache entry), (3) that first load hanging for the full default 10s timeout — abnormal for a local npm package resolution — and (4) a second $.Actions call landing in the few-millisecond gap between the outer timeout firing and the registration's own timeout firing shortly after. When it does hit, the failure is a clear (if slightly misattributed) timeout error, not silent data loss or a security issue, and the very next request succeeds normally since the cache entry is evicted on rejection regardless.
9ac6bad to
a2af57b
Compare
a2af57b to
c8aa219
Compare
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.
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.
…tale 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.
… to {}
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).
…negative result 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.
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.
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.
…cal-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.
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.
…ater executions 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.
…tion 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.
…onSerializable's root call
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.
…lization and the epoch guard
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").
…n-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.
…ol-keyed results 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.
…tive 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.
…lidate Neither has any caller outside their own test file; local-execution.ts only ever uses start()/isCurrent()/concludeIfCurrent().
…stration wrapper 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.
…ent 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.
Motivation
@datadog/action-catalogand@datadog/apps-backendregister runtime context via a shared, module-level setter. A concurrent execution's registration could silently redirect an in-flight call to the wrong identity, with no error. See the RFC's Decisions and Trade-Offs.executeActioncall could still fire for real, attributed to whichever execution is current by then.$.Actionsreference alone doesn't close every gap:globalThis.$was one shared mutable property, so a zombie's fresh read of it (not one captured before abandonment) still resolved to whichever$a newer execution had most recently installed — letting a stale execution act under a newer execution's identity/allowedConnectionIds, a confused-deputy risk.globalThis.$per execution viaAsyncLocalStorage.AsyncLocalStorageaccessor breaks any customer module that assigns toglobalThis.$(e.g. importingzx/globals, which does exactly this) — it would throw instead of working as it did before.AsyncLocalStoragevalue so it's read/write, still isolated per execution.EpochGuard(execution-epoch.ts) as the single source of truth for scope currency, and memoize registration and the runtime build.JSON.stringifysilently convertsNaN/Infinityto"null"without throwing, unlike every other non-serializable shape this check already catches (Map/Set/BigInt/function/symbol) — a customer bug producing a non-finite result would otherwise return a silentnullinstead of a clear, attributed error.numberresult the same way the existing checks reject those other shapes.Architecture
enqueueserializes every local execution through one promise chain; within each slot,AsyncLocalStoragescopes that execution's own identity, and a sharedEpochGuardmarks it superseded the moment a later one starts. Two adapters registered once, for the process's lifetime, resolve identity dynamically at call time rather than at registration time — so a zombie's call always resolves to its own, now-invalid scope, never a newer execution's.Changes
EpochGuardprimitive: a generation counter that marks a started scope superseded the moment a newer one starts, independently unit-tested.enqueue) instead of running concurrently.BigInt, or a bare function/Symbol(whichJSON.stringifywould otherwise silently drop).$.Actionscalls now reject instead of running under a newer execution's identity, checked via whether its own scope is still current.@datadog/action-catalogtyped-wrapper call is guarded separately: the registered dispatcher is stable for the process's lifetime and resolves the calling execution's own dispatch fromAsyncLocalStorageat call time, rejecting once that execution has concluded.globalThis.$is now scoped per execution viaAsyncLocalStorageinstead of a plain mutable property, so a zombie's fresh$read always resolves to its own identity, never a newer execution's.globalThis.$are boxed per execution, so a customer module assigning to it (e.g. importingzx/globals) only shadows it for that execution — the prior value is visible again once the execution completes, with no throw.@datadog/action-catalogand@datadog/apps-backendregistration are now memoized byloadModuleidentity, so a real dev server (which reuses the samessrLoadModule) pays the install-check/load cost once per process instead of on every execution; each test still gets an isolated run since it constructs its ownloadModule.globalThisorder marker, not a mock); the queue keeps flowing after a rejection; aloadModulerejection surfaces cleanly; all three non-serializable-result shapes; the no-token-exposure and$.Sourceinvariants from #479 re-verified against the queued path.$.Actionsreference and its action-catalog typed-wrapper call both reject instead of running under a newer execution's dispatch; a zombie's freshglobalThis.$read resolves to its own identity mid-flight.globalThis.$without throwing, the prior value restores after the execution completes, and one execution's override never leaks into a later one; a genuinely failing sibling registration doesn't stop the other's typed-wrapper call from still correctly rejecting once its own execution concludes.NaN/Infinityresult is rejected with a clear, attributed error instead of silently serializing to"null".Map,Set,NaN, orInfinitynested anywhere inside a returned result (not just at the top level) is rejected the same way a top-level one is, sinceJSON.stringify's replacer runs on every key/value pair it visits.assertJsonSerializable's replacer no longer conflates a real, non-root empty-string key ({ '': someFunction }) withJSON.stringify's own root-call invocation, which also passes an empty string — switched from comparing the key to a one-shot flag set on the replacer's first invocation, since the root is always visited first regardless of key.enqueueserialization and theEpochGuardno longer matched the code: the former blamed a shared-setter race that's now WeakMap-guarded and idempotent (the real hazard is a customer function deletingglobalThis.$mid-flight); the latter mischaracterized the guard as a redundant backstop when it's the only thing rejecting a timed-out execution's late dispatch during the overlap window serialization intentionally permits. Both rewritten to state the actual mechanism.as Errorcasts in the test file with a narrowing assertion helper; fixed a UK spelling ("cancelled" → "canceled").$outside an active execution (e.g. a customer module's top-leveltypeof $ !== 'undefined'feature check) now resolves toundefinedinstead of throwing —$genuinely isn't a global property yet in production at that point, so an unresolvable reference reads asundefinedthere too, pertypeof's spec-defined behavior; the earlier throw broke that parity.assertJsonSerializablenow also rejects a Symbol-KEYED property (e.g.{ [Symbol('x')]: value }) —JSON.stringify's replacer is never invoked for these at all (only for a Symbol-valued property under a string key, already caught), so they silently vanished with no chance to be flagged.registerActionCatalogIfInstalled/registerBackendRuntimeIfInstalledshared the identical no-op/cache-check/register-once/evict-on-rejection wrapper logic around two otherwise-unrelated registration bodies — extracted into a sharedregisterOnceIfInstalled, both call sites keep their existing signatures. Also removedEpochGuard's unusedhasActiveScope/forceInvalidatemethods.run()'s try/finally only wrapped the customer-function call, so a failure while loading/resolving the module (a rejectingloadModule, or the export not being a function) skippedconcludeExecution()— now the whole body is wrapped, so every exit path concludes the scope. Also extracted the three near-identical "execution already concluded" error messages into oneabandonedExecutionError()helper, and corrected theEpochGuarddoc 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.QA Instructions
yarn test:unit packages/plugins/apps/src/vite/local-execution.test.ts # Expected: Test Suites: 1 passed / Tests: 73 passed ✅ VERIFIEDyarn test:unit packages/plugins/apps # Expected: Test Suites: 30 passed / Tests: 516 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 ✅ VERIFIEDManual QA — real scaffolded app, real dev server, real timeout
This module isn't independently reachable from
npm run devon its own (that requires #481) — exercised via a real scaffolded app running the full stack (npm link'd@datadog/vite-pluginbuilt from this stack's tip).Added a backend function that captures
$.Actionsup front, sleeps 15s (past the 10s default timeout), then attempts a real$.Actions.foo.bar(...)call:{"success":false,"error":"Local execution of \"hangThenCallAction\" timed out after 10000ms"}Confirmed via the dev server's own log that the abandoned call, ~5s later, was rejected immediately with
"...already concluded; refusing to run \"com.datadoghq.foo.bar\" as this stale execution..."— no real HTTP call to Datadog's API went out. ✅ VERIFIED (re-run end-to-end against the current tip)Note for anyone repeating this: the first attempt showed the call going out for real (a genuine
preview-asyncrequest reachingapi.datadoghq.com, rejected only by the server'sACTION_NOT_FOUND, not by this fix) — traced to a stalenpm link'd build (prepare-linkhad linked an olddist/).rm -rf dist && yarn build:all-no-typesbefore re-linking fixed it. Worth flagging since it's an easy false negative to chase after a rebase.Re-verified against the current tip (post-rebase onto master): the full automated suite (every concurrency/epoch/zombie/JSON-serialization test) re-ran clean, and #481's own manual QA pass drove real requests through the same$.Actions reference once its own execution is abandoned, even after a newer execution has taken over", "Should resolve a zombie execution's FRESH read of globalThis.$ to its OWN identity...", and "Should reject an abandoned execution's action-catalog typed-wrapper call, not silently run it under a newer registration" — which use real
enqueue/EpochGuardmachinery via a real dev server (doubleNumberexecuting in-process,alwaysThrowsreturning a clean error) — confirming the wiring this PR adds is still intact end-to-end. The specific 15s-timeout-then-zombie-call scenario documented above was not independently re-run against a live server this pass (it takes real wall-clock time to reproduce manually); the equivalent behavior is covered bylocal-execution.test.ts's real-timer regression tests — e.g. "Should reject a capturedsetTimeoutwith short (20-100ms) delays to reproduce the actual wall-clock race, and which did re-run clean.Blast Radius
local-execution.tsstill isn't called from anywhere in the existing dev server.Out of Scope / Follow-ups
handleExecuteAction, threading a realLoadModule,/__dd/executeActionViaCloudsplit, realpreview-asynccalls)$.Actionsexecutionnet.Socket.prototype.connect,fetch, andchild_process'sspawn/exec/execSyncfor the duration of a local execution, exempted only around the internal$.Actions→executeActioncallwithTimeoutstarts a few ticks later with the same durationDocumentation