Skip to content

Importers: one capped stream reader instead of five - #1017

Merged
ryanbr merged 2 commits into
mainfrom
refactor/read-capped
Aug 1, 2026
Merged

ryanbr merged 2 commits into
mainfrom
refactor/read-capped

Conversation

@ryanbr

@ryanbr ryanbr commented Aug 1, 2026 •

Copy link
Copy Markdown
Owner

Five copies of a safety guard become one, with the tests none of them had.

What readCapped is

The memory-exhaustion guard on user-supplied import files. A content:// URI or a zip entry can claim any size it likes, and every importer reads the whole thing into memory before parsing — so without a ceiling a malicious or merely broken file takes the process down with an OOM.

It existed five times, with identical bodies, under two names (readCapped / readCappedBytes) and two shapes (extension function / member function):

File Name Shape Cap
ActivityFileImporter readCapped extension 128 MB
NutritionCsvImporter readCapped extension 64 MB
LiftingImporter readCapped extension 64 MB
LabMarkerCsvImport readCappedBytes extension 32 MB
WearableExportImporter readCapped member 256 MB

The duplication was known, not accidental — each copy carried a comment noting that the twins elsewhere "are not visible here". That is the wrong thing to accept for a guard: a hardening or a fix lands on one and misses four.

What is deliberately NOT shared

The cap. It is policy and it legitimately differs by file type — 32 MB for a lab-marker CSV against 512 MB for a Xiaomi zip entry. Each importer keeps its own constant and passes it in. Only the mechanism moves.

The failure wording, preserved exactly via a what parameter: everything says "Input exceeds …" except the zip-entry path, which says "Entry exceeds …". Those messages reach users through an import failure, so none of them change.

Tests

Seven, where the five copies had none between them:

  • content under the cap returned byte-for-byte (a guard must not corrupt what it allows)
  • reads spanning multiple 64 KiB chunks
  • exactly at the cap is allowed — the check is > cap, not >=, and nothing pinned that
  • one byte over throws
  • an empty stream is allowed (an empty file should fail in the parser, not here)
  • both failure messages, verbatim
  • it throws before buffering the whole oversize stream — asserted with a counting InputStream, because "returns an error eventually" is not the property this exists for; bounded peak allocation is

Verification

  • Kotlin: full src/main/java compile against a main worktree — 2517 errors both sides, error sets byte-identical, 0 in any of the six touched files.
  • The new suite runs green locally: OK (7 tests).
  • Tools/doc_comment_lint.py clean.
  • Nine orphaned java.io imports removed with the bodies. Kotlin only warns on unused imports and the ad-hoc compile runs -nowarn, so these would have sat there unnoticed — worth flagging as a gap in how I verify, not just a tidy-up.

Net: 81 lines deleted, 7 added across the importers, plus one 40-line helper and its tests.

Parity

Android-only. The Swift importers take Data from the document picker rather than streaming an InputStream, so there is no equivalent read loop to consolidate. No stored value or analytic changes.

Re-review: the extraction silently widened visibility

All five originals were file-private. Extracting them produced a public extension on InputStream — a wider API than the refactor asked for, surfacing on every InputStream in the module's autocomplete and callable from anywhere.

Nothing outside com.noop.ingest calls it, and Kotlin has no package-private, so it is now internal: the tightest scope that still reaches the five importers and the unit tests. Precedent in the tree — HealthConnectImporter.maxSourceLong is internal and tested the same way.

Worth naming because it is a whole class of quiet change: moving code between scopes alters its visibility, and the compiler never complains about getting more access than you had. Widening a private helper to public API should be asked for, not arrive as a side effect of a refactor.

Also re-verified this pass: zin is a ZipInputStream, so the extension resolves on the subclass and read() still returns -1 at the end of the entry rather than the stream — the behaviour the removed member function relied on is preserved. And WearableExportImporter legitimately still imports InputStream, which is why only its ByteArrayOutputStream import was removed.

ryanbr added 2 commits August 1, 2026 00:22
readCapped is the memory-exhaustion guard on user-supplied import files - a
content:// URI or a zip entry can claim any size, and every importer reads
the whole thing before parsing. It existed FIVE times, with identical bodies,
under two names (readCapped / readCappedBytes) and two shapes (extension /
member).

The duplication was known, not accidental: each copy carried a comment
saying the twins elsewhere "are not visible here". That is the wrong thing to
accept for a guard - a hardening or a fix lands on one and misses four.

The CAP is deliberately NOT centralised. It is policy and legitimately
differs by file type: 32 MB for a lab-marker CSV, 512 MB for a Xiaomi zip
entry. Each importer keeps its constant and passes it; only the mechanism
moves.

The failure wording is preserved exactly via a `what` parameter - everything
says "Input exceeds ..." except the zip-entry path, which says "Entry" - so
no user-visible message changes.

Seven tests, where the five copies had none between them: content under the
cap returned byte-for-byte, multi-chunk reads, exactly-at-cap allowed,
one-over throws, empty allowed, both failure messages, and that it throws
BEFORE buffering an oversize stream, which is the property the guard exists
for rather than merely returning an error.

Nine orphaned java.io imports removed with the bodies. Kotlin only warns on
those and the ad-hoc compile runs -nowarn, so they would have sat unnoticed.
All five originals were file-private. Extracting them produced a PUBLIC
extension on InputStream, which is a wider API than the refactor asked for -
it would surface on every InputStream in the module's autocomplete and be
callable from anywhere.

Nothing outside com.noop.ingest calls it, and Kotlin has no package-private,
so internal is the tightest scope that still reaches the five importers and
the unit tests. Precedent in the tree: HealthConnectImporter.maxSourceLong is
internal and tested the same way.

Widening a private helper to public API is the kind of change that should be
asked for, not arrive as a side effect of moving code.
@ryanbr
ryanbr merged commit 89d84e0 into main Aug 1, 2026
3 checks passed
@ryanbr
ryanbr deleted the refactor/read-capped branch August 1, 2026 07:39
simoncad7 pushed a commit to simoncad7/noop that referenced this pull request Aug 17, 2026
* Importers: one capped stream reader instead of five

readCapped is the memory-exhaustion guard on user-supplied import files - a
content:// URI or a zip entry can claim any size, and every importer reads
the whole thing before parsing. It existed FIVE times, with identical bodies,
under two names (readCapped / readCappedBytes) and two shapes (extension /
member).

The duplication was known, not accidental: each copy carried a comment
saying the twins elsewhere "are not visible here". That is the wrong thing to
accept for a guard - a hardening or a fix lands on one and misses four.

The CAP is deliberately NOT centralised. It is policy and legitimately
differs by file type: 32 MB for a lab-marker CSV, 512 MB for a Xiaomi zip
entry. Each importer keeps its constant and passes it; only the mechanism
moves.

The failure wording is preserved exactly via a `what` parameter - everything
says "Input exceeds ..." except the zip-entry path, which says "Entry" - so
no user-visible message changes.

Seven tests, where the five copies had none between them: content under the
cap returned byte-for-byte, multi-chunk reads, exactly-at-cap allowed,
one-over throws, empty allowed, both failure messages, and that it throws
BEFORE buffering an oversize stream, which is the property the guard exists
for rather than merely returning an error.

Nine orphaned java.io imports removed with the bodies. Kotlin only warns on
those and the ad-hoc compile runs -nowarn, so they would have sat unnoticed.

* Keep the shared reader internal, as every copy it replaces was private

All five originals were file-private. Extracting them produced a PUBLIC
extension on InputStream, which is a wider API than the refactor asked for -
it would surface on every InputStream in the module's autocomplete and be
callable from anywhere.

Nothing outside com.noop.ingest calls it, and Kotlin has no package-private,
so internal is the tightest scope that still reaches the five importers and
the unit tests. Precedent in the tree: HealthConnectImporter.maxSourceLong is
internal and tested the same way.

Widening a private helper to public API is the kind of change that should be
asked for, not arrive as a side effect of moving code.
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