From 2a2beeaf927ab5752e57232f99e9d4719187e587 Mon Sep 17 00:00:00 2001 From: Alexander Slaa Date: Thu, 3 Sep 2026 16:30:51 -0400 Subject: [PATCH] fix: broken ESM entry point, colliding net ids, and daemonize pidfile races - package.json/vite.config.ts: the ESM build output was named dist/index.es, an extension Node's loader doesn't recognize as JavaScript (ERR_UNKNOWN_FILE_EXTENSION on plain `import "node-qemu"`). Renamed to dist/index.mjs, unambiguously ESM regardless of any consumer's own package.json "type". - args.ts: every QemuNet entry without an explicit `id` defaulted to the literal "net0" regardless of position, so 2+ net entries collided with "Duplicate ID 'net0' for netdev". Now defaults to `net${index}`, mirroring how drives already default to `drive${index}`. - args.ts: a `virtio: true` drive never got `if=none` on its -drive clause, so QEMU's legacy "no if= means if=ide" behavior auto-attached it to an IDE device, and the explicit -device virtio-blk-pci attach then failed with "Drive 'driveN' is already in use ... did you need 'if=none'?". virtio drives now emit if=none. - manager.ts: with daemonize:true, the spawned process forks and exits almost immediately as part of normal, successful daemonization -- but the exit handler treated that exit like the real process dying and deleted config.pidfile, racing against (and usually winning against) QEMU's own -pidfile write for the actual detached daemon. Skip both the manager's own pre-write and the delete-on-exit when daemonize is set; QEMU owns that file exclusively in that mode. All four surfaced together while daemonizing a 3-NIC, 2-virtio-drive VM. Added regression coverage for each in tests/process/args.test.ts and tests/process/manager.test.ts. --- package.json | 4 ++-- src/process/args.ts | 22 +++++++++++++------ src/process/manager.ts | 15 +++++++++++-- tests/process/args.test.ts | 40 +++++++++++++++++++++++++++++++++++ tests/process/manager.test.ts | 30 ++++++++++++++++++++++++++ vite.config.ts | 2 +- 6 files changed, 102 insertions(+), 11 deletions(-) diff --git a/package.json b/package.json index 4f5eaec..3c40abe 100644 --- a/package.json +++ b/package.json @@ -4,11 +4,11 @@ "description": "QEMU management library for Node.js", "type": "module", "main": "./dist/index.cjs", - "module": "./dist/index.es", + "module": "./dist/index.mjs", "types": "./dist/index.d.ts", "exports": { ".": { - "import": "./dist/index.es", + "import": "./dist/index.mjs", "require": "./dist/index.cjs", "types": "./dist/index.d.ts" } diff --git a/src/process/args.ts b/src/process/args.ts index 6afa8df..b3ef38f 100644 --- a/src/process/args.ts +++ b/src/process/args.ts @@ -51,7 +51,15 @@ export function buildArgs(config: QemuConfig): string[] { (config.drives ?? []).forEach((drive, i) => { const id = drive.id ?? (drive.virtio ? `drive${i}` : undefined); - const parts: string[] = [`file=${drive.file}`, `format=${drive.format ?? "raw"}`]; + const parts: string[] = []; + // A virtio drive is attached to its virtio-blk-pci device by the + // separate -device clause below. Without if=none, QEMU's legacy + // "-drive without if= defaults to if=ide" behavior auto-attaches it + // to an IDE device too, and the drive ends up attached twice: QEMU + // rejects the second (virtio-blk-pci) attach with "Drive 'driveN' is + // already in use ... did you need 'if=none'?". + if (drive.virtio) parts.push("if=none"); + parts.push(`file=${drive.file}`, `format=${drive.format ?? "raw"}`); if (id) parts.push(`id=${id}`); if (drive.media) parts.push(`media=${drive.media}`); if (drive.cache) parts.push(`cache=${drive.cache}`); @@ -62,9 +70,7 @@ export function buildArgs(config: QemuConfig): string[] { if (drive.virtio) args.push("-device", `virtio-blk-pci,drive=${id}`); }); - for (const net of config.net ?? []) { - appendNet(args, net); - } + (config.net ?? []).forEach((net, i) => appendNet(args, net, i)); if (config.vnc) { let v = config.vnc.display; @@ -90,13 +96,17 @@ export function buildArgs(config: QemuConfig): string[] { return args; } -function appendNet(args: string[], net: QemuNet): void { +function appendNet(args: string[], net: QemuNet, index: number): void { if (net.type === "none") { args.push("-nic", "none"); return; } - const id = net.id ?? "net0"; + // Every net entry defaulted to the literal "net0" regardless of + // position, unlike drives' per-index default id -- multiple net + // entries without an explicit id collided with "Duplicate ID 'net0' + // for netdev". + const id = net.id ?? `net${index}`; if (net.type === "tap") { const parts = [`tap`, `id=${id}`]; diff --git a/src/process/manager.ts b/src/process/manager.ts index 42fe713..d1d8545 100644 --- a/src/process/manager.ts +++ b/src/process/manager.ts @@ -71,7 +71,13 @@ export class QemuProcess extends EventEmitter { this.proc = proc; - if (this.config.pidfile && proc.pid != null) { + // With daemonize:true, QEMU itself forks and writes config.pidfile + // (via the -pidfile arg in buildArgs) with the *real* detached + // daemon's pid once it's ready — this spawned wrapper process is + // just the launcher and its own pid is not the one that ends up + // running. Writing it here would race qemu's own write with the + // wrong pid, so skip it in that case. + if (this.config.pidfile && proc.pid != null && !this.config.daemonize) { await writeFile(this.config.pidfile, String(proc.pid), "utf8"); } @@ -84,7 +90,12 @@ export class QemuProcess extends EventEmitter { proc.on("exit", (code, signal) => { this.proc = null; - if (this.config.pidfile) { + // With daemonize:true, this "exit" is the launcher forking and + // returning — expected, successful, and unrelated to whether the + // real daemon (still running, detached) is alive. Deleting the + // pidfile here would delete the live daemon's own pidfile out + // from under it. + if (this.config.pidfile && !this.config.daemonize) { rm(this.config.pidfile, { force: true }).catch(() => {}); } this.emit("exit", code, signal); diff --git a/tests/process/args.test.ts b/tests/process/args.test.ts index dd7e2a8..ebb46a1 100644 --- a/tests/process/args.test.ts +++ b/tests/process/args.test.ts @@ -118,6 +118,46 @@ describe("buildArgs", () => { expect(args[idx + 1]).toContain("discard=unmap"); }); + it("adds if=none for a virtio drive, so the -device attach doesn't fight QEMU's implicit if=ide attach", () => { + const args = buildArgs({ + drives: [{ file: "/vm.qcow2", format: "qcow2", virtio: true }], + }); + const idx = args.indexOf("-drive"); + expect(args[idx + 1]).toContain("if=none"); + expect(args).toContain("-device"); + expect(args[args.indexOf("-device") + 1]).toBe("virtio-blk-pci,drive=drive0"); + }); + + it("does not add if=none for a non-virtio drive", () => { + const args = buildArgs({ + drives: [{ file: "/seed.img", format: "raw", media: "cdrom" }], + }); + const idx = args.indexOf("-drive"); + expect(args[idx + 1]).not.toContain("if=none"); + }); + + it("gives each net entry a distinct default id instead of always 'net0'", () => { + const args = buildArgs({ + net: [ + { type: "tap", ifname: "tap0", script: "no", downscript: "no" }, + { type: "tap", ifname: "tap1", script: "no", downscript: "no" }, + { type: "tap", ifname: "tap2", script: "no", downscript: "no" }, + ], + }); + const netdevs = args.filter((a) => a.startsWith("tap,id=")); + expect(netdevs).toEqual([ + "tap,id=net0,ifname=tap0,script=no,downscript=no", + "tap,id=net1,ifname=tap1,script=no,downscript=no", + "tap,id=net2,ifname=tap2,script=no,downscript=no", + ]); + const devices = args.filter((_, i) => args[i - 1] === "-device"); + expect(devices).toEqual([ + "virtio-net-pci,netdev=net0", + "virtio-net-pci,netdev=net1", + "virtio-net-pci,netdev=net2", + ]); + }); + it("adds tap network", () => { const args = buildArgs({ net: [{ type: "tap", id: "net0", ifname: "tap0", script: "no", downscript: "no" }], diff --git a/tests/process/manager.test.ts b/tests/process/manager.test.ts index f22d7b0..8f9445b 100644 --- a/tests/process/manager.test.ts +++ b/tests/process/manager.test.ts @@ -5,6 +5,7 @@ vi.mock("node:child_process", () => ({ spawn: vi.fn() })); vi.mock("node:fs/promises", () => ({ writeFile: vi.fn().mockResolvedValue(undefined), rm: vi.fn().mockResolvedValue(undefined) })); import { spawn } from "node:child_process"; +import { writeFile, rm } from "node:fs/promises"; import { QemuProcess } from "../../src/process/manager.js"; class MockProcess extends EventEmitter { @@ -88,6 +89,35 @@ describe("QemuProcess", () => { expect(spy).toHaveBeenCalledWith(0, null); }); + it("writes the pidfile itself when not daemonized", async () => { + const mock = new MockProcess(); + vi.mocked(spawn).mockReturnValue(mock as unknown as ReturnType); + const proc = new QemuProcess({ config: { pidfile: "/run/vm.pid" } }); + await proc.start(); + expect(writeFile).toHaveBeenCalledWith("/run/vm.pid", "12345", "utf8"); + }); + + it("deletes the pidfile on exit when not daemonized", async () => { + const mock = new MockProcess(); + vi.mocked(spawn).mockReturnValue(mock as unknown as ReturnType); + const proc = new QemuProcess({ config: { pidfile: "/run/vm.pid" } }); + await proc.start(); + mock.emit("exit", 0, null); + expect(rm).toHaveBeenCalledWith("/run/vm.pid", { force: true }); + }); + + it("does not touch the pidfile when daemonized, since QEMU itself owns it after forking", async () => { + const mock = new MockProcess(); + vi.mocked(spawn).mockReturnValue(mock as unknown as ReturnType); + const proc = new QemuProcess({ config: { pidfile: "/run/vm.pid", daemonize: true } }); + await proc.start(); + expect(writeFile).not.toHaveBeenCalled(); + // The wrapper process exiting (code 0) is normal daemonize behavior, + // not the real daemon dying -- must not delete the daemon's pidfile. + mock.emit("exit", 0, null); + expect(rm).not.toHaveBeenCalled(); + }); + it("implements Symbol.asyncDispose", () => { expect(typeof makeProc()[Symbol.asyncDispose]).toBe("function"); }); diff --git a/vite.config.ts b/vite.config.ts index 1edf97e..4ae2ab3 100644 --- a/vite.config.ts +++ b/vite.config.ts @@ -7,7 +7,7 @@ export default defineConfig({ lib: { entry: "src/index.ts", formats: ["es", "cjs"], - fileName: (format) => format === "es" ? "index.es" : "index.cjs", + fileName: (format) => format === "es" ? "index.mjs" : "index.cjs", }, rollupOptions: { external: [/^node:/],