From d3d041094ef88da50330da0cbfb94ea270d380ea Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Thu, 9 Jan 2025 09:18:21 +0000 Subject: [PATCH 1/3] test(cli/ssh): sync post-build to fix build race on slow runners Fixes coder/internal#269 --- cli/ssh_test.go | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/cli/ssh_test.go b/cli/ssh_test.go index bd107852251f7..be36a159c5cd6 100644 --- a/cli/ssh_test.go +++ b/cli/ssh_test.go @@ -154,16 +154,18 @@ func TestSSH(t *testing.T) { // a start build of the workspace. isFirstBuild := true buildURL := regexp.MustCompile("/api/v2/workspaces/.*/builds") - buildReq := make(chan struct{}) + buildSync := make(chan struct{}) buildResume := make(chan struct{}) buildSyncMW := func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.Method == http.MethodPost && buildURL.MatchString(r.URL.Path) { if !isFirstBuild { - t.Log("buildSyncMW: blocking build") - buildReq <- struct{}{} - <-buildResume - t.Log("buildSyncMW: resuming build") + defer func() { + t.Log("buildSyncMW: blocking post-build") + buildSync <- struct{}{} + <-buildResume + t.Log("buildSyncMW: resuming...") + }() } else { isFirstBuild = false } @@ -212,7 +214,7 @@ func TestSSH(t *testing.T) { pty.ExpectMatchContext(ctx, "Workspace was stopped, starting workspace to allow connecting to") } for range ptys { - testutil.RequireRecvCtx(ctx, t, buildReq) + testutil.RequireRecvCtx(ctx, t, buildSync) } close(buildResume) From 979951ec706ec39b2ad5ba55bfbe30edcb4b464f Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Thu, 9 Jan 2025 09:49:26 +0000 Subject: [PATCH 2/3] improve synchro --- cli/ssh_test.go | 33 ++++++++++++++++++++++++--------- 1 file changed, 24 insertions(+), 9 deletions(-) diff --git a/cli/ssh_test.go b/cli/ssh_test.go index be36a159c5cd6..5b616c4f00b30 100644 --- a/cli/ssh_test.go +++ b/cli/ssh_test.go @@ -154,17 +154,22 @@ func TestSSH(t *testing.T) { // a start build of the workspace. isFirstBuild := true buildURL := regexp.MustCompile("/api/v2/workspaces/.*/builds") - buildSync := make(chan struct{}) - buildResume := make(chan struct{}) + buildPause := make(chan struct{}) + buildDone := make(chan struct{}) + buildReturnSync := make(chan struct{}) buildSyncMW := func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.Method == http.MethodPost && buildURL.MatchString(r.URL.Path) { if !isFirstBuild { + t.Log("buildSyncMW: pausing build") + <-buildPause + t.Log("buildSyncMW: resuming build") defer func() { - t.Log("buildSyncMW: blocking post-build") - buildSync <- struct{}{} - <-buildResume - t.Log("buildSyncMW: resuming...") + t.Log("buildSyncMW: sending build done") + buildDone <- struct{}{} + t.Log("buildSyncMW: waiting for return sync") + <-buildReturnSync + t.Log("buildSyncMW: returning") }() } else { isFirstBuild = false @@ -213,10 +218,20 @@ func TestSSH(t *testing.T) { for _, pty := range ptys { pty.ExpectMatchContext(ctx, "Workspace was stopped, starting workspace to allow connecting to") } - for range ptys { - testutil.RequireRecvCtx(ctx, t, buildSync) + + // Allow one build to complete. + testutil.RequireSendCtx(ctx, t, buildPause, struct{}{}) + testutil.RequireRecvCtx(ctx, t, buildDone) + + // Allow the remaining builds to continue. + for i := 0; i < len(ptys)-1; i++ { + testutil.RequireSendCtx(ctx, t, buildPause, struct{}{}) + } + for i := 0; i < len(ptys)-1; i++ { + testutil.RequireRecvCtx(ctx, t, buildDone) } - close(buildResume) + // Allow all three endpoints to return. + close(buildReturnSync) var foundConflict int for _, pty := range ptys { From 92c651b6ca813b34860d90ab9a7810c62a497f3b Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Fri, 10 Jan 2025 12:04:41 +0000 Subject: [PATCH 3/3] remove build race by faking conflict @ api --- cli/ssh_test.go | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/cli/ssh_test.go b/cli/ssh_test.go index 5b616c4f00b30..4fd52971df1cf 100644 --- a/cli/ssh_test.go +++ b/cli/ssh_test.go @@ -154,22 +154,24 @@ func TestSSH(t *testing.T) { // a start build of the workspace. isFirstBuild := true buildURL := regexp.MustCompile("/api/v2/workspaces/.*/builds") - buildPause := make(chan struct{}) + buildPause := make(chan bool) buildDone := make(chan struct{}) - buildReturnSync := make(chan struct{}) buildSyncMW := func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.Method == http.MethodPost && buildURL.MatchString(r.URL.Path) { if !isFirstBuild { t.Log("buildSyncMW: pausing build") - <-buildPause + if shouldContinue := <-buildPause; !shouldContinue { + // We can't force the API to trigger a build conflict (racy) so we fake it. + t.Log("buildSyncMW: return conflict") + w.WriteHeader(http.StatusConflict) + return + } t.Log("buildSyncMW: resuming build") defer func() { t.Log("buildSyncMW: sending build done") buildDone <- struct{}{} - t.Log("buildSyncMW: waiting for return sync") - <-buildReturnSync - t.Log("buildSyncMW: returning") + t.Log("buildSyncMW: done") }() } else { isFirstBuild = false @@ -220,18 +222,13 @@ func TestSSH(t *testing.T) { } // Allow one build to complete. - testutil.RequireSendCtx(ctx, t, buildPause, struct{}{}) + testutil.RequireSendCtx(ctx, t, buildPause, true) testutil.RequireRecvCtx(ctx, t, buildDone) // Allow the remaining builds to continue. for i := 0; i < len(ptys)-1; i++ { - testutil.RequireSendCtx(ctx, t, buildPause, struct{}{}) - } - for i := 0; i < len(ptys)-1; i++ { - testutil.RequireRecvCtx(ctx, t, buildDone) + testutil.RequireSendCtx(ctx, t, buildPause, false) } - // Allow all three endpoints to return. - close(buildReturnSync) var foundConflict int for _, pty := range ptys {