Skip to content

multi: run the wasm build under a Node host, not only a browser - #1064

Merged
Roasbeef merged 4 commits into
mainfrom
nodejs-storage-backend
Aug 13, 2026
Merged

multi: run the wasm build under a Node host, not only a browser#1064
Roasbeef merged 4 commits into
mainfrom
nodejs-storage-backend

Conversation

@Roasbeef

Copy link
Copy Markdown
Member

In this PR, we let the js/wasm build store its databases wherever the host
can actually keep them, so the daemon runs in a Node process and not only in a
browser.

The DSN builders hardcoded vfs=opfs, which is correct for the only host they
were written for and wrong for the other one: a Node process has no OPFS at
all, but it does have a real filesystem, reachable through the nodefs VFS in
go-wasmsqlite#9. This
PR is the wavelength half of that work.

See each commit message for a detailed description w.r.t the incremental
changes.

Depends on go-wasmsqlite#9

This compiles and passes make unit against the currently pinned
go-wasmsqlite, since the changes here are DSN values rather than new symbols.
But vfs=nodefs and require_persistent mean nothing to the pinned driver, so
this needs a go.mod bump once go-wasmsqlite#9 lands before it does
anything at runtime. Happy to land that as a follow-up commit here, or fold it
in once that PR merges, whichever you prefer.

Host detection in one place

internal/wasmhost answers "which host is this?" rather than each DSN builder
deciding for itself, because more than one package needs the same answer and
the daemon's databases must not disagree about where they live. It checks for a
Node version string and for the absence of a Worker constructor together,
since a bundler may define a process shim in a browser and neither signal
alone is proof.

Both DSN builders needed the change, not just the one in db. lwwallet has
its own, and it is the wallet's key store, so a wallet whose waved.db landed
on disk while wallet.db went to OPFS-that-is-not-there would be the worst of
both.

Two things follow from having a real filesystem. The database path is used as
configured instead of being hashed into an origin-local name, since that
hashing exists to keep browser origins from colliding and here it would only
hide where the data went. And every wasm open now sets require_persistent, so
a storage failure surfaces as a startup error rather than as a wallet running
happily against memory it will lose on exit.

"Not implemented under js/wasm" was about the glue, not the platform

ensureDataDir and ensureSwapDBDir were unconditional no-ops on this build,
and the comment explaining why said os.MkdirAll is not implemented under
js/wasm.

That turns out to be a statement about wasm_exec.js. Go's glue installs a
stub filesystem whose calls all fail with ENOSYS only when the host has not
provided a real one. A Node host assigns globalThis.fs from node:fs before
instantiating the module, and os.MkdirAll then reaches the host filesystem
exactly as it would natively. Verified rather than assumed: a js/wasm binary
run under Node creates real nested directories and files on disk.

This matters now because the nodefs VFS expects the data directory to exist,
and because swallowing every failure meant a Node host given an unwritable path
learned about it at its first database open instead of at startup. Both
functions now ask for the directory and read ENOSYS as the browser answering
that it has no filesystem, while any other error is real and is returned.

Why the exclusive locking mode is load-bearing

The last commit is a comment, but it is the one I would most like a second pair
of eyes on.

Both wasm DSN builders ask for journal_mode=WAL and both append
locking_mode=EXCLUSIVE, and the second of those reads like a tidy consequence
of running a single connection. It is not. No wasm VFS implements xShmMap, so
the only WAL available to us is the mode SQLite documents for hosts with no
shared memory, where an EXCLUSIVE connection keeps the WAL index on the heap.
Drop the exclusive lock and WAL is not merely slower, it is unreachable.

That matters more than a lost optimization would, because the durability
settings beside it were chosen for WAL. db/sqlite.go sets
synchronous=normal with a comment reasoning explicitly in WAL terms. Under
WAL that costs the recently committed tail on power loss; under a rollback
journal, SQLite documents the same setting as risking the database. So the
pairing was not a preference, and until go-wasmsqlite#9 we were quietly getting
the wrong half of it.

The driver side of that PR now applies these pragmas before the journal mode
and reads the effective mode back, so a future edit that separates the two
turns into a startup error rather than a wallet running on guarantees nobody
chose for it. The comment here is so the next person to tidy that DSN knows
what they are holding.

Testing

make unit: 23 packages ok, no Go test failures. The js/wasm packages build
clean against the currently pinned dependency, including swapclientserver
under the swapruntime tag.

End to end under Node, twice against one data dir, with the driver side of
go-wasmsqlite#9 in place:

run 1: CREATED wallet, 24 words   identity: 024968...049046
run 2: UNLOCKED existing          identity: 024968...049046
       swaps.db  wallet.db  waved.db   all present, all with -wal

Same identity across a full process restart, and stale locks reclaimed after
run 1 exited without closing.

Not covered here

No Ark round, swap, or payment was driven through a Node-hosted daemon. The
wallet reaches a real chain backend and derives its identity; the settlement
path past the operator handshake is untested on this host.

@gemini-code-assist

Copy link
Copy Markdown

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b407fef027

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread db/sqlite_open_wasm.go
values := url.Values{}
values.Set("file", browserSQLiteFileName(cfg.DatabaseFileName))
values.Set("vfs", wasmSQLiteVFS)
values.Set("vfs", wasmhost.SQLiteVFS())

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Upgrade the driver before selecting nodefs

For every Node-hosted run built from this commit, go.mod still pins go-wasmsqlite at v0.0.0-20260627090804-0dce68fc5287, which predates support for both vfs=nodefs and require_persistent. Consequently these new DSNs cannot provide the promised disk-backed storage: the open may fail or the unsupported persistence guard may be ignored, risking transient wallet and daemon state. The driver update that implements these options must land with this selection.

Useful? React with 👍 / 👎.

Comment thread Makefile
cp $(WASMSQLITE_DIR)/bridge/sqlite-worker.js $(WASM_WALLET_OUT)/
# sqlite-node-vfs.js is loaded only by a Node host, but it ships in every
# bundle so one asset set serves both hosts.
cp $(WASMSQLITE_DIR)/bridge/sqlite-node-vfs.js $(WASM_WALLET_OUT)/

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Include the Node VFS in released runtime bundles

When users obtain the runtime through either tagged release channel, this newly copied asset is discarded: the archive command in .github/workflows/mobile-bindings.yml lines 189–197 and the explicit upload list in scripts/publish-wasm-assets.sh lines 35–44 both retain the old browser-only file set. A Node consumer of the release archive or hosted bundle therefore cannot load sqlite-node-vfs.js, so the Node-host support added here is unavailable outside a local make wasm-wallet build; add the asset to both packaging lists (and the corresponding consumer manifest).

Useful? React with 👍 / 👎.

@litbot-9000

litbot-9000 commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Doc drift advisory

This PR moves the js/wasm build from browser-only to browser-or-Node storage, which leaves the per-package docs for db, lwwallet, swapclientserver, and waved stale — none of them mentions the wasm storage path, and waved.ensureDataDir / swapclientserver.ensureSwapDBDir are documented nowhere even though they stopped being no-ops. (cmd/wavewalletdk-wasm was already updated in the PR; internal/wasmhost has no CLAUDE.md, so it is the nightly sweep's job, not this advisory's.)

Proposed doc updates:

diff --git a/db/AGENTS.md b/db/AGENTS.md
index 1a980931..7aae8c52 100644
--- a/db/AGENTS.md
+++ b/db/AGENTS.md
@@ -105,7 +105,8 @@ For field-level detail, use `go doc github.com/lightninglabs/wavelength/db.<Symb
   persistence), `ledger` (interfaces + domain types), `wallet` (domain
   types for boarding sweeps and the pending-intent outbox), `vtxo`
   (VTXO/ancestry domain types), `round` (round-state domain types),
-  `vhtlcrecovery` (recovery-job domain types).
+  `vhtlcrecovery` (recovery-job domain types), `internal/wasmhost` (js/wasm
+  host detection for SQLite VFS selection, `js && wasm` builds only).
 - **Depended on by**: `round`, `vtxo`, `oor`, `wallet` (storage
   interfaces), `waved` (wires DB backends).
 
@@ -140,6 +141,44 @@ For field-level detail, use `go doc github.com/lightninglabs/wavelength/db.<Symb
   jobs store their registered kind plus the domain-owned durable ref
   needed to reconstruct the same spend policy after restart.
 
+### js/wasm SQLite backend
+
+`openSQLiteDatabase` has one implementation per build
+(`sqlite_open_native.go` / `sqlite_open_wasm.go`). The wasm one opens the
+`wasmsqlite` driver and must satisfy two hosts, so the DSN it builds is
+where the browser/Node differences are resolved:
+
+- The VFS name comes from `wasmhost.SQLiteVFS()` — `opfs` in a browser,
+  `nodefs` under Node. Both are durable, so an open that asks for one and
+  cannot have it fails instead of falling back to memory.
+- `require_persistent=true` is always set. The daemon's databases are the
+  only record of VTXO, swap, and round state and there is no server to
+  re-fetch them from, so a storage failure has to be a startup error
+  rather than a wallet that looks healthy and forgets everything on exit.
+- A browser host maps `DatabaseFileName` through
+  `browserSQLiteFileName`, a hashed origin-local name that keeps
+  same-basename databases (regtest and signet `client.db`, two
+  `swaps.db`) from colliding within one origin. A Node host has a real
+  filesystem and keeps the configured path as given.
+- `journal_mode` and `busy_timeout` travel as their own DSN keys rather
+  than in the `pragma` list, because the driver applies the journal mode
+  last and then reads the effective mode back.
+- `locking_mode=EXCLUSIVE` is always appended and is **not** an
+  optimization for a single-connection handle. Neither wasm VFS
+  implements `xShmMap`, so the only reachable WAL is the mode SQLite
+  documents for hosts without shared memory, which keeps the WAL index on
+  the heap and requires an already-exclusive connection. The driver
+  hoists the locking mode ahead of the journal mode for that reason and
+  verifies the mode it ended up in, so a regression surfaces as a startup
+  error instead of a database quietly running on the rollback journal
+  while `synchronous` was chosen for WAL.
+- `fullfsync` is dropped: it asks Darwin for a stronger barrier than
+  `fsync`, and neither wasm VFS can express it (OPFS has no such concept,
+  and the `node:fs` VFS already issues a full fsync on every `xSync`).
+- The handle is pinned to one connection (`SetMaxOpenConns(1)` /
+  `SetMaxIdleConns(1)`); multiple SQL connections would race the same
+  database through one worker.
+
 ### Migration baseline
 
 The migration history was squashed to a domain-grouped baseline ahead of
diff --git a/db/CLAUDE.md b/db/CLAUDE.md
index 1a980931..7aae8c52 100644
--- a/db/CLAUDE.md
+++ b/db/CLAUDE.md
@@ -105,7 +105,8 @@ For field-level detail, use `go doc github.com/lightninglabs/wavelength/db.<Symb
   persistence), `ledger` (interfaces + domain types), `wallet` (domain
   types for boarding sweeps and the pending-intent outbox), `vtxo`
   (VTXO/ancestry domain types), `round` (round-state domain types),
-  `vhtlcrecovery` (recovery-job domain types).
+  `vhtlcrecovery` (recovery-job domain types), `internal/wasmhost` (js/wasm
+  host detection for SQLite VFS selection, `js && wasm` builds only).
 - **Depended on by**: `round`, `vtxo`, `oor`, `wallet` (storage
   interfaces), `waved` (wires DB backends).
 
@@ -140,6 +141,44 @@ For field-level detail, use `go doc github.com/lightninglabs/wavelength/db.<Symb
   jobs store their registered kind plus the domain-owned durable ref
   needed to reconstruct the same spend policy after restart.
 
+### js/wasm SQLite backend
+
+`openSQLiteDatabase` has one implementation per build
+(`sqlite_open_native.go` / `sqlite_open_wasm.go`). The wasm one opens the
+`wasmsqlite` driver and must satisfy two hosts, so the DSN it builds is
+where the browser/Node differences are resolved:
+
+- The VFS name comes from `wasmhost.SQLiteVFS()` — `opfs` in a browser,
+  `nodefs` under Node. Both are durable, so an open that asks for one and
+  cannot have it fails instead of falling back to memory.
+- `require_persistent=true` is always set. The daemon's databases are the
+  only record of VTXO, swap, and round state and there is no server to
+  re-fetch them from, so a storage failure has to be a startup error
+  rather than a wallet that looks healthy and forgets everything on exit.
+- A browser host maps `DatabaseFileName` through
+  `browserSQLiteFileName`, a hashed origin-local name that keeps
+  same-basename databases (regtest and signet `client.db`, two
+  `swaps.db`) from colliding within one origin. A Node host has a real
+  filesystem and keeps the configured path as given.
+- `journal_mode` and `busy_timeout` travel as their own DSN keys rather
+  than in the `pragma` list, because the driver applies the journal mode
+  last and then reads the effective mode back.
+- `locking_mode=EXCLUSIVE` is always appended and is **not** an
+  optimization for a single-connection handle. Neither wasm VFS
+  implements `xShmMap`, so the only reachable WAL is the mode SQLite
+  documents for hosts without shared memory, which keeps the WAL index on
+  the heap and requires an already-exclusive connection. The driver
+  hoists the locking mode ahead of the journal mode for that reason and
+  verifies the mode it ended up in, so a regression surfaces as a startup
+  error instead of a database quietly running on the rollback journal
+  while `synchronous` was chosen for WAL.
+- `fullfsync` is dropped: it asks Darwin for a stronger barrier than
+  `fsync`, and neither wasm VFS can express it (OPFS has no such concept,
+  and the `node:fs` VFS already issues a full fsync on every `xSync`).
+- The handle is pinned to one connection (`SetMaxOpenConns(1)` /
+  `SetMaxIdleConns(1)`); multiple SQL connections would race the same
+  database through one worker.
+
 ### Migration baseline
 
 The migration history was squashed to a domain-grouped baseline ahead of
diff --git a/lwwallet/AGENTS.md b/lwwallet/AGENTS.md
index 3f4f8b4b..62ebf1a5 100644
--- a/lwwallet/AGENTS.md
+++ b/lwwallet/AGENTS.md
@@ -74,7 +74,9 @@ base logic with the neutrino-backed `btcwbackend` sibling via the extracted
 - **Depends on**: `walletcore` (shared HD key mgmt, signing, boarding base —
   also used by `btcwbackend`), `chainsource` (implements `ChainBackend`),
   `wallet` (implements `BoardingBackend`), `chainbackends` (typed
-  `PackageTxError` for package-relay results).
+  `PackageTxError` for package-relay results), `internal/sqlbase` +
+  `internal/wasmhost` (js/wasm wallet store DSN and host detection,
+  `js && wasm` builds only).
 - **Depended on by**: `waved` (alternative to LND-backed wallet), `sdk`
   (embedded-wallet config references).
 
@@ -119,6 +121,32 @@ base logic with the neutrino-backed `btcwbackend` sibling via the extracted
 - `Stop()` explicitly closes btcwallet's internal database to prevent resource
   leaks.
 
+### js/wasm wallet store
+
+`newWalletLoaderOptions` is build-tagged (`walletdb_native.go` /
+`walletdb_wasm.go`). Under `js/wasm` btcwallet's SQL `walletdb` is opened
+through the go-wasmsqlite driver rather than a native file, and the DSN
+built by `wasmWalletDBDSN` resolves the browser/Node split:
+
+- The VFS name comes from `wasmhost.SQLiteVFS()` (`opfs` or `nodefs`). A
+  browser host maps `dbDir` to the hashed origin-local
+  `wasmWalletDBFileNamePattern` (`/wallet-%016x.db`) via
+  `wasmWalletDBFileName`; a Node host writes `nodeWalletDBFileName`
+  (`wallet.db`) inside the real `dbDir`, beside the daemon's own
+  databases, since the containing directory already distinguishes one
+  wallet from another.
+- `require_persistent=true` is always set. The seed and key state have no
+  second copy anywhere, so an in-memory substitute for the store would be
+  worse than not starting.
+- `journal_mode=WAL` travels as its own DSN key (only that route is
+  checked against the mode SQLite actually reached), and WAL survives
+  only because of `locking_mode=EXCLUSIVE`: no wasm VFS implements
+  `xShmMap`, so this is the heap-WAL-index mode, which requires an
+  already-exclusive connection. Dropping the exclusive lock fails the
+  open rather than silently moving the wallet onto the rollback journal.
+- The handle stays single-connection (`wasmWalletDBMaxConnections = 1`)
+  with `busy_timeout` of `wasmWalletDBBusyTimeoutMS` (30 000 ms).
+
 ## Deep Docs
 
 - [ARCHITECTURE.md](../ARCHITECTURE.md) — System-wide package map.
diff --git a/lwwallet/CLAUDE.md b/lwwallet/CLAUDE.md
index 3f4f8b4b..62ebf1a5 100644
--- a/lwwallet/CLAUDE.md
+++ b/lwwallet/CLAUDE.md
@@ -74,7 +74,9 @@ base logic with the neutrino-backed `btcwbackend` sibling via the extracted
 - **Depends on**: `walletcore` (shared HD key mgmt, signing, boarding base —
   also used by `btcwbackend`), `chainsource` (implements `ChainBackend`),
   `wallet` (implements `BoardingBackend`), `chainbackends` (typed
-  `PackageTxError` for package-relay results).
+  `PackageTxError` for package-relay results), `internal/sqlbase` +
+  `internal/wasmhost` (js/wasm wallet store DSN and host detection,
+  `js && wasm` builds only).
 - **Depended on by**: `waved` (alternative to LND-backed wallet), `sdk`
   (embedded-wallet config references).
 
@@ -119,6 +121,32 @@ base logic with the neutrino-backed `btcwbackend` sibling via the extracted
 - `Stop()` explicitly closes btcwallet's internal database to prevent resource
   leaks.
 
+### js/wasm wallet store
+
+`newWalletLoaderOptions` is build-tagged (`walletdb_native.go` /
+`walletdb_wasm.go`). Under `js/wasm` btcwallet's SQL `walletdb` is opened
+through the go-wasmsqlite driver rather than a native file, and the DSN
+built by `wasmWalletDBDSN` resolves the browser/Node split:
+
+- The VFS name comes from `wasmhost.SQLiteVFS()` (`opfs` or `nodefs`). A
+  browser host maps `dbDir` to the hashed origin-local
+  `wasmWalletDBFileNamePattern` (`/wallet-%016x.db`) via
+  `wasmWalletDBFileName`; a Node host writes `nodeWalletDBFileName`
+  (`wallet.db`) inside the real `dbDir`, beside the daemon's own
+  databases, since the containing directory already distinguishes one
+  wallet from another.
+- `require_persistent=true` is always set. The seed and key state have no
+  second copy anywhere, so an in-memory substitute for the store would be
+  worse than not starting.
+- `journal_mode=WAL` travels as its own DSN key (only that route is
+  checked against the mode SQLite actually reached), and WAL survives
+  only because of `locking_mode=EXCLUSIVE`: no wasm VFS implements
+  `xShmMap`, so this is the heap-WAL-index mode, which requires an
+  already-exclusive connection. Dropping the exclusive lock fails the
+  open rather than silently moving the wallet onto the rollback journal.
+- The handle stays single-connection (`wasmWalletDBMaxConnections = 1`)
+  with `busy_timeout` of `wasmWalletDBBusyTimeoutMS` (30 000 ms).
+
 ## Deep Docs
 
 - [ARCHITECTURE.md](../ARCHITECTURE.md) — System-wide package map.
diff --git a/swapclientserver/AGENTS.md b/swapclientserver/AGENTS.md
index f615a03d..f8b06197 100644
--- a/swapclientserver/AGENTS.md
+++ b/swapclientserver/AGENTS.md
@@ -112,6 +112,15 @@ protocol behavior remain entirely inside `sdk/swaps` and `swapdk-server`.
   receiver was previously installed. `Register` therefore installs the
   mailbox receiver immediately after `NewSwapClientWithStore`, before
   `resumePending` revives persisted sessions.
+- `ensureSwapDBDir` is build-tagged (`fs_native.go` / `fs_wasm.go`) and
+  prepares the directory holding the swap database before `Register` opens it.
+  The `js/wasm` build is no longer an unconditional no-op: it calls
+  `os.MkdirAll` and treats only `syscall.ENOSYS` as success, which is how a
+  browser reports that `wasm_exec.js` installed a stub filesystem. Every other
+  error is returned, so a Node host (which assigns `globalThis.fs` from
+  `node:fs`) given an unwritable path fails here rather than at the first
+  database open. See `waved.ensureDataDir` for the same contract on the
+  daemon's data directory.
 
 ## Deep Docs
 
diff --git a/swapclientserver/CLAUDE.md b/swapclientserver/CLAUDE.md
index f615a03d..f8b06197 100644
--- a/swapclientserver/CLAUDE.md
+++ b/swapclientserver/CLAUDE.md
@@ -112,6 +112,15 @@ protocol behavior remain entirely inside `sdk/swaps` and `swapdk-server`.
   receiver was previously installed. `Register` therefore installs the
   mailbox receiver immediately after `NewSwapClientWithStore`, before
   `resumePending` revives persisted sessions.
+- `ensureSwapDBDir` is build-tagged (`fs_native.go` / `fs_wasm.go`) and
+  prepares the directory holding the swap database before `Register` opens it.
+  The `js/wasm` build is no longer an unconditional no-op: it calls
+  `os.MkdirAll` and treats only `syscall.ENOSYS` as success, which is how a
+  browser reports that `wasm_exec.js` installed a stub filesystem. Every other
+  error is returned, so a Node host (which assigns `globalThis.fs` from
+  `node:fs`) given an unwritable path fails here rather than at the first
+  database open. See `waved.ensureDataDir` for the same contract on the
+  daemon's data directory.
 
 ## Deep Docs
 
diff --git a/waved/AGENTS.md b/waved/AGENTS.md
index b778f21c..c91aaad3 100644
--- a/waved/AGENTS.md
+++ b/waved/AGENTS.md
@@ -143,6 +143,19 @@ For field-level detail, use `go doc github.com/lightninglabs/wavelength/waved.<S
   window boundary only when the local dynamic critical threshold plus retry
   buffer remains intact. When that cached boundary fires, it fetches a fresh
   `GetInfo` snapshot and rechecks the window before reserving the input.
+- `ensureDataDir` is build-tagged (`fs_native.go` / `fs_wasm.go`) and creates
+  `Config.NetworkDir()` at the top of `initDatabase`. The `js/wasm` build is no
+  longer an unconditional no-op: it calls `os.MkdirAll(dir, 0o700)` and treats
+  only `syscall.ENOSYS` as success. Rather than sniffing the host, that reads
+  ENOSYS as a browser answering that `wasm_exec.js` left it with a stub
+  filesystem and persistent state lives in OPFS instead; a Node host assigns
+  `globalThis.fs` from `node:fs` before instantiating the module, so
+  `os.MkdirAll` reaches the real filesystem exactly as it would natively and
+  creates the directory the `node:fs` SQLite VFS expects to find. Every other
+  error is returned, so a Node host given an unwritable path fails at startup
+  instead of at the first database open. This is why `cmd/wavewalletdk-wasm`
+  refuses to inject its browser default `data_dir` (`/wavelength`) under Node:
+  that path would ask the host to `mkdir` at the filesystem root.
 
 ## Deep Docs
 
diff --git a/waved/CLAUDE.md b/waved/CLAUDE.md
index b778f21c..c91aaad3 100644
--- a/waved/CLAUDE.md
+++ b/waved/CLAUDE.md
@@ -143,6 +143,19 @@ For field-level detail, use `go doc github.com/lightninglabs/wavelength/waved.<S
   window boundary only when the local dynamic critical threshold plus retry
   buffer remains intact. When that cached boundary fires, it fetches a fresh
   `GetInfo` snapshot and rechecks the window before reserving the input.
+- `ensureDataDir` is build-tagged (`fs_native.go` / `fs_wasm.go`) and creates
+  `Config.NetworkDir()` at the top of `initDatabase`. The `js/wasm` build is no
+  longer an unconditional no-op: it calls `os.MkdirAll(dir, 0o700)` and treats
+  only `syscall.ENOSYS` as success. Rather than sniffing the host, that reads
+  ENOSYS as a browser answering that `wasm_exec.js` left it with a stub
+  filesystem and persistent state lives in OPFS instead; a Node host assigns
+  `globalThis.fs` from `node:fs` before instantiating the module, so
+  `os.MkdirAll` reaches the real filesystem exactly as it would natively and
+  creates the directory the `node:fs` SQLite VFS expects to find. Every other
+  error is returned, so a Node host given an unwritable path fails at startup
+  instead of at the first database open. This is why `cmd/wavewalletdk-wasm`
+  refuses to inject its browser default `data_dir` (`/wavelength`) under Node:
+  that path would ask the host to `mkdir` at the filesystem root.
 
 ## Deep Docs

How to apply: save the block above to a file and git apply it, or run the doc-gardening skill locally (/doc-gardening db lwwallet swapclientserver waved) and let it regenerate the same edits. Each CLAUDE.md must stay byte-identical to its sibling AGENTS.md, which is why both appear in the diff.

make doc-check passes on this diff. (The only error it reports on this runner is a pre-existing ./.claude-pr/CLAUDE.md missing its AGENTS.md, an untracked CI-harness directory unrelated to these changes.)


This check is advisory and does not fail CI. Run: https://github.com/lightninglabs/wavelength/actions/workflows/doc-gardening-pr.yml

@litbot-9000 litbot-9000 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

FINAL — Reviewed at b407fef in a clean worktree. No prior interim state existed for this PR, so everything below is from this run.

The premise is right, and I confirmed it is a real fix. The pinned driver's worker (bridge/sqlite-worker.js:392-397) silently falls back to :memory: when vfs=opfs is requested and unavailable — and only when require_persistent is unset. The pre-PR code asked for vfs=opfs unconditionally with no require_persistent, so a Node-hosted wallet would have opened a memory database and lost its keys on exit. Adding require_persistent=true closes exactly that hole, and I verified it is plumbed end to end (open.go:198 parse → bridge_adapter.go:53 jsOpts.Set("requirePersistent", ...)sqlite-worker.js:355 throw). That is the strongest part of this PR.

Host detection is sound and fails closed in both directions. I built the real internal/wasmhost into a js/wasm probe and ran it under Node v24 (UnderNode()=true, SQLiteVFS()=nodefs) and under a browser-simulating harness that leaves globalThis.fs unset so wasm_exec.js installs its ENOSYS stub and defines Worker (UnderNode()=false, SQLiteVFS()=opfs). Node 24 has no global Worker and a string process.versions.node, so the two-signal test works. On a wrong answer: a browser misdetected as Node asks for nodefs, which no browser has → hard open error; a Node-like host misdetected as a browser (Worker polyfill, Electron renderer, Bun — which sets process.versions.node and a global Worker) asks for opfs, which is unavailable there and is saved from the :memory: fallback only by the new require_persistent. Neither direction can silently open a non-durable database. Worth saying plainly: the detection is heuristic and Bun/Electron will land on the browser branch, but the consequence is a refusal to start, not a lost wallet.

The ENOSYS reading is correct, and I checked it rather than assuming. Go 1.26's lib/wasm/wasm_exec.js:46 stubs mkdir with callback(enosys()), and syscall/tables_js.go:403 maps the "ENOSYS" code to syscall.ENOSYS, which errors.Is reaches through MkdirAll's *PathError. Empirically under Node the probe created a nested directory, and returned EACCES for an unwritable parent and ENOENT for a nonexistent one — both propagated, neither swallowed. Under the browser stub it returned nil. The ENOSYS branch does not mask real failures.

No third DSN builder. grep for Set("vfs"/opfs/nodefs/require_persistent across the tree finds only the two builders this PR touches; db/migrate/sqlite_wasm_driver.go and internal/sqlbase consume a handle/DSN rather than building one. Both builders now route through wasmhost, so waved.db and wallet.db cannot disagree. The unhashed-path change is new-host-only — the browser else branches are byte-identical to the prior behavior, so there is no migration concern and no collision risk (under Node the paths are real and already unique; DBDir is NetworkDir(), which initDatabase creates at waved/server.go:3565 before the wallet opens at server.go:1797).

Blocking issues (inline): make wasm-wallet is broken at the pinned dep, and vfs=nodefs means every Node run dies at the first open until the go.mod bump.

Merge order, in one sentence: this PR must not merge before the go-wasmsqlite bump — with the pinned driver make wasm-wallet fails outright and every Node-hosted start fails at the first database open, though it fails loudly rather than falling back to memory, so nothing can lose funds.

CI triage — I pulled annotations and full logs for all three; none show the ARC "lost communication" shape (all three are plain exit code 2 with no null-conclusion steps):

  • Commit Messagereal, author's. Reproduced locally: b407fef (db: record why exclusive locking is what buys us WAL) has nine body lines over 72 chars (L3-L7, L13-L15, L17). Fix with python3 scripts/commit_message.py reword --commit b407fef.
  • Static Checksreal, author's. make fmt-changed-check (Makefile:327), reproduced locally: llformat wants rewraps in db/sqlite_open_wasm.go, lwwallet/walletdb_wasm.go, and a blank line removed after the wrapped if in internal/wasmhost/host.go:33. Note the mechanical rewrap produces "WAL-without- shared-memory" with a stray space — please reword the sentence rather than commit that artifact.
  • Run unit tests (unit tags="test_postgres")infrastructure, not this PR. The failure is TestDurableAskErrorMessagePreserved in internal/actortest, with database is in a dirty state at version 15 and relation "ledger_entries" does not exist from the Postgres fixture. Every Go file this PR touches is behind //go:build js && wasm, so no linux/amd64 test binary contains any of this code.

Builds and tests I ran: GOOS=js GOARCH=wasm go build -tags="mobile wavewalletrpc swapruntime" ./cmd/wavewalletdk-wasm passes; GOOS=js GOARCH=wasm go vet over the touched packages is clean apart from a pre-existing swapclientserver/service.go:451 context-leak warning in a file this PR does not touch. go test ./lwwallet/... and go test -tags=swapruntime ./swapclientserver/... pass.

Non-blocking notes I could not anchor to a diff line:

  • cmd/wavewalletdk-wasm/main.go:275 is now stale: it justifies the /wavelength default with "(see waved.ensureDataDir, a no-op under js/wasm)". This PR makes it not a no-op. See the inline comment on waved/fs_wasm.go for why that matters beyond the wording.
  • internal/wasmhost ships with no tests. It is js && wasm-only so this is awkward, but UnderNode's two-signal logic is the kind of thing worth a table test run under a GOOS=js exec wrapper, especially given the Bun/Electron cases.

Comment thread Makefile
cp $(WASMSQLITE_DIR)/bridge/sqlite-worker.js $(WASM_WALLET_OUT)/
# sqlite-node-vfs.js is loaded only by a Node host, but it ships in every
# bundle so one asset set serves both hosts.
cp $(WASMSQLITE_DIR)/bridge/sqlite-node-vfs.js $(WASM_WALLET_OUT)/

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🔴 blocker — This cp breaks make wasm-wallet today. bridge/sqlite-node-vfs.js does not exist in the pinned go-wasmsqlite (v0.0.0-20260627090804-0dce68fc5287); that module's bridge/ contains only sqlite-bridge.js and sqlite-worker.js. cp exits non-zero and takes the target down, and wasm-publish runs wasm-wallet first, so the release path goes with it.

Separately — and this one survives the go.mod bump — scripts/publish-wasm-assets.sh carries an explicit FILES=(...) allowlist, and uploads exactly that set on purpose ("so we publish exactly the manifest set and never leak stray files that a previous build may have left in the asset dir"). sqlite-node-vfs.js is not in it, so even once the copy works the published bundle will not contain the Node VFS — and neither the pre-flight existence check nor the post-upload verification notices, because both only iterate the files already listed. The comment here says the asset "ships in every bundle"; for the bundle that actually reaches consumers, it does not. So the codex thread on this is correct.

Add sqlite-node-vfs.js to FILES in scripts/publish-wasm-assets.sh as well. That script's header says the array must stay in sync with RUNTIME_ASSET_FILES in packages/web/src/runtime-manifest.ts and the FILES array in apps/web-wallet-demo/scripts/fetch-runtime-assets.sh, so this needs a companion wavelength-sdk change to land with it.

Comment thread internal/wasmhost/host.go
// fails rather than quietly running against memory.
func SQLiteVFS() string {
if UnderNode() {
return "nodefs"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🔴 blockernodefs does not exist in the pinned driver, so this makes every Node run fail at the first database open until the go.mod bump lands. I traced it rather than guessing:

  • grep -rn nodefs over go list -m -f '{{.Dir}}' github.com/lightninglabs/go-wasmsqlite returns nothing at all.
  • bridge/sqlite-worker.js:59 normalizeRequestedVFS passes an unrecognized name straight through its default branch.
  • open() (sqlite-worker.js:369) therefore builds candidates = ["nodefs"], and openPersistentDB throws at lines 328-330: requested SQLite VFS is unavailable: nodefs.
  • Because requestedVFS !== "auto" and requestedVFS !== "opfs", the memory-fallback branch at 392-397 is skipped and the error rethrows.

The important part is the direction of failure: this is a hard open error, not a silent memory-backed database. So the codex P1 is right that the driver must be upgraded first, but it is a broken-startup bug, not a fund-safety one.

Merge-order consequence, plainly: this PR must not merge before the go-wasmsqlite bump, because with the currently pinned driver every Node-hosted start dies at the first open. The browser path is unaffected either way.

Comment thread waved/fs_wasm.go Outdated
return nil
}

err := os.MkdirAll(dir, 0700)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🟠 major — Making this real turns the wasm entrypoint's own default data dir into a hard startup failure under Node, and nothing in this PR updates that default.

cmd/wavewalletdk-wasm/main.go injects browserDataDir = "/wavelength" in startConfig whenever the caller does not set data_dir. Config.NetworkDir() (waved/config.go:1591) makes that /wavelength/data/<network>, and waved/server.go:3565 hands it straight to this function. Under Node that is now a real os.MkdirAll at the filesystem root: I ran it and got EROFS in this container, and it would be EACCES for any ordinary non-root Node process. Neither is ENOSYS, so it propagates and initDatabase fails with "unable to create data dir" before the daemon ever boots.

Failing loudly is the right instinct — I am not asking you to swallow it. The problem is that the default a Node embedder gets by not configuring anything is now guaranteed to fail. Either pick a host-appropriate default under Node (cwd-relative, os.TempDir(), or $HOME-based) or make startConfig refuse to inject /wavelength when wasmhost.UnderNode(), so the caller gets an actionable "set data_dir" instead of an EROFS from mkdir.

While you are here: the comment block at cmd/wavewalletdk-wasm/main.go:275 justifies that /wavelength default with "(see waved.ensureDataDir, a no-op under js/wasm)". This PR is what falsifies that parenthetical, so it should be updated in the same change.

Nit: this uses 0700 while the sibling swapclientserver/fs_wasm.go:23 added in the same PR uses 0o700. Pick one.

Comment thread db/sqlite_open_wasm.go Outdated
// implements xShmMap, so the only WAL available is the mode SQLite
// documents for hosts without shared memory, where an EXCLUSIVE
// connection keeps the WAL index on the heap. The driver applies these
// pragmas before the journal mode for exactly that reason, and then

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🟠 major — This comment states the opposite of what the pinned driver does, and it is the load-bearing claim of the commit that added it (db: record why exclusive locking is what buys us WAL).

bridge/sqlite-worker.js:413-419 runs:

if (args.journalMode) {
  opened.db.exec(`PRAGMA journal_mode=${args.journalMode}`);
}
for (const pragma of args.pragma || []) {
  if (pragma) opened.db.exec(`PRAGMA ${pragma}`);
}

The journal mode is applied first, and locking_mode=EXCLUSIVE — which travels in the pragma list built on line 76 — is applied after. That is precisely the inverted order, and the order is the whole point: SQLite's WAL-without-shared-memory mode only works if locking_mode=EXCLUSIVE is set prior to the first WAL-mode database access.

There is also no read-back of the effective journal mode anywhere in the module. grep -n journal over the whole pinned module hits only sqlite-worker.js:415 (the exec above), bridge_adapter.go:52 (sets the option), and open.go:127,210 (DSN encode/parse). Nothing checks what mode the connection actually ended up in.

So the safety net described here does not exist: a regression that drops the exclusive lock cannot "surface as a startup error", because nothing is looking. Depending on what the VFS returns for the unimplemented xShmMap, PRAGMA journal_mode=WAL either throws (loud, fine) or returns the unchanged prior mode (the silent rollback-journal case this comment promises is impossible) — and the code cannot tell those apart. That matters because synchronous is being chosen on the assumption WAL is in effect.

The same claim appears twice more: lines 48-51 above ("it applies it last and then reads the effective mode back") and lwwallet/walletdb_wasm.go:131-137. If go-wasmsqlite#9 adds the reordering and the read-back, then all three are forward references to an unreleased driver — one more reason the bump has to land first. Either way, please don't land a comment that tells the next on-call engineer a guarantee is in place when it isn't.

Comment thread lwwallet/walletdb_wasm.go Outdated
// below. No wasm VFS implements xShmMap, so this is SQLite's WAL-without-
// shared-memory mode, which holds the WAL index on the heap and requires
// the connection to already be exclusive. The driver applies the pragmas
// first and then checks the journal mode it actually got, so dropping the

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🟠 major — Same incorrect claim as db/sqlite_open_wasm.go:72 — see there for the full trace. In short: the pinned driver applies journal_mode before the pragma list (bridge/sqlite-worker.js:413-419), not after, so the locking_mode=EXCLUSIVE set on line 150 lands too late to be what enables WAL-without-shm; and nothing anywhere in the module reads the effective journal mode back. "Dropping the exclusive lock would fail the open rather than silently move the wallet onto the rollback journal" is therefore backwards — silently moving the wallet onto the rollback journal is exactly what nothing here would catch.

@Roasbeef
Roasbeef force-pushed the nodejs-storage-backend branch from b407fef to 038f444 Compare August 11, 2026 03:18
The three commits that follow ask the driver for things the pinned
version does not have: vfs=nodefs, require_persistent, and the
sqlite-node-vfs.js asset the wasm bundle copies. Without this bump a
Node-hosted start dies at the first database open with "requested SQLite
VFS is unavailable: nodefs", and make wasm-wallet fails outright on the
missing asset, so the bump goes first rather than alongside.

It also brings the pragma ordering these commits document. The driver
now hoists locking_mode ahead of the journal mode and reads the
effective journal mode back, refusing a mode SQLite could not honor
instead of running quietly on a weaker one. That is a real behavior
change for the browser too: a caller asking for WAL with an exclusive
lock was silently getting the rollback journal, and now genuinely gets
WAL. Worth knowing before rollback: a database this driver converts to
WAL cannot be reopened by an older build.
In this commit, we let the js/wasm build store its databases wherever
the host can actually keep them. The DSN builders hardcoded vfs=opfs,
which is correct for the only host they were written for and wrong for
the other one we now care about: a Node process, which has no OPFS at
all but does have a real filesystem, reachable through the go-wasmsqlite
nodefs VFS.

The host detection lives in internal/wasmhost rather than in each DSN
builder, because more than one package needs the same answer and the
daemon's databases must not disagree about where they live. It checks
for a Node version string and for the absence of a Worker constructor
together, since a bundler may define a process shim in a browser and
neither signal alone is proof.

Two things follow from having a real filesystem. The database path is
used as configured instead of being hashed into an origin-local name,
because that hashing exists to keep browser origins from colliding and
here it would only hide where the data went. And every wasm open now
sets require_persistent, so a storage failure surfaces as a startup
error rather than as a wallet running happily against memory it will
lose on exit.
@Roasbeef
Roasbeef force-pushed the nodejs-storage-backend branch from 038f444 to 021b80c Compare August 11, 2026 03:40
In this commit, we stop treating "no filesystem" as a property of
js/wasm rather than of the host running it. ensureDataDir and
ensureSwapDBDir were unconditional no-ops on this build, and the comment
explaining why said os.MkdirAll is not implemented under js/wasm.

That turns out to be a statement about wasm_exec.js, not about the
platform. Go's glue installs a stub filesystem whose calls all fail with
ENOSYS only when the host has not provided a real one; a Node host
assigns globalThis.fs from node:fs before instantiating the module, and
os.MkdirAll then reaches the host filesystem exactly as it would
natively. Verified rather than assumed: a js/wasm binary run under Node
creates real nested directories and files on disk.

This matters now because the nodefs SQLite VFS expects the data
directory to exist, and because swallowing every failure meant a Node
host given an unwritable path learned about it at its first database
open instead of at startup. So both functions now ask for the directory
and read ENOSYS as the browser answering that it has no filesystem,
while any other error is real and is returned.
In this commit, we write down an invariant that both wasm DSN builders
now depend on and neither states. Both ask SQLite for journal_mode=WAL
and both append locking_mode=EXCLUSIVE, and until recently the second of
those looked like a tidy consequence of running a single connection. It
is not. No wasm VFS implements xShmMap, so the only WAL available to us
is the mode SQLite documents for hosts with no shared memory, where an
EXCLUSIVE connection keeps the WAL index on the heap. Drop the exclusive
lock and WAL is not merely slower, it is unreachable.

That matters more than a lost optimization would, because the durability
settings beside it were chosen for WAL. synchronous=normal under WAL
means losing the recently committed tail on power loss; the same setting
under a rollback journal means risking the database. The driver hoists
the locking mode ahead of the journal mode for exactly that reason, and
then reads the effective mode back, so a future edit that separates the
two turns into a startup error rather than a wallet running quietly on
guarantees nobody chose for it.

Both builders pass the journal mode as its own DSN key rather than in
the pragma list, because that is the route the driver checks.
@Roasbeef
Roasbeef force-pushed the nodejs-storage-backend branch from 021b80c to 76342e6 Compare August 11, 2026 04:06

@litbot-9000 litbot-9000 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

FINAL — Re-review of the four commits at head 76342e64. All five findings from my earlier review are resolved. Nothing here still blocks; the CHANGES_REQUESTED that is holding this PR at BLOCKED should be dismissed. Two small notes below, neither of which needs another round.

Note on the head: the branch was force-pushed again while I was reviewing. 021b80c1 no longer exists; 76342e64 is the same tree plus one commit that syncs cmd/wavewalletdk-wasm/AGENTS.md with CLAUDE.md. Everything below was checked at 76342e64. No prior interim state existed for this run, so all of it is fresh.

Close-out of the five prior findings

  1. blocker Makefile:506 — RESOLVED. bridge/sqlite-node-vfs.js exists in the bumped module. I ran make wasm-wallet end to end: exit 0, and bin/wasm/ contains sqlite-node-vfs.js (27472 bytes) alongside the other seven assets. The publish half is fixed too — sqlite-node-vfs.js is now in the FILES allowlist in scripts/publish-wasm-assets.sh:41 and in the .github/workflows/mobile-bindings.yml:195 upload list, so the asset actually reaches the published bundle rather than only bin/wasm.
  2. blocker internal/wasmhost/host.go:46 (vfs=nodefs) — RESOLVED. nodefs is first-class in the bumped driver: utils.go:25 (VFSTypeNodeFS), driver.go:291, sqlite-worker.js:48,177-188, and the VFS implementation itself in bridge/sqlite-node-vfs.js.
  3. major db/sqlite_open_wasm.go:72 and 4. major lwwallet/walletdb_wasm.go:135 (the inverted comments) — RESOLVED, and the comments are now accurate rather than accidentally right. The merged worker hoists locking mode first and applies journal mode last (sqlite-worker.js:700-714, orderPragmas), and applyJournalMode (sqlite-worker.js:594-618) reads PRAGMA journal_mode back and throws on mismatch. So both claims — "hoists the locking mode ahead of the journal mode" and "reads back the mode it ended up in" — are now true of the pinned driver. The read-back is skipped for non-persistent handles, but require_persistent=true makes that unreachable here.
  4. major waved/fs_wasm.go:31 (the /wavelength default becoming a hard startup dependency) — RESOLVED, and better than I asked for. startConfig now returns an error and walletCall rejects the promise (main.go:54-57, 315-318), so a Node caller that omits data_dir gets errNodeDataDirRequired at start instead of an EROFS from mkdir later. The stale main.go:275 parenthetical is gone, the reasoning is rewritten for both hosts, and CLAUDE.md/AGENTS.md were updated to match. The 0700 vs 0o700 nit is fixed — both files use 0o700.

The integration risks you asked me to trace

  • journal_mode conflict: not reachable. Both DSN builders pass the journal mode only as the journal_mode key, never in the pragma list, so extractJournalMode's new "conflicting journal modes" error cannot fire. The daemon builder routes it there explicitly (db/sqlite_open_wasm.go:44-50, the case "journal_mode" arm, with the pragma list built from the default arm only); lwwallet hardcodes journal_mode=WAL as a key with a pragma list of foreign_keys=on;auto_vacuum=incremental;locking_mode=EXCLUSIVE. I dumped the real DSNs to confirm rather than reading it off the source. There is no third builder — grep for Set("vfs"/opfs/nodefs/require_persistent across the tree hits only these two plus internal/wasmhost.
  • require_persistent parses. Both builders send the literal string "true", which strconv.ParseBool accepts (open.go:211-216).
  • I ran both DSNs through the real merged driver under Node (Node v24.16.0), by compiling ./lwwallet and ./db as GOOS=js GOARCH=wasm test binaries and running them under the driver's own scripts/node-wasm-test-exec.cjs harness. Results:
    • wallet DB: UnderNode()=true, vfs=nodefs, effective journal_mode=wal, effective locking_mode=exclusive, real 8192-byte file on the host filesystem at the configured path.
    • daemon DB, with the exact pragma set NewSqliteStore builds (foreign_keys, journal_mode=WAL, busy_timeout=30000, synchronous=normal, fullfsync=true): opens, effective journal_mode=wal, synchronous=1 (NORMAL), real file on disk.
      That is the whole premise of the PR, executed rather than argued: the Node path opens, is durable, and genuinely lands in WAL. I removed the probe files afterwards.
  • Multi-process: nothing in wavelength needs more than the lock provides. The daemon runs one process with one handle per file (SetMaxOpenConns(1)), the three databases are distinct files, and no wavelength comment claims cross-process access. The driver's own limits (writer-only, one pid namespace, local filesystems — sqlite-node-vfs.js:18-38) are documented there and are not contradicted by anything here.
  • The fullfsync comment is correct, which I checked because it is a new durability claim: sqlite-node-vfs.js:543-565 always calls fsyncSync and deliberately does not degrade to fdatasync on the DATAONLY hint, and additionally syncs the parent directory on first sync of a created file.

CI

  • Both failures that were genuinely the author's are now fixed. I reproduced the fixes locally: make commitmsg-lint range="origin/main..HEAD" passes on all four commits, and make fmt-changed-check is clean. make lint-changed-local reports 0 issues.
  • Scoped doc drift advisory is not this PR's to fix. Run 31456084307's log ends with "result": "You've hit your session limit · resets 4am (UTC)" — the advisory bot ran out of its own quota. The job is also explicitly non-blocking by design (.github/workflows/doc-gardening-pr.yml:6-10: "It never pushes, never opens a PR, and never fails the build") and is not a required check. Separately, the author has already brought CLAUDE.md and AGENTS.md in line with the change, and the two files are byte-identical.

Builds and tests I ran

make wasm-wallet (exit 0, full asset set). GOOS=js GOARCH=wasm go build over ./db/... ./lwwallet/... ./internal/wasmhost/... ./waved/..., over ./swapclientserver/... with -tags=swapruntime, and over ./cmd/wavewalletdk-wasm with -tags="mobile wavewalletrpc swapruntime" — all clean. GOOS=js GOARCH=wasm go vet over the same set: only the two pre-existing findings (swapclientserver/service.go:451 context leak, in a file this PR does not touch, and a test-only modernc.org/libc build-constraint failure in db/actordelivery). go test ./db/... ./lwwallet/... ./waved/... ./internal/... and go test -tags=swapruntime ./swapclientserver/... all pass.

What I could not verify

I could not run a browser in this container (no chromium system libraries, no root), so the one thing I could not execute is the OPFS side of the new journal-mode read-back. See the inline note on db/sqlite_open_wasm.go — it is a five-minute check for someone with a browser, not a reason to hold the PR.

internal/wasmhost still ships with no tests. It is js && wasm-only so this is awkward, and the driver ships a go test -exec harness that would make it runnable, but I am not asking for it in this PR.

Comment thread db/sqlite_open_wasm.go
}
}

// Exclusive locking is not merely an optimization for a

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🟡 minor — This comment is now accurate — I verified the hoist and the read-back in the merged driver, and measured journal_mode=wal end to end under nodefs. One thing worth confirming before merge, because it lands on the browser path rather than the new Node one.

The bump changes browser behaviour, not just Node behaviour. The driver's own CHANGELOG says the old pragma ordering "made journal_mode=WAL unreachable on every VFS this driver offers" and that "a WAL request silently produced a rollback journal". So today, on the pinned-before-this-PR driver, the shipping browser wallet is running on the rollback journal while this code asks for WAL. After the bump, applyJournalMode (bridge/sqlite-worker.js:594-618) reads the effective mode back and throws on mismatch. Two outcomes, and they are very different:

  • OPFS + locking_mode=EXCLUSIVE can deliver WAL-without-shm → the browser quietly gets the mode it has been asking for all along. Good.
  • It cannot → every browser open now throws journal_mode=WAL was requested but SQLite is in "delete" mode, and the browser wallet stops starting.

The analysis points at the first outcome: SQLite passes bNoShm = pPager->exclusiveMode into sqlite3WalOpen, so under EXCLUSIVE locking the wal-index is on the heap and the VFS's shm methods are never called at all — which is exactly why it works under nodefs, and nothing in that path is OPFS-specific. But I could not run it: this container has no browser (missing libglib-2.0, no root to install it), and I could not find the assertion anywhere in the merged module either — browser_e2e_test.go, driver_browser_js_wasm_test.go and tests/example-opfs-wl.spec.js contain no journal/WAL/locking assertions, while the Node suite covers it thoroughly (tests/node-backend.test.cjs:475, :508, :544).

So the one VFS whose WAL behaviour is untested is the one that is already in production. Please open the wasm wallet in a real browser once against this bump and confirm the database opens and reports wal — or add the OPFS counterpart of tests/node-backend.test.cjs:475 upstream. If it does open, it is worth saying in this comment that the browser was silently on the rollback journal before this bump, since that also means synchronous=normal meant something weaker there than this code assumed.

Comment thread lwwallet/walletdb_wasm.go
// wallet onto the rollback journal.
//
// The journal mode travels as its own DSN key rather than in the pragma
// list, because only that route is checked against the mode SQLite

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit — "only that route is checked" is not true of the driver you just pinned. extractJournalMode (bridge/sqlite-worker.js:534-565) deliberately pulls a journal_mode= entry out of the pragma list and routes it through the same checked applyJournalMode path, precisely so that a caller who writes it where every other pragma goes does not get the silent downgrade. Its own comment says as much: "Routing it to the checked path is kinder than rejecting it and gives the same answer."

Nothing breaks — passing it as the DSN key is still the right thing to do, and the equivalent comment in db/sqlite_open_wasm.go:48-51 states the rationale without the "only" and reads fine. This is just the one sentence in the new comments that a reader could check against the driver and find wrong. Suggest dropping "only", or noting instead that both routes converge on the checked path and the key is the clearer of the two.

(There is a real sharp edge nearby, though it is the dependency's rather than yours: supplying the journal mode as both the key and a pragma with different values is now a hard "conflicting journal modes" error. Neither builder here does that, so it cannot fire.)

@litbot-9000
litbot-9000 dismissed their stale review August 11, 2026 04:28

Superseded: all five findings resolved at 76342e6 and verified end to end (make wasm-wallet, both DSNs opened against the merged driver under Node, WAL confirmed). See the follow-up review.

@Roasbeef
Roasbeef merged commit c7e3497 into main Aug 13, 2026
58 of 62 checks passed
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.

2 participants