Skip to content

fix(combos): clear LKGP pins when a combo is deleted - #12330

Closed
xiaoyaner0201 wants to merge 2 commits into
diegosouzapw:release/v3.8.51from
xiaoyaner0201:fix/12326-combo-delete-lkgp-cleanup
Closed

xiaoyaner0201 wants to merge 2 commits into
diegosouzapw:release/v3.8.51from
xiaoyaner0201:fix/12326-combo-delete-lkgp-cleanup

Conversation

@xiaoyaner0201

@xiaoyaner0201 xiaoyaner0201 commented Sep 1, 2026 •

Copy link
Copy Markdown
Contributor

Closes #12326.

Problem

deleteCombo() deleted the combos row but left the combo's LKGP pins in key_value. Pins are keyed by ${comboName}:${modelId}, so once the combo is gone those rows become unreachable:

  • the combo itself 404s, so no API or UI path addresses them
  • clearLKGP() needs a modelId the caller no longer knows
  • clearAllLKGP() wipes the namespace for every combo, which is not a targeted cleanup

On an instance with 26 live combos the lkgp namespace held 54 rows — 52 for live combos plus 2 orphans from a combo deleted earlier that day. Roughly 2 rows leak per deleted combo, so the table grows without bound for anyone who creates and deletes combos regularly.

Fix

Provider-connection deletion already handles this via deleteLKGPByConnectionIds() (#8887), and providers/deletion.ts wraps it in try/catch so cleanup failures never block the delete. This applies the same pattern to combo deletion:

  • deleteLKGPByComboName(comboName) in settings/lkgp.ts — deletes every pin under the ${comboName}: prefix and calls invalidateCachedLKGP() for each, so a stale pin cannot be served from the 5s read cache for the rest of its TTL.
  • deleteCombo() resolves the combo name before deleting the row, then clears its pins inside try/catch — a cleanup failure is logged, never fatal to the delete.

The : delimiter in the prefix keeps sibling combos safe (prod does not match prod-canary), and LIKE wildcards in combo names are escaped so a name like temp_a cannot match tempXa.

Tests

New tests/unit/combo-delete-lkgp-cleanup-12326.test.ts, structured after the #8887 suite:

Test Covers
deleting a combo removes its LKGP pins core fix, survivor pins untouched
invalidates warmed read-cache entries the invalidateCachedLKGP() path
prefix-sharing sibling keeps its pins prod vs prod-canary
LIKE wildcards do not widen cleanup temp_a vs tempXa
unknown combo id leaves state untouched early-return branch
combo without pins deletes cleanly no-op path

Verified the tests actually exercise the fix — with the cleanup block removed, 4 of 6 fail; with it restored, all 6 pass.

$ npx tsx --test tests/unit/combo-delete-lkgp-cleanup-12326.test.ts
# tests 6
# pass 6
# fail 0

No regressions in the existing LKGP suites:

$ npx tsx --test tests/unit/delete-provider-connection-invalidates-lkgp-8887.test.ts \
    tests/unit/lkgp-enabled-context-11181.test.ts \
    tests/unit/lkgp-stale-pin-exhaustion-11911.test.ts \
    tests/unit/combo-delete-lkgp-cleanup-12326.test.ts
# tests 18
# pass 18
# fail 0

eslint and prettier --check are clean on the touched files; tsc --noEmit reports no errors in them (the repo's existing baseline errors are unrelated and untouched).

Notes

  • Changelog fragment added at changelog.d/fixes/12330-combo-delete-lkgp-cleanup.md.
  • No behaviour change for combos that are never deleted; the new query only runs on the delete path.

deleteCombo() removed the combos row but left the combo's LKGP pins in
key_value. Pins are keyed by `${comboName}:${modelId}`, so once the combo
is gone the rows are unreachable: the combo 404s, clearLKGP() needs a
modelId the caller no longer has, and clearAllLKGP() is far too broad.

Provider-connection deletion already cleans up its pins via
deleteLKGPByConnectionIds() (diegosouzapw#8887). This applies the same discipline to
combo deletion with deleteLKGPByComboName(), which also invalidates the
read cache so a stale pin cannot be served for the rest of the TTL.

Closes diegosouzapw#12326
@xiaoyaner0201
xiaoyaner0201 force-pushed the fix/12326-combo-delete-lkgp-cleanup branch from 261d121 to 9d5ab8c Compare September 1, 2026 16:18
@xiaoyaner0201

Copy link
Copy Markdown
Contributor Author

The three red checks on this PR are pre-existing base reds on release/v3.8.51, not regressions from this change.

No new ESLint warnings — the three errors are all in tests/unit/stream-passthrough-usage-estimation.test.ts, which this PR does not touch. Reproducible on a clean checkout of the base branch:

$ git checkout upstream/release/v3.8.51
$ npx eslint tests/unit/stream-passthrough-usage-estimation.test.ts --max-warnings 0
  37:10  error  'collectSSE' is defined but never used
  37:21  error  'stream' is defined but never used
  38:17  error  'writable' is defined but never used
✖ 3 problems (3 errors, 0 warnings)          # exit 1

Unit Tests fast-path — the only failing assertion in the shard logs is createSSEStream passthrough forwards OpenAI usage-only empty choices chunks, from the same file.

Both are already being addressed by #12327 ("drain the 2026-09-01 base-red window — passthrough usage regression") and #12331, which touch that same test file.

For this PR's own scope:

$ npx eslint src/lib/db/settings/lkgp.ts \
    src/lib/db/repositories/sqliteComboRepository.ts \
    src/lib/db/settings.ts \
    tests/unit/combo-delete-lkgp-cleanup-12326.test.ts --max-warnings 0
# exit 0, no output

$ DISABLE_SQLITE_AUTO_BACKUP=true node --import tsx/esm \
    --import ./open-sse/utils/setupPolyfill.ts \
    --import ./tests/_setup/isolateDataDir.ts \
    --test tests/unit/combo-delete-lkgp-cleanup-12326.test.ts
# tests 6
# pass 6
# fail 0

Happy to rebase once the base-red PRs land if that makes the run easier to read.

@diegosouzapw

Copy link
Copy Markdown
Owner

Thanks for the PR! Looking at the current release/v3.8.51 tip, this exact fix already
shipped via #12425 ("fix(combos): clear LKGP pins on delete", merged 2026-09-04) —
deleteCombo() in src/lib/db/repositories/sqliteComboRepository.ts already calls
deleteLKGPRowsByComboName() + invalidates the read cache, and the tip even carries a
test file with the same name you added
(tests/unit/combo-delete-lkgp-cleanup-12326.test.ts) covering the same scenarios
(sibling-combo isolation, LIKE-wildcard escaping, cache invalidation). Since #12326 is
already fixed on the branch you're targeting, I'm closing this one as subsumed by #12425
— appreciate you tackling it independently, the approach itself was solid.

Triage note: this is the review recommendation — the close itself happens only after the maintainer's per-PR sign-off (and, where a superseding PR is named, after it has landed). Nothing is being closed by this comment.

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.

fix(backend): deleteCombo() leaves orphaned LKGP entries in key_value

2 participants