Merge pull request #269 from gkatz2/feature/targeted-user-cache-patch

Add targeted user cache patch on miss
2026-09-18 23:43:49 +08:00 · 2026-05-14 23:01:12 +02:00
parent 400c900774 2d19f8c752
commit 342e3e3cd2
3 changed files with 261 additions and 19 deletions
@@ -555,7 +555,7 @@ func (ch *ConversationsHandler) ConversationsHistoryHandler(ctx context.Context,

 	ch.logger.Debug("Fetched conversation history", zap.Int("message_count", len(history.Messages)))

-	messages := ch.convertMessagesFromHistory(history.Messages, params.channel, params.activity)
+	messages := ch.convertMessagesFromHistory(ctx, history.Messages, params.channel, params.activity)

 	if len(messages) > 0 && history.HasMore {
 		messages[len(messages)-1].Cursor = history.ResponseMetaData.NextCursor
@@ -594,7 +594,7 @@ func (ch *ConversationsHandler) ConversationsRepliesHandler(ctx context.Context,
 	}
 	ch.logger.Debug("Fetched conversation replies", zap.Int("count", len(replies)))

-	messages := ch.convertMessagesFromHistory(replies, params.channel, params.activity)
+	messages := ch.convertMessagesFromHistory(ctx, replies, params.channel, params.activity)
 	if len(messages) > 0 && hasMore {
 		messages[len(messages)-1].Cursor = nextCursor
 	}
@@ -604,7 +604,7 @@ func (ch *ConversationsHandler) ConversationsRepliesHandler(ctx context.Context,
 func (ch *ConversationsHandler) ConversationsSearchHandler(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) {
 	ch.logger.Debug("ConversationsSearchHandler called", zap.Any("params", request.Params))

-	params, err := ch.parseParamsToolSearch(request)
+	params, err := ch.parseParamsToolSearch(ctx, request)
 	if err != nil {
 		ch.logger.Error("Failed to parse search params", zap.Error(err))
 		return nil, err
@@ -630,7 +630,7 @@ func (ch *ConversationsHandler) ConversationsSearchHandler(ctx context.Context,
 	}
 	ch.logger.Debug("Search completed", zap.Int("matches", len(messagesRes.Matches)))

-	messages := ch.convertMessagesFromSearch(messagesRes.Matches)
+	messages := ch.convertMessagesFromSearch(ctx, messagesRes.Matches)
 	if len(messages) > 0 && messagesRes.Pagination.Page < messagesRes.Pagination.PageCount {
 		nextCursor := fmt.Sprintf("page:%d", messagesRes.Pagination.Page+1)
 		messages[len(messages)-1].Cursor = base64.StdEncoding.EncodeToString([]byte(nextCursor))
@@ -918,7 +918,7 @@ func (ch *ConversationsHandler) processClientCountsResponse(ctx context.Context,
 		unreadChannels[i].UnreadCount = len(history.Messages)

 		// Convert messages
-		channelMessages := ch.convertMessagesFromHistory(history.Messages, unreadChannels[i].ChannelName, false)
+		channelMessages := ch.convertMessagesFromHistory(ctx, history.Messages, unreadChannels[i].ChannelName, false)
 		allMessages = append(allMessages, channelMessages...)
 	}

@@ -1044,7 +1044,7 @@ func (ch *ConversationsHandler) getUnreadsViaConversationsInfo(ctx context.Conte
 			continue
 		}

-		channelMessages := ch.convertMessagesFromHistory(history.Messages, uc.ChannelName, false)
+		channelMessages := ch.convertMessagesFromHistory(ctx, history.Messages, uc.ChannelName, false)
 		allMessages = append(allMessages, channelMessages...)
 	}

@@ -1535,8 +1535,8 @@ func (ch *ConversationsHandler) resolveChannelID(ctx context.Context, channel st
 	return channelsMaps.Channels[chn].ID, nil
 }

-func (ch *ConversationsHandler) convertMessagesFromHistory(slackMessages []slack.Message, channel string, includeActivity bool) []Message {
-	usersMap := ch.apiProvider.ProvideUsersMap()
+func (ch *ConversationsHandler) convertMessagesFromHistory(ctx context.Context, slackMessages []slack.Message, channel string, includeActivity bool) []Message {
+	resolver := ch.newUserResolver(ctx)
 	var messages []Message
 	warn := false

@@ -1545,7 +1545,7 @@ func (ch *ConversationsHandler) convertMessagesFromHistory(slackMessages []slack
 			continue
 		}

-		userName, realName, ok := getUserInfo(msg.User, usersMap.Users)
+		userName, realName, ok := resolver.resolve(msg.User)

 		if !ok && msg.SubType == "bot_message" {
 			userName, realName, ok = getBotInfo(msg.Username)
@@ -1622,17 +1622,19 @@ func (ch *ConversationsHandler) convertMessagesFromHistory(slackMessages []slack
 	return messages
 }

-func (ch *ConversationsHandler) convertMessagesFromSearch(slackMessages []slack.SearchMessage) []Message {
-	usersMap := ch.apiProvider.ProvideUsersMap()
+func (ch *ConversationsHandler) convertMessagesFromSearch(ctx context.Context, slackMessages []slack.SearchMessage) []Message {
+	resolver := ch.newUserResolver(ctx)
 	var messages []Message
 	warn := false

 	for _, msg := range slackMessages {
-		userName, realName, ok := getUserInfo(msg.User, usersMap.Users)
+		userName, realName, ok := resolver.resolve(msg.User)

 		if !ok && msg.User == "" && msg.Username != "" {
 			userName, realName, ok = getBotInfo(msg.Username)
-		} else if !ok {
+		}
+
+		if !ok {
 			warn = true
 		}

@@ -1989,7 +1991,7 @@ func (ch *ConversationsHandler) parseParamsToolMark(request mcp.CallToolRequest)
 		ts:      ts,
 	}, nil
 }
-func (ch *ConversationsHandler) parseParamsToolSearch(req mcp.CallToolRequest) (*searchParams, error) {
+func (ch *ConversationsHandler) parseParamsToolSearch(ctx context.Context, req mcp.CallToolRequest) (*searchParams, error) {
 	rawQuery := strings.TrimSpace(req.GetString("search_query", ""))
 	freeText, filters := splitQuery(rawQuery)

@@ -2004,7 +2006,7 @@ func (ch *ConversationsHandler) parseParamsToolSearch(req mcp.CallToolRequest) (
 		}
 		addFilter(filters, "in", f)
 	} else if im := req.GetString("filter_in_im_or_mpim", ""); im != "" {
-		f, err := ch.paramFormatUser(im)
+		f, err := ch.paramFormatUser(ctx, im)
 		if err != nil {
 			ch.logger.Error("Invalid IM/MPIM filter", zap.String("filter", im), zap.Error(err))
 			return nil, err
@@ -2012,7 +2014,7 @@ func (ch *ConversationsHandler) parseParamsToolSearch(req mcp.CallToolRequest) (
 		addFilter(filters, "in", f)
 	}
 	if with := req.GetString("filter_users_with", ""); with != "" {
-		f, err := ch.paramFormatUser(with)
+		f, err := ch.paramFormatUser(ctx, with)
 		if err != nil {
 			ch.logger.Error("Invalid with-user filter", zap.String("filter", with), zap.Error(err))
 			return nil, err
@@ -2020,7 +2022,7 @@ func (ch *ConversationsHandler) parseParamsToolSearch(req mcp.CallToolRequest) (
 		addFilter(filters, "with", f)
 	}
 	if from := req.GetString("filter_users_from", ""); from != "" {
-		f, err := ch.paramFormatUser(from)
+		f, err := ch.paramFormatUser(ctx, from)
 		if err != nil {
 			ch.logger.Error("Invalid from-user filter", zap.String("filter", from), zap.Error(err))
 			return nil, err
@@ -2087,13 +2089,20 @@ func isSlackUserIDPrefix(s string) bool {
 	return strings.HasPrefix(s, "U") || strings.HasPrefix(s, "W")
 }

-func (ch *ConversationsHandler) paramFormatUser(raw string) (string, error) {
+func (ch *ConversationsHandler) paramFormatUser(ctx context.Context, raw string) (string, error) {
 	users := ch.apiProvider.ProvideUsersMap()
 	raw = strings.TrimSpace(raw)
 	if isSlackUserIDPrefix(raw) {
 		u, ok := users.Users[raw]
 		if !ok {
-			return "", fmt.Errorf("user %q not found", raw)
+			// Targeted fetch: single users.info call instead of full cache rebuild
+			patched, err := ch.apiProvider.PatchUser(ctx, raw)
+			if err != nil {
+				ch.logger.Debug("Targeted user fetch failed, user not found",
+					zap.String("user_id", raw), zap.Error(err))
+				return "", fmt.Errorf("user %q not found", raw)
+			}
+			return fmt.Sprintf("<@%s>", patched.ID), nil
 		}
 		return fmt.Sprintf("<@%s>", u.ID), nil
 	}
@@ -2144,6 +2153,42 @@ func getUserInfo(userID string, usersMap map[string]slack.User) (userName, realN
 	return userID, userID, false
 }

+// userResolver resolves user IDs to names, fetching unknown users from the
+// Slack API on demand. It caches the snapshot locally and remembers which IDs
+// it already tried to fetch, so a user that doesn't exist in Slack is only
+// looked up once per batch rather than once per message.
+type userResolver struct {
+	apiProvider  *provider.ApiProvider
+	ctx          context.Context
+	usersMap     *provider.UsersCache
+	attemptedIDs map[string]bool
+}
+
+func (ch *ConversationsHandler) newUserResolver(ctx context.Context) *userResolver {
+	return &userResolver{
+		apiProvider:  ch.apiProvider,
+		ctx:          ctx,
+		usersMap:     ch.apiProvider.ProvideUsersMap(),
+		attemptedIDs: make(map[string]bool),
+	}
+}
+
+func (r *userResolver) resolve(userID string) (userName, realName string, ok bool) {
+	if u, ok := r.usersMap.Users[userID]; ok {
+		return u.Name, u.RealName, true
+	}
+	if userID == "" || r.attemptedIDs[userID] {
+		return userID, userID, false
+	}
+	r.attemptedIDs[userID] = true
+	patched, err := r.apiProvider.PatchUser(r.ctx, userID)
+	if err != nil {
+		return userID, userID, false
+	}
+	r.usersMap = r.apiProvider.ProvideUsersMap()
+	return patched.Name, patched.RealName, true
+}
+
 func getBotInfo(botID string) (userName, realName string, ok bool) {
 	return botID, botID, true
 }
@@ -832,6 +832,45 @@ func (ap *ApiProvider) ForceRefreshUsers(ctx context.Context) error {
 	return ap.refreshUsersInternal(ctx, true)
 }

+// PatchUser fetches a single user by ID from the Slack API and adds them to
+// the in-memory users snapshot. This is much cheaper than a full cache rebuild
+// for a single cache miss (O(1) API call vs O(all users)).
+// Disk persistence is skipped — the next full refresh will persist the entry.
+func (ap *ApiProvider) PatchUser(ctx context.Context, userID string) (*slack.User, error) {
+	usersInfo, err := ap.client.GetUsersInfo(userID)
+	if err != nil {
+		ap.logger.Warn("Failed to fetch user for cache patch", zap.String("user_id", userID), zap.Error(err))
+		return nil, err
+	}
+	if usersInfo == nil || len(*usersInfo) == 0 {
+		ap.logger.Debug("User not found via API", zap.String("user_id", userID))
+		return nil, errors.New("user not found")
+	}
+
+	user := (*usersInfo)[0]
+	current := ap.usersSnapshot.Load()
+
+	newSnapshot := &UsersCache{
+		Users:    make(map[string]slack.User, len(current.Users)+1),
+		UsersInv: make(map[string]string, len(current.UsersInv)+1),
+	}
+	for k, v := range current.Users {
+		newSnapshot.Users[k] = v
+	}
+	for k, v := range current.UsersInv {
+		newSnapshot.UsersInv[k] = v
+	}
+	newSnapshot.Users[user.ID] = user
+	newSnapshot.UsersInv[user.Name] = user.ID
+
+	ap.usersSnapshot.Store(newSnapshot)
+	ap.logger.Debug("Patched user into cache",
+		zap.String("user_id", user.ID),
+		zap.String("user_name", user.Name))
+
+	return &user, nil
+}
+
 func (ap *ApiProvider) refreshUsersInternal(ctx context.Context, force bool) error {
 	ap.usersMu.Lock()

@@ -0,0 +1,158 @@
+package provider
+
+import (
+	"context"
+	"errors"
+	"sync/atomic"
+	"testing"
+
+	"github.com/slack-go/slack"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"go.uber.org/zap"
+)
+
+// mockSlackClient implements just enough of SlackAPI for PatchUser tests.
+type mockSlackClient struct {
+	SlackAPI // embed interface to satisfy all methods; only override what we need
+
+	usersInfoResult *[]slack.User
+	usersInfoErr    error
+}
+
+func (m *mockSlackClient) GetUsersInfo(users ...string) (*[]slack.User, error) {
+	return m.usersInfoResult, m.usersInfoErr
+}
+
+func newTestApiProvider(client SlackAPI, snapshot *UsersCache) *ApiProvider {
+	ap := &ApiProvider{
+		client: client,
+		logger: zap.NewNop(),
+	}
+	ap.usersSnapshot.Store(snapshot)
+	return ap
+}
+
+// TestUnitPatchUser verifies the targeted single-user cache patch behavior.
+func TestUnitPatchUser(t *testing.T) {
+	t.Run("fetches and adds new user to snapshot", func(t *testing.T) {
+		initial := &UsersCache{
+			Users:    map[string]slack.User{"U001": {ID: "U001", Name: "alice"}},
+			UsersInv: map[string]string{"alice": "U001"},
+		}
+
+		newUser := slack.User{ID: "U002", Name: "bob"}
+		ap := newTestApiProvider(
+			&mockSlackClient{usersInfoResult: &[]slack.User{newUser}},
+			initial,
+		)
+
+		result, err := ap.PatchUser(context.Background(), "U002")
+		require.NoError(t, err)
+		assert.Equal(t, "U002", result.ID)
+		assert.Equal(t, "bob", result.Name)
+
+		snapshot := ap.usersSnapshot.Load()
+		assert.Len(t, snapshot.Users, 2)
+		assert.Equal(t, "bob", snapshot.Users["U002"].Name)
+		assert.Equal(t, "U002", snapshot.UsersInv["bob"])
+		assert.Equal(t, "alice", snapshot.Users["U001"].Name)
+	})
+
+	t.Run("API error leaves snapshot unchanged", func(t *testing.T) {
+		initial := &UsersCache{
+			Users:    map[string]slack.User{"U001": {ID: "U001", Name: "alice"}},
+			UsersInv: map[string]string{"alice": "U001"},
+		}
+
+		ap := newTestApiProvider(
+			&mockSlackClient{usersInfoErr: errors.New("slack API error")},
+			initial,
+		)
+
+		result, err := ap.PatchUser(context.Background(), "U999")
+		assert.Error(t, err)
+		assert.Nil(t, result)
+
+		snapshot := ap.usersSnapshot.Load()
+		assert.Len(t, snapshot.Users, 1)
+	})
+
+	t.Run("empty API result returns not found", func(t *testing.T) {
+		initial := &UsersCache{
+			Users:    map[string]slack.User{},
+			UsersInv: map[string]string{},
+		}
+
+		ap := newTestApiProvider(
+			&mockSlackClient{usersInfoResult: &[]slack.User{}},
+			initial,
+		)
+
+		result, err := ap.PatchUser(context.Background(), "U999")
+		assert.Error(t, err)
+		assert.Nil(t, result)
+		assert.Contains(t, err.Error(), "not found")
+	})
+
+	t.Run("nil API result returns not found", func(t *testing.T) {
+		initial := &UsersCache{
+			Users:    map[string]slack.User{},
+			UsersInv: map[string]string{},
+		}
+
+		ap := newTestApiProvider(
+			&mockSlackClient{usersInfoResult: nil},
+			initial,
+		)
+
+		result, err := ap.PatchUser(context.Background(), "U999")
+		assert.Error(t, err)
+		assert.Nil(t, result)
+	})
+
+	t.Run("does not mutate original snapshot", func(t *testing.T) {
+		initial := &UsersCache{
+			Users:    map[string]slack.User{"U001": {ID: "U001", Name: "alice"}},
+			UsersInv: map[string]string{"alice": "U001"},
+		}
+
+		var snapshotRef atomic.Pointer[UsersCache]
+		snapshotRef.Store(initial)
+
+		newUser := slack.User{ID: "U002", Name: "bob"}
+		ap := newTestApiProvider(
+			&mockSlackClient{usersInfoResult: &[]slack.User{newUser}},
+			initial,
+		)
+
+		_, err := ap.PatchUser(context.Background(), "U002")
+		require.NoError(t, err)
+
+		orig := snapshotRef.Load()
+		_, hasNew := orig.Users["U002"]
+		assert.False(t, hasNew, "original snapshot should not be mutated")
+		assert.Len(t, orig.Users, 1)
+	})
+
+	t.Run("overwrites existing user with fresh data", func(t *testing.T) {
+		initial := &UsersCache{
+			Users:    map[string]slack.User{"U001": {ID: "U001", Name: "alice_old"}},
+			UsersInv: map[string]string{"alice_old": "U001"},
+		}
+
+		updatedUser := slack.User{ID: "U001", Name: "alice_new"}
+		ap := newTestApiProvider(
+			&mockSlackClient{usersInfoResult: &[]slack.User{updatedUser}},
+			initial,
+		)
+
+		result, err := ap.PatchUser(context.Background(), "U001")
+		require.NoError(t, err)
+		assert.Equal(t, "alice_new", result.Name)
+
+		snapshot := ap.usersSnapshot.Load()
+		assert.Equal(t, "alice_new", snapshot.Users["U001"].Name)
+		assert.Equal(t, "U001", snapshot.UsersInv["alice_new"])
+	})
+}