From 117cbd6590729441ab865ef819f7b5a9f1ffbdc1 Mon Sep 17 00:00:00 2001
From: Seebs <seebs@molecula.com>
Date: Thu, 16 Mar 2023 11:58:51 -0500
Subject: [PATCH] retry thing that depends on something asynchronous

There's no actual way to forcibly sequence our check of the history
until after the history has been updated, but it's pretty fast usually,
just not always instantaneous. Without this, adding a few millisecond
delay in the tracker reliably produces the test failures we kept seeing
with an unexpectedly low length of 3. With this, it passes consistently
even with the artificial delay.

The query tracker being asynchronous is probably fine, but we need
to test it as though it might take a while.
---
 server/handler_test.go | 45 ++++++++++++++++++++++++++++++------------
 1 file changed, 32 insertions(+), 13 deletions(-)

diff --git a/server/handler_test.go b/server/handler_test.go
index fd6e48b37..19d338f3c 100644
--- a/server/handler_test.go
+++ b/server/handler_test.go
@@ -1479,21 +1479,40 @@ func TestQueryHistory(t *testing.T) {
 	test.Do(t, "POST", cmd.URL()+"/index/i0/query", "Set(3000000, f0=0)")
 	test.Do(t, "POST", cmd.URL()+"/index/i0/query", "TopN(f0)")
 
-	h.ServeHTTP(w, test.MustNewHTTPRequest("GET", "/query-history", nil))
-	if w.Code != http.StatusOK {
-		t.Fatalf("unexpected status code: %d %s", w.Code, w.Body.String())
-	}
-
+	tries := 0
 	ret := make([]pilosa.PastQueryStatus, 4)
-	b, err := io.ReadAll(w.Body)
-	if err != nil {
-		t.Fatalf("reading: %v", err)
-	}
-	err = json.Unmarshal(b, &ret)
-	if err != nil {
-		t.Fatalf("unmarshalling: %v", err)
-	}
+	// We retry this looking for a length of 4. Why? Because the update of
+	// history is actually asynchronous in two ways. First, we're using a
+	// buffered channel for the status updates, so we can have finished
+	// writing to the channel before the tracker picks up the message.
+	// Second, after the tracker gets the message, it has to record it
+	// in a locked data structure. That could take non-zero time, but
+	// we're checking right away. In practice, that means that if we're
+	// slow, such as if we're running `go test -race`, we might miss
+	// it. So we retry until we have four items, or give up after a
+	// few milliseconds.
+	for tries < 3 {
+		h.ServeHTTP(w, test.MustNewHTTPRequest("GET", "/query-history", nil))
+		if w.Code != http.StatusOK {
+			t.Fatalf("unexpected status code: %d %s", w.Code, w.Body.String())
+		}
 
+		b, err := io.ReadAll(w.Body)
+		if err != nil {
+			t.Fatalf("reading: %v", err)
+		}
+		err = json.Unmarshal(b, &ret)
+		if err != nil {
+			t.Fatalf("unmarshalling: %v", err)
+		}
+
+		// verify result length
+		if len(ret) == 4 {
+			break
+		}
+		time.Sleep(2 * time.Millisecond)
+		tries++
+	}
 	// verify result length
 	if len(ret) != 4 {
 		// each set query executes on both nodes once