Harden server with panic reduction and resource limits#14
Conversation
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (16)
📝 WalkthroughWalkthroughAdds rotating file logging for the detached server process (daily log rotation via ChangesServer Logging, Resource Limits, and Hardening
Sequence Diagram(s)sequenceDiagram
participant CLI as embers CLI
participant Ensure as ensure_server_process
participant ServerProc as Detached Server Process
participant InitServer as init_server_tracing
participant RollingAppender as Rolling File Appender
participant Runtime as Runtime
participant Client as Connected Client
CLI->>CLI: log_filter = cli.log_filter()
CLI->>Ensure: ensure_server_process(socket, log_filter)
Ensure->>ServerProc: spawn with EMBERS_LOG=log_filter
ServerProc->>InitServer: init_server_tracing(log_filter, socket_dir)
InitServer->>RollingAppender: create daily-rotating appender
ServerProc->>ServerProc: install panic hook for tracing
Client->>Runtime: SessionRequest::Create
Runtime->>Runtime: check sessions.len() vs max_sessions
alt at limit
Runtime-->>Client: ErrorCode::Conflict (EMBERS_MAX_SESSIONS)
else under limit
Runtime-->>Client: session created
end
Runtime->>Client: broadcast event via try_send
alt channel full (slow client)
Runtime->>Runtime: drop slow client subscription + warn
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 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. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/embers-server/src/server.rs (1)
3164-3173:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winBounded
send(...).awaitcan indefinitely block connection teardown under backpressure.When a client stops reading, the bounded outbound queue can fill and these
send(...).awaitcalls can block the read task indefinitely. That task then cannot finish detach/shutdown flows cleanly, and shutdown quiescing can hang waiting for connection tasks.💡 Proposed fix (fail-fast on saturated outbound queue)
- if outbound - .send(ServerEnvelope::Response(response)) - .await - .is_err() - { - return Err(MuxError::transport("connection writer closed")); - } + match outbound.try_send(ServerEnvelope::Response(response)) { + Ok(()) => {} + Err(mpsc::error::TrySendError::Closed(_)) + | Err(mpsc::error::TrySendError::Full(_)) => { + return Err(MuxError::transport("connection writer closed")); + } + }Also applies to: 3180-3192, 3198-3207, 3218-3223
🤖 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 `@crates/embers-server/src/server.rs` around lines 3164 - 3173, The bounded outbound queue can cause indefinite blocking when clients stop reading and backpressure fills the queue, preventing proper connection teardown and shutdown flows. Replace the blocking `.send(...).await` calls with a fail-fast approach using `try_send()` instead, so that when the outbound queue is saturated, the send operation fails immediately and returns an error rather than blocking the read task. This change should be applied to all the protocol error response sends in the connection handling logic, specifically in the locations around the protocol_error_response calls and other outbound.send() invocations that occur during frame processing and error handling.
🤖 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 `@crates/embers-server/src/server.rs`:
- Around line 940-953: The buffer limit check at the BufferRequest::Create
handler only guards direct buffer creation but does not prevent helper buffers
created via the open_history_buffer path (using state.create_helper_buffer) from
bypassing the max_buffers limit. Locate where open_history_buffer or
state.create_helper_buffer is called and add the same capacity guard (checking
if state.buffers.len() >= self.limits.max_buffers) before allowing helper buffer
creation, returning the same conflict error response if the limit would be
exceeded.
---
Outside diff comments:
In `@crates/embers-server/src/server.rs`:
- Around line 3164-3173: The bounded outbound queue can cause indefinite
blocking when clients stop reading and backpressure fills the queue, preventing
proper connection teardown and shutdown flows. Replace the blocking
`.send(...).await` calls with a fail-fast approach using `try_send()` instead,
so that when the outbound queue is saturated, the send operation fails
immediately and returns an error rather than blocking the read task. This change
should be applied to all the protocol error response sends in the connection
handling logic, specifically in the locations around the protocol_error_response
calls and other outbound.send() invocations that occur during frame processing
and error handling.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 18b96774-6b7c-44b7-81a4-427c86a7c5f6
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
Cargo.tomlREADME.mdcrates/embers-cli/src/bin/embers-cli.rscrates/embers-cli/src/lib.rscrates/embers-cli/src/main.rscrates/embers-cli/tests/interactive.rscrates/embers-core/Cargo.tomlcrates/embers-core/src/diagnostics.rscrates/embers-core/src/lib.rscrates/embers-server/src/buffer_runtime.rscrates/embers-server/src/config.rscrates/embers-server/src/lib.rscrates/embers-server/src/server.rscrates/embers-server/src/state.rscrates/embers-server/src/terminal_backend.rsdocs/configuration.md
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@crates/embers-server/src/config.rs`:
- Around line 115-126: The test function that calls parse_limit_env with unsafe
environment variable mutations lacks serialization guarantees and will race when
tests run in parallel. Add the serial_test crate as a test dependency and
annotate this test function with the #[serial] attribute to ensure it executes
serially and prevents concurrent access to the global environment state. This
guarantees the SAFETY comment's assumption of single-threaded execution holds
true even when cargo test runs multiple tests concurrently.
In `@crates/embers-server/src/terminal_backend.rs`:
- Line 159: The scrolling_history field is being initialized from the
environment using from_env() which can be overridden by client-supplied
environment overlays, allowing clients to bypass the operator-configured
scrollback limit. Instead of reading from the environment at runtime
initialization, use a server-enforced scrollback limit that is set from the
environment once during server startup and then reused consistently across all
buffer spawns, ensuring the limit cannot be bypassed by client-supplied
environment variables.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 3d801d66-283f-494e-8640-f1a83eba4177
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
Cargo.tomlREADME.mdcrates/embers-cli/src/bin/embers-cli.rscrates/embers-cli/src/lib.rscrates/embers-cli/src/main.rscrates/embers-cli/tests/interactive.rscrates/embers-core/Cargo.tomlcrates/embers-core/src/diagnostics.rscrates/embers-core/src/lib.rscrates/embers-server/src/buffer_runtime.rscrates/embers-server/src/config.rscrates/embers-server/src/lib.rscrates/embers-server/src/server.rscrates/embers-server/src/state.rscrates/embers-server/src/terminal_backend.rsdocs/configuration.md
| // SAFETY: single-threaded test; we set and remove the var within it. | ||
| let var = "EMBERS_TEST_PARSE_LIMIT_ENV"; | ||
| for value in ["0", "", " ", "nope", "-5"] { | ||
| unsafe { std::env::set_var(var, value) }; | ||
| assert_eq!( | ||
| parse_limit_env(var), | ||
| None, | ||
| "value {value:?} should be rejected" | ||
| ); | ||
| } | ||
| unsafe { std::env::set_var(var, " 42 ") }; | ||
| assert_eq!(parse_limit_env(var), Some(42)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find env mutation sites in Rust tests/source.
rg -n --type rust 'std::env::set_var|std::env::remove_var' crates
# Check for explicit test serialization markers/usages.
rg -n --type rust '#\[(serial|serial_test::serial)\]|serial_test' .
# Check common CI/workflow files for single-threaded test execution.
fd -HI '.github|Cargo.toml|Makefile|justfile' | xargs -I{} sh -c 'echo "---- {} ----"; rg -n "test-threads|RUST_TEST_THREADS|cargo test" "{}" || true'Repository: Pajn/Embers
Length of output: 323
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look at the test function definition and surrounding context in the file
echo "=== Test function context ==="
rg -B5 -A10 'parse_limit_env.*value' crates/embers-server/src/config.rs | head -40
# Check for any #[test] or serialization attributes on this test
echo -e "\n=== Test attributes around lines 115-130 ==="
sed -n '110,130p' crates/embers-server/src/config.rs
# Check Cargo.toml for test-related configurations
echo -e "\n=== Cargo.toml test configurations ==="
rg -A5 '\[\[test\]\]|\[profile.test\]|test-threads' Cargo.toml crates/embers-server/Cargo.toml 2>/dev/null || true
# Check if there's a .cargo/config.toml with test settings
echo -e "\n=== .cargo/config.toml ==="
cat .cargo/config.toml 2>/dev/null || echo "No .cargo/config.toml found"Repository: Pajn/Embers
Length of output: 85
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the specific file and show lines around the test
echo "=== Lines 100-135 of config.rs ==="
sed -n '100,135p' crates/embers-server/src/config.rs
# Find the test function name containing this code
echo -e "\n=== Search for test function definition ==="
rg -B20 'parse_limit_env.*value.*None' crates/embers-server/src/config.rs | head -50Repository: Pajn/Embers
Length of output: 1180
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for any CI configuration files that might control test execution
echo "=== GitHub Actions workflows ==="
find .github -type f -name '*.yml' -o -name '*.yaml' 2>/dev/null | while read f; do
echo "---- $f ----"
cat "$f" | grep -A5 -B5 'test\|TEST_THREADS\|test-threads' || echo "No test config found"
done
# Check root Cargo.toml for any profile configurations
echo -e "\n=== Root Cargo.toml test profile ==="
cat Cargo.toml | grep -A10 '\[profile.test\]' || echo "No test profile found"
# Final confirmation: check if serial_test is in dependencies anywhere
echo -e "\n=== Check for serial_test dependency ==="
rg 'serial_test|serial-test' --type toml . || echo "serial_test not found in dependencies"Repository: Pajn/Embers
Length of output: 3897
Unsafe env mutation test lacks serialization guarantees.
This test mutates process-wide environment variables without any serialization mechanism. The SAFETY comment assumes single-threaded execution, but the test harness runs tests in parallel by default—cargo test --workspace in CI does not specify --test-threads=1, and no serial_test dependency or #[serial] attribute exists to enforce ordering. Concurrent test execution could race on std::env::set_var calls.
🤖 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 `@crates/embers-server/src/config.rs` around lines 115 - 126, The test function
that calls parse_limit_env with unsafe environment variable mutations lacks
serialization guarantees and will race when tests run in parallel. Add the
serial_test crate as a test dependency and annotate this test function with the
#[serial] attribute to ensure it executes serially and prevents concurrent
access to the global environment state. This guarantees the SAFETY comment's
assumption of single-threaded execution holds true even when cargo test runs
multiple tests concurrently.
1 participant
Summary by CodeRabbit
--log-level/--logand environment-variable overrides) across foreground and detached server modes.