Redis Fix - namespace key can grow to GB size

[kdybicz]

Describe the bug
Since I migrated to keyv it looks like the amount of freeable memory is declining in constant rate: https://imgur.com/a/77kOTD7 (20 Jul is when I released changes for keyv migration).

I expect it to be related to the namespace:... key, that in my case can grow to more than 15GB in actively used cache.

To Reproduce
I would expect that adding a bunch of big keys for short living values will cause the namespace:... size to constantly grow.

Expected behavior
I would expect to have some auto-purging mechanism?

Tests (Bonus!!!)
N/A

Additional context
N/A

[jaredwray]

HI 👋 @kdybicz - do you have the storage adapter you are using and also how many objects are in there?

Can we see the code that you are adding in objects and setting the TTL?

Also, have you tried to do a clear() and saw what it is doing?

[kdybicz]

Hi @jaredwray! I did some investigation and I'm no longer so sure that the migration to keyv was at fault. Though the fact remain that before I cleared the cache the namespace:... exceeded 15 GB.

The only adapters/wrappers I use is https://github.com/kdybicz/apollo-server-compression-cache-wrapper - though it's only responsible for compression of the cached data, it doesn't change keys AND additionally I use KeyvAdapter to be able to use it with Apollo Server.

I'm using cache only with apollo RESTDataSource, so no additional magic there. This is the code I use for the setup:

const keyv = new Keyv(process.env.REDIS_HOST, {
  adapter: 'redis',
  namespace: '...',
  lazyConnect: true,
  connectTimeout: 5000,
  reconnectOnError: (error: Error) => {
    log.error(`Reconnect on error: ${error.message}`);
    const targetErrors = [/READONLY/, /ETIMEDOUT/];
    return targetErrors.some((targetError) => targetError.test(error.message));
  },
  retryStrategy: (times: number) => {
    ...
  },
});

const cache = new KeyvAdapter(keyv, { disableBatchReads: true });

const compressedCache = new CompressionCacheWrapper(cache, {
  compress: (data: Buffer) => zlib.brotliCompressSync(data, {
    [zlib.constants.BROTLI_PARAM_MODE]: zlib.constants.BROTLI_MODE_TEXT,
    [zlib.constants.BROTLI_PARAM_QUALITY]: 3,
  }),
  decompress: (data: Buffer) => zlib.brotliDecompressSync(data),
});

const server = new ApolloServer({
  csrfPrevention: true,
  schema: makeExecutableSchema({
    typeDefs: schema,
    resolvers,
  }),
  dataSources: () => ({
    ...
  }),
  cache: compressedCache,
  context: ({ event, context }) => ({
    functionName: context.functionName,
    event,
    context,
  }),
  ...

Edit:
Caching is controlled from the RESTDataSoruce level:

    try {
      const response = await this.get(
        'autocomplete',
        { territory: resolveTerritoryCode(territory), query },
        {
          cacheOptions: { ttl: defaultCacheMaxAge },
        },
      );

where I cache only some of the data I fetch from the internet.

[jaredwray]

This looks pretty straight forward and we have not had an issue with Redis recently around aging out with time to live.

Have you tried setting the ttl to a specific time and see if it is doing the right thing by aging out the cache?

[kdybicz]

I played with the TTL a lot and that's not the problem. All of the keys that I'm caching have the TTL set, beside one which I think is created and controlled by keyv or apollo itself and it is some sort of intermediate namespace:... hash. And that's the object that can grow, because afaik there is no way to set TTL on keys in a hash. So in my case, where keys change a lot and can be long it adds up, slowly but surely. Hope that summary make sense.

[jaredwray]

I wonder if we can code up an example that does this as a test case. Think you have time to get a simple version that just does it as a unit test?

[kdybicz]

Hope that helps:

test('namespace is not cleaned up by TTL', async t => {
	// Setup
	const keyv = new Keyv(redisURI, {
		adapter: 'redis',
		namespace: 'v3',
	});

	const length = 2048;
	const key = [...Array.from({length}).keys()].join('');

	await keyv.set(key, 'value', 1);

	await new Promise(r => {
		setTimeout(r, 250);
	});

	await keyv.disconnect();

	// Test
	const redis = new Redis(redisURI);

	// Stored key/value does expire
	const expiredValue = await redis.get(`v3:${key}`);
	t.is(expiredValue, null);

	// But the namespace doesn't expire
	t.true(await redis.exists('namespace:v3') === 1);

	// And take out some of the memory for each of the keys
	t.true(await redis.memory('USAGE', 'namespace:v3') === 7336);

	// Even though the value is no longer there
	const storedValue = await redis.mget('namespace:v3', `v3:${key}`);
	t.deepEqual(storedValue, [null, null]);
});

[jaredwray]

@kdybicz - thanks for doing this and we are looking at it now.

[kdybicz]

@jaredwray have you had a chance to take a look at the problem and think if is solvable on your side?

[jaredwray]

Hi @kdybicz - @alphmth said they will be looking at this.

[jaredwray]

@kdybicz - one thing we could do is just not use namespace which most likely would fix the issue. We could make it so the Redis system has an option to not use that.

[kdybicz]

@jaredwray I think that making it optional might help to solve part of the problem, but as it's not a "real" bug and solution would be partial - as it would not help people wanting to use namespaces - I would hold off with any bigger changes. The fix for "clean" method should be enough, plus maybe some note in docs.

[loris]

Hi! Can confirm the same issue on our production clusters (namespace:keyv set with millions of members) and can easily reproduce it locally.
Actually, I had a quick look at the redis implementation, the issue is that the members are removed from the namespace key only when cache entries are deleted manually (using delete or clear), but nothing happen when cache entries are automatically removed by Redis TTL feature (which is probably supposed to be the majority of the keys for a basic cache use-case)

[jaredwray]

We have added more partial fixes on this #513

[jaredwray]

@kdybicz @loris - Wanted to get you an update on this storage adapter and what we are planning to do in November. Our current goal is to re-write part of the redis adapter as the usage needs to be enabled to not use the namespace like we are doing today.

With that said we will be adding in an option to not use the namespace and might make that the default moving forward over time. Initially we will make it a feature flag which will solve this issue and we are targeting in November 2022 to do that update.

[PhantomRay]

As a temporary workaround, I use the following code:

class MyKeyvRedis extends KeyvRedis {
  async set(key, value, ttl) {
    if (typeof value === 'undefined') {
      return Promise.resolve(undefined);
    }

    return Promise.resolve().then(() => {
      if (typeof ttl === 'number') {
        return this.redis.set(key, value, 'PX', ttl);
      }

      return this.redis.set(key, value);
    });
  }
}

As you can see, I removed this.redis.sadd(this._getNamespace(), key);

[KODerFunk]

I have another issue around this.redis.sadd(.
I use commandTimeout option and some errors dont catch in project-code. As it turned out, these are timeouts on the sadd command. Until I figured out why my catch does not work and the error goes to unhandledRejection.
I need the ability to disable this, I also have to make class MyKeyvRedis extends KeyvRedis in the project and override not only the set method, but also clear (remove smembers, add throw new MyError) and delete (remove srem).

[haritonstefan]

Hello @jaredwray I was wondering what's the status of this?
I'm open to help with it.

[jaredwray]

@haritonstefan - we have started the rewrite but had to work on the test-suite upgrade first which should be releasing this week. Then we will get the new Redis client built and live.

Note that we are moving the new Redis client to Typescript and also solving for the architecture issues above by not using the namespacing inside redis anymore unless you explicitly set the option to use it.

Happy for you to help and will ping you next week on the status and plan.

[dylanseago]

This issue is also impacting us. Started using keyv-redis as a short ttl cache for GET requests. The namespace key has grown massive in size slowly over time as it accumulates every key ever cached without clearing it as ttl expires

[kaykdm]

This issue is also impacting us. Started using keyv-redis as a short ttl cache for GET requests. The namespace key has grown massive in size slowly over time as it accumulates every key ever cached without clearing it as ttl expires

We are also facing the same issue.
@jaredwray any update?

[kdybicz]

As a temporary workaround I've created stupid simple cron Lambda to run once a month and just remove the namespace key in the middle of the night, when the traffic is the lowest. That might not work for all of you, but it kinda solved the issue for me.

[PhantomRay]

My solution above should solve the problem.

As a temporary workaround I've created stupid simple cron Lambda to run once a month and just remove the namespace key in the middle of the night, when the traffic is the lowest. That might not work for all of you, but it kinda solved the issue for me.

[haritonstefan]

@haritonstefan - we have started the rewrite but had to work on the test-suite upgrade first which should be releasing this week. Then we will get the new Redis client built and live.

Note that we are moving the new Redis client to Typescript and also solving for the architecture issues above by not using the namespacing inside redis anymore unless you explicitly set the option to use it.

Happy for you to help and will ping you next week on the status and plan.

Hi @jaredwray I wanted to follow up on this message, how can I help?

[jaredwray]

Adding in the namespace disable capability as we are building the next version of @keyv/redis now.

#729

@klippz FYI

[jaredwray]

@haritonstefan @PhantomRay @loris @KODerFunk - we have merged the changes in and should be releasing an update with the change to enable or disable using Redis Sets for name spacing in the next 10 days.

[kdybicz]

@jaredwray amazing news! I will setup myself a reminder to take a look at your changes in the next couple of weeks. Thank you for the update!

[jaredwray]

@loris @KODerFunk @PhantomRay @haritonstefan @kdybicz - This has now been released with instructions here https://github.com/jaredwray/keyv/releases/tag/2023-07-01

To summarize it:

For high performance environments and to not have the namespace (SETS) grow we are providing an option not to use it. Simply change your code with this option:

const Keyv = require('keyv');
const keyv = new Keyv('redis://user:pass@localhost:6379', { useRedisSets: false });

Please let me know your thoughts on this.