diff --git a/backend/src/__tests__/blueprints-remote-deploy.test.ts b/backend/src/__tests__/blueprints-remote-deploy.test.ts index 081087d9..1d41ea6e 100644 --- a/backend/src/__tests__/blueprints-remote-deploy.test.ts +++ b/backend/src/__tests__/blueprints-remote-deploy.test.ts @@ -73,7 +73,7 @@ beforeEach(() => { afterEach(() => vi.restoreAllMocks()); describe('BlueprintService remote deploy', () => { - it('creates the stack, writes compose then marker, then deploys, in order', async () => { + it('applies atomically via the remote apply-local endpoint in a single call', async () => { const node = seedRemoteNode(); const bp = seedBlueprint([node.id]); const nodeObj = DatabaseService.getInstance().getNode(node.id)!; @@ -81,32 +81,27 @@ describe('BlueprintService remote deploy', () => { vi.spyOn(axios, 'get').mockResolvedValue({ status: 200, data: [] }); // hasNameConflict: no stacks const putSpy = vi.spyOn(axios, 'put').mockResolvedValue({ status: 200, data: {} }); - const postSpy = vi.spyOn(axios, 'post') - .mockResolvedValueOnce({ status: 201, data: {} }) // create stack - .mockResolvedValueOnce({ status: 200, data: {} }); // deploy + const postSpy = vi.spyOn(axios, 'post').mockResolvedValue({ status: 200, data: { deployed: true } }); const result = await BlueprintService.getInstance().deployToNode(bpObj, nodeObj); expect(result.status).toBe('active'); - expect(postSpy.mock.calls[0][0]).toMatch(/\/api\/stacks$/); - expect(putSpy.mock.calls[0][0]).toContain('docker-compose.yml'); - expect(putSpy.mock.calls[1][0]).toContain('.blueprint.json'); - expect(postSpy.mock.calls[1][0]).toMatch(/\/deploy$/); - // Assert global interleaving across spies, not just per-method order: - // create < compose < marker < deploy. (mock.calls indices alone would not - // catch the deploy POST firing before the file PUTs.) - const [createOrder, deployOrder] = postSpy.mock.invocationCallOrder; - const [composeOrder, markerOrder] = putSpy.mock.invocationCallOrder; - expect(createOrder).toBeLessThan(composeOrder); - expect(composeOrder).toBeLessThan(markerOrder); - expect(markerOrder).toBeLessThan(deployOrder); + // One atomic call to the remote (create + write + deploy run under the + // remote's lock); no separate file PUTs from the hub. + expect(postSpy).toHaveBeenCalledTimes(1); + expect(postSpy.mock.calls[0][0]).toMatch(/\/api\/blueprints\/apply-local$/); + expect(putSpy).not.toHaveBeenCalled(); + const payload = postSpy.mock.calls[0][1] as { stackName: string; composeContent: string; markerContent: string }; + expect(payload.stackName).toBe(bpObj.name); + expect(typeof payload.composeContent).toBe('string'); + expect(typeof payload.markerContent).toBe('string'); const dep = DatabaseService.getInstance().getDeployment(bp.id, node.id); expect(dep?.status).toBe('active'); expect(dep?.applied_revision).toBe(bpObj.revision); }); - it('treats a 409 on stack create as already-exists and proceeds', async () => { + it('falls back to the legacy create/write/deploy flow when the remote lacks apply-local (404)', async () => { const node = seedRemoteNode(); const bp = seedBlueprint([node.id]); const nodeObj = DatabaseService.getInstance().getNode(node.id)!; @@ -115,17 +110,40 @@ describe('BlueprintService remote deploy', () => { vi.spyOn(axios, 'get').mockResolvedValue({ status: 200, data: [] }); const putSpy = vi.spyOn(axios, 'put').mockResolvedValue({ status: 200, data: {} }); const postSpy = vi.spyOn(axios, 'post') - .mockResolvedValueOnce({ status: 409, data: { error: 'already exists' } }) - .mockResolvedValueOnce({ status: 200, data: {} }); + .mockResolvedValueOnce({ status: 404, data: {} }) // apply-local missing on older node + .mockResolvedValueOnce({ status: 201, data: {} }) // legacy create stack + .mockResolvedValueOnce({ status: 200, data: {} }); // legacy deploy const result = await BlueprintService.getInstance().deployToNode(bpObj, nodeObj); expect(result.status).toBe('active'); - expect(putSpy).toHaveBeenCalledTimes(2); - expect(postSpy).toHaveBeenCalledTimes(2); + expect(postSpy.mock.calls[0][0]).toMatch(/\/api\/blueprints\/apply-local$/); + expect(postSpy.mock.calls[1][0]).toMatch(/\/api\/stacks$/); + expect(putSpy).toHaveBeenCalledTimes(2); // compose + marker + expect(postSpy.mock.calls[2][0]).toMatch(/\/deploy$/); }); - it('maps a remote deploy failure to status=failed with the HTTP error', async () => { + it('maps a remote apply lock-conflict (409) to status=failed', async () => { + const node = seedRemoteNode(); + const bp = seedBlueprint([node.id]); + const nodeObj = DatabaseService.getInstance().getNode(node.id)!; + const bpObj = DatabaseService.getInstance().getBlueprint(bp.id)!; + + vi.spyOn(axios, 'get').mockResolvedValue({ status: 200, data: [] }); + const putSpy = vi.spyOn(axios, 'put').mockResolvedValue({ status: 200, data: {} }); + vi.spyOn(axios, 'post').mockResolvedValue({ + status: 409, + data: { error: 'web is busy: another operation (update) is already in progress' }, + }); + + const result = await BlueprintService.getInstance().deployToNode(bpObj, nodeObj); + + expect(result.status).toBe('failed'); + expect(result.error).toContain('already in progress'); + expect(putSpy).not.toHaveBeenCalled(); // no legacy file writes on conflict + }); + + it('maps a remote apply failure to status=failed with the HTTP error', async () => { const node = seedRemoteNode(); const bp = seedBlueprint([node.id]); const nodeObj = DatabaseService.getInstance().getNode(node.id)!; @@ -133,9 +151,7 @@ describe('BlueprintService remote deploy', () => { vi.spyOn(axios, 'get').mockResolvedValue({ status: 200, data: [] }); vi.spyOn(axios, 'put').mockResolvedValue({ status: 200, data: {} }); - vi.spyOn(axios, 'post') - .mockResolvedValueOnce({ status: 201, data: {} }) - .mockResolvedValueOnce({ status: 500, data: { error: 'boom' } }); + vi.spyOn(axios, 'post').mockResolvedValue({ status: 500, data: { error: 'boom' } }); const result = await BlueprintService.getInstance().deployToNode(bpObj, nodeObj); diff --git a/backend/src/__tests__/blueprints-route-validation.test.ts b/backend/src/__tests__/blueprints-route-validation.test.ts index a91cf2a4..ce4de5dd 100644 --- a/backend/src/__tests__/blueprints-route-validation.test.ts +++ b/backend/src/__tests__/blueprints-route-validation.test.ts @@ -7,6 +7,7 @@ let tmpDir: string; let app: import('express').Express; let DatabaseService: typeof import('../services/DatabaseService').DatabaseService; let LicenseService: typeof import('../services/LicenseService').LicenseService; +let StackOpLockService: typeof import('../services/StackOpLockService').StackOpLockService; let adminCookie: string; let counter = 0; @@ -14,6 +15,7 @@ beforeAll(async () => { tmpDir = await setupTestDb(); ({ DatabaseService } = await import('../services/DatabaseService')); ({ LicenseService } = await import('../services/LicenseService')); + ({ StackOpLockService } = await import('../services/StackOpLockService')); vi.spyOn(LicenseService.getInstance(), 'getTier').mockReturnValue('paid'); @@ -26,6 +28,7 @@ afterAll(() => cleanupTestDb(tmpDir)); beforeEach(() => { vi.restoreAllMocks(); vi.spyOn(LicenseService.getInstance(), 'getTier').mockReturnValue('paid'); + StackOpLockService.resetForTests(); const db = DatabaseService.getInstance().getDb(); db.prepare('DELETE FROM blueprint_deployments').run(); db.prepare('DELETE FROM blueprints').run(); @@ -80,3 +83,49 @@ describe('Blueprint route compose validation', () => { expect(res.body.error).toContain(`${MAX_BLUEPRINT_COMPOSE_BYTES} bytes or fewer`); }); }); + +describe('POST /api/blueprints/apply-local (node-to-node atomic apply)', () => { + it('rejects an invalid stack name', async () => { + const res = await request(app) + .post('/api/blueprints/apply-local') + .set('Cookie', adminCookie) + .send({ stackName: '../escape', composeContent: 'services: {}', markerContent: '{}' }); + expect(res.status).toBe(400); + expect(res.body.error).toContain('Invalid stack name'); + }); + + it('rejects a missing compose/marker payload', async () => { + const res = await request(app) + .post('/api/blueprints/apply-local') + .set('Cookie', adminCookie) + .send({ stackName: 'apply-local-stack' }); + expect(res.status).toBe(400); + }); + + it('rejects a structurally invalid marker', async () => { + const res = await request(app) + .post('/api/blueprints/apply-local') + .set('Cookie', adminCookie) + .send({ stackName: 'apply-local-stack', composeContent: 'services:\n app:\n image: nginx\n', markerContent: '{}' }); + expect(res.status).toBe(400); + expect(res.body.error).toContain('marker'); + }); + + it('returns 409 without deploying when the per-stack lock is held', async () => { + // The local node (id 1) holds the lock for this stack; the apply must be + // rejected before it can create or write any files. + StackOpLockService.getInstance().tryAcquire(1, 'apply-local-busy', 'update', 'admin'); + const res = await request(app) + .post('/api/blueprints/apply-local') + .set('Cookie', adminCookie) + .send({ + stackName: 'apply-local-busy', + composeContent: 'services:\n app:\n image: nginx\n', + markerContent: JSON.stringify({ blueprintId: 1, revision: 1, lastApplied: 123 }), + }); + expect(res.status).toBe(409); + expect(res.body.code).toBe('stack_op_in_progress'); + // The manual op still owns the lock; the apply never acquired it. + expect(StackOpLockService.getInstance().get(1, 'apply-local-busy')?.action).toBe('update'); + }); +}); diff --git a/backend/src/__tests__/blueprints.test.ts b/backend/src/__tests__/blueprints.test.ts index 8f2122f0..434d3439 100644 --- a/backend/src/__tests__/blueprints.test.ts +++ b/backend/src/__tests__/blueprints.test.ts @@ -24,6 +24,7 @@ let DatabaseService: typeof import('../services/DatabaseService').DatabaseServic let BlueprintReconciler: typeof import('../services/BlueprintReconciler').BlueprintReconciler; let NodeLabelService: typeof import('../services/NodeLabelService').NodeLabelService; let BlueprintService: typeof import('../services/BlueprintService').BlueprintService; +let StackOpLockService: typeof import('../services/StackOpLockService').StackOpLockService; let counter = 0; beforeAll(async () => { @@ -32,6 +33,7 @@ beforeAll(async () => { ({ BlueprintReconciler } = await import('../services/BlueprintReconciler')); ({ NodeLabelService } = await import('../services/NodeLabelService')); ({ BlueprintService } = await import('../services/BlueprintService')); + ({ StackOpLockService } = await import('../services/StackOpLockService')); }); afterAll(() => cleanupTestDb(tmpDir)); @@ -43,6 +45,7 @@ beforeEach(() => { db.prepare('DELETE FROM node_labels').run(); db.prepare("DELETE FROM nodes WHERE is_default = 0").run(); db.prepare("UPDATE global_settings SET value = '0' WHERE key = 'developer_mode'").run(); + StackOpLockService.resetForTests(); vi.restoreAllMocks(); }); @@ -393,6 +396,67 @@ describe('BlueprintReconciler developer-mode diagnostics', () => { }); }); +describe('BlueprintService per-stack lock', () => { + it('deploy under a free lock writes compose then marker, then deploys', async () => { + const nodeId = seedNode(); + const bp = seedBlueprint({ classification: 'stateless', nodeIds: [nodeId] }); + const node = DatabaseService.getInstance().getNode(nodeId)!; + const { FileSystemService } = await import('../services/FileSystemService'); + const { ComposeService } = await import('../services/ComposeService'); + // Spy the file/deploy primitives so the locked critical section runs + // without touching the real filesystem or Docker. + vi.spyOn(FileSystemService.prototype, 'createStack').mockResolvedValue(undefined); + const writeSpy = vi.spyOn(FileSystemService.prototype, 'writeStackFile').mockResolvedValue(undefined); + const deploySpy = vi.spyOn(ComposeService.prototype, 'deployStack').mockResolvedValue(undefined); + + const outcome = await BlueprintService.getInstance().deployToNode(bp, node); + + expect(outcome.status).toBe('active'); + expect(deploySpy).toHaveBeenCalledWith(bp.name, undefined, false); + // Compose is written first, then the marker, both before the deploy. + expect(writeSpy).toHaveBeenCalledTimes(2); + expect(writeSpy.mock.calls[0][2]).toBe(bp.compose_content); + expect(writeSpy.mock.calls[1][2]).toContain('"blueprintId"'); + const [composeOrder, markerOrder] = writeSpy.mock.invocationCallOrder; + const [deployOrder] = deploySpy.mock.invocationCallOrder; + expect(composeOrder).toBeLessThan(markerOrder); + expect(markerOrder).toBeLessThan(deployOrder); + }); + + it('deploy skips, writes no stack files, and records failed when the stack lock is held', async () => { + const nodeId = seedNode(); + const bp = seedBlueprint({ classification: 'stateless', nodeIds: [nodeId] }); + const node = DatabaseService.getInstance().getNode(nodeId)!; + // A manual operation holds the lock; the reconcile deploy must not race + // it, and must not mutate compose/marker files before owning the lock. + StackOpLockService.getInstance().tryAcquire(nodeId, bp.name, 'update', 'admin'); + + const outcome = await BlueprintService.getInstance().deployToNode(bp, node); + + expect(outcome.status).toBe('failed'); + expect(outcome.error).toContain('already in progress'); + // No marker file was written (the lock guards the file writes too). + expect(await BlueprintService.getInstance().readMarker(bp.name, node)).toBeNull(); + // The manual op still holds the lock; the deploy never acquired it. + expect(StackOpLockService.getInstance().get(nodeId, bp.name)?.action).toBe('update'); + }); + + it('withdraw skips and records failed when a manual operation holds the stack lock', async () => { + const nodeId = seedNode(); + const bp = seedBlueprint({ classification: 'stateless', nodeIds: [nodeId] }); + const node = DatabaseService.getInstance().getNode(nodeId)!; + // A manual operation holds the lock; the withdraw must not race it. + StackOpLockService.getInstance().tryAcquire(nodeId, bp.name, 'update', 'admin'); + + const outcome = await BlueprintService.getInstance().withdrawFromNode(bp, node); + + expect(outcome.status).toBe('failed'); + expect(outcome.error).toContain('already in progress'); + // The lock is still held by the manual op (the withdraw never acquired it). + expect(StackOpLockService.getInstance().get(nodeId, bp.name)?.action).toBe('update'); + }); +}); + describe('BlueprintService marker parsing + name-conflict guard', () => { it('parseMarker accepts a well-formed marker', () => { const marker = BlueprintService.parseMarker(JSON.stringify({ blueprintId: 7, revision: 3, lastApplied: 12345 })); diff --git a/backend/src/__tests__/labels-bulk-actions.test.ts b/backend/src/__tests__/labels-bulk-actions.test.ts index 5383376d..7978133d 100644 --- a/backend/src/__tests__/labels-bulk-actions.test.ts +++ b/backend/src/__tests__/labels-bulk-actions.test.ts @@ -49,6 +49,7 @@ let authHeader: string; let db: import('../services/DatabaseService').DatabaseService; let LicenseService: typeof import('../services/LicenseService').LicenseService; let activeBulkActions: typeof import('../routes/labels').activeBulkActions; +let StackOpLockService: typeof import('../services/StackOpLockService').StackOpLockService; let labelCounter = 0; beforeAll(async () => { @@ -56,6 +57,7 @@ beforeAll(async () => { ({ app } = await import('../index')); ({ LicenseService } = await import('../services/LicenseService')); ({ activeBulkActions } = await import('../routes/labels')); + ({ StackOpLockService } = await import('../services/StackOpLockService')); const { DatabaseService } = await import('../services/DatabaseService'); db = DatabaseService.getInstance(); authHeader = `Bearer ${jwt.sign({ username: TEST_USERNAME }, TEST_JWT_SECRET, { expiresIn: '1m' })}`; @@ -77,6 +79,7 @@ beforeEach(() => { restartContainer.mockResolvedValue(undefined); enforcePolicyPreDeploy.mockResolvedValue({ ok: true }); activeBulkActions.clear(); + StackOpLockService.resetForTests(); db.getDb().prepare('DELETE FROM stack_label_assignments').run(); db.getDb().prepare('DELETE FROM stack_labels').run(); }); @@ -150,6 +153,27 @@ describe('Stack Labels bulk actions', () => { expect(restartContainer).not.toHaveBeenCalled(); }); + it('skips a stack whose per-stack lock is held by a manual operation', async () => { + const label = await createAssignedLabel(['alpha', 'beta']); + // A manual operation holds 'alpha'; the bulk deploy must not race it. + StackOpLockService.getInstance().tryAcquire(label.node_id, 'alpha', 'update', 'admin'); + + const res = await request(app) + .post(`/api/labels/${label.id}/action`) + .set('Authorization', authHeader) + .send({ action: 'deploy' }); + + expect(res.status).toBe(200); + const alpha = res.body.results.find((r: { stackName: string }) => r.stackName === 'alpha'); + const beta = res.body.results.find((r: { stackName: string }) => r.stackName === 'beta'); + expect(alpha).toMatchObject({ stackName: 'alpha', success: false }); + expect(alpha.error).toContain('another operation (update) is already in progress'); + expect(beta).toEqual({ stackName: 'beta', success: true }); + // 'alpha' was skipped; only 'beta' reached ComposeService. + expect(deployStack).toHaveBeenCalledTimes(1); + expect(deployStack).toHaveBeenCalledWith('beta', undefined, false); + }); + it('dry-run deploy runs the policy gate and reports blocked stacks honestly', async () => { const label = await createAssignedLabel(['alpha']); enforcePolicyPreDeploy.mockResolvedValue({ diff --git a/backend/src/__tests__/scheduler-service.test.ts b/backend/src/__tests__/scheduler-service.test.ts index 6713010a..0fe6a09a 100644 --- a/backend/src/__tests__/scheduler-service.test.ts +++ b/backend/src/__tests__/scheduler-service.test.ts @@ -195,9 +195,13 @@ vi.mock('../services/PolicyEnforcement', () => ({ })); import { SchedulerService } from '../services/SchedulerService'; +import { StackOpLockService } from '../services/StackOpLockService'; beforeEach(() => { vi.clearAllMocks(); + // Lifecycle/update handlers run through the real StackOpLockService; reset it + // so a lock left held by one test cannot make a later test skip its op. + StackOpLockService.resetForTests(); // clearAllMocks only clears call history, not implementations, so restore the // mocks that individual tests mutate (tier, node lookup, proxy target) to // their documented defaults. Without this a test that points getNode at a @@ -1643,6 +1647,17 @@ describe('SchedulerService - lifecycle actions', () => { expect(mockUpdateScheduledTaskRun).toHaveBeenCalledWith(1, expect.objectContaining({ status: 'failure' })); }); + it('records failure (not success) and skips the op when the stack lock is held', async () => { + // A manual operation holds the lock; the scheduled lifecycle op must skip + // rather than race, and surface as a failed run instead of a silent success. + StackOpLockService.getInstance().tryAcquire(1, 'my-stack', 'deploy', 'admin'); + mockGetScheduledTask.mockReturnValue(makeLifecycleTask('auto_stop')); + await SchedulerService.getInstance().triggerTask(300); + expect(mockRunCommand).not.toHaveBeenCalled(); + expect(mockUpdateScheduledTaskRun).toHaveBeenCalledWith(1, expect.objectContaining({ status: 'failure' })); + expect(mockUpdateScheduledTask).toHaveBeenCalledWith(300, expect.objectContaining({ last_status: 'failure' })); + }); + it('paid tier executes lifecycle actions', async () => { mockGetTier.mockReturnValue('paid'); mockGetScheduledTask.mockReturnValue(makeLifecycleTask('auto_stop')); diff --git a/backend/src/__tests__/stack-op-lock-service.test.ts b/backend/src/__tests__/stack-op-lock-service.test.ts index 2e41d9ed..12163e03 100644 --- a/backend/src/__tests__/stack-op-lock-service.test.ts +++ b/backend/src/__tests__/stack-op-lock-service.test.ts @@ -6,7 +6,7 @@ * `resetForTests()` so state doesn't leak between cases. */ import { describe, it, expect, beforeEach } from 'vitest'; -import { StackOpLockService } from '../services/StackOpLockService'; +import { StackOpLockService, stackOpSkipMessage } from '../services/StackOpLockService'; beforeEach(() => { StackOpLockService.resetForTests(); @@ -94,3 +94,56 @@ describe('StackOpLockService', () => { expect(lock!.startedAt).toBeLessThanOrEqual(Date.now()); }); }); + +describe('StackOpLockService.runExclusive', () => { + it('runs fn and releases the lock when the slot is free', async () => { + const svc = StackOpLockService.getInstance(); + const outcome = await svc.runExclusive(1, 'web', 'deploy', 'system', async () => 'done'); + expect(outcome).toEqual({ ran: true, result: 'done' }); + // Released after fn resolves, so a later op acquires. + expect(svc.size()).toBe(0); + }); + + it('holds the lock for the duration of fn, blocking a concurrent acquire', async () => { + const svc = StackOpLockService.getInstance(); + let observed: ReturnType | null = null; + const outcome = await svc.runExclusive(1, 'web', 'update', 'system', async () => { + // A manual op attempting to acquire mid-operation must be rejected. + observed = svc.tryAcquire(1, 'web', 'deploy', 'admin'); + return 42; + }); + expect(outcome).toEqual({ ran: true, result: 42 }); + expect(observed!.acquired).toBe(false); + }); + + it('skips (ran=false) and returns the holder when the lock is already held', async () => { + const svc = StackOpLockService.getInstance(); + svc.tryAcquire(1, 'web', 'rollback', 'admin'); + let called = false; + const outcome = await svc.runExclusive(1, 'web', 'deploy', 'system', async () => { + called = true; + return 'should not run'; + }); + expect(called).toBe(false); + expect(outcome.ran).toBe(false); + if (!outcome.ran) expect(outcome.existing.action).toBe('rollback'); + }); + + it('releases the lock even when fn throws, then propagates', async () => { + const svc = StackOpLockService.getInstance(); + await expect( + svc.runExclusive(1, 'web', 'deploy', 'system', async () => { + throw new Error('boom'); + }), + ).rejects.toThrow('boom'); + expect(svc.size()).toBe(0); + }); +}); + +describe('stackOpSkipMessage', () => { + it('names the stack and the conflicting action', () => { + expect(stackOpSkipMessage('web', 'update')).toBe( + 'Skipped "web": another operation (update) is already in progress.', + ); + }); +}); diff --git a/backend/src/__tests__/update-guard-rollback.test.ts b/backend/src/__tests__/update-guard-rollback.test.ts index 6e14d128..41e026a2 100644 --- a/backend/src/__tests__/update-guard-rollback.test.ts +++ b/backend/src/__tests__/update-guard-rollback.test.ts @@ -30,7 +30,7 @@ const baseInputs = (over: Partial = {}): RollbackInputs => ({ backup: { exists: true, timestamp: NOW - 3_600_000 }, envSummary: { exists: true, envPresent: true, keys: ['DB_HOST', 'DB_PASS'] }, stackHasEnv: true, - rollbackTarget: { target: 'nginx:1.27.1' }, + rollbackTarget: { target: 'nginx:1.27.1', moving: false }, lastDeployAt: NOW - 3_600_000, containers: [{ name: 'app-web-1', state: 'running', health: 'healthy', exitCode: null, @@ -89,15 +89,22 @@ describe('buildRollbackItems', () => { }); it('marks the previous image unknown when no rollback target is known', () => { - expect(itemById(baseInputs({ rollbackTarget: { target: null } }), 'previous_images').state).toBe('unknown'); + expect(itemById(baseInputs({ rollbackTarget: { target: null, moving: false } }), 'previous_images').state).toBe('unknown'); expect(itemById(baseInputs({ rollbackTarget: 'error' }), 'previous_images').state).toBe('unknown'); const known = itemById(baseInputs(), 'previous_images'); expect(known.state).toBe('ready'); expect(known.detail).toContain('nginx:1.27.1'); }); + it('downgrades the previous image to not_covered for a moving tag', () => { + const item = itemById(baseInputs({ rollbackTarget: { target: 'nginx:latest', moving: true } }), 'previous_images'); + expect(item.state).toBe('not_covered'); + expect(item.detail).toContain('moving image tag'); + expect(item.detail).toContain('nginx:latest'); + }); + it('does not mistake an image literally named error for a failed preview', () => { - const item = itemById(baseInputs({ rollbackTarget: { target: 'error' } }), 'previous_images'); + const item = itemById(baseInputs({ rollbackTarget: { target: 'error', moving: false } }), 'previous_images'); expect(item.state).toBe('ready'); expect(item.detail).toContain('error'); }); @@ -128,7 +135,12 @@ describe('aggregateRollbackOverall', () => { }); it('is partial when the previous image tag is unknown', () => { - const items = buildRollbackItems(baseInputs({ rollbackTarget: { target: null } }), NOW); + const items = buildRollbackItems(baseInputs({ rollbackTarget: { target: null, moving: false } }), NOW); + expect(aggregateRollbackOverall(items)).toBe('partial'); + }); + + it('is partial when the rollback target is a moving tag', () => { + const items = buildRollbackItems(baseInputs({ rollbackTarget: { target: 'nginx:latest', moving: true } }), NOW); expect(aggregateRollbackOverall(items)).toBe('partial'); }); diff --git a/backend/src/__tests__/update-guard-service.test.ts b/backend/src/__tests__/update-guard-service.test.ts index d70e6de5..b78d3da0 100644 --- a/backend/src/__tests__/update-guard-service.test.ts +++ b/backend/src/__tests__/update-guard-service.test.ts @@ -11,6 +11,8 @@ const { mockGetLatest, mockGetPreview, mockGetBackupInfo, + mockGetBackupEnvSummary, + mockEnvExists, mockGetOpenDriftFindings, mockGetGlobalSettings, mockFsSize, @@ -20,6 +22,8 @@ const { mockGetLatest: vi.fn(), mockGetPreview: vi.fn(), mockGetBackupInfo: vi.fn(), + mockGetBackupEnvSummary: vi.fn(), + mockEnvExists: vi.fn(), mockGetOpenDriftFindings: vi.fn(), mockGetGlobalSettings: vi.fn(), mockFsSize: vi.fn(), @@ -40,7 +44,10 @@ vi.mock('../services/ComposeDoctorService', () => ({ ComposeDoctorService: { getInstance: () => ({ getLatest: mockGetLatest }) }, })); -vi.mock('../services/UpdatePreviewService', () => ({ +vi.mock('../services/UpdatePreviewService', async (importOriginal) => ({ + // Keep the real pure helpers (isMovingTag, parseSemverTag) that + // UpdateGuardService imports; only stub the service singleton. + ...(await importOriginal()), UpdatePreviewService: { getInstance: () => ({ getPreview: mockGetPreview }) }, })); @@ -48,8 +55,8 @@ vi.mock('../services/FileSystemService', () => ({ FileSystemService: { getInstance: () => ({ getBackupInfo: mockGetBackupInfo, - getBackupEnvSummary: vi.fn().mockRejectedValue(new Error('not used here')), - envExists: vi.fn().mockRejectedValue(new Error('not used here')), + getBackupEnvSummary: mockGetBackupEnvSummary, + envExists: mockEnvExists, }), }, })); @@ -81,6 +88,9 @@ const inspectResult = (over: Record = {}) => ({ beforeEach(() => { vi.clearAllMocks(); mockGetGlobalSettings.mockReturnValue({ host_disk_limit: '90' }); + // Sensible defaults for the rollback-readiness inputs (only computeRollbackReadiness reads these). + mockGetBackupEnvSummary.mockResolvedValue({ exists: true, envPresent: true, keys: ['DB_HOST'] }); + mockEnvExists.mockResolvedValue(true); }); describe('UpdateGuardService.probeContainers', () => { @@ -153,3 +163,36 @@ describe('UpdateGuardService.computeUpdateReadiness wiring', () => { expect(report.verdict).toBe('ready'); }); }); + +describe('UpdateGuardService.computeRollbackReadiness moving-tag wiring', () => { + const preview = (images: Array<{ current_tag: string }>) => ({ + stack_name: 'app', + images, + summary: { + has_update: false, primary_image: 'app', current_tag: images[0]?.current_tag ?? null, + next_tag: null, semver_bump: 'none', update_kind: 'none', blocked: false, blocked_reason: null, + }, + rollback_target: 'app:1.2.3', + changelog: null, + }); + + beforeEach(() => { + mockGetBackupInfo.mockResolvedValue({ exists: true, timestamp: Date.now() }); + mockListContainers.mockResolvedValue([]); + }); + + it('marks previous_images not_covered (overall partial) when any image uses a moving tag', async () => { + // Primary pinned, sidecar on a moving tag: a file rollback cannot revert it. + mockGetPreview.mockResolvedValue(preview([{ current_tag: '1.2.3' }, { current_tag: 'latest' }])); + const report = await UpdateGuardService.getInstance().computeRollbackReadiness(0, 'app'); + expect(report.items.find(i => i.id === 'previous_images')?.state).toBe('not_covered'); + expect(report.overall).toBe('partial'); + }); + + it('marks previous_images ready (overall ready) when every image is pinned', async () => { + mockGetPreview.mockResolvedValue(preview([{ current_tag: '1.2.3' }, { current_tag: 'v2.0.1' }])); + const report = await UpdateGuardService.getInstance().computeRollbackReadiness(0, 'app'); + expect(report.items.find(i => i.id === 'previous_images')?.state).toBe('ready'); + expect(report.overall).toBe('ready'); + }); +}); diff --git a/backend/src/__tests__/update-preview-service.test.ts b/backend/src/__tests__/update-preview-service.test.ts index 2e601305..603fe7ef 100644 --- a/backend/src/__tests__/update-preview-service.test.ts +++ b/backend/src/__tests__/update-preview-service.test.ts @@ -5,6 +5,7 @@ import { computeSemverBump, computeImagePreview, buildSummary, + isMovingTag, type ComputePreviewDeps, } from '../services/UpdatePreviewService'; @@ -25,6 +26,21 @@ describe('parseSemverTag', () => { }); }); +describe('isMovingTag', () => { + it('treats fully-pinned semver as immutable', () => { + expect(isMovingTag('1.2.3')).toBe(false); + expect(isMovingTag('v1.2.3')).toBe(false); + expect(isMovingTag('27.1.4-alpine')).toBe(false); + }); + it('treats latest, branches, and unpinned major/minor as moving', () => { + expect(isMovingTag('latest')).toBe(true); + expect(isMovingTag('main')).toBe(true); + expect(isMovingTag('stable')).toBe(true); + expect(isMovingTag('1.25')).toBe(true); + expect(isMovingTag('unknown')).toBe(true); + }); +}); + describe('findNextTag', () => { it('picks highest semver greater than current', () => { const tags = ['27.1.3', '27.1.4', '27.1.5', '27.2.0', '27.1.5-alpine']; diff --git a/backend/src/routes/blueprints.ts b/backend/src/routes/blueprints.ts index 78da3cc0..c8d42b5b 100644 --- a/backend/src/routes/blueprints.ts +++ b/backend/src/routes/blueprints.ts @@ -1,6 +1,7 @@ import { Router, type Request, type Response } from 'express'; import { authMiddleware } from '../middleware/auth'; import { requirePaid, requireAdmin, requireBody } from '../middleware/tierGates'; +import { requirePermission } from '../middleware/permissions'; import { DatabaseService, type BlueprintSelector, @@ -310,6 +311,53 @@ blueprintsRouter.delete('/:id', async (req: Request, res: Response): Promise => { + if (!requirePaid(req, res)) return; + const body = (req.body ?? {}) as { stackName?: unknown; composeContent?: unknown; markerContent?: unknown }; + if (typeof body.stackName !== 'string' || !isValidStackName(body.stackName)) { + res.status(400).json({ error: 'Invalid stack name' }); + return; + } + // Same per-stack RBAC as writing the compose file and deploying it directly. + if (!requirePermission(req, res, 'stack:edit', 'stack', body.stackName)) return; + if (!requirePermission(req, res, 'stack:deploy', 'stack', body.stackName)) return; + if (typeof body.composeContent !== 'string' || typeof body.markerContent !== 'string') { + res.status(400).json({ error: 'composeContent and markerContent are required strings' }); + return; + } + if (Buffer.byteLength(body.composeContent, 'utf8') > MAX_BLUEPRINT_COMPOSE_BYTES) { + res.status(413).json({ error: 'compose content too large' }); + return; + } + if (BlueprintService.parseMarker(body.markerContent) === null) { + res.status(400).json({ error: 'Invalid blueprint marker' }); + return; + } + try { + const outcome = await BlueprintService.getInstance().applyLocalUnderLock( + req.nodeId, body.stackName, body.composeContent, body.markerContent, '/api/blueprints/apply-local', + ); + if (!outcome.ran) { + res.status(409).json({ + error: `${body.stackName} is busy: another operation (${outcome.existingAction}) is already in progress`, + code: 'stack_op_in_progress', + inProgress: { action: outcome.existingAction }, + }); + return; + } + res.json({ deployed: true }); + } catch (error) { + console.error('[Blueprints] apply-local error:', sanitizeForLog(getErrorMessage(error, 'apply failed'))); + res.status(500).json({ error: getErrorMessage(error, 'Blueprint apply failed') }); + } +}); + blueprintsRouter.post('/:id/apply', async (req: Request, res: Response): Promise => { if (!requirePaid(req, res)) return; if (!requireAdmin(req, res)) return; diff --git a/backend/src/routes/fleet.ts b/backend/src/routes/fleet.ts index 045945b4..370603b5 100644 --- a/backend/src/routes/fleet.ts +++ b/backend/src/routes/fleet.ts @@ -12,6 +12,7 @@ import { computeNodeNetworkingSummary, type NodeNetworkingSummary } from '../ser import DockerController from '../services/DockerController'; import { FileSystemService } from '../services/FileSystemService'; import { ComposeService } from '../services/ComposeService'; +import { StackOpLockService } from '../services/StackOpLockService'; import SelfUpdateService from '../services/SelfUpdateService'; import { getSenchoVersion, isValidVersion } from '../services/CapabilityRegistry'; import { authMiddleware } from '../middleware/auth'; @@ -2043,7 +2044,13 @@ async function applySnapshotStackFiles( // the remote node), so this only performs the deploy itself. async function redeploySnapshotStack(node: Node, stackName: string): Promise { if (node.type === 'local') { - await ComposeService.getInstance(node.id).deployStack(stackName); + const lock = await StackOpLockService.getInstance().runExclusive( + node.id, stackName, 'deploy', 'system', + () => ComposeService.getInstance(node.id).deployStack(stackName), + ); + if (!lock.ran) { + throw new Error(`Cannot redeploy "${stackName}": another operation (${lock.existing.action}) is already in progress.`); + } return; } const ctx = buildRemoteProxyContext(node); diff --git a/backend/src/routes/imageUpdates.ts b/backend/src/routes/imageUpdates.ts index 670fc376..03f92d6c 100644 --- a/backend/src/routes/imageUpdates.ts +++ b/backend/src/routes/imageUpdates.ts @@ -7,6 +7,7 @@ import { CacheService } from '../services/CacheService'; import { ImageUpdateService } from '../services/ImageUpdateService'; import { FileSystemService } from '../services/FileSystemService'; import { ComposeService } from '../services/ComposeService'; +import { StackOpLockService, stackOpSkipMessage } from '../services/StackOpLockService'; import { NotificationService } from '../services/NotificationService'; import { enforcePolicyPreDeploy } from '../services/PolicyEnforcement'; import { HealthGateService } from '../services/HealthGateService'; @@ -321,7 +322,14 @@ autoUpdateRouter.post('/execute', authMiddleware, async (req: Request, res: Resp continue; } - await compose.updateStack(stackName, undefined, atomic); + const lock = await StackOpLockService.getInstance().runExclusive( + req.nodeId, stackName, 'update', 'system', + () => compose.updateStack(stackName, undefined, atomic), + ); + if (!lock.ran) { + results.push(stackOpSkipMessage(stackName, lock.existing.action)); + continue; + } db.clearStackUpdateStatus(req.nodeId, stackName); HealthGateService.getInstance().begin(req.nodeId, stackName, 'update', `auto-update:${req.user?.username ?? 'scheduler'}`); diff --git a/backend/src/routes/labels.ts b/backend/src/routes/labels.ts index 2f57f55a..8f4fcbe3 100644 --- a/backend/src/routes/labels.ts +++ b/backend/src/routes/labels.ts @@ -2,6 +2,7 @@ import { Router, type Request, type Response } from 'express'; import { DatabaseService } from '../services/DatabaseService'; import { FileSystemService } from '../services/FileSystemService'; import { ComposeService } from '../services/ComposeService'; +import { StackOpLockService, stackOpSkipMessage } from '../services/StackOpLockService'; import DockerController from '../services/DockerController'; import { enforcePolicyPreDeploy } from '../services/PolicyEnforcement'; import { authMiddleware } from '../middleware/auth'; @@ -234,7 +235,16 @@ labelsRouter.post('/:id/action', authMiddleware, async (req: Request, res: Respo results.push({ stackName, success: true, dryRun: true }); continue; } - await ComposeService.getInstance(req.nodeId).deployStack(stackName, undefined, false); + // Per-stack lock so a bulk deploy cannot race a manual + // deploy/update/rollback/backup on the same stack and node. + const lock = await StackOpLockService.getInstance().runExclusive( + req.nodeId, stackName, 'deploy', 'system', + () => ComposeService.getInstance(req.nodeId).deployStack(stackName, undefined, false), + ); + if (!lock.ran) { + results.push({ stackName, success: false, error: stackOpSkipMessage(stackName, lock.existing.action) }); + continue; + } } else { // stop / restart have no pre-action policy gate; dry-run just // confirms the stack would be reached. @@ -242,12 +252,21 @@ labelsRouter.post('/:id/action', authMiddleware, async (req: Request, res: Respo results.push({ stackName, success: true, dryRun: true }); continue; } - const dockerController = DockerController.getInstance(req.nodeId); - const containers = await dockerController.getContainersByStack(stackName); - if (action === 'stop') { - await Promise.all(containers.map(c => dockerController.stopContainer(c.Id))); - } else { - await Promise.all(containers.map(c => dockerController.restartContainer(c.Id))); + const lock = await StackOpLockService.getInstance().runExclusive( + req.nodeId, stackName, action === 'stop' ? 'stop' : 'restart', 'system', + async () => { + const dockerController = DockerController.getInstance(req.nodeId); + const containers = await dockerController.getContainersByStack(stackName); + if (action === 'stop') { + await Promise.all(containers.map(c => dockerController.stopContainer(c.Id))); + } else { + await Promise.all(containers.map(c => dockerController.restartContainer(c.Id))); + } + }, + ); + if (!lock.ran) { + results.push({ stackName, success: false, error: stackOpSkipMessage(stackName, lock.existing.action) }); + continue; } } results.push({ stackName, success: true }); diff --git a/backend/src/routes/stacks.ts b/backend/src/routes/stacks.ts index c64bb8f1..a0d91aa2 100644 --- a/backend/src/routes/stacks.ts +++ b/backend/src/routes/stacks.ts @@ -1612,7 +1612,7 @@ stacksRouter.post('/:stackName/rollback', async (req: Request, res: Response) => await ComposeService.getInstance(req.nodeId).deployStack(stackName, getTerminalWs(req.get(DEPLOY_SESSION_HEADER)), false); invalidateNodeCaches(req.nodeId); dlog(`[Stacks] Rollback completed: ${sanitizeForLog(stackName)}`); - res.json({ message: 'Stack rolled back successfully.' }); + res.json({ message: 'Stack rolled back: compose and env files restored.' }); notifyActionSuccess('deploy_success', `${stackName} rolled back`, stackName, req.user?.username ?? 'system'); } catch (error: unknown) { console.error('[Stacks] Rollback failed: %s', sanitizeForLog(stackName), error); diff --git a/backend/src/services/BlueprintService.ts b/backend/src/services/BlueprintService.ts index e35878fb..ac4913c3 100644 --- a/backend/src/services/BlueprintService.ts +++ b/backend/src/services/BlueprintService.ts @@ -9,6 +9,7 @@ import { type Node, } from './DatabaseService'; import { ComposeService } from './ComposeService'; +import { StackOpLockService, stackOpSkipMessage, type StackOpAction } from './StackOpLockService'; import { FileSystemService } from './FileSystemService'; import { NodeRegistry } from './NodeRegistry'; import { PROXY_TIER_HEADER } from './license-headers'; @@ -379,8 +380,8 @@ export class BlueprintService { // ---- local primitives ---- - private async stackDirExists(node: Node, blueprintName: string): Promise { - const baseDir = NodeRegistry.getInstance().getComposeDir(node.id); + private async stackDirExists(nodeId: number, blueprintName: string): Promise { + const baseDir = NodeRegistry.getInstance().getComposeDir(nodeId); const stackDir = path.resolve(baseDir, blueprintName); if (!stackDir.startsWith(path.resolve(baseDir))) return false; try { @@ -403,35 +404,79 @@ export class BlueprintService { throw new Error(`Policy "${gate.policy?.name}" blocked deploy: ${gate.violations.length} image(s) exceed ${gate.policy?.max_severity}`); } - const fs = FileSystemService.getInstance(node.id); - if (!(await this.stackDirExists(node, blueprint.name))) { - await fs.createStack(blueprint.name); - } - await fs.writeStackFile(blueprint.name, COMPOSE_FILENAME, blueprint.compose_content); - await fs.writeStackFile(blueprint.name, MARKER_FILENAME, JSON.stringify(marker, null, 2)); - await assertPolicyGateAllows( - blueprint.name, + const outcome = await this.applyLocalUnderLock( node.id, - buildSystemPolicyGateOptions('blueprint', { - auditPath: `/api/blueprints/${blueprint.id}/deployments/${node.id}`, - }), + blueprint.name, + blueprint.compose_content, + JSON.stringify(marker, null, 2), + `/api/blueprints/${blueprint.id}/deployments/${node.id}`, ); - await ComposeService.getInstance(node.id).deployStack(blueprint.name, undefined, false); + if (!outcome.ran) { + throw new Error(stackOpSkipMessage(blueprint.name, outcome.existingAction)); + } triggerPostDeployScan(blueprint.name, node.id).catch(err => { console.error('[BlueprintService] post-deploy scan failed for "%s" on node %s: %s', sanitizeForLog(blueprint.name), node.id, sanitizeForLog(BlueprintService.formatError(err))); }); } + /** + * Create the stack if needed, write the compose and marker files, run the + * deploy policy gate, and deploy, all under the per-stack operation lock so + * none of it can race a manual deploy/update/rollback/backup on the same + * stack and node. Runs on the node that owns the stack: deployLocal calls it + * for the hub's own node, and the /api/blueprints/apply-local route calls it + * on a remote node receiving a blueprint apply from its hub (so the file + * writes hold the remote's lock, not just the deploy). On lock conflict + * nothing is written and { ran: false } is returned. + */ + async applyLocalUnderLock( + nodeId: number, + stackName: string, + composeContent: string, + markerContent: string, + auditPath: string, + ): Promise<{ ran: true } | { ran: false; existingAction: StackOpAction }> { + const fs = FileSystemService.getInstance(nodeId); + const lock = await StackOpLockService.getInstance().runExclusive( + nodeId, stackName, 'deploy', 'system', + async () => { + if (!(await this.stackDirExists(nodeId, stackName))) { + await fs.createStack(stackName); + } + await fs.writeStackFile(stackName, COMPOSE_FILENAME, composeContent); + await fs.writeStackFile(stackName, MARKER_FILENAME, markerContent); + await assertPolicyGateAllows( + stackName, + nodeId, + buildSystemPolicyGateOptions('blueprint', { auditPath }), + ); + await ComposeService.getInstance(nodeId).deployStack(stackName, undefined, false); + }, + ); + return lock.ran ? { ran: true } : { ran: false, existingAction: lock.existing.action }; + } + private async withdrawLocal(blueprint: Blueprint, node: Node): Promise { - try { - await ComposeService.getInstance(node.id).downStack(blueprint.name); - } catch (err) { - // best-effort: continue to delete the directory even if down fails - console.warn(`[BlueprintService] downStack failed for "${blueprint.name}" on node ${node.id}: ${BlueprintService.formatError(err)}`); - } - if (await this.stackDirExists(node, blueprint.name)) { - await FileSystemService.getInstance(node.id).deleteStack(blueprint.name); + // Hold the per-stack lock across both the compose down and the directory + // delete so a withdraw cannot race a manual operation, nor tear the + // files out from under one that starts mid-withdraw. + const lock = await StackOpLockService.getInstance().runExclusive( + node.id, blueprint.name, 'down', 'system', + async () => { + try { + await ComposeService.getInstance(node.id).downStack(blueprint.name); + } catch (err) { + // best-effort: continue to delete the directory even if down fails + console.warn(`[BlueprintService] downStack failed for "${blueprint.name}" on node ${node.id}: ${BlueprintService.formatError(err)}`); + } + if (await this.stackDirExists(node.id, blueprint.name)) { + await FileSystemService.getInstance(node.id).deleteStack(blueprint.name); + } + }, + ); + if (!lock.ran) { + throw new Error(stackOpSkipMessage(blueprint.name, lock.existing.action)); } } @@ -452,6 +497,45 @@ export class BlueprintService { const baseUrl = target.apiUrl.replace(/\/$/, ''); const headers = this.remoteHeaders(target.apiToken); + // Atomic apply: the remote runs create + write compose/marker + deploy + // under its own per-stack lock, so the file writes cannot race a manual + // operation on that node. Older nodes without this route answer 404; we + // fall back to the legacy multi-call flow there (not lock-atomic). + const res = await axios.post( + `${baseUrl}/api/blueprints/apply-local`, + { + stackName: blueprint.name, + composeContent: blueprint.compose_content, + markerContent: JSON.stringify(marker, null, 2), + }, + { headers, timeout: REMOTE_HTTP_TIMEOUT_MS, validateStatus: () => true }, + ); + if (res.status === 404) { + console.warn(`[BlueprintService] remote node ${node.id} lacks /api/blueprints/apply-local; using legacy non-atomic apply`); + await this.deployRemoteLegacy(blueprint, node, marker); + return; + } + if (res.status === 409) { + throw new Error(`blueprint apply skipped: ${BlueprintService.extractApiError(res.data) || 'another operation is already in progress'}`); + } + if (res.status >= 400) { + throw new Error(`blueprint apply: HTTP ${res.status} ${BlueprintService.extractApiError(res.data)}`); + } + } + + /** + * Legacy remote apply for nodes that predate /api/blueprints/apply-local: + * create, write compose, write marker, deploy as separate calls. The remote + * deploy locks, but the preceding file writes do not, so this is not atomic + * against a concurrent manual operation on that node. Kept only as a + * compatibility fallback. + */ + private async deployRemoteLegacy(blueprint: Blueprint, node: Node, marker: BlueprintMarker): Promise { + const target = NodeRegistry.getInstance().getProxyTarget(node.id); + if (!target) throw new Error(`Remote node "${node.name}" has no proxy target configured`); + const baseUrl = target.apiUrl.replace(/\/$/, ''); + const headers = this.remoteHeaders(target.apiToken); + // 1. Ensure stack exists. POST returns 409 when already exists; we treat that as success. const createRes = await axios.post(`${baseUrl}/api/stacks`, { stackName: blueprint.name }, diff --git a/backend/src/services/ComposeService.ts b/backend/src/services/ComposeService.ts index 4d7f1688..512b4ed2 100644 --- a/backend/src/services/ComposeService.ts +++ b/backend/src/services/ComposeService.ts @@ -341,7 +341,7 @@ export class ComposeService { await this.withRegistryAuth(async (env) => { await this.execute('docker', await this.authoredComposeArgs(stackName, ['up', '-d', '--remove-orphans']), stackDir, ws, true, env); }, sendOutput); - sendOutput('=== Rolled back successfully ===\n'); + sendOutput('=== Restored previous compose and env files ===\n'); return true; } catch (rollbackError) { console.error('Rollback failed for %s:', sanitizeForLog(stackName), getErrorMessage(rollbackError, 'unknown error')); @@ -413,7 +413,7 @@ export class ComposeService { if (debug) console.debug(`[ComposeService:debug] deployStack completed in ${Date.now() - t0}ms`, { stackName }); } catch (deployError) { if (atomic) { - sendOutput('\n=== Deployment failed - rolling back to previous version ===\n'); + sendOutput('\n=== Deployment failed - restoring previous compose and env files ===\n'); const rolledBack = await this.restoreAtomicBackup(stackName, stackDir, ws, sendOutput); throw new ComposeRollbackError(deployError, true, rolledBack); } @@ -613,7 +613,7 @@ export class ComposeService { if (debug) console.debug(`[ComposeService:debug] updateStack completed in ${Date.now() - t0}ms`, { stackName }); } catch (updateError) { if (atomic) { - sendOutput('\n=== Update failed - rolling back to previous version ===\n'); + sendOutput('\n=== Update failed - restoring previous compose and env files ===\n'); const rolledBack = await this.restoreAtomicBackup(stackName, stackDir, ws, sendOutput); throw new ComposeRollbackError(updateError, true, rolledBack); } diff --git a/backend/src/services/GitSourceService.ts b/backend/src/services/GitSourceService.ts index 67d89976..c266e126 100644 --- a/backend/src/services/GitSourceService.ts +++ b/backend/src/services/GitSourceService.ts @@ -8,6 +8,7 @@ import { CryptoService } from './CryptoService'; import { DatabaseService, type StackGitSource, type GitSourceAuthType, type GitSourceAppliedSpec } from './DatabaseService'; import { FileSystemService } from './FileSystemService'; import { ComposeService } from './ComposeService'; +import { StackOpLockService } from './StackOpLockService'; import { HealthGateService } from './HealthGateService'; import { NodeRegistry } from './NodeRegistry'; import { assertPolicyGateAllows, buildSystemPolicyGateOptions } from '../helpers/policyGate'; @@ -1315,7 +1316,15 @@ export class GitSourceService { auditPath: `/api/stacks/${stackName}/git-source/apply`, }), ); - await ComposeService.getInstance().deployStack(stackName); + const lock = await StackOpLockService.getInstance().runExclusive( + nodeId, stackName, 'deploy', 'system', + () => ComposeService.getInstance(nodeId).deployStack(stackName), + ); + if (!lock.ran) { + const busy = `Auto-deploy skipped: another operation (${lock.existing.action}) is already in progress for ${stackName}.`; + console.warn(`[GitSource] ${busy}`); + return { applied: true, deployed: false, deployError: busy }; + } HealthGateService.getInstance().begin(nodeId, stackName, 'deploy', 'system:git-source'); console.log(`[GitSource] Applied and deployed ${stackName} at ${commitSha.slice(0, 7)}`); return { applied: true, deployed: true }; diff --git a/backend/src/services/MeshService.ts b/backend/src/services/MeshService.ts index 1bcfbf6a..63e1854a 100644 --- a/backend/src/services/MeshService.ts +++ b/backend/src/services/MeshService.ts @@ -4,6 +4,7 @@ import fs from 'fs/promises'; import { EventEmitter } from 'events'; import * as YAML from 'yaml'; import { ComposeService } from './ComposeService'; +import { StackOpLockService } from './StackOpLockService'; import { DatabaseService, type NodeMode } from './DatabaseService'; import DockerController from './DockerController'; import { FileSystemService } from './FileSystemService'; @@ -2380,7 +2381,13 @@ export class MeshService extends EventEmitter implements MeshForwarderHost { auditPath: `/api/mesh/nodes/${nodeId}/stacks/${stackName}/redeploy`, }), ); - await ComposeService.getInstance(nodeId).deployStack(stackName); + const lock = await StackOpLockService.getInstance().runExclusive( + nodeId, stackName, 'deploy', 'system', + () => ComposeService.getInstance(nodeId).deployStack(stackName), + ); + if (!lock.ran) { + throw new Error(`Cannot redeploy "${stackName}": another operation (${lock.existing.action}) is already in progress.`); + } this.logActivity({ source: 'mesh', level: 'info', type: 'mesh.enable', nodeId, diff --git a/backend/src/services/SchedulerService.ts b/backend/src/services/SchedulerService.ts index e1247b4e..ba4db381 100644 --- a/backend/src/services/SchedulerService.ts +++ b/backend/src/services/SchedulerService.ts @@ -5,6 +5,7 @@ import { LicenseService } from './LicenseService'; import { PROXY_TIER_HEADER } from './license-headers'; import DockerController from './DockerController'; import { ComposeService } from './ComposeService'; +import { StackOpLockService, stackOpSkipMessage as skipMessage } from './StackOpLockService'; import { FileSystemService } from './FileSystemService'; import { HealthGateService } from './HealthGateService'; import { ImageUpdateService } from './ImageUpdateService'; @@ -488,7 +489,14 @@ export class SchedulerService { await this.postToRemoteStack(task.node_id, `${encodeURIComponent(task.target_id)}/backup`); return `Backed up stack "${task.target_id}" files on remote node`; } - await FileSystemService.getInstance(task.node_id).backupStackFiles(task.target_id); + const localNodeId = task.node_id ?? NodeRegistry.getInstance().getDefaultNodeId(); + const lock = await StackOpLockService.getInstance().runExclusive( + localNodeId, task.target_id, 'backup', 'system', + () => FileSystemService.getInstance(localNodeId).backupStackFiles(task.target_id), + ); + // Throw (not return) so the skip records as a failed run instead of a + // silent success; the next scheduled tick retries once the lock frees. + if (!lock.ran) throw new Error(skipMessage(task.target_id, lock.existing.action)); return `Backed up stack "${task.target_id}" files`; } @@ -498,7 +506,12 @@ export class SchedulerService { await this.postToRemoteStack(task.node_id, `${encodeURIComponent(task.target_id)}/stop`); return `Stopped stack "${task.target_id}" (containers preserved) on remote node`; } - await ComposeService.getInstance(task.node_id).runCommand(task.target_id, 'stop'); + const localNodeId = task.node_id ?? NodeRegistry.getInstance().getDefaultNodeId(); + const lock = await StackOpLockService.getInstance().runExclusive( + localNodeId, task.target_id, 'stop', 'system', + () => ComposeService.getInstance(localNodeId).runCommand(task.target_id, 'stop'), + ); + if (!lock.ran) throw new Error(skipMessage(task.target_id, lock.existing.action)); return `Stopped stack "${task.target_id}" (containers preserved)`; } @@ -508,7 +521,12 @@ export class SchedulerService { await this.postToRemoteStack(task.node_id, `${encodeURIComponent(task.target_id)}/down`); return `Took down stack "${task.target_id}" (containers removed) on remote node`; } - await ComposeService.getInstance(task.node_id).runCommand(task.target_id, 'down'); + const localNodeId = task.node_id ?? NodeRegistry.getInstance().getDefaultNodeId(); + const lock = await StackOpLockService.getInstance().runExclusive( + localNodeId, task.target_id, 'down', 'system', + () => ComposeService.getInstance(localNodeId).runCommand(task.target_id, 'down'), + ); + if (!lock.ran) throw new Error(skipMessage(task.target_id, lock.existing.action)); return `Took down stack "${task.target_id}" (containers removed)`; } @@ -527,7 +545,12 @@ export class SchedulerService { 'Auto-start', `/api/scheduled-tasks/${task.id}/run`, ); - await ComposeService.getInstance(task.node_id).deployStack(task.target_id); + const localNodeId = task.node_id ?? NodeRegistry.getInstance().getDefaultNodeId(); + const lock = await StackOpLockService.getInstance().runExclusive( + localNodeId, task.target_id, 'deploy', 'system', + () => ComposeService.getInstance(localNodeId).deployStack(task.target_id), + ); + if (!lock.ran) throw new Error(skipMessage(task.target_id, lock.existing.action)); return `Started stack "${task.target_id}"`; } @@ -860,7 +883,11 @@ export class SchedulerService { // Atomic backup/rollback is the default deploy mode: take a pre-op // backup and roll back on failure for every scheduled auto-update. const atomic = true; - await compose.updateStack(stackName, undefined, atomic); + const lock = await StackOpLockService.getInstance().runExclusive( + nodeId, stackName, 'update', 'system', + () => compose.updateStack(stackName, undefined, atomic), + ); + if (!lock.ran) return skipMessage(stackName, lock.existing.action); db.clearStackUpdateStatus(nodeId, stackName); HealthGateService.getInstance().begin(nodeId, stackName, 'update', 'system:scheduler'); diff --git a/backend/src/services/StackOpLockService.ts b/backend/src/services/StackOpLockService.ts index e79cc326..ef3bb6c5 100644 --- a/backend/src/services/StackOpLockService.ts +++ b/backend/src/services/StackOpLockService.ts @@ -11,6 +11,14 @@ export type StackOpAction = 'deploy' | 'down' | 'restart' | 'stop' | 'start' | 'update' | 'rollback' | 'backup'; +/** + * Note returned by a background path that skipped its operation because a manual + * or concurrent operation already held the stack's lock. + */ +export function stackOpSkipMessage(stackName: string, existingAction: StackOpAction): string { + return `Skipped "${stackName}": another operation (${existingAction}) is already in progress.`; +} + export interface StackOpLock { action: StackOpAction; startedAt: number; @@ -62,6 +70,34 @@ export class StackOpLockService { this.locks.delete(this.key(nodeId, stackName)); } + /** + * Acquire the per-(nodeId, stackName) lock for the duration of `fn`, then + * release it. Returns `{ ran: true, result }` when the lock was free, or + * `{ ran: false, existing }` when another operation already holds it, so the + * caller can skip rather than race. Background/system paths (scheduler, + * webhook, Git source, image auto-update, label bulk actions, fleet snapshot + * redeploy, mesh redeploy) run their lifecycle calls through this so they + * cannot interleave with a manual deploy/update/rollback/backup on the same + * stack and node. An error thrown by `fn` still releases the lock, then + * propagates to the caller. + */ + public async runExclusive( + nodeId: number, + stackName: string, + action: StackOpAction, + user: string, + fn: () => Promise, + ): Promise<{ ran: true; result: T } | { ran: false; existing: StackOpLock }> { + const acquired = this.tryAcquire(nodeId, stackName, action, user); + if (!acquired.acquired) return { ran: false, existing: acquired.existing }; + try { + const result = await fn(); + return { ran: true, result }; + } finally { + this.release(nodeId, stackName); + } + } + public get(nodeId: number, stackName: string): StackOpLock | undefined { return this.locks.get(this.key(nodeId, stackName)); } diff --git a/backend/src/services/UpdateGuardService.ts b/backend/src/services/UpdateGuardService.ts index ee62db37..f46c5dad 100644 --- a/backend/src/services/UpdateGuardService.ts +++ b/backend/src/services/UpdateGuardService.ts @@ -3,7 +3,7 @@ import DockerController from './DockerController'; import { DatabaseService } from './DatabaseService'; import { FileSystemService } from './FileSystemService'; import { ComposeDoctorService } from './ComposeDoctorService'; -import { UpdatePreviewService } from './UpdatePreviewService'; +import { UpdatePreviewService, isMovingTag } from './UpdatePreviewService'; import { withTimeout } from '../utils/withTimeout'; import { getErrorMessage } from '../utils/errors'; import { sanitizeForLog } from '../utils/safeLog'; @@ -140,7 +140,14 @@ export class UpdateGuardService { backup, envSummary, stackHasEnv, - rollbackTarget: preview === 'error' ? 'error' : { target: preview.rollback_target }, + rollbackTarget: preview === 'error' + ? 'error' + : { + target: preview.rollback_target, + // Any image on a moving tag means restoring files cannot guarantee + // the image reverts, so the rollback target is not a true revert. + moving: preview.images.some(img => isMovingTag(img.current_tag)), + }, lastDeployAt, containers, }, now); diff --git a/backend/src/services/UpdatePreviewService.ts b/backend/src/services/UpdatePreviewService.ts index 507449fb..56f41a9f 100644 --- a/backend/src/services/UpdatePreviewService.ts +++ b/backend/src/services/UpdatePreviewService.ts @@ -76,6 +76,17 @@ export function parseSemverTag(tag: string): SemverParts | null { }; } +/** + * A tag is "moving" when restoring the compose file would not revert the image + * behind it: `latest`, a branch name, or an unpinned major/minor like `1.25`. + * Only a fully-pinned semver tag (X.Y.Z, optionally `v`-prefixed and/or with a + * `-prerelease` suffix) is treated as immutable, matching how a file rollback + * restores the exact tag. + */ +export function isMovingTag(tag: string): boolean { + return parseSemverTag(tag) === null; +} + function compareSemver(a: SemverParts, b: SemverParts): number { if (a.major !== b.major) return a.major - b.major; if (a.minor !== b.minor) return a.minor - b.minor; diff --git a/backend/src/services/WebhookService.ts b/backend/src/services/WebhookService.ts index cc71a811..df4b4ae1 100644 --- a/backend/src/services/WebhookService.ts +++ b/backend/src/services/WebhookService.ts @@ -1,5 +1,6 @@ import crypto from 'crypto'; import { ComposeService } from './ComposeService'; +import { StackOpLockService, stackOpSkipMessage, type StackOpAction } from './StackOpLockService'; import { DatabaseService, type Webhook } from './DatabaseService'; import { FileSystemService } from './FileSystemService'; import { GitSourceService } from './GitSourceService'; @@ -17,6 +18,16 @@ type ExecutionStatus = 'success' | 'failure'; const REMOTE_WEBHOOK_REQUEST_TIMEOUT_MS = 30_000; +// Maps a webhook lifecycle action to the per-stack lock action. 'pull' updates, +// so it locks as 'update'; 'git-pull' is excluded (it locks inside GitSourceService). +const WEBHOOK_LOCK_ACTION: Record = { + deploy: 'deploy', + restart: 'restart', + stop: 'stop', + start: 'start', + pull: 'update', +}; + export class WebhookService { private static instance: WebhookService; // Stable per-process decoy secret used to keep HMAC work non-skippable on @@ -125,42 +136,57 @@ export class WebhookService { const startTime = Date.now(); try { - const compose = ComposeService.getInstance(nodeId); - switch (action) { - case 'deploy': - await assertPolicyGateAllows( - stackName, - nodeId, - buildSystemPolicyGateOptions('webhook', { auditPath: `/api/webhooks/${webhookId}/execute` }), - ); - await compose.deployStack(stackName, undefined, atomic); - HealthGateService.getInstance().begin(nodeId, stackName, 'deploy', 'system:webhook'); - break; - case 'restart': - await compose.runCommand(stackName, 'restart'); - break; - case 'stop': - await compose.runCommand(stackName, 'stop'); - break; - case 'start': - await compose.runCommand(stackName, 'start'); - break; - case 'pull': - await assertPolicyGateAllows( - stackName, - nodeId, - buildSystemPolicyGateOptions('webhook', { auditPath: `/api/webhooks/${webhookId}/execute` }), - ); - await compose.updateStack(stackName, undefined, atomic); - HealthGateService.getInstance().begin(nodeId, stackName, 'update', 'system:webhook'); - break; - case 'git-pull': - return this.executeLocalGitPull(webhookId, stackName, action, triggerSource, startTime); - default: - throw new Error(`Unknown action: ${action}`); + // git-pull pulls then deploys through GitSourceService, which holds + // the per-stack lock itself; locking here too would self-conflict. + if (action === 'git-pull') { + return this.executeLocalGitPull(webhookId, stackName, action, triggerSource, startTime); } + const lockAction = WEBHOOK_LOCK_ACTION[action]; + if (!lockAction) throw new Error(`Unknown action: ${action}`); + const compose = ComposeService.getInstance(nodeId); + // Run the lifecycle op under the per-stack lock so a webhook cannot + // race a manual deploy/update/rollback/backup on the same stack. + const lock = await StackOpLockService.getInstance().runExclusive( + nodeId, stackName, lockAction, 'system', + async () => { + switch (action) { + case 'deploy': + await assertPolicyGateAllows( + stackName, + nodeId, + buildSystemPolicyGateOptions('webhook', { auditPath: `/api/webhooks/${webhookId}/execute` }), + ); + await compose.deployStack(stackName, undefined, atomic); + HealthGateService.getInstance().begin(nodeId, stackName, 'deploy', 'system:webhook'); + break; + case 'restart': + await compose.runCommand(stackName, 'restart'); + break; + case 'stop': + await compose.runCommand(stackName, 'stop'); + break; + case 'start': + await compose.runCommand(stackName, 'start'); + break; + case 'pull': + await assertPolicyGateAllows( + stackName, + nodeId, + buildSystemPolicyGateOptions('webhook', { auditPath: `/api/webhooks/${webhookId}/execute` }), + ); + await compose.updateStack(stackName, undefined, atomic); + HealthGateService.getInstance().begin(nodeId, stackName, 'update', 'system:webhook'); + break; + } + }, + ); const durationMs = Date.now() - startTime; + if (!lock.ran) { + const error = stackOpSkipMessage(stackName, lock.existing.action); + this.recordExecution(webhookId, action, 'failure', triggerSource, durationMs, error); + return { success: false, error, duration_ms: durationMs }; + } this.recordExecution(webhookId, action, 'success', triggerSource, durationMs, null); return { success: true, duration_ms: durationMs }; } catch (err) { diff --git a/backend/src/services/updateGuard/readiness.ts b/backend/src/services/updateGuard/readiness.ts index d3aedb8c..7b27994f 100644 --- a/backend/src/services/updateGuard/readiness.ts +++ b/backend/src/services/updateGuard/readiness.ts @@ -188,9 +188,12 @@ export interface RollbackInputs { /** * UpdatePreview.rollback_target wrapped in an object so the Errored sentinel * cannot be absorbed into the string domain (an image literally named - * "error" must not read as a failed preview). + * "error" must not read as a failed preview). `moving` is true when any image + * in the stack uses a moving tag (latest, a branch, an unpinned major/minor), + * in which case restoring files does not revert the image because the local + * tag already resolves to the newer digest. */ - rollbackTarget: { target: string | null } | Errored; + rollbackTarget: { target: string | null; moving: boolean } | Errored; /** Timestamp of the most recent deploy_success activity event, if any. */ lastDeployAt: number | null | Errored; containers: ContainerProbe[] | Errored; @@ -224,8 +227,10 @@ export function buildRollbackItems(inputs: RollbackInputs, now: number): Rollbac if (inputs.rollbackTarget === 'error') { items.push({ id: 'previous_images', state: 'unknown', label: 'Previous image tag', detail: 'The update preview is unavailable.' }); + } else if (inputs.rollbackTarget.target && inputs.rollbackTarget.moving) { + items.push({ id: 'previous_images', state: 'not_covered', label: 'Previous image tag', detail: `Rollback target ${inputs.rollbackTarget.target}. This stack uses a moving image tag, so restoring the compose and env files does not revert the image: the local tag still resolves to the newer digest. Pin every image to an immutable version tag for a true image rollback.` }); } else if (inputs.rollbackTarget.target) { - items.push({ id: 'previous_images', state: 'ready', label: 'Previous image tag', detail: `Known rollback target: ${inputs.rollbackTarget.target}. If the compose file uses a moving tag, restoring files alone does not revert the image; pin this tag to be exact.` }); + items.push({ id: 'previous_images', state: 'ready', label: 'Previous image tag', detail: `Known rollback target: ${inputs.rollbackTarget.target}. The compose file pins an immutable tag, so restoring files also restores the image.` }); } else { items.push({ id: 'previous_images', state: 'unknown', label: 'Previous image tag', detail: 'The previous image tag could not be determined. A rollback restores compose and env files; a moving tag may keep the newer image.' }); } diff --git a/docs/features/atomic-deployments.mdx b/docs/features/atomic-deployments.mdx index 265cc072..573e99f8 100644 --- a/docs/features/atomic-deployments.mdx +++ b/docs/features/atomic-deployments.mdx @@ -12,7 +12,7 @@ The same backup also powers the **Rollback** action in the stack editor, so you 1. **Backup.** Before the action runs, Sencho copies `compose.yaml` (or `compose.yml` / `docker-compose.yaml` / `docker-compose.yml`) and `.env`, if present, into the backup directory. The deploy progress modal streams `=== Backup created for atomic deployment ===` once the copy completes, before any `docker compose` output. 2. **Run the action.** Sencho executes the requested compose action: `up -d` for a deploy, or a pull-then-`up -d` recreate for an update. 3. **Health probe.** Sencho waits 3 seconds, then lists every container with the `com.docker.compose.project=` label and checks each one for a non-zero exit code. Any container that has exited with a non-zero status counts as a crash. -4. **Auto-rollback on failure.** When a crash is detected, Sencho streams `=== Deployment failed - rolling back to previous version ===`, restores the backed-up files, and re-runs `docker compose up -d` with the restored configuration. On success it streams `=== Rolled back successfully ===`. The original deploy error is preserved and reported as the deploy result, so a failed-then-rolled-back deploy still registers as a failure in the deploy progress modal. +4. **Auto-rollback on failure.** When a crash is detected, Sencho streams `=== Deployment failed - restoring previous compose and env files ===`, restores the backed-up files, and re-runs `docker compose up -d` with the restored configuration. On success it streams `=== Restored previous compose and env files ===`. Restoring the files reverts the compose and `.env` configuration; an image referenced by a moving tag (such as `latest`) is not reverted, because the local tag still resolves to the newly pulled digest. The original deploy error is preserved and reported as the deploy result, so a failed-then-rolled-back deploy still registers as a failure in the deploy progress modal. If the rollback itself fails (for example, the re-deploy step cannot pull a previously available image, or the file restore is blocked by filesystem permissions), Sencho streams `=== Rollback failed - manual intervention may be required ===`. The backup files remain at `/backups//` so you can copy them back manually. diff --git a/docs/openapi.yaml b/docs/openapi.yaml index 5ef81ea0..5f7e77bb 100644 --- a/docs/openapi.yaml +++ b/docs/openapi.yaml @@ -1174,7 +1174,7 @@ paths: schema: $ref: "#/components/schemas/SuccessMessage" example: - message: Stack rolled back successfully. + message: 'Stack rolled back: compose and env files restored.' "403": $ref: "#/components/responses/Forbidden" "404": diff --git a/frontend/package-lock.json b/frontend/package-lock.json index 1b588b5e..6eb07537 100644 --- a/frontend/package-lock.json +++ b/frontend/package-lock.json @@ -6792,9 +6792,9 @@ } }, "node_modules/undici": { - "version": "7.25.0", - "resolved": "https://registry.npmjs.org/undici/-/undici-7.25.0.tgz", - "integrity": "sha512-xXnp4kTyor2Zq+J1FfPI6Eq3ew5h6Vl0F/8d9XU5zZQf1tX9s2Su1/3PiMmUANFULpmksxkClamIZcaUqryHsQ==", + "version": "7.28.0", + "resolved": "https://registry.npmjs.org/undici/-/undici-7.28.0.tgz", + "integrity": "sha512-cRZYrTDwWznlnRiPjggAGxZXanty6M8RV1ff8Wm4LWXBp7/IG8v5DnOm74DtUBp9OONpK75YlPnIjQqX0dBDtA==", "dev": true, "license": "MIT", "engines": { diff --git a/frontend/src/components/EditorLayout/RecoveryChip.tsx b/frontend/src/components/EditorLayout/RecoveryChip.tsx index d1e077b9..c9b1ebe2 100644 --- a/frontend/src/components/EditorLayout/RecoveryChip.tsx +++ b/frontend/src/components/EditorLayout/RecoveryChip.tsx @@ -62,7 +62,7 @@ export function RecoveryChip({

{result.errorMessage ?? 'The operation did not complete.'} - {result.rolledBack && ' · rolled back to previous version'} + {result.rolledBack && ' · restored previous compose and env files'}

diff --git a/frontend/src/components/EditorLayout/RecoveryPanel.tsx b/frontend/src/components/EditorLayout/RecoveryPanel.tsx index 974e7be4..362024a0 100644 --- a/frontend/src/components/EditorLayout/RecoveryPanel.tsx +++ b/frontend/src/components/EditorLayout/RecoveryPanel.tsx @@ -63,7 +63,7 @@ export function RecoveryPanel({

{result.errorMessage ?? 'The operation did not complete.'} - {result.rolledBack && ' · rolled back to previous version'} + {result.rolledBack && ' · restored previous compose and env files'}

diff --git a/frontend/src/components/EditorLayout/ShellOverlays.tsx b/frontend/src/components/EditorLayout/ShellOverlays.tsx index 9d21815f..9ece1c16 100644 --- a/frontend/src/components/EditorLayout/ShellOverlays.tsx +++ b/frontend/src/components/EditorLayout/ShellOverlays.tsx @@ -96,6 +96,7 @@ export function ShellOverlays({ setUpdateReadiness(null)} onProceed={() => updateReadiness?.proceed()} /> diff --git a/frontend/src/components/EditorLayout/hooks/useOverlayState.ts b/frontend/src/components/EditorLayout/hooks/useOverlayState.ts index 4214cca4..7297ce3c 100644 --- a/frontend/src/components/EditorLayout/hooks/useOverlayState.ts +++ b/frontend/src/components/EditorLayout/hooks/useOverlayState.ts @@ -18,6 +18,10 @@ type PolicyBlock = { stackFile: string; action: PolicyBlockableAction; payload: PolicyBlockPayload; + // Node captured when the block was raised, so a bypass retry (deploy, update, + // or rollback) targets that node even if the active node changes while the + // dialog is open. + nodeId: number | null; }; type Container = { id: string; name: string }; @@ -90,10 +94,13 @@ export function useOverlayState() { const [policyBypassing, setPolicyBypassing] = useState(false); // Pre-update readiness dialog. `proceed` runs the actual update when the - // user confirms; opened by useStackActions.requestStackUpdate. + // user confirms; opened by useStackActions.requestStackUpdate. `nodeId` is + // captured at open time so both the readiness fetch and the update run against + // the same node even if the active node changes while the dialog is open. const [updateReadiness, setUpdateReadiness] = useState<{ stackName: string; stackFile: string; + nodeId: number | null; proceed: () => void; } | null>(null); diff --git a/frontend/src/components/EditorLayout/hooks/useStackActions.test.ts b/frontend/src/components/EditorLayout/hooks/useStackActions.test.ts index e9010960..a928a5e1 100644 --- a/frontend/src/components/EditorLayout/hooks/useStackActions.test.ts +++ b/frontend/src/components/EditorLayout/hooks/useStackActions.test.ts @@ -513,6 +513,19 @@ describe('useStackActions.bypassPolicyAndRetry', () => { expect(urls).toContain('/stacks/web.yml/rollback?ignorePolicy=true'); }); + it('retries on the node captured in the policy block, not the live active node', async () => { + vi.mocked(apiFetch).mockResolvedValueOnce(new Response(null, { status: 200 })); // update OK + vi.mocked(apiFetch).mockResolvedValueOnce(new Response('[]', { status: 200 })); // containers refresh + const { result } = setup({ + activeNode: { id: 1, type: 'local' } as never, // active node has since moved to 1 + overlay: { policyBlock: { stackName: 'web', stackFile: 'web.yml', action: 'update', payload, nodeId: 9 } as never }, + }); + await result.current.bypassPolicyAndRetry(); + const updateCall = vi.mocked(apiFetch).mock.calls.find(c => String(c[0]).includes('/update?ignorePolicy=true')); + expect(updateCall).toBeDefined(); + expect((updateCall![1] as { nodeId?: number | null }).nodeId).toBe(9); + }); + it('does nothing when no policy block is stored', async () => { const { result } = setup({ overlay: { policyBlock: null as never } }); await result.current.bypassPolicyAndRetry(); diff --git a/frontend/src/components/EditorLayout/hooks/useStackActions.ts b/frontend/src/components/EditorLayout/hooks/useStackActions.ts index 65b8b92c..6676892a 100644 --- a/frontend/src/components/EditorLayout/hooks/useStackActions.ts +++ b/frontend/src/components/EditorLayout/hooks/useStackActions.ts @@ -660,14 +660,16 @@ export function useStackActions(options: UseStackActionsOptions) { }; // Parse a 409 body for a scan-policy block. When it is one, record it (with - // the originating action and file so the bypass retries the right endpoint) - // so PolicyBlockDialog can open, and return the policy name. Returns null - // when the body is not a policy block (e.g. a stack-op-in-progress 409). + // the originating action, file, and the node the operation targeted so the + // bypass retries the right endpoint on the right node) so PolicyBlockDialog + // can open, and return the policy name. Returns null when the body is not a + // policy block (e.g. a stack-op-in-progress 409). const tryOpenPolicyBlock = ( rawBody: string, stackName: string, stackFile: string, action: PolicyBlockableAction, + opNodeId: number | null, ): string | null => { let parsed: PolicyBlockPayload | null = null; try { @@ -676,7 +678,7 @@ export function useStackActions(options: UseStackActionsOptions) { /* not JSON */ } if (parsed && parsed.policy && Array.isArray(parsed.violations)) { - overlayState.setPolicyBlock({ stackName, stackFile, action, payload: parsed }); + overlayState.setPolicyBlock({ stackName, stackFile, action, payload: parsed, nodeId: opNodeId }); return parsed.policy.name; } return null; @@ -712,7 +714,7 @@ export function useStackActions(options: UseStackActionsOptions) { toast.error(message); return { ok: false, errorMessage: message }; } - const blockedBy = tryOpenPolicyBlock(rawBody, stackName, stackFile, 'deploy'); + const blockedBy = tryOpenPolicyBlock(rawBody, stackName, stackFile, 'deploy', opNodeId ?? null); if (blockedBy) { const message = `Deploy blocked by policy "${blockedBy}"`; toast.error(message); @@ -747,7 +749,7 @@ export function useStackActions(options: UseStackActionsOptions) { const errorMessage = deployError.message || 'Failed to deploy stack'; toast.error( deployError.rolledBack === true - ? `${errorMessage} - automatically rolled back to previous version.` + ? `${errorMessage} - automatically restored the previous compose and env files.` : errorMessage, ); recordActionFailureFor(stackFile, stackName, 'deploy', startedAt, errorMessage, deployError.rolledBack === true, deployError.failure); @@ -831,19 +833,20 @@ export function useStackActions(options: UseStackActionsOptions) { const bypassPolicyAndRetry = async () => { const policyBlock = overlayState.policyBlock; if (!policyBlock) return; - const { stackName, stackFile, action } = policyBlock; + // Retry on the node the block was raised against, not the live active node, + // which may have changed while the dialog was open. + const { stackName, stackFile, action, nodeId: opNodeId } = policyBlock; const existingFile = stackListState.files.includes(stackFile) ? stackFile : (stackListState.files.find(f => f.replace(/\.(yml|yaml)$/, '') === stackName) ?? stackFile); overlayState.setPolicyBypassing(true); try { if (action === 'update') { - await runStackAction(existingFile, 'update', 'update', 'running', 'Stack updated successfully!', true); + await runStackAction(existingFile, 'update', 'update', 'running', 'Stack updated successfully!', true, opNodeId); } else if (action === 'rollback') { - await rollbackStack(true); + await rollbackStack(true, opNodeId); } else { stackListState.setStackAction(existingFile, 'deploy'); - const opNodeId = activeNode?.id ?? null; try { await runWithLog({ stackName, action: 'deploy', nodeId: opNodeId }, (started, ds) => runDeploy(stackName, existingFile, true, started, ds, opNodeId), @@ -858,7 +861,7 @@ export function useStackActions(options: UseStackActionsOptions) { } }; - const rollbackStack = async (ignorePolicy = false) => { + const rollbackStack = async (ignorePolicy = false, opNodeId: number | null = activeNode?.id ?? null) => { if (!stackListState.selectedFile || stackListState.isStackBusy(stackListState.selectedFile)) return; const stackFile = stackListState.selectedFile; @@ -870,7 +873,7 @@ export function useStackActions(options: UseStackActionsOptions) { const path = ignorePolicy ? `/stacks/${stackFile}/rollback?ignorePolicy=true` : `/stacks/${stackFile}/rollback`; - const res = await apiFetch(path, { method: 'POST' }); + const res = await apiFetch(path, { method: 'POST', nodeId: opNodeId }); if (!res.ok) { const rawBody = await res.text(); if (res.status === 409) { @@ -880,7 +883,7 @@ export function useStackActions(options: UseStackActionsOptions) { toast.error(message); return; } - const blockedBy = tryOpenPolicyBlock(rawBody, stackName, stackFile, 'rollback'); + const blockedBy = tryOpenPolicyBlock(rawBody, stackName, stackFile, 'rollback', opNodeId); if (blockedBy) { toast.error(`Rollback blocked by policy "${blockedBy}"`); return; @@ -889,7 +892,7 @@ export function useStackActions(options: UseStackActionsOptions) { throw parseStackActionError(rawBody, 'Rollback failed', res.status); } overlayState.setPolicyBlock(null); - toast.success('Stack rolled back successfully.'); + toast.success('Stack rolled back: compose and env files restored.'); stackListState.recordActionSuccess(stackFile); // The rollback already succeeded; a failure of the cosmetic refetches below // (containers redeployed by the rollback, restored compose content, backup @@ -968,6 +971,11 @@ export function useStackActions(options: UseStackActionsOptions) { optimisticStatus: 'running' | 'exited', successMessage: string, ignorePolicy = false, + // Node the operation targets. Defaults to the live active node for direct + // callers (toolbar stop/restart); the update path passes the node captured + // when its readiness dialog opened so a mid-dialog node switch cannot + // retarget the update. + opNodeId: number | null = activeNode?.id ?? null, ): Promise => { if (stackListState.isStackBusy(stackFile)) return; const stackName = stackFile.replace(/\.(yml|yaml)$/, ''); @@ -975,9 +983,6 @@ export function useStackActions(options: UseStackActionsOptions) { const startedAt = Date.now(); stackListState.setStackAction(stackFile, action); stackListState.setOptimisticStatus(stackFile, optimisticStatus); - // Snapshot the node once so stop/restart/update stays bound to it even if - // the active node changes while the operation is in flight. - const opNodeId = activeNode?.id ?? null; try { await runWithLog({ stackName, action, nodeId: opNodeId }, async (started, ds) => { await started; @@ -996,7 +1001,7 @@ export function useStackActions(options: UseStackActionsOptions) { return { ok: false as const, errorMessage: message }; } if (action === 'update') { - const blockedBy = tryOpenPolicyBlock(errText, stackName, stackFile, 'update'); + const blockedBy = tryOpenPolicyBlock(errText, stackName, stackFile, 'update', opNodeId); if (blockedBy) { const message = `Update blocked by policy "${blockedBy}"`; toast.error(message); @@ -1091,11 +1096,15 @@ export function useStackActions(options: UseStackActionsOptions) { const requestStackUpdate = async (stackFile: string): Promise => { if (stackListState.isStackBusy(stackFile)) return; const stackName = stackFile.replace(/\.(yml|yaml)$/, ''); - const run = () => runStackAction(stackFile, 'update', 'update', 'running', 'Stack updated successfully!'); + // Capture the node now so the readiness fetch and the update both target it + // even if the active node changes while the readiness dialog is open. + const opNodeId = activeNode?.id ?? null; + const run = () => runStackAction(stackFile, 'update', 'update', 'running', 'Stack updated successfully!', false, opNodeId); if (hasUpdateGuard) { overlayState.setUpdateReadiness({ stackName, stackFile, + nodeId: opNodeId, proceed: () => { overlayState.setUpdateReadiness(null); void run(); @@ -1208,6 +1217,9 @@ export function useStackActions(options: UseStackActionsOptions) { } const stackName = stackFile.replace(/\.(yml|yaml)$/, ''); const startedAt = Date.now(); + // Bind this sidebar action to the active node now so a policy-block bypass + // retries on the same node even if the active node changes meanwhile. + const opNodeId = activeNode?.id ?? null; stackListState.setStackAction(stackFile, action); if (action === 'stop') { @@ -1217,7 +1229,7 @@ export function useStackActions(options: UseStackActionsOptions) { } try { - const response = await apiFetch(`/stacks/${stackName}/${endpoint}`, { method: 'POST' }); + const response = await apiFetch(`/stacks/${stackName}/${endpoint}`, { method: 'POST', nodeId: opNodeId }); if (!response.ok) { const errText = await response.text(); if (response.status === 409) { @@ -1227,7 +1239,7 @@ export function useStackActions(options: UseStackActionsOptions) { return; } if (action === 'deploy') { - const blockedBy = tryOpenPolicyBlock(errText, stackName, stackFile, action); + const blockedBy = tryOpenPolicyBlock(errText, stackName, stackFile, action, opNodeId); if (blockedBy) { toast.error(`Deploy blocked by policy "${blockedBy}"`); return; @@ -1253,7 +1265,7 @@ export function useStackActions(options: UseStackActionsOptions) { const msg = actionError.message || `Failed to ${action} stack`; toast.error( action === 'deploy' && actionError.rolledBack === true - ? `${msg} - automatically rolled back to previous version.` + ? `${msg} - automatically restored the previous compose and env files.` : msg, ); recordActionFailureFor(stackFile, stackName, action, startedAt, msg, actionError.rolledBack === true, actionError.failure); diff --git a/frontend/src/components/EditorLayout/recovery-format.ts b/frontend/src/components/EditorLayout/recovery-format.ts index 67164f2f..0fb00ae0 100644 --- a/frontend/src/components/EditorLayout/recovery-format.ts +++ b/frontend/src/components/EditorLayout/recovery-format.ts @@ -27,7 +27,7 @@ export function buildDiagnostics( `Stack: ${stackName}`, `Node: ${activeNode?.name ?? 'local'}${activeNode?.id != null ? ` (id ${activeNode.id})` : ''}`, `Action: ${result.action}`, - `Outcome: failed${result.rolledBack ? ' (rolled back to previous version)' : ''}`, + `Outcome: failed${result.rolledBack ? ' (restored previous compose and env files)' : ''}`, `Elapsed: ${formatElapsed(result.endedAt - result.startedAt)}`, `Error: ${result.errorMessage ?? 'unknown'}`, `Backup: ${backupInfo.exists diff --git a/frontend/src/components/stack/UpdateReadinessDialog.tsx b/frontend/src/components/stack/UpdateReadinessDialog.tsx index 227f84c4..13222795 100644 --- a/frontend/src/components/stack/UpdateReadinessDialog.tsx +++ b/frontend/src/components/stack/UpdateReadinessDialog.tsx @@ -9,7 +9,6 @@ import { toast } from '@/components/ui/toast-store'; import { apiFetch } from '@/lib/api'; import { cn } from '@/lib/utils'; import { formatTimeAgo } from '@/lib/relativeTime'; -import { useNodes } from '@/context/NodeContext'; import { useAuth } from '@/context/AuthContext'; // Mirrors the backend payload shape (the frontend never imports backend). @@ -92,6 +91,12 @@ const UNKNOWN_FALLBACK = (detail: string): UpdateReadinessReport => ({ interface UpdateReadinessDialogProps { open: boolean; stackName: string; + /** + * Node captured when the dialog opened. The readiness fetch and snapshot + * coverage run against this node, not the live active node, so a node switch + * while the dialog is open cannot mismatch the readiness from the update. + */ + nodeId: number | null; onCancel: () => void; /** Caller closes the dialog and starts the update. */ onProceed: () => void; @@ -103,10 +108,8 @@ interface UpdateReadinessDialogProps { * the single hard block. A slow or failed readiness fetch degrades to an * unknown verdict so this dialog can never strand the update path. */ -export function UpdateReadinessDialog({ open, stackName, onCancel, onProceed }: UpdateReadinessDialogProps) { - const { activeNode } = useNodes(); +export function UpdateReadinessDialog({ open, stackName, nodeId, onCancel, onProceed }: UpdateReadinessDialogProps) { const { isAdmin } = useAuth(); - const nodeId = activeNode?.id ?? null; const [report, setReport] = useState(null); const [snapshotAt, setSnapshotAt] = useState(null); @@ -133,7 +136,7 @@ export function UpdateReadinessDialog({ open, stackName, onCancel, onProceed }: const load = async () => { try { - const res = await apiFetch(`/stacks/${stackName}/update-readiness`, { signal: controller.signal }); + const res = await apiFetch(`/stacks/${stackName}/update-readiness`, { nodeId, signal: controller.signal }); if (!res.ok) { const unreachable = res.status === 502 || res.status === 503 || res.status === 504; setReport(UNKNOWN_FALLBACK(unreachable diff --git a/frontend/src/components/stack/__tests__/UpdateReadinessDialog.test.tsx b/frontend/src/components/stack/__tests__/UpdateReadinessDialog.test.tsx index a0c5bf3b..2aa1e2be 100644 --- a/frontend/src/components/stack/__tests__/UpdateReadinessDialog.test.tsx +++ b/frontend/src/components/stack/__tests__/UpdateReadinessDialog.test.tsx @@ -7,11 +7,6 @@ vi.mock('@/components/ui/toast-store', () => ({ toast: { success: vi.fn(), error: vi.fn(), info: vi.fn() }, })); -const nodesState = { activeNode: { id: 1, type: 'local', name: 'local' } }; -vi.mock('@/context/NodeContext', () => ({ - useNodes: () => nodesState, -})); - const authState = { isAdmin: true }; vi.mock('@/context/AuthContext', () => ({ useAuth: () => authState, @@ -55,6 +50,7 @@ function setup(props: Partial[0]> = {}) const base = { open: true, stackName: 'web', + nodeId: 1, onCancel: vi.fn(), onProceed: vi.fn(), ...props, @@ -86,6 +82,17 @@ describe('UpdateReadinessDialog', () => { expect(screen.getByTestId('readiness-proceed')).toBeEnabled(); }); + it('fetches readiness and coverage against the captured node, not the active node', async () => { + routeApi(); + setup({ nodeId: 7 }); + await waitFor(() => expect(screen.getByTestId('readiness-verdict')).toBeInTheDocument()); + const readinessCall = vi.mocked(apiFetch).mock.calls.find(c => String(c[0]).includes('/update-readiness')); + expect(readinessCall).toBeDefined(); + expect((readinessCall![1] as { nodeId?: number | null } | undefined)?.nodeId).toBe(7); + const coverageCall = vi.mocked(apiFetch).mock.calls.find(c => String(c[0]).includes('/snapshots/coverage')); + expect(String(coverageCall?.[0])).toContain('nodeId=7'); + }); + it('degrades to unknown on a plain network failure, and stays non-blocking', async () => { routeApi({ readiness: () => Promise.reject(new Error('connection refused')) }); const props = setup(); @@ -107,7 +114,7 @@ describe('UpdateReadinessDialog', () => { } return Promise.resolve(new Response(JSON.stringify({ latestAt: null }), { status: 200 })); }); - render(); + render(); await act(async () => { await vi.advanceTimersByTimeAsync(4_100); }); expect(screen.getByTestId('readiness-verdict')).toHaveAttribute('data-verdict', 'unknown'); expect(screen.getByText(/did not respond in time/)).toBeInTheDocument(); @@ -134,7 +141,7 @@ describe('UpdateReadinessDialog', () => { return Promise.resolve(new Response(JSON.stringify({ latestAt: null }), { status: 200 })); }); - const props = { stackName: 'web', onCancel: vi.fn(), onProceed: vi.fn() }; + const props = { stackName: 'web', nodeId: 1, onCancel: vi.fn(), onProceed: vi.fn() }; const { rerender } = render(); rerender(); rerender();