fix(packages): bound archive downloads and gate them in offline mode - #126
Open
Krunal-Karena wants to merge 1 commit into
Open
fix(packages): bound archive downloads and gate them in offline mode#126Krunal-Karena wants to merge 1 commit into
Krunal-Karena wants to merge 1 commit into
Conversation
`PackageFetcher` downloaded with `urlretrieve()`, which has no timeout, so one unresponsive mirror hung `ebuild build` until the user killed it. Downloads now stream through `urllib.request.urlopen(..., timeout=...)` -- 30 s default, configurable via `PackageFetcher(..., timeout=...)` -- refuse anything over 512 MB while streaming, and are written to a `.part` file renamed into place only when complete, so a connection that dies mid-body no longer leaves a truncated archive in the download cache to satisfy every later fetch. `EBUILD_OFFLINE=1` and `ebuild update-index --offline` already governed index synchronization; archive fetching was exempt and hit the network anyway. A fetch whose archive is not already cached now fails with a message naming the missing archive. A cached archive still extracts, so air-gapped rebuilds work from a warmed cache. 22 fetcher tests pass, 6 of them new. The full suite is unchanged from master apart from those: the 9 `test_index_sync.py` failures on master (`PackageRecipe.to_dict()`, dropped in embeddedos-org#112) are untouched here.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
There are verified correctness issues in the new/updated codepaths/tests (a non-context-manager urlopen stub and an offline-mode crash when recipe.url is missing).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens PackageFetcher’s archive download path to be safer and more predictable under adverse network conditions, and aligns archive fetching with the project’s offline mode behavior.
Changes:
- Replace
urlretrieve()with streamingurllib.request.urlopen(..., timeout=...), add a default timeout, and make it configurable viaPackageFetcher(timeout=...). - Enforce a maximum archive size while streaming and write downloads atomically via a
.partfile +os.replace(). - Gate network downloads in offline mode and add/port tests; document behavior in the changelog and record a separate known issue in
TASKS.md.
File summaries
| File | Description |
|---|---|
ebuild/packages/fetcher.py |
Adds streaming downloads with timeout, size cap, atomic .part writes, and offline gating for uncached archives. |
tests/ebuild/test_package_fetcher.py |
Ports tests from urlretrieve stubs to urlopen stubs and adds coverage for timeout/size/atomicity/offline behavior. |
CHANGELOG.md |
Documents the download hardening and offline gating fixes. |
TASKS.md |
Records the pre-existing PackageRecipe.to_dict() callsite issue as T-006. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+341
to
+344
| def _urlopen(request, timeout=None): | ||
| seen["timeout"] = timeout | ||
| return io.BytesIO(b"") | ||
|
|
Comment on lines
+78
to
+88
| # Offline mode must gate archive fetching the same way it gates index | ||
| # synchronization: a fetch that is not already in the download cache | ||
| # requires the network, and in offline mode there is no network. | ||
| if is_offline() and not self.is_downloaded(recipe): | ||
| archive_path = self._archive_path(recipe) | ||
| where = f" ({archive_path})" if archive_path else "" | ||
| raise FetchError( | ||
| f"Offline mode (EBUILD_OFFLINE=1): package {recipe.name} " | ||
| f"v{recipe.version} is not in the download cache{where}. " | ||
| f"Re-run without offline mode to download it." | ||
| ) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
ebuild/packages/fetcher.py(PackageFetcher) — four related downloadhardening fixes:
Timeout. Downloads used
urlretrieve(), which has no timeout, soone unresponsive mirror hung
ebuild builduntil the user killed it.Downloads now stream through
urllib.request.urlopen(..., timeout=...)— 30 s default, configurable via
PackageFetcher(..., timeout=...).Size cap. Anything over 512 MB is rejected while streaming, so a
server that lies about (or omits)
Content-Lengthstill cannot fillthe disk.
Atomic writes. The archive streams to a
.partfile that isrenamed into place only when complete. A connection that dies
mid-body no longer leaves a truncated archive in the download cache —
_download()short-circuits on existence, so a partial archive wouldotherwise be served by every later fetch as if it were the real thing.
Offline gate.
EBUILD_OFFLINE=1andebuild update-index --offlinealready governed index synchronization; archive fetchingwas exempt and hit the network anyway (the CHANGELOG itself noted
"package archive fetching is not yet offline-gated"). A fetch whose
archive is not already cached now fails with a message naming the
missing archive. A cached archive still extracts, so air-gapped
rebuilds work from a warmed cache.
Why
All four are the same failure shape: the fetcher trusted the network
more than it should. A pin (URL + SHA-256) protects integrity, but
nothing bounded availability (hang) or disk use (runaway response), and
a truncated download silently poisoned the cache. The offline gap was an
acknowledged inconsistency between the two halves of the package
subsystem.
Testing / validation
tests/ebuild/test_package_fetcher.py: timeout reachesurlopen, oversized download rejected with no files left behind,truncated download leaves neither a cache entry nor
.partlitter,offline refusal, offline reuse of a warmed cache, default timeout.
urlretrieveseam to anurlopenstub; no test touches the network.22/22fetcher tests pass. Full suite: 676 passed, identical tomaster's result except for the 6 new passing tests (see note below).
ruff checkclean on touched files;mypyclean onfetcher.pyapart from one pre-existing error described below.
Note: a pre-existing failure observed, not touched
PackageRecipe.to_dict()was dropped in #112, butebuild/packages/index_sync.py:354still calls it — soebuild update-indexfails withAttributeErroron the first recipe it caches,and 9 tests in
tests/unit/test_index_sync.pyfail on currentmaster.This PR deliberately does not fix it to keep the review surface small;
it is recorded in
TASKS.md(T-006) and would make a clean follow-up PR.