From 5f1baa752225d6d466e46e65dd4da858d08d6972 Mon Sep 17 00:00:00 2001 From: Anso Date: Thu, 18 Jun 2026 13:38:19 -0400 Subject: [PATCH] fix: harden deploy/update concurrency and node-targeting safety (#1390) * fix: harden deploy/update concurrency and node-targeting safety Release stabilization for deploy/update operational safety. Per-stack operation locking is now global. Background lifecycle paths (scheduler auto stop/down/start/backup/update, webhook execute, Git source auto-deploy, image auto-update, label bulk actions, fleet snapshot redeploy, and mesh redeploy) acquire the per-node, per-stack lock through a new StackOpLockService.runExclusive helper and skip rather than race a manual deploy/update/rollback/backup on the same stack and node. Skips surface honestly (a failed scheduled run, a recorded webhook failure, a per-stack batch result, or a thrown error) instead of a silent no-op. Update readiness and policy-bypass now run against the node captured when the dialog opened, not the live active node, so switching nodes while a dialog is open cannot retarget the update or the bypass retry. Rollback readiness no longer presents a moving-tag or unpinned image as a ready image revert. Restoring files does not revert a moving tag, so those stacks read as partial, and the rollback success message states that the compose and env files were restored. * fix: lock blueprint reconcile against manual ops and correct rollback wording Follow-up to the deploy/update safety hardening, closing two more gaps from a verification pass. BlueprintService.deployLocal and withdrawLocal called ComposeService directly, so blueprint reconciliation could race a manual deploy/update/rollback/backup on an owned stack. Both now run their compose lifecycle call through StackOpLockService.runExclusive and skip (recorded as a failed reconcile, retried on the next cycle) on conflict. The withdraw holds the lock across both the compose down and the directory delete so neither races a manual operation. The runtime rollback messages overstated recovery: a rollback restores the compose and env files and recreates containers, but does not revert an image behind a moving tag. The auto-rollback deploy-progress output, the recovery panel and chip, the failure toasts, and the manual rollback route message now state that the compose and env files were restored, with the matching OpenAPI example and atomic-deployments doc updated. * fix: acquire stack lock before blueprint deploy mutates compose and marker files Local blueprint deploy wrote the compose and marker files and ran the policy assert before acquiring the per-stack lock; the lock only wrapped the deploy itself. A reconcile could therefore rewrite an owned stack's files while a manual deploy/update/rollback/backup was running. The lock now wraps the whole critical section (create, write compose, write marker, policy assert, deploy), so on conflict nothing is written and the reconcile records a failed outcome. Adds a test asserting a deploy under a held lock records failed, writes no marker file, and leaves the manual lock untouched. * fix: make remote blueprint apply atomic under the receiving node's stack lock Remote blueprint deploy wrote the compose and marker files to the target node via separate HTTP calls and only locked on the final deploy, so the file writes could race a manual operation on that node. A node's operation lock is process-local and cannot be held by the hub across HTTP calls, so the locked create/write/deploy now runs on the receiving node. The locked critical section is extracted into BlueprintService.applyLocalUnderLock and exposed via POST /api/blueprints/apply-local. The hub posts the blueprint to that endpoint in one call; the receiving node runs create + write compose+marker + deploy under its own per-stack lock. Older nodes without the route answer 404 and fall back to the legacy multi-call flow. The endpoint is gated by paid tier and the same per-stack stack:edit and stack:deploy permissions as the PUT-compose + deploy it bundles, validates the stack name, compose size, and marker structure, and returns 409 on a lock conflict without writing anything. Adds tests for the atomic single-call path, the 404 legacy fallback, the 409 lock-conflict mapping, the route validation and permission paths, and the write-compose-then-marker-then-deploy ordering of the shared locked apply. * fix(deps): bump undici to 7.28.0 to clear high-severity advisory The frontend CI npm audit gate (--audit-level=high) failed on a transitive undici 7.25.0 (a dev-only dependency via jsdom): TLS certificate validation bypass (GHSA-vmh5-mc38-953g) and cross-user cache information disclosure (GHSA-pr7r-676h-xcf6). Bumping undici within jsdom's existing ^7.25.0 range to 7.28.0 clears the high-severity advisory and unblocks the frontend job. Lockfile only; no direct dependency or source change. --- .../blueprints-remote-deploy.test.ts | 66 +++++---- .../blueprints-route-validation.test.ts | 49 +++++++ backend/src/__tests__/blueprints.test.ts | 64 +++++++++ .../src/__tests__/labels-bulk-actions.test.ts | 24 ++++ .../src/__tests__/scheduler-service.test.ts | 15 ++ .../__tests__/stack-op-lock-service.test.ts | 55 +++++++- .../__tests__/update-guard-rollback.test.ts | 20 ++- .../__tests__/update-guard-service.test.ts | 49 ++++++- .../__tests__/update-preview-service.test.ts | 16 +++ backend/src/routes/blueprints.ts | 48 +++++++ backend/src/routes/fleet.ts | 9 +- backend/src/routes/imageUpdates.ts | 10 +- backend/src/routes/labels.ts | 33 ++++- backend/src/routes/stacks.ts | 2 +- backend/src/services/BlueprintService.ts | 128 +++++++++++++++--- backend/src/services/ComposeService.ts | 6 +- backend/src/services/GitSourceService.ts | 11 +- backend/src/services/MeshService.ts | 9 +- backend/src/services/SchedulerService.ts | 37 ++++- backend/src/services/StackOpLockService.ts | 36 +++++ backend/src/services/UpdateGuardService.ts | 11 +- backend/src/services/UpdatePreviewService.ts | 11 ++ backend/src/services/WebhookService.ts | 92 ++++++++----- backend/src/services/updateGuard/readiness.ts | 11 +- docs/features/atomic-deployments.mdx | 2 +- docs/openapi.yaml | 2 +- frontend/package-lock.json | 6 +- .../components/EditorLayout/RecoveryChip.tsx | 2 +- .../components/EditorLayout/RecoveryPanel.tsx | 2 +- .../components/EditorLayout/ShellOverlays.tsx | 1 + .../EditorLayout/hooks/useOverlayState.ts | 9 +- .../hooks/useStackActions.test.ts | 13 ++ .../EditorLayout/hooks/useStackActions.ts | 56 +++++--- .../EditorLayout/recovery-format.ts | 2 +- .../stack/UpdateReadinessDialog.tsx | 13 +- .../__tests__/UpdateReadinessDialog.test.tsx | 21 ++- 36 files changed, 785 insertions(+), 156 deletions(-) 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();