fix(node-cache): Fixing issues with metrics reporting

packages/node-cache/src/index.ts

@@ -141,6 +141,7 @@ export class NodeCache<T> extends Hookified {
 		}
 
 		const keyValue = this.formatKey(key);
+		const existing = this.store.get(keyValue);
 		let expirationTimestamp = 0; // 0 = never delete
 
 		if (this.isNegativeTtl(ttl)) {
@@ -170,7 +171,7 @@ export class NodeCache<T> extends Hookified {
 		/* v8 ignore next -- @preserve */
 		if (this.options.maxKeys) {
 			const { maxKeys } = this.options;
-			if (maxKeys > -1 && this.store.size >= maxKeys) {
+			if (maxKeys > -1 && this.store.size >= maxKeys && !existing) {
 				throw this.createError(NodeCacheErrors.ECACHEFULL, this.formatKey(key));
 			}
 		}
@@ -184,6 +185,11 @@ export class NodeCache<T> extends Hookified {
 		// Event
 		this.emit("set", keyValue, value, expirationTimestamp);
 
+		if (existing) {
+			this._stats.decreaseKSize(keyValue);
+			this._stats.decreaseVSize(existing.value);
+		}
+
 		// Add the bytes to the stats
 		this._stats.incrementKSize(keyValue);
 		this._stats.incrementVSize(value);
@@ -285,9 +291,12 @@ export class NodeCache<T> extends Hookified {
 	 * @returns {T | undefined} the value or undefined
 	 */
 	public take<V = T>(key: string | number): V | undefined {
+		const keyValue = this.formatKey(key);
+		const item = this.store.get(keyValue);
+		const exists = item ? !(item.ttl > 0 && item.ttl < Date.now()) : false;
 		const result = this.get(key);
 
-		if (result) {
+		if (exists) {
 			this.del(key);
 			if (this.options.useClones) {
 				return this.clone(result) as V;
@@ -296,7 +305,7 @@ export class NodeCache<T> extends Hookified {
 			return result as V;
 		}
 
-		return undefined;
+		return result as V;
 	}
 
 	/**

packages/node-cache/src/store.ts

@@ -35,6 +35,7 @@ export class NodeCacheStore<T> extends Hookified {
 	private _useClones: boolean;
 	private _deleteOnExpire: boolean;
 	private _keys = new Set<string>();
+	private _values = new Map<string, T>();
 	private _ttls = new Map<string, number>();
 	private _intervalId: number | NodeJS.Timeout = 0;
 
@@ -128,19 +129,17 @@ export class NodeCacheStore<T> extends Hookified {
 		const finalTtl = this.resolveTtl(ttl);
 		const valueToStore = this._useClones ? this.clone(value) : value;
 
-		const isOverwrite = this._keys.has(keyValue);
+		const isOverwrite = this._values.has(keyValue);
 		this._keys.add(keyValue);
 		if (isOverwrite) {
-			const oldValue = await this._keyv.get(keyValue);
+			const oldValue = this._values.get(keyValue) as T;
 			this._stats.decreaseKSize(keyValue);
-			if (oldValue !== undefined) {
-				this._stats.decreaseVSize(oldValue);
-			}
-
+			this._stats.decreaseVSize(oldValue);
 			this._stats.decreaseCount();
 		}
 
 		await this._keyv.set(keyValue, valueToStore as T, finalTtl);
+		this._values.set(keyValue, value);
 
 		this.trackExpiration(keyValue, finalTtl);
 
@@ -221,13 +220,15 @@ export class NodeCacheStore<T> extends Hookified {
 	 */
 	public async del(key: string | number): Promise<boolean> {
 		const keyValue = key.toString();
-		const value = await this._keyv.get(keyValue);
+		const hadValue = this._values.has(keyValue);
+		const value = hadValue ? (this._values.get(keyValue) as T) : undefined;
 		const result = await this._keyv.delete(keyValue);
 		if (result) {
 			this._keys.delete(keyValue);
+			this._values.delete(keyValue);
 			this._ttls.delete(keyValue);
 			this._stats.decreaseKSize(keyValue);
-			if (value !== undefined) {
+			if (hadValue) {
 				this._stats.decreaseVSize(value);
 			}
 
@@ -256,6 +257,7 @@ export class NodeCacheStore<T> extends Hookified {
 	public async clear(): Promise<void> {
 		await this._keyv.clear();
 		this._keys.clear();
+		this._values.clear();
 		this._ttls.clear();
 		this._stats.resetStoreValues();
 	}
@@ -267,6 +269,7 @@ export class NodeCacheStore<T> extends Hookified {
 	public async flushAll(): Promise<void> {
 		await this._keyv.clear();
 		this._keys.clear();
+		this._values.clear();
 		this._ttls.clear();
 		this._stats = new Stats({ enabled: true });
 		this.emit("flush");
@@ -283,8 +286,8 @@ export class NodeCacheStore<T> extends Hookified {
 		ttl?: number | string,
 	): Promise<boolean> {
 		const keyValue = key.toString();
-		const item = await this._keyv.get(keyValue);
-		if (item !== undefined) {
+		if (this._values.has(keyValue)) {
+			const item = this._values.get(keyValue) as T;
 			const finalTtl = this.resolveTtl(ttl);
 			await this._keyv.set(keyValue, item, finalTtl);
 			this.trackExpiration(keyValue, finalTtl);
@@ -361,9 +364,10 @@ export class NodeCacheStore<T> extends Hookified {
 		}
 
 		const result = await this._keyv.get<V>(keyValue);
-		if (result !== undefined) {
+		if (this._values.has(keyValue)) {
 			await this._keyv.delete(keyValue);
 			this._keys.delete(keyValue);
+			this._values.delete(keyValue);
 			this._ttls.delete(keyValue);
 			this._stats.incrementHits();
 			this._stats.decreaseKSize(keyValue);
@@ -490,7 +494,8 @@ export class NodeCacheStore<T> extends Hookified {
 			return;
 		}
 
-		const value = await this._keyv.get(key);
+		const hadValue = this._values.has(key);
+		const value = hadValue ? this._values.get(key) : undefined;
 
 		/* v8 ignore next 3 -- @preserve: race condition guard when key is refreshed during async get */
 		if (!this.isExpired(key)) {
@@ -502,11 +507,11 @@ export class NodeCacheStore<T> extends Hookified {
 		if (this._deleteOnExpire) {
 			await this._keyv.delete(key);
 			this._keys.delete(key);
+			this._values.delete(key);
 			this._ttls.delete(key);
 			if (wasTracked) {
 				this._stats.decreaseKSize(key);
-				/* v8 ignore next 3 -- @preserve: value is typically undefined when Keyv also expires the item */
-				if (value !== undefined) {
+				if (hadValue) {
 					this._stats.decreaseVSize(value);
 				}
 

packages/node-cache/test/index.test.ts

@@ -124,6 +124,21 @@ describe("NodeCache", () => {
 		expect(cache.take(faker.string.uuid())).toBe(undefined);
 	});
 
+	test("should take falsy values and clear stats", () => {
+		const cache = new NodeCache({ checkperiod: 0 });
+		const key = faker.string.uuid();
+		cache.set(key, 0);
+
+		const taken = cache.take(key);
+		expect(taken).toBe(0);
+		expect(cache.has(key)).toBe(false);
+
+		const stats = cache.getStats();
+		expect(stats.keys).toBe(0);
+		expect(stats.ksize).toBe(0);
+		expect(stats.vsize).toBe(0);
+	});
+
 	test("should delete a key", () => {
 		const key = faker.string.uuid();
 		cache.set(key, faker.lorem.word());
@@ -330,6 +345,51 @@ describe("NodeCache", () => {
 		expect(cache.getStats().keys).toBe(0);
 	});
 
+	test("should not inflate ksize/vsize when overwriting an existing key", () => {
+		const key = faker.string.uuid();
+		const firstValue = faker.lorem.word();
+		const secondValue = faker.lorem.words(6);
+
+		const overwrittenCache = new NodeCache<string>({ checkperiod: 0 });
+		overwrittenCache.set(key, firstValue);
+		overwrittenCache.set(key, secondValue);
+
+		const baselineCache = new NodeCache<string>({ checkperiod: 0 });
+		baselineCache.set(key, secondValue);
+
+		const overwrittenStats = overwrittenCache.getStats();
+		const baselineStats = baselineCache.getStats();
+
+		expect(overwrittenStats.keys).toBe(1);
+		expect(overwrittenStats.ksize).toBe(baselineStats.ksize);
+		expect(overwrittenStats.vsize).toBe(baselineStats.vsize);
+
+		overwrittenCache.del(key);
+		const statsAfterDelete = overwrittenCache.getStats();
+		expect(statsAfterDelete.keys).toBe(0);
+		expect(statsAfterDelete.ksize).toBe(0);
+		expect(statsAfterDelete.vsize).toBe(0);
+	});
+
+	test("should fully clear ksize/vsize after overwritten key expires", async () => {
+		const cache = new NodeCache<string>({
+			checkperiod: 0,
+			deleteOnExpire: true,
+		});
+		const key = faker.string.uuid();
+
+		cache.set(key, faker.lorem.word());
+		cache.set(key, faker.lorem.word(), 0.05);
+
+		await sleep(100);
+		expect(cache.get(key)).toBe(undefined);
+
+		const stats = cache.getStats();
+		expect(stats.keys).toBe(0);
+		expect(stats.ksize).toBe(0);
+		expect(stats.vsize).toBe(0);
+	});
+
 	test("should flush all the keys", () => {
 		const cache = new NodeCache({ checkperiod: 0 });
 		const key1 = faker.string.uuid();

packages/node-cache/test/store.test.ts

@@ -906,6 +906,37 @@ describe("NodeCacheStore - overwrite stats", () => {
 		const stats = store.getStats();
 		expect(stats.keys).toBe(1);
 	});
+
+	test("should not leak vsize when overwriting undefined value", async () => {
+		const store = new NodeCacheStore();
+		const key = faker.string.uuid();
+
+		await store.set(key, undefined as unknown);
+		await store.set(key, faker.lorem.word());
+
+		const overwriteStats = store.getStats();
+		expect(overwriteStats.keys).toBe(1);
+
+		await store.del(key);
+		const afterDeleteStats = store.getStats();
+		expect(afterDeleteStats.keys).toBe(0);
+		expect(afterDeleteStats.ksize).toBe(0);
+		expect(afterDeleteStats.vsize).toBe(0);
+	});
+
+	test("should not leak vsize when undefined value expires", async () => {
+		const store = new NodeCacheStore({ checkperiod: 0, deleteOnExpire: true });
+		const key = faker.string.uuid();
+
+		await store.set(key, undefined as unknown, 50);
+		await sleep(100);
+
+		expect(await store.get(key)).toBeUndefined();
+		const stats = store.getStats();
+		expect(stats.keys).toBe(0);
+		expect(stats.ksize).toBe(0);
+		expect(stats.vsize).toBe(0);
+	});
 });
 
 describe("NodeCacheStore - setTtl with falsy values", () => {