Skip to content

fix(gcsartifact): recognize wrapped storage.ErrObjectNotExist - #1375

Open
shoemoney wants to merge 1 commit into
google:mainfrom
shoemoney:fix/gcsartifact-wrapped-not-exist
Open

fix(gcsartifact): recognize wrapped storage.ErrObjectNotExist#1375
shoemoney wants to merge 1 commit into
google:mainfrom
shoemoney:fix/gcsartifact-wrapped-not-exist

Conversation

@shoemoney

Copy link
Copy Markdown

Link to Issue or Description of Change

2. Or, if no issue exists, describe the change:

Problem:

artifact/gcsartifact/service.go has two identical checks (Load and GetArtifactVersion) that compare the error from blob.attrs(ctx) against storage.ErrObjectNotExist with ==:

attrs, err := blob.attrs(ctx)
if err != nil {
    if err == storage.ErrObjectNotExist {
        return nil, fmt.Errorf("artifact '%s' not found: %w", blobName, fs.ErrNotExist)
    }
    return nil, fmt.Errorf("could not get blob attributes: %w", err)
}

The pinned cloud.google.com/go/storage v1.64.0 always wraps this sentinel. ObjectHandle.Attrs routes through httpStorageClient.GetObjectformatObjectErr:

func formatObjectErr(err error) error {
    var e *googleapi.Error
    if s, ok := status.FromError(err); (ok && s.Code() == codes.NotFound) ||
        (errors.As(err, &e) && e.Code == http.StatusNotFound) {
        return fmt.Errorf("%w: %w", ErrObjectNotExist, err)
    }
    return err
}

So in production err == storage.ErrObjectNotExist is always false, and the fs.ErrNotExist translation is dead code. A caller doing errors.Is(err, fs.ErrNotExist) after a real not-found gets a generic "could not get blob attributes" error instead of a not-found signal.

This package already documents the fix for this exact class of bug two functions up, on ErrVersionConflict:

// ErrVersionConflict ... It is always wrapped, so test for it with [errors.Is]

This PR applies that same rule to storage.ErrObjectNotExist at both call sites.

Solution:

Change both err == storage.ErrObjectNotExist checks to errors.Is(err, storage.ErrObjectNotExist). Both errors and storage were already imported. No other behavior changes — one concern only.

Testing Plan

Unit Tests:

  • I have added or updated unit tests for my change.
  • All unit tests pass locally.

Added TestNotFoundIsWrappedSentinel in artifact/gcsartifact/gcs_test.go, covering both Load and GetArtifactVersion. It adds a small attrsErr hook to the existing in-memory fake (fakeBucket/fakeObject) so attrs() can return a wrapped storage.ErrObjectNotExist — mirroring what the real client actually returns — instead of the fake's previous bare-sentinel behavior, which never exercised this bug.

Confirmed RED on the pre-fix code (test added, fix withheld):

=== RUN   TestNotFoundIsWrappedSentinel/Load
    gcs_test.go:246: err = could not get blob attributes: storage: object doesn't exist: googleapi: Error 404: Not Found, want errors.Is(err, fs.ErrNotExist) = true
--- FAIL: TestNotFoundIsWrappedSentinel/Load (0.00s)
=== RUN   TestNotFoundIsWrappedSentinel/GetArtifactVersion
    gcs_test.go:246: err = could not get blob attributes: storage: object doesn't exist: googleapi: Error 404: Not Found, want errors.Is(err, fs.ErrNotExist) = true
--- FAIL: TestNotFoundIsWrappedSentinel/GetArtifactVersion (0.00s)
FAIL

GREEN after the fix, along with the full existing package suite:

go test -race -mod=readonly -count=1 -shuffle=on ./artifact/... ./artifact/gcsartifact/...
ok  	google.golang.org/adk/v2/artifact	1.028s
ok  	google.golang.org/adk/v2/artifact/gcsartifact	1.043s

Also ran, all clean:

  • golangci-lint run ./artifact/... — 0 issues
  • golangci-lint fmt ./artifact/... — no changes
  • go mod tidy -diff — empty
  • go build -mod=readonly work — succeeds

Manual End-to-End (E2E) Tests:

Not applicable — this is a pure error-classification fix inside gcsService.Load/GetArtifactVersion, fully covered by the unit test above; no UI, runner, or live-GCS surface is touched.

Checklist

  • I have read the CONTRIBUTING.md document.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.
  • I have manually tested my changes end-to-end. (not applicable — see above)
  • Any dependent changes have been merged and published in downstream modules.

@shoemoney
shoemoney force-pushed the fix/gcsartifact-wrapped-not-exist branch from 94920b4 to 25e2ef7 Compare August 24, 2026 15:31
Load and GetArtifactVersion compared blob.attrs' error against
storage.ErrObjectNotExist with ==. cloud.google.com/go/storage always
wraps that sentinel (formatObjectErr: fmt.Errorf("%w: %w",
ErrObjectNotExist, err)), so the check never matched in production and
callers doing errors.Is(err, fs.ErrNotExist) got a generic "could not
get blob attributes" error instead of a not-found signal.

ErrVersionConflict a few lines up in this same file already documents
the fix: "It is always wrapped, so test for it with [errors.Is]".
Apply that same rule to storage.ErrObjectNotExist at both call sites.
@shoemoney
shoemoney force-pushed the fix/gcsartifact-wrapped-not-exist branch from 25e2ef7 to ceda149 Compare August 25, 2026 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant