Repository navigation
fix(cli): scope explicit preview port discovery - #6242
miguel-heygen merged 3 commits into
Conversation
Preview approval requiredThis pull request comes from a fork, so Mintlify won't build a preview until someone with write access approves commit |
miguel-heygen
left a comment
There was a problem hiding this comment.
Thanks for this, and for the careful boundary tests. Reviewed at 7a715b1.
Starting the window at the explicit port and probing the saved port once when it falls outside keeps ownership intact without widening discovery, and the shutdown wait in awaitServerGone now follows the owned server to its saved port instead of rescanning a window that does not contain it. I checked that the tests pin each part: with the original previewLifecycle.ts the seven new cases fail, dropping the addSavedPortServer call fails seven, and sending the shutdown wait back to a plain scan fails the three replacement cases. The new tests call startBackgroundPreview with the start port equal to the preferred port, which is the same shape the preview command passes.
Two optional follow-ups, neither blocking:
--stop --port <n>and--status --port <n>still start their scan at the saved port (preview.ts:319and:338pass no preferred port). A host limited to a port range that runspreview --stop --port 32521after a session saved on 3021 still probes 3021-3120. With a saved record, stop only looks at the saved port (previewLifecycle.ts:667), so it could probe that one port. You scoped this out on purpose, so a follow-up is fine, maybe with a line in the new troubleshooting section saying the scoping applies to starting a preview.awaitServerGoneonly ever checks a single port, so it could always probe that port and drop the new branch and the full rescan on every poll. Three existing tests would need aprobestub alongsidescan.
All CI checks are green at this head.
Verdict: APPROVE
Reasoning: The explicit-port window is now scoped as the issue asks, the saved preview is still found and fully stopped before replacement, every new behaviour is pinned by tests that fail without it, and CI is green.
miguel-heygen
left a comment
There was a problem hiding this comment.
Thanks for picking up both follow-ups, and for going further with --status. Reviewed at 87b05a7.
The delta reads well. With a saved session, --stop probes only the saved port, the shutdown wait checks only the port being stopped, and --status --port uses the requested window plus one probe of an out-of-range saved port. The new preview.test.ts block goes through the real preview command, so the argv-to-preferredPort wiring for --status and --stop is now covered too. I checked that each piece is pinned. Reverting to the previous commit fails the new status and stop cases, dropping the forwarding in preview.ts fails the matching 32521 case, making a session stop scan again fails five tests, and making the shutdown wait return early fails nine. The single-port probe is the same activeServerOnPort the scan uses, so stop and --kill-all keep the OS-confirmed PID. The new troubleshooting paragraph matches the code.
One non-blocking point on --status --port. readBackgroundPreviewStatus reports the first same-project server it finds (previewLifecycle.ts:315). In the requested window that is the lowest port, even when the managed preview is alive on its saved port. Example: the saved session is on 32600 and an unmanaged preview of the same project is on 32530. preview --status --port 32521 then reports port 32530 with that server's PID, next to the saved session's Log: path (previewLifecycle.ts:317-323). A following preview --stop stops 32600, not the server status named. If preferring the requested port is intended (the test at previewLifecycle.test.ts:1043 suggests it is), it might be cleaner to pick the server exactly on the requested port first, then the saved-port server, then any other match, and only attach saved.logPath when the reported server is the saved one. That test can't currently tell "requested port" from "lowest port", because the requested port is also the lowest one in it. A follow-up is fine.
All CI checks are green at this head.
Verdict: APPROVE
Reasoning: The new commit scopes status, stop and shutdown discovery as described, every new behaviour is pinned by a test that fails without it, and the only gap is a status-reporting order question that does not stop or record the wrong server.
miguel-heygen
left a comment
There was a problem hiding this comment.
Thanks for taking the status ordering point all the way. Reviewed at 3e05aec.
--status now picks the server on the requested port first, then the saved preview's port, then any other match for the project, and attaches the saved log only when that server is proven to be the background preview: on the saved port, an OS-confirmed PID, the wrapper's birth token still matching, and the listener being the wrapper or its child. That is the same ownership check --stop already uses, so status and stop now agree on which process is "the" preview. Not borrowing the wrapper's PID when the listener PID is unknown, and not inventing a log path when there is no session, are both good calls.
I checked that the new tests pin each piece. With the previous commit's production code, 14 of the new tests fail. Dropping the requested-port preference fails 4, dropping the saved-port preference fails the "lower sibling" case, and skipping the descendant, birth-token or OS-PID checks each fails its own case. Removing the --list de-duplication for an unknown PID fails the scanned-metadata case, and forcing background mode or the Log: line fails the reporting cases. --kill-all reads only the project and port from these statuses, so the nullable PID never reaches a signal path. The troubleshooting paragraph matches the code. All CI checks are green at this head.
Verdict: APPROVE
Reasoning: The new commit fixes the status ordering and log attribution we raised, reuses the existing ownership proof, touches no stop or record-write path, and every new behaviour fails a test when removed.
An explicit managed-preview port still scanned 100 ports starting at a saved session's port. For example, a saved port of 3021 made
preview --background --port 32521probe 3021–3120 before checking 32521.Startup and
--statusdiscovery now use the explicit port's window and check an out-of-range saved port individually. With a saved session,--stopprobes only that project's owned port; without one, an explicit stop port sets the discovery window. Shutdown waits probe only their target port, and replacements still wait for the owned listener before recording a new session.Status prefers the exact requested port, then the saved port, then another same-project match. If the requested port is absent and an unmanaged preview at 32530 precedes the saved preview at 32600, status reports 32600. It attaches the saved log only after confirming the selected listener belongs to the recorded wrapper. Unverified listeners retain their reported PID, or null when unknown, and do not receive a background-mode claim or a fabricated log path. The guide documents discovery, reporting, and stop behavior.
Closes #6204.
Validation: