Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2239 +/- ##
==========================================
- Coverage 93.75% 93.73% -0.03%
==========================================
Files 181 181
Lines 12877 12897 +20
==========================================
+ Hits 12073 12089 +16
- Misses 804 808 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
Browser temp directory
PlaywrightPersistentBrowser.close()removed its temporary profile directory with a singleshutil.rmtree(ignore_errors=True)call, 100 ms after closing the context. On Windows the browser process can still hold files inside that directory open at that point, so the removal failed,ignore_errors=Trueswallowed the error, and the directory stayed behind. Persistent context is the default forPlaywrightCrawler, so a browser close on Windows could leak a profile directory without reporting anything.The removal now retries until the directory is gone, up to 50 attempts 100 ms apart, and logs a warning if it never succeeds. The fixed sleep in
close()is gone, so the usual case finishes as soon as the first attempt succeeds.Both
close()and the context'scloselistener ask for the removal. They take a lock, and whichever one acquires it first clears_temp_dirwhile holding it, so a single close spends the retry budget once.close()still blocks until the removal finishes, whichever coroutine ran it.This surfaced as an intermittent failure of
tests/unit/browsers/test_playwright_browser.py::test_delete_temp_folder_with_close_browseronwindows-latest(run 35085779018), where every other job of the same commit passed.Crawler state after a failed run
These follow up on #2229, released in 1.10.1.
BasicCrawler.run()reset_runningonly around theawait run_taskphase, so an exception raised during setup (add_requestsfailing, for example) left_runningset, and every laterrun()raisedRuntimeError('This crawler instance is already running...'). The whole run body is now inside thetry, with the reset in itsfinally.#2229 also documented that "a run that ended with an exception does not count as a previous run, so a retry keeps the requests that were still pending". That holds only while no earlier run has succeeded: once
_has_finished_beforeis set, the next run purges the queue and drops whatever the failed run left pending. A_last_run_failedflag now gates the implicit purge, which makes the documented behavior true in both cases.The post-crawl steps, statistics logging and
_save_crawler_state(), sit inside the sametry, so a failure there marks the run as failed too. A_save_crawler_state()error otherwise left_has_finished_beforeset and_last_run_failedclear, and the next run purged the queue and dropped its pending requests.The purge exemption also wins over an explicit
purge_request_queue=True, which the docstring now states.AutoscaledPool
run.result.exception()is guarded against a cancelled future.BasicCrawlercancels the run task on SIGINT, which cancelsrun.result; the unguarded call then raisedCancelledErrorinside the orchestrator'sfinallyand skipped the rest of it, including the error log below. The worker drain it also skipped is covered byrun()'s ownfinally, so the visible effect is the lost traceback.The orchestrator's fallback error message is now distinct from the identical one
run()already emits, so the two paths can be told apart in logs.Breaking change
BREAKING CHANGE: the
AutoscaledPoolorchestrator's fallback error log message changed fromException in worker task orchestratortoUnpropagated exception in worker task orchestrator. The old wording shipped in 1.10.1, so log-based tooling that matches on it needs updating. The identical message emitted byrun()keeps the old wording.✍️ Drafted by Claude Code