diff --git a/admin/app/services/docker_service.ts b/admin/app/services/docker_service.ts index ca8fc0d2..ef238da2 100644 --- a/admin/app/services/docker_service.ts +++ b/admin/app/services/docker_service.ts @@ -5,6 +5,7 @@ import { inject } from '@adonisjs/core' import transmit from '@adonisjs/transmit/services/main' import { doResumableDownloadWithRetry } from '../utils/downloads.js' import { pickAmdHsaOverride } from '../utils/amd_hsa_override.js' +import { joinInFlight } from '../utils/in_flight.js' import { join } from 'path' import os from 'node:os' import env from '#start/env' @@ -35,6 +36,9 @@ const AMD_GFX_MARKER_PATH = '/app/storage/.nomad-amd-gfx' export class DockerService { public docker: Docker private activeInstallations: Set = new Set() + // Static, not per-instance: controllers get a fresh DockerService per request, and + // the installs that race (#1439) arrive on separate requests. + private static installsInFlight = new Map>() public static NOMAD_NETWORK = 'project-nomad_default' public static ADMIN_CONTAINER_NAME = 'nomad_admin' // The admin's own storage mount destination inside its container (compose maps @@ -627,6 +631,17 @@ export class DockerService { async _createContainer( service: Service & { dependencies?: Service[] }, containerConfig: any + ): Promise { + // A service that is already installing (its own install, or another service's + // dependency install) is joined rather than started again (#1439). + return joinInFlight(DockerService.installsInFlight, service.service_name, () => + this._installContainer(service, containerConfig) + ) + } + + private async _installContainer( + service: Service & { dependencies?: Service[] }, + containerConfig: any ): Promise { try { this._broadcast(service.service_name, 'initializing', '') @@ -652,11 +667,19 @@ export class DockerService { ) for (const dependency of dependencies) { if (!dependency.installed) { - this._broadcast( - service.service_name, - 'dependency-not-installed', - `Dependency service ${dependency.service_name} is not installed. Installing it first...` - ) + if (DockerService.installsInFlight.has(dependency.service_name)) { + this._broadcast( + service.service_name, + 'dependency-installing', + `Dependency service ${dependency.service_name} is already being installed. Waiting for it to finish...` + ) + } else { + this._broadcast( + service.service_name, + 'dependency-not-installed', + `Dependency service ${dependency.service_name} is not installed. Installing it first...` + ) + } await this._createContainer( dependency, this._parseContainerConfig(dependency.container_config) diff --git a/admin/app/utils/in_flight.ts b/admin/app/utils/in_flight.ts new file mode 100644 index 00000000..058c65cb --- /dev/null +++ b/admin/app/utils/in_flight.ts @@ -0,0 +1,29 @@ +/** + * Shares one run of an async task between every caller that asks for the same + * key while it is running. + * + * Used by `DockerService._createContainer` so that two installs of the same + * service can't run side by side (#1439). Installing Ollama and Qdrant together + * from the setup wizard starts Qdrant's own install, then Ollama's dependency + * check, which sees Qdrant not yet installed and starts a second one. The second + * hits Docker's 409 on the container name, and its failure cleanup removes the + * container the first install created. + * + * The run is registered synchronously, before `start` yields, so a caller that + * arrives on the next tick already finds it. It is removed once it settles, + * whether it resolved or rejected, so a failed install can be retried. + */ +export function joinInFlight( + inFlight: Map>, + key: string, + start: () => Promise +): Promise { + const running = inFlight.get(key) + if (running) return running + + const run = start().finally(() => { + if (inFlight.get(key) === run) inFlight.delete(key) + }) + inFlight.set(key, run) + return run +} diff --git a/admin/tests/unit/in_flight.spec.ts b/admin/tests/unit/in_flight.spec.ts new file mode 100644 index 00000000..d7cf0415 --- /dev/null +++ b/admin/tests/unit/in_flight.spec.ts @@ -0,0 +1,79 @@ +import * as assert from 'node:assert/strict' +import { test } from 'node:test' + +import { joinInFlight } from '../../app/utils/in_flight.js' + +function deferred() { + let resolve!: () => void + let reject!: (err: Error) => void + const promise = new Promise((res, rej) => { + resolve = res + reject = rej + }) + return { promise, resolve, reject } +} + +test('a second install of a service that is still installing joins the first (#1439)', async () => { + const inFlight = new Map>() + const qdrant = deferred() + let starts = 0 + const install = () => { + starts++ + return qdrant.promise + } + + // Qdrant's own install, then Ollama's dependency check for Qdrant, on the next tick. + const own = joinInFlight(inFlight, 'nomad_qdrant', install) + await Promise.resolve() + const fromDependency = joinInFlight(inFlight, 'nomad_qdrant', install) + + assert.equal(starts, 1) + assert.equal(fromDependency, own) + + qdrant.resolve() + await fromDependency + assert.equal(inFlight.has('nomad_qdrant'), false) +}) + +test('a joined install that fails rejects every caller once, and can be retried', async () => { + const inFlight = new Map>() + const first = deferred() + let starts = 0 + + const a = joinInFlight(inFlight, 'nomad_qdrant', () => { + starts++ + return first.promise + }) + const b = joinInFlight(inFlight, 'nomad_qdrant', () => { + starts++ + return Promise.resolve() + }) + first.reject(new Error('pull failed')) + + await assert.rejects(a, /pull failed/) + await assert.rejects(b, /pull failed/) + assert.equal(starts, 1) + assert.equal(inFlight.has('nomad_qdrant'), false) + + await joinInFlight(inFlight, 'nomad_qdrant', () => { + starts++ + return Promise.resolve() + }) + assert.equal(starts, 2) +}) + +test('installs of different services run independently', async () => { + const inFlight = new Map>() + const qdrant = deferred() + let ollamaStarted = false + + const q = joinInFlight(inFlight, 'nomad_qdrant', () => qdrant.promise) + await joinInFlight(inFlight, 'nomad_ollama', async () => { + ollamaStarted = true + }) + + assert.equal(ollamaStarted, true) + assert.equal(inFlight.has('nomad_qdrant'), true) + qdrant.resolve() + await q +})