Address review feedback on port conflict rescue#33
Merged
Conversation
- parsePort: only treat `--port=` (not any `--port`-prefixed flag such as `--port-range=`) as the long-form port, avoiding a wrong-port rescue. - Port-conflict detection now gates the error-log match on recency: a log last modified before the process's current run start (pm_uptime) is stale and ignored, so an old bind error can't prompt killing whatever legitimately holds the port now. - ProcessRowView: key the free-port confirmation to the specific port rather than a Bool, so a poll that changes portConflict mid-confirm can't authorize killing a different port's holder. - DemoData: freePort now transfers port ownership — the holder is stopped and loses the port, the rescued process comes online bound to it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 6 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
The 3s reset task cancels on re-arm, but if it passes its isCancelled check just as a newer confirmation is armed, its queued MainActor.run could still clear the fresh confirmation. Guard the clear on the port the timer armed for so a newer confirmation for a different port stands. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Follow-up to #32 (merged), addressing the four issues cubic raised in review. All four were valid.
Fixes
parsePort— restrict long-form to--port=(PM2Process.swift)hasPrefix("--port")also matched unrelated flags like--port-range=9000, which would parse the wrong port and could trigger the kill/restart rescue against it. Now matches--port=exactly. Added a regression test.Recency gate on conflict detection (
PM2Service.swift/PM2Service+Parsing.swift)Detection claimed to require a recent bind error but didn't enforce it. It now ignores an error log last modified before the process's current run start (
pm_uptime), so a staleEADDRINUSEfrom a previous run can't prompt the user to kill whatever legitimately holds the port now.Confirmation keyed to the port, not a Bool (
ProcessRowView.swift)confirmingFreePortis nowInt?(the port awaiting confirmation). If a poll changesportConflictbetween the two clicks, a stale confirmation can no longer authorize killing a different port's holder.Demo transfers port ownership on rescue (
DemoData.swift)The demo's one-click free previously left the old holder still showing
:3000and the rescued service with no port.freePortnow mirrors the real action: the holder is stopped and loses the port, and the rescued process comes online bound to it.Tests
Full suite green (133 tests), incl. the new
--port-range=regression test.swiftlint --strictclean.🤖 Generated with Claude Code
Summary by cubic
Hardens port-conflict rescue to avoid wrong-port kills and ignore stale bind errors. Aligns the demo and confirmation flow with real behavior, and ensures the confirm timeout can’t clear a newer confirm.
--port=; ignore--port-range=. Adds a regression test.pm_uptime; staleEADDRINUSEis ignored.Int?) and scope the 3s timeout to that port to avoid authorizing or clearing the wrong confirm after a poll.freePortnow transfers port ownership—stop the holder, clear its port, and bring the target online bound to it.Written for commit 3523993. Summary will update on new commits.