Skip to content

Commit 6cb3a7d

Browse files
sjsyrekclaude
andcommitted
fix: resolve cache eviction and filesystem efficiency issues
Issue #9 (MEDIUM): Cache eviction efficiency - Replaced loading all entries into memory with SQL DELETE ... LIMIT - Estimate entries to delete based on average size with 20% buffer - Executes entirely in SQLite without loading data into Node.js - Prevents memory spikes for large caches - Added test for efficient eviction behavior Issue #8 (MEDIUM): Redundant file system calls - Cache fs.Stats result in translate() method - Pass cached stats to isFilePath() and translateFile() - Eliminates duplicate statSync/existsSync calls (3 syscalls → 1) - Significant performance improvement for file translation - Updated method signatures to accept optional cached stats All 1140 tests pass with no regressions. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 83de2b6 commit 6cb3a7d

3 files changed

Lines changed: 86 additions & 34 deletions

File tree

src/cli/commands/translate.ts

Lines changed: 26 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -92,22 +92,24 @@ export class TranslateCommand {
9292

9393
/**
9494
* Translate text, file, or directory
95+
* Fix for Issue #8: Cache stats to avoid redundant filesystem calls
9596
*/
9697
async translate(textOrPath: string, options: TranslateOptions): Promise<string> {
97-
// Check if input is a directory
98-
// Use statSync() directly to avoid duplicate syscalls (existsSync + statSync)
98+
// Check if input is a file/directory with a single stat() call
99+
// Cache the stats result to avoid duplicate syscalls later
100+
let stats: fs.Stats | null = null;
99101
try {
100-
const stats = fs.statSync(textOrPath);
102+
stats = fs.statSync(textOrPath);
101103
if (stats.isDirectory()) {
102104
return this.translateDirectory(textOrPath, options);
103105
}
104106
} catch {
105107
// Not a file/directory, treat as text
106108
}
107109

108-
// Check if input is a file path
109-
if (this.isFilePath(textOrPath)) {
110-
return this.translateFile(textOrPath, options);
110+
// Check if input is a file path (passing cached stats to avoid re-stating)
111+
if (this.isFilePath(textOrPath, stats)) {
112+
return this.translateFile(textOrPath, options, stats);
111113
}
112114

113115
return this.translateText(textOrPath, options);
@@ -117,10 +119,16 @@ export class TranslateCommand {
117119
* Check if input is a file path
118120
* Handles cross-platform paths (Windows backslashes and Unix forward slashes)
119121
* Excludes URLs from being treated as file paths
122+
* Fix for Issue #8: Accept cached stats to avoid redundant filesystem calls
120123
*/
121-
private isFilePath(input: string): boolean {
122-
// Check if file exists
123-
if (fs.existsSync(input)) {
124+
private isFilePath(input: string, cachedStats?: fs.Stats | null): boolean {
125+
// If we have cached stats and it's a file, return true immediately
126+
if (cachedStats && cachedStats.isFile()) {
127+
return true;
128+
}
129+
130+
// Check if file exists (only if we don't have cached stats)
131+
if (!cachedStats && fs.existsSync(input)) {
124132
return true;
125133
}
126134

@@ -190,8 +198,9 @@ export class TranslateCommand {
190198

191199
/**
192200
* Translate file
201+
* Fix for Issue #8: Accept cached stats to avoid redundant filesystem calls
193202
*/
194-
private async translateFile(filePath: string, options: TranslateOptions): Promise<string> {
203+
private async translateFile(filePath: string, options: TranslateOptions, cachedStats?: fs.Stats | null): Promise<string> {
195204
if (!options.output) {
196205
throw new Error('Output file path is required for file translation. Use --output <path>');
197206
}
@@ -229,8 +238,13 @@ export class TranslateCommand {
229238
// Smart routing for text-based files
230239
// Use text API (cached) for small text files, document API for large files or binaries
231240
if (this.isTextBasedFile(filePath)) {
232-
// Get file size once to avoid duplicate stat() calls
233-
const fileSize = this.getFileSize(filePath);
241+
// Get file size from cached stats or stat the file (Fix for Issue #8)
242+
let fileSize: number | null;
243+
if (cachedStats) {
244+
fileSize = cachedStats.size;
245+
} else {
246+
fileSize = this.getFileSize(filePath);
247+
}
234248

235249
if (fileSize === null) {
236250
throw new Error(`File not found or cannot be accessed: ${filePath}`);

src/storage/cache.ts

Lines changed: 20 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -258,6 +258,7 @@ export class CacheService {
258258

259259
/**
260260
* Evict oldest entries if needed to make space
261+
* Fix for Issue #9: Use SQL DELETE ... LIMIT to avoid loading all entries into memory
261262
*/
262263
private evictIfNeeded(newEntrySize: number): void {
263264
const stats = this.stats();
@@ -270,27 +271,24 @@ export class CacheService {
270271
// Add a small buffer to ensure we have enough space
271272
const toFree = stats.totalSize + newEntrySize - this.maxSize + 1;
272273

273-
// Get oldest entries to delete
274-
const stmt = this.db.prepare(`
275-
SELECT key, size
276-
FROM cache
277-
ORDER BY timestamp ASC
278-
`);
279-
280-
const rows = stmt.all() as Array<{ key: string; size: number }>;
281-
282-
// Delete entries until we've freed enough space
283-
let freed = 0;
284-
const deleteStmt = this.db.prepare('DELETE FROM cache WHERE key = ?');
285-
286-
for (const row of rows) {
287-
deleteStmt.run(row.key);
288-
freed += row.size;
289-
290-
// Keep deleting until we've freed enough
291-
if (freed >= toFree) {
292-
break;
293-
}
294-
}
274+
// Estimate how many entries to delete based on average size
275+
// This avoids loading all entries into memory for large caches
276+
const avgSize = stats.entries > 0 ? stats.totalSize / stats.entries : 1024;
277+
const estimatedEntries = Math.ceil(toFree / avgSize);
278+
279+
// Add 20% buffer to ensure we delete enough entries
280+
// This handles cases where oldest entries are larger than average
281+
const entriesToDelete = Math.ceil(estimatedEntries * 1.2);
282+
283+
// Delete oldest entries efficiently using SQL subquery
284+
// This executes entirely in SQLite without loading data into Node.js memory
285+
this.db.prepare(`
286+
DELETE FROM cache
287+
WHERE key IN (
288+
SELECT key FROM cache
289+
ORDER BY timestamp ASC
290+
LIMIT ?
291+
)
292+
`).run(entriesToDelete);
295293
}
296294
}

tests/unit/cache-service.test.ts

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -269,6 +269,46 @@ describe('CacheService', () => {
269269
const oldEntry = cacheService.get('key-0');
270270
expect(oldEntry).toBeNull();
271271
});
272+
273+
it('should evict entries efficiently without loading all into memory (Issue #9)', () => {
274+
// Create a cache service with small max size for testing
275+
const smallCache = new CacheService({
276+
dbPath: testCachePath,
277+
maxSize: 10 * 1024, // 10KB
278+
});
279+
280+
// Fill cache with many small entries (simulate large cache)
281+
for (let i = 0; i < 50; i++) {
282+
smallCache.set(`key-${i}`, { text: 'x'.repeat(200) }); // ~200 bytes each
283+
}
284+
285+
const statsBefore = smallCache.stats();
286+
287+
// Cache should be at capacity
288+
expect(statsBefore.totalSize).toBeLessThanOrEqual(statsBefore.maxSize);
289+
290+
// Add new entry that requires eviction
291+
smallCache.set('new-key', { text: 'x'.repeat(1000) }); // ~1KB
292+
293+
const statsAfter = smallCache.stats();
294+
295+
// Cache should still be under max size
296+
expect(statsAfter.totalSize).toBeLessThanOrEqual(statsAfter.maxSize);
297+
298+
// New entry should exist
299+
const newEntry = smallCache.get('new-key');
300+
expect(newEntry).toBeDefined();
301+
302+
// Oldest entries should be evicted
303+
const veryOldEntry = smallCache.get('key-0');
304+
expect(veryOldEntry).toBeNull();
305+
306+
// Some recent entries should still exist
307+
const recentEntry = smallCache.get(`key-${49}`);
308+
expect(recentEntry).toBeDefined();
309+
310+
smallCache.close();
311+
});
272312
});
273313

274314
describe('TTL (Time To Live)', () => {

0 commit comments

Comments
 (0)