router: refresh ModelRoute indexes on update - #1333
Conversation
![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 refactors the model route deletion logic by extracting index removal into a helper method, 'removeModelRouteFromIndexesLocked', and uses it in 'AddOrUpdateModelRoute' to clean up old indexes during updates. It also adds unit tests to verify that updating a model route correctly updates LoRA and gateway routes. The review feedback correctly identifies a critical issue where ignoring the return values of 'removeModelRouteFromIndexesLocked' in 'AddOrUpdateModelRoute' leads to a goroutine leak, as unreferenced request queues are never closed. A code suggestion is provided to properly clean up and close these queues.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
@hzxuzhonghu @LiZhenCheng9527 ptal, found this while at ModelRoute update paths..... the store was adding new indexes without clearing the old ones first. Let me know if further changes req |
1d1adc8 to
4984aff
Compare
4984aff to
fb903f5
Compare
|
ptal @hzxuzhonghu, updated the queue cleanup path to reject new pushes after shutdown and wake any drained waiters, including the semaphore-mode stop path |
|
/assign @YaoZengzeng |
fb903f5 to
a3e6d4d
Compare
|
@YaoZengzeng ptal when get chance |
|
@YaoZengzeng ptal when get chance, and lmk if changes needed |
a3e6d4d to
4636fe8
Compare
Signed-off-by: Avinash Kumar Deepak <avinash8655279@gmail.com>
4636fe8 to
30b9ee4
Compare
|
@LiZhenCheng9527 renamed it to queueCleanupCandidates as suggested. PTAL. |
| var deletedRoute *aiv1alpha1.ModelRoute | ||
| // Collect all model/lora names that may have associated queues (for cleanup after unlock) | ||
| var namesToCleanQueue []string | ||
| var queueCleanupCandidates []string |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hzxuzhonghu The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind bug
What this PR does / why we need it:
Refreshes ModelRoute indexes before storing an updated route.
Without this, updating a ModelRoute can leave stale entries in the old model, LoRA, or gateway indexes. A removed LoRA adapter or old parentRef may still match after the route has been updated.
Which issue(s) this PR fixes:
Fixes #N/A
Special notes for your reviewer:
Adds regression coverage for updating LoRA adapters and gateway parentRefs on the same ModelRoute.
Does this PR introduce a user-facing change?: