diff --git a/dashcaddy-api/__tests__/bundled-workflows-health-check.test.js b/dashcaddy-api/__tests__/bundled-workflows-health-check.test.js index dc5ae18..7ace68a 100644 --- a/dashcaddy-api/__tests__/bundled-workflows-health-check.test.js +++ b/dashcaddy-api/__tests__/bundled-workflows-health-check.test.js @@ -52,15 +52,15 @@ describe('WorkflowEngine.healthCheckService — DC-042 followup (getState bug)', const result = await engine.healthCheckService('{{serviceId}}'); expect(readMock).toHaveBeenCalledTimes(1); - expect(result).toEqual({ checked: 0, healthy: 0, results: [] }); + expect(result).toEqual({ checked: 0, healthy: 0, results: [], failing: [] }); }); - test('returns checked/healthy counts from read() output', async () => { + test('returns checked/healthy counts from read() output (all healthy)', async () => { const docker = { client: { getContainer: jest.fn((id) => ({ inspect: jest.fn().mockResolvedValue({ - State: { Running: id === 'c1' }, + State: { Running: true, Health: { Status: 'healthy' } }, }), })), }, @@ -79,10 +79,38 @@ describe('WorkflowEngine.healthCheckService — DC-042 followup (getState bug)', const result = await engine.healthCheckService('{{serviceId}}'); expect(result.checked).toBe(2); // svc-3 skipped (no containerId) - expect(result.healthy).toBe(1); // c1 is running, c2 is not + expect(result.healthy).toBe(2); // both containers healthy expect(result.results).toHaveLength(2); expect(result.results[0]).toMatchObject({ service: 'svc-1', healthy: true }); - expect(result.results[1]).toMatchObject({ service: 'svc-2', healthy: false }); + expect(result.results[1]).toMatchObject({ service: 'svc-2', healthy: true }); + expect(result.failing).toEqual([]); + }); + + test('throws when any service is unhealthy — surfaces failing service IDs', async () => { + const docker = { + client: { + getContainer: jest.fn((id) => ({ + inspect: jest.fn().mockResolvedValue({ + State: { Running: id !== 'c2', Health: { Status: id === 'c2' ? 'unhealthy' : 'healthy' } }, + }), + })), + }, + }; + const engine = makeEngine({ + servicesStateManager: { + read: jest.fn().mockResolvedValue([ + { id: 'svc-1', containerId: 'c1' }, + { id: 'svc-2', containerId: 'c2' }, + ]), + }, + docker, + }); + + await expect(engine.healthCheckService('{{serviceId}}')).rejects.toThrow(/svc-2/); + await expect(engine.healthCheckService('{{serviceId}}')).rejects.toMatchObject({ + failingServices: ['svc-2'], + workflowResult: expect.objectContaining({ checked: 2, healthy: 1, failing: ['svc-2'] }), + }); }); test('gracefully degrades if read() throws — empty services list, no crash', async () => { @@ -96,7 +124,7 @@ describe('WorkflowEngine.healthCheckService — DC-042 followup (getState bug)', // Before the fix, this rejected because .read() wasn't called and the // .catch(() => []) fallback didn't exist. Now it should resolve to empty. const result = await engine.healthCheckService('{{serviceId}}'); - expect(result).toEqual({ checked: 0, healthy: 0, results: [] }); + expect(result).toEqual({ checked: 0, healthy: 0, results: [], failing: [] }); }); test('servicesStateManager absent on ctx → no crash, empty result', async () => { @@ -111,7 +139,7 @@ describe('WorkflowEngine.healthCheckService — DC-042 followup (getState bug)', } const result = await engine.healthCheckService('{{serviceId}}'); - expect(result).toEqual({ checked: 0, healthy: 0, results: [] }); + expect(result).toEqual({ checked: 0, healthy: 0, results: [], failing: [] }); }); test('single service (non-template serviceId) path still works', async () => { @@ -128,4 +156,245 @@ describe('WorkflowEngine.healthCheckService — DC-042 followup (getState bug)', const result = await engine.healthCheckService('single-svc-id'); expect(result).toEqual({ serviceId: 'single-svc-id', healthy: true }); }); + + test('single-service check throws when container is unhealthy', async () => { + const engine = makeEngine({ + docker: { + client: { + getContainer: jest.fn(() => ({ + inspect: jest.fn().mockResolvedValue({ State: { Running: false } }), + })), + }, + }, + }); + + await expect(engine.healthCheckService('down-svc')).rejects.toMatchObject({ + failingServices: ['down-svc'], + }); + }); +}); + +/** + * DC-044 root-cause fix tests: notify-on-failure gating + template interpolation. + * + * The original code in executeAction had TWO latent bugs: + * 1. notify-on-failure sent unconditionally (its comment said "Only send if + * previous action failed" but the code never checked). + * 2. healthCheckService returned no serviceId field, so templates like + * `Health check failed for {{serviceId}}` never interpolated and stayed + * literal in every alert. + * + * These tests exercise the full executeWorkflow path with a stub workflow + * that pairs `health-check` with `notify-on-failure`. + */ +describe('WorkflowEngine._runActions — DC-044 root-cause (notify-on-failure + template)', () => { + // Build an engine and call _runActions directly with arbitrary action + // sequences. Bypasses BUNDLED_WORKFLOWS lookup so tests are isolated and + // don't mutate module state. + function makeEngine(opts = {}) { + const ctx = { + servicesStateManager: opts.servicesStateManager || { read: jest.fn().mockResolvedValue([]) }, + docker: opts.docker || { client: { getContainer: jest.fn(() => ({ inspect: jest.fn().mockResolvedValue({ State: { Running: true } }) })) } }, + notification: opts.notification || { send: jest.fn() }, + }; + const engine = new WorkflowEngine(ctx); + if (engine.scheduledJobs) { + for (const job of engine.scheduledJobs.values()) clearInterval(job); + engine.scheduledJobs.clear(); + } + return engine; + } + + test('notify-on-failure is a no-op when the previous action succeeded', async () => { + const notify = jest.fn(); + const engine = makeEngine({ + servicesStateManager: { read: jest.fn().mockResolvedValue([{ id: 'svc-1', containerId: 'c1' }]) }, + docker: { client: { getContainer: jest.fn(() => ({ + inspect: jest.fn().mockResolvedValue({ State: { Running: true } }), + })) } }, + notification: { send: notify }, + }); + + const results = await engine._runActions( + [ + { type: 'health-check', target: '{{serviceId}}' }, + { type: 'notify-on-failure', message: 'Health check failed for {{failingServices}}' }, + ], + { trigger: 'manual' } + ); + + const notifyResult = results.find(r => r.action === 'notify-on-failure'); + expect(notifyResult.success).toBe(true); + expect(notifyResult.result).toEqual({ skipped: true, reason: 'no previous failure' }); + expect(notify).not.toHaveBeenCalled(); + }); + + test('notify-on-failure fires and interpolates {{failingServices}} when previous action failed', async () => { + const notify = jest.fn(); + const engine = makeEngine({ + servicesStateManager: { read: jest.fn().mockResolvedValue([{ id: 'svc-broken', containerId: 'c1' }]) }, + docker: { client: { getContainer: jest.fn(() => ({ + inspect: jest.fn().mockResolvedValue({ State: { Running: false } }), + })) } }, + notification: { send: notify }, + }); + + const results = await engine._runActions( + [ + { type: 'health-check', target: '{{serviceId}}' }, + { type: 'notify-on-failure', message: 'Health check failed for {{failingServices}}' }, + ], + { trigger: 'manual' } + ); + + const healthResult = results.find(r => r.action === 'health-check'); + const notifyResult = results.find(r => r.action === 'notify-on-failure'); + expect(healthResult.success).toBe(false); + expect(healthResult.failingServices).toEqual(['svc-broken']); + expect(notifyResult.success).toBe(true); + expect(notify).toHaveBeenCalledTimes(1); + // notification.send signature: (category, title, message, level) + const sentMessage = notify.mock.calls[0][2]; + expect(sentMessage).toBe('Health check failed for svc-broken'); + expect(sentMessage).not.toContain('{{'); + }); + + test('notify (not notify-on-failure) fires unconditionally — regression guard', async () => { + const notify = jest.fn(); + const engine = makeEngine({ notification: { send: notify } }); + + const results = await engine._runActions( + [{ type: 'notify', message: 'always sent' }], + { trigger: 'manual' } + ); + + expect(notify).toHaveBeenCalledTimes(1); + expect(notify.mock.calls[0][2]).toBe('always sent'); + expect(results[0].success).toBe(true); + }); + + test('notify-on-failure as first action is a no-op (no previous result)', async () => { + const notify = jest.fn(); + const engine = makeEngine({ notification: { send: notify } }); + + const results = await engine._runActions( + [{ type: 'notify-on-failure', message: 'should not fire' }], + { trigger: 'manual' } + ); + + const notifyResult = results[0]; + expect(notifyResult.success).toBe(true); + expect(notifyResult.result).toEqual({ skipped: true, reason: 'no previous failure' }); + expect(notify).not.toHaveBeenCalled(); + }); + + test('multi-service batch failure: {{failingServices}} interpolates comma-joined list', async () => { + const notify = jest.fn(); + const engine = makeEngine({ + servicesStateManager: { read: jest.fn().mockResolvedValue([ + { id: 'svc-ok', containerId: 'c1' }, + { id: 'svc-broken-1', containerId: 'c2' }, + { id: 'svc-broken-2', containerId: 'c3' }, + ]) }, + docker: { client: { getContainer: jest.fn((id) => ({ + inspect: jest.fn().mockResolvedValue({ + State: { Running: id === 'c1', Health: { Status: id === 'c1' ? 'healthy' : 'unhealthy' } }, + }), + })) } }, + notification: { send: notify }, + }); + + const results = await engine._runActions( + [ + { type: 'health-check', target: '{{serviceId}}' }, + { type: 'notify-on-failure', message: 'Failing: {{failingServices}}' }, + ], + { trigger: 'manual' } + ); + + expect(notify).toHaveBeenCalledTimes(1); + const sentMessage = notify.mock.calls[0][2]; + expect(sentMessage).toBe('Failing: svc-broken-1,svc-broken-2'); + expect(results.find(r => r.action === 'notify-on-failure').success).toBe(true); + }); + + // B2 regression: hit the actual bundled health-check-on-interval workflow + // end-to-end via executeWorkflow. The bundled template uses + // {{failingServices}} (DC-044 fix). Earlier it used {{serviceId}} which + // never resolved because no per-service ID is in workflow scope. This test + // would have failed with the old template. + test('executeWorkflow on bundled health-check-on-interval: no literal {{...}} in notification', async () => { + const { BUNDLED_WORKFLOWS } = require('../src/recipes/bundled-workflows'); + expect(BUNDLED_WORKFLOWS['health-check-on-interval']).toBeDefined(); + + const notify = jest.fn(); + const engine = makeEngine({ + servicesStateManager: { read: jest.fn().mockResolvedValue([ + { id: 'svc-broken', containerId: 'c1' }, + ]) }, + docker: { client: { getContainer: jest.fn(() => ({ + inspect: jest.fn().mockResolvedValue({ + State: { Running: false, Health: { Status: 'unhealthy' } }, + }), + })) } }, + notification: { send: notify }, + }); + + const result = await engine.executeWorkflow('health-check-on-interval', { trigger: 'manual' }); + + // Either the bundled workflow fired notification (with interpolated + // message) OR every action resolved — but in NO case may a literal + // {{...}} template token leak into notification.send. + if (notify.mock.calls.length > 0) { + const sentMessage = notify.mock.calls[0][2]; + expect(sentMessage).not.toMatch(/\{\{/); + expect(sentMessage).not.toMatch(/\}\}/); + // The new bundled template substitutes failingServices — make sure + // the actual service ID made it through. + expect(sentMessage).toContain('svc-broken'); + } + // Workflow must always complete (success or failure), never throw. + expect(result).toBeDefined(); + expect(result.workflowId).toBe('health-check-on-interval'); + }); + + // B3 regression: a running container with Health.Status === 'unhealthy' + // must be reported as unhealthy. Previously checkContainerHealth compared + // info.State.Health itself (an object) to the string 'unhealthy', which + // was always false — so any container with an explicit healthcheck was + // always considered healthy. The fix reads info.State.Health.Status. + test('checkContainerHealth treats running-but-unhealthy container as unhealthy', async () => { + const engine = makeEngine({ + docker: { client: { getContainer: jest.fn(() => ({ + inspect: jest.fn().mockResolvedValue({ + State: { Running: true, Health: { Status: 'unhealthy' } }, + }), + })) } }, + }); + + const healthy = await engine.checkContainerHealth('running-but-unhealthy'); + expect(healthy).toBe(false); + }); + + test('checkContainerHealth treats running-with-no-healthcheck as healthy', async () => { + const engine = makeEngine({ + docker: { client: { getContainer: jest.fn(() => ({ + inspect: jest.fn().mockResolvedValue({ State: { Running: true } }), + })) } }, + }); + + const healthy = await engine.checkContainerHealth('no-healthcheck'); + expect(healthy).toBe(true); + }); + + test('checkContainerHealth treats stopped container as unhealthy', async () => { + const engine = makeEngine({ + docker: { client: { getContainer: jest.fn(() => ({ + inspect: jest.fn().mockResolvedValue({ State: { Running: false } }), + })) } }, + }); + + const healthy = await engine.checkContainerHealth('stopped'); + expect(healthy).toBe(false); + }); }); diff --git a/dashcaddy-api/src/recipes/bundled-workflows.js b/dashcaddy-api/src/recipes/bundled-workflows.js index d2d216a..801e7b2 100644 --- a/dashcaddy-api/src/recipes/bundled-workflows.js +++ b/dashcaddy-api/src/recipes/bundled-workflows.js @@ -45,7 +45,12 @@ const BUNDLED_WORKFLOWS = { interval: 15 * 60 * 1000, // 15 minutes actions: [ { type: 'health-check', target: '{{serviceId}}' }, - { type: 'notify-on-failure', message: 'Health check failed for {{serviceId}}' } + // failingServices is set by healthCheckService when it throws (any + // service failed). It's a comma-joined string of failing service IDs. + // Previously this used {{serviceId}} which never resolved because + // no per-service ID is in scope at the workflow level — that's the + // DC-044 root-cause bug fix. + { type: 'notify-on-failure', message: 'Health check failed for {{failingServices}}' } ] }, 'disk-space-alert': { @@ -194,34 +199,23 @@ class WorkflowEngine extends EventEmitter { if (!workflow) { throw new Error(`Unknown workflow: ${workflowId}`); } - + if (!this.enabled.get(workflowId)) { console.log(`[WorkflowEngine] Workflow ${workflowId} is disabled, skipping`); return { skipped: true, reason: 'disabled' }; } - + const executionId = `${workflowId}-${Date.now()}`; const startTime = Date.now(); - + console.log(`[WorkflowEngine] Executing workflow: ${workflowId}`); this.emit('workflow-start', { workflowId, executionId, triggerData }); - - const results = []; - - for (const action of workflow.actions) { - try { - const result = await this.executeAction(action, triggerData); - results.push({ action: action.type, success: true, result }); - } catch (error) { - console.error(`[WorkflowEngine] Action ${action.type} failed:`, error.message); - results.push({ action: action.type, success: false, error: error.message }); - // Continue with other actions but log failure - } - } - + + const results = await this._runActions(workflow.actions, triggerData); + const duration = Date.now() - startTime; const allSucceeded = results.every(r => r.success); - + const historyEntry = { executionId, workflowId, @@ -232,22 +226,63 @@ class WorkflowEngine extends EventEmitter { success: allSucceeded, results }; - + this.history.push(historyEntry); - + // Keep history to last 500 entries if (this.history.length > 500) { this.history = this.history.slice(-500); } - + this.saveHistory(); - + this.emit('workflow-complete', historyEntry); console.log(`[WorkflowEngine] Workflow ${workflowId} completed in ${duration}ms, success: ${allSucceeded}`); - + return historyEntry; } + /** + * Run a sequence of actions and collect their results. Extracted from + * executeWorkflow so the per-action result threading (notify-on-failure + * gating) and the failingServices context surface can be unit-tested + * directly. executeWorkflow() is the production entry point; _runActions + * is an internal helper that callers shouldn't reach for. + */ + async _runActions(actions, triggerData = {}) { + const results = []; + + for (let i = 0; i < actions.length; i++) { + const action = actions[i]; + const previousResult = i > 0 ? results[i - 1] : null; + // notify-on-failure needs to see the previous action's outcome to decide + // whether to fire. Passing the full results array in the trigger data lets + // executeAction do that lookup without changing the action shape. + // Also surface failingServices (set by healthCheckService on throw) so + // template variables like {{failingServices}} can interpolate. + const actionContext = { + ...triggerData, + previousResult, + failingServices: previousResult && previousResult.failingServices ? previousResult.failingServices : undefined, + }; + try { + const result = await this.executeAction(action, actionContext); + results.push({ action: action.type, success: true, result }); + } catch (error) { + console.error(`[WorkflowEngine] Action ${action.type} failed:`, error.message); + results.push({ + action: action.type, + success: false, + error: error.message, + failingServices: error.failingServices, + }); + // Continue with other actions but log failure + } + } + + return results; + } + /** * Execute a single action */ @@ -269,7 +304,12 @@ class WorkflowEngine extends EventEmitter { ); case 'notify-on-failure': - // Only send if previous action failed + // Only send if previous action failed (success: false). The + // previousResult is injected by executeWorkflow's loop. If there + // was no previous action, this is a no-op (returns skipped). + if (!context.previousResult || context.previousResult.success !== false) { + return { skipped: true, reason: 'no previous failure' }; + } return this.notify( this.interpolate(action.message, context), action.channel @@ -323,11 +363,31 @@ class WorkflowEngine extends EventEmitter { } } } - return { checked: results.length, healthy: results.filter(r => r.healthy).length, results }; + // Surface failing service IDs so downstream notify-on-failure actions + // can interpolate `{{failingServices}}` into the alert message. Without + // this, templates like `Health check failed for {{serviceId}}` stay + // literal because there's no serviceId in scope. + const failing = results.filter(r => !r.healthy).map(r => r.service); + const healthy = results.filter(r => r.healthy).length; + const result = { checked: results.length, healthy, results, failing }; + if (failing.length > 0) { + // Throw so the action's success:false path is taken and notify-on-failure fires. + const err = new Error(`Health check failed for ${failing.length} service(s): ${failing.join(', ')}`); + err.failingServices = failing; + err.workflowResult = result; + throw err; + } + return result; } - + // Single service check const healthy = await this.checkContainerHealth(serviceId); + if (!healthy) { + const err = new Error(`Health check failed for ${serviceId}`); + err.failingServices = [serviceId]; + err.workflowResult = { serviceId, healthy }; + throw err; + } return { serviceId, healthy }; } @@ -338,10 +398,18 @@ class WorkflowEngine extends EventEmitter { try { const docker = this.ctx.docker?.client; if (!docker) return false; - + const container = docker.getContainer(containerId); const info = await container.inspect(); - return info.State && info.State.Running && info.State.Health !== 'unhealthy'; + // A container is healthy if it's running AND (it has no explicit + // health check OR its health check reports healthy/starting). + // info.State.Health is undefined when no HEALTHCHECK is declared. + // info.State.Health.Status is 'starting' | 'healthy' | 'unhealthy' + // when the health check IS declared. + if (!info.State || !info.State.Running) return false; + if (!info.State.Health) return true; // no health check defined → running = healthy + const status = info.State.Health.Status; + return status === 'healthy' || status === 'starting'; } catch (error) { return false; }