feat(sessions): SQLite-backed two-tier session store — fixes 140%+ CPU at scale#58550
feat(sessions): SQLite-backed two-tier session store — fixes 140%+ CPU at scale#58550alexdeg92 wants to merge 5 commits intoopenclaw:mainfrom
Conversation
Replace flat sessions.json with SQLite hot index + existing .jsonl cold storage. Problem: - sessions.json grows to 42MB+ with 1000+ sessions - Every operation reads/writes the entire file (O(n)) - Causes 140%+ CPU and 6s+ response times at scale - Related: openclaw#58534, openclaw#57497 Solution: - SQLite hot index for session metadata (O(1) lookups, indexed queries) - Existing .jsonl cold storage unchanged (only loaded on explicit history requests) - Auto-migration from sessions.json on first boot - Opt-in via config: session.storeType = 'sqlite' - Fallback to JSON if SQLite unavailable (Node < 22.5) - CLI: openclaw sessions migrate [--all-agents] [--dry-run] Performance (1000 sessions): - Load: 800ms → 15ms - Single update: 800ms → 5ms - Memory: 42MB → 2MB - CPU on save: 100%+ → <5%
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 918c06c0e4
ℹ️ 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".
| // eslint-disable-next-line @typescript-eslint/no-require-imports | ||
| require("node:sqlite"); | ||
| return true; |
There was a problem hiding this comment.
Probe node:sqlite with createRequire
This module runs in an ESM codebase (package.json uses "type": "module"), so bare require(...) throws ReferenceError. Because the error is caught here, isSqliteAvailable() returns false even on runtimes that do support node:sqlite, which forces resolveEffectiveStoreType() back to JSON and makes sessions migrate report SQLite as unavailable. Use the existing createRequire/requireNodeSqlite pattern for this probe.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b69917e — replaced bare require('node:sqlite') with requireNodeSqlite() from src/infra/node-sqlite.ts, which uses createRequire(import.meta.url). Same pattern already used throughout the codebase.
| * SQLite is recommended for installations with 100+ sessions. | ||
| * Related issues: #58534 (perf), #57497 (Postgres request) | ||
| */ | ||
| storeType?: SessionStoreType; |
There was a problem hiding this comment.
Add session.storeType to runtime config validation
This introduces session.storeType as a documented/typed setting, but the runtime config validator still uses a strict SessionSchema that does not include this key, so setting session.storeType: "sqlite" causes loadConfig() to fail with INVALID_CONFIG instead of enabling the new backend. The schema/generated config artifacts need to be updated in lockstep with this type addition.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b69917e — added storeType: z.enum(["json", "sqlite"]).optional() to SessionSchema in zod-schema.session.ts. Setting session.storeType: "sqlite" no longer causes INVALID_CONFIG.
| } catch (err) { | ||
| log.warn("failed to load sessions from SQLite, returning empty store", { error: err }); | ||
| return {}; |
There was a problem hiding this comment.
Re-throw SQLite load errors to preserve JSON fallback
The facade is written to fall back to JSON when SQLite fails, but this function swallows all SQLite read/open errors and returns {}. In failure scenarios (e.g., locked/corrupt DB), callers treat that as a real empty store; subsequent updateSessionStore saves can then persist incomplete state instead of using JSON as a fallback source. Let this path throw (or return an explicit error result) so the fallback in loadSessionStoreWithFacade can actually run.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b69917e — loadSessionStoreSqlite now re-throws on error instead of returning {}. The facade's catch block in loadSessionStoreWithFacade can now correctly fall back to the JSON loader when SQLite fails.
Greptile SummaryThis PR introduces a two-tier SQLite-backed session store to address severe performance degradation (140%+ CPU, 6s+ response times) caused by a growing Critical issue blocking the feature:
Minor issues:
Confidence Score: 4/5Not safe to merge until the ESM There is a single P1 defect — src/config/sessions/store-facade.ts — the
|
| Filename | Overview |
|---|---|
| src/config/sessions/store-facade.ts | New abstraction layer for JSON/SQLite backends. Contains a critical P1 bug: isSqliteAvailable() uses bare require() in an ESM module, which always throws and returns false, making the SQLite feature entirely non-functional. |
| src/config/sessions/store-sqlite.ts | New SQLite backend implementation. Well-structured with WAL mode, proper permissions, schema migrations, and transaction safety. Uses the correct requireNodeSqlite helper for ESM-safe node:sqlite access. |
| src/commands/sessions-migrate.ts | New migration command. Mostly clean, but has two minor issues: unused cfg variable in sessionsStoreInfoCommand (double loadConfig() call) and an unused resolveEffectiveStoreType import. |
| src/config/sessions/store.ts | Integration of the facade into the existing store. Existing JSON logic is cleanly wrapped inside the facade callback, preserving all retry/atomic-write behavior. |
| src/config/sessions/store-sqlite.test.ts | Good test coverage for the SQLite store: upsert, delete, prune, cap, migration, and field round-trip checks. beforeEach/afterEach properly close the DB and clean up the temp directory. |
| src/config/types.base.ts | Adds SessionStoreType union type and optional storeType field to SessionConfig. Clean, backward-compatible addition. |
| src/cli/program/register.status-health-sessions.ts | Registers sessions migrate and sessions store-info CLI subcommands. Follows existing patterns for option handling and help text. |
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/config/sessions/store-facade.ts
Line: 47-55
Comment:
**`require()` not available in ESM — `isSqliteAvailable()` always returns `false`**
This project has `"type": "module"` in `package.json`, making every `.ts`/`.js` file an ES module. In ESM, `require` is not a global — calling it throws `ReferenceError: require is not defined`, which is silently swallowed by the `catch` block. The function therefore **always returns `false`** regardless of Node version.
This cascades into:
- `resolveEffectiveStoreType()` always falls back to `"json"` when `storeType: "sqlite"` is configured
- `sessionsMigrateCommand` always exits early with "SQLite is not available in this Node runtime"
- The SQLite feature is entirely non-functional as shipped
The rest of the codebase already solves this correctly via `createRequire(import.meta.url)` in `src/infra/node-sqlite.ts`. `isSqliteAvailable` should use that same pattern:
```typescript
import { requireNodeSqlite } from "../../infra/node-sqlite.js";
export function isSqliteAvailable(): boolean {
try {
requireNodeSqlite();
return true;
} catch {
return false;
}
}
```
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/commands/sessions-migrate.ts
Line: 231
Comment:
**Unused `cfg` variable — `loadConfig()` called twice**
`cfg` is loaded here but never referenced anywhere in `sessionsStoreInfoCommand`. `getSessionStoreStats` (called on line 235) already invokes `loadConfig()` internally through `resolveSessionStoreType()`, so the config ends up being parsed twice without benefit.
```suggestion
const rich = isRich();
```
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/commands/sessions-migrate.ts
Line: 9
Comment:
**Unused import `resolveEffectiveStoreType`**
`resolveEffectiveStoreType` is imported from `store-facade.js` but is never called anywhere in this file. This should be removed to avoid dead imports and potential lint failures.
```suggestion
isSqliteAvailable,
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat(sessions): SQLite-backed two-tier s..." | Re-trigger Greptile
| export function isSqliteAvailable(): boolean { | ||
| try { | ||
| // eslint-disable-next-line @typescript-eslint/no-require-imports | ||
| require("node:sqlite"); | ||
| return true; | ||
| } catch { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
require() not available in ESM — isSqliteAvailable() always returns false
This project has "type": "module" in package.json, making every .ts/.js file an ES module. In ESM, require is not a global — calling it throws ReferenceError: require is not defined, which is silently swallowed by the catch block. The function therefore always returns false regardless of Node version.
This cascades into:
resolveEffectiveStoreType()always falls back to"json"whenstoreType: "sqlite"is configuredsessionsMigrateCommandalways exits early with "SQLite is not available in this Node runtime"- The SQLite feature is entirely non-functional as shipped
The rest of the codebase already solves this correctly via createRequire(import.meta.url) in src/infra/node-sqlite.ts. isSqliteAvailable should use that same pattern:
import { requireNodeSqlite } from "../../infra/node-sqlite.js";
export function isSqliteAvailable(): boolean {
try {
requireNodeSqlite();
return true;
} catch {
return false;
}
}Prompt To Fix With AI
This is a comment left during a code review.
Path: src/config/sessions/store-facade.ts
Line: 47-55
Comment:
**`require()` not available in ESM — `isSqliteAvailable()` always returns `false`**
This project has `"type": "module"` in `package.json`, making every `.ts`/`.js` file an ES module. In ESM, `require` is not a global — calling it throws `ReferenceError: require is not defined`, which is silently swallowed by the `catch` block. The function therefore **always returns `false`** regardless of Node version.
This cascades into:
- `resolveEffectiveStoreType()` always falls back to `"json"` when `storeType: "sqlite"` is configured
- `sessionsMigrateCommand` always exits early with "SQLite is not available in this Node runtime"
- The SQLite feature is entirely non-functional as shipped
The rest of the codebase already solves this correctly via `createRequire(import.meta.url)` in `src/infra/node-sqlite.ts`. `isSqliteAvailable` should use that same pattern:
```typescript
import { requireNodeSqlite } from "../../infra/node-sqlite.js";
export function isSqliteAvailable(): boolean {
try {
requireNodeSqlite();
return true;
} catch {
return false;
}
}
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed in 86673c7 — isSqliteAvailable() now uses requireNodeSqlite() from src/infra/node-sqlite.ts (ESM-safe via createRequire(import.meta.url)).
| opts: SessionsStoreInfoOptions, | ||
| runtime: RuntimeEnv, | ||
| ): Promise<void> { | ||
| const cfg = loadConfig(); |
There was a problem hiding this comment.
cfg variable — loadConfig() called twice
cfg is loaded here but never referenced anywhere in sessionsStoreInfoCommand. getSessionStoreStats (called on line 235) already invokes loadConfig() internally through resolveSessionStoreType(), so the config ends up being parsed twice without benefit.
| const cfg = loadConfig(); | |
| const rich = isRich(); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/commands/sessions-migrate.ts
Line: 231
Comment:
**Unused `cfg` variable — `loadConfig()` called twice**
`cfg` is loaded here but never referenced anywhere in `sessionsStoreInfoCommand`. `getSessionStoreStats` (called on line 235) already invokes `loadConfig()` internally through `resolveSessionStoreType()`, so the config ends up being parsed twice without benefit.
```suggestion
const rich = isRich();
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed in b69917e — removed the unused const cfg = loadConfig() from sessionsStoreInfoCommand.
| import { | ||
| getSessionStoreStats, | ||
| isSqliteAvailable, | ||
| resolveEffectiveStoreType, |
There was a problem hiding this comment.
resolveEffectiveStoreType
resolveEffectiveStoreType is imported from store-facade.js but is never called anywhere in this file. This should be removed to avoid dead imports and potential lint failures.
| resolveEffectiveStoreType, | |
| isSqliteAvailable, |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/commands/sessions-migrate.ts
Line: 9
Comment:
**Unused import `resolveEffectiveStoreType`**
`resolveEffectiveStoreType` is imported from `store-facade.js` but is never called anywhere in this file. This should be removed to avoid dead imports and potential lint failures.
```suggestion
isSqliteAvailable,
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed in 86673c7 — removed the unused resolveEffectiveStoreType import.
…ty check
bare require('node:sqlite') throws ReferenceError in ESM modules.
Replace with requireNodeSqlite() from src/infra/node-sqlite.ts which
uses createRequire(import.meta.url) — already the established pattern
in this codebase.
Also remove unused resolveEffectiveStoreType import in sessions-migrate.ts.
Fixes isSqliteAvailable() always returning false in ESM context.
- Add storeType to SessionSchema zod validation so session.storeType: sqlite does not cause INVALID_CONFIG on loadConfig() (Codex P1) - Throw instead of swallowing errors in loadSessionStoreSqlite so the JSON fallback in loadSessionStoreWithFacade can actually trigger (Codex P1) - Remove unused cfg variable in sessionsStoreInfoCommand (Greptile P2)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b33c4abc3
ℹ️ 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".
| } catch { | ||
| // Fall back to JSON if SQLite fails | ||
| return jsonSaver(); |
There was a problem hiding this comment.
Avoid JSON fallback on SQLite write errors
In sqlite mode this catch block writes updates to sessions.json when SQLite throws, but future reads still prefer SQLite and will ignore that fallback file as soon as SQLite is readable again. A transient SQLite error (for example SQLITE_BUSY) can therefore silently drop the just-saved session changes in subsequent loads, because the source of truth remains the stale SQLite DB.
Useful? React with 👍 / 👎.
| } catch (err) { | ||
| log.error("failed to migrate JSON to SQLite", { error: err }); | ||
| return 0; |
There was a problem hiding this comment.
Surface JSON-to-SQLite migration failures
This catch converts all migration errors into 0 migrated rows, which callers currently treat as a normal "no data" outcome. If JSON parsing or SQLite writes fail, the command path and facade can proceed as though migration succeeded/was empty, leaving users with an empty SQLite hot store instead of a hard failure or JSON fallback.
Useful? React with 👍 / 👎.
- Fix import: use `warn` instead of non-existent `warning` from globals - Fix JSON output: use `writeRuntimeJson()` helper instead of `runtime.writeJson()` - Remove invalid `allowMulti` option from resolveSessionStoreTargetsOrExit call - Add null check for targets before iteration - Fix Windows path test: use path.join() for platform-agnostic assertions - Add session.storeType to schema labels, help, and generated schema Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8197d03aad
ℹ️ 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".
| const store = JSON.parse(raw) as Record<string, SessionEntry>; | ||
| if (!store || typeof store !== "object" || Array.isArray(store)) { |
There was a problem hiding this comment.
Apply session-store migrations before importing JSON rows
The JSON loader path runs applySessionStoreMigrations to normalize legacy keys (for example provider/lastProvider to channel/lastChannel), but migrateJsonToSqlite inserts raw parsed entries directly. For older sessions.json files, this can persist rows with missing hot-index columns and drop routing metadata semantics after migration until each affected session is rewritten.
Useful? React with 👍 / 👎.
| if (migrationAttempted.has(key)) { | ||
| return true; | ||
| } | ||
| migrationAttempted.add(key); |
There was a problem hiding this comment.
Defer migration-attempt memoization until migration succeeds
migrationAttempted is marked before any migration work runs, so a transient first-run failure leaves the key permanently memoized for the rest of the process. Subsequent loads then skip JSON→SQLite migration and may read an empty/stale SQLite store, making existing JSON sessions appear missing until restart or manual intervention.
Useful? React with 👍 / 👎.
|
This pull request has been automatically marked as stale due to inactivity. |
|
Codex review: needs changes before merge. Summary Reproducibility: yes. for the blocking PR defects: configure sqlite mode and simulate a SQLite save error, invalid JSON migration, or migration write failure; the diff shows writes can fall back to JSON while reads still prefer SQLite, and migration failures can be converted to zero-row success. I did not run tests because this review was read-only. Next step before merge Security Review findings
Review detailsBest possible solution: Queue an autorepair or replacement PR that rebases the SQLite idea onto the current session-store split, keeps JSON as the default, makes SQLite mode a single authoritative store, fails or explicitly switches source-of-truth on SQLite errors, normalizes legacy JSON rows during migration, and updates docs plus changelog. Do we have a high-confidence way to reproduce the issue? Yes for the blocking PR defects: configure sqlite mode and simulate a SQLite save error, invalid JSON migration, or migration write failure; the diff shows writes can fall back to JSON while reads still prefer SQLite, and migration failures can be converted to zero-row success. I did not run tests because this review was read-only. Is this the best way to solve the issue? No. The high-level local SQLite store may be a maintainable performance direction, but this implementation is not mergeable until the source-of-truth and migration semantics are repaired and the branch is rebased onto current main. Full review comments:
Overall correctness: patch is incorrect Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against f73826360492. |
|
@clawsweeper autorepair |
|
🦞🦞 I asked ClawSweeper to answer this maintainer mention in the next review comment. Tiny claws, bounded scope: this is a read-only assist pass unless it produces one of the existing structured safe-action markers. Request: autorepair |
SQLite-backed Session Store
Problem
The flat
sessions.jsonfile causes severe performance issues at scale:Related issues: #58534 (perf), #57497 (Postgres request)
Solution: Two-tier Architecture
Hot Index (SQLite)
A lightweight SQLite database replaces
sessions.jsonfor metadata:Schema columns:
session_key(PRIMARY KEY) - session identifiersession_id- UUIDupdated_at,created_at- timestamps (indexed)channel,last_channel,last_to,last_account_id,last_thread_id- routinglabel,display_name,status- display infomodel,model_provider,total_tokens,input_tokens,output_tokens- model statemessage_count,archived- metadataentry_json- full SessionEntry blob for complex fieldsBenefits:
Cold Storage (unchanged)
Existing
.jsonltranscript files stay as-is:sessions_historycallsConfiguration
Add to
openclaw.json:{ "session": { "storeType": "sqlite" // "json" (default) or "sqlite" } }Migration
Automatic (on first access)
When
storeType: "sqlite"is set, existingsessions.jsonis automatically migrated to SQLite on first load.Manual (CLI)
Fallback Behavior
sessions.jsonis preserved during migration (not deleted)Files Changed
New Files
src/config/sessions/store-sqlite.ts- SQLite storage implementationsrc/config/sessions/store-facade.ts- Backend abstraction layersrc/commands/sessions-migrate.ts- Migration commandModified Files
src/config/types.base.ts- AddedSessionStoreTypeandstoreTypeconfigsrc/config/sessions/store.ts- Integrated facade for load/savesrc/cli/program/register.status-health-sessions.ts- CLI commandsPerformance Expectations
Testing
Backward Compatibility
storeType: "json"for backward compatibilitysessions.jsonfiles continue to worknode:sqlite)Closes #58534
Related #57497