Skip to content

Commit b446ad5

Browse files
DavertMikclaude
andcommitted
fix(pause): remove leaked dispatcher listeners and guard session restore
Every pause() call registered two permanent listeners on the global event dispatcher (step.after, test.finished) and never removed them. Repeated pauses — now the normal case because the MCP server drives pause() programmatically via setPauseHandler/pauseNow — accumulated listeners, fired finish() multiple times, and ran an unconditional recorder.session.restore('pause') on every test finish even when no pause session was open, unbalancing the recorder's session stack (the hang class blocking 4.0). - Convert the two anonymous listeners into named handlers (onStepAfter, onTestFinished) and register them through an idempotent helper that removes any prior registration first, so repeated pause()/pauseNow() keep exactly one of each. onTestFinished removes both listeners when the test finishes. - Track an open-pause flag and only restore the 'pause' session when one is actually open (set on session.start in pauseSession, cleared at all three restore sites). - pauseNow now performs the same idempotent registration as pause(). setPauseHandler/pauseNow signatures and resolve semantics are unchanged (bin/mcp-server.js untouched). 4 regression tests cover idempotent registration, listener removal on finish, the no-double-restore guard, and the MCP pauseNow lifecycle; reverting the fix fails the listener tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent a70c814 commit b446ad5

2 files changed

Lines changed: 109 additions & 17 deletions

File tree

lib/pause.js

Lines changed: 34 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,35 @@ let finish
1919
let next
2020
let registeredVariables = {}
2121
let externalHandler = null
22+
let pauseSessionOpen = false
23+
24+
function onStepAfter() {
25+
recorder.add('Start next pause session', () => {
26+
// test already finished, nothing to pause
27+
if (!store.currentTest) return
28+
if (!next) return
29+
return pauseSession()
30+
})
31+
}
32+
33+
function onTestFinished() {
34+
if (typeof finish === 'function') finish()
35+
if (pauseSessionOpen) {
36+
recorder.session.restore('pause')
37+
pauseSessionOpen = false
38+
}
39+
if (rl) rl.close()
40+
if (!externalHandler) history.save()
41+
event.dispatcher.removeListener(event.step.after, onStepAfter)
42+
event.dispatcher.removeListener(event.test.finished, onTestFinished)
43+
}
44+
45+
function registerPauseListeners() {
46+
event.dispatcher.removeListener(event.step.after, onStepAfter)
47+
event.dispatcher.removeListener(event.test.finished, onTestFinished)
48+
event.dispatcher.on(event.step.after, onStepAfter)
49+
event.dispatcher.on(event.test.finished, onTestFinished)
50+
}
2251

2352
/**
2453
* Pauses test execution and starts interactive shell
@@ -28,35 +57,22 @@ const pause = function (passedObject = {}) {
2857
if (store.dryRun) return
2958

3059
next = false
31-
// add listener to all next steps to provide next() functionality
32-
event.dispatcher.on(event.step.after, () => {
33-
recorder.add('Start next pause session', () => {
34-
// test already finished, nothing to pause
35-
if (!store.currentTest) return
36-
if (!next) return
37-
return pauseSession()
38-
})
39-
})
40-
41-
event.dispatcher.on(event.test.finished, () => {
42-
if (typeof finish === 'function') finish()
43-
recorder.session.restore('pause')
44-
if (rl) rl.close()
45-
if (!externalHandler) history.save()
46-
})
60+
registerPauseListeners()
4761

4862
recorder.add('Start new session', () => pauseSession(passedObject))
4963
}
5064

5165
function pauseSession(passedObject = {}) {
5266
registeredVariables = passedObject
5367
recorder.session.start('pause')
68+
pauseSessionOpen = true
5469

5570
if (externalHandler) {
5671
store.onPause = true
5772
return externalHandler({ registeredVariables }).then(() => {
5873
store.onPause = false
5974
recorder.session.restore('pause')
75+
pauseSessionOpen = false
6076
})
6177
}
6278

@@ -107,6 +123,7 @@ async function parseInput(cmd) {
107123
if (!cmd || cmd === 'resume' || cmd === 'exit') {
108124
if (typeof finish === 'function') finish()
109125
recorder.session.restore('pause')
126+
pauseSessionOpen = false
110127
rl.close()
111128
history.save()
112129
return nextStep()
@@ -265,6 +282,7 @@ function setPauseHandler(handler) {
265282
*/
266283
function pauseNow(passedObject = {}) {
267284
if (store.dryRun) return
285+
registerPauseListeners()
268286
recorder.add('Triggered pause', () => pauseSession(passedObject))
269287
}
270288

test/unit/pause_test.js

Lines changed: 75 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,18 @@
11
import { expect } from 'chai'
2-
import { setPauseHandler } from '../../lib/pause.js'
2+
import sinon from 'sinon'
3+
import pause, { setPauseHandler, pauseNow } from '../../lib/pause.js'
4+
import recorder from '../../lib/recorder.js'
5+
import event from '../../lib/event.js'
6+
import store from '../../lib/store.js'
7+
8+
const settles = (promise, ms = 2000) =>
9+
Promise.race([
10+
promise,
11+
new Promise((_, reject) => {
12+
const t = setTimeout(() => reject(new Error(`did not settle within ${ms}ms`)), ms)
13+
t.unref?.()
14+
}),
15+
])
316

417
describe('pause external handler hook', () => {
518
afterEach(() => {
@@ -26,3 +39,64 @@ describe('pause external handler hook', () => {
2639
expect(received).to.deep.equal({ registeredVariables: { foo: 1 } })
2740
})
2841
})
42+
43+
describe('pause listener lifecycle', () => {
44+
beforeEach(() => {
45+
store.dryRun = false
46+
setPauseHandler(null)
47+
recorder.reset()
48+
recorder.stop()
49+
})
50+
51+
afterEach(() => {
52+
setPauseHandler(null)
53+
event.dispatcher.emit(event.test.finished)
54+
recorder.reset()
55+
})
56+
57+
it('keeps exactly one listener no matter how many pause() calls', () => {
58+
const baseStep = event.dispatcher.listenerCount(event.step.after)
59+
const baseFin = event.dispatcher.listenerCount(event.test.finished)
60+
pause()
61+
pause()
62+
pause()
63+
expect(event.dispatcher.listenerCount(event.step.after)).to.equal(baseStep + 1)
64+
expect(event.dispatcher.listenerCount(event.test.finished)).to.equal(baseFin + 1)
65+
})
66+
67+
it('removes both pause listeners when the test finishes', () => {
68+
const baseStep = event.dispatcher.listenerCount(event.step.after)
69+
const baseFin = event.dispatcher.listenerCount(event.test.finished)
70+
pause()
71+
expect(event.dispatcher.listenerCount(event.step.after)).to.equal(baseStep + 1)
72+
event.dispatcher.emit(event.test.finished)
73+
expect(event.dispatcher.listenerCount(event.step.after)).to.equal(baseStep)
74+
expect(event.dispatcher.listenerCount(event.test.finished)).to.equal(baseFin)
75+
})
76+
77+
it('does not restore the pause session when none is open on test.finished', async () => {
78+
pause()
79+
recorder.start()
80+
const restoreSpy = sinon.spy(recorder.session, 'restore')
81+
event.dispatcher.emit(event.test.finished)
82+
event.dispatcher.emit(event.test.finished)
83+
const pauseRestores = restoreSpy.getCalls().filter(c => c.args[0] === 'pause').length
84+
restoreSpy.restore()
85+
expect(pauseRestores, 'no pause restore when nothing is open').to.equal(0)
86+
expect(recorder.getCurrentSessionId()).to.equal(null)
87+
await settles(recorder.promise())
88+
})
89+
90+
it('pauseNow drives the external handler and restores the session', async () => {
91+
let received = null
92+
setPauseHandler(arg => {
93+
received = arg
94+
return Promise.resolve()
95+
})
96+
recorder.start()
97+
pauseNow({ foo: 1 })
98+
await settles(recorder.promise())
99+
expect(received).to.deep.equal({ registeredVariables: { foo: 1 } })
100+
expect(recorder.getCurrentSessionId()).to.equal(null)
101+
})
102+
})

0 commit comments

Comments
 (0)