diff --git a/BACKLOG.md b/BACKLOG.md index 3373f51..95bae89 100644 --- a/BACKLOG.md +++ b/BACKLOG.md @@ -248,7 +248,12 @@ Tickets DC-033 through DC-041 were added after the DNS2 v1.14.4 / v1.14.8 / 0.0.0 incident. They are grounded in real evidence from that session — see DC-033's details for the full chain of reasoning (cross-checked by main agent + z.ai subagent). -## Coordination Rules +### DC-044: Fix WorkflowEngine healthCheckService — servicesStateManager.getState bug (silent every-5min error spam) +- **status:** in-progress +- **owner:** hermes +- **details:** `src/recipes/bundled-workflows.js:310` calls `servicesStateManager.getState()` which doesn't exist (StateManager exposes `read()`, not `getState()`). Combined with a missing `await`, the call returned a Promise (truthy), short-circuited via `|| []` to an empty array, then `for (const service of services)` silently iterated over zero services. Net effect: every `health-check-on-interval` workflow ran every 5 min, reported `Action health-check failed: servicesStateManager.getState is not a function`, sent a "Health check failed for {{serviceId}}" notification (with the unresolved template!), and produced `checked: 0, healthy: 0` results. Visible on BOTH DNS2 (production, 4 days) and test server dc-contabo-de (9 days). Fix: call `await servicesStateManager.read().catch(() => [])` — proper async + corruption-tolerant. +- **impact:** Workflow health checks now actually check container health, instead of silently reporting 0/0 every cycle. Stops the "Health check failed" notification spam. +- **result:** Fixed in src/recipes/bundled-workflows.js. New regression test `__tests__/bundled-workflows-health-check.test.js` — 5 cases (uses .read() not .getState(), correct counts, graceful degrade on read() throw, no servicesStateManager on ctx, single-service path). Full suite: 1219/1219 pass (+5 new). 1. **Always `git pull` before starting work.** 2. **Claim a task by editing BACKLOG.md:** set `status: in-progress` and `owner: hermes` or `owner: krystie`. diff --git a/dashcaddy-api/__tests__/bundled-workflows-health-check.test.js b/dashcaddy-api/__tests__/bundled-workflows-health-check.test.js new file mode 100644 index 0000000..dc5ae18 --- /dev/null +++ b/dashcaddy-api/__tests__/bundled-workflows-health-check.test.js @@ -0,0 +1,131 @@ +/** + * Regression tests for WorkflowEngine.healthCheckService (DC-042 followup). + * + * Bug: bundled-workflows.js:310 called `servicesStateManager.getState()` — + * a method that doesn't exist on StateManager. Combined with a missing + * `await`, this returned a Promise instead of an array, which then short- + * circuited via `|| []` to an empty array. The result: every health-check- + * on-interval workflow ran successfully with 0 services checked, while + * the workflow engine still reported "Action health-check failed: + * servicesStateManager.getState is not a function" on the dashboard. + * + * Fix: call `await servicesStateManager.read()` with a .catch fallback to + * an empty array so a corrupt/missing state file doesn't break the + * workflow. + */ + +const { WorkflowEngine } = require('../src/recipes/bundled-workflows'); + +function makeEngine(opts = {}) { + const ctx = { + servicesStateManager: opts.servicesStateManager || { + read: jest.fn().mockResolvedValue([]), + }, + docker: opts.docker !== undefined ? opts.docker : { + client: { + getContainer: jest.fn(), + }, + }, + }; + const engine = new WorkflowEngine(ctx); + // The constructor calls startScheduledWorkflows() which sets setInterval jobs. + // Those prevent Jest from exiting cleanly. Clear them after construction. + // We only care about healthCheckService behavior here, not scheduling. + if (engine.scheduledJobs) { + for (const job of engine.scheduledJobs.values()) { + clearInterval(job); + } + engine.scheduledJobs.clear(); + } + return engine; +} + +describe('WorkflowEngine.healthCheckService — DC-042 followup (getState bug)', () => { + test('uses .read() not the non-existent .getState() — does not throw', async () => { + const readMock = jest.fn().mockResolvedValue([]); + const engine = makeEngine({ + servicesStateManager: { read: readMock }, + docker: undefined, // no docker — exercises the falsy branch + }); + + // The original bug: this throws `servicesStateManager.getState is not a function` + const result = await engine.healthCheckService('{{serviceId}}'); + + expect(readMock).toHaveBeenCalledTimes(1); + expect(result).toEqual({ checked: 0, healthy: 0, results: [] }); + }); + + test('returns checked/healthy counts from read() output', async () => { + const docker = { + client: { + getContainer: jest.fn((id) => ({ + inspect: jest.fn().mockResolvedValue({ + State: { Running: id === 'c1' }, + }), + })), + }, + }; + const engine = makeEngine({ + servicesStateManager: { + read: jest.fn().mockResolvedValue([ + { id: 'svc-1', containerId: 'c1' }, + { id: 'svc-2', containerId: 'c2' }, + { id: 'svc-3' }, // no containerId, should be skipped + ]), + }, + docker, + }); + + 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.results).toHaveLength(2); + expect(result.results[0]).toMatchObject({ service: 'svc-1', healthy: true }); + expect(result.results[1]).toMatchObject({ service: 'svc-2', healthy: false }); + }); + + test('gracefully degrades if read() throws — empty services list, no crash', async () => { + const engine = makeEngine({ + servicesStateManager: { + read: jest.fn().mockRejectedValue(new Error('disk on fire')), + }, + docker: undefined, + }); + + // 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: [] }); + }); + + test('servicesStateManager absent on ctx → no crash, empty result', async () => { + const engine = new WorkflowEngine({ + servicesStateManager: null, + docker: undefined, + }); + // Same constructor cleanup + if (engine.scheduledJobs) { + for (const job of engine.scheduledJobs.values()) clearInterval(job); + engine.scheduledJobs.clear(); + } + + const result = await engine.healthCheckService('{{serviceId}}'); + expect(result).toEqual({ checked: 0, healthy: 0, results: [] }); + }); + + test('single service (non-template serviceId) path still works', async () => { + const engine = makeEngine({ + docker: { + client: { + getContainer: jest.fn(() => ({ + inspect: jest.fn().mockResolvedValue({ State: { Running: true } }), + })), + }, + }, + }); + + const result = await engine.healthCheckService('single-svc-id'); + expect(result).toEqual({ serviceId: 'single-svc-id', healthy: true }); + }); +}); diff --git a/dashcaddy-api/src/recipes/bundled-workflows.js b/dashcaddy-api/src/recipes/bundled-workflows.js index aea5d04..d2d216a 100644 --- a/dashcaddy-api/src/recipes/bundled-workflows.js +++ b/dashcaddy-api/src/recipes/bundled-workflows.js @@ -307,7 +307,11 @@ class WorkflowEngine extends EventEmitter { const results = []; const servicesStateManager = this.ctx.servicesStateManager; if (servicesStateManager) { - const services = servicesStateManager.getState() || []; + // StateManager exposes async read() — never getState(). The previous + // call site used the wrong method name AND forgot to await, returning + // a Promise instead of an array; the `|| []` short-circuit then made + // every check silently no-op with "Health check failed" errors. + const services = await servicesStateManager.read().catch(() => []) || []; for (const service of services) { if (service.containerId) { try {