🐛 Fixed email analytics job timestamp not updating on recurrence

[9larsons]

no ref

The email analytics job would not set the latest event timestamp when doing back-to-back processing of batches (ie during high volume events). If a site got rebooted during this period, it would necessitate processing all of those events again, unnecessarily.

[coderabbitai]

Walkthrough

The EmailAnalyticsService's event fetching finalization logic has been modified to simplify the condition for advancing the lastEventTimestamp. The eventCount < maxEvents upper bound check has been removed from the condition, leaving checks for error absence, event existence, and timestamp currency (>2 seconds behind current time). Additionally, an else branch now explicitly sets the job status to 'finished' when the finalization condition is not met, whereas previously this case had no explicit status update.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• The removal of the eventCount < maxEvents bound requires understanding the intent and potential behavioral impact on event fetching loops
• The new else branch introduces explicit status management that changes the job lifecycle; the implications of marking jobs as 'finished' in the new condition path need validation
• Consider whether removing the upper bound could lead to extended event processing or unexpected job state transitions
• Verify the interaction between the 2-second timing logic and the removed event count constraint

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: fixing a bug where the email analytics job timestamp doesn't update during recurring/back-to-back batch processing.
Description check	✅ Passed	The description is directly related to the changeset, explaining the bug fix and its rationale regarding timestamp updates during batch processing.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix-email-job-timestamp

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 113f6b5 and 3914b86.

📒 Files selected for processing (1)

• ghost/core/core/server/services/email-analytics/EmailAnalyticsService.js (1 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

ghost/core/core/server/**/*.{js,ts}

📄 CodeRabbit inference engine (AGENTS.md)

Core Ghost backend logic belongs in ghost/core/core/server/, with database schema in ghost/core/core/server/data/schema/, API routes in ghost/core/core/server/api/, services in ghost/core/core/server/services/, and models in ghost/core/core/server/models/

Files:

• ghost/core/core/server/services/email-analytics/EmailAnalyticsService.js

🧠 Learnings (3)

📓 Common learnings

Learnt from: troyciesco
Repo: TryGhost/Ghost PR: 25288
File: ghost/core/core/server/services/member-welcome-emails/jobs/lib/process-entries.js:46-64
Timestamp: 2025-11-10T23:10:17.470Z
Learning: In ghost/core/core/server/services/member-welcome-emails/jobs/lib/process-entries.js and the outbox processing flow, entries are marked as PROCESSING before being processed. If a failure occurs after email send but before deletion, the entry remains stuck in PROCESSING state (not reprocessed). This intentional design prevents duplicate emails. Handling stuck PROCESSING entries is planned for a separate PR.

📚 Learning: 2025-10-30T17:13:26.190Z

Learnt from: sam-lord
Repo: TryGhost/Ghost PR: 25303
File: ghost/core/core/server/services/email-service/BatchSendingService.js:19-19
Timestamp: 2025-10-30T17:13:26.190Z
Learning: In ghost/core/core/server/services/email-service/BatchSendingService.js and similar files in the Ghost codebase, prefer using `{...options}` spread syntax without explicit guards like `...(options || {})` when spreading potentially undefined objects, as the maintainer prefers cleaner syntax over defensive patterns when the behavior is safe.

Applied to files:

• ghost/core/core/server/services/email-analytics/EmailAnalyticsService.js

📚 Learning: 2025-11-10T23:10:17.470Z

Learnt from: troyciesco
Repo: TryGhost/Ghost PR: 25288
File: ghost/core/core/server/services/member-welcome-emails/jobs/lib/process-entries.js:46-64
Timestamp: 2025-11-10T23:10:17.470Z
Learning: In ghost/core/core/server/services/member-welcome-emails/jobs/lib/process-entries.js and the outbox processing flow, entries are marked as PROCESSING before being processed. If a failure occurs after email send but before deletion, the entry remains stuck in PROCESSING state (not reprocessed). This intentional design prevents duplicate emails. Handling stuck PROCESSING entries is planned for a separate PR.

Applied to files:

• ghost/core/core/server/services/email-analytics/EmailAnalyticsService.js

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)

• GitHub Check: Unit tests (Node 22.13.1)
• GitHub Check: Lint
• GitHub Check: Ghost-CLI tests
• GitHub Check: Acceptance tests (Node 22.13.1, sqlite3)
• GitHub Check: Legacy tests (Node 22.13.1, sqlite3)
• GitHub Check: Acceptance tests (Node 22.13.1, mysql8)
• GitHub Check: Legacy tests (Node 22.13.1, mysql8)
• GitHub Check: Build & Push

🔇 Additional comments (2)

ghost/core/core/server/services/email-analytics/EmailAnalyticsService.js (2)

454-458: LGTM! Correctly fixes timestamp advancement during high-volume processing.

Removing the eventCount < maxEvents condition allows the timestamp to advance even when hitting the batch limit during high-volume events. This directly addresses the PR objective: preventing unnecessary reprocessing after a reboot by ensuring progress is maintained during back-to-back batch processing.

---

459-462: Verify the explicit status update is correct for all scenarios.

The new else branch marks the job as 'finished' when the timestamp isn't advanced. This occurs when:

• No events were processed (eventCount === 0)
• No lastEventTimestamp exists yet
• The timestamp is too recent (< 2 seconds old)

Since line 454 checks !error, error cases won't reach this else branch (they'll throw at line 467 instead). The explicit status update improves job tracking, but verify this aligns with your status management expectations—particularly for the "no events" and "timestamp too recent" scenarios where the job completes successfully but makes no progress.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}