fix(core): restore sessions and propagate errors on session/within/retryTo error paths#5633
Merged
DavertMik merged 1 commit intoJun 13, 2026
Conversation
…tryTo 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): 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 <noreply@anthropic.com>
f921147 to
dca7626
Compare
974e699
into
advisor/001-promise-core-characterization
11 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Implements the actual fixes for the latent promise-core divergences that #5622 characterized. Each fix makes an error path symmetric with its already-working success path, and the corresponding characterization assertion is updated to the corrected behavior in the same commit.
advisor/001-promise-core-characterization); review only thefix(core): …commit. Merge #5622 first.Fixes (4 of the 7 findings)
within()async error skipped_withinEndand detached the errorfinishHelpers()(so_withinEndruns on error) andreturn recorder.promise()(so the error propagates throughwithin())session()async error leaked the session idrecorder.session.restoreviarecorder.addso the recorder session is restored on errorsession()sync queued-throw leaked the session idfinallyrecorder.catchnow callsrecorder.session.restorebefore re-throwing (the only place that runs on a rejected chain)retryTo()rejected prematurely /tries === 2on firstrecorder.throwinstead of an immediatereject(err), so the retry logic owns the outcome — throw-then-succeed now resolves; reject only aftermaxTries.triesstarts at 1; retry count preserved (tries < maxTries)Important: the session fix is deliberately surgical
The session error paths are entangled with the Playwright browser lifecycle. An earlier version of this PR routed the async-error cleanup through
finalize()— which also swaps the original (no-op)restoreVars(sessionName)call for the realrestoreVars(). That closed the browser context underBROWSER_RESTART=sessionand failed the Playwright CI job (4Cannot create page: browser context has been closedfailures insession_test.js).The fix is now minimal: only add the missing
recorder.session.restore(the session-id leak the finding is about); the existingrestoreVars/listener cleanup is left exactly as-is. Netlib/session.jschange is two added lines.Verification
npm run test:unit→ 748 passed, 0 failed, 11 skippednpm run test:runner→ 273 passed, 0 failed — incl.should rerun flaky tests,should rerun retried steps,should use retryFailedStep plugin for failed steps, etc.BROWSER_RESTART=browserandBROWSER_RESTART=session—within/session/elsgreen, 0browser context has been closedfailures. (The one remaining local acceptance failure isconfig_test.js's API call to the local mock at:3001/api/users, which 404s only in this sandbox and passes in CI — unrelated to these changes.)Deliberately not changed here
recorder.retriesnever cleared byreset()—lib/plugin/retryFailedStep.js(on by default) reads/mutatesrecorder.retries; the in-code comment warns clearing it breaks the plugin. Needs per-test scoping coordinated with that plugin.add()'s no-op-when-stopped is a contract theshould not add steps when stoppedtest depends on; these error-path fixes reduce the wedging, but a blanket change is unsafe.within/sessionkeep start+restore at one level. The characterization test pins the correct synchronous id semantics.Happy to tackle any of these three as a focused follow-up.
🤖 Generated with Claude Code