From da56b54abe06de7aca0f3d277bbdc2024e4ef4ef Mon Sep 17 00:00:00 2001 From: AlinsRan Date: Mon, 3 Aug 2026 16:20:04 +0800 Subject: [PATCH 1/5] fix(etcd): do not advance the watch revision on a timeout #12514 samples the global etcd revision with an out-of-band readdir before each watch and, when the watch times out, moves watch_ctx.rev up to that sample. A timeout only means that no bytes arrived 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 loss does not heal. sync_data only sets need_reload on compacted or 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, with nothing but one info level log line to show for it. Reproduced by putting a TCP proxy between APISIX and etcd that forwards short requests normally but, for the watch stream, forwards the response headers and then silently discards the body without FIN or RST. Writes made during that window are skipped and never recovered. The asymmetry matters: when 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 or conntrack reaping long connections while short ones pass, asymmetric packet loss, and an etcd side watch stall with range still healthy. Removing the jump restores the pre-#12514 behaviour: an idle prefix can fall behind compaction again and recover with a full readdir. That is a bounded, self-healing and observable cost, unlike the silent permanent configuration drift it replaces. Fixes #13067 --- apisix/core/config_etcd.lua | 25 +++++++------------------ t/core/config_etcd.t | 11 +++++------ 2 files changed, 12 insertions(+), 24 deletions(-) diff --git a/apisix/core/config_etcd.lua b/apisix/core/config_etcd.lua index d7d6f1971a48..02439009ae50 100644 --- a/apisix/core/config_etcd.lua +++ b/apisix/core/config_etcd.lua @@ -196,18 +196,13 @@ local function do_run_watch(premature) opts.need_cancel = true opts.start_revision = watch_ctx.rev - -- get latest revision - local res, err = watch_ctx.cli:readdir(watch_ctx.prefix .. "/phantomkey") - if err then - log.error("failed to get latest revision, err: ", err) - end - local latest_rev - if res and res.body and res.body.header and res.body.header.revision then - latest_rev = tonumber(res.body.header.revision) - else - log.error("failed to get latest revision, res: ", json.delay_encode(res)) - end - + -- Do not advance start_revision on a watch timeout. A timeout only means + -- that no bytes arrived for watch_timeout seconds; it cannot tell an idle + -- prefix apart from a stream that established and then died silently. In + -- the latter case etcd has already written the pending events into the + -- dead stream, so moving the revision forward skips them for good, and the + -- loss never heals: the next start revision is fresh, so compaction is not + -- triggered and need_reload is never set. See #13067. log.info("restart watchdir: start_revision=", opts.start_revision) local res_func, err, http_cli = watch_ctx.cli:watchdir(watch_ctx.prefix, opts) @@ -227,12 +222,6 @@ local function do_run_watch(premature) then log.error("wait watch event: ", err) end - if err == "timeout" then - if latest_rev and watch_ctx.rev < latest_rev + 1 then - watch_ctx.rev = latest_rev + 1 - log.info("etcd watch timeout, upgrade revision to ", watch_ctx.rev) - end - end cancel_watch(http_cli) break end diff --git a/t/core/config_etcd.t b/t/core/config_etcd.t index a40f425b684c..869c497f9890 100644 --- a/t/core/config_etcd.t +++ b/t/core/config_etcd.t @@ -521,7 +521,7 @@ main etcd watcher initialised, revision= -=== TEST 14: watch revision should be upgraded when timeout occurs +=== TEST 14: watch revision must not be upgraded when the watch times out --- yaml_config deployment: role: traditional @@ -547,7 +547,8 @@ nginx_config: return end ngx.sleep(2) - -- we will assert 4 lines of revision upgrade log because we have one worker and one privileged agent + -- write outside the watched prefix so that the global revision + -- moves on while the watch itself stays idle and keeps timing out for i = 1, 2 do local _, err = etcd_cli:set("/apache", "apisix") if err then @@ -563,10 +564,8 @@ nginx_config: GET /t --- response_body passed ---- grep_error_log eval -qr/etcd watch timeout, upgrade revision to/ ---- grep_error_log_out eval -qr/(etcd watch timeout, upgrade revision to\n){2,}/ +--- no_error_log +etcd watch timeout, upgrade revision to From e7473a7e71230f3a9b322284aca979ad45296c25 Mon Sep 17 00:00:00 2001 From: AlinsRan Date: Mon, 3 Aug 2026 17:32:24 +0800 Subject: [PATCH 2/5] perf(etcd): reuse unchanged items on a full reload A full reload is how APISIX recovers from a compacted watch, and today it rebuilds everything unconditionally: every item is re-validated through check_schema, the checker and the filter, and 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 too, so downstream caches keyed on them all miss. None of that is necessary when nothing actually changed, which is the common case for the deployment that suffers from this: a prefix idle enough to fall behind compaction is a prefix whose configuration did not change. Compare each key against the previous snapshot and reuse the item when the modifiedIndex matches. etcd increments mod_revision on every write, so an equal modifiedIndex means equal content. A reload that changes nothing now keeps the existing objects, leaves conf_version alone and rebuilds no routers. This is the same semantics the incremental watch path already has: sync_data re-runs the checker and filter only for the keys that changed, and leaves the other items untouched. Every filter but /plugins' only mutates fields of the item it is given, so an item that was filtered once is already in its filtered state; /plugins is single_item and is left out of the optimisation because its filter calls plugin.load(), which has global effects. Deletions need an explicit check. 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. Fixes #12167 --- apisix/core/config_etcd.lua | 52 ++++++++--- t/core/config_etcd.t | 181 ++++++++++++++++++++++++++++++++++++ 2 files changed, 222 insertions(+), 11 deletions(-) diff --git a/apisix/core/config_etcd.lua b/apisix/core/config_etcd.lua index 02439009ae50..7bfe19a52f83 100644 --- a/apisix/core/config_etcd.lua +++ b/apisix/core/config_etcd.lua @@ -26,6 +26,7 @@ local json = require("apisix.core.json") local etcd_apisix = require("apisix.core.etcd") local core_str = require("apisix.core.string") local new_tab = require("table.new") +local nkeys = require("table.nkeys") local inspect = require("inspect") local process = require("ngx.process") local check_schema = require("apisix.core.schema").check @@ -549,6 +550,8 @@ end local function load_full_data(self, dir_res, headers, prev_values, prev_values_hash) local err local changed = false + -- how many of the previous keys are still present, used to detect deletions + local matched_prev = 0 if self.single_item then self.values = new_tab(1, 0) @@ -610,6 +613,25 @@ local function load_full_data(self, dir_res, headers, prev_values, prev_values_h for _, item in ipairs(values) do local key = short_key(self, item.key) + local prev_item = get_prev_item(prev_values, prev_values_hash, key) + if prev_item then + matched_prev = matched_prev + 1 + end + + -- Nothing changed for this key, so reuse the item we already have + -- instead of rebuilding it. This keeps the object identity stable, + -- which matters because downstream caches are keyed on it, and it + -- leaves `changed` alone so that a reload which changed nothing + -- does not bump conf_version and rebuild every router. + -- Same semantics as the incremental watch path in sync_data, which + -- only re-runs the checker and filter for the keys that changed. + 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 + end + local data_valid = true err = nil if type(item.value) ~= "table" then @@ -649,20 +671,28 @@ local function load_full_data(self, dir_res, headers, prev_values, prev_values_h self.filter(item) end - else - local prev_item = get_prev_item(prev_values, prev_values_hash, key) - if prev_item then - -- keep serving with the last valid configuration instead of - -- silently dropping the whole item on a full reload, see the - -- incremental path in sync_data for the same semantics - log.warn("failed to check item data of [", self.key, "/", key, - "], keep the previous configuration, err: ", err) - insert_tab(self.values, prev_item) - self.values_hash[key] = #self.values - end + elseif prev_item then + -- keep serving with the last valid configuration instead of + -- silently dropping the whole item on a full reload, see the + -- incremental path in sync_data for the same semantics + log.warn("failed to check item data of [", self.key, "/", key, + "], keep the previous configuration, err: ", err) + insert_tab(self.values, prev_item) + self.values_hash[key] = #self.values end self:upgrade_version(item.modifiedIndex) + + ::continue:: + end + + -- Keys present in the previous snapshot but absent now were deleted + -- while we were not watching. Every surviving key can be untouched and + -- still leave us with a changed configuration, so this has to be + -- checked separately or a reload that only deletes would keep serving + -- the removed items. + if prev_values_hash and matched_prev < nkeys(prev_values_hash) then + changed = true end end diff --git a/t/core/config_etcd.t b/t/core/config_etcd.t index 869c497f9890..14290ced3af6 100644 --- a/t/core/config_etcd.t +++ b/t/core/config_etcd.t @@ -861,3 +861,184 @@ GET /t invalid new item loaded: false --- no_error_log keep the previous configuration + + + +=== TEST 19: a full reload that changes nothing reuses the items and does not bump conf_version +--- timeout: 25 +--- yaml_config +deployment: + role: traditional + role_traditional: + config_provider: etcd + etcd: + host: + - "http://127.0.0.1:2379" + prefix: /apisix +--- extra_yaml_config +nginx_config: + worker_processes: 1 +--- config + location /t { + content_by_lua_block { + local core = require("apisix.core") + local etcd = require("resty.etcd") + local etcd_cli, err = etcd.new({ + http_host = "http://127.0.0.1:2379", + }) + if not etcd_cli then + ngx.say("failed to create etcd client: ", err) + return + end + + etcd_cli:set("/apisix/global_rules/1", { + id = "1", + create_time = 1700000000, + update_time = 1700000000, + plugins = {["response-rewrite"] = {headers = {set = {["X-T"] = "a"}}}} + }) + ngx.sleep(2) + + local obj = core.config.fetch_created_obj("/global_rules") + local before_version = obj.conf_version + + -- Two independent probes. A reload always builds a fresh `values` + -- array, so losing this one proves the reload actually ran; the + -- incremental path only mutates elements and would keep it. + obj.values.array_probe = "old" + -- The items inside must survive: reusing them is the whole point. + for _, item in ipairs(obj.values) do + if item and item.value and item.value.id == "1" then + item.reload_probe = "kept" + end + end + + -- Arm the recovery path taken after a `compacted` error, then write + -- a second rule. sync_data is parked in waitdir, so the write is + -- what wakes it: the incremental path adds /2 and bumps + -- conf_version once, and the next sync_data round reaches the + -- need_reload branch. By then /1 and /2 are both in memory at the + -- revisions etcd reports, so the reload has nothing to change and + -- must not bump conf_version a second time. + obj.need_reload = true + etcd_cli:set("/apisix/global_rules/2", { + id = "2", + create_time = 1700000000, + update_time = 1700000000, + plugins = {["response-rewrite"] = {headers = {set = {["X-T2"] = "b"}}}} + }) + ngx.sleep(3) + + local probe_kept = false + for _, item in ipairs(obj.values) do + if item and item.value and item.value.id == "1" then + probe_kept = (item.reload_probe == "kept") + end + end + + ngx.say("reload ran: ", obj.values.array_probe == nil) + ngx.say("item reused: ", probe_kept) + ngx.say("conf_version bumped once, not twice: ", + obj.conf_version == before_version + 1) + + etcd_cli:delete("/apisix/global_rules/1") + etcd_cli:delete("/apisix/global_rules/2") + ngx.sleep(1) + } + } +--- request +GET /t +--- response_body +reload ran: true +item reused: true +conf_version bumped once, not twice: true + + + +=== TEST 20: a full reload that only deletes must still bump conf_version +--- timeout: 25 +--- yaml_config +deployment: + role: traditional + role_traditional: + config_provider: etcd + etcd: + host: + - "http://127.0.0.1:2379" + prefix: /apisix +--- extra_yaml_config +nginx_config: + worker_processes: 1 +--- config + location /t { + content_by_lua_block { + local core = require("apisix.core") + local etcd = require("resty.etcd") + local etcd_cli, err = etcd.new({ + http_host = "http://127.0.0.1:2379", + }) + if not etcd_cli then + ngx.say("failed to create etcd client: ", err) + return + end + + etcd_cli:set("/apisix/global_rules/1", { + id = "1", + create_time = 1700000000, + update_time = 1700000000, + plugins = {["response-rewrite"] = {headers = {set = {["X-T"] = "a"}}}} + }) + ngx.sleep(2) + + local obj = core.config.fetch_created_obj("/global_rules") + obj.values.array_probe = "old" + + -- An item that is live in memory but gone from etcd, so the reload + -- has to drop it. Every surviving key is untouched and therefore + -- reused, so without an explicit deletion check `changed` would + -- stay false, conf_version would not move, and the routers would + -- go on serving the dropped item. + local ghost = { + key = "/apisix/global_rules/ghost", + modifiedIndex = 1, + value = {id = "ghost", plugins = {}}, + } + core.table.insert(obj.values, ghost) + obj.values_hash["ghost"] = #obj.values + + local before_version = obj.conf_version + + -- same wake-up mechanism as TEST 19: /2 arrives incrementally + -- (+1), then the reload drops the ghost (+1) + obj.need_reload = true + etcd_cli:set("/apisix/global_rules/2", { + id = "2", + create_time = 1700000000, + update_time = 1700000000, + plugins = {["response-rewrite"] = {headers = {set = {["X-T2"] = "b"}}}} + }) + ngx.sleep(3) + + local found_ghost = false + for _, item in ipairs(obj.values) do + if item and item.value and item.value.id == "ghost" then + found_ghost = true + end + end + + ngx.say("reload ran: ", obj.values.array_probe == nil) + ngx.say("ghost dropped: ", not found_ghost) + ngx.say("conf_version bumped for the deletion: ", + obj.conf_version == before_version + 2) + + etcd_cli:delete("/apisix/global_rules/1") + etcd_cli:delete("/apisix/global_rules/2") + ngx.sleep(1) + } + } +--- request +GET /t +--- response_body +reload ran: true +ghost dropped: true +conf_version bumped for the deletion: true From a66db76d3e65c58a9455a8421bce2f48103549c3 Mon Sep 17 00:00:00 2001 From: AlinsRan Date: Tue, 4 Aug 2026 14:48:07 +0800 Subject: [PATCH 3/5] test(etcd): assert delivery after the timeouts and check etcd setup errors Addresses review feedback on the new coverage. TEST 14 only asserted the absence of a log line, which would also hold if the watch were broken outright. Write under the watched prefix once the timeouts have happened and assert the event is still delivered, so the test pins the behaviour from both sides. The new tests also ignored the return value of every etcd_cli:set, so a failed setup surfaced as a confusing assertion mismatch rather than an error. Check the writes and bail out with a message. Cleanup deletes only warn: a cleanup failure should not mask the result of the assertions that already ran. --- t/core/config_etcd.t | 76 ++++++++++++++++++++++++++++++++++++++------ 1 file changed, 66 insertions(+), 10 deletions(-) diff --git a/t/core/config_etcd.t b/t/core/config_etcd.t index 14290ced3af6..06c083080834 100644 --- a/t/core/config_etcd.t +++ b/t/core/config_etcd.t @@ -538,6 +538,7 @@ nginx_config: --- config location /t { content_by_lua_block { + local core = require("apisix.core") local etcd = require("resty.etcd") local etcd_cli, err = etcd.new({ http_host = "http://127.0.0.1:2379", @@ -557,13 +558,44 @@ nginx_config: end ngx.sleep(1) end - ngx.say("passed") + + -- The watch has timed out and restarted several times by now. Assert + -- it still delivers events for the watched prefix: the other + -- assertion here is the absence of a log line, which would also hold + -- if the watch were broken outright. + local _, err = etcd_cli:set("/apisix/routes/after-timeout", { + id = "after-timeout", + uri = "/after-timeout", + create_time = 1700000000, + update_time = 1700000000, + upstream = {type = "roundrobin", nodes = {["127.0.0.1:1980"] = 1}} + }) + if err then + ngx.say("failed to set route: ", err) + return + end + ngx.sleep(1) + + local delivered = false + local obj = core.config.fetch_created_obj("/routes") + for _, item in ipairs(obj and obj.values or {}) do + if item and item.value and item.value.id == "after-timeout" then + delivered = true + end + end + ngx.say("update after timeout delivered: ", delivered) + + local _, err = etcd_cli:delete("/apisix/routes/after-timeout") + if err then + ngx.log(ngx.WARN, "failed to clean up route: ", err) + end + ngx.sleep(1) } } --- request GET /t --- response_body -passed +update after timeout delivered: true --- no_error_log etcd watch timeout, upgrade revision to @@ -891,12 +923,16 @@ nginx_config: return end - etcd_cli:set("/apisix/global_rules/1", { + local _, err = etcd_cli:set("/apisix/global_rules/1", { id = "1", create_time = 1700000000, update_time = 1700000000, plugins = {["response-rewrite"] = {headers = {set = {["X-T"] = "a"}}}} }) + if err then + ngx.say("failed to set global_rules/1: ", err) + return + end ngx.sleep(2) local obj = core.config.fetch_created_obj("/global_rules") @@ -921,12 +957,16 @@ nginx_config: -- revisions etcd reports, so the reload has nothing to change and -- must not bump conf_version a second time. obj.need_reload = true - etcd_cli:set("/apisix/global_rules/2", { + local _, err = etcd_cli:set("/apisix/global_rules/2", { id = "2", create_time = 1700000000, update_time = 1700000000, plugins = {["response-rewrite"] = {headers = {set = {["X-T2"] = "b"}}}} }) + if err then + ngx.say("failed to set global_rules/2: ", err) + return + end ngx.sleep(3) local probe_kept = false @@ -941,8 +981,12 @@ nginx_config: ngx.say("conf_version bumped once, not twice: ", obj.conf_version == before_version + 1) - etcd_cli:delete("/apisix/global_rules/1") - etcd_cli:delete("/apisix/global_rules/2") + for _, key in ipairs({"/apisix/global_rules/1", "/apisix/global_rules/2"}) do + local _, del_err = etcd_cli:delete(key) + if del_err then + ngx.log(ngx.WARN, "failed to clean up ", key, ": ", del_err) + end + end ngx.sleep(1) } } @@ -982,12 +1026,16 @@ nginx_config: return end - etcd_cli:set("/apisix/global_rules/1", { + local _, err = etcd_cli:set("/apisix/global_rules/1", { id = "1", create_time = 1700000000, update_time = 1700000000, plugins = {["response-rewrite"] = {headers = {set = {["X-T"] = "a"}}}} }) + if err then + ngx.say("failed to set global_rules/1: ", err) + return + end ngx.sleep(2) local obj = core.config.fetch_created_obj("/global_rules") @@ -1011,12 +1059,16 @@ nginx_config: -- same wake-up mechanism as TEST 19: /2 arrives incrementally -- (+1), then the reload drops the ghost (+1) obj.need_reload = true - etcd_cli:set("/apisix/global_rules/2", { + local _, err = etcd_cli:set("/apisix/global_rules/2", { id = "2", create_time = 1700000000, update_time = 1700000000, plugins = {["response-rewrite"] = {headers = {set = {["X-T2"] = "b"}}}} }) + if err then + ngx.say("failed to set global_rules/2: ", err) + return + end ngx.sleep(3) local found_ghost = false @@ -1031,8 +1083,12 @@ nginx_config: ngx.say("conf_version bumped for the deletion: ", obj.conf_version == before_version + 2) - etcd_cli:delete("/apisix/global_rules/1") - etcd_cli:delete("/apisix/global_rules/2") + for _, key in ipairs({"/apisix/global_rules/1", "/apisix/global_rules/2"}) do + local _, del_err = etcd_cli:delete(key) + if del_err then + ngx.log(ngx.WARN, "failed to clean up ", key, ": ", del_err) + end + end ngx.sleep(1) } } From 99180ea8fd4144e0bb190e6d1bc9020cc1f3b59c Mon Sep 17 00:00:00 2001 From: AlinsRan Date: Tue, 4 Aug 2026 15:26:07 +0800 Subject: [PATCH 4/5] test(etcd): give the new timeout assertion room and poll for delivery CI hit 'client socket timed out' on this block, deterministically -- the rerun failed the same way. The block had no explicit --- timeout, and the added write plus wait pushed it past whatever the inherited default resolves to, so the request never returned and both assertions saw an empty body. Declare --- timeout: 20 as the other tests in this file do, drop the redundant sleep after cleanup, and poll for the delivery instead of sleeping a fixed second. A fixed wait is only a guess about how slow the etcd round trip gets on a loaded runner, and guessing low turns this into a flaky assertion failure rather than a real signal. --- t/core/config_etcd.t | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/t/core/config_etcd.t b/t/core/config_etcd.t index 06c083080834..b556d1ef2016 100644 --- a/t/core/config_etcd.t +++ b/t/core/config_etcd.t @@ -574,13 +574,21 @@ nginx_config: ngx.say("failed to set route: ", err) return end - ngx.sleep(1) - + -- poll rather than sleep a fixed amount: delivery is normally + -- immediate, and a fixed wait would only be a guess about how slow + -- the etcd round trip can get on a loaded CI machine local delivered = false - local obj = core.config.fetch_created_obj("/routes") - for _, item in ipairs(obj and obj.values or {}) do - if item and item.value and item.value.id == "after-timeout" then - delivered = true + for _ = 1, 25 do + ngx.sleep(0.2) + local obj = core.config.fetch_created_obj("/routes") + for _, item in ipairs(obj and obj.values or {}) do + if item and item.value and item.value.id == "after-timeout" then + delivered = true + break + end + end + if delivered then + break end end ngx.say("update after timeout delivered: ", delivered) @@ -589,9 +597,9 @@ nginx_config: if err then ngx.log(ngx.WARN, "failed to clean up route: ", err) end - ngx.sleep(1) } } +--- timeout: 20 --- request GET /t --- response_body From c78f3c599842a69b9b4630155d9fd8a9db33d760 Mon Sep 17 00:00:00 2001 From: AlinsRan Date: Tue, 4 Aug 2026 16:02:14 +0800 Subject: [PATCH 5/5] chore(etcd): trim the comments added by this change Several of them restated what the code says or, worse, explained code that is no longer there: the seven-line note about not advancing the revision on a timeout sat next to the start_revision assignment, forty lines away from the branch it described, and reproduced an argument the commit message and the issue already carry. Keep only what is not obvious from the code: that the reuse branch leaves `changed` alone on purpose, that this matches sync_data, and that a deletion has to be detected separately. Rename matched_prev to prev_keys_still_present so the counter explains itself instead of needing a comment. --- apisix/core/config_etcd.lua | 36 +++++++++++++----------------------- t/core/config_etcd.t | 34 ++++++++++++---------------------- 2 files changed, 25 insertions(+), 45 deletions(-) diff --git a/apisix/core/config_etcd.lua b/apisix/core/config_etcd.lua index 7bfe19a52f83..e59dc6b32d71 100644 --- a/apisix/core/config_etcd.lua +++ b/apisix/core/config_etcd.lua @@ -197,13 +197,9 @@ local function do_run_watch(premature) opts.need_cancel = true opts.start_revision = watch_ctx.rev - -- Do not advance start_revision on a watch timeout. A timeout only means - -- that no bytes arrived for watch_timeout seconds; it cannot tell an idle - -- prefix apart from a stream that established and then died silently. In - -- the latter case etcd has already written the pending events into the - -- dead stream, so moving the revision forward skips them for good, and the - -- loss never heals: the next start revision is fresh, so compaction is not - -- triggered and need_reload is never set. See #13067. + -- A watch timeout must not advance start_revision: it cannot tell an idle + -- prefix from a stream that died silently, and skipping ahead loses the + -- events etcd already wrote into that stream. See #13067. log.info("restart watchdir: start_revision=", opts.start_revision) local res_func, err, http_cli = watch_ctx.cli:watchdir(watch_ctx.prefix, opts) @@ -550,8 +546,7 @@ end local function load_full_data(self, dir_res, headers, prev_values, prev_values_hash) local err local changed = false - -- how many of the previous keys are still present, used to detect deletions - local matched_prev = 0 + local prev_keys_still_present = 0 if self.single_item then self.values = new_tab(1, 0) @@ -615,16 +610,13 @@ local function load_full_data(self, dir_res, headers, prev_values, prev_values_h local key = short_key(self, item.key) local prev_item = get_prev_item(prev_values, prev_values_hash, key) if prev_item then - matched_prev = matched_prev + 1 + prev_keys_still_present = prev_keys_still_present + 1 end - -- Nothing changed for this key, so reuse the item we already have - -- instead of rebuilding it. This keeps the object identity stable, - -- which matters because downstream caches are keyed on it, and it - -- leaves `changed` alone so that a reload which changed nothing - -- does not bump conf_version and rebuild every router. - -- Same semantics as the incremental watch path in sync_data, which - -- only re-runs the checker and filter for the keys that changed. + -- Deliberately leaves `changed` alone, so a reload that changed + -- nothing does not bump conf_version and rebuild every router. + -- Same semantics as sync_data, which re-runs the checker and filter + -- only for the keys that changed. if prev_item and prev_item.modifiedIndex == item.modifiedIndex then insert_tab(self.values, prev_item) self.values_hash[key] = #self.values @@ -686,12 +678,10 @@ local function load_full_data(self, dir_res, headers, prev_values, prev_values_h ::continue:: end - -- Keys present in the previous snapshot but absent now were deleted - -- while we were not watching. Every surviving key can be untouched and - -- still leave us with a changed configuration, so this has to be - -- checked separately or a reload that only deletes would keep serving - -- the removed items. - if prev_values_hash and matched_prev < nkeys(prev_values_hash) then + -- A deletion leaves every surviving key untouched, so it has to be + -- detected separately or a reload that only deletes would keep + -- serving the removed items. + if prev_values_hash and prev_keys_still_present < nkeys(prev_values_hash) then changed = true end end diff --git a/t/core/config_etcd.t b/t/core/config_etcd.t index b556d1ef2016..02fbee1a7c67 100644 --- a/t/core/config_etcd.t +++ b/t/core/config_etcd.t @@ -559,10 +559,8 @@ nginx_config: ngx.sleep(1) end - -- The watch has timed out and restarted several times by now. Assert - -- it still delivers events for the watched prefix: the other - -- assertion here is the absence of a log line, which would also hold - -- if the watch were broken outright. + -- The only other assertion here is that a log line is absent, which + -- would also hold if the watch were broken outright. So check delivery. local _, err = etcd_cli:set("/apisix/routes/after-timeout", { id = "after-timeout", uri = "/after-timeout", @@ -574,9 +572,7 @@ nginx_config: ngx.say("failed to set route: ", err) return end - -- poll rather than sleep a fixed amount: delivery is normally - -- immediate, and a fixed wait would only be a guess about how slow - -- the etcd round trip can get on a loaded CI machine + -- polled, not slept: a fixed wait would just guess at CI latency local delivered = false for _ = 1, 25 do ngx.sleep(0.2) @@ -946,9 +942,8 @@ nginx_config: local obj = core.config.fetch_created_obj("/global_rules") local before_version = obj.conf_version - -- Two independent probes. A reload always builds a fresh `values` - -- array, so losing this one proves the reload actually ran; the - -- incremental path only mutates elements and would keep it. + -- A reload always builds a fresh `values` array, so losing this probe + -- proves it ran; the incremental path only mutates elements. obj.values.array_probe = "old" -- The items inside must survive: reusing them is the whole point. for _, item in ipairs(obj.values) do @@ -957,13 +952,10 @@ nginx_config: end end - -- Arm the recovery path taken after a `compacted` error, then write - -- a second rule. sync_data is parked in waitdir, so the write is - -- what wakes it: the incremental path adds /2 and bumps - -- conf_version once, and the next sync_data round reaches the - -- need_reload branch. By then /1 and /2 are both in memory at the - -- revisions etcd reports, so the reload has nothing to change and - -- must not bump conf_version a second time. + -- Arm the recovery path taken after `compacted`. sync_data is parked + -- in waitdir, so the write below is what wakes it: /2 arrives + -- incrementally (+1), then the reload runs with /1 and /2 already in + -- memory at the revisions etcd reports, so it must not bump again. obj.need_reload = true local _, err = etcd_cli:set("/apisix/global_rules/2", { id = "2", @@ -1049,11 +1041,9 @@ nginx_config: local obj = core.config.fetch_created_obj("/global_rules") obj.values.array_probe = "old" - -- An item that is live in memory but gone from etcd, so the reload - -- has to drop it. Every surviving key is untouched and therefore - -- reused, so without an explicit deletion check `changed` would - -- stay false, conf_version would not move, and the routers would - -- go on serving the dropped item. + -- Live in memory, gone from etcd. Every surviving key is untouched + -- and therefore reused, so without an explicit deletion check + -- conf_version would not move and the routers would keep serving it. local ghost = { key = "/apisix/global_rules/ghost", modifiedIndex = 1,