Skip to content

Commit 4ef2af2

Browse files
authored
Fix recorder: stop mutating shared defaultRetryOptions (#5572)
`Object.assign(defaultRetryOptions, retryOpts)` returned and used the shared module-level `defaultRetryOptions` object as the merge target, so any custom retry option (e.g. `minTimeout`, `factor`) persisted into the defaults for every later `recorder.retry()` call. Subsequent steps and tests then silently inherited the previous run's overrides. A test that explicitly set `minTimeout=1000` poisoned the defaults for the rest of the session — later steps used 1000ms even when no `minTimeout` was passed. Pass a fresh `{}` as the first arg to `Object.assign` so a new merged options object is produced each call without touching the shared defaults.
1 parent ec22981 commit 4ef2af2

2 files changed

Lines changed: 49 additions & 1 deletion

File tree

lib/recorder.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -217,7 +217,7 @@ export default {
217217
}
218218

219219
const retryRules = this.retries.slice().reverse()
220-
return promiseRetry(Object.assign(defaultRetryOptions, retryOpts), (retry, number) => {
220+
return promiseRetry(Object.assign({}, defaultRetryOptions, retryOpts), (retry, number) => {
221221
if (number > 1) output.log(`${currentQueue()}Retrying... Attempt #${number}`)
222222
const [promise, timer] = getTimeoutPromise(timeout, taskName)
223223
return Promise.race([promise, Promise.resolve(res).then(fn)])

test/unit/recorder_test.js

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,54 @@ describe('Recorder', () => {
9595
return recorder.promise()
9696
})
9797

98+
it('should not leak custom minTimeout to subsequent recorder runs', async function () {
99+
// Regression: Object.assign(defaultRetryOptions, retryOpts) mutated the
100+
// module-level defaultRetryOptions object. A custom minTimeout in one run
101+
// leaked into the defaults for every later run.
102+
this.timeout(5000)
103+
104+
let attempts = []
105+
recorder.retry({ retries: 1, minTimeout: 800, factor: 1, maxTimeout: 1000 })
106+
recorder.add(
107+
() => {
108+
attempts.push(Date.now())
109+
if (attempts.length < 2) throw new Error('first run')
110+
},
111+
undefined,
112+
undefined,
113+
true,
114+
)
115+
try {
116+
await recorder.promise()
117+
} catch (e) {
118+
await recorder.catchWithoutStop(err => err)
119+
}
120+
121+
expect(attempts[1] - attempts[0]).to.be.greaterThan(700, 'first retry should honor minTimeout=800')
122+
123+
// Fresh recorder, do not pass minTimeout — should fall back to default (150ms),
124+
// not 800 leaked from the previous run.
125+
recorder.start()
126+
attempts = []
127+
recorder.retry({ retries: 1, factor: 1, maxTimeout: 1000 })
128+
recorder.add(
129+
() => {
130+
attempts.push(Date.now())
131+
if (attempts.length < 2) throw new Error('second run')
132+
},
133+
undefined,
134+
undefined,
135+
true,
136+
)
137+
try {
138+
await recorder.promise()
139+
} catch (e) {
140+
await recorder.catchWithoutStop(err => err)
141+
}
142+
143+
expect(attempts[1] - attempts[0]).to.be.lessThan(500, 'second retry must use default minTimeout, not leaked 800ms')
144+
})
145+
98146
it('should prefer opts for non-when retry when possible', () => {
99147
let counter = 0
100148
const errorText = 'noerror'

0 commit comments

Comments
 (0)