Allow NUM_WORKERS override #2776
Conversation
|
|
WalkthroughAdds a new NUM_WORKERS environment variable and Env getter, implements concrete numWorkers() in the base config, removes hard-coded overrides in Preprod/Prod, keeps DevConfig unchanged (whitespace only), adds startup.sh pre-run validation for NUM_WORKERS in non-dev environments, and propagates NUM_WORKERS through CI/deploy scripts. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CI as CI/Workflow
participant Deploy as deploy.sh
participant Host as Startup Script
participant Config as DefaultServerConfig
participant Env as Env getter
note right of CI `#f9f0c1`: CI forwards NUM_WORKERS
CI->>Deploy: set NUM_WORKERS env
Deploy->>Host: start container (NUM_WORKERS present)
Host->>Env: read NUM_WORKERS (if GAME_ENV != dev)
Env-->>Host: return string or undefined
Host->>Config: Config.numWorkers() called
Config->>Env: Env.NUM_WORKERS -> string
Config-->>Host: parsed and validated worker count (number)
alt NUM_WORKERS missing in non-dev
Host-->>Host: print error & exit
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks✅ Passed checks (5 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 1
🧹 Nitpick comments (2)
src/core/configuration/DefaultConfig.ts (1)
164-173: Consider adding JSDoc for clarity.Adding documentation would help other developers understand the method's purpose, validation rules, and behavior with decimal inputs.
🔎 Suggested documentation
+/** + * Reads the NUM_WORKERS environment variable with validation. + * @param defaultValue - Fallback value when NUM_WORKERS is unset or invalid + * @returns The number of workers (floored to integer) or the default value + */ envNumWorkers(defaultValue: number): number { const raw = Env.NUM_WORKERS; if (!raw) return defaultValue; const parsed = Number(raw); if (!Number.isFinite(parsed) || parsed <= 0) { console.warn(`Invalid NUM_WORKERS value "${raw}", using ${defaultValue}`); return defaultValue; } return Math.floor(parsed); }src/core/configuration/ProdConfig.ts (1)
4-17: Consider composition pattern for future refactoring.The class inheritance pattern works but could be simplified using composition with typed configuration objects instead of class hierarchies. This would make the code easier to test and reason about.
Example approach for future consideration:
type ServerConfig = { numWorkers: number; env: GameEnv; jwtAudience: string; turnstileSiteKey: string; }; export const prodConfig: ServerConfig = { numWorkers: envNumWorkers(20), env: GameEnv.Prod, jwtAudience: "openfront.io", turnstileSiteKey: "0x4AAAAAACFLkaecN39lS8sk", };
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
example.envsrc/core/configuration/DefaultConfig.tssrc/core/configuration/DevConfig.tssrc/core/configuration/Env.tssrc/core/configuration/PreprodConfig.tssrc/core/configuration/ProdConfig.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/core/configuration/DefaultConfig.ts (1)
src/core/configuration/Env.ts (1)
Env(47-95)
🔇 Additional comments (5)
example.env (1)
17-18: Documentation looks clear.The commented example provides clear guidance on the optional override.
src/core/configuration/PreprodConfig.ts (1)
8-10: Delegation pattern looks correct.The method properly delegates to
envNumWorkerswith a sensible default for the preprod environment.src/core/configuration/DevConfig.ts (1)
31-33: Delegation looks good.The change correctly routes through the base class validation method while maintaining the appropriate default for dev.
src/core/configuration/Env.ts (1)
92-94: Follows the established pattern.The getter implementation is consistent with the other environment variable accessors in this file.
src/core/configuration/ProdConfig.ts (1)
5-7: Validation is solid.The
envNumWorkersmethod inDefaultConfig.ts(lines 164–173) properly validates theNUM_WORKERSenvironment variable:
- Checks for finite numbers and positive values
- Falls back to the environment-specific default (20 for production) on error
- Warns to the console when input is invalid
- Handles decimals with
Math.floor()The change correctly enables per-environment overrides while maintaining safe defaults.
| envNumWorkers(defaultValue: number): number { | ||
| const raw = Env.NUM_WORKERS; | ||
| if (!raw) return defaultValue; | ||
| const parsed = Number(raw); | ||
| if (!Number.isFinite(parsed) || parsed <= 0) { | ||
| console.warn(`Invalid NUM_WORKERS value "${raw}", using ${defaultValue}`); | ||
| return defaultValue; | ||
| } | ||
| return Math.floor(parsed); | ||
| } |
There was a problem hiding this comment.
Add an upper bound to prevent resource exhaustion.
The validation correctly checks for positive, finite numbers, but allows arbitrarily large values. Setting an excessive worker count (e.g., 10000) could cause performance degradation, resource contention, or container startup failures.
🔎 Suggested fix with reasonable upper bound
envNumWorkers(defaultValue: number): number {
const raw = Env.NUM_WORKERS;
if (!raw) return defaultValue;
const parsed = Number(raw);
- if (!Number.isFinite(parsed) || parsed <= 0) {
- console.warn(`Invalid NUM_WORKERS value "${raw}", using ${defaultValue}`);
+ if (!Number.isFinite(parsed) || parsed <= 0 || parsed > 128) {
+ console.warn(`Invalid NUM_WORKERS value "${raw}" (must be between 1 and 128), using ${defaultValue}`);
return defaultValue;
}
return Math.floor(parsed);
}The upper limit of 128 workers should be sufficient for most deployments while preventing accidental resource exhaustion. Adjust based on your infrastructure if needed.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| envNumWorkers(defaultValue: number): number { | |
| const raw = Env.NUM_WORKERS; | |
| if (!raw) return defaultValue; | |
| const parsed = Number(raw); | |
| if (!Number.isFinite(parsed) || parsed <= 0) { | |
| console.warn(`Invalid NUM_WORKERS value "${raw}", using ${defaultValue}`); | |
| return defaultValue; | |
| } | |
| return Math.floor(parsed); | |
| } | |
| envNumWorkers(defaultValue: number): number { | |
| const raw = Env.NUM_WORKERS; | |
| if (!raw) return defaultValue; | |
| const parsed = Number(raw); | |
| if (!Number.isFinite(parsed) || parsed <= 0 || parsed > 128) { | |
| console.warn(`Invalid NUM_WORKERS value "${raw}" (must be between 1 and 128), using ${defaultValue}`); | |
| return defaultValue; | |
| } | |
| return Math.floor(parsed); | |
| } |
🤖 Prompt for AI Agents
In src/core/configuration/DefaultConfig.ts around lines 164 to 173, the
envNumWorkers validation allows arbitrarily large positive numbers; add an upper
bound (suggested 128) to prevent resource exhaustion by checking parsed > 0 &&
parsed <= 128, log a warning if raw is out of range, and return
Math.floor(parsed) when valid; keep returning defaultValue on non-finite,
non-positive, or >128 values.
There was a problem hiding this comment.
I don't know the right number, so I ask someone else.
There was a problem hiding this comment.
@Aotumuri, that sounds like a good approach! Consulting with someone familiar with your infrastructure and deployment environment will help determine the appropriate upper limit based on your actual resource constraints and scaling needs.
The right maximum will depend on factors like:
- Available CPU cores in your container/server
- Memory resources
- Expected load distribution across workers
- Container orchestration limits (if using Kubernetes, Docker Compose, etc.)
Feel free to ping me if you need any further clarification once you've determined the appropriate limit. 🐰
🐰💭
ears perked up thoughtfully
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
evanpelle
left a comment
evanpelle
left a comment
There was a problem hiding this comment.
I think we should make num workers a required argument when starting the container. (except for dev, we can leave at 2).
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
example.envsrc/core/configuration/DefaultConfig.tssrc/core/configuration/DevConfig.tssrc/core/configuration/PreprodConfig.tssrc/core/configuration/ProdConfig.tsstartup.sh
💤 Files with no reviewable changes (2)
- src/core/configuration/PreprodConfig.ts
- src/core/configuration/ProdConfig.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- example.env
- src/core/configuration/DevConfig.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/core/configuration/DefaultConfig.ts (1)
src/core/configuration/Env.ts (1)
Env(47-95)
🔇 Additional comments (1)
src/core/configuration/DefaultConfig.ts (1)
164-174: Implementation looks solid for required configuration.The strict error-throwing approach is appropriate for mandatory production configuration. The validation correctly checks for finite positive numbers and returns an integer value.
Note: The upper bound discussion from the previous review is still ongoing—once you've consulted with your infrastructure team about the right maximum worker count, that validation can be added as discussed.
github-project-automation
bot
moved this from Final Review
to Development
in OpenFront Release Management
evanpelle
left a comment
evanpelle
left a comment
There was a problem hiding this comment.
Make sure the environment arg is passed to the update script in deploy.sh. You'll also need to update release.yml and deploy.yml, use vars.NUM_WORKERS, and i can add the var to github
src/core/configuration/DevConfig.ts
Outdated
| const raw = Env.NUM_WORKERS; | ||
| if (!raw) return 2; | ||
| const parsed = Number(raw); | ||
| if (!Number.isFinite(parsed) || parsed <= 0) { | ||
| throw new Error(`Invalid NUM_WORKERS value "${raw}"`); | ||
| } | ||
| return Math.floor(parsed); |
There was a problem hiding this comment.
let's just do: "return 2" to keep it simple
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
deploy.sh (1)
143-143: NUM_WORKERS successfully propagated to deployment environment.The implementation correctly passes NUM_WORKERS to the remote server's environment file, following the same pattern as other configuration variables.
Optional: Add validation for clearer error messages
Consider adding validation before deployment to fail fast if NUM_WORKERS is required but not set. This would provide clearer error messages during the deployment phase rather than later during container startup:
# Check required environment variables if [ -z "$SERVER_HOST" ]; then echo "Error: ${HOST} not defined in .env file or environment" exit 1 fi + +# Validate NUM_WORKERS for non-dev environments +if [ "$ENV" != "dev" ] && [ -z "$NUM_WORKERS" ]; then + echo "Error: NUM_WORKERS must be set for ${ENV} environment" + exit 1 +fiThis is optional since startup.sh already validates NUM_WORKERS presence.
.github/workflows/deploy.yml (1)
115-115: NUM_WORKERS correctly added to deployment environment.The implementation properly reads NUM_WORKERS from GitHub Actions repository variables and passes it to the deployment script.
Optional: Document required GitHub Actions variable
Consider adding a note in the workflow comments or project documentation that NUM_WORKERS must be configured as a repository variable for non-dev deployments. For example, near line 107 before the deploy step:
# NUM_WORKERS repository variable should be configured for production deployments - name: 🚢 DeployThis helps maintainers understand required configuration.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/deploy.yml.github/workflows/release.ymldeploy.sh
🔇 Additional comments (1)
.github/workflows/release.yml (1)
73-73: NUM_WORKERS consistently added to all release deployment jobs.The implementation correctly propagates NUM_WORKERS to all four deployment targets (alpha, beta, blue, green), ensuring consistent configuration across the release pipeline.
Also applies to: 125-125, 177-177, 229-229
|
looks like ci is failing |
4 participants
Resolves #2610
Description:
Add NUM_WORKERS env override for server worker count.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
aotumuri