Skip to content

Fix temp file leak in pull-through metadata streaming. #7933

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
aKlimau wants to merge 1 commit into
base: main
Choose a base branch
from
+7 −3
Open

Fix temp file leak in pull-through metadata streaming. #7933

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES/7846.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix temp file leak in pull-through metadata streaming.
9 changes: 6 additions & 3 deletions pulpcore/content/handler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -904,6 +904,7 @@ async def _match_and_stream(self, path, request):
# Try to stream the RemoteArtifact and potentially save it as a new Content unit
save_artifact = (
remote.get_remote_artifact_content_type(original_rel_path) is not None
and remote.policy != Remote.STREAMED
)
ca = ContentArtifact(relative_path=original_rel_path)
ra = RemoteArtifact(remote=remote, url=url, content_artifact=ca)
Expand Down Expand Up @@ -1202,6 +1203,8 @@ async def _stream_remote_artifact(
"""

remote = await remote_artifact.remote.acast()
if remote.policy == Remote.STREAMED:
save_artifact = False
Comment on lines +1206 to +1207

@mdellweg mdellweg Aug 3, 2026

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.

Now these lines suprise me again.
I'd rather remove the default True from the function definition and move this comparison to the call site.

log.debug(
"Streaming content for {url} from Remote {remote}-{source}".format(
url=request.match_info["path"], remote=remote.name, source=remote_artifact.url
Expand Down Expand Up @@ -1289,12 +1292,12 @@ async def handle_data(data):
data_size_handled = data_size_handled + len(data)
else:
await response.write(data)
if remote.policy != Remote.STREAMED:
if save_artifact:
await original_handle_data(data)

@mdellweg mdellweg Jul 29, 2026

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.

This is where the downloader saves the file to disk?
At the very least this change looks reasonable to me.

@mdellweg mdellweg Jul 29, 2026

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.

It looks like there are several places where we check for exactly save_artifact and remote.policy != Remote.STREAMED can we redefine the variable so that it already contains the streamed condition?

(Somehow this may be a copy-and-paste / missed-all-the-places-to-update bug coming from exactly there.)

@aKlimau aKlimau Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is where the downloader saves the file to disk?

Yes, original_handle_data that usually points to BasedDownloader handle_data that does the writing (if not overriden by plugins downloader)


async def finalize():
nonlocal failed_download
if save_artifact and remote.policy != Remote.STREAMED:
if save_artifact:
await original_finalize()
failed_download = False

Expand Down Expand Up @@ -1342,7 +1345,7 @@ async def finalize():
if hasattr(downloader, "session"):
await downloader.session.close()

if save_artifact and remote.policy != Remote.STREAMED:
if save_artifact:
content_artifacts = await asyncio.shield(
sync_to_async(self._save_artifact)(download_result, remote_artifact, request)
)
Expand Down
Loading