apache / apache/trafficserver

crash on add_to_keep_alive_queue and add_to_active_queue

Open
#13,358 12 comments 0 reactions 2 assignees Claimed by @moonchen View on GitHub
Bug Crash Threads
Dominant language
C++
Stars
2k
Forks
874
Avg merge
6d 15h
Merged PRs (30d)
46

Description

# Crash: `ink_release_assert` in `UnixNetVConnection::add_to_keep_alive_queue()` / `add_to_active_queue()` — NetHandler queue mutated off the owner thread (HTTP/1.1 and HTTP/2)

## Summary

On a forward-proxy deployment doing TLS bump (MITM) + caching of decrypted HTTPS objects, `traffic_server` aborts intermittently with an `ink_release_assert` fired from `UnixNetVConnection::add_to_keep_alive_queue()` or `UnixNetVConnection::add_to_active_queue()`:

```
BUG: It must have acquired the NetHandler's lock before doing anything on keep_alive_queue.
BUG: It must have acquired the NetHandler's lock before doing anything on active_queue.
```

Root cause: the per-thread NetHandler queues (`active_queue` / `keep_alive_queue`) are mutated from a thread that is **not** the owner thread of the underlying `NetVConnection` (`vc->thread`). The `MUTEX_TRY_LOCK(nh->mutex, ...)` in those functions fails because the owner thread currently holds `nh->mutex`, and the code aborts instead of deferring/redirecting the operation to the owner thread.

This reproduces for **both HTTP/1.1 and HTTP/2**, and is **not** correlated with high load — it happens under normal, low traffic (< 1000 rps, ~7–8k concurrent connections, 32 ET_NET threads on a 32-core box; CPU only ~200–300%).

## Version

- Apache Traffic Server **10.1.2** (build Jun 24 2026), Linux x86_64, OpenSSL 1.1.1k, jemalloc.

## Configuration (relevant bits)

```yaml
records:
exec_thread:
autoconfig: { enabled: 1, scale: 1.0 }
limit: 32 # 32 ET_NET threads on a 32-core box
http:
server_ports: >
3080:ip-in=0.0.0.0:proto=http;http2
3443:ip-in=127.0.0.1:ssl:proto=http;http2
3180:ip-in=0.0.0.0:tr-in:proto=http;http2:pp
3444:ip-in=0.0.0.0:ssl:proto=http;http2:tr-in:pp
```

Forward proxy: client `CONNECT` on 3080 → local TLS listener 3443 → `certifier` plugin generates per-SNI certs (MITM) → decrypted HTTP objects are cached. HTTP/2 enabled on all inbound ports; PROXY protocol enabled on 3180/3444.

## Crash stacks (3 occurrences, same root cause)

### (A) HTTP/2, teardown path — `release_stream` → keep_alive_queue

```
_ink_assert
UnixNetVConnection::add_to_keep_alive_queue()
Http2ConnectionState::release_stream()
Http2Stream::~Http2Stream()
Http2Stream::terminate_if_possible()
Http2Stream::transaction_done()
HttpSM::kill_this()
HttpSM::main_handler()
HttpTunnel::main_handler()
CacheVC::openWriteMain(int, Event*) <-- driven by cache-write completion
EThread::process_event()
EThread::execute_regular()
```

### (B) HTTP/1.1, teardown path — `transaction_done` → keep_alive_queue

```
_ink_assert
UnixNetVConnection::add_to_keep_alive_queue()
Http1ClientTransaction::transaction_done()
HttpSM::kill_this()
HttpSM::main_handler()
HttpTunnel::main_handler()
CacheVC::openWriteMain(int, Event*) <-- driven by cache-write completion
EThread::process_event()
EThread::process_queue()
EThread::execute_regular()
```

### (C) HTTP/2, setup path — `create_stream` → active_queue

```
_ink_assert
UnixNetVConnection::add_to_active_queue()
Http2ConnectionState::create_stream()
Http2ConnectionState::rcv_headers_frame()
Http2ConnectionState::rcv_frame()
Http2CommonSession::do_complete_frame_read()
Http2CommonSession::do_process_frame_read()
Http2CommonSession::state_start_frame_read()
Http2ClientSession::main_event_handler()
EThread::process_event()
EThread::execute_regular() <-- dispatched from the event queue, not the net poll
```

The failing frame in all three:

```cpp
// UnixNetVConnection.cc
void UnixNetVConnection::add_to_keep_alive_queue() {
MUTEX_TRY_LOCK(lock, nh->mutex, this_ethread());
if (lock.is_locked()) { nh->add_to_keep_alive_queue(this); }
else { ink_release_assert(!"BUG: ... keep_alive_queue."); } // <-- abort
}
bool UnixNetVConnection::add_to_active_queue() {
MUTEX_TRY_LOCK(lock, nh->mutex, this_ethread());
if (lock.is_locked()) { return nh->add_to_active_queue(this); }
else { ink_release_assert(!"BUG: ... active_queue."); } // <-- abort
}
```

## Root cause analysis

1. Each ET_NET thread owns one `NetHandler`; at `initialize_thread_for_net()`, `nh->thread = thread` (bound once, never changes) and `nh->mutex = new_ProxyMutex()`. The `active_queue` / `keep_alive_queue` and their size counters are **per-thread private data**, meant to be mutated only on `nh->thread` (== the netvc owner thread `O`, i.e. `vc->thread`).

2. `add_to_*_queue()` uses `MUTEX_TRY_LOCK(nh->mutex)` + `ink_release_assert` as a **thread-affinity contract check**, not as optimistic lock retry: on the owner thread the lock is already held (reentrant → success); a foreign thread `T` fails the try-lock because `O` currently holds it. The abort therefore means "this ran on the wrong thread", not "transient lock contention". Retrying to acquire the lock would be wrong — mutating another thread's per-thread queue/counters is unsafe even if the lock were obtained.

3. **HTTP/1.1** partially guards this via `ProxyTransaction::adjust_thread()` (which reschedules onto the fixed `vc->thread`), called from `HttpSM::state_cache_open_write()`. But the cache-write **completion** teardown path (`CacheVC::openWriteMain` → `HttpTunnel` → `HttpSM::kill_this` → `Http1ClientTransaction::transaction_done` → `Http1ClientSession::release` → `add_to_keep_alive_queue`) does **not** pass through `state_cache_open_write` again, so it can run on a non-owner thread → stack (B).

4. **HTTP/2** aligns stream processing to `Http2Stream::_thread` (the stream's creating thread) via `_switch_thread_if_not_on_right_thread()`, which is **independent** from the underlying session netvc's owner thread. When teardown (`release_stream`) or setup (`create_stream`) touches the **session netvc's** queue while running on the stream thread `T ≠ O`, it aborts → stacks (A) and (C).

5. **Why intermittent:** most frame processing is delivered on the owner thread via `NetHandler::process_ready_list` (holding `nh->mutex`), so `T == O`. But when a frame is not fully available, `Http2CommonSession::state_complete_frame_read()` reschedules via:
```cpp
this->_reenable_event =
this->get_mutex()->thread_holding->schedule_in(this->get_proxy_session(),
HRTIME_MSECONDS(1),
HTTP2_SESSION_EVENT_REENABLE, vio);
```
i.e. onto **whatever thread currently holds the session mutex**, not the netvc owner thread. Resuming there later runs `create_stream` off-thread → stack (C). Note stack (C) is dispatched from `EThread::execute_regular()` via the event queue (not the net poll path), consistent with a scheduled reenable event.

## Why this is rarely reported

The `ink_release_assert` was introduced in 2019 (#4379 / #4787) to turn silent cross-thread races into explicit aborts and has been unchanged since. Over 8.x/9.x the common paths were aligned one-by-one (`adjust_thread`, #5120, #6843, …), while a general fix (#5950) was abandoned. The remaining unguarded paths require an uncommon combination — forward proxy + TLS bump + caching of decrypted HTTPS + PROXY protocol + HTTP/2 + fragmented frames triggering the reenable reschedule — which typical reverse-proxy/CDN deployments don't hit.

## Related issues / PRs

- #4379 / #4787 — introduced the asserts on keep_alive/active queue thread-safety.
- #5943 — ATS 9 NetHandler lock abort.
- #5950 — "Fix assert due to unheld nh->mutex" (abandoned).
- #4504 — Crash on `Http2ConnectionState::release_stream()`.
- #5120 / #6843 — reschedule work back onto the original/owner thread (the correct pattern).

## Suggested fix

**(1) Correct / upstream-style:** ensure these queue operations always run on `vc->thread`. Before calling `add_to_*_queue()` on the session netvc, if `this_ethread() != vc->thread`, reschedule onto the **fixed** `vc->thread` (like `adjust_thread()`, using the owner thread rather than the transient mutex holder). Lifetime of the netvc on the teardown path needs care.

**(2) Minimal / defensive (what we run in production):** guard at the lowest level so an off-thread call degrades gracefully instead of aborting:

```cpp
void UnixNetVConnection::add_to_keep_alive_queue() {
if (this->thread != this_ethread()) return; // not owner thread: skip; InactivityCop still reclaims it
MUTEX_TRY_LOCK(lock, nh->mutex, this_ethread());
if (lock.is_locked()) nh->add_to_keep_alive_queue(this);
else ink_release_assert(!"BUG: ... keep_alive_queue.");
}
bool UnixNetVConnection::add_to_active_queue() {
if (this->thread != this_ethread()) return true; // not owner thread: skip accounting but ALLOW the stream
bool result = false;
MUTEX_TRY_LOCK(lock, nh->mutex, this_ethread());
if (lock.is_locked()) result = nh->add_to_active_queue(this);
else ink_release_assert(!"BUG: ... active_queue.");
return result;
}
```

Key point: `add_to_active_queue()` **must return `true`** on the off-thread path — its return value gates `create_stream()`, and returning `false` would wrongly refuse valid streams as "maxed out active connections". Connection lifetime is unaffected: reclamation is done by `InactivityCop` over `open_list`, independent of these queues. The only cost is that a rare off-thread connection is not accounted in the per-thread active/keep-alive LRU bookkeeping and may bypass the per-thread active-connection limit (negligible at 8k/40k).

## Workaround status in production

Running with the defensive guard (option 2) on `add_to_keep_alive_queue()` and `add_to_active_queue()` only. Crashes stopped; no connection leaks observed. The two `remove_from_*_queue()` variants have the same assert but were not observed to fire in production, so they are left unchanged for now.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.