Make the forge's input limits settings, raising the artifact ones []

dev_tools/pylibs/edgacknowledg/forge/config.py

@@ -20,6 +20,27 @@
 
 ENV_PREFIX = 'ACKNOWLEDG_FORGE_'
 
+# The defaults of the limits on what a pull request and its CI artifacts
+# may make the service do (``workspace``), which the settings may raise for
+# large ones, given the memory (and time) they take.
+#
+# The most sparse-checkout patterns a checkout may have: matching costs
+# patterns x paths, and they are built from pull request content. Their
+# total size must also stay within a small multiple of the diff's (see
+# DEFAULT_MAX_DIFF_BYTES): a test gets at most two patterns, each about as
+# long as its path, and the only text that is part of many patterns, a
+# suite's name, is short.
+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
+# 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
+# results reach about 170,000 entries, of about 106 bytes each there.
+DEFAULT_MAX_ARTIFACT_ENTRIES = 500_000
+DEFAULT_MAX_ARTIFACT_DIRECTORY_BYTES = 64 * 1024 * 1024
+
 
 class ConfigError(Exception):
   '''Raised for a missing or malformed setting.'''
@@ -102,6 +123,10 @@ class ForgeServiceConfig:
   bench_artifact: str
   max_artifact_bytes: int
   max_extracted_bytes: int
+  max_artifact_entries: int
+  max_artifact_directory_bytes: int
+  max_diff_bytes: int
+  max_sparse_patterns: int
   max_concurrency: int
   rematerialize_timeout: float
   unsandboxed: bool
@@ -240,6 +265,19 @@ def get_list(name: str) -> tuple[str, ...]:
       max_extracted_bytes = int(
         get_number('MAX_EXTRACTED_BYTES', 2 * 1024 * 1024 * 1024)
       ),
+      max_artifact_entries = int(
+        get_number('MAX_ARTIFACT_ENTRIES', DEFAULT_MAX_ARTIFACT_ENTRIES)
+      ),
+      max_artifact_directory_bytes = int(get_number(
+        'MAX_ARTIFACT_DIRECTORY_BYTES',
+        DEFAULT_MAX_ARTIFACT_DIRECTORY_BYTES
+      )),
+      max_diff_bytes = int(
+        get_number('MAX_DIFF_BYTES', DEFAULT_MAX_DIFF_BYTES)
+      ),
+      max_sparse_patterns = int(
+        get_number('MAX_SPARSE_PATTERNS', DEFAULT_MAX_PATTERNS)
+      ),
       max_concurrency = max_concurrency,
       rematerialize_timeout = get_number('REMATERIALIZE_TIMEOUT', 15 * 60),
       unsandboxed = unsandboxed,

dev_tools/pylibs/edgacknowledg/forge/processor.py

@@ -411,7 +411,10 @@ async def _changes_section(self,
       record.review_id,
       'base',
       merge_base,
-      changes_patterns(diff)
+      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]}"
@@ -509,7 +512,9 @@ async def _stage_artifact(self,
           extract_artifact,
           zip_path,
           staging,
-          max_bytes = self._config.max_extracted_bytes
+          max_bytes = self._config.max_extracted_bytes,
+          max_entries = self._config.max_artifact_entries,
+          max_directory_bytes = self._config.max_artifact_directory_bytes
         )
       finally:
         zip_path.unlink(missing_ok = True)
@@ -649,7 +654,10 @@ async def _build_ci_sections(self,
         record.review_id,
         'head',
         tested_sha,
-        failures_patterns(failed_tests)
+        failures_patterns(
+          failed_tests,
+          max_patterns = self._config.max_sparse_patterns
+        )
       )
 
       sections: list[SectionRecord] = []

dev_tools/pylibs/edgacknowledg/forge/service.py

@@ -104,7 +104,11 @@ async def _run_components(config: ForgeServiceConfig,
   try:
     wake = asyncio.Event()
     ingestor = Ingestor(store, forge, config, wake = wake)
-    workspaces = Workspaces(config.home, forge)
+    workspaces = Workspaces(
+      config.home,
+      forge,
+      max_diff_bytes = config.max_diff_bytes
+    )
     # Serving reviewers happens in the publisher container.
     publisher = PublisherClient(
       url = publisher_service_url,

dev_tools/pylibs/edgacknowledg/forge/workspace.py

@@ -45,6 +45,13 @@
 import edgutil
 
 from edgacknowledg.forge import Forge, PullRequestInfo
+from edgacknowledg.forge.config import (
+  DEFAULT_MAX_ARTIFACT_DIRECTORY_BYTES,
+  DEFAULT_MAX_ARTIFACT_ENTRIES,
+  DEFAULT_MAX_DIFF_BYTES,
+  DEFAULT_MAX_PATTERNS,
+  ENV_PREFIX,
+)
 from edgacknowledg.forge.runner.roles import BUILD_STAGING_DIR, REVIEWS_DIR
 from edgtest.runtest import get_bare_name
 
@@ -61,21 +68,6 @@
 
 BENCH_BASELINE_PATTERNS = ('/benchmarks/baselines/',)
 
-# The most sparse-checkout patterns a checkout may have: matching costs
-# patterns x paths, and they are built from pull request content. Their
-# total size must also stay within a small multiple of the diff's (see
-# MAX_DIFF_BYTES): a test gets at most two patterns, each about as long as
-# its path, and the only text that is part of many patterns, a suite's
-# name, is short.
-MAX_PATTERNS = 20_000
-# The largest diff (of tests/, in bytes) a pull request may have: it is
-# held in memory, and copied and split into lines more than once.
-MAX_DIFF_BYTES = 16 * 1024 * 1024
-# The most entries an artifact may have (its zip's, and its tarballs'
-# together), and the largest its zip's central directory may be: zipfile
-# reads all of that directory into memory, before we can count.
-MAX_ARTIFACT_ENTRIES = 100_000
-MAX_CENTRAL_DIRECTORY_BYTES = 16 * 1024 * 1024
 # Seconds a git process being stopped has to clean up (e.g. remove its lock
 # files) before it is killed; well within docker stop's default 10.
 _TERMINATE_ALLOWANCE = 5.0
@@ -106,7 +98,7 @@ def patterns_for_test(test_path: PurePosixPath) -> list[str]:
   as 'tests/tests/suite/dir/name.sft.cpp'): the test file and its recordings
   (where the test runner keeps them, '.<bare name>.rto'), or the whole
   directory of a multi-file test. At most two, each about as long as the
-  path, however it is named (see MAX_PATTERNS).
+  path, however it is named (see config.DEFAULT_MAX_PATTERNS).
   '''
   directory = _escape_pattern(str(test_path.parent))
   if '.mft.' in test_path.name:
@@ -148,9 +140,12 @@ def changed_paths(diff: str) -> list[str]:
   return paths
 
 
-def changes_patterns(diff: str) -> list[str]:
+def changes_patterns(diff: 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.
+  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
@@ -160,12 +155,16 @@ def changes_patterns(diff: str) -> list[str]:
       path = PurePosixPath(match.group(1))
       current_suite = None
       if path.parts[:2] == ('tests', 'tests'):
-        _add_patterns(patterns, _patterns_for_changed_test_file(path))
+        _add_patterns(
+          patterns,
+          _patterns_for_changed_test_file(path),
+          max_patterns
+        )
       elif (path.parts[:2] == ('tests', 'expectations') and
             path.suffix == '.log'):
         # Expectation lines name tests relative to the suite, so its name is
-        # part of the patterns of each (see MAX_PATTERNS). A file whose name
-        # no suite may have (e.g. too long) names no tests.
+        # part of the patterns of each (see config.DEFAULT_MAX_PATTERNS). A
+        # file whose name no suite may have (e.g. too long) names no tests.
         if edgtest.is_valid_suite_name(path.stem):
           current_suite = path.stem
       continue
@@ -174,33 +173,43 @@ def changes_patterns(diff: str) -> list[str]:
     if line.startswith(('-', '+')):
       test_name = line[1:].split(':')[0].strip()
       if test_name:
-        _add_patterns(patterns, patterns_for_test(
-          PurePosixPath('tests', 'tests', current_suite, test_name)
-        ))
+        _add_patterns(
+          patterns,
+          patterns_for_test(
+            PurePosixPath('tests', 'tests', current_suite, test_name)
+          ),
+          max_patterns
+        )
   return list(patterns)
 
 
-def failures_patterns(test_ids: Iterable[str]) -> list[str]:
+def failures_patterns(test_ids: Iterable[str],
+                      *,
+                      max_patterns: int = DEFAULT_MAX_PATTERNS) -> list[str]:
   '''Sparse-checkout patterns for serving CI results naming ``test_ids``
-  ('<suite>/<path>', as in .elist files), plus bench baselines.
+  ('<suite>/<path>', as in .elist files), plus bench baselines; at most
+  ``max_patterns``.
   '''
   patterns = dict.fromkeys([*_TEST_HOME_PATTERNS, *BENCH_BASELINE_PATTERNS])
   for test_id in test_ids:
     test_path = PurePosixPath('tests', 'tests', test_id)
     if '..' in test_path.parts or test_path.is_absolute():
       continue
-    _add_patterns(patterns, patterns_for_test(test_path))
+    _add_patterns(patterns, patterns_for_test(test_path), max_patterns)
   return list(patterns)
 
 
-def _add_patterns(patterns: dict[str, None], more: list[str]) -> None:
+def _add_patterns(patterns: dict[str, None],
+                  more: list[str],
+                  max_patterns: int) -> None:
   '''Add ``more`` to ``patterns`` (an ordered set), within
-  ``MAX_PATTERNS``.
+  ``max_patterns``.
   '''
   patterns.update(dict.fromkeys(more))
-  if len(patterns) > MAX_PATTERNS:
+  if len(patterns) > max_patterns:
     raise WorkspaceError(
-      f"more than {MAX_PATTERNS} sparse-checkout patterns (too many tests)"
+      f"more than {max_patterns} sparse-checkout patterns (too many tests; "
+      f"{ENV_PREFIX}MAX_SPARSE_PATTERNS)"
     )
 
 
@@ -293,9 +302,10 @@ def remove_tree(path: Path) -> None:
 
 class _Budget:
   '''What extracting an artifact may still spend: bytes, and entries.'''
-  def __init__(self, max_bytes: int) -> None:
+  def __init__(self, max_bytes: int, max_entries: int) -> None:
     self.bytes = max_bytes
-    self.entries = MAX_ARTIFACT_ENTRIES
+    self.max_entries = max_entries
+    self.entries = max_entries
 
   def spend(self, count: int) -> None:
     self.bytes -= count
@@ -306,7 +316,8 @@ def count_entry(self) -> None:
     self.entries -= 1
     if self.entries < 0:
       raise UnsafeArchiveError(
-        f"artifact has more than {MAX_ARTIFACT_ENTRIES} entries"
+        f"artifact has more than {self.max_entries} entries "
+        f"({ENV_PREFIX}MAX_ARTIFACT_ENTRIES)"
       )
 
 
@@ -353,24 +364,24 @@ def _check_inner_tarball(path: Path, budget: _Budget) -> None:
     raise UnsafeArchiveError(f"{path.name} is not a valid tarball") from ex
 
 
-def extract_artifact(zip_path: Path,
-                     destination: Path,
-                     *,
-                     max_bytes: int) -> None:
+def extract_artifact(
+      zip_path: Path, destination: Path, *, max_bytes: int,
+      max_entries: int = DEFAULT_MAX_ARTIFACT_ENTRIES,
+      max_directory_bytes: int = DEFAULT_MAX_ARTIFACT_DIRECTORY_BYTES) -> None:
   '''Safely extract a CI artifact zip into the (new, empty) ``destination``.
 
-  Rejects absolute or escaping paths and symlinks, and stops once more than
-  ``max_bytes`` would be written, or ``MAX_ARTIFACT_ENTRIES`` entries made
-  (counting inner tarballs' contents too).
+  Rejects absolute or escaping paths and symlinks, and a zip whose central
+  directory is over ``max_directory_bytes``, and stops once more than
+  ``max_bytes`` would be written, or ``max_entries`` entries made (counting
+  inner tarballs' contents too).
   '''
   directory_size = _central_directory_size(zip_path)
-  if (directory_size is not None and
-      directory_size > MAX_CENTRAL_DIRECTORY_BYTES):
+  if directory_size is not None and directory_size > max_directory_bytes:
     raise UnsafeArchiveError(
-      f"artifact's central directory is over "
-      f"{MAX_CENTRAL_DIRECTORY_BYTES} bytes"
+      f"artifact's central directory is over {max_directory_bytes} bytes "
+      f"({ENV_PREFIX}MAX_ARTIFACT_DIRECTORY_BYTES)"
     )
-  budget = _Budget(max_bytes)
+  budget = _Budget(max_bytes, max_entries)
   tarballs = []
   try:
     with zipfile.ZipFile(zip_path) as archive:
@@ -406,11 +417,18 @@ def extract_artifact(zip_path: Path,
 
 
 class Workspaces:
-  '''Clones and worktrees under ``home`` for one forge.'''
+  '''Clones and worktrees under ``home`` for one forge; a diff (``diff``)
+  may be at most ``max_diff_bytes``.
+  '''
 
-  def __init__(self, home: Path, forge: Forge) -> None:
+  def __init__(self,
+               home: Path,
+               forge: Forge,
+               *,
+               max_diff_bytes: int = DEFAULT_MAX_DIFF_BYTES) -> None:
     self._home = home
     self._forge = forge
+    self._max_diff_bytes = max_diff_bytes
     self._repos_dir = home / 'repos' / forge.name
     self._reviews_dir = home / REVIEWS_DIR
     self._locks: dict[str, asyncio.Lock] = {}
@@ -597,7 +615,7 @@ async def diff(self,
         'diff', '--no-renames', '--no-color', '--no-ext-diff', '--no-textconv',
         base, head, '--', *paths,
         git_dir = self.clone_dir(repository),
-        max_output = MAX_DIFF_BYTES
+        max_output = self._max_diff_bytes
       )
       return result.stdout
 

dev_tools/services/acknowledg-forge/README.md

@@ -112,6 +112,10 @@ from the environment variable shown as a fallback.
 | `ACKNOWLEDG_FORGE_SETTLE_TIMEOUT` | `7200` | Seconds after a push to stop waiting for CI before commenting |
 | `ACKNOWLEDG_FORGE_MAX_ARTIFACT_BYTES` | 256 MiB | Largest artifact download |
 | `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_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 |
 | `ACKNOWLEDG_FORGE_UNSANDBOXED` | `0` | `1` runs without containers: the service serves the runner servers and the publisher server itself, on `127.0.0.1`, where any local process can reach them. **Local development on a single-user machine only.** |

dev_tools/services/acknowledg-forge/SANDBOXING.md

@@ -526,6 +526,9 @@ them. Several layers keep the service from reading through one:
 | Resource | Limit |
 | --- | --- |
 | 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 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 |
 | Each container's `/tmp` | 512 MiB tmpfs; copies made for diffing are streamed there |

dev_tools/services/acknowledg-forge/tests/conftest.py

@@ -301,7 +301,11 @@ def __init__(self,
       builder.start()
       _builder_clients.append(builder)
     self.sandbox = Sandbox(builder = builder)
-    self.workspaces = Workspaces(config.home, forge)
+    self.workspaces = Workspaces(
+      config.home,
+      forge,
+      max_diff_bytes = config.max_diff_bytes
+    )
     self.ingestor = Ingestor(
       store,
       forge,