fix(etcd): advance the watch revision only on in-stream progress

[AlinsRan]

Fixes #13067

Blocked on

Depends on api7/lua-resty-etcd#224 being merged and released as v1.10.7. Once it is, this PR bumps lua-resty-etcd in apisix-master-0.rockspec from 1.10.6-0 to 1.10.7-0 and comes out of draft. The rockspec is deliberately left untouched for now, so CI here runs against 1.10.6.

Why the dependency: with progress_notify enabled, etcd can coalesce a notification right behind an event response in a single read, and lua-resty-etcd currently drops the event in that case. Merging this PR against 1.10.6 would reintroduce the very class of silent event loss it fixes.

Problem

#12514 samples the global etcd revision with an out-of-band readdir before each watch, and on a watch timeout moves watch_ctx.rev up to that sample:

if err == "timeout" then
    if latest_rev and watch_ctx.rev < latest_rev + 1 then
        watch_ctx.rev = latest_rev + 1
    end
end

A timeout only means "no bytes for watch_timeout seconds". It does not distinguish an idle prefix from a watch stream that established and then died silently. In the second case etcd has already written the pending events into the dead stream, so skipping to the sampled revision drops them permanently.

The failure is silent and does not self-heal: sync_data only sets need_reload on compacted / restarted, and after the jump the start revision is fresh, so compaction never fires either. The worker serves a stale configuration — deleted routes still routing, new routes and certificates never applied — until that key is written again or the worker restarts. The only trace is one info-level log line.

Reproduced on master by putting a TCP proxy between APISIX and etcd that forwards short requests normally but, for the watch stream, forwards the HTTP response headers and then silently discards the body without FIN/RST. Writes made during that window are skipped and never recovered; the route stayed 404 across many healthy watch cycles afterwards while etcd still held it. The asymmetric shape matters: if the whole link goes dark the sampling readdir fails too, latest_rev is nil and the guard prevents the jump. Real-world equivalents are NAT/conntrack reaping long connections while short ones pass, asymmetric packet loss, and an etcd-side watch stall with range still healthy.

Fix

Create the watch with progress_notify and advance the revision only on responses read from the stream itself.

etcd sends progress notifications only to watchers that are fully synced (watchable_store.go: progressIfSync bails out if any watcher is unsynced), so the revision a notification carries is a server-side guarantee that every event up to it was already delivered on that same stream. A stream that dies silently stops producing them, so the revision simply stops advancing — the safe direction.

The out-of-band readdir is removed rather than kept for diagnostics: its result no longer has any safe use, it costs a range RPC on every watch cycle, and keeping it around invites reintroducing the jump.

The notification branch must run before the "smaller revision" check. On an idle prefix watch_ctx.rev is the last event revision plus one, while a notification carries the current store revision — one lower. Handled after the check it would be read as "etcd may be restarted" and force a full resync on every notification.

Trade-off, stated up front

etcd sends notifications every 10 minutes by default, while an idle APISIX watch stream only lives for watch_timeout (50s by default). So under a default etcd configuration an idle stream will usually not see one, the revision stops moving while the prefix is idle, and behaviour falls back to what it was before #12514: unrelated writes advance the global revision, a compaction then cancels the watch with compacted, and APISIX does one full readdir reload to recover. That is the CPU spike #12167 complained about, at compaction frequency.

This is deliberate. A bounded, self-healing, observable reload is a better failure mode than an unbounded, silent, permanent configuration drift. A gateway that reads etcd once more is a slower gateway; a gateway that loses configuration is a wrong one.

Operators who want #12514's benefit back can set --experimental-watch-progress-notify-interval below watch_timeout (e.g. 30s, minimum 100ms) on the etcd side. Then an idle stream is notified before it times out, never times out, keeps a fresh revision, and compaction never cancels it — same outcome as #12514 but with the server-side delivery guarantee behind it. This is documented in the FAQ and configured for the CI etcd.

Older etcd versions without the flag degrade to the fixed 10-minute interval, i.e. the plain "remove the jump" behaviour. progress_notify itself has been in the etcd v3 watch API since its first release, so there is no compatibility floor.

Alternatives considered

• Keep the sampling and verify the (rev, latest] range is free of events for this prefix before jumping. readdir has no min_mod_revision filter, so the library would need changing; verify-then-jump is not atomic, so events landing after the check are still skipped; and the cost approaches a full readdir anyway. Not worth it over simply not jumping.
• Client-driven WatchProgressRequest (what the k8s apiserver does — notification frequency controlled by the client, no dependency on an etcd flag). Requires writing further request frames onto an established HTTP/1.1 chunked stream; neither lua-resty-http's request/body_reader model nor lua-resty-etcd's request_chunk supports that. Worth doing as a follow-up, out of scope here.
• Plain revert of fix(etcd): upgrade revision when watch request timeout #12514. Same correctness, but gives up the ability to avoid compaction storms entirely on a well-configured etcd. progress_notify costs ~10 lines and nothing at all when the server is not asked for it.

Tests

• t/core/config_etcd.t TEST 14 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 test and new test are mirrors, which pins the change in both directions.
• TEST 16 is new: default watch_timeout so the stream stays up, idle for 10s, then asserts the notification log appears and that a route created afterwards still takes effect — i.e. the notification advanced the revision without breaking delivery.
• ci/pod/docker-compose.common.yml sets ETCD_EXPERIMENTAL_WATCH_PROGRESS_NOTIFY_INTERVAL: 3s so notifications land inside a stream's lifetime. Its only other effect is that idle watch streams stop being rebuilt every 50s.
• The blackhole scenario cannot be pinned down in test-nginx — it needs a selectively silent TCP proxy — so it stays a manual verification procedure, described above.

Not run locally; relying on CI.

Backport

Affects 3.14.0 and later. Should be backported to the 3.14 / 3.15 release branches.

[AlinsRan]

Two things a review surfaced that belong in the description of this PR.

A client-driven alternative exists and should be recorded as evaluated-but-deferred. WatchRequest.progress_request / clientv3.RequestProgress gives the same in-stream guarantee (etcd replies with a progress response only when the watcher is already synced) without needing any server-side flag — so it would keep the #12514 benefit and the #13067 correctness on a stock etcd config. It is not free: lua-resty-etcd's HTTP/JSON request_chunk writes a fixed body then only reads, so it would need a chunked request body; the gRPC path is already bidirectional and would be a smaller change. Worth naming in the description so "change the etcd flag" doesn't read as the only option.

One doc nit on the trade-off section: on etcd >= 3.6 the flag is renamed --watch-progress-notify-interval (the --experimental- prefix is deprecated), so the FAQ should mention both spellings, plus the 100ms lower bound.

Neither blocks the direction — trading a silent-staleness optimization for the correct-but-costly compaction reload is the right call. Just make the deferred option explicit.