mirror of
https://github.com/heygen-com/hyperframes.git
synced 2026-09-07 10:06:21 +00:00
fix(cli,core,studio): close 15 review findings + 2 R5 blockers
R5 blockers
- Negative install-state latch was cached for the process lifetime, but
only `true` is monotonic across processes. A long-lived preview server
held a stale `false` and could re-enrol after another process tripped
the breaker. Only the positive is cached now; `false` re-reads.
- The real breaker writer used writeConfig(), which collapses
{ok:true, mirrored:false} to success, so a run that mirrored nothing
reported done with the latch only on the erasable store. It consumes
writeConfigWithResult and retries until both stores carry it.
Bucketing integrity
- Storage-restricted Studio profiles all bucketed on the literal
"anonymous": computed against the shipped hash, 100% of them were
enrolled in calibration-50 rather than 50%, and they merged into one
PostHog person. Per-session random id instead — persists nothing.
- bucketSeed had read/write authority backwards: install-state is
write-once authoritative, but readConfig took config.json's blindly, so
the stores could hold different seeds until a re-mint flipped every
cohort. Merged on read, like the latch.
- An unwritable ~/.hyperframes with no config.json re-minted per call,
re-rolling the seed on every command, and the "cohorts will not be
stable" warning was unreachable on that path.
- A corrupt PRE-MOVE state file was never deleted, so a machine reset
with `rm -rf ~/.hyperframes` reported predecessorFound/stateFileCorrupt
forever — poisoning the exact metric this work exists to produce.
Opt-out honoring
- CLI canary decisions memoized per process, so `hyperframes telemetry
disable` during a running preview server was ignored for hours while
the server kept serving pre-opt-out decisions. The memo is keyed on the
telemetry posture.
- shouldTrack() memoized, contradicting policy.ts's documented "not
memoized" contract that policy.test.ts asserts.
- The Studio override path resolved the bucket unit eagerly as an
argument, minting and PERSISTING a tracking id for an opted-out profile
— a value evaluateCanary discards unread.
- Storage reads could throw out of telemetry into a post-commit catch
block, reporting an already-committed edit as failed.
- readConfig printed an unsilenceable stderr warning on every invocation
for installs that opted out of telemetry entirely.
Host split
- isLoopbackHost rejected 0.0.0.0, so the documented
HYPERFRAMES_PREVIEW_HOST LAN mode silently lost CLI→Studio identity
stitching and split one user across two PostHog persons. Identity is
now allowed when the operator explicitly opted into LAN binding.
- Corrected the comment claiming the guard refuses spoofed Hosts: a
non-browser client sets Host freely. It is a browser DNS-rebinding
mitigation, not access control, and now says so.
Semantics and test hygiene
- percentage:100 did not mean everyone — exclude and no_unit_id sat above
the fast path, so the registry's "delete the entry at 100" step was an
unstaged flip for CI and seedless installs.
- CLI cohort adoption returned before evaluateCanary, dropping Studio's
own webdriver exclusion.
- overdueCanaries() was asserted against wall-clock time, so the whole
core suite would go red on 2026-09-15 for every unrelated PR; and `>`
against midnight made a canary overdue ON its sunset date.
- Statistical assertions ran on unseeded randomUUID() populations tight
enough to fail ~1 run in 200. Seeded.
Also: broke a config -> policy -> transport -> config import cycle by
moving POSTHOG_API_KEY to a leaf module.
Tests: 2347 CLI (bundle absent), 3153 Studio, 1450 core. Fault injection
covers the latch, seed authority, LAN identity, webdriver exclusion and
the anonymous-bucket fix. Two pre-existing tests asserted behaviour these
findings identify as wrong (shouldTrack memoization, 100%-excludes-CI)
and were rewritten with the reasoning stated.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
5e2a9432f1
commit
3f69a2c635
@@ -30,11 +30,14 @@ const configState = vi.hoisted(
|
||||
cache: Record<string, unknown> | null;
|
||||
writeConfigCalls: Array<Record<string, unknown>>;
|
||||
failWrites: number;
|
||||
/** Config write lands but the install-state mirror does not. */
|
||||
failMirrors: number;
|
||||
} => ({
|
||||
disk: { telemetryEnabled: true, deParallelRouterTrialFired: true },
|
||||
cache: null,
|
||||
writeConfigCalls: [],
|
||||
failWrites: 0,
|
||||
failMirrors: 0,
|
||||
}),
|
||||
);
|
||||
|
||||
@@ -147,6 +150,22 @@ vi.mock("../telemetry/config.js", () => ({
|
||||
configState.cache = { ...config };
|
||||
return true;
|
||||
}),
|
||||
// The breaker's safety path uses this rather than writeConfig, so it can
|
||||
// see a mirror failure instead of having it collapsed into `true`.
|
||||
writeConfigWithResult: vi.fn((config: Record<string, unknown>) => {
|
||||
configState.writeConfigCalls.push({ ...config });
|
||||
if (configState.failWrites > 0) {
|
||||
configState.failWrites--;
|
||||
return { ok: false, error: "mock write failure" };
|
||||
}
|
||||
configState.disk = { ...config };
|
||||
configState.cache = { ...config };
|
||||
if (configState.failMirrors > 0) {
|
||||
configState.failMirrors--;
|
||||
return { ok: true, mirrored: false };
|
||||
}
|
||||
return { ok: true };
|
||||
}),
|
||||
}));
|
||||
|
||||
vi.mock("../telemetry/client.js", () => ({
|
||||
@@ -216,6 +235,7 @@ describe("renderLocal browser GPU config", () => {
|
||||
configState.disk = { telemetryEnabled: true, deParallelRouterTrialFired: true };
|
||||
configState.cache = null;
|
||||
configState.failWrites = 0;
|
||||
configState.failMirrors = 0;
|
||||
configState.writeConfigCalls = [];
|
||||
trackingState.shouldTrack = true;
|
||||
trackingState.renderObservations = [];
|
||||
@@ -1050,6 +1070,33 @@ describe("renderLocal — DE parallel-router CLI trial", () => {
|
||||
expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined();
|
||||
});
|
||||
|
||||
// The config write landing is NOT enough: config.json is the copy a stale
|
||||
// writer or a re-mint can erase, so a run that mirrored nothing has left the
|
||||
// safety fact on the erasable store only. writeConfig() collapsed
|
||||
// {ok:true, mirrored:false} to success and the loop stopped there.
|
||||
it("retries when the install-state mirror fails even though config.json landed", async () => {
|
||||
configState.disk = {
|
||||
telemetryEnabled: true,
|
||||
deParallelRouterTrialFired: false,
|
||||
telemetryNoticeShown: true,
|
||||
};
|
||||
configState.failMirrors = 1; // first attempt mirrors nothing
|
||||
producerState.executeImpl = async (job) => {
|
||||
job.perfSummary = {
|
||||
resolution: { width: 100, height: 100 },
|
||||
drawElement: { parallelRouter: "reverted" },
|
||||
};
|
||||
};
|
||||
await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions);
|
||||
|
||||
expect(configState.disk.deParallelRouterTrialFired).toBe(true);
|
||||
// Two writes: the one whose mirror failed, then the retry that mirrored.
|
||||
const firedWrites = configState.writeConfigCalls.filter(
|
||||
(c) => c.deParallelRouterTrialFired === true,
|
||||
);
|
||||
expect(firedWrites.length).toBeGreaterThanOrEqual(2);
|
||||
});
|
||||
|
||||
it("re-asserts the fired flag when the write is lost (concurrent clobber / transient failure), without re-counting the render", async () => {
|
||||
configState.disk = {
|
||||
telemetryEnabled: true,
|
||||
|
||||
@@ -66,6 +66,7 @@ import {
|
||||
readConfigFresh,
|
||||
recordRecentRender,
|
||||
writeConfig,
|
||||
writeConfigWithResult,
|
||||
type HyperframesConfig,
|
||||
} from "../telemetry/config.js";
|
||||
import { shouldTrack } from "../telemetry/client.js";
|
||||
@@ -1253,13 +1254,23 @@ function resolveDeParallelRouterOutcome(job: RenderJob): string | undefined {
|
||||
*/
|
||||
function persistDeParallelRouterTrialFired(): boolean {
|
||||
const MAX_ATTEMPTS = 3;
|
||||
let mirrored = false;
|
||||
for (let attempt = 0; attempt < MAX_ATTEMPTS; attempt++) {
|
||||
const config = readConfigFresh();
|
||||
if (config.deParallelRouterTrialFired) return true;
|
||||
// Both stores must carry the latch, not just config.json. Checking only
|
||||
// the config let a run stop early after a failed mirror — and config.json
|
||||
// is the copy a stale writer or a re-mint can erase, so the durable one
|
||||
// is exactly the one that was missing. The read side merges install-state
|
||||
// back in, so the two together are what make the trip survive.
|
||||
if (config.deParallelRouterTrialFired && mirrored) return true;
|
||||
config.deParallelRouterTrialFired = true;
|
||||
if (!writeConfig(config)) return false;
|
||||
const result = writeConfigWithResult(config);
|
||||
if (!result.ok) return false;
|
||||
mirrored = result.mirrored !== false;
|
||||
if (mirrored) return true;
|
||||
// Config landed but the mirror did not — retry rather than report success.
|
||||
}
|
||||
return Boolean(readConfigFresh().deParallelRouterTrialFired);
|
||||
return false;
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -37,7 +37,9 @@ async function loadTelemetryCommand(options?: {
|
||||
vi.doMock("../utils/env.js", () => ({
|
||||
isDevMode: () => options?.devMode ?? false,
|
||||
}));
|
||||
vi.doMock("../telemetry/transport.js", () => ({
|
||||
// The key moved to a leaf module to break a config -> policy -> transport
|
||||
// -> config import cycle; policy.ts reads it from there now.
|
||||
vi.doMock("../telemetry/posthogKey.js", () => ({
|
||||
POSTHOG_API_KEY: options?.apiKey ?? "phc_test",
|
||||
}));
|
||||
const module = await import("./telemetry.js");
|
||||
|
||||
Reference in New Issue
Block a user