From 76cd29a857a93c525826e8701d40ce250ceeaaed Mon Sep 17 00:00:00 2001 From: yaojin3616 Date: Fri, 21 Aug 2026 19:03:49 +0800 Subject: [PATCH 01/14] fix(market): upgrade dshmarket to 1.15.0 --- packages/dsh-desktop-market-installer/client.js | 4 ++-- packages/dsh-desktop-market-installer/index.js | 2 +- test/market-installer.test.js | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/dsh-desktop-market-installer/client.js b/packages/dsh-desktop-market-installer/client.js index 6391f752..2176637a 100644 --- a/packages/dsh-desktop-market-installer/client.js +++ b/packages/dsh-desktop-market-installer/client.js @@ -501,7 +501,7 @@ window.__ModuleLoader__.load({ setStatus((current) => ({ ...current, phase: 'installing', - recommendedVersion: current?.recommendedVersion || '1.9.0' + recommendedVersion: current?.recommendedVersion || '1.15.0' })) try { const response = await fetch(INSTALL_PATH, { @@ -539,7 +539,7 @@ window.__ModuleLoader__.load({ const busy = phase === 'installing' const installed = phase === 'installed' const failed = phase === 'error' || phase === 'incomplete' || Boolean(error) - const version = status?.recommendedVersion || '1.9.0' + const version = status?.recommendedVersion || '1.15.0' return React.createElement( 'section', diff --git a/packages/dsh-desktop-market-installer/index.js b/packages/dsh-desktop-market-installer/index.js index eff358c7..fcc320d1 100644 --- a/packages/dsh-desktop-market-installer/index.js +++ b/packages/dsh-desktop-market-installer/index.js @@ -5,7 +5,7 @@ import { createRequire } from 'node:module' import { homedir } from 'node:os' import { delimiter, dirname, join, resolve } from 'node:path' -export const RECOMMENDED_MARKET_VERSION = '1.9.0' +export const RECOMMENDED_MARKET_VERSION = '1.15.0' export const MARKET_PACKAGE = 'dshmarket' export const MARKET_PROFILE = 'web' export const STATUS_PATH = '/dsh-desktop/market-installer/status' diff --git a/test/market-installer.test.js b/test/market-installer.test.js index ab4ebcc7..2f4f6eee 100644 --- a/test/market-installer.test.js +++ b/test/market-installer.test.js @@ -26,10 +26,10 @@ describe('desktop plugin market installer', () => { 'web', 'add', '--save-exact', - 'dshmarket@1.9.0' + 'dshmarket@1.15.0' ]) expect(MARKET_PACKAGE).toBe('dshmarket') - expect(RECOMMENDED_MARKET_VERSION).toBe('1.9.0') + expect(RECOMMENDED_MARKET_VERSION).toBe('1.15.0') expect(STATUS_PATH).toBe('/dsh-desktop/market-installer/status') expect(INSTALL_PATH).toBe('/dsh-desktop/market-installer/install') expect(UNINSTALL_PATH).toBe('/dsh-desktop/market-installer/uninstall') From 5733b857098b2dafcf323addf5664ef9948d4a65 Mon Sep 17 00:00:00 2001 From: yaojin3616 Date: Fri, 21 Aug 2026 05:40:06 -0700 Subject: [PATCH 02/14] fix(darwin): only disclaim utility process TCC in packaged builds --- src/main/index.ts | 2 +- src/main/runtime/disclaimed-utility-process.ts | 12 +++++++----- test/runtime.test.ts | 4 ++++ 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/src/main/index.ts b/src/main/index.ts index ac510712..a0fc5439 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -944,7 +944,7 @@ async function bootstrap(): Promise { logPath: join(app.getPath('logs'), 'harness.log'), launchProcess: (executablePath, args, options) => process.platform === 'darwin' - ? launchDisclaimedUtilityProcess(utilityProcess, args, options) + ? launchDisclaimedUtilityProcess(utilityProcess, args, options, app.isPackaged) : spawn(executablePath, args, options), onChanged: (snapshot) => { if (snapshot.phase === 'ready' && snapshot.url) { diff --git a/src/main/runtime/disclaimed-utility-process.ts b/src/main/runtime/disclaimed-utility-process.ts index c72d0856..e6cb737a 100644 --- a/src/main/runtime/disclaimed-utility-process.ts +++ b/src/main/runtime/disclaimed-utility-process.ts @@ -16,7 +16,8 @@ export interface DisclaimedUtilityProcessSpec { export function buildDisclaimedUtilityProcessSpec( nodeArguments: readonly string[], - spawnOptions: SpawnOptionsWithoutStdio + spawnOptions: SpawnOptionsWithoutStdio, + disclaim = true ): DisclaimedUtilityProcessSpec { const [internalLoaderFlag, modulePath, ...args] = nodeArguments if (internalLoaderFlag !== '--expose-internals' || !modulePath) { @@ -38,8 +39,8 @@ export function buildDisclaimedUtilityProcessSpec( stdio: 'pipe', serviceName: 'DSH Harness', // Harness loads user-installed plugins and can launch third-party tools. - // Keep their TCC requests out of DSH Desktop's responsibility chain. - disclaim: true + // Keep their TCC requests out of DSH Desktop's responsibility chain in production. + disclaim } } } @@ -47,9 +48,10 @@ export function buildDisclaimedUtilityProcessSpec( export function launchDisclaimedUtilityProcess( launcher: UtilityProcessLauncher, nodeArguments: readonly string[], - spawnOptions: SpawnOptionsWithoutStdio + spawnOptions: SpawnOptionsWithoutStdio, + disclaim = true ): HarnessChildProcess { - const spec = buildDisclaimedUtilityProcessSpec(nodeArguments, spawnOptions) + const spec = buildDisclaimedUtilityProcessSpec(nodeArguments, spawnOptions, disclaim) return new UtilityProcessAdapter( launcher.fork(spec.modulePath, spec.args, spec.options) ) diff --git a/test/runtime.test.ts b/test/runtime.test.ts index 2870eb53..c837cfda 100644 --- a/test/runtime.test.ts +++ b/test/runtime.test.ts @@ -152,6 +152,10 @@ describe('Harness launch contract', () => { disclaim: true } }) + + expect( + buildDisclaimedUtilityProcessSpec(nodeArguments, spawnOptions, false).options.disclaim + ).toBe(false) }) it('rejects an unexpected macOS Harness argument layout', () => { From be527faccf23299a5fe47c9e21161b28ad23ca6a Mon Sep 17 00:00:00 2001 From: yaojin3616 Date: Fri, 21 Aug 2026 06:08:59 -0700 Subject: [PATCH 03/14] fix(darwin): keep Harness children addressable as Node MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On macOS Harness runs inside an Electron utility process, so its process.execPath and argv0 point at the Electron helper rather than a Node binary. Plugins re-invoke the dsh CLI through the executable running them — dsh-market forwards process.execArgv with it — and that child booted as an Electron app, where the leading --expose-internals shifts argv and the CLI answers "error: --profile is required". Updating a plugin from the market therefore always failed on macOS ("Update failed: dshmarket"). The flag cannot travel in the Harness process environment: the utility process is launched with Chromium switches Node rejects as bad options, so setting it there stops Harness from starting at all (exit 2304). The Harness entry declares Node mode from the inside instead, after its own switches are parsed, marking only the children. The packaged node/pnpm shims declare it themselves too, so a caller that scrubs the environment still gets Node semantics. Verified end to end on a dev launch: POST /dsh-market/update for dshmarket returned exitCode 0 and moved the profile from 1.15.0 to 1.17.1. --- build/harness-node-entry.mjs | 13 ++++++++++++ .../dsh-desktop-market-installer/index.js | 13 ++++++++---- src/main/runtime/harness-runtime.ts | 4 ++++ test/market-installer.test.js | 4 ++++ test/runtime.test.ts | 21 +++++++++++++++++++ 5 files changed, 51 insertions(+), 4 deletions(-) diff --git a/build/harness-node-entry.mjs b/build/harness-node-entry.mjs index c86fbd90..0e096ee0 100644 --- a/build/harness-node-entry.mjs +++ b/build/harness-node-entry.mjs @@ -1,5 +1,18 @@ import { pathToFileURL } from 'node:url' +// On macOS Harness runs inside an Electron utility process (TCC responsibility +// isolation), so `process.execPath` and `argv0` point at the Electron helper +// instead of a Node binary. Plugins re-invoke the dsh CLI through the +// executable running them — dsh-market forwards `process.execArgv` with it — +// and without Node mode that child boots as an Electron app, where the leading +// `--expose-internals` shifts argv and the CLI answers "--profile is +// required" instead of installing. Declaring it here, after this process has +// already parsed the Chromium switches it was launched with, marks only the +// children as Node processes. Bundled-Node hosts (Windows, Linux) skip it. +if (process.versions.electron !== undefined) { + process.env.ELECTRON_RUN_AS_NODE = '1' +} + const [dshEntryPath, ...dshArguments] = process.argv.slice(2) function report(label, value) { diff --git a/packages/dsh-desktop-market-installer/index.js b/packages/dsh-desktop-market-installer/index.js index fcc320d1..3f332638 100644 --- a/packages/dsh-desktop-market-installer/index.js +++ b/packages/dsh-desktop-market-installer/index.js @@ -134,31 +134,36 @@ export async function ensurePnpmShim(home = dshHome()) { const pnpmEntry = resolvePnpmEntry() const executable = process.execPath + // The packaged executable is Electron on macOS, where Harness runs as a + // utility process. Anything invoked through these shims expects Node + // semantics — a leading node flag included — so the shims declare Node mode + // themselves instead of relying on the caller's environment. The real Node + // runtime bundled on the other platforms ignores the variable. if (process.platform === 'win32') { const pnpmPath = join(directory, 'pnpm.cmd') await writeFile( pnpmPath, - `@chcp 65001 >nul\r\n@echo off\r\n\"${executable}\" \"${pnpmEntry}\" %*\r\n`, + `@chcp 65001 >nul\r\n@echo off\r\n@set ELECTRON_RUN_AS_NODE=1\r\n\"${executable}\" \"${pnpmEntry}\" %*\r\n`, 'utf8' ) const nodePath = join(directory, 'node.cmd') await writeFile( nodePath, - `@chcp 65001 >nul\r\n@echo off\r\n\"${executable}\" %*\r\n`, + `@chcp 65001 >nul\r\n@echo off\r\n@set ELECTRON_RUN_AS_NODE=1\r\n\"${executable}\" %*\r\n`, 'utf8' ) } else { const pnpmPath = join(directory, 'pnpm') await writeFile( pnpmPath, - `#!/bin/sh\nexec ${shellQuote(executable)} ${shellQuote(pnpmEntry)} \"$@\"\n`, + `#!/bin/sh\nexport ELECTRON_RUN_AS_NODE=1\nexec ${shellQuote(executable)} ${shellQuote(pnpmEntry)} \"$@\"\n`, { encoding: 'utf8', mode: 0o755 } ) await chmod(pnpmPath, 0o755) const nodePath = join(directory, 'node') await writeFile( nodePath, - `#!/bin/sh\nexec ${shellQuote(executable)} \"$@\"\n`, + `#!/bin/sh\nexport ELECTRON_RUN_AS_NODE=1\nexec ${shellQuote(executable)} \"$@\"\n`, { encoding: 'utf8', mode: 0o755 } ) await chmod(nodePath, 0o755) diff --git a/src/main/runtime/harness-runtime.ts b/src/main/runtime/harness-runtime.ts index 49607777..c4336c5f 100644 --- a/src/main/runtime/harness-runtime.ts +++ b/src/main/runtime/harness-runtime.ts @@ -52,6 +52,10 @@ export function buildHarnessSpawnOptions( const { ELECTRON_RUN_AS_NODE: _runAsNode, ...parentEnvironment } = environment const pathKey = platform === 'win32' ? 'Path' : 'PATH' + // ELECTRON_RUN_AS_NODE must not reach the Harness process itself: the macOS + // utility process is launched with Chromium switches (--type=utility, …) + // that Node rejects as bad options. The Harness entry re-declares Node mode + // from the inside, for its children only. return { cwd: launchDirectory, env: { diff --git a/test/market-installer.test.js b/test/market-installer.test.js index 2f4f6eee..c91cbd76 100644 --- a/test/market-installer.test.js +++ b/test/market-installer.test.js @@ -57,13 +57,17 @@ describe('desktop plugin market installer', () => { const nodeCmd = await readFile(join(binDir, 'node.cmd'), 'utf8') expect(pnpmCmd).toContain(process.execPath) expect(pnpmCmd).toContain('pnpm') + expect(pnpmCmd).toContain('set ELECTRON_RUN_AS_NODE=1') expect(nodeCmd).toContain(process.execPath) + expect(nodeCmd).toContain('set ELECTRON_RUN_AS_NODE=1') } else { const pnpmScript = await readFile(join(binDir, 'pnpm'), 'utf8') const nodeScript = await readFile(join(binDir, 'node'), 'utf8') expect(pnpmScript).toContain(process.execPath) expect(pnpmScript).toContain('pnpm') + expect(pnpmScript).toContain('export ELECTRON_RUN_AS_NODE=1') expect(nodeScript).toContain(process.execPath) + expect(nodeScript).toContain('export ELECTRON_RUN_AS_NODE=1') } const npmrc = await readFile(join(home, 'profiles', 'web', '.npmrc'), 'utf8') diff --git a/test/runtime.test.ts b/test/runtime.test.ts index c837cfda..ee662e89 100644 --- a/test/runtime.test.ts +++ b/test/runtime.test.ts @@ -1,3 +1,5 @@ +import { readFile } from 'node:fs/promises' +import { join } from 'node:path' import { describe, expect, it } from 'vitest' import { buildHarnessArguments, @@ -158,6 +160,25 @@ describe('Harness launch contract', () => { ).toBe(false) }) + it('declares Node mode for Harness children without imposing it on the utility process', async () => { + // dsh-market re-runs the dsh CLI as `execPath [...execArgv] bin.js plugin + // --profile web add …`. On macOS execPath is the Electron helper, so + // without Node mode that child boots as an Electron app, the leading + // `--expose-internals` shifts argv, and the CLI answers "--profile + // is required" instead of installing. The flag cannot travel in the + // process environment: the utility process is launched with Chromium + // switches Node rejects, so the entry sets it from the inside instead. + const macOptions = buildHarnessSpawnOptions('/launch-root', '/harness', 'darwin', { + PATH: '/usr/bin', + ELECTRON_RUN_AS_NODE: '1' + }) + expect(macOptions.env).not.toHaveProperty('ELECTRON_RUN_AS_NODE') + + const entry = await readFile(join(process.cwd(), 'build', 'harness-node-entry.mjs'), 'utf8') + expect(entry).toContain('process.versions.electron !== undefined') + expect(entry).toContain("process.env.ELECTRON_RUN_AS_NODE = '1'") + }) + it('rejects an unexpected macOS Harness argument layout', () => { expect(() => buildDisclaimedUtilityProcessSpec(['entry.mjs'], { From f669f5b47dee74b60a91dc9afd30bdafcca2a212 Mon Sep 17 00:00:00 2001 From: yaojin3616 Date: Fri, 21 Aug 2026 06:17:19 -0700 Subject: [PATCH 04/14] fix(win32): recover the profile install from a locked rename MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Windows cannot replace a directory while something holds a handle inside it, and pnpm finishes each package by renaming _tmp__ onto . With Harness running — it has the profile's modules loaded, and the platform's scanners open files behind everyone's back — that final rename fails, which is what "Update failed: dshmarket — EPERM ... rename '…argparse_tmp_19856_4' -> '…argparse'" is. dshmarket 1.15.0 made this reachable by gaining runtime dependencies (js-yaml -> argparse, undici) where 1.9.0 had none. pnpm is reached by name through the packaged shim by every profile package operation — the desktop installer and the community market alike — so the shim now points at a runner that owns the recovery for both: retry once (a scanner's handle is gone within a second), then move the blocked directory aside and let pnpm install over the freed name. Renaming the directory itself succeeds where replacing its contents does not. Anything unrecognized passes straight through with the same exit code and output. The sidelined copies are swept alongside pnpm's staging directories before Harness next starts, when nothing holds them. Verified on macOS that install and failure propagation still behave through the new shim path (dshmarket@1.15.0 installs; a bad spec still exits 1). The recovery itself is covered by unit tests over the real failure text — its effect needs a Windows run to confirm. --- .../dsh-desktop-market-installer/index.js | 22 ++- .../pnpm-runner.mjs | 144 +++++++++++++++++ src/main/state/plugin-recovery.ts | 13 +- test/market-installer.test.js | 10 +- test/plugin-recovery.test.ts | 12 ++ test/pnpm-runner.test.js | 148 ++++++++++++++++++ 6 files changed, 343 insertions(+), 6 deletions(-) create mode 100644 packages/dsh-desktop-market-installer/pnpm-runner.mjs create mode 100644 test/pnpm-runner.test.js diff --git a/packages/dsh-desktop-market-installer/index.js b/packages/dsh-desktop-market-installer/index.js index 3f332638..404ed20f 100644 --- a/packages/dsh-desktop-market-installer/index.js +++ b/packages/dsh-desktop-market-installer/index.js @@ -1,9 +1,12 @@ import { spawn } from 'node:child_process' import { existsSync } from 'node:fs' -import { chmod, mkdir, readFile, readdir, rename, rm, writeFile } from 'node:fs/promises' +import { chmod, copyFile, mkdir, readFile, readdir, rename, rm, writeFile } from 'node:fs/promises' import { createRequire } from 'node:module' import { homedir } from 'node:os' import { delimiter, dirname, join, resolve } from 'node:path' +import { fileURLToPath } from 'node:url' + +import { SIDELINE_MARKER } from './pnpm-runner.mjs' export const RECOMMENDED_MARKET_VERSION = '1.15.0' export const MARKET_PACKAGE = 'dshmarket' @@ -26,13 +29,18 @@ export function profileDirectory(home = dshHome()) { return join(home, 'profiles', MARKET_PROFILE) } +/** Leftovers of an interrupted pnpm run, or of a Windows locked-rename recovery. */ +export function isDisposableModuleDirectory(name) { + return name.includes('_tmp_') || name.includes(SIDELINE_MARKER) +} + export async function cleanStaleTemporaryDirectories(home = dshHome()) { const directory = profileDirectory(home) const nodeModulesPath = join(directory, 'node_modules') try { const entries = await readdir(nodeModulesPath, { withFileTypes: true }) for (const entry of entries) { - if (entry.isDirectory() && entry.name.includes('_tmp_')) { + if (entry.isDirectory() && isDisposableModuleDirectory(entry.name)) { await rm(join(nodeModulesPath, entry.name), { recursive: true, force: true }).catch(() => undefined) } } @@ -134,6 +142,12 @@ export async function ensurePnpmShim(home = dshHome()) { const pnpmEntry = resolvePnpmEntry() const executable = process.execPath + // pnpm is reached through this shim by every profile package operation — + // DSH Desktop's installer and the community market alike — so the runner it + // points at is where a Windows locked rename gets recovered for both. + const runnerPath = join(directory, 'pnpm-runner.mjs') + await copyFile(fileURLToPath(new URL('./pnpm-runner.mjs', import.meta.url)), runnerPath) + // The packaged executable is Electron on macOS, where Harness runs as a // utility process. Anything invoked through these shims expects Node // semantics — a leading node flag included — so the shims declare Node mode @@ -143,7 +157,7 @@ export async function ensurePnpmShim(home = dshHome()) { const pnpmPath = join(directory, 'pnpm.cmd') await writeFile( pnpmPath, - `@chcp 65001 >nul\r\n@echo off\r\n@set ELECTRON_RUN_AS_NODE=1\r\n\"${executable}\" \"${pnpmEntry}\" %*\r\n`, + `@chcp 65001 >nul\r\n@echo off\r\n@set ELECTRON_RUN_AS_NODE=1\r\n\"${executable}\" \"${runnerPath}\" \"${pnpmEntry}\" %*\r\n`, 'utf8' ) const nodePath = join(directory, 'node.cmd') @@ -156,7 +170,7 @@ export async function ensurePnpmShim(home = dshHome()) { const pnpmPath = join(directory, 'pnpm') await writeFile( pnpmPath, - `#!/bin/sh\nexport ELECTRON_RUN_AS_NODE=1\nexec ${shellQuote(executable)} ${shellQuote(pnpmEntry)} \"$@\"\n`, + `#!/bin/sh\nexport ELECTRON_RUN_AS_NODE=1\nexec ${shellQuote(executable)} ${shellQuote(runnerPath)} ${shellQuote(pnpmEntry)} \"$@\"\n`, { encoding: 'utf8', mode: 0o755 } ) await chmod(pnpmPath, 0o755) diff --git a/packages/dsh-desktop-market-installer/pnpm-runner.mjs b/packages/dsh-desktop-market-installer/pnpm-runner.mjs new file mode 100644 index 00000000..9b485157 --- /dev/null +++ b/packages/dsh-desktop-market-installer/pnpm-runner.mjs @@ -0,0 +1,144 @@ +/** + * The pnpm entry every profile package operation goes through — DSH Desktop's + * own installer and the community market alike, because both reach pnpm by + * name and the packaged shim in `.desktop-bin` points here. + * + * Windows cannot replace a directory while something holds a handle inside it, + * and pnpm finishes each package by renaming `_tmp__` onto + * ``. With Harness running — it has the profile's modules loaded, and the + * platform's own scanners open files behind everyone's back — that final + * rename fails: + * + * EPERM: operation not permitted, rename '…\argparse_tmp_19856_4' -> '…\argparse' + * + * Two recoveries, in order of how little they disturb: retry once (a scanner's + * handle is gone within a second), then move the blocked target aside and + * retry (a rename of the directory itself succeeds where replacing its + * contents does not, and pnpm recreates the package under the free name). The + * leftovers are swept before Harness next starts, when nothing holds them. + * + * Anything unrecognized is passed straight through: same exit code, same + * output, one pnpm run. + */ +import { spawn } from 'node:child_process' +import { existsSync } from 'node:fs' +import { rename } from 'node:fs/promises' +import { fileURLToPath, pathToFileURL } from 'node:url' + +export const SIDELINE_MARKER = '.dsh-old-' +export const RETRY_DELAY_MS = 750 + +/** + * The destination pnpm could not claim, or undefined when the failure is not a + * locked rename inside a profile's node_modules. Only a path under + * node_modules qualifies: a rename failure anywhere else is not ours to + * rearrange. + * @param {string} output - the failed run's combined stdout and stderr. + * @returns {string | undefined} the blocked destination path. + */ +export function lockedRenameTarget(output) { + const failure = + /(?:EPERM|EBUSY|EACCES|ENOTEMPTY|EEXIST)[^\n]*?rename[^\n]*?->\s*'([^']+)'/u.exec(output) + if (failure === null) return undefined + const target = failure[1] + const inModules = target.split(/[\\/]/u).includes('node_modules') + return inModules ? target : undefined +} + +/** + * Where a blocked directory is moved so pnpm can claim the name it wants: the + * same path under a suffixed name, which keeps it a sibling without parsing + * separators the host may not own. The marker is what the pre-launch sweep + * looks for. + * @param {string} target - the blocked destination path. + * @param {number} now - timestamp making the name unique across attempts. + * @returns {string} the sideline path. + */ +export function sidelinePath(target, now = Date.now()) { + return `${target}${SIDELINE_MARKER}${now}` +} + +function delay(milliseconds) { + return new Promise((resolve) => setTimeout(resolve, milliseconds)) +} + +/** + * Run pnpm once, mirroring its streams to this process while keeping a copy + * for failure classification. + */ +function runPnpm(executable, args, spawnProcess = spawn) { + return new Promise((resolve, reject) => { + const child = spawnProcess(executable, args, { + stdio: ['inherit', 'pipe', 'pipe'], + windowsHide: true + }) + let output = '' + const keep = (chunk) => { + output = `${output}${chunk}`.slice(-256 * 1024) + } + child.stdout.on('data', (chunk) => { + keep(chunk) + process.stdout.write(chunk) + }) + child.stderr.on('data', (chunk) => { + keep(chunk) + process.stderr.write(chunk) + }) + child.once('error', reject) + child.once('exit', (code, signal) => resolve({ code, signal, output })) + }) +} + +/** + * Run pnpm, recovering from a Windows locked rename. Returns the exit code of + * the run that decided the outcome. + */ +export async function runWithLockRecovery(executable, args, options = {}) { + const { + spawnProcess = spawn, + moveAside = rename, + exists = existsSync, + wait = delay, + now = Date.now, + retryDelayMs = RETRY_DELAY_MS + } = options + + const first = await runPnpm(executable, args, spawnProcess) + if (first.code === 0 || lockedRenameTarget(first.output) === undefined) return first + + await wait(retryDelayMs) + const second = await runPnpm(executable, args, spawnProcess) + const target = second.code === 0 ? undefined : lockedRenameTarget(second.output) + if (target === undefined) return second + + if (!exists(target)) return second + try { + await moveAside(target, sidelinePath(target, now())) + } catch { + // The directory itself is held too — nothing left to try, and the run's + // own diagnostics are already on stderr. + return second + } + return runPnpm(executable, args, spawnProcess) +} + +/* v8 ignore start -- the process wrapper around the tested runner */ +if (process.argv[1] !== undefined && import.meta.url === pathToFileURL(process.argv[1]).href) { + const [pnpmEntry, ...pnpmArguments] = process.argv.slice(2) + if (pnpmEntry === undefined) { + process.stderr.write('dsh-desktop: the pnpm runner needs the pnpm entry path.\n') + process.exitCode = 1 + } else { + const executable = process.execPath + const result = await runWithLockRecovery(executable, [pnpmEntry, ...pnpmArguments]) + if (result.signal) { + process.stderr.write(`dsh-desktop: pnpm terminated with ${result.signal}.\n`) + process.exitCode = 1 + } else { + process.exitCode = result.code ?? 1 + } + } +} +/* v8 ignore stop */ + +export const RUNNER_PATH = fileURLToPath(import.meta.url) diff --git a/src/main/state/plugin-recovery.ts b/src/main/state/plugin-recovery.ts index fad72676..e50d2778 100644 --- a/src/main/state/plugin-recovery.ts +++ b/src/main/state/plugin-recovery.ts @@ -3,6 +3,17 @@ import { readFile, readdir, rm, writeFile } from 'node:fs/promises' import { dirname, join, resolve } from 'node:path' import { parse } from 'yaml' +/** + * Directories under the profile's node_modules that no longer belong to any + * package: pnpm's `_tmp__` staging left by an interrupted run, + * and the `.dsh-old-` copies the packaged pnpm runner moves aside + * when Windows refuses to replace a directory still held open. Both are only + * safely removable before Harness starts, which is when this sweep runs. + */ +export function isDisposableModuleDirectory(name: string): boolean { + return name.includes('_tmp_') || name.includes('.dsh-old-') +} + export function profilePackageJsonPath(dshHome: string): string { return join(dshHome, 'profiles', 'web', 'package.json') } @@ -495,7 +506,7 @@ export async function pruneMissingProfileBundles(dshHome: string): Promise undefined) } } diff --git a/test/market-installer.test.js b/test/market-installer.test.js index c91cbd76..b6ebe551 100644 --- a/test/market-installer.test.js +++ b/test/market-installer.test.js @@ -52,11 +52,15 @@ describe('desktop plugin market installer', () => { const binDir = await ensurePnpmShim(home) expect(binDir).toBe(join(home, '.desktop-bin')) + const runner = await readFile(join(binDir, 'pnpm-runner.mjs'), 'utf8') + expect(runner).toContain('runWithLockRecovery') + if (process.platform === 'win32') { const pnpmCmd = await readFile(join(binDir, 'pnpm.cmd'), 'utf8') const nodeCmd = await readFile(join(binDir, 'node.cmd'), 'utf8') expect(pnpmCmd).toContain(process.execPath) expect(pnpmCmd).toContain('pnpm') + expect(pnpmCmd).toContain(join(binDir, 'pnpm-runner.mjs')) expect(pnpmCmd).toContain('set ELECTRON_RUN_AS_NODE=1') expect(nodeCmd).toContain(process.execPath) expect(nodeCmd).toContain('set ELECTRON_RUN_AS_NODE=1') @@ -65,6 +69,7 @@ describe('desktop plugin market installer', () => { const nodeScript = await readFile(join(binDir, 'node'), 'utf8') expect(pnpmScript).toContain(process.execPath) expect(pnpmScript).toContain('pnpm') + expect(pnpmScript).toContain(join(binDir, 'pnpm-runner.mjs')) expect(pnpmScript).toContain('export ELECTRON_RUN_AS_NODE=1') expect(nodeScript).toContain(process.execPath) expect(nodeScript).toContain('export ELECTRON_RUN_AS_NODE=1') @@ -75,18 +80,21 @@ describe('desktop plugin market installer', () => { expect(npmrc).toContain('child-concurrency=1') }) - it('cleans up stale _tmp_ directories left by interrupted installations', async () => { + it('cleans up staging and sidelined directories left by interrupted installations', async () => { const home = await mkdtemp(join(tmpdir(), 'dsh-market-clean-')) const nodeModules = join(home, 'profiles', 'web', 'node_modules') const staleTmpDir = join(nodeModules, 'argparse_tmp_12345_1') + const sidelinedDir = join(nodeModules, 'argparse.dsh-old-1787317710932') const validDir = join(nodeModules, 'argparse') await mkdir(staleTmpDir, { recursive: true }) + await mkdir(sidelinedDir, { recursive: true }) await mkdir(validDir, { recursive: true }) await cleanStaleTemporaryDirectories(home) const { existsSync } = await import('node:fs') expect(existsSync(staleTmpDir)).toBe(false) + expect(existsSync(sidelinedDir)).toBe(false) expect(existsSync(validDir)).toBe(true) }) diff --git a/test/plugin-recovery.test.ts b/test/plugin-recovery.test.ts index f7c67673..ce6cc7f9 100644 --- a/test/plugin-recovery.test.ts +++ b/test/plugin-recovery.test.ts @@ -3,6 +3,7 @@ import { join } from 'node:path' import { afterEach, beforeEach, describe, expect, it } from 'vitest' import { parse, stringify } from 'yaml' import { + isDisposableModuleDirectory, isThirdPartyPackageName, profilePackageJsonPath, pruneMissingProfileBundles, @@ -697,5 +698,16 @@ describe('plugin-recovery', () => { const modified = await pruneMissingProfileBundles(testDir) expect(modified).toBe(false) }) + + it('sweeps pnpm staging and sidelined package directories before launch', () => { + // Windows refuses to replace a directory something still holds open, so + // the packaged pnpm runner moves the blocked package aside and lets pnpm + // install over the freed name. Nothing holds either leftover before + // Harness starts, which is when this sweep runs. + expect(isDisposableModuleDirectory('argparse_tmp_19856_4')).toBe(true) + expect(isDisposableModuleDirectory('argparse.dsh-old-1787317710932')).toBe(true) + expect(isDisposableModuleDirectory('argparse')).toBe(false) + expect(isDisposableModuleDirectory('js-yaml')).toBe(false) + }) }) diff --git a/test/pnpm-runner.test.js b/test/pnpm-runner.test.js new file mode 100644 index 00000000..21310756 --- /dev/null +++ b/test/pnpm-runner.test.js @@ -0,0 +1,148 @@ +import { EventEmitter } from 'node:events' +import { describe, expect, it, vi } from 'vitest' +import { + SIDELINE_MARKER, + lockedRenameTarget, + runWithLockRecovery, + sidelinePath +} from '../packages/dsh-desktop-market-installer/pnpm-runner.mjs' + +const WINDOWS_LOCK_FAILURE = [ + 'Update failed: dshmarket', + "error: EPERM: operation not permitted, rename 'C:\\Users\\u\\AppData\\Roaming\\dsh-desktop-dev\\harness\\profiles\\web\\node_modules\\argparse_tmp_19856_4' -> 'C:\\Users\\u\\AppData\\Roaming\\dsh-desktop-dev\\harness\\profiles\\web\\node_modules\\argparse'", + ' at Worker. (D:\\AA\\DSH Desktop Dev\\resources\\app\\node_modules\\pnpm\\dist\\pnpm.cjs:104217:22)' +].join('\n') + +const BLOCKED_TARGET = + 'C:\\Users\\u\\AppData\\Roaming\\dsh-desktop-dev\\harness\\profiles\\web\\node_modules\\argparse' + +function fakePnpm(runs) { + const calls = [] + const spawnProcess = (executable, args) => { + const run = runs[calls.length] ?? { code: 0, output: '' } + calls.push({ executable, args }) + const child = new EventEmitter() + child.stdout = new EventEmitter() + child.stderr = new EventEmitter() + queueMicrotask(() => { + if (run.output) child.stderr.emit('data', run.output) + child.emit('exit', run.code, null) + }) + return child + } + return { spawnProcess, calls } +} + +describe('packaged pnpm runner', () => { + it('recognizes a Windows locked rename inside a profile', () => { + expect(lockedRenameTarget(WINDOWS_LOCK_FAILURE)).toBe(BLOCKED_TARGET) + expect( + lockedRenameTarget( + "EBUSY: resource busy or locked, rename '/p/node_modules/a_tmp_1_1' -> '/p/node_modules/a'" + ) + ).toBe('/p/node_modules/a') + }) + + it('leaves unrelated failures alone', () => { + expect(lockedRenameTarget('ERR_PNPM_FETCH_500 GET https://registry/x failed')).toBeUndefined() + expect( + lockedRenameTarget("EPERM: operation not permitted, rename '/tmp/a' -> '/etc/passwd'") + ).toBeUndefined() + expect(lockedRenameTarget('')).toBeUndefined() + }) + + it('names the sidelined directory next to the blocked one', () => { + expect(sidelinePath('/p/node_modules/argparse', 42)).toBe( + `/p/node_modules/argparse${SIDELINE_MARKER}42` + ) + }) + + it('passes a successful run straight through', async () => { + const { spawnProcess, calls } = fakePnpm([{ code: 0, output: '' }]) + const moveAside = vi.fn() + + const result = await runWithLockRecovery('/node', ['/pnpm.cjs', 'add', 'x'], { + spawnProcess, + moveAside, + wait: async () => undefined + }) + + expect(result.code).toBe(0) + expect(calls).toHaveLength(1) + expect(moveAside).not.toHaveBeenCalled() + }) + + it('does not retry a failure that is not a locked rename', async () => { + const { spawnProcess, calls } = fakePnpm([{ code: 1, output: 'ERR_PNPM_NO_MATCHING_VERSION' }]) + + const result = await runWithLockRecovery('/node', ['/pnpm.cjs', 'add', 'x'], { + spawnProcess, + wait: async () => undefined + }) + + expect(result.code).toBe(1) + expect(calls).toHaveLength(1) + }) + + it('retries once for a lock that clears on its own', async () => { + const { spawnProcess, calls } = fakePnpm([ + { code: 1, output: WINDOWS_LOCK_FAILURE }, + { code: 0, output: '' } + ]) + const moveAside = vi.fn() + + const result = await runWithLockRecovery('/node', ['/pnpm.cjs', 'add', 'x'], { + spawnProcess, + moveAside, + wait: async () => undefined + }) + + expect(result.code).toBe(0) + expect(calls).toHaveLength(2) + expect(moveAside).not.toHaveBeenCalled() + }) + + it('moves the held directory aside and installs over the freed name', async () => { + const { spawnProcess, calls } = fakePnpm([ + { code: 1, output: WINDOWS_LOCK_FAILURE }, + { code: 1, output: WINDOWS_LOCK_FAILURE }, + { code: 0, output: '' } + ]) + const moveAside = vi.fn(async () => undefined) + + const result = await runWithLockRecovery('/node', ['/pnpm.cjs', 'add', 'x'], { + spawnProcess, + moveAside, + exists: () => true, + wait: async () => undefined, + now: () => 1234 + }) + + expect(result.code).toBe(0) + expect(calls).toHaveLength(3) + expect(moveAside).toHaveBeenCalledWith( + BLOCKED_TARGET, + `${BLOCKED_TARGET}${SIDELINE_MARKER}1234` + ) + }) + + it('reports pnpm\u2019s own failure when the directory cannot be moved either', async () => { + const { spawnProcess, calls } = fakePnpm([ + { code: 1, output: WINDOWS_LOCK_FAILURE }, + { code: 1, output: WINDOWS_LOCK_FAILURE } + ]) + + const result = await runWithLockRecovery('/node', ['/pnpm.cjs', 'add', 'x'], { + spawnProcess, + moveAside: async () => { + throw new Error('EPERM') + }, + exists: () => true, + wait: async () => undefined + }) + + expect(result.code).toBe(1) + expect(result.output).toContain('EPERM') + expect(calls).toHaveLength(2) + }) +}) From bdb298b407603814268122b47c06e4615287f906 Mon Sep 17 00:00:00 2001 From: yaojin3616 Date: Fri, 21 Aug 2026 06:52:15 -0700 Subject: [PATCH 05/14] fix(win32): make the pnpm recovery visible and fail fast MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two things the first cut of the runner got wrong, both found by a Windows report that looked exactly like the one before it. It could not be told apart from an unwrapped pnpm. The runner propagates pnpm's own diagnostics verbatim when recovery does not help, so an unchanged error message proved nothing about whether the runner ran at all. Every step now announces itself on the same stream the market reports verbatim: a failure report without "dsh-desktop pnpm runner:" lines is a report from a pnpm this runner never wrapped, which is the first thing to establish before reading anything else into it. And it made a failing install slower — up to three pnpm runs where the hosts already allow fifteen minutes for one, which is what a failed install feeling like a hang is made of. A run that stops producing output for two minutes (DSH_DESKTOP_PNPM_IDLE_TIMEOUT_MS) is now stopped rather than waited out, and a run stopped that way is not retried: whatever wedged it is still there. The Windows kill takes the process tree, since pnpm leaves children behind. A runner that cannot be staged no longer takes the shims down with it: pnpm stays reachable without the recovery, and the harness log says which of the two happened. --- .../dsh-desktop-market-installer/index.js | 35 +++++- .../pnpm-runner.mjs | 119 +++++++++++++++--- test/market-installer.test.js | 9 +- test/pnpm-runner.test.js | 48 +++++++ 4 files changed, 185 insertions(+), 26 deletions(-) diff --git a/packages/dsh-desktop-market-installer/index.js b/packages/dsh-desktop-market-installer/index.js index 404ed20f..f69267ce 100644 --- a/packages/dsh-desktop-market-installer/index.js +++ b/packages/dsh-desktop-market-installer/index.js @@ -136,6 +136,22 @@ export function resolvePnpmEntry(requireFrom = import.meta.url) { return entry } +/** + * Put the lock-recovery runner where the shims can invoke it. + * @returns the staged runner path, or undefined when neither the staged copy + * nor the packaged original can be used — the shims then call pnpm directly. + */ +export async function stagePnpmRunner(directory) { + const source = fileURLToPath(new URL('./pnpm-runner.mjs', import.meta.url)) + const staged = join(directory, 'pnpm-runner.mjs') + try { + await copyFile(source, staged) + return staged + } catch { + return existsSync(source) ? source : undefined + } +} + export async function ensurePnpmShim(home = dshHome()) { const directory = join(home, '.desktop-bin') await mkdir(directory, { recursive: true }) @@ -144,9 +160,18 @@ export async function ensurePnpmShim(home = dshHome()) { // pnpm is reached through this shim by every profile package operation — // DSH Desktop's installer and the community market alike — so the runner it - // points at is where a Windows locked rename gets recovered for both. - const runnerPath = join(directory, 'pnpm-runner.mjs') - await copyFile(fileURLToPath(new URL('./pnpm-runner.mjs', import.meta.url)), runnerPath) + // points at is where a Windows locked rename gets recovered for both. A + // runner that cannot be staged must not take the shims down with it: pnpm + // still has to be reachable, just without the recovery, and the harness log + // has to say so rather than leaving a stale shim to be mistaken for a fresh + // one. + const runnerPath = await stagePnpmRunner(directory) + const pnpmCommand = runnerPath === undefined ? [pnpmEntry] : [runnerPath, pnpmEntry] + process.stderr.write( + runnerPath === undefined + ? 'dsh-desktop: pnpm shim written without the lock-recovery runner\n' + : `dsh-desktop: pnpm shim written via ${runnerPath}\n` + ) // The packaged executable is Electron on macOS, where Harness runs as a // utility process. Anything invoked through these shims expects Node @@ -157,7 +182,7 @@ export async function ensurePnpmShim(home = dshHome()) { const pnpmPath = join(directory, 'pnpm.cmd') await writeFile( pnpmPath, - `@chcp 65001 >nul\r\n@echo off\r\n@set ELECTRON_RUN_AS_NODE=1\r\n\"${executable}\" \"${runnerPath}\" \"${pnpmEntry}\" %*\r\n`, + `@chcp 65001 >nul\r\n@echo off\r\n@set ELECTRON_RUN_AS_NODE=1\r\n\"${executable}\" ${pnpmCommand.map((part) => `\"${part}\"`).join(' ')} %*\r\n`, 'utf8' ) const nodePath = join(directory, 'node.cmd') @@ -170,7 +195,7 @@ export async function ensurePnpmShim(home = dshHome()) { const pnpmPath = join(directory, 'pnpm') await writeFile( pnpmPath, - `#!/bin/sh\nexport ELECTRON_RUN_AS_NODE=1\nexec ${shellQuote(executable)} ${shellQuote(runnerPath)} ${shellQuote(pnpmEntry)} \"$@\"\n`, + `#!/bin/sh\nexport ELECTRON_RUN_AS_NODE=1\nexec ${shellQuote(executable)} ${pnpmCommand.map(shellQuote).join(' ')} \"$@\"\n`, { encoding: 'utf8', mode: 0o755 } ) await chmod(pnpmPath, 0o755) diff --git a/packages/dsh-desktop-market-installer/pnpm-runner.mjs b/packages/dsh-desktop-market-installer/pnpm-runner.mjs index 9b485157..97e77f8a 100644 --- a/packages/dsh-desktop-market-installer/pnpm-runner.mjs +++ b/packages/dsh-desktop-market-installer/pnpm-runner.mjs @@ -27,6 +27,19 @@ import { fileURLToPath, pathToFileURL } from 'node:url' export const SIDELINE_MARKER = '.dsh-old-' export const RETRY_DELAY_MS = 750 +/** + * How long pnpm may stay silent before this runner stops it. pnpm narrates + * resolution, fetching and linking as it goes, so silence this long is a stuck + * run, not a slow one — and failing here beats the hosts' fifteen-minute + * ceiling, which is what makes a failed install feel like a hang. + */ +export const IDLE_TIMEOUT_MS = Number(process.env.DSH_DESKTOP_PNPM_IDLE_TIMEOUT_MS) || 120_000 +/** Prefix of every line this runner contributes to a package operation's output. */ +export const MARKER = 'dsh-desktop pnpm runner:' + +function errorText(error) { + return error instanceof Error ? error.message : String(error) +} /** * The destination pnpm could not claim, or undefined when the failure is not a @@ -65,30 +78,74 @@ function delay(milliseconds) { /** * Run pnpm once, mirroring its streams to this process while keeping a copy * for failure classification. + * + * A run that stops saying anything is stopped rather than waited out: pnpm + * narrates its progress line by line, so silence is not slow work, and the + * hosts above only bound the whole operation at fifteen minutes — long enough + * for a wedged install to look like a hang to the person watching. */ -function runPnpm(executable, args, spawnProcess = spawn) { +function runPnpm(executable, args, options = {}) { + const { + spawnProcess = spawn, + idleTimeoutMs = IDLE_TIMEOUT_MS, + report = () => undefined + } = options + return new Promise((resolve, reject) => { const child = spawnProcess(executable, args, { stdio: ['inherit', 'pipe', 'pipe'], windowsHide: true }) let output = '' - const keep = (chunk) => { + let idle + let stopped = false + + const finish = (result) => { + clearTimeout(idle) + resolve({ ...result, output, idleTimedOut: stopped }) + } + const heartbeat = () => { + clearTimeout(idle) + if (idleTimeoutMs <= 0) return + idle = setTimeout(() => { + stopped = true + report(`pnpm said nothing for ${Math.round(idleTimeoutMs / 1000)}s; stopping it`) + killTree(child) + }, idleTimeoutMs) + idle.unref?.() + } + const observe = (chunk, stream) => { output = `${output}${chunk}`.slice(-256 * 1024) + stream.write(chunk) + heartbeat() } - child.stdout.on('data', (chunk) => { - keep(chunk) - process.stdout.write(chunk) - }) - child.stderr.on('data', (chunk) => { - keep(chunk) - process.stderr.write(chunk) + + child.stdout.on('data', (chunk) => observe(chunk, process.stdout)) + child.stderr.on('data', (chunk) => observe(chunk, process.stderr)) + child.once('error', (error) => { + clearTimeout(idle) + reject(error) }) - child.once('error', reject) - child.once('exit', (code, signal) => resolve({ code, signal, output })) + child.once('exit', (code, signal) => finish({ code: stopped ? 1 : code, signal })) + heartbeat() }) } +function killTree(child) { + if (process.platform === 'win32' && child.pid !== undefined) { + try { + spawn('taskkill', ['/pid', String(child.pid), '/t', '/f'], { + stdio: 'ignore', + windowsHide: true + }) + return + } catch { + // fall through to the plain kill below + } + } + child.kill('SIGKILL') +} + /** * Run pnpm, recovering from a Windows locked rename. Returns the exit code of * the run that decided the outcome. @@ -100,26 +157,48 @@ export async function runWithLockRecovery(executable, args, options = {}) { exists = existsSync, wait = delay, now = Date.now, - retryDelayMs = RETRY_DELAY_MS + retryDelayMs = RETRY_DELAY_MS, + idleTimeoutMs = IDLE_TIMEOUT_MS, + // Every step announces itself on the same stream pnpm's own diagnostics + // travel, because the market reports that stream verbatim: a report + // without these lines is a report from a pnpm this runner never wrapped. + report = (message) => process.stderr.write(`${MARKER} ${message}\n`) } = options - const first = await runPnpm(executable, args, spawnProcess) - if (first.code === 0 || lockedRenameTarget(first.output) === undefined) return first + const run = () => runPnpm(executable, args, { spawnProcess, idleTimeoutMs, report }) + const first = await run() + const blocked = first.code === 0 ? undefined : lockedRenameTarget(first.output) + // A run stopped for silence is not retried: whatever wedged it is still + // there, and three stuck runs are three times the wait for the same answer. + if (blocked === undefined || first.idleTimedOut) return first + + report(`${blocked} could not be replaced; retrying in ${retryDelayMs}ms (2 of 3)`) await wait(retryDelayMs) - const second = await runPnpm(executable, args, spawnProcess) + const second = await run() const target = second.code === 0 ? undefined : lockedRenameTarget(second.output) - if (target === undefined) return second + if (target === undefined) { + if (second.code === 0) report('the retry succeeded') + return second + } - if (!exists(target)) return second + if (!exists(target)) { + report(`${target} is gone; leaving pnpm's own diagnosis in place`) + return second + } + const sideline = sidelinePath(target, now()) try { - await moveAside(target, sidelinePath(target, now())) - } catch { + await moveAside(target, sideline) + } catch (error) { // The directory itself is held too — nothing left to try, and the run's // own diagnostics are already on stderr. + report(`${target} could not be moved aside either (${errorText(error)})`) return second } - return runPnpm(executable, args, spawnProcess) + report(`moved ${target} to ${sideline}; installing over the freed name (3 of 3)`) + const third = await run() + report(third.code === 0 ? 'the install succeeded' : 'the install failed again') + return third } /* v8 ignore start -- the process wrapper around the tested runner */ diff --git a/test/market-installer.test.js b/test/market-installer.test.js index b6ebe551..db7ce3ac 100644 --- a/test/market-installer.test.js +++ b/test/market-installer.test.js @@ -14,7 +14,8 @@ import { ensurePnpmShim, isTrustedRequest, readMarketInstallation, - resolvePnpmEntry + resolvePnpmEntry, + stagePnpmRunner } from '../packages/dsh-desktop-market-installer/index.js' describe('desktop plugin market installer', () => { @@ -54,6 +55,12 @@ describe('desktop plugin market installer', () => { const runner = await readFile(join(binDir, 'pnpm-runner.mjs'), 'utf8') expect(runner).toContain('runWithLockRecovery') + await expect(stagePnpmRunner(binDir)).resolves.toBe(join(binDir, 'pnpm-runner.mjs')) + // A directory that cannot hold the staged copy still leaves pnpm reachable + // through the packaged original rather than taking the shims down. + await expect(stagePnpmRunner(join(binDir, 'missing', 'deeper'))).resolves.toMatch( + /packages[/\\]dsh-desktop-market-installer[/\\]pnpm-runner\.mjs$/u + ) if (process.platform === 'win32') { const pnpmCmd = await readFile(join(binDir, 'pnpm.cmd'), 'utf8') diff --git a/test/pnpm-runner.test.js b/test/pnpm-runner.test.js index 21310756..cdf9e7db 100644 --- a/test/pnpm-runner.test.js +++ b/test/pnpm-runner.test.js @@ -1,6 +1,7 @@ import { EventEmitter } from 'node:events' import { describe, expect, it, vi } from 'vitest' import { + MARKER, SIDELINE_MARKER, lockedRenameTarget, runWithLockRecovery, @@ -24,6 +25,12 @@ function fakePnpm(runs) { const child = new EventEmitter() child.stdout = new EventEmitter() child.stderr = new EventEmitter() + child.pid = 4242 + child.kill = () => { + child.emit('exit', null, 'SIGKILL') + return true + } + if (run.silent) return child queueMicrotask(() => { if (run.output) child.stderr.emit('data', run.output) child.emit('exit', run.code, null) @@ -126,6 +133,47 @@ describe('packaged pnpm runner', () => { ) }) + it('stops a pnpm that has gone silent instead of waiting out the host timeout', async () => { + const { spawnProcess, calls } = fakePnpm([{ silent: true }]) + const lines = [] + + const result = await runWithLockRecovery('/node', ['/pnpm.cjs', 'add', 'x'], { + spawnProcess, + idleTimeoutMs: 5, + report: (message) => lines.push(message) + }) + + expect(result.code).toBe(1) + expect(result.idleTimedOut).toBe(true) + // A wedged run is not retried — three stuck runs are three times the wait. + expect(calls).toHaveLength(1) + expect(lines[0]).toContain('said nothing') + }) + + it('says what it did, so a report without those lines names an unwrapped pnpm', async () => { + const { spawnProcess } = fakePnpm([ + { code: 1, output: WINDOWS_LOCK_FAILURE }, + { code: 1, output: WINDOWS_LOCK_FAILURE }, + { code: 0, output: '' } + ]) + const lines = [] + + await runWithLockRecovery('/node', ['/pnpm.cjs', 'add', 'x'], { + spawnProcess, + moveAside: async () => undefined, + exists: () => true, + wait: async () => undefined, + now: () => 1234, + report: (message) => lines.push(message) + }) + + expect(lines[0]).toContain('retrying') + expect(lines[1]).toContain(`moved ${BLOCKED_TARGET}`) + expect(lines[1]).toContain(SIDELINE_MARKER) + expect(lines.at(-1)).toContain('succeeded') + expect(MARKER).toContain('dsh-desktop') + }) + it('reports pnpm\u2019s own failure when the directory cannot be moved either', async () => { const { spawnProcess, calls } = fakePnpm([ { code: 1, output: WINDOWS_LOCK_FAILURE }, From 54304297282fffad251cc1cbe62460108edd39b5 Mon Sep 17 00:00:00 2001 From: yaojin3616 Date: Fri, 21 Aug 2026 06:58:14 -0700 Subject: [PATCH 06/14] fix: heal a profile an earlier failed install left behind MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A pnpm run that dies partway through leaves a directory under the profile's node_modules carrying a package's name without being one — the Windows locked rename is the failure that keeps producing them. pnpm cannot rename its staging directory onto that name afterwards, so the profile stays stuck and every later attempt fails identically, whatever version of DSH Desktop is installed over it. Upgrading has to be enough to get out of that state. Nothing holds those directories before Harness starts, which makes the launch path the one place they can be cleared safely — so that is where the repair goes: clear the directories that are not packages, then reinstall the profile's dependencies with Harness still stopped. Clearing alone would amputate, since the packages are still wanted; the existing prune now runs after the repair, so it only drops what the install could not restore. An undamaged profile costs one directory scan. A repair failure is not fatal: the prune still keeps the profile bootable, and both outcomes are written to the Harness log, which is where someone diagnosing a failed install is already looking. --- src/main/index.ts | 46 +++++++- src/main/runtime/harness-runtime.ts | 21 +++- src/main/runtime/profile-plugin-command.ts | 44 +++++++- src/main/state/profile-repair.ts | 102 ++++++++++++++++++ test/profile-repair.test.ts | 117 +++++++++++++++++++++ 5 files changed, 322 insertions(+), 8 deletions(-) create mode 100644 src/main/state/profile-repair.ts create mode 100644 test/profile-repair.test.ts diff --git a/src/main/index.ts b/src/main/index.ts index a0fc5439..fb9a8dac 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -16,7 +16,11 @@ import { } from 'electron' import { extractFailureCause, HarnessRuntime } from './runtime/harness-runtime' import { launchDisclaimedUtilityProcess } from './runtime/disclaimed-utility-process' -import { removeProfilePluginWithDsh } from './runtime/profile-plugin-command' +import { + installProfileDependenciesWithDsh, + removeProfilePluginWithDsh +} from './runtime/profile-plugin-command' +import { clearDamagedPackageDirectories, hasProfile } from './state/profile-repair' import { LanMobileBridge } from './mobile/lan-mobile-bridge' import { detectPluginRecovery, @@ -445,13 +449,51 @@ async function showSplash(): Promise { window.focus() } +/** + * Clear what an earlier failed package operation left behind, then put the + * packages back — both while Harness is stopped, the only moment either is + * safe. A profile damaged by an older build heals on the first launch of this + * one; an undamaged profile costs a directory scan. Failure here is not fatal: + * the prune below still keeps the profile bootable, and Harness reports + * whatever remains. + */ +async function repairProfilePackages(dshHome: string): Promise { + try { + if (!hasProfile(dshHome)) return + const removed = await clearDamagedPackageDirectories(dshHome) + if (removed.length === 0) return + + runtime.note( + `[desktop] repairing profile: cleared ${removed.length} damaged package ${ + removed.length === 1 ? 'directory' : 'directories' + }` + ) + const result = await installProfileDependenciesWithDsh({ + dshHome, + dshEntryPath: dshEntryPath(), + nodeExecutablePath: bundledNodePath(), + pnpmEntryPath: bundledPnpmEntryPath() + }) + runtime.note( + result.ok + ? '[desktop] profile repair completed' + : `[desktop] profile repair failed: ${result.detail ?? 'unknown error'}` + ) + } catch (error) { + runtime.note( + `[desktop] profile repair failed: ${error instanceof Error ? error.message : String(error)}` + ) + } +} + function launchHarness(): Promise { if (harnessLaunchOperation) return harnessLaunchOperation harnessLaunchOperation = (async () => { const dshHome = join(app.getPath('userData'), 'harness') - await pruneMissingProfileBundles(dshHome).catch(() => false) await showSplash() + await repairProfilePackages(dshHome) + await pruneMissingProfileBundles(dshHome).catch(() => false) await runtime.start(launchDirectory) })().finally(() => { harnessLaunchOperation = undefined diff --git a/src/main/runtime/harness-runtime.ts b/src/main/runtime/harness-runtime.ts index c4336c5f..9a7647ec 100644 --- a/src/main/runtime/harness-runtime.ts +++ b/src/main/runtime/harness-runtime.ts @@ -1,6 +1,6 @@ import type { SpawnOptionsWithoutStdio } from 'node:child_process' import type { EventEmitter } from 'node:events' -import { createWriteStream, existsSync, type WriteStream } from 'node:fs' +import { createWriteStream, existsSync, mkdirSync, type WriteStream } from 'node:fs' import { mkdir } from 'node:fs/promises' import { createServer } from 'node:net' import { dirname, join } from 'node:path' @@ -145,7 +145,7 @@ export class HarnessRuntime { await mkdir(this.options.dshHome, { recursive: true }) await mkdir(dirname(this.options.logPath), { recursive: true }) - this.logStream = createWriteStream(this.options.logPath, { flags: 'a' }) + this.logStream ??= createWriteStream(this.options.logPath, { flags: 'a' }) const port = await reservePort() const url = `http://127.0.0.1:${port}` @@ -268,6 +268,23 @@ ${cause}` } } + /** + * Record a line the desktop wants in the Harness log, including before a + * launch: what happens to the profile between launches is exactly what + * someone reading the log after a failed install needs to see. + */ + note(line: string): void { + if (!this.logStream) { + try { + mkdirSync(dirname(this.options.logPath), { recursive: true }) + this.logStream = createWriteStream(this.options.logPath, { flags: 'a' }) + } catch { + // Keep the line in the in-memory buffer regardless. + } + } + this.writeLog(line) + } + private writeLog(line: string): void { this.logLines.push(line) if (this.logLines.length > 200) this.logLines.splice(0, this.logLines.length - 200) diff --git a/src/main/runtime/profile-plugin-command.ts b/src/main/runtime/profile-plugin-command.ts index 3bbbc1c7..200238b5 100644 --- a/src/main/runtime/profile-plugin-command.ts +++ b/src/main/runtime/profile-plugin-command.ts @@ -5,6 +5,7 @@ import { delimiter, dirname, join } from 'node:path' const PROFILE = 'web' const OPERATION_TIMEOUT_MS = 15 * 60 * 1000 +const REPAIR_TIMEOUT_MS = 5 * 60 * 1000 const MAX_OUTPUT_BYTES = 32 * 1024 export interface ProfilePluginCommandOptions { @@ -31,6 +32,16 @@ export function buildProfilePluginRemoveArguments( return [dshEntryPath, 'plugin', '--profile', PROFILE, 'remove', pluginName] } +/** + * Reinstall everything the profile manifest asks for. This runs before Harness + * starts, which is the only moment the packages it would otherwise hold open + * can be replaced — so it is also how a profile left damaged by an earlier + * failure gets its packages back. + */ +export function buildProfileInstallArguments(dshEntryPath: string): string[] { + return [dshEntryPath, 'plugin', '--profile', PROFILE, 'install'] +} + export async function ensureProfilePnpmShim(options: ProfilePluginCommandOptions): Promise { const directory = join(options.dshHome, '.desktop-bin') await mkdir(directory, { recursive: true }) @@ -111,6 +122,28 @@ function killProcessTree(child: ReturnType): void { export async function removeProfilePluginWithDsh( options: ProfilePluginCommandOptions, pluginName: string +): Promise { + return runProfileCommand(options, buildProfilePluginRemoveArguments(options.dshEntryPath, pluginName), 'Plugin removal', OPERATION_TIMEOUT_MS) +} + +/** + * Restore the profile's packages with Harness stopped. + * @param timeoutMs - shorter than a user-initiated operation on purpose: this + * one sits between the user and their window, so it gives up rather than + * turning a damaged profile into a launch that looks hung. + */ +export async function installProfileDependenciesWithDsh( + options: ProfilePluginCommandOptions, + timeoutMs = REPAIR_TIMEOUT_MS +): Promise { + return runProfileCommand(options, buildProfileInstallArguments(options.dshEntryPath), 'Profile repair', timeoutMs) +} + +async function runProfileCommand( + options: ProfilePluginCommandOptions, + commandArguments: string[], + label: string, + timeoutMs: number ): Promise { const requiredPaths = [ options.dshEntryPath, @@ -137,7 +170,7 @@ export async function removeProfilePluginWithDsh( const child = spawn( options.nodeExecutablePath, - buildProfilePluginRemoveArguments(options.dshEntryPath, pluginName), + commandArguments, { cwd: profileDirectory, env: environment, @@ -158,7 +191,7 @@ export async function removeProfilePluginWithDsh( const timer = setTimeout(() => { timedOut = true killProcessTree(child) - }, OPERATION_TIMEOUT_MS) + }, timeoutMs) try { const exit = await new Promise<{ code: number | null; signal: NodeJS.Signals | null }>( @@ -168,7 +201,10 @@ export async function removeProfilePluginWithDsh( } ) if (timedOut) { - return { ok: false, detail: 'Plugin removal timed out after 15 minutes.' } + return { + ok: false, + detail: `${label} timed out after ${Math.round(timeoutMs / 60_000)} minutes.` + } } if (exit.code !== 0) { const detail = output.trim().split(/\r?\n/u).at(-1)?.slice(0, 800) @@ -176,7 +212,7 @@ export async function removeProfilePluginWithDsh( ok: false, detail: detail || - `Plugin removal exited with ${exit.signal ? `signal ${exit.signal}` : `code ${exit.code}`}.` + `${label} exited with ${exit.signal ? `signal ${exit.signal}` : `code ${exit.code}`}.` } } return { ok: true } diff --git a/src/main/state/profile-repair.ts b/src/main/state/profile-repair.ts new file mode 100644 index 00000000..a964ed5a --- /dev/null +++ b/src/main/state/profile-repair.ts @@ -0,0 +1,102 @@ +import { existsSync } from 'node:fs' +import { readFile, readdir, rm } from 'node:fs/promises' +import { dirname, join } from 'node:path' +import { isDisposableModuleDirectory, profilePackageJsonPath } from './plugin-recovery' + +/** + * What a failed package operation leaves behind, and how the next launch gets + * rid of it. + * + * A pnpm run that dies partway through — the Windows locked rename is the one + * that keeps happening — leaves a directory under the profile's node_modules + * that carries a package's name without being one. pnpm cannot rename its + * staging directory onto that name afterwards, so every later attempt fails + * the same way: the profile is stuck until someone deletes the leftover by + * hand. + * + * Nothing holds these directories before Harness starts, which makes the + * launch path the one place they can be cleared safely. Clearing alone would + * amputate — the packages are still wanted — so a damaged profile is followed + * by an install that puts them back, and only what the install could not + * restore is pruned from the manifest afterwards. + */ + +/** Entries pnpm owns that are not packages and must never be judged as ones. */ +function isReservedEntry(name: string): boolean { + return name.startsWith('.') +} + +async function isMaterializedPackage(directory: string): Promise { + try { + const manifest = await readFile(join(directory, 'package.json'), 'utf8') + return typeof (JSON.parse(manifest) as { name?: unknown }).name === 'string' + } catch { + return false + } +} + +/** + * Package directories that carry a name without being a package: no readable + * manifest behind it. Scoped directories are inspected one level down, where + * the packages actually live. Symlinks are left alone — pnpm's isolated layout + * points them into the virtual store, and a broken link is not ours to judge. + * @param dshHome - the desktop's DSH home. + * @returns absolute paths, in the order found. + */ +export async function findDamagedPackageDirectories(dshHome: string): Promise { + const nodeModulesPath = join(dirname(profilePackageJsonPath(dshHome)), 'node_modules') + const damaged: string[] = [] + + const scan = async (directory: string, allowScopes: boolean): Promise => { + let entries + try { + entries = await readdir(directory, { withFileTypes: true }) + } catch { + return + } + + for (const entry of entries) { + if (isReservedEntry(entry.name) || entry.isSymbolicLink() || !entry.isDirectory()) continue + const path = join(directory, entry.name) + + if (isDisposableModuleDirectory(entry.name)) { + damaged.push(path) + continue + } + if (allowScopes && entry.name.startsWith('@')) { + await scan(path, false) + continue + } + if (!(await isMaterializedPackage(path))) damaged.push(path) + } + } + + await scan(nodeModulesPath, true) + return damaged +} + +/** + * Clear what {@link findDamagedPackageDirectories} found. + * @returns the paths actually removed. + */ +export async function clearDamagedPackageDirectories(dshHome: string): Promise { + const damaged = await findDamagedPackageDirectories(dshHome) + const removed: string[] = [] + + for (const path of damaged) { + try { + await rm(path, { recursive: true, force: true }) + removed.push(path) + } catch { + // Still held by something. The install below will fail on this name and + // report it, which beats deleting half of it here. + } + } + + return removed +} + +/** Whether the profile is worth repairing at all — no profile, nothing to do. */ +export function hasProfile(dshHome: string): boolean { + return existsSync(profilePackageJsonPath(dshHome)) +} diff --git a/test/profile-repair.test.ts b/test/profile-repair.test.ts new file mode 100644 index 00000000..8287b332 --- /dev/null +++ b/test/profile-repair.test.ts @@ -0,0 +1,117 @@ +import { mkdir, mkdtemp, rm, symlink, writeFile } from 'node:fs/promises' +import { existsSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, describe, expect, it } from 'vitest' +import { + clearDamagedPackageDirectories, + findDamagedPackageDirectories, + hasProfile +} from '../src/main/state/profile-repair' +import { buildProfileInstallArguments } from '../src/main/runtime/profile-plugin-command' + +describe('profile repair', () => { + const homes: string[] = [] + + async function profileHome(): Promise<{ home: string; nodeModules: string }> { + const home = await mkdtemp(join(tmpdir(), 'dsh-profile-repair-')) + homes.push(home) + const profile = join(home, 'profiles', 'web') + const nodeModules = join(profile, 'node_modules') + await mkdir(nodeModules, { recursive: true }) + await writeFile( + join(profile, 'package.json'), + JSON.stringify({ dependencies: { dshmarket: '1.15.0' } }), + 'utf8' + ) + return { home, nodeModules } + } + + async function materialize(directory: string, name: string): Promise { + const path = join(directory, name) + await mkdir(path, { recursive: true }) + await writeFile(join(path, 'package.json'), JSON.stringify({ name, version: '1.0.0' }), 'utf8') + return path + } + + afterEach(async () => { + await Promise.all(homes.splice(0).map((home) => rm(home, { recursive: true, force: true }))) + }) + + it('reports nothing for a profile whose packages are all materialized', async () => { + const { home, nodeModules } = await profileHome() + await materialize(nodeModules, 'dshmarket') + await materialize(join(nodeModules, '@deepseek-ai'), 'dsh-settings') + + await expect(findDamagedPackageDirectories(home)).resolves.toEqual([]) + }) + + it('finds the leftovers a failed pnpm run blocks the next one with', async () => { + const { home, nodeModules } = await profileHome() + // What the Windows locked rename leaves: a name taken by something that is + // not a package, which pnpm can then never rename its staging onto. + const halfWritten = join(nodeModules, 'cose-base') + await mkdir(halfWritten, { recursive: true }) + await writeFile(join(halfWritten, 'index.js'), '', 'utf8') + const staging = join(nodeModules, 'cose-base_tmp_15968_6') + await mkdir(staging, { recursive: true }) + await writeFile(join(staging, 'package.json'), JSON.stringify({ name: 'cose-base' }), 'utf8') + const sidelined = join(nodeModules, 'argparse.dsh-old-1787317710932') + await materialize(nodeModules, 'argparse') + await mkdir(sidelined, { recursive: true }) + + const damaged = await findDamagedPackageDirectories(home) + + expect(new Set(damaged)).toEqual(new Set([halfWritten, staging, sidelined])) + }) + + it('spares pnpm’s own entries, scoped packages, and symlinks', async () => { + const { home, nodeModules } = await profileHome() + await mkdir(join(nodeModules, '.pnpm'), { recursive: true }) + await mkdir(join(nodeModules, '.bin'), { recursive: true }) + // A scope directory holds packages and has no manifest of its own. + await materialize(join(nodeModules, '@deepseek-ai'), 'dsh-settings') + const target = await materialize(nodeModules, 'dshmarket') + await symlink(target, join(nodeModules, 'linked-plugin'), 'dir') + + await expect(findDamagedPackageDirectories(home)).resolves.toEqual([]) + }) + + it('finds a damaged package inside a scope', async () => { + const { home, nodeModules } = await profileHome() + const damagedScoped = join(nodeModules, '@linxin666', 'dsh-web-ui-all') + await mkdir(damagedScoped, { recursive: true }) + + await expect(findDamagedPackageDirectories(home)).resolves.toEqual([damagedScoped]) + }) + + it('clears what it found so the install can claim the names again', async () => { + const { home, nodeModules } = await profileHome() + const halfWritten = join(nodeModules, 'cose-base') + await mkdir(halfWritten, { recursive: true }) + const intact = await materialize(nodeModules, 'dshmarket') + + await expect(clearDamagedPackageDirectories(home)).resolves.toEqual([halfWritten]) + expect(existsSync(halfWritten)).toBe(false) + expect(existsSync(intact)).toBe(true) + await expect(findDamagedPackageDirectories(home)).resolves.toEqual([]) + }) + + it('leaves a home without a profile alone', async () => { + const home = await mkdtemp(join(tmpdir(), 'dsh-profile-repair-empty-')) + homes.push(home) + + expect(hasProfile(home)).toBe(false) + await expect(clearDamagedPackageDirectories(home)).resolves.toEqual([]) + }) + + it('restores the cleared packages through the profile’s own installer', () => { + expect(buildProfileInstallArguments('/app/dsh/bin.js')).toEqual([ + '/app/dsh/bin.js', + 'plugin', + '--profile', + 'web', + 'install' + ]) + }) +}) From 9c51c0e90c1207eb74ce87ad5473ca8a5851fabb Mon Sep 17 00:00:00 2001 From: yaojin3616 Date: Fri, 21 Aug 2026 07:01:57 -0700 Subject: [PATCH 07/14] fix(win32): never wait on a kill that did not land MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The idle timeout could hang on exactly the platform it was written for. killTree returned as soon as taskkill was spawned, so a taskkill that misses left the runner waiting forever on an exit that never comes — the hang the idle timeout exists to prevent. Windows CI caught it as a five-second test timeout. The direct kill is now the guarantee and taskkill only adds the tree, and a killed run is written off after a grace period regardless, so no failure of the kill itself can turn into a wait. Both paths are covered, with the kill injected so the test states the behavior instead of the platform. Also drop a directory symlink from the repair test onto a junction, which Windows can create without Developer Mode or elevation. --- .../pnpm-runner.mjs | 35 ++++++++++++++++--- test/pnpm-runner.test.js | 18 ++++++++++ test/profile-repair.test.ts | 4 ++- 3 files changed, 51 insertions(+), 6 deletions(-) diff --git a/packages/dsh-desktop-market-installer/pnpm-runner.mjs b/packages/dsh-desktop-market-installer/pnpm-runner.mjs index 97e77f8a..f2405cb5 100644 --- a/packages/dsh-desktop-market-installer/pnpm-runner.mjs +++ b/packages/dsh-desktop-market-installer/pnpm-runner.mjs @@ -34,6 +34,8 @@ export const RETRY_DELAY_MS = 750 * ceiling, which is what makes a failed install feel like a hang. */ export const IDLE_TIMEOUT_MS = Number(process.env.DSH_DESKTOP_PNPM_IDLE_TIMEOUT_MS) || 120_000 +/** How long a killed run may take to actually go away before it is written off. */ +export const KILL_GRACE_MS = 5_000 /** Prefix of every line this runner contributes to a package operation's output. */ export const MARKER = 'dsh-desktop pnpm runner:' @@ -88,6 +90,8 @@ function runPnpm(executable, args, options = {}) { const { spawnProcess = spawn, idleTimeoutMs = IDLE_TIMEOUT_MS, + killGraceMs = KILL_GRACE_MS, + kill = killTree, report = () => undefined } = options @@ -98,10 +102,15 @@ function runPnpm(executable, args, options = {}) { }) let output = '' let idle + let grace let stopped = false + let settled = false const finish = (result) => { + if (settled) return + settled = true clearTimeout(idle) + clearTimeout(grace) resolve({ ...result, output, idleTimedOut: stopped }) } const heartbeat = () => { @@ -110,7 +119,11 @@ function runPnpm(executable, args, options = {}) { idle = setTimeout(() => { stopped = true report(`pnpm said nothing for ${Math.round(idleTimeoutMs / 1000)}s; stopping it`) - killTree(child) + kill(child) + // A kill that does not land must not become the hang this guards + // against, so the run is written off either way. + grace = setTimeout(() => finish({ code: 1, signal: null }), killGraceMs) + grace.unref?.() }, idleTimeoutMs) idle.unref?.() } @@ -124,13 +137,23 @@ function runPnpm(executable, args, options = {}) { child.stderr.on('data', (chunk) => observe(chunk, process.stderr)) child.once('error', (error) => { clearTimeout(idle) - reject(error) + clearTimeout(grace) + if (!settled) { + settled = true + reject(error) + } }) child.once('exit', (code, signal) => finish({ code: stopped ? 1 : code, signal })) heartbeat() }) } +/** + * Stop a pnpm run and everything it started. On Windows `kill` reaches only + * the wrapper, so the tree goes through taskkill — but never *instead of* the + * direct kill: a taskkill that does not land would leave this runner waiting + * on an exit that never comes, which is the hang it exists to prevent. + */ function killTree(child) { if (process.platform === 'win32' && child.pid !== undefined) { try { @@ -138,9 +161,8 @@ function killTree(child) { stdio: 'ignore', windowsHide: true }) - return } catch { - // fall through to the plain kill below + // The direct kill below is the guarantee. } } child.kill('SIGKILL') @@ -159,13 +181,16 @@ export async function runWithLockRecovery(executable, args, options = {}) { now = Date.now, retryDelayMs = RETRY_DELAY_MS, idleTimeoutMs = IDLE_TIMEOUT_MS, + killGraceMs = KILL_GRACE_MS, + kill = killTree, // Every step announces itself on the same stream pnpm's own diagnostics // travel, because the market reports that stream verbatim: a report // without these lines is a report from a pnpm this runner never wrapped. report = (message) => process.stderr.write(`${MARKER} ${message}\n`) } = options - const run = () => runPnpm(executable, args, { spawnProcess, idleTimeoutMs, report }) + const run = () => + runPnpm(executable, args, { spawnProcess, idleTimeoutMs, killGraceMs, kill, report }) const first = await run() const blocked = first.code === 0 ? undefined : lockedRenameTarget(first.output) diff --git a/test/pnpm-runner.test.js b/test/pnpm-runner.test.js index cdf9e7db..f3ae84f7 100644 --- a/test/pnpm-runner.test.js +++ b/test/pnpm-runner.test.js @@ -140,6 +140,7 @@ describe('packaged pnpm runner', () => { const result = await runWithLockRecovery('/node', ['/pnpm.cjs', 'add', 'x'], { spawnProcess, idleTimeoutMs: 5, + kill: (child) => child.kill(), report: (message) => lines.push(message) }) @@ -150,6 +151,23 @@ describe('packaged pnpm runner', () => { expect(lines[0]).toContain('said nothing') }) + it('gives up on a run whose kill never lands', async () => { + // taskkill can miss on Windows. Waiting for an exit that never comes would + // be the very hang the idle timeout exists to prevent. + const { spawnProcess } = fakePnpm([{ silent: true }]) + + const result = await runWithLockRecovery('/node', ['/pnpm.cjs', 'add', 'x'], { + spawnProcess, + idleTimeoutMs: 5, + killGraceMs: 5, + kill: () => undefined, + report: () => undefined + }) + + expect(result.code).toBe(1) + expect(result.idleTimedOut).toBe(true) + }) + it('says what it did, so a report without those lines names an unwrapped pnpm', async () => { const { spawnProcess } = fakePnpm([ { code: 1, output: WINDOWS_LOCK_FAILURE }, diff --git a/test/profile-repair.test.ts b/test/profile-repair.test.ts index 8287b332..d0f13dcd 100644 --- a/test/profile-repair.test.ts +++ b/test/profile-repair.test.ts @@ -72,7 +72,9 @@ describe('profile repair', () => { // A scope directory holds packages and has no manifest of its own. await materialize(join(nodeModules, '@deepseek-ai'), 'dsh-settings') const target = await materialize(nodeModules, 'dshmarket') - await symlink(target, join(nodeModules, 'linked-plugin'), 'dir') + // Directory symlinks need Developer Mode or elevation on Windows; the rest + // of the expectation still holds where they cannot be created. + await symlink(target, join(nodeModules, 'linked-plugin'), 'junction').catch(() => undefined) await expect(findDamagedPackageDirectories(home)).resolves.toEqual([]) }) From cbf779d31a7c304ca1981465afdf6038d08cba14 Mon Sep 17 00:00:00 2001 From: yaojin3616 Date: Fri, 21 Aug 2026 07:12:27 -0700 Subject: [PATCH 08/14] fix(ci): route pnpm shim log to stdout and allow targeting windows in release workflow --- .github/workflows/release.yml | 21 +++++++++++++++++-- .../dsh-desktop-market-installer/index.js | 2 +- test/release.test.ts | 4 ++-- 3 files changed, 22 insertions(+), 5 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index de544dc9..ed7178e4 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -9,6 +9,15 @@ on: - main workflow_dispatch: inputs: + target: + description: Platform to build + required: false + default: all + type: choice + options: + - all + - windows + - macos windows_prerelease_tag: description: Optional non-v tag for a Windows Dev pre-release required: false @@ -24,6 +33,10 @@ concurrency: jobs: macos-apple-silicon: name: macOS Apple Silicon + if: >- + startsWith(github.ref, 'refs/tags/v') || + github.event_name == 'pull_request' || + (github.event_name == 'workflow_dispatch' && (inputs.target == 'all' || inputs.target == 'macos' || (inputs.target == '' && inputs.windows_prerelease_tag == ''))) runs-on: macos-15 steps: - uses: actions/checkout@v4 @@ -154,7 +167,9 @@ jobs: macos-intel: name: macOS Intel - if: startsWith(github.ref, 'refs/tags/v') || github.event_name == 'workflow_dispatch' + if: >- + startsWith(github.ref, 'refs/tags/v') || + (github.event_name == 'workflow_dispatch' && (inputs.target == 'all' || inputs.target == 'macos' || (inputs.target == '' && inputs.windows_prerelease_tag == ''))) runs-on: macos-15-intel steps: - uses: actions/checkout@v4 @@ -285,7 +300,9 @@ jobs: windows-x64: name: Windows x64 - if: startsWith(github.ref, 'refs/tags/v') || github.event_name == 'workflow_dispatch' + if: >- + startsWith(github.ref, 'refs/tags/v') || + (github.event_name == 'workflow_dispatch' && (inputs.target == 'all' || inputs.target == 'windows' || inputs.target == '' || inputs.windows_prerelease_tag != '')) runs-on: windows-2022 steps: - uses: actions/checkout@v4 diff --git a/packages/dsh-desktop-market-installer/index.js b/packages/dsh-desktop-market-installer/index.js index f69267ce..9af3a9eb 100644 --- a/packages/dsh-desktop-market-installer/index.js +++ b/packages/dsh-desktop-market-installer/index.js @@ -167,7 +167,7 @@ export async function ensurePnpmShim(home = dshHome()) { // one. const runnerPath = await stagePnpmRunner(directory) const pnpmCommand = runnerPath === undefined ? [pnpmEntry] : [runnerPath, pnpmEntry] - process.stderr.write( + process.stdout.write( runnerPath === undefined ? 'dsh-desktop: pnpm shim written without the lock-recovery runner\n' : `dsh-desktop: pnpm shim written via ${runnerPath}\n` diff --git a/test/release.test.ts b/test/release.test.ts index 3c0a8bc1..20665535 100644 --- a/test/release.test.ts +++ b/test/release.test.ts @@ -278,10 +278,10 @@ describe('GitHub release contract', () => { expect(workflow.match(/CSC_IDENTITY_AUTO_DISCOVERY: 'false'/g)).toHaveLength(2) expect(workflow).not.toContain("CSC_LINK: ''") expect(workflow).toMatch( - /macos-apple-silicon:\r?\n name: macOS Apple Silicon\r?\n runs-on: macos-15\r?\n steps:/ + /macos-apple-silicon:\r?\n\s+name: macOS Apple Silicon\r?\n(?:[\s\S]*?)runs-on: macos-15\r?\n\s+steps:/ ) expect(workflow).toMatch( - /macos-intel:\r?\n name: macOS Intel\r?\n if: [^\r\n]+\r?\n runs-on: macos-15-intel\r?\n steps:/ + /macos-intel:\r?\n\s+name: macOS Intel\r?\n(?:[\s\S]*?)runs-on: macos-15-intel\r?\n\s+steps:/ ) }) From 53cdc6f6efd390db66ec1bd4081b1379e51d33bc Mon Sep 17 00:00:00 2001 From: yaojin3616 Date: Fri, 21 Aug 2026 07:19:57 -0700 Subject: [PATCH 09/14] ci: run windows-only for non-tag events and keep full matrix for v-tags --- .github/workflows/release.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index ed7178e4..09ed7acf 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -35,8 +35,7 @@ jobs: name: macOS Apple Silicon if: >- startsWith(github.ref, 'refs/tags/v') || - github.event_name == 'pull_request' || - (github.event_name == 'workflow_dispatch' && (inputs.target == 'all' || inputs.target == 'macos' || (inputs.target == '' && inputs.windows_prerelease_tag == ''))) + (github.event_name == 'workflow_dispatch' && (inputs.target == 'all' || inputs.target == 'macos')) runs-on: macos-15 steps: - uses: actions/checkout@v4 @@ -169,7 +168,7 @@ jobs: name: macOS Intel if: >- startsWith(github.ref, 'refs/tags/v') || - (github.event_name == 'workflow_dispatch' && (inputs.target == 'all' || inputs.target == 'macos' || (inputs.target == '' && inputs.windows_prerelease_tag == ''))) + (github.event_name == 'workflow_dispatch' && (inputs.target == 'all' || inputs.target == 'macos')) runs-on: macos-15-intel steps: - uses: actions/checkout@v4 @@ -302,6 +301,7 @@ jobs: name: Windows x64 if: >- startsWith(github.ref, 'refs/tags/v') || + github.event_name == 'pull_request' || (github.event_name == 'workflow_dispatch' && (inputs.target == 'all' || inputs.target == 'windows' || inputs.target == '' || inputs.windows_prerelease_tag != '')) runs-on: windows-2022 steps: From b666b961d1e2514cc53c24a31867035e211494ab Mon Sep 17 00:00:00 2001 From: yaojin3616 Date: Fri, 21 Aug 2026 07:24:34 -0700 Subject: [PATCH 10/14] ci: enable package build, smoke test, and artifact upload for windows non-tag runs --- .github/workflows/release.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 09ed7acf..0d93b94d 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -322,10 +322,10 @@ jobs: if: startsWith(github.ref, 'refs/tags/v') run: npm run package:win - name: Build isolated Windows development package - if: github.event_name == 'workflow_dispatch' + if: ${{ !startsWith(github.ref, 'refs/tags/v') }} run: npm run package:dev:win - name: Smoke test packaged Windows Harness - if: github.event_name == 'workflow_dispatch' + if: ${{ !startsWith(github.ref, 'refs/tags/v') }} shell: pwsh run: | $userData = Join-Path $env:APPDATA 'dsh-desktop-dev' @@ -431,7 +431,7 @@ jobs: dist/latest.yml if-no-files-found: error - uses: actions/upload-artifact@v4 - if: github.event_name == 'workflow_dispatch' + if: ${{ !startsWith(github.ref, 'refs/tags/v') }} with: name: windows-x64-dev path: | From e1ca42aa52d2a40d468ea97327f6d9e84060d038 Mon Sep 17 00:00:00 2001 From: yaojin3616 Date: Fri, 21 Aug 2026 07:36:32 -0700 Subject: [PATCH 11/14] fix: stop killing pnpm runs that are quiet but working MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The idle timeout fired on Windows against a live install. Silence was the wrong signal: without a TTY pnpm drops its progress display, so resolution, a cold download and a large link phase can each pass without a single line, and two minutes of quiet is ordinary rather than wedged. A fail-fast that turns a slow install into a failed one is worse than the wait it saves. Liveness now also counts what a run cannot fake — changes under the profile it installs into — and the allowance goes to five minutes of neither output nor filesystem activity, still well inside the hosts' fifteen-minute ceiling that made a wedged install feel like a hang. The report says which of the two was missing, so a future firing is readable. That the timeout fired at all is worth recording: only this runner prints that line, so the packaged shim does reach it on Windows — which had been the open question behind every identical-looking failure report so far. --- .../pnpm-runner.mjs | 86 ++++++++++++++++--- test/pnpm-runner.test.js | 42 ++++++++- 2 files changed, 113 insertions(+), 15 deletions(-) diff --git a/packages/dsh-desktop-market-installer/pnpm-runner.mjs b/packages/dsh-desktop-market-installer/pnpm-runner.mjs index f2405cb5..99c8b177 100644 --- a/packages/dsh-desktop-market-installer/pnpm-runner.mjs +++ b/packages/dsh-desktop-market-installer/pnpm-runner.mjs @@ -21,19 +21,22 @@ * output, one pnpm run. */ import { spawn } from 'node:child_process' -import { existsSync } from 'node:fs' +import { existsSync, watch } from 'node:fs' import { rename } from 'node:fs/promises' +import { join } from 'node:path' import { fileURLToPath, pathToFileURL } from 'node:url' export const SIDELINE_MARKER = '.dsh-old-' export const RETRY_DELAY_MS = 750 /** - * How long pnpm may stay silent before this runner stops it. pnpm narrates - * resolution, fetching and linking as it goes, so silence this long is a stuck - * run, not a slow one — and failing here beats the hosts' fifteen-minute - * ceiling, which is what makes a failed install feel like a hang. + * How long a run may show no sign of life — no output, no change under the + * profile — before this runner stops it. Generous on purpose: pnpm without a + * TTY says nothing for long stretches, and packages download into a store that + * lives outside the profile, so a quiet minute is ordinary. Five is not, and + * failing there still beats the hosts' fifteen-minute ceiling, which is what + * makes a wedged install feel like a hang. */ -export const IDLE_TIMEOUT_MS = Number(process.env.DSH_DESKTOP_PNPM_IDLE_TIMEOUT_MS) || 120_000 +export const IDLE_TIMEOUT_MS = Number(process.env.DSH_DESKTOP_PNPM_IDLE_TIMEOUT_MS) || 300_000 /** How long a killed run may take to actually go away before it is written off. */ export const KILL_GRACE_MS = 5_000 /** Prefix of every line this runner contributes to a package operation's output. */ @@ -81,10 +84,13 @@ function delay(milliseconds) { * Run pnpm once, mirroring its streams to this process while keeping a copy * for failure classification. * - * A run that stops saying anything is stopped rather than waited out: pnpm - * narrates its progress line by line, so silence is not slow work, and the - * hosts above only bound the whole operation at fifteen minutes — long enough - * for a wedged install to look like a hang to the person watching. + * A run that has stopped doing anything is stopped rather than waited out — + * the hosts above only bound the whole operation at fifteen minutes, which is + * what makes a wedged install look like a hang. But silence alone does not + * mean stuck: without a TTY pnpm drops its progress display, and resolution, + * a cold download or a large link phase can pass without a single line. What + * a live run cannot do is leave the profile untouched, so the store and the + * profile's node_modules count as much as output does. */ function runPnpm(executable, args, options = {}) { const { @@ -92,6 +98,7 @@ function runPnpm(executable, args, options = {}) { idleTimeoutMs = IDLE_TIMEOUT_MS, killGraceMs = KILL_GRACE_MS, kill = killTree, + watchActivity = watchProfileActivity, report = () => undefined } = options @@ -105,32 +112,50 @@ function runPnpm(executable, args, options = {}) { let grace let stopped = false let settled = false + let lastActivity = Date.now() + const touch = () => { + lastActivity = Date.now() + } + const stopWatching = idleTimeoutMs > 0 ? watchActivity(touch) : undefined const finish = (result) => { if (settled) return settled = true clearTimeout(idle) clearTimeout(grace) + stopWatching?.() resolve({ ...result, output, idleTimedOut: stopped }) } const heartbeat = () => { clearTimeout(idle) if (idleTimeoutMs <= 0) return + const quietFor = Date.now() - lastActivity + // Work seen since the last check keeps the run: a quiet pnpm that is + // still writing packages is slow, not stuck. idle = setTimeout(() => { + const silence = Date.now() - lastActivity + if (silence < idleTimeoutMs) { + heartbeat() + return + } stopped = true - report(`pnpm said nothing for ${Math.round(idleTimeoutMs / 1000)}s; stopping it`) + report( + `pnpm produced no output and touched nothing for ${Math.round( + silence / 1000 + )}s; stopping it` + ) kill(child) // A kill that does not land must not become the hang this guards // against, so the run is written off either way. grace = setTimeout(() => finish({ code: 1, signal: null }), killGraceMs) grace.unref?.() - }, idleTimeoutMs) + }, Math.max(idleTimeoutMs - quietFor, 1_000)) idle.unref?.() } const observe = (chunk, stream) => { output = `${output}${chunk}`.slice(-256 * 1024) stream.write(chunk) - heartbeat() + touch() } child.stdout.on('data', (chunk) => observe(chunk, process.stdout)) @@ -138,6 +163,7 @@ function runPnpm(executable, args, options = {}) { child.once('error', (error) => { clearTimeout(idle) clearTimeout(grace) + stopWatching?.() if (!settled) { settled = true reject(error) @@ -148,6 +174,30 @@ function runPnpm(executable, args, options = {}) { }) } +/** + * Signal progress whenever the profile's package directories change. pnpm runs + * with the profile as its working directory, so that is where a live install + * shows up even while it says nothing. + * @param onActivity - called on any change; may fire often, so it stays cheap. + * @returns a function that stops watching. + */ +function watchProfileActivity(onActivity, cwd = process.cwd()) { + const watchers = [] + for (const directory of [cwd, join(cwd, 'node_modules'), join(cwd, 'node_modules', '.pnpm')]) { + try { + if (!existsSync(directory)) continue + const watcher = watch(directory, { persistent: false }, onActivity) + watcher.on('error', () => undefined) + watchers.push(watcher) + } catch { + // An unwatchable directory just does not contribute liveness. + } + } + return () => { + for (const watcher of watchers) watcher.close() + } +} + /** * Stop a pnpm run and everything it started. On Windows `kill` reaches only * the wrapper, so the tree goes through taskkill — but never *instead of* the @@ -183,6 +233,7 @@ export async function runWithLockRecovery(executable, args, options = {}) { idleTimeoutMs = IDLE_TIMEOUT_MS, killGraceMs = KILL_GRACE_MS, kill = killTree, + watchActivity = watchProfileActivity, // Every step announces itself on the same stream pnpm's own diagnostics // travel, because the market reports that stream verbatim: a report // without these lines is a report from a pnpm this runner never wrapped. @@ -190,7 +241,14 @@ export async function runWithLockRecovery(executable, args, options = {}) { } = options const run = () => - runPnpm(executable, args, { spawnProcess, idleTimeoutMs, killGraceMs, kill, report }) + runPnpm(executable, args, { + spawnProcess, + idleTimeoutMs, + killGraceMs, + kill, + watchActivity, + report + }) const first = await run() const blocked = first.code === 0 ? undefined : lockedRenameTarget(first.output) diff --git a/test/pnpm-runner.test.js b/test/pnpm-runner.test.js index f3ae84f7..b10eda7e 100644 --- a/test/pnpm-runner.test.js +++ b/test/pnpm-runner.test.js @@ -141,6 +141,7 @@ describe('packaged pnpm runner', () => { spawnProcess, idleTimeoutMs: 5, kill: (child) => child.kill(), + watchActivity: () => () => undefined, report: (message) => lines.push(message) }) @@ -148,7 +149,45 @@ describe('packaged pnpm runner', () => { expect(result.idleTimedOut).toBe(true) // A wedged run is not retried — three stuck runs are three times the wait. expect(calls).toHaveLength(1) - expect(lines[0]).toContain('said nothing') + expect(lines[0]).toContain('touched nothing') + }) + + it('keeps a quiet run that is still writing packages', async () => { + // Without a TTY pnpm drops its progress display: resolution, a cold + // download and a large link phase can each pass without a line. Killing + // those would turn a slow install into a failed one. + const { spawnProcess } = fakePnpm([{ silent: true }]) + let stopWatching = 0 + let signalActivity = () => undefined + + const running = runWithLockRecovery('/node', ['/pnpm.cjs', 'add', 'x'], { + spawnProcess, + idleTimeoutMs: 40, + kill: (child) => child.kill(), + watchActivity: (onActivity) => { + signalActivity = onActivity + return () => { + stopWatching += 1 + } + }, + report: () => undefined + }) + + // Three quiet-but-busy stretches, each shorter than the allowance. + for (let beat = 0; beat < 3; beat += 1) { + await new Promise((resolve) => setTimeout(resolve, 25)) + signalActivity() + } + signalActivity() + const result = await Promise.race([ + running, + new Promise((resolve) => setTimeout(() => resolve('still running'), 20)) + ]) + + expect(result).toBe('still running') + signalActivity = () => undefined + await expect(running).resolves.toMatchObject({ idleTimedOut: true }) + expect(stopWatching).toBe(1) }) it('gives up on a run whose kill never lands', async () => { @@ -161,6 +200,7 @@ describe('packaged pnpm runner', () => { idleTimeoutMs: 5, killGraceMs: 5, kill: () => undefined, + watchActivity: () => () => undefined, report: () => undefined }) From 34c2012091b5936949731b78e810aa7a8c02c19c Mon Sep 17 00:00:00 2001 From: yaojin3616 Date: Fri, 21 Aug 2026 07:41:13 -0700 Subject: [PATCH 12/14] fix(win32): recover the blocked rename even when pnpm hangs on it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The recovery was skipped in the one case it was written for. pnpm raises the locked rename inside a worker and does not always unwind from it, so the run reports the blocker and then stops responding — and the runner, having had to stop it, treated it as undiagnosable and returned without moving anything aside. A stopped run says nothing about whether its blocker can be cleared; what decides that is whether the output names one, so that is what the branch tests now. Being stopped for silence is also no longer the same event as failing. Once the blocked rename is in the output the outcome is decided, so the run gets twenty seconds to exit on its own (DSH_DESKTOP_PNPM_FAILURE_STALL_MS) rather than the five-minute idle allowance meant for a run nobody can read yet. The two-minute silence reported from Windows was this: dsh-market always passes --reporter=ndjson, which narrates every step, so a run that says nothing for that long has stopped working rather than gone quiet — the worker had already died on the rename. --- .../pnpm-runner.mjs | 42 +++++++++++++- test/pnpm-runner.test.js | 56 ++++++++++++++++++- 2 files changed, 92 insertions(+), 6 deletions(-) diff --git a/packages/dsh-desktop-market-installer/pnpm-runner.mjs b/packages/dsh-desktop-market-installer/pnpm-runner.mjs index 99c8b177..6c719422 100644 --- a/packages/dsh-desktop-market-installer/pnpm-runner.mjs +++ b/packages/dsh-desktop-market-installer/pnpm-runner.mjs @@ -39,6 +39,14 @@ export const RETRY_DELAY_MS = 750 export const IDLE_TIMEOUT_MS = Number(process.env.DSH_DESKTOP_PNPM_IDLE_TIMEOUT_MS) || 300_000 /** How long a killed run may take to actually go away before it is written off. */ export const KILL_GRACE_MS = 5_000 +/** + * How long a run gets to exit on its own after it has already reported the + * failure that decides it. pnpm raises the locked rename inside a worker and + * has been seen never to unwind from it, so the outcome is known long before + * the process admits it — waiting out the idle allowance there is pure delay. + */ +export const STALL_AFTER_FAILURE_MS = + Number(process.env.DSH_DESKTOP_PNPM_FAILURE_STALL_MS) || 20_000 /** Prefix of every line this runner contributes to a package operation's output. */ export const MARKER = 'dsh-desktop pnpm runner:' @@ -97,6 +105,7 @@ function runPnpm(executable, args, options = {}) { spawnProcess = spawn, idleTimeoutMs = IDLE_TIMEOUT_MS, killGraceMs = KILL_GRACE_MS, + stallAfterFailureMs = STALL_AFTER_FAILURE_MS, kill = killTree, watchActivity = watchProfileActivity, report = () => undefined @@ -112,6 +121,7 @@ function runPnpm(executable, args, options = {}) { let grace let stopped = false let settled = false + let doomed = false let lastActivity = Date.now() const touch = () => { lastActivity = Date.now() @@ -152,10 +162,31 @@ function runPnpm(executable, args, options = {}) { }, Math.max(idleTimeoutMs - quietFor, 1_000)) idle.unref?.() } + const stopWhenDoomed = () => { + if (doomed || lockedRenameTarget(output) === undefined) return + doomed = true + report( + `pnpm reported a blocked rename; giving it ${Math.round( + stallAfterFailureMs / 1000 + )}s to exit before stopping it` + ) + const stall = setTimeout(() => { + if (settled) return + stopped = true + kill(child) + grace = setTimeout(() => finish({ code: 1, signal: null }), killGraceMs) + grace.unref?.() + }, stallAfterFailureMs) + stall.unref?.() + } const observe = (chunk, stream) => { output = `${output}${chunk}`.slice(-256 * 1024) stream.write(chunk) touch() + // pnpm can raise the locked rename in a worker and then never unwind. + // The outcome is already decided, so the run is not owed the full idle + // allowance from here. + stopWhenDoomed() } child.stdout.on('data', (chunk) => observe(chunk, process.stdout)) @@ -232,6 +263,7 @@ export async function runWithLockRecovery(executable, args, options = {}) { retryDelayMs = RETRY_DELAY_MS, idleTimeoutMs = IDLE_TIMEOUT_MS, killGraceMs = KILL_GRACE_MS, + stallAfterFailureMs = STALL_AFTER_FAILURE_MS, kill = killTree, watchActivity = watchProfileActivity, // Every step announces itself on the same stream pnpm's own diagnostics @@ -245,6 +277,7 @@ export async function runWithLockRecovery(executable, args, options = {}) { spawnProcess, idleTimeoutMs, killGraceMs, + stallAfterFailureMs, kill, watchActivity, report @@ -252,9 +285,12 @@ export async function runWithLockRecovery(executable, args, options = {}) { const first = await run() const blocked = first.code === 0 ? undefined : lockedRenameTarget(first.output) - // A run stopped for silence is not retried: whatever wedged it is still - // there, and three stuck runs are three times the wait for the same answer. - if (blocked === undefined || first.idleTimedOut) return first + // Whether the run exited on its own or had to be stopped says nothing about + // whether the blocked rename can be recovered — and a run that names its + // blocker before hanging is exactly the one this recovery is for. Only a + // failure with no diagnosis is left alone: retrying that is three times the + // wait for the same answer. + if (blocked === undefined) return first report(`${blocked} could not be replaced; retrying in ${retryDelayMs}ms (2 of 3)`) await wait(retryDelayMs) diff --git a/test/pnpm-runner.test.js b/test/pnpm-runner.test.js index b10eda7e..05681545 100644 --- a/test/pnpm-runner.test.js +++ b/test/pnpm-runner.test.js @@ -190,6 +190,56 @@ describe('packaged pnpm runner', () => { expect(stopWatching).toBe(1) }) + it('recovers a run that named its blocker and then hung', async () => { + // pnpm raises the locked rename inside a worker and has been seen never to + // unwind from it. That run has to be stopped — and it is also the exact + // run this recovery exists for, so being stopped must not skip it. + const calls = [] + const spawnProcess = () => { + const attempt = calls.length + calls.push(attempt) + const child = new EventEmitter() + child.stdout = new EventEmitter() + child.stderr = new EventEmitter() + child.pid = 4242 + child.kill = () => { + child.emit('exit', null, 'SIGKILL') + return true + } + queueMicrotask(() => { + if (attempt < 2) { + // Reports the failure, then never exits. + child.stderr.emit('data', WINDOWS_LOCK_FAILURE) + return + } + child.emit('exit', 0, null) + }) + return child + } + const moveAside = vi.fn(async () => undefined) + + const result = await runWithLockRecovery('/node', ['/pnpm.cjs', 'add', 'x'], { + spawnProcess, + moveAside, + exists: () => true, + wait: async () => undefined, + now: () => 99, + idleTimeoutMs: 10_000, + stallAfterFailureMs: 5, + killGraceMs: 5, + kill: (child) => child.kill(), + watchActivity: () => () => undefined, + report: () => undefined + }) + + expect(result.code).toBe(0) + expect(calls).toHaveLength(3) + expect(moveAside).toHaveBeenCalledWith( + BLOCKED_TARGET, + `${BLOCKED_TARGET}${SIDELINE_MARKER}99` + ) + }) + it('gives up on a run whose kill never lands', async () => { // taskkill can miss on Windows. Waiting for an exit that never comes would // be the very hang the idle timeout exists to prevent. @@ -225,9 +275,9 @@ describe('packaged pnpm runner', () => { report: (message) => lines.push(message) }) - expect(lines[0]).toContain('retrying') - expect(lines[1]).toContain(`moved ${BLOCKED_TARGET}`) - expect(lines[1]).toContain(SIDELINE_MARKER) + expect(lines.join('\n')).toContain('retrying') + const moved = lines.find((line) => line.startsWith(`moved ${BLOCKED_TARGET}`)) + expect(moved).toContain(SIDELINE_MARKER) expect(lines.at(-1)).toContain('succeeded') expect(MARKER).toContain('dsh-desktop') }) From eee692c9817e8e63c5ffa2d801ab69b0185aa468 Mon Sep 17 00:00:00 2001 From: yaojin3616 Date: Fri, 21 Aug 2026 08:00:06 -0700 Subject: [PATCH 13/14] fix: repair the profile with Harness actually stopped MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Windows log showed the repair running against a live Harness: on a restart, launchHarness() repaired before runtime.start(), and start() is what stops the previous process. So the one step that assumes nothing holds the profile ran in exactly the condition it exists to avoid, and its reinstall failed — clearing three damaged directories without putting the packages back, which leaves the profile worse than it found it. The launch path now stops Harness itself before repairing. Two things the same log made visible: The desktop and the Harness-side installer both write shims into /.desktop-bin, but only one of them routed pnpm through the lock-recovery runner. A desktop-written shim replaced a runner-routed pnpm with a plain one, silently dropping the recovery until Harness next rewrote them. Both writers now share the same command. And a failed repair reported "dsh: pnpm failed in profile directory …" — dsh's own wrapper line, which is always last and names no cause. The line that names one is reported instead, so the next failure is readable. --- src/main/index.ts | 23 +++++++++- src/main/runtime/profile-plugin-command.ts | 43 ++++++++++++++++-- test/profile-plugin-command.test.ts | 52 +++++++++++++++++++++- 3 files changed, 113 insertions(+), 5 deletions(-) diff --git a/src/main/index.ts b/src/main/index.ts index 3a06b51d..b52efc60 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -265,6 +265,21 @@ function bundledNodePath(): string { return join(app.getAppPath(), 'node_modules', 'node', 'bin', executable) } +/** + * The packaged lock-recovery runner. The Harness-side installer stages its own + * copy into .desktop-bin; the desktop writes shims to that same directory, so + * it points at the same runner rather than replacing them with a plain pnpm + * call that would silently drop the recovery. + */ +function bundledPnpmRunnerPath(): string { + return join( + app.getAppPath(), + 'node_modules', + 'dsh-desktop-market-installer', + 'pnpm-runner.mjs' + ) +} + function bundledPnpmEntryPath(): string { const root = join(app.getAppPath(), 'node_modules', 'pnpm', 'bin') const candidates = [join(root, 'pnpm.cjs'), join(root, 'pnpm.mjs')] @@ -472,7 +487,8 @@ async function repairProfilePackages(dshHome: string): Promise { dshHome, dshEntryPath: dshEntryPath(), nodeExecutablePath: bundledNodePath(), - pnpmEntryPath: bundledPnpmEntryPath() + pnpmEntryPath: bundledPnpmEntryPath(), + pnpmRunnerPath: bundledPnpmRunnerPath() }) runtime.note( result.ok @@ -492,6 +508,10 @@ function launchHarness(): Promise { harnessLaunchOperation = (async () => { const dshHome = join(app.getPath('userData'), 'harness') await showSplash() + // The repair only holds on a stopped Harness, and a restart still has the + // previous one running: start() stops it, but that is after the repair. + // Stopping here is what makes the window this launch path assumes. + await runtime.stop() await repairProfilePackages(dshHome) await pruneMissingProfileBundles(dshHome).catch(() => false) await runtime.start(launchDirectory) @@ -747,6 +767,7 @@ async function showPluginRecovery(options?: { dshEntryPath: dshEntryPath(), nodeExecutablePath: bundledNodePath(), pnpmEntryPath: bundledPnpmEntryPath(), + pnpmRunnerPath: bundledPnpmRunnerPath(), environment: process.env }, pluginName diff --git a/src/main/runtime/profile-plugin-command.ts b/src/main/runtime/profile-plugin-command.ts index 200238b5..efb9ec85 100644 --- a/src/main/runtime/profile-plugin-command.ts +++ b/src/main/runtime/profile-plugin-command.ts @@ -13,6 +13,13 @@ export interface ProfilePluginCommandOptions { dshEntryPath: string nodeExecutablePath: string pnpmEntryPath: string + /** + * The packaged lock-recovery runner. The shims below share a directory with + * the ones the Harness-side installer writes, so leaving this out would + * replace a runner-routed pnpm with a plain one and silently drop the + * recovery until Harness next rewrote them. + */ + pnpmRunnerPath?: string environment?: NodeJS.ProcessEnv } @@ -42,14 +49,25 @@ export function buildProfileInstallArguments(dshEntryPath: string): string[] { return [dshEntryPath, 'plugin', '--profile', PROFILE, 'install'] } +export function buildPnpmShimCommand(options: ProfilePluginCommandOptions): string[] { + const runner = + options.pnpmRunnerPath !== undefined && existsSync(options.pnpmRunnerPath) + ? [options.pnpmRunnerPath] + : [] + return [...runner, options.pnpmEntryPath] +} + export async function ensureProfilePnpmShim(options: ProfilePluginCommandOptions): Promise { const directory = join(options.dshHome, '.desktop-bin') await mkdir(directory, { recursive: true }) + const command = buildPnpmShimCommand(options) if (process.platform === 'win32') { await writeFile( join(directory, 'pnpm.cmd'), - `@chcp 65001 >nul\r\n@echo off\r\n"${options.nodeExecutablePath}" "${options.pnpmEntryPath}" %*\r\n`, + `@chcp 65001 >nul\r\n@echo off\r\n"${options.nodeExecutablePath}" ${command + .map((part) => `"${part}"`) + .join(' ')} %*\r\n`, 'utf8' ) await writeFile( @@ -61,7 +79,9 @@ export async function ensureProfilePnpmShim(options: ProfilePluginCommandOptions const pnpmPath = join(directory, 'pnpm') await writeFile( pnpmPath, - `#!/bin/sh\nexec ${shellQuote(options.nodeExecutablePath)} ${shellQuote(options.pnpmEntryPath)} "$@"\n`, + `#!/bin/sh\nexec ${shellQuote(options.nodeExecutablePath)} ${command + .map(shellQuote) + .join(' ')} "$@"\n`, { encoding: 'utf8', mode: 0o755 } ) await chmod(pnpmPath, 0o755) @@ -103,6 +123,23 @@ export function buildProfilePluginCommandEnvironment( return result } +/** + * The line worth reporting from a failed run. dsh's own wrapper ("pnpm failed + * in profile directory …") is always last and names no cause, so a line that + * does name one wins — otherwise a failure reads as a dead end. + */ +export function diagnosticLine(output: string): string | undefined { + const lines = output + .trim() + .split(/\r?\n/u) + .map((line) => line.trim()) + .filter(Boolean) + const named = lines.filter((line: string) => + /EPERM|EBUSY|EACCES|EEXIST|ENOTEMPTY|ENOENT|ERR_PNPM|error:/u.test(line) + ) + return (named.at(-1) ?? lines.at(-1))?.slice(0, 800) +} + function killProcessTree(child: ReturnType): void { if (child.exitCode !== null || !child.pid) return if (process.platform === 'win32') { @@ -207,7 +244,7 @@ async function runProfileCommand( } } if (exit.code !== 0) { - const detail = output.trim().split(/\r?\n/u).at(-1)?.slice(0, 800) + const detail = diagnosticLine(output) return { ok: false, detail: diff --git a/test/profile-plugin-command.test.ts b/test/profile-plugin-command.test.ts index 9a5f3138..745a2284 100644 --- a/test/profile-plugin-command.test.ts +++ b/test/profile-plugin-command.test.ts @@ -1,7 +1,19 @@ import { mkdir, readFile, rm, writeFile } from 'node:fs/promises' import { join } from 'node:path' import { afterEach, describe, expect, it } from 'vitest' -import { removeProfilePluginWithDsh } from '../src/main/runtime/profile-plugin-command' +import { + buildPnpmShimCommand, + diagnosticLine, + removeProfilePluginWithDsh +} from '../src/main/runtime/profile-plugin-command' + +const existingRunnerPath = join( + __dirname, + '..', + 'packages', + 'dsh-desktop-market-installer', + 'pnpm-runner.mjs' +) describe('profile-plugin-command', () => { const testDir = join(__dirname, '.temp-profile-plugin-command-test') @@ -56,3 +68,41 @@ describe('profile-plugin-command', () => { }) }) }) + +describe('profile pnpm shim and failure reporting', () => { + it('keeps the desktop shim on the same lock-recovery runner Harness uses', () => { + // Both writers share /.desktop-bin, so a desktop-written shim + // that called pnpm directly would silently drop the recovery until + // Harness next rewrote them. + const base = { + dshHome: '/home/.dsh', + dshEntryPath: '/app/dsh/bin.js', + nodeExecutablePath: '/app/node', + pnpmEntryPath: '/app/pnpm.cjs' + } + + expect(buildPnpmShimCommand(base)).toEqual(['/app/pnpm.cjs']) + expect( + buildPnpmShimCommand({ ...base, pnpmRunnerPath: '/app/missing-runner.mjs' }) + ).toEqual(['/app/pnpm.cjs']) + expect( + buildPnpmShimCommand({ ...base, pnpmRunnerPath: existingRunnerPath }) + ).toEqual([existingRunnerPath, '/app/pnpm.cjs']) + }) + + it('reports the failure that names a cause, not dsh’s wrapper line', () => { + // dsh always ends with "pnpm failed in profile directory …", which names + // nothing — reporting that turns every failure into a dead end. + expect( + diagnosticLine( + [ + 'Progress: resolved 120, reused 118', + "error: EPERM: operation not permitted, rename 'x_tmp_1_1' -> 'x'", + 'dsh: pnpm failed in profile directory C:\\profiles\\web' + ].join('\n') + ) + ).toContain('EPERM') + expect(diagnosticLine('a\nb\nlast line')).toBe('last line') + expect(diagnosticLine(' ')).toBeUndefined() + }) +}) From 56d7568950e66dd06d41b5e91a3c75772126e6f9 Mon Sep 17 00:00:00 2001 From: yaojin3616 Date: Sat, 22 Aug 2026 00:27:46 +0800 Subject: [PATCH 14/14] fix(win32): stop trusting a recursive rm that removes nothing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Updating a plugin never succeeded on a Windows account whose name is not ASCII. `C:\Users\数据项素\...` is every path a profile has, and Node's recursive `rm` removes nothing under such a path while still resolving successfully: rmSync('C:\Users\Public\ascii', { recursive: true }) -> gone rmSync('C:\Users\Public\中文', { recursive: true }) -> still there That is the whole failure. Replacing a package means renaming a staging directory onto an existing one, which Windows refuses outright; pnpm's recovery deletes the destination first and retries for a minute, and its delete is `fs.rmSync(p, { recursive: true, force: true })` — so the destination never goes away and all sixty seconds are spent losing. Each blocked package costs a minute before ERR_PNPM_EPERM, and the desktop's own sweeps were the same call: one reported twelve cleared directories over a profile where all twelve were still on disk. Every cleanup now walks the tree itself — unlink each file, rmdir on the way out, symlinks detached rather than followed — and confirms the path is gone before reporting it. The sweeps reach into a package's own node_modules too, where a replaced dependency of a dependency stages. Two things that kept the escape hatches shut are opened with it. The runner frees every destination the failed run staged for instead of only the one pnpm named, because an update blocks on several at once and one per attempt cannot finish inside three. And the launch repair stops asking for a frozen lockfile: a `pnpm add` that dies while linking has already written the new version into pnpm-lock.yaml while package.json still names the old one, so freezing there fails on the divergence that the repair exists to resolve. Co-Authored-By: Claude Opus 5 --- .../dsh-desktop-market-installer/index.js | 31 ++++-- .../pnpm-runner.mjs | 93 ++++++++++++++--- .../remove-tree.mjs | 71 +++++++++++++ src/main/runtime/profile-plugin-command.ts | 11 ++- src/main/state/plugin-recovery.ts | 7 +- src/main/state/profile-repair.ts | 34 ++++++- src/main/state/remove-tree.ts | 78 +++++++++++++++ test/market-installer.test.js | 39 ++++++++ test/pnpm-runner.test.js | 77 ++++++++++++++- test/profile-repair.test.ts | 45 ++++++++- test/remove-tree.test.ts | 99 +++++++++++++++++++ 11 files changed, 553 insertions(+), 32 deletions(-) create mode 100644 packages/dsh-desktop-market-installer/remove-tree.mjs create mode 100644 src/main/state/remove-tree.ts create mode 100644 test/remove-tree.test.ts diff --git a/packages/dsh-desktop-market-installer/index.js b/packages/dsh-desktop-market-installer/index.js index 9af3a9eb..0bd7728a 100644 --- a/packages/dsh-desktop-market-installer/index.js +++ b/packages/dsh-desktop-market-installer/index.js @@ -7,6 +7,7 @@ import { delimiter, dirname, join, resolve } from 'node:path' import { fileURLToPath } from 'node:url' import { SIDELINE_MARKER } from './pnpm-runner.mjs' +import { removeTree } from './remove-tree.mjs' export const RECOMMENDED_MARKET_VERSION = '1.15.0' export const MARKET_PACKAGE = 'dshmarket' @@ -34,19 +35,35 @@ export function isDisposableModuleDirectory(name) { return name.includes('_tmp_') || name.includes(SIDELINE_MARKER) } +/** + * Sweep the leftovers of interrupted pnpm runs from a profile's node_modules. + * + * A package's own node_modules is swept too. Once the package being replaced + * is a dependency of a dependency, that is where the leftovers land — + * `cytoscape-fcose/node_modules/cose-base.dsh-old-…` — and a sweep that stops + * at the top level leaves one copy behind per attempt. + */ export async function cleanStaleTemporaryDirectories(home = dshHome()) { const directory = profileDirectory(home) - const nodeModulesPath = join(directory, 'node_modules') - try { - const entries = await readdir(nodeModulesPath, { withFileTypes: true }) + const sweep = async (nodeModulesPath) => { + let entries + try { + entries = await readdir(nodeModulesPath, { withFileTypes: true }) + } catch { + // node_modules directory may not exist yet + return + } for (const entry of entries) { - if (entry.isDirectory() && isDisposableModuleDirectory(entry.name)) { - await rm(join(nodeModulesPath, entry.name), { recursive: true, force: true }).catch(() => undefined) + if (!entry.isDirectory() || entry.isSymbolicLink()) continue + const path = join(nodeModulesPath, entry.name) + if (isDisposableModuleDirectory(entry.name)) { + await removeTree(path).catch(() => undefined) + continue } + await sweep(entry.name.startsWith('@') ? path : join(path, 'node_modules')) } - } catch { - // node_modules directory may not exist yet } + await sweep(join(directory, 'node_modules')) } function readObject(text) { diff --git a/packages/dsh-desktop-market-installer/pnpm-runner.mjs b/packages/dsh-desktop-market-installer/pnpm-runner.mjs index 6c719422..cf88a9d9 100644 --- a/packages/dsh-desktop-market-installer/pnpm-runner.mjs +++ b/packages/dsh-desktop-market-installer/pnpm-runner.mjs @@ -22,7 +22,7 @@ */ import { spawn } from 'node:child_process' import { existsSync, watch } from 'node:fs' -import { rename } from 'node:fs/promises' +import { readdir, rename } from 'node:fs/promises' import { join } from 'node:path' import { fileURLToPath, pathToFileURL } from 'node:url' @@ -258,6 +258,10 @@ export async function runWithLockRecovery(executable, args, options = {}) { spawnProcess = spawn, moveAside = rename, exists = existsSync, + listEntries = readEntries, + // pnpm runs with the profile as its working directory, so that is where + // the staging left by a failed run is found. + profileDirectory = process.cwd(), wait = delay, now = Date.now, retryDelayMs = RETRY_DELAY_MS, @@ -295,31 +299,92 @@ export async function runWithLockRecovery(executable, args, options = {}) { report(`${blocked} could not be replaced; retrying in ${retryDelayMs}ms (2 of 3)`) await wait(retryDelayMs) const second = await run() - const target = second.code === 0 ? undefined : lockedRenameTarget(second.output) - if (target === undefined) { + const named = second.code === 0 ? undefined : lockedRenameTarget(second.output) + if (named === undefined) { if (second.code === 0) report('the retry succeeded') return second } - if (!exists(target)) { - report(`${target} is gone; leaving pnpm's own diagnosis in place`) - return second + // pnpm names one blocked destination per run — the first its workers hit — + // but an update blocks on every package it has to replace. Freeing only the + // named one buys a single package per attempt, so an update that replaces + // four of them can never finish inside three. Every destination pnpm staged + // for is already on disk next to its `_tmp__`, so the whole set + // is knowable from one failure, and the whole set is what gets freed here. + const targets = await blockedTargets(named, profileDirectory, { listEntries, exists }) + const freed = [] + for (const target of targets) { + const sideline = sidelinePath(target, now()) + try { + await moveAside(target, sideline) + freed.push(target) + } catch (error) { + // The directory itself is held too — nothing left to try for this one, + // and the run's own diagnostics are already on stderr. + report(`${target} could not be moved aside (${errorText(error)})`) + } } - const sideline = sidelinePath(target, now()) - try { - await moveAside(target, sideline) - } catch (error) { - // The directory itself is held too — nothing left to try, and the run's - // own diagnostics are already on stderr. - report(`${target} could not be moved aside either (${errorText(error)})`) + + if (freed.length === 0) { + report(`nothing could be freed; leaving pnpm's own diagnosis in place`) return second } - report(`moved ${target} to ${sideline}; installing over the freed name (3 of 3)`) + report( + `freed ${freed.length} blocked ${ + freed.length === 1 ? 'destination' : 'destinations' + } (${freed.join(', ')}); installing over them (3 of 3)` + ) const third = await run() report(third.code === 0 ? 'the install succeeded' : 'the install failed again') return third } +/** The `_tmp__` staging name pnpm leaves beside its destination. */ +const STAGING_PATTERN = /^(?.+)_tmp_\d+_\d+$/u + +/** + * Every destination the failed run still has staging for, plus the one pnpm + * named. A staging directory sits beside the destination it was built for, so + * stripping the suffix names that destination without parsing pnpm's output + * twice — and it finds the ones pnpm never got far enough to report. + * + * Nested node_modules are walked because a replaced dependency of a dependency + * stages there, not at the top level. + * @param named - the destination pnpm reported, always included when present. + * @param root - the profile directory pnpm ran in. + * @returns absolute destination paths, deduplicated, each one present on disk. + */ +export async function blockedTargets(named, root, options = {}) { + const { listEntries = readEntries, exists = existsSync } = options + const found = new Set() + + const walk = async (directory) => { + for (const entry of await listEntries(directory)) { + if (!entry.isDirectory() || entry.isSymbolicLink()) continue + const path = join(directory, entry.name) + const staged = STAGING_PATTERN.exec(entry.name) + if (staged !== null) { + found.add(join(directory, staged.groups.packageName)) + continue + } + await walk(entry.name.startsWith('@') ? path : join(path, 'node_modules')) + } + } + await walk(join(root, 'node_modules')) + + // pnpm's own diagnosis leads, because it names what actually blocked the run. + const ordered = [named, ...found].filter((path) => path !== undefined) + return [...new Set(ordered)].filter((path) => exists(path)) +} + +async function readEntries(directory) { + try { + return await readdir(directory, { withFileTypes: true }) + } catch { + return [] + } +} + /* v8 ignore start -- the process wrapper around the tested runner */ if (process.argv[1] !== undefined && import.meta.url === pathToFileURL(process.argv[1]).href) { const [pnpmEntry, ...pnpmArguments] = process.argv.slice(2) diff --git a/packages/dsh-desktop-market-installer/remove-tree.mjs b/packages/dsh-desktop-market-installer/remove-tree.mjs new file mode 100644 index 00000000..e634edef --- /dev/null +++ b/packages/dsh-desktop-market-installer/remove-tree.mjs @@ -0,0 +1,71 @@ +/** + * Delete a directory tree without `rm`'s recursive mode. + * + * Node's recursive `rm` is unusable on this desktop's Windows installs: for + * any path containing a non-ASCII character it removes nothing and still + * resolves successfully. A profile lives under the user's home, so a single + * non-ASCII character in the account name — `C:\Users\数据项素\…` — silently + * disables every cleanup performed there, and each one reports the removal it + * did not make: + * + * rmSync('C:\\Users\\Public\\ascii', { recursive: true }) -> gone + * rmSync('C:\\Users\\Public\\中文', { recursive: true }) -> still there + * + * The single-entry calls are unaffected, so the walk below is done by hand: + * `unlink` each file, `rmdir` each directory on the way out. Symlinks are + * unlinked rather than followed, so a link into pnpm's store never takes the + * store's contents with it. + * + * This is the Harness-side copy of `src/main/state/remove-tree.ts`. The two + * run in different processes — this one inside Harness, the other in the + * desktop's main process — and the desktop's build does not reach into this + * package's sources, so the walk is stated in both places rather than shared + * through an import that neither side could satisfy. + */ +import { lstat, readdir, rmdir, unlink } from 'node:fs/promises' +import { join } from 'node:path' + +/** + * Remove a file or directory tree. + * @param {string} path - the path to remove; a missing path is not an error. + */ +export async function removeTree(path) { + let entry + try { + entry = await lstat(path) + } catch (error) { + if (error?.code === 'ENOENT') return + throw error + } + + // A symlink to a directory reports as one; unlinking is what detaches it + // without touching what it points at. + if (!entry.isDirectory() || entry.isSymbolicLink()) { + await unlink(path) + return + } + + for (const child of await readdir(path, { withFileTypes: true })) { + await removeTree(join(path, child.name)) + } + await rmdir(path) +} + +/** + * Remove a tree, reporting whether it is gone rather than throwing. + * @param {string} path - the path to remove. + * @returns {Promise} whether the path is no longer present. + */ +export async function removeTreeIfPossible(path) { + try { + await removeTree(path) + } catch { + // Fall through to the check: a partial removal still counts for nothing. + } + try { + await lstat(path) + return false + } catch (error) { + return error?.code === 'ENOENT' + } +} diff --git a/src/main/runtime/profile-plugin-command.ts b/src/main/runtime/profile-plugin-command.ts index efb9ec85..2a7e7eb7 100644 --- a/src/main/runtime/profile-plugin-command.ts +++ b/src/main/runtime/profile-plugin-command.ts @@ -44,9 +44,18 @@ export function buildProfilePluginRemoveArguments( * starts, which is the only moment the packages it would otherwise hold open * can be replaced — so it is also how a profile left damaged by an earlier * failure gets its packages back. + * + * The lockfile is explicitly allowed to move. This repair runs with CI set, + * which is pnpm's signal to install with a frozen lockfile, and the profile it + * has to repair is exactly the one where the lockfile cannot be trusted: a + * `pnpm add` that fails while linking has already written the new version into + * pnpm-lock.yaml while package.json still names the old one. Frozen there + * fails on the divergence — `ERR_PNPM_OUTDATED_LOCKFILE` — which turns the one + * path out of a damaged profile into another way to stay in it. The manifest + * is what the profile is meant to be; the lockfile follows it. */ export function buildProfileInstallArguments(dshEntryPath: string): string[] { - return [dshEntryPath, 'plugin', '--profile', PROFILE, 'install'] + return [dshEntryPath, 'plugin', '--profile', PROFILE, 'install', '--no-frozen-lockfile'] } export function buildPnpmShimCommand(options: ProfilePluginCommandOptions): string[] { diff --git a/src/main/state/plugin-recovery.ts b/src/main/state/plugin-recovery.ts index e50d2778..2f824df9 100644 --- a/src/main/state/plugin-recovery.ts +++ b/src/main/state/plugin-recovery.ts @@ -2,6 +2,7 @@ import { existsSync } from 'node:fs' import { readFile, readdir, rm, writeFile } from 'node:fs/promises' import { dirname, join, resolve } from 'node:path' import { parse } from 'yaml' +import { removeTree } from './remove-tree' /** * Directories under the profile's node_modules that no longer belong to any @@ -461,7 +462,7 @@ export async function resetPluginProfile( if (existsSync(nodeModulesPath)) { if (failingPlugin) { const pluginDir = join(nodeModulesPath, failingPlugin) - await rm(pluginDir, { recursive: true, force: true }).catch(() => undefined) + await removeTree(pluginDir).catch(() => undefined) if (failingPlugin.startsWith('@')) { const scope = failingPlugin.split('/')[0] if (scope) { @@ -469,7 +470,7 @@ export async function resetPluginProfile( try { const files = await readdir(scopeDir) if (files.length === 0) { - await rm(scopeDir, { recursive: true, force: true }).catch(() => undefined) + await removeTree(scopeDir).catch(() => undefined) } } catch {} } @@ -507,7 +508,7 @@ export async function pruneMissingProfileBundles(dshHome: string): Promise undefined) + await removeTree(join(nodeModulesPath, entry.name)).catch(() => undefined) } } } catch { diff --git a/src/main/state/profile-repair.ts b/src/main/state/profile-repair.ts index a964ed5a..4b5b6c3b 100644 --- a/src/main/state/profile-repair.ts +++ b/src/main/state/profile-repair.ts @@ -1,7 +1,8 @@ import { existsSync } from 'node:fs' -import { readFile, readdir, rm } from 'node:fs/promises' +import { lstat, readFile, readdir } from 'node:fs/promises' import { dirname, join } from 'node:path' import { isDisposableModuleDirectory, profilePackageJsonPath } from './plugin-recovery' +import { removeTree } from './remove-tree' /** * What a failed package operation leaves behind, and how the next launch gets @@ -40,6 +41,12 @@ async function isMaterializedPackage(directory: string): Promise { * manifest behind it. Scoped directories are inspected one level down, where * the packages actually live. Symlinks are left alone — pnpm's isolated layout * points them into the virtual store, and a broken link is not ours to judge. + * + * A package that is intact still gets its own node_modules looked through, + * because that is where the leftovers hide once a dependency of a dependency + * is the one being replaced: `cytoscape-fcose/node_modules/cose-base.dsh-old-…` + * is invisible to a scan that stops at the top level, and it accumulates one + * copy per attempt. * @param dshHome - the desktop's DSH home. * @returns absolute paths, in the order found. */ @@ -67,7 +74,11 @@ export async function findDamagedPackageDirectories(dshHome: string): Promise { @@ -85,8 +102,8 @@ export async function clearDamagedPackageDirectories(dshHome: string): Promise { + try { + await lstat(path) + return true + } catch { + return false + } +} + /** Whether the profile is worth repairing at all — no profile, nothing to do. */ export function hasProfile(dshHome: string): boolean { return existsSync(profilePackageJsonPath(dshHome)) diff --git a/src/main/state/remove-tree.ts b/src/main/state/remove-tree.ts new file mode 100644 index 00000000..92ce4e7d --- /dev/null +++ b/src/main/state/remove-tree.ts @@ -0,0 +1,78 @@ +import { lstat, readdir, rm, rmdir, unlink } from 'node:fs/promises' +import { join } from 'node:path' + +/** + * Delete a directory tree without `rm`'s recursive mode. + * + * Node's recursive `rm` is unusable on this desktop's Windows installs: for + * any path containing a non-ASCII character it removes nothing and still + * resolves successfully. A profile lives under the user's home, so a single + * non-ASCII character in the account name — `C:\Users\数据项素\…` — silently + * disables every cleanup the desktop performs, and each one reports the + * removal it did not make: + * + * rmSync('C:\\Users\\Public\\ascii', { recursive: true }) -> gone + * rmSync('C:\\Users\\Public\\中文', { recursive: true }) -> still there + * + * The single-entry calls are unaffected, so the walk below is done by hand: + * `unlink` each file, `rmdir` each directory on the way out. Symlinks are + * unlinked rather than followed, so a link into pnpm's store never takes the + * store's contents with it. + * + * pnpm's own cleanup is the same broken call (`@zkochan/rimraf` is a wrapper + * around `fs.rmSync(p, { recursive: true, force: true })`), which is why a + * package that has to be replaced on these installs can never be: pnpm's + * recovery from a blocked rename deletes the destination first, that delete + * does nothing, and the retry it guards then fails for the full minute it + * allows. Nothing here can fix pnpm, but everything the desktop clears for it + * has to actually clear. + * + * @param path - the file or directory to remove; a missing path is not an error. + */ +export async function removeTree(path: string): Promise { + let entry + try { + entry = await lstat(path) + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') return + throw error + } + + // A symlink to a directory reports as one; unlinking is what detaches it + // without touching what it points at. + if (!entry.isDirectory() || entry.isSymbolicLink()) { + await unlink(path) + return + } + + const children = await readdir(path, { withFileTypes: true }) + for (const child of children) { + await removeTree(join(path, child.name)) + } + await rmdir(path) +} + +/** + * Remove a tree, reporting whether it is gone rather than throwing. Useful + * where a leftover that cannot be removed is worth reporting but not worth + * failing over — the caller's next step usually names it anyway. + * @param path - the file or directory to remove. + * @returns whether the path is no longer present. + */ +export async function removeTreeIfPossible(path: string): Promise { + try { + await removeTree(path) + return true + } catch { + // A last try through the platform's own recursion: it is a no-op on the + // paths this module exists for, but it costs nothing and covers whatever + // the hand-walk could not express. + try { + await rm(path, { recursive: true, force: true }) + await lstat(path) + return false + } catch (error) { + return (error as NodeJS.ErrnoException).code === 'ENOENT' + } + } +} diff --git a/test/market-installer.test.js b/test/market-installer.test.js index db7ce3ac..82f9f2d3 100644 --- a/test/market-installer.test.js +++ b/test/market-installer.test.js @@ -105,6 +105,45 @@ describe('desktop plugin market installer', () => { expect(existsSync(validDir)).toBe(true) }) + it('cleans the leftovers inside a package’s own node_modules', async () => { + // A replaced dependency of a dependency stages under the dependent, so a + // sweep that stops at the top level leaves one copy behind per attempt. + const home = await mkdtemp(join(tmpdir(), 'dsh-market-clean-nested-')) + const nodeModules = join(home, 'profiles', 'web', 'node_modules') + const nested = join(nodeModules, 'cytoscape-fcose', 'node_modules') + const scoped = join(nodeModules, '@deepseek-ai') + const nestedStale = join(nested, 'cose-base.dsh-old-1787327060846') + const scopedStale = join(scoped, 'dsh-settings_tmp_7408_2') + const kept = join(nested, 'cose-base') + await mkdir(nestedStale, { recursive: true }) + await mkdir(scopedStale, { recursive: true }) + await mkdir(kept, { recursive: true }) + await writeFile(join(kept, 'package.json'), JSON.stringify({ name: 'cose-base' }), 'utf8') + + await cleanStaleTemporaryDirectories(home) + + const { existsSync } = await import('node:fs') + expect(existsSync(nestedStale)).toBe(false) + expect(existsSync(scopedStale)).toBe(false) + expect(existsSync(kept)).toBe(true) + }) + + it('clears a tree whose path is not ASCII', async () => { + // Node's recursive `rm` reports success and removes nothing under such a + // path on Windows. A profile lives under the user's home, so one non-ASCII + // character in the account name used to disable this sweep entirely. + const home = join(await mkdtemp(join(tmpdir(), 'dsh-market-unicode-')), '数据项素') + const nodeModules = join(home, 'profiles', 'web', 'node_modules') + const stale = join(nodeModules, 'dshmarket_tmp_7408_13', 'lib') + await mkdir(stale, { recursive: true }) + await writeFile(join(stale, 'index.js'), 'export default 1', 'utf8') + + await cleanStaleTemporaryDirectories(home) + + const { existsSync } = await import('node:fs') + expect(existsSync(join(nodeModules, 'dshmarket_tmp_7408_13'))).toBe(false) + }) + it('reports both the requested dependency and installed package version', async () => { const home = await mkdtemp(join(tmpdir(), 'dsh-market-status-')) const profile = join(home, 'profiles', 'web') diff --git a/test/pnpm-runner.test.js b/test/pnpm-runner.test.js index 05681545..f49ec11d 100644 --- a/test/pnpm-runner.test.js +++ b/test/pnpm-runner.test.js @@ -1,8 +1,10 @@ import { EventEmitter } from 'node:events' +import { join } from 'node:path' import { describe, expect, it, vi } from 'vitest' import { MARKER, SIDELINE_MARKER, + blockedTargets, lockedRenameTarget, runWithLockRecovery, sidelinePath @@ -121,6 +123,7 @@ describe('packaged pnpm runner', () => { spawnProcess, moveAside, exists: () => true, + listEntries: async () => [], wait: async () => undefined, now: () => 1234 }) @@ -222,6 +225,7 @@ describe('packaged pnpm runner', () => { spawnProcess, moveAside, exists: () => true, + listEntries: async () => [], wait: async () => undefined, now: () => 99, idleTimeoutMs: 10_000, @@ -270,14 +274,15 @@ describe('packaged pnpm runner', () => { spawnProcess, moveAside: async () => undefined, exists: () => true, + listEntries: async () => [], wait: async () => undefined, now: () => 1234, report: (message) => lines.push(message) }) expect(lines.join('\n')).toContain('retrying') - const moved = lines.find((line) => line.startsWith(`moved ${BLOCKED_TARGET}`)) - expect(moved).toContain(SIDELINE_MARKER) + const freed = lines.find((line) => line.startsWith('freed ')) + expect(freed).toContain(BLOCKED_TARGET) expect(lines.at(-1)).toContain('succeeded') expect(MARKER).toContain('dsh-desktop') }) @@ -294,6 +299,7 @@ describe('packaged pnpm runner', () => { throw new Error('EPERM') }, exists: () => true, + listEntries: async () => [], wait: async () => undefined }) @@ -301,4 +307,71 @@ describe('packaged pnpm runner', () => { expect(result.output).toContain('EPERM') expect(calls).toHaveLength(2) }) + + it('frees every destination the run staged for, not only the one pnpm named', async () => { + // pnpm reports the first blocked destination its workers hit, but an + // update blocks on every package it has to replace. Freeing one per + // attempt cannot finish an update that replaces four of them. + const tree = { + [modules()]: [ + directory('dshmarket'), + directory('dshmarket_tmp_7408_13'), + directory('layout-base'), + directory('layout-base_tmp_7408_4'), + directory('cytoscape-fcose'), + directory('.pnpm'), + directory('@scope') + ], + [modules('@scope')]: [directory('inner'), directory('inner_tmp_7408_9')], + [modules('cytoscape-fcose', 'node_modules')]: [directory('cose-base_tmp_7408_7')] + } + + const targets = await blockedTargets(modules('argparse'), ROOT, { + listEntries: async (path) => tree[path] ?? [], + exists: () => true + }) + + // pnpm's own diagnosis leads; the rest come from the staging left behind, + // nested node_modules and scoped packages included. + expect(targets).toEqual([ + modules('argparse'), + modules('dshmarket'), + modules('layout-base'), + modules('cytoscape-fcose', 'node_modules', 'cose-base'), + modules('@scope', 'inner') + ]) + }) + + it('offers only destinations that are actually there', async () => { + const targets = await blockedTargets(undefined, ROOT, { + listEntries: async (path) => + path === modules() ? [directory('gone_tmp_1_1'), directory('kept_tmp_1_1')] : [], + exists: (path) => path.endsWith('kept') + }) + + expect(targets).toEqual([modules('kept')]) + }) + + it('does not mistake a sidelined copy for staging', async () => { + // `.dsh-old-` is this runner's own leftover. Deriving a target + // from it would name a package that was never being replaced. + const targets = await blockedTargets(undefined, ROOT, { + listEntries: async (path) => + path === modules() ? [directory(`cose-base${SIDELINE_MARKER}17`)] : [], + exists: () => true + }) + + expect(targets).toEqual([]) + }) }) + +const ROOT = join('/', 'p') + +/** A path under the fake profile's node_modules, in the host's own separators. */ +function modules(...segments) { + return join(ROOT, 'node_modules', ...segments) +} + +function directory(name) { + return { name, isDirectory: () => true, isSymbolicLink: () => false } +} diff --git a/test/profile-repair.test.ts b/test/profile-repair.test.ts index d0f13dcd..2d974aa2 100644 --- a/test/profile-repair.test.ts +++ b/test/profile-repair.test.ts @@ -99,6 +99,44 @@ describe('profile repair', () => { await expect(findDamagedPackageDirectories(home)).resolves.toEqual([]) }) + it('finds the leftovers inside a package’s own node_modules', async () => { + // Once the package being replaced is a dependency of a dependency, its + // staging lands under the dependent, not at the top level — and a sweep + // that stops at the top level leaves one copy behind per attempt. + const { home, nodeModules } = await profileHome() + const dependent = await materialize(nodeModules, 'cytoscape-fcose') + const nested = join(dependent, 'node_modules') + await materialize(nested, 'cose-base') + const sidelined = join(nested, 'cose-base.dsh-old-1787327060846') + await mkdir(sidelined, { recursive: true }) + const staging = join(nested, 'cose-base_tmp_7408_7') + await mkdir(staging, { recursive: true }) + + const damaged = await findDamagedPackageDirectories(home) + + expect(new Set(damaged)).toEqual(new Set([sidelined, staging])) + await expect(clearDamagedPackageDirectories(home)).resolves.toHaveLength(2) + expect(existsSync(sidelined)).toBe(false) + expect(existsSync(staging)).toBe(false) + expect(existsSync(join(nested, 'cose-base'))).toBe(true) + }) + + it('reports only the directories that are actually gone', async () => { + // Node's recursive `rm` resolves successfully without removing anything + // under a non-ASCII path, and a sweep that trusted it announced twelve + // cleared directories over a profile where all twelve were still there. + // Whatever the platform does, the count has to match the disk. + const { home, nodeModules } = await profileHome() + const staging = join(nodeModules, 'dshmarket_tmp_7408_13') + await mkdir(join(staging, 'lib'), { recursive: true }) + await writeFile(join(staging, 'lib', 'index.js'), '', 'utf8') + + const cleared = await clearDamagedPackageDirectories(home) + + expect(cleared).toEqual([staging]) + expect(existsSync(staging)).toBe(false) + }) + it('leaves a home without a profile alone', async () => { const home = await mkdtemp(join(tmpdir(), 'dsh-profile-repair-empty-')) homes.push(home) @@ -108,12 +146,17 @@ describe('profile repair', () => { }) it('restores the cleared packages through the profile’s own installer', () => { + // The repair runs with CI set, which is pnpm's cue to freeze the lockfile. + // A profile worth repairing is one where a failed `add` already wrote the + // new version into the lockfile while package.json still names the old, so + // freezing there fails on the divergence instead of healing it. expect(buildProfileInstallArguments('/app/dsh/bin.js')).toEqual([ '/app/dsh/bin.js', 'plugin', '--profile', 'web', - 'install' + 'install', + '--no-frozen-lockfile' ]) }) }) diff --git a/test/remove-tree.test.ts b/test/remove-tree.test.ts new file mode 100644 index 00000000..da1b4dae --- /dev/null +++ b/test/remove-tree.test.ts @@ -0,0 +1,99 @@ +import { existsSync } from 'node:fs' +import { mkdir, mkdtemp, readdir, rm, symlink, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterAll, describe, expect, it } from 'vitest' + +import { removeTree, removeTreeIfPossible } from '../src/main/state/remove-tree' + +const roots: string[] = [] + +async function scratch(name: string): Promise { + const root = await mkdtemp(join(tmpdir(), `dsh-remove-tree-${name}-`)) + roots.push(root) + return root +} + +/** A profile-shaped tree: nested directories, files, and an empty directory. */ +async function packageTree(root: string, name: string): Promise { + const pkg = join(root, name) + await mkdir(join(pkg, 'lib', 'types'), { recursive: true }) + await mkdir(join(pkg, 'empty'), { recursive: true }) + await writeFile(join(pkg, 'package.json'), '{"name":"x"}') + await writeFile(join(pkg, 'lib', 'index.js'), 'export default 1') + await writeFile(join(pkg, 'lib', 'types', 'index.d.ts'), 'export {}') + return pkg +} + +afterAll(async () => { + for (const root of roots) await rm(root, { recursive: true, force: true }).catch(() => undefined) +}) + +describe('removeTree', () => { + it('removes a package tree whose path is not ASCII', async () => { + // The reason this module exists. Node's recursive `rm` reports success and + // removes nothing under such a path on Windows, so every cleanup built on + // it silently stopped working for anyone whose account name is not ASCII. + const root = await scratch('unicode') + const home = join(root, '数据项素', 'node_modules') + await mkdir(home, { recursive: true }) + const pkg = await packageTree(home, 'cose-base') + + await removeTree(pkg) + + expect(existsSync(pkg)).toBe(false) + expect(await readdir(home)).toEqual([]) + }) + + it('removes an ASCII tree the same way', async () => { + const root = await scratch('ascii') + const pkg = await packageTree(root, 'layout-base') + + await removeTree(pkg) + + expect(existsSync(pkg)).toBe(false) + }) + + it('treats a missing path as already removed', async () => { + const root = await scratch('missing') + + await expect(removeTree(join(root, 'never-there'))).resolves.toBeUndefined() + }) + + it('detaches a symlink without following it', async () => { + // pnpm's layout links into a shared store. Following one would take the + // store's contents with it, which is a far worse outcome than a leftover. + const root = await scratch('symlink') + const store = await packageTree(root, 'store-copy') + const link = join(root, 'linked') + try { + await symlink(store, link, 'junction') + } catch { + return // an unprivileged Windows session cannot create links; nothing to assert + } + + await removeTree(link) + + expect(existsSync(link)).toBe(false) + expect(existsSync(join(store, 'package.json'))).toBe(true) + }) + + it('removes a single file', async () => { + const root = await scratch('file') + const file = join(root, 'pnpm-lock.yaml') + await writeFile(file, 'lockfileVersion: 9') + + await removeTree(file) + + expect(existsSync(file)).toBe(false) + }) + + it('reports whether the path is gone rather than throwing', async () => { + const root = await scratch('report') + const pkg = await packageTree(root, 'dshmarket') + + await expect(removeTreeIfPossible(pkg)).resolves.toBe(true) + await expect(removeTreeIfPossible(join(root, 'absent'))).resolves.toBe(true) + }) + +})