From b6d2b4a41c2ee0bbe64fd5d9d782ed0fd5ed2046 Mon Sep 17 00:00:00 2001 From: Nick Sullivan <142708+TechNickAI@users.noreply.github.com> Date: Mon, 17 Aug 2026 06:10:13 -0500 Subject: [PATCH] Compression telemetry retention has never deleted a row (same unit bug as #9625) (#10559) * fix(db): align compression_run_telemetry cleanup cutoff with millisecond column cleanupCompressionRunTelemetry() computed its cutoff in epoch seconds while insertCompressionRunTelemetryRow() stamps the timestamp column with Date.now() (epoch milliseconds). A millisecond timestamp is ~1000x larger than a seconds cutoff, so DELETE WHERE timestamp < cutoff never matched an old row and the retention sweep added by #6848 to bound storage.sqlite growth was inert. This is the same defect as domain_cost_history (#9625), whose fix corrected cleanupDomainCostHistory() ~90 lines earlier in this file and missed this sibling call site. The stale docstring asserting a unix-epoch column is corrected too. The repro test seeds through the real writer to establish the stored unit, so it also fails if the producer format diverges from the consumer again. * docs(changelog): add fragment for the telemetry retention unit fix --- .../compression-run-telemetry-retention-ms.md | 1 + src/lib/db/cleanup.ts | 6 +- ...repro-compression-run-telemetry-ms.test.ts | 99 +++++++++++++++++++ .../unit/telemetry-auto-cleanup-6848.test.ts | 13 ++- 4 files changed, 110 insertions(+), 9 deletions(-) create mode 100644 changelog.d/fixes/compression-run-telemetry-retention-ms.md create mode 100644 tests/unit/repro-compression-run-telemetry-ms.test.ts diff --git a/changelog.d/fixes/compression-run-telemetry-retention-ms.md b/changelog.d/fixes/compression-run-telemetry-retention-ms.md new file mode 100644 index 0000000000..cbaedbe25e --- /dev/null +++ b/changelog.d/fixes/compression-run-telemetry-retention-ms.md @@ -0,0 +1 @@ +- **fix(db):** the `compression_run_telemetry` retention sweep now actually deletes expired rows. Its cutoff was computed in epoch seconds while the column stores epoch milliseconds, so `WHERE timestamp < cutoff` never matched and the table added by #6848 to bound `storage.sqlite` growth was unbounded in practice. Same unit mismatch as #9625, which corrected the sibling `domain_cost_history` sweep and missed this call site diff --git a/src/lib/db/cleanup.ts b/src/lib/db/cleanup.ts index 39e044b0ec..617bf4c226 100644 --- a/src/lib/db/cleanup.ts +++ b/src/lib/db/cleanup.ts @@ -344,14 +344,16 @@ export async function cleanupXpAuditLog(): Promise { /** * Clean up old compression_run_telemetry based on retention settings. (#6848) - * Uses unix-epoch `timestamp` column (INTEGER). + * The `timestamp` column stores epoch milliseconds (recordCompressionRun stamps + * Date.now()), so the cutoff must be in milliseconds to match. Same unit bug as + * domain_cost_history (#9625), which this function was missed by. */ export async function cleanupCompressionRunTelemetry(): Promise { const db = getDbInstance(); const retention = getRetentionSettings(); const retentionDays = retention.compressionRunTelemetry; - const cutoffEpoch = Math.floor(Date.now() / 1000) - retentionDays * 86_400; + const cutoffEpoch = Date.now() - retentionDays * 86_400_000; const result: CleanupResult = { deleted: 0, errors: 0 }; diff --git a/tests/unit/repro-compression-run-telemetry-ms.test.ts b/tests/unit/repro-compression-run-telemetry-ms.test.ts new file mode 100644 index 0000000000..143ef0037c --- /dev/null +++ b/tests/unit/repro-compression-run-telemetry-ms.test.ts @@ -0,0 +1,99 @@ +/** + * compression_run_telemetry cleanup cutoff unit mismatch. + * + * cleanupCompressionRunTelemetry() computed its cutoff in epoch seconds + * (Math.floor(Date.now() / 1000)) while the timestamp column stores epoch + * milliseconds, stamped by insertCompressionRunTelemetryRow() as Date.now(). + * + * This is the same defect as domain_cost_history (#9625). That fix corrected + * cleanupDomainCostHistory() ~90 lines earlier in cleanup.ts and missed this + * sibling call site, so the retention sweep added by #6848 to bound + * storage.sqlite growth has been permanently inert: a millisecond timestamp is + * ~1000x larger than a seconds cutoff, so `WHERE timestamp < cutoff` never + * matched an old row. + * + * The first test uses the REAL writer to establish the stored unit, so it + * cannot pass if the producer's format ever changes independently. + */ + +import test from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + +const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-crt-ms-")); +process.env.DATA_DIR = TEST_DATA_DIR; + +const { cleanupCompressionRunTelemetry } = await import("../../src/lib/db/cleanup.ts"); +const { insertCompressionRunTelemetryRow } = await import( + "../../src/lib/db/compressionRunTelemetry.ts" +); +const { getDbInstance, resetDbInstance } = await import("../../src/lib/db/core.ts"); + +test.after(() => { + resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); +}); + +const DAY_MS = 86_400_000; + +function seedRow(requestId: string): void { + insertCompressionRunTelemetryRow({ + requestId, + model: "m", + provider: "p", + source: "s", + tokensBefore: 100, + tokensAfter: 50, + ratio: 0.5, + }); +} + +test("the writer stamps epoch milliseconds, not seconds", () => { + seedRow("req-unit-probe"); + const db = getDbInstance()!; + const row = db + .prepare("SELECT timestamp FROM compression_run_telemetry WHERE request_id = ?") + .get("req-unit-probe") as { timestamp: number }; + + // A seconds stamp is ~1.7e9; a milliseconds stamp is ~1.7e12. + assert.ok( + row.timestamp > 1e12, + `timestamp ${row.timestamp} is not millisecond-scale; the cleanup cutoff unit must match the writer` + ); +}); + +test("cleanupCompressionRunTelemetry deletes rows older than the retention window", async () => { + const db = getDbInstance()!; + db.exec("DELETE FROM compression_run_telemetry"); + + // Insert through the real writer, then backdate to simulate age. Backdating + // preserves the producer's unit while letting the test control the age. + const now = Date.now(); + for (const id of ["old-1", "old-2", "old-3"]) { + seedRow(id); + db.prepare("UPDATE compression_run_telemetry SET timestamp = ? WHERE request_id = ?").run( + now - 40 * DAY_MS, + id + ); + } + for (const id of ["new-1", "new-2"]) { + seedRow(id); + db.prepare("UPDATE compression_run_telemetry SET timestamp = ? WHERE request_id = ?").run( + now - 5 * DAY_MS, + id + ); + } + + const result = await cleanupCompressionRunTelemetry(); + + // With the pre-fix seconds cutoff this asserted 0 deleted: the sweep was a no-op. + assert.strictEqual(result.deleted, 3, "should delete the 3 rows aged 40 days"); + assert.strictEqual(result.errors, 0); + + const remaining = db + .prepare("SELECT COUNT(*) as cnt FROM compression_run_telemetry") + .get() as { cnt: number }; + assert.strictEqual(remaining.cnt, 2, "should keep the 2 rows aged 5 days"); +}); diff --git a/tests/unit/telemetry-auto-cleanup-6848.test.ts b/tests/unit/telemetry-auto-cleanup-6848.test.ts index 2952304628..06491e2404 100644 --- a/tests/unit/telemetry-auto-cleanup-6848.test.ts +++ b/tests/unit/telemetry-auto-cleanup-6848.test.ts @@ -47,7 +47,6 @@ test.after(() => { fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); }); -const DAY_SECONDS = 86_400; const DAY_MS = 86_400_000; /** Ensure compression_run_telemetry table exists (created lazily in production). */ @@ -161,14 +160,15 @@ test("#6848 cleanupCompressionRunTelemetry: deletes rows older than retention wi ensureTelemetryTable(); const db = getDbInstance()!; const now = Date.now(); - const nowSeconds = Math.floor(now / 1000); const insert = db.prepare( "INSERT INTO compression_run_telemetry (timestamp, tokens_before, tokens_after) VALUES (?, ?, ?)" ); - insert.run(nowSeconds - 40 * DAY_SECONDS, 1000, 500); - insert.run(nowSeconds - 40 * DAY_SECONDS, 2000, 800); - insert.run(nowSeconds - 5 * DAY_SECONDS, 1500, 600); + // insertCompressionRunTelemetryRow() stamps Date.now() — epoch MILLISECONDS. + // Seeding seconds here encoded the same unit mismatch the cleanup had. + insert.run(now - 40 * DAY_MS, 1000, 500); + insert.run(now - 40 * DAY_MS, 2000, 800); + insert.run(now - 5 * DAY_MS, 1500, 600); const result = await cleanupCompressionRunTelemetry(); @@ -184,7 +184,6 @@ test("#6848 cleanupCompressionRunTelemetry: deletes rows older than retention wi test("#6848 no rows deleted when all data is within retention window (calls all 4 real functions)", async () => { ensureTelemetryTable(); const db = getDbInstance()!; - const nowSeconds = Math.floor(Date.now() / 1000); const nowMilliseconds = Date.now(); const recentISO = new Date().toISOString(); @@ -201,7 +200,7 @@ test("#6848 no rows deleted when all data is within retention window (calls all ).run("k", "a", 5, recentISO); db.prepare( "INSERT INTO compression_run_telemetry (timestamp, tokens_before, tokens_after) VALUES (?, ?, ?)" - ).run(nowSeconds - DAY_SECONDS, 100, 50); + ).run(nowMilliseconds - DAY_MS, 100, 50); const r1 = await cleanupDomainCostHistory(); const r2 = await cleanupCompressionCacheStats();