fix(etcd): do not advance the watch revision on a timeout, and make the recovery reload cheap

[AlinsRan]

Fixes #13067
Fixes #12167

Two commits, one for each issue. They belong together: the first removes an unsafe way of avoiding the recovery reload, the second makes that reload cheap enough not to need avoiding.

commit	issue	change
fix(etcd): do not advance the watch revision on a timeout	#13067	−18/+7, all additions comments
perf(etcd): reuse unchanged items on a full reload	#12167	−11/+41

---

Part 1 — #13067: silent, permanent config loss

The problem

config_etcd.lua samples the global etcd revision on a separate connection before each watch, then jumps to it if the watch times out:

-- before every watch: sample the latest revision on another connection
local res, err = watch_ctx.cli:readdir(watch_ctx.prefix .. "/phantomkey")
local latest_rev = tonumber(res.body.header.revision)

-- ... and when the watch times out:
if err == "timeout" then
    if latest_rev and watch_ctx.rev < latest_rev + 1 then
        watch_ctx.rev = latest_rev + 1        -- skip straight to the sample
    end
end

A timeout only means "no bytes arrived for watch_timeout seconds". That is equally true of an idle prefix and of a stream that established and then died silently — and the code cannot tell them apart:

t0   watch established, start_revision = R
t1   readdir samples latest = A            (on a different, healthy connection)
t2   watch stream dies silently            (no FIN, no RST — nothing observable)
t3   etcd writes the events for (R, A] into that dead stream
t4   50s elapse, zero bytes read           -> "timeout"
t5   watch_ctx.rev = A + 1                 -> everything in (R, A] is skipped

The root of it: the health of one connection is used to infer the delivery progress of another. Those two facts are unrelated.

It never heals

sync_data only sets need_reload on compacted / restarted. After the jump the start revision is fresh, so compaction never fires either — nothing will ever go back for the skipped range.

The worker keeps serving a stale configuration until that key is written again or the worker restarts:

• deleted routes keep routing traffic
• new routes and certificates never take effect
• the only trace is a single info-level log line

Each worker has its own watcher, so some workers can be stale while others are current.

The fix

Delete the sampling and the jump.

Reproducing it

Needs an asymmetric fault: the watch stream goes dark while short requests keep working. If the whole link fails, the sampling readdir fails too, latest_rev is nil, and the existing guard already prevents the jump.

Option 1 — iptables, no code needed

# 1. start etcd + APISIX, wait for the watch to establish
grep "restart watchdir: start_revision=" logs/error.log

# 2. find the watch connection (the long-lived one; short requests use new sockets)
ss -tn 'dst :2379'

# 3. blackhole only that connection — no RST, and range requests are untouched
iptables -I INPUT -p tcp --sport 2379 --dport <WATCH_SPORT> -j DROP

# 4. write a route during the blackhole
curl -X PUT http://127.0.0.1:9180/apisix/admin/routes/blackhole \
  -H "X-API-KEY: $admin_key" \
  -d '{"uri":"/blackhole","upstream":{"nodes":{"127.0.0.1:1980":1}}}'

# 5. wait out watch_timeout (50s), then restore
grep "etcd watch timeout, upgrade revision to" logs/error.log
iptables -D INPUT -p tcp --sport 2379 --dport <WATCH_SPORT> -j DROP

Result: etcdctl get /apisix/routes/blackhole returns the route, but /blackhole stays 404 — across every healthy watch cycle that follows, indefinitely.

Option 2 — selectively silent TCP proxy (what I used)

Put a proxy between APISIX and etcd, point deployment.etcd.host at it, and have it read the first request line:

• POST /v3/watch → forward the request, forward the response headers, then silently discard the body and hold the connection open (no FIN, no RST)
• anything else (/v3/kv/range, …) → forward normally

Then follow steps 4–5 above.

Real-world equivalents: NAT/conntrack reaping long-lived connections while short ones pass, conntrack tables being flushed (kube-proxy restart, iptables -F, CNI rebuild), asymmetric packet loss, an etcd-side watch stall with range still healthy.

Alternatives considered

Verify the (rev, latest] range is event-free before jumping — killed by deletes

Worth spelling out, because it looks like it should work.

RangeRequest does have min_mod_revision / max_mod_revision, so the check is expressible. Non-atomicity is not the problem either: an event landing after the check carries a revision above latest, so the next watch starting at latest + 1 still receives it.

What kills it is deletion. A key deleted inside (rev, latest] is not in the range result at all — the check comes back empty, the jump is taken, and the delete event is lost for good. Deletes are the worst case to lose: a removed route keeps serving traffic. No range-based verification can see them.

Use progress_notify and advance only on in-stream notifications — correct, but inert by default

etcd only sends progress notifications to fully synced watchers, so the revision one carries is a server-side guarantee that everything up to it was already delivered on that same stream. Correct in principle.

The catch: the progress ticker is created per stream and defaults to 10 minutes, while an idle APISIX watch stream only lives for watch_timeout (50s). Under a default etcd it never fires. It would be inert unless the operator also sets --watch-progress-notify-interval (--experimental-watch-progress-notify-interval before etcd 3.6; the flag only exists since 3.4.11).

An ordering-sensitive new branch, a lua-resty-etcd version floor and an FAQ entry, for something that does nothing out of the box — not worth it next to Part 2. Can be revisited on its own merits.

---

Part 2 — #12167: the CPU spike this used to paper over

Removing the jump means an idle prefix on a shared etcd can fall behind compaction again and recover with a full readdir. That is exactly what #12167 reported, so it gets fixed properly rather than avoided by guesswork.

The reporter's ask is specific — "APISIX CPU usage fluctuates when 'compacted' errors occur. I want to avoid this problem." Not the log line: the CPU.

Why the reload is expensive

Recovery from compacted rebuilds everything unconditionally:

• every item is re-validated through check_schema, the checker and the filter
• load_full_data sets changed as soon as any item is valid, so conf_version always moves and every router rebuilds its radixtree
• the item tables are new objects, so downstream caches keyed on them all miss

And it happens for every resource type — routes, services, upstreams, consumers, ssls, global_rules, plugin_configs … — in every worker, because need_reload is per config instance and produce_res(nil, "compacted") broadcasts to all of them.

None of that work is necessary when nothing actually changed — which is precisely the case here. A prefix idle enough to fall behind compaction is a prefix whose configuration did not change.

The fix

Compare each key against the previous snapshot and reuse the item when modifiedIndex matches:

local prev_item = get_prev_item(prev_values, prev_values_hash, key)
if prev_item and prev_item.modifiedIndex == item.modifiedIndex then
    insert_tab(self.values, prev_item)
    self.values_hash[key] = #self.values
    self:upgrade_version(item.modifiedIndex)
    goto continue                       -- note: `changed` is left alone
end

etcd increments mod_revision on every write, so an equal modifiedIndex means equal content. The prev_values / get_prev_item plumbing already exists — it was added so an item whose new data fails validation can keep serving its last valid value.

Why skipping the filter is safe

The incremental watch path already works this way: sync_data re-runs the checker and filter only for the keys that changed, and never touches the other items. This aligns the reload path with the watch path rather than inventing new semantics.

Checked every filter individually:

config	what its filter does	reused?
/routes	has_domain, set_plugins_meta_parent, host lowercasing, filter_upstream	✅ reused
/services	same	✅ reused
/upstreams	has_domain, filter_upstream	✅ reused
/consumers, /consumer_groups, /global_rules, /plugin_configs	set_plugins_meta_parent	✅ reused
/ssls	sni lowercasing, trailing-dot strip	✅ reused
/plugins	plugin.load(item) — rebuilds the global plugin registry, touches nothing on the item	❌ not reused — excluded from the optimisation

The first eight are idempotent and only mutate fields of the item they are handed, so an item filtered once is already in its filtered state.

/plugins is the one exception and is handled by construction rather than by a special case: it is the only config in the tree declared single_item (plugin.lua:893), and the reuse branch lives entirely in the multi-item else branch of load_full_data. The single_item path is untouched, so plugin.load() still runs on every reload. Reusing there would skip the registry rebuild — newly enabled plugins would not take effect and disabled ones would keep running — and it would save nothing anyway, since there is exactly one item.

Deletions need an explicit check

This is the trap. Keys that vanished while we were not watching leave every surviving key untouched, so changed would stay false, conf_version would not move, and the routers would go on serving the deleted items:

if prev_values_hash and matched_prev < nkeys(prev_values_hash) then
    changed = true
end

What this does not do

The readdir itself still happens on every compacted: without reading the full snapshot there is no way to know what was missed. The transfer and JSON parse remain. What goes away is the rebuild on top of it — the part that scales with configuration size and shows up as the spike.

---

Tests

Every assertion below was verified to be discriminating — a test that passes either way proves nothing.

TEST 14 (Part 1) asserted exactly the removed behaviour (etcd watch timeout, upgrade revision to appearing at least twice). Same topology, inverted assertion: the log must not appear at all. Old and new are mirrors, pinning the change in both directions.

TEST 19 (Part 2) — a reload with nothing changed. Two independent probes: a tag on the values array (a reload always allocates a fresh one, so losing it proves the reload really ran) and a tag on the item inside it (which must survive). Asserts conf_version moved once for the incremental write that wakes the watcher, not twice. Against unpatched master:

 reload ran: true
-item reused: true
+item reused: false
-conf_version bumped once, not twice: true
+conf_version bumped once, not twice: false

TEST 20 (Part 2) — a reload whose only change is a deletion. This one passes on unpatched master (which bumps unconditionally), so it was verified against the variant that actually matters: the reuse optimisation with the deletion check disabled:

 reload ran: true
 ghost dropped: true
-conf_version bumped for the deletion: true
+conf_version bumped for the deletion: false

Local full-file run: the failure set is identical with and without this branch (TEST 3/4/5/6/9/12, which need a TLS etcd on :12379 that this machine does not have), except that TEST 14 and TEST 19 flip from failing to passing. TEST 16/17/18 — the existing full-reload tests — still pass.

The blackhole scenario cannot be expressed in test-nginx (it needs a selectively silent TCP proxy), so it stays the manual procedure above.

Backport

Part 1 affects 3.14.0 and later and should be backported to the 3.14 / 3.15 release branches. Part 2 is a performance change and can follow or not, as maintainers prefer — the two commits are separable if you would rather backport only the first.

[membphis]

LGTM

[nic-6443]

I opened #13777 to track the remaining etcd read pressure. It proposes using WatchProgressRequest on the same stream, so an idle watcher advances only after etcd confirms event delivery, without bringing back the event-loss window fixed here. A snapshot reload remains the correctness fallback for an actual compacted response.