Merge pull request #225 from flacoste/feat/stale-while-revalidate-cache

feat: stale-while-revalidate cache for large Slack workspaces
2026-09-18 23:43:49 +08:00 · 2026-05-14 22:10:59 +02:00
parent 21775f640d df22121447
commit 60566a196f
4 changed files with 438 additions and 83 deletions
@@ -8,6 +8,7 @@ import (
 	"strconv"
 	"strings"
 	"sync"
+	"time"

 	"github.com/korotovsky/slack-mcp-server/pkg/provider"
 	"github.com/korotovsky/slack-mcp-server/pkg/server"
@@ -86,10 +87,14 @@ func main() {

 	switch transport {
 	case "stdio":
-		if ready, _ := p.IsReady(); !ready && !noCache {
-			logger.Info("Slack MCP Server is still warming up caches, starting server anyway",
-				zap.String("context", "console"),
-			)
+		// Wait for caches to be ready before accepting stdio requests.
+		// With stale-while-revalidate this exits in one tick (~100ms).
+		// On cold start (no cache), this blocks until the initial fetch completes.
+		for {
+			if ready, _ := p.IsReady(); ready {
+				break
+			}
+			time.Sleep(100 * time.Millisecond)
 		}
 		if err := s.ServeStdio(); err != nil {
 			logger.Fatal("Server error",
@@ -284,6 +284,8 @@ docker-compose up -d
 | `SLACK_MCP_ADD_MESSAGE_UNFURLING` | No        | `nil`                     | Enable to let Slack unfurl posted links or set comma-separated list of domains e.g. `github.com,slack.com` to whitelist unfurling only for them. If text contains whitelisted and unknown domain unfurling will be disabled for security reasons.                                         |
 | `SLACK_MCP_USERS_CACHE`           | No        | `.users_cache.json`       | Path to the users cache file. Used to cache Slack user information to avoid repeated API calls on startup.                                                                                                                                                                                |
 | `SLACK_MCP_CHANNELS_CACHE`        | No        | `.channels_cache_v2.json` | Path to the channels cache file. Used to cache Slack channel information to avoid repeated API calls on startup.                                                                                                                                                                          |
+| `SLACK_MCP_CACHE_TTL`             | No        | `24h`                     | Cache time-to-live. Supports duration format (`24h`, `30m`) or seconds (`3600`). Set to `0` to disable TTL (cache forever). When the cache expires, stale data is served immediately while a background refresh fetches fresh data.                                                       |
+| `SLACK_MCP_MIN_REFRESH_INTERVAL`  | No        | `30s`                     | Minimum interval between forced cache refreshes. Prevents API abuse from repeated force-refresh requests. Supports duration format (`30s`, `1m`) or seconds (`60`). Set to `0` to disable rate limiting.                                                                                  |
 | `SLACK_MCP_LOG_LEVEL`             | No        | `info`                    | Log-level for stdout or stderr. Valid values are: `debug`, `info`, `warn`, `error`, `panic` and `fatal`                                                                                                                                                                                   |
 | `SLACK_MCP_ENABLED_TOOLS`         | No        | `nil`                     | Comma-separated list of tools to register. If empty, all read-only tools and usergroups tools are registered; write tools (`conversations_add_message`, `reactions_add`, `reactions_remove`, `attachment_get_data`) require their specific env var to be set OR must be explicitly listed here. When a write tool is listed here, it's enabled without channel restrictions. Available tools: `conversations_history`, `conversations_replies`, `conversations_add_message`, `reactions_add`, `reactions_remove`, `attachment_get_data`, `conversations_search_messages`, `conversations_join`, `conversations_leave`, `conversations_unreads`, `conversations_mark`, `channels_list`, `usergroups_list`, `usergroups_me`, `usergroups_create`, `usergroups_update`, `usergroups_users_update`, `users_search`. |

@@ -4,6 +4,7 @@ import (
 	"context"
 	"encoding/json"
 	"errors"
+	"fmt"
 	"io"
 	"os"
 	"path/filepath"
@@ -26,7 +27,7 @@ import (
 const usersNotReadyMsg = "users cache is not ready yet, sync process is still running... please wait"
 const channelsNotReadyMsg = "channels cache is not ready yet, sync process is still running... please wait"
 const defaultUA = "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/136.0.0.0 Safari/537.36"
-const defaultCacheTTL = 1 * time.Hour
+const defaultCacheTTL = 24 * time.Hour
 const defaultMinRefreshInterval = 30 * time.Second

 var AllChanTypes = []string{"mpim", "im", "public_channel", "private_channel"}
@@ -37,6 +38,38 @@ var ErrUsersNotReady = errors.New(usersNotReadyMsg)
 var ErrChannelsNotReady = errors.New(channelsNotReadyMsg)
 var ErrRefreshRateLimited = errors.New("refresh skipped due to rate limiting")

+// atomicWriteFile writes data to a file atomically using a temp file and rename.
+// Uses os.CreateTemp for unpredictable temp file names (prevents symlink attacks)
+// and cleans up the temp file on rename failure.
+func atomicWriteFile(path string, data []byte, perm os.FileMode) error {
+	dir := filepath.Dir(path)
+	tmp, err := os.CreateTemp(dir, ".cache_*.tmp")
+	if err != nil {
+		return fmt.Errorf("creating temp file: %w", err)
+	}
+	tmpPath := tmp.Name()
+
+	if _, err := tmp.Write(data); err != nil {
+		tmp.Close()
+		os.Remove(tmpPath)
+		return fmt.Errorf("writing temp file: %w", err)
+	}
+	if err := tmp.Chmod(perm); err != nil {
+		tmp.Close()
+		os.Remove(tmpPath)
+		return fmt.Errorf("setting file permissions: %w", err)
+	}
+	if err := tmp.Close(); err != nil {
+		os.Remove(tmpPath)
+		return fmt.Errorf("closing temp file: %w", err)
+	}
+	if err := os.Rename(tmpPath, path); err != nil {
+		os.Remove(tmpPath)
+		return fmt.Errorf("renaming temp file: %w", err)
+	}
+	return nil
+}
+
 // getCacheDir returns the appropriate cache directory for slack-mcp-server
 func getCacheDir() string {
 	cacheDir, err := os.UserCacheDir()
@@ -46,14 +79,14 @@ func getCacheDir() string {
 	}

 	dir := filepath.Join(cacheDir, "slack-mcp-server")
-	if err := os.MkdirAll(dir, 0755); err != nil {
+	if err := os.MkdirAll(dir, 0700); err != nil {
 		// Fallback to current directory if we can't create cache dir
 		return "."
 	}
 	return dir
 }

-// getCacheTTL returns the cache TTL from SLACK_MCP_CACHE_TTL env var or default (1 hour).
+// getCacheTTL returns the cache TTL from SLACK_MCP_CACHE_TTL env var or default (24 hours).
 // Supports formats: "1h", "30m", "3600" (seconds), "0" (disable TTL, cache forever)
 // Negative values are rejected and fall back to default.
 func getCacheTTL() time.Duration {
@@ -249,16 +282,20 @@ type ApiProvider struct {
 	// Users cache: atomic pointer to immutable snapshot (no copy on read)
 	usersSnapshot          atomic.Pointer[UsersCache]
 	usersCachePath         string
-	usersReady             bool
+	usersReady             atomic.Bool
+	refreshingUsers        atomic.Bool // true while a background refresh goroutine is running
 	lastForcedUsersRefresh time.Time
-	usersMu                sync.RWMutex // protects usersReady, lastForcedUsersRefresh
+	usersMu                sync.RWMutex // protects lastForcedUsersRefresh
+	fetchUsersMu           sync.Mutex   // serializes fetchAndStoreUsers calls

 	// Channels cache: atomic pointer to immutable snapshot (no copy on read)
 	channelsSnapshot          atomic.Pointer[ChannelsCache]
 	channelsCachePath         string
-	channelsReady             bool
+	channelsReady             atomic.Bool
+	refreshingChannels        atomic.Bool // true while a background refresh goroutine is running
 	lastForcedChannelsRefresh time.Time
-	channelsMu                sync.RWMutex // protects channelsReady, lastForcedChannelsRefresh
+	channelsMu                sync.RWMutex // protects lastForcedChannelsRefresh
+	fetchChannelsMu           sync.Mutex   // serializes fetchAndStoreChannels calls
 }

 func NewMCPSlackClient(authProvider auth.Provider, logger *zap.Logger) (*MCPSlackClient, error) {
@@ -782,14 +819,8 @@ func (ap *ApiProvider) ForceRefreshUsers(ctx context.Context) error {

 func (ap *ApiProvider) refreshUsersInternal(ctx context.Context, force bool) error {
 	ap.usersMu.Lock()
-	defer ap.usersMu.Unlock()

-	var (
-		list        []slack.User
-		optionLimit = slack.GetUsersOptionLimit(1000)
-	)
-
-	// Check if we should use cache (not forced, cache exists, and within TTL)
+	// Check if we should use cache (not forced, cache exists)
 	if !force {
 		if data, err := os.ReadFile(ap.usersCachePath); err == nil {
 			var cachedUsers []slack.User
@@ -801,43 +832,82 @@ func (ap *ApiProvider) refreshUsersInternal(ctx context.Context, force bool) err
 				ap.logger.Warn("Users cache is empty or null, will refetch",
 					zap.String("cache_file", ap.usersCachePath))
 			} else {
+				// Build snapshot from cache
+				newSnapshot := &UsersCache{
+					Users:    make(map[string]slack.User, len(cachedUsers)),
+					UsersInv: make(map[string]string, len(cachedUsers)),
+				}
+				for _, u := range cachedUsers {
+					newSnapshot.Users[u.ID] = u
+					newSnapshot.UsersInv[u.Name] = u.ID
+				}
+				ap.usersSnapshot.Store(newSnapshot)
+				ap.usersReady.Store(true)
+
 				// Check cache TTL using file modification time
-				cacheValid := true
+				cacheExpired := false
 				if ap.cacheTTL > 0 {
 					if fileInfo, err := os.Stat(ap.usersCachePath); err == nil {
 						cacheAge := time.Since(fileInfo.ModTime())
 						if cacheAge > ap.cacheTTL {
-							ap.logger.Info("Users cache expired, will refetch",
+							cacheExpired = true
+							ap.logger.Info("Serving stale users cache, background refresh starting",
 								zap.Duration("cache_age", cacheAge),
 								zap.Duration("ttl", ap.cacheTTL),
+								zap.Int("count", len(cachedUsers)),
 								zap.String("cache_file", ap.usersCachePath))
-							cacheValid = false
 						}
 					}
 				}

-				if cacheValid {
-					// Build new snapshot from cache
-					newSnapshot := &UsersCache{
-						Users:    make(map[string]slack.User, len(cachedUsers)),
-						UsersInv: make(map[string]string, len(cachedUsers)),
-					}
-					for _, u := range cachedUsers {
-						newSnapshot.Users[u.ID] = u
-						newSnapshot.UsersInv[u.Name] = u.ID
-					}
-					ap.usersSnapshot.Store(newSnapshot)
+				if !cacheExpired {
 					ap.logger.Info("Loaded users from cache",
 						zap.Int("count", len(cachedUsers)),
 						zap.String("cache_file", ap.usersCachePath))
-					ap.usersReady = true
+					ap.usersMu.Unlock()
 					return nil
 				}
+
+				// Cache is expired: release lock, spawn background refresh, return immediately
+				ap.usersMu.Unlock()
+				ap.spawnBackgroundUsersRefresh()
+				return nil
 			}
 		}
 	}

-	// Fetch fresh data from Slack API
+	// No usable cache: fetch fresh data synchronously (first run or force)
+	ap.usersMu.Unlock()
+	return ap.fetchAndStoreUsers(ctx)
+}
+
+// spawnBackgroundUsersRefresh starts a background goroutine to fetch fresh user data.
+// Uses refreshingUsers flag to prevent concurrent background refreshes.
+func (ap *ApiProvider) spawnBackgroundUsersRefresh() {
+	if !ap.refreshingUsers.CompareAndSwap(false, true) {
+		ap.logger.Debug("Skipping background users refresh, already in progress")
+		return
+	}
+	go func() {
+		defer ap.refreshingUsers.Store(false)
+		if err := ap.fetchAndStoreUsers(context.Background()); err != nil {
+			ap.logger.Warn("Background users refresh failed, continuing with stale data",
+				zap.Error(err))
+		}
+	}()
+}
+
+// fetchAndStoreUsers fetches all users from the Slack API and updates the snapshot and cache file.
+// Serialized by fetchUsersMu to prevent concurrent fetches from racing on snapshot/file writes.
+func (ap *ApiProvider) fetchAndStoreUsers(ctx context.Context) error {
+	ap.fetchUsersMu.Lock()
+	defer ap.fetchUsersMu.Unlock()
+
+	var (
+		list        []slack.User
+		optionLimit = slack.GetUsersOptionLimit(1000)
+	)
+
 	users, err := ap.client.GetUsersContext(ctx,
 		optionLimit,
 	)
@@ -845,6 +915,15 @@ func (ap *ApiProvider) refreshUsersInternal(ctx context.Context, force bool) err
 		ap.logger.Error("Failed to fetch users", zap.Error(err))
 		return err
 	}
+
+	if len(users) == 0 {
+		if ap.usersReady.Load() {
+			ap.logger.Warn("API returned zero users, keeping existing cache")
+			return nil
+		}
+		return errors.New("API returned zero users and no existing cache is available")
+	}
+
 	list = append(list, users...)

 	// Build new snapshot
@@ -888,7 +967,8 @@ func (ap *ApiProvider) refreshUsersInternal(ctx context.Context, force bool) err
 	if data, err := json.MarshalIndent(list, "", "  "); err != nil {
 		ap.logger.Error("Failed to marshal users for cache", zap.Error(err))
 	} else {
-		if err := os.WriteFile(ap.usersCachePath, data, 0644); err != nil {
+		// Atomic write: temp file + rename to prevent partial/corrupt files
+		if err := atomicWriteFile(ap.usersCachePath, data, 0600); err != nil {
 			ap.logger.Error("Failed to write cache file",
 				zap.String("cache_file", ap.usersCachePath),
 				zap.Error(err))
@@ -899,7 +979,7 @@ func (ap *ApiProvider) refreshUsersInternal(ctx context.Context, force bool) err
 		}
 	}

-	ap.usersReady = true
+	ap.usersReady.Store(true)

 	return nil
 }
@@ -935,9 +1015,8 @@ func (ap *ApiProvider) ForceRefreshChannels(ctx context.Context) error {

 func (ap *ApiProvider) refreshChannelsInternal(ctx context.Context, force bool) error {
 	ap.channelsMu.Lock()
-	defer ap.channelsMu.Unlock()

-	// Check if we should use cache (not forced, cache exists, and within TTL)
+	// Check if we should use cache (not forced, cache exists)
 	if !force {
 		if data, err := os.ReadFile(ap.channelsCachePath); err == nil {
 			var cachedChannels []Channel
@@ -949,66 +1028,103 @@ func (ap *ApiProvider) refreshChannelsInternal(ctx context.Context, force bool)
 				ap.logger.Warn("Channels cache is empty or null, will refetch",
 					zap.String("cache_file", ap.channelsCachePath))
 			} else {
+				// Re-map channels with current users cache to ensure DM names are populated
+				usersMap := ap.ProvideUsersMap().Users
+				newSnapshot := &ChannelsCache{
+					Channels:    make(map[string]Channel, len(cachedChannels)),
+					ChannelsInv: make(map[string]string, len(cachedChannels)),
+				}
+				for _, c := range cachedChannels {
+					if c.IsIM {
+						remappedChannel := mapChannel(
+							c.ID, "", "", c.Topic, c.Purpose,
+							c.User, c.Members, c.MemberCount,
+							c.IsIM, c.IsMpIM, c.IsPrivate, c.IsExtShared,
+							usersMap,
+						)
+						newSnapshot.Channels[c.ID] = remappedChannel
+						newSnapshot.ChannelsInv[remappedChannel.Name] = c.ID
+					} else {
+						newSnapshot.Channels[c.ID] = c
+						newSnapshot.ChannelsInv[c.Name] = c.ID
+					}
+				}
+				ap.channelsSnapshot.Store(newSnapshot)
+				ap.channelsReady.Store(true)
+
 				// Check cache TTL using file modification time
-				cacheValid := true
+				cacheExpired := false
 				if ap.cacheTTL > 0 {
 					if fileInfo, err := os.Stat(ap.channelsCachePath); err == nil {
 						cacheAge := time.Since(fileInfo.ModTime())
 						if cacheAge > ap.cacheTTL {
-							ap.logger.Info("Channels cache expired, will refetch",
+							cacheExpired = true
+							ap.logger.Info("Serving stale channels cache, background refresh starting",
 								zap.Duration("cache_age", cacheAge),
 								zap.Duration("ttl", ap.cacheTTL),
+								zap.Int("count", len(cachedChannels)),
 								zap.String("cache_file", ap.channelsCachePath))
-							cacheValid = false
 						}
 					}
 				}

-				if cacheValid {
-					// Re-map channels with current users cache to ensure DM names are populated
-					usersMap := ap.ProvideUsersMap().Users
-					newSnapshot := &ChannelsCache{
-						Channels:    make(map[string]Channel, len(cachedChannels)),
-						ChannelsInv: make(map[string]string, len(cachedChannels)),
-					}
-					for _, c := range cachedChannels {
-						// For IM channels, re-generate the name and purpose using current users cache
-						if c.IsIM {
-							// Re-map the channel to get updated user name if available
-							remappedChannel := mapChannel(
-								c.ID, "", "", c.Topic, c.Purpose,
-								c.User, c.Members, c.MemberCount,
-								c.IsIM, c.IsMpIM, c.IsPrivate, c.IsExtShared,
-								usersMap,
-							)
-							newSnapshot.Channels[c.ID] = remappedChannel
-							newSnapshot.ChannelsInv[remappedChannel.Name] = c.ID
-						} else {
-							newSnapshot.Channels[c.ID] = c
-							newSnapshot.ChannelsInv[c.Name] = c.ID
-						}
-					}
-					ap.channelsSnapshot.Store(newSnapshot)
+				if !cacheExpired {
 					ap.logger.Info("Loaded channels from cache and re-mapped DM names",
 						zap.Int("count", len(cachedChannels)),
 						zap.String("cache_file", ap.channelsCachePath))
-					ap.channelsReady = true
+					ap.channelsMu.Unlock()
 					return nil
 				}
+
+				// Cache is expired: release lock, spawn background refresh, return immediately
+				ap.channelsMu.Unlock()
+				ap.spawnBackgroundChannelsRefresh()
+				return nil
 			}
 		}
 	}

-	// Fetch fresh data from Slack API
+	// No usable cache: fetch fresh data synchronously (first run or force)
+	ap.channelsMu.Unlock()
+	return ap.fetchAndStoreChannels(ctx)
+}
+
+// spawnBackgroundChannelsRefresh starts a background goroutine to fetch fresh channel data.
+func (ap *ApiProvider) spawnBackgroundChannelsRefresh() {
+	if !ap.refreshingChannels.CompareAndSwap(false, true) {
+		ap.logger.Debug("Skipping background channels refresh, already in progress")
+		return
+	}
+	go func() {
+		defer ap.refreshingChannels.Store(false)
+		if err := ap.fetchAndStoreChannels(context.Background()); err != nil {
+			ap.logger.Warn("Background channels refresh failed, continuing with stale data",
+				zap.Error(err))
+		}
+	}()
+}
+
+// fetchAndStoreChannels fetches all channels from the Slack API and updates the snapshot and cache file.
+// Serialized by fetchChannelsMu to prevent concurrent fetches from racing on snapshot/file writes.
+func (ap *ApiProvider) fetchAndStoreChannels(ctx context.Context) error {
+	ap.fetchChannelsMu.Lock()
+	defer ap.fetchChannelsMu.Unlock()
+
 	channels := ap.GetChannels(ctx, AllChanTypes)

 	if len(channels) == 0 {
-		ap.logger.Warn("No channels fetched from Slack API, not writing empty cache",
-			zap.String("cache_file", ap.channelsCachePath))
-	} else if data, err := json.MarshalIndent(channels, "", "  "); err != nil {
+		if ap.channelsReady.Load() {
+			ap.logger.Warn("API returned zero channels, keeping existing cache")
+			return nil
+		}
+		return errors.New("API returned zero channels and no existing cache is available")
+	}
+
+	if data, err := json.MarshalIndent(channels, "", "  "); err != nil {
 		ap.logger.Error("Failed to marshal channels for cache", zap.Error(err))
 	} else {
-		if err := os.WriteFile(ap.channelsCachePath, data, 0644); err != nil {
+		// Atomic write: temp file + rename to prevent partial/corrupt files
+		if err := atomicWriteFile(ap.channelsCachePath, data, 0600); err != nil {
 			ap.logger.Error("Failed to write cache file",
 				zap.String("cache_file", ap.channelsCachePath),
 				zap.Error(err))
@@ -1019,7 +1135,7 @@ func (ap *ApiProvider) refreshChannelsInternal(ctx context.Context, force bool)
 		}
 	}

-	ap.channelsReady = true
+	ap.channelsReady.Store(true)

 	return nil
 }
@@ -1178,10 +1294,10 @@ func (ap *ApiProvider) ProvideChannelsMaps() *ChannelsCache {
 }

 func (ap *ApiProvider) IsReady() (bool, error) {
-	if !ap.usersReady {
+	if !ap.usersReady.Load() {
 		return false, ErrUsersNotReady
 	}
-	if !ap.channelsReady {
+	if !ap.channelsReady.Load() {
 		return false, ErrChannelsNotReady
 	}
 	return true, nil
@@ -1191,13 +1307,8 @@ func (ap *ApiProvider) IsReady() (bool, error) {
 // any data. Lookups by #channel-name or @username will not work; callers
 // must use channel/user IDs instead.
 func (ap *ApiProvider) SkipCache() {
-	ap.usersMu.Lock()
-	ap.usersReady = true
-	ap.usersMu.Unlock()
-
-	ap.channelsMu.Lock()
-	ap.channelsReady = true
-	ap.channelsMu.Unlock()
+	ap.usersReady.Store(true)
+	ap.channelsReady.Store(true)
 }

 func (ap *ApiProvider) ServerTransport() string {
@@ -1248,7 +1359,7 @@ func (ap *ApiProvider) SearchUsers(ctx context.Context, query string, limit int)
 // searchUsersInCache performs a case-insensitive regex search on cached users.
 // Matches against username, real name, display name, and email.
 func (ap *ApiProvider) searchUsersInCache(query string, limit int) ([]slack.User, error) {
-	if !ap.usersReady {
+	if !ap.usersReady.Load() {
 		return nil, ErrUsersNotReady
 	}

@@ -4,9 +4,12 @@ import (
 	"encoding/json"
 	"os"
 	"path/filepath"
+	"sync"
+	"sync/atomic"
 	"testing"
 	"time"

+	"github.com/slack-go/slack"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -320,6 +323,139 @@ func TestGetCacheDir(t *testing.T) {
 	assert.True(t, info.IsDir(), "cache path should be a directory")
 }

+// TestDefaultCacheTTLIs24Hours verifies that the default cache TTL is 24 hours.
+func TestDefaultCacheTTLIs24Hours(t *testing.T) {
+	assert.Equal(t, 24*time.Hour, defaultCacheTTL,
+		"default cache TTL should be 24 hours")
+}
+
+// TestAtomicReadyFlags verifies that usersReady and channelsReady are atomic.Bool
+// and safe for concurrent access. This test is meaningful under `go test -race`.
+func TestAtomicReadyFlags(t *testing.T) {
+	var usersReady, channelsReady atomic.Bool
+
+	// Initially false
+	assert.False(t, usersReady.Load())
+	assert.False(t, channelsReady.Load())
+
+	done := make(chan struct{})
+
+	// Concurrent writers
+	go func() {
+		for i := 0; i < 1000; i++ {
+			usersReady.Store(true)
+			channelsReady.Store(true)
+		}
+		close(done)
+	}()
+
+	// Concurrent readers (would race on plain bool under -race)
+	for i := 0; i < 1000; i++ {
+		_ = usersReady.Load()
+		_ = channelsReady.Load()
+	}
+
+	<-done
+	assert.True(t, usersReady.Load())
+	assert.True(t, channelsReady.Load())
+}
+
+// TestRefreshingFlagPreventsConcurrentRefreshes verifies that CompareAndSwap on
+// the refreshing flag prevents a second background refresh from starting.
+func TestRefreshingFlagPreventsConcurrentRefreshes(t *testing.T) {
+	var refreshing atomic.Bool
+
+	// First caller succeeds
+	assert.True(t, refreshing.CompareAndSwap(false, true),
+		"first refresh should acquire the flag")
+
+	// Second caller is blocked
+	assert.False(t, refreshing.CompareAndSwap(false, true),
+		"second refresh should be blocked while first is in progress")
+
+	// After first completes, next one can proceed
+	refreshing.Store(false)
+	assert.True(t, refreshing.CompareAndSwap(false, true),
+		"refresh should succeed after previous one completes")
+}
+
+// TestStaleWhileRevalidateReadyFlag verifies the stale-while-revalidate pattern:
+// when an expired cache file exists, the ready flag is set immediately from stale data,
+// without waiting for a fresh API fetch.
+func TestStaleWhileRevalidateReadyFlag(t *testing.T) {
+	tempDir, err := os.MkdirTemp("", "slack-mcp-swr-test")
+	require.NoError(t, err)
+	defer os.RemoveAll(tempDir)
+
+	cacheFile := filepath.Join(tempDir, "users_cache.json")
+
+	// Write a valid cache file with user data
+	users := []struct {
+		ID   string `json:"id"`
+		Name string `json:"name"`
+	}{
+		{ID: "U001", Name: "alice"},
+		{ID: "U002", Name: "bob"},
+	}
+	data, err := json.Marshal(users)
+	require.NoError(t, err)
+	err = os.WriteFile(cacheFile, data, 0644)
+	require.NoError(t, err)
+
+	// Set mtime to 48 hours ago (well past 24h default TTL)
+	staleTime := time.Now().Add(-48 * time.Hour)
+	err = os.Chtimes(cacheFile, staleTime, staleTime)
+	require.NoError(t, err)
+
+	// Simulate the stale-while-revalidate logic from refreshUsersInternal:
+	// 1. Read and unmarshal cache
+	// 2. Build snapshot and set ready flag
+	// 3. Check TTL — expired means we'd spawn background refresh
+	var usersReady atomic.Bool
+	var usersSnapshot atomic.Pointer[UsersCache]
+
+	fileData, err := os.ReadFile(cacheFile)
+	require.NoError(t, err)
+
+	type simpleUser struct {
+		ID   string `json:"id"`
+		Name string `json:"name"`
+	}
+	var cachedUsers []simpleUser
+	err = json.Unmarshal(fileData, &cachedUsers)
+	require.NoError(t, err)
+
+	// Build snapshot (mirrors refreshUsersInternal logic)
+	snapshot := &UsersCache{
+		Users:    make(map[string]slack.User, len(cachedUsers)),
+		UsersInv: make(map[string]string, len(cachedUsers)),
+	}
+	for _, u := range cachedUsers {
+		snapshot.Users[u.ID] = slack.User{ID: u.ID, Name: u.Name}
+		snapshot.UsersInv[u.Name] = u.ID
+	}
+	usersSnapshot.Store(snapshot)
+	usersReady.Store(true)
+
+	// Ready flag should be true immediately (before any background refresh)
+	assert.True(t, usersReady.Load(),
+		"ready flag should be set immediately from stale cache")
+
+	// Snapshot should contain the stale data
+	loaded := usersSnapshot.Load()
+	require.NotNil(t, loaded)
+	assert.Len(t, loaded.Users, 2, "snapshot should contain cached users")
+	assert.Equal(t, "U001", loaded.Users["U001"].ID)
+	assert.Equal(t, "U002", loaded.UsersInv["bob"])
+
+	// Verify the cache IS expired (would trigger background refresh)
+	fileInfo, err := os.Stat(cacheFile)
+	require.NoError(t, err)
+	cacheAge := time.Since(fileInfo.ModTime())
+	assert.True(t, cacheAge > defaultCacheTTL,
+		"cache should be detected as expired (age=%v > TTL=%v)", cacheAge, defaultCacheTTL)
+}
+
 // TestGetMinRefreshInterval tests the rate limiting configuration parsing.
 func TestGetMinRefreshInterval(t *testing.T) {
 	tests := []struct {
@@ -380,3 +516,104 @@ func TestGetMinRefreshInterval(t *testing.T) {
 		})
 	}
 }
+
+// TestFetchSerializationWithMutex verifies that fetchUsersMu/fetchChannelsMu
+// serializes concurrent calls to fetchAndStore*. This test validates the mutex
+// pattern rather than calling the actual Slack API.
+func TestFetchSerializationWithMutex(t *testing.T) {
+	var mu sync.Mutex
+	var order []int
+
+	// Simulate two concurrent fetchAndStore calls serialized by a mutex
+	done := make(chan struct{})
+	started := make(chan struct{})
+
+	go func() {
+		mu.Lock()
+		close(started) // Signal that the lock is held
+		time.Sleep(50 * time.Millisecond)
+		order = append(order, 1)
+		mu.Unlock()
+	}()
+
+	go func() {
+		<-started // Wait for first goroutine to hold the lock
+		mu.Lock()
+		order = append(order, 2)
+		mu.Unlock()
+		close(done)
+	}()
+
+	<-done
+	assert.Equal(t, []int{1, 2}, order,
+		"second fetch should wait for first to complete")
+}
+
+// TestEmptyAPIResultGuard verifies that an empty API result does not overwrite
+// valid cached data. This tests the guard pattern in fetchAndStoreUsers/fetchAndStoreChannels.
+func TestEmptyAPIResultGuard(t *testing.T) {
+	tempDir, err := os.MkdirTemp("", "slack-mcp-empty-guard-test")
+	require.NoError(t, err)
+	defer os.RemoveAll(tempDir)
+
+	cacheFile := filepath.Join(tempDir, "users_cache.json")
+
+	// Write a valid cache file with user data
+	validData := []slack.User{
+		{ID: "U001", Name: "alice"},
+		{ID: "U002", Name: "bob"},
+	}
+	data, err := json.MarshalIndent(validData, "", "  ")
+	require.NoError(t, err)
+	err = os.WriteFile(cacheFile, data, 0644)
+	require.NoError(t, err)
+
+	// Simulate the guard: API returns empty result
+	emptyUsers := []slack.User{}
+	assert.Len(t, emptyUsers, 0, "simulated API returned zero users")
+
+	// The guard should prevent overwriting the cache file
+	// (In production code: if len(users) == 0 { return nil })
+	if len(emptyUsers) == 0 {
+		// Don't write to cache — verify original data is preserved
+		preserved, err := os.ReadFile(cacheFile)
+		require.NoError(t, err)
+
+		var loadedUsers []slack.User
+		err = json.Unmarshal(preserved, &loadedUsers)
+		require.NoError(t, err)
+		assert.Len(t, loadedUsers, 2, "original cache should be preserved when API returns empty")
+		assert.Equal(t, "U001", loadedUsers[0].ID)
+	}
+}
+
+// TestEmptyCacheFileTreatedAsMiss verifies that an empty cache file (valid JSON [])
+// is treated as a cache miss rather than valid data with zero entries.
+func TestEmptyCacheFileTreatedAsMiss(t *testing.T) {
+	tempDir, err := os.MkdirTemp("", "slack-mcp-empty-cache-test")
+	require.NoError(t, err)
+	defer os.RemoveAll(tempDir)
+
+	cacheFile := filepath.Join(tempDir, "users_cache.json")
+
+	// Write an empty but valid JSON array
+	err = os.WriteFile(cacheFile, []byte(`[]`), 0644)
+	require.NoError(t, err)
+
+	// Read and unmarshal
+	data, err := os.ReadFile(cacheFile)
+	require.NoError(t, err)
+
+	var cachedUsers []slack.User
+	err = json.Unmarshal(data, &cachedUsers)
+	require.NoError(t, err)
+
+	// The guard: empty cache should be treated as a miss
+	assert.Len(t, cachedUsers, 0, "empty cache file should unmarshal to zero users")
+
+	// In production code, this triggers: "treating as cache miss"
+	// and falls through to fetchAndStoreUsers instead of setting ready=true
+	isCacheMiss := len(cachedUsers) == 0
+	assert.True(t, isCacheMiss,
+		"empty cache file should be treated as cache miss, not valid data")
+}