Skip to content

fix(build-context): bound the per-file read into file_cache - #377

Open
wernerkasselman-au wants to merge 1 commit into
NVIDIA:mainfrom
wernerkasselman-au:fix/bound-file-cache-read
Open

fix(build-context): bound the per-file read into file_cache#377
wernerkasselman-au wants to merge 1 commit into
NVIDIA:mainfrom
wernerkasselman-au:fix/bound-file-cache-read

Conversation

@wernerkasselman-au

Copy link
Copy Markdown
Contributor

What this fixes

_read_file_cache() calls _read_text_no_follow() on every discovered
component, and that does an unbounded read:

def _read_text_no_follow(path: Path) -> str:
    """Read a regular file without following symlinks at open time."""
    with _open_regular_file_no_follow(path) as source:
        return source.read().decode("utf-8", errors="replace")

So a local directory scan materializes each file whole before any analyzer sees
it. A multi-GB file in a skill drives peak memory to its full size and is only
then skipped downstream at MAX_FILE_CHARS.

INGEST_MAX_BYTES does not cover this. Its docstring scopes it to "Each
remote/archive ingest path". A local directory target reaches build_context
via validate_local_input_path(), which does no sizing. MAX_FILE_BYTES is
not a gate here either
: in build_context it appears only inside
_is_valid_oms_signature() and in a size_bytes metadata field.

The change

A size check before the read, reusing the stat() already taken for the
S_ISREG test so it costs no extra syscall. It emits LedgerOutcome.SKIPPED
with LedgerReason.SIZE_LIMIT rather than raising, matching what the static,
AST, taint, and YARA analyzers already do for oversized input. The file is
reported as not-inspected and flows into analysis_completeness normally,
instead of silently vanishing from the cache.

The bound is derived, not picked

MAX_CACHE_READ_BYTES = MAX_PYTHON_AST_CACHE_SOURCE_CHARS * 4

Every downstream consumer bounds itself in characters: analyzers skip at
static_runner.MAX_FILE_CHARS, and the prewarmed AST cache rejects a single
source above MAX_PYTHON_AST_CACHE_SOURCE_CHARS, the larger of the two. UTF-8
uses at most 4 bytes per character, and errors="replace" yields one character
per undecodable byte, so nothing above 4x that character budget can decode to
a size any consumer would accept.

The gate is therefore outcome-preserving by construction. It cannot exclude
content that would otherwise have been analyzed; it only declines to
materialize bytes already guaranteed to be skipped.
test_cache_read_bound_cannot_exclude_content_any_consumer_accepts fails if a
consumer ever raises its budget past the bound, so the two cannot drift apart
silently.

One behavior change worth your eye

The LLM semantic_* path has no character cap of its own, so a file above this
bound previously would have been chunked and sent to the provider. It now
reaches get_batches() absent from the cache. I believe that is the right
direction for a scanner (it is a real cost and resource exposure), and the
ledger event makes it visible rather than silent, but it is a change and not a
pure no-op, so I would rather flag it than bury it.

Minor: the canned message for SIZE_LIMIT reads "File exceeds this analyzer's
character limit", which is slightly off for a cache-phase byte gate. I left the
shared string alone since four analyzers depend on it. Happy to reword it in a
follow-up if you want.

Tests

Four tests: the skip and its ledger fields, the inclusive boundary, and the
derivation assertion.

test_build_context_never_reads_an_oversized_file spies on
_read_text_no_follow, because asserting only that the path is missing from
file_cache would still pass if the file were read in full and then discarded,
which leaves the peak-memory problem exactly where it was.

Verified against three mutants rather than assumed:

Mutant Caught by
Remove the gate entirely skip test + spy test
Read first, then test size (read-then-discard) spy test only
Exclusive bound (>=) boundary test

The middle row is why the spy test earns its place.

Oversized fixtures are sparse files via truncate(), so they cost no disk and
the suite stays fast.

pytest -q         # 2189 passed, 17 skipped, 4 xfailed
ruff check        # All checks passed
ruff format --check   # 175 files already formatted

Refs #131, which bounded the ingest layer; this closes the local-directory path
that budget does not reach.

`_read_file_cache()` called `_read_text_no_follow()` on every discovered
component, and that does an unbounded `source.read()`. A local directory scan
therefore materialized each file whole before any analyzer looked at it, so a
multi-GB file in a skill drove peak memory to its full size and was only then
skipped downstream at `MAX_FILE_CHARS`.

`INGEST_MAX_BYTES` does not cover this. Its own docstring scopes it to
"Each remote/archive ingest path", and a local directory target reaches
`build_context` through `validate_local_input_path()`, which does no sizing.
`MAX_FILE_BYTES` is not a gate here either: in this module it is used only
inside `_is_valid_oms_signature()` and for a `size_bytes` metadata field.

The gate reuses the stat already taken for the `S_ISREG` check, so it costs no
extra syscall, and it emits `LedgerOutcome.SKIPPED` with
`LedgerReason.SIZE_LIMIT` rather than raising. That matches what the static,
AST, taint, and YARA analyzers already do for oversized input, so the file is
reported as not-inspected instead of silently vanishing, and it flows into
`analysis_completeness` the same way.

The bound is derived rather than picked. Every downstream consumer limits
itself in characters, the largest being `MAX_PYTHON_AST_CACHE_SOURCE_CHARS`.
UTF-8 uses at most 4 bytes per character, and `errors="replace"` yields one
character per undecodable byte, so nothing above 4x that character budget can
decode to a size any consumer accepts. The gate is outcome-preserving by
construction: it cannot exclude content that would otherwise have been
analyzed, it only declines to materialize bytes already guaranteed to be
skipped. `test_cache_read_bound_cannot_exclude_content_any_consumer_accepts`
fails if a consumer ever raises its budget past the bound.

One behavior change worth calling out: the LLM `semantic_*` path has no
character cap of its own, so a file above this bound previously would have been
chunked and sent to the provider. It now reaches `get_batches()` absent from
the cache. That is the intended direction for a scanner, and the ledger event
makes it visible rather than silent.

Tests cover the skip, the ledger event fields, the inclusive boundary, and the
derivation. `test_build_context_never_reads_an_oversized_file` spies on
`_read_text_no_follow` because asserting only that the path is missing from
`file_cache` would still pass if the file were read in full and discarded,
which would leave the peak-memory problem exactly where it was. Verified
against three mutants: removing the gate, making it read before testing size,
and making the bound exclusive. The read-then-discard mutant is caught by that
spy test alone.

2189 passed, 17 skipped, 4 xfailed. Ruff clean.

Signed-off-by: Werner Kasselman <145896621+wernerkasselman-au@users.noreply.github.com>
@wernerkasselman-au

Copy link
Copy Markdown
Contributor Author

Hi Keshav,

Context for this one sits in a longer note I left on #375, so I will keep this
short rather than repeat it. The short version is that this PR carries the one
finding from #19 that is still true on current main (I have closed #19
itself), and it follows your ledger convention rather than the fail-closed
approach I originally proposed, so an oversized file is recorded as skipped
with LedgerReason.SIZE_LIMIT instead of aborting the scan.

Like #375 it is MERGEABLE and rebased on 5680c2c, and it is waiting on a fork
workflow approval before the checks can run, which is the one thing I cannot
trigger from my side.

Thanks,
Werner

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant