From 2d19f8c752bc20591756568d2e227f050b6abea7 Mon Sep 17 00:00:00 2001 From: Greg Katz Date: Thu, 2 Apr 2026 19:22:14 -0700 Subject: [PATCH] Add targeted user cache patch on miss When a user ID is not in the in-memory cache, message rendering degrades to raw IDs and paramFormatUser fails the tool call entirely. This is common on Enterprise Grid workspaces where the user cache (50K+ users) can be hours stale. On cache miss, fetch the single user via users.info and patch the snapshot atomically. This costs one API call instead of rebuilding the entire user cache. Disk persistence is skipped; the next full refresh cycle handles it. Fixes #268 Co-Authored-By: Claude Opus 4.6 (1M context) --- pkg/handler/conversations.go | 83 +++++++++++++---- pkg/provider/api.go | 39 ++++++++ pkg/provider/api_patch_test.go | 158 +++++++++++++++++++++++++++++++++ 3 files changed, 261 insertions(+), 19 deletions(-) create mode 100644 pkg/provider/api_patch_test.go diff --git a/pkg/handler/conversations.go b/pkg/handler/conversations.go index 67afd1d..8c1bb11 100644 --- a/pkg/handler/conversations.go +++ b/pkg/handler/conversations.go @@ -527,7 +527,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 @@ -566,7 +566,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 } @@ -576,7 +576,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 @@ -602,7 +602,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)) @@ -890,7 +890,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...) } @@ -1016,7 +1016,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...) } @@ -1507,8 +1507,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 @@ -1517,7 +1517,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) @@ -1587,17 +1587,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 } @@ -1916,7 +1918,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) @@ -1931,7 +1933,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 @@ -1939,7 +1941,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 @@ -1947,7 +1949,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 @@ -2014,13 +2016,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 } @@ -2071,6 +2080,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 } diff --git a/pkg/provider/api.go b/pkg/provider/api.go index dc0565d..4859a07 100644 --- a/pkg/provider/api.go +++ b/pkg/provider/api.go @@ -780,6 +780,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() defer ap.usersMu.Unlock() diff --git a/pkg/provider/api_patch_test.go b/pkg/provider/api_patch_test.go new file mode 100644 index 0000000..7a3ca3d --- /dev/null +++ b/pkg/provider/api_patch_test.go @@ -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"]) + }) +}