From 3323ba6a5d3c4a533b226f401aee1dc21e7033c9 Mon Sep 17 00:00:00 2001 From: Satyaki Ghosh Date: Tue, 25 Aug 2026 15:13:03 -0600 Subject: [PATCH 1/2] Enable FileDB in prod and only clean up storage dirs that are older --- assets/featureFlag/prod.json | 4 +- src/datastore/FileStoreFactory.ts | 3 +- src/datastore/LMDBStoreFactory.ts | 9 ++--- src/datastore/VersionDirectory.ts | 24 ++++++++++++ tst/unit/datastore/FileStore.test.ts | 15 +++++--- tst/unit/datastore/LMDB.recovery.test.ts | 10 ++++- tst/unit/datastore/VersionDirectory.test.ts | 41 +++++++++++++++++++++ 7 files changed, 91 insertions(+), 15 deletions(-) create mode 100644 src/datastore/VersionDirectory.ts create mode 100644 tst/unit/datastore/VersionDirectory.test.ts diff --git a/assets/featureFlag/prod.json b/assets/featureFlag/prod.json index 3ef2fde9..5bd2215a 100644 --- a/assets/featureFlag/prod.json +++ b/assets/featureFlag/prod.json @@ -45,8 +45,8 @@ ] }, "FileDb": { - "enabled": false, - "fleetPercentage": 0 + "enabled": true, + "fleetPercentage": 5 } } } diff --git a/src/datastore/FileStoreFactory.ts b/src/datastore/FileStoreFactory.ts index 7dc56d80..6f02c490 100644 --- a/src/datastore/FileStoreFactory.ts +++ b/src/datastore/FileStoreFactory.ts @@ -10,6 +10,7 @@ import { DataStore, DataStoreFactory, PersistedStores, StoreName, TotalMaxDatast import { encryptionKey } from './file/Encryption'; import { KeyedFileStore } from './file/KeyedFileStore'; import { recordDiscardedData, recordDiskUsage, recordOutOfDiskFailure, StoreOperation } from './Utils'; +import { isOlderVersionDirectory } from './VersionDirectory'; export class FileStoreFactory implements DataStoreFactory { private readonly log = LoggerFactory.getLogger('FileStore.Global'); @@ -119,7 +120,7 @@ export class FileStoreFactory implements DataStoreFactory { const entries = readdirSync(this.fileDbRoot, { withFileTypes: true }); for (const entry of entries) { try { - if (entry.name !== Version) { + if (entry.isDirectory() && isOlderVersionDirectory(entry.name, VersionNumber)) { this.telemetry.count('oldVersion.cleanup.count', 1); rmSync(join(this.fileDbRoot, entry.name), { recursive: true, force: true }); } diff --git a/src/datastore/LMDBStoreFactory.ts b/src/datastore/LMDBStoreFactory.ts index 8bae0e54..154e33e2 100644 --- a/src/datastore/LMDBStoreFactory.ts +++ b/src/datastore/LMDBStoreFactory.ts @@ -15,6 +15,7 @@ import { LMDBOwnershipTracker } from './lmdb/OwnershipTracker'; import { stats } from './lmdb/Stats'; import { encryptionStrategy } from './lmdb/Utils'; import { recordDiscardedData, recordDiskUsage, recordOutOfDiskFailure, StoreOperation } from './Utils'; +import { isOlderVersionDirectory } from './VersionDirectory'; const MetricsIntervalMs = 60 * 1000; const CleanupDelayMs = 2 * 60 * 1000; @@ -343,12 +344,10 @@ export class LMDBStoreFactory implements DataStoreFactory { const entries = readdirSync(this.lmdbDir, { withFileTypes: true }); for (const entry of entries) { try { - if (entry.name === Version || entry.name === LMDBOwnershipTracker.DirName) { - continue; + if (entry.isDirectory() && isOlderVersionDirectory(entry.name, VersionNumber)) { + this.telemetry.count('oldVersion.cleanup.count', 1); + rmSync(join(this.lmdbDir, entry.name), { recursive: true, force: true }); } - - this.telemetry.count('oldVersion.cleanup.count', 1); - rmSync(join(this.lmdbDir, entry.name), { recursive: true, force: true }); } catch (error) { this.log.error(error, 'Failed to cleanup old LMDB versions'); this.telemetry.count('oldVersion.cleanup.error', 1); diff --git a/src/datastore/VersionDirectory.ts b/src/datastore/VersionDirectory.ts new file mode 100644 index 00000000..fc0a731c --- /dev/null +++ b/src/datastore/VersionDirectory.ts @@ -0,0 +1,24 @@ +const VersionDirectoryPattern = /^v(\d+)$/; + +/** + * True only for a strict `v` directory whose version is below {@link currentVersion}. + * + * Background cleanup runs from whichever binary happens to be executing, and an older binary must + * never delete the store a newer binary is actively using. So this returns false for the current + * version, any newer version, non-version names (markers, stray files, malformed names), and any + * digit string too large to compare as a safe integer — leaving only strictly older stores eligible + * for removal. + */ +export function isOlderVersionDirectory(directoryName: string, currentVersion: number): boolean { + const match = VersionDirectoryPattern.exec(directoryName); + if (match === null) { + return false; + } + + const version = Number(match[1]); + if (!Number.isSafeInteger(version)) { + return false; + } + + return version < currentVersion; +} diff --git a/tst/unit/datastore/FileStore.test.ts b/tst/unit/datastore/FileStore.test.ts index c5b1cb23..34d084b8 100644 --- a/tst/unit/datastore/FileStore.test.ts +++ b/tst/unit/datastore/FileStore.test.ts @@ -548,22 +548,27 @@ describe('FileStore', () => { expect(factory.timeout.hasRef()).toBe(false); }); - it('should cleanup old version directories', () => { + it('should remove older version directories but preserve current, newer, and unrelated directories', () => { const fileDbRoot = join(testDir, 'filedb'); - // Create old version directories mkdirSync(join(fileDbRoot, 'v1'), { recursive: true }); writeFileSync(join(fileDbRoot, 'v1', 'data.enc'), 'old'); + mkdirSync(join(fileDbRoot, 'v4'), { recursive: true }); + writeFileSync(join(fileDbRoot, 'v4', 'data.enc'), 'newer'); + mkdirSync(join(fileDbRoot, 'backup'), { recursive: true }); + writeFileSync(join(fileDbRoot, 'v2'), 'not a directory'); - // Current version should exist from factory constructor + // Current version exists from the factory constructor expect(existsSync(join(fileDbRoot, 'v3'))).toBe(true); - expect(existsSync(join(fileDbRoot, 'v1'))).toBe(true); // Trigger cleanup directly (normally runs after 2min timeout) - (fileFactory as any).cleanupOldVersions(); + (fileFactory as unknown as { cleanupOldVersions(): void }).cleanupOldVersions(); expect(existsSync(join(fileDbRoot, 'v1'))).toBe(false); expect(existsSync(join(fileDbRoot, 'v3'))).toBe(true); + expect(existsSync(join(fileDbRoot, 'v4'))).toBe(true); + expect(existsSync(join(fileDbRoot, 'backup'))).toBe(true); + expect(existsSync(join(fileDbRoot, 'v2'))).toBe(true); }); it('should handle cleanup when directory does not exist', async () => { diff --git a/tst/unit/datastore/LMDB.recovery.test.ts b/tst/unit/datastore/LMDB.recovery.test.ts index 87df4d2e..7b8e8c94 100644 --- a/tst/unit/datastore/LMDB.recovery.test.ts +++ b/tst/unit/datastore/LMDB.recovery.test.ts @@ -239,16 +239,22 @@ describe('LMDB fork detection and recovery', () => { await factory.close(); }); - it('should remove old version directories but preserve the markers directory', () => { + it('should remove older version directories but preserve current, newer, unrelated, and marker directories', () => { const lmdbDir = join(testDir, 'lmdb'); const markersDir = join(lmdbDir, LMDBOwnershipTracker.DirName); fs.mkdirSync(join(lmdbDir, 'v1'), { recursive: true }); + fs.mkdirSync(join(lmdbDir, 'v7'), { recursive: true }); + fs.mkdirSync(join(lmdbDir, 'backup'), { recursive: true }); + fs.writeFileSync(join(lmdbDir, 'v2'), 'not a directory'); (factory as unknown as { cleanupOldVersions(): void }).cleanupOldVersions(); expect(fs.existsSync(join(lmdbDir, 'v1'))).toBe(false); - expect(fs.existsSync(markersDir)).toBe(true); expect(fs.existsSync(join(lmdbDir, 'v6'))).toBe(true); + expect(fs.existsSync(join(lmdbDir, 'v7'))).toBe(true); + expect(fs.existsSync(join(lmdbDir, 'backup'))).toBe(true); + expect(fs.existsSync(join(lmdbDir, 'v2'))).toBe(true); + expect(fs.existsSync(markersDir)).toBe(true); }); }); }); diff --git a/tst/unit/datastore/VersionDirectory.test.ts b/tst/unit/datastore/VersionDirectory.test.ts new file mode 100644 index 00000000..108d9790 --- /dev/null +++ b/tst/unit/datastore/VersionDirectory.test.ts @@ -0,0 +1,41 @@ +import { describe, it, expect } from 'vitest'; +import { isOlderVersionDirectory } from '../../../src/datastore/VersionDirectory'; + +describe('isOlderVersionDirectory', () => { + const currentVersion = 6; + + it('should return true for a strictly older version directory', () => { + expect(isOlderVersionDirectory('v1', currentVersion)).toBe(true); + expect(isOlderVersionDirectory('v5', currentVersion)).toBe(true); + }); + + it('should treat v0 as an older version directory', () => { + expect(isOlderVersionDirectory('v0', currentVersion)).toBe(true); + }); + + it('should return false for the current version directory', () => { + expect(isOlderVersionDirectory('v6', currentVersion)).toBe(false); + }); + + it('should return false for a newer version directory', () => { + expect(isOlderVersionDirectory('v7', currentVersion)).toBe(false); + expect(isOlderVersionDirectory('v100', currentVersion)).toBe(false); + }); + + it('should return false for non-version directory names', () => { + for (const name of ['markers', 'backup', 'lmdb', 'v', 'version1', 'V1', '1', '']) { + expect(isOlderVersionDirectory(name, currentVersion)).toBe(false); + } + }); + + it('should return false for names that only partially match the version format', () => { + for (const name of ['v1.0', 'v-1', 'v1 ', ' v1', 'v1a', 'av1', 'v1/', 'v_1']) { + expect(isOlderVersionDirectory(name, currentVersion)).toBe(false); + } + }); + + it('should return false for a numeric value too large to compare as a safe integer', () => { + const overflowing = `v${'9'.repeat(30)}`; + expect(isOlderVersionDirectory(overflowing, currentVersion)).toBe(false); + }); +}); From 30e204016c4f42673d4128460125577b07c2321f Mon Sep 17 00:00:00 2001 From: Satyaki Ghosh Date: Tue, 25 Aug 2026 15:24:02 -0600 Subject: [PATCH 2/2] add explicity check for lmdb ownership name --- src/datastore/LMDBStoreFactory.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/datastore/LMDBStoreFactory.ts b/src/datastore/LMDBStoreFactory.ts index 154e33e2..43de65d9 100644 --- a/src/datastore/LMDBStoreFactory.ts +++ b/src/datastore/LMDBStoreFactory.ts @@ -344,7 +344,11 @@ export class LMDBStoreFactory implements DataStoreFactory { const entries = readdirSync(this.lmdbDir, { withFileTypes: true }); for (const entry of entries) { try { - if (entry.isDirectory() && isOlderVersionDirectory(entry.name, VersionNumber)) { + if ( + entry.isDirectory() && + isOlderVersionDirectory(entry.name, VersionNumber) && + entry.name !== LMDBOwnershipTracker.DirName + ) { this.telemetry.count('oldVersion.cleanup.count', 1); rmSync(join(this.lmdbDir, entry.name), { recursive: true, force: true }); }