[Bugfix] Return HTTP 500 when is_exist reports an error in handle_exist#2602
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the handle_exist method in mooncake_store_service.py to properly handle store health check failures (indicated by a negative return value) by returning an HTTP 500 error instead of an HTTP 200 with a false positive. It also adds a corresponding unit test. The review feedback suggests improving the robustness of the new unit test by asserting the specific error message rather than just checking for the presence of the 'error' key.
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.
| body = json.loads(resp.text) | ||
| self.assertIn("error", body) |
There was a problem hiding this comment.
The current assertion self.assertIn("error", body) only checks for the presence of the "error" key in the response. If an unexpected exception occurs in the try block of handle_exist, the except Exception block will catch it and return a 500 response with an "error" key containing the exception string. This would cause the test to pass even if the specific error path wasn't hit.
To make the test more robust and prevent false positives, assert the specific error message "Exist check failed" in body["error"], matching the pattern used in other tests like test_handle_get_exist_check_failure.
| body = json.loads(resp.text) | |
| self.assertIn("error", body) | |
| body = json.loads(resp.text) | |
| self.assertIn("Exist check failed", body["error"]) |
There was a problem hiding this comment.
I agree with this suggestion. The test should assert the specific "Exist check failed" message so it verifies the new exists < 0 path rather than any generic 500 error response.
There was a problem hiding this comment.
Good call, done in 0365cd5. The assertion now checks body["error"] == "Exist check failed", so it pins the exists < 0 branch specifically and won't pass on the generic except path (which returns {"error": str(e)}).
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
handle_exist used bool(is_exist(key)) to build the response body.
is_exist() returns 0 (not found), a positive integer (found), or a
negative integer (store error). bool(-1) is True, so a store error was
silently surfaced as HTTP 200 {"exists": true} instead of HTTP 500.
Fix: check exists < 0 and return 500 "Exist check failed", mirroring
the pattern introduced for handle_get in kvcache-ai#2587. Also replace bool(exists)
with exists > 0 for clarity.
Adds test_handle_exist_store_error to cover the previously untested
negative-return path.
Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>
1e7f25c to
772f646
Compare
Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>
Lin-z-w
left a comment
There was a problem hiding this comment.
Thanks for the fix. LGTM.
…st (kvcache-ai#2602) Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>
Problem
handle_existusedbool(is_exist(key))to populate the response body.is_exist()returns three distinct values:0bool(-1)evaluates toTruein Python, so a store error was silently reported as HTTP 200{"exists": true}instead of HTTP 500. Callers have no way to distinguish "key found" from "store is unhealthy".Fix
Mirror the pattern introduced for
handle_getin #2587: checkexists < 0first and return HTTP 500{"error": "Exist check failed"}. Also replacebool(exists)withexists > 0to make the positive-vs-zero distinction explicit.Test plan
New test
test_handle_exist_store_errorsetsis_exist = lambda key: -1and asserts the response is HTTP 500 with an"error"key.To verify:
cd mooncake-wheel python -m pytest tests/test_mooncake_store_service_api.py -k exist -vAll exist-related tests pass. Four unrelated tests (
test_handle_put_missing_value,test_reconfigure_*,test_unmount_shm_string_segment_id_coercion) fail on upstreammainas existing failures before this change and are unaffected.Relation to #2587
#2587 fixed the same class of bug in
handle_get(which also callsis_existand previously usedif not valueas a truthiness check). This PR applies the same correction to thehandle_existendpoint.