From 1d1fba0e3356fff820182ec00a2e79de66345462 Mon Sep 17 00:00:00 2001 From: SaelixCode Date: Tue, 21 Jul 2026 11:47:56 -0400 Subject: [PATCH] fix: fail closed on corrupt mute schedules and clean invalid replicas Empty or whitespace stored schedules no longer act as all-day mutes. Invalid schedules trigger remote DELETE cleanup, and the weekly-window form gains accessibility attributes plus component coverage. --- .../__tests__/notification-schedule.test.ts | 7 + ...ion-suppression-schedule-migration.test.ts | 43 +++++ .../notification-suppression-sync.test.ts | 57 +++++- backend/src/helpers/notificationSchedule.ts | 19 +- .../helpers/notificationSuppressionSync.ts | 20 ++- .../NotificationSuppressionSection.tsx | 20 ++- .../NotificationSuppressionSection.test.tsx | 166 ++++++++++++++++-- 7 files changed, 303 insertions(+), 29 deletions(-) diff --git a/backend/src/__tests__/notification-schedule.test.ts b/backend/src/__tests__/notification-schedule.test.ts index ea9eaaca..c9b8da67 100644 --- a/backend/src/__tests__/notification-schedule.test.ts +++ b/backend/src/__tests__/notification-schedule.test.ts @@ -103,5 +103,12 @@ describe('scheduleAllowsSuppression / stored parse', () => { expect(scheduleAllowsSuppression(null, true, Date.now())).toBe(false); expect(parseStoredNotificationSchedule('{not-json')).toEqual({ kind: 'invalid' }); expect(parseStoredNotificationSchedule(null)).toEqual({ kind: 'null' }); + expect(parseStoredNotificationSchedule(undefined)).toEqual({ kind: 'null' }); + }); + + it('treats empty and whitespace strings as invalid, not legacy null', () => { + expect(parseStoredNotificationSchedule('')).toEqual({ kind: 'invalid' }); + expect(parseStoredNotificationSchedule(' ')).toEqual({ kind: 'invalid' }); + expect(parseStoredNotificationSchedule('null')).toEqual({ kind: 'invalid' }); }); }); diff --git a/backend/src/__tests__/notification-suppression-schedule-migration.test.ts b/backend/src/__tests__/notification-suppression-schedule-migration.test.ts index 23f80003..dec4d541 100644 --- a/backend/src/__tests__/notification-suppression-schedule-migration.test.ts +++ b/backend/src/__tests__/notification-suppression-schedule-migration.test.ts @@ -7,6 +7,7 @@ import os from 'os'; import path from 'path'; import Database from 'better-sqlite3'; import { DatabaseService } from '../services/DatabaseService'; +import { scheduleAllowsSuppression } from '../helpers/notificationSchedule'; function resetDatabaseSingleton(): void { const holder = DatabaseService as unknown as { instance?: DatabaseService }; @@ -89,4 +90,46 @@ describe('notification suppression schedule column migration', () => { const cols2 = db2.getDb().prepare('PRAGMA table_info(notification_suppression_rules)').all() as Array<{ name: string }>; expect(cols2.filter((c) => c.name === 'schedule')).toHaveLength(1); }); + + it('empty-string schedule column is invalid and does not suppress via enabled load', { timeout: 60_000 }, () => { + scratchDir = fs.mkdtempSync(path.join(os.tmpdir(), 'sencho-supp-sched-empty-')); + const dbPath = path.join(scratchDir, 'sencho.db'); + const seed = new Database(dbPath); + try { + seed.exec(` + CREATE TABLE notification_suppression_rules ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + name TEXT NOT NULL, + node_id INTEGER NULL, + stack_patterns TEXT NOT NULL, + label_ids TEXT NULL, + categories TEXT NULL, + levels TEXT NULL, + applies_to TEXT NOT NULL, + enabled INTEGER DEFAULT 1, + expires_at INTEGER NULL, + schedule TEXT NULL, + created_at INTEGER NOT NULL, + updated_at INTEGER NOT NULL + ); + INSERT INTO notification_suppression_rules + (name, node_id, stack_patterns, label_ids, categories, levels, applies_to, enabled, expires_at, schedule, created_at, updated_at) + VALUES ('Empty schedule mute', NULL, '[]', NULL, '["monitor_alert"]', NULL, 'both', 1, NULL, '', 1, 1); + `); + } finally { + seed.close(); + } + + prevDataDir = process.env.DATA_DIR; + process.env.DATA_DIR = scratchDir; + resetDatabaseSingleton(); + const db = DatabaseService.getInstance(); + + const rule = db.getEnabledNotificationSuppressionRules().find((r) => r.name === 'Empty schedule mute'); + expect(rule).toBeDefined(); + expect(rule!.schedule).toBeNull(); + expect(rule!.scheduleInvalid).toBe(true); + + expect(scheduleAllowsSuppression(rule!.schedule, rule!.scheduleInvalid, Date.now())).toBe(false); + }); }); diff --git a/backend/src/__tests__/notification-suppression-sync.test.ts b/backend/src/__tests__/notification-suppression-sync.test.ts index 6240222b..3a401972 100644 --- a/backend/src/__tests__/notification-suppression-sync.test.ts +++ b/backend/src/__tests__/notification-suppression-sync.test.ts @@ -155,16 +155,67 @@ describe('notificationSuppressionSync', () => { expect(error.mock.calls.some((c) => String(c[0]).includes('cleanup pending'))).toBe(true); }); - it('skips sync when scheduleInvalid (does not POST as unscheduled)', async () => { + it('scheduleInvalid: DELETE success, no POST', async () => { const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const error = vi.spyOn(console, 'error').mockImplementation(() => {}); + mockFetch.mockResolvedValue({ ok: true, status: 200, text: async () => '' }); + syncSuppressionRuleToFleet(makeRule({ node_id: 10, schedule: null, scheduleInvalid: true, })); - await vi.waitFor(() => expect(warn).toHaveBeenCalled()); + await vi.waitFor(() => { + expect(warn.mock.calls.some((c) => String(c[0]).includes('replica removed'))).toBe(true); + }); + + expect(mockFetch.mock.calls.every((c) => (c[1] as { method: string }).method === 'DELETE')).toBe(true); + expect(error.mock.calls.some((c) => String(c[0]).includes('cleanup pending'))).toBe(false); + }); + + it('scheduleInvalid: DELETE 404 counts as cleanup success, no POST', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + mockFetch.mockResolvedValue({ ok: false, status: 404, text: async () => 'gone' }); + + syncSuppressionRuleToFleet(makeRule({ + node_id: 10, + schedule: null, + scheduleInvalid: true, + })); + await vi.waitFor(() => { + expect(warn.mock.calls.some((c) => String(c[0]).includes('replica removed'))).toBe(true); + }); + expect(mockFetch.mock.calls.every((c) => (c[1] as { method: string }).method === 'DELETE')).toBe(true); + }); + + it('scheduleInvalid: DELETE failure logs pending cleanup, no POST', async () => { + const error = vi.spyOn(console, 'error').mockImplementation(() => {}); + mockFetch.mockResolvedValue({ ok: false, status: 503, text: async () => 'down' }); + + syncSuppressionRuleToFleet(makeRule({ + node_id: 10, + schedule: null, + scheduleInvalid: true, + })); + await vi.waitFor(() => expect(error).toHaveBeenCalled()); + expect(mockFetch.mock.calls.every((c) => (c[1] as { method: string }).method === 'DELETE')).toBe(true); + expect(error.mock.calls.some((c) => String(c[0]).includes('cleanup pending'))).toBe(true); + }); + + it('scheduleInvalid: no proxy target logs pending, no POST', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const error = vi.spyOn(console, 'error').mockImplementation(() => {}); + mockGetProxyTarget.mockReturnValue(null); + + syncSuppressionRuleToFleet(makeRule({ + node_id: 10, + schedule: null, + scheduleInvalid: true, + })); + await vi.waitFor(() => expect(error).toHaveBeenCalled()); expect(mockFetch).not.toHaveBeenCalled(); - expect(warn.mock.calls.some((c) => String(c[0]).includes('corrupt schedule'))).toBe(true); + expect(warn.mock.calls.some((c) => String(c[0]).includes('replica removed'))).toBe(false); + expect(error.mock.calls.some((c) => String(c[0]).includes('cleanup pending'))).toBe(true); }); it('unscheduled-to-scheduled on unsupported target attempts DELETE', async () => { diff --git a/backend/src/helpers/notificationSchedule.ts b/backend/src/helpers/notificationSchedule.ts index c506c3b4..5f7465bb 100644 --- a/backend/src/helpers/notificationSchedule.ts +++ b/backend/src/helpers/notificationSchedule.ts @@ -73,22 +73,29 @@ export function parseNotificationSchedule(raw: unknown): ParseNotificationSchedu } /** - * Parse a DB column value. Missing/null → legacy null (always in window). - * Non-null corrupt JSON → invalid (caller must not suppress). + * Parse a DB column value. Only SQL null / undefined → legacy null (always in window). + * Empty string, whitespace, or any non-null corrupt value → invalid (must not suppress). */ export function parseStoredNotificationSchedule( raw: unknown, ): { kind: 'null' } | { kind: 'ok'; schedule: NotificationSchedule } | { kind: 'invalid' } { - if (raw == null || raw === '') return { kind: 'null' }; - let parsed: unknown = raw; + if (raw == null) return { kind: 'null' }; if (typeof raw === 'string') { + const trimmed = raw.trim(); + if (trimmed === '') return { kind: 'invalid' }; + let parsed: unknown; try { - parsed = JSON.parse(raw); + parsed = JSON.parse(trimmed); } catch { return { kind: 'invalid' }; } + // JSON null is not SQL NULL; treat as corrupt rather than legacy always-on. + if (parsed == null) return { kind: 'invalid' }; + const result = parseNotificationSchedule(parsed); + if (!result.ok) return { kind: 'invalid' }; + return { kind: 'ok', schedule: result.schedule }; } - const result = parseNotificationSchedule(parsed); + const result = parseNotificationSchedule(raw); if (!result.ok) return { kind: 'invalid' }; return { kind: 'ok', schedule: result.schedule }; } diff --git a/backend/src/helpers/notificationSuppressionSync.ts b/backend/src/helpers/notificationSuppressionSync.ts index 1387f1ad..a5f641c8 100644 --- a/backend/src/helpers/notificationSuppressionSync.ts +++ b/backend/src/helpers/notificationSuppressionSync.ts @@ -102,11 +102,25 @@ async function pushOrCleanupScheduled(node: Node, rule: NotificationSuppressionR } } +async function cleanupInvalidScheduleReplica(node: Node, rule: NotificationSuppressionRule): Promise { + // Never POST an invalid schedule (would mute all day on remotes that ignore the field). + // Attempt DELETE so a prior valid/unscheduled replica cannot keep muting. + try { + await deleteRuleOnNode(node, rule.id); + console.warn( + `[SuppressionSync] Corrupt schedule on rule ${rule.id}: replica removed on node "${node.name}" (id=${node.id}); not posting`, + ); + } catch (err) { + console.error( + `[SuppressionSync] Corrupt schedule on rule ${rule.id}: cleanup pending on node "${node.name}" (id=${node.id}); ` + + `DELETE failed (${getErrorMessage(err, String(err))}). Prior replica may remain until connectivity returns`, + ); + } +} + async function syncRuleToNode(node: Node, rule: NotificationSuppressionRule): Promise { if (rule.scheduleInvalid) { - console.warn( - `[SuppressionSync] Skipping push of rule ${rule.id} to node "${node.name}": corrupt schedule; not syncing as unscheduled`, - ); + await cleanupInvalidScheduleReplica(node, rule); return; } if (rule.schedule != null) { diff --git a/frontend/src/components/settings/NotificationSuppressionSection.tsx b/frontend/src/components/settings/NotificationSuppressionSection.tsx index 1855b011..06af75ee 100644 --- a/frontend/src/components/settings/NotificationSuppressionSection.tsx +++ b/frontend/src/components/settings/NotificationSuppressionSection.tsx @@ -583,6 +583,8 @@ export function NotificationSuppressionSection({ size="sm" variant={selected ? 'default' : 'outline'} className="h-7 px-2 text-xs" + aria-pressed={selected} + aria-label={label} onClick={() => { setFormScheduleDays((prev) => selected @@ -598,12 +600,22 @@ export function NotificationSuppressionSection({
- - setFormScheduleStart(e.target.value)} /> + + setFormScheduleStart(e.target.value)} + />
- - setFormScheduleEnd(e.target.value)} /> + + setFormScheduleEnd(e.target.value)} + />

diff --git a/frontend/src/components/settings/__tests__/NotificationSuppressionSection.test.tsx b/frontend/src/components/settings/__tests__/NotificationSuppressionSection.test.tsx index c1fea07c..8fd53da3 100644 --- a/frontend/src/components/settings/__tests__/NotificationSuppressionSection.test.tsx +++ b/frontend/src/components/settings/__tests__/NotificationSuppressionSection.test.tsx @@ -1,8 +1,8 @@ /** - * NotificationSuppressionSection stack pattern chips. + * NotificationSuppressionSection stack pattern chips and weekly schedule UI. */ import { describe, it, expect, vi, beforeEach } from 'vitest'; -import { render, screen, waitFor } from '@testing-library/react'; +import { fireEvent, render, screen, waitFor } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; vi.mock('@/lib/api', () => ({ apiFetch: vi.fn() })); @@ -38,10 +38,49 @@ vi.mock('@/lib/muteRules', () => ({ })); import { apiFetch } from '@/lib/api'; +import { toast } from '@/components/ui/toast-store'; import { NotificationSuppressionSection } from '../NotificationSuppressionSection'; const mockedFetch = apiFetch as unknown as ReturnType; +const scheduledRule = { + id: 42, + name: 'Weekend mute', + node_id: null, + stack_patterns: ['prod-*'], + label_ids: null, + categories: null, + levels: null, + applies_to: 'both', + enabled: true, + expires_at: null, + schedule: { + days: [6], + start_minute: 120, + end_minute: 360, + tz: 'UTC', + }, + created_at: 1, + updated_at: 1, +}; + +function mockListRules(rules: unknown[] = []) { + mockedFetch.mockImplementation(async (url: string, opts?: { method?: string }) => { + if (url === '/notification-suppression-rules' && !opts?.method) { + return { ok: true, json: async () => rules }; + } + if (url === '/stacks') return { ok: true, json: async () => ['staging'] }; + if (url === '/labels') return { ok: true, json: async () => [] }; + if (url === '/notification-suppression-rules' && opts?.method === 'POST') { + return { ok: true, json: async () => ({ id: 1 }) }; + } + if (typeof url === 'string' && url.startsWith('/notification-suppression-rules/') && opts?.method === 'PUT') { + return { ok: true, json: async () => ({ id: 42 }) }; + } + return { ok: true, json: async () => ([]) }; + }); +} + async function openMuteForm() { render(); await waitFor(() => expect(screen.getByRole('button', { name: /Add mute rule|Add rule/i })).toBeInTheDocument()); @@ -49,20 +88,16 @@ async function openMuteForm() { await waitFor(() => expect(screen.getByRole('dialog', { name: /New mute rule/i })).toBeInTheDocument()); } +async function enableWeeklyWindow() { + await userEvent.click(screen.getByLabelText(/Weekly window \(UTC\)/i)); + await waitFor(() => expect(screen.getByRole('button', { name: 'Sat' })).toBeInTheDocument()); +} + describe('NotificationSuppressionSection', () => { beforeEach(() => { mockedFetch.mockReset(); - mockedFetch.mockImplementation(async (url: string, opts?: { method?: string }) => { - if (url === '/notification-suppression-rules' && !opts?.method) { - return { ok: true, json: async () => [] }; - } - if (url === '/stacks') return { ok: true, json: async () => ['staging'] }; - if (url === '/labels') return { ok: true, json: async () => [] }; - if (url === '/notification-suppression-rules' && opts?.method === 'POST') { - return { ok: true, json: async () => ({ id: 1 }) }; - } - return { ok: true, json: async () => ([]) }; - }); + vi.mocked(toast.error).mockClear(); + mockListRules([]); }); it('posts normalized stack patterns and null levels', async () => { @@ -129,4 +164,109 @@ describe('NotificationSuppressionSection', () => { expect(posts).toHaveLength(0); }); }); + + it('posts a weekly UTC schedule with accessible day and time controls', async () => { + await openMuteForm(); + + await userEvent.type(screen.getByPlaceholderText(/Mute staging/i), 'Sat window'); + await userEvent.type(screen.getByPlaceholderText(/Type a pattern/i), 'prod-*{Enter}'); + await enableWeeklyWindow(); + + const sat = screen.getByRole('button', { name: 'Sat' }); + expect(sat).toHaveAttribute('aria-pressed', 'false'); + await userEvent.click(sat); + expect(sat).toHaveAttribute('aria-pressed', 'true'); + + expect(screen.getByLabelText('Start (UTC)')).toHaveAttribute('id', 'mute-schedule-start'); + expect(screen.getByLabelText('End (UTC)')).toHaveAttribute('id', 'mute-schedule-end'); + fireEvent.change(screen.getByLabelText('Start (UTC)'), { target: { value: '02:00' } }); + fireEvent.change(screen.getByLabelText('End (UTC)'), { target: { value: '06:00' } }); + + await userEvent.click(screen.getByRole('button', { name: /Create|Update/i })); + await waitFor(() => { + const post = mockedFetch.mock.calls.find( + ([url, opts]) => url === '/notification-suppression-rules' && (opts as { method?: string })?.method === 'POST', + ); + expect(post).toBeTruthy(); + const body = JSON.parse((post![1] as { body: string }).body); + expect(body.schedule).toEqual({ + days: [6], + start_minute: 120, + end_minute: 360, + tz: 'UTC', + }); + }); + }); + + it('blocks scheduled create when no weekday is selected', async () => { + await openMuteForm(); + + await userEvent.type(screen.getByPlaceholderText(/Mute staging/i), 'No days'); + await userEvent.type(screen.getByPlaceholderText(/Type a pattern/i), 'prod-*{Enter}'); + await enableWeeklyWindow(); + await userEvent.click(screen.getByRole('button', { name: /Create|Update/i })); + + await waitFor(() => { + expect(toast.error).toHaveBeenCalledWith('Select at least one day for the weekly window.'); + }); + const posts = mockedFetch.mock.calls.filter( + ([url, opts]) => url === '/notification-suppression-rules' && (opts as { method?: string })?.method === 'POST', + ); + expect(posts).toHaveLength(0); + }); + + it('hydrates edit form from an existing schedule and keeps it on PUT', async () => { + mockListRules([scheduledRule]); + render(); + await waitFor(() => expect(screen.getByText('Weekend mute')).toBeInTheDocument()); + expect(screen.getByText(/UTC Sat 02:00-06:00/)).toBeInTheDocument(); + + await userEvent.click(screen.getByTitle('Edit')); + await waitFor(() => expect(screen.getByRole('dialog', { name: /Edit mute rule/i })).toBeInTheDocument()); + + expect(screen.getByLabelText(/Weekly window \(UTC\)/i)).toHaveAttribute('aria-checked', 'true'); + expect(screen.getByRole('button', { name: 'Sat' })).toHaveAttribute('aria-pressed', 'true'); + expect(screen.getByLabelText('Start (UTC)')).toHaveValue('02:00'); + expect(screen.getByLabelText('End (UTC)')).toHaveValue('06:00'); + + await userEvent.click(screen.getByRole('button', { name: /Create|Update/i })); + await waitFor(() => { + const put = mockedFetch.mock.calls.find( + ([url, opts]) => + url === '/notification-suppression-rules/42' && (opts as { method?: string })?.method === 'PUT', + ); + expect(put).toBeTruthy(); + const body = JSON.parse((put![1] as { body: string }).body); + expect(body.schedule).toEqual({ + days: [6], + start_minute: 120, + end_minute: 360, + tz: 'UTC', + }); + }); + }); + + it('clears schedule to null when weekly window is turned off on edit', async () => { + mockListRules([scheduledRule]); + render(); + await waitFor(() => expect(screen.getByText('Weekend mute')).toBeInTheDocument()); + + await userEvent.click(screen.getByTitle('Edit')); + await waitFor(() => expect(screen.getByRole('dialog', { name: /Edit mute rule/i })).toBeInTheDocument()); + await userEvent.click(screen.getByLabelText(/Weekly window \(UTC\)/i)); + await waitFor(() => { + expect(screen.queryByRole('button', { name: 'Sat' })).not.toBeInTheDocument(); + }); + + await userEvent.click(screen.getByRole('button', { name: /Create|Update/i })); + await waitFor(() => { + const put = mockedFetch.mock.calls.find( + ([url, opts]) => + url === '/notification-suppression-rules/42' && (opts as { method?: string })?.method === 'PUT', + ); + expect(put).toBeTruthy(); + const body = JSON.parse((put![1] as { body: string }).body); + expect(body.schedule).toBeNull(); + }); + }); });