style(apps): tighten comments to their essential why

Trims verbose comments across execution-epoch.ts, local-execution.ts, and
their tests down to the load-bearing reasoning — the concurrency/race
explanations keep their substance, just without restating mechanics the
surrounding code already makes clear.

Author: tyffical

packages/plugins/apps/src/vite/execution-epoch.ts

@@ -3,21 +3,20 @@
 // Copyright 2019-Present Datadog, Inc.
 
 /**
- * The generation-counter guard against the "abandoned scope's late cleanup
- * corrupts a newer, currently-active scope" race — shared by
- * `network-guard.ts`, `env-guard.ts`, and `local-execution.ts`'s own
- * execution bookkeeping, each of which independently reimplemented this
- * exact pattern before this extraction.
+ * Generation-counter guard against an abandoned scope's late cleanup
+ * corrupting a newer, currently-active scope. Shared by `network-guard.ts`,
+ * `env-guard.ts`, and `local-execution.ts`'s execution bookkeeping — each
+ * has a shared, process-wide resource (network-guard's monkey-patches,
+ * env-guard's `process.env` swap, local-execution's action-catalog/
+ * apps-backend registrations) that an abandoned scope's own cleanup must not
+ * touch once a newer scope owns it.
  *
  * A `runScriptLocally` execution isn't cancelled on timeout, only abandoned
- * — its `fn` may still be running (or, for a genuine hang, never settle) well
- * after a newer execution has started. Any cleanup an abandoned scope's own
- * `finally` performs on a shared, process-wide resource (network-guard's
- * monkey-patches, env-guard's `process.env` swap) must not run once a newer
- * scope has taken over — an equality check against `myGeneration` at the
- * moment cleanup actually fires is what makes that check load-bearing rather
- * than a race in itself: `activeGeneration` only changes under a later
- * `start()`/`forceInvalidate()`, never out from under a still-current scope.
+ * — its `fn` may still be running, or never settle, well after a newer
+ * execution has started. Checking `myGeneration` against `activeGeneration`
+ * at the moment cleanup fires is safe because `activeGeneration` only
+ * changes under a later `start()`/`forceInvalidate()`, never out from under
+ * a still-current scope.
  */
 export interface EpochScope {
     /** True until a newer scope starts, or this one (or every scope) is concluded/invalidated. */

packages/plugins/apps/src/vite/local-execution.ts

@@ -91,10 +91,10 @@ function assertConnectionIdAllowed(
  * the wrong `$.Actions`/user identity, with no error at all. See the RFC's
  * Decisions and Trade-Offs for the full reasoning.
  *
- * Implementation: a simple promise-chain mutex. `queueTail` always resolves
- * (errors are swallowed via `.catch(() => {})` before being chained) so a
- * rejected execution never wedges the queue for whatever runs after it; the
- * real rejection is still preserved and returned to that call's own caller.
+ * `queueTail` is a promise-chain mutex whose chained handlers always resolve
+ * (rejections are swallowed via `.catch(() => {})`), so a rejected execution
+ * never wedges the queue — the real rejection still propagates to that
+ * execution's own caller via `result`.
  */
 let queueTail: Promise<unknown> = Promise.resolve();
 
@@ -192,20 +192,17 @@ function isModuleNotFoundError(error: unknown): boolean {
 /**
  * Returns a synchronous re-register function bound to the resolved
  * `setExecuteActionImplementation` setter, so a later poison/replace doesn't
- * need to re-run `loadModule` (an async call) — see `runScriptLocally`'s
- * `poisonActionCatalogRegistration` for why that matters: poisoning has to be
+ * need to re-run `loadModule` (an async call) — poisoning has to be
  * synchronous relative to the timeout that triggers it, or a newer
  * execution's own registration could land in between and get clobbered.
- * Returns `undefined` if action-catalog isn't installed, same as before.
+ * Returns `undefined` if action-catalog isn't installed.
  *
  * `isCurrent` is checked inside `register` itself, not just before this
- * function's own initial call — `loadModule` above is async and can be slow
- * enough for a newer execution to start and validly register its own
- * implementation while this call is still in flight. Guarding only the
- * initial call would still leave a later call to the returned `register`
- * (e.g. `runScriptLocally`'s own poison call) free to clobber that newer
- * registration; checking fresh, live epoch state inside `register` itself
- * protects every call site the same way, at the moment it actually runs.
+ * function's own initial call, because `loadModule` above is async: a newer
+ * execution can start and validly register its own implementation while
+ * this call is still in flight. Checking only the initial call would leave
+ * a later call to the returned `register` (e.g. a poison call) free to
+ * clobber that newer registration.
  */
 async function registerActionCatalogIfInstalled(
     loadModule: LoadModule,
@@ -398,37 +395,32 @@ async function runScriptLocally(
 
     // A timed-out execution is abandoned, not cancelled — enqueue() lets the
     // next queued execution start as soon as this one loses the Promise.race
-    // below, while this one may still be running in the background. If it
-    // later calls executeAction, it must not be allowed to silently run under
-    // whatever execution is now current. There are two distinct call paths,
-    // each needing its own guard:
+    // below, while this one may still be running in the background. A late
+    // executeAction call from it must not run under whatever execution is
+    // now current, via two distinct paths that each need their own guard:
     //
-    // 1. A raw `$.Actions` call made through a reference the customer's code
+    // 1. A raw `$.Actions` call through a reference the customer's code
     //    captured early (e.g. `const { Actions } = $` before any `await`).
-    //    That reference closes over `guardedExecuteAction` specifically, so
-    //    checking `abandoned` inside it protects this path correctly no
-    //    matter what a later execution does.
+    //    That reference closes over `guardedExecuteAction`, so checking
+    //    `abandoned` inside it protects this path regardless of what a
+    //    later execution does.
     // 2. An @datadog/action-catalog typed-wrapper call. This does NOT go
-    //    through the closure above — action-catalog stores exactly one
-    //    `executeAction` implementation in shared, module-level state
-    //    (`setExecuteActionImplementation`), and a typed-wrapper call always
-    //    invokes whichever implementation is CURRENTLY registered. Once a
-    //    newer execution registers its own (valid) implementation over this
-    //    one, this execution's `abandoned` flag becomes unreachable — nobody
-    //    calls this closure anymore, so checking it protects nothing. The
-    //    real fix is to proactively replace (poison) the registration itself
-    //    as soon as this execution concludes, via `poisonActionCatalogRegistration`
-    //    below, so a late call finds a rejecting stub already in place
-    //    instead of finding out mid-call that it's stale.
+    //    through that closure — action-catalog stores exactly one
+    //    `executeAction` implementation in shared, module-level state, and a
+    //    typed-wrapper call always invokes whichever implementation is
+    //    CURRENTLY registered. Once a newer execution registers its own
+    //    implementation over this one, this execution's `abandoned` flag
+    //    becomes unreachable — nobody calls this closure anymore. The fix is
+    //    to proactively replace (poison) the registration itself as soon as
+    //    this execution concludes, via `poisonActionCatalogRegistration`
+    //    below, so a late call finds a rejecting stub already in place.
     //
     // Poisoning is only safe if nothing newer has started yet — a newer
-    // execution may already be running (having registered its own valid
-    // implementation) by the time this one's `fn()` finally settles or its
-    // timeout fires, and clobbering ITS registration would be the same bug
-    // in a new shape. `scope` gates that everywhere (see `concludeExecution`
-    // below) — including the `finally` block, since settling late for an
-    // abandoned execution is exactly the case this guards against, not just
-    // the timeout path.
+    // execution may already be running by the time this one's `fn()`
+    // finally settles or its timeout fires, and clobbering its registration
+    // would be the same bug in a new shape. `scope` gates every poison call
+    // against that, including in the `finally` block below, where settling
+    // late for an abandoned execution is the case that guard exists for.
     const scope = executionEpoch.start();
     let abandoned = false;
     let reRegisterActionCatalog: ((nextExecuteAction: ExecuteAction) => void) | undefined;
@@ -446,10 +438,9 @@ async function runScriptLocally(
         return executeAction(fqn, inputs, connectionId);
     };
 
-    // An external package's setter throwing on re-registration must not be
-    // allowed to propagate — that would otherwise mask a successful result
-    // via `finally`, or abort the `setTimeout` callback below before it can
-    // reject.
+    // An external package's setter throwing on re-registration must not
+    // propagate, or it would mask a successful result via `finally`, or
+    // abort the `setTimeout` callback below before it can reject.
     const poisonActionCatalogRegistration = () => {
         try {
             reRegisterActionCatalog?.(() =>
@@ -475,21 +466,17 @@ async function runScriptLocally(
         }
     };
 
-    // Shared by the finally block below and the timeout handler: only safe
-    // to poison if nothing newer has started yet. For the timeout handler
-    // this is (currently) always true, since enqueue's mutex can't start a
-    // newer execution before this same synchronous callback runs — but the
-    // finally block runs whenever this execution's own `fn()` settles,
-    // which for an abandoned execution can be well after a newer one has
-    // already started and registered, so the check is load-bearing there.
-    // Kept in one place so both call sites stay consistent.
+    // Shared by the finally block below and the timeout handler, so both stay
+    // consistent: only safe to poison if nothing newer has started yet. The
+    // finally block is where this matters — it runs whenever this
+    // execution's own `fn()` settles, which for an abandoned execution can be
+    // well after a newer one has already started and registered.
     //
-    // Poisons BEFORE concluding, not after: `register()`'s own re-registration
-    // guard (see `registerActionCatalogIfInstalled`) checks `scope.isCurrent()`
-    // too, and the poison calls below route back through that same `register`
-    // closure — concluding first would make the scope look already-inactive
-    // to that inner check, silently skipping the poison it was meant to
-    // perform.
+    // Poisons BEFORE concluding: `register()`'s own re-registration guard
+    // also checks `scope.isCurrent()`, and the poison calls below route
+    // through that same `register` closure — concluding first would make
+    // the scope look already-inactive to that inner check and silently skip
+    // the poison.
     const concludeExecution = () => {
         if (scope.isCurrent()) {
             abandoned = true;
@@ -509,11 +496,9 @@ async function runScriptLocally(
         (globalThis as Record<string, unknown>).$ = $;
         // Publish each handle the moment its own promise resolves, not via a
         // destructuring assignment after Promise.all: if one registration
-        // call rejects (or just outlasts the timeout) while the other has
-        // already invoked its shared setter, a post-Promise.all assignment
-        // would never run at all, losing the completed registration's
-        // poison handle and leaving a timed-out execution's live setter
-        // reachable by a newer execution.
+        // rejects or outlasts the timeout while the other has already fired
+        // its shared setter, a post-Promise.all assignment would never run,
+        // losing the completed registration's poison handle.
         await Promise.all([
             registerActionCatalogIfInstalled(
                 loadModule,
@@ -530,11 +515,10 @@ async function runScriptLocally(
             ),
         ]);
         if (abandoned) {
-            // This execution was already abandoned while the registration
-            // calls above were still in flight — registering is a side
-            // effect of the calls themselves, not something we can undo, so
-            // immediately poison what they just registered rather than
-            // leaving it live for a newer execution to inherit.
+            // Abandoned while the registration calls above were still in
+            // flight — registering is a side effect of those calls, not
+            // something we can undo, so poison what they just registered
+            // instead of leaving it live for a newer execution to inherit.
             poisonActionCatalogRegistration();
             poisonBackendRuntimeRegistration();
         }
@@ -551,22 +535,20 @@ async function runScriptLocally(
             }
             if (abandoned) {
                 // Already known-abandoned before the customer function was
-                // even reached (the registration Promise.all above was slow
-                // enough to outlast the timeout, or this loadModule call
-                // was) — no point invoking it now; concludeExecution()
-                // below is a no-op re-poison at this point, which is fine.
+                // even reached (the registration Promise.all above, or this
+                // loadModule call, outlasted the timeout) — 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 {
-            // Covers the loadModule/export-validation steps above too, not
-            // just the fn() call — if either of those throws (including the
-            // native-module load failure case below), this execution has
-            // still concluded and must poison the registrations the same
-            // way a successful or thrown fn() call would.
+            // Also covers the loadModule/export-validation steps above, not
+            // just the fn() call — however this execution ends, it has
+            // concluded and must poison the registrations the same way a
+            // successful or thrown fn() call would.
             concludeExecution();
         }
     };