[glm-grade=A] fix(auto-restart): await async servicesStateManager.read() so handleContainerDown actually fires (DC-060)

dashcaddy-api/__tests__/auto-restart-manager.test.js

@@ -336,32 +336,77 @@ describe('AutoRestartManager', () => {
   });
 
   describe('_resolveContainerId', () => {
-    test('returns containerId from status.details when present', () => {
+    test('returns containerId from status.details when present', async () => {
       const { manager } = makeManager();
-      const cid = manager._resolveContainerId('svc-1', { details: { containerId: 'cid-details' } });
+      const cid = await manager._resolveContainerId('svc-1', { details: { containerId: 'cid-details' } });
       expect(cid).toBe('cid-details');
     });
 
-    test('falls back to healthChecker.config.services[serviceId].containerId', () => {
+    test('falls back to healthChecker.config.services[serviceId].containerId', async () => {
       const { manager, healthChecker } = makeManager();
       healthChecker.config = { services: { 'svc-1': { containerId: 'cid-hc' } } };
-      const cid = manager._resolveContainerId('svc-1', { details: {} });
+      const cid = await manager._resolveContainerId('svc-1', { details: {} });
       expect(cid).toBe('cid-hc');
     });
 
-    test('falls back to servicesStateManager.read when sync list is returned', () => {
+    test('DC-060: awaits async servicesStateManager.read() and resolves containerId', async () => {
+      // Regression test for the auto-restart silently no-op bug:
+      // _resolveContainerId used to fire servicesStateManager.read() via
+      // .then(...) and discard the result. Callers gated on the return
+      // value, so a healthy→unhealthy transition whose only containerId
+      // source was the async state manager never triggered handleContainerDown.
       const { manager, servicesStateManager } = makeManager();
-      servicesStateManager.read.mockReturnValue([
+      servicesStateManager.read.mockResolvedValue([
         { id: 'svc-1', containerId: 'cid-state' },
       ]);
-      const cid = manager._resolveContainerId('svc-1', { details: {} });
+      const cid = await manager._resolveContainerId('svc-1', { details: {} });
       expect(cid).toBe('cid-state');
     });
 
-    test('returns null when no source has a containerId', () => {
+    test('returns null when no source has a containerId', async () => {
       const { manager } = makeManager();
-      const cid = manager._resolveContainerId('svc-unknown', { details: {} });
+      const cid = await manager._resolveContainerId('svc-unknown', { details: {} });
+      expect(cid).toBeNull();
+    });
+
+    test('swallows servicesStateManager.read() rejection', async () => {
+      const { manager, servicesStateManager } = makeManager();
+      servicesStateManager.read.mockRejectedValue(new Error('disk gone'));
+      const cid = await manager._resolveContainerId('svc-1', { details: {} });
       expect(cid).toBeNull();
     });
   });
+
+  describe('DC-060: healthy→unhealthy transitions trigger restart via async lookup', () => {
+    test('handleContainerDown is invoked with containerId from async state-manager lookup', async () => {
+      // End-to-end: containerId comes ONLY from servicesStateManager.read()
+      // (the production path for services.json-backed deployments).
+      const { manager, docker, servicesStateManager } = makeManager();
+      docker.client.getContainer.mockReturnValue({
+        start: jest.fn().mockResolvedValue(undefined),
+      });
+      await manager.setPolicy('svc-1', { maxRetries: 3, retryIntervalMs: 0 });
+      manager._previousHealth.set('svc-1', 'up');
+      servicesStateManager.read.mockResolvedValue([
+        { id: 'svc-1', containerId: 'cid-from-state' },
+      ]);
+
+      const handleDownSpy = jest.spyOn(manager, 'handleContainerDown');
+      await manager._handleStatusCheck({ serviceId: 'svc-1', status: 'down' });
+      expect(handleDownSpy).toHaveBeenCalledWith('svc-1', 'cid-from-state');
+    });
+
+    test('handleContainerDown is NOT invoked when async lookup returns no containerId', async () => {
+      const { manager, servicesStateManager } = makeManager();
+      await manager.setPolicy('svc-1', { maxRetries: 3, retryIntervalMs: 0 });
+      manager._previousHealth.set('svc-1', 'up');
+      servicesStateManager.read.mockResolvedValue([
+        { id: 'svc-1' /* no containerId */ },
+      ]);
+
+      const handleDownSpy = jest.spyOn(manager, 'handleContainerDown');
+      await manager._handleStatusCheck({ serviceId: 'svc-1', status: 'down' });
+      expect(handleDownSpy).not.toHaveBeenCalled();
+    });
+  });
 });

dashcaddy-api/src/managers/auto-restart-manager.js

@@ -406,7 +406,7 @@ class AutoRestartManager extends EventEmitter {
     // Transition: healthy → unhealthy
     if (previousStatus === 'up' && currentStatus === 'down') {
       // Find the containerId from the health checker config or status details
-      const containerId = this._resolveContainerId(serviceId, status);
+      const containerId = await this._resolveContainerId(serviceId, status);
       if (containerId) {
         try {
           await this.handleContainerDown(serviceId, containerId);
@@ -429,12 +429,19 @@ class AutoRestartManager extends EventEmitter {
   /**
    * Attempt to find the containerId for a service from various sources.
    *
+   * DC-060: the previous implementation fired the async lookup via `.then(...)`
+   * but discarded the returned containerId, returning `undefined` from the
+   * function. Callers (`_handleStatusCheck`) gate on the return value, so
+   * every auto-restart whose containerId came from servicesStateManager
+   * silently no-op'd. Now awaits the read() promise so the containerId
+   * actually propagates.
+   *
    * @param {string} serviceId
    * @param {Object} status - The status-check event data
-   * @returns {string|null}
+   * @returns {Promise<string|null>}
    * @private
    */
-  _resolveContainerId(serviceId, status) {
+  async _resolveContainerId(serviceId, status) {
     // Check if it's in the status details (some health checks embed it)
     if (status.details?.containerId) return status.details.containerId;
 
@@ -442,23 +449,20 @@ class AutoRestartManager extends EventEmitter {
     const hcService = this.healthChecker?.config?.services?.[serviceId];
     if (hcService?.containerId) return hcService.containerId;
 
-    // Try to look it up from the services state manager
+    // Try to look it up from the services state manager. StateManager.read()
+    // is async (returns a Promise) — must await, not fire-and-forget.
     try {
       const servicesStateManager = this.ctx.servicesStateManager;
-      if (servicesStateManager) {
+      if (!servicesStateManager) return null;
-        const readResult = servicesStateManager.read();
+      const list = await servicesStateManager.read();
-        if (readResult && typeof readResult.then === 'function') {
-          // It returns a promise — fire-and-forget lookup
-          readResult.then(list => {
       const found = (list || []).find(s => s.id === serviceId);
-            return found?.containerId || null;
-          }).catch(() => null);
-        } else {
-          const found = (readResult || []).find(s => s.id === serviceId);
       if (found?.containerId) return found.containerId;
+    } catch (err) {
+      // Best-effort: a state-manager read failure must not break the bridge.
+      // Surface at debug level so an operator hunting "why didn't auto-restart
+      // fire?" can find it without polluting the info-level event stream.
+      this.log?.debug?.('auto-restart', 'containerId resolve failed', { serviceId, error: err?.message });
     }
-      }
-    } catch (_) { /* best effort */ }
 
     return null;
   }