From 4c4675ef4fc4695edf5fa3c1c3168f7e53f68f54 Mon Sep 17 00:00:00 2001 From: yuj Date: Tue, 19 May 2026 12:18:22 +0800 Subject: [PATCH 1/2] fix(update): wait for writer 'finish' + honour backpressure + clean up tmp MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `downloadFile()` in `src/update/self-update.ts` had three concrete reliability issues, all triggered in the self-update path: 1. `resolve()` was called as soon as the read loop hit `done`, but `writer.end()` only *queues* the close — bytes may still be in the kernel pagecache. The very next step, `verifySha256()`, reads back the same file; on slower disks (HDD, network FS, CI runners) this races the flush and surfaces as a spurious "Checksum mismatch" error mid-update. Replace the `resolve()` call with a `writer.on( 'finish')` handler so we only proceed once the file is durably closed. 2. `writer.write(value)` was called without honouring its boolean return value. For large binaries the writer's internal buffer grows without bound until the entire download is in memory. Wait for the `'drain'` event when `write()` returns `false`. 3. On any error (network reset, 5xx mid-stream, writer error), the half-written file at `dest` was left in `tmpdir()`. Unlink it in a catch block before rethrowing. 4. The Web Streams reader's lock was never released — neither on the happy path nor on errors. Move the `releaseLock()` into a `finally` so the underlying body stream is always freed. No behavior change on the success path beyond the (correct) flush wait. `tsc --noEmit` passes. --- src/update/self-update.ts | 49 ++++++++++++++++++++++++++------------- 1 file changed, 33 insertions(+), 16 deletions(-) diff --git a/src/update/self-update.ts b/src/update/self-update.ts index 1542b69..2cd8bf3 100644 --- a/src/update/self-update.ts +++ b/src/update/self-update.ts @@ -111,22 +111,39 @@ async function downloadFile(url: string, dest: string, onProgress?: (pct: number const writer = createWriteStream(dest); const reader = res.body!.getReader(); - await new Promise((resolve, reject) => { - writer.on('error', reject); - const pump = async () => { - try { - while (true) { - const { done, value } = await reader.read(); - if (done) { writer.end(); break; } - writer.write(value); - received += value.length; - if (onProgress && total > 0) onProgress(Math.round(received / total * 100)); - } - resolve(); - } catch (e) { reject(e); } - }; - pump(); - }); + try { + await new Promise((resolve, reject) => { + writer.on('error', reject); + // Resolve only after the writer has actually flushed and closed — + // otherwise verifySha256() can race the kernel's pagecache flush and + // produce spurious checksum mismatches on slow disks. + writer.on('finish', () => resolve()); + const pump = async () => { + try { + while (true) { + const { done, value } = await reader.read(); + if (done) { writer.end(); break; } + // Honour backpressure so we don't grow the writer's internal buffer + // unboundedly on large binaries / slow disks. + if (!writer.write(value)) { + await new Promise(r => writer.once('drain', r)); + } + received += value.length; + if (onProgress && total > 0) onProgress(Math.round(received / total * 100)); + } + } catch (e) { reject(e); } + }; + pump(); + }); + } catch (err) { + // Don't leave a half-downloaded binary in /tmp on failure. + try { (await import('fs')).unlinkSync(dest); } catch { /* best-effort */ } + throw err; + } finally { + // Always release the Web Streams reader lock — the API contract requires + // a paired acquire/release, and not doing so traps the underlying body. + reader.releaseLock(); + } } export async function resolveUpdateTarget(channel: Channel): Promise { From 0791c282bd840f73901f80025f8835dfa1404586 Mon Sep 17 00:00:00 2001 From: yuj Date: Fri, 22 May 2026 10:48:02 +0800 Subject: [PATCH 2/2] test(self-update): cover stream-failure cleanup + tighten error teardown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback on #158 (thanks @NianJiuZst): the error path needs to tear the writer down BEFORE unlinking dest, otherwise the writer's buffered bytes can race the unlink and leave a partial file in the pagecache. And the function still had no test coverage, so the `finish`/`drain`/cleanup/`releaseLock()` behaviour could regress silently. Changes: * `downloadFile`: on error, `writer.destroy()` and await `'close'` before calling `unlinkSync(dest)`. Skip the wait when the writer is already torn down so the rejection's exception isn't masked. * Export `downloadFile` from `src/update/self-update.ts` so unit tests can drive it directly (still not part of the public CLI surface — callers go through `applySelfUpdate`). * Add `test/update/self-update.test.ts` with three tests, matching the existing `test/files/download.test.ts` style: - mid-stream `ReadableStream` error path: rejects, dest gone, tempdir empty (the case the reviewer specifically asked for) - happy path: payload written, dest exists - 4xx response: rejects with CLIError, dest not created `npx tsc --noEmit` passes. --- src/update/self-update.ts | 17 +++++- test/update/self-update.test.ts | 97 +++++++++++++++++++++++++++++++++ 2 files changed, 112 insertions(+), 2 deletions(-) create mode 100644 test/update/self-update.test.ts diff --git a/src/update/self-update.ts b/src/update/self-update.ts index 2cd8bf3..1b401ce 100644 --- a/src/update/self-update.ts +++ b/src/update/self-update.ts @@ -101,7 +101,9 @@ async function verifySha256(filePath: string, expected: string): Promise { } } -async function downloadFile(url: string, dest: string, onProgress?: (pct: number) => void): Promise { +// Exported for unit tests (see test/update/self-update.test.ts). Not part of +// the public CLI surface; callers go through `applySelfUpdate`. +export async function downloadFile(url: string, dest: string, onProgress?: (pct: number) => void): Promise { const res = await fetch(url, { signal: AbortSignal.timeout(120_000) }); if (!res.ok) throw new CLIError(`Download failed: ${res.status} ${res.statusText}`, ExitCode.GENERAL); @@ -136,8 +138,19 @@ async function downloadFile(url: string, dest: string, onProgress?: (pct: number pump(); }); } catch (err) { + // Tear down the writer BEFORE unlinking dest so any buffered bytes + // can't race the unlink and leave a partial file in the pagecache, and + // so a slow flush doesn't race the next process opening the same path. + // Wait for `close` (fires after destroy on Node streams) unless the + // writer has already torn down. + if (!writer.destroyed) { + await new Promise(r => { + writer.once('close', () => r()); + writer.destroy(); + }); + } // Don't leave a half-downloaded binary in /tmp on failure. - try { (await import('fs')).unlinkSync(dest); } catch { /* best-effort */ } + try { (await import('fs')).unlinkSync(dest); } catch { /* best-effort — race with concurrent cleanup is fine */ } throw err; } finally { // Always release the Web Streams reader lock — the API contract requires diff --git a/test/update/self-update.test.ts b/test/update/self-update.test.ts new file mode 100644 index 0000000..58847a3 --- /dev/null +++ b/test/update/self-update.test.ts @@ -0,0 +1,97 @@ +import { afterEach, describe, expect, it } from 'bun:test'; +import { existsSync, mkdtempSync, readdirSync, rmSync } from 'fs'; +import { tmpdir } from 'os'; +import { join } from 'path'; +import { downloadFile } from '../../src/update/self-update'; + +const originalFetch = globalThis.fetch; +const tempDirs: string[] = []; + +function makeTempDir(): string { + const dir = mkdtempSync(join(tmpdir(), 'mmx-self-update-test-')); + tempDirs.push(dir); + return dir; +} + +afterEach(() => { + globalThis.fetch = originalFetch; + for (const dir of tempDirs.splice(0)) { + rmSync(dir, { recursive: true, force: true }); + } +}); + +describe('self-update downloadFile', () => { + it('cleans up the temp file when the response stream fails mid-download', async () => { + // Regression for review feedback on PR #158: a network failure mid-stream + // must leave NOTHING behind at `dest` — earlier versions of this path + // left a partial binary in tmpdir after a 5xx mid-stream / TLS reset. + const dir = makeTempDir(); + const destPath = join(dir, 'mmx-update'); + + globalThis.fetch = (async () => { + let sentChunk = false; + const stream = new ReadableStream({ + pull(controller) { + if (!sentChunk) { + sentChunk = true; + controller.enqueue(new TextEncoder().encode('half-of-the-binary')); + return; + } + controller.error(new Error('connection reset by peer')); + }, + }); + return new Response(stream, { + status: 200, + headers: { 'content-length': '1000' }, + }); + }) as unknown as typeof fetch; + + await expect( + downloadFile('https://example.com/mmx-linux-amd64', destPath), + ).rejects.toThrow('connection reset by peer'); + + // The exact assertion the reviewer asked for: the partial file is gone, + // i.e. cleanup runs AFTER the writer is torn down. The directory should + // contain no leftover artefact at all. + expect(existsSync(destPath)).toBe(false); + expect(readdirSync(dir)).toEqual([]); + }); + + it('writes the full payload and waits for the writer to flush on success', async () => { + // Pair-test: the happy path must still resolve with a fully-written file. + // Catches a regression where the new error-path teardown accidentally + // also runs on success and unlinks a valid binary. + const dir = makeTempDir(); + const destPath = join(dir, 'mmx-update'); + const payload = new TextEncoder().encode('the-entire-binary-payload'); + + globalThis.fetch = (async () => new Response(payload, { + status: 200, + headers: { 'content-length': String(payload.byteLength) }, + })) as unknown as typeof fetch; + + await expect( + downloadFile('https://example.com/mmx-linux-amd64', destPath), + ).resolves.toBeUndefined(); + + expect(existsSync(destPath)).toBe(true); + expect(readdirSync(dir)).toEqual(['mmx-update']); + }); + + it('rejects with a CLIError on non-2xx response without creating a file', async () => { + const dir = makeTempDir(); + const destPath = join(dir, 'mmx-update'); + + globalThis.fetch = (async () => new Response('not found', { + status: 404, + statusText: 'Not Found', + })) as unknown as typeof fetch; + + await expect( + downloadFile('https://example.com/missing', destPath), + ).rejects.toThrow(/Download failed: 404/); + + expect(existsSync(destPath)).toBe(false); + expect(readdirSync(dir)).toEqual([]); + }); +});