Skip to content

Commit 2398c76

Browse files
sjsyrekclaude
andauthored
fix(cache): enforce the size cap from the DB and keep healthy DBs on non-corruption errors (#107)
Size accounting lived in a process-local counter that drifted in both directions and went negative under concurrent processes, so the cap stopped being enforced; eviction under-freed on skewed sizes; an entry larger than maxSize wiped the cache; the expired sweep never ran in sub-60s processes; and getInstance/close cycles accumulated signal listeners. The total is now read from SUM(size), eviction loops until space is actually freed, oversized entries are skipped, the sweep runs on first use, and handlers register once per process. The constructor's corruption recovery is now allowlisted to SQLITE_CORRUPT/SQLITE_NOTADB: SQLITE_BUSY waits via busy_timeout and then propagates (previously it renamed a healthy cache aside, emptied it, and killed the other process's transaction), a newer-schema DB is refused instead of destroyed, backup WAL sidecars are preserved before close under names SQLite can resolve, and rename-aside backups are pruned to the most recent three. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude <noreply@anthropic.com>
1 parent 1e34744 commit 2398c76

6 files changed

Lines changed: 415 additions & 62 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
3434

3535
### Fixed
3636

37+
- **cache**: The size cap is now actually enforced. Size accounting previously lived in a process-local counter that drifted in both directions — downward when an overwrite's eviction deleted the very key being replaced (after which the cap stopped firing and the DB grew without bound), upward when `get()` deleted expired rows without decrementing (evicting entries that didn't need evicting), and negative when a concurrent process's rows were swept (disabling eviction for the process lifetime). The total is now always read from `SELECT SUM(size)`. Eviction now deletes oldest rows in batches until enough space is actually freed, instead of a one-shot estimate from the average row size that under-evicted whenever sizes were skewed. An entry larger than `maxSize` itself is skipped instead of stored — previously it wiped every other entry first (eviction deleted all rows and inserted the oversized one anyway), leaving a cache that re-wiped on every subsequent write. The expired-entry sweep's throttle timer is seeded so the sweep runs on a process's first operation; it was seeded to construction time, so it never ran in any process shorter than 60 seconds — essentially every CLI invocation. Repeated `getInstance()`/`close()` cycles no longer accumulate process signal listeners until Node prints `MaxListenersExceededWarning` to stderr. Docs now describe eviction as oldest-first rather than LRU, which is what the code has always done (`timestamp` is only written on `set()`).
38+
- **cache**: Only genuine corruption (`SQLITE_CORRUPT` / `SQLITE_NOTADB`) now triggers the rename-aside-and-recreate recovery. The constructor previously treated *every* unexpected error as corruption, so transient lock contention (`SQLITE_BUSY` — e.g. two concurrent invocations against a cache on a filesystem where WAL cannot be enabled) renamed a healthy cache aside, recreated it empty, and broke the other process's open transaction; and a DB written by a newer CLI version was destroyed instead of refused, defeating the schema check's stated purpose. Lock waits now go through `PRAGMA busy_timeout` (5s), and everything that isn't corruption propagates so the CLI degrades to an uncached run with a warning. When recovery does run, the backup's WAL/SHM sidecars are copied before the failed handle is closed (SQLite deletes them on close) and named `<backup>-wal` / `<backup>-shm` so SQLite can actually recover the preserved data — the old naming (`<original>-wal<suffix>`) orphaned the WAL, silently losing any rows not yet checkpointed. Rename-aside backups are pruned to the most recent three so repeated corruption cannot fill the disk.
3739
- **sync**: `deepl sync pull` no longer discards existing translations for keys named after `Object.prototype` members. `sanitizePullKeysResponse` built its result on a plain object, and callers test membership with `pulledKeys[key] !== undefined` / `??` — so for a source key called `toString`, `constructor`, `valueOf`, `hasOwnProperty`, or `__proto__`, the lookup returned an inherited *function*, which is neither `undefined` nor nullish. The entry was therefore treated as a freshly approved TMS translation and won over the real one, then vanished from the file when `JSON.stringify` dropped the function value — while the lockfile recorded it as `translated` / `human_reviewed`, so no later sync repaired it. The accumulator now has a null prototype.
3840
- **sync**: ICU plural/select messages with text around the block are now preserved. The detection pattern was anchored to the start of the string, so `You have {count, plural, one {# item} other {# items}} in your cart.` — arguably the most common real shape — was not recognised as ICU at all and the raw syntax went to the engine as prose. Confirmed against the live API: it returned `{count, Plural, ein {…} weiteres {…}}`, translating the format keyword *and* both selectors, leaving a message with no valid format type and no `other` fallback. Detection now matches an ICU block anywhere in the string, and the prose on either side becomes a translatable segment instead of being silently dropped from the reassembled output (trailing text was previously lost outright). Re-verified end to end after the fix: `plural`, `one`, `other`, and the `count` argument all survive while the surrounding prose is translated. Note this affects `deepl sync`, the only path with ICU preservation; `deepl translate` on a raw ICU string is unchanged.
3941
- **sync**: ICU structural damage is now detected instead of reported as passing. `checkIcuBrackets` compared only brace counts and nesting depth, which are identical when the engine translates the keyword, a selector, or the argument name — so `fail_on_error` never tripped on a message that no longer renders. Validation now compares the parsed argument/format-type headers and the selector keyword sets, while tolerating reordered selectors (order is not meaningful in ICU).

‎README.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515
- **🎙️ Voice Translation (Pro/Enterprise)** - Real-time speech translation via WebSocket streaming (Voice API)
1616
- **👀 Watch Mode** - Real-time file watching with auto-translation
1717
- **✍️ Writing Enhancement** - Grammar, style, and tone suggestions (DeepL Write API)
18-
- **💾 Smart Caching** - Local SQLite cache with LRU eviction
18+
- **💾 Smart Caching** - Local SQLite cache that evicts oldest entries first
1919
- **🎯 Context-Aware** - Preserves code blocks, variables, and formatting
2020
- **📦 Batch Processing** - Translate multiple files with parallel processing
2121
- **💰 Cost Transparency** - Track actual billed characters for budget planning
@@ -1430,7 +1430,7 @@ Storage (SQLite Cache, Config) + Static Data (src/data — language registry)
14301430
- **Watch Service** — file watching with debouncing
14311431
- **Glossary Service** — glossary management and application
14321432
- **Translation Memory Service** — reuse approved translations (`--translation-memory`)
1433-
- **Cache** — SQLite cache with LRU eviction (`src/storage/cache.ts`)
1433+
- **Cache** — SQLite cache, oldest-entry-first eviction (`src/storage/cache.ts`)
14341434
- **Preservation utilities** — `src/utils/` helpers for code blocks, variables, and ICU MessageFormat
14351435

14361436
## 🧪 Testing

‎src/storage/cache.ts‎

Lines changed: 131 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
/**
22
* Cache Service
3-
* SQLite-based translation cache with LRU eviction
3+
* SQLite-based translation cache; evicts oldest-written entries first
44
*/
55

66
import { DatabaseSync } from 'node:sqlite';
@@ -10,12 +10,12 @@ import { resolvePaths } from '../utils/paths.js';
1010
import { ConfigError } from '../utils/errors.js';
1111
import { Logger } from '../utils/logger.js';
1212
import { errorMessage } from '../utils/error-message.js';
13-
import { isNativeModuleLoadError } from '../utils/native-module-error.js';
1413

1514
export interface CacheServiceOptions {
1615
dbPath?: string;
1716
maxSize?: number; // in bytes
1817
ttl?: number; // in milliseconds, 0 = disabled
18+
busyTimeoutMs?: number; // how long SQLite waits on a locked DB before erroring
1919
}
2020

2121
export interface CacheStats {
@@ -35,6 +35,31 @@ interface CacheRow {
3535
const DEFAULT_MAX_SIZE = 1024 * 1024 * 1024; // 1GB
3636
const DEFAULT_TTL = 30 * 24 * 60 * 60 * 1000; // 30 days in milliseconds
3737
const CLEANUP_INTERVAL = 60_000; // 60 seconds
38+
const DEFAULT_BUSY_TIMEOUT_MS = 5_000;
39+
const EVICTION_BATCH = 32;
40+
const CORRUPT_BACKUPS_TO_KEEP = 3;
41+
42+
// SQLite primary result codes that mean the file itself is damaged. Everything
43+
// else (BUSY, PERM, a deliberate ConfigError, a backend that cannot load) must
44+
// propagate: the DB on disk is healthy and renaming it aside would destroy it.
45+
const SQLITE_CORRUPT = 11;
46+
const SQLITE_NOTADB = 26;
47+
48+
// Duck-typed rather than `instanceof Error`: node:sqlite errors are created
49+
// in the host realm, so instanceof fails against another realm's Error (as
50+
// in a jest test context).
51+
function isCorruptionError(error: unknown): boolean {
52+
if (typeof error !== 'object' || error === null) return false;
53+
const { errcode, message } = error as { errcode?: unknown; message?: unknown };
54+
if (typeof errcode === 'number') {
55+
const primary = errcode & 0xff;
56+
return primary === SQLITE_CORRUPT || primary === SQLITE_NOTADB;
57+
}
58+
return (
59+
typeof message === 'string' &&
60+
/file is not a database|database disk image is malformed/i.test(message)
61+
);
62+
}
3863

3964
/**
4065
* Current on-disk schema version for the SQLite cache. Stamped into
@@ -52,10 +77,13 @@ export class CacheService {
5277
private db!: DatabaseSync;
5378
private maxSize: number;
5479
private ttl: number;
80+
private busyTimeoutMs: number;
5581
private enabled: boolean = true;
5682
private isClosed: boolean = false;
57-
private currentSize: number = 0;
58-
private lastCleanupTime: number = Date.now();
83+
// Seeded to 0 so the first operation of every process sweeps expired rows;
84+
// seeding to Date.now() would keep the sweep from ever running in a process
85+
// shorter than CLEANUP_INTERVAL — i.e. nearly every CLI invocation.
86+
private lastCleanupTime: number = 0;
5987

6088
/**
6189
* Create a new CacheService instance. Production code should use
@@ -66,44 +94,87 @@ export class CacheService {
6694
const dbPath = options.dbPath ?? resolvePaths().cacheFile;
6795
this.maxSize = options.maxSize ?? DEFAULT_MAX_SIZE;
6896
this.ttl = options.ttl ?? DEFAULT_TTL;
97+
this.busyTimeoutMs = options.busyTimeoutMs ?? DEFAULT_BUSY_TIMEOUT_MS;
6998

7099
try {
71100
this.openDatabase(dbPath);
72101
} catch (error) {
73-
// A backend that cannot load (node:sqlite missing on an older
74-
// Node runtime) is not corruption: the database on disk is
75-
// healthy, so renaming it aside would throw away a warm cache.
76-
if (isNativeModuleLoadError(error)) {
102+
// Only genuine file damage may take the rename-aside path. Lock
103+
// contention (SQLITE_BUSY), a schema written by a newer CLI
104+
// (ConfigError), permission errors, and a backend that cannot load
105+
// all leave a healthy database on disk — propagate them so the
106+
// cache loader degrades to an uncached run instead.
107+
if (!isCorruptionError(error)) {
77108
throw error;
78109
}
79-
// Corrupted DB: rename-aside rather than unlink, so the user keeps
80-
// 30 days of cache history (and a forensic artifact) instead of
81-
// losing both silently. Suffix with a timestamp so repeated
82-
// corruption doesn't clobber earlier backups.
110+
// Rename-aside rather than unlink, so the user keeps 30 days of
111+
// cache history (and a forensic artifact) instead of losing both
112+
// silently. Suffix with a timestamp so repeated corruption doesn't
113+
// clobber earlier backups.
83114
const suffix = `.corrupt-${Date.now()}`;
84115
Logger.warn(
85116
`Cache database corrupted, backing up and recreating: ${(error as Error).message}. ` +
86-
`Previous contents preserved at ${path.basename(dbPath)}${suffix} (and -wal${suffix} / -shm${suffix} if present).`,
117+
`Previous contents preserved at ${path.basename(dbPath)}${suffix} (and ${path.basename(dbPath)}${suffix}-wal / -shm if present).`,
87118
);
88119
try {
89-
if (fs.existsSync(dbPath)) fs.renameSync(dbPath, dbPath + suffix);
120+
// Sidecars must be preserved BEFORE closing the failed handle —
121+
// SQLite unlinks -wal/-shm when the last connection closes — and as
122+
// <backup>-wal / <backup>-shm, because SQLite resolves a database's
123+
// sidecars as <db-file>-wal / <db-file>-shm.
90124
for (const sidecar of ['-wal', '-shm']) {
91125
const f = dbPath + sidecar;
92-
if (fs.existsSync(f)) fs.renameSync(f, f + suffix);
126+
if (fs.existsSync(f)) fs.copyFileSync(f, dbPath + suffix + sidecar);
127+
}
128+
// Close before renaming the main file: an open fd on the renamed
129+
// file blocks the rename on Windows.
130+
try {
131+
(this.db as DatabaseSync | undefined)?.close();
132+
} catch {
133+
// Never opened, or already closed.
134+
}
135+
if (fs.existsSync(dbPath)) fs.renameSync(dbPath, dbPath + suffix);
136+
for (const sidecar of ['-wal', '-shm']) {
137+
fs.rmSync(dbPath + sidecar, { force: true });
93138
}
139+
this.pruneCorruptBackups(dbPath);
94140
this.openDatabase(dbPath);
95141
} catch {
96142
throw error;
97143
}
98144
}
99145
}
100146

147+
/**
148+
* Keep only the most recent CORRUPT_BACKUPS_TO_KEEP rename-aside backups
149+
* (each can be up to maxSize) so repeated corruption cannot fill the disk.
150+
*/
151+
private pruneCorruptBackups(dbPath: string): void {
152+
try {
153+
const dir = path.dirname(dbPath);
154+
const prefix = `${path.basename(dbPath)}.corrupt-`;
155+
const stamps = [...new Set(
156+
fs.readdirSync(dir)
157+
.filter((f) => f.startsWith(prefix))
158+
.map((f) => /\.corrupt-(\d+)/.exec(f)?.[1])
159+
.filter((s): s is string => s !== undefined),
160+
)].sort((a, b) => Number(b) - Number(a));
161+
for (const stamp of stamps.slice(CORRUPT_BACKUPS_TO_KEEP)) {
162+
for (const suffix of ['', '-wal', '-shm']) {
163+
fs.rmSync(`${dbPath}.corrupt-${stamp}${suffix}`, { force: true });
164+
}
165+
}
166+
} catch {
167+
// Best-effort: a failed prune must not block cache recreation.
168+
}
169+
}
170+
101171
private openDatabase(dbPath: string): void {
102172
const dir = path.dirname(dbPath);
103173
if (!fs.existsSync(dir)) {
104174
fs.mkdirSync(dir, { recursive: true, mode: 0o700 });
105175
}
106176
this.db = new DatabaseSync(dbPath);
177+
this.db.exec(`PRAGMA busy_timeout = ${this.busyTimeoutMs}`);
107178
fs.chmodSync(dbPath, 0o600);
108179
this.initialize();
109180
}
@@ -170,9 +241,19 @@ export class CacheService {
170241
if (userVersion < CACHE_SCHEMA_VERSION) {
171242
this.db.exec(`PRAGMA user_version = ${CACHE_SCHEMA_VERSION}`);
172243
}
244+
}
173245

174-
const row = this.db.prepare('SELECT COALESCE(SUM(size), 0) as total FROM cache').get() as { total: number };
175-
this.currentSize = row.total;
246+
/**
247+
* Total stored bytes, always read from the database. A process-local
248+
* counter cannot stay correct here: get()-path deletes, eviction during
249+
* overwrites, and concurrent processes sharing the file all move the real
250+
* total without this process seeing it.
251+
*/
252+
private totalSize(): number {
253+
const row = this.db
254+
.prepare('SELECT COALESCE(SUM(size), 0) as total FROM cache')
255+
.get() as { total: number };
256+
return row.total;
176257
}
177258

178259
/**
@@ -234,6 +315,15 @@ export class CacheService {
234315
const size = Buffer.byteLength(json, 'utf8');
235316
const timestamp = Date.now();
236317

318+
// An entry that alone exceeds the cap can never be cached without
319+
// evicting everything else first — skip it rather than wipe the cache.
320+
if (size > this.maxSize) {
321+
Logger.verbose(
322+
`Skipping cache write: entry of ${size} bytes exceeds the ${this.maxSize}-byte cache size limit.`,
323+
);
324+
return;
325+
}
326+
237327
// Clean up expired entries
238328
this.cleanupExpired();
239329

@@ -250,12 +340,10 @@ export class CacheService {
250340
`);
251341

252342
stmt.run(key, json, timestamp, size);
253-
this.currentSize = this.currentSize - existingSize + size;
254343
}
255344

256345
clear(): void {
257346
this.db.exec('DELETE FROM cache');
258-
this.currentSize = 0;
259347
}
260348

261349
/**
@@ -308,8 +396,10 @@ export class CacheService {
308396
this.db.close();
309397
this.isClosed = true;
310398
if (CacheService.instance === this) {
399+
// handlersRegistered stays true: the process handlers tolerate a
400+
// null instance, and re-registering on every getInstance/close
401+
// cycle accumulates listeners until Node warns on stderr.
311402
CacheService.instance = null;
312-
CacheService.handlersRegistered = false;
313403
}
314404
}
315405
}
@@ -330,40 +420,30 @@ export class CacheService {
330420
}
331421
this.lastCleanupTime = now;
332422

333-
const expirationTime = now - this.ttl;
334-
const sizeStmt = this.db.prepare('SELECT COALESCE(SUM(size), 0) as total FROM cache WHERE timestamp < ?');
335-
const sizeRow = sizeStmt.get(expirationTime) as { total: number };
336-
const deletedSize = sizeRow.total;
337-
338-
this.db.prepare('DELETE FROM cache WHERE timestamp < ?').run(expirationTime);
339-
this.currentSize -= deletedSize;
423+
this.db.prepare('DELETE FROM cache WHERE timestamp < ?').run(now - this.ttl);
340424
}
341425

342426
private evictIfNeeded(newEntrySize: number): void {
343-
if (this.currentSize + newEntrySize <= this.maxSize) {
344-
return;
427+
let excess = this.totalSize() + newEntrySize - this.maxSize;
428+
429+
// Delete oldest rows in batches until enough space is freed. A one-shot
430+
// estimate from the average row size under-evicts whenever sizes are
431+
// skewed, leaving the cache over its cap.
432+
while (excess > 0) {
433+
const deleted = this.db.prepare(`
434+
DELETE FROM cache
435+
WHERE key IN (
436+
SELECT key FROM cache
437+
ORDER BY timestamp ASC
438+
LIMIT ${EVICTION_BATCH}
439+
)
440+
RETURNING size
441+
`).all() as { size: number }[];
442+
443+
if (deleted.length === 0) {
444+
return;
445+
}
446+
excess -= deleted.reduce((sum, row) => sum + row.size, 0);
345447
}
346-
347-
const toFree = this.currentSize + newEntrySize - this.maxSize + 1;
348-
const { count: entries } = this.db.prepare(
349-
'SELECT COUNT(*) as count FROM cache'
350-
).get() as { count: number };
351-
352-
const avgSize = entries > 0 ? this.currentSize / entries : 1024;
353-
const estimatedEntries = Math.ceil(toFree / avgSize);
354-
const entriesToDelete = Math.ceil(estimatedEntries * 1.2);
355-
356-
const deleted = this.db.prepare(`
357-
DELETE FROM cache
358-
WHERE key IN (
359-
SELECT key FROM cache
360-
ORDER BY timestamp ASC
361-
LIMIT ?
362-
)
363-
RETURNING size
364-
`).all(entriesToDelete) as { size: number }[];
365-
366-
const deletedSize = deleted.reduce((sum, row) => sum + row.size, 0);
367-
this.currentSize -= deletedSize;
368448
}
369449
}

0 commit comments

Comments
 (0)