mirror of
https://github.com/Second-Hand-Friends/kleinanzeigen-bot.git
synced 2026-07-09 20:51:05 +02:00
fix: fail closed on persistence errors (#1203)
## ℹ️ Description - Link to the related issue(s): No linked issue; follows the confirmed P1 from `.slim/deepwork/p1-p2-validation.md`. - Prevent publish/update workflows from reporting success or running after-publish cleanup when local YAML persistence fails after a remote submit succeeds. ## 📋 Changes Summary - Add a non-retryable `PostPublishPersistenceError` carrying the remote ad ID. - Raise that error after logging post-submit local persistence failures. - Count persistence failures as failed publish/update items and skip post-submit confirmation / `AFTER_PUBLISH` cleanup. - Add tests for direct persistence failure, publish cleanup-skip behavior, and update non-retry/batch continuation. - Add German translations for the new warnings. ### ⚙️ Type of Change - [x] 🐞 Bug fix (non-breaking change which fixes an issue) - [ ] ✨ New feature (adds new functionality without breaking existing usage) - [ ] 💥 Breaking change (changes that might break existing user setups, scripts, or configurations) ## ✅ Checklist Before requesting a review, confirm the following: - [x] I have reviewed my changes to ensure they meet the project's standards. - [x] I have tested my changes and ensured that all tests pass (`pdm run test`). - [x] I have formatted the code (`pdm run format`). - [x] I have verified that linting passes (`pdm run lint`). - [x] I have updated documentation where necessary. By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved handling of post-publish save failures so they’re surfaced as a clear error instead of being silently logged. * Publishing and updating now stop retrying when local persistence fails after a successful remote action, while still logging details and capturing diagnostics. * Other retry behavior for existing publish issues remains unchanged. * **Tests** * Added and updated coverage for persistence failures, including error propagation, logging, and workflow behavior across multiple items. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This commit is contained in:
@@ -40,6 +40,16 @@ LOG = _loggers.get_logger(__name__)
|
||||
SUBMISSION_MAX_RETRIES:Final[int] = 3
|
||||
|
||||
|
||||
class PostPublishPersistenceError(RuntimeError):
|
||||
"""Raised when local persistence fails after successful remote publish/update."""
|
||||
|
||||
def __init__(self, *, ad_id:int | None, ad_title:str, original:Exception) -> None:
|
||||
self.ad_id = ad_id
|
||||
self.ad_title = ad_title
|
||||
super().__init__(f"Post-publish persistence failed for '{ad_title}' (ad ID {ad_id})")
|
||||
self.original = original
|
||||
|
||||
|
||||
async def check_publishing_result(web:WebScrapingMixin) -> bool:
|
||||
"""Check for publishing success messages (checking-done or not-completed)."""
|
||||
return await web.web_check(By.ID, "checking-done", Is.DISPLAYED) or await web.web_check(By.ID, "not-completed", Is.DISPLAYED)
|
||||
@@ -164,12 +174,13 @@ async def publish_ad(
|
||||
ad_file, ad_cfg, ad_cfg_orig, old_ad_id, ad_id, mode,
|
||||
config = config,
|
||||
)
|
||||
except Exception:
|
||||
except Exception as ex:
|
||||
LOG.error( # noqa: G201 — must use .error(exc_info=True) for translation lookup
|
||||
"Post-publish persistence failed for '%s' (ad ID %s - ad is live on "
|
||||
"Kleinanzeigen but local YAML may be out of sync)",
|
||||
ad_cfg.title, ad_id, exc_info = True,
|
||||
)
|
||||
raise PostPublishPersistenceError(ad_id = ad_id, ad_title = ad_cfg.title, original = ex) from ex
|
||||
|
||||
|
||||
async def publish_ads(
|
||||
@@ -260,6 +271,16 @@ async def publish_ads(
|
||||
)
|
||||
failed_count += 1
|
||||
break
|
||||
except PostPublishPersistenceError as ex:
|
||||
if capture_diagnostics:
|
||||
await capture_diagnostics(ad_cfg, ad_cfg_orig, ad_file, attempt, ex)
|
||||
LOG.warning(
|
||||
"Persistence failed for '%s' after ad submission. Ad ID: %s. "
|
||||
"No retry performed. If the ad is online, sync local state manually.",
|
||||
ad_cfg.title, ex.ad_id,
|
||||
)
|
||||
failed_count += 1
|
||||
break
|
||||
except (TimeoutError, ProtocolException) as ex:
|
||||
if capture_diagnostics:
|
||||
await capture_diagnostics(ad_cfg, ad_cfg_orig, ad_file, attempt, ex)
|
||||
@@ -394,6 +415,16 @@ async def update_ads(
|
||||
)
|
||||
failed_count += 1
|
||||
break
|
||||
except PostPublishPersistenceError as ex:
|
||||
if capture_diagnostics:
|
||||
await capture_diagnostics(ad_cfg, ad_cfg_orig, ad_file, attempt, ex)
|
||||
LOG.warning(
|
||||
"Persistence failed for '%s' after ad update submission. Ad ID: %s. "
|
||||
"No retry performed. If the ad is online, sync local state manually.",
|
||||
ad_cfg.title, ex.ad_id,
|
||||
)
|
||||
failed_count += 1
|
||||
break
|
||||
except CategoryResolutionError as ex:
|
||||
if capture_diagnostics:
|
||||
await capture_diagnostics(ad_cfg, ad_cfg_orig, ad_file, attempt, ex)
|
||||
|
||||
@@ -194,6 +194,8 @@ kleinanzeigen_bot/publishing_workflow.py:
|
||||
"Manual recovery required for '%s'. Check 'Meine Anzeigen' to confirm whether the ad was posted.": "Manuelle Wiederherstellung für '%s' erforderlich. Prüfen Sie in 'Meine Anzeigen', ob die Anzeige veröffentlicht wurde."
|
||||
? "If posted, sync local state with 'kleinanzeigen-bot download --ads=new' or 'kleinanzeigen-bot download --ads=<id>'; otherwise rerun publish for this ad."
|
||||
: "Falls veröffentlicht, lokalen Stand mit 'kleinanzeigen-bot download --ads=new' oder 'kleinanzeigen-bot download --ads=<id>' synchronisieren; andernfalls Veröffentlichung für diese Anzeige erneut starten."
|
||||
? "Persistence failed for '%s' after ad submission. Ad ID: %s. No retry performed. If the ad is online, sync local state manually."
|
||||
: "Persistenz für '%s' nach dem Absenden der Anzeige fehlgeschlagen. Anzeigen-ID: %s. Kein erneuter Versuch ausgeführt. Falls die Anzeige online ist, lokalen Stand manuell synchronisieren."
|
||||
"All %s attempts failed for '%s': %s. Skipping ad.": "Alle %s Versuche fehlgeschlagen für '%s': %s. Überspringe Anzeige."
|
||||
"Category resolution failed for '%s': %s. Skipping ad (configuration error, no retry).": "Kategorieauflösung fehlgeschlagen für '%s': %s. Anzeige wird übersprungen (Konfigurationsfehler, kein erneuter Versuch)."
|
||||
"DONE: (Re-)published %s (%s failed after retries)": "FERTIG: %s (erneut) veröffentlicht (%s fehlgeschlagen nach Wiederholungen)"
|
||||
@@ -207,6 +209,8 @@ kleinanzeigen_bot/publishing_workflow.py:
|
||||
"Attempt %s/%s for '%s' reached submit boundary but failed: %s. Not retrying to prevent duplicate modifications.": "Versuch %s/%s für '%s' hat die Sendegrenze erreicht, ist aber fehlgeschlagen: %s. Kein weiterer Versuch, um doppelte Änderungen zu vermeiden."
|
||||
"Category resolution failed for '%s': %s. Skipping ad (configuration error, no retry).": "Kategorieauflösung fehlgeschlagen für '%s': %s. Anzeige wird übersprungen (Konfigurationsfehler, kein erneuter Versuch)."
|
||||
"Manual recovery required for '%s'. Check 'Meine Anzeigen' to confirm whether the update was applied.": "Manuelle Nachbearbeitung für '%s' erforderlich. Bitte unter 'Meine Anzeigen' prüfen, ob die Aktualisierung übernommen wurde."
|
||||
? "Persistence failed for '%s' after ad update submission. Ad ID: %s. No retry performed. If the ad is online, sync local state manually."
|
||||
: "Persistenz für '%s' nach dem Absenden der Anzeigenaktualisierung fehlgeschlagen. Anzeigen-ID: %s. Kein erneuter Versuch ausgeführt. Falls die Anzeige online ist, lokalen Stand manuell synchronisieren."
|
||||
"All %s attempts failed for '%s': %s. Skipping ad.": "Alle %s Versuche für '%s' sind fehlgeschlagen: %s. Anzeige wird übersprungen."
|
||||
"Attempt %s/%s failed for '%s': %s. Retrying...": "Versuch %s/%s für '%s' ist fehlgeschlagen: %s. Erneuter Versuch..."
|
||||
" -> Could not confirm update for '%s', but changes may be online": " -> Aktualisierung für '%s' konnte nicht bestätigt werden, aber Änderungen könnten online sein"
|
||||
|
||||
@@ -17,6 +17,7 @@ from kleinanzeigen_bot.app import KleinanzeigenBot
|
||||
from kleinanzeigen_bot.local_path_renaming import ImageRenameResult, LocalPathRenameResult, RenameStatus
|
||||
from kleinanzeigen_bot.model.ad_model import Ad, AdUpdateStrategy
|
||||
from kleinanzeigen_bot.model.config_model import Config
|
||||
from kleinanzeigen_bot.publishing_workflow import PostPublishPersistenceError
|
||||
|
||||
|
||||
def _make_rename_result(*, renamed:bool = False, blocked:bool = False, id_mismatch:bool = True) -> LocalPathRenameResult:
|
||||
@@ -283,10 +284,15 @@ class TestPersistPublishedAdSaveRollback:
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_publish_ad_survives_persistence_failure() -> None:
|
||||
"""When persist_published_ad raises, publish_ad logs and does not propagate."""
|
||||
async def test_publish_ad_raises_post_publish_persistence_error(caplog:pytest.LogCaptureFixture) -> None:
|
||||
"""When persist_published_ad fails, publish_ad raises a post-submit persistence error."""
|
||||
ad = _make_min_ad()
|
||||
ad_cfg_orig:dict[str, Any] = {"title": "Test Ad Title", "description": "Test description for the ad listing.", "type": "OFFER", "category": "160"}
|
||||
ad_cfg_orig:dict[str, Any] = {
|
||||
"title": "Test Ad Title",
|
||||
"description": "Test description for the ad listing.",
|
||||
"type": "OFFER",
|
||||
"category": "160",
|
||||
}
|
||||
cfg = _make_config()
|
||||
bot = KleinanzeigenBot()
|
||||
bot.browser = MagicMock()
|
||||
@@ -301,6 +307,21 @@ async def test_publish_ad_survives_persistence_failure() -> None:
|
||||
patch("kleinanzeigen_bot.publishing_submission.submit_and_confirm_ad", new_callable = AsyncMock, return_value = 12345),
|
||||
patch("kleinanzeigen_bot.publishing_persistence.persist_published_ad",
|
||||
side_effect = RuntimeError("disk full")),
|
||||
patch("kleinanzeigen_bot.utils.misc.now"),
|
||||
caplog.at_level("ERROR"),
|
||||
pytest.raises(
|
||||
PostPublishPersistenceError,
|
||||
match = r"Post-publish persistence failed for 'Test Ad Title' \(ad ID 12345\)",
|
||||
) as exc_info,
|
||||
):
|
||||
# Must not raise — the try/except in publish_ad swallows the error
|
||||
await bot.publish_ad("test.yaml", ad, ad_cfg_orig, [], AdUpdateStrategy.REPLACE)
|
||||
|
||||
assert exc_info.value.ad_id == 12345
|
||||
assert isinstance(exc_info.value.__cause__, RuntimeError)
|
||||
assert str(exc_info.value.__cause__) == "disk full"
|
||||
|
||||
diagnostics = [
|
||||
record for record in caplog.records
|
||||
if "Post-publish persistence failed for 'Test Ad Title'" in record.getMessage()
|
||||
]
|
||||
assert diagnostics
|
||||
|
||||
@@ -24,7 +24,7 @@ from kleinanzeigen_bot.model.config_model import (
|
||||
AutoPriceReductionConfig,
|
||||
DiagnosticsConfig,
|
||||
)
|
||||
from kleinanzeigen_bot.publishing_workflow import SUBMISSION_MAX_RETRIES
|
||||
from kleinanzeigen_bot.publishing_workflow import SUBMISSION_MAX_RETRIES, PostPublishPersistenceError
|
||||
from kleinanzeigen_bot.utils.exceptions import CategoryResolutionError, PublishSubmissionUncertainError
|
||||
from tests.conftest import build_published_ads, build_update_ad
|
||||
|
||||
@@ -347,6 +347,107 @@ class TestKleinanzeigenBotPublishAdsBasics:
|
||||
assert publish_mock.await_count == 1
|
||||
sleep_mock.assert_not_awaited()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_publish_ads_treats_post_publish_persistence_error_as_fail_closed(
|
||||
self,
|
||||
test_bot:KleinanzeigenBot,
|
||||
base_ad_config:dict[str, Any],
|
||||
mock_page:MagicMock,
|
||||
caplog:pytest.LogCaptureFixture,
|
||||
) -> None:
|
||||
test_bot.page = mock_page
|
||||
test_bot.config.publishing.delete_old_ads = "AFTER_PUBLISH"
|
||||
test_bot.keep_old_ads = False
|
||||
|
||||
ad_cfg = Ad.model_validate(base_ad_config)
|
||||
ad_cfg_orig = copy.deepcopy(base_ad_config)
|
||||
ad_file = "ad.yaml"
|
||||
|
||||
with (
|
||||
patch.object(
|
||||
test_bot,
|
||||
"web_request",
|
||||
new_callable = AsyncMock,
|
||||
return_value = {"content": json.dumps({"ads": [], "paging": {"pageNum": 1, "last": 1}})},
|
||||
),
|
||||
patch(
|
||||
"kleinanzeigen_bot.publishing_workflow.publish_ad",
|
||||
new_callable = AsyncMock,
|
||||
side_effect = PostPublishPersistenceError(
|
||||
ad_id = 12345,
|
||||
ad_title = ad_cfg.title,
|
||||
original = RuntimeError("disk full"),
|
||||
),
|
||||
) as publish_ad_mock,
|
||||
patch.object(
|
||||
test_bot,
|
||||
"_capture_publish_error_diagnostics_if_enabled",
|
||||
new_callable = AsyncMock,
|
||||
) as capture_mock,
|
||||
patch.object(test_bot, "web_sleep", new_callable = AsyncMock) as sleep_mock,
|
||||
patch.object(test_bot, "web_await", new_callable = AsyncMock) as web_await_mock,
|
||||
patch("kleinanzeigen_bot.delete_flow.delete_ad", new_callable = AsyncMock) as delete_ad_mock,
|
||||
caplog.at_level("INFO"),
|
||||
):
|
||||
await test_bot.publish_ads([(ad_file, ad_cfg, ad_cfg_orig)])
|
||||
|
||||
assert web_await_mock.await_count == 0
|
||||
assert sleep_mock.await_count == 0
|
||||
assert publish_ad_mock.await_count == 1
|
||||
assert delete_ad_mock.await_count == 0
|
||||
capture_mock.assert_awaited_once()
|
||||
|
||||
assert any("DONE: (Re-)published 0 ads (1 failed after retries)" in record.getMessage() for record in caplog.records)
|
||||
assert any("Persistence failed for 'Test Title' after ad submission" in record.getMessage() for record in caplog.records)
|
||||
assert any("Ad ID: 12345" in record.getMessage() for record in caplog.records)
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_update_ads_persistence_failure_is_not_retried_and_still_processes_next_ad(
|
||||
self,
|
||||
test_bot:KleinanzeigenBot,
|
||||
base_ad_config:dict[str, Any],
|
||||
) -> None:
|
||||
ad_one = build_update_ad(base_ad_config, 401, "Persistence Error Update")
|
||||
ad_two = build_update_ad(base_ad_config, 402, "Healthy Update")
|
||||
|
||||
async def publish_side_effect(
|
||||
_web:Any,
|
||||
_ad_file:str,
|
||||
ad_cfg:Ad,
|
||||
_ad_cfg_orig:dict[str, Any],
|
||||
_published_ads:list[dict[str, Any]],
|
||||
_mode:AdUpdateStrategy,
|
||||
**kwargs:Any,
|
||||
) -> None:
|
||||
if ad_cfg.id == 401:
|
||||
raise PostPublishPersistenceError(
|
||||
ad_id = 401,
|
||||
ad_title = ad_cfg.title,
|
||||
original = RuntimeError("disk full"),
|
||||
)
|
||||
|
||||
with (
|
||||
patch(
|
||||
"kleinanzeigen_bot.published_ads.fetch_published_ads",
|
||||
new_callable = AsyncMock,
|
||||
return_value = build_published_ads((401, "active"), (402, "active")),
|
||||
),
|
||||
patch(
|
||||
"kleinanzeigen_bot.publishing_workflow.publish_ad",
|
||||
new_callable = AsyncMock,
|
||||
side_effect = publish_side_effect,
|
||||
) as publish_mock,
|
||||
patch.object(test_bot, "_capture_publish_error_diagnostics_if_enabled", new_callable = AsyncMock) as capture_mock,
|
||||
patch.object(test_bot, "web_sleep", new_callable = AsyncMock) as sleep_mock,
|
||||
patch.object(test_bot, "web_await", new_callable = AsyncMock, return_value = True) as web_await_mock,
|
||||
):
|
||||
await test_bot.update_ads([ad_one, ad_two])
|
||||
|
||||
assert publish_mock.await_count == 2
|
||||
sleep_mock.assert_not_awaited()
|
||||
web_await_mock.assert_awaited_once()
|
||||
capture_mock.assert_awaited_once()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.parametrize(
|
||||
("mode", "expected_path"),
|
||||
|
||||
Reference in New Issue
Block a user