Skip to content

Commit 6471b3f

Browse files
elibosleyelibosley
authored
fix(state-watch): watch canonical ini files atomically (#1953)
## Summary - switch the state watcher back to watching canonical `*.ini` files directly instead of using directory-level `.ini.new` normalization - configure an explicit atomic replacement window for state file watchers so `.ini.new -> .ini` replacements collapse into canonical file change handling - keep the existing polling behavior for `disks.ini` and `shares.ini`, and update the state watcher tests to cover the canonical-file watcher shape ## Testing - `pnpm --filter ./api test src/__test__/store/watch/state-watch.test.ts` - `pnpm --filter ./api test src/__test__/store/watch/state-watch.integration.test.ts` - deployed `api` to `devgen` via `pnpm --filter ./api unraid:deploy devgen` - verified on `devgen` that an atomic `var.ini.new -> var.ini` replacement emitted `Loading state file for var after change` in `/var/log/graphql-api.log` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Enhanced state file watching tests to verify per-file monitoring with atomic replacement detection and conditional polling behavior. * **Refactor** * Improved state file watching mechanism for more reliable detection of file changes and atomic file replacements across critical system state files. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 parent 7265105 commit 6471b3f

File tree

3 files changed

+47
-117
lines changed

3 files changed

+47
-117
lines changed

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

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises';
1+
import { mkdir, mkdtemp, rename, rm, writeFile } from 'node:fs/promises';
22
import { tmpdir } from 'node:os';
33
import { join } from 'node:path';
44

@@ -64,6 +64,8 @@ describe('StateManager integration', () => {
6464
tempRoot = await mkdtemp(join(tmpdir(), 'state-watch-'));
6565
statesDirectory = join(tempRoot, 'state');
6666
await mkdir(statesDirectory);
67+
await writeFile(join(statesDirectory, 'var.ini'), 'NAME="Before"\n');
68+
await writeFile(join(statesDirectory, 'disks.ini'), '[disk1]\nname=disk1\n');
6769
testContext.states = statesDirectory;
6870

6971
const { StateManager } = await import('@app/store/watch/state-watch.js');
@@ -84,20 +86,21 @@ describe('StateManager integration', () => {
8486
await rm(tempRoot, { recursive: true, force: true });
8587
});
8688

87-
it('reloads var state when emhttp writes var.ini.new into the state directory', async () => {
89+
it('reloads var state when emhttp atomically replaces var.ini', async () => {
8890
const { store } = await import('@app/store/index.js');
8991
const { loadSingleStateFile } = await import('@app/store/modules/emhttp.js');
9092
const { loadRegistrationKey } = await import('@app/store/modules/registration.js');
9193

9294
await writeFile(join(statesDirectory, 'var.ini.new'), 'NAME="Gamer5"\n');
95+
await rename(join(statesDirectory, 'var.ini.new'), join(statesDirectory, 'var.ini'));
9396

9497
await vi.waitFor(() => {
9598
expect(store.dispatch).toHaveBeenNthCalledWith(1, loadSingleStateFile(StateFileKey.var));
9699
expect(store.dispatch).toHaveBeenNthCalledWith(2, loadRegistrationKey());
97100
});
98101
});
99102

100-
it('does not route disks.ini.new through the directory watcher', async () => {
103+
it('does not react to disks.ini.new before the canonical file is replaced', async () => {
101104
const { store } = await import('@app/store/index.js');
102105

103106
await writeFile(join(statesDirectory, 'disks.ini.new'), '[disk1]\nname=disk1\n');

‎api/src/__test__/store/watch/state-watch.test.ts‎

Lines changed: 26 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -73,34 +73,45 @@ describe('StateManager', () => {
7373
StateManager.instance = null;
7474
});
7575

76-
it('watches the emhttp state directory and keeps polling scoped to replacement-prone files', async () => {
76+
it('watches each canonical emhttp state file and keeps polling scoped to replacement-prone files', async () => {
7777
const { StateManager } = await import('@app/store/watch/state-watch.js');
7878

7979
await StateManager.getInstance().ready;
8080

81-
expect(chokidarWatch).toHaveBeenCalledTimes(3);
81+
expect(chokidarWatch).toHaveBeenCalledTimes(Object.values(StateFileKey).length);
8282
expect(chokidarWatch).toHaveBeenNthCalledWith(
8383
1,
84-
'/usr/local/emhttp/state',
84+
'/usr/local/emhttp/state/var.ini',
8585
expect.objectContaining({
86+
atomic: 200,
8687
ignoreInitial: true,
8788
usePolling: false,
88-
ignored: expect.any(Function),
8989
})
9090
);
9191
expect(chokidarWatch).toHaveBeenNthCalledWith(
9292
2,
93-
'/usr/local/emhttp/state/disks.ini',
93+
'/usr/local/emhttp/state/devs.ini',
94+
expect.objectContaining({
95+
atomic: 200,
96+
ignoreInitial: true,
97+
usePolling: false,
98+
})
99+
);
100+
expect(chokidarWatch).toHaveBeenNthCalledWith(
101+
5,
102+
'/usr/local/emhttp/state/shares.ini',
94103
expect.objectContaining({
104+
atomic: 200,
95105
ignoreInitial: true,
96106
usePolling: true,
97107
interval: 10_000,
98108
})
99109
);
100110
expect(chokidarWatch).toHaveBeenNthCalledWith(
101-
3,
102-
'/usr/local/emhttp/state/shares.ini',
111+
6,
112+
'/usr/local/emhttp/state/disks.ini',
103113
expect.objectContaining({
114+
atomic: 200,
104115
ignoreInitial: true,
105116
usePolling: true,
106117
interval: 10_000,
@@ -127,7 +138,7 @@ describe('StateManager', () => {
127138
expect(dispatchedStateLoads).toEqual(Object.values(StateFileKey));
128139
});
129140

130-
it('routes non-polled state files through the standard directory watcher', async () => {
141+
it('routes non-polled state files through their canonical file watchers', async () => {
131142
const { StateManager } = await import('@app/store/watch/state-watch.js');
132143
const { store } = await import('@app/store/index.js');
133144
const { loadSingleStateFile } = await import('@app/store/modules/emhttp.js');
@@ -136,7 +147,7 @@ describe('StateManager', () => {
136147
vi.mocked(store.dispatch).mockClear();
137148

138149
const standardWatcher = watchRegistrations.find(
139-
(registration) => registration.options.usePolling === false
150+
(registration) => registration.path === '/usr/local/emhttp/state/devs.ini'
140151
);
141152
const changeHandler = standardWatcher?.handlers.change;
142153
expect(changeHandler).toBeDefined();
@@ -146,29 +157,6 @@ describe('StateManager', () => {
146157
expect(store.dispatch).toHaveBeenCalledWith(loadSingleStateFile(StateFileKey.devs));
147158
});
148159

149-
it('ignores non-state files while still allowing non-polled state files through the directory watcher', async () => {
150-
const { StateManager } = await import('@app/store/watch/state-watch.js');
151-
152-
await StateManager.getInstance().ready;
153-
154-
const standardWatcher = watchRegistrations.find(
155-
(registration) => registration.options.usePolling === false
156-
);
157-
const ignored = standardWatcher?.options.ignored;
158-
159-
expect(ignored).toBeTypeOf('function');
160-
expect((ignored as (path: string) => boolean)('/usr/local/emhttp/state')).toBe(false);
161-
expect((ignored as (path: string) => boolean)('/usr/local/emhttp/state/README.txt')).toBe(true);
162-
expect((ignored as (path: string) => boolean)('/usr/local/emhttp/state/devs.ini')).toBe(false);
163-
expect((ignored as (path: string) => boolean)('/usr/local/emhttp/state/var.ini.new')).toBe(
164-
false
165-
);
166-
expect((ignored as (path: string) => boolean)('/usr/local/emhttp/state/disks.ini')).toBe(true);
167-
expect((ignored as (path: string) => boolean)('/usr/local/emhttp/state/disks.ini.new')).toBe(
168-
true
169-
);
170-
});
171-
172160
it('reloads registration key when var.ini is replaced after boot', async () => {
173161
const { StateManager } = await import('@app/store/watch/state-watch.js');
174162
const { store } = await import('@app/store/index.js');
@@ -178,40 +166,19 @@ describe('StateManager', () => {
178166
await StateManager.getInstance().ready;
179167
vi.mocked(store.dispatch).mockClear();
180168

181-
const standardWatcher = watchRegistrations.find(
182-
(registration) => registration.options.usePolling === false
169+
const watcher = watchRegistrations.find(
170+
(registration) => registration.path === '/usr/local/emhttp/state/var.ini'
183171
);
184-
const addHandler = standardWatcher?.handlers.add;
185-
expect(addHandler).toBeDefined();
186-
187-
await addHandler?.('/usr/local/emhttp/state/var.ini');
188-
189-
expect(store.dispatch).toHaveBeenNthCalledWith(1, loadSingleStateFile(StateFileKey.var));
190-
expect(store.dispatch).toHaveBeenNthCalledWith(2, loadRegistrationKey());
191-
});
192-
193-
it('reloads registration key when var.ini.new is observed after boot', async () => {
194-
const { StateManager } = await import('@app/store/watch/state-watch.js');
195-
const { store } = await import('@app/store/index.js');
196-
const { loadSingleStateFile } = await import('@app/store/modules/emhttp.js');
197-
const { loadRegistrationKey } = await import('@app/store/modules/registration.js');
198-
199-
await StateManager.getInstance().ready;
200-
vi.mocked(store.dispatch).mockClear();
201-
202-
const standardWatcher = watchRegistrations.find(
203-
(registration) => registration.options.usePolling === false
204-
);
205-
const addHandler = standardWatcher?.handlers.add;
206-
expect(addHandler).toBeDefined();
172+
const changeHandler = watcher?.handlers.change;
173+
expect(changeHandler).toBeDefined();
207174

208-
await addHandler?.('/usr/local/emhttp/state/var.ini.new');
175+
await changeHandler?.('/usr/local/emhttp/state/var.ini');
209176

210177
expect(store.dispatch).toHaveBeenNthCalledWith(1, loadSingleStateFile(StateFileKey.var));
211178
expect(store.dispatch).toHaveBeenNthCalledWith(2, loadRegistrationKey());
212179
});
213180

214-
it('routes polled state files through the polling directory watcher', async () => {
181+
it('routes polled state files through their canonical file watchers', async () => {
215182
const { StateManager } = await import('@app/store/watch/state-watch.js');
216183
const { store } = await import('@app/store/index.js');
217184
const { loadSingleStateFile } = await import('@app/store/modules/emhttp.js');

‎api/src/store/watch/state-watch.ts‎

Lines changed: 15 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -12,45 +12,19 @@ import { StateFileKey } from '@app/store/types.js';
1212

1313
const POLLED_STATE_KEYS = [StateFileKey.disks, StateFileKey.shares] as const;
1414
const POLLED_STATE_KEY_SET = new Set<StateFileKey>(POLLED_STATE_KEYS);
15-
const STATE_FILE_NAMES = new Set(Object.values(StateFileKey).map((key) => `${key}.ini`));
15+
const ATOMIC_REPLACEMENT_WINDOW_MS = 200;
1616

17-
const getNormalizedStateFileName = (path: string): string | null => {
18-
const parsed = parse(path);
19-
if (STATE_FILE_NAMES.has(parsed.base)) {
20-
return parsed.base;
21-
}
22-
23-
if (!parsed.base.endsWith('.new')) {
24-
return null;
25-
}
26-
27-
const originalStateFileName = parsed.base.slice(0, -'.new'.length);
28-
return STATE_FILE_NAMES.has(originalStateFileName) ? originalStateFileName : null;
29-
};
30-
31-
const shouldIgnoreStatePath = (path: string): boolean => {
32-
const normalizedStateFileName = getNormalizedStateFileName(path);
33-
34-
if (!normalizedStateFileName) {
35-
return true;
36-
}
37-
38-
const stateFileKey = StateFileKey[normalizedStateFileName.slice(0, -'.ini'.length)];
39-
return POLLED_STATE_KEY_SET.has(stateFileKey);
40-
};
41-
42-
const chokidarOptionsForStateDirectory = (statesPath: string): ChokidarOptions => ({
17+
const chokidarOptionsForStateKey = (key: StateFileKey): ChokidarOptions => ({
18+
atomic: ATOMIC_REPLACEMENT_WINDOW_MS,
4319
ignoreInitial: true,
44-
ignored: (path, stats) => {
45-
if (path === statesPath) {
46-
return false;
47-
}
48-
if (stats?.isDirectory()) {
49-
return false;
50-
}
51-
return shouldIgnoreStatePath(path);
52-
},
53-
usePolling: CHOKIDAR_USEPOLLING,
20+
...(POLLED_STATE_KEY_SET.has(key)
21+
? {
22+
usePolling: true,
23+
interval: 10_000,
24+
}
25+
: {
26+
usePolling: CHOKIDAR_USEPOLLING,
27+
}),
5428
});
5529

5630
export class StateManager {
@@ -80,12 +54,8 @@ export class StateManager {
8054
}
8155

8256
private getStateFileKeyFromPath(path: string): StateFileKey | undefined {
83-
const normalizedStateFileName = getNormalizedStateFileName(path);
84-
if (!normalizedStateFileName) {
85-
return undefined;
86-
}
87-
88-
return StateFileKey[normalizedStateFileName.slice(0, -'.ini'.length)];
57+
const parsed = parse(path);
58+
return StateFileKey[parsed.name];
8959
}
9060

9161
private async reloadStateFile(stateFile: StateFileKey, reason: 'add' | 'change' | 'startup-sync') {
@@ -124,20 +94,10 @@ export class StateManager {
12494
private readonly setupChokidarWatchForState = async () => {
12595
const { states } = getters.paths();
12696

127-
emhttpLogger.debug('Setting up watch for path: %s', states);
128-
const directoryWatch = watch(states, chokidarOptionsForStateDirectory(states));
129-
directoryWatch.on('add', async (path) => this.handleStateFileUpdate(path, 'add'));
130-
directoryWatch.on('change', async (path) => this.handleStateFileUpdate(path, 'change'));
131-
this.fileWatchers.push(directoryWatch);
132-
133-
for (const key of POLLED_STATE_KEYS) {
97+
for (const key of Object.values(StateFileKey)) {
13498
const pathToWatch = join(states, `${key}.ini`);
13599
emhttpLogger.debug('Setting up watch for path: %s', pathToWatch);
136-
const stateWatch = watch(pathToWatch, {
137-
ignoreInitial: true,
138-
usePolling: true,
139-
interval: 10_000,
140-
});
100+
const stateWatch = watch(pathToWatch, chokidarOptionsForStateKey(key));
141101
stateWatch.on('add', async (path) => this.handleStateFileUpdate(path, 'add'));
142102
stateWatch.on('change', async (path) => this.handleStateFileUpdate(path, 'change'));
143103
this.fileWatchers.push(stateWatch);

0 commit comments

Comments
 (0)