From 611e9c04bc6bf2ead2a6c3e5670f525af4323f29 Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Fri, 21 Aug 2026 00:04:27 +0100 Subject: [PATCH] docs(hot-reload): honest settle semantics and TTL comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Micro-fixes from re-review, all text/bound-level, no behavior change to production logic: 1. settleToBaseline's doc comment (config_hot_reload_test.go) claimed to guard the propagation window unconditionally. Reworded: it is a real guard on the fired path (two known rebuilds to wait out), best effort only on the skip path, where no amount of polling can prove a rebuild will never eventually fire — waiting is still required there because the earlier disappear-only check was a guaranteed-zero-wait bug, not because settling can prove safety on that path. 2. Restored the cleanup settle bound from 20s to 30s, matching the longest propagation delay actually observed during verification. 3. README's Config hot-reload intro dropped "on every run" and now states the same fired-vs-skip distinction the test's own Skip message makes: zero non-200s is blip-free-reload evidence only when the watch fires within the test's bound; otherwise it shows just that the stack survived an unreloaded rewrite attempt. 4. auth.go's group.cacheTTL field comment still said a positive value "overrides it for this group's cache entries only" — reworded to the corrected write-time/shared semantics: it sets the TTL written when that group's own request populates an entry, entries are shared across groups, not scoped by group. Gate: go build/vet/gofmt clean on both modules; go test ./... -count=1 green. --- README.md | 10 ++++++-- auth.go | 12 ++++++--- integration/config_hot_reload_test.go | 37 +++++++++++++++------------ 3 files changed, 37 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index 420a72a..d4c29a3 100644 --- a/README.md +++ b/README.md @@ -897,8 +897,14 @@ Traefik's exact draining mechanism for requests already in flight on the old instance at swap time. What is verified: the integration suite's sustained-traffic reload test (`TestConfigHotReload`, `integration/config_hot_reload_test.go`) sends requests throughout the -whole rewrite-and-propagation window and sees zero non-200 responses, on -every run. +whole rewrite-and-propagation window and sees zero non-200 responses. +That result means different things depending on whether Traefik's +directory watch actually picks up the change within the test's own +bound. When it does, zero non-200s is evidence of a blip-free reload. +When it does not — a host-dependent propagation delay, not a plugin +defect — the same zero-non-200s result only shows the stack stayed +healthy through an unreloaded rewrite attempt, not that a reload itself +is blip-free. A rebuilt instance keeps some state and resets the rest: diff --git a/auth.go b/auth.go index 6abdfbc..cac3772 100644 --- a/auth.go +++ b/auth.go @@ -37,10 +37,14 @@ type group struct { agents []string // cacheTTL is GroupConfig.CacheTTL parsed and validated at construction // (newAuthStore below): 0 inherits the global responseCache's TTL - // (effectiveTTL, cache.go), a positive value overrides it for this - // group's cache entries only. GroupConfig.CacheTTL == "" is the only - // input that produces 0 here — every other value either becomes a - // positive duration or fails newAuthStore as a constructor error. + // (effectiveTTL, cache.go); a positive value sets the TTL written + // when THIS group's own request populates a cache entry. It is not a + // per-group scope — cache entries are shared across every group that + // can reach the model (groupCacheEnabled, cache.go), so a positive + // cacheTTL only ever controls a write's TTL, never which group can + // later read the entry. GroupConfig.CacheTTL == "" is the only input + // that produces 0 here — every other value either becomes a positive + // duration or fails newAuthStore as a constructor error. cacheTTL time.Duration } diff --git a/integration/config_hot_reload_test.go b/integration/config_hot_reload_test.go index be4704f..d1d9163 100644 --- a/integration/config_hot_reload_test.go +++ b/integration/config_hot_reload_test.go @@ -116,21 +116,26 @@ const settleConsecutivePolls = 3 // consecutive polls, or bound elapses without ever reaching that streak; // it returns whether it settled. // -// This is called unconditionally by TestConfigHotReload's cleanup, on -// BOTH outcomes of the earlier alias poll (the watch firing within its -// own bound, or not) — fixing a real bug found during verification: an +// TestConfigHotReload's cleanup calls this unconditionally, on both +// outcomes of the earlier alias poll. On the fired path it is a real +// guard: two rebuilds are known to have been triggered (the original +// add, then this cleanup's own restore), and settling confirms both +// landed before cleanup returns. On the skip path it is best effort +// only — if the original add's rebuild is still pending past the 30s +// alias-poll bound, or never fires at all, no amount of polling here can +// prove that; it can only improve the odds that a still-in-flight +// rebuild resolves before this test hands control back. +// +// Waiting here is still required on the skip path, not a no-op: an // earlier version of this cleanup polled only for the injected alias to -// DISAPPEAR, which trivially and instantly "succeeded" on the path where -// the watch never fired at all, since the alias was never live in the -// first place and "not present" was already true on the very first -// check. That left the exact propagation window this function now -// guards completely unwatched, and it raced TestUsersFileHotReload's own -// (pre-existing, non-atomic) write to users.json during verification: a -// rebuild this test's own restore triggered landed later, mid-write, read -// a torn file, and failed plugin construction outright — a global 404 -// outage until some later, unrelated config change happened to trigger a -// successful rebuild. Requiring a real settle, on every path, removes -// that window. +// DISAPPEAR, which trivially and instantly "succeeded" on that path (the +// alias was never live, so "not present" was already true on the first +// check) — a guaranteed-zero-wait bug, found when it raced +// TestUsersFileHotReload's own non-atomic write to users.json during +// verification: a rebuild this test's own restore triggered landed +// later, mid-write, read a torn file, and failed plugin construction +// outright — a global 404 outage until an unrelated later config change +// triggered a successful rebuild. func settleToBaseline(baseURL, apiKey string, baseline map[string]bool, bound time.Duration) bool { consecutive := 0 deadline := time.Now().Add(bound) @@ -319,8 +324,8 @@ func TestConfigHotReload(t *testing.T) { t.Errorf("rename restore temp config over %q: %v", configPath, err) return } - if !settleToBaseline(traefik1URL, hotReloadProbeKey, baselineIDs, 20*time.Second) { - t.Logf("cleanup: /v1/models did not settle back to the baseline model set within 20s of restoring the config — a later test may race a still-pending rebuild") + if !settleToBaseline(traefik1URL, hotReloadProbeKey, baselineIDs, 30*time.Second) { + t.Logf("cleanup: /v1/models did not settle back to the baseline model set within 30s of restoring the config — a later test may race a still-pending rebuild") } })