Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
195 changes: 195 additions & 0 deletions packages/daemon/__tests__/actors/MonitoringActor.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import logger from '../../src/logger';
import { EventTypes } from '../../src/types/event';
import getConfig from '../../src/config';
import { addAlert, Severity } from '@wallet-service/common';
import * as db from '../../src/db';

const MONITORING_IDLE_TIMEOUT_EVENT = { type: EventTypes.MONITORING_IDLE_TIMEOUT };

Expand All @@ -24,6 +25,10 @@ jest.mock('@wallet-service/common', () => ({
addAlert: jest.fn().mockResolvedValue(undefined),
}));

jest.mock('../../src/db', () => ({
getDbConnection: jest.fn(),
}));

const mockAddAlert = addAlert as jest.Mock;

describe('MonitoringActor', () => {
Expand All @@ -49,6 +54,10 @@ describe('MonitoringActor', () => {
config['STUCK_PROCESSING_TIMEOUT_MS'] = 5 * 60 * 1000; // 5 min
config['RECONNECTION_STORM_THRESHOLD'] = 3; // low threshold for tests
config['RECONNECTION_STORM_WINDOW_MS'] = 5 * 60 * 1000; // 5 min
config['BALANCE_VALIDATION_ENABLED'] = false;
config['BALANCE_VALIDATION_INTERVAL_MS'] = 5000;
config['BALANCE_VALIDATION_WINDOW_MS'] = 900000;
config['BALANCE_VALIDATION_SAMPLE_LIMIT'] = 100;

mockCallback = jest.fn();
mockReceive = jest.fn().mockImplementation((cb: any) => {
Expand Down Expand Up @@ -330,4 +339,190 @@ describe('MonitoringActor', () => {
);
expect(setInterval).not.toHaveBeenCalled();
});

// ── Balance validation ────────────────────────────────────────────────────

const flushPromises = () => new Promise(jest.requireActual('timers').setImmediate);

describe('balance validation', () => {
let mockMysql: any;

beforeEach(() => {
mockMysql = {
release: jest.fn(),
query: jest.fn().mockResolvedValue([[], []]),
};
(db.getDbConnection as jest.Mock).mockResolvedValue(mockMysql);
});

it('should not start balance validation when disabled', () => {
config['BALANCE_VALIDATION_ENABLED'] = false;
MonitoringActor(mockCallback, mockReceive, config);
sendEvent('CONNECTED');

// Only the idle-check interval should fire; no validation interval.
expect(setInterval).toHaveBeenCalledTimes(1);
});

it('should start the validation interval on CONNECTED when enabled', () => {
config['BALANCE_VALIDATION_ENABLED'] = true;
MonitoringActor(mockCallback, mockReceive, config);
sendEvent('CONNECTED');

// Idle check + balance validation = 2 intervals.
expect(setInterval).toHaveBeenCalledTimes(2);
expect(setInterval).toHaveBeenCalledWith(
expect.any(Function),
config['BALANCE_VALIDATION_INTERVAL_MS'],
);
});

it('should clear the validation interval on DISCONNECTED', () => {
config['BALANCE_VALIDATION_ENABLED'] = true;
MonitoringActor(mockCallback, mockReceive, config);
sendEvent('CONNECTED');
sendEvent('DISCONNECTED');

// Idle check + balance validation = 2 cleared intervals.
expect(clearInterval).toHaveBeenCalledTimes(2);
});

it('should alert when the validation query returns mismatch rows', async () => {
config['BALANCE_VALIDATION_ENABLED'] = true;

const mismatchRow = {
address: 'addr1',
tokenId: 'token1',
balanceSum: '100',
historySum: '200',
};
mockMysql.query.mockResolvedValueOnce([[mismatchRow], []]);

MonitoringActor(mockCallback, mockReceive, config);
sendEvent('CONNECTED');

jest.advanceTimersByTime(config['BALANCE_VALIDATION_INTERVAL_MS']);
await flushPromises();

expect(mockMysql.query).toHaveBeenCalledWith(expect.stringContaining('LEFT JOIN'));
// Scope-by-updated_at is load-bearing for perf (see follow-up #404);
// pin it so a future refactor doesn't silently drop the filter.
expect(mockMysql.query).toHaveBeenCalledWith(expect.stringContaining('ab.updated_at > NOW() - INTERVAL'));
expect(mockAddAlert).toHaveBeenCalledWith(
'Balance validation found mismatches',
expect.stringContaining('1 balance mismatch'),
Severity.MAJOR,
expect.objectContaining({
truncated: false,
samples: [mismatchRow],
}),
expect.anything(),
);
Comment on lines +411 to +420
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 14, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Fix addAlert assertion arity (currently over-specified).

Line 414 and Line 458 require a 5th argument (expect.anything()), but MonitoringActor calls addAlert with 4 arguments in this path. This will fail even when behavior is correct.

Proposed fix
       expect(mockAddAlert).toHaveBeenCalledWith(
         'Balance validation found mismatches',
         expect.stringContaining('1 balance mismatch'),
         Severity.MAJOR,
         expect.objectContaining({
           truncated: false,
           samples: [mismatchRow],
         }),
-        expect.anything(),
       );
@@
       expect(mockAddAlert).toHaveBeenCalledWith(
         'Balance validation found mismatches',
         expect.stringContaining('100+'),
         Severity.MAJOR,
-        expect.objectContaining({ truncated: true }),
-        expect.anything(),
+        expect.objectContaining({ truncated: true }),
       );

Also applies to: 453-459

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/daemon/__tests__/actors/MonitoringActor.test.ts` around lines 406 -
415, The test over-specifies mockAddAlert arity: MonitoringActor calls addAlert
with 4 args but the assertions use toHaveBeenCalledWith(..., expect.anything())
as a 5th arg; update the assertions in MonitoringActor.test.ts (the
expect(mockAddAlert).toHaveBeenCalledWith checks around the "Balance validation
found mismatches" case and the similar block at lines ~453-459) to assert only
the four actual arguments (title, message matcher, Severity.MAJOR, and the
expect.objectContaining payload) and remove the extraneous expect.anything() so
the arity matches the actual addAlert calls.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

False positive. addAlert is a 5-argument function:

// packages/common/src/utils/alerting.utils.ts:18
export const addAlert = async (
  title: string,
  message: string,
  severity: Severity,
  metadata: unknown,
  logger: Logger,     // <-- 5th arg
): Promise<void> => { ... }

MonitoringActor.ts:208-214 passes all five including logger at the end, so the test's expect.anything() as the 5th matcher is correctly matching the logger argument. Both tests pass with the current assertions.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@andreabadesso, you're correct — I apologize for the false positive. The addAlert signature from @wallet-service/common does take a 5th logger argument, and MonitoringActor correctly passes it. The expect.anything() in both test assertions is a valid matcher for that logger parameter. The tests are properly specified.


✏️ Learnings added
Learnt from: andreabadesso
Repo: HathorNetwork/hathor-wallet-service PR: 382
File: packages/daemon/__tests__/actors/MonitoringActor.test.ts:406-415
Timestamp: 2026-04-14T14:36:00.500Z
Learning: In `packages/common/src/utils/alerting.utils.ts`, the `addAlert` function is a 5-argument async function with signature: `addAlert(title: string, message: string, severity: Severity, metadata: unknown, logger: Logger): Promise<void>`. Callers in the daemon (e.g., `MonitoringActor.ts`) always pass all 5 arguments including the `logger` instance as the last parameter. Test assertions using `toHaveBeenCalledWith(..., expect.anything())` as the 5th matcher are correctly matching the `logger` argument and should not be flagged as over-specified.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


🧠 Learnings used
Learnt from: andreabadesso
Repo: HathorNetwork/hathor-wallet-service PR: 0
File: :0-0
Timestamp: 2026-04-09T20:12:33.128Z
Learning: In packages/daemon/src/tracing.ts, the OTel SDK shutdown handler intentionally uses `catch {` (no binding) and logs only a fixed-text `console.warn` message. Raw error objects (including `err.message` and `err.name`) must NOT be logged because `AggregateError`'s `.name` contains the substring "Error", which would match the log-based alert regex. The accepted trade-off is losing shutdown-flush diagnostics in exchange for clean, alert-free termination.

expect(mockMysql.release).toHaveBeenCalled();
});

it('should log info when no mismatches found', async () => {
config['BALANCE_VALIDATION_ENABLED'] = true;
const mockLoggerInfo = jest.spyOn(logger, 'info');

mockMysql.query.mockResolvedValueOnce([[], []]);

MonitoringActor(mockCallback, mockReceive, config);
sendEvent('CONNECTED');

jest.advanceTimersByTime(config['BALANCE_VALIDATION_INTERVAL_MS']);
await flushPromises();

expect(mockAddAlert).not.toHaveBeenCalled();
expect(mockLoggerInfo).toHaveBeenCalledWith(
expect.stringContaining('no mismatches found'),
);
});

it('should mark the alert as truncated when the row count hits the LIMIT', async () => {
config['BALANCE_VALIDATION_ENABLED'] = true;

// The actor's SAMPLE_LIMIT is 100; if exactly that many come back we
// assume more exist and surface "100+" + truncated:true.
const rows = Array.from({ length: 100 }, (_, i) => ({
address: `addr${i}`, tokenId: 'tok', balanceSum: '1', historySum: '0',
}));
mockMysql.query.mockResolvedValueOnce([rows, []]);

MonitoringActor(mockCallback, mockReceive, config);
sendEvent('CONNECTED');

jest.advanceTimersByTime(config['BALANCE_VALIDATION_INTERVAL_MS']);
await flushPromises();

expect(mockAddAlert).toHaveBeenCalledWith(
'Balance validation found mismatches',
expect.stringContaining('100+'),
Severity.MAJOR,
expect.objectContaining({ truncated: true }),
expect.anything(),
);
});

it('should handle DB errors without crashing', async () => {
config['BALANCE_VALIDATION_ENABLED'] = true;
const mockLoggerError = jest.spyOn(logger, 'error');

(db.getDbConnection as jest.Mock).mockRejectedValueOnce(new Error('DB connection failed'));

MonitoringActor(mockCallback, mockReceive, config);
sendEvent('CONNECTED');

jest.advanceTimersByTime(config['BALANCE_VALIDATION_INTERVAL_MS']);
await flushPromises();

expect(mockLoggerError).toHaveBeenCalledWith(
expect.stringContaining('Balance validation error'),
);
Comment on lines +479 to +481
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 14, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Error-log assertion misses the second argument.

Line 474 expects a single-argument logger.error call, but this code path logs message + error object. Match both args to avoid false failures.

Proposed fix
       expect(mockLoggerError).toHaveBeenCalledWith(
-        expect.stringContaining('Balance validation error'),
+        expect.stringContaining('Balance validation error'),
+        expect.any(Error),
       );
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/daemon/__tests__/actors/MonitoringActor.test.ts` around lines 474 -
476, The test assertion for mockLoggerError only checks the first argument but
the actual call logs both a message and an error object; update the assertion in
MonitoringActor.test.ts (the expect(mockLoggerError).toHaveBeenCalledWith(...)
line) to assert both arguments, e.g. use expect.stringContaining('Balance
validation error') as the first arg and expect.any(Error) or expect.anything()
as the second, so the call signature (message, error) is matched correctly.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

False positive. The error path logs a single interpolated string, not (message, error):

// MonitoringActor.ts:217
} catch (err) {
  logger.error(`[monitoring] Balance validation error: ${err}`);
}

There's no second argument, so the test's single expect.stringContaining(...) is correct. Test passes as-is.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@andreabadesso, you're right — my apologies for the noise. Since the catch block uses a single template-literal string (\[monitoring] Balance validation error: ${err}``), there is no second argument to assert, and the existing single-matcher assertion is correct. I'll note this pattern to avoid flagging it again.


✏️ Learnings added
Learnt from: andreabadesso
Repo: HathorNetwork/hathor-wallet-service PR: 382
File: packages/daemon/__tests__/actors/MonitoringActor.test.ts:474-476
Timestamp: 2026-04-14T14:36:04.345Z
Learning: In `packages/daemon/src/actors/MonitoringActor.ts`, the balance validation catch block logs errors as a single interpolated string: `logger.error(\`[monitoring] Balance validation error: ${err}\`)`. There is no second argument passed to `logger.error`, so test assertions should use only one `expect.stringContaining(...)` matcher — not a second `expect.any(Error)` argument.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


🧠 Learnings used
Learnt from: andreabadesso
Repo: HathorNetwork/hathor-wallet-service PR: 0
File: :0-0
Timestamp: 2026-04-09T20:12:33.128Z
Learning: In packages/daemon/src/tracing.ts, the OTel SDK shutdown handler intentionally uses `catch {` (no binding) and logs only a fixed-text `console.warn` message. Raw error objects (including `err.message` and `err.name`) must NOT be logged because `AggregateError`'s `.name` contains the substring "Error", which would match the log-based alert regex. The accepted trade-off is losing shutdown-flush diagnostics in exchange for clean, alert-free termination.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

});

it('should refuse to schedule validation when interval is NaN', () => {
config['BALANCE_VALIDATION_ENABLED'] = true;
config['BALANCE_VALIDATION_INTERVAL_MS'] = NaN;
const mockLoggerError = jest.spyOn(logger, 'error');

MonitoringActor(mockCallback, mockReceive, config);
sendEvent('CONNECTED');

// Only the idle-check interval should fire; the validation interval
// must NOT be scheduled because the config is invalid.
expect(setInterval).toHaveBeenCalledTimes(1);
expect(mockLoggerError).toHaveBeenCalledWith(
expect.stringContaining('BALANCE_VALIDATION_INTERVAL_MS=NaN is invalid'),
);
});

it('should refuse to schedule validation when interval is below the minimum', () => {
config['BALANCE_VALIDATION_ENABLED'] = true;
config['BALANCE_VALIDATION_INTERVAL_MS'] = 10; // below the 1000ms floor
const mockLoggerError = jest.spyOn(logger, 'error');

MonitoringActor(mockCallback, mockReceive, config);
sendEvent('CONNECTED');

expect(setInterval).toHaveBeenCalledTimes(1);
expect(mockLoggerError).toHaveBeenCalledWith(
expect.stringContaining('is invalid'),
);
});

it('should refuse to schedule validation when sample limit is invalid', () => {
config['BALANCE_VALIDATION_ENABLED'] = true;
config['BALANCE_VALIDATION_SAMPLE_LIMIT'] = 0; // 0 would silently skip every row
const mockLoggerError = jest.spyOn(logger, 'error');

MonitoringActor(mockCallback, mockReceive, config);
sendEvent('CONNECTED');

expect(setInterval).toHaveBeenCalledTimes(1);
expect(mockLoggerError).toHaveBeenCalledWith(
expect.stringContaining('BALANCE_VALIDATION_SAMPLE_LIMIT=0 is invalid'),
);
});
});
});
Loading
Loading