fix(desktop): harden before-pack cleanup against the same non-ASCII rmSync no-op
Review follow-ups to the previous commit: - before-pack.mjs: cleanStaleAppOutDir kept the retrying rmSync (its EBUSY resilience is worth keeping) but now verifies the tree is gone and falls back to the libuv-backed removeDirSync when the native rmSync silently no-ops on a non-ASCII path — previously it logged 'removed stale unpacked dir' while the stale tree survived. - removeDirSync: export it for reuse; handle a plain file or symlink at the path (rm -rf semantics) instead of throwing ENOTDIR, and tolerate broken symlinks via lstat. - Add a source-level guard test: repo CI runs on Linux where the native implementations happen to work, so the accented staging test cannot catch a cpSync/rmSync reintroduction there. - Link the upstream Node issues in the banner (nodejs/node#61878, fixed in v24.15.0; nodejs/node#56049, fixed in v24.13.1) and stop attributing the native rewrite to Node 24 — only the observation was on v24.11.1. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
@@ -60,7 +60,7 @@
|
||||
import { existsSync, rmSync, renameSync } from 'node:fs'
|
||||
import path from 'node:path'
|
||||
import { Arch } from 'electron-builder'
|
||||
import { stageNodePty, stageGetWindows } from './stage-native-deps.mjs'
|
||||
import { removeDirSync, stageNodePty, stageGetWindows } from './stage-native-deps.mjs'
|
||||
|
||||
export function cleanStaleAppOutDir(appOutDir) {
|
||||
if (!appOutDir || typeof appOutDir !== 'string') {
|
||||
@@ -73,6 +73,13 @@ export function cleanStaleAppOutDir(appOutDir) {
|
||||
// can't block the wipe. retry/maxRetries rides out transient EBUSY on
|
||||
// Windows where an AV/indexer may briefly hold a handle.
|
||||
rmSync(appOutDir, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 })
|
||||
// Node's native rmSync silently deletes nothing on non-ASCII Windows
|
||||
// paths (nodejs/node#56049, fixed in v24.13.1) — without this check the
|
||||
// stale tree survives and the "removed" log below lies. Fall back to
|
||||
// the libuv-backed walk, which handles those paths on every version.
|
||||
if (existsSync(appOutDir)) {
|
||||
removeDirSync(appOutDir)
|
||||
}
|
||||
return true
|
||||
}
|
||||
|
||||
|
||||
@@ -16,6 +16,7 @@ import {
|
||||
chmodSync,
|
||||
copyFileSync,
|
||||
existsSync,
|
||||
lstatSync,
|
||||
mkdirSync,
|
||||
readdirSync,
|
||||
readFileSync,
|
||||
@@ -36,14 +37,17 @@ function makeExecutable(filePath) {
|
||||
|
||||
// ─── libuv-safe fs primitives ────────────────────────────────────────
|
||||
//
|
||||
// Node 24's native rewrite of fs.cpSync/fs.rmSync mishandles non-ASCII
|
||||
// Windows paths (observed on v24.11.1 with an accented Windows user name,
|
||||
// i.e. a default %LOCALAPPDATA%\hermes home): a recursive cpSync fails
|
||||
// with EIO "Access is denied" or hard-crashes the process, an overwriting
|
||||
// cpSync fails with a bogus errno-0 unlink error, and rmSync silently
|
||||
// deletes nothing — leaving a half-staged tree that breaks every retry.
|
||||
// copyFileSync/unlinkSync/rmdirSync/readdirSync go through libuv and
|
||||
// handle those paths correctly, so staging uses them exclusively.
|
||||
// Node's native (non-libuv) rewrite of fs.cpSync/fs.rmSync mishandles
|
||||
// non-ASCII Windows paths (observed on v24.11.1 with an accented Windows
|
||||
// user name, i.e. a default %LOCALAPPDATA%\hermes home): a recursive
|
||||
// cpSync fails with EIO "Access is denied" or hard-crashes the process,
|
||||
// an overwriting cpSync fails with a bogus errno-0 unlink error, and
|
||||
// rmSync silently deletes nothing — leaving a half-staged tree that
|
||||
// breaks every retry. Fixed upstream (nodejs/node#61878 → v24.15.0;
|
||||
// nodejs/node#56049 → v24.13.1), but the installer builds on whatever
|
||||
// Node the user already has, so staging sticks to libuv-backed
|
||||
// primitives (copyFileSync/unlinkSync/rmdirSync/readdirSync), which
|
||||
// handle those paths correctly on every affected version.
|
||||
|
||||
/** Recursively copy a directory without fs.cpSync. */
|
||||
function copyDirSync(srcDir, destDir) {
|
||||
@@ -60,12 +64,25 @@ function copyDirSync(srcDir, destDir) {
|
||||
}
|
||||
|
||||
/**
|
||||
* Recursively delete a directory without fs.rmSync (missing dir is fine),
|
||||
* then verify the tree is actually gone — a silent no-op here surfaces
|
||||
* later as an inexplicable staging failure, so fail loudly instead.
|
||||
* Recursively delete a path without fs.rmSync — missing paths are fine,
|
||||
* a plain file or symlink at the path is unlinked (rm -rf semantics).
|
||||
* Verifies the tree is actually gone afterwards: a silent no-op here
|
||||
* surfaces later as an inexplicable staging failure, so fail loudly.
|
||||
*
|
||||
* Also used by before-pack.mjs as the fallback when the native rmSync
|
||||
* silently leaves the stale unpacked dir behind.
|
||||
*/
|
||||
function removeDirSync(dir) {
|
||||
if (!existsSync(dir)) return
|
||||
export function removeDirSync(dir) {
|
||||
let stats
|
||||
try {
|
||||
stats = lstatSync(dir)
|
||||
} catch {
|
||||
return
|
||||
}
|
||||
if (!stats.isDirectory()) {
|
||||
unlinkSync(dir)
|
||||
return
|
||||
}
|
||||
for (const entry of readdirSync(dir, { withFileTypes: true })) {
|
||||
const full = join(dir, entry.name)
|
||||
if (entry.isDirectory()) {
|
||||
|
||||
@@ -348,6 +348,19 @@ test.skipIf(process.platform === 'win32')(
|
||||
// This test stages into an accented src/dest tree — twice, so the
|
||||
// restage exercises the delete-then-recopy path — to keep it that way.
|
||||
|
||||
test('non-ASCII paths: staging never calls fs.cpSync/fs.rmSync', () => {
|
||||
// The Node bug is Windows-only while CI runs on Linux, so the accented
|
||||
// staging test below cannot catch a reintroduction there. Guard at the
|
||||
// source level instead: no cpSync/rmSync call may come back into the
|
||||
// staging path.
|
||||
const source = fs.readFileSync(new URL('./stage-native-deps.mjs', import.meta.url), 'utf8')
|
||||
assert.doesNotMatch(
|
||||
source,
|
||||
/(?:cpSync|rmSync)\(/,
|
||||
'stage-native-deps.mjs must stay on libuv-backed fs primitives (see the banner comment)'
|
||||
)
|
||||
})
|
||||
|
||||
test('non-ASCII paths: staging into an accented tree works and restages cleanly', () => {
|
||||
const tmp = fs.mkdtempSync(join(os.tmpdir(), 'hermes-stage-'))
|
||||
try {
|
||||
|
||||
Reference in New Issue
Block a user