Skip to content

fix: Uncontrolled Parallelism in drainQueue leads to Resource Exhaustion - #47

Open
ankittroy-21 wants to merge 10 commits into
madhav2348:developfrom
ankittroy-21:main
Open

fix: Uncontrolled Parallelism in drainQueue leads to Resource Exhaustion#47
ankittroy-21 wants to merge 10 commits into
madhav2348:developfrom
ankittroy-21:main

Conversation

@ankittroy-21

@ankittroy-21 ankittroy-21 commented Jun 10, 2026

Copy link
Copy Markdown

Summary: Implement concurrency control for job processing in drainQueue by utilizing the existing queueWorker.

This PR addresses a bug where drainQueue would initiate processing for all queued jobs simultaneously, leading to resource exhaustion. It now uses a sequential worker-based approach.

Issue ticket number and link

Closes #45

Changes

  • src/server/runtime.ts: Refactored drainQueue to trigger the queueWorker instead of manually iterating and spawning unawaited promises.
  • src/server/services/workers/queueWorker.ts: Updated to pass job.id to the pipeline and removed redundant manual status updates, relying on the pipeline's internal state management.
  • src/server/main.ts: Removed unused createQueueWorker import.
  • src/app/api/screenshots/route.ts: Removed unused writeFile import.

Testing

  • [✅] Tested locally: Verified fix by running TypeScript type checks (npm run backend:check) and ESLint (npm run lint).
  • Added/updated tests: No existing test suite found; manual verification through static analysis and dependency integration.

Checklist before requesting a review

  • [✅] Code follows the project's TypeScript style conventions
  • [✅] No secrets or .env values are committed
  • [✅] I have performed a self-review of my code
  • [✅] CI passes (Local type check and lint passed)

@github-actions

Copy link
Copy Markdown

🎉 Thanks for submitting a PR, @ankittroy-21!

Please confirm the following checklist before review:

  • ⭐ I have starred this repository
  • 🍴 I have forked this repository
  • 📖 I have read the Contributing Guidelines
  • 🔀 My changes are pushed to the develop branch

A maintainer will review your PR shortly. Thank you! 🚀

@madhav2348

Copy link
Copy Markdown
Owner

Removing job status= processing make sense at some level but removing of job status = failed, how would you handle that , if you remove

@ankittroy-21
ankittroy-21 changed the base branch from main to develop June 10, 2026 11:13
@ankittroy-21

Copy link
Copy Markdown
Author

@madhav2348 the removal wasn't just a deletion, but a centralization of the logic let me explain
The reason I removed the explicit job.status = "failed" from the worker is that I updated the call to include the jobId: pipeline.process(job.payload, job.id).
If you look at src/server/services/pipeline/screenshotPipeline.ts, the process method now handles the entire lifecycle of the job status. Specifically, it has its own try/catch block
> catch (err) {
> if (jobId && this.queue) {
> this.queue.updateStatus(
> jobId,
> "failed",
> null,
> err instanceof Error ? err.message : "Unknown error",
> );
> }
> throw err; // Re-thrown so the worker can still log it
> }
> ```
>
> By passing the jobId into the pipeline, we get two main benefits:
Consistency: The pipeline uses `queue.updateStatus(...)`, which ensures the `updatedAt` timestamp is correctly refreshed (the worker was previously modifying the object properties
directly).
Granularity: The pipeline can mark exactly where it failed (e.g., during the 'ocr' stage or the 'vision' stage), providing better debugging info than the worker could.
So, the job is still guaranteed to be marked as `failed`, but the logic now lives in the pipeline where the specific error context is known.

@madhav2348

Copy link
Copy Markdown
Owner

Impressive wait for approval

@ankittroy-21

Copy link
Copy Markdown
Author

@madhav2348 when will it get merged ?

@madhav2348

Copy link
Copy Markdown
Owner

Have some patience

@madhav2348

Copy link
Copy Markdown
Owner

Please resolve merged conflict

@madhav2348

Copy link
Copy Markdown
Owner

@ankittroy-21

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Uncontrolled Parallelism in drainQueue leads to Resource Exhaustion

3 participants