Fix #2380: [Bug] Overview/health model slots misreport "Not called yet" once the slot's new - #2381
Open
Memtensor-AI wants to merge 2 commits into
Open
Memtensor-AI wants to merge 2 commits into
Memtensor-AI wants to merge 2 commits into
Conversation
…MemTensor#2380) findLatestPersistedModelStatus() previously fetched the newest 500 api_logs rows and post-filtered in JS for role/provider/model. On busy installs the target slot (e.g. skillEvolver) is pushed >500 positions below the head by high-frequency llm/embedding writes, causing the lookup to silently return null and the Overview card to show 'Not called yet' even when historical rows exist. Fix: add findLatestModelStatus(role, provider, model) to the api_logs repo. It uses json_extract() to filter at SQL level with no window, returning the single newest matching row. Update findLatestPersistedModelStatus() to delegate to this method instead. Regression test seeds 600 llm rows after one skillEvolver row (putting the target 600 positions out of the old window) and asserts health() returns the correct lastOkAt timestamp.
Memtensor-AI
added
ai:generated
Collaborator
Author
🤖 Open Code ReviewTarget: PR #2381 ✅ OpenCodeReview: Review complete: 0 finding(s) across 5 selected item(s). Generated by cloud-assistant via Open Code Review. |
Collaborator
Author
🔧 Open Code Review requested Agent fixOpen Code Review found 1 issue(s). I have resumed the development Agent to fix them.
The Agent will push a new commit to this PR branch. OCR will recheck after the commit is pushed. |
Follow-up to MemTensor#2380 addressing OCR review feedback: the json_extract() predicates in findLatestModelStatus() had no supporting index, so the lookup was a full-table scan on every /health poll (api_logs holds up to ~10k rows on busy installs). Add migration 013 with a partial functional index that covers tool_name='system_model_status' and matches the query's filter + ORDER BY shape. SQLite's planner picks it up and turns the scan into an index seek. Migration 013 uses tableExists('api_logs') so the partial-schema test path in migrator.test.ts continues to pass. Regression-test the property that the /health lookup is index-driven (EXPLAIN QUERY PLAN sees `USING INDEX idx_api_logs_model_status`) so future changes can't quietly regress it.
Memtensor-AI
added
status:ready
3 tasks
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.
Description
Fixed #2380: findLatestPersistedModelStatus() in core/pipeline/memory-core.ts previously scanned the newest 500 api_logs rows in JS for a matching role/provider/model triple. On busy installs the 500-row window fills with llm/embedding entries, silently dropping lower-frequency slots (skillEvolver) and causing Overview cards to misreport "Not called yet". Fix: added findLatestModelStatus(role, provider, model) to the api_logs repo using json_extract() SQL filtering with no window, and updated the caller to use it. Added regression test seeding 600 llm rows after one skillEvolver row to pin the fix. All 76 pipeline unit tests pass including the new #2380 regression test.
Related Issue (Required): Fixes #2380
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Not run; documentation-only change.
Checklist
@whipser030, @hijzy please review this PR.
Reviewer Checklist