fix(storage): propagate blob commit errors - #634
Merged
Conversation
Signed-off-by: chlins <chlins.zhang@gmail.com>
chlins
force-pushed
the
fix/propagate-blob-commit-error
branch
from
August 14, 2026 06:08
ab052df to
499d5df
Compare
chlins
enabled auto-merge (squash)
August 14, 2026 06:09
aftersnow
approved these changes
Aug 14, 2026
aftersnow
left a comment
Contributor
There was a problem hiding this comment.
LGTM. Verified locally: the new test passes with the fix and fails when the return "", 0, err line is reverted, so it's a real regression test. Also grepped the repo, no other if err != nil { return ..., nil } left.
Worth noting the impact is wider than the description suggests. With the old code, pull.go treated a failed commit as success and kept writing the manifest, producing a local model that looks complete but is missing layers. And build/local.go fed digest="", size=0 straight into the descriptor.
Two follow-ups, not blocking this PR:
- The error paths after
Blobs().Create()never callblob.Cancel(ctx), so the upload dir leaks.blobWriter.Commitbails beforeremoveResourceson error, and I confirmed one leftover entry under_uploads/after a failed commit. This only became reachable now that commit errors actually surface. OutputConfiginbuild/local.goshadows thedigestparameter with thePushBlobreturn value, sohooks.OnError(digest, err)gets an empty string whileOnStartused the original digest, meaning the progress entry never gets aborted.OutputLayerkeys onrelPathso it's fine.
Minor: require.ErrorAs would give a better failure message than require.True(t, errors.As(...)), which just prints "Should be true".
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.
This pull request improves error handling in the
PushBlobmethod and adds a corresponding unit test to ensure the correct propagation of errors. The main focus is to make sure that whenblob.Commitfails, the error is properly returned to the caller instead of being silently ignored.Error handling improvements:
PushBlobinpkg/storage/distribution/distribution.goto return the actual error fromblob.Commitinstead of returningnil, ensuring that commit errors are properly propagated.Testing enhancements:
TestPushBlobPropagatesCommitErrorinpkg/storage/distribution/distribution_test.goto verify thatPushBlobreturns the correct error type when a commit fails due to an invalid digest.