diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2bd82c02..2021aba3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -6,6 +6,9 @@ on: pull_request: workflow_dispatch: +permissions: + contents: read + jobs: check: strategy: @@ -121,3 +124,46 @@ jobs: env: CODSPEED_SKIP_UPLOAD: true CODSPEED_DEBUG: true + + # Bazel's rules_js runs node by absolute path, so the runner's `node` wrapper + # on PATH never injects the V8 flags and the plugin must restart itself. + restart-without-node-wrapper: + runs-on: "ubuntu-latest" + name: "restart without node wrapper (${{ matrix.mode }})" + strategy: + matrix: + mode: [simulation, walltime, memory] + fail-fast: false + steps: + - uses: "actions/checkout@v4" + with: + fetch-depth: 0 + submodules: true + - uses: pnpm/action-setup@v4 + - uses: actions/setup-node@v6 + with: + cache: pnpm + node-version: 24 + - name: Restore turbo cache + uses: ./.github/actions/turbo-cache + with: + key-suffix: node24 + - run: pnpm install --frozen-lockfile --prefer-offline + - run: pnpm turbo run build + # Resolved before the action prefixes PATH with its wrapper. + - run: echo "REAL_NODE=$(command -v node)" >> "$GITHUB_ENV" + - uses: CodSpeedHQ/action@main + with: + mode: ${{ matrix.mode }} + working-directory: examples/with-javascript-esm + run: | + set -o pipefail + "$REAL_NODE" tinybench.js 2>&1 | tee tinybench.log + "$REAL_NODE" benchmark-js.js 2>&1 | tee benchmark-js.log + env: + CODSPEED_SKIP_UPLOAD: true + CODSPEED_DEBUG: true + - name: Check the benchmarks ran with the V8 flags + working-directory: examples/with-javascript-esm + run: | + ! grep -E "missing required flags|failed to restart node" tinybench.log benchmark-js.log diff --git a/packages/benchmark.js-plugin/src/index.ts b/packages/benchmark.js-plugin/src/index.ts index fa69a35a..dcbaf158 100644 --- a/packages/benchmark.js-plugin/src/index.ts +++ b/packages/benchmark.js-plugin/src/index.ts @@ -1,4 +1,5 @@ import { + ensureV8Flags, getCallingFile, InstrumentHooks, mongoMeasurement, @@ -19,6 +20,8 @@ import { CodSpeedBenchmark } from "./types"; declare const __VERSION__: string; tryIntrospect(); +// Must run before any benchmark state exists: it may replace this process. +ensureV8Flags(); interface WithCodSpeedBenchmark extends Omit< Benchmark, diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 8dc74641..e281405e 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -52,7 +52,7 @@ export type { SetupInstrumentsRequestBody, SetupInstrumentsResponse, } from "./generated/openapi"; -export { getV8Flags, tryIntrospect } from "./introspection"; +export { ensureV8Flags, getV8Flags, tryIntrospect } from "./introspection"; export { getUnsupportedNodeVersionWarning, SUPPORTED_NODE_MAJORS, diff --git a/packages/core/src/introspection.ts b/packages/core/src/introspection.ts index 8906c1d9..979fd9af 100644 --- a/packages/core/src/introspection.ts +++ b/packages/core/src/introspection.ts @@ -1,11 +1,14 @@ -import { writeFileSync } from "fs"; +import { readlinkSync, writeFileSync } from "fs"; import path from "path"; +import { isMainThread } from "worker_threads"; -import { getInstrumentMode } from "./runnerMode"; +import native_core from "./native_core"; +import { getCodspeedRunnerMode, getInstrumentMode } from "./runnerMode"; const CUSTOM_INTROSPECTION_EXIT_CODE = 0; const V8_LOG_FILENAME_PATTERN = "codspeed-v8-%p.log"; +const RESTARTED_PID_ENV = "__CODSPEED_NODE_CORE_RESTARTED_PID__"; // Fixed seeds so Math.random() and hash table layout match on every run const getSeedFlags = (nodeVersionMajor: number) => { @@ -91,6 +94,96 @@ export const tryIntrospect = () => { } }; +const EVAL_FLAGS = ["-e", "--eval", "-p", "--print"]; + +// `node -` or `node` without a script reads the script from stdin (or opens the +// REPL). The input is consumed by then, so the restarted process would get none. +const readsScriptFromStdin = () => { + const scriptPath = process.argv[1]; + if (scriptPath === "-") { + return true; + } + if (scriptPath !== undefined) { + return false; + } + return !process.execArgv.some((arg) => + EVAL_FLAGS.some((flag) => arg === flag || arg.startsWith(`${flag}=`)), + ); +}; + +/** + * Restarts the process with the V8 flags the runner mode requires when the + * launcher did not pass them (e.g. Bazel executes node by absolute path, so the + * runner's `node` wrapper never runs). Must be called before instrument-hooks + * is used: the restart replaces this process. + * + * Cases that cannot restart safely, including a failed exec, run without the + * flags; `checkV8Flags` warns once instrument-hooks is set up. + */ +export const ensureV8Flags = () => { + if (getCodspeedRunnerMode() === "disabled" || !native_core.isBound) { + return; + } + + const missingFlags = getV8Flags().filter( + (flag) => !process.execArgv.includes(flag), + ); + if (missingFlags.length === 0) { + return; + } + + // Worker threads and IPC children cannot replace their process, and an IPC + // channel would be lost. + if (!isMainThread || process.send !== undefined) { + return; + } + + if (readsScriptFromStdin()) { + return; + } + + // The restart did not help (its launcher stripped the flags again): do not + // restart in a loop. + if (process.env[RESTARTED_PID_ENV] === String(process.pid)) { + return; + } + + const getExecutable = () => { + switch (process.platform) { + case "linux": + // rules_js replaces process.execPath with its shell shim. + return readlinkSync("/proc/self/exe"); + case "darwin": + return process.execPath; + default: + return undefined; + } + }; + + try { + const executable = getExecutable(); + if (executable === undefined) { + return; + } + + process.env[RESTARTED_PID_ENV] = String(process.pid); + // The runner's wrapper passes its flags before the user's, so user flags win. + native_core.restart(executable, [ + executable, + ...missingFlags, + ...process.execArgv, + ...process.argv.slice(1), + ]); + } catch (error) { + // No /proc, or exec failed: the current image is still usable, so measure + // without the flags and let checkV8Flags warn. + delete process.env[RESTARTED_PID_ENV]; + console.warn( + `[CodSpeed] failed to restart node with the required V8 flags: ${String(error)}`, + ); + } +}; + export const checkV8Flags = () => { const requiredFlags = getV8Flags(); const actualFlags = process.execArgv; diff --git a/packages/core/src/native_core/index.ts b/packages/core/src/native_core/index.ts index 4e64f654..8c5e5c3b 100644 --- a/packages/core/src/native_core/index.ts +++ b/packages/core/src/native_core/index.ts @@ -5,6 +5,7 @@ import { LinuxPerf } from "./linux_perf/linux_perf"; interface NativeCore { InstrumentHooks: InstrumentHooks; LinuxPerf: typeof LinuxPerf; + restart(executable: string, args: string[]): never; } interface NativeCoreWithBindingStatus extends NativeCore { @@ -26,6 +27,11 @@ try { logDebug("Failed to bind native core, instruments will not work."); logDebug(e); native_core = { + restart: () => { + throw new Error( + `[CodSpeed] Cannot restart Node.js because the native core module is not bound: ${String(e)}`, + ); + }, LinuxPerf: class LinuxPerf { start() { return false; diff --git a/packages/core/src/native_core/instruments/hooks_wrapper.cc b/packages/core/src/native_core/instruments/hooks_wrapper.cc index c616e61a..2133886c 100644 --- a/packages/core/src/native_core/instruments/hooks_wrapper.cc +++ b/packages/core/src/native_core/instruments/hooks_wrapper.cc @@ -6,32 +6,35 @@ namespace codspeed_native { namespace instruments { namespace hooks_wrapper { -static InstrumentHooks *hooks = nullptr; +namespace { -void InitializeGlobal() { - if (!hooks) { - hooks = instrument_hooks_init(); - } +// Created on first use: init opens the runner FIFO, so importing the module must +// not trigger it. +InstrumentHooks *GetHooks() { + static InstrumentHooks *hooks = instrument_hooks_init(); + return hooks; } +} // namespace + Napi::Boolean IsInstrumented(const Napi::CallbackInfo &info) { Napi::Env env = info.Env(); - bool instrumented = instrument_hooks_is_instrumented(hooks); + bool instrumented = instrument_hooks_is_instrumented(GetHooks()); return Napi::Boolean::New(env, instrumented); } Napi::Number StartBenchmark(const Napi::CallbackInfo &info) { Napi::Env env = info.Env(); - uint8_t result = instrument_hooks_start_benchmark(hooks); + uint8_t result = instrument_hooks_start_benchmark(GetHooks()); return Napi::Number::New(env, result); } Napi::Number StopBenchmark(const Napi::CallbackInfo &info) { Napi::Env env = info.Env(); - uint8_t result = instrument_hooks_stop_benchmark(hooks); + uint8_t result = instrument_hooks_stop_benchmark(GetHooks()); return Napi::Number::New(env, result); } @@ -54,7 +57,7 @@ Napi::Number SetExecutedBenchmark(const Napi::CallbackInfo &info) { std::string uri = info[1].As().Utf8Value(); uint8_t result = - instrument_hooks_set_executed_benchmark(hooks, pid, uri.c_str()); + instrument_hooks_set_executed_benchmark(GetHooks(), pid, uri.c_str()); return Napi::Number::New(env, result); } @@ -76,8 +79,8 @@ Napi::Number SetIntegration(const Napi::CallbackInfo &info) { std::string name = info[0].As().Utf8Value(); std::string version = info[1].As().Utf8Value(); - uint8_t result = - instrument_hooks_set_integration(hooks, name.c_str(), version.c_str()); + uint8_t result = instrument_hooks_set_integration( + GetHooks(), name.c_str(), version.c_str()); return Napi::Number::New(env, result); } @@ -104,7 +107,7 @@ Napi::Number SetEnvironment(const Napi::CallbackInfo &info) { std::string value = info[2].As().Utf8Value(); uint8_t result = instrument_hooks_set_environment( - hooks, section_name.c_str(), key.c_str(), value.c_str()); + GetHooks(), section_name.c_str(), key.c_str(), value.c_str()); return Napi::Number::New(env, result); } @@ -125,7 +128,7 @@ Napi::Number WriteEnvironment(const Napi::CallbackInfo &info) { int32_t pid = info[0].As().Int32Value(); - uint8_t result = instrument_hooks_write_environment(hooks, pid); + uint8_t result = instrument_hooks_write_environment(GetHooks(), pid); return Napi::Number::New(env, result); } @@ -193,7 +196,7 @@ Napi::Number AddMarker(const Napi::CallbackInfo &info) { } uint8_t result = - instrument_hooks_add_marker(hooks, pid, marker_type, timestamp); + instrument_hooks_add_marker(GetHooks(), pid, marker_type, timestamp); return Napi::Number::New(env, result); } @@ -221,8 +224,6 @@ Napi::Value __attribute__ ((noinline)) __codspeed_root_frame__(const Napi::Callb Napi::Object Initialize(Napi::Env env, Napi::Object exports) { Napi::Object instrumentHooksObj = Napi::Object::New(env); - InitializeGlobal(); - instrumentHooksObj.Set(Napi::String::New(env, "isInstrumented"), Napi::Function::New(env, IsInstrumented)); instrumentHooksObj.Set(Napi::String::New(env, "startBenchmark"), diff --git a/packages/core/src/native_core/instruments/hooks_wrapper.h b/packages/core/src/native_core/instruments/hooks_wrapper.h index d5c92daf..dd62e9a8 100644 --- a/packages/core/src/native_core/instruments/hooks_wrapper.h +++ b/packages/core/src/native_core/instruments/hooks_wrapper.h @@ -7,11 +7,6 @@ namespace codspeed_native { namespace instruments { namespace hooks_wrapper { -// Global initialization -// WARNING: InitializeGlobal() must be called before using any other functions -// All API functions assume the global instance is already initialized -void InitializeGlobal(); - Napi::Boolean IsInstrumented(const Napi::CallbackInfo &info); Napi::Number StartBenchmark(const Napi::CallbackInfo &info); Napi::Number StopBenchmark(const Napi::CallbackInfo &info); diff --git a/packages/core/src/native_core/native_core.cc b/packages/core/src/native_core/native_core.cc index aa1e5c71..69010178 100644 --- a/packages/core/src/native_core/native_core.cc +++ b/packages/core/src/native_core/native_core.cc @@ -2,11 +2,143 @@ #include "instruments/hooks_wrapper.h" #include +#include +#include +#include +#include + +#if !defined(_WIN32) +#include +#include +#endif + namespace codspeed_native { +namespace { + +bool ContainsNulByte(const std::string &value) { + return value.find('\0') != std::string::npos; +} + +#if !defined(_WIN32) +// `execv` closes descriptors marked close-on-exec, and libuv marks the stdio +// descriptors that way. Clear the flag like Node's own `process.execve` does; +// otherwise the restarted process writes to /dev/null. +int PersistStandardStream(int fd) { + const int flags = fcntl(fd, F_GETFD, 0); + if (flags < 0) { + return flags; + } + return fcntl(fd, F_SETFD, flags & ~FD_CLOEXEC); +} +#endif + +// Replaces the process image via `execv`, keeping the PID, environment and open +// descriptors. Throws on failure; the current image stays usable. +Napi::Value Restart(const Napi::CallbackInfo &info) { + Napi::Env env = info.Env(); + + if (info.Length() != 2) { + Napi::TypeError::New(env, "Expected 2 arguments: executable and args") + .ThrowAsJavaScriptException(); + return env.Undefined(); + } + + if (!info[0].IsString()) { + Napi::TypeError::New(env, "Expected string (executable)") + .ThrowAsJavaScriptException(); + return env.Undefined(); + } + + if (!info[1].IsArray()) { + Napi::TypeError::New(env, "Expected array of strings (args)") + .ThrowAsJavaScriptException(); + return env.Undefined(); + } + + const std::string executable = info[0].As().Utf8Value(); + if (executable.empty()) { + Napi::TypeError::New(env, "Expected non-empty string (executable)") + .ThrowAsJavaScriptException(); + return env.Undefined(); + } + if (ContainsNulByte(executable)) { + Napi::TypeError::New(env, "executable must not contain NUL bytes") + .ThrowAsJavaScriptException(); + return env.Undefined(); + } + + const Napi::Array args = info[1].As(); + const uint32_t argc = args.Length(); + if (argc == 0) { + Napi::TypeError::New(env, "Expected at least argv[0] in args: pass the " + "complete argv, including the program name") + .ThrowAsJavaScriptException(); + return env.Undefined(); + } + + // Fill the strings before taking any pointer out of them: `execv` needs + // stable storage, so the vector must not reallocate afterwards. + std::vector arg_strings; + arg_strings.reserve(argc); + for (uint32_t i = 0; i < argc; i++) { + const Napi::Value arg = args.Get(i); + if (!arg.IsString()) { + Napi::TypeError::New(env, "Expected string (args[" + std::to_string(i) + + "])") + .ThrowAsJavaScriptException(); + return env.Undefined(); + } + arg_strings.push_back(arg.As().Utf8Value()); + if (ContainsNulByte(arg_strings.back())) { + Napi::TypeError::New(env, "args[" + std::to_string(i) + + "] must not contain NUL bytes") + .ThrowAsJavaScriptException(); + return env.Undefined(); + } + } + +#if defined(_WIN32) + Napi::Error::New(env, "restart() is not supported on Windows: pass the " + "required Node.js flags from the launcher") + .ThrowAsJavaScriptException(); + return env.Undefined(); +#else + std::vector argv; + argv.reserve(arg_strings.size() + 1); + for (std::string &arg : arg_strings) { + argv.push_back(const_cast(arg.c_str())); + } + argv.push_back(nullptr); + + if (PersistStandardStream(0) < 0 || PersistStandardStream(1) < 0 || + PersistStandardStream(2) < 0) { + const int fcntl_errno = errno; + Napi::Error::New(env, "Failed to keep stdio open across restart: " + + std::string(std::strerror(fcntl_errno))) + .ThrowAsJavaScriptException(); + return env.Undefined(); + } + + execv(executable.c_str(), argv.data()); + + // Reaching this point means the exec failed and the current process is still + // running. Capture errno before any further N-API call can clobber it. + const int exec_errno = errno; + Napi::Error::New(env, "Failed to restart with execv(\"" + executable + + "\"): " + std::strerror(exec_errno)) + .ThrowAsJavaScriptException(); + return env.Undefined(); +#endif +} + +} // namespace + Napi::Object Initialize(Napi::Env env, Napi::Object exports) { codspeed_native::LinuxPerf::Initialize(env, exports); codspeed_native::instruments::hooks_wrapper::Initialize(env, exports); + exports.Set(Napi::String::New(env, "restart"), + Napi::Function::New(env, Restart)); return exports; } diff --git a/packages/core/tests/ensureV8Flags.integ.test.ts b/packages/core/tests/ensureV8Flags.integ.test.ts new file mode 100644 index 00000000..208758f9 --- /dev/null +++ b/packages/core/tests/ensureV8Flags.integ.test.ts @@ -0,0 +1,147 @@ +/* eslint-disable @typescript-eslint/no-require-imports */ +import { spawnSync } from "child_process"; +import fs from "fs"; +import os from "os"; +import path from "path"; + +export {}; // Make this a module + +enum ScriptEntry { + File, + Stdin, + Eval, +} + +interface BenchmarkProcess { + spawnedPid: number | undefined; + pid: number; + execArgv: string[]; + stderr: string; +} + +const runBenchmarkProcess = ( + env: NodeJS.ProcessEnv, + nodeArgs: string[] = [], + prelude = "", + entry = ScriptEntry.File, +): BenchmarkProcess => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "codspeed-ensure-v8-")); + const script = path.join(dir, "bench.js"); + fs.writeFileSync( + script, + `${prelude} +const core = require(${JSON.stringify(require.resolve(".."))}); +core.ensureV8Flags(); +process.stdout.write( + JSON.stringify({ pid: process.pid, execArgv: process.execArgv }), +); +`, + ); + try { + const baseEnv = { ...process.env }; + delete baseEnv.CODSPEED_ENV; + delete baseEnv.CODSPEED_RUNNER_MODE; + const entryArgs = { + [ScriptEntry.File]: [script], + [ScriptEntry.Stdin]: ["-"], + [ScriptEntry.Eval]: ["-e", `require(${JSON.stringify(script)})`], + }[entry]; + const result = spawnSync(process.execPath, [...nodeArgs, ...entryArgs], { + env: { ...baseEnv, ...env }, + encoding: "utf8", + input: entry === ScriptEntry.Stdin ? fs.readFileSync(script) : undefined, + }); + if (result.status !== 0) { + throw new Error(`child failed: ${result.stderr}`); + } + return { + spawnedPid: result.pid, + stderr: result.stderr, + ...(JSON.parse(result.stdout) as { pid: number; execArgv: string[] }), + }; + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } +}; + +describe("ensureV8Flags", () => { + it("restarts the process with the required flags when they are missing", () => { + const { spawnedPid, pid, execArgv } = runBenchmarkProcess({ + CODSPEED_ENV: "runner", + CODSPEED_RUNNER_MODE: "instrumentation", + }); + + // Same pid: the process was replaced, not started as a child. + expect(pid).toBe(spawnedPid); + expect(execArgv.filter((flag) => flag === "--no-opt")).toHaveLength(1); + expect(execArgv).toContain("--predictable"); + }); + + it("restarts a process started with -e", () => { + const { spawnedPid, pid, execArgv } = runBenchmarkProcess( + { CODSPEED_ENV: "runner", CODSPEED_RUNNER_MODE: "instrumentation" }, + [], + "", + ScriptEntry.Eval, + ); + + expect(pid).toBe(spawnedPid); + expect(execArgv).toContain("--no-opt"); + }); + + it("keeps a process that read its script from stdin", () => { + // Parsing the output proves the script ran instead of an empty restart. + const { spawnedPid, pid, execArgv } = runBenchmarkProcess( + { CODSPEED_ENV: "runner", CODSPEED_RUNNER_MODE: "instrumentation" }, + [], + "", + ScriptEntry.Stdin, + ); + + expect(pid).toBe(spawnedPid); + expect(execArgv).not.toContain("--no-opt"); + }); + + it("adds the flags before the user's so user flags win", () => { + const { execArgv } = runBenchmarkProcess( + { CODSPEED_ENV: "runner", CODSPEED_RUNNER_MODE: "instrumentation" }, + ["--max-old-space-size=8192"], + ); + + expect(execArgv.indexOf("--max-old-space-size=4096")).toBeLessThan( + execArgv.indexOf("--max-old-space-size=8192"), + ); + }); + + it("warns and continues when the restart fails", () => { + // Unlinking the running executable makes /proc/self/exe point at a path + // that no longer exists, so execv fails with ENOENT. + const { execArgv, stderr } = runBenchmarkProcess( + { CODSPEED_ENV: "runner", CODSPEED_RUNNER_MODE: "instrumentation" }, + [], + `const fs = require("fs"), path = require("path"); +const copy = path.join(__dirname, "node-copy"); +if (!process.env.__NODE_COPY_RUNNING__) { + fs.copyFileSync(process.execPath, copy); + fs.chmodSync(copy, 0o755); + const r = require("child_process").spawnSync(copy, process.argv.slice(1), { + stdio: "inherit", + env: { ...process.env, __NODE_COPY_RUNNING__: "1" }, + }); + process.exit(r.status ?? 1); +} +fs.unlinkSync(copy);`, + ); + + expect(execArgv).not.toContain("--no-opt"); + expect(stderr).toContain("failed to restart node"); + }); + + it("keeps the process when CodSpeed is disabled", () => { + const { execArgv } = runBenchmarkProcess({ + CODSPEED_RUNNER_MODE: "instrumentation", + }); + + expect(execArgv).not.toContain("--no-opt"); + }); +}); diff --git a/packages/tinybench-plugin/src/index.ts b/packages/tinybench-plugin/src/index.ts index 298f97b9..7855a783 100644 --- a/packages/tinybench-plugin/src/index.ts +++ b/packages/tinybench-plugin/src/index.ts @@ -1,4 +1,5 @@ import { + ensureV8Flags, getCallingFile, getCodspeedRunnerMode, getInstrumentMode, @@ -16,6 +17,8 @@ import { getOrCreateUriMap } from "./uri"; import { setupCodspeedWalltimeBench } from "./walltime"; tryIntrospect(); +// Must run before any benchmark state exists: it may replace this process. +ensureV8Flags(); export function withCodSpeed(bench: Bench): Bench { const codspeedRunnerMode = getCodspeedRunnerMode();