Tested automation poll enqueueing - #29026
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe AutomationsService poll re-arming mechanism changes from a synchronous enqueue-now callback to an async enqueue-at callback. ChangesSuggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Caller
participant AutomationsService
participant SchedulerAdapter
Caller->>AutomationsService: rescheduleAll()
AutomationsService->>AutomationsService: await `#enqueuePollAt`(date)
AutomationsService->>SchedulerAdapter: schedule(date)
SchedulerAdapter-->>AutomationsService: scheduled
AutomationsService-->>Caller: Promise resolved
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
8856a82 to
fbd3897
Compare
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run ghost:test:ci:integration |
✅ Succeeded | 2m 49s | View ↗ |
nx run ghost:test:legacy |
✅ Succeeded | 3m 3s | View ↗ |
nx run ghost:test:integration |
✅ Succeeded | 2m 33s | View ↗ |
nx run-many --target=build --projects=tag:publi... |
✅ Succeeded | <1s | View ↗ |
nx run-many -t test:unit -p ghost |
✅ Succeeded | 30s | View ↗ |
nx run ghost:test:e2e |
✅ Succeeded | 2m 15s | View ↗ |
nx run @tryghost/admin:build |
✅ Succeeded | 11s | View ↗ |
nx run ghost:build:assets |
✅ Succeeded | 2s | View ↗ |
Additional runs (2) |
✅ Succeeded | ... | View ↗ |
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗
☁️ Nx Cloud last updated this comment at 2026-07-06 15:39:10 UTC
There was a problem hiding this comment.
Pull request overview
Adds unit coverage for the automations poll enqueueing path (past/now vs. future scheduling), and adjusts the automations rescheduler behavior to be awaitable so tests can reliably observe event dispatch timing. This supports the broader effort to make automation polling idempotent (NY-1396) by exercising previously untested scheduling logic.
Changes:
- Use
node:timers/promisessetImmediate()in unit tests to reliably flush the macrotask queue. - Add tests covering enqueueing behavior for past/now (dispatch in-process poll) vs. future (schedule via scheduler adapter).
- Update
AutomationsService.rescheduleAll()to be async and add a test-only enqueue helper.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| ghost/core/test/unit/server/services/automations/service.test.js | Adds unit tests for poll enqueueing behavior and updates existing tests to await macrotask flushing. |
| ghost/core/core/server/services/automations/service.ts | Makes rescheduling awaitable and exposes a test-only hook to exercise enqueue logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Notably, `rescheduleAll` now returns a promise. That's okay--we `await` the result anyway. See `SchedulingBase.prototype.rescheduleAll`.
fbd3897 to
654a178
Compare
|
Bugbot is not enabled for this team, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![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/test/unit/server/services/automations/service.test.js (1)
78-87: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value"date is now" test relies on real wall-clock timing.
Using
new Date()directly ties the "now" boundary test to actual elapsed time between construction and the internal now-vs-future comparison inside__testOnlyEnqueuePollAt. This is inherently racy at the exact millisecond boundary; the "past" test already exercises the dispatch-not-schedule path, so this test's specific value (verifying the boundary itself) is undermined by not controlling time deterministically (e.g., viasinon.useFakeTimers).🤖 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/unit/server/services/automations/service.test.js` around lines 78 - 87, The StartAutomationsPollEvent boundary test in automations service is relying on the real clock via new Date(), which makes it racy. Update the test around __testOnlyEnqueuePollAt to control time deterministically, preferably with sinon.useFakeTimers or a fixed timestamp, so the “date is now” case is evaluated against a stable mocked now. Keep the existing assertions on domainEvents.dispatch and schedulerAdapter.schedule, but ensure the test setup explicitly freezes time before invoking __testOnlyEnqueuePollAt.
🤖 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/test/unit/server/services/automations/service.test.js`:
- Around line 78-87: The StartAutomationsPollEvent boundary test in automations
service is relying on the real clock via new Date(), which makes it racy. Update
the test around __testOnlyEnqueuePollAt to control time deterministically,
preferably with sinon.useFakeTimers or a fixed timestamp, so the “date is now”
case is evaluated against a stable mocked now. Keep the existing assertions on
domainEvents.dispatch and schedulerAdapter.schedule, but ensure the test setup
explicitly freezes time before invoking __testOnlyEnqueuePollAt.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: db28f623-d169-4aec-a4a6-6dc5756dad86
📒 Files selected for processing (2)
ghost/core/core/server/services/automations/service.tsghost/core/test/unit/server/services/automations/service.test.js
|
Bugbot is not enabled for this team, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Bugbot is not enabled for this team, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
closes https://linear.app/ghost/issue/NY-1405 **Depends on #29026.** ## What In addition to telling the scheduler when to do an automation poll, also use an in-memory timer. This change should have no user impact. ## Why 1. [Soon][NY-1396], we plan to reduce the precision of requests to the scheduler. Instead of telling the scheduler *exactly* when to poll, we'll round to the nearest 30 minutes (with some per-site jitter). This will reduce load on the scheduler and will boot Pro servers less. 1. If the scheduler service is down, polls will still happen at the right time if the server is up. [NY-1396]: https://linear.app/ghost/issue/NY-1396/make-automations-poll-arming-idempotent-in-scheduler
towards https://linear.app/ghost/issue/NY-1396
I recommend reviewing this commit-by-commit.
This change should have no user impact.
As part of our work to make automation polling idempotent, I'd like to test a previously-untested part of the code.