Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -33,27 +33,30 @@ describe('DiskSensorsService', () => {
});

describe('isAvailable', () => {
it('should return true when disks exist', async () => {
it('should return true without checking disks', async () => {
vi.mocked(disksService.getDisks).mockResolvedValue([
{ id: 'disk1', device: '/dev/sda', name: 'Test Disk' } as unknown as Disk,
]);

const available = await service.isAvailable();
expect(available).toBe(true);
expect(disksService.getDisks).not.toHaveBeenCalled();
});

it('should return false when no disks exist', async () => {
it('should return true when no disks exist', async () => {
vi.mocked(disksService.getDisks).mockResolvedValue([]);

const available = await service.isAvailable();
expect(available).toBe(false);
expect(available).toBe(true);
expect(disksService.getDisks).not.toHaveBeenCalled();
});

it('should return false when DisksService throws', async () => {
it('should return true when DisksService would throw', async () => {
vi.mocked(disksService.getDisks).mockRejectedValue(new Error('Failed'));

const available = await service.isAvailable();
expect(available).toBe(false);
expect(available).toBe(true);
expect(disksService.getDisks).not.toHaveBeenCalled();
});
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,7 @@ export class DiskSensorsService implements TemperatureSensorProvider {
constructor(private readonly disksService: DisksService) {}

async isAvailable(): Promise<boolean> {
// Disks are always "available" since DisksService exists
try {
const disks = await this.disksService.getDisks();
return disks.length > 0;
} catch {
return false;
}
return true;
}

async read(): Promise<RawTemperatureSensor[]> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { ConfigService } from '@nestjs/config';

import { beforeEach, describe, expect, it, vi } from 'vitest';

import type { AppReadyEvent } from '@app/unraid-api/app/app-lifecycle.events.js';
import { DiskSensorsService } from '@app/unraid-api/graph/resolvers/metrics/temperature/sensors/disk_sensors.service.js';
import { IpmiSensorsService } from '@app/unraid-api/graph/resolvers/metrics/temperature/sensors/ipmi_sensors.service.js';
import { LmSensorsService } from '@app/unraid-api/graph/resolvers/metrics/temperature/sensors/lm_sensors.service.js';
Expand Down Expand Up @@ -80,7 +81,7 @@ describe('TemperatureService', () => {

describe('initialization', () => {
it('should initialize available providers', async () => {
await service.onModuleInit();
await service.initializeProviders();

expect(lmSensors.isAvailable).toHaveBeenCalled();
expect(diskSensors.isAvailable).toHaveBeenCalled();
Expand All @@ -89,17 +90,43 @@ describe('TemperatureService', () => {
it('should handle provider initialization errors gracefully', async () => {
vi.mocked(lmSensors.isAvailable!).mockRejectedValue(new Error('Failed'));

await service.onModuleInit();
await service.initializeProviders();

// Should not throw
const metrics = await service.getMetrics();
expect(metrics).toBeDefined();
});

it('should initialize providers when the app ready event is emitted', async () => {
const event: AppReadyEvent = {
reason: 'nestjs-server-listening',
};

await service.handleAppReady(event);

expect(lmSensors.isAvailable).toHaveBeenCalled();
expect(diskSensors.isAvailable).toHaveBeenCalled();
});

it('should warn when initialization fails after the app is ready', async () => {
const event: AppReadyEvent = {
reason: 'nestjs-server-listening',
};
const warnSpy = vi.spyOn(service['logger'], 'warn');
vi.spyOn(service, 'initializeProviders').mockRejectedValue(new Error('disk scan failed'));

await service.handleAppReady(event);

expect(warnSpy).toHaveBeenCalledWith(
'Temperature provider initialization after startup failed',
expect.any(Error)
);
});
});

describe('getMetrics', () => {
beforeEach(async () => {
await service.onModuleInit();
await service.initializeProviders();
});

it('should return temperature metrics', async () => {
Expand All @@ -123,7 +150,7 @@ describe('TemperatureService', () => {
history,
temperatureConfigService
);
await emptyService.onModuleInit();
await emptyService.initializeProviders();

const metrics = await emptyService.getMetrics();
expect(metrics).toBeNull();
Expand Down Expand Up @@ -155,7 +182,7 @@ describe('TemperatureService', () => {
thresholds: { cpu_warning: 60, cpu_critical: 80 },
});

await service.onModuleInit();
await service.initializeProviders();

vi.mocked(lmSensors.read!).mockResolvedValue([
{
Expand All @@ -182,7 +209,7 @@ describe('TemperatureService', () => {
thresholds: {},
});

await service.onModuleInit();
await service.initializeProviders();

vi.mocked(lmSensors.read!).mockResolvedValue([
{
Expand Down Expand Up @@ -210,7 +237,7 @@ describe('TemperatureService', () => {
thresholds: {},
});

await service.onModuleInit();
await service.initializeProviders();

vi.mocked(lmSensors.read!).mockResolvedValue([
{
Expand Down Expand Up @@ -239,7 +266,7 @@ describe('TemperatureService', () => {
thresholds: {},
});

await service.onModuleInit();
await service.initializeProviders();

vi.mocked(lmSensors.read!).mockResolvedValue([
{
Expand Down Expand Up @@ -269,7 +296,7 @@ describe('TemperatureService', () => {
thresholds: { cpu_warning: 160 },
});

await service.onModuleInit();
await service.initializeProviders();

vi.mocked(lmSensors.read!).mockResolvedValue([
{
Expand All @@ -294,7 +321,7 @@ describe('TemperatureService', () => {

describe('buildSummary', () => {
it('should calculate correct average', async () => {
await service.onModuleInit();
await service.initializeProviders();
vi.mocked(lmSensors.read!).mockResolvedValue([
{
id: 'sensor1',
Expand All @@ -317,7 +344,7 @@ describe('TemperatureService', () => {
});

it('should identify hottest and coolest sensors', async () => {
await service.onModuleInit();
await service.initializeProviders();
vi.mocked(lmSensors.read!).mockResolvedValue([
{
id: 's1',
Expand Down Expand Up @@ -349,7 +376,7 @@ describe('TemperatureService', () => {
});
describe('edge cases', () => {
it('should handle provider read timeout gracefully', async () => {
await service.onModuleInit();
await service.initializeProviders();

// Simulate a slow/hanging provider
vi.mocked(lmSensors.read!).mockImplementation(
Expand All @@ -362,7 +389,7 @@ describe('TemperatureService', () => {
}, 10000);

it('should deduplicate sensors with same ID from different providers', async () => {
await service.onModuleInit();
await service.initializeProviders();

// Both providers return a sensor with the same ID
vi.mocked(lmSensors.read!).mockResolvedValue([
Expand Down Expand Up @@ -391,7 +418,7 @@ describe('TemperatureService', () => {
});

it('should handle empty sensor name', async () => {
await service.onModuleInit();
await service.initializeProviders();

vi.mocked(lmSensors.read!).mockResolvedValue([
{
Expand All @@ -409,7 +436,7 @@ describe('TemperatureService', () => {
});

it('should handle negative temperature values', async () => {
await service.onModuleInit();
await service.initializeProviders();

vi.mocked(lmSensors.read!).mockResolvedValue([
{
Expand All @@ -428,7 +455,7 @@ describe('TemperatureService', () => {
});

it('should handle extremely high temperature values', async () => {
await service.onModuleInit();
await service.initializeProviders();

vi.mocked(lmSensors.read!).mockResolvedValue([
{
Expand All @@ -447,7 +474,7 @@ describe('TemperatureService', () => {
});

it('should handle NaN temperature values', async () => {
await service.onModuleInit();
await service.initializeProviders();

vi.mocked(lmSensors.read!).mockResolvedValue([
{
Expand All @@ -465,7 +492,7 @@ describe('TemperatureService', () => {
});

it('should handle mix of valid and NaN temperature values', async () => {
await service.onModuleInit();
await service.initializeProviders();

vi.mocked(lmSensors.read!).mockResolvedValue([
{
Expand Down Expand Up @@ -493,7 +520,7 @@ describe('TemperatureService', () => {
});

it('should handle all providers failing', async () => {
await service.onModuleInit();
await service.initializeProviders();

vi.mocked(lmSensors.read!).mockRejectedValue(new Error('lm-sensors failed'));
vi.mocked(diskSensors.read!).mockRejectedValue(new Error('disk sensors failed'));
Expand All @@ -504,7 +531,7 @@ describe('TemperatureService', () => {
});

it('should handle partial provider failures', async () => {
await service.onModuleInit();
await service.initializeProviders();

vi.mocked(lmSensors.read!).mockRejectedValue(new Error('lm-sensors failed'));
vi.mocked(diskSensors.read!).mockResolvedValue([
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
import { Injectable, Logger, OnModuleInit } from '@nestjs/common';
import { Injectable, Logger } from '@nestjs/common';
import { OnEvent } from '@nestjs/event-emitter';

import type { AppReadyEvent } from '@app/unraid-api/app/app-lifecycle.events.js';
import { APP_READY_EVENT } from '@app/unraid-api/app/app-lifecycle.events.js';
import { DiskSensorsService } from '@app/unraid-api/graph/resolvers/metrics/temperature/sensors/disk_sensors.service.js';
import { IpmiSensorsService } from '@app/unraid-api/graph/resolvers/metrics/temperature/sensors/ipmi_sensors.service.js';
import { LmSensorsService } from '@app/unraid-api/graph/resolvers/metrics/temperature/sensors/lm_sensors.service.js';
Expand All @@ -21,9 +24,11 @@ import {

// temperature.service.ts
@Injectable()
export class TemperatureService implements OnModuleInit {
export class TemperatureService {
private readonly logger = new Logger(TemperatureService.name);
private availableProviders: TemperatureSensorProvider[] = [];
private initializationPromise: Promise<void> | null = null;
private initialized = false;

private cache: TemperatureMetrics | null = null;
private cacheTimestamp = 0;
Expand All @@ -40,17 +45,38 @@ export class TemperatureService implements OnModuleInit {
private readonly configService: TemperatureConfigService
) {}

async onModuleInit() {
// Initialize all providers and check availability
await this.initializeProviders();
async initializeProviders(): Promise<void> {
if (this.initialized) {
return;
}

if (this.initializationPromise) {
return this.initializationPromise;
}

this.initializationPromise = this.loadAvailableProviders().finally(() => {
this.initializationPromise = null;
});

return this.initializationPromise;
}

private async initializeProviders(): Promise<void> {
@OnEvent(APP_READY_EVENT, { async: true })
async handleAppReady(_event: AppReadyEvent): Promise<void> {
try {
await this.initializeProviders();
} catch (error: unknown) {
this.logger.warn('Temperature provider initialization after startup failed', error);
}
}

private async loadAvailableProviders(): Promise<void> {
// 1. Get sensor specific configs
const config = this.configService.getConfig(false);
const lmSensorsConfig = config?.sensors?.lm_sensors;
const smartctlConfig = config?.sensors?.smartctl;
const ipmiConfig = config?.sensors?.ipmi;
const availableProviders: TemperatureSensorProvider[] = [];

// 2. Define providers with their config checks
// We default to TRUE if the config is missing
Expand Down Expand Up @@ -79,7 +105,7 @@ export class TemperatureService implements OnModuleInit {

try {
if (await provider.service.isAvailable()) {
this.availableProviders.push(provider.service);
availableProviders.push(provider.service);
this.logger.log(`Temperature provider available: ${provider.service.id}`);
} else {
this.logger.debug(`Temperature provider not available: ${provider.service.id}`);
Expand All @@ -89,12 +115,17 @@ export class TemperatureService implements OnModuleInit {
}
}

this.availableProviders = availableProviders;
this.initialized = true;

if (this.availableProviders.length === 0) {
this.logger.warn('No temperature providers available');
}
}

async getMetrics(): Promise<TemperatureMetrics | null> {
await this.initializeProviders();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Handle initialization failures inside getMetrics.

At Line 127, initializeProviders() is awaited outside the method’s error handling block. If initialization rejects (e.g., unexpected config/runtime error), this can bubble and fail the request instead of returning null gracefully like the rest of getMetrics.

Suggested fix
 async getMetrics(): Promise<TemperatureMetrics | null> {
-    await this.initializeProviders();
+    try {
+        await this.initializeProviders();
+    } catch (err) {
+        this.logger.error('Failed to initialize temperature providers', err);
+        return null;
+    }
 
     // Check if we can use recent history instead of re-reading sensors
     const mostRecent = this.history.getMostRecentReading();
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
await this.initializeProviders();
try {
await this.initializeProviders();
} catch (err) {
this.logger.error('Failed to initialize temperature providers', err);
return null;
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@api/src/unraid-api/graph/resolvers/metrics/temperature/temperature.service.ts`
at line 127, In getMetrics, initializeProviders() is awaited outside the
existing error handling, so if initializeProviders rejects the rejection will
bubble instead of returning null like other failures; wrap the await
this.initializeProviders() call inside the getMetrics try/catch (or add a
dedicated try/catch around it) so any thrown error is caught, logged (use the
same logger used elsewhere in this service), and getMetrics returns null on
failure; reference the getMetrics method and the initializeProviders() method
when making the change.


// Check if we can use recent history instead of re-reading sensors
const mostRecent = this.history.getMostRecentReading();
const canUseHistory =
Expand Down
Loading
Loading