From efbc2b92bbc2a1123fc6520d4c51b9f37183bdbb Mon Sep 17 00:00:00 2001 From: Zach Wasserman Date: Fri, 18 Mar 2022 09:30:45 -0700 Subject: [PATCH] Fix race condition in updates test (#4661) Copy the DefaultOptions in order to prevent a data race on the Targets map. This race should only have effected testing. Race detector output: ``` WARNING: DATA RACE Read at 0x00c0000908d0 by goroutine 15: runtime.mapaccess1_faststr() /opt/hostedtoolcache/go/1.18.0/x64/src/runtime/map_faststr.go:13 +0x0 github.com/fleetdm/fleet/v4/orbit/pkg/update.TestMakeRepoPath.func1() /home/runner/work/fleet/fleet/orbit/pkg/update/update_test.go:58 +0xb6 testing.tRunner() /opt/hostedtoolcache/go/1.18.0/x64/src/testing/testing.go:1439 +0x213 testing.(*T).Run.func1() /opt/hostedtoolcache/go/1.18.0/x64/src/testing/testing.go:1486 +0x47 Previous write at 0x00c0000908d0 by goroutine 12: runtime.mapassign_faststr() /opt/hostedtoolcache/go/1.18.0/x64/src/runtime/map_faststr.go:203 +0x0 github.com/fleetdm/fleet/v4/orbit/pkg/update.TestMakeRepoPath.func1() /home/runner/work/fleet/fleet/orbit/pkg/update/update_test.go:62 +0x1cb testing.tRunner() /opt/hostedtoolcache/go/1.18.0/x64/src/testing/testing.go:1439 +0x213 testing.(*T).Run.func1() /opt/hostedtoolcache/go/1.18.0/x64/src/testing/testing.go:1486 +0x47 Goroutine 15 (running) created at: testing.(*T).Run() /opt/hostedtoolcache/go/1.18.0/x64/src/testing/testing.go:1486 +0x724 github.com/fleetdm/fleet/v4/orbit/pkg/update.TestMakeRepoPath() /home/runner/work/fleet/fleet/orbit/pkg/update/update_test.go:53 +0x1a4 testing.tRunner() /opt/hostedtoolcache/go/1.18.0/x64/src/testing/testing.go:1439 +0x213 testing.(*T).Run.func1() /opt/hostedtoolcache/go/1.18.0/x64/src/testing/testing.go:1486 +0x47 Goroutine 12 (running) created at: testing.(*T).Run() /opt/hostedtoolcache/go/1.18.0/x64/src/testing/testing.go:1486 +0x724 github.com/fleetdm/fleet/v4/orbit/pkg/update.TestMakeRepoPath() /home/runner/work/fleet/fleet/orbit/pkg/update/update_test.go:53 +0x1a4 testing.tRunner() /opt/hostedtoolcache/go/1.18.0/x64/src/testing/testing.go:1439 +0x213 testing.(*T).Run.func1() /opt/hostedtoolcache/go/1.18.0/x64/src/testing/testing.go:1486 +0x47 ``` --- orbit/pkg/update/update_test.go | 7 ++++++- .../tables/20201011162341_CleanupSoftDeletedColumns.go | 4 ++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/orbit/pkg/update/update_test.go b/orbit/pkg/update/update_test.go index 678feb2d02..9f6c6233a4 100644 --- a/orbit/pkg/update/update_test.go +++ b/orbit/pkg/update/update_test.go @@ -6,6 +6,7 @@ import ( "testing" "github.com/fleetdm/fleet/v4/orbit/pkg/constant" + "github.com/jinzhu/copier" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -53,7 +54,11 @@ func TestMakeRepoPath(t *testing.T) { t.Run(tt.expected, func(t *testing.T) { t.Parallel() - opt := DefaultOptions + var opt Options + // Must deep copy DefaultOptions, otherwise there is a race condition when modifying the + // opt.Targets map in parallel tests below. + err := copier.CopyWithOption(&opt, DefaultOptions, copier.Option{DeepCopy: true}) + require.NoError(t, err) osqueryd := opt.Targets[tt.name] osqueryd.Platform = tt.platform diff --git a/server/datastore/mysql/migrations/tables/20201011162341_CleanupSoftDeletedColumns.go b/server/datastore/mysql/migrations/tables/20201011162341_CleanupSoftDeletedColumns.go index 36c6c5ad6a..2157c3cc23 100644 --- a/server/datastore/mysql/migrations/tables/20201011162341_CleanupSoftDeletedColumns.go +++ b/server/datastore/mysql/migrations/tables/20201011162341_CleanupSoftDeletedColumns.go @@ -56,7 +56,7 @@ func cleanupSoftDeleteFields(tx *sql.Tx, dbTable string) error { } func addSoftDeleteFields(tx *sql.Tx, dbTable string) error { - addDeletedStmt := fmt.Sprint( + addDeletedStmt := fmt.Sprintf( "ALTER TABLE `%s` "+ "ADD COLUMN `deleted` TINYINT(1) NOT NULL DEFAULT FALSE;", dbTable) @@ -65,7 +65,7 @@ func addSoftDeleteFields(tx *sql.Tx, dbTable string) error { return err } - addDeletedAtStmt := fmt.Sprint( + addDeletedAtStmt := fmt.Sprintf( "ALTER TABLE `%s` "+ "ADD COLUMN `deleted_at` TIMESTAMP NULL DEFAULT NULL;", dbTable)