fix(frontend): raise soft RLIMIT_NOFILE at startup to avoid accept() EMFILE spiral - #11802
Conversation
|
👋 Hi mikekg! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
WalkthroughThe frontend defines a target soft file-descriptor limit, raises the current limit within the hard-limit bound when needed, and performs this setup before launching the asynchronous runtime. ChangesFrontend startup
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
components/src/dynamo/frontend/main.py (2)
64-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove
resourceto module scope.This standard-library import is inside
_raise_fd_limit(), hiding a module dependency. Add it with the existing standard-library imports and remove the function-local import.As per coding guidelines, imports must remain at module top.
Proposed change
+# with the other standard-library imports +import resource + def _raise_fd_limit(target: int = FRONTEND_FD_LIMIT_TARGET) -> None: - import resource🤖 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 `@components/src/dynamo/frontend/main.py` at line 64, Move the resource import from inside _raise_fd_limit() to the module-level standard-library imports, and remove the function-local import while leaving the function’s behavior unchanged.Sources: Coding guidelines, Path instructions
70-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse lazy logging formatting.
Pass values as logging arguments instead of constructing an f-string eagerly.
As per coding guidelines, logger calls must use lazy formatting.
Proposed change
logger.info( - f"Raised RLIMIT_NOFILE soft limit {soft} -> {new_soft} (hard={hard})" + "Raised RLIMIT_NOFILE soft limit %s -> %s (hard=%s)", + soft, + new_soft, + hard, )🤖 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 `@components/src/dynamo/frontend/main.py` around lines 70 - 72, Update the logger.info call reporting the RLIMIT_NOFILE change to use a format string with soft, new_soft, and hard passed as logging arguments, eliminating the eagerly constructed f-string while preserving the existing message.Sources: Coding guidelines, Path instructions
🤖 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 `@components/src/dynamo/frontend/main.py`:
- Around line 66-69: Update the RLIMIT_NOFILE handling around resource.getrlimit
so resource.RLIM_INFINITY, including negative sentinel representations, is
treated as an unlimited hard limit rather than passed to min. Preserve the
existing cap for finite hard limits, and add coverage for both finite and
unlimited cases.
---
Nitpick comments:
In `@components/src/dynamo/frontend/main.py`:
- Line 64: Move the resource import from inside _raise_fd_limit() to the
module-level standard-library imports, and remove the function-local import
while leaving the function’s behavior unchanged.
- Around line 70-72: Update the logger.info call reporting the RLIMIT_NOFILE
change to use a format string with soft, new_soft, and hard passed as logging
arguments, eliminating the eagerly constructed f-string while preserving the
existing message.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d2694af6-e839-4400-93f6-be2fcb3a3f5e
📒 Files selected for processing (1)
components/src/dynamo/frontend/main.py
…E spiral The frontend's TCP accept loop (lib/runtime/src/pipeline/network/tcp/server.rs) retries accept() with no backoff on failure. Under high concurrency with a non-keep-alive HTTP client, established connections can reach roughly 2x the nominal benchmark concurrency, and the default soft RLIMIT_NOFILE (1024 on most distros) gets exhausted, causing accept() to fail with EMFILE in a tight zero-backoff loop. Raise the soft limit at process startup, bounded by the hard limit. No-op if the environment already provides enough headroom. See issue ai-dynamo#11801. Signed-off-by: mikekg <mgschwind@nvidia.com>
4ff5352 to
e724b1e
Compare
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Mike G <mgschwind+mikekg@nvidia.com>
RitwijParmar
left a comment
There was a problem hiding this comment.
The EMFILE failure mode is real and the startup limit is a reasonable first layer. One portability issue needs attention before merge: resource is Unix-only, so importing it at module scope can make the frontend fail during import on Windows. I left an inline comment with the suggested lazy import and fallback. A small platform test would keep this safe across the supported runners.
`resource` is Unix-only, so a module-scope import broke main.py on Windows CI. Move the import into _raise_fd_limit and make the whole raise best-effort: no-op when resource is unavailable (Windows) or when setrlimit is denied (restricted env). Adds platform-safety unit tests. Addresses review feedback on ai-dynamo#11802. Signed-off-by: mikekg <mgschwind@nvidia.com>
Signed-off-by: Michael Gschwind <mgschwind@nvidia.com>
`resource` is Unix-only, so a module-scope import broke main.py on Windows CI. Move the import into _raise_fd_limit and make the whole raise best-effort: no-op when resource is unavailable (Windows) or when setrlimit is denied (restricted env). Adds platform-safety unit tests. Addresses review feedback on ai-dynamo#11802. Signed-off-by: mikekg <mgschwind@nvidia.com>
Per review, allow operators to tune the RLIMIT_NOFILE target at deploy time via DYN_FRONTEND_FD_LIMIT_TARGET (default 8192). A non-positive or non-integer value disables the raise, so it can be turned off without a code change. Signed-off-by: mikekg <mgschwind@nvidia.com>
58ea65e to
f672fd8
Compare
|
/ok to test f672fd8 |
RLIM_INFINITY is -1, so the old 'new_soft > soft' check saw an unlimited soft limit (-1) as smaller than the target and would setrlimit() it DOWN to 8192. Short-circuit when the soft limit is already RLIM_INFINITY. Signed-off-by: mikekg <mgschwind@nvidia.com>
|
/ok to test 1626577 |
|
/ok to test 1626577 |
Fixes the accept() failure spiral described in #11801.
The frontend's TCP accept loop (
lib/runtime/src/pipeline/network/tcp/server.rs) retriesaccept()with no backoff on failure. Under high concurrency with a non-keep-alive HTTP client, established connections can reach roughly 2x the nominal benchmark concurrency, and the default softRLIMIT_NOFILE(1024 on most distros) gets exhausted, causingaccept()to fail with EMFILE in a tight zero-backoff loop that also floods stdout (observed 15.8GB/86.9GB logs within minutes).This raises the process's soft
RLIMIT_NOFILEat startup (components/src/dynamo/frontend/main.py), bounded by whatever hard limit the environment provides. No-op if the environment already has enough headroom. No changes to the accept-loop logic itself.See #11801 for full repro details, environment, and logs.
Summary by CodeRabbit