From 1b8f1504b32177ce1e12f47d8a618b320e3baa29 Mon Sep 17 00:00:00 2001 From: ethernet Date: Wed, 8 Jul 2026 10:14:29 -0400 Subject: [PATCH] test(desktop): extract profile-delete routing decision for real unit tests profile-delete-respawn.test.ts regexed main.ts source text to check that prepareProfileDeleteRequest returns the torn-down profile name, and that the hermes:api ipcMain handler captures that return value and routes to the primary backend instead of respawning a pool backend for the just- deleted profile. Extract the pure decision logic into profile-delete-routing.ts (no Electron import): - profileNameFromDeleteRequest(request): parses a DELETE /api/profiles/ path, moved verbatim (already pure). - decideProfileDeleteAction(profile, deps): the branch decision (noop / teardown-primary / teardown-pool) and the profile name to return, parameterized over isDefaultProfile/isValidProfileName/primaryProfileKey. - resolveRouteProfile(tornDownProfile, profile): the routing ternary from the hermes:api handler. prepareProfileDeleteRequest in main.ts now calls decideProfileDeleteAction for the decision and only performs the async side effects (teardown + writeActiveDesktopProfile) the decision calls for. profile-delete-routing.test.ts replaces profile-delete-respawn.test.ts (which was never wired into test:desktop:platforms), importing the pure functions directly and asserting real return values across every branch -- no readFileSync, no regex-on-source. Wired the new test file into test:desktop:platforms in package.json. --- apps/desktop/electron/main.ts | 56 ++++------- .../electron/profile-delete-respawn.test.ts | 62 ------------ .../electron/profile-delete-routing.test.ts | 83 ++++++++++++++++ .../electron/profile-delete-routing.ts | 97 +++++++++++++++++++ apps/desktop/package.json | 2 +- 5 files changed, 198 insertions(+), 102 deletions(-) delete mode 100644 apps/desktop/electron/profile-delete-respawn.test.ts create mode 100644 apps/desktop/electron/profile-delete-routing.test.ts create mode 100644 apps/desktop/electron/profile-delete-routing.ts diff --git a/apps/desktop/electron/main.ts b/apps/desktop/electron/main.ts index 986208084..a870e8bb3 100644 --- a/apps/desktop/electron/main.ts +++ b/apps/desktop/electron/main.ts @@ -28,7 +28,7 @@ import { systemPreferences } from 'electron' import nodePty from 'node-pty' - +import { decideProfileDeleteAction, profileNameFromDeleteRequest, resolveRouteProfile } from './profile-delete-routing' import { stopBackendChild as stopBackendChildImpl } from './backend-child' import { dashboardFallbackArgs, sourceDeclaresServe } from './backend-command' import { buildDesktopBackendEnv, normalizeHermesHomeRoot } from './backend-env' @@ -6612,59 +6612,37 @@ function stopAllPoolBackends() { } } -function profileNameFromDeleteRequest(request) { - if (!request || String(request.method || 'GET').toUpperCase() !== 'DELETE') { - return null - } - - const match = String(request.path || '').match(/^\/api\/profiles\/([^/?#]+)(?:[?#].*)?$/) - - if (!match) { - return null - } - - let raw = '' - - try { - raw = decodeURIComponent(match[1]) - } catch { - return null - } - - const name = raw.trim() - - if (!name) { - return null - } - - if (name.toLowerCase() === 'default') { - return 'default' - } - - return name.toLowerCase() -} - // Returns the profile name whose backend was torn down, or null when the // request is not a profile-delete. The caller uses this to skip ensureBackend // for the just-torn-down profile — otherwise ensureBackend respawns a pool // backend whose ensure_hermes_home() recreates the deleted profile directory. +// +// The routing *decision* (which branch fires, what profile name gets +// returned) lives in the pure decideProfileDeleteAction() in +// profile-delete-routing.ts; this function only performs the side effects +// that decision calls for. async function prepareProfileDeleteRequest(request) { const profile = profileNameFromDeleteRequest(request) + const decision = decideProfileDeleteAction(profile, { + isDefaultProfile: p => p === 'default', + isValidProfileName: p => PROFILE_NAME_RE.test(p), + primaryProfileKey + }) - if (!profile || profile === 'default' || !PROFILE_NAME_RE.test(profile)) { + if (decision.action === 'noop') { return null } - if (profile === primaryProfileKey()) { + if (decision.action === 'teardown-primary') { writeActiveDesktopProfile('default') await teardownPrimaryBackendAndWait() - return profile + return decision.profile } - await teardownPoolBackendAndWait(profile) + await teardownPoolBackendAndWait(decision.profile) - return profile + return decision.profile } async function startHermes() { @@ -7849,7 +7827,7 @@ ipcMain.handle('hermes:api', async (_event, request) => { // backend instead of spawning a fresh pool backend. A freshly spawned // backend calls ensure_hermes_home() which recreates the profile directory, // defeating the deletion and leaving a zombie process. - const routeProfile = tornDownProfile ? null : profile + const routeProfile = resolveRouteProfile(tornDownProfile, profile) const connection = await ensureBackend(routeProfile) const timeoutMs = resolveTimeoutMs(request?.timeoutMs, DEFAULT_FETCH_TIMEOUT_MS) diff --git a/apps/desktop/electron/profile-delete-respawn.test.ts b/apps/desktop/electron/profile-delete-respawn.test.ts deleted file mode 100644 index 2fbddfc5a..000000000 --- a/apps/desktop/electron/profile-delete-respawn.test.ts +++ /dev/null @@ -1,62 +0,0 @@ -'use strict' - -import assert from 'node:assert/strict' -import fs from 'node:fs' -import path from 'node:path' -import test from 'node:test' - -const ELECTRON_DIR = import.meta.dirname - -function readElectronFile(name) { - return fs.readFileSync(path.join(ELECTRON_DIR, name), 'utf8').replace(/\r\n/g, '\n') -} - -// --------------------------------------------------------------------------- -// prepareProfileDeleteRequest must return the torn-down profile name so the -// caller can skip ensureBackend for that profile (issue #52279). -// --------------------------------------------------------------------------- - -test('prepareProfileDeleteRequest returns the torn-down profile name', () => { - const source = readElectronFile('main.ts') - - // Locate the function definition and its closing brace. - const fnStart = source.indexOf('async function prepareProfileDeleteRequest(') - assert.notEqual(fnStart, -1, 'prepareProfileDeleteRequest function not found') - - // The function must contain "return profile" (pool and primary paths). - const fnBody = source.slice(fnStart, fnStart + 800) - const returnProfileCount = (fnBody.match(/return profile/g) || []).length - assert.ok( - returnProfileCount >= 2, - `expected at least 2 "return profile" statements (primary + pool paths), found ${returnProfileCount}` - ) - - // The early-exit guard must return null (not void/undefined). - assert.match(fnBody, /return null/, 'early-exit guard should return null, not undefined') -}) - -test('hermes:api handler routes profile-delete requests to the primary backend', () => { - const source = readElectronFile('main.ts') - - // The handler must capture prepareProfileDeleteRequest's return value. - assert.match( - source, - /const tornDownProfile = await prepareProfileDeleteRequest\(request\)/, - 'handler should capture the return value of prepareProfileDeleteRequest' - ) - - // The handler must use the return value to skip ensureBackend for the - // torn-down profile, routing to the primary (null) instead. - assert.match( - source, - /const routeProfile = tornDownProfile \? null : profile/, - 'handler should route to primary backend when a profile was just torn down' - ) - - // ensureBackend must be called with the conditional route profile. - assert.match( - source, - /const connection = await ensureBackend\(routeProfile\)/, - 'handler should pass routeProfile (not raw profile) to ensureBackend' - ) -}) diff --git a/apps/desktop/electron/profile-delete-routing.test.ts b/apps/desktop/electron/profile-delete-routing.test.ts new file mode 100644 index 000000000..b22859c2c --- /dev/null +++ b/apps/desktop/electron/profile-delete-routing.test.ts @@ -0,0 +1,83 @@ +'use strict' + +import assert from 'node:assert/strict' +import test from 'node:test' + +import { decideProfileDeleteAction, profileNameFromDeleteRequest, resolveRouteProfile } from './profile-delete-routing' + +// --------------------------------------------------------------------------- +// profileNameFromDeleteRequest +// --------------------------------------------------------------------------- + +test('profileNameFromDeleteRequest parses a DELETE /api/profiles/ path', () => { + assert.equal(profileNameFromDeleteRequest({ method: 'DELETE', path: '/api/profiles/worker' }), 'worker') +}) + +test('profileNameFromDeleteRequest lowercases the profile name', () => { + assert.equal(profileNameFromDeleteRequest({ method: 'DELETE', path: '/api/profiles/Worker' }), 'worker') +}) + +test('profileNameFromDeleteRequest returns null for non-DELETE methods', () => { + assert.equal(profileNameFromDeleteRequest({ method: 'GET', path: '/api/profiles/worker' }), null) +}) + +test('profileNameFromDeleteRequest returns null when the path does not match', () => { + assert.equal(profileNameFromDeleteRequest({ method: 'DELETE', path: '/api/sessions' }), null) +}) + +test('profileNameFromDeleteRequest returns null for an empty/whitespace name', () => { + assert.equal(profileNameFromDeleteRequest({ method: 'DELETE', path: '/api/profiles/%20' }), null) +}) + +test('profileNameFromDeleteRequest returns null for an undecodable path segment', () => { + assert.equal(profileNameFromDeleteRequest({ method: 'DELETE', path: '/api/profiles/%E0%A4%A' }), null) +}) + +// --------------------------------------------------------------------------- +// decideProfileDeleteAction +// --------------------------------------------------------------------------- + +const deps = { + isDefaultProfile: p => p === 'default', + isValidProfileName: p => /^[a-z0-9][a-z0-9_-]{0,63}$/.test(p), + primaryProfileKey: () => 'primary-profile' +} + +test('decideProfileDeleteAction is a noop for the default profile', () => { + assert.deepEqual(decideProfileDeleteAction('default', deps), { action: 'noop', profile: null }) +}) + +test('decideProfileDeleteAction is a noop for null (no profile parsed)', () => { + assert.deepEqual(decideProfileDeleteAction(null, deps), { action: 'noop', profile: null }) +}) + +test('decideProfileDeleteAction is a noop for an invalid profile name', () => { + assert.deepEqual(decideProfileDeleteAction('Not Valid!', deps), { action: 'noop', profile: null }) +}) + +test('decideProfileDeleteAction tears down the primary backend for the primary profile', () => { + assert.deepEqual(decideProfileDeleteAction('primary-profile', deps), { + action: 'teardown-primary', + profile: 'primary-profile' + }) +}) + +test('decideProfileDeleteAction tears down the pool backend for any other valid profile', () => { + assert.deepEqual(decideProfileDeleteAction('worker', deps), { action: 'teardown-pool', profile: 'worker' }) +}) + +// --------------------------------------------------------------------------- +// resolveRouteProfile +// --------------------------------------------------------------------------- + +test('resolveRouteProfile routes to the primary backend (null) when a profile was torn down', () => { + assert.equal(resolveRouteProfile('worker', 'other-profile'), null) +}) + +test('resolveRouteProfile passes the requested profile through when nothing was torn down', () => { + assert.equal(resolveRouteProfile(null, 'other-profile'), 'other-profile') +}) + +test('resolveRouteProfile passes through undefined when nothing was torn down and no profile was requested', () => { + assert.equal(resolveRouteProfile(null, undefined), undefined) +}) diff --git a/apps/desktop/electron/profile-delete-routing.ts b/apps/desktop/electron/profile-delete-routing.ts new file mode 100644 index 000000000..f16a67fba --- /dev/null +++ b/apps/desktop/electron/profile-delete-routing.ts @@ -0,0 +1,97 @@ +'use strict' + +// Profile-delete routing logic for the `hermes:api` IPC handler. +// +// When the renderer issues DELETE /api/profiles/, the handler must +// tear down that profile's backend (primary window backend or pool backend) +// and then route the *next* request away from the just-deleted profile's +// pool backend -- spawning a fresh one would call ensure_hermes_home() and +// recreate the profile directory the delete just removed, leaving a zombie +// process behind (issue #52279). +// +// These helpers are pure so they can be unit-tested without Electron. + +/** + * Parse a `hermes:api` request into the profile name a DELETE targets, or + * null when the request is not a profile-delete at all (wrong method, wrong + * path, empty/invalid name). + */ +export function profileNameFromDeleteRequest(request) { + if (!request || String(request.method || 'GET').toUpperCase() !== 'DELETE') { + return null + } + + const match = String(request.path || '').match(/^\/api\/profiles\/([^/?#]+)(?:[?#].*)?$/) + + if (!match) { + return null + } + + let raw = '' + + try { + raw = decodeURIComponent(match[1]) + } catch { + return null + } + + const name = raw.trim() + + if (!name) { + return null + } + + if (name.toLowerCase() === 'default') { + return 'default' + } + + return name.toLowerCase() +} + +export type ProfileDeleteAction = 'noop' | 'teardown-primary' | 'teardown-pool' + +export interface ProfileDeleteDecision { + action: ProfileDeleteAction + profile: string | null +} + +export interface ProfileDeleteDecisionDeps { + isDefaultProfile: (profile: string) => boolean + isValidProfileName: (profile: string) => boolean + primaryProfileKey: () => string +} + +/** + * Pure decision logic for prepareProfileDeleteRequest: given the parsed + * profile name (or null), decide which side-effecting branch the caller + * should take and what profile name it should ultimately report as + * torn-down. No I/O, no async -- the caller performs the actual teardown + * based on `action`. + */ +export function decideProfileDeleteAction( + profile: string | null, + deps: ProfileDeleteDecisionDeps +): ProfileDeleteDecision { + if (!profile || deps.isDefaultProfile(profile) || !deps.isValidProfileName(profile)) { + return { action: 'noop', profile: null } + } + + if (profile === deps.primaryProfileKey()) { + return { action: 'teardown-primary', profile } + } + + return { action: 'teardown-pool', profile } +} + +/** + * Route the next `hermes:api` request away from the primary/window backend + * whenever a profile was just torn down -- otherwise ensureBackend would + * spawn a fresh pool backend for the deleted profile, whose + * ensure_hermes_home() recreates the directory the delete just removed. + */ +export function resolveRouteProfile( + tornDownProfile: string | null, + profile: string | undefined +): string | null | undefined { + return tornDownProfile ? null : profile +} diff --git a/apps/desktop/package.json b/apps/desktop/package.json index 9269dce25..9a67c7fdd 100644 --- a/apps/desktop/package.json +++ b/apps/desktop/package.json @@ -38,7 +38,7 @@ "test:desktop:nsis": "node scripts/test-desktop.mjs nsis", "test:desktop:existing": "node scripts/test-desktop.mjs existing", "test:desktop:fresh": "node scripts/test-desktop.mjs fresh", - "test:desktop:platforms": "node --test electron/bootstrap-platform.test.ts electron/hardening.test.ts electron/backend-env.test.ts electron/backend-probes.test.ts electron/backend-ready.test.ts electron/bootstrap-runner.test.ts electron/connection-config.test.ts electron/dashboard-token.test.ts electron/gateway-ws-probe.test.ts electron/oauth-net-request.test.ts electron/desktop-uninstall.test.ts electron/session-windows.test.ts electron/link-title-window.test.ts electron/workspace-cwd.test.ts electron/fs-read-dir.test.ts electron/git-root.test.ts electron/git-worktree-ops.test.ts electron/windows-child-options.test.ts electron/update-remote.test.ts electron/update-count.test.ts electron/update-rebuild.test.ts electron/update-marker.test.ts electron/update-relaunch.test.ts electron/windows-user-env.test.ts electron/wsl-clipboard-image.test.ts electron/titlebar-overlay-width.test.ts electron/window-state.test.ts electron/zoom.test.ts electron/windows-hermes-path.test.ts electron/oauth-session-request.test.ts", + "test:desktop:platforms": "node --test electron/bootstrap-platform.test.ts electron/hardening.test.ts electron/backend-env.test.ts electron/backend-probes.test.ts electron/backend-ready.test.ts electron/bootstrap-runner.test.ts electron/connection-config.test.ts electron/dashboard-token.test.ts electron/gateway-ws-probe.test.ts electron/oauth-net-request.test.ts electron/desktop-uninstall.test.ts electron/session-windows.test.ts electron/profile-delete-routing.test.ts electron/link-title-window.test.ts electron/workspace-cwd.test.ts electron/fs-read-dir.test.ts electron/git-root.test.ts electron/git-worktree-ops.test.ts electron/windows-child-options.test.ts electron/update-remote.test.ts electron/update-count.test.ts electron/update-rebuild.test.ts electron/update-marker.test.ts electron/update-relaunch.test.ts electron/windows-user-env.test.ts electron/wsl-clipboard-image.test.ts electron/titlebar-overlay-width.test.ts electron/window-state.test.ts electron/zoom.test.ts electron/windows-hermes-path.test.ts electron/oauth-session-request.test.ts", "typecheck": "tsc -p . --noEmit && tsc -p tsconfig.electron.json --noEmit", "lint": "eslint src/ electron/", "lint:fix": "eslint src/ electron/ --fix",