fix: one source edit no longer wipes the campaign id and the backfill position
Build images / sign-extension (push) Successful in 2s
Build images / build-agent (push) Successful in 6s
CI / lint (push) Successful in 1s
CI / extension-version (push) Successful in 2s
CI / backend-lint-and-test (push) Successful in 39s
CI / frontend-build (push) Successful in 28s
Build images / build-web (push) Successful in 1m10s
Build images / smoke-web (push) Skipped
Build images / build-ml (push) Successful in 3m4s
Build images / promote (push) Skipped
CI / integration (push) Successful in 2m22s
Build images / sign-extension (push) Successful in 2s
Build images / build-agent (push) Successful in 6s
CI / lint (push) Successful in 1s
CI / extension-version (push) Successful in 2s
CI / backend-lint-and-test (push) Successful in 39s
CI / frontend-build (push) Successful in 28s
Build images / build-web (push) Successful in 1m10s
Build images / smoke-web (push) Skipped
Build images / build-ml (push) Successful in 3m4s
Build images / promote (push) Skipped
CI / integration (push) Successful in 2m22s
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LNXXULQDjVZmbuNa2G9mD9
This commit is contained in:
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user