From 962442ccd5f1fa3f0120976c39cba033a0f7d4ef Mon Sep 17 00:00:00 2001 From: sudacode Date: Thu, 10 Sep 2026 13:32:12 -0700 Subject: [PATCH] fix(launcher): address review feedback on setup and runtime cleanup --- scripts/package-bun-source.mjs | 34 +++++++------- scripts/package-bun-source.test.ts | 28 ++++++++++++ src/main.ts | 5 +-- src/main/runtime/managed-launcher.test.ts | 30 +++++++++++++ src/main/runtime/managed-launcher.ts | 2 +- .../runtime/update/launcher-updater.test.ts | 45 ++++++++++++++++--- src/main/runtime/update/launcher-updater.ts | 17 +++---- 7 files changed, 126 insertions(+), 35 deletions(-) diff --git a/scripts/package-bun-source.mjs b/scripts/package-bun-source.mjs index 6aefeaae..1d88d3c6 100644 --- a/scripts/package-bun-source.mjs +++ b/scripts/package-bun-source.mjs @@ -207,7 +207,7 @@ async function run(command, args, options = {}) { }); } -async function downloadArchive(source, cacheDir, fetchImpl) { +export async function downloadArchive(source, cacheDir, fetchImpl) { const archivePath = path.join(cacheDir, `${source.name}-${source.revision}.tar.gz`); try { if ((await sha256File(archivePath)) === source.sha256) return archivePath; @@ -218,22 +218,26 @@ async function downloadArchive(source, cacheDir, fetchImpl) { const url = `https://codeload.github.com/${source.repository}/tar.gz/${source.revision}`; const temporaryPath = `${archivePath}.download-${randomUUID()}`; - const response = await fetchImpl(url); - if (!response.ok || !response.body) - throw new Error(`Unable to download ${url}: HTTP ${response.status}`); - await pipeline( - Readable.fromWeb(response.body), - createWriteStream(temporaryPath, { flags: 'wx' }), - ); - const actualSha256 = await sha256File(temporaryPath); - if (actualSha256 !== source.sha256) { - await fs.rm(temporaryPath, { force: true }); - throw new Error( - `Source checksum mismatch for ${source.name}: expected ${source.sha256}, received ${actualSha256}.`, + try { + const response = await fetchImpl(url); + if (!response.ok || !response.body) + throw new Error(`Unable to download ${url}: HTTP ${response.status}`); + await pipeline( + Readable.fromWeb(response.body), + createWriteStream(temporaryPath, { flags: 'wx' }), ); + const actualSha256 = await sha256File(temporaryPath); + if (actualSha256 !== source.sha256) { + throw new Error( + `Source checksum mismatch for ${source.name}: expected ${source.sha256}, received ${actualSha256}.`, + ); + } + await fs.rename(temporaryPath, archivePath); + return archivePath; + } finally { + // Cleanup must not replace the original download, validation, or rename error. + await fs.rm(temporaryPath, { force: true }).catch(() => {}); } - await fs.rename(temporaryPath, archivePath); - return archivePath; } async function materializeArchive(source, root, cacheDir, fetchImpl) { diff --git a/scripts/package-bun-source.test.ts b/scripts/package-bun-source.test.ts index 4ed655c9..d361523a 100644 --- a/scripts/package-bun-source.test.ts +++ b/scripts/package-bun-source.test.ts @@ -1,7 +1,10 @@ import { describe, expect, test } from 'bun:test'; +import { createHash } from 'node:crypto'; import fs from 'node:fs/promises'; +import os from 'node:os'; import path from 'node:path'; import { + downloadArchive, parseRegisteredRepositories, parseSourceManifest, validateBunPins, @@ -11,6 +14,31 @@ import { const projectRoot = path.resolve(import.meta.dir, '..'); describe('Bun corresponding-source manifest', () => { + test('removes partial downloads when placing a valid archive fails', async () => { + const cacheDir = await fs.mkdtemp(path.join(os.tmpdir(), 'subminer-bun-source-test-')); + try { + const payload = new Uint8Array([1, 2, 3]); + const source = { + name: 'fixture', + repository: 'example/fixture', + revision: 'a'.repeat(40), + sha256: createHash('sha256').update(payload).digest('hex'), + }; + const archivePath = path.join(cacheDir, `${source.name}-${source.revision}.tar.gz`); + await expect( + downloadArchive(source, cacheDir, async () => { + await fs.mkdir(archivePath); + return { ok: true, status: 200, body: new Response(payload).body }; + }), + ).rejects.toMatchObject({ syscall: 'rename' }); + + const entries = await fs.readdir(cacheDir); + expect(entries.filter((entry) => entry.includes('.download-'))).toEqual([]); + } finally { + await fs.rm(cacheDir, { recursive: true, force: true }); + } + }); + test('pins the source to the revision reported by the distributed binary', async () => { const manifest = parseSourceManifest( JSON.parse( diff --git a/src/main.ts b/src/main.ts index d9fb1533..f20a24ab 100644 --- a/src/main.ts +++ b/src/main.ts @@ -1523,10 +1523,7 @@ const firstRunSetupService = createFirstRunSetupService({ }, installCommandLineLauncher: async () => { const snapshot = await installCommandLineLauncher(createCommandLineLauncherRuntimeOptions()); - const ok = - snapshot.status === 'ready' || - snapshot.status === 'installed_bun_missing' || - snapshot.status === 'not_on_path'; + const ok = snapshot.status === 'ready' || snapshot.status === 'not_on_path'; return { ok, installPath: snapshot.installPath, diff --git a/src/main/runtime/managed-launcher.test.ts b/src/main/runtime/managed-launcher.test.ts index 7f705cda..31b942a0 100644 --- a/src/main/runtime/managed-launcher.test.ts +++ b/src/main/runtime/managed-launcher.test.ts @@ -305,3 +305,33 @@ test('Windows stages a new runtime version while the prior Bun executable is run assert.equal(fs.existsSync(path.dirname(first.bunPath)), false); assert.ok(fs.existsSync(expectedSecond.bunPath)); }); + +test('Windows cleanup removes an obsolete runtime with no Bun executable', (t) => { + if (process.platform !== 'win32') return; + const root = workspace(t); + const current = windowsManagedRuntimePaths({ + platform: 'win32', + localAppData: root, + appVersion: '2.0.0', + }); + const obsolete = windowsManagedRuntimePaths({ + platform: 'win32', + localAppData: root, + appVersion: '1.0.0', + }); + fs.mkdirSync(path.join(path.dirname(obsolete.bunPath), 'licenses'), { recursive: true }); + fs.writeFileSync( + path.join(path.dirname(obsolete.bunPath), 'licenses', 'Bun-LICENSE.md'), + 'license', + ); + fs.mkdirSync(path.dirname(current.bunPath), { recursive: true }); + + cleanupOldWindowsManagedRuntimes({ + platform: 'win32', + localAppData: root, + appVersion: '2.0.0', + }); + + assert.equal(fs.existsSync(path.dirname(obsolete.bunPath)), false); + assert.ok(fs.existsSync(path.dirname(current.bunPath))); +}); diff --git a/src/main/runtime/managed-launcher.ts b/src/main/runtime/managed-launcher.ts index 0f2ece8f..0afa66d8 100644 --- a/src/main/runtime/managed-launcher.ts +++ b/src/main/runtime/managed-launcher.ts @@ -101,7 +101,7 @@ export function cleanupOldWindowsManagedRuntimes( if (!entry.isDirectory() || entry.name === path.win32.basename(paths.directory)) continue; const oldDirectory = path.win32.join(paths.rootDirectory, entry.name); try { - fs.rmSync(path.win32.join(oldDirectory, 'bun.exe')); + fs.rmSync(path.win32.join(oldDirectory, 'bun.exe'), { force: true }); } catch { continue; } diff --git a/src/main/runtime/update/launcher-updater.test.ts b/src/main/runtime/update/launcher-updater.test.ts index adbfaaaa..71c0ea7d 100644 --- a/src/main/runtime/update/launcher-updater.test.ts +++ b/src/main/runtime/update/launcher-updater.test.ts @@ -160,7 +160,7 @@ test('app-managed wrappers are never overwritten by the standalone release scrip }); test('GUI updates defer recognized standalone launcher migration to app startup', async () => { - let accessed = false; + const accessed: string[] = []; let downloaded = false; const result = await updateLauncherAtPath({ launcherPath: '/home/tester/.local/bin/subminer', @@ -174,8 +174,8 @@ test('GUI updates defer recognized standalone launcher migration to app startup' fs: { stat: async () => ({ isFile: () => true }), readFile: async () => Buffer.from('#!/bin/sh\n# SubMiner launcher\n'), - access: async () => { - accessed = true; + access: async (targetPath) => { + accessed.push(targetPath); }, writeFile: async () => {}, chmod: async () => {}, @@ -189,7 +189,7 @@ test('GUI updates defer recognized standalone launcher migration to app startup' path: '/home/tester/.local/bin/subminer', message: 'Launcher migration is deferred until the updated SubMiner app starts.', }); - assert.equal(accessed, false); + assert.deepEqual(accessed, ['/home/tester/.local/bin/subminer', '/home/tester/.local/bin']); assert.equal(downloaded, false); }); @@ -213,9 +213,7 @@ test('release launcher updater propagates GUI migration deferral', async () => { fs: { stat: async () => ({ isFile: () => true }), readFile: async () => Buffer.from('#!/bin/sh\n# SubMiner launcher\n'), - access: async () => { - throw new Error('must not check writability before app startup'); - }, + access: async () => {}, writeFile: async () => {}, chmod: async () => {}, rename: async () => {}, @@ -227,3 +225,36 @@ test('release launcher updater propagates GUI migration deferral', async () => { assert.match(result.message ?? '', /deferred until the updated SubMiner app starts/); assert.equal(downloaded, false); }); + +test('GUI migration reports a protected launcher when its file or parent is not writable', async () => { + const launcherPath = '/usr/local/bin/subminer'; + for (const protectedPath of [launcherPath, '/usr/local/bin']) { + const result = await updateLauncherAtPath({ + launcherPath, + assetUrl: 'https://example.test/subminer', + expectedSha256: launcherHash, + deferRecognizedLauncherUpdate: true, + download: async () => { + throw new Error('Protected launchers must not download a replacement.'); + }, + fs: { + stat: async () => ({ isFile: () => true }), + readFile: async () => Buffer.from('#!/usr/bin/env bun\n// SubMiner launcher\n'), + access: async (targetPath) => { + if (targetPath === protectedPath) throw new Error('EACCES'); + }, + writeFile: async () => { + throw new Error('Protected launchers must not be written.'); + }, + chmod: async () => {}, + rename: async () => {}, + unlink: async () => {}, + }, + }); + assert.equal(result.status, 'protected', protectedPath); + assert.equal( + result.command, + buildProtectedLauncherUpdateCommand('https://example.test/subminer', launcherPath), + ); + } +}); diff --git a/src/main/runtime/update/launcher-updater.ts b/src/main/runtime/update/launcher-updater.ts index 0be6bf33..f1583cab 100644 --- a/src/main/runtime/update/launcher-updater.ts +++ b/src/main/runtime/update/launcher-updater.ts @@ -112,16 +112,9 @@ export async function updateLauncherAtPath(options: { message: 'Existing executable does not look like a SubMiner launcher.', }; } - if (options.deferRecognizedLauncherUpdate) { - return { - status: 'skipped', - path: options.launcherPath, - message: 'Launcher migration is deferred until the updated SubMiner app starts.', - }; - } - try { await fsDeps.access(options.launcherPath); + await fsDeps.access(path.dirname(options.launcherPath)); } catch { return { status: 'protected', @@ -130,6 +123,14 @@ export async function updateLauncherAtPath(options: { }; } + if (options.deferRecognizedLauncherUpdate) { + return { + status: 'skipped', + path: options.launcherPath, + message: 'Launcher migration is deferred until the updated SubMiner app starts.', + }; + } + const data = await options.download(); const actualSha256 = sha256(data); if (actualSha256 !== options.expectedSha256.toLowerCase()) {