Repository navigation
fix(deploy): resend a live deploy only when Harper refused it, never after it may have started - #1898
Draft
dawsontoth wants to merge 2 commits into
Draft
dawsontoth wants to merge 2 commits into
dawsontoth wants to merge 2 commits into
Conversation
…after it may have started When the SSE deploy got no event stream, onDeployComponentSubmit fell back to the buffered deploy_component on any SSEUnsupportedError: a gateway 502/503/504, a 2xx that was not a stream, a dropped connection, or the 120s idle watchdog firing before headers. Harper starts an SSE operation as soon as it accepts it and answers with a 200 stream, so each of those can follow a deploy that is already running, and the fallback deployed the same package a second time (a second install and rolling restart, or two deploys of one project racing). SSEUnsupportedError now carries the response status. The deploy falls back only on a 4xx, which Harper answers for a request it refused before running it (authentication, permissions, validation). Anything else becomes SSEInconclusiveError, which the import, redeploy and activate callers already answer by checking instead of resending. Closes #1889 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
deployComponentStream's contract still told every caller to retry on SSEUnsupportedError, which is the resend this branch removes. Also trims two comments that restated the design note. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates the live (SSE) deployment logic to prevent duplicate deployments. It ensures that a fallback to the buffered POST path only occurs when a 4xx status code is received, proving the server never started the deployment. For other inconclusive errors (such as 5xx gateway errors, dropped connections, or non-stream 2xx responses), the system now throws an SSEInconclusiveError instead of resending. The changes include adding a status property to SSEUnsupportedError, updating the deployment submission flow, adding comprehensive unit tests, and updating the design documentation. I have no feedback to provide as there are no review comments.
dawsontoth
added this pull request to stack #1901
October 11, 2026 21:25
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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.
⊙ Problem
When a live (SSE) deploy didn't get an event stream, Studio sent the same
deploy_componentagain as a buffered POST, even when the first request may already have started the deploy.onDeployComponentSubmitfell back on everySSEUnsupportedError, on the premise that "no deploy has started yet".streamOperationthrows that error for a gateway 502/503/504, for a 2xx that is nottext/event-stream, for a connection that drops after the POST went out, and when the 120s idle watchdog fires before headers arrive. Harper starts an SSE-capable operation as soon as it accepts the request: the SSE branch ofhandlePostRequestsetstext/event-streamand runs the operation in the background (harperserver/serverHelpers/serverHandlers.js:188-194, the same atv5.1.0:149-155), and it writes: stream openstraight away (server/serverHelpers/progressEmitter.ts:79-86). So any of those failures can follow a deploy that is already running. The fallback then deployed the package a second time, with a second install and a second rolling restart, or with two deploys of one project racing. The fallback is a plainfetchwith no interceptors, so #1891's fix to the gateway retry doesn't reach it. Closes #1889.💡 Solution
SSEUnsupportedErrornow carries the response'sstatus. It is undefined when no response arrived, andstreamOperationfills it in. The deploy falls back only on a 4xx, because Harper refuses a request before it starts the operation with a 4xx: authentication fails 401 inauthHandler(serverHandlers.js:139), and body validation andchooseOperation's permission check run before the SSE branch (:170,:177). harperserver/DESIGN.md:231states the contract: such a refusal is answered as a JSON error status, and the stream keeps its in-bandevent: errorat 200. A 4xx from an intermediary (a 413, a 429, a 404 route) is also a refusal before forwarding. Everything else becomesSSEInconclusiveError, with the original error ascause. The import, redeploy and activate callers already handle that by checking the result instead of sending again. A live import or redeploy runs the post-deploy health check. A stage or an activation says it "may not be" done and points at the Deployments list.streamOperationdoesn't make the decision itself. It reports the status and leaves the choice to the caller, becauseget_deploymentis a read and may fall back to polling on anything. Its path is unchanged.⚖️ Alternatives
streamOperationthrowSSEInconclusiveErrorfor the ambiguous cases. Rejected: that would change the transport's contract for every caller, including theget_deploymentread, to fix one write. Whether a resend is safe depends on whether the request is a write, which only the caller knows.text/event-stream, so this response comes from something rewriting it. The issue's direction, treating it like any other inconclusive outcome, is enough to stop the double deploy.get_deploymentbefore resending. That finds the outcome itself rather than inferring it. Rejected for this PR: a deploy whose stream never opened has nodeployment_idto look up, and inconclusive callers already have a check.🔧 Changes
src/integrations/api/instance/applications/deployComponent.ts:refusedBeforeRunning, and a fallback that rethrows anything that isn't a refusal asSSEInconclusiveError.src/integrations/api/sse/errors.ts:SSEUnsupportedError.status, and a doc comment that no longer says every caller should fall back.src/integrations/api/sse/streamOperation.ts: passes the status, and its comments now say a failedfetchmay have sent the request.src/integrations/api/instance/applications/deployComponentStream.ts: the stream's doc comment no longer tells every caller to resend buffered.src/features/instance/applications/DESIGN.md: a new note, "A live deploy is sent once", indexed from the rootDESIGN.md.Overlap with #1841 → #1871 (deploy failure guidance and inline display). #1871 rewrites the import and redeploy callers' error branches, including how they display
SSEInconclusiveError. This PR leaves those callers alone. It only sends themSSEInconclusiveErrorin cases that used to become a second POST. Whichever lands second should check that its inconclusive branch still fits a stream that never opened, as well as one that dropped. #1890's PR (#1897, on #1891) separately makes a buffered write's gateway status read as "outcome unknown". The two don't touch the same lines.✅ Verification
deployComponent.fallback.test.tsrunsonDeployComponentSubmit→deployComponentStream→streamOperationwith onlyfetchand the connection resolver stubbed, and counts the buffered client's POSTs. A gateway 502/503/504, a 2xx JSON response, a dropped connection, and the idle watchdog firing before headers (fake timers, 120s) each rejectSSEInconclusiveErrorand send nothing more. A 401 or 403 still goes out once on the buffered path.deployComponent.test.tsfalls back on 400/401/403/404/406/413/429, and never resends after 500/502/503/504, a 2xx, or no status, keeping the original error ascause. The old "falls back when the stream is unsupported" test asserted the bug, so it now uses a 4xx.streamOperation.test.tschecks that the status is carried, and is undefined whenfetchrejects.claude/1787-stage-and-activate(2d5c9e09c), 14 of the new tests fail with the bug's signature:promise resolved "{ message: 'Successfully deployed: m…' }" instead of rejecting, which is the second POST.vitest runpassed 408 files / 3,855 tests.npx tsc -b,npx oxlint --format stylish .andnpx dprint checkeach exited 0.fetch-level test above is how this was verified end to end.COMMENTS. Codex's one finding is fixed in the second commit:deployComponentStream's doc comment still told every caller to resend buffered onSSEUnsupportedError. Gemini's comment nits are fixed in the same commit. That commit changes only comments, so it was not reviewed again. The Cursor leg failedno-receipt(the 1Password SSH agent refusedgit fetch), and the Harper domain leg failedauth(expired OAuth session), so no adjudication ran. That is an infrastructure gap, not an open finding.🤖 Generated with Claude Code (Claude Opus 5.5); posted via @dawsontoth.
Related PRs: #1876 overlaps (stack base — the deploy code this changes lives there), #1868 overlaps (below #1876 in the chain; shares deployComponent tests and DESIGN.md), #1841 overlaps (heads the #1871 chain, which rewrites the deploy callers' error branches), 2 others independent
Complexity: low
Review-Coverage: authored=claude; ran=codex,gemini; blocked=cursor-composer(no-receipt),domain(auth); declined=cursor-grok,cursor-kimi,cursor-muse; rounds=1; full=1 @ 3e81ae4
Review-Attention: read ~3m (raised: degraded review) @ 3e81ae4