refactor: split task group into data-plane and control-plane - #950
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
| self._request_lease_release, | ||
| ) | ||
| tg.start_soon(self.handle_lease, status.lease_name, tg, lease_scope) | ||
| conns_tg.start_soon(self.handle_lease, status.lease_name, conns_tg, lease_scope) |
There was a problem hiding this comment.
if session_for_lease() raises during handle_lease, execution jumps to the outer finally without entering the inner try/finally that cleans up. The exporter hence could hang permanently.
| async with create_task_group() as conns_tg: | ||
| await self._run_control_plane(status_tx, status_rx, conns_tg) | ||
| if self._fatal_stream_error: | ||
| name, err = self._fatal_stream_error | ||
| logger.warning( | ||
| "Control plane down (%s: %s), cancelling active connections", | ||
| name, | ||
| err, | ||
| ) | ||
| conns_tg.cancel_scope.cancel() |
There was a problem hiding this comment.
Could we try keeping connections alive during control-plane recovery?
| if previous_state == LeaseState.IDLE and status.lease_name != "": | ||
| self._on_lease_acquired(status, tg) | ||
| self._on_lease_acquired(status, tg, conns_tg) | ||
| elif ( |
There was a problem hiding this comment.
Consider clearing _lease_context in _on_lease_released after the shielded wait completes, and have the handle_lease finally guard against double-clearing.
| # Start task to handle EndSession requests (runs afterLease hook when client signals done) | ||
| tg.start_soon(self._handle_end_session, lease_scope) | ||
| # Runs on control-plane group so it's cancelled with Status/Listen, not data-plane | ||
| self._tg.start_soon(self._handle_end_session, lease_scope) |
There was a problem hiding this comment.
Consider also moving the Listen stream to self._tg so its lifecycle matches the other control-plane streams.
479b280 to
4f46d3e
Compare
a61b1ec to
2cccab8
Compare
4ddbb3d to
f0b2ec4
Compare
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
3 participants
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.
Depends on #949
Next: #951