From dca7626b7da86f47e9ee4517a0a51e0e99234eb0 Mon Sep 17 00:00:00 2001 From: DavertMik Date: Sat, 13 Jun 2026 17:15:06 +0300 Subject: [PATCH] fix(core): restore sessions and propagate errors on session/within/retryTo error paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes four of the latent error-path divergences characterized in #5622, by making the error paths symmetric with the success paths. The characterization assertions are updated to the corrected behavior in the same commit. - within() async error (lib/effects.js): the catch now calls finishHelpers() (so helpers' _withinEnd runs on error, not just success) and returns recorder.promise() (so the error propagates through within() instead of being detached onto a trailing task that a caller awaiting within() never sees). - session() async error (lib/session.js): schedules a recorder task that runs recorder.session.restore so the recorder session is restored on error (it was only restored on success, leaking the session id). The existing restoreVars/listener cleanup is kept as-is — switching to finalize()'s real restoreVars() closes the browser context under BROWSER_RESTART=session. - session() sync error (lib/session.js): the finally recorder.catch now calls recorder.session.restore before re-throwing (the only place that runs on a rejected chain). - retryTo() (lib/effects.js): a thrown callback no longer reject()s the outer promise prematurely — it routes through recorder.throw so the retry logic owns the outcome (retry, or reject once maxTries is exhausted). A callback that throws then succeeds on a later attempt now resolves instead of rejecting. tries now starts at 1 on the first attempt (was 2); the retry count is preserved (tries < maxTries). Verified: unit 748/0, runner 273/0 (incl. all retryFailedStep/rerun tests), acceptance within/session/els green under both BROWSER_RESTART=browser and =session. Not changed here (would be unsafe or out of scope, see PR): recorder.retries clearing (retryFailedStep depends on it), the stopped-recorder no-op contract, and nested cross-level restore ordering. Co-Authored-By: Claude Opus 4.8 --- lib/effects.js | 8 ++-- lib/session.js | 2 + test/unit/session_composition_test.js | 63 ++++++++++++++++----------- 3 files changed, 44 insertions(+), 29 deletions(-) diff --git a/lib/effects.js b/lib/effects.js index 94db32069..82b1da169 100644 --- a/lib/effects.js +++ b/lib/effects.js @@ -50,8 +50,10 @@ function within(context, fn) { return recorder.promise().then(() => res) }) .catch(e => { + finishHelpers() finalize() recorder.throw(e) + return recorder.promise() }) } @@ -203,7 +205,7 @@ async function retryTo(callback, maxTries, pollInterval = 200) { const sessionName = 'retryTo' return new Promise((done, reject) => { - let tries = 1 + let tries = 0 function handleRetryException(err) { recorder.throw(err) @@ -216,7 +218,7 @@ async function retryTo(callback, maxTries, pollInterval = 200) { try { await callback(tries) } catch (err) { - handleRetryException(err) + recorder.throw(err) } // Call done if no errors @@ -228,7 +230,7 @@ async function retryTo(callback, maxTries, pollInterval = 200) { // Catch errors and retry recorder.session.catch(err => { recorder.session.restore(`${sessionName} ${tries}`) - if (tries <= maxTries) { + if (tries < maxTries) { output.debug(`Error ${err}... Retrying`) recorder.add(`${sessionName} ${tries}`, () => setTimeout(tryBlock, pollInterval)) } else { diff --git a/lib/session.js b/lib/session.js index 367c1adf8..fc4c7b4e1 100644 --- a/lib/session.js +++ b/lib/session.js @@ -109,6 +109,7 @@ function session(sessionName, config, fn) { output.stepShift = 0 session.restoreVars(sessionName) event.dispatcher.removeListener(event.step.after, addContextToStep) + recorder.add('restore session on error', () => recorder.session.restore(`session:${sessionName}`)) recorder.throw(e) return recorder.promise() }) @@ -124,6 +125,7 @@ function session(sessionName, config, fn) { session.restoreVars(sessionName) output.stepShift = 0 event.dispatcher.removeListener(event.step.after, addContextToStep) + recorder.session.restore(`session:${sessionName}`) throw e }) } diff --git a/test/unit/session_composition_test.js b/test/unit/session_composition_test.js index 7f67522c8..9f7921991 100644 --- a/test/unit/session_composition_test.js +++ b/test/unit/session_composition_test.js @@ -94,31 +94,28 @@ describe('promise-core composition (characterization)', () => { expect(recorder.getCurrentSessionId()).to.equal(null) }) - it('FINDING: async callback error leaks the session id and never calls recorder.session.restore', async () => { + it('async callback error surfaces the error and restores the session id', async () => { session('asyncerr', async () => { throw new Error('boom') }) - let rejected - await settles(recorder.promise()).catch(e => (rejected = e)) - // characterized behavior, see plans/001-findings.md - expect(rejected, 'outer chain rejects').to.be.instanceof(Error) - expect(rejected.message).to.equal('boom') - expect(recorder.getCurrentSessionId(), 'session id leaks').to.equal('session:asyncerr') + const err = await drain() + expect(err, 'outer chain rejects').to.be.instanceof(Error) + expect(err.message).to.equal('boom') + expect(helper.calls.restoreVars, 'restoreVars called on error').to.be.greaterThan(0) + expect(recorder.getCurrentSessionId(), 'session id restored on error').to.equal(null) }) - it('FINDING: sync callback whose queued task throws restores vars but leaks the session id', async () => { + it('sync callback whose queued task throws surfaces the error and restores the session id', async () => { session('syncerr', () => { recorder.add(() => { throw new Error('boomsync') }) }) const err = await drain() - // The finally recorder.catch re-throws before finalize() runs - // recorder.session.restore, so the session id leaks. See plans/001-findings.md expect(err, 'error surfaces after draining').to.be.instanceof(Error) expect(err.message).to.equal('boomsync') - expect(helper.calls.restoreVars, 'restoreVars called by the finally handler').to.equal(1) - expect(recorder.getCurrentSessionId(), 'session id leaks').to.equal('session:syncerr') + expect(helper.calls.restoreVars, 'restoreVars called by the finally handler').to.be.greaterThan(0) + expect(recorder.getCurrentSessionId(), 'session id restored on error').to.equal(null) }) }) @@ -134,22 +131,40 @@ describe('promise-core composition (characterization)', () => { expect(inside).to.equal(true) }) - it('FINDING: async callback error skips _withinEnd and detaches the error onto a trailing task', async () => { + it('async callback error runs _withinEnd and propagates the error', async () => { within('ctx', async () => { throw new Error('boomwithin') }) const err = await drain() - // The error is only visible after draining trailing tasks because within()'s - // async catch omits `return recorder.promise()`. See plans/001-findings.md - expect(err, 'error surfaces after draining').to.be.instanceof(Error) + expect(err, 'error surfaces').to.be.instanceof(Error) expect(err.message).to.equal('boomwithin') expect(helper.calls.withinBegin, '_withinBegin ran').to.be.greaterThan(0) - expect(helper.calls.withinEnd, '_withinEnd skipped on error').to.equal(0) + expect(helper.calls.withinEnd, '_withinEnd runs on error').to.be.greaterThan(0) }) }) describe('retryTo()', () => { - it('FINDING: a synchronously-throwing callback rejects but keeps retrying past the rejection', async () => { + it('retries a throwing callback and resolves when a later attempt succeeds', async () => { + let firstTries + let calls = 0 + let rejected = null + await settles( + retryTo( + tries => { + if (firstTries === undefined) firstTries = tries + calls++ + if (tries < 3) throw new Error('not yet') + }, + 5, + 20, + ), + ).catch(e => (rejected = e)) + expect(rejected, 'resolves once an attempt succeeds (no premature reject)').to.equal(null) + expect(firstTries, 'first attempt receives tries === 1').to.equal(1) + expect(calls, 'ran until the succeeding attempt').to.equal(3) + }) + + it('a callback that always throws rejects only after exhausting maxTries', async () => { let firstTries let calls = 0 let rejected @@ -165,15 +180,11 @@ describe('promise-core composition (characterization)', () => { ), 3000, ).catch(e => (rejected = e)) - await new Promise(r => setTimeout(r, 300)) - const finalCalls = calls - await new Promise(r => setTimeout(r, 300)) - // characterized behavior, see plans/001-findings.md - expect(rejected, 'retryTo rejects with the real error').to.be.instanceof(Error) + await new Promise(r => setTimeout(r, 200)) + expect(rejected, 'rejects with the real error').to.be.instanceof(Error) expect(rejected.message).to.equal('always') - expect(firstTries, 'first attempt receives tries === 2 (tries starts at 1, incremented before callback)').to.equal(2) - expect(calls, 'retrying continues past the first rejection').to.be.greaterThan(1) - expect(calls, 'retries stop once drained (no perpetual loop)').to.equal(finalCalls) + expect(firstTries, 'first attempt receives tries === 1').to.equal(1) + expect(calls, 'retried exactly maxTries times').to.equal(3) }) it('retries via recorder failures then resolves', async () => {