-
Notifications
You must be signed in to change notification settings - Fork 10k
fix(observability): start every lfx serve request task from a clean context #14908
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
ogabrielluiz
wants to merge
3
commits into
release-1.13.0
Choose a base branch
from
fix-lfx-serve-contextvars
base: release-1.13.0
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+105
−3
Open
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
73 changes: 73 additions & 0 deletions
73
src/lfx/tests/unit/cli/test_serve_request_context_isolation.py
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,73 @@ | ||
| """Every ``lfx serve`` launch path must start each request task from a clean context. | ||
|
|
||
| A request that arrives while another is still in flight on the same connection is queued as a | ||
| pipelined request, and uvicorn starts it from inside the finishing request's task | ||
| (``httptools_impl.on_response_complete`` -> ``_start_asgi_task``). ``create_task`` copies the | ||
| context, so without ``reset_contextvars`` the new request begins with the previous request's | ||
| already-ended server span still current. OpenTelemetry's ASGI middleware reads that as nesting | ||
| and emits the request as an INTERNAL child of an unrelated finished request rather than as a | ||
| SERVER root, merging unrelated traces and hiding most HTTP traffic from RED metrics. | ||
|
|
||
| langflow set the flag on both of its launch paths; lfx serve has three and had none of them, | ||
| so the same defect stayed live on the runtime that actually serves traffic. These tests cover | ||
| all three, and the source-level one covers a fourth if anyone adds it. | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import ast | ||
| from pathlib import Path | ||
|
|
||
| import lfx.cli.commands | ||
| import pytest | ||
| import uvicorn | ||
|
|
||
| pytest.importorskip("gunicorn", reason="the gunicorn worker path is Unix-only") | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
|
|
||
|
|
||
| def test_the_gunicorn_worker_resets_the_context_and_keeps_its_inherited_options(): | ||
| """Asserted through a real ``uvicorn.Config``, so a renamed or rejected option fails here. | ||
|
|
||
| The inherited keys matter too: ``CONFIG_KWARGS`` is spread over the base class's, and | ||
| replacing it outright would silently drop uvicorn's loop and http selection. | ||
| """ | ||
| from lfx.cli.serve_gunicorn import LFXUvicornWorker | ||
| from uvicorn.workers import UvicornWorker | ||
|
|
||
| config = uvicorn.Config("lfx.cli.serve_app:create_serve_app", **LFXUvicornWorker.CONFIG_KWARGS) | ||
|
|
||
| assert config.reset_contextvars is True | ||
| for key, value in UvicornWorker.CONFIG_KWARGS.items(): | ||
| assert LFXUvicornWorker.CONFIG_KWARGS[key] == value, f"dropped inherited {key}" | ||
|
|
||
|
|
||
| def _uvicorn_run_calls() -> list[ast.Call]: | ||
| source = Path(lfx.cli.commands.__file__).read_text(encoding="utf-8") | ||
| return [ | ||
| node | ||
| for node in ast.walk(ast.parse(source)) | ||
| if isinstance(node, ast.Call) | ||
| and isinstance(node.func, ast.Attribute) | ||
| and node.func.attr == "run" | ||
| and isinstance(node.func.value, ast.Name) | ||
| and node.func.value.id == "uvicorn" | ||
| ] | ||
|
|
||
|
|
||
| def test_every_uvicorn_launch_in_the_serve_command_resets_the_context(): | ||
| """Read from the source rather than by starting a server or patching ``uvicorn.run``. | ||
|
|
||
| This is the guard that would have caught the original gap. Both call sites are literal | ||
| kwargs, and a new launch path added later fails here until it opts in too. | ||
| """ | ||
| calls = _uvicorn_run_calls() | ||
| assert len(calls) >= 2, "expected the single-worker and Windows multi-worker launches" | ||
|
|
||
| for call in calls: | ||
| passed = {kw.arg: kw.value for kw in call.keywords} | ||
| assert "reset_contextvars" in passed, f"uvicorn.run at line {call.lineno} does not reset the context" | ||
| value = passed["reset_contextvars"] | ||
| assert isinstance(value, ast.Constant), ( | ||
| f"uvicorn.run at line {call.lineno} passes a non-literal reset_contextvars={ast.unparse(value)}" | ||
| ) | ||
| assert value.value is True, f"uvicorn.run at line {call.lineno} passes reset_contextvars={value.value!r}" | ||
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.