fix: reload var state when emhttp writes temp files

[Ajit-Mehrotra]

Summary

• normalize *.ini.new state-file events back to their underlying state file key in the state watcher
• stop ignoring the watched root state directory when chokidar calls the ignore callback without stats
• add unit and integration coverage for the temp-file watcher path

Why

On the Unraid server, emhttp writes temp files like var.ini.new and swaps them in. The existing watcher only recognized exact *.ini basenames, so var.ini.new updates were ignored and the API store could stay stale even when disk state was correct.

Testing

• pnpm --filter ./api test src/__test__/store/watch/state-watch.test.ts src/__test__/store/watch/state-watch.integration.test.ts src/unraid-api/graph/resolvers/servers/server.service.spec.ts
• pnpm build
• deployed to 192.168.1.176 with pnpm unraid:deploy 192.168.1.176
• verified live GraphQL returned Gamer5 after restart
• verified live rename flow Gamer5 -> Gamer5tmp -> Gamer5 and saw GraphQL follow both changes correctly

Summary by CodeRabbit

Release Notes

• New Features

  • StateManager now provides a public method to gracefully close and clean up file watchers.

• Tests

  • Added comprehensive integration test suite for StateManager that validates state reload behavior triggered by filesystem events in the state directory.
  • Extended directory watcher tests with validation for temporary state file variants and proper event handling during state updates.

[coderabbitai]

Walkthrough

This PR adds comprehensive testing and filename normalization logic for handling temporary state files (.new variants) in the StateManager's file watcher. It introduces a normalization function to map temporary files to their original state file counterparts, updates the directory watcher's ignore predicate, adds a close mechanism to StateManager, and validates the behavior through new integration and unit tests.

Changes

Cohort / File(s)	Summary
State Watch Implementation api/src/store/watch/state-watch.ts	Added getNormalizedStateFileName() to normalize *.new temporary files to their *.ini counterparts. Updated shouldIgnoreStatePath() and getStateFileKeyFromPath() to use normalization for proper file filtering and key resolution. Modified chokidarOptionsForStateDirectory() to accept statesPath parameter and exclude the directory itself from being ignored. Added StateManager.close() method to asynchronously close watchers and reset the singleton instance.
State Watch Tests api/src/__test__/store/watch/state-watch.test.ts	Extended directory watcher's ignored predicate tests to validate handling of *.new files and the states directory itself. Added test case simulating add event for var.ini.new file and verifying correct store dispatch sequence (loadSingleStateFile followed by loadRegistrationKey).
State Watch Integration Tests api/src/__test__/store/watch/state-watch.integration.test.ts	Introduced new Vitest integration test suite with temporary directory setup/teardown. First test validates that writing var.ini.new triggers correct store dispatch sequence. Second test confirms that disks.ini.new updates do not trigger dispatch, validating file-specific filtering logic.

Sequence Diagram

sequenceDiagram
    participant FileWatcher as File Watcher
    participant StateManager as StateManager
    participant Normalizer as getNormalizedStateFileName()
    participant Filter as shouldIgnoreStatePath()
    participant KeyResolver as getStateFileKeyFromPath()
    participant Store as Store

    FileWatcher->>StateManager: File event (var.ini.new)
    StateManager->>Normalizer: Normalize filename
    Normalizer-->>StateManager: "var.ini"
    StateManager->>Filter: Check if should ignore
    Filter->>Normalizer: Get normalized name
    Normalizer-->>Filter: "var.ini"
    Filter-->>StateManager: false (not ignored)
    StateManager->>KeyResolver: Get StateFileKey
    KeyResolver->>Normalizer: Normalize for key lookup
    Normalizer-->>KeyResolver: "var.ini"
    KeyResolver-->>StateManager: StateFileKey.var
    StateManager->>Store: dispatch(loadSingleStateFile(var))
    StateManager->>Store: dispatch(loadRegistrationKey())
    Store-->>StateManager: Acknowledged

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 With whiskers twitching at each file,
The watcher now sees .new with style!
Normalize, filter, dispatch with grace—
Temp files find their rightful place! ✨

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix: reload var state when emhttp writes temp files' accurately summarizes the main change: handling temporary .ini.new state files written by emhttp to trigger state reloads.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch codex/fix-state-watch-temp-files

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codecov]

Codecov Report

❌ Patch coverage is 93.54839% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 51.77%. Comparing base (776c8cc) to head (0e66a0f).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
api/src/store/watch/state-watch.ts	93.54%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1950      +/-   ##
==========================================
+ Coverage   51.74%   51.77%   +0.02%     
==========================================
  Files        1026     1026              
  Lines       70725    70748      +23     
  Branches     7894     7903       +9     
==========================================
+ Hits        36599    36628      +29     
+ Misses      34003    33997       -6     
  Partials      123      123              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

This plugin has been deployed to Cloudflare R2 and is available for testing.
Download it at this URL:

https://preview.dl.unraid.net/unraid-api/tag/PR1950/dynamix.unraid.net.plg

[coderabbitai]

🧹 Nitpick comments (2)

api/src/__test__/store/watch/state-watch.integration.test.ts (2)

100-107: Acceptable approach for negative test, but consider documenting the delay.

For negative tests (asserting something did NOT happen), a fixed delay is often necessary. The 250ms delay is reasonable, though a brief comment explaining the timing choice would help maintainability.

💡 Optional comment addition

     it('does not route disks.ini.new through the directory watcher', async () => {
         const { store } = await import('@app/store/index.js');
 
         await writeFile(join(statesDirectory, 'disks.ini.new'), '[disk1]\nname=disk1\n');
+        // Allow sufficient time for the watcher to process the file event
         await new Promise((resolve) => setTimeout(resolve, 250));
 
         expect(store.dispatch).not.toHaveBeenCalled();
     });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@api/src/__test__/store/watch/state-watch.integration.test.ts` around lines
100 - 107, Add a brief comment explaining the 250ms delay in the test "does not
route disks.ini.new through the directory watcher" so future readers understand
it’s a conservative wait for the file-watcher debounce/async processing; locate
the test block containing the setTimeout call (the async it(...) that writes
'disks.ini.new' and awaits new Promise((resolve) => setTimeout(resolve, 250)))
and insert a one-line comment above the await describing why 250ms was chosen
(e.g., to allow the watcher’s debounce/async loop to run) — no code behavior
changes required.

---

73-73: Consider using vi.waitFor instead of fixed delay.

The 500ms fixed delay for watcher readiness could cause flakiness in CI environments with varying I/O performance. Consider using vi.waitFor with a condition that confirms the watcher is ready, similar to the pattern used in the first test (lines 94-97).

💡 Suggested approach

If chokidar emits a 'ready' event, you could wait for that signal instead of a fixed timeout. Alternatively, if no reliable signal exists, the current approach may be acceptable but consider increasing the timeout or adding a comment explaining why 500ms was chosen.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@api/src/__test__/store/watch/state-watch.integration.test.ts` at line 73,
Replace the fixed 500ms delay (the await new Promise(setTimeout) call) with a
deterministic wait using vi.waitFor that verifies the watcher is ready; for
example, wait for the chokidar watcher to emit its 'ready' event or for a
boolean/spy indicating readiness (reference the watcher instance used in this
test and the vi.waitFor helper), falling back to increasing the timeout only if
no readiness signal is available.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@api/src/__test__/store/watch/state-watch.integration.test.ts`:
- Around line 100-107: Add a brief comment explaining the 250ms delay in the
test "does not route disks.ini.new through the directory watcher" so future
readers understand it’s a conservative wait for the file-watcher debounce/async
processing; locate the test block containing the setTimeout call (the async
it(...) that writes 'disks.ini.new' and awaits new Promise((resolve) =>
setTimeout(resolve, 250))) and insert a one-line comment above the await
describing why 250ms was chosen (e.g., to allow the watcher’s debounce/async
loop to run) — no code behavior changes required.
- Line 73: Replace the fixed 500ms delay (the await new Promise(setTimeout)
call) with a deterministic wait using vi.waitFor that verifies the watcher is
ready; for example, wait for the chokidar watcher to emit its 'ready' event or
for a boolean/spy indicating readiness (reference the watcher instance used in
this test and the vi.waitFor helper), falling back to increasing the timeout
only if no readiness signal is available.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 37780d66-536e-4c38-baab-98491f5125a4

📥 Commits

Reviewing files that changed from the base of the PR and between 8f4ea98 and 0e66a0f.

📒 Files selected for processing (3)

• api/src/__test__/store/watch/state-watch.integration.test.ts
• api/src/__test__/store/watch/state-watch.test.ts
• api/src/store/watch/state-watch.ts