Run on Node 24 - #6307
Open
francoisferrand wants to merge 10 commits into
Open
Run on Node 24#6307francoisferrand wants to merge 10 commits into
francoisferrand wants to merge 10 commits into
Conversation
Contributor
Hello francoisferrand,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
... and 3 files with indirect coverage changes @@ Coverage Diff @@
## development/9.5 #6307 +/- ##
===================================================
- Coverage 86.60% 86.52% -0.09%
===================================================
Files 213 213
Lines 14620 14619 -1
===================================================
- Hits 12662 12649 -13
- Misses 1958 1970 +12
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
francoisferrand
force-pushed
the
improvement/CLDSRV-996-node-24
branch
2 times, most recently
from
September 25, 2026 09:56
e65085d to
4d4bdeb
Compare
Contributor
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
francoisferrand
force-pushed
the
improvement/CLDSRV-996-node-24
branch
from
September 25, 2026 16:04
1990fdb to
01f506f
Compare
Bump the Dockerfile base image and the CI node-version pins (lint, tests, and the shared setup-ci action) from 22 to 24, ahead of raising the actual engines floor once the remaining dependency blockers are cleared. Bump the nan resolution from 2.22.0 to 2.23.0: it doesn't support Node 24's V8/ABI, which made the ioctl optionalDependency silently fail to build (yarn masks this as a harmless warning). Bump bucketclient to 8.2.10, which now declares arsenal as a peerDependency instead of pulling in its own arsenal (and the aws-sdk v2/diskusage that come with it) as a full dependency. With both fixed, engines.node can move to >=24 and the --ignore-engines flag can come off the install steps. Track the 24 major in CI rather than a patch pin, so runs pick up security patches without a manual bump. The node_modules cache key already uses the version setup-node resolves, so it invalidates on its own when the patch moves. Issue: CLDSRV-996
8.5.8 predates the Node 24 fixes; pin to the current tip of the unmerged improvement/VLTCLT-69 branch (116f6811a6d80a55fbfd5d682185222b8f0b1cd8, version 8.5.9) until it's tagged and released. No tagged release exists yet with these fixes. Issue: CLDSRV-996
Bump a curated set of dependencies to the latest version already allowed by their existing semver ranges: bufferutil, eslint-plugin-import, eslint-plugin-promise, express, ioredis, mocha, mocha-multi-reporters, moment, node-forge, node-mocks-http, nodemon, utf-8-validate, uuid, ws, and @azure/storage-blob. All patch/minor bumps within declared ranges, no package.json range changes needed. Left out anything that would pull a large multi-minor jump (AWS SDK v3 packages, mongodb, eslint) or risk reformatting the codebase (prettier), since those need their own dedicated review. Issue: CLDSRV-996
The VLTCLT-69 Node 24 fixes are now tagged and released as 8.5.9, so pin to that tag instead of the unmerged branch-tip commit hash used previously. Issue: CLDSRV-996
Picks up ARSN-642 (parseRequestTarget helper, needed to replace url.parse() on client-supplied paths) and arsenal's own Node 24 dependency prep. No stable 8.6.0 tag exists yet, but this preview tag is a tagged, immutable ref. Use arsenal's parseRequestTarget for x-amz-copy-source parsing, as url.parse() is deprecated and its WHATWG-style normalization (dot-segment collapsing, // read as authority) can change what object a client-supplied copy source actually addresses. Arsenal's parseRequestTarget parses the header as an opaque path instead, matching the semantics cloudserver relies on. Issue: CLDSRV-996
Verified with isolated builds on Node 24 that arsenal's ioctl native module compiles fine against nan 2.23.0 (the version yarn already resolves) with or without this pin, so it's not doing anything for us here. Note: utapi still bundles its own resolutions.nan: v2.22.0 in its own package.json, which breaks its diskusage build on Node 24 the same way. That's isolated inside utapi's git-dependency build step and can't be fixed from cloudserver's resolutions field at all - it needs a utapi release with the fix (branch improvement/UTAPI-125-node24 upstream, not tagged yet). Documented as a blocker in the PR. Issue: CLDSRV-996
utapi's earlier releases (8.2.4, 8.3.1) bundle their own resolutions.nan
pin (v2.22.0) inside their isolated git-dependency build, which cannot
be overridden from cloudserver's package.json and breaks diskusage's
native build on Node 24 -- a clean 'yarn install --frozen-lockfile'
hard-fails (exit 1) on Node 24 with those tags.
8.4.0 ships the UTAPI-125 fix, so pin to the tag as usual.
Verified with a clean Node 24 yarn install: require('ioctl'),
require('diskusage'), and require('utapi') all succeed afterwards.
Issue: CLDSRV-996
Both overrides were added when ts-morph used older internal deps (minimatch@3, picomatch@2) to force a fix for old CVEs. ts-morph is now on ^28.0.0, whose minimatch@10/tinyglobby already require brace-expansion@^5.0.2 and picomatch@^4.0.3 -- ranges that dedupe naturally into the same up-to-date versions the rest of the tree already uses (5.0.12 and 4.0.4/4.0.7), with or without the override. Confirmed by force-clearing the affected yarn.lock entries and letting yarn re-resolve from scratch: same versions come back, no duplicate/older copies. jsonwebtoken and fast-xml-parser resolutions are unrelated and still needed (they override genuinely outdated, vulnerable versions pinned by oas-tools and various aws-sdk xml-builder packages), so they're left untouched. Issue: CLDSRV-996
Signing needs the decoded path while the request must keep the encoded one, which previously meant temporarily overwriting req.path and putting it back. On Node >= 24 that collides with ClientRequest's path setter rejecting raw multi-byte UTF-8, so it needed a property-shadowing hack to bypass the check, and the restore never really unshadowed it. Pass arsenal a delegate object carrying the decoded path instead. The real request is never mutated, so there is nothing to restore; header mutations still reach it through the prototype chain. Signatures are unchanged: verified byte-identical authorization headers against the previous approach, with and without caller-supplied headers. Issue: CLDSRV-996
francoisferrand
force-pushed
the
improvement/CLDSRV-996-node-24
branch
from
September 25, 2026 16:48
01f506f to
4a12afc
Compare
francoisferrand
marked this pull request as ready for review
September 25, 2026 16:48
The dependency refresh split express into two resolutions: the dev-only
^4.21.1 range moved to 4.22.3 and stayed at the root, while utapi's
^4.21.2 remained on 4.21.2 and got nested. A production install drops
the dev-only root copy, so root-hoisted oas-tools -- which requires
express without declaring it -- could no longer resolve it, and every
cloudserver process died on require('utapi') before binding port 8000.
oas-tools declares express in devDependencies, which yarn never installs
for a transitive package, so it has no edge to place express against:
the require only ever worked through a single copy hoisted at the root.
Nothing makes that hold on its own -- rerunning the install on the split
lockfile keeps the two ranges on separate versions -- hence the
resolution, which forces the whole tree onto one express.
Issue: CLDSRV-996
francoisferrand
force-pushed
the
improvement/CLDSRV-996-node-24
branch
from
September 26, 2026 09:35
4a12afc to
d454d68
Compare
SylvainSenechal
approved these changes
Sep 28, 2026
delthas
approved these changes
Sep 28, 2026
| "utf8": "^3.0.0", | ||
| "uuid": "^11.0.3", | ||
| "vaultclient": "scality/vaultclient#116f6811a6d80a55fbfd5d682185222b8f0b1cd8", | ||
| "vaultclient": "scality/vaultclient#8.5.9", |
Contributor
There was a problem hiding this comment.
Could we squash this into the previoius bump commit? (To avoid having a bump to a hash in the history)
This branch has not been deployed
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
4 participants
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.
Move cloudserver to Node 24, as part of the OS-1155 Node 22 -> 24 upgrade epic. CI and the Dockerfiles build and test on 24.21.0, and
engines.nodeis raised to>=24.Most of the work is in the dependencies, since several of them don't build or run on 24:
diskusagemodule into our install.--ignore-enginesfrom the install steps: it was hiding these failures rather than telling us about them.resolutionsentries that are no longer doing anything (nan, and thets-morphoverrides).Two code changes were needed for 24:
parseCopySourceuses arsenal'srequestUrl.parseRequestTargetinstead ofurl.parse(). Beyond the deprecation,url.parse()normalizes the path, so a client-suppliedx-amz-copy-sourcecould end up addressing a different object than it says.req.pathin place, which Node 24 rejects for non-ASCII paths.expressresolution to keep a single copy at the root.oas-toolsneeds it without declaring it, and the version pull from utapi could drift form our dev-only version and cause duplicate package, causing runtime failure.Also refreshed
yarn.lockwith the patch/minor bumps already allowed by our ranges. Anything needing real review on its own (AWS SDK v3, mongodb, eslint, prettier) is left for later.Issue: CLDSRV-996