Repository navigation
Make a Promote publication happen at most once - #207
Merged
Merged
Conversation
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>
ThreatCrush Security Scan35 finding(s) HIGH/CRITICAL: 3 | MEDIUM: 23 | LOW: 9
Snippets are redacted; ThreatCrush never prints matched credential material. |
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>
ralyodio
changed the title
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>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…mote 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>
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.
Closes acceptance criteria 10 and 14 in
docs/promote-engine-architecture.md.The bug
The sweep decided and published in one pass, "claiming" a campaign by pushing
next_run_atforward. That UPDATE carried no predicate onnext_run_at, so it was a read-then-write — two sweeps that both read the row both won it. The worker runs the sweep on a 60s interval and out-of-band whenever someone clicks "Post now" (POST /dashboard/promote/sweep), so overlapping runs are designed in, not rare.Nothing downstream was idempotent either. A crash after
postViaAccount()published but before thepromo_postinsert left no record it happened, andlast_promoted_atis 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.The fix
promo_jobmakes the intended publication the unit of work, written down before anything is sent.Plan before publishing. Jobs are keyed on
sha256(list, link, account, destination, kind, slot), where the slot is thenext_run_atvalue 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 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 observequeued.The campaign-level claim keeps its place and now carries a predicate too — it re-asserts the same "still due" condition the select used (
.lte("next_run_at", dueBy)), so once the winner has pushed the campaign forward the loser matches nothing. It re-asserts the condition rather than matching the exact timestamp read back: a predicate that silently never matched would stop every campaign posting with nothing in the logs. Either way it is an optimization that avoids duplicated work — the guarantee is in the job.At most once, on purpose
A job still
publishingpast 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 deliberately not refunded: refunding a post that did land is the other way to be wrong, and support can refund from history.Retry stays available for failures that provably happened before the publish call — that is what the
retryingstate andattempt_countare for.promo_job.user_idreferencespublic.profiles(id), matchingpromo_list— notauth.users, which would let a job outlive the profile row the rest of the feature is keyed to and cascade differently from its own campaign.Deploy order — migration first
planJobs()logs loudly and publishes nothing ifpromo_jobis missing, rather than failing the select and stopping every campaign in silence the waysource_mixdid in #204. That is a guard, not the fix: applysupabase/migrations/20260819120000_promote_jobs.sqlbefore this deploys.Also here
The first commit reconciles the architecture doc with #205 and #206, which both landed under it the same day:
effectiveMix(). Anyone reading it to extend the blend would have rebuilt the bug that mislabelled six production posts asvia_fallback.lib/sp/accountHealth.tsmakes it half-built, at the account layer.Verification
npm run typecheck— cleannpm test— 1667 passed, 7 skipped (132 files), including 19 new tests intests/promote/jobs.test.tscovering key stability, the racing-sweep collapse, single-winner claim, and the reaper refusing to requeue or to overwrite a slow worker that finished firstnext lintis broken at HEAD under Next 16 and is not in CI; not touched here🤖 Generated with Claude Code