Skip to content

Commit 1f9c70c

Browse files
fix(producer): guard fileServer.close in all render cleanup paths (#1406)
* fix(producer): guard fileServer.close in distributed cleanup paths `plan()` and `renderChunk()` both close the probe/chunk file server with a bare `fileServer.close()` in their cleanup sequence. `FileServerHandle.close` tears down the underlying http.Server, whose `close()` throws `ERR_SERVER_NOT_RUNNING` if the server was already torn down (for example a cancellation path that closed it once already). An unguarded throw there escapes the cleanup and masks the original plan/render result, exactly the failure the adjacent probe-session close already guards against with a try/catch (its comment even spells this out). Add `closeFileServerSafely`, which wraps the close in a try/catch and logs, and route both cleanup sites through it so the two stay consistent and a throwing close can never mask the real result. Covered by unit tests for both the throwing and happy paths. * fix(producer): extend fileServer.close guard to renderOrchestrator success path --------- Co-authored-by: Carlos Alcaraz <193642530+calcarazgre646@users.noreply.github.com>
1 parent 8f71378 commit 1f9c70c

5 files changed

Lines changed: 76 additions & 4 deletions

File tree

packages/producer/src/services/distributed/plan.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ import { join, relative, sep } from "node:path";
3838
import { type CanvasResolution } from "@hyperframes/core";
3939
import { type EngineConfig, getEncoderPreset, resolveConfig } from "@hyperframes/engine";
4040
import { defaultLogger, type ProducerLogger } from "../../logger.js";
41+
import { closeFileServerSafely } from "../fileServer.js";
4142
import { runAudioStage } from "../render/stages/audioStage.js";
4243
import { runCompileStage } from "../render/stages/compileStage.js";
4344
import { runExtractVideosStage } from "../render/stages/extractVideosStage.js";
@@ -833,7 +834,7 @@ export async function plan(
833834
job.duration = probeResult.duration;
834835
job.totalFrames = probeResult.totalFrames;
835836
const totalFrames = probeResult.totalFrames;
836-
if (probeResult.fileServer) probeResult.fileServer.close();
837+
if (probeResult.fileServer) closeFileServerSafely(probeResult.fileServer, "plan", log);
837838
if (probeResult.probeSession) {
838839
// Close inside a try/catch — leaking a Chrome process here would mask
839840
// the original plan() result on cancellation paths.

packages/producer/src/services/distributed/renderChunk.ts

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,12 @@ import {
6565
} from "../render/stages/freezePlan.js";
6666
import { sha256Hex } from "../render/stages/planHash.js";
6767
import { applyRuntimeEnvSnapshot } from "../render/runtimeEnvSnapshot.js";
68-
import { buildVirtualTimeShim, createFileServer, type FileServerHandle } from "../fileServer.js";
68+
import {
69+
buildVirtualTimeShim,
70+
closeFileServerSafely,
71+
createFileServer,
72+
type FileServerHandle,
73+
} from "../fileServer.js";
6974
import {
7075
buildSyntheticRenderJob,
7176
type DistributedFormat,
@@ -654,7 +659,7 @@ export async function renderChunk(
654659
});
655660
}
656661
}
657-
fileServer.close();
662+
closeFileServerSafely(fileServer, "renderChunk", log);
658663
// Leave the temp work dir on failure (helps debugging); remove it on
659664
// success below.
660665
}

packages/producer/src/services/fileServer.test.ts

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import { mkdirSync, mkdtempSync, rmSync, symlinkSync, writeFileSync } from "node
33
import path, { join } from "node:path";
44
import { tmpdir } from "node:os";
55
import {
6+
closeFileServerSafely,
67
createFileServer,
78
HF_BRIDGE_SCRIPT,
89
HF_EARLY_STUB,
@@ -11,6 +12,45 @@ import {
1112
VIRTUAL_TIME_SHIM,
1213
} from "./fileServer.js";
1314

15+
function captureLogger() {
16+
const warnings: { message: string; meta?: Record<string, unknown> }[] = [];
17+
return {
18+
warnings,
19+
log: {
20+
error() {},
21+
warn(message: string, meta?: Record<string, unknown>) {
22+
warnings.push({ message, meta });
23+
},
24+
info() {},
25+
debug() {},
26+
},
27+
};
28+
}
29+
30+
describe("closeFileServerSafely", () => {
31+
it("swallows and logs a throwing close instead of propagating", () => {
32+
const { log, warnings } = captureLogger();
33+
const fileServer = {
34+
close: () => {
35+
// http.Server.close() throws ERR_SERVER_NOT_RUNNING on a second close.
36+
throw new Error("Server is not running.");
37+
},
38+
};
39+
expect(() => closeFileServerSafely(fileServer, "plan", log)).not.toThrow();
40+
expect(warnings).toHaveLength(1);
41+
expect(warnings[0].message).toContain("[plan]");
42+
expect(warnings[0].meta?.error).toBe("Server is not running.");
43+
});
44+
45+
it("closes once and stays quiet on the happy path", () => {
46+
const { log, warnings } = captureLogger();
47+
let closed = 0;
48+
closeFileServerSafely({ close: () => closed++ }, "renderChunk", log);
49+
expect(closed).toBe(1);
50+
expect(warnings).toHaveLength(0);
51+
});
52+
});
53+
1454
describe("injectScriptsIntoHtml", () => {
1555
it("injects the virtual time shim into head content before authored scripts", () => {
1656
const html = `<!DOCTYPE html>

packages/producer/src/services/fileServer.ts

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import { join, extname, resolve, sep } from "node:path";
1616
import { injectScriptsAtHeadStart, injectScriptsIntoHtml } from "@hyperframes/core/compiler";
1717
import { getVerifiedHyperframeRuntimeSource } from "./hyperframeRuntimeLoader.js";
1818
import { getHfEarlyStub } from "../generated/hf-early-stub-inline.js";
19+
import { defaultLogger, type ProducerLogger } from "../logger.js";
1920

2021
export { injectScriptsAtHeadStart };
2122

@@ -558,6 +559,30 @@ export interface FileServerHandle {
558559
addPreHeadScript: (script: string) => void;
559560
}
560561

562+
/**
563+
* Close a file server handle, swallowing and logging any error.
564+
*
565+
* `FileServerHandle.close` tears down the underlying http.Server, whose
566+
* `close()` throws `ERR_SERVER_NOT_RUNNING` if the server is already torn down
567+
* (for example a cancellation path that closed it once already). An unguarded
568+
* throw inside a cleanup or `finally` block would mask the original render or
569+
* plan result, so cleanup callers must go through this instead of calling
570+
* `close()` directly.
571+
*/
572+
export function closeFileServerSafely(
573+
fileServer: Pick<FileServerHandle, "close">,
574+
label: string,
575+
log: ProducerLogger = defaultLogger,
576+
): void {
577+
try {
578+
fileServer.close();
579+
} catch (err) {
580+
log.warn(`[${label}] file server close failed`, {
581+
error: err instanceof Error ? err.message : String(err),
582+
});
583+
}
584+
}
585+
561586
export function createFileServer(options: FileServerOptions): Promise<FileServerHandle> {
562587
const { projectDir, compiledDir, port = 0, stripEmbeddedRuntime = true } = options;
563588

packages/producer/src/services/renderOrchestrator.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,7 @@ import { join, dirname, resolve } from "path";
7070
import { randomUUID } from "crypto";
7171
import { fileURLToPath } from "url";
7272
import {
73+
closeFileServerSafely,
7374
createFileServer,
7475
type FileServerHandle,
7576
HF_PAGE_SIDE_COMPOSITING_STUB,
@@ -1567,7 +1568,7 @@ export async function executeRenderJob(
15671568
if (frameLookup) frameLookup.cleanup();
15681569

15691570
// Stop file server
1570-
fileServer.close();
1571+
closeFileServerSafely(fileServer, "renderOrchestrator", log);
15711572
fileServer = null;
15721573

15731574
// ── Stage 6: Assemble ───────────────────────────────────────────────

0 commit comments

Comments
 (0)