Skip to content

fix(pkg): bound local mirror archive reads - #42

Draft
TheHalfMoon wants to merge 3 commits into
mainfrom
fix/local-mirror-archive-bound
Draft

fix(pkg): bound local mirror archive reads#42
TheHalfMoon wants to merge 3 commits into
mainfrom
fix/local-mirror-archive-bound

Conversation

@TheHalfMoon

@TheHalfMoon TheHalfMoon commented Aug 26, 2026

Copy link
Copy Markdown
Owner

Scope

Repair issue #38 by enforcing one commandF-owned compressed package archive size limit across both public registry acquisition and local-mirror acquisition.

Base at branch creation:

main: eeecb0bc03c7040bb18b70bce8b69d618384f783

Current head:

a7dbda4eac62adf8aaff15519ac3876ce4828d74

This PR is Draft for early qualification only; do not merge ahead of the current cache-integrity repair sequence without re-reading live main and reconciling dependencies.

Repair

  • define MAX_PACKAGE_ARCHIVE_BYTES = 128 MiB once in package-source code;
  • registry acquisition continues using the same 128 MiB bound, now via the shared constant;
  • LocalMirrorSource::archive opens the selected package and reads at most MAX + 1 bytes instead of fs::read allocating the whole caller-controlled file;
  • oversize input fails closed with a stable InvalidRequest classification;
  • package-not-found behavior and source/provenance semantics remain unchanged.

Regression

The bounded reader is generic over Read, allowing cheap unit tests that prove:

  • exact bound accepted;
  • bound+1 rejected;

without allocating a 128 MiB fixture in CI.

The existing ignored real-registry smoke expectations are preserved unchanged.

Non-goals

Qualification

Before Ready/merge: reconcile against live canonical main, run all exact-head mandatory/path-applicable workflows, obtain fresh Qodo + CodeRabbit review truth, and require zero unresolved substantive findings.

Refs #38.


Summary by cubic

Fixes issue #38 by applying the same 128 MiB compressed archive size limit to local mirror reads that the public registry already enforced. Oversized local archives now fail closed with InvalidRequest instead of being fully read into memory.

  • Extracts the archive limit into a shared constant used by both registry and local mirror acquisition.
  • Adds a bounded reader that is generic over Read, so unit tests verify the exact-limit and over-limit behavior without allocating a 128 MiB fixture.
  • Package-not-found behavior and source/provenance semantics are unchanged.

Written for commit a7dbda4. Summary will update on new commits.

Review in cubic

@coderabbitai

coderabbitai Bot commented Aug 26, 2026

Copy link
Copy Markdown

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

Comment @coderabbitai help to get the list of available commands.

Copy link
Copy Markdown
Owner Author

@qodo review

Please perform an early review of exact Draft head a7dbda4eac62adf8aaff15519ac3876ce4828d74. Focus on whether the 128 MiB limit remains identical for registry and local mirror, whether local reads are truly bounded before full allocation, preservation of PackageNotFound/source semantics, correctness of the small-limit regression seam, and accidental changes outside #38. This is early review only, not merge qualification.

@qodo-code-review

Copy link
Copy Markdown

review

Please perform an early review of exact Draft head a7dbda4eac62adf8aaff15519ac3876ce4828d74. Focus on whether the 128 MiB limit remains identical for registry and local mirror, whether local reads are truly bounded before full allocation, preservation of PackageNotFound/source semantics, correctness of the small-limit regression seam, and accidental changes outside #38. This is early review only, not merge qualification.

Early review of draft a7dbda4

I found no correctness issue in the supplied diff for the requested scope; no findings to report.

  • Shared 128 MiB limit: MAX_PACKAGE_ARCHIVE_BYTES is defined once as 128 * 1024 * 1024 in source.rs:9 and imported by registry.rs:8-10. Registry downloads apply it at registry.rs:148-153; local mirrors apply the same constant at source.rs:99. The accepted maximum is therefore identical.
  • Local-read bounding: source.rs:103-113 opens the file first and reads through Take(max_bytes + 1), so it can consume at most 128 MiB plus one probe byte—not the complete file—before returning InvalidRequest. It no longer uses fs::read, which would allocate the entire archive.
  • Not-found and source semantics: The File::open error mapping at source.rs:89-98 preserves the prior PackageNotFound behavior for missing archives and Io for other open failures. source_id() and registry PackageArchive.source behavior are unchanged.
  • Regression seam: read_bounded_archive accepts an injected max_bytes; the tests at source.rs:122-132 cover exactly-at-limit success and limit-plus-one failure without requiring a 128 MiB fixture. The saturating_add(1) is also safe for the helper’s arbitrary test inputs.
  • Scope: The diff is limited to the shared archive-limit extraction and local bounded reading in source.rs, plus registry reuse in registry.rs; I found no accidental changes outside that scope.

One minor nuance: the local path intentionally reads one byte beyond the limit to distinguish an exact-limit file from an oversized one, so its failure path may transiently hold 128 MiB + 1 byte. That does not change the 128 MiB accepted limit or permit full-file allocation.

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