Skip to content

fix(db): let current compression engine rows win over legacy keys - #15607

Merged
diegosouzapw merged 4 commits into
diegosouzapw:release/v3.8.52from
woodsonl:fix/compression-legacy-key-shadow
Oct 6, 2026
Merged

diegosouzapw merged 4 commits into
diegosouzapw:release/v3.8.52from
woodsonl:fix/compression-legacy-key-shadow

Conversation

@woodsonl

@woodsonl woodsonl commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

⚠️ base-red inherited: #15306 (release/v3.8.52 is red per the Release-Green tracker; the failures there are not this branch's defect)

Summary

getCompressionSettings (src/lib/db/compression.ts:674) reads the compression namespace with SELECT key, value FROM key_value WHERE namespace = ? and no ORDER BY. Its read cases map the legacy keys aggressiveConfig, ultraConfig and headroomConfig onto the same fields as the current keys aggressive, ultra and headroom, so the row read second wins. The query walks the (namespace, key) primary-key index, and the legacy key always sorts after the current key it shadows. On a database holding a legacy row (manual SQL, an outside tool, or a database written by another build; no app write path creates one), every save on the Aggressive, Ultra or Headroom engine page returned 200 while GET and live compression kept serving the legacy values. A read-only repro confirmed it: with legacy rows present, saving aggressive.maxTokensPerMessage=4096, ultra.compressionRate=0.9 and headroom.minRows=50 still read back 1111, 0.1 and 3.

The fix builds a Set of the keys present in the read and skips a legacy key when its current key is present in the same read. The bug came from relying on unspecified row order, so the fix stays order-independent instead of picking a new ORDER BY.

Behavior kept on purpose:

Commits:

  • 79170cb fix(db): let current compression engine rows win over legacy keys
  • 4309eb3 test(db): cover legacy fallbacks for every engine pair and pin corrupt-row precedence
  • 330bc18 test(db): cover unparseable-JSON corruption modes for engine keys

Test Coverage

CODE PATHS — src/lib/db/compression.ts :: getCompressionSettings
================================================================
 rows = SELECT key,value WHERE namespace='compression' (no ORDER BY)
   ├─ rowKeys = Set(rows→key→string filter)            [★★ TESTED] implicit in all 10 tests
   │    └─ non-string key filter                       [DEFENSIVE-UNREACHABLE] key TEXT NOT NULL — documented
   ├─ row loop: pre-existing skip branches
   │    ├─ BLOB value → warn+continue                  [★★★ TESTED] corrupt current row suppresses legacy (pin)
   │    └─ unparseable JSON → warn+continue            [★★★ TESTED] current row suppresses legacy
   │                                                   [★★  TESTED] legacy-only row → defaults
   ├─ case "aggressive" (unchanged arm)                [★★  TESTED] read-back 4096 after save
   ├─ case "aggressiveConfig" (MODIFIED)
   │    ├─ guard skip (rowKeys.has → true)             [★★★ TESTED] save beats seeded legacy row
   │    └─ guard apply (no current row)                [★★  TESTED] legacy-only fallback
   ├─ case "ultra" (unchanged arm)                     [★★  TESTED] read-back 0.9
   ├─ case "ultraConfig" (MODIFIED)
   │    ├─ guard skip                                  [★★★ TESTED]
   │    └─ guard apply                                 [★★  TESTED]
   ├─ case "headroom" (unchanged arm)                  [★★  TESTED] (+ headroom-minrows-persist-8056)
   ├─ case "headroomConfig" (MODIFIED)
   │    ├─ guard skip                                  [★★★ TESTED]
   │    └─ guard apply                                 [★★  TESTED]
   └─ legacy non-object value → normalizer defaults    [★★  TESTED] (headroom representative)

USER FLOWS
================================================================
 PUT /api/settings/compression → updateCompressionSettings    [★★ TESTED @ DB layer]
   └─ engine-page save wins over legacy row (3 engines)       [★★★ TESTED] write→read round-trip
 GET /api/settings/compression → getCompressionSettings       [★★ TESTED @ DB layer]
 Live compression (chatCore → getCompressionSettings)         [★★ TESTED @ DB layer]
 Route layer (auth/Zod/sanitize) — UNCHANGED by diff          [NOTE] no engine-key route test; not a diff gap

COVERAGE: 13/13 reachable paths tested (100%) | QUALITY: 5×★★★, 5×★★

Tests: 6783 → 6783 tracked test files; the new file holds 10 tests.

Pre-Landing Review

Five specialists (testing, maintainability, security, performance, simplification) plus Claude and Codex adversarial passes reviewed the diff. Five informational findings: three fixed (legacy-fallback coverage for ultra/headroom, tsc-clean test literals, corrupt-row precedence pinned with a test and a comment), two declined with recorded reasons: a warn on every suppressed legacy row (permanent state on a 5s-hot read path; chronic log noise), and legacy-row retirement (needs a migration; separate scope). Codex adversarial: no actionable defects.

Known limits

  • Mixed-version deployments (an older build sharing the same DATA_DIR) still run the order-dependent read in the old build. A one-shot cleanup of legacy rows would let this dual-key read path be deleted later.
  • The suppressed legacy row is dropped silently by design; both rows stay visible in the Storage panel.

Verification Results

  • node:test, 9 compression DB suites: 60 pass / 0 fail (evidence receipt exit 0)
  • npm run test:vitest: pass (receipt exit 0)
  • npm run typecheck:core: clean
  • eslint on changed files with repo suppressions: clean
  • Coverage audit: 13/13 reachable paths (100%), 0 gaps
  • The full unit matrix and the production build run on this PR's CI

Documentation

Status: current — no documentation edits required. Audited the branch diff (3 commits, 2 files) against 12 docs; all reviewed docs remain factually accurate. The fix changes internal DB read precedence that no documentation described before or after.

Documentation debt: legacy engine-row precedence and the corrupt-row reset behavior have zero doc coverage; a note in docs/compression/COMPRESSION_ENGINES.md would fill both.

Test plan

  • node --import tsx/esm --test tests/unit/compression/*.test.ts (9 DB-layer files): 60 pass / 0 fail
  • npm run test:vitest: pass
  • npm run typecheck:core: clean

getCompressionSettings read the compression namespace with no ORDER BY and
mapped the legacy keys aggressiveConfig/ultraConfig/headroomConfig onto the
same fields as aggressive/ultra/headroom, so the row read second won. With
the (namespace, key) primary-key index that was always the longer legacy
key: on a database holding a legacy row (manual SQL, outside tool, or a
database from another build), every engine-page save returned 200 while GET
and live compression kept serving the legacy values.

Skip a legacy key when its current key is present in the same read. A
legacy row with no current counterpart still applies, and a parsed
non-object legacy value still resets the engine to defaults.
…t-row precedence

Review follow-up: the legacy-only fallback was tested for one of three
engine pairs, the test file had partial literals that fail tsc under
Partial<CompressionConfig>'s nested full-shape rule, and the deliberate
presence-based precedence (a corrupt current row suppresses a valid legacy
row) was undocumented. Adds the missing ultra/headroom fallback tests, the
corrupt-row pin, spreads the exported defaults into the literals, and
states the invariant on the read path.
Ship coverage-audit follow-up: pins the second diegosouzapw#13456 corruption mode —
an unparseable-JSON current row still shadows a valid legacy row, and an
unparseable legacy-only row resets the engine to defaults.
@woodsonl
woodsonl requested a review from diegosouzapw as a code owner October 5, 2026 23:42
@diegosouzapw
diegosouzapw merged commit ed9188f into diegosouzapw:release/v3.8.52 Oct 6, 2026
43 of 51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants