Skip to content

fix(config): poll for reloaded org instead of reload counter in flaky watcher test - #5464

Merged
kubestellar-prow[bot] merged 1 commit into
v4from
fix/5448-flaky-watcher-test
Sep 1, 2026
Merged

fix(config): poll for reloaded org instead of reload counter in flaky watcher test#5464
kubestellar-prow[bot] merged 1 commit into
v4from
fix/5448-flaky-watcher-test

Conversation

@clubanderson

Copy link
Copy Markdown
Contributor

Fixes #5448

Root cause — a race in the test, not in the watcher

TestWatcher_ReloadsOnChange failed the required v2 Tests gate on PRs that do not touch pkg/config (#5440, run 33488167240, job test (rest 2/3)):

watcher_test.go:62: expected org = "updated-org" after reload, got ""

The onChange callback increments the counter before it stores the value:

reloadCount.Add(1)
lastOrg.Store(cfg.Project.Org)

while the wait loop exited as soon as reloadCount > 0 and then immediately read lastOrg. If the poll observes the counter in the window between those two statements, lastOrg is still unset, the type assertion lastOrg.Load().(string) returns ok=false with the zero value, and the test reports got "".

The 50ms poll interval usually hides the window; a scheduler stall under -race on a loaded runner lands the observation inside it.

This is not the originally suspected cause

The issue proposed a partial read of a non-atomic os.WriteFile. That mechanism cannot produce this failure — every truncation of the config is rejected before onChange runs:

written content LoadWithDashboardOverlay result
empty error: project.org is required
first 1/2 error: yaml: unmarshal errors
first 1/4 error: could not find expected ':'
valid YAML, no org error: project.org is required

A failed load returns early and never invokes onChange, so no empty org can ever be stored. The observed "" is the never-stored case, which is why the fix targets the observation order rather than write atomicity.

Fix

Poll for the observable outcome (lastOrg == "updated-org") instead of the reload counter, with a generous deadline and a tighter poll interval.

The assertion is not weakened: the test still requires reloadCount > 0 and still requires the reloaded config to carry exactly updated-org. A watcher that failed to reload, or reloaded stale content, still fails — now by timing out rather than by reading a half-published result. The failure message also reports the reload count for easier diagnosis.

Verification (all -short -race)

Reproduced deterministically by injecting a stall between the increment and the store, modelling a scheduler preemption:

scenario before after
120ms stall injected 10/10 FAIL (identical message) 0/25 fail
3s stall injected (extreme) 0/10 fail
real test, -count=200 0/200 fail
all TestWatcher_*, -count=100 under 12-way CPU saturation 0 fail

The pre-fix run reproduces the exact CI message and line, and the fix survives a 3s stall — far beyond any plausible CI delay, so this is not merely a retuned timeout.

Notes

🤖 Generated with Claude Code

… watcher test

TestWatcher_ReloadsOnChange failed the required v2 Tests gate on PRs that
do not touch pkg/config (e.g. #5440, run 33488167240, job `test (rest 2/3)`)
with:

    watcher_test.go:62: expected org = "updated-org" after reload, got ""

Root cause is a race in the test, not in the watcher. The onChange callback
increments reloadCount before it stores lastOrg:

    reloadCount.Add(1)
    lastOrg.Store(cfg.Project.Org)

while the wait loop exited as soon as reloadCount > 0 and then immediately
read lastOrg. When the poll observes the counter in the window between those
two statements, lastOrg is still unset and the type assertion yields "" — the
exact reported failure. The window is tiny locally but is readily hit under
-race on a loaded CI runner.

Note this is NOT a partial/truncated read of a non-atomic os.WriteFile as
first suspected: every truncation of the config fails to parse or fails
validation ("project.org is required"), so onChange is never invoked and no
empty org can ever be stored. The observed "" is the never-stored case.

Fix: poll for the observable outcome (lastOrg == "updated-org") rather than
for the reload counter, with a generous deadline and a tighter poll interval.
The assertion is unchanged in strength — the test still proves the watcher
reloads and delivers the new content, and still requires reloadCount > 0.

Verification, all under -short -race:
- Reproduced deterministically by injecting a stall between the increment and
  the store (modelling a scheduler preemption): 10/10 failures with the
  identical message, 0/25 after the fix; still 0/10 with a 3s stall.
- Real test: 200/200 pass; all three TestWatcher_* 100x under 12-way CPU
  saturation: pass.

Fixes #5448

Signed-off-by: Andrew Anderson <andy@clubanderson.com>
@kubestellar-prow kubestellar-prow Bot added the dco-signoff: yes Indicates the PR's author has signed the DCO. label Sep 1, 2026
@clubanderson clubanderson added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 1, 2026
@kubestellar-prow kubestellar-prow Bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 1, 2026
@kubestellar-prow

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by:

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown

Changelog: this PR changes code but does not touch CHANGELOG.md.

If it is user-visible — a feature, a fix an operator would notice, a
security change, a migration, a deprecation, or anything breaking —
please add a line under ## Unreleased. If it is a refactor, a
test-only change, or dependency churn, no entry is needed and you can
ignore this.

This is a reminder, not a gate; it never blocks a merge.

@kubestellar-prow
kubestellar-prow Bot merged commit 7417caa into v4 Sep 1, 2026
52 checks passed
@kubestellar-prow
kubestellar-prow Bot deleted the fix/5448-flaky-watcher-test branch September 1, 2026 13:24
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown

Thank you for your contribution! Your PR has been merged.

Check out what's new:

Stay connected: Slack #kubestellar-dev | Multi-Cluster Survey

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has signed the DCO. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[ci-maintainer] flaky: TestWatcher_ReloadsOnChange (pkg/config) — non-atomic WriteFile lets watcher reload a partial file, org=""

1 participant