From ccea407eee93280776d2bc3372268e3a32759662 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 13 Jun 2023 13:32:17 +0000 Subject: [PATCH 1/4] test(coderd): Close metricscache and avoid background context --- coderd/coderdtest/coderdtest.go | 4 ++++ coderd/metricscache/metricscache_test.go | 1 + coderd/workspaceapps/apptest/setup.go | 2 +- enterprise/coderd/coderdenttest/coderdenttest.go | 5 +++-- 4 files changed, 9 insertions(+), 3 deletions(-) diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index f0c57552087cf..b6623fbd6f942 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -169,6 +169,8 @@ func newWithCloser(t testing.TB, options *Options) (*codersdk.Client, io.Closer) } func NewOptions(t testing.TB, options *Options) (func(http.Handler), context.CancelFunc, *url.URL, *coderd.Options) { + t.Helper() + if options == nil { options = &Options{} } @@ -402,6 +404,8 @@ func NewWithAPI(t testing.TB, options *Options) (*codersdk.Client, io.Closer, *c // well with coderd testing. It registers the "echo" provisioner for // quick testing. func NewProvisionerDaemon(t testing.TB, coderAPI *coderd.API) io.Closer { + t.Helper() + echoClient, echoServer := provisionersdk.MemTransportPipe() ctx, cancelFunc := context.WithCancel(context.Background()) t.Cleanup(func() { diff --git a/coderd/metricscache/metricscache_test.go b/coderd/metricscache/metricscache_test.go index b7d3aa9eaf957..5cbe9b10203e1 100644 --- a/coderd/metricscache/metricscache_test.go +++ b/coderd/metricscache/metricscache_test.go @@ -435,6 +435,7 @@ func TestCache_DeploymentStats(t *testing.T) { cache := metricscache.New(db, slogtest.Make(t, nil), metricscache.Intervals{ DeploymentStats: testutil.IntervalFast, }) + defer cache.Close() _, err := db.InsertWorkspaceAgentStat(context.Background(), database.InsertWorkspaceAgentStatParams{ ID: uuid.New(), diff --git a/coderd/workspaceapps/apptest/setup.go b/coderd/workspaceapps/apptest/setup.go index 60a1ad76cabbb..f91664717609d 100644 --- a/coderd/workspaceapps/apptest/setup.go +++ b/coderd/workspaceapps/apptest/setup.go @@ -347,7 +347,7 @@ func createWorkspaceWithApps(t *testing.T, client *codersdk.Client, orgID uuid.U primaryAppHost, err := client.AppHost(appHostCtx) require.NoError(t, err) if primaryAppHost.Host != "" { - manifest, err := agentClient.Manifest(context.Background()) + manifest, err := agentClient.Manifest(appHostCtx) require.NoError(t, err) proxyURL := fmt.Sprintf( "http://{{port}}--%s--%s--%s%s", diff --git a/enterprise/coderd/coderdenttest/coderdenttest.go b/enterprise/coderd/coderdenttest/coderdenttest.go index 40f819ea67507..06758d013ded8 100644 --- a/enterprise/coderd/coderdenttest/coderdenttest.go +++ b/enterprise/coderd/coderdenttest/coderdenttest.go @@ -12,7 +12,6 @@ import ( "github.com/golang-jwt/jwt/v4" "github.com/google/uuid" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/coder/coder/coderd/coderdtest" @@ -58,6 +57,8 @@ func New(t *testing.T, options *Options) *codersdk.Client { } func NewWithAPI(t *testing.T, options *Options) (*codersdk.Client, io.Closer, *coderd.API) { + t.Helper() + if options == nil { options = &Options{} } @@ -77,7 +78,7 @@ func NewWithAPI(t *testing.T, options *Options) (*codersdk.Client, io.Closer, *c Keys: Keys, ProxyHealthInterval: options.ProxyHealthInterval, }) - assert.NoError(t, err) + require.NoError(t, err) setHandler(coderAPI.AGPL.RootHandler) var provisionerCloser io.Closer = nopcloser{} if options.IncludeProvisionerDaemon { From ce3e4749d173d71c09de6c0de023fb25fa16a50e Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 13 Jun 2023 15:58:23 +0000 Subject: [PATCH 2/4] Close coordinator --- enterprise/coderd/coderd.go | 1 + 1 file changed, 1 insertion(+) diff --git a/enterprise/coderd/coderd.go b/enterprise/coderd/coderd.go index ee860cfba9ed0..be7d6acb6c731 100644 --- a/enterprise/coderd/coderd.go +++ b/enterprise/coderd/coderd.go @@ -410,6 +410,7 @@ func (api *API) updateEntitlements(ctx context.Context) error { // is actually changing. changed = false } else { + _ = coordinator.Close() coordinator = haCoordinator } From a72d04ce0a943b21e2a2492a84a869deb10c75e8 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 13 Jun 2023 16:43:37 +0000 Subject: [PATCH 3/4] Misc fixes --- enterprise/replicasync/replicasync_test.go | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/enterprise/replicasync/replicasync_test.go b/enterprise/replicasync/replicasync_test.go index eb3a7175bee6e..f2c0eebd8cd5c 100644 --- a/enterprise/replicasync/replicasync_test.go +++ b/enterprise/replicasync/replicasync_test.go @@ -34,7 +34,10 @@ func TestReplica(t *testing.T) { db, pubsub := dbtestutil.NewDB(t) closeChan := make(chan struct{}, 1) cancel, err := pubsub.Subscribe(replicasync.PubsubEvent, func(ctx context.Context, message []byte) { - closeChan <- struct{}{} + select { + case closeChan <- struct{}{}: + default: + } }) require.NoError(t, err) defer cancel() @@ -70,6 +73,8 @@ func TestReplica(t *testing.T) { RelayAddress: "http://169.254.169.254", }) require.NoError(t, err) + defer server.Close() + require.Len(t, server.Regional(), 1) require.Equal(t, peer.ID, server.Regional()[0].ID) require.Empty(t, server.Self().Error) @@ -113,6 +118,8 @@ func TestReplica(t *testing.T) { TLSConfig: tlsConfig, }) require.NoError(t, err) + defer server.Close() + require.Len(t, server.Regional(), 1) require.Equal(t, peer.ID, server.Regional()[0].ID) require.Empty(t, server.Self().Error) @@ -138,6 +145,8 @@ func TestReplica(t *testing.T) { RelayAddress: "http://127.0.0.1:1", }) require.NoError(t, err) + defer server.Close() + require.Len(t, server.Regional(), 1) require.Equal(t, peer.ID, server.Regional()[0].ID) require.NotEmpty(t, server.Self().Error) @@ -152,6 +161,7 @@ func TestReplica(t *testing.T) { defer cancelCtx() server, err := replicasync.New(ctx, slogtest.Make(t, nil), db, pubsub, nil) require.NoError(t, err) + defer server.Close() srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) })) From 401ba7b7562fd287fdf27804efb43f6c9add9d3e Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 13 Jun 2023 16:46:04 +0000 Subject: [PATCH 4/4] Ensure closure on error --- enterprise/coderd/coderd.go | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/enterprise/coderd/coderd.go b/enterprise/coderd/coderd.go index be7d6acb6c731..5da6f392fc431 100644 --- a/enterprise/coderd/coderd.go +++ b/enterprise/coderd/coderd.go @@ -35,7 +35,7 @@ import ( // New constructs an Enterprise coderd API instance. // This handler is designed to wrap the AGPL Coder code and // layer Enterprise functionality on top as much as possible. -func New(ctx context.Context, options *Options) (*API, error) { +func New(ctx context.Context, options *Options) (_ *API, err error) { if options.EntitlementsUpdateInterval == 0 { options.EntitlementsUpdateInterval = 10 * time.Minute } @@ -59,6 +59,11 @@ func New(ctx context.Context, options *Options) (*API, error) { AGPL: coderd.New(options.Options), Options: options, } + defer func() { + if err != nil { + _ = api.Close() + } + }() api.AGPL.Options.SetUserGroups = api.setUserGroups @@ -312,8 +317,12 @@ type API struct { func (api *API) Close() error { api.cancel() - _ = api.replicaManager.Close() - _ = api.derpMesh.Close() + if api.replicaManager != nil { + _ = api.replicaManager.Close() + } + if api.derpMesh != nil { + _ = api.derpMesh.Close() + } return api.AGPL.Close() }