Skip to content

fix(sources): handle streaming request errors - #1533

Merged
lengau merged 4 commits into
canonical:mainfrom
gcomneno:contrib/issue-1396-file-source-request-errors
May 27, 2026
Merged

fix(sources): handle streaming request errors#1533
lengau merged 4 commits into
canonical:mainfrom
gcomneno:contrib/issue-1396-file-source-request-errors

Conversation

@gcomneno

@gcomneno gcomneno commented Apr 3, 2026

Copy link
Copy Markdown
Contributor

Summary

Wrap request exceptions raised while streaming file source downloads into NetworkRequestError, so file source failures surface as source-facing errors instead of raw requests exceptions.

Testing

  • pytest -q tests/unit/sources/test_base.py -rs

Issue

Closes #1396

@gcomneno
gcomneno requested a review from a team as a code owner April 3, 2026 17:04

@bepri bepri left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @gcomneno. Checked this out locally and it seems good! Undoing your change in base.py breaks the unit test as I'd expect.

Would you mind adding a change log entry for this as well? You can assume it'll be part of the next patch release and put a placeholder like "Unreleased" for the date and we'll just update it once we know more about the next date/version.

@bepri
bepri requested a review from a team April 10, 2026 14:45
@gcomneno

Copy link
Copy Markdown
Contributor Author

Thanks, Imani — done. I added a changelog entry under Unreleased and pushed the update.

@gcomneno

Copy link
Copy Markdown
Contributor Author

I checked the current CI failures.

The QA/lint / files failure appears unrelated to this PR: the docs linkcheck is failing on an external packages.ubuntu.com URL with a 500 response in dotnet_v2_plugin.rst.

The test-other-plugins failure also looks transient/external from the current logs: the first failing job hits an interrupted Launchpad download and raises NetworkRequestError, after which the remaining matrix jobs are cancelled.

Could someone please rerun the failed jobs when convenient?

@gcomneno

Copy link
Copy Markdown
Contributor Author

I checked the current CI failures.

The QA/lint / files failure appears unrelated to this PR: docs linkcheck is failing on an external packages.ubuntu.com URL in dotnet_v2_plugin.rst with a 500 response.

The test-other-plugins failures also look external/transient from the logs: the first failing job hits an interrupted Launchpad download while pulling a tar source, after which the rest of the matrix gets cancelled.

Could someone please rerun the failed jobs when convenient?

@gcomneno
gcomneno force-pushed the contrib/issue-1396-file-source-request-errors branch from 3b29cf9 to 163fb09 Compare May 20, 2026 21:26
@gcomneno

Copy link
Copy Markdown
Contributor Author

Hi! Just gently following up on this one.

This PR is approved, and the current remaining blockers look CI-related rather than code review feedback:

  • Security scan / OSV-scanner
  • test-common / Slow tests (noble, ppc64el, edge, 3.12)
  • test-java-plugins / Slow tests (ubuntu-24.04-arm, 3.12)

Could someone please rerun the failed jobs when convenient? Happy to adjust anything if I missed a code-related failure.

@lengau lengau left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@lengau

lengau commented May 26, 2026

Copy link
Copy Markdown
Contributor

@gcomneno I've merged the latest and approved test runs — we have a test-flaky job now which should contain those flaky tests so we can hopefully limit the flakiness issue :-)

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR improves file-source download error handling by ensuring exceptions raised during streaming downloads surface as source-facing NetworkRequestError rather than raw requests exceptions.

Changes:

  • Move the streaming download (url_utils.download_request(...)) into the request exception-handling block so streaming failures are wrapped consistently.
  • Add a unit test covering a streaming ChunkedEncodingError during URL pulls.
  • Document the fix in the unreleased changelog.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
craft_parts/sources/base.py Wraps streaming download exceptions into NetworkRequestError by moving the streaming call inside the existing try/except.
tests/unit/sources/test_base.py Adds a unit test to assert streaming request exceptions are raised as NetworkRequestError.
docs/reference/changelog.rst Adds an unreleased changelog entry documenting the bug fix.

Comment thread craft_parts/sources/base.py Outdated
…file-source-request-errors

# Conflicts:
#	craft_parts/sources/base.py
@lengau
lengau merged commit bbc6f9f into canonical:main May 27, 2026
48 of 50 checks passed
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.

Certain requests exceptions are poorly handled with file sources

4 participants