Fixed scheduler retrying publish jobs when there is nothing to publish - #29306
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughScheduler publishing now returns a no-op result when a job fires before its scheduled time or the resource no longer exists. The schedules endpoint handles missing resources with empty responses, centralizes cache-invalidation headers, and continues to propagate non- Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run ghost:test:ci:integration |
✅ Succeeded | 2m 52s | View ↗ |
nx run ghost:test:integration |
✅ Succeeded | 2m 58s | View ↗ |
nx run ghost:test:legacy |
✅ Succeeded | 3m 12s | View ↗ |
nx run ghost:test:e2e |
✅ Succeeded | 2m 37s | View ↗ |
nx run @tryghost/admin:build |
✅ Succeeded | 2m 23s | View ↗ |
nx run-many -t test:unit -p ghost |
✅ Succeeded | 31s | View ↗ |
nx run ghost-monorepo:lint:boundaries |
✅ Succeeded | 18s | View ↗ |
nx run-many -t lint -p ghost,ghost-monorepo |
✅ Succeeded | 22s | View ↗ |
nx run-many --target=build --projects=tag:publi... |
✅ Succeeded | <1s | View ↗ |
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗
☁️ Nx Cloud last updated this comment at 2026-07-16 10:04:02 UTC
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ghost/core/core/server/services/posts/post-scheduling.js (1)
34-51: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSolid fix, but the TOCTOU window between
read()andedit()isn't covered.The
try/catchonly wraps the initialapi[resourceType].read(...)call. If the resource is deleted (or unscheduled) in the window between this read and theedit()call further down (line 65),edit()can itself throw aNotFoundErrorthat is not caught here and will propagate as an error — reintroducing exactly the noisy-retry scenario this PR is meant to eliminate, just with a narrower window.Consider extending the no-op conversion to the
edit()call as well (e.g. wrap both calls, or wrap the whole publish attempt).♻️ Suggested approach
const editedResource = {}; editedResource[resourceType] = [{ status: 'published', updated_at: moment(preScheduledResource.updated_at).toISOString(true) }]; - const editResult = await api[resourceType].edit( - editedResource, - _.pick(options, ['context', 'id', 'transacting', 'forUpdate']) - ); + let editResult; + try { + editResult = await api[resourceType].edit( + editedResource, + _.pick(options, ['context', 'id', 'transacting', 'forUpdate']) + ); + } catch (err) { + if (errors.utils.isGhostError(err) && err.errorType === 'NotFoundError') { + return NO_OP; + } + throw err; + } const scheduledResource = editResult[resourceType][0];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ghost/core/core/server/services/posts/post-scheduling.js` around lines 34 - 51, Extend the NotFoundError-to-NO_OP handling in the post-scheduling flow to include the later api[resourceType].edit() call, covering resources deleted or unscheduled after the initial read. Wrap the read-and-edit publish attempt in the existing error handling while preserving propagation of all non-NotFoundError failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@ghost/core/core/server/services/posts/post-scheduling.js`:
- Around line 34-51: Extend the NotFoundError-to-NO_OP handling in the
post-scheduling flow to include the later api[resourceType].edit() call,
covering resources deleted or unscheduled after the initial read. Wrap the
read-and-edit publish attempt in the existing error handling while preserving
propagation of all non-NotFoundError failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c7389256-de70-4f5a-a434-2c238b5f2ddf
📒 Files selected for processing (3)
ghost/core/core/server/api/endpoints/schedules.jsghost/core/core/server/services/posts/post-scheduling.jsghost/core/test/legacy/api/admin/schedules.test.js
756efc4 to
c2cc976
Compare
allouis
changed the title
c2cc976 to
5f023b3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ghost/core/test/legacy/api/admin/schedules.test.js (1)
155-173: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for deleted and unscheduled posts.
These tests cover early and late scheduler timing, but the PR objective also requires deleted and no-longer-scheduled posts to return
200with an emptypostsarray.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ghost/core/test/legacy/api/admin/schedules.test.js` around lines 155 - 173, Add tests in the schedule firing suite alongside the existing timing cases for deleted posts and posts that are no longer scheduled. Exercise the same PUT endpoint and assert a 200 response with a JSON body whose posts array is empty, preserving the existing response-header assertions where applicable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ghost/core/core/server/services/posts/post-scheduling.js`:
- Around line 12-16: Update the scheduling flow around api[resourceType].read()
to catch NotFoundError and return the existing NO_OP tuple when the scheduled
resource was deleted or is no longer scheduled. Preserve propagation of all
other errors and leave normal resource handling unchanged.
---
Nitpick comments:
In `@ghost/core/test/legacy/api/admin/schedules.test.js`:
- Around line 155-173: Add tests in the schedule firing suite alongside the
existing timing cases for deleted posts and posts that are no longer scheduled.
Exercise the same PUT endpoint and assert a 200 response with a JSON body whose
posts array is empty, preserving the existing response-header assertions where
applicable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c177ed22-13bf-4790-9176-670938dbc472
📒 Files selected for processing (3)
ghost/core/core/server/api/endpoints/schedules.jsghost/core/core/server/services/posts/post-scheduling.jsghost/core/test/legacy/api/admin/schedules.test.js
5f023b3 to
57a1bcd
Compare
allouis
changed the title
57a1bcd to
7a4177b
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #29306 +/- ##
==========================================
+ Coverage 73.76% 73.97% +0.20%
==========================================
Files 1552 1572 +20
Lines 134208 137437 +3229
Branches 16112 16607 +495
==========================================
+ Hits 99004 101663 +2659
- Misses 34195 34741 +546
- Partials 1009 1033 +24
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
295c3b9 to
1e29463
Compare
The schedules publish endpoint returned a 404 when a job fired ahead of the scheduled time (because the post had been rescheduled to a later time) or when the post or page had since been deleted. External schedulers treat any non-2xx response as a failure and retry it, so these benign no-ops caused retry storms and noisy error logs that drown out real failures. Both cases now return a 2xx no-op so the job is treated as done. Firing well after the scheduled time without a force flag still errors, since that is a genuinely dropped publish worth surfacing. The deleted case needs a custom permissions handler, because the permission stage loads the resource and 404s a missing one before the controller runs. It swallows only NotFoundError, so permission enforcement on resources that still exist is unchanged.
1e29463 to
11eb31b
Compare
1 participant
Ghost's scheduler pings the publish endpoint (
PUT /schedules/:resource/:id) when a scheduled post or page is due. Previously this endpoint returned a 404 in two situations where there is genuinely nothing to publish: when it fires ahead of the scheduled time, and when the target resource no longer exists. Schedulers treat any non-2xx response as a failure and retry it — Ghost's production remote scheduler retries any non-2xx up to a limit — so each of these benign cases turned into a retry storm plus error logs that drown out real failures.This changes both cases to a 2xx no-op so the job is treated as done:
NotFound.permissionshandler on the endpoint. The handler runs the normalpost:publishcheck but tolerates a missing resource, letting the controller return an empty list.Firing well after the scheduled time without a
forceflag still errors — that's a genuinely dropped publish worth surfacing, and its behavior is unchanged.Security
The custom permissions handler swallows only
NotFoundError(resource missing).NoPermissionErrorand every other error still propagate, so permission enforcement on resources that DO exist is unchanged: a caller withoutpost:publishhitting an existing post still gets a 403 (covered by a regression test). No new information is exposed either — an authenticated caller could already distinguish missing (404) from unauthorized-but-existing (403) today. This only changes the missing case's status to 200, and it remains a pure no-op with no side effects.Testing
Regression tests were added and updated in the legacy schedules API suite: fired-ahead returns 200 with an empty body; a deleted resource returns 200 with an empty body; past-without-force still returns 404; and the existing "no access" test (non-scheduler key on an existing post → 403) guards that the permissions handler only swallows
NotFound, never a permission error.