mirror of
https://github.com/heygen-com/hyperframes.git
synced 2026-09-11 06:30:03 +00:00
fix(producer): serve /health from a worker_thread so probes survive main-thread stalls (#1733)
* fix(producer): serve /health from a worker_thread so probes survive main-thread stalls Adds an off-main-thread /health endpoint that listens on its own port (default 9848, env PRODUCER_HEALTH_PORT). The endpoint binds inside a Node worker_thread with a minimal node:http server — separate event loop, separate isolate — so probe responses don't depend on whatever the producer's main thread is doing. Why now ------- Today's hyperframes-producer crashloop traced to an infinite GSAP timeline -> distributed planner trying to enumerate ~300,000,000,000 frames -> sidecar /health stops landing within k8s's 5s window -> otherwise-healthy pods killed. Miguel is shipping the root-cause fix at plan() time (impossible / non-finite / sentinel durations get rejected before chunk planning). That removes today's wedge. This change is defense-in-depth for the kill mechanism. Even with the plan() guard, future wedge classes can stall the main event loop for seconds at a time: large synchronous file I/O (see the companion fileServer streaming PR), GC pauses on long-running renders, tight loops in user-authored GSAP / Three.js / canvas code, future activity / pool changes whose runtime cost we haven't yet characterized. Probe responsiveness should reflect process liveness, not main-thread event-loop responsiveness. If the entire Node process is dead the OS tears down both threads' sockets simultaneously and k8s correctly kills the pod. Anything short of that and the worker thread's listener keeps answering. Backwards-compatible: the main-thread /health on PRODUCER_PORT (9847) keeps working exactly as before. The k8s sidecar probe config in heygen-com/app can migrate to the worker port at its own pace. A companion heygen-com/app PR in this batch raises the probe timeout from 5s -> 30s as a last-resort backstop. TODO: link Miguel's upstream plan() duration guard PR once known. Test: healthWorker.test.ts (vitest) — 3 tests pass locally, including the load-bearing one: stays responsive while the main thread is blocked on a 500ms sync busy-spin. — Jerrai Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(producer): tighten healthWorker startup race + shutdown semantics Addresses Miga's review on #1733. - server.ts: store the worker as a Promise<HealthWorkerHandle | null> instead of mutating a `let` from inside `.then`. A SIGTERM landing before the `.then` callback fired would previously see `healthWorker === null` and skip cleanup. shutdown() now `await`s the promise with a bounded 1.5s timeout so a hung-startup worker can't keep SIGTERM waiting (worker.terminate() from process exit still kills it). - healthWorkerThread.ts: replace `process.exit()` inside the worker with `parentPort.close()` + natural event-loop drain. Node-version semantics for `process.exit()` from a worker have been historically inconsistent; the documented clean path is to close the channel and let the worker exit naturally. Also drops the redundant 2s force-exit on shutdown — the parent already owns the authoritative deadline via Promise.race + worker.terminate(), so the worker-side timer was belt-and-suspenders noise. Co-Authored-By: Jerrai <noreply@anthropic.com> — Jerrai --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
@@ -36,6 +36,7 @@ import {
|
||||
type RenderConfig,
|
||||
} from "./services/renderOrchestrator.js";
|
||||
import { prepareHyperframeLintBody, runHyperframeLint } from "./services/hyperframeLint.js";
|
||||
import { startHealthWorker, type HealthWorkerHandle } from "./services/healthWorker.js";
|
||||
import { isVideoFrameFormat } from "@hyperframes/engine";
|
||||
import { resolveRenderPaths } from "./utils/paths.js";
|
||||
import { defaultLogger, type ProducerLogger } from "./logger.js";
|
||||
@@ -738,10 +739,47 @@ export function startServer(options: ServerOptions = {}) {
|
||||
(server as unknown as import("node:http").Server).requestTimeout = 0;
|
||||
(server as unknown as import("node:http").Server).keepAliveTimeout = 0;
|
||||
|
||||
// Start the worker-thread health endpoint alongside the main listener.
|
||||
// The main thread keeps serving /health on `port` for backwards
|
||||
// compatibility; the worker thread additionally serves /health on
|
||||
// PRODUCER_HEALTH_PORT (default 9848) so k8s liveness/readiness probes can
|
||||
// migrate to a listener that doesn't share an event loop with renders.
|
||||
//
|
||||
// Opt-out: set PRODUCER_DISABLE_HEALTH_WORKER=1 (e.g. for tests that don't
|
||||
// want a worker spawned, or for environments where the extra port isn't
|
||||
// wanted).
|
||||
//
|
||||
// We store the *promise* (not the resolved handle) so a SIGTERM that
|
||||
// arrives before the worker has finished booting still has something to
|
||||
// await. Awaiting a `let healthWorker = null` mutated from inside `.then`
|
||||
// would race: if SIGTERM lands before the `.then` callback fires,
|
||||
// `shutdown()` sees `null` and skips worker cleanup. The promise pattern
|
||||
// closes that window without making startup blocking.
|
||||
const healthWorkerPromise: Promise<HealthWorkerHandle | null> =
|
||||
process.env.PRODUCER_DISABLE_HEALTH_WORKER === "1"
|
||||
? Promise.resolve(null)
|
||||
: startHealthWorker({ logger: log }).catch((err: Error) => {
|
||||
// Don't crash the producer if the worker fails to start — the main
|
||||
// /health is still up. Log loudly so the operator notices.
|
||||
log.error(`[server] health worker failed to start: ${err.message}`);
|
||||
return null;
|
||||
});
|
||||
|
||||
async function shutdown(signal: string) {
|
||||
log.info(`Received ${signal}, shutting down`);
|
||||
const { drainBrowserPool } = await import("@hyperframes/engine");
|
||||
await drainBrowserPool().catch(() => {});
|
||||
// Bounded await: if the worker hasn't come online within 1.5s of
|
||||
// shutdown there's no useful cleanup left to do — `worker.terminate()`
|
||||
// from process exit will kill the thread regardless, and we'd rather
|
||||
// not let a hung-startup worker keep the SIGTERM path waiting.
|
||||
const handle = await Promise.race<HealthWorkerHandle | null>([
|
||||
healthWorkerPromise,
|
||||
new Promise<null>((res) => setTimeout(() => res(null), 1_500).unref()),
|
||||
]);
|
||||
if (handle) {
|
||||
await handle.shutdown().catch(() => {});
|
||||
}
|
||||
server.close(() => {
|
||||
log.info("Server closed");
|
||||
process.exit(0);
|
||||
|
||||
Reference in New Issue
Block a user