fix: add configurable SQLite mmap disable for NFS-backed storage#60349
fix: add configurable SQLite mmap disable for NFS-backed storage#60349AndyMoore wants to merge 8 commits into
Conversation
SQLite's memory-mapped I/O is incompatible with NFS. When database files live on an NFS mount (common in Kubernetes with NFS provisioners), a transient network interruption causes the kernel to deliver SIGBUS (exit code 135) when the process accesses a page no longer backed by the server. WAL mode's shared-memory (-shm) files are particularly affected. Add a configurable option to disable mmap via: - `OPENCLAW_SQLITE_MMAP=0` environment variable (takes precedence) - `storage.sqliteMmap: false` in openclaw.json Default behaviour is unchanged (mmap enabled). The env var is convenient for Kubernetes deployments where it can be set in the pod spec without touching config. Applied to all four SQLite connection sites: - Task registry (runs.sqlite) - Task Flow registry (registry.sqlite) - Memory sync store - QMD manager (read-only index)
openclaw-barnacle
Bot
added
extensions: memory-core
Greptile SummaryThis PR adds
Confidence Score: 4/5Do not merge until the config-argument omission and boundary violation are fixed. The env-var mitigation works today, so the SIGBUS fix provides real value. However the documented All four Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/infra/node-sqlite.ts
Line: 50-57
Comment:
**`storage.sqliteMmap` config option is silently ignored**
All four call sites (`task-registry.store.sqlite.ts:421`, `task-flow-registry.store.sqlite.ts:346`, `manager-sync-ops.ts:98`, `qmd-manager.ts:1873`) call `applySqliteMmapPragma(db)` with only one argument, leaving `config` as `undefined`. Because `isSqliteMmapEnabled` only falls through to `config?.storage?.sqliteMmap` when the env var is unset, the JSON config option `storage.sqliteMmap: false` can never take effect — the PR's documented "no env var" path is broken. Users following the `openclaw.json` approach will still get SIGBUS crashes.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/memory-host-sdk/src/host/sqlite.ts
Line: 21
Comment:
**Package boundary violation — relative import escapes package root**
The line `export { applySqliteMmapPragma } from "../../../../src/infra/node-sqlite.js"` reaches four levels up and into `src/infra/`, which is core-internal code. The CLAUDE.md guardrail is explicit: *"inside a bundled plugin package, do not use relative imports/exports that resolve outside that same package root."* Core logic that a plugin package needs should be exported through a dedicated `src/plugin-sdk/*` subpath and then imported via `openclaw/plugin-sdk/…`, not pulled in with an escaping relative path.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix: add configurable SQLite mmap disabl..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: eee2775add
ℹ️ 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".
- Config option no longer dead code: `storage.sqliteMmap` is bridged to OPENCLAW_SQLITE_MMAP env var during config loading in io.ts, before any SQLite databases are opened. - Fix package boundary violation: replace escaping relative import (../../../../src/infra/) with proper plugin-sdk subpath (openclaw/plugin-sdk/infra-sqlite). - Simplify applySqliteMmapPragma to env-var only (no config param), since the config bridge sets the env var upstream.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ac3777132
ℹ️ 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".
Without this entry, the build does not generate
dist/plugin-sdk/infra-sqlite.{js,d.ts}, breaking the
subpath import from memory-host-sdk.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad589d07a4
ℹ️ 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".
PRAGMA mmap_size = 0 disables mmap for the main database file but WAL mode independently mmaps the -shm index via xShmMap. Document this limitation and note that PRAGMA locking_mode = EXCLUSIVE eliminates the -shm mmap for single-process deployments.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 864314f62d
ℹ️ 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 (env.OPENCLAW_SQLITE_MMAP?.trim()) { | ||
| return; // env var already set, takes precedence | ||
| } | ||
| if (config.storage?.sqliteMmap === false) { | ||
| env.OPENCLAW_SQLITE_MMAP = "0"; |
There was a problem hiding this comment.
applySqliteMmapConfigBridge makes storage.sqliteMmap=false sticky across subsequent config reloads in the same process: once it sets OPENCLAW_SQLITE_MMAP="0", later loadConfig() calls with storage.sqliteMmap=true (or the key removed) cannot clear it because the early return treats the bridged value as user-supplied env. This means new SQLite connections keep applying PRAGMA mmap_size = 0 until restart, so runtime config changes are not honored.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is intentional — the bridge is designed to be one-way: env var takes precedence, config seeds it once at startup.
"Fixing" this would actually introduce a bug: if a user explicitly sets OPENCLAW_SQLITE_MMAP=0 in their environment and then config reloads without the storage.sqliteMmap key, we'd clear their env var — the opposite of the documented "env takes precedence" contract.
It's also not a practical concern: existing SQLite connections already have PRAGMA mmap_size = 0 applied and aren't affected by later env changes. The only scenario where this matters is hot-swapping NFS-backed storage to local storage mid-process, which isn't realistic.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 964602c037
ℹ️ 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".
| storage: z | ||
| .object({ | ||
| /** Enable SQLite memory-mapped I/O. Set to false for NFS-backed volumes. Default: true. */ | ||
| sqliteMmap: z.boolean().optional(), | ||
| }) |
There was a problem hiding this comment.
Adding storage.sqliteMmap to OpenClawSchema updates validation, but this commit does not update the generated schema artifacts (src/config/schema.base.generated.ts and the docs/.generated/config-baseline*.json files). Since buildConfigSchema serves schema data from GENERATED_BASE_CONFIG_SCHEMA (src/config/schema.ts), schema consumers (config UI/schema lookup/docs tooling) will not see the new key even though loadConfig() accepts it, and drift checks will report stale generated outputs.
Useful? React with 👍 / 👎.
|
Thanks for the context here. I did a careful shell check against current Close this older PR: the central NFS-backed SQLite crash problem is now solved on current main by a merged, better-scoped WAL journal-mode mitigation, and the remaining branch diff adds an obsolete config/env/API surface around a weaker mmap-size workaround. So I’m closing this older PR as already covered on Review detailsBest possible solution: Close this stale branch and keep the centralized NFS journal-mode mitigation from current main; any remaining storage policy should build on that helper rather than adding an mmap config toggle. Do we have a high-confidence way to reproduce the issue? No live reproduction was run in this read-only review. The source path and linked replacement proof are high-confidence: WAL on NFS was the risky path, and current main now switches detected NFS-backed databases to rollback journaling. Is this the best way to solve the issue? No. The best fix is the merged current-main helper that detects NFS-backed paths and avoids WAL, not this branch’s mmap-size config toggle. Security review: Security review cleared: No concrete security or supply-chain concern was found; the risk is compatibility and availability from an obsolete config/API mitigation path. AGENTS.md: found and applied where relevant. What I checked:
Likely related people:
Codex review notes: model internal, reasoning high; reviewed against 37e3e895b020; fix evidence: release v2026.6.8-beta.1, commit 5b21a0337bbe. |
clawsweeper
Bot
added
rating: 🧂 unranked krab
openclaw-barnacle
Bot
added
the
triage: needs-real-behavior-proof
clawsweeper
Bot
added
P2
|
ClawSweeper PR egg 🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat. Where did the egg go?
|
|
This pull request has been automatically marked as stale due to inactivity. |
clawsweeper
Bot
removed
the
rating: 🧂 unranked krab
clawsweeper
Bot
added
rating: 🌊 off-meta tidepool
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
ClawSweeper applied the proposed close for this PR.
|
1 participant
Summary
OPENCLAW_SQLITE_MMAP=0env var andstorage.sqliteMmap: falseconfig option to disable SQLite memory-mapped I/OProblem
When openclaw's state directory lives on an NFS mount (common in Kubernetes with NFS provisioners like
nfs-subdir-external-provisioner), SQLite's WAL mode shared-memory (-shm) files use mmap which NFS does not support reliably. A transient server interruption causes the kernel to deliver SIGBUS (exit code 135), crashing the gateway process.We observed this in production on a Raspberry Pi k3s cluster with NFS-backed PVCs — the openclaw pod would crash every ~15 hours with exit code 135 and no application-level error in the logs.
Fix
A new helper
applySqliteMmapPragma()conditionally appliesPRAGMA mmap_size = 0at connection open time. Resolution order:OPENCLAW_SQLITE_MMAPenv var (0/falseto disable,1/trueto enable)storage.sqliteMmapin openclaw.jsontrue(preserves existing behaviour)The env var is convenient for Kubernetes where you can set it in the pod spec without touching config:
Or in openclaw.json:
{ "storage": { "sqliteMmap": false } }Performance impact
Negligible for the small databases openclaw uses.
PRAGMA mmap_size = 0falls back to regularread()syscalls — the same I/O path SQLite used before mmap was introduced.