Skip to content

Commit 40bb601

Browse files
committed
improved timeouts handling
1 parent 2027d78 commit 40bb601

9 files changed

Lines changed: 255 additions & 37 deletions

File tree

CHANGELOG.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,9 @@ All notable changes to `src-cli` are documented in this file.
1515

1616
### Changed
1717

18+
- HTTP requests now fail instead of hanging forever if the server does not start responding within 5 minutes. Set the `SRC_RESPONSE_HEADER_TIMEOUT` environment variable to change this timeout, or to `0` to disable it. Responses that stream data for a long time (for example, large search job results) are not affected.
19+
- `src search-jobs logs` and `src search-jobs results` now use the standard API client, gaining proxy support, `-insecure-skip-verify`, and cross-host redirect protection, and now report an error on non-200 responses instead of writing the error page into the output.
20+
1821
### Removed
1922

2023
- Removed `src sbom` and `src signature` commands. SBOMs and container signatures are no longer published as of Sourcegraph 7.1.0.

README.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -197,6 +197,10 @@ mv /usr/local/bin/src /usr/local/bin/src-cli
197197

198198
You can then invoke it via `src-cli`.
199199

200+
## Timeouts
201+
202+
`src` waits up to 5 minutes for the server to start responding to a request. To change this, set the `SRC_RESPONSE_HEADER_TIMEOUT` environment variable to a duration such as `30s` or `10m`, or to `0` to disable the timeout. This timeout only applies until the server sends its response headers — responses that stream data for a long time, such as large search job results, are not interrupted.
203+
200204
## Telemetry
201205

202206
`src` includes the operating system and architecture in the `User-Agent` header sent to Sourcegraph. For example, running `src` version 3.21.10 on an x86-64 Linux host will result in this header:

cmd/src/search_jobs.go

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@ import (
44
"encoding/json"
55
"flag"
66
"fmt"
7+
"io"
8+
"net/http"
79
"strings"
810

911
"github.com/sourcegraph/src-cli/internal/api"
@@ -167,6 +169,31 @@ func parseSearchJobsArgs(flagSet *flag.FlagSet, args []string) error {
167169
return nil
168170
}
169171

172+
// fetchSearchJobFile downloads a file (logs or results) belonging to a search
173+
// job. The request goes through the API client so that it picks up the
174+
// configured transport (timeouts, proxy, TLS and redirect handling).
175+
func fetchSearchJobFile(client api.Client, fileURL string) (io.ReadCloser, error) {
176+
req, err := http.NewRequest("GET", fileURL, nil)
177+
if err != nil {
178+
return nil, err
179+
}
180+
181+
req.Header.Add("Authorization", "token "+cfg.accessToken)
182+
183+
resp, err := client.Do(req)
184+
if err != nil {
185+
return nil, err
186+
}
187+
188+
if resp.StatusCode != http.StatusOK {
189+
body, _ := io.ReadAll(io.LimitReader(resp.Body, 1024))
190+
resp.Body.Close()
191+
return nil, fmt.Errorf("error: %s\n\n%s", resp.Status, body)
192+
}
193+
194+
return resp.Body, nil
195+
}
196+
170197
// validateJobID validates that a job ID was provided
171198
func validateJobID(args []string) (string, error) {
172199
if len(args) != 1 {

cmd/src/search_jobs_logs.go

Lines changed: 3 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -4,32 +4,19 @@ import (
44
"flag"
55
"fmt"
66
"io"
7-
"net/http"
87
"os"
98

109
"github.com/sourcegraph/src-cli/internal/api"
1110
"github.com/sourcegraph/src-cli/internal/cmderrors"
1211
)
1312

1413
// fetchJobLogs retrieves logs for a search job from its log URL
15-
func fetchJobLogs(jobID string, logURL string) (io.ReadCloser, error) {
14+
func fetchJobLogs(client api.Client, jobID string, logURL string) (io.ReadCloser, error) {
1615
if logURL == "" {
1716
return nil, fmt.Errorf("no logs URL found for search job %s", jobID)
1817
}
1918

20-
req, err := http.NewRequest("GET", logURL, nil)
21-
if err != nil {
22-
return nil, err
23-
}
24-
25-
req.Header.Add("Authorization", "token "+cfg.accessToken)
26-
27-
resp, err := http.DefaultClient.Do(req)
28-
if err != nil {
29-
return nil, err
30-
}
31-
32-
return resp.Body, nil
19+
return fetchSearchJobFile(client, logURL)
3320
}
3421

3522
func outputLogs(logs io.Reader, outputPath string) error {
@@ -88,7 +75,7 @@ func init() {
8875
return fmt.Errorf("no job found with ID %s", jobID)
8976
}
9077

91-
logsData, err := fetchJobLogs(jobID, job.LogURL)
78+
logsData, err := fetchJobLogs(client, jobID, job.LogURL)
9279
if err != nil {
9380
return err
9481
}

cmd/src/search_jobs_results.go

Lines changed: 3 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -4,32 +4,19 @@ import (
44
"flag"
55
"fmt"
66
"io"
7-
"net/http"
87
"os"
98

109
"github.com/sourcegraph/src-cli/internal/api"
1110
"github.com/sourcegraph/src-cli/internal/cmderrors"
1211
)
1312

1413
// fetchJobResults retrieves results for a search job from its results URL
15-
func fetchJobResults(jobID string, resultsURL string) (io.ReadCloser, error) {
14+
func fetchJobResults(client api.Client, jobID string, resultsURL string) (io.ReadCloser, error) {
1615
if resultsURL == "" {
1716
return nil, fmt.Errorf("no results URL found for search job %s", jobID)
1817
}
1918

20-
req, err := http.NewRequest("GET", resultsURL, nil)
21-
if err != nil {
22-
return nil, err
23-
}
24-
25-
req.Header.Add("Authorization", "token "+cfg.accessToken)
26-
27-
resp, err := http.DefaultClient.Do(req)
28-
if err != nil {
29-
return nil, err
30-
}
31-
32-
return resp.Body, nil
19+
return fetchSearchJobFile(client, resultsURL)
3320
}
3421

3522
// outputResults writes results to either a file or stdout
@@ -90,7 +77,7 @@ func init() {
9077
return fmt.Errorf("no job found with ID %s", jobID)
9178
}
9279

93-
resultsData, err := fetchJobResults(jobID, job.URL)
80+
resultsData, err := fetchJobResults(client, jobID, job.URL)
9481
if err != nil {
9582
return err
9683
}

cmd/src/search_jobs_test.go

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
package main
2+
3+
import (
4+
"io"
5+
"net/http"
6+
"net/http/httptest"
7+
"net/url"
8+
"strings"
9+
"testing"
10+
11+
"github.com/sourcegraph/src-cli/internal/api"
12+
)
13+
14+
func TestFetchSearchJobFile(t *testing.T) {
15+
var gotAuth string
16+
mux := http.NewServeMux()
17+
mux.HandleFunc("/ok", func(w http.ResponseWriter, r *http.Request) {
18+
gotAuth = r.Header.Get("Authorization")
19+
_, _ = w.Write([]byte("results data"))
20+
})
21+
mux.HandleFunc("/fail", func(w http.ResponseWriter, r *http.Request) {
22+
http.Error(w, "something went wrong", http.StatusInternalServerError)
23+
})
24+
server := httptest.NewServer(mux)
25+
defer server.Close()
26+
27+
endpointURL, err := url.Parse(server.URL)
28+
if err != nil {
29+
t.Fatal(err)
30+
}
31+
cfg = &config{
32+
endpointURL: endpointURL,
33+
accessToken: "test-token",
34+
}
35+
defer func() { cfg = nil }()
36+
37+
client := api.NewClient(api.ClientOpts{EndpointURL: endpointURL, Out: io.Discard})
38+
39+
t.Run("success", func(t *testing.T) {
40+
body, err := fetchSearchJobFile(client, server.URL+"/ok")
41+
if err != nil {
42+
t.Fatal(err)
43+
}
44+
defer body.Close()
45+
46+
data, err := io.ReadAll(body)
47+
if err != nil {
48+
t.Fatal(err)
49+
}
50+
if string(data) != "results data" {
51+
t.Fatalf("got body %q, want %q", data, "results data")
52+
}
53+
if gotAuth != "token test-token" {
54+
t.Fatalf("got Authorization header %q, want %q", gotAuth, "token test-token")
55+
}
56+
})
57+
58+
t.Run("non-200 response", func(t *testing.T) {
59+
_, err := fetchSearchJobFile(client, server.URL+"/fail")
60+
if err == nil {
61+
t.Fatal("expected an error for a non-200 response, got nil")
62+
}
63+
if !strings.Contains(err.Error(), "something went wrong") {
64+
t.Fatalf("error %q does not contain the response body", err)
65+
}
66+
})
67+
}

internal/api/api.go

Lines changed: 32 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import (
1212
"net/url"
1313
"os"
1414
"runtime"
15+
"time"
1516

1617
ioaux "github.com/jig/teereadcloser"
1718
"github.com/kballard/go-shellquote"
@@ -95,10 +96,36 @@ type ClientOpts struct {
9596
// ErrCIAccessTokenRequired indicates SRC_ACCESS_TOKEN must be set when CI=true.
9697
var ErrCIAccessTokenRequired = errors.New("SRC_ACCESS_TOKEN must be set when CI=true")
9798

99+
// defaultResponseHeaderTimeout bounds how long we wait for a server to start
100+
// responding. It is deliberately generous because some GraphQL queries take a
101+
// long time server-side before the first response byte is written.
102+
const defaultResponseHeaderTimeout = 5 * time.Minute
103+
104+
// responseHeaderTimeout returns the timeout to wait for a server's response
105+
// headers, honoring the SRC_RESPONSE_HEADER_TIMEOUT environment variable (a Go
106+
// duration string; "0" disables the timeout).
107+
func responseHeaderTimeout() time.Duration {
108+
if v := os.Getenv("SRC_RESPONSE_HEADER_TIMEOUT"); v != "" {
109+
if d, err := time.ParseDuration(v); err == nil && d >= 0 {
110+
return d
111+
}
112+
}
113+
return defaultResponseHeaderTimeout
114+
}
115+
116+
// BaseTransport returns a clone of http.DefaultTransport (which carries dial
117+
// and TLS handshake timeouts) with a response header timeout applied, so that
118+
// an unresponsive server cannot stall requests indefinitely.
119+
func BaseTransport() *http.Transport {
120+
tp := http.DefaultTransport.(*http.Transport).Clone()
121+
tp.ResponseHeaderTimeout = responseHeaderTimeout()
122+
return tp
123+
}
124+
98125
func buildTransport(opts ClientOpts, flags *Flags) http.RoundTripper {
99126
var transport http.RoundTripper
100127
{
101-
tp := http.DefaultTransport.(*http.Transport).Clone()
128+
tp := BaseTransport()
102129

103130
if flags.insecureSkipVerify != nil && *flags.insecureSkipVerify {
104131
tp.TLSClientConfig = &tls.Config{InsecureSkipVerify: true}
@@ -138,6 +165,10 @@ func NewClient(opts ClientOpts) Client {
138165

139166
transport := buildTransport(opts, flags)
140167

168+
// Note: no Client.Timeout is set on purpose. It would cap the entire
169+
// request including reading the response body, but downloads (search job
170+
// results, batch change archives, ...) may legitimately stream for a very
171+
// long time. The transport's connection-phase timeouts bound the rest.
141172
httpClient := &http.Client{
142173
Transport: transport,
143174
CheckRedirect: checkRedirect,

internal/api/timeout_test.go

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,107 @@
1+
package api
2+
3+
import (
4+
"context"
5+
"io"
6+
"net/http"
7+
"net/http/httptest"
8+
"net/url"
9+
"testing"
10+
"time"
11+
)
12+
13+
func newTestClient(t *testing.T, serverURL string) Client {
14+
t.Helper()
15+
endpointURL, err := url.Parse(serverURL)
16+
if err != nil {
17+
t.Fatal(err)
18+
}
19+
return NewClient(ClientOpts{EndpointURL: endpointURL, Out: io.Discard})
20+
}
21+
22+
func TestUnresponsiveServerTimesOut(t *testing.T) {
23+
t.Setenv("SRC_RESPONSE_HEADER_TIMEOUT", "100ms")
24+
25+
block := make(chan struct{})
26+
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
27+
<-block
28+
}))
29+
defer server.Close()
30+
defer close(block)
31+
32+
client := newTestClient(t, server.URL)
33+
req, err := client.NewHTTPRequest(context.Background(), http.MethodGet, "", nil)
34+
if err != nil {
35+
t.Fatal(err)
36+
}
37+
38+
start := time.Now()
39+
resp, err := client.Do(req)
40+
if err == nil {
41+
resp.Body.Close()
42+
t.Fatal("expected a timeout error, got nil")
43+
}
44+
if elapsed := time.Since(start); elapsed > 5*time.Second {
45+
t.Fatalf("request took %s, expected it to fail within the response header timeout", elapsed)
46+
}
47+
}
48+
49+
func TestSlowStreamingDownloadSucceeds(t *testing.T) {
50+
t.Setenv("SRC_RESPONSE_HEADER_TIMEOUT", "200ms")
51+
52+
const chunks = 5
53+
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
54+
flusher := w.(http.Flusher)
55+
w.WriteHeader(http.StatusOK)
56+
flusher.Flush()
57+
// Stream the body for much longer than the response header timeout.
58+
for range chunks {
59+
time.Sleep(100 * time.Millisecond)
60+
_, _ = w.Write([]byte("chunk"))
61+
flusher.Flush()
62+
}
63+
}))
64+
defer server.Close()
65+
66+
client := newTestClient(t, server.URL)
67+
req, err := client.NewHTTPRequest(context.Background(), http.MethodGet, "", nil)
68+
if err != nil {
69+
t.Fatal(err)
70+
}
71+
72+
resp, err := client.Do(req)
73+
if err != nil {
74+
t.Fatal(err)
75+
}
76+
defer resp.Body.Close()
77+
78+
body, err := io.ReadAll(resp.Body)
79+
if err != nil {
80+
t.Fatalf("reading a slow streaming body failed: %v", err)
81+
}
82+
if want := len("chunk") * chunks; len(body) != want {
83+
t.Fatalf("got %d body bytes, want %d", len(body), want)
84+
}
85+
}
86+
87+
func TestResponseHeaderTimeoutEnv(t *testing.T) {
88+
tests := []struct {
89+
value string
90+
want time.Duration
91+
}{
92+
{value: "", want: defaultResponseHeaderTimeout},
93+
{value: "30s", want: 30 * time.Second},
94+
{value: "0", want: 0},
95+
{value: "garbage", want: defaultResponseHeaderTimeout},
96+
{value: "-5s", want: defaultResponseHeaderTimeout},
97+
}
98+
99+
for _, test := range tests {
100+
t.Run(test.value, func(t *testing.T) {
101+
t.Setenv("SRC_RESPONSE_HEADER_TIMEOUT", test.value)
102+
if got := responseHeaderTimeout(); got != test.want {
103+
t.Fatalf("got %s, want %s", got, test.want)
104+
}
105+
})
106+
}
107+
}

0 commit comments

Comments
 (0)