ruvnet / ruvnet/ruflo

backupMemoryDb() never integrity-checks the source — corrupt DBs are backed up as success, rotating out the last clean snapshot

Open
#2,895 2 comments 0 reactions 0 assignees View on GitHub
Dominant language
TypeScript
Stars
72.7k
Forks
8.6k
Avg merge
2d 23h
Merged PRs (30d)
83

Description

## Summary

`backupMemoryDb()` never runs `PRAGMA integrity_check` on the **source** before snapshotting `.swarm/memory.db`. A corrupt database is copied forward every night, recorded as a success, and the last clean snapshot rotates out of the retention window unnoticed.

`restoreMemoryDbFromBackup()` already does the right thing on the way *out* — it verifies each candidate and walks backward to a clean one (line ~186). There is simply no equivalent check on the way *in*.

## Field evidence

An operator lost a `.swarm/memory.db` to corruption and went to the backups. The corruption window was pinnable precisely *from the backup history itself*:

- `memory-2026-07-19T14-14-19-691Z.db` → `PRAGMA integrity_check` = `ok`
- `memory-2026-07-21T23-46-23-733Z.db` → same corruption already present

Every nightly run after the 21st faithfully preserved the broken file, with `backedUp: true` each time. By the time anyone looked, the last clean snapshot had been rotated away by `keep`.

## Both success paths are affected

`dist/src/services/memory-backup.js` (v3.34.0), `backupMemoryDb()` at line 24:

1. **Online-backup path** — `db.backup()` succeeds → returns `{ backedUp: true }`. No source check.
2. **Byte-copy fallback** — `fs.copyFileSync(dbPath, destPath)` at line 63, then rotation and the same success return. No source check.

To be clear, the fallback itself is correct and well-reasoned — the in-code comment explains it exists because an encrypted sql.js blob isn't native SQLite, so the online-backup API throws and users with encryption previously got **zero** backups behind a green status. That was a good fix. The point here is narrower: neither path establishes whether what it just preserved is worth preserving.

The fallback is arguably the more dangerous of the two, because a database corrupt enough that better-sqlite3 refuses to open it will fail the online-backup API and then be byte-copied and reported as success.

`grep -c integrity_check` on the file returns 2 — one docstring at ~147 and one call at ~186, both inside the restore function.

## Suggested fix

Run the check on the source and let the result travel with the result object:

```js
let sourceIntegrity = 'unknown';
try {
const db = new Database(dbPath, { readonly: true });
sourceIntegrity = String(db.pragma('integrity_check', { simple: true }) ?? '');
db.close();
} catch (e) { sourceIntegrity = `unreadable: ${e?.message ?? e}`; }
const sourceCorrupt = sourceIntegrity !== 'ok';
```

Then, in both paths:

- **Still take the backup.** A corrupt-but-present database is better than nothing, and refusing to snapshot would be its own failure mode.
- **Mark it.** Tagging the filename (e.g. `*.CORRUPT.db`) keeps it out of the pool of things anyone mistakes for a clean restore point. It also composes with the existing restore logic — a tagged file still matches the `/^memory-.*\.db$/` candidate regex but fails its own `integrity_check` and is correctly skipped in favour of an older clean one.
- **Say so unconditionally.** A `console.warn` that isn't gated behind a verbose flag, plus `sourceIntegrity` / `sourceCorrupt` in the returned object so `runBackupWorker()` can surface it in `backup.json` and the daemon log.

The essential property is that "the backup ran" and "you have a good backup" stop being the same signal.

## Why this is worth fixing even though the data was already corrupt

The corruption itself wasn't caused by this code. But the backup worker is the mechanism an operator relies on to *bound* the damage, and here it silently converted a one-day problem into a five-day one by overwriting the recovery window with copies of the fault.

This is the same shape as #2798 ("Nightly memory backup silently no-ops … worker still reports 100% success") and #2737 ("Default `doctor` run never executes its memory integrity checks — a corrupt DB still ends 'All checks passed!'"): the operation reports success without having verified the thing that makes success meaningful.

## Environment

- `@claude-flow/cli` as shipped with `ruflo` 3.34.0 (source read from the installed dist)
- Originally observed on ruflo 3.32.9, Node v22.23.1, Linux container (WSL2 host); re-verified against 3.34.0 on macOS 26.4.1, Node v22.12.0 — the gap is present in both, and the fallback path is new since the original report.

Happy to open a PR if useful.

Contributor guide

Open the contributing guide

Research direction

Start in dist/src/services/memory-backup.js at backupMemoryDb() and compare both backup paths with restoreMemoryDbFromBackup() around line 186. Trace how runBackupWorker() consumes the result. Done means both paths check the source, preserve corrupt databases without treating them as clean, warn unconditionally, and expose sourceIntegrity/sourceCorrupt in the result.

Written by the indexing model from the issue text.

Assessment

Tech stack
node.js, sqlite, typescript
Domain
backend, databases
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.