[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: 65222cc | 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
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
eb29e16 to
ddb4722
Compare
ddb4722 to
0e4c540
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
0eb79ba to
d7263a5
Compare
f554d9b to
0456c5b
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0456c5b29a
ℹ️ 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".
|
|
||
| /** 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; | ||
|
|
There was a problem hiding this comment.
When another dependency such as zx/globals defines globalThis.$ before this module is imported, this import-time defineProperty replaces that value while globalDollarOutsideExecution is still undefined. The restoration test only assigns its “pre-existing” value after this module has already loaded, so it misses the real initialization order; after loading the plugin, unrelated code outside an execution observes undefined instead of its original $. Initialize the outside-execution slot from the existing property before installing the accessor (and account for an incompatible descriptor).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a5a9d26 — seeded globalDollarOutsideExecution from Reflect.get(globalThis, '$') before installing the accessor, and added a regression test using jest.isolateModules to verify a pre-existing $ survives module load.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
Suppressed comments (11)
packages/plugins/apps/src/vite/local-execution.test.ts:830
- Avoid introducing another
anyescape hatch for$;testDollar()already centralizes and types this runtime-only global.
() => resolve((globalThis as Record<string, any>).$.backendFunctionArgs),
packages/plugins/apps/src/vite/local-execution.test.ts:898
- Use the existing typed
testDollar()helper instead of adding aRecord<string, any>assertion, in line with the repository's no-anyconvention.
const { Actions } = (globalThis as Record<string, any>).$;
packages/plugins/apps/src/vite/local-execution.test.ts:952
- This is another avoidable
anyassertion for the runtime global. ReusetestDollar()so access remains centralized and typed.
const $ = (globalThis as Record<string, any>).$;
packages/plugins/apps/src/vite/local-execution.test.ts:1045
- The timeout handler no longer poisons or replaces
registeredImpl; this is the stable adapter, which rejects because the calling async context is concluded. Update the explanation to match the implementation.
// 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.
packages/plugins/apps/src/vite/local-execution.test.ts:1053
- There is no poison stub for a newer registration to overwrite anymore. This regression protects the stable adapter's call-time context resolution, so the current explanation is obsolete.
// 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.
packages/plugins/apps/src/vite/local-execution.test.ts:1086
- Execution B does not self-poison on completion in the new design; it only concludes its epoch scope. Remove the obsolete poisoning description so the timing rationale matches the stable-adapter implementation.
// 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.
packages/plugins/apps/src/vite/local-execution.test.ts:1112
- This still says B self-poisons its registration, but conclusion now only invalidates B's scope and leaves the stable registration in place.
// 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.
packages/plugins/apps/src/vite/local-execution.test.ts:1181
- Late cleanup no longer re-poisons registration; it only attempts to conclude the abandoned execution's epoch scope. The comment should describe that current invariant.
// 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.
packages/plugins/apps/src/vite/local-execution.test.ts:1235
- B's conclusion no longer poisons the registration. This assertion captures the stable adapter and verifies that A's later cleanup does not replace it, so the comment currently describes behavior that cannot occur.
// Captures whatever B's own conclusion left registered — B poisoning its own registration on completion is fine; nothing else must overwrite it.
packages/plugins/apps/src/vite/local-execution.test.ts:1130
- This explanation refers to removed handle-publishing and poisoning logic. Both registrations are started immediately by
Promise.all; action-catalog can independently finish installing its stable adapter even though apps-backend never settles.
// 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.
packages/plugins/apps/src/vite/execution-epoch.ts:16
- The example names
network-guard.ts, which is not present in this PR or the current repository. Keep this API documentation generic until that consumer exists.
/** True if some started scope hasn't yet been concluded or superseded (e.g. for `network-guard.ts`'s `runAllowed`). */
| const backendGlobalsContext = new AsyncLocalStorage<BackendGlobalsBox>(); | ||
|
|
||
| /** 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; |
There was a problem hiding this comment.
Fixed in a5a9d26 — same root cause as the sibling Codex finding on this line; seeded from Reflect.get(globalThis, "$") before installing the accessor.
| 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. |
There was a problem hiding this comment.
Fixed in a5a9d26 — reworded this and the other 7 flagged locations to describe the actual call-time AsyncLocalStorage dispatch resolution, not the removed poisoning mechanism.
| // 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`). */ |
There was a problem hiding this comment.
Fixed in a5a9d26 — dropped the network-guard.ts/env-guard.ts mentions from both doc comments; this file only describes local-execution.ts as a consumer today.
a5a9d26 to
24f39d2
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).
24f39d2 to
65222cc
Compare
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 was current by then.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.execution-epoch.tsguard, and the apps-backend runtime/registration rebuilt on every execution/access instead of once.execution-epoch.ts'sEpochGuard, and memoize registration and the runtime build.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
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, since it always invokes whichever implementation is currently registered in shared state: the registration is proactively replaced with a rejecting stub once an execution concludes.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.try/finallyas the customer function call, so a genuine failure in one no longer skips poisoning an already-succeeded sibling.loadModuleidentity, 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 registration; 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 still poisons the completed one.QA Instructions
yarn test:unit packages/plugins/apps/src/vite/local-execution.test.ts # Expected: Test Suites: 1 passed / Tests: 42 passed ✅ VERIFIEDyarn test:unit packages/plugins/apps # Expected: Test Suites: 25 passed / Tests: 338 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
"...was abandoned after timing out; refusing to run \"com.datadoghq.foo.bar\"..."— no real HTTP call to Datadog's API went out. ✅ VERIFIEDNote 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.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→executeActioncallDocumentation