RSPEED-3444: read root_path from fastapi app instead of ASGI scope - #2380
RSPEED-3444: read root_path from fastapi app instead of ASGI scope#2380thepetk wants to merge 1 commit into
fastapi app instead of ASGI scope#2380Conversation
…stead of app Signed-off-by: thepetk <thepetk@gmail.com>
Walkthrough
ChangesRoot path metrics matching
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 7✅ Passed checks (7 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
fastapi app instead of ASGI scope
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/app/test_main_middleware.py`:
- Around line 253-288: The new test duplicates
test_rest_api_metrics_strips_root_path; remove
test_rest_api_metrics_uses_app_root_path_not_scope or change it to use a
conflicting non-empty scope root_path while keeping fastapi_app.root_path set,
then assert metrics use the application root.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c7065dbc-1fec-44da-9aed-6ca74752b197
📒 Files selected for processing (2)
src/app/main.pytests/unit/app/test_main_middleware.py
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: E2E: server mode / ci / group 2
- GitHub Check: E2E: library mode / ci / group 1
- GitHub Check: E2E: server mode / ci / group 3
- GitHub Check: E2E: library mode / ci / group 2
- GitHub Check: E2E Tests for Lightspeed Evaluation job
🧰 Additional context used
📓 Path-based instructions (3)
**/*
📄 CodeRabbit inference engine (Custom checks)
**/*: Flag meaningful O(n^2)+ algorithms on non-trivial inputs, including handlers and Kubernetes list operations.
Flag N+1 patterns that list items and then query once per item, including Kubernetes API and database access.
Flag expensive work inside loops, including API calls, JSON parsing, and regex compilation.
Flag unbounded growth in caches, watchers, or buffers when eviction or limits are missing.
Flag missing pagination or limits on list operations and API endpoints.
Flag secrets or tokens logged in plaintext or hardcoded in source.
Flag API endpoints missing authentication or authorization.
Flag injection vulnerabilities, including SQL injection, command injection, and path traversal.
Flag sensitive data leaked in API responses, WebSocket messages, or logs.
Flag Kubernetes Secrets and Red Hat secrets missing OwnerReferences.
Files:
src/app/main.pytests/unit/app/test_main_middleware.py
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.py: Use absolute imports for internal modules and follow the prescribed FastAPI and Llama Stack import conventions.
All modules must begin with descriptive docstrings; uselogger = get_logger(__name__)fromlog.pyfor module logging; package__init__.pyfiles must contain brief package descriptions.
Define shared constants in the centralconstants.pymodule, add descriptive comments, and annotate constants withFinal[type].
Use complete type annotations for function parameters, return types, class attributes, and type aliases; prefer specific types overAny, use modern union syntax, and usetyping_extensions.Selffor model validators.
All functions and classes require descriptive Google-style docstrings, including appropriateParameters,Returns,Raises, andAttributessections.
Use descriptive snake_case, action-oriented function names such asget_,validate_, andcheck_; use PascalCase class names with standard suffixes such asConfiguration,Error/Exception,Resolver, andInterface.
Avoid modifying input parameters in place; return a newly constructed data structure instead.
Useasync deffor I/O operations and external API calls; API endpoints should raise FastAPIHTTPExceptionwith appropriate status codes and handle Llama StackAPIConnectionError.
Usefrom log import get_loggerand standard logger levels:debugfor diagnostics,infofor general execution,warningfor unexpected conditions or potential problems, anderrorfor serious failures.
Configuration models must extendConfigurationBase, setextra="forbid"to reject unknown fields, use Pydantic validators for custom validation, and use types such asOptional[FilePath],PositiveInt, andSecretStrwhere appropriate.
Abstract interfaces must useABCand@abstractmethoddecorators.
Never commit secrets or keys; use environment variables for sensitive data.
Files:
src/app/main.py
tests/unit/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use pytest for unit tests, shared fixtures in
conftest.py,pytest-mockfor mocks,pytest.mark.asynciofor async tests, and maintain at least 60% unit-test coverage.
Files:
tests/unit/app/test_main_middleware.py
🧠 Learnings (3)
📚 Learning: 2026-06-24T13:45:37.249Z
Learnt from: Jdubrick
Repo: lightspeed-core/lightspeed-stack PR: 1971
File: src/utils/markdown_repair.py:31-36
Timestamp: 2026-06-24T13:45:37.249Z
Learning: In the lightspeed-stack repository, docstrings must use the section header name "Parameters:" (not "Args:") for function arguments, even if the project references Google Python docstring conventions. Ensure docstrings follow the project’s established "Parameters:" header format for any documented function parameters.
Applied to files:
src/app/main.pytests/unit/app/test_main_middleware.py
📚 Learning: 2026-07-06T15:26:18.398Z
Learnt from: Jdubrick
Repo: lightspeed-core/lightspeed-stack PR: 2071
File: src/models/config.py:2416-2422
Timestamp: 2026-07-06T15:26:18.398Z
Learning: In this repo’s Python code under src/**, don’t treat differences in string concatenation style as a style inconsistency when Black has effectively forced (or made clearer) use of explicit `+` string concatenation in multi-line logger/string expressions. If adjacent-literal implicit concatenation is avoided/changed specifically to accommodate Black’s formatting in these call sites, accept the `+` usage and don’t recommend converting it solely for consistency with nearby blocks that use implicit concatenation.
Applied to files:
src/app/main.py
📚 Learning: 2026-07-17T19:25:05.325Z
Learnt from: Jdubrick
Repo: lightspeed-core/lightspeed-stack PR: 2166
File: src/utils/saved_prompts.py:129-157
Timestamp: 2026-07-17T19:25:05.325Z
Learning: For any endpoint that handles saved prompts and calls `src/utils/saved_prompts.py::create_saved_prompt`, treat the endpoint as the validation boundary. Before calling `create_saved_prompt`, validate the incoming saved-prompt name and content, specifically using `validate_saved_prompt_name` and then persist (store) the normalized value it returns. Do not call `create_saved_prompt` with unvalidated/raw name/content.
Applied to files:
src/app/main.py
🔇 Additional comments (3)
src/app/main.py (1)
215-215: LGTM!tests/unit/app/test_main_middleware.py (2)
12-18: LGTM!
198-198: LGTM!Also applies to: 211-213
| @pytest.mark.asyncio | ||
| async def test_rest_api_metrics_uses_app_root_path_not_scope( | ||
| mocker: MockerFixture, | ||
| ) -> None: | ||
| """Middleware must read root_path from app.root_path, not scope["root_path"]. | ||
|
|
||
| This regression test guards against reverting the fix for RSPEED-2941: the scope | ||
| carries no root_path (as uvicorn actually sends), while app.root_path is set to the | ||
| correct prefix. If someone switches back to scope.get("root_path", ""), this test | ||
| fails because the path would not be stripped and the metric would not be recorded. | ||
| """ | ||
| mocker.patch("app.main.app_routes_paths", ["/v1/infer"]) | ||
| mocker.patch.object(fastapi_app, "root_path", "/api/lightspeed") | ||
| mock_measure_duration = mocker.patch( | ||
| "app.main.recording.measure_response_duration", return_value=nullcontext() | ||
| ) | ||
| mock_record_call = mocker.patch("app.main.recording.record_rest_api_call") | ||
|
|
||
| async def ok_app(_scope: Scope, _receive: Receive, send: Send) -> None: | ||
| await send({"type": "http.response.start", "status": 200, "headers": []}) | ||
| await send({"type": "http.response.body", "body": b"ok"}) | ||
|
|
||
| middleware = RestApiMetricsMiddleware(ok_app) | ||
| collector = _ResponseCollector() | ||
|
|
||
| # Scope intentionally has no root_path — matching what uvicorn actually provides. | ||
| # The middleware must strip the prefix using app.root_path, not the scope. | ||
| await middleware( | ||
| _make_scope("/api/lightspeed/v1/infer"), | ||
| _noop_receive, | ||
| collector, | ||
| ) | ||
|
|
||
| assert collector.status_code == 200 | ||
| mock_measure_duration.assert_called_once_with("/v1/infer") | ||
| mock_record_call.assert_called_once_with("/v1/infer", 200) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Remove this duplicate test or make it cover a different input.
test_rest_api_metrics_strips_root_path at Lines 193-221 already sets fastapi_app.root_path to /api/lightspeed, omits scope["root_path"], and asserts /v1/infer metrics. The new test repeats the same setup and assertions. Keep one test, or pass a conflicting non-empty scope root path to verify application-root precedence.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/app/test_main_middleware.py` around lines 253 - 288, The new test
duplicates test_rest_api_metrics_strips_root_path; remove
test_rest_api_metrics_uses_app_root_path_not_scope or change it to use a
conflicting non-empty scope root_path while keeping fastapi_app.root_path set,
then assert metrics use the application root.
|
Will mark it again as "ready for review" once I complete a final round of tests |
Description
Fix REST API middleware-level metrics never recording when the service is deployed with a non-empty
root_path.RestApiMetricsMiddlewarehas prefix-stripping logic to handle deployments where a reverse proxy forwards requests with the full prefixed path. However, it readsroot_pathfrom the ASGI scope (scope.get("root_path", "")), which is always"". The actual configured value lives on the FastAPI application object (app.root_path), not in the scope. As a result, the stripping logic never activates, every request path fails the route check, and no middleware-level metrics are ever recorded.The fix replaces the scope read with
app.root_path, which always holds the correctly configured value. Deployments with an emptyroot_pathare unaffected.Type of change
pyproject.toml+uv.lock]requirements.*.txtfor Konflux]Tools used to create PR
Related Tickets & Documents
Checklist before requesting a review
Testing
To verify the fix end-to-end:
root_path./metrics— middleware-level metrics should now have samples where before they had none.Unit test coverage:
test_rest_api_metrics_strips_root_path— updated to patchapp.root_pathdirectly (scope carries noroot_path, matching actual runtime behaviour).test_rest_api_metrics_no_root_path_unchanged— unchanged; confirms emptyroot_pathdeployments are unaffected.test_rest_api_metrics_uses_app_root_path_not_scope— new regression test. Setsapp.root_pathto the prefix while the scope carries noroot_path. Fails if the middleware is switched back to reading from the scope, preventing silent regression.Summary by CodeRabbit
/v1/infer.