Skip to content
Merged
Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 65 additions & 5 deletions src/infrastructure/native.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

import { createRequire } from 'node:module';
import os from 'node:os';
import { fileURLToPath } from 'node:url';
import { EngineError, toErrorMessage } from '../shared/errors.js';
import type { NativeAddon } from '../types.js';
import { debug } from './logger.js';
Expand Down Expand Up @@ -44,31 +45,90 @@ const PLATFORM_PACKAGES: Record<string, string> = {
'win32-x64': '@optave/codegraph-win32-x64-msvc',
};

/**
* Map of (platform-arch[-libc]) → locally compiled binary filename.
* Used as a dev-mode fallback when the npm package is not installed,
* e.g. when working with Rust changes that haven't been published yet.
*/
Comment on lines +49 to +53

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 The JSDoc for PLATFORM_LOCAL_BINARIES says it is a "dev-mode fallback when the npm package is not installed", but the load order now puts it before the npm package — it is checked regardless of whether the npm package is installed. The comment misrepresents the new priority semantics.

Suggested change
/**
* Map of (platform-arch[-libc]) locally compiled binary filename.
* Used as a dev-mode fallback when the npm package is not installed,
* e.g. when working with Rust changes that haven't been published yet.
*/
/**
* Map of (platform-arch[-libc]) locally compiled binary filename.
* Checked before the npm package so that locally compiled Rust changes
* are picked up immediately in development without publishing a new release.
*/

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Fix in Claude Code

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — updated the JSDoc to 'Checked before the npm package so that locally compiled Rust changes are picked up immediately in development without publishing a new release.' (commit 183f78e)

const PLATFORM_LOCAL_BINARIES: Record<string, string> = {
'linux-x64-gnu': 'codegraph-core.linux-x64-gnu.node',
'linux-x64-musl': 'codegraph-core.linux-x64-musl.node',
'linux-arm64-gnu': 'codegraph-core.linux-arm64-gnu.node',
'linux-arm64-musl': 'codegraph-core.linux-arm64-musl.node',
'darwin-arm64': 'codegraph-core.darwin-arm64.node',
'darwin-x64': 'codegraph-core.darwin-x64.node',
'win32-x64': 'codegraph-core.win32-x64-msvc.node',
};

/** Compute the platform key used to index PLATFORM_PACKAGES / PLATFORM_LOCAL_BINARIES. */
function resolvePlatformKey(): string {
const platform = os.platform();
const arch = os.arch();
return platform === 'linux' ? `${platform}-${arch}-${detectLibc()}` : `${platform}-${arch}`;
}

/**
* Resolve the platform-specific npm package name for the native addon.
* Returns null if the current platform is not supported.
*/
function resolvePlatformPackage(): string | null {
const platform = os.platform();
const arch = os.arch();
const key = platform === 'linux' ? `${platform}-${arch}-${detectLibc()}` : `${platform}-${arch}`;
return PLATFORM_PACKAGES[key] || null;
return PLATFORM_PACKAGES[resolvePlatformKey()] ?? null;
}

/**
* Try to load the native napi addon.
* Returns the module on success, null on failure.
*
* Load order:
* 1. NAPI_RS_NATIVE_LIBRARY_PATH env var (explicit override)
* 2. locally compiled binary in crates/codegraph-core/ (dev mode — preferred
* over the npm package so that Rust changes are picked up immediately
* without publishing a new release)
* 3. npm platform package (production path)
*/
export function loadNative(): NativeAddon | null {
if (_cached !== undefined) return _cached;

const pkg = resolvePlatformPackage();
const platformKey = resolvePlatformKey();

// 1. Explicit path override — highest priority.
const envPath = process.env.NAPI_RS_NATIVE_LIBRARY_PATH;
if (envPath) {
try {
_cached = _require(envPath) as NativeAddon;
debug(`loadNative: loaded from NAPI_RS_NATIVE_LIBRARY_PATH: ${envPath}`);
return _cached;
} catch (err) {
_loadError = err as Error;
debug(`loadNative: NAPI_RS_NATIVE_LIBRARY_PATH load failed: ${toErrorMessage(err as Error)}`);
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Explicit env-var failure silently falls through to other loaders

When NAPI_RS_NATIVE_LIBRARY_PATH is set but the load fails (bad path, ABI mismatch, corrupt file, etc.), the code catches the error, logs a debug line, and continues to the local binary and npm package. An explicit override failing silently and loading a different binary is hard to diagnose — the operator set the variable intentionally and has no indication it was ignored. This path should either return null immediately (preserving _loadError so getNative() surfaces the real reason) or at least log at warning level rather than debug. As written, _loadError is also left pointing at the env-var failure even when a subsequent fallback succeeds, creating a misleading error state if _loadError is ever inspected externally.

Fix in Claude Code

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — when NAPI_RS_NATIVE_LIBRARY_PATH is set but fails to load, we now: (1) log at warn level instead of debug, (2) set _cached = null and return immediately, so no other loader is tried. This ensures an operator who set the variable explicitly gets a visible warning and a clear error from getNative() rather than silently loading a different binary. (commit 183f78e)


// 2. Locally compiled dev binary — preferred over npm package so that Rust
// changes are visible without publishing. Only used when the file exists.
const localFile = PLATFORM_LOCAL_BINARIES[platformKey];
if (localFile) {
try {
const localPath = fileURLToPath(
new URL(`../../crates/codegraph-core/${localFile}`, import.meta.url),
);
_cached = _require(localPath) as NativeAddon;
debug(`loadNative: loaded local dev binary: ${localPath}`);
return _cached;
} catch (err) {
debug(`loadNative: local dev binary not available: ${toErrorMessage(err as Error)}`);
}
}
Comment on lines +114 to +138

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Local binary load failure silently falls through to the npm package

When the local dev binary exists on disk but fails to load (e.g., a stale .node file from a prior build after an ABI change), the catch block logs at debug level and falls through to the npm package — the exact scenario the PR is trying to prevent. A developer would see their Rust changes "tested" but the integration suite would actually run against the stale npm binary, potentially giving a false-green.

The env-var path (step 1) already distinguishes "explicit operator choice, failure is fatal" from general failures. The same distinction is worth making here: err.code === 'MODULE_NOT_FOUND' means the file simply isn't there (normal in CI/production, silent skip is correct); any other error means the file exists but failed to load — that case should log at warn level and ideally return null rather than fall through.

Fix in Claude Code

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — when a local binary exists on disk but fails to load, we now use existsSync to distinguish the two cases: if the file is absent, fall through normally to the npm package; if the file is present but require throws, log at warn level and return null immediately (no fallthrough). This mirrors the same pattern used for NAPI_RS_NATIVE_LIBRARY_PATH failures. Commit: f57ad9c.


// 3. Published npm platform package — production path.
const pkg = PLATFORM_PACKAGES[platformKey] ?? null;
if (pkg) {
try {
_cached = _require(pkg) as NativeAddon;
return _cached;
} catch (err) {
_loadError = err as Error;
debug(`loadNative: npm package ${pkg} not available: ${toErrorMessage(err as Error)}`);
}
} else {
_loadError = new Error(`Unsupported platform: ${os.platform()}-${os.arch()}`);
Expand Down
Loading