fix: retry sqlite-vec load without .dll suffix on Windows#69059
fix: retry sqlite-vec load without .dll suffix on Windows#69059JustInCache wants to merge 2 commits into
Conversation
Greptile SummaryAdds a Windows-specific fallback in Confidence Score: 5/5Safe to merge; the fallback logic is correct and non-breaking on all platforms. Both changed files apply an identical, well-scoped retry that only activates on Windows when the bundled load path ends with No files require special attention. Prompt To Fix All With AIThis is a comment left during a code review.
Path: packages/memory-host-sdk/src/host/sqlite-vec.ts
Line: 38
Comment:
**Original error silently lost on retry failure**
If `params.db.loadExtension(extensionPath.slice(0, -4))` also throws, the exception propagates to the outer catch block and `firstErr` is silently dropped. Diagnostics will only show the retry error, making it harder to understand why both attempts failed.
Consider chaining the original cause:
```ts
// Replace the retry call to chain errors:
try {
params.db.loadExtension(extensionPath.slice(0, -4));
} catch (retryErr) {
const err = new Error(`sqlite-vec retry without .dll suffix also failed`, { cause: retryErr });
(err as any).firstError = firstErr;
throw err;
}
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/memory-host-sdk/host/sqlite-vec.ts
Line: 38
Comment:
**Original error silently lost on retry failure**
Same issue as in `packages/memory-host-sdk/src/host/sqlite-vec.ts`: if the retry `loadExtension` call throws, `firstErr` is silently discarded and only the retry error surfaces in the outer catch. This makes debugging dual-failure scenarios harder.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix: retry sqlite-vec load without .dll ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // If the bundled load() call fails and the resolved path ends with | ||
| // .dll, retry by passing the path directly without the suffix. | ||
| if (process.platform === "win32" && extensionPath.toLowerCase().endsWith(".dll")) { | ||
| params.db.loadExtension(extensionPath.slice(0, -4)); |
There was a problem hiding this comment.
If params.db.loadExtension(extensionPath.slice(0, -4)) also throws, the exception propagates to the outer catch block and firstErr is silently dropped. Diagnostics will only show the retry error, making it harder to understand why both attempts failed.
Consider chaining the original cause:
// Replace the retry call to chain errors:
try {
params.db.loadExtension(extensionPath.slice(0, -4));
} catch (retryErr) {
const err = new Error(`sqlite-vec retry without .dll suffix also failed`, { cause: retryErr });
(err as any).firstError = firstErr;
throw err;
}Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/memory-host-sdk/src/host/sqlite-vec.ts
Line: 38
Comment:
**Original error silently lost on retry failure**
If `params.db.loadExtension(extensionPath.slice(0, -4))` also throws, the exception propagates to the outer catch block and `firstErr` is silently dropped. Diagnostics will only show the retry error, making it harder to understand why both attempts failed.
Consider chaining the original cause:
```ts
// Replace the retry call to chain errors:
try {
params.db.loadExtension(extensionPath.slice(0, -4));
} catch (retryErr) {
const err = new Error(`sqlite-vec retry without .dll suffix also failed`, { cause: retryErr });
(err as any).firstError = firstErr;
throw err;
}
```
How can I resolve this? If you propose a fix, please make it concise.| // without the .dll suffix so SQLite can append it automatically, | ||
| // mirroring what it does on Linux (.so) and macOS (.dylib). | ||
| // If the bundled load() call fails and the resolved path ends with | ||
| // .dll, retry by passing the path directly without the suffix. |
There was a problem hiding this comment.
Same issue as in packages/memory-host-sdk/src/host/sqlite-vec.ts: if the retry loadExtension call throws, firstErr is silently discarded and only the retry error surfaces in the outer catch. This makes debugging dual-failure scenarios harder.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/memory-host-sdk/host/sqlite-vec.ts
Line: 38
Comment:
**Original error silently lost on retry failure**
Same issue as in `packages/memory-host-sdk/src/host/sqlite-vec.ts`: if the retry `loadExtension` call throws, `firstErr` is silently discarded and only the retry error surfaces in the outer catch. This makes debugging dual-failure scenarios harder.
How can I resolve this? If you propose a fix, please make it concise.![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f1d8bbfe29
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // If the bundled load() call fails and the resolved path ends with | ||
| // .dll, retry by passing the path directly without the suffix. | ||
| if (process.platform === "win32" && extensionPath.toLowerCase().endsWith(".dll")) { | ||
| params.db.loadExtension(extensionPath.slice(0, -4)); |
There was a problem hiding this comment.
When the Windows .dll retry path succeeds, this function still returns the original extensionPath (with .dll), and callers persist that value (see extensions/memory-core/src/memory/manager-sync-ops.ts where loaded.extensionPath is saved and reused). On the next vector reset/reload, loading goes through the resolvedPath branch (db.loadExtension(pathWithDll)) with no fallback, so the same Windows environments this commit targets can fail again after reindex/reset even though the first load succeeded. Return/update the suffixless path after a successful retry so subsequent loads remain stable.
Useful? React with 👍 / 👎.
CI note + review feedback addressedFailing checks ( Review feedback addressed in the follow-up commit (
|
c485f5a to
efa5e51
Compare
|
Codex review: needs changes before merge. Reviewed July 3, 2026, 2:56 AM ET / 06:56 UTC. Summary PR surface: Source +30, Tests +80, Docs +1. Total +111 across 3 files. Reproducibility: yes. for the patch defect: source inspection shows the suffixless retry can return success without current main's Review metrics: 2 noteworthy metrics.
Stored data model Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Copy recommended automerge instructionNext step before merge
Security Review findings
Review detailsBest possible solution: Land a current-main repair that adds a verified Windows Do we have a high-confidence way to reproduce the issue? Yes for the patch defect: source inspection shows the suffixless retry can return success without current main's Is this the best way to solve the issue? No, not as submitted. The retry idea is the right narrow fix, but it should be layered into current main's verified load helper and platform-variant fallback rather than merged from this conflicting branch shape. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 8ed6c78b7891. Label changesLabel justifications:
Evidence reviewedPR surface: Source +30, Tests +80, Docs +1. Total +111 across 3 files. View PR surface stats
Acceptance criteria:
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
Closes openclaw#68892 On Windows, node:sqlite's DatabaseSync.loadExtension() may require the shared library path without the .dll suffix, letting SQLite append it automatically — the same convention used on Linux (.so) and macOS (.dylib). When the sqlite-vec package's bundled load() call fails on Windows and the resolved extension path ends with .dll, the loader now retries by stripping the suffix. The fix is applied to both the main SDK copy (src/memory-host-sdk/host/sqlite-vec.ts) and the packages workspace copy (packages/memory-host-sdk/src/host/sqlite-vec.ts) so both build targets stay in sync. User-supplied extensionPath overrides are passed through unchanged so callers who already pin a working path are unaffected. Made-with: Cursor
- Codex P2: honor explicit extensionPath before importing the bundled sqlite-vec module. Explicit paths now short-circuit at the top of loadSqliteVecExtension (matching current main's ordering), so configured override paths work even when the sqlite-vec package is not installed — consistent with the regression test added by BunsDev. - Codex P3: add a CHANGELOG entry under Unreleased/Fixes referencing openclaw#68892. - Tests: extend sqlite-vec.test.ts with four focused cases: Windows retry succeeds and returns the suffixless path; Windows dual failure surfaces both errors via { cause }; non-Windows platforms do not retry on bundled load failure; explicit extensionPath is already covered by the existing test and remains unaffected. Squashes the previously separate review-feedback commit into a single clean rebased state on top of current main. Co-authored-by: Cursor <[email protected]>
efa5e51 to
5e8e95e
Compare
openclaw-barnacle
Bot
added
the
triage: needs-real-behavior-proof
|
Addressed all review feedback (clawsweeper P2/P3) in the latest push. The branch has also been rebased onto current main, squashing the two previous commits into the same clean shape. Clawsweeper P2 — Honor explicit extensionPath before importing sqlite-vec
Clawsweeper P3 — Changelog entry Added a single-line entry under Tests added (extends the existing
Note on The previous commits touched the Re-review progress:
|
clawsweeper
Bot
added
the
proof: sufficient
openclaw-barnacle
Bot
removed
the
proof: sufficient
clawsweeper
Bot
added
the
proof: sufficient
openclaw-barnacle
Bot
removed
the
proof: sufficient
clawsweeper
Bot
added
the
proof: sufficient
openclaw-barnacle
Bot
removed
the
proof: sufficient
clawsweeper
Bot
added
the
proof: sufficient
openclaw-barnacle
Bot
removed
the
proof: sufficient
|
This pull request has been automatically marked as stale due to inactivity. |
openclaw-barnacle
Bot
added
stale
clawsweeper
Bot
added
rating: 🌊 off-meta tidepool
clawsweeper
Bot
added
proof: sufficient
clawsweeper
Bot
removed
the
rating: 🧂 unranked krab
3 participants
Summary
Fixes #68892.
On Windows,
node:sqlite'sDatabaseSync.loadExtension()may require the shared-library path without the platform suffix so SQLite can append it automatically — the same convention already used on Linux (.so) and macOS (.dylib). When thesqlite-vecpackage's bundledload()call fails on Windows and the auto-resolved extension path ends with.dll, the loader now retries by stripping the suffix before callingdb.loadExtension()directly.Root cause
sqlite-vec.load(db)resolves the bundledvec0.dllpath viagetLoadablePath()and passes it todb.loadExtension(). On some Windows Node.js builds,loadExtension('path/to/vec0.dll')fails because SQLite tries to openvec0.dllas-is; passing'path/to/vec0'(no suffix) allows SQLite to append.dllitself, matching its normal extension-loading convention.Change
Applied to
packages/memory-host-sdk/src/host/sqlite-vec.ts:User-supplied
extensionPathoverrides are checked first (before importingsqlite-vec) and passed through unchanged — callers who already pin a working path are unaffected.Test plan
sqlite-vec-windows-x64installed:npx openclaw memory indexsucceeds and logs nosqlite-vec unavailablewarningopenclaw doctorshows vector search as availableextensionPathis set: it is used as-is with no suffix strippingReal behavior proof
Behavior or issue addressed: On Windows,
sqlite-vecmemory vector search failed with "sqlite-vec unavailable" becauseDatabaseSync.loadExtension("path/vec0.dll")fails on some Node.js 22 Windows builds; SQLite requires the path without the.dllsuffix so it can append the suffix itself.Real environment tested: Windows 11 22H2, Node.js 22.4.0 x64,
sqlite-vec-windows-x640.1.9,openclawgateway local install.Exact steps or command run after fix:
Evidence after fix:
Terminal capture — before and after:
Observed result after fix:
openclaw memory indexcompletes without the "sqlite-vec unavailable" warning, the vector index is built, andopenclaw doctorreports vector search as available.What was not tested: ARM64 Windows; Node.js 20 on Windows (only 22 tested).