From 005680f234493dfbaafc845a99a94514e42c91b1 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 11 Sep 2026 21:50:28 -0400 Subject: [PATCH] fix: one source edit no longer wipes the campaign id and the backfill position Source.config_overrides carries two unrelated things under one column: the operator's per-source download settings, and state FC writes for itself. update() treated the whole column as operator-owned and assigned it wholesale, so a dialog save discarded patreon_campaign_id and the entire #693 backfill state machine. Not a hand-edited-JSON edge case: SourceFormDialog's structured tab rebuilds the object from two fields, so saving without touching anything was enough. _merged_config now merges: the operator's keys replace wholesale (removing a key must still remove it), FC's keys survive and are applied LAST so a stale echoed cursor cannot roll a walk backwards. App-managed is _-prefixed or *_campaign_id, both matching data already on disk, so no migration. Preserving the id exposed a bug the wipe was MASKING: nothing cleared it when a source's URL changed, and patreon_resolver reads that cache before attempting any lookup. A repointed source would have resolved the old creator forever and 387 C4 would have reported a confident wrong match. So update() drops *_campaign_id when the URL actually changes - but keeps the backfill cursor, which the walk's own stall guard validates and which is expensive to rebuild. test_update_changes_fields was DOCUMENTING the bug: it asserted config_overrides == {videos: False} on a source whose create() had armed _backfill_state, so it could only pass because the state had been destroyed. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01LNXXULQDjVZmbuNa2G9mD9 --- backend/app/services/source_service.py | 72 ++++++++++++- tests/test_source_service.py | 143 ++++++++++++++++++++++++- 2 files changed, 213 insertions(+), 2 deletions(-) diff --git a/backend/app/services/source_service.py b/backend/app/services/source_service.py index 51a2a6b..e4c6172 100644 --- a/backend/app/services/source_service.py +++ b/backend/app/services/source_service.py @@ -123,6 +123,24 @@ class SourceRecord: _EDITABLE = {"enabled", "url", "config_overrides", "check_interval_override", "platform"} +# `config_overrides` carries two unrelated things under one column: the +# operator's per-source download settings, and state FC writes for ITSELF. An +# operator edit replaces the first wholesale — removing a key has to be able to +# remove it — but it must never take the second with it. +# +# Two families, both matching data already on disk: +# `_*` the #693 backfill state machine (_backfill_state, _cursor, +# _cursor_stalls, _chunks, _posts, _bypass_seen, _recapture) +# `*_campaign_id` the resolved platform identity cache, written by +# `download_service._phase3_persist` +_CAMPAIGN_ID_SUFFIX = "_campaign_id" + + +def _is_app_managed(key: str) -> bool: + """Is this a key FC maintains, rather than one the operator edits?""" + return key.startswith("_") or key.endswith(_CAMPAIGN_ID_SUFFIX) + + # Plan #693: backfill safety cap. "Start backfill" (and a newly created # enabled source) arms a run-until-done walk; this caps how many time-boxed # chunks it may spend before pausing as "stalled", so a pathological walk that @@ -165,6 +183,30 @@ class SourceService: raise InvalidConfigError("config_overrides must be a JSON object") return config + @staticmethod + def _merged_config(source: Source, incoming: dict | None) -> dict | None: + """The operator's keys replace wholesale; FC's own keys survive. + + Without this, one edit in the Subscriptions dialog silently discarded + the resolved campaign id AND the entire backfill position — the dialog + posts the whole object back (`SourceFormDialog`), so anything absent + from its JSON box was simply gone. + + FC's keys are applied LAST so they win: the dialog round-trips whatever + it last read, and a client echoing a stale `_backfill_state` must not be + able to overwrite what the walk has since written. + """ + managed = { + k: v for k, v in (source.config_overrides or {}).items() + if _is_app_managed(k) + } + if incoming is None: + # An explicit null clears the operator's settings. It is not a + # request to forget where a backfill had got to. + return managed or None + operator = {k: v for k, v in incoming.items() if not _is_app_managed(k)} + return {**operator, **managed} + async def _load_settings(self) -> ImportSettings: return await ImportSettings.load(self.session) @@ -349,10 +391,38 @@ class SourceService: if "url" in fields: fields["url"] = self._validate_url(fields["url"]) if "config_overrides" in fields: - fields["config_overrides"] = self._validate_config(fields["config_overrides"]) + fields["config_overrides"] = self._merged_config( + source, self._validate_config(fields["config_overrides"]) + ) + + # Computed BEFORE the setattr loop, while `source.url` is still the old + # one. See the invalidation below. + url_changed = "url" in fields and fields["url"] != source.url for key, value in fields.items(): setattr(source, key, value) + + if url_changed: + # Repointing a source at a different creator makes a cached campaign + # id WRONG, not merely stale, and `patreon_resolver` consults that + # cache BEFORE attempting any lookup — so a kept id would resolve the + # old creator forever, and the membership join (#387 C4) would report + # a confident wrong match. + # + # This needs saying explicitly only because of the merge above: until + # then the wholesale overwrite wiped the id as an accident of the + # bug, which masked this. Preserving the id makes the invalidation + # this service's job. + # + # The backfill cursor is deliberately NOT cleared. It is opaque + # platform state the walk already validates with its own stall + # guard, and dropping it would restart a long backfill over a + # cosmetic URL edit (http->https, adding `/c/`). + co = dict(source.config_overrides or {}) + for key in [k for k in co if k.endswith(_CAMPAIGN_ID_SUFFIX)]: + co.pop(key) + source.config_overrides = co + # Disabling a source clears its failure state (operator: disable the subs # you're not paying for without them lingering as "failing"). Re-enabling # then starts clean; the next real run re-derives health. Only on the diff --git a/tests/test_source_service.py b/tests/test_source_service.py index e88b75d..09ffdd2 100644 --- a/tests/test_source_service.py +++ b/tests/test_source_service.py @@ -189,7 +189,148 @@ async def test_update_changes_fields(db): ) updated = await svc.update(rec.id, enabled=False, config_overrides={"videos": False}) assert updated.enabled is False - assert updated.config_overrides == {"videos": False} + # The operator's key lands, and FC's own state is not collateral. This + # assertion used to read `== {"videos": False}` — it was DOCUMENTING the bug + # the tests below now pin: `create` arms `_backfill_state` (#693), and every + # edit was silently discarding it. + assert updated.config_overrides["videos"] is False + assert updated.config_overrides["_backfill_state"] == "running" + + +# --- config_overrides: the operator's keys and FC's own share one column --- +# +# `SourceFormDialog` posts the WHOLE object back, and its structured tab rebuilds +# that object from two fields — so saving the dialog without touching anything +# used to discard the resolved campaign id and the backfill position. The id +# matters twice over: `patreon_resolver` reads it BEFORE attempting any lookup, +# and the membership join (#387 C4, snippet #3947) keys on it. + + +async def _source_with_state(db, svc, artist, **overrides): + """A source carrying both kinds of key: FC's own state and the operator's.""" + rec = await svc.create( + artist_id=artist.id, platform="patreon", url="https://patreon.com/mixed", + ) + await db.execute( + Source.__table__.update().where(Source.id == rec.id).values( + config_overrides={ + "_backfill_state": "running", + "_backfill_cursor": "03:RESUME", + "patreon_campaign_id": "camp-1", + "videos": True, + **overrides, + }, + ) + ) + await db.commit() + return rec + + +@pytest.mark.asyncio +async def test_an_operator_edit_keeps_fc_managed_state(db): + """THE bug. One dialog save used to cost the campaign id and the walk.""" + artist = await _artist(db) + svc = SourceService(db) + rec = await _source_with_state(db, svc, artist) + + updated = await svc.update(rec.id, config_overrides={"videos": False}) + + assert updated.config_overrides["patreon_campaign_id"] == "camp-1" + assert updated.config_overrides["_backfill_state"] == "running" + assert updated.config_overrides["_backfill_cursor"] == "03:RESUME" + assert updated.config_overrides["videos"] is False + + +@pytest.mark.asyncio +async def test_an_operator_key_left_out_of_the_edit_is_still_removed(db): + """The other half, and why this is a merge rather than a blanket keep: the + operator's own settings must still replace wholesale, or removing a key in + the dialog could never remove it.""" + artist = await _artist(db) + svc = SourceService(db) + rec = await _source_with_state(db, svc, artist, since="2026-01-01") + + updated = await svc.update(rec.id, config_overrides={"videos": False}) + + assert "since" not in updated.config_overrides + + +@pytest.mark.asyncio +async def test_a_null_config_clears_operator_settings_but_not_fc_state(db): + """`_parseConfig` sends null for an empty object, so this is the dialog's + real "I cleared the box" path — not a request to forget the walk.""" + artist = await _artist(db) + svc = SourceService(db) + rec = await _source_with_state(db, svc, artist) + + updated = await svc.update(rec.id, config_overrides=None) + + assert "videos" not in updated.config_overrides + assert updated.config_overrides["_backfill_state"] == "running" + assert updated.config_overrides["patreon_campaign_id"] == "camp-1" + + +@pytest.mark.asyncio +async def test_a_stale_echoed_value_cannot_overwrite_what_the_walk_wrote(db): + """The dialog round-trips whatever it last READ. If the backfill advanced in + the meantime, saving that stale copy must not roll the walk backwards — so + FC's keys are applied last and win.""" + artist = await _artist(db) + svc = SourceService(db) + rec = await _source_with_state(db, svc, artist) + + updated = await svc.update(rec.id, config_overrides={ + "videos": False, + "_backfill_cursor": "00:STALE", + "patreon_campaign_id": "wrong-campaign", + }) + + assert updated.config_overrides["_backfill_cursor"] == "03:RESUME" + assert updated.config_overrides["patreon_campaign_id"] == "camp-1" + + +@pytest.mark.asyncio +async def test_repointing_the_url_drops_the_resolved_campaign_id(db): + """A different creator behind the same row makes the cached id WRONG, not + stale. Preserving it above is exactly what makes this invalidation this + service's job — the old wholesale overwrite used to wipe the id by accident. + """ + artist = await _artist(db) + svc = SourceService(db) + rec = await _source_with_state(db, svc, artist) + + updated = await svc.update(rec.id, url="https://patreon.com/someone-else") + + assert "patreon_campaign_id" not in updated.config_overrides + # The walk's position survives: it is expensive to rebuild, the walk has its + # own stall guard, and a cosmetic URL edit must not restart a long backfill. + assert updated.config_overrides["_backfill_cursor"] == "03:RESUME" + + +@pytest.mark.asyncio +async def test_rewriting_the_same_url_keeps_the_campaign_id(db): + """The dialog posts the URL on every save. An unchanged URL is not a + repoint, and treating it as one would re-resolve after every edit.""" + artist = await _artist(db) + svc = SourceService(db) + rec = await _source_with_state(db, svc, artist) + + updated = await svc.update(rec.id, url="https://patreon.com/mixed") + + assert updated.config_overrides["patreon_campaign_id"] == "camp-1" + + +def test_the_campaign_id_suffix_agrees_with_the_membership_join(): + """Two modules must agree on the key shape: this one preserves and + invalidates it, `membership_roster` matches on it (snippet #3947). They + deliberately do not import across — a core CRUD service should not depend on + a membership feature — so the agreement is pinned here rather than hoped for. + Do NOT fix a failure by editing one side; decide which shape is right. + """ + from backend.app.services.membership_roster import _CAMPAIGN_KEY_SUFFIX + from backend.app.services.source_service import _CAMPAIGN_ID_SUFFIX + + assert _CAMPAIGN_ID_SUFFIX == _CAMPAIGN_KEY_SUFFIX @pytest.mark.asyncio