Forwarded worker event-bus emits to the main process#28429
Open
rob-ghost wants to merge 1 commit into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
WalkthroughThis PR implements cross-worker event forwarding: EventRegistry now extends EventEmitter, serializes event args (wrapping Bookshelf-like models and cleaning non-cloneable values), and forwards non-local emits over parentPort. The main process consumes IPC messages with eventRegistryInstance.replayForwarded(message), deserializes args, and re-emits without re-forwarding. job-service workerMessageHandler now defers to replayForwarded for forwarded events. E2E tests and testing env configs were updated to exercise worker-persisted notifications surfacing in the admin API. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 |
![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
🧹 Nitpick comments (1)
ghost/core/test/e2e-api/admin/notifications.test.js (1)
182-186: AwaitmockUpdateServershutdown inafterEachto avoid lingering handles
afterEachcurrently callsmockUpdateServer.close()without waiting (around line 184); use theclosecallback/Promise so teardown completes before the next test proceeds.Proposed fix
afterEach(async function () { if (mockUpdateServer) { - mockUpdateServer.close(); + await new Promise(resolve => mockUpdateServer.close(resolve)); mockUpdateServer = null; } await jobService.removeJob(UPDATE_CHECK_JOB).catch(() => {});🤖 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/e2e-api/admin/notifications.test.js` around lines 182 - 186, The afterEach teardown calls mockUpdateServer.close() without awaiting its completion, which can leave the server handle open; modify the afterEach that references mockUpdateServer to await shutdown by either wrapping mockUpdateServer.close in a Promise (resolving in the close callback) or using mockUpdateServer.close()'s returned Promise if available, then set mockUpdateServer = null only after the awaited close completes so teardown fully finishes before the next test.
🤖 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/common/events.js`:
- Around line 95-106: The current guard treats any object with a __model key as
a bridge envelope and can accidentally rewrite normal payloads; tighten the
match in the envelope-detection block so it only converts true bridge envelopes
by checking for a specific marker or expected shape: update the condition around
__model to require either a bridge marker (e.g. arg.__model.__isGhostBridge ===
true) or a proven model shape (e.g.
Object.prototype.hasOwnProperty.call(arg.__model, 'attributes') && typeof
arg.__model.attributes === 'object' or
Object.prototype.hasOwnProperty.call(arg.__model, 'id')); change the condition
in the block that returns get/toJSON/attributes (the code handling arg.__model)
to use this stricter check so ordinary payloads with a __model key are left
untouched.
---
Nitpick comments:
In `@ghost/core/test/e2e-api/admin/notifications.test.js`:
- Around line 182-186: The afterEach teardown calls mockUpdateServer.close()
without awaiting its completion, which can leave the server handle open; modify
the afterEach that references mockUpdateServer to await shutdown by either
wrapping mockUpdateServer.close in a Promise (resolving in the close callback)
or using mockUpdateServer.close()'s returned Promise if available, then set
mockUpdateServer = null only after the awaited close completes so teardown fully
finishes before the next test.
🪄 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: d9802678-dee4-4d3a-a942-8ca9014066a5
📒 Files selected for processing (5)
ghost/core/core/server/lib/common/events.jsghost/core/core/server/services/jobs/job-service.jsghost/core/core/shared/config/env/config.testing-mysql.jsonghost/core/core/shared/config/env/config.testing.jsonghost/core/test/e2e-api/admin/notifications.test.js
The shared events bus was process-local. Listeners registered in the main process (settingsCache, the notifications cleanup hook, etc.) never reacted to events emitted from inside a Bree worker, so state writes performed by workers left main-process subscribers stale until restart. This bit the update-check worker: notifications were persisted to the DB but the in-process settingsCache didn't refresh, so the admin notifications endpoint returned an empty list and no banner rendered. Bridged the gap inside the events module itself. When loaded in a worker thread, every emit is also postMessaged across the parent port. The main process replays the message on its own emitter via a hook installed in the worker message handler. Existing subscribers receive the event identical to a local emission. Payloads survive structured-clone via a small serializer: Bookshelf models are unwrapped via toJSON and rehydrated into an adapter that preserves the .get/.toJSON/.attributes shape listeners depend on; other objects are deep-cleaned of values structured-clone rejects (Bookshelf attaches query callbacks and a Knex transaction handle that listeners never read). EventEmitter internals (newListener, removeListener, error) are intentionally not forwarded. Unserializable payloads warn once per event name and skip the forward; the local emit always runs. Added an HTTP-level e2e test that spawns the update-check worker and asserts GET /ghost/api/admin/notifications/ returns the worker-added notification. The test exercises the contract the admin UI depends on without observing the bridge mechanism. Verified the test fails when the bridge is disabled. Bumped the test environment's global brute-force freeRetries (was 4, now 99) so worker-spawning tests don't trip the per-IP login limiter; the dedicated rate-limiting test reads its threshold from config so it stays correct.
rob-ghost
force-pushed
the
fix/worker-events-bridge
branch
from
f3aff4b to
0daa10a
Compare
Contributor
Author
|
Marking as draft before this very well might start showing upgrade notifications to Ghost(Pro) customers when it was silently failing before. We want to see if we can add the ability to subscribe / unsubscribe to specific "channels" as a hoster so you can pick and choose which notifications go to your customers before we merge this. |
Contributor
Author
|
Release notification banner concern was over-stated, we will fix, but this won't make anything worse. |
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
1 participant
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
State writes performed inside a Bree worker leave main-process subscribers stale. Specifically: when the update-check worker called
notifications.add, the notification was correctly persisted to the database, but the main process's in-memory settings cache never refreshed. The admin notifications endpoint kept returning the pre-worker snapshot, so the alert banner did not render until Ghost was restarted.The shared events bus is process-local. Workers and the main process each instantiate their own emitter. Events fired in one process never reach subscribers in the other. The cache freshness contract, several model-listener hooks, and any future worker that touches cached settings all silently break across this boundary.
Solution
Bridge the events bus across the worker boundary. When the module is loaded inside a worker thread, every emit is also posted across the parent port. The main process replays the message on its own emitter from inside the existing worker message handler. Subscribers receive the event identically to a local emission, so settings cache, model-listener hooks, and anything else listening continue to work without changes.
The serializer is the only piece with surface area worth thinking about. Bookshelf models are unwrapped via their toJSON method and rehydrated on the receiving side into an adapter that preserves the get, toJSON, and attributes shape listeners expect. Other objects are deep-cleaned of values that structured-clone rejects (Bookshelf attaches query callbacks and a Knex transaction handle to option objects, neither of which listeners read). EventEmitter internals are intentionally not forwarded.
If a payload can't be cloned for any reason, the bridge logs once per event name and skips the forward. The local emit always runs regardless, so worker-side behaviour is unchanged.
Added an HTTP-level test that spawns the update-check worker and asserts the admin notifications endpoint returns the worker-added notification on the next read. The test exercises the contract the admin UI depends on without observing the bridge mechanism. Bumped the test environment's per-IP login limiter so multi-login files don't trip it; the dedicated rate-limiting test reads its threshold from config so it stays accurate.