From 2480ffc6f483bfc064f9a7d4efb3d95a42a60690 Mon Sep 17 00:00:00 2001 From: Adam Eivy Date: Thu, 3 Sep 2026 22:18:30 +0000 Subject: [PATCH] fix: release the update lock when the update launcher rejects (#6036) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit POST /api/update/execute acquires the atomic update lock, then fires executeUpdate without awaiting it. Both of executeUpdate's resolved outcomes clear the lock via recordUpdateResult, but a REJECTION (e.g. spawnDetached throwing on a permissions or missing-binary failure, before any child listener is attached) skipped that path entirely — the route's .catch emitted portos:update:error and left updateInProgress set. The stuck lock wedged the install: every later update answered 409 UPDATE_IN_PROGRESS, and isUpdateInProgress() blocked every CoS agent spawn, until the 30-minute stale timeout aged it out or the server was restarted. The .catch now releases the lock (logging, not swallowing, a failure to release) after the error still reaches the socket channel. Also backfills the execute route's untested branches. makeApp() never attached an `io`, so every socket emission the route makes was a silent no-op no test could see; it now attaches a mock, covering the step, complete and error events, the version fallback when the script reports none, the 409 when the lock is already held, and the 400 INVALID_TAG option-injection guard. Attaching `io` routes error responses through errorEvents, which throws its payload when emitted with no listener, so the suite registers the no-op subscriber the real server always has. Claude-Session: https://claude.ai/code/session_01VjkWVTfzKyRuAv3HEsspwN --- server/routes/update.js | 11 ++- server/routes/update.test.js | 138 ++++++++++++++++++++++++++++++++++- 2 files changed, 146 insertions(+), 3 deletions(-) diff --git a/server/routes/update.js b/server/routes/update.js index 78f5dfec37..5e8c930893 100644 --- a/server/routes/update.js +++ b/server/routes/update.js @@ -235,10 +235,19 @@ router.post('/execute', asyncHandler(async (req, res) => { io.emit('portos:update:error', { message: result.errorMessage ?? 'Update failed', step: result.failedStep ?? 'unknown' }); } } - }).catch(err => { + }).catch(async err => { + console.error(`❌ Update launch failed for ${tag}: ${err.message}`); if (io) { 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 we acquired above 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}`); + }); }); res.json({ started: true, tag }); diff --git a/server/routes/update.test.js b/server/routes/update.test.js index 110ccaa0ab..008b7a2ac3 100644 --- a/server/routes/update.test.js +++ b/server/routes/update.test.js @@ -1,7 +1,7 @@ -import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { describe, it, expect, vi, beforeEach, beforeAll, afterAll } from 'vitest'; import express from 'express'; import { request } from '../lib/testHelper.js'; -import { errorMiddleware } from '../lib/errorHandler.js'; +import { errorMiddleware, errorEvents } from '../lib/errorHandler.js'; // Mock the services the execute route depends on. executeUpdate is fire-and- // forget in the route (not awaited), so a resolved stub is enough. @@ -53,9 +53,22 @@ import { readPersistentMindStateForSafetyCheck } from '../services/cosState.js'; import { filterLiveAgentIds } from '../services/cosAgentLifecycle.js'; import updateRoutes from './update.js'; +// The execute route streams all progress over `req.app.get('io')`; without one +// attached, every socket emission in the route is a silent no-op and untestable. +const mockIo = { emit: vi.fn() }; + +// Attaching `io` also routes every error response through `errorEvents`, and a +// Node EventEmitter THROWS the payload when 'error' is emitted with no listener +// — which would break the error envelope for every non-200 case here. The real +// server always has a subscriber; this no-op stands in for it. +const noopErrorListener = () => {}; +beforeAll(() => { errorEvents.on('error', noopErrorListener); }); +afterAll(() => { errorEvents.off('error', noopErrorListener); }); + const makeApp = () => { const app = express(); app.use(express.json()); + app.set('io', mockIo); app.use('/api/update', updateRoutes); app.use(errorMiddleware); return app; @@ -331,6 +344,127 @@ describe('POST /api/update/execute — active CoS agent gating', () => { }); }); +describe('POST /api/update/execute — lock handling and socket progress', () => { + beforeEach(() => { + vi.clearAllMocks(); + mockSpawningTasks.clear(); + updateChecker.setUpdateInProgress.mockResolvedValue(true); + updateChecker.getUpdateStatus.mockResolvedValue(baseStatus()); + executeUpdate.mockResolvedValue({ success: true, version: '1.27.0' }); + getActiveAgentIds.mockReturnValue([]); + vi.mocked(filterLiveAgentIds).mockImplementation(async (ids) => ids); + mockCosState.persistentMind = { queuedMessages: [], activeTurn: null }; + readPersistentMindStateForSafetyCheck.mockImplementation(async () => ({ + trusted: true, + persistentMind: mockCosState.persistentMind, + })); + }); + + // The atomic check-and-set is the only thing standing between two callers and + // two concurrent `update.sh` runs; ignoring its `false` return would let the + // second one through. + it('409s UPDATE_IN_PROGRESS when the lock is already held', async () => { + updateChecker.setUpdateInProgress.mockResolvedValue(false); + 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(); + // A lost race must not release the lock the winner holds. + expect(updateChecker.setUpdateInProgress).not.toHaveBeenCalledWith(false); + }); + + // The tag is handed to update.sh; anything that isn't a plain semver release + // is an option/argument-injection vector and must never reach the lock. + it('400s INVALID_TAG for a non-semver release tag, without acquiring the lock', async () => { + updateChecker.getUpdateStatus.mockResolvedValue( + baseStatus({ latestRelease: { tag: 'v1.27.0; rm -rf /', version: '1.27.0' } }) + ); + const res = await request(makeApp()).post('/api/update/execute').send({}); + expect(res.status).toBe(400); + expect(res.body.code).toBe('INVALID_TAG'); + expect(updateChecker.setUpdateInProgress).not.toHaveBeenCalled(); + expect(executeUpdate).not.toHaveBeenCalled(); + }); + + // Regression for issue #6036: executeUpdate rejecting (e.g. spawnDetached + // throwing before any child listener is attached) skips recordUpdateResult, + // so the route 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')); + const res = await request(makeApp()).post('/api/update/execute').send({}); + expect(res.status).toBe(200); + await vi.waitFor(() => { + expect(updateChecker.setUpdateInProgress).toHaveBeenCalledWith(false); + }); + expect(mockIo.emit).toHaveBeenCalledWith('portos:update:error', { + message: 'spawn EACCES', + step: 'unknown', + }); + }); + + // 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' }); + await request(makeApp()).post('/api/update/execute').send({}); + await vi.waitFor(() => { + expect(mockIo.emit).toHaveBeenCalledWith('portos:update:complete', { + success: true, + newVersion: '1.28.3', + versionKnown: true, + }); + }); + }); + + // 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 }); + await request(makeApp()).post('/api/update/execute').send({}); + await vi.waitFor(() => { + expect(mockIo.emit).toHaveBeenCalledWith('portos:update:complete', { + success: true, + newVersion: '1.27.0', + versionKnown: false, + }); + }); + }); + + // 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)', + }); + await request(makeApp()).post('/api/update/execute').send({}); + await vi.waitFor(() => { + expect(mockIo.emit).toHaveBeenCalledWith('portos:update:error', { + message: 'Update failed at step "install" (exit code 1)', + step: 'install', + }); + }); + }); + + // The `emit` callback the route hands executeUpdate 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) => { + emit('pull', 'running', 'Pulling latest code...'); + return { success: true, version: '1.27.0' }; + }); + await request(makeApp()).post('/api/update/execute').send({}); + expect(mockIo.emit).toHaveBeenCalledWith('portos:update:step', { + step: 'pull', + status: 'running', + message: 'Pulling latest code...', + timestamp: expect.any(Number), + }); + }); +}); + describe('GET /api/update/status — activeCosAgents', () => { beforeEach(() => { vi.clearAllMocks();