From 25a04d7fc8f90d1c3d6737a5716364850a08af73 Mon Sep 17 00:00:00 2001 From: Via Date: Thu, 16 Jul 2026 05:56:35 +0000 Subject: [PATCH] fix(cli): don't override exit code after artifact validated MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The render command's post-artifact-validated cleanup (telemetry flush, feedback prompt, worker/browser teardown, stray promise rejections) can throw AFTER the producer has committed a valid MP4 to disk. Field signal ts=1784169760, ts=1784171150, ts=1784172467 (all win32/x64, CLI 0.7.58, ffmpeg=no, 1080x1920): ffprobe + visual QA confirmed the outputs are valid, but the CLI exited 1 after the terminal "artifact validated" log with no final error message. Introduce a `renderSucceeded` sentinel that flips after `executeRenderJob` (or the Docker child render) resolves cleanly. From that point on: - Post-render steps in the render command (trackRenderMetrics, printRenderComplete, warnIfWebmAlphaDropped, maybePromptRenderFeedback) run through `runPostRenderStep`/`runPostRenderStepAsync` guards that swallow throws, log a compact warning to stderr, and sanitize a stray `process.exitCode` back to 0. - The CLI's top-level `uncaughtException` handler logs the throw for diagnosis but exits 0 instead of 1 when the render already succeeded. - The CLI's `unhandledRejection` handler stops flipping `commandFailed` (which drove the success:false telemetry field) when the render already succeeded. Co-Authored-By: Claude 🤖 Generated with [Claude Code](https://claude.com/claude-code) — Via --- packages/cli/src/cli.ts | 44 ++++- packages/cli/src/commands/render.ts | 78 ++++++--- .../src/utils/render-success-state.test.ts | 153 ++++++++++++++++++ .../cli/src/utils/render-success-state.ts | 95 +++++++++++ 4 files changed, 345 insertions(+), 25 deletions(-) create mode 100644 packages/cli/src/utils/render-success-state.test.ts create mode 100644 packages/cli/src/utils/render-success-state.ts diff --git a/packages/cli/src/cli.ts b/packages/cli/src/cli.ts index 609633459..a1ac2a036 100644 --- a/packages/cli/src/cli.ts +++ b/packages/cli/src/cli.ts @@ -103,6 +103,7 @@ import { defineCommand, runMain } from "citty"; import type { ArgsDef, CommandDef } from "citty"; import { getRunId } from "./telemetry/runId.js"; import { reportCommandFailure, trackCommandFailures } from "./utils/command-failure-tracking.js"; +import { isRenderSucceeded } from "./utils/render-success-state.js"; const isHelp = process.argv.includes("--help") || process.argv.includes("-h"); @@ -299,6 +300,30 @@ process.on("uncaughtException", (error) => { commandFailed = true; process.exit(0); } + // Post-artifact-validated shutdown throws (worker teardown, browser + // process cleanup, stray subprocess stream error) must not turn a valid + // render into an exit-1 "no final error message" failure. The render + // command sets `renderSucceeded` right after the producer resolves and + // the artifact is committed — from that point on, every throw here is + // a teardown artifact, not a render failure. Field signals: + // ts=1784169760, ts=1784171150, ts=1784172467 (all win32/x64, CLI + // 0.7.58, ffmpeg=no, 1080x1920, valid MP4s on disk). + if (isRenderSucceeded()) { + // Log to stderr for diagnosis but do NOT flip commandFailed and do NOT + // exit non-zero. The render is valid. + process.stderr.write( + ` [hyperframes] Post-render uncaughtException (render already succeeded): ${error.message}\n`, + ); + _trackCliError?.({ + error_name: error.name, + error_message: error.message, + stack_trace: error.stack, + command, + kind: "uncaught_exception", + }); + _flushSync?.(); + process.exit(0); + } commandFailed = true; _trackCliError?.({ error_name: error.name, @@ -315,8 +340,25 @@ process.on("uncaughtException", (error) => { // running if the rejection is non-fatal (e.g. a fire-and-forget promise). // The exit handler above will still fire with the real exit code. process.on("unhandledRejection", (reason) => { - commandFailed = true; const error = reason instanceof Error ? reason : new Error(String(reason)); + // Same rationale as the uncaughtException branch above: a stray promise + // rejection during post-artifact-validated cleanup must not mark a valid + // render as failed. `commandFailed` gates the success:true telemetry + // field — keep it false when the render actually succeeded. + if (isRenderSucceeded()) { + process.stderr.write( + ` [hyperframes] Post-render unhandledRejection (render already succeeded): ${error.message}\n`, + ); + _trackCliError?.({ + error_name: error.name, + error_message: error.message, + stack_trace: error.stack, + command, + kind: "unhandled_rejection", + }); + return; + } + commandFailed = true; _trackCliError?.({ error_name: error.name, error_message: error.message, diff --git a/packages/cli/src/commands/render.ts b/packages/cli/src/commands/render.ts index c5b684f35..f747b0ca2 100644 --- a/packages/cli/src/commands/render.ts +++ b/packages/cli/src/commands/render.ts @@ -81,6 +81,11 @@ import { runEnvironmentChecks } from "../browser/preflight.js"; import { detectH264EncoderMode } from "../browser/ffmpeg.js"; import { chromeLaunchRemediation } from "../browser/linuxDeps.js"; import { killOrphanedProcesses } from "../utils/orphanCleanup.js"; +import { + markRenderSucceeded, + runPostRenderStep, + runPostRenderStepAsync, +} from "../utils/render-success-state.js"; import type { ProducerLogger, RenderJob } from "@hyperframes/producer"; import { MAX_VP9_CPU_USED, @@ -1413,23 +1418,35 @@ async function renderDocker( const elapsed = Date.now() - startTime; + // Docker child exited 0 → the containerized producer already validated + // AND committed the artifact. Mirror renderLocal's post-success guarantee + // so any late throw here (telemetry flush, feedback prompt) cannot flip + // the exit code. + markRenderSucceeded(); + // Track metrics (no job object available from Docker — use a minimal stub) - trackRenderComplete({ - durationMs: elapsed, - fps: fpsToNumber(options.fps), - quality: options.quality, - workers: options.workers, - docker: true, - gpu: options.gpu, - authoringSkill: options.authoringSkill, - ...getMemorySnapshot(), - }); + runPostRenderStep("trackRenderComplete", () => + trackRenderComplete({ + durationMs: elapsed, + fps: fpsToNumber(options.fps), + quality: options.quality, + workers: options.workers, + docker: true, + gpu: options.gpu, + authoringSkill: options.authoringSkill, + ...getMemorySnapshot(), + }), + ); // ponytail: Docker runs the producer in a child process, so no perfSummary is // threaded back here; the summary shows render time only (never a wrong video // length). Probe the output with ffprobe if a duration figure is wanted here. - printRenderComplete(outputPath, elapsed, options.quiet); - warnIfWebmAlphaDropped(outputPath, options.format, options.quiet); + runPostRenderStep("printRenderComplete", () => + printRenderComplete(outputPath, elapsed, options.quiet), + ); + runPostRenderStep("warnIfWebmAlphaDropped", () => + warnIfWebmAlphaDropped(outputPath, options.format, options.quiet), + ); if (options.exitAfterComplete) scheduleRenderProcessExit(); return { renderTimeMs: elapsed }; } @@ -1559,6 +1576,13 @@ export async function renderLocal( ); } + // Render resolved without throwing → producer's `artifact validated` + // checkpoint fired AND the artifact was committed to disk. From this + // point on, ANY thrown teardown error must not be allowed to override + // the exit code. Field signal ts=1784169760 / ts=1784171150 / ts=1784172467 + // (win32/x64, CLI 0.7.58): valid MP4 on disk, exited 1 with no error print. + markRenderSucceeded(); + maybeConsumeDeParallelRouterTrial(deParallelRouterTrialArmed, job, options.quiet); const elapsed = Date.now() - startTime; if (job.outcome === "completed_with_warnings") { @@ -1566,20 +1590,26 @@ export async function renderLocal( console.warn(c.warn(` [${warning.code}] ${warning.message}`)); } } - trackRenderMetrics(job, elapsed, options, false); - printRenderComplete( - outputPath, - elapsed, - options.quiet, - job.perfSummary?.compositionDurationSeconds, - job.perfSummary?.totalFrames, + runPostRenderStep("trackRenderMetrics", () => trackRenderMetrics(job, elapsed, options, false)); + runPostRenderStep("printRenderComplete", () => + printRenderComplete( + outputPath, + elapsed, + options.quiet, + job.perfSummary?.compositionDurationSeconds, + job.perfSummary?.totalFrames, + ), + ); + runPostRenderStep("warnIfWebmAlphaDropped", () => + warnIfWebmAlphaDropped(outputPath, options.format, options.quiet), ); - warnIfWebmAlphaDropped(outputPath, options.format, options.quiet); if (!options.skipFeedback) { - await maybePromptRenderFeedback({ - renderDurationMs: elapsed, - quiet: options.quiet, - }); + await runPostRenderStepAsync("maybePromptRenderFeedback", () => + maybePromptRenderFeedback({ + renderDurationMs: elapsed, + quiet: options.quiet, + }), + ); } if (options.exitAfterComplete) scheduleRenderProcessExit(); const durationMs = job.perfSummary diff --git a/packages/cli/src/utils/render-success-state.test.ts b/packages/cli/src/utils/render-success-state.test.ts new file mode 100644 index 000000000..4a148de2f --- /dev/null +++ b/packages/cli/src/utils/render-success-state.test.ts @@ -0,0 +1,153 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { + _resetRenderSuccessForTests, + isRenderSucceeded, + markRenderSucceeded, + runPostRenderStep, + runPostRenderStepAsync, +} from "./render-success-state.js"; + +describe("render-success-state flag", () => { + afterEach(() => { + _resetRenderSuccessForTests(); + }); + + it("starts unset", () => { + expect(isRenderSucceeded()).toBe(false); + }); + + it("flips true after markRenderSucceeded()", () => { + markRenderSucceeded(); + expect(isRenderSucceeded()).toBe(true); + }); + + it("stays true across repeated calls (idempotent)", () => { + markRenderSucceeded(); + markRenderSucceeded(); + markRenderSucceeded(); + expect(isRenderSucceeded()).toBe(true); + }); + + it("reset restores the initial state", () => { + markRenderSucceeded(); + _resetRenderSuccessForTests(); + expect(isRenderSucceeded()).toBe(false); + }); +}); + +describe("runPostRenderStep", () => { + const originalExitCode = process.exitCode; + + afterEach(() => { + process.exitCode = originalExitCode; + _resetRenderSuccessForTests(); + }); + + it("runs the step and returns normally on success", () => { + const sink = vi.fn(); + const step = vi.fn(); + runPostRenderStep("noop", step, sink); + expect(step).toHaveBeenCalledTimes(1); + expect(sink).not.toHaveBeenCalled(); + }); + + it("swallows a thrown error and reports it to the sink", () => { + const sink = vi.fn(); + const err = new Error("teardown blew up"); + runPostRenderStep( + "trackRenderMetrics", + () => { + throw err; + }, + sink, + ); + expect(sink).toHaveBeenCalledTimes(1); + const msg = sink.mock.calls[0]?.[0] as string; + expect(msg).toContain("trackRenderMetrics"); + expect(msg).toContain("teardown blew up"); + expect(msg).toContain("render already succeeded"); + }); + + it("sanitizes a stray non-zero process.exitCode back to 0", () => { + // Regression: a cleanup step that sets process.exitCode=1 (or a helper it + // calls that does) must not leave the CLI exiting 1 after a successful + // render. Field signal ts=1784169760 / ts=1784171150 / ts=1784172467. + process.exitCode = 1; + runPostRenderStep( + "printRenderComplete", + () => { + throw new Error("stat threw"); + }, + vi.fn(), + ); + expect(process.exitCode).toBe(0); + }); + + it("does not touch process.exitCode when the step succeeds", () => { + process.exitCode = 1; + runPostRenderStep("ok", () => undefined, vi.fn()); + expect(process.exitCode).toBe(1); + }); + + it("does not touch process.exitCode when it was already 0", () => { + process.exitCode = 0; + runPostRenderStep( + "warnIfWebmAlphaDropped", + () => { + throw new Error("stat missing"); + }, + vi.fn(), + ); + expect(process.exitCode).toBe(0); + }); + + it("stringifies a non-Error throw value", () => { + const sink = vi.fn(); + runPostRenderStep( + "misc", + () => { + throw "just a string"; + }, + sink, + ); + const msg = sink.mock.calls[0]?.[0] as string; + expect(msg).toContain("just a string"); + }); +}); + +describe("runPostRenderStepAsync", () => { + const originalExitCode = process.exitCode; + + afterEach(() => { + process.exitCode = originalExitCode; + _resetRenderSuccessForTests(); + }); + + it("awaits the async step and returns normally on success", async () => { + const sink = vi.fn(); + const step = vi.fn(() => Promise.resolve()); + await runPostRenderStepAsync("feedback", step, sink); + expect(step).toHaveBeenCalledTimes(1); + expect(sink).not.toHaveBeenCalled(); + }); + + it("swallows a rejected promise and reports it to the sink", async () => { + const sink = vi.fn(); + const err = new Error("feedback prompt crashed"); + await runPostRenderStepAsync("maybePromptRenderFeedback", () => Promise.reject(err), sink); + expect(sink).toHaveBeenCalledTimes(1); + const msg = sink.mock.calls[0]?.[0] as string; + expect(msg).toContain("maybePromptRenderFeedback"); + expect(msg).toContain("feedback prompt crashed"); + }); + + it("sanitizes process.exitCode back to 0 on async failure", async () => { + process.exitCode = 1; + await runPostRenderStepAsync( + "maybePromptRenderFeedback", + () => Promise.reject(new Error("stdin closed")), + vi.fn(), + ); + expect(process.exitCode).toBe(0); + }); +}); diff --git a/packages/cli/src/utils/render-success-state.ts b/packages/cli/src/utils/render-success-state.ts new file mode 100644 index 000000000..d69c8cb95 --- /dev/null +++ b/packages/cli/src/utils/render-success-state.ts @@ -0,0 +1,95 @@ +/** + * Shared render-success sentinel + post-artifact-validated cleanup guards. + * + * Read by the top-level CLI process handlers (uncaughtException / + * unhandledRejection) to sanitize the exit code when a post-artifact-validated + * cleanup step throws. + * + * Set by the render command AFTER `executeRenderJob` (or the Docker child + * render) resolves cleanly — the point at which the artifact has been + * validated AND committed to disk. Any throw after this point (worker + * teardown, browser shutdown, telemetry flush, feedback prompt, stray + * promise rejection) must not turn a valid render into an exit-1 + * "no final error message" failure. + * + * Field signal (all win32/x64, CLI 0.7.58, ffmpeg=no, 1080x1920 renders): + * - ts=1784169760 — 6-worker capture retried down after Runtime.evaluate + * timeout, completed all 1260 frames, printed 'artifact validated', + * exited 1 with no final error message. Output MP4 valid on disk. + * - ts=1784171150 — full REPRO command provided; identical shape. + * - ts=1784172467 — `--workers 2`, identical shape. + * All three: ffprobe + visual QA confirmed the output was valid; the CLI + * still exited 1 after the terminal "artifact validated" checkpoint. + */ + +let renderSucceeded = false; + +/** + * Called by the render command after the producer's `executeRenderJob` (or + * the Docker child) resolves cleanly. From this point on, any thrown + * teardown error must not be allowed to override the exit code. + */ +export function markRenderSucceeded(): void { + renderSucceeded = true; +} + +/** + * Read by cli.ts process handlers to decide whether a late throw is fatal. + * When true, the handlers log the throw at warn level (so it's still visible + * for diagnosis) but do not surface it as an exit-1 failure. + */ +export function isRenderSucceeded(): boolean { + return renderSucceeded; +} + +/** Test-only reset. Not exported from the package. */ +export function _resetRenderSuccessForTests(): void { + renderSucceeded = false; +} + +type PostRenderErrorSink = (message: string) => void; + +const defaultErrorSink: PostRenderErrorSink = (message) => { + process.stderr.write(`${message}\n`); +}; + +/** + * Run a post-artifact-validated cleanup step so a throw cannot flip the CLI + * exit code. `markRenderSucceeded()` MUST have been called first — this + * helper is only safe on the success path where the artifact is already + * committed to disk. Logs a compact warning to stderr, resets a stray + * `process.exitCode` back to 0, and swallows the error. + */ +export function runPostRenderStep( + label: string, + fn: () => void, + sink: PostRenderErrorSink = defaultErrorSink, +): void { + try { + fn(); + } catch (err) { + reportPostRenderStepFailure(label, err, sink); + } +} + +export async function runPostRenderStepAsync( + label: string, + fn: () => Promise, + sink: PostRenderErrorSink = defaultErrorSink, +): Promise { + try { + await fn(); + } catch (err) { + reportPostRenderStepFailure(label, err, sink); + } +} + +function reportPostRenderStepFailure(label: string, err: unknown, sink: PostRenderErrorSink): void { + const message = err instanceof Error ? err.message : String(err); + sink(` [hyperframes] Post-render step '${label}' failed (render already succeeded): ${message}`); + // Guard against the failing step (or something it triggered) setting a + // non-zero exitCode. The render succeeded → the CLI must exit 0. + if (process.exitCode !== undefined && process.exitCode !== 0) { + process.exitCode = 0; + } +}