hotswap: compare config bytes exactly and check the marshal error

[dwin-gharibi]

Closes #1470.

Motivation

FileOperator.reload used bytes.EqualFold to decide whether the config had changed.
EqualFold is a case-insensitive comparison, so any edit that only changed ASCII letter case —
level: INFO → level: info, host: Foo.internal → host: foo.internal, driver: MySQL →
driver: mysql — was reported as "no change" and never pushed to listeners.

The yaml.Marshal error on the same line was also unchecked. On failure new is nil, so the
baseline backupOldCfg was wiped to nil and notify() still fired with the unmarshalable config.
Init() at :71-74 already checks the same call, so this was an inconsistency within one file.

What this changes

Both copies of the package — CubeMaster/pkg/base/hotswap/file.go and Cubelet/pkg/hotswap/file.go
— get the same two-line change:

new, err := yaml.Marshal(config)
if err != nil {
    return false
}
if bytes.Equal(o.backupOldCfg, new) {
    return false
}

bytes.Equal replaces EqualFold, and the len(...) == len(...) pre-check goes away because
bytes.Equal already compares lengths first. A marshal failure now returns false without
corrupting the baseline or notifying listeners.

No comment changes.

Per CONTRIBUTING's "one component per commit", this should land as two commits — one for CubeMaster,
one for Cubelet — since the two packages are separate copies. The diff is identical in both.

Testing

New: CubeMaster/pkg/base/hotswap/file_reload_test.go and Cubelet/pkg/hotswap/file_reload_test.go
(identical).

• TestReloadDetectsCaseOnlyKeyValueChange — INFO → info is detected.
• TestReloadDetectsCaseOnlyHostChange — Foo.internal → foo.internal is detected.
• TestReloadReportsNoChangeForIdenticalConfig — an unchanged file still reports no change, so the
  fix does not turn every watcher tick into a spurious notify.
• TestReloadDetectsOrdinaryChange — regression guard for the normal path.

The tests drive reload() directly via NewWatcher rather than Init(), so no fsnotify goroutine is
started.

Red/green verified — with file.go reverted to master:

--- FAIL: TestReloadDetectsCaseOnlyKeyValueChange
--- FAIL: TestReloadDetectsCaseOnlyHostChange
FAIL

and with the fix applied:

ok  github.com/tencentcloud/CubeSandbox/CubeMaster/pkg/base/hotswap  0.363s
ok  github.com/tencentcloud/CubeSandbox/Cubelet/pkg/hotswap          0.512s

CI gates checked locally:

• gofmt -l on both packages — clean (fmt-check).
• GOOS=linux go build on both packages — clean.

Risk / rollout

Low. The change makes reload strictly more sensitive, so the only behavioural difference is that
case-only edits now take effect — which is the intent. Listeners already had to tolerate being
notified on any change.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[cubesandboxbot]

Good catch checking the marshal error — previously a failed yaml.Marshal returned nil bytes, which wiped backupOldCfg to nil and still fired notify().

One suggestion: this is the only error path in this file that returns silently. load logs read/unmarshal failures and listenFile logs rewatch/add failures, but here a config that unmarshals yet fails to marshal will be silently ignored on every reload (the ticker retries forever with no signal). Consider logging before returning, consistent with the surrounding code:

if err != nil {
    o.Errorf(context.Background(), "Marshal file:%s fail:%v", path, err)
    return false
}

(Applies identically to the Cubelet/pkg/hotswap/file.go copy.)

[cubesandboxbot]

AI-generated review — not a human approval.

Verdict: Approve (with a minor suggestion)

This is a small, correct fix applied identically to the two vendored copies of the hotswap package. I reviewed the diff against the base branch (master) and verified the surrounding code in both CubeMaster/pkg/base/hotswap/file.go and Cubelet/pkg/hotswap/file.go; the PR head files are not checked out, so I reasoned from the base tree plus the diff.

What the change does

In (*FileOperator).reload:

new, err := yaml.Marshal(config)
if err != nil {
    return false
}
if bytes.Equal(o.backupOldCfg, new) {
    return false
}

Two fixes in one:

1. bytes.Equal replaces bytes.EqualFold — the comparison is now case-sensitive, so a case-only edit (level: INFO → level: info) is correctly reported as a change and pushed to listeners. This is the intended behavior per [Bug Report] Config hot-reload uses bytes.EqualFold, so case-only edits are silently ignored #1470.
2. The yaml.Marshal error is now checked — this is a real latent bug. On marshal failure new is nil, so the old len(...) == len(...) guard failed against a non-empty baseline and the code wiped backupOldCfg to nil and still called notify(). Now it returns false without corrupting the baseline, consistent with how Init() (:71-74) already handled the same call.

Verification against the base tree

• The len(...) == len(...) pre-check removal is safe. bytes.Equal already compares lengths first, so dropping the redundant guard changes nothing on that axis.
• The fix only makes detection stricter; it introduces no missed-change regressions. Exact equality is a subset of case-insensitive equality, so the only newly-detected edits are case-only ones — which is the point.
• No spurious notifications for semantically-identical edits. The comparison runs on the canonical marshalled form of the parsed config, not the raw file bytes. Key reordering, comment-only changes, whitespace, and case-normalized scalars (true → TRUE for booleans, 0x1A → 26) all re-marshal to identical bytes and still correctly report "no change". Case-only edits are detected only for string values, where the case genuinely changes the value — the correct semantics.
• The two packages are the only copies of this logic. I grepped the whole repo for EqualFold; the only occurrences of this reload comparison are the two hotswap/file.go files, and both are patched. No third copy was missed.
• Compiles / no unused imports. bytes is still used by bytes.Equal; the new err branch is checked. The new test files introduce no name collisions (write, newReloadProbe, reloadProbeCfg don't clash — these packages previously had zero test files).
• The tests are sound. They drive reload() directly via NewWatcher (no Init()), so no fsnotify goroutine starts and o.Watcher stays nil — reload never touches it. The "no change" test correctly relies on yaml.Marshal being deterministic for a plain struct (no map iteration), which holds. First-call baseline establishment returns true because bytes.Equal(nil, non-nil) is false.

Minor suggestions

1. Silent swallow of the marshal error (inline comment posted on the CubeMaster copy; applies identically to the Cubelet copy): every other error path in this file logs (load logs read/unmarshal failures, listenFile logs rewatch/add failures), but the new if err != nil { return false } returns silently. A config that unmarshals but fails to marshal will be silently ignored on every ticker tick with no operator-visible signal. Consider o.Errorf(context.Background(), "Marshal file:%s fail:%v", path, err) before returning.
2. No test exercises the new marshal-error branch. It's hard to trigger (requires a type that unmarshals but fails to marshal — e.g., a custom MarshalYAML that returns an error), but a small test would lock in the "baseline is preserved and listeners are not notified" behavior that this PR introduces. The four added tests only cover the happy paths.
3. Process nit (acknowledged in the PR body): the contributor already flags that per CONTRIBUTING's "one component per commit" this should land as two commits (one per package). Since the diff is byte-identical in both, keeping them split on merge is a reasonable choice; no action needed beyond what's stated.

Testing assessment

The four tests directly target the behavior change (INFO→info, Foo.internal→foo.internal) and include the important guard that an identical file still reports no change — which is what prevents this fix from turning every watcher tick into a spurious notify. Red/green evidence in the PR description is consistent with the static analysis. I could not run the tests myself (no shell in this review environment), but nothing in the test code suggests it would fail to compile or flake.

No security or concurrency concerns: reload runs on the single listenFile goroutine (or a test goroutine), and the change doesn't touch locking or the fsnotify lifecycle.