Skip to content

Commit 50a2bac

Browse files
zharinovzharinov
authored
fix(cache): Improve SQLite cache error handling (#42557)
* fix(cache): Improve SQLite cache error handling * Update minimum coverage threshold for functions * Apply suggestions from code review Co-authored-by: Sergei Zharinov <zharinov@users.noreply.github.com>
1 parent 27f0797 commit 50a2bac

2 files changed

Lines changed: 245 additions & 63 deletions

File tree

‎lib/util/cache/package/impl/sqlite.spec.ts‎

Lines changed: 198 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -1,22 +1,53 @@
1+
import { promisify } from 'node:util';
2+
import zlib from 'node:zlib';
13
import { withDir } from 'tmp-promise';
4+
import { logger as _logger } from '~test/util.ts';
25
import { GlobalConfig } from '../../../../config/global.ts';
36
import { PackageCacheSqlite } from './sqlite.ts';
47

5-
function withSqlite<T>(
6-
fn: (sqlite: PackageCacheSqlite) => Promise<T>,
7-
): Promise<T> {
8+
const { logger } = _logger;
9+
10+
export const brotliCompress = promisify(zlib.brotliCompress);
11+
12+
function withSqliteDir<T>(fn: (cacheDir: string) => Promise<T>): Promise<T> {
813
return withDir(
914
async ({ path }) => {
1015
GlobalConfig.set({ cacheDir: path });
11-
const sqlite = await PackageCacheSqlite.create(path);
12-
const res = await fn(sqlite);
13-
await sqlite.destroy();
14-
return res;
16+
return await fn(path);
1517
},
1618
{ unsafeCleanup: true },
1719
);
1820
}
1921

22+
function withSqlite<T>(
23+
fn: (sqlite: PackageCacheSqlite) => Promise<T>,
24+
): Promise<T> {
25+
return withSqliteDir(async (cacheDir) => {
26+
const sqlite = await PackageCacheSqlite.create(cacheDir);
27+
28+
try {
29+
return await fn(sqlite);
30+
} finally {
31+
await sqlite.destroy();
32+
}
33+
});
34+
}
35+
36+
function insertRawCacheEntry(
37+
sqlite: PackageCacheSqlite,
38+
key: string,
39+
value: Buffer,
40+
): void {
41+
sqlite.client
42+
.prepare(
43+
`
44+
INSERT INTO package_cache (namespace, key, expiry, data)
45+
VALUES (?, ?, unixepoch() + 300, ?)
46+
`,
47+
)
48+
.run('_test-namespace', key, value);
49+
}
50+
2051
describe('util/cache/package/impl/sqlite', () => {
2152
describe('get', () => {
2253
it('returns undefined on cache miss', async () => {
@@ -25,6 +56,104 @@ describe('util/cache/package/impl/sqlite', () => {
2556
);
2657

2758
expect(res).toBeUndefined();
59+
expect(logger.warn).not.toHaveBeenCalled();
60+
});
61+
62+
it('returns undefined for invalid compressed payload', async () => {
63+
const res = await withSqlite(async (sqlite) => {
64+
insertRawCacheEntry(sqlite, 'bar', Buffer.from('not-brotli'));
65+
66+
return sqlite.get('_test-namespace', 'bar');
67+
});
68+
69+
expect(res).toBeUndefined();
70+
expect(logger.once.warn).toHaveBeenCalledWith(
71+
{ err: expect.any(Error) },
72+
'Error while reading SQLite cache value',
73+
);
74+
expect(logger.warn).not.toHaveBeenCalled();
75+
});
76+
77+
it('returns undefined for invalid JSON payload', async () => {
78+
const res = await withSqlite(async (sqlite) => {
79+
const compressed = await brotliCompress('not-json');
80+
insertRawCacheEntry(sqlite, 'bar', compressed);
81+
82+
return sqlite.get('_test-namespace', 'bar');
83+
});
84+
85+
expect(res).toBeUndefined();
86+
expect(logger.once.warn).toHaveBeenCalledWith(
87+
{ err: expect.any(Error) },
88+
'Error while reading SQLite cache value',
89+
);
90+
expect(logger.warn).not.toHaveBeenCalled();
91+
});
92+
93+
it('returns undefined when the read fails', async () => {
94+
await withSqliteDir(async (cacheDir) => {
95+
const sqlite = await PackageCacheSqlite.create(cacheDir);
96+
97+
try {
98+
sqlite.client.exec('DROP TABLE package_cache');
99+
100+
const res = await sqlite.get('_test-namespace', 'bar');
101+
102+
expect(res).toBeUndefined();
103+
expect(logger.once.warn).toHaveBeenCalledWith(
104+
{ err: expect.any(Error) },
105+
'Error while reading SQLite cache value',
106+
);
107+
expect(logger.trace).not.toHaveBeenCalledWith(
108+
{ namespace: '_test-namespace', key: 'bar' },
109+
'Cache miss',
110+
);
111+
expect(logger.warn).not.toHaveBeenCalled();
112+
} finally {
113+
sqlite.client.close();
114+
}
115+
});
116+
});
117+
});
118+
119+
describe('set', () => {
120+
it('logs a warning and continues when serialization fails', async () => {
121+
const circular: { self?: unknown } = {};
122+
circular.self = circular;
123+
124+
await expect(
125+
withSqlite((sqlite) =>
126+
sqlite.set('_test-namespace', 'bar', circular, 5),
127+
),
128+
).resolves.toBeUndefined();
129+
130+
expect(logger.once.warn).toHaveBeenCalledWith(
131+
{ err: expect.any(TypeError) },
132+
'Error while setting SQLite cache value',
133+
);
134+
expect(logger.warn).not.toHaveBeenCalled();
135+
});
136+
137+
it('logs a warning and continues when the write fails', async () => {
138+
await withSqliteDir(async (cacheDir) => {
139+
const sqlite = await PackageCacheSqlite.create(cacheDir);
140+
141+
try {
142+
sqlite.client.exec('DROP TABLE package_cache');
143+
144+
await expect(
145+
sqlite.set('_test-namespace', 'bar', { foo: 'bar' }, 5),
146+
).resolves.toBeUndefined();
147+
148+
expect(logger.once.warn).toHaveBeenCalledWith(
149+
{ err: expect.any(Error) },
150+
'Error while setting SQLite cache value',
151+
);
152+
expect(logger.warn).not.toHaveBeenCalled();
153+
} finally {
154+
sqlite.client.close();
155+
}
156+
});
28157
});
29158
});
30159

@@ -54,46 +183,76 @@ describe('util/cache/package/impl/sqlite', () => {
54183

55184
describe('destroy', () => {
56185
it('deletes expired entries and closes database', async () => {
57-
const res = await withDir(
58-
async ({ path }) => {
59-
GlobalConfig.set({ cacheDir: path });
60-
61-
const client1 = await PackageCacheSqlite.create(path);
62-
await client1.set('_test-namespace', 'expired', 'old', -1);
63-
await client1.set('_test-namespace', 'valid', 'fresh', 5);
64-
await client1.destroy();
65-
66-
const client2 = await PackageCacheSqlite.create(path);
67-
const expired = await client2.get('_test-namespace', 'expired');
68-
const valid = await client2.get('_test-namespace', 'valid');
69-
await client2.destroy();
70-
return { expired, valid };
71-
},
72-
{ unsafeCleanup: true },
73-
);
186+
const res = await withSqliteDir(async (cacheDir) => {
187+
const client1 = await PackageCacheSqlite.create(cacheDir);
188+
await client1.set('_test-namespace', 'expired', 'old', -1);
189+
await client1.set('_test-namespace', 'valid', 'fresh', 5);
190+
await client1.destroy();
191+
192+
const client2 = await PackageCacheSqlite.create(cacheDir);
193+
const expired = await client2.get('_test-namespace', 'expired');
194+
const valid = await client2.get('_test-namespace', 'valid');
195+
await client2.destroy();
196+
197+
return { expired, valid };
198+
});
74199

75200
expect(res.expired).toBeUndefined();
76201
expect(res.valid).toBe('fresh');
77202
});
203+
204+
it('resolves and still closes when cleanup throws', async () => {
205+
await withSqliteDir(async (cacheDir) => {
206+
const sqlite = await PackageCacheSqlite.create(cacheDir);
207+
sqlite.client.exec('DROP TABLE package_cache');
208+
209+
await expect(sqlite.destroy()).resolves.toBeUndefined();
210+
211+
expect(logger.warn).toHaveBeenCalledWith(
212+
{ err: expect.any(Error) },
213+
'SQLite package cache cleanup failed',
214+
);
215+
expect(() => sqlite.client.prepare('SELECT 1').get()).toThrow();
216+
});
217+
});
218+
219+
it('resolves when close throws', async () => {
220+
await withSqliteDir(async (cacheDir) => {
221+
const sqlite = await PackageCacheSqlite.create(cacheDir);
222+
insertRawCacheEntry(sqlite, 'bar', Buffer.from('value'));
223+
const iterator = sqlite.client
224+
.prepare('SELECT * FROM package_cache')
225+
.iterate();
226+
227+
try {
228+
iterator.next();
229+
230+
await expect(sqlite.destroy()).resolves.toBeUndefined();
231+
232+
expect(logger.warn).toHaveBeenCalledWith(
233+
{ err: expect.any(Error) },
234+
'SQLite package cache close failed',
235+
);
236+
} finally {
237+
iterator.return?.();
238+
sqlite.client.close();
239+
}
240+
});
241+
});
78242
});
79243

80244
describe('persistence', () => {
81245
it('retrieves value from persistent storage after reopening', async () => {
82-
const res = await withDir(
83-
async ({ path }) => {
84-
GlobalConfig.set({ cacheDir: path });
85-
86-
const client1 = await PackageCacheSqlite.create(path);
87-
await client1.set('_test-namespace', 'bar', 'baz', 5);
88-
await client1.destroy();
89-
90-
const client2 = await PackageCacheSqlite.create(path);
91-
const data = await client2.get('_test-namespace', 'bar');
92-
await client2.destroy();
93-
return data;
94-
},
95-
{ unsafeCleanup: true },
96-
);
246+
const res = await withSqliteDir(async (cacheDir) => {
247+
const client1 = await PackageCacheSqlite.create(cacheDir);
248+
await client1.set('_test-namespace', 'bar', 'baz', 5);
249+
await client1.destroy();
250+
251+
const client2 = await PackageCacheSqlite.create(cacheDir);
252+
const data = await client2.get('_test-namespace', 'bar');
253+
await client2.destroy();
254+
return data;
255+
});
97256

98257
expect(res).toBe('baz');
99258
});

‎lib/util/cache/package/impl/sqlite.ts‎

Lines changed: 47 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,8 @@ async function decompress<T>(input: Buffer): Promise<T> {
3030
}
3131

3232
export class PackageCacheSqlite extends PackageCacheBase {
33+
private static readonly busyTimeoutMs = 100;
34+
3335
static async create(cacheDir: string): Promise<PackageCacheSqlite> {
3436
const Sqlite = await sqlite();
3537
const sqliteDir = upath.join(cacheDir, 'renovate/renovate-cache-sqlite');
@@ -51,14 +53,15 @@ export class PackageCacheSqlite extends PackageCacheBase {
5153
private readonly deleteExpiredRows: Statement<unknown[]>;
5254
private readonly countStatement: Statement<unknown[]>;
5355

54-
private readonly client: Database;
56+
readonly client: Database;
5557

5658
private constructor(client: Database) {
5759
super();
5860
this.client = client;
5961

6062
client.pragma('journal_mode = WAL');
6163
client.pragma("encoding = 'UTF-8'");
64+
client.pragma(`busy_timeout = ${PackageCacheSqlite.busyTimeoutMs}`);
6265

6366
client
6467
.prepare(
@@ -111,40 +114,60 @@ export class PackageCacheSqlite extends PackageCacheBase {
111114
value: unknown,
112115
hardTtlMinutes: number,
113116
): Promise<void> {
114-
const compressedData = await compress(value);
115-
const ttlSeconds = hardTtlMinutes * 60;
116-
this.upsertStatement.run({
117-
namespace,
118-
key,
119-
data: compressedData,
120-
ttlSeconds,
121-
});
117+
try {
118+
const compressedData = await compress(value);
119+
const ttlSeconds = hardTtlMinutes * 60;
120+
this.upsertStatement.run({
121+
namespace,
122+
key,
123+
data: compressedData,
124+
ttlSeconds,
125+
});
126+
} catch (err) {
127+
logger.once.warn({ err }, 'Error while setting SQLite cache value');
128+
}
122129
}
123130

124131
override async get<T = unknown>(
125132
namespace: PackageCacheNamespace,
126133
key: string,
127134
): Promise<T | undefined> {
128-
const data = this.getStatement.get({ namespace, key }) as
129-
| Buffer
130-
| undefined;
131-
132-
if (!data) {
135+
try {
136+
const data = this.getStatement.get({ namespace, key }) as
137+
| Buffer
138+
| undefined;
139+
140+
if (!data) {
141+
logger.trace({ namespace, key }, 'Cache miss');
142+
return undefined;
143+
}
144+
145+
return await decompress<T>(data);
146+
} catch (err) {
147+
logger.once.warn({ err }, 'Error while reading SQLite cache value');
133148
return undefined;
134149
}
135-
136-
return await decompress<T>(data);
137150
}
138151

139152
override destroy(): Promise<void> {
140-
const startTime = Date.now();
141-
const totalCount = this.countStatement.get() as number;
142-
const { changes: deletedCount } = this.deleteExpiredRows.run();
143-
const durationMs = Date.now() - startTime;
144-
logger.debug(
145-
`SQLite package cache: deleted ${deletedCount} of ${totalCount} entries in ${durationMs}ms`,
146-
);
147-
this.client.close();
153+
try {
154+
const startTime = Date.now();
155+
const totalCount = this.countStatement.get() as number;
156+
const { changes: deletedCount } = this.deleteExpiredRows.run();
157+
const durationMs = Date.now() - startTime;
158+
logger.debug(
159+
`SQLite package cache: deleted ${deletedCount} of ${totalCount} entries in ${durationMs}ms`,
160+
);
161+
} catch (err) {
162+
logger.warn({ err }, 'SQLite package cache cleanup failed');
163+
}
164+
165+
try {
166+
this.client.close();
167+
} catch (err) {
168+
logger.warn({ err }, 'SQLite package cache close failed');
169+
}
170+
148171
return Promise.resolve();
149172
}
150173
}

0 commit comments

Comments
 (0)