Skip to content

Commit 3c84326

Browse files
authored
fix(api): keep plugin config loads side-effect free (#2025)
## Summary Makes API config loading side-effect free for CLI plugin management commands, so plugin install operations preserve the plugin list they are updating. ## Root Cause `loadApiConfig()` always ran the stale Connect cleanup when `/boot/config/plugins/dynamix.unraid.net.plg` was not visible. CLI plugin commands also load API config, so an install command could remove `unraid-api-plugin-connect` while trying to add it. During Unraid plugin-manager installation, the Connect `.plg` may not be committed to `/boot/config/plugins` yet. That allowed the API package to install and restart successfully while `api.json` still contained `"plugins": []`, so the Connect Nest module never loaded. ## Change - Gates the stale Connect cleanup on `environment.IS_MAIN_PROCESS`. - Keeps CLI config loading side-effect free for plugin management operations. - Preserves the existing runtime cleanup behavior when the main API process starts. - Adds tests for both CLI-style config loading and main-process cleanup. - Builds `@unraid/shared` before API type-check so workspace package-exported types are current in CI. ## Validation - `pnpm -C api run type-check` - `pnpm -C api exec vitest run src/unraid-api/config/api-config.test.ts` - `pnpm --filter @unraid/connect-plugin test` - Prior to broadening this fix, installed a patched `.plg` on `root@unraid.local` and verified the expected runtime outcome: - install output included `Added bundled plugin unraid-api-plugin-connect` - `api.json` contained `"plugins": ["unraid-api-plugin-connect"]` - `unraid-api plugins list` reported `unraid-api-plugin-connect@file:packages/unraid-api-plugin-connect-4.25.3.tgz` - `/var/log/graphql-api.log` showed `ConnectPluginModule` initialized and Mothership connected Note: an accidental full `pnpm --filter ./api test -- api/src/unraid-api/config/api-config.test.ts` run executed the broader API suite and surfaced an existing unrelated metrics spec failure in `metrics.resolver.spec.ts`; the focused config test passes.
1 parent 264ddf0 commit 3c84326

3 files changed

Lines changed: 79 additions & 5 deletions

File tree

api/package.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
"// Code Quality": "",
3636
"lint": "eslint --config .eslintrc.ts src/",
3737
"lint:fix": "eslint --fix --config .eslintrc.ts src/",
38+
"pretype-check": "pnpm --filter @unraid/shared build",
3839
"type-check": "tsc --noEmit",
3940
"// Testing": "",
4041
"test": "NODE_ENV=test vitest run",

api/src/unraid-api/config/api-config.module.ts

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import { ConfigFilePersister } from '@unraid/shared/services/config-file.js';
77
import { csvStringToArray } from '@unraid/shared/util/data.js';
88

99
import { isConnectPluginInstalled } from '@app/connect-plugin-cleanup.js';
10-
import { API_VERSION, PATHS_CONFIG_MODULES } from '@app/environment.js';
10+
import { API_VERSION, environment, PATHS_CONFIG_MODULES } from '@app/environment.js';
1111
import { OnboardingTrackerModule } from '@app/unraid-api/config/onboarding-tracker.module.js';
1212

1313
export { type ApiConfig };
@@ -31,8 +31,9 @@ export const loadApiConfig = async () => {
3131
const apiHandler = new ApiConfigPersistence(new ConfigService()).getFileHandler();
3232

3333
const diskConfig: Partial<ApiConfig> = await apiHandler.loadConfig();
34-
// Hack: cleanup stale connect plugin entry if necessary
35-
if (!isConnectPluginInstalled()) {
34+
// Hack: cleanup stale connect plugin entry if necessary.
35+
// Keep config loading side-effect free for CLI plugin management commands.
36+
if (environment.IS_MAIN_PROCESS && !isConnectPluginInstalled()) {
3637
diskConfig.plugins = diskConfig.plugins?.filter(
3738
(plugin) => plugin !== 'unraid-api-plugin-connect'
3839
);

api/src/unraid-api/config/api-config.test.ts

Lines changed: 74 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,15 +3,22 @@ import { readFile } from 'fs/promises';
33
import path from 'path';
44

55
import type { ApiConfig } from '@unraid/shared/services/api-config.js';
6-
import { writeFile as atomicWriteFile } from 'atomically';
6+
import { fileExists as sharedFileExists } from '@unraid/shared/util/file.js';
7+
import { readFile as atomicReadFile, writeFile as atomicWriteFile } from 'atomically';
78
import { Subject } from 'rxjs';
89
import { beforeEach, describe, expect, it, vi } from 'vitest';
910

10-
import { API_VERSION, PATHS_CONFIG_MODULES } from '@app/environment.js';
11+
import { API_VERSION, environment, PATHS_CONFIG_MODULES } from '@app/environment.js';
1112
import { ApiConfigPersistence, loadApiConfig } from '@app/unraid-api/config/api-config.module.js';
1213
import { OnboardingOverrideService } from '@app/unraid-api/config/onboarding-override.service.js';
1314
import { OnboardingTrackerService } from '@app/unraid-api/config/onboarding-tracker.module.js';
1415

16+
const connectPluginCleanupMocks = vi.hoisted(() => ({
17+
isConnectPluginInstalled: vi.fn(),
18+
}));
19+
20+
vi.mock('@app/connect-plugin-cleanup.js', () => connectPluginCleanupMocks);
21+
1522
vi.mock('@app/core/utils/files/file-exists.js', () => ({
1623
fileExists: vi.fn(),
1724
}));
@@ -39,10 +46,13 @@ vi.mock('@app/store/index.js', () => ({
3946
}));
4047

4148
vi.mock('atomically', () => ({
49+
readFile: vi.fn(),
4250
writeFile: vi.fn(),
4351
}));
4452

4553
const mockReadFile = vi.mocked(readFile);
54+
const mockSharedFileExists = vi.mocked(sharedFileExists);
55+
const mockAtomicReadFile = vi.mocked(atomicReadFile);
4656
const mockAtomicWriteFile = vi.mocked(atomicWriteFile);
4757

4858
const createOnboardingTracker = (configService: ConfigService) => {
@@ -296,6 +306,10 @@ describe('OnboardingTracker', () => {
296306
describe('loadApiConfig', () => {
297307
beforeEach(() => {
298308
vi.clearAllMocks();
309+
environment.IS_MAIN_PROCESS = false;
310+
mockSharedFileExists.mockResolvedValue(false);
311+
mockAtomicReadFile.mockRejectedValue(new Error('Not found'));
312+
connectPluginCleanupMocks.isConnectPluginInstalled.mockReturnValue(false);
299313
});
300314

301315
it('should return default config with current API_VERSION', async () => {
@@ -321,4 +335,62 @@ describe('loadApiConfig', () => {
321335
plugins: [],
322336
});
323337
});
338+
339+
it('does not clean stale Connect plugin entries outside the main API process', async () => {
340+
mockSharedFileExists.mockResolvedValue(true);
341+
mockAtomicReadFile.mockResolvedValue(
342+
Buffer.from(
343+
JSON.stringify({
344+
version: API_VERSION,
345+
extraOrigins: [],
346+
sandbox: true,
347+
ssoSubIds: [],
348+
plugins: ['unraid-api-plugin-connect'],
349+
})
350+
)
351+
);
352+
353+
const result = await loadApiConfig();
354+
355+
expect(connectPluginCleanupMocks.isConnectPluginInstalled).not.toHaveBeenCalled();
356+
expect(mockAtomicWriteFile).not.toHaveBeenCalled();
357+
expect(result).toEqual({
358+
version: API_VERSION,
359+
extraOrigins: [],
360+
sandbox: true,
361+
ssoSubIds: [],
362+
plugins: ['unraid-api-plugin-connect'],
363+
});
364+
});
365+
366+
it('cleans stale Connect plugin entries in the main API process', async () => {
367+
environment.IS_MAIN_PROCESS = true;
368+
mockSharedFileExists.mockResolvedValue(true);
369+
mockAtomicReadFile.mockResolvedValue(
370+
Buffer.from(
371+
JSON.stringify({
372+
version: API_VERSION,
373+
extraOrigins: [],
374+
sandbox: true,
375+
ssoSubIds: [],
376+
plugins: ['unraid-api-plugin-connect', 'other-plugin'],
377+
})
378+
)
379+
);
380+
381+
const result = await loadApiConfig();
382+
383+
expect(connectPluginCleanupMocks.isConnectPluginInstalled).toHaveBeenCalledOnce();
384+
expect(mockAtomicWriteFile).toHaveBeenCalledWith(
385+
path.join(PATHS_CONFIG_MODULES, 'api.json'),
386+
expect.not.stringContaining('unraid-api-plugin-connect')
387+
);
388+
expect(result).toEqual({
389+
version: API_VERSION,
390+
extraOrigins: [],
391+
sandbox: true,
392+
ssoSubIds: [],
393+
plugins: ['other-plugin'],
394+
});
395+
});
324396
});

0 commit comments

Comments
 (0)