merged PR #5423: fix(natives): distinguish process-stale from disk-stale sentinel mismatch

2026-07-16 03:32:03 +02:00
parent 217678bfd8 e4a10450ec
commit 29b6649ff9
4 changed files with 109 additions and 1 deletions
@@ -59,6 +59,7 @@

 - Fixed the native build script failing to locate the `@napi-rs/cli` `napi` binary on Windows because the `PATH` lookup joined entries with a Unix `:` separator instead of the platform delimiter (`path.delimiter`).
 - Fixed a Windows regression where an abnormal `omp` exit or bash cancellation could `TerminateProcess` unrelated `pwsh.exe` / `powershell.exe` sessions (including other Cursor terminal tabs). `SpawnRegistry` stored only the raw pid of each brush-spawned child and re-opened it via `Process::from_pid` at cancellation time; between those two moments Windows could recycle a freed pid onto an unrelated PowerShell, and `signal_tree` then walked the wrong subtree via Toolhelp. The observer now pins a stable `Process` handle at spawn time — on Windows the open handle keeps the pid slot reserved, on Linux the pidfd carries identity, on macOS the `(pid, start_time)` triple detects impersonation — so cancellation can only reach children this run actually launched. The registry sweeps exited entries once the recorded set crosses a small threshold so a long bash loop of short external commands cannot pin one owned OS handle per historical spawn. ([#4605](https://github.com/can1357/oh-my-pi/issues/4605))
+- Fixed the pi-natives version sentinel emitting "reinstall to re-sync" when a long-lived process survives an in-place upgrade: the loader now detects that the resident addon exposes a *prior* release's sentinel and reports "omp was upgraded while this session was running — restart to pick up the new version (disk is already consistent)" instead of misdiagnosing it as a stale on-disk file ([#4812](https://github.com/can1357/oh-my-pi/issues/4812)).

 ## [16.3.6] - 2026-07-04

@@ -86,4 +86,16 @@ export interface SelectCpuVariantResult {

 export function selectCpuVariant(input: SelectCpuVariantInput): SelectCpuVariantResult;

+export interface ValidateLoadedBindingsContext {
+	isWorkspaceLoad: boolean;
+	packageVersion: string;
+	versionSentinelExport: string;
+}
+
+export function validateLoadedBindings(
+	ctx: ValidateLoadedBindingsContext,
+	bindings: Record<string, unknown>,
+	candidate: string,
+): void;
+
 export function loadNative(): Record<string, unknown>;
@@ -583,7 +583,7 @@ function maybeStageNodeModulesAddon(ctx, errors) {
 	return stagedPath;
 }

-function validateLoadedBindings(ctx, bindings, candidate) {
+export function validateLoadedBindings(ctx, bindings, candidate) {
 	// In workspace dev (running out of `packages/natives/native/` rather than a
 	// `node_modules` install or a compiled bundle) the local `.node` only gains
 	// the renamed sentinel after `bun --cwd=packages/natives run build`. Skip
@@ -591,6 +591,30 @@ function validateLoadedBindings(ctx, bindings, candidate) {
 	// completes; install and compiled-binary paths still validate.
 	if (ctx.isWorkspaceLoad) return;
 	if (typeof bindings[ctx.versionSentinelExport] === "function") return;
+
+	// The expected sentinel is missing. Distinguish two failure modes by the
+	// sentinel the bindings DO carry:
+	//   - disk stale: the `.node` on disk predates this loader (its own build);
+	//     reinstalling re-syncs the file.
+	//   - process stale: an in-place upgrade landed a new release on disk while
+	//     this process still holds the previous addon generation resident in the
+	//     dynamic-loader's native-module cache. `require` returns those old
+	//     exports, which carry the PRIOR sentinel — disk is already consistent,
+	//     so reinstall is a no-op and only restarting the process re-syncs.
+	const residentSentinel = Object.keys(bindings).find(
+		key => key !== ctx.versionSentinelExport && /^__piNativesV[A-Za-z0-9_]+$/.test(key),
+	);
+	if (residentSentinel) {
+		const residentVersion = residentSentinel.slice("__piNativesV".length).replace(/_/g, ".");
+		throw new Error(
+			`Loaded ${candidate}, which exposes the @oh-my-pi/pi-natives@${residentVersion} version ` +
+				`sentinel \`${residentSentinel}\` but not the @${ctx.packageVersion} sentinel ` +
+				`\`${ctx.versionSentinelExport}\` this loader expects. omp was upgraded to ` +
+				`${ctx.packageVersion} while this session was running; the ${residentVersion} addon is ` +
+				"still resident in this process. Disk is already consistent — restart omp to pick up " +
+				`${ctx.packageVersion} (reinstalling changes nothing).`,
+		);
+	}
 	throw new Error(
 		`Loaded ${candidate} but it does not expose the @oh-my-pi/pi-natives@${ctx.packageVersion} ` +
 			`version sentinel \`${ctx.versionSentinelExport}\`. The .node file on disk is from a different ` +
@@ -0,0 +1,71 @@
+/**
+ * Repro for https://github.com/can1357/oh-my-pi/issues/4812
+ *
+ * A long-lived omp session that survives an in-place `bun install -g` upgrade
+ * keeps the previous pi-natives NAPI addon resident in the process. A tab
+ * worker spawned afterwards runs the freshly-installed JS loader, which expects
+ * the new sentinel (e.g. `__piNativesV16_3_11`), but `require` returns the
+ * resident old exports carrying the PRIOR sentinel (`__piNativesV16_3_10`).
+ *
+ * The contract this test pins down: `validateLoadedBindings` distinguishes a
+ * process-stale mix (disk consistent — restart to re-sync) from a genuinely
+ * disk-stale addon (reinstall to re-sync), and never tells the operator to
+ * reinstall when the bindings already carry a versioned sentinel.
+ */
+import { describe, expect, it } from "bun:test";
+import { validateLoadedBindings } from "../native/loader-state.js";
+
+const candidate = "/home/u/.bun/install/global/node_modules/@oh-my-pi/pi-natives-linux-x64/pi_natives.linux-x64.node";
+
+function ctxFor(version: string) {
+	return {
+		isWorkspaceLoad: false,
+		packageVersion: version,
+		versionSentinelExport: `__piNativesV${version.replace(/[^A-Za-z0-9]/g, "_")}`,
+	};
+}
+
+describe("issue 4812: pi-natives sentinel process-stale diagnosis", () => {
+	it("accepts bindings that expose the expected sentinel", () => {
+		const ctx = ctxFor("16.3.11");
+		expect(() =>
+			validateLoadedBindings(ctx, { __piNativesV16_3_11: () => {}, grep: () => {} }, candidate),
+		).not.toThrow();
+	});
+
+	it("reports a mid-session upgrade (restart) when bindings carry an older sentinel", () => {
+		const ctx = ctxFor("16.3.11");
+		const resident = { __piNativesV16_3_10: () => {}, grep: () => {} };
+		let message = "";
+		try {
+			validateLoadedBindings(ctx, resident, candidate);
+		} catch (err) {
+			message = err instanceof Error ? err.message : String(err);
+		}
+		expect(message).toContain("16.3.10");
+		expect(message).toContain("restart omp");
+		expect(message).toContain("Disk is already consistent");
+		// The disk-stale advice must NOT appear for a process-stale mix.
+		expect(message).not.toContain("reinstall to re-sync");
+		expect(message).not.toContain("from a different release than this loader");
+	});
+
+	it("still reports disk-stale (reinstall) when no versioned sentinel is present", () => {
+		const ctx = ctxFor("16.3.11");
+		const stale = { grep: () => {}, astGrep: () => {} };
+		let message = "";
+		try {
+			validateLoadedBindings(ctx, stale, candidate);
+		} catch (err) {
+			message = err instanceof Error ? err.message : String(err);
+		}
+		expect(message).toContain("from a different release than this loader");
+		expect(message).toContain("reinstall to re-sync");
+		expect(message).not.toContain("restart omp");
+	});
+
+	it("skips validation entirely in workspace dev", () => {
+		const ctx = { ...ctxFor("16.3.11"), isWorkspaceLoad: true };
+		expect(() => validateLoadedBindings(ctx, { grep: () => {} }, candidate)).not.toThrow();
+	});
+});