fix(batch): bullmq worker requires maxretriesperrequest null redis

[LucasSantana-Dev]

Summary

• Fixed BullMQ Worker startup failure caused by redis maxRetriesPerRequest: 3 config
• Created dedicated redis connection for BullMQ with maxRetriesPerRequest: null
• Added proper lifecycle management and error handling for BullMQ redis connection

Root Cause

BullMQ Worker strictly requires a redis connection with maxRetriesPerRequest: null to avoid connection state queueing conflicts. The shared redis client was configured with maxRetriesPerRequest: 3, which caused:

Error: BullMQ: Your redis options maxRetriesPerRequest must be null
TypeError: Cannot read properties of undefined (reading 'client')

Changes

• Created createBullMQRedisConnection() function with correct ioredis options
• Updated startBatchJobWorker() to instantiate BullMQ redis separately
• Added cleanup logic in stopBatchJobWorker() to disconnect the BullMQ redis instance
• Updated unit tests to verify BullMQ connection config and proper cleanup

Verification

• 16 tests passing for batchJobWorker
• Type check passing for all workspaces
• No type errors in the updated code

Closes #1652

---

Summary by cubic

Fixes the batch bullmq worker by using a dedicated ioredis connection with maxRetriesPerRequest: null and lazy connect. Prevents startup failures, handles missing shared Redis, and ensures clean shutdown. Closes #1652.

• Bug Fixes
  • Added createBullMQRedisConnection() using ENVIRONMENT_CONFIG with maxRetriesPerRequest: null and lazyConnect: true.
  • Worker now uses this dedicated connection and returns early if the shared Redis client is unavailable.
  • On executor registration or startup failure and on shutdown, we disconnect BullMQ Redis and clear the worker reference; tests cover connection options, usage, and cleanup.

^{Written for commit 22fc6af. Summary will update on new commits.}

Summary by CodeRabbit

• Bug Fixes
  • Improved batch job worker reliability by using a dedicated Redis connection for background processing.
  • Added stronger startup failure handling to ensure the dedicated connection is disconnected and cleared correctly.
  • Ensured shutdown always releases the dedicated Redis connection, even if worker closing throws.
• Tests
  • Updated and expanded worker tests to cover Redis unavailable behavior and verify that shutdown disconnects the dedicated connection.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 3e414907-185d-4f99-a589-dbf827be4f95

📥 Commits

Reviewing files that changed from the base of the PR and between 48d6569 and 22fc6af.

📒 Files selected for processing (1)

• packages/bot/src/workers/batchJobWorker.ts

🚧 Files skipped from review as they are similar to previous changes (1)

• packages/bot/src/workers/batchJobWorker.ts

---

📝 Walkthrough

Walkthrough

The batch job worker now uses a dedicated BullMQ Redis connection with maxRetriesPerRequest: null, cleans it up on startup failure and shutdown, and updates the worker spec to mock and verify the shared Redis and BullMQ Redis paths separately.

Changes

Dedicated BullMQ Redis connection

Layer / File(s)	Summary
Worker connection state and dedicated Redis creation packages/bot/src/workers/batchJobWorker.ts	Adds a Redis import and bullmqRedis state, implements createBullMQRedisConnection() with maxRetriesPerRequest: null and lazyConnect: true, and wires startBatchJobWorker() to use the dedicated connection after checking shared redis availability.
Failure and shutdown cleanup packages/bot/src/workers/batchJobWorker.ts	Disconnects the dedicated Redis connection when executor registration or Worker creation fails, and stopBatchJobWorker() disconnects it in a finally block while closing and nulling the worker.
Spec updates for dual Redis mocking packages/bot/src/workers/batchJobWorker.spec.ts	Reworks the test harness to separate sharedRedis from a BullMQ-specific MockRedis/ioredis mock with disconnect, adds ENVIRONMENT_CONFIG.REDIS mocking, and updates assertions for worker options and disconnect behavior.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related PRs

• LucasSantana-Dev/Lucky#1564: Directly updates the same batch worker module and its BullMQ startup path.

Suggested labels: backend, non-destructive-confirmed

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately reflects the main change: fixing the BullMQ worker Redis configuration to use maxRetriesPerRequest null.
Linked Issues check	✅ Passed	The PR implements the BullMQ-specific Redis connection, null maxRetriesPerRequest, and cleanup/error handling required by #1652.
Out of Scope Changes check	✅ Passed	The changes stay focused on the batch worker Redis/BullMQ startup fix and related tests, with no clear unrelated additions.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/batch-worker-bullmq-redis

---

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

[github-actions]

Failed to generate code suggestions for PR

[cubic-dev-ai]

2 issues found across 2 files

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[cubic-dev-ai]

0 issues found across 1 file (changes from recent commits).

<sub>Requires human review: Bug fix creating a dedicated Redis connection for BullMQ worker. Changes involve core infrastructure (Redis connection lifecycle, error handling) and could impact job processing stability if connection setup or cleanup contains defects.

Re-trigger cubic</sub>

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (3)

packages/bot/src/workers/batchJobWorker.spec.ts (1)

57-67: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Keep the Redis mock instance aligned with bullmqRedisCtor. When a custom ctor is passed, mockBullmqRedis still reflects the fallback branch, so the helper returns a different object than the ctor path. If this helper is meant to support custom Redis constructors, derive the returned instance from that path too, or drop the extra return value.

🤖 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 `@packages/bot/src/workers/batchJobWorker.spec.ts` around lines 57 - 67, The
Redis mock setup in batchJobWorker.spec.ts is not aligned with the custom
constructor path, so the helper can return a different instance than the one
created by bullmqRedisCtor. Update the mock construction around
MockRedis/mockBullmqRedis so the returned Redis instance is derived from the
custom ctor when provided, or remove the separate fallback instance entirely if
it is not needed. Keep the behavior consistent for both the default and
ctor-injected paths so tests exercise the same object that BullMQ would use.

packages/bot/src/workers/batchJobWorker.ts (2)

198-201: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

disconnect() is synchronous — await/try-catch around it don't add real protection.

ioredis#disconnect() Disconnect from Redis. and does not return a Promise, so the await at lines 200/238 resolves immediately regardless of actual socket teardown, and the try/catch at lines 237-245 will rarely have anything to catch. If waiting for a clean close (e.g., no dropped in-flight replies) matters, Redis#quit() is the documented alternative: "If you want to wait for the pending replies, use Redis#quit instead." This is purely a robustness nit since lazyConnect: true means these failure paths likely never established a real connection anyway.

Also applies to: 235-248, 271-274

🤖 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 `@packages/bot/src/workers/batchJobWorker.ts` around lines 198 - 201,
`bullmqRedis.disconnect()` in `batchJobWorker` is being treated like an async
cleanup step, but it is synchronous and the surrounding `await`/try-catch gives
a false sense of protection. Update the cleanup paths in the
executor-registration failure handling and the related teardown blocks to call
`disconnect()` directly, or switch to `quit()` only if you truly need to wait
for pending replies; keep the nulling/reset logic in the same `batchJobWorker`
cleanup flow.

---

149-166: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Consider adding an error listener on the dedicated connection.

createBullMQRedisConnection() never attaches an .on('error', ...) handler to bullmqRedis. BullMQ's own production guidance recommends this explicitly: "It is really useful to attach a handler for the error event which will be triggered when there are connection issues. This will be helpful when debugging your queues and prevent "unhandled errors"." Given this file exists specifically to fix a Redis-connectivity startup failure (#1652), it's worth having visibility into connection-level errors on this dedicated client rather than relying solely on BullMQ's internal handling.

♻️ Proposed addition

 function createBullMQRedisConnection(): Redis {
     // No try/catch: with lazyConnect the constructor never touches the
     // network — connection errors surface later through BullMQ's own
     // error handling, so a catch here would be a false safety net.
-    return new Redis({
+    const connection = new Redis({
         host: ENVIRONMENT_CONFIG.REDIS.HOST,
         port: ENVIRONMENT_CONFIG.REDIS.PORT,
         password: ENVIRONMENT_CONFIG.REDIS.PASSWORD,
         db: ENVIRONMENT_CONFIG.REDIS.DB,
         // BullMQ requirement: must be null to avoid request queueing conflicts
         maxRetriesPerRequest: null,
         lazyConnect: true,
     })
+    connection.on('error', (error) => {
+        errorLog({ message: 'BullMQ redis connection error', error })
+    })
+    return connection
 }

🤖 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 `@packages/bot/src/workers/batchJobWorker.ts` around lines 149 - 166, The
dedicated Redis client created in createBullMQRedisConnection() is missing an
error event handler, so connection-level failures can be hidden or become
unhandled. Update the BullMQ Redis setup in batchJobWorker.ts by attaching an
.on('error', ...) listener to the Redis instance returned from
createBullMQRedisConnection(), and log the error with enough context to help
debug queue connectivity issues.

🤖 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 `@packages/bot/src/workers/batchJobWorker.ts`:
- Around line 254-276: The shutdown flow in stopBatchJobWorker leaves a stale
worker reference if worker.close() throws because worker = null is only set on
the success path. Move the worker reset into the finally block in
stopBatchJobWorker so the module state is cleared unconditionally after any
close attempt, alongside the existing bullmqRedis cleanup, and keep the logging
in the try/catch unchanged.

---

Nitpick comments:
In `@packages/bot/src/workers/batchJobWorker.spec.ts`:
- Around line 57-67: The Redis mock setup in batchJobWorker.spec.ts is not
aligned with the custom constructor path, so the helper can return a different
instance than the one created by bullmqRedisCtor. Update the mock construction
around MockRedis/mockBullmqRedis so the returned Redis instance is derived from
the custom ctor when provided, or remove the separate fallback instance entirely
if it is not needed. Keep the behavior consistent for both the default and
ctor-injected paths so tests exercise the same object that BullMQ would use.

In `@packages/bot/src/workers/batchJobWorker.ts`:
- Around line 198-201: `bullmqRedis.disconnect()` in `batchJobWorker` is being
treated like an async cleanup step, but it is synchronous and the surrounding
`await`/try-catch gives a false sense of protection. Update the cleanup paths in
the executor-registration failure handling and the related teardown blocks to
call `disconnect()` directly, or switch to `quit()` only if you truly need to
wait for pending replies; keep the nulling/reset logic in the same
`batchJobWorker` cleanup flow.
- Around line 149-166: The dedicated Redis client created in
createBullMQRedisConnection() is missing an error event handler, so
connection-level failures can be hidden or become unhandled. Update the BullMQ
Redis setup in batchJobWorker.ts by attaching an .on('error', ...) listener to
the Redis instance returned from createBullMQRedisConnection(), and log the
error with enough context to help debug queue connectivity issues.

🪄 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: 1c3b22ba-f2b7-48d6-aa45-47a3ab2252b6

📥 Commits

Reviewing files that changed from the base of the PR and between 54f1f73 and 48d6569.

📒 Files selected for processing (2)

• packages/bot/src/workers/batchJobWorker.spec.ts
• packages/bot/src/workers/batchJobWorker.ts

[cubic-dev-ai]

0 issues found across 1 file (changes from recent commits).

<sub>Requires human review: Changes Redis connection handling and worker lifecycle for BullMQ; requires human review to verify correctness.

Re-trigger cubic</sub>

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
90.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud