Remote Caching: Unhandled HTTP Exception & Client Timeout Feature Request

kathleenfrench commented 2 years ago

What version of Turborepo are you using?

1.5.1

What package manager are you using / does the bug impact?

pnpm

What operating system are you using?

Mac

Describe the Bug

While implementing a remote cache service, we were running into issues with some cache objects never being uploaded despite the logs reporting a successful write. Server-side, however, no http requests were coming through.

I recompiled turbo locally after adding some additional logging here (as reflected in the RESPONSE STATUS output above)

Currently, the only exception handling is for 403 errors

    resp, err := c.HttpClient.Do(req)
    if err != nil {
        return fmt.Errorf("failed to store files in HTTP cache: %w", err)
    }
    defer func() { _ = resp.Body.Close() }()
    if resp.StatusCode == http.StatusForbidden {
        return c.handle403(resp.Body)
    }

It would be great if non 200 status codes could likewise be handled/reported in the logs. The 413 issue we could resolve internally, as that was just an nginx ingress configuration, but the only way we were able to successfully leverage remote caching for larger cache objects was by changing the client timeout here.

    client := &ApiClient{
        baseUrl:      remoteConfig.APIURL,
        turboVersion: turboVersion,
        HttpClient: &retryablehttp.Client{
            HTTPClient: &http.Client{
                Timeout: time.Duration(customTimeout * time.Second),
            },
                         ...
        },

This could be made available as part of an official release in one of two ways - either providing the flexibility to set a client timeout (perhaps by environment variable or flag) which would fall back to the default of 20 if it's not set or ensuring no http retry is initiated if the client connection hasn't been closed from the server/an upload is actively in progress. The former would likely be the most straightforward approach for this particular edge case.

I am happy to open a pull request for whatever implementation is preferred if you would be open to adding this feature!

Expected Behavior

http errors would be logged and reported by the command line when there are failures uploading to the remote cache
writing to cache... would not appear after failed writes
success would not be the final status if there were any unsuccessful writes
pending requests would not be suspended in favor of a retry and/or client timeouts could be modified so as to prevent retries that suspend uploads prior to completion

To Reproduce

This is currently reproducible if you're working with especially large cache outputs (💫 monorepos), but could also potentially be reproduced by synthetically throttling your network connection to simulate a timeout during a remote cache write.

You can add additional logging to stdout in cli/internal/client/client.go as further confirmation:

    resp, err := c.HttpClient.Do(req)
    if err != nil {
        return fmt.Errorf("failed to store files in HTTP cache: %w", err)
    }

    defer func() { _ = resp.Body.Close() }()
    if resp.StatusCode == http.StatusForbidden {
        return c.handle403(resp.Body)
    }

        if resp.StatusCode != http.StatusOK {
           fmt.Println("RESPONSE STATUS: ", resp.Status, " URL ", requestURL)
        }

As we're running an internal remote cache service, we could also confirm server-side no PUT requests were coming through for artifacts reported by the turbo cli to have successfully written by referencing the server logs, and that this was only resolved by recompiling the cli with an updated client timeout to accommodate those larger cache objects.

nandorojo commented 7 months ago

I'm getting the same issue building my Next.js app. It just says payload too large. Any ideas? Thanks!

macrozone commented 3 months ago

I'm getting the same issue building my Next.js app. It just says payload too large. Any ideas? Thanks!

@nandorojo i created another issue for that https://github.com/vercel/turbo/issues/8772

vercel / turborepo