hot_fix: looping hot-reload flag config

[dewabisma]

Summary

Infinite loop hot-reload because of atomic change in file.

[n13]

Here's my review of PR #62 — hot_fix: looping hot-reload flag config.

---

Summary

The PR fixes an infinite loop in the WalletFeatureFlagsService file watcher. Atomic file saves (common in editors like vim, VS Code, etc.) emit bursts of events (Create, Rename, Modify, Metadata) which caused the watcher to reload continuously. The fix uses a three-pronged defense:

1. Event kind filtering — a new is_reload_event_kind method ignores Metadata and Other modify events
2. Debouncing — a 250ms tokio::select! debounce collapses event bursts into a single reload
3. Content equality check — skips the write and log if the parsed flags are identical to what's already held

CI is green.

---

What's Good

• Thorough approach. Any one of the three defenses would help, but layering all three makes the fix robust against different OS/editor behaviors.
• Correct debounce pattern. The biased select with a conditional sleep guard is idiomatic tokio. Events are always drained before the timer branch fires, so rapid-fire events keep pushing the reload out.
• Pre-extracting watched_file_name outside the loop avoids redundant work on every event.
• Existing tests still pass. The 250ms debounce fits well within the 3-second wait_until timeout in the existing test.

---

Issues / Suggestions

1. Initial sleep fires immediately (minor)

The sleep is created before the loop starts:

let reload_sleep = time::sleep(debounce_duration);
tokio::pin!(reload_sleep);

This sleep completes 250ms after service construction, not 250ms after the first event. The if reload_pending guard prevents spurious execution, so it's safe, but it means the very first reload event won't actually debounce for the full 250ms — it'll fire nearly instantly if the sleep already elapsed. Not a real problem in practice (the first event should reload fast), but worth being aware of. Initializing with a far-future instant (time::sleep_until(time::Instant::now() + Duration::from_secs(86400))) would be more precise if strictness is desired.

2. No test coverage for the new behavior

The PR introduces meaningful logic — event filtering and debouncing — but doesn't add tests for:

• Verifying that Metadata-only events don't trigger a reload
• Verifying that content-identical writes don't update the flags / emit a log
• Verifying that rapid bursts collapse into a single reload

Unit tests for is_reload_event_kind would be trivial and high-value since that function encodes the core filtering logic.

3. PartialEq on serde_json::Value — subtle ordering note

The content equality check:

if *write_guard == updated_flags {
    continue;
}

This works correctly when serde_json uses BTreeMap (the default). However, if the preserve_order feature is enabled (uses IndexMap), key insertion order affects equality. Worth confirming the project doesn't enable that feature — or noting it doesn't matter since the same file is always re-parsed into the same order.

4. PR description is sparse

The body just says "Infinite loop hot-reload because of atomic change in file." It would help future readers to briefly describe the symptoms (e.g., log spam, CPU spike) and the specific approach taken (event filtering + debounce + equality check).

---

Verdict

This is a solid, well-structured fix. The core logic is correct and the approach is sound. I'd suggest adding unit tests for is_reload_event_kind and the equality-check path before merging, but the fix itself is ready.