Skip to content

feat: add UpsertModelParametersBatch for batched model parameter sync - #4800

Merged
akshaydeo merged 2 commits into
devfrom
06-30-fix_sync_batch_model_parameters_and_remove_mcp_library_sync_from_the_model_force_sync_endpoint
Jul 1, 2026
Merged

feat: add UpsertModelParametersBatch for batched model parameter sync#4800
akshaydeo merged 2 commits into
devfrom
06-30-fix_sync_batch_model_parameters_and_remove_mcp_library_sync_from_the_model_force_sync_endpoint

Conversation

@impoiler

@impoiler impoiler commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Summary

Replaces the per-row transactional upsert loop used during model parameter sync with a single batched upsert, reducing DB round-trips from N (one per model) to a small number of batched INSERT ... ON CONFLICT statements.

Changes

  • Added UpsertModelParametersBatch to RDBConfigStore and the ConfigStore interface, which accepts a slice of TableModelParameters and upserts them in batches of 100 using GORM's CreateInBatches with an ON CONFLICT clause.
  • Replaced the ExecuteTransaction + UpsertModelParameters loop in SyncModelParamsFromURL with a single call to UpsertModelParametersBatch, removing the need to open an explicit transaction and iterate row-by-row.
  • Removed the MCP library sync goroutine from ForceReloadPricing, which was being launched concurrently alongside pricing/params sync.
  • Added UpsertModelParametersBatch stub to MockConfigStore to satisfy the updated interface.
  • Added TestUpsertModelParametersBatch_SQLite covering initial insert, upsert (update existing row), and no-duplicate behavior.

Type of change

  • Bug fix
  • Feature
  • Refactor
  • Documentation
  • Chore/CI

Affected areas

  • Core (Go)
  • Transports (HTTP)
  • Providers/Integrations
  • Plugins
  • UI (React)
  • Docs

How to test

go test ./framework/configstore/... -run TestUpsertModelParametersBatch_SQLite -v
go test ./framework/modelcatalog/...
go test ./transports/bifrost-http/lib/...

Expected: all tests pass; TestUpsertModelParametersBatch_SQLite verifies that three records are inserted, an update to one record is reflected correctly, and the total row count remains 3 after the upsert.

Verified using a cross-region remote database to simulate a real scenario, rather than having a local database on the same machine.

Before
image.png

After
image.png

Breaking changes

  • Yes
  • No

Related issues

Security considerations

No auth, secrets, or PII implications. The batch upsert uses parameterized queries via GORM, consistent with existing patterns.

Checklist

  • I read docs/contributing/README.md and followed the guidelines
  • I added/updated tests where appropriate
  • I updated documentation where needed
  • I verified builds succeed (Go and UI)
  • I verified the CI pipeline passes locally if applicable

@coderabbitai

coderabbitai Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 43 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 2f9f5a07-e10c-42c3-8b0b-8f375be86fae

📥 Commits

Reviewing files that changed from the base of the PR and between c74e500 and 255e8e0.

📒 Files selected for processing (6)
  • framework/configstore/rdb.go
  • framework/configstore/rdb_test.go
  • framework/configstore/store.go
  • framework/modelcatalog/datasheet/params.go
  • framework/modelcatalog/main.go
  • transports/bifrost-http/lib/config_test.go
📝 Walkthrough

Walkthrough

Adds batch model-parameter upsert support to the config store, switches model-parameter syncing to use it, removes MCP library sync from forced pricing reload, and updates tests and mocks.

Changes

Batch Model Parameters Upsert

Layer / File(s) Summary
ConfigStore interface and RDB implementation
framework/configstore/store.go, framework/configstore/rdb.go
ConfigStore gains UpsertModelParametersBatch; RDBConfigStore implements it with batch sizing, deduplication, CreateInBatches, and optional transaction reuse.
SyncModelParamsFromURL refactor and ForceReloadPricing cleanup
framework/modelcatalog/datasheet/params.go, framework/modelcatalog/main.go
SyncModelParamsFromURL builds TableModelParameters rows and calls UpsertModelParametersBatch; ForceReloadPricing removes the syncMCPLibrary goroutine.
Tests and mock stub
framework/configstore/rdb_test.go, transports/bifrost-http/lib/config_test.go
TestUpsertModelParametersBatch_SQLite covers insert, update, and duplicate-model behavior; MockConfigStore gains a stub for the new batch method.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested reviewers: akshaydeo, danpiths

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: adding batched model parameter upserts.
Description check ✅ Passed The description matches the template well and covers summary, changes, testing, impact, security, and checklist items.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch 06-30-fix_sync_batch_model_parameters_and_remove_mcp_library_sync_from_the_model_force_sync_endpoint

Comment @coderabbitai help to get the list of available commands.

@mintlify

mintlify Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
Bifrost 🟡 Building Jun 30, 2026, 11:00 AM

💡 Tip: Enable Workflows to automatically generate PRs for you.

@impoiler
impoiler changed the base branch from main to graphite-base/4800 June 30, 2026 11:13
@impoiler
impoiler force-pushed the 06-30-fix_sync_batch_model_parameters_and_remove_mcp_library_sync_from_the_model_force_sync_endpoint branch from cc76ad9 to 0008078 Compare June 30, 2026 11:13
@impoiler
impoiler changed the base branch from graphite-base/4800 to 06-25-docs_security-best-practices_docs June 30, 2026 11:13
@impoiler impoiler changed the title Fix: Sync batch model parameters and remove MCP library sync from the model force sync endpoint feat: add UpsertModelParametersBatch for batched model parameter sync Jun 30, 2026
@impoiler impoiler self-assigned this Jun 30, 2026
@impoiler
impoiler marked this pull request as ready for review June 30, 2026 11:14
@coderabbitai
coderabbitai Bot requested review from akshaydeo and danpiths June 30, 2026 11:15

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 54232115-88b4-4e67-9349-9be14a86f72c

📥 Commits

Reviewing files that changed from the base of the PR and between 541bf43 and 0008078.

📒 Files selected for processing (6)
  • framework/configstore/rdb.go
  • framework/configstore/rdb_test.go
  • framework/configstore/store.go
  • framework/modelcatalog/datasheet/params.go
  • framework/modelcatalog/main.go
  • transports/bifrost-http/lib/config_test.go
💤 Files with no reviewable changes (1)
  • framework/modelcatalog/main.go

Comment thread framework/configstore/rdb_test.go
@impoiler
impoiler force-pushed the 06-30-fix_sync_batch_model_parameters_and_remove_mcp_library_sync_from_the_model_force_sync_endpoint branch from 0008078 to 11d1653 Compare June 30, 2026 11:20
@greptile-apps

greptile-apps Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Confidence Score: 4/5

The batch upsert implementation is correct and well-tested, but ForceReloadPricing now silently omits the MCP library sync that it previously performed.

The new UpsertModelParametersBatch implementation is consistent with the existing UpsertModelPricesBatch pattern, deduplication logic is correct, transaction wrapping preserves atomicity across all batches, and the SQLite note addresses prior feedback. The sole concern is the removal of syncMCPLibrary from ForceReloadPricing flagged in prior review: operators who call the force-reload endpoint expecting a full catalog refresh (pricing + params + MCP library) now silently skip the MCP sync, with the background worker as the only remaining trigger (up to an hour later).

framework/modelcatalog/main.go — the MCP library sync removal from ForceReloadPricing warrants a second look before merging.

Important Files Changed

Filename Overview
framework/configstore/rdb.go Adds UpsertModelParametersBatch: deduplication (last-write-wins), optional external transaction, and a single CreateInBatches ON CONFLICT upsert. Implementation is consistent with UpsertModelPricesBatch and includes the SQLite compatibility note from prior review feedback.
framework/configstore/rdb_test.go Adds TestUpsertModelParametersBatch_SQLite covering initial insert, upsert-update, duplicate-key deduplication (last-wins), and no-duplicate row-count assertion. Good coverage for the new batch path.
framework/configstore/store.go Extends ConfigStore interface with UpsertModelParametersBatch. One-line change; all implementations updated.
framework/modelcatalog/datasheet/params.go Replaces the per-row transactional loop in SyncModelParamsFromURL with a single UpsertModelParametersBatch call. Atomicity is preserved by the batch method's internal transaction.
framework/modelcatalog/main.go Removes the MCP library sync goroutine from ForceReloadPricing. The sync still runs on the background ticker (hourly), but a forced reload no longer triggers it — a behavioral gap flagged in previous review comments.
transports/bifrost-http/lib/config_test.go Adds no-op UpsertModelParametersBatch stub to MockConfigStore to satisfy the updated interface.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Caller
    participant SyncModelParamsFromURL
    participant UpsertModelParametersBatch
    participant GORM
    participant DB

    Caller->>SyncModelParamsFromURL: SyncModelParamsFromURL(ctx)
    SyncModelParamsFromURL->>SyncModelParamsFromURL: fetch + parse paramsData (N models)
    SyncModelParamsFromURL->>SyncModelParamsFromURL: build []TableModelParameters records
    SyncModelParamsFromURL->>UpsertModelParametersBatch: UpsertModelParametersBatch(ctx, records)
    UpsertModelParametersBatch->>UpsertModelParametersBatch: dedup by model (last-write-wins)
    UpsertModelParametersBatch->>GORM: db.Transaction(upsert)
    loop batches of 100
        GORM->>DB: INSERT INTO ... ON CONFLICT (model) DO UPDATE SET ...
    end
    DB-->>GORM: OK
    GORM-->>UpsertModelParametersBatch: nil
    UpsertModelParametersBatch-->>SyncModelParamsFromURL: nil
    SyncModelParamsFromURL-->>Caller: nil
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant Caller
    participant SyncModelParamsFromURL
    participant UpsertModelParametersBatch
    participant GORM
    participant DB

    Caller->>SyncModelParamsFromURL: SyncModelParamsFromURL(ctx)
    SyncModelParamsFromURL->>SyncModelParamsFromURL: fetch + parse paramsData (N models)
    SyncModelParamsFromURL->>SyncModelParamsFromURL: build []TableModelParameters records
    SyncModelParamsFromURL->>UpsertModelParametersBatch: UpsertModelParametersBatch(ctx, records)
    UpsertModelParametersBatch->>UpsertModelParametersBatch: dedup by model (last-write-wins)
    UpsertModelParametersBatch->>GORM: db.Transaction(upsert)
    loop batches of 100
        GORM->>DB: INSERT INTO ... ON CONFLICT (model) DO UPDATE SET ...
    end
    DB-->>GORM: OK
    GORM-->>UpsertModelParametersBatch: nil
    UpsertModelParametersBatch-->>SyncModelParamsFromURL: nil
    SyncModelParamsFromURL-->>Caller: nil
Loading

Reviews (4): Last reviewed commit: "Fix: Sync batch model parameters and rem..." | Re-trigger Greptile

coderabbitai[bot]
coderabbitai Bot previously approved these changes Jun 30, 2026
Comment thread framework/configstore/rdb.go
akshaydeo
akshaydeo previously approved these changes Jun 30, 2026
@impoiler
impoiler force-pushed the 06-30-fix_sync_batch_model_parameters_and_remove_mcp_library_sync_from_the_model_force_sync_endpoint branch from 11d1653 to c74e500 Compare July 1, 2026 06:41
@coderabbitai
coderabbitai Bot requested a review from akshaydeo July 1, 2026 06:42

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
framework/configstore/rdb.go (1)

2772-2772: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Closure parameter shadows outer tx variadic param.

upsert := func(tx *gorm.DB) error reuses the name tx, shadowing the outer tx ...*gorm.DB parameter. Not a bug (Go closures scope correctly), but it makes the function harder to scan since two different-typed tx identifiers coexist.

♻️ Suggested rename for clarity
-	upsert := func(tx *gorm.DB) error {
+	upsert := func(txn *gorm.DB) error {
 		// Unlike TableModelPricing, TableModelParameters has no nullable default
 		// columns, so GORM's multi-row INSERT does not emit DEFAULT values that
 		// SQLite rejects.
-		if err := tx.Clauses(onConflict).CreateInBatches(deduped, modelParametersUpsertBatchSize).Error; err != nil {
+		if err := txn.Clauses(onConflict).CreateInBatches(deduped, modelParametersUpsertBatchSize).Error; err != nil {
 			return s.parseGormError(err)
 		}
 		return nil
 	}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@framework/configstore/rdb.go` at line 2772, Rename the inner closure
parameter in the upsert logic to avoid shadowing the outer variadic tx
parameter; in the function that defines upsert, change the func(tx *gorm.DB)
signature to use a distinct name like txn or dbTx so the outer tx ...*gorm.DB
remains easy to distinguish and the code is clearer to scan.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@framework/configstore/rdb.go`:
- Line 2772: Rename the inner closure parameter in the upsert logic to avoid
shadowing the outer variadic tx parameter; in the function that defines upsert,
change the func(tx *gorm.DB) signature to use a distinct name like txn or dbTx
so the outer tx ...*gorm.DB remains easy to distinguish and the code is clearer
to scan.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: b4aa8d30-3338-463f-9fe3-abdd9abbb20a

📥 Commits

Reviewing files that changed from the base of the PR and between 11d1653 and c74e500.

📒 Files selected for processing (6)
  • framework/configstore/rdb.go
  • framework/configstore/rdb_test.go
  • framework/configstore/store.go
  • framework/modelcatalog/datasheet/params.go
  • framework/modelcatalog/main.go
  • transports/bifrost-http/lib/config_test.go
💤 Files with no reviewable changes (1)
  • framework/modelcatalog/main.go
🚧 Files skipped from review as they are similar to previous changes (4)
  • transports/bifrost-http/lib/config_test.go
  • framework/configstore/store.go
  • framework/modelcatalog/datasheet/params.go
  • framework/configstore/rdb_test.go

@impoiler
impoiler force-pushed the 06-25-docs_security-best-practices_docs branch from 541bf43 to 8774309 Compare July 1, 2026 06:57
@impoiler
impoiler force-pushed the 06-30-fix_sync_batch_model_parameters_and_remove_mcp_library_sync_from_the_model_force_sync_endpoint branch from c74e500 to 255e8e0 Compare July 1, 2026 06:57

akshaydeo commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Merge activity

  • Jul 1, 8:29 AM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Jul 1, 8:30 AM UTC: @akshaydeo merged this pull request with Graphite.

@akshaydeo
akshaydeo changed the base branch from 06-25-docs_security-best-practices_docs to graphite-base/4800 July 1, 2026 08:29
@akshaydeo
akshaydeo changed the base branch from graphite-base/4800 to dev July 1, 2026 08:29
@akshaydeo
akshaydeo dismissed stale reviews from coderabbitai[bot] and themself July 1, 2026 08:29

The base branch was changed.

@akshaydeo
akshaydeo merged commit 9b38a02 into dev Jul 1, 2026
11 of 12 checks passed
@akshaydeo
akshaydeo deleted the 06-30-fix_sync_batch_model_parameters_and_remove_mcp_library_sync_from_the_model_force_sync_endpoint branch July 1, 2026 08:30
akshaydeo pushed a commit that referenced this pull request Jul 1, 2026
…nc (#4800)

## Summary

Replaces the per-row transactional upsert loop used during model parameter sync with a single batched upsert, reducing DB round-trips from N (one per model) to a small number of batched `INSERT ... ON CONFLICT` statements.

## Changes

- Added `UpsertModelParametersBatch` to `RDBConfigStore` and the `ConfigStore` interface, which accepts a slice of `TableModelParameters` and upserts them in batches of 100 using GORM's `CreateInBatches` with an `ON CONFLICT` clause.
- Replaced the `ExecuteTransaction` + `UpsertModelParameters` loop in `SyncModelParamsFromURL` with a single call to `UpsertModelParametersBatch`, removing the need to open an explicit transaction and iterate row-by-row.
- Removed the MCP library sync goroutine from `ForceReloadPricing`, which was being launched concurrently alongside pricing/params sync.
- Added `UpsertModelParametersBatch` stub to `MockConfigStore` to satisfy the updated interface.
- Added `TestUpsertModelParametersBatch_SQLite` covering initial insert, upsert (update existing row), and no-duplicate behavior.

## Type of change

- [ ] Bug fix
- [x] Feature
- [x] Refactor
- [ ] Documentation
- [ ] Chore/CI

## Affected areas

- [x] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (React)
- [ ] Docs

## How to test

```sh
go test ./framework/configstore/... -run TestUpsertModelParametersBatch_SQLite -v
go test ./framework/modelcatalog/...
go test ./transports/bifrost-http/lib/...
```

Expected: all tests pass; `TestUpsertModelParametersBatch_SQLite` verifies that three records are inserted, an update to one record is reflected correctly, and the total row count remains 3 after the upsert.  
  
Verified using a cross-region remote database to simulate a real scenario, rather than having a local database on the same machine.  
  
Before  
![image.png](https://app.graphite.com/user-attachments/assets/b20faaf7-24ef-41c2-87ac-c5ac6597e616.png)

  
After  
![image.png](https://app.graphite.com/user-attachments/assets/f406860f-1560-462e-bacc-4aadbb3b808c.png)



##   
Breaking changes

- [ ] Yes
- [x] No

## Related issues

## Security considerations

No auth, secrets, or PII implications. The batch upsert uses parameterized queries via GORM, consistent with existing patterns.

## Checklist

- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
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