Add a test to verify that the user directory is properly updated after a lazy load join finishes syncing

[H-Shay]

As the title states.

[H-Shay]

In the interest of expediency I stuck a sleep here. I attempted on lines 3363-3374 (I commented out the attempt in case it's helpful to see) to use client.WithRetryUntil to loop until the condition was met, but I could not figure out how to make that function work with a request body, which is necessary for this API (I think?). It worked for the first iteration but then complained about an empty body for the next iteration, my best guess is that the request body is being consumed on the first attempt? I tried a few different formulations to feed the request body in but failed, any insight here would be appreciated.

[squahtx]

the request body is being consumed on the first attempt

That looks like what is happening. req.Body is set to an io.Reader containing the JSON. At the end of the first request, the read position lies at the end of the JSON, and so the retry reads 0 bytes.

There's a GetBody field that can be set on http.Request. If we modify WithRawBody, we can get the retries to work:

diff --git a/internal/client/client.go b/internal/client/client.go
index 7ffacff..05dc056 100644
--- a/internal/client/client.go
+++ b/internal/client/client.go
@@ -522,6 +522,10 @@ func (c *CSAPI) GetDefaultRoomVersion(t *testing.T) gomatrixserverlib.RoomVersio
 // WithRawBody sets the HTTP request body to `body`
 func WithRawBody(body []byte) RequestOpt {
        return func(req *http.Request) {
-               req.Body = ioutil.NopCloser(bytes.NewBuffer(body))
+               req.Body = ioutil.NopCloser(bytes.NewReader(body))
+               req.GetBody = func() (io.ReadCloser, error) {
+                       r := bytes.NewReader(body)
+                       return ioutil.NopCloser(r), nil
+               }
                // we need to manually set this because we don't set the body
                // in http.NewRequest due to using functional options, and only in NewRequest
                // does the stdlib set this for us.

We replace NewBuffer with NewReader, because the docs say

NewBuffer creates and initializes a new Buffer using buf as its initial contents. The new Buffer takes ownership of buf, and the caller should not use buf after this call.

[H-Shay]

I put this change in this PR, let me know if it should be in a separate PR instead.

[H-Shay]

Related: matrix-org/synapse#12815

[squahtx]

the request body is being consumed on the first attempt

That looks like what is happening. req.Body is set to an io.Reader containing the JSON. At the end of the first request, the read position lies at the end of the JSON, and so the retry reads 0 bytes.

There's a GetBody field that can be set on http.Request. If we modify WithRawBody, we can get the retries to work:

diff --git a/internal/client/client.go b/internal/client/client.go
index 7ffacff..05dc056 100644
--- a/internal/client/client.go
+++ b/internal/client/client.go
@@ -522,6 +522,10 @@ func (c *CSAPI) GetDefaultRoomVersion(t *testing.T) gomatrixserverlib.RoomVersio
 // WithRawBody sets the HTTP request body to `body`
 func WithRawBody(body []byte) RequestOpt {
        return func(req *http.Request) {
-               req.Body = ioutil.NopCloser(bytes.NewBuffer(body))
+               req.Body = ioutil.NopCloser(bytes.NewReader(body))
+               req.GetBody = func() (io.ReadCloser, error) {
+                       r := bytes.NewReader(body)
+                       return ioutil.NopCloser(r), nil
+               }
                // we need to manually set this because we don't set the body
                // in http.NewRequest due to using functional options, and only in NewRequest
                // does the stdlib set this for us.

We replace NewBuffer with NewReader, because the docs say

NewBuffer creates and initializes a new Buffer using buf as its initial contents. The new Buffer takes ownership of buf, and the caller should not use buf after this call.

[squahtx]

I forgot to mention: this passed when I ran it locally with workers, modulo the missing Notifier.notify_replication call of course.

[H-Shay]

My editor insisted on this change, I can edit the file here if it's a problem.

[squahtx]

Can we revert this change please?

[squahtx]

Looks good!

If you're feeling up for it, you could try factoring out the common code from the 3 checks into an assertUserDirectoryContains local function. createJoinEvent has an example of the syntax.

[DMRobertson]

If you're feeling up for it, you could try factoring out the common code from the 3 checks into an assertUserDirectoryContains local function. createJoinEvent has an example of the syntax.

In the interests of getting faster joins out there I'm going to merge this as-is; though by all means do feel free to factor this out.