Deliver VCONN_CLOSE for parked TLS hooks; fix SNI queue accounting by moonchen · Pull Request #13406 · apache/trafficserver

moonchen · 2026-07-19T17:30:38Z

Rebased onto master after 508c1be and 7c0dfb0 landed. Those already fixed the inverted sweep
loop condition and the max_age expiry detach, so this PR no longer contains either. What remains is
a core hook-dispatch bug and the queued-connection half of the rate_limit accounting, which depends
on it.

A connection that closes while parked in a TLS handshake hook never reaches the plugin's close
hook. TLSEventSupport::callHooks() moves the hook state to DONE when a connection closes, but it
kept curHook pointing into whichever handshake hook list the connection was parked in. Each hook id
owns a separate list, so advancing curHook walked the handshake list rather than the close list: the
close event was dropped once that list ran out, and delivered to the next handshake plugin when it did
not. The fix restarts from the head of the close list unless we are already iterating it, and routes
TS_EVENT_VCONN_OUTBOUND_CLOSE through the same path, which previously invoked nothing at all for a
connection parked in the outbound pre-handshake hook.

In the rate_limit SNI queue the consequence is a freed TSVConn left on the queue and a leaked
selector lease, after which the next sweep reenables freed memory.

A connection that closes while still queued gives back a slot it never took. A connection parked
at the ClientHello hook holds no reservation; the sweep reserves the slot only when it resumes the
connection. The VCONN_CLOSE handler nonetheless called limiter->free() unconditionally, so a close
in that state decremented _active with no matching reserve(). Once the counter wraps, the next
reserve() trips TSReleaseAssert(_active <= _limit) and the server aborts. RateLimiter::remove()
now dequeues the connection and reports whether it was queued, and the handler frees a slot only when
it was not. Dequeuing also drops what would otherwise be a stale entry.

These land together because neither is complete alone: the plugin fix is unreachable until close
delivery works, and the core fix on its own makes the unmatched free() reachable and reintroduces
the abort. The plugin commits come first so no bisect point has the core fix without its prerequisite.

The sweep takes no lock. reserve(), free(), pop() and remove() are each internally
synchronized, and the one composite failure, a slot reserved when the queue turns out to be empty,
hands the slot straight back. An earlier revision of this PR serialized the sweep against close under
a plugin-global mutex; that is gone.

Tests. tls_hooks_close_while_parked drives the core fix with the existing ssl_hook_test.so,
using a delayed ClientHello hook and a close hook plus a 1s handshake timeout that fires inside the 2s
park. Reverting the core fix makes it fail with iterated to curHook=0x0 and no close callback. Three
rate_limit SNI autests cover queue-then-resume, max_age expiry and reject teardown.
rate_limit_sni_reject is new coverage rather than a regression test for this change.
rate_limit_sni_expiry overlaps rate_limit_sni.test.py in scenario, but asserts positively that
expiry ran and reads traffic.out, where ink_abort writes.

Not tested. The outbound close path is reasoned from the code, not exercised. No config-reload,
selector-teardown or concurrent-load coverage. RateLimiter::remove() is an O(queue-depth) scan on
every rate-limited close, measured by microbenchmark at 3ns for an empty queue, 270ns at depth 100 and
2.2us at depth 1000, but not measured under load. The rate_limit autests need coreutils timeout and
fail rather than skip without it.

Copilot

Pull request overview

Fixes correctness and concurrency issues in the experimental rate_limit plugin’s SNI queue handling during TLS handshakes, preventing active-slot counter underflow and stabilizing sweep vs. close interactions. Adds gold tests to regression-test the queue, expiry, and reject paths against a TLS listener.

Changes:

Fix SNI queue slot accounting by reserving before dequeue/resume, detaching expired queued VCs, and only releasing slots for VCs that actually own a slot.
Synchronize the periodic sweep and TS_EVENT_VCONN_CLOSE handling with a shared mutex to prevent sweep/close interleavings corrupting queue / slot / lease state.
Add new AuTest gold tests (plus bash+openssl clients) covering reject, queue, and max_age expiry behaviors.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
tests/gold_tests/pluginTest/rate_limit/rate_limit_sni_reject.test.py	Adds an autest covering the no-queue reject path under TLS.
tests/gold_tests/pluginTest/rate_limit/rate_limit_sni_reject_client.sh	Bash/openssl client to generate concurrent handshakes to trigger rejects.
tests/gold_tests/pluginTest/rate_limit/rate_limit_sni_queue.test.py	Adds a regression autest for queued-VC close vs. slot underflow.
tests/gold_tests/pluginTest/rate_limit/rate_limit_sni_queue_client.sh	Deterministic bash/openssl reproducer for the historical underflow scenario.
tests/gold_tests/pluginTest/rate_limit/rate_limit_sni_expiry.test.py	Adds a regression autest for the `max_age` expiry accounting path.
tests/gold_tests/pluginTest/rate_limit/rate_limit_sni_expiry_client.sh	Bash/openssl client to hold a slot while a queued VC ages out.
plugins/experimental/rate_limit/sni_selector.cc	Fixes sweep logic (reserve→pop→reenable) and detaches expired queued VCs; adds sweep/close synchronization.
plugins/experimental/rate_limit/sni_limiter.cc	Fixes close-time accounting (remove-if-queued vs free-slot) and serializes with sweep under the shared mutex.
plugins/experimental/rate_limit/limiter.h	Adds `remove()` to drop still-queued elements so queued closes don’t decrement the active slot counter.

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

cmcfarlen

A bit concerned that the rate_limit ops are now globally serialized. Consider finer grained locks.

cmcfarlen · 2026-08-04T19:22:44Z

+
+// Shared lock (defined in sni_limiter.cc) serializing the queue/slot transactions below
+// against the net-thread VCONN_CLOSE handler.
+extern std::mutex gQueueMutex;


Could the lock be per-limiter instead of global?

Could the lock be per-limiter instead of global?

I removed the global lock, so the only locking left is per-limiter: the
_queue_lock and _active_lock that RateLimiter already had.

cmcfarlen · 2026-08-04T19:24:15Z

+  {
+    std::lock_guard<std::mutex> lock(_queue_lock);
+
+    for (auto it = _queue.begin(); it != _queue.end(); ++it) {


how big can queue get?

how big can queue get?

Unbounded by default, which I agree is not sensible.

_max_queue is 0 (no queue) until a queue: block appears, and then
limiter.h:214 is:

_max_queue = queue["size"] ? queue["size"].as<uint32_t>() : UINT32_MAX;

A queue: block without a size: gets UINT32_MAX, and
full() (_size >= max_queue()) can then never trip. The practical ceiling
becomes proxy.config.net.connections_throttle, 30000 by default.

_queue is a std::deque, so an erase from the middle is O(n). I'm filing an issue: #13511 to update defaults, and revisit this queue data structure.

A queued SNI connection never reserves a slot, but its VCONN_CLOSE released one unconditionally. A queued connection that closed therefore decremented the active-slot counter without a matching increment; it wrapped below zero and the next reserve() aborted the server on TSReleaseAssert(_active <= _limit). Balance the accounting: resume queued connections with reserve-then-pop so a resumed connection owns a real slot; release a slot on close only when the connection is no longer queued (a still-queued one never held one) and drop it from the queue; detach an expired connection the same way the reject path does. Removing a closing connection from the queue also fixes a stale-pointer dereference when a parked queued connection is reset. Add deterministic regressions for the resume and max_age paths.

Exercise the sync-reject path against a TLS listener: a holder reserves the one slot and a burst of concurrent handshakes is rejected mid-handshake (TS_EVENT_ERROR) with the allocator freelists disabled. Asserts the reject path is reached and every rejected handshake VC is freed without a memory-safety fault.

Annotate the TestRun parameters like the surrounding class-based gold tests, and create the holder FIFO inside a fresh mktemp -d directory instead of on an unlinked mktemp -u path, whose creation is not atomic.

callHooks() moves the hook state to DONE when a connection closes, but it kept curHook pointing into whichever handshake hook list the connection was parked in. Each hook id owns a separate list, so advancing curHook walked the handshake list rather than the close list: the close event was dropped once that list ran out, and delivered to the next handshake plugin when it did not. A plugin that parks a connection therefore never learns that it died. In the rate_limit SNI queue that leaves a freed TSVConn on the queue and leaks the selector lease, and the next sweep reenables freed memory. Restart from the head of the close hook list unless we are already iterating it. Take the same path for TS_EVENT_VCONN_OUTBOUND_CLOSE, which previously invoked nothing at all for a connection parked in the outbound pre-handshake hook.

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.

Suppressed comments (6)

tests/gold_tests/tls_hooks/tls_hooks_close_while_parked.test.py:76

tr.StillRunningAfter is assigned twice, so the first assignment is overwritten. If the harness expects StillRunningAfter to track multiple processes, this will likely only assert one of them is still running. Use the framework’s supported way to register multiple processes (e.g., a list/collection API if available) so both ts and server are checked.

tr.StillRunningAfter = ts
tr.StillRunningAfter = server

tests/gold_tests/pluginTest/rate_limit/rate_limit_sni_queue_client.sh:53

These gold tests rely on the external timeout utility and on it supporting fractional durations (0.3). timeout (and fractional support) is not consistently available across all CI/OS environments (e.g., some BSD/macOS setups). Consider implementing the timeout behavior in a more portable way (e.g., via Python in the test harness, or a small helper that is already used elsewhere in this repo’s gold tests) to avoid platform-specific flakes.

timeout 0.3 ${OSSL} </dev/null >/dev/null 2>&1 &

tests/gold_tests/pluginTest/rate_limit/rate_limit_sni_queue_client.sh:62

These gold tests rely on the external timeout utility and on it supporting fractional durations (0.3). timeout (and fractional support) is not consistently available across all CI/OS environments (e.g., some BSD/macOS setups). Consider implementing the timeout behavior in a more portable way (e.g., via Python in the test harness, or a small helper that is already used elsewhere in this repo’s gold tests) to avoid platform-specific flakes.

timeout 2 ${OSSL} </dev/null >/dev/null 2>&1 || true

tests/gold_tests/pluginTest/rate_limit/rate_limit_sni_queue.test.py:28

The new rate_limit gold tests shell out to openssl s_client with flags that may not exist on non-OpenSSL implementations (e.g., LibreSSL) and will fail if openssl isn’t present. Add an explicit skip gate for the required client tooling/version (similar to the tls_hooks_close_while_parked test) so the suite skips cleanly rather than failing due to missing/unsupported openssl.

Test.SkipUnless(Condition.PluginExists('rate_limit.so'))

plugins/experimental/rate_limit/limiter.h:347

remove() does a linear scan and takes elem by value. If the queue can grow large, this makes closes O(n) and can become a hotspot under load. Consider (a) taking elem as const T& to avoid copies, and (b) if large queues are expected, maintaining an auxiliary index (e.g., map from element to iterator) to make removals O(1); alternatively document/enforce a small maximum queue size to bound the cost.

  bool
  remove(T elem)
  {
    std::lock_guard<std::mutex> lock(_queue_lock);

    for (auto it = _queue.begin(); it != _queue.end(); ++it) {
      if (std::get<0>(*it) == elem) {
        _queue.erase(it);
        --_size;
        return true;
      }
    }

    return false;
  }

plugins/experimental/rate_limit/sni_selector.cc:231

When pop() returns nullptr, the code frees the reserved slot and breaks out of the loop. If the queue becomes non-empty again immediately after (or if size() was stale due to concurrent modifications), the sweep won’t attempt to resume other queued VCs until the next sweep tick. Consider continue-ing after free() (or re-checking size() under the same synchronization used by pop()) to make the loop more robust and reduce avoidable resume latency.

      while (limiter->size() > 0 && limiter->reserve() == ReserveStatus::RESERVED) {
        auto [vc, contp, start_time] = limiter->pop();

        if (nullptr == vc) { // A concurrent close emptied the queue; give the slot back
          limiter->free();
          break;
        }

Drop the dependency on coreutils "timeout", which is absent on macOS and made the gold tests fail rather than skip there, and which was relied on for fractional deadlines. A small sleep-and-kill helper replaces it. Also drop -verify_quiet, which is redundant with -quiet and is not accepted by every s_client implementation. Take the element by const reference in RateLimiter::remove(), and record what bounds the scan: the configured queue size, or connections_throttle when a "queue" is given without a "size". Correct the queue test's narration. It described the counter wrapping and the probe aborting the server, which is what happened before 508c1be fixed the sweep's resume condition; the test now pins that fix rather than reproducing it.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

cmcfarlen · 2026-08-07T17:49:25Z

Both of my earlier points are addressed, and I verified them rather than taking the replies at face value. From my side this is clear.

Global lock → per-limiter. Confirmed: the only lock acquisition this PR adds is std::lock_guard<std::mutex> lock(_queue_lock) inside the new remove(), and _queue_lock/_active_lock are pre-existing per-limiter members (limiter.h:407, already used by reserve(), free(), push() and pop()). Grepping the diff for any static/global mutex turns up nothing. So remove() is exactly as granular as the methods it sits next to, and my serialization concern does not apply.

Queue depth. The inline comment on remove() now records the caveat where someone reading the O(n) erase will actually see it — that a queue: block without a size: leaves _max_queue at UINT32_MAX so full() never trips, leaving proxy.config.net.connections_throttle as the only real bound. With #13511 filed to fix the default and revisit the container, that is the right split: this PR stays a bug fix and the questionable default gets handled on its own.

While re-reading I went looking for a residual race and convinced myself it is closed, which is worth recording because it is the subtle part of the change. My worry was the check-then-act at the close site:

if (!limiter->remove(vc)) {
  limiter->free();
}

remove() works under _queue_lock and free() under _active_lock, so the decision and the decrement are not atomic with respect to each other. If the sweep could dequeue a VC and only afterwards reserve its slot, a close landing in that window would see "not queued", call free(), and decrement a slot nothing had taken — the unmatched decrement that wraps the counter and makes the next reserve() trip TSReleaseAssert(_active <= _limit), exactly the abort your test drives.

Reversing the sweep to reserve before dequeuing is what closes it, and the nullptr == vc give-back handles the case where the queue emptied underneath:

while (limiter->size() > 0 && limiter->reserve() == ReserveStatus::RESERVED) {
  auto [vc, contp, start_time] = limiter->pop();
  if (nullptr == vc) { // A concurrent close emptied the queue; give the slot back
    limiter->free();
    break;
  }

That makes the three cases add up: still queued, remove() returns true and no slot is released because none was held; already resumed, remove() returns false and free() releases precisely the slot the sweep granted; sweep raced and got nothing, the slot goes straight back. The max_age expiry loop is consistent too, since it pops without reserving and detaches with TSUserArgSet(vc, gVCIdx, nullptr) so the eventual VCONN_CLOSE cannot release a slot it never had. The "Reserving before dequeuing means a resumed VC owns the slot it was granted" comment is doing real work — worth keeping verbatim.

The TLSEventSupport.cc half reads correctly as well: with a connection closing while parked in a handshake hook, curHook still points into the handshake hook's list, and since each hook id owns a separate list, advancing it would walk the handshake list instead of the close list — dropping the close event or handing it to the wrong plugin. Resetting to nullptr unless already in HANDSHAKE_HOOKS_DONE (i.e. unless already iterating the close list) is the right guard, and picking up TS_EVENT_VCONN_OUTBOUND_CLOSE alongside TS_EVENT_VCONN_CLOSE closes the same gap on the outbound side.

I will leave the formal approval to a separate action so my CHANGES_REQUESTED stops blocking. One process note, purely about release timing rather than the code: this grew from a focused rate_limit accounting fix to +621/-5 across 11 files with the VCONN_CLOSE delivery work folded in, and 10.2.0 is days from its RC. The two halves are genuinely related — the parked-hook close is what produces the queued-close accounting path — so I am not asking for a split on technical grounds. It is worth an explicit RM call on whether this lands in 10.2.0 or 10.2.1 given the Core/TLS blast radius.

…13406) * rate_limit: balance the SNI active-slot counter for queued connections A queued SNI connection never reserves a slot, but its VCONN_CLOSE released one unconditionally. A queued connection that closed therefore decremented the active-slot counter without a matching increment; it wrapped below zero and the next reserve() aborted the server on TSReleaseAssert(_active <= _limit). Balance the accounting: resume queued connections with reserve-then-pop so a resumed connection owns a real slot; release a slot on close only when the connection is no longer queued (a still-queued one never held one) and drop it from the queue; detach an expired connection the same way the reject path does. Removing a closing connection from the queue also fixes a stale-pointer dereference when a parked queued connection is reset. Add deterministic regressions for the resume and max_age paths. * rate_limit: add an SNI reject-teardown autest Exercise the sync-reject path against a TLS listener: a holder reserves the one slot and a burst of concurrent handshakes is rejected mid-handshake (TS_EVENT_ERROR) with the allocator freelists disabled. Asserts the reject path is reached and every rejected handshake VC is freed without a memory-safety fault. * rate_limit tests: annotate helpers and create the FIFO atomically Annotate the TestRun parameters like the surrounding class-based gold tests, and create the holder FIFO inside a fresh mktemp -d directory instead of on an unlinked mktemp -u path, whose creation is not atomic. * Deliver VCONN_CLOSE for connections parked in a TLS handshake hook callHooks() moves the hook state to DONE when a connection closes, but it kept curHook pointing into whichever handshake hook list the connection was parked in. Each hook id owns a separate list, so advancing curHook walked the handshake list rather than the close list: the close event was dropped once that list ran out, and delivered to the next handshake plugin when it did not. A plugin that parks a connection therefore never learns that it died. In the rate_limit SNI queue that leaves a freed TSVConn on the queue and leaks the selector lease, and the next sweep reenables freed memory. Restart from the head of the close hook list unless we are already iterating it. Take the same path for TS_EVENT_VCONN_OUTBOUND_CLOSE, which previously invoked nothing at all for a connection parked in the outbound pre-handshake hook. * rate_limit: address review feedback Drop the dependency on coreutils "timeout", which is absent on macOS and made the gold tests fail rather than skip there, and which was relied on for fractional deadlines. A small sleep-and-kill helper replaces it. Also drop -verify_quiet, which is redundant with -quiet and is not accepted by every s_client implementation. Take the element by const reference in RateLimiter::remove(), and record what bounds the scan: the configured queue size, or connections_throttle when a "queue" is given without a "size". Correct the queue test's narration. It described the counter wrapping and the probe aborting the server, which is what happened before 508c1be fixed the sweep's resume condition; the test now pins that fix rather than reproducing it. (cherry picked from commit b9b9109)

cmcfarlen · 2026-08-09T17:25:09Z

Cherry-picked to the 10.2.x branch as 6b00633 for the 10.2.0 release.

Copilot AI lite review requested due to automatic review settings July 19, 2026 17:30

moonchen added Plugins Bug rate_limit rate_limit plugin labels Jul 19, 2026

Copilot started reviewing on behalf of moonchen July 19, 2026 17:31 View session

Copilot AI reviewed Jul 19, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 19, 2026 17:37

moonchen force-pushed the rate-limit-sni-queue-fixes branch from f7e15a5 to 516e40c Compare July 19, 2026 17:37

Copilot started reviewing on behalf of moonchen July 19, 2026 17:38 View session

moonchen requested a review from zwoop July 19, 2026 17:40

Copilot AI reviewed Jul 19, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 19, 2026 20:43

Copilot started reviewing on behalf of moonchen July 19, 2026 20:43 View session

Copilot AI reviewed Jul 19, 2026

View reviewed changes

moonchen self-assigned this Jul 19, 2026

moonchen added this to the 11.0.0 milestone Jul 19, 2026

moonchen added this to ATS v10.2.x Jul 19, 2026

cmcfarlen self-requested a review August 3, 2026 23:14

cmcfarlen requested changes Aug 4, 2026

View reviewed changes

moonchen added 4 commits August 4, 2026 17:17

rate_limit tests: annotate helpers and create the FIFO atomically

e724b54

Annotate the TestRun parameters like the surrounding class-based gold tests, and create the holder FIFO inside a fresh mktemp -d directory instead of on an unlinked mktemp -u path, whose creation is not atomic.

moonchen force-pushed the rate-limit-sni-queue-fixes branch from 5b1dece to 313a70f Compare August 5, 2026 14:39

Copilot AI review requested due to automatic review settings August 5, 2026 14:39

moonchen added Core TLS labels Aug 5, 2026

moonchen changed the title ~~rate_limit: fix SNI queue slot accounting and the sweep/close race~~ Deliver VCONN_CLOSE for parked TLS hooks; fix SNI queue accounting Aug 5, 2026

Copilot AI reviewed Aug 5, 2026

View reviewed changes

Copilot started reviewing on behalf of moonchen August 5, 2026 14:57 View session

Copilot AI review requested due to automatic review settings August 6, 2026 18:35

Copilot AI reviewed Aug 6, 2026

Copilot started reviewing on behalf of moonchen August 6, 2026 22:52 View session

cmcfarlen approved these changes Aug 7, 2026

View reviewed changes

cmcfarlen merged commit b9b9109 into apache:master Aug 7, 2026
15 checks passed

github-project-automation Bot moved this to For v10.2.0 in ATS v10.2.x Aug 7, 2026

cmcfarlen mentioned this pull request Aug 9, 2026

Cherry-picks for 10.2.0 RC, round 3 (2026-08-09) #13520

Merged

cmcfarlen moved this from For v10.2.0 to Picked v10.2.0 in ATS v10.2.x Aug 9, 2026

cmcfarlen modified the milestones: 11.0.0, 10.2.0 Aug 9, 2026

Uh oh!

Conversation

moonchen commented Jul 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

cmcfarlen left a comment

Choose a reason for hiding this comment

Uh oh!

cmcfarlen Aug 4, 2026

Choose a reason for hiding this comment

Uh oh!

moonchen Aug 6, 2026

Choose a reason for hiding this comment

Uh oh!

cmcfarlen Aug 4, 2026

Choose a reason for hiding this comment

Uh oh!

moonchen Aug 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

cmcfarlen commented Aug 7, 2026

Uh oh!

Uh oh!

cmcfarlen commented Aug 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

moonchen commented Jul 19, 2026 •

edited

Loading