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
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