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..43de65d9 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,14 @@ 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) && + entry.name !== LMDBOwnershipTracker.DirName + ) { + 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); + }); +});