in_forward: fix 100% CPU hang and connection teardown use-after-free#12044
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR hardens forward connection teardown and downstream destruction handling, and adds an integration test that churns idle TLS forward connections before verifying the listener still works. ChangesForward connection lifecycle fix
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 (1)
tests/integration/scenarios/in_forward/tests/test_in_forward_001.py (1)
1030-1030: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnused
monkeypatchparameter.The test never uses
monkeypatch. Consider dropping it unless a follow-up change needs it.♻️ Proposed fix
-def test_in_forward_tls_idle_connection_timeout_churn(monkeypatch): +def test_in_forward_tls_idle_connection_timeout_churn():🤖 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 `@tests/integration/scenarios/in_forward/tests/test_in_forward_001.py` at line 1030, The test function test_in_forward_tls_idle_connection_timeout_churn has an unused monkeypatch parameter, so remove it from the function signature unless it is needed for a future fixture change. Update the test definition to match the actual setup used in the body and keep the name-based locator unchanged so the test remains easy to find.
🤖 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 `@tests/integration/scenarios/in_forward/tests/test_in_forward_001.py`:
- Line 1030: The test function test_in_forward_tls_idle_connection_timeout_churn
has an unused monkeypatch parameter, so remove it from the function signature
unless it is needed for a future fixture change. Update the test definition to
match the actual setup used in the body and keep the name-based locator
unchanged so the test remains easy to find.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 06787a48-dc46-48de-915f-2b5c537b9300
📒 Files selected for processing (4)
plugins/in_forward/fw_conn.csrc/flb_downstream.ctests/integration/scenarios/in_forward/config/in_forward_conn_churn.yamltests/integration/scenarios/in_forward/tests/test_in_forward_001.py
edsiper
force-pushed
the
fix-12025-priority-queue-stale-event
branch
from
July 4, 2026 00:59
6d2e4a3 to
45eec99
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d2e4a3451
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A connection destroyed by the downstream timeout/pending-destroy machinery can still have its event linked in the priority bucket queue after mk_event_inject (only FLB_ENGINE_LOOP_MAX_ITER events are processed per loop round). destroy_conn freed the connection unconditionally, leaving a dangling _priority_head in the bucket queue. On the next loop iteration flb_bucket_queue_pop_min dereferenced the freed node (use-after-free), and the stale self-referential linkage made flb_event_load_bucket_queue_event skip the event forever, so a socket in CLOSE_WAIT kept waking epoll while its handler was never dispatched, spinning the input thread at 100% CPU. Mirror the guard already used by upstream destroy_conn: keep the connection in the destroy_queue until its event has drained from the bucket queue, then free it on a later sweep. Fixes #12025 Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
The plugin kept a fw_conn wrapper in ctx->connections for every active connection but never registered a drop notification callback. When the engine destroyed the underlying connection on its own (e.g. an IO timeout via prepare_destroy_conn), the wrapper was left pointing at a freed connection. The next fw_conn_del_all (backpressure pause or shutdown) then dereferenced it, causing a use-after-free crash. Register a drop_notification_callback that detaches and frees the wrapper when the engine reclaims the connection, mirroring the http_server pattern. Split the wrapper cleanup (fw_conn_release) from the downstream release so fw_conn_del and the drop path don't double-release, and guard fw_conn_event against a wrapper already dropped by an injected event that fires after teardown. Fixes #12025 Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Regression test for issue #12025: open many idle TLS connections so net.io_timeout expires for the whole batch in one downstream timeout sweep, repeat, then verify the listener survives and still ingests. This exercises the priority-queue teardown and the connection wrapper drop path that previously spun the input thread at 100% CPU and crashed on pause/shutdown. Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
edsiper
force-pushed
the
fix-12025-priority-queue-stale-event
branch
from
July 4, 2026 02:22
45eec99 to
7111417
Compare
Merged
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #12025. The
in_forwardinput thread could spin at 100% CPU with connections stuck inCLOSE_WAITand their event handlers never dispatched. Root cause is in the shared downstream connection teardown path; while reproducing it I also found and fixed a related use-after-free on the pause/shutdown path.Root cause
When a batch of connections times out in a single sweep, each gets
prepare_destroy_conn()+mk_event_inject(). The event loop only processesFLB_ENGINE_LOOP_MAX_ITER(10) events per round, butflb_downstream_conn_pending_destroy_list()freed all queued connections at the end of the round — including those whose injected event was still linked in the priority bucket queue via_priority_head.That produced two failures:
100% CPU hang (the reported issue). The freed connection left a stale, non-NULL
_priority_headin the bucket queue.flb_event_load_bucket_queue_event()skips it (prev == NULLcheck fails), so the readableCLOSE_WAITfd keeps waking epoll while its handler is never dispatched → the input thread spins. There is also a use-after-free when the freed node is popped on the next iteration.Pause/shutdown crash.
in_forwardnever registered a drop-notification callback, so when the engine reclaimed a timed-out connection the plugin'sfw_connwrapper was left pointing at freed memory. The nextfw_conn_del_all()(backpressure pause or shutdown) dereferenced it.The upstream connection path already guards against (1) — see e7caf86 — but the downstream path was missing the equivalent protection.
Fix
flb_downstream.c(primary): defer destroying a connection while its event is still linked in the priority bucket queue; it stays in the destroy queue and is freed on a later sweep once the event has drained. Mirrors the existing upstream guard.in_forward(fw_conn.c): register adrop_notification_callbackso the plugin wrapper is released in sync with the connection, matching the patternflb_http_serveralready uses. Splits wrapper cleanup from the downstream release to avoid double-release, and guardsfw_conn_eventagainst an injected event firing after teardown.Reproduction
Opening many idle TLS connections so
net.io_timeoutexpires for the whole batch in one sweep reproduces all three reported symptoms on the pre-fix build: forward thread at ~100% CPU with no clients connected, sockets inCLOSE_WAIT, and (via gdb) events with stale non-NULL_priority_head. Valgrind shows the use-after-free with the exact stack from the issue (freed indestroy_conn, read inflb_bucket_queue_pop_minfrominput_thread).Testing
test_in_forward_tls_idle_connection_timeout_churn) drives the idle-connection-timeout churn and asserts the listener survives and keeps ingesting.scenarios/in_forwardintegration suite passes.Note: the primary change is in shared core code (
flb_downstream.c) and affects all network inputs, not just forward.Summary by CodeRabbit