All of lore.kernel.org
 help / color / mirror / Atom feed
From: Christian Brauner <brauner@kernel.org>
To: "Kernel.org Tools" <tools@kernel.org>
Cc: Konstantin Ryabitsev <konstantin@linuxfoundation.org>,
	 "Christian Brauner (Amutable)" <brauner@kernel.org>
Subject: [PATCH RFC v2 13/25] review: test the per-revision poll sweep
Date: Wed, 12 Aug 2026 23:46:54 +0200	[thread overview]
Message-ID: <20260812-work-b4-multiver-rows-v2-13-305d53cd723a@kernel.org> (raw)
In-Reply-To: <20260812-work-b4-multiver-rows-v2-0-305d53cd723a@kernel.org>

Cover the sweep wiring around update_revision_message_counts(): a busy
branch keeping its poll while the ref is left alone, the thread snapshot
moving onto the catalog row so seen_bump stops re-counting read mail,
offline not counting as a failed poll, a poll that raises being counted
rather than dropped at debug level, and the cron reporting of
per-revision results and unreachable revisions.

Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 src/tests/test_review.py          | 688 ++++++++++++++++++++++++++++++++++++++
 src/tests/test_review_tracking.py | 281 ++++++++++++++++
 src/tests/test_tui_tracking.py    |  29 ++
 3 files changed, 998 insertions(+)

diff --git a/src/tests/test_review.py b/src/tests/test_review.py
index 4f566898..a7c9cb15 100644
--- a/src/tests/test_review.py
+++ b/src/tests/test_review.py
@@ -1,9 +1,11 @@
 import argparse
 import email.message
 import importlib.util
+import io
 import json
 import logging
 import os
+import sqlite3
 from typing import Any, Dict, Iterator, List, Optional, Tuple, Union
 from unittest import mock
 
@@ -11,6 +13,7 @@ import pytest
 
 import b4
 import b4.review.tracking
+import liblore
 from b4 import review, review_tui
 from b4.review import _review
 from b4.review._review import REVIEW_MAGIC_MARKER, check_series_attestation
@@ -4855,3 +4858,688 @@ class TestCreateReviewBranchCleanup:
         assert ecode == 0
         assert head.strip() == tip
         assert not b4.git_branch_exists(gitdir, branch)
+
+
+def test_update_all_tracking_polls_revisions_capped(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """The sweep must actually drive the per-revision poller, with a cap.
+
+    Uncapped this is one lore round-trip per known older revision of every
+    tracked series on every 'U' press.
+    """
+    series = [
+        {'change_id': 'a', 'subject': 's-a', 'status': 'new', 'sender_name': 'A'},
+        {'change_id': 'b', 'subject': 's-b', 'status': 'reviewing', 'sender_name': 'B'},
+    ]
+    monkeypatch.setattr(
+        b4.review.tracking, 'get_all_tracked_series', lambda identifier: series
+    )
+    monkeypatch.setattr(
+        review,
+        'update_series_tracking',
+        lambda one, identifier, linkmask, topdir=None: {
+            'new_revisions': 0,
+            'new_trailers': 0,
+            'error': None,
+        },
+    )
+    calls: List[Dict[str, Any]] = []
+
+    def fake_counts(
+        identifier: str,
+        series_list: List[Dict[str, Any]],
+        topdir: Optional[str] = None,
+        max_revisions_per_series: Optional[int] = None,
+        cancel_cb: Optional[Any] = None,
+        status_cb: Optional[Any] = None,
+        force: bool = False,
+    ) -> Dict[str, int]:
+        calls.append(
+            {
+                'change_ids': [s['change_id'] for s in series_list],
+                'cap': max_revisions_per_series,
+                'cancel_cb': cancel_cb,
+                'force': force,
+            }
+        )
+        return {'updated': 5, 'new_mail': 2, 'errors': 0}
+
+    monkeypatch.setattr(
+        b4.review.tracking, 'update_revision_message_counts', fake_counts
+    )
+    result = review.update_all_tracking(
+        'poller',
+        'https://lore.example/r/%s',
+        cancel_cb=lambda: False,
+    )
+    # One call for the whole list: the poller loops internally and applies
+    # the cap per series, so handing it a series at a time only reopened the
+    # database once per entry.
+    assert [c['change_ids'] for c in calls] == [['a', 'b']]
+    assert {c['cap'] for c in calls} == {b4.review.tracking.REVISION_POLL_LIMIT}
+    assert all(c['cancel_cb'] is not None for c in calls)
+    # Nothing asked for a forced poll, so recently checked versions keep
+    # their minimum-age schedule.
+    assert all(c['force'] is False for c in calls)
+    # The summary reports new mail, not every row the poller touched.
+    assert result['revision_counts_updated'] == 2
+    assert result['revision_errors'] == 0
+
+
+def test_update_all_tracking_reports_revision_poll_errors(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """A poller that never works must not look like a quiet mailing list.
+
+    Counted apart from result['errors'], which drives the per-series error
+    report: a revision that will not fetch is not a series that failed.
+    """
+    series = [{'change_id': 'a', 'subject': 's-a', 'status': 'new', 'sender_name': 'A'}]
+    monkeypatch.setattr(
+        b4.review.tracking, 'get_all_tracked_series', lambda identifier: series
+    )
+    monkeypatch.setattr(
+        review,
+        'update_series_tracking',
+        lambda one, identifier, linkmask, topdir=None: {
+            'new_revisions': 0,
+            'new_trailers': 0,
+            'error': None,
+        },
+    )
+    monkeypatch.setattr(
+        b4.review.tracking,
+        'update_revision_message_counts',
+        lambda *a, **kw: {'updated': 0, 'new_mail': 0, 'errors': 3, 'polled': 0},
+    )
+    result = review.update_all_tracking('poller-err', 'https://lore.example/r/%s')
+    assert result['revision_errors'] == 3
+    assert result['revision_polled'] == 0
+    assert result['errors'] == 0
+
+
+def test_update_all_tracking_feeds_poll_progress(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """The poll phase reports through the same callback as the series loop.
+
+    It runs after the bar has reached N/N and can be minutes of lore
+    traffic; without its own progress the update modal reads as hung.
+    """
+    series = [{'change_id': 'a', 'subject': 's-a', 'status': 'new', 'sender_name': 'A'}]
+    monkeypatch.setattr(
+        b4.review.tracking, 'get_all_tracked_series', lambda identifier: series
+    )
+    monkeypatch.setattr(
+        review,
+        'update_series_tracking',
+        lambda one, identifier, linkmask, topdir=None: {
+            'new_revisions': 0,
+            'new_trailers': 0,
+            'error': None,
+        },
+    )
+
+    def fake_counts(
+        identifier: str,
+        series_list: List[Dict[str, Any]],
+        topdir: Optional[str] = None,
+        max_revisions_per_series: Optional[int] = None,
+        cancel_cb: Optional[Any] = None,
+        status_cb: Optional[Any] = None,
+        force: bool = False,
+    ) -> Dict[str, int]:
+        assert status_cb is not None
+        status_cb(series_list[0]['subject'])
+        return {'updated': 0, 'new_mail': 0, 'errors': 0, 'polled': 1}
+
+    monkeypatch.setattr(
+        b4.review.tracking, 'update_revision_message_counts', fake_counts
+    )
+    seen: List[Any] = []
+    review.update_all_tracking(
+        'poll-progress',
+        'https://lore.example/r/%s',
+        progress_cb=lambda c, t, s: seen.append((c, t, s)),
+    )
+    # Held at N/N: the series loop already drove the bar there, and the
+    # poller hands over a subject and nothing else, so there is no second
+    # count to walk it backwards.  The label carries the poll's progress.
+    polled = [(c, t) for c, t, s in seen if s.startswith('Polling earlier')]
+    assert polled == [(1, 1)]
+
+
+@pytest.mark.parametrize(
+    ('polled', 'fresh', 'level'),
+    [(0, 3, 'WARNING'), (2, 3, 'DEBUG'), (0, 0, 'DEBUG')],
+)
+def test_cron_reports_unreachable_revisions_only_when_nothing_polled(
+    monkeypatch: pytest.MonkeyPatch,
+    caplog: pytest.LogCaptureFixture,
+    polled: int,
+    fresh: int,
+    level: str,
+) -> None:
+    """One dead message-id must not mail the maintainer on every sweep.
+
+    Nothing ever retires a revision whose message-id is permanently gone:
+    it keeps its place in the rotation and fails again on every pass.  A
+    sweep that polled something has a working poller and says so quietly.
+    And an empty sweep is only worth cron mail when something failed for
+    the *first* time: a known-dead revision (attempted before, never
+    fetched once) failing again is old news -- gating on the other
+    revisions' success alone still mailed forever whenever the dead one
+    was the only candidate at all.
+    """
+    monkeypatch.setattr(
+        _review,
+        'update_all_tracking',
+        lambda identifier, linkmask, topdir=None, **kw: {
+            'series_checked': 1,
+            'series_updated': 0,
+            'errors': 0,
+            'gone': 0,
+            'followup_updated': 0,
+            'revision_counts_updated': 0,
+            'revision_errors': 3,
+            'revision_fresh_errors': fresh,
+            'revision_polled': polled,
+            'branch_busy_skipped': 0,
+            'error_details': [],
+            'cancelled': False,
+        },
+    )
+    with caplog.at_level(logging.DEBUG, logger=b4.logger.name):
+        _review._cron_update('cron-poller', None)
+    records = [r for r in caplog.records if 'Could not poll' in r.getMessage()]
+    assert [r.levelname for r in records] == [level]
+
+
+def test_update_all_tracking_cancelled_poller_stops_sweep(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """Cancellation now reaches the handler -- the fetch helpers used to
+    swallow OperationCancelledError, making that branch unreachable.
+
+    The poll is one batched call after the series loop, so a cancel raised
+    there marks the sweep cancelled without unwinding the per-series work
+    that already finished.  A cancel the *maintainer* asks for still stops
+    the series loop itself: it checks cancel_cb at the top of every pass.
+    """
+    series = [
+        {'change_id': 'a', 'subject': 's-a', 'status': 'new', 'sender_name': 'A'},
+        {'change_id': 'b', 'subject': 's-b', 'status': 'new', 'sender_name': 'B'},
+    ]
+    monkeypatch.setattr(
+        b4.review.tracking, 'get_all_tracked_series', lambda identifier: series
+    )
+    monkeypatch.setattr(
+        review,
+        'update_series_tracking',
+        lambda one, identifier, linkmask, topdir=None: {
+            'new_revisions': 0,
+            'new_trailers': 0,
+            'error': None,
+        },
+    )
+
+    def cancelled(*a: Any, **kw: Any) -> Dict[str, int]:
+        raise liblore.OperationCancelledError('Request cancelled')
+
+    monkeypatch.setattr(b4.review.tracking, 'update_revision_message_counts', cancelled)
+    result = review.update_all_tracking('poller-cancel', 'https://lore.example/r/%s')
+    assert result['cancelled'] is True
+    assert result['series_checked'] == 2
+
+
+def _stub_sweep(monkeypatch: pytest.MonkeyPatch, series: List[Dict[str, Any]]) -> None:
+    """Point update_all_tracking at *series* with a no-op per-series update."""
+    monkeypatch.setattr(
+        b4.review.tracking, 'get_all_tracked_series', lambda identifier: series
+    )
+    monkeypatch.setattr(
+        review,
+        'update_series_tracking',
+        lambda one, identifier, linkmask, topdir=None: {
+            'new_revisions': 0,
+            'new_trailers': 0,
+            'error': None,
+        },
+    )
+
+
+def test_a_busy_branch_still_polls_its_revisions(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """Only the branch is left alone, not the per-revision poll.
+
+    The poll writes DB rows and a loose blob and moves no ref, while a
+    series under review is normally the checked-out branch -- so skipping
+    it there is skipping it almost always, and late replies to old
+    versions stop being noticed at all.
+    """
+    series = [
+        {'change_id': 'a', 'subject': 's-a', 'status': 'new', 'sender_name': 'A'},
+        {'change_id': 'b', 'subject': 's-b', 'status': 'new', 'sender_name': 'B'},
+    ]
+    monkeypatch.setattr(
+        b4.review.tracking, 'get_all_tracked_series', lambda identifier: series
+    )
+    monkeypatch.setattr(
+        review,
+        'update_series_tracking',
+        lambda one, identifier, linkmask, topdir=None: {
+            'new_revisions': 0,
+            'new_trailers': 0,
+            'error': None,
+            'branch_busy': one['change_id'] == 'a',
+        },
+    )
+    polled: List[str] = []
+
+    def _record(
+        identifier: str, series_list: List[Dict[str, Any]], **kw: Any
+    ) -> Dict[str, int]:
+        polled.extend(s['change_id'] for s in series_list)
+        return {'updated': 0, 'new_mail': 0, 'errors': 0}
+
+    monkeypatch.setattr(b4.review.tracking, 'update_revision_message_counts', _record)
+    result = review.update_all_tracking('poll-co', 'https://lore.example/r/%s')
+    # 'a' is the checked-out one, and it is still handed to the poller.
+    assert polled == ['a', 'b']
+    assert result['branch_busy_skipped'] == 1
+
+
+def test_update_all_tracking_forwards_the_forced_poll(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """A user-initiated sweep polls recently checked versions too.
+
+    The poller's minimum-age skip exists for unattended sweeps; on an
+    explicit u/U it silently withheld exactly the per-version counts the
+    refresh was pressed for.
+    """
+    series = [{'change_id': 'a', 'subject': 's-a', 'status': 'new', 'sender_name': 'A'}]
+    monkeypatch.setattr(
+        b4.review.tracking, 'get_all_tracked_series', lambda identifier: series
+    )
+    monkeypatch.setattr(
+        review,
+        'update_series_tracking',
+        lambda one, identifier, linkmask, topdir=None: {
+            'new_revisions': 0,
+            'new_trailers': 0,
+            'error': None,
+        },
+    )
+    forced: List[bool] = []
+
+    def fake_counts(
+        identifier: str,
+        series_list: List[Dict[str, Any]],
+        force: bool = False,
+        **kw: Any,
+    ) -> Dict[str, int]:
+        forced.append(force)
+        return {'updated': 0, 'new_mail': 0, 'errors': 0}
+
+    monkeypatch.setattr(
+        b4.review.tracking, 'update_revision_message_counts', fake_counts
+    )
+    review.update_all_tracking(
+        'force-poll', 'https://lore.example/r/%s', force_revision_poll=True
+    )
+    assert forced == [True]
+
+
+def test_busy_reported_only_when_an_update_was_forgone(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """The busy-branch report follows the statuses whose branch the sweep writes.
+
+    branch_busy guards every ref writer, but an accepted or thanked
+    series' branch was never going to be written -- counting it made the
+    'N checked-out branch(es) left alone' toast report branches the sweep
+    never touches, on every sweep, for as long as they stay checked out.
+    """
+    monkeypatch.setattr(b4, 'git_worktree_busy', lambda topdir, branch: True)
+    monkeypatch.setattr(
+        b4.review.tracking, 'store_revision_thread_blob', lambda *a, **kw: None
+    )
+    monkeypatch.setattr(b4.review.tracking, '_store_thread_blob', lambda *a, **kw: None)
+
+    def _run(status: str) -> Dict[str, Any]:
+        identifier = f'cog-{status}'
+        change_id = f'cog-{status}-cid'
+        conn = b4.review.tracking.init_db(identifier)
+        b4.review.tracking.add_series_to_db(
+            conn,
+            change_id,
+            1,
+            'Subject',
+            'Author',
+            'a@example.com',
+            '2024-01-15T10:00:00+00:00',
+            'cover@example.com',
+            2,
+        )
+        b4.review.tracking.update_series_status(conn, change_id, status)
+        conn.close()
+
+        msg = email.message.EmailMessage()
+        msg['Message-Id'] = f'<{change_id}-r1@example.com>'
+        msg['Date'] = 'Mon, 27 Jul 2026 10:00:00 +0000'
+        mock_lmbx = mock.Mock()
+        mock_lmbx.series = {}
+        mock_lmbx.covers = {}
+        mock_lmbx.get_series.return_value = None
+        series_dict: Dict[str, Any] = {
+            'change_id': change_id,
+            'revision': 1,
+            'status': status,
+            'message_id': 'cover@example.com',
+        }
+        with (
+            mock.patch(
+                'b4.review._review.retrieve_series_messages', return_value=[msg]
+            ),
+            mock.patch('b4.LoreMailbox', return_value=mock_lmbx),
+        ):
+            return review.update_series_tracking(
+                series_dict, identifier, 'https://example.com/%s', topdir='/nonexistent'
+            )
+
+    assert _run('reviewing').get('branch_busy') is True
+    assert not _run('accepted').get('branch_busy')
+
+
+def _seed_catalog_blob(identifier: str, blob_sha: str) -> None:
+    conn = b4.review.tracking.init_db(identifier)
+    b4.review.tracking.add_revision(conn, 'cid', 1, 'v1@x')
+    conn.execute(
+        "UPDATE revisions SET thread_blob = ? WHERE change_id = 'cid'", (blob_sha,)
+    )
+    conn.commit()
+    conn.close()
+
+
+def test_prev_thread_msgids_reads_the_catalog_snapshot(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """The catalog row's blob outranks the tracking commit's.
+
+    The catalog copy advances on every counted fetch, including sweeps
+    that leave a checked-out branch's ref alone; the tracking commit's
+    blob is frozen then, and deriving seen bumps from it re-counted the
+    same read messages as new on every sweep.
+    """
+    _seed_catalog_blob('prev-snap', 'c0ffee')
+    msgs = []
+    for i in range(2):
+        msg = email.message.EmailMessage()
+        msg['Message-Id'] = f'<prev-{i}@example.com>'
+        msg['Date'] = 'Mon, 27 Jul 2026 10:00:00 +0000'
+        msg.set_content('body')
+        msgs.append(msg)
+    buf = io.BytesIO()
+    b4.save_mboxrd_mbox(msgs, buf)
+    asked: List[str] = []
+
+    def _mbox(topdir: str, sha: str) -> bytes:
+        asked.append(sha)
+        return buf.getvalue()
+
+    monkeypatch.setattr(b4.review.tracking, 'get_thread_mbox', _mbox)
+
+    def _no_branch(*a: Any, **kw: Any) -> bool:
+        raise AssertionError('tracking commit consulted despite a catalog snapshot')
+
+    monkeypatch.setattr(b4, 'git_branch_exists', _no_branch)
+    got = _review._prev_thread_msgids('/nonexistent', 'prev-snap', 'cid', 1)
+    assert asked == ['c0ffee']
+    assert got == {'prev-0@example.com', 'prev-1@example.com'}
+
+
+def test_prev_thread_msgids_pruned_catalog_blob_is_no_snapshot(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """A recorded blob that no longer reads back yields None, not a fallback.
+
+    The tracking commit's copy is older than the pruned one, and bumps
+    against it were already applied; "cannot tell which messages are
+    new" is the honest answer.
+    """
+    _seed_catalog_blob('prev-gcd', 'deadbeef')
+    monkeypatch.setattr(b4.review.tracking, 'get_thread_mbox', lambda topdir, sha: None)
+
+    def _no_branch(*a: Any, **kw: Any) -> bool:
+        raise AssertionError('tracking commit consulted despite a catalog snapshot')
+
+    monkeypatch.setattr(b4, 'git_branch_exists', _no_branch)
+    assert _review._prev_thread_msgids('/nonexistent', 'prev-gcd', 'cid', 1) is None
+
+
+def test_a_raising_poller_is_counted_not_swallowed(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """At debug level the whole feature can break while sweeps look clean."""
+    _stub_sweep(
+        monkeypatch,
+        [{'change_id': 'a', 'subject': 's-a', 'status': 'new', 'sender_name': 'A'}],
+    )
+
+    def _boom(*a: Any, **kw: Any) -> Dict[str, int]:
+        raise sqlite3.OperationalError('database is locked')
+
+    monkeypatch.setattr(b4.review.tracking, 'update_revision_message_counts', _boom)
+    result = review.update_all_tracking('poll-raise', 'https://lore.example/r/%s')
+    assert result['revision_errors'] == 1
+    # Still not a per-series failure: the series itself updated fine.
+    assert result['errors'] == 0
+
+
+def test_a_cancelled_sweep_does_not_start_the_poller(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """The poll is minutes of lore traffic; a cancel must land before it."""
+    _stub_sweep(
+        monkeypatch,
+        [{'change_id': 'a', 'subject': 's-a', 'status': 'new', 'sender_name': 'A'}],
+    )
+    called: List[int] = []
+
+    def _record(*a: Any, **kw: Any) -> Dict[str, int]:
+        called.append(1)
+        return {'updated': 0, 'new_mail': 0, 'errors': 0}
+
+    monkeypatch.setattr(b4.review.tracking, 'update_revision_message_counts', _record)
+    review.update_all_tracking(
+        'poll-cancelled', 'https://lore.example/r/%s', cancel_cb=lambda: True
+    )
+    assert called == []
+
+
+class TestCancelledPollKeepsItsTally:
+    """A cancel mid-poll must not discard the work already committed.
+
+    The per-revision poller commits each revision as it goes, so a cancel
+    raised on revision N leaves revisions 1..N-1 counted in the database.
+    Reporting that sweep as `{'revision_polled': 0}` makes those badges
+    appear with nothing having announced them -- and makes a permanently
+    broken poller read exactly like a cancelled one.
+    """
+
+    @staticmethod
+    def _msgs(count: int) -> List[Any]:
+        """A thread of *count* messages, each carrying a Date header."""
+        out: List[Any] = []
+        for i in range(count):
+            msg = email.message.EmailMessage()
+            msg['Message-ID'] = f'<poll-{i}@example.com>'
+            msg['Subject'] = f'Re: [PATCH] thing ({i})'
+            msg['Date'] = 'Tue, 07 Jul 2026 12:00:00 +0000'
+            msg.set_content(f'body {i}')
+            out.append(msg)
+        return out
+
+    def test_partial_poll_tallies_survive_a_cancel(
+        self, monkeypatch: pytest.MonkeyPatch, tmp_path: Any
+    ) -> None:
+        identifier = 'poll-cancel-partial'
+        conn = b4.review.tracking.init_db(identifier)
+        # Two older versions, both already counted once: a second fetch that
+        # grows is then real new mail rather than a first count, which the
+        # sweep deliberately does not report as new mail.
+        for rev in (1, 2):
+            b4.review.tracking.add_revision(conn, 'a', rev, f'a-v{rev}@example.com')
+        conn.execute(
+            'UPDATE revisions SET message_count = 2, seen_message_count = 2'
+            " WHERE change_id = 'a'"
+        )
+        conn.commit()
+        conn.close()
+
+        series = [
+            {
+                'change_id': 'a',
+                'revision': 3,
+                'subject': 's-a',
+                'status': 'new',
+                'sender_name': 'A',
+            }
+        ]
+        monkeypatch.setattr(b4, 'can_network', True)
+        monkeypatch.setattr(
+            b4.review.tracking, 'get_all_tracked_series', lambda identifier: series
+        )
+        monkeypatch.setattr(
+            review,
+            'update_series_tracking',
+            lambda one, identifier, linkmask, topdir=None: {
+                'new_revisions': 0,
+                'new_trailers': 0,
+                'error': None,
+            },
+        )
+
+        fetched: List[int] = []
+
+        def fetch(
+            identifier_: str,
+            conn_: Any,
+            change_id: str,
+            rev: Dict[str, Any],
+        ) -> List[Any]:
+            fetched.append(int(rev['revision']))
+            if len(fetched) == 1:
+                # Grew 2 -> 3: one revision's worth of genuine new mail,
+                # committed before the cancel lands.
+                return TestCancelledPollKeepsItsTally._msgs(3)
+            raise liblore.OperationCancelledError('Request cancelled')
+
+        monkeypatch.setattr(b4.review.tracking, '_fetch_revision_thread_msgs', fetch)
+
+        result = review.update_all_tracking(identifier, 'https://lore.example/r/%s')
+
+        # One revision completed, the next raised.
+        assert len(fetched) == 2
+        assert result['cancelled'] is True
+
+        # The completed revision really is committed -- this is the work the
+        # summary below has to account for, not a hypothetical.
+        conn = b4.review.tracking.get_db(identifier)
+        grown = conn.execute(
+            "SELECT COUNT(*) FROM revisions WHERE change_id = 'a' AND message_count = 3"
+        ).fetchone()[0]
+        conn.close()
+        assert grown == 1
+
+        # ...so the sweep must report it rather than returning zeros.
+        assert result['revision_polled'] == 1
+        assert result['revision_counts_updated'] == 1
+
+
+class TestExplicitUpdateReachesASnoozedSeries:
+    """[u] on one row is a question about that row, [U] is a sweep.
+
+    An explicit single-series update already spends the larger network
+    cost on a snoozed series -- update_all_tracking does not filter a
+    caller-supplied series_list, so the tracked revision's thread is
+    fetched and its trailers re-read.  Declining only the cheaper
+    per-revision top-up afterwards is not a network-thrift policy, it is
+    an inconsistency: the maintainer asked about this series and got a
+    partial answer with nothing saying so.  The scheduled sweep keeps
+    skipping snoozed rows, which is what 'snoozed' is for.
+    """
+
+    @staticmethod
+    def _seed(identifier: str) -> List[Dict[str, Any]]:
+        conn = b4.review.tracking.init_db(identifier)
+        for rev in (1, 2):
+            b4.review.tracking.add_revision(conn, 'snz', rev, f'snz-v{rev}@example.com')
+        conn.close()
+        return [
+            {
+                'change_id': 'snz',
+                'revision': 2,
+                'subject': 's-snz',
+                'status': 'snoozed',
+                'sender_name': 'S',
+            }
+        ]
+
+    def _run(
+        self,
+        identifier: str,
+        monkeypatch: pytest.MonkeyPatch,
+        forced: bool,
+    ) -> List[int]:
+        series_list = self._seed(identifier)
+        monkeypatch.setattr(b4, 'can_network', True)
+        monkeypatch.setattr(
+            review,
+            'update_series_tracking',
+            lambda one, ident, linkmask, topdir=None: {
+                'new_revisions': 0,
+                'new_trailers': 0,
+                'error': None,
+            },
+        )
+        fetched: List[int] = []
+
+        def fetch(
+            ident: str, conn: Any, change_id: str, rev: Dict[str, Any]
+        ) -> List[Any]:
+            fetched.append(int(rev['revision']))
+            msg = email.message.EmailMessage()
+            msg['Message-ID'] = '<snz-reply@example.com>'
+            msg['Date'] = 'Tue, 07 Jul 2026 12:00:00 +0000'
+            msg.set_content('late reply')
+            return [msg]
+
+        monkeypatch.setattr(b4.review.tracking, '_fetch_revision_thread_msgs', fetch)
+        review.update_all_tracking(
+            identifier,
+            'https://lore.example/r/%s',
+            series_list=series_list,
+            force_revision_poll=forced,
+        )
+        return fetched
+
+    def test_u_polls_the_older_versions_of_a_snoozed_series(
+        self, monkeypatch: pytest.MonkeyPatch, tmp_path: Any
+    ) -> None:
+        """[u] is the maintainer asking now -- answer about every version."""
+        assert self._run('snooze-forced', monkeypatch, forced=True) == [1]
+
+    def test_the_scheduled_sweep_still_leaves_a_snoozed_series_alone(
+        self, monkeypatch: pytest.MonkeyPatch, tmp_path: Any
+    ) -> None:
+        """...but nothing unattended goes near it.
+
+        Pinned alongside the case above so the fix cannot be "drop
+        skip_statuses", which would put every snoozed series back into
+        the cron sweep's lore budget.
+        """
+        assert self._run('snooze-sweep', monkeypatch, forced=False) == []
diff --git a/src/tests/test_review_tracking.py b/src/tests/test_review_tracking.py
index df867938..e2fcdf85 100644
--- a/src/tests/test_review_tracking.py
+++ b/src/tests/test_review_tracking.py
@@ -5054,6 +5054,27 @@ class TestUpdateSkipsABusyWorktree:
         # DB-side maintenance still ran
         assert result.get('counts_updated') is True
 
+    def test_a_quiescent_checkout_is_written(self, gitdir: str) -> None:
+        """Checked out is not busy, so the branch section is entered.
+
+        Skipping every checkout deferred the tracking commit for as long
+        as a series stayed under review -- which is most of its life --
+        over a write that cannot disturb a worktree standing still.  It
+        reaches the mocked-away series and fails on the sentinel error,
+        which is what shows it got that far.
+        """
+        change_id = 'co-quiescent'
+        branch = _create_review_branch(
+            gitdir, change_id, self._tracking_data(change_id)
+        )
+        ecode, _ = b4.git_run_command(gitdir, ['checkout', branch])
+        assert ecode == 0
+
+        result = self._run_update(gitdir, change_id)
+
+        assert result.get('branch_busy') is None
+        assert result.get('error') == 'Could not find series v1 in retrieved messages'
+
     def test_parked_branch_still_updated(self, gitdir: str) -> None:
         """Control: with the branch not checked out the section is entered
         (and fails on the mocked-away series — the sentinel error)."""
@@ -7942,6 +7963,60 @@ class TestPrunedThreadBlobIsReCached:
         )
         assert stored == [8]
 
+    @staticmethod
+    def _quiet_tracked_update(
+        identifier: str, monkeypatch: pytest.MonkeyPatch, blob_alive: bool
+    ) -> list[int]:
+        """Run the tracked revision's writer over an unchanged thread."""
+        conn = review_tracking.init_db(identifier)
+        review_tracking.add_revision(conn, 'cid', 1, 'v1@x')
+        conn.execute(
+            'UPDATE revisions SET message_count = 2, seen_message_count = 2,'
+            " thread_blob = 'deadbeef' WHERE change_id = 'cid' AND revision = 1"
+        )
+        conn.commit()
+        conn.close()
+        stored: list[int] = []
+        monkeypatch.setattr(
+            review_tracking, '_thread_blob_exists', lambda topdir, sha: blob_alive
+        )
+        monkeypatch.setattr(
+            review_tracking,
+            'store_revision_thread_blob',
+            lambda conn, topdir, change_id, revision, msgs: stored.append(len(msgs)),
+        )
+        conn = review_tracking.get_db(identifier)
+        review_tracking.update_message_count_from_msgs(
+            conn, 'cid', 1, _thread_msgs(2), topdir='/nonexistent'
+        )
+        conn.close()
+        return stored
+
+    def test_a_gc_d_blob_is_replaced_on_a_quiet_tracked_update(
+        self, tmp_path: pytest.TempPathFactory, monkeypatch: pytest.MonkeyPatch
+    ) -> None:
+        """The tracked revision is the one the poller never covers.
+
+        _tracked_revisions keeps the poll off it, so its writer's quiet
+        path is the only place that can notice git gc took its thread --
+        and a settled series (accepted, thanked, waiting on a new version)
+        is exactly the one whose count stops moving for weeks.  Left
+        unnoticed, _prev_thread_msgids reads back nothing and the
+        maintainer's own replies badge as unread on the next sweep that
+        does see mail.
+        """
+        assert self._quiet_tracked_update('tracked-blob-gc', monkeypatch, False) == [2]
+
+    def test_a_live_blob_is_left_alone_on_a_quiet_tracked_update(
+        self, tmp_path: pytest.TempPathFactory, monkeypatch: pytest.MonkeyPatch
+    ) -> None:
+        """Same rule as the poller's: existence, not a rewrite.
+
+        Re-serializing an mbox that is already stored under that SHA would
+        also throw away a stitched series_blob that is still good.
+        """
+        assert self._quiet_tracked_update('tracked-blob-live', monkeypatch, True) == []
+
 
 class TestSeenWritersStayConsistent:
     @staticmethod
@@ -8071,3 +8146,209 @@ class TestMigrationDeclinesWhatItCannotCarry:
         conn.close()
         assert 'branch_sha' not in series_cols
         assert 'branch_sha' in chg_cols
+
+
+class TestForceLiftsTheSnoozedSkip:
+    """Snoozed is a sweep policy, not a fact about the series.
+
+    'u' on a snoozed row already updates that series' tracked revision, so
+    silently declining to poll its other versions answers half the request.
+    'U' and the cron sweep never set force, so they keep skipping.
+    """
+
+    def _polled(
+        self, identifier: str, status: str, force: bool, monkeypatch: pytest.MonkeyPatch
+    ) -> int:
+        conn = review_tracking.init_db(identifier)
+        review_tracking.add_series_to_db(
+            conn,
+            change_id='cid',
+            revision=2,
+            subject='[PATCH v2] s',
+            sender_name='S',
+            sender_email='s@e.com',
+            sent_at='2026-01-01T00:00:00+00:00',
+            message_id='v2@x',
+            num_patches=1,
+        )
+        review_tracking.add_revision(conn, 'cid', 1, 'v1@x')
+        conn.close()
+        monkeypatch.setattr(b4, 'can_network', True)
+        seen: list[int] = []
+
+        def _fetch(
+            identifier: str,
+            conn: Any,
+            change_id: str,
+            rev: Dict[str, Any],
+        ) -> list[EmailMessage]:
+            seen.append(int(rev['revision']))
+            return _thread_msgs(3)
+
+        monkeypatch.setattr(review_tracking, '_fetch_revision_thread_msgs', _fetch)
+        review_tracking.update_revision_message_counts(
+            identifier,
+            [_poller_series('cid', 2, 'v2@x', status=status)],
+            force=force,
+        )
+        return len(seen)
+
+    def test_a_sweep_still_skips_snoozed(
+        self, tmp_path: pytest.TempPathFactory, monkeypatch: pytest.MonkeyPatch
+    ) -> None:
+        assert self._polled('snz-sweep', 'snoozed', False, monkeypatch) == 0
+
+    def test_asking_directly_polls_it(
+        self, tmp_path: pytest.TempPathFactory, monkeypatch: pytest.MonkeyPatch
+    ) -> None:
+        assert self._polled('snz-force', 'snoozed', True, monkeypatch) == 1
+
+    def test_archived_is_skipped_even_then(
+        self, tmp_path: pytest.TempPathFactory, monkeypatch: pytest.MonkeyPatch
+    ) -> None:
+        """No tracking-list row means no way to ask, so nothing to lift."""
+        assert self._polled('arc-force', 'archived', True, monkeypatch) == 0
+
+
+class TestThreadBlobIsHashedOnce:
+    """Both writers of the snapshot want the same blob.
+
+    Blobs are content-addressed, so serializing the mbox a second time and
+    running hash-object on it again produces the SHA the first write
+    already returned -- at the cost of a second subprocess, on every sweep
+    for every series whose count moved.
+    """
+
+    def test_one_write_serves_the_catalog_and_the_tracking_commit(
+        self, tmp_path: pytest.TempPathFactory, monkeypatch: pytest.MonkeyPatch
+    ) -> None:
+        conn = review_tracking.init_db('blob-once')
+        review_tracking.add_revision(conn, 'cid', 1, 'v1@x')
+
+        writes: list[int] = []
+
+        def _fake_write(topdir: str, msgs: Any) -> str:
+            writes.append(len(msgs))
+            return 'a' * 40
+
+        monkeypatch.setattr(review_tracking, '_write_mbox_blob', _fake_write)
+        msg = EmailMessage()
+        msg['Message-Id'] = '<v1@x>'
+        msg['Date'] = 'Mon, 27 Jul 2026 10:00:00 +0000'
+
+        assert review_tracking.update_message_count_from_msgs(
+            conn, 'cid', 1, [msg], topdir='/nonexistent'
+        )
+        conn.close()
+        assert writes == [1]
+
+    def test_a_failed_write_still_lets_the_second_writer_try(
+        self, tmp_path: pytest.TempPathFactory, monkeypatch: pytest.MonkeyPatch
+    ) -> None:
+        """None is 'no SHA to reuse', not 'do not bother'."""
+        conn = review_tracking.init_db('blob-once-fail')
+        review_tracking.add_revision(conn, 'cid', 1, 'v1@x')
+
+        writes: list[int] = []
+
+        def _failing_write(topdir: str, msgs: Any) -> None:
+            writes.append(len(msgs))
+            return None
+
+        monkeypatch.setattr(review_tracking, '_write_mbox_blob', _failing_write)
+        msg = EmailMessage()
+        msg['Message-Id'] = '<v1@x>'
+        msg['Date'] = 'Mon, 27 Jul 2026 10:00:00 +0000'
+
+        review_tracking.update_message_count_from_msgs(
+            conn, 'cid', 1, [msg], topdir='/nonexistent'
+        )
+        conn.close()
+        assert writes == [1, 1]
+
+
+class TestThreadBlobResolution:
+    """A frozen tracking commit must not serve a stale thread.
+
+    A sweep whose branch is checked out -- the normal state for a series
+    under review -- advances the catalog copy and leaves the tracking
+    commit's behind.  Every reader of the snapshot has to prefer the
+    catalog, or the follow-ups that raised the unread badge are exactly
+    the ones it cannot see.
+    """
+
+    @staticmethod
+    def _blob(gitdir: str, text: str) -> str:
+        """A real blob, so the existence check has something to find."""
+        ecode, out = b4.git_run_command(
+            gitdir, ['hash-object', '-w', '--stdin'], stdin=text.encode()
+        )
+        assert ecode == 0
+        return out.strip()
+
+    @staticmethod
+    def _enroll(gitdir: str, identifier: str) -> None:
+        review_tracking.save_repo_metadata(
+            b4.git_get_common_dir(gitdir) or gitdir, identifier
+        )
+
+    @staticmethod
+    def _block(**extra: Any) -> Dict[str, Any]:
+        """A tracking-commit series block, spelled the way b4 writes one."""
+        block: Dict[str, Any] = {
+            'change-id': 'cid',
+            'revision': 2,
+            'thread-blob': 'stalesha',
+        }
+        block.update(extra)
+        return block
+
+    def _seed(self, gitdir: str, identifier: str, blob: Optional[str]) -> None:
+        conn = review_tracking.init_db(identifier)
+        review_tracking.add_revision(conn, 'cid', 2, 'v2@x')
+        if blob is not None:
+            review_tracking.set_revision_thread_blob(conn, 'cid', 2, blob)
+        conn.close()
+        self._enroll(gitdir, identifier)
+
+    def test_catalog_wins_over_a_frozen_tracking_commit(self, gitdir: str) -> None:
+        fresh = self._blob(gitdir, 'the thread as the sweep last counted it')
+        self._seed(gitdir, 'blob-resolve', fresh)
+        assert review_tracking.resolve_thread_blob(gitdir, self._block()) == fresh
+
+    def test_the_block_is_read_rather_than_described(self, gitdir: str) -> None:
+        """The id comes out of the block, so it cannot disagree with it.
+
+        save_tracking_ref() writes 'change-id'.  A reader handed the id
+        beside the block asked for 'change_id', got '', and fell through to
+        the frozen copy without a word -- the catalog lookup is guarded on
+        a non-empty id, so the miss is silent.  With no second argument to
+        get wrong, a block carrying no id at all is what is left.
+        """
+        fresh = self._blob(gitdir, 'fresh')
+        self._seed(gitdir, 'blob-keys', fresh)
+        block = self._block()
+        del block['change-id']
+        assert review_tracking.resolve_thread_blob(gitdir, block) == 'stalesha'
+
+    def test_falls_back_to_the_tracking_commit(self, gitdir: str) -> None:
+        """Rows written before the catalog had a copy still resolve."""
+        self._seed(gitdir, 'blob-fallback', None)
+        assert review_tracking.resolve_thread_blob(gitdir, self._block()) == 'stalesha'
+
+    def test_a_pruned_catalog_blob_is_not_a_snapshot(self, gitdir: str) -> None:
+        """gc takes loose blobs; the SHA on the row outlives them.
+
+        Thread blobs are written with hash-object -w and referenced only
+        from the database, so a recorded SHA can name an object that is no
+        longer there.  Returning it anyway shadows the tracking commit's
+        copy -- a different blob, which may well still be readable -- and
+        hands the caller a dead SHA where it had a working one before the
+        catalog existed.
+        """
+        self._seed(gitdir, 'blob-pruned', 'd' * 40)
+        assert review_tracking.resolve_thread_blob(gitdir, self._block()) == 'stalesha'
+
+    def test_no_topdir_still_answers(self) -> None:
+        assert review_tracking.resolve_thread_blob(None, {'thread-blob': 'x'}) == 'x'
+        assert review_tracking.resolve_thread_blob(None, {}) == ''
diff --git a/src/tests/test_tui_tracking.py b/src/tests/test_tui_tracking.py
index cc80e7b1..2803bf3d 100644
--- a/src/tests/test_tui_tracking.py
+++ b/src/tests/test_tui_tracking.py
@@ -6001,3 +6001,32 @@ class TestRethreadFlagReachesTheThreadFetch:
             screen._fetch_thread()
         assert seen['is_rethreaded'] is True
         assert seen['revision'] == 2
+
+
+class TestUpdateAllDoesNotForceThePoll:
+    """'u' asks about one series; 'U' must not force the schedule everywhere.
+
+    Both keys push the same UpdateAllScreen, so a single flag on the screen
+    made 'U' bypass the minimum-age skip for every non-snoozed series --
+    REVISION_POLL_LIMIT round-trips apiece where a scheduled sweep would
+    have done almost none.
+    """
+
+    @pytest.mark.asyncio
+    async def test_u_forces_and_capital_u_does_not(
+        self, tmp_path: pathlib.Path
+    ) -> None:
+        _seed_db('force-scope', SAMPLE_SERIES)
+        seen = []
+
+        app = TrackingApp('force-scope')
+        async with app.run_test(size=(120, 30)) as pilot:
+            await pilot.pause()
+            with patch.object(
+                app, 'push_screen', lambda s, callback=None: seen.append(s)
+            ):
+                app.action_update_one()
+                app.action_update_all()
+        assert len(seen) == 2
+        assert seen[0]._force_revision_poll is True
+        assert seen[1]._force_revision_poll is False

-- 
2.53.0


  parent reply	other threads:[~2026-08-12 21:47 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-08-12 21:46 [PATCH RFC v2 00/25] review: track and browse every version of a tracked series Christian Brauner
2026-08-12 21:46 ` [PATCH RFC v2 01/25] review-tui: fix rethreaded series thread viewing Christian Brauner
2026-08-12 21:46 ` [PATCH RFC v2 02/25] review: do not clear fields a re-adding caller does not know Christian Brauner
2026-08-12 21:46 ` [PATCH RFC v2 03/25] review-tui: keep the rethread flag on an upgraded series row Christian Brauner
2026-08-12 21:46 ` [PATCH RFC v2 04/25] review: guard the tracking-commit amend on the worktree, not the checkout Christian Brauner
2026-08-12 21:46 ` [PATCH RFC v2 05/25] review-tui: recompute an evicted A·R·T cache entry Christian Brauner
2026-08-12 21:46 ` [PATCH RFC v2 06/25] review: test the prerequisite fixes Christian Brauner
2026-08-12 21:46 ` [PATCH RFC v2 07/25] review: serialize schema migrations against a concurrent opener Christian Brauner
2026-08-12 21:46 ` [PATCH RFC v2 08/25] review: test the migration serialization Christian Brauner
2026-08-12 21:46 ` [PATCH RFC v2 09/25] review: track message counts for all revisions of a series Christian Brauner
2026-08-12 21:46 ` [PATCH RFC v2 10/25] review: give per-change_id state its own table Christian Brauner
2026-08-12 21:46 ` [PATCH RFC v2 11/25] review: test per-revision message tracking Christian Brauner
2026-08-12 21:46 ` [PATCH RFC v2 12/25] review-tui: poll every revision on u/U updates Christian Brauner
2026-08-12 21:46 ` Christian Brauner [this message]
2026-08-12 21:46 ` [PATCH RFC v2 14/25] review-tui: resolve the tracked revision in revision lists Christian Brauner
2026-08-12 21:46 ` [PATCH RFC v2 15/25] review-tui: fall back when a cached thread blob has no series Christian Brauner
2026-08-12 21:46 ` [PATCH RFC v2 16/25] review-tui: test revision resolution and the range-diff fallback Christian Brauner
2026-08-12 21:46 ` [PATCH RFC v2 17/25] review: skip the catalog mirror when nothing moved Christian Brauner
2026-08-12 21:46 ` [PATCH RFC v2 18/25] review: match a stray posting by message-id Christian Brauner
2026-08-12 21:47 ` [PATCH RFC v2 19/25] review: add backward discovery of older series revisions Christian Brauner
2026-08-12 21:47 ` [PATCH RFC v2 20/25] review-tui: add a "Find older revisions" action Christian Brauner
2026-08-12 21:47 ` [PATCH RFC v2 21/25] review: test the catalog mirror, stray matching and backward discovery Christian Brauner
2026-08-12 21:47 ` [PATCH RFC v2 22/25] review-tui: extract the Msgs column renderer from TrackedSeriesItem Christian Brauner
2026-08-12 21:47 ` [PATCH RFC v2 23/25] review-tui: give the unseen badge a column of its own Christian Brauner
2026-08-12 21:47 ` [PATCH RFC v2 24/25] review-tui: expand tracked series into per-version rows Christian Brauner
2026-08-12 21:47 ` [PATCH RFC v2 25/25] review-tui: test per-version tracker rows Christian Brauner

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260812-work-b4-multiver-rows-v2-13-305d53cd723a@kernel.org \
    --to=brauner@kernel.org \
    --cc=konstantin@linuxfoundation.org \
    --cc=tools@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.