From 6172d79dc2237a02d18084c3bef30b0dca8e51a4 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Fri, 10 Jul 2026 14:51:11 -0700 Subject: [PATCH] fix(cli): atomic config writes, gated trial warning, and write-failure signal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five findings from a fifth (final scoped) max-effort review of the previous commit, all local: 1. writeConfig now writes atomically (pid-suffixed temp file + renameSync — rename within one directory is atomic on POSIX). This closes the real hazard behind the review's torn-read finding: readConfig's corrupted-file catch RESETS the config to defaults (telemetry re-enabled, anonymousId rotated, trial fields wiped), so a concurrent reader catching a non-atomic write mid-flight would silently destroy the user's config — and the previous commit's per-render readConfigFresh() at the arm site multiplied exposure to exactly that window. Verified against a real filesystem, not just the mocked unit tests. 2. writeConfig now returns whether the write landed (errors still swallowed — telemetry must never break the CLI). persistDeParallelRouterTrialFired uses it to stop immediately on a genuine fs failure (retrying an unwritable file is pointless) and reserve its retries for actual concurrent clobbers, instead of 3 blind write attempts + 4 disk reads. 3. The persistence-failure console.warn is now !quiet-gated like every other trial message — a quiet/batch-json render on an unwritable ~/.hyperframes no longer emits unexpected stderr that CI wrappers asserting empty stderr would misread as a render failure. The in-process latch already guarantees the safety behavior whether or not the warning prints. 4. The arm site short-circuits on the in-process fired latch BEFORE the fresh config read — post-fired batch rows no longer pay a per-row config read + parse + shared-cache invalidation for an answer module state already knows. 5. Replaced the new `as T` assertions in render.test.ts's config-state factory with an explicitly typed vi.hoisted return (repo TypeScript convention: no `as T`). config.test.ts: node:fs mock gains renameSync (faithful to the new atomic write); new test covers the success/failure return and asserts no temp file survives a write. Co-Authored-By: Claude Fable 5 --- packages/cli/src/commands/render.test.ts | 47 +++++++++++++---------- packages/cli/src/commands/render.ts | 35 +++++++++++++---- packages/cli/src/telemetry/config.test.ts | 22 +++++++++++ packages/cli/src/telemetry/config.ts | 25 ++++++++++-- 4 files changed, 96 insertions(+), 33 deletions(-) diff --git a/packages/cli/src/commands/render.test.ts b/packages/cli/src/commands/render.test.ts index a18efa35c..28b187451 100644 --- a/packages/cli/src/commands/render.test.ts +++ b/packages/cli/src/commands/render.test.ts @@ -10,26 +10,30 @@ const producerState = vi.hoisted(() => ({ executeImpl: async (_job: Record): Promise => undefined, })); -const configState = vi.hoisted(() => ({ - // Defaults to "trial already fired" so the pre-existing renderLocal tests - // below (which predate the DE-parallel-router trial and don't expect - // HF_DE_PARALLEL_ROUTER to be touched) keep their exact prior behavior. - // - // `disk` is the authoritative "file"; `cache` models config.ts's real - // process-lifetime cachedConfig. Modeling them SEPARATELY matters: a mock - // where readConfig/readConfigFresh both read one live object hides - // exactly the class of bug where production code reads the stale cache - // when it needed a fresh disk read (review finding). - disk: { - telemetryEnabled: true, - deParallelRouterTrialFired: true, - } as Record, - cache: null as Record | null, - writeConfigCalls: [] as Array>, - // Simulate the real writeConfig's silent fs-error swallowing (unwritable - // ~/.hyperframes): the next N writes are recorded but never reach `disk`. - failWrites: 0, -})); +// Defaults to "trial already fired" so the pre-existing renderLocal tests +// below (which predate the DE-parallel-router trial and don't expect +// HF_DE_PARALLEL_ROUTER to be touched) keep their exact prior behavior. +// +// `disk` is the authoritative "file"; `cache` models config.ts's real +// process-lifetime cachedConfig. Modeling them SEPARATELY matters: a mock +// where readConfig/readConfigFresh both read one live object hides exactly +// the class of bug where production code reads the stale cache when it +// needed a fresh disk read (review finding). `failWrites` simulates the +// real writeConfig's silent fs-error swallowing (unwritable ~/.hyperframes): +// the next N writes are recorded but never reach `disk`. +const configState = vi.hoisted( + (): { + disk: Record; + cache: Record | null; + writeConfigCalls: Array>; + failWrites: number; + } => ({ + disk: { telemetryEnabled: true, deParallelRouterTrialFired: true }, + cache: null, + writeConfigCalls: [], + failWrites: 0, + }), +); const trackingState = vi.hoisted(() => ({ // maybeEnableDeParallelRouterTrial gates on the real shouldTrack(), which @@ -91,10 +95,11 @@ vi.mock("../telemetry/config.js", () => ({ configState.writeConfigCalls.push({ ...config }); if (configState.failWrites > 0) { configState.failWrites--; - return; // swallowed silently, like the real writeConfig's catch {} + return false; // swallowed silently, like the real writeConfig's catch {} } configState.disk = { ...config }; configState.cache = { ...config }; + return true; }), })); diff --git a/packages/cli/src/commands/render.ts b/packages/cli/src/commands/render.ts index 43f0758c2..f68cd32ab 100644 --- a/packages/cli/src/commands/render.ts +++ b/packages/cli/src/commands/render.ts @@ -1485,7 +1485,7 @@ export async function renderLocal( try { await producer.executeRenderJob(job, projectDir, outputPath, onProgress); } catch (error: unknown) { - maybeConsumeDeParallelRouterTrial(deParallelRouterTrialArmed, job); + maybeConsumeDeParallelRouterTrial(deParallelRouterTrialArmed, job, options.quiet); handleRenderError( error, options, @@ -1497,7 +1497,7 @@ export async function renderLocal( ); } - maybeConsumeDeParallelRouterTrial(deParallelRouterTrialArmed, job); + maybeConsumeDeParallelRouterTrial(deParallelRouterTrialArmed, job, options.quiet); const elapsed = Date.now() - startTime; trackRenderMetrics(job, elapsed, options, false); printRenderComplete( @@ -1741,6 +1741,14 @@ function stopManagingDeParallelRouterTrial(): void { */ function maybeEnableDeParallelRouterTrial(quiet: boolean, disabled: boolean): boolean { if (disabled) return false; + // The in-process latch alone decides once it's set — short-circuit before + // the disk read so post-fired batch rows don't pay a config read + parse + + // shared-cache invalidation per row for an answer module state already + // knows (review finding). + if (deParallelRouterTrialFiredThisProcess) { + stopManagingDeParallelRouterTrial(); + return false; + } const userSetIt = process.env.HF_DE_PARALLEL_ROUTER !== undefined && !deParallelRouterTrialManagedByUs; if (userSetIt) return false; @@ -1795,9 +1803,11 @@ function resolveDeParallelRouterOutcome(job: RenderJob): string | undefined { * re-asserting a boolean is idempotent, so retries can't corrupt anything, * unlike the render counter (a re-applied increment double-counts the * render when our write landed but a later concurrent write raced our - * verify read — review finding). Returns false when every attempt failed - * to stick (e.g. `writeConfig` silently swallowing fs errors on an - * unwritable `~/.hyperframes`). + * verify read — review finding). Returns false as soon as `writeConfig` + * reports an fs failure (unwritable `~/.hyperframes` — retrying a failed + * write is pointless, so the retries are reserved for genuine concurrent + * clobbers, where the write landed but a racing writer's stale snapshot + * overwrote it — review finding). */ function persistDeParallelRouterTrialFired(): boolean { const MAX_ATTEMPTS = 3; @@ -1805,7 +1815,7 @@ function persistDeParallelRouterTrialFired(): boolean { const config = readConfigFresh(); if (config.deParallelRouterTrialFired) return true; config.deParallelRouterTrialFired = true; - writeConfig(config); + if (!writeConfig(config)) return false; } return Boolean(readConfigFresh().deParallelRouterTrialFired); } @@ -1838,7 +1848,11 @@ function persistDeParallelRouterTrialFired(): boolean { * flag is the safety-critical bit and IS verified/re-asserted — see * `persistDeParallelRouterTrialFired`. */ -function maybeConsumeDeParallelRouterTrial(trialArmed: boolean, job: RenderJob): void { +function maybeConsumeDeParallelRouterTrial( + trialArmed: boolean, + job: RenderJob, + quiet: boolean, +): void { if (!trialArmed) return; const outcome = resolveDeParallelRouterOutcome(job); if (outcome === undefined) return; @@ -1855,7 +1869,12 @@ function maybeConsumeDeParallelRouterTrial(trialArmed: boolean, job: RenderJob): stopManagingDeParallelRouterTrial(); } writeConfig(config); - if (fired && !persistDeParallelRouterTrialFired()) { + // `!quiet`-gated like every other trial message: quiet/batch-json renders + // must produce no unexpected terminal output — CI wrappers asserting + // empty stderr would misread the warning as a render failure (review + // finding). The in-process latch above already guarantees the safety + // behavior the warning describes, whether or not it prints. + if (fired && !persistDeParallelRouterTrialFired() && !quiet) { console.warn( c.warn( " Could not persist the parallel drawElement trial's off-switch to " + diff --git a/packages/cli/src/telemetry/config.test.ts b/packages/cli/src/telemetry/config.test.ts index 7ee9670be..47411d538 100644 --- a/packages/cli/src/telemetry/config.test.ts +++ b/packages/cli/src/telemetry/config.test.ts @@ -21,6 +21,13 @@ vi.mock("node:fs", () => ({ writeFileSync: vi.fn((path: string, content: string) => { fsState.files.set(path, content); }), + // writeConfig writes atomically: temp file + rename over the target. + renameSync: vi.fn((from: string, to: string) => { + const content = fsState.files.get(from); + if (content === undefined) throw new Error(`ENOENT: ${from}`); + fsState.files.set(to, content); + fsState.files.delete(from); + }), })); describe("config.ts — readConfig / readConfigFresh / writeConfig (real module, faked fs)", () => { @@ -93,4 +100,19 @@ describe("config.ts — readConfig / readConfigFresh / writeConfig (real module, expect(config.telemetryEnabled).toBe(true); expect(config.anonymousId).toBeTruthy(); }); + + it("writeConfig reports success, leaves no temp file behind, and reports failure when the fs throws", async () => { + const config = readConfig(); + expect(writeConfig(config)).toBe(true); + // Atomic write: the pid-suffixed temp file must have been renamed away. + for (const path of fsState.files.keys()) { + expect(path.endsWith(".tmp")).toBe(false); + } + + const fs = await import("node:fs"); + vi.mocked(fs.writeFileSync).mockImplementationOnce(() => { + throw new Error("EACCES: permission denied"); + }); + expect(writeConfig(config)).toBe(false); + }); }); diff --git a/packages/cli/src/telemetry/config.ts b/packages/cli/src/telemetry/config.ts index ad4a7fb8d..863f3cee3 100644 --- a/packages/cli/src/telemetry/config.ts +++ b/packages/cli/src/telemetry/config.ts @@ -1,4 +1,4 @@ -import { existsSync, mkdirSync, readFileSync, writeFileSync } from "node:fs"; +import { existsSync, mkdirSync, readFileSync, renameSync, writeFileSync } from "node:fs"; import { join } from "node:path"; import { homedir } from "node:os"; import { randomUUID } from "node:crypto"; @@ -162,15 +162,32 @@ export function readConfigFresh(): HyperframesConfig { } /** - * Persist config to disk. Updates the in-memory cache. + * Persist config to disk. Updates the in-memory cache on success. + * + * Atomic: writes to a pid-suffixed temp file and renames it over the config — + * `rename(2)` within one directory is atomic on POSIX, so a concurrent + * reader can never observe a partially-written file. That matters beyond + * hygiene: `readConfig`'s corrupted-file catch RESETS the config to defaults + * (new anonymousId, telemetry re-enabled, all optional fields wiped), so a + * torn read of a non-atomic write would silently destroy the user's config + * (review finding). + * + * Returns whether the write actually landed — errors are still swallowed + * (telemetry must never break the CLI), but callers that need persistence + * certainty (e.g. the DE parallel-router trial's off-switch) can react + * instead of re-implementing read-back verification. */ -export function writeConfig(config: HyperframesConfig): void { +export function writeConfig(config: HyperframesConfig): boolean { try { mkdirSync(CONFIG_DIR, { recursive: true, mode: 0o700 }); - writeFileSync(CONFIG_FILE, JSON.stringify(config, null, 2) + "\n", { mode: 0o600 }); + const tmpFile = `${CONFIG_FILE}.${process.pid}.tmp`; + writeFileSync(tmpFile, JSON.stringify(config, null, 2) + "\n", { mode: 0o600 }); + renameSync(tmpFile, CONFIG_FILE); cachedConfig = { ...config }; + return true; } catch { // Non-fatal — telemetry should never break the CLI + return false; } }