Skip to content

[TS API] Add plugin API for config reload framework - #13146

Merged
brbzull0 merged 11 commits into
apache:masterfrom
brbzull0:plugin-config-registry-api-v2
Sep 2, 2026
Merged

[TS API] Add plugin API for config reload framework#13146
brbzull0 merged 11 commits into
apache:masterfrom
brbzull0:plugin-config-registry-api-v2

Conversation

@brbzull0

@brbzull0 brbzull0 commented May 8, 2026

Copy link
Copy Markdown
Contributor

Adds a small TSCfg* plugin-facing API that lets plugins register
configuration files with the same reload framework core configs
already use (ConfigRegistry / ConfigContext). Plugin reloads then
surface in traffic_ctl config status with full state tracking,
can receive RPC payloads, and follow the deferred-completion
contract - none of which were available via TSMgmtUpdateRegister.

Motivation

Today, plugins react to traffic_ctl config reload only via
TSMgmtUpdateRegister. That gives them a notification but nothing
else: no per-key targeting, no payload, no _reload directives,
no companion files, no way to surface reload outcome in config status. Every plugin that wants config-reload behaviour ends up
re-implementing pieces of the framework - regex_revalidate, for
example, ships its own file-mtime watcher.

This PR exposes the framework directly so plugins integrate with
it instead of re-implementing it.

New API surface

Types & enums (in include/ts/apidefs.h.in)

Registration (called from TSPluginInit)

Function Declaration Implementation
TSCfgRegister ts.h:1285 InkAPI.cc:3335
TSCfgAttachReloadTrigger ts.h:1330 InkAPI.cc:3409
TSCfgAddFileDependency ts.h:1347 InkAPI.cc:3435

Per-reload context (used inside the plugin's TSCfgLoadCb)

Function Declaration Implementation
TSCfgLoadCtxInProgress ts.h:1371 InkAPI.cc:3519
TSCfgLoadCtxComplete ts.h:1384 InkAPI.cc:3535
TSCfgLoadCtxFail ts.h:1397 InkAPI.cc:3541
TSCfgLoadCtxAddLog ts.h:1412 InkAPI.cc:3547
TSCfgLoadCtxAddSubtask ts.h:1427 InkAPI.cc:3572
TSCfgLoadCtxGetFilename ts.h:1460 InkAPI.cc:3596
TSCfgLoadCtxGetReloadToken ts.h:1476 InkAPI.cc:3608
TSCfgLoadCtxGetSuppliedYaml ts.h:1490 InkAPI.cc:3620
TSCfgLoadCtxGetReloadDirectives ts.h:1503 InkAPI.cc:3636

What this gives plugins, on top of TSMgmtUpdateRegister

  • Per-key targeting - handler runs only when this plugin's
    registered file or trigger record actually changes.
  • RPC payload - configs registered with TS_CFG_SOURCE_FILE_AND_RPC
    receive YAML content via JSONRPC and react to _reload directives.
  • Status surface - success / fail / in-progress / timeout plus
    log entries appear in traffic_ctl config status for config reload, file changes, and record changes during a reload cycle.
  • Subtasks - handlers can split work into named subtasks that
    aggregate into the parent's status.
  • Companion files - TSCfgAddFileDependency declares an extra
    file whose changes invoke the same handler, optionally routing
    inline RPC content via dep_key.
  • Deferred completion - handlers may stash the context, return,
    and finish on another thread later. Same contract core handlers
    already have.

Limitations

  • Not supported in remap plugins (TSRemapInit /
    TSRemapNewInstance); the reload framework is centred on global
    plugins.
  • is_required is propagated to FileManager but not enforced at
    reload time today (catalog/inspection only).
  • TSCfgAttachReloadTrigger is not a free-form record-change
    subscription; it triggers a reload of the registered config and
    nothing else.

Documentation

Fixes: #12967

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR exposes the existing config-reload framework (ConfigRegistry/ConfigContext/ReloadCoordinator) to global plugins via a new TSCfg* API, allowing plugin-owned config reloads to participate in the same reload lifecycle, status tracking, and JSONRPC/YAML payload flow as core configs.

Changes:

  • Introduces a plugin-facing registration + per-reload context API (TSCfgRegister, triggers/deps, and TSCfgLoadCtx* functions).
  • Propagates plugin ownership into reload task metadata and traffic_ctl config status output ([plugin: <name>] and meta.plugin_name).
  • Adds gold tests and unit tests covering plugin reload behavior, directives separation, deferred completion, and SSL reload status propagation.

Reviewed changes

Copilot reviewed 26 out of 26 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
tests/gold_tests/jsonrpc/plugins/CMakeLists.txt Adds new autest plugins for exercising TSCfg*.
tests/gold_tests/jsonrpc/plugins/cfg_plugin_test.cc Test plugin covering core TSCfg* APIs and subtask logging.
tests/gold_tests/jsonrpc/plugins/cfg_plugin_deferred_test.cc Test plugin demonstrating deferred completion contract.
tests/gold_tests/jsonrpc/plugins/cfg_plugin_directives_test.cc Test plugin validating _reload directive extraction vs content.
tests/gold_tests/jsonrpc/config_reload_ssl_state.test.py New gold test validating SSL reload status propagation and severity tags.
tests/gold_tests/jsonrpc/config_reload_ssl_bulk.test.py New gold test exercising bulk SSL reload status detail and partial failure.
tests/gold_tests/jsonrpc/config_reload_plugin_api.test.py End-to-end gold test for the new plugin API and status output.
tests/gold_tests/jsonrpc/config_reload_directives_plugin.test.py Gold test validating directive delivery via plugin API.
tests/gold_tests/jsonrpc/config_reload_deferred.test.py Gold test validating deferred plugin completion within a full reload.
src/traffic_ctl/jsonrpc/CtrlRPCRequests.h Adds plugin_name to reload task metadata in the client model.
src/traffic_ctl/jsonrpc/ctrl_yaml_codecs.h Decodes meta.plugin_name from YAML into the traffic_ctl model.
src/traffic_ctl/CtrlPrinters.cc Prints plugin ownership tag in task tree output.
src/records/unit_tests/test_ConfigRegistry.cc Adds unit coverage for plugin registration plumbing + dependency collision behavior.
src/mgmt/config/FileManager.cc Routes file mtime updates for plugin registry keys directly into ConfigRegistry reload scheduling.
src/mgmt/config/ConfigRegistry.cc Adds plugin registration entrypoint, directive extraction helper, and plugin name propagation into contexts/tasks.
src/mgmt/config/ConfigContext.cc Adds get_reload_token() and plugin name setter forwarding into task info.
src/api/InkAPI.cc Implements the TSCfg* plugin API layer and TSCfgLoadCtx wrapper semantics.
plugins/regex_revalidate/regex_revalidate.cc Migrates plugin reload integration from TSMgmtUpdateRegister to TSCfgRegister.
include/ts/ts.h Public API declarations and detailed doxygen for TSCfg*.
include/ts/apidefs.h.in Adds new plugin API types/enums and option structs for ABI-stable registration.
include/records/YAMLConfigReloadTaskEncoder.h Encodes plugin_name into JSONRPC YAML task metadata.
include/mgmt/config/ConfigReloadTrace.h Adds plugin_name to reload task info and setter.
include/mgmt/config/ConfigRegistry.h Adds plugin registration API and _reload extraction helper declaration.
include/mgmt/config/ConfigContext.h Adds reload-token getter and plugin-name propagation hook.
doc/developer-guide/config-reload-framework.en.rst Documents plugin participation and traffic_ctl ownership tagging.
doc/developer-guide/api/functions/TSCfgRegister.en.rst New reference page for TSCfgRegister and the TSCfgLoadCtx* family.

Comment thread src/mgmt/config/ConfigRegistry.cc Outdated
Comment thread doc/developer-guide/api/functions/TSCfgRegister.en.rst Outdated
Comment thread tests/gold_tests/jsonrpc/config_reload_plugin_api.test.py
Comment thread tests/gold_tests/jsonrpc/config_reload_plugin_api.test.py Outdated
Comment thread tests/gold_tests/jsonrpc/config_reload_directives_plugin.test.py Outdated
Comment thread plugins/regex_revalidate/regex_revalidate.cc
@bryancall bryancall changed the title [TS API] Add plugin API for the config reload framework. [TS API] Add plugin API for the config reload framework May 11, 2026
@brbzull0 brbzull0 changed the title [TS API] Add plugin API for the config reload framework [TS API] Add plugin API for config reload framework May 12, 2026
@brbzull0
brbzull0 marked this pull request as ready for review May 18, 2026 07:05
@bryancall
bryancall requested a review from cmcfarlen May 18, 2026 22:37
Copilot AI review requested due to automatic review settings May 26, 2026 09:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 26 out of 26 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

src/mgmt/config/ConfigRegistry.cc:361

  • ConfigRegistry::attach mutates entry.trigger_records before verifying that wire_record_callback() succeeds. If RecRegisterConfigUpdateCb fails, the registry will still report the trigger as attached even though no callback is wired. Consider wiring the callback first (or rolling back trigger_records on failure) so the registry state matches runtime behavior.
  // Single lock for check-and-modify.
  {
    std::unique_lock lock(_mutex);
    auto             it = _entries.find(key);
    if (it == _entries.end()) {
      Warning("Cannot attach trigger to unknown config: %s", key.c_str());
      return -1;
    }

    // Store record in entry - owned trigger
    it->second.trigger_records.emplace_back(record_name);
    config_key = it->second.key;
  }
  // Lock released before external call to RecRegisterConfigUpdateCb

  Dbg(dbg_ctl, "Attaching trigger '%s' to config '%s'", record_name, key.c_str());
  return wire_record_callback(record_name, config_key);

Comment thread src/api/InkAPI.cc Outdated
Comment thread src/api/InkAPI.cc Outdated
Comment thread src/mgmt/config/ConfigRegistry.cc Outdated

@cmcfarlen cmcfarlen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens when a plugin is unloaded or reinitialized? I couldn't find where the config context is unregistered when a plugin is unloaded.

@brbzull0

Copy link
Copy Markdown
Contributor Author

What happens when a plugin is unloaded or reinitialized? I couldn't find where the config context is unregistered when a plugin is unloaded.

This is only for global plugins, IIRC we do not unload/reinitialize global plugins. (unless we have a new feature that I am not aware of).

@brbzull0
brbzull0 requested a review from cmcfarlen June 5, 2026 12:34
@brbzull0

Copy link
Copy Markdown
Contributor Author

@cmcfarlen ping. 😄

@cmcfarlen cmcfarlen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work — the design is sound and the docs are unusually thorough. Two things I think should be sorted before merge, plus a handful of smaller items.

Blocking

1. Handler-less registry entries can now abort the server

do_register() now passes the registry key as configName to FileManager::addFile() whenever filename_record is empty, and process_config_update() routes an unknown configName that exists in the registry straight to schedule_reload().

Two core entries are registered with an empty filename_record and no handler:

// src/traffic_server/traffic_server.cc:795,797
reg.register_static_file("storage", ts::filename::STORAGE, {}, true);
reg.register_static_file("plugin",  ts::filename::PLUGIN);

Repro: edit storage.config (or plugin.config), then traffic_ctl config reloadrereadConfig() sees the mtime change → fileChanged("storage.config", "storage")RecGetRecordType("storage") fails → new branch → contains("storage") is true → schedule_reload("storage")execute_reload()ink_release_assert(entry_copy.handler) → traffic_server aborts.

Today configName is "" for these two files, so process_config_update() is a no-op — this PR introduces the path.

Suggest fixing both ends:

  • In the new FileManager branch, resolve the entry and require a handler before scheduling (find(configName) + entry->handler), otherwise keep the existing Dbg.
  • Downgrade ink_release_assert(entry_copy.handler) in execute_reload() to a Warning + return. A release assert reachable by editing a config file is a liability independent of this specific case.

2. The TSCfgLoadCtx liveness guard is itself a use-after-free

finalize_plugin_ctx() ends with delete pctx. Every subsequent call on that handle — plugin_ctx_alive(), or a second Complete/Fail reaching pctx->consumed.exchange(true) and Warning(..., pctx->reload_token.c_str()) — dereferences freed memory. So the consumed flag doesn't actually protect anything: the diagnostic meant to catch plugin misuse is a heap-use-after-free read, and under the ASAN autest builds it will be reported as such rather than printing the warning.

Two ways out:

  • Make it real: don't delete on finalize. Keep the handle in a process-lifetime side table (or intrusive list under a lock) keyed by an opaque id/generation, mark it consumed, and reclaim at reload-cycle end. Post-finalize calls then become genuine no-ops.
  • Make it honest: delete as today, but remove plugin_ctx_alive()'s dereference-based check and the ink_assert, and document that use after Complete/Fail is undefined behaviour — which is what the code actually implements.

As written, the header comment ("After the completing call, the handle is invalid") and the code's apparent tolerance of post-finalize calls contradict each other, and plugin authors will trust the wrong one.

Significant

TSCfgRegister reports success when the registration was dropped. do_register() only Warnings on a duplicate key, register_plugin_config() returns void, and TSCfgRegister() returns TS_SUCCESS for "registration attempt". A plugin whose key collides is never called and has no way to find out. Suggest having do_register/register_plugin_config return bool and propagating it. Also: TSCfgIsRegistered is declared and implemented but appears in neither the new .rst nor the PR summary table.

No versioning on the option structs. TSCfgRegister.en.rst says "new fields may be appended in future |TS| versions without breaking source compatibility". True for source, not for binary: a plugin built against today's header passes a smaller TSCfgRegistrationInfo, and a newer core reads past its end. Either add a leading size_t struct_size (or version tag) and check it, or scope the doc claim to recompiled plugins.

The wrapper lambda rediscovers what it already knows. In TSCfgRegister:

auto const &key = ctx.get_description();
if (auto const *entry = config::ConfigRegistry::Get_Instance().find(key); entry != nullptr) {
  handle->filename = entry->resolve_filename();
}

key_str is in scope at registration time — capturing it avoids depending on description == key, which is an incidental invariant of the current create_config_context() call sites, and drops a shared-lock registry lookup per reload.

TSCfgLoadCtxGetFilename semantics are ambiguous. resolve_config_filename() deliberately returns the bare filename (no sysconfdir prefix). A plugin registering "myplugin.yaml" gets "myplugin.yaml" back and has to prepend TSConfigDirGet(), while one registering an absolute path gets a directly openable path. Both new test plugins register absolute paths, so the relative case is untested. Either resolve to an absolute path in the plugin layer, or state it explicitly — the doc currently reads "the resolved file path the framework expects this handler to read".

Related: for a companion file added via TSCfgAddFileDependency, the handler gets the parent's filename and no indication of which file changed. Worth documenting, and arguably worth exposing.

Smaller items

  • Global namespace detail in include/mgmt/config/ConfigContext.h plus friend class detail::RecordTriggeredReloadContinuation is namespace pollution in a shared header. config::detail would be better — or thread plugin_name through ReloadCoordinator::create_config_context() and drop the friendship and forward declaration entirely.
  • attach() lock split: check under shared_lock, wire, then re-find under unique_lock. The if (it != _entries.end()) on the second pass silently swallows the case where the entry vanished. Init-time-only today, so benign, but a Dbg/Warning beats dropping the trigger record silently.
  • In RecordTriggeredReloadContinuation, ctx.set_plugin_name() in the !ctx standalone branch is a guaranteed no-op (_task.lock() is null). Harmless, but it reads as though attribution works on that path.
  • regex_revalidate: the TSCfgRegister() return value is discarded while every other call in that file logs; config_cont can now be declared inside the if (!disable_timed_reload) block since that's its only use. Also worth a look: a registry-triggered reload right after a timed one legitimately reports "unchanged" (via last_load), which may read as a failed reload in config status.
  • The reworded execute_reload non-terminal warning is terser but drops the actionable guidance the old text had about calling ctx.complete()/ctx.fail() from the deferred path. I'd keep that.
  • Diff noise: the en-dash→hyphen rewrites across 8 files and the Entry::has_handler() removal are cosmetic churn inside a 3k-line feature PR. Splitting them into a separate commit would make re-review considerably easier.
  • Docs could state that the std::string_view fields are copied during the TSCfgRegister call, so temporaries are safe — otherwise authors will assume they must keep backing storage alive.

Test coverage

Good breadth on the happy paths: file reload, RPC YAML, _reload directives, subtask success/failure, and two-stage deferred completion, all asserted through traffic_ctl config status. Gaps that line up with the findings above:

  • Nothing exercises the new FileManagerschedule_reload routing, which is where #1 lives — a test that touches storage.config and reloads would fail today.
  • No test for handle misuse (double Complete, use-after-Fail); a small Catch2 test around finalize_plugin_ctx would have surfaced #2 without a running server.
  • No coverage for duplicate-key registration, empty plugin_name refusal, or a relative config_path.

Security / performance

No concerns. The API is gated to TSPluginInit after a successful TSPluginRegister via validate_plugin_init(), message text is correctly passed as a %.*s argument rather than a format string, and the RPC content path is unchanged. Reload is a cold path, so the per-reload allocation and registry lookup don't matter.

Damian Meden added 9 commits August 7, 2026 09:23
Expose the centralized config reload framework to plugins via a new
TSCfg* C API so they can register configuration files alongside core
ATS configs and participate in the reload status, RPC inline-YAML, and
diagnostics machinery on equal footing.

Public API (include/ts/ts.h, include/ts/apidefs.h.in):
- TSCfgRegister(const TSCfgRegistrationInfo *) registers a plugin
  config with a TSCfgLoadCb handler. The plugin's canonical name is
  captured automatically from TSPluginRegister and surfaced in logs
  and traffic_ctl output.
- TSCfgRegistrationInfo and TSCfgFileDependencyInfo option structs
  keep the API extensible without breaking source compatibility.
- TSCfgAttachTrigger, TSCfgAddFileDependency, TSCfgSetEnabled cover
  reload triggers, file dependencies, and runtime enable/disable.
  TSCfgAddFileDependency supports operator-tunable filename via
  filename_record and inline-YAML routing via dep_key.
- TSCfgLoadCtx handler-context API: InProgress / Complete / Fail for
  deferred completion, AddLog for severity-aware messages, AddSubtask
  for nested handlers, and getters for filename, reload token, the
  RPC-supplied YAML, and reload directives.
- TSCfgLoadCb callback type and TSCfgSourceType enum.

Core integration:
- ConfigRegistry::register_plugin_config attributes plugin name to
  the entry; ConfigContext / ConfigReloadTrace propagate it through
  tasks and logs.
- traffic_ctl config status renders plugin attribution and task
  trees with severity-prefixed log lines.
- regex_revalidate migrated to the new API as the reference example.

Tests:
- cfg_plugin_test exercises every public API entrypoint.
- cfg_plugin_deferred_test demonstrates two-stage deferred completion
  via scheduled continuations on ET_TASK.
- cfg_plugin_directives_test covers _reload directive routing.
- New autests: config_reload_plugin_api, config_reload_deferred,
  config_reload_directives_plugin, config_reload_ssl_bulk,
  config_reload_ssl_state.

Documentation:
- doc/developer-guide/api/functions/TSCfgRegister.en.rst consolidates
  the reference for TSCfgRegister and TSCfgLoadCtx.
- doc/developer-guide/config-reload-framework.en.rst gains a Plugin
  Configuration Reload section with example, lifecycle, and
  traffic_ctl status output.

Ref: apache#12967
Make code simpler.
Tidy up.
Fix three review issues in the TSCfg* config-reload API:

- FileManager/ConfigRegistry: skip reload for catalog-only entries
  that have no handler instead of aborting. Add is_reloadable() and
  downgrade the handler-less release assert in execute_reload() to a
  warning so a storage.config-style change can no longer crash TS.
- InkAPI TSCfgLoadCtx: replace the raw-pointer handle plus consumed
  flag with an id-keyed registry so finalize is exactly-once and stale
  handles become safe no-ops, eliminating the use-after-free.
- TSCfgRegister: propagate registration failure. do_register and
  register_plugin_config now return bool and TSCfgRegister returns
  TS_ERROR on duplicate keys instead of reporting false success.

Add unit tests for duplicate-key failure and is_reloadable, and update
the TSCfgRegister docs and ts.h comments to match the new behavior.
Copilot AI review requested due to automatic review settings August 7, 2026 07:52
@brbzull0
brbzull0 force-pushed the plugin-config-registry-api-v2 branch from 85cee31 to ede51fc Compare August 7, 2026 07:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 26 out of 26 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

tests/gold_tests/jsonrpc/config_reload_ssl_bulk.test.py:42

  • The variable ssl_src_dir is assigned but never used, which adds noise and can confuse readers about where cert material is expected to come from in this test.
ssl_dir = ts.Variables.SSLDir
ssl_src_dir = os.path.join(ts.Variables.AtsTestToolsDir, "ssl")

Comment thread src/api/InkAPI.cc Outdated
PluginCtxRegistry::lookup() returned a raw pointer after releasing the
registry mutex, so a Complete/Fail on another thread could extract and
free the context while an accessor was still dereferencing it. Since
the API supports deferred completion from a plugin thread, concurrent
use of a single handle is a supported pattern rather than exotic
misuse, so this was a reachable use-after-free.

Store contexts as shared_ptr and have lookup() hand out a strong
reference taken under the lock. Finalize still unregisters under that
same lock, so a double finalize remains impossible, but the context now
survives until the last in-flight accessor releases it.

The plugin-facing contract is unchanged: handles stay opaque monotonic
ids and the registry remains the sole validity oracle.
Copilot AI review requested due to automatic review settings August 7, 2026 08:22

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Suppressed comments (3)

src/api/InkAPI.cc:3416

  • The public API docs in ts/ts.h say a null/unknown TSCfgLoadCtx is a no-op, but resolve_plugin_ctx() logs a Warning and triggers ink_assert when the handle is null/invalid. This can crash debug builds or at least contradict the advertised no-op semantics. Consider treating nullptr as a silent no-op and removing the assert for invalid/finished handles (keep a Warning if you still want misuse visibility).
  auto pctx = PluginCtxRegistry::instance().lookup(handle);
  if (pctx == nullptr) {
    Warning("%s called on an invalid or already-finalized TSCfgLoadCtx; ignoring", api_fn);
    ink_assert(!"TSCfgLoadCtx used after Complete/Fail or otherwise invalid");
  }

src/api/InkAPI.cc:3611

  • TSCfgLoadCtxComplete/Fail are documented as no-ops for null/unknown contexts, but finalize_plugin_ctx() currently warns and asserts on invalid handles (including nullptr). This makes the API behavior harsher than documented and could crash debug builds for plugin misuse. Add an explicit nullptr fast-path and drop the assert so null/unknown truly behave as no-ops (optionally keep the Warning for non-null invalid handles).
  auto pctx = PluginCtxRegistry::instance().extract(handle);
  if (pctx == nullptr) {
    Warning("TSCfgLoadCtx%s called on an invalid or already-finalized handle; ignoring", complete ? "Complete" : "Fail");
    ink_assert(!"TSCfgLoadCtx finalized more than once or otherwise invalid");
    return;

include/mgmt/config/ConfigReloadTrace.h:207

  • Plugin-owned subtasks created via ConfigContext::add_dependent_ctx() / TSCfgLoadCtxAddSubtask() won’t be tagged with meta.plugin_name unless plugin_name is propagated to child tasks. Currently ConfigReloadTask::add_child() (src/mgmt/config/ConfigReloadTrace.cc) creates the child task without copying the parent’s plugin_name, so traffic_ctl config status and JSONRPC will omit [plugin: ...] / meta.plugin_name for those subtasks despite the docs claiming every plugin-owned entry is tagged.
    std::string                      filename;         ///< source file, if applicable
    std::vector<ConfigReloadTaskPtr> sub_tasks;        ///< child tasks (if any)
    bool                             main_task{false}; ///< true for the top-level reload task
    std::string                      plugin_name;      ///< Registering plugin (empty for core).
  };

traffic_ctl config status dropped a task's description whenever the
task had a filename, so SSLCertificateConfig and QUICCertConfig both
rendered as ssl_multicert.yaml with no way to tell them apart. Render
both when they differ, which also makes the config_reload_ssl_bulk
assertion on SSLCertificateConfig satisfiable.

Scrub control characters from plugin-supplied descriptions so a stray
newline cannot garble the tree layout.
Copilot AI review requested due to automatic review settings August 7, 2026 11:03

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Suppressed comments (2)

src/api/InkAPI.cc:3594

  • TSCfgLoadCtxComplete/Fail are documented as no-ops for null/unknown handles (ts.h:1300-1311), but finalize_plugin_ctx() logs a Warning and hits ink_assert when called with a null handle (extract() returns nullptr). Null should be silently ignored to match the documented contract; keep the Warning/assert for non-null invalid or double-finalize handles if desired.
  auto pctx = PluginCtxRegistry::instance().extract(handle);
  if (pctx == nullptr) {
    Warning("TSCfgLoadCtx%s called on an invalid or already-finalized handle; ignoring", complete ? "Complete" : "Fail");
    ink_assert(!"TSCfgLoadCtx finalized more than once or otherwise invalid");
    return;

src/api/InkAPI.cc:3416

  • The public API docs in include/ts/ts.h state that a null TSCfgLoadCtx is a no-op (e.g. TSCfgLoadCtxInProgress / AddLog / accessors). However, resolve_plugin_ctx() currently treats a null handle the same as an invalid/finalized handle, emitting a Warning and triggering ink_assert. This makes a documented no-op path noisy and can trip debug builds when plugins defensively pass nullptr.

This issue also appears on line 3590 of the same file.

  auto pctx = PluginCtxRegistry::instance().lookup(handle);
  if (pctx == nullptr) {
    Warning("%s called on an invalid or already-finalized TSCfgLoadCtx; ignoring", api_fn);
    ink_assert(!"TSCfgLoadCtx used after Complete/Fail or otherwise invalid");
  }

@brbzull0

brbzull0 commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

[approve ci autest 2]

@brbzull0

brbzull0 commented Aug 9, 2026

Copy link
Copy Markdown
Contributor Author

[approve ci freebsd]

@brbzull0

Copy link
Copy Markdown
Contributor Author

Hi @cmcfarlen, thanks for the review, I made some changes base on your feedback. thanks.

@cmcfarlen cmcfarlen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed the three commits since my last pass (ede51fca7c, d427d269e5, c04093a524). Both blockers are genuinely fixed, and fixed the right way rather than papered over. What's left is small.

Verified fixed

Handler-less entries can no longer abort the server. is_reloadable() does the existence-and-handler check atomically under the registry lock, process_config_update() routes on that instead of contains(), and execute_reload()'s ink_release_assert(entry_copy.handler) is now a Warning + return. Both ends, as suggested. The storage.config / plugin.config repro no longer reaches schedule_reload().

The TSCfgLoadCtx handle is now actually safe. Opaque monotonic ids instead of raw pointers (so no ABA on a stale handle), shared_ptr ownership with lookup() returning a reference the caller holds for the duration of its call, and extract() removing under the same lock so a second finalize can't double-free. This resolves Copilot's cross-thread finding as well — the deferred-completion contract is now honored rather than nominally supported.

Also confirmed fixed: TSCfgRegister returns TS_ERROR on a dropped registration, with do_register/register_plugin_config returning bool; the wrapper lambda captures key_str rather than rediscovering it via get_description(); apply_passed_config() uses a const cfg_view so _reload is no longer inserted into the payload; add_file_dependency() wires the record callback before touching FileManager, with a comment explaining why; and regex_revalidate gained a real reload_mutex taken on both the timed and framework paths, created and destroyed with the plugin state.

New unit tests for duplicate-key rejection and is_reloadable cover the logic behind both blockers.

Correction to my previous review

I claimed TSCfgLoadCtxGetFilename hands back a bare filename for a relative config_path, forcing plugins to prepend TSConfigDirGet(). That was wrong — Entry::resolve_filename() prepends sysconfdir for any non-absolute path, and has done so on master all along. I was looking at the free resolve_config_filename(). Plugins get an openable path either way; disregard that item.

It does leave a small doc inaccuracy, though. TSCfgRegister.en.rst:363 says step 2 "returns config_path as-registered", which isn't true for a relative path — it comes back sysconfdir-prefixed. Worth correcting since it's the difference between a plugin calling open() directly or not.

Remaining items

1. ink_assert makes the documented no-ops fatal in the builds CI uses

ts.h now says of TSCfgLoadCtxComplete: "A later call on the same handle (double finalize, or any accessor) is ignored and logged as misuse, not acted on. Null/unknown ctx is a no-op." The registry comment makes the same promise — "a genuine no-op instead of a use-after-free read."

Both paths then call ink_assert(!"..."), and _ink_assert is TS_NORETURN. ink_assert is compiled in under DEBUG (ink_assert.h:42), which is exactly what the autest and ASAN presets build. So:

TSCfgLoadCtxComplete(nullptr, {});   // -> id 0, never valid (_next_id starts at 1)
                                     // -> extract() misses -> Warning + ink_assert -> abort

A documented no-op aborts traffic_server in a debug build. Same for any post-finalize accessor. The safety property the new registry buys you is only observable in release builds.

Pick one contract and make both the code and the docs say it:

  • Supported no-op (matches what's written): drop the two ink_asserts, keep the Warning. Misuse is diagnosable without being fatal, and the property becomes testable.
  • Undefined behavior with a diagnostic: keep the asserts, and drop "is a no-op" / "not acted on" from ts.h and the .rst.

I'd take the first — it's what the registry rewrite was for, and it's a two-line change. Null-handle tolerance in particular is worth keeping, since it lets a plugin's error path call Complete unconditionally.

2. Option-struct ABI is still unversioned

TSCfgRegistrationInfo and TSCfgFileDependencyInfo have no size or version field, and the docs claim (:47-52) that "new fields may be appended in future |TS| versions without breaking source compatibility", with a nearby note that the struct "stays ABI-stable".

Source compatibility is right. Binary compatibility isn't: a plugin compiled against today's header allocates today's struct and passes a pointer; a newer core that appended a field reads past the end of that allocation. Given how carefully the rest of the API surface is kept ABI-stable, this seems worth closing now rather than at the first append:

struct TSCfgRegistrationInfo {
  size_t struct_size{sizeof(TSCfgRegistrationInfo)};  // set by the plugin's compiler
  ...
};

with TSCfgRegister refusing anything smaller than the fields it reads. Alternatively, scope the doc claim to recompiled plugins and say appending is an ABI break — that's a valid choice, just not the one currently documented.

3. Minor leftovers

  • Global namespace detail in include/mgmt/config/ConfigContext.h:46, sitting right beside a properly nested namespace config, plus friend class detail::RecordTriggeredReloadContinuation. config::detail costs nothing and stops a very generic name from leaking out of a shared header.
  • ctx.set_plugin_name(entry->plugin_name) at ConfigRegistry.cc:125 is still a guaranteed no-op on the standalone branch — _task.lock() is null there. Harmless, but it reads as though attribution works on that path.
  • regex_revalidate: TSCfgRegister(&cfg_info); still discards the return with a comment that errors are logged internally. That's now true, but the plugin proceeds believing it's registered when it isn't and silently loses framework reloads — worth at least a Dbg. config_cont can also move inside the if (!disable_timed_reload) block, its only use.
  • The en-dash → hyphen rewrites are still in the diff across several files. Cosmetic churn inside a 3k-line feature PR makes re-review harder than it needs to be; a follow-up commit would be cleaner, though not worth another round-trip at this point.
  • The _live entry for a handler that never finalizes persists for process lifetime. The comment is honest about it being one entry per un-finalized reload, and it's no worse than the old leak, but reaping at reload-cycle end would bound it for a buggy plugin.

Test coverage

The unit tests added since my last pass cover the two blockers' logic well. Two gaps remain:

  • No misuse-path coverage — double Complete, accessor after Fail, null handle. This is the code that got rewritten, and its whole point is defined behavior under misuse. Note these tests can't be written while item 1 stands, since a debug build would abort; fixing item 1 unblocks them.
  • Nothing exercises a file change mapping to a handler-less registry key end to end. The is_reloadable unit test covers the decision, which is the important half, so this is optional.

One coordination note

This adds five autests as tests/gold_tests/jsonrpc/*.test.py plus three test plugins. #13545 converts the entire tree to Uranium/pytest manifests under tests/uranium_tests/. Whichever lands second will need its tests ported, and the plugin CMakeLists.txt under tests/gold_tests/jsonrpc/plugins/ moves with it. Worth agreeing on an order now rather than discovering it in a conflict — the five tests here are mostly traffic_ctl config status output assertions, which map onto replay manifests cleanly enough, but not for free.

Nice work on the rework — the handle registry in particular is a real improvement over what was there.

@brbzull0
brbzull0 merged commit 83335d4 into apache:master Sep 2, 2026
15 checks passed
bneradt pushed a commit to bneradt/trafficserver that referenced this pull request Sep 2, 2026
std::unordered_map::emplace may construct its node before discovering
the key is already present, consuming the moved-from Entry even though
nothing was inserted. The duplicate-registration warning then read
plugin_name out of that gutted Entry and reported the incoming owner as
"core". try_emplace leaves the argument untouched when the key exists,
so the warning can name the registration it rejected.

The read after the move came in with apache#13146, so the clang-analyzer job's
Clang-Tidy stage (bugprone-use-after-move) now fails on master.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
bneradt pushed a commit to bneradt/trafficserver that referenced this pull request Sep 2, 2026
std::unordered_map::emplace may construct its node before discovering
the key is already present, consuming the moved-from Entry even though
nothing was inserted. The duplicate-registration warning then read
plugin_name out of that gutted Entry and reported the incoming owner as
"core". try_emplace leaves the argument untouched when the key exists,
so the warning can name the registration it rejected.

The read after the move came in with apache#13146, so the clang-analyzer job's
Clang-Tidy stage (bugprone-use-after-move) now fails on master.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
bneradt added a commit that referenced this pull request Sep 2, 2026
std::unordered_map::emplace may construct its node before discovering
the key is already present, consuming the moved-from Entry even though
nothing was inserted. The duplicate-registration warning then read
plugin_name out of that gutted Entry and reported the incoming owner as
"core". try_emplace leaves the argument untouched when the key exists,
so the warning can name the registration it rejected.

The read after the move came in with #13146, so the clang-analyzer job's
Clang-Tidy stage (bugprone-use-after-move) now fails on master.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ATS Reload migration task. Expose ConfigRegistry to plugins

3 participants