fix: add defensive chunking for SQL IN clauses and remove external_id length limit - #1665
fix: add defensive chunking for SQL IN clauses and remove external_id length limit#1665survivor998 wants to merge 1 commit into
Conversation
… length limit Prevents SQLite 'too many SQL variables' error by chunking all IDIn/ModelIDIn calls with a safe limit of 500 variables. Also removes MaxLen(512) from external_id fields and uses MySQL 'text' type instead, preventing truncation issues with long external IDs (e.g. Copilot Response API IDs >400 chars). Changes: - Add exported SQLiteMaxVariableLimit constant (500) in biz package - Chunk queryConfiguredModelFacades ModelIDIn calls - Chunk BulkArchiveModels/BulkDisableModels/BulkEnableModels/BulkDeleteModels - Chunk BulkCreateModels existence check (2 vars per predicate) - Replace openai.go hardcoded 500 with biz.SQLiteMaxVariableLimit - Change external_id from MaxLen(512) to SchemaType(text) in request schema - Change external_id from MaxLen(512) to SchemaType(text) in request_execution schema Note: Ent code regeneration required (go generate ./internal/server/gql)
Greptile SummaryThis PR adds defensive chunking to all SQL
Confidence Score: 3/5The read-only chunking and schema changes are safe, but the four Bulk mutation methods are no longer atomic when more than 500 IDs are passed, which could leave models in a partially modified or permanently partially deleted state. The chunked UPDATE/DELETE loops in the Bulk mutation methods commit each chunk independently. A failure on chunk 2+ returns an error to the caller while the already-committed chunks remain applied — making BulkDeleteModels in particular capable of permanently and partially deleting a set of models with no way to roll back. The rest of the PR (read-path chunking, schema type changes) is straightforward and correct. internal/server/biz/model.go — the four Bulk mutation methods need a wrapping transaction around their chunk loops. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[BulkArchive/Disable/Enable/DeleteModels\ncalled with N IDs] --> B{N > 500?}
B -- No --> C[Single UPDATE/DELETE\nchunk ✅ atomic]
B -- Yes --> D[Chunk 1: UPDATE/DELETE\nIDs 1-500 ✅ committed]
D --> E[Chunk 2: UPDATE/DELETE\nIDs 501-N]
E -- success --> F[All IDs updated ✅]
E -- error --> G[Error returned to caller\nbut Chunk 1 already committed ⚠️\nPartial mutation persisted]
H[BulkCreateModels / queryConfiguredModelFacades /\nopenai.go ListModels\nread-only chunked queries] --> I[Chunk 1 SELECT → accumulate]
I --> J[Chunk N SELECT → accumulate]
J --> K[Return combined results ✅ safe]
|
|
Is it a real case for more than 500 models and external id more than 512 chars? |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses SQLite's variable limit by implementing chunking for bulk database operations and queries using IN clauses. It also updates the external_id field schema in the Request and RequestExecution entities to use the text type for MySQL. Feedback suggests wrapping chunked bulk operations in transactions to maintain atomicity and deduplicating input IDs to prevent duplicate results in model queries.
| Save(ctx) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to bulk archive models: %w", err) | ||
| for _, chunk := range lo.Chunk(ids, SQLiteMaxVariableLimit) { |
There was a problem hiding this comment.
Chunking the update operation breaks its atomicity. If one chunk fails, previous chunks remain committed, which changes the behavior from the original single-query implementation. Consider wrapping the loop in a transaction to ensure all-or-nothing behavior, matching the original intent of a bulk operation.
| if len(allowedModelIDs) > 0 { | ||
| query = query.Where(model.ModelIDIn(allowedModelIDs...)) | ||
| } | ||
| // Chunk allowedModelIDs to avoid SQLite "too many SQL variables" error. |
There was a problem hiding this comment.
If allowedModelIDs contains duplicate IDs, the subsequent chunked queries will return duplicate model entities, which can lead to duplicate models in the API response. It is recommended to deduplicate the IDs before processing.
if len(allowedModelIDs) > 0 {
allowedModelIDs = lo.Uniq(allowedModelIDs)
// Chunk allowedModelIDs to avoid SQLite "too many SQL variables" error.2 participants
Summary
external_id#1471)Problem
The /models API 500 error was caused by unchunked SQL IN clauses exceeding SQLite variable limit. The original fix in openai.go only protected one call path, leaving several others vulnerable:
Additionally, external_id field had MaxLen(512) which caused MySQL Data too long errors with long external IDs (e.g. Copilot Response API IDs >400 chars, as reported in #1471).
Changes
Defensive Chunking (biz.SQLiteMaxVariableLimit = 500)
Schema Changes
Post-merge Required
Related Issues
external_id#1471 (external_id too long for MySQL)