fix(queue): allow QUEUE_WORKERS env var to override hardcoded worker count#429
Open
scnerd wants to merge 5 commits intorishikanthc:mainfrom
Open
fix(queue): allow QUEUE_WORKERS env var to override hardcoded worker count#429scnerd wants to merge 5 commits intorishikanthc:mainfrom
scnerd wants to merge 5 commits intorishikanthc:mainfrom
Conversation
Add tests verifying that getOptimalWorkerCount() respects the QUEUE_WORKERS environment variable and that NewTaskQueue() should allow QUEUE_WORKERS to override the hardcoded legacy worker count. Includes a failing test (TestNewTaskQueue_EnvOverridesLegacy) that reproduces the bug where QUEUE_WORKERS is always overridden by the hardcoded legacyWorkers parameter. Ref: rishikanthc#379 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…count The QUEUE_WORKERS environment variable was defined and read in getOptimalWorkerCount(), but NewTaskQueue() unconditionally overwrote the result with the hardcoded legacyWorkers parameter (always 2). This made QUEUE_WORKERS effectively dead code. Now legacyWorkers is only used as a fallback when QUEUE_WORKERS is not set, preserving the default of 2 workers while allowing users to control concurrency via the environment variable. Set QUEUE_WORKERS=1 to serialize all transcription jobs and prevent system overload during bulk uploads. Fixes: rishikanthc#379 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Switch from raw t.Errorf to testify/assert for consistency with the rest of the codebase. Use t.Setenv() instead of manual os.Setenv/defer os.Unsetenv for automatic cleanup. Simplify table structs where min and max are always equal. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
All six test functions now have // TestFoo verifies... comments matching the project's existing convention. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…debt and we should just remove the legacy workers argument entirely.
Author
|
Stuck as WIP while I work through manual testing. This doesn't seem to have resolved the issue in my local setup yet. |
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Summary
The
QUEUE_WORKERSenv var is supposed to control how many queue workers get created. This is handled byinternal/queue/queue.go:getOptimalWorkerCountwhich checks:However, when this value gets used, it's overwritten by a legacy workers argument without checking if the env var was set in
internal/queue/queue.go:NewTaskQueue:Since
NewTaskQueueis called unconditionally withlegacyWorkers = 2(seemain.go:130):this means that QUEUE_WORKERS is always ignored.
The right solution would seem to be to remove the
legacyWorkersargument entirely, since the behavior of selecting worker pool size ranges and auto-scaling is now handled ingetOptimalWorkerCount()instead of inmain.go. However, the fact that this seemingly-simple refactor wasn't done whengetOptimalWorkerCountwas first added suggests that it's not as simple as it seems, and as a new contributor, I'll err on the side of caution. As such, this PR only proposes ignoringlegacyWorkerswhenQUEUE_WORKERSis explicitly set. I've left a TODO comment recommending thatlegacyWorkersbe properly deprecated and removed when practical.This development followed TDD. Tests were verified to fail before changes and work after them.
Fixes #379
Fixes #417
Fixes #428
Disclaimer: Used Claude Code to find bug and implement changes. All code and comments are fully manually reviewed.