Fix issues from container build digest code review#55322
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 91cd6a00-c452-4c2b-b80a-b7f6784aeaff
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 91cd6a00-c452-4c2b-b80a-b7f6784aeaff
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 91cd6a00-c452-4c2b-b80a-b7f6784aeaff
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 91cd6a00-c452-4c2b-b80a-b7f6784aeaff
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 91cd6a00-c452-4c2b-b80a-b7f6784aeaff
|
Azure Pipelines: Successfully started running 1 pipeline(s). 2 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
This PR follows up on the container build digest-validation review by tightening resource cleanup and extending unit test coverage for digest mismatch scenarios in the container registry/layer code paths.
Changes:
- Add unit tests to validate behavior when cached/downloaded blobs have invalid digests, and ensure temp artifacts are cleaned up.
- Ensure temp files created during blob download and layer creation are cleaned up via
finallyblocks. - Minor logging template adjustment and removal of an unused digest helper.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/Microsoft.NET.Build.Containers.UnitTests/RegistryTests.cs | Adds tests for redownloading cached blobs with invalid digests and rejecting downloaded blobs with invalid digests. |
| test/Microsoft.NET.Build.Containers.UnitTests/LayerTests.cs | Adds a test to ensure temp files are deleted when Layer.FromDirectory fails. |
| src/Containers/Microsoft.NET.Build.Containers/Registry/Registry.cs | Ensures blob download temp file is cleaned up and improves cached-digest failure logging. |
| src/Containers/Microsoft.NET.Build.Containers/Layer.cs | Ensures layer creation temp file is cleaned up on failure. |
| src/Containers/Microsoft.NET.Build.Containers/DigestUtils.cs | Removes an unused internal hash-validation helper. |
| writer.WriteEntry(entry); | ||
|
|
||
| if (entry.DataStream is not null) | ||
| { | ||
| // no longer relying on the `using` of the FileStream, so need to do it manually | ||
| entry.DataStream.Dispose(); | ||
| } | ||
| if (entry.DataStream is not null) | ||
| { | ||
| // no longer relying on the `using` of the FileStream, so need to do it manually |
|
|
||
| // On Unix, we can determine the x-bit based on the filesystem permission. | ||
| // On Windows, we use executable permissions for all entries. | ||
| return (OperatingSystem.IsWindows() || ((file.UnixFileMode | UnixFileMode.UserExecute) != 0)) ? executeMode : nonExecuteMode; |
| finally | ||
| { | ||
| File.Delete(tempTarballPath); | ||
| } |
There was a problem hiding this comment.
these try/catches for deletes are really bad whitespace-wise. maybe a structure that wraps a file-create in a Disposable so it can be deleted-on-leaving-scope in a clean way would be easier to read and use correctly without touching so much code?
| finally | ||
| { | ||
| File.Delete(tempTarballPath); | ||
| } |
| { | ||
| retryCount++; | ||
| if (retryCount >= MaxDownloadRetries) | ||
| catch (Exception ex) |
There was a problem hiding this comment.
This will catch cancellation exceptions. Those should bubble up as-is.
| { | ||
| // Incorrect digest | ||
| _logger.LogTrace( | ||
| "Digest validation failed for cached blob {1} ({2}), redownloading from registry.", |
There was a problem hiding this comment.
Is this correct? I think this is using .NET's String.Format placeholders, which only accept numeric placeholders.
| { | ||
| retryCount++; | ||
| if (retryCount >= MaxDownloadRetries) | ||
| catch (Exception ex) |
| finally | ||
| { | ||
| File.Delete(tempTarballPath); | ||
| } |
There was a problem hiding this comment.
these try/catches for deletes are really bad whitespace-wise. maybe a structure that wraps a file-create in a Disposable so it can be deleted-on-leaving-scope in a clean way would be easier to read and use correctly without touching so much code?
| } | ||
| finally | ||
| { | ||
| File.Delete(tempTarballPath); |
There was a problem hiding this comment.
similar comment here: this is high-touch and a Disposable file would solve it more cleanly for review purposes.
This PR fixes Copilot's code review comments from #55301.