diff --git a/client/src/components/apps/tabs/UpdateTab.jsx b/client/src/components/apps/tabs/UpdateTab.jsx index 55d89ae27e..57274c68cd 100644 --- a/client/src/components/apps/tabs/UpdateTab.jsx +++ b/client/src/components/apps/tabs/UpdateTab.jsx @@ -22,6 +22,7 @@ const STEP_LABELS = { build: 'Building client', restart: 'Restarting PortOS', restarting: 'Restarting PortOS', + verify: 'Verifying PortOS is back', complete: 'Complete' }; @@ -29,6 +30,9 @@ function StepIndicator({ status }) { if (status === 'running') return ; if (status === 'done') return ; if (status === 'error') return ; + // The post-restart health check reports 'warning' when it could not confirm + // the server came back — the update finished, but the install may be down. + if (status === 'warning') return ; return ; } diff --git a/docs/MANAGED_APP_UPDATES.md b/docs/MANAGED_APP_UPDATES.md index e5a06f436b..7e4fdde0ed 100644 --- a/docs/MANAGED_APP_UPDATES.md +++ b/docs/MANAGED_APP_UPDATES.md @@ -26,3 +26,18 @@ database migrations, generated assets, and build. Use the dedicated package-manager behavior is never invoked merely because PortOS updated it. When more than one is present, the configured **Update Command** wins, then `portos:update`, then the conventional script. + +## PortOS is itself a managed app + +The PortOS record appears in App Management like any other app, so **Update** +there runs the same `appUpdater` flow described above. It is the one app whose +update routine deletes the process running that flow, so it takes a different +launcher: the conventional-script branch delegates to `executeUpdate()` in +`server/services/updateExecutor.js`, whose double-fork keeps `update.sh` alive +through its own `pm2 delete` step, and the trailing PM2 restart is skipped +because the script starts the ecosystem itself. See +[Self-Update Flow](SELF_UPDATE.md#every-portos-update-goes-through-the-detached-launcher). + +A custom **Update Command** on the PortOS record keeps the ordinary attached +path, since delegating would silently run `update.sh` instead of the configured +command. diff --git a/docs/SELF_UPDATE.md b/docs/SELF_UPDATE.md index 4ea67094a9..72213338e0 100644 --- a/docs/SELF_UPDATE.md +++ b/docs/SELF_UPDATE.md @@ -2,7 +2,7 @@ How PortOS notices a new release and updates itself. PortOS is distributed software — many people run it, and a large share run it from a **personal fork**, so every step here is fork-aware. Breaking that assumption produces silent no-op updates. -Code: `server/services/updateChecker.js`, `server/routes/update.js`, `server/lib/gitRemote.js`, `update.sh` / `update.ps1`, `client/src/components/apps/tabs/UpdateTab.jsx`. +Code: `server/services/updateChecker.js`, `server/services/updateExecutor.js`, `server/services/appUpdater.js`, `server/routes/update.js`, `server/lib/gitRemote.js`, `server/lib/detachedSpawn.js`, `update.sh` / `update.ps1`, `scripts/verify-server-health.js`, `client/src/components/apps/tabs/UpdateTab.jsx`. ## Release polling always targets upstream @@ -63,6 +63,24 @@ To prevent that confusion, `POST /api/update/execute` rejects fork runs with **4 - the request body sets `acknowledgeFork: true`, or - `lastForkSync.fullName` matches `remoteInfo.fullName` (compared case-insensitively — GitHub owner/repo names are) and is less than 10 minutes old. The service computes this once as `status.forkSyncFresh` from `FORK_SYNC_FRESHNESS_MS`; the route and the UI both read that flag rather than re-implementing the time math. +## Every PortOS update goes through the detached launcher + +`update.sh` deletes and restarts every PortOS PM2 entry. PM2's TreeKill walks **PPID**, so a script left attached to `portos-server` is killed by its own `pm2 delete` step — mid-list, before it can run the closing `pm2 start` — and the install is left headless. `spawnDetached`'s double-fork (`server/lib/detachedSpawn.js`) is what reparents the script to init so it survives; `executeUpdate()` in `server/services/updateExecutor.js` is the single launcher that applies it, along with the `STEP:` progress parsing, the still-running-script guard, and `recordUpdateResult()`. + +**PortOS is also a managed app**, so an update started from **App Management** reaches `update.sh` through `appUpdater.js` rather than `routes/update.js`. That path delegates to `executeUpdate()` for the PortOS record instead of spawning the script itself — a second detached-spawn implementation would be one more thing to keep in sync, and the attached one it replaced produced exactly the headless failure above (#5976). `appUpdater` also **skips its own `restart` step** for that case: the script runs `pm2 start ecosystem.config.cjs` itself, so restarting on top of it would be redundant and would race the script. + +Both entry points take the same atomic `setUpdateInProgress(true)` lock before launching, so they cannot run `update.sh` concurrently — and because that flag is what `subAgentSpawner`, `agentLifecycle` and `persistentMindSupervisor` gate on, holding it also stops a CoS agent from being spawned into a process the script is about to `pm2 delete` (#4124). + +A PortOS record carrying a custom `updateCommand`, or a `repoPath` that is not this checkout, keeps the ordinary attached path — delegating there would silently run `update.sh` instead of the configured command. That decision is logged rather than silent, since the attached path is the one that failed. The `repoPath` comparison resolves symlinks and case-folds on macOS/Windows: `repoPath` is user-editable and not force-synced, so a trailing slash or a different spelling must not be mistaken for a different checkout. Non-PortOS managed apps are unaffected and still get their own PM2 restart from `appUpdater`. The dashboard handoff it starts before that restart is PortOS-only — it opens the PortOS dashboard, so it would be meaningless after another app's update. + +## Post-update health verification + +`pm2 start` exiting 0 is not proof the server came back, and the process that would notice is the one that did not. Both platform scripts therefore close with a `verify` step that polls `/api/system/health` (`scripts/verify-server-health.js`) until it reports `ok` or the budget — `PORTOS_HEALTH_WAIT_MS`, default 120s — runs out. On failure they spend one more `pm2 start ecosystem.config.cjs` and then log the outcome loudly, with the manual recovery command. + +The probe tries the loopback HTTP mirror (`:5553`) first, then the API port over HTTP and HTTPS, because the listening scheme depends on whether a cert is provisioned; `/api/system/health` is in the always-public set, so it works with the optional instance password on. The recovery only fires when the probe fails, so it cannot make a healthy update worse. + +**When the probe still fails after the recovery, the scripts say so and exit non-zero** — the closing banner reads "Update applied, but PortOS is DOWN" instead of "Update Complete". The script outlives the server it restarts, so its exit status and the tail of `data/update.log` are the only signals a wrapper, a CI job, or an operator still has; printing a success banner over a confirmed-headless install is how the failure went unnoticed for hours in the first place. + ## Syncing a fork `POST /api/update/sync-fork` shells out to: diff --git a/scripts/verify-server-health.js b/scripts/verify-server-health.js new file mode 100644 index 0000000000..f9c2b79c84 --- /dev/null +++ b/scripts/verify-server-health.js @@ -0,0 +1,167 @@ +#!/usr/bin/env node +/** + * "Did portos-server actually come back after the update restarted it?" + * + * `update.sh` / `update.ps1` delete every PortOS PM2 entry and start it again. + * When that bracket half-fails, the install is left headless — and nothing else + * on the machine notices, because the thing that would have noticed is the + * server that did not come back (#5976: a no-op update left the install down + * for hours). The update script is the last PortOS process still running at + * that point, so the check has to live here. + * + * Usage as a CLI (what update.sh and update.ps1 call): + * node scripts/verify-server-health.js + * + * Exit 0 → the server answered /api/system/health with status "ok". + * Exit 1 → it did not, within the budget. The caller re-runs `pm2 start`. + * + * Fails CLOSED, unlike `pm2-daemon-refresh.js`: an unreachable server is + * exactly the condition being detected, so anything short of a positive "ok" + * is reported as unhealthy. The recovery it triggers is one extra `pm2 start`, + * which cannot make an already-healthy install worse. + * + * `/api/system/health` is in the always-public set (`PUBLIC_API_PATHS`), so + * this works with the optional instance password on. All three candidate URLs + * are probed because the listening scheme/port depends on whether a cert is + * provisioned: HTTPS on :5555 plus the loopback HTTP mirror on :5553, or plain + * HTTP on :5555. Probing beats re-deriving the cert state — the answer we want + * is "is something serving", not "which URL should we advertise". + */ + +import http from 'node:http'; +import https from 'node:https'; +import { PORTS } from '../server/lib/ports.js'; +import { isDirectlyInvoked } from './lib/directInvocation.js'; + +const HEALTH_PATH = '/api/system/health'; +const DEFAULT_TIMEOUT_MS = 120_000; +const DEFAULT_INTERVAL_MS = 2_000; +const PROBE_TIMEOUT_MS = 5_000; + +/** + * Read a non-negative millisecond budget from the environment. `|| DEFAULT` + * would be wrong here: it collapses "unset" and "not a number" together with a + * deliberate `0` (fail fast, one pass and out), which `waitForHealthy` + * explicitly supports. + * + * @param {string|undefined} raw + * @param {number} fallback + * @returns {number} + */ +export function parseTimeoutMs(raw, fallback = DEFAULT_TIMEOUT_MS) { + if (raw === undefined || raw === null || String(raw).trim() === '') return fallback; + const parsed = Number(raw); + return Number.isFinite(parsed) && parsed >= 0 ? parsed : fallback; +} + +/** + * The loopback URLs a healthy PortOS could be answering on, in the order worth + * trying: the plain-HTTP mirror first (always cert-free), then the API port + * over each scheme. Deduped so a plain-HTTP install (mirror port unbound, + * API port serving HTTP) does not probe the same URL twice. + * + * @param {{apiPort: number, mirrorPort: number}} ports + * @returns {string[]} + */ +export function healthProbeUrls({ apiPort, mirrorPort }) { + const urls = [ + `http://127.0.0.1:${mirrorPort}${HEALTH_PATH}`, + `http://127.0.0.1:${apiPort}${HEALTH_PATH}`, + `https://127.0.0.1:${apiPort}${HEALTH_PATH}`, + ]; + return [...new Set(urls)]; +} + +/** + * One request. Resolves true only on a 200 whose JSON body says status "ok" — + * a 502 from something else on the port, a hung socket, or a half-booted + * server that answers but not with "ok" all count as not-yet-healthy. + * + * `rejectUnauthorized: false` matches the rest of PortOS's loopback probing: + * the cert is issued for the Tailscale hostname, so 127.0.0.1 never validates, + * and there is no trust boundary to cross on loopback. + * + * @param {string} url + * @param {number} timeoutMs + * @returns {Promise} + */ +export function probeHealth(url, timeoutMs = PROBE_TIMEOUT_MS) { + return new Promise((resolve) => { + const transport = url.startsWith('https:') ? https : http; + const req = transport.get(url, { timeout: timeoutMs, rejectUnauthorized: false }, (res) => { + if (res.statusCode !== 200) { + res.resume(); + resolve(false); + return; + } + let body = ''; + res.setEncoding('utf8'); + res.on('data', (chunk) => { body += chunk; }); + res.on('end', () => { + // A response that is not the health payload — a proxy error page, a + // truncated body — is not a healthy server. + try { + resolve(JSON.parse(body)?.status === 'ok'); + } catch { + resolve(false); + } + }); + res.on('error', () => resolve(false)); + }); + req.on('timeout', () => { req.destroy(); resolve(false); }); + req.on('error', () => resolve(false)); + }); +} + +/** + * Poll the candidate URLs until one reports healthy or the budget runs out. + * Clock and probe are injected so the timeout contract is testable without + * real sleeps or a real server. + * + * @param {object} options + * @param {string[]} options.urls + * @param {number} [options.timeoutMs] - total budget across all attempts + * @param {number} [options.intervalMs] - pause between full passes + * @param {(url: string) => Promise} [options.probe] + * @param {() => number} [options.now] + * @param {(ms: number) => Promise} [options.sleep] + * @returns {Promise<{healthy: boolean, url: string|null, attempts: number}>} + */ +export async function waitForHealthy({ + urls, + timeoutMs = DEFAULT_TIMEOUT_MS, + intervalMs = DEFAULT_INTERVAL_MS, + probe = probeHealth, + now = Date.now, + sleep = (ms) => new Promise((r) => setTimeout(r, ms)), +}) { + const deadline = now() + timeoutMs; + let attempts = 0; + // Always make one full pass, even with a zero/expired budget — the check is + // worthless if it can report "unhealthy" without having asked. + for (;;) { + for (const url of urls) { + attempts += 1; + if (await probe(url)) return { healthy: true, url, attempts }; + } + if (now() >= deadline) return { healthy: false, url: null, attempts }; + await sleep(intervalMs); + } +} + +async function runCli() { + const apiPort = Number(process.env.PORT) || PORTS.API; + const mirrorPort = Number(process.env.PORTOS_HTTP_PORT) || PORTS.API_LOCAL; + const timeoutMs = parseTimeoutMs(process.env.PORTOS_HEALTH_WAIT_MS); + const urls = healthProbeUrls({ apiPort, mirrorPort }); + + const result = await waitForHealthy({ urls, timeoutMs }); + if (result.healthy) { + console.log(`✅ PortOS is serving ${HEALTH_PATH} (${result.url})`); + return 0; + } + console.error(`❌ PortOS did not answer ${HEALTH_PATH} within ${Math.round(timeoutMs / 1000)}s (${result.attempts} attempts)`); + return 1; +} + +if (isDirectlyInvoked(import.meta.url)) process.exit(await runCli()); diff --git a/scripts/verify-server-health.test.js b/scripts/verify-server-health.test.js new file mode 100644 index 0000000000..d9699f61a9 --- /dev/null +++ b/scripts/verify-server-health.test.js @@ -0,0 +1,132 @@ +import { describe, expect, it, vi } from 'vitest'; +import { createServer } from 'node:http'; +import { healthProbeUrls, parseTimeoutMs, probeHealth, waitForHealthy } from './verify-server-health.js'; + +/** Start a loopback server that answers one canned response, and return its URL. */ +async function withServer(handler, run) { + const server = createServer(handler); + await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)); + try { + return await run(`http://127.0.0.1:${server.address().port}/api/system/health`); + } finally { + await new Promise((resolve) => server.close(resolve)); + } +} + +describe('post-update server health verification', () => { + it('probes the loopback mirror before the API port, and dedupes a plain-HTTP install', () => { + expect(healthProbeUrls({ apiPort: 5555, mirrorPort: 5553 })).toEqual([ + 'http://127.0.0.1:5553/api/system/health', + 'http://127.0.0.1:5555/api/system/health', + 'https://127.0.0.1:5555/api/system/health', + ]); + // No cert provisioned: the mirror never binds and the API port serves HTTP, + // so the two http candidates collapse into one. + expect(healthProbeUrls({ apiPort: 5555, mirrorPort: 5555 })).toEqual([ + 'http://127.0.0.1:5555/api/system/health', + 'https://127.0.0.1:5555/api/system/health', + ]); + }); + + it('accepts only a 200 that actually reports status "ok"', async () => { + const ok = await withServer((_req, res) => { + res.writeHead(200, { 'content-type': 'application/json' }); + res.end(JSON.stringify({ status: 'ok', version: '0.0.0-test' })); + }, (url) => probeHealth(url)); + expect(ok).toBe(true); + + // A half-booted server, or something else squatting the port, answers — + // treating that as healthy would skip the recovery the caller exists for. + const degraded = await withServer((_req, res) => { + res.writeHead(200, { 'content-type': 'application/json' }); + res.end(JSON.stringify({ status: 'degraded' })); + }, (url) => probeHealth(url)); + expect(degraded).toBe(false); + + const notJson = await withServer((_req, res) => { + res.writeHead(200, { 'content-type': 'text/html' }); + res.end('proxy error'); + }, (url) => probeHealth(url)); + expect(notJson).toBe(false); + + const serverError = await withServer((_req, res) => { + res.writeHead(503); + res.end(''); + }, (url) => probeHealth(url)); + expect(serverError).toBe(false); + }); + + it('reports unhealthy for a port nothing is listening on', async () => { + // Bind then release so the port is known-free rather than guessed. + const port = await withServer(() => {}, (url) => Number(new URL(url).port)); + expect(await probeHealth(`http://127.0.0.1:${port}/api/system/health`, 1_000)).toBe(false); + }); + + it('keeps polling a booting server until it answers, without spending the whole budget', async () => { + let clock = 0; + const probe = vi.fn() + .mockResolvedValueOnce(false) + .mockResolvedValueOnce(false) + .mockResolvedValueOnce(true); + + const result = await waitForHealthy({ + urls: ['http://127.0.0.1:5553/api/system/health'], + timeoutMs: 120_000, + intervalMs: 2_000, + probe, + now: () => clock, + sleep: async (ms) => { clock += ms; }, + }); + + expect(result).toEqual({ healthy: true, url: 'http://127.0.0.1:5553/api/system/health', attempts: 3 }); + expect(clock).toBe(4_000); + }); + + it('gives up once the budget is spent, after asking at least once', async () => { + let clock = 0; + const probe = vi.fn().mockResolvedValue(false); + + const result = await waitForHealthy({ + urls: ['http://a/health', 'http://b/health'], + timeoutMs: 5_000, + intervalMs: 2_000, + probe, + now: () => clock, + sleep: async (ms) => { clock += ms; }, + }); + + expect(result.healthy).toBe(false); + expect(result.url).toBe(null); + // The contract is "kept asking for the whole budget, then stopped", not a + // particular pass schedule: every candidate is asked on every pass, at + // least one full pass happened, and it only gave up past the deadline. + expect(probe.mock.calls.length % 2).toBe(0); + expect(probe.mock.calls.length).toBeGreaterThanOrEqual(2); + expect(clock).toBeGreaterThanOrEqual(5_000); + }); + + it('reads a deliberate zero budget from the environment instead of falling back', () => { + // `|| DEFAULT` would turn a fail-fast 0 — and a typo — into a silent 120s. + expect(parseTimeoutMs('0')).toBe(0); + expect(parseTimeoutMs('30000')).toBe(30_000); + expect(parseTimeoutMs(undefined)).toBe(120_000); + expect(parseTimeoutMs('')).toBe(120_000); + expect(parseTimeoutMs('12O')).toBe(120_000); + expect(parseTimeoutMs('-1')).toBe(120_000); + }); + + it('still makes one full pass when the budget is already exhausted', async () => { + const probe = vi.fn().mockResolvedValue(false); + + const result = await waitForHealthy({ + urls: ['http://a/health'], + timeoutMs: 0, + probe, + now: () => 0, + sleep: async () => {}, + }); + + expect(result.healthy).toBe(false); + expect(probe).toHaveBeenCalledTimes(1); + }); +}); diff --git a/server/services/appUpdater.js b/server/services/appUpdater.js index c93063092f..6be5fd9ff0 100644 --- a/server/services/appUpdater.js +++ b/server/services/appUpdater.js @@ -1,5 +1,5 @@ -import { existsSync } from 'fs'; -import { join } from 'path'; +import { existsSync, realpathSync } from 'fs'; +import { join, resolve } from 'path'; import { readFile } from 'fs/promises'; import { tmpdir } from 'os'; import * as gitService from './git.js'; @@ -7,7 +7,10 @@ import * as pm2Service from './pm2.js'; import { bufferedSpawnOrThrow } from '../lib/bufferedSpawn.js'; import { parseCommandArgs, validateCommand } from '../lib/commandSecurity.js'; import { isDetachedRunning, spawnDetached } from '../lib/detachedSpawn.js'; +import { PATHS } from '../lib/fileUtils.js'; import { PORTOS_APP_ID } from '../lib/appIdentity.js'; +import { executeUpdate } from './updateExecutor.js'; +import { setUpdateInProgress } from './updateChecker.js'; import { syncManagedAppFork } from './managedAppRepositories.js'; const CMD_TIMEOUT_MS = 5 * 60 * 1000; @@ -32,6 +35,10 @@ const DASHBOARD_OPEN_CONTROL_DIR = join(tmpdir(), 'portos-dashboard-open'); * double-fork helper: PM2's tree-kill would otherwise take the helper down * with portos-server before it can wait for the browser to return. * + * Only the paths that restart PortOS from HERE need it. The delegated + * self-update does not: update.sh runs `open-ui-in-browser.js` itself once the + * ecosystem is back up. + * * @param {object} app * @returns {Promise} */ @@ -64,17 +71,50 @@ async function startDashboardHandoff(app) { }); } +/** + * Whether two filesystem paths name the same directory. A trailing slash, a + * symlinked checkout, or a different case on APFS/NTFS all spell one path more + * than one way — and the caller below turns "these differ" into "take the + * ATTACHED spawn", which is exactly the headless failure of #5976. Resolve + * symlinks where possible, and case-fold on the platforms whose filesystems + * are case-insensitive by default (mirrors `scripts/lib/directInvocation.js`). + * + * @param {string} a + * @param {string} b + * @returns {boolean} + */ +function isSamePath(a, b) { + if (!a || !b) return false; + const caseFold = process.platform === 'win32' || process.platform === 'darwin'; + const normalize = (path) => { + // realpath throws when the path does not exist yet; resolve() alone still + // collapses a trailing slash and any '..' segment. + const absolute = (() => { + try { + return realpathSync(resolve(path)); + } catch { + return resolve(path); + } + })(); + return caseFold ? absolute.toLowerCase() : absolute; + }; + return normalize(a) === normalize(b); +} + /** * Run a full update cycle for an app: * 1. switch to origin's default branch and fast-forward it * 2. run an explicitly declared app update routine, when one exists * 3. restart the app's PM2 processes * - * PortOS owns its comprehensive update.sh/update.ps1 lifecycle separately. * A generic managed app must opt in to dependency installs, migrations, or a * build: guessing those steps from a package.json can freeze or break apps * whose lifecycle does not resemble PortOS. * + * PortOS itself is a managed app, and its comprehensive update.sh/update.ps1 + * lifecycle is delegated to `updateExecutor` — which also owns the restart and + * the dashboard handoff for that case. See the app-update step in `_doUpdate`. + * * @param {object} app - The app object (must have repoPath, pm2ProcessNames, pm2Home) * @param {function} emit - Callback (step, status, message) for progress updates * @param {{syncFork?: boolean}} options @@ -137,7 +177,16 @@ async function _doUpdate(app, emit, { syncFork }) { const configuredUpdate = typeof app.updateCommand === 'string' ? app.updateCommand.trim() : ''; const standardScript = process.platform === 'win32' ? 'update.ps1' : 'update.sh'; const standardScriptPath = join(dir, standardScript); - if (configuredUpdate || pkg?.scripts?.['portos:update'] || existsSync(standardScriptPath)) { + const usesStandardScript = !configuredUpdate && !pkg?.scripts?.['portos:update'] && existsSync(standardScriptPath); + // PortOS running THIS checkout's own standard update script is the one case + // whose update routine deletes the process awaiting it — and the only shape + // updateExecutor knows how to launch, since it resolves update.sh from + // `PATHS.root` rather than from the app record. Both narrowings matter: a + // PortOS record carrying a custom `updateCommand`, or pointing somewhere + // other than this checkout, keeps the ordinary attached path rather than + // silently running a different script than the one configured. + const detachSelfUpdate = app.id === PORTOS_APP_ID && usesStandardScript && isSamePath(dir, PATHS.root); + if (configuredUpdate || pkg?.scripts?.['portos:update'] || usesStandardScript) { // A configured runtime may be an absolute Bun path, which is trusted app // configuration but not a commandSecurity allowlist token. Only free-form // registry commands go through that parser; the package-script form is a @@ -150,13 +199,63 @@ async function _doUpdate(app, emit, { syncFork }) { ? { valid: true, baseCommand: 'powershell', args: ['-ExecutionPolicy', 'Bypass', '-File', standardScriptPath] } : { valid: true, baseCommand: standardScriptPath, args: [] }; if (!command.valid) throw new Error(`Update command is not allowed: ${command.error}`); + if (app.id === PORTOS_APP_ID && !detachSelfUpdate) { + // Never silent: this is PortOS about to run its update routine ATTACHED, + // and an attached run is what left the install headless in #5976. Name + // which of the three narrowings declined it, so an operator debugging the + // misconfiguration is not sent after the wrong one. + const reason = configuredUpdate + ? 'a custom update command is configured' + : pkg?.scripts?.['portos:update'] + ? 'a portos:update package script is configured' + : 'repoPath is not this checkout'; + console.log(`⚠️ PortOS update is using the attached path — ${reason}`); + } emit('app-update', 'running', 'Running the app update routine...'); - await runCommand(command.baseCommand, command.args, dir); + if (detachSelfUpdate) { + // PortOS is itself a managed app, so an App Management update reaches + // update.sh through THIS path — and the script's own + // `pm2 delete ecosystem.config.cjs` step tree-kills portos-server. + // PM2 walks PPID, so an attached spawn dies with the server it just + // deleted, taking the in-flight `pm2 delete` with it and never reaching + // the closing `pm2 start`: the install is left headless, with only the + // entries declared after portos-cos still online (#5976). + // + // updateExecutor already owns the double-fork launch that survives that, + // plus the STEP: progress parsing that maps straight onto this emit + // contract, the still-running-script guard and recordUpdateResult — so + // delegate rather than keeping a second detached-spawn implementation + // in sync here. The version is only a logging/fallback label; the true + // post-update version comes from the script's completion marker. + // Acquiring the update flag is what holds CoS agent spawns off a process + // update.sh is about to `pm2 delete` (#4124) — `subAgentSpawner`, + // `agentLifecycle` and `persistentMindSupervisor` all gate on it. It is + // also the atomic lock `POST /api/update/execute` takes, so the two entry + // points into update.sh cannot launch it concurrently. + const acquired = await setUpdateInProgress(true); + if (!acquired) throw new Error('A PortOS update is already in progress'); + const version = typeof pkg?.version === 'string' ? pkg.version : 'unknown'; + // Every outcome executeUpdate REPORTS clears the flag again through + // recordUpdateResult; a rejection from the launcher itself reports none. + const outcome = await executeUpdate(version, emit).catch(async (err) => { + await setUpdateInProgress(false); + throw err; + }); + if (!outcome.success) { + throw new Error(outcome.errorMessage || `PortOS update failed at step "${outcome.failedStep || 'unknown'}"`); + } + } else { + await runCommand(command.baseCommand, command.args, dir); + } emit('app-update', 'done', 'App update routine complete'); steps.push({ step: 'app-update', success: true }); } - const processNames = app.pm2ProcessNames || []; + // update.sh/update.ps1 close with their own `pm2 start ecosystem.config.cjs` + // (and their own dashboard handoff), so restarting PortOS on top of the + // detached script would be redundant and would race it — the script may not + // have finished re-registering the processes we would be restarting. + const processNames = detachSelfUpdate ? [] : (app.pm2ProcessNames || []); if (processNames.length > 0) { emit('restart', 'running', 'Restarting app...'); await startDashboardHandoff(app); diff --git a/server/services/appUpdater.test.js b/server/services/appUpdater.test.js index 22d51bc73f..2e212293a5 100644 --- a/server/services/appUpdater.test.js +++ b/server/services/appUpdater.test.js @@ -4,8 +4,14 @@ import { join } from 'path'; import { tmpdir } from 'os'; const mock = vi.hoisted(() => ({ + // updateExecutor resolves update.sh from PATHS.root, so the delegation is + // gated on the app record pointing at this checkout — point it at the + // per-test temp repo instead. + paths: { root: '' }, updateDefaultBranch: vi.fn(), spawn: vi.fn(), + executeUpdate: vi.fn(), + setUpdateInProgress: vi.fn(), dashboardOpen: vi.fn(), dashboardRunning: vi.fn(), dashboardHandle: { on: vi.fn() }, @@ -13,12 +19,18 @@ const mock = vi.hoisted(() => ({ syncFork: vi.fn(), })); +vi.mock('../lib/fileUtils.js', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, PATHS: mock.paths }; +}); vi.mock('./git.js', () => ({ updateDefaultBranch: mock.updateDefaultBranch })); vi.mock('./pm2.js', () => ({ restartApp: mock.restart })); vi.mock('../lib/bufferedSpawn.js', async (importOriginal) => { const actual = await importOriginal(); return { ...actual, bufferedSpawnOrThrow: mock.spawn }; }); +vi.mock('./updateExecutor.js', () => ({ executeUpdate: mock.executeUpdate })); +vi.mock('./updateChecker.js', () => ({ setUpdateInProgress: mock.setUpdateInProgress })); vi.mock('../lib/detachedSpawn.js', () => ({ isDetachedRunning: mock.dashboardRunning, spawnDetached: mock.dashboardOpen, @@ -33,11 +45,14 @@ describe('managed app updates', () => { beforeEach(async () => { vi.clearAllMocks(); repo = await mkdtemp(join(tmpdir(), 'portos-app-updater-')); + mock.paths.root = repo; await mkdir(join(repo, 'client')); await writeFile(join(repo, 'package.json'), JSON.stringify({ scripts: { setup: 'example-setup' } })); await writeFile(join(repo, 'client', 'package.json'), JSON.stringify({})); mock.updateDefaultBranch.mockResolvedValue({ branch: 'main', output: 'Already up to date' }); mock.spawn.mockResolvedValue({ stdout: '', stderr: '' }); + mock.executeUpdate.mockResolvedValue({ success: true, version: '9.9.9' }); + mock.setUpdateInProgress.mockResolvedValue(true); mock.dashboardRunning.mockResolvedValue(false); mock.dashboardOpen.mockResolvedValue(mock.dashboardHandle); mock.restart.mockResolvedValue({ success: true }); @@ -94,63 +109,179 @@ describe('managed app updates', () => { ); }); - it('starts the trusted dashboard handoff before restarting PortOS', async () => { + it('launches PortOS\'s own update through the detached executor, never the attached spawn', async () => { + // PortOS is a managed app, so App Management updates route through here — + // and update.sh's `pm2 delete` tree-kills the server that would be this + // spawn's PPID parent, taking the script down mid-delete (#5976). The + // detached launcher in updateExecutor is what survives it. + await writeFile(join(repo, 'update.sh'), '#!/bin/sh\nexit 0\n'); + await writeFile(join(repo, 'update.ps1'), 'exit 0\n'); + await writeFile(join(repo, 'package.json'), JSON.stringify({ version: '2.56.0' })); const emit = vi.fn(); - const managed = { + + const result = await updateApp({ id: 'portos-default', name: 'PortOS', type: 'express', repoPath: repo, - pm2ProcessNames: ['portos-server', 'portos-browser'], - }; - - await updateApp(managed, emit); + pm2ProcessNames: ['portos-server', 'portos-cos', 'portos-browser'], + }, emit); - expect(mock.dashboardOpen).toHaveBeenCalledWith( - process.execPath, - [join(repo, 'scripts/open-ui-in-browser.js')], - expect.objectContaining({ - cwd: repo, - cleanup: true, - controlDir: expect.stringContaining('portos-dashboard-open'), - }), - ); - expect(mock.dashboardRunning).toHaveBeenCalledWith( - expect.stringContaining('portos-dashboard-open'), - { - executable: process.execPath, - args: [join(repo, 'scripts/open-ui-in-browser.js')], - }, - ); - expect(mock.dashboardOpen.mock.invocationCallOrder[0]).toBeLessThan(mock.restart.mock.invocationCallOrder[0]); + expect(result.success).toBe(true); + expect(mock.executeUpdate).toHaveBeenCalledWith('2.56.0', emit); + expect(mock.spawn).not.toHaveBeenCalled(); + // The flag CoS spawn gates read (#4124) has to be up before the script that + // deletes portos-cos starts, not after. + expect(mock.setUpdateInProgress).toHaveBeenCalledWith(true); + expect(mock.setUpdateInProgress.mock.invocationCallOrder[0]) + .toBeLessThan(mock.executeUpdate.mock.invocationCallOrder[0]); + expect(emit).toHaveBeenCalledWith('app-update', 'done', 'App update routine complete'); }); - it('does not overwrite an unreadable dashboard handoff control dir', async () => { + it('leaves the PM2 restart to update.sh instead of double-restarting PortOS', async () => { + await writeFile(join(repo, 'update.sh'), '#!/bin/sh\nexit 0\n'); + await writeFile(join(repo, 'update.ps1'), 'exit 0\n'); const emit = vi.fn(); - mock.dashboardRunning.mockRejectedValueOnce(new Error('control dir unavailable')); - const managed = { + + const result = await updateApp({ + id: 'portos-default', + name: 'PortOS', + type: 'express', + repoPath: repo, + pm2ProcessNames: ['portos-server', 'portos-cos'], + }, emit); + + expect(mock.restart).not.toHaveBeenCalled(); + expect(result.steps.some((step) => step.step === 'restart')).toBe(false); + expect(emit).not.toHaveBeenCalledWith('restart', expect.anything(), expect.anything()); + // update.sh runs open-ui-in-browser.js itself once the ecosystem is back. + expect(mock.dashboardOpen).not.toHaveBeenCalled(); + }); + + it('surfaces a failed PortOS update instead of reporting success', async () => { + await writeFile(join(repo, 'update.sh'), '#!/bin/sh\nexit 1\n'); + await writeFile(join(repo, 'update.ps1'), 'exit 1\n'); + mock.executeUpdate.mockResolvedValue({ success: false, failedStep: 'npm-install', errorMessage: 'Update failed at step "npm-install" (exit code 1)' }); + + await expect(updateApp({ id: 'portos-default', name: 'PortOS', type: 'express', repoPath: repo, pm2ProcessNames: ['portos-server'], - }; + }, vi.fn())).rejects.toThrow('Update failed at step "npm-install" (exit code 1)'); + }); - await updateApp(managed, emit); + it('refuses to launch a second update while one already holds the flag', async () => { + // The same atomic lock POST /api/update/execute takes — the two entry points + // into update.sh must not launch it concurrently. + await writeFile(join(repo, 'update.sh'), '#!/bin/sh\nexit 0\n'); + await writeFile(join(repo, 'update.ps1'), 'exit 0\n'); + mock.setUpdateInProgress.mockResolvedValue(false); - expect(mock.dashboardOpen).not.toHaveBeenCalled(); + await expect(updateApp({ + id: 'portos-default', + name: 'PortOS', + type: 'express', + repoPath: repo, + pm2ProcessNames: ['portos-server'], + }, vi.fn())).rejects.toThrow(/already in progress/i); + + expect(mock.executeUpdate).not.toHaveBeenCalled(); + }); + + it('still delegates when the record spells this checkout differently', async () => { + // repoPath is user-editable and not force-synced, so a trailing slash or a + // '..' segment is a realistic spelling — and treating it as "not this + // checkout" would silently re-arm the attached spawn of #5976. + await writeFile(join(repo, 'update.sh'), '#!/bin/sh\nexit 0\n'); + await writeFile(join(repo, 'update.ps1'), 'exit 0\n'); + + await updateApp({ + id: 'portos-default', + name: 'PortOS', + type: 'express', + repoPath: `${repo}/client/..`, + pm2ProcessNames: ['portos-server'], + }, vi.fn()); + + expect(mock.executeUpdate).toHaveBeenCalled(); + expect(mock.spawn).not.toHaveBeenCalled(); + }); + + it('does not delegate when the PortOS record points outside this checkout', async () => { + // executeUpdate resolves update.sh from PATHS.root, not from the record — + // delegating a record aimed elsewhere would run a different script than the + // one the update was configured to run. + await writeFile(join(repo, 'update.sh'), '#!/bin/sh\nexit 0\n'); + await writeFile(join(repo, 'update.ps1'), 'exit 0\n'); + mock.paths.root = join(repo, 'somewhere-else'); + + await updateApp({ + id: 'portos-default', + name: 'PortOS', + type: 'express', + repoPath: repo, + pm2ProcessNames: ['portos-server'], + }, vi.fn()); + + expect(mock.executeUpdate).not.toHaveBeenCalled(); + expect(mock.spawn).toHaveBeenCalled(); expect(mock.restart).toHaveBeenCalledWith('portos-server', undefined); }); - it('runs an explicit update command before restarting', async () => { - const emit = vi.fn(); - const managed = { + it('keeps a non-PortOS app on the attached spawn and its own PM2 restart', async () => { + // The detached launcher is PortOS-only — it hard-codes this checkout's + // update script, which is not another app's update routine. + await writeFile(join(repo, 'update.sh'), '#!/bin/sh\nexit 0\n'); + await writeFile(join(repo, 'update.ps1'), 'exit 0\n'); + + await updateApp({ + id: 'example-managed-app', + name: 'Example App', + type: 'express', + repoPath: repo, + pm2ProcessNames: ['example-app'], + }, vi.fn()); + + expect(mock.executeUpdate).not.toHaveBeenCalled(); + expect(mock.spawn).toHaveBeenCalled(); + expect(mock.restart).toHaveBeenCalledWith('example-app', undefined); + }); + + it('honors a custom update command configured on the PortOS record', async () => { + // Delegating here would silently run update.sh instead of what the user + // configured, so the explicit command keeps the ordinary attached path. + await writeFile(join(repo, 'update.sh'), '#!/bin/sh\nexit 0\n'); + await writeFile(join(repo, 'update.ps1'), 'exit 0\n'); + + await updateApp({ id: 'portos-default', name: 'PortOS', type: 'express', repoPath: repo, updateCommand: 'npm run update', pm2ProcessNames: ['portos-server'], + }, vi.fn()); + + expect(mock.executeUpdate).not.toHaveBeenCalled(); + expect(mock.spawn).toHaveBeenCalledWith('npm', ['run', 'update'], expect.objectContaining({ cwd: repo })); + expect(mock.restart).toHaveBeenCalledWith('portos-server', undefined); + // This path still restarts PortOS itself, so it still owns the dashboard + // handoff — only the delegated one hands that to update.sh. + expect(mock.dashboardOpen).toHaveBeenCalled(); + expect(mock.dashboardOpen.mock.invocationCallOrder[0]).toBeLessThan(mock.restart.mock.invocationCallOrder[0]); + }); + + it('runs an explicit update command before restarting', async () => { + const emit = vi.fn(); + const managed = { + id: 'example-managed-app', + name: 'Example App', + type: 'express', + repoPath: repo, + updateCommand: 'npm run update', + pm2ProcessNames: ['example-app'], }; const result = await updateApp(managed, emit); diff --git a/update.ps1 b/update.ps1 index a296b56ba3..8168aa4a6b 100644 --- a/update.ps1 +++ b/update.ps1 @@ -395,6 +395,34 @@ $global:LASTEXITCODE = 0 Step "restart" "done" "PortOS started" Write-SafeHost "" +# Defense in depth (#5976): `pm2 start` exiting 0 is not proof the server came +# back — a half-failed delete/start bracket leaves the install headless, and +# this script is the only PortOS process still running to notice. Poll +# /api/system/health, and on failure spend one more `pm2 start` before saying +# so loudly. Mirrors update.sh. +$verifyFailed = 0 +Step "verify" "running" "Verifying PortOS came back..." +Invoke-Logged node scripts/verify-server-health.js +if ($LASTEXITCODE -eq 0) { + Step "verify" "done" "PortOS is answering /api/system/health" +} else { + Write-SafeHost "PortOS did not answer /api/system/health after the restart - re-running pm2 start" -ForegroundColor Yellow + Invoke-Logged node ./node_modules/pm2/bin/pm2 start ecosystem.config.cjs + $global:LASTEXITCODE = 0 + Invoke-Logged node scripts/verify-server-health.js + if ($LASTEXITCODE -eq 0) { + Step "verify" "done" "PortOS recovered after a second pm2 start" + Write-SafeHost "PortOS recovered after a second pm2 start" -ForegroundColor Green + } else { + $verifyFailed = 1 + Step "verify" "warning" "PortOS is not answering /api/system/health" + Write-SafeHost "PortOS is STILL not answering /api/system/health." -ForegroundColor Red + Write-SafeHost " Recover with: node ./node_modules/pm2/bin/pm2 start ecosystem.config.cjs" -ForegroundColor Red + } +} +$global:LASTEXITCODE = 0 +Write-SafeHost "" + # Open the dashboard in the PortOS-managed browser. Fail-soft — explicitly # reset $LASTEXITCODE to 0 after the call so a non-zero exit from the auto- # open script doesn't propagate as the script's own exit code (the update @@ -402,9 +430,18 @@ Write-SafeHost "" Invoke-Logged node scripts/open-ui-in-browser.js $global:LASTEXITCODE = 0 -Write-SafeHost "===================================" -ForegroundColor Green -Write-SafeHost " ✅ Update Complete!" -ForegroundColor Green -Write-SafeHost "===================================" -ForegroundColor Green +if ($verifyFailed -eq 0) { + Write-SafeHost "===================================" -ForegroundColor Green + Write-SafeHost " ✅ Update Complete!" -ForegroundColor Green + Write-SafeHost "===================================" -ForegroundColor Green +} else { + # The source update finished, but the install is down. Say so where the + # banner would have been — a wrapper reading only the tail of the log, or + # this script's exit status, must not read a headless install as a clean run. + Write-SafeHost "===================================" -ForegroundColor Red + Write-SafeHost " ⚠️ Update applied, but PortOS is DOWN" -ForegroundColor Red + Write-SafeHost "===================================" -ForegroundColor Red +} Write-SafeHost "" # Tell the user where to open PortOS — leads with the working local URL @@ -438,3 +475,7 @@ if ($stashedForBranch) { } Write-SafeHost " The stash entry is at the top of 'git stash list'." -ForegroundColor Cyan } + +# Exit non-zero when the install did not come back. This script outlives the +# server it restarts, so its status is the only signal a caller still has. +exit $verifyFailed diff --git a/update.sh b/update.sh index f4873d8d15..9b14b08043 100755 --- a/update.sh +++ b/update.sh @@ -358,13 +358,47 @@ run node ./node_modules/pm2/bin/pm2 save || true step "restart" "done" "PortOS started" log "" +# Defense in depth (#5976): `pm2 start` exiting 0 is not proof the server came +# back — a half-failed delete/start bracket leaves the install headless, and +# this script is the only PortOS process still running to notice. Poll +# /api/system/health, and on failure spend one more `pm2 start` before saying +# so loudly. A recovery that only fires when the probe fails cannot make a +# healthy update worse. +verify_failed=0 +step "verify" "running" "Verifying PortOS came back..." +if run node scripts/verify-server-health.js; then + step "verify" "done" "PortOS is answering /api/system/health" +else + log "⚠️ PortOS did not answer /api/system/health after the restart — re-running pm2 start" + run node ./node_modules/pm2/bin/pm2 start ecosystem.config.cjs || true + if run node scripts/verify-server-health.js; then + step "verify" "done" "PortOS recovered after a second pm2 start" + log "✅ PortOS recovered after a second pm2 start" + else + verify_failed=1 + step "verify" "warning" "PortOS is not answering /api/system/health" + log "❌ PortOS is STILL not answering /api/system/health." + log " Recover with: node ./node_modules/pm2/bin/pm2 start ecosystem.config.cjs" + fi +fi +log "" + # Open the dashboard in the PortOS-managed browser. Fail-soft — never blocks # the update return. run node scripts/open-ui-in-browser.js || true -log "===================================" -log " ✅ Update Complete!" -log "===================================" +if [ "$verify_failed" -eq 0 ]; then + log "===================================" + log " ✅ Update Complete!" + log "===================================" +else + # The source update finished, but the install is down. Say so where the + # banner would have been — a wrapper reading only the tail of the log, or + # this script's exit status, must not read a headless install as a clean run. + log "===================================" + log " ⚠️ Update applied, but PortOS is DOWN" + log "===================================" +fi log "" # Tell the user where to open PortOS — leads with the working local URL @@ -393,3 +427,7 @@ if [ -n "$stashed_for_branch" ]; then fi log " The stash entry is at the top of 'git stash list'." fi + +# Exit non-zero when the install did not come back. This script outlives the +# server it restarts, so its status is the only signal a caller still has. +exit "$verify_failed"