Skip to content

Commit 54c6843

Browse files
committed
refactor(apps): extract shared execution-epoch guard from local-execution.ts
The generation-counter pattern protecting against an abandoned scope's late cleanup corrupting a newer, currently-active one was independently reimplemented in local-execution.ts, network-guard.ts, and env-guard.ts. Extracts the shared logic into execution-epoch.ts's createEpochGuard(); network-guard.ts and env-guard.ts adopt it in later commits.
1 parent ac54e3a commit 54c6843

4 files changed

Lines changed: 192 additions & 15 deletions

File tree

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
// Unless explicitly stated otherwise all files in this repository are licensed under the MIT License.
2+
// This product includes software developed at Datadog (https://www.datadoghq.com/).
3+
// Copyright 2019-Present Datadog, Inc.
4+
5+
import { createEpochGuard } from '@dd/apps-plugin/vite/execution-epoch';
6+
7+
describe('execution-epoch — createEpochGuard', () => {
8+
test('Should report a fresh scope as current and report no active scope before any start()', () => {
9+
const guard = createEpochGuard();
10+
expect(guard.hasActiveScope()).toBe(false);
11+
12+
const scope = guard.start();
13+
expect(scope.isCurrent()).toBe(true);
14+
expect(guard.hasActiveScope()).toBe(true);
15+
});
16+
17+
test('Should invalidate an older scope once a newer one starts', () => {
18+
const guard = createEpochGuard();
19+
const older = guard.start();
20+
expect(older.isCurrent()).toBe(true);
21+
22+
const newer = guard.start();
23+
expect(older.isCurrent()).toBe(false);
24+
expect(newer.isCurrent()).toBe(true);
25+
expect(guard.hasActiveScope()).toBe(true);
26+
});
27+
28+
test('Should make concludeIfCurrent a no-op returning false for an already-superseded scope', () => {
29+
const guard = createEpochGuard();
30+
const older = guard.start();
31+
guard.start();
32+
33+
expect(older.concludeIfCurrent()).toBe(false);
34+
// The newer scope must be unaffected by the older one's no-op conclude.
35+
expect(guard.hasActiveScope()).toBe(true);
36+
});
37+
38+
test('Should conclude a still-current scope, clearing hasActiveScope', () => {
39+
const guard = createEpochGuard();
40+
const scope = guard.start();
41+
42+
expect(scope.concludeIfCurrent()).toBe(true);
43+
expect(scope.isCurrent()).toBe(false);
44+
expect(guard.hasActiveScope()).toBe(false);
45+
});
46+
47+
test('Should make a second concludeIfCurrent call on the same scope a no-op', () => {
48+
const guard = createEpochGuard();
49+
const scope = guard.start();
50+
51+
expect(scope.concludeIfCurrent()).toBe(true);
52+
expect(scope.concludeIfCurrent()).toBe(false);
53+
});
54+
55+
test('Should invalidate the active scope and clear hasActiveScope on forceInvalidate, without starting a new one', () => {
56+
const guard = createEpochGuard();
57+
const scope = guard.start();
58+
59+
guard.forceInvalidate();
60+
61+
expect(scope.isCurrent()).toBe(false);
62+
expect(guard.hasActiveScope()).toBe(false);
63+
});
64+
65+
test('Should make forceInvalidate followed by a fresh start() behave like an ordinary new scope', () => {
66+
const guard = createEpochGuard();
67+
const abandoned = guard.start();
68+
guard.forceInvalidate();
69+
70+
const current = guard.start();
71+
72+
expect(abandoned.isCurrent()).toBe(false);
73+
expect(current.isCurrent()).toBe(true);
74+
expect(guard.hasActiveScope()).toBe(true);
75+
76+
// The abandoned scope's late conclude must not corrupt the new one.
77+
expect(abandoned.concludeIfCurrent()).toBe(false);
78+
expect(current.isCurrent()).toBe(true);
79+
});
80+
81+
test('Should keep independently-created guards from sharing any state', () => {
82+
const guardA = createEpochGuard();
83+
const guardB = createEpochGuard();
84+
85+
const scopeA = guardA.start();
86+
expect(guardB.hasActiveScope()).toBe(false);
87+
expect(scopeA.isCurrent()).toBe(true);
88+
});
89+
});
Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,78 @@
1+
// Unless explicitly stated otherwise all files in this repository are licensed under the MIT License.
2+
// This product includes software developed at Datadog (https://www.datadoghq.com/).
3+
// Copyright 2019-Present Datadog, Inc.
4+
5+
/**
6+
* The generation-counter guard against the "abandoned scope's late cleanup
7+
* corrupts a newer, currently-active scope" race — shared by
8+
* `network-guard.ts`, `env-guard.ts`, and `local-execution.ts`'s own
9+
* execution bookkeeping, each of which independently reimplemented this
10+
* exact pattern before this extraction.
11+
*
12+
* A `runScriptLocally` execution isn't cancelled on timeout, only abandoned
13+
* — its `fn` may still be running (or, for a genuine hang, never settle) well
14+
* after a newer execution has started. Any cleanup an abandoned scope's own
15+
* `finally` performs on a shared, process-wide resource (network-guard's
16+
* monkey-patches, env-guard's `process.env` swap) must not run once a newer
17+
* scope has taken over — an equality check against `myGeneration` at the
18+
* moment cleanup actually fires is what makes that check load-bearing rather
19+
* than a race in itself: `activeGeneration` only changes under a later
20+
* `start()`/`forceInvalidate()`, never out from under a still-current scope.
21+
*/
22+
export interface EpochScope {
23+
/** True until a newer scope starts, or this one (or every scope) is concluded/invalidated. */
24+
isCurrent(): boolean;
25+
/**
26+
* If this scope is still current, marks no scope as active and returns
27+
* `true`; otherwise a no-op returning `false`. Use in a `finally` to gate
28+
* cleanup of a shared resource on still owning it.
29+
*/
30+
concludeIfCurrent(): boolean;
31+
}
32+
33+
export interface EpochGuard {
34+
/** Starts a new scope, superseding whichever one was previously active. */
35+
start(): EpochScope;
36+
/**
37+
* True if some started scope hasn't yet been concluded or superseded —
38+
* only needed by a consumer with its own separate "is anything currently
39+
* active" query (e.g. `network-guard.ts`'s `runAllowed`).
40+
*/
41+
hasActiveScope(): boolean;
42+
/**
43+
* Unconditionally invalidates the active scope, without starting a new
44+
* one — the backstop for a scope whose own `fn` never settles (see
45+
* `network-guard.ts`'s `forceReset` for why this exists independently of
46+
* a scope's own `finally`).
47+
*/
48+
forceInvalidate(): void;
49+
}
50+
51+
export function createEpochGuard(): EpochGuard {
52+
let currentGeneration = 0;
53+
let activeGeneration: number | null = null;
54+
55+
return {
56+
start() {
57+
const myGeneration = ++currentGeneration;
58+
activeGeneration = myGeneration;
59+
return {
60+
isCurrent: () => activeGeneration === myGeneration,
61+
concludeIfCurrent: () => {
62+
if (activeGeneration === myGeneration) {
63+
activeGeneration = null;
64+
return true;
65+
}
66+
return false;
67+
},
68+
};
69+
},
70+
hasActiveScope() {
71+
return activeGeneration !== null;
72+
},
73+
forceInvalidate() {
74+
currentGeneration += 1;
75+
activeGeneration = null;
76+
},
77+
};
78+
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -878,7 +878,7 @@ describe('local-execution — executeScriptLocally', () => {
878878

879879
const makeLoadModule = (actionCatalogDelayMs: number): LoadModule => {
880880
return async (specifier: string) => {
881-
if (specifier === func.absolutePath) {
881+
if (specifier === func.absolutePath + LOCAL_EXECUTION_LOAD_SUFFIX) {
882882
return { example: () => 'result' };
883883
}
884884
if (specifier === '@datadog/action-catalog/action-execution') {

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

Lines changed: 24 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@ import type { Logger } from '@dd/core/types';
2222
import type { BackendFunction } from '../backend/types';
2323
import { LOCAL_EXECUTION_LOAD_SUFFIX } from '../constants';
2424

25+
import { createEpochGuard } from './execution-epoch';
26+
2527
type BackendOutputs = { data: unknown };
2628

2729
interface ActionCallArgs {
@@ -107,13 +109,15 @@ function assertConnectionIdAllowed(
107109
let queueTail: Promise<unknown> = Promise.resolve();
108110

109111
/**
110-
* Bumped once per execution, in `runScriptLocally`, before anything else.
111-
* On timeout, an execution checks this against its own epoch to decide
112+
* Started once per execution, in `runScriptLocally`, before anything else.
113+
* On timeout, an execution checks its own scope against this to decide
112114
* whether it's safe to poison the shared action-catalog registration — see
113115
* `runScriptLocally`'s own comment for why this check, rather than a
114-
* per-call guard, is what actually protects that path.
116+
* per-call guard, is what actually protects that path. See
117+
* `execution-epoch.ts` for the shared generation-counter pattern this and
118+
* `network-guard.ts`/`env-guard.ts` all rely on.
115119
*/
116-
let currentExecutionEpoch = 0;
120+
const executionEpoch = createEpochGuard();
117121

118122
function enqueue<T>(run: () => Promise<T>): Promise<T> {
119123
const result = queueTail.then(run);
@@ -446,11 +450,11 @@ async function runScriptLocally(
446450
// execution may already be running (having registered its own valid
447451
// implementation) by the time this one's `fn()` finally settles or its
448452
// timeout fires, and clobbering ITS registration would be the same bug
449-
// in a new shape. `currentExecutionEpoch` gates that everywhere (see
450-
// `concludeExecution` below) — including the `finally` block, since
451-
// settling late for an abandoned execution is exactly the case this
452-
// guards against, not just the timeout path.
453-
const myEpoch = ++currentExecutionEpoch;
453+
// in a new shape. `scope` gates that everywhere (see `concludeExecution`
454+
// below) — including the `finally` block, since settling late for an
455+
// abandoned execution is exactly the case this guards against, not just
456+
// the timeout path.
457+
const scope = executionEpoch.start();
454458
let abandoned = false;
455459
let reRegisterActionCatalog: ((nextExecuteAction: ExecuteAction) => void) | undefined;
456460
let poisonBackendRuntime: BackendRuntimePoison | undefined;
@@ -504,11 +508,19 @@ async function runScriptLocally(
504508
// which for an abandoned execution can be well after a newer one has
505509
// already started and registered, so the check is load-bearing there.
506510
// Kept in one place so both call sites stay consistent.
511+
//
512+
// Poisons BEFORE concluding, not after: `register()`'s own re-registration
513+
// guard (see `registerActionCatalogIfInstalled`) checks `scope.isCurrent()`
514+
// too, and the poison calls below route back through that same `register`
515+
// closure — concluding first would make the scope look already-inactive
516+
// to that inner check, silently skipping the poison it was meant to
517+
// perform.
507518
const concludeExecution = () => {
508-
if (currentExecutionEpoch === myEpoch) {
519+
if (scope.isCurrent()) {
509520
abandoned = true;
510521
poisonActionCatalogRegistration();
511522
poisonBackendRuntimeRegistration();
523+
scope.concludeIfCurrent();
512524
}
513525
};
514526

@@ -518,18 +530,16 @@ async function runScriptLocally(
518530
Source: LOCAL_DEV_SOURCE,
519531
};
520532

521-
const isCurrent = () => currentExecutionEpoch === myEpoch;
522-
523533
const run = async (): Promise<BackendOutputs> => {
524534
(globalThis as Record<string, unknown>).$ = $;
525535
[reRegisterActionCatalog, poisonBackendRuntime] = await Promise.all([
526536
registerActionCatalogIfInstalled(
527537
loadModule,
528538
guardedExecuteAction,
529539
func.allowedConnectionIds,
530-
isCurrent,
540+
scope.isCurrent,
531541
),
532-
registerBackendRuntimeIfInstalled(loadModule, $, isCurrent, func.name),
542+
registerBackendRuntimeIfInstalled(loadModule, $, scope.isCurrent, func.name),
533543
]);
534544
if (abandoned) {
535545
// This execution was already abandoned while the registration

0 commit comments

Comments
 (0)