fix: include tiered billing models in model listing - #4431
Conversation
WalkthroughThe PR refactors model billing configuration detection to include tiered billing expressions. It introduces Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
controller/model_list_test.go (1)
58-94: Nit:initModelListColumnNamesrelies on a subtle post-close invariant.This helper calls
model.InitDB()purely as a side effect to initializecommonGroupCol/commonKeyCol, then closes the connection while leaving those package-level column-name variables set. It works but is easy to break if someone later clears or reassigns those globals duringInitDBteardown.Consider extracting a small dedicated setter (e.g.,
model.SetColumnNamesForSQLite()) to make the intent explicit and the test setup less coupled toInitDB's internals. Optional — fine to defer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/model_list_test.go` around lines 58 - 94, The test helper initModelListColumnNames relies on side effects from model.InitDB to populate package globals commonGroupCol/commonKeyCol and then closes the DB; extract a dedicated setter in model (e.g., model.SetColumnNamesForSQLite or model.InitColumnNamesForSQLite) that accepts the SQLite DSN or sets column-name vars directly, update initModelListColumnNames to call that new setter instead of model.InitDB and removing the DB open/close and SQL_DSN juggling, and keep commonGroupCol/commonKeyCol assignment logic inside the new function so tests no longer depend on InitDB internals.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@model/pricing.go`:
- Around line 80-87: InvalidatePricingCache currently sets pricingMap,
vendorsList, and lastGetPricingTime to nil/zero but leaves dependent caches
(modelSupportEndpointTypes, supportedEndpointMap, modelEnableGroups,
modelQuotaTypeMap) populated causing stale reads; update InvalidatePricingCache
to also nil those dependent maps under the same updatePricingLock (or the
appropriate locks if different) so callers like GetModelSupportEndpointTypes and
GetSupportedEndpointMap see the cleared state immediately, or alternatively
document the narrower invalidation scope—make the change in the
InvalidatePricingCache function and ensure you touch the specific symbols
modelSupportEndpointTypes, supportedEndpointMap, modelEnableGroups,
modelQuotaTypeMap while holding updatePricingLock.
---
Nitpick comments:
In `@controller/model_list_test.go`:
- Around line 58-94: The test helper initModelListColumnNames relies on side
effects from model.InitDB to populate package globals
commonGroupCol/commonKeyCol and then closes the DB; extract a dedicated setter
in model (e.g., model.SetColumnNamesForSQLite or model.InitColumnNamesForSQLite)
that accepts the SQLite DSN or sets column-name vars directly, update
initModelListColumnNames to call that new setter instead of model.InitDB and
removing the DB open/close and SQL_DSN juggling, and keep
commonGroupCol/commonKeyCol assignment logic inside the new function so tests no
longer depend on InitDB internals.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b39e6701-247a-476a-9918-da333037a660
📒 Files selected for processing (5)
controller/model.gocontroller/model_list_test.gomodel/option.gomodel/pricing.gorelay/helper/price.go
| func InvalidatePricingCache() { | ||
| updatePricingLock.Lock() | ||
| defer updatePricingLock.Unlock() | ||
|
|
||
| pricingMap = nil | ||
| vendorsList = nil | ||
| lastGetPricingTime = time.Time{} | ||
| } |
There was a problem hiding this comment.
Stale reads from dependent caches between invalidation and next refresh.
InvalidatePricingCache only clears pricingMap, vendorsList, and lastGetPricingTime, but leaves the related caches populated by updatePricing: modelSupportEndpointTypes, supportedEndpointMap, modelEnableGroups, and modelQuotaTypeMap. Callers that read these maps directly (e.g., GetModelSupportEndpointTypes, GetSupportedEndpointMap) will continue to return pre-invalidation data until the next GetPricing() call triggers a rebuild.
For a billing_setting update this is probably fine (the affected fields are BillingMode/BillingExpr on pricing entries, which do get refreshed via pricingMap=nil), but if this helper is ever reused for a wider config change, the behavior could surprise. Consider either documenting the scope or also nilling the dependent maps under their respective locks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@model/pricing.go` around lines 80 - 87, InvalidatePricingCache currently sets
pricingMap, vendorsList, and lastGetPricingTime to nil/zero but leaves dependent
caches (modelSupportEndpointTypes, supportedEndpointMap, modelEnableGroups,
modelQuotaTypeMap) populated causing stale reads; update InvalidatePricingCache
to also nil those dependent maps under the same updatePricingLock (or the
appropriate locks if different) so callers like GetModelSupportEndpointTypes and
GetSupportedEndpointMap see the cleared state immediately, or alternatively
document the narrower invalidation scope—make the change in the
InvalidatePricingCache function and ensure you touch the specific symbols
modelSupportEndpointTypes, supportedEndpointMap, modelEnableGroups,
modelQuotaTypeMap while holding updatePricingLock.
fix: include tiered billing models in model listing
…edBillingModel Upstream test added in v0.13.1 (QuantumNous#4431) forgot to call setupModelListControllerTestDB(t) in its test body, leaving common.RedisEnabled at its default true. Without Redis actually initialized, cacheGetUserBase nil-deref panics when ListModels walks the user/group cache path for an anonymous (id=0) context. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…g-model-list fix: include tiered billing models in model listing
2 participants
Important
📝 变更描述 / Description
(简述：做了什么？为什么这样改能生效？请基于你对代码逻辑的理解来写，避免粘贴未经整理的内容)
修复配置“阶梯定价”的模型不会出现在 /v1/models 返回结果中的问题。
🚀 变更类型 / Type of change
🔗 关联任务 / Related Issue
/v1/models接口正常返回 #4430✅ 提交前检查项 / Checklist
Bug fix，我已提交或关联对应 Issue，且不会将设计取舍、预期不一致或理解偏差直接归类为 bug。📸 运行证明 / Proof of Work
(请在此粘贴截图、关键日志或测试报告，以证明变更生效)
修改前：
修改后：
Summary by CodeRabbit
Bug Fixes
Tests