Skip to content

Commit 7bec4d3

Browse files
committed
fix: replace unbounded retry loops with bounded exponential backoff in Turtle and Block cache methods
- Extract shared retryWithBackoff utility (js/utils/retryWithBackoff.js) - Fix Turtle._createCache: unbounded setTimeout retry -> bounded (max 20 retries) - Fix Turtle.updateCache: unbounded async recursion with broken promise chain -> bounded retry - Fix Turtle.updateCache: incorrect log message ('Block container' -> 'Turtle container') - Refactor Block._createCache and Block.updateCache to use shared utility - Register retryWithBackoff in RequireJS loader config (loader.js, activity.js) - Add 19 unit tests for retryWithBackoff utility
1 parent 70ed297 commit 7bec4d3

File tree

6 files changed

+583
-98
lines changed

6 files changed

+583
-98
lines changed
Lines changed: 351 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,351 @@
1+
/**
2+
* @license
3+
* MusicBlocks v3.4.1
4+
*
5+
* This program is free software: you can redistribute it and/or modify
6+
* it under the terms of the GNU Affero General Public License as published by
7+
* the Free Software Foundation, either version 3 of the License, or
8+
* (at your option) any later version.
9+
*
10+
* This program is distributed in the hope that it will be useful,
11+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
12+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
13+
* GNU Affero General Public License for more details.
14+
*
15+
* You should have received a copy of the GNU Affero General Public License
16+
* along with this program. If not, see <https://www.gnu.org/licenses/>.
17+
*/
18+
19+
const { retryWithBackoff } = require("../utils/retryWithBackoff");
20+
21+
/**
22+
* A no-op delay function that resolves immediately.
23+
* Injected into retryWithBackoff to avoid real timers in tests.
24+
*/
25+
const instantDelay = () => Promise.resolve();
26+
27+
describe("retryWithBackoff", () => {
28+
describe("immediate success (no retries needed)", () => {
29+
it("should resolve immediately when check returns truthy on first call", async () => {
30+
const onSuccess = jest.fn();
31+
const check = jest.fn(() => ({ x: 0, y: 0, width: 100, height: 100 }));
32+
33+
const result = await retryWithBackoff({
34+
check,
35+
onSuccess,
36+
delayFn: instantDelay,
37+
maxRetries: 5
38+
});
39+
40+
expect(check).toHaveBeenCalledTimes(1);
41+
expect(onSuccess).toHaveBeenCalledTimes(1);
42+
expect(onSuccess).toHaveBeenCalledWith({ x: 0, y: 0, width: 100, height: 100 });
43+
expect(result).toEqual({ x: 0, y: 0, width: 100, height: 100 });
44+
});
45+
46+
it("should pass the check result to onSuccess", async () => {
47+
const bounds = { x: 10, y: 20, width: 200, height: 300 };
48+
const onSuccess = jest.fn();
49+
50+
await retryWithBackoff({
51+
check: () => bounds,
52+
onSuccess,
53+
delayFn: instantDelay
54+
});
55+
56+
expect(onSuccess).toHaveBeenCalledWith(bounds);
57+
});
58+
59+
it("should not call onRetry when check succeeds immediately", async () => {
60+
const onRetry = jest.fn();
61+
62+
await retryWithBackoff({
63+
check: () => true,
64+
onSuccess: jest.fn(),
65+
onRetry,
66+
delayFn: instantDelay
67+
});
68+
69+
expect(onRetry).not.toHaveBeenCalled();
70+
});
71+
});
72+
73+
describe("retry behavior", () => {
74+
it("should retry when check returns falsy and eventually succeed", async () => {
75+
let callCount = 0;
76+
const check = jest.fn(() => {
77+
callCount++;
78+
// Fail first 3 times, succeed on 4th
79+
return callCount >= 4 ? { ready: true } : null;
80+
});
81+
const onSuccess = jest.fn();
82+
83+
await retryWithBackoff({
84+
check,
85+
onSuccess,
86+
delayFn: instantDelay,
87+
maxRetries: 10
88+
});
89+
90+
expect(check).toHaveBeenCalledTimes(4);
91+
expect(onSuccess).toHaveBeenCalledWith({ ready: true });
92+
});
93+
94+
it("should call onRetry before each retry attempt with the attempt number", async () => {
95+
let callCount = 0;
96+
const onRetry = jest.fn();
97+
98+
await retryWithBackoff({
99+
check: () => {
100+
callCount++;
101+
return callCount >= 3 ? true : null;
102+
},
103+
onSuccess: jest.fn(),
104+
onRetry,
105+
delayFn: instantDelay,
106+
maxRetries: 10
107+
});
108+
109+
// 2 retries before success on 3rd check
110+
expect(onRetry).toHaveBeenCalledTimes(2);
111+
expect(onRetry).toHaveBeenNthCalledWith(1, 0);
112+
expect(onRetry).toHaveBeenNthCalledWith(2, 1);
113+
});
114+
115+
it("should work without onRetry callback", async () => {
116+
let callCount = 0;
117+
118+
const result = await retryWithBackoff({
119+
check: () => {
120+
callCount++;
121+
return callCount >= 2 ? "done" : null;
122+
},
123+
onSuccess: jest.fn(),
124+
delayFn: instantDelay,
125+
maxRetries: 5
126+
});
127+
128+
expect(result).toBe("done");
129+
});
130+
131+
it("should call delayFn with exponentially increasing delays", async () => {
132+
let callCount = 0;
133+
const delayFn = jest.fn(() => Promise.resolve());
134+
135+
await retryWithBackoff({
136+
check: () => {
137+
callCount++;
138+
return callCount >= 4 ? true : null;
139+
},
140+
onSuccess: jest.fn(),
141+
delayFn,
142+
maxRetries: 10,
143+
initialDelay: 50
144+
});
145+
146+
// 3 retries: delays should be 50*2^0=50, 50*2^1=100, 50*2^2=200
147+
expect(delayFn).toHaveBeenCalledTimes(3);
148+
expect(delayFn).toHaveBeenNthCalledWith(1, 50);
149+
expect(delayFn).toHaveBeenNthCalledWith(2, 100);
150+
expect(delayFn).toHaveBeenNthCalledWith(3, 200);
151+
});
152+
});
153+
154+
describe("max retries exceeded", () => {
155+
it("should reject with an error when max retries are exhausted", async () => {
156+
const check = jest.fn(() => null);
157+
158+
await expect(
159+
retryWithBackoff({
160+
check,
161+
onSuccess: jest.fn(),
162+
delayFn: instantDelay,
163+
maxRetries: 3,
164+
errorMessage: "COULD NOT CREATE CACHE"
165+
})
166+
).rejects.toThrow("COULD NOT CREATE CACHE");
167+
168+
// check called for attempts 0, 1, 2, 3 then fail on count=4 > maxRetries=3
169+
expect(check).toHaveBeenCalledTimes(4);
170+
});
171+
172+
it("should use default error message when none provided", async () => {
173+
await expect(
174+
retryWithBackoff({
175+
check: () => null,
176+
onSuccess: jest.fn(),
177+
delayFn: instantDelay,
178+
maxRetries: 0
179+
})
180+
).rejects.toThrow("Retry limit exceeded");
181+
});
182+
183+
it("should use custom error message", async () => {
184+
await expect(
185+
retryWithBackoff({
186+
check: () => null,
187+
onSuccess: jest.fn(),
188+
delayFn: instantDelay,
189+
maxRetries: 0,
190+
errorMessage: "Turtle._createCache: bounds unavailable"
191+
})
192+
).rejects.toThrow("Turtle._createCache: bounds unavailable");
193+
});
194+
});
195+
196+
describe("default parameters", () => {
197+
it("should default maxRetries to 20", async () => {
198+
let callCount = 0;
199+
200+
// Succeed on attempt 19 (within default 20)
201+
await retryWithBackoff({
202+
check: () => {
203+
callCount++;
204+
return callCount > 19 ? true : null;
205+
},
206+
onSuccess: jest.fn(),
207+
delayFn: instantDelay
208+
});
209+
210+
expect(callCount).toBe(20);
211+
});
212+
213+
it("should default initialDelay to 50ms", async () => {
214+
let callCount = 0;
215+
const delayFn = jest.fn(() => Promise.resolve());
216+
217+
await retryWithBackoff({
218+
check: () => {
219+
callCount++;
220+
return callCount >= 2 ? true : null;
221+
},
222+
onSuccess: jest.fn(),
223+
delayFn
224+
});
225+
226+
// First delay should be 50 * 2^0 = 50
227+
expect(delayFn).toHaveBeenCalledWith(50);
228+
});
229+
});
230+
231+
describe("error propagation", () => {
232+
it("should reject if check throws an error", async () => {
233+
await expect(
234+
retryWithBackoff({
235+
check: () => {
236+
throw new Error("check exploded");
237+
},
238+
onSuccess: jest.fn(),
239+
delayFn: instantDelay
240+
})
241+
).rejects.toThrow("check exploded");
242+
});
243+
244+
it("should reject if onSuccess throws an error", async () => {
245+
await expect(
246+
retryWithBackoff({
247+
check: () => true,
248+
onSuccess: () => {
249+
throw new Error("onSuccess failed");
250+
},
251+
delayFn: instantDelay
252+
})
253+
).rejects.toThrow("onSuccess failed");
254+
});
255+
256+
it("should reject if async onSuccess rejects", async () => {
257+
await expect(
258+
retryWithBackoff({
259+
check: () => true,
260+
onSuccess: async () => {
261+
throw new Error("async onSuccess failed");
262+
},
263+
delayFn: instantDelay
264+
})
265+
).rejects.toThrow("async onSuccess failed");
266+
});
267+
});
268+
269+
describe("edge cases", () => {
270+
it("should handle check returning various truthy values", async () => {
271+
const onSuccess = jest.fn();
272+
273+
// Number truthy
274+
await retryWithBackoff({
275+
check: () => 42,
276+
onSuccess,
277+
delayFn: instantDelay
278+
});
279+
expect(onSuccess).toHaveBeenCalledWith(42);
280+
281+
// String truthy
282+
onSuccess.mockClear();
283+
await retryWithBackoff({
284+
check: () => "ready",
285+
onSuccess,
286+
delayFn: instantDelay
287+
});
288+
expect(onSuccess).toHaveBeenCalledWith("ready");
289+
290+
// Array truthy
291+
onSuccess.mockClear();
292+
await retryWithBackoff({
293+
check: () => [1, 2, 3],
294+
onSuccess,
295+
delayFn: instantDelay
296+
});
297+
expect(onSuccess).toHaveBeenCalledWith([1, 2, 3]);
298+
});
299+
300+
it("should treat 0, empty string, and false as falsy (trigger retry)", async () => {
301+
let callCount = 0;
302+
303+
// Returns 0 first, then true
304+
await retryWithBackoff({
305+
check: () => {
306+
callCount++;
307+
if (callCount === 1) return 0;
308+
if (callCount === 2) return "";
309+
if (callCount === 3) return false;
310+
return "success";
311+
},
312+
onSuccess: jest.fn(),
313+
delayFn: instantDelay,
314+
maxRetries: 10
315+
});
316+
317+
expect(callCount).toBe(4);
318+
});
319+
320+
it("should handle maxRetries of 0 (fail immediately if check is falsy)", async () => {
321+
const check = jest.fn(() => null);
322+
323+
await expect(
324+
retryWithBackoff({
325+
check,
326+
onSuccess: jest.fn(),
327+
delayFn: instantDelay,
328+
maxRetries: 0
329+
})
330+
).rejects.toThrow();
331+
});
332+
333+
it("should handle maxRetries of 1 (one retry allowed)", async () => {
334+
let callCount = 0;
335+
const onSuccess = jest.fn();
336+
337+
await retryWithBackoff({
338+
check: () => {
339+
callCount++;
340+
return callCount >= 2 ? true : null;
341+
},
342+
onSuccess,
343+
delayFn: instantDelay,
344+
maxRetries: 1
345+
});
346+
347+
expect(callCount).toBe(2);
348+
expect(onSuccess).toHaveBeenCalled();
349+
});
350+
});
351+
});

js/activity.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,7 @@ let MYDEFINES = [
7171
// 'mespeak',
7272
"Chart",
7373
"utils/utils",
74+
"utils/retryWithBackoff",
7475
"activity/artwork",
7576
"widgets/status",
7677
"widgets/help",

0 commit comments

Comments
 (0)