From ffd5bb501f51822536ea00ec8bfe0e3d17912c98 Mon Sep 17 00:00:00 2001 From: AFFAANh Date: Fri, 7 Aug 2026 15:33:39 +0530 Subject: [PATCH] fix(campaigns): delete the campaign when the chain fails after creating it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first real connection test left a live campaign in the ad account. sync_campaign assigns meta_campaign_id on the spec it was handed before it creates the ad set, so when the ad set was rejected the exception escaped with `synced` unbound and `created` still empty — and the cleanup keyed on exactly those two. Nothing was deleted, and nothing said so. The reconciler then found the unknown campaign and adopted it, which is why /discover reported zero: by the time anyone looked, the orphan was a known record. A leak that hides itself. Cleanup now falls back to the spec's own meta_campaign_id. Two tests: a mid-chain failure deletes the campaign, and a failure before any campaign exists still issues no delete at all. Co-Authored-By: Claude Opus 5 --- src/adclaw/campaigns/diagnostics.py | 12 ++++++-- tests/test_campaign_diagnostics.py | 46 +++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 2 deletions(-) diff --git a/src/adclaw/campaigns/diagnostics.py b/src/adclaw/campaigns/diagnostics.py index 903345a..d44385c 100644 --- a/src/adclaw/campaigns/diagnostics.py +++ b/src/adclaw/campaigns/diagnostics.py @@ -366,8 +366,16 @@ async def run_smoke_test(meta, ad_account_id: str, actor: str) -> dict[str, Any] except Exception as exc: steps.append({"step": "create", "status": FAILED, "detail": str(exc)}) finally: - target = (synced.meta_campaign_id if synced else None) or created.get( - "campaign_id", + # `campaign` last, and it is the one that matters: sync_campaign + # assigns meta_campaign_id on the spec it was handed *before* it + # creates the ad set, so a mid-chain failure leaves the campaign + # live in the account while `synced` is still None and `created` is + # still empty. Without this fallback that campaign is abandoned — + # which is exactly what happened on the first real run. + target = ( + (synced.meta_campaign_id if synced else None) + or created.get("campaign_id") + or campaign.meta_campaign_id ) if target: try: diff --git a/tests/test_campaign_diagnostics.py b/tests/test_campaign_diagnostics.py index 181e0b5..8b79760 100644 --- a/tests/test_campaign_diagnostics.py +++ b/tests/test_campaign_diagnostics.py @@ -418,3 +418,49 @@ async def test_no_token_blocks_the_connection_test(monkeypatch): assert report["can_smoke_test"] is False assert report["smoke_test_blocking"] == ["Meta access token"] + + +# --- a half-built chain must not be abandoned --- + + +async def test_a_failure_after_the_campaign_exists_still_deletes_it(page): + """The real failure mode, reproduced. + + sync_campaign assigns meta_campaign_id on the spec before it creates + the ad set. When the ad set is rejected, `synced` is never bound and + `created` is never populated — so a cleanup keyed only on those two + leaves a live campaign sitting in the ad account. On the first real run + that is exactly what happened, and the reconciler then adopted the + orphan into the database. + """ + + class AdSetRejects(SmokeMeta): + async def create_ad_set(self, ad_account_id, **kwargs): + raise MetaError( + "Invalid parameter", + code=100, + subcode=2490487, + user_message="Bid amount or bid constraints required.", + ) + + meta = AdSetRejects() + + result = await run_smoke_test(meta, "act_1", actor="farman") + + assert result["passed"] is False + # The campaign was created, so it must have been deleted again. + assert meta.deleted == ["camp_1"] + cleanup = next(s for s in result["steps"] if s["step"] == "clean up") + assert cleanup["status"] == OK + assert "camp_1" in cleanup["detail"] + + +async def test_nothing_is_deleted_when_the_campaign_was_never_created(page): + """No campaign id means no cleanup call — not a delete of None.""" + meta = SmokeMeta() + meta.fail_on = "campaign" + + result = await run_smoke_test(meta, "act_1", actor="farman") + + assert meta.deleted == [] + assert not [s for s in result["steps"] if s["step"] == "clean up"]