[GODRIVER-3104] Fix Bucket.Delete/DeleteContext returning ErrFileNotFound when chunks exist Created: 24/Jan/24  Updated: 07/Feb/24

Status: Waiting for Reporter
Project: Go Driver
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Improvement Priority: Minor - P4
Reporter: Ed Pelc Assignee: Preston Vasquez
Resolution: Unresolved Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Assigned Teams:
Go Drivers

 Description   

Context

Bucket.Delete and DeleteContext return an ErrFileNotFound error when the file document does not exist but chunks may have been deleted.

 

It should check the chunk error and only return ErrFileNotFound if no file or chunk documents were found.

 

Why do this?

After a failed upload you can have chunks with no file document in mongodb using gridfs. The Delete methods should be able to clean this up without erroring. Also returning an error when Delete did in fact delete/fully cleanup a file doesn't make any sense.

Definition of done

What must be done to consider the task complete?

 

A gridfs upload can fail and leave chunks in the chunks collection without a corresponding file document. If you call Delete or DeleteContext using the file id of these chunks then an error is returned that the file doesn't exist. It should return nil instead when these existing chunks are cleaned up. ErrFileNotFound should only be returned if both the file document and chunks do not exist.

Pitfalls

Existing code could potentially be relying on this and require a version bump. However I believe it's unlikely as most code probably ignores the error from delete when used for cleanup after a failed file upload.

 

See https://github.com/mongodb/mongo-go-driver/blob/v1/mongo/gridfs/bucket.go#L277

 

The chunks error is ignored. This should be checked and if nil/deleted chunk documents and the file error is ErrFileNotFound then no error should be returned.



 Comments   
Comment by PM Bot [ 07/Feb/24 ]

Hi epelc@greatcloak.com! GODRIVER-3104 is awaiting your response.

If this is still an issue for you, please open Jira to review the latest status and provide your feedback. Thanks!

Comment by Preston Vasquez [ 31/Jan/24 ]

Hey epelc@greatcloak.com, thanks for reaching out. ErrFileNotFound serves as a sentinel (io.EOF, for example) that signals to the user that a gridfs operation occurred without corresponding files. The occurrence of this SE indicates a potential inconsistency in the data storage, i.e. the absence of corresponding files might suggest a problem in the data structure or storage process (like losing a connection mid upsert). At the very least, this may be something a user needs to log. For this reason, removing the error in v1 would be backwards breaking, as you note.

err := b.Delete(ctx, fileID)
if errors.Is(err, gridfs.ErrFileNotFound) {
    log.Infof("orphaned chunks for filedID %q deleted", fileID)
} else if err != nil {
    return fmt.Errorf("failed to delete: %w", err)
}

SERVER-163 notes that there is no safe way to avoid this problem server-side.

Comment by Ed Pelc [ 24/Jan/24 ]

Sorry for the poor formatting. I submitted before finishing editing.

"A gridfs upload can fail and leave chunks in the chunks collection without a corresponding file document. If you call Delete or DeleteContext using the file id of these chunks then an error is returned that the file doesn't exist. It should return nil instead when these existing chunks are cleaned up. ErrFileNotFound should only be returned if both the file document and chunks do not exist."

should be at the top and is the main synopsis. It seems Jira doesn't let you edit the description.

Comment by PM Bot [ 24/Jan/24 ]

Hi epelc@greatcloak.com, thank you for reporting this issue! The team will look into it and get back to you soon.

Generated at Thu Feb 08 08:40:04 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.