From fdcbd6f7fe9ce25d3c3647a6bb1216d3c2bff223 Mon Sep 17 00:00:00 2001 From: Danilo Alonso Date: Sat, 11 Jul 2026 13:20:41 -0400 Subject: [PATCH] fix(update): stream binary update with progress and stall timeout A bare fetch() with no timeout hung forever on a stalled connection, with no error surfaced and no progress shown. Stream to disk with a live progress readout, abort on a 30s stall, and stage the swap in the target's own directory to avoid a cross-filesystem EXDEV rename. --- .changeset/update-progress-stall.md | 5 + src/cli/update.ts | 70 +++++++++++- src/core/update/types.ts | 1 + src/core/update/updater.ts | 163 ++++++++++++++++++--------- tests/core/update/updater.test.ts | 165 ++++++++++++++++++++++++++++ 5 files changed, 351 insertions(+), 53 deletions(-) create mode 100644 .changeset/update-progress-stall.md create mode 100644 tests/core/update/updater.test.ts diff --git a/.changeset/update-progress-stall.md b/.changeset/update-progress-stall.md new file mode 100644 index 00000000..c6524367 --- /dev/null +++ b/.changeset/update-progress-stall.md @@ -0,0 +1,5 @@ +--- +"@noormdev/cli": patch +--- + +`fix(update):` the binary self-update no longer hangs indefinitely and now shows download progress. `noorm update` streamed the ~70MB release binary with a bare `fetch()` — no timeout, so a stalled connection hung forever with no error and no feedback, indistinguishable from a freeze. It now streams to disk with a live `Downloading X / Y MB (Z%)` readout, aborts with a clear error if the transfer stalls (no bytes for 30s), and stages the replacement in the target's own directory so the atomic swap can't fail with a cross-filesystem `EXDEV` (e.g. `os.tmpdir()` on a different volume than `~/.local/bin`). diff --git a/src/cli/update.ts b/src/cli/update.ts index 9faf0237..d87bb964 100644 --- a/src/cli/update.ts +++ b/src/cli/update.ts @@ -7,10 +7,18 @@ import { defineCommand } from 'citty'; import { attempt } from '@logosdx/utils'; +import { observer } from '../core/observer.js'; import { checkForUpdate, getCurrentVersion } from '../core/update/checker.js'; import { installUpdate } from '../core/update/updater.js'; import { outputError, outputResult, sharedArgs } from './_utils.js'; +/** Render a byte count as MB with one decimal (e.g. 41.2). */ +function toMb(bytes: number): string { + + return (bytes / 1024 / 1024).toFixed(1); + +} + const updateCommand = defineCommand({ meta: { name: 'update', @@ -70,9 +78,67 @@ const updateCommand = defineCommand({ } process.stdout.write(`Update available: ${currentVersion} → ${checkResult.latestVersion}\n`); - process.stdout.write('Installing...\n'); - const result = await installUpdate(checkResult.latestVersion); + // Render a live progress line for the binary download. TTY output uses a + // carriage return to update in place; when stdout is piped (e.g. CI, JSON + // mode) there's no cursor to rewind, so fall back to periodic newlines. + const isTty = Boolean(process.stdout.isTTY) && !args.json; + + const onProgress = ({ received, total }: { received: number; total: number }) => { + + const pct = total > 0 ? ` (${Math.floor((received / total) * 100)}%)` : ''; + const of = total > 0 ? ` / ${toMb(total)}` : ''; + const line = `Downloading ${toMb(received)}${of} MB${pct}`; + + if (isTty) { + + process.stdout.write(`\r${line} `); + + } + + }; + + if (!args.json) { + + process.stdout.write(isTty ? 'Installing...\n' : 'Installing (downloading binary)...\n'); + + } + + observer.on('update:progress', onProgress); + + const [result, installErr] = await attempt(() => installUpdate(checkResult.latestVersion)); + + observer.off('update:progress', onProgress); + + if (isTty) { + + process.stdout.write('\n'); + + } + + if (installErr || !result) { + + // installUpdate resolves with a result even on failure; a throw here + // is unexpected (e.g. abort wiring) — surface it rather than hang. + const errorMsg = installErr?.message ?? 'Update failed'; + + process.stderr.write(`Update failed: ${errorMsg}\n`); + + if (args.json) { + + outputResult(args, { + currentVersion, + latestVersion: checkResult.latestVersion, + updateAvailable: true, + installed: false, + error: errorMsg, + }, ''); + + } + + process.exit(1); + + } if (result.success) { diff --git a/src/core/update/types.ts b/src/core/update/types.ts index 1915a637..9ed14256 100644 --- a/src/core/update/types.ts +++ b/src/core/update/types.ts @@ -117,6 +117,7 @@ export interface UpdateEvents { // Update installation 'update:installing': { version: string }; + 'update:progress': { version: string; received: number; total: number }; 'update:complete': { previousVersion: string; newVersion: string }; 'update:failed': { version: string; error: string }; 'update:skipped': { version: string; reason: 'user-denied' | 'user-deferred' }; diff --git a/src/core/update/updater.ts b/src/core/update/updater.ts index 79ef073a..d8bf5057 100644 --- a/src/core/update/updater.ts +++ b/src/core/update/updater.ts @@ -14,9 +14,7 @@ * ``` */ import { spawn } from 'child_process'; -import { writeFile, rename, unlink, chmod } from 'fs/promises'; -import { tmpdir } from 'os'; -import { join } from 'path'; +import { open, rename, unlink, chmod } from 'fs/promises'; import { attempt } from '@logosdx/utils'; @@ -32,6 +30,16 @@ import type { UpdateResult } from './types.js'; /** NPM package name to install */ const PACKAGE_NAME = '@noormdev/cli'; +/** + * Abort the download if no bytes arrive for this long. A bare `fetch()` has no + * timeout, so a stalled connection would otherwise hang the process forever + * with no error to surface — this converts a silent hang into a real failure. + */ +const DOWNLOAD_STALL_MS = 30_000; + +/** Emit a progress event at most once per this many bytes, to avoid spamming. */ +const PROGRESS_EMIT_BYTES = 512 * 1024; + // ============================================================================= // npm Updater // ============================================================================= @@ -124,74 +132,134 @@ function installViaNpm(version: string, previousVersion: string): Promise { +export async function downloadToFile( + url: string, + destPath: string, + version: string, + stallMs: number = DOWNLOAD_STALL_MS, +): Promise { - const url = getBinaryDownloadUrl(version); + const controller = new AbortController(); + + let stallTimer: ReturnType | undefined; - const [response, fetchErr] = await attempt(() => fetch(url)); + const armStall = () => { - if (fetchErr || !response || !response.ok) { + clearTimeout(stallTimer); + stallTimer = setTimeout( + () => controller.abort(new Error(`download stalled — no data for ${stallMs / 1000}s`)), + stallMs, + ); + + }; - const errorMsg = fetchErr?.message ?? `HTTP ${response?.status} downloading binary`; + const response = await fetch(url, { signal: controller.signal }); - observer.emit('update:failed', { version, error: errorMsg }); + if (!response.ok || !response.body) { - return { - success: false, - previousVersion, - newVersion: version, - error: errorMsg, - }; + clearTimeout(stallTimer); + + throw new Error(`HTTP ${response.status} downloading binary`); } - const [buffer, readErr] = await attempt(() => response.arrayBuffer()); + const total = Number(response.headers.get('content-length')) || 0; + let received = 0; + let sinceLastEmit = 0; + + const handle = await open(destPath, 'w'); + + // A stall or write failure must still close the handle and clear the timer, + // so wrap the streaming loop and clean up in both outcomes. + const [, streamErr] = await attempt(async () => { + + armStall(); + + for await (const chunk of response.body as AsyncIterable) { + + await handle.write(chunk); + + received += chunk.byteLength; + sinceLastEmit += chunk.byteLength; + armStall(); + + if (sinceLastEmit >= PROGRESS_EMIT_BYTES) { + + sinceLastEmit = 0; + observer.emit('update:progress', { version, received, total }); + + } + + } + + // Final tick so consumers see 100% even when the last chunk was small. + observer.emit('update:progress', { version, received, total: total || received }); - if (readErr || !buffer) { + }); + + clearTimeout(stallTimer); + await handle.close(); - const errorMsg = readErr?.message ?? 'Failed to read binary response'; + if (streamErr) { - observer.emit('update:failed', { version, error: errorMsg }); + // controller.abort(reason) surfaces the stall reason as the thrown + // error's `cause`; prefer it so the message says "stalled", not "aborted". + const cause = streamErr.cause; - return { - success: false, - previousVersion, - newVersion: version, - error: errorMsg, - }; + throw cause instanceof Error ? cause : streamErr; } - // Write to temp file, then atomic rename to current executable path + await chmod(destPath, 0o755); + +} + +/** + * Install update by downloading a replacement binary from GitHub releases. + * + * Streams the platform-appropriate binary to a temp file **in the target's own + * directory** — a cross-filesystem `rename` (e.g. `os.tmpdir()` on a different + * volume than `~/.local/bin`) throws `EXDEV`, so the swap must stage next to the + * destination — then atomically replaces the current executable. + */ +async function installViaBinary(version: string, previousVersion: string): Promise { + + const url = getBinaryDownloadUrl(version); const currentExe = process.execPath; - const tmpPath = join(tmpdir(), `noorm-update-${version}-${Date.now()}`); + const tmpPath = `${currentExe}.download`; - const [, writeErr] = await attempt(async () => { + const fail = (error: string): UpdateResult => { - await writeFile(tmpPath, Buffer.from(buffer)); - await chmod(tmpPath, 0o755); + observer.emit('update:failed', { version, error }); - }); + return { success: false, previousVersion, newVersion: version, error }; - if (writeErr) { + }; + + const [, downloadErr] = await attempt(() => downloadToFile(url, tmpPath, version)); - observer.emit('update:failed', { version, error: writeErr.message }); + if (downloadErr) { - return { - success: false, - previousVersion, - newVersion: version, - error: writeErr.message, - }; + await attempt(() => unlink(tmpPath)); + + return fail(downloadErr.message); } - // Atomic replace: rename old → backup, rename new → current, remove backup + // Atomic replace: rename old → backup, rename new → current, remove backup. + // All three paths share `currentExe`'s directory, so every rename is + // same-filesystem and atomic. const backupPath = `${currentExe}.backup`; const [, swapErr] = await attempt(async () => { @@ -207,14 +275,7 @@ async function installViaBinary(version: string, previousVersion: string): Promi await attempt(() => rename(backupPath, currentExe)); await attempt(() => unlink(tmpPath)); - observer.emit('update:failed', { version, error: swapErr.message }); - - return { - success: false, - previousVersion, - newVersion: version, - error: swapErr.message, - }; + return fail(swapErr.message); } diff --git a/tests/core/update/updater.test.ts b/tests/core/update/updater.test.ts new file mode 100644 index 00000000..34903f7d --- /dev/null +++ b/tests/core/update/updater.test.ts @@ -0,0 +1,165 @@ +/** + * Tests for the binary-download path of the updater. + * + * These exercise the real streaming download (`downloadToFile`) against a live + * local HTTP server — no fetch/fs mocks — because the regressions we care about + * are behavioral: does it stream to disk, does it report progress, and does it + * fail (rather than hang forever) when the connection stalls? The `installUpdate` + * swap step is deliberately not tested here: in a test process `process.execPath` + * is the test runner's own binary, and swapping it would be catastrophic. + */ +import { describe, it, expect, beforeAll, afterAll } from 'bun:test'; +import { readFile, stat, mkdtemp, rm } from 'fs/promises'; +import { tmpdir } from 'os'; +import { join } from 'path'; + +import { attempt } from '@logosdx/utils'; + +import { downloadToFile } from '../../../src/core/update/updater.js'; +import { observer } from '../../../src/core/observer.js'; + +let server: ReturnType; +let baseUrl: string; +let workDir: string; + +// A payload large enough to cross the progress-emit threshold (512 KiB) a few +// times, so we can assert on real chunked progress rather than a single tick. +const PAYLOAD = new Uint8Array(1_500_000).fill(65); // 1.5 MB of 'A' + +beforeAll(async () => { + + workDir = await mkdtemp(join(tmpdir(), 'noorm-updater-test-')); + + server = Bun.serve({ + port: 0, + async fetch(req) { + + const url = new URL(req.url); + + // Healthy download with a correct Content-Length. + if (url.pathname === '/ok') { + + return new Response(PAYLOAD, { + headers: { 'content-length': String(PAYLOAD.byteLength) }, + }); + + } + + // Sends headers + a first chunk, then holds the stream open forever + // without sending more — the exact "connected but stalled" case a + // bare fetch() would hang on. + if (url.pathname === '/stall') { + + const stream = new ReadableStream({ + start(controller) { + + controller.enqueue(new Uint8Array(1024).fill(66)); + // never close, never enqueue again + + }, + }); + + return new Response(stream, { + headers: { 'content-length': String(PAYLOAD.byteLength) }, + }); + + } + + if (url.pathname === '/notfound') { + + return new Response('nope', { status: 404 }); + + } + + return new Response('unknown', { status: 500 }); + + }, + }); + + baseUrl = `http://localhost:${server.port}`; + +}); + +afterAll(async () => { + + server.stop(true); + await rm(workDir, { recursive: true, force: true }); + +}); + +describe('downloadToFile', () => { + + it('streams the full body to disk and makes it executable', async () => { + + const dest = join(workDir, 'ok.bin'); + + await downloadToFile(`${baseUrl}/ok`, dest, '1.0.0-test'); + + const written = await readFile(dest); + expect(written.byteLength).toBe(PAYLOAD.byteLength); + expect(written[0]).toBe(65); + + // chmod 0o755 — owner-executable bit must be set + const info = await stat(dest); + expect(info.mode & 0o111).not.toBe(0); + + }); + + it('emits monotonic progress that reaches the total', async () => { + + const dest = join(workDir, 'progress.bin'); + const ticks: Array<{ received: number; total: number }> = []; + + const onProgress = (p: { version: string; received: number; total: number }) => { + + ticks.push({ received: p.received, total: p.total }); + + }; + + observer.on('update:progress', onProgress); + await downloadToFile(`${baseUrl}/ok`, dest, '1.0.0-test'); + observer.off('update:progress', onProgress); + + expect(ticks.length).toBeGreaterThan(1); // real chunked progress, not one tick + + // received is non-decreasing + for (let i = 1; i < ticks.length; i++) { + + expect(ticks[i]!.received).toBeGreaterThanOrEqual(ticks[i - 1]!.received); + + } + + // the last tick reports the full payload against the advertised total + const last = ticks[ticks.length - 1]!; + expect(last.received).toBe(PAYLOAD.byteLength); + expect(last.total).toBe(PAYLOAD.byteLength); + + }); + + it('aborts with a "stalled" error instead of hanging when the stream stops', async () => { + + const dest = join(workDir, 'stall.bin'); + + const start = performance.now(); + const [, err] = await attempt(() => downloadToFile(`${baseUrl}/stall`, dest, '1.0.0-test', 300)); + const elapsed = performance.now() - start; + + expect(err).toBeTruthy(); + expect(err!.message).toContain('stalled'); + // proves it did not hang: bounded by the injected 300ms stall window + expect(elapsed).toBeLessThan(3000); + + }); + + it('rejects a non-OK response with the status code', async () => { + + const dest = join(workDir, 'nf.bin'); + + const [, err] = await attempt(() => downloadToFile(`${baseUrl}/notfound`, dest, '1.0.0-test')); + + expect(err).toBeTruthy(); + expect(err!.message).toContain('404'); + + }); + +});