Skip to content

Release updateInProgress lock on executeUpdate rejection and backfill missing POST /api/update/execute route test branches #6036

Description

@atomantic

Problem

In server/routes/update.js:175, POST /api/update/execute acquires the atomic update lock via await updateChecker.setUpdateInProgress(true). At line 227–242, it launches the update asynchronously:

executeUpdate(tag, emit, { forceCleanWorkspaces }).then(result => {
  if (io) {
    if (result.success) {
      io.emit('portos:update:complete', { success: true, newVersion: result.version || tag.replace(/^v/, ''), versionKnown: !!result.version });
    } else {
      io.emit('portos:update:error', { message: result.errorMessage ?? 'Update failed', step: result.failedStep ?? 'unknown' });
    }
  }
}).catch(err => {
  if (io) {
    io.emit('portos:update:error', { message: err.message, step: 'unknown' });
  }
});

If executeUpdate rejects (for example, when server/services/updateExecutor.js:98-102 rejects await spawnDetached(...) due to filesystem permissions or missing binaries before child event listeners can be attached), the .catch handler emits portos:update:error but never calls updateChecker.setUpdateInProgress(false).

Contrast this with server/services/appUpdater.js:240-243, which explicitly handles this failure mode:

const outcome = await executeUpdate(version, emit).catch(async (err) => {
  await setUpdateInProgress(false);
  throw err;
});

Additionally, server/routes/update.test.js completely lacks test coverage for:

  1. setUpdateInProgress(true) resolving to false (the 409 UPDATE_IN_PROGRESS guard at line 177).
  2. Invalid release tag formats (the 400 INVALID_TAG guard at line 171).
  3. executeUpdate rejection leading to lock release and socket error emission.
  4. Socket event emissions (portos:update:step, portos:update:complete, portos:update:error) because makeApp() never attaches an io instance to the Express app.

Trigger

A user or automation requests a PortOS update via POST /api/update/execute. spawnDetached throws an error during launch (or executeUpdate fails before child process setup).

Impact

The atomic update lock updateInProgress remains stuck at true in data/update.json, and isUpdateInProgress() stays true in memory:

  • Every subsequent call to POST /api/update/execute returns HTTP 409 UPDATE_IN_PROGRESS.
  • All CoS agent spawns across the entire PortOS installation are blocked by isUpdateInProgress() until the 30-minute stale timeout elapses or the server process is restarted.
  • The failure to release the lock is completely undetected by CI because server/routes/update.test.js only checks that executeUpdate was called and does not verify async settlement.

Fix

  1. In server/routes/update.js:238-243, update the .catch handler to await updateChecker.setUpdateInProgress(false) when handling the error.
  2. In server/routes/update.test.js:
    • Provide an io mock in makeApp() (app.set('io', mockIo)) to record socket emissions.
    • Add a test asserting that when updateChecker.setUpdateInProgress.mockResolvedValue(false), POST /api/update/execute returns 409 UPDATE_IN_PROGRESS.
    • Add a test asserting that invalid tag strings (e.g. "not-semver", "; rm -rf /") return 400 INVALID_TAG without acquiring the lock.
    • Add a test asserting that when executeUpdate rejects, updateChecker.setUpdateInProgress(false) is invoked and io.emit('portos:update:error', ...) is sent.
    • Add tests asserting that successful executeUpdate emits portos:update:complete and result.success === false emits portos:update:error.

Rejected alternative: Releasing the lock inside updateExecutor.js instead of the route was rejected because executeUpdate is a low-level launcher that does not own the pre-lock acquisition performed in update.js.

Acceptance criteria

  • When executeUpdate rejects in server/routes/update.js, updateChecker.setUpdateInProgress(false) is called and portos:update:error is emitted.
  • server/routes/update.test.js tests that POST /api/update/execute returns 409 UPDATE_IN_PROGRESS when the update lock is already held.
  • server/routes/update.test.js tests that POST /api/update/execute returns 400 INVALID_TAG for non-semver tag formats.
  • server/routes/update.test.js verifies portos:update:step, portos:update:complete, and portos:update:error socket emissions via a mock io instance.
  • All tests in server/routes/update.test.js pass with cd server && npm test routes/update.test.js.

Metadata

Metadata

Assignees

No one assigned

    Labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions