10.2.x: backport 11 PRs for the 10.2.1 release - #13659
Merged
Merged
Conversation
…13110) * Add _reload directive support to config reload framework Config handlers need a way to receive operational parameters (e.g. scoping a reload to a single entry) without conflating them with config content. This adds a reserved _reload key inside the configs YAML node that the framework extracts before invoking handlers. Framework: ConfigContext gains reload_directives() getter; the _reload node is extracted in ConfigRegistry::execute_reload() and stripped from supplied_yaml(). Fixes stale _passed_configs entries not being erased after consumption. CLI: traffic_ctl config reload gains --directive (-D) flag using dot-notation (config_key.directive_key=value). Multiple directives are space-separated after a single -D. Tests: unit tests for parse_directive and ConfigContext directive propagation; autest coverage for directive RPC structure handling. Docs: developer guide and traffic_ctl reference updated. (cherry picked from commit 07813a3)
The rate_limit plugin is experimental at this time and not guaranteed to be in the build. Most of the rate_limit AuTests already handle this appropriately, but a few were missing the `SkipUnless` directive. (cherry picked from commit e23cede)
Newer Clang releases expose latent ownership and error-handling issues, while the analyzer preset currently produces a GCC compilation database that Clang cannot reliably consume. This patch selects Clang explicitly for the analyzer preset, fixes the reported leaks, unchecked stream calls, and directory scanning under a mutex, and reshapes the remaining flagged code so the analyzer can follow it. That gives ATS a clean diagnostic baseline before the job moves to Ubuntu 26.04. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit c31517c)
TSSslClientCertUpdate has been unable to find normally configured outbound client contexts since 7dbb6cb changed the lookup key from certificate-and-key paths to the resolved certificate path. The existing AuTest hid the regression because its lowercase Streams.all assignments did not register assertions. This patch updates every matching CA bucket using the stored certificate path, drops the cached certificate data so that contexts created later do not resurrect the pre-update PEM, preserves working contexts when a replacement cannot be built, and releases the SSL configuration after use. It also corrects the API documentation and strengthens the AuTest to verify every CA bucket and the expected certificate subjects. Fixes: apache#13575 (cherry picked from commit a2011c2)
Never registered nor called since it was added in apache#7478, and its void(YAML::Node) signature never matched the method handler shape it sat among. Shutdown already syncs the cache dir from the event thread in traffic_server.cc; doing it from an RPC thread would race the dir writers. Dropping it also removes mgmt's relative-path include of iocore's private P_CacheDir.h. (cherry picked from commit 2b4988e)
Resolve concurrent read/write data races by using std::scoped_lock in operator= and locking other.ctx_mutex at the start of the copy constructor. (cherry picked from commit 6106f6b)
This includes a body delay feature Masaori worked on. See: https://github.com/yahoo/proxy-verifier#content-delay-specification (cherry picked from commit 0265a52)
…e#13558) When decoding a DNS response, the path that copies an unaligned A or AAAA record into the host entry buffer moved the write position without updating the count of space remaining, so the two disagreed for the rest of the response. Update the count after the copy, the same way the name, CNAME and PTR paths already do. (cherry picked from commit 80e89e5)
* Stop variable-arg options consuming later options ArgParser options declared with MORE_THAN_ZERO_ARG_N or MORE_THAN_ONE_ARG_N collected every remaining token, so an option written after one of them was silently swallowed as a value and never parsed. Collection now stops at a token naming another option of the same command, "--" ends option recognition so a value can still start with '-', and only the range actually consumed is erased. Separately, the --option=value path took the name up to the first '=' but the value from the last one, truncating any value containing '='. That made --directive=key.sub=val unusable, since directive values are key=value pairs by definition. Fixes: apache#13569 * Drop the -D placement workaround from traffic_ctl The guard rejecting directive values that start with '-' existed only because variable-argument parsing swallowed any option written after -D. That no longer happens, so the guard can only fire for a value the caller passed deliberately, and its advice to place -D last is now wrong. A malformed value is reported by the directive format check instead. Require values for both -D and -d. Supplying either with no values built a request identical to a plain reload, silently widening a scoped reload to every handler. Also document that -D may appear anywhere among the options and can be combined with -d, which the previous note said was impossible. * Add an at-most-one-argument arity to ArgParser An option whose value is optional had to be declared as taking zero or more values, the only variable arity available, so it also consumed the positional arguments of its own command. That is why traffic_ctl rejected "config get -c FILE RECORD" with an error naming get, and why --cold only worked written last or as --cold=FILE. Add AT_MOST_ONE_ARG_N, the equivalent of nargs='?' in Python argparse which this parser imitates, and declare --cold with it. The count check for the --option=value form now asks is_variable_arg_num() rather than comparing against the sentinels, so a third sentinel is not mistaken for a literal argument count. * Correct the variable-arg comment in handle_args The comment claimed the command's positional arguments were left in place, but collection only stops at a token naming another option, so positional tokens are still taken as values. Say so, and point at AT_MOST_ONE_ARG_N for an option whose value is optional. * Stop fixed-arity options consuming later options An option expecting a fixed number of values took whatever token followed it, so "traffic_ctl server debug enable -t -a" set the debug tags to the literal "-a" and wrote that to the running configuration. Apply the rule the variable-arity path already follows: a token naming another option of the same command is not a value, so the missing value is reported, and "--" still passes a value that starts with '-'. * Reject a repeated at-most-one-argument option is_variable_arg_num() exempts AT_MOST_ONE_ARG_N from the count check for the --option=value form, so --cold=a --cold=b silently kept the first and dropped the second, where the fixed arity equivalent is a usage error. * Correct the bare -c example in the traffic_ctl docs A bare -c before a record takes the record as the file name, which leaves config get with no records and exits with a usage error rather than reading a file named for the record. * Count both --cold spellings against the at-most-one limit The repetition check only counted the --option=value form, so "-c a -c b" silently kept the last file name, and mixing the two spellings put two values in an option that permits one. Fixed arity options keep their existing last-one-wins behaviour, which is a separate concern. * Say what "--" does to the options that follow it Option recognition stays off for the rest of a variable-length value list, so every later token becomes a value and any option written afterwards is swallowed. Neither the guide nor the traffic_ctl page said so, which made "--" look safe to use before other options. * Accumulate a repeated variable argument option Each occurrence reset the entry rather than adding to it, so "-D a -D b" kept only b and "-d f1 -d f2" reloaded only f2, silently dropping inline content the documentation says is merged. The --option=value form has always accumulated, so the two spellings of one option disagreed. A fixed arity option keeps its last-one-wins behaviour. The default command retry starts from a clean Arguments, since a global option is otherwise parsed twice and would collect its values twice. * Reject an empty value for an at-most-one-argument option An empty token was taken as the value, and traffic_ctl reads an empty --cold file name as a request for the default file, so a set whose file name came from an unset variable wrote to the live records.yaml and exited zero. The --option=value spelling already refuses an empty value, so the two spellings disagreed. The error names the option as it was written, matching that spelling. * Report the set symptom in the bare -c documentation A bare -c before a record takes the record as the file name, and the docs quoted only the error config get produces. config set is left short of its own arguments and reports a different one, so an operator who hit that did not find their message. * Drop the default command test that leaked into other tests set_default() writes a file scope default_command that nothing clears, so the test left every later parse in the same binary inserting "info" into its arguments. The mutex group and option dependency tests share that binary and failed on four platforms, while the file passed on its own. The retry path the test covered keeps its guard in parse(); it cannot be exercised without changing global state for the rest of the run. (cherry picked from commit d0fb283)
…vert (apache#13583) * Metrics: one gate for id validation, and fix the off-by-one valid(), lookup(IdType), name() and rename() each carried their own copy of the same range test, and the copies had drifted. valid() rejected an offset past MAX_SIZE; the other three did not. Since _splitID passes the low 16 bits of an id through unmasked and the offset check only applied when the id named the current blob, an id such as 0x0000FFFF indexed well past the end of a blob's 1024 entry arrays once a second blob existed. Ids reaching these accessors come from plugins through the TSStat* API, so they are untrusted. All four now go through Storage::_is_allocated(), which rejects a negative id, an offset no _makeId could have produced, an unallocated blob, and a slot at or past the allocation point. That last comparison also fixes an off-by-one: create() returns the id and then advances, so _cur_off is the next free slot, and the old <= / > tests accepted it. An increment there landed on the slot create() would hand out next, and since create() writes only the name and never the value, the next plugin to call TSStatCreate() received a metric already carrying someone else's count. Nothing depended on the loose bound: end() builds an id at the allocation point that is compared but never dereferenced, iterator::next() keeps the offset in range, and find() returns end() on a miss. * Metrics: publish the allocation point with release/acquire The lock removal in apache#13567 left the reader path reading _cur_blob, _cur_off and _blobs while a concurrent create() advances them, which is the data race apache#13310 took the mutex to close. Close it without the mutex instead. Making each counter atomic does not make the pair update atomically, and it does not need to. _cur_blob and _cur_off are publication points: each is written last, with a release store, after whatever it makes visible -- the blob pointer and the reset offset for _cur_blob, the slot's name for _cur_off. A reader acquires _cur_blob first, so observing a value for it also observes everything addBlob() wrote before releasing it. The torn pair a reader could otherwise see, a new blob index with the previous blob's stale offset, is unreachable rather than merely unlikely, so neither a packed word nor per-blob counters are needed. _blobs stays non-atomic. It is only read at an index no greater than _cur_blob, and that write is sequenced before the release store the reader acquired, so there is no race to close. Writers all hold the mutex and load relaxed. What remains is that a reader can observe an older _cur_blob with an already reset _cur_off and reject an id naming the previous blob, which drops an increment rather than misattributing one. Verified with a TSAN harness running eight readers validating and resolving ids across the whole space while a writer creates 2600 metrics across several blob boundaries: three reported races before this change, none after. * Metrics: cover concurrent id lookup, and make _extractType total Add a test that resolves ids from several threads while another registers metrics across a few blob boundaries. Nothing single threaded exercises the publication order the previous commit relies on; under the tsan preset, making either allocation counter non-atomic again reports a data race here. The test cannot catch a downgrade of the release/acquire pairs to relaxed -- atomics are race free at any ordering -- and says so, so the memory orders are not mistaken for tested. _extractType shifted a signed IdType, so _extractType(NOT_FOUND) sign extended to -4, a MetricType outside its enumeration, returned by Metrics::type(). Shifting unsigned is not enough on its own: the sign bit sits above the type field, so NOT_FOUND still yields 4. Mask to the single bit _makeId writes, which makes the function total for any input. * Add a ts::Metrics micro benchmark Nothing in tree measured the metric read paths, which is why a global mutex on the hottest one went unnoticed until it showed up in a production profile. Four cases, scaled by thread count: increment(id) what TSStatIntIncrement does, the path that regressed increment(ptr) what core and cripts do, the floor lookup(id) the lock free id resolution alone lookup(name) the same resolution through the mutex guarded name map lookup(name) is deliberately included as a positive control. It still takes the lock, so it must degrade with thread count; if it ever stops doing so, the harness is not loading the machine and the other three numbers mean nothing. Built only with ENABLE_BENCHMARKS, as with the rest of tools/benchmark. * Metrics: trim comments to the invariants State what holds rather than how it came to hold. Drops the explanations of which write order a comparison compensates for, what a reader would have seen otherwise, and what each benchmark case is meant to prove. Also shortens the createSpan boundary test's preamble, which describes the bug it covers at more length than the assertion needs. * Metrics: probe real ids in the concurrent lookup test The reader swept ids as consecutive integers, but an id packs the blob index above the offset, so 0..N only ever named blob 0 and everything from MAX_SIZE up decoded to an offset that validation rejects. Earlier cases in this file leave blob 0 full, so the reader was walking settled slots while the writer worked in a blob it never named. Take ids from what the writer has registered instead, and assert the ids span more than one blob so a future change cannot quietly confine the sweep again. Also assert the readers resolved something, since every id being skipped would otherwise pass. * Metrics: make _is_allocated private, tidy createSpan's index handling _is_allocated is only called by Storage's own accessors, so it does not belong in the public section; valid() remains the public gate. createSpan loaded _cur_off twice and _cur_blob once for its two guards, then re-read both unconditionally in case addBlob() had moved them. Load the pair once and refresh it only in the branch that grows a blob, which drops two atomic loads from the common path. Re-reading rather than adjusting the locals by hand keeps the caller from restating what addBlob() sets. * Publish the next free slot as one packed value _cur_blob and _cur_off were two atomics, and readers need the pair to be coherent. addBlob() reset the offset then bumped the blob, so a reader between the two saw the old blob with a zero offset and rejected every id in the just-completed blob. With ENABLE_FAST_SDK=OFF that reaches _TSReleaseAssert through TSStatInt*, so it aborts rather than losing a count. Reversing the stores only trades it for accepting ids in a blob nothing has been written to; two atomics have no coherent pair either way. One atomic holding the blob index above the offset, packed as an id is packed, fixes it: crossing a blob is a single release store. The value only ever increases, so an id is allocated exactly when it packs below the bound, which reduces the gate on every id based accessor to one acquire load and one compare. Acquiring the bound also acquires the blob install, so the null blob check goes away. It is also the id of the next free slot, which is what iteration wants for its end bound. Drop createSpan with it. It has no callers outside the tests, and it was the only path that could leave a blob partly filled -- it skipped to a fresh blob when a span did not fit, abandoning tail slots that were never handed out and that the packed bound would count as allocated. Without it, blobs fill contiguously and "packs below the bound" means exactly "was handed out". * Make the concurrent lookup test check what it claims Two ways it could pass without testing anything. Readers only published their tally on exit and nothing made them run before the writer finished, so on one CPU every reader could see stop and resolve nothing while resolved > 0 still held; it now publishes each resolution as it happens and the writer waits for one before stopping. And an id that lookup() clamps resolves to the reserved bad_id slot, whose name is not empty, so the name check could not detect a clamp; it now compares against the name that id must have. * Include <limits> and stop binding an unused offset Dropping createSpan took swoc/MemSpan.h with it, and that was what supplied <limits> for NOT_FOUND's numeric_limits. The header still compiles, through some other transitive path, which is exactly what makes it worth declaring. addBlob() destructured the packed value but only ever used the blob half. * Take the lock before touching a slot's name in rename() The name is the key _lookups is indexed by, so replacing it belongs entirely inside the lock. Nothing read the string outside it before -- binding a reference to it does not touch its bytes -- but computing that reference outside the lock made the boundary look wider than it is, and there is no reason for anything here to sit outside. * Metrics: trim comments, and drop ones about a check that is gone Two comments in the malformed-offset test explained that the null blob check, not the offset test, would reject those ids with only one blob allocated. The packed bound removed that check, so the reasoning no longer applied. * Remove rename() It mutated a slot's name while name() and lookup(id, &out_name) read that same std::string without the mutex and hand out views into it, which moonchen reproduced as a TSAN race. Locking rename() does not fix it; the readers are the lock free paths this PR exists to keep. Giving names immutable storage with its own lifetime rules would, but nothing outside the tests calls rename(). Without it a name is written once before the store that publishes it and never changes, so those readers are correct by construction. (cherry picked from commit c7af2e3)
Valid URLs with long query strings can miss regex_remap redirects. The old PCRE matcher used recursive calls for backtracking, so its recursion limit was reduced from 2047 to 1750 after stack crashes in apache#6819. The PCRE2 conversion in apache#12575 accidentally reused 1750 as a matching-work limit, causing ordinary long queries that previously matched to fail. This patch removes the work-limit override while retaining the per-instance match context and existing JIT stack behavior. PCRE2's normal work default is 10 million, allowing more worst-case CPU time per match while still bounding excessive backtracking. Its depth and heap limits remain intact. Since PCRE2 10.30, interpreter backtracking frames reside on the heap; JIT ignores the depth limit and uses a separately bounded stack. The old stack-derived value therefore does not translate into a suitable matching-work budget. This patch adds long-query redirect and capture-preservation coverage and extends the excessive-backtracking input to exercise the default work limit. The original 3 KB lookahead case remains a non-redirecting crash guard from apache#5762; its failure predates the PCRE2 conversion. Independent rule-specific log assertions preserve both checks. Fixes: apache#13651 Reported-by: Vinith Bindiganavale Co-authored-by: Codex Astra Medium (cherry picked from commit 7ed34a3)
cmcfarlen
force-pushed
the
10.2.x-picks-20260909
branch
from
September 9, 2026 22:09
690ae73 to
72c0d24
Compare
cmcfarlen
changed the title
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
7 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.
Backports queued for the 10.2.1 release, picked in master merge order.
All eleven are single-parent squash merges cherry-picked with
-x, none labeledIncompatible, and none previously present on10.2.x.07813a3875b5_reloaddirective support for config reload frameworke23cedefb9cbc31517c86b12a2011c2fc4a12b4988ef3e836106f6b57f460265a523cb1f80e89e5130bbd0fb28340693c7af2e39b5c27ed34a3d3c57Notes
#13110 continues the config-reload framework series already on this branch (#13354,
#13369, #13502). It is also a prerequisite here: #13570 adjusts the
traffic_ctl config reload -Dhandling that #13110 introduces, so picking #13570 without it conflicts andwould require splitting the PR. With #13110 first, #13570 applies clean.
#13593 is required to get this branch's Clang-Analyzer check green. The CI analyzer
moved to Clang 21; master absorbed the resulting findings in #13593, but
10.2.xnever gotit, so the job reports pre-existing bugs in files no other pick here touches
(
OCSPStapling.cc,QPACK.cc,Http3Session.cc,LogFieldFallback.cc,healthchecks.cc,HostDB.cc, ...) andscan-build --status-bugsfails on any report. The last greenanalyzer run on a
10.2.xPR was 2026-08-25, before that toolchain change. Picking #13593also stops every future
10.2.xPR from showing a red analyzer.Conflicts, two of eleven.
#13110, in the unit-test list in
src/records/CMakeLists.txt: this branch hastest_RecHiddenMetricLookup.ccand already liststest_RecDumpRecords.cc,while master adds both
test_ReloadDirectives.ccandtest_RecDumpRecords.cc. Resolvedby keeping the branch's entries and adding only
test_ReloadDirectives.cc, the one filethe commit actually creates.
#13593, in
HttpProxyServerMain.cc: master passes&portto theHttpSessionAccept/Http2SessionAcceptconstructors, which comes from master-only #13514 ("Share HTTP acceptproperties"). Took master's
needs_proberestructuring with this branch's constructorsignatures. The
needs_probe == !port.isQUIC()invariant holds here:probeis onlydereferenced in the SSL and plain-fallback branches, both non-QUIC.
Every other pick's diff matches its master counterpart exactly.
Local verification: full build clean;
ctest167/168. The single failure,test_jsonrpcserver, aborts at unix-socket bind withEPERMunder/var/folders— aknown macOS-local issue, not a regression. #13578 was ruled out as the cause: it only
removes code from
handlers/server/Server.{h,cc}, and no rpc unit test referencesserver_shutdown. Autests could not be run locally, so this PR's CI is the gate for theautest-touching picks (#13589, #13642, #13110, #13570).
Draft on purpose: this lands on
10.2.xby fast-forward once CI is green, so the branchhistory stays identical to master's.