Fix panic on empty user_ids in RetrieveTopKCandidatesRequest - #73
Open
AnonRish wants to merge 1 commit into
Open
Fix panic on empty user_ids in RetrieveTopKCandidatesRequest#73AnonRish wants to merge 1 commit into
AnonRish wants to merge 1 commit into
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
user_idsinRetrieveTopKCandidatesRequestwith aStatus::invalid_argumentinstead of panickingNUM_REQUESTS_REJECTEDmetric on this path,matching the other rejection paths already in this function
Problem
RecsysRetrievalPredictorImpl::retrieve_top_k_candidates_innerdoes:user_idsis a protobufrepeatedfield, so nothing prevents a callerfrom sending it empty.
.pop()on an empty Vec returns None, and.unwrap()panics -- this is reachable directly from an external gRPCrequest, past the admission-control and deadline-shedding checks earlier
in the same function. Any caller sending a request with no user IDs can
take down this handler.
The line right below handles the identically-shaped
sequencesfieldcorrectly (kept as an Option), and
columnar_sequencestwo lines furtherdown does the same with a
.filter(...)chain -- this looks like anisolated gap rather than an intentional invariant.
Fix
user_ids.pop()is now matched as an Option. An empty list returnsStatus::invalid_argumentand incrementsNUM_REQUESTS_REJECTEDwith thesame label pattern already used three times earlier in this function,
instead of panicking.
Verification
Status-returning branches in this function forlabel/style consistency.
rustc >=1.85; see the companion MSRV PR) -- please run
cargo test -p xai-recsys-enginebefore merging.No existing harness for constructing
RetrieveTopKCandidatesRequesttestfixtures was found in this crate. Didn't add one as part of this fix
rather than invent test infrastructure the change doesn't otherwise need
-- happy to add a regression test if there's an existing pattern for this
maintainers would prefer.