From ddb726a10d2170734054e1d002d1f70ffea72093 Mon Sep 17 00:00:00 2001 From: Wyatt Childers Date: Tue, 3 Nov 2026 16:15:31 +0000 Subject: [PATCH 1/2] Stream pull request diffs to disk, and prune CI's uploaded runs [] The forge service held a pull request's tests/ diff in memory several times over: as git's output, decoded, stripped, and split into lines. The diff is now processed in a streamed fashion. Since the diff now costs disk rather than memory, its default limit rises to 512 MiB. A failing CI leg's upload held every test's .rt.tar.gz (edgy creates a .rt.tar.gz for every test, this can be useful for analysis but is not useful for AcknowlEDG Forge and results in larger amounts of dating needing to be stored and processed. Now, before uploading, the leg now runs the new edg-prune-test-run (edgtest.prune_run_archives), which removes .rt.tar.gz files not associated with a changes.elist file. --- .github/workflows/build-and-test-ci.yml | 2 +- .github/workflows/ci-toolchain.yml | 13 +- .github/workflows/promote-test-cache.yml | 2 +- .github/workflows/strict-build-check.yml | 8 +- dev_tools/bin/edg-prune-test-run | 58 +++++++++ .../pylibs/edgacknowledg/forge/config.py | 5 +- .../pylibs/edgacknowledg/forge/processor.py | 65 ++++++---- .../pylibs/edgacknowledg/forge/workspace.py | 109 ++++++++++------ dev_tools/pylibs/edgtest/__init__.py | 80 ++++++++++++ dev_tools/services/acknowledg-forge/README.md | 2 +- .../services/acknowledg-forge/SANDBOXING.md | 2 +- .../acknowledg-forge/tests/test_safety.py | 91 ++++++++++--- .../acknowledg-forge/tests/test_state.py | 2 +- dev_tools/tests/test_edgtest_prune.py | 122 ++++++++++++++++++ 14 files changed, 467 insertions(+), 94 deletions(-) create mode 100755 dev_tools/bin/edg-prune-test-run create mode 100644 dev_tools/tests/test_edgtest_prune.py diff --git a/.github/workflows/build-and-test-ci.yml b/.github/workflows/build-and-test-ci.yml index 09dd662f66..7a354ff978 100644 --- a/.github/workflows/build-and-test-ci.yml +++ b/.github/workflows/build-and-test-ci.yml @@ -308,7 +308,7 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 120 container: - image: ghcr.io/edgcpp/ci-runner@sha256:da140ca90c420977f187279d7939fe0c7cb3c4b656e5ab2dc0f6b9ac2a43a255 + image: ghcr.io/edgcpp/ci-runner@sha256:c5c2c5c969f095b6d7d91b8d97f08b359bf1b5b0ed0814996f799e035a659f47 defaults: run: shell: edg-container-exec "bash -eo pipefail {0}" diff --git a/.github/workflows/ci-toolchain.yml b/.github/workflows/ci-toolchain.yml index 1aec80fa48..6099ff85b3 100644 --- a/.github/workflows/ci-toolchain.yml +++ b/.github/workflows/ci-toolchain.yml @@ -66,7 +66,7 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 120 container: - image: &ci-runner-image ghcr.io/edgcpp/ci-runner@sha256:da140ca90c420977f187279d7939fe0c7cb3c4b656e5ab2dc0f6b9ac2a43a255 + image: &ci-runner-image ghcr.io/edgcpp/ci-runner@sha256:c5c2c5c969f095b6d7d91b8d97f08b359bf1b5b0ed0814996f799e035a659f47 defaults: &container-defaults run: shell: edg-container-exec "bash -eo pipefail {0}" @@ -267,6 +267,17 @@ jobs: edg-prefix-env-loader .tmp/extensive-ci.edgivk + # Reviews (acknowledg-forge, edgy-review) read only the .rt.tar.gz + # files changes.elist names; edgy -A archives every test's. Pruning + # must never keep the results from being uploaded, hence + # continue-on-error. + - name: Prune .rt.tar.gz results changes.elist doesn't name + if: ${{ failure() }} + continue-on-error: true + env: + RUN_TIMESTAMP: ${{ inputs.run_timestamp }} + run: edg-prune-test-run "tests/runs/${RUN_TIMESTAMP}" + - name: Upload partial elist, elog, and .rt.tar.gz results if: ${{ failure() }} uses: actions/upload-artifact@v7 diff --git a/.github/workflows/promote-test-cache.yml b/.github/workflows/promote-test-cache.yml index 1d24f6ab89..821a779a01 100644 --- a/.github/workflows/promote-test-cache.yml +++ b/.github/workflows/promote-test-cache.yml @@ -72,7 +72,7 @@ jobs: # The test legs' image, so that actions/cache picks the same compression # (part of an entry's version) and the test legs can restore the entry. container: - image: ghcr.io/edgcpp/ci-runner@sha256:da140ca90c420977f187279d7939fe0c7cb3c4b656e5ab2dc0f6b9ac2a43a255 + image: ghcr.io/edgcpp/ci-runner@sha256:c5c2c5c969f095b6d7d91b8d97f08b359bf1b5b0ed0814996f799e035a659f47 strategy: fail-fast: false matrix: diff --git a/.github/workflows/strict-build-check.yml b/.github/workflows/strict-build-check.yml index 258e9aa8b6..ff0e11d614 100644 --- a/.github/workflows/strict-build-check.yml +++ b/.github/workflows/strict-build-check.yml @@ -158,7 +158,7 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 120 container: - image: ghcr.io/edgcpp/ci-runner@sha256:da140ca90c420977f187279d7939fe0c7cb3c4b656e5ab2dc0f6b9ac2a43a255 + image: ghcr.io/edgcpp/ci-runner@sha256:c5c2c5c969f095b6d7d91b8d97f08b359bf1b5b0ed0814996f799e035a659f47 defaults: run: shell: edg-container-exec "bash -eo pipefail {0}" @@ -185,7 +185,7 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 120 container: - image: ghcr.io/edgcpp/ci-runner@sha256:da140ca90c420977f187279d7939fe0c7cb3c4b656e5ab2dc0f6b9ac2a43a255 + image: ghcr.io/edgcpp/ci-runner@sha256:c5c2c5c969f095b6d7d91b8d97f08b359bf1b5b0ed0814996f799e035a659f47 defaults: run: shell: edg-container-exec "bash -eo pipefail {0}" @@ -212,7 +212,7 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 120 container: - image: ghcr.io/edgcpp/ci-runner@sha256:da140ca90c420977f187279d7939fe0c7cb3c4b656e5ab2dc0f6b9ac2a43a255 + image: ghcr.io/edgcpp/ci-runner@sha256:c5c2c5c969f095b6d7d91b8d97f08b359bf1b5b0ed0814996f799e035a659f47 defaults: run: shell: edg-container-exec "bash -eo pipefail {0}" @@ -239,7 +239,7 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 120 container: - image: ghcr.io/edgcpp/ci-runner@sha256:da140ca90c420977f187279d7939fe0c7cb3c4b656e5ab2dc0f6b9ac2a43a255 + image: ghcr.io/edgcpp/ci-runner@sha256:c5c2c5c969f095b6d7d91b8d97f08b359bf1b5b0ed0814996f799e035a659f47 defaults: run: shell: edg-container-exec "bash -eo pipefail {0}" diff --git a/dev_tools/bin/edg-prune-test-run b/dev_tools/bin/edg-prune-test-run new file mode 100755 index 0000000000..451f49fc5a --- /dev/null +++ b/dev_tools/bin/edg-prune-test-run @@ -0,0 +1,58 @@ +#!/usr/bin/env python3 + +# 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 + +'''Prune a test run down to what a review of it reads, e.g., before CI +uploads the run: + + edg-prune-test-run tests/runs/ + +Reviews of a run read only the .rt.tar files its changes.elist files name. +The replays directory is removed whole. In each configuration, a suite +with no test named there is removed whole (its directory, and its +suite- logs), as is every suite of a configuration without a +changes.elist; of the other suites, the .rt.tar files not named there are +removed. +''' + +import argparse +import sys + +import edgtest +import edgutil + +from pathlib import Path + +def _parse_args() -> argparse.Namespace: + parser = argparse.ArgumentParser( + description = ( + "Remove the .rt.tar files of a test run that no configuration's " + 'changes.elist names.' + ) + ) + parser.add_argument( + 'run_dir', + type = Path, + help = 'the run directory (e.g., tests/runs/)' + ) + return parser.parse_args() + +def main() -> int: + args = _parse_args() + if not args.run_dir.is_dir(): + edgutil.eprint(f"error: {args.run_dir} is not a directory") + return 1 + + # Counts: of .rt.tar files kept and removed, and of suites removed. + kept, removed, suites = edgtest.prune_run_archives(args.run_dir) + print( + f"Kept {kept} .rt.tar files named by changes.elist files; removed " + f"{removed} others, and {suites} suites with no changes." + ) + return 0 + +if __name__ == '__main__': + sys.exit(main()) diff --git a/dev_tools/pylibs/edgacknowledg/forge/config.py b/dev_tools/pylibs/edgacknowledg/forge/config.py index 07485371f1..5f10ab8d45 100644 --- a/dev_tools/pylibs/edgacknowledg/forge/config.py +++ b/dev_tools/pylibs/edgacknowledg/forge/config.py @@ -32,8 +32,9 @@ # suite's name, is short. DEFAULT_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. -DEFAULT_MAX_DIFF_BYTES = 64 * 1024 * 1024 +# kept with the review's rematerialized run (on disk, never in memory +# whole), and the builder applies all of it. +DEFAULT_MAX_DIFF_BYTES = 512 * 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. CI's test diff --git a/dev_tools/pylibs/edgacknowledg/forge/processor.py b/dev_tools/pylibs/edgacknowledg/forge/processor.py index ac17bc9405..7f990d6a1f 100644 --- a/dev_tools/pylibs/edgacknowledg/forge/processor.py +++ b/dev_tools/pylibs/edgacknowledg/forge/processor.py @@ -397,42 +397,54 @@ async def _changes_section(self, if reused is not None: return reused - diff = await self._workspaces.diff( - info.repository, - merge_base, - head_sha, - ['tests/'] - ) - if diff.strip() == '': - return None - - worktree = await self._workspaces.checkout( - info, - record.review_id, - 'base', - merge_base, - changes_patterns( - diff, - max_patterns = self._config.max_sparse_patterns - ) - ) - runs_dir = ensure_real_directory(worktree, PurePosixPath('tests', 'runs')) - run_dir = runs_dir / f"remat-{_sha256(source_key.encode())[:16]}" - remove_tree(run_dir) - # The builder sees the worktree read-only, so it writes elsewhere (on # the same file system, for the rename into place). + run_name = f"remat-{_sha256(source_key.encode())[:16]}" staging_root = ensure_real_directory( self._workspaces.build_staging_dir.parent, PurePosixPath(self._workspaces.build_staging_dir.name) ) staging = Path(tempfile.mkdtemp( - prefix = f"{record.review_id}-{run_dir.name}-", + prefix = f"{record.review_id}-{run_name}-", dir = staging_root )) try: + # Only ever in this file, never held in memory whole: it may be large. diff_file = staging / 'pull-request.diff' - diff_file.write_text(diff, encoding = 'utf-8') + await self._workspaces.diff( + info.repository, + merge_base, + head_sha, + ['tests/'], + diff_file + ) + if diff_file.stat().st_size == 0: + remove_tree(staging) + return None + with open(diff_file, encoding = 'utf-8') as handle: + patterns = changes_patterns( + handle, + max_patterns = self._config.max_sparse_patterns + ) + + worktree = await self._workspaces.checkout( + info, + record.review_id, + 'base', + merge_base, + patterns + ) + # Prepare where the run goes once built: tests/runs/ in the + # worktree (through real directories only, not links the pull request + # committed), cleared of any earlier attempt so that the staging + # directory can be renamed into place. + runs_dir = ensure_real_directory( + worktree, + PurePosixPath('tests', 'runs') + ) + run_dir = runs_dir / run_name + remove_tree(run_dir) + timestamp = self._clock().strftime(edgutil.RUN_TIMESTAMP_FORMAT) result = await self._sandbox.run( [ @@ -477,6 +489,7 @@ async def _changes_section(self, ) remove_tree(staging) return None + fingerprint = await asyncio.to_thread(_file_sha256, diff_file) staging.rename(run_dir) except BaseException: remove_tree(staging) @@ -487,7 +500,7 @@ async def _changes_section(self, name = CHANGES_NAME, slug = section_slug(CHANGES_NAME), source_key = source_key, - content_fingerprint = _sha256(diff.encode()), + content_fingerprint = fingerprint, root = run_dir / timestamp, home = worktree / 'tests', details = {'run_dir': str(run_dir)}, diff --git a/dev_tools/pylibs/edgacknowledg/forge/workspace.py b/dev_tools/pylibs/edgacknowledg/forge/workspace.py index 6079f52b22..f7f56d63b4 100644 --- a/dev_tools/pylibs/edgacknowledg/forge/workspace.py +++ b/dev_tools/pylibs/edgacknowledg/forge/workspace.py @@ -26,6 +26,7 @@ import asyncio import base64 +import codecs import logging import os import re @@ -39,7 +40,7 @@ from dataclasses import dataclass from pathlib import Path, PurePosixPath -from typing import Iterable, Optional, Sequence +from typing import Callable, Iterable, Optional, Sequence import edgtest import edgutil @@ -81,6 +82,10 @@ class UnsafeArchiveError(WorkspaceError): '''Raised for an artifact that is too large or has unsafe entries.''' +class _OutputTooLarge(Exception): + '''Raised when a process's output is over its limit (it is stopped).''' + + @dataclass(frozen = True) class GitResult: returncode: int @@ -140,16 +145,18 @@ def changed_paths(diff: str) -> list[str]: return paths -def changes_patterns(diff: str, +def changes_patterns(lines_iter: Iterable[str], *, max_patterns: int = DEFAULT_MAX_PATTERNS) -> list[str]: - '''Sparse-checkout patterns for rematerializing ``diff`` (of tests/) on - the base tree: the test home and every affected test; at most + '''Sparse-checkout patterns for rematerializing the diff (of tests/) of + ``lines_iter`` (with or without their line ends, e.g. a file's) on the + base tree: the test home and every affected test; at most ``max_patterns``. ''' patterns = dict.fromkeys(_TEST_HOME_PATTERNS) current_suite: Optional[str] = None - for line in diff.splitlines(): + for line in lines_iter: + line = line.rstrip('\n') match = _DIFF_HEADER.match(line) if match is not None: path = PurePosixPath(match.group(1)) @@ -478,9 +485,13 @@ async def git(self, cwd: Optional[Path] = None, stdin: Optional[str] = None, 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). + max_output: Optional[int] = None, + stdout_sink: Optional[Callable[[bytes], None]] = None + ) -> GitResult: + '''Run git; with ``stdout_sink``, its output goes there as it comes, + rather than into the result, and may be at most ``max_output`` bytes + (it is stopped, and ``_OutputTooLarge`` raised, beyond: the caller + knows which limit that is). ''' command = ['git'] if git_dir is not None: @@ -497,18 +508,15 @@ async def git(self, cwd = cwd, env = self._git_env() ) - if max_output is None: + if stdout_sink is None: + assert 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 stdin is None and max_output is not None + stderr = await _communicate_bounded(process, max_output, stdout_sink) + stdout = b'' assert process.returncode is not None result = GitResult( process.returncode, @@ -609,15 +617,42 @@ async def diff(self, repository: str, base: str, head: str, - paths: Sequence[str]) -> str: + paths: Sequence[str], + destination: Path) -> None: + '''Write the diff of ``paths`` from ``base`` to ``head`` to (the new + file) ``destination``, as it comes (it may be large): UTF-8, with what + isn't replaced (U+FFFD). + ''' + decoder = codecs.getincrementaldecoder('utf-8')(errors = 'replace') async with self.lock(repository): - result = await self.git( - 'diff', '--no-renames', '--no-color', '--no-ext-diff', '--no-textconv', - base, head, '--', *paths, - git_dir = self.clone_dir(repository), - max_output = self._max_diff_bytes - ) - return result.stdout + with open(destination, 'xb') as handle: + + # Decoded and encoded again so that what isn't UTF-8 is replaced: + # the builder's edg-rematerialize-test-run (and changes_patterns) + # read it as UTF-8 text, and the Changes section's fingerprint + # (the file's) stays what it was when the diff was decoded whole. + # Incrementally, so a character split between chunks survives. + + def write(chunk: bytes) -> None: + handle.write(decoder.decode(chunk).encode()) + + try: + await self.git( + 'diff', '--no-renames', '--no-color', '--no-ext-diff', + '--no-textconv', base, head, '--', *paths, + git_dir = self.clone_dir(repository), + max_output = self._max_diff_bytes, + stdout_sink = write + ) + except _OutputTooLarge: + raise WorkspaceError( + f"the diff is over {self._max_diff_bytes} bytes " + f"({ENV_PREFIX}MAX_DIFF_BYTES)" + ) from None + + # Finish decoding: bytes the decoder still holds (a character the + # output ended in the middle of) are written too, replaced. + handle.write(decoder.decode(b'', final = True).encode()) async def checkout(self, info: PullRequestInfo, @@ -702,31 +737,31 @@ async def collect_garbage(self, repository: str, open_count: int) -> None: async def _communicate_bounded(process: asyncio.subprocess.Process, - limit: int) -> Optional[tuple[bytes, bytes]]: - '''``process``'s output (stdout, stderr), once it exits; ``None``, having - stopped it, should its stdout be over ``limit`` bytes. Should this be - cancelled (or fail), it is stopped too, rather than left writing to a - pipe no one reads. + limit: int, + sink: Callable[[bytes], None]) -> bytes: + '''Pass ``process``'s stdout to ``sink`` as it comes, in chunks (none held + here); return its stderr once it exits. Should its stdout be over + ``limit`` bytes, it is stopped, and ``_OutputTooLarge`` raised. Should + this be cancelled (or fail), it is stopped too, rather than left writing + to a pipe no one reads. ''' assert process.stdout is not None and process.stderr is not None - errors = asyncio.create_task(process.stderr.read()) - chunks: list[bytes] = [] + reading_stderr = asyncio.create_task(process.stderr.read()) size = 0 try: while chunk := await process.stdout.read(64 * 1024): size += len(chunk) if size > limit: - await edgutil.async_terminate_within(process, _TERMINATE_ALLOWANCE) - return None - chunks.append(chunk) - output = b''.join(chunks), await errors + raise _OutputTooLarge() + sink(chunk) + stderr = await reading_stderr await process.wait() - return output + return stderr except BaseException: await edgutil.async_terminate_within(process, _TERMINATE_ALLOWANCE) raise finally: - errors.cancel() + reading_stderr.cancel() def basic_auth_header(user: str, password: str) -> str: diff --git a/dev_tools/pylibs/edgtest/__init__.py b/dev_tools/pylibs/edgtest/__init__.py index 0b8ef00085..07ce2a675c 100644 --- a/dev_tools/pylibs/edgtest/__init__.py +++ b/dev_tools/pylibs/edgtest/__init__.py @@ -306,6 +306,86 @@ def find_changes_files(run_dir: Path) -> Dict[str, Path]: changes_files[dir_entry] = changes_file return changes_files +def _remove_run_tree(path: Path) -> int: + '''Remove the directory path (of a test run) whole; return how many + .rt.tar files it held. + ''' + count = 0 + for _, _, file_names in os.walk(path): + count += sum( + 1 for file_name in file_names + if file_name.endswith(runtest.RUN_TEST_OUTPUT_SUFFIX) + ) + shutil.rmtree(path) + return count + +def prune_run_archives(run_dir: Path) -> Tuple[int, int, int]: + '''Prune the run in run_dir down to what a review of it reads (the .rt.tar + files changes.elist names; see _scan_changes_file), e.g., to keep CI's + uploads small. The replays directory is removed whole. In each + configuration, a suite with no test changes.elist names (every suite, + without a changes.elist) is removed whole: its directory, and its + suite- logs. Of the other suites, the .rt.tar files changes.elist + doesn't name are removed. Return how many .rt.tar files were kept, how + many removed, and how many suites were removed. + ''' + kept = 0 + removed = 0 + suites = 0 + for dir_entry in os.listdir(run_dir): + config_dir = run_dir / dir_entry + if not config_dir.is_dir() or config_dir.is_symlink(): + continue + if dir_entry == 'replays': + removed += _remove_run_tree(config_dir) + continue + if dir_entry in ('recording-updates', 'patched-tests'): + continue + + # The .rt.tar files changes.elist names, by suite. + named: Dict[str, Set[Path]] = {} + changes_file = config_dir / 'changes.elist' + if changes_file.is_file(): + with open(changes_file, 'r') as file_handle: + test_ids = [ + test_id for test_id in (line.strip() for line in file_handle) + if test_id != '' + ] + for test_id in test_ids: + test_path = PurePosixPath(test_id) + named.setdefault(test_path.parts[0], set()).add( + config_dir / test_path.with_suffix(runtest.RUN_TEST_OUTPUT_SUFFIX) + ) + + # Each suite edgy ran (and logged) in the configuration. + for log_path in config_dir.glob('suite-*.elog'): + suite = log_path.stem[len('suite-'):] + suite_dir = config_dir / suite + walkable = suite_dir.is_dir() and not suite_dir.is_symlink() + if suite not in named: + if walkable: + removed += _remove_run_tree(suite_dir) + log_path.unlink() + try: + log_path.with_suffix('.log').unlink() + except FileNotFoundError: + pass + suites += 1 + continue + if not walkable: + continue + for root, _, file_names in os.walk(suite_dir): + for file_name in file_names: + if not file_name.endswith(runtest.RUN_TEST_OUTPUT_SUFFIX): + continue + file_path = Path(root, file_name) + if file_path in named[suite]: + kept += 1 + else: + file_path.unlink() + removed += 1 + return kept, removed, suites + def describe_rt_path(rt_file_path: Path, run_dir: Path) -> Tuple[str, str]: '''Return (relative_rt_path, config_name) for an .rt.tar under the run. diff --git a/dev_tools/services/acknowledg-forge/README.md b/dev_tools/services/acknowledg-forge/README.md index 1568da7961..87edb3054a 100644 --- a/dev_tools/services/acknowledg-forge/README.md +++ b/dev_tools/services/acknowledg-forge/README.md @@ -114,7 +114,7 @@ from the environment variable shown as a fallback. | `ACKNOWLEDG_FORGE_MAX_EXTRACTED_BYTES` | 2 GiB | Largest extracted artifact (including inner tarballs) | | `ACKNOWLEDG_FORGE_MAX_ARTIFACT_ENTRIES` | `500000` | Most entries in an artifact (including inner tarballs'). CI's test results reach about 170,000 | | `ACKNOWLEDG_FORGE_MAX_ARTIFACT_DIRECTORY_BYTES` | 64 MiB | Largest artifact zip central directory, which is read into memory whole (about 106 bytes per entry in CI's test results) | -| `ACKNOWLEDG_FORGE_MAX_DIFF_BYTES` | 64 MiB | Largest diff of a pull request's `tests/`, which is held in memory and copied more than once: raising it may need a higher `mem_limit` for the service ([`compose.yaml`](compose.yaml)) | +| `ACKNOWLEDG_FORGE_MAX_DIFF_BYTES` | 512 MiB | Largest diff of a pull request's `tests/`. It is streamed to disk, not held in memory, and kept with the review's rematerialized run; the builder applies all of it within `ACKNOWLEDG_FORGE_REMATERIALIZE_TIMEOUT` | | `ACKNOWLEDG_FORGE_MAX_SPARSE_PATTERNS` | `20000` | Most sparse-checkout patterns in a review's worktree (up to two per test). Checking out takes time proportional to patterns times paths | | `ACKNOWLEDG_FORGE_MAX_CONCURRENCY` | `2` | Pull requests processed at once | | `ACKNOWLEDG_FORGE_REMATERIALIZE_TIMEOUT` | `900` | Seconds `edg-rematerialize-test-run` may run | diff --git a/dev_tools/services/acknowledg-forge/SANDBOXING.md b/dev_tools/services/acknowledg-forge/SANDBOXING.md index c89d1401a9..2cc095e27d 100644 --- a/dev_tools/services/acknowledg-forge/SANDBOXING.md +++ b/dev_tools/services/acknowledg-forge/SANDBOXING.md @@ -527,7 +527,7 @@ them. Several layers keep the service from reading through one: | --- | --- | | Artifact download | `ACKNOWLEDG_FORGE_MAX_ARTIFACT_BYTES` (256 MiB), also counting the decompressed size of inner tarballs | | Artifact entries | `ACKNOWLEDG_FORGE_MAX_ARTIFACT_ENTRIES` (500,000), counting inner tarballs' too; and its zip's central directory, `ACKNOWLEDG_FORGE_MAX_ARTIFACT_DIRECTORY_BYTES` (64 MiB), checked before zipfile reads it | -| A pull request's diff of `tests/` | `ACKNOWLEDG_FORGE_MAX_DIFF_BYTES` (64 MiB) | +| A pull request's diff of `tests/` | `ACKNOWLEDG_FORGE_MAX_DIFF_BYTES` (512 MiB) | | A review worktree's sparse-checkout patterns | `ACKNOWLEDG_FORGE_MAX_SPARSE_PATTERNS` (20,000) | | A file read for a reviewer | 64 MiB; a larger one fails the request | | A source shown to a reviewer | only as much as can be displayed is read | diff --git a/dev_tools/services/acknowledg-forge/tests/test_safety.py b/dev_tools/services/acknowledg-forge/tests/test_safety.py index 822538d76a..5d27f9f93c 100644 --- a/dev_tools/services/acknowledg-forge/tests/test_safety.py +++ b/dev_tools/services/acknowledg-forge/tests/test_safety.py @@ -23,6 +23,8 @@ from edgacknowledg.forge.workspace import ( UnsafeArchiveError, WorkspaceError, + Workspaces, + _OutputTooLarge, _central_directory_size, _communicate_bounded, changes_patterns, @@ -32,6 +34,9 @@ patterns_for_test, ) +from conftest import Origin, pull_request_info +from fakes import REPOSITORY, FakeForge + def _zip(path: Path, entries: dict[str, bytes]) -> Path: with zipfile.ZipFile(path, 'w') as archive: for name, data in entries.items(): @@ -226,7 +231,7 @@ def test_changes_patterns_cover_recordings_and_expectations() -> None: '-dir/b.sft.cpp: PASS', '+dir/b.sft.cpp: FAIL', ]) - patterns = changes_patterns(diff) + patterns = changes_patterns(diff.splitlines()) assert '/tests/tests/s/.a.rto/' in patterns assert '/tests/tests/s/a.*' in patterns assert '/tests/tests/other/dir/b.sft.cpp' in patterns @@ -254,10 +259,10 @@ def expectations(suite: str) -> str: '+a.sft.cpp: PASS', ]) - patterns = changes_patterns(expectations('cwg')) + patterns = changes_patterns(expectations('cwg').splitlines()) assert '/tests/tests/cwg/a.sft.cpp' in patterns for suite in ['s' * 65, 'a.b', 'a b']: - assert changes_patterns(expectations(suite)) == [ + assert changes_patterns(expectations(suite).splitlines()) == [ '/tests/.edgy/', '/tests/expectations/', ] @@ -276,34 +281,80 @@ def test_patterns_are_capped() -> None: *(f"+{n}.sft.cpp: FAIL" for n in range(cap)), ]) with pytest.raises(WorkspaceError, match = 'sparse-checkout patterns'): - changes_patterns(diff, max_patterns = cap) + changes_patterns(diff.splitlines(), max_patterns = cap) # Within a higher cap. - changes_patterns(diff, max_patterns = 3 * cap) + changes_patterns(diff.splitlines(), max_patterns = 3 * cap) 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') + # Stdout goes to the sink as it comes; stderr is returned. + sunk: list[bytes] = [] + process = await asyncio.create_subprocess_exec( + 'sh', '-c', 'printf out; printf err >&2', + stdout = asyncio.subprocess.PIPE, + stderr = asyncio.subprocess.PIPE + ) + assert await _communicate_bounded(process, 3, sunk.append) == b'err' + assert process.returncode is not None + assert b''.join(sunk) == b'out' # One that would never stop is stopped (letting it clean up). process = await asyncio.create_subprocess_exec( 'yes', stdout = asyncio.subprocess.PIPE, stderr = asyncio.subprocess.PIPE ) - assert await _communicate_bounded(process, 1024 * 1024) is None + with pytest.raises(_OutputTooLarge): + await _communicate_bounded(process, 1024 * 1024, sunk.append) assert process.returncode == -signal.SIGTERM +async def test_diffs_are_written_as_they_come(tmp_path: Path, + origin: Origin, + forge: FakeForge) -> None: + # Longer than a chunk git's output comes in, with characters of several + # bytes across them. + lines = [f"{n}.sft.cpp: {'é€😀' * 10}" for n in range(2000)] + head = origin.push_pull_request(1, { + 'tests/expectations/x/s.log': ''.join(f"{line}\n" for line in lines), + }) + workspaces = Workspaces(tmp_path / 'home', forge) + info = pull_request_info(1, head, origin.main_sha) + head, _ = await workspaces.fetch_pull_request(info) + diff_file = tmp_path / 'pull-request.diff' + await workspaces.diff( + REPOSITORY, + origin.main_sha, + head, + ['tests/'], + diff_file + ) + diff = diff_file.read_text(encoding = 'utf-8') + assert len(diff_file.read_bytes()) > 64 * 1024 + assert '\ufffd' not in diff + added = [ + line[1:] for line in diff.splitlines() + if line.startswith('+') and not line.startswith('+++') + ] + assert added == lines + # Its patterns, read from the file as from the text. + with open(diff_file, encoding = 'utf-8') as handle: + assert changes_patterns(handle) == changes_patterns(diff.splitlines()) + assert '/tests/tests/s/1999.sft.cpp' in changes_patterns(diff.splitlines()) + + # Too long: refused. + workspaces = Workspaces(tmp_path / 'home', forge, max_diff_bytes = 1024) + with pytest.raises( + WorkspaceError, + match = r'over 1024 bytes \(ACKNOWLEDG_FORGE_MAX_DIFF_BYTES\)'): + await workspaces.diff( + REPOSITORY, + origin.main_sha, + head, + ['tests/'], + tmp_path / 'too-long.diff' + ) + + async def test_cancelled_git_output_reads_stop_git() -> None: # As when the service stops while git still has output to write: left # writing to a pipe no one reads, it would never exit. @@ -312,7 +363,9 @@ async def test_cancelled_git_output_reads_stop_git() -> None: stdout = asyncio.subprocess.PIPE, stderr = asyncio.subprocess.PIPE ) - reading = asyncio.create_task(_communicate_bounded(process, 1 << 40)) + reading = asyncio.create_task( + _communicate_bounded(process, 1 << 40, lambda chunk: None) + ) await asyncio.sleep(0.2) reading.cancel() await asyncio.wait_for(asyncio.gather(reading, return_exceptions = True), 10) diff --git a/dev_tools/services/acknowledg-forge/tests/test_state.py b/dev_tools/services/acknowledg-forge/tests/test_state.py index 2b45050ae7..b1149425f1 100644 --- a/dev_tools/services/acknowledg-forge/tests/test_state.py +++ b/dev_tools/services/acknowledg-forge/tests/test_state.py @@ -427,7 +427,7 @@ def test_webhooks_need_the_service_bind(tmp_path: Path) -> None: @pytest.mark.parametrize('setting, problem', [ - ('MAX_DIFF_BYTES', "git diff's output is over 1 bytes"), + ('MAX_DIFF_BYTES', 'MAX_DIFF_BYTES'), ('MAX_SPARSE_PATTERNS', 'MAX_SPARSE_PATTERNS'), ('MAX_ARTIFACT_ENTRIES', 'MAX_ARTIFACT_ENTRIES'), ('MAX_ARTIFACT_DIRECTORY_BYTES', 'MAX_ARTIFACT_DIRECTORY_BYTES'), diff --git a/dev_tools/tests/test_edgtest_prune.py b/dev_tools/tests/test_edgtest_prune.py new file mode 100644 index 0000000000..06ec2a4500 --- /dev/null +++ b/dev_tools/tests/test_edgtest_prune.py @@ -0,0 +1,122 @@ +# Part of the EDG Compiler Project, under the Apache License v2.0 with LLVM +# Exceptions. +# See https://edgcpp.org/LICENSE.txt for license information. +# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +'''Tests for pruning a test run's .rt.tar files (edgtest.prune_run_archives) +and edg-prune-test-run. + +Run with: python3 -m unittest discover -s dev_tools/tests +(with dev_tools/pylibs on PYTHONPATH). +''' + +import os +import subprocess +import sys +import tempfile +import unittest + +from pathlib import Path +from typing import List + +import edgtest + +from edgtest import runtest + +DEV_TOOLS_DIR = Path(__file__).resolve().parents[1] +PRUNE_TOOL_PATH = DEV_TOOLS_DIR / 'bin' / 'edg-prune-test-run' + +SUFFIX = runtest.RUN_TEST_OUTPUT_SUFFIX + +def _touch(path: Path) -> Path: + path.parent.mkdir(parents = True, exist_ok = True) + path.write_bytes(b'') + return path + +def _files(root: Path) -> List[str]: + return sorted( + Path(directory, name).relative_to(root).as_posix() + for directory, _, names in os.walk(root) + for name in names + ) + +class PruneRunArchivesTests(unittest.TestCase): + + def setUp(self) -> None: + temp_dir = tempfile.TemporaryDirectory() + self.addCleanup(temp_dir.cleanup) + self.run_dir = Path(temp_dir.name) / 'run' + # One configuration whose changes.elist names two tests of one suite + # (and one without results), with every test's results... + config = self.run_dir / 'edg_x86_64' + _touch(config / 'changes.elist').write_text( + 'suite/a.sft.cpp\nsuite/dir/b.sft.c\nsuite/none.sft.cpp\n\n' + ) + for name in ('changes.elog', 'executed.elist', 'suite-suite.elog', + 'suite-suite.log', 'suite-other.elog', 'suite-other.log'): + _touch(config / name) + for name in ('suite/a.sft', 'suite/dir/b.sft', 'suite/c.sft', + 'other/a.sft'): + _touch(config / f"{name}{SUFFIX}") + # ...one without a changes.elist... + config = self.run_dir / 'edg_x86_32' + for name in ('executed.elist', 'suite-suite.elog', 'suite-suite.log', + f"suite/a.sft{SUFFIX}"): + _touch(config / name) + # ...and the run's replays (removed whole). + _touch(self.run_dir / 'replays' / f"suite/c.sft{SUFFIX}") + + def test_keeps_only_what_changes_elist_names(self) -> None: + changes_file = self.run_dir / 'edg_x86_64' / 'changes.elist' + read = list(edgtest._scan_changes_file(changes_file)) + + self.assertEqual(edgtest.prune_run_archives(self.run_dir), (2, 4, 2)) + # Suites with no changes go whole, and unnamed results with them. + self.assertEqual(_files(self.run_dir), [ + 'edg_x86_32/executed.elist', + 'edg_x86_64/changes.elist', + 'edg_x86_64/changes.elog', + 'edg_x86_64/executed.elist', + 'edg_x86_64/suite-suite.elog', + 'edg_x86_64/suite-suite.log', + 'edg_x86_64/suite/a.sft' + SUFFIX, + 'edg_x86_64/suite/dir/b.sft' + SUFFIX, + ]) + # A review reads what it read before. + self.assertEqual(list(edgtest._scan_changes_file(changes_file)), read) + + # Pruning again changes nothing. + self.assertEqual(edgtest.prune_run_archives(self.run_dir), (2, 0, 0)) + + def test_keeps_nothing_without_changes_elist(self) -> None: + (self.run_dir / 'edg_x86_64' / 'changes.elist').unlink() + self.assertEqual(edgtest.prune_run_archives(self.run_dir), (0, 6, 3)) + self.assertEqual(_files(self.run_dir), [ + 'edg_x86_32/executed.elist', + 'edg_x86_64/changes.elog', + 'edg_x86_64/executed.elist', + ]) + + def test_command(self) -> None: + env = dict(os.environ) + env['PYTHONPATH'] = str(DEV_TOOLS_DIR / 'pylibs') + result = subprocess.run( + [sys.executable, str(PRUNE_TOOL_PATH), str(self.run_dir)], + capture_output = True, + text = True, + env = env + ) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertIn('Kept 2', result.stdout) + self.assertIn('removed 4 others, and 2 suites', result.stdout) + + missing = subprocess.run( + [sys.executable, str(PRUNE_TOOL_PATH), str(self.run_dir / 'missing')], + capture_output = True, + text = True, + env = env + ) + self.assertEqual(missing.returncode, 1) + +if __name__ == '__main__': + unittest.main() From 8d6e0c5a42691104721a1a782c7f8edb48a903e6 Mon Sep 17 00:00:00 2001 From: Wyatt Childers Date: Fri, 6 Nov 2026 22:56:44 +0000 Subject: [PATCH 2/2] Add safety limits on line length [] --- dev_tools/bin/edg-rematerialize-test-run | 55 ++++-- dev_tools/pylibs/edgacknowledg/__init__.py | 7 +- .../pylibs/edgacknowledg/client/shared.py | 137 +++++++++++--- .../edgacknowledg/client/test/__init__.py | 133 ++++++++------ dev_tools/pylibs/edgacknowledg/forge/http.py | 43 ++++- .../pylibs/edgacknowledg/forge/processor.py | 15 +- .../pylibs/edgacknowledg/forge/workspace.py | 120 +++++++++---- dev_tools/pylibs/edgtest/runtest/__init__.py | 136 ++++++++++++-- dev_tools/pylibs/edgutil/__init__.py | 49 +++++ .../acknowledg-forge/tests/test_display.py | 127 +++++++++++++ .../acknowledg-forge/tests/test_github.py | 35 ++++ .../tests/test_rematerialize.py | 167 +++++++++++++++++- .../acknowledg-forge/tests/test_safety.py | 119 ++++++++++++- .../acknowledg-forge/tests/test_symlinks.py | 15 +- dev_tools/services/acknowledg/PROTOCOL.md | 42 ++++- .../acknowledg/client/src/BenchReviewView.vue | 25 ++- .../acknowledg/client/src/TestReviewView.vue | 32 +++- .../acknowledg/client/src/display-content.js | 27 +++ .../services/acknowledg/client/src/sockets.js | 6 +- 19 files changed, 1120 insertions(+), 170 deletions(-) create mode 100644 dev_tools/services/acknowledg-forge/tests/test_display.py create mode 100644 dev_tools/services/acknowledg/client/src/display-content.js diff --git a/dev_tools/bin/edg-rematerialize-test-run b/dev_tools/bin/edg-rematerialize-test-run index 4f2c99b7f1..d4c3948613 100755 --- a/dev_tools/bin/edg-rematerialize-test-run +++ b/dev_tools/bin/edg-rematerialize-test-run @@ -73,8 +73,14 @@ def build_diff_files(diff_file: Path, curr_diff_file_descr = None diff_file_to_handle = {} - for line in diff_file_handle: - line_match = GIT_DIFF_REGEX.match(line) + # Lines are read in bounded pieces so that an enormous one is never held + # whole; only a piece that starts a line can be a header, and every piece + # is written through unchanged. + for line, at_line_start in edgutil.iter_line_pieces( + diff_file_handle, + edgutil.MAX_LINE_CHARS + ): + line_match = GIT_DIFF_REGEX.match(line) if at_line_start else None if line_match is not None: if curr_diff_file_descr is not None: # Close the file handle for the previous diff file (if any) to @@ -180,8 +186,15 @@ def collect_affected_tests(test_home: Path, suite_name = file_path.stem # An expectations update. with open(diff_file.diff_file) as file_handle: - for line in file_handle: - if line.startswith('---') or line.startswith('+++'): + # edgutil.MAX_LINE_CHARS is far more than any genuine expectation + # entry needs, so a longer line is refused (as when the file is + # read), without being held whole. + try: + for line in edgutil.iter_limited_lines( + file_handle, + edgutil.MAX_LINE_CHARS + ): + if line.startswith('---') or line.startswith('+++'): # Do not take in lines like: # # --- a/tests/expectations/edg_x86_64_cp/modules.log @@ -189,13 +202,19 @@ def collect_affected_tests(test_home: Path, # continue - if line.startswith('-') or line.startswith('+'): - # Do a naive parse to get the test file name. - test_file_rel_path = PurePosixPath(line[1:].split(':')[0]) - result.tests.add( - tests_dir.joinpath(suite_name, test_file_rel_path) - ) - result.configs.add(config_name) + if line.startswith('-') or line.startswith('+'): + # Do a naive parse to get the test file name. + test_file_rel_path = PurePosixPath(line[1:].split(':')[0]) + result.tests.add( + tests_dir.joinpath(suite_name, test_file_rel_path) + ) + result.configs.add(config_name) + except edgutil.LineTooLongError: + eprint( + f"{diff_file.file_name}: a changed line is longer than " + f"{edgutil.MAX_LINE_CHARS} characters" + ) + sys.exit(1) return result @@ -548,10 +567,20 @@ async def simulate_test(test_home: Path, config_run_dir: Path, return test_status_lines def read_suite_expectation_lines(suite_file: Path) -> List[str]: + '''The lines of ``suite_file`` (none if there is no such file); exits if + one is longer than ``edgutil.MAX_LINE_CHARS``, rather than holding it. + ''' if not suite_file.exists(): return [] - with open(suite_file, 'r') as file_handle: - return file_handle.readlines() + try: + with open(suite_file, 'r') as file_handle: + return list(edgutil.iter_limited_lines( + file_handle, + edgutil.MAX_LINE_CHARS + )) + except edgutil.LineTooLongError as ex: + eprint(f"{suite_file}: {ex}") + sys.exit(1) def load_test_suite_expectations(all_expectations: Dict[str, List[str]], suite_name: str, expectations_dir: Path, diff --git a/dev_tools/pylibs/edgacknowledg/__init__.py b/dev_tools/pylibs/edgacknowledg/__init__.py index 61697fc2f7..403706faed 100644 --- a/dev_tools/pylibs/edgacknowledg/__init__.py +++ b/dev_tools/pylibs/edgacknowledg/__init__.py @@ -121,12 +121,17 @@ def request_tags_with_prefix(prefix: str) -> FrozenSet[str]: ALLOWED_BENCH_EDIT_TAGS ) -PROTOCOL_REVISION = 19 +PROTOCOL_REVISION = 20 # 10MiB MAX_MESSAGE_SIZE = 10485760 # 8MiB MAX_DISPLAY_CONTENT_SIZE = 8388608 +# 16Ki characters: a longer line is cut for display (see lines_truncated). +MAX_DISPLAY_LINE_SIZE = 16384 +# 1Ki characters: a longer label (e.g. a status line's variant, at most a few +# hundred in practice) is cut for display, ending in an ellipsis. +MAX_DISPLAY_LABEL_SIZE = 1024 WebSocketsMessageType = Union[str, bytes] ProtocolMessage = Dict[str, Any] diff --git a/dev_tools/pylibs/edgacknowledg/client/shared.py b/dev_tools/pylibs/edgacknowledg/client/shared.py index 675f781cc1..ad2a243f7c 100644 --- a/dev_tools/pylibs/edgacknowledg/client/shared.py +++ b/dev_tools/pylibs/edgacknowledg/client/shared.py @@ -7,6 +7,7 @@ import asyncio import errno +import io import os import signal import stat @@ -19,6 +20,7 @@ Dict, List, IO, + Iterable, Iterator, NamedTuple, Optional, @@ -37,6 +39,8 @@ from edgacknowledg import ( MAX_DISPLAY_CONTENT_SIZE, + MAX_DISPLAY_LABEL_SIZE, + MAX_DISPLAY_LINE_SIZE, ProtocolMessage, RequestTags, RichName, @@ -83,6 +87,8 @@ async def handle_request(self, channel: Channel) -> None: ReviewClientHandler = Any # type: ignore[misc,assignment] +import edgutil + from edgutil import get_running_event_loop, is_path_relative_to CoroFactory = Callable[[], Awaitable[Any]] @@ -149,32 +155,111 @@ async def announce_section(channel: Channel, )) -def trunc_display_content(content: str) -> Dict[str, Union[str, bool]]: - '''Truncate ``content`` for UI display; may set ``truncated``.''' - max_content_size = MAX_DISPLAY_CONTENT_SIZE - if len(content) > max_content_size: - # Attempt to truncate at line boundaries. - # - # To save time, find the newline closest to the boundary before walking - # the file size back. - last_newline_permitted = content.find('\n', max_content_size) - if last_newline_permitted != -1: - content = content[:last_newline_permitted] - # Now walk the file size back. - while len(content) > max_content_size: - last_new_line_idx = content.rfind('\n') - if last_new_line_idx != -1: - content = content[:(last_new_line_idx - 1)] - else: - # If there are no more newlines, resort to truncating within - # the line. - content = content[:(max_content_size - 4)] - content += '...' - return { - 'truncated': True, - 'content': content - } - return { 'content': content } +def trunc_display_label(text: str) -> str: + '''``text`` (one line, e.g. a status) cut for UI display to + ``MAX_DISPLAY_LABEL_SIZE`` characters, a cut ending in an ellipsis. + ''' + if len(text) > MAX_DISPLAY_LABEL_SIZE: + text = text[:MAX_DISPLAY_LABEL_SIZE - 1] + '\N{HORIZONTAL ELLIPSIS}' + return text + + +def _display_lines( + pieces: Iterable[Tuple[str, bool]]) -> Iterator[Tuple[str, str, bool]]: + '''Join ``(piece, at_line_start)`` pieces (as ``edgutil.iter_line_pieces`` + yields them) into lines, yielding ``(text, line_end, cut)`` for each: at + most ``MAX_DISPLAY_LINE_SIZE`` characters of its text (``cut`` if it had + more, which are dropped as they come), and its end ('\\n', or '' for a + last line without one). + + A piece that starts a line ends the one before it, so a text that doesn't + end in a line end (e.g. a command's output) never runs into the next. + ''' + # The line being joined (as much of it as is kept), and whether some was + # dropped; ``None`` between lines. + line: Optional[str] = None + cut = False + for piece, at_line_start in pieces: + if line is not None and at_line_start: + yield line, '\n', cut + line = None + if line is None: + line = '' + cut = False + text = piece[:-1] if piece.endswith('\n') else piece + room = MAX_DISPLAY_LINE_SIZE - len(line) + if len(text) > room: + cut = True + line += text[:room] + if piece.endswith('\n'): + yield line, '\n', cut + line = None + if line is not None: + yield line, '', cut + + +def display_content(pieces: Iterable[Tuple[str, bool]]) -> Dict[str, Any]: + '''The ``content`` of a display payload, from ``(piece, at_line_start)`` + pieces (as ``edgutil.iter_line_pieces`` yields them), and as needed + ``truncated`` and ``lines_truncated``: + + - Each line is cut to ``MAX_DISPLAY_LINE_SIZE`` characters, its 1-based + number listed in ``lines_truncated``. + - The whole is cut to ``MAX_DISPLAY_CONTENT_SIZE``, after the last line + that fits (setting ``truncated``); no more pieces are taken then, so + what they come from is read no further. (Closing ``pieces``, should it + be a generator with files open, is left to the caller.) + + Neither key is set when nothing was cut, and nothing marks a cut in the + content itself: the UI does. + ''' + kept: List[str] = [] + size = 0 + lines_truncated: List[int] = [] + truncated = False + for line_number, (text, line_end, cut) in enumerate( + _display_lines(pieces), + start = 1 + ): + num_chars_for_line = len(text) + len(line_end) + if size + num_chars_for_line > MAX_DISPLAY_CONTENT_SIZE: + truncated = True + if not kept: + # The first line alone doesn't fit (MAX_DISPLAY_LINE_SIZE is + # larger than MAX_DISPLAY_CONTENT_SIZE): keep what does of it, as + # a cut line. + kept.append(text[:MAX_DISPLAY_CONTENT_SIZE]) + lines_truncated.append(line_number) + break + kept.append(text + line_end) + size += num_chars_for_line + if cut: + lines_truncated.append(line_number) + + content = ''.join(kept) + if truncated and content.endswith('\n'): + # Without the end of the last line kept. + content = content[:-1] + result: Dict[str, Any] = { 'content': content } + if truncated: + result['truncated'] = True + if lines_truncated: + result['lines_truncated'] = lines_truncated + return result + + +def display_text(handle: IO[str]) -> Dict[str, Any]: + '''``display_content`` of the text ``handle`` reads: only as much as is + shown, and of a line, at most a character more. + ''' + return display_content( + edgutil.iter_line_pieces(handle, MAX_DISPLAY_LINE_SIZE + 1) + ) + + +def trunc_display_content(content: str) -> Dict[str, Any]: + '''``display_content`` of ``content``, already in memory.''' + return display_text(io.StringIO(content)) def reviewer_of(request: Any) -> Optional[str]: diff --git a/dev_tools/pylibs/edgacknowledg/client/test/__init__.py b/dev_tools/pylibs/edgacknowledg/client/test/__init__.py index c6a40c1b55..827df4a015 100644 --- a/dev_tools/pylibs/edgacknowledg/client/test/__init__.py +++ b/dev_tools/pylibs/edgacknowledg/client/test/__init__.py @@ -6,6 +6,9 @@ '''AcknowlEDG test-review provider package.''' import asyncio +import contextlib +import filecmp +import functools import os import re import shutil @@ -27,7 +30,7 @@ ) from edgacknowledg import ( - MAX_DISPLAY_CONTENT_SIZE, + MAX_DISPLAY_LINE_SIZE, ProtocolMessage, RequestTags, WebSocketsMessageType, @@ -40,8 +43,11 @@ ConfinedPath, IllegalFileAccessError, SandboxContext, + display_content, + display_text, reviewer_of, trunc_display_content, + trunc_display_label, ) from edgutil import eprint @@ -70,7 +76,13 @@ def _create_status_dict(elog_file_paths: List[ConfinedPath], *, for elog_file_path in elog_file_paths: config_name = elog_file_path.parent.name with elog_file_path.open() as file_handle: - for line in file_handle: + # edgutil.MAX_LINE_CHARS is far more than any genuine status line + # needs, so a longer line is refused (failing the request, as a line + # that isn't a status line does), without being held whole. + for line in edgutil.iter_limited_lines( + file_handle, + edgutil.MAX_LINE_CHARS + ): key, parsed_diff = ( edgtest.runtest.parse_test_status_diff(line) ) @@ -79,7 +91,7 @@ def _create_status_dict(elog_file_paths: List[ConfinedPath], *, pending[key] = {} lines_by_test_config[key] = {} if parsed_diff.remark: - remarks[key] = parsed_diff.remark + remarks[key] = trunc_display_label(parsed_diff.remark) if config_name not in pending[key]: rt_file_path = _rt_path_for(elog_file_path.parent, key) @@ -89,9 +101,9 @@ def _create_status_dict(elog_file_paths: List[ConfinedPath], *, lines_by_test_config[key][config_name] = [] lines_by_test_config[key][config_name].append({ - "qualifier": parsed_diff.qualifier, - "variant": parsed_diff.variant, - "status": parsed_diff.status.full_status + "qualifier": trunc_display_label(parsed_diff.qualifier), + "variant": trunc_display_label(parsed_diff.variant), + "status": trunc_display_label(parsed_diff.status.full_status) }) keyed_statuses: _StatusDict = {} @@ -286,13 +298,17 @@ def test_id_to_path(test_id: str) -> ConfinedPath: )) return - async def form_curr_diff_text(config_name: str, - case_rt_path: ConfinedPath) -> str: - '''Live-diff observed vs recordings per command. - - Per-command body content is diffed (recording command line stripped), - then equal bodies are collapsed with # command comments — same - presentation as static. + async def write_curr_diff(config_name: str, + case_rt_path: ConfinedPath, + out_path: Path) -> None: + '''Write the live diff of observed output vs recordings to + ``out_path``. + + Per-command body content is diffed (recording command line + stripped), then equal bodies are collapsed with # command + comments, the same presentation as static. The per-command diffs + are files of its own, gone once ``out_path`` is written; none is + held in memory. ''' suite_name = test_id.split('/')[0] suite_path = test_home / 'tests' / suite_name @@ -311,8 +327,7 @@ async def form_curr_diff_text(config_name: str, with tempfile.TemporaryDirectory() as tmp_dir_name: tmp_dir = Path(tmp_dir_name) - entries: List[Tuple[Dict[str, Any], List[str]]] = [] - + entries: List[Tuple[Dict[str, Any], Path]] = [] with edgtest.runtest.RunTestOutput(case_rt_path.path) as rt_output: for case_number, case in enumerate( rt_output.metadata.get('cases', []), @@ -355,13 +370,9 @@ async def form_curr_diff_text(config_name: str, if diff_hash is None: continue - with open(change_diff_path, 'r', - errors = 'replace') as file_handle: - diff_lines = file_handle.readlines() - command_entry = dict(command) command_entry['diff_hash'] = diff_hash - entries.append((command_entry, diff_lines)) + entries.append((command_entry, change_diff_path)) if case.get('matches') is None: continue @@ -395,57 +406,80 @@ async def form_curr_diff_text(config_name: str, if matches_diff_hash is None: continue - with open(matches_change_diff, 'r', - errors = 'replace') as file_handle: - matches_diff_lines = file_handle.readlines() - matches_entry = { 'command': edgtest.runtest.case_matches_annotation( case_number ), 'diff_hash': matches_diff_hash } - entries.append((matches_entry, matches_diff_lines)) - - return ''.join( - edgtest.runtest.format_collapsed_case_diffs(entries) - ) + entries.append((matches_entry, matches_change_diff)) + + with open(out_path, 'w', encoding = 'utf-8') as out_handle: + with contextlib.closing( + edgtest.runtest.iter_collapsed_case_diff_pieces( + [ + (entry, + functools.partial(open, path, 'r', errors = 'replace')) + for entry, path in entries + ], + edgutil.MAX_LINE_CHARS + ) + ) as pieces: + for piece, _ in pieces: + out_handle.write(piece) + # What is shown is read only as far as it is shown: never a whole + # member of the run artifact, or a whole line of one. + piece_limit = MAX_DISPLAY_LINE_SIZE + 1 if request_tag == RequestTags.TEST_SHOW_DIFF: with edgtest.runtest.RunTestOutput(rt_file_path.path) as rt_output: - static_text = ''.join(rt_output.combined_static_diff()) + with contextlib.closing( + rt_output.iter_static_diff_pieces(piece_limit) + ) as pieces: + shown = display_content(pieces) client_response = { 'id': configured_test_id, - **trunc_display_content(static_text) + **shown } elif request_tag == RequestTags.TEST_SHOW_RAW_OUTPUT: with edgtest.runtest.RunTestOutput(rt_file_path.path) as rt_output: - raw_output = ''.join(rt_output.combined_raw_output()) + with contextlib.closing( + rt_output.iter_raw_output_pieces(piece_limit) + ) as pieces: + shown = display_content(pieces) client_response = { 'id': configured_test_id, - **trunc_display_content(raw_output) + **shown } elif request_tag == RequestTags.TEST_SHOW_CURR_DIFF: rep_config = changes_file_path.parent.name group_configs = request_value.get('configs') or [rep_config] - rep_curr_text = await form_curr_diff_text(rep_config, rt_file_path) divergent_configs: List[str] = [] - for config_name in group_configs: - if config_name == rep_config: - continue - sibling_changes = changes_files[config_name] - sibling_rt = _rt_path_for(sibling_changes.parent, test_id) - if not sibling_rt.exists(): - divergent_configs.append(config_name) - continue - sibling_text = await form_curr_diff_text(config_name, sibling_rt) - if sibling_text != rep_curr_text: - divergent_configs.append(config_name) + with tempfile.TemporaryDirectory() as tmp_dir_name: + rep_path = Path(tmp_dir_name, 'curr.diff') + await write_curr_diff(rep_config, rt_file_path, rep_path) + for config_name in group_configs: + if config_name == rep_config: + continue + sibling_changes = changes_files[config_name] + sibling_rt = _rt_path_for(sibling_changes.parent, test_id) + if not sibling_rt.exists(): + divergent_configs.append(config_name) + continue + # Compared a chunk at a time, then gone. + sibling_path = Path(tmp_dir_name, 'sibling-curr.diff') + await write_curr_diff(config_name, sibling_rt, sibling_path) + if not filecmp.cmp(rep_path, sibling_path, shallow = False): + divergent_configs.append(config_name) + sibling_path.unlink() + + with open(rep_path, encoding = 'utf-8') as curr_diff_handle: + shown = display_text(curr_diff_handle) client_response = { 'id': configured_test_id, - **trunc_display_content(rep_curr_text) + **shown } if len(divergent_configs) != 0: @@ -549,10 +583,9 @@ async def form_curr_diff_text(config_name: str, } )) else: - # Only as much as can be shown is read (one character more marks - # the content as truncated). + # Only as much as can be shown is read. with path_to_show.open(errors = 'replace') as file_handle: - shown = file_handle.read(MAX_DISPLAY_CONTENT_SIZE + 1) + shown = display_text(file_handle) await channel.send(message( RequestTags.TEST_SHOW_SOURCE, value = { @@ -560,7 +593,7 @@ async def form_curr_diff_text(config_name: str, 'client_response': { 'id': test_id, 'assoc_file': request_value.get('assoc_file'), - **trunc_display_content(shown) + **shown } } )) diff --git a/dev_tools/pylibs/edgacknowledg/forge/http.py b/dev_tools/pylibs/edgacknowledg/forge/http.py index 1398076465..7a905c86d5 100644 --- a/dev_tools/pylibs/edgacknowledg/forge/http.py +++ b/dev_tools/pylibs/edgacknowledg/forge/http.py @@ -26,6 +26,9 @@ _log = logging.getLogger(__name__) PAGE_SIZE = 100 +# The largest API response read (decoded): far more than a page of +# PAGE_SIZE items; downloads have their own limit. +MAX_RESPONSE_BYTES = 32 * 1024 * 1024 MAX_RATE_LIMIT_WAIT = 15 * 60.0 MAX_ATTEMPTS = 4 @@ -88,17 +91,19 @@ async def request(self, params: Optional[Mapping[str, Any]] = None, json_body: Optional[Any] = None) -> httpx.Response: '''Send a request, retrying transient failures and waiting out rate - limits; raise ``ForgeError`` on failure. + limits; raise ``ForgeError`` on failure, or for a response over + ``MAX_RESPONSE_BYTES`` (read only that far). ''' for attempt in range(MAX_ATTEMPTS): last_attempt = attempt + 1 == MAX_ATTEMPTS try: - response = await self._client.request( + request = self._client.build_request( method, path, params = params, json = json_body ) + response = await _send_capped(self._client, request, method, path) except httpx.HTTPError as ex: if last_attempt: raise ForgeError(f"{method} {path} failed: {ex}") from ex @@ -179,6 +184,40 @@ async def download(self, raise ForgeError(f"download of {path} failed: {ex}") from ex +async def _send_capped(client: httpx.AsyncClient, + request: httpx.Request, + method: str, + path: str) -> httpx.Response: + '''Send ``request``; return its response, its body read (decoded) as it + comes, and only to ``MAX_RESPONSE_BYTES``: ``ForgeError`` beyond. + ''' + response = await client.send(request, stream = True) + try: + body = bytearray() + async for chunk in response.aiter_bytes(): + body += chunk + if len(body) > MAX_RESPONSE_BYTES: + raise ForgeError( + f"{method} {path}: the response is over {MAX_RESPONSE_BYTES} bytes" + ) + finally: + await response.aclose() + # The same response with its body in place; it is decoded already, so + # without the headers that describe how it was sent. + headers = [ + (name, value) for name, value in response.headers.multi_items() + if name.lower() not in ('content-encoding', 'content-length', + 'transfer-encoding') + ] + return httpx.Response( + response.status_code, + headers = headers, + content = bytes(body), + request = request, + extensions = response.extensions + ) + + async def _write_capped(stream: httpx.Response, destination: Path, max_bytes: int, diff --git a/dev_tools/pylibs/edgacknowledg/forge/processor.py b/dev_tools/pylibs/edgacknowledg/forge/processor.py index 7f990d6a1f..3dbf594592 100644 --- a/dev_tools/pylibs/edgacknowledg/forge/processor.py +++ b/dev_tools/pylibs/edgacknowledg/forge/processor.py @@ -76,6 +76,9 @@ # message. MAX_CI_METADATA_BYTES = 64 * 1024 MAX_TEST_IDS_BYTES = 4 * 1024 * 1024 +# The longest test id taken from CI (a path within a suite); a longer line +# names no test, and is skipped rather than carried along. +MAX_TEST_ID_CHARS = 4096 # 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. @@ -117,7 +120,8 @@ def _reusable(section: Optional[SectionRecord], def _changes_test_ids(changes_elists: Iterable[Path]) -> list[str]: '''The test ids ``changes_elists`` name, within ``MAX_TEST_IDS_BYTES`` - (together, both as read and as JSON); ``WorkspaceError`` beyond. + (together, both as read and as JSON); ``WorkspaceError`` beyond. A line + longer than ``MAX_TEST_ID_CHARS`` is skipped. ''' test_ids = [] remaining = MAX_TEST_IDS_BYTES @@ -130,7 +134,14 @@ def _changes_test_ids(changes_elists: Iterable[Path]) -> list[str]: 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()] + for line in text.splitlines(): + if len(line) > MAX_TEST_ID_CHARS: + _log.warning('skipped a changes.elist line of %d characters', + len(line)) + continue + test_id = line.strip() + if test_id: + test_ids.append(test_id) # JSON escapes control and non-ASCII characters (e.g. the U+FFFD of bytes # that aren't UTF-8), up to six bytes each. if len(json.dumps(test_ids)) > MAX_TEST_IDS_BYTES: diff --git a/dev_tools/pylibs/edgacknowledg/forge/workspace.py b/dev_tools/pylibs/edgacknowledg/forge/workspace.py index f7f56d63b4..9c675ab37a 100644 --- a/dev_tools/pylibs/edgacknowledg/forge/workspace.py +++ b/dev_tools/pylibs/edgacknowledg/forge/workspace.py @@ -40,7 +40,7 @@ from dataclasses import dataclass from pathlib import Path, PurePosixPath -from typing import Callable, Iterable, Optional, Sequence +from typing import IO, Callable, Iterable, Optional, Sequence import edgtest import edgutil @@ -73,6 +73,14 @@ # files) before it is killed; well within docker stop's default 10. _TERMINATE_ALLOWANCE = 5.0 +# The most output a git command run for its result (not one whose output +# goes to a sink) may have; far more than a rev-parse or for-each-ref of a +# pull request's refs has. +_MAX_GIT_OUTPUT_BYTES = 16 * 1024 * 1024 +# How much of a git command's stderr is kept (the rest is read, and +# dropped): a fetch relays what the remote says, which needn't be short. +_MAX_GIT_STDERR_BYTES = 64 * 1024 + class WorkspaceError(Exception): '''Raised when a workspace operation fails or input is unsafe.''' @@ -135,28 +143,26 @@ def _patterns_for_changed_test_file(path: PurePosixPath) -> list[str]: return [f"/{_escape_pattern(str(path.parent))}/"] -def changed_paths(diff: str) -> list[str]: - '''Return the paths a ``git diff --no-renames`` output touches.''' - paths = [] - for line in diff.splitlines(): - match = _DIFF_HEADER.match(line) - if match is not None: - paths.append(match.group(1)) - return paths - - -def changes_patterns(lines_iter: Iterable[str], +def changes_patterns(diff: IO[str], *, max_patterns: int = DEFAULT_MAX_PATTERNS) -> list[str]: - '''Sparse-checkout patterns for rematerializing the diff (of tests/) of - ``lines_iter`` (with or without their line ends, e.g. a file's) on the - base tree: the test home and every affected test; at most - ``max_patterns``. + '''Sparse-checkout patterns for rematerializing the diff (of tests/) read + from ``diff`` on the base tree: the test home and every affected test; at + most ``max_patterns``. ''' patterns = dict.fromkeys(_TEST_HOME_PATTERNS) current_suite: Optional[str] = None - for line in lines_iter: - line = line.rstrip('\n') + limit = edgutil.MAX_LINE_CHARS + for piece, at_line_start in edgutil.iter_line_pieces(diff, limit + 1): + # Only whole lines within edgutil.MAX_LINE_CHARS matter: no header or + # expectation line is longer, and a longer one (e.g. of a recording) is + # passed over in pieces, never held whole. + if not at_line_start: + continue + text_length = len(piece) - (1 if piece.endswith('\n') else 0) + if text_length > limit: + continue + line = piece.rstrip('\n') match = _DIFF_HEADER.match(line) if match is not None: path = PurePosixPath(match.group(1)) @@ -491,7 +497,9 @@ async def git(self, '''Run git; with ``stdout_sink``, its output goes there as it comes, rather than into the result, and may be at most ``max_output`` bytes (it is stopped, and ``_OutputTooLarge`` raised, beyond: the caller - knows which limit that is). + knows which limit that is). Without, its output goes into the result, + and may be at most ``_MAX_GIT_OUTPUT_BYTES`` (``WorkspaceError`` + beyond). Only the start of its stderr is kept. ''' command = ['git'] if git_dir is not None: @@ -508,14 +516,30 @@ async def git(self, cwd = cwd, env = self._git_env() ) + stdin_bytes = stdin.encode() if stdin is not None else None if stdout_sink is None: assert max_output is None - stdout, stderr = await process.communicate( - stdin.encode() if stdin is not None else None - ) + collected = bytearray() + try: + stderr = await _communicate_bounded( + process, + _MAX_GIT_OUTPUT_BYTES, + collected.extend, + stdin_bytes + ) + except _OutputTooLarge: + raise WorkspaceError( + f"git {' '.join(args)} wrote over {_MAX_GIT_OUTPUT_BYTES} bytes" + ) from None + stdout = bytes(collected) else: - assert stdin is None and max_output is not None - stderr = await _communicate_bounded(process, max_output, stdout_sink) + assert max_output is not None + stderr = await _communicate_bounded( + process, + max_output, + stdout_sink, + stdin_bytes + ) stdout = b'' assert process.returncode is not None result = GitResult( @@ -736,17 +760,49 @@ async def collect_garbage(self, repository: str, open_count: int) -> None: await self.git('gc', '--auto', '--quiet', git_dir = clone_dir) +async def _read_start(stream: asyncio.StreamReader, keep: int) -> bytes: + '''Read ``stream`` to its end; return its first ``keep`` bytes (the rest + is dropped as it comes). + ''' + kept = bytearray() + while chunk := await stream.read(64 * 1024): + kept += chunk[:max(0, keep - len(kept))] + return bytes(kept) + + +async def _write_stdin(process: asyncio.subprocess.Process, + data: bytes) -> None: + '''Write ``data`` to ``process``'s stdin, then close it.''' + assert process.stdin is not None + try: + process.stdin.write(data) + await process.stdin.drain() + except (BrokenPipeError, ConnectionResetError): + # It exited without reading it all; its exit status tells. + pass + finally: + process.stdin.close() + + async def _communicate_bounded(process: asyncio.subprocess.Process, limit: int, - sink: Callable[[bytes], None]) -> bytes: + sink: Callable[[bytes], None], + stdin: Optional[bytes] = None) -> bytes: '''Pass ``process``'s stdout to ``sink`` as it comes, in chunks (none held - here); return its stderr once it exits. Should its stdout be over - ``limit`` bytes, it is stopped, and ``_OutputTooLarge`` raised. Should - this be cancelled (or fail), it is stopped too, rather than left writing - to a pipe no one reads. + here), having written ``stdin`` (if any) to it; return the start of its + stderr (``_MAX_GIT_STDERR_BYTES``) once it exits. Should its stdout be + over ``limit`` bytes, it is stopped, and ``_OutputTooLarge`` raised. + Should this be cancelled (or fail), it is stopped too, rather than left + writing to a pipe no one reads. ''' assert process.stdout is not None and process.stderr is not None - reading_stderr = asyncio.create_task(process.stderr.read()) + reading_stderr = asyncio.create_task( + _read_start(process.stderr, _MAX_GIT_STDERR_BYTES) + ) + writing_stdin = ( + asyncio.create_task(_write_stdin(process, stdin)) + if stdin is not None else None + ) size = 0 try: while chunk := await process.stdout.read(64 * 1024): @@ -754,6 +810,8 @@ async def _communicate_bounded(process: asyncio.subprocess.Process, if size > limit: raise _OutputTooLarge() sink(chunk) + if writing_stdin is not None: + await writing_stdin stderr = await reading_stderr await process.wait() return stderr @@ -762,6 +820,8 @@ async def _communicate_bounded(process: asyncio.subprocess.Process, raise finally: reading_stderr.cancel() + if writing_stdin is not None: + writing_stdin.cancel() def basic_auth_header(user: str, password: str) -> str: diff --git a/dev_tools/pylibs/edgtest/runtest/__init__.py b/dev_tools/pylibs/edgtest/runtest/__init__.py index 0308d4b651..ff0002bdb2 100644 --- a/dev_tools/pylibs/edgtest/runtest/__init__.py +++ b/dev_tools/pylibs/edgtest/runtest/__init__.py @@ -12,6 +12,7 @@ import functools import hashlib import io +import itertools import json import os import re @@ -27,8 +28,8 @@ from collections import OrderedDict from pathlib import Path, PurePath from typing import ( - Any, Callable, Dict, IO, Iterator, List, Mapping, NamedTuple, Optional, - Pattern, Set, Tuple, cast + Any, Callable, Dict, Generator, IO, Iterator, List, Mapping, NamedTuple, + Optional, Pattern, Set, Tuple, cast ) def get_default_config_name(env: Optional[Dict[str, str]] = None) -> str: @@ -1195,6 +1196,59 @@ def emit_one(commands: List[Dict[str, Any]], result.extend(emit_one(group['commands'], group['diff_lines'])) return result +# A text that is opened when (and if) it is read. +TextOpener = Callable[[], IO[str]] + +def _hash_text_stream(open_text: TextOpener) -> str: + '''``hash_diff_text`` of the text ``open_text`` opens, read in chunks.''' + digest = hashlib.sha256() + with open_text() as handle: + while True: + chunk = handle.read(64 * 1024) + if not chunk: + break + digest.update(chunk.encode('utf-8', errors = 'replace')) + return digest.hexdigest() + +def iter_collapsed_case_diff_pieces( + command_diff_entries: List[Tuple[Dict[str, Any], TextOpener]], + limit: int) -> Generator[Tuple[str, bool], None, None]: + '''The collapsed ``format_collapsed_case_diffs`` of + ``command_diff_entries`` (with their stored labels), whose diffs are + opened rather than given as lines, as ``edgutil.iter_line_pieces`` pieces + of at most ``limit`` characters (each ``# command`` comment is one piece, + however long). A diff is read only as far as the pieces are taken, so + neither a long line nor a long diff is held whole. + ''' + # First-seen order of unique diff bodies (keyed by stored hash). + groups: Dict[str, Tuple[List[Dict[str, Any]], TextOpener]] = ( + OrderedDict() + ) + for command, open_text in command_diff_entries: + group_key = command.get('diff_hash') or _hash_text_stream(open_text) + if group_key not in groups: + groups[group_key] = ([], open_text) + groups[group_key][0].append(command) + + for commands, open_text in groups.values(): + with open_text() as handle: + pieces = edgutil.iter_line_pieces(handle, limit) + # The ---/+++ header, if it has one, comes before the comments; its + # lines are short (labels), so each must be one piece: a diff whose + # first lines are longer has no header (and isn't held to look). + head = list(itertools.islice(pieces, 2)) + if (len(head) == 2 and + all(at_line_start and piece.endswith('\n') + for piece, at_line_start in head) and + head[0][0].startswith('---') and + head[1][0].startswith('+++')): + yield from head + head = [] + for command in commands: + yield f"# {case_command_from_metadata(command)}\n", True + yield from head + yield from pieces + class RtTarContents: '''Manifest of filesystem files to pack into a .rt.tar[.gz]. @@ -1349,6 +1403,16 @@ def _require_tar(self) -> tarfile.TarFile: def _read_lines(self, member_name: str) -> List[str]: return _read_tar_lines_member(self._require_tar(), member_name) + def open_text_member(self, member_name: str) -> IO[str]: + '''Open a member as text (UTF-8, errors replaced), read as it is + decompressed: nothing is held but what is read. + ''' + return io.TextIOWrapper( + _extract_tar_member(self._require_tar(), member_name), + encoding = 'utf-8', + errors = 'replace' + ) + @property def metadata(self) -> Dict[str, Any]: if self._metadata is None: @@ -1430,9 +1494,9 @@ def iter_command_diffs(self) -> Iterator[List[str]]: for _entry, diff_lines in self.iter_command_diff_entries(): yield diff_lines - def iter_command_diff_entries( - self) -> Iterator[Tuple[Dict[str, Any], List[str]]]: - '''Yield (annotation metadata, change.diff lines) for diffs. + def iter_command_diff_members( + self) -> Iterator[Tuple[Dict[str, Any], str]]: + '''Yield (annotation metadata, change.diff member name) for diffs. Command diffs use the stored command metadata. Matches diffs use a synthetic command string ``Matches for case N``. @@ -1443,21 +1507,29 @@ def iter_command_diff_entries( case.get('commands', []), start = 1 ): - diff_lines = self.command_diff(case_number, command_number) - if diff_lines is not None: - yield command, diff_lines + member = f"cases/{case_number}/{command_number}/change.diff" + if member in self.member_names: + yield command, member matches = case.get('matches') if matches is None: continue - matches_diff_lines = self.case_matches_diff(case_number) - if matches_diff_lines is None: + member = f"cases/{case_number}/matches/change.diff" + if member not in self.member_names: continue matches_entry = { 'command': case_matches_annotation(case_number), 'diff_hash': matches.get('diff_hash') } - yield matches_entry, matches_diff_lines + yield matches_entry, member + + def iter_command_diff_entries( + self) -> Iterator[Tuple[Dict[str, Any], List[str]]]: + '''Yield (annotation metadata, change.diff lines) for diffs (see + ``iter_command_diff_members``). + ''' + for entry, member in self.iter_command_diff_members(): + yield entry, self._read_lines(member) def combined_static_diff(self, *, @@ -1475,6 +1547,48 @@ def combined_static_diff(self, collapse = collapse ) + def iter_static_diff_pieces( + self, + limit: int) -> Generator[Tuple[str, bool], None, None]: + '''``combined_static_diff()`` (collapsed, with the stored labels) as + ``edgutil.iter_line_pieces`` pieces of at most ``limit`` characters, + each member read only as far as the pieces are taken. + ''' + return iter_collapsed_case_diff_pieces( + [ + (entry, functools.partial(self.open_text_member, member)) + for entry, member in self.iter_command_diff_members() + ], + limit + ) + + def iter_raw_output_pieces( + self, + limit: int) -> Generator[Tuple[str, bool], None, None]: + '''``combined_raw_output()`` as ``edgutil.iter_line_pieces`` pieces of + at most ``limit`` characters (each ``# `` header line is one piece, + however long), each member read only as far as the pieces are taken. + ''' + for case_number, case in enumerate(self.metadata.get('cases', []), + start = 1): + for command_number, command in enumerate( + case.get('commands', []), + start = 1 + ): + command_str = case_command_from_metadata(command) + if command_str: + yield f"# {command_str}\n", True + with self.open_text_member( + f"cases/{case_number}/{command_number}/output.txt" + ) as handle: + yield from edgutil.iter_line_pieces(handle, limit) + + matches_member = f"cases/{case_number}/matches/output.txt" + if matches_member in self.member_names: + yield f"# {case_matches_annotation(case_number)}\n", True + with self.open_text_member(matches_member) as handle: + yield from edgutil.iter_line_pieces(handle, limit) + def combined_raw_output(self) -> List[str]: '''Concatenate command lines from metadata with each command's body. diff --git a/dev_tools/pylibs/edgutil/__init__.py b/dev_tools/pylibs/edgutil/__init__.py index e2dba357ea..a817d13729 100644 --- a/dev_tools/pylibs/edgutil/__init__.py +++ b/dev_tools/pylibs/edgutil/__init__.py @@ -27,7 +27,9 @@ Callable, Coroutine, Dict, + IO, Iterable, + Iterator, List, Optional, Set, @@ -193,6 +195,53 @@ def __exit__(self, def eprint(msg: str) -> None: print(msg, file = sys.stderr) +def iter_line_pieces(handle: IO[str], + limit: int) -> Iterator[Tuple[str, bool]]: + '''Yield ``(piece, at_line_start)`` for the text ``handle``, a line at a + time but never holding more than ``limit`` characters of one: a longer + line comes as several pieces, only the first of which has + ``at_line_start`` set. Concatenating the pieces gives back the text + exactly. + ''' + at_line_start = True + while True: + piece = handle.readline(limit) + if not piece: + break + yield piece, at_line_start + at_line_start = piece.endswith('\n') + +class LineTooLongError(ValueError): + '''Raised by ``iter_limited_lines`` for a line over its limit.''' + + def __init__(self, line_number: int, limit: int) -> None: + super().__init__( + f"line {line_number} is longer than {limit} characters" + ) + self.line_number = line_number + self.limit = limit + +def iter_limited_lines(handle: IO[str], limit: int) -> Iterator[str]: + '''Yield each line of the text ``handle``, with its line end ('\\n') if + it has one; raise ``LineTooLongError`` for a line longer than ``limit`` + characters (its end not counted), having read only ``limit + 1`` of + them. + ''' + # Every piece up to an overlong line's first is a whole line. + for line_number, (line, _) in enumerate( + iter_line_pieces(handle, limit + 1), + start = 1 + ): + text_length = len(line) - (1 if line.endswith('\n') else 0) + if text_length > limit: + raise LineTooLongError(line_number, limit) + yield line + +# The longest line a reader of untrusted line-oriented text (a pull +# request's diff, a CI artifact's files) need hold: far more than any path, +# test name, or status line, and small enough not to matter in memory. +MAX_LINE_CHARS = 64 * 1024 + def indent_text_block(text_block: str, n_times: int = 1) -> str: indent = ' ' * n_times return re.sub(r'^(.)', f"{indent}\\1", text_block, flags = re.MULTILINE) diff --git a/dev_tools/services/acknowledg-forge/tests/test_display.py b/dev_tools/services/acknowledg-forge/tests/test_display.py new file mode 100644 index 0000000000..f4e43c2fc6 --- /dev/null +++ b/dev_tools/services/acknowledg-forge/tests/test_display.py @@ -0,0 +1,127 @@ +# 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 + +'''Content cut for display to reviewers: whole, and line by line.''' + +import contextlib +import time + +from typing import Iterator + +import pytest + +from edgacknowledg import MAX_DISPLAY_LABEL_SIZE +from edgacknowledg.client import shared +from edgacknowledg.client.shared import ( + display_content, + trunc_display_content, + trunc_display_label, +) + + +@pytest.fixture +def small_limits(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr(shared, 'MAX_DISPLAY_CONTENT_SIZE', 40) + monkeypatch.setattr(shared, 'MAX_DISPLAY_LINE_SIZE', 8) + + +def test_short_content_is_untouched(small_limits: None) -> None: + for content in ('', 'a', 'a\nb\n', 'a\n\n\nb', '12345678\n'): + assert trunc_display_content(content) == {'content': content} + + +def test_long_lines_are_cut_and_numbered(small_limits: None) -> None: + result = trunc_display_content('short\n123456789\nok\n' + 'x' * 20) + assert result == { + 'content': 'short\n12345678\nok\n' + 'x' * 8, + 'lines_truncated': [2, 4], + } + + +def test_long_content_is_cut_at_a_line_end(small_limits: None) -> None: + # Eight lines of six (with their ends) fit six times in 40. + lines = [f"line{n}" for n in range(8)] + result = trunc_display_content('\n'.join(lines)) + assert result == { + 'content': '\n'.join(lines[:6]), + 'truncated': True, + } + + +def test_cut_lines_count_as_cut(small_limits: None) -> None: + # Lines cut, then the whole: only lines kept are listed. + content = '\n'.join(['y' * 20] * 10) + result = trunc_display_content(content) + assert result['truncated'] + assert result['content'] == '\n'.join(['y' * 8] * 4) + assert result['lines_truncated'] == [1, 2, 3, 4] + assert len(result['content']) <= 40 + + +def test_one_line_larger_than_everything( + monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr(shared, 'MAX_DISPLAY_CONTENT_SIZE', 10) + monkeypatch.setattr(shared, 'MAX_DISPLAY_LINE_SIZE', 100) + assert trunc_display_content('z' * 50 + '\nmore') == { + 'content': 'z' * 10, + 'truncated': True, + 'lines_truncated': [1], + } + + +def test_no_characters_are_lost_between_lines(small_limits: None) -> None: + # The old walk back dropped the character before a line end. + lines = ['abcdefg'] * 10 + content = trunc_display_content('\n'.join(lines))['content'] + assert all(line == 'abcdefg' for line in content.split('\n')) + + +def test_large_content_is_linear() -> None: + # Many short lines past the content limit, and a huge one, go quickly. + content = 'a\n' * 8_000_000 + 'b' * 20_000_000 + start = time.monotonic() + result = trunc_display_content(content) + assert time.monotonic() - start < 20 + assert result['truncated'] + assert 'lines_truncated' not in result + + +def test_labels_end_in_an_ellipsis() -> None: + assert trunc_display_label('PASS') == 'PASS' + fits = 'x' * MAX_DISPLAY_LABEL_SIZE + assert trunc_display_label(fits) == fits + label = trunc_display_label(fits + 'x') + assert len(label) == MAX_DISPLAY_LABEL_SIZE + assert label.endswith('\N{HORIZONTAL ELLIPSIS}') + + +def test_display_reads_only_what_fits(small_limits: None) -> None: + taken: list[str] = [] + + def pieces() -> Iterator[tuple[str, bool]]: + for n in range(1000): + taken.append(f"{n}") + yield f"line{n}\n", True + + with contextlib.closing(pieces()) as generator: + shown = display_content(generator) + assert shown['truncated'] + # Up to the first line that doesn't fit, and no more. + assert len(taken) == 7 + assert shown['content'] == '\n'.join(f"line{n}" for n in range(6)) + + +def test_display_cuts_lines_across_pieces(small_limits: None) -> None: + # A line in several pieces; then a text without a last line end, which + # the next line doesn't run into. + pieces = [ + ('abcde', True), ('fghij', False), ('k\n', False), + ('no end', True), + ('# next\n', True), + ] + assert display_content(pieces) == { + 'content': 'abcdefgh\nno end\n# next\n', + 'lines_truncated': [1], + } diff --git a/dev_tools/services/acknowledg-forge/tests/test_github.py b/dev_tools/services/acknowledg-forge/tests/test_github.py index 3c333825a7..be2b08f64a 100644 --- a/dev_tools/services/acknowledg-forge/tests/test_github.py +++ b/dev_tools/services/acknowledg-forge/tests/test_github.py @@ -235,3 +235,38 @@ def handler(request: httpx.Request) -> httpx.Response: assert posted == [{'body': 'hello'}] assert await commenter.find_comment(REPO, 1, 'MARK') == '2' assert await commenter.find_comment(REPO, 1, 'OTHER') is None + + +async def test_responses_are_read_only_so_far( + monkeypatch: pytest.MonkeyPatch) -> None: + from edgacknowledg.forge import ForgeError + from edgacknowledg.forge import http as forge_http + import gzip + + def handler(request: httpx.Request) -> httpx.Response: + if request.url.path == '/small': + # Compressed, and paginated: decoded once, links kept. + return httpx.Response( + 200, + headers = { + 'content-encoding': 'gzip', + 'link': f'<{API}/small?page=2>; rel="next"', + }, + content = gzip.compress(json.dumps({'ok': True}).encode()) + ) + return httpx.Response(200, json = {'pad': 'x' * 1000}) + + client = forge_http.ForgeHttpClient( + base_url = API, + headers = {}, + transport = httpx.MockTransport(handler) + ) + try: + monkeypatch.setattr(forge_http, 'MAX_RESPONSE_BYTES', 100) + response = await client.request('GET', '/small') + assert response.json() == {'ok': True} + assert response.links['next']['url'] == f"{API}/small?page=2" + with pytest.raises(ForgeError, match = 'over 100 bytes'): + await client.get_json('/large') + finally: + await client.aclose() diff --git a/dev_tools/services/acknowledg-forge/tests/test_rematerialize.py b/dev_tools/services/acknowledg-forge/tests/test_rematerialize.py index 7123d85ca2..05dcea285b 100644 --- a/dev_tools/services/acknowledg-forge/tests/test_rematerialize.py +++ b/dev_tools/services/acknowledg-forge/tests/test_rematerialize.py @@ -12,8 +12,18 @@ import sys from pathlib import Path, PurePosixPath +from typing import Any import edgtest +import edgutil + +# (Not 'from ... import TestSourceHandler', which pytest would collect.) +import edgacknowledg.client.test as test_provider + +from edgacknowledg import RequestTags, message +from edgacknowledg.client.shared import SandboxContext + +from test_symlinks import FakeChannel DEV_TOOLS = Path(__file__).resolve().parents[3] REMATERIALIZE = DEV_TOOLS / 'bin' / 'edg-rematerialize-test-run' @@ -155,7 +165,16 @@ def static_diff(test: str) -> str: edgtest.runtest.RUN_TEST_OUTPUT_SUFFIX ) with edgtest.runtest.RunTestOutput(rt_path) as rt_output: - return ''.join(rt_output.combined_static_diff()) + text = ''.join(rt_output.combined_static_diff()) + # As pieces (for display), the same, header lines fitting in one. + for limit in (len('--- Expected Output\n'), 1000): + assert ''.join( + piece for piece, _ in rt_output.iter_static_diff_pieces(limit) + ) == text + assert ''.join( + piece for piece, _ in rt_output.iter_raw_output_pieces(limit) + ) == ''.join(rt_output.combined_raw_output()) + return text c_diff = static_diff('suite/c.sft.cpp') assert '# cpfe -DTEST_NUMBER=1 Test_name.c\n' in c_diff @@ -179,3 +198,149 @@ def static_diff(test: str) -> str: for config, changes_file in changes_files.items() for test in changes_file.read_text().split() ) + + +def _rematerialize_change(tmp_path: Path, + change: dict[str, str]) -> tuple[ + subprocess.CompletedProcess[str], Path]: + '''Run the tool on the diff ``change`` makes to a small test home; return + its result and the run's directory. + ''' + repo = tmp_path / 'repo' + repo.mkdir() + git(repo, 'init', '--quiet', '--initial-branch=main') + write_files(repo, { + 'tests/.edgy/config.json': '{}\n', + 'tests/expectations/edg_x86_64/suite.log': '', + 'tests/expectations/edg_x86_64_cp/suite.log': '', + 'tests/tests/suite/a.sft.cpp': '//type:cp\nint a;\n', + 'tests/tests/suite/.a.rto/default.1.1.txt': ( + 'cpfe -DTEST_NUMBER=1 Test_name.c\nold output\n' + ), + }) + git(repo, 'add', '-A') + git(repo, 'commit', '--quiet', '-m', 'base') + write_files(repo, change) + git(repo, 'add', '-A') + git(repo, 'commit', '--quiet', '-m', 'change') + diff_file = tmp_path / 'pr.diff' + diff_file.write_text(git( + repo, 'diff', '--no-renames', '--no-color', 'HEAD~1', 'HEAD', '--', 'tests' + )) + git(repo, 'checkout', '--quiet', 'HEAD~1') + + runs_dir = tmp_path / 'runs' + timestamp = '2026.01.02-03.04.05' + env = dict(os.environ) + env['EDG_TEST_HOME'] = str(repo / 'tests') + env['PYTHONPATH'] = str(DEV_TOOLS / 'pylibs') + env.pop('RUN_TEST_DEFAULT_CONFIG', None) + result = subprocess.run( + [sys.executable, str(REMATERIALIZE), + '--runs-dir', str(runs_dir), + '--run-timestamp', timestamp, + str(diff_file)], + cwd = repo, + env = env, + capture_output = True, + text = True + ) + return result, runs_dir / timestamp + + +def test_rematerialize_passes_huge_lines_through(tmp_path: Path) -> None: + # Far longer than a line is ever held whole; written (and patched) in + # full. + huge = 'h' * (5 * edgutil.MAX_LINE_CHARS) + result, run_dir = _rematerialize_change(tmp_path, { + 'tests/tests/suite/.a.rto/default.1.1.txt': ( + f"cpfe -DTEST_NUMBER=1 Test_name.c\n{huge}\n" + ), + }) + assert result.returncode == 0, result.stdout + result.stderr + rt_path = (run_dir / 'edg_x86_64' / 'suite/a.sft.cpp').with_suffix( + edgtest.runtest.RUN_TEST_OUTPUT_SUFFIX + ) + with edgtest.runtest.RunTestOutput(rt_path) as rt_output: + static_diff = ''.join(rt_output.combined_static_diff()) + assert f"+{huge}\n" in static_diff + + +def test_rematerialize_refuses_huge_expectation_lines(tmp_path: Path) -> None: + result, _ = _rematerialize_change(tmp_path, { + 'tests/expectations/edg_x86_64/suite.log': ( + 'a.sft.cpp:cp:-DTEST_NUMBER=1:FAIL:' + + 'r' * edgutil.MAX_LINE_CHARS + '\n' + ), + }) + assert result.returncode == 1 + assert ( + 'tests/expectations/edg_x86_64/suite.log: a changed line is longer than' + ) in result.stderr + + +async def test_curr_diff_is_shown_and_compared(tmp_path: Path) -> None: + result, run_dir = _rematerialize_change(tmp_path, { + 'tests/tests/suite/.a.rto/default.1.1.txt': ( + 'cpfe -DTEST_NUMBER=1 Test_name.c\nnew output\n' + ), + # Brings the other configuration into the run (which inherits the + # recording's change). + 'tests/expectations/edg_x86_64_cp/suite.log': ( + 'a.sft.cpp:cp:-DTEST_NUMBER=1:FAIL:\n' + ), + }) + assert result.returncode == 0, result.stdout + result.stderr + context = SandboxContext(tmp_path) + handler = test_provider.TestSourceHandler( + 'Test', + 'test', + context, + context.subpath(tmp_path / 'repo' / 'tests'), + context.subpath(run_dir) + ) + + async def show_curr_diff() -> dict[str, Any]: + channel = FakeChannel(message( + RequestTags.TEST_SHOW_CURR_DIFF, + value = { + 'id': '(edg_x86_64) suite/a.sft.cpp', + 'configs': ['edg_x86_64', 'edg_x86_64_cp'], + 'reviewer_id': 'r', + } + )) + await handler.handle_request(channel) # type: ignore[arg-type] + [reply] = channel.sent + response: dict[str, Any] = reply['v']['client_response'] + return response + + # Observed against the run's (the pull request's) recordings, which it + # was made from: no difference. + response = await show_curr_diff() + assert response['content'] == '' + assert 'divergent_configs' not in response + + # Recordings changed since: the difference is shown, the same for both + # configurations (the other uses the default recording). + recordings = run_dir / 'patched-tests' / 'suite' / '.a.rto' + (recordings / 'default.1.1.txt').write_text( + 'cpfe -DTEST_NUMBER=1 Test_name.c\nnewer output\n' + ) + response = await show_curr_diff() + assert response['content'] == ( + '--- Expected Output\n' + '+++ Observed Output\n' + '# cpfe -DTEST_NUMBER=1 Test_name.c\n' + '@@ -1 +1 @@\n' + '-newer output\n' + '+new output\n' + ) + assert 'divergent_configs' not in response + + # A recording of its own makes the other configuration's differ. + (recordings / 'edg_x86_64_cp.1.1.txt').write_text( + 'cpfe -DTEST_NUMBER=1 Test_name.c\nnew output\n' + ) + response = await show_curr_diff() + assert '-newer output\n+new output' in response['content'] + assert response['divergent_configs'] == ['edg_x86_64_cp'] diff --git a/dev_tools/services/acknowledg-forge/tests/test_safety.py b/dev_tools/services/acknowledg-forge/tests/test_safety.py index 5d27f9f93c..9ade8cc0ca 100644 --- a/dev_tools/services/acknowledg-forge/tests/test_safety.py +++ b/dev_tools/services/acknowledg-forge/tests/test_safety.py @@ -17,6 +17,10 @@ import pytest +import edgutil + +from edgacknowledg.forge import workspace + from edgacknowledg.forge import processor from edgacknowledg.forge.processor import _changes_test_ids, _read_ci_metadata from edgacknowledg.forge.sandbox import Sandbox, SandboxUnavailableError @@ -231,7 +235,7 @@ def test_changes_patterns_cover_recordings_and_expectations() -> None: '-dir/b.sft.cpp: PASS', '+dir/b.sft.cpp: FAIL', ]) - patterns = changes_patterns(diff.splitlines()) + patterns = changes_patterns(io.StringIO(diff)) assert '/tests/tests/s/.a.rto/' in patterns assert '/tests/tests/s/a.*' in patterns assert '/tests/tests/other/dir/b.sft.cpp' in patterns @@ -248,6 +252,63 @@ def test_test_patterns_are_linear_in_the_path() -> None: ] +def test_line_pieces_are_bounded_and_lose_nothing() -> None: + text = ( + 'diff --git a/tests/tests/s/a.sft.cpp b/tests/tests/s/a.sft.cpp\n' + '+' + 'x' * 1000 + '\n\n+last' + ) + pieces = list(edgutil.iter_line_pieces(io.StringIO(text), 16)) + assert ''.join(piece for piece, _ in pieces) == text + assert all(len(piece) <= 16 for piece, _ in pieces) + # Only a line's first piece starts it. + assert [piece for piece, at_line_start in pieces if at_line_start] == [ + 'diff --git a/tes', '+xxxxxxxxxxxxxxx', '\n', '+last' + ] + + +def test_limited_lines_refuse_overlong_lines() -> None: + # Up to the limit (its end not counted), lines come whole. + text = '0123456789\nshort\n' + 'y' * 11 + '\nnever read\n' + lines = edgutil.iter_limited_lines(io.StringIO(text), 10) + assert next(lines) == '0123456789\n' + assert next(lines) == 'short\n' + with pytest.raises(edgutil.LineTooLongError, + match = 'line 3 is longer than 10 characters'): + next(lines) + # A last line without an end is checked too. + def limited(text: str) -> list[str]: + return list(edgutil.iter_limited_lines(io.StringIO(text), 10)) + + assert limited('a\n' + 'b' * 10) == ['a\n', 'b' * 10] + with pytest.raises(edgutil.LineTooLongError): + limited('a\n' + 'b' * 11) + + +def test_changes_patterns_pass_over_huge_lines_in_pieces() -> None: + # A line far over the limit, in an expectations file, names no test (and + # is never held whole); the lines after it still count. + diff = ( + 'diff --git a/tests/expectations/x/s.log b/tests/expectations/x/s.log\n' + '+++ b/tests/expectations/x/s.log\n' + '+big.sft.cpp:' + 'q' * (4 * edgutil.MAX_LINE_CHARS) + '\n' + '+ok.sft.cpp:cp:-DTEST_NUMBER=1:PASS:\n' + ) + handle = io.StringIO(diff) + real_readline = handle.readline + sizes: list[int] = [] + + def readline(size: int = -1) -> str: + piece = real_readline(size) + sizes.append(len(piece)) + return piece + + handle.readline = readline # type: ignore[method-assign] + patterns = changes_patterns(handle) + assert '/tests/tests/s/ok.sft.cpp' in patterns + assert not any('big' in pattern for pattern in patterns) + assert max(sizes) <= edgutil.MAX_LINE_CHARS + 1 + + def test_changes_patterns_skip_suites_no_suite_may_be_named() -> None: # A suite is in the patterns of every test its expectation file names. def expectations(suite: str) -> str: @@ -259,10 +320,10 @@ def expectations(suite: str) -> str: '+a.sft.cpp: PASS', ]) - patterns = changes_patterns(expectations('cwg').splitlines()) + patterns = changes_patterns(io.StringIO(expectations('cwg'))) assert '/tests/tests/cwg/a.sft.cpp' in patterns for suite in ['s' * 65, 'a.b', 'a b']: - assert changes_patterns(expectations(suite).splitlines()) == [ + assert changes_patterns(io.StringIO(expectations(suite))) == [ '/tests/.edgy/', '/tests/expectations/', ] @@ -281,9 +342,9 @@ def test_patterns_are_capped() -> None: *(f"+{n}.sft.cpp: FAIL" for n in range(cap)), ]) with pytest.raises(WorkspaceError, match = 'sparse-checkout patterns'): - changes_patterns(diff.splitlines(), max_patterns = cap) + changes_patterns(io.StringIO(diff), max_patterns = cap) # Within a higher cap. - changes_patterns(diff.splitlines(), max_patterns = 3 * cap) + changes_patterns(io.StringIO(diff), max_patterns = 3 * cap) async def test_git_output_can_be_bounded() -> None: @@ -308,6 +369,39 @@ async def test_git_output_can_be_bounded() -> None: assert process.returncode == -signal.SIGTERM +async def test_git_stdin_is_written_and_stderr_bounded() -> None: + sunk: list[bytes] = [] + # It reads its input (larger than a pipe holds) while writing more + # stderr than is kept. + process = await asyncio.create_subprocess_exec( + 'sh', '-c', 'cat; head -c 1000000 /dev/zero | tr "\\0" e >&2', + stdin = asyncio.subprocess.PIPE, + stdout = asyncio.subprocess.PIPE, + stderr = asyncio.subprocess.PIPE + ) + data = b'pattern\n' * 100_000 + stderr = await _communicate_bounded( + process, + len(data), + sunk.append, + data + ) + assert process.returncode == 0 + assert b''.join(sunk) == data + assert stderr == b'e' * workspace._MAX_GIT_STDERR_BYTES + + +async def test_git_results_are_bounded(tmp_path: Path, + forge: FakeForge, + monkeypatch: pytest.MonkeyPatch + ) -> None: + workspaces = Workspaces(tmp_path / 'home', forge) + assert (await workspaces.git('version')).stdout.startswith('git version') + monkeypatch.setattr(workspace, '_MAX_GIT_OUTPUT_BYTES', 4) + with pytest.raises(WorkspaceError, match = 'wrote over 4 bytes'): + await workspaces.git('version') + + async def test_diffs_are_written_as_they_come(tmp_path: Path, origin: Origin, forge: FakeForge) -> None: @@ -336,10 +430,9 @@ async def test_diffs_are_written_as_they_come(tmp_path: Path, if line.startswith('+') and not line.startswith('+++') ] assert added == lines - # Its patterns, read from the file as from the text. + # Its patterns, read from the file. with open(diff_file, encoding = 'utf-8') as handle: - assert changes_patterns(handle) == changes_patterns(diff.splitlines()) - assert '/tests/tests/s/1999.sft.cpp' in changes_patterns(diff.splitlines()) + assert '/tests/tests/s/1999.sft.cpp' in changes_patterns(handle) # Too long: refused. workspaces = Workspaces(tmp_path / 'home', forge, max_diff_bytes = 1024) @@ -401,6 +494,16 @@ def test_ci_reads_are_capped(tmp_path: Path, _read_ci_metadata(metadata) +def test_ci_test_ids_skip_overlong_lines(tmp_path: Path) -> None: + elist = tmp_path / 'a.elist' + elist.write_text( + 's/a.sft.cpp\n' + + 's/' + 'x' * processor.MAX_TEST_ID_CHARS + '\n' + + 's/b.sft.cpp\n' + ) + assert _changes_test_ids([elist]) == ['s/a.sft.cpp', 's/b.sft.cpp'] + + 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 diff --git a/dev_tools/services/acknowledg-forge/tests/test_symlinks.py b/dev_tools/services/acknowledg-forge/tests/test_symlinks.py index efb70626b4..baea625424 100644 --- a/dev_tools/services/acknowledg-forge/tests/test_symlinks.py +++ b/dev_tools/services/acknowledg-forge/tests/test_symlinks.py @@ -557,7 +557,6 @@ async def test_show_source_reads_only_what_is_shown( monkeypatch: pytest.MonkeyPatch) -> None: home, root = _test_tree(tmp_path, secret) (home / 'tests' / 'suite' / 'big.sft.cpp').write_text('int x;\n' * 1000) - monkeypatch.setattr(test_provider, 'MAX_DISPLAY_CONTENT_SIZE', 64) monkeypatch.setattr(shared, 'MAX_DISPLAY_CONTENT_SIZE', 64) reads: list[int] = [] real_open = shared.ConfinedPath.open @@ -566,12 +565,17 @@ def open_recording_reads(self: shared.ConfinedPath, *args: Any, **kwargs: Any) -> Any: handle = real_open(self, *args, **kwargs) - real_read = handle.read + real_readline = handle.readline + + def readline(size: int = -1) -> Any: + line = real_readline(size) + reads.append(len(line)) + return line def read(size: int = -1) -> Any: - reads.append(size) - return real_read(size) + raise AssertionError('read whole') + handle.readline = readline handle.read = read return handle @@ -594,7 +598,8 @@ def read(size: int = -1) -> Any: response = reply['v']['client_response'] assert response['truncated'] and len(response['content']) <= 64 assert response['content'].startswith('int x;\n') - assert reads == [65] + # A line at a time, and no more than one past what fits. + assert reads == [7] * 10 async def test_sandbox_context_commands(tmp_path: Path) -> None: diff --git a/dev_tools/services/acknowledg/PROTOCOL.md b/dev_tools/services/acknowledg/PROTOCOL.md index 07188c5904..a9c700388e 100644 --- a/dev_tools/services/acknowledg/PROTOCOL.md +++ b/dev_tools/services/acknowledg/PROTOCOL.md @@ -3,7 +3,7 @@ This document describes the WebSocket protocol used by the AcknowlEDG server, the Vue client, and the `edgy-review` / `edg-bench-review` / `edg-acknowledg-cli` providers. -The current protocol revision is **19**. +The current protocol revision is **20**. All frames are JSON text. Implementations live in `edgacknowledg` (`dev_tools/pylibs/edgacknowledg`) and must stay in sync with the JavaScript @@ -74,15 +74,15 @@ work can flow immediately. Providers may optionally send identity updates at any time (on dedicated sockets or nested in `batch.event`): ```json -{ "t": "meta.section.name", "r": 19, "v": "" } +{ "t": "meta.section.name", "r": 20, "v": "" } ``` ```json -{ "t": "meta.review.name", "r": 19, "v": [ ] } +{ "t": "meta.review.name", "r": 20, "v": [ ] } ``` ```json -{ "t": "meta.review.timestamp", "r": 19, "v": "" } +{ "t": "meta.review.timestamp", "r": 20, "v": "" } ``` - `meta.section.name` sets the display name for the section tab (shown in @@ -329,7 +329,7 @@ content that isn't the provider user's own, one reached through a symlink), the provider doesn't answer the request. It reports the reviewer instead: ```json -{ "t": "illegal-access", "r": 19, "v": { "reviewer_id": "" } } +{ "t": "illegal-access", "r": 20, "v": { "reviewer_id": "" } } ``` The server then closes that reviewer's socket with code **3001**, without @@ -345,7 +345,7 @@ access), it doesn't answer the request either. It tells the reviewer it failed, through the server, and keeps serving the section: ```json -{ "t": "invalid", "r": 19, "v": { "reviewer_id": "" } } +{ "t": "invalid", "r": 20, "v": { "reviewer_id": "" } } ``` The server sends `invalid` to that reviewer alone, with `v` `null` and `s` @@ -362,6 +362,19 @@ missing) and may set `truncated: true` when the body exceeds 8 MiB (`MAX_DISPLAY_CONTENT_SIZE`). Protocol messages themselves are capped at 10 MiB (`MAX_MESSAGE_SIZE`). +Each line of `content` is at most 16 Ki characters +(`MAX_DISPLAY_LINE_SIZE`). A provider cuts a longer line to that length and +lists its 1-based line number in `lines_truncated` (sorted ascending, and +omitted when no line was cut). Nothing in `content` itself marks a cut. The +reviewer UI appends `…` (U+2026) to each listed line and shows a "Some Lines +Truncated" notice. Line cuts happen before the 8 MiB cap, so `truncated` and +`lines_truncated` may both be set; `lines_truncated` lists only lines +present in `content`. + +Single-line labels (the `qualifier`, `variant`, and `status` of +`test.src.list-statuses` lines, and its `remark`) are cut to 1 Ki characters +(`MAX_DISPLAY_LABEL_SIZE`), ending in `…`. + **`test.src.list-statuses`** ```json @@ -392,7 +405,12 @@ missing) and may set `truncated: true` when the body exceeds 8 MiB **`test.src.show-diff`**, **`test.src.show-output`** ```json -{ "id": "", "content": "", "truncated": false } +{ + "id": "", + "content": "", + "truncated": false, + "lines_truncated": [1] +} ``` **`test.src.show-diff-curr`** — same as above, plus optional @@ -412,7 +430,8 @@ match the representative. "id": "", "assoc_file": "", "content": "", - "truncated": false + "truncated": false, + "lines_truncated": [1] } ``` @@ -434,7 +453,12 @@ match the representative. **`bench.src.show-annotate`** ```json -{ "id": "", "content": "", "truncated": false } +{ + "id": "", + "content": "", + "truncated": false, + "lines_truncated": [1] +} ``` ### Editor `v` as seen by the reviewer diff --git a/dev_tools/services/acknowledg/client/src/BenchReviewView.vue b/dev_tools/services/acknowledg/client/src/BenchReviewView.vue index 63484d1cd0..e469749601 100644 --- a/dev_tools/services/acknowledg/client/src/BenchReviewView.vue +++ b/dev_tools/services/acknowledg/client/src/BenchReviewView.vue @@ -22,6 +22,7 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception } from './bench-metrics.js' import { formatBenchAnchor, parseBenchAnchor } from './review-anchor.js' import { protocolMessage } from './sockets.js' + import { displayLines } from './display-content.js' import AppButton from './AppButton.vue' import AppSelect from './AppSelect.vue' import BenchStatus from './BenchStatus.vue' @@ -47,8 +48,14 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception ] const selectedBenchmarkId = ref(null) const displayTruncated = ref(false) + // 1-based numbers of the displayed lines the provider cut. + const displayedLinesTruncated = ref([]) const displayedOutput = ref(null) + const displayedOutputLines = computed(() => { + return displayLines(displayedOutput.value, displayedLinesTruncated.value) + }) + provide('activeBenchmarkId', selectedBenchmarkId) provide('benchSortMode', sortMode) @@ -177,7 +184,8 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception const sorted = benchmarks.value.slice() sorted.sort((left, right) => { const leftChange = computePercentChange(left.baseline_ops, left.run_ops) - const rightChange = computePercentChange(right.baseline_ops, right.run_ops) + const rightChange = computePercentChange(right.baseline_ops, + right.run_ops) if (sortMode.value === 'greatest-speedup') { return rightChange - leftChange } @@ -212,12 +220,14 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception } else if (message.t === 'bench.src.show-annotate') { if (message.v.id === selectedBenchmarkId.value) { displayTruncated.value = false + displayedLinesTruncated.value = [] if (message.v.content === null) { displayedOutput.value = 'Annotation unavailable!' } else { if (message.v.truncated) { displayTruncated.value = true } + displayedLinesTruncated.value = message.v.lines_truncated ?? [] displayedOutput.value = message.v.content } } @@ -353,7 +363,8 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
Average Change (OPs)
{{ formatSignedOps(Math.round(mean(sampleOpDeltas))) }} - σ {{ Math.round(populationStdev(sampleOpDeltas)).toLocaleString() }} + σ {{ Math.round(populationStdev(sampleOpDeltas)) + .toLocaleString() }}
@@ -404,15 +415,21 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception class="truncation-warning"> Content Too Long: Display Truncated
+
+ Some Lines Truncated +
- 
{{ displayedOutput }}
+ 
{{ displayedOutputLines.join('\n') }}
-
+
Select a benchmark to continue...
diff --git a/dev_tools/services/acknowledg/client/src/TestReviewView.vue b/dev_tools/services/acknowledg/client/src/TestReviewView.vue index ccc35b99a9..82d6eaea56 100644 --- a/dev_tools/services/acknowledg/client/src/TestReviewView.vue +++ b/dev_tools/services/acknowledg/client/src/TestReviewView.vue @@ -19,6 +19,7 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception import { protocolMessage } from './sockets.js' import { computeGradientStyleForConfigs } from './config-color.js' import { formatTestAnchor, parseTestAnchor } from './review-anchor.js' + import { displayLines } from './display-content.js' import DiffLine from './DiffLine.vue' import LoadingSpinner from './LoadingSpinner.vue' @@ -44,11 +45,17 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception const displayedTestAssocFiles = ref(null) const displayedTestAssocFileName = ref(null) const displayTruncated = ref(false) + // 1-based numbers of the displayed lines the provider cut. + const displayedLinesTruncated = ref([]) const currDiffDivergenceNotice = ref(null) const displayedOutput = ref("") const outputViewModeTarget = ref("test.src.show-diff") const isMultiConfig = ref(false) + const displayedOutputLines = computed(() => { + return displayLines(displayedOutput.value, displayedLinesTruncated.value) + }) + const displayedTestConfig = computed(() => { return displayedGroupConfigs.value[0] ?? "" }) @@ -279,10 +286,12 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception isMultiConfig.value = message.v.configs.length > 1 testStatuses.value = message.v.statuses applyHashSelection() - } else if (message.t === "test.src.show-diff" || message.t === "test.src.show-diff-curr" || + } else if (message.t === "test.src.show-diff" || + message.t === "test.src.show-diff-curr" || message.t === "test.src.show-output") { if (message.v.id === configuredIdForDisplayedTest()) { displayTruncated.value = false + displayedLinesTruncated.value = [] currDiffDivergenceNotice.value = null if (message.v.content === null) { displayedOutput.value = "Missing .rt.tar file!" @@ -290,6 +299,7 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception if (message.v.truncated) { displayTruncated.value = true } + displayedLinesTruncated.value = message.v.lines_truncated ?? [] displayedOutput.value = message.v.content } if (message.t === "test.src.show-diff-curr" && @@ -307,6 +317,7 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception } else if (message.t === "test.src.show-src") { if (message.v.id === displayedTestName.value) { displayTruncated.value = false + displayedLinesTruncated.value = [] currDiffDivergenceNotice.value = null if (message.v.content === null) { displayedOutput.value = "File not found!" @@ -314,6 +325,7 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception if (message.v.truncated) { displayTruncated.value = true } + displayedLinesTruncated.value = message.v.lines_truncated ?? [] displayedOutput.value = message.v.content } } @@ -462,7 +474,8 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-
+
@@ -487,6 +500,11 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception class="content-notice"> Content Too Long: Display Truncated
+
+ Some Lines Truncated +
{{ currDiffDivergenceNotice }} @@ -499,21 +517,23 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception style="overflow: visible; display: grid;">
-
+
Select a test status difference to continue...
-
+
{ + if (content === null || content === undefined) { + return [] + } + const lines = content.split('\n') + for (const lineNumber of linesTruncated) { + lines[lineNumber - 1] += TRUNCATION_MARK + } + return lines +} diff --git a/dev_tools/services/acknowledg/client/src/sockets.js b/dev_tools/services/acknowledg/client/src/sockets.js index cd57a09e9a..3a5b2f98ac 100644 --- a/dev_tools/services/acknowledg/client/src/sockets.js +++ b/dev_tools/services/acknowledg/client/src/sockets.js @@ -5,7 +5,7 @@ See https://edgcpp.org/LICENSE.txt for license information. SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception */ -const PROTOCOL_REVISION = 19 +const PROTOCOL_REVISION = 20 const getSocketHost = () => { if (process.env.NODE_ENV === "development") { @@ -26,7 +26,9 @@ export const acquireSocketForReview = (reviewId) => { ) } -export const protocolMessage = (tag, value = undefined, section = undefined) => { +export const protocolMessage = (tag, + value = undefined, + section = undefined) => { const message = { t: tag, r: PROTOCOL_REVISION } if (value !== undefined) { message.v = value