Skip to content

Commit 1f0fe23

Browse files
jiaenrenclaude
andauthored
Release the Redis log reader when a client disconnects (#1317)
Tailing a workflow's live logs opens a Redis client that polls XREAD once a second. If the client goes away while the workflow is quiet, nothing tells the server: Starlette installs a disconnect listener only for ASGI servers older than spec 2.4, and on 2.4 -- which uvicorn advertises -- it infers a disconnect from a failed body send. A body with nothing to send never fails, so the request polls forever and its Redis connection is never released. Two changes, both required. Measured in KIND over 20 disconnects: 4 connections leaked before, 2 with only the first, 0 with both. - ClosingStreamingResponse restores the disconnect listener unconditionally and closes the body afterwards. Applied to the Redis log branch only; the other streaming endpoints serve finite object-store reads whose next send fails promptly, so they end on their own. - redis_log_streamer closes its client under anyio.CancelScope(shield=True). The disconnect tears the reader down by cancelling the task driving it, and cancellation lands on the sleep inside the generator, so an unshielded close in the finally is cancelled before the socket is released. Covered by a test that fails without the shield. contextlib.aclosing at the two chain links makes closure reach the streamer; closing an async generator does not close one it is iterating. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent fb2f040 commit 1f0fe23

10 files changed

Lines changed: 409 additions & 21 deletions

File tree

src/service/core/BUILD

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,16 @@ load("@osmo_python_deps//:requirements.bzl", "requirement")
2222
load("@rules_oci//oci:defs.bzl", "oci_image", "oci_push", "oci_load")
2323
load("@osmo_constants//:constants.bzl", "BASE_IMAGE_URL", "IMAGE_TAG")
2424

25+
osmo_py_library(
26+
name = "responses",
27+
srcs = ["responses.py"],
28+
deps = [
29+
requirement("fastapi"),
30+
requirement("starlette"),
31+
],
32+
visibility = ["//visibility:public"],
33+
)
34+
2535
osmo_py_library(
2636
name = "service_lib",
2737
srcs = [

src/service/core/responses.py

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
"""
2+
SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
16+
SPDX-License-Identifier: Apache-2.0
17+
"""
18+
19+
import asyncio
20+
from typing import AsyncGenerator, cast
21+
22+
import fastapi.responses
23+
from starlette.types import Receive, Scope, Send
24+
25+
26+
class ClosingStreamingResponse(fastapi.responses.StreamingResponse):
27+
"""Streaming response that ends and releases its body when the client leaves.
28+
29+
Starlette only installs a disconnect listener for ASGI servers older than
30+
spec 2.4; on 2.4 and later it infers a disconnect from a failed body send.
31+
Uvicorn advertises 2.4, so a body that never sends -- a log tail on a quiet
32+
workflow -- is never told the client is gone, and the request runs forever.
33+
34+
This restores the listener unconditionally and closes the body afterwards,
35+
so whatever the body holds open is released with the response. Use it for a
36+
body that can stay idle; a body that always has a next chunk does not need
37+
it, because its next send will fail and end the request anyway.
38+
"""
39+
40+
async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None:
41+
if scope['type'] != 'http':
42+
await super().__call__(scope, receive, send)
43+
return
44+
45+
streaming = asyncio.ensure_future(self.stream_response(send))
46+
watching = asyncio.ensure_future(self.listen_for_disconnect(receive))
47+
try:
48+
await asyncio.wait((streaming, watching), return_when=asyncio.FIRST_COMPLETED)
49+
finally:
50+
streaming.cancel()
51+
watching.cancel()
52+
# Both have to finish unwinding before the body can be closed, or
53+
# aclose() lands on a generator that is still running.
54+
await asyncio.wait((streaming, watching))
55+
await cast(AsyncGenerator[bytes, None], self.body_iterator).aclose()
56+
57+
if not streaming.cancelled():
58+
# Surface a body failure; a cancelled stream means the client left.
59+
streaming.result()
60+
61+
if self.background is not None:
62+
await self.background()

src/service/core/tests/BUILD

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,17 @@ osmo_py_test(
5959
visibility = ["//visibility:public"],
6060
)
6161

62+
osmo_py_test(
63+
name = "test_responses",
64+
srcs = ["test_responses.py"],
65+
deps = [
66+
"//src/service/core:responses",
67+
osmo_requirement("starlette"),
68+
],
69+
size = "small",
70+
visibility = ["//visibility:public"],
71+
)
72+
6273
osmo_py_test(
6374
name = "test_asyncio_startup",
6475
srcs = ["test_asyncio_startup.py"],
Lines changed: 170 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,170 @@
1+
"""
2+
SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
16+
SPDX-License-Identifier: Apache-2.0
17+
"""
18+
import asyncio
19+
import unittest
20+
21+
from starlette.applications import Starlette
22+
from starlette.background import BackgroundTask
23+
from starlette.middleware.base import BaseHTTPMiddleware
24+
from starlette.routing import Route
25+
26+
from src.service.core import responses
27+
28+
29+
def _scope(spec_version: str = '2.4') -> dict:
30+
"""Minimal ASGI HTTP scope for calling a response directly."""
31+
return {'type': 'http', 'asgi': {'version': '3.0', 'spec_version': spec_version}}
32+
33+
34+
async def _idle_receive():
35+
"""ASGI receive for a client that stays connected and sends nothing further."""
36+
await asyncio.Future()
37+
38+
39+
async def _discard_send(message) -> None: # pylint: disable=unused-argument
40+
"""ASGI send that drops every message."""
41+
42+
43+
def _disconnect_once(after: asyncio.Event):
44+
"""ASGI receive that reports a disconnect once `after` is set."""
45+
46+
async def receive():
47+
await after.wait()
48+
return {'type': 'http.disconnect'}
49+
50+
return receive
51+
52+
53+
def _quiet_body(started: asyncio.Event, closed: asyncio.Event):
54+
"""Body that blocks before its first chunk, recording start and closure."""
55+
56+
async def body():
57+
try:
58+
started.set()
59+
await asyncio.Future()
60+
yield 'unreachable\n'
61+
finally:
62+
closed.set()
63+
64+
return body()
65+
66+
67+
class TestClosingStreamingResponse(unittest.IsolatedAsyncioTestCase):
68+
"""Covers release of an idle body when the client disconnects."""
69+
70+
async def test_quiet_body_is_closed_when_client_disconnects(self):
71+
for spec_version in ('2.3', '2.4'):
72+
with self.subTest(spec_version=spec_version):
73+
started, closed = asyncio.Event(), asyncio.Event()
74+
response = responses.ClosingStreamingResponse(_quiet_body(started, closed))
75+
await asyncio.wait_for(
76+
response(_scope(spec_version), _disconnect_once(started), _discard_send),
77+
timeout=1,
78+
)
79+
self.assertTrue(closed.is_set(), 'A quiet body outlived its client.')
80+
81+
async def test_body_is_closed_after_it_is_fully_sent(self):
82+
closed = asyncio.Event()
83+
sent = []
84+
85+
async def body():
86+
try:
87+
yield 'one\n'
88+
yield 'two\n'
89+
finally:
90+
closed.set()
91+
92+
async def collecting_send(message):
93+
if message['type'] == 'http.response.body':
94+
sent.append(message['body'])
95+
96+
response = responses.ClosingStreamingResponse(body())
97+
await asyncio.wait_for(
98+
response(_scope(), _idle_receive, collecting_send), timeout=1)
99+
100+
self.assertEqual(b''.join(sent), b'one\ntwo\n')
101+
self.assertTrue(closed.is_set(), 'The body was not closed after a full send.')
102+
103+
async def test_body_failure_propagates(self):
104+
105+
async def failing_body():
106+
# The unreachable yield is what makes this an async generator; the
107+
# raise fires on the first __anext__.
108+
if False: # pylint: disable=using-constant-test
109+
yield 'unreachable\n'
110+
raise OSError('backend read failed')
111+
112+
response = responses.ClosingStreamingResponse(failing_body())
113+
with self.assertRaisesRegex(OSError, 'backend read failed'):
114+
await asyncio.wait_for(
115+
response(_scope(), _idle_receive, _discard_send), timeout=1)
116+
117+
async def test_background_task_runs_after_the_body_is_closed(self):
118+
order = []
119+
120+
async def body():
121+
try:
122+
yield 'one\n'
123+
finally:
124+
order.append('body closed')
125+
126+
async def background():
127+
order.append('background')
128+
129+
response = responses.ClosingStreamingResponse(
130+
body(), background=BackgroundTask(background))
131+
await asyncio.wait_for(
132+
response(_scope(), _idle_receive, _discard_send), timeout=1)
133+
134+
self.assertEqual(order, ['body closed', 'background'])
135+
136+
async def test_quiet_body_is_closed_behind_http_middleware(self):
137+
"""The core service wraps requests in BaseHTTPMiddleware; it must not
138+
prevent the disconnect from reaching the response."""
139+
started, closed = asyncio.Event(), asyncio.Event()
140+
request_sent = False
141+
142+
async def endpoint(request): # pylint: disable=unused-argument
143+
return responses.ClosingStreamingResponse(_quiet_body(started, closed))
144+
145+
async def pass_through(request, call_next):
146+
return await call_next(request)
147+
148+
async def receive():
149+
nonlocal request_sent
150+
if not request_sent:
151+
request_sent = True
152+
return {'type': 'http.request', 'body': b'', 'more_body': False}
153+
await started.wait()
154+
return {'type': 'http.disconnect'}
155+
156+
app = Starlette(routes=[Route('/logs', endpoint)])
157+
app.add_middleware(BaseHTTPMiddleware, dispatch=pass_through)
158+
await asyncio.wait_for(
159+
app({**_scope(), 'http_version': '1.1', 'method': 'GET', 'scheme': 'http',
160+
'path': '/logs', 'raw_path': b'/logs', 'query_string': b'', 'headers': [],
161+
'client': ('127.0.0.1', 12345), 'server': ('testserver', 80)},
162+
receive, _discard_send),
163+
timeout=1,
164+
)
165+
166+
self.assertTrue(closed.is_set(), 'A quiet body was not closed through HTTP middleware.')
167+
168+
169+
if __name__ == '__main__':
170+
unittest.main()

src/service/core/workflow/BUILD

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ osmo_py_library(
4141
"//src/lib/utils:osmo_errors",
4242
"//src/lib/utils:redact",
4343
"//src/lib/utils:validation",
44+
"//src/service/core:responses",
4445
"//src/service/core/config:configmap_loader_lib",
4546
"//src/utils:ssl_config",
4647
"//src/utils:static_config",

src/service/core/workflow/workflow_service.py

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
"""
1919

2020
import collections
21+
import contextlib
2122
import dataclasses
2223
import datetime
2324
import enum
@@ -37,6 +38,7 @@
3738
from src.lib.utils import common, credentials, login, osmo_errors, priority as wf_priority
3839
from src.lib.utils.redact import redact_secrets
3940
from src.utils.job import common as job_common, jobs, workflow, task
41+
from src.service.core import responses
4042
from src.service.core.workflow import helpers, objects
4143
from src.utils import connectors
4244

@@ -771,10 +773,11 @@ def get_file_info(name: str, redis_name: str, file_name: str,
771773
async def async_filter_log(log_generator: AsyncGenerator[str, None])\
772774
-> AsyncGenerator[str, None]:
773775
''' Returns whether to send the log '''
774-
async for line in log_generator:
775-
if not regexes or \
776-
all(compiled_regex.search(line) for compiled_regex in compiled_regexes):
777-
yield line
776+
async with contextlib.aclosing(log_generator) as log_stream:
777+
async for line in log_stream:
778+
if not regexes or \
779+
all(compiled_regex.search(line) for compiled_regex in compiled_regexes):
780+
yield line
778781

779782
def filter_log(log_generator: storage.LinesStream) -> Generator[str, None, None]:
780783
''' Returns whether to send the log '''
@@ -783,8 +786,9 @@ def filter_log(log_generator: storage.LinesStream) -> Generator[str, None, None]
783786
all(compiled_regex.search(line) for compiled_regex in compiled_regexes):
784787
yield line
785788

789+
response: fastapi.responses.StreamingResponse
786790
if parsed_result.scheme in ('redis', 'rediss') and not download:
787-
response = fastapi.responses.StreamingResponse(
791+
response = responses.ClosingStreamingResponse(
788792
async_filter_log(
789793
connectors.redis_log_formatter(log_info.logs, redis_name, last_n_lines)))
790794
else:

src/utils/connectors/BUILD

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ osmo_py_library(
1313
requirement("pydantic"),
1414
requirement("redis"),
1515
requirement("aiofiles"),
16+
requirement("anyio"),
1617
requirement("pyyaml"),
1718
"//src/lib/data/storage",
1819
"//src/lib/data/storage/constants",

0 commit comments

Comments
 (0)