PublishNextArticleJob blocks on the first candidate — iterate to find a publishable one #133

New issue

Open

opened 2026-08-10 23:04:53 +02:00 by myrmidex · 0 comments

myrmidex commented

2026-08-10 23:04:53 +02:00

Owner

Summary

PublishNextArticleJob selects the single oldest eligible RouteArticle and publishes it. If that article cannot be published (skipped as duplicate, channel has no account, channel is throttled), the job ends. On the next tick the same article is still the oldest — and if it remains unpublishable, every other channel stalls behind it indefinitely.

Scenarios that trigger blocking

1. Skipped-as-duplicate (in the code today)

When ArticlePublishingService returns PublishOutcome::skipped(), PublishRouteArticleAction::recordSkipped() sets publish_status = SKIPPED but does not call recordPublishAttemptFailed(). So publish_attempts stays 0 and next_attempt_at stays null — scopeDueForPublishing() still considers the row eligible. Every tick picks it, skips it, and returns. Every other route article is permanently blocked.

2. Channel with no active account (no backoff today)

ArticlePublishingService::publishRouteArticle() returns PublishOutcome::failure('No active account for channel'). This triggers backoff (5 → 30 → 120 → 360 min), so the article drops out of eligibility temporarily. After 4 attempts hasExhaustedPublishAttempts() returns true and it is permanently excluded — silently dead, no notification. Blocking is temporary but the final state is unreported.

3. Per-channel publish interval (future — #130)

When #130 adds per-channel interval throttling, a channel inside its interval must be skipped over. The current single-candidate selection cannot do this.

Design

Replace the single ->first() with an iteration loop over the N oldest candidates, publishing the first one that succeeds and skipping over failures.

for each candidate (oldest first, up to a sensible limit):
    attempt publish
    if succeeded → return (one published, done for this tick)
    if terminal failure → skip to next candidate
    if transient failure → backoff applies, skip to next candidate

Limit

A limit(N) with a small N (e.g., 10) prevents the loop from scanning the entire table if all candidates are blocked. 10 candidates × 5 min ticks = 50 minutes before the loop exhausts, which is plenty of time for transient issues to resolve or for a human to notice.

PublishOutcome semantics

The loop needs to distinguish three outcomes:

Outcome	Action
`succeeded()`	One published — return. Done for this tick.
`wasSkipped()`	Permanent skip (duplicate). Mark the `RouteArticle` as resolved so it never blocks the queue again. Continue to next candidate.
`failed()`	Transient failure (no account, Lemmy down). Backoff already applied by `recordPublishAttemptFailed()`. Continue to next candidate.

Marking skipped rows as terminal

A skipped-as-duplicate RouteArticle should not be retried indefinitely. Options:

A: Set publish_status = SKIPPED and publish_attempts = MAX so dueForPublishing() excludes it permanently. Simplest.
B: Add a publish_status = TERMINAL_SKIPPED enum value distinct from SKIPPED (which today means "skipped this attempt, try again"). More precise but adds a migration.

Recommend A — SKIPPED already means "this will never publish" in practice (it is a duplicate), so making it terminal is correct. The existing SKIPPED tests would need updating to assert the terminal state.

Daily cap interaction

The daily cap check at the top of handle() gates on total publications today. The loop publishes at most one article, so the cap still applies — no change needed. The cap gates the entire job, not per-candidate.

Tasks

Replace ->first() + single publish with a capped iteration loop
Make skipped-as-duplicate terminal: set publish_attempts = MAX so dueForPublishing() excludes it
Failed candidates: backoff works as before; loop continues to next candidate
When all candidates in the batch are blocked, the job returns without error — this is normal, not a failure
Log each skipped/failed candidate at INFO so the sequence is traceable
Tests:
- Two eligible candidates, first is a duplicate → second publishes
- All candidates blocked → job returns cleanly
- Published-at-least-one guard: only one publication per tick even with multiple eligible candidates
- Skipped status is terminal (not re-picked on subsequent ticks)
- Failed status with backoff: not picked while next_attempt_at is in the future, then retried
- Existing publish tests still pass

Edge cases

Loop limit exhausted: all N candidates blocked. Job returns without publishing. This is not an error — the next tick will try the next N.
Mid-loop approval: a new RouteArticle is approved while the loop runs. It won't be in the fetched batch and will be picked on the next tick. Acceptable.
Mid-loop ShouldBeUnique: the job holds the unique lock for uniqueFor = 300. The loop takes < 1s, well within the lock window.
Existing ShouldBeUnique early-return bug: if the loop returns without publishing (all candidates blocked), the unique lock still releases normally — ShouldBeUnique releases on handle() completion, not on side effects.

Acceptance criteria

A skipped-as-duplicate candidate does not block the next candidate in the same tick
A skipped-as-duplicate candidate is not re-picked on subsequent ticks
A failed candidate (with backoff) does not block the next candidate in the same tick
At most one article is published per job run (daily cap semantics unchanged)
The job returns cleanly when all candidates in the batch are blocked
The loop is bounded — no unbounded scanning
Existing publish tests still pass

#123 — concurrent publish race; the duplicate-skip path this ticket makes terminal was added as the backstop there
#119 — scopeDueForPublishing is the single source of publish eligibility; this ticket's terminal-skip relies on it
#130 — per-channel publish interval; depends on this iteration loop to skip throttled channels
#131 — dead-letter monitoring; the "no active account → silent dead" path would be caught by that ticket
.claude/PLATFORM.md — head-of-line blocking is already documented as a known risk

## Summary `PublishNextArticleJob` selects the **single oldest** eligible `RouteArticle` and publishes it. If that article cannot be published (skipped as duplicate, channel has no account, channel is throttled), the job ends. On the next tick the same article is still the oldest — and if it remains unpublishable, every other channel stalls behind it indefinitely. ## Scenarios that trigger blocking ### 1. Skipped-as-duplicate (in the code today) When `ArticlePublishingService` returns `PublishOutcome::skipped()`, `PublishRouteArticleAction::recordSkipped()` sets `publish_status = SKIPPED` but does **not** call `recordPublishAttemptFailed()`. So `publish_attempts` stays 0 and `next_attempt_at` stays null — `scopeDueForPublishing()` still considers the row eligible. Every tick picks it, skips it, and returns. Every other route article is permanently blocked. ### 2. Channel with no active account (no backoff today) `ArticlePublishingService::publishRouteArticle()` returns `PublishOutcome::failure('No active account for channel')`. This triggers backoff (5 → 30 → 120 → 360 min), so the article drops out of eligibility temporarily. After 4 attempts `hasExhaustedPublishAttempts()` returns true and it is permanently excluded — silently dead, no notification. Blocking is temporary but the final state is unreported. ### 3. Per-channel publish interval (future — #130) When #130 adds per-channel interval throttling, a channel inside its interval must be skipped over. The current single-candidate selection cannot do this. ## Design Replace the single `->first()` with an iteration loop over the N oldest candidates, publishing the first one that succeeds and skipping over failures. ``` for each candidate (oldest first, up to a sensible limit): attempt publish if succeeded → return (one published, done for this tick) if terminal failure → skip to next candidate if transient failure → backoff applies, skip to next candidate ``` ### Limit A `limit(N)` with a small N (e.g., 10) prevents the loop from scanning the entire table if all candidates are blocked. 10 candidates × 5 min ticks = 50 minutes before the loop exhausts, which is plenty of time for transient issues to resolve or for a human to notice. ### PublishOutcome semantics The loop needs to distinguish three outcomes: | Outcome | Action | |---------|--------| | `succeeded()` | One published — return. Done for this tick. | | `wasSkipped()` | Permanent skip (duplicate). Mark the `RouteArticle` as resolved so it never blocks the queue again. Continue to next candidate. | | `failed()` | Transient failure (no account, Lemmy down). Backoff already applied by `recordPublishAttemptFailed()`. Continue to next candidate. | ### Marking skipped rows as terminal A skipped-as-duplicate `RouteArticle` should not be retried indefinitely. Options: - **A**: Set `publish_status = SKIPPED` and `publish_attempts = MAX` so `dueForPublishing()` excludes it permanently. Simplest. - **B**: Add a `publish_status = TERMINAL_SKIPPED` enum value distinct from `SKIPPED` (which today means "skipped this attempt, try again"). More precise but adds a migration. **Recommend A** — `SKIPPED` already means "this will never publish" in practice (it is a duplicate), so making it terminal is correct. The existing `SKIPPED` tests would need updating to assert the terminal state. ### Daily cap interaction The daily cap check at the top of `handle()` gates on total publications today. The loop publishes at most one article, so the cap still applies — no change needed. The cap gates the entire job, not per-candidate. ## Tasks - [ ] Replace `->first()` + single publish with a capped iteration loop - [ ] Make skipped-as-duplicate terminal: set `publish_attempts = MAX` so `dueForPublishing()` excludes it - [ ] Failed candidates: backoff works as before; loop continues to next candidate - [ ] When all candidates in the batch are blocked, the job returns without error — this is normal, not a failure - [ ] Log each skipped/failed candidate at `INFO` so the sequence is traceable - [ ] Tests: - Two eligible candidates, first is a duplicate → second publishes - All candidates blocked → job returns cleanly - Published-at-least-one guard: only one publication per tick even with multiple eligible candidates - Skipped status is terminal (not re-picked on subsequent ticks) - Failed status with backoff: not picked while `next_attempt_at` is in the future, then retried - Existing publish tests still pass ## Edge cases - **Loop limit exhausted**: all N candidates blocked. Job returns without publishing. This is not an error — the next tick will try the next N. - **Mid-loop approval**: a new `RouteArticle` is approved while the loop runs. It won't be in the fetched batch and will be picked on the next tick. Acceptable. - **Mid-loop `ShouldBeUnique`**: the job holds the unique lock for `uniqueFor = 300`. The loop takes < 1s, well within the lock window. - **Existing `ShouldBeUnique` early-return bug**: if the loop returns without publishing (all candidates blocked), the unique lock still releases normally — `ShouldBeUnique` releases on `handle()` completion, not on side effects. ## Acceptance criteria - [ ] A skipped-as-duplicate candidate does not block the next candidate in the same tick - [ ] A skipped-as-duplicate candidate is not re-picked on subsequent ticks - [ ] A failed candidate (with backoff) does not block the next candidate in the same tick - [ ] At most one article is published per job run (daily cap semantics unchanged) - [ ] The job returns cleanly when all candidates in the batch are blocked - [ ] The loop is bounded — no unbounded scanning - [ ] Existing publish tests still pass ## Related - #123 — concurrent publish race; the duplicate-skip path this ticket makes terminal was added as the backstop there - #119 — `scopeDueForPublishing` is the single source of publish eligibility; this ticket's terminal-skip relies on it - #130 — per-channel publish interval; depends on this iteration loop to skip throttled channels - #131 — dead-letter monitoring; the "no active account → silent dead" path would be caught by that ticket - `.claude/PLATFORM.md` — head-of-line blocking is already documented as a known risk