Fix the npm byte-budget race; add a local dependency reproduction harness

review/src/oc_review/collect.py

@@ -90,11 +90,15 @@
 REGISTRY_LOOKUP_BUDGET_SECONDS = 240.0
 # Unique keys looked up per run; past the cap a key fails without a lookup.
 MAX_REGISTRY_LOOKUPS = 3000
-# Decoded response bytes for all lookups of a run (K-18, K-19). Each fetch reserves its cap
-# (or what is left) before it starts, so reserved plus received bytes never
-# pass this. It bounds download volume; memory per worker is bounded by the
-# per-response cap (a parsed packument is about 6.4x its body).
-MAX_REGISTRY_BYTES = 1024 * 1024 * 1024
+# Decoded response bytes for all lookups of a run (K-18, K-19, K-20). Only
+# bytes received count: a fetch starts only while fewer than this have
+# arrived, so a run can pass it by at most the fetches in flight, each at most
+# its response cap (8 x 128 MiB). It is a backstop on download volume: memory
+# is bounded per response by the cap (a parsed packument is about 6.4x its
+# body) and time by the lookup time budget, which binds first at the observed
+# rate (about 64 MB/s decoded, so about 15 GB in 240 s). 8 GiB is about 14x
+# the largest measured legitimate run (front-end PR 2027, 578 MB decoded).
+MAX_REGISTRY_BYTES = 8 * 1024 * 1024 * 1024
 # Fixed failure reasons ("incomplete dependency data"), in the order a file
 # with several picks its one reason.
 LOOKUP_BUDGET_EXCEEDED = "registry lookup failed: lookup time budget exceeded"
@@ -682,7 +686,10 @@ def lookup_registry(
     the deadline fails. Any error in a lookup fails that key only, with a
     reason that tells a response over its size cap, and a run past its
     `byte_budget` download budget, from other failures (K-18); each failure
-    is logged with its exception class.
+    is logged with its exception class. Only bytes received count against
+    `byte_budget`, including the part of a body a failed fetch read (K-21):
+    no fetch starts once they reach it, and the fetches in flight may pass it
+    by at most `concurrency` response caps (K-20).
 
     Collect does not wait for a lookup past the deadline. The workers are
     daemon threads, so a hung lookup also does not hold the process open
@@ -700,41 +707,40 @@ def lookup_registry(
     results: dict[RegistryKey, deps.RegistryInfo | str] = {}
     finished = 0  # groups whose worker is done with them
     closed = False  # set at the deadline or when all groups finish
-    reserved = 0  # response bytes received or reserved by fetches in flight
+    received = 0  # decoded response bytes of finished fetches
 
     def remaining() -> float:
         return deadline - clock()
 
     def bounded_fetch(url: str, max_bytes: int, timeout: float) -> deps.FetchResult:
-        nonlocal reserved
+        nonlocal received
         left = remaining()
         if left <= 0:
             raise _BudgetSpent("lookup time budget exceeded")
+        # No reservation (K-20): a fetch starts with its full cap while fewer
+        # than `byte_budget` bytes have arrived, so a refusal means the
+        # budget is really spent, never that fetches in flight hold it.
         with state:
-            room = byte_budget - reserved
-            if room <= 0:
+            if received >= byte_budget:
                 raise _BytesSpent("download byte budget exceeded")
-            grant = min(max_bytes, room)
-            reserved += grant
-        # Bytes that count against the budget. A fetch that fails before a
-        # body arrives (timeout, network error, 5xx) downloaded nothing that
-        # counts, so it keeps none of its reservation. A response that was
-        # read past its cap, or a body that did not decode, was a real
-        # download: it keeps the whole reservation.
+        # Bytes that count against the budget, once per attempt. A fetch that
+        # fails before a body arrives (5xx, refused connection) counts 0; one
+        # that fails after part of a body (reset, timeout) counts what it
+        # read (K-21). A response that was read past its cap, or a body that
+        # did not decode, was a real download: it counts as its cap.
         got = 0
         try:
-            result = fetch(url, grant, min(timeout, left))
+            result = fetch(url, max_bytes, min(timeout, left))
             got = len(result.body)
-        except (deps.ResponseTooLargeError, deps.ResponseEncodingError) as exc:
-            got = grant
-            if grant < max_bytes and isinstance(exc, deps.ResponseTooLargeError):
-                raise _BytesSpent("download byte budget exceeded") from exc
+        except (deps.ResponseTooLargeError, deps.ResponseEncodingError):
+            got = max_bytes
+            raise
+        except Exception as exc:
+            got = deps.received_bytes(exc)
             raise
         finally:
             with state:
-                reserved -= grant - got
-        if grant < max_bytes and got > grant:
-            raise _BytesSpent("download byte budget exceeded")
+                received += got
         return result
 
     def bounded_sleep(seconds: float) -> None:

review/src/oc_review/deps.py

@@ -57,6 +57,9 @@
 # checks alike: the four install scripts, plus the three npm also runs on
 # install.
 NPM_LIFECYCLE_SCRIPTS = ("preinstall", "install", "postinstall", "prepare", "preprepare", "postprepare", "prepublish")
+# A lookup keeps at most this many characters of each registry lifecycle
+# script, so the results of a run stay small whatever the byte budget (K-21).
+NPM_SCRIPT_TEXT_CHARS = 1000
 
 FAMILIES = (
     "package.json",
@@ -1481,10 +1484,27 @@ class FetchResult:
 
 
 # fetch(url, max_bytes, timeout_seconds) -> FetchResult. It raises OSError,
-# TimeoutError or http.client.HTTPException on a network failure.
+# TimeoutError or http.client.HTTPException on a network failure. A failure
+# after part of the body arrived sets `received_bytes` on the exception to the
+# decoded bytes read (K-21).
 Fetcher = Callable[[str, int, float], FetchResult]
 
 
+def received_bytes(exc: BaseException) -> int:
+    """Decoded body bytes a fetch read before it raised `exc`; 0 when the
+    fetcher did not set them."""
+    value = getattr(exc, "received_bytes", 0)
+    return value if isinstance(value, int) and value > 0 else 0
+
+
+def _with_received(exc: BaseException, size: int) -> BaseException:
+    try:
+        exc.received_bytes = size  # type: ignore[attr-defined]
+    except AttributeError:
+        pass
+    return exc
+
+
 class _NoRedirect(urllib.request.HTTPRedirectHandler):
     def redirect_request(self, *args: Any, **kwargs: Any) -> None:  # noqa: D401
         return None
@@ -1511,6 +1531,7 @@ def default_fetch(
         headers={"Accept": "application/json", "Accept-Encoding": "gzip", "User-Agent": "oc-claude-review"},
     )
     deadline = clock() + timeout
+    size = 0  # decoded bytes; a failure reports them as `received_bytes`
     try:
         with opener.open(request, timeout=timeout) as resp:
             encoding = (resp.headers.get("Content-Encoding") or "identity").strip().lower()
@@ -1519,7 +1540,6 @@ def default_fetch(
             # 31 = gzip wrapper only (no zlib or raw deflate).
             inflater = zlib.decompressobj(31) if encoding != "identity" else None
             chunks: list[bytes] = []
-            size = 0  # decoded bytes
             wire = 0  # encoded bytes
             while size <= max_bytes:
                 if clock() > deadline:
@@ -1554,7 +1574,9 @@ def default_fetch(
     except urllib.error.HTTPError as exc:
         return FetchResult(status=exc.code, body=b"", url=url)
     except http.client.HTTPException as exc:
-        raise OSError(f"{url}: {type(exc).__name__}: {exc}") from exc
+        raise _with_received(OSError(f"{url}: {type(exc).__name__}: {exc}"), size) from exc
+    except Exception as exc:
+        raise _with_received(exc, size)
 
 
 @dataclass(frozen=True)
@@ -1653,9 +1675,13 @@ def lookup_npm(
     found = {}
     for key in NPM_LIFECYCLE_SCRIPTS:
         if key in scripts:
-            if not isinstance(scripts[key], str):
+            text = scripts[key]
+            if not isinstance(text, str):
                 raise RegistryError(f"{url}: script {key} is not a string")
-            found[key] = scripts[key]
+            if len(text) > NPM_SCRIPT_TEXT_CHARS:
+                cut = len(text) - NPM_SCRIPT_TEXT_CHARS
+                text = f"{text[:NPM_SCRIPT_TEXT_CHARS]} ... [{cut} more characters cut]"
+            found[key] = text
     return RegistryInfo(
         ecosystem="npm",
         name=name,