From b446ad5bb4a4dbfe50633657b64f3fb7079371ff Mon Sep 17 00:00:00 2001 From: DavertMik Date: Sat, 13 Jun 2026 13:59:30 +0300 Subject: [PATCH] fix(pause): remove leaked dispatcher listeners and guard session restore MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- lib/pause.js | 50 ++++++++++++++++++--------- test/unit/pause_test.js | 76 ++++++++++++++++++++++++++++++++++++++++- 2 files changed, 109 insertions(+), 17 deletions(-) diff --git a/lib/pause.js b/lib/pause.js index 8960376ad..c3d1c591e 100644 --- a/lib/pause.js +++ b/lib/pause.js @@ -19,6 +19,35 @@ let finish let next let registeredVariables = {} let externalHandler = null +let pauseSessionOpen = false + +function onStepAfter() { + recorder.add('Start next pause session', () => { + // test already finished, nothing to pause + if (!store.currentTest) return + if (!next) return + return pauseSession() + }) +} + +function onTestFinished() { + if (typeof finish === 'function') finish() + if (pauseSessionOpen) { + recorder.session.restore('pause') + pauseSessionOpen = false + } + if (rl) rl.close() + if (!externalHandler) history.save() + event.dispatcher.removeListener(event.step.after, onStepAfter) + event.dispatcher.removeListener(event.test.finished, onTestFinished) +} + +function registerPauseListeners() { + event.dispatcher.removeListener(event.step.after, onStepAfter) + event.dispatcher.removeListener(event.test.finished, onTestFinished) + event.dispatcher.on(event.step.after, onStepAfter) + event.dispatcher.on(event.test.finished, onTestFinished) +} /** * Pauses test execution and starts interactive shell @@ -28,22 +57,7 @@ const pause = function (passedObject = {}) { if (store.dryRun) return next = false - // add listener to all next steps to provide next() functionality - event.dispatcher.on(event.step.after, () => { - recorder.add('Start next pause session', () => { - // test already finished, nothing to pause - if (!store.currentTest) return - if (!next) return - return pauseSession() - }) - }) - - event.dispatcher.on(event.test.finished, () => { - if (typeof finish === 'function') finish() - recorder.session.restore('pause') - if (rl) rl.close() - if (!externalHandler) history.save() - }) + registerPauseListeners() recorder.add('Start new session', () => pauseSession(passedObject)) } @@ -51,12 +65,14 @@ const pause = function (passedObject = {}) { function pauseSession(passedObject = {}) { registeredVariables = passedObject recorder.session.start('pause') + pauseSessionOpen = true if (externalHandler) { store.onPause = true return externalHandler({ registeredVariables }).then(() => { store.onPause = false recorder.session.restore('pause') + pauseSessionOpen = false }) } @@ -107,6 +123,7 @@ async function parseInput(cmd) { if (!cmd || cmd === 'resume' || cmd === 'exit') { if (typeof finish === 'function') finish() recorder.session.restore('pause') + pauseSessionOpen = false rl.close() history.save() return nextStep() @@ -265,6 +282,7 @@ function setPauseHandler(handler) { */ function pauseNow(passedObject = {}) { if (store.dryRun) return + registerPauseListeners() recorder.add('Triggered pause', () => pauseSession(passedObject)) } diff --git a/test/unit/pause_test.js b/test/unit/pause_test.js index bd65bafb2..50a0b44db 100644 --- a/test/unit/pause_test.js +++ b/test/unit/pause_test.js @@ -1,5 +1,18 @@ import { expect } from 'chai' -import { setPauseHandler } from '../../lib/pause.js' +import sinon from 'sinon' +import pause, { setPauseHandler, pauseNow } from '../../lib/pause.js' +import recorder from '../../lib/recorder.js' +import event from '../../lib/event.js' +import store from '../../lib/store.js' + +const settles = (promise, ms = 2000) => + Promise.race([ + promise, + new Promise((_, reject) => { + const t = setTimeout(() => reject(new Error(`did not settle within ${ms}ms`)), ms) + t.unref?.() + }), + ]) describe('pause external handler hook', () => { afterEach(() => { @@ -26,3 +39,64 @@ describe('pause external handler hook', () => { expect(received).to.deep.equal({ registeredVariables: { foo: 1 } }) }) }) + +describe('pause listener lifecycle', () => { + beforeEach(() => { + store.dryRun = false + setPauseHandler(null) + recorder.reset() + recorder.stop() + }) + + afterEach(() => { + setPauseHandler(null) + event.dispatcher.emit(event.test.finished) + recorder.reset() + }) + + it('keeps exactly one listener no matter how many pause() calls', () => { + const baseStep = event.dispatcher.listenerCount(event.step.after) + const baseFin = event.dispatcher.listenerCount(event.test.finished) + pause() + pause() + pause() + expect(event.dispatcher.listenerCount(event.step.after)).to.equal(baseStep + 1) + expect(event.dispatcher.listenerCount(event.test.finished)).to.equal(baseFin + 1) + }) + + it('removes both pause listeners when the test finishes', () => { + const baseStep = event.dispatcher.listenerCount(event.step.after) + const baseFin = event.dispatcher.listenerCount(event.test.finished) + pause() + expect(event.dispatcher.listenerCount(event.step.after)).to.equal(baseStep + 1) + event.dispatcher.emit(event.test.finished) + expect(event.dispatcher.listenerCount(event.step.after)).to.equal(baseStep) + expect(event.dispatcher.listenerCount(event.test.finished)).to.equal(baseFin) + }) + + it('does not restore the pause session when none is open on test.finished', async () => { + pause() + recorder.start() + const restoreSpy = sinon.spy(recorder.session, 'restore') + event.dispatcher.emit(event.test.finished) + event.dispatcher.emit(event.test.finished) + const pauseRestores = restoreSpy.getCalls().filter(c => c.args[0] === 'pause').length + restoreSpy.restore() + expect(pauseRestores, 'no pause restore when nothing is open').to.equal(0) + expect(recorder.getCurrentSessionId()).to.equal(null) + await settles(recorder.promise()) + }) + + it('pauseNow drives the external handler and restores the session', async () => { + let received = null + setPauseHandler(arg => { + received = arg + return Promise.resolve() + }) + recorder.start() + pauseNow({ foo: 1 }) + await settles(recorder.promise()) + expect(received).to.deep.equal({ registeredVariables: { foo: 1 } }) + expect(recorder.getCurrentSessionId()).to.equal(null) + }) +})