Fix concurrent credential saves losing updates

.changes/unreleased/Patch-2026-08-20T130432Z.yaml

@@ -0,0 +1,3 @@
+kind: Patch
+body: Fixed a bug where removing and re-adding a credential with the same name within a single process would silently lose the re-added credential from disk. Restructured credential persistence to read the file fresh before each mutation, eliminating reliance on stale in-memory state.
+time: 2026-08-20T13:04:32Z

.gitignore

@@ -32,4 +32,6 @@ docs/usageGuide/.DS_Store
 docs/.DS_Store
 
 # intelij settings
-/.idea
+/.idea
+
+.plans/

common/clicfg/credentials/aura.go

@@ -14,7 +14,21 @@ import (
 type AuraCredentials struct {
 	DefaultCredential string            `json:"default-credential"`
 	Credentials       []*AuraCredential `json:"credentials"`
-	onUpdate          func()
+	refresh           func() error
+	persist           func() error
+}
+
+// refreshAndPersist re-reads the on-disk state into c immediately before mutate runs, so mutate
+// always applies its delta on top of the current file rather than a snapshot that may have gone
+// stale since load. It only persists if mutate succeeds.
+func (c *AuraCredentials) refreshAndPersist(mutate func() error) error {
+	if err := c.refresh(); err != nil {
+		return err
+	}
+	if err := mutate(); err != nil {
+		return err
+	}
+	return c.persist()
 }
 
 func (c *AuraCredentials) List() []*AuraCredential {
@@ -33,52 +47,54 @@ func (config *AuraCredentials) Print(writer io.Writer) error {
 }
 
 func (c *AuraCredentials) Add(name string, clientId string, clientSecret string) error {
-	auraCredentials := c.Credentials
-	for _, credential := range auraCredentials {
-		if credential.Name == name {
-			return clierr.NewUsageError("already have credential with name %s", name)
+	return c.refreshAndPersist(func() error {
+		for _, credential := range c.Credentials {
+			if credential.Name == name {
+				return clierr.NewUsageError("already have credential with name %s", name)
+			}
 		}
-	}
 
-	c.Credentials = append(c.Credentials, &AuraCredential{Name: name, ClientId: clientId, ClientSecret: clientSecret})
-	if len(c.Credentials) == 1 {
-		c.SetDefault(name)
-	}
-	c.onUpdate()
-	return nil
+		c.Credentials = append(c.Credentials, &AuraCredential{Name: name, ClientId: clientId, ClientSecret: clientSecret})
+		if len(c.Credentials) == 1 {
+			c.DefaultCredential = name
+		}
+		return nil
+	})
 }
 
 func (c *AuraCredentials) Remove(name string) error {
-	var indexToRemove = -1
-
-	for i, credential := range c.Credentials {
-		if credential.Name == name {
-			indexToRemove = i
-			break
+	return c.refreshAndPersist(func() error {
+		var indexToRemove = -1
+
+		for i, credential := range c.Credentials {
+			if credential.Name == name {
+				indexToRemove = i
+				break
+			}
 		}
-	}
 
-	if indexToRemove == -1 {
-		return clierr.NewUsageError("could not find credential with name %s to remove", name)
-	}
+		if indexToRemove == -1 {
+			return clierr.NewUsageError("could not find credential with name %s to remove", name)
+		}
 
-	if c.DefaultCredential == name {
-		c.DefaultCredential = ""
-	}
+		if c.DefaultCredential == name {
+			c.DefaultCredential = ""
+		}
 
-	c.Credentials = append(c.Credentials[:indexToRemove], c.Credentials[indexToRemove+1:]...)
-	c.onUpdate()
-	return nil
+		c.Credentials = append(c.Credentials[:indexToRemove], c.Credentials[indexToRemove+1:]...)
+		return nil
+	})
 }
 
 func (c *AuraCredentials) SetDefault(name string) error {
-	if !c.credentialExists(name) {
-		return clierr.NewUsageError("could not find credential with name %s", name)
-	}
+	return c.refreshAndPersist(func() error {
+		if !c.credentialExists(name) {
+			return clierr.NewUsageError("could not find credential with name %s", name)
+		}
 
-	c.DefaultCredential = name
-	c.onUpdate()
-	return nil
+		c.DefaultCredential = name
+		return nil
+	})
 }
 
 func (c *AuraCredentials) GetDefault() (*AuraCredential, error) {
@@ -98,29 +114,43 @@ func (c *AuraCredentials) Get(name string) (*AuraCredential, error) {
 }
 
 func (c *AuraCredentials) UpdateAccessToken(cred *AuraCredential, accessToken string, expiresInSeconds int64) *AuraCredential {
-	credential, err := c.Get(cred.Name)
+	var credential *AuraCredential
+	err := c.refreshAndPersist(func() error {
+		var err error
+		credential, err = c.Get(cred.Name)
+		if err != nil {
+			return err
+		}
+
+		const expireToleranceSeconds = 60
+		now := time.Now().UnixMilli()
+
+		credential.TokenExpiry = now + (expiresInSeconds-expireToleranceSeconds)*1000
+		credential.AccessToken = accessToken
+		return nil
+	})
 	if err != nil {
 		panic(err)
 	}
-	const expireToleranceSeconds = 60
-
-	now := time.Now().UnixMilli()
-
-	credential.TokenExpiry = now + (expiresInSeconds-expireToleranceSeconds)*1000
-	credential.AccessToken = accessToken
-	c.onUpdate()
 	return credential
 }
 
 func (c *AuraCredentials) ClearAccessToken(cred *AuraCredential) (*AuraCredential, error) {
-	credential, err := c.Get(cred.Name)
+	var credential *AuraCredential
+	err := c.refreshAndPersist(func() error {
+		var err error
+		credential, err = c.Get(cred.Name)
+		if err != nil {
+			return err
+		}
+
+		credential.TokenExpiry = 0
+		credential.AccessToken = ""
+		return nil
+	})
 	if err != nil {
 		return nil, err
 	}
-
-	credential.TokenExpiry = 0
-	credential.AccessToken = ""
-	c.onUpdate()
 	return credential, nil
 }
 

common/clicfg/credentials/credentials.go

@@ -38,7 +38,6 @@ func (c *Credentials) load() {
 	var credentials CredentialsFile = CredentialsFile{
 		Aura: &AuraCredentials{
 			Credentials: []*AuraCredential{},
-			onUpdate:    c.save,
 		},
 	}
 	if fileHasData {
@@ -48,13 +47,39 @@ func (c *Credentials) load() {
 	}
 
 	c.Aura = credentials.Aura
+	c.Aura.refresh = c.refreshAura
+	c.Aura.persist = c.save
 
 	if !fileHasData {
 		c.save()
 	}
 }
 
-func (c *Credentials) save() {
+// refreshAura re-reads the credentials file from disk into the existing c.Aura, discarding
+// whatever was loaded or mutated in memory before this call. It updates the struct in place
+// rather than replacing it, so the refresh/persist closures wired up in load stay intact.
+func (c *Credentials) refreshAura() error {
+	data := fileutils.ReadFileSafe(c.fs, c.filePath)
+
+	var credFile CredentialsFile
+	if len(data) != 0 {
+		if err := json.Unmarshal(data, &credFile); err != nil {
+			return err
+		}
+	}
+
+	if credFile.Aura == nil {
+		c.Aura.Credentials = []*AuraCredential{}
+		c.Aura.DefaultCredential = ""
+		return nil
+	}
+
+	c.Aura.Credentials = credFile.Aura.Credentials
+	c.Aura.DefaultCredential = credFile.Aura.DefaultCredential
+	return nil
+}
+
+func (c *Credentials) save() error {
 	data, err := json.Marshal(CredentialsFile{
 		Aura: c.Aura,
 	})
@@ -63,4 +88,5 @@ func (c *Credentials) save() {
 	}
 
 	fileutils.WriteFile(c.fs, c.filePath, data)
+	return nil
 }

common/clicfg/credentials/credentials_test.go

@@ -0,0 +1,67 @@
+// Copyright (c) "Neo4j"
+// Neo4j Sweden AB [http://neo4j.com]
+
+package credentials_test
+
+import (
+	"testing"
+
+	"github.com/neo4j/cli/common/clicfg/credentials"
+	"github.com/spf13/afero"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestConcurrentSaveDoesNotLoseAnEarlierWrite(t *testing.T) {
+	fs := afero.NewMemMapFs()
+	const configPrefix = "/config"
+
+	// Two CLI processes starting at (roughly) the same time both load the
+	// same, initially-empty store.
+	processA := credentials.NewCredentials(fs, configPrefix)
+	processB := credentials.NewCredentials(fs, configPrefix)
+
+	// Process A finishes first: `credential add --name first ...`.
+	assert.NoError(t, processA.Aura.Add("first", "client-a", "secret-a"))
+
+	// Process B finishes second — e.g. a routine token refresh triggering a
+	// save as a side effect, or a second `credential add`. It never re-read
+	// the file, so its in-memory view still thinks the store is empty.
+	assert.NoError(t, processB.Aura.Add("second", "client-b", "secret-b"))
+
+	// A fresh read of the file is what the next command would see.
+	onDisk := credentials.NewCredentials(fs, configPrefix)
+	names := make([]string, 0, len(onDisk.Aura.List()))
+	for _, c := range onDisk.Aura.List() {
+		names = append(names, c.Name)
+	}
+
+	assert.ElementsMatch(t, []string{"first", "second"}, names,
+		"a save from one process must not silently erase a credential a concurrently-running process already saved")
+}
+
+func TestRemoveThenReadSameNameWithinProcessDoesNotLoseCredential(t *testing.T) {
+	fs := afero.NewMemMapFs()
+	const configPrefix = "/config"
+
+	// A single process:
+	// 1. Loads an empty store
+	cli := credentials.NewCredentials(fs, configPrefix)
+
+	// 2. Adds a credential
+	assert.NoError(t, cli.Aura.Add("test", "client-id", "client-secret"))
+
+	// 3. Removes it
+	assert.NoError(t, cli.Aura.Remove("test"))
+
+	// 4. Re-adds it with the same name (but new client ID/secret)
+	assert.NoError(t, cli.Aura.Add("test", "new-client-id", "new-client-secret"))
+
+	// 5. A fresh read of the file should have the credential
+	onDisk := credentials.NewCredentials(fs, configPrefix)
+	cred, err := onDisk.Aura.Get("test")
+
+	assert.NoError(t, err)
+	assert.NotNil(t, cred)
+	assert.Equal(t, "new-client-id", cred.ClientId)
+	assert.Equal(t, "new-client-secret", cred.ClientSecret)
+}