internal/frontend/search: don't return 500 on timeout

If searches time out, return 408 (request timed out) instead of 500. We previously did this for symbol searches, but sometimes package searches can also time out. Fixes #64422. Change-Id: I423845e0663866f9743e19ed768f68e6abde6250 Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/638596 LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Hyang-Ah Hana Kim <[email protected]> kokoro-CI: kokoro <[email protected]>
golang · Dec 26, 2024 · 8bad909 · 8bad909
1 parent 06c6edf
commit 8bad909
Show file tree

Hide file tree

Showing 3 changed files with 20 additions and 6 deletions.
diff --git a/internal/frontend/fetchserver/server_test.go b/internal/frontend/fetchserver/server_test.go
@@ -30,6 +30,8 @@ type serverTestCase struct {
 	name string
 	// path to use in an HTTP GET request
 	urlPath string
+	// make the request time out
+	timeOut bool
 	// statusCode we expect to see in the headers.
 	wantStatusCode int
 	// if non-empty, contents of Location header. For testing redirects.
@@ -1056,6 +1058,12 @@ var searchGroupingTestCases = []serverTestCase{
 		wantStatusCode: http.StatusOK,
 		want:           notIn(".Pagination-nav"),
 	},
+	{
+		name:           "search timeout",
+		urlPath:        "/search?q=-http",
+		timeOut:        true,
+		wantStatusCode: http.StatusRequestTimeout,
+	},
 }
 
 // TestServer checks the contents of served pages by looking for
@@ -1103,7 +1111,13 @@ func testServer(t *testing.T, testCases []serverTestCase) {
 	for _, test := range testCases {
 		t.Run(test.name, func(t *testing.T) { // remove initial '/' for name
 			w := httptest.NewRecorder()
-			handler.ServeHTTP(w, httptest.NewRequest("GET", test.urlPath, nil))
+			rctx := ctx
+			if test.timeOut {
+				var cancel func()
+				rctx, cancel = context.WithTimeout(ctx, time.Microsecond)
+				defer cancel()
+			}
+			handler.ServeHTTP(w, httptest.NewRequestWithContext(rctx, "GET", test.urlPath, nil))
 			res := w.Result()
 			if res.StatusCode != test.wantStatusCode {
 				t.Fatalf("GET %q = %d, want %d", test.urlPath, res.StatusCode, test.wantStatusCode)

diff --git a/internal/frontend/search.go b/internal/frontend/search.go
@@ -138,14 +138,14 @@ func determineSearchAction(r *http.Request, ds internal.DataSource, vulnClient *
 	}
 	page, err := fetchSearchPage(ctx, ds, cq, symbol, pageParams, mode == searchModeSymbol, vulnClient)
 	if err != nil {
-		// Instead of returning a 500, return a 408, since symbol searches may
-		// timeout for very popular symbols.
-		if mode == searchModeSymbol && strings.Contains(err.Error(), "i/o timeout") {
+		// Instead of returning a 500, return a 408, since symbol searches may time
+		// out for very popular symbols, and package searches can also time out.
+		if errors.Is(err, context.DeadlineExceeded) || strings.Contains(err.Error(), "i/o timeout") {
 			return nil, &serrors.ServerError{
 				Status: http.StatusRequestTimeout,
 				Epage: &pagepkg.ErrorPage{
 					MessageTemplate: template.MakeTrustedTemplate(
-						`<h3 class="Error-message">Request timed out. Please try again!</h3>`),
+						`<h3 class="Error-message">Request timed out.</h3>`),
 				},
 			}
 		}

diff --git a/internal/postgres/search.go b/internal/postgres/search.go
@@ -243,7 +243,7 @@ func (db *DB) hedgedSearch(ctx context.Context, q string, limit int, opts Search
 	// error ratio. That didn't behave well if Postgres was overloaded.
 	resp := <-responses
 	if resp.err != nil {
-		return nil, fmt.Errorf("%q search failed: %v", resp.source, resp.err)
+		return nil, fmt.Errorf("%q search failed: %w", resp.source, resp.err)
 	}
 	// cancel proactively here: we've got the search result we need.
 	cancel()