perf(users): rebuild user index with one shared scan and a bulk write

internal/users/charindex.go

@@ -69,19 +69,25 @@ func (ci *CharacterIndex) Find(name string) (userId int, found bool) {
 	return
 }
 
-// Rebuild clears the index and repopulates it from every user record on disk
+// Rebuild clears the index and repopulates it from every user file on disk
 // plus all currently online users. Only active character names are added here;
 // the alt-characters module is responsible for adding alt names after this
 // runs.
 func (ci *CharacterIndex) Rebuild() {
-	newMap := make(map[string]int)
+	ci.RebuildFromScan(ScanUserFiles())
+}
 
-	SearchOfflineUsers(func(u *UserRecord) bool {
-		if u.Character != nil && u.Character.Name != "" {
-			newMap[strings.ToLower(u.Character.Name)] = u.UserId
+// RebuildFromScan is Rebuild fed by an existing user file scan, so startup
+// can share one scan between the user index and the character index instead
+// of fully parsing every user record a second time.
+func (ci *CharacterIndex) RebuildFromScan(scan []UserFileScan) {
+	newMap := make(map[string]int, len(scan))
+
+	for _, s := range scan {
+		if s.CharacterName != "" {
+			newMap[strings.ToLower(s.CharacterName)] = s.UserId
 		}
-		return true
-	})
+	}
 
 	for _, u := range GetAllActiveUsers() {
 		if u.Character != nil && u.Character.Name != "" {

internal/users/charindex_test.go

@@ -4,6 +4,8 @@ import (
 	"fmt"
 	"sync"
 	"testing"
+
+	"github.com/GoMudEngine/GoMud/internal/mudlog"
 )
 
 func freshCharacterIndex() *CharacterIndex {
@@ -116,8 +118,12 @@ func TestCharacterIndex_MultipleUsersMultipleNames(t *testing.T) {
 }
 
 func TestCharacterIndex_Rebuild(t *testing.T) {
+	// The scan warns about the missing users directory in a test env, so the
+	// logger must be initialized.
+	mudlog.SetupLogger(nil, "", "", false)
+
 	// Swap in a fresh singleton so Rebuild exercises the real code path
-	// without touching disk (SearchOfflineUsers finds nothing in a test env).
+	// without touching disk (ScanUserFiles finds nothing in a test env).
 	orig := characterIndex
 	defer func() { characterIndex = orig }()
 
@@ -127,7 +133,7 @@ func TestCharacterIndex_Rebuild(t *testing.T) {
 	// Pre-populate with stale data that Rebuild should clear.
 	ci.Add("stale", 99)
 
-	// Rebuild will call SearchOfflineUsers (returns nothing in test env) and
+	// Rebuild will call ScanUserFiles (returns nothing in test env) and
 	// GetAllActiveUsers (returns nothing since userManager is empty). The stale
 	// entry must be gone.
 	ci.Rebuild()

internal/users/index.go

@@ -104,16 +104,19 @@ func (idx *UserIndex) loadRecords() {
 
 	f, err := os.Open(idx.Filename)
 	if err != nil {
+		mudlog.Error("UserIndex", "error", "failed to open index file", "path", idx.Filename, "details", err)
 		return
 	}
 	defer f.Close()
 
 	dataSize := idx.metaData.RecordCount * idx.metaData.RecordSize
 	buf := make([]byte, dataSize)
 	if _, err := f.Seek(int64(idx.metaData.MetaDataSize), io.SeekStart); err != nil {
+		mudlog.Error("UserIndex", "error", "failed to seek past index header", "path", idx.Filename, "details", err)
 		return
 	}
 	if _, err := io.ReadFull(f, buf); err != nil {
+		mudlog.Error("UserIndex", "error", "index file is truncated or corrupt, a rebuild will recreate it", "path", idx.Filename, "details", err)
 		return
 	}
 
@@ -209,85 +212,102 @@ func computeDirChecksum(basePath string) (uint64, error) {
 }
 
 // IsUpToDate returns true if the index file exists, has the current version,
-// and its stored FNV-64 checksum matches the current state of the user directory.
+// actually loaded every record its header claims, and its stored FNV-64
+// checksum matches the current state of the user directory.
 func (idx *UserIndex) IsUpToDate() bool {
+	basePath := util.FilePath(string(configs.GetFilePathsConfig().DataFiles), `/`, `users`)
+	return idx.isUpToDateForDir(basePath)
+}
+
+// isUpToDateForDir is IsUpToDate parameterized by the directory to compare
+// against, so tests can point it at a synthetic users directory.
+func (idx *UserIndex) isUpToDateForDir(basePath string) bool {
 	if !idx.Exists() {
 		return false
 	}
 	if idx.metaData.IndexVersion != IndexVersion {
 		return false
 	}
 
-	basePath := util.FilePath(string(configs.GetFilePathsConfig().DataFiles), `/`, `users`)
+	// A truncated or unreadable records section leaves fewer records in
+	// memory than the header claims. Such an index must never be trusted:
+	// with empty maps, GetUniqueUserId would start handing out userids that
+	// already belong to existing user files.
+	if uint64(len(idx.records)) != idx.metaData.RecordCount {
+		return false
+	}
+
 	current, err := computeDirChecksum(basePath)
 	if err != nil {
 		return false
 	}
 	return idx.metaData.Checksum == current
 }
 
-// Rebuild recreates the index from all offline user records.
-// It calls Create() internally so it is self-contained.
-// After building, it computes and persists a directory checksum so that
-// IsUpToDate can detect stale indexes on the next startup.
+// Rebuild recreates the index from the user files on disk. It runs a
+// lightweight scan (userid and username only) instead of fully loading every
+// user record, then writes the whole index in one atomic pass (temp file +
+// rename) with a single sync, instead of appending and syncing once per
+// user. The directory checksum is folded into that same write so IsUpToDate
+// can detect stale indexes on the next startup.
 func (idx *UserIndex) Rebuild() error {
-	if err := idx.Create(); err != nil {
-		return fmt.Errorf("index create failed: %w", err)
-	}
-
-	var firstErr error
-	SearchOfflineUsers(func(u *UserRecord) bool {
-		if err := idx.AddUser(u.UserId, u.Username); err != nil {
-			mudlog.Error("UserIndex.Rebuild", "error", err.Error(), "userId", u.UserId, "username", u.Username)
-			if firstErr == nil {
-				firstErr = err
-			}
-		}
-		return true
-	})
-
-	if firstErr != nil {
-		return firstErr
-	}
+	return idx.RebuildFromScan(ScanUserFiles())
+}
 
+// RebuildFromScan is Rebuild fed by an existing scan, so startup can share
+// one scan between the user index and the character index.
+func (idx *UserIndex) RebuildFromScan(scan []UserFileScan) error {
 	basePath := util.FilePath(string(configs.GetFilePathsConfig().DataFiles), `/`, `users`)
 	checksum, err := computeDirChecksum(basePath)
 	if err != nil {
 		return fmt.Errorf("checksum compute failed: %w", err)
 	}
-	if err := idx.writeChecksum(checksum); err != nil {
-		return fmt.Errorf("checksum write failed: %w", err)
-	}
-
-	return nil
+	return idx.applyScan(scan, checksum)
 }
 
-// writeChecksum persists a new checksum value into the index header on disk
-// and updates the in-memory metadata.
-func (idx *UserIndex) writeChecksum(checksum uint64) error {
+// applyScan replaces the in-memory records and lookup maps with the scan
+// results, then writes the complete index to disk once.
+func (idx *UserIndex) applyScan(scan []UserFileScan, checksum uint64) error {
+
+	records := make([]IndexUserRecord, 0, len(scan))
+	for _, s := range scan {
+		rec := IndexUserRecord{UserID: int64(s.UserId)}
+		copy(rec.Username[:], strings.ToLower(s.Username))
+		records = append(records, rec)
+	}
+
 	idx.mu.Lock()
 	defer idx.mu.Unlock()
 
-	idx.metaData.Checksum = checksum
-
-	headerBytes, err := idx.metaData.Format()
-	if err != nil {
-		return err
+	idx.metaData = IndexMetaData{
+		MetaDataSize: FixedHeaderTotalLength,
+		IndexVersion: IndexVersion,
+		RecordCount:  uint64(len(records)),
+		RecordSize:   IndexRecordSizeV1,
+		Checksum:     checksum,
 	}
 
-	f, err := os.OpenFile(idx.Filename, os.O_RDWR, 0644)
-	if err != nil {
-		return err
-	}
-	defer f.Close()
+	idx.records = records
+	idx.byUsername = make(map[string]int64, len(records))
+	idx.byUserId = make(map[int64]string, len(records))
+	idx.highestUserId = 0
 
-	if _, err := f.Seek(0, io.SeekStart); err != nil {
-		return err
+	for _, rec := range records {
+		username := string(bytes.TrimRight(rec.Username[:], "\x00"))
+		idx.byUsername[username] = rec.UserID
+		idx.byUserId[rec.UserID] = username
+		if int(rec.UserID) > idx.highestUserId {
+			idx.highestUserId = int(rec.UserID)
+		}
 	}
-	if _, err := f.Write(headerBytes); err != nil {
-		return err
+
+	// The in-memory state is updated even if the disk write fails - the
+	// running process must trust what was just scanned, not a stale file.
+	if err := idx.writeCompleteIndex(records); err != nil {
+		return fmt.Errorf("index write failed: %w", err)
 	}
-	return f.Sync()
+
+	return nil
 }
 
 func (idx *UserIndex) GetMetaData() IndexMetaData {
@@ -360,6 +380,7 @@ func (idx *UserIndex) getMetaDataFromFile() IndexMetaData {
 	headerContent := strings.TrimSpace(string(header[:FixedHeaderTotalLength-1]))
 	n, _ := fmt.Sscanf(headerContent, "VERSION=%d,RECORDCOUNT=%d,RECORDSIZE=%d,CHECKSUM=%d", &meta.IndexVersion, &meta.RecordCount, &meta.RecordSize, &meta.Checksum)
 	if n < 3 {
+		mudlog.Error("UserIndex", "error", "index header is unparseable, a rebuild will recreate it", "path", idx.Filename)
 		return IndexMetaData{}
 	}
 

internal/users/indexscan.go

@@ -0,0 +1,110 @@
+package users
+
+import (
+	"os"
+	"path/filepath"
+	"strconv"
+	"strings"
+
+	"github.com/GoMudEngine/GoMud/internal/configs"
+	"github.com/GoMudEngine/GoMud/internal/mudlog"
+	"github.com/GoMudEngine/GoMud/internal/util"
+	"gopkg.in/yaml.v2"
+)
+
+// UserFileScan holds the fields a lightweight pass over a user file yields.
+// The user index and the character index are both built from these at
+// startup, without paying for a full UserRecord unmarshal per file.
+type UserFileScan struct {
+	UserId        int
+	Username      string
+	CharacterName string
+}
+
+// userFileScanFields is the minimal unmarshal target for a scan. The file
+// is still lexed in full, but decoding into this instead of a full
+// UserRecord is measurably cheaper and builds no throwaway record - see
+// BenchmarkScanVsFullUnmarshal.
+type userFileScanFields struct {
+	UserId    int    `yaml:"userid"`
+	Username  string `yaml:"username"`
+	Character struct {
+		Name string `yaml:"name"`
+	} `yaml:"character"`
+}
+
+// ScanUserFiles runs a lightweight scan over the configured users directory.
+func ScanUserFiles() []UserFileScan {
+	basePath := util.FilePath(string(configs.GetFilePathsConfig().DataFiles), `/`, `users`)
+	return scanUserFilesInDir(basePath)
+}
+
+// scanUserFilesInDir reads the userid, username, and active character name
+// of every user file under basePath. Alt files are never opened. Files that
+// cannot be read or parsed are skipped with a warning instead of aborting
+// the scan, and anomalies that usually mean hand-edited data (duplicate
+// userids, duplicate usernames, a numeric filename that disagrees with the
+// userid inside the file) are logged so they get noticed.
+func scanUserFilesInDir(basePath string) []UserFileScan {
+
+	results := []UserFileScan{}
+	seenIds := make(map[int]string)
+	seenNames := make(map[string]string)
+
+	filepath.Walk(basePath, func(path string, info os.FileInfo, err error) error {
+
+		if err != nil {
+			mudlog.Warn("ScanUserFiles", "path", path, "walk_error", err)
+			return nil
+		}
+
+		if info.IsDir() {
+			return nil
+		}
+
+		if !strings.HasSuffix(path, `.yaml`) || strings.HasSuffix(path, `.alts.yaml`) {
+			return nil
+		}
+
+		fileBytes, err := os.ReadFile(path)
+		if err != nil {
+			mudlog.Warn("ScanUserFiles", "path", path, "read_error", err)
+			return nil
+		}
+
+		var scanned userFileScanFields
+		if err := yaml.Unmarshal(fileBytes, &scanned); err != nil {
+			mudlog.Warn("ScanUserFiles", "path", path, "unmarshal_error", err)
+			return nil
+		}
+
+		if scanned.UserId < 1 || scanned.Username == `` {
+			mudlog.Warn("ScanUserFiles", "info", "skipping user file missing userid or username", "path", path)
+			return nil
+		}
+
+		if fileId, convErr := strconv.Atoi(strings.TrimSuffix(filepath.Base(path), `.yaml`)); convErr == nil && fileId != scanned.UserId {
+			mudlog.Warn("ScanUserFiles", "info", "filename does not match userid in file", "path", path, "userid", scanned.UserId)
+		}
+
+		if otherPath, ok := seenIds[scanned.UserId]; ok {
+			mudlog.Warn("ScanUserFiles", "info", "duplicate userid", "userid", scanned.UserId, "path", path, "otherpath", otherPath)
+		}
+		lowerName := strings.ToLower(scanned.Username)
+		if otherPath, ok := seenNames[lowerName]; ok {
+			mudlog.Warn("ScanUserFiles", "info", "duplicate username", "username", scanned.Username, "path", path, "otherpath", otherPath)
+		}
+		seenIds[scanned.UserId] = path
+		seenNames[lowerName] = path
+
+		results = append(results, UserFileScan{
+			UserId:        scanned.UserId,
+			Username:      scanned.Username,
+			CharacterName: scanned.Character.Name,
+		})
+
+		return nil
+	})
+
+	return results
+}