fix(gateway): handle streaming errors, IPIP 332

[hacdias]

While working on #9029, I noticed the Trailer HTTP header was missing on CAR responses. If there was an error, the client would likely miss it. curl, for example, would not show the trailing header on the output.

Docs: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Trailer

Update: see #9333 (comment)

[lidel]

I believe we did not include them here on purpose – because of this bug in Nodejs: nodejs/undici#432 (comment)

Better late than never, Node folks realized the mess, and it seems to be resolved now (nodejs/undici#1418), but older versions like Node 18.0.0 or Node 16.14.2 with Undici 5.1.1 will be impacted. Probably better for us to play safe here.

@hacdias are you able to identify Node versions with the fix?
Did the Undici fix landed in LTS?

ps. Trailers are not supported by browsers, the docs you've linked are invalid: mdn/browser-compat-data#14703 🙈

There are too many issues with Trailers: for these reasons, we did not include it the gateway specs at all. It is up to the client to verify the received CAR stream anyway, so client has means of detecting errors and retry if any blocks from the requested DAG are missing. We could merge this in Kubo tho, as long it does not cause issues with Nodejs used in the wild.

[hacdias]

@lidel no, this fix does not check for Node.js versions.

I also want to notice that by not setting the Trailer header, tools that would otherwise support them (curl), do not show the trailer headers, even if they're emitted.

[lidel]

So.. this header only impacts non-browser contexts and my understanding is that no matter what we do here it is always breaking something:

If we have to choose between (A) not showing a header in curl (cosmetic), and (B) making it impossible to read valid responses with NodeJS version ranges using undici library with nodejs/undici#432 (comment) bug, I'd rather not break Nodejs clients, as we would have to triage these avoidable bugs 🙈

Spec-wise, I am against this header in undocumented form

iiuc X-Stream-Error is a proprietary, undocumented header, it was invented in https://github.com/ipfs/go-ipfs-cmds/ for Kubo RPC and is not part of gateway specs.

JS in Browser can't access these headers anyway – we need to double-check what is the value here: is it worth leaking RPC conventions into Gateway if it brings no value to Web use, and causes issues outside of it?

Next steps

I suggest we fully remove these undocumented Trailer headers from the gateway use, and only introduce error-handling that works in web browser (e.g. force-close the connection?)

If we feel there is value or functional here, we should create a spec IPIP that clarifies error handling on gateways, and not ship undocumented stuff.

[hacdias]

@lidel @Jorropo I updated this PR to force close the connections when there is a streaming error (CARs, Blocks, UnixFS). If this gets merged before #9029, I can reuse the function there.

[hacdias]

@lidel I opened an IPIP here: ipfs/specs#332 I think it's important to be clear on how to handle this situations. I don't expect them to happen frequently, but it's still important to be aware of them.

We can discuss if must vs. should and/or recommended vs. required.

[Jorropo]

I'll review once the spec is merged.

But I think a header to optout of this behaviour might be nice. This has a real performance cost and will require clients to reopen a new connection (since we don't support HTTP2 there due to the lack of TLS cert).
If your client as able to detect errors in car files or support trailing headers, you would probably like thoses instead of reopenning the connection.

[lidel]

Closing due to reasons explained in ipfs/specs#332 (review)