fix(slots): prevent dead worker re-entering pool on double error listener#29518
fix(slots): prevent dead worker re-entering pool on double error listener#2951810done wants to merge 10 commits into
Conversation
|
Welcome to Cal.diy, @10done! Thanks for opening this pull request. A few things to keep in mind:
A maintainer will review your PR soon. Thanks for contributing! |
|
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:
📝 WalkthroughWalkthroughThe pull request refactors the task-specific worker event listeners in 🚥 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 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/api/v2/src/modules/slots/slots-2024-04-15/services/slots-worker.service.bug.spec.ts (1)
6-9: ⚡ Quick winTrim the comments down to intent.
Most of these banners narrate the mock/test mechanics instead of the failure mode the spec is pinning down. Keeping only the comments that explain why a case exists will make the file much easier to scan.
As per coding guidelines, "Only add code comments that explain why, not what" and "Never add comments that simply restate what the code does."Also applies to: 23-26, 65-67, 95-98, 121-125
🤖 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/api/v2/src/modules/slots/slots-2024-04-15/services/slots-worker.service.bug.spec.ts` around lines 6 - 9, Replace the verbose banner comment "Shape of the mocked Worker instances created inside the jest.mock factory." with a single one-line intent comment that explains why the mock shape is declared (e.g., "Define the mocked Worker interface used by the jest.mock factory so tests share a single type"), and likewise shorten the other large banner-style comments in this spec to one-line intent comments that state why each test or helper exists (not how it works) — apply this change to the other multi-line banner blocks near the mocked Worker and test-case sections so comments explain intent only.Source: Coding guidelines
🤖 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/api/v2/src/modules/slots/slots-2024-04-15/services/slots-worker.service.ts`:
- Around line 205-206: The code registers messageListener and errorListener via
worker.once(...) before calling worker.postMessage(...) and fails to remove them
if postMessage throws or the subsequent catch path requeues the worker; update
the catch blocks around worker.postMessage(...) to call worker.off("message",
messageListener) and worker.off("error", errorListener) (or
worker.removeListener) before pushing the worker back into availableWorkers or
reusing it so stale handlers don't remain; apply the same removal in the other
catch block that also requeues the worker.
- Around line 181-183: handleWorkerFailure currently mutates this.workerPool
with splice(this.workerPool.indexOf(failedWorker), 1) and can run twice for the
same worker, and processNextTask attaches once("message")/once("error") before
calling worker.postMessage(...) so a synchronous postMessage throw leaves
dangling listeners; fix by making failure handling idempotent and cleaning up
listeners: in handleWorkerFailure (and the worker.on("error")/worker.on("exit")
callers) first check that the worker is still in this.workerPool (index !== -1)
or set and check a per-worker flag (e.g., worker.__failed) before splicing, and
when processNextTask sets up once("message")/once("error"), wrap the postMessage
call so that on synchronous exception you remove those once listeners and return
the worker to availableWorkers (or mark it failed) to avoid handlers firing
later.
---
Nitpick comments:
In
`@apps/api/v2/src/modules/slots/slots-2024-04-15/services/slots-worker.service.bug.spec.ts`:
- Around line 6-9: Replace the verbose banner comment "Shape of the mocked
Worker instances created inside the jest.mock factory." with a single one-line
intent comment that explains why the mock shape is declared (e.g., "Define the
mocked Worker interface used by the jest.mock factory so tests share a single
type"), and likewise shorten the other large banner-style comments in this spec
to one-line intent comments that state why each test or helper exists (not how
it works) — apply this change to the other multi-line banner blocks near the
mocked Worker and test-case sections so comments explain intent only.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7f943eaf-91d7-48be-896a-82fbffd8b03c
📒 Files selected for processing (2)
apps/api/v2/src/modules/slots/slots-2024-04-15/services/slots-worker.service.bug.spec.tsapps/api/v2/src/modules/slots/slots-2024-04-15/services/slots-worker.service.ts
|
|
Apologies. Will make the changes. |
@kartik-212004 I have made the changes. |
please check the above comments aswell |
Which other comments exactly. Can you please point to them. |
https://github.com/calcom/cal.diy/pull/29518/changes#r3370822336 |
I can't see the inline comments in Files changed (sidebar shows 0 of 2 — possibly outdated). |
|
that's weird, it should have been visible. |
|
@kartik-212004 I have addressed the comments. P.S. : Those comments were in the pending state, need to be submitted. |
|
@kartik-212004 Can you please review whenever you have time. Thank You. |
|
@bandhan-majumder Can I have a review please. Thank You. |
|
|
3 participants
What does this PR do?
When a worker errored mid-task, both the persistent lifecycle on("error")
and the task-specific once("error") fired. The task listener was pushing
the terminated worker back into availableWorkers after handleWorkerFailure
had already cleaned it up, inflating the pool with a dead worker reference.
Fix: errorListener now removes its sibling messageListener and does not
push the worker back — pool cleanup is owned solely by handleWorkerFailure.
Add unit tests covering all five corruption scenarios.
Visual Demo (For contributors especially)
Image Demo (if applicable):
Before the fix
After the fix
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
cd apps/api/v2
corepack yarn jest slots-worker.service.bug --no-coverage --verbose
No additional environment variables required. The unit tests mock all worker thread infrastructure — no real worker processes are spawned.
The tests are fully self-contained unit tests with no database, no API keys, and no external dependencies.