From 98d235679eb7ea7a02bda8c185903233f25dbd71 Mon Sep 17 00:00:00 2001
From: Chris Sherwood <chris@crosstalksolutions.com>
Date: Sat, 6 Jun 2026 16:03:17 -0700
Subject: [PATCH] fix(docker): reject failed image pulls instead of treating
 them as success
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Every Docker pull went through `followProgress(pullStream, resolve)`, passing
the Promise's resolve as dockerode's onFinished(err, output) callback — so the
error argument was ignored and a failed pull (dropped/metered connection, bad
manifest, registry error, disk full mid-pull) resolved as if it had succeeded.
The code then tried to create/start a container from a missing or partial
image, surfacing a confusing downstream error rather than the real cause. (#790)

Add a DockerService.pullImage() helper that rejects when followProgress reports
an error, and route all five pull sites through it:
  - service install
  - AMD ROCm image pull
  - service update
  - force-reinstall / recreate (forcePull)
  - sysbench benchmark image pull

Closes #790

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---
 admin/app/services/benchmark_service.ts |  3 +--
 admin/app/services/docker_service.ts    | 33 +++++++++++++++++++------
 2 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/admin/app/services/benchmark_service.ts b/admin/app/services/benchmark_service.ts
index 47e4cf1..710b7ad 100644
--- a/admin/app/services/benchmark_service.ts
+++ b/admin/app/services/benchmark_service.ts
@@ -614,8 +614,7 @@ export class BenchmarkService {
       await this.dockerService.docker.getImage(SYSBENCH_IMAGE).inspect()
     } catch {
       this._updateStatus('starting', `Pulling sysbench image...`)
-      const pullStream = await this.dockerService.docker.pull(SYSBENCH_IMAGE)
-      await new Promise((resolve) => this.dockerService.docker.modem.followProgress(pullStream, resolve))
+      await this.dockerService.pullImage(SYSBENCH_IMAGE)
     }
   }
 
diff --git a/admin/app/services/docker_service.ts b/admin/app/services/docker_service.ts
index e907ca6..b4d40e4 100644
--- a/admin/app/services/docker_service.ts
+++ b/admin/app/services/docker_service.ts
@@ -58,6 +58,27 @@ export class DockerService {
     }
   }
 
+  /**
+   * Pull a Docker image and resolve only when the pull genuinely completes.
+   *
+   * dockerode's `followProgress(stream, onFinished)` reports failures via the
+   * first argument of onFinished. Every call site used to pass the Promise's
+   * `resolve` directly as that callback, so a failed pull (dropped/metered
+   * connection, bad manifest, registry error, disk full mid-pull) resolved as
+   * if it had succeeded — and the code then tried to create/start a container
+   * from a missing or partial image, surfacing a confusing downstream error.
+   * Rejecting on that error here lets callers fail fast with the real cause (#790).
+   */
+  async pullImage(imageName: string): Promise<void> {
+    const pullStream = await this.docker.pull(imageName)
+    await new Promise<void>((resolve, reject) => {
+      this.docker.modem.followProgress(pullStream, (error: Error | null) => {
+        if (error) reject(error)
+        else resolve()
+      })
+    })
+  }
+
   async affectContainer(
     serviceName: string,
     action: 'start' | 'stop' | 'restart'
@@ -632,13 +653,12 @@ export class DockerService {
         )
       } else {
         // Start pulling the Docker image and wait for it to complete
-        const pullStream = await this.docker.pull(service.container_image)
         this._broadcast(
           service.service_name,
           'pulling',
           `Pulling Docker image ${service.container_image}...`
         )
-        await new Promise((res) => this.docker.modem.followProgress(pullStream, res))
+        await this.pullImage(service.container_image)
       }
 
       if (service.service_name === SERVICE_NAMES.KIWIX) {
@@ -728,8 +748,7 @@ export class DockerService {
                 'pulling',
                 `Pulling Docker image ${finalImage}...`
               )
-              const rocmPullStream = await this.docker.pull(finalImage)
-              await new Promise((res) => this.docker.modem.followProgress(rocmPullStream, res))
+              await this.pullImage(finalImage)
             }
 
             const amdDevices = await this._discoverAMDDevices()
@@ -1523,8 +1542,7 @@ export class DockerService {
 
       // Step 1: Pull new image (runtimeImage diverges from newImage for AMD, see above)
       this._broadcast(serviceName, 'update-pulling', `Pulling image ${runtimeImage}...`)
-      const pullStream = await this.docker.pull(runtimeImage)
-      await new Promise((res) => this.docker.modem.followProgress(pullStream, res))
+      await this.pullImage(runtimeImage)
 
       // Step 2: Find and stop existing container
       this._broadcast(serviceName, 'update-stopping', `Stopping current container...`)
@@ -1996,8 +2014,7 @@ export class DockerService {
 
       // Pull the image if it's missing locally, or always when forcePull (e.g. :latest updates).
       if (opts.forcePull || !(await this._checkImageExists(service.container_image))) {
-        const pullStream = await this.docker.pull(service.container_image)
-        await new Promise((res) => this.docker.modem.followProgress(pullStream, res))
+        await this.pullImage(service.container_image)
       }
 
       const newContainer = await this.docker.createContainer({