fix(stacks): serialize concurrent lifecycle operations per stack (#1182)

* fix(stacks): serialize concurrent lifecycle operations per stack Two simultaneous POSTs to /api/stacks/:name/{deploy,down,restart,stop, start,update} could race against the same compose project, doubling notifications, doubling post-deploy scans, and corrupting the atomic-deploy backup snapshot. Each lifecycle route now acquires a per-(nodeId, stackName) in-process lock; the second caller gets 409 with {code: 'stack_op_in_progress', inProgress: {action, startedAt, user}} and the frontend surfaces a "X is already deploying" toast. The lock is process-local on purpose: it shares a lifetime with the docker compose child process. A Sencho restart clears all locks, which matches the truth that an in-flight compose op is gone too. The existing policy-block 409 is shape-distinguishable (has policy / violations) and continues to work; the frontend checks the new code discriminator first before falling through to policy handling. * chore(stacks): validate action enum in 409 parser; cover start collision The frontend parseStackOpInProgress used to cast the parsed action directly to StackOpAction. A backend bug or spoofed payload returning action='wibble' would slip through. Validate against the known enum set before returning the parsed info. Adds an integration test for the deploy-blocks-while-start-in-flight case so all six lifecycle verbs have collision coverage (the existing suite covered deploy/down/restart/stop/update; start was indirect).
2026-08-24 17:36:42 +00:00 · 2026-05-24 01:12:07 -04:00
parent 8ba88755b1
commit 60ecd574b3
5 changed files with 621 additions and 20 deletions
@@ -0,0 +1,297 @@
+/**
+ * Integration tests for the per-(nodeId, stackName) lifecycle mutex.
+ *
+ * The mutex prevents two simultaneous compose actions from racing against the
+ * same stack. The second caller receives 409 with a structured envelope so the
+ * frontend can show "X is already deploying" instead of doubling up.
+ *
+ * Each lifecycle route (deploy, down, restart, stop, start, update) is
+ * exercised: the first request is held mid-call via a deferred promise so the
+ * second request lands while the lock is still held, asserting both 409 and
+ * the expected `{code, inProgress}` payload.
+ */
+import { describe, it, expect, beforeAll, afterAll, beforeEach, vi } from 'vitest';
+import request from 'supertest';
+import { setupTestDb, cleanupTestDb, loginAsTestAdmin } from './helpers/setupTestDb';
+
+const {
+  mockDeployStack,
+  mockRunCommand,
+  mockUpdateStack,
+  mockGetContainersByStack,
+  mockRestartContainer,
+  mockStopContainer,
+  mockStartContainer,
+} = vi.hoisted(() => ({
+  mockDeployStack: vi.fn(),
+  mockRunCommand: vi.fn(),
+  mockUpdateStack: vi.fn(),
+  mockGetContainersByStack: vi.fn(),
+  mockRestartContainer: vi.fn(),
+  mockStopContainer: vi.fn(),
+  mockStartContainer: vi.fn(),
+}));
+
+vi.mock('../services/ComposeService', async () => {
+  const actual = await vi.importActual<typeof import('../services/ComposeService')>(
+    '../services/ComposeService',
+  );
+  return {
+    ...actual,
+    ComposeService: {
+      ...actual.ComposeService,
+      getInstance: () => ({
+        deployStack: mockDeployStack,
+        runCommand: mockRunCommand,
+        updateStack: mockUpdateStack,
+      }),
+    },
+  };
+});
+
+vi.mock('../services/DockerController', async () => {
+  const actual = await vi.importActual<typeof import('../services/DockerController')>(
+    '../services/DockerController',
+  );
+  return {
+    ...actual,
+    default: {
+      ...actual.default,
+      getInstance: () => ({
+        getContainersByStack: mockGetContainersByStack,
+        restartContainer: mockRestartContainer,
+        stopContainer: mockStopContainer,
+        startContainer: mockStartContainer,
+      }),
+    },
+  };
+});
+
+vi.mock('../services/FileSystemService', () => ({
+  FileSystemService: {
+    getInstance: () => ({
+      getBaseDir: () => '/tmp/compose',
+      hasComposeFile: vi.fn().mockResolvedValue(true),
+    }),
+  },
+}));
+
+let tmpDir: string;
+let app: import('express').Express;
+let authCookie: string;
+
+beforeAll(async () => {
+  tmpDir = await setupTestDb();
+  ({ app } = await import('../index'));
+  authCookie = await loginAsTestAdmin(app);
+
+  const { NotificationService } = await import('../services/NotificationService');
+  vi.spyOn(NotificationService.getInstance(), 'dispatchAlert').mockResolvedValue(undefined);
+});
+
+afterAll(() => {
+  vi.restoreAllMocks();
+  cleanupTestDb(tmpDir);
+});
+
+beforeEach(async () => {
+  mockDeployStack.mockReset();
+  mockRunCommand.mockReset();
+  mockUpdateStack.mockReset();
+  mockGetContainersByStack.mockReset();
+  mockRestartContainer.mockReset();
+  mockStopContainer.mockReset();
+  mockStartContainer.mockReset();
+  const { StackOpLockService } = await import('../services/StackOpLockService');
+  StackOpLockService.resetForTests();
+});
+
+interface Deferred<T> {
+  promise: Promise<T>;
+  resolve: (value: T) => void;
+  reject: (err: unknown) => void;
+}
+
+function deferred<T>(): Deferred<T> {
+  let resolve!: (value: T) => void;
+  let reject!: (err: unknown) => void;
+  const promise = new Promise<T>((res, rej) => {
+    resolve = res;
+    reject = rej;
+  });
+  return { promise, resolve, reject };
+}
+
+describe('Stack lifecycle mutex', () => {
+  it('returns 409 with stack_op_in_progress when a deploy is already running', async () => {
+    const gate = deferred<void>();
+    mockDeployStack.mockImplementationOnce(() => gate.promise);
+
+    const first = request(app)
+      .post('/api/stacks/web/deploy')
+      .set('Cookie', authCookie)
+      .send({ skip_scan: true })
+      .then(r => r);
+
+    await vi.waitFor(() => expect(mockDeployStack).toHaveBeenCalled());
+
+    const second = await request(app)
+      .post('/api/stacks/web/deploy')
+      .set('Cookie', authCookie)
+      .send({ skip_scan: true });
+
+    expect(second.status).toBe(409);
+    expect(second.body).toMatchObject({
+      code: 'stack_op_in_progress',
+      inProgress: { action: 'deploy' },
+    });
+    expect(second.body.error).toMatch(/already deploying/i);
+    expect(typeof second.body.inProgress.startedAt).toBe('number');
+
+    gate.resolve();
+    const firstRes = await first;
+    expect(firstRes.status).toBe(200);
+  });
+
+  it('releases the lock after a successful deploy so the next request acquires', async () => {
+    mockDeployStack.mockResolvedValueOnce(undefined);
+    const first = await request(app)
+      .post('/api/stacks/web/deploy')
+      .set('Cookie', authCookie)
+      .send({ skip_scan: true });
+    expect(first.status).toBe(200);
+
+    mockDeployStack.mockResolvedValueOnce(undefined);
+    const second = await request(app)
+      .post('/api/stacks/web/deploy')
+      .set('Cookie', authCookie)
+      .send({ skip_scan: true });
+    expect(second.status).toBe(200);
+  });
+
+  it('releases the lock after a failed deploy', async () => {
+    mockDeployStack.mockRejectedValueOnce(new Error('image pull failed'));
+    const first = await request(app)
+      .post('/api/stacks/web/deploy')
+      .set('Cookie', authCookie)
+      .send({ skip_scan: true });
+    expect(first.status).toBe(500);
+
+    mockDeployStack.mockResolvedValueOnce(undefined);
+    const second = await request(app)
+      .post('/api/stacks/web/deploy')
+      .set('Cookie', authCookie)
+      .send({ skip_scan: true });
+    expect(second.status).toBe(200);
+  });
+
+  it('blocks restart while a deploy is in flight on the same stack', async () => {
+    const gate = deferred<void>();
+    mockDeployStack.mockImplementationOnce(() => gate.promise);
+
+    const deploy = request(app)
+      .post('/api/stacks/web/deploy')
+      .set('Cookie', authCookie)
+      .send({ skip_scan: true })
+      .then(r => r);
+    await vi.waitFor(() => expect(mockDeployStack).toHaveBeenCalled());
+
+    const restart = await request(app)
+      .post('/api/stacks/web/restart')
+      .set('Cookie', authCookie);
+    expect(restart.status).toBe(409);
+    expect(restart.body.code).toBe('stack_op_in_progress');
+    expect(restart.body.inProgress.action).toBe('deploy');
+
+    gate.resolve();
+    await deploy;
+  });
+
+  it('allows concurrent ops on different stacks', async () => {
+    const gate = deferred<void>();
+    mockDeployStack.mockImplementation(() => gate.promise);
+
+    const webDeploy = request(app)
+      .post('/api/stacks/web/deploy')
+      .set('Cookie', authCookie)
+      .send({ skip_scan: true })
+      .then(r => r);
+    await vi.waitFor(() => expect(mockDeployStack).toHaveBeenCalledTimes(1));
+
+    const apiDeploy = request(app)
+      .post('/api/stacks/api/deploy')
+      .set('Cookie', authCookie)
+      .send({ skip_scan: true })
+      .then(r => r);
+    await vi.waitFor(() => expect(mockDeployStack).toHaveBeenCalledTimes(2));
+
+    gate.resolve();
+    const [webRes, apiRes] = await Promise.all([webDeploy, apiDeploy]);
+    expect(webRes.status).toBe(200);
+    expect(apiRes.status).toBe(200);
+  });
+
+  it('blocks update while down is in flight', async () => {
+    const gate = deferred<void>();
+    mockRunCommand.mockImplementationOnce(() => gate.promise);
+
+    const down = request(app)
+      .post('/api/stacks/web/down')
+      .set('Cookie', authCookie)
+      .then(r => r);
+    await vi.waitFor(() => expect(mockRunCommand).toHaveBeenCalled());
+
+    const update = await request(app)
+      .post('/api/stacks/web/update')
+      .set('Cookie', authCookie)
+      .send({ skip_scan: true });
+    expect(update.status).toBe(409);
+    expect(update.body.inProgress.action).toBe('down');
+
+    gate.resolve();
+    await down;
+  });
+
+  it('returns 409 for deploy while start is in flight', async () => {
+    mockGetContainersByStack.mockResolvedValue([{ Id: 'c1' }]);
+    const gate = deferred<void>();
+    mockStartContainer.mockImplementationOnce(() => gate.promise);
+
+    const start = request(app)
+      .post('/api/stacks/web/start')
+      .set('Cookie', authCookie)
+      .then(r => r);
+    await vi.waitFor(() => expect(mockStartContainer).toHaveBeenCalled());
+
+    const deploy = await request(app)
+      .post('/api/stacks/web/deploy')
+      .set('Cookie', authCookie)
+      .send({ skip_scan: true });
+    expect(deploy.status).toBe(409);
+    expect(deploy.body.inProgress.action).toBe('start');
+
+    gate.resolve();
+    await start;
+  });
+
+  it('returns 409 for stop while restart is in flight', async () => {
+    mockGetContainersByStack.mockResolvedValue([{ Id: 'c1' }]);
+    const gate = deferred<void>();
+    mockRestartContainer.mockImplementationOnce(() => gate.promise);
+
+    const restart = request(app)
+      .post('/api/stacks/web/restart')
+      .set('Cookie', authCookie)
+      .then(r => r);
+    await vi.waitFor(() => expect(mockRestartContainer).toHaveBeenCalled());
+
+    const stop = await request(app)
+      .post('/api/stacks/web/stop')
+      .set('Cookie', authCookie);
+    expect(stop.status).toBe(409);
+    expect(stop.body.inProgress.action).toBe('restart');
+
+    gate.resolve();
+    await restart;
+  });
+});
@@ -0,0 +1,96 @@
+/**
+ * Unit tests for StackOpLockService.
+ *
+ * The service is the in-memory mutex behind the 409 fast-fail for concurrent
+ * stack lifecycle operations. Each test resets the singleton via
+ * `resetForTests()` so state doesn't leak between cases.
+ */
+import { describe, it, expect, beforeEach } from 'vitest';
+import { StackOpLockService } from '../services/StackOpLockService';
+
+beforeEach(() => {
+  StackOpLockService.resetForTests();
+});
+
+describe('StackOpLockService', () => {
+  it('returns a singleton instance', () => {
+    const a = StackOpLockService.getInstance();
+    const b = StackOpLockService.getInstance();
+    expect(a).toBe(b);
+  });
+
+  it('acquires a lock when the slot is empty', () => {
+    const svc = StackOpLockService.getInstance();
+    const result = svc.tryAcquire(1, 'web', 'deploy', 'admin');
+    expect(result.acquired).toBe(true);
+    expect(svc.size()).toBe(1);
+  });
+
+  it('returns acquired=false with the existing lock when already held', () => {
+    const svc = StackOpLockService.getInstance();
+    svc.tryAcquire(1, 'web', 'deploy', 'admin');
+    const result = svc.tryAcquire(1, 'web', 'restart', 'bob');
+    expect(result.acquired).toBe(false);
+    if (!result.acquired) {
+      expect(result.existing.action).toBe('deploy');
+      expect(result.existing.user).toBe('admin');
+      expect(typeof result.existing.startedAt).toBe('number');
+    }
+  });
+
+  it('different stacks on the same node lock independently', () => {
+    const svc = StackOpLockService.getInstance();
+    expect(svc.tryAcquire(1, 'web', 'deploy', 'admin').acquired).toBe(true);
+    expect(svc.tryAcquire(1, 'api', 'deploy', 'admin').acquired).toBe(true);
+    expect(svc.size()).toBe(2);
+  });
+
+  it('same stack name on different nodes locks independently', () => {
+    const svc = StackOpLockService.getInstance();
+    expect(svc.tryAcquire(1, 'web', 'deploy', 'admin').acquired).toBe(true);
+    expect(svc.tryAcquire(2, 'web', 'deploy', 'admin').acquired).toBe(true);
+    expect(svc.size()).toBe(2);
+  });
+
+  it('release frees the slot so the next caller acquires', () => {
+    const svc = StackOpLockService.getInstance();
+    svc.tryAcquire(1, 'web', 'deploy', 'admin');
+    svc.release(1, 'web');
+    expect(svc.size()).toBe(0);
+    expect(svc.tryAcquire(1, 'web', 'restart', 'bob').acquired).toBe(true);
+  });
+
+  it('release on an unheld key is a no-op', () => {
+    const svc = StackOpLockService.getInstance();
+    expect(() => svc.release(1, 'nope')).not.toThrow();
+    expect(svc.size()).toBe(0);
+  });
+
+  it('get returns the lock contents or undefined', () => {
+    const svc = StackOpLockService.getInstance();
+    expect(svc.get(1, 'web')).toBeUndefined();
+    svc.tryAcquire(1, 'web', 'update', 'eve');
+    const lock = svc.get(1, 'web');
+    expect(lock?.action).toBe('update');
+    expect(lock?.user).toBe('eve');
+  });
+
+  it('resetForTests clears all state and returns a fresh instance', () => {
+    const before = StackOpLockService.getInstance();
+    before.tryAcquire(1, 'web', 'deploy', 'admin');
+    StackOpLockService.resetForTests();
+    const after = StackOpLockService.getInstance();
+    expect(after).not.toBe(before);
+    expect(after.size()).toBe(0);
+  });
+
+  it('records the lock startedAt as a recent timestamp', () => {
+    const svc = StackOpLockService.getInstance();
+    const t0 = Date.now();
+    svc.tryAcquire(1, 'web', 'deploy', 'admin');
+    const lock = svc.get(1, 'web');
+    expect(lock).toBeDefined();
+    expect(lock!.startedAt).toBeGreaterThanOrEqual(t0);
+    expect(lock!.startedAt).toBeLessThanOrEqual(Date.now());
+  });
+});