mirror of
https://github.com/heygen-com/hyperframes.git
synced 2026-09-03 04:38:33 +00:00
fix(cli): fix batch re-entrancy, config race, exposure cap, and shouldTrack gap in DE trial
Four confirmed findings from a max-effort code review of the CLI trial mechanism: 1. maybeEnableDeParallelRouterTrial's `process.env.HF_DE_PARALLEL_ROUTER !== undefined` guard couldn't distinguish "the user set this" from "an earlier renderLocal() call in this same process already armed it" — so in --batch (all rows share one process), only row 1's outcome could ever reach maybeConsumeDeParallelRouterTrial. A revert on any later row was silently never persisted. Added a module-level deParallelRouterTrialManagedByUs flag to disambiguate, with a test-only reset export since it's process-lifetime state a real CLI invocation never needs to reset but a test suite sharing one module instance does. 2. writeConfig is a non-atomic whole-file overwrite with no locking, and readConfig's cache never invalidates — a concurrently running second CLI process (another terminal, a parallel script; doesn't even need to be a render, any command calls incrementCommandCount) could silently clobber a just-persisted deParallelRouterTrialFired:true with its own stale snapshot. Added readConfigFresh (bypasses the cache) and use it immediately before the trial's read-modify-write, narrowing the race window without a full config-subsystem locking rewrite. 3. The prior commit's semantics flip removed the only exposure cap — a healthy router that never reverts now force-enabled the experimental path on every eligible render forever. Added DE_PARALLEL_ROUTER_TRIAL_MAX_RENDERS (25) as a backstop: the trial turns off after this many engaged renders even absent an actual failure. 4. maybeEnableDeParallelRouterTrial only checked config.telemetryEnabled, not shouldTrack() — so a dev-mode run or a DO_NOT_TRACK/ HYPERFRAMES_NO_TELEMETRY user got the experimental path silently armed while telemetry was simultaneously blocked underneath it. Now gates on shouldTrack() (a strict superset). Also fixed, lower severity: readConfig's deParallelRouterTrialFired/ deParallelRouterTrialRenderCount parsing now validates the JSON type explicitly instead of a bare truthy/nullish read, so a hand-edited or corrupted config can't have the string "false" misread as truthy. Refactored maybeEnableDeParallelRouterTrial into three smaller functions (isDeParallelRouterTrialBlocked, stopManagingDeParallelRouterTrial) to bring cyclomatic/cognitive complexity back under the repo's threshold — also de-duplicates the "stop managing the env var" logic shared with maybeConsumeDeParallelRouterTrial. 14 new/updated tests (43 total in render.test.ts), including a direct regression test for the batch re-entrancy scenario and a loop test for the render-count cap. Verified the config primitives end-to-end against a real file, not just the mocked unit tests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Sonnet 5
parent
19f90b0b92
commit
532dad7cc7
@@ -75,6 +75,12 @@ export interface HyperframesConfig {
|
||||
* `maybeEnableDeParallelRouterTrial`/`maybeConsumeDeParallelRouterTrial`.
|
||||
*/
|
||||
deParallelRouterTrialFired?: boolean;
|
||||
/**
|
||||
* Count of engaged (routed or reverted) trial renders so far — the
|
||||
* backstop that caps exposure even absent an actual failure. See
|
||||
* `DE_PARALLEL_ROUTER_TRIAL_MAX_RENDERS` in `render.ts`.
|
||||
*/
|
||||
deParallelRouterTrialRenderCount?: number;
|
||||
}
|
||||
|
||||
const DEFAULT_CONFIG: HyperframesConfig = {
|
||||
@@ -120,7 +126,16 @@ export function readConfig(): HyperframesConfig {
|
||||
skillsUpdateAvailable: parsed.skillsUpdateAvailable,
|
||||
skillsOutdatedCount: parsed.skillsOutdatedCount,
|
||||
skillsMissingCount: parsed.skillsMissingCount,
|
||||
deParallelRouterTrialFired: parsed.deParallelRouterTrialFired,
|
||||
// Explicit `=== true`/typeof-number checks rather than a truthy/nullish
|
||||
// read — a hand-edited or corrupted config could plausibly carry a
|
||||
// non-boolean/non-number JSON value (e.g. the STRING "false", which is
|
||||
// truthy in JS) for these two fields specifically, since they're read
|
||||
// with a bare truthy check at the call site (review finding).
|
||||
deParallelRouterTrialFired: parsed.deParallelRouterTrialFired === true ? true : undefined,
|
||||
deParallelRouterTrialRenderCount:
|
||||
typeof parsed.deParallelRouterTrialRenderCount === "number"
|
||||
? parsed.deParallelRouterTrialRenderCount
|
||||
: undefined,
|
||||
};
|
||||
|
||||
cachedConfig = config;
|
||||
@@ -133,6 +148,19 @@ export function readConfig(): HyperframesConfig {
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Re-read the config from disk, bypassing the in-process cache. Use
|
||||
* immediately before a targeted single-field read-modify-write (e.g. the DE
|
||||
* parallel-router trial's render count/fired flag) to narrow — though not
|
||||
* eliminate, there is no cross-process file locking here — the window for a
|
||||
* lost update against a concurrently-running CLI process that wrote other
|
||||
* fields in the meantime.
|
||||
*/
|
||||
export function readConfigFresh(): HyperframesConfig {
|
||||
cachedConfig = null;
|
||||
return readConfig();
|
||||
}
|
||||
|
||||
/**
|
||||
* Persist config to disk. Updates the in-memory cache.
|
||||
*/
|
||||
|
||||
Reference in New Issue
Block a user