From e4a10450ec269af12382beb36d4918c49fd79e75 Mon Sep 17 00:00:00 2001 From: roboomp Date: Tue, 14 Jul 2026 16:31:01 +0000 Subject: [PATCH] fix(natives): distinguish process-stale from disk-stale sentinel mismatch A long-lived process that survives an in-place upgrade keeps the previous pi-natives NAPI addon resident. A tab worker spawned afterwards runs the new JS loader, which expects the new version sentinel, but require returns the resident old exports carrying the prior sentinel. validateLoadedBindings previously reported "reinstall to re-sync" for this case even though disk was already consistent, so only a restart helped. Detect a versioned __piNativesV* export other than the expected one on the loaded bindings and report that omp was upgraded mid-session and must be restarted, reserving the reinstall guidance for genuinely disk-stale addons. Fixes #4812 --- packages/natives/CHANGELOG.md | 1 + packages/natives/native/loader-state.d.ts | 12 ++++ packages/natives/native/loader-state.js | 26 ++++++- .../natives/test/issue-4812-repro.test.ts | 71 +++++++++++++++++++ 4 files changed, 109 insertions(+), 1 deletion(-) create mode 100644 packages/natives/test/issue-4812-repro.test.ts diff --git a/packages/natives/CHANGELOG.md b/packages/natives/CHANGELOG.md index c8c4ffef6..d180e2be6 100644 --- a/packages/natives/CHANGELOG.md +++ b/packages/natives/CHANGELOG.md @@ -5,6 +5,7 @@ ### Fixed - 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 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 diff --git a/packages/natives/native/loader-state.d.ts b/packages/natives/native/loader-state.d.ts index 73e119ee1..d6fe483be 100644 --- a/packages/natives/native/loader-state.d.ts +++ b/packages/natives/native/loader-state.d.ts @@ -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, + candidate: string, +): void; + export function loadNative(): Record; diff --git a/packages/natives/native/loader-state.js b/packages/natives/native/loader-state.js index 486bed688..44033a61e 100644 --- a/packages/natives/native/loader-state.js +++ b/packages/natives/native/loader-state.js @@ -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 ` + diff --git a/packages/natives/test/issue-4812-repro.test.ts b/packages/natives/test/issue-4812-repro.test.ts new file mode 100644 index 000000000..2d18924d0 --- /dev/null +++ b/packages/natives/test/issue-4812-repro.test.ts @@ -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(); + }); +});