Skip to content

feat: auto-pick a free port in webmux service install - #243

Merged
centdix merged 7 commits into
mainfrom
service-install-auto-port
May 19, 2026
Merged

feat: auto-pick a free port in webmux service install#243
centdix merged 7 commits into
mainfrom
service-install-auto-port

Conversation

@hugocasa

Copy link
Copy Markdown
Contributor

Summary

Second-project webmux service install no longer silently collides on port 5111. When --port is omitted, the install command scans the live instance registry and the already-installed systemd/launchd unit files for webmux to find a port that isn't already claimed, and writes that port into the new unit file.

This builds on the symmetric peer registry from #241: the registry tells us which ports are currently live, the unit-file scan covers installed-but-stopped services that still own their port.

Changes

  • New bin/src/install-ports.ts: pure pickFreePort(start, taken) + thin I/O over readInstalledServicePorts (parses --port N out of ~/.config/systemd/user/webmux-*.service and ~/Library/LaunchAgents/com.webmux.*.plist) and discoverTakenPorts which unions those with createInstanceRegistry().listLive().
  • bin/src/service.ts:
    • Threads a portExplicit flag through the install path.
    • Reinstalls reuse the existing unit's port (parsed back out of the file), so re-running webmux service install is idempotent without re-passing --port.
    • Fresh installs auto-pick when no --port flag is supplied and 5111 / the requested start is already taken.
    • --port always wins, even against a colliding installed unit.
    • Help text updated to call out the auto-pick behavior.
  • 12 unit tests covering pickFreePort boundary cases, both unit-file shapes (systemd, launchd), and the exclude-self-on-reinstall path.

Test plan

  • bun run test — full suite green (backend 350, packages 4, bin 121, frontend 73)
  • tsc --strict over the new bin sources — clean
  • Manual: cd ~/projects/foo && webmux service install then cd ~/projects/bar && webmux service install — second install should land on 5112, both services start cleanly, both visible in the InstanceSwitcher

Generated with Claude Code

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented May 18, 2026

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
webmux aac6f1c Commit Preview URL

Branch Preview URL
May 19 2026, 07:34 AM

@claude

claude Bot commented May 18, 2026

Copy link
Copy Markdown

Claude finished @hugocasa's task in 2m 57s —— View job


Code Review — feat: auto-pick a free port in webmux service install

  • Read PR context and AGENTS.md
  • Review bin/src/install-ports.ts
  • Review bin/src/service.ts
  • Review bin/src/install-ports.test.ts
  • Post review findings

Overall this is a clean, well-factored PR. Pure logic (pickFreePort) is separated from I/O (readInstalledServicePorts, discoverTakenPorts), tests cover boundary cases for both systemd and launchd formats, and the comment block at service.ts:187-192 does what AGENTS.md asks of comments — explain why, not what. Three small things below; none are blocking.

Findings

1. Fragile assumption in UNIT_PORT_RE (install-ports.ts:9)

const UNIT_PORT_RE = /--port[\s\S]{0,40}?(\d{2,5})/;

The 40-char window happens to fit both formats:

  • systemd: --port 5111 (1 char gap)
  • launchd: --port</string>\n <string>5111 (~22 chars gap, depending on indentation)

If generateLaunchdPlist ever gets re-indented (e.g. 4-space → tab, or wrapped deeper), readPortFromUnit could silently start returning null for plists this code itself wrote, and the reinstall-keeps-port behavior would regress to "auto-pick again." Two ways to harden:

  • Tighter regex per format: /<string>--port<\/string>\s*<string>(\d+)<\/string>/ for plist, /--port\s+(\d+)/ for systemd, dispatched by extension.
  • Or at minimum, add a round-trip test: readPortFromUnit(generateLaunchdPlist({...port: 5117})) returns 5117. That catches a future regression immediately.

Fix this →

2. Dead-code guard (install-ports.ts:68)

const port = parseInt(match[1], 10);
return Number.isNaN(port) ? null : port;

match[1] only ever holds a string of digits (the regex is (\d{2,5})), so parseInt(..., 10) cannot return NaN. The Number.isNaN branch is unreachable. Minor — leave it if you like the defensiveness, drop it if you follow CLAUDE.md's "no error handling for scenarios that can't happen" rule strictly.

3. Explicit --port collision is silent (service.ts:197-211)

The PR description correctly notes "--port always wins, even against a colliding installed unit." But when a user runs webmux service install --port 5111 and 5111 is already owned by another installed webmux unit, the install succeeds and the service will then fail to bind at runtime, with the failure showing up only in journalctl/launchctl logs. Consider a non-blocking warning in that case (still install, but tell them what they're walking into):

if (portExplicit) {
  const taken = discoverTakenPorts({ excludeUnitPath: filePath });
  if (taken.has(requestedPort)) {
    p.log.warn(`Port ${requestedPort} is already claimed by another webmux instance. The service will fail to bind on start.`);
  }
}

Fix this →

Nits (take or leave)

  • install-ports.ts:32-47 reads both systemd and launchd dirs on every platform. Harmless (the missing-dir branch short-circuits) but slightly redundant. A platform-based skip would document intent without saving meaningful work.
  • pickFreePort has no upper bound. Pathological input could climb past 65535. Realistically unreachable, just flagging.
  • Reinstall path calls uninstallCommands (stop/disable, no unlinkSync) and then reads the existing port from the same file — that's correct because the file isn't removed until the new Bun.write overwrites it. Worth a one-liner comment in the install function if you want to make that ordering invariant explicit.

Things I checked and was happy with

  • discoverTakenPorts unions listLive() (which already prunes dead PIDs via process.kill(pid, 0)) with unit-file ports, so a crashed peer that left a stale registry entry is correctly not counted as taking its port. Tests at install-ports.test.ts:152-178 use process.pid to guarantee the entry stays "live" during the test — nice trick.
  • excludePath / excludeUnitPath plumbing means reinstall is genuinely idempotent. The dedicated test at install-ports.test.ts:180-193 proves it.
  • bin/backend/ import is consistent with worktree-commands.ts, oneshot.ts, completions.ts. Not a new coupling.
    · branch service-install-auto-port

hugocasa and others added 5 commits May 18, 2026 17:25
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ision

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@hugocasa
hugocasa marked this pull request as ready for review May 18, 2026 17:53
@claude

claude Bot commented May 18, 2026

Copy link
Copy Markdown

Claude finished @hugocasa's task in 3m 2s —— View job


Code Review — feat: auto-pick a free port in webmux service install

  • Read PR context and AGENTS.md
  • Read changed files in detail
  • Cross-reference comments + tests
  • Post review findings

The auto-port logic that was the original target of this PR is in good shape — all three of the prior review's findings landed:

  • UNIT_PORT_RE was split into SYSTEMD_PORT_RE / LAUNCHD_PORT_RE and dispatched on file extension (install-ports.ts:9-10, L68), plus dedicated round-trip tests that feed the generator's output back through the parser (install-ports.test.ts:94-146). A future re-indent in generateLaunchdPlist will now fail loudly.
  • The Number.isNaN dead branch is gone.
  • Explicit --port collisions surface a p.log.warn before the install proceeds (service.ts:262-269).

The newer commits added service-restart.ts (for webmux update) and a port-walking bindServer loop in the backend. A few things I noticed there:

Findings

1. webmuxPath can be empty when which webmux fails (webmux.ts:326-330)

const whichResult = Bun.spawnSync(["which", "webmux"], { stdout: "pipe", stderr: "pipe" });
const webmuxPath = whichResult.success ? whichResult.stdout.toString().trim() : "";

const outcome = updateInstalledService(svc, webmuxPath);

If which webmux fails (uncommon, but possible after a botched bun install --global), webmuxPath is "". That flows straight through parseInstalledServiceConfiggenerateServiceFile and rewrites the unit file with ExecStart= serve --port 5111 / <string></string><string>serve</string>…. The next restart fails silently and the user has a broken service. Worth a guard: when webmuxPath is empty, skip regeneration entirely and just attempt a restart against the existing unit (the binary path inside is still valid).

Fix this →

2. launchd reload can leave the service stopped on partial failure (service-restart.ts:108-112)

run("launchctl", ["unload", service.filePath]);
return run("launchctl", ["load", "-w", service.filePath]);

If unload succeeds and load then fails (bad XML after a regeneration bug, permissions, etc.), the service ends up unloaded and the user has to manually launchctl load. The error reaches the outcome, but the failure mode is "your dashboard is offline" rather than "we couldn't refresh it." This is a real concern given that the regeneration path is on the autopilot webmux update flow. Either probe with a dry parse before unloading, or document the "service may need manual load on failure" outcome in the caller's output line.

3. service-restart.ts uses writeFileSync, not Bun.write (service-restart.ts:1, L137)

bin/src/service.ts:298 uses await Bun.write(filePath, content). The new sibling file uses writeFileSync. Inconsistent within the same feature, and backend/CLAUDE.md is explicit ("Prefer Bun.write() over Node equivalents"). The Bun version is also non-blocking, which matters less here but is a nicer default.

Nits (take or leave)

  • No test for updateInstalledService itself (service-restart.test.ts) — the pure pieces (listInstalledServices, parseInstalledServiceConfig, generate → parse → generate round-trip, restartCommand) are covered, but the orchestrating function that decides regenerate-vs-restart, calls reloadAfterRegenerate, and produces the UpdateOutcome isn't. The empty-webmuxPath bug above would have surfaced as a test. Mocking run/reloadAfterRegenerate would let you assert: "when parse returns null, no write happens"; "when content is unchanged, no reload happens"; "when reload fails, restart isn't attempted." Adding even one or two of these covers the bulk of the orchestration logic.
  • MAX_INCREMENTAL_BIND_ATTEMPTS = 100 is generous (server.ts:1935) — fine in practice, and the OS-picked fallback handles the (unlikely) all-100-taken case cleanly. Just flagging that 100 sequential Bun.serve() attempts on every cold start would noticeably delay startup if you ever did hit it. Not a blocker.
  • service.ts:271 reassigns the config parameter (config = { ...config, port: chosenPort };). Works, but a const installConfig = … next to the existing block reads more clearly given how many config.* references follow.
  • install-ports.ts:33-47 still scans both systemd and launchd dirs on every platform. Carried over from before — the missing-dir branch short-circuits, so it's harmless.

Things I checked and was happy with

  • WEBMUX_PORT_STRICT flows cleanly from CLI → backend (webmux.ts:385server.ts:1936). Service unit files always set Environment=PORT=…, which parseRootArgs treats as explicit, so installed services are correctly strict-bind by default — that's the right policy.
  • parseInstalledServiceConfig taking serviceName from the file basename rather than re-deriving it (service.ts:170-174) preserves the OS-tracked unit name across project renames. The comment block at L153-158 captures why — exactly the kind of comment AGENTS.md asks for.
  • The reinstall path order (uninstall commands → read existing port → write new file) is correct because uninstallCommands deliberately doesn't unlinkSync. Test at install-ports.test.ts:190-205 locks it in.
  • Backend's strict-mode error message specifically points at the fix (drop --port / PORT to let webmux pick a free port) rather than just logging EADDRINUSE — good operator UX.
    · branch service-install-auto-port

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@centdix
centdix merged commit 052f5f9 into main May 19, 2026
2 checks passed
@centdix
centdix deleted the service-install-auto-port branch May 19, 2026 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants