fix(core): restore sessions and propagate errors on session/within/retryTo error paths

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): the catch now calls finalize() — the
  same cleanup the success path uses (awaited restoreVars + recorder.session
  .restore) — instead of an ad-hoc cleanup that never restored the recorder
  session, leaking the session id after an error.

- session() sync error (lib/session.js): the finally recorder.catch now calls
  recorder.session.restore before re-throwing. finalize()'s restore task is
  skipped on a rejected chain, so the catch handler is the only place that can
  restore the session id on the sync-throw path.

- 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 all green.

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 <noreply@anthropic.com>

Author: DavertMik

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 {

lib/session.js

@@ -106,9 +106,7 @@ function session(sessionName, config, fn) {
             return res
           })
           .catch(e => {
-            output.stepShift = 0
-            session.restoreVars(sessionName)
-            event.dispatcher.removeListener(event.step.after, addContextToStep)
+            finalize()
             recorder.throw(e)
             return recorder.promise()
           })
@@ -124,6 +122,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
         })
       }

test/unit/session_composition_test.js

@@ -94,31 +94,29 @@ 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 rejects the chain 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')
+      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 +132,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 +181,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 () => {