Skip to content

[needs-vps] fix(electron): strip hashed native modules from standalone and fix removal path - #7123

Closed
hoan9an wants to merge 8 commits into
diegosouzapw:release/v3.8.49from
hoan9an:fix/electron-better-sqlite3-abi-mismatch
Closed

[needs-vps] fix(electron): strip hashed native modules from standalone and fix removal path#7123
hoan9an wants to merge 8 commits into
diegosouzapw:release/v3.8.49from
hoan9an:fix/electron-better-sqlite3-abi-mismatch

Conversation

@hoan9an

@hoan9an hoan9an commented Jul 14, 2026

Copy link
Copy Markdown

Problem

Electron desktop app (3.8.45–3.8.48) fails with Internal Server Error because better-sqlite3 cannot load — the server falls back to sql.js WASM which OOMs on non-trivial databases.

Root cause: Two issues in the Electron build pipeline leave stale Node-ABI native modules in the standalone bundle, shadowing the correctly rebuilt Electron-ABI copies.

Details

  1. removeNativeModules targets wrong path: Uses .next instead of the actual distDir (.build/next), so Next.js-traced native modules with mangled names (better-sqlite3-{hash}, keytar-{hash}, sqlite-vec-{hash}) are never removed.

  2. patchTurbopackChunks not enabled: Without it, server chunks still reference the mangled module names (e.g. require('better-sqlite3-90e2652d1716b047')). The server loads the stale Node-ABI .node from the mangled copy, while the correctly rebuilt Electron-ABI copy at node_modules/better-sqlite3/ sits unused.

Changes

  • Enable patchTurbopackChunks: true in the assembleStandalone call (strips hash suffixes from require() calls in server chunks)
  • Fix removeNativeModules path: .nextNEXT_DIST_DIR
  • Add sqlite-vec to the native module removal list

Verification

The fix was manually verified on Windows 11 with OmniRoute 3.8.48 by:

  1. Copying the correctly-compiled better_sqlite3.node into the mangled module path — server started successfully
  2. Tracing the build pipeline to confirm the root cause

Closes #7082

@hoan9an
hoan9an requested a review from diegosouzapw as a code owner July 14, 2026 03:04
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@diegosouzapw

Copy link
Copy Markdown
Owner

Thanks for the clean root-cause writeup — the diagnosis (hashed native-module copies under the wrong NEXT_DIST_DIR path shadowing the Electron-ABI-rebuilt better-sqlite3, plus unpatched require() calls still targeting the hashed name) matches the app.log symptoms in #7082 exactly, and the fix reuses patchTurbopackChunks, which is already battle-tested on the npm-CLI packaging path (scripts/build/prepublish.ts) — so this isn't new logic, just enabling a proven option for a second consumer. The NEXT_DIST_DIR path fix is also correct by inspection: the old code hardcoded .next even though the file's own NEXT_DIST_DIR constant (and every other call site in the same script) already accounts for the configurable .build/next dist dir. One gap before this can merge per our test-coverage rule: there's no automated regression test (the manual-binary-swap verification on Windows is useful but not a repeatable check), and this specific script/function isn't unit-tested anywhere in the repo yet. Could you either (a) add a small unit test extracting/covering removeNativeModules()'s prefix-matching + path resolution against synthetic temp dirs (similar pattern to tests/unit/build/assemble-standalone.test.ts), or (b) if that's too heavy for a 3-line fix, attach a full electron:build → package → install → launch log confirming better-sqlite3 loads (not the sql.js WASM fallback) on a clean machine? Either closes the loop so we can merge with confidence.

@diegosouzapw
diegosouzapw changed the base branch from main to release/v3.8.49 July 15, 2026 08:19
@diegosouzapw diegosouzapw changed the title fix(electron): strip hashed native modules from standalone and fix removal path [needs-vps] fix(electron): strip hashed native modules from standalone and fix removal path Jul 15, 2026
@diegosouzapw

Copy link
Copy Markdown
Owner

Additional field confirmation for this PR, plus a suggested regression guard.

Confirmation — Discussion #7144 (@agd-stack, Windows 11 x64) isolated the trigger with a clean A/B on v3.8.48: clean DATA_DIR → /home = 200; exact copy of an existing 41 MB v3.8.44 DB → /home = 500, same binary. PRAGMA quick_check: ok, identical SHA-256, so the DB is healthy. They then rebuilt v3.8.48 from source with OMNIROUTE_USE_TURBOPACK=0 and a native better-sqlite3 rebuild for Electron 43.1.0 / ABI 148 — and the same DB works end to end (migrations through v122, /home = 200). That independently sidesteps both halves this PR fixes (the .next vs .build/next removal path, and the unpatched Turbopack hashed require), which supports the diagnosis on #7132.

Suggested regression guard. scripts/dev/smoke-electron-packaged.mjs:411-412 always boots into a fresh mkdtemp() DATA_DIR:

process.env.ELECTRON_SMOKE_DATA_DIR ||
  (await mkdtemp(join(tmpdir(), "omniroute-electron-smoke-")));

The failing code path is the existing-DB probe branch in src/lib/db/core.ts, guarded by if (fs.existsSync(sqliteFile)) — a fresh DATA_DIR never reaches it. So the packaged smoke test has zero coverage for this bug and passes green on every affected release. That is why v3.8.45 → v3.8.48 all shipped it.

The script already supports an ELECTRON_SMOKE_DATA_DIR override, so the guard is cheap: a second smoke run against a pre-populated DATA_DIR (boot once to create the DB, exit fully, boot again pointed at the same dir, assert /home = 200). That is exactly the cold-restart sequence @RCrushMe asked for on #7132 (fresh install → first launch → full exit → second cold start → dashboard healthy), and it is the difference between this fix being verified and being assumed.

Worth noting the smoke step is continue-on-error for Windows/macOS-arm64 in .github/workflows/electron-release.yml:157-172 — so even with the new coverage, a failure there would not block the release. If this scenario is the one that keeps biting users, that flag may deserve a look for Windows specifically (separate call, not this PR's job).

Not blocking the merge — this PR addresses the trigger correctly. Flagging the coverage gap so the fix does not silently regress later. Context: #7132, #7144, and the split-out fallback-ordering follow-up in #7288.

@diegosouzapw

Copy link
Copy Markdown
Owner

This PR is broader than its title suggests, and a new macOS report (#7346 / discussion #7345) confirms it. Flagging because the needs-vps label reads as "Windows fix awaiting Windows validation", and that undersells what is blocked behind it.

It is not a native-module fix — it is a packaging fix for every externalized package. @jrickybt's macOS report enumerates 14 hash-mangled externals that all fail to resolve in the packaged app, of which only three are native:

ws, pino, zod, typescript, playwright, jsdom, node-machine-id,
wreq-js, tls-client-node, @ngrok/ngrok, @huggingface/transformers,
better-sqlite3, keytar, sqlite-vec

ws is simply the first one the instrumentation hook imports, so it crashes first. The patchTurbopackChunks: true this PR enables is a blanket regex over all server chunks (assembleStandalone.mjs:347-392, HASH_RE with no module allowlist, scoped-package and subpath aware), so it fixes the whole class in one pass — which is the right shape, and worth saying explicitly in the PR title/body since reviewers may read it as scoped to better-sqlite3/keytar.

It also fixes macOS, despite being verified only on Windows. electron/package.jsonbuild:mac / build:mac-x64 / build:mac-arm64 all run the same npm run prepare:bundleprepare-electron-standalone.mjs, and the patchTurbopackChunks call is unconditional (the sole process.platform branch is the better-sqlite3 rebuild). So the code path is shared; macOS is untested, not unfixed. Net: this single PR should close out the whole "Internal Server Error" cluster — #7132, #7082, #6358, #7152, #7097, #7346 — across platforms.

Residual gap worth a second look before merge. Most of the 14 are in next.config.mjsserverExternalPackages, so Next copies the real unhashed package into the standalone bundle and the stripped require('pkg') resolves. typescript, playwright, jsdom are not — they are reached via ordinary imports (open-sse/services/compression/engines/rtk/codeStripper.ts:1, open-sse/services/browserPool.ts, src/lib/docsSanitizer.ts:2). Output-file tracing should still copy them, but if it misses one, this PR converts MODULE_NOT_FOUND: typescript-<hash> into MODULE_NOT_FOUND: typescript — better error, same dead app. A packaged launch on mac and win would settle it; static reading cannot.

Suggested regression guard (the "how did this ship" half). scripts/dev/smoke-electron-packaged.mjs already does the right thing — spawns the packaged binary, polls http://127.0.0.1:20128/login for 200, and treats /MODULE_NOT_FOUND/ and /Cannot find module/i as fatal. It would have caught both #7132 and #7346. The problem:

$ grep -rln "smoke-electron-packaged" .github/workflows/
(nothing)

It exists only as the manual electron:smoke:packaged npm script, so no packaged mac/win build is smoke-tested in CI. Wiring it into the electron release workflow — plus, per my earlier comment, a second run against a pre-populated ELECTRON_SMOKE_DATA_DIR to cover the existing-DB path from #7132 — would guard both failure modes this PR fixes.

Not blocking: the fix itself is correct and I would land it. Flagging the scope so it gets prioritized as the cross-platform release-blocker it is, rather than a Windows-only nice-to-have.

@diegosouzapw

Copy link
Copy Markdown
Owner

Two things worth knowing before this merges — a sibling PR on the same root cause, and why this one has no real CI signal.

1. #6794 attacks the same bug from the opposite direction. #6794 (materialize Turbopack hashed-module symlinks during packaging) targets the identical failure with an inverse strategy:

#6794 this PR
Strategy Materialize the hashed entries (dereference/relink the symlinks) Strip the hash so require('ws-<hash>')require('ws')
File scripts/build/assembleStandalone.mjs scripts/build/prepare-electron-standalone.mjs
Actually wired? NomaterializeSymlinks defaults to false and nothing enables it YespatchTurbopackChunks: true

Zero files in common, so both merge clean and go green independently, and the runtime outcome would be settled by whichever executes first rather than by a decision. Flagging rather than resolving — the pick is yours. My read: they are complementary (strip the require and leave no dangling links to shadow resolution), but only this PR is currently live; #6794's helper is orphaned as submitted (I have said the same over there).

One concrete conflict to settle either way: keytar. This PR adds it to the removeNativeModules list; #6794's description explicitly proposes to stop stripping keytar (though its diff does not actually do that). Both cannot be right.

2. #6794 has the better root-cause evidence, and it resolves my earlier caveat. Its author captured the actual packaged app:

ws-a972e7ffa40ff725 -> /Users/runner/work/OmniRoute/OmniRoute/.build/next/standalone/node_modules/ws

The hashed entries are not missing — they are dangling absolute symlinks into the CI runner. That explains @jrickybt's macOS xattr "No such file" on all 14 paths (#7346), and it also answers the caveat I raised in my previous comment: since every hashed entry is a symlink pointing at a real sibling in standalone/node_modules/<pkg>, the real unhashed packages do exist in the bundle — including typescript, playwright and jsdom, the three I flagged as not being in serverExternalPackages. So the hash-strip in this PR should resolve for all 14, not just the traced ones. Still worth confirming on a packaged mac build, but the static risk I raised is materially lower than I stated.

3. Why this PR looks stuck — it is not the [needs-vps] flag. It has essentially no CI signal:

PR #7123: 3 checks (2 Mergify NEUTRAL + 1 semgrep SUCCESS)  → mergeStateStatus: UNSTABLE
PR #6794: 11 checks, all SUCCESS                            → mergeStateStatus: CLEAN
a fresh PR opened yesterday: 12 checks

The base has moved 480 commits since this PR's merge-base, so the full suite (unit, build, docs gates, quality) never ran against it. A gh pr update-branch would refresh the base and trigger real CI — worth doing before judging it, since right now the green/red comparison between #6794 and this PR is not measuring code quality, it is measuring base staleness. I have not touched the branch: it is @hoan9an's fork, and that call is yours.

Net: this is the PR that actually fixes the packaged app today, and it fixes the whole externalized-module class (not just the natives) across mac/win/linux — see my earlier comment. But it should not be judged against #6794 on CI colour until its base is refreshed.

@hoan9an
hoan9an force-pushed the fix/electron-better-sqlite3-abi-mismatch branch 3 times, most recently from ec56dfd to 4a91ac3 Compare July 16, 2026 12:04
diegosouzapw and others added 7 commits July 17, 2026 08:12
…uto_merge_conditions (rules-based path is EOL 2026-07-16) (diegosouzapw#7216)
…moval path

Two issues caused better-sqlite3 (and other native modules) to fail
with NODE_MODULE_VERSION mismatch in the Electron desktop app:

1. removeNativeModules targeted '.next' instead of the actual distDir
   ('.build/next'), so Next.js-traced native modules with mangled names
   (better-sqlite3-{hash}, keytar-{hash}, sqlite-vec-{hash}) were never
   removed from the standalone bundle.

2. patchTurbopackChunks was not enabled for the Electron build, so
   server chunks still referenced the mangled module names. The server
   loaded the stale Node-ABI .node from the mangled copy, while the
   correctly rebuilt Electron-ABI copy sat unused.

Changes:
- Enable patchTurbopackChunks in assembleStandalone call (strips hash
  suffixes from require() calls in server chunks)
- Fix removeNativeModules path: '.next' -> NEXT_DIST_DIR
- Add sqlite-vec to the native module removal list

Fixes: diegosouzapw#7082
Extract removeNativeModules() into a side-effect-free module
(scripts/build/lib/removeNativeModules.mjs) so it can be unit-tested
without importing the full Electron build harness.

Add 6 tests covering:
- no-op when baseDir does not exist / is empty
- removal of hashed native modules (better-sqlite3-{hash}, keytar-{hash})
- removal of unhashed native modules matching prefixes
- default prefixes only remove keytar
- preservation of non-matching modules (prefix not at start)

Also clean up unused readdirSync import from prepare-electron-standalone.mjs.
….mergify.yml scope creep (diegosouzapw#7123)

The extract-removeNativeModules refactor dropped the buildRebuildSpawnPlan
import (still used at rebuildBetterSqlite3ForElectron), which would crash the
Electron packaging step with ReferenceError. Restore it. Also revert the
unrelated .mergify.yml merge-queue-config changes that had crept into this
Electron packaging PR (release-infra config, out of scope).

Rebased onto release/v3.8.49 so diegosouzapw#6794's materializeSymlinks:true and this PR's
patchTurbopackChunks:true both remain in the assembleStandalone() call.
@diegosouzapw
diegosouzapw force-pushed the fix/electron-better-sqlite3-abi-mismatch branch from 4a91ac3 to cd6140a Compare July 17, 2026 11:15
@diegosouzapw

Copy link
Copy Markdown
Owner

Maintainer fix-in-place pass (rebased onto release/v3.8.49, authorship preserved):

  1. Restored the buildRebuildSpawnPlan import — the extract-removeNativeModules-to-a-pure-module refactor also dropped import { buildRebuildSpawnPlan } from "./electronRebuildPlan.mjs", but buildRebuildSpawnPlan(process.platform) is still called in rebuildBetterSqlite3ForElectron(). As-was, the Electron packaging step would crash with ReferenceError: buildRebuildSpawnPlan is not defined. Re-added the import alongside the new removeNativeModules one.
  2. Dropped the unrelated .mergify.yml changes — the merge-queue config edits (auto-enqueue / dast-smoke exception / batching) are release-infra scope that had already landed on release separately; they don't belong in an Electron packaging PR. Reverted that file to release; the net diff is now the 3 Electron files only.
  3. Rebased onto release/v3.8.49 so [needs-vps] fix(electron): materialize Turbopack hashed-module symlinks during packaging (#6724, #6594) #6794's materializeSymlinks: true and this PR's patchTurbopackChunks: true both remain in the assembleStandalone() call (verified — no ordering conflict; hash-stripping runs on the already-materialized standalone).

Validation: the extracted removeNativeModules() is now unit-tested (tests/unit/build/prepare-electron-standalone.test.ts, 6/6 pass) covering the NEXT_DIST_DIR-vs-.next path fix + prefix matching + no-op edge cases. patchTurbopackChunks reuses the path already proven on the npm-CLI packaging build (scripts/build/prepublish.ts). The full packaged .deb build+boot was not re-run here (headless VPS) — flagging for a manual Electron smoke before/after merge if desired.

@diegosouzapw

Copy link
Copy Markdown
Owner

Thank you for digging into the Electron better-sqlite3 ABI mismatch — that's a genuinely nasty, hard-to-diagnose class of bug, and the analysis in your PR was sharp. 🙏

I'm closing it as subsumed, and I want to show the proof because your diagnosis was actually correct — the root cause just got covered by two other merges since I first reviewed this. Both mechanisms you propose are now already on release/v3.8.49: #6605 added rebuildBetterSqlite3ForElectron() + removeNativeModules(['better-sqlite3','keytar']) under .next/node_modules (the exact hashed-module shadowing you describe), and #7353 (merged 2026-07-19) flipped patchTurbopackChunks: true for the Electron build. I verified both are present on the current tip.

So the bug you targeted should now be fixed on the release branch. If you build from the latest release and still hit an ABI mismatch, please reopen or file an issue with the exact error — I'd want to know. Really appreciate the careful work here; please keep contributing! 🚀

NBN-N3 added a commit to NBN-N3/OmniRoute that referenced this pull request Jul 27, 2026
removeNativeModules() was called with a hardcoded ".next" path while the actual
distDir is NEXT_DIST_DIR (".build/next" by default). Because the function
early-returns when the directory does not exist, the cleanup silently no-opped
and the plain-Node-ABI better-sqlite3 copy produced by `next build` survived
into the packaged app.

At runtime the standalone server runs under ELECTRON_RUN_AS_NODE, so it needs
the Electron ABI (148 for electron 43). Loading the ABI-137 copy fails with
ERR_DLOPEN_FAILED, the app falls back to the sql.js WASM driver, the connection
is closed and retried in a loop, WASM memory is never reclaimed and the process
OOMs -> HTTP 500 on every route.

Also adds assertNoStaleHashedNatives() so a wrong baseDir fails the build
instead of silently shipping a broken installer. This has regressed at least
twice (diegosouzapw#1497 with ABI 127 vs 145, diegosouzapw#7082/diegosouzapw#7681 with 137 vs 148).

Refs diegosouzapw#7082, diegosouzapw#7681, diegosouzapw#1497, diegosouzapw#8792. Supersedes the abandoned diegosouzapw#7123.
diegosouzapw pushed a commit that referenced this pull request Jul 28, 2026
…8794)

removeNativeModules() was called with a hardcoded ".next" path while the actual
distDir is NEXT_DIST_DIR (".build/next" by default). Because the function
early-returns when the directory does not exist, the cleanup silently no-opped
and the plain-Node-ABI better-sqlite3 copy produced by `next build` survived
into the packaged app.

At runtime the standalone server runs under ELECTRON_RUN_AS_NODE, so it needs
the Electron ABI (148 for electron 43). Loading the ABI-137 copy fails with
ERR_DLOPEN_FAILED, the app falls back to the sql.js WASM driver, the connection
is closed and retried in a loop, WASM memory is never reclaimed and the process
OOMs -> HTTP 500 on every route.

Also adds assertNoStaleHashedNatives() so a wrong baseDir fails the build
instead of silently shipping a broken installer. This has regressed at least
twice (#1497 with ABI 127 vs 145, #7082/#7681 with 137 vs 148).

Refs #7082, #7681, #1497, #8792. Supersedes the abandoned #7123.
HouMinXi pushed a commit to HouMinXi/OmniRoute that referenced this pull request Aug 2, 2026
…iegosouzapw#8794)

removeNativeModules() was called with a hardcoded ".next" path while the actual
distDir is NEXT_DIST_DIR (".build/next" by default). Because the function
early-returns when the directory does not exist, the cleanup silently no-opped
and the plain-Node-ABI better-sqlite3 copy produced by `next build` survived
into the packaged app.

At runtime the standalone server runs under ELECTRON_RUN_AS_NODE, so it needs
the Electron ABI (148 for electron 43). Loading the ABI-137 copy fails with
ERR_DLOPEN_FAILED, the app falls back to the sql.js WASM driver, the connection
is closed and retried in a loop, WASM memory is never reclaimed and the process
OOMs -> HTTP 500 on every route.

Also adds assertNoStaleHashedNatives() so a wrong baseDir fails the build
instead of silently shipping a broken installer. This has regressed at least
twice (diegosouzapw#1497 with ABI 127 vs 145, diegosouzapw#7082/diegosouzapw#7681 with 137 vs 148).

Refs diegosouzapw#7082, diegosouzapw#7681, diegosouzapw#1497, diegosouzapw#8792. Supersedes the abandoned diegosouzapw#7123.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(startup): Internal Server Error since 3.8.45

2 participants