Cherry-picks for 10.2.0 RC, round 3 (2026-08-09) - #13520
Merged
Merged
Conversation
…e#13487) A transaction that loses the cache write lock and schedules a retry hands HttpSM a reusable captive action owned by HttpCacheSM. When the retry fires, HttpSM::state_cache_open_write() assigns the result of adjust_thread() to pending_action before releasing that delivered action. The callback is normally already on the correct thread, so adjust_thread() returns nullptr, and assigning nullptr to a PendingAction cancels whatever it was holding. The transaction thereby cancels its own captive action, and the cache read that the retry immediately issues comes back on an action already marked cancelled. Debug builds abort on the resulting assertion in HttpCacheSM, which is how this was found in production; release builds instead take the cancelled early return, drop a valid cache callback, and stall the transaction until it times out. This patch clears the delivered action before the thread adjustment rather than after it. Clearing first is safe because the cache action has already called back, and it means a genuine reschedule installs its event as the new pending action instead of canceling a captive action that is still in use. This also adds an autest in which two transactions contend for the cache write lock with read-while-writer disabled, so the loser's write retry delivers a synchronous cache read callback. That test aborts reliably on an unpatched debug build. (cherry picked from commit 6a96d50)
Pre-initialization plugin log buffers can be waiting when the logging workers start. A preprocessing thread can consume one before the flush queue exists and crash traffic_server while pushing the buffer to a null queue. This patch addresses the initialization race by constructing every logging notification and queue before spawning either worker. No logging thread can observe partially initialized shared queue state. This completes the startup ordering protection from apache#13472, which prevents plugins from waking a preprocessor before its notification exists but does not protect the flush queue after that worker starts. (cherry picked from commit 3177997)
* Bound the ring walk of ParentConsistentHash::selectParent When every parent in a consistent_hash pool is down, selectParent walked the whole hash ring taking the global host_status_rwlock on every hop. The ring holds 1024 replica nodes per parent (num_parents * 1024 nodes) and the chash_lookup() gate withholds wrap_around until the ring is traversed twice, so one all-down selection cost ~2 * num_parents * 1024 HostStatus::getHostStatus() calls (~49k for 24 parents) -- inline ET_NET CPU that starved the loopback health probe and drove the VIP flap in inc-p1s2-260703. Track the distinct parents examined on each ring: skip the locked getHostStatus read for a parent already seen, and force wrap_around once every distinct parent has been rejected. The expensive locked read is now paid at most once per parent (O(num_parents)); the ring still advances ~O(N*logN) cheap, lock-free hops to reach every distinct parent. Selection order and the retry-window logic are unchanged. The seen-parent tracking is sized to num_parents (std::vector<bool>), not MAX_PARENTS: the parent.config parser does not cap num_parents at MAX_PARENTS, so a fixed [MAX_PARENTS] array would overflow the stack for pools larger than 64. Add consistent_hash_ring_walk.test.py: an all-down 100-parent pool (marked down via HostStatus, >MAX_PARENTS on purpose) must report "getHostStatus calls: 100", proving the walk reads each parent once instead of walking the full ring. * Keep the parent seen-flags out of the heap in selectParent selectParent() runs inline on ET_NET for every transaction, so the two std::vector<bool> allocations per call are pure overhead for what is a 64-flag bitmap in the ordinary case. ts::LocalBuffer<bool, MAX_PARENTS> keeps both rings' flags on the stack (80 bytes each) and falls back to the heap only for a pool larger than MAX_PARENTS, which stays necessary because the parent.config parser does not cap num_parents. (cherry picked from commit 38076cc)
…pache#13475) * slice: purge every block of an object, not just those before a gap A PURGE is meant to discard the object, but the block walk stopped at the first block that was not in cache, so any object whose cached blocks were not a contiguous run from block 0 was only partially purged, and the client still got a 200. A gap in the middle left every block behind it cached; an uncached first block purged nothing and relayed that block's 404; and a "bytes=-N" purge deleted the head while leaving the tail it had named. The stop was load-bearing. The walk's only other terminator needs the object length, which slice only ever learned from a 206's Content-Range, and a PURGE response has none. So the core now reports the removed object's extent as X-Purged-Content-Range on a PURGE cache hit, and the walk learns where the object ends from the blocks it is already deleting. It is not Content-Range itself, since that header on a 200 is meaningless under RFC 9110 and cache_range_requests reads the pair as a stored 206 and rewrites the status. PURGE gets its own state machine in the plugin, so it no longer routes through handleFirstServerHeader, whose double duty as "form and emit the client response" is what leaked the 404. A 404 for a block is stepped over, nothing is written downstream until the walk finishes, and the response is then synthesized: 200 if any block was removed, 404 if none was. The extent is taken as a maximum rather than the first value seen, since blocks of one object disagree when the origin object was replaced in place. Until some block reports an extent the walk has no end but a miss bound, so add --purge-probe-blocks, default 8, capping consecutive uncached blocks. It never limits how many blocks a purge removes. A per-request override named by --purge-probe-header, default X-Slice-Purge-Probe, lets an operator who knows the object size widen it. A suffix range names its blocks by distance from an end slice does not know yet, so such a purge is widened to the whole object, a superset of what was asked. A PURGE whose Range cannot be parsed is refused with a 400 rather than guessing which blocks were meant. Tests cover the traversal over gaps, an uncached first block, both open-ended range forms, blocks that disagree about the object length, the miss bound and its override, and the refusal. They measure on the origin rather than the response body, since a purged block and a surviving block are indistinguishable to the client. Two further tests reproduce the client-visible failures of an origin object replaced in place under a child/parent hierarchy, which is how this problem was found. * Doc: Fix example of HTTP/1.1 messages * slice: pace the two PURGE request-validation error logs Both values are client supplied, so a bad one repeats as fast as requests arrive. Route them through Config::canLogError() like the other slice error paths. * slice: let a PURGE range bound the walk before any extent is known The requested range end comes from the client's Range header, but the walk only consulted it once some block had reported the object's extent, which only a block that was actually removed can do. A closed-range PURGE whose leading blocks were uncached therefore ran past its range end and removed blocks the client never named. * slice: do not report a partial PURGE as a success A block PURGE answering neither 200 nor 404 was read as "already absent", so a 403 from ip_allow or a 502 counted as a miss and the walk carried on to answer 200 on the strength of blocks it had removed earlier, telling the client the object was gone while part of it was still cached. Such a status says nothing about the blocks behind it either, so the walk now stops there and reports it. (cherry picked from commit 48fc842)
) RegexMatches::operator[] only checked the index against the ovector count. A group that does not participate in the match has unset offsets, and an optional group that precedes a participating one is still within that count, so the check passes and the subject pointer is advanced by PCRE2_UNSET. The resulting view has length zero, so callers see an empty string today, but the pointer is invalid. (cherry picked from commit e3bd689)
* Destroy replaced configs on ET_TASK ConfigProcessor::set() scheduled the deferred destruction of the replaced config with schedule_in(), which defaults to ET_CALL, so a network thread ran the destructor 60 seconds later inside the drain phase of its event loop. The destructor blocks that thread for as long as the config takes to release, which is bounded only by the size of the config. ConfigProcessor::release() is the only place a config is destroyed, and two callers reach it: the releaser at 60 seconds, which destroys the config whenever nothing else still holds a reference, and a transaction that outlived the releaser and drops the last reference itself. Schedule the releaser on ET_TASK, and hand the destructor from the transaction path to ET_TASK as well, so neither can block a network thread. The 60 second wait is unchanged. Shortening it would narrow the window that makes the load-then-increment in get() safe. The config debug tag now reports the duration of each destruction and the thread that ran it. (cherry picked from commit 46be2f5)
Several AuTests fail nondeterministically in parallel CI. The gRPC server can stop before its final response reaches the client, and the port allocator both ignores bound UDP ports and assumes every datagram address has a numeric port. The heavyweight strategy tests also rely on filename ordering that the parallel runner does not preserve. These failures appear as 502s, bind errors, setup exceptions, or port collisions. This patch addresses the races by counting completed RPCs, reserving bound IPv4 and IPv6 UDP ports while ignoring Unix sockets, and running both ordering-sensitive strategy tests after the parallel workers. Ports bound when the queue is initialized stay excluded for the full run, safely reducing the pool available on busy hosts. (cherry picked from commit 816420e)
HttpSM owns the write buffer attached to an HTTP/2 stream. After WRITE_COMPLETE it may release the buffer while a connection-level write-ready event can restart the stream through the non-owning _send_reader alias. This leaves restart_sending vulnerable to a use-after-free. Clear _send_reader before delivering WRITE_COMPLETE to HttpSM, and check completed write VIOs before inspecting the reader during connection restarts. This preserves zero-byte completion processing, including END_STREAM. Co-authored-by: bneradt <bneradt@yahooinc.com> (cherry picked from commit c0351de)
A transaction that revalidates a stale cached object and cannot take the cache write lock is sent back through a second cache lookup while it still holds the cache read connection its first lookup opened. The read that completes for that second lookup replaces the connection the transaction is using: debug builds abort on the read connection assertion in HttpCacheSM::state_cache_open_read(), and release builds close that connection out from under the stale object saved as the retry fallback, leaving the fallback pointing into freed memory. The re-lookup runs for every cache_open_write_fail_action rather than only for the two that configure a read retry, so fail action 2, which is documented to serve the stale object instead of retrying anything, aborts a debug build several times a day under production traffic. This patch limits the re-lookup to the fail actions that configure a read retry. A transaction that loses the write lock with a cached object and no retry configured now hands that object straight to the freshness handling that serves stale content, with no second lookup. The retry actions do want that lookup, so this also makes replacing the read connection explicit and drops the saved stale object along with the connection that owns it, since neither can outlive the other. This adds an autest covering both configurations that does not depend on contention between transactions: denying the write lock through max_open_write_retries makes the failure synchronous, and each configuration aborts an unpatched debug build on the production assertion. The re-lookup arrived with the fail action 6 work in apache#12852, which applied it to every non-default fail action; that commit's own test notes the stale path is timing sensitive and does not exercise it. The resulting aborts resemble the ones apache#13487 fixed, because both land in HttpCacheSM while a cache write retry dispatches events, but they are a distinct failure. apache#13487 stopped HttpSM from canceling its own captive action, which aborts on the cancellation assertion in HttpCacheSM.cc:138; this is the read connection assertion ten lines later, reached with that action perfectly valid. Both fixes are needed, and neither subsumes the other. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit f5c1b09)
…pache#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)
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
4 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.
Third round of cherry-picks for the 10.2.0 release candidate, covering PRs at "For v10.2.0" in the ATS v10.2.x project.
All picked with
git cherry-pick -xin master merge order; each commit's diffstat matches its master commit exactly.ParentConsistentHash::selectParentThis brings in four crash fixes (#13487, #13509, #13504, #13515) plus the regression #13515 addresses, which #12852 introduced and which is already present on this branch.
Merge order was preserved because
src/proxy/http/HttpTransact.ccis touched by both #13475 and #13515, and #13515 builds on #13487 inHttpCacheSM/HttpSM.One adaptation. #13406's new autests configured certificates through
ts.Disk.ssl_multicert_yaml, which only exists on master; this branch has the flatssl_multicert_config. Converted in all four affected files, matching the form theelsebranch of the existinghasattr(ts.Disk, "ssl_multicert_yaml")helpers intests/gold_tests/h3/already uses:Files:
rate_limit_sni_expiry.test.py,rate_limit_sni_reject.test.py,rate_limit_sni_queue.test.py,tls_hooks_close_while_parked.test.py. Amended into the #13406 pick rather than added as a follow-up commit.#13328 is deliberately not in this batch. "cache: shared-memory-backed Dir for fast restart" cannot be picked as-is: its
traffic_ctlintegration attachescache shmto acache_commandand routes throughCacheCommand, neither of which exists on 10.2.x. Both arrived with #13418 ("Add traffic_ctl cache clear command"), which is labeledNew Feature, sits at milestone 11.0.0, and is not tracked for this release. Auto-merge also wanted to pull the master-onlyConvertConfigCommand.ccandSSLMultiCertCommand.ccintosrc/traffic_ctl/CMakeLists.txt. Rather than freehand an adaptation of a 43-file change days before the RC, the pick was aborted and its project item left at "For v10.2.0" for a decision: either pick #13418 as a prerequisite, or ask the author for a PR against 10.2.x directly.Verified before pushing: every
Disk.*attribute andTest.*helper used by the picked tests exists on this branch (Test.AddConfigReload()is present via #13502), and all 21proxy.config.*records the new tests reference exist inRecordsConfig.cc.Local build is clean and
ctestpasses 164/165; the failure is the known macOS-localtest_jsonrpcserverunix-socket/restart timing flake.yapf0.43.0 reports the four edited test files clean, and theformattarget makes no changes.Draft so the full CI matrix runs before the release branch moves; it will be landed by fast-forward.