fix: reject a stalled image pull instead of hanging (#646)

checkAndPullImage now rejects when the pull stream produces no progress
for DEFAULT_PULL_INACTIVITY_TIMEOUT (120 s) and destroys the stream. The
same value is set as the Docker client socket timeout, so a daemon that
accepts the request but never answers is covered as well. The timeout is
an optional second argument for callers that want a different bound.

Add regression tests for #564 that drive the real followProgress with a
fake pull stream, so they run without a Docker daemon: request failure,
stream error after progress, stall, slow-but-alive pull, and success.

Follow-up to #643 and #565 (thanks @turbocrime for the report).

Author: jleni

src/constants.ts

@@ -29,6 +29,8 @@ export const DEFAULT_START_TIMEOUT = 30000
 export const KILL_TIMEOUT = 5000
 export const DEFAULT_METHOD_TIMEOUT = 15000
 export const DEFAULT_WAIT_TIMEOUT = 45000
+// Reject an image pull that produces no progress events for this long
+export const DEFAULT_PULL_INACTIVITY_TIMEOUT = 120000
 
 export const DEFAULT_NANO_APPROVE_KEYWORD = 'APPROVE'
 export const DEFAULT_NANO_REJECT_KEYWORD = 'REJECT'

src/emulator.ts

@@ -17,6 +17,7 @@
 import path from 'node:path'
 import { Transform } from 'node:stream'
 import Docker, { type Container } from 'dockerode'
+import { DEFAULT_PULL_INACTIVITY_TIMEOUT } from './constants'
 
 // Development certificate key for emulator testing only - NOT FOR PRODUCTION USE
 // This is a well-known test key used by the Ledger emulator for development purposes
@@ -67,18 +68,46 @@ export default class EmuContainer {
     await Promise.all(targets.map((info) => docker.getContainer(info.Id).remove({ force: true })))
   }
 
-  static async checkAndPullImage(imageName: string): Promise<void> {
-    const docker = new Docker()
+  /**
+   * Pulls `imageName` if it is not present locally.
+   *
+   * Rejects instead of stalling when the daemon is unreachable, the pull fails, or the pull stream
+   * stops producing progress events for `inactivityTimeout` ms. The same value is applied as the
+   * socket timeout for the initial request, so a daemon that accepts the connection but never
+   * answers is covered too.
+   */
+  static async checkAndPullImage(imageName: string, inactivityTimeout: number = DEFAULT_PULL_INACTIVITY_TIMEOUT): Promise<void> {
+    const docker = new Docker({ timeout: inactivityTimeout })
     const stream = await docker.pull(imageName)
 
     await new Promise<void>((resolve, reject) => {
+      let watchdog: NodeJS.Timeout | undefined
+
+      const stopWatchdog = (): void => {
+        if (watchdog !== undefined) clearTimeout(watchdog)
+      }
+
+      const armWatchdog = (): void => {
+        stopWatchdog()
+        watchdog = setTimeout(() => {
+          const err = new Error(`[DOCKER] pull of ${imageName} produced no progress for ${inactivityTimeout} ms`)
+          process.stdout.write(`${err.message}\n`)
+          reject(err)
+          // Tear down the request so the daemon side is not left hanging
+          const destroyable = stream as { destroy?: (error?: Error) => void }
+          if (typeof destroyable.destroy === 'function') destroyable.destroy(err)
+        }, inactivityTimeout)
+      }
+
       const onProgress = (event: any): void => {
+        armWatchdog()
         const progress = event?.progress ?? ''
         const status = event?.status ?? ''
         process.stdout.write(`[DOCKER] ${status}: ${progress}\n`)
       }
 
       const onFinished = (err: Error | null): void => {
+        stopWatchdog()
         if (err != null) {
           process.stdout.write(`[DOCKER] ${err}\n`)
           reject(err)
@@ -88,6 +117,7 @@ export default class EmuContainer {
       }
 
       docker.modem.followProgress(stream, onFinished, onProgress)
+      armWatchdog()
     })
   }
 

tests/check-and-pull-image.test.ts

@@ -0,0 +1,126 @@
+/** ******************************************************************************
+ *  (c) 2018 - 2024 Zondax AG
+ *
+ *  Licensed under the Apache License, Version 2.0 (the "License");
+ *  you may not use this file except in compliance with the License.
+ *  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  Unless required by applicable law or agreed to in writing, software
+ *  distributed under the License is distributed on an "AS IS" BASIS,
+ *  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *  See the License for the specific language governing permissions and
+ *  limitations under the License.
+ ******************************************************************************* */
+import { PassThrough } from 'node:stream'
+import Docker from 'dockerode'
+import { afterEach, beforeEach, describe, expect, test, vi } from 'vitest'
+import Zemu from '../src'
+import { DEFAULT_EMU_IMG } from '../src/constants'
+import EmuContainer from '../src/emulator'
+
+// Regression tests for #564. None of these talk to a Docker daemon: `Docker.prototype.pull` is
+// replaced with a fake stream, while `docker.modem.followProgress` stays real so the tests drive the
+// same code path that used to stall when the pull failed.
+
+const IMAGE = 'zondax/does-not-matter:test'
+
+function fakePull(): PassThrough {
+  const stream = new PassThrough()
+  vi.spyOn(Docker.prototype, 'pull').mockImplementation(() => Promise.resolve(stream as any))
+  return stream
+}
+
+describe('EmuContainer.checkAndPullImage', () => {
+  beforeEach(() => {
+    // Keep pull progress out of the test output
+    vi.spyOn(process.stdout, 'write').mockImplementation(() => true)
+  })
+
+  afterEach(() => {
+    vi.restoreAllMocks()
+  })
+
+  test('rejects when the pull request itself fails', async () => {
+    const error = new Error('connect ENOENT /var/run/docker.sock')
+    vi.spyOn(Docker.prototype, 'pull').mockImplementation(() => Promise.reject(error))
+
+    await expect(EmuContainer.checkAndPullImage(IMAGE)).rejects.toBe(error)
+  })
+
+  test('rejects when the pull stream errors after progress started', async () => {
+    const stream = fakePull()
+    const error = new Error('unexpected EOF')
+
+    const pending = EmuContainer.checkAndPullImage(IMAGE)
+    stream.write('{"status":"Pulling from zondax/does-not-matter"}\n')
+    setImmediate(() => stream.emit('error', error))
+
+    await expect(pending).rejects.toBe(error)
+  })
+
+  test('rejects when the pull stream produces no progress for longer than the inactivity timeout', async () => {
+    const stream = fakePull()
+    const destroy = vi.spyOn(stream, 'destroy')
+
+    const pending = EmuContainer.checkAndPullImage(IMAGE, 50)
+    stream.write('{"status":"Waiting"}\n')
+
+    await expect(pending).rejects.toThrow(/no progress for 50 ms/)
+    expect(destroy).toHaveBeenCalledTimes(1)
+  })
+
+  test('keeps waiting while progress events keep arriving', async () => {
+    const stream = fakePull()
+
+    const pending = EmuContainer.checkAndPullImage(IMAGE, 80)
+    // Each event lands inside the timeout window but the whole pull takes longer than one window
+    for (let i = 0; i < 5; i++) {
+      await new Promise((r) => setTimeout(r, 40))
+      stream.write(`{"status":"Downloading","progress":"${i * 20}%"}\n`)
+    }
+    stream.end()
+
+    await expect(pending).resolves.toBeUndefined()
+  })
+
+  test('resolves when the pull completes', async () => {
+    const stream = fakePull()
+
+    const pending = EmuContainer.checkAndPullImage(IMAGE)
+    stream.write('{"status":"Pull complete"}\n')
+    stream.end()
+
+    await expect(pending).resolves.toBeUndefined()
+  })
+
+  test('passes the inactivity timeout to the Docker client as socket timeout', async () => {
+    const stream = fakePull()
+    let clientTimeout: number | undefined
+    vi.spyOn(Docker.prototype, 'pull').mockImplementation(function (this: Docker) {
+      clientTimeout = (this.modem as { timeout?: number }).timeout
+      return Promise.resolve(stream as any)
+    })
+
+    const pending = EmuContainer.checkAndPullImage(IMAGE, 1234)
+    stream.end()
+    await pending
+
+    expect(clientTimeout).toBe(1234)
+  })
+})
+
+describe('Zemu.checkAndPullImage', () => {
+  afterEach(() => {
+    vi.restoreAllMocks()
+  })
+
+  test('pulls the default emulator image and propagates failures', async () => {
+    const error = new Error('pull failed')
+    const spy = vi.spyOn(EmuContainer, 'checkAndPullImage').mockRejectedValue(error)
+
+    await expect(Zemu.checkAndPullImage()).rejects.toBe(error)
+    expect(spy).toHaveBeenCalledWith(DEFAULT_EMU_IMG)
+  })
+})