Remove flock, change consent write to be atomic, measure file count i…

…n set test

go.mod

@@ -4,7 +4,6 @@ go 1.23.4
 
 require (
 	github.com/BurntSushi/toml v1.4.0
-	github.com/gofrs/flock v0.12.1
 	github.com/spf13/cobra v1.8.1
 	github.com/spf13/pflag v1.0.5
 	github.com/stretchr/testify v1.10.0
@@ -14,8 +13,7 @@ require (
 require (
 	github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect
 	github.com/inconshreveable/mousetrap v1.1.0 // indirect
-	github.com/kr/text v0.2.0 // indirect
+	github.com/kr/pretty v0.3.1 // indirect
 	github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 // indirect
-	github.com/rogpeppe/go-internal v1.9.0 // indirect
-	golang.org/x/sys v0.22.0 // indirect
+	gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c // indirect
 )

go.sum

@@ -4,14 +4,16 @@ github.com/cpuguy83/go-md2man/v2 v2.0.4/go.mod h1:tgQtvFlXSQOSOSIRvRPT7W67SCa46t
 github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E=
 github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc h1:U9qPSI2PIWSS1VwoXQT9A3Wy9MM3WgvqSxFWenqJduM=
 github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
-github.com/gofrs/flock v0.12.1 h1:MTLVXXHf8ekldpJk3AKicLij9MdwOWkZ+a/jHHZby9E=
-github.com/gofrs/flock v0.12.1/go.mod h1:9zxTsyu5xtJ9DK+1tFZyibEV7y3uwDxPPfbxeeHCoD0=
 github.com/inconshreveable/mousetrap v1.1.0 h1:wN+x4NVGpMsO7ErUn/mUI3vEoE6Jt13X2s0bqwp9tc8=
 github.com/inconshreveable/mousetrap v1.1.0/go.mod h1:vpF70FUmC8bwa3OWnCshd2FqLfsEA9PFc4w1p2J65bw=
+github.com/kr/pretty v0.2.1/go.mod h1:ipq/a2n7PKx3OHsz4KJII5eveXtPO4qwEXGdVfWzfnI=
 github.com/kr/pretty v0.3.1 h1:flRD4NNwYAUpkphVc1HcthR4KEIFJ65n8Mw5qdRn3LE=
 github.com/kr/pretty v0.3.1/go.mod h1:hoEshYVHaxMs3cyo3Yncou5ZscifuDolrwPKZanG3xk=
+github.com/kr/pty v1.1.1/go.mod h1:pFQYn66WHrOpPYNljwOMqo10TkYh1fy3cYio2l3bCsQ=
+github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI=
 github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY=
 github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE=
+github.com/pkg/diff v0.0.0-20210226163009-20ebb0f2a09e/go.mod h1:pJLUxLENpZxwdsKMEsNbx1VGcRFpLqf3715MtcvvzbA=
 github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 h1:Jamvg5psRIccs7FGNTlIRMkT8wgtp5eCXdBlqhYGL6U=
 github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
 github.com/rogpeppe/go-internal v1.9.0 h1:73kH8U+JUqXU8lRuOHeVHaa/SZPifC7BkcraZVejAe8=
@@ -23,8 +25,6 @@ github.com/spf13/pflag v1.0.5 h1:iy+VFUOCP1a+8yFto/drg2CJ5u0yRoB7fZw3DKv/JXA=
 github.com/spf13/pflag v1.0.5/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An2Bg=
 github.com/stretchr/testify v1.10.0 h1:Xv5erBjTwe/5IxqUQTdXv5kgmIvbHo3QQyRwhJsOfJA=
 github.com/stretchr/testify v1.10.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY=
-golang.org/x/sys v0.22.0 h1:RI27ohtqKCnwULzJLqkv897zojh5/DwS/ENaMzUOaWI=
-golang.org/x/sys v0.22.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
 gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
 gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c h1:Hei/4ADfdWqJk1ZMxUNpqntNwaWcugrBjAiHlqqRiVk=
 gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c/go.mod h1:JHkPIbrfpd72SG/EVd6muEfDQjcINNoR0C8j2r3qZ4Q=

internal/consent/consent.go

@@ -10,7 +10,6 @@ import (
 	"strings"
 
 	"github.com/BurntSushi/toml"
-	"github.com/gofrs/flock"
 	"github.com/ubuntu/ubuntu-insights/internal/constants"
 )
 
@@ -160,42 +159,34 @@ func readConsentFile(filePath string) (*consentFile, error) {
 	if _, err := os.Stat(filePath); os.IsNotExist(err) {
 		return &consent, nil
 	}
-
-	lock := flock.New(filePath + ".lock")
-	lockAcquired, err := lock.TryRLock()
-	if err != nil {
-		return &consent, err
-	}
-	if !lockAcquired {
-		return &consent, fmt.Errorf("could not acquire lock on %s", filePath)
-	}
-	defer lock.Unlock()
-
-	_, err = toml.DecodeFile(filePath, &consent)
+	_, err := toml.DecodeFile(filePath, &consent)
 
 	return &consent, err
 }
 
-// writeConsentFile writes the given consent file to the given path, replacing it if it already exists.
-func writeConsentFile(filePath string, consent *consentFile) error {
-	lock := flock.New(filePath + ".lock")
-	lockAcquired, err := lock.TryLock()
+// writeConsentFile writes the given consent file to the given path atomically, replacing it if it already exists.
+// Not atomic in Windows.
+func writeConsentFile(filePath string, consent *consentFile) (err error) {
+	dir := filepath.Dir(filePath)
+	tempFile, err := os.CreateTemp(dir, "consent-*.tmp")
 	if err != nil {
-		return err
+		return fmt.Errorf("could not create temporary file: %w", err)
 	}
-	if !lockAcquired {
-		return fmt.Errorf("could not acquire lock on %s", filePath)
+	defer func() {
+		tempFile.Close()
+		os.Remove(tempFile.Name())
+	}()
+
+	if err := toml.NewEncoder(tempFile).Encode(consent); err != nil {
+		return fmt.Errorf("could not encode consent file: %w", err)
 	}
-	defer lock.Unlock()
 
-	file, err := os.OpenFile(filePath, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0600)
-	if err != nil {
-		return err
+	if err := tempFile.Close(); err != nil {
+		return fmt.Errorf("could not close temporary file: %w", err)
 	}
-	defer file.Close()
 
-	if err := toml.NewEncoder(file).Encode(consent); err != nil {
-		return err
+	if err := os.Rename(tempFile.Name(), filePath); err != nil {
+		return fmt.Errorf("could not rename temporary file: %w", err)
 	}
 
 	return nil

internal/consent/consent_test.go

@@ -7,71 +7,49 @@ import (
 	"path/filepath"
 	"testing"
 
-	"github.com/gofrs/flock"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"github.com/ubuntu/ubuntu-insights/internal/consent"
 	"github.com/ubuntu/ubuntu-insights/internal/testutils"
 )
 
-type lockType uint
-
-const (
-	noLock lockType = iota
-	readLock
-	writeLock
-)
-
 // consentDir is a struct that holds a test's temporary directory and its locks.
 // It should be cleaned up after the test is done.
 type consentDir struct {
-	dir         string
-	sourceLocks map[string]*flock.Flock
-	globalLock  *flock.Flock
+	dir string
 }
 
 func TestGetConsentStates(t *testing.T) {
 	t.Parallel()
 
 	tests := map[string]struct {
-		sources     []string
-		lockSources map[string]lockType
-		globalFile  string
-		lockGlobal  lockType
+		sources    []string
+		globalFile string
 
 		wantErr bool
 	}{
-		"No Global File, No Locks": {},
+		"No Global File": {},
 
 		// Global File Tests
-		"Valid True Global File, No Locks":          {globalFile: "valid_true-consent.toml"},
-		"Valid False Global File, No Locks":         {globalFile: "valid_false-consent.toml"},
-		"Valid True Global File, Global Read Lock":  {globalFile: "valid_true-consent.toml", lockGlobal: readLock},
-		"Valid True Global File, Global Write Lock": {globalFile: "valid_true-consent.toml", lockGlobal: writeLock},
-		"Invalid Value Global File, No Locks":       {globalFile: "invalid_value-consent.toml"},
-		"Invalid File Global File, No Locks":        {globalFile: "invalid_file-consent.toml"},
-
-		// Lock sources
-		"Valid True Global File, Source Read Lock":                         {globalFile: "valid_true-consent.toml", lockSources: map[string]lockType{"valid_true": readLock}},
-		"Valid True Global File, Source Write Lock":                        {globalFile: "valid_true-consent.toml", lockSources: map[string]lockType{"valid_true": writeLock}},
-		"Valid True Global File, Source Read Lock, Global Read Lock":       {globalFile: "valid_true-consent.toml", lockSources: map[string]lockType{"valid_true": readLock}, lockGlobal: readLock},
-		"Valid True Global File, Source Write Lock, Global Read Lock":      {globalFile: "valid_true-consent.toml", lockSources: map[string]lockType{"valid_true": writeLock}, lockGlobal: readLock},
-		"Valid True Global File, Dual Source Write Lock, Global Read Lock": {globalFile: "valid_true-consent.toml", lockSources: map[string]lockType{"valid_true": writeLock, "invalid_value": writeLock}, lockGlobal: readLock},
+		"Valid True Global File":    {globalFile: "valid_true-consent.toml"},
+		"Valid False Global File":   {globalFile: "valid_false-consent.toml"},
+		"Invalid Value Global File": {globalFile: "invalid_value-consent.toml"},
+		"Invalid File Global File":  {globalFile: "invalid_file-consent.toml"},
 
 		// Source Specific Tests
-		"Valid True Global File, No Locks, Valid True Source, No Locks":                       {globalFile: "valid_true-consent.toml", sources: []string{"valid_true"}},
-		"Valid True Global File, No Locks, Valid False Source, No Locks":                      {globalFile: "valid_true-consent.toml", sources: []string{"valid_false"}},
-		"Valid True Global File, No Locks, Invalid Value Source, No Locks":                    {globalFile: "valid_true-consent.toml", sources: []string{"invalid_value"}},
-		"Valid True Global File, No Locks, Invalid File Source, No Locks":                     {globalFile: "valid_true-consent.toml", sources: []string{"invalid_file"}},
-		"Valid True Global File, No Locks, No File Source, No Locks":                          {globalFile: "valid_true-consent.toml", sources: []string{"not_a_file"}},
-		"Valid True Global File, No Locks, 2 Multiple Sources (VTrue, VFalse), No Locks":      {globalFile: "valid_true-consent.toml", sources: []string{"valid_true", "valid_false"}},
-		"Valid True Global File, No Locks, 3 Multiple Sources (VTrue, VFalse, NAF), No Locks": {globalFile: "valid_true-consent.toml", sources: []string{"valid_true", "valid_false", "not_a_file"}},
+		"Valid True Global File, Valid True Source":                       {globalFile: "valid_true-consent.toml", sources: []string{"valid_true"}},
+		"Valid True Global File, Valid False Source":                      {globalFile: "valid_true-consent.toml", sources: []string{"valid_false"}},
+		"Valid True Global File, Invalid Value Source":                    {globalFile: "valid_true-consent.toml", sources: []string{"invalid_value"}},
+		"Valid True Global File, Invalid File Source":                     {globalFile: "valid_true-consent.toml", sources: []string{"invalid_file"}},
+		"Valid True Global File, No File Source":                          {globalFile: "valid_true-consent.toml", sources: []string{"not_a_file"}},
+		"Valid True Global File, 2 Multiple Sources (VTrue, VFalse)":      {globalFile: "valid_true-consent.toml", sources: []string{"valid_true", "valid_false"}},
+		"Valid True Global File, 3 Multiple Sources (VTrue, VFalse, NAF)": {globalFile: "valid_true-consent.toml", sources: []string{"valid_true", "valid_false", "not_a_file"}},
 	}
 
 	for name, tc := range tests {
 		t.Run(name, func(t *testing.T) {
 			t.Parallel()
-			cdir, err := setupTmpConsentFiles(t, tc.lockSources, tc.globalFile, tc.lockGlobal)
+			cdir, err := setupTmpConsentFiles(t, tc.globalFile)
 			require.NoError(t, err, "failed to setup temporary consent files")
 			defer cdir.cleanup(t)
 			cm := consent.New(cdir.dir)
@@ -95,50 +73,41 @@ func TestSetConsentStates(t *testing.T) {
 	tests := map[string]struct {
 		sources       []string
 		consentStates map[string]bool
-		lockSources   map[string]lockType
 		globalFile    string
-		lockGlobal    lockType
 
 		writeSource string
 		writeState  bool
 
 		wantErr bool
 	}{
-		// New File Tests No Locks
-		"New File, No Locks, Write Global False": {},
-		"New File, No Locks, Write Global True":  {writeState: true},
-		"New File, No Locks, Write Source True":  {writeSource: "new_true", writeState: true},
-		"New File, No Locks, Write Source False": {writeSource: "new_false"},
-
-		// Overwrite File, No Locks, Different State
-		"Overwrite File, No Locks, Write Diff Global False": {globalFile: "valid_true-consent.toml", writeState: false},
-		"Overwrite File, No Locks, Write Diff Global True":  {globalFile: "valid_false-consent.toml", writeState: true},
-		"Overwrite File, No Locks, Write Diff Source True":  {globalFile: "valid_true-consent.toml", writeSource: "valid_false", writeState: true},
-		"Overwrite File, No Locks, Write Diff Source False": {globalFile: "valid_true-consent.toml", writeSource: "valid_true", writeState: false},
-
-		// Overwrite File, No Locks, Same State
-		"Overwrite File, No Locks, Write Global True":  {globalFile: "valid_true-consent.toml", writeState: true},
-		"Overwrite File, No Locks, Write Global False": {globalFile: "valid_false-consent.toml", writeState: false},
-		"Overwrite File, No Locks, Write Source True":  {globalFile: "valid_true-consent.toml", writeSource: "valid_true", writeState: true},
-		"Overwrite File, No Locks, Write Source False": {globalFile: "valid_false-consent.toml", writeSource: "valid_false"},
-
-		// Overwrite File, Read Locks, Different State
-		"Overwrite File, Source Read Lock, Write Global False": {globalFile: "valid_true-consent.toml", lockGlobal: readLock, writeState: false, wantErr: true},
-		"Overwrite File, Source Read Lock, Write Global True":  {globalFile: "valid_false-consent.toml", lockGlobal: readLock, writeState: true, wantErr: true},
-		"Overwrite File, Source Read Lock, Write Source True":  {globalFile: "valid_true-consent.toml", lockSources: map[string]lockType{"valid_false": readLock}, writeSource: "valid_false", writeState: true, wantErr: true},
-		"Overwrite File, Source Read Lock, Write Source False": {globalFile: "valid_false-consent.toml", lockSources: map[string]lockType{"valid_true": readLock}, writeSource: "valid_true", writeState: false, wantErr: true},
-
-		// Overwrite File, Write Locks, Different State
-		"Overwrite File, Source Write Lock, Write Global False": {globalFile: "valid_true-consent.toml", lockGlobal: writeLock, writeState: false, wantErr: true},
-		"Overwrite File, Source Write Lock, Write Global True":  {globalFile: "valid_false-consent.toml", lockGlobal: writeLock, writeState: true, wantErr: true},
-		"Overwrite File, Source Write Lock, Write Source True":  {globalFile: "valid_true-consent.toml", lockSources: map[string]lockType{"valid_false": writeLock}, writeSource: "valid_false", writeState: true, wantErr: true},
-		"Overwrite File, Source Write Lock, Write Source False": {globalFile: "valid_false-consent.toml", lockSources: map[string]lockType{"valid_true": writeLock}, writeSource: "valid_true", writeState: false, wantErr: true},
+		// New File Tests
+		"New File, Write Global False": {},
+		"New File, Write Global True":  {writeState: true},
+		"New File, Write Source True":  {writeSource: "new_true", writeState: true},
+		"New File, Write Source False": {writeSource: "new_false"},
+
+		// Overwrite File, Different State
+		"Overwrite File, Write Diff Global False": {globalFile: "valid_true-consent.toml", writeState: false},
+		"Overwrite File, Write Diff Global True":  {globalFile: "valid_false-consent.toml", writeState: true},
+		"Overwrite File, Write Diff Source True":  {globalFile: "valid_true-consent.toml", writeSource: "valid_false", writeState: true},
+		"Overwrite File, Write Diff Source False": {globalFile: "valid_true-consent.toml", writeSource: "valid_true", writeState: false},
+
+		// Overwrite File, Same State
+		"Overwrite File, Write Global True":  {globalFile: "valid_true-consent.toml", writeState: true},
+		"Overwrite File, Write Global False": {globalFile: "valid_false-consent.toml", writeState: false},
+		"Overwrite File, Write Source True":  {globalFile: "valid_true-consent.toml", writeSource: "valid_true", writeState: true},
+		"Overwrite File, Write Source False": {globalFile: "valid_false-consent.toml", writeSource: "valid_false"},
+	}
+
+	type goldenFile struct {
+		States    *consent.States
+		FileCount uint
 	}
 
 	for name, tc := range tests {
 		t.Run(name, func(t *testing.T) {
 			t.Parallel()
-			cdir, err := setupTmpConsentFiles(t, tc.lockSources, tc.globalFile, tc.lockGlobal)
+			cdir, err := setupTmpConsentFiles(t, tc.globalFile)
 			require.NoError(t, err, "failed to setup temporary consent files")
 			defer cdir.cleanup(t)
 			cm := consent.New(cdir.dir)
@@ -150,9 +119,13 @@ func TestSetConsentStates(t *testing.T) {
 			}
 			require.NoError(t, err, "got an unexpected error")
 
-			got, err := cm.GetConsentStates(tc.sources)
+			states, err := cm.GetConsentStates(tc.sources)
 			require.NoError(t, err, "got an unexpected error while getting consent states")
 
+			d, err := os.ReadDir(cdir.dir)
+			require.NoError(t, err, "failed to read temporary directory")
+			got := goldenFile{States: states, FileCount: uint(len(d))}
+
 			want := testutils.LoadWithUpdateFromGoldenYAML(t, got)
 			require.Equal(t, want, got, "GetConsentStates should return expected consent states")
 		})
@@ -162,14 +135,6 @@ func TestSetConsentStates(t *testing.T) {
 // cleanup unlocks all the locks and removes the temporary directory including its contents.
 func (cdir consentDir) cleanup(t *testing.T) {
 	t.Helper()
-	for i := range cdir.sourceLocks {
-		assert.NoError(t, cdir.sourceLocks[i].Unlock(), "failed to unlock source lock")
-	}
-
-	if cdir.globalLock != nil {
-		assert.NoError(t, cdir.globalLock.Unlock(), "failed to unlock global lock")
-	}
-
 	assert.NoError(t, os.RemoveAll(cdir.dir), "failed to remove temporary directory")
 }
 
@@ -207,9 +172,9 @@ func copyDir(srcDir, dstDir string) error {
 	})
 }
 
-func setupTmpConsentFiles(t *testing.T, lockSources map[string]lockType, globalFile string, lockGlobal lockType) (*consentDir, error) {
+func setupTmpConsentFiles(t *testing.T, globalFile string) (*consentDir, error) {
 	t.Helper()
-	cdir := consentDir{sourceLocks: make(map[string]*flock.Flock)}
+	cdir := consentDir{}
 
 	// Setup temporary directory
 	var err error
@@ -229,45 +194,5 @@ func setupTmpConsentFiles(t *testing.T, lockSources map[string]lockType, globalF
 		}
 	}
 
-	// Setup lock files
-	for source, lock := range lockSources {
-		if lock == noLock {
-			continue
-		}
-
-		lockPath := filepath.Join(cdir.dir, source+"-consent.toml.lock")
-		cdir.sourceLocks[source] = flock.New(lockPath)
-		switch lock {
-		case readLock:
-			err = cdir.sourceLocks[source].RLock()
-		case writeLock:
-			err = cdir.sourceLocks[source].Lock()
-		}
-
-		if err != nil {
-			return &cdir, fmt.Errorf("failed to acquire lock on consent file for source %s: %v", source, err)
-		}
-	}
-
-	// Setup global lock file
-	if lockGlobal != noLock {
-		if globalFile == "" {
-			return &cdir, fmt.Errorf("global file must be provided if global lock is requested")
-		}
-
-		lockPath := filepath.Join(cdir.dir, "consent.toml.lock")
-		cdir.globalLock = flock.New(lockPath)
-		switch lockGlobal {
-		case readLock:
-			err = cdir.globalLock.RLock()
-		case writeLock:
-			err = cdir.globalLock.Lock()
-		}
-
-		if err != nil {
-			return &cdir, fmt.Errorf("failed to acquire lock on global consent file: %v", err)
-		}
-	}
-
 	return &cdir, nil
 }