Repository navigation
fix(sessions)!: make session retirement permanent - #2348
gauravch-code wants to merge 2 commits into
Conversation
Persist retirement independently of error scores and cover session and pool restoration. BREAKING CHANGE: Retired sessions cannot become usable again after mark_good().
github-actions
Bot
added
the
t-tooling
|
|
||
| This page summarizes the breaking changes between Crawlee for Python v1 and v2. | ||
|
|
||
| ## Session retirement is permanent |
There was a problem hiding this comment.
Nit: The other v2 sections were added at the end of this page. Could you move this one after the last section to match?
| def test_session_without_retirement_field_can_be_restored() -> None: | ||
| """Saved sessions without a retirement field remain loadable.""" | ||
| state = Session().get_state(as_dict=True) | ||
| state.pop('retired', None) |
There was a problem hiding this comment.
Nit: get_state() always includes retired, so the default isn't needed. With del, the test fails if the field is renamed, so it can't silently stop covering old state.
| state.pop('retired', None) | |
| del state['retired'] |
vdusek
left a comment
vdusek
left a comment
There was a problem hiding this comment.
A three more comments, regarding comparision with Crawlee JS
| """Indicate whether the session is blocked based on the error score..""" | ||
| return self._error_score >= self._max_error_score | ||
| """Indicate whether the session is retired or blocked by its error score.""" | ||
| return self._retired or self._error_score >= self._max_error_score |
There was a problem hiding this comment.
Suggestion: JS v4 checks the flag in isUsable() and keeps isBlocked() score-only, so isBlocked() still separates a burned session from one that just expired or hit its usage limit. The self-retire in markGood()/markBad() only sets the flag there. Here mark_good()/mark_bad() self-retire through retire(), so an expired session also gets max_error_score added and reports is_blocked. Could we check _retired in is_usable and have the self-retire path set only the flag?
|
|
||
| ## Session retirement is permanent | ||
|
|
||
| `Session.retire()` permanently blocks the session. Calling `mark_good()` can still lower its error score, but cannot make it usable again. Create a new session to replace a retired one. Repeated calls to `retire()` leave the error score unchanged. |
There was a problem hiding this comment.
Suggestion: In JS, markGood() returns early on a retired session, so neither the usage count nor the error score changes. An if self._retired: return at the top of mark_good() would match that, and this sentence could go.
| self._usage_count = usage_count | ||
| self._max_usage_count = max_usage_count | ||
| self._error_score = error_score | ||
| self._retired = retired |
There was a problem hiding this comment.
Nit: JS exposes a public retired getter. A retired property here would let callers read the state without going through get_state().
|
Thanks for the careful review, @vdusek! I addressed the five comments in 261dab6:
All 49 session tests, full repository lint, and the migration Markdown lint pass. The full type check passes targeting Linux; native Windows reports three unrelated multiprocessing fork-type errors in tests/unit/_utils/test_system.py. Both commit hooks passed using an external Linux-targeted type configuration with the same repository rules. Happy to adjust anything further. Thanks again! CI note: the new factory-review approval lookup failed because GitHub returned HTTP 502, while the AI review job passed. GitHub requires repository admin rights to rerun the failed job, so I could not retry it from my fork. Could you rerun that lookup when you have a chance? |
| if not self._retired: | ||
| self._retired = True | ||
| self._error_score += self._max_error_score |
There was a problem hiding this comment.
Is there any point in doing this for retired session?
self._error_score += self._max_error_score
if not, I would suggest to do just
def retire(self)
"""..."""
self._retired = True
There was a problem hiding this comment.
Good point. The session pool only needs the retired flag through is_usable, so increasing the score is not required to keep a retired session out of the pool.
I kept the explicit retire() penalty to preserve its existing error_score/is_blocked behavior. Crawlee JS also still adds maxErrorScore in explicit retire(); automatic retirement now sets only the flag in this PR, as requested in the earlier review.
Making retire() flag-only would mean a manually retired session can report is_blocked=False, even though it remains permanently unusable. That is reasonable if we want to separate those concepts fully, but it is an additional observable behavior change. @vdusek, would you prefer that policy for v2? Happy to simplify retire() and update the tests and migration notes once the intended behavior is confirmed.
Description
Keep retired sessions from becoming usable again when
mark_good()lowers their error score. A session explicitly retired after a blocked request should not be handed out again by the pool, including after saving and restoring the pool.SessionModel.Falsewhen loading older saved state.retire()calls leave the error score unchanged.Issues
Closes #2312
Testing
retire()followed bymark_good()(error score 3.0 -> 2.5). Two regression tests failed before the fix.uv run pytest tests/unit/sessions tests/unit/crawlers/_basic -q --timeout=90: 193 passed, 1 existing skip on Windows / Python 3.12.13.--python-platform linux.The default Windows repository-wide type check reports three errors in the unchanged
tests/unit/_utils/test_system.pyinvolvingForkContext,ForkServerContext, andBaseContext.Process. Full unit and platform end-to-end suites were not run.Checklist