Skip to content

Commit 479b280

Browse files
committed
refactor: split task group into data-plane and control-plane
Introduce an outer conns_tg (data-plane) that hosts handle_lease and _handle_client_conn, and an inner tg (control-plane) that hosts Status/Listen streams and _handle_end_session. When _cancel_with_fatal_error fires (Status stream terminal error), only the inner group is cancelled. Active client tunnels on conns_tg remain alive until serve() explicitly cancels the outer group. Add TestTaskGroupIsolation to verify a connection task survives control-plane cancellation. Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assisted-by: claude-opus-4.6
1 parent 74a44b0 commit 479b280

2 files changed

Lines changed: 80 additions & 20 deletions

File tree

python/packages/jumpstarter/jumpstarter/exporter/exporter.py

Lines changed: 21 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1000,7 +1000,7 @@ async def _skip_stale_lease(self, lease_name: str, lease_scope: LeaseContext, co
10001000
lease_scope.after_lease_hook_done.set()
10011001
return True
10021002

1003-
async def handle_lease(self, lease_name: str, tg: TaskGroup, lease_scope: LeaseContext) -> None:
1003+
async def handle_lease(self, lease_name: str, conns_tg: TaskGroup, lease_scope: LeaseContext) -> None:
10041004
"""Handle all incoming client connections for a lease.
10051005
10061006
This method orchestrates the complete lifecycle of managing connections during
@@ -1016,7 +1016,7 @@ async def handle_lease(self, lease_name: str, tg: TaskGroup, lease_scope: LeaseC
10161016
10171017
Args:
10181018
lease_name: Name of the lease to handle connections for
1019-
tg: TaskGroup for spawning concurrent connection handler tasks
1019+
conns_tg: Data-plane TaskGroup for spawning connection handler tasks
10201020
lease_scope: LeaseScope with before_lease_hook event (session/socket set here)
10211021
10221022
Note:
@@ -1076,7 +1076,8 @@ async def handle_lease(self, lease_name: str, tg: TaskGroup, lease_scope: LeaseC
10761076
# session creation (e.g., BEFORE_LEASE_HOOK when hooks are configured).
10771077

10781078
# Start task to handle EndSession requests (runs afterLease hook when client signals done)
1079-
tg.start_soon(self._handle_end_session, lease_scope)
1079+
# Runs on control-plane group so it's cancelled with Status/Listen, not data-plane
1080+
self._tg.start_soon(self._handle_end_session, lease_scope)
10801081

10811082
# Process client connections until lease ends
10821083
# The lease can end via:
@@ -1114,7 +1115,7 @@ async def process_connections():
11141115
lease_name,
11151116
request.router_endpoint,
11161117
)
1117-
tg.start_soon(
1118+
conns_tg.start_soon(
11181119
self._handle_client_conn,
11191120
lease_scope.socket_path,
11201121
request.router_endpoint,
@@ -1166,14 +1167,16 @@ async def serve(self):
11661167
pass
11671168
status_tx, status_rx = create_memory_object_stream[jumpstarter_pb2.StatusResponse](max_buffer_size=5)
11681169
try:
1169-
await self._run_control_plane(status_tx, status_rx)
1170-
if self._fatal_stream_error:
1171-
name, err = self._fatal_stream_error
1172-
logger.warning(
1173-
"Control plane down (%s: %s)",
1174-
name,
1175-
err,
1176-
)
1170+
async with create_task_group() as conns_tg:
1171+
await self._run_control_plane(status_tx, status_rx, conns_tg)
1172+
if self._fatal_stream_error:
1173+
name, err = self._fatal_stream_error
1174+
logger.warning(
1175+
"Control plane down (%s: %s), cancelling active connections",
1176+
name,
1177+
err,
1178+
)
1179+
conns_tg.cancel_scope.cancel()
11771180
finally:
11781181
self._tg = None
11791182
self._fatal_stream_error = None
@@ -1184,6 +1187,7 @@ async def _run_control_plane(
11841187
self,
11851188
status_tx: MemoryObjectSendStream[jumpstarter_pb2.StatusResponse],
11861189
status_rx: MemoryObjectReceiveStream[jumpstarter_pb2.StatusResponse],
1190+
conns_tg: TaskGroup,
11871191
) -> None:
11881192
"""Start control-plane streams and process status updates."""
11891193
async with create_task_group() as tg:
@@ -1199,21 +1203,22 @@ async def _run_control_plane(
11991203
status_tx,
12001204
)
12011205
async for status in status_rx:
1202-
if await self._apply_status(status, tg):
1206+
if await self._apply_status(status, tg, conns_tg):
12031207
break
12041208

12051209
async def _apply_status(
12061210
self,
12071211
status: jumpstarter_pb2.StatusResponse,
12081212
tg: TaskGroup,
1213+
conns_tg: TaskGroup,
12091214
) -> bool:
12101215
"""Process a single status update. Returns True to stop the status loop."""
12111216
previous_state = self._lease_state
12121217
current_leased = status.leased
12131218

12141219
if current_leased:
12151220
if previous_state == LeaseState.IDLE and status.lease_name != "":
1216-
self._on_lease_acquired(status, tg)
1221+
self._on_lease_acquired(status, tg, conns_tg)
12171222
elif (
12181223
previous_state == LeaseState.LEASED
12191224
and self._lease_context
@@ -1236,6 +1241,7 @@ def _on_lease_acquired(
12361241
self,
12371242
status: jumpstarter_pb2.StatusResponse,
12381243
tg: TaskGroup,
1244+
conns_tg: TaskGroup,
12391245
) -> None:
12401246
"""Handle new lease assignment: create context and spawn lease handler."""
12411247
self._started = True
@@ -1257,7 +1263,7 @@ def _on_lease_acquired(
12571263
self.stop,
12581264
self._request_lease_release,
12591265
)
1260-
tg.start_soon(self.handle_lease, status.lease_name, tg, lease_scope)
1266+
conns_tg.start_soon(self.handle_lease, status.lease_name, conns_tg, lease_scope)
12611267

12621268
def _on_lease_update(self, status: jumpstarter_pb2.StatusResponse) -> None:
12631269
"""Update client info on every leased status tick."""

python/packages/jumpstarter/jumpstarter/exporter/exporter_test.py

Lines changed: 59 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1146,7 +1146,7 @@ async def test_overlap_rejection_returns_false(self):
11461146
status.context = {}
11471147

11481148
async with create_task_group() as tg:
1149-
result = await exporter._apply_status(status, tg)
1149+
result = await exporter._apply_status(status, tg, tg)
11501150
tg.cancel_scope.cancel()
11511151

11521152
assert result is False
@@ -1164,7 +1164,7 @@ async def test_overlap_same_lease_name_not_rejected(self):
11641164
status.context = {}
11651165

11661166
async with create_task_group() as tg:
1167-
result = await exporter._apply_status(status, tg)
1167+
result = await exporter._apply_status(status, tg, tg)
11681168
tg.cancel_scope.cancel()
11691169

11701170
assert result is False
@@ -1187,7 +1187,7 @@ async def fake_handle_lease(lease_name, tg, lease_scope):
11871187
status.context = {}
11881188

11891189
async with create_task_group() as tg:
1190-
result = await exporter._apply_status(status, tg)
1190+
result = await exporter._apply_status(status, tg, tg)
11911191
await anyio.sleep(0.05)
11921192
tg.cancel_scope.cancel()
11931193

@@ -1222,7 +1222,7 @@ async def fake_handle_lease(lease_name, tg, lease_scope):
12221222
status.context = {"env": "staging"}
12231223

12241224
async with create_task_group() as tg:
1225-
await exporter._apply_status(status, tg)
1225+
await exporter._apply_status(status, tg, tg)
12261226
await anyio.sleep(0.05)
12271227
tg.cancel_scope.cancel()
12281228

@@ -1244,7 +1244,7 @@ async def test_leased_to_idle_calls_on_lease_released(self):
12441244
status.context = {}
12451245

12461246
async with create_task_group() as tg:
1247-
await exporter._apply_status(status, tg)
1247+
await exporter._apply_status(status, tg, tg)
12481248
tg.cancel_scope.cancel()
12491249

12501250

@@ -1296,6 +1296,7 @@ async def fake_retry_stream(stream_name, stream_factory, send_tx, **kwargs):
12961296
exporter._cleanup_after_lease = AsyncMock()
12971297

12981298
async with create_task_group() as tg:
1299+
exporter._tg = tg
12991300
tg.start_soon(exporter.handle_lease, "conn-lease", tg, lease_ctx)
13001301
with fail_after(5):
13011302
await conn_arrived.wait()
@@ -1339,6 +1340,7 @@ async def fake_retry_stream(stream_name, stream_factory, send_tx, **kwargs):
13391340
exporter._listen_stream_factory = MagicMock(return_value=MagicMock())
13401341

13411342
async with create_task_group() as tg:
1343+
exporter._tg = tg
13421344
tg.start_soon(exporter.handle_lease, "fallback-lease", tg, lease_ctx)
13431345
await anyio.sleep(0.1)
13441346
lease_ctx.lease_ended.set()
@@ -1582,3 +1584,55 @@ def tracking_set(**kwargs):
15821584

15831585
assert calls == [{"client": "ci-bot"}]
15841586
clear_log_context()
1587+
1588+
1589+
class TestTaskGroupIsolation:
1590+
"""Verify that control-plane failure does not cancel data-plane connections.
1591+
1592+
The split: inner tg (control-plane: Status/Listen streams) and outer
1593+
conns_tg (data-plane: handle_lease, _handle_client_conn). When
1594+
_cancel_with_fatal_error cancels tg, connections on conns_tg must
1595+
remain alive until serve() explicitly cancels conns_tg.
1596+
"""
1597+
1598+
@pytest.mark.anyio
1599+
async def test_conn_alive_after_control_plane_cancel(self):
1600+
"""Between _cancel_with_fatal_error and serve() cancelling conns_tg,
1601+
connection tasks on conns_tg are still running."""
1602+
exporter = _make_serve_exporter()
1603+
conn_alive_after_cp_cancel = False
1604+
conn_started = Event()
1605+
cp_cancelled = Event()
1606+
1607+
async def fake_conn():
1608+
nonlocal conn_alive_after_cp_cancel
1609+
conn_started.set()
1610+
await cp_cancelled.wait()
1611+
conn_alive_after_cp_cancel = True
1612+
1613+
async def fake_retry_stream(name, factory, tx, **kwargs):
1614+
if name == "Status":
1615+
await tx.send(
1616+
MagicMock(leased=True, lease_name="test-lease", client_name="c", context={})
1617+
)
1618+
await conn_started.wait()
1619+
exporter._cancel_with_fatal_error("Status", Exception("controller gone"))
1620+
cp_cancelled.set()
1621+
else:
1622+
await anyio.sleep_forever()
1623+
1624+
exporter._retry_stream = fake_retry_stream
1625+
1626+
async def fake_handle_lease(lease_name, conns_tg, lease_ctx):
1627+
conns_tg.start_soon(fake_conn)
1628+
await lease_ctx.lease_ended.wait()
1629+
lease_ctx.after_lease_hook_done.set()
1630+
1631+
exporter.handle_lease = fake_handle_lease
1632+
1633+
await exporter.serve()
1634+
1635+
assert conn_alive_after_cp_cancel, (
1636+
"Connection task was killed before serve() cancelled conns_tg — "
1637+
"control-plane cancellation leaked into data-plane"
1638+
)

0 commit comments

Comments
 (0)