Stop an all-owned campaign posting its own links as fallbacks - #205
Merged
Merged
Conversation
Found in production minutes after the sources migration landed. The "lives" campaign — 39 hand-pasted owned links, no content sources — posted six links and flagged every one of them via_fallback. maxFallbackItemsPerDay is 3, so the following tick would have selected nothing and the campaign would have gone silent with no error anywhere. The migration backfills every existing list with the 70/30 default mix. A campaign with no shared content then reads that as "30% short on shared", finds no shared inventory, and covers with owned content — which is, by the letter of the policy, a fallback. It is not one. A class the campaign has no inventory and no source for is not starved; it is simply not part of that campaign's mix. So the mix is now narrowed to the classes a campaign can actually supply: a class survives if it has a link ready or an enabled source feeding it. An all-owned campaign ends up with a 100%-owned effective mix and posts on target forever, exactly as it did before sources existed. The moment the user adds a keyword source, shared re-enters the mix and the ratio starts applying — and then covering for an empty shared queue genuinely is a fallback, and the daily cap genuinely should bound it. Four existing blend tests encoded the old assumption that an empty class is always starved. They now pass hasSource explicitly, which is what those scenarios always meant. Production data was repaired alongside this: lists with no sources were set to a 100%-owned mix, and the six mislabelled posts had via_fallback cleared so the rolling cap stopped counting them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ThreatCrush Security Scan35 finding(s) HIGH/CRITICAL: 3 | MEDIUM: 23 | LOW: 9
Snippets are redacted; ThreatCrush never prints matched credential material. |
ralyodio
added a commit
that referenced
this pull request
Aug 19, 2026
* docs(promote): reconcile the architecture doc with #205 and #206 The architecture doc was written in #204 and hasn't moved since, but two PRs landed under it the same day and both changed things it describes as settled. #205 added effectiveMix(): the ownership mix is narrowed to the classes a campaign can actually supply before any deficit is computed. §4.1 still described the raw configured mix, which is the version that mislabelled six production posts as via_fallback and was three posts from silencing the campaign. Someone reading §4 to extend the blend would have rebuilt the bug, so the rule and the reason it exists are now in the doc rather than only in the PR that fixed it. §4.2 gets the corresponding scope limit: covering for a class the campaign never had is not a fallback and is not capped as one. #206 added lib/sp/accountHealth.ts, which partly satisfies §14's "pause campaigns after repeated provider rejections" — at the account layer, not the campaign layer. Noted as half-built, because the Reddit provider will still need the campaign-level version: a subreddit rejection is a destination fact, not an account one. Docs only; no behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Make a Promote publication happen at most once (#AC10, #AC14) The sweep decided and published in one pass, "claiming" a campaign by pushing next_run_at forward. That UPDATE carried no predicate on next_run_at, so it was a read-then-write: two sweeps that both read the row both won it. And the worker runs the sweep on a 60s interval *and* out-of-band whenever someone clicks "Post now", so overlapping runs are designed in, not rare. Nothing downstream was idempotent either. If the process died after postViaAccount() published but before the promo_post insert, there was no record it had happened — and last_promoted_at is only stamped at the end of the campaign, so the same link was still least-recently-promoted on the next tick and went out again. The user sees a duplicate; the logs show nothing. promo_job makes the intended publication the unit of work, written down before anything is sent. Two mechanisms: - Plan before publishing. Jobs are keyed on sha256(list, link, account, destination, kind, slot), where the slot is the next_run_at value the sweep observed as due. A racing sweep reads the same due row, derives the same keys, loses to the unique index, and gets nothing back — so it publishes nothing. Keying on the wall clock instead would give each sweep its own key and rebuild the bug. - Claim by compare-and-swap: update ... where id = ? and state = 'queued'. Read and write are one statement, so two workers cannot both see 'queued'. The campaign-level claim keeps its place but now carries its predicate. It is an optimization — it avoids duplicated work. The guarantee is in the job. AT MOST ONCE, ON PURPOSE. A job still 'publishing' past a 10 minute lease is failed, never retried. No provider we publish through accepts an idempotency key, so an interrupted publish has genuinely unknown outcome — it may be live. Re-running it is the duplicate this exists to prevent. The reaper closes it with the outcome recorded as unknown and leaves it in history for a human; the credit is not refunded, because refunding a post that did land is the other way to be wrong. Retry stays available for failures that provably happened before the publish call, which is what 'retrying' and attempt_count are for. Stray jobs are closed rather than left queued: nothing reclaims a queued job, so one stranded by a disconnected account or a credit pause would sit there forever misreporting the campaign as backed up. MIGRATION FIRST. planJobs() logs loudly and publishes nothing if promo_job is missing, rather than failing the select and stopping every campaign in silence the way source_mix did. That is a guard, not the fix — apply 20260819120000_promote_jobs.sql before this deploys. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Claim a campaign on "still due" rather than an exact timestamp match The claim predicate compared next_run_at to the exact string we read back. That is correct only if a timestamptz round-trips to a byte-identical value through PostgREST, and if it ever did not, the update would match nothing, no campaign would be claimed, and Promote would stop posting entirely with nothing in the logs that looks like a failure. That is the same silent-stop shape the source_mix migration cost us a day for, and it is not worth risking to save a predicate. Re-asserting the condition the select already used gives the same atomicity with none of that exposure: the minimum cadence is 300s, so whoever wins the claim pushes next_run_at well into the future and the loser's `lte` cannot match. The scheduling slot the idempotency key is built from is still the observed next_run_at, so racing sweeps still agree on the slot. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(promote): match §9.2 to the claim predicate that shipped Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(promote): key promo_job.user_id to profiles, like every other promote table promo_list.user_id references public.profiles(id). Pointing promo_job at auth.users instead would let a job outlive the profile row the rest of the feature is keyed to, and would cascade differently from its own campaign. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #204, found in production minutes after the migration landed.
What went wrong
The "lives" campaign (39 hand-pasted owned links, no content sources) posted 6 links at 19:14Z and flagged every one
via_fallback.maxFallbackItemsPerDayis 3, so the next tick would have selected nothing and the campaign would have gone silent — no error, no paused status, just no posts.Cause: the migration backfills every existing list with the 70/30 default mix. A campaign with no shared content reads that as "30% short on shared", finds none, and covers with owned content — which the policy scores as a fallback. It isn't one.
The fix
A class the campaign has no inventory and no source for is not starved — it's not part of that campaign's mix.
effectiveMix()narrows the mix to classes that have a link ready or an enabled source feeding them.Production already repaired
Not waiting on this merge — the data was fixed at 19:16Z:
via_fallbackcleared so the rolling cap stopped counting them.Confirmed the 19:29Z tick would proceed normally.
Tests
selectNextLinkreproducing the exact production shape (50 owned posts, 1 owned link, 0 sources).hasSourceexplicitly, which is what those scenarios always meant.vitest run: 1638 passed / 0 failed.tsc --noEmitclean.🤖 Generated with Claude Code