diff --git a/src/wordstat/collector.py b/src/wordstat/collector.py index d34bfe6..b16e9e4 100644 --- a/src/wordstat/collector.py +++ b/src/wordstat/collector.py @@ -74,46 +74,59 @@ def _is_untrustworthy_empty_export(view: WordstatView, dataset: CsvDataset) -> bool: """True if an empty CSV for this view can never be a legitimate export. - TOP_POPULAR/TOP_RELATED/DYNAMICS are checked — every view for which - _select_view hard-gates TABLE_ROW_SELECTOR.length > 0 on the DOM before - the "Скачать" click is ever issued (see the docstring on _select_view - and CLAUDE.md's issue #3/#13 section). REGIONS is the only view exempt - from that gate (it is a map with no table rows in its DOM), so it is the - only view exempt here too — this set must stay in lockstep with - _select_view's `if view != WordstatView.REGIONS:` condition, not be - picked per-view by hand. + Only DYNAMICS is checked. This used to also cover TOP_POPULAR/ + TOP_RELATED on the theory that _select_view's pre-download hard gate + (TABLE_ROW_SELECTOR.length > 0, see that method's docstring and + CLAUDE.md's issue #3/#13 section) makes an empty CSV a structural + contradiction for every table-based view alike — "the DOM proved + rows>0 moments earlier, so the export cannot legitimately be empty". - A phrase that clears the pre-download gate has already had the code - itself prove TABLE_ROW_SELECTOR.length > 0 moments earlier; genuinely - zero rows never reaches the download step at all — it dies inside - _select_view's retry loop with InterfaceChangedError instead. So by the - time a dataset for one of these views is parsed here, an empty - dataset.rows already contradicts a state the code itself proved moments - earlier; there is no code path left where that emptiness is legitimate, - for any of the three table-based views alike. + Issue #22 found that premise false, with a live counterexample: issue + #11 recorded TOP_POPULAR/TOP_RELATED returning an empty CSV from a + manual click on the export link, entirely outside this code path — so + the DOM showing rows before the click does *not* prove the exported + file cannot be empty. Worse, TOP_POPULAR is the first view in + VIEW_SELECTORS and is *always* empty on live Wordstat regardless of + phrase — not a rare race but a permanent property of that report. Once + this predicate covered it, every full collect() run failed closed on + the very first view and never reached DYNAMICS/REGIONS at all: the + fix for issue #11 (never silently swallow an anomalous empty export) + had turned into "the tool no longer completes a single run". - This used to be conditioned on a second, post-download DOM read - (`rendered_rows > 0`) — but re-querying the DOM after the download's + So "the DOM proved rows>0 structurally" was never the right criterion. + The criterion that actually distinguishes these views, per issue #11's + live measurements and PR #20's, is empirical: does Wordstat reliably + return a non-empty file for this view at all? For TOP_POPULAR/ + TOP_RELATED, no — never, confirmed live, so an empty export from them + is the expected, honest case, not an anomaly to fail closed on; the + caller (._collect_one) still records it as a normal ExportSummary with + row_count: 0, which is what makes the emptiness visible in + manifest.json instead of silently absent (issue #16's concern, at + least for these two views — see CLAUDE.md and this issue for the + `status`/`missing_views` computed fields that make row_count: 0 alone + not sufficient for the third, DYNAMICS). For DYNAMICS, yes — issue + #11 measured 24 rows across three separate live runs, never empty, and + PR #20 corroborates it; an empty DYNAMICS export is genuinely + anomalous, so it stays behind the fail-closed gate below (with + _collect_one's existing empty_export_retry_seconds retry ahead of it). + + REGIONS was never part of this set: it is the one view _select_view's + hard gate itself exempts (a map has no TABLE_ROW_SELECTOR rows in its + DOM at all), so there is no structural premise to begin with, and no + live evidence of an empty regions.parquet either. + + This used to also be conditioned on a second, post-download DOM read + (`rendered_rows > 0`) — re-querying the DOM after the download's unbounded polling window can observe a table that has since emptied (rerender, auth transition, page degradation), which let the empty export through as an apparently valid `row_count: 0` and silently - corrupt the manifest into `status: "complete"` — the exact failure mode - issue #11's fix was meant to close. The pre-download gate is already - proof enough; no second opinion from a later DOM read is needed or - trustworthy. See tests/test_collector_view.py. - - DYNAMICS was previously excluded on the strength of issue #11's live-CDP - data (24 rows across three runs, never empty) — but that is evidence the - gate rarely fires for DYNAMICS, not evidence that omitting it is safe. - The gate's premise is structural (the DOM proved rows>0 immediately - before the click), and that premise holds for DYNAMICS exactly as it - does for TOP_POPULAR/TOP_RELATED; selecting views by observed frequency - of emptiness rather than by the structural gate they share was the bug. + corrupt the manifest into `status: "complete"` — the exact failure + mode issue #11's fix was meant to close for DYNAMICS. That reasoning + is unaffected by this change and still applies to DYNAMICS: no second + opinion from a later DOM read is needed or trustworthy here. See + tests/test_collector_view.py. """ - return ( - view in (WordstatView.TOP_POPULAR, WordstatView.TOP_RELATED, WordstatView.DYNAMICS) - and not dataset.rows - ) + return view is WordstatView.DYNAMICS and not dataset.rows def _without_traceback(error: Exception) -> Exception: @@ -163,11 +176,7 @@ def __init__( # chosen without a live timing measurement, not derived from one. # Cost: +1s per non-map view, +3s per phrase (3 of 4 views are # non-map), so +50s across a 50-phrase batch. Do not remove without - # a way to verify nothing regresses — issue #22 currently blocks a - # full CLI run from reaching DYNAMICS at all (see collector.py's - # fail-closed behavior on empty top exports), so there is no cheap - # end-to-end signal today that would catch a regression from - # removing this. + # a way to verify nothing regresses against a live run. # # empty_export_retry_seconds: unlike the above, this one does have # a concrete, structural trigger — it only fires after @@ -257,6 +266,11 @@ async def collect_many( allowed_domains=["wordstat.yandex.ru", "passport.yandex.ru"], keep_alive=True, ) + # Declared before the try so the finally below can check "was a + # tab actually created" even if session.start() itself raised + # (in which case new_page() was never reached and there is + # nothing of ours to close). + page = None try: await session.start() page = await session.new_page() @@ -320,6 +334,22 @@ def _mark_region_ready() -> None: # SystemExit) deliberately still propagates. failures.append(PhraseFailure(phrase=phrase, error=_without_traceback(error))) finally: + # Close only the tab this batch created (issue #9): new_page() + # above opens exactly one tab for the whole batch (not one per + # phrase), keep_alive=True correctly leaves the user's own + # Chrome and its other tabs untouched, but that same flag also + # means session.stop() below never closes *our* tab either — + # left unclosed, every CLI invocation orphans one more tab. + # Must run before session.stop(): the CDP handle backing + # `page` may no longer be usable once the session itself has + # stopped. Wrapped the same way as session.stop() below — + # a failed close() (tab already gone, CDP hiccup) must not + # discard the results/failures already collected. + if page is not None: + try: + await session.close_page(page) + except Exception: # noqa: BLE001 + pass try: await session.stop() except Exception: # noqa: BLE001 diff --git a/src/wordstat/models.py b/src/wordstat/models.py index e73f6a7..85ea4c6 100644 --- a/src/wordstat/models.py +++ b/src/wordstat/models.py @@ -55,18 +55,32 @@ class CollectionManifest(BaseModel): a manifest on disk that honestly describes what it has so far rather than none at all. - ``exports`` is the single source of truth for completeness: both - ``missing_views`` (every :class:`WordstatView` not yet present in - ``exports``, in enum declaration order) and ``status`` are *derived* from - it via ``computed_field`` rather than stored fields, so there is no way - to construct a manifest where they disagree with ``exports`` — the bug - this feature exists to avoid (a caller building - ``CollectionManifest(exports=[])`` without separately remembering to set - ``missing_views`` would otherwise silently get a manifest that lies about - being complete). Both are still plain JSON fields in ``manifest.json`` on - disk (pydantic includes computed fields in ``model_dump_json`` by - default), which is what makes "incomplete" visible to a reader of the - file itself, not only on the in-memory object. + ``exports`` is the single source of truth for completeness: ``missing_views`` + (every :class:`WordstatView` not yet present in ``exports``, in enum + declaration order), ``empty_views`` (views present in ``exports`` whose + ``row_count`` is zero) and ``status`` are all *derived* from it via + ``computed_field`` rather than stored fields, so there is no way to + construct a manifest where they disagree with ``exports`` — the bug this + feature exists to avoid (a caller building ``CollectionManifest(exports=[])`` + without separately remembering to set ``missing_views`` would otherwise + silently get a manifest that lies about being complete). All three are + still plain JSON fields in ``manifest.json`` on disk (pydantic includes + computed fields in ``model_dump_json`` by default), which is what makes + "incomplete" visible to a reader of the file itself, not only on the + in-memory object. + + ``empty_views`` exists because of issue #22/#16: ``TOP_POPULAR``/ + ``TOP_RELATED`` are collected and recorded in ``exports`` even when + Wordstat returns them empty (a permanent property of those two reports on + live Wordstat, not a collection failure — see + ``collector._is_untrustworthy_empty_export``), so they are not + "missing" — the run did produce a file for them, with the header schema + preserved (issue #18). But a manifest with zero missing views and two + zero-row exports must still not read as an unqualified success, or issue + #16's original concern (``status: "complete"`` at zero rows) resurfaces + for exactly the two views this run legitimately can't fill. ``status`` + is therefore ``"incomplete"`` if either ``missing_views`` or + ``empty_views`` is non-empty, not only the former. Three timestamp/URL fields describe when and where the data came from, and their semantics differ deliberately once ``--resume-dir`` is in @@ -115,10 +129,15 @@ def missing_views(self) -> list[WordstatView]: present = {export.view for export in self.exports} return [view for view in WordstatView if view not in present] + @computed_field # type: ignore[prop-decorator] + @property + def empty_views(self) -> list[WordstatView]: + return [export.view for export in self.exports if export.row_count == 0] + @computed_field # type: ignore[prop-decorator] @property def status(self) -> str: - return "incomplete" if self.missing_views else "complete" + return "incomplete" if self.missing_views or self.empty_views else "complete" @model_validator(mode="after") def _exports_have_unique_views(self) -> "CollectionManifest": diff --git a/src/wordstat/storage.py b/src/wordstat/storage.py index 4aa0092..acf16b0 100644 --- a/src/wordstat/storage.py +++ b/src/wordstat/storage.py @@ -164,11 +164,11 @@ def merge_export( Keeps ``exports`` ordered by :class:`WordstatView` declaration order (not append order) so a resumed run's manifest looks the same as one - collected in a single pass. ``missing_views``/``status`` are computed - fields derived straight from ``exports`` (see + collected in a single pass. ``missing_views``/``empty_views``/``status`` + are computed fields derived straight from ``exports`` (see :class:`~wordstat.models.CollectionManifest`), so updating only ``exports`` here is enough to keep them correct — there is nothing else - to recompute for those two. + to recompute for those three. ``updated_at`` is bumped to ``now`` (defaulting to the current UTC time, like :func:`create_run_directory` — a caller can pass a fixed value for diff --git a/tests/test_collector_view.py b/tests/test_collector_view.py index e5dd4a4..48d0aa3 100644 --- a/tests/test_collector_view.py +++ b/tests/test_collector_view.py @@ -155,25 +155,37 @@ def _dataset(view: WordstatView, rows: list[dict[str, str]]) -> CsvDataset: return CsvDataset(view=view, headers=["query", "count"], rows=rows) -@pytest.mark.parametrize( - "view", [WordstatView.TOP_POPULAR, WordstatView.TOP_RELATED, WordstatView.DYNAMICS] -) -def test_empty_export_is_untrustworthy_for_table_views_regardless_of_dom_state(view): - # Regression guard for issue #11 (and its cycle-2 follow-up): _select_view - # already hard-gates TABLE_ROW_SELECTOR.length > 0 on the DOM before - # "Скачать" is ever clicked for every view but REGIONS, so an empty CSV - # reaching this point is already a contradiction for all three of these - # table-based views — it must be rejected unconditionally, with no - # second, later DOM read able to wave it through as "legitimately empty" - # (that re-read can observe a table that has since emptied and silently - # accept a corrupted export — the exact bug this predicate replaces). - assert _is_untrustworthy_empty_export(view, _dataset(view, [])) is True - - -@pytest.mark.parametrize( - "view", [WordstatView.TOP_POPULAR, WordstatView.TOP_RELATED, WordstatView.DYNAMICS] -) -def test_non_empty_export_is_trusted_for_table_views(view): +def test_empty_dynamics_export_is_untrustworthy(): + # Regression guard for issue #11 (and its cycle-2 follow-up): live CDP + # measurements found DYNAMICS reliably non-empty (24 rows across three + # separate runs, corroborated by PR #20), so an empty DYNAMICS export + # reaching this point is anomalous and must be rejected, with no second, + # later DOM read able to wave it through as "legitimately empty" (that + # re-read can observe a table that has since emptied and silently accept + # a corrupted export — the exact bug this predicate replaces). + assert _is_untrustworthy_empty_export(WordstatView.DYNAMICS, _dataset(WordstatView.DYNAMICS, [])) is True + + +def test_non_empty_dynamics_export_is_trusted(): + dataset = _dataset(WordstatView.DYNAMICS, [{"query": "a", "count": "1"}]) + assert _is_untrustworthy_empty_export(WordstatView.DYNAMICS, dataset) is False + + +@pytest.mark.parametrize("view", [WordstatView.TOP_POPULAR, WordstatView.TOP_RELATED]) +def test_empty_top_export_is_not_flagged_by_this_gate(view): + # Issue #22: TOP_POPULAR/TOP_RELATED are empty on live Wordstat every + # time, regardless of phrase — a permanent property of those reports, + # not an anomaly (issue #11 recorded an empty CSV for them from a manual + # click on the export link, entirely outside this code path, so the DOM + # showing rows before the download click does not prove the exported + # file cannot be empty). TOP_POPULAR is also the first view in + # VIEW_SELECTORS, so flagging it here previously fail-closed every full + # collect() run before DYNAMICS/REGIONS were ever reached. + assert _is_untrustworthy_empty_export(view, _dataset(view, [])) is False + + +@pytest.mark.parametrize("view", [WordstatView.TOP_POPULAR, WordstatView.TOP_RELATED]) +def test_non_empty_top_export_is_also_trusted(view): assert _is_untrustworthy_empty_export(view, _dataset(view, [{"query": "a", "count": "1"}])) is False