fix: narrows engines.node to the runtime the shipped binary targets - #12
Open
eastagiletracker wants to merge 1 commit into
Open
Conversation
binding/muhammara.node.zip has shipped the node-v127 (Node 22) prebuild
since node 22 support was added, but engines.node still allowed Node
20.18.1 and up. On a nodejs20.x Lambda the copy step in the README
therefore produces a module that cannot load: require("muhammara") fails
with ERR_DLOPEN_FAILED, NODE_MODULE_VERSION 127 vs 115.
Narrows engines.node (and its lockfile mirror) to ">= 22.0.0 <23", which
matches .node-version, the package description and LAMBDA_NODE_VERSION in
scripts/release.js.
install.js now reads the NODE_MODULE_VERSION back out of the unpacked
binary and compares it to the runtime engines.node declares, so the two
cannot drift apart unnoticed again. The check runs where install.js
already runs -- `npm ci`, i.e. step 3 of the release script -- and not on
consumer installs, where npm does not run a dependency's prepare script.
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.
This PR proposes narrowing
engines.nodeto the Node 22 runtime the shipped binary is actually built for, and havinginstall.jsverify that the two agree. We include this PR work along with a full history of your repo at https://eastagiletracker.com/projects/335. You can sign in with your GitHub ID to claim ownership of the project.What's wrong
binding/muhammara.node.ziphas contained thenode-v127prebuild since node 22 support landed — the binary exportsnode_register_module_v127, so it only loads on Node 22. Butengines.nodestill reads">= 20.18.1 <23", which is the bound from the Node 20 era. Every other place that names a runtime already says 22: thedescriptionfield,.node-version, the README version history, andLAMBDA_NODE_VERSIONinscripts/release.js.The consequence is that
lambda-muhammaraadvertises support for a runtime it cannot run on. Onnodejs20.xthe install succeeds, the copy step from the README succeeds, the deploy succeeds — and then the function dies on its first invocation. Reproduced against currentmain(the binary in the tree at63c6ba2is the samenode-v127build), using the Lambda base image for a runtimeengines.nodecurrently permits:The same commands against
public.ecr.aws/lambda/nodejs:22printmuhammara OK on v22.23.2.The change
engines.nodebecomes">= 22.0.0 <23"inpackage.jsonand in itspackage-lock.jsonmirror, so the declared range matches the prebuild inbinding/and the runtime the rest of the repo names.install.jsnow reads theNODE_MODULE_VERSIONback out of the unpacked binary — a compiled addon exports it asnode_register_module_v<N>— and compares it against the major thatengines.nodedeclares, using the same ABI tableMAINTAINERS.mdsends you to when picking a prebuild. That is the invariantscripts/release.jscurrently asks a human to hold ("Keep in sync with the runtime named in package.json'sdescription"), and it drifted once already. A mismatch is now a hard error naming the prebuild to ship; an unrecognised Node major or a binary with no such symbol warns and carries on, so the check can never block a release it does not understand.How it was verified
npm cion Node 22.23.2 with npm 12.0.2 is green both before and after — same result, no new failures — and now printsBinary targets Node 22 (NODE_MODULE_VERSION 127). The evidence that the check works is that it fails on the tree as it stands today: applying only theinstall.jshalf, without touchingengines.node, exits 1 withbinding/muhammara.node is a NODE_MODULE_VERSION 127 build, but package.json declares Node 20 (NODE_MODULE_VERSION 115). With theengines.nodecorrection applied it passes. Its other branches were exercised directly: declaring Node 24 fails and asks for thenode-v137prebuild, an unknown major and a non-addon file both warn and exit 0, and a missing zip now reports one clean line instead of an unhandled string rejection.Beyond the test tree,
npm packfrom this branch produces the same six-file artifact as before, and that tarball run through the Docker sanity check inMAINTAINERS.mdloads onpublic.ecr.aws/lambda/nodejs:22and writes a real PDF.Two notes on the parts that touch things you own. The new check cannot affect anyone installing from npm:
prepareis not run for a dependency's tarball, which I confirmed withnpm install --foreground-scriptsagainst the packed build — the unzippedbinding/muhammara.nodeis already in the tarball, so the README copy step is untouched. And narrowing the lower bound does tighten who can install from a Node 20 or 21 machine, which is a judgement call rather than a fact: it is consistent with howengines.nodehas tracked the target Lambda runtime for this package's whole history, and with the<23ceiling andengine-strict=trueyou already have, but if you would ratherengines.nodedescribe the developer's machine, I am glad to drop that hunk and keep only the check plus a correcteddescription.One thing spotted in passing and deliberately left alone:
mainpoints atinstall.js, which requiresextract-zip, a devDependency — sorequire("lambda-muhammara")throws on a consumer install. Nothing documents that entry point, so it is out of scope here.How this was managed
This work was tracked as a single story, engines.node still permits Node 20/21, but the shipped binary is a Node 22 (ABI 127) build, on a board at https://eastagiletracker.com/projects/335 imported from this repository's own issues and pull requests — 11 stories, with the merged pull requests sitting in the Done iterations they were merged in.
If you'd rather not receive contributions like this, reply
no-more-prson this pull request and we won't open any further ones on your repositories.Lawrence W. Sinclair
CEO / East Agile
linkedin.com/in/lwsinclair/
eastagile.com