[grade=B] fix(workflows): DC-044 root-cause — gate notify-on-failure, interpolate failingServices, fix Health.Status check

The original DC-044 fix (b492e1c) repaired servicesStateManager.getState() but
missed two latent bugs at the same code path that were still spamming DNS2
every 15 minutes:

1. notify-on-failure fired unconditionally. The comment said 'Only send if
   previous action failed' but executeAction never checked. Every
   health-check-on-interval cycle ran notify regardless of outcome.

2. {{serviceId}} template never interpolated. healthCheckService returned
   { checked, healthy, results } with no serviceId in scope, so the
   production alert 'Health check failed for {{serviceId}}' stayed literal
   in every notification.

3. checkContainerHealth compared info.State.Health (an object) to the string
   'unhealthy' — always true, so any container with an explicit HEALTHCHECK
   was always reported healthy.

Fix:
- Extract _runActions(actions, triggerData) from executeWorkflow so the
  per-action result threading and failingServices context surface are
  testable in isolation.
- Gate notify-on-failure on previousResult.success === false. Returns
  { skipped: true, reason: 'no previous failure' } when no preceding failure.
- healthCheckService throws an Error with .failingServices attached when
  any service is unhealthy, surfacing IDs into the next action's context.
- checkContainerHealth now reads info.State.Health.Status: 'healthy' or
  'starting' → healthy, 'unhealthy' or no health check + stopped → unhealthy.
- Update bundled health-check-on-interval template from {{serviceId}} to
  {{failingServices}} (the variable now in scope).

Tests (12 new, 16 total in file):
- 5 _runActions tests (gate, interpolation, multi-service batch, first-action
  no-op, plain notify regression guard)
- 1 end-to-end executeWorkflow test against bundled health-check-on-interval
  asserting no literal {{...}} tokens reach notification.send
- 3 checkContainerHealth tests (running-but-unhealthy, no-healthcheck, stopped)
- 1 healthCheckService throw test with failingServices attached
- 2 updates to existing assertions for new return shape

Full suite: 1461/1463 (2 pre-existing license-keygen failures in DC-054
territory, unrelated to this commit).

Co-graded: Codex B urn:ump:b2nzzoulodwsullt3rhz4mtzou7fqgwiuoyrzxho67gdpwx3uvaa

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);
+  });
 });

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': {
@@ -206,18 +211,7 @@ class WorkflowEngine extends EventEmitter {
     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);
@@ -248,6 +242,47 @@ class WorkflowEngine extends EventEmitter {
     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 };
   }
 
@@ -341,7 +401,15 @@ class WorkflowEngine extends EventEmitter {
 
       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;
     }