apache / apache/trafficserver

config reload: task tree reports success before plugin callbacks have run

Open
#13,604 0 comments 0 reactions 1 assignee Claimed by @brbzull0 View on GitHub
Dominant language
C++
Stars
2k
Forks
874
Avg merge
6d 15h
Merged PRs (30d)
46

Description

`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 /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: /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 `...` / ``)

`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.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.