From 12897347709bf0213985fb5bc088b29e1de2db80 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miguel=20=C3=81ngel?= Date: Wed, 21 Oct 2026 02:46:17 +0000 Subject: [PATCH] fix(cli): a finished capture exits even when a site's DNS never answers (#5983) * fix(cli): a finished capture exits even when a site's DNS never answers Capture resolved every page request with the system lookup and gave up after 10 s, but a system lookup cannot be cancelled: the abandoned one kept the process alive until the OS resolver timed out, so a capture that had printed its result could run past the app's 5-minute cap and lose its folder. Resolve through c-ares instead, as hostAnswers does, and cancel it at the same 10 s deadline. * test(cli): a capture lookup that answers must exit right after too Dropping the timer cleanup kept every successful lookup alive 10 s and no test noticed. * fix(cli): capture falls back to the system lookup and exits once its result is out c-ares answers first. When it has no answer, the system lookup (hosts file, OS-only DNS) runs in a child process killed at the 10 s deadline, cached per host, because Node waits for an in-process system lookup even in process.exit. Capture then asks the CLI to exit 100 ms after printing its result, as render does. * fix(cli): capture reads the hosts file first and kills its lookup child on exit The hosts file answers before DNS, as the system lookup does (overrides and 0.0.0.0 blocks keep working). A host whose system answer is cached skips c-ares, a caller that joins a running lookup keeps its own deadline, and a pending lookup child is killed when the CLI exits. * fix(cli): capture's forced exit waits until a slow reader has the whole result The root exit now waits for stdout and stderr to hand over what is already written before it calls process.exit, so a large JSON result on a slow pipe arrives complete. A hostname listed several times in the hosts file now has every address checked, not only the first. * fix(cli): keep process exit in the CLI root and let test fixtures exit The output wait is now a helper that only waits; cli.ts still owns the process.exit. The process-ownership check treats test fixtures like tests, since each one runs as its own process. * chore(cli): run the process-ownership check before every commit A process.exit outside a process root used to pass the local hooks and fail only in CI's Preflight. --- .fallowrc.jsonc | 2 + lefthook.yml | 3 + .../src/capture/captureProxy.browser.test.ts | 38 ++++-- .../cli/src/capture/publicNetwork.fixture.ts | 49 +++++++ .../capture/publicNetwork.integration.test.ts | 32 +++-- .../src/capture/publicNetwork.process.test.ts | 84 ++++++++++++ .../cli/src/capture/publicNetwork.test.ts | 109 ++++++++++----- packages/cli/src/capture/publicNetwork.ts | 124 +++++++++++++++--- packages/cli/src/cli.ts | 6 +- packages/cli/src/commands/capture.test.ts | 54 +++++++- packages/cli/src/commands/capture.ts | 4 +- packages/cli/src/commands/render.ts | 26 +--- .../cli/src/utils/commandResult.fixture.ts | 6 + .../src/utils/commandResult.process.test.ts | 23 ++++ packages/cli/src/utils/commandResult.ts | 27 ++++ packages/cli/src/utils/dnsTestServer.ts | 37 ++++++ packages/cli/src/utils/hostAnswers.test.ts | 33 +---- scripts/check-cli-process-ownership.mjs | 4 +- scripts/check-cli-process-ownership.test.mjs | 19 ++- 19 files changed, 553 insertions(+), 127 deletions(-) create mode 100644 packages/cli/src/capture/publicNetwork.fixture.ts create mode 100644 packages/cli/src/capture/publicNetwork.process.test.ts create mode 100644 packages/cli/src/utils/commandResult.fixture.ts create mode 100644 packages/cli/src/utils/commandResult.process.test.ts create mode 100644 packages/cli/src/utils/dnsTestServer.ts diff --git a/.fallowrc.jsonc b/.fallowrc.jsonc index c40522d656..29a8930702 100644 --- a/.fallowrc.jsonc +++ b/.fallowrc.jsonc @@ -62,6 +62,8 @@ // Warm-check client, spawned by path from warmCheck.process.test.ts. "packages/cli/src/warmCheckClient.fixture.ts", "packages/cli/src/browser/chromeOrphans.fixture.ts", + "packages/cli/src/capture/publicNetwork.fixture.ts", + "packages/cli/src/utils/commandResult.fixture.ts", // Test fixture worker, spawned by path via the pools' workerEntryPath // option from the crash-recovery tests; has no import-graph referrer. "packages/producer/src/services/__fixtures__/crashOnMessageWorker.mjs", diff --git a/lefthook.yml b/lefthook.yml index ae3bad3df5..28b634fac0 100644 --- a/lefthook.yml +++ b/lefthook.yml @@ -33,6 +33,9 @@ pre-commit: # fallow reuses one base-snapshot cache per checkout; `audit-cache prune` drops caches unused for a day, # including those of removed checkouts. The commit gets the audit's own status. run: env -u GIT_DIR -u GIT_INDEX_FILE -u GIT_WORK_TREE sh -c 'node_modules/.bin/fallow audit --base origin/main --fail-on-issues; status=$?; node_modules/.bin/fallow audit-cache prune --max-age-days 1 > /dev/null; exit $status' + cli-process-ownership: + glob: "packages/cli/src/**/*.ts" + run: bun run check:cli-process-ownership largefiles: # Rejects staged binaries over the limit that are not LFS pointers; see # the script header. CI runs the same script with --range. diff --git a/packages/cli/src/capture/captureProxy.browser.test.ts b/packages/cli/src/capture/captureProxy.browser.test.ts index a4b59c19c1..e3d32f6069 100644 --- a/packages/cli/src/capture/captureProxy.browser.test.ts +++ b/packages/cli/src/capture/captureProxy.browser.test.ts @@ -11,14 +11,36 @@ const fixture = vi.hoisted(() => ({ rebound: false, dialed: [] as string[], })); -vi.mock("node:dns/promises", () => ({ - lookup: vi.fn(async (host: string) => { - if (host === "capture-fixture.example.com") { - return [{ address: fixture.rebound ? "127.0.0.1" : "93.184.216.34", family: 4 }]; - } - throw new Error("Fixture has no such DNS name"); - }), -})); +vi.mock("node:dns", async (importOriginal) => { + const real = await importOriginal(); + return { + ...real, + promises: { + ...real.promises, + Resolver: class { + async resolve4(host: string) { + if (host === "capture-fixture.example.com") { + return [fixture.rebound ? "127.0.0.1" : "93.184.216.34"]; + } + throw new Error("Fixture has no such DNS name"); + } + cancel() {} + }, + }, + }; +}); +vi.mock("node:child_process", async (importOriginal) => { + const real = await importOriginal(); + return { + ...real, + execFile: (...args: Parameters) => { + if (!(args[1] as string[] | undefined)?.includes("-e")) return real.execFile(...args); + const callback = args.at(-1) as (error: Error, stdout: string, stderr: string) => void; + callback(new Error("Fixture has no system DNS"), "", "ENOTFOUND"); + return { once() {}, kill() {} } as unknown as ReturnType; + }, + }; +}); vi.mock("node:http", async (importOriginal) => { const original = await importOriginal(); return { diff --git a/packages/cli/src/capture/publicNetwork.fixture.ts b/packages/cli/src/capture/publicNetwork.fixture.ts new file mode 100644 index 0000000000..d5aabf9f06 --- /dev/null +++ b/packages/cli/src/capture/publicNetwork.fixture.ts @@ -0,0 +1,49 @@ +import { promises as dns } from "node:dns"; +import { createRequire, syncBuiltinESMExports } from "node:module"; + +const server = process.env.HF_TEST_DNS ?? ""; +if (!server) throw new Error("Missing DNS server"); +const Resolver = dns.Resolver; +dns.Resolver = class extends Resolver { + constructor(options?: ConstructorParameters[0]) { + super(options); + this.setServers([server]); + } +}; +const requireBuiltin = createRequire(import.meta.url); +const fsPromises = requireBuiltin("node:fs/promises"); +const readFile = fsPromises.readFile; +fsPromises.readFile = (path: string, ...rest: unknown[]) => + /hosts$/.test(String(path)) + ? Promise.resolve(process.env.HF_TEST_HOSTS ?? "") + : readFile(path, ...rest); +// HF_TEST_SYSTEM: "stall" for a system lookup that never returns, or the address it answers. +const system = process.env.HF_TEST_SYSTEM ?? "stall"; +const childProcess = requireBuiltin("node:child_process"); +const execFile = childProcess.execFile; +let lookupPid: number | undefined; +childProcess.execFile = (file: string, _args: string[], ...rest: unknown[]) => { + const child = execFile( + file, + [ + "-e", + system === "stall" ? "setInterval(() => {}, 1000)" : `process.stdout.write('["${system}"]')`, + ], + ...rest, + ); + lookupPid = child.pid; + return child; +}; +syncBuiltinESMExports(); +const exitAfterMs = Number(process.env.HF_TEST_EXIT_AFTER_MS ?? 0); +if (exitAfterMs) { + setTimeout(() => { + process.stdout.write(`exit ${lookupPid}\n`); + process.exit(0); + }, exitAfterMs); +} +const { resolvePublicAddress } = await import("./publicNetwork.js"); +const outcome = await resolvePublicAddress("assets.capture-fixture.com").catch( + (error: Error) => error.message, +); +process.stdout.write(`${outcome}\n`); diff --git a/packages/cli/src/capture/publicNetwork.integration.test.ts b/packages/cli/src/capture/publicNetwork.integration.test.ts index e135a76215..909ea89fc4 100644 --- a/packages/cli/src/capture/publicNetwork.integration.test.ts +++ b/packages/cli/src/capture/publicNetwork.integration.test.ts @@ -14,13 +14,25 @@ import type { TLSSocket } from "node:tls"; import { afterAll, beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; const transport = vi.hoisted(() => ({ - lookup: vi.fn(), + resolve4: vi.fn(), dials: [] as HttpsOptions[], certificate: "", trustCertificate: true, })); -vi.mock("node:dns/promises", () => ({ lookup: transport.lookup })); +vi.mock("node:dns", async (importOriginal) => { + const real = await importOriginal(); + return { + ...real, + promises: { + ...real.promises, + Resolver: class { + resolve4 = transport.resolve4; + cancel() {} + }, + }, + }; +}); // Only the already-checked numeric peer is mapped to an isolated loopback server. // The real Node HTTP/TLS transport, headers and certificate checks remain active. @@ -172,9 +184,9 @@ describe("publicRequest controlled socket integration", () => { receipts.length = 0; transport.dials.length = 0; transport.trustCertificate = true; - transport.lookup.mockReset(); - transport.lookup.mockResolvedValue([{ address: "127.0.0.1", family: 4 }]); - transport.lookup.mockResolvedValueOnce([{ address: "93.184.216.34", family: 4 }]); + transport.resolve4.mockReset(); + transport.resolve4.mockResolvedValue(["127.0.0.1"]); + transport.resolve4.mockResolvedValueOnce(["93.184.216.34"]); }); afterAll(async () => { @@ -193,12 +205,8 @@ describe("publicRequest controlled socket integration", () => { it("pins the checked address without a second DNS lookup and preserves HTTP Host/path", async () => { const response = await publicRequest(`http://assets.capture-fixture.com:${httpPort}/asset?q=1`); expect(await response.text()).toBe("fixture response"); - expect(transport.lookup).toHaveBeenCalledTimes(1); - expect(transport.lookup).toHaveBeenCalledWith("assets.capture-fixture.com", { - all: true, - family: 4, - verbatim: true, - }); + expect(transport.resolve4).toHaveBeenCalledTimes(1); + expect(transport.resolve4).toHaveBeenCalledWith("assets.capture-fixture.com"); expect(transport.dials).toHaveLength(1); expect(transport.dials[0]?.hostname).toBe("93.184.216.34"); expect(receipts).toEqual([ @@ -255,7 +263,7 @@ describe("publicRequest controlled socket integration", () => { }); it("rejects private DNS answers before dialing or reaching a target", async () => { - transport.lookup.mockReset().mockResolvedValue([{ address: "127.0.0.1", family: 4 }]); + transport.resolve4.mockReset().mockResolvedValue(["127.0.0.1"]); await expect( publicRequest(`http://assets.capture-fixture.com:${httpPort}/asset`), ).rejects.toThrow(/non-public/); diff --git a/packages/cli/src/capture/publicNetwork.process.test.ts b/packages/cli/src/capture/publicNetwork.process.test.ts new file mode 100644 index 0000000000..3f7d6a3320 --- /dev/null +++ b/packages/cli/src/capture/publicNetwork.process.test.ts @@ -0,0 +1,84 @@ +import { spawn } from "node:child_process"; +import { dirname, join } from "node:path"; +import { fileURLToPath } from "node:url"; +import { expect, it, onTestFinished } from "vitest"; +import { startTestDnsServer } from "../utils/dnsTestServer.js"; +import { testProcessIsAlive } from "../utils/processTestUtils.js"; +import { systemLookupInKillableChild } from "./publicNetwork.js"; + +const fixture = join(dirname(fileURLToPath(import.meta.url)), "publicNetwork.fixture.ts"); + +/** What the fixture printed, and how long its process lived after printing it. */ +async function resolveInChild( + mode: "silent" | "answer", + system: string, + ipv4?: string, + env: Record = {}, +) { + const server = await startTestDnsServer(mode, ipv4); + onTestFinished(() => server.close()); + const child = spawn(process.execPath, ["--import", "tsx", fixture], { + env: { ...process.env, HF_TEST_DNS: server.address, HF_TEST_SYSTEM: system, ...env }, + stdio: ["ignore", "pipe", "inherit"], + }); + onTestFinished(() => void child.kill("SIGKILL")); + let printed = ""; + let printedAt = 0; + child.stdout.setEncoding("utf8"); + child.stdout.on("data", (chunk: string) => { + printed += chunk; + if (printedAt) return; + printedAt = Date.now(); + setTimeout(() => child.kill("SIGKILL"), 5_000).unref(); + }); + await new Promise((resolve) => child.once("exit", resolve)); + return { printed: printed.trim(), livedOnMs: Date.now() - printedAt }; +} + +it("gives up when neither DNS nor the system lookup answers, and the process is gone right after", async () => { + const run = await resolveInChild("silent", "stall"); + expect(run.printed).toBe("Capture DNS timed out"); + expect(run.livedOnMs).toBeLessThan(1_000); +}, 30_000); + +it("falls back to the system answer when DNS gives none", async () => { + const run = await resolveInChild("silent", "8.8.4.4"); + expect(run.printed).toBe("8.8.4.4"); + expect(run.livedOnMs).toBeLessThan(1_000); +}, 30_000); + +it("uses the DNS answer without asking the system", async () => { + const run = await resolveInChild("answer", "8.8.4.4", "93.184.216.34"); + expect(run.printed).toBe("93.184.216.34"); + expect(run.livedOnMs).toBeLessThan(1_000); +}, 30_000); + +it("takes the hosts file's answer over the DNS one, as the system lookup does", async () => { + const hosts = "# local override\n8.8.4.4 assets.capture-fixture.com\n"; + const run = await resolveInChild("answer", "stall", "93.184.216.34", { HF_TEST_HOSTS: hosts }); + expect(run.printed).toBe("8.8.4.4"); +}, 30_000); + +it("refuses a host the hosts file blocks", async () => { + const hosts = "0.0.0.0 assets.capture-fixture.com\n"; + const run = await resolveInChild("answer", "stall", "93.184.216.34", { HF_TEST_HOSTS: hosts }); + expect(run.printed).toBe("Capture DNS returned a non-public address"); +}, 30_000); + +it("refuses a host when any of its hosts file addresses is not public", async () => { + const hosts = "8.8.4.4 assets.capture-fixture.com\n0.0.0.0 assets.capture-fixture.com\n"; + const run = await resolveInChild("answer", "stall", "93.184.216.34", { HF_TEST_HOSTS: hosts }); + expect(run.printed).toBe("Capture DNS returned a non-public address"); +}, 30_000); + +it("leaves no system lookup running when the CLI exits mid-lookup", async () => { + const run = await resolveInChild("silent", "stall", undefined, { HF_TEST_EXIT_AFTER_MS: "6000" }); + const pid = Number(/^exit (\d+)$/m.exec(run.printed)?.[1]); + onTestFinished(() => void (testProcessIsAlive(pid) && process.kill(pid, "SIGKILL"))); + expect(pid).toBeGreaterThan(0); + expect(testProcessIsAlive(pid)).toBe(false); +}, 30_000); + +it("runs the real system lookup in its child", async () => { + await expect(systemLookupInKillableChild("localhost", 10_000)).resolves.toContain("127.0.0.1"); +}, 30_000); diff --git a/packages/cli/src/capture/publicNetwork.test.ts b/packages/cli/src/capture/publicNetwork.test.ts index 3b8850e5df..b6464e2dde 100644 --- a/packages/cli/src/capture/publicNetwork.test.ts +++ b/packages/cli/src/capture/publicNetwork.test.ts @@ -1,13 +1,33 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; import { EventEmitter } from "node:events"; import { PassThrough } from "node:stream"; -const mocks = vi.hoisted(() => ({ lookup: vi.fn(), request: vi.fn() })); -vi.mock("node:dns/promises", () => ({ lookup: mocks.lookup })); +const mocks = vi.hoisted(() => ({ resolve4: vi.fn(), request: vi.fn(), execFile: vi.fn() })); +vi.mock("node:child_process", () => ({ execFile: mocks.execFile })); +vi.mock("node:fs/promises", () => ({ readFile: async () => "" })); +vi.mock("node:dns", () => ({ + promises: { + Resolver: class { + resolve4 = mocks.resolve4; + cancel() {} + }, + }, +})); vi.mock("node:http", () => ({ request: mocks.request })); vi.mock("node:https", () => ({ request: mocks.request })); import { isPublicIPv4, publicRequest, resolvePublicAddress } from "./publicNetwork.js"; -beforeEach(() => vi.resetAllMocks()); +/** The system lookup child prints `stdout`, or fails with `stderr`. */ +function systemLookupPrints(stdout: string, stderr = "") { + mocks.execFile.mockImplementation((_file, _args, _options, callback) => { + callback(stdout ? null : new Error("Command failed"), stdout, stderr); + return new EventEmitter(); + }); +} + +beforeEach(() => { + vi.resetAllMocks(); + systemLookupPrints("", "ENOTFOUND"); +}); describe("public address policy", () => { it.each([ "127.0.0.1", @@ -30,7 +50,7 @@ describe("public address policy", () => { ])("rejects %s", (ip) => expect(isPublicIPv4(ip)).toBe(false)); it("accepts public literals without DNS", async () => { await expect(resolvePublicAddress("8.8.8.8")).resolves.toBe("8.8.8.8"); - expect(mocks.lookup).not.toHaveBeenCalled(); + expect(mocks.resolve4).not.toHaveBeenCalled(); }); it.each([ "localhost", @@ -41,26 +61,61 @@ describe("public address policy", () => { "[::1]", ])("blocks alias/literal %s before DNS", async (host) => { await expect(resolvePublicAddress(host)).rejects.toThrow(); - expect(mocks.lookup).not.toHaveBeenCalled(); + expect(mocks.resolve4).not.toHaveBeenCalled(); }); it("rejects mixed DNS before opening a socket", async () => { - mocks.lookup.mockResolvedValue([ - { address: "8.8.8.8", family: 4 }, - { address: "127.0.0.1", family: 4 }, - ]); + mocks.resolve4.mockResolvedValue(["8.8.8.8", "127.0.0.1"]); await expect(publicRequest("http://example.com/")).rejects.toThrow(); expect(mocks.request).not.toHaveBeenCalled(); }); - it.each([{ answers: [] }, { answers: [{ address: "8.8.8.8", family: 6 }] }])( + it.each([{ answers: [] }, { answers: ["::ffff:8.8.8.8"] }])( "rejects empty or malformed DNS", async ({ answers }) => { - mocks.lookup.mockResolvedValue(answers); + mocks.resolve4.mockResolvedValue(answers); await expect(resolvePublicAddress("example.com")).rejects.toThrow(); }, ); - it("propagates DNS errors", async () => { - mocks.lookup.mockRejectedValue(new Error("DNS failed")); - await expect(resolvePublicAddress("example.com")).rejects.toThrow("DNS failed"); + it("fails when neither DNS nor the system lookup answers", async () => { + mocks.resolve4.mockRejectedValue(new Error("queryA ENOTFOUND")); + await expect(resolvePublicAddress("example.com")).rejects.toThrow( + "Capture DNS failed: ENOTFOUND", + ); + }); + it("checks a system answer like a DNS one, and asks the system once per host", async () => { + mocks.resolve4.mockRejectedValue(new Error("queryA ENOTFOUND")); + systemLookupPrints('["127.0.0.1"]'); + await expect(resolvePublicAddress("hosts-file.example.org")).rejects.toThrow(/non-public/); + systemLookupPrints('["8.8.8.8"]'); + await expect(resolvePublicAddress("cached.example.org")).resolves.toBe("8.8.8.8"); + await expect(resolvePublicAddress("cached.example.org")).resolves.toBe("8.8.8.8"); + expect(mocks.execFile).toHaveBeenCalledTimes(2); + }); + it("holds a later caller to its own deadline when it joins a system lookup already running", async () => { + vi.useFakeTimers(); + try { + mocks.resolve4.mockRejectedValue(new Error("queryA ENOTFOUND")); + mocks.execFile.mockImplementation(() => new EventEmitter()); + const first = resolvePublicAddress("slow.example.org").catch((error: Error) => error.message); + await vi.advanceTimersByTimeAsync(9_000); + const second = resolvePublicAddress("slow.example.org").catch( + (error: Error) => error.message, + ); + await vi.advanceTimersByTimeAsync(1_000); + expect(await first).toBe("Capture DNS timed out"); + await vi.advanceTimersByTimeAsync(8_000); + expect(await Promise.race([second, "still waiting"])).toBe("still waiting"); + await vi.advanceTimersByTimeAsync(1_000); + expect(await second).toBe("Capture DNS timed out"); + expect(mocks.execFile).toHaveBeenCalledOnce(); + expect(mocks.resolve4).toHaveBeenCalledOnce(); + } finally { + vi.useRealTimers(); + } + }); + it("never starts the system lookup when DNS answers", async () => { + mocks.resolve4.mockResolvedValue(["8.8.8.8"]); + await expect(resolvePublicAddress("example.org")).resolves.toBe("8.8.8.8"); + expect(mocks.execFile).not.toHaveBeenCalled(); }); }); @@ -86,14 +141,12 @@ function fakeRequest() { } describe("pinned request", () => { it("dials the checked numeric IP only, preserving TLS identity and manual redirects", async () => { - mocks.lookup - .mockResolvedValueOnce([{ address: "8.8.8.8", family: 4 }]) - .mockResolvedValue([{ address: "127.0.0.1", family: 4 }]); + mocks.resolve4.mockResolvedValueOnce(["8.8.8.8"]).mockResolvedValue(["127.0.0.1"]); fakeRequest(); const response = await publicRequest("https://example.com:8443/path?q=1", { headers: { host: "evil.test", "accept-encoding": "gzip" }, }); - expect(mocks.lookup).toHaveBeenCalledTimes(1); + expect(mocks.resolve4).toHaveBeenCalledTimes(1); expect(mocks.request).toHaveBeenCalledTimes(1); expect(mocks.request.mock.calls[0]?.[0]).toMatchObject({ hostname: "8.8.8.8", @@ -116,9 +169,9 @@ describe("pinned request", () => { ); it("does not dial after abort during DNS", async () => { const controller = new AbortController(); - mocks.lookup.mockImplementation(async () => { + mocks.resolve4.mockImplementation(async () => { controller.abort(); - return [{ address: "8.8.8.8", family: 4 }]; + return ["8.8.8.8"]; }); await expect( publicRequest("https://example.com", { signal: controller.signal }), @@ -128,20 +181,8 @@ describe("pinned request", () => { }); describe("transport lifecycle", () => { - it("bounds stalled DNS without dialing", async () => { - vi.useFakeTimers(); - try { - mocks.lookup.mockImplementation(() => new Promise(() => {})); - const result = expect(publicRequest("https://example.com")).rejects.toThrow("DNS timed out"); - await vi.advanceTimersByTimeAsync(10_000); - await result; - expect(mocks.request).not.toHaveBeenCalled(); - } finally { - vi.useRealTimers(); - } - }); it("cancels the upstream stream and socket when the body is cancelled", async () => { - mocks.lookup.mockResolvedValue([{ address: "8.8.8.8", family: 4 }]); + mocks.resolve4.mockResolvedValue(["8.8.8.8"]); const incoming = Object.assign(new PassThrough(), { statusCode: 200, headers: {} }); const req = Object.assign(new EventEmitter(), { end: vi.fn(), destroy: vi.fn() }); mocks.request.mockImplementation((_options, callback) => { @@ -161,7 +202,7 @@ describe("transport lifecycle", () => { }).not.toThrow(); }); it("fails unsupported compressed responses instead of returning corrupted asset bytes", async () => { - mocks.lookup.mockResolvedValue([{ address: "8.8.8.8", family: 4 }]); + mocks.resolve4.mockResolvedValue(["8.8.8.8"]); const incoming = Object.assign(new PassThrough(), { statusCode: 200, headers: { "content-encoding": "gzip" }, diff --git a/packages/cli/src/capture/publicNetwork.ts b/packages/cli/src/capture/publicNetwork.ts index 1d8b61d6ce..fe3191d413 100644 --- a/packages/cli/src/capture/publicNetwork.ts +++ b/packages/cli/src/capture/publicNetwork.ts @@ -1,8 +1,13 @@ -import { lookup } from "node:dns/promises"; +import { execFile, type ChildProcess } from "node:child_process"; +import { promises as dns } from "node:dns"; +import { readFile } from "node:fs/promises"; import { isIP } from "node:net"; import { request as httpRequest, type IncomingMessage, type ClientRequest } from "node:http"; import { request as httpsRequest } from "node:https"; +const DNS_TIMEOUT_MS = 10_000; +const CARES_TIMEOUT_MS = 5_000; + function ipv4Number(address: string): number { return address.split(".").reduce((value, octet) => value * 256 + Number(octet), 0); } @@ -50,29 +55,112 @@ export async function resolvePublicAddress(hostname: string): Promise { ) { throw new Error("Capture requires a public hostname"); } - let timer: ReturnType | undefined; - try { - const answers = await Promise.race([ - lookup(host, { all: true, family: 4, verbatim: true }), - new Promise((_resolve, reject) => { - timer = setTimeout(() => reject(new Error("Capture DNS timed out")), 10_000); - timer.unref(); - }), - ]); - if ( - !answers[0] || - answers.some((answer) => answer.family !== 4 || !isPublicIPv4(answer.address)) - ) { - throw new Error("Capture DNS returned a non-public address"); + const answers = await resolveIPv4(host); + const first = answers[0]; + if (!first || !answers.every(isPublicIPv4)) { + throw new Error("Capture DNS returned a non-public address"); + } + return first; +} + +async function resolveIPv4(host: string): Promise { + const fromHostsFile = (await hostsFileAnswers()).get(host); + if (fromHostsFile) return fromHostsFile; + const deadline = Date.now() + DNS_TIMEOUT_MS; + const fromSystem = systemAnswers.get(host); + if (fromSystem) return beforeDeadline(fromSystem, deadline); + return resolveWithCares(host).catch(() => + beforeDeadline(systemLookupInKillableChild(host, deadline - Date.now()), deadline), + ); +} + +let hostsFile: Promise> | undefined; + +function hostsFileAnswers(): Promise> { + const path = + process.platform === "win32" + ? `${process.env.SystemRoot ?? "C:\\Windows"}\\System32\\drivers\\etc\\hosts` + : "/etc/hosts"; + hostsFile ??= readFile(path, "utf8").then(parseHostsFile, () => new Map()); + return hostsFile; +} + +function parseHostsFile(text: string): Map { + const answers = new Map(); + for (const line of text.split(/\r?\n/)) { + const [address, ...names] = line.replace(/#.*/, "").trim().split(/\s+/); + if (!address || isIP(address) !== 4) continue; + for (const name of names) { + const key = name.toLowerCase().replace(/\.$/, ""); + answers.set(key, [...(answers.get(key) ?? []), address]); } - const first = answers[0]; - if (!first) throw new Error("Capture DNS returned no address"); - return first.address; + } + return answers; +} + +function beforeDeadline(work: Promise, deadline: number): Promise { + let timer: ReturnType | undefined; + const late = new Promise((_resolve, reject) => { + timer = setTimeout( + () => reject(new Error("Capture DNS timed out")), + Math.max(0, deadline - Date.now()), + ); + }); + return Promise.race([work, late]).finally(() => clearTimeout(timer)); +} + +async function resolveWithCares(host: string): Promise { + const cancellableResolver = new dns.Resolver(); + const timer = setTimeout(() => cancellableResolver.cancel(), CARES_TIMEOUT_MS); + try { + return await cancellableResolver.resolve4(host); } finally { clearTimeout(timer); } } +const systemAnswers = new Map>(); +const SYSTEM_LOOKUP = `require("node:dns").lookup(process.argv[1], { all: true, family: 4 }, (error, answers) => + error ? process.stderr.write(String(error.code)) : process.stdout.write(JSON.stringify(answers.map((a) => a.address))))`; + +const liveLookups = new Set(); + +export function systemLookupInKillableChild(host: string, timeoutMs: number): Promise { + if (!process.listeners("exit").includes(killLiveLookups)) process.on("exit", killLiveLookups); + const answers = new Promise((resolve, reject) => { + const child = execFile( + process.execPath, + ["-e", SYSTEM_LOOKUP, host], + { + timeout: Math.max(1, timeoutMs), + killSignal: "SIGKILL", + windowsHide: true, + env: { ...process.env, NODE_OPTIONS: "" }, + }, + (error, stdout, stderr) => { + if (error?.killed) reject(new Error("Capture DNS timed out")); + else if (!stdout) reject(new Error(`Capture DNS failed: ${stderr || error?.message}`)); + else { + try { + resolve(JSON.parse(stdout) as string[]); + } catch (parseError) { + reject(parseError); + } + } + }, + ); + liveLookups.add(child); + child.once("exit", () => liveLookups.delete(child)); + }); + systemAnswers.set(host, answers); + answers.catch(() => systemAnswers.delete(host)); + return answers; +} + +function killLiveLookups(): void { + for (const child of liveLookups) child.kill("SIGKILL"); +} + /** Pinned asset transport; redirects are returned for caller validation. */ export async function publicRequest(url: string | URL, init: RequestInit = {}): Promise { const target = new URL(url); diff --git a/packages/cli/src/cli.ts b/packages/cli/src/cli.ts index c6f0362010..afae651083 100644 --- a/packages/cli/src/cli.ts +++ b/packages/cli/src/cli.ts @@ -98,6 +98,7 @@ import { CliRuntimeError, CliUsageError, consumeCommandResult, + outputWritten, registerRootExitCodeSanitizer, registerRootExitRequester, type CommandResult, @@ -352,7 +353,10 @@ registerRootExitRequester((exitCode) => { exitCode, kind: exitCode === 0 ? "success" : "runtime_error", presented: true, - }).finally(() => process.exit(exitCode)); + }).finally(async () => { + await outputWritten(); + process.exit(exitCode); + }); }); registerRootExitCodeSanitizer(() => { diff --git a/packages/cli/src/commands/capture.test.ts b/packages/cli/src/commands/capture.test.ts index 42f9e72299..0bdbe5dedd 100644 --- a/packages/cli/src/commands/capture.test.ts +++ b/packages/cli/src/commands/capture.test.ts @@ -10,7 +10,7 @@ import { } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { CliRuntimeError } from "../utils/commandResult.js"; +import { CliRuntimeError, registerRootExitRequester } from "../utils/commandResult.js"; import { CAPTURE_PHASE_SCHEMA } from "../capture/types.js"; const { captureWebsiteMock } = vi.hoisted(() => ({ @@ -205,6 +205,58 @@ describe("capture command — vision control", () => { ); }); + it.each([ + [true, 0], + [false, 1], + ])("asks to end the process once its result is out (ok %s)", async (ok, exitCode) => { + vi.useFakeTimers(); + const exit = vi.fn(); + registerRootExitRequester(exit); + try { + vi.spyOn(console, "log").mockImplementation(() => {}); + vi.spyOn(console, "error").mockImplementation(() => {}); + captureWebsiteMock.mockResolvedValueOnce({ + ok, + projectDir: "/tmp/capture", + url: "https://example.com", + title: "Example", + extracted: {}, + screenshots: [], + tokens: { sections: [], fonts: [] }, + assets: [], + warnings: [], + }); + await captureCommand.run!({ + args: { url: "https://example.com", output: "/tmp/hf-exit", json: true }, + } as never).catch(() => {}); + expect(exit).not.toHaveBeenCalled(); + vi.advanceTimersByTime(100); + expect(exit).toHaveBeenCalledWith(exitCode); + } finally { + registerRootExitRequester(() => {}); + vi.useRealTimers(); + } + }); + + it("asks to end the process with 1 after a capture that threw", async () => { + vi.useFakeTimers(); + const exit = vi.fn(); + registerRootExitRequester(exit); + try { + vi.spyOn(console, "log").mockImplementation(() => {}); + vi.spyOn(console, "error").mockImplementation(() => {}); + captureWebsiteMock.mockRejectedValueOnce(new Error("browser crashed")); + await captureCommand.run!({ + args: { url: "https://example.com", output: "/tmp/hf-exit-threw", json: true }, + } as never).catch(() => {}); + vi.advanceTimersByTime(100); + expect(exit).toHaveBeenCalledWith(1); + } finally { + registerRootExitRequester(() => {}); + vi.useRealTimers(); + } + }); + it("returns a nonzero command result after presenting failed capture JSON once", async () => { const log = vi.spyOn(console, "log").mockImplementation(() => {}); vi.spyOn(console, "error").mockImplementation(() => {}); diff --git a/packages/cli/src/commands/capture.ts b/packages/cli/src/commands/capture.ts index c423490a86..0195c81468 100644 --- a/packages/cli/src/commands/capture.ts +++ b/packages/cli/src/commands/capture.ts @@ -1,5 +1,5 @@ import { DEFAULT_MAX_SCREENSHOTS } from "../capture/types.js"; -import { failCommand } from "../utils/commandResult.js"; +import { failCommand, scheduleCliExit } from "../utils/commandResult.js"; import { defineCommand } from "citty"; import { resolve } from "node:path"; import type { Example } from "./_examples.js"; @@ -303,8 +303,10 @@ export default defineCommand({ } else { console.error(`\n ✗ Capture failed: ${errMsg}\n`); } + scheduleCliExit(1); failCommand(); } + scheduleCliExit(captureFailed ? 1 : 0); if (captureFailed) failCommand(); }, }); diff --git a/packages/cli/src/commands/render.ts b/packages/cli/src/commands/render.ts index 85c8859b66..662ab1d570 100644 --- a/packages/cli/src/commands/render.ts +++ b/packages/cli/src/commands/render.ts @@ -1,6 +1,6 @@ import { captureArtifactRevision } from "../utils/artifactCapture.js"; import { recordRenderSuccess, registerRenderOutput } from "@hyperframes/studio-server/history"; -import { failCommand, requestCliExit } from "../utils/commandResult.js"; +import { failCommand, requestCliExit, scheduleCliExit } from "../utils/commandResult.js"; import { defineCommand } from "citty"; import type { Example } from "./_examples.js"; import { mkdtempSync, readdirSync, readFileSync, statSync, writeFileSync, rmSync } from "node:fs"; @@ -921,7 +921,7 @@ async function renderDocker( runPostRenderStep("warnIfWebmAlphaDropped", () => warnIfWebmAlphaDropped(outputPath, options.format, options.quiet), ); - if (options.exitAfterComplete) scheduleRenderProcessExit(); + if (options.exitAfterComplete) scheduleCliExit(); return { renderTimeMs: elapsed }; } @@ -1205,7 +1205,7 @@ async function executeLocalRender( }), ); } - if (options.exitAfterComplete) scheduleRenderProcessExit(); + if (options.exitAfterComplete) scheduleCliExit(); const durationMs = job.perfSummary ? Math.round(job.perfSummary.compositionDurationSeconds * 1000) : undefined; @@ -1219,26 +1219,6 @@ async function executeLocalRender( }; } -type UnrefableTimer = { - unref: () => void; -}; - -function isUnrefableTimer( - timer: ReturnType, -): timer is ReturnType & UnrefableTimer { - return ( - typeof timer === "object" && - timer !== null && - "unref" in timer && - typeof timer.unref === "function" - ); -} - -function scheduleRenderProcessExit(): void { - const timer = setTimeout(() => requestCliExit(0), 100); - if (isUnrefableTimer(timer)) timer.unref(); -} - function getMemorySnapshot() { return { peakMemoryMb: bytesToMb(process.memoryUsage.rss()), diff --git a/packages/cli/src/utils/commandResult.fixture.ts b/packages/cli/src/utils/commandResult.fixture.ts new file mode 100644 index 0000000000..5a1535d5e6 --- /dev/null +++ b/packages/cli/src/utils/commandResult.fixture.ts @@ -0,0 +1,6 @@ +import { outputWritten, registerRootExitRequester, scheduleCliExit } from "./commandResult.js"; + +registerRootExitRequester((exitCode) => void outputWritten().then(() => process.exit(exitCode))); +setInterval(function stalledLookup() {}, 1_000); +console.log(JSON.stringify({ title: "x".repeat(1024 * 1024) })); +scheduleCliExit(0); diff --git a/packages/cli/src/utils/commandResult.process.test.ts b/packages/cli/src/utils/commandResult.process.test.ts new file mode 100644 index 0000000000..f00352b56c --- /dev/null +++ b/packages/cli/src/utils/commandResult.process.test.ts @@ -0,0 +1,23 @@ +import { spawn } from "node:child_process"; +import { dirname, join } from "node:path"; +import { fileURLToPath } from "node:url"; +import { expect, it, onTestFinished } from "vitest"; + +const fixture = join(dirname(fileURLToPath(import.meta.url)), "commandResult.fixture.ts"); + +it("a forced exit still hands a slow reader the whole result", async () => { + const child = spawn(process.execPath, ["--import", "tsx", fixture], { + stdio: ["ignore", "pipe", "inherit"], + }); + onTestFinished(() => void child.kill("SIGKILL")); + let printed = ""; + child.stdout.setEncoding("utf8"); + child.stdout.on("data", (chunk: string) => { + printed += chunk; + child.stdout.pause(); + setTimeout(() => child.stdout.resume(), 100); + }); + const exitCode = await new Promise((resolve) => child.once("close", resolve)); + expect(exitCode).toBe(0); + expect((JSON.parse(printed) as { title: string }).title).toHaveLength(1024 * 1024); +}, 60_000); diff --git a/packages/cli/src/utils/commandResult.ts b/packages/cli/src/utils/commandResult.ts index e281de656f..84d685e8b9 100644 --- a/packages/cli/src/utils/commandResult.ts +++ b/packages/cli/src/utils/commandResult.ts @@ -108,6 +108,12 @@ export function sanitizeSuccessfulExitCode(): void { rootExitCodeSanitizer?.(); } +export async function outputWritten(): Promise { + const written = (stream: NodeJS.WriteStream) => + new Promise((resolve) => stream.write("", () => resolve())); + await Promise.all([written(process.stdout), written(process.stderr)]); +} + /** Ask cli.ts to finalize telemetry/output and then terminate the process. */ export function requestCliExit(exitCode = 0): void { if (!rootExitRequester) { @@ -120,3 +126,24 @@ export function requestCliExit(exitCode = 0): void { } rootExitRequester(exitCode); } + +type UnrefableTimer = { + unref: () => void; +}; + +function isUnrefableTimer( + timer: ReturnType, +): timer is ReturnType & UnrefableTimer { + return ( + typeof timer === "object" && + timer !== null && + "unref" in timer && + typeof timer.unref === "function" + ); +} + +/** For a command whose result is out: ends the process if anything it started is still holding it. */ +export function scheduleCliExit(exitCode = 0): void { + const timer = setTimeout(() => requestCliExit(exitCode), 100); + if (isUnrefableTimer(timer)) timer.unref(); +} diff --git a/packages/cli/src/utils/dnsTestServer.ts b/packages/cli/src/utils/dnsTestServer.ts new file mode 100644 index 0000000000..65cde3baa7 --- /dev/null +++ b/packages/cli/src/utils/dnsTestServer.ts @@ -0,0 +1,37 @@ +import { createSocket } from "node:dgram"; + +export interface TestDnsServer { + /** `host:port`, for a Resolver's setServers. */ + address: string; + close(): void; +} + +/** A DNS server: "silent" never replies, "answer" answers A with `ipv4`, "no-aaaa" answers A and drops AAAA. */ +export async function startTestDnsServer( + mode: "silent" | "answer" | "no-aaaa", + ipv4 = "127.0.0.1", +): Promise { + const socket = createSocket("udp4"); + socket.on("message", (msg, peer) => { + if (mode === "silent") return; + let end = 12; + while (msg[end] !== 0) end += (msg[end] ?? 0) + 1; + end += 5; + const isA = msg.readUInt16BE(end - 4) === 1; + if (mode === "no-aaaa" && !isA) return; + const header = Buffer.alloc(12); + msg.copy(header, 0, 0, 2); + header.writeUInt16BE(0x8180, 2); + header.writeUInt16BE(1, 4); + header.writeUInt16BE(isA ? 1 : 0, 6); + const answer = Buffer.from([0xc0, 0x0c, 0, 1, 0, 1, 0, 0, 0, 60, 0, 4]); + const record = Buffer.concat([answer, Buffer.from(ipv4.split(".").map(Number))]); + socket.send( + Buffer.concat([header, msg.subarray(12, end), ...(isA ? [record] : [])]), + peer.port, + peer.address, + ); + }); + await new Promise((resolve) => socket.bind(0, "127.0.0.1", resolve)); + return { address: `127.0.0.1:${socket.address().port}`, close: () => socket.close() }; +} diff --git a/packages/cli/src/utils/hostAnswers.test.ts b/packages/cli/src/utils/hostAnswers.test.ts index 6fc3558a92..4c08f5d610 100644 --- a/packages/cli/src/utils/hostAnswers.test.ts +++ b/packages/cli/src/utils/hostAnswers.test.ts @@ -1,5 +1,5 @@ -import { createSocket, type Socket } from "node:dgram"; import { afterEach, describe, expect, it, vi } from "vitest"; +import { startTestDnsServer, type TestDnsServer } from "./dnsTestServer.js"; // Point every Resolver hostAnswers creates at a local UDP server this test controls. const dnsServer = vi.hoisted(() => ({ address: "" })); @@ -14,38 +14,17 @@ vi.mock("node:dns", async (importOriginal) => { return { ...real, promises: { ...real.promises, Resolver } }; }); -let socket: Socket | undefined; +let server: TestDnsServer | undefined; -/** A DNS server: "silent" never replies, "answer" answers A with 127.0.0.1, "no-aaaa" answers A and drops AAAA. */ async function startServer(mode: "silent" | "answer" | "no-aaaa"): Promise { - socket = createSocket("udp4"); - socket.on("message", (msg, peer) => { - if (mode === "silent") return; - let end = 12; - while (msg[end] !== 0) end += (msg[end] ?? 0) + 1; - end += 5; - const isA = msg.readUInt16BE(end - 4) === 1; - if (mode === "no-aaaa" && !isA) return; - const header = Buffer.alloc(12); - msg.copy(header, 0, 0, 2); - header.writeUInt16BE(0x8180, 2); - header.writeUInt16BE(1, 4); - header.writeUInt16BE(isA ? 1 : 0, 6); - const record = Buffer.from([0xc0, 0x0c, 0, 1, 0, 1, 0, 0, 0, 60, 0, 4, 127, 0, 0, 1]); - socket?.send( - Buffer.concat([header, msg.subarray(12, end), ...(isA ? [record] : [])]), - peer.port, - peer.address, - ); - }); - await new Promise((resolve) => socket?.bind(0, "127.0.0.1", resolve)); - dnsServer.address = `127.0.0.1:${socket.address().port}`; + server = await startTestDnsServer(mode); + dnsServer.address = server.address; } describe("hostAnswers", () => { afterEach(() => { - socket?.close(); - socket = undefined; + server?.close(); + server = undefined; }); it("says no, within its short timeout, when DNS never replies", async () => { diff --git a/scripts/check-cli-process-ownership.mjs b/scripts/check-cli-process-ownership.mjs index 1f6a6ffb25..d82a675689 100644 --- a/scripts/check-cli-process-ownership.mjs +++ b/scripts/check-cli-process-ownership.mjs @@ -39,7 +39,9 @@ function listTypeScriptFiles(directory, roots) { return readdirSync(directory).flatMap((name) => { const path = join(directory, name); if (statSync(path).isDirectory()) return listTypeScriptFiles(path, roots); - if (!path.endsWith(".ts") || path.endsWith(".test.ts") || roots.includes(path)) return []; + if (!path.endsWith(".ts") || /\.(test|fixture)\.ts$/.test(path) || roots.includes(path)) { + return []; + } return [path]; }); } diff --git a/scripts/check-cli-process-ownership.test.mjs b/scripts/check-cli-process-ownership.test.mjs index 9452ab9e56..f4cd4ff7d2 100644 --- a/scripts/check-cli-process-ownership.test.mjs +++ b/scripts/check-cli-process-ownership.test.mjs @@ -1,6 +1,12 @@ import assert from "node:assert/strict"; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; import { describe, it } from "node:test"; -import { listDirectProcessTermination } from "./check-cli-process-ownership.mjs"; +import { + checkCliProcessOwnership, + listDirectProcessTermination, +} from "./check-cli-process-ownership.mjs"; describe("CLI process ownership checker", () => { it("reports direct process exit and exitCode writes", () => { @@ -25,4 +31,15 @@ describe("CLI process ownership checker", () => { [], ); }); + + it("lets a test fixture, spawned as its own process, exit", (t) => { + const root = mkdtempSync(join(tmpdir(), "hf-ownership-")); + t.after(() => rmSync(root, { recursive: true, force: true })); + mkdirSync(join(root, "packages/cli/src/utils"), { recursive: true }); + writeFileSync(join(root, "packages/cli/src/utils/a.fixture.ts"), "process.exit(0);\n"); + writeFileSync(join(root, "packages/cli/src/utils/a.ts"), "process.exit(0);\n"); + assert.deepEqual(checkCliProcessOwnership(root), [ + "packages/cli/src/utils/a.ts:1:1 uses process.exit", + ]); + }); });