From f011000323b3f51115f682f38bd9cbbf18f21d89 Mon Sep 17 00:00:00 2001 From: "Theodor S. Midtlien" Date: Wed, 16 Sep 2026 09:44:50 +0200 Subject: [PATCH] Recover from dup active profiles that cannot be resolved with username. --- client/internal/profilemanager/error.go | 5 + client/internal/profilemanager/migration.go | 61 +++++++--- .../internal/profilemanager/migration_test.go | 107 ++++++++++++++++++ client/internal/profilemanager/service.go | 6 +- .../internal/profilemanager/service_test.go | 16 +++ client/server/server.go | 8 ++ 6 files changed, 187 insertions(+), 16 deletions(-) diff --git a/client/internal/profilemanager/error.go b/client/internal/profilemanager/error.go index d83fe5c1c..4f896a5f4 100644 --- a/client/internal/profilemanager/error.go +++ b/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") ) diff --git a/client/internal/profilemanager/migration.go b/client/internal/profilemanager/migration.go index e0057cd62..0cc1855da 100644 --- a/client/internal/profilemanager/migration.go +++ b/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. // diff --git a/client/internal/profilemanager/migration_test.go b/client/internal/profilemanager/migration_test.go index 207fc8027..c4ac76e8d 100644 --- a/client/internal/profilemanager/migration_test.go +++ b/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") + }) +} diff --git a/client/internal/profilemanager/service.go b/client/internal/profilemanager/service.go index d171f5c2e..b1c66d12a 100644 --- a/client/internal/profilemanager/service.go +++ b/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 { diff --git a/client/internal/profilemanager/service_test.go b/client/internal/profilemanager/service_test.go index 4eca3f714..936ec5a39 100644 --- a/client/internal/profilemanager/service_test.go +++ b/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") + }) +} diff --git a/client/server/server.go b/client/server/server.go index 7a23e2e6d..8dc1a9110 100644 --- a/client/server/server.go +++ b/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)