From b3bc223c45b9e8c22194e4ca2cb0109bb2aba264 Mon Sep 17 00:00:00 2001 From: Anso Date: Sun, 5 Jul 2026 21:55:32 -0400 Subject: [PATCH] fix(deploy): selective compose recreate on Save and Deploy (#1565) (#1568) * feat: move core Blueprint orchestration to Community tier Blueprints CRUD, reconciliation, and drift modes are now available on Community. Pin remains Admiral-only via Federation placement controls. * test: update NodeCard cordon tests for Admiral-only tier gate Cordon now requires both isPaid and node:manage permission, matching the backend requirePaid + requirePermission guard. Three tests still used isPaid:false but expected the menu to be visible. --- backend/src/__tests__/compose-service.test.ts | 50 ++++++- .../src/__tests__/docker-controller.test.ts | 42 ++++++ backend/src/services/ComposeService.ts | 8 +- backend/src/services/DockerController.ts | 124 +++++++++++------- .../FleetView/__tests__/NodeCard.test.tsx | 9 +- 5 files changed, 175 insertions(+), 58 deletions(-) diff --git a/backend/src/__tests__/compose-service.test.ts b/backend/src/__tests__/compose-service.test.ts index 39c49920..ac8a029d 100644 --- a/backend/src/__tests__/compose-service.test.ts +++ b/backend/src/__tests__/compose-service.test.ts @@ -12,7 +12,7 @@ import type WebSocket from 'ws'; const { mockSpawn, - mockGetContainersByStack, mockRemoveContainers, mockListContainers, + mockGetContainersByStack, mockGetLegacyOrphanContainersByStack, mockRemoveContainers, mockListContainers, mockContainerInspect, mockContainerLogs, mockGetRegistries, mockResolveDockerConfig, mockBackupStackFiles, mockRestoreStackFiles, @@ -24,6 +24,7 @@ const { } = vi.hoisted(() => ({ mockSpawn: vi.fn(), mockGetContainersByStack: vi.fn().mockResolvedValue([]), + mockGetLegacyOrphanContainersByStack: vi.fn().mockResolvedValue([]), mockRemoveContainers: vi.fn().mockResolvedValue([]), mockListContainers: vi.fn().mockResolvedValue([]), mockContainerInspect: vi.fn().mockResolvedValue({ State: { ExitCode: 0 } }), @@ -75,6 +76,7 @@ vi.mock('../services/DockerController', () => ({ default: { getInstance: () => ({ getContainersByStack: mockGetContainersByStack, + getLegacyOrphanContainersByStack: mockGetLegacyOrphanContainersByStack, removeContainers: mockRemoveContainers, pruneDanglingImages: mockPruneDanglingImages, getDocker: () => ({ @@ -588,7 +590,7 @@ describe('ComposeService - deployStack', () => { const error = await result; expect(error?.message).toContain('1:1'); - expect(mockGetContainersByStack).not.toHaveBeenCalled(); + expect(mockGetLegacyOrphanContainersByStack).not.toHaveBeenCalled(); expect(mockRemoveContainers).not.toHaveBeenCalled(); expect(mockSpawn).toHaveBeenCalledTimes(1); }); @@ -629,7 +631,47 @@ describe('ComposeService - deployStack', () => { await vi.advanceTimersByTimeAsync(3100); await expect(promise).resolves.toBeUndefined(); - expect(mockGetContainersByStack).toHaveBeenCalledWith('my-stack'); + expect(mockGetLegacyOrphanContainersByStack).toHaveBeenCalledWith('my-stack'); + }); + + it('does not remove compose-managed containers before deploy (#1565)', async () => { + setupAutoCloseSpawn(); + mockListContainers.mockResolvedValue([]); + mockGetLegacyOrphanContainersByStack.mockResolvedValue([]); + + const svc = ComposeService.getInstance(1); + const promise = svc.deployStack('my-stack'); + await vi.advanceTimersByTimeAsync(3100); + await promise; + + expect(mockGetLegacyOrphanContainersByStack).toHaveBeenCalledWith('my-stack'); + expect(mockRemoveContainers).not.toHaveBeenCalled(); + expect(mockSpawn).toHaveBeenCalledWith( + 'docker', + ['compose', 'up', '-d', '--remove-orphans'], + expect.any(Object), + ); + }); + + it('removes legacy orphan containers when compose ps is empty', async () => { + setupAutoCloseSpawn(); + mockListContainers.mockResolvedValue([]); + mockGetLegacyOrphanContainersByStack.mockResolvedValue([ + { Id: 'legacy-c1' }, + { Id: 'legacy-c2' }, + ]); + + const svc = ComposeService.getInstance(1); + const promise = svc.deployStack('my-stack'); + await vi.advanceTimersByTimeAsync(3100); + await promise; + + expect(mockRemoveContainers).toHaveBeenCalledWith(['legacy-c1', 'legacy-c2']); + expect(mockSpawn).toHaveBeenCalledWith( + 'docker', + ['compose', 'up', '-d', '--remove-orphans'], + expect.any(Object), + ); }); it('runs docker compose up -d --remove-orphans', async () => { @@ -672,7 +714,7 @@ describe('ComposeService - deployStack', () => { 'Atomic deployment backup failed', ); expect(mockSpawn).not.toHaveBeenCalled(); - expect(mockGetContainersByStack).not.toHaveBeenCalled(); + expect(mockGetLegacyOrphanContainersByStack).not.toHaveBeenCalled(); }); it('throws sanitized CONTAINER_CRASHED when exited container has non-zero exit code', async () => { diff --git a/backend/src/__tests__/docker-controller.test.ts b/backend/src/__tests__/docker-controller.test.ts index ff3544f0..bfa8d3ca 100644 --- a/backend/src/__tests__/docker-controller.test.ts +++ b/backend/src/__tests__/docker-controller.test.ts @@ -31,6 +31,7 @@ vi.mock('../services/NodeRegistry', () => ({ getInstance: () => ({ getDocker: () => mockDocker, getDefaultNodeId: () => 1, + getComposeDir: () => '/test/compose', }), }, })); @@ -1567,3 +1568,44 @@ describe('DockerController - getBulkStackStatuses partial status', () => { expect(result['empty-stack'].total).toBeUndefined(); }); }); + +// ── getLegacyOrphanContainersByStack (#1565) ─────────────────────────── + +describe('DockerController - getLegacyOrphanContainersByStack', () => { + it('returns [] when compose ps already manages containers', async () => { + const dc = DockerController.getInstance(1); + const fetchSpy = vi.spyOn(dc as unknown as { fetchComposePsContainers: (...a: unknown[]) => Promise }, 'fetchComposePsContainers') + .mockResolvedValue([{ ID: 'managed-c1', Name: 'web-1' }]); + const fallbackSpy = vi.spyOn(dc as unknown as { smartFallback: (...a: unknown[]) => Promise }, 'smartFallback') + .mockResolvedValue([{ Id: 'legacy-c1' }]); + + await expect(dc.getLegacyOrphanContainersByStack('my-stack')).resolves.toEqual([]); + expect(fallbackSpy).not.toHaveBeenCalled(); + fetchSpy.mockRestore(); + fallbackSpy.mockRestore(); + }); + + it('returns legacy orphan IDs when compose ps is empty', async () => { + const dc = DockerController.getInstance(1); + const fetchSpy = vi.spyOn(dc as unknown as { fetchComposePsContainers: (...a: unknown[]) => Promise }, 'fetchComposePsContainers') + .mockResolvedValue([]); + const fallbackSpy = vi.spyOn(dc as unknown as { smartFallback: (...a: unknown[]) => Promise }, 'smartFallback') + .mockResolvedValue([{ Id: 'legacy-c1' }, { Id: '' }, {}]); + + await expect(dc.getLegacyOrphanContainersByStack('my-stack')).resolves.toEqual([{ Id: 'legacy-c1' }]); + fetchSpy.mockRestore(); + fallbackSpy.mockRestore(); + }); + + it('falls back to legacy orphan lookup when compose ps throws', async () => { + const dc = DockerController.getInstance(1); + const fetchSpy = vi.spyOn(dc as unknown as { fetchComposePsContainers: (...a: unknown[]) => Promise }, 'fetchComposePsContainers') + .mockRejectedValue(new Error('compose ps failed')); + const fallbackSpy = vi.spyOn(dc as unknown as { smartFallback: (...a: unknown[]) => Promise }, 'smartFallback') + .mockResolvedValue([{ Id: 'legacy-c2' }]); + + await expect(dc.getLegacyOrphanContainersByStack('my-stack')).resolves.toEqual([{ Id: 'legacy-c2' }]); + fetchSpy.mockRestore(); + fallbackSpy.mockRestore(); + }); +}); diff --git a/backend/src/services/ComposeService.ts b/backend/src/services/ComposeService.ts index 5c6163e8..e6017c27 100644 --- a/backend/src/services/ComposeService.ts +++ b/backend/src/services/ComposeService.ts @@ -472,10 +472,10 @@ export class ComposeService { try { try { const dockerController = DockerController.getInstance(this.nodeId); - const legacyContainers = await dockerController.getContainersByStack(stackName); - if (legacyContainers && legacyContainers.length > 0) { - sendOutput(`=== Cleaning up existing containers for clean deployment ===\n`); - await dockerController.removeContainers(legacyContainers.map((c: any) => c.Id)); + const legacyOrphans = await dockerController.getLegacyOrphanContainersByStack(stackName); + if (legacyOrphans.length > 0) { + sendOutput(`=== Cleaning up legacy orphan containers before deployment ===\n`); + await dockerController.removeContainers(legacyOrphans.map((c) => c.Id)); } } catch (e) { console.warn('Failed to clean up legacy containers for %s:', sanitizeForLog(stackName), e); diff --git a/backend/src/services/DockerController.ts b/backend/src/services/DockerController.ts index 47c4a7bd..2f4b5e82 100644 --- a/backend/src/services/DockerController.ts +++ b/backend/src/services/DockerController.ts @@ -16,6 +16,16 @@ import { sanitizeForLog } from '../utils/safeLog'; import { describeSpawnError } from '../utils/spawnErrors'; import { authoredComposeFileArgs, authoredComposeEnvFileArgs } from '../utils/authoredComposeArgs'; +/** Parsed row from `docker compose ps --format json`. */ +interface ComposePsContainer { + ID?: string; + Name?: string; + Service?: string; + State?: string; + Status?: string; + Publishers?: { URL?: string; TargetPort?: number; PublishedPort?: number; Protocol?: string }[]; +} + const execFileAsync = promisify(execFile); const COMPOSE_DIR = process.env.COMPOSE_DIR || '/app/compose'; @@ -1460,6 +1470,70 @@ class DockerController { return result; } + /** + * Containers visible to `docker compose ps` for this stack. Empty when Compose + * does not manage any containers (including first deploy or mislabeled legacy). + */ + private async fetchComposePsContainers(stackName: string, stackDir: string): Promise { + const filePrefix = authoredComposeFileArgs(stackName, this.nodeId); + const envFileArgs = await authoredComposeEnvFileArgs(stackName, this.nodeId); + const { stdout, stderr } = await execFileAsync( + 'docker', + ['compose', ...filePrefix, ...envFileArgs, 'ps', '--format', 'json', '-a'], + { + cwd: stackDir, + env: { + ...process.env, + PATH: process.env.PATH || '/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin' + } + } + ); + + let containers: ComposePsContainer[] = []; + if (stdout && stdout.trim() !== '') { + try { + const parsed = JSON.parse(stdout); + containers = Array.isArray(parsed) ? parsed : [parsed]; + } catch (parseError) { + try { + const lines = stdout.trim().split('\n').filter(line => line.trim() !== ''); + containers = lines.map(line => JSON.parse(line) as ComposePsContainer); + } catch (innerError) { + console.error('Docker Compose JSON Parse Error for %s:', sanitizeForLog(stackName), sanitizeForLog(stderr || (parseError as Error).message)); + } + } + } + return containers; + } + + /** + * Legacy orphan containers that Compose ps cannot see but would block a deploy + * (wrong project labels). When Compose already manages the stack, returns [] so + * deploy can rely on selective `compose up` recreation. + */ + public async getLegacyOrphanContainersByStack(stackName: string): Promise> { + const stackDir = path.join(NodeRegistry.getInstance().getComposeDir(this.nodeId), stackName); + const toIds = (list: Array<{ Id?: string }>) => + list.filter((c): c is { Id: string } => typeof c.Id === 'string' && c.Id.length > 0) + .map((c) => ({ Id: c.Id })); + + try { + const composeContainers = await this.fetchComposePsContainers(stackName, stackDir); + if (composeContainers.length > 0) return []; + return toIds(await this.smartFallback(stackName, stackDir)); + } catch (error) { + const execError = error as NodeJS.ErrnoException & { stderr?: string }; + const mapped = describeSpawnError(execError, { command: 'docker compose ps' }); + const detail = execError.stderr || mapped.message; + console.error('Docker Compose Error for %s:', sanitizeForLog(stackName), sanitizeForLog(detail)); + try { + return toIds(await this.smartFallback(stackName, stackDir)); + } catch { + return []; + } + } + } + public async getContainersByStack(stackName: string) { // Resolve the compose dir and the authored prefix for THIS controller's node, // not the process default, so a non-default local node sees its own stack dir @@ -1467,55 +1541,7 @@ class DockerController { const stackDir = path.join(NodeRegistry.getInstance().getComposeDir(this.nodeId), stackName); try { - // Splice the authored multi-file prefix (-f files + -p + --project-directory) - // so a Git stack's override-only services are listed; single-file stacks get an - // empty prefix and behave exactly as before. execFile avoids shell quoting on - // the absolute --project-directory path. - const filePrefix = authoredComposeFileArgs(stackName, this.nodeId); - const envFileArgs = await authoredComposeEnvFileArgs(stackName, this.nodeId); - const { stdout, stderr } = await execFileAsync( - 'docker', - ['compose', ...filePrefix, ...envFileArgs, 'ps', '--format', 'json', '-a'], - { - cwd: stackDir, - env: { - ...process.env, - PATH: process.env.PATH || '/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin' - } - } - ); - - // Robust JSON parsing - handle both JSON array and newline-separated JSON objects - // Docker Compose v2 may return either format depending on version - interface ComposeContainer { - ID?: string; - Name?: string; - Service?: string; - State?: string; - Status?: string; - Publishers?: { URL?: string, TargetPort?: number, PublishedPort?: number, Protocol?: string }[]; - } - - let containers: ComposeContainer[] = []; - - // Only parse if stdout has content - if (stdout && stdout.trim() !== '') { - try { - // Try parsing as a standard JSON array - const parsed = JSON.parse(stdout); - containers = Array.isArray(parsed) ? parsed : [parsed]; - } catch (parseError) { - // Fallback: parse newline-separated JSON objects, filtering out empty lines - try { - const lines = stdout.trim().split('\n').filter(line => line.trim() !== ''); - containers = lines.map(line => JSON.parse(line) as ComposeContainer); - } catch (innerError) { - // Log parsing failure with stderr for debugging - console.error('Docker Compose JSON Parse Error for %s:', sanitizeForLog(stackName), sanitizeForLog(stderr || (parseError as Error).message)); - // Don't return empty - trigger smart fallback below - } - } - } + const containers = await this.fetchComposePsContainers(stackName, stackDir); // If containers found via docker compose ps, return them if (containers.length > 0) { diff --git a/frontend/src/components/FleetView/__tests__/NodeCard.test.tsx b/frontend/src/components/FleetView/__tests__/NodeCard.test.tsx index c0d1b726..cbd4518c 100644 --- a/frontend/src/components/FleetView/__tests__/NodeCard.test.tsx +++ b/frontend/src/components/FleetView/__tests__/NodeCard.test.tsx @@ -61,6 +61,13 @@ describe('NodeCard', () => { expect(screen.queryByRole('button', { name: 'Node actions' })).not.toBeInTheDocument(); }); + it('hides the actions menu for a Community admin with node:manage when cordon requires Admiral', () => { + useLicenseMock.mockReturnValue({ isPaid: false }); + render(); + // Cordon is Admiral-only; without edit/delete props, no menu items are available to Community. + expect(screen.queryByRole('button', { name: 'Node actions' })).not.toBeInTheDocument(); + }); + it('exposes the actions menu (cordon entry point) for a paid admin', () => { useLicenseMock.mockReturnValue({ isPaid: true }); render(); @@ -69,7 +76,7 @@ describe('NodeCard', () => { expect(screen.getByRole('button', { name: 'Node actions' })).toBeInTheDocument(); }); - it('exposes the cordon control for a node-admin via the node:manage permission', async () => { + it('exposes the cordon control for an Admiral node-admin via the node:manage permission', async () => { const can = vi.fn((action: string) => action === 'node:manage'); useAuthMock.mockReturnValue({ isAdmin: false, can }); useLicenseMock.mockReturnValue({ isPaid: true });