From b586bad18a50d063b35e72af420119b436f8ee4b Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Wed, 9 Sep 2026 05:15:16 +0000 Subject: [PATCH 1/2] refactor: make self-update launch readiness explicit (#6694) --- docs/SELF_UPDATE.md | 10 ++- server/routes/update.test.js | 80 ++++++++++--------- server/services/portosSelfUpdate.js | 70 +++++----------- server/services/portosSelfUpdate.test.js | 60 ++++++++------ server/services/updateExecutor.js | 42 +++++----- server/services/updateExecutor.test.js | 42 +++++++++- server/services/updatePreflightParity.test.js | 10 +-- 7 files changed, 172 insertions(+), 142 deletions(-) diff --git a/docs/SELF_UPDATE.md b/docs/SELF_UPDATE.md index dd325437ba..3237540f3a 100644 --- a/docs/SELF_UPDATE.md +++ b/docs/SELF_UPDATE.md @@ -67,7 +67,7 @@ To prevent that confusion, `POST /api/update/execute` rejects fork runs with **4 ## 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()`. +`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; `launchUpdate()` 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()`. **Windows needs the same escape, and cannot get it from `detached: true`.** PM2 kills there with `taskkill /pid /T /F`, which walks the identical parent→child tree, so an attached `update.ps1` dies at `pm2-stop` exactly as an attached `update.sh` would. But Node's `detached: true` maps to **`DETACHED_PROCESS`** on Windows, which denies the child a console — and a console host like `powershell.exe` given no console exits **0 within ~100ms without running a single line**. That combination is why the in-app update silently did nothing on Windows while reporting a successful update to the target release (#6169): the script never ran, `executeUpdate` saw exit 0, and stamped the triggering tag onto a "Success" result. So on Windows `spawnDetached` launches a short-lived PowerShell launcher that `Process.Start`s a **supervisor** (with `CreateNoWindow`, per [WINDOWS_CONSOLE.md](WINDOWS_CONSOLE.md)) and exits. The supervisor's parent is then gone, so `taskkill /T` from the server never reaches it; it redirects the script's output into the control dir and records its pid and exit status there, so the same tailer streams `STEP:` progress on both platforms. This is the *only* win32 path — it shipped in #6169 behind a `windowsDetached` opt-in, because the plain-spawn fallback beside it handed back a real `ChildProcess` whose `kill` tree-killed the job's own descendants and the supervisor path could then only signal the job's pid; #6170 routed the supervisor handle's `kill` (and the boot-time orphan reaper) through the same `taskkill /T /F`, which removed the trade-off and with it the option. @@ -76,7 +76,7 @@ Two guards keep a launch that did nothing from being reported as an update: - A run that exits 0 having emitted **no `STEP:` line at all** is recorded as a failure, not a success — both scripts emit `git-pull:running` before touching anything, so silence means the script never ran. - When `data/update-complete.json` is missing (usually because the restarted server already consumed it), the recorded version comes from **package.json on disk**, never from the triggering tag: the tag is only what the update aimed at. -**PortOS is also a managed app**, so an update started from **App Management**'s Git tab reaches `update.sh` through `appUpdater.js` rather than `routes/update.js`. Both entry points call the same `startPortosSelfUpdate()` (`server/services/portosSelfUpdate.js`), which owns the whole lifecycle: the preflight refusals, the atomic `setUpdateInProgress(true)` lock, the post-lock re-check, and the `executeUpdate()` launch. They differ only in `mode`: +**PortOS is also a managed app**, so an update started from **App Management**'s Git tab reaches `update.sh` through `appUpdater.js` rather than `routes/update.js`. Both entry points call the same `startPortosSelfUpdate()` (`server/services/portosSelfUpdate.js`), which owns the whole lifecycle: the preflight refusals, the atomic `setUpdateInProgress(true)` lock, the post-lock re-check, and the `launchUpdate()` launch. They differ only in `mode`: | Surface | Mode | Gate | |---|---|---| @@ -92,7 +92,11 @@ Because the lock is taken in one place, the two entry points cannot run `update. `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. -### Nothing awaits the script, on either side +### Launch and completion are separate phases + +`launchUpdate(tag, emit, options)` resolves to `{ started: false, result }` when a previous script is still running, or `{ started: true, completion }` after the detached spawn succeeds and progress listeners are attached. Spawn failures reject. `completion` tracks the script lifetime and its persisted result; it is observed for socket reporting without delaying the launch response. A refusal retains the 409 `UPDATE_LAUNCH_FAILED` response, and a rejected launch releases the update lock. + +`executeUpdate()` remains a compatibility adapter: it returns the refusal result or awaits `completion`, preserving the existing result shape and calling `onLaunched` once after a successful spawn. New handoff callers use `launchUpdate()` directly so they need no callback flag or promise race. `startPortosSelfUpdate()` resolves as soon as the detached script is RUNNING. It cannot report the outcome, because `update.sh` `pm2 delete`s this server partway through and the process awaiting it dies there — an awaited launch simply never runs its own completion code. The Git tab used to await it, which is why it hung: `app:update:complete` never fired, the operation was never cleared, and the row sat on "Stopping PortOS apps..." forever while the update finished fine in the background. So `appUpdater` returns `{ selfUpdateStarted: true }` at the launch, and `server/sockets/apps.js` deliberately **leaves the operation registered** and emits no completion — the map dies with the process, and the remaining `STEP:` frames keep rendering right up to the moment the server goes down. diff --git a/server/routes/update.test.js b/server/routes/update.test.js index 5cf3ffe5e5..3444f45ee0 100644 --- a/server/routes/update.test.js +++ b/server/routes/update.test.js @@ -3,7 +3,7 @@ import express from 'express'; import { request } from '../lib/testHelper.js'; import { errorMiddleware, errorEvents } from '../lib/errorHandler.js'; -// Mock the services the execute route depends on. executeUpdate is fire-and- +// Mock the services the execute route depends on. Completion is fire-and- // forget in the route (not awaited), so a resolved stub is enough. vi.mock('../services/updateChecker.js', () => ({ getUpdateStatus: vi.fn(), @@ -16,7 +16,7 @@ vi.mock('../services/updateChecker.js', () => ({ setUpdateInProgress: vi.fn().mockResolvedValue(true) })); vi.mock('../services/updateExecutor.js', () => ({ - executeUpdate: vi.fn().mockResolvedValue({ success: true, version: '1.26.0' }) + launchUpdate: vi.fn().mockResolvedValue({ started: true, completion: Promise.resolve({ success: true, version: '1.26.0' }) }) })); // getActiveAgentIds reads live-process maps and spawningTasks holds in-flight // spawns; mock both so tests control the "are CoS agents running?" signal @@ -47,7 +47,7 @@ vi.mock('../services/cosState.js', () => ({ })); import * as updateChecker from '../services/updateChecker.js'; -import { executeUpdate } from '../services/updateExecutor.js'; +import { launchUpdate } from '../services/updateExecutor.js'; import { getActiveAgentIds } from '../services/agentState.js'; import { readPersistentMindStateForSafetyCheck } from '../services/cosState.js'; import { filterLiveAgentIds } from '../services/cosAgentLifecycle.js'; @@ -89,7 +89,7 @@ describe('POST /api/update/execute — reconcile gating (issue #1779)', () => { beforeEach(() => { vi.clearAllMocks(); updateChecker.setUpdateInProgress.mockResolvedValue(true); - executeUpdate.mockResolvedValue({ success: true, version: '1.26.0' }); + launchUpdate.mockResolvedValue({ started: true, completion: Promise.resolve({ success: true, version: '1.26.0' }) }); getActiveAgentIds.mockReturnValue([]); vi.mocked(filterLiveAgentIds).mockImplementation(async (ids) => ids); mockCosState.persistentMind = { queuedMessages: [], activeTurn: null }; @@ -104,7 +104,7 @@ describe('POST /api/update/execute — reconcile gating (issue #1779)', () => { const res = await request(makeApp()).post('/api/update/execute').send({ reconcile: true }); expect(res.status).toBe(400); expect(res.body.code).toBe('ALREADY_IN_SYNC'); - expect(executeUpdate).not.toHaveBeenCalled(); + expect(launchUpdate).not.toHaveBeenCalled(); }); it('rejects reconcile when install state could not be determined (null)', async () => { @@ -112,7 +112,7 @@ describe('POST /api/update/execute — reconcile gating (issue #1779)', () => { const res = await request(makeApp()).post('/api/update/execute').send({ reconcile: true }); expect(res.status).toBe(503); expect(res.body.code).toBe('INSTALL_STATE_UNAVAILABLE'); - expect(executeUpdate).not.toHaveBeenCalled(); + expect(launchUpdate).not.toHaveBeenCalled(); }); it('runs the reconcile when out of sync, targeting the current version and forcing clean of stale workspaces', async () => { @@ -130,7 +130,7 @@ describe('POST /api/update/execute — reconcile gating (issue #1779)', () => { expect(res.status).toBe(200); expect(res.body).toEqual({ started: true, tag: 'v1.26.0' }); // Only the stale workspaces, with 'root' mapped to update.sh's '.' token. - expect(executeUpdate).toHaveBeenCalledWith('v1.26.0', expect.any(Function), expect.objectContaining({ forceCleanWorkspaces: ['.', 'server'] })); + expect(launchUpdate).toHaveBeenCalledWith('v1.26.0', expect.any(Function), expect.objectContaining({ forceCleanWorkspaces: ['.', 'server'] })); }); it('reconcile with no stale deps (build/migration staleness) forces no clean', async () => { @@ -139,7 +139,7 @@ describe('POST /api/update/execute — reconcile gating (issue #1779)', () => { })); const res = await request(makeApp()).post('/api/update/execute').send({ reconcile: true }); expect(res.status).toBe(200); - expect(executeUpdate).toHaveBeenCalledWith('v1.26.0', expect.any(Function), expect.objectContaining({ forceCleanWorkspaces: [] })); + expect(launchUpdate).toHaveBeenCalledWith('v1.26.0', expect.any(Function), expect.objectContaining({ forceCleanWorkspaces: [] })); }); it('reconcile runs even with NO cached release (out of sync)', async () => { @@ -160,7 +160,7 @@ describe('POST /api/update/execute — reconcile gating (issue #1779)', () => { const res = await request(makeApp()).post('/api/update/execute').send({ reconcile: true }); expect(res.status).toBe(412); expect(res.body.code).toBe('FORK_SYNC_REQUIRED'); - expect(executeUpdate).not.toHaveBeenCalled(); + expect(launchUpdate).not.toHaveBeenCalled(); }); it('a non-reconcile update still requires a cached release tag', async () => { @@ -175,7 +175,7 @@ describe('POST /api/update/execute — reconcile gating (issue #1779)', () => { const res = await request(makeApp()).post('/api/update/execute').send({}); expect(res.status).toBe(200); expect(res.body.tag).toBe('v1.27.0'); - expect(executeUpdate).toHaveBeenCalledWith('v1.27.0', expect.any(Function), expect.objectContaining({ forceCleanWorkspaces: undefined })); + expect(launchUpdate).toHaveBeenCalledWith('v1.27.0', expect.any(Function), expect.objectContaining({ forceCleanWorkspaces: undefined })); }); }); @@ -185,7 +185,7 @@ describe('POST /api/update/execute — active CoS agent gating', () => { mockSpawningTasks.clear(); updateChecker.setUpdateInProgress.mockResolvedValue(true); updateChecker.getUpdateStatus.mockResolvedValue(baseStatus()); - executeUpdate.mockResolvedValue({ success: true, version: '1.26.0' }); + launchUpdate.mockResolvedValue({ started: true, completion: Promise.resolve({ success: true, version: '1.26.0' }) }); getActiveAgentIds.mockReturnValue([]); vi.mocked(filterLiveAgentIds).mockImplementation(async (ids) => ids); mockCosState.persistentMind = { queuedMessages: [], activeTurn: null }; @@ -200,7 +200,7 @@ describe('POST /api/update/execute — active CoS agent gating', () => { const res = await request(makeApp()).post('/api/update/execute').send({}); expect(res.status).toBe(409); expect(res.body.code).toBe('AGENTS_ACTIVE'); - expect(executeUpdate).not.toHaveBeenCalled(); + expect(launchUpdate).not.toHaveBeenCalled(); // Guard runs before the in-progress lock is acquired. expect(updateChecker.setUpdateInProgress).not.toHaveBeenCalled(); }); @@ -213,7 +213,7 @@ describe('POST /api/update/execute — active CoS agent gating', () => { const res = await request(makeApp()).post('/api/update/execute').send({}); expect(res.status).toBe(409); expect(res.body.code).toBe('AGENTS_ACTIVE'); - expect(executeUpdate).not.toHaveBeenCalled(); + expect(launchUpdate).not.toHaveBeenCalled(); }); it('re-checks after acquiring the lock and releases it if an agent started during the git/fork awaits', async () => { @@ -224,7 +224,7 @@ describe('POST /api/update/execute — active CoS agent gating', () => { const res = await request(makeApp()).post('/api/update/execute').send({}); expect(res.status).toBe(409); expect(res.body.code).toBe('AGENTS_ACTIVE'); - expect(executeUpdate).not.toHaveBeenCalled(); + expect(launchUpdate).not.toHaveBeenCalled(); // Lock was acquired then released (true, then false), leaving no stuck lock. expect(updateChecker.setUpdateInProgress).toHaveBeenNthCalledWith(1, true); expect(updateChecker.setUpdateInProgress).toHaveBeenCalledWith(false); @@ -238,14 +238,14 @@ describe('POST /api/update/execute — active CoS agent gating', () => { expect(res.body.code).toBe('AGENTS_ACTIVE'); // Pluralized message names both agents. expect(res.body.error).toMatch(/2 CoS agents are running/); - expect(executeUpdate).not.toHaveBeenCalled(); + expect(launchUpdate).not.toHaveBeenCalled(); }); it('proceeds normally when no agents are running', async () => { getActiveAgentIds.mockReturnValue([]); const res = await request(makeApp()).post('/api/update/execute').send({}); expect(res.status).toBe(200); - expect(executeUpdate).toHaveBeenCalled(); + expect(launchUpdate).toHaveBeenCalled(); }); it('rejects before locking when queued image work cannot survive an older reader', async () => { @@ -260,7 +260,7 @@ describe('POST /api/update/execute — active CoS agent gating', () => { expect(res.body.code).toBe('PERSISTENT_MIND_IMAGES_IN_FLIGHT'); expect(res.body.error).toMatch(/Drain the image-bearing work, or create a backup/); expect(updateChecker.setUpdateInProgress).not.toHaveBeenCalled(); - expect(executeUpdate).not.toHaveBeenCalled(); + expect(launchUpdate).not.toHaveBeenCalled(); }); it('re-checks image work after locking and releases the update lock on a race', async () => { @@ -289,7 +289,7 @@ describe('POST /api/update/execute — active CoS agent gating', () => { expect(res.body.code).toBe('PERSISTENT_MIND_IMAGES_IN_FLIGHT'); expect(updateChecker.setUpdateInProgress).toHaveBeenNthCalledWith(1, true); expect(updateChecker.setUpdateInProgress).toHaveBeenCalledWith(false); - expect(executeUpdate).not.toHaveBeenCalled(); + expect(launchUpdate).not.toHaveBeenCalled(); }); it('releases the update lock when the post-lock safety read fails', async () => { @@ -302,7 +302,7 @@ describe('POST /api/update/execute — active CoS agent gating', () => { expect(res.status).toBe(500); expect(updateChecker.setUpdateInProgress).toHaveBeenNthCalledWith(1, true); expect(updateChecker.setUpdateInProgress).toHaveBeenCalledWith(false); - expect(executeUpdate).not.toHaveBeenCalled(); + expect(launchUpdate).not.toHaveBeenCalled(); }); it('fails closed when persisted Persistent Mind state is untrusted', async () => { @@ -313,7 +313,7 @@ describe('POST /api/update/execute — active CoS agent gating', () => { expect(res.status).toBe(409); expect(res.body.code).toBe('PERSISTENT_MIND_STATE_UNTRUSTED'); expect(updateChecker.setUpdateInProgress).not.toHaveBeenCalled(); - expect(executeUpdate).not.toHaveBeenCalled(); + expect(launchUpdate).not.toHaveBeenCalled(); }); it('allows an explicit backup acknowledgement when queued image work cannot drain', async () => { @@ -327,7 +327,7 @@ describe('POST /api/update/execute — active CoS agent gating', () => { }); expect(res.status).toBe(200); - expect(executeUpdate).toHaveBeenCalled(); + expect(launchUpdate).toHaveBeenCalled(); }); // The phantom that pinned the Update page: the CoS Runner kept advertising @@ -340,7 +340,7 @@ describe('POST /api/update/execute — active CoS agent gating', () => { const res = await request(makeApp()).post('/api/update/execute').send({}); expect(res.status).toBe(200); - expect(executeUpdate).toHaveBeenCalled(); + expect(launchUpdate).toHaveBeenCalled(); }); }); @@ -350,7 +350,7 @@ describe('POST /api/update/execute — lock handling and socket progress', () => mockSpawningTasks.clear(); updateChecker.setUpdateInProgress.mockResolvedValue(true); updateChecker.getUpdateStatus.mockResolvedValue(baseStatus()); - executeUpdate.mockResolvedValue({ success: true, version: '1.27.0' }); + launchUpdate.mockResolvedValue({ started: true, completion: Promise.resolve({ success: true, version: '1.27.0' }) }); getActiveAgentIds.mockReturnValue([]); vi.mocked(filterLiveAgentIds).mockImplementation(async (ids) => ids); mockCosState.persistentMind = { queuedMessages: [], activeTurn: null }; @@ -368,7 +368,7 @@ describe('POST /api/update/execute — lock handling and socket progress', () => const res = await request(makeApp()).post('/api/update/execute').send({}); expect(res.status).toBe(409); expect(res.body.code).toBe('UPDATE_IN_PROGRESS'); - expect(executeUpdate).not.toHaveBeenCalled(); + expect(launchUpdate).not.toHaveBeenCalled(); // A lost race must not release the lock the winner holds. expect(updateChecker.setUpdateInProgress).not.toHaveBeenCalledWith(false); }); @@ -383,15 +383,15 @@ describe('POST /api/update/execute — lock handling and socket progress', () => expect(res.status).toBe(400); expect(res.body.code).toBe('INVALID_TAG'); expect(updateChecker.setUpdateInProgress).not.toHaveBeenCalled(); - expect(executeUpdate).not.toHaveBeenCalled(); + expect(launchUpdate).not.toHaveBeenCalled(); }); - // Regression for issue #6036: executeUpdate rejecting (e.g. spawnDetached + // Regression for issue #6036: launchUpdate rejecting (e.g. spawnDetached // throwing before any child listener is attached) skips recordUpdateResult, // so the launcher is the only place left that can release the lock. Leaving // it set wedges every later update at 409 and blocks all CoS agent spawns. - it('releases the update lock and emits an error when executeUpdate rejects', async () => { - executeUpdate.mockRejectedValue(new Error('spawn EACCES')); + it('releases the update lock and emits an error when launchUpdate rejects', async () => { + launchUpdate.mockRejectedValue(new Error('spawn EACCES')); const res = await request(makeApp()).post('/api/update/execute').send({}); // NOT 200: the rejection happens during the LAUNCH, before any script is // running, so the caller is told the update never started rather than being @@ -410,7 +410,7 @@ describe('POST /api/update/execute — lock handling and socket progress', () => // The response is already sent by then, so the socket is the client's only // channel for the outcome and the version it should now expect. it('emits portos:update:complete with the version the script actually landed on', async () => { - executeUpdate.mockResolvedValue({ success: true, version: '1.28.3' }); + launchUpdate.mockResolvedValue({ started: true, completion: Promise.resolve({ success: true, version: '1.28.3' }) }); await request(makeApp()).post('/api/update/execute').send({}); await vi.waitFor(() => { expect(mockIo.emit).toHaveBeenCalledWith('portos:update:complete', { @@ -424,7 +424,7 @@ describe('POST /api/update/execute — lock handling and socket progress', () => // No marker version: fall back to the triggering tag, but flag it as a guess // so the UI doesn't present it as the confirmed installed version. it('falls back to the triggering tag with versionKnown=false when no version is resolved', async () => { - executeUpdate.mockResolvedValue({ success: true }); + launchUpdate.mockResolvedValue({ started: true, completion: Promise.resolve({ success: true }) }); await request(makeApp()).post('/api/update/execute').send({}); await vi.waitFor(() => { expect(mockIo.emit).toHaveBeenCalledWith('portos:update:complete', { @@ -437,11 +437,13 @@ describe('POST /api/update/execute — lock handling and socket progress', () => // A resolved failure is a different code path from a rejection and must still // surface the failing step rather than the generic 'unknown'. - it('emits portos:update:error with the failed step when executeUpdate resolves unsuccessfully', async () => { - executeUpdate.mockResolvedValue({ - success: false, - failedStep: 'install', - errorMessage: 'Update failed at step "install" (exit code 1)', + it('emits portos:update:error with the failed step when launchUpdate resolves unsuccessfully', async () => { + launchUpdate.mockResolvedValue({ + started: true, completion: Promise.resolve({ + success: false, + failedStep: 'install', + errorMessage: 'Update failed at step "install" (exit code 1)', + }), }); await request(makeApp()).post('/api/update/execute').send({}); await vi.waitFor(() => { @@ -452,12 +454,12 @@ describe('POST /api/update/execute — lock handling and socket progress', () => }); }); - // The `emit` callback the route hands executeUpdate is what turns update.sh's + // The `emit` callback the route hands launchUpdate is what turns update.sh's // STEP: lines into the client's progress bar. - it('forwards executeUpdate progress callbacks as portos:update:step events', async () => { - executeUpdate.mockImplementation(async (_tag, emit) => { + it('forwards launchUpdate progress callbacks as portos:update:step events', async () => { + launchUpdate.mockImplementation(async (_tag, emit) => { emit('pull', 'running', 'Pulling latest code...'); - return { success: true, version: '1.27.0' }; + return { started: true, completion: Promise.resolve({ success: true, version: '1.27.0' }) }; }); await request(makeApp()).post('/api/update/execute').send({}); expect(mockIo.emit).toHaveBeenCalledWith('portos:update:step', { diff --git a/server/services/portosSelfUpdate.js b/server/services/portosSelfUpdate.js index b120b3d625..ea05613b39 100644 --- a/server/services/portosSelfUpdate.js +++ b/server/services/portosSelfUpdate.js @@ -27,7 +27,7 @@ import { ServerError } from '../lib/errorHandler.js'; import { withStateLock } from './cosState.js'; -import { executeUpdate } from './updateExecutor.js'; +import { launchUpdate } from './updateExecutor.js'; import * as updateChecker from './updateChecker.js'; import { agentsActiveError, @@ -181,26 +181,28 @@ export async function startPortosSelfUpdate({ const forceCleanWorkspaces = mode === 'release' ? undefined : forceCleanWorkspacesFor(status); - // `executeUpdate` is two phases behind one promise: a LAUNCH (the - // still-running guard, then the double-fork spawn) that can refuse or throw, - // and then the script's whole lifetime. Only the second is fire-and-forget. - // Reporting `started: true` for a script that never spawned is what would - // leave a caller waiting for a restart that is not coming — and on the App - // Management path it also leaves the operation registered forever, since that - // handler deliberately skips its cleanup for a real handoff, so every later - // update is then refused as a duplicate. So hold the return until the spawn. - let launchedFlag = false; - let markLaunched; - const launched = new Promise((resolve) => { - markLaunched = () => { launchedFlag = true; resolve(); }; + const reportFailure = async (err) => { + console.error(`❌ Update launch failed for ${tag}: ${err.message}`); + io?.emit('portos:update:error', { message: err.message, step: 'unknown' }); + // Rejections bypass recordUpdateResult, which normally releases the lock. + await updateChecker.setUpdateInProgress(false).catch(releaseErr => { + console.error(`❌ Failed to release update lock after launch failure: ${releaseErr.message}`); + }); + }; + const launch = await launchUpdate(tag, emit, { forceCleanWorkspaces }).catch(async err => { + await reportFailure(err); + throw err; }); + if (!launch.started) { + const result = launch.result; + io?.emit('portos:update:error', { message: result.errorMessage ?? 'Update failed', step: result.failedStep ?? 'unknown' }); + throw new ServerError(result.errorMessage || 'PortOS update failed to launch', + { status: 409, code: 'UPDATE_LAUNCH_FAILED' }); + } - // The script writes the true post-update version to data/update-complete.json, - // which the server reads on boot, so `tag` is only the label this launch is - // reported under. - const run = executeUpdate(tag, emit, { forceCleanWorkspaces, onLaunched: markLaunched }); - - run.then(result => { + // Observe the lifetime without awaiting it: the script normally kills this + // server at pm2-stop, then the restarted server reads its completion marker. + launch.completion.then(result => { // May never fire: update.sh's PM2 delete usually kills this process first. // The client polls /api/system/health instead of relying on it. if (!io) return; @@ -213,35 +215,7 @@ export async function startPortosSelfUpdate({ } else { io.emit('portos:update:error', { message: result.errorMessage ?? 'Update failed', step: result.failedStep ?? 'unknown' }); } - }).catch(async err => { - console.error(`❌ Update launch failed for ${tag}: ${err.message}`); - io?.emit('portos:update:error', { message: err.message, step: 'unknown' }); - // A rejection means executeUpdate never reached `recordUpdateResult`, which - // is what normally clears the lock on both its resolved outcomes. Without - // this release the lock stays set until the 30-minute stale timeout — - // wedging every later update at 409 UPDATE_IN_PROGRESS and blocking every - // CoS agent spawn in the meantime (issue #6036). - await updateChecker.setUpdateInProgress(false).catch(releaseErr => { - console.error(`❌ Failed to release update lock after launch failure: ${releaseErr.message}`); - }); - }); - - // Settling before the launch signal means the LAUNCH failed: the - // still-running guard refused (a resolved `success: false`) or the spawn threw - // (a rejection). Both are the caller's to report. After the signal this - // resolves regardless — a script that fails later is the fire-and-forget - // handler's business, and re-throwing it here would be an unhandled rejection - // (fatal on Node >= 15) because the race below has already settled. - const launchFailure = run.then( - (result) => { - if (!launchedFlag && !result.success) { - throw new ServerError(result.errorMessage || 'PortOS update failed to launch', - { status: 409, code: 'UPDATE_LAUNCH_FAILED' }); - } - }, - (err) => { if (!launchedFlag) throw err; }, - ); - await Promise.race([launched, launchFailure]); + }).catch(reportFailure); return { started: true, tag }; } diff --git a/server/services/portosSelfUpdate.test.js b/server/services/portosSelfUpdate.test.js index 3e2211a732..ca85fa1072 100644 --- a/server/services/portosSelfUpdate.test.js +++ b/server/services/portosSelfUpdate.test.js @@ -10,7 +10,7 @@ vi.mock('./updateChecker.js', () => ({ setUpdateInProgress: vi.fn().mockResolvedValue(true), })); vi.mock('./updateExecutor.js', () => ({ - executeUpdate: vi.fn().mockResolvedValue({ success: true, version: '1.26.0' }), + launchUpdate: vi.fn().mockResolvedValue({ started: true, completion: Promise.resolve({ success: true, version: '1.26.0' }) }), })); const { mockSpawningTasks } = vi.hoisted(() => ({ mockSpawningTasks: new Set() })); vi.mock('./agentState.js', () => ({ @@ -27,7 +27,7 @@ vi.mock('./cosState.js', () => ({ })); import * as updateChecker from './updateChecker.js'; -import { executeUpdate } from './updateExecutor.js'; +import { launchUpdate } from './updateExecutor.js'; import { startPortosSelfUpdate } from './portosSelfUpdate.js'; // An install with nothing pending and no newer release — the state a reconcile @@ -50,13 +50,7 @@ describe('startPortosSelfUpdate — refresh mode', () => { mockSpawningTasks.clear(); updateChecker.setUpdateInProgress.mockResolvedValue(true); updateChecker.getUpdateStatus.mockResolvedValue(inSyncStatus()); - // Signal the launch the way the real executeUpdate does — the launcher holds - // its return until the spawn, so a mock that never signals is a mock of a - // launch that never happened. - executeUpdate.mockImplementation(async (_tag, _emit, opts) => { - opts?.onLaunched?.(); - return { success: true, version: '1.26.0' }; - }); + launchUpdate.mockResolvedValue({ started: true, completion: Promise.resolve({ success: true, version: '1.26.0' }) }); }); it('runs on an in-sync install, where a reconcile would refuse', async () => { @@ -71,7 +65,7 @@ describe('startPortosSelfUpdate — refresh mode', () => { const result = await startPortosSelfUpdate({ io, mode: 'refresh' }); expect(result).toEqual({ started: true, tag: 'v1.26.0' }); - expect(executeUpdate).toHaveBeenCalledOnce(); + expect(launchUpdate).toHaveBeenCalledOnce(); }); it('force-cleans the workspaces whose deps are stale, since update.sh sees no commit diff', async () => { @@ -95,7 +89,7 @@ describe('startPortosSelfUpdate — refresh mode', () => { await startPortosSelfUpdate({ io, mode: 'refresh' }); - expect(executeUpdate).toHaveBeenCalledWith( + expect(launchUpdate).toHaveBeenCalledWith( 'v1.26.0', expect.any(Function), expect.objectContaining({ forceCleanWorkspaces: ['.', 'client'] }), @@ -107,10 +101,9 @@ describe('startPortosSelfUpdate — refresh mode', () => { // the launcher has to feed both sinks — otherwise the Git tab's progress // row stays empty for the whole update. const onStep = vi.fn(); - executeUpdate.mockImplementation(async (_tag, emit, opts) => { - opts?.onLaunched?.(); + launchUpdate.mockImplementation(async (_tag, emit) => { emit('pm2-stop', 'running', 'Stopping PortOS apps...'); - return { success: true, version: '1.26.0' }; + return { started: true, completion: Promise.resolve({ success: true, version: '1.26.0' }) }; }); await startPortosSelfUpdate({ io, mode: 'refresh', onStep }); @@ -124,22 +117,24 @@ describe('startPortosSelfUpdate — refresh mode', () => { }); it('refuses rather than reporting a start when a prior script is still running', async () => { - // executeUpdate's still-running guard RESOLVES `success: false` without ever + // launchUpdate's still-running guard returns `started: false` without ever // spawning. Returning `started: true` for that leaves App Management's // operation registered forever (its handler skips cleanup on a real // handoff), so every later update is refused as a duplicate while the UI // waits for a restart that is not coming. - executeUpdate.mockResolvedValue({ - success: false, failedStep: 'starting', - errorMessage: 'A previous update script is still running', + launchUpdate.mockResolvedValue({ + started: false, result: { + success: false, failedStep: 'starting', + errorMessage: 'A previous update script is still running', + }, }); await expect(startPortosSelfUpdate({ io, mode: 'refresh' })) - .rejects.toThrow(/still running/i); + .rejects.toMatchObject({ status: 409, code: 'UPDATE_LAUNCH_FAILED' }); }); it('reports a launch that threw, instead of claiming the script started', async () => { - executeUpdate.mockRejectedValue(new Error('spawn EACCES')); + launchUpdate.mockRejectedValue(new Error('spawn EACCES')); await expect(startPortosSelfUpdate({ io, mode: 'refresh' })) .rejects.toThrow('spawn EACCES'); @@ -150,9 +145,8 @@ describe('startPortosSelfUpdate — refresh mode', () => { // return at the spawn. A promise that only settles when the script is done // would never resolve here — the pm2 delete kills the awaiting process. let finish; - executeUpdate.mockImplementation((_tag, _emit, { onLaunched }) => { - onLaunched(); - return new Promise((resolve) => { finish = resolve; }); + launchUpdate.mockResolvedValue({ + started: true, completion: new Promise((resolve) => { finish = resolve; }), }); await expect(startPortosSelfUpdate({ io, mode: 'refresh' })) @@ -167,11 +161,25 @@ describe('startPortosSelfUpdate — refresh mode', () => { )); }); + it('observes a rejected completion after returning the successful handoff', async () => { + let fail; + launchUpdate.mockResolvedValue({ + started: true, completion: new Promise((_resolve, reject) => { fail = reject; }), + }); + await expect(startPortosSelfUpdate({ io, mode: 'refresh' })) + .resolves.toEqual({ started: true, tag: 'v1.26.0' }); + fail(new Error('completion failed')); + await vi.waitFor(() => expect(updateChecker.setUpdateInProgress).toHaveBeenCalledWith(false)); + expect(io.emit).toHaveBeenCalledWith('portos:update:error', { + message: 'completion failed', step: 'unknown', + }); + }); + it('releases the update lock when the launch itself rejects', async () => { - // executeUpdate clears the flag through recordUpdateResult on both of its - // RESOLVED outcomes; a rejection reports none, and the stuck flag then + // The executor clears the flag through recordUpdateResult on refusal and + // completion; a rejection reports none, and the stuck flag then // wedges every later update and every CoS agent spawn (#6036). - executeUpdate.mockRejectedValue(new Error('spawn EACCES')); + launchUpdate.mockRejectedValue(new Error('spawn EACCES')); await startPortosSelfUpdate({ io, mode: 'refresh' }).catch(() => {}); diff --git a/server/services/updateExecutor.js b/server/services/updateExecutor.js index 8cc1191453..2c708ccdb3 100644 --- a/server/services/updateExecutor.js +++ b/server/services/updateExecutor.js @@ -7,8 +7,12 @@ import { getCurrentVersion, recordUpdateResult } from './updateChecker.js'; const UPDATE_SH = join(PATHS.root, 'update.sh'); const UPDATE_PS1 = join(PATHS.root, 'update.ps1'); +// Workspaces update.sh / update.ps1 know how to clean-reinstall — the env +// passthrough is allowlisted to these so nothing arbitrary reaches the scripts. +const CLEANABLE_WORKSPACES = new Set(['.', 'client', 'server', 'autofixer']); + /** - * Execute the PortOS update script (git pull to latest). + * Launch the PortOS update script (git pull to latest). * * The script is launched via spawnDetached so it leaves this process's tree and * SURVIVES pm2's TreeKill. A plain `spawn(..., { detached: true })` does NOT @@ -36,15 +40,11 @@ const UPDATE_PS1 = join(PATHS.root, 'update.ps1'); * @param {function} emit - Callback (step, status, message) for progress * @param {object} [options] * @param {string[]} [options.forceCleanWorkspaces] - workspaces to reinstall from scratch - * @param {function} [options.onLaunched] - called once the script is spawned, before - * the returned promise starts tracking its lifetime - * @returns {Promise<{success: boolean, version?: string, failedStep?: string, errorMessage?: string}>} + * @returns {Promise<{started: false, result: object} | {started: true, completion: Promise}>} + * Refusal returns its recorded result; spawn failures reject. A successful + * launch returns separately from completion, which may outlive this server. */ -// Workspaces update.sh / update.ps1 know how to clean-reinstall — the env -// passthrough is allowlisted to these so nothing arbitrary reaches the scripts. -const CLEANABLE_WORKSPACES = new Set(['.', 'client', 'server', 'autofixer']); - -export async function executeUpdate(tag, emit, { forceCleanWorkspaces, onLaunched } = {}) { +export async function launchUpdate(tag, emit, { forceCleanWorkspaces } = {}) { const targetVersion = tag.replace(/^v/, ''); const isWindows = process.platform === 'win32'; const cmd = isWindows ? 'powershell' : 'bash'; @@ -93,7 +93,7 @@ export async function executeUpdate(tag, emit, { forceCleanWorkspaces, onLaunche log: errorMessage }).catch(e => console.error(`❌ Failed to record update result: ${e.message}`)); emit('starting', 'error', errorMessage); - return { success: false, failedStep: 'starting', errorMessage }; + return { started: false, result: { success: false, failedStep: 'starting', errorMessage } }; } const child = await spawnDetached(cmd, args, { @@ -106,14 +106,7 @@ export async function executeUpdate(tag, emit, { forceCleanWorkspaces, onLaunche pollMs: 1000 }); - // The script is running from here on. Everything ABOVE can still refuse (a - // prior update script is still alive) or throw (spawn error); nothing below - // can — the returned promise then tracks the script's whole lifetime. A - // caller that must tell "the launch failed" from "the update failed" waits on - // this signal rather than on the promise. See `portosSelfUpdate`'s launch gate. - onLaunched?.(); - - return new Promise((resolve) => { + const completion = new Promise((resolve) => { let lastStep = 'starting'; // Whether the script ever reported a step. A run that exits 0 without one // never executed the script — the scripts emit `git-pull:running` before @@ -237,4 +230,17 @@ export async function executeUpdate(tag, emit, { forceCleanWorkspaces, onLaunche // Nothing to unref: spawnDetached's handle is a plain EventEmitter tailing // the control dir, and its launcher already unref'd the process it spawned. }); + return { started: true, completion }; +} + +/** + * Compatibility adapter for callers awaiting the complete update result. + * onLaunched fires once after a successful spawn, before awaiting completion; + * it never fires for a refusal or a rejected spawn. + */ +export async function executeUpdate(tag, emit, { onLaunched, ...options } = {}) { + const launch = await launchUpdate(tag, emit, options); + if (!launch.started) return launch.result; + onLaunched?.(); + return launch.completion; } diff --git a/server/services/updateExecutor.test.js b/server/services/updateExecutor.test.js index 579d12222e..b37a13b1d2 100644 --- a/server/services/updateExecutor.test.js +++ b/server/services/updateExecutor.test.js @@ -25,7 +25,7 @@ vi.mock('./updateChecker.js', () => ({ import { spawnDetached, isDetachedRunning } from '../lib/detachedSpawn.js'; import { readFile } from 'fs/promises'; import { getCurrentVersion, recordUpdateResult } from './updateChecker.js'; -import { executeUpdate } from './updateExecutor.js'; +import { executeUpdate, launchUpdate } from './updateExecutor.js'; // The spawnDetached handle deliberately has NO unref (its launcher already // unref'd), so executeUpdate must never call one. @@ -115,7 +115,9 @@ describe('executeUpdate', () => { const child = createMockChild(); spawnDetached.mockResolvedValue(child); - const { promise } = await startUpdate('v1.0.0', () => {}); + const onLaunched = vi.fn(); + const { promise } = await startUpdate('v1.0.0', () => {}, { onLaunched }); + expect(onLaunched).toHaveBeenCalledExactlyOnceWith(); emitStep(child); child.emit('close', 0); await promise; @@ -159,9 +161,11 @@ describe('executeUpdate', () => { isDetachedRunning.mockResolvedValue(true); const emits = []; - const { promise } = await startUpdate('v1.0.0', (...args) => emits.push(args)); + const onLaunched = vi.fn(); + const { promise } = await startUpdate('v1.0.0', (...args) => emits.push(args), { onLaunched }); const result = await promise; + expect(onLaunched).not.toHaveBeenCalled(); expect(result.success).toBe(false); expect(result.failedStep).toBe('starting'); expect(spawnDetached).not.toHaveBeenCalled(); @@ -359,3 +363,35 @@ describe('executeUpdate', () => { expect(env.PORTOS_FORCE_CLEAN_WORKSPACES).toBeUndefined(); }); }); + +// Pins the public two-phase boundary using process events rather than a mocked +// executor: launch must settle while the child is still alive and observed. +describe('launchUpdate', () => { + it('returns at spawn with progress listeners ready and completion still pending', async () => { + const child = createMockChild(); + spawnDetached.mockResolvedValue(child); + const emit = vi.fn(); + const launch = await launchUpdate('v1.0.0', emit); + expect(launch.started).toBe(true); + const completed = vi.fn(); + launch.completion.then(completed); + await flush(); + expect(completed).not.toHaveBeenCalled(); + emitStep(child); + child.emit('close', 1); + await expect(launch.completion).resolves.toMatchObject({ success: false, failedStep: 'git-pull' }); + expect(emit).toHaveBeenCalledWith('starting', 'done', 'Update script running'); + }); + + it('distinguishes a recorded refusal from a rejected spawn', async () => { + isDetachedRunning.mockResolvedValueOnce(true); + await expect(launchUpdate('v1.0.0', () => {})).resolves.toMatchObject({ + started: false, result: { success: false, failedStep: 'starting' }, + }); + expect(spawnDetached).not.toHaveBeenCalled(); + spawnDetached.mockRejectedValueOnce(new Error('spawn EACCES')); + const onLaunched = vi.fn(); + await expect(executeUpdate('v1.0.0', () => {}, { onLaunched })).rejects.toThrow('spawn EACCES'); + expect(onLaunched).not.toHaveBeenCalled(); + }); +}); diff --git a/server/services/updatePreflightParity.test.js b/server/services/updatePreflightParity.test.js index 3e7a712b48..aa82b37154 100644 --- a/server/services/updatePreflightParity.test.js +++ b/server/services/updatePreflightParity.test.js @@ -16,7 +16,7 @@ vi.mock('../services/updateChecker.js', () => ({ setUpdateInProgress: vi.fn().mockResolvedValue(true), })); vi.mock('../services/updateExecutor.js', () => ({ - executeUpdate: vi.fn().mockResolvedValue({ success: true, version: '1.26.0' }), + launchUpdate: vi.fn().mockResolvedValue({ started: true, completion: Promise.resolve({ success: true, version: '1.26.0' }) }), })); const { mockSpawningTasks } = vi.hoisted(() => ({ mockSpawningTasks: new Set() })); vi.mock('../services/agentState.js', () => ({ @@ -49,7 +49,7 @@ vi.mock('../services/pm2Standardizer.js', () => ({})); vi.mock('../services/streamingDetect.js', () => ({ streamDetection: vi.fn() })); import * as updateChecker from '../services/updateChecker.js'; -import { executeUpdate } from '../services/updateExecutor.js'; +import { launchUpdate } from '../services/updateExecutor.js'; import { getActiveAgentIds } from '../services/agentState.js'; import { readPersistentMindStateForSafetyCheck } from '../services/cosState.js'; import { updateApp as appUpdaterUpdateApp } from '../services/appUpdater.js'; @@ -100,7 +100,7 @@ describe('PortOS update preflight parity — route vs. socket', () => { mockSpawningTasks.clear(); updateChecker.setUpdateInProgress.mockResolvedValue(true); updateChecker.getUpdateStatus.mockResolvedValue(baseStatus()); - executeUpdate.mockResolvedValue({ success: true, version: '1.26.0' }); + launchUpdate.mockResolvedValue({ started: true, completion: Promise.resolve({ success: true, version: '1.26.0' }) }); getActiveAgentIds.mockReturnValue([]); mockCosState.persistentMind = { queuedMessages: [], activeTurn: null }; readPersistentMindStateForSafetyCheck.mockImplementation(async () => ({ @@ -164,7 +164,7 @@ describe('PortOS update preflight parity — route vs. socket', () => { const ackRouteRes = await request(makeRouteApp()).post('/api/update/execute').send({ acknowledgeFork: true }); expect(ackRouteRes.status).toBe(200); - appUpdaterUpdateApp.mockResolvedValue({ success: true, steps: [] }); + appUpdaterUpdateApp.mockResolvedValue({ started: true, completion: Promise.resolve({ success: true, steps: [] }) }); const { fireUpdate: fireAckUpdate } = makeSocketHarness(); await fireAckUpdate({ appId: PORTOS_APP_ID, acknowledgeFork: true }); expect(appUpdaterUpdateApp).toHaveBeenCalledWith(portosApp, expect.any(Function), { @@ -176,7 +176,7 @@ describe('PortOS update preflight parity — route vs. socket', () => { const { getAppById } = await import('../services/apps.js'); getAppById.mockResolvedValueOnce({ id: 'some-other-app', name: 'Other App', repoPath: '/other' }); getActiveAgentIds.mockReturnValue(['agent-1']); // would refuse a PortOS update - appUpdaterUpdateApp.mockResolvedValue({ success: true, steps: [] }); + appUpdaterUpdateApp.mockResolvedValue({ started: true, completion: Promise.resolve({ success: true, steps: [] }) }); const { fireUpdate, emitted } = makeSocketHarness(); await fireUpdate({ appId: 'some-other-app' }); From 9fe0f0be7d81c100d10d612c73b992b9d4310992 Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Wed, 9 Sep 2026 05:17:16 +0000 Subject: [PATCH 2/2] test: preserve app updater mock contract in preflight checks --- server/services/updatePreflightParity.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/services/updatePreflightParity.test.js b/server/services/updatePreflightParity.test.js index aa82b37154..9e32510038 100644 --- a/server/services/updatePreflightParity.test.js +++ b/server/services/updatePreflightParity.test.js @@ -164,7 +164,7 @@ describe('PortOS update preflight parity — route vs. socket', () => { const ackRouteRes = await request(makeRouteApp()).post('/api/update/execute').send({ acknowledgeFork: true }); expect(ackRouteRes.status).toBe(200); - appUpdaterUpdateApp.mockResolvedValue({ started: true, completion: Promise.resolve({ success: true, steps: [] }) }); + appUpdaterUpdateApp.mockResolvedValue({ success: true, steps: [] }); const { fireUpdate: fireAckUpdate } = makeSocketHarness(); await fireAckUpdate({ appId: PORTOS_APP_ID, acknowledgeFork: true }); expect(appUpdaterUpdateApp).toHaveBeenCalledWith(portosApp, expect.any(Function), { @@ -176,7 +176,7 @@ describe('PortOS update preflight parity — route vs. socket', () => { const { getAppById } = await import('../services/apps.js'); getAppById.mockResolvedValueOnce({ id: 'some-other-app', name: 'Other App', repoPath: '/other' }); getActiveAgentIds.mockReturnValue(['agent-1']); // would refuse a PortOS update - appUpdaterUpdateApp.mockResolvedValue({ started: true, completion: Promise.resolve({ success: true, steps: [] }) }); + appUpdaterUpdateApp.mockResolvedValue({ success: true, steps: [] }); const { fireUpdate, emitted } = makeSocketHarness(); await fireUpdate({ appId: 'some-other-app' });