From d45dc041541bd3d94d2c343ac0e1e91f0c287ab3 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. --- .../pylibs/edgacknowledg/client/__init__.py | 45 ++- .../edgacknowledg/client/bench/__init__.py | 16 +- .../pylibs/edgacknowledg/client/shared.py | 13 + .../edgacknowledg/client/test/__init__.py | 17 +- .../pylibs/edgacknowledg/client/websocket.py | 5 +- .../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 | 124 ++++--- .../edgacknowledg/forge/publisher/__init__.py | 50 ++- .../forge/publisher/acknowledg.py | 244 ++++++++------ .../edgacknowledg/forge/publisher/client.py | 185 +++++------ .../edgacknowledg/forge/publisher/pool.py | 281 +++++++++++----- .../forge/publisher/pool_process.py | 38 ++- .../edgacknowledg/forge/publisher/protocol.py | 20 +- .../edgacknowledg/forge/publisher/server.py | 304 +++++++++--------- .../edgacknowledg/forge/runner/client.py | 199 ++++++++---- .../edgacknowledg/forge/runner/server.py | 222 +++++++------ .../pylibs/edgacknowledg/forge/service.py | 68 ++-- .../pylibs/edgacknowledg/forge/serving.py | 140 ++++++++ .../pylibs/edgacknowledg/forge/webhooks.py | 28 +- .../pylibs/edgacknowledg/forge/workspace.py | 156 +++++++-- dev_tools/services/acknowledg-forge/README.md | 10 +- .../services/acknowledg-forge/SANDBOXING.md | 165 +++++----- .../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 | 43 ++- .../tests/test_acknowledg_integration.py | 123 ++++++- .../acknowledg-forge/tests/test_containers.py | 57 ++-- .../acknowledg-forge/tests/test_pool.py | 63 +++- .../acknowledg-forge/tests/test_publisher.py | 276 +++++++++++++--- .../acknowledg-forge/tests/test_runner.py | 87 +++-- .../acknowledg-forge/tests/test_safety.py | 133 +++++++- .../acknowledg-forge/tests/test_state.py | 111 ++++++- .../acknowledg-forge/tests/test_symlinks.py | 47 +-- .../server/acknowledg_server/__init__.py | 15 + 37 files changed, 2420 insertions(+), 1091 deletions(-) create mode 100644 dev_tools/pylibs/edgacknowledg/forge/serving.py diff --git a/dev_tools/pylibs/edgacknowledg/client/__init__.py b/dev_tools/pylibs/edgacknowledg/client/__init__.py index 26d89caec8..504c9fd785 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,24 @@ 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 Exception: + self._review.discard_handler(handler) + raise async def remove_section(self, handler: ReviewClientHandler) -> None: '''Unsubscribe, then unregister.''' @@ -513,8 +530,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) @@ -681,8 +698,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) def _signal_section_ready(self, handler_key: int) -> None: ready = self._section_first_ready.get(handler_key) 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..28477318c2 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 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..0d3e0d8215 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, @@ -43,7 +43,6 @@ KIND_TEST_FAILURES, Publisher, PublisherUnavailableError, - ReviewIdentity, ) from edgacknowledg.forge.sandbox import Sandbox from edgacknowledg.forge.workspace import ( @@ -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,7 +190,9 @@ def busy_pull_requests(self) -> frozenset[int]: # Loop async def run(self) -> None: - '''Restore what was being served, then process changes forever.''' + '''Once the publisher serves what is stored, clean up after a restart, + then process changes forever. + ''' await self._publisher.wait_connected() await self.restore() last_maintenance = self._clock() @@ -198,28 +234,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: # 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 +271,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 +295,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 +319,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 +497,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 +608,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..c830e19815 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,11 @@ import uuid from dataclasses import dataclass -from typing import Optional, Protocol, Sequence +from typing import Optional, Protocol -from edgacknowledg import RichName +from edgacknowledg import 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' @@ -37,7 +36,9 @@ class PublisherUnavailableError(Exception): - '''Raised when AcknowlEDG can't be reached right now (retry later).''' + '''Raised when the publisher can't take a request right now (retry + later). + ''' @dataclass(frozen = True) @@ -47,19 +48,40 @@ class ReviewIdentity: timestamp: str +def review_identity(record: PullRequestRecord) -> ReviewIdentity: + '''The review of a pull request, as published.''' + 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() + ) + + class Publisher(Protocol): - '''Where review sections are served (AcknowlEDG, or a test double).''' + '''Where review sections are served (AcknowlEDG, or a test double): + what is stored in the service's database. + ''' async def wait_connected(self) -> None: ... - async def sync(self, - review: ReviewIdentity, - sections: Sequence[SectionRecord]) -> None: - '''Serve exactly ``sections`` for ``review``.''' + 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. + ''' ... - 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). + ''' ... 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..a27a084ff5 100644 --- a/dev_tools/pylibs/edgacknowledg/forge/publisher/client.py +++ b/dev_tools/pylibs/edgacknowledg/forge/publisher/client.py @@ -11,20 +11,20 @@ import asyncio import json import logging -import time import uuid from pathlib import Path from typing import Any, Optional, Sequence from websockets.asyncio.client import ClientConnection, connect -from websockets.exceptions import ConnectionClosed, InvalidHandshake +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 ( @@ -37,9 +37,7 @@ 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. - ''' + '''The client's end of a connection to the publisher server.''' def __init__(self, connection: ClientConnection) -> None: self._connection = connection @@ -60,7 +58,6 @@ async def receive(self) -> Optional[bytes]: 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: @@ -70,11 +67,13 @@ async def close(self) -> None: 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. + 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``). ''' def __init__(self, @@ -86,9 +85,9 @@ 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: + store: ForgeStore, + forge_name: str, + request_timeout: float = 15 * 60.0) -> None: self._url = url self._api_host = api_host self._api_port = api_port @@ -96,22 +95,19 @@ def __init__(self, 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. + # The current connection, if any; set once it has served every review + # stored in the database. self._connection: Optional[_Connection] = None self._connected = asyncio.Event() - # Serializes requests (so at most one awaits a reply), and replaying - # after reconnecting. + # Serializes requests (so at most one awaits a reply), and serving the + # reviews stored in the database on connecting. 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 + self._sync_all_task: Optional[asyncio.Task[None]] = None # Publisher @@ -123,20 +119,25 @@ def section_url(self, 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) + raise PublisherUnavailableError( + 'not connected to the publisher server' + ) + 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}") + await self._sync(review_identity(record), + await self._store.sections(record.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._connected.is_set(): + raise PublisherUnavailableError( + 'not connected to the publisher server' + ) + await self._request({'op': 'delist', 'review_id': str(review_id)}) async def _sync(self, review: ReviewIdentity, @@ -146,10 +147,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 @@ -185,8 +182,6 @@ async def _request(self, request: dict[str, Any]) -> None: self._reply = None error = answer.get('error') - if error == 'unavailable': - raise PublisherUnavailableError(str(answer.get('message'))) if error is not None: raise RuntimeError(f"publisher server: {answer.get('message')}") @@ -198,39 +193,48 @@ def _handle_message(self, line: bytes) -> 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(): + if 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]) - 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. + async def sync_all(self) -> None: + '''On connecting, 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 serving failed) it stops serving, and marks to be rebuilt; since + section fingerprints are content-derived, rebuilding yields the same + state (and so no comment). ''' async with self._lock: - if self._needs_replay: - for review, sections in list(self._desired.values()): + 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 + review = review_identity(record) try: + if not all(section.root.is_dir() for section in sections): + raise FileNotFoundError('its files are gone') 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') + served.append(str(review.review_id)) + except PublisherUnavailableError: + raise + except Exception as ex: + _log.warning('rebuilding %s#%s: %s', record.info.repository, + record.info.number, ex) + await self._sync(review, []) + await self._store.mark_dirty(record.id) + await self._request({'op': 'prune', 'keep': served}) + except PublisherUnavailableError as ex: + _log.warning('serving the reviews failed (%s); will retry on the ' + 'next connection', ex) + return + _log.info('connected to the publisher server') self._connected.set() # Connection @@ -240,48 +244,28 @@ 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. + '''Stay connected to the publisher server, until cancelled: the + websockets library reconnects whenever the connection goes, backing off + exponentially while it can't reach the server. ''' self._reviews_dir.mkdir(parents = True, exist_ok = True) - delay = self._restart_delay - while True: - started = time.monotonic() - 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() + async for connection in connect( + self._url, + max_size = MESSAGE_LIMIT, + open_timeout = DEFAULT_OPEN_TIMEOUT_S): + await self._run_connection(_Connection(connection)) + _log.warning('lost the publisher server; reconnecting') + + async def _run_connection(self, connection: _Connection) -> None: + '''Serve ``connection`` until it goes.''' self._connection = connection try: await connection.send(self._init_message()) + self._sync_all_task = asyncio.create_task(self.sync_all()) while self._connection is connection: message = await connection.receive() if message is None: @@ -291,12 +275,11 @@ async def _run_connection(self) -> None: 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() + if self._sync_all_task is not None: + self._sync_all_task.cancel() diff --git a/dev_tools/pylibs/edgacknowledg/forge/publisher/pool.py b/dev_tools/pylibs/edgacknowledg/forge/publisher/pool.py index 4e6c949b78..4459f632e2 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") @@ -242,16 +343,22 @@ 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 @@ -264,22 +371,29 @@ async def _start(self, slot: _Slot) -> None: slot.failed_starts = 0 _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 +424,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..5952ee0e45 100644 --- a/dev_tools/pylibs/edgacknowledg/forge/runner/client.py +++ b/dev_tools/pylibs/edgacknowledg/forge/runner/client.py @@ -7,9 +7,16 @@ 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, which every command it runs shares: the websockets library +reconnects whenever it goes, backing off exponentially while it can't reach +the runner. 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 command waits for the +connection, within its own time limit; the connection going fails the +commands in flight on it, as the runner kills them. ''' import asyncio @@ -17,10 +24,11 @@ import logging from pathlib import Path -from typing import Any, Callable, Mapping, Optional, Sequence +from types import TracebackType +from typing import Any, Callable, Mapping, Optional, Sequence, Type from websockets.asyncio.client import ClientConnection, connect -from websockets.exceptions import ConnectionClosed, InvalidHandshake +from websockets.exceptions import ConnectionClosed from edgacknowledg.client.shared import CompletedCommand from edgacknowledg.forge.runner.server import ID_BYTES, OUTPUT_CHUNK @@ -31,6 +39,10 @@ # (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'] @@ -39,6 +51,37 @@ class RunnerError(Exception): '''The runner couldn't be reached, or 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.''' @@ -51,24 +94,58 @@ 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``); a connection takes at most ``open_timeout`` + seconds to open, and a runner that can't be reached fails ``describe`` + within it. ''' - def __init__(self, url: str) -> None: + def __init__(self, + url: str, + *, + open_timeout: float = _OPEN_TIMEOUT_S) -> None: self.url = url + self._open_timeout = open_timeout + # The task keeping the connection, while the client 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 - self._reader: Optional[asyncio.Task[None]] = None - self._connecting: Optional[asyncio.Lock] = None + self._connected = asyncio.Event() + # 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``).''' + if self._running is None: + self._running = asyncio.create_task(self._keep_connected()) + + async def close(self) -> None: + '''Close the connection (killing whatever it still runs), and stop + keeping it. + ''' + if self._running is not None: + self._running.cancel() + await asyncio.gather(self._running, return_exceptions = True) + self._running = None + + 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) + timeout = self._open_timeout) return dict(value['description']) async def run(self, @@ -80,12 +157,13 @@ async def run(self, 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. + to ``output`` as they come); its output beyond ``MAX_OUTPUT_BYTES`` is + dropped from the middle. Cancelling this kills the command. ''' - collected = bytearray() + collected = _Output() def collect(chunk: bytes) -> None: - collected.extend(chunk) + collected.add(chunk) if output is not None: output(chunk) @@ -102,7 +180,7 @@ def collect(chunk: bytes) -> None: ) return CompletedCommand( int(value['returncode']), - bytes(collected), + collected.value(), bool(value['timed_out']) ) @@ -112,36 +190,29 @@ 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 - ) - 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 _keep_connected(self) -> None: + '''Keep a connection to the runner, until cancelled.''' + async for connection in connect( + self.url, + open_timeout = self._open_timeout, + max_size = ID_BYTES + OUTPUT_CHUNK + 1024): + self._connection = connection + self._connected.set() + try: + await self._read(connection) + finally: + self._connected.clear() + 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}") + ) + _log.warning('lost the runner at %s; reconnecting', self.url) 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. ''' try: async for message in connection: @@ -157,16 +228,6 @@ async def _read(self, connection: ClientConnection) -> None: 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], @@ -174,30 +235,34 @@ async def _request(self, timeout: float, output: Optional[Callable[[bytes], None]] = None ) -> dict[str, Any]: - '''Send ``request``; pass any output to ``output`` and return the final - answer. + '''Send ``request`` once connected; pass any output to ``output`` and + return the final answer, all within ``timeout`` seconds. ''' - connection = await self._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 + connection: Optional[ClientConnection] = None try: - try: + async with asyncio.timeout(timeout): + await self._connected.wait() + connection = self._connection + assert connection is not None + self._pending[request_id] = pending await connection.send(json.dumps({'id': request_id, **request})) - value = await asyncio.wait_for(asyncio.shield(pending.answer), - timeout) - except asyncio.TimeoutError: + value = await asyncio.shield(pending.answer) + except TimeoutError: + if connection is not None: 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. + 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. + if connection is not None: 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 + 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..8656e771ca 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,10 @@ from pathlib import Path from typing import Any, Mapping, Optional, Sequence -from websockets.asyncio.server import Server, ServerConnection, serve -from websockets.exceptions import ConnectionClosed +from quart import Blueprint, websocket +from quart.wrappers import ClientDisconnectedError, Websocket +from edgacknowledg.forge import serving from edgacknowledg.forge.runner.roles import ROLES, RunnerPolicy _log = logging.getLogger('edgacknowledg.forge.runner.server') @@ -189,96 +191,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 +255,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 +274,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 +376,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/service.py b/dev_tools/pylibs/edgacknowledg/forge/service.py index 604e3fa004..ca27dc9d84 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 @@ -165,15 +149,23 @@ async def _run_components(config: ForgeServiceConfig, task_group.create_task(publisher.run()) 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..6c3230068e --- /dev/null +++ b/dev_tools/pylibs/edgacknowledg/forge/serving.py @@ -0,0 +1,140 @@ +# 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).''' + await asyncio.shield(self._task) + + async def close(self) -> None: + self._stop.set() + try: + 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..aa7b38e919 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 @@ -56,6 +57,22 @@ BENCH_BASELINE_PATTERNS = ('/benchmarks/baselines/',) +# The longest file name (in bytes) a test can have; a longer one names no +# file, so it needs no pattern (and its patterns would be quadratic in its +# length, one per dot, each as long as the name). +_NAME_MAX = 255 +# The most sparse-checkout patterns a checkout may have: matching costs +# patterns x paths, and they are built from pull request content. +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 + class WorkspaceError(Exception): '''Raised when a workspace operation fails or input is unsafe.''' @@ -90,6 +107,8 @@ def patterns_for_test(test_path: PurePosixPath) -> list[str]: as 'tests/tests/suite/dir/name.sft.cpp'): the test file and its recordings, or the whole directory of a multi-file test. ''' + if len(test_path.name.encode()) > _NAME_MAX: + return [] directory = _escape_pattern(str(test_path.parent)) if '.mft.' in test_path.name: return [f"/{directory}/"] @@ -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,7 +160,7 @@ 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. @@ -152,27 +171,34 @@ def changes_patterns(diff: str) -> list[str]: 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 +288,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 +353,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 +452,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 +472,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 +589,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 +676,28 @@ 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 + killed it, should its stdout be over ``limit`` bytes. + ''' + 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: + process.kill() + return None + chunks.append(chunk) + return b''.join(chunks), await errors + finally: + errors.cancel() + await process.wait() + + 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/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..2be434f0ef 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,9 @@ 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` | 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 +152,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 +160,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 +197,11 @@ 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, which all its +commands share (each request carries an id its answer and output come back +with), and reconnects whenever it goes, backing off while the runner can't +be reached; a command waits for the connection, within its time limit, and +the connection going fails the commands in flight on it. ### Trade-offs @@ -227,8 +231,8 @@ 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] + start([start]) --> connected1[wait until the publisher client
has served the stored sections] + connected1 --> restore[restore: drop builds
the restart interrupted] restore --> connected2[wait until connected] connected2 --> due[start processing every dirty pull request,
a few at a time] due --> maintain{6 h since the
last maintenance?} @@ -239,7 +243,7 @@ flowchart TB 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] open -->|yes| fetchbuild[fetch it, build its sections] fetchbuild --> sync[sync the sections] sync -->|ok| record[record the sections,
delete superseded runs] @@ -273,14 +277,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 +336,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,43 +367,42 @@ 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] + ready -->|connection closed| exited[fail the request in flight
as unavailable] exited --> backoff[wait 1 s, doubling
up to 60 s; back to 1 s
after a long run] backoff --> spawn ``` @@ -415,8 +425,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 +436,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 +460,25 @@ 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, the pull request is deferred until it can. +- 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..adf65f2100 100644 --- a/dev_tools/services/acknowledg-forge/tests/fakes.py +++ b/dev_tools/services/acknowledg-forge/tests/fakes.py @@ -20,10 +20,11 @@ PullRequestInfo, PullRequestState, ) -from edgacknowledg.forge.db import SectionRecord +from edgacknowledg.forge.db import ForgeStore, SectionRecord from edgacknowledg.forge.publisher import ( PublisherUnavailableError, ReviewIdentity, + review_identity, ) REPOSITORY = 'edg/compiler' @@ -103,9 +104,13 @@ 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 @@ -114,6 +119,35 @@ def __init__(self) -> None: async def wait_connected(self) -> None: pass + async def sync_one(self, review_id: uuid.UUID) -> None: + 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 PublisherUnavailableError('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.sync(review_identity(record), []) + 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: @@ -128,9 +162,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_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..9b2d69321a 100644 --- a/dev_tools/services/acknowledg-forge/tests/test_pool.py +++ b/dev_tools/services/acknowledg-forge/tests/test_pool.py @@ -35,10 +35,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 +59,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 +148,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) @@ -222,12 +270,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..faeb2a7e09 100644 --- a/dev_tools/services/acknowledg-forge/tests/test_publisher.py +++ b/dev_tools/services/acknowledg-forge/tests/test_publisher.py @@ -13,10 +13,13 @@ ''' 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 @@ -29,7 +32,9 @@ rich_link, rich_text, ) -from edgacknowledg.forge.db import SectionRecord +from edgacknowledg.forge import serving +from edgacknowledg.forge.db import ForgeStore, SectionRecord +from edgacknowledg.forge.ingest import utc_now from edgacknowledg.forge.local import LocalDeployment, local_deployment from edgacknowledg.forge.publisher import ( KIND_TEST_CHANGES, @@ -38,12 +43,15 @@ ) 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.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 +112,27 @@ 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]) -> uuid.UUID: + '''Store pull request ``number`` with ``sections`` as served, as the + processor does; its review. + ''' + await store.upsert_pull_request( + FakeForge.name, + pull_request_info(number, 'a' * 40, 'b' * 40), + 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,71 +144,231 @@ 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) + publisher = _client(local.publisher_service_url, reviews, store, + api_port = server.port) task = asyncio.create_task(publisher.run()) 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._connection + assert connection is not None + connection.abort() + await _wait_for(lambda: publisher._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: + task.cancel() + await asyncio.gather(task, return_exceptions = True) + + +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) + task = asyncio.create_task(publisher.run()) + 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: + task.cancel() + await asyncio.gather(task, return_exceptions = True) + + +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) + task = asyncio.create_task(earlier.run()) + 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: + task.cancel() + await asyncio.gather(task, return_exceptions = True) + + # 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) + task = asyncio.create_task(later.run()) + try: + await asyncio.wait_for(later.wait_connected(), 30) + await _wait_for_sources(server.port, review_id, []) + finally: + task.cancel() + await asyncio.gather(task, return_exceptions = True) + await other_store.aclose() + - # 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_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) + task = asyncio.create_task(publisher.run()) + 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: + task.cancel() + await asyncio.gather(task, return_exceptions = True) + - 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) + task = asyncio.create_task(publisher.run()) + 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) +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) + connection = asyncio.create_task(client.run()) 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) + await asyncio.wait_for(client.wait_connected(), 30) + await asyncio.wait_for(client.sync_one(await _store(store, 1, [])), 30) assert not connection.done() finally: connection.cancel() await asyncio.gather(connection, return_exceptions = True) - listening.close() - await listening.wait_closed() - await publisher_server.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: + 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: + await listening.close() class _SilentConnection: @@ -210,24 +393,25 @@ 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, '') + review_id = await _store(store, 1, [section]) with pytest.raises(PublisherUnavailableError, match = 'stopped answering'): - await publisher.sync(review, [section]) + await publisher.sync_one(review_id) assert connection.aborted # 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]) + await publisher.sync_one(review_id) assert len(connection.sent) == 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..17b3387658 100644 --- a/dev_tools/services/acknowledg-forge/tests/test_runner.py +++ b/dev_tools/services/acknowledg-forge/tests/test_runner.py @@ -13,9 +13,10 @@ import asyncio 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 +24,8 @@ import pytest import pytest_asyncio +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 +33,19 @@ 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.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 +75,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 +163,26 @@ 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) + streamed = bytearray() + result = await runner.client.run( + ['sh', '-c', 'printf 0123456789; printf abcdefgh; printf WXYZ'], + timeout = 10, + output = streamed.extend + ) + # Its start, and its end (where its errors are), within the limit. + assert result.output == b'012345\n[... 12 bytes of output dropped ...]\nWXYZ' + # What streams isn't cut. + assert streamed == b'0123456789abcdefghWXYZ' + + 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') @@ -241,11 +256,13 @@ async def test_losing_the_connection_kills_its_commands( timeout = 60 )) command = await _started(pid_file) - await runner.client.close() + connection = runner.client._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 @@ -266,6 +283,25 @@ async def test_one_connection_carries_every_command(runner: _Running) -> None: assert runner.client._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._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._connection is connection + + def test_strays_are_everything_but_the_runner_and_its_ancestors() -> None: child = subprocess.Popen(['sleep', '30']) try: @@ -328,7 +364,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..f0674be0eb 100644 --- a/dev_tools/services/acknowledg-forge/tests/test_safety.py +++ b/dev_tools/services/acknowledg-forge/tests/test_safety.py @@ -5,6 +5,7 @@ '''Handling of untrusted input: archives, paths, and the sandbox.''' +import asyncio import io import os import stat @@ -15,14 +16,19 @@ import pytest +from edgacknowledg.forge import processor, workspace +from edgacknowledg.forge.processor import _changes_test_ids, _read_ci_metadata from edgacknowledg.forge.sandbox import ( RunnerClient, 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 +93,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'{}', @@ -150,6 +216,66 @@ def test_changes_patterns_cover_recordings_and_expectations() -> None: assert not any(p.startswith('/dev_tools/') for p in patterns) +def test_long_test_names_need_no_patterns() -> None: + # No file has such a name; its patterns would be quadratic in its length. + name = '.'.join(['x'] * 200) + '.sft.cpp' + assert patterns_for_test(PurePosixPath('tests/tests/s', name)) == [] + + +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. + assert await run('yes', limit = 1024 * 1024) is None + + +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 @@ -168,6 +294,7 @@ async def test_sandbox_requires_its_runners(tmp_path: Path, 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() + async with RunnerClient(unreachable_url, open_timeout = 0.5) as client: + unreachable = Sandbox(builder = client) + 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..1367da801e 100644 --- a/dev_tools/services/acknowledg-forge/tests/test_state.py +++ b/dev_tools/services/acknowledg-forge/tests/test_state.py @@ -8,19 +8,25 @@ 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, 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 +78,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. @@ -102,6 +132,11 @@ async def test_publisher_outage_defers_without_counting( await harness.step() record = await store.find_pull_request('fake', REPOSITORY, 1) assert record is not None and record.is_dirty and record.error_count == 0 + # Stored all the same: the database leads, and the publisher client + # serves what it stores once it can. + assert [s.name for s in await store.sections(record.id)] == [ + 'Test - Changes' + ] harness.publisher.available = True await harness.step() @@ -110,6 +145,42 @@ async def test_publisher_outage_defers_without_counting( ] +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, forge: FakeForge, @@ -240,7 +311,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 +331,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']))