Recover from dup active profiles that cannot be resolved with username.

client/internal/profilemanager/error.go

@@ -6,4 +6,9 @@ var (
 	ErrProfileNotFound      = errors.New("profile not found")
 	ErrProfileAlreadyExists = errors.New("profile already exists")
 	ErrNoActiveProfile      = errors.New("no active profile set")
+
+	// ErrAmbiguousActiveProfile is returned when the active profile state names
+	// an ID that several profiles hold and does not say whose directory the
+	// active one sits in.
+	ErrAmbiguousActiveProfile = errors.New("active profile is ambiguous")
 )

client/internal/profilemanager/migration.go

@@ -42,7 +42,8 @@ func (s *ServiceManager) MigrateLegacyProfiles() error {
 		return fmt.Errorf("active profile state: %w", err)
 	}
 
-	if err := s.rekeyDuplicateIDs(profiles, active); err != nil {
+	unresolved, err := s.rekeyDuplicateIDs(profiles, active)
+	if err != nil {
 		return err
 	}
 
@@ -50,6 +51,11 @@ func (s *ServiceManager) MigrateLegacyProfiles() error {
 		return err
 	}
 
+	if unresolved != "" {
+		return fmt.Errorf("%w: %q is still held by more than one profile, the active profile state does not say which one is active",
+			ErrAmbiguousActiveProfile, unresolved)
+	}
+
 	if err := os.MkdirAll(dest, 0700); err != nil {
 		return fmt.Errorf("create shared profile directory: %w", err)
 	}
@@ -61,7 +67,9 @@ func (s *ServiceManager) MigrateLegacyProfiles() error {
 // rekeyDuplicateIDs gives every profile sharing an ID a fresh one, in place.
 // The file stays in its directory, only its name changes, so nothing that holds
 // a path to a sibling file is disturbed.
-func (s *ServiceManager) rekeyDuplicateIDs(profiles []Profile, active *ActiveProfileState) error {
+//
+// It returns the one ID it could not settle, empty when it settled them all.
+func (s *ServiceManager) rekeyDuplicateIDs(profiles []Profile, active *ActiveProfileState) (ID, error) {
 	groups := make(map[ID][]*Profile, len(profiles))
 	for i := range profiles {
 		p := &profiles[i]
@@ -71,6 +79,7 @@ func (s *ServiceManager) rekeyDuplicateIDs(profiles []Profile, active *ActivePro
 		groups[p.ID] = append(groups[p.ID], p)
 	}
 
+	var unresolved ID
 	activeDir := sanitizeProfileName(active.Username)
 	for id, group := range groups {
 		if len(group) < 2 {
@@ -84,6 +93,7 @@ func (s *ServiceManager) rekeyDuplicateIDs(profiles []Profile, active *ActivePro
 		// way.
 		if id == active.ID && activeDir == "" {
 			log.Warnf("leaving %d profiles named %q as they are, the active profile state does not say which one is active", len(group), id)
+			unresolved = id
 			continue
 		}
 
@@ -92,25 +102,34 @@ func (s *ServiceManager) rekeyDuplicateIDs(profiles []Profile, active *ActivePro
 
 			fresh, err := generateProfileID()
 			if err != nil {
-				return fmt.Errorf("generate profile ID: %w", err)
+				return "", fmt.Errorf("generate profile ID: %w", err)
 			}
 			if err := rekeyProfile(p, fresh); err != nil {
-				return err
+				return "", err
 			}
 
 			if wasActive {
 				active.ID = fresh
 				if err := s.SetActiveProfileState(active); err != nil {
-					return fmt.Errorf("repoint active profile: %w", err)
+					return "", fmt.Errorf("repoint active profile: %w", err)
 				}
 			}
 		}
 	}
 
-	return nil
+	return unresolved, nil
 }
 
-// rekeyProfile renames a profile and its sidecars to a fresh ID.
+// renameFile is os.Rename, replaced in tests that need a rename to fail.
+var renameFile = os.Rename
+
+// movedFile is a completed rename, kept so it can be undone.
+type movedFile struct{ at, was string }
+
+// rekeyProfile renames a profile and its state file to a fresh ID.
+//
+// The state file move first and the profile file last, we try to restore if
+// renaming the profile file.
 func rekeyProfile(p *Profile, fresh ID) error {
 	// A legacy profile's display name is its filename, so it has to be in the
 	// file before the filename stops meaning anything. The loader already
@@ -121,19 +140,25 @@ func rekeyProfile(p *Profile, fresh ID) error {
 	}
 
 	dir := filepath.Dir(p.Path)
-	target := filepath.Join(dir, fresh.String()+".json")
-	if err := os.Rename(p.Path, target); err != nil {
-		return fmt.Errorf("rekey %s: %w", p.ID, err)
-	}
 
+	var moved []movedFile
 	for _, suffix := range []string{stateFileSuffix, prefsFileSuffix} {
 		src := filepath.Join(dir, p.ID.String()+suffix)
 		if _, err := os.Stat(src); err != nil {
 			continue
 		}
-		if err := os.Rename(src, filepath.Join(dir, fresh.String()+suffix)); err != nil {
-			log.Warnf("could not rename %s alongside its profile: %v", src, err)
+		dst := filepath.Join(dir, fresh.String()+suffix)
+		if err := renameFile(src, dst); err != nil {
+			undoMoves(moved)
+			return fmt.Errorf("rekey %s alongside profile %s: %w", filepath.Base(src), p.ID, err)
 		}
+		moved = append(moved, movedFile{at: dst, was: src})
+	}
+
+	target := filepath.Join(dir, fresh.String()+".json")
+	if err := renameFile(p.Path, target); err != nil {
+		undoMoves(moved)
+		return fmt.Errorf("rekey %s: %w", p.ID, err)
 	}
 
 	log.Infof("profile %q in %s now has the unique ID %s", p.ID, dir, fresh)
@@ -141,6 +166,16 @@ func rekeyProfile(p *Profile, fresh ID) error {
 	return nil
 }
 
+// undoMoves puts back what a half-finished rekey moved, so the next start finds
+// the profile as this one did and can rekey it from scratch.
+func undoMoves(moved []movedFile) {
+	for _, m := range moved {
+		if err := renameFile(m.at, m.was); err != nil {
+			log.Errorf("could not move %s back to %s after a failed rekey, it is now orphaned: %v", m.at, m.was, err)
+		}
+	}
+}
+
 // stampActiveUserDir records the owner of every unowned profile in the
 // directory of the account the active profile state names.
 //

client/internal/profilemanager/migration_test.go

@@ -1,11 +1,13 @@
 package profilemanager
 
 import (
+	"errors"
 	"os"
 	"os/user"
 	"path/filepath"
 	"runtime"
 	"strconv"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -119,3 +121,108 @@ func TestMigrate_UnresolvableAccountLeavesNoMarker(t *testing.T) {
 			"an unfinished run leaves no marker, so the next start tries again")
 	})
 }
+
+// failRenamesOf makes every rename of a file with the given suffix fail, and
+// returns the function that lets them through again.
+func failRenamesOf(t *testing.T, suffix string) func() {
+	t.Helper()
+
+	orig := renameFile
+	failing := true
+	renameFile = func(from, to string) error {
+		if failing && strings.HasSuffix(from, suffix) {
+			return errors.New("simulated rename failure")
+		}
+		return orig(from, to)
+	}
+	t.Cleanup(func() { renameFile = orig })
+
+	return func() { failing = false }
+}
+
+func TestMigrate_AStateFileThatCannotFollowRollsBackTheRekey(t *testing.T) {
+	username, _ := currentUserPrincipal(t)
+
+	withLegacyLayout(t, func(sm *ServiceManager, configDir string) {
+		dir := sanitizeProfileName(username)
+		mine := writeLegacyProfile(t, configDir, dir, "work", nil)
+		writeLegacyProfile(t, configDir, "someone-else", "work", nil)
+		state := filepath.Join(configDir, dir, "work"+stateFileSuffix)
+		require.NoError(t, os.WriteFile(state, []byte("{}"), 0600))
+		require.NoError(t, os.WriteFile(filepath.Join(configDir, dir, "work"+prefsFileSuffix), []byte("{}"), 0600))
+		require.NoError(t, sm.SetActiveProfileState(&ActiveProfileState{ID: "work", Username: username}))
+
+		allowRenames := failRenamesOf(t, prefsFileSuffix)
+
+		require.Error(t, sm.MigrateLegacyProfiles())
+		assert.NoDirExists(t, filepath.Join(configDir, DefaultProfilePathDir),
+			"a rekey that could not finish leaves no marker, so the next start tries again")
+		assert.FileExists(t, mine, "the profile is back under the ID its state file still carry")
+		assert.FileExists(t, state, "and so is the state file that had already moved")
+
+		// The next start finds the profile exactly as the failed one did, and
+		// the state file are still beside it once the rekey goes through.
+		allowRenames()
+		require.NoError(t, sm.MigrateLegacyProfiles())
+
+		profiles, err := sm.loadAllProfiles()
+		require.NoError(t, err)
+
+		ids := map[ID]int{}
+		var rolledBack *Profile
+		for i := range profiles {
+			p := &profiles[i]
+			ids[p.ID]++
+			if p.ID != defaultProfileName && filepath.Dir(p.Path) == filepath.Join(configDir, dir) {
+				rolledBack = p
+			}
+		}
+		for id, n := range ids {
+			assert.Equal(t, 1, n, "%s is still shared after migration", id)
+		}
+
+		require.NotNil(t, rolledBack)
+		for _, suffix := range []string{stateFileSuffix, prefsFileSuffix} {
+			assert.FileExists(t, filepath.Join(configDir, dir, rolledBack.ID.String()+suffix),
+				"%s follows the profile it belongs to", suffix)
+		}
+	})
+}
+
+func TestMigrate_NamesakesTheStateCannotTellApartLeaveNoMarker(t *testing.T) {
+	username, _ := currentUserPrincipal(t)
+
+	withLegacyLayout(t, func(sm *ServiceManager, configDir string) {
+		dir := sanitizeProfileName(username)
+		mine := writeLegacyProfile(t, configDir, dir, "work", nil)
+		theirs := writeLegacyProfile(t, configDir, "someone-else", "work", nil)
+		require.NoError(t, sm.SetActiveProfileState(&ActiveProfileState{ID: "work"}))
+
+		require.ErrorIs(t, sm.MigrateLegacyProfiles(), ErrAmbiguousActiveProfile)
+		assert.NoDirExists(t, filepath.Join(configDir, DefaultProfilePathDir),
+			"the marker would retire the only pass that can still separate them")
+		assert.FileExists(t, mine, "so both namesakes are left as they are")
+		assert.FileExists(t, theirs)
+
+		// And until they are separated the active profile does not resolve to
+		// whichever of them happened to sort first.
+		state, err := sm.GetActiveProfileState()
+		require.NoError(t, err)
+		_, err = sm.ActiveProfilePath(state)
+		require.ErrorIs(t, err, ErrAmbiguousActiveProfile)
+
+		// Selecting a profile records the directory that tells them apart, and
+		// the start after that finishes the job.
+		require.NoError(t, sm.SetActiveProfileState(&ActiveProfileState{ID: "work", Username: username}))
+		require.NoError(t, sm.MigrateLegacyProfiles())
+		assert.DirExists(t, filepath.Join(configDir, DefaultProfilePathDir))
+
+		state, err = sm.GetActiveProfileState()
+		require.NoError(t, err)
+		active, err := sm.ActiveProfilePath(state)
+		require.NoError(t, err)
+		assert.NotEqual(t, ID("work"), state.ID, "the namesakes are separated")
+		assert.Equal(t, dir, filepath.Base(filepath.Dir(active)),
+			"and the active one still sits in the account that was running it")
+	})
+}

client/internal/profilemanager/service.go

@@ -206,9 +206,9 @@ func (s *ServiceManager) ActiveProfilePath(a *ActiveProfileState) (string, error
 		}
 	}
 
-	log.Warnf("%d profiles share the ID %q and none of them sits in %q, using %s",
-		len(matches), a.ID, a.Username, matches[0].Path)
-	return matches[0].Path, nil
+	// Nothing left to tell them apart, so this fails rather than guesses.
+	return "", fmt.Errorf("%w: %d profiles hold the ID %q and the active profile state does not say which account's directory it is in",
+		ErrAmbiguousActiveProfile, len(matches), a.ID)
 }
 
 func NewServiceManager(defaultConfigPath string) *ServiceManager {

client/internal/profilemanager/service_test.go

@@ -643,3 +643,19 @@ func TestListProfiles_UnidentifiedCallerGetsNothing(t *testing.T) {
 		assert.Empty(t, got)
 	})
 }
+
+func TestActiveProfilePath_RefusesToGuessBetweenNamesakes(t *testing.T) {
+	withLegacyLayout(t, func(sm *ServiceManager, configDir string) {
+		writeLegacyProfile(t, configDir, "alice", "work", nil)
+		writeLegacyProfile(t, configDir, "bob", "work", nil)
+
+		_, err := sm.ActiveProfilePath(&ActiveProfileState{ID: "work"})
+		require.ErrorIs(t, err, ErrAmbiguousActiveProfile,
+			"running one account's config under another account's name is worse than not running")
+
+		path, err := sm.ActiveProfilePath(&ActiveProfileState{ID: "work", Username: "bob"})
+		require.NoError(t, err)
+		assert.Equal(t, filepath.Join(configDir, "bob", "work.json"), path,
+			"the recorded directory is what tells the namesakes apart")
+	})
+}

client/server/server.go

@@ -301,6 +301,14 @@ func (s *Server) Start() error {
 	}
 
 	config, existingConfig, err := s.getConfig(activeProf)
+	if errors.Is(err, profilemanager.ErrAmbiguousActiveProfile) {
+		// Running one of the namesakes anyway could connect the machine as
+		// another users profile, so the daemon comes up on the default
+		// profile instead of refusing to start.
+		log.Errorf("starting on the default profile, the active one could not be resolved: %v", err)
+		activeProf = &profilemanager.ActiveProfileState{ID: profilemanager.DefaultProfileName}
+		config, existingConfig, err = s.getConfig(activeProf)
+	}
 	if err != nil {
 		log.Errorf("failed to get active profile config: %v", err)