Skip to content

Commit 5756436

Browse files
committed
fix(core): confirm a missing page before reporting a failed store
- query a title reported as missing a second time, 5 s later, so a read from a lagging database replica cannot fail a store that applied - a failing verification query no longer discards the results of the whole store_entity call: the pages stay reported and the error is logged - add tests for both paths
1 parent f84a529 commit 5756436

2 files changed

Lines changed: 97 additions & 9 deletions

File tree

‎src/osw/core.py‎

Lines changed: 41 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
import warnings
1010
from copy import deepcopy
1111
from enum import Enum
12+
from time import sleep
1213
from typing import Any, Dict, List, Optional, Type, Union, overload
1314
from uuid import UUID, uuid4
1415
from warnings import warn
@@ -1643,8 +1644,10 @@ class StoreEntityParam(OswBaseModel):
16431644
"""If set to True, the existence of every edited page is queried after the
16441645
upload. A page that does not exist afterwards is reported in
16451646
StoreEntityResult.failed instead of StoreEntityResult.pages. This costs one
1646-
additional API request per 50 edited pages. Has no effect if 'offline' is
1647-
True."""
1647+
additional API request per 50 edited pages, and one further request some
1648+
seconds later if a page is reported as missing. If the query itself fails,
1649+
the pages are reported as stored and an error is logged. Has no effect if
1650+
'offline' is True."""
16481651
_overwrite_per_class: Dict[str, Dict[str, OSW.OverwriteClassParam]] = (
16491652
PrivateAttr()
16501653
)
@@ -1739,9 +1742,32 @@ def __init__(self, title: str):
17391742
f"rejected by the wiki without an error response."
17401743
)
17411744

1742-
def _get_missing_page_titles(self, titles: List[str]) -> List[str]:
1745+
def _get_missing_page_titles(
1746+
self, titles: List[str], confirm_delay_s: int = 5
1747+
) -> List[str]:
17431748
"""Returns those of the given page titles that do not exist on the wiki.
17441749
1750+
A title the wiki reports as missing is queried a second time after
1751+
confirm_delay_s seconds. A read can be answered by a database replica that
1752+
does not have the write yet, and a title that is still absent seconds later
1753+
is not explained by that lag.
1754+
1755+
Parameters
1756+
----------
1757+
titles:
1758+
Full page titles to check.
1759+
confirm_delay_s:
1760+
Seconds to wait before the second query. Set to 0 to query only once.
1761+
"""
1762+
missing = self._query_missing_page_titles(titles)
1763+
if missing and confirm_delay_s:
1764+
sleep(confirm_delay_s)
1765+
missing = self._query_missing_page_titles(missing)
1766+
return missing
1767+
1768+
def _query_missing_page_titles(self, titles: List[str]) -> List[str]:
1769+
"""Asks the wiki once which of the given page titles do not exist.
1770+
17451771
The query goes to the MediaWiki API directly and not through
17461772
WtSite.get_page(), because the page cache would answer with the state from
17471773
before the write.
@@ -2032,7 +2058,18 @@ def failure_title_(upload_object: UploadObject) -> str:
20322058
titles_to_verify = [
20332059
title for title in edited_titles if title in created_pages
20342060
]
2035-
for title in self._get_missing_page_titles(titles_to_verify):
2061+
try:
2062+
missing_titles = self._get_missing_page_titles(titles_to_verify)
2063+
except Exception as e:
2064+
# A failed query is no evidence that the writes failed. Report it
2065+
# and keep the pages, instead of discarding everything this call
2066+
# has collected so far.
2067+
missing_titles = []
2068+
_logger.error(
2069+
f"Could not verify {len(titles_to_verify)} stored pages, they "
2070+
f"are reported as stored without being checked: {e}"
2071+
)
2072+
for title in missing_titles:
20362073
error = OSW.PageNotCreatedError(title)
20372074
_logger.error(f"Error storing entity '{title}': {error}")
20382075
failed[title] = error

‎tests/test_store_entity_verify.py‎

Lines changed: 56 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212

1313
import pytest
1414

15+
import osw.core as core_mod
1516
import osw.model.entity as model
1617
from osw.core import OSW
1718
from osw.utils.wiki import get_namespace, get_title
@@ -23,21 +24,33 @@ def _title(entity):
2324

2425

2526
class _FakeMwSite:
26-
"""Records the API queries and answers them from a set of missing titles."""
27+
"""Records the API queries and answers them from a set of missing titles.
2728
28-
def __init__(self, missing_titles=(), normalized=None):
29+
missing_once holds titles that are reported as missing by the first query
30+
only, which is what a read from a lagging database replica looks like.
31+
"""
32+
33+
def __init__(
34+
self, missing_titles=(), normalized=None, missing_once=(), fails=False
35+
):
2936
self.missing_titles = set(missing_titles)
37+
self.missing_once = set(missing_once)
3038
self.normalized = normalized or {}
39+
self.fails = fails
3140
self.queries = []
3241

3342
def api(self, action, **kwargs):
3443
assert action == "query"
44+
if self.fails:
45+
raise RuntimeError("the API is not reachable")
3546
titles = kwargs["titles"].split("|")
3647
self.queries.append(titles)
48+
missing_now = self.missing_titles | self.missing_once
49+
self.missing_once = set()
3750
pages = {}
3851
for i, title in enumerate(titles):
3952
reported = self.normalized.get(title, title)
40-
if title in self.missing_titles:
53+
if title in missing_now:
4154
pages[str(-(i + 1))] = {"title": reported, "missing": ""}
4255
else:
4356
pages[str(i + 1)] = {"title": reported, "pageid": i + 1}
@@ -62,9 +75,11 @@ def offline_osw(monkeypatch):
6275
OSW, "_apply_overwrite_policy", staticmethod(lambda param: param.page)
6376
)
6477
monkeypatch.setattr(WtPage, "edit", lambda self, *a, **kw: None)
78+
# the delay before the confirmation query, not worth waiting for in a test
79+
monkeypatch.setattr(core_mod, "sleep", lambda *a, **kw: None)
6580

66-
def _make(missing_titles=(), normalized=None):
67-
mw_site = _FakeMwSite(missing_titles, normalized)
81+
def _make(missing_titles=(), normalized=None, missing_once=(), fails=False):
82+
mw_site = _FakeMwSite(missing_titles, normalized, missing_once, fails)
6883
return OSW.construct(site=_FakeSite(mw_site)), mw_site
6984

7085
return _make
@@ -137,6 +152,42 @@ def test_verification_is_skipped_offline(offline_osw):
137152
assert set(result.pages.keys()) == {title}
138153

139154

155+
def test_a_missing_page_is_confirmed_by_a_second_query(offline_osw):
156+
item = model.Item(label=[model.Label(text="Ghost")])
157+
title = _title(item)
158+
osw_obj, mw_site = offline_osw(missing_titles=[title])
159+
160+
with pytest.raises(OSW.StoreEntityPartialError):
161+
osw_obj.store_entity(OSW.StoreEntityParam(entities=[item], parallel=False))
162+
163+
assert mw_site.queries == [[title], [title]]
164+
165+
166+
def test_a_page_that_appears_on_the_second_query_is_not_reported(offline_osw):
167+
"""A read answered by a lagging database replica must not fail the store."""
168+
items = [model.Item(label=[model.Label(text=f"Lag{i}")]) for i in range(2)]
169+
titles = [_title(it) for it in items]
170+
osw_obj, mw_site = offline_osw(missing_once=[titles[0]])
171+
172+
result = osw_obj.store_entity(OSW.StoreEntityParam(entities=items, parallel=False))
173+
174+
assert set(result.pages.keys()) == set(titles)
175+
assert result.failed == {}
176+
# the second query asks only for the title the first one reported as missing
177+
assert mw_site.queries[1] == [titles[0]]
178+
179+
180+
def test_a_failing_query_keeps_the_pages_and_does_not_raise(offline_osw):
181+
items = [model.Item(label=[model.Label(text=f"Unverified{i}")]) for i in range(2)]
182+
titles = [_title(it) for it in items]
183+
osw_obj, _mw_site = offline_osw(fails=True)
184+
185+
result = osw_obj.store_entity(OSW.StoreEntityParam(entities=items, parallel=False))
186+
187+
assert set(result.pages.keys()) == set(titles)
188+
assert result.failed == {}
189+
190+
140191
def test_titles_are_queried_in_batches_of_fifty(offline_osw):
141192
osw_obj, mw_site = offline_osw()
142193
titles = [f"Item:OSW{i:04d}" for i in range(120)]

0 commit comments

Comments
 (0)