fix(provider): gc removed model state#1577
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe PR introduces a provider cleanup lifecycle by adding methods to delete provider model statuses and clear provider model stores, then wires these cleanup operations into the provider removal flow via configurable hooks in ProviderHelper. Changes
Sequence DiagramsequenceDiagram
participant ConfigPresenter
participant ProviderHelper
participant ModelStatusHelper
participant ProviderModelHelper
participant ElectronStore
ConfigPresenter->>ProviderHelper: setCleanupHooks(deleteProviderModelStatuses, clearProviderModelStore)
Note over ProviderHelper: Stores hook callbacks
ConfigPresenter->>ProviderHelper: removeProviderAtomic(providerId)
ProviderHelper->>ElectronStore: Remove provider from persisted list
ProviderHelper->>ModelStatusHelper: deleteProviderModelStatuses(providerId)
ModelStatusHelper->>ElectronStore: Scan & delete model_status_<provider>_* keys
ModelStatusHelper->>ModelStatusHelper: Invalidate provider cache
ProviderHelper->>ProviderModelHelper: clearProviderModelStore(providerId)
ProviderModelHelper->>ElectronStore: clear() provider model store
ProviderModelHelper->>ProviderModelHelper: Remove in-memory store reference
ProviderModelHelper->>ProviderModelHelper: Invalidate models cache
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/presenter/configPresenter/modelStatusHelper.ts`:
- Around line 235-251: The current prefix match in deleteProviderModelStatuses
(using MODEL_STATUS_KEY_PREFIX + providerId + '_') can delete keys for other
providers with IDs containing that providerId; fix by changing the delimiter
used when composing model-status keys to a collision-safe token (e.g., replace
the '_' with a delimiter like '##' or encode providerId) and update all places
that create/read model-status keys accordingly: update MODEL_STATUS_KEY_PREFIX
usage and every key-creation site to use
`${MODEL_STATUS_KEY_PREFIX}${providerId}##` (or use
encodeURIComponent/base64(providerId) when composing keys), then change
deleteProviderModelStatuses to compute that exact prefix and only delete keys
starting with that new prefix, leaving clearProviderModelStatusCache and
store.delete calls intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: af272571-cb57-4f88-8495-48ed4e8c372f
📒 Files selected for processing (7)
src/main/presenter/configPresenter/index.tssrc/main/presenter/configPresenter/modelStatusHelper.tssrc/main/presenter/configPresenter/providerHelper.tssrc/main/presenter/configPresenter/providerModelHelper.tstest/main/presenter/configPresenter/modelStatusHelper.test.tstest/main/presenter/configPresenter/providerHelper.test.tstest/main/presenter/configPresenter/providerModelHelper.test.ts
Comment on lines
+235
to
+251
| deleteProviderModelStatuses(providerId: string): void { | ||
| const prefix = `${MODEL_STATUS_KEY_PREFIX}${providerId}_` | ||
| const candidate = this.store as ElectronStore<any> & { | ||
| store?: Record<string, unknown> | ||
| } | ||
| const rawStore = candidate.store | ||
|
|
||
| if (rawStore && typeof rawStore === 'object') { | ||
| for (const key of Object.keys(rawStore)) { | ||
| if (key.startsWith(prefix)) { | ||
| this.store.delete(key) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| this.clearProviderModelStatusCache(providerId) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Inspect key construction/deletion logic"
rg -n -C3 "MODEL_STATUS_KEY_PREFIX|getStatusKey|deleteProviderModelStatuses|clearProviderModelStatusCache" src/main/presenter/configPresenter/modelStatusHelper.ts
echo
echo "2) Locate provider ID validation rules or schema constraints"
rg -n --type=ts --type=tsx -C3 "providerId|id.*provider|addProvider|setProviderById|updateProviderAtomic|regex\\(|z\\.string\\(" src testRepository: ThinkInAIXYZ/deepchat
Length of output: 3625
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for provider schema/type definitions ==="
rg -n "providerId.*schema|providerId.*z\.|interface.*Provider|type.*Provider" src --type=ts 2>/dev/null | head -30
echo
echo "=== Search for provider validation and creation logic ==="
rg -n "addProvider|createProvider|validateProvider|providerId.*=" src --type=ts 2>/dev/null | head -30
echo
echo "=== Look for LLMProvider class/interface definitions ==="
fd -name "*[Pp]rovider*.ts" src | head -20Repository: ThinkInAIXYZ/deepchat
Length of output: 7167
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Read ProviderSchema definition ==="
sed -n '140,180p' src/shared/types/model-db.ts
echo
echo "=== Search for provider ID character restrictions ==="
rg -n "providerId.*regex|providerId.*match|id.*regex|z\.string\(\).*regex|pattern" src/shared/types/model-db.ts
echo
echo "=== Look for any provider ID sanitization/validation ==="
rg -n "trim\(\)|toLowerCase\(\)|sanitize.*provider|validate.*provider" src --type=ts | grep -i "id" | head -20Repository: ThinkInAIXYZ/deepchat
Length of output: 1154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Find ProviderSchema definition ==="
rg -n "ProviderSchema\s*=" src/shared/types/model-db.ts -A 15
echo
echo "=== Check what characters are allowed in provider IDs ==="
rg -n "id.*z\.|providerId.*z\." src/shared/types/model-db.ts -B 2 -A 2
echo
echo "=== Check LLM_PROVIDER interface/type ==="
fd -name "*.ts" src | xargs rg -l "LLM_PROVIDER|interface.*Provider" | head -5 | xargs rg -n "interface LLM_PROVIDER|type LLM_PROVIDER" -A 20Repository: ThinkInAIXYZ/deepchat
Length of output: 8211
Prefix-based deletion can cause cross-provider data loss.
The deleteProviderModelStatuses() method uses a simple prefix match (model_status_${providerId}_) to delete status keys. Since provider IDs have no character restrictions (ProviderSchema only requires z.string().min(1)), this creates a collision risk: deleting provider foo will also delete statuses for provider foo_bar because the prefix model_status_foo_ matches both model_status_foo_xyz and model_status_foo_bar_xyz.
The fix requires either: (1) validate provider IDs to exclude underscores, (2) use exact key matching instead of prefix matching, or (3) use a delimiter that cannot appear in provider IDs (e.g., ## or a null byte).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/presenter/configPresenter/modelStatusHelper.ts` around lines 235 -
251, The current prefix match in deleteProviderModelStatuses (using
MODEL_STATUS_KEY_PREFIX + providerId + '_') can delete keys for other providers
with IDs containing that providerId; fix by changing the delimiter used when
composing model-status keys to a collision-safe token (e.g., replace the '_'
with a delimiter like '##' or encode providerId) and update all places that
create/read model-status keys accordingly: update MODEL_STATUS_KEY_PREFIX usage
and every key-creation site to use `${MODEL_STATUS_KEY_PREFIX}${providerId}##`
(or use encodeURIComponent/base64(providerId) when composing keys), then change
deleteProviderModelStatuses to compute that exact prefix and only delete keys
starting with that new prefix, leaving clearProviderModelStatusCache and
store.delete calls intact.
zerob13
added a commit
that referenced
this pull request
Apr 30, 2026
* refactor(model): derive selectable model source * refactor(model): unify selection resolver (#1576) * refactor(chat): split acp status bar state (#1578) * refactor(model): unify selection resolver * refactor(chat): extract acp status bar state * perf(agent): refresh acp agents incrementally (#1579) * fix(provider): gc removed model state (#1577) * fix: address pr review feedback --------- Co-authored-by: zerob13 <zerob13@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary\n- clean persisted model status entries when a provider is removed\n- clear the provider-specific stored model/custom-model payload at the same time\n- wire provider removal through cleanup hooks and cover the helper behavior with focused main-process tests\n\n## Testing\n- commit hook ran oxfmt and typecheck automatically during git commit\n
Summary by CodeRabbit
Bug Fixes
Tests