diff --git a/.github/workflows/action-contract.yml b/.github/workflows/action-contract.yml index ba30b4c..ddc82d6 100644 --- a/.github/workflows/action-contract.yml +++ b/.github/workflows/action-contract.yml @@ -10,7 +10,11 @@ on: branches: [main] paths: - 'action.yml' + - 'bin/ocr.js' + - 'bin/ocr.test.js' - 'package.json' + - 'scripts/platform.js' + - 'scripts/version.js' - 'scripts/github-actions/**' - 'examples/github_actions/README.md' - '.github/workflows/action-contract.yml' @@ -18,7 +22,11 @@ on: branches: [main] paths: - 'action.yml' + - 'bin/ocr.js' + - 'bin/ocr.test.js' - 'package.json' + - 'scripts/platform.js' + - 'scripts/version.js' - 'scripts/github-actions/**' - 'examples/github_actions/README.md' - '.github/workflows/action-contract.yml' @@ -48,3 +56,6 @@ jobs: - name: Test GitHub Actions contracts run: npm run test:github-actions + + - name: Test launcher contracts + run: npm run test:launcher diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 159a3f9..093c197 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -139,6 +139,9 @@ jobs: - name: Test run: go test -count=1 ./... + - name: Test Node launcher + run: npm run test:launcher + - name: Build run: go build -o opencodereview.exe ./cmd/opencodereview diff --git a/bin/ocr.js b/bin/ocr.js index 61c59f1..cc20496 100755 --- a/bin/ocr.js +++ b/bin/ocr.js @@ -5,7 +5,7 @@ "use strict"; -const { spawnSync, spawn } = require("child_process"); +const { spawn } = require("child_process"); const path = require("path"); const fs = require("fs"); const os = require("os"); @@ -25,7 +25,39 @@ function launcherExitCode(result) { return result.status ?? (result.error ? 1 : 0); } -module.exports = { launcherExitCode }; +// Returns a signal handler that forwards the signal to a child process. +function forwardSignal(child, signal) { + return () => { + child.kill(signal); + }; +} + +function installSignalHandlers(child, platform, signalTarget) { + const handlers = []; + function add(signal, handler) { + handlers.push([signal, handler]); + signalTarget.on(signal, handler); + } + + // Windows sends console Ctrl+C to both attached processes. Keep the wrapper + // alive so it can wait for the binary, but do not call child.kill("SIGINT"): + // Node maps that operation to an abrupt TerminateProcess call on Windows. + if (platform === "win32") { + add("SIGINT", () => {}); + } else { + for (const signal of ["SIGINT", "SIGTERM"]) { + add(signal, forwardSignal(child, signal)); + } + } + + return () => { + for (const [signal, handler] of handlers) { + signalTarget.removeListener(signal, handler); + } + }; +} + +module.exports = { launcherExitCode, forwardSignal, installSignalHandlers }; if (require.main !== module) { // Required as a module (tests); the launcher body below must not run. @@ -77,9 +109,33 @@ if (!process.env.OCR_NO_UPDATE) { } } -const result = spawnSync(binaryPath, process.argv.slice(2), { +const child = spawn(binaryPath, process.argv.slice(2), { stdio: "inherit", env: process.env, }); -process.exit(launcherExitCode(result)); +const removeSignalHandlers = installSignalHandlers( + child, + process.platform, + process +); + +let settled = false; +function settle(result) { + if (settled) return; + settled = true; + removeSignalHandlers(); + process.exitCode = launcherExitCode(result); +} + +// Spawn failures surface through "error", not "close" alone. Setting +// exitCode lets a piped diagnostic drain before the wrapper terminates. +child.on("error", (err) => { + console.error(`[ERROR] Failed to start ${binaryPath}: ${err.message}`); + settle({ error: err }); +}); + +// Wait for child to exit and propagate its exit status +child.on("close", (code, signal) => { + settle({ status: code, signal: signal }); +}); diff --git a/bin/ocr.test.js b/bin/ocr.test.js index e2eccbe..a8e23c1 100644 --- a/bin/ocr.test.js +++ b/bin/ocr.test.js @@ -5,8 +5,9 @@ const assert = require("assert"); const os = require("os"); +const { EventEmitter } = require("events"); -const { launcherExitCode } = require("./ocr"); +const { launcherExitCode, installSignalHandlers } = require("./ocr"); // Normal exits pass the child's status through unchanged. assert.strictEqual(launcherExitCode({ status: 0 }), 0); @@ -33,3 +34,227 @@ assert.strictEqual( ); console.log("ocr launcher exit-code tests passed"); + +// POSIX signals are relayed to the child and all installed handlers are +// removable once the child has settled. +{ + const signalTarget = new EventEmitter(); + const relayed = []; + const remove = installSignalHandlers( + { kill: (signal) => relayed.push(signal) }, + "linux", + signalTarget + ); + signalTarget.emit("SIGINT"); + signalTarget.emit("SIGTERM"); + assert.deepStrictEqual(relayed, ["SIGINT", "SIGTERM"]); + remove(); + assert.strictEqual(signalTarget.listenerCount("SIGINT"), 0); + assert.strictEqual(signalTarget.listenerCount("SIGTERM"), 0); +} + +// On Windows, console Ctrl+C already reaches both attached processes. The +// launcher keeps itself alive but must not turn SIGINT into TerminateProcess. +{ + const signalTarget = new EventEmitter(); + const relayed = []; + const remove = installSignalHandlers( + { kill: (signal) => relayed.push(signal) }, + "win32", + signalTarget + ); + signalTarget.emit("SIGINT"); + assert.deepStrictEqual(relayed, []); + assert.strictEqual(signalTarget.listenerCount("SIGTERM"), 0); + remove(); + assert.strictEqual(signalTarget.listenerCount("SIGINT"), 0); +} + +console.log("ocr launcher signal policy tests passed"); + +const { spawn } = require("child_process"); +const path = require("path"); +const fs = require("fs"); +const { + BINARY_FILENAME, + getPlatformPackageName, +} = require("../scripts/platform"); + +function createLauncherFixture() { + const root = fs.mkdtempSync(path.join(os.tmpdir(), "ocr-launcher-test-")); + const packageName = getPlatformPackageName(); + assert.ok(packageName, `unsupported test platform: ${process.platform}-${process.arch}`); + + for (const relativePath of [ + "bin/ocr.js", + "scripts/platform.js", + "scripts/version.js", + "package.json", + ]) { + const destination = path.join(root, relativePath); + fs.mkdirSync(path.dirname(destination), { recursive: true }); + fs.copyFileSync(path.join(__dirname, "..", relativePath), destination); + } + + const packageDir = path.join(root, "node_modules", ...packageName.split("/")); + const binaryPath = path.join(packageDir, "bin", BINARY_FILENAME); + fs.mkdirSync(path.dirname(binaryPath), { recursive: true }); + fs.writeFileSync( + path.join(packageDir, "package.json"), + JSON.stringify({ name: packageName, version: "0.0.0" }) + ); + + if (process.platform === "win32") { + fs.copyFileSync(process.execPath, binaryPath); + } else { + fs.symlinkSync(process.execPath, binaryPath); + } + + const home = path.join(root, "home"); + fs.mkdirSync(home); + return { + root, + home, + binaryPath, + launcher: path.join(root, "bin", "ocr.js"), + nodePath: path.join(root, "node_modules"), + }; +} + +function writeTarget(fixture, name, source) { + const target = path.join(fixture.root, name); + fs.writeFileSync(target, source); + return target; +} + +function runLauncher(fixture, args, signal, readyToken) { + return new Promise((resolve, reject) => { + const nodePath = process.env.NODE_PATH + ? `${fixture.nodePath}${path.delimiter}${process.env.NODE_PATH}` + : fixture.nodePath; + const launcher = spawn(process.execPath, [fixture.launcher, ...args], { + stdio: ["ignore", "pipe", "pipe"], + env: { + ...process.env, + HOME: fixture.home, + USERPROFILE: fixture.home, + NODE_PATH: nodePath, + OCR_NO_UPDATE: "1", + }, + }); + + let output = ""; + let signalSent = !signal; + let finished = false; + + function onData(chunk) { + output += chunk.toString(); + if (!signalSent && output.includes(readyToken)) { + signalSent = true; + launcher.kill(signal); + } + } + + launcher.stdout.on("data", onData); + launcher.stderr.on("data", onData); + + const guard = setTimeout(() => { + if (finished) return; + finished = true; + launcher.kill("SIGKILL"); + reject(new Error(`launcher test timed out; output: ${output}`)); + }, 8000); + + launcher.on("error", (err) => { + if (finished) return; + finished = true; + clearTimeout(guard); + reject(err); + }); + launcher.on("close", (code, receivedSignal) => { + if (finished) return; + finished = true; + clearTimeout(guard); + resolve({ code, signal: receivedSignal, output, signalSent }); + }); + }); +} + +async function testExitCodePropagation(fixture) { + const target = writeTarget(fixture, "exit-23.js", "process.exit(23);\n"); + const result = await runLauncher(fixture, [target]); + assert.strictEqual(result.code, 23, "launcher must propagate a normal exit code"); +} + +async function testGracefulSignalRelay(fixture, signal) { + const ready = `TARGET_READY_${signal}`; + const received = `TARGET_RECEIVED_${signal}`; + const target = writeTarget( + fixture, + `relay-${signal}.js`, + `process.on(${JSON.stringify(signal)}, () => {\n` + + ` console.log(${JSON.stringify(received)});\n` + + ` process.exit(7);\n` + + `});\n` + + `console.log(${JSON.stringify(ready)});\n` + + `setTimeout(() => process.exit(3), 5000);\n` + ); + const result = await runLauncher(fixture, [target], signal, ready); + + assert.ok(result.signalSent, `${signal}: target never reported ready`); + assert.ok( + result.output.includes(received), + `${signal}: launcher did not relay the signal; output: ${result.output}` + ); + assert.strictEqual(result.code, 7, `${signal}: child exit code was not propagated`); +} + +async function testSignalDeathExitCode(fixture) { + const ready = "TARGET_READY_FOR_SIGTERM"; + const target = writeTarget( + fixture, + "signal-death.js", + `console.log(${JSON.stringify(ready)});\n` + + `setTimeout(() => process.exit(3), 5000);\n` + ); + const result = await runLauncher(fixture, [target], "SIGTERM", ready); + + assert.strictEqual( + result.code, + 128 + os.constants.signals.SIGTERM, + "launcher must map child signal death to 128 + signo" + ); +} + +async function testSpawnFailure(fixture) { + fs.unlinkSync(fixture.binaryPath); + fs.mkdirSync(fixture.binaryPath); + const result = await runLauncher(fixture, []); + + assert.strictEqual(result.code, 1, `spawn failure should exit 1; ${result.output}`); + assert.ok( + result.output.includes("Failed to start"), + `spawn failure should be reported; output: ${result.output}` + ); +} + +(async () => { + const fixture = createLauncherFixture(); + try { + await testExitCodePropagation(fixture); + if (process.platform === "win32") { + console.log("skipping POSIX signal relay cases on win32"); + } else { + await testGracefulSignalRelay(fixture, "SIGINT"); + await testGracefulSignalRelay(fixture, "SIGTERM"); + await testSignalDeathExitCode(fixture); + } + await testSpawnFailure(fixture); + console.log("ocr launcher integration tests passed"); + } finally { + fs.rmSync(fixture.root, { recursive: true, force: true }); + } +})().catch((err) => { + console.error(err.stack || err.message); + process.exitCode = 1; +}); diff --git a/extensions/vscode/src/extension/services/CliService.ts b/extensions/vscode/src/extension/services/CliService.ts index 4813896..eab5836 100644 --- a/extensions/vscode/src/extension/services/CliService.ts +++ b/extensions/vscode/src/extension/services/CliService.ts @@ -115,6 +115,9 @@ export class CliService { const proc = spawn(resolveBin(this.cliPath), args, { cwd, env: envExtra ? { ...getShellEnv(), ...envExtra } : getShellEnv(), + // A dedicated POSIX process group lets cancellation escalate without + // leaving the native CLI or any of its subprocesses behind. + detached: process.platform !== 'win32', }); this.current = proc; let stdout = ''; @@ -160,10 +163,34 @@ export class CliService { cancel(): void { if (this.current && this.current.pid) { - this.current.kill('SIGTERM'); const proc = this.current; + + // Windows does not deliver POSIX signals to the launcher. Node maps + // kill('SIGTERM') to TerminateProcess, which kills only the launcher and + // can orphan its native child. taskkill /T terminates the complete tree. + if (process.platform === 'win32') { + const treeKill = spawn('taskkill', ['/pid', String(proc.pid), '/t', '/f'], { + stdio: 'ignore', + windowsHide: true, + }); + const fallback = () => { + if (proc.exitCode === null && proc.signalCode === null) proc.kill('SIGKILL'); + }; + treeKill.once('error', fallback); + treeKill.once('close', (code) => { + if (code !== 0) fallback(); + }); + return; + } + + proc.kill('SIGTERM'); const forceKillTimer = setTimeout(() => { - if (proc.exitCode === null && proc.signalCode === null) proc.kill('SIGKILL'); + if (proc.exitCode !== null || proc.signalCode !== null) return; + try { + process.kill(-proc.pid!, 'SIGKILL'); + } catch (err) { + if ((err as NodeJS.ErrnoException).code !== 'ESRCH') proc.kill('SIGKILL'); + } }, 3000); proc.once('close', () => clearTimeout(forceKillTimer)); } diff --git a/extensions/vscode/src/extension/services/__tests__/CliService.cancel.test.ts b/extensions/vscode/src/extension/services/__tests__/CliService.cancel.test.ts index a29aa41..38ac072 100644 --- a/extensions/vscode/src/extension/services/__tests__/CliService.cancel.test.ts +++ b/extensions/vscode/src/extension/services/__tests__/CliService.cancel.test.ts @@ -13,6 +13,11 @@ jest.mock('child_process', () => ({ })); const mockedSpawn = jest.mocked(spawn); +const platformDescriptor = Object.getOwnPropertyDescriptor(process, 'platform'); + +function setPlatform(platform: NodeJS.Platform): void { + Object.defineProperty(process, 'platform', { value: platform }); +} function createMockProcess() { const proc = Object.assign(new EventEmitter(), { @@ -33,9 +38,15 @@ function createMockProcess() { } describe('CliService.cancel', () => { + beforeEach(() => { + jest.spyOn(process, 'kill').mockImplementation(() => true); + }); + afterEach(() => { jest.useRealTimers(); mockedSpawn.mockReset(); + jest.restoreAllMocks(); + if (platformDescriptor) Object.defineProperty(process, 'platform', platformDescriptor); }); it('子进程忽略 SIGTERM 时,超时后发送 SIGKILL', () => { @@ -48,8 +59,15 @@ describe('CliService.cancel', () => { svc.cancel(); jest.advanceTimersByTime(3000); - expect(proc.kill).toHaveBeenNthCalledWith(1, 'SIGTERM'); - expect(proc.kill).toHaveBeenNthCalledWith(2, 'SIGKILL'); + expect(mockedSpawn).toHaveBeenNthCalledWith( + 1, + 'node', + [], + expect.objectContaining({ detached: true }), + ); + expect(proc.kill).toHaveBeenCalledTimes(1); + expect(proc.kill).toHaveBeenCalledWith('SIGTERM'); + expect(process.kill).toHaveBeenCalledWith(-123, 'SIGKILL'); }); it('子进程已正常退出时,不再发送 SIGKILL', async () => { @@ -67,5 +85,83 @@ describe('CliService.cancel', () => { expect(proc.kill).toHaveBeenCalledTimes(1); expect(proc.kill).toHaveBeenCalledWith('SIGTERM'); + expect(process.kill).not.toHaveBeenCalled(); + }); + + it('falls back to the launcher PID when the POSIX group kill fails', () => { + jest.useFakeTimers(); + jest.mocked(process.kill).mockImplementation(() => { + throw Object.assign(new Error('not permitted'), { code: 'EPERM' }); + }); + const proc = createMockProcess(); + mockedSpawn.mockReturnValue(proc as unknown as ReturnType); + + const svc = new CliService('node'); + void svc.runRaw([], '.', () => {}); + svc.cancel(); + jest.advanceTimersByTime(3000); + + expect(process.kill).toHaveBeenCalledWith(-123, 'SIGKILL'); + expect(proc.kill).toHaveBeenNthCalledWith(1, 'SIGTERM'); + expect(proc.kill).toHaveBeenNthCalledWith(2, 'SIGKILL'); + }); + + it('kills the complete process tree on Windows', () => { + setPlatform('win32'); + const proc = createMockProcess(); + const treeKill = createMockProcess(); + mockedSpawn + .mockReturnValueOnce(proc as unknown as ReturnType) + .mockReturnValueOnce(treeKill as unknown as ReturnType); + + const svc = new CliService('node'); + void svc.runRaw([], '.', () => {}); + svc.cancel(); + + expect(mockedSpawn).toHaveBeenNthCalledWith( + 1, + 'node', + [], + expect.objectContaining({ detached: false }), + ); + expect(mockedSpawn).toHaveBeenNthCalledWith( + 2, + 'taskkill', + ['/pid', '123', '/t', '/f'], + { stdio: 'ignore', windowsHide: true }, + ); + expect(proc.kill).not.toHaveBeenCalled(); + }); + + it('falls back when taskkill cannot be started on Windows', () => { + setPlatform('win32'); + const proc = createMockProcess(); + const treeKill = createMockProcess(); + mockedSpawn + .mockReturnValueOnce(proc as unknown as ReturnType) + .mockReturnValueOnce(treeKill as unknown as ReturnType); + + const svc = new CliService('node'); + void svc.runRaw([], '.', () => {}); + svc.cancel(); + treeKill.emit('error', new Error('taskkill unavailable')); + + expect(proc.kill).toHaveBeenCalledWith('SIGKILL'); + }); + + it('falls back when taskkill exits unsuccessfully on Windows', () => { + setPlatform('win32'); + const proc = createMockProcess(); + const treeKill = createMockProcess(); + mockedSpawn + .mockReturnValueOnce(proc as unknown as ReturnType) + .mockReturnValueOnce(treeKill as unknown as ReturnType); + + const svc = new CliService('node'); + void svc.runRaw([], '.', () => {}); + svc.cancel(); + treeKill.emit('close', 1); + + expect(proc.kill).toHaveBeenCalledWith('SIGKILL'); }); });