Skip to content

fix(storage): propagate blob commit errors - #634

Merged
chlins merged 1 commit into
mainfrom
fix/propagate-blob-commit-error
Aug 14, 2026
Merged

fix(storage): propagate blob commit errors#634
chlins merged 1 commit into
mainfrom
fix/propagate-blob-commit-error

Conversation

@chlins

@chlins chlins commented Aug 14, 2026

Copy link
Copy Markdown
Member

This pull request improves error handling in the PushBlob method and adds a corresponding unit test to ensure the correct propagation of errors. The main focus is to make sure that when blob.Commit fails, the error is properly returned to the caller instead of being silently ignored.

Error handling improvements:

  • Updated PushBlob in pkg/storage/distribution/distribution.go to return the actual error from blob.Commit instead of returning nil, ensuring that commit errors are properly propagated.

Testing enhancements:

  • Added a new test TestPushBlobPropagatesCommitError in pkg/storage/distribution/distribution_test.go to verify that PushBlob returns the correct error type when a commit fails due to an invalid digest.

@chlins chlins added the bug Something isn't working label Aug 14, 2026
Signed-off-by: chlins <chlins.zhang@gmail.com>
@chlins
chlins force-pushed the fix/propagate-blob-commit-error branch from ab052df to 499d5df Compare August 14, 2026 06:08
@chlins
chlins enabled auto-merge (squash) August 14, 2026 06:09

@aftersnow aftersnow 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.

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:

  1. The error paths after Blobs().Create() never call blob.Cancel(ctx), so the upload dir leaks. blobWriter.Commit bails before removeResources on error, and I confirmed one leftover entry under _uploads/ after a failed commit. This only became reachable now that commit errors actually surface.
  2. OutputConfig in build/local.go shadows the digest parameter with the PushBlob return value, so hooks.OnError(digest, err) gets an empty string while OnStart used the original digest, meaning the progress entry never gets aborted. OutputLayer keys on relPath so it's fine.

Minor: require.ErrorAs would give a better failure message than require.True(t, errors.As(...)), which just prints "Should be true".

@chlins
chlins merged commit e1de618 into main Aug 14, 2026
5 checks passed
@chlins
chlins deleted the fix/propagate-blob-commit-error branch August 14, 2026 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants