fix: release the update lock when the update launcher rejects (#6036) - #6128
Merged
Conversation
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
POST /api/update/executeacquires the atomic update lock, then firesexecuteUpdatewithout awaiting it. Both ofexecuteUpdate's resolved outcomes clear the lock throughrecordUpdateResult, but a rejection — e.g.spawnDetachedthrowing on a permissions or missing-binary failure, before any child listener is attached — skipped that path entirely. The route's.catchemittedportos:update:errorand leftupdateInProgressset.That stuck lock wedged the whole install: every later update answered
409 UPDATE_IN_PROGRESS, andisUpdateInProgress()blocked every CoS agent spawn, until the 30-minute stale timeout aged it out or the server was restarted.server/routes/update.js— the.catchnow releases the lock. The rejection is not swallowed: it is logged and still reaches the socket error channel first (the HTTP response is long since sent), and a failure of the release itself is logged rather than left as an unhandled rejection.server/routes/update.test.js— backfills the execute route's untested branches.makeApp()never attached anio, so every socket emission the route makes was a silent no-op that no test could observe.New coverage, each naming a distinct regression:
portos:update:error409 UPDATE_IN_PROGRESSwhen the lock is heldfalse, allowing two concurrentupdate.shruns400 INVALID_TAGfor a non-semver tag, lock untouchedupdate.shportos:update:completewith the resolved versionversionKnown: falsefallback to the triggering tagportos:update:errorwith the realfailedStep'unknown'portos:update:stepforwardingOne non-obvious detail worth flagging for review: attaching
ioalso routes every error response througherrorEvents, and a NodeEventEmitterthrows its payload when'error'is emitted with no listener — which broke the error envelope for every non-200 case in the file. The real server always has a subscriber, so the suite registers a no-op stand-in.Test plan
1 failed | 40 passed) and passes with the fix.cd server && npx vitest run routes/update.test.js→41 passed.../scripts,../lib,../autofixer) → 1917 files passed, 38680 tests passed, 0 failed.Closes #6036
https://claude.ai/code/session_01VjkWVTfzKyRuAv3HEsspwN