Repository navigation
Keep each VM on the JDK image file it started with - #39
Conversation
…e cannot mix two images
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 38 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
There was a problem hiding this comment.
The PR fixes a bug where VMs would read the wrong JDK image file after an in-place upgrade. Previously, imageFd cached file descriptors by path only, so a replaced file (same path, new inode) would be read through the old descriptor. Now imageFd keys by path and inode, and each backend (ImageBackend, NodeBackend) retains its own descriptor per image file, so a VM continues reading the image it started with. The new test verifies that a backend created before a rename keeps reading the old file while a new backend reads the new file.
review-bot, model nvidia/nemotron-3-ultra-550b-a55b:free, verdict approve
There was a problem hiding this comment.
The PR fixes a bug where in-place JDK image upgrades caused VMs to read wrong bytes. The imageFd cache now keys descriptors by path and inode, so a replaced file gets a new descriptor. Each ImageBackend and NodeBackend instance caches its own descriptor at first use, so a VM continues reading the image it started with. A new test verifies that a backend created before a rename keeps reading the old file while a new backend reads the new file. The known limitation (descriptors accumulate per upgrade) is acknowledged.
review-bot, model nvidia/nemotron-3-ultra-550b-a55b:free, verdict approve
There was a problem hiding this comment.
The PR fixes a bug where in-place JDK image upgrades caused VMs started after the upgrade to read the new boot.pack index through the old image file's descriptor, returning wrong class bytes. The fix keys the global descriptor cache by path+inode so replaced files get new descriptors, and each backend (ImageBackend, NodeBackend) caches its own descriptor per image file at first use so a VM reads the image it started with. A new test verifies the behavior. The known trade-off (descriptors accumulate across upgrades) is documented.
review-bot, model nvidia/nemotron-3-ultra-550b-a55b:free, verdict approve
There was a problem hiding this comment.
The PR fixes a bug where an in-place JDK image upgrade would cause VMs started after the upgrade to read the new boot.pack index through the old image file's descriptor, returning wrong class bytes. The fix keys the global descriptor cache by (path, inode) so a replaced file gets a new descriptor, and each backend instance caches its own descriptor at first use so a VM reads the image it started with for its lifetime. A new test verifies the old backend keeps reading the old file after a rename upgrade while a new backend reads the new file.
review-bot, model nvidia/nemotron-3-ultra-550b-a55b:free, verdict approve
Since #37, Node reads the JDK image (
build/jdk/lib/modules, orjdk/image.jarin the package) through descriptors that stay open. They were shared by every VM in the process and keyed only by path.So if an upgrade replaced the package in place (a new
image.jarandboot.packrenamed over the old ones) while a server process kept running, a VM started after the upgrade would read the newboot.packindex with offsets into the new image, but through the descriptor of the old, replaced file. That returns the wrong bytes for every class not in a pack.imageFdnow keys open descriptors by path and inode, so a replaced file gets a new descriptor.ImageBackend, andNodeBackendforbuild/jdk) takes its descriptor once and keeps it, so a VM reads the image it started with to the end. The replaced file stays readable for it, as an open file does on POSIX.Each image file a process actually runs on stays open once, as before.
Checks:
test/image.test.ts: an image file renamed over the old one is read by a new backend, while one created before keeps reading the old file. With the old cache, the new backend read the old file's bytes.pnpm typecheckandpnpm testpass (145 tests).Noticed, not fixed: