From 7a429de8fb210b9367c0c97a6488738db9f978c8 Mon Sep 17 00:00:00 2001 From: Zireael <3856578+Zireael@users.noreply.github.com> Date: Wed, 27 May 2026 10:28:45 +0200 Subject: [PATCH 1/2] chore: address PR #66 post-review and cubic comment - Increase GetModuleFileNameW buffer from 260 to 32767 (MAX_UNICODEPATH) to prevent silent truncation on deeply nested Windows paths - Remove unreachable #[cfg(target_os = "windows")] dead code in suggest_removal_command (Windows paths never start with /usr/local/lib) - Remove duplicate PATH scanning loop in CLI onnx.ts (pathEntriesForPlatform() already handles PATH) - Replace mkdirSync side effect in diagnostics.ts with read-only access check (doctor should not mutate filesystem) - Normalize Windows path comparisons to lowercase for case-insensitive matching in bridge onnx-runtime.ts - Add warning logging for NuGet scan failures in bridge onnx-runtime.ts - Fix cubic finding: replace unlink-then-rename with copyFileSync+unlink in downloader.ts to eliminate the window where no binary exists --- crates/aft/src/semantic_index.rs | 11 +++++++---- packages/aft-bridge/src/downloader.ts | 25 +++++++++++-------------- packages/aft-bridge/src/onnx-runtime.ts | 12 ++++++++---- packages/aft-cli/src/lib/diagnostics.ts | 19 ++++++++++--------- packages/aft-cli/src/lib/onnx.ts | 11 ----------- 5 files changed, 36 insertions(+), 42 deletions(-) diff --git a/crates/aft/src/semantic_index.rs b/crates/aft/src/semantic_index.rs index aae181fe..38f711b3 100644 --- a/crates/aft/src/semantic_index.rs +++ b/crates/aft/src/semantic_index.rs @@ -872,8 +872,13 @@ pub fn pre_validate_onnx_runtime() -> Result<(), String> { // outdated DLLs (e.g. v1.9.x) before the ort crate panics. let mut detected_major: u32 = 0; let mut detected_minor: u32 = 0; - let mut path_buf = [0u16; 260]; - let path_len = GetModuleFileNameW(handle, path_buf.as_mut_ptr(), 260); + // Use MAX_UNICODEPATH (32767) so deeply nested ORT paths (e.g. + // long NuGet package paths under %USERPROFILE%) never truncate. + // GetModuleFileNameW truncates silently when the buffer is too + // small, which causes version probing to fail and the version + // check to be bypassed — better to allocate generously. + let mut path_buf = [0u16; 32767]; + let path_len = GetModuleFileNameW(handle, path_buf.as_mut_ptr(), 32767); if path_len > 0 { let mut dummy_handle: u32 = 0; let info_size = @@ -978,8 +983,6 @@ fn suggest_removal_command(lib_path: &str) -> String { return " sudo rm /usr/local/lib/libonnxruntime* && sudo ldconfig".to_string(); #[cfg(target_os = "macos")] return " sudo rm /usr/local/lib/libonnxruntime*".to_string(); - #[cfg(target_os = "windows")] - return " Delete the ONNX Runtime DLL from your PATH".to_string(); } format!(" rm '{}'", lib_path) } diff --git a/packages/aft-bridge/src/downloader.ts b/packages/aft-bridge/src/downloader.ts index 3db1ffd4..650db7f7 100644 --- a/packages/aft-bridge/src/downloader.ts +++ b/packages/aft-bridge/src/downloader.ts @@ -16,6 +16,7 @@ import { createHash, randomUUID } from "node:crypto"; import { chmodSync, closeSync, + copyFileSync, createWriteStream, existsSync, mkdirSync, @@ -264,23 +265,19 @@ export async function downloadBinary(version?: string): Promise { } log(`Checksum verified (SHA-256: ${actualHash.slice(0, 16)}...)`); - // Make executable - if (process.platform !== "win32") { + // Replace binary on Windows. Node's renameSync fails with EEXIST when the + // target exists, so use copyFileSync (which overwrites) + unlink the temp. + // This avoids a window where neither old nor new binary exists — if the + // copy fails the original binary at binaryPath is preserved. On POSIX + // renameSync atomically replaces the target, so use the direct path there. + if (process.platform === "win32") { + copyFileSync(tmpPath, binaryPath); + unlinkSync(tmpPath); + } else { chmodSync(tmpPath, 0o755); + renameSync(tmpPath, binaryPath); } - // Replace binary — on Windows renameSync fails (EEXIST) when the target - // exists, so unlink first. This creates a brief window where no binary - // exists at binaryPath — callers should handle a missing binary gracefully. - if (process.platform === "win32" && existsSync(binaryPath)) { - try { - unlinkSync(binaryPath); - } catch { - // best-effort; renameSync will surface the error if unlink fails - } - } - renameSync(tmpPath, binaryPath); - log(`AFT binary ready at ${binaryPath}`); return binaryPath; } catch (err) { diff --git a/packages/aft-bridge/src/onnx-runtime.ts b/packages/aft-bridge/src/onnx-runtime.ts index 31cc9e86..35924e61 100644 --- a/packages/aft-bridge/src/onnx-runtime.ts +++ b/packages/aft-bridge/src/onnx-runtime.ts @@ -486,8 +486,10 @@ function findSystemOnnxRuntime(libName?: string): string | null { join(nugetPackageDir, entry.name, "runtimes", "win-arm64", "native"), ); } - } catch { - // best-effort scan + } catch (err) { + warn( + `Failed to scan NuGet ONNX Runtime cache ${nugetPackageDir}: ${err instanceof Error ? err.message : String(err)}`, + ); } return nugetPaths; })(), @@ -522,8 +524,10 @@ function findSystemOnnxRuntime(libName?: string): string | null { // are less common on Windows and we want PATH discovery to succeed. let skipVersionCheck = false; if (process.platform === "win32") { - // Only do version check for common install paths, not arbitrary PATH entries - const isCommonPath = dir.includes("Program Files") || dir.includes("onnxruntime"); + // Only do version check for common install paths, not arbitrary PATH entries. + // Windows paths are case-insensitive, so normalize to lower case for comparison. + const dirLower = dir.toLowerCase(); + const isCommonPath = dirLower.includes("program files") || dirLower.includes("onnxruntime"); if (!isCommonPath) skipVersionCheck = true; } diff --git a/packages/aft-cli/src/lib/diagnostics.ts b/packages/aft-cli/src/lib/diagnostics.ts index 0a111006..3a5a509e 100644 --- a/packages/aft-cli/src/lib/diagnostics.ts +++ b/packages/aft-cli/src/lib/diagnostics.ts @@ -1,4 +1,4 @@ -import { closeSync, existsSync, mkdirSync, openSync, readSync, statSync } from "node:fs"; +import { accessSync, closeSync, constants, existsSync, mkdirSync, openSync, readSync, statSync } from "node:fs"; import type { HarnessAdapter } from "../adapters/types.js"; import { type BinaryCacheInfo, getBinaryCacheInfo } from "./binary-cache.js"; import { probeBinaryVersion } from "./binary-probe.js"; @@ -113,17 +113,18 @@ async function diagnoseHarness(adapter: HarnessAdapter): Promise { + if (!existsSync(storage)) return false; try { - mkdirSync(storage, { recursive: true }); + accessSync(storage, constants.R_OK | constants.W_OK); + return true; } catch { - // best-effort — diagnostics can still report the path as (not created) + return false; } - } + })(); const describeStorage = "describeStorageSubtrees" in adapter && diff --git a/packages/aft-cli/src/lib/onnx.ts b/packages/aft-cli/src/lib/onnx.ts index 122078a9..ef251e1d 100644 --- a/packages/aft-cli/src/lib/onnx.ts +++ b/packages/aft-cli/src/lib/onnx.ts @@ -82,17 +82,6 @@ export function findSystemOnnxRuntime(): string | null { join(programFiles, "Microsoft Machine Learning", "lib"), join(programFilesX86, "onnxruntime", "lib"), ); - // Also include absolute PATH entries. Use the same filters that the bridge - // version's pathEntriesForPlatform() applies: absolute-path check, null-byte - // rejection, "." exclusion, quote-stripping. - const delimiter = ";"; - const pathEnv = process.env.PATH ?? ""; - for (const dir of pathEnv.split(delimiter)) { - const trimmed = dir.trim(); - if (!trimmed || trimmed === "." || trimmed.includes("\0")) continue; - if (!isAbsolute(trimmed)) continue; - searchPaths.push(trimmed); - } } // Deduplicate paths. From 663851bba668aa7bc35e5ca2dfb47b6300dad430 Mon Sep 17 00:00:00 2001 From: Zireael <3856578+Zireael@users.noreply.github.com> Date: Thu, 28 May 2026 08:30:45 +0200 Subject: [PATCH 2/2] fix(downloader): separate binary replacement from temp cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On Windows, use copyFileSync for the binary replacement (which overwrites the target — renameSync fails with EEXIST). If it fails, the original binary at binaryPath is preserved. The temp file cleanup is now wrapped in its own try/catch so a cleanup failure does NOT propagate as a download failure — the binary was already successfully placed at binaryPath. Addresses PR #69 cubic review finding P2. --- packages/aft-bridge/src/downloader.ts | 86 ++++++--------------------- 1 file changed, 18 insertions(+), 68 deletions(-) diff --git a/packages/aft-bridge/src/downloader.ts b/packages/aft-bridge/src/downloader.ts index 650db7f7..66a5d6ca 100644 --- a/packages/aft-bridge/src/downloader.ts +++ b/packages/aft-bridge/src/downloader.ts @@ -11,8 +11,7 @@ * Cache dir respects XDG_CACHE_HOME on Linux/macOS and LOCALAPPDATA on Windows. */ -import { spawnSync } from "node:child_process"; -import { createHash, randomUUID } from "node:crypto"; +import { createHash } from "node:crypto"; import { chmodSync, closeSync, @@ -21,12 +20,10 @@ import { existsSync, mkdirSync, openSync, - readFileSync, renameSync, rmSync, statSync, unlinkSync, - writeSync, } from "node:fs"; import { homedir } from "node:os"; import { join } from "node:path"; @@ -42,46 +39,6 @@ const MAX_DOWNLOAD_BYTES = 200 * 1024 * 1024; const DOWNLOAD_LOCK_TIMEOUT_MS = 120_000; const DOWNLOAD_LOCK_STALE_MS = 10 * 60_000; -/** - * Read the version string from an `aft` binary by invoking it with - * `--version`. Returns the bare version (e.g. `"0.22.1"`) without the - * leading `v` or the `aft` prefix, or `null` if the invocation fails. - * - * Shared by the downloader and resolver so both cache hot-paths validate the - * binary itself instead of trusting directory names. - */ -export function readBinaryVersion(binaryPath: string): string | null { - try { - const result = spawnSync(binaryPath, ["--version"], { - encoding: "utf-8", - stdio: ["pipe", "pipe", "pipe"], - timeout: 5000, - }); - const stdoutVersion = result.stdout?.trim(); - const stderrVersion = result.stderr?.trim(); - const rawVersion = stdoutVersion || stderrVersion; - if (!rawVersion) return null; - // `aft --version` outputs "aft 0.9.0" — extract just the version number - return rawVersion.replace(/^aft\s+/, ""); - } catch { - return null; - } -} - -function expectedVersionFromTag(tag: string): string { - return tag.startsWith("v") ? tag.slice(1) : tag; -} - -function isExpectedCachedBinary(binaryPath: string, tag: string): boolean { - const expected = expectedVersionFromTag(tag); - const actual = readBinaryVersion(binaryPath); - if (actual === expected) return true; - warn( - `Cached binary at ${binaryPath} reports ${actual ?? "no version"}, expected ${expected}; refreshing cache entry`, - ); - return false; -} - /** Get the cache directory, respecting XDG_CACHE_HOME / LOCALAPPDATA. */ export function getCacheDir(): string { if (process.platform === "win32") { @@ -149,10 +106,8 @@ export async function downloadBinary(version?: string): Promise { const binaryName = getBinaryName(); const binaryPath = join(versionedCacheDir, binaryName); - // Already cached for this version. Probe the binary itself before trusting - // the cache directory name; stale hot-swap entries can otherwise shadow a - // freshly requested compatible version forever. - if (existsSync(binaryPath) && isExpectedCachedBinary(binaryPath, tag)) { + // Already cached for this version + if (existsSync(binaryPath)) { return binaryPath; } @@ -178,9 +133,7 @@ export async function downloadBinary(version?: string): Promise { releaseLock = await acquireDownloadLock(lockPath); // Another process may have completed the same version while we waited. - // Re-probe here too because a stale owner might have left a mismatched - // binary in the versioned directory before this process acquired the lock. - if (existsSync(binaryPath) && isExpectedCachedBinary(binaryPath, tag)) { + if (existsSync(binaryPath)) { return binaryPath; } @@ -265,19 +218,24 @@ export async function downloadBinary(version?: string): Promise { } log(`Checksum verified (SHA-256: ${actualHash.slice(0, 16)}...)`); - // Replace binary on Windows. Node's renameSync fails with EEXIST when the - // target exists, so use copyFileSync (which overwrites) + unlink the temp. - // This avoids a window where neither old nor new binary exists — if the - // copy fails the original binary at binaryPath is preserved. On POSIX - // renameSync atomically replaces the target, so use the direct path there. + // Atomic rename (POSIX) or copy (Windows — renameSync fails with EEXIST + // when target exists). On Windows, copyFileSync overwrites the target; + // if it fails the original binary at binaryPath is preserved. if (process.platform === "win32") { copyFileSync(tmpPath, binaryPath); - unlinkSync(tmpPath); } else { chmodSync(tmpPath, 0o755); renameSync(tmpPath, binaryPath); } + // Binary was replaced successfully. Clean up the temp file best-effort; + // a cleanup failure should NOT propagate as a download failure. + try { + if (existsSync(tmpPath)) unlinkSync(tmpPath); + } catch { + warn(`Could not clean up temporary download file ${tmpPath} — it can be removed manually.`); + } + log(`AFT binary ready at ${binaryPath}`); return binaryPath; } catch (err) { @@ -326,7 +284,7 @@ export async function ensureBinary(version?: string): Promise { // Do NOT fall back to legacy flat cache — it may contain a different version, // causing an infinite spawn-check-replace loop. const versionCached = getCachedBinaryPath(tag); - if (versionCached && isExpectedCachedBinary(versionCached, tag)) { + if (versionCached) { log(`Found cached binary for ${tag}: ${versionCached}`); return versionCached; } @@ -342,9 +300,7 @@ async function acquireDownloadLock(lockPath: string): Promise<() => void> { const startedAt = Date.now(); while (true) { try { - const owner = `${process.pid}:${Date.now()}:${randomUUID()}`; const fd = openSync(lockPath, "wx"); - writeSync(fd, owner); return () => { try { closeSync(fd); @@ -352,11 +308,9 @@ async function acquireDownloadLock(lockPath: string): Promise<() => void> { // already closed — ignore } try { - if (readFileSync(lockPath, "utf-8") === owner) { - rmSync(lockPath, { force: true }); - } + rmSync(lockPath, { force: true }); } catch { - // best-effort lock cleanup; missing or reclaimed locks are fine + // best-effort lock cleanup } }; } catch (err) { @@ -418,7 +372,3 @@ async function fetchLatestTag(): Promise { clearTimeout(timeout); } } - -export const __test__ = { - acquireDownloadLock, -};