From 82108b0da32c3974a7d30946d52a06ae03285293 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 29 Oct 2026 15:27:09 -0700 Subject: [PATCH] fix(wsl): uninstall ends a recorded holder its command line proves, even without a start time (#36126) --- .../wsl-holder-uninstall-preflight.test.ts | 7 +- .../startup/wsl-holder-uninstall-preflight.ts | 2 +- ...sl-distro-holder-process-classification.ts | 12 ++- src/main/wsl/wsl-distro-holder-types.ts | 8 +- src/main/wsl/wsl-distro-holder.test.ts | 74 +++++++++++++++++++ src/main/wsl/wsl-distro-holder.ts | 34 +++++++-- 6 files changed, 121 insertions(+), 16 deletions(-) diff --git a/src/main/startup/wsl-holder-uninstall-preflight.test.ts b/src/main/startup/wsl-holder-uninstall-preflight.test.ts index 9665b694f00..d5ea35eac66 100644 --- a/src/main/startup/wsl-holder-uninstall-preflight.test.ts +++ b/src/main/startup/wsl-holder-uninstall-preflight.test.ts @@ -44,9 +44,10 @@ function holder(release: () => Promise): WslDistroHol describe('WSL holder release on uninstall', () => { it('succeeds only when every recorded holder is proven gone', async () => { vi.spyOn(console, 'warn').mockImplementation(() => {}) - await expect( - releaseWslHoldersForUninstall(holder(async () => [{ distro: 'ubuntu', state: 'exited' }])) - ).resolves.toBe(0) + const released = holder(async () => [{ distro: 'ubuntu', state: 'exited' }]) + await expect(releaseWslHoldersForUninstall(released)).resolves.toBe(0) + // Uninstall trusts a matching holder command line; nothing later could release it. + expect(released.releaseAll).toHaveBeenCalledWith({ forUninstall: true }) await expect(releaseWslHoldersForUninstall(holder(async () => []))).resolves.toBe(0) await expect( releaseWslHoldersForUninstall( diff --git a/src/main/startup/wsl-holder-uninstall-preflight.ts b/src/main/startup/wsl-holder-uninstall-preflight.ts index dba5a78afa3..a1148db9d36 100644 --- a/src/main/startup/wsl-holder-uninstall-preflight.ts +++ b/src/main/startup/wsl-holder-uninstall-preflight.ts @@ -11,7 +11,7 @@ const UNINSTALL_RELEASE_TIMEOUT_MS = 30_000 /** Exit code 0 only when every recorded holder is proven gone; unverifiable ones are left running. */ export async function releaseWslHoldersForUninstall(holder: WslDistroHolder): Promise { try { - const released = await holder.releaseAll() + const released = await holder.releaseAll({ forUninstall: true }) for (const { distro, state } of released) { if (state !== 'exited') { console.warn(`[wsl] uninstall left the ${distro} holder ${state}`) diff --git a/src/main/wsl/wsl-distro-holder-process-classification.ts b/src/main/wsl/wsl-distro-holder-process-classification.ts index d08d6c9ea27..d0af3d20678 100644 --- a/src/main/wsl/wsl-distro-holder-process-classification.ts +++ b/src/main/wsl/wsl-distro-holder-process-classification.ts @@ -3,10 +3,14 @@ import { isWslDistroHolderCommandLine } from './wsl-distro-holder-command' import type { WslDistroHolderRecord } from './wsl-distro-holder-record' import type { WslDistroHolderState } from './wsl-distro-holder-types' -/** Pid + creation time + command line must all agree; any one contradicting means pid reuse. */ +/** + * Pid + creation time + command line must all agree; any one contradicting means pid reuse. + * `commandLineSuffices` (uninstall only): a matching command line is enough when a time is missing. + */ export function classifyWslDistroHolderProcess( record: WslDistroHolderRecord, - evidence: WindowsProcessEvidence + evidence: WindowsProcessEvidence, + { commandLineSuffices = false } = {} ): WslDistroHolderState { if (evidence.status === 'unavailable') { return 'unverifiable' @@ -25,5 +29,7 @@ export function classifyWslDistroHolderProcess( return 'exited' } // A record written without a creation time never had one to compare; the command line decides. - return timeKnown || record.creationTimeMs === null ? 'live' : 'unverifiable' + return timeKnown || record.creationTimeMs === null || commandLineSuffices + ? 'live' + : 'unverifiable' } diff --git a/src/main/wsl/wsl-distro-holder-types.ts b/src/main/wsl/wsl-distro-holder-types.ts index 2e34f80403b..6131bac172a 100644 --- a/src/main/wsl/wsl-distro-holder-types.ts +++ b/src/main/wsl/wsl-distro-holder-types.ts @@ -16,6 +16,12 @@ export type WslDistroHolderChild = EventEmitter & { readonly pid?: number; unref export type WslDistroHolderAdoption = { distro: string; state: WslDistroHolderState } +/** + * Uninstall only: nothing will ever release a holder left now, so its marker and distro on the + * command line prove it ours without a start time, and an unreadable table is read once more. + */ +export type WslDistroHolderReleaseOptions = { forUninstall?: boolean } + export type WslDistroHolderDependencies = { userDataPath: string /** Test seam; production spawns `wsl.exe` detached through process-host. */ @@ -54,6 +60,6 @@ export type WslDistroHolder = { /** Explicit stop only; the distro then idles out on WSL's own schedule. */ release(distro: string): Promise /** Every recorded holder (uninstall, hosts turned off); kills only verified holders. */ - releaseAll(): Promise + releaseAll(options?: WslDistroHolderReleaseOptions): Promise holderState(distro: string): Promise } diff --git a/src/main/wsl/wsl-distro-holder.test.ts b/src/main/wsl/wsl-distro-holder.test.ts index 426acb93696..579c1be199e 100644 --- a/src/main/wsl/wsl-distro-holder.test.ts +++ b/src/main/wsl/wsl-distro-holder.test.ts @@ -463,3 +463,77 @@ describe('createWslDistroHolder', () => { expect(onHolderExit).toHaveBeenCalledWith('Alpine', { code: 1, signal: null }) }) }) + +describe('releaseAll for uninstall', () => { + beforeEach(() => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + }) + + afterEach(() => { + vi.restoreAllMocks() + }) + + it('kills a matching holder with no start time on either side, and logs its pid', async () => { + host.processes.set(77, { commandLine: HOLDER_COMMAND('Alpine'), startedAtMs: 5_000 }) + await writeRecord(record({ creationTimeMs: null })) + host.processes.set(88, { commandLine: HOLDER_COMMAND('Debian'), startedAtMs: null }) + await writeRecord(record({ distro: 'Debian', pid: 88, creationTimeMs: 6_000 })) + const released = await makeHolder().releaseAll({ forUninstall: true }) + expect(released.sort((a, b) => a.distro.localeCompare(b.distro))).toEqual([ + { distro: 'alpine', state: 'exited' }, + { distro: 'debian', state: 'exited' } + ]) + expect(host.killed.sort()).toEqual([77, 88]) + expect(await recordFiles()).toEqual([]) + expect(console.warn).toHaveBeenCalledWith( + '[wsl] uninstall: Alpine holder pid 77 was live, now exited' + ) + }) + + it('never kills a pid whose command line is not the holder for the recorded distro', async () => { + host.processes.set(77, { commandLine: 'C:\\Windows\\notepad.exe', startedAtMs: 5_000 }) + await writeRecord(record({ creationTimeMs: null })) + host.processes.set(88, { commandLine: HOLDER_COMMAND('Ubuntu'), startedAtMs: null }) + await writeRecord(record({ distro: 'Debian', pid: 88, creationTimeMs: null })) + const released = await makeHolder().releaseAll({ forUninstall: true }) + expect(released.sort((a, b) => a.distro.localeCompare(b.distro))).toEqual([ + { distro: 'alpine', state: 'exited' }, + { distro: 'debian', state: 'exited' } + ]) + expect(host.killed).toEqual([]) + }) + + it('still trusts a known start time that contradicts the record', async () => { + host.processes.set(77, { commandLine: HOLDER_COMMAND('Alpine'), startedAtMs: 9_999 }) + await writeRecord(record()) + expect(await makeHolder().releaseAll({ forUninstall: true })).toEqual([ + { distro: 'alpine', state: 'exited' } + ]) + expect(host.killed).toEqual([]) + }) + + it('reads an unavailable process table once more before giving up', async () => { + host.processes.set(77, { commandLine: HOLDER_COMMAND('Alpine'), startedAtMs: 5_000 }) + await writeRecord(record()) + host.tableFails = true + const sleep = vi.fn(async () => { + host.tableFails = false + }) + expect(await makeHolder({ sleep }).releaseAll({ forUninstall: true })).toEqual([ + { distro: 'alpine', state: 'exited' } + ]) + expect(host.killed).toEqual([77]) + + host.processes.set(78, { commandLine: HOLDER_COMMAND('Alpine'), startedAtMs: 5_000 }) + await writeRecord(record({ pid: 78 })) + host.tableFails = true + expect(await makeHolder().releaseAll({ forUninstall: true })).toEqual([ + { distro: 'alpine', state: 'unverifiable' } + ]) + expect(host.killed).toEqual([77]) + expect(await recordFiles()).toEqual(['alpine.json']) + expect(console.warn).toHaveBeenCalledWith( + '[wsl] uninstall: Alpine holder pid 78 was unverifiable, now unverifiable' + ) + }) +}) diff --git a/src/main/wsl/wsl-distro-holder.ts b/src/main/wsl/wsl-distro-holder.ts index 41bc443500b..9f0e0888a89 100644 --- a/src/main/wsl/wsl-distro-holder.ts +++ b/src/main/wsl/wsl-distro-holder.ts @@ -31,6 +31,7 @@ import type { WslDistroHolderChild, WslDistroHolderDependencies, WslDistroHolderEnsureResult, + WslDistroHolderReleaseOptions, WslDistroHolderState } from './wsl-distro-holder-types' @@ -187,11 +188,20 @@ export function createWslDistroHolder(dependencies: WslDistroHolderDependencies) async function stopRecordedHolder( record: WslDistroHolderRecord, - { spawnedHere = false } = {} + { spawnedHere = false, forUninstall = false } = {} ): Promise { - let state = classifyWslDistroHolderProcess(record, await queryProcess(record.pid)) + const classify = async (): Promise => + classifyWslDistroHolderProcess(record, await queryProcess(record.pid), { + commandLineSuffices: forUninstall + }) + let state = await classify() + if (state === 'unverifiable' && forUninstall) { + await sleep(BOOT_POLL_INTERVAL_MS) + state = await classify() + } // A command line alone can't tell our holder from a reused pid running the same argv. - const provenOurs = spawnedHere || record.creationTimeMs !== null + const provenOurs = spawnedHere || record.creationTimeMs !== null || forUninstall + const stateBeforeKill = state if (state === 'live' && provenOurs) { try { // WSL 2.7.10 relaunches a same-argv child; killing the recorded parent ends both. @@ -201,7 +211,7 @@ export function createWslDistroHolder(dependencies: WslDistroHolderDependencies) } const deadline = monotonicNow() + RELEASE_EXIT_TIMEOUT_MS for (;;) { - state = classifyWslDistroHolderProcess(record, await queryProcess(record.pid)) + state = await classify() // Mid-exit rows may retain PID but lose command line; only exited proves termination. if (state === 'exited' || monotonicNow() >= deadline) { break @@ -212,6 +222,11 @@ export function createWslDistroHolder(dependencies: WslDistroHolderDependencies) if (state === 'exited') { await removeWslDistroHolderRecord(userDataPath, record.distro) } + if (forUninstall) { + console.warn( + `[wsl] uninstall: ${record.distro} holder pid ${record.pid} was ${stateBeforeKill}, now ${state}` + ) + } return state === 'live' && !provenOurs ? 'unverifiable' : state } @@ -289,10 +304,10 @@ export function createWslDistroHolder(dependencies: WslDistroHolderDependencies) release: (distro) => releaseRecorded(distro), - async releaseAll() { + async releaseAll(options) { const distros = await listWslDistroHolderRecordDistros(userDataPath) return Promise.all( - distros.map(async (distro) => ({ distro, state: await releaseRecorded(distro) })) + distros.map(async (distro) => ({ distro, state: await releaseRecorded(distro, options) })) ) }, @@ -302,7 +317,10 @@ export function createWslDistroHolder(dependencies: WslDistroHolderDependencies) } return holder - function releaseRecorded(distro: string): Promise { + function releaseRecorded( + distro: string, + options: WslDistroHolderReleaseOptions = {} + ): Promise { return serialize(distro, async () => { const read = await readWslDistroHolderRecord(userDataPath, distro) if (read.status === 'unreadable') { @@ -312,7 +330,7 @@ export function createWslDistroHolder(dependencies: WslDistroHolderDependencies) await removeWslDistroHolderRecord(userDataPath, distro) return 'exited' } - return stopRecordedHolder(read.record) + return stopRecordedHolder(read.record, options) }) } }