Memcached: Fix issue where non-zero TTLs were trunctated to zero (#530)

Fixes an issue where if a non-zero TTL smaller than a full second was passed
to the `SetAsync` or `SetMultiAsync` methods, the TTL would be truncated to
zero seconds which results in the item being cached forever.

Specifically, this fixes an issue in Mimir where items that were supposed to
have a several minute TTL computed as `$ttl - time.Since($start)` ended up being
cached forever. They were never evicted by Memcached because there was no need
to free space in this particular cache due to low usage.

Signed-off-by: Nick Pillitteri <[email protected]>

CHANGELOG.md

@@ -243,3 +243,4 @@
 * [BUGFIX] ring: don't mark trace spans as failed if `DoUntilQuorum` terminates due to cancellation. #449
 * [BUGFIX] middleware: fix issue where applications that used the httpgrpc tracing middleware would generate duplicate spans for incoming HTTP requests. #451
 * [BUGFIX] httpgrpc: store headers in canonical form when converting from gRPC to HTTP. #518
+* [BUGFIX] Memcached: Don't truncate sub-second TTLs to 0 which results in them being cached forever. #530 

cache/memcached_client.go

@@ -315,22 +315,27 @@ func (c *MemcachedClient) Name() string {
 }
 
 func (c *MemcachedClient) SetMultiAsync(data map[string][]byte, ttl time.Duration) {
-	c.setMultiAsync(data, ttl, func(key string, buf []byte, ttl time.Duration) error {
-		return c.client.Set(&memcache.Item{
-			Key:        key,
-			Value:      buf,
-			Expiration: int32(ttl.Seconds()),
-		})
-	})
+	c.setMultiAsync(data, ttl, c.setSingleItem)
 }
 
 func (c *MemcachedClient) SetAsync(key string, value []byte, ttl time.Duration) {
-	c.setAsync(key, value, ttl, func(key string, buf []byte, ttl time.Duration) error {
-		return c.client.Set(&memcache.Item{
-			Key:        key,
-			Value:      buf,
-			Expiration: int32(ttl.Seconds()),
-		})
+	c.setAsync(key, value, ttl, c.setSingleItem)
+}
+
+func (c *MemcachedClient) setSingleItem(key string, value []byte, ttl time.Duration) error {
+	ttlSeconds := int32(ttl.Seconds())
+	// If a TTL of exactly 0 is passed, we honor it and pass it to Memcached which will
+	// interpret it as an infinite TTL. However, if we get a non-zero TTL that is truncated
+	// to 0 seconds, we discard the update since the caller didn't intend to set an infinite
+	// TTL.
+	if ttl != 0 && ttlSeconds <= 0 {
+		return nil
+	}
+
+	return c.client.Set(&memcache.Item{
+		Key:        key,
+		Value:      value,
+		Expiration: ttlSeconds,
 	})
 }
 

cache/memcached_client_test.go

@@ -61,11 +61,44 @@ func TestMemcachedClientConfig_Validate(t *testing.T) {
 	}
 }
 
-func TestMemcachedClient_GetMulti(t *testing.T) {
-	setup := setupDefaultMemcachedClient
+func TestMemcachedClient_SetAsync(t *testing.T) {
+	t.Run("with non-zero TTL", func(t *testing.T) {
+		client, _, err := setupDefaultMemcachedClient()
+		require.NoError(t, err)
+		client.SetAsync("foo", []byte("bar"), 10*time.Second)
+		require.NoError(t, client.wait())
+
+		ctx := context.Background()
+		res := client.GetMulti(ctx, []string{"foo"})
+		require.Equal(t, map[string][]byte{"foo": []byte("bar")}, res)
+	})
+
+	t.Run("with truncated zero TTL", func(t *testing.T) {
+		client, _, err := setupDefaultMemcachedClient()
+		require.NoError(t, err)
+		client.SetAsync("foo", []byte("bar"), 100*time.Millisecond)
+		require.NoError(t, client.wait())
+
+		ctx := context.Background()
+		res := client.GetMulti(ctx, []string{"foo"})
+		require.Empty(t, res)
+	})
 
+	t.Run("with zero TTL", func(t *testing.T) {
+		client, _, err := setupDefaultMemcachedClient()
+		require.NoError(t, err)
+		client.SetAsync("foo", []byte("bar"), 0)
+		require.NoError(t, client.wait())
+
+		ctx := context.Background()
+		res := client.GetMulti(ctx, []string{"foo"})
+		require.Equal(t, map[string][]byte{"foo": []byte("bar")}, res)
+	})
+}
+
+func TestMemcachedClient_GetMulti(t *testing.T) {
 	t.Run("no allocator", func(t *testing.T) {
-		client, backend, err := setup()
+		client, backend, err := setupDefaultMemcachedClient()
 		require.NoError(t, err)
 		client.SetAsync("foo", []byte("bar"), 10*time.Second)
 		require.NoError(t, client.wait())
@@ -77,7 +110,7 @@ func TestMemcachedClient_GetMulti(t *testing.T) {
 	})
 
 	t.Run("with allocator", func(t *testing.T) {
-		client, backend, err := setup()
+		client, backend, err := setupDefaultMemcachedClient()
 		require.NoError(t, err)
 		client.SetAsync("foo", []byte("bar"), 10*time.Second)
 		require.NoError(t, client.wait())