fix tarsplit file leak #2323
Merged
fix tarsplit file leak #2323
Conversation
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
8106f3d to
577cdc5
Compare
kolyshkin
reviewed
Apr 29, 2025
577cdc5 to
3cf1a04
Compare
mtrmac
requested changes
Apr 30, 2025
There was a problem hiding this comment.
I think this fixes the part where closing chunkedDiffer closes / deletes the temporary file, by “stealing” the file handle. That’s fine.
But the current rules as documented in PrepareStagedLayer are that the caller only should only call CleanupStagedLayer if ApplyStagedLayer is not called / does not succeed; so, closing the file only in Cleanup… looks wrong. (Arguably the API is inconvenient, and should be changed in a way that callers always call “cleanup”, even on success of ApplyStagedLayer.)
3cf1a04 to
5408ffe
Compare
mtrmac
reviewed
May 6, 2025
mtrmac
approved these changes
May 6, 2025
kolyshkin
reviewed
May 6, 2025
store.go
Outdated
| @@ -365,6 +365,8 @@ type Store interface { | |||
| // PrepareStagedLayer applies a diff to a layer. | |||
| // It is the caller responsibility to clean the staging directory if it is not | |||
| // successfully applied with ApplyStagedLayer. | |||
| // The caller must ensure ApplyStagedLayer or CleanupStagedLayer is called eventually with | |||
There was a problem hiding this comment.
nit: we can start using references to other functions/methods, i.e.
Suggested change
| // The caller must ensure ApplyStagedLayer or CleanupStagedLayer is called eventually with | |
| // The caller must ensure [Store.ApplyStagedLayer] or [Store.CleanupStagedLayer] is called eventually with |
There was a problem hiding this comment.
thanks, I've fixed only this instance for now and the new comments I've added
5408ffe to
e6746b8
Compare
The ApplyDiffWithDiffer function was marked as deprecated, with PrepareStagedLayer being the recommended replacement. Its implementation was just a wrapper around PrepareStagedLayer. Remove the deprecated function from the Store and LayerStore interfaces and its implementation. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
This change makes the final `nil` error return explicit, making the successful exit path clearer. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Transfer ownership of the `tocDigest` file handle upon successful completion of `ApplyDiff` in the chunked differ. This prevents the handle from being closed by the differ if the operation succeeds, as ownership is effectively passed along. Explicitly close the `TarSplit` reader during `CleanupStagedLayer` and `ApplyDiffWithDiffer`. This ensures the associated file handle is released even if the layer application process is aborted or fails before the reader is naturally consumed and closed elsewhere. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Ensure temporary file descriptors for tar-split data are closed properly in error paths within zstd:chunked handling. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
The chunkedDiffer object holds state and resources that are managed within a single ApplyDiff call. Reusing the same differ instance for multiple ApplyDiff calls could lead to incorrect state or errors related to already-closed resources. Add a flag and check to ensure ApplyDiff cannot be called more than once on the same chunkedDiffer instance, making its usage pattern explicit and preventing potential misuse. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
it is an explicit API breaking change, so that cannot be used by old users (e.g. an older containers/image version) that are not ported to support the new semantic that only one ApplyDiffWithDiffer call is supported for one differ object. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
The comment for the `TarSplit` field in `DriverWithDifferOutput` was updated to make its ownership and the responsibility for closing the file descriptor explicit. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
e6746b8 to
6e95dfc
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giuseppe, mtrmac The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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.
follow-up for #2312