Nodejs-storage: Spurious precondition failures caused by ifGenerationMatch preserved across save() calls

[paulmelnikow]

Blocked

• [ ] https://github.com/googleapis/gcs-resumable-upload/pull/243
• [ ] https://github.com/googleapis/gcs-resumable-upload/pull/244

---

I'm using file('example', { generation: 0 }).save() and discovered a strange behavior. I've managed to get resumable saves stuck in my config store, and as a result, when I subsequently invoke file('example').save(), the ifGenerationMatch header is still present on the response.

This is caused by the resumable upload code, which is turned on by default. I'm not sure whether the bug is here or in that library.

Steps to reproduce:

const bucket = new Storage({ projectId }).bucket(bucketName)

await bucket.file('example').save('hello', { resumable: false })

try {
  await bucket.file('example', { generation: 0 }).save('hello')
} catch (e) {
  // Catch a precondition failure
}

// Saves a new version as expected.
await bucket.file('example').save('hello', { resumable: false })

// Expected to save a new version, but throws precondition failure
await bucket.file('example').save('hello')

[stephenplusplus]

This seems like it could be an annoying side effect of gcs-resumable-upload. Most of the time, it should just work-- after a file is saved successfully, the config file is trashed. If something goes wrong, it keeps the config file around, so that the upload can be restarted, which is really the only purpose of that library.

So I would guess that for some reason, the first resumable upload attempt appeared to fail in the eyes of gcs-resumable-upload, even though it was saved successfully on the server. The resumable config file references saved URLs by bucket name and file name (source), but I suppose it could add the generation as well if one is provided, i.e. bucket/file/generation. That should work for this case, since if you don't provide a generation, it wouldn't find a match, and would start a new upload.

Thanks for reporting!

[paulmelnikow]

So I would guess that for some reason, the first resumable upload attempt appeared to fail in the eyes of gcs-resumable-upload, even though it was saved successfully on the server.

This is close. The first resumable upload attempt failed appropriately because the precondition failed. This is effectively permanent, however it appeared to gcs-resumable-upload as recoverable.

The resumable config file references saved URLs by bucket name and file name (source),

True for the “key.” However the value saves the full url with query string. That includes the generation. Effectively that means the generation is memoized, as in that example.

but I suppose it could add the generation as well if one is provided, i.e. bucket/file/generation. That should work for this case, since if you don't provide a generation, it wouldn't find a match, and would start a new upload.

Agreed!

What kind of error gcs-resumable-upload intending to patch over in practice? Are they mostly 5xx, and things like timeouts?

If so, another possibility would be to make the behavior more conservative, by considering 4xx errors “possibly permanent", and in those cases removing them from the cache.

[fhinkel]

Ping @stephenplusplus any updates on this?

[jkwlui]

gentle ping @stephenplusplus

[jiren]

await bucket.file('example').save('hello', { resumable: false })

This will correctly upload and after when trying with zero generation number it will fail as per doc

When a match precondition uses the value 0 instead of a generation number, the request only succeeds if there are no live objects in the Cloud Storage bucket with the name specified in the request. If there is such an object, the request fails and a 412 Precondition Failed response is returned.

If required to upload with generation number value then use the same generation number by fetching object metadata. If the generation number does not match then upload will fail and it will create an entry in configstore due to the default behavior of upload is resumable upload.

One more thing,

In upload, a default value of resumable is true, so every time start upload with resumable true storage lib create an entry in ~/.config/configstore/gcs-resumable-upload.json and on upload complete entry will be removed from the configstore file.

So once resumable upload with generation number fails, next time it will pickup the same resumable URL from configstore even if user has not passed any generation number.

Here is the sample entry from configstore.

{
  "node-gzip-test/test/14": {
    "txt": {
      "uri": "https://www.googleapis.com/upload/storage/v1/b/node-gzip-test/o?name=test%2F14.txt&uploadType=resumable&ifGenerationMatch=1560869372383619&upload_id=A8_OV8hAiWoEFNJRIRteCycguZ8fgqPjrM.........",
      "firstChunk": {
        "type": "Buffer",
        "data": [
          104,
          101,
          108
        ]
      }
    }
  },

Solution is that key("node-gzip-test/test/14") of the configstore have all the options with file extension and query params while uploading
i.e node-gzip-test/test/14 to node-gzip-test/test/14.txt?uploadType=resumable

Other solution is that helper api function to clear cache.

bucket.file('example').cleanupResumableCache();

As per current implementation if the same file name with different extension impact upload if one of the file upload fails.
i.e
Failure of test/14.txt file will create error for test/14.jpg

[stephenplusplus]

I have sent a PR: https://github.com/googleapis/gcs-resumable-upload/pull/243

[stephenplusplus]

And a PR to expose uploadInstance.deleteConfig(): https://github.com/googleapis/gcs-resumable-upload/pull/244. This will have to be re-exposed through the Storage API after it ships in gcs-resumable-upload.