fix: change keyfile watcher to poll instead of inotify on FAT32 by pujitm · Pull Request #1820 · unraid/api

pujitm · 2025-12-08T15:09:28Z

Summary

Fixed GraphQL registration state not updating when license keys are installed/upgraded
Root cause: /boot/config is on FAT32 which doesn't support inotify - the file watcher was silently failing

Changes

Enable polling for key file watcher (required for FAT32 filesystem)
Add retry logic to reload var.ini after key changes to handle emhttpd update timing variation

Test plan

Unit tests for retry logic (will run in CI)
Manual test on Unraid: install/upgrade license key, verify GraphQL returns updated state within ~8 seconds

Summary by CodeRabbit

Tests
- Added a comprehensive test suite covering retry behavior, exponential backoff timing, and various registration-change scenarios.
Refactor
- Switched registration key monitoring to a polling-based watcher with an exponential-backoff retry for config reloads; added event logging and improved retry/stopping behavior to make state updates more reliable and observable.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai · 2025-12-08T15:09:58Z

Walkthrough

Adds reloadVarIniWithRetry(maxRetries) with exponential-backoff retrying of var.ini reloads until registration state (regTy) changes or retries exhausted; replaces inotify watcher with a 5s polling-based registration key watcher and adds tests validating retry paths and timing.

Changes

Cohort / File(s)	Summary
Registration watch + tests `api/src/store/watch/registration-watch.ts`, `api/src/__test__/store/watch/registration-watch.test.ts`	New exported `reloadVarIniWithRetry(maxRetries = 3)` implementing exponential backoff (500ms, 1000ms, 2000ms) that dispatches `loadSingleStateFile(StateFileKey.var)` repeatedly until `regTy` changes or max retries reached; watcher refactored to use 5s polling for key file changes and to call `loadRegistrationKey()` then `reloadVarIniWithRetry()`; new test suite added covering early exit, max-retries, stop-on-change, undefined regTy, and backoff timings using Vitest fake timers and mocked modules.

Sequence Diagram

sequenceDiagram
    participant Watcher as Key Watcher (polling)
    participant Logger
    participant Reg as Registration loader
    participant Store as Vuex Store
    participant Emhttp as emhttp (loadSingleStateFile / getters)

    Watcher->>Logger: log file event
    Watcher->>Reg: loadRegistrationKey()
    Watcher->>Store: call reloadVarIniWithRetry(maxRetries)

    par Retry loop
        Store->>Emhttp: read current regTy (before)
        Store->>Emhttp: loadSingleStateFile(StateFileKey.var)
        Emhttp-->>Store: action (state file payload)
        Store->>Store: dispatch(action)
        Store->>Emhttp: read regTy (after)
        alt regTy changed
            Store->>Logger: log change, stop retries
        else regTy unchanged and retries left
            Store->>Store: wait backoff (500ms → 1000ms → 2000ms)
            Store->>Store: increment retry count
        else max retries reached
            Store->>Logger: log max retries, stop
        end
    end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Review areas to focus on:
- reloadVarIniWithRetry timing, backoff calculation, and stop conditions
- Correct use of loadSingleStateFile dispatch and regTy comparisons (getters)
- Polling watcher replacement: interval setup and event logging
- Test reliability: fake timers, mocks, and asserted dispatch counts

Poem

🐰 I hopped on keys with patient paws,

I tried, retried, then tried some more,
Backoffs grew, the var danced true,
Polling beats the inotify score,
A rabbit cheers — the reloads roar! 🎉

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The pull request title clearly and specifically describes the main change: switching the keyfile watcher from inotify to polling for FAT32 compatibility.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch fix/registration-sync

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a4bea90 and ebdb013.

📒 Files selected for processing (1)

api/src/__test__/store/watch/registration-watch.test.ts (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

api/src/test/store/watch/registration-watch.test.ts

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

codecov · 2025-12-08T15:23:42Z

Codecov Report

❌ Patch coverage is 72.41379% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 52.06%. Comparing base (832e9d0) to head (ebdb013).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
api/src/store/watch/registration-watch.ts	72.41%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1820      +/-   ##
==========================================
+ Coverage   52.04%   52.06%   +0.02%     
==========================================
  Files         874      874              
  Lines       50372    50396      +24     
  Branches     5017     5014       -3     
==========================================
+ Hits        26214    26238      +24     
  Misses      24083    24083              
  Partials       75       75

☔ 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.

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (4)

api/src/store/watch/registration-watch.ts (2)

9-32: Retry helper is correct and bounded; consider guarding against overlapping calls

The reloadVarIniWithRetry logic looks sound: it compares regTy before/after, uses bounded exponential backoff, and logs clearly. One minor improvement: because this function can be triggered for every key‑file event, you could add a simple in‑flight guard (e.g., skip starting a new retry loop if one is already running) to avoid multiple overlapping reloads hammering loadSingleStateFile and producing noisy logs under bursty events. This is optional, since the current behavior is still correct and side‑effect safe.

34-50: Chokidar watcher config fits FAT32 needs; verify ignored behavior and consider a glob for clarity

The switch to polling with usePolling: true and a 5s interval is appropriate for /boot/config on FAT32, and the extra logging on key events plus reloadVarIniWithRetry wiring looks good. Two minor points:

With ignored: (path) => !path.endsWith('.key'), depending on chokidar internals, there’s a small risk the root directory path (/boot/config) might also be treated as ignored. Please verify in manual testing that .key changes under /boot/config reliably emit events. If there’s any flakiness, a more explicit pattern like watch('/boot/config/*.key', { ... }) (and dropping the ignored callback) would avoid ambiguity.

If in the future you need to process an already‑present key at startup (similar to the DockerEventService behavior), you may want ignoreInitial: false so existing .key files trigger the handler once on boot; for this PR your separate startup logic likely makes ignoreInitial: true fine. Based on learnings, this mirrors how Docker’s watcher ensures existing files are processed at startup.
api/src/__test__/store/watch/registration-watch.test.ts (2)
34-53: Simplify mock typing to avoid double casts in beforeEach

The pattern:

let store: { dispatch: ReturnType<typeof vi.fn> };

store = storeModule.store as unknown as typeof store;

Similar for getters and loadSingleStateFile

works but relies on as unknown as casts that the guidelines try to avoid. You can make this cleaner by declaring a typed interface for the mocked modules and casting once at import, e.g.:
type MockedStoreModule = {
    store: { dispatch: ReturnType<typeof vi.fn> };
    getters: { emhttp: ReturnType<typeof vi.fn> };
};

const storeModule = (await import('@app/store/index.js')) as MockedStoreModule;
const emhttpModule = (await import('@app/store/modules/emhttp.js')) as {
    loadSingleStateFile: ReturnType<typeof vi.fn>;
};
This keeps everything strongly typed without chaining unknown casts.

55-149: Tests give good coverage; consider asserting with StateFileKey.var instead of 'var'

The test scenarios for reloadVarIniWithRetry (early change, max‑retry, mid‑loop change, undefined regTy, and backoff timing) are thorough and align well with the helper’s behavior. One small robustness tweak: in the first test you assert
expect(loadSingleStateFile).toHaveBeenCalledWith('var');
Since the production code calls loadSingleStateFile(StateFileKey.var), importing StateFileKey into the test and asserting against StateFileKey.var would keep the test resilient if the enum’s underlying string value ever changes:
import { StateFileKey } from '@app/store/types.js';

expect(loadSingleStateFile).toHaveBeenCalledWith(StateFileKey.var);
Functionally everything is correct as‑is; this would just decouple the test from the literal enum value.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 832e9d0 and a4bea90.

📒 Files selected for processing (2)

api/src/__test__/store/watch/registration-watch.test.ts (1 hunks)
api/src/store/watch/registration-watch.ts (1 hunks)

🧰 Additional context used

📓 Path-based instructions (10)

**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.{ts,tsx,js,jsx}: Always use TypeScript imports with .js extensions for ESM compatibility
Never add comments unless they are needed for clarity of function
Never add comments for obvious things, and avoid commenting when starting and ending code blocks

Files:

api/src/store/watch/registration-watch.ts
api/src/__test__/store/watch/registration-watch.test.ts

api/**/*

📄 CodeRabbit inference engine (CLAUDE.md)

Prefer adding new files to the NestJS repo located at api/src/unraid-api/ instead of the legacy code

Files:

api/src/store/watch/registration-watch.ts
api/src/__test__/store/watch/registration-watch.test.ts

**/*.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.{ts,tsx}: Never use the any type. Always prefer proper typing
Avoid using casting whenever possible, prefer proper typing from the start

Files:

api/src/store/watch/registration-watch.ts
api/src/__test__/store/watch/registration-watch.test.ts

api/**/*.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

cache-manager v7 expects TTL values in milliseconds, not seconds (e.g., 600000 for 10 minutes, not 600)

Files:

api/src/store/watch/registration-watch.ts
api/src/__test__/store/watch/registration-watch.test.ts

**/*

📄 CodeRabbit inference engine (.cursor/rules/default.mdc)

Never add comments unless they are needed for clarity of function

Files:

api/src/store/watch/registration-watch.ts
api/src/__test__/store/watch/registration-watch.test.ts

**/store/**/*.ts

📄 CodeRabbit inference engine (.cursor/rules/web-testing-rules.mdc)

Ensure Vue reactivity imports like computed, ref, and watchEffect are added to store files even with Nuxt auto-imports enabled

Files:

api/src/store/watch/registration-watch.ts
api/src/__test__/store/watch/registration-watch.test.ts

**/*.test.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.test.{ts,tsx,js,jsx}: Use VITEST for test suite, not jest
Use .rejects.toThrow() without arguments to test that functions throw errors, not exact error message strings

Files:

api/src/__test__/store/watch/registration-watch.test.ts

api/**/*.test.{ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/api-rules.mdc)

api/**/*.test.{ts,tsx}: Use Vitest for the test suite, not Jest
Prefer to not mock simple dependencies
For error testing, use .rejects.toThrow() without arguments; do not test exact error message strings unless the message format is specifically what you're testing

Files:

api/src/__test__/store/watch/registration-watch.test.ts

**/*.test.ts

📄 CodeRabbit inference engine (.cursor/rules/web-testing-rules.mdc)

**/*.test.ts: Use .rejects.toThrow() without arguments to test that functions throw errors. Don't test exact error message strings unless the message format is specifically what you're testing
Test what the code does, not implementation details like exact error message wording
Mock external services and API calls
Use vi.mock() for module-level mocks
Specify return values for component methods with vi.spyOn()
Reset mocks between tests with vi.clearAllMocks()
Always await async operations before making assertions

Files:

api/src/__test__/store/watch/registration-watch.test.ts

**/__test__/store/**/*.ts

📄 CodeRabbit inference engine (.cursor/rules/web-testing-rules.mdc)

**/__test__/store/**/*.ts: Use createPinia and setActivePinia when testing Pinia store files
Use createPinia() instead of createTestingPinia() for most Pinia store tests
Only use createTestingPinia if you specifically need its testing features for Pinia stores
Let stores initialize with their natural default state instead of forcing initial state
Do not mock the store being tested in the test file when using createPinia()
Place all mock declarations at the top level in Pinia store tests
Use factory functions for module mocks in Pinia store tests to avoid hoisting issues
Test Pinia action side effects and state changes
Verify Pinia actions are called with correct parameters
Mock external dependencies appropriately in Pinia store tests
Test computed properties in Pinia stores by accessing them directly
Verify state changes after Pinia store actions
Test Pinia store getter dependencies are properly mocked
Test Pinia store interactions with other stores
Verify proper error handling in Pinia store tests
Test async operations completely in Pinia store tests
Override specific Pinia action implementations when needed in tests
Set initial state for focused Pinia store testing

Files:

api/src/__test__/store/watch/registration-watch.test.ts

🧠 Learnings (11)

📚 Learning: 2025-04-07T14:34:47.255Z

Learnt from: elibosley
Repo: unraid/api PR: 1334
File: api/src/unraid-api/graph/resolvers/docker/docker-event.service.ts:63-66
Timestamp: 2025-04-07T14:34:47.255Z
Learning: In DockerEventService, the chokidar file watcher is configured with `ignoreInitial: false` to ensure that existing files (like the Docker socket) are detected and processed at application startup, not just when files change after the watcher is initialized.

Applied to files:

api/src/store/watch/registration-watch.ts