perf: poison non-HTTP flows to avoid repeated parse-fail-clear cycles#42
perf: poison non-HTTP flows to avoid repeated parse-fail-clear cycles#42
Conversation
Add per-direction poisoned flag to HttpFlowState. After the first parse error on a direction with no prior successful parse, mark it poisoned and skip all future buffering/parsing for that direction. - request_poisoned / response_poisoned bools on HttpFlowState - non_http_flows counter in HttpAnalyzer, surfaced in summarize() - Updated test to verify poisoned direction skips subsequent data Reduces parse_errors from 14 to 2 on http-full.cap fixture. Fixes #18
- Add POISON_THRESHOLD (3 errors) before poisoning to tolerate mid-stream joins where first segments are body data - Fix non_http_flows double-counting: use per-flow counted_as_non_http flag so counter increments once per flow, not once per direction - Add poisoned_bytes_skipped counter for observability of discarded data - Add tests: threshold behavior, direction independence, flow counter accuracy, poison cleared after flow close
There was a problem hiding this comment.
Pull request overview
This PR improves HTTP analyzer performance on non-HTTP TCP streams by “poisoning” a flow direction after repeated header-parse failures, preventing repeated parse-fail-clear cycles and adding summary counters for observability.
Changes:
- Add per-direction poisoning state and counters to
HttpFlowState(with aPOISON_THRESHOLDof 3). - Skip future data for poisoned directions while tracking
non_http_flowsandpoisoned_bytes_skipped. - Expand HTTP analyzer tests to cover poisoning threshold behavior, per-direction independence, per-flow counting, and reset on flow close.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/analyzer/http.rs |
Introduces per-direction poisoning logic and new summary metrics to avoid repeated parsing on non-HTTP traffic. |
tests/http_analyzer_tests.rs |
Adds/updates unit tests validating poisoning threshold, direction independence, per-flow counting, and cleanup on close. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Some(Err(e)) => { | ||
| if !had_success { | ||
| self.parse_errors += 1; | ||
| if let Some(state) = self.flows.get_mut(flow_key) { | ||
| state.request_error_count = state.request_error_count.saturating_add(1); | ||
| if state.request_error_count >= POISON_THRESHOLD { | ||
| state.request_poisoned = true; | ||
| if !state.counted_as_non_http { | ||
| state.counted_as_non_http = true; | ||
| self.non_http_flows += 1; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The code increments request_error_count toward POISON_THRESHOLD, but it never resets the counter after a successful request parse. That means poisoning is based on “total errors so far” rather than consecutive errors as described, and a flow could be poisoned even if valid HTTP was parsed in-between. Reset request_error_count to 0 on the success path (and similarly for responses).
| Some(Err(e)) => { | ||
| if !had_success { | ||
| self.parse_errors += 1; | ||
| if let Some(state) = self.flows.get_mut(flow_key) { | ||
| state.response_error_count = | ||
| state.response_error_count.saturating_add(1); | ||
| if state.response_error_count >= POISON_THRESHOLD { | ||
| state.response_poisoned = true; | ||
| if !state.counted_as_non_http { | ||
| state.counted_as_non_http = true; | ||
| self.non_http_flows += 1; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Same issue as requests: response_error_count is incremented toward POISON_THRESHOLD but never reset after a successful response parse, so poisoning is not actually based on consecutive errors. Reset response_error_count to 0 when a response is successfully parsed/drained.
| if state.request_error_count >= POISON_THRESHOLD { | ||
| state.request_poisoned = true; | ||
| if !state.counted_as_non_http { | ||
| state.counted_as_non_http = true; | ||
| self.non_http_flows += 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
non_http_flows is incremented whenever a direction becomes poisoned, but poisoning can happen for parse errors that still indicate HTTP (e.g., TooManyHeaders). This makes the metric name misleading and can inflate “non-HTTP” counts with malformed/abusive HTTP. Consider renaming the metric to something like poisoned_flows, or only incrementing non_http_flows for error kinds that strongly indicate non-HTTP traffic (and keeping a separate counter for poisoned flows).
| let valid = b"GET /index.html HTTP/1.1\r\nHost: example.com\r\n\r\n"; | ||
| analyzer.on_data(&fk, Direction::ClientToServer, valid, 0); | ||
|
|
||
| assert_eq!(analyzer.parse_error_count(), 3); // no new errors (poisoned, not retried) | ||
| assert!(analyzer.method_counts().get("GET").is_none()); // never parsed |
There was a problem hiding this comment.
These tests validate poisoning behavior, but they don’t assert the new poisoned_bytes_skipped observability counter. Adding an assertion here (e.g., that it increases by the length of the skipped valid request) would prevent regressions where data is skipped without being accounted for (or vice versa).
| let valid = b"GET /index.html HTTP/1.1\r\nHost: example.com\r\n\r\n"; | |
| analyzer.on_data(&fk, Direction::ClientToServer, valid, 0); | |
| assert_eq!(analyzer.parse_error_count(), 3); // no new errors (poisoned, not retried) | |
| assert!(analyzer.method_counts().get("GET").is_none()); // never parsed | |
| let valid = b"GET /index.html HTTP/1.1\r\nHost: example.com\r\n\r\n"; | |
| let poisoned_bytes_skipped_before = analyzer.poisoned_bytes_skipped(); | |
| analyzer.on_data(&fk, Direction::ClientToServer, valid, 0); | |
| assert_eq!(analyzer.parse_error_count(), 3); // no new errors (poisoned, not retried) | |
| assert!(analyzer.method_counts().get("GET").is_none()); // never parsed | |
| assert_eq!( | |
| analyzer.poisoned_bytes_skipped(), | |
| poisoned_bytes_skipped_before + valid.len() | |
| ); |
Summary
HttpFlowStateto skip non-HTTP TCP flows after repeated parse failuresPOISON_THRESHOLD(3 consecutive errors) to tolerate mid-stream joins where initial segments are body datanon_http_flowsper flow (not per direction) andpoisoned_bytes_skippedfor observabilityFixes #18
Test plan
test_parse_error_poisons_direction_after_threshold— 3 errors poison, 4th data skippedtest_single_error_does_not_poison— 1 error below threshold, next valid request parsestest_poison_request_does_not_affect_response— direction independencetest_non_http_flows_counts_per_flow_not_direction— counter accuracytest_poison_cleared_after_flow_close— poison doesn't persist across flow reusecargo fmtclean