Skip to content

blob/fileblob: fix nil underlying error in IfNotExist precondition check#3734

Open
LeSingh1 wants to merge 3 commits into
google:masterfrom
LeSingh1:fix/fileblob-ifnotexist-nil-error
Open

blob/fileblob: fix nil underlying error in IfNotExist precondition check#3734
LeSingh1 wants to merge 3 commits into
google:masterfrom
LeSingh1:fix/fileblob-ifnotexist-nil-error

Conversation

@LeSingh1

Copy link
Copy Markdown
Contributor

Bug

In both writer.Close() and writerWithSidecar.Close(), the IfNotExist precondition check has two bugs:

  1. Nil underlying error: When os.Stat succeeds (meaning the file exists), err is nil. That nil is passed directly to gcerr.New() as the underlying error, resulting in an error chain with no meaningful cause. errors.Is / errors.As callers get no wrapped context.

  2. Typo: The message "File already exist" is grammatically incorrect and inconsistent with how memblob reports the same condition.

// Before (both writer.Close and writerWithSidecar.Close)
_, err = os.Stat(w.path)
if err == nil {
    return gcerr.New(gcerrors.FailedPrecondition, err, 1, "File already exist")
    //                                            ^^^-- nil!
}

Fix

Replace the nil underlying error with a descriptive fmt.Errorf, and update the message to be consistent with memblob's phrasing:

// After
_, err = os.Stat(w.path)
if err == nil {
    return gcerr.New(gcerrors.FailedPrecondition, fmt.Errorf("blob %q already exists", w.path), 1, "fileblob: IfNotExist precondition failed")
}

This matches the pattern in memblob where IfNotExist returns:

err := fmt.Errorf("a blob already exists for key %q", w.key)
return gcerr.New(gcerrors.FailedPrecondition, err, 1, "IfNotExist precondition failed")

Testing

The existing drivertest conformance suite exercises IfNotExist writes; this fix ensures the error returned has a non-nil cause when the precondition triggers.

LeSingh1 added 3 commits June 11, 2026 23:53
Copy was assigning the source *blobEntry pointer directly to the
destination key. Two bugs follow from that.

First, the src and dst entries shared the same *driver.Attributes
pointer and the same Metadata map. Any in-place mutation of the
attributes or metadata through one key would silently corrupt the other.

Second, when copying onto an existing key, the destination's original
CreateTime was replaced with the source's CreateTime. The reference
implementation (fileblob) routes all copies through NewTypedWriter,
which preserves the existing CreateTime of any blob being overwritten.
memblob did not do the same.

The fix deep-copies the Attributes struct and Metadata map for the new
destination entry, and carries forward the destination's previous
CreateTime when it already exists.

Two new unit tests cover both cases: TestCopyPreservesDestinationCreateTime
and TestCopyDoesNotAliasEntry.
…rivertest

Per review, copying onto an existing key does not preserve the
destination's CreateTime consistently across providers, so Copy no
longer tries to preserve it; it now only deep-copies the source's
attributes and metadata so the two entries are independent.

The memblob-specific tests are replaced by a drivertest subtest under
testCopy that makes a blob, copies it, mutates the source's attributes
through the driver, and verifies the copy is unchanged. It fails for
memblob without the fix and passes with it. It is commented out for
now because enabling it requires regenerating the record/replay files
for providers like S3 and GCS.
In both writer.Close() and writerWithSidecar.Close(), when the IfNotExist
precondition is triggered (a blob already exists at the target path),
gcerr.New() was called with err==nil as the underlying error. This is
because os.Stat() succeeds (err=nil) when the file exists, and that nil
value was passed directly to gcerr.New().

Fix by constructing a descriptive error using fmt.Errorf so that the error
chain carries meaningful context about which blob triggered the failure.
Also fix the message typo 'File already exist' -> 'IfNotExist precondition
failed', consistent with how memblob reports this condition.
Comment thread blob/memblob/memblob.go

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.

This seems unrelated?

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

Please rebase to HEAD and force-push, I think that's where the unrelated changes are coming from.

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.

2 participants