From 9e108b80baa5f1c061cb8063d1367fd796f2b5fe Mon Sep 17 00:00:00 2001 From: Wyatt Childers Date: Fri, 23 Oct 2026 14:28:46 +0000 Subject: [PATCH] Improve deployment stability by adding health check endpoints [] This allows tests to know when services are ready/pauses "docker compose up"'s return until services are actually listening. This also refactors the deployment to use Quart for the servers (consistent with the AcknowlEDG API server) so that developers do not need to maintain knowledge of multiple WebSocket server patterns. Additionally, management of process pool jobs has been improved so that a task taking longer than they are allowed, responding to a review no longer being monitored, or similar, are properly cleaned up without negatively impacting other tasks. In a similar "reliability" vein, caps have been added to ensure input data stays within reasonable bounds to preserve service health. Finally, a number of cleanup passes have been taken to improve code quality. --- dev_tools/bin/edgy | 33 +- dev_tools/bin/ifc-qt | 10 +- .../pylibs/edgacknowledg/client/__init__.py | 167 +++--- .../edgacknowledg/client/bench/__init__.py | 16 +- .../pylibs/edgacknowledg/client/shared.py | 13 + .../edgacknowledg/client/test/__init__.py | 17 +- .../pylibs/edgacknowledg/client/websocket.py | 15 +- .../pylibs/edgacknowledg/forge/config.py | 26 +- .../pylibs/edgacknowledg/forge/db/store.py | 70 ++- dev_tools/pylibs/edgacknowledg/forge/local.py | 50 +- .../pylibs/edgacknowledg/forge/processor.py | 128 +++-- .../edgacknowledg/forge/publisher/__init__.py | 71 ++- .../forge/publisher/acknowledg.py | 244 +++++---- .../edgacknowledg/forge/publisher/client.py | 381 +++++++------- .../edgacknowledg/forge/publisher/pool.py | 311 +++++++---- .../forge/publisher/pool_process.py | 38 +- .../edgacknowledg/forge/publisher/protocol.py | 20 +- .../edgacknowledg/forge/publisher/server.py | 304 ++++++----- .../edgacknowledg/forge/runner/client.py | 229 ++++---- .../edgacknowledg/forge/runner/server.py | 238 +++++---- .../pylibs/edgacknowledg/forge/sandbox.py | 4 +- .../edgacknowledg/forge/sandbox_client.py | 160 ++++++ .../pylibs/edgacknowledg/forge/service.py | 71 ++- .../pylibs/edgacknowledg/forge/serving.py | 144 ++++++ .../pylibs/edgacknowledg/forge/webhooks.py | 28 +- .../pylibs/edgacknowledg/forge/workspace.py | 197 +++++-- .../pylibs/edgifc/decoration/__init__.py | 4 +- dev_tools/pylibs/edgtest/__init__.py | 9 + dev_tools/pylibs/edgtest/runtest/__init__.py | 4 +- dev_tools/pylibs/edgutil/__init__.py | 85 ++- dev_tools/services/acknowledg-forge/README.md | 10 +- .../services/acknowledg-forge/SANDBOXING.md | 198 +++---- .../services/acknowledg-forge/compose.yaml | 54 +- .../acknowledg-forge/run-dev-oss-repo.sh | 3 +- .../acknowledg-forge/tests/conftest.py | 23 +- .../services/acknowledg-forge/tests/fakes.py | 50 +- .../tests/test_acknowledg_integration.py | 123 ++++- .../tests/test_client_cancellation.py | 208 ++++++++ .../acknowledg-forge/tests/test_containers.py | 57 +- .../acknowledg-forge/tests/test_pool.py | 93 +++- .../acknowledg-forge/tests/test_publisher.py | 488 +++++++++++++++--- .../acknowledg-forge/tests/test_runner.py | 184 ++++++- .../acknowledg-forge/tests/test_safety.py | 186 ++++++- .../acknowledg-forge/tests/test_state.py | 149 +++++- .../acknowledg-forge/tests/test_symlinks.py | 47 +- .../server/acknowledg_server/__init__.py | 15 + dev_tools/tests/test_edgtest_suites.py | 42 ++ dev_tools/tests/test_edgutil.py | 122 +++++ dev_tools/typecheck-python.sh | 2 +- 49 files changed, 3758 insertions(+), 1383 deletions(-) create mode 100644 dev_tools/pylibs/edgacknowledg/forge/sandbox_client.py create mode 100644 dev_tools/pylibs/edgacknowledg/forge/serving.py create mode 100644 dev_tools/services/acknowledg-forge/tests/test_client_cancellation.py create mode 100644 dev_tools/tests/test_edgtest_suites.py create mode 100644 dev_tools/tests/test_edgutil.py diff --git a/dev_tools/bin/edgy b/dev_tools/bin/edgy index 6058130b7f..81161aa491 100755 --- a/dev_tools/bin/edgy +++ b/dev_tools/bin/edgy @@ -255,15 +255,36 @@ def load_edgy_config(args: argparse.Namespace, test_home: Path) -> EdgyConfig: ) test_suites = config_file_dict.get('test_suites', {}) + primary_test_suites = test_suites.get('primary', []) + secondary_test_suites = test_suites.get('secondary', []) + _validate_configured_suites( + config_file_path, + [*primary_test_suites, *secondary_test_suites] + ) config_sets = config_file_dict.get('config_sets', {}) return EdgyConfig( test_home, loaded_configs, config_sets, - test_suites.get('primary', []), - test_suites.get('secondary', []) + primary_test_suites, + secondary_test_suites ) +def _validate_configured_suites(config_file_path: Path, + test_suite_names: List[Any]) -> None: + '''Exit unless every suite ``config_file_path`` lists has a valid name.''' + invalid_test_suite_encountered = False + for test_suite in test_suite_names: + if not (isinstance(test_suite, str) and + edgtest.is_valid_suite_name(test_suite)): + edgutil.eprint(f"Invalid test suite name {test_suite!r} in " + f"{config_file_path} (expected 1-64 of A-Z, a-z, " + "0-9, _ and -).") + invalid_test_suite_encountered = True + + if invalid_test_suite_encountered: + sys.exit(1) + def get_expectation_path(run_config: TestRunConfig, test_suite_name: str) -> Path: return Path(run_config.expectations_dir, f"{test_suite_name}.log") @@ -627,7 +648,7 @@ def _diagnose_bad_expectations_file(file_path: Path) -> None: line_ctr += 1 try: parse_test_status(line_text) - except: + except Exception: if len(line_text) != 0: edgutil.eprint( f"{file_path_str}, line {line_ctr}: " @@ -668,7 +689,7 @@ def _construct_replay_filters( ) for key in comparison: desired_tests.add(parsed_test_status_key_to_name(key)) - except: + except Exception: edgutil.eprint( f"Failed to compare the previous run of the {test_suite.name} " 'suite to its expectations.' @@ -691,7 +712,7 @@ def _construct_replay_filters( elif (args.replay == 'output-mismatch' and parsed_test_status.is_output_mismatch()): desired_tests.add(parsed_test_status_key_to_name(key)) - except: + except Exception: edgutil.eprint( f"Failed to parse test statuses in previous run of {test_suite.name}." ) @@ -1233,7 +1254,7 @@ class TestStatusChecker: encountered_lines: List[str]) -> Optional['TestStatusChecker']: try: return TestStatusChecker(expected_lines, encountered_lines) - except: + except Exception: return None def _get_status_for_comparison( diff --git a/dev_tools/bin/ifc-qt b/dev_tools/bin/ifc-qt index a4f9584772..a80fcdd12a 100755 --- a/dev_tools/bin/ifc-qt +++ b/dev_tools/bin/ifc-qt @@ -28,7 +28,11 @@ import edgifc.decoration from enum import IntEnum from pathlib import Path -from PySide6 import QtCore, QtWidgets, QtGui # type: ignore[import-not-found,import] +from PySide6 import ( # type: ignore[import-not-found,import] + QtCore, + QtWidgets, + QtGui +) from edgifc import ( IFCReader, @@ -383,7 +387,7 @@ class PartitionNodeFormatter: values.append(template_string.format(value.description)) else: values.append(format_fn(value)) - except: + except Exception: traceback.print_exc() values.append(edgifc.decoration.UNKNOWN_VALUE) @@ -428,7 +432,7 @@ class PartitionNodeFormatter: stream_reader, tacit_field.name() ) - except: + except Exception: traceback.print_exc() field_value = 0 diff --git a/dev_tools/pylibs/edgacknowledg/client/__init__.py b/dev_tools/pylibs/edgacknowledg/client/__init__.py index 26d89caec8..f98bce09e7 100644 --- a/dev_tools/pylibs/edgacknowledg/client/__init__.py +++ b/dev_tools/pylibs/edgacknowledg/client/__init__.py @@ -400,8 +400,12 @@ async def _send_reply_best_effort(channel: Channel, async def _reply_to_failed_request(channel: Channel, - handler: ReviewClientHandler) -> None: - '''Answer a request whose handler raised, and log why. + handler: ReviewClientHandler, + error: BaseException) -> None: + '''Tell the reviewer whose request failed with ``error`` (the handler + records them on it, in ``reviewer_id``) that it did, through the server, + and log why. With no reviewer to tell, it is only logged: the server + could forward the reply to no one. The section keeps serving afterwards: one bad request (which a reviewer can trigger) shouldn't take the whole section down. @@ -410,7 +414,12 @@ async def _reply_to_failed_request(channel: Channel, f"Request to section {handler.subscription_type}/{handler.slug} failed:" ) traceback.print_exc() - await _send_reply_best_effort(channel, message(RequestTags.INVALID_REQUEST)) + reviewer_id = getattr(error, 'reviewer_id', None) + if reviewer_id is not None: + await _send_reply_best_effort(channel, message( + RequestTags.INVALID_REQUEST, + value = {'reviewer_id': reviewer_id} + )) async def _report_illegal_access(channel: Channel, @@ -422,7 +431,7 @@ async def _report_illegal_access(channel: Channel, ''' if error.reviewer_id is None: # Not traced to a reviewer; just a failed request. - await _reply_to_failed_request(channel, handler) + await _reply_to_failed_request(channel, handler, error) else: eprint( f"Reviewer {error.reviewer_id} of section " @@ -476,16 +485,25 @@ def sections(self) -> List[ReviewClientHandler]: return self._review.sections() async def add_section(self, handler: ReviewClientHandler) -> None: - '''Register handler and subscribe it on the batch socket.''' + '''Register handler and subscribe it on the batch socket; while not + connected, it is subscribed once the session (re)connects, with every + other registered section. + ''' # Serialized with reconnect restores so a section added mid-restore is # subscribed exactly once. async with self._client._subscriptions_lock: self._review.register_handler(handler) - try: - await self.subscribe_section(handler) - except Exception: - self._review.discard_handler(handler) - raise + # Not connected (sending would wait for it): the next session's + # restore subscribes it. + if self._client._session_ready.is_set(): + try: + await self.subscribe_section(handler) + except NotConnectedError: + pass + except BaseException: + # Failed or cancelled: not added. + self._review.discard_handler(handler) + raise async def remove_section(self, handler: ReviewClientHandler) -> None: '''Unsubscribe, then unregister.''' @@ -513,8 +531,8 @@ async def _run_handler_loop(self, raise except IllegalFileAccessError as ex: await _report_illegal_access(channel, handler, ex) - except Exception: - await _reply_to_failed_request(channel, handler) + except Exception as ex: + await _reply_to_failed_request(channel, handler, ex) finally: self._client._inbounds.pop(channel.subscription_id, None) self._subscription_tasks.pop(channel.subscription_id, None) @@ -637,7 +655,6 @@ def __init__(self, self._client = client self._review = _ClientReview(review_id, name, timestamp) self._section_tasks: Dict[int, asyncio.Task] = {} - self._section_first_ready: Dict[int, asyncio.Event] = {} @property def review_id(self) -> uuid.UUID: @@ -659,7 +676,8 @@ async def add_section(self, handler: ReviewClientHandler) -> None: self._review.register_handler(handler) try: await self.subscribe_section(handler) - except Exception: + except BaseException: + # Failed or cancelled: not added. self._review.discard_handler(handler) raise @@ -681,13 +699,8 @@ async def _run_handler_loop(self, raise except IllegalFileAccessError as ex: await _report_illegal_access(channel, handler, ex) - except Exception: - await _reply_to_failed_request(channel, handler) - - def _signal_section_ready(self, handler_key: int) -> None: - ready = self._section_first_ready.get(handler_key) - if ready is not None and not ready.is_set(): - ready.set() + except Exception as ex: + await _reply_to_failed_request(channel, handler, ex) async def _section_connection_loop(self, handler: ReviewClientHandler) -> None: @@ -710,34 +723,33 @@ async def _section_connection_loop(self, open_timeout = self._client._open_timeout, status_listener = self._client._status_fan_in ) - try: - async for websocket in resilient: - if (self._client._closed or - handler_key not in self._section_tasks): - break + async for websocket in resilient: + if (self._client._closed or + handler_key not in self._section_tasks): + break - channel = _DirectChannel(websocket) - try: - await announce_section(channel, handler, self._review) - self._signal_section_ready(handler_key) - await self._run_handler_loop(handler, channel) - except RoleOccupied: - self._section_tasks.pop(handler_key, None) - self._review.discard_handler(handler) - break - except IncompatibleProtocolVersion as ex: - self._client.abort(ex) - break - except ConnectionClosed: - continue + channel = _DirectChannel(websocket) + try: + await announce_section(channel, handler, self._review) + await self._run_handler_loop(handler, channel) + except RoleOccupied: + self._section_tasks.pop(handler_key, None) + self._review.discard_handler(handler) + break + except IncompatibleProtocolVersion as ex: + self._client.abort(ex) + break + except ConnectionClosed: + continue - if (self._client._closed or - handler_key not in self._section_tasks): - break - finally: - self._signal_section_ready(handler_key) + if (self._client._closed or + handler_key not in self._section_tasks): + break async def subscribe_section(self, handler: ReviewClientHandler) -> None: + '''Start keeping a socket for handler's section; as with a batch + session that isn't connected, it is served once that socket connects. + ''' self._client._require_connected() handler_key = id(handler) if handler_key in self._section_tasks: @@ -746,29 +758,15 @@ async def subscribe_section(self, handler: ReviewClientHandler) -> None: handler.subscription_type ) - ready = asyncio.Event() - self._section_first_ready[handler_key] = ready loop = get_running_event_loop() # Register the key before starting so the loop treats this section as # wanted; unsubscribe pops the key to stop reconnecting. task = loop.create_task(self._section_connection_loop(handler)) self._section_tasks[handler_key] = task - try: - await ready.wait() - if handler_key not in self._section_tasks: - raise NotConnectedError( - f"section failed to connect: " - f"{handler.subscription_type}/{handler.slug}" - ) - finally: - self._section_first_ready.pop(handler_key, None) async def unsubscribe_section(self, handler: ReviewClientHandler) -> None: handler_key = id(handler) task = self._section_tasks.pop(handler_key, None) - ready = self._section_first_ready.pop(handler_key, None) - if ready is not None and not ready.is_set(): - ready.set() if task is not None: task.cancel() try: @@ -1019,18 +1017,21 @@ async def _recv_loop(self, websocket: ClientConnection) -> None: subscription_id = uuid.UUID(str(value['subscription_id'])) except (KeyError, ValueError, TypeError): continue - # Bind inbound routing before completing the subscribe future so - # early batch.event frames are not dropped. - if subscription_id not in self._inbounds: - self._inbounds[subscription_id] = asyncio.Queue() key = ( str(value.get('id')), str(value.get('type')), str(value.get('slug', '')), ) future = self._pending_subscribe.pop(key, None) - if future is not None and not future.done(): - future.set_result(value) + if future is None or future.done(): + # Whoever subscribed gave up: claimed by no one. + await self._release(subscription_id) + continue + # Bind inbound routing before completing the subscribe future so + # early batch.event frames are not dropped. + if subscription_id not in self._inbounds: + self._inbounds[subscription_id] = asyncio.Queue() + future.set_result(value) elif tag == RequestTags.BATCH_UNSUBSCRIBE: if not isinstance(value, dict): @@ -1079,9 +1080,18 @@ async def _subscribe(self, payload: Dict[str, Any]) -> uuid.UUID: SUBSCRIBE_TIMEOUT_S, f"subscribe {key[1]}/{key[2]}" ) - except Exception: - self._pending_subscribe.pop(key, None) - if not future.done(): + except BaseException: + # Failed or cancelled: no longer awaited. + if self._pending_subscribe.get(key) is future: + del self._pending_subscribe[key] + if (future.done() and not future.cancelled() and + future.exception() is None): + # Answered all the same: claimed by no one. Shielded, so being + # cancelled again can't keep the release from being sent. + await asyncio.shield(self._release( + uuid.UUID(str(future.result()['subscription_id'])) + )) + elif not future.done(): future.cancel() raise subscription_id = uuid.UUID(str(echo['subscription_id'])) @@ -1101,12 +1111,31 @@ async def _unsubscribe(self, subscription_id: uuid.UUID) -> None: UNSUBSCRIBE_TIMEOUT_S, f"unsubscribe {subscription_id}" ) - except Exception: + except BaseException: + # Failed or cancelled: no longer awaited. self._pending_unsubscribe.pop(subscription_id, None) if not future.done(): future.cancel() raise + async def _release(self, subscription_id: uuid.UUID) -> None: + '''Have the server end a subscription nothing here claimed; its answer + isn't awaited (``_recv_loop`` drops it). + + Sent only while connected: the server ends a subscription with its + connection, so there's nothing to wait for a reconnect to release. + ''' + self._inbounds.pop(subscription_id, None) + connection = self._connection + if connection is not None: + try: + await connection.send_if_connected(encode_message(message( + RequestTags.BATCH_UNSUBSCRIBE, + value = { 'subscription_id': str(subscription_id) } + ))) + except ConnectionClosed: + pass + def _make_channel(self, subscription_id: uuid.UUID) -> _SubscriptionChannel: inbound = self._inbounds.setdefault(subscription_id, asyncio.Queue()) return _SubscriptionChannel(subscription_id, inbound, self) diff --git a/dev_tools/pylibs/edgacknowledg/client/bench/__init__.py b/dev_tools/pylibs/edgacknowledg/client/bench/__init__.py index fdf093b6da..616ce73ee7 100644 --- a/dev_tools/pylibs/edgacknowledg/client/bench/__init__.py +++ b/dev_tools/pylibs/edgacknowledg/client/bench/__init__.py @@ -33,6 +33,7 @@ ConfinedPathComponent, IllegalFileAccessError, SandboxContext, + reviewer_of, trunc_display_content, ) import edgutil @@ -280,7 +281,9 @@ async def run_bench_source_handler(channel: Channel, request_tag, request_value, _section = decode_response(await channel.recv()) if request_value is None or 'reviewer_id' not in request_value: - await channel.send(message(RequestTags.INVALID_REQUEST)) + # Not a reviewer's request (none names one) but the server's, e.g. its + # error for a reply it refused: answering would only bounce back and + # forth. return reviewer_id = request_value['reviewer_id'] @@ -340,8 +343,8 @@ async def annotate_job() -> str: value = { 'reviewer_id': reviewer_id } )) except Exception as ex: - if isinstance(ex, IllegalFileAccessError): - ex.reviewer_id = reviewer_id + # Whom the failure reply (or the illegal-access report) is for. + setattr(ex, 'reviewer_id', reviewer_id) if sys.version_info >= (3, 11): ex.add_note(f"request triggered by reviewer: \"{reviewer_id}\"") raise ex @@ -380,7 +383,10 @@ async def handle_bench_edit_request(session: BenchReviewSession, } ) - return message(RequestTags.INVALID_REQUEST) + return message( + RequestTags.INVALID_REQUEST, + value = {'reviewer_id': request_value['reviewer_id']} + ) class BenchSourceHandler: @@ -441,6 +447,8 @@ def slug(self) -> str: async def handle_request(self, channel: Channel) -> None: request = await channel.recv() + if reviewer_of(request) is None: + return async def edit_job() -> None: reply = await handle_bench_edit_request(self._session, request) diff --git a/dev_tools/pylibs/edgacknowledg/client/shared.py b/dev_tools/pylibs/edgacknowledg/client/shared.py index 05281849a9..675f781cc1 100644 --- a/dev_tools/pylibs/edgacknowledg/client/shared.py +++ b/dev_tools/pylibs/edgacknowledg/client/shared.py @@ -40,6 +40,7 @@ ProtocolMessage, RequestTags, RichName, + decode_response, message, ) @@ -176,6 +177,18 @@ def trunc_display_content(content: str) -> Dict[str, Union[str, bool]]: return { 'content': content } +def reviewer_of(request: Any) -> Optional[str]: + '''The reviewer who made ``request``, as a handler receives it (the + server names them in every reviewer's request); ``None`` if it isn't a + reviewer's but the server's, e.g. its error for a reply it refused, + which a handler doesn't answer (an answer would only bounce back and + forth). + ''' + _tag, value, _section = decode_response(request) + reviewer_id = value.get('reviewer_id') if isinstance(value, dict) else None + return None if reviewer_id is None else str(reviewer_id) + + class IllegalFileAccessError(Exception): '''Raised when a request asks for a file outside the allowed tree. diff --git a/dev_tools/pylibs/edgacknowledg/client/test/__init__.py b/dev_tools/pylibs/edgacknowledg/client/test/__init__.py index 970b5b8582..75b759a754 100644 --- a/dev_tools/pylibs/edgacknowledg/client/test/__init__.py +++ b/dev_tools/pylibs/edgacknowledg/client/test/__init__.py @@ -40,6 +40,7 @@ ConfinedPath, IllegalFileAccessError, SandboxContext, + reviewer_of, trunc_display_content, ) from edgutil import eprint @@ -206,6 +207,9 @@ def test_id_to_path(test_id: str) -> ConfinedPath: return test_home.joinpath('tests', *test_id_parts) + # Only a reviewer's request is answered: one naming none is the + # server's, e.g. its error for a reply it refused, and answering it would + # only bounce back and forth. if request_value is not None and 'reviewer_id' in request_value: reviewer_id = request_value['reviewer_id'] @@ -568,13 +572,11 @@ async def form_curr_diff_text(config_name: str, } )) except Exception as ex: - if isinstance(ex, IllegalFileAccessError): - ex.reviewer_id = reviewer_id + # Whom the failure reply (or the illegal-access report) is for. + setattr(ex, 'reviewer_id', reviewer_id) if sys.version_info >= (3, 11): ex.add_note(f"request triggered by reviewer: \"{reviewer_id}\"") raise ex - else: - await channel.send(message(RequestTags.INVALID_REQUEST)) class TestSourceHandler: @@ -702,7 +704,10 @@ async def handle_test_edit_request( } ) - return message(RequestTags.INVALID_REQUEST) + return message( + RequestTags.INVALID_REQUEST, + value = {'reviewer_id': request_value['reviewer_id']} + ) class TestEditHandler: @@ -729,6 +734,8 @@ def slug(self) -> str: async def handle_request(self, channel: Channel) -> None: request = await channel.recv() + if reviewer_of(request) is None: + return async def edit_job() -> None: reply = await handle_test_edit_request(request) diff --git a/dev_tools/pylibs/edgacknowledg/client/websocket.py b/dev_tools/pylibs/edgacknowledg/client/websocket.py index f4814bb05d..1b4e467c30 100644 --- a/dev_tools/pylibs/edgacknowledg/client/websocket.py +++ b/dev_tools/pylibs/edgacknowledg/client/websocket.py @@ -26,7 +26,10 @@ import websockets try: - from websockets.asyncio.client import ClientConnection, connect # type: ignore + from websockets.asyncio.client import ( # type: ignore + ClientConnection, + connect, + ) _LEGACY_WEBSOCKETS = False except Exception: from websockets.client import connect # type: ignore @@ -289,3 +292,13 @@ async def send(self, data: Any) -> None: raise _connection_closed() async with self._send_lock: await websocket.send(data) + + async def send_if_connected(self, data: Any) -> None: + '''Send on the current connection, if there is one; otherwise drop + ``data``, for what doesn't outlast a connection. + ''' + websocket = self._websocket + if websocket is None: + return + async with self._send_lock: + await websocket.send(data) diff --git a/dev_tools/pylibs/edgacknowledg/forge/config.py b/dev_tools/pylibs/edgacknowledg/forge/config.py index 5e08ff4aed..af6e034249 100644 --- a/dev_tools/pylibs/edgacknowledg/forge/config.py +++ b/dev_tools/pylibs/edgacknowledg/forge/config.py @@ -87,8 +87,11 @@ class ForgeServiceConfig: api_url: Optional[str] # Seconds between polls; 0 polls once at startup only. poll_interval: float - # 'host:port' to serve webhooks on; None runs no web server at all. - webhook_bind: Optional[str] + # 'host:port' to serve the web app (serving) on: health checks, and webhooks + # if enabled; None serves none. + service_bind: Optional[str] + # Whether the web app receives webhooks. + webhooks: bool comments: bool # CI workflow (GitHub workflow file or id; GitLab: unused) whose runs are # tracked; None disables CI tracking. @@ -182,6 +185,22 @@ def get_list(name: str) -> tuple[str, ...]: if max_concurrency < 1: raise ConfigError(f"{ENV_PREFIX}MAX_CONCURRENCY must be at least 1") + service_bind = get('SERVICE_BIND') + if service_bind is not None: + host, _, port = service_bind.rpartition(':') + if host == '' or not port.isdigit(): + raise ConfigError(f"{ENV_PREFIX}SERVICE_BIND must be host:port") + webhooks = get_flag('WEBHOOKS') + if get('WEBHOOK_BIND') is not None: + raise ConfigError( + f"{ENV_PREFIX}WEBHOOK_BIND is gone: set {ENV_PREFIX}SERVICE_BIND " + f"(compose.yaml does) and {ENV_PREFIX}WEBHOOKS=1 instead" + ) + if webhooks and service_bind is None: + raise ConfigError( + f"{ENV_PREFIX}WEBHOOKS is set, but {ENV_PREFIX}SERVICE_BIND is not" + ) + unsandboxed = get_flag('UNSANDBOXED') builder_service_url = service_url('BUILDER', environ = environ) publisher_service_url = service_url('PUBLISHER', environ = environ) @@ -208,7 +227,8 @@ def get_list(name: str) -> tuple[str, ...]: base_branches = frozenset(get_list('BASE_BRANCHES') or ('main',)), api_url = get('API_URL'), poll_interval = get_number('POLL_INTERVAL', 300), - webhook_bind = get('WEBHOOK_BIND'), + service_bind = service_bind, + webhooks = webhooks, comments = get_flag('COMMENTS'), ci_workflow = get('CI_WORKFLOW'), settle_timeout = get_number('SETTLE_TIMEOUT', 2 * 60 * 60), diff --git a/dev_tools/pylibs/edgacknowledg/forge/db/store.py b/dev_tools/pylibs/edgacknowledg/forge/db/store.py index 40dc78205f..55093b8d77 100644 --- a/dev_tools/pylibs/edgacknowledg/forge/db/store.py +++ b/dev_tools/pylibs/edgacknowledg/forge/db/store.py @@ -364,6 +364,16 @@ async def find_pull_request(self, row = result.scalar_one_or_none() return _pull_request_record(row) if row is not None else None + async def find_pull_request_by_review(self, + review_id: uuid.UUID + ) -> Optional[PullRequestRecord]: + async with self._session() as session: + result = await session.execute( + select(PullRequest).where(PullRequest.review_id == review_id) + ) + row = result.scalar_one_or_none() + return _pull_request_record(row) if row is not None else None + async def open_pull_requests(self, forge: str, repository: Optional[str] = None @@ -451,36 +461,66 @@ async def mark_processed(self, row.next_attempt_at = None row.last_error = None - async def mark_failed(self, - pull_request_id: int, - generation: int, - error: str, - *, - now: datetime, - max_attempts: int, - base_delay: float, - max_delay: float) -> bool: - '''Record a processing failure and schedule a retry with exponential - backoff. After ``max_attempts`` the service gives up until something - about the pull request changes. Returns whether it gave up. + async def begin_attempt(self, + pull_request_id: int, + generation: int, + *, + now: datetime, + max_attempts: int, + base_delay: float, + max_delay: float) -> bool: + '''Count an attempt at processing ``generation``, before it starts, + as a failure: so one that never finishes (the service killed part-way, + say by a pull request that exhausts its memory) counts too. It + schedules the retry, with exponential backoff, and records why, until + ``mark_processed`` or ``mark_failed`` says how it went (or + ``refund_attempt`` takes it back). After ``max_attempts`` the service + gives up until something about the pull request changes. Returns + whether it gave up (then there is no attempt). ''' async with self._session() as session, session.begin(): row = await session.get(PullRequest, pull_request_id) if row is None: return True - row.last_error = error if row.generation != generation: - # Changed while processing; retry right away with the new state. + # Changed since; the attempt is at the new state. return False - row.error_count += 1 if row.error_count >= max_attempts: row.processed_generation = generation row.next_attempt_at = None return True + row.error_count += 1 + row.last_error = 'processing never finished (the service stopped)' delay = min(max_delay, base_delay * 2 ** (row.error_count - 1)) row.next_attempt_at = now + timedelta(seconds = delay) return False + async def mark_failed(self, + pull_request_id: int, + generation: int, + error: str) -> None: + '''Record why the attempt at ``generation`` (counted already, by + ``begin_attempt``) failed. + ''' + async with self._session() as session, session.begin(): + row = await session.get(PullRequest, pull_request_id) + # Changed while processing: its counts were reset, to retry at once. + if row is not None and row.generation == generation: + row.last_error = error + + async def refund_attempt(self, record: PullRequestRecord) -> None: + '''Take back the attempt ``begin_attempt`` counted at ``record``'s + generation, which didn't fail through the pull request's fault (the + publisher couldn't be reached, or the service is stopping): as it was + in ``record``, read before. + ''' + async with self._session() as session, session.begin(): + row = await session.get(PullRequest, record.id) + if row is not None and row.generation == record.generation: + row.error_count = record.error_count + row.next_attempt_at = record.next_attempt_at + row.last_error = record.last_error + async def sections(self, pull_request_id: int) -> list[SectionRecord]: async with self._session() as session: result = await session.execute( diff --git a/dev_tools/pylibs/edgacknowledg/forge/local.py b/dev_tools/pylibs/edgacknowledg/forge/local.py index e1ba4bba61..7b1e78320c 100644 --- a/dev_tools/pylibs/edgacknowledg/forge/local.py +++ b/dev_tools/pylibs/edgacknowledg/forge/local.py @@ -24,28 +24,28 @@ from pathlib import Path from typing import AsyncIterator -from websockets.asyncio.server import Server +from quart import Blueprint -from edgacknowledg.forge.publisher.server import PublisherServer +from edgacknowledg.forge import serving +from edgacknowledg.forge.publisher.protocol import MESSAGE_LIMIT +from edgacknowledg.forge.publisher.server import create_publisher from edgacknowledg.forge.runner.client import RunnerClient from edgacknowledg.forge.runner.roles import ROLES -from edgacknowledg.forge.runner.server import RunnerServer +from edgacknowledg.forge.runner.server import MAX_REQUEST_BYTES, create_runner +from edgacknowledg.forge.serving import Listening HOST = '127.0.0.1' -def _url(server: Server) -> str: - port = next(iter(server.sockets)).getsockname()[1] - return f"ws://{HOST}:{port}" - - @contextlib.asynccontextmanager -async def _serving(server: Server) -> AsyncIterator[Server]: +async def _serving(server: Blueprint, + max_message_size: int) -> AsyncIterator[Listening]: + listening = await serving.start(server, host = HOST, port = 0, + max_message_size = max_message_size) try: - yield server + yield listening finally: - server.close() - await server.wait_closed() + await listening.close() @dataclass(frozen = True) @@ -53,7 +53,6 @@ class LocalDeployment: builder: RunnerClient tools: RunnerClient publisher_service_url: str - publisher_server: PublisherServer @contextlib.asynccontextmanager @@ -64,13 +63,10 @@ async def local_runner(role: str, '''Serve ``role``'s runner server (see ``runner.roles``) for the deployment in ``home``; yield its client. ''' - server = RunnerServer(ROLES[role](home = home, tools_dir = tools_dir)) - async with _serving(await server.start(HOST, 0)) as listening: - client = RunnerClient(_url(listening)) - try: - yield client - finally: - await client.close() + runner = create_runner(ROLES[role](home = home, tools_dir = tools_dir)) + async with (_serving(runner, MAX_REQUEST_BYTES) as listening, + RunnerClient(listening.url()) as client): + yield client @contextlib.asynccontextmanager @@ -87,15 +83,13 @@ async def local_deployment(*, tools = await stack.enter_async_context( local_runner('tools', home = home, tools_dir = tools_dir) ) - publisher_server = PublisherServer(tools_service_url = tools.url) - # Its pool stops once it stops listening. - stack.push_async_callback(publisher_server.close) - listening = await stack.enter_async_context( - _serving(await publisher_server.start(HOST, 0)) - ) + # Its pool runs while it serves. + listening = await stack.enter_async_context(_serving( + create_publisher(tools_service_url = tools.url), + MESSAGE_LIMIT + )) yield LocalDeployment( builder = builder, tools = tools, - publisher_service_url = _url(listening), - publisher_server = publisher_server + publisher_service_url = listening.url() ) diff --git a/dev_tools/pylibs/edgacknowledg/forge/processor.py b/dev_tools/pylibs/edgacknowledg/forge/processor.py index 93bbc276e8..23e285305b 100644 --- a/dev_tools/pylibs/edgacknowledg/forge/processor.py +++ b/dev_tools/pylibs/edgacknowledg/forge/processor.py @@ -20,12 +20,12 @@ from dataclasses import dataclass from datetime import datetime from pathlib import Path, PurePosixPath -from typing import Awaitable, Callable, Optional +from typing import Any, Awaitable, Callable, Iterable, Optional import edgtest import edgutil -from edgacknowledg import rich_link, rich_text, section_slug +from edgacknowledg import section_slug from edgacknowledg.forge import ( ArtifactInfo, CIConclusion, @@ -42,10 +42,9 @@ KIND_TEST_CHANGES, KIND_TEST_FAILURES, Publisher, - PublisherUnavailableError, - ReviewIdentity, ) from edgacknowledg.forge.sandbox import Sandbox +from edgacknowledg.forge.sandbox_client import ServiceUnavailableError from edgacknowledg.forge.workspace import ( BENCH_BASELINE_PATTERNS, WorkspaceError, @@ -71,6 +70,11 @@ ) CI_METADATA_FILE = 'ci-meta.json' +# The largest that file may be, and the most (in bytes) the changes.elist +# files of a failures artifact may hold: their test ids are kept in its +# section's details, which go to the publisher in one message. +MAX_CI_METADATA_BYTES = 64 * 1024 +MAX_TEST_IDS_BYTES = 4 * 1024 * 1024 # Retries for a failing pull request: 1 min, 2 min, ... up to 1 h; give up # after this many attempts until the pull request changes again. @@ -110,9 +114,39 @@ def _reusable(section: Optional[SectionRecord], return section -def _changes_test_ids(changes_elist: Path) -> list[str]: - with open(changes_elist, encoding = 'utf-8', errors = 'replace') as handle: - return [line.strip() for line in handle if line.strip()] +def _changes_test_ids(changes_elists: Iterable[Path]) -> list[str]: + '''The test ids ``changes_elists`` name, within ``MAX_TEST_IDS_BYTES`` + (together); ``WorkspaceError`` beyond. + ''' + test_ids = [] + remaining = MAX_TEST_IDS_BYTES + for changes_elist in changes_elists: + with open(changes_elist, 'rb') as handle: + content = handle.read(remaining + 1) + remaining -= len(content) + if remaining < 0: + raise WorkspaceError( + f"CI's changes.elist files are over {MAX_TEST_IDS_BYTES} bytes" + ) + text = content.decode('utf-8', errors = 'replace') + test_ids += [line.strip() for line in text.splitlines() if line.strip()] + return test_ids + + +def _read_ci_metadata(path: Path) -> dict[str, Any]: + '''The CI metadata ``path`` holds (an object), if it's there.''' + metadata: dict[str, Any] = {} + if path.is_file(): + with open(path, 'rb') as handle: + content = handle.read(MAX_CI_METADATA_BYTES + 1) + if len(content) > MAX_CI_METADATA_BYTES: + raise WorkspaceError( + f"{CI_METADATA_FILE} is over {MAX_CI_METADATA_BYTES} bytes" + ) + value = json.loads(content) + if isinstance(value, dict): + metadata = value + return metadata @dataclass @@ -156,12 +190,10 @@ def busy_pull_requests(self) -> frozenset[int]: # Loop async def run(self) -> None: - '''Restore what was being served, then process changes forever.''' - await self._publisher.wait_connected() + '''Clean up after a restart, then process changes forever.''' await self.restore() last_maintenance = self._clock() while True: - await self._publisher.wait_connected() self._wake.clear() await self.start_due() if self._after_sweep is not None: @@ -198,28 +230,35 @@ async def _process_guarded(self, pull_request_id: int) -> None: return generation = record.generation label = f"{record.info.repository}#{record.info.number}" + if await self._store.begin_attempt( + pull_request_id, + generation, + now = self._clock(), + max_attempts = MAX_ATTEMPTS, + base_delay = RETRY_BASE_DELAY, + max_delay = RETRY_MAX_DELAY): + _log.error('giving up on %s until it changes again (last error: ' + '%s)', label, record.last_error) + return try: await self.process(record) - except PublisherUnavailableError as ex: + except ServiceUnavailableError as ex: # Not the pull request's fault; it stays dirty and is retried # once the connection is back. _log.warning('deferring %s: %s', label, ex) + await self._store.refund_attempt(record) return except asyncio.CancelledError: + # The service is stopping: not the pull request's fault either. + await self._store.refund_attempt(record) raise except Exception as ex: _log.exception('processing %s failed', label) - gave_up = await self._store.mark_failed( + await self._store.mark_failed( pull_request_id, generation, - f"{type(ex).__name__}: {ex}", - now = self._clock(), - max_attempts = MAX_ATTEMPTS, - base_delay = RETRY_BASE_DELAY, - max_delay = RETRY_MAX_DELAY + f"{type(ex).__name__}: {ex}" ) - if gave_up: - _log.error('giving up on %s until it changes again', label) return await self._store.mark_processed(pull_request_id, generation) # It may have changed while it was processed. @@ -228,26 +267,13 @@ async def _process_guarded(self, pull_request_id: int) -> None: self._in_flight.pop(pull_request_id, None) async def restore(self) -> None: - '''Re-serve what was served before a restart, without rebuilding. - - Pull requests whose files are gone are marked to be rebuilt instead; - since section fingerprints are content-derived, rebuilding yields the - same state (and so no comment). + '''Clean up after a restart: drop builds it interrupted. (Serving again + what was served is the publisher client's, on connecting.) ''' - # Builds interrupted by the restart. staging_root = self._workspaces.build_staging_dir if staging_root.is_dir(): for leftover in staging_root.iterdir(): remove_tree(leftover) - for record in await self._store.open_pull_requests(self._forge.name): - sections = await self._store.sections(record.id) - if len(sections) == 0: - continue - if all(section.root.is_dir() for section in sections): - await self._publisher.sync(self._identity(record), sections) - else: - await self._store.mark_dirty(record.id) - _log.info('restored previously served reviews') async def maintain(self) -> None: '''Periodic git housekeeping for every repository.''' @@ -265,18 +291,6 @@ async def maintain(self) -> None: # Processing - def _identity(self, record: PullRequestRecord) -> ReviewIdentity: - info = record.info - return ReviewIdentity( - review_id = record.review_id, - # The pull request reference links to it; the title is only text. - name = [ - rich_link(info.url, f"{info.repository}#{info.number}"), - rich_text(f" {info.title}"), - ], - timestamp = record.head_changed_at.isoformat() - ) - async def process(self, record: PullRequestRecord) -> None: info = record.info if (info.state != PullRequestState.OPEN or @@ -301,19 +315,22 @@ async def process(self, record: PullRequestRecord) -> None: desired.append(changes) desired += await self._ci_sections(record, head_sha, current) - await self._publisher.sync(self._identity(record), desired) + # The database leads: once it stores the new sections, nothing needs + # the old trees; the publisher catches up with what is stored. await self._store.replace_sections(record.id, desired) - kept_run_dirs = {section.details.get('run_dir') for section in desired} for section in current.values(): run_dir = section.details.get('run_dir') if run_dir is not None and run_dir not in kept_run_dirs: remove_tree(Path(run_dir)) + await self._publisher.sync_one(record.review_id) async def _tear_down(self, record: PullRequestRecord) -> None: '''Stop serving a pull request and free its disk space; no comment.''' info = record.info - await self._publisher.remove(record.review_id) + # Not served from here on, though still stored until its files are + # completely removed. + await self._publisher.delist(record.review_id) await self._workspaces.remove_review(info, record.review_id) await self._store.replace_sections(record.id, []) await self._workspaces.collect_garbage( @@ -476,12 +493,10 @@ async def _stage_artifact(self, zip_path.unlink(missing_ok = True) tested_sha = None - metadata_path = staging / CI_METADATA_FILE - if metadata_path.is_file(): - metadata = json.loads(metadata_path.read_text(encoding = 'utf-8')) - value = metadata.get('tested_sha') - if isinstance(value, str) and len(value) in (40, 64): - tested_sha = value + metadata = _read_ci_metadata(staging / CI_METADATA_FILE) + value = metadata.get('tested_sha') + if isinstance(value, str) and len(value) in (40, 64): + tested_sha = value return _StagedArtifact(staging, fingerprint, tested_sha) except BaseException: remove_tree(staging) @@ -589,8 +604,7 @@ async def _build_ci_sections(self, changes_files = {} if staged_failures is not None: changes_files = edgtest.find_changes_files(staged_failures.directory) - for changes_file in changes_files.values(): - failed_tests += _changes_test_ids(changes_file) + failed_tests += _changes_test_ids(changes_files.values()) if kept_failures is not None: failed_tests += kept_failures.details.get('tests', []) diff --git a/dev_tools/pylibs/edgacknowledg/forge/publisher/__init__.py b/dev_tools/pylibs/edgacknowledg/forge/publisher/__init__.py index 124cd4d763..972ef4cafd 100644 --- a/dev_tools/pylibs/edgacknowledg/forge/publisher/__init__.py +++ b/dev_tools/pylibs/edgacknowledg/forge/publisher/__init__.py @@ -7,10 +7,9 @@ - ``Publisher`` is what the service publishes through (and tests fake). - The publisher container runs the publisher server (``server``), which - serves the service's requests with an ``AcknowledgPublisher`` - (``acknowledg``): one AcknowlEDG connection, and the handlers that answer - reviewers by reading pull request content. Its commands go to the tools - runner. + queues the service's requests for ``acknowledg.publish``: one AcknowlEDG + batch session, and the handlers that answer reviewers by reading pull + request content. Its commands go to the tools runner. - The service reaches it with the publisher client (``client``), which implements ``Publisher``; ``protocol`` is the messages between them. ''' @@ -18,11 +17,16 @@ import uuid from dataclasses import dataclass -from typing import Optional, Protocol, Sequence - -from edgacknowledg import RichName +from typing import Optional, Protocol + +from edgacknowledg import ( + MAX_RICH_NAME_LENGTH, + RichName, + rich_link, + rich_text, +) from edgacknowledg.client import form_section_http_url -from edgacknowledg.forge.db import SectionRecord +from edgacknowledg.forge.db import PullRequestRecord, SectionRecord # Section kinds and the AcknowlEDG subscription type serving each. KIND_TEST_CHANGES = 'test.changes' @@ -36,8 +40,8 @@ } -class PublisherUnavailableError(Exception): - '''Raised when AcknowlEDG can't be reached right now (retry later).''' +# Ends a pull request title cut short to fit in its review's name. +TITLE_ELLIPSIS = '\u2026' @dataclass(frozen = True) @@ -47,19 +51,48 @@ class ReviewIdentity: timestamp: str -class Publisher(Protocol): - '''Where review sections are served (AcknowlEDG, or a test double).''' +def review_identity(record: PullRequestRecord) -> ReviewIdentity: + '''The review of a pull request, as published.''' + info = record.info + reference = f"{info.repository}#{info.number}" + title = f" {info.title}" + # A forge's titles can be longer than a review's name may be: cut the + # title (never the link) to fit. + room = MAX_RICH_NAME_LENGTH - len(info.url) - len(reference) + if len(title) > room: + kept = max(room - len(TITLE_ELLIPSIS), 0) + title = title[:kept].rstrip() + TITLE_ELLIPSIS + return ReviewIdentity( + review_id = record.review_id, + # The pull request reference links to it; the title is only text. + name = [ + rich_link(info.url, reference), + rich_text(title), + ], + timestamp = record.head_changed_at.isoformat() + ) - async def wait_connected(self) -> None: - ... - async def sync(self, - review: ReviewIdentity, - sections: Sequence[SectionRecord]) -> None: - '''Serve exactly ``sections`` for ``review``.''' +class Publisher(Protocol): + '''Where review sections are served (AcknowlEDG, or a test double): + what is stored in the service's database. + ''' + + async def sync_one(self, review_id: uuid.UUID) -> None: + '''Serve what is now stored in the database for the review (none: stop + serving it); once this returns, the publisher has queued it, and stops + serving what was stored in the database before moments later. While + the publisher can't take it, it returns all the same: the publisher + serves what is stored on connecting. + ''' ... - async def remove(self, review_id: uuid.UUID) -> None: + async def delist(self, review_id: uuid.UUID) -> None: + '''Stop serving the review, though its sections are still stored in + the database: their files are about to go (and, after them, what is + stored in the database). Raises ``ServiceUnavailableError`` unless + the publisher has queued it. + ''' ... def section_url(self, diff --git a/dev_tools/pylibs/edgacknowledg/forge/publisher/acknowledg.py b/dev_tools/pylibs/edgacknowledg/forge/publisher/acknowledg.py index 77d768f242..f11f4f987e 100644 --- a/dev_tools/pylibs/edgacknowledg/forge/publisher/acknowledg.py +++ b/dev_tools/pylibs/edgacknowledg/forge/publisher/acknowledg.py @@ -3,10 +3,12 @@ # See https://edgcpp.org/LICENSE.txt for license information. # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception -'''``AcknowledgPublisher``: serves review sections over one AcknowlEDG batch -connection. The publisher server runs one per connection; the requests of -reviewers go to its process pool (``pool``), which answers them with the -stock handlers. +'''Publishing review sections to AcknowlEDG (``publish``), over one batch +session, from a queue of what to publish, and what to stop publishing +(``Wanted``), applied in order; the requests of reviewers go to the +process pool (``pool``), which answers them with the stock handlers. The +client library keeps the session connected, and serves what was published +while not connected once it is. ''' import asyncio @@ -15,35 +17,99 @@ from dataclasses import dataclass from pathlib import Path -from typing import Optional, Sequence +from typing import AbstractSet, Optional, Protocol, Sequence, TypeAlias from edgacknowledg.client import ( DEFAULT_OPEN_TIMEOUT_S, + BatchClient, BatchClientSession, - ClientError, - NotConnectedError, ReviewClient, ) from edgacknowledg.forge.db import SectionRecord -from edgacknowledg.forge.publisher import ( - PublisherUnavailableError, - ReviewIdentity, - section_url, -) +from edgacknowledg.forge.publisher import ReviewIdentity from edgacknowledg.forge.publisher.pool import ProcessPool, UnixSocketHandler -from edgacknowledg.forge.workspace import require_real_directory_under +from edgacknowledg.forge.workspace import ( + WorkspaceError, + require_real_directory_under, +) _log = logging.getLogger(__name__) +class WantedItem(Protocol): + '''Something wanted of what is published (``Sync``, ``Delist``, + ``Prune``). + ''' + + async def apply_to(self, published: '_Published') -> None: + '''Make ``published`` what is wanted.''' + ... + + +@dataclass(frozen = True) +class Sync: + '''Serve exactly ``sections`` for ``review`` (none: stop serving it).''' + review: ReviewIdentity + sections: list[SectionRecord] + + async def apply_to(self, published: '_Published') -> None: + await published.sync(self.review, self.sections) + + +@dataclass(frozen = True) +class Delist: + '''Stop serving a review.''' + review_id: uuid.UUID + + async def apply_to(self, published: '_Published') -> None: + await published.remove(self.review_id) + + +@dataclass(frozen = True) +class Prune: + '''Stop serving every review but those in ``keep``.''' + keep: frozenset[uuid.UUID] + + async def apply_to(self, published: '_Published') -> None: + await published.prune(self.keep) + + +# What to publish, and to stop publishing, in order, for ``publish`` to +# apply. +Wanted: TypeAlias = 'asyncio.Queue[WantedItem]' + + +def _check_directories(section: SectionRecord, reviews_dir: Path) -> None: + '''Require the section's directories to be real ones (no symlink + anywhere below the reviews directory), the root and baselines inside the + home; the handlers rely on that. + ''' + directories = [section.home, section.root] + if 'baseline_dir' in section.details: + directories.append(Path(section.details['baseline_dir'])) + for directory in directories: + require_real_directory_under(reviews_dir, directory) + for directory in directories[1:]: + require_real_directory_under(section.home, directory) + + +def _build_handler(section: SectionRecord, + pool: ProcessPool, + reviews_dir: Optional[Path]) -> UnixSocketHandler: + if reviews_dir is not None: + _check_directories(section, reviews_dir) + return UnixSocketHandler(section, pool) + + @dataclass class _ServedSection: record: SectionRecord handler: UnixSocketHandler -class AcknowledgPublisher: - '''Serves sections, read-only, answering reviewers in ``pool``. +class _Published: + '''What ``session`` publishes, read-only, answering reviewers in + ``pool``. The handlers there treat the trees as untrusted: they follow no symlinks, and with ``reviews_dir`` every section's directories must be @@ -51,96 +117,21 @@ class AcknowledgPublisher: ''' def __init__(self, - *, - api_host: str, - api_port: int, - api_key: Optional[str], - ui_host: str, - ui_port: int, + session: BatchClientSession, pool: ProcessPool, - reviews_dir: Optional[Path] = None) -> None: - self._session = BatchClientSession( - status_listener = self, - api_host = api_host, - api_port = api_port, - api_key = api_key, - open_timeout = DEFAULT_OPEN_TIMEOUT_S - ) - self._ui_host = ui_host - self._ui_port = ui_port + reviews_dir: Optional[Path]) -> None: + self._session = session self._pool = pool self._reviews_dir = reviews_dir - self._connected = asyncio.Event() - self._lock = asyncio.Lock() self._reviews: dict[uuid.UUID, ReviewClient] = {} self._served: dict[uuid.UUID, dict[str, _ServedSection]] = {} - # ConnectionStatusListener - def connected(self) -> None: - _log.info('connected to AcknowlEDG') - self._connected.set() - - def attempting_reconnect(self) -> None: - pass - - def disconnected(self) -> None: - _log.warning('lost the AcknowlEDG connection; reconnecting') - self._connected.clear() - - async def run(self) -> None: - '''Keep the batch connection up until cancelled.''' - try: - await self._session.start() - await self._session.stopped() - finally: - await self._session.stop() - - async def wait_connected(self) -> None: - await self._connected.wait() - - def _require_connected(self) -> None: - if not self._connected.is_set(): - raise PublisherUnavailableError('not connected to AcknowlEDG') - - def section_url(self, - review_id: uuid.UUID, - section: SectionRecord) -> Optional[str]: - return section_url(self._ui_host, self._ui_port, review_id, section) - - def _check_directories(self, section: SectionRecord) -> None: - '''Require the section's directories to be real ones (no symlink - anywhere below the reviews directory), the root and baselines inside - the home; the handlers rely on that. - ''' - if self._reviews_dir is None: - return - directories = [section.home, section.root] - if 'baseline_dir' in section.details: - directories.append(Path(section.details['baseline_dir'])) - for directory in directories: - require_real_directory_under(self._reviews_dir, directory) - for directory in directories[1:]: - require_real_directory_under(section.home, directory) - - def _build_handler(self, section: SectionRecord) -> UnixSocketHandler: - self._check_directories(section) - return UnixSocketHandler(section, self._pool) - async def sync(self, review: ReviewIdentity, sections: Sequence[SectionRecord]) -> None: - async with self._lock: - self._require_connected() - try: - await self._sync(review, sections) - except (NotConnectedError, ClientError) as ex: - raise PublisherUnavailableError(str(ex)) from ex - - async def _sync(self, - review: ReviewIdentity, - sections: Sequence[SectionRecord]) -> None: + '''Serve exactly ``sections`` for ``review`` (none: stop serving it).''' if len(sections) == 0: - await self._remove(review.review_id) + await self.remove(review.review_id) return client_review = self._reviews.get(review.review_id) @@ -175,16 +166,69 @@ async def _sync(self, for slug, section in desired.items(): if slug in served: continue - handler = await asyncio.to_thread(self._build_handler, section) + try: + handler = await asyncio.to_thread( + _build_handler, + section, + self._pool, + self._reviews_dir + ) + except WorkspaceError as ex: + # The service only sends real directories; this guards the + # handlers should it ever not. + _log.error('not serving section %s of review %s: %s', slug, + review.review_id, ex) + continue await client_review.add_section(handler) served[slug] = _ServedSection(section, handler) - async def remove(self, review_id: uuid.UUID) -> None: - async with self._lock: - await self._remove(review_id) + async def prune(self, keep: AbstractSet[uuid.UUID]) -> None: + '''Stop serving every review but those in ``keep``.''' + for review_id in list(self._reviews): + if review_id not in keep: + await self.remove(review_id) - async def _remove(self, review_id: uuid.UUID) -> None: + async def remove(self, review_id: uuid.UUID) -> None: + '''Stop serving a review.''' client_review = self._reviews.pop(review_id, None) self._served.pop(review_id, None) if client_review is not None: await self._session.remove_review(client_review) + + +class _Logging: + '''Logs the batch session's connection.''' + + def connected(self) -> None: + _log.info('connected to AcknowlEDG') + + def attempting_reconnect(self) -> None: + pass + + def disconnected(self) -> None: + _log.warning('lost the AcknowlEDG connection; reconnecting') + + +async def publish(wanted: Wanted, + *, + api_host: str, + api_port: int, + api_key: Optional[str], + pool: ProcessPool, + reviews_dir: Optional[Path] = None) -> None: + '''Publish what ``wanted`` asks for, until cancelled.''' + async with BatchClient( + api_host = api_host, + api_port = api_port, + api_key = api_key, + open_timeout = DEFAULT_OPEN_TIMEOUT_S, + status_listener = _Logging()) as session: + published = _Published(session, pool, reviews_dir) + while True: + item = await wanted.get() + try: + await item.apply_to(published) + except Exception: + # One review mustn't stop the rest. + _log.exception('publishing failed; it is published again once it ' + 'changes') diff --git a/dev_tools/pylibs/edgacknowledg/forge/publisher/client.py b/dev_tools/pylibs/edgacknowledg/forge/publisher/client.py index 3d3950d7b5..04939e92a5 100644 --- a/dev_tools/pylibs/edgacknowledg/forge/publisher/client.py +++ b/dev_tools/pylibs/edgacknowledg/forge/publisher/client.py @@ -5,26 +5,27 @@ '''The publisher client (``PublisherClient``): how the service publishes, through the publisher server in the publisher container (see -``publisher.server``). +``publisher.server``), over one long-lived connection (kept by a +``ForgeSandboxClient``). ''' import asyncio import json import logging -import time import uuid from pathlib import Path -from typing import Any, Optional, Sequence +from types import TracebackType +from typing import Any, Optional, Sequence, Type -from websockets.asyncio.client import ClientConnection, connect -from websockets.exceptions import ConnectionClosed, InvalidHandshake +from websockets.asyncio.client import ClientConnection +from websockets.exceptions import ConnectionClosed from edgacknowledg.client import DEFAULT_OPEN_TIMEOUT_S -from edgacknowledg.forge.db import SectionRecord +from edgacknowledg.forge.db import ForgeStore, SectionRecord from edgacknowledg.forge.publisher import ( - PublisherUnavailableError, ReviewIdentity, + review_identity, section_url, ) from edgacknowledg.forge.publisher.protocol import ( @@ -32,49 +33,26 @@ encode_review, encode_section, ) +from edgacknowledg.forge.sandbox_client import ( + ForgeSandboxClient, + ServiceUnavailableError, +) _log = logging.getLogger(__name__) -class _Connection: - '''The client's end of a connection to the publisher server, which keeps - one publisher for it, for as long as it lasts. - ''' - - def __init__(self, connection: ClientConnection) -> None: - self._connection = connection - - async def send(self, message: dict[str, Any]) -> None: - try: - await self._connection.send(json.dumps(message)) - except ConnectionClosed as ex: - raise PublisherUnavailableError( - f"the publisher server went away: {ex}" - ) from ex - - async def receive(self) -> Optional[bytes]: - try: - message = await self._connection.recv() - except ConnectionClosed: - return None - return message.encode() if isinstance(message, str) else message - - def abort(self) -> None: - # Dropping the connection makes the server drop its publisher. - self._connection.transport.abort() - - async def close(self) -> None: - await self._connection.close() - - class PublisherClient: - '''Publishes through the publisher server at ``url``. - - Each connection gets a publisher of its own on the server, which sees - the review trees, read-only, and has the network (for AcknowlEDG), but - not the service's home, database, credentials, or clones; its reviewer - commands run in the tools container. Should the connection go, the - client connects again and serves every review's sections again. + '''Publishes through the publisher server at ``url``, while it runs + (``async with``, or ``start`` and ``close``). + + The server's publisher sees the review trees, read-only, and has the + network (for AcknowlEDG), but not the service's home, database, + credentials, or clones; its reviewer commands run in the tools + container. It serves what is stored in the service's database + (``store``) for ``forge_name``: when told a review's sections stored in + the database changed (``sync_one``), and on every connection, every + review stored in the database (``sync_all``). So telling it is only a + shortcut: what it can't take then, it serves on connecting. ''' def __init__(self, @@ -86,57 +64,87 @@ def __init__(self, ui_host: str, ui_port: int, reviews_dir: Path, - request_timeout: float = 15 * 60.0, - restart_delay: float = 1.0, - max_restart_delay: float = 60.0) -> None: - self._url = url + store: ForgeStore, + forge_name: str, + request_timeout: float = 15 * 60.0) -> None: + self._link = ForgeSandboxClient( + url, + peer = 'the publisher server', + serve = self._serve, + open_timeout = DEFAULT_OPEN_TIMEOUT_S, + max_size = MESSAGE_LIMIT + ) self._api_host = api_host self._api_port = api_port self._api_key = api_key self._ui_host = ui_host self._ui_port = ui_port self._reviews_dir = reviews_dir + self._store = store + self._forge_name = forge_name self._request_timeout = request_timeout - self._restart_delay = restart_delay - self._max_restart_delay = max_restart_delay - - # The current connection, if any. - self._connection: Optional[_Connection] = None - self._connected = asyncio.Event() - # Serializes requests (so at most one awaits a reply), and replaying - # after reconnecting. + # Serializes requests, for two reasons. Answers carry no request ID, so + # at most one request may await (read) one. And sync_one and delist + # must wait for sync_all to finish (even one a reconnect started), or + # they could interleave with it: it would resend stale sections, or + # prune a review sync_one just served. self._lock = asyncio.Lock() - self._reply: Optional[asyncio.Future[dict[str, Any]]] = None - # What each review should serve, to replay after reconnecting. - self._desired: dict[uuid.UUID, - tuple[ReviewIdentity, list[SectionRecord]]] = {} - self._needs_replay = False - self._replay_task: Optional[asyncio.Task[None]] = None - # Publisher + def start(self) -> None: + '''Start keeping the connection; each one serves every review stored + in the database before any request (``sync_all``). + ''' + self._reviews_dir.mkdir(parents = True, exist_ok = True) + self._link.start() + + async def close(self) -> None: + '''Close the connection, and stop keeping it.''' + await self._link.close() + + async def __aenter__(self) -> 'PublisherClient': + self.start() + return self + + async def __aexit__(self, + exc_type: Optional[Type[BaseException]], + exc: Optional[BaseException], + tb: Optional[TracebackType]) -> None: + await self.close() async def wait_connected(self) -> None: - await self._connected.wait() + '''Wait until connected (requests then wait for every review stored in + the database to be served). + ''' + await self._link.wait_connected() + + # Publisher def section_url(self, review_id: uuid.UUID, section: SectionRecord) -> Optional[str]: return section_url(self._ui_host, self._ui_port, review_id, section) - async def sync(self, - review: ReviewIdentity, - sections: Sequence[SectionRecord]) -> None: + async def sync_one(self, review_id: uuid.UUID) -> None: async with self._lock: - if not self._connected.is_set(): - raise PublisherUnavailableError('not connected to AcknowlEDG') - await self._sync(review, sections) + if not self._link.connected: + _log.info('not connected; serving %s on connecting', review_id) + return + record = await self._store.find_pull_request_by_review(review_id) + if record is None: + raise LookupError(f"no pull request has review {review_id}") + try: + await self._sync(review_identity(record), + await self._store.sections(record.id)) + except ServiceUnavailableError as ex: + _log.warning('%s; serving %s on connecting', ex, review_id) - async def remove(self, review_id: uuid.UUID) -> None: + async def delist(self, review_id: uuid.UUID) -> None: async with self._lock: - # A new connection's publisher serves nothing for it either way. - self._desired.pop(review_id, None) - if self._connection is not None: - await self._request({'op': 'remove', 'review_id': str(review_id)}) + if not self._link.connected: + raise ServiceUnavailableError( + 'not connected to the publisher server' + ) + await self._request({'op': 'delist', 'review_id': str(review_id)}) async def _sync(self, review: ReviewIdentity, @@ -146,10 +154,6 @@ async def _sync(self, 'review': encode_review(review), 'sections': [encode_section(section) for section in sections], }) - if len(sections) == 0: - self._desired.pop(review.review_id, None) - else: - self._desired[review.review_id] = (review, list(sections)) # Requests @@ -157,81 +161,103 @@ async def _request(self, request: dict[str, Any]) -> None: '''Send ``request`` to the server and wait for its answer; the caller holds the lock. ''' - connection = self._connection + connection = self._link.connection if connection is None: - raise PublisherUnavailableError( + raise ServiceUnavailableError( 'not connected to the publisher server' ) - reply: asyncio.Future[dict[str, Any]] = ( - asyncio.get_running_loop().create_future() - ) - self._reply = reply + await self._send(connection, request) try: - await connection.send(request) - try: - answer = await asyncio.wait_for(asyncio.shield(reply), - self._request_timeout) - except asyncio.TimeoutError: - _log.error('the publisher server did not answer within %s s; ' - 'reconnecting', self._request_timeout) - # Send it nothing more: a late answer must not be taken for the - # next request's. - self._connection = None - connection.abort() - raise PublisherUnavailableError( - 'the publisher server stopped answering' - ) from None - finally: - self._reply = None + message = await asyncio.wait_for(self._receive(connection), + self._request_timeout) + except asyncio.TimeoutError: + _log.error('the publisher server did not answer within %s s; ' + 'reconnecting', self._request_timeout) + # A late answer must not be taken for the next request's. + self._link.drop(connection) + raise ServiceUnavailableError( + 'the publisher server stopped answering' + ) from None + if message is None: + raise ServiceUnavailableError('the publisher connection closed') - error = answer.get('error') - if error == 'unavailable': - raise PublisherUnavailableError(str(answer.get('message'))) - if error is not None: + try: + answer = json.loads(message) + except ValueError: + answer = None + if not isinstance(answer, dict): + _log.error('bad answer from the publisher server: %r; reconnecting', + message[:200]) + # Out of step with the server: no later answer can be trusted. + self._link.drop(connection) + raise ServiceUnavailableError( + 'bad answer from the publisher server' + ) + if answer.get('error') is not None: raise RuntimeError(f"publisher server: {answer.get('message')}") - def _handle_message(self, line: bytes) -> None: + @staticmethod + async def _send(connection: ClientConnection, + message: dict[str, Any]) -> None: try: - value = json.loads(line) - except ValueError: - value = None - if not isinstance(value, dict): - _log.error('bad message from the publisher server: %r', line[:200]) - return - event = value.get('event') - if event == 'connected': - if self._replay_task is None or self._replay_task.done(): - self._replay_task = asyncio.create_task(self._on_connected()) - elif event == 'disconnected': - self._connected.clear() - elif self._reply is not None and not self._reply.done(): - self._reply.set_result(value) - else: - _log.error('unexpected answer from the publisher server: %r', - line[:200]) + await connection.send(json.dumps(message)) + except ConnectionClosed as ex: + raise ServiceUnavailableError( + f"the publisher server went away: {ex}" + ) from ex - async def _on_connected(self) -> None: - '''Once the publisher is connected to AcknowlEDG, serve again what the - last connection's served; only then accept new requests. + @staticmethod + async def _receive(connection: ClientConnection) -> Optional[bytes]: + try: + message = await connection.recv() + except ConnectionClosed: + return None + return message.encode() if isinstance(message, str) else message + + async def sync_all(self, connection: ClientConnection) -> None: + '''On ``connection``, serve every open pull request's sections stored in + the database (the publisher may have restarted, losing what it served), + and have it stop serving anything else (what it serves outlasts a + connection, so it may still serve what an earlier service did); only + then accept new requests. Those it can't serve (their files are gone, + or the server rejected them) it leaves out of what it keeps serving, + and marks to be rebuilt; since section fingerprints are content-derived, + rebuilding yields the same state (and so no comment). Should anything + else fail, the connection goes, to try again on the next. ''' async with self._lock: - if self._needs_replay: - for review, sections in list(self._desired.values()): - try: - await self._sync(review, sections) - except PublisherUnavailableError as ex: - _log.warning('serving the reviews again failed (%s); will ' - 'retry on the next connection', ex) - return - except Exception: - # Don't let one review hold up every other one; it is served - # again once its pull request changes. - _log.exception('serving review %s again failed; dropping it', - review.review_id) - self._desired.pop(review.review_id, None) - self._needs_replay = False - _log.info('connected to AcknowlEDG') - self._connected.set() + await self._sync_all(connection) + + async def _sync_all(self, connection: ClientConnection) -> None: + '''``sync_all``, for a caller holding the lock.''' + try: + served: list[str] = [] + for record in await self._store.open_pull_requests(self._forge_name): + sections = await self._store.sections(record.id) + if len(sections) == 0: + continue + try: + if not all(section.root.is_dir() for section in sections): + raise FileNotFoundError('its files are gone') + await self._sync(review_identity(record), sections) + except ServiceUnavailableError: + raise + except Exception as ex: + _log.warning('rebuilding %s#%s: %s', record.info.repository, + record.info.number, ex) + await self._store.mark_dirty(record.id) + continue + served.append(str(record.review_id)) + await self._request({'op': 'prune', 'keep': served}) + except ServiceUnavailableError as ex: + _log.warning('serving the reviews failed (%s); will retry on the ' + 'next connection', ex) + return + except Exception: + _log.exception('serving the reviews failed; reconnecting') + self._link.drop(connection) + return + _log.info('connected to the publisher server') # Connection @@ -240,63 +266,20 @@ def _init_message(self) -> dict[str, Any]: 'api_host': self._api_host, 'api_port': self._api_port, 'api_key': self._api_key, - 'ui_host': self._ui_host, - 'ui_port': self._ui_port, 'reviews_dir': str(self._reviews_dir), } - async def run(self) -> None: - '''Stay connected to the publisher server, reconnecting whenever the - connection goes, until cancelled. + async def _serve(self, connection: ClientConnection) -> None: + '''Serve ``connection`` until it goes: initialize it, then serve every + review (``sync_all``), holding the lock throughout, so that requests + come after. Requests read their own answers (the server sends nothing + else); a request waiting on one when it goes fails. ''' - self._reviews_dir.mkdir(parents = True, exist_ok = True) - delay = self._restart_delay - while True: - started = time.monotonic() + async with self._lock: try: - await self._run_connection() - except asyncio.CancelledError: - raise - except (OSError, ConnectionClosed, InvalidHandshake, - asyncio.TimeoutError) as ex: - _log.warning("couldn't reach the publisher server: %r", ex) - except Exception: - _log.exception('the publisher connection failed') - if time.monotonic() - started > self._max_restart_delay: - delay = self._restart_delay - _log.warning('lost the publisher server; reconnecting in %.0f s', - delay) - await asyncio.sleep(delay) - delay = min(delay * 2, self._max_restart_delay) - - async def _open(self) -> _Connection: - connection = await connect( - self._url, - max_size = MESSAGE_LIMIT, - open_timeout = DEFAULT_OPEN_TIMEOUT_S - ) - return _Connection(connection) - - async def _run_connection(self) -> None: - connection = await self._open() - self._connection = connection - try: - await connection.send(self._init_message()) - while self._connection is connection: - message = await connection.receive() - if message is None: - break - self._handle_message(message) - finally: - if self._connection is connection: - self._connection = None - self._connected.clear() - self._needs_replay = True - if self._reply is not None and not self._reply.done(): - self._reply.set_exception( - PublisherUnavailableError('the publisher connection closed') - ) - connection.abort() - await connection.close() - if self._replay_task is not None: - self._replay_task.cancel() + await self._send(connection, self._init_message()) + except ServiceUnavailableError as ex: + _log.warning('the publisher server went away at once: %s', ex) + return + await self._sync_all(connection) + await connection.wait_closed() diff --git a/dev_tools/pylibs/edgacknowledg/forge/publisher/pool.py b/dev_tools/pylibs/edgacknowledg/forge/publisher/pool.py index 4e6c949b78..d0121d16da 100644 --- a/dev_tools/pylibs/edgacknowledg/forge/publisher/pool.py +++ b/dev_tools/pylibs/edgacknowledg/forge/publisher/pool.py @@ -11,25 +11,33 @@ So the server keeps only section management, and answers each request in a pool process (``pool_process``): -- ``AcknowledgPublisher`` registers a ``UnixSocketHandler`` per section, +- ``acknowledg.publish`` registers a ``UnixSocketHandler`` per section, which stands in for the section's handler: it passes each request, with the section it is for, to the pool, and relays the replies. - ``ProcessPool`` keeps a fixed number of pool processes, each with one end - of a socket pair as its channel, and sends each request to the next one - in turn (round-robin). A process answers one request at a time; requests - for a busy one wait their turn. -- A request has a deadline, counted from when its process starts on it. A - process that misses it is killed (with whatever it started) and replaced; - so is one that exits. Either way the request fails, and the client - library answers the reviewer with its failure reply. + of a socket pair as its channel. A process answers one request at a + time, so requests queue, in order, and each process has a dispatcher + task that takes the next one only once its process has answered the + last: no process is given a request while busy with another. +- A request has a deadline, counted from when its process is given it (so + never including another's work). A process that misses it is killed + (with whatever it started) and replaced; so is one that exits. Either + way the request fails, and the client library answers the reviewer with + its failure reply. +- A request abandoned (cancelled, say, as its section is removed) before + it is given to a process is skipped; one abandoned after is still + answered (its process stays busy with it until then), and the answer + dropped. Messages on a channel are JSON objects, one per line: a process first says -``{"ready": true}``; then the server sends ``{"section": ..., "request": -...}``, and the process answers ``{"replies": [...]}``, plus ``"error": -"illegal-access" | "failed"`` (and ``"message"``, ``"reviewer_id"``) if the -handler raised. A process that exits before it is ready has failed to -start, and is retried after a delay, doubling from 1 s up to 60 s; until -then, requests for it fail at once. +``{"ready": true}``; then the server sends ``{"id", "section": ..., +"request": ...}``, and the process answers ``{"id", "replies": [...]}``, +plus ``"error": "illegal-access" | "failed"`` (and ``"message"``, +``"reviewer_id"``) if the handler raised; an answer to another id means +the two lost track, and the process is replaced. A process that exits +before it is ready has failed to start, and is retried after a delay, +doubling from 1 s up to 60 s; meanwhile, the other processes take the +requests, and while none can, requests fail at once. ''' import asyncio @@ -47,11 +55,18 @@ from typing import Any, Optional from edgacknowledg import ProtocolMessage -from edgacknowledg.client.shared import Channel, IllegalFileAccessError +from edgacknowledg.client.shared import ( + Channel, + IllegalFileAccessError, + reviewer_of, +) from edgacknowledg.forge.config import ENV_PREFIX from edgacknowledg.forge.db import SectionRecord from edgacknowledg.forge.publisher import SUBSCRIPTION_TYPES -from edgacknowledg.forge.publisher.protocol import MESSAGE_LIMIT, encode_section +from edgacknowledg.forge.publisher.protocol import ( + MESSAGE_LIMIT, + encode_section, +) _log = logging.getLogger(__name__) @@ -69,8 +84,10 @@ class PoolError(Exception): '''A request the pool couldn't answer: its process missed the deadline, - exited, or isn't running. + exited, or isn't running, or the handler failed. ``UnixSocketHandler`` + records the reviewer who made it, whom the failure reply is for. ''' + reviewer_id: Optional[str] = None def python_paths() -> list[Path]: @@ -117,21 +134,40 @@ class Answer: reviewer_id: Optional[str] = None +@dataclass +class _Job: + '''A request waiting for, or being answered by, a pool process.''' + id: int + section: SectionRecord + request: Any + answered: 'asyncio.Future[dict[str, Any]]' + + @dataclass class _Slot: '''One place in the pool, and the process filling it.''' index: int - # One request at a time; waiting requests queue here, in order. - lock: asyncio.Lock = field(default_factory = asyncio.Lock) process: Optional[asyncio.subprocess.Process] = None messages: Optional[asyncio.StreamReader] = None channel: Optional[asyncio.StreamWriter] = None + # The task giving its process requests, one at a time. + dispatcher: Optional['asyncio.Task[None]'] = None # Starts that failed in a row, and when the next may be tried. failed_starts: int = 0 not_before: float = 0.0 def running(self) -> bool: - return self.process is not None and self.process.returncode is None + '''Whether its process can take a request (``_kill`` closes the + channel at once, before the process is seen to exit). + ''' + return (self.process is not None and self.process.returncode is None and + self.channel is not None) + + def starting_later(self) -> bool: + '''Whether its process failed to start, and isn't to be tried again + yet. + ''' + return not self.running() and time.monotonic() < self.not_before class ProcessPool: @@ -150,17 +186,35 @@ def __init__(self, self._tools_service_url = tools_service_url self._request_timeout = request_timeout self._slots = [_Slot(index) for index in range(processes)] - self._next = 0 + # Requests not yet given to a process, in order. + self._jobs: asyncio.Queue[_Job] = asyncio.Queue() + self._next_id = 0 async def start(self) -> None: - '''Start every process; raise ``PoolError`` if one fails to.''' - for slot in self._slots: - await self._start(slot) + '''Start every process, and its dispatcher; raise ``PoolError`` if a + process fails to start (its dispatcher tries again later). + ''' + try: + for slot in self._slots: + await self._start(slot) + finally: + # Afterwards: a dispatcher starts its process itself, should it not + # be running. + for slot in self._slots: + slot.dispatcher = asyncio.create_task(self._dispatch(slot)) async def close(self) -> None: - '''Stop every process.''' + '''Stop every dispatcher and process; fail the requests still queued.''' for slot in self._slots: + if slot.dispatcher is not None: + slot.dispatcher.cancel() + await asyncio.gather(slot.dispatcher, return_exceptions = True) + slot.dispatcher = None await self._stop(slot) + while not self._jobs.empty(): + job = self._jobs.get_nowait() + if not job.answered.done(): + job.answered.set_exception(PoolError('the pool is closing')) def pids(self) -> list[int]: return [slot.process.pid for slot in self._slots @@ -169,56 +223,103 @@ def pids(self) -> list[int]: async def answer(self, section: SectionRecord, request: Any) -> Answer: - '''Have the next process (round-robin) answer ``request`` for - ``section``; raise ``PoolError`` if it can't. + '''Have the next free process answer ``request`` for ``section``; + raise ``PoolError`` if it can't. Cancelling this abandons the request. ''' - slot = self._slots[self._next] - self._next = (self._next + 1) % len(self._slots) + if all(slot.starting_later() for slot in self._slots): + raise PoolError('every pool process is restarting') if isinstance(request, bytes): request = request.decode() - async with slot.lock: + job = _Job( + id = self._next_id, + section = section, + request = request, + answered = asyncio.get_running_loop().create_future() + ) + self._next_id += 1 + self._jobs.put_nowait(job) + value = await job.answered + return Answer( + replies = list(value.get('replies', [])), + error = value.get('error'), + message = str(value.get('message', '')), + reviewer_id = value.get('reviewer_id') + ) + + async def _dispatch(self, slot: _Slot) -> None: + '''Give ``slot``'s process the queued requests, one at a time, each + once it has answered the last, until cancelled. + ''' + while True: + # A running process first (replaced at once if it went), so it takes + # requests only when it can answer them; while it can't start, the + # others take them. if not slot.running(): - await self._replace(slot) - assert slot.channel is not None and slot.messages is not None + try: + await self._replace(slot) + except PoolError: + await asyncio.sleep(max(slot.not_before - time.monotonic(), 0.1)) + continue + job = await self._jobs.get() + if job.answered.done(): + # Abandoned before its turn. + continue try: - slot.channel.write( - (json.dumps({ - 'section': encode_section(section), - 'request': request, - }) + '\n').encode() - ) - await slot.channel.drain() - line = await asyncio.wait_for(slot.messages.readline(), - self._request_timeout) - except asyncio.TimeoutError: - _log.error('pool process %d missed the %g s deadline; replacing it', - slot.index, self._request_timeout) - await self._stop(slot) - # Not a failure to start: replace it at once. - slot.process = None - raise PoolError('the request timed out') from None - except (BrokenPipeError, ConnectionResetError, ValueError) as ex: + value = await self._ask(slot, job) + except PoolError as ex: + # Its process is replaced before the next request (stopping one + # that couldn't start does nothing). await self._stop(slot) - raise PoolError(f"pool process {slot.index} failed: {ex}") from ex - if line == b'': - returncode = await self._stop(slot) - _log.error('pool process %d exited (%s) while answering', - slot.index, returncode) - raise PoolError('the pool process exited') - value = json.loads(line) - return Answer( - replies = list(value.get('replies', [])), - error = value.get('error'), - message = str(value.get('message', '')), - reviewer_id = value.get('reviewer_id') + if not job.answered.done(): + job.answered.set_exception(ex) + continue + except BaseException: + if not job.answered.done(): + job.answered.set_exception(PoolError('the pool is closing')) + raise + # Unless abandoned since it was given. + if not job.answered.done(): + job.answered.set_result(value) + + async def _ask(self, slot: _Slot, job: _Job) -> dict[str, Any]: + '''Have ``slot``'s process, idle, answer ``job``; raise ``PoolError`` + if it can't (its process then at fault, unless it couldn't start). + ''' + if not slot.running(): + await self._replace(slot) + assert slot.channel is not None and slot.messages is not None + try: + slot.channel.write( + (json.dumps({ + 'id': job.id, + 'section': encode_section(job.section), + 'request': job.request, + }) + '\n').encode() ) + await slot.channel.drain() + line = await asyncio.wait_for(slot.messages.readline(), + self._request_timeout) + value = json.loads(line) if line != b'' else None + except asyncio.TimeoutError: + _log.error('pool process %d missed the %g s deadline', slot.index, + self._request_timeout) + raise PoolError('the request timed out') from None + except (BrokenPipeError, ConnectionResetError, ValueError) as ex: + # ValueError: an oversized or malformed answer. + raise PoolError(f"pool process {slot.index} failed: {ex}") from ex + if value is None: + _log.error('pool process %d exited while answering', slot.index) + raise PoolError('the pool process exited') + if not isinstance(value, dict) or value.get('id') != job.id: + raise PoolError(f"pool process {slot.index} answered another request") + return value async def _replace(self, slot: _Slot) -> None: '''Replace ``slot``'s process, which isn't running.''' if slot.process is not None: returncode = await self._stop(slot) slot.process = None - _log.warning('pool process %d exited (%s); replacing it', + _log.warning('pool process %d stopped (%s); replacing it', slot.index, returncode) if time.monotonic() < slot.not_before: raise PoolError(f"pool process {slot.index} is restarting") @@ -226,7 +327,25 @@ async def _replace(self, slot: _Slot) -> None: async def _start(self, slot: _Slot) -> None: '''Start ``slot``'s process and wait until it is ready; raise - ``PoolError`` (and delay the next try) if it isn't. + ``PoolError`` (and delay the next try) if it isn't, whatever the + reason (e.g. no process could be started at all). + ''' + try: + await self._launch(slot) + except Exception as ex: + returncode = await self._stop(slot) + slot.process = None + slot.failed_starts += 1 + delay = min(2.0 ** (slot.failed_starts - 1), _MAX_RESTART_DELAY_S) + slot.not_before = time.monotonic() + delay + _log.error('pool process %d failed to start (%s; exit status %s); ' + 'trying again in %g s', slot.index, ex, returncode, delay) + raise PoolError(f"pool process {slot.index} failed to start") from ex + slot.failed_starts = 0 + + async def _launch(self, slot: _Slot) -> None: + '''Start ``slot``'s process and wait until it is ready; raise if it + isn't. ''' parent_end, process_end = socket.socketpair() try: @@ -242,44 +361,49 @@ async def _start(self, slot: _Slot) -> None: raise finally: process_end.close() - slot.messages, slot.channel = await asyncio.open_unix_connection( - sock = parent_end, - limit = MESSAGE_LIMIT - ) + # Recorded at once, so it is stopped whatever happens next. slot.process = process try: + slot.messages, slot.channel = await asyncio.open_unix_connection( + sock = parent_end, + limit = MESSAGE_LIMIT + ) ready = await asyncio.wait_for(slot.messages.readline(), _READY_TIMEOUT_S) except asyncio.TimeoutError: ready = b'' + except BaseException: + # Abandoned before it was ready: its ready line mustn't be taken for + # a request's answer. + self._kill(slot) + raise if ready.strip() != b'{"ready": true}': - returncode = await self._stop(slot) - slot.process = None - slot.failed_starts += 1 - delay = min(2.0 ** (slot.failed_starts - 1), _MAX_RESTART_DELAY_S) - slot.not_before = time.monotonic() + delay - _log.error('pool process %d failed to start (%s); trying again in ' - '%g s', slot.index, returncode, delay) - raise PoolError(f"pool process {slot.index} failed to start") - slot.failed_starts = 0 + raise PoolError('it never said it was ready') _log.info('started pool process %d (pid %d)', slot.index, process.pid) - async def _stop(self, slot: _Slot) -> Optional[int]: - '''Kill ``slot``'s process and whatever it started; return how it - exited. + def _kill(self, slot: _Slot) -> None: + '''Kill ``slot``'s process and whatever it started, and close its + channel, without waiting for it to exit (``_stop`` waits). ''' - process = slot.process if slot.channel is not None: slot.channel.close() slot.channel = None slot.messages = None - if process is None: - return None - if process.returncode is None: + process = slot.process + if process is not None and process.returncode is None: try: os.killpg(process.pid, signal.SIGKILL) except (ProcessLookupError, PermissionError): pass + + async def _stop(self, slot: _Slot) -> Optional[int]: + '''Kill ``slot``'s process and whatever it started; return how it + exited. + ''' + self._kill(slot) + process = slot.process + if process is None: + return None return await process.wait() @@ -310,12 +434,23 @@ def slug(self) -> str: async def handle_request(self, channel: Channel) -> None: request = await channel.recv() - answer = await self._pool.answer(self._section, request) + reviewer_id = reviewer_of(request) + if reviewer_id is None: + # The server's (e.g. its error for a reply it refused), which the + # stock handlers don't answer: no need to bother a process. + return + try: + answer = await self._pool.answer(self._section, request) + except PoolError as ex: + ex.reviewer_id = reviewer_id + raise for reply in answer.replies: await channel.send(reply) if answer.error == 'illegal-access': - error = IllegalFileAccessError(answer.message) - error.reviewer_id = answer.reviewer_id - raise error + illegal = IllegalFileAccessError(answer.message) + illegal.reviewer_id = answer.reviewer_id + raise illegal if answer.error is not None: - raise PoolError(f"the request failed: {answer.message}") + failed = PoolError(f"the request failed: {answer.message}") + failed.reviewer_id = reviewer_id + raise failed diff --git a/dev_tools/pylibs/edgacknowledg/forge/publisher/pool_process.py b/dev_tools/pylibs/edgacknowledg/forge/publisher/pool_process.py index 939325d6f4..7d7d40670e 100644 --- a/dev_tools/pylibs/edgacknowledg/forge/publisher/pool_process.py +++ b/dev_tools/pylibs/edgacknowledg/forge/publisher/pool_process.py @@ -39,7 +39,10 @@ KIND_TEST_CHANGES, KIND_TEST_FAILURES, ) -from edgacknowledg.forge.publisher.protocol import MESSAGE_LIMIT, decode_section +from edgacknowledg.forge.publisher.protocol import ( + MESSAGE_LIMIT, + decode_section, +) from edgacknowledg.forge.runner.client import RunnerClient from edgacknowledg.forge.sandbox import Sandbox @@ -130,24 +133,27 @@ async def _run(channel_fd: int) -> None: sock = sock, limit = MESSAGE_LIMIT ) + bench_workers = BackgroundWorkerPool(_BENCH_WORKERS) # One connection to the tools runner, for every command this process # runs. - tools = RunnerClient(os.environ.get(f"{ENV_PREFIX}TOOLS_SERVICE_URL", '')) - sandbox = Sandbox(tools = tools) - bench_workers = BackgroundWorkerPool(_BENCH_WORKERS) - try: - channel.write(b'{"ready": true}\n') - await channel.drain() - while True: - line = await messages.readline() - if line == b'': - return - reply = await answer(json.loads(line), sandbox, bench_workers) - channel.write((json.dumps(reply) + '\n').encode()) + async with RunnerClient( + os.environ.get(f"{ENV_PREFIX}TOOLS_SERVICE_URL", '')) as tools: + sandbox = Sandbox(tools = tools) + try: + channel.write(b'{"ready": true}\n') await channel.drain() - finally: - channel.close() - await tools.close() + while True: + line = await messages.readline() + if line == b'': + return + request = json.loads(line) + reply = await answer(request, sandbox, bench_workers) + # Which request it answers. + reply['id'] = request.get('id') + channel.write((json.dumps(reply) + '\n').encode()) + await channel.drain() + finally: + channel.close() def main() -> None: diff --git a/dev_tools/pylibs/edgacknowledg/forge/publisher/protocol.py b/dev_tools/pylibs/edgacknowledg/forge/publisher/protocol.py index ac57be1fd7..d795e952c9 100644 --- a/dev_tools/pylibs/edgacknowledg/forge/publisher/protocol.py +++ b/dev_tools/pylibs/edgacknowledg/forge/publisher/protocol.py @@ -7,10 +7,22 @@ publisher server: JSON objects, one per WebSocket text message. The client sends one initialization message (settings and the API key, -kept out of any command line or environment), then requests ({'op': 'sync' -| 'remove', ...}), one at a time; the server answers each ({'ok': true} or -{'error': 'unavailable' | 'failed', 'message'}) and, at any time, reports -its AcknowlEDG connection ({'event': 'connected' | 'disconnected'}). +kept out of any command line or environment), then requests, one at a +time: + +- ``{'op': 'sync', 'review', 'sections'}`` serves exactly a review's + sections; with none, it stops serving the review. +- ``{'op': 'delist', 'review_id'}`` stops serving a review whose sections + are still stored in the database, as its files are about to go. +- ``{'op': 'prune', 'keep': [review_id, ...]}`` stops serving every other + review. What the server publishes outlasts a connection, so on + connecting, after serving every review stored in the service's + database, the client sends this, so that nothing else (e.g. what an + earlier service served) stays published. + +The server answers each ({'ok': true} or {'error': 'failed', 'message'}) +once it has queued what to publish, whether or not it is connected to +AcknowlEDG: it publishes what is queued once it is. ''' import uuid diff --git a/dev_tools/pylibs/edgacknowledg/forge/publisher/server.py b/dev_tools/pylibs/edgacknowledg/forge/publisher/server.py index 8ed12850d2..259995f37d 100644 --- a/dev_tools/pylibs/edgacknowledg/forge/publisher/server.py +++ b/dev_tools/pylibs/edgacknowledg/forge/publisher/server.py @@ -3,9 +3,10 @@ # See https://edgcpp.org/LICENSE.txt for license information. # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception -'''The publisher server (``PublisherServer``): serves the service's -publisher client over a WebSocket, in the publisher container (or, without -containers, in the service's process; see ``local``). +'''The publisher server (``create_publisher``): serves the service's +publisher client over a WebSocket at ``/`` (and health checks; see +``serving``), in the publisher container (or, without containers, in the +service's process; see ``local``). It answers reviewers by reading pull request content, so it runs away from the service's credentials, database, and clones: it sees the review trees, @@ -15,11 +16,12 @@ reviewers to the tools runner, so a tools runner they can't reach fails only the requests running one. -Each connection gets a publisher of its own (an ``AcknowledgPublisher``, -with the settings the client sends first), which lasts as long as the -connection: when the connection goes, so do the publisher and everything it -served, and the client serves it all again on its next connection. A new -connection replaces the current one. See ``protocol`` for the messages. +Connections only queue what to publish (``acknowledg.Wanted``); one +publishing task (``acknowledg.publish``, with the settings a client sends +first) publishes it, whenever it is connected to AcknowlEDG. What it +publishes outlasts any connection; a client that connects serves every +review stored in the service's database (again). See ``protocol`` for the +messages. ''' import argparse @@ -30,15 +32,21 @@ import sys import uuid +from dataclasses import dataclass from pathlib import Path -from typing import Any, Optional +from typing import Any, AsyncIterator, Optional -from websockets.asyncio.server import Server, ServerConnection, serve -from websockets.exceptions import ConnectionClosed +from quart import Blueprint, websocket +from edgacknowledg.forge import serving from edgacknowledg.forge.config import ENV_PREFIX, ConfigError -from edgacknowledg.forge.publisher import PublisherUnavailableError -from edgacknowledg.forge.publisher.acknowledg import AcknowledgPublisher +from edgacknowledg.forge.publisher.acknowledg import ( + Delist, + Prune, + Sync, + Wanted, + publish, +) from edgacknowledg.forge.publisher.pool import ( DEFAULT_PROCESSES, DEFAULT_REQUEST_TIMEOUT_S, @@ -53,166 +61,141 @@ _log = logging.getLogger(__name__) -class _ReportingPublisher(AcknowledgPublisher): - '''Also reports its AcknowlEDG connection to the client.''' - - def __init__(self, - outgoing: 'asyncio.Queue[dict[str, Any]]', - **kwargs: Any) -> None: - super().__init__(**kwargs) - self._outgoing = outgoing - - def connected(self) -> None: - super().connected() - self._outgoing.put_nowait({'event': 'connected'}) - - def disconnected(self) -> None: - super().disconnected() - self._outgoing.put_nowait({'event': 'disconnected'}) +@dataclass(frozen = True) +class _Settings: + '''What a client sends first: the publisher's settings.''' + api_host: str + api_port: int + api_key: Optional[str] + reviews_dir: Path -async def _handle(publisher: AcknowledgPublisher, - request: dict[str, Any]) -> None: - operation = request.get('op') - if operation == 'sync': - await publisher.sync( - decode_review(request['review']), - [decode_section(section) for section in request['sections']] - ) - elif operation == 'remove': - await publisher.remove(uuid.UUID(request['review_id'])) - else: - raise ValueError(f"unknown operation: {operation!r}") - - -class _Session: - '''One connection, and the publisher serving it.''' +def _decode_settings(value: Any) -> _Settings: + return _Settings( + api_host = str(value['api_host']), + api_port = int(value['api_port']), + api_key = None if value['api_key'] is None else str(value['api_key']), + reviews_dir = Path(value['reviews_dir']) + ) - def __init__(self, - connection: ServerConnection, - server: 'PublisherServer') -> None: - self.connection = connection - self._server = server - # Everything sent to the client, in order: answers and events. - self._outgoing: asyncio.Queue[dict[str, Any]] = asyncio.Queue() - async def run(self) -> None: - settings = json.loads(await self.connection.recv()) - publisher = _ReportingPublisher( - self._outgoing, - api_host = settings['api_host'], - api_port = int(settings['api_port']), - api_key = settings['api_key'], - ui_host = settings['ui_host'], - ui_port = int(settings['ui_port']), - pool = self._server.pool, - reviews_dir = Path(settings['reviews_dir']) - ) - del settings - tasks = [ - asyncio.create_task(publisher.run()), - asyncio.create_task(self._send()), - asyncio.create_task(self._serve_requests(publisher)), - ] +def _answer(wanted: Wanted, request: dict[str, Any]) -> dict[str, Any]: + '''Queue what ``request`` asks to publish; its answer.''' + answer: dict[str, Any] = {'ok': True} + try: + operation = request.get('op') + if operation == 'sync': + wanted.put_nowait(Sync( + decode_review(request['review']), + [decode_section(section) for section in request['sections']] + )) + elif operation == 'delist': + wanted.put_nowait(Delist(uuid.UUID(request['review_id']))) + elif operation == 'prune': + wanted.put_nowait(Prune(frozenset( + uuid.UUID(value) for value in request['keep'] + ))) + else: + raise ValueError(f"unknown operation: {operation!r}") + except Exception as ex: + _log.exception('request %s failed', request.get('op')) + answer = {'error': 'failed', 'message': f"{type(ex).__name__}: {ex}"} + return answer + + +async def _keep_publishing(settings: 'asyncio.Queue[_Settings]', + wanted: Wanted, + pool: ProcessPool) -> None: + '''Publish what ``wanted`` asks for with the latest ``settings`` a client + sent, until cancelled: from the first ones on; again with new ones, or + should publishing fail (what it published goes with it; the clients + serve it all again on connecting). + ''' + current = await settings.get() + while True: + publishing = asyncio.create_task(publish( + wanted, + api_host = current.api_host, + api_port = current.api_port, + api_key = current.api_key, + pool = pool, + reviews_dir = current.reviews_dir + )) + changing = asyncio.create_task(settings.get()) try: - # Until the connection goes (or the publisher fails). - await asyncio.wait(tasks, return_when = asyncio.FIRST_COMPLETED) + done, _ = await asyncio.wait({publishing, changing}, + return_when = asyncio.FIRST_COMPLETED) finally: - for task in tasks: + for task in (publishing, changing): task.cancel() - await asyncio.gather(*tasks, return_exceptions = True) - - async def _serve_requests(self, publisher: AcknowledgPublisher) -> None: - '''Answer requests, one at a time, until the client goes.''' - try: - async for message in self.connection: - request = json.loads(message) - answer: dict[str, Any] - try: - await _handle(publisher, request) - answer = {'ok': True} - except PublisherUnavailableError as ex: - answer = {'error': 'unavailable', 'message': str(ex)} - except Exception as ex: - _log.exception('request %s failed', request.get('op')) - answer = {'error': 'failed', 'message': f"{type(ex).__name__}: {ex}"} - self._outgoing.put_nowait(answer) - except ConnectionClosed: - pass - - async def _send(self) -> None: - try: - while True: - await self.connection.send(json.dumps(await self._outgoing.get())) - except ConnectionClosed: - pass - - -class PublisherServer: - '''Serves a publisher to each connection from the client, answering its - reviewers in a pool of ``processes`` processes, whose commands run in the - tools runner at ``tools_service_url``. + await asyncio.gather(publishing, changing, return_exceptions = True) + if changing in done: + _log.warning('restarting the publisher with new settings') + current = changing.result() + else: + _log.error('the publisher stopped (%r); restarting it', + publishing.exception()) + await asyncio.sleep(1) + + +def create_publisher(*, + tools_service_url: str, + processes: int = DEFAULT_PROCESSES, + request_timeout: float = DEFAULT_REQUEST_TIMEOUT_S + ) -> Blueprint: + '''The publisher server, publishing for its clients over a WebSocket at + ``/``, answering reviewers in a pool of ``processes`` processes, whose + commands run in the tools runner at ``tools_service_url``; both for as + long as the app serves. Serve it with ``serving``, taking messages of up + to ``MESSAGE_LIMIT``. ''' - - def __init__(self, - *, - tools_service_url: str, - processes: int = DEFAULT_PROCESSES, - request_timeout: float = DEFAULT_REQUEST_TIMEOUT_S) -> None: - # Shared by every connection's publisher, for the server's life. - self.pool = ProcessPool( - tools_service_url = tools_service_url, - processes = processes, - request_timeout = request_timeout - ) - self._current: Optional[_Session] = None - - async def start(self, host: str, port: int) -> Server: - '''Start the pool, and serving (port 0: any free one); close the - server, then ``close``, to stop. - ''' - await self.pool.start() - try: - return await serve(self._handle, host, port, max_size = MESSAGE_LIMIT) - except BaseException: - await self.pool.close() - raise - - async def close(self) -> None: - '''Stop the pool.''' - await self.pool.close() - - async def serve(self, host: str, port: int) -> None: + blueprint = Blueprint('publisher', __name__) + pool = ProcessPool( + tools_service_url = tools_service_url, + processes = processes, + request_timeout = request_timeout + ) + wanted: Wanted = asyncio.Queue() + # The settings clients send, when they differ from the last ones. + settings: asyncio.Queue[_Settings] = asyncio.Queue() + latest_settings: Optional[_Settings] = None + + @blueprint.while_app_serving + async def run_publishing() -> AsyncIterator[None]: + await pool.start() + publishing = asyncio.create_task(_keep_publishing(settings, wanted, pool)) try: - async with await self.start(host, port) as server: - await server.serve_forever() + yield finally: - await self.close() - - def drop_connection(self) -> None: - '''Drop the current connection, if any (the client reconnects).''' - if self._current is not None: - self._current.connection.transport.abort() - - async def _handle(self, connection: ServerConnection) -> None: - if self._current is not None: - _log.warning('a new connection replaces the current one') - self.drop_connection() - session = _Session(connection, self) - self._current = session - _log.info('serving a publisher for a new connection') + publishing.cancel() + await asyncio.gather(publishing, return_exceptions = True) + await pool.close() + + @blueprint.websocket('/') + async def serve_connection() -> None: + '''Queue what the connection's client asks to publish, until the + connection goes (Quart cancels this). + ''' + nonlocal latest_settings + client_settings = _decode_settings(json.loads(await websocket.receive())) + if client_settings != latest_settings: + latest_settings = client_settings + settings.put_nowait(client_settings) + _log.info('a client connected') try: - await session.run() + while True: + request = json.loads(await websocket.receive()) + await websocket.send(json.dumps(_answer(wanted, request))) finally: - if self._current is session: - self._current = None - _log.info('the connection, and its publisher, are gone') + _log.info('a client disconnected') + + return blueprint def main() -> None: '''The publisher container's entry point.''' parser = argparse.ArgumentParser( - description = 'Serve a publisher to each connection from the service.' + description = 'Publish for the service, over its connections.' ) parser.add_argument( '--host', @@ -246,12 +229,17 @@ def main() -> None: print(f"configuration error: {ENV_PREFIX}TOOLS_SERVICE_URL must be a " f"ws:// URL", file = sys.stderr) sys.exit(2) - server = PublisherServer( + publisher = create_publisher( tools_service_url = tools_service_url, processes = args.processes, request_timeout = args.request_timeout ) try: - asyncio.run(server.serve(args.host, args.port)) + asyncio.run(serving.serve( + publisher, + host = args.host, + port = args.port, + max_message_size = MESSAGE_LIMIT + )) except KeyboardInterrupt: pass diff --git a/dev_tools/pylibs/edgacknowledg/forge/runner/client.py b/dev_tools/pylibs/edgacknowledg/forge/runner/client.py index 1f24e79865..8df5eb4160 100644 --- a/dev_tools/pylibs/edgacknowledg/forge/runner/client.py +++ b/dev_tools/pylibs/edgacknowledg/forge/runner/client.py @@ -7,42 +7,83 @@ container (see ``runner.server``), over one long-lived connection. The service has one, for its builds; each of the publisher's pool processes -has one, for the commands its handlers run (through ``SandboxContext``). A -client connects when first used, and again when its connection goes (which -fails the commands in flight on it: the server kills them). +has one, for the commands its handlers run (through ``SandboxContext``). + +While it runs (``async with``), a client keeps one connection to its +runner (with a ``ForgeSandboxClient``: reconnecting whenever it goes, and +never giving up), which every command it runs shares. Each +request carries an ``id`` of the client's choosing, which the runner's +answer, and every chunk of the command's output, carry back, so any number +of commands run at once over it. A request waits for the connection, +however long it takes to come back, and the runner keeps a command's time; +the connection going fails the commands in flight on it, as the runner +kills them. ''' import asyncio import json -import logging from pathlib import Path -from typing import Any, Callable, Mapping, Optional, Sequence +from types import TracebackType +from typing import Any, Mapping, Optional, Sequence, Type -from websockets.asyncio.client import ClientConnection, connect -from websockets.exceptions import ConnectionClosed, InvalidHandshake +from websockets.asyncio.client import ClientConnection +from websockets.exceptions import ConnectionClosed from edgacknowledg.client.shared import CompletedCommand from edgacknowledg.forge.runner.server import ID_BYTES, OUTPUT_CHUNK +from edgacknowledg.forge.sandbox_client import ForgeSandboxClient -_log = logging.getLogger(__name__) - -# Seconds to wait for the runner to answer beyond the command's own limit -# (it enforces that one). -_ANSWER_GRACE_S = 30.0 _OPEN_TIMEOUT_S = 10.0 +# The most of a command's output ``run`` keeps (in bytes): its start, and +# (where its errors are) its last ``OUTPUT_TAIL_BYTES``. +MAX_OUTPUT_BYTES = 8 * 1024 * 1024 +OUTPUT_TAIL_BYTES = 64 * 1024 __all__ = ['CompletedCommand', 'RunnerClient', 'RunnerError'] class RunnerError(Exception): - '''The runner couldn't be reached, or refused the command.''' + '''The connection to the runner went, or it refused the command.''' + + +class _Output: + '''A command's output, kept within ``MAX_OUTPUT_BYTES``: what goes beyond + is dropped from its middle. + ''' + + def __init__(self) -> None: + self._head = bytearray() + self._tail = bytearray() + self._dropped = 0 + + def add(self, chunk: bytes) -> None: + room = MAX_OUTPUT_BYTES - OUTPUT_TAIL_BYTES - len(self._head) + if room > 0: + self._head += chunk[:room] + chunk = chunk[room:] + self._tail += chunk + excess = len(self._tail) - OUTPUT_TAIL_BYTES + if excess > 0: + del self._tail[:excess] + self._dropped += excess + + def value(self) -> bytes: + if self._dropped == 0: + return bytes(self._head + self._tail) + return b''.join([ + self._head, + f"\n[... {self._dropped} bytes of output dropped ...]\n".encode(), + self._tail, + ]) class _Pending: - '''A request awaiting its answer.''' + '''A request awaiting its answer, collecting its command's output (in + ``output``), if it runs one. + ''' - def __init__(self, output: Optional[Callable[[bytes], None]]) -> None: + def __init__(self, output: Optional[_Output]) -> None: self.answer: asyncio.Future[dict[str, Any]] = ( asyncio.get_running_loop().create_future() ) @@ -51,24 +92,53 @@ def __init__(self, output: Optional[Callable[[bytes], None]]) -> None: class RunnerClient: '''The client of the runner server at ``url``, e.g. - ``ws://builder:2730``. + ``ws://builder:2730``, connected while it runs (``async with``, or + ``start`` and ``close``, which kills whatever it still runs); a + connection takes at most ``open_timeout`` seconds to open. ''' - def __init__(self, url: str) -> None: + def __init__(self, + url: str, + *, + open_timeout: float = _OPEN_TIMEOUT_S) -> None: self.url = url - self._connection: Optional[ClientConnection] = None - self._reader: Optional[asyncio.Task[None]] = None - self._connecting: Optional[asyncio.Lock] = None + self._link = ForgeSandboxClient( + url, + peer = 'the runner', + serve = self._serve, + open_timeout = open_timeout, + max_size = ID_BYTES + OUTPUT_CHUNK + 1024 + ) + # The requests sent on the connection, awaiting their answers, by id. self._pending: dict[int, _Pending] = {} self._next_id = 0 def __repr__(self) -> str: return f"RunnerClient({self.url!r})" + def start(self) -> None: + '''Start keeping the connection (until ``close``).''' + self._link.start() + + async def close(self) -> None: + '''Close the connection (killing whatever it still runs), and stop + keeping it. + ''' + await self._link.close() + + async def __aenter__(self) -> 'RunnerClient': + self.start() + return self + + async def __aexit__(self, + exc_type: Optional[Type[BaseException]], + exc: Optional[BaseException], + tb: Optional[TracebackType]) -> None: + await self.close() + async def describe(self) -> dict[str, Any]: '''What the runner allows.''' - value = await self._request({'op': 'describe'}, - timeout = _OPEN_TIMEOUT_S + _ANSWER_GRACE_S) + value = await self._request({'op': 'describe'}) return dict(value['description']) async def run(self, @@ -76,19 +146,13 @@ async def run(self, *, cwd: Optional[Path] = None, env: Mapping[str, str] = {}, - timeout: float, - output: Optional[Callable[[bytes], None]] = None - ) -> CompletedCommand: - '''Run ``argv`` in the runner (stdout and stderr together, also passed - to ``output`` as they come). Cancelling this kills the command. + timeout: float) -> CompletedCommand: + '''Run ``argv`` in the runner (stdout and stderr together), which ends + it after ``timeout`` seconds. Its output beyond ``MAX_OUTPUT_BYTES`` + is dropped from the middle. + Cancelling this kills the command. ''' - collected = bytearray() - - def collect(chunk: bytes) -> None: - collected.extend(chunk) - if output is not None: - output(chunk) - + output = _Output() value = await self._request( { 'op': 'run', @@ -97,12 +161,11 @@ def collect(chunk: bytes) -> None: 'env': dict(env), 'timeout': timeout, }, - timeout = timeout + _ANSWER_GRACE_S, - output = collect + output = output ) return CompletedCommand( int(value['returncode']), - bytes(collected), + output.value(), bool(value['timed_out']) ) @@ -112,36 +175,23 @@ async def run_command(self, '''``run``, as a ``SandboxContext`` command runner.''' return await self.run(argv, timeout = timeout) - async def close(self) -> None: - '''Close the connection (killing whatever it still runs).''' - connection = self._connection - if connection is not None: - await connection.close() - if self._reader is not None: - await asyncio.gather(self._reader, return_exceptions = True) - - async def _connected(self) -> ClientConnection: - if self._connecting is None: - self._connecting = asyncio.Lock() - async with self._connecting: - if self._connection is None: - try: - connection = await connect( - self.url, - open_timeout = _OPEN_TIMEOUT_S, - max_size = ID_BYTES + OUTPUT_CHUNK + 1024 + async def _serve(self, connection: ClientConnection) -> None: + '''Take requests on ``connection``; fail those in flight when it + goes. + ''' + try: + await self._read(connection) + finally: + for pending in self._pending.values(): + if not pending.answer.done(): + pending.answer.set_exception( + RunnerError(f"lost the runner at {self.url}") ) - except (OSError, InvalidHandshake, asyncio.TimeoutError) as ex: - raise RunnerError( - f"the runner at {self.url} didn't answer: {ex!r}" - ) from ex - self._connection = connection - self._reader = asyncio.create_task(self._read(connection)) - return self._connection async def _read(self, connection: ClientConnection) -> None: '''Hand each message to the request it answers, until the connection - goes; then fail what is still waiting. + goes. A message that isn't one the runner sends raises: no later one + can be trusted. ''' try: async for message in connection: @@ -149,55 +199,42 @@ async def _read(self, connection: ClientConnection) -> None: request_id = int.from_bytes(message[:ID_BYTES], 'big') pending = self._pending.get(request_id) if pending is not None and pending.output is not None: - pending.output(message[ID_BYTES:]) + pending.output.add(message[ID_BYTES:]) continue value = json.loads(message) - pending = self._pending.get(value.get('id')) + if not isinstance(value, dict): + raise ValueError(f"not an answer: {message[:200]!r}") + answers = value.get('id') + pending = (self._pending.get(answers) + if isinstance(answers, int) else None) if pending is not None and not pending.answer.done(): pending.answer.set_result(value) except ConnectionClosed: pass - except Exception: - _log.exception('reading from the runner at %s failed', self.url) - finally: - if self._connection is connection: - self._connection = None - for pending in self._pending.values(): - if not pending.answer.done(): - pending.answer.set_exception( - RunnerError(f"lost the runner at {self.url}") - ) async def _request(self, request: dict[str, Any], *, - timeout: float, - output: Optional[Callable[[bytes], None]] = None - ) -> dict[str, Any]: - '''Send ``request``; pass any output to ``output`` and return the final - answer. + output: Optional[_Output] = None) -> dict[str, Any]: + '''Send ``request`` once connected; collect any output in ``output``, + and return the final answer (the runner keeps the time for the + command). ''' - connection = await self._connected() + connection = await self._link.wait_connected() request_id = self._next_id self._next_id = (self._next_id + 1) % 2 ** (8 * ID_BYTES) pending = _Pending(output) self._pending[request_id] = pending try: - try: - await connection.send(json.dumps({'id': request_id, **request})) - value = await asyncio.wait_for(asyncio.shield(pending.answer), - timeout) - except asyncio.TimeoutError: - await self._cancel(connection, request_id) - raise RunnerError( - f"the runner at {self.url} didn't answer in time" - ) from None - except asyncio.CancelledError: - # The caller gave up: kill the command. - await asyncio.shield(self._cancel(connection, request_id)) - raise - except ConnectionClosed as ex: - raise RunnerError(f"lost the runner at {self.url}: {ex}") from ex + await connection.send(json.dumps({'id': request_id, **request})) + value = await pending.answer + except asyncio.CancelledError: + # The caller gave up: kill the command. Shielded, so being cancelled + # again can't keep the kill from being sent. + await asyncio.shield(self._cancel(connection, request_id)) + raise + except ConnectionClosed as ex: + raise RunnerError(f"lost the runner at {self.url}: {ex}") from ex finally: self._pending.pop(request_id, None) if 'error' in value: diff --git a/dev_tools/pylibs/edgacknowledg/forge/runner/server.py b/dev_tools/pylibs/edgacknowledg/forge/runner/server.py index 954cefdfd0..c30a4b7faa 100644 --- a/dev_tools/pylibs/edgacknowledg/forge/runner/server.py +++ b/dev_tools/pylibs/edgacknowledg/forge/runner/server.py @@ -14,7 +14,8 @@ to write), hold no secret, and have no network but the one they share with their client (see compose.yaml). ``client`` is the client. -The client keeps one connection open, and sends requests on it (JSON text +It serves a WebSocket at ``/`` (and health checks; see ``serving``). The +client keeps one connection open, and sends requests on it (JSON text messages), each with an ``id`` of its choosing that the answers carry: - ``{"id", "op": "run", "argv": [...], "cwd", "env": {...}, "timeout"}`` @@ -51,9 +52,12 @@ from pathlib import Path from typing import Any, Mapping, Optional, Sequence -from websockets.asyncio.server import Server, ServerConnection, serve -from websockets.exceptions import ConnectionClosed +import quart.wrappers +from quart import Blueprint, websocket +from quart.wrappers import Websocket + +from edgacknowledg.forge import serving from edgacknowledg.forge.runner.roles import ROLES, RunnerPolicy _log = logging.getLogger('edgacknowledg.forge.runner.server') @@ -70,6 +74,20 @@ class RequestError(Exception): '''A request the server refuses; the message says why.''' +class _NeverRaised(Exception): + '''Stands in for an exception the installed Quart doesn't have.''' + + +# What Quart raises when a client disconnects changed: from Quart 0.23 +# onward (Python 3.13+), ClientDisconnectedError is used. Prior to this, +# nothing is raised and Quart only cancels the handler. +_ClientDisconnectedError: type[Exception] = getattr( + quart.wrappers, + 'ClientDisconnectedError', + _NeverRaised +) + + def _under_roots(value: str, roots: Sequence[Path]) -> bool: # Lexically: the container's mounts are the actual boundary, this only # keeps a client from naming anything else. @@ -189,96 +207,49 @@ def _empty_directory(path: Path) -> None: pass -class RunnerServer: - '''Serves ``policy`` to its client.''' - - def __init__(self, - policy: RunnerPolicy, - *, - container: bool = False) -> None: - self._policy = policy - self._container = container - self._commands = policy.resolved_commands() - self._active = 0 - - async def start(self, host: str, port: int) -> Server: - '''Start serving (port 0: any free one); close the server to stop.''' - return await serve( - self._handle, - host, - port, - max_size = MAX_REQUEST_BYTES - ) - - async def serve(self, host: str, port: int) -> None: - async with await self.start(host, port) as server: - await server.serve_forever() - - async def _handle(self, connection: ServerConnection) -> None: - '''Serve one connection's requests until it goes, then kill whatever - it still runs. - ''' - commands: dict[int, asyncio.Task[None]] = {} - try: - async for raw in connection: - try: - request = json.loads(raw) - except ValueError: - _log.warning('ignored a malformed request') - continue - request_id = request.get('id') if isinstance(request, dict) else None - if (not isinstance(request_id, int) or - not 0 <= request_id < 2 ** (8 * ID_BYTES)): - _log.warning('ignored a request without a valid id') - continue - op = request.get('op') - if op == 'cancel': - command = commands.get(request_id) - if command is not None: - command.cancel() - continue - try: - if op == 'describe': - await connection.send(json.dumps({ - 'id': request_id, - 'description': self._policy.describe(), - })) - elif op == 'run': - job = check_request(request, self._policy, self._commands) - command = asyncio.create_task( - self._run(connection, request_id, job) - ) - commands[request_id] = command +async def _relay(connection: Websocket, + request_id: int, + process: asyncio.subprocess.Process) -> None: + assert process.stdout is not None + prefix = request_id.to_bytes(ID_BYTES, 'big') + while True: + chunk = await process.stdout.read(OUTPUT_CHUNK) + if chunk == b'': + return + await connection.send(prefix + chunk) - def forget(_: 'asyncio.Task[None]', - request_id: int = request_id) -> None: - commands.pop(request_id, None) - command.add_done_callback(forget) - else: - raise RequestError(f"unknown operation: {op!r}") - except RequestError as ex: - _log.warning('refused a request: %s', ex) - await connection.send(json.dumps({ - 'id': request_id, - 'error': str(ex), - })) - except ConnectionClosed: +def _clean_up() -> None: + '''With no command running, leave nothing of one behind (only inside a + container of its own). + ''' + for pid in stray_processes(): + try: + os.kill(pid, signal.SIGKILL) + except (ProcessLookupError, PermissionError): pass - finally: - for command in list(commands.values()): - command.cancel() - await asyncio.gather(*commands.values(), return_exceptions = True) + _empty_directory(Path('/tmp')) - async def _run(self, - connection: ServerConnection, - request_id: int, - job: Job) -> None: + +def create_runner(policy: RunnerPolicy, + *, + container: bool = False) -> Blueprint: + '''The runner server serving ``policy`` to its client, over a WebSocket + at ``/``; with ``container``, cleaning up after its commands. Serve it + with ``serving``, taking messages of up to ``MAX_REQUEST_BYTES``. + ''' + blueprint = Blueprint('runner', __name__) + commands_allowed = policy.resolved_commands() + # Commands running, over every connection. + active = 0 + + async def run(connection: Websocket, request_id: int, job: Job) -> None: '''Run ``job``, relaying its output, until it ends, times out, or is cancelled (by the client, or with its connection). ''' + nonlocal active _log.info('running %s', shlex.join(job.argv)) - self._active += 1 + active += 1 try: try: process = await asyncio.create_subprocess_exec( @@ -300,7 +271,7 @@ async def _run(self, timed_out = False try: await asyncio.wait_for( - self._relay(connection, request_id, process), + _relay(connection, request_id, process), job.timeout ) except asyncio.TimeoutError: @@ -319,35 +290,70 @@ async def _run(self, if process.returncode is None: process.kill() await process.wait() - except ConnectionClosed: + except _ClientDisconnectedError: pass finally: - self._active -= 1 - if self._active == 0: - self._clean_up() - - @staticmethod - async def _relay(connection: ServerConnection, - request_id: int, - process: asyncio.subprocess.Process) -> None: - assert process.stdout is not None - prefix = request_id.to_bytes(ID_BYTES, 'big') - while True: - chunk = await process.stdout.read(OUTPUT_CHUNK) - if chunk == b'': - return - await connection.send(prefix + chunk) + active -= 1 + if active == 0 and container: + _clean_up() + + @blueprint.websocket('/') + async def serve_connection() -> None: + '''Serve one connection's requests until it goes (Quart cancels this), + then kill whatever it still runs. + ''' + commands: dict[int, asyncio.Task[None]] = {} + try: + while True: + raw = await websocket.receive() + try: + request = json.loads(raw) + except ValueError: + _log.warning('ignored a malformed request') + continue + request_id = request.get('id') if isinstance(request, dict) else None + if (not isinstance(request_id, int) or + not 0 <= request_id < 2 ** (8 * ID_BYTES)): + _log.warning('ignored a request without a valid id') + continue + op = request.get('op') + if op == 'cancel': + command = commands.get(request_id) + if command is not None: + command.cancel() + continue + try: + if op == 'describe': + await websocket.send(json.dumps({ + 'id': request_id, + 'description': policy.describe(), + })) + elif op == 'run': + job = check_request(request, policy, commands_allowed) + command = asyncio.create_task(run(websocket, request_id, job)) + commands[request_id] = command - def _clean_up(self) -> None: - '''With no command running, leave nothing of one behind.''' - if not self._container: - return - for pid in stray_processes(): - try: - os.kill(pid, signal.SIGKILL) - except (ProcessLookupError, PermissionError): - pass - _empty_directory(Path('/tmp')) + def forget(_: 'asyncio.Task[None]', + request_id: int = request_id) -> None: + commands.pop(request_id, None) + + command.add_done_callback(forget) + else: + raise RequestError(f"unknown operation: {op!r}") + except RequestError as ex: + _log.warning('refused a request: %s', ex) + await websocket.send(json.dumps({ + 'id': request_id, + 'error': str(ex), + })) + except _ClientDisconnectedError: + pass + finally: + for command in list(commands.values()): + command.cancel() + await asyncio.gather(*commands.values(), return_exceptions = True) + + return blueprint def main(role: str) -> None: @@ -386,8 +392,12 @@ def main(role: str) -> None: home = args.home.absolute(), tools_dir = args.tools_dir.absolute() ) - server = RunnerServer(policy, container = args.container) try: - asyncio.run(server.serve(args.host, args.port)) + asyncio.run(serving.serve( + create_runner(policy, container = args.container), + host = args.host, + port = args.port, + max_message_size = MAX_REQUEST_BYTES + )) except KeyboardInterrupt: pass diff --git a/dev_tools/pylibs/edgacknowledg/forge/sandbox.py b/dev_tools/pylibs/edgacknowledg/forge/sandbox.py index 062621c643..32797c8ebb 100644 --- a/dev_tools/pylibs/edgacknowledg/forge/sandbox.py +++ b/dev_tools/pylibs/edgacknowledg/forge/sandbox.py @@ -83,8 +83,8 @@ def tools(self) -> Optional[RunnerClient]: return self._tools async def check(self) -> None: - '''Raise ``SandboxUnavailableError`` unless this process's runners - answer. + '''Wait until this process's runners answer; raise + ``SandboxUnavailableError`` if there is none, or one refuses. ''' clients = [c for c in (self._builder, self._tools) if c is not None] if len(clients) == 0: diff --git a/dev_tools/pylibs/edgacknowledg/forge/sandbox_client.py b/dev_tools/pylibs/edgacknowledg/forge/sandbox_client.py new file mode 100644 index 0000000000..19155896f9 --- /dev/null +++ b/dev_tools/pylibs/edgacknowledg/forge/sandbox_client.py @@ -0,0 +1,160 @@ +# Part of the EDG Compiler Project, under the Apache License v2.0 with LLVM +# Exceptions. +# See https://edgcpp.org/LICENSE.txt for license information. +# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +'''Keeping one connection to a server in another sandbox container +(``ForgeSandboxClient``), whichever protocol the server speaks: each client +of one, ``RunnerClient`` (``runner.client``) and ``PublisherClient`` +(``publisher.client``), has one, and serves its protocol over each +connection it makes. + +While it runs (``start`` until ``close``), it keeps one connection to its +server, and never gives up on it: the websockets library reconnects +whenever it goes, or can't be made, backing off exponentially. Connecting +failing in a way retrying may not fix (e.g. being refused) is logged, and +retried all the same: the server may yet be fixed. +''' + +import asyncio +import logging + +from typing import Awaitable, Callable, Generator, Optional + +from websockets.asyncio.client import ClientConnection, connect +from websockets.client import backoff, process_exception + +_log = logging.getLogger(__name__) + +# Seconds to wait before connecting again, so that a connection that keeps +# going at once (e.g. while serving it fails) doesn't spin. +RECONNECT_DELAY = 1.0 + + +class ServiceUnavailableError(Exception): + '''Raised when a server in another sandbox container can't take a request + right now (retry later). + ''' + + +class ForgeSandboxClient: + '''Keeps a connection to the server at ``url`` (``peer`` names it, for + messages), while it runs; a connection takes at most ``open_timeout`` + seconds to open, and takes messages of up to ``max_size`` bytes. + + ``serve`` serves each connection until it goes (raising has it go, to + connect again); requests wait for one (``wait_connected``), or check + there is one (``connected``). + ''' + + def __init__(self, + url: str, + *, + peer: str, + serve: Callable[[ClientConnection], Awaitable[None]], + open_timeout: float, + max_size: int) -> None: + self.url = url + self._peer = peer + self._serve = serve + self._open_timeout = open_timeout + self._max_size = max_size + # The task keeping the connection, while it runs; the connection, and + # whether there is one, are its alone to change. + self._running: Optional[asyncio.Task[None]] = None + self._connection: Optional[ClientConnection] = None + # Set while connected, or once closed. + self._connected = asyncio.Event() + self._closed = False + + def start(self) -> None: + '''Start keeping the connection (until ``close``).''' + if self._running is None: + self._closed = False + self._connected.clear() + self._running = asyncio.create_task(self._keep_connected()) + + async def close(self) -> None: + '''Close the connection, and stop keeping it; requests waiting for it + fail. + ''' + self._closed = True + try: + if self._running is not None: + self._running.cancel() + await asyncio.gather(self._running, return_exceptions = True) + self._running = None + finally: + self._connected.set() + + @property + def connection(self) -> Optional[ClientConnection]: + '''The connection being served, if any.''' + return self._connection + + @property + def connected(self) -> bool: + '''Whether there is a connection now.''' + return (not self._closed and self._connection is not None and + self._connected.is_set()) + + async def wait_connected(self) -> ClientConnection: + '''The connection, once there is one; raises + ``ServiceUnavailableError`` if closed first. + ''' + while True: + await self._connected.wait() + if self._closed: + raise ServiceUnavailableError( + f"the client of {self._peer} at {self.url} is closed" + ) + # It may have gone since the event was set: then wait for the next. + connection = self._connection + if connection is not None and self._connected.is_set(): + return connection + + def drop(self, connection: ClientConnection) -> None: + '''Send ``connection`` nothing more, and have it go (to connect + again). + ''' + if self._connection is connection: + self._connection = None + self._connected.clear() + connection.transport.abort() + + # Connection + + @staticmethod + def _reconnect_delays() -> Generator[float, None, None]: + '''Seconds to wait between attempts to connect.''' + return backoff() + + def _retry(self, ex: Exception) -> Optional[Exception]: + '''Have websockets retry connecting, whatever the error.''' + if process_exception(ex) is not None: + _log.warning("can't connect to %s at %s (%s); retrying", self._peer, + self.url, ex) + return None + + async def _keep_connected(self) -> None: + '''Keep a connection to the server, until cancelled.''' + async for connection in connect( + self.url, + open_timeout = self._open_timeout, + max_size = self._max_size, + process_exception = self._retry, + reconnect_delays = self._reconnect_delays): + self._connection = connection + self._connected.set() + try: + await self._serve(connection) + except Exception: + _log.exception('serving the connection to %s at %s failed', + self._peer, self.url) + finally: + self._connected.clear() + self._connection = None + connection.transport.abort() + await connection.close() + _log.warning('lost %s at %s; reconnecting', self._peer, self.url) + await asyncio.sleep(RECONNECT_DELAY) diff --git a/dev_tools/pylibs/edgacknowledg/forge/service.py b/dev_tools/pylibs/edgacknowledg/forge/service.py index 604e3fa004..dc2b288afa 100644 --- a/dev_tools/pylibs/edgacknowledg/forge/service.py +++ b/dev_tools/pylibs/edgacknowledg/forge/service.py @@ -14,7 +14,8 @@ tools, and publisher containers run (``local``), - the processor (and, if enabled, the announcer after each sweep), - the poller (always: at least the startup sync), -- the webhook receiver and its event worker (only if configured). +- with a service bind, the web app (``serving``): health checks, and if + enabled, the webhook receiver (``webhooks``) and its event worker. ''' import asyncio @@ -26,7 +27,7 @@ from dataclasses import dataclass from typing import Callable, Optional -from edgacknowledg.forge import Forge, ForgeCommenter, ForgeWebhooks +from edgacknowledg.forge import Forge, ForgeCommenter, ForgeWebhooks, serving from edgacknowledg.forge.announcer import Announcer from edgacknowledg.forge.config import ConfigError, ForgeServiceConfig from edgacknowledg.forge.db import ForgeStore @@ -40,7 +41,7 @@ Sandbox, SandboxUnavailableError, ) -from edgacknowledg.forge.webhooks import EventQueue, create_app +from edgacknowledg.forge.webhooks import EventQueue, create_receiver from edgacknowledg.forge.workspace import Workspaces _log = logging.getLogger(__name__) @@ -54,33 +55,12 @@ class ForgeComponents: commenter: Optional[ForgeCommenter] = None -async def _serve_webhooks(app: object, bind: str) -> None: - # Only needed (and imported) when webhooks are enabled. - from hypercorn.asyncio import serve - from hypercorn.config import Config - - hypercorn_config = Config() - hypercorn_config.bind = [bind] - hypercorn_config.accesslog = None - # Log through the service's logging setup only (not twice). - hypercorn_config.errorlog = None - # Without a shutdown trigger Hypercorn installs its own SIGINT/SIGTERM - # handlers and shuts down alone; the service cancels it with everything - # else instead. - never = asyncio.Event() - await serve( - app, # type: ignore[arg-type] - hypercorn_config, - shutdown_trigger = never.wait - ) - - async def run_service(config: ForgeServiceConfig, components: ForgeComponents) -> None: - if config.webhook_bind is not None and components.webhooks is None: + if config.webhooks and components.webhooks is None: raise ConfigError( - 'ACKNOWLEDG_FORGE_WEBHOOK_BIND is set, but webhooks are not ' - 'configured (is the webhook secret set?)' + 'ACKNOWLEDG_FORGE_WEBHOOKS is set, but webhooks are not configured ' + '(is the webhook secret set?)' ) if config.comments and components.commenter is None: raise ConfigError( @@ -101,8 +81,9 @@ async def run_service(config: ForgeServiceConfig, else: assert config.builder_service_url is not None assert config.publisher_service_url is not None - builder = RunnerClient(config.builder_service_url) - stack.push_async_callback(builder.close) + builder = await stack.enter_async_context( + RunnerClient(config.builder_service_url) + ) publisher_service_url = config.publisher_service_url if len(config.environment_secrets) != 0: _log.warning('secrets come from the environment (%s); use ' @@ -132,7 +113,10 @@ async def _run_components(config: ForgeServiceConfig, api_key = config.acknowledg_api_key, ui_host = config.acknowledg_ui_host, ui_port = config.acknowledg_ui_port, - reviews_dir = workspaces.reviews_dir + reviews_dir = workspaces.reviews_dir, + # What it serves: the database leads. + store = store, + forge_name = forge.name ) processor: Optional[Processor] = None @@ -161,19 +145,26 @@ async def _run_components(config: ForgeServiceConfig, after_sweep = announcer.sweep if announcer is not None else None ) - async with asyncio.TaskGroup() as task_group: - task_group.create_task(publisher.run()) + async with publisher, asyncio.TaskGroup() as task_group: task_group.create_task(processor.run()) task_group.create_task(run_poller(ingestor, config)) - if config.webhook_bind is not None: - assert components.webhooks is not None - events = EventQueue() - task_group.create_task(events.run(ingestor)) - task_group.create_task(_serve_webhooks( - create_app(components.webhooks, events), - config.webhook_bind + # Only now, with everything started, is the service healthy. + if config.service_bind is not None: + blueprints = [] + if config.webhooks: + assert components.webhooks is not None + events = EventQueue() + task_group.create_task(events.run(ingestor)) + blueprints.append(create_receiver(components.webhooks, events)) + host, _, port = config.service_bind.rpartition(':') + task_group.create_task(serving.serve( + *blueprints, + host = host.strip('[]'), + port = int(port) )) - _log.info('receiving webhooks on %s', config.webhook_bind) + _log.info('serving on %s (%s)', config.service_bind, + 'health checks and webhooks' if config.webhooks + else 'health checks') finally: await forge.aclose() await store.aclose() diff --git a/dev_tools/pylibs/edgacknowledg/forge/serving.py b/dev_tools/pylibs/edgacknowledg/forge/serving.py new file mode 100644 index 0000000000..83b788d876 --- /dev/null +++ b/dev_tools/pylibs/edgacknowledg/forge/serving.py @@ -0,0 +1,144 @@ +# Part of the EDG Compiler Project, under the Apache License v2.0 with LLVM +# Exceptions. +# See https://edgcpp.org/LICENSE.txt for license information. +# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +'''Serving every container's server the same way: a Quart app of the +server's blueprints, and the shared ``health`` one (``create_app``), run on +Hypercorn (``start``, ``serve``). The blueprints are the service's +(``webhooks``), the runner's (``runner.server``), and the publisher's +(``publisher.server``), whose WebSockets serve their client at ``/``. + +``health`` answers a plain HTTP ``GET`` of ``HEALTH_PATH``, for +compose.yaml's healthchecks. For the WebSocket servers it is a route of its +own, so a probe never opens a connection. +''' + +import asyncio +import contextlib +import socket + +from http import HTTPStatus +from typing import Any, AsyncIterator, Optional + +from hypercorn.asyncio import serve as hypercorn_serve +from hypercorn.config import Config +from quart import Blueprint, Quart + +HEALTH_PATH = '/healthz' + +# Seconds ``Listening.close`` waits for Hypercorn to stop, before stopping +# it anyway: its shutdown waits for every open connection, so a client that +# never ends its own would hold it up. +_CLOSE_TIMEOUT = 5.0 + +health = Blueprint('health', __name__) + + +@health.get(HEALTH_PATH) +async def healthz() -> Any: + return 'ok\n', HTTPStatus.OK + + +def create_app(*blueprints: Blueprint) -> Quart: + '''An app of ``blueprints``, answering health checks too.''' + app = Quart(__name__) + app.register_blueprint(health) + for blueprint in blueprints: + app.register_blueprint(blueprint) + return app + + +class Listening: + '''An app being served, until ``close``.''' + + def __init__(self, + host: str, + port: int, + task: 'asyncio.Task[None]', + stop: asyncio.Event) -> None: + self.host = host + self.port = port + self._task = task + self._stop = stop + + def url(self, scheme: str = 'ws') -> str: + return f"{scheme}://{self.host}:{self.port}" + + async def wait(self) -> None: + '''Until serving stops (with ``close``, or failing).''' + # Shielded: a waiter giving up (being cancelled) mustn't cut the serving + # off; ``close`` stops it gracefully (as ``serve`` does once cancelled). + await asyncio.shield(self._task) + + async def close(self) -> None: + self._stop.set() + try: + # Shielded: if this is cancelled, the shutdown just started still + # finishes; only running out of time cuts it short (below). + await asyncio.wait_for(asyncio.shield(self._task), _CLOSE_TIMEOUT) + except asyncio.TimeoutError: + self._task.cancel() + with contextlib.suppress(asyncio.CancelledError): + await self._task + + +async def start(*blueprints: Blueprint, + host: str, + port: int, + max_message_size: Optional[int] = None) -> Listening: + '''Serve the app of ``blueprints`` on ``host`` and ``port`` (0: any free + one, hence the socket made here: Hypercorn doesn't tell which), taking + WebSocket messages of up to ``max_message_size`` bytes (default: + Hypercorn's). Started (its blueprints' ``before_app_serving`` and + ``while_app_serving``, e.g. the publisher's pool) and listening on + return, so clients may connect at once. + ''' + app = create_app(*blueprints) + started = asyncio.Event() + + # Last, after the blueprints' own. + @app.while_serving + async def report_started() -> AsyncIterator[None]: + started.set() + yield + + family = socket.getaddrinfo(host, port, type = socket.SOCK_STREAM)[0][0] + sock = socket.create_server((host, port), family = family) + port = sock.getsockname()[1] + config = Config() + # Hypercorn takes the socket (and closes it). + config.bind = [f"fd://{sock.detach()}"] + if max_message_size is not None: + config.websocket_max_message_size = max_message_size + config.accesslog = None + # Log through the process's logging setup only (not twice). + config.errorlog = None + # Without a shutdown trigger Hypercorn installs its own SIGINT/SIGTERM + # handlers and shuts down alone; the caller stops it instead. + stop = asyncio.Event() + task = asyncio.create_task(hypercorn_serve( + app, # type: ignore[arg-type] + config, + shutdown_trigger = stop.wait + )) + # Until started, or failing to (e.g. Hypercorn's LifespanFailureError). + starting = asyncio.create_task(started.wait()) + await asyncio.wait([starting, task], return_when = asyncio.FIRST_COMPLETED) + starting.cancel() + if task.done(): + task.result() + return Listening(host, port, task, stop) + + +async def serve(*blueprints: Blueprint, + host: str, + port: int, + max_message_size: Optional[int] = None) -> None: + '''Serve the app of ``blueprints`` (as ``start``) until cancelled.''' + listening = await start(*blueprints, host = host, port = port, + max_message_size = max_message_size) + try: + await listening.wait() + finally: + await listening.close() diff --git a/dev_tools/pylibs/edgacknowledg/forge/webhooks.py b/dev_tools/pylibs/edgacknowledg/forge/webhooks.py index b6bfb847ab..8b369ed239 100644 --- a/dev_tools/pylibs/edgacknowledg/forge/webhooks.py +++ b/dev_tools/pylibs/edgacknowledg/forge/webhooks.py @@ -3,7 +3,8 @@ # See https://edgcpp.org/LICENSE.txt for license information. # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception -'''Webhook receiver (optional): a small Quart app feeding the ingestor. +'''Webhook receiver (optional): a Quart blueprint (``create_receiver``) of +the service's app (``serving``) feeding the ingestor. Deliveries only identify what may have changed; the ingestor re-queries the forge for the actual state. Events are queued and deduplicated so a burst of @@ -16,7 +17,8 @@ from http import HTTPStatus from typing import Any -from quart import Quart, request +from quart import Blueprint, request +from quart.blueprints import BlueprintSetupState from edgacknowledg.forge import ( CIRunTouched, @@ -64,11 +66,19 @@ async def run(self, ingestor: Ingestor) -> None: _log.exception('failed to handle %s', event) -def create_app(webhooks: ForgeWebhooks, events: EventQueue) -> Quart: - app = Quart(__name__) - app.config['MAX_CONTENT_LENGTH'] = MAX_DELIVERY_BYTES +def create_receiver(webhooks: ForgeWebhooks, + events: EventQueue) -> Blueprint: + '''The receiver, at ``/webhook``, for the service's app (``serving``): + verifies and parses deliveries with ``webhooks``, and queues their events + on ``events``. + ''' + blueprint = Blueprint('webhooks', __name__) - @app.post('/webhook') + @blueprint.record_once + def limit_deliveries(state: BlueprintSetupState) -> None: + state.app.config['MAX_CONTENT_LENGTH'] = MAX_DELIVERY_BYTES + + @blueprint.post('/webhook') async def receive_webhook() -> Any: body = await request.get_data(cache = False, as_text = False) assert isinstance(body, bytes) @@ -80,8 +90,4 @@ async def receive_webhook() -> Any: events.put(event) return '', HTTPStatus.ACCEPTED - @app.get('/healthz') - async def healthz() -> Any: - return 'ok\n', HTTPStatus.OK - - return app + return blueprint diff --git a/dev_tools/pylibs/edgacknowledg/forge/workspace.py b/dev_tools/pylibs/edgacknowledg/forge/workspace.py index dfe154b14f..a1021b3c96 100644 --- a/dev_tools/pylibs/edgacknowledg/forge/workspace.py +++ b/dev_tools/pylibs/edgacknowledg/forge/workspace.py @@ -32,6 +32,7 @@ import secrets import shutil import stat +import struct import tarfile import urllib.parse import zipfile @@ -40,8 +41,12 @@ from pathlib import Path, PurePosixPath from typing import Iterable, Optional, Sequence +import edgtest +import edgutil + from edgacknowledg.forge import Forge, PullRequestInfo from edgacknowledg.forge.runner.roles import BUILD_STAGING_DIR, REVIEWS_DIR +from edgtest.runtest import get_bare_name _log = logging.getLogger(__name__) @@ -56,6 +61,25 @@ BENCH_BASELINE_PATTERNS = ('/benchmarks/baselines/',) +# The most sparse-checkout patterns a checkout may have: matching costs +# patterns x paths, and they are built from pull request content. Their +# total size must also stay within a small multiple of the diff's (see +# MAX_DIFF_BYTES): a test gets at most two patterns, each about as long as +# its path, and the only text that is part of many patterns, a suite's +# name, is short. +MAX_PATTERNS = 20_000 +# The largest diff (of tests/, in bytes) a pull request may have: it is +# held in memory, and copied and split into lines more than once. +MAX_DIFF_BYTES = 16 * 1024 * 1024 +# The most entries an artifact may have (its zip's, and its tarballs' +# together), and the largest its zip's central directory may be: zipfile +# reads all of that directory into memory, before we can count. +MAX_ARTIFACT_ENTRIES = 100_000 +MAX_CENTRAL_DIRECTORY_BYTES = 16 * 1024 * 1024 +# Seconds a git process being stopped has to clean up (e.g. remove its lock +# files) before it is killed; well within docker stop's default 10. +_TERMINATE_ALLOWANCE = 5.0 + class WorkspaceError(Exception): '''Raised when a workspace operation fails or input is unsafe.''' @@ -77,26 +101,21 @@ def _escape_pattern(path: str) -> str: return re.sub(r'([\\*?\[\]!#])', r'\\\1', path) -def _test_base_names(file_name: str) -> list[str]: - '''Return the candidate base names of a test file (for its '..rto' - recordings directory), e.g. 'a.b.sft.cpp' -> ['a.b.sft', 'a.b', 'a']. - ''' - parts = file_name.split('.') - return ['.'.join(parts[:count]) for count in range(len(parts) - 1, 0, -1)] - - def patterns_for_test(test_path: PurePosixPath) -> list[str]: '''Sparse-checkout patterns for one test (a repository-relative path such - as 'tests/tests/suite/dir/name.sft.cpp'): the test file and its recordings, - or the whole directory of a multi-file test. + as 'tests/tests/suite/dir/name.sft.cpp'): the test file and its recordings + (where the test runner keeps them, '..rto'), or the whole + directory of a multi-file test. At most two, each about as long as the + path, however it is named (see MAX_PATTERNS). ''' directory = _escape_pattern(str(test_path.parent)) if '.mft.' in test_path.name: return [f"/{directory}/"] - patterns = [f"/{_escape_pattern(str(test_path))}"] - for base_name in _test_base_names(test_path.name): - patterns.append(f"/{directory}/.{_escape_pattern(base_name)}.rto/") - return patterns + bare_name = _escape_pattern(get_bare_name(test_path)) + return [ + f"/{_escape_pattern(str(test_path))}", + f"/{directory}/.{bare_name}.rto/", + ] def _patterns_for_changed_test_file(path: PurePosixPath) -> list[str]: @@ -133,7 +152,7 @@ def changes_patterns(diff: str) -> list[str]: '''Sparse-checkout patterns for rematerializing ``diff`` (of tests/) on the base tree: the test home and every affected test. ''' - patterns = list(_TEST_HOME_PATTERNS) + patterns = dict.fromkeys(_TEST_HOME_PATTERNS) current_suite: Optional[str] = None for line in diff.splitlines(): match = _DIFF_HEADER.match(line) @@ -141,38 +160,48 @@ def changes_patterns(diff: str) -> list[str]: path = PurePosixPath(match.group(1)) current_suite = None if path.parts[:2] == ('tests', 'tests'): - patterns += _patterns_for_changed_test_file(path) + _add_patterns(patterns, _patterns_for_changed_test_file(path)) elif (path.parts[:2] == ('tests', 'expectations') and path.suffix == '.log'): - # Expectation lines name tests relative to the suite. - current_suite = path.stem + # Expectation lines name tests relative to the suite, so its name is + # part of the patterns of each (see MAX_PATTERNS). A file whose name + # no suite may have (e.g. too long) names no tests. + if edgtest.is_valid_suite_name(path.stem): + current_suite = path.stem continue if current_suite is None or line.startswith(('---', '+++')): continue if line.startswith(('-', '+')): test_name = line[1:].split(':')[0].strip() if test_name: - patterns += patterns_for_test( + _add_patterns(patterns, patterns_for_test( PurePosixPath('tests', 'tests', current_suite, test_name) - ) - return _unique(patterns) + )) + return list(patterns) def failures_patterns(test_ids: Iterable[str]) -> list[str]: '''Sparse-checkout patterns for serving CI results naming ``test_ids`` ('/', as in .elist files), plus bench baselines. ''' - patterns = [*_TEST_HOME_PATTERNS, *BENCH_BASELINE_PATTERNS] + patterns = dict.fromkeys([*_TEST_HOME_PATTERNS, *BENCH_BASELINE_PATTERNS]) for test_id in test_ids: test_path = PurePosixPath('tests', 'tests', test_id) if '..' in test_path.parts or test_path.is_absolute(): continue - patterns += patterns_for_test(test_path) - return _unique(patterns) + _add_patterns(patterns, patterns_for_test(test_path)) + return list(patterns) -def _unique(items: Iterable[str]) -> list[str]: - return list(dict.fromkeys(items)) +def _add_patterns(patterns: dict[str, None], more: list[str]) -> None: + '''Add ``more`` to ``patterns`` (an ordered set), within + ``MAX_PATTERNS``. + ''' + patterns.update(dict.fromkeys(more)) + if len(patterns) > MAX_PATTERNS: + raise WorkspaceError( + f"more than {MAX_PATTERNS} sparse-checkout patterns (too many tests)" + ) def ensure_real_directory(root: Path, relative: PurePosixPath) -> Path: @@ -262,23 +291,57 @@ def remove_tree(path: Path) -> None: path.unlink() -class _ByteBudget: - def __init__(self, limit: int) -> None: - self.remaining = limit +class _Budget: + '''What extracting an artifact may still spend: bytes, and entries.''' + def __init__(self, max_bytes: int) -> None: + self.bytes = max_bytes + self.entries = MAX_ARTIFACT_ENTRIES def spend(self, count: int) -> None: - self.remaining -= count - if self.remaining < 0: + self.bytes -= count + if self.bytes < 0: raise UnsafeArchiveError('artifact exceeds the extracted size limit') + def count_entry(self) -> None: + self.entries -= 1 + if self.entries < 0: + raise UnsafeArchiveError( + f"artifact has more than {MAX_ARTIFACT_ENTRIES} entries" + ) -def _check_inner_tarball(path: Path, budget: _ByteBudget) -> None: - '''Charge a (gzipped) tarball's decompressed size to ``budget``; reviewers' - requests decompress these, so they're bombs-in-waiting otherwise. + +def _central_directory_size(zip_path: Path) -> Optional[int]: + '''The size of ``zip_path``'s central directory, as its end records give + it (found as zipfile finds them); ``None`` if it has none. + ''' + # The end of central directory record (22 bytes, then a comment of up to + # 64 KiB), maybe preceded by a ZIP64 locator (20 bytes) and record (56). + with open(zip_path, 'rb') as handle: + size = handle.seek(0, os.SEEK_END) + start = max(0, size - (22 + 0xFFFF + 20 + 56)) + handle.seek(start) + tail = handle.read() + end = tail.rfind(b'PK\x05\x06', 0, len(tail) - 22 + 4) + if end < 0: + return None + directory_size: int = struct.unpack_from('= 56 and tail[locator:locator + 4] == b'PK\x06\x07': + record = locator - 56 + if tail[record:record + 4] == b'PK\x06\x06': + directory_size = struct.unpack_from(' None: + '''Charge a (gzipped) tarball's decompressed size and entries to + ``budget``; reviewers' requests decompress these, so they're + bombs-in-waiting otherwise. ''' try: with tarfile.open(path, 'r:*') as archive: for member in archive: + budget.count_entry() if not (member.isfile() or member.isdir()): raise UnsafeArchiveError(f"{path.name} contains a link or device") budget.spend(member.size) @@ -293,14 +356,22 @@ def extract_artifact(zip_path: Path, '''Safely extract a CI artifact zip into the (new, empty) ``destination``. Rejects absolute or escaping paths and symlinks, and stops once more than - ``max_bytes`` would be written (counting the decompressed contents of - inner tarballs too). + ``max_bytes`` would be written, or ``MAX_ARTIFACT_ENTRIES`` entries made + (counting inner tarballs' contents too). ''' - budget = _ByteBudget(max_bytes) + directory_size = _central_directory_size(zip_path) + if (directory_size is not None and + directory_size > MAX_CENTRAL_DIRECTORY_BYTES): + raise UnsafeArchiveError( + f"artifact's central directory is over " + f"{MAX_CENTRAL_DIRECTORY_BYTES} bytes" + ) + budget = _Budget(max_bytes) tarballs = [] try: with zipfile.ZipFile(zip_path) as archive: for info in archive.infolist(): + budget.count_entry() name = info.filename member_path = PurePosixPath(name) if (member_path.is_absolute() or '\\' in name or @@ -384,7 +455,11 @@ async def git(self, git_dir: Optional[Path] = None, cwd: Optional[Path] = None, stdin: Optional[str] = None, - check: bool = True) -> GitResult: + check: bool = True, + max_output: Optional[int] = None) -> GitResult: + '''Run git; with ``max_output``, its output may be at most that many + bytes (it is stopped, and ``WorkspaceError`` raised, beyond). + ''' command = ['git'] if git_dir is not None: command += ['--git-dir', str(git_dir)] @@ -400,9 +475,18 @@ async def git(self, cwd = cwd, env = self._git_env() ) - stdout, stderr = await process.communicate( - stdin.encode() if stdin is not None else None - ) + if max_output is None: + stdout, stderr = await process.communicate( + stdin.encode() if stdin is not None else None + ) + else: + assert stdin is None + bounded = await _communicate_bounded(process, max_output) + if bounded is None: + raise WorkspaceError( + f"git {args[0]}'s output is over {max_output} bytes" + ) + stdout, stderr = bounded assert process.returncode is not None result = GitResult( process.returncode, @@ -508,7 +592,8 @@ async def diff(self, result = await self.git( 'diff', '--no-renames', '--no-color', '--no-ext-diff', '--no-textconv', base, head, '--', *paths, - git_dir = self.clone_dir(repository) + git_dir = self.clone_dir(repository), + max_output = MAX_DIFF_BYTES ) return result.stdout @@ -594,6 +679,34 @@ async def collect_garbage(self, repository: str, open_count: int) -> None: await self.git('gc', '--auto', '--quiet', git_dir = clone_dir) +async def _communicate_bounded(process: asyncio.subprocess.Process, + limit: int) -> Optional[tuple[bytes, bytes]]: + '''``process``'s output (stdout, stderr), once it exits; ``None``, having + stopped it, should its stdout be over ``limit`` bytes. Should this be + cancelled (or fail), it is stopped too, rather than left writing to a + pipe no one reads. + ''' + assert process.stdout is not None and process.stderr is not None + errors = asyncio.create_task(process.stderr.read()) + chunks: list[bytes] = [] + size = 0 + try: + while chunk := await process.stdout.read(64 * 1024): + size += len(chunk) + if size > limit: + await edgutil.async_terminate_within(process, _TERMINATE_ALLOWANCE) + return None + chunks.append(chunk) + output = b''.join(chunks), await errors + await process.wait() + return output + except BaseException: + await edgutil.async_terminate_within(process, _TERMINATE_ALLOWANCE) + raise + finally: + errors.cancel() + + def basic_auth_header(user: str, password: str) -> str: '''An HTTP Basic 'Authorization' header (as git's http.extraHeader).''' token = base64.b64encode(f"{user}:{password}".encode()).decode() diff --git a/dev_tools/pylibs/edgifc/decoration/__init__.py b/dev_tools/pylibs/edgifc/decoration/__init__.py index 8b2afcf7b8..aa20c2d930 100644 --- a/dev_tools/pylibs/edgifc/decoration/__init__.py +++ b/dev_tools/pylibs/edgifc/decoration/__init__.py @@ -111,7 +111,7 @@ def name(self): def decorate(self, ifc_reader, value): try: return ifc_reader.get_decomposed_index_sort(self.sort_name, value)[0] - except: + except Exception: sort, _ = ifc_reader.get_decomposed_index_sort_values( self.sort_name, value ) @@ -127,7 +127,7 @@ def name(self): def decorate(self, ifc_reader, value): try: return ifc_reader.get_decomposed_index_sort(self.sort_name, value)[1] - except: + except Exception: _, idx = ifc_reader.get_decomposed_index_sort_values( self.sort_name, value ) diff --git a/dev_tools/pylibs/edgtest/__init__.py b/dev_tools/pylibs/edgtest/__init__.py index cf8e36c3a5..0b8ef00085 100644 --- a/dev_tools/pylibs/edgtest/__init__.py +++ b/dev_tools/pylibs/edgtest/__init__.py @@ -204,6 +204,15 @@ def load_test_suite_env(test_suite_dir: Path) -> Dict[str, str]: {'sft.cpp', 'sft.c', 'mft.cpp', 'mft.c'} ) +# What a test suite (a directory of the test home's tests/) may be named. +# Tools taking suite names from untrusted input (e.g. the AcknowlEDG Forge, +# from expectation file names) rely on it to bound them. +_SUITE_NAME_REGEX = re.compile(r'[A-Za-z0-9_-]{1,64}') + +def is_valid_suite_name(name: str) -> bool: + '''Return whether ``name`` may name a test suite.''' + return _SUITE_NAME_REGEX.fullmatch(name) is not None + def test_extensions_for_suite(test_home: Path, suite_name: str) -> Set[str]: '''Return a list of file extensions that should be considered when searching diff --git a/dev_tools/pylibs/edgtest/runtest/__init__.py b/dev_tools/pylibs/edgtest/runtest/__init__.py index 35ef6e7a3d..0308d4b651 100644 --- a/dev_tools/pylibs/edgtest/runtest/__init__.py +++ b/dev_tools/pylibs/edgtest/runtest/__init__.py @@ -25,7 +25,7 @@ from enum import IntEnum from collections import OrderedDict -from pathlib import Path +from pathlib import Path, PurePath from typing import ( Any, Callable, Dict, IO, Iterator, List, Mapping, NamedTuple, Optional, Pattern, Set, Tuple, cast @@ -83,7 +83,7 @@ def get_matches_possible_names(test_suite_env: Dict[str, str], results.append(f"default.{case_number}.matches.txt") return results -def get_bare_name(test_file_path: Path) -> str: +def get_bare_name(test_file_path: PurePath) -> str: '''Return the test name without the file extension.''' return test_file_path.name.split('.')[0] diff --git a/dev_tools/pylibs/edgutil/__init__.py b/dev_tools/pylibs/edgutil/__init__.py index a06c6a39a6..d070ccec9f 100644 --- a/dev_tools/pylibs/edgutil/__init__.py +++ b/dev_tools/pylibs/edgutil/__init__.py @@ -4,6 +4,7 @@ # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception import asyncio +import contextlib import difflib import functools import json @@ -15,9 +16,11 @@ import sys import tempfile import traceback +import weakref from datetime import datetime from pathlib import Path +from types import TracebackType from typing import ( Any, BinaryIO, @@ -29,6 +32,7 @@ Optional, Set, Tuple, + Type, cast ) @@ -180,7 +184,10 @@ def __enter__(self) -> 'CurrentWorkingDirectorySwap': os.chdir(self.dest_dir) return self - def __exit__(self, _type, _value, _tb): + def __exit__(self, + _type: Optional[Type[BaseException]], + _value: Optional[BaseException], + _tb: Optional[TracebackType]) -> None: os.chdir(self.original_cwd) def eprint(msg: str) -> None: @@ -406,12 +413,49 @@ def _active_asyncio_tasks( else: def _active_asyncio_tasks( loop: asyncio.AbstractEventLoop) -> Set[asyncio.Task]: - return asyncio.Task.all_tasks(loop) # type: ignore[attr-defined] + # Unlike asyncio.all_tasks, Task.all_tasks includes tasks that are done. + return { + task + for task in asyncio.Task.all_tasks(loop) # type: ignore[attr-defined] + if not task.done() + } + +# The tasks ``_stop_asyncio_loop`` cancelled, which ``_finish_asyncio_tasks`` +# doesn't cancel again: that would cut their cleanup short. +_stopped_asyncio_tasks: 'weakref.WeakSet[asyncio.Task]' = weakref.WeakSet() def _stop_asyncio_loop(loop: asyncio.AbstractEventLoop) -> None: + '''Cancel every task on ``loop``; again, should it be asked again (e.g. a + second SIGINT, to cut a stuck cleanup short). + ''' for task in _active_asyncio_tasks(loop): + _stopped_asyncio_tasks.add(task) task.cancel() +def _finish_asyncio_tasks(loop: asyncio.AbstractEventLoop) -> None: + '''Run ``loop`` until every task on it has finished, cancelling those + ``_stop_asyncio_loop`` hasn't (e.g. those started as others cleaned up), + as ``asyncio.run`` does; report what failed other than by being + cancelled. + ''' + tasks = _active_asyncio_tasks(loop) + while tasks: + for task in tasks: + if task not in _stopped_asyncio_tasks: + _stopped_asyncio_tasks.add(task) + task.cancel() + loop.run_until_complete( + asyncio.gather(*tasks, return_exceptions = True) + ) + for task in tasks: + if not task.cancelled() and task.exception() is not None: + loop.call_exception_handler({ + 'message': 'unhandled exception while stopping the loop', + 'exception': task.exception(), + 'task': task, + }) + tasks = _active_asyncio_tasks(loop) + @functools.lru_cache(maxsize = None) def lazy_init_event_loop() -> asyncio.AbstractEventLoop: '''Upon first execution, create a new event loop and install it as the main @@ -449,8 +493,10 @@ def run_asyncio_loop( the initial async function asyncio. If that task raises ``AsyncLoopFatalError``, remaining tasks are cancelled - the same way a SIGINT/SIGTERM would cancel them, those cancelled tasks are - awaited, and the error is re-raised to the caller. + the same way a SIGINT/SIGTERM would cancel them, and the error is + re-raised to the caller. Either way, once stopped, the loop runs until + every task has finished (cancelling any started meanwhile), so each + cancelled task's cleanup completes. Pass ``install_signal_handlers=False`` when the caller already registers process-level handlers and must not have them replaced by the event loop. @@ -466,9 +512,30 @@ async def run_task_consumer() -> Any: _stop_asyncio_loop(loop) raise - return loop.run_until_complete( - asyncio.shield(loop.create_task(run_task_consumer())) - ) + consumer = loop.create_task(run_task_consumer()) + try: + return loop.run_until_complete(consumer) + finally: + if consumer in _stopped_asyncio_tasks: + _finish_asyncio_tasks(loop) + +async def async_terminate_within(process: asyncio.subprocess.Process, + allowance: float) -> int: + '''Terminate ``process`` (SIGTERM, so it may clean up, e.g. remove its + lock files), killing it should it not exit within ``allowance`` seconds; + return its exit status once it has exited. A process that has already + exited is left alone. + ''' + if process.returncode is None: + # Raised should asyncio already be done with it (its transport closed). + with contextlib.suppress(ProcessLookupError): + process.terminate() + try: + await asyncio.wait_for(process.wait(), allowance) + except asyncio.TimeoutError: + with contextlib.suppress(ProcessLookupError): + process.kill() + return await process.wait() def handle_proc_cancel(proc: Optional[asyncio.subprocess.Process]) -> bool: '''Terminate the given process prematurely. @@ -493,7 +560,7 @@ def handle_proc_cancel(proc: Optional[asyncio.subprocess.Process]) -> bool: class AsyncJobLoop: def __init__(self, loop: asyncio.AbstractEventLoop, *, max_jobs: int, - max_update_interval = 1) -> None: + max_update_interval: float = 1) -> None: self.loop = loop self.max_jobs = max_jobs self.active_tasks: List[asyncio.Task] = [] @@ -612,7 +679,7 @@ def cleanup_status_line(self) -> None: self._print_status_line() sys.stdout.write('\n') - def print(self, string: str, end = '\n') -> None: + def print(self, string: str, end: str = '\n') -> None: # Setup to overwrite the status line if one is present. This is # particularly important for piped stdout as we don't want to pollute the # output stream with a bunch of nonsensical return characters. diff --git a/dev_tools/services/acknowledg-forge/README.md b/dev_tools/services/acknowledg-forge/README.md index cf5f3bef4d..02f3e6e112 100644 --- a/dev_tools/services/acknowledg-forge/README.md +++ b/dev_tools/services/acknowledg-forge/README.md @@ -103,7 +103,8 @@ from the environment variable shown as a fallback. | `ACKNOWLEDG_FORGE_BASE_BRANCHES` | `main` | Comma-separated target branches whose pull requests are served | | `ACKNOWLEDG_FORGE_API_URL` | github.com / gitlab.com | Forge API URL, e.g. `https://ghe.example/api/v3` or `https://gitlab.example/api/v4` | | `ACKNOWLEDG_FORGE_POLL_INTERVAL` | `300` | Seconds between polls; `0` polls only at startup | -| `ACKNOWLEDG_FORGE_WEBHOOK_BIND` | (unset) | `host:port` to receive webhooks on (in `compose.yaml`: `service-frontend:2728`); unset runs no web server at all | +| `ACKNOWLEDG_FORGE_SERVICE_BIND` | (unset; `compose.yaml`: `service-frontend:2728`) | `host:port` to serve the service's web app on, once the service has started: health checks (`GET /healthz`, for the container's healthcheck), and with webhooks, `POST /webhook`. Unset serves nothing | +| `ACKNOWLEDG_FORGE_WEBHOOKS` | `0` | `1` receives webhooks on the service bind (needs the webhook secret) | | `ACKNOWLEDG_FORGE_CI_WORKFLOW` | (unset) | CI to track: the GitHub workflow file (e.g. `build-and-test-ci.yml`), or the GitLab pipeline name (`*` for any). Unset disables CI tracking (and the Failures and Bench sections) | | `ACKNOWLEDG_FORGE_TEST_ARTIFACT` | `test-results-all` | Test results artifact (GitLab: job) name | | `ACKNOWLEDG_FORGE_BENCH_ARTIFACT` | `bench-results` | Benchmark results artifact (GitLab: job) name | @@ -136,7 +137,8 @@ from the environment variable shown as a fallback. individual events* and clear *Pushes*, which the service ignores; a push to a pull request arrives as a *Pull requests* event). Point it at `https:///webhook`, typically behind a reverse proxy to - `ACKNOWLEDG_FORGE_WEBHOOK_BIND` (published on the host's `127.0.0.1`). + `ACKNOWLEDG_FORGE_SERVICE_BIND` (published on the host's `127.0.0.1`; + proxy only `/webhook`). - **CI:** set `ACKNOWLEDG_FORGE_CI_WORKFLOW=build-and-test-ci.yml`. Its `publish-test-results` job uploads `test-results-all`, including the `ci-meta.json` that records the tested (merge) commit. Benchmark results @@ -277,7 +279,7 @@ URLs is configured here, by the host's administrator. As root: The webhook secret (the same value as in the forge's webhook settings; for example, generate one with `openssl rand -hex 32`). Webhooks are optional: to only poll, skip this, and remove the unit's `webhook-secret` line and - the settings' `ACKNOWLEDG_FORGE_WEBHOOK_BIND` (step 5), or the service + the settings' `ACKNOWLEDG_FORGE_WEBHOOKS` (step 5), or the service refuses to start: ``` @@ -349,7 +351,7 @@ URLs is configured here, by the host's administrator. As root: ACKNOWLEDG_FORGE_CI_WORKFLOW=build-and-test-ci.yml ACKNOWLEDG_FORGE_COMMENTS=1 # Webhooks (with the webhook secret, step 4); remove to only poll. - ACKNOWLEDG_FORGE_WEBHOOK_BIND=service-frontend:2728 + ACKNOWLEDG_FORGE_WEBHOOKS=1 ``` 6. Optionally, use PostgreSQL instead of the default SQLite database (which diff --git a/dev_tools/services/acknowledg-forge/SANDBOXING.md b/dev_tools/services/acknowledg-forge/SANDBOXING.md index 979fa2a4c0..d83987df62 100644 --- a/dev_tools/services/acknowledg-forge/SANDBOXING.md +++ b/dev_tools/services/acknowledg-forge/SANDBOXING.md @@ -68,7 +68,7 @@ flowchart TB end subgraph publisher [Container: publisher] - publisherserver[publisher server:
a publisher per connection] -->|each request,
round-robin| poolprocess[pool processes:
the stock handlers] + publisherserver[publisher server:
one publisher, for every connection] -->|each request, to the
next free process| poolprocess[pool processes:
the stock handlers] poolprocess --> toolsclient[tools runner client] end @@ -93,7 +93,7 @@ flowchart TB rematerialize -.->|reads, read-only| reviews rematerialize -.->|writes the rebuilt run| staging processor -.->|checks the run,
renames it into place| staging - processor -->|sync, remove| publisherclient + processor -->|review changed| publisherclient processor -->|after each sweep| announcer announcer -->|comments with section links| forge publisherclient <-->|network: publish| publisherserver @@ -121,9 +121,16 @@ edgacknowledg.forge.`), what listens in it is a `…Server` | Container | Runs | Its server | Its client, in | | --- | --- | --- | --- | | `service` | `acknowledg-forge-github` (`edgacknowledg.forge.service`, `processor`, …) | (webhooks only) | — | -| `builder` | `edgacknowledg.forge.builder` | `RunnerServer` with the builder role (`runner.server`, `runner.roles`) | `RunnerClient` (`runner.client`), in the service, through `Sandbox.run` | -| `publisher` | `edgacknowledg.forge.publisher` | `PublisherServer` (`publisher.server`), hosting an `AcknowledgPublisher` (`publisher.acknowledg`) per connection, and the `ProcessPool` (`publisher.pool`) of pool processes (`publisher.pool_process`) answering its reviewers | `PublisherClient` (`publisher.client`), in the service | -| `tools` | `edgacknowledg.forge.tools` | `RunnerServer` with the tools role | `RunnerClient`, in each pool process, through `Sandbox.context_for` | +| `builder` | `edgacknowledg.forge.builder` | the runner server (`create_runner` in `runner.server`) with the builder role (`runner.roles`) | `RunnerClient` (`runner.client`), in the service, through `Sandbox.run` | +| `publisher` | `edgacknowledg.forge.publisher` | the publisher server (`create_publisher` in `publisher.server`), publishing what every connection queues (`publish` in `publisher.acknowledg`), and the `ProcessPool` (`publisher.pool`) of pool processes (`publisher.pool_process`) answering its reviewers | `PublisherClient` (`publisher.client`), in the service | +| `tools` | `edgacknowledg.forge.tools` | the runner server with the tools role | `RunnerClient`, in each pool process, through `Sandbox.context_for` | + +Each client has a `ForgeSandboxClient` (`forge.sandbox_client`), and +speaks its protocol over each connection that makes. It keeps one +connection to its server, and never gives up on it: the websockets +library reconnects whenever it goes (after a second, so a connection that +keeps going doesn't spin), or can't be made (backing off exponentially, +up to 90 s). The builder and tools containers run the same server, the runner, with a role each. `publisher.protocol` is the publisher's messages; `publisher` @@ -152,7 +159,7 @@ need no translation. | --- | --- | --- | --- | --- | --- | | `service` | the service (`acknowledg-forge-github` or `-gitlab`) | the whole state directory | the whole state directory | `frontend` (the forge; webhooks from the host), `build`, `publish` | the credentials | | `builder` | a runner server (builder role): `edg-rematerialize-test-run` only | `reviews/` | `build-staging/` only | `build` (internal) | nothing | -| `publisher` | the publisher server: `AcknowledgPublisher`, and pool processes running the stock handlers | `reviews/` | nothing | `publish` (internal), `tools` (internal), `egress` (AcknowlEDG) | the API key, in the server's memory | +| `publisher` | the publisher server: `publish`, and pool processes running the stock handlers | `reviews/` | nothing | `publish` (internal), `tools` (internal), `egress` (AcknowlEDG) | the API key, in the server's memory | | `tools` | a runner server (tools role): `ifc-dump` and `cg_annotate` only | `reviews/` | nothing | `tools` (internal) | nothing | - **Networks.** Each pair of containers that talk shares a network of its @@ -160,8 +167,9 @@ need no translation. runner, nor they it. Internal networks have no route out, so the builder and tools runner can reach nothing but their one client. A listener on a container with several networks binds only on its pair's (the publisher - server on `publish`; the webhook receiver, with - `ACKNOWLEDG_FORGE_WEBHOOK_BIND=service-frontend:2728`, on `frontend`). + server on `publish`; the service's web app (health checks and webhooks), + with `ACKNOWLEDG_FORGE_SERVICE_BIND=service-frontend:2728`, on + `frontend`). That is what keeps each server to its one client: the connections carry no credentials of their own, so `tests/test_containers.py` checks every pairing, and a change to the networks in `compose.yaml` that breaks one @@ -196,8 +204,13 @@ carry, so commands run concurrently over it. For each, the runner: The service has one runner client, for its builds. Each pool process has one too, for the commands its handlers run: they run every command through their `SandboxContext` (`run_command`), whose command runner is that -client. A client connects when first used, and again when its connection -goes, which fails the commands in flight on it. +client. A client keeps one connection to its runner (with its +`ForgeSandboxClient`), which all its commands share (each request carries +an id its answer and output come back with). A command waits for the +connection, however long it takes to come back, and the runner keeps its +time; the connection going fails the commands in flight on it. A message +the client can't read has the connection go, as no later one can be +trusted. ### Trade-offs @@ -227,23 +240,22 @@ hands them to the publisher to serve. ```mermaid flowchart TB - start([start]) --> connected1[wait until the publisher
is connected to AcknowlEDG] - connected1 --> restore[restore: serve the stored sections again,
or mark their pull requests dirty if their files are gone] - restore --> connected2[wait until connected] - connected2 --> due[start processing every dirty pull request,
a few at a time] + start([start]) --> restore[restore: drop builds
the restart interrupted] + restore --> due[start processing every dirty pull request,
a few at a time] due --> maintain{6 h since the
last maintenance?} maintain -->|yes| gc[git housekeeping] --> sleep maintain -->|no| sleep[wait to be woken,
or 60 s] - sleep --> connected2 + sleep --> due due -.-> process subgraph process [Processing one pull request] open{open, against a
served branch?} - open -->|no| teardown[remove its review,
delete its worktrees] + open -->|no| teardown[stop serving its review,
delete its worktrees] + teardown -->|unavailable| defer[stays dirty:
retried on the next sweep] open -->|yes| fetchbuild[fetch it, build its sections] - fetchbuild --> sync[sync the sections] - sync -->|ok| record[record the sections,
delete superseded runs] - sync -->|unavailable| defer[stays dirty:
retried once connected] + fetchbuild --> record[record the sections,
delete superseded runs] + record --> sync[sync the sections] + sync -->|unavailable| later[served on connecting] fetchbuild -->|error| retry[retried with backoff:
1 min up to 1 h,
at most 8 times] sync -->|error| retry end @@ -273,14 +285,16 @@ sequenceDiagram B-->>S: output, exit status Note over S: check_plain_tree(staging),
no hard links either Note over S: rename staging into the worktree + Note over S: record the sections,
delete superseded run directories S->>P: sync(review, sections) P-->>S: ok - Note over S: record the sections,
delete superseded run directories ``` -A failed check fails the section rather than serving it. Superseded run -directories are deleted only after the publisher confirms it no longer -serves them. Staging directories a restart interrupted are deleted at startup. +A failed check fails the section rather than serving it. Once the new +sections are recorded, nothing needs the superseded run directories, so +they are deleted before the publisher is told; until it catches up, a +reviewer's request for one gets the handler's failure reply. Staging +directories a restart interrupted are deleted at startup. ## Serving reviewers @@ -330,20 +344,25 @@ through the service. ## The publisher -The publisher container runs the publisher server (`PublisherServer`), -which serves reviewers with `AcknowledgPublisher`, acknowledg-forge's -AcknowlEDG client. It holds one batch connection to AcknowlEDG and manages -each review's sections; the stock providers from `edgacknowledg.client` +The publisher container runs the publisher server (`create_publisher`), +which serves reviewers with `publish`, acknowledg-forge's AcknowlEDG +client. It holds one batch session with AcknowlEDG (the client library's +`BatchClient`, which keeps it connected) and manages each review's +sections; the stock providers from `edgacknowledg.client` (`TestSourceHandler`, `BenchReviewSession`), the same handlers that `edgy-review` and `edg-bench-review` use, answer reviewers' requests in its process pool. The service drives it through the publisher client (`PublisherClient`), which implements the same `Publisher` interface, so the rest of the service doesn't know the publisher is elsewhere. -Each connection from the client gets an `AcknowledgPublisher` of its own, -made from the settings the client sends first; when the connection goes, so -does that publisher, with everything it served. A new connection replaces -the current one. +Connections only queue what their client asks to publish (`Wanted`, in +the order asked), and answer; one publishing task (`publish`, +with the settings a client sends first) works through the queue, whether +or not AcknowlEDG is reachable just then. What it serves is what the +service's database stores: the processor stores a review's sections, then +tells the publisher client the review changed, and the client sends what is +stored; on every connection (the publisher may have restarted), it sends +every stored review. ### The process pool @@ -356,52 +375,50 @@ which its event loop never waits on for more than a request's deadline: for the section's handler: it passes each request, with the section it is for, to the pool, and relays the replies. - The pool keeps a fixed number of pool processes (`--processes`, 4), each - with one end of a socket pair as its channel, and sends each request to - the next one in turn (round-robin). A process answers one request at a - time, building the section's handler for it and dropping it afterwards; - requests for a busy process wait their turn. + with one end of a socket pair as its channel. Requests queue, in order; + each process has a dispatcher that gives it the next one only once it + has answered the last, so a process answers one request at a time, + building the section's handler for it and dropping it afterwards, and a + slow request holds up only its own process. Each request carries an id + its answer carries back; a request abandoned before its turn (as when + its section is removed) is skipped, and one abandoned while answered + still keeps its process busy until then, its answer dropped. - A request has a deadline (`--request-timeout`, 150 s, longer than the - tools runner's 2 min), counted from when its process starts on it. A + tools runner's 2 min), counted from when its process is given it. A process that misses it is killed, with whatever it started, and replaced; so is one that exits. Either way that request fails, and the client library answers the reviewer with its failure reply; a request for a file outside the section still gets the reviewer dropped. - A process that exits before it is ready has failed to start: the next - try waits, doubling from 1 s up to 60 s, and until then requests for it - fail at once. The server doesn't start unless every process does. + try waits, doubling from 1 s up to 60 s; meanwhile the other processes + take the requests, and while none can, requests fail at once. The server doesn't start unless every process does. The client library answers one request at a time per section, so one section's requests occupy at most one process at a time. ### Reconnect loop -The publisher client keeps one connection up, and makes sure the next -connection's publisher serves whatever the last one did: +The publisher client keeps one connection up, and on each one has the +publisher serve exactly what the service's database stores as served: ```mermaid flowchart TB - spawn[connect to the publisher server;
it makes a publisher
for the connection] + spawn[connect to the publisher server] spawn --> init[send the settings,
with the API key] - init --> read[read the server's messages] - read -->|connected| replay{replay needed?} - replay -->|yes| resync[sync every review's
desired sections again] - resync --> ready - replay -->|no| ready[accept requests] - ready --> read - read -->|disconnected| notready[refuse requests:
unavailable] - notready --> read - read -->|request unanswered
for 15 min| kill[drop the connection,
and its publisher;
send it nothing more] + init --> serve[sync every open pull request's
stored sections; stop serving
those it can't, to be rebuilt] + serve --> prune[prune: stop serving
every other review] + prune --> ready[accept requests] + ready -->|request unanswered
for 15 min| kill[drop the connection;
send it nothing more] kill --> exited - read -->|connection closed| exited[fail the request in flight
as unavailable;
replay needed] - exited --> backoff[wait 1 s, doubling
up to 60 s; back to 1 s
after a long run] + ready -->|connection closed| exited[fail the request in flight
as unavailable] + exited --> backoff[wait 1 s, then
back off while it
can't connect, up to 90 s] backoff --> spawn ``` -At startup, the service checks that it can reach the builder -(`Sandbox.check`), and refuses to start otherwise. Nothing checks the tools -runner up front: only requests that run a command (`ifc-dump`, -`cg_annotate`) need it, so while it can't be reached, those fail, each with -the client library's failure reply, and everything else is still served. +At startup, the service waits until it can reach the builder +(`Sandbox.check`). Nothing checks the tools runner up front: only requests +that run a command (`ifc-dump`, `cg_annotate`) need it, so while it can't +be reached, those wait for it, and everything else is still served. Without containers (`ACKNOWLEDG_FORGE_UNSANDBOXED=1`, for local development only), all of this is the same: the service runs the publisher @@ -415,8 +432,10 @@ message (`publisher.protocol`). #### Requests -After the settings, the service sends a request each time a review's -sections change, and waits for its answer: +After the settings, the service serves every review its database stores as +served, and has the publisher stop serving every other one; then it sends +a request each time a review's stored sections change, and waits for its +answer: ```mermaid sequenceDiagram @@ -424,16 +443,20 @@ sequenceDiagram participant P as Publisher server S->>P: settings, including the AcknowlEDG API key - loop each time a review's sections change - alt the review serves sections + loop every review it serves + S->>P: {"op": "sync", "review": ..., "sections": [...]} + P-->>S: {"ok": true} + end + S->>P: {"op": "prune", "keep": [review ids]} + P-->>S: {"ok": true} + loop each time a review's sections change, or it is torn down + alt its stored sections changed S->>P: {"op": "sync", "review": ..., "sections": [...]} - else the review is gone - S->>P: {"op": "remove", "review_id": ...} + else torn down (its files about to go) + S->>P: {"op": "delist", "review_id": ...} end - alt served + alt queued P-->>S: {"ok": true} - else not connected to AcknowlEDG - P-->>S: {"error": "unavailable", "message": ...} else failed P-->>S: {"error": "failed", "message": ...} end @@ -444,28 +467,27 @@ sequenceDiagram environment, where `/proc` would show it, nor a file in the publisher container. - Requests go one at a time, so each answer belongs to the request in - flight. `unavailable` defers the pull request until the connection is - back; `failed` fails it. - -#### Connection events - -Separately, and only when its AcknowlEDG connection changes, the -connection's publisher reports it, at any time between answers: - -```mermaid -sequenceDiagram - participant S as Service
(publisher client) - participant P as Publisher server - - opt the publisher connects to AcknowlEDG - P-->>S: {"event": "connected"} - Note over S: replay sections if needed,
then accept requests - end - opt the publisher loses AcknowlEDG - P-->>S: {"event": "disconnected"} - Note over S: refuse requests as unavailable
until it reconnects - end -``` + flight. `failed` fails the pull request. If the publisher server can't + be reached, a `sync` is left to the next connection, which serves what + is stored anyway; a `delist` defers the pull request's teardown, which + is retried on the next sweep. +- The server queues a request whether or not it is connected to + AcknowlEDG: the client library serves what it publishes once connected + (again). Sections it no longer serves, it withdraws as soon as the + publishing task reaches the update. +- The service's database leads: the processor stores a review's sections, + deletes the trees of the sections they replace (nothing needs them + then), and only then tells the publisher; should the publisher not take + it, the pull request is processed again, and the next connection serves + what is stored anyway. Until the publisher catches up, a reviewer's + request for a deleted tree is answered with the handler's failure + reply. Tearing a review down goes the other way: the publisher is told + to stop serving it (`delist`) while it is still stored, then its files + go, and only then what is stored. +- What the publisher serves outlasts the connection, so on connecting, + after serving every review it stores, the service has it stop serving + every other one (`prune`): nothing an earlier service served, and this + one doesn't, stays published. ## Symlinks and links diff --git a/dev_tools/services/acknowledg-forge/compose.yaml b/dev_tools/services/acknowledg-forge/compose.yaml index 75592e7436..806e5af7a9 100644 --- a/dev_tools/services/acknowledg-forge/compose.yaml +++ b/dev_tools/services/acknowledg-forge/compose.yaml @@ -59,6 +59,23 @@ x-environment: &environment x-python: &python /edg/acknowledg-forge/${FORGE:-github}/bin/python +# Every container's healthcheck: a GET of its /healthz +# (edgacknowledg.forge.serving). Every server is a Quart app, and answers it +# beside its WebSocket at /, so a probe never opens a connection. +# Dependents wait until healthy, so a client's first connection finds its +# server listening. (A runner's clean-up may kill a probe; the retries +# cover it.) +x-health-probe: &health_probe |- + import sys, urllib.request + urllib.request.urlopen(sys.argv[1], timeout = 3).close() + +x-healthcheck: &healthcheck + interval: 30s + timeout: 5s + retries: 3 + start_period: 60s + start_interval: 1s + services: service: <<: *container @@ -71,6 +88,9 @@ services: CREDENTIALS_DIRECTORY: /run/credentials ACKNOWLEDG_FORGE_BUILDER_SERVICE_URL: ws://builder:2730 ACKNOWLEDG_FORGE_PUBLISHER_SERVICE_URL: ws://publisher:2731 + # The web app: health checks, and webhooks with + # ACKNOWLEDG_FORGE_WEBHOOKS=1. Only on frontend (see networks). + ACKNOWLEDG_FORGE_SERVICE_BIND: service-frontend:2728 EDG_ACKNOWLEDG_API_HOST: ${EDG_ACKNOWLEDG_API_HOST:-localhost} EDG_ACKNOWLEDG_API_PORT: ${EDG_ACKNOWLEDG_API_PORT:-2727} EDG_ACKNOWLEDG_UI_HOST: ${EDG_ACKNOWLEDG_UI_HOST:-localhost} @@ -95,13 +115,23 @@ services: # is published. - 127.0.0.1:${WEBHOOK_PORT:-2728}:2728 networks: - # For webhooks, bind only here: ACKNOWLEDG_FORGE_WEBHOOK_BIND= - # service-frontend:2728. + # The web app binds only here (ACKNOWLEDG_FORGE_SERVICE_BIND). frontend: aliases: [service-frontend] build: {} publish: {} - depends_on: [builder, publisher] + # Healthy once everything has started. + healthcheck: + <<: *healthcheck + test: + - CMD + - *python + - -c + - *health_probe + - http://service-frontend:2728/healthz + depends_on: + builder: {condition: service_healthy} + publisher: {condition: service_healthy} mem_limit: 2g builder: @@ -131,6 +161,9 @@ services: target: /var/lib/acknowledg-forge/build-staging bind: {create_host_path: false} networks: [build] + healthcheck: + <<: *healthcheck + test: [CMD, *python, -c, *health_probe, "http://127.0.0.1:2730/healthz"] mem_limit: 2g publisher: @@ -161,7 +194,17 @@ services: aliases: [publisher-publish] tools: {} egress: {} - depends_on: [tools] + # Listening only once its pool has started. + healthcheck: + <<: *healthcheck + test: + - CMD + - *python + - -c + - *health_probe + - http://publisher-publish:2731/healthz + depends_on: + tools: {condition: service_healthy} # The server and its pool processes. mem_limit: 2g @@ -187,6 +230,9 @@ services: read_only: true bind: {create_host_path: false} networks: [tools] + healthcheck: + <<: *healthcheck + test: [CMD, *python, -c, *health_probe, "http://127.0.0.1:2732/healthz"] networks: # The service's: the forge, and webhooks from the host. diff --git a/dev_tools/services/acknowledg-forge/run-dev-oss-repo.sh b/dev_tools/services/acknowledg-forge/run-dev-oss-repo.sh index d076f4e666..e6325ad103 100755 --- a/dev_tools/services/acknowledg-forge/run-dev-oss-repo.sh +++ b/dev_tools/services/acknowledg-forge/run-dev-oss-repo.sh @@ -48,7 +48,8 @@ export ACKNOWLEDG_FORGE_CI_WORKFLOW='build-and-test-ci.yml' export ACKNOWLEDG_FORGE_POLL_INTERVAL="${ACKNOWLEDG_FORGE_POLL_INTERVAL:-60}" export ACKNOWLEDG_FORGE_UNSANDBOXED=1 export ACKNOWLEDG_FORGE_COMMENTS=0 -unset ACKNOWLEDG_FORGE_API_URL ACKNOWLEDG_FORGE_WEBHOOK_BIND +unset ACKNOWLEDG_FORGE_API_URL ACKNOWLEDG_FORGE_SERVICE_BIND +unset ACKNOWLEDG_FORGE_WEBHOOKS unset ACKNOWLEDG_FORGE_DATABASE_URL export EDG_ACKNOWLEDG_API_HOST='localhost' diff --git a/dev_tools/services/acknowledg-forge/tests/conftest.py b/dev_tools/services/acknowledg-forge/tests/conftest.py index a2505216d1..af2cbaac91 100644 --- a/dev_tools/services/acknowledg-forge/tests/conftest.py +++ b/dev_tools/services/acknowledg-forge/tests/conftest.py @@ -233,6 +233,10 @@ def make_tools_dir(path: Path) -> Path: # The builder runner the local_builder fixture serves for the test, which # make_config points configurations at. _BUILDER_SERVICE_URL = 'ACKNOWLEDG_FORGE_BUILDER_SERVICE_URL' +# The test's other clients of that runner (each Harness's), which +# local_builder closes before it stops the runner: the runner's shutdown +# waits for their connections. +_builder_clients: list[RunnerClient] = [] @pytest_asyncio.fixture(autouse = True) @@ -249,7 +253,11 @@ async def local_builder(tmp_path: Path, home = tmp_path / 'home', tools_dir = make_tools_dir(tmp_path / 'tools')) as client: monkeypatch.setenv(_BUILDER_SERVICE_URL, client.url) - yield client + try: + yield client + finally: + while len(_builder_clients) != 0: + await _builder_clients.pop().close() def make_config(home: Path, **overrides: str) -> ForgeServiceConfig: @@ -283,14 +291,16 @@ def __init__(self, self.forge = forge self.config = config self.clock = clock - self.publisher = publisher or FakePublisher() + self.publisher = publisher or FakePublisher(store, forge.name) self.commenter = commenter or FakeCommenter() self.wake = asyncio.Event() config.home.mkdir(parents = True, exist_ok = True) - self.sandbox = Sandbox( - builder = (RunnerClient(config.builder_service_url) - if config.builder_service_url is not None else None) - ) + builder = None + if config.builder_service_url is not None: + builder = RunnerClient(config.builder_service_url) + builder.start() + _builder_clients.append(builder) + self.sandbox = Sandbox(builder = builder) self.workspaces = Workspaces(config.home, forge) self.ingestor = Ingestor( store, @@ -321,6 +331,7 @@ def __init__(self, async def start(self) -> None: await self.announcer.start() + await self.publisher.sync_all() await self.processor.restore() async def step(self) -> None: diff --git a/dev_tools/services/acknowledg-forge/tests/fakes.py b/dev_tools/services/acknowledg-forge/tests/fakes.py index 97df9f397c..a755057713 100644 --- a/dev_tools/services/acknowledg-forge/tests/fakes.py +++ b/dev_tools/services/acknowledg-forge/tests/fakes.py @@ -20,11 +20,12 @@ PullRequestInfo, PullRequestState, ) -from edgacknowledg.forge.db import SectionRecord +from edgacknowledg.forge.db import ForgeStore, SectionRecord from edgacknowledg.forge.publisher import ( - PublisherUnavailableError, ReviewIdentity, + review_identity, ) +from edgacknowledg.forge.sandbox_client import ServiceUnavailableError REPOSITORY = 'edg/compiler' @@ -103,22 +104,54 @@ async def aclose(self) -> None: class FakePublisher: - '''Records what would be served.''' + '''Records what would be served: what ``store`` stores for + ``forge_name``. + ''' - def __init__(self) -> None: + def __init__(self, store: ForgeStore, forge_name: str) -> None: + self._store = store + self._forge_name = forge_name self.reviews: dict[uuid.UUID, list[SectionRecord]] = {} self.names: dict[uuid.UUID, RichName] = {} self.available = True self.sync_count = 0 - async def wait_connected(self) -> None: - pass + async def sync_one(self, review_id: uuid.UUID) -> None: + if not self.available: + # Served on connecting (sync_all), as by ``PublisherClient``. + return + record = await self._store.find_pull_request_by_review(review_id) + assert record is not None + await self.sync(review_identity(record), + await self._store.sections(record.id)) + + async def delist(self, review_id: uuid.UUID) -> None: + if not self.available: + raise ServiceUnavailableError('fake outage') + self.reviews.pop(review_id, None) + + async def sync_all(self) -> None: + '''Serve what is stored, and nothing else, as ``PublisherClient`` does + on connecting. + ''' + served = set() + for record in await self._store.open_pull_requests(self._forge_name): + sections = await self._store.sections(record.id) + if len(sections) == 0: + continue + if all(section.root.is_dir() for section in sections): + await self.sync(review_identity(record), sections) + served.add(record.review_id) + else: + await self._store.mark_dirty(record.id) + for review_id in set(self.reviews) - served: + del self.reviews[review_id] async def sync(self, review: ReviewIdentity, sections: Sequence[SectionRecord]) -> None: if not self.available: - raise PublisherUnavailableError('fake outage') + raise ServiceUnavailableError('fake outage') self.sync_count += 1 for section in sections: assert section.root.is_dir(), section @@ -128,9 +161,6 @@ async def sync(self, self.reviews[review.review_id] = list(sections) self.names[review.review_id] = review.name - async def remove(self, review_id: uuid.UUID) -> None: - self.reviews.pop(review_id, None) - def section_url(self, review_id: uuid.UUID, section: SectionRecord) -> Optional[str]: diff --git a/dev_tools/services/acknowledg-forge/tests/test_acknowledg_integration.py b/dev_tools/services/acknowledg-forge/tests/test_acknowledg_integration.py index 873b3897ea..1674a88510 100644 --- a/dev_tools/services/acknowledg-forge/tests/test_acknowledg_integration.py +++ b/dev_tools/services/acknowledg-forge/tests/test_acknowledg_integration.py @@ -3,7 +3,9 @@ # See https://edgcpp.org/LICENSE.txt for license information. # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception -'''AcknowledgPublisher against a real AcknowlEDG server.''' +'''Publishing (``publisher.acknowledg``) against a real AcknowlEDG +server. +''' import asyncio import json @@ -31,8 +33,12 @@ rich_text, ) from edgacknowledg.forge.db import SectionRecord -from edgacknowledg.forge.publisher import KIND_TEST_CHANGES, ReviewIdentity -from edgacknowledg.forge.publisher.acknowledg import AcknowledgPublisher +from edgacknowledg.forge.publisher import ( + KIND_TEST_CHANGES, + ReviewIdentity, + section_url, +) +from edgacknowledg.forge.publisher.acknowledg import Sync, Wanted, publish from edgacknowledg.forge.publisher.pool import ProcessPool # services/acknowledg/server, relative to services/acknowledg-forge/tests. @@ -167,26 +173,24 @@ async def test_publish_heal_and_survive_bad_requests( # No command runs here, so the tools runner is never reached. pool = ProcessPool(tools_service_url = unreachable_url, processes = 2) await pool.start() - publisher = AcknowledgPublisher( + wanted: Wanted = asyncio.Queue() + task = asyncio.create_task(publish( + wanted, api_host = '127.0.0.1', api_port = server.port, api_key = None, - ui_host = 'ui.example', - ui_port = 443, pool = pool - ) - task = asyncio.create_task(publisher.run()) + )) try: - await asyncio.wait_for(publisher.wait_connected(), 30) name = [ rich_link('https://forge.example/edg/compiler/pull/1', 'edg/compiler#1'), rich_text(': test'), ] review = ReviewIdentity(uuid.uuid4(), name, '') - await publisher.sync(review, [section]) + wanted.put_nowait(Sync(review, [section])) await _wait_for_sources(server.port, review.review_id, ['test-changes']) assert await _names(server.port, review.review_id) == (name, name) - assert publisher.section_url(review.review_id, section) == ( + assert section_url('ui.example', 443, review.review_id, section) == ( f"https://ui.example/review/{review.review_id}/tests/test-changes" ) @@ -261,7 +265,7 @@ async def test_publish_heal_and_survive_bad_requests( await server.start() await _wait_for_sources(server.port, review.review_id, ['test-changes']) - await publisher.sync(review, []) + wanted.put_nowait(Sync(review, [])) await _wait_for_sources(server.port, review.review_id, []) finally: task.cancel() @@ -269,6 +273,101 @@ async def test_publish_heal_and_survive_bad_requests( await pool.close() +async def test_what_is_queued_while_away_is_published_once_back( + server: Server, + section: SectionRecord, + unreachable_url: str + ) -> None: + pool = ProcessPool(tools_service_url = unreachable_url, processes = 1) + await pool.start() + wanted: Wanted = asyncio.Queue() + server.stop() + task = asyncio.create_task(publish( + wanted, + api_host = '127.0.0.1', + api_port = server.port, + api_key = None, + pool = pool + )) + try: + # Queued before AcknowlEDG was ever reached... + first = ReviewIdentity(uuid.uuid4(), [rich_text('edg/compiler#2')], '') + wanted.put_nowait(Sync(first, [section])) + await server.start() + await _wait_for_sources(server.port, first.review_id, ['test-changes']) + + # ...and while it was away again: the client library serves both once + # it is back. + server.stop() + second = ReviewIdentity(uuid.uuid4(), [rich_text('edg/compiler#3')], '') + wanted.put_nowait(Sync(second, [section])) + await asyncio.sleep(0.5) + await server.start() + await _wait_for_sources(server.port, first.review_id, ['test-changes']) + await _wait_for_sources(server.port, second.review_id, ['test-changes']) + finally: + task.cancel() + await asyncio.gather(task, return_exceptions = True) + await pool.close() + + +async def test_a_failed_request_is_reported_to_its_reviewer( + server: Server, + section: SectionRecord, + unreachable_url: str + ) -> None: + pool = ProcessPool(tools_service_url = unreachable_url, processes = 1) + await pool.start() + answered = 0 + answer = pool.answer + + async def counting(*args: Any) -> Any: + nonlocal answered + answered += 1 + return await answer(*args) + + pool.answer = counting # type: ignore[method-assign] + wanted: Wanted = asyncio.Queue() + task = asyncio.create_task(publish( + wanted, + api_host = '127.0.0.1', + api_port = server.port, + api_key = None, + pool = pool + )) + try: + review = ReviewIdentity(uuid.uuid4(), [rich_text('edg/compiler#4')], '') + wanted.put_nowait(Sync(review, [section])) + await _wait_for_sources(server.port, review.review_id, ['test-changes']) + + # A request its handler fails on (a source too large to read): the + # reviewer is told, once, and the error doesn't bounce between the + # provider and the server (each time through the pool). + big = section.home / 'tests' / 'suite' / 'big.sft.cpp' + with big.open('wb') as file: + file.truncate(65 * 1024 * 1024) + async with websockets.connect( + f"ws://127.0.0.1:{server.port}/review/{review.review_id}" + ) as reviewer: + await reviewer.send(encode_message(message( + 'test.src.show-src', + value = {'id': 'suite/big.sft.cpp', 'assoc_file': None}, + section = 'test-changes' + ))) + tag = None + while tag != 'invalid': + tag, _, reply_section = decode_response( + await asyncio.wait_for(reviewer.recv(), 10) + ) + assert reply_section == 'test-changes' + await asyncio.sleep(1) + assert answered == 1 + finally: + task.cancel() + await asyncio.gather(task, return_exceptions = True) + await pool.close() + + async def test_reviewer_with_outdated_protocol_is_told(server: Server) -> None: async with websockets.connect( f"ws://127.0.0.1:{server.port}/review/{uuid.uuid4()}") as reviewer: diff --git a/dev_tools/services/acknowledg-forge/tests/test_client_cancellation.py b/dev_tools/services/acknowledg-forge/tests/test_client_cancellation.py new file mode 100644 index 0000000000..172a471397 --- /dev/null +++ b/dev_tools/services/acknowledg-forge/tests/test_client_cancellation.py @@ -0,0 +1,208 @@ +# Part of the EDG Compiler Project, under the Apache License v2.0 with LLVM +# Exceptions. +# See https://edgcpp.org/LICENSE.txt for license information. +# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +'''Cancelling AcknowlEDG client requests undoes them, as failing does.''' + +import asyncio +import uuid + +from typing import Any + +from edgacknowledg import ( + RequestTags, + decode_response, + encode_message, + message, +) +from edgacknowledg.client import ( + BatchClientSession, + _BatchClientReview, + _DirectClientReview, +) +from edgacknowledg.client.websocket import ResilientWebsocket + + +class _Unanswered: + '''A batch connection the server never answers on.''' + + def __init__(self) -> None: + self.sent: list[Any] = [] + self.sent_if_connected: list[Any] = [] + + async def send(self, data: Any) -> None: + self.sent.append(data) + + async def send_if_connected(self, data: Any) -> None: + self.sent_if_connected.append(data) + + +class _Handler: + subscription_type = 'test.src' + slug = 'test-changes' + + +def _session() -> BatchClientSession: + session = BatchClientSession( + status_listener = None, + api_host = '127.0.0.1', + api_port = 1, + api_key = None, + open_timeout = None + ) + session._connection = _Unanswered() # type: ignore[assignment] + session._session_ready.set() + return session + + +def _released(session: BatchClientSession) -> list[str]: + '''The subscription ids ``session`` had the server end, unasked.''' + connection = session._connection + assert isinstance(connection, _Unanswered) + released = [] + for data in connection.sent_if_connected: + tag, value, _section = decode_response(data) + assert tag == RequestTags.BATCH_UNSUBSCRIBE + released.append(value['subscription_id']) + return released + + +class _Replying: + '''A batch socket that sends ``replies``, then nothing.''' + + def __init__(self, *replies: Any) -> None: + self._replies = list(replies) + + async def recv(self) -> Any: + if not self._replies: + await asyncio.Event().wait() + return self._replies.pop(0) + + +def _subscribed(payload: dict[str, Any], subscription_id: str) -> str: + '''The server's answer to subscribing ``payload``.''' + return encode_message(message( + RequestTags.BATCH_SUBSCRIBE, + value = {**payload, 'subscription_id': subscription_id} + )) + + +async def _cancel_once_sent(session: BatchClientSession, + request: 'asyncio.Task[Any]') -> None: + connection = session._connection + assert isinstance(connection, _Unanswered) + while len(connection.sent) == 0: + await asyncio.sleep(0.01) + request.cancel() + await asyncio.gather(request, return_exceptions = True) + assert request.cancelled() + + +async def test_cancelled_subscribes_are_not_awaited() -> None: + session = _session() + subscribing = asyncio.create_task(session._subscribe({ + 'id': str(uuid.uuid4()), 'type': 'test.src', 'slug': 'test-changes', + })) + await _cancel_once_sent(session, subscribing) + assert session._pending_subscribe == {} + + unsubscribing = asyncio.create_task(session._unsubscribe(uuid.uuid4())) + session._connection = _Unanswered() # type: ignore[assignment] + await _cancel_once_sent(session, unsubscribing) + assert session._pending_unsubscribe == {} + + +async def test_cancelled_batch_sections_are_not_added() -> None: + session = _session() + review = _BatchClientReview(session, uuid.uuid4(), [], '') + adding = asyncio.create_task(review.add_section(_Handler())) # type: ignore + await _cancel_once_sent(session, adding) + # Not left to be subscribed on reconnecting. + assert review.sections() == [] + assert session._pending_subscribe == {} + + +class _DirectSession: + def _require_connected(self) -> tuple[str, int]: + return '127.0.0.1', 1 + + +async def test_direct_sections_are_added_without_connecting() -> None: + review = _DirectClientReview( + _DirectSession(), # type: ignore[arg-type] + uuid.uuid4(), + [], + '' + ) + connecting = asyncio.Event() + stopped = asyncio.Event() + + async def connection_loop(handler: Any) -> None: + connecting.set() + try: + await asyncio.Event().wait() + finally: + stopped.set() + + review._section_connection_loop = connection_loop # type: ignore + handler = _Handler() + # Nothing to cancel it in: it doesn't wait for the section to connect. + await asyncio.wait_for(review.add_section(handler), 5) # type: ignore + assert review.sections() == [handler] + await asyncio.wait_for(connecting.wait(), 5) + + await review.remove_section(handler) # type: ignore[arg-type] + assert review.sections() == [] + assert review._section_tasks == {} + assert stopped.is_set() + + +async def test_late_subscribes_are_released() -> None: + session = _session() + payload = {'id': str(uuid.uuid4()), 'type': 'test.src', 'slug': 'test'} + subscribing = asyncio.create_task(session._subscribe(payload)) + await _cancel_once_sent(session, subscribing) + + # Answered after it was given up on. + subscription_id = str(uuid.uuid4()) + receiving = asyncio.create_task(session._recv_loop( + _Replying(_subscribed(payload, subscription_id)) # type: ignore + )) + async def released() -> None: + while _released(session) == []: + await asyncio.sleep(0.01) + + try: + await asyncio.wait_for(released(), 5) + except TimeoutError: + pass + receiving.cancel() + await asyncio.gather(receiving, return_exceptions = True) + assert _released(session) == [subscription_id] + assert session._inbounds == {} + + +async def test_subscribes_answered_as_cancelled_are_released() -> None: + session = _session() + payload = {'id': str(uuid.uuid4()), 'type': 'test.src', 'slug': 'test'} + subscribing = asyncio.create_task(session._subscribe(payload)) + connection = session._connection + assert isinstance(connection, _Unanswered) + while len(connection.sent) == 0: + await asyncio.sleep(0.01) + + # Answered, then cancelled before it resumes to take the answer. + subscription_id = str(uuid.uuid4()) + (future,) = session._pending_subscribe.values() + future.set_result({**payload, 'subscription_id': subscription_id}) + subscribing.cancel() + await asyncio.gather(subscribing, return_exceptions = True) + assert subscribing.cancelled() + assert _released(session) == [subscription_id] + + +async def test_releases_are_dropped_while_disconnected() -> None: + connection = ResilientWebsocket('ws://127.0.0.1:1', {}) + # Not left waiting for a connection: the server ended what it would end. + await asyncio.wait_for(connection.send_if_connected('{}'), 5) diff --git a/dev_tools/services/acknowledg-forge/tests/test_containers.py b/dev_tools/services/acknowledg-forge/tests/test_containers.py index 5849eb2474..7e158558a9 100644 --- a/dev_tools/services/acknowledg-forge/tests/test_containers.py +++ b/dev_tools/services/acknowledg-forge/tests/test_containers.py @@ -41,10 +41,12 @@ reason = 'needs Docker and ACKNOWLEDG_FORGE_TEST_IMAGE' ) -# The service container's command, so probes can run in it. +# The service container's command, so probes can run in it (and so no +# healthcheck: nothing answers it). _OVERRIDE = '''services: service: command: ["sleep", "infinity"] + healthcheck: {disable: true} ''' @@ -209,7 +211,7 @@ def test_networks_pair_containers(deployment: Deployment) -> None: def test_webhooks_bound_to_the_frontend_reach_only_the_host( deployment: Deployment) -> None: - # As the service does with ACKNOWLEDG_FORGE_WEBHOOK_BIND= + # As the service does with ACKNOWLEDG_FORGE_SERVICE_BIND= # service-frontend:2728 (a stand-in server; the service isn't running). deployment.compose( 'exec', '--detach', 'service', PYTHON, '-m', 'http.server', @@ -240,12 +242,14 @@ def test_the_service_builds_through_the_builder( 'import asyncio, json\n' 'from edgacknowledg.forge.sandbox import RunnerClient, Sandbox\n' 'async def main():\n' - " sandbox = Sandbox(builder = RunnerClient('ws://builder:2730'))\n" - ' await sandbox.check()\n' - ' allowed = await sandbox.run(\n' - " ['python3', '/edg/tools/bin/edg-rematerialize-test-run', '--help'],\n" - ' timeout = 60)\n' - " refused = await sandbox.run(['sh', '-c', 'id'], timeout = 60)\n" + " async with RunnerClient('ws://builder:2730') as builder:\n" + ' sandbox = Sandbox(builder = builder)\n' + ' await sandbox.check()\n' + ' allowed = await sandbox.run(\n' + " ['python3', '/edg/tools/bin/edg-rematerialize-test-run',\n" + " '--help'],\n" + ' timeout = 60)\n' + " refused = await sandbox.run(['sh', '-c', 'id'], timeout = 60)\n" ' print(json.dumps([allowed.returncode, allowed.output,\n' ' refused.returncode, refused.output]))\n' 'asyncio.run(main())\n' @@ -265,16 +269,15 @@ def test_the_publisher_runs_tools_through_its_runner( 'from edgacknowledg.forge.runner.client import RunnerClient, ' 'RunnerError\n' 'async def main():\n' - " client = RunnerClient('ws://tools:2732')\n" - ' codes = []\n' - " for argv in (['ifc-dump', '--help'], ['cg_annotate', '--version']):\n" - ' codes.append((await client.run(argv, timeout = 60)).returncode)\n' - ' try:\n' - " await client.run(['sh', '-c', 'id'], timeout = 60)\n" - " codes.append('ran')\n" - ' except RunnerError as ex:\n' - ' codes.append(str(ex))\n' - ' await client.close()\n' + " async with RunnerClient('ws://tools:2732') as client:\n" + ' codes = []\n' + " for argv in (['ifc-dump', '--help'], ['cg_annotate', '--version']):\n" + ' codes.append((await client.run(argv, timeout = 60)).returncode)\n' + ' try:\n' + " await client.run(['sh', '-c', 'id'], timeout = 60)\n" + " codes.append('ran')\n" + ' except RunnerError as ex:\n' + ' codes.append(str(ex))\n' ' print(json.dumps(codes))\n' 'asyncio.run(main())\n' )) @@ -289,24 +292,24 @@ def test_the_publisher_runs_tools_through_its_runner( def test_the_publisher_serves_each_connection_itself( deployment: Deployment) -> None: # The service connects (here: a probe), and the publisher server answers - # its requests, with a publisher of its own for the connection; its pool - # processes stand ready for reviewers. + # its requests with its publisher; its pool processes stand ready for + # reviewers. result = deployment.python('service', ( 'import asyncio, json, uuid\n' 'from websockets.asyncio.client import connect\n' + 'from edgacknowledg import rich_text\n' + 'from edgacknowledg.forge.publisher import ReviewIdentity\n' + 'from edgacknowledg.forge.publisher.protocol import encode_review\n' 'async def main():\n' " async with connect('ws://publisher:2731') as connection:\n" ' await connection.send(json.dumps({\n' " 'api_host': 'acknowledg.invalid', 'api_port': 2727,\n" " 'api_key': None, 'ui_host': 'ui.invalid', 'ui_port': 443,\n" f" 'reviews_dir': '{STATE}/reviews'}}))\n" - ' await connection.send(json.dumps({\n' - " 'op': 'remove', 'review_id': str(uuid.uuid4())}))\n" - ' while True:\n' - ' answer = json.loads(await connection.recv())\n' - " if 'event' not in answer:\n" - ' print(json.dumps(answer))\n' - ' return\n' + " review = ReviewIdentity(uuid.uuid4(), [rich_text('probe')], '')\n" + " await connection.send(json.dumps({'op': 'sync',\n" + " 'review': encode_review(review), 'sections': []}))\n" + ' print(await connection.recv())\n' 'asyncio.run(main())\n' )) assert result.stdout.strip().splitlines()[-1] == '{"ok": true}', result diff --git a/dev_tools/services/acknowledg-forge/tests/test_pool.py b/dev_tools/services/acknowledg-forge/tests/test_pool.py index d43c253d20..3e629c79c0 100644 --- a/dev_tools/services/acknowledg-forge/tests/test_pool.py +++ b/dev_tools/services/acknowledg-forge/tests/test_pool.py @@ -8,6 +8,7 @@ ''' import asyncio +import errno import os import signal import time @@ -35,10 +36,12 @@ from conftest import make_tools_dir -# Stands in for ifc-dump in the tools runner: hangs on files named slow*. +# Stands in for ifc-dump in the tools runner: hangs on files named slow*, +# and takes a second on files named medium*. STUB_IFC_DUMP = '''#!/bin/sh case "$1" in */slow*) sleep 60 ;; + */medium*) sleep 1 ;; esac echo "dumped $(basename "$1")" ''' @@ -57,7 +60,7 @@ async def env(tmp_path: Path) -> AsyncIterator[_Env]: home = tmp_path / 'home' tests_home = home / 'reviews' / str(uuid.uuid4()) / 'base' / 'tests' (tests_home / 'tests' / 'suite').mkdir(parents = True) - for name in ('a.ifc', 'slow.ifc'): + for name in ('a.ifc', 'medium.ifc', 'slow.ifc'): (tests_home / 'tests' / 'suite' / name).write_text('ifc\n') root = tests_home / 'runs' / 'r1' (root / 'edg_x86_64').mkdir(parents = True) @@ -146,6 +149,52 @@ async def test_a_slow_request_is_cut_off_alone(env: _Env, assert pool.pids()[0] != pids[0] and pool.pids()[1] == pids[1] +async def test_an_abandoned_requests_answer_is_dropped( + env: _Env, + make_pool: Any) -> None: + pool = await make_pool(processes = 1) + [pid] = pool.pids() + # Cancelled while its process works on it (as when its section is + # removed): the process still answers it... + medium = asyncio.create_task( + pool.answer(env.section, _show_source('suite/medium.ifc')) + ) + await asyncio.sleep(0.3) + medium.cancel() + with pytest.raises(asyncio.CancelledError): + await medium + # ...but the next request, on the same process, gets its own answer. + answer = await pool.answer(env.section, _show_source('suite/a.ifc')) + assert 'dumped a.ifc' in _content(answer) + assert pool.pids() == [pid] + + +async def test_abandoned_requests_dont_crowd_out_the_next( + env: _Env, + make_pool: Any) -> None: + # Its deadline (1.5 s) would be missed, were the abandoned requests' work + # (1 s each) counted in it. + pool = await make_pool(processes = 1, request_timeout = 1.5) + [pid] = pool.pids() + first, second = ( + asyncio.create_task( + pool.answer(env.section, _show_source('suite/medium.ifc')) + ) + for _ in range(2) + ) + await asyncio.sleep(0.3) + # One being answered, one still queued: the first keeps its process busy + # until answered; the second is never given to it. + first.cancel() + second.cancel() + await asyncio.gather(first, second, return_exceptions = True) + started = time.monotonic() + answer = await pool.answer(env.section, _show_source('suite/a.ifc')) + assert 'dumped a.ifc' in _content(answer) + assert time.monotonic() - started < 1.5 + assert pool.pids() == [pid] + + async def test_an_exited_process_is_replaced(env: _Env, make_pool: Any) -> None: pool = await make_pool(processes = 1) @@ -188,6 +237,35 @@ async def test_a_process_failing_to_start_backs_off( await pool.close() +async def test_no_process_starting_at_all_backs_off( + env: _Env, + monkeypatch: pytest.MonkeyPatch) -> None: + create_subprocess_exec = asyncio.create_subprocess_exec + failing = True + + async def out_of_processes(*args: Any, **kwargs: Any) -> Any: + if failing: + raise OSError(errno.EAGAIN, 'Resource temporarily unavailable') + return await create_subprocess_exec(*args, **kwargs) + + monkeypatch.setattr(asyncio, 'create_subprocess_exec', out_of_processes) + pool = ProcessPool(tools_service_url = env.tools.url, processes = 1) + try: + # As when it starts but isn't ready. + with pytest.raises(PoolError, match = 'failed to start'): + await pool.start() + with pytest.raises(PoolError, match = 'restarting'): + await pool.answer(env.section, _show_source('suite/a.ifc')) + + # Its dispatcher still tries again, once it may. + failing = False + await asyncio.sleep(1.5) + answer = await pool.answer(env.section, _show_source('suite/a.ifc')) + assert 'dumped a.ifc' in _content(answer) + finally: + await pool.close() + + class _Channel: '''One request in; the replies the stand-in handler relays, out.''' @@ -222,12 +300,19 @@ async def test_the_stand_in_handler_relays_and_raises( await handler.handle_request(channel) # type: ignore[arg-type] assert caught.value.reviewer_id == REVIEWER - # Any other failure: the client library sends its failure reply. + # Any other failure: the client library sends its failure reply, to the + # reviewer. broken = UnixSocketHandler( SectionRecord(**{**env.section.__dict__, 'kind': 'no-such-kind'}), pool ) - with pytest.raises(PoolError, match = 'failed'): + with pytest.raises(PoolError, match = 'failed') as failed: await broken.handle_request( # type: ignore[arg-type] _Channel(_show_source('suite/a.ifc')) ) + assert failed.value.reviewer_id == REVIEWER + + # The server's own messages (naming no reviewer) aren't answered. + channel = _Channel(encode_message(message('invalid'))) + await handler.handle_request(channel) # type: ignore[arg-type] + assert channel.sent == [] diff --git a/dev_tools/services/acknowledg-forge/tests/test_publisher.py b/dev_tools/services/acknowledg-forge/tests/test_publisher.py index 4c83a473f9..c4d20c023f 100644 --- a/dev_tools/services/acknowledg-forge/tests/test_publisher.py +++ b/dev_tools/services/acknowledg-forge/tests/test_publisher.py @@ -13,37 +13,55 @@ ''' import asyncio +import dataclasses +import json +import urllib.request import uuid from pathlib import Path -from typing import Any, AsyncIterator, Optional +from typing import Any, AsyncIterator, Callable, Optional import pytest import pytest_asyncio import websockets +from websockets.asyncio.client import ClientConnection + from edgacknowledg import ( + MAX_RICH_NAME_LENGTH, decode_response, encode_message, + is_valid_rich_name, message, rich_link, rich_text, ) -from edgacknowledg.forge.db import SectionRecord +from edgacknowledg.forge import serving +from edgacknowledg.forge.db import ( + ForgeStore, + PullRequestRecord, + SectionRecord, +) +from edgacknowledg.forge.ingest import utc_now from edgacknowledg.forge.local import LocalDeployment, local_deployment from edgacknowledg.forge.publisher import ( KIND_TEST_CHANGES, - PublisherUnavailableError, + TITLE_ELLIPSIS, ReviewIdentity, + review_identity, ) from edgacknowledg.forge.publisher.client import PublisherClient from edgacknowledg.forge.publisher.protocol import ( + MESSAGE_LIMIT, decode_review, encode_review, ) -from edgacknowledg.forge.publisher.server import PublisherServer +from edgacknowledg.forge.publisher.server import create_publisher +from edgacknowledg.forge.sandbox_client import ServiceUnavailableError +from edgacknowledg.forge.serving import HEALTH_PATH, Listening -from conftest import make_tools_dir +from conftest import make_tools_dir, pull_request_info +from fakes import REPOSITORY, FakeForge from test_acknowledg_integration import ( Server, _wait_for_sources, @@ -104,12 +122,32 @@ async def deployment(tmp_path: Path) -> AsyncIterator[_Deployment]: yield local, reviews, section -def _url(listening: Any) -> str: - return f"ws://127.0.0.1:{next(iter(listening.sockets)).getsockname()[1]}" +async def _store(store: ForgeStore, + number: int, + sections: list[SectionRecord], + *, + url: Optional[str] = None) -> uuid.UUID: + '''Store pull request ``number`` (at ``url``, if given) with ``sections`` + as served, as the processor does; its review. + ''' + info = pull_request_info(number, 'a' * 40, 'b' * 40) + if url is not None: + info = dataclasses.replace(info, url = url) + await store.upsert_pull_request( + FakeForge.name, + info, + now = utc_now(), + settle_timeout = 0 + ) + record = await store.find_pull_request(FakeForge.name, REPOSITORY, number) + assert record is not None + await store.replace_sections(record.id, sections) + return record.review_id def _client(url: str, reviews: Path, + store: ForgeStore, *, api_port: int, **kwargs: Any) -> PublisherClient: @@ -121,85 +159,334 @@ def _client(url: str, ui_host = 'ui.example', ui_port = 443, reviews_dir = reviews, + store = store, + forge_name = FakeForge.name, **kwargs ) -@pytest.mark.parametrize('ending', ['server drops it', 'client drops it']) -async def test_publishing_survives_reconnecting(deployment: _Deployment, +def _empty_sync() -> dict[str, Any]: + '''A request serving nothing for a review nothing serves.''' + return { + 'op': 'sync', + 'review': encode_review(ReviewIdentity(uuid.uuid4(), NAME, '')), + 'sections': [], + } + + +async def _wait_for(condition: Callable[[], bool]) -> None: + for _ in range(300): + if condition(): + return + await asyncio.sleep(0.05) + raise AssertionError('timed out') + + +async def test_publishing_outlasts_reconnecting(deployment: _Deployment, server: Server, - ending: str) -> None: + store: ForgeStore) -> None: local, reviews, section = deployment - publisher = _client(local.publisher_service_url, reviews, - api_port = server.port, restart_delay = 0.1) - task = asyncio.create_task(publisher.run()) + publisher = _client(local.publisher_service_url, reviews, store, + api_port = server.port) + publisher.start() try: await asyncio.wait_for(publisher.wait_connected(), 30) - review = ReviewIdentity(uuid.uuid4(), NAME, '') - await publisher.sync(review, [section]) - await _wait_for_sources(server.port, review.review_id, ['test-changes']) - value = await _show_source(server.port, review.review_id) + review_id = await _store(store, 1, [section]) + await publisher.sync_one(review_id) + await _wait_for_sources(server.port, review_id, ['test-changes']) + + # The connection goes; what it published stays, and still does once + # the client connects again. + connection = publisher._link.connection + assert connection is not None + connection.transport.abort() + await _wait_for(lambda: publisher._link.connection is not connection) + await _wait_for_sources(server.port, review_id, ['test-changes']) + await asyncio.wait_for(publisher.wait_connected(), 30) + await _wait_for_sources(server.port, review_id, ['test-changes']) + value = await _show_source(server.port, review_id) assert 'int a;' in str(value) + finally: + await publisher.close() + + +async def test_connecting_serves_what_is_stored(deployment: _Deployment, + server: Server, + store: ForgeStore) -> None: + local, reviews, section = deployment + served = await _store(store, 1, [section]) + # Its files are gone (as after a disk cleanup). + gone = await _store(store, 2, [ + dataclasses.replace(section, root = section.root.parent / 'gone') + ]) + for record in await store.open_pull_requests(FakeForge.name): + await store.mark_processed(record.id, record.generation) + + publisher = _client(local.publisher_service_url, reviews, store, + api_port = server.port) + publisher.start() + try: + await asyncio.wait_for(publisher.wait_connected(), 30) + await _wait_for_sources(server.port, served, ['test-changes']) + await _wait_for_sources(server.port, gone, []) + # Not served, but rebuilt. + dirty = { + record.review_id: record.is_dirty + for record in await store.open_pull_requests(FakeForge.name) + } + assert dirty == {served: False, gone: True} + finally: + await publisher.close() + + +async def test_connecting_leaves_out_what_the_server_rejects( + deployment: _Deployment, + server: Server, + store: ForgeStore) -> None: + local, reviews, section = deployment + served = await _store(store, 1, [section]) + # A name linking to anything but a web URL is rejected. + rejected = await _store(store, 2, [section], url = 'ftp://forge.example/2') + for record in await store.open_pull_requests(FakeForge.name): + await store.mark_processed(record.id, record.generation) + + publisher = _client(local.publisher_service_url, reviews, store, + api_port = server.port) + publisher.start() + try: + # It connects all the same, serving the rest. + await asyncio.wait_for(publisher.wait_connected(), 30) + await _wait_for_sources(server.port, served, ['test-changes']) + await _wait_for_sources(server.port, rejected, []) + dirty = { + record.review_id: record.is_dirty + for record in await store.open_pull_requests(FakeForge.name) + } + assert dirty == {served: False, rejected: True} + finally: + await publisher.close() + + +async def test_failing_to_serve_on_connecting_reconnects( + deployment: _Deployment, + server: Server, + store: ForgeStore, + monkeypatch: pytest.MonkeyPatch) -> None: + local, reviews, section = deployment + review_id = await _store(store, 1, [section]) + open_pull_requests = store.open_pull_requests + calls = 0 + + async def failing_once(forge_name: str) -> Any: + nonlocal calls + calls += 1 + if calls == 1: + raise RuntimeError('database outage') + return await open_pull_requests(forge_name) + + monkeypatch.setattr(store, 'open_pull_requests', failing_once) + publisher = _client(local.publisher_service_url, reviews, store, + api_port = server.port) + publisher.start() + try: + # The connection that failed went, rather than staying unready; the + # next one serves it. + await asyncio.wait_for(publisher.wait_connected(), 30) + assert calls == 2 + await _wait_for_sources(server.port, review_id, ['test-changes']) + finally: + await publisher.close() + + +async def test_a_connection_lost_at_once_reconnects( + deployment: _Deployment, + server: Server, + store: ForgeStore, + monkeypatch: pytest.MonkeyPatch) -> None: + local, reviews, _ = deployment + send = PublisherClient._send + calls = 0 + + async def failing_once(connection: ClientConnection, + message: dict[str, Any]) -> None: + nonlocal calls + calls += 1 + if calls == 1: + raise ServiceUnavailableError('the publisher server went away') + await send(connection, message) + + # Gone before it is even initialized. + monkeypatch.setattr(PublisherClient, '_send', staticmethod(failing_once)) + publisher = _client(local.publisher_service_url, reviews, store, + api_port = server.port) + publisher.start() + try: + await asyncio.wait_for(publisher.wait_connected(), 30) + finally: + await publisher.close() - # The connection goes (the server's side, or the client gave up on - # it): its publisher, and what it served, go with it, and come back - # with the next connection's. - if ending == 'server drops it': - local.publisher_server.drop_connection() - else: - connection = publisher._connection - assert connection is not None - connection.abort() - await _wait_for_sources(server.port, review.review_id, []) + +async def test_a_new_service_drops_what_it_does_not_store( + deployment: _Deployment, + server: Server, + store: ForgeStore) -> None: + local, reviews, section = deployment + earlier = _client(local.publisher_service_url, reviews, store, + api_port = server.port) + earlier.start() + try: + await asyncio.wait_for(earlier.wait_connected(), 30) + review_id = await _store(store, 1, [section]) + await earlier.sync_one(review_id) + await _wait_for_sources(server.port, review_id, ['test-changes']) + finally: + await earlier.close() + + # The publisher still serves it, until a service whose database doesn't + # store it connects. + await _wait_for_sources(server.port, review_id, ['test-changes']) + other_store = await ForgeStore.open('sqlite+aiosqlite://') + later = _client(local.publisher_service_url, reviews, other_store, + api_port = server.port) + later.start() + try: + await asyncio.wait_for(later.wait_connected(), 30) + await _wait_for_sources(server.port, review_id, []) + finally: + await later.close() + await other_store.aclose() + + +async def test_delisting_stops_serving_what_is_still_stored( + deployment: _Deployment, + server: Server, + store: ForgeStore) -> None: + local, reviews, section = deployment + publisher = _client(local.publisher_service_url, reviews, store, + api_port = server.port) + publisher.start() + try: await asyncio.wait_for(publisher.wait_connected(), 30) - await _wait_for_sources(server.port, review.review_id, ['test-changes']) + review_id = await _store(store, 1, [section]) + await publisher.sync_one(review_id) + await _wait_for_sources(server.port, review_id, ['test-changes']) + + # As when tearing it down, before its files go. + await publisher.delist(review_id) + await _wait_for_sources(server.port, review_id, []) + record = await store.find_pull_request_by_review(review_id) + assert record is not None and len(await store.sections(record.id)) == 1 + finally: + await publisher.close() + - await publisher.remove(review.review_id) - await _wait_for_sources(server.port, review.review_id, []) +async def test_connections_share_the_publisher(deployment: _Deployment, + server: Server, + store: ForgeStore) -> None: + local, reviews, section = deployment + publisher = _client(local.publisher_service_url, reviews, store, + api_port = server.port) + publisher.start() + try: + await asyncio.wait_for(publisher.wait_connected(), 30) + review_id = await _store(store, 1, [section]) + await publisher.sync_one(review_id) + await _wait_for_sources(server.port, review_id, ['test-changes']) + + # Another connection's requests go to the same publisher, and its + # opening and closing change nothing it publishes. + async with websockets.connect(local.publisher_service_url) as other: + await other.send(json.dumps(publisher._init_message())) + await other.send(json.dumps(_empty_sync())) + answer = json.loads(await asyncio.wait_for(other.recv(), 30)) + assert answer == {'ok': True} + await _wait_for_sources(server.port, review_id, ['test-changes']) + await publisher.sync_one(await _store(store, 1, [])) + await _wait_for_sources(server.port, review_id, []) finally: - task.cancel() - await asyncio.gather(task, return_exceptions = True) + await publisher.close() + + +async def _start_publisher(tools_service_url: str) -> Listening: + return await serving.start( + create_publisher(tools_service_url = tools_service_url, processes = 1), + host = '127.0.0.1', + port = 0, + max_message_size = MESSAGE_LIMIT + ) async def test_publishing_needs_no_tools_runner( tmp_path: Path, + store: ForgeStore, unreachable_port: int, unreachable_url: str) -> None: reviews, _ = _layout(tmp_path) # Only requests running a command need the tools runner; the connection # is served without it. - publisher_server = PublisherServer(tools_service_url = unreachable_url, - processes = 1) - listening = await publisher_server.start('127.0.0.1', 0) - client = _client(_url(listening), reviews, api_port = unreachable_port) - connection = asyncio.create_task(client._run_connection()) + listening = await _start_publisher(unreachable_url) + client = _client(listening.url(), reviews, store, + api_port = unreachable_port) + client.start() + try: + await asyncio.wait_for(client.wait_connected(), 30) + await asyncio.wait_for(client.sync_one(await _store(store, 1, [])), 30) + finally: + await client.close() + await listening.close() + + +def _get(url: str) -> int: + with urllib.request.urlopen(url, timeout = 10) as response: + return int(response.status) + + +async def test_health_checks_leave_the_connection( + tmp_path: Path, + unreachable_port: int, + unreachable_url: str) -> None: + # A health check is a route of its own: it leaves the connection + # serving. + listening = await _start_publisher(unreachable_url) try: - for _ in range(100): - if client._connection is not None: - break - await asyncio.sleep(0.05) - await asyncio.wait_for(client.remove(uuid.uuid4()), 30) - assert not connection.done() + async with websockets.connect(listening.url()) as connection: + await connection.send(json.dumps({ + 'api_host': '127.0.0.1', 'api_port': unreachable_port, + 'api_key': None, 'ui_host': 'ui.invalid', 'ui_port': 443, + 'reviews_dir': str(tmp_path), + })) + health = listening.url('http') + HEALTH_PATH + assert await asyncio.to_thread(_get, health) == 200 + # The connection still serves its publisher. + await connection.send(json.dumps(_empty_sync())) + answer = json.loads(await asyncio.wait_for(connection.recv(), 30)) + assert answer == {'ok': True} finally: - connection.cancel() - await asyncio.gather(connection, return_exceptions = True) - listening.close() - await listening.wait_closed() - await publisher_server.close() + await listening.close() class _SilentConnection: - '''Stands in for a connection whose server never answers.''' + '''Stands in for a connection whose server never answers (or, given + ``answer``, answers that). + ''' - def __init__(self) -> None: + def __init__(self, answer: Optional[bytes] = None) -> None: self.sent: list[Any] = [] self.aborted = False + self._answer = answer async def send(self, message: Any) -> None: self.sent.append(message) - async def receive(self) -> Optional[bytes]: - return None + async def recv(self) -> Optional[bytes]: + if self._answer is None: + await asyncio.Event().wait() + return self._answer + + @property + def transport(self) -> '_SilentConnection': + # Its own transport, to abort. + return self def abort(self) -> None: self.aborted = True @@ -210,24 +497,62 @@ async def close(self) -> None: async def test_unanswered_request_drops_the_connection( tmp_path: Path, + store: ForgeStore, unreachable_port: int, unreachable_url: str) -> None: reviews, section = _layout(tmp_path) - publisher = _client(unreachable_url, reviews, api_port = unreachable_port, - request_timeout = 0.1) + publisher = _client(unreachable_url, reviews, store, + api_port = unreachable_port, request_timeout = 0.1) connection = _SilentConnection() - publisher._connection = connection # type: ignore[assignment] - publisher._connected.set() - review = ReviewIdentity(uuid.uuid4(), NAME, '') - with pytest.raises(PublisherUnavailableError, match = 'stopped answering'): - await publisher.sync(review, [section]) + publisher._link._connection = connection # type: ignore[assignment] + publisher._link._connected.set() + review_id = await _store(store, 1, [section]) + with pytest.raises(ServiceUnavailableError, match = 'stopped answering'): + await publisher.delist(review_id) + assert connection.aborted + + # Nothing more is sent on (or read from) it, so its late answer can't be + # taken for the next request's. + with pytest.raises(ServiceUnavailableError, match = 'not connected'): + await publisher.delist(review_id) + assert len(connection.sent) == 1 + + +async def test_bad_answer_drops_the_connection( + tmp_path: Path, + store: ForgeStore, + unreachable_port: int, + unreachable_url: str) -> None: + reviews, section = _layout(tmp_path) + publisher = _client(unreachable_url, reviews, store, + api_port = unreachable_port) + # Out of step with the server, whose later answers can't be trusted. + connection = _SilentConnection(b'[]') + publisher._link._connection = connection # type: ignore[assignment] + publisher._link._connected.set() + review_id = await _store(store, 1, [section]) + with pytest.raises(ServiceUnavailableError, match = 'bad answer'): + await publisher.delist(review_id) assert connection.aborted + assert publisher._link.connection is None + - # Its late answer isn't taken for the next request's: nothing more is - # sent on it. - publisher._handle_message(b'{"ok": true}\n') - with pytest.raises(PublisherUnavailableError, match = 'not connected'): - await publisher.sync(review, [section]) +async def test_unanswered_sync_is_left_to_connecting( + tmp_path: Path, + store: ForgeStore, + unreachable_port: int, + unreachable_url: str) -> None: + reviews, section = _layout(tmp_path) + publisher = _client(unreachable_url, reviews, store, + api_port = unreachable_port, request_timeout = 0.1) + connection = _SilentConnection() + publisher._link._connection = connection # type: ignore[assignment] + publisher._link._connected.set() + review_id = await _store(store, 1, [section]) + # What is stored is served on connecting, so neither fails. + await publisher.sync_one(review_id) + assert connection.aborted + await publisher.sync_one(review_id) assert len(connection.sent) == 1 @@ -243,3 +568,38 @@ def test_review_names_survive_the_protocol() -> None: # Rejected rather than shown as the repr of whatever it was. with pytest.raises(ValueError): decode_review({**encode_review(review), 'name': 'edg/compiler#1: test'}) + + +def test_long_titles_are_cut_to_fit_the_review_name() -> None: + info = dataclasses.replace( + pull_request_info(1, 'head', 'base'), + title = 'x' * MAX_RICH_NAME_LENGTH + ) + now = utc_now() + record = PullRequestRecord( + id = 1, + forge = 'github', + review_id = uuid.uuid4(), + info = info, + head_changed_at = now, + settle_deadline = now, + generation = 1, + processed_generation = 0, + error_count = 0, + next_attempt_at = None, + last_error = None, + ) + review = review_identity(record) + link, title = review.name + + # The link is whole; only the title is cut, just enough. + assert link == rich_link(info.url, f"{REPOSITORY}#1") + assert title['text'].endswith(TITLE_ELLIPSIS) + assert is_valid_rich_name(review.name) + length = len(link['url']) + len(link['text']) + len(title['text']) + assert length == MAX_RICH_NAME_LENGTH + assert decode_review(encode_review(review)) == review + + # A title that fits is left alone. + short = dataclasses.replace(record, info = pull_request_info(1, 'h', 'b')) + assert review_identity(short).name[1] == rich_text(' Pull request 1') diff --git a/dev_tools/services/acknowledg-forge/tests/test_runner.py b/dev_tools/services/acknowledg-forge/tests/test_runner.py index 7f62030318..f38f5e04b2 100644 --- a/dev_tools/services/acknowledg-forge/tests/test_runner.py +++ b/dev_tools/services/acknowledg-forge/tests/test_runner.py @@ -11,11 +11,14 @@ ''' import asyncio +import itertools +import json import os import shutil -import socket import subprocess import time +import urllib.error +import urllib.request from pathlib import Path from typing import AsyncIterator @@ -23,6 +26,11 @@ import pytest import pytest_asyncio +from websockets.asyncio.server import ServerConnection +from websockets.asyncio.server import serve as ws_serve + +from edgacknowledg.forge import serving +from edgacknowledg.forge.runner import client as runner_client from edgacknowledg.forge.runner.client import RunnerClient, RunnerError from edgacknowledg.forge.runner.roles import ( RunnerPolicy, @@ -30,23 +38,23 @@ tools_policy, ) from edgacknowledg.forge.runner.server import ( + MAX_REQUEST_BYTES, RequestError, - RunnerServer, check_request, + create_runner, stray_processes, ) from edgacknowledg.forge.sandbox import Sandbox, SandboxUnavailableError +from edgacknowledg.forge.sandbox_client import ( + ForgeSandboxClient, + ServiceUnavailableError, +) +from edgacknowledg.forge.serving import HEALTH_PATH from edgacknowledg.forge.workspace import Workspaces from fakes import FakeForge -def _free_port() -> int: - with socket.socket() as sock: - sock.bind(('127.0.0.1', 0)) - return int(sock.getsockname()[1]) - - def _alive(pid: int) -> bool: try: state = Path(f"/proc/{pid}/stat").read_text().rsplit(')', 1)[1].split()[0] @@ -76,19 +84,15 @@ def __init__(self, root: Path, client: RunnerClient) -> None: async def runner(tmp_path: Path) -> AsyncIterator[_Running]: root = tmp_path / 'reviews' root.mkdir() - port = _free_port() - server = RunnerServer(_policy(root)) - task = asyncio.create_task(server.serve('127.0.0.1', port)) - for _ in range(100): - try: - _, writer = await asyncio.open_connection('127.0.0.1', port) - writer.close() - break - except OSError: - await asyncio.sleep(0.05) - yield _Running(root, RunnerClient(f"ws://127.0.0.1:{port}")) - task.cancel() - await asyncio.gather(task, return_exceptions = True) + listening = await serving.start(create_runner(_policy(root)), + host = '127.0.0.1', port = 0, + max_message_size = MAX_REQUEST_BYTES) + client = RunnerClient(listening.url()) + client.start() + yield _Running(root, client) + # Its connection first: the server's shutdown waits for it. + await client.close() + await listening.close() # What's allowed @@ -168,6 +172,22 @@ async def test_relays_output_and_status(runner: _Running) -> None: assert result.returncode != 0 +async def test_output_kept_is_capped(runner: _Running, + monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr(runner_client, 'MAX_OUTPUT_BYTES', 10) + monkeypatch.setattr(runner_client, 'OUTPUT_TAIL_BYTES', 4) + result = await runner.client.run( + ['sh', '-c', 'printf 0123456789; printf abcdefgh; printf WXYZ'], + timeout = 10 + ) + # Its start, and its end (where its errors are), within the limit. + assert result.output == b'012345\n[... 12 bytes of output dropped ...]\nWXYZ' + + result = await runner.client.run(['sh', '-c', 'printf 0123456789'], + timeout = 10) + assert result.output == b'0123456789' + + async def test_environment_is_cleared(runner: _Running, monkeypatch: pytest.MonkeyPatch) -> None: monkeypatch.setenv('EDG_SECRET', 'leaked') @@ -186,6 +206,94 @@ async def test_refusals_reach_the_client(runner: _Running) -> None: assert description['roots'] == [str(runner.root)] +async def test_refused_connections_are_retried( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + caplog: pytest.LogCaptureFixture) -> None: + # Retrying at once, rather than after websockets' backoff. + monkeypatch.setattr(ForgeSandboxClient, '_reconnect_delays', + staticmethod(lambda: itertools.repeat(0.1))) + refused = 0 + + async def refuse(reader: asyncio.StreamReader, + writer: asyncio.StreamWriter) -> None: + nonlocal refused + refused += 1 + await reader.readuntil(b'\r\n\r\n') + writer.write(b'HTTP/1.1 404 Not Found\r\nContent-Length: 0\r\n\r\n') + await writer.drain() + writer.close() + + server = await asyncio.start_server(refuse, '127.0.0.1', 0) + port = server.sockets[0].getsockname()[1] + async with RunnerClient(f"ws://127.0.0.1:{port}", + open_timeout = 1) as client: + # Refused, but not given up on: requests wait for the connection. + describing = asyncio.create_task(client.describe()) + try: + for _ in range(50): + if refused >= 2: + break + await asyncio.sleep(0.1) + assert refused >= 2 + assert "can't connect to the runner" in caplog.text + assert not describing.done() + finally: + server.close() + await server.wait_closed() + + # Once a runner answers there, the client connects to it, and the + # request is served. + root = tmp_path / 'reviews' + root.mkdir() + listening = await serving.start(create_runner(_policy(root)), + host = '127.0.0.1', port = port, + max_message_size = MAX_REQUEST_BYTES) + try: + description = await asyncio.wait_for(describing, 10) + assert description['roots'] == [str(root)] + finally: + describing.cancel() + await client.close() + await listening.close() + + +@pytest.mark.parametrize('answer', ['not json', '[]']) +async def test_bad_answers_reconnect(answer: str) -> None: + connections = 0 + + async def handle(websocket: ServerConnection) -> None: + nonlocal connections + connections += 1 + first = connections == 1 + async for message in websocket: + request = json.loads(message) + await websocket.send(answer if first else json.dumps({ + 'id': request['id'], + 'description': {'roots': []}, + })) + + async with ws_serve(handle, '127.0.0.1', 0) as server: + port = list(server.sockets)[0].getsockname()[1] + async with RunnerClient(f"ws://127.0.0.1:{port}") as client: + with pytest.raises(RunnerError, match = 'lost'): + await client.describe() + # No later answer on that connection could be trusted; the next one + # serves. + assert await client.describe() == {'roots': []} + assert connections == 2 + + +async def test_closing_fails_waiting_requests(unreachable_url: str) -> None: + client = RunnerClient(unreachable_url) + client.start() + describing = asyncio.create_task(client.describe()) + await asyncio.sleep(0.2) + await client.close() + with pytest.raises(ServiceUnavailableError, match = 'closed'): + await asyncio.wait_for(describing, 1) + + async def test_timeout_kills_the_whole_group(runner: _Running) -> None: child_file = runner.root / 'child' result = await runner.client.run( @@ -241,18 +349,20 @@ async def test_losing_the_connection_kills_its_commands( timeout = 60 )) command = await _started(pid_file) - await runner.client.close() + connection = runner.client._link.connection + assert connection is not None + connection.transport.abort() with pytest.raises(RunnerError, match = 'lost'): await run assert await _gone(command) - # The next request connects again. + # The client connects again by itself. result = await runner.client.run(['true'], timeout = 10) assert result.returncode == 0 async def test_one_connection_carries_every_command(runner: _Running) -> None: await runner.client.run(['true'], timeout = 10) - connection = runner.client._connection + connection = runner.client._link.connection assert connection is not None # Commands run concurrently over it... started = time.monotonic() @@ -263,7 +373,26 @@ async def test_one_connection_carries_every_command(runner: _Running) -> None: assert time.monotonic() - started < 3 # ...each answer reaching its own request. assert [r.output for r in results] == [f"{n}\n".encode() for n in range(4)] - assert runner.client._connection is connection + assert runner.client._link.connection is connection + + +def _get(url: str) -> int: + with urllib.request.urlopen(url, timeout = 10) as response: + return int(response.status) + + +async def test_answers_health_checks_beside_its_client( + runner: _Running) -> None: + await runner.client.run(['true'], timeout = 10) + connection = runner.client._link.connection + url = runner.client.url.replace('ws://', 'http://') + HEALTH_PATH + assert await asyncio.to_thread(_get, url) == 200 + # Anything else is still a WebSocket handshake. + with pytest.raises(urllib.error.HTTPError): + await asyncio.to_thread(_get, url.replace(HEALTH_PATH, '/')) + result = await runner.client.run(['true'], timeout = 10) + assert result.returncode == 0 + assert runner.client._link.connection is connection def test_strays_are_everything_but_the_runner_and_its_ancestors() -> None: @@ -328,7 +457,10 @@ def test_roles_serve_the_workspace_layout(tmp_path: Path) -> None: ) tools = tools_policy(home = home, tools_dir = tools_dir) assert tools.roots == (workspaces.reviews_dir,) - assert {prefix[0] for prefix in tools.commands} == {'ifc-dump', 'cg_annotate'} + assert {prefix[0] for prefix in tools.commands} == { + 'ifc-dump', + 'cg_annotate', + } def test_missing_programs_are_refused_not_fatal(tmp_path: Path) -> None: diff --git a/dev_tools/services/acknowledg-forge/tests/test_safety.py b/dev_tools/services/acknowledg-forge/tests/test_safety.py index df6ab07cba..57f72dddbe 100644 --- a/dev_tools/services/acknowledg-forge/tests/test_safety.py +++ b/dev_tools/services/acknowledg-forge/tests/test_safety.py @@ -5,8 +5,10 @@ '''Handling of untrusted input: archives, paths, and the sandbox.''' +import asyncio import io import os +import signal import stat import tarfile import zipfile @@ -15,14 +17,15 @@ import pytest -from edgacknowledg.forge.sandbox import ( - RunnerClient, - Sandbox, - SandboxUnavailableError, -) +from edgacknowledg.forge import processor, workspace +from edgacknowledg.forge.processor import _changes_test_ids, _read_ci_metadata +from edgacknowledg.forge.sandbox import Sandbox, SandboxUnavailableError from edgacknowledg.forge.workspace import ( + MAX_PATTERNS, UnsafeArchiveError, WorkspaceError, + _central_directory_size, + _communicate_bounded, changes_patterns, ensure_real_directory, extract_artifact, @@ -87,6 +90,66 @@ def test_extract_caps_inner_tarballs(tmp_path: Path) -> None: extract_artifact(archive, destination, max_bytes = 10_000) +def test_extract_caps_entries(tmp_path: Path, + monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr(workspace, 'MAX_ARTIFACT_ENTRIES', 3) + destination = tmp_path / 'out' + destination.mkdir() + archive = _zip(tmp_path / 'a.zip', {f"{n}.txt": b'' for n in range(4)}) + with pytest.raises(UnsafeArchiveError, match = 'more than 3 entries'): + extract_artifact(archive, destination, max_bytes = 10_000) + + # Inner tarballs' entries count too. + payload = io.BytesIO() + with tarfile.open(fileobj = payload, mode = 'w:gz') as tarball: + for n in range(2): + tarball.addfile(tarfile.TarInfo(f"{n}.txt"), io.BytesIO()) + archive = _zip(tmp_path / 'b.zip', { + 'a.txt': b'', + 't.rt.tar.gz': payload.getvalue(), + }) + destination = tmp_path / 'out2' + destination.mkdir() + with pytest.raises(UnsafeArchiveError, match = 'more than 3 entries'): + extract_artifact(archive, destination, max_bytes = 10_000) + + +def test_extract_caps_the_central_directory( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch) -> None: + # zipfile would read it all before the entries could be counted. + archive = _zip(tmp_path / 'a.zip', {f"{n}.txt": b'' for n in range(10)}) + size = _central_directory_size(archive) + assert size is not None + monkeypatch.setattr(workspace, 'MAX_CENTRAL_DIRECTORY_BYTES', size - 1) + destination = tmp_path / 'out' + destination.mkdir() + with pytest.raises(UnsafeArchiveError, match = 'central directory'): + extract_artifact(archive, destination, max_bytes = 10_000) + assert list(destination.iterdir()) == [] + + +def _zipfile_directory_size(path: Path) -> int: + with open(path, 'rb') as handle: + end_record = zipfile._EndRecData(handle) # type: ignore[attr-defined] + return int(end_record[zipfile._ECD_SIZE]) # type: ignore[attr-defined] + + +def test_central_directory_size_matches_zipfiles(tmp_path: Path) -> None: + plain = _zip(tmp_path / 'plain.zip', {'a.txt': b'a', 'b/c.txt': b'c'}) + commented = tmp_path / 'commented.zip' + with zipfile.ZipFile(commented, 'w') as archive: + archive.writestr('a.txt', b'a') + # Holding what looks like another end record. + archive.comment = b'PK\x05\x06' + b'x' * 1000 + # Too many entries for the plain end record: a ZIP64 one. + zip64 = _zip(tmp_path / 'zip64.zip', {f"{n}": b'' for n in range(0x10000)}) + for path in (plain, commented, zip64): + assert _central_directory_size(path) == _zipfile_directory_size(path) + (tmp_path / 'not.zip').write_bytes(b'not a zip') + assert _central_directory_size(tmp_path / 'not.zip') is None + + def test_extract_valid_artifact(tmp_path: Path) -> None: archive = _zip(tmp_path / 'a.zip', { 'ci-meta.json': b'{}', @@ -113,9 +176,10 @@ def test_ensure_real_directory_refuses_symlinks(tmp_path: Path) -> None: def test_test_patterns() -> None: + # Recordings are where the test runner keeps them: named for the part of + # the test's name before its first dot. assert patterns_for_test(PurePosixPath('tests/tests/s/d/a.sft.cpp')) == [ '/tests/tests/s/d/a.sft.cpp', - '/tests/tests/s/d/.a.sft.rto/', '/tests/tests/s/d/.a.rto/', ] # Multi-file tests own their directory. @@ -150,14 +214,115 @@ def test_changes_patterns_cover_recordings_and_expectations() -> None: assert not any(p.startswith('/dev_tools/') for p in patterns) +def test_test_patterns_are_linear_in_the_path() -> None: + # Two, however many dots its name has. + name = '.'.join(['x'] * 2100) + '.sft.cpp' + assert patterns_for_test(PurePosixPath('tests/tests/s', name)) == [ + f"/tests/tests/s/{name}", + '/tests/tests/s/.x.rto/', + ] + + +def test_changes_patterns_skip_suites_no_suite_may_be_named() -> None: + # A suite is in the patterns of every test its expectation file names. + def expectations(suite: str) -> str: + log = f"tests/expectations/edg_x86_64/{suite}.log" + return '\n'.join([ + f"diff --git a/{log} b/{log}", + f"--- a/{log}", + f"+++ b/{log}", + '+a.sft.cpp: PASS', + ]) + + assert '/tests/tests/cwg/a.sft.cpp' in changes_patterns(expectations('cwg')) + for suite in ['s' * 65, 'a.b', 'a b']: + assert changes_patterns(expectations(suite)) == [ + '/tests/.edgy/', '/tests/expectations/', + ] + + +def test_patterns_are_capped() -> None: + # Repeats aren't counted. + failures_patterns(['s/a.sft.cpp'] * (MAX_PATTERNS + 1)) + with pytest.raises(WorkspaceError, match = 'sparse-checkout patterns'): + failures_patterns([f"s/{n}.sft.cpp" for n in range(MAX_PATTERNS)]) + diff = '\n'.join([ + 'diff --git a/tests/expectations/x/s.log b/tests/expectations/x/s.log', + *(f"+{n}.sft.cpp: FAIL" for n in range(MAX_PATTERNS)), + ]) + with pytest.raises(WorkspaceError, match = 'sparse-checkout patterns'): + changes_patterns(diff) + + +async def test_git_output_can_be_bounded() -> None: + async def run(*argv: str, limit: int) -> object: + process = await asyncio.create_subprocess_exec( + *argv, + stdout = asyncio.subprocess.PIPE, + stderr = asyncio.subprocess.PIPE + ) + result = await _communicate_bounded(process, limit) + assert process.returncode is not None + return result + + assert await run('sh', '-c', 'printf out; printf err >&2', + limit = 3) == (b'out', b'err') + # One that would never stop is stopped (letting it clean up). + process = await asyncio.create_subprocess_exec( + 'yes', + stdout = asyncio.subprocess.PIPE, + stderr = asyncio.subprocess.PIPE + ) + assert await _communicate_bounded(process, 1024 * 1024) is None + assert process.returncode == -signal.SIGTERM + + +async def test_cancelled_git_output_reads_stop_git() -> None: + # As when the service stops while git still has output to write: left + # writing to a pipe no one reads, it would never exit. + process = await asyncio.create_subprocess_exec( + 'yes', + stdout = asyncio.subprocess.PIPE, + stderr = asyncio.subprocess.PIPE + ) + reading = asyncio.create_task(_communicate_bounded(process, 1 << 40)) + await asyncio.sleep(0.2) + reading.cancel() + await asyncio.wait_for(asyncio.gather(reading, return_exceptions = True), 10) + assert process.returncode == -signal.SIGTERM + + +def test_ci_reads_are_capped(tmp_path: Path, + monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr(processor, 'MAX_TEST_IDS_BYTES', 20) + first = tmp_path / 'a.elist' + first.write_text('s/a.sft.cpp\n\n') + second = tmp_path / 'b.elist' + second.write_text('s/b.sft.cpp\n') + assert _changes_test_ids([first]) == ['s/a.sft.cpp'] + # Within the limit together, not each. + with pytest.raises(WorkspaceError, match = 'over 20 bytes'): + _changes_test_ids([first, second]) + + monkeypatch.setattr(processor, 'MAX_CI_METADATA_BYTES', 30) + metadata = tmp_path / 'ci-meta.json' + assert _read_ci_metadata(metadata) == {} + metadata.write_text('{"tested_sha": "abc"}') + assert _read_ci_metadata(metadata) == {'tested_sha': 'abc'} + metadata.write_text('[1]') + assert _read_ci_metadata(metadata) == {} + metadata.write_text('{"tested_sha": "' + 'a' * 40 + '"}') + with pytest.raises(WorkspaceError, match = 'over 30 bytes'): + _read_ci_metadata(metadata) + + def test_failures_patterns_ignore_escaping_ids() -> None: patterns = failures_patterns(['s/a.sft.cpp', '../../etc/passwd']) assert '/tests/tests/s/a.sft.cpp' in patterns assert not any('etc' in pattern for pattern in patterns) -async def test_sandbox_requires_its_runners(tmp_path: Path, - unreachable_url: str) -> None: +async def test_sandbox_requires_its_runners(tmp_path: Path) -> None: # A sandbox runs nothing itself: without a runner, it refuses. sandbox = Sandbox() with pytest.raises(SandboxUnavailableError, match = 'no runner'): @@ -166,8 +331,3 @@ async def test_sandbox_requires_its_runners(tmp_path: Path, sandbox.context_for(tmp_path) with pytest.raises(SandboxUnavailableError, match = 'builder runner'): await sandbox.run(['true'], timeout = 5) - - # One that doesn't answer fails the check. - unreachable = Sandbox(builder = RunnerClient(unreachable_url)) - with pytest.raises(SandboxUnavailableError, match = "didn't answer"): - await unreachable.check() diff --git a/dev_tools/services/acknowledg-forge/tests/test_state.py b/dev_tools/services/acknowledg-forge/tests/test_state.py index 067128f085..376fcbfc07 100644 --- a/dev_tools/services/acknowledg-forge/tests/test_state.py +++ b/dev_tools/services/acknowledg-forge/tests/test_state.py @@ -8,19 +8,26 @@ import hashlib import hmac import json +import shutil import zipfile from http import HTTPStatus from pathlib import Path +import pytest + from edgacknowledg.forge import ( ArtifactInfo, CIConclusion, + PullRequestState, PullRequestTouched, CIRunTouched, ) +from edgacknowledg.forge.config import ConfigError from edgacknowledg.forge.db import ForgeStore -from edgacknowledg.forge.webhooks import EventQueue, create_app +from edgacknowledg.forge.serving import HEALTH_PATH +from edgacknowledg.forge.serving import create_app +from edgacknowledg.forge.webhooks import EventQueue, create_receiver from conftest import ( Harness, @@ -72,12 +79,36 @@ async def test_failures_back_off_then_give_up(store: ForgeStore, ) record = await store.find_pull_request('fake', REPOSITORY, 1) assert record is not None - for attempt in range(3): - gave_up = await store.mark_failed( - record.id, record.generation, 'boom', + + async def attempt() -> bool: + return await store.begin_attempt( + record.id, record.generation, now = clock(), max_attempts = 3, base_delay = 60, max_delay = 3600 ) - assert gave_up == (attempt == 2) + + # An attempt counts before it starts, so one that never finishes (the + # service killed part-way) counts too, and isn't retried at once. + assert not await attempt() + assert await store.due_pull_requests('fake', clock()) == [] + record = await store.get_pull_request(record.id) + assert record is not None and record.error_count == 1 + assert record.last_error is not None + assert 'never finished' in record.last_error + + # One taken back (not the pull request's fault) doesn't count. + before = record + assert not await attempt() + await store.refund_attempt(before) + record = await store.get_pull_request(record.id) + assert record is not None and record.error_count == 1 + + # Failed ones count, then it gives up. + for _ in range(2): + assert not await attempt() + await store.mark_failed(record.id, record.generation, 'boom') + record = await store.get_pull_request(record.id) + assert record is not None and record.last_error == 'boom' + assert await attempt() assert await store.due_pull_requests('fake', clock()) == [] # A new push makes it due again immediately. @@ -88,7 +119,7 @@ async def test_failures_back_off_then_give_up(store: ForgeStore, assert len(await store.due_pull_requests('fake', clock())) == 1 -async def test_publisher_outage_defers_without_counting( +async def test_publisher_outage_leaves_serving_to_connecting( tmp_path: Path, origin: Origin, forge: FakeForge, @@ -100,15 +131,89 @@ async def test_publisher_outage_defers_without_counting( forge.set_pull_request(pull_request_info(1, head, origin.main_sha)) harness.publisher.available = False await harness.step() + # Processed all the same: the database leads, and the publisher client + # serves what it stores on connecting. record = await store.find_pull_request('fake', REPOSITORY, 1) - assert record is not None and record.is_dirty and record.error_count == 0 + assert record is not None + assert not record.is_dirty and record.error_count == 0 + assert [s.name for s in await store.sections(record.id)] == [ + 'Test - Changes' + ] + assert harness.publisher.served_names(record.review_id) == [] harness.publisher.available = True + await harness.publisher.sync_all() + assert harness.publisher.served_names(record.review_id) == [ + 'Test - Changes' + ] + + +async def test_publisher_outage_defers_tearing_down_without_counting( + tmp_path: Path, + origin: Origin, + forge: FakeForge, + store: ForgeStore, + clock: Clock) -> None: + harness = Harness(store, forge, make_config(tmp_path / 'home'), clock) + await harness.start() + head = origin.push_pull_request(1, PR_FILES) + forge.set_pull_request(pull_request_info(1, head, origin.main_sha)) + await harness.step() + record = await store.find_pull_request('fake', REPOSITORY, 1) + assert record is not None + + # Its files stay until the publisher stops serving them. + forge.update_pull_request(1, state = PullRequestState.CLOSED) + harness.publisher.available = False await harness.step() + closed = await store.find_pull_request('fake', REPOSITORY, 1) + assert closed is not None and closed.is_dirty and closed.error_count == 0 + assert harness.workspaces.review_dir(record.review_id).exists() assert harness.publisher.served_names(record.review_id) == [ 'Test - Changes' ] + harness.publisher.available = True + await harness.step() + assert not harness.workspaces.review_dir(record.review_id).exists() + assert harness.publisher.served_names(record.review_id) == [] + + +async def test_connecting_stops_serving_what_is_gone(tmp_path: Path, + origin: Origin, + forge: FakeForge, + store: ForgeStore, + clock: Clock) -> None: + config = make_config(tmp_path / 'home') + harness = Harness(store, forge, config, clock) + await harness.start() + head = origin.push_pull_request(1, PR_FILES) + forge.set_pull_request(pull_request_info(1, head, origin.main_sha)) + await harness.step() + record = await store.find_pull_request('fake', REPOSITORY, 1) + assert record is not None + assert harness.publisher.served_names(record.review_id) == [ + 'Test - Changes' + ] + + # Its files go while the service is down; the publisher, which outlasts + # it, still serves them until the service connects again. + [section] = harness.publisher.reviews[record.review_id] + shutil.rmtree(section.root) + restarted = Harness(store, forge, config, clock, + publisher = harness.publisher, + commenter = harness.commenter) + await restarted.start() + assert restarted.publisher.served_names(record.review_id) == [] + record = await store.find_pull_request('fake', REPOSITORY, 1) + assert record is not None and record.is_dirty + + # Rebuilt, it is served again. + await restarted.step() + assert restarted.publisher.served_names(record.review_id) == [ + 'Test - Changes' + ] + async def test_settle_deadline_announces_without_ci(tmp_path: Path, origin: Origin, @@ -240,7 +345,10 @@ def parse(self, headers: dict[str, str], body: bytes) -> list[object]: async def test_webhook_endpoint_verifies_and_queues() -> None: queue = EventQueue() - app = create_app(_SignedWebhooks(b'secret'), queue) # type: ignore[arg-type] + app = create_app(create_receiver( + _SignedWebhooks(b'secret'), # type: ignore[arg-type] + queue + )) client = app.test_client() body = json.dumps({'repository': REPOSITORY, 'number': 3}).encode() @@ -257,8 +365,31 @@ async def test_webhook_endpoint_verifies_and_queues() -> None: assert response.status_code == HTTPStatus.ACCEPTED assert queue._queue.get_nowait() == PullRequestTouched(REPOSITORY, 3) - response = await client.get('/healthz') + response = await client.get(HEALTH_PATH) + assert response.status_code == HTTPStatus.OK + + +async def test_app_without_webhooks_answers_only_health_checks() -> None: + # As the service serves it without ACKNOWLEDG_FORGE_WEBHOOKS. + client = create_app().test_client() + response = await client.get(HEALTH_PATH) assert response.status_code == HTTPStatus.OK + assert await response.get_data(as_text = True) == 'ok\n' + response = await client.post('/webhook', data = b'{}') + assert response.status_code == HTTPStatus.NOT_FOUND + + +def test_webhooks_need_the_service_bind(tmp_path: Path) -> None: + config = make_config(tmp_path / 'home', SERVICE_BIND = '127.0.0.1:0', + WEBHOOKS = '1') + assert config.service_bind == '127.0.0.1:0' and config.webhooks + config = make_config(tmp_path / 'home', SERVICE_BIND = '127.0.0.1:0') + assert not config.webhooks + with pytest.raises(ConfigError, match = 'SERVICE_BIND is not'): + make_config(tmp_path / 'home', WEBHOOKS = '1') + # The old setting isn't silently ignored. + with pytest.raises(ConfigError, match = 'WEBHOOK_BIND is gone'): + make_config(tmp_path / 'home', WEBHOOK_BIND = '127.0.0.1:0') async def test_bench_sections_from_ci_artifact(tmp_path: Path, diff --git a/dev_tools/services/acknowledg-forge/tests/test_symlinks.py b/dev_tools/services/acknowledg-forge/tests/test_symlinks.py index 9c956747e1..a62390d5d4 100644 --- a/dev_tools/services/acknowledg-forge/tests/test_symlinks.py +++ b/dev_tools/services/acknowledg-forge/tests/test_symlinks.py @@ -44,7 +44,7 @@ from edgacknowledg.forge.db import ForgeStore, SectionRecord from edgtest.runtest import RunTestOutput, RunTestOutputLinkError from edgacknowledg.forge.publisher import KIND_BENCH, KIND_TEST_FAILURES -from edgacknowledg.forge.publisher.acknowledg import AcknowledgPublisher +from edgacknowledg.forge.publisher.acknowledg import _build_handler from edgacknowledg.forge.publisher.pool import ProcessPool from edgacknowledg.forge.workspace import ( WorkspaceError, @@ -324,17 +324,31 @@ def channel() -> FakeChannel: message(RequestTags.ILLEGAL_ACCESS, value = {'reviewer_id': 'reviewer-1'}) ] - # ...unless it can't be traced to one, like any other failure. + # ...unless it can't be traced to one: like any other failure, with no + # reviewer to tell, it is only logged. for send in ( lambda c: _report_illegal_access( # type: ignore c, handler, IllegalFileAccessError('outside')), - lambda c: _reply_to_failed_request(c, handler)): # type: ignore - invalid = channel() + lambda c: _reply_to_failed_request( # type: ignore + c, handler, ValueError('broken'))): + untraced = channel() try: raise ValueError('broken') except ValueError: - await send(invalid) - assert invalid.sent == [message(RequestTags.INVALID_REQUEST)] + await send(untraced) + assert untraced.sent == [] + + # A failure traced to a reviewer is reported to them. + failed = channel() + error = ValueError('broken') + setattr(error, 'reviewer_id', 'reviewer-1') + try: + raise error + except ValueError: + await _reply_to_failed_request(failed, handler, error) # type: ignore + assert failed.sent == [ + message(RequestTags.INVALID_REQUEST, value = {'reviewer_id': 'reviewer-1'}) + ] def test_confined_path(tmp_path: Path, secret: Path) -> None: @@ -620,26 +634,17 @@ def test_permitted_roots(tmp_path: Path, secret: Path) -> None: def test_publisher_requires_real_section_directories( tmp_path: Path, secret: Path, - unreachable_port: int, unreachable_url: str) -> None: reviews = tmp_path / 'home' / 'reviews' worktree = reviews / str(uuid.uuid4()) / 'head' (worktree / 'benchmarks' / 'runs' / 'ci-1' / 'c').mkdir(parents = True) - publisher = AcknowledgPublisher( - api_host = '127.0.0.1', - api_port = unreachable_port, - api_key = None, - ui_host = 'ui.example', - ui_port = 443, - # Never started: no request is answered here. - pool = ProcessPool(tools_service_url = unreachable_url), - reviews_dir = reviews - ) + # Never started: no request is answered here. + pool = ProcessPool(tools_service_url = unreachable_url) # A test home that is a link (as if a pull request committed 'tests'). os.symlink(secret.parent, worktree / 'tests') with pytest.raises(WorkspaceError): - publisher._build_handler(SectionRecord( + _build_handler(SectionRecord( kind = KIND_TEST_FAILURES, name = 'Test - Failures', slug = 'test-failures', @@ -647,12 +652,12 @@ def test_publisher_requires_real_section_directories( content_fingerprint = 'f', root = worktree / 'tests' / 'runs' / 'ci-1', home = worktree / 'tests', - )) + ), pool, reviews) # Baselines outside the bench home. (tmp_path / 'elsewhere').mkdir() with pytest.raises(WorkspaceError): - publisher._build_handler(SectionRecord( + _build_handler(SectionRecord( kind = KIND_BENCH, name = 'Bench - C', slug = 'bench-c', @@ -664,7 +669,7 @@ def test_publisher_requires_real_section_directories( 'baseline_dir': str(tmp_path / 'elsewhere'), 'tag': 'release-c', }, - )) + ), pool, reviews) def test_bench_session_trees(tmp_path: Path) -> None: diff --git a/dev_tools/services/acknowledg/server/acknowledg_server/__init__.py b/dev_tools/services/acknowledg/server/acknowledg_server/__init__.py index b7244f4c0e..4fa04997c6 100644 --- a/dev_tools/services/acknowledg/server/acknowledg_server/__init__.py +++ b/dev_tools/services/acknowledg/server/acknowledg_server/__init__.py @@ -671,6 +671,21 @@ async def _receive_provider_replies( ): continue + if request_tag == RequestTags.INVALID_REQUEST: + # A provider failed a reviewer's request: tell the reviewer (only + # its own reviewers can be reached, as they are per review). One + # naming no reviewer isn't answered with another error: the two + # would bounce back and forth for as long as the section is served. + try: + failed_for = uuid.UUID(str(request_value['reviewer_id'])) + except (KeyError, TypeError, ValueError): + continue + await run_descr.publish_to_reviewer( + failed_for, + message(RequestTags.INVALID_REQUEST, section = section) + ) + continue + if request_tag == RequestTags.ILLEGAL_ACCESS: try: offender = uuid.UUID(str(request_value['reviewer_id'])) diff --git a/dev_tools/tests/test_edgtest_suites.py b/dev_tools/tests/test_edgtest_suites.py new file mode 100644 index 0000000000..ea981ddb96 --- /dev/null +++ b/dev_tools/tests/test_edgtest_suites.py @@ -0,0 +1,42 @@ +# Part of the EDG Compiler Project, under the Apache License v2.0 with LLVM +# Exceptions. +# See https://edgcpp.org/LICENSE.txt for license information. +# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +'''Tests for the test suite names edgtest accepts (which edgy checks on +loading .edgy/config.json). + +Run with: python3 -m unittest discover -s dev_tools/tests +(with dev_tools/pylibs on PYTHONPATH). +''' + +import json +import unittest + +import edgtest + +from pathlib import Path + +REPO_DIR = Path(__file__).resolve().parents[2] + +class SuiteNameTests(unittest.TestCase): + def test_valid_names(self) -> None: + for name in ['cwg', 'edg_x86', 'cpp-23', 'A1', 's' * 64]: + with self.subTest(name = name): + self.assertTrue(edgtest.is_valid_suite_name(name)) + + def test_invalid_names(self) -> None: + for name in ['', 's' * 65, 'a.b', 'a/b', '..', 'a b', 'café', + 'cwg\n']: + with self.subTest(name = name): + self.assertFalse(edgtest.is_valid_suite_name(name)) + + def test_the_test_homes_suites_are_valid(self) -> None: + config_path = REPO_DIR / 'tests' / '.edgy' / 'config.json' + test_suites = json.loads(config_path.read_text())['test_suites'] + for name in [*test_suites['primary'], *test_suites['secondary']]: + with self.subTest(name = name): + self.assertTrue(edgtest.is_valid_suite_name(name)) + +if __name__ == '__main__': + unittest.main() diff --git a/dev_tools/tests/test_edgutil.py b/dev_tools/tests/test_edgutil.py new file mode 100644 index 0000000000..d9155298e6 --- /dev/null +++ b/dev_tools/tests/test_edgutil.py @@ -0,0 +1,122 @@ +# Part of the EDG Compiler Project, under the Apache License v2.0 with LLVM +# Exceptions. +# See https://edgcpp.org/LICENSE.txt for license information. +# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +'''Tests for edgutil. + +Run with: python3 -m unittest discover -s dev_tools/tests +(with dev_tools/pylibs on PYTHONPATH). +''' + +import asyncio +import os +import signal +import subprocess +import sys +import textwrap +import time +import unittest + +import edgutil + +class TerminateWithinTests(unittest.IsolatedAsyncioTestCase): + async def test_terminates(self) -> None: + process = await asyncio.create_subprocess_exec('sleep', '30') + started = time.monotonic() + status = await edgutil.async_terminate_within(process, 5) + self.assertEqual(status, -signal.SIGTERM) + self.assertLess(time.monotonic() - started, 2) + + async def test_kills_what_outlasts_its_allowance(self) -> None: + # It ignores SIGTERM (as exec keeps ignored signals ignored). + process = await asyncio.create_subprocess_exec( + 'sh', '-c', 'trap "" TERM; echo ready; exec sleep 30', + stdout = asyncio.subprocess.PIPE + ) + assert process.stdout is not None + self.assertEqual(await process.stdout.readline(), b'ready\n') + started = time.monotonic() + status = await edgutil.async_terminate_within(process, 0.5) + self.assertEqual(status, -signal.SIGKILL) + self.assertGreaterEqual(time.monotonic() - started, 0.5) + + async def test_leaves_what_has_exited(self) -> None: + process = await asyncio.create_subprocess_exec('true') + await process.wait() + self.assertEqual(await edgutil.async_terminate_within(process, 5), 0) + +# Each runs in a process of its own: run_asyncio_loop installs its loop (and +# signal handlers) for the process. +_PROGRAM = textwrap.dedent(''' + import asyncio, os, signal, sys + import edgutil + + scenario = sys.argv[1] + + async def clean_up_slowly(name): + try: + await asyncio.sleep(100) + finally: + await asyncio.sleep(0.1) + print(name, 'cleaned up', flush = True) + + async def consumer(loop): + loop.create_task(clean_up_slowly('worker')) + await asyncio.sleep(0.05) + if scenario == 'fatal': + raise edgutil.AsyncLoopFatalError() + os.kill(os.getpid(), signal.SIGINT) + try: + await asyncio.sleep(100) + except asyncio.CancelledError: + # Starting a task while stopping: it's cancelled, and awaited, too. + loop.create_task(clean_up_slowly('latecomer')) + if scenario == 'sigint': + raise + return 'returned' + + try: + print(edgutil.run_asyncio_loop(consumer), flush = True) + except BaseException as ex: + print('raised', type(ex).__name__, flush = True) + loop = edgutil.lazy_init_event_loop() + print('left running:', len(asyncio.all_tasks(loop)), flush = True) +''') + +def _run(scenario: str) -> list[str]: + pylibs = os.path.join(os.path.dirname(__file__), '..', 'pylibs') + completed = subprocess.run( + [sys.executable, '-c', _PROGRAM, scenario], + env = {**os.environ, 'PYTHONPATH': os.path.abspath(pylibs)}, + capture_output = True, + text = True, + timeout = 30, + check = True + ) + return completed.stdout.splitlines() + +class RunAsyncioLoopTests(unittest.TestCase): + '''Stopping: the tasks it cancels finish their cleanup.''' + + def test_fatal_errors_wait_for_cancelled_tasks(self) -> None: + self.assertEqual(_run('fatal'), [ + 'worker cleaned up', + 'raised AsyncLoopFatalError', + 'left running: 0', + ]) + + def test_signals_wait_for_cancelled_tasks(self) -> None: + lines = _run('sigint') + self.assertEqual(sorted(lines[:2]), + ['latecomer cleaned up', 'worker cleaned up']) + self.assertEqual(lines[2:], ['raised CancelledError', 'left running: 0']) + + def test_signals_caught_wait_for_cancelled_tasks(self) -> None: + lines = _run('sigint-caught') + self.assertEqual(sorted(lines[:2]), + ['latecomer cleaned up', 'worker cleaned up']) + self.assertEqual(lines[2:], ['returned', 'left running: 0']) + +if __name__ == '__main__': + unittest.main() diff --git a/dev_tools/typecheck-python.sh b/dev_tools/typecheck-python.sh index 2719027da9..f9fea1df24 100755 --- a/dev_tools/typecheck-python.sh +++ b/dev_tools/typecheck-python.sh @@ -131,7 +131,7 @@ pushd pylibs/ > /dev/null check_types __init__.py popd > /dev/null pushd edgutil/ > /dev/null - check_types __init__.py + check_types_strict __init__.py popd > /dev/null popd > /dev/null