Skip to content

Commit 9e007dd

Browse files
authored
Added safety tests for AdapterCacheRedis error and edge-case paths (TryGhost#27479)
ref TryGhost#19627 [HKG-1709](https://linear.app/ghost/issue/HKG-1709/deduplicate-concurrent-cache-misses) - We're preparing to merge changes to the Redis cache adapter(concurrent read deduplication). These tests lock in the current error-handling behavior, so any behavioral regression is caught immediately. - The current adapter silently swallows errors in several paths and the exact behavior isn't documented by existing tests. The concurrent read deduplication PR will change the `get` method structure to share in-flight promises, which makes error paths more sensitive — one failure now affects all callers waiting on the same key. We need to know exactly how errors behave today before changing anything.
1 parent 22a276a commit 9e007dd

2 files changed

Lines changed: 181 additions & 0 deletions

File tree

ghost/core/test/integration/adapters/redis/adapter-cache-redis.test.js

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,44 @@ describe('Integration: AdapterCacheRedis', function () {
132132
});
133133
});
134134

135+
describe('get with fetchData (error paths)', function () {
136+
it('does not cache errors — a subsequent call retries fetchData', async function () {
137+
const cache = createCache();
138+
const fetcher = sinon.stub();
139+
fetcher.onFirstCall().rejects(new Error('transient DB error'));
140+
fetcher.onSecondCall().resolves('recovered');
141+
142+
await cache.get('retry-key', fetcher);
143+
144+
const value = await cache.get('retry-key', fetcher);
145+
146+
assert.equal(fetcher.callCount, 2);
147+
assert.equal(value, 'recovered');
148+
});
149+
150+
it('stores and retrieves complex nested objects', async function () {
151+
const cache = createCache();
152+
const complexValue = {
153+
posts: [
154+
{
155+
id: 'abc123',
156+
title: 'Test Post',
157+
tags: [{id: 't1', name: 'News'}],
158+
authors: [{id: 'a1', name: 'Jane'}]
159+
}
160+
],
161+
meta: {
162+
pagination: {page: 1, limit: 15, pages: 3, total: 42, next: 2, prev: null}
163+
}
164+
};
165+
166+
await cache.set('complex', complexValue);
167+
const retrieved = await cache.get('complex');
168+
169+
assert.deepEqual(retrieved, complexValue);
170+
});
171+
});
172+
135173
describe('without a keyPrefix', function () {
136174
it('still stores and retrieves values', async function () {
137175
const cache = buildCache(undefined);

ghost/core/test/unit/server/adapters/lib/redis/adapter-cache-redis.test.js

Lines changed: 143 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,130 @@ describe('Adapter Cache Redis', function () {
111111
assert.equal(secondRead, 'Da Value');
112112
});
113113

114+
it('returns undefined and logs error when fetchData rejects on a cache miss', async function () {
115+
const redisCacheInstanceStub = {
116+
get: sinon.stub().resolves(null),
117+
set: sinon.stub().resolves(),
118+
store: {
119+
getClient: sinon.stub().returns({
120+
on: sinon.stub()
121+
})
122+
}
123+
};
124+
const cache = new RedisCache({
125+
cache: redisCacheInstanceStub
126+
});
127+
128+
const fetchData = sinon.stub().rejects(new Error('DB is down'));
129+
130+
const value = await cache.get('key', fetchData);
131+
132+
assert.equal(value, undefined);
133+
sinon.assert.calledOnce(fetchData);
134+
sinon.assert.calledOnce(logging.error);
135+
});
136+
137+
it('retries fetchData on next call after a previous fetchData rejection', async function () {
138+
let cachedValue = null;
139+
const redisCacheInstanceStub = {
140+
get: sinon.stub().callsFake(() => cachedValue),
141+
set: sinon.stub().callsFake((_key, value) => {
142+
cachedValue = value;
143+
}),
144+
store: {
145+
getClient: sinon.stub().returns({
146+
on: sinon.stub()
147+
})
148+
}
149+
};
150+
const cache = new RedisCache({
151+
cache: redisCacheInstanceStub
152+
});
153+
154+
const fetchData = sinon.stub();
155+
fetchData.onFirstCall().rejects(new Error('transient failure'));
156+
fetchData.onSecondCall().resolves('recovered value');
157+
158+
await cache.get('key', fetchData);
159+
160+
const value = await cache.get('key', fetchData);
161+
162+
assert.equal(fetchData.callCount, 2);
163+
assert.equal(value, 'recovered value');
164+
});
165+
166+
it('does not call fetchData when the underlying Redis get throws', async function () {
167+
const redisCacheInstanceStub = {
168+
get: sinon.stub().rejects(new Error('Redis connection lost')),
169+
set: sinon.stub().resolves(),
170+
store: {
171+
getClient: sinon.stub().returns({
172+
on: sinon.stub()
173+
})
174+
}
175+
};
176+
const cache = new RedisCache({
177+
cache: redisCacheInstanceStub
178+
});
179+
180+
const fetchData = sinon.stub().resolves('fallback value');
181+
182+
const value = await cache.get('key', fetchData);
183+
184+
assert.equal(value, undefined);
185+
assert.equal(fetchData.callCount, 0);
186+
sinon.assert.calledOnce(logging.error);
187+
});
188+
189+
it('returns the cached value when background refresh fails', async function () {
190+
const KEY = 'bg-refresh-error';
191+
let cachedValue = null;
192+
193+
const redisCacheInstanceStub = {
194+
get: function (key) {
195+
assert(key === KEY);
196+
return cachedValue;
197+
},
198+
set: function (key, value) {
199+
assert(key === KEY);
200+
cachedValue = value;
201+
},
202+
ttl: function () {
203+
return 5;
204+
},
205+
store: {
206+
getClient: sinon.stub().returns({
207+
on: sinon.stub()
208+
})
209+
}
210+
};
211+
const cache = new RedisCache({
212+
cache: redisCacheInstanceStub,
213+
ttl: 100,
214+
refreshAheadFactor: 0.2
215+
});
216+
217+
const fetchData = sinon.stub();
218+
fetchData.onFirstCall().resolves('Original Value');
219+
fetchData.onSecondCall().rejects(new Error('refresh failed'));
220+
221+
const first = await cache.get(KEY, fetchData);
222+
assert.equal(first, 'Original Value');
223+
sinon.assert.calledOnce(fetchData);
224+
225+
const second = await cache.get(KEY, fetchData);
226+
assert.equal(second, 'Original Value');
227+
sinon.assert.calledTwice(fetchData);
228+
229+
// The .catch() handler fires asynchronously — wait for it to settle
230+
await new Promise((resolve) => {
231+
setTimeout(resolve, 10);
232+
});
233+
sinon.assert.calledOnce(logging.error);
234+
assert.equal(logging.error.firstCall.args[0].message, 'There was an error refreshing cache data in the background');
235+
assert.equal(logging.error.firstCall.args[0].error.message, 'refresh failed');
236+
});
237+
114238
it('Can do a background update of the cache', async function () {
115239
const KEY = 'update-cache-in-background';
116240
let cachedValue = null;
@@ -188,6 +312,25 @@ describe('Adapter Cache Redis', function () {
188312
assert.equal(cacheStub.set.args[0][0], `${PREFIX_HASH}key-here`);
189313
});
190314

315+
it('logs error and does not throw when the underlying Redis set throws', async function () {
316+
const redisCacheInstanceStub = {
317+
set: sinon.stub().rejects(new Error('Redis write failed')),
318+
store: {
319+
getClient: sinon.stub().returns({
320+
on: sinon.stub()
321+
})
322+
}
323+
};
324+
const cache = new RedisCache({
325+
cache: redisCacheInstanceStub
326+
});
327+
328+
const value = await cache.set('key-here', 'new value');
329+
330+
assert.equal(value, undefined);
331+
sinon.assert.calledOnce(logging.error);
332+
});
333+
191334
it('sets a key based on keyPrefix', async function () {
192335
const cacheStub = createCacheStub({keyPrefix: 'testing-prefix:'});
193336
const cache = new RedisCache({

0 commit comments

Comments
 (0)