fix: honor explicit --port when reusing a background preview server - #3915
Open
miga-heygen wants to merge 3 commits into
Open
miga-heygen wants to merge 3 commits into
miga-heygen wants to merge 3 commits into
Conversation
startBackgroundPreview never compared a reuse candidate's actual bound port against the caller's explicitly requested --port before returning it, so a second `preview --port <N>` call could silently return an existing server on a different port instead of honoring the request. Reuses the existing PreviewServerPortMismatchError (already used by findPreviewServerForProject for the same discipline) instead of adding a new error type: an explicit --port that doesn't match the reuse candidate now throws a clear conflict rather than substituting the wrong port silently. A bare launch with no --port keeps reusing any project-matching server, unchanged. Deliberately out of scope: the interactive/embedded launch path (runEmbeddedMode -> findPortAndServe) has the same silent-substitution shape when a same-project server is found mid-scan at a port other than the one requested. That path's output isn't the JSON lifecycle schema this bug was reported against, and fixing it would mean touching the interactive dev-server bind/scan loop rather than a simple pre-return check, so it's left as a follow-up candidate. Co-Authored-By: Miguel Angel <miguel.sierra@heygen.com>
`startBackgroundPreview` already refused to reuse a same-project server on
the wrong port when --port was explicit, but the fresh-launch path still
trusted whatever port the detached child bound. The child scans upward from
--port and takes the first free port, so `--background --port N` with N busy
reported `{ type: "started", port: N+1 }` and recorded ownership of it.
- After the child comes up, compare its port with the explicit --port. On a
mismatch, reap the wrapper, wait until the substitute stops answering, and
throw `PreviewPortUnavailableError` (JSON code `preview-port-unavailable`).
If the substitute never stops, fail loudly instead of reporting it.
- Share one `unmetPreferredPort` predicate between the reuse and launch
paths, and one `awaitServerGone` wait between the reap path and
`stopBackgroundPreview`.
- When several same-project servers are running, prefer the one on the
explicit --port as the reuse candidate instead of the lowest port.
- Map launch failures to JSON codes in one place
(`backgroundStartFailureCode`).
Tests: preferred port bound → reported as-is; preferred port taken → reaped
and rejected with both ports, no session record; substitute that keeps
serving → distinct error; bare launch keeps the next free port; explicit
port picks the matching sibling. Deleting the launch-path check, the
candidate ordering, or the post-kill wait each turns its test red.
Co-Authored-By: Miguel Ángel <miguel.sierra@heygen.com>
Contributor
Author
|
Addressed in c86e5f9. |
`startBackgroundPreview` picked its reuse candidate before honouring `--port`, so with an owned server on 3002 and an unmanaged same-project server on 3003, `--port 3003` rejected with "No Studio preview server … on port 3003. Matching server port: 3002" even though 3003 was running. - A policy-matching same-project server already on the explicit port is now the reuse candidate, owned or not; the owned server stays authoritative otherwise, and an explicit GPU-policy change on the requested port still replaces it. - `PreviewServerPortMismatchError` receives every policy-matching same-project server, so the ports it reports are the ones running. - `policyMatchingServers` is the single owner of the same-project + GPU-policy predicate; `matchingServer` derives from it. - `BackgroundPreviewResult` names the launch result union once; `PREVIEW_PORT_MISMATCH_CODE` replaces three literal error codes. Tests: unmanaged sibling on the explicit port is reused without touching the owned server; mismatch error lists every same-project port and no foreign-project port; owned server on the explicit port is replaced on a GPU-policy change. Each new check fails when its guard is removed. Co-Authored-By: Miguel Ángel <miguel.sierra@heygen.com>
Contributor
Author
|
Follow-up in dad0541: reuse the server on the explicit port; truthful mismatch message. |
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
1 participant
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
hyperframes preview --port <N>(background/scriptable launch mode) could silently report a preview server on a different port than the one requested, instead of honoring--portor surfacing a conflict. This happened on both exit paths ofstartBackgroundPreview:reusableExisting) computed from any already-running, project-matching server was returned unconditionally whenever--force-newwasn't passed, with no check against the requested port.findPortAndServe) scans upward from--portand binds the first free port. With N busy, the child came up on N+1 and the launcher reported{ type: "started", port: N+1 }and recorded ownership of it.A sibling function,
findPreviewServerForProject, already guards the reuse case viaPreviewServerPortMismatchError; that discipline just wasn't wired into the background-launch path.Fix
preferredPort?: numbertostartBackgroundPreview's dependencies, set only when the caller passed an explicit--port(via the existinghasExplicitPreviewPort(process.argv)check, previously wired only to the--context/--selectionquery flows).unmetPreferredPort, decides whether a port satisfies the explicit--port; a bare launch has no preferred port and accepts any port, so the common case is unchanged.reuseExistingPreview): a candidate on a different port throws the existingPreviewServerPortMismatchError(reused, not reimplemented). When several same-project servers are running,policyMatchingServerscollects every candidate; the one on the explicit--portwins (onPreferredPort), so--port 3003reuses the 3003 server rather than reporting a mismatch against 3002, and when none matches the error lists all candidates.--portis reaped (wrapper PID, same as the never-ready path), the launcher waits until no same-project server answers on the substitute port, and then throws a newPreviewPortUnavailableErrorcarrying both ports. No session record is written. If the substitute never stops answering, a distinct "did not stop" error is raised instead of reporting success. The wait reusesawaitServerGone, extracted fromstopBackgroundPreview, so both paths share one unreachability loop.preview.tsmaps the launch failure to a JSON error code in one place (backgroundStartFailureCode):preview-port-mismatchfor the reuse conflict,preview-port-unavailablewhen the requested port could not be bound,preview-start-failedotherwise. The plain-text path prints the same message.Deliberately out of scope: the interactive/embedded launch path (
runEmbeddedMode→findPortAndServe) still substitutes the next free port; that path's output isn't the JSON lifecycle schema this bug was reported against, and fixing it means touching the interactive dev-server bind/scan loop rather than a launcher-side check — left as a follow-up candidate. Also unchanged:--force-new --port Nstill tears down the owned preview before launching, so if N turns out to be taken the user ends with no preview and the port-unavailable error; that is the documented meaning of--force-new.Behaviour with an explicit
--port Nreused, port NPreviewServerPortMismatchError/preview-port-mismatchstarted, port NPreviewPortUnavailableError/preview-port-unavailable, no session record--portTest plan
previewLifecycle.test.ts: reuse mismatch rejects withPreviewServerPortMismatchErrorand spawns nothing; matching explicit port reuses normally; explicit port among several same-project servers picks the one on that port; child bound on the explicit port →startedon it with a session record; child bound elsewhere → rejects withPreviewPortUnavailableError { requestedPort, boundPort }, wrapper PID killed exactly once, no session record; reaped child that keeps serving → "did not stop" error; bare launch keeps the next free port.bunx tsc --noEmitclean;oxlint/oxfmt --checkclean on all touched files.preview.test.ts+previewLifecycle.test.ts+studioSelectionClient.test.ts: 91/91 pass.