From e4d199acfd07449001471bd68feb1523e0cf2f22 Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Thu, 3 Sep 2026 04:05:38 +0000 Subject: [PATCH 1/3] fix: run PortOS's App Management update through the detached launcher (#5976) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Updating PortOS from App Management stopped portos-server and never brought it back, even when the pull was a no-op. PortOS is itself a managed app, so that button reaches update.sh through appUpdater's plain spawn rather than routes/update.js. update.sh's own `pm2 delete ecosystem.config.cjs` then tree-kills portos-server — and PM2 walks PPID, so it takes the still-attached script down with it, mid-delete and long before the closing `pm2 start`. The fingerprint was a partial deletion: only portos-server and portos-cos, the two entries declared before the script died, went missing. updateExecutor already solved this for the self-update route with spawnDetached's double-fork, so the PortOS record now delegates to executeUpdate() instead of keeping a second detached-spawn implementation in sync. appUpdater also skips its trailing restart step for that case, since update.sh starts the ecosystem itself. The delegation is narrowed to the PortOS record running this checkout's standard script: a custom updateCommand, or a repoPath pointing elsewhere, keeps the attached path so we never silently run a different script than the one configured. Non-PortOS managed apps are untouched. Defense in depth: both platform scripts now close with a `verify` step that polls /api/system/health until it reports ok, and on timeout spends one more `pm2 start` before logging the manual recovery command. Nothing previously noticed "I stopped the server and it did not come back" — the process that would have noticed is the one that did not come back. Removes appUpdater's dashboard handoff, which the delegation makes unreachable; update.sh runs open-ui-in-browser.js itself, with its own health-wait and navigate retry. Closes #5976 --- client/src/components/apps/tabs/UpdateTab.jsx | 4 + docs/MANAGED_APP_UPDATES.md | 15 ++ docs/SELF_UPDATE.md | 16 +- scripts/verify-server-health.js | 151 +++++++++++++++++ scripts/verify-server-health.test.js | 120 +++++++++++++ server/services/appUpdater.js | 91 +++++----- server/services/appUpdater.test.js | 157 +++++++++++++----- update.ps1 | 26 +++ update.sh | 23 +++ 9 files changed, 510 insertions(+), 93 deletions(-) create mode 100644 scripts/verify-server-health.js create mode 100644 scripts/verify-server-health.test.js 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..dd480f93d2 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,20 @@ 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. + +A PortOS record carrying a custom `updateCommand` keeps the ordinary attached path — delegating there would silently run `update.sh` instead of the configured command. Non-PortOS managed apps are unaffected. + +## 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. + ## 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..d93d51284e --- /dev/null +++ b/scripts/verify-server-health.js @@ -0,0 +1,151 @@ +#!/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; + +/** + * 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 = Number(process.env.PORTOS_HEALTH_WAIT_MS) || DEFAULT_TIMEOUT_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..bc8354136d --- /dev/null +++ b/scripts/verify-server-health.test.js @@ -0,0 +1,120 @@ +import { describe, expect, it, vi } from 'vitest'; +import { createServer } from 'node:http'; +import { healthProbeUrls, 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 deadline is only checked after a full pass, so passes run at t=0, + // 2000 and 4000, plus the one at t=6000 that finds the budget spent — and + // every candidate URL is asked on every pass. + expect(probe).toHaveBeenCalledTimes(8); + }); + + 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..82a010fad8 100644 --- a/server/services/appUpdater.js +++ b/server/services/appUpdater.js @@ -1,13 +1,13 @@ import { existsSync } from 'fs'; import { join } from 'path'; import { readFile } from 'fs/promises'; -import { tmpdir } from 'os'; import * as gitService from './git.js'; 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 { syncManagedAppFork } from './managedAppRepositories.js'; const CMD_TIMEOUT_MS = 5 * 60 * 1000; @@ -23,46 +23,6 @@ function runCommand(cmd, args, cwd) { // Per-app lock to prevent concurrent updates const updatingApps = new Set(); -const DASHBOARD_OPEN_SCRIPT = 'scripts/open-ui-in-browser.js'; -const DASHBOARD_OPEN_CONTROL_DIR = join(tmpdir(), 'portos-dashboard-open'); - -/** - * Start the post-update dashboard handoff before any PortOS process is - * restarted. The handoff is deliberately detached through the shared - * 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. - * - * @param {object} app - * @returns {Promise} - */ -async function startDashboardHandoff(app) { - if (app.id !== PORTOS_APP_ID) return; - - const scriptPath = join(app.repoPath, DASHBOARD_OPEN_SCRIPT); - const alreadyRunning = await isDetachedRunning(DASHBOARD_OPEN_CONTROL_DIR, { - executable: process.execPath, - args: [scriptPath], - }).catch((err) => { - // Do not let an unreadable control dir be mistaken for an idle one: the - // detached helper clears stale sentinels before launching and could then - // race a handoff that is still alive after the previous PM2 restart. - console.error(`⚠️ Dashboard auto-open status check failed: ${err.message}`); - return true; - }); - if (alreadyRunning) return; - - const handoff = await spawnDetached( - process.execPath, - [scriptPath], - { cwd: app.repoPath, controlDir: DASHBOARD_OPEN_CONTROL_DIR, cleanup: true }, - ).catch((err) => { - console.error(`⚠️ Dashboard auto-open could not start: ${err.message}`); - return null; - }); - handoff?.on('error', (err) => { - console.error(`⚠️ Dashboard auto-open failed: ${err.message}`); - }); -} /** * Run a full update cycle for an app: @@ -70,11 +30,14 @@ async function startDashboardHandoff(app) { * 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 +100,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 && 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 @@ -151,15 +123,40 @@ async function _doUpdate(app, emit, { syncFork }) { : { valid: true, baseCommand: standardScriptPath, args: [] }; if (!command.valid) throw new Error(`Update command is not allowed: ${command.error}`); 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. + const version = typeof pkg?.version === 'string' ? pkg.version : 'unknown'; + const outcome = await executeUpdate(version, emit); + 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); const restartResults = await Promise.all( processNames.map(name => pm2Service.restartApp(name, app.pm2Home).then(() => null, e => e) diff --git a/server/services/appUpdater.test.js b/server/services/appUpdater.test.js index 22d51bc73f..6e2d618bc3 100644 --- a/server/services/appUpdater.test.js +++ b/server/services/appUpdater.test.js @@ -4,25 +4,28 @@ 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(), - dashboardOpen: vi.fn(), - dashboardRunning: vi.fn(), - dashboardHandle: { on: vi.fn() }, + executeUpdate: vi.fn(), restart: vi.fn(), 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('../lib/detachedSpawn.js', () => ({ - isDetachedRunning: mock.dashboardRunning, - spawnDetached: mock.dashboardOpen, -})); +vi.mock('./updateExecutor.js', () => ({ executeUpdate: mock.executeUpdate })); vi.mock('./managedAppRepositories.js', () => ({ syncManagedAppFork: mock.syncFork })); import { updateApp } from './appUpdater.js'; @@ -33,13 +36,13 @@ 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.dashboardRunning.mockResolvedValue(false); - mock.dashboardOpen.mockResolvedValue(mock.dashboardHandle); + mock.executeUpdate.mockResolvedValue({ success: true, version: '9.9.9' }); mock.restart.mockResolvedValue({ success: true }); mock.syncFork.mockResolvedValue({ alreadyUpToDate: false, @@ -49,10 +52,6 @@ describe('managed app updates', () => { }); afterEach(async () => { - await Promise.all(mock.dashboardOpen.mock.calls - .map(([, , options]) => options?.controlDir) - .filter(Boolean) - .map((controlDir) => rm(controlDir, { recursive: true, force: true }))); await rm(repo, { recursive: true, force: true }); }); @@ -94,63 +93,131 @@ 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(); + 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()); + }); + + 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('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'); - expect(mock.dashboardOpen).not.toHaveBeenCalled(); + 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); + }); + + 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..e452994855 100644 --- a/update.ps1 +++ b/update.ps1 @@ -395,6 +395,32 @@ $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. +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 { + 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 diff --git a/update.sh b/update.sh index f4873d8d15..8ee2461545 100755 --- a/update.sh +++ b/update.sh @@ -358,6 +358,29 @@ 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. +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 + 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 From 15f030cb24b65cd01539aa907eb3f36d9e8d808f Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Thu, 3 Sep 2026 04:17:01 +0000 Subject: [PATCH 2/3] fix: hold the update lock and report a failed health verify honestly (#5976) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on the App Management self-update path. The delegated launch now takes `setUpdateInProgress(true)` before starting update.sh. That flag is the same atomic lock POST /api/update/execute takes, so the two entry points can no longer launch the script concurrently — and it is what subAgentSpawner, agentLifecycle and persistentMindSupervisor gate on, so holding it also stops a CoS agent from being spawned into a process the script is about to `pm2 delete` (#4124). The route's remaining acknowledgement-bearing preflight guards need schema and UI work and are tracked in #5984. The `repoPath == PATHS.root` test that decides whether to delegate now resolves symlinks and case-folds on macOS/Windows. repoPath is user-editable and not force-synced, so a trailing slash or a different spelling of the same checkout would otherwise silently fall back to the attached spawn — re-arming the exact failure this fixes. The fallback is logged rather than silent. update.sh / update.ps1 no longer print "Update Complete" and exit 0 after the health verify failed twice. They print "Update applied, but PortOS is DOWN" and exit non-zero: the script outlives the server it restarts, so its exit status and the tail of data/update.log are the only signals a wrapper or an operator still has. Restores appUpdater's dashboard handoff. It is redundant for the delegated case, which update.sh handles, but a PortOS record with a custom updateCommand still restarts from here and would otherwise never reopen the dashboard. Also stops PORTOS_HEALTH_WAIT_MS=0 (and a typo'd value) collapsing into the 120s default, and drops a health-poll test assertion that pinned the loop's internal pass schedule rather than its timeout contract. --- docs/SELF_UPDATE.md | 6 +- scripts/verify-server-health.js | 18 ++++- scripts/verify-server-health.test.js | 22 ++++-- server/services/appUpdater.js | 103 +++++++++++++++++++++++++-- server/services/appUpdater.test.js | 64 +++++++++++++++++ update.ps1 | 21 +++++- update.sh | 21 +++++- 7 files changed, 238 insertions(+), 17 deletions(-) diff --git a/docs/SELF_UPDATE.md b/docs/SELF_UPDATE.md index dd480f93d2..88556a6e21 100644 --- a/docs/SELF_UPDATE.md +++ b/docs/SELF_UPDATE.md @@ -69,7 +69,9 @@ To prevent that confusion, `POST /api/update/execute` rejects fork runs with **4 **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. -A PortOS record carrying a custom `updateCommand` keeps the ordinary attached path — delegating there would silently run `update.sh` instead of the configured command. Non-PortOS managed apps are unaffected. +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 and dashboard handoff from `appUpdater`. ## Post-update health verification @@ -77,6 +79,8 @@ A PortOS record carrying a custom `updateCommand` keeps the ordinary attached pa 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 index d93d51284e..f9c2b79c84 100644 --- a/scripts/verify-server-health.js +++ b/scripts/verify-server-health.js @@ -38,6 +38,22 @@ 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 @@ -136,7 +152,7 @@ export async function waitForHealthy({ async function runCli() { const apiPort = Number(process.env.PORT) || PORTS.API; const mirrorPort = Number(process.env.PORTOS_HTTP_PORT) || PORTS.API_LOCAL; - const timeoutMs = Number(process.env.PORTOS_HEALTH_WAIT_MS) || DEFAULT_TIMEOUT_MS; + const timeoutMs = parseTimeoutMs(process.env.PORTOS_HEALTH_WAIT_MS); const urls = healthProbeUrls({ apiPort, mirrorPort }); const result = await waitForHealthy({ urls, timeoutMs }); diff --git a/scripts/verify-server-health.test.js b/scripts/verify-server-health.test.js index bc8354136d..d9699f61a9 100644 --- a/scripts/verify-server-health.test.js +++ b/scripts/verify-server-health.test.js @@ -1,6 +1,6 @@ import { describe, expect, it, vi } from 'vitest'; import { createServer } from 'node:http'; -import { healthProbeUrls, probeHealth, waitForHealthy } from './verify-server-health.js'; +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) { @@ -97,10 +97,22 @@ describe('post-update server health verification', () => { expect(result.healthy).toBe(false); expect(result.url).toBe(null); - // The deadline is only checked after a full pass, so passes run at t=0, - // 2000 and 4000, plus the one at t=6000 that finds the budget spent — and - // every candidate URL is asked on every pass. - expect(probe).toHaveBeenCalledTimes(8); + // 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 () => { diff --git a/server/services/appUpdater.js b/server/services/appUpdater.js index 82a010fad8..812e4c063a 100644 --- a/server/services/appUpdater.js +++ b/server/services/appUpdater.js @@ -1,13 +1,16 @@ -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'; 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; @@ -23,6 +26,80 @@ function runCommand(cmd, args, cwd) { // Per-app lock to prevent concurrent updates const updatingApps = new Set(); +const DASHBOARD_OPEN_SCRIPT = 'scripts/open-ui-in-browser.js'; +const DASHBOARD_OPEN_CONTROL_DIR = join(tmpdir(), 'portos-dashboard-open'); + +/** + * Start the post-update dashboard handoff before any PortOS process is + * restarted. The handoff is deliberately detached through the shared + * 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} + */ +async function startDashboardHandoff(app) { + if (app.id !== PORTOS_APP_ID) return; + + const scriptPath = join(app.repoPath, DASHBOARD_OPEN_SCRIPT); + const alreadyRunning = await isDetachedRunning(DASHBOARD_OPEN_CONTROL_DIR, { + executable: process.execPath, + args: [scriptPath], + }).catch((err) => { + // Do not let an unreadable control dir be mistaken for an idle one: the + // detached helper clears stale sentinels before launching and could then + // race a handoff that is still alive after the previous PM2 restart. + console.error(`⚠️ Dashboard auto-open status check failed: ${err.message}`); + return true; + }); + if (alreadyRunning) return; + + const handoff = await spawnDetached( + process.execPath, + [scriptPath], + { cwd: app.repoPath, controlDir: DASHBOARD_OPEN_CONTROL_DIR, cleanup: true }, + ).catch((err) => { + console.error(`⚠️ Dashboard auto-open could not start: ${err.message}`); + return null; + }); + handoff?.on('error', (err) => { + console.error(`⚠️ Dashboard auto-open failed: ${err.message}`); + }); +} + +/** + * 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: @@ -108,7 +185,12 @@ async function _doUpdate(app, emit, { syncFork }) { // 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 && dir === PATHS.root; + const detachSelfUpdate = app.id === PORTOS_APP_ID && usesStandardScript && isSamePath(dir, PATHS.root); + if (app.id === PORTOS_APP_ID && !detachSelfUpdate) { + // Never silent: this is the branch that runs update.sh attached, and an + // attached run is what left the install headless in #5976. + console.log(`⚠️ PortOS update is using the attached path — ${usesStandardScript ? 'repoPath is not this checkout' : 'a custom update command is configured'}`); + } 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 @@ -138,8 +220,20 @@ async function _doUpdate(app, emit, { syncFork }) { // 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'; - const outcome = await executeUpdate(version, emit); + // 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'}"`); } @@ -157,6 +251,7 @@ async function _doUpdate(app, emit, { syncFork }) { const processNames = detachSelfUpdate ? [] : (app.pm2ProcessNames || []); if (processNames.length > 0) { emit('restart', 'running', 'Restarting app...'); + await startDashboardHandoff(app); const restartResults = await Promise.all( processNames.map(name => pm2Service.restartApp(name, app.pm2Home).then(() => null, e => e) diff --git a/server/services/appUpdater.test.js b/server/services/appUpdater.test.js index 6e2d618bc3..2e212293a5 100644 --- a/server/services/appUpdater.test.js +++ b/server/services/appUpdater.test.js @@ -11,6 +11,10 @@ const mock = vi.hoisted(() => ({ updateDefaultBranch: vi.fn(), spawn: vi.fn(), executeUpdate: vi.fn(), + setUpdateInProgress: vi.fn(), + dashboardOpen: vi.fn(), + dashboardRunning: vi.fn(), + dashboardHandle: { on: vi.fn() }, restart: vi.fn(), syncFork: vi.fn(), })); @@ -26,6 +30,11 @@ vi.mock('../lib/bufferedSpawn.js', async (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, +})); vi.mock('./managedAppRepositories.js', () => ({ syncManagedAppFork: mock.syncFork })); import { updateApp } from './appUpdater.js'; @@ -43,6 +52,9 @@ describe('managed app updates', () => { 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 }); mock.syncFork.mockResolvedValue({ alreadyUpToDate: false, @@ -52,6 +64,10 @@ describe('managed app updates', () => { }); afterEach(async () => { + await Promise.all(mock.dashboardOpen.mock.calls + .map(([, , options]) => options?.controlDir) + .filter(Boolean) + .map((controlDir) => rm(controlDir, { recursive: true, force: true }))); await rm(repo, { recursive: true, force: true }); }); @@ -114,6 +130,11 @@ describe('managed app updates', () => { 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'); }); @@ -133,6 +154,8 @@ describe('managed app updates', () => { 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 () => { @@ -149,6 +172,43 @@ describe('managed app updates', () => { }, vi.fn())).rejects.toThrow('Update failed at step "npm-install" (exit code 1)'); }); + 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); + + 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 @@ -207,6 +267,10 @@ describe('managed app updates', () => { 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 () => { diff --git a/update.ps1 b/update.ps1 index e452994855..8168aa4a6b 100644 --- a/update.ps1 +++ b/update.ps1 @@ -400,6 +400,7 @@ Write-SafeHost "" # 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) { @@ -413,6 +414,7 @@ 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 @@ -428,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 @@ -464,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 8ee2461545..9b14b08043 100755 --- a/update.sh +++ b/update.sh @@ -364,6 +364,7 @@ log "" # /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" @@ -374,6 +375,7 @@ else 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" @@ -385,9 +387,18 @@ log "" # 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 @@ -416,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" From 495d885ab9dc775fd0b38ec587099c405d0fe01d Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Thu, 3 Sep 2026 04:22:22 +0000 Subject: [PATCH 3/3] fix: name the real reason when a PortOS update declines the detached path (#5976) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The decline log fired even when no update routine ran at all, and reported 'a custom update command is configured' for a record that had none — sending an operator after the wrong misconfiguration. It now fires only where the routine actually runs, and distinguishes all three narrowings. Also corrects a docs sentence that credited non-PortOS managed apps with a dashboard handoff; that handoff opens the PortOS dashboard and returns early for every other app. --- docs/SELF_UPDATE.md | 2 +- server/services/appUpdater.js | 17 ++++++++++++----- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/docs/SELF_UPDATE.md b/docs/SELF_UPDATE.md index 88556a6e21..72213338e0 100644 --- a/docs/SELF_UPDATE.md +++ b/docs/SELF_UPDATE.md @@ -71,7 +71,7 @@ To prevent that confusion, `POST /api/update/execute` rejects fork runs with **4 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 and dashboard handoff from `appUpdater`. +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 diff --git a/server/services/appUpdater.js b/server/services/appUpdater.js index 812e4c063a..6be5fd9ff0 100644 --- a/server/services/appUpdater.js +++ b/server/services/appUpdater.js @@ -186,11 +186,6 @@ async function _doUpdate(app, emit, { syncFork }) { // 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 (app.id === PORTOS_APP_ID && !detachSelfUpdate) { - // Never silent: this is the branch that runs update.sh attached, and an - // attached run is what left the install headless in #5976. - console.log(`⚠️ PortOS update is using the attached path — ${usesStandardScript ? 'repoPath is not this checkout' : 'a custom update command is configured'}`); - } 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 @@ -204,6 +199,18 @@ 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...'); if (detachSelfUpdate) { // PortOS is itself a managed app, so an App Management update reaches