From 5455740faaaaea4d457ec529e5cb00df292fd83c Mon Sep 17 00:00:00 2001 From: Koosha Paridehpour <42529354+KooshaPari@users.noreply.github.com> Date: Thu, 17 Sep 2026 06:43:37 -0700 Subject: [PATCH] fix(compression): log warnings for unreadable settings rows (#13522) * fix(compression): log warnings for unreadable settings rows getCompressionSettings() silently skipped non-string (BLOB) and invalid-JSON settings rows, making it impossible to diagnose config drift between the panel and the runtime. Now logs a warn-level message for each unreadable row, including the key name and a remediation hint (re-save from the Storage panel). Also warns when the 'engines' row exists but yields no valid toggles, so operators know their panel-configured engines map is being silently replaced by the legacy fallback. Fixes #13456 * test(compression): cover getCompressionSettings warnings for unreadable rows The test for #13456 only asserted a stubbed console.warn recorded a message and never called getCompressionSettings(), so it never exercised the production change. Seed a BLOB row, an invalid-JSON row, and an 'engines' row that isn't a usable object, and assert the resulting warnings; also assert a legitimately empty (but valid) 'engines' map does not warn. Also stop warning on a valid-but-empty 'engines' row: parseStoredEnginesMap returns null both for an unreadable row and for a well-formed {} (an operator who deliberately disabled every engine), so only warn when the stored value isn't a usable object at all. Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com> --------- Co-authored-by: Koosha Pari Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com> --- src/lib/db/compression.ts | 37 +++++- .../compression-settings-warn.test.ts | 115 ++++++++++++++++++ 2 files changed, 149 insertions(+), 3 deletions(-) create mode 100644 tests/unit/compression/compression-settings-warn.test.ts diff --git a/src/lib/db/compression.ts b/src/lib/db/compression.ts index 9f8d7b042d..6799478532 100644 --- a/src/lib/db/compression.ts +++ b/src/lib/db/compression.ts @@ -656,9 +656,27 @@ export async function getCompressionSettings(): Promise { const record = toRecord(row); const key = typeof record.key === "string" ? record.key : null; const rawValue = typeof record.value === "string" ? record.value : null; - if (!key || rawValue === null) continue; + if (!key || rawValue === null) { + // #13456: non-string values (BLOB from backup/restore/migration tooling) are + // silently ignored — log so operators can diagnose config drift. + if (key && typeof record.value !== "string" && record.value !== null) { + console.warn( + `[COMPRESSION] Settings row '${key}' has non-string value type ` + + `(${typeof record.value}); skipping. This may indicate a backup/restore ` + + `issue — re-save the setting from the Storage panel to fix.` + ); + } + continue; + } const parsed = parseJsonSafe(rawValue); - if (parsed === undefined) continue; + if (parsed === undefined) { + // #13456: invalid JSON is also silently ignored — log it. + console.warn( + `[COMPRESSION] Settings row '${key}' has unparseable JSON value; skipping. ` + + `Re-save the setting from the Storage panel to fix.` + ); + continue; + } switch (key) { case "enabled": @@ -768,6 +786,16 @@ export async function getCompressionSettings(): Promise { break; case "engines": storedEngines = parseStoredEnginesMap(parsed); + // #13456: only warn when the row itself isn't a usable object — a valid object + // that simply yields zero toggles (e.g. `{}`, an operator deliberately disabling + // every engine) is legitimate config, not a parse failure, and must not warn. + if (storedEngines === null && (!parsed || typeof parsed !== "object")) { + console.warn( + `[COMPRESSION] 'engines' settings row is present but unreadable; ` + + `falling back to legacy settings. Re-save the engines map from the ` + + `Storage panel to fix.` + ); + } break; case "activeComboId": config.activeComboId = typeof parsed === "string" && parsed.trim() ? parsed.trim() : null; @@ -917,7 +945,10 @@ let proactiveRatioCache: { value: number; readAt: number } | null = null; export function getProactiveCompressionRatio(): number { const now = Date.now(); - if (proactiveRatioCache && now - proactiveRatioCache.readAt < PROACTIVE_COMPRESSION_CACHE_TTL_MS) { + if ( + proactiveRatioCache && + now - proactiveRatioCache.readAt < PROACTIVE_COMPRESSION_CACHE_TTL_MS + ) { return proactiveRatioCache.value; } let ratio = PROACTIVE_COMPRESSION_DEFAULT_RATIO; diff --git a/tests/unit/compression/compression-settings-warn.test.ts b/tests/unit/compression/compression-settings-warn.test.ts new file mode 100644 index 0000000000..4061b13b5d --- /dev/null +++ b/tests/unit/compression/compression-settings-warn.test.ts @@ -0,0 +1,115 @@ +/** + * Tests for #13456: compression settings row warnings for unreadable values. + * + * Before the fix, non-string (BLOB) and invalid-JSON settings rows were + * silently ignored. Operators had no way to diagnose config drift from the + * panel vs. the runtime. + */ +import { test, after, beforeEach } 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-compression-warn-")); +const ORIGINAL_DATA_DIR = process.env.DATA_DIR; +process.env.DATA_DIR = TEST_DATA_DIR; + +const { getDbInstance, resetDbInstance } = await import("../../../src/lib/db/core.ts"); +const { getCompressionSettings } = await import("../../../src/lib/db/compression.ts"); + +const warnings: string[] = []; +const originalWarn = console.warn; + +function freshDir() { + resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 }); + fs.mkdirSync(TEST_DATA_DIR, { recursive: true }); +} + +beforeEach(() => { + warnings.length = 0; + console.warn = (...args: unknown[]) => { + warnings.push(args.join(" ")); + }; +}); + +after(() => { + console.warn = originalWarn; + resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 }); + if (ORIGINAL_DATA_DIR === undefined) { + delete process.env.DATA_DIR; + } else { + process.env.DATA_DIR = ORIGINAL_DATA_DIR; + } +}); + +test("warns when a settings row is stored as a non-string (BLOB) value", async () => { + freshDir(); + const db = getDbInstance(); + db.prepare("INSERT OR REPLACE INTO key_value (namespace, key, value) VALUES (?, ?, ?)").run( + "compression", + "cacheMinutes", + Buffer.from("corrupt-blob") + ); + + await getCompressionSettings(); + + assert.ok( + warnings.some((w) => w.includes("cacheMinutes") && w.includes("non-string value type")), + `expected a non-string-value warning for 'cacheMinutes', got: ${JSON.stringify(warnings)}` + ); +}); + +test("warns when a settings row has unparseable JSON", async () => { + freshDir(); + const db = getDbInstance(); + db.prepare("INSERT OR REPLACE INTO key_value (namespace, key, value) VALUES (?, ?, ?)").run( + "compression", + "cacheMinutes", + "{not valid json" + ); + + await getCompressionSettings(); + + assert.ok( + warnings.some((w) => w.includes("cacheMinutes") && w.includes("unparseable JSON")), + `expected an unparseable-JSON warning for 'cacheMinutes', got: ${JSON.stringify(warnings)}` + ); +}); + +test("warns when the 'engines' row is not a usable object", async () => { + freshDir(); + const db = getDbInstance(); + db.prepare("INSERT OR REPLACE INTO key_value (namespace, key, value) VALUES (?, ?, ?)").run( + "compression", + "engines", + "42" + ); + + await getCompressionSettings(); + + assert.ok( + warnings.some((w) => w.includes("'engines'") && w.includes("unreadable")), + `expected an unreadable-engines warning, got: ${JSON.stringify(warnings)}` + ); +}); + +test("does NOT warn when the 'engines' row is a valid but empty object", async () => { + freshDir(); + const db = getDbInstance(); + db.prepare("INSERT OR REPLACE INTO key_value (namespace, key, value) VALUES (?, ?, ?)").run( + "compression", + "engines", + "{}" + ); + + await getCompressionSettings(); + + assert.equal( + warnings.some((w) => w.includes("'engines'")), + false, + `a deliberately empty (but valid) engines map must not warn, got: ${JSON.stringify(warnings)}` + ); +});