Refactored internal scheduler integration caching for rotation#27849
Merged
Conversation
Contributor
|
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:
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 (26)
💤 Files with no reviewable changes (4)
🚧 Files skipped from review as they are similar to previous changes (21)
WalkthroughThe PR replaces the frontend-data service with a new internal-keys AutoFillingMap cache and makes frontend key retrieval asynchronous via the frontend proxy. Boot no longer creates/returns a dataService and now calls initFrontend() with no arguments. Scheduler signing and scheduling use resolved internal API keys from internalKeys.get instead of full integration objects; post-scheduling, gifts, automations, scheduling utils, and Integration.getApiKeyBySlug were updated accordingly. Tests and helper suites were modified to use the internal-keys cache. Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
rob-ghost
force-pushed
the
chore/internal-api-key-caching
branch
9 times, most recently
from
5bde1c5 to
2827cdb
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ghost/core/core/boot.js (1)
355-397:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMove
stripe.init()into the service initializationPromise.allto parallelize boot time.The sequential
await stripe.init()before thePromise.alladds unnecessary latency to boot time. Members, tiers, and donations services do not depend on stripe initialization completing first—members only reads stripe's static configuration properties (which exist immediately afterrequire), while tiers and donations have no stripe dependencies at all. Movestripe.init()into the Promise.all to parallelize it with the other service inits.🤖 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/boot.js` around lines 355 - 397, The code currently awaits stripe.init() before calling Promise.all, which serializes startup; remove the standalone await stripe.init() and add stripe.init() as one of the promises inside the existing Promise.all array so stripe initialization runs in parallel with identityTokens.init(), members.init(), tiers.init(), giftService.init({...}), new AutomationsService().init({...}) and the other service inits; keep createSchedulerAdapter() called prior to building the array so schedulerAdapter can still be passed into postScheduling.init and giftService.init.
🧹 Nitpick comments (3)
ghost/core/core/server/services/post-scheduling/index.js (1)
34-46: 💤 Low valueOptional: parallelize the prebuild loop.
buildJobis async and the loop awaits each call serially. With many scheduled posts/pages on boot, this serializes N×2 awaits. SinceinternalKeysmemoizes after the first call, the bulk of the latency is justurlUtils/signing work, but aPromise.all/flatMapwould still cut wall time linearly. Only worth doing if boot time on large sites is a concern.♻️ Suggested rewrite
-const resolvePreScheduled = async function (apiUrl) { - const scheduled = await loadScheduledResources(); - const preScheduled = []; - for (const resourceType of Object.keys(scheduled)) { - for (const model of scheduled[resourceType]) { - preScheduled.push({ - unscheduleJob: await buildJob({apiUrl, resourceType, model}, 'unscheduled'), - scheduleJob: await buildJob({apiUrl, resourceType, model}) - }); - } - } - return preScheduled; -}; +const resolvePreScheduled = async function (apiUrl) { + const scheduled = await loadScheduledResources(); + const tasks = Object.entries(scheduled).flatMap(([resourceType, models]) => + models.map(async (model) => ({ + unscheduleJob: await buildJob({apiUrl, resourceType, model}, 'unscheduled'), + scheduleJob: await buildJob({apiUrl, resourceType, model}) + })) + ); + return Promise.all(tasks); +};🤖 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/post-scheduling/index.js` around lines 34 - 46, The resolvePreScheduled function currently builds jobs serially by awaiting buildJob inside nested loops; refactor it to parallelize job creation by collecting all buildJob promises (for both unscheduleJob and scheduleJob) across scheduled resources returned by loadScheduledResources and then awaiting them with Promise.all; update references inside resolvePreScheduled so each scheduled item returns an object {unscheduleJob, scheduleJob} after the paired promises resolve (use buildJob for both), keeping the same shape but eliminating per-iteration awaits to reduce boot latency.ghost/core/test/unit/server/services/post-scheduling/post-scheduler-service.test.js (1)
50-72: ⚡ Quick winReplace the 100 ms sleep with a deterministic signal.
The test emits
post.scheduledand then waits 100 ms before assertingadapter.schedulewas called. This is timing-dependent — on a slow/busy CI runner the async event handler inPostSchedulerService(which awaitsbuildJob→ DB stub → key signing) may not have resolved within 100 ms, causing intermittent failures. Sinceadapter.scheduleis a sinon spy you control, prefer awaiting a promise that resolves when the spy is called (or wrap the handler so the test can await it).♻️ Deterministic wait helper
- events.emit('post.scheduled', post); - await new Promise((resolve) => { - setTimeout(resolve, 100); - }); + const scheduled = new Promise((resolve) => { + adapter.schedule.callsFake(() => resolve()); + }); + events.emit('post.scheduled', post); + await scheduled;Additionally, the three
nock(...)interceptors registered on lines 50–52 don't appear to match any HTTP traffic the test produces (the assertions are purely on the spy). If they're dead from the prior implementation, they can be dropped.🤖 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/post-scheduling/post-scheduler-service.test.js` around lines 50 - 72, The test is using a flaky 100ms sleep after events.emit('post.scheduled') instead of deterministically waiting for the async handler to finish; replace the setTimeout wait by turning the sinon spy adapter.schedule into a promise-aware spy (e.g., wrap or stub adapter.schedule so it resolves a promise when invoked) and await that promise after emitting the event so assertions run only after adapter.schedule is called; update references to the spy used in the assertions (adapter.schedule) and ensure any helper used to wait resolves with the call args for assertions (the code paths that call buildJob / key signing will then complete before assertions). Also remove the three unused nock(...) interceptors since this test asserts only on the adapter.schedule spy and they do not match any HTTP traffic.ghost/core/core/server/services/post-scheduling/post-scheduler-service.js (1)
20-36: 💤 Low valueCache
model.previous('published_at')to avoid duplicate lookups.On the
unscheduledpath,model.previous('published_at')is evaluated twice (line 22 to determinepublishedAt, then again on line 33 foroldTime). A local variable would eliminate the duplicate Bookshelf lookup and clarify the relationship betweentimeandoldTime.Note: The concern about
oldTimesemantics on freshpost.scheduledevents is not applicable—tests confirm thatmodel.previous('published_at')is correctlynullfor newly scheduled posts (no prior state to capture), sooldTimeis already set tonullas expected by the adapter.🤖 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/post-scheduling/post-scheduler-service.js` around lines 20 - 36, In buildJob, avoid calling model.previous('published_at') twice by caching it in a local variable (e.g., prevPublishedAt = model.previous('published_at')); use prevPublishedAt when event === 'unscheduled' to compute publishedAt and reuse prevPublishedAt to compute oldTime (oldTime = prevPublishedAt ? moment(prevPublishedAt).valueOf() : null), leaving the rest of buildJob (time: moment(publishedAt).valueOf(), url, signed token creation via getSignedAdminToken, etc.) unchanged.
🤖 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/post-scheduling/post-scheduler-service.js`:
- Around line 56-73: The async event handlers registered with events.on for each
resource (using SCHEDULED_RESOURCES) call buildJob and then
adapter.schedule/adapter.unschedule but aren’t protected against buildJob
rejections; wrap the body of each listener (the callbacks for
`${resource}.scheduled`, `${resource}.rescheduled`, and
`${resource}.unscheduled`) in a try/catch so any error from buildJob or adapter
calls is caught and logged via `@tryghost/logging` (e.g., incluir a descriptive
message and the error object) instead of letting the Promise rejection become
unhandled; keep the existing calls to adapter.schedule and adapter.unschedule
but ensure errors are handled per-listener and do not propagate.
---
Outside diff comments:
In `@ghost/core/core/boot.js`:
- Around line 355-397: The code currently awaits stripe.init() before calling
Promise.all, which serializes startup; remove the standalone await stripe.init()
and add stripe.init() as one of the promises inside the existing Promise.all
array so stripe initialization runs in parallel with identityTokens.init(),
members.init(), tiers.init(), giftService.init({...}), new
AutomationsService().init({...}) and the other service inits; keep
createSchedulerAdapter() called prior to building the array so schedulerAdapter
can still be passed into postScheduling.init and giftService.init.
---
Nitpick comments:
In `@ghost/core/core/server/services/post-scheduling/index.js`:
- Around line 34-46: The resolvePreScheduled function currently builds jobs
serially by awaiting buildJob inside nested loops; refactor it to parallelize
job creation by collecting all buildJob promises (for both unscheduleJob and
scheduleJob) across scheduled resources returned by loadScheduledResources and
then awaiting them with Promise.all; update references inside
resolvePreScheduled so each scheduled item returns an object {unscheduleJob,
scheduleJob} after the paired promises resolve (use buildJob for both), keeping
the same shape but eliminating per-iteration awaits to reduce boot latency.
In `@ghost/core/core/server/services/post-scheduling/post-scheduler-service.js`:
- Around line 20-36: In buildJob, avoid calling model.previous('published_at')
twice by caching it in a local variable (e.g., prevPublishedAt =
model.previous('published_at')); use prevPublishedAt when event ===
'unscheduled' to compute publishedAt and reuse prevPublishedAt to compute
oldTime (oldTime = prevPublishedAt ? moment(prevPublishedAt).valueOf() : null),
leaving the rest of buildJob (time: moment(publishedAt).valueOf(), url, signed
token creation via getSignedAdminToken, etc.) unchanged.
In
`@ghost/core/test/unit/server/services/post-scheduling/post-scheduler-service.test.js`:
- Around line 50-72: The test is using a flaky 100ms sleep after
events.emit('post.scheduled') instead of deterministically waiting for the async
handler to finish; replace the setTimeout wait by turning the sinon spy
adapter.schedule into a promise-aware spy (e.g., wrap or stub adapter.schedule
so it resolves a promise when invoked) and await that promise after emitting the
event so assertions run only after adapter.schedule is called; update references
to the spy used in the assertions (adapter.schedule) and ensure any helper used
to wait resolves with the call args for assertions (the code paths that call
buildJob / key signing will then complete before assertions). Also remove the
three unused nock(...) interceptors since this test asserts only on the
adapter.schedule spy and they do not match any HTTP traffic.
🪄 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: 47df6550-3dc2-4f27-a89f-20b34e976533
📒 Files selected for processing (27)
ghost/core/core/boot.jsghost/core/core/frontend/services/proxy.jsghost/core/core/server/adapters/scheduling/utils.jsghost/core/core/server/lib/memoize-by-key.tsghost/core/core/server/models/integration.jsghost/core/core/server/services/automations/index.jsghost/core/core/server/services/frontend-data-service/front-end-data-service.jsghost/core/core/server/services/frontend-data-service/index.jsghost/core/core/server/services/gifts/gift-service-wrapper.jsghost/core/core/server/services/gifts/gift-service.tsghost/core/core/server/services/internal-keys/index.tsghost/core/core/server/services/post-scheduling/index.jsghost/core/core/server/services/post-scheduling/post-scheduler-service.jsghost/core/test/e2e-api/admin/automations.test.jsghost/core/test/e2e-api/admin/gift-reminders.test.jsghost/core/test/unit/frontend/helpers/comment-count.test.jsghost/core/test/unit/frontend/helpers/comments.test.jsghost/core/test/unit/frontend/helpers/content-api-key.test.jsghost/core/test/unit/frontend/helpers/ghost-head.test.jsghost/core/test/unit/server/lib/memoize-by-key.test.tsghost/core/test/unit/server/models/integration.test.jsghost/core/test/unit/server/services/automations/index.test.jsghost/core/test/unit/server/services/frontend-data-service/frontend-data-service.test.jsghost/core/test/unit/server/services/frontend-data-service/index.test.jsghost/core/test/unit/server/services/gifts/gift-service.test.tsghost/core/test/unit/server/services/internal-keys/index.test.tsghost/core/test/unit/server/services/post-scheduling/post-scheduler-service.test.js
💤 Files with no reviewable changes (4)
- ghost/core/core/server/services/frontend-data-service/front-end-data-service.js
- ghost/core/core/server/services/frontend-data-service/index.js
- ghost/core/test/unit/server/services/frontend-data-service/index.test.js
- ghost/core/test/unit/server/services/frontend-data-service/frontend-data-service.test.js
rob-ghost
commented
May 14, 2026
rob-ghost
force-pushed
the
chore/internal-api-key-caching
branch
4 times, most recently
from
af3d00b to
3669367
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/lib/auto-filling-map.ts`:
- Around line 17-24: The current get method uses super.get(key) and treats
undefined as “missing”, which causes recomputation when a key is present with
value undefined; change the presence check to use super.has(key) (or equivalent)
so get(key) only calls this.#compute(key) and super.set(key, value) when the map
truly lacks the key, ensuring values of undefined are cached and not recomputed
repeatedly.
🪄 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: 0396551a-ddb8-4cf6-a30c-593544541af5
📒 Files selected for processing (26)
ghost/core/core/boot.jsghost/core/core/frontend/services/proxy.jsghost/core/core/server/adapters/scheduling/utils.jsghost/core/core/server/lib/auto-filling-map.tsghost/core/core/server/models/integration.jsghost/core/core/server/services/automations/index.jsghost/core/core/server/services/frontend-data-service/front-end-data-service.jsghost/core/core/server/services/frontend-data-service/index.jsghost/core/core/server/services/gifts/gift-service-wrapper.jsghost/core/core/server/services/gifts/gift-service.tsghost/core/core/server/services/internal-keys/index.tsghost/core/core/server/services/post-scheduling/index.jsghost/core/core/server/services/post-scheduling/post-scheduler-service.jsghost/core/test/e2e-api/admin/automations.test.jsghost/core/test/e2e-api/admin/gift-reminders.test.jsghost/core/test/unit/frontend/helpers/comment-count.test.jsghost/core/test/unit/frontend/helpers/comments.test.jsghost/core/test/unit/frontend/helpers/content-api-key.test.jsghost/core/test/unit/frontend/helpers/ghost-head.test.jsghost/core/test/unit/server/lib/auto-filling-map.test.tsghost/core/test/unit/server/models/integration.test.jsghost/core/test/unit/server/services/automations/index.test.jsghost/core/test/unit/server/services/frontend-data-service/frontend-data-service.test.jsghost/core/test/unit/server/services/frontend-data-service/index.test.jsghost/core/test/unit/server/services/gifts/gift-service.test.tsghost/core/test/unit/server/services/post-scheduling/post-scheduler-service.test.js
💤 Files with no reviewable changes (4)
- ghost/core/core/server/services/frontend-data-service/index.js
- ghost/core/test/unit/server/services/frontend-data-service/frontend-data-service.test.js
- ghost/core/test/unit/server/services/frontend-data-service/index.test.js
- ghost/core/core/server/services/frontend-data-service/front-end-data-service.js
🚧 Files skipped from review as they are similar to previous changes (14)
- ghost/core/test/unit/frontend/helpers/content-api-key.test.js
- ghost/core/test/unit/server/services/automations/index.test.js
- ghost/core/test/unit/frontend/helpers/comments.test.js
- ghost/core/core/server/services/internal-keys/index.ts
- ghost/core/test/e2e-api/admin/gift-reminders.test.js
- ghost/core/test/e2e-api/admin/automations.test.js
- ghost/core/core/server/services/automations/index.js
- ghost/core/core/server/models/integration.js
- ghost/core/test/unit/server/models/integration.test.js
- ghost/core/core/server/services/gifts/gift-service.ts
- ghost/core/core/frontend/services/proxy.js
- ghost/core/core/boot.js
- ghost/core/core/server/adapters/scheduling/utils.js
- ghost/core/test/unit/server/services/gifts/gift-service.test.ts
kevinansfield
approved these changes
May 14, 2026
Member
kevinansfield
left a comment
kevinansfield
left a comment
There was a problem hiding this comment.
Left two comments that aren't necessarily blockers but would be good to address here or in the subsequent PRs. Otherwise the approach looks solid to me 🙌
Member
|
From a Codex review: In That means if key lookup or scheduler enqueue fails, the poll run can still report success while the retry/future poll was never queued, and the rejection is detached from the surrounding |
rob-ghost
force-pushed
the
chore/internal-api-key-caching
branch
from
3669367 to
9e647fe
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ghost/core/core/server/services/internal-keys/index.ts (1)
43-45: 💤 Low valueDocument that rejected Promises are cached.
The
AutoFillingMapcaches the Promise returned bymodels.Integration.getApiKeyBySlug, not the resolved value. If the database fetch fails (e.g., transient database error), the rejected Promise is cached and subsequentget(slug)calls will receive the same rejected Promise until the cache entry is explicitly invalidated via.delete(slug)or.clear().For the current use case (missing internal integration keys), this is acceptable since such errors are permanent until the key is created or rotated. However, if transient database errors are a concern, callers should be aware that they won't automatically retry after a failed fetch.
🤖 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/internal-keys/index.ts` around lines 43 - 45, Document that AutoFillingMap caches the Promise returned by models.Integration.getApiKeyBySlug under the internalKeys map (constructed with AutoFillingMap<InternalIntegrationSlug, Promise<InternalApiKey>>), so rejected Promises from transient DB errors will be stored and subsequent internalKeys.get(slug) calls will receive the same rejected Promise until the entry is removed; add a short comment near the internalKeys declaration explaining this behavior and mention callers can invalidate entries using internalKeys.delete(slug) or internalKeys.clear() if they need retries on transient failures, and reference SLUG_KEY_TYPE to make clear which key type is used for the fetch.
🤖 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/internal-keys/index.ts`:
- Around line 43-45: Document that AutoFillingMap caches the Promise returned by
models.Integration.getApiKeyBySlug under the internalKeys map (constructed with
AutoFillingMap<InternalIntegrationSlug, Promise<InternalApiKey>>), so rejected
Promises from transient DB errors will be stored and subsequent
internalKeys.get(slug) calls will receive the same rejected Promise until the
entry is removed; add a short comment near the internalKeys declaration
explaining this behavior and mention callers can invalidate entries using
internalKeys.delete(slug) or internalKeys.clear() if they need retries on
transient failures, and reference SLUG_KEY_TYPE to make clear which key type is
used for the fetch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 38e4c919-db73-4f68-b261-5c58547acf40
📒 Files selected for processing (26)
ghost/core/core/boot.jsghost/core/core/frontend/services/proxy.jsghost/core/core/server/adapters/scheduling/utils.jsghost/core/core/server/lib/auto-filling-map.tsghost/core/core/server/models/integration.jsghost/core/core/server/services/automations/index.jsghost/core/core/server/services/frontend-data-service/front-end-data-service.jsghost/core/core/server/services/frontend-data-service/index.jsghost/core/core/server/services/gifts/gift-service-wrapper.jsghost/core/core/server/services/gifts/gift-service.tsghost/core/core/server/services/internal-keys/index.tsghost/core/core/server/services/post-scheduling/index.jsghost/core/core/server/services/post-scheduling/post-scheduler-service.jsghost/core/test/e2e-api/admin/automations.test.jsghost/core/test/e2e-api/admin/gift-reminders.test.jsghost/core/test/unit/frontend/helpers/comment-count.test.jsghost/core/test/unit/frontend/helpers/comments.test.jsghost/core/test/unit/frontend/helpers/content-api-key.test.jsghost/core/test/unit/frontend/helpers/ghost-head.test.jsghost/core/test/unit/server/lib/auto-filling-map.test.tsghost/core/test/unit/server/models/integration.test.jsghost/core/test/unit/server/services/automations/index.test.jsghost/core/test/unit/server/services/frontend-data-service/frontend-data-service.test.jsghost/core/test/unit/server/services/frontend-data-service/index.test.jsghost/core/test/unit/server/services/gifts/gift-service.test.tsghost/core/test/unit/server/services/post-scheduling/post-scheduler-service.test.js
💤 Files with no reviewable changes (4)
- ghost/core/test/unit/server/services/frontend-data-service/frontend-data-service.test.js
- ghost/core/core/server/services/frontend-data-service/front-end-data-service.js
- ghost/core/test/unit/server/services/frontend-data-service/index.test.js
- ghost/core/core/server/services/frontend-data-service/index.js
✅ Files skipped from review due to trivial changes (1)
- ghost/core/test/unit/server/services/automations/index.test.js
🚧 Files skipped from review as they are similar to previous changes (17)
- ghost/core/test/e2e-api/admin/gift-reminders.test.js
- ghost/core/test/unit/frontend/helpers/comment-count.test.js
- ghost/core/test/unit/frontend/helpers/content-api-key.test.js
- ghost/core/test/e2e-api/admin/automations.test.js
- ghost/core/test/unit/server/lib/auto-filling-map.test.ts
- ghost/core/core/server/services/automations/index.js
- ghost/core/test/unit/frontend/helpers/comments.test.js
- ghost/core/test/unit/server/models/integration.test.js
- ghost/core/test/unit/server/services/post-scheduling/post-scheduler-service.test.js
- ghost/core/core/server/services/gifts/gift-service-wrapper.js
- ghost/core/core/server/services/gifts/gift-service.ts
- ghost/core/core/server/services/post-scheduling/post-scheduler-service.js
- ghost/core/core/server/adapters/scheduling/utils.js
- ghost/core/test/unit/frontend/helpers/ghost-head.test.js
- ghost/core/test/unit/server/services/gifts/gift-service.test.ts
- ghost/core/core/boot.js
- ghost/core/core/server/services/post-scheduling/index.js
rob-ghost
force-pushed
the
chore/internal-api-key-caching
branch
from
9e647fe to
8298068
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/test/unit/server/models/integration.test.js`:
- Around line 78-125: Tests install the mock-knex tracker via tracker.install()
in each it block but never uninstall it, risking cross-test leakage; add a
teardown that calls tracker.uninstall() after each test (e.g., an afterEach
hook) so the tracker is removed between tests. Update the test file's lifecycle
to call tracker.uninstall() (or otherwise remove the installed tracker) after
tests that use tracker.install() around the
models.Integration.getApiKeyBySlug(...) scenarios to ensure no handlers/state
persist across tests.
🪄 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: f25948e2-dffa-4ee4-ba9e-7f40a46617aa
📒 Files selected for processing (26)
ghost/core/core/boot.jsghost/core/core/frontend/services/proxy.jsghost/core/core/server/adapters/scheduling/utils.jsghost/core/core/server/lib/auto-filling-map.tsghost/core/core/server/models/integration.jsghost/core/core/server/services/automations/index.jsghost/core/core/server/services/frontend-data-service/front-end-data-service.jsghost/core/core/server/services/frontend-data-service/index.jsghost/core/core/server/services/gifts/gift-service-wrapper.jsghost/core/core/server/services/gifts/gift-service.tsghost/core/core/server/services/internal-keys/index.tsghost/core/core/server/services/post-scheduling/index.jsghost/core/core/server/services/post-scheduling/post-scheduler-service.jsghost/core/test/e2e-api/admin/automations.test.jsghost/core/test/e2e-api/admin/gift-reminders.test.jsghost/core/test/unit/frontend/helpers/comment-count.test.jsghost/core/test/unit/frontend/helpers/comments.test.jsghost/core/test/unit/frontend/helpers/content-api-key.test.jsghost/core/test/unit/frontend/helpers/ghost-head.test.jsghost/core/test/unit/server/lib/auto-filling-map.test.tsghost/core/test/unit/server/models/integration.test.jsghost/core/test/unit/server/services/automations/index.test.jsghost/core/test/unit/server/services/frontend-data-service/frontend-data-service.test.jsghost/core/test/unit/server/services/frontend-data-service/index.test.jsghost/core/test/unit/server/services/gifts/gift-service.test.tsghost/core/test/unit/server/services/post-scheduling/post-scheduler-service.test.js
💤 Files with no reviewable changes (4)
- ghost/core/test/unit/server/services/frontend-data-service/index.test.js
- ghost/core/test/unit/server/services/frontend-data-service/frontend-data-service.test.js
- ghost/core/core/server/services/frontend-data-service/front-end-data-service.js
- ghost/core/core/server/services/frontend-data-service/index.js
🚧 Files skipped from review as they are similar to previous changes (21)
- ghost/core/test/e2e-api/admin/gift-reminders.test.js
- ghost/core/test/unit/frontend/helpers/content-api-key.test.js
- ghost/core/test/unit/frontend/helpers/comment-count.test.js
- ghost/core/test/unit/server/services/automations/index.test.js
- ghost/core/test/unit/frontend/helpers/comments.test.js
- ghost/core/test/e2e-api/admin/automations.test.js
- ghost/core/core/server/lib/auto-filling-map.ts
- ghost/core/core/server/services/internal-keys/index.ts
- ghost/core/core/frontend/services/proxy.js
- ghost/core/test/unit/frontend/helpers/ghost-head.test.js
- ghost/core/core/server/models/integration.js
- ghost/core/core/server/services/gifts/gift-service.ts
- ghost/core/test/unit/server/lib/auto-filling-map.test.ts
- ghost/core/test/unit/server/services/post-scheduling/post-scheduler-service.test.js
- ghost/core/core/server/services/automations/index.js
- ghost/core/core/server/services/gifts/gift-service-wrapper.js
- ghost/core/core/server/adapters/scheduling/utils.js
- ghost/core/core/boot.js
- ghost/core/test/unit/server/services/gifts/gift-service.test.ts
- ghost/core/core/server/services/post-scheduling/index.js
- ghost/core/core/server/services/post-scheduling/post-scheduler-service.js
Internal API key reads were spread across four services, each with its
own caching and closure-capture pattern, so rotating a key required a
full restart for any of them to pick up the new value. To make key
rotation feasible without restart, the data path was restructured
rather than wrapped.
Integration.getApiKeyBySlug now returns a flat {id, secret} DTO so
callers no longer traverse api_keys arrays or call toJSON. It throws
NotFoundError when the integration or its key is missing, because
internal integrations are seeded by fixtures and migrations and any
absence indicates a broken install.
A new services/internal-keys repository memoizes by slug using a typed
InternalIntegrationSlug union and exposes invalidate() to clear
per-slug or all entries on rotation. PostScheduler, Automations, and
the Gift service now consume the repository directly, removing the
getSchedulerIntegration function-injection plumbing.
PostSchedulerService's constructor is now synchronous; the wrapper
resolves preScheduled job specs before construction, so the two-phase
init API is gone. FrontendDataService and its dataService DI dance are
deleted entirely — the frontend proxy reads the internal frontend key
directly, with a null-on-error wrapper at that boundary. The memoize
helper now uses Object.assign instead of an as-cast; otherwise
unchanged.
rob-ghost
force-pushed
the
chore/internal-api-key-caching
branch
from
8298068 to
a85cb10
Compare
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
2 participants
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.
Problem
Internal integration API keys (the scheduler, the internal frontend) are cached by the services that consume them — as constructor closures or per-service instance state. Rotating those keys in the database doesn't reach the cached copies, so any rotation forces a Ghost restart. That's painful for hosted customers and a hard blocker for self-hosters and third-party hosts that don't control the runtime.
Solution
Replace the per-service caches with a single process-lifetime cache, keyed by integration slug, that every consumer reads through. Consumers see a read-only Map; rotation orchestration uses the full Map surface to evict an entry after the underlying database row has been rotated, so the next read repopulates from the new key.
Scheduled posts and pages have signed callback URLs baked at schedule time, so after rotation the queued URLs would fail validation. The post-scheduling service now exposes a rescheduling path that re-signs every queued entry under the current key (with a hook for the rotation flow to unschedule the old entries with the previous key).
This unblocks "Reset all authentication" in the follow-up PR, without requiring a Ghost restart.