From 71d20ceef38e37b3c61ccc78c5d8478f014d3a81 Mon Sep 17 00:00:00 2001 From: Hermes Date: Tue, 18 Aug 2026 05:52:32 -0700 Subject: [PATCH] [glm-grade=A] fix(auto-restart): await async servicesStateManager.read() so handleContainerDown actually fires (DC-060) --- .../__tests__/auto-restart-manager.test.js | 63 ++++++++++++++++--- .../src/managers/auto-restart-manager.js | 40 ++++++------ 2 files changed, 76 insertions(+), 27 deletions(-) diff --git a/dashcaddy-api/__tests__/auto-restart-manager.test.js b/dashcaddy-api/__tests__/auto-restart-manager.test.js index a1624d2..be64e23 100644 --- a/dashcaddy-api/__tests__/auto-restart-manager.test.js +++ b/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(); + }); + }); }); diff --git a/dashcaddy-api/src/managers/auto-restart-manager.js b/dashcaddy-api/src/managers/auto-restart-manager.js index c70b607..a438ba7 100644 --- a/dashcaddy-api/src/managers/auto-restart-manager.js +++ b/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} * @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) { - const readResult = 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 (_) { /* best effort */ } + if (!servicesStateManager) return null; + const list = await servicesStateManager.read(); + const found = (list || []).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 }); + } return null; }