Avoid Lua full-set reads for unfiltered function lists#13
Draft
SegmondFault wants to merge 1 commit into
Draft
Conversation
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
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.
Summary
Avoid routing unfiltered function list requests through the Lua search path.
When the Functions table loads without filters, the existing Lua script can fall back to reading
collection:all_functionsbefore pagination. In a large or partially ingested collection, this can make/api/function/search?collection=main&limit=100&min_cohesion=0.95time out and leave Kvrocks unresponsive.This adds a bounded route-level path for unfiltered function list requests:
SCARDfor the total countSSCANfor the requested pageAddresses #12
Provenance
This PR came from reproducing a Kvrocks hang while investigating the function-search dashboard path on a large/partially indexed
maincollection. The original symptom was an unfiltered function-list request timing out, followed by Kvrocks becoming unresponsive toPING.The initial PR was opened against
main, but after maintainer feedback that current stable work is ondev, I fetchedMISP/bsimvisdevand rechecked the change there.On current
origin/dev, the broader build-sim Kvrocks EVAL-lock issue appears to have been addressed separately by:PR #13 is therefore scoped more narrowly: it still covers the unfiltered function-search/listing path. On
origin/dev, that path still callssearch_function.luawith an emptygroupslist, and the Lua script still falls back to:where
producer.keyiscollection .. ":all_functions".Validation Against
devI created a temporary detached worktree from
origin/dev, cherry-picked this PR commit onto it, and verified that it applies cleanly.Validation performed after retargeting the PR to
dev:git diff --check origin/dev..HEADuv run python -m compileall bsimvis/app/routes/search_function.pyI also ran a small fake-Redis route harness to verify the failure trigger without stressing a real Kvrocks instance:
origin/dev:GET /api/function/search?collection=main&limit=2reaches Lua withgroups: [], reproducing the riskySMEMBERS main:all_functionspathdev: the same request does not call Lua; it usesSCARD main:all_functions, boundedSSCAN main:all_functions, then the normal enrichment pipelineImplementation Notes
I initially tried keeping this entirely inside
bsimvis/app/lua/search_function.luaby replacing the unfilteredSMEMBERSpath with bounded Lua-side pagination. That still reproduced the failure in the isolated repro runtime: the request timed out after about 20s and Kvrocks stopped responding toPING.Because of that, this PR avoids invoking the Lua search script at all for the no-filter function list case. Filtered searches still use the existing Lua producer/filter path.
Scope
This is intentionally limited to
bsimvis/app/routes/search_function.pyand the default unfiltered function list path. It does not change the UI, storage schema, ingestion/indexing model, similarity build logic, or filtered search behavior.Tradeoffs
This PR intentionally avoids adding a new ordered function-list index, so there are two remaining edge cases in the fallback path:
Deep pagination without a sort index may still be slower.
If the request asks for a high
offsetand there is no usable sorted function index, the fallback has to scan past earlier set members before collecting the requested page. This is still bounded to page collection rather than loading the full function set into Lua, but it is not as efficient as a real ordered index.Default
sort_by=idfallback is not globally sorted.collection:all_functionsis a set, soSSCANdoes not provide stable global ordering. The fallback returns a bounded page of function IDs, but it does not reproduce the old behavior of loading and sorting the entire set by ID. Preserving stable ID ordering efficiently would require a maintained ordered function-list index.A future follow-up could add/backfill such an index, but that would touch ingestion/indexing behavior, so I left it out to keep this PR focused on the reproduced Kvrocks hang.
Testing
luac -p bsimvis/app/lua/search_function.luaPYTHONPYCACHEPREFIX=/tmp/bsimvis-pycache uv run python -m py_compile bsimvis/app/routes/search_function.pygit diff --check origin/main...HEADPINGhungPINGhungHTTP 200 total 0.014008PINGreturnedPONGdev:origin/devworktree applied cleanlygit diff --check origin/dev..HEADuv run python -m compileall bsimvis/app/routes/search_function.pydevreaches the unfiltered Lua path and patcheddevavoids it