[Router][Bugfix] Defensive fallbacks in kvaware routing: 503 on no endpoints, session/QPS on unmapped instance - #1061
Conversation
…dpoints, session/QPS on unmapped instance Two crash paths, both pre-existing: - KvawareRouter.route_request accessed endpoints[0] unguarded - no available endpoints raised IndexError; LoadAwareRouter already answers 503 for this case, kvaware now mirrors it. - A KV lookup that matches an instance the ip mapping cannot place raised an unhandled KeyError and failed the request with a 500 (observed live: QueryInstMsg keys instances by IP alone, so several engines sharing one IP - e.g. one multi-GPU host - leave all but one instance unmapped; a stale registration does the same). The router can still serve the request without the cache hit, so a mapping miss now logs a warning and falls back to the existing session/QPS routing. Signed-off-by: tyler <tcr@enfuse.io>
![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 introduces defensive fallbacks in KvawareRouter.route_request to handle scenarios where no endpoints are available (raising a 503 instead of an IndexError) or when a KV lookup matches an unmapped instance (falling back to session/QPS routing instead of raising a KeyError). It also adds a new test suite to verify these fallbacks. The feedback recommends removing the accidentally committed .venv/pyvenv.cfg file and refactoring the duplicated fallback logic in routing_logic.py by moving the fallback_url method to the base class KvawareRouter.
| home = /opt/homebrew/opt/python@3.12/bin | ||
| include-system-site-packages = false | ||
| version = 3.12.12 | ||
| executable = /opt/homebrew/Cellar/python@3.12/3.12.12/Frameworks/Python.framework/Versions/3.12/bin/python3.12 | ||
| command = /opt/homebrew/opt/python@3.12/bin/python3.12 -m venv --clear /private/tmp/claude-502/-Users-enfuse-code-pvamu--claude-worktrees-prod-monitoring-alerting-research-9b4131/334ce31a-dbb3-4ec9-a7cb-f2d26c484c93/scratchpad/production-stack/.venv |
There was a problem hiding this comment.
The virtual environment configuration file .venv/pyvenv.cfg should not be committed to the repository. It contains absolute paths and settings specific to your local machine, which can cause issues for other contributors and CI/CD environments. Please remove this file from the repository and ensure that .venv/ is added to your .gitignore file.
There was a problem hiding this comment.
Good catch — removed in f2f255d (and .venv/ added to .gitignore so it cannot recur).
| session_id = self.extract_session_id(request, request_json) | ||
| self._update_hash_ring(endpoints) | ||
| if session_id is None: | ||
| return self._qps_routing(endpoints, request_stats) | ||
| return self.hash_ring.get_node(session_id) |
There was a problem hiding this comment.
The fallback logic implemented here is identical to the fallback logic at lines 434-444, and also matches the fallback_url method defined in LoadAwareRouter (lines 702-716). Since LoadAwareRouter inherits from KvawareRouter, we can eliminate this duplication by moving the fallback_url method up to KvawareRouter. This would allow both fallback paths in KvawareRouter.route_request to simply call self.fallback_url(endpoints, request_stats, request, request_json).
There was a problem hiding this comment.
Applied in f2f255d — fallback_url hoisted from LoadAwareRouter up to KvawareRouter (behavior-preserving given the subclass relationship) and both kvaware fallback sites now call it. Suite 222/222.
… .venv - fallback_url moves up from LoadAwareRouter to KvawareRouter (its subclass relationship makes this behavior-preserving) and both kvaware fallback sites - lookup miss and the new mapping miss - now call it, removing the triplicated session/QPS block. - .venv/pyvenv.cfg was accidentally committed; removed and .venv/ added to .gitignore. Signed-off-by: tyler <tcr@enfuse.io>
2 participants
Two pre-existing crash paths in
KvawareRouter.route_request, both turning conditions the router can survive into failed requests. Independent of #1045 and #1060 — small diff (one source file + a new test file), mergeable in any order.1. No endpoints available →
IndexErrorroute_requestaccessedendpoints[0]unguarded, so an empty endpoint list (service-discovery lag, all backends unhealthy) raisedIndexError.LoadAwareRouter.route_requestalready answers this withHTTPException(503, "No backend endpoints available")— kvaware now mirrors it.2. KV lookup matches an instance the ip mapping cannot place → unhandled
KeyError→ 500 (observed live)QueryInstMsgkeys instances by IP alone, so several engines sharing one IP (e.g. one multi-GPU host) leave all but one instance unmapped — and a stale registration produces the same shape. When the lookup then matched an unmapped instance,self.instance_id_to_ip[...]raisedKeyErrorand the whole request failed with a 500:The router can still serve the request without the cache-hit placement, so a mapping miss now logs a warning (including the current mapping, for diagnosability) and takes the existing session/QPS fallback — the same degradation path a lookup miss takes.
(Making instance identity include the worker port — so multi-engine hosts map correctly rather than falling back — is a larger change touching the lmcache message flow; left for a follow-up. This PR just stops the crash.)
Tests
Two added, self-contained: empty endpoints → 503, unmapped instance → request served via QPS fallback (no exception). Full router suite passes (222/222).
🤖 Generated with Claude Code