fix: stop failing closed on always-empty top views, close orphaned tabs (#22, #9)

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

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

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

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