feat(infra): use a message queue service for IPC#477
Conversation
wpf500
force-pushed
the
feat/better-broadcast
branch
2 times, most recently
from
c17cc47 to
7d4c819
Compare
wpf500
force-pushed
the
feat/better-broadcast
branch
from
7d4c819 to
0be4966
Compare
There was a problem hiding this comment.
Good approach overall 👍
A few issues should be addressed before merging — see inline comments.
Additional notes (not on diff lines)
serviceSecret is now unused (packages/core/src/config/config.ts:166)
BEABEE_SERVICE_SECRET / config.serviceSecret was only used by the removed NetworkCommunicatorService for JWT signing. Since Redis Pub/Sub doesn't need JWT auth, this config entry and env var can be removed — either here or as a follow-up.
| log.error('Uncaught error', err); | ||
| } | ||
| await db.close(); | ||
| await mq.close(); |
There was a problem hiding this comment.
Bug: SIGTERM handler does not close Redis connections
mq.close() is correctly called here in runApp(), but the startServer() SIGTERM handler (line 30) only calls db.close() — mq.close() is missing there. This can cause hanging Redis connections during Kubernetes pod shutdowns.
| const log = mainLogger.child({ app: 'message-queue' }); | ||
|
|
||
| const subClient = createClient({ url: config.messageQueueUrl }); | ||
| const pubClient = subClient.duplicate(); |
There was a problem hiding this comment.
Missing error event handlers on Redis clients
The redis client emits error events, and unhandled EventEmitter errors will crash the Node.js process. Both clients need error handlers:
subClient.on("error", (err) => log.error("Redis sub client error", err));
pubClient.on("error", (err) => log.error("Redis pub client error", err));|
|
||
| const log = mainLogger.child({ app: 'message-queue' }); | ||
|
|
||
| const subClient = createClient({ url: config.messageQueueUrl }); |
There was a problem hiding this comment.
Module-level side effects
Redis clients are created immediately on import. This means any code that imports message-queue (including transitively via OptionsService) requires a valid Redis URL at import time, which makes testing harder.
Consider deferring client creation into connect(). Not a blocker for this PR.
| BEABEE_DATABASE_URL=postgres://membership_system:membership_system@db/membership_system | ||
|
|
||
| # Redis connection URL [REQUIRED] | ||
| BEABEE_MESSAGEQUEUE_URL=redis://redis:6379 |
There was a problem hiding this comment.
Host mismatch with docker-compose service name
The Docker Compose service is named mq, not redis. This default URL will not resolve within the Docker network.
| BEABEE_MESSAGEQUEUE_URL=redis://redis:6379 | |
| BEABEE_MESSAGEQUEUE_URL=redis://mq:6379 |
|
|
||
| export async function connect(): Promise<void> { | ||
| await subClient.connect(); | ||
| await subClient.subscribe('service:broadcast', handleMessage); |
There was a problem hiding this comment.
No reconnect handling for subscriber
The redis v5 client has auto-reconnect, but in subscriber mode subscriptions need to be re-established after reconnect. If the connection drops, the service:broadcast subscription will be lost silently. Not critical for dev, but important for production reliability.
2 participants
This is part of a series of PR to make beabee ready for Kubernetes.
To move to Kubernetes we need to remove the fixed Docker-style references to other services (e.g.
api_app,webhook_app). This could be simply replaced with Kubernetes-style references, but to keep compatibility with Docker stacks (e.g. for self hosters), and because in general it's a better solution, this PR adds Redis as a message queue service for IPC.