fix(campaigns): delete the campaign when the chain fails after creating it
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 <noreply@anthropic.com>
This commit is contained in:
@@ -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:
|
||||
|
||||
@@ -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"]
|
||||
|
||||
Reference in New Issue
Block a user