Repository navigation
perf: full hot-path review — limiter, signature, path-shape optimizations - #3
Conversation
Optimize string-key limiters, adaptive detector windows, and upstream circuit breaker shards to use get_mut fast paths instead of repeated contains_key/entry/to_string triple lookups. Hash request signatures incrementally with blake3 to avoid an intermediate format! basis string. Reuse precomputed path_shape across filter evaluation, adaptive observe hooks, and rate-limit rejection telemetry.
|
Warning Review limit reached
Next review available in: 29 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds an optional path-shape hint parameter to filter evaluation ( ChangesPath-shape aware filtering and map refactors
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant handle_http
participant RateLimiters
participant AdaptiveDetector
participant FilterEngine
Client->>handle_http: HTTP request
handle_http->>RateLimiters: maybe_rate_limit_response(ctx, path_shape)
RateLimiters->>AdaptiveDetector: observe_with_path_shape(ctx, path_shape)
handle_http->>FilterEngine: evaluate_with_path_shape(ctx, path_shape)
FilterEngine-->>handle_http: FilterDecision
handle_http-->>Client: response
Related PRs: None identified. Suggested labels: refactor, performance Suggested reviewers: None identified. Poem 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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.
🧹 Nitpick comments (2)
src/limiter.rs (1)
283-294: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant HashSet lookup for distinct-token tracking.
contains(token)followed byinsert(token_owned.clone())performs two hash lookups whereinsert's own return value already indicates whether the value was newly added. This is on the per-request short-token-sibling rate-limit path and runs counter to this PR's stated goal of eliminating redundant lookups.♻️ Use insert's return value instead of contains+insert
- if !entry.distinct_tokens.contains(token) { - let token_owned = token.to_string(); - entry.distinct_tokens.insert(token_owned.clone()); - entry.token_order.push_back(token_owned); + let token_owned = token.to_string(); + if entry.distinct_tokens.insert(token_owned.clone()) { + entry.token_order.push_back(token_owned); while entry.distinct_tokens.len() > SHORT_TOKEN_SIBLING_DISTINCT_CAP { let Some(oldest) = entry.token_order.pop_front() else { break; }; entry.distinct_tokens.remove(&oldest); } }🤖 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 `@src/limiter.rs` around lines 283 - 294, The distinct-token tracking in the limiter path does an unnecessary double lookup by calling contains(token) before insert; update the short-token sibling logic in the limiter method that manages entry.distinct_tokens and entry.token_order to rely on insert’s return value instead. Use the result of inserting the owned token to decide whether to push into the order queue and run the SHORT_TOKEN_SIBLING_DISTINCT_CAP eviction loop, while keeping the existing token ownership and eviction behavior unchanged.src/filter.rs (1)
761-815: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnify the signature-basis emitter
append_signature_basis_stringandappend_signature_basisstill duplicate the same field order. A small shared emitter would keeprequest_signature,legacy_request_signature, andsignature_basisfrom drifting apart.🤖 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 `@src/filter.rs` around lines 761 - 815, The signature-basis construction logic is duplicated between append_signature_basis_string and append_signature_basis, which can cause request_signature, legacy_request_signature, and signature_basis to diverge. Refactor the shared field emission into one helper used by both functions, keeping the exact same ordering and normalization for method, normalized path, query shape, USER_AGENT family, and ACCEPT class. Preserve the current behavior in request_signature, legacy_request_signature, and signature_basis by routing all three through the same emitter logic.
🤖 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.
Nitpick comments:
In `@src/filter.rs`:
- Around line 761-815: The signature-basis construction logic is duplicated
between append_signature_basis_string and append_signature_basis, which can
cause request_signature, legacy_request_signature, and signature_basis to
diverge. Refactor the shared field emission into one helper used by both
functions, keeping the exact same ordering and normalization for method,
normalized path, query shape, USER_AGENT family, and ACCEPT class. Preserve the
current behavior in request_signature, legacy_request_signature, and
signature_basis by routing all three through the same emitter logic.
In `@src/limiter.rs`:
- Around line 283-294: The distinct-token tracking in the limiter path does an
unnecessary double lookup by calling contains(token) before insert; update the
short-token sibling logic in the limiter method that manages
entry.distinct_tokens and entry.token_order to rely on insert’s return value
instead. Use the result of inserting the owned token to decide whether to push
into the order queue and run the SHORT_TOKEN_SIBLING_DISTINCT_CAP eviction loop,
while keeping the existing token ownership and eviction behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 29b1bc34-dd42-4069-bf13-f5c8922b31de
📒 Files selected for processing (4)
src/adaptive.rssrc/filter.rssrc/http_proxy.rssrc/limiter.rs
Probe admin endpoints with borrowed method/path before allocating signature and path-shape strings. Run body-size rejection with lazy derivatives only on the reject path. Ignore configs/runtime/ event-log side effects from local runs.
Body-size pre-check rejects must emit observed (weak) then body_too_large (strong) adaptive observations, matching pre-deferral behavior. Add regression test asserting signature observed_count is 2 for the hot-path sequence vs 1 for body_too_large alone.
1 participant
Summary
Systematic performance review of the AlturaProt request hot path, TCP relay, off-band telemetry/adaptive hooks, and Python benchmark tooling. All confirmed hot-path issues are fixed without changing DDoS/security guardrail semantics.
Modules reviewed
http_proxy.rs,limiter.rs,filter.rs,adaptive.rstcp_proxy.rstelemetry.rs,log_limiter.rslistener.rs,daemon.rsrun_local_bench.py,run_defense_bench.py,codex_analyzer.py,codexsdgate.pyHTTP hot-path findings & fixes
1. String-key limiter triple lookup (
limiter.rs)Finding:
KeyRateLimiter::checkandShortTokenSiblingRateLimiter::checkdidcontains_key→push_back(key.to_string())→entry(key.to_string())— up to three hash lookups and 2–3Stringallocations per new key on every check.Fix:
get_mut(key)fast path (zero alloc for existing keys); single-allocation insert for new keys; one tokenStringon first sibling sighting.2. Adaptive detector shards (
adaptive.rs)Finding:
observe_signature/observe_path_shaperepeated the same triple-lookup pattern on every observation under mutex hold.Fix: Same
get_mut+ single-insert pattern.3. Upstream circuit breaker (
http_proxy.rs)Finding:
record_failurecalledpath_shape.to_string()twice (order deque + map insert).Fix:
get_mutfast path with single-allocation insert.4. Redundant path-shape recompute (
http_proxy.rs)Finding: Proxy computed
path_shapeonce, butdetector.observe()and several rate-limit rejection hooks recomputed it viarequest_path_shape(ctx.path).Fix: Pass precomputed
path_shapethroughobserve_with_path_shapeon observe, body-too-large, filter-block, and all rate-limit rejection paths.5. Filter evaluation path-shape redo (
filter.rs)Finding:
FilterEngine::evaluaterecomputed path shape inFilterEvalScratcheven though the proxy already derived it.Fix:
evaluate_with_path_shape(ctx, Some(&path_shape))on the hot path.6. Signature basis
format!allocation (filter.rs)Finding:
request_signaturebuilt a full intermediatesignature_basisstring before blake3 hashing.Fix: Stream components into
blake3::Hasherviaappend_signature_basis; canonical string preserved for events/tests.7. Early-rejection allocation waste (
http_proxy.rs)Finding:
method.to_string()happened beforemethod_allowed;path/signature/path_shape/short_token_parentwere allocated before the admin fast-path probe and before the body-size pre-check.Fix: Use borrowed
method/path/querythrough validation; probeadmin_endpointwithout allocation; allocate derivatives only inside the admin branch or after body-size check passes (lazy alloc on body-too-large reject for telemetry only).8. Runtime artifact hygiene (
.gitignore)Finding: Local proxy runs wrote empty
configs/runtime/attack_events.jsonloutside existingruntime/gitignore rules.Fix: Added
configs/runtime/to.gitignoreand removed the stray file.TCP relay — concrete review findings (no change)
relay_directionuses a stack-allocated 16 KiB buffer (tcp_proxy.rs:400) — appropriate for loopback/local relay without heap churn per read.tokio::select!withbiasedshutdown priority andmpsc::channel(1)activity notifications — idle timeout does not add per-byte overhead.TcpRateGuard::observeonly activates after grace period; hot path for fast streams is two comparisons + saturating add.Telemetry — concrete review findings (no change)
EventLogger::logusesSyncSender::try_send(telemetry.rs:357) — O(1) enqueue, drops on full queue without blocking request tasks.Stats::incusesOrdering::Relaxedatomics — minimal fence cost on hot counters.Python tooling — concrete review findings (no change)
codex_analyzer.read_eventsuses a boundeddeque(maxlen=max_events)tail read (codex_analyzer.py:227) — O(file) scan but capped output, offline-only.deterministic_signature_filtersgroups by signature in a single pass (codex_analyzer.py:252-254) — already O(n) over events; analyzer runs post-bench, not in proxy hot path.run_local_bench.py/run_defense_bench.pysubprocess overhead is dominated by proxy runtime + flood workers; bench JSON shows identical 18-scenario coverage pre/post fix.Guardrails preserved
Verification evidence (scratch dir)
baseline-cargo-test.logrust-checks.log=== cargo fmt --check ===,=== cargo clippy ===,=== cargo test ===(370 pass)python-tools.logbaseline-local-bench.jsonlocal-bench.json+local-bench-assert.logbaseline-defense-bench.jsondefense-bench.json+defense-bench-assert.logbench-comparison.txtlaunch-origin-body.txtlaunch-run1.log/launch-run2.logPROXY_BODY_RUN{1,2}blocks; HTTP 200; bodies identical to originpr-summary.txtNon-changes rationale
TCP relay, telemetry, and Python analyzer findings above document why each area was inspected and why no patch was justified, per acceptance criterion 1.