Skip to content

Commit 2fb68b9

Browse files
authored
fix(github): prevent PR cache sync from exhausting all pages (renovatebot#41462)
* fix(github): prevent PR cache sync from exhausting all pages * refactor(github): derive cutoff from cached items, simplify reconcile - When lastModified is missing but items exist (via updateItem()), derive cutoff from max(item.updated_at) instead of falling back to reconcile's return value. Eliminates the fallback branch. - reconcile() now called purely for side effects; return value ignored. - MAX_SYNC_PAGES guard checks needNextPageSync (not needNextPageFetch) to avoid spurious warning when cutoff already stopped the loop. - Add test for mixed pages (Renovate + non-Renovate PRs interleaved). - Update derived-cutoff test for caches without lastModified. * fix(github): advance PR cache watermark from unfiltered page * fix(github): add comment clarifying MAX_SYNC_PAGES is a safety net
1 parent 07c6e73 commit 2fb68b9

File tree

4 files changed

+437
-14
lines changed

4 files changed

+437
-14
lines changed

lib/modules/platform/github/api-cache.spec.ts

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,65 @@ describe('modules/platform/github/api-cache', () => {
9090
});
9191
});
9292

93+
describe('getLastModified', () => {
94+
it('returns undefined when no lastModified in cache', () => {
95+
const apiCache = new ApiCache({ items: {} });
96+
97+
expect(apiCache.getLastModified()).toBeUndefined();
98+
});
99+
100+
it('returns stored value when present', () => {
101+
const apiCache = new ApiCache({ items: {}, lastModified: t2 });
102+
103+
expect(apiCache.getLastModified()).toBe(t2);
104+
});
105+
106+
it('returns updated value after reconcile', () => {
107+
const apiCache = new ApiCache({ items: {}, lastModified: t1 });
108+
109+
apiCache.reconcile([{ number: 1, updated_at: t3 }]);
110+
111+
expect(apiCache.getLastModified()).toBe(t3);
112+
});
113+
});
114+
115+
describe('updateLastModified', () => {
116+
it('sets lastModified when not present', () => {
117+
const apiCache = new ApiCache({ items: {} });
118+
119+
apiCache.updateLastModified(t2);
120+
121+
expect(apiCache.getLastModified()).toBe(t2);
122+
});
123+
124+
it('advances lastModified to newer timestamp', () => {
125+
const apiCache = new ApiCache({ items: {}, lastModified: t1 });
126+
127+
apiCache.updateLastModified(t3);
128+
129+
expect(apiCache.getLastModified()).toBe(t3);
130+
});
131+
132+
it('does not regress lastModified to older timestamp', () => {
133+
const apiCache = new ApiCache({ items: {}, lastModified: t3 });
134+
135+
apiCache.updateLastModified(t1);
136+
137+
expect(apiCache.getLastModified()).toBe(t3);
138+
});
139+
});
140+
93141
describe('reconcile', () => {
142+
it('returns false for empty page', () => {
143+
const apiCache = new ApiCache({
144+
items: { 1: { number: 1, updated_at: t1 } },
145+
lastModified: t1,
146+
});
147+
148+
expect(apiCache.reconcile([])).toBeFalse();
149+
expect(apiCache.getLastModified()).toBe(t1);
150+
});
151+
94152
it('appends new items', () => {
95153
const apiCache = new ApiCache({ items: {} });
96154

lib/modules/platform/github/api-cache.ts

Lines changed: 22 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,7 @@ export class ApiCache<T extends ApiPageItem> {
1111
}
1212

1313
getItems(): T[] {
14-
const items = Object.values(this.cache.items);
15-
return items;
14+
return Object.values(this.cache.items);
1615
}
1716

1817
getItem(number: number): T | null {
@@ -29,17 +28,31 @@ export class ApiCache<T extends ApiPageItem> {
2928
this.cache.items[item.number] = item;
3029
}
3130

31+
getLastModified(): string | undefined {
32+
return this.cache.lastModified;
33+
}
34+
35+
updateLastModified(timestamp: string): void {
36+
const current = this.cache.lastModified
37+
? DateTime.fromISO(this.cache.lastModified)
38+
: null;
39+
const incoming = DateTime.fromISO(timestamp);
40+
if (!current || incoming > current) {
41+
this.cache.lastModified = timestamp;
42+
}
43+
}
44+
3245
/**
33-
* Copies items from `page` to `cache`.
34-
* Updates internal cache timestamp.
46+
* Copies items from `page` to cache and updates the internal timestamp.
3547
*
36-
* @param cache Cache object
37-
* @param page List of cacheable items, sorted by `updated_at` field
38-
* starting from the most recently updated.
39-
* @returns `true` when the next page is likely to contain fresh items,
40-
* otherwise `false`.
48+
* @param page Items sorted by `updated_at` desc (most recent first).
49+
* @returns `true` when the next page is likely to contain fresh items.
4150
*/
4251
reconcile(page: T[]): boolean {
52+
if (page.length === 0) {
53+
return false;
54+
}
55+
4356
const { items } = this.cache;
4457
let { lastModified } = this.cache;
4558

0 commit comments

Comments
 (0)