bryancall opened a new pull request, #13681:
URL: https://github.com/apache/trafficserver/pull/13681
Fixes #13679.
`rate_limit_sni_queue` choreographed its setup with wall-clock sleeps racing
live
TLS handshakes. That made it fail intermittently on loaded CI runners, and,
less
obviously, let it pass without exercising the queue at all.
## What was actually wrong
The only positive assertion was `ContainsExpression('Queueing the VC')`,
which does
not say *which* connection queued. Two separate failure modes hid behind
that:
**On Linux, under load, the holder never holds.** Reproduced on a 32-core
Fedora 44
box with 96 busy loops, 4 of 8 runs failed with exactly the CI signature:
```
Reserving a slot, active entities == 1
Releasing a slot, active entities == 0 <- holder gone 6ms after connecting
Reserving a slot, active entities == 1
Releasing a slot, active entities == 0
```
No `Queueing the VC` anywhere. The second connection found the slot free and
was
served rather than queued, so the precondition failed while every other
assertion
passed and ATS stayed healthy.
**On macOS, the holder never releases, and the guard was inert.** LibreSSL's
`s_client` does not exit on the FIFO EOF the script used to end the holder,
so the
run went:
```
Reserving a slot == 1 (holder)
Queueing the VC (second connection)
Rejecting connection, we're at capacity and queue is full (probe)
```
No `Releasing`, no `Enabling queued VC`. The resume and release paths never
ran.
Reverting the one-line fix from 508c1bea26 and re-running confirmed it:
**the old
test passes against the unfixed plugin on macOS.** It has not been guarding
anything on that platform.
## The change
Every step now waits for the plugin's own debug line in `traffic.out`
instead of
sleeping, with a 30s ceiling that fails loudly and dumps the log tail. Every
connection takes stdin from a FIFO the parent holds open read-write on fd 3
and
inherits via `<&3`, so nothing depends on how `s_client` reacts to EOF, and
each is
ended by signalling openssl's own PID. The one remaining `sleep` is a
deliberate
lower bound that lets the 300ms sweep tick while the slot is held; a correct
sweep
emits nothing to wait on, and a slower runner only gives it more ticks.
Assertions added so the queue path cannot be skipped silently:
- `ContainsExpression('Enabling queued VC')` - the sweep really resumed it
- `ExcludesExpression('Rejecting connection')` - the holder really released
- `ExcludesExpression(r'Releasing a slot, active entities == [0-9]{4,}')` -
no wrap
The existing `ExcludesExpression('_active <= _limit|received signal')` is
unchanged
and still hard.
Note the release assertion matches only a wrapped value, not any non-zero
one:
`Limiter::free()` logs `_active` after dropping the lock, so a concurrent
`reserve()`
can legitimately make a release read back as 1. An unmatched decrement of a
`uint32`
at limit 1 is unmistakable regardless.
## Scope note
The header now says explicitly that a client cannot close a connection
parked in the
ClientHello hook, so the "closes while parked" step the old script appeared
to
perform never did anything. ATS does not read the socket while the hook is
invoked,
so the FIN sits in the kernel until the sweep reenables the VC. Measured on
both
platforms: no release for the full 2s a connection sat parked and killed,
with the
release appearing only after `Enabling queued VC`. The old 0.3s kill
therefore always
resolved to the same resume-then-close path this script now drives
deterministically.
## Verification
Both platforms ran byte-identical files (md5 checked). Oracle = revert the
508c1bea26 one-liner in `sni_selector.cc`, rebuild `rate_limit.so`,
reinstall, rerun.
| Check | macOS 15 arm64, LibreSSL 3.3.6, 14 cores | Fedora 44, OpenSSL
3.5.8, 32 cores |
| --- | --- | --- |
| Fixed, idle | 3/3 pass, 4s (was 25s) | 5/5 pass, 4-5s |
| Fixed, under load | 8/8 (42 hogs) | 8/8 (96 hogs), 6/6 (64 hogs) |
| Original, idle | pass | pass |
| Original, under load | 8/8 pass (does not flake here) | **4/8 fail** |
| Original vs unfixed plugin | **passes - guard inert** | n/a |
| Fixed vs unfixed plugin | **fails** | **fails** |
The oracle run on both platforms:
```
Enabling queued VC after 153ms <- resumed with no reservation
Releasing a slot, active entities == 0
Releasing a slot, active entities == 4294967295
Fatal: limiter.h:262: failed assertion `_active <= _limit`
traffic_server: received signal 6
```
The two platforms failed the old test for opposite reasons, which is why the
fix
removes the dependency on `s_client` lifetime behaviour entirely rather than
tuning
a timeout.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
