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']))