fix: broken ESM entry, colliding net ids, daemonize pidfile races - #1
Merged
Merged
Conversation
… 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.
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Four bugs found together while using this library to daemonize 3-NIC,
2-virtio-drive VMs from a real provisioning script (nested-VM cluster PoC):
exports["."].importpointed atdist/index.es, an extension Node'sESM loader doesn't recognize -- plain
import "node-qemu"fails withERR_UNKNOWN_FILE_EXTENSION. The package was unusable viaimportaspublished on npm (1.2.1), only via
require. Renamed the ESM buildoutput to
dist/index.mjs.netentry without an explicitiddefaulted to the literal"net0"regardless of position (unlikedrives, which alreadyindexes its default id as
drive${i}), so 2+ net entries collidedwith
Duplicate ID 'net0' for netdev.virtio: truedrive never gotif=noneon its-driveclause, soQEMU's legacy "no
if=meansif=ide" auto-attach fought the explicit-device virtio-blk-pciattach:Drive 'driveN' is already in use ... did you need 'if=none'?.daemonize: true, the spawned process forks and exits almostimmediately as part of normal, successful daemonization -- but the
exithandler treated that exit like the real process dying anddeleted
config.pidfile, racing against (and usually winning against)QEMU's own
-pidfilewrite for the actual detached daemon. Both themanager's own pre-write and the delete-on-exit are now skipped when
daemonizeis set; QEMU owns that file exclusively in that mode.Test plan
npx vitest run-- 101/102 pass; the 1 remaining failure(
tests/image/qemu-img.test.ts, arequire()of a.tspath)pre-exists on
mainunrelated to this change, confirmed viagit stash.args.ts/manager.tsfixes.npx vite buildthennode -e "import('./dist/index.mjs')..."--resolves and exposes all expected exports.
qemu-system-x86_64VM with the patched build; boots, all 3 tapscome up with correct MACs, SSH reachable, pidfile stable after the
launcher process exits.
Note:
package-lock.jsonis untouched -- installing locally with npm9.2.0 (this sandbox's version) rewrote unrelated
libcmetadata acrossthe lockfile; reverted that noise before committing.