Warning: This pull request is touching the following templated files:

• .github/workflows/ci.yaml - .github/workflows/ci.yaml (GitHub Actions) should be updated in synthtool

No region tags are edited in this PR.

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

• Refresh this comment

What's the schedule on this / anything that can be helped with? I'm facing various issues in the current gaxios / node-fetch versions that have been fixed later.

Forked this for testing, I'll update this comment in case I find anything.

• Is the change in file.delete intended? The request contains a double forward-slash: DELETE /storage/v1/b/$some_bucket//o/$some_object
• a similar change can be seen with file.setMetadata
• file.delete seems to hang as the callback is never invoked (see service-object.ts#L304)

Forked this for testing, I'll update this comment in case I find anything.

• Is the change in file.delete intended? The request contains a double forward-slash: DELETE /storage/v1/b/$some_bucket//o/$some_object
• a similar change can be seen with file.setMetadata
• file.delete seems to hang as the callback is never invoked (see service-object.ts#L304)

cc: @thiyaguk09

Forked this for testing, I'll update this comment in case I find anything.

• Is the change in file.delete intended? The request contains a double forward-slash: DELETE /storage/v1/b/$some_bucket//o/$some_object
• a similar change can be seen with file.setMetadata
• file.delete seems to hang as the callback is never invoked (see service-object.ts#L304)

Thanks for forking and testing! I appreciate you digging into that.

Regarding the double forward-slash in DELETE /storage/v1/b/$some_bucket//o/$some_object for file.delete and file.setMetadata, that's definitely not intended and sounds like a bug. The hanging callback on file.delete is also a critical issue.

I'm currently tied up with some urgent work, but I'll make sure to prioritize looking into these issues as soon as I can. Your detailed findings are really helpful!

Deleting files will still hang after the above commits, and the test doesn't catch it because it explicitly calls done from the mocked function. Check out the diff here: https://github.com/googleapis/nodejs-storage/pull/2596/files

I believe I've also found another regression in file.ts#L1702:

      this.storageTransport
        .makeRequest(reqOpts, async (err, stream, rawResponse) => {
          (stream as Readable).on('error', err => {
            throughStream.destroy(err);
          });

The cast here might be erroneous. Looking at Gaxios documentation, specifically GaxiosOptions.responseType:

    /**
     * If the `fetchImplementation` is native `fetch`, the
     * stream is a `ReadableStream`, otherwise `readable.Stream`
     */
    responseType?: 'arraybuffer' | 'blob' | 'json' | 'text' | 'stream' | 'unknown';

I'm not sure if an user can override the fetch implementation, but assuming they can't, the correct type is ReadableStream.

This results in the following error: Cannot read properties of null (reading 'on')

Thoughts?

@thiyaguk09 can you address the comments from @organize. Also, do you know why the docs CI check is 429'ing on a bunch of samples now?

can you address the comments from @organize. Also, do you know why the docs CI check is 429'ing on a bunch of samples now?

Sure, I’ll address the comments from @organize. Regarding the docs CI check returning 429 errors on several samples, I’ll investigate the cause—could be rate limiting or a recent config change. Will update shortly.

There still seems to be a regression in downloading objects, and using ReadableStream didn't help.

I believe I've also found another regression in file.ts#L1702:

      this.storageTransport
        .makeRequest(reqOpts, async (err, stream, rawResponse) => {
          (stream as Readable).on('error', err => {
            throughStream.destroy(err);
          });

Here, for some reason, stream is null intermittently. I'll try to track it down further.

Turns out I was missing storage.objects.get from some of my earlier testing. I'm pretty sure the IAM-related error is being swallowed now, with Cannot read properties of null (reading 'on') being thrown instead.