fix(worker): await GroupMQ worker.run() to prevent silent shard-stuck - #362
fix(worker): await GroupMQ worker.run() to prevent silent shard-stuck#362ayushjhanwar-png wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesWorker startup handling
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
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 `@apps/worker/src/boot-workers.ts`:
- Around line 221-223: Avoid double-handling GroupWorker errors: the
shard-specific worker.on('error') listener in boot-workers should not be
attached when the shared workers.forEach error handler already logs these
failures. Update the error handling around GroupWorker creation so the existing
shared listener carries the shard and queueName context, or conditionally skip
the generic listener for GroupWorker while keeping the unique context in the
single remaining handler.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c4d71423-78a5-464f-b5a5-70250f3f49a0
📒 Files selected for processing (1)
apps/worker/src/boot-workers.ts
| worker.on('error', (err) => { | ||
| logger.error('Worker runtime error', { shard: index, queueName, err }); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Avoid double-attaching error handlers to GroupWorker. The existing workers.forEach(...) block already logs errors for these workers, so this shard-specific listener will emit the same failure twice. Fold the shard/queue context into the shared handler or skip the generic listener for GroupWorker.
🤖 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 `@apps/worker/src/boot-workers.ts` around lines 221 - 223, Avoid
double-handling GroupWorker errors: the shard-specific worker.on('error')
listener in boot-workers should not be attached when the shared workers.forEach
error handler already logs these failures. Update the error handling around
GroupWorker creation so the existing shared listener carries the shard and
queueName context, or conditionally skip the generic listener for GroupWorker
while keeping the unique context in the single remaining handler.
d957a62 to
756f97d
Compare
…silent shard-stuck
worker.run() in bootWorkers() was fire-and-forget: no await, no .catch().
If the underlying redis.duplicate() blocking-client setup silently fails
for a specific shard (e.g., during rolling-deploy connection burst hitting
a transient Aiven Redis blip), the promise rejects with no handler:
- Node.js logs an unhandled-rejection warning to stderr (usually buried)
- The worker enters GroupMQ's exponential backoff sleep forever
- No further error logs — worker just… doesn't consume
- "Started worker for events_N" log line still prints (misleading)
- k8s liveness probe passes (pod is technically running)
- The shard's jobs pile up in Redis indefinitely
Incident 2026-07-09 IST: 3 shards (0, 18, 19) silently stuck for ~2h after
a rolling deploy. 570K jobs accumulated. Redis memory 2%→22% (~9 GB).
Cascaded to API pod OOM (increased Redis load + baseline leak). Zero error
logs anywhere in worker code path — first symptom was the SigNoz Redis
memory alert. Manual rolling-restart of the StatefulSet cleared it (fresh
redis.duplicate() calls succeeded the second time).
The fix mirrors the pattern already used for the Kafka consumer startup
right below this block: await + explicit error handler. Failure now:
- Logs the error with shard context
- Calls process.exit(1) to crash the pod
- k8s restarts the pod
- Fresh redis.duplicate() call — usually succeeds on retry
- Loud failure that we notice in seconds, not hours
Also adds a runtime worker.on('error', ...) listener so runtime failures
on the blocking client (BZPOPMIN, etc.) after startup are also visible.
Root cause is upstream code (introduced by upstream openpanel maintainer
in da59622, 2025-11-15, "fix: overall perf improvements"). Upstream
still has the same bug — separate upstream issue/PR to follow.
Follow-up work (separate PRs):
- Per-shard groupmq_events_consumed_total{shard=N} metric
- SigNoz alert: any shard consumption rate = 0 for 5min while others > 0
- k8s startupProbe verifying all assigned shards emit 'ready' within 60s
756f97d to
ee0b58a
Compare
worker.run()was fire-and-forget. Ifredis.duplicate()silently fails for a specific shard during startup, the worker enters GroupMQ backoff sleep forever with no error logs — pod looks healthy, shard never consumes.Incident 2026-07-09: 3 shards stuck ~2h → 570K job backlog → Redis 22% mem → API OOM.
Fix:
.catch()onworker.run()— crash loud, k8s restarts, fresh Redis connection retries usually work. Mirrors the pattern already used for the Kafka consumer below.Also adds
worker.on('error', ...)for runtime errors.Bug is upstream (
da59622dc, 2025-11-15). Same issue on upstreammain— upstream issue + PR to follow.Summary by CodeRabbit