brbzull0 opened a new issue, #13604: URL: https://github.com/apache/trafficserver/issues/13604
`traffic_ctl config reload` reports a completed reload before plugin config callbacks have run, and a plugin that rejects a config has no way to say so. Found while reviewing #13600; `rate_limit` is used below as the probe, but this is not a `rate_limit` defect. ### Impact Plugin config callbacks work, and have since 2015 — `TSMgmtUpdateRegister` dispatch is unaffected by this. What is missing is that the reload task tree neither waits for those callbacks nor records their outcome, so the RPC asserts a completeness it cannot know. Concretely: a plugin that rejects a config keeps its previous configuration, logs to `diags.log`, and the reload still exits 0 with every subtask green. An operator who fixes a malformed plugin config, reloads, and sees a green result has no signal that the running configuration is not the one on disk. The callback's return value is discarded, so there is nowhere for the plugin to put the answer. Six global plugins register on master: | plugin | registration | can it reject a config today? | |---|---|---| | `experimental/rate_limit` | 3-arg, file + mtime gate | **yes** — `return false` keeps the previous config | | `regex_revalidate` | 2-arg | no reject path surfaced | | `compress` | 2-arg | no — `management_update()` calls `load_global_configuration()` and returns 0 | | `lua` | 2-arg | no reject path surfaced | | `stats_over_http` | 2-arg | no reject path surfaced | | `experimental/wasm` | 2-arg | no reject path surfaced | `rate_limit` is the only one passing a filename, so the only one gated on mtime; the others fire on every reload whether or not their config changed. It is also the only one where the tree currently reports a *wrong* answer rather than merely an incomplete one. **Since when.** The dispatch is old — `ConfigUpdateCbTable` traces to 2015 — but it was not wrong until there was a task tree to be absent from. #12892 (`5bab268cb4`, 2026-03-13) introduced `ReloadCoordinator`, the per-task status model, and the `reserve_subtask` protocol, wiring in `FileManager`, `Configuration.cc`, and `traffic_server.cc`. It did not touch `src/api/ConfigUpdateCbTable.cc` or `include/api/InkAPIInternal.h`. Before that commit the RPC made no granular claim; after it, it does. ``` Version: master @ 956747452dcd36e8e56064aeaa47b0d8493ad0ac Platform: Darwin 25.6.0, Apple clang 21.0.0 Config: plugin.config -> rate_limit.so <path>/rate_limit.yaml ``` ### Proof Two halves of one protocol; the plugin path has only the second. `ConfigRegistry::on_record_change()` reserves before scheduling, and says why: https://github.com/apache/trafficserver/blob/956747452dcd36e8e56064aeaa47b0d8493ad0ac/src/mgmt/config/ConfigRegistry.cc#L165-L174 ```cpp // Pre-register a CREATED subtask so the main task knows work is pending. // Without this, aggregate_status() can reach SUCCESS before the continuation // runs and creates the subtask. ReloadCoordinator::Get_Instance().reserve_subtask(ctx->config_key); eventProcessor.schedule_imm(new RecordTriggeredReloadContinuation(ctx->mutex, ctx->config_key), ET_TASK); ``` `ConfigUpdateCbTable::invoke()` schedules without reserving — exactly the case that comment warns about: https://github.com/apache/trafficserver/blob/956747452dcd36e8e56064aeaa47b0d8493ad0ac/src/api/ConfigUpdateCbTable.cc#L75-L79 ```cpp void ConfigUpdateCbTable::invoke(INKContInternal *contp) { eventProcessor.schedule_imm(new ConfigUpdateCallback(contp), ET_TASK); } ``` `ConfigUpdateCallback::event_handler` then discards the callback's return value, so even a convention-based failure signal would have nowhere to go: https://github.com/apache/trafficserver/blob/956747452dcd36e8e56064aeaa47b0d8493ad0ac/include/api/InkAPIInternal.h#L117-L134 ```cpp m_cont->handleEvent(TS_EVENT_MGMT_UPDATE, nullptr); delete this; ``` #### Reproduction (rate_limit used as a probe) A duplicate-SNI config, which takes `rate_limit`'s existing non-throwing rejection path: ```yaml # rate_limit.yaml selector: - sni: dup.example.com limit: 100 - sni: dup.example.com limit: 200 ``` The plugin rejects it and keeps the previous configuration: ``` [Aug 31 16:37:24.701] [ET_TASK 0] ERROR: [rate_limit] Duplicate SNIs being added (dup.example.com) [Aug 31 16:37:24.701] [ET_TASK 0] ERROR: [rate_limit] Failed to reload YAML file: <path>/rate_limit.yaml ``` The same reload cycle reports success and exits 0: ``` $ traffic_ctl config reload -m -t rldtk-probe-1 ✔ Reload scheduled [rldtk-probe-1] ✔ [rldtk-probe-1] ████████████████████ 1/1 success (13ms) ``` Human: ``` $ traffic_ctl config status -t rldtk-probe-1 ✔ Reload [success] — rldtk-probe-1 Started : 2026 Aug 31 14:37:48.885 Finished: 2026 Aug 31 14:37:48.898 Duration: 13ms ✔ 1 success ◌ 0 in-progress ✗ 0 failed (1 total) Tasks: ✔ ssl_ticket_key ······························· 0ms [Note] SSL ticket key loading ... [Note] SSL ticket key reloaded ``` JSON: ``` $ traffic_ctl config status -t rldtk-probe-1 --format json {"jsonrpc": "2.0", "result": {"tasks": [{"config_token": "rldtk-probe-1", "status": "success", "description": "Main reload task - ...", "config_key": "", "filename": "", "meta": {"created_time_ms": "...", "last_updated_time_ms": "...", "main_task": "true"}, "logs": [], "sub_tasks": [{"config_token": "rldtk-probe-1", "status": "success", "description": "ssl_ticket_key", "config_key": "ssl_ticket_key", "filename": "", "meta": {"created_time_ms": "...", "last_updated_time_ms": "...", "main_task": "false"}, "logs": [{"level": "3", "text": "SSL ticket key loading ..."}, {"level": "3", "text": "SSL ticket key reloaded"}], "sub_tasks": []}]}]}, "id": "..."} ``` (timestamps, `id`, and absolute paths elided as `...` / `<path>`) `ssl_ticket_key` is the only subtask in either form — `rate_limit` never appears, in the tree or in the exit code. The main task's `last_updated_time_ms` falls in the same millisecond as the plugin's ERROR lines: the task closed while the callback was still failing beside it. The `1/1` and the `13ms` are the clearest statement of the problem — the tree finished counting before the plugin was consulted. ### Proposed change Two defects, and they need fixing together rather than separately. **(a) No reservation.** `invoke()` schedules on `ET_TASK` without `reserve_subtask()`, so `aggregate_status()` reaches SUCCESS before the callback has run. Affects all six registrants regardless of whether they can fail — it is what produces `1/1 success (13ms)` above with `rate_limit` absent from the tree. **(b) No failure channel.** `ConfigUpdateCallback::event_handler` discards the callback's return value, so a plugin that rejects a config has nowhere to report it. Worth stating explicitly: **fixing (a) alone would be worse than the current behaviour.** A reserved subtask with no way to fail can only ever be marked complete, so the tree would show `rate_limit: success` for a config `rate_limit` had just rejected. Today it shows nothing, which is at least not a false claim. I have deliberately not proposed a patch, because the right shape depends on a decision I should not make alone — whether `TSMgmtUpdateRegister` is worth extending at all: - **Extend it.** Add `reserve_subtask()` in `invoke()` (the plugin name is already the `cb_table` key) *and* a failure channel — for example treating `TS_EVENT_ERROR` from the callback as failure and mapping it to `ConfigContext::fail()`. No new TSAPI, no enum change, no ABI break, and each existing plugin becomes fixable with a one-line change. Would need a sweep of the six to confirm none returns `TS_EVENT_ERROR` today for an unrelated reason. - **Replace it.** #13146 adds `TSCfgRegister` / `TSCfgLoadCtx*`, which gives migrated plugins both halves properly. If that is the intended direction, this issue becomes "migrate the six and deprecate `TSMgmtUpdateRegister`" once it lands. Noting it as a candidate, not a dependency — the defect above is present on master today either way. Not done: no test. An autest asserting a non-zero exit from `traffic_ctl config reload` when a plugin rejects its config would be the regression guard, and would fail today. -- 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]
